Loopback: PUT model/{id} (updateAttributes) implicitly invokes findById & access hook

[shaharz]

I believe this is a similar issue to https://github.com/strongloop/loopback/issues/2064. According to the docs, updateAttributes doesn't trigger the 'access' hook. However, when you PUT model/{id} there is an implicit findById call in model.js that does trigger it. As a result, PUT model/{id} might fail even though they're not looking to access the result, only in update attributes.

In addition, none of the remote hooks have been called at this point so there's no context to the operation hook to help make an intelligent decision as to whether to let the request go through or not.

Relevant stack trace:

at Function.findById (/app/user/node_modules/loopback-datasource-juggler/lib/dao.js:1134:10)
at Function.ModelCtor.sharedCtor (/app/user/node_modules/loopback/lib/model.js:160:19)
at SharedMethod.invoke (/app/user/node_modules/strong-remoting/lib/shared-method.js:252:25)
at HttpContext.invoke (/app/user/node_modules/strong-remoting/lib/http-context.js:384:12)
at restPrototypeMethodHandler (/app/user/node_modules/strong-remoting/lib/rest-adapter.js:427:9)

[superkhau]

Can you provide a sample repo for me to reproduce the issue on my end?

[superkhau]

As a result, PUT model/{id} might fail even though they're not looking to access the result, only in update attributes.

@shaharz What do you mean might fail? If you are not accessing the result, it should be passed through as though you didn't add the hook.

It sounds like you're saying the problem is sending a PUT request triggers the access hook when it shouldn't.

@Amir-61 @bajtos I remember one of you guys working on refactoring the PUT feature. What is the expected behaviour with regard to what hooks get triggered for a PUT request? The docs specify before save, after save, loaded and persisted which seem to make sense to me.

/cc @raymondfeng

[Amir-61]

According to the docs, updateAttributes doesn't trigger the 'access' hook.

That's right!

The docs specify before save, after save, loaded and persisted which seem to make sense to me.

Yes you are right for the code please see here... The following hooks are triggered by this order:'before save', 'persist', 'loaded' and 'after save'...

when you PUT model/{id} there is an implicit findById call in model.js that does trigger it

AFAICT updateAttributes does not call findById...

[shaharz]

I later noticed the following under 'access':

Prototype methods don't trigger the access hook because the hook was already triggered by the method that loaded the model instance from the database.

For example, when you call a prototype method via the REST API, two model calls are made: static findById() (that triggers the "access" hook) and then the prototype method as requested.

So in fact it is documented, but my point regarding the lack of context in the hook still stands.

[superkhau]

So in fact it is documented, but my point regarding the lack of context in the hook still stands.

@shaharz So is this a feature request to get access to context in the op hook then?

[shaharz]

When I say context I don't mean the Loopback context, but the broad sense of the word. Suppose I I have a remote hook beforeRemote('**'), it will fire after the operation hook, if at all. As someone else said on #2064, "from the description of the beforeRemote hook I would expect that that hook is triggered before anything else happens.".

Since that's not the case, the choice is basically to either give up the access hook altogether or use middleware (that is guaranteed to fire before the hook).

If this is still unclear I can put together a minimal case to illustrate the problem.

[superkhau]

If this is still unclear I can put together a minimal case to illustrate the problem.

This would definitely help. See https://github.com/strongloop/loopback/wiki/Reporting-issues#bug-report

[shaharz]

https://github.com/shaharz/loopback-sandbox/tree/implicit-findById

[superkhau]

@shaharz In your example, I can't reproduce it properly. beforeRemote gets called first and access is never called (opposite of what you describe in the readme).

[superkhau]

Actually I could use your example after modifying a few things. However, the result I get is remote hook first, then access, which seems correct. What are you expecting to happen?

[shaharz]

@superkhau I pushed npm-shrinkwrap.json to make sure we're running the same version. Also fixed formatting in the README. What you're saying is correct for PUT /Tests/ (beforeRemote then access) but not for PUT /Tests/1 (access only).

[bajtos]

Hello, as for the "access" hook: the way how this hook is designed, it should be called whenever any model instance(s) are accessed. This way it's possible to implement custom access control by registering "access" hook observer.

We are discussing with @Amir-61 that we should eventually implement a static version of updateAttributes that would not load the instance, i.e. something like MyModel.patchById(id, data). However, such method will still invoke the "access" hook.

When I say context I don't mean the Loopback context, but the broad sense of the word. Suppose I I have a remote hook beforeRemote('**'), it will fire after the operation hook, if at all. As someone else said on #2064, "from the description of the beforeRemote hook I would expect that that hook is triggered before anything else happens.".

Since that's not the case, the choice is basically to either give up the access hook altogether or use middleware (that is guaranteed to fire before the hook).

If I understand your comments posted later, then you are looking for a remoting hook that would be called before sharedCtor is invoked, but that would include all context information for the prototype method. Is that correct assessment?

[shaharz]

Yes, I was expecting the remote hook to be called before sharedCtor. As to whether it's this hook or a different/new one, I defer to your expertise!

[superkhau]

Escalating to feature. Thanks for the input everyone.