Fix #481 and #474

[fabien]

http://docs.strongloop.com/display/public/LB/Operation+hooks will need updates

[raymondfeng]

LGTM. @bajtos PTAL.

[bajtos]

I am very strongly opposed to this change, see #474 (comment) and #481 (comment).

[bajtos]

We can reopen this PR if the discussion in #474 and #481 proves me wrong. In that case this patch requires unit-tests demonstrating how the changes made to ctx.instance are handled, especially when both ctx.data and ctx.instance is changed.

[fabien]

@bajtos I will adapt the unit tests - this is a critical flaw that needed to be addressed, so I disagree completely with your strong rejection.

[bajtos]

@fabien have you read my comments in the other two issues? Your pull request is a hack fixing the wrong thing in my opinion. I really don't want to end up in the same situation as we have with the old model hooks, where it was not possible to fix them and I had to rewrite them from scratch.

Please don't work on this pull request until we come to an agreement on what's the right way how to address your problem, as otherwise you may just waste your time.

In case I did not make myself clear, I truly want to improve the operations hooks to serve your use case. At the same time, I want to find a solution that will not compromise the current design, will not add even more confusion and will not make the future maintenance more difficult.

For example, the question of using "this" vs. "data" is a commonly asked question about the old hooks and I really don't want to introduce the issue back into the new hooks.

[fabien]

@bajtos yes, I read your comments, and I can see what you mean. However, I don't think this PR is moving in the wrong way, it just needs to be fleshed out. Let's go ahead constructively.

Let me remind you that the current working state of operation hooks simply cannot do what the old hooks and events could do - the context currently doesn't always provide the instance that was previously available to work with.

First of all, can we agree on:

Any hook operating on an instance-level should also receive the instance in the context.

I agree, at the moment the PR is incomplete, because it doesn't handle the instance vs. data situation, and besides resolving this, the docs should be clear on this purpose. What about:

If an instance is passed in the context, it should never be used to manipulate data - it is only there to get the current state (for comparison for example). Any changes would go into the data property of the context.

[bajtos]

If an instance is passed in the context, it should never be used to manipulate data - it is only there to get the current state (for comparison for example). Any changes would go into the data property of the context.

This is what I disagree with. One of the ideas behind ORM and OOP in general is that the objects should encapsulate the behaviour and hide implementation details. An example is a property with custom setter that creates a normalised version of the input value, or a prototype method that performs some calculation and updates several properties.

Another example is a code that changes some model properties and then would like to get a value computed from these changed properties.

That's why I believe users should use the instance object to manipulate the data in as many places as possible. Because if they had only the data object, then they would have to build a new instance in order to use these prototype methods.

Also because we perform a validation of the updated model instance, in many cases we would have to 1) extract data from the instance before calling the hooks, 2) pass the data back to the instance for validation. At the moment, we can skip 1) because hooks are manipulating the instance directly.

Any hook operating on an instance-level should also receive the instance in the context.

There are two basic ways how the hook may want to use the instance:

A) It wants to modify the data that will be persisted to the database, this is specific to "before save" hook. We must make it very clear whether the save will update the whole record (ctx.instance ATM) or perform a partial update (ctx.data ATM).

B) It wants to get more information about the model instance affected. IIRC, there are only two cases where we are not exposing this data even though we have it available: prototype.remove and prototype.updateAttributes.

Let's focus at updateAttributes first. For hooks interested in modifying the data (A), the API must make it very clear that there is no instance to manipulate, the hook must update the partial data object instead. Therefore we cannot use ctx.instance to provide the readonly info for (B), because that's inconsistent with the way all operations work, it means one can no longer check if(ctx.instance) to decide whether he has a full instance or partial data for modifications.

When it comes to prototype.remove, there are no restrictions similar to what we have in updateAttributes. However, I am reluctant to use ctx.instance simply because it will be inconsistent with the way how "before save" use ctx.instance.

My conclusion is that we need to come up with a new property name that will be set in the where+data case and that will make it very clear that the changes made to this instance will be ignored by the operation. Perhaps ctx.targetInstance would be a good name?

Thoughts? I'd like to hear @raymondfeng and @ritch opinion too.

[fabien]

@bajtos this is another way to look at it, but it is fine with me, as long as we make a clear distinction.

From what I've learned during refactoring to operation hooks, it is indeed just the prototype.updateAttributes and prototype.remove methods that need a fix.

Say we'd go with targetInstance which we'd only use current read-only state, wouldn't it be better to have targetData, which wouldn't even be an actual instance? Just to be fully clear/safe?

[bajtos]

Say we'd go with targetInstance which we'd only use current read-only state, wouldn't it be better to have targetData, which wouldn't even be an actual instance? Just to be fully clear/safe?

My concern is that converting the instance to data is not free, it takes some time and allocates extra memory. I don't want all users to pay this price.

Can we come up with a better name that would make the intent more clear than targetInstance? Perhaps affectedInstance?

[fabien]

What about currentInstance it denotes a sense of unchanged 'state' better than targetInstance, I think.

[bajtos]

What about currentInstance it denotes a sense of unchanged 'state' better than targetInstance, I think.

Sounds good too. Let's go with currentInstance then and see what @ritch and @raymondfeng think of it.

[fabien]

@bajtos now that PR #485 is here - can I work on top of this (in a separate feature branch) or do I need to go fully independent and fix conflicts later (there will be quite some, because I'll work on the same parts of the code now).

[fabien]

@bajtos will currentInstance only be passed in a before save + updateAttributes scenario? Can we identify where currentInstance comes into play?

Currently, in updateAttributes after save there's an instance being passed. Also, do we want this to be the case with prototype.delete as well?

[fabien]

Closing this in favor of #486