Deprecate getCurrentContext()

[ritch]

Well its been fun. And by fun I mean: not so fun. We've tried to create a simple method for getting at contextual information no matter where your code is executing. We did this by depending on a sophisticated module called continuation-local-storage .

Continuation-local storage works like thread-local storage in threaded programming, but is based on chains of Node-style callbacks instead of threads.

Someday there will be a more reliable mechanism than the userland cls module, and we can re-add first class support for getCurrentContext(). But until then I am proposing we deprecate this method with the standard warning message.

What does this mean for users of getCurrentContext()? You will get an annoying message (configurable) every time you use the method. We will still support it, and we won't break semver. But we strongly encourage you to refactor.

For an immediate work around, take a look at #1495. A more sustainable workaround will be added as a feature to the framework.

/cc @bajtos @raymondfeng @superkhau @piscisaureus

[fabien]

I'm concerned about this, as I have been using getCurrentContext extensively. And without any issues thusfar. I'm on mobile right now, so I don't have access to the whole discussion(s) - IIRC mainly regarding unreliable availability on different platforms - but what's the reasoning here?

Can we have a flag to silence the deprecation warning? ;-)

[csvan]

I abandoned getCurrentContext pretty quickly since it led to unpredictable behavior. Notably, retrieving the Access Token using something like getCurrentContext().get('accessToken') seemed to work at random, if at all, so we worked around it by getting the accessToken from the request instead. Apart from that, we have not needed it anywhere.

[bajtos]

I was thinking about the problems of getCurrentContext we are experiencing. It seems to me that most of the issues are caused by the fact that any connector using connection pool must be CLS aware and include few lines of code to switch to the right CLS context. I am wondering we can add this small piece into loopback-datasource-juggler, so that we don't depend on connector implementation anymore?

An example to illustrate my point:

DAO.create = function(data, options, cb) {
  // setup instance, call hooks, etc.
  clsCtx = // save current CLS context;
  connector.create(/*args*/, function(err, result) {
    // restore CLS context from clsCtx
    // continue as before
  });
};

If that's not a viable approach, then I support @fabien's request for a feature flag. This flag can have three states:

• undefined - print a warning (default)
• true - getCurrentContext is enabled (no warning is printed)
• false - getCurrentContext is disabled (no warning is printed)

Thoughts?

[raymondfeng]

@bajtos Can you or somebody to do a quick PoC to verify your theory?

I'm willing to advocate explicit context passing as the preferred way as it is the only reliable/performant way. It's fine with me to keep the getCurrentContext() if @bajtos's idea works.

[ritch]

I don't think fixing the connector boundary and lack of CLS support in the connectors is the only issue. I've seen issues from our usage of async (which were eventually fixed).

I'm OK with a flag too. Opt-in and with a bit of warning in the documentation. As far as regular API - I still think we should deprecate it.

This flag can have three states:

The states LGTM.

[fabien]

+1 for flag, please

[sachinhub]

loopback.getCurrentContext() has given lot of hard time while developing unit test cases so far. We had to setup dummy context before executing tests.

However today we faced one major issue where loopback.getCurrentContext () is returned as null when a async callback function gets called.

We are making an external HTTP call from within loopback API in async manner. loopback.getCurrnetContext () is consistently coming as null in the callback made by that http request. The loopback api call is returned by that time.

Is there any way to completely avoid loopback.getCurrentContext() and still make context information available everywhere without explicitly passing it around?

[digitalsadhu]

Don't think so.
You can get the context everywhere except in Model.observes callbacks. Our
preference has been to avoid doing anything that needs context in
Model.observes callbacks and get by without getCurrentContext or passing
context around. We have found that we can usually get by in this way,
sometimes using beforeRemote/afterRemote as a substitute.

On Wed, 23 Dec 2015 at 17:42, Sachin Mane notifications@github.com wrote:

loopback.getCurrentContext() has given lot of hard time while developing
unit test cases so far. We had to setup dummy context before executing
tests.

However today we faced one major issue where loopback.getCurrentContext ()
is returned as null when a async callback function gets called.

We are making an external HTTP call from within loopback API in async
manner. loopback.getCurrnetContext () is consistently coming as null in the
callback made by that http request. The loopback api call is returned by
that time.

Is there any way to completely avoid loopback.getCurrentContext() and
still make context information available everywhere without explicitly
passing it around?

—
Reply to this email directly or view it on GitHub
#1676 (comment)
.

[violet-day]

Any progress about cls?

[bajtos]

Another bug in node-continuation-local-storage:

It looks like setImmediate breaks cls, therefore any modules using setImmediate are going to break current-context feature. An example: express-session, see strongloop/loopback-context#6

[dancingshell]

I also was playing around with a solution. I would love someone from Strongloop team or the community to chime in about the roleResolver AccessConext and whether this is a viable direction. It's a work in progress and I haven't tested all edge cases including remote methods.

https://gist.github.com/dancingshell/26b8101e0620b27244d63b71f2b8f55a

[fabien]

@dancingshell I think I've seen you mention this solution before, but somehow it slipped my mind while churning away on things.

Your solution seems to work pretty well for your use-case, as a workaround it's perfect. However, I think it's less suited as a general solution. Sometimes, READ access will also need the current context/user, but it looks like that could be supported as well.

My main concern is that in the case of a static method, you still need to hijack one of the args to pass on the context - data as in the specific create case - whereas options should probably be considered as the more generalized argument for this.

In fact, I think most of what this 'roleResolver' does, can also be done in a remote before hook, since you'd have access to ctx.instance there as well (in the case of proto methods), or ctx.args to do what you need. I did not verify this yet, but it would mean that you don't need to fiddle around with (the order of) ACLs to get this to work.

One thing that came to mind is a more barebones/integrated CLS-like solution, where strong-remoting would guarantee that the callback that is passed into the actual remote method call(stack) - including the operation hooks - had a reference to the remoting context attached to it, extractable like so:

Model.myMethod = function(id, callback) {
   var ctx = callback.remotingContext;
   ...
});

I don't know if I'm making any sense here, but I'd love to hear some opinions on this.

[ritch]

@fabien thanks for posting your gist. Do you think we should replace my original solution with a link to your gist? I know you took care to make it a general solution, and it looks much more complete than what I suggested a while ago.

I hope we can prevent similar deprecations of (once) core features of the framework, and find a stable solution to this concept after all.

I think argument injection is the only bullet proof solution to this problem. It is not perfect either, but it is predictable. This is what I'd prefer:

MyModel.myMethod = function(params, cb) {
  // params.userId => injected by user code
  // params.foo => mapped by system
};

MyModel.remoteMethod('myMethod', {
  params: { // alternative behavior to 'accepts'
    userId: {
      type: 'number',
      required: true,
      http: function(ctx) {
        return ctx.user.id;
      }
    },
    foo: {type: 'number'}
  }
});

Which solves a couple of things. One being the argument position issue you ran into when having to add the options param. It also allows us to map more directly to swagger's param specification.

[raymondfeng]

The other approach is to introduce request-scoped model repositories (one instance per request) so that a repository instance itself can carry context information as its state. Please note that LoopBack models are singletons as of today and they are tied with attached datasources.

With model repository, LoopBack runtime can create a new instance (or acquire an instance from a pool) per request:

Conceptually, model repository = request context + model definition + attached data source(s)

var modelRepository = new ModelRepository(model, dataSources, ctx);
modelRepository.find(filter, options, ...)

To navigate over relations, modelRepository.orders will return a function backed by an instance of model repository for Order that shares the same context.

[fabien]

@ritch if you have a chance to try my gist first, then yes, please link to it. I am using this code as part of a larger codebase, so some things might be different if you run it on plain LB.

I really like your use of having a single params argument, it's much more consistent! It will surely break BC all over the place though, although I think we can mitigate this for remoting calls by allowing either params or accepts and handle it appropriately. So the upgrade path for custom methods that still use the multi-arg method signatures is less rough to begin with.

@raymondfeng I'm not sure I follow you when you say models are singletons as of today - does this mean that prototype methods will not be handled anymore from now on? If so, I feel another huge wave of BC breaks coming up 👎 . Is this part of v.3?

[doublemarked]

@raymondfeng do I understand correctly that essentially model.js files would then get executed per-request, to ensure that the model handle used by all methods is the one tied to the context-linked repository? And thus, all hooks (etc etc) get initialized per request as well.

[a-legrand]

@fabien I don't get it,
how do you get the remote context from a before save hook for example ?
I just need to access the user id it in
Mymodel.observe('before save', function(ctx, next) {
[...need user id...]
next();
});

tx for you reply, I don't know loopback a lot, but I think your gist can save me :)

[bajtos]

Thank you @fabien for posting a detailed comment about your experience and taking the time to extract your code into a gist that can be used by general audience. The solution you proposed looks reasonable to me at high level 👍

I especially like the ability to configure options-injection at model setting level, i.e. by editing Model JSON file.

What I personally/subjectively don't like that much, is a "big" remotingContext instance containing many properties. The way how I see design of LoopBack apps, most (remote) model methods should be still callable directly from code, without going through strong-remoting's REST layer. Now my concern is that with a big remotingContext, it will be difficult to figure out which context properties are required by a method one wants to call direclty, and which properties can be omitted.

I'd also like to correct few points you made in your comment.

before, options was considered private/not exposed to remoting. I found a number of uses for options that you would not want to simply expose, yet the proposed workaround forces you. In short, you have to be careful, and by default user/remoting supplied options should be ignored.

The solution proposed in my patch #2762 uses custom http mapping to ensure the options object is always constructed by our code and no client-provided values can sneak in.

Swagger/Explorer signatures are polluted with a useless options argument.

Because the http mapping is a function, Swagger/Explorer skips the options argument when describing a remote method.

[bajtos]

I think argument injection is the only bullet proof solution to this problem.

MyModel.myMethod = function(params, cb) {
  // params.userId => injected by user code
  // params.foo => mapped by system
};

As I mentioned in my previous comment, I personally want remote methods to be regular javascript functions that can be called without strong-remoting too. Using params object instead of regular function arguments means that some ES6 constructs like default parameters won't work, it may be more difficult (if possible at all) for IDE/tooling to understand this convention and provide good auto-completion (intellisense).

The other approach is to introduce request-scoped model repositories (one instance per request) so that a repository instance itself can carry context information as its state. Please note that LoopBack models are singletons as of today and they are tied with attached datasources.

This is a very similar concept to what I proposed for Controllers. The proposal is unfortunately buried in our internal repository (https://github.com/strongloop-internal/scrum-loopback/issues/614) , below is an excerpt:

• The controller code does not depend directly on anything from LB/strong-remoting, this makes it easy to package it in a shared npmjs module. (If it depended on LB, e.g. the base Controller or Model class, then the shared package could end up depending on a different version/copy of LoopBack than the LoopBack copy installed for the top-level project/app.)
• The controller code is a self-contained piece that can be analysed by IDEs, run in isolation by unit-tests, etc.
• LoopBack entities are injected via dependency injection at runtime. As I am envisioning it now, a new throw-away Controller instance should be created for each request. That way we don't have to worry about the order in which dependencies are constructed (models, datasources, boot scripts, etc), because they are all already available by the time we have received a request.
• We no longer distinguish between static and prototype methods, as we do in current model/sharedClass based approach. Everything that is exposed in the public API must be a prototype method on a controller.
• Controllers are sitting between REST API and Models, therefore they can access the full remoting context.

Let me illustrate this on an example from apiary's Example Poll API. The API defines an HTTP endpoint "Vote on a Choice" exposed at POST /questions/:question_id/choices/:choice_id, which can be used to record a vote.

What I would like to see, is a controller that defines "Vote on a Choice" method and tells strong-remoting's REST adapter to use POST /questions/:question_id/choices/:choice_id.

// server/controllers/question-ctrl.js
// I'm using ES6 class syntax and Promises,
// but this can be rewritten in ES5 with callbacks too.
module.exports = class QuestionController {
  constructor(app, remotingCtx) {
    this.Question = app.models.Question;
    this.Choice = app.models.Choice;
    this.ctx = remotingCtx;
  }

  vote(questionId, choiceId) {
    return this.Question.findById(questionId)
      .then(function(question) {
        return question.choices.findOne({ where: { choiceId: choiceId } } ;
      })
      .then(function(choice) {
        // TODO check that choice exists
        choice.count++;
        return choice.save();
     });
  }
};

Now let's add some remoting metadata using LoopBack JSON files similar to what we have here, the code here is a mockup

// server/controllers/question-ctrl.json
{
  "methods": {
    "vote": {
      "accepts": [
        { "arg": "question_id", ... },
        { "arg": "choice_id", ... },
      ],
      // ...
      "http": { "verb": "post", "path": "/questions/:question_id/choices/:choice_id" }
    }
  }
}

[bajtos]

@fabien I'm not sure I follow you when you say models are singletons as of today - does this mean that prototype methods will not be handled anymore from now on? If so, I feel another huge wave of BC breaks coming up 👎 . Is this part of v.3?

I cannot speak for @raymondfeng, but here is what I think he meant: Right now, you have only one global entity for each model, which is the model constructor. When you invoke a static method like Product.find, there is no per-request "instance" to which we could attach additional per-request data.

Compare this with my proposal for Controllers above, where we would create new controller instance for each request, and thus we would be able to attach extra data to this per-request controller instance.

As for the second part of your comment, we don't have any plans to stop supporting prototype model methods, or at least I am personally not aware of any. A change like this is definitely out of scope of v3. LoopBack 3.0 is in a release-candidate mode right now (see the announcement blog post) and we don't anticipate any new breaking changes, with the exception of dropping support for Node v0.10/v0.12 as discussed in #2807.

[raymondfeng]

@bajtos Please note ES6 supports destructuring for parameters, for example:

// ES6
function myFunc({name = 'Default user', age = 'N/A'}) {
}

@doublemarked @bajtos There are different ways to have isolated instances for the model per request. We can rebuild it per request (which is expensive). It also be done by something like:

var ProductForMyRequest = class extends Product() {};

The idea is to move away from singletons if needed so that we can carry states for such objects per request.

[bostondv]

You could also implement the optional extractRemotingOptions method or use the inject context hook notification to further filter or enhance the options being passed.

@fabien Would you be able to share an example of how to do this? I'm trying to extend the context in a boot script to include the current user object. Thanks!

[yagobski]

@fabien how can i get the current connected user inside a model using your solution. Can you please share an example. Do you have any other solution to get current user in models.

Thanks

[ebarault]

@fabien, thanks for sharing this, it seems we are a number of folks out there currently building custom solutions to solve context-passing related issues...

I like the general idea of your solution (RemoteCtx setters & getters, user-tempering proofness, possibility to setup in model .json) although after testing it the main cons i'd point out that would not make it a generic enough solution imo (some of these are probably out of your code reach, and more to impute to loopback inner logics) would be that the remotingContext object is not consistent in the various middleware stages with regard to the ctx object of that stage, for example:

• role-resolving (Role.registerResolver(name, function (roleName,ctx, cb) {})) : the remotingContext available in ctx object at that stage is inconsistent with the remotingContext from your solution. This is by construction since your custom remotingContext object is then attached after, via a before-remote hook, as ctx.remotingContext.args.options.remotingContext: we should find a solution that enables the user to inject some params in ctx.remotingContext during role-resolving that are available at later stages in a generic way. Exemple: if i attach my own params to ctx.remotingContext.args.options, they are not overwritten by your solution, but obviously they are not available through the RemoteCtx.prototype.get getter right out the box. I can think of a mapping function using the contextProperties array you provide as a starting point, but if we want to avoid it becoming even more messy, we should probably specify where user should inject this type of data at this stage.
• before-remote hook (Model.beforeRemote ('findById', function (ctx, instance, cb) {})): remotingContext is not available in ctx object (given that remoteObject.before hooks take place after Model.beforeRemote hook, see my solution in next post
• after-remote hook (Model.afterRemote ('findById', function (ctx, instance, cb) {})): remotingContext is available as ctx.remoteContext
• operation hooks: (Model.observe ('access', function (ctx, cb) {}) )remotingContext is available as ctx.options.remoteContext

Maybe we could we think of a generic nested ctx object format with generic getters/setters methods (as you provided, great) that the user could customize to its tastes by setting any property required?
Also: do we have a way to inject the RemoteCtx class at a sooner stage than before remote? this would help solving the issues with param injection at role resolving stage.

++ We could also argue the concept of method and contextProperties whitelist, vs. blacklists, as things may break with new loopback releases (for exemple new method names in loopback 3)

ps. really smart the optional per-model extractRemotingOptions method

[ebarault]

Please, see my revised version of @fabien 's gist.
https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a

in a nutshell

• it fixes a few bits to make it work in a more code-base agnostic env
• it solves the issue with ctx.remotingContext not being available in Model.beforeRemote hooks
• it leverages on a data object in http context to pass data from earlier stages (such as role-resolving) to the new remotingContext object available in further hooks and in remote method code

[bostondv]

Hey @ebarault, thanks for the gist! However, my Model.beforeRemote hooks still are being executed before the context beforeRemote hook. I have the same problem with @fabien's gist as well.

I've got your gist in a boot script and it's being run before all other boot scripts. The Model.beforeRemote is in the model's javascript definition file under common/models/MyModel.js

Any idea how to get the context hook running first?

[ebarault]

@bostondv : so yes, I've no clean way to do this right now (there might be a better way, but i took the shortest track for my project to move on). So here it is

in the bootscript (cf. gist), i register the beforeRemote for ctx injection on ** :

    for (let key in app.models) {
        app.models[key].beforeRemote('**', function (ctx, instance, next) {
            inject(ctx, next);
        });
    }

now, you need to know that the ** beforeRemote hooks are actually executed after all per-method beforeRemote registered hooks. So first, you need to register a similar ** hook in your Models if you want to get a chance for it to be executed after the ctx injection hook.

secondly, you still need to make sure that the actual registration of the ** hook in your model is done after the ctx injection one, otherwise your hook might still be executed before the ctx injection one. Depending on your application it can be done in a number of ways with emitted events so you wait for the models to be fully loaded before registering your beforeRemote hooks. Usually you can get it done with:

app.on('started', function () {
    myModel.beforeRemote('**', function (ctx, instance, next) {
        next();
    });
});

if you don't do this the result can be random as the user defined Models are loaded in lexicographical order

Remember: you can still discriminate the method called in the beforeRemote('**',...) hook by inspecting the method name with the ctx.mehod.name property

[bostondv]

@ebarault thanks so much, that's exactly what I needed!

[ebarault]

all: some archeology to excavate an old debate: #1362 around the ctx propagation question

[nidhhoggr]

I was having issues with remote context a while back, especially after wrapping a remote method call in a third party library callback (particularly jsdom). I have been trying to find some practical working example of this aside from the examples that have been provided in this thread. Is it safe to say that https://github.com/snowyu/loopback-component-remote-ctx.js should pretty much address this issue? If anyone knows of a non-coffee script package, references would be greatly appreciated.

[ebarault]

@nidhhoggr : i just went through the code from @snowyu component. It is essentially a port to coffee script of one of the general idea developed in this thread (and this one) about passing the context as an argument injected in the methods definitions (options argument) : so yes it should work, though the method to access the remoteCtx may not be consistent whether you access it from methods code, or from remote methods hooks or from operation hooks

If you want a pure node implem which is also making sure the context is available the same way in methods definition, remote methods hooks and operation hooks, please see my revision of @fabien's optionnated implementation here

[nidhhoggr]

@ebarault: I can confirm @snowyu implementation is working and your snippet does give me a better understanding of what is happening with method injection etc. I see that your RemoteContext has a prototype method for setting the access token. Ideally would this be set in some middleware? The legacy implementation using getCurrentContext looked something like this:

app.use(loopback.context());
app.use(loopback.token());
app.use(function (req, res, next) {
  var accessToken = req.query.access_token || false;
  if (!accessToken) accessToken = req.headers.authorization || false;
  if(accessToken) {
    var loopbackContext = loopback.getCurrentContext();
    if (loopbackContext) {
      console.log(accessToken);
      loopbackContext.set('accessToken', accessToken);
    }
  }
  return next();
});

So my question, is there a way to set the accessToken of the remote context that is injected into each of the method with middleware as the previous implementation is doing above, or is it best to just access the accesstoken from the context request headers within that method?

[ebarault]

@nidhhoggr : there's actually a method for getting the accessToken, not for setting it.

RemoteCtx.prototype.getAccessToken = function () {
	return this.req && this.req.accessToken;
};

the accessToken is always available in the remotingContext, in the req object, so you can always get it from the helper remoteCtx.getAccessToken() provided in the snippet

[nidhhoggr]

@ebarault Okay I see now, so it's being set from the items in contextProperties array. Thanks you Sir!

[bajtos]

Hello, the official implementation of "options" injection has been landed via #3023 (3.x) and #3048 (2.x, behind a feature flag, disabled by default for backwards compatibility) and released in 2.37.0 and 3.2.0. The documentation will be updated soon, see ttps://github.com/loopbackio/loopback.io/pull/227.

I am closing this issue as done.