Regression: dynamic coercion of Model will clobber property values on update

[fabien]

I finally went through a full update cycle with my apps, and in updating from 2.18.0 to 2.23.0 I discovered the following undesired (and BC breaking) behavior:

With a property that has some default value (either static or a fn/dynamic value):

"properties": {
  "foo": {
    "type": "string",
    "default": "bar"
  }
}

// create receives a new instance, with defaults filled in
// which we'll overwrite by our new data
this.request.post('/api/things')
  .send({ foo: 'other' }).end(function(err, res) {
  res.body.foo.should.equal('other');
  // id: 1234567890
  next();
});

// updateAttributes now receives a new instance, with defaults filled in
// but this time, we don't have any value for 'foo' set explicitly
// and thus the default will be used for the update(!)
this.request.put('/api/things/1234567890')
  .send({ biz: 'baz' }).end(function(err, res) {
  res.body.biz.should.equal('baz');
  res.body.foo.should.equal('other'); // Fail: overwritten by default: 'bar'
  next();
});

I think it's related to this commit: strongloop/strong-remoting@34b0665

/cc @bajtos @ritch

---

See #1806 (comment) for the proposed solution.

• Implement support for arg.model in loopback-swagger
• Rework remoting metadata in LoopBack, use { type: 'object', model: modelName } everywhere. Make sure to add unit-tests reproducing the problem and then demonstrating that the problem is fixed.

The last item from the proposal is tracked by a standalone issue: #2313

Fix swagger metadata for updateAttributes and friends to correctly indicate all properties are optional. Check other schema parts like "default" values, we may need to remove them too.

[fabien]

I was able to identify where the issue exactly happens, but the fix is unsatisfactory because it prevents dynamic (object) type conversion.

However, it's extremely difficult to not end up with the issue here - how can we declare that the coerced values are in fact just defaults and that they should not overwrite any existing data on update?

[fabien]

@raymondfeng any thoughts on this? The fix I mention over at strongloop/strong-remoting#254 works fine, but I'm not sure if this has any actual drawbacks at the moment.

[raymondfeng]

@fabien Should we introduce an option such as applyDefaults (default to true) for the model constructor so that the coercion can skip the defaults?

[fabien]

@raymondfeng sounds reasonable, yes - then again, I don't think strong-remoting is even aware of Loopback models, let alone the passing of such options to a constructor?

[fabien]

@bajtos +1 for label: major

[raymondfeng]

@fabien It starts to make me wonder if we should introduce a static method on the model class to handle the coercion, such as MyModel.coerce(rawValue).

We have a few different flavors in creating a model instance.

• Construct a model instance, applying defaults and validating properties
• Loading raw json object from DB or REST into a model instance as-is (No defaults will be applied, with or without validation)

[fabien]

Having a public method like that would certainly be great (I found myself wishing for this before), and it looks like this will resolve the issue.

[fabien]

@raymondfeng digging further, I did find applyDefaultValues to be already implemented here: https://github.com/strongloop/loopback-datasource-juggler/blob/934786200b0180ed3d3cf45f78986b2019511e79/lib/model.js#L79

Which suggests that we can change the following: https://github.com/strongloop/loopback/blob/master/lib/model.js#L131

Into:

    RemoteObjects.convert(typeName, function(val) {
      return val ? new ModelCtor(val, { applyDefaultValues: false }) : val;
    });

Unfortunately I wasn't able to test this, because of multiple test failures (out of the box) with the latest pull of loopback. This is a blocking issue for me - your feedback is welcome! Thanks.

[fabien]

Update: the above does not work at all. I'm really starting to wonder if we actually want this coercion to be applied when the httpContext is being created. We already have a proper sharedCtor to handle this for us. Think about it for a moment:

The httpContext coerced model instance will always be a 'new' model; I can't really think of any use for such a model to be made available in coerced form at this point of the remoting invocation. We feed plain objects to our objects anyway (and whenever we don't, we convert them internally using .toObject first (see ModelBaseClass.prototype._initProperties), and the sharedCtor takes care of this where it's needed right now.

The issue here has only been introduced recently, when strong-remoting was updated to do this coercion. Can we please revert to the former behavior? Using the workaround mentioned?

In fact, when I completely remove the RemoteObjects.convert call here: https://github.com/strongloop/loopback/blob/master/lib/model.js#L130 everything still works fine, and the issue is gone too.

In short: can anyone please explain why we even need this coercion to take place in httpContext? If not, it should not have been added. What's the use-case? Please defend this choice, or revert.