setInstanceProperty fails comparison on objects or custom types with objects

[kapouer]

Hi, the comparison if (opts.data[key] !== value) { at https://github.com/dresende/node-orm2/blob/38e76722/lib/Instance.js#L453 will never be true when modifying a property of a column that is an object. The horrible workaround is:

var props = source.prop;
props.file = filename;
source.prop = null; // or anything that is not props
source.prop = props;

i suggest to improve the comparison when the value is an object:

if (opts.data[key] !== value ||
    (typeof value == "object" &&
     propertyToValue(value) !== propertyToValue(opts.data[key])
    )
   ) {../..}

so a thorough comparison is done in case of equality of objects.

[dxg]

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..

[kapouer]

1. item.set('position.x', 5) (or variants)

[dxg]

That sounds better than my ideas.

[dxg]

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.

[kapouer]

that's wonderful, thank you

[kapouer]

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