Inject remoting context to options arg

[bajtos]

In this pull request, I am proposing a solution for Injecting Remote Context via Options:

Methods accepting an options argument should declare this argument in their remoting metadata and use a special method loopback.optionsFromContext as the http mapping. This way there is no magic happening in beforeRemote hooks and method authors are in full control of how the options argument is (or is not) initialized.

An example using trimmed-down metadata of PersistedModel.create:

PersistedModel.remoteMethod('create', {
  accepts: [
    { arg: 'data', type: 'object', model: typeName, http: { source: 'body' } },
    { arg: 'options', type: 'object', http: loopback.optionsFromContext },
  ],
  returns: { arg: 'data', type: typeName, root: true },
  http: { verb: 'post', path: '/' },
});

The method optionsFromContext returns an options object with the following properties:

• remotingContext - the full context as provided by strong-remoting, typically an instance of HttpContext
• accessToken - the access token parsed from the request or null
• currentUserId - the id of the user making the request, null for anonymous requests

An example operation observer accessing the current user:

MyModel.observe('before save', function(ctx, next) {
  console.log('User %s is about to save some changes.', ctx.options.currentUserId);
});

Tasks

• Implement optionsFromContext
• Add API docs
• Modify PersistedModel methods
• Modify relation methods

Requires strongloop/strong-remoting#355 to prevent timeout failures in unit-tests.

Connect to #1495

@ritch please review

[ritch]

Will this be handled correctly by the swagger generation side of things? Is there something we can add here to hint that this argument is not an API parameter?

[bajtos]

Yes, see https://github.com/strongloop/loopback-swagger/blob/3b609025641f3cc9a832b92f61d15a420f385b99/lib/specgen/route-helper.js#L75-L80

    // Filter out parameters that are generated from the incoming request,
    // or generated by functions that use those resources.
    accepts = accepts.filter(function(arg) {
      if (!arg.http) return true;
      // Don't show derived arguments.
      if (typeof arg.http === 'function') return false;
      // Don't show arguments set to the incoming http request.
      // Please note that body needs to be shown, such as User.create().
      if (arg.http.source === 'req' ||
        arg.http.source === 'res' ||
        arg.http.source === 'context') {
        return false;
      }
      return true;
    });

[ritch]

It would be great if ctx.options was extensible through a model or mixin. Eg. being able to implement the constructor of the options object.

[guanbo]

+1, minxin is great. Actually, most of my scenarios need currentUser with full property, not only currentUserId. someone maybe another.

[ritch]

Looks good so far, but I'd like to see the docs before final sign off.

[doublemarked]

Small complaint - I realize that the docs claim it's unimplemented, however we've been happily declaring our remote methods inside the Model JSON files for some time and I am aware of other projects doing the same. We would run into problems/annoyances here w/ the need to specify a function handle for http.

Why not expand the vocabulary slightly, allowing http.source to contain a value of options? This would avoid the function handle but also might feel more consistent with the other accepted source values like req, res, and (undocumented) header.

[doublemarked]

Another thought - what about middleware access to options? Perhaps the options object itself should be constructed by a piece of middleware and added to req, thus allowing other middleware to work with it before remoting? Could be a more standard approach to controlling how options are initialized.

[snowyu]

The optionsFromContext variable should be removed and use loopback.optionsFromContext directly.
The user could customize the loopback,optionsFromContext via replacing this.

[bajtos]

Thank you all for great feedback!

It would be great if ctx.options was extensible through a model or mixin. Eg. being able to implement the constructor of the options object.

we've been happily declaring our remote methods inside the Model JSON files for some time and I am aware of other projects doing the same. We would run into problems/annoyances here w/ the need to specify a function handle for http.

What if the http function was setup in such way that it calls a static method on the model? E.g. MyModel.createOptionsFromContext. The current implementation of loopback.optionsFromContext would be moved to Model.createOptionsFromContext to serve as the default implementation.

It should make it easier to declare the http mapping via a string name only, because now we have a "registry" where to look for the function implementation (the model constructor).

Why not expand the vocabulary slightly, allowing http.source to contain a value of options? This would avoid the function handle but also might feel more consistent with the other accepted source values like req, res, and (undocumented) header.

In my view, there is no such concept as options at the strong-remoting level. I think we need to implement a more generic solution in strong-remoting level, some sort of a registry of named functions that be used to resolve custom http mappings.

I'll think about this more.

Another thought - what about middleware access to options? Perhaps the options object itself should be constructed by a piece of middleware and added to req, thus allowing other middleware to work with it before remoting? Could be a more standard approach to controlling how options are initialized.

Right now, middleware operates at much lower level of abstraction. It can access req and res only, there is not even a remoting context created yet. I think that's the right way.

One can pass data from middleware to options this way:

• The middleware sets the data on the req object, for example loopback#token sets req.accessToken.
• The function building the options object copies this data from req to options, as we already do for accessToken.

+1, minxin is great. Actually, most of my scenarios need currentUser with full property, not only currentUserId. someone maybe another.

Setting full currentUser instead of currentUserId is a bit tricky, as it requires an async call to retrieve the user. The code building arguments in strong-remoting is sync and I am not sure if it's a good idea to introduce asynchronicity there (I guess my main concern is performance).

However, once we allow customization of optionsFromContext, then one can create a middleware to fetch the current user to req.currentUser, allowing optionsFromContext to stay sync.

[doublemarked]

What if the http function was setup in such way that it calls a static method on the model?

I like that it solves my problem. It feels a little weird for optionsFromContext to be associated with models. Is there a meaningful reason for there to be per-model customization of optionsFromContext?

In my view, there is no such concept as options at the strong-remoting level.

Ok, yeah I can see what you mean.

I think we need to implement a more generic solution in strong-remoting level, some sort of a registry of named functions that be used to resolve custom http mappings.

This sounds appealing and somehow more "whole" of a solution.

Right now, middleware operates at much lower level of abstraction. It can access req and res only, there is not even a remoting context created yet. I think that's the right way.

I agree with you that that's the right way. However, middleware already has access to the full request context (req). optionsFromContext is actually just dumping all of that into options, bubbling a few properties up to the top of the object for convenience. It does not seem so far off that we might early in the request pipeline create a structure for indicating which properties we consider worth bubbling up, and then allow middleware to add to it. This is similar to what the old loopback.context middleware did.

One can pass data from middleware to options this way: [...]
The function building the options object copies this data from req to options, as we already do for accessToken.

What feels wrong here is that this requires us to build a customized optionsFromContext that is aware of the behavior of middleware that wants to provide context to application code. Enabling middleware that wants to inform application code (imagine taking a certain header and transforming it via a 3rd-party service or lib) will require both configuration in middleware.json and code that improves the options building function, not to mention the actual application code.

A final question/thought - is it perhaps wrong for optionsFromContext to add to the top-level key space of options? Perhaps it should be dumping those things in options.context or something?

[bajtos]

Closing in favour of #3023