Embedded models broken in repository 1.5.5

[mgabeler-lee-6rs]

Description / Steps to reproduce / Feature proposal

After upgrading my project's LB4 dependencies, I'm finding that models that have a property that is another model type are breaking. I've traced this down to a PR that, ironically, was meant to make this work where it wasn't for others before: #2505 fixing #2130.

I think things were working for me before because I was using the @property({type: AnotherModelType}) as a workaround for not having proper embedsOne support (I'm aware of #1450).

The issue I'm finding is that the first attachment of a model to a DataSource works, but this modifies the model class (specifically rebuilds entries in its definition) in such a way that a second attachment of that model to a DataSource fails.

This multiple-attachment is not something that happens with my project in production, but it does happen very much when running unit tests.

Current Behavior

• Attaching the model to a datasource modifies the .definition:
  • The properties.foo.type is changed from the class AnotherModel to function AnotherModel(data, options) resulting from the juggler conversion
• Attaching the model to a second datasource results in a bad model:
  • Deep in the model setup, isTypeResolver is called on the modified properties.foo.type object, and it returns true
  • This results in the property type being replaced again, this type with a call to that "resolve" ... but it's not a resolver, it's a constructor function
  • Now the property type is actually an instance of the model class
  • Usage of the containing model now fails in places where it tries to do instanceOf using the instance on the right hand side, which fails. And if that didn't fail, it would have tried to "call" the instance as a constructor function, which would also have failed.

Expected Behavior

• Either @property({type: ModelType}) or @embedsOne should "work"
• Attaching models to datasources should not modify the model definition
  • Or if it does, it should do so in a way such that the model can still be attached to other datasources

Attempted Workarounds

I gave embedsOne whirl even though it's not listed as supported and the most obvious issue I hit at first is that it doesn't result in a property being declared on the model, which means that the embedded object is excluded from .toJSON(). That breakage was sufficiently fundamental to my use case I didn't get any farther in testing things.

I experimented with modifying the ModelClass.definition.properties.foo object to make the type property un-settable, and this "seems" to work around my issue, but it's ... a strange and worrying thing to do.

Possible Fix

In legacy-juggler-bridge.ts ~ line 142, it looks like making it always clone the original property definition would be viable:

    Object.entries(definition.properties).forEach(([key, value]) => {
      if (value.type === 'array' || value.type === Array) {
        value = Object.assign({}, value, {
          type: [value.itemType && this.resolvePropertyType(value.itemType)],
        });
        delete value.itemType;
// BEGIN CHANGE
      } else {
        value = Object.assign({}, value, {
          type: this.resolvePropertyType(value.type),
        });
      }
      properties[key] = value;
// END CHANGE
    });

Applying this change locally gets my unit tests to pass, and causes my "protect the type property" workaround code from firing.

[dhmlau]

@mgabeler-lee-6rs, would you like to submit a patch?
If you're new to contributing to LB, you can see this https://loopback.io/doc/en/lb4/submitting_a_pr.html as a reference. Thanks.

[mgabeler-lee-6rs]

Yep, I've submitted before, and am working on submitting a patch for this. Testing locally in my environment first, and working out some kinks unrelated to this particular problem, but which are also giving me some headaches in updating to the current LB4 release.

(Having problems with DTO-style Model classes that have an id property that is not the id property)

[mgabeler-lee-6rs]

PR submitted -- I verified that it fixes the issue within my app, hopefully it does not cause problems with different use cases