Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configuration for relation properties #2256

Closed
orshlom opened this issue Jan 16, 2019 · 10 comments
Closed

Allow configuration for relation properties #2256

orshlom opened this issue Jan 16, 2019 · 10 comments
Assignees
Labels
bug needs grooming Relations Model relations (has many, etc.)

Comments

@orshlom
Copy link

orshlom commented Jan 16, 2019

Description

When defining a new relation in a model file, the relation property is also decorated with the @property decorator called inside the relation decorator.
The problem when creating a relation property this way is that it cannot be configured the same as a standard property.

Current Behavior

For example in the Order model, the property customerId from the @belongsTo relation cannot be configured to be of type UUID (the same type as the defined Customer model id property):

@belongsTo(() => Customer, {keyTo: 'id'})
customerId: string;

For example, when trying to create a foreign key for "customerId", it fails with an incompatible type error:

detail: 'Key columns "customerid" and "id" are of incompatible types: text and uuid.'

Expected Behavior (proposal)

I think that implementing support for property configuration in relation decorators is required.
Ability to config relation properties, kind of:

@belongsTo(() => Customer, {keyTo: 'id'}, {
    type: 'string',
    defaultFn: 'uuid',
    required: true})
customerId: string;
@dhmlau dhmlau added the Relations Model relations (has many, etc.) label Jan 16, 2019
@b-admike b-admike added the bug label Jan 17, 2019
@elv1s
Copy link
Contributor

elv1s commented Feb 6, 2019

It might be possible to add the @property annotation in addition to @belongsTo to specify the datatype for the DB?

@elv1s
Copy link
Contributor

elv1s commented Feb 11, 2019

answer: it's not possible to add @property with @belongsTo as shown in #2345

@orshlom
Copy link
Author

orshlom commented Feb 12, 2019

answer: it's not possible to add @property with @belongsTo as shown in #2345

You are correct. The @property decorator is nested within the @belongsTo decorator (or any other relational decorator). This exactly is the problem with trying to configure a relational property.

@dhmlau
Copy link
Member

dhmlau commented Feb 12, 2019

@elv1s, thanks for taking this. I just assigned it to you. Thanks!

@bajtos
Copy link
Member

bajtos commented Mar 7, 2019

Re-posting #2442 (comment):

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
Copy link
Member

bajtos commented Mar 7, 2019

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

@b-admike
Copy link
Contributor

b-admike commented Mar 8, 2019

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.

IIUC, using the current implementation, AddressBookId would be decorated with belongsTo() and is actually part of the model definition. As a short term fix, I thought we want to remove the call to the property decorator inside belongsTo() decorator. Thoughts?

@elv1s
Copy link
Contributor

elv1s commented Mar 8, 2019

@b-admike See #2442 (comment)

The short term fix would be to land the belongsTo() property decorator allow optional property definitions (#2442)

The next, backward-compatible update would be to move relationship decorators to the model and deprecate use on property.

@elv1s
Copy link
Contributor

elv1s commented Mar 8, 2019

This issue is essentially a dupe of #2345 and can be closed

@bajtos
Copy link
Member

bajtos commented Mar 11, 2019

Let's keep this issue, it has been opened first and there is already discussion unfolding here. I am going to close #2345.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs grooming Relations Model relations (has many, etc.)
Projects
None yet
Development

No branches or pull requests

5 participants