Injecting Remote Context via Options

[ritch]

UPDATE 2016-12-22 We ended up implementing a different solution as described in #3023.

Tasks

• Make the change in the master branch - see Inject remoting context to options arg #3023 (released in loopback@3.2.0)
• Back-port to 2.x - see Inject remoting context to options arg [2.x] #3048 (released in loopback@2.37.0)
• Update the documentation - see Rework current-context docs loopbackio/loopback.io#227
• Close other context-related issues and point users to this new solution.

Original description for posterity

The example below allows you to inject the remote context object (from strong remoting) into all methods that accept an options argument.

function inject(ctx, next) {
  var options = hasOptions(ctx.method.accepts) && (ctx.args.options || {});
  if(options) {
    options.remoteCtx = ctx;
    ctx.args.options = options;
  }
  next();
}

app.remotes().before('*.*', inject);

app.remotes().before('*.prototype.*', function(ctx, instance, next) {
  inject(ctx, next);
});

// unfortunately this requires us to add the options object
// to the remote method definition
app.remotes().methods().forEach(function(method) {
  if(!hasOptions(method.accepts)) {
    method.accepts.push({
      arg: 'options',
      type: 'object',
      injectCtx: true
    });
  }
});

function hasOptions(accepts) {
  for (var i = 0; i < accepts.length; i++) {
    var argDesc = accepts[i];
    if (argDesc.arg === 'options' && argDesc.injectCtx) {
      return true;
    }
  }
}

Why? So you can use the remote context in remote methods, operation hooks, connector implementation, etc.

MyModel.observe('before save', function(ctx, next) {
  console.log(ctx.options.remoteCtx.accessToken.userId); // current user
});

This approach is specifically designed to allow you to do what is possible with loopback.getCurrentContext() but without the dependency on cls. The injection approach is much simpler and should be quite a bit faster since cls adds some significant overhead.

@bajtos @fabien @raymondfeng

[pulkitsinghal]

@ritch - the initial chunk of code sits in server/server.js correct?

[ritch]

A boot script works fine... The goal is to add this to core

[bajtos]

The mechanism of passing the context object via the options argument looks ok, it's consistent with what we already do for transactions.

However, I dislike the fact that we want to expose the full remoting context to all model methods.

IMO, model methods should be as much transport (remoting-context) agnostic as possible. In my mind, here is the basic promise of LoopBack and strong-remoting: write your model methods once, then invoke them using whatever transport is most appropriate (REST RPC, JSON-RPC, WebSocket, MQTT, etc.). Once users start coupling their method implementation with remoting context details, it will be difficult to switch to a different transport. For example, I believe the websocket transport does not have the concept of req and res objects as provided by express.

We already had a similar discussion when implementing loopback.context() middleware, see #337 (comment) and the following comments. The agreement was to implement a flag (disabled by default) which turns on this new behavior.

I am proposing to do the same here, for example:

// usage - code-first
app.enableRemotingContextInjection();

// usage via loopback-boot
// server/config.json
{
  port: 3000,
  enableRemotingContextInjection: true,
  // etc.
}

// implementation - provided by LoopBack
app.enableRemotingContextInjection = function() {
  app.remotes().before('*.*', inject);

  app.remotes().before('*.prototype.*', function(ctx, instance, next) {
    inject(ctx, next);
  });
  // etc.
};

Thoughts?

[fabien]

I think we should leave users the choice, either this or use the cls approach. Sometimes you need access to the context, possibly even outside of the methods covered above.

[digitalsadhu]

@ritch how would one set the accessToken or a user object?

eg. When I create a middleware after loopback.token() to set things up, how would I get access to ctx.options.remoteCtx to set things?

[digitalsadhu]

nvm, I understand that ctx.options.remoteCtx is an object with req and res etc.

[digitalsadhu]

I'm finding that using the above sample code does not inject the remoteCtx when calling a models methods directly.

Eg. I have a contractor model that hasMany contracts. The Contract.before('access', function () { }) hook is breaking when I try to access a contractor (ctx.options is undefined) but works fine when accessing the contract directly

Example:
In contractor.js

Contractor.afterRemote('**', function (ctx, next) {
  var Contract = Contractor.app.models.Contract
  Contract.find(function (contracts) {
    console.log(contracts)
  })
})

Blows up with a 500 error when accessing contractor via explorer with the message:
TypeError: Cannot read property 'req' of undefined which comes from the Contract.before('access') when trying to access ctx.options.remoteCtx.req which works fine when accessing Contract directly

[digitalsadhu]

Just to summarize the error I'm seeing...

Anytime I access a related model, whether it's using includes in the url or just running a MyRelatedModel.find, the before access hooks in the myRelatedModel instance context.options does not contain remoteCtx.

[bajtos]

Anytime I access a related model, whether it's using includes in the url or just running a MyRelatedModel.find, the before access hooks in the myRelatedModel instance context.options does not contain remoteCtx.

This is a know issue of remoting hooks, see #737. If you are accessing your Contract model via Contractor model, then you need to configure context-injecting hook for Contractor too.

[ritch]

You need to pass the options.remoteCtx yourself in the example you mentioned above.

Contractor.afterRemote('**', function (ctx, next) {
  var Contract = Contractor.app.models.Contract
  var filter = {};
  var options = {remoteCtx: ctx};
  // the example above was also missing the `err` argument
  Contract.find(filter, options, function (err, contracts) {
    console.log(contracts);
  });
});

This highlights a downside of this approach: you must pass the remoteCtx around yourself. The advantage is you get to control when the remoteCtx is set. It is also much less magic (and error prone) than using the cls approach.

[ryedin]

This approach breaks User.login for us. The 3rd argument here: https://github.com/strongloop/loopback/blob/master/common/models/user.js#L169 is now the options arg (containing the remoteCtx object), which results in an error here: https://github.com/strongloop/loopback/blob/master/common/models/user.js#L228

[digitalsadhu]

Thanks @ritch most helpful info. I think i'm almost there. Small problem though:

Via explorer: GET /contracts

Contract.observe('access', function (ctx, next) {
  //ctx has no access to remoting context
  next();
});

I believe this is just the same issue you describe a fix for above except that I'm not sure (since I'm not directly calling Contract.find) how I can inject the remote context via the options object you describe above. My understanding is that if I could directly call:

Contract.find(filter, { remoteCtx: ctx }, function (err, contracts) {  
})

then in:

Contract.observe('access', function (ctx, next) {
  //ctx.options.remoteCtx will be set
});

[bajtos]

@digitalsadhu you need to inject the remoting context into options argument via a remoting hook. See the code in the issue description.

function inject(ctx, next) {
  var options = hasOptions(ctx.method.accepts) && (ctx.args.options || {});
  if(options) {
    options.remoteCtx = ctx;
    ctx.args.options = options;
  }
  next();
}

app.remotes().before('*.*', inject);

app.remotes().before('*.prototype.*', function(ctx, instance, next) {
  inject(ctx, next);
});

This code will inject remoting context to all models and all methods. If you prefer to inject the context only for Contract methods, then you can change the hook spec:

app.remotes().before('Contract.*', inject);

app.remotes().before('Contract.prototype.*', function(ctx, instance, next) {
  inject(ctx, next);
});

[digitalsadhu]

Thanks @bajtos I am doing that in a context.js boot script. Thats working fine in most cases. There are a couple places where its not working:

Contract.observe('access') doesn't get ctx.options.remoteCtx where as Contract.observe('after save') does.

I should also say, I've logged out the injection and it does happen before the access hook is called and all looks well, it just never makes it onto the access hook ctx object

[digitalsadhu]

This works:

Contract.observe('after save', function createHistory(ctx, next) {
    var History = Contract.app.models.History;
    var contract = ctx.instance;

    History.create({
      contractId: contract.id,
      createdBy: ctx.options.remoteCtx.req.user.name,
      snapshot: JSON.stringify(contract)
    }, next);
  });

This doesn't work:

Contract.observe('access', function limitContractStaff(ctx, next) {
  var user = ctx.options.remoteCtx.req.user;
  //ctx.options === {}
  if (user.group === roles.MANAGER)
    ctx.query.where = merge(ctx.query.where || {}, { createdBy: user.name });

  next();
});

[digitalsadhu]

NVM, pretty sure its just an overly complicated model with side effects I wasn't aware of. Sorry for the spam ;)

[bajtos]

@digitalsadhu OK :) OTOH, it is possible that your model method doesn't have options argument and therefore the remoting hook does not inject remoteCtx.

[digitalsadhu]

Nope. I've tracked down the last of my issues and now have a working solution. Every case was just me not explicitly passing {remoteCtx:ctx} to model methods such as find or findById etc. once I had tracked down all of those everything worked.

[digitalsadhu]

@bajtos @ritch

FWIW The reason I've jumped through hoops to get rid of getCurrentContext with this approach is that I've repeatedly run into issues with getCurrentContext returning null with various connectors or platforms.
Eg. windows with mssql connector

The latest I've discovered (and should file a bug) is that if you use the memory connector and give a file to add persistence. The use of async.queue to save changes to the json file seems to be the culprit (I did some investigation and removed the use of async.queue and the issue went away).

I have a feeling you guys are already pretty aware of these types of issues and I suspect its mostly related to cls right?

Anyway, my comment being, I think it be really great to see a simpler approach such as the one provided in this issue used.

Thanks for all the help!

[ritch]

The use of async.queue to save changes to the json file seems to be the culprit (I did some investigation and removed the use of async.queue and the issue went away).

Thanks for digging in this far...

I have a feeling you guys are already pretty aware of these types of issues and I suspect its mostly related to cls right?

Yes - cls is not a dependency I am comfortable with. The only reason we ever considered it was the simplicity in using getCurrentContext()... Your experience with it not reliably returning the context is what I have feared for a while. I would recommend moving away from getCurrentContext() and using my prescribed method. You have to be diligent about passing the remoteCtx around, which as you've seen is a bit tricky... but this is much easier than trying to figure out if getCurrentContext is broken or not every time you see it returning null.

[digitalsadhu]

Tip:
For put requests to update a model eg. PUT model/1
I found I needed to modify my injection script like so:

app.remotes().before('*.prototype.*', function(ctx, instance, next) {
  if (typeof instance === 'function') {
    next = instance
  }
  inject(ctx, next);
});

As for some reason instance is the callback function and next is some other context like object. (Perhaps instance and next are reversed?)

A bit odd but seems to do the trick.

[digitalsadhu]

@ritch @bajtos
I've found some significant performance issues with this approach.

The problem:
Significantly increased wait times (experiencing 20 second TTFB) with multiple simultaneous requests. You don't really notice anything if you just perform 1 request after another (with eg. curl.)

This function is the culprit:

app.remotes().methods().forEach(function(method) {
  if(!hasOptions(method.accepts)) {
    method.accepts.push({
      arg: 'options',
      type: 'object',
      injectCtx: true
    });
  }
});

As soon as that code is commented out, problem goes away.

Any ideas why this might be? Is there anyway I can work around this? (Other than reducing simultaneous queries to the server)

[digitalsadhu]

@ritch @bajtos Ok figured it out. You can't just inject the entire httpcontext. It's just not performant. I am now injecting my user object directly and all is well again.

function inject(ctx, next) {
  var options = hasOptions(ctx.method.accepts) && (ctx.args.options || {});
  if(options) {
    options.user = ctx.req.user;
    ctx.args.options = options;
  }
  next();
}

@ritch One further thing to note is that the use of the hasOptions method in your implementation above results in some remote methods getting 2 options args because if the remote method already had an options object arg that did not have the injectCtx property on it then it is treated as having no options object at all. A new one then gets added and so accepts ends up like:

accepts: [
  {arg: 'options', type: 'object'},
  {arg: 'options', type: 'object', injectCtx: true}
]

Not a huge deal as in practice this only happens for the change-stream methods

[ritch]

I have some ideas for solving the performance issues and keeping a clean separation between remote hooks and impls.

[josieusa]

Hi everyone,
following my comment here on Aug 10, I closed that PR and deleted that branch (in favor of the newer strongloop/loopback-context/pull/11 which works for me and is only waiting for review).
EDIT Please use #2728 for comments about it, and not this, thank you

[bajtos]

Hello, the patch allowing remote methods to receive remoting context via options argument has been landed to both master (3.x) and 2.x.

Remaining tasks:

• Update the documentation at https://loopback.io/doc/en/lb3/Using-current-context.html using the information in Inject remoting context to options arg #3023.
• Close other context-related issues and point users to this new solution.

Please note that I am still intending to land (and release) the solution proposed by @josieusa in strongloop/loopback-context#11 too, as it offers an important alternative to our "official" approach.

[bajtos]

Ouch, I pressed "comment" button too soon. The pull request for 2.x is waiting for CI right now (see #3048).

[ebarault]

@bajtos

as it offers an important alternative to our "official" approach

which do you consider as the target approach ?

[tvdstaaij]

@bajtos When this update is published, is there a possibility it will break 2.x projects using the workaround that already uses options?

[bajtos]

@ebarault

which do you consider as the target approach ?

Propagating context via "options" argument is our recommended and supported approach. While it has downsides (e.g you have to manually add options argument to all your methods and be disciplined in passing them around), it is also the only solution that is 100% reliable (meaning a change in a dependency like a database driver won't break it).

I don't know how reliable the new loopback-context version based on cls-hooked will be in practice. AFAIK, "continuation local storage" in Node.js is still relying on subtle tricks like pre-loading cls-hooked as the first module, monkey-patching certain Node.js API, etc. As a result, there may be edge cases where CLS-based solutions don't work and thus we cannot recommend a CLS-based context propagation as a universal tool for all LoopBack apps.

@tvdstaaij

When this update is published, is there a possibility it will break 2.x projects using the workaround that already uses options?

That's a very good question, indeed. It really depends on how well your workaround handles existing options arguments. AFAICT, the code you pointed to uses injectCtxflag to recognize options arguments already added. Because LoopBack itself does not have any concept of injectCtx, the built-in options argument do not include this flag. As a result, I think you will end up with two options arguments and the application will break, as the second options argument will be passed in as a value for callback function argument.

The main reason for back-porting this new context propagation to 2.x is to allow the recently added access-token invalidation skip the access token of the current user making the request, which I consider as an important bug to be fixed (see #3034).

I am happy to discuss how to minimise the impact of this back-ported change on existing LoopBack 2.x applications. Please move this discussion to #3048.

[bajtos]

📢 I have released this new feature in both 2.x (2.37.0) and 3.x (3.2.0) 📢

[bajtos]

The documentation at http://loopback.io/doc/en/lb3/Using-current-context.html was updated too (please allow few minutes for the changes to propagate to the website).

I am calling this issue finally done 🎉

[Saganus]

This is great work, thanks!

One question though, any recommended path to upgrade from 2.29.1 to 2.37.0?

I mean, are there any specific warnings or issues I should be aware of? Or is it enough to just change the package.json loopback version and then installing it?

I know I have to clean up my code to stop using the previous context code, but is it as simple as removing it before the update, installing the new version, and then using the new features as described in the docs?

Thanks again for solving this!

[bajtos]

@Saganus Yes, upgrading should be as easy as you described:

1. Update LoopBack version
2. Set injectOptionsFromRemoteContext in your model JSON files
3. Rework your methods to use the new context propagation API

However, there may be community-maintained modules that may have issues with the new computed "options" argument, see e.g. mean-expert-official/loopback-sdk-builder#310. I recommend to run your test suite (including client tests) after enabling injectOptionsFromRemoteContext in step 2, just to make sure all layers of your app correctly support computed arguments.

[matt-landers]

On node 8.9.1 with the MongoDB connector, getCurrentContext returns null if it is not set outside of the call to a model like:

req.app.models.User.findById(req.accessToken.userId, function (err: Error, user: any) {
    var lbc = loopbackContext.getCurrentContext({ bind: true });
    if (lbc) {
        lbc.set('currentUser', user);
    }
    next();
});

However, it does work when you move it outside:

var lbc = loopbackContext.getCurrentContext({ bind: true });
req.app.models.User.findById(req.accessToken.userId, function (err: Error, user: any) {
    if (lbc) {
        lbc.set('currentUser', user);
    }
    next();
});

You also must use the bind option or it will return null as well. The documentation on loopback-context will have you pulling your hair out because it will not work as written.

[bajtos]

I have cross-posted the comment above to strongloop/loopback-context#21.

I am going to lock down this issue to collaborators only, please open new issues if there is anything else to discuss.