Loopback-datasource-juggler: Implement static patchById and replaceById methods in PersistedModel

[Nagarjuna-S]

The associated connector method for updating the existing record(PUT /model/{id}) is “updateAttributes” method. How ever I noticed that when ever we invoke PUT with ID operation, 'all' method is invoked first and on successful response of all method control is going to 'updateAttributes' method. I am thinking this is intentional. I have few concerns here.

1. There are many endpoints charged based on number api's called. As we are invoking internally two endpoint api calls(one in all method and another in updateAttributes method) for single operation, number of endpoint api's used will be doubled for Update operation(PUT with ID). Users and connector developers may surprise on number of calls.
2. Response time will be more which is crucial in API world.

As we are expecting the connector developer to handle updating the existing record in updateAttributes method, why do we need to call all method from framework. I could think reasons behind this are

1. We want to make sure the record existence in endpoint before making the updateAttributes method.
2. Few endpoints expects full record for update operation.

But logic for above scenarios should be part of connector updateAttributes method. If connector developer needs to verify the record existence or for full record, they can call 'all method' from connector updateAttributes method.

Maybe I am missing something which is essential for framework. Can you please provide your comments

Raymond Comments

I tend to agree with you. We should introduce static patchById and replaceById methods to PersistedModel so that:

PATCH /api//:id --> patchById
PUT /api//:id --> replaceById

It will be consistent with findById and deleteById.

[Amir-61]

Hey @Nagarjuna-S ,

I'm afraid we decided to put implementation of patchById out of scope of 3.x for some difficulties we noticed.
For more info Please see:
https://github.com/strongloop/loopback-datasource-juggler/pull/910/files#r61268157
and https://github.com/strongloop-internal/scrum-loopback/issues/752#issuecomment-215112749

Connect to https://github.com/strongloop/loopback-datasource-juggler/pull/910
Connect to https://github.com/strongloop-internal/scrum-loopback/issues/752

CC: @bajtos

Thanks!

[bajtos]

_Cross-posting from strongloop-internal/scrum-loopback#752_

Let me elaborate a bit more on my last comment.

Ideally, patchById should have these two features:

It provides the updated model instance to the callback
It validates the subset of model properties which is being updated.

The first feature require connectors to return data of the updated instance, which is not the case in the current versions of our SQL connectors.

The second feature is a bit complicated because at the moment we have only method to validate full model instance, and additionally one can use MongoDB operators like $inc that should be ideally automatically detected and excluded from the validation.

Now both features can be implemented, but it will require more time than we have right now. And I feel that without them, patchById will be less useful (and possibly dangerous!), plus it will be difficult to introduce them later as they are both a breaking change.

[raymondfeng]

Ideally, patchById should have these two features:

1. It provides the updated model instance to the callback
2. It validates the subset of model properties which is being updated.

Why does patchById need to return the updated model instance? For most cases, the client already has the existing instance and tries to update a few attributes, for example, changing email for user profile. If my assumption is true, the response can be a flag or count to tell if the patch is successful.

The validation becomes interesting. It seems that we need to have APIs for both model instance level and property value level. If we want to do full client side validation before hitting the DB, maybe replaceById makes more sense.

[bajtos]

Why does patchById need to return the updated model instance? For most cases, the client already has the existing instance and tries to update a few attributes, for example, changing email for user profile. If my assumption is true, the response can be a flag or count to tell if the patch is successful.

Here are my thoughts:

1. Backwards compatibility. Many applications built on LoopBack 2.x use updateAttributes to update data of a single instance, since that's the easiest way. I would expect there are quite few clients of those apps that rely on updateAttributes to return the updated instance. If we suddenly stop returning full instance data, the upgrade path would be much more difficult for those users.
2. When the app is under reasonable load, it's likely there will be multiple clients modifying the same record. In that case it's good to get the actual version of the model instance after partial update was performed. This becomes even more important when atomic operators like MongoDB's $inc are used. For example, I would find very useful to have updateAttributes({ count: { $inc: 1} }) return back the updated count property.

Having said that, I would also love to have a fast & atomic updateById implementation in place instead of the current one that's prone to race conditions. If we agree the cost of breaking backwards compatibility so much is acceptable, then I am ok with pursuing an implementation of patchById with less features than the current prototype.updateAttributes offer.

[Amir-61]

@bajtos @raymondfeng
Just to add up: the validation for static update* methods is a real problem. Currently we have update method which literally does not have any validation and I'm surprised why I have not seen GH issues for this problem. Probably most users use prototype.updateAttributes...

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.