updateAttributes operational hooks: use instance, not where

[fabien]

Because it operates on an instance, you know ...

[raymondfeng]

I ran into the same confusion too. updateAttributes is an instance method, the ctx.instance should be set. We should also keep ctx.data to the data argument which contains the name/value pairs to be changed.

[fabien]

Sounds great @raymondfeng - it's a blocking issue for me at the moment. Any chance of pushing this soon?

[raymondfeng]

@fabien Do you have time to submit a patch?

[fabien]

Hmm, I'll try tonight - but I'm guessing @bajtos is more familiar with the ins-and-outs.

[fabien]

@raymondfeng working on it - #474 as well

[fabien]

@raymondfeng this is a first stab at it - I'll try to tackle #474 later tonight - please review

[fabien]

@raymondfeng I'm thinking it makes sense to propagate an operation object as part of the context as a generic 'stash' for keeping state between before and after:

Model.observe('before delete', function(ctx, next) {
  if (ctx.instance) {
    ctx.operation.instances = [ctx.instance];
  } else {
    Model.find({ where: ctx.where }, function(err, instances) {
      ctx.operation.instances = instances;
      next();
    });
  } else {
    next();
  }
});

Model.observe('after delete', function(ctx, next) {
  if (ctx.operation.instances) {
    async.forEach(ctx.operation.instances, removeFromDisk);
  } else {
    next();
  }
});

[bajtos]

Let me explain the reasons behind the current decision and why I think we should take a different approach.

1. The old hooks provided both the instance via "this" and data via function arg. It caused confusion as sometimes the correct approach was to change the properties via "this" and sometimes via "data" and it was not clear when to use which.
2. While updateAttributes operates on an instance, it performs a partial update. Originally I used ctx.instance in the "before save" hook, but that make it difficult to find out what modifications the user made, how to map the instance properties into the partial data to update. Especially in such way that when the user does not make any modifications then no performance penalty is paid. See Operation hooks for persistent models (intent-based hooks) #403 item "5) prototype.updateAttributes and data modified by hooks" and Operation hooks for persistent models (intent-based hooks) #403 (comment)

Here is my proposal: add a new method called e.g. prototype.update, mapped to POST /{models}/:id. This new method will perform a full update (replace) of all properties and thus set ctx.instance. This new method should become the new default to use when one wants to update the model instance (e.g. via $save() in Angular, see strongloop/loopback-sdk-angular#125 (comment)), while prototype.updateAttributes will be left for advanced scenarios only.

I'm thinking it makes sense to propagate an operation object as part of the context as a generic 'stash' for keeping state between before and after:

+1, thought I would use a different property name than "operation".

@fabien please note that your code contains a race condition: it's possible that the list of models returned by Model.find({where}) is different from the list of models deleted by Model.deleteAll(where), because a third party may modify relevant model instances in the small window between these two database queries. There are two solutions:

1. In the "before delete" hook, convert the arbitrary where query into an id-based query. Perform Model.find and then build a query using the returned ids. In this case you don't need to pass any data between hooks, because the list of ids can be easily extracted from the where property.
2. Drop "before delete" hook and rework the "after delete" hook to perform a full scan: find all instances that still exist in the database, compare it to the list of files, and then remove any extra files. Another advantage of this solutions is that if some files cannot be (temporarily) removed from the disk, they will be eventually removed later. again, you don't need ctx.operation.

[fabien]

@bajtos having prototype.update as the new default would be a huge breaking change - don't do it, please. Yes, updateAttributes performs a partial update, so data is not complete, however, the instance would be - that's the basic premise of it.

Any thoughts on the state property name (now called 'operation')?

Thanks for the pointers on my example:

1. how would this be different from passing along the instances as opposed to the ids of them? I gather either instances or ids from the same where condition, so I don't see where the difference would be.
2. seems very wasteful, as I would have to map all the files on disk to what's in the db - this could mean comparing /finding thousands of entries first.

[bajtos]

@bajtos having prototype.update as the new default would be a huge breaking change - don't do it, please.

It won't be a breaking change, because the old "updateAttributes" (POST /:id) will remain in place. I meant that we will stop telling users to call "prototype.updateAttributes" and tell them to call "prototype.update" instead.

Yes, updateAttributes performs a partial update, so data is not complete, however, the instance would be - that's the basic premise of it.

Here is my understanding of updateAttributes (I did not run a test though):

• Let's have a Person with {id: 1, name: 'John', age: 42 }

• Run the following two requests in parallel:

  PUT /people/1
  { "name": "Jane" }
  
  PUT /people/1
  { "age": 24 }
  

• When both requests complete, we always end up with {id: 1, name: 'Jane', age: 24 }, regardless of the order in which the database queries end up executed.

That's why updateAttributes don't necessarily have a complete instance, and why I am proposing to add a new method "update" that will have the complete instance.

See the code - it passes only the properties from the request to the connector, not the full instance data.

 inst._adapter().updateAttributes(model, getIdValue(inst.constructor, inst),
   inst.constructor._forDB(typedData), function (err) {...});

[raymondfeng]

@bajtos You misunderstood the updateAttributes(). It in fact applies all changes in an atomic operation. For RDBs, the SQL is something like:

UPDATE MyModel SET name='Jane', age=24

It's also the case for mongodb.

The updateAttribute is patching instead of replacing. I'm open to introduce a new instance method for replacing.

[fabien]

@raymondfeng thanks for chiming in - yes, an additional method would be OK though.

[raymondfeng]

Maybe we need to have enough information in the ctx to tell if a save or persist operation is:

1. Create a new record (create)
2. Replace an existing record (by id) - to be added
3. Patch an existing record (by id) - prototype.updateAttributes
4. Update records matching the criteria (by where)

There are other combinations too, for example,

1. findOrCreate - create if not found
2. updateOrCreate/upsert - create or update
3. prototype.save - create or update (probably should be full replacement)

Depending on the connector implementations, the compound operations can be atomic too.

[bajtos]

Any thoughts on the "state" property name (now called 'operation')?

That's better. Few more alternatives to consider: "appData", "hookData", "hookState".

1. how would this be different from passing along the instances as opposed to the ids of them? I gather either instances or ids from the same where condition, so I don't see where the difference would be.

Consider the following ordering of database changes for "deleteAll where color is red":

1. "before delete" hook lists the instances
  SELECT * FROM Cars WHERE color = 'red' 
 and receives Car1, Car3

2. another client updates the data in parallel
   UPDATE Cars SET color = "red" WHERE id = 2;

3. yet another client updates the data in parallel
  UPDATE Cars SET color = "black" WHERE id = 1

4. Finally the "delete" operation is executed
  DELETE FROM Cars WHERE color = "red"
 and deletes Car2, Car3

 5. "after hook" receives Car1 and Car2

Using my proposal, the step1 changes the query from color: "red" to id: { inq: [1,3] }, thus any changes made in between will not affect the list of delete instances.

2. seems very wasteful, as I would have to map all the files on disk to what's in the db - this could mean comparing /finding thousands of entries first.

Fair enough, as long as you have a good retry mechanism for failed deletes.

Let's move the discussion about delete hooks to #474 please.

[bajtos]

@raymondfeng @fabien I am afraid you are wrong and my understanding of updateAttributes is correct, see the following gist: https://gist.github.com/bajtos/348e1468d112180d4223 I have run the test using both Memory and MongoDB connectors, in both cases updateAttributes performed only a partial update of the properties specified in the request, leaving all other properties unchanged.

Now that we have clarified the real behaviour of "updateAttributes" as observed by running a real code, is it clear why I made the decision to pass "data" in "before save" hook and why I am very reluctant to pass the "instance"?

The updateAttribute is patching instead of replacing. I'm open to introduce a new instance method for replacing.

There are two steps involved:

1. How to combine the instance data with the request data (patch or replace).
2. What properties of the combined data should be persisted in the database (partial or full update)

"updateAttributes" applies a patch and makes a partial update.

The way how I envision the new "update" method is to apply a patch in the first step, but run a full update in the second step:

function update(data, cb) {
  for (var k in data) { this[k] = data[k]; }
  data = this.toObject();
  // note: keep undefined properties, they should be reset in the database
  connector.updateAttributes(model, id, data, cb...);
}

[fabien]

@bajtos sorry, I didn't explain myself correctly - I was aware that updateAttributes performs a partial update. Hence I wrote #483 (comment)

So finally, I would like proceed with what I propose there. I really need to have a working solution this morning - I'm resolving all the deprecations with a massive code base that relies heavily on the old behavior. As I said, updating is non-trivial at best - it's a blocking issue in some cases even.

[bajtos]

BTW I opened a new issue for implementing hook state - see #484

[bajtos]

Fixed by #488 which added ctx.currentInstance.

[jjhesk]

what is the proper way or the best practice to do update operation?