feat(repository): allow optional property definition on belongsTo decorator

[derdeka]

allow optional property definition on belongsTo decorator as property decorator can not applied twice

Fixes #2345
Fixes #2256

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated
• Agree to the CLA (Contributor License Agreement) by clicking and signing

[raymondfeng]

@derdeka Thank you for the PR. Would you please add a test case to cover such usage?

[derdeka]

@raymondfeng
I've added a unit test - hopefully in the correct file.
The build currently fails because of issue #2455

[raymondfeng]

ok to test

[raymondfeng]

I'm not very happy that we have to mix the property metadata with relations.

Ideally, @belongsTo should be applied to the addressBook relational property and @property to the addressBookId. Something like:

      @model()
      class Address extends Entity {
        id: string;
        @belongsTo(
          () => AddressBook,
          {},
        )
        addressBook?: AddressBook;

        @property({
            length: 36,
            postgresql: {
              dataType: 'uuid',
            },
          })
        addressBookId: string;

WDYT?

[elv1s]

What will the address table look like in the database? Specifically what is the column type of addressBook

Another approach might be to allow multiple decorators on a property, with the constraint that the decorator keys must be unique.

[orshlom]

I think that using two different properties in a model to define a relation is redundant.
In my opinion, the property addressBook?: AddressBook; @raymondfeng suggested should be also the one internally creating the column addressBookId in the DB. This way it can get automatically configured to comply with the PK it references.
In addition, property decorator (@belongsTo()) may also support additional configuration for the FK column in the DB (addressBookId).

[derdeka]

      @model()
      class Address extends Entity {
        id: string;
        @belongsTo(
          () => AddressBook,
          {},
        )
        addressBook?: AddressBook;

        @property({
            length: 36,
            postgresql: {
              dataType: 'uuid',
            },
          })
        addressBookId: string;

@raymondfeng I really like this approach as it is clearly seperates concerns and gives the developer more control of just reading the id or loading the related object (with subquery to the database).

Should this work already with the latest release or are changes needed to make this possible? (I tried this approach on my application and got an exception "500 Error: Invalid type for property type")

[elv1s]

When I run the code it creates a text column for addressBook. This seems confusing to have a AddressBook table and AddressBook column with the same information. I agree with @orshlom that two properties are redundant and maybe the @belongsTo property can automatically create an addressBookId property to link to the primary key.

Also I'm wondering what the API for the user will be when they call the Address endpoint. Will the user be able to create an Address and AddressBook in a single POST. How will they add a reference to an existing addressBook?

@derdeka The issue you are running into might be related to length. You can move it to dataLength within postgresql, but when I do that I'm getting an error type modifier is not allowed for type "uuid"

[drdmitry]

As I understood, as belongsTo decorator uses property decorator inside the code, this means that on every belongsTo decorator call - new property will be created, and then belongsTo creating the relation for the same.

https://github.com/strongloop/loopback-next/blob/4cee896e7f7b2b503b118e560c31036626587f47/packages/repository/src/relations/belongs-to/belongs-to.decorator.ts#L19-L56

For the suggested approach of splitting belongsTo and property decorators for defining the relation - this doesn't work, because it is trying to create the same property 2nd time.
Could we somehow map property which is created/defined in belongsTo to the property created with property decorator?
Or could we have creating property in belongsTo only when it is not created yet?
What do you think? How this could be solved?

[elv1s]

Is this PR still valid? I'm interested in this pull request since it is the same fix for #2256

It sounds like we don't want to mix the property and belongsTo decorators. The solution provided by @raymondfeng might work but it adds redundant info to the db and schema which will be confusing to the user.

Is there a way to prevent the addressBook property from being part of the schema or created in the db?

[derdeka]

I also tried to map addressBook to the existing addressBookId, but i did not get it working. For me is the PR still a valid workaround.

@raymondfeng @bajtos Can you please advise how to continue?

[bajtos]

It sounds like we don't want to mix the property and belongsTo decorators. The solution provided by @raymondfeng might work but it adds redundant info to the db and schema which will be confusing to the user.

Here is one reason why we should not mix the property and the relation decorator:

• When creating a new model instance, or updating an existing model instance, we don't want the data type to include the navigational property. addressRepo.create(data) does not support updating the AddressBook instance we are belonging to, therefore data type should exclude addressBook property.
• Navigational properties are needed only when querying data via repo.find.
• This difference must be propagated to OpenAPI schemas used by REST API endpoints.

For long term, I would like to propose following API at conceptual level (implementation details will be most likely different):

// Relations are defined at model-level
@belongsTo(() => AddressBook)
@model()
class Address extends Entity {
  // the foreign key is defined explicitly
  @foreignKey(() => AddressBook, 'id')
  @property({
    length: 36,
    postgresql: {
      dataType: 'uuid',
    },
  })
  addressBookId: string;
}

// A custom class or interface describes format
// of data returned by a query
@model()
class AddressWithRelations extends Address {
  @property()
  addressBook?: AddressBook;
}

// Repository API
class DefaultCrudRepository {
  find(filter: Filter<Address>): Promise<AddressWithRelations>;
  create(data: Address): Promise<Address>;
  // ...
}

[bajtos]

Relatively recently, we changed @hasMany decorator so that it no longer calls @property decorator under the hood. IMO, we should make the same change to @belongsTo and @hasOne decorators too, to ensure consistent behavior.

Unfortunately, such change is backwards-incompatible and requires a new semver-major release of @loopback/repository. We need to follow the steps in Making breaking changes section of our DEV docs.

While we are making a new major version, I think it would be good to use this opportunity to review other existing problems in the current implementation of relations and ideally fix all those that are backwards incompatible. (E.g. rework relation decorators to be applied on model classes, not navigational properties.)

Considering the effort required for that, it may be best to land this pull request as a short-term workaround.

[bajtos]

Related: in #2152, we will be investigating how to best represent navigational properties in model classes.

[bajtos]

Relatively recently, we changed @hasmany decorator so that it no longer calls @Property decorator under the hood. IMO, we should make the same change to @belongsTo and @hasone decorators too, to ensure consistent behavior.

Unfortunately, such change is backwards-incompatible and requires a new semver-major release of @loopback/repository. We need to follow the steps in Making breaking changes section of our DEV docs.

While we are making a new major version, I think it would be good to use this opportunity to review other existing problems in the current implementation of relations and ideally fix all those that are backwards incompatible. (E.g. rework relation decorators to be applied on model classes, not navigational properties.)

On the second thought, I think there is a way how to make these changes in a backwards-compatible way.

1. For existing applications, keep @belongsTo calling @property under the hood. Land this PR to allow consumers to provide additional property metadata.

2. Going forward, extend @belongsTo to support decoration of model classes, as I outlined in
   feat(repository): allow optional property definition on belongsTo decorator #2442 (comment). Keep supporting property-based decoration for backwards compatibility, possibly deprecate it at runtime via depd.

export function belongsTo<T extends Entity>(
  targetResolver: EntityResolver<T>,
  definition?: Partial<BelongsToDefinition>,
) {
  return applyBelongsTo;

  function applyBelongsTo(decoratedTarget: typeof Entity): void;
  function applyBelongsTo(decoratedTarget: Entity, decoratedKey: string): void;
  function applyBelongsTo(
    decoratedTarget: Entity | typeof Entity,
    decoratedKey?: string,
  ) {
    if (typeof decoratedTarget === 'function' && decoratedKey === undefined) {
      // model-level decoration
   } else {
     // property-level decoration
  }
}

[bajtos]

I agree with @raymondfeng that in the longer term, @belongsTo decorator should not be setting up the foreign-key property.

At the same time, I'd like use to fix ASAP the problem where it is not possible to customize the configuration of the foreign key property at all.

In that light, I'd like us to land this pull request as a short-term fix, and thus buy us more time to address this issue in a wider context.

The implementation looks mostly good to me, I have two comments regarding the tests.

[raymondfeng]

In that light, I'd like us to land this pull request as a short-term fix, and thus buy us more time to address this issue in a wider context.

+1.

[derdeka]

@bajtos what is your timeline for merging this PR?

[bajtos]

Landed, thank you @derdeka for the contribution 🎉