Inject remoting context to options arg

[bajtos]

Based on the feedback in #2762 and #1676, I would like to discuss a slightly improved proposal for Injecting Remote Context via Options.

Important points I am addressing:

1. The "options" argument must be initialised in such way that HTTP clients cannot inject unwanted properties. E.g. the client must not be able to inject options.accessToken to override the access token set by the authorisation layer.
2. The injected "options" argument must be "hidden" and excluded from the Swagger resource generated by loopback-swagger
3. Developers must be able to configure the "options" argument via model JSON files
4. It should be easy to customise the way how the "options" argument is built, preferably via a mixin. It should be easy to share and reuse such customisation via a loopback component (a standalone npm package).
5. Ideally, multiple mixins can contribute additional context properties.
6. Some context properties (e.g. full currentUser) may require async computation. This use case should be supported too.

Because we are unlikely to get this new API right on the first try, and because experimenting with features is difficult in LoopBack core, I would like to ship a minimal infrastructure that will allow 3rd party components to experiment with different approaches for building the "options" object, as we are already seeing to happen:

• https://github.com/snowyu/loopback-component-remote-ctx.js
• https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a

Solution details

Methods accepting an options argument should declare this argument in their remoting metadata and use a special value optionsFromRequest 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.

Under the hood, this "magic string" value is converted by Model.remoteMethod to a function for strong-remoting, i.e. strong-remoting receives accepts.http as a function, which is already a well-supported use case.

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

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

At runtime, optionsFromRequest invokes a static Model method called createOptionsFromRemotingContext to build the argument value. Subclasses and mixins are free to override this method, e.g. to return a class instance as shown in https://gist.github.com/ebarault/fdc39c5d07cc40f08664256f9e00905a.

In line with my goal of keeping this feature minimal, the built-in method returns options with a single property accessToken:

  Model.createOptionsFromRemotingContext = function(ctx) {
    return {
      accessToken: ctx.req.accessToken,
    };
  };

Extending the "options" object

1. Override createOptionsFromRemotingContext in your Model:

   MyModel.createOptionsFromRemotingContext = function(ctx) {
     var base = this.base.createOptionsFromRemotingContext(ctx);
     return extend(base, {
       currentUserId: base.accessToken && base.accessToken.userId,
     });
   };

2. Use a "beforeRemote" hook

   MyModel.beforeRemote('saveOptions', function(ctx, unused, next) {
     if (!ctx.args.options.accessToken) return next();
     User.findById(ctx.args.options.accessToken.userId, function(err, user) {
       if (err) return next(err);
       ctx.args.options.currentUser = user;
       next();
     });
   });

3. Use a custom strong-remoting phase that's run before any remoting hooks are triggered

     app.remotes().phases
       .addBefore('invoke', 'options-from-request')
       .use(function(ctx, next) {
         if (!ctx.args.options.accessToken) return next();
         User.findById(ctx.args.options.accessToken.userId, function(err, user) {
           if (err) return next(err);
           ctx.args.options.currentUser = user;
           next();
         });
       });

Caveats

Relations methods are defined on "modelFrom", therefore they will invoke createOptionsFromRemotingContext on "modelFrom" too, despite the fact that under the hood a method on "modelTo" is called.

For example, if we have a relation called Category hasMany Product, then a request to /categories/1/products will call Category.createOptionsFromRemotingContext to build an options object that will be eventually passed to Product.find. This is a limitation of the current implementation of scope/relation methods, fixing it is out of scope of this pull request.

Tasks

• Implement optionsFromContext
• Modify PersistedModel methods
• Modify relation methods

Out of scope:

• Propagate options in the authentication layer, see ebarault/loopback-sandbox@09b9a8c

What's next

I would like to get more feedback on my proposal before moving forward.

• @fabien @ebarault @snowyu @doublemarked I hope that the solution proposed here will allow you to simplify the implementation of your custom "options"/"remotingContext" builders, so that you can remove the code injecting the "options" arg and focus on building the "options" arg instead. What's your opinion? Is there anything missing in my proposal, something that you would have to work-around in your mixins/components?
• @raymondfeng @ritch What's your take? I know this proposal is probably not ideal and we may want to implement a different/better solution in long term. However, my intention here is to offer a reasonable solution that LoopBack users can start using now, instead of waiting for a hypothetical unicorn from the future.
• @pulkitsinghal @Diadara @azatoth @digitalsadhu @dancingshell @a-legrand @bostondv @yagobski @nidhhoggr @ryedin @ezraroi @rimvydas-urbonas @Morriz @wprater @eggerdo @hotaru355 @kevinsschmidt @tomhallam @pkgulati @souluniversal @pbtcjasper @denkan @mrfelton @zanemcca @guanbo @pongstr @skyserpent @pkgulati @Saganus @chris-hydra @aorlic @fperks I am pinging you, since you were involved in the discussion at some point in the past. Do you have any opinion on my proposal? (I apologise to everyone who may feel spammed.)

[bajtos]

One more thought: the community was able to come up with a solution for injecting the "options" argument even without support from LoopBack. Does it perhaps mean there is no need to change LoopBack core and we should be promoting community components instead?

I personally think (hope?) that the solution offered in this pull request is better than the two existing alternatives (gist and remote-ctx), but then we need to consider also the costs of maintaining this feature as part of LoopBack core.

[bajtos]

@doublemarked responding to your #2762 (comment) from the earlier pull request:

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?

I am concerned that if we have a single optionsFromContext, and there are multiple independent components that want to customize it, then we may easily end up with a situation where the components are overriding this single function and the behaviour of the application depends on the order in which the components are configured.

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.

I agree having to implement custom "options" properties in two places (in a middleware and then in an options builder) is cumbersome. OTOH, I am rather reluctant to introduce opinionated conventions that may not work for everyone. Instead, I am proposing to leave this outside of LoopBack core and allow 3rd party components to contribute this feature.

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?

IMO, the "options" object should contain things that one may want to pass when calling the method directly too. For example, imagine that your Product model is set up to produce audit logs to trace who made what operation. Then you may to write:

User.login({ email: '[email protected]', password: 'pass' })
  .then((token) => {
    return Product.create({ name: 'Pen' }, { accessToken: token });
  });

That's why I think it's ok to have options.accessToken instead of options.context.accessToken.

OTOH, components and applications are free to come up with different conventions.

[PaddyMann]

"One more thought: the community was able to come up with a solution for injecting the "options" argument even without support from LoopBack. Does it perhaps mean there is no need to change LoopBack core and we should be promoting community components instead?"

No - this is so fundamental for building even basic APIs that a proper solution needs to be part of the core. I, like many others in the discussion, have lost days to understanding and trying to resolve the issue. It came across really badly that this issue was open so long and that no-one on the Loopback team was providing a well document workaround. It makes a huge difference if there is official guidance on how to resolve the issue, vs. a community workaround that may conflict with a future official solution, may not work in all use-cases, and may come with zero support.

We've created our own solution that doesn't directly match your new recommendations and may lead to another future refactor.

Loopback tries to achieve way too much and a lot of it should be removed from the core roadmap and delegated to community efforts. But this is not one of those things - this is the basic stuff that's pretty much required for any API.

Sorry to sound negative - I really appreciate your work and the direction you're providing @bajtos. But this one caused a lot of pain, remained as an open issue for way too long, and from the comments on the thread (which on their own take an hour to get through) clearly caused a lot of others a lot of pain too.

Good luck with the outstanding work to get a proper solution rolled out :)

[ebarault]

to anyone interested in testing @bajtos proposal, i've set up a sandbox here
just pick the token generated for user in logs, open the swagger explorer, and shoot the testInjectedOptions method on model user with the single argument id: 1

@bajtos: testing results so far

• can inject ctx options in custom role resolvers*
• can read ctx options in remote method hooks, injected from previous phases*
• can inject ctx options in remote method hooks*
• can read ctx options in operation hooks, injected from previous phases (built-in remote method)
• can read ctx options in operation hooks, injected from previous phases (custom remote method) **
• can inject ctx options in operation hooks (built-in remote method)
• can inject ctx options in operation hooks (custom remote method) **
• can extend builtin createOptionsFromRemotingContext method

* (custom and built-in remote methods)
** (through ctx propagation from custom remote method to built-in method invocation)

thoughts

1. homogeneous support of ctx.options through the stack: OK 👍 (+ see point 3)
2. as @doublemarked, i'd prefer to avoid duplicating my custom createOptionsFromRemotingContext in every models. If you still want to make it a per model method, I think we should provide a way to override/extend the builtin method globally
3. ctx.options injection at role resolving stage should be supported in built-in role resolvers, and documented for custom role-resolvers (this already works)

[bajtos]

@PaddyMann thank you for a thoughtful comment!

@ebarault This is awesome, I really appreciate you have set up a test app that's demonstrating what you need from this feature.

making the ctx options available in an homogeneous way across the stack is key imo. here we still lack the support in operation hooks.

I am afraid I don't understand. Could you please be more specific? I believe that all operation hooks invoked by built-in data-access methods like find and create store the options argument in ctx.options, where operation hooks can read and modify it.

The first part (reading options) is already tested in the newly added PersistedModel tests, see e.g.

https://github.com/strongloop/loopback/pull/3023/files#diff-3963e5ddedf38e53684571a414bae3e4R207

Product.observe('access', function(ctx, next) {
  actualOptions = ctx.options;
  next();
});

Are you perhaps looking for a slightly different functionality? Could you please provide code snippets showing what would you like to accomplish?

also: as @doublemarked, i'd prefer to avoid duplicating my custom createOptionsFromRemotingContext in every models. If you still want to make it a per model method, I think we should provide a way to override/extend the builtin method globally

Here are few options with the current proposal that I have in my mind:

1. Create a mixin that provides a custom createOptionsFromRemotingContext, apply this mixin to all your models. I guess this does not remove the repetitiveness (i.e. does not address your concern).
2. Create a new base model extending from PersistedModel and use this new model as the base of your application models. (Using inheritance to share functionality - not ideal, but also quite common.)
3. Override Model.createOptionsFromRemotingContext before calling boot(app). I think the third options is very similar to overriding loopback.createOptionsFromRemotingContext if there was such a global function. The problem is that our bootstrapping process (loopback-boot) does not provide hooks for application-specific code to run at the very beginning, before any models are defined. OTOH, when we finally implement mechanism for customising built-in models (e.g. via redefine: true flag described in A better way for customizing built-in models and models from components #397), it would become easy to change Model.createOptionsFromRemotingContext using common/models/model.{json,js} files.

If none of those way is good and easy to use, than I guess loopback can provide both a global and a per-model extension point, i.e. by changing Model.createOptionsFromRemotingContext to call loopback.Model.createOptionsFromRemotingContext by default.

Model.createOptionsFromRemotingContext = function(ctx) {
  return loopback.createOptionsFromRemotingContext(ctx);
};

loopback.createOptionsFromRemotingContext = function(ctx) {
  return {
    accessToken: ctx.req.accessToken,
  };
};

My concern is that if one of your models uses a mixin that modifies only this model's createOptionsFromRemotingContext, and does it in such way that the result of your custom loopback.createOptionsFromRemotingContext is discarded, then you will end up with a situation where it's difficult to figure out why context has only a subset of expected properties and which code did cause the problem.

Thoughts?

[bajtos]

(you have a code typo in you first post: in your createOptionsFromRemotingContext extension example)

Good catch, it took me a while to spot it. I edited the description, I hope the code is fixed now.

[ebarault]

@bajtos : about

making the ctx options available in an homogeneous way across the stack is key imo. here we still lack the support in operation hooks.

ok, i see it now... i was doing my operation hook test in a custom remote method. I just realized that your PR right now does not cover those.
I tested with built-in method find and it indeed works.

Could you confirm your intention is to make the solution available for custom remote method?
In your pr checklist, i only see "Modify relation methods".

about the fix for createOptionsFromRemotingContext extension, i was personally falling back to ctx.req, but yours work too :-)

user.createOptionsFromRemotingContext = function(ctx) {
    return extend(this.base.createOptionsFromRemotingContext(ctx), {
      currentUserId: ctx.req.accessToken && ctx.req.accessToken.userId,
    });
};  

Note: I updated my previous post so to avoid any further confusion.

[bajtos]

Could you confirm your intention is to make the solution available for custom remote method?
In your pr checklist, i only see "Modify relation methods".

@ebarault Yes, the solution will be available for custom remote methods too, as long as the method correctly describes the "options" argument in remoting metadata.

As far as I understand your test app (ebarault/loopback-sandbox@09b9a8c), you would like to see ctx.options in an operation hook triggered by the authentication layer. Right now, Role.isOwner() does not accept options arg and therefore the call to modelClass.findById() is made with no options. This (missing options/context propagation in our ACL layer) is definitely something to address, either in this pull request or in a follow-up one.

Note that your custom method testInjectedOptions does not trigger any hooks itself, therefore I think it's expected the options object was not modified by your custom "loaded" hook.

When I change your custom method to call findById (and thus trigger "loaded" hook), all checks become true.

    user.testInjectedOptions = function(id, ctx) {
      return this.findById(id, {}, ctx).then(function(u) {
        return {
          accessToken: ctx.accessToken,
          user: ctx.currentUser,
          currentUserId: ctx.currentUserId,
         canInjectCtxOptionsInCustomRoleResolver: ctx.canInjectCtxOptionsInCustomRoleResolver || false,
         canReadCtxOptionsInObserveLoaded: ctx.canReadCtxOptionsInObserveLoaded || false,
         canInjectCtxOptionsInObserveLoaded: ctx.canInjectCtxOptionsInObserveLoaded || false,
         canReadCtxOptionsInBeforeRemote: ctx.canReadCtxOptionsInBeforeRemote || false,
        };
      });
    };

I am little bit confused about what kind of remote methods would you like to see supported by this new context propagation. Could you please clarify?

I am adding a new (sub)task to fix context propagation in the authentication layer to my checklist at the top.

[PaddyMann]

"I am little bit confused about what kind of remote methods would you like to see supported by this new context propagation. Could you please clarify?"

I might have misunderstood, but if you're asking about why we'd want context available in the operation hooks, then a classic use-case is setting updatedBy/createdBy when saving. This should be achievable as a simple mixin but isn't unless you pass in the userId.

[ebarault]

@bajtos : so... yes, indeed you're right, i was a bit too hastly in my tests, my custom method was not triggering the operation hook at all, i was misguided by the logs from the first call to user model in the beforeRemoteMethod hook (enriching the ctx.options with currentUser). My bad.

when propagating the ctx inside the custom method as you did with findById, all is good.
I also tested when authorizing the findById method to a custom dynamic role through a custom role resolver : injection in the custom role resolver is also available in operation hooks.

---

As for the business requirements underneath this technical demand: generally speaking once you decide to offload a certain business logic outside the remote method themselves, directly inside model operation hooks, you expect to find similar information in those hooks whatever the remote method at the origin of the operation on the model.

Generally speaking : it can be for write purposes as described by @PaddyMann, such as inserting information on who performed the change on data, it can also be for read purposes such as more precisely filtering the data returned according to the destinating user, which is complementary to access control.

But is there anything more to be done here? your pr seems to solve this requirement. Yes, it's not direct since we need to propagate the ctx from the custom remote method to the built-in method invocation, but do we have another solution without introducing some kind of magic under the hook (though not easily documentable, maintainable, and so on)?

---

About how to make a single definition of a one-for-all createOptionsFromRemotingContext method:
i'd rather prefer option 2:

Create a new base model extending from PersistedModel and use this new model as the base of your application models

as an interim solution, waiting for the more satisfying built-in models customization you refer to in option 3

implement mechanism for customizing built-in models (#397)

---

About support of ctx.options in authorization stage (Model.checkAccess): yes having the option to inject ctx.options args directly at authorization stage in built-in resolvers (static roles, smart roles) would be nice, in a general manner, not only for role $owner.
Note: this can already be done with custom role resolvers and either your pr here, or the solution from community contribution. I already use this kind of injection to get more context on user's authorization in methods or hooks.

• So to keep focussed, we should probably leave aside this discussion to another pr, while keeping in mind the constraints exerted on the present one (it's somehow linked to my related pr on injecting resolved roles in remotingContext)

[bajtos]

@ebarault

So to keep focussed, we should probably leave aside this discussion to another pr

Yes, I came to the same conclusion myself. Let's keep the part "Propagate options in the authentication layer" out of scope of this pull request.

do we have another solution without introducing some kind of magic under the hook (though not easily documentable, maintainable, and so on)?

It looks like there are ways how to get loopback-context to work reasonably well on recent Node versions, see strongloop/loopback-context#11. It's hard to tell whether it will work for all users though.

Besides that, I don't have any better solution to offer right now.

About how to make a single definition of a one-for-all createOptionsFromRemotingContext method:
i'd rather prefer option 2 as an interim solution, waiting for the more satisfying built-in models customization you refer to in option 3

Sounds good!

---

@PaddyMann

I might have misunderstood, but if you're asking about why we'd want context available in the operation hooks, then a classic use-case is setting updatedBy/createdBy when saving. This should be achievable as a simple mixin but isn't unless you pass in the userId.

Thank you for joining the discussion!

I believe my proposal here does support the use case you have described. It would be great if you could write a small sample app as @ebarault did to verify that you can indeed achieve what you need. That way we have a working code that shows your exact requirements.

---

My conclusion is that there are no major issues in my current proposal and we should move forward. I'll give few more days for the community to chime in and work on modifying the relation methods in the meantime.

[PaddyMann]

Based on the above discussion, I'm pretty certain the use-case I describe will work fine. Thanks!

[bajtos]

I added context propagation to relation and scope methods. I have discovered a bug in juggler along the way, loopbackio/loopback-datasource-juggler#1197 is need to make CI of this pull request green.

This patch is ready for final review. @ritch @raymondfeng @superkhau and/or anybody else who is interested, please take a look at the code changes and let me know if there are any changes/improvements to make.

[bajtos]

I added a "Caveat" section to pull request description:

Relations methods are defined on "modelFrom", therefore they will invoke createOptionsFromRemotingContext on "modelFrom" too, despite the fact that under the hood a method on "modelTo" is called.

For example, if we have a relation called Category hasMany Product, then a request to /categories/1/products will call Category.createOptionsFromRemotingContext to build an options object that will be eventually passed to Product.find. This is a limitation of the current implementation of scope/relation methods, fixing it is out of scope of this pull request.

[ebarault]

a request to /categories/1/products will call Category.createOptionsFromRemotingContext to build an options object that will be eventually passed to Product.find

I'm confused, isn't this a good thing that the option object is passed to Product.find when calling /categories/1/products as it - for example - would enable us to control what product.find would return based on ctx.options content ?

[bajtos]

@ebarault

a request to /categories/1/products will call Category.createOptionsFromRemotingContext to build an options object that will be eventually passed to Product.find

I'm confused, isn't this a good thing that the option object is passed to Product.find when calling /categories/1/products as it - for example - would enable us to control what product.find would return based on ctx.options content ?

Ah, sorry if my wording is confusing. What I wanted to say: some users (like me for example 😄 ) may expect that GET /categories/1/products will call Product.createOptionsFromRemotingContext to build the options argument. However, this request calls Category.createOptionsFromRemotingContext instead. I think this may become confusing if each model returns a different options value - I find it surprising that a Product method is receiving an options argument intended for Category methods.

Could you offer any suggestions on how to improve my note to make this more clear?

[rmg]

@slnode test please

[superkhau]

{arg: 'options', type: 'object', http: 'optionsFromRequest'},

[fabien]

@bajtos I've been following along on the sidelines a bit - still haven't had the time to delve into this properly, but I applaud all the work done. Thanks! AFAIK this won't be part of LB v2 LTS?

[bajtos]

@fabien Thank you for the kind works, I hope the version we ended up with will work well for you too!

I am back-porting this patch to LB v2 LTS right now, we need it to fix #3034.

[PaddyMann]

Awesome work - and for the explanation in another thread of why you recommend this approach vs the new cls-hooked method! Have a great Christmas :)

[BramKleinhout]

Thanks for this solution! Although the options argument in the "createChangeStream" method is not used in the default implementation, could this solution also be implemented to the "createChangeStream" method so it can be used there in a custom implementation as well?

[bajtos]

Although the options argument in the "createChangeStream" method is not used in the default implementation, could this solution also be implemented to the "createChangeStream" method so it can be used there in a custom implementation as well?

@BramKleinhout Good catch, would you mind contributing this improvement yourself?

[BramKleinhout]

@bajtos Of course! I created PR #3064.

[bajtos]

@BramKleinhout lovely, thank you 👏

[bajtos]

Released in [email protected].

[PaddyMann]

I've now upgraded and implemented this - thanks again @bajtos

A couple of minor notes on the docs at https://loopback.io/doc/en/lb3/Using-current-context.html#use-a-custom-strong-remoting-phase:

"There are several ways to customize this value: - Override createOptionsFromRemotingContext in your model. - Use a “beforeRemote” hook. - Use a custom strong-remoting phase." should be styled over multiple lines

The final example needs a little tinkering to work - User isn't defined (so needs to be set to app.models.User), and ctx.args.options doesn't exist for every call.

[bajtos]

@PaddyMann awesome, thank you for the feedback! We have already fixed the first docs issue you have described. Could you please send a pull request fixing the final example? See http://loopback.io/doc/en/contrib/

[PaddyMann]

@bajtos Will do. Very short on time currently as on paternity leave but it's on my todo list.

Just hit an edge case which I think isn't supported - custom validators. @horatu355 raised it in #1495

My use case: I need a validator to check that a given Person belongs to a given organizationId. personId is provided in the data, whereas organizationId is extracted from the URL and passed in with my request options.

Am I right in thinking this isn't supported currently? Any recommendations on what to do if not?

[bajtos]

@PaddyMann

Will do. Very short on time currently as on paternity leave but it's on my todo list.

No worries, take your time and enjoy your paternity leave in the first place! Your child is a new-born only once in their live ;-)

@horatu355 raised it in #1495

I was not able to find the comment you are referring to :(

My use case: I need a validator to check that a given Person belongs to a given organizationId. personId is provided in the data, whereas organizationId is extracted from the URL and passed in with my request options.

Am I right in thinking this isn't supported currently?

AFAICT, the method isValid does support an options argument - see https://github.com/strongloop/loopback-datasource-juggler/blob/5a7283386c7dbda1b73ab07690a3e91b08867417/lib/validations.js#L438 and
https://github.com/strongloop/loopback-datasource-juggler/blob/5a7283386c7dbda1b73ab07690a3e91b08867417/lib/validations.js#L585

Also at least some DAO methods, e.g. create(), do pass the options argument to isValid, see https://github.com/strongloop/loopback-datasource-juggler/blob/5a7283386c7dbda1b73ab07690a3e91b08867417/lib/dao.js#L325.

So in theory, options should be passed down all the way to custom validators. If they are not, then it's a bug, most likely somewhere in loopback-datasource-juggler.

Any recommendations on what to do if not?

Create a reproduction case - either using loopback-sandbox per our bug-reporting, or a failing unit-test that can be added to loopback and/or juggler. If you can contribute the fix, then that would be even better of course :)

[PaddyMann]

It looks like a bug and I've done my best to contribute a fix @bajtos - see loopbackio/loopback-datasource-juggler#1229

[barocsi]

in

model.js

  Model.createOptionsFromRemotingContext = function(ctx) {
    return {
      accessToken: ctx.req.accessToken,
    };
  };

only runs once even if there are multiple consecutive calls to different models within one request session.

How can I propagate this ctx to all consecutive Model calls?
Seems like its tied only to the Model that had the initial remotemethod defined...but then I do not get the concept of the options injection.

All built-in method have been already modified to include this new “options” parameter.

Or shall I pass options as an argument for every built in method? I mean I like have hundreds of findby calls for built in methods and I will need to rewrite them to pass the options?

What is the purpose for all of this? Its then not a framework but a hand-made parameter passing game that php session users will just ask: why?

And then after developers migrate their codes to adhere this bad pattern you will find out...ooops we have fixed currentContext so options messing is not needed anymore, roll back guys!

[DaGaMs]

Could I draw your attention to the bug report that I posted above? I have included a test repo and quite a bit of detail. In short, it seems that access control lists using the dynamic $owner role in some cases don't play nice with this new context passing system, resulting in no options array when you use findById, whereas it works with find.

[barocsi]

So its getting very interesting in loopback 3.3.0.
my bdata is just a persistedModel.

 app.models.bdata.findById(bdata_instance.id,cb})

this yields no ctx.options in any observer

 app.models.bdata.findById(bdata_instance.id,{},cb})

this yields an options object
and the best part is

 app.models.bdata.findById(bdata_instance.id,{include: ['partner']},cb})

this yielded an options object as well
And the best part is, the 'access' observer for the bdata model FIRES TWICE when using include.
First time options is null second time its there in ctx.options.

 app.models.bdata.findById(bdata_instance.id,{include: ['partner']},options,cb})

this yielded an options object as well
The 'access' observer for the bdata model FIRES TWICE when using include.
First time options is therel second time its there as well in ctx.options.

but for example

 app.models.bdata.findOne({where:{id:bdata_instance.id},include: ['partner']},{},{},cb})

throws an error as I would expect
Seems like a parameter checking problem for findById.

I think this whole options injection should be better covered and clarified.

[bajtos]

@DaGaMs I see you have already submitted a pull request #3209 to fix the issue #3209 you have reported, thank you for that and let's keep further discussions in those two places.

@barocsi thank you for reporting these problems. Please open a new issue and provide us with full steps how to reproduce the problem (e.g. how are your model relations set up?). It would be even better if you could contribute the changes needed to fix the problems you are encountering, I am happy to help you along the way. See http://loopback.io/doc/en/contrib/code-contrib.html to get started.

[bajtos]

I am going to lock the discussion here. Please open a new issue for any problems related to (or caused by) this patch. Feel free to mention my handle in issue description to bring it into my attention.