setInstanceProperty fails comparison on objects or custom types with objects #517

Activity

1. dxg commented on Jun 4, 2014

   

   dxg

   on Jun 4, 2014

   Collaborator

   More actions

   This problem is harder to solve than it seems.
   I tried the above as well as a few other things, and nothing worked.

   var Place = db.define('place', {
     position: { type: 'object' }
   });
   
   ...
   
   Place.create({ position: { x: 2, y: 3 } }, ...);
   
   Place.get(1, function (err, item) {
     console.log(item.saved()); // "true"
     item.position.x = 5;
     console.log(item.saved()); // "true"
   })

   .save() will only persist to the database if saved() returns false.

   Unfortunately, in the above example when we set x to 5, the code on line 453 isn't called.
   And even if it was (eg, by doing p = item.position; item.position = null; item.position = p) the comparison still wouldn't return true because value points to the same object in memory as opts.data[key] so we'd be comparing it to itself.

   The solutions I can see are:

   1. Always assume object properties have changed
   2. When saving, fetch all Object properties from the database and compare to what we have
   3. Keep a shadow copy of all instance data and do a comparison when calling save()
   4. Allow something like item.save({ force: true }, ...) and maybe a global setting for this
   5. Add item.markAsDirty('position')

   Number 1 goes against the principle of least surprise.
   Number 2 would result in extra queries, negatively impacting speed.
   Number 3 would double memory usage - problematic if a property is storing a Buffer() of a large file.
   Number 4 & 5 is rather manual.

   I'm leaning towards number 4 and/or 5 but it's not exactly a nice solution..
2. kapouer commented on Jun 4, 2014

   

   kapouer

   on Jun 4, 2014

   ContributorAuthor

   More actions

   1. item.set('position.x', 5)
      (or variants)
3. dxg commented on Jun 4, 2014

   

   dxg

   on Jun 4, 2014

   Collaborator

   More actions

   That sounds better than my ideas.
4. dxg

   added a commit that references this issue on Jun 5, 2014

   Add Instance.set() #517

   7e6e9ab
5. dxg commented on Jun 5, 2014

   

   dxg

   on Jun 5, 2014

   Collaborator

   More actions

   We now have:

   item.set('position.x', 2)
   item.set(['position','x'], 2) // in case you have objects like { "a.b" : 4  }
   item.markAsDirty('position')

   set() will only mark the object as dirty if you are changing the value. If x was 2 and you set it to 2, it won't affect dirty state.

   Published as version 2.1.15.
6. dxg

   closed this as completedon Jun 5, 2014
7. kapouer commented on Jun 5, 2014

   

   kapouer

   on Jun 5, 2014

   ContributorAuthor

   More actions

   that's wonderful, thank you
8. kapouer commented on Sep 10, 2014

   

   kapouer

   on Sep 10, 2014

   ContributorAuthor

   More actions

   The only missing thing now is a mention of that in the docs...