Notify observers for embedded relations

[fabien]

Fixes #497 by triggering before/after save and before/after delete hooks on the embedded model instance. Because such instances are usually transient, the expected hooks were not called.

[fabien]

/cc @bajtos @raymondfeng

[raymondfeng]

LGTM

[fabien]

@raymondfeng regarding https://github.com/fabien/loopback-datasource-juggler/blob/feature/embedded-hooks/lib/relation-definition.js#L2258

Would you expect setAttributes to be called before before save or afterwards? I gravitate towards calling it before the before hook (there's an inconsistency I need to fix - see embedsOne - there it's indeed done before the hook).

[fabien]

@bajtos I think I need to review this a bit - obviously, to allow partial updates, setAttributes should be called after before save. And I believe the hook context should include data and currentInstance to be consistent.

[bajtos]

setAttributes should be called after before save. And I believe the hook context should include data and currentInstance to be consistent.

Yes please, send "where+data+currentInstance" to "before save" hook. AFAICT, the method updateById behaves like updateAttributes, thus the context should be similar or the same.

The "after save" hook should have "ctx.instance" though.

[bajtos]

Unrelated bug: the modelTo instance should be validated before save, similarly as EmbedsOne.prototype.create does.

[fabien]

Actually, I think that call is obsolete in EmbedsOne.prototype.create - a custom validation has been setup to handle this from the parent model. I prefer to remove it and let that validation handle it. I'll investigate.

[fabien]

@bajtos there's no where for transient models - it wouldn't make sense. I'll adapt the code and add data.

[bajtos]

there's no where for transient models - it wouldn't make sense. I'll adapt the code and add data.

"updateAttributes" sets ctx.where to { id: this.id } (using the appropriate id property name). That allows developers to use the same code for "updateAttributes" and "updateAll" in their hook. Remember, the current rule is "either ctx.instance or ctx.where + ctx.data". I am not going to push for this too hard, I can live with a hook that provides ctx.currentInstance instead of ctx.where. In the worst case, we can always add ctx.where later.

[bajtos]

I have a question for you: If I read the code correctly, an update of an embedded model triggers hooks on both the embedded model and the owner model. If Product has embedded Image, then "before save" hook is triggered on both Product and Image.

Is this intentional? If not, can you fix that as part of this patch please?

Regardless of the answers to my two questions above, please add unit-tests to verify and describe the expected behaviour.

[fabien]

Yes, this is fully intentional - it's the sole purpose of this PR. Before, we were able to add certain logic (property transforms, like urlify/sluggify, timestamps...) using beforeValidate and because isValid was used, all was well. This stopped working in the advent of the new hooks implementation. Instead of reinventing specific embedded hook events, I prefer to keep the same hook conventions.

[fabien]

The intended order is:

1. before save on embedded model
2. before save on parent model
3. parent model performs save
4. after save on parent model
5. after save on embedded model

I'll add some tests to check this specifically.

[fabien]

@bajtos PTAL

[bajtos]

AFAICT, there are no tests checking the order of hooks invoked when an embedded model is deleted/removed. Can you add some please?

[bajtos]

As I am looking at the new code, it seems to me that I was wrong when I wrote Yes please, send "where+data+currentInstance" to "before save" hook. AFAICT, the method updateById behaves like updateAttributes, thus the context should be similar or the same.

If my understanding is correct, both "prototype.update" and "updateById" fetch the current model instance, apply a partial update using the request data, but then they are making a full save of the whole updated instance. In that case it is ok to use ctx.instance, because that's what will be written to the database.

Assuming that updateById is mapped to "PUT /persons/bajtos/address/home", and that the address model has properties "city" and "country, then the following two parallel requests create a race condition when the result of one request is effectively discarded by the other:

PUT /persons/bajtos/address/home
{ city: "New City" }

PUT /persons/bajtos/address/home
{ country: "New Country" }

[fabien]

@bajtos I think currentInstance is correct, not just instance, because we have an updateAttributes-like situation (partial update) by the use of setAttributes.

The race condition is nothing new - I'd be the first to admit that the implementation is a bit naive. Perhaps we should open a ticket: perform true partial - granular - updates for embedded models?

[fabien]

@bajtos you'd run into the same kind of race condition with referencesMany right now - we're not using $push or anything like that for MongoDB for example.

I have been working with CouchDB for a long time, and since it doesn't support any operators like MongoDB's $set and $push, they only support optimistic locking using a _rev property to check for conflicts. In fact, I adopted the same technique using a mixin I can apply to such models.

[fabien]

@bajtos here's the mixin: https://gist.github.com/fabien/20c73aec683be5392326

[bajtos]

@bajtos I think currentInstance is correct, not just instance, because we have an updateAttributes-like situation (partial update) by the use of setAttributes.

Let me repeat what I wrote while discussing "before save" hooks earlier. Each update operation has two phases that can be "full" or "partial": The first phase interprets request data as a full instance or a partial update. The second phase persists the change to the database and either updates only subset of model properties, or performs a full update (replace). The "before save" hook cares only about the second phase, it does not care whether the initial request data were partial or not.

So for example, prototype.updateAttributes(data) interprets "data" as a partial set of properties to modify in the database:

inst.setAttributes(data);
connector.update(inst.id, data); // Notice that we are not sending a full instance data

On the other hands, the implementation of the embedded methods always updates the full instance in the database:

embeddedModel.setAttributes(data); // treat data as partial, hooks don't care
modelTo.updateAttribute(propertyName, inst);
  // calls under the hood something like
  connector.update(inst.id, { propertyName: inst.toObject() })
  // Notice that we are modifying all data of the embedded instance,
  // not only the changed properties.

The reason why I introduced ctx.data for prototype.updateAttributes was because ctx.instance does not preserve the important information which properties should be persisted (updated) in the database.

In the case of embedded models, we are always updating the whole embedded instance, therefore we don't need to preserve the information which properties should be updated and there is no need to use ctx.data.

Think about it in this way: the current implementation of embedded models is closer to an ideal ORM, where you always have the full ctx.instance available in your hooks, compared to lib/dao.js. It's great that we can use ctx.instance and don't have to put even more burden of dealing with ctx.data to users.

The race condition is nothing new - I'd be the first to admit that the implementation is a bit naive. Perhaps we should open a ticket: perform true partial - granular - updates for embedded models?

The race condition is out of scope of this pull request, I just wanted to point out what is the current behaviour of DAO methods for embedded operations. Feel free to open a new ticket, though I don't think we will have time to work on that ourselves.

[fabien]

@bajtos I can see your point - but if I were to use the same kind of before save hooks, regardless of being a transient/embedded model or a persisted model, I think having data is more consistent. You are of course right, but actually, I think it's more or less an implementation detail. From the outside, you're performing updateAttributes on an object, period.

For example, I might want to filter out some properties from data before it even hits the embedded object. With your implementation, it would prove difficult to do so, no?

[fabien]

@bajtos I can live with your point of view as well, as long as we can move forward with this - there's definitely two ways to look at it.

Correction: I prefer to know what's going to change, and therefor having data and currentInstance still seems best to me.

[bajtos]

Correction: I prefer to know what's going to change, and therefor having data and currentInstance still seems best to me.

And this is big problem: you believe that ctx.data is what is going to change, but that's not necessarily true, it's the whole ctx.instance that is going to be persisted. If ctx.instance does no longer match what's in the database, then you will make more changes than what's described in ctx.data. This is IMO exactly the case where the API makes the user to believe that he understands what's going under the hood and that there won't be any race condition because he is applying a partial update only, but the reality is rather different.

For example, I might want to filter out some properties from data before it even hits the embedded object. With your implementation, it would prove difficult to do so, no?

While I admit this is a valid use-case, "before save" hook is no the right tool for that job.

When the model is not embedded, one can write the following code that will not provide ctx.data to your hook:

Person.findById(id, function(err, person) {
  person.name = 'Freddy';
  person.save(cb); // ctx.instance == person
});

I see that embedded models don't have an equivalent of PersistedModel.save, however you can achieve the same result by modifying the embedded object directly on the parent:

// Assuming Person embedsOne Address
Person.findById(id, function(err, person) {
  person.passport.name = 'Freddy';
  person.save(cb);
});

The last example above will actually not trigger the hooks on the embedded model, which is IMO a bug. Another example where the hooks are not triggered by the current implementation:

Person.create({
 name: 'person-name', passport: { name: 'passport-name' }
}, cb);

Let's figure out first how to trigger the hooks in all cases where an embedded model is modified/deleted and then we can come back to this discussion with more context knowledge at our hands.

One more thing for you to thing through: at the moment, embedded relations don't trigger the "query" hook. Is that ok? Should the hook be triggered?

[bajtos]

Here's the mixin: https://gist.github.com/fabien/20c73aec683be5392326

This is IMO pretty broken. Your mixin works only for a subset of methods and it does not report any error/warning when an unsupported method is called. The revision is not updated in such case and thus the next write may discard changes made via these unsupported methods.

Of course, it's your code, you are free to use it, especially since you are most likley aware of all the ramifications. It's just that I would personally not recommend your mixin as a general-purpose solution for wide LoopBack audience and thus I am reluctant to optimise the hooks for that mixin.

[bajtos]

I have been working with CouchDB for a long time, and since it doesn't support any operators like MongoDB's $set and $push, they only support optimistic locking using a _rev property to check for conflicts. In fact, I adopted the same technique using a mixin I can apply to such models.

The change-tracking and replication implementation provides some sort of an optimistic locking scheme too, see loopback/lib/PersistedModel.js. The current implementation has a trade-off where it goes long way to ensure correctness even at the cost of a lower performance. It would be nice if LoopBack had a built-in versioning scheme that could be used both for your optimistic locking scheme and replication's bulkUpdate. A single model setting that will provide _rev which works for all methods, possibly backed up by connector-specific implementation.

[fabien]

@bajtos yes, the mixin doesn't cover every interaction, but you know what? It's only meant for the limited interaction model that's possible through the remoting API. IMO this is what we should focus on to get the ball rolling, and this should apply for this PR as well.

Juggler is not a true ORM in the sense that everything is in memory all the time, as a single consistent system, so we have to deal with that in many different ways (just look at the complexities the new hooks highlight afterall). There are many ways where things might get into an inconsistent state.

Working with a single instance over various operations is one of them, and this is where your critique regarding situations missing the hook calls comes from I think.

Again, let's optimize for the fire-once way that remoting enforces, and then build from there.

[bajtos]

It's only meant for the limited interaction model that's possible through the remoting API.

I don't agree with that in the context of this pull request. We have already learned that providing an incomplete solution which does provide any indication on when it does not work is a bad thing to do. It creates confusion among LB users, adds more support work for us and even discourages users from using LB at all.

However, as I shown above, you can bypass the hooks implemented by this patch even using vanilla REST API, so you need to rework it even if we agree to stick to REST API methods only.

Let's set this disagreement aside for a while, so that you can focus on implementing a solution that covers all REST API methods, and then we can decide how to move forward. In the worst case, I can finish the patch myself to add support for the remaining use cases.

[fabien]

@bajtos if you are referring to bypassing as being able to overwrite a embedded property directly (through the parent model), then yes, not all methods are covered. However, I actually think that we should filter out these properties by default, so you cannot set them directly, unless a user chooses not to (using some setting/flag).

When this filtering is in place, I don't see any REST API method being left to cover.

[bajtos]

I'll respond to your comments later.

Since #515 was landed, please consider including support for isNewInstance in this patch too.

[bajtos]

if you are referring to bypassing as being able to overwrite a embedded property directly (through the parent model), then yes, not all methods are covered. However, I actually think that we should filter out these properties by default, so you cannot set them directly, unless a user chooses not to (using some setting/flag).

I am not sure how far we can get with filtering out properties pointing to embedded models.

#491 is adding validation of nested models when a parent model is updated if I understand the patch correctly. strongloop/loopback#1196 is asking for creating embedded models through the parent model.

[zelphir]

@bajtos @fabien Hi guys, any news about this PR? Are you planning to add remote hooks for embedded models?

[bajtos]

I am tired of arguing for an implementation that is complete and consistent. As I see it, the current implementation of embedded models have several shortcomings. It should be marked as a "labs" feature, or at least the doc page should list all known issues.

In the light of the above, while this pull request is creating new issues that we will have to fix later, it's not making the situation substantially worse, so I am inclining to let @fabien land it as it is now. Again, we should document the new known issues and edge cases, so that users are aware of the dangers waiting for them when using embedded models.

@fabien @raymondfeng thoughts?

The list of issues compiled from the discussion above:

• Possible bug: async validations are not supported, see Notify observers for embedded relations #516 (comment).
• Possible bug: operations hook are not triggered when the embedded model is manipulated via parent methods, see the second part of Notify observers for embedded relations #516 (comment) and Notify observers for embedded relations #516 (comment).
• Inconsistency: in the case of ctx.data, there is no ctx.where, only ctx.currentInstance. See Notify observers for embedded relations #516 (comment) and Notify observers for embedded relations #516 (comment).
• Inconsistency: regular hooks provide ctx.data when the database record is updated partially, while embedded hooks are providing ctx.data even when the database record is replaced as whole. IMO a consistent solution would be to pass ctx.instance in that case - see Notify observers for embedded relations #516 (comment).
• ctx.isNewInstance is not implemented yet.

[bajtos]

In the light of the above, while this pull request is creating new issues that we will have to fix later, it's not making the situation substantially worse, so I am inclining to let @fabien land it as it is now. Again, we should document the new known issues and edge cases, so that users are aware of the dangers waiting for them when using embedded models.

@fabien @raymondfeng ping, what's your opinion here? The patch is more than 6 months old and does not cleanly merge with the current master, should we close it as abandoned?

[ShaharHD]

Is there an ETA on when a decision will be taken?

[meleksomai]

Any progress on this issue?

[chrisamoore]

@bajtos @raymondfeng This should be addressed.

[bajtos]

I am afraid this patch looks abandoned. @protactinium @chrisamoore @ShaharHD would you like to pick this work up and get it to the state where it can be landed?

[ShaharHD]

I have to share our personal experience about this issue.

In the past two months we've been in discussions with IBM about using the StrongLoop Standard and Professional licenses.
This current pull request is the one I keep referencing every time the discussion comes up - as a critical issue which have been in limbo for over a year now.

The IBM sales main argument has been something like: "IBM is taking ownership over StrongLoop (and a new thing called API Connect) and will own the product, and as a paying customer we will have the full force of IBM fixing issues just like this".

When my answer have always been that StrongLoop and API Connect are not LoopBack.
I kept emphasizing that I don't care additional features and profiling tools or nicer UI to StrongLoop ARC WebUI while the backend (LoopBack) is broken.
And in any case, most of the advance stuff you use day to day can't be done via the ARC UI in any case.

Moving forward, @bajtos asking the community to assist on this issue ...
which makes me doubt about the need for licensing: why do I need to pay IBM for a license if I need my developers to do the actual work?

@protactinium @chrisamoore
I was updated by IBM that this is has not been a priority issue (licensed users) - do you know if there are StrongLoop licensed clients affected by this issue?

[bajtos]

I have landed these new hooks via #904 and #911, see #497 (comment) for more details.

@zelphir @protactinium @chrisamoore @ShaharHD could you please try out the new hooks and let us know how they work for you? If you run into any problems, then please open new issue(s) and mention my github handle in the description.

[ShaharHD]

@bajtos are those fixes were pushed to the npm package of juggler or we need to reference a specific commit in our package.json?

[bajtos]

@ShaharHD I haven't released a new version, can you npm install strongloop/loopback-datasource-juggler#2.x for testing? It would be great to get some feedback before releasing to npmjs.org, that way we can make fix the newly added code before it's released.

[alexthewilde]

@bajtos thanks for making operation hooks work with embedded models!