Retrieving an embedded property (type extends Model) has an extra field `id` with value undefined

jannyHou commented 3 years ago

Steps to reproduce

Use case: Customer extends Entity Address extends Model Customer model has a property address: Address Data is stored in mongodb

Current Behavior

Problem: Fetch Customer entity from db using customerRepo.findById method, its address property has an id field with null value.

E.g.

{
  name: 'Jack',
  address: {
    // id should not exist
    id: undefined
    city: 'NYC',
    zipcode: '000000'
  }
}

Expected Behavior

{
  name: 'Jack',
  address: {
    city: 'NYC',
    zipcode: '000000'
  }
}

Link to reproduction sandbox

Additional information

The problem is https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/model.js#L88 When the retrieved customer’s address property gets converted to an instance of Address , that line has an additional id field even it’s not defined in the Address model.

Quoted solution from Miroslav:

Actually, I think the code building the juggler model is correct: https://github.com/strongloop/loopback-next/blob/ec4c10c1d38ba80f59014ef447565894cf8b484b/packages/repository/src/repositories/legacy-juggler-bridge.ts#L176-L210 The problem is that we are attaching the embedded model to the datasource, which in turn adds a PK (id property) to the model definition: https://github.com/strongloop/loopback-next/blob/ec4c10c1d38ba80f59014ef447565894cf8b484b/packages/repository/src/repositories/legacy-juggler-bridge.ts#L211

I think the behavior should be as follows:

If the juggler model is backing the LB4 model the repository is for, then we must keep calling modelClass.attachTo(dataSource)
If the juggler model is created for nested/embedded models, then we must attach it to the dataSource. A possible implementation is to introduce options: {attachToDataSource: boolean} argument to ensurePersistedModel and definePersistedModel functions and then use the new option to decide whether a model should or should not be attached. I think we should be attaching by default for backwards compatibility, and change resolvePropertyType to set attachToDataSource: false.

Related Issues

See Reporting Issues for more tips on writing good issues

Fr4nZ82 commented 3 years ago

malins commented 3 years ago

Hello!

I have the same problem. Is having nested/embedded models in MongoDB somewhat of an advanced usage pattern?

Thank you, Manuel

jannyHou commented 3 years ago

@Fr4nZ82 I believe yes, they are caused by the same reason, As @bajtos pointed out, the problem is we shouldn't attach the embedded lb4 model to datasource. Would you be interested in submit a PR to fix it? The solution is explained in the "Additional information".

jannyHou commented 3 years ago

Let me close this one as a dup of https://github.com/strongloop/loopback-next/issues/6029

loopbackio / loopback-next