feat(repository): implement belongsTo relation

[bajtos]

Implement missing pieces for belongsTo relation:

• define a new interface describing belongsTo relation metadata
• fix the decorator to set all required metadata
• define an accessor function used in repositories in lie of a repository class
• create a factory function for obtaining the accessor function
• etc.

This pull request is another step on my journey to land #1618 in several smaller chunks.

This pull request is related to #1361.

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

[bajtos]

@raymondfeng @b-admike I am not able to make the following test pass right now:

  1) TodoListApplication
       creates todo for a todoList:
     Error: expected 200 "OK", got 422 "Unprocessable Entity"

Source code:

    const todo = givenTodo({todoListId: undefined});
    const response = await client
      .post(`/todo-lists/${persistedTodoList.id}/todos`)
      .send(todo)
      .expect(200);

The situation is as follows:

• @belongsTo decorator marks the foreign key property (todoListId) as required
• The client making the request wants to omit that value and let the server set it from the path parameter.
• Our REST API is not flexible enough to describe the endpoint as accepting Todo body without the required todoListId property (this is a well known issue OpenAPI decorator does not properly generate schemas of type Partial #1179).

I'd like to ask you for your opinion on how to address this problem. I see two options:

• Don't mark the foreign key as a required property. Let the repository and/or database server to check and enforce its presence.
• Modify the failing test to send todoListId in the request body. This means that all clients of LB4 REST APIs will have to include the foreign key value when creating related model instance.

Thoughts?

[bajtos]

@raymondfeng @b-admike I just realized another problem. Notice that in some tests, we are applying @belongsTo decorator on the foreign key property, e.g.

https://github.com/strongloop/loopback-next/blob/d407ea3efdc0c7788a0e9def5d9f6c41012efab6/packages/repository/test/fixtures/models/order.model.ts#L29-L30

but in other places we apply the decorator on the target model getter property, e.g.

https://github.com/strongloop/loopback-next/blob/d407ea3efdc0c7788a0e9def5d9f6c41012efab6/packages/repository/test/unit/decorator/model-and-relation.decorator.unit.ts#L101-L105

The second approach have two problems:

• It is not enough to define customer property, belongsTo always required the foreign key to be defined on the source model too (e.g. customerId).
• A getter property is introducing a dependency cycle that's not possible to break as long as TypeScript is setting design:type metadata. In our test showing @belongsTo on customer, we use ICustomer instead of Customer as the property type.

I think we should unify the code in this pull request to use only one of this approaches; and I am leaning towards decorating the foreign key property.

Thoughts?

[b-admike]

The situation is as follows:

@belongsTo decorator marks the foreign key property (todoListId) as required
The client making the request wants to omit that value and let the server set it from the path parameter.
Our REST API is not flexible enough to describe the endpoint as accepting Todo body without the required todoListId property (this is a well known issue #1179).
I'd like to ask you for your opinion on how to address this problem. I see two options:

Don't mark the foreign key as a required property. Let the repository and/or database server to check and enforce its presence.
Modify the failing test to send todoListId in the request body. This means that all clients of LB4 REST APIs will have to include the foreign key value when creating related model instance.

Thanks for explaining the issue in detail @bajtos. My vote is for not marking the foreign key as a required property and to let the database check its presence. But I guess that may not be true for all the databases we support (not sure if referential integrity is enforced in NoSQL dbs). I am okay with the other approach if there is strong reason to do so, but I see this as a temporary issue since it'll be resolved in #1179.

[b-admike]

@raymondfeng @b-admike I just realized another problem. Notice that in some tests, we are applying @belongsTo decorator on the foreign key property, e.g.

loopback-next/packages/repository/test/fixtures/models/order.model.ts

Lines 29 to 30 in d407ea3

@belongsTo(() => Customer)
customerId: number;
but in other places we apply the decorator on the target model getter property, e.g.

loopback-next/packages/repository/test/unit/decorator/model-and-relation.decorator.unit.ts

Lines 101 to 105 in d407ea3

@belongsTo(() => Customer)
// TypeScript does not allow us to reference Customer here, the compiled
// code throws at runtime when trying to set design:type metadata to Customer
// class that is not defined yet.
customer: ICustomer;
The second approach have two problems:

• It is not enough to define customer property, belongsTo always required the foreign key to be defined on the source model too (e.g. customerId).
• A getter property is introducing a dependency cycle that's not possible to break as long as TypeScript is setting design:type metadata. In our test showing @belongsTo on customer, we use ICustomer instead of Customer as the property type.

I think we should unify the code in this pull request to use only one of this approaches; and I am leaning towards decorating the foreign key property.

Thoughts?

I was not aware of the places where we use the @belongsTo decorator for the target model getter property. I think that might have been an oversight on our part and agree to unify it to be on the foreign key property.

[marioestradarosa]

not sure if referential integrity is enforced in NoSQL dbs

The ref integrity has to be enforced at the client level, the document based DB has no rules for this at the DB Engine level.

[bajtos]

A getter property is introducing a dependency cycle that's not possible to break as long as TypeScript is setting design:type metadata. In our test showing @belongsTo on customer, we use ICustomer instead of Customer as the property type.

I filled a TypeScript issue for this. IMO, the compiler should report compile-time error if it cannot emit code that works. See microsoft/TypeScript#27519

[bajtos]

Thank you for the comments. I am going to make the following changes:

• Keep the foreign key as an optional property.
• Move @belongsTo decorator to the foreign key property. I am not sure if this is the right design decision, I feel it's inconsistent with @hasMany that's decorating a property for accessing the related model instances, but I don't have any better solution right now (especially because of Design:type metadata for cyclic dependencies throw at runtime microsoft/TypeScript#27519).

@marioestradarosa

The ref integrity has to be enforced at the client level, the document based DB has no rules for this at the DB Engine level.

When you say "client level", do you mean our ORM layer (@loopback/repository) as the database client, or a web/mobile/IoT application acting as the client sending REST requests to an LB4 API server?

Ideally, I'd like LoopBack to enforce referential integrity inside our ORM layer (@loopback/repository). That is difficult right now because of #1179.

When a model is created via a relational API, e.g. customerRepo.order(customerId).create(), then the foreign key is always set. However, the regular CRUD API (orderRepo.create()) is not checking for the presence of the foreign key (because it's an optional property now) and even if the foreign key is set, it does not check whether it is referring to an existing target model instance.

Are you ok with proceeding with the currently proposed solution and add LoopBack-level checks of foreign key values later?

[bajtos]

@raymondfeng @b-admike @jannyHou @marioestradarosa the pull request is done and ready for the final review. PTAL.

I am leaving documentation out of scope of this pull request in order to get it lander sooner. I'll open a new PR to document this new relation type.

[raymondfeng]

💯

[b-admike]

Great work and thank you for adding TSDocs / fixing up existing test suites 🎉

[b-admike]

is the required: false line meant to be commented (I guess it doesn't make a difference since AFAIK it's false by default)?

[bajtos]

Good catch. I'll keep the commented line to make it clear what is TODO, but change it to required: true.

[b-admike]

Is required: true set by juggler because this is the only property in the model def?

[bajtos]

No, required: true used to be set by @belongsTo decorator before I changed this behavior based on our discussion.

[b-admike]

Dont we want to clean up the repos in the before() handler so that we can inspect the records after if tests fail and to start with clean repository instances?

[bajtos]

Sure! I was copying this code from elsewhere, will fix it.

[b-admike]

HasMany relation -> belongsTo relation