Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RequestManager] 'body' should be a valid key in the request #8507

Closed
Jopie01 opened this issue Mar 28, 2023 · 3 comments · Fixed by #8511
Closed

[RequestManager] 'body' should be a valid key in the request #8507

Jopie01 opened this issue Mar 28, 2023 · 3 comments · Fixed by #8511
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@Jopie01
Copy link

Jopie01 commented Mar 28, 2023

I'm trying to get the requestManager working but walked straight into the problem that the body key is not a valid key. Below the very basic code.

// service/jsonrpc.js
import RequestManager from '@ember-data/request';
import { Fetch } from '@ember-data/request/fetch';
import config from '../config/environment';

export default class extends RequestManager {
  constructor(args) {
    super(args);
    this.use([Fetch]);
  }

  buildHeader(headers) {
    headers.append(
      'Content-type', 'application/json'
    );
    return headers;
  }

  async makeRequest(method, params, headers, useDatabase=true, context = {}) {
    let host = '';
    if (config.apiHost) {
      host = config.apiHost.replace(/\/$/, ''); // remove the last slash
    }

    if (useDatabase === true) {
      host += '/' + config.apiDatabase + '/';
    }

    let askHeaders = new Headers();
    this.buildHeader(askHeaders);

    const response = await this.request({
      url: host,
      headers: askHeaders,
      method: 'POST',
      body: JSON.stringify({ method: method, params: params})  // using `data` here sends the request without the body
    });
    // do something if needed
    return response;
  }
}

// route/application.js
import Route from '@ember/routing/route';
import { service } from '@ember/service';

export default class ApplicationRoute extends Route {
  @service jsonrpc;

  model() {
    this.jsonrpc.makeRequest(
      'common.server.version',
      [],
      null,
      false,
      {}
    );
  }
}

data is a valid key that is somehow not send to the backend. Also https://developer.mozilla.org/en-US/docs/Web/API/Request names body as a valid key. Adding a hack to make the body to be a valid key, everything works and I get the response back with the version of the backend.

@runspired
Copy link
Contributor

runspired commented Mar 29, 2023

btw, while this "works" and is within your right to do (as extending this class and making it a service is fine as long as the public contract for request is not altered), this isn't the pattern I'd encourage:

  async makeRequest(method, params, headers, useDatabase=true, context = {}) {
    let host = '';
    if (config.apiHost) {
      host = config.apiHost.replace(/\/$/, ''); // remove the last slash
    }

    if (useDatabase === true) {
      host += '/' + config.apiDatabase + '/';
    }

    let askHeaders = new Headers();
    this.buildHeader(askHeaders);

    const response = await this.request({
      url: host,
      headers: askHeaders,
      method: 'POST',
      body: JSON.stringify({ method: method, params: params})  // using `data` here sends the request without the body
    });
    // do something if needed
    return response;
  }

you'll get better long-term utility out of a handler + util

const JSONRPCHandler = {
  request(context, next) {
     if (context.request.op !== 'jsonrpc') return next(context.request);
  
     const headers = context.request.headers?.clone() || new Headers();
     headers.append('Content-type', 'application/json');
     const body = JSON.stringify(context.request.data);
     
     return next(Object.assign({}, context.request, { headers, body });
  }
}

function buildJSONRPCRequest({ method, params, headers }, useDatabase=true) {
  let host = '';
  if (config.apiHost) {
    host = config.apiHost.replace(/\/$/, ''); // remove the last slash
  }

  if (useDatabase === true) {
    host += '/' + config.apiDatabase + '/';
  }
  return { url: host, op: 'jsonrpc', data: { method, params }, headers};
}

then use it like this

requestManager.request(
  buildJSONRPCRequest({ method: 'some method', params: {} })
);

The general guidance here is this:

  • let handlers handle universal things
  • let utils handle request-specific things
  • generate the url as close to the source of the request as possible (vs in a handler)
  • if using caching either the url should be the key or associate a stable key to it via some other mechanism (you can utilize EmberData's caching even without using EmberData's presentation logic by issuing the request via store.request instead of via requestManager.request)

There's a few limitations of the current state of canary that definitely in the way here, small things to tidy/fix up, but generally speaking this is the approach we're encouraging (and where our debug asserts are too aggressive right now and in the way we need to work on tuning them).

@Jopie01
Copy link
Author

Jopie01 commented Mar 29, 2023

Thanks a lot for the guidance. I'm mostly writing Python code and do server-side things, so much appreciated!

@Jopie01
Copy link
Author

Jopie01 commented Mar 29, 2023

  • if using caching either the url should be the key or associate a stable key to it via some other mechanism (you can utilize EmberData's caching even without using EmberData's presentation logic by issuing the request via store.request instead of via requestManager.request)

What I discovered is that when using store.request goes through the Cache but store.requestManager.request bypasses it. So at the moment calling this.store.findAll('model') in the route modelhook does two things for me:

  1. using the store.requestManager.request returns the payload as-is
  2. using the store.request returns an error because I have a JSON-RPC payload which does not fit into a JSONAPI Cache.

For each request to the backend I generate a unique UID key (I'm going to use UUID) so that can be used as key in the Cache.

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants