Add new hook 'loaded'

[PradnyaBaviskar]

Connect to #441

List of methods that are triggering the hook: create, updateOrCreate, findOrCreate, find (and friends), updateAttributes.

Caveat: by default, create and updateAttributes do not apply database updates to the model instance returned to the callback, therefore any changes made by "loaded" hooks are discarded. Users can set a per-model option updateOnLoad: true to change this behaviour.

/to @raymondfeng, can you please review?
/cc @bajtos

[bajtos]

If I understand the current implementation correctly, then the new "loaded" hook is basically the same as "after save", except that it is called for "DAO.find" too. It seems to me that such implementation may be less powerful.

The way I have envisioned the loaded hook, it should pass context.data instead of context.instance where the data is the raw data returned by the database (connector) call. Some DAO methods use this data to update the model instance (e.g. set fields auto-generated by the database), in which case a loaded hook can modify these properties before they are set on the model instance.

Using the example of encrypting property values when stored in the database, here is a list of steps & hooks that I would like to be perform on update/create:

1. access hook: no-op in this case
2. before save hook with a model instance: no-op in this case
3. persist hook with data for the connector: encrypt the property value in ctx.data
4. connector call: save the modified data
5. loaded hook with the data from the connector: decrypt the property value in ctx.data
6. DAO updates model instance with data from DB

Having said that, this is only one of many possible solutions and use-cases and it may be suboptimal. @raymondfeng @fabien what's your take on this?

[fabien]

@bajtos @raymondfeng I actually thought - just by the name of it - that this would provide the ability to handle the - raw - data right after it was fetched by the connector. That would have been a great hook to have IMHO.

[bajtos]

I actually thought - just by the name of it - that this would provide the ability to handle the model right after it was fetched by the connector. That would have been a great hook to have IMHO.

@fabien Thank you for chiming in!

My vision is to invoke this hook after the data was fetched by the connector, before we create a model instance from that data. The reason for that is to allow hooks to decrypt data (for example). Is this the same what you were thinking about? If not, then could you please advise a better name that would make this distinction more clear? I.e. to make it more clear that this new hook is called with the raw database data, not a full model instance?

Will the hook as I proposed it work for you, or do you actually need another hook to be called after a model instance is constructed?

[fabien]

@bajtos it's exactly what I had in mind, would you be interested in implementing it?

[bajtos]

@fabien

it's exactly what I had in mind

Good, I am glad we are on the same page here :)

would you be interested in implementing it?

@PradnyaBaviskar will work on the implementation, I'll help her as needed and review the code.

[fabien]

@bajtos @PradnyaBaviskar great! Looking forward to it - I already have some use-cases lined up.

[fabien]

@PradnyaBaviskar @bajtos any progress on this?

[PradnyaBaviskar]

Reworked on the loaded hook, so that it is invoked just after the data is fetched by the connector.
@bajtos - can you please review?

[bajtos]

Yes, this is a problem that affects the "persist" hook too. I am proposing to add a new model setting that will enable this behaviour (updating the created instance using the data returned from the connector). See #565 for an inspiration how to implement such option.

[bajtos]

@PradnyaBaviskar could you please add tests to verify the order in which the hooks are called by individual methods? I think it's important to clarify whether "loaded" is called before or after the "after save" hook.

[PradnyaBaviskar]

Made following changes

1. Since the context of create() does NOT have data updated through persist/ loaded hooks, so, introduced a new Model setting applyHookUpdates. Thereby,

   if(Model.settings.applyHookUpdates) {
     obj = new Model(context.data);
   }
   

   Is this the kind of implementation, that you are looking for? Because, here a new Model instance is created and obj is overwritten by the new instance.

   Or should we be looking out for differences between obj and context.data, and accordingly update obj?

2. Secondly, added tests, to verify the execution sequence of hooks, for each method.

@bajtos, Can you please review?

[bajtos]

@PradnyaBaviskar you are on the right track. I think it's better to call obj.setAttributes(context.data) instead of creating a new Model instance.

Or should we be looking out for differences between obj and context.data, and accordingly update obj?

That would be nice, but I am afraid it will cause too much performance overhead, especially in the case where the data was not changed at all. Let's keep that out of the initial implementation.

I have one more question for you. As I am reading the implementation of updateAttributes, I see that it is triggering the "loaded" hook, but I don't see where the updates made by the "loaded" hooks are applied to the instance?

[bajtos]

@slnode ok to test

[bajtos]

The name applyHookUpdates seems like too broad to me, I think it would be better to come up with a name that would communicate the fact that this setting affects the "loaded" hook only. How about applyLoadUpdates or updateOnLoad?

[PradnyaBaviskar]

Regarding -

I think it would be better to come up with a name that would communicate the fact that this setting affects the "loaded" hook only.

Should this setting be affecting the loaded hook only? Or the persist hook as well? I'm going ahead with the name updateOnLoad, since it relates more closely to the fact that we are updating context.instance in the load hook. Does that sound good?

About your question,

As I am reading the implementation of updateAttributes, I see that it is triggering the "loaded" hook, but I don't see where the updates made by the "loaded" hooks are applied to the instance?

That was a mistake on my part! In the test which asserted that the updates made by loaded hooks are applied to the instance, I had mistakenly called save(), rather than updateAttributes(). After correcting the test case, I realized that, we'll need to use the same setting i.e. Model.settings.updateOnLoad for updateAttributes() as well!

[PradnyaBaviskar]

Changed the following -

1. Renamed setting applyHookUpdates to updateOnLoad
2. And used the Model setting updateOnLoad in updateAttributes as well!

Can you please review?

[bajtos]

This looks like a forgotten code from "loaded" hook that was not removed completely. Could you please remove/revert? I think there should be no changes in the updateAll method.

[bajtos]

I think it would be better to come up with a name that would communicate the fact that this setting affects the "loaded" hook only.

Should this setting be affecting the loaded hook only? Or the persist hook as well? I'm going ahead with the name updateOnLoad, since it relates more closely to the fact that we are updating context.instance in the load hook. Does that sound good?

👍

IMO, it's ok to affect the "loaded" hook only, because that's the hook which is affecting data used to initalize the model instance. The persist hook is manipulating the data stored in the database.

@raymondfeng @fabien This pull request is almost ready to be merged, there are only few minor coding-style issues to fix. I think it would be good if you could take at least a quick look at the solution we have arrived at and let us now if there are any major issues we didn't spot ourselves. If you don't leave any comments by Wednesday morning CEST, I'll go ahead and land this.

[bajtos]

@slnode test please

[fabien]

@bajtos LGTM @PradnyaBaviskar thanks!

[PradnyaBaviskar]

Done with the fixes for coding-style issues.

[bajtos]

Landed, thank you!

[barocsi]

+1 for ctx.data https://docs.strongloop.com/display/public/LB/Operation+hooks#Operationhooks-loaded
and additionally the context has some problems

 User.observe('loaded', function observeLoaded(ctx, next) {
   var loopback = require('loopback');
    var context = loopback.getCurrentContext();
    var user = context.get('currentUser');

According to the docs the ctx.data property should be there. The user exists.
The ctx.instance is there, ctx.data is empty.

In any other cases I can get a hold of the user reference. In this case user is undefined.

[barocsi]

In fact, in any case I add an observer to the User model the above wont work, e.g. I cannot access the currently logged in user. It works with other models.
Added as strongloop/loopback#2004