feat(repository): hasOne relation #2005

b-admike · 2018-11-09T16:46:31Z

**EDIT - Continuation of #1879 **

First iteration of hasOne relation. This PR includes the the repository factory, repository interface and default hasOne repository, and has one decorator implementations (mostly taken from our hasMany implementation since they're very similar in nature). It also includes an acceptance test with a Customer has one Address relation to drive it and a unit test as well. Please note that only the EDIT get and create methods are implemented at the moment and tests covering them, so that we can focus on that and incrementally add patch and delete methods to the hasOne repository interface.

Todos

Add documentation for hasOne relation

Add example that uses hasOne relation

See discussion in #1956. @bajtos proposed using the PK of a target model in a hasOne relation to enforce uniqueness and 1-to-1 relation between source and target model instances (see #1956 (comment)). ~~This is just a quick spike to see the feasibility of the proposal and get more feedback.~~

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 · 2018-11-12T13:33:16Z

Looks good to me at high level 👍 But! One of the has-many tests is failing, so your implementation may need few tweaks?

I think the next task is to verify this implementation against real databases, e.g. MongoDB and MySQL. It would be great to have a full example app with hasOne relation configured for that. Can we come up with a meaningful entity that we could add to the existing todo-list example?

bajtos

Uh oh, looks like I forgot to post few review comments the last time I was reviewing these changes.

packages/repository/test/acceptance/has-one.relation.acceptance.ts

packages/repository/test/fixtures/models/address.model.ts

packages/repository/src/relations/has-one/index.ts

examples/todo-list/src/models/author.model.ts

examples/todo-list/src/controllers/author.controller.ts

examples/todo-list/src/repositories/todo-list.repository.ts

bajtos · 2018-11-29T14:52:01Z

packages/repository/src/relations/has-one/has-one.repository.ts

+      options,
+    );
+    if (found.length < 1) {
+      // We don't have a direct access to the foreign key value here :(


Should we leave a TODO comment here? Maybe open a new issue to follow-up later?

Opened #2114 as a follow-up issue.

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

packages/repository/test/fixtures/repositories/customer.repository.ts

examples/todo-list/src/controllers/todo-list-image.controller.ts

bajtos

Looks reasonably good for the first iteration.

Please address the comment containing exclamation mark ❗️ before landing this pull request.

Remaining comments can be addressed either as part of this PR or in a follow-up pull request.

packages/repository/src/relations/belongs-to/belongs-to.decorator.ts

packages/repository/src/relations/has-one/has-one-repository.factory.ts

packages/repository/src/relations/has-one/has-one.repository.ts

packages/repository/test/acceptance/has-one.relation.acceptance.ts

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

packages/repository/test/unit/repositories/has-one-repository-factory.unit.ts

bajtos · 2018-12-03T14:39:22Z

packages/repository/src/relations/belongs-to/belongs-to.decorator.ts

@@ -11,8 +11,11 @@ import {BelongsToDefinition, RelationType} from '../relation.types';

 /**
 * Decorator for belongsTo
- * @param targetResolver
- * @param definition
+ * @param targetResolver A resolver function that retruns the target model for


typo: retruns

packages/repository/test/acceptance/has-one.relation.acceptance.ts

bajtos · 2018-12-03T14:41:35Z

packages/repository/test/unit/repositories/has-one-repository-factory.unit.ts

@@ -132,7 +132,7 @@ describe('createHasOneRepositoryFactory', () => {
    province: String;
  }

-  class Order extends Entity {
+  class ModelWithoutIndexFK extends Entity {
    static definition = new ModelDefinition('Order')


Please fix the model name string to match the class name.

bajtos · 2018-12-03T14:42:24Z

packages/repository/test/unit/repositories/has-one-repository-factory.unit.ts

@@ -142,18 +142,23 @@ describe('createHasOneRepositoryFactory', () => {
        type: 'string',
        required: true,
      })
+      .addProperty('id', {
+        type: 'number',
+        id: true,


I am little bit confused. The model name is ModelWithoutIndexFK, but it has a regular id property. What am I missing?

Good question. I added an id property because the model itself extends from Entity, but the purpose here is the decorated relational property is not part of the composite index, thus the name ModelWithoutIndexFK. Thoughts on improving it if it's confusing? I can extend from Model and remove the id property as well.

I can extend from Model and remove the id property as well.

If this works, then I think it would be a good solution.

If not, then please capture the information from your comment above in a code comment.

jannyHou

@b-admike Great effort! I haven't got enough time to review each single detail but the new example and test cases LGTM in general 👍

I have a question regarding the BelongsToUnique decorator: My understanding is, regardless it's a hasMany relation or hasOne relation, one child instance could only have one parent, why would we create a specific belongsToUnique decorator for hasOne?

b-admike · 2018-12-04T06:12:50Z

I have a question regarding the BelongsToUnique decorator: My understanding is, regardless it's a hasMany relation or hasOne relation, one child instance could only have one parent, why would we create a specific belongsToUnique decorator for hasOne?

Great question @jannyHou. The reason we created a sugar syntax decorator belongsToUniquely is so that the decorated property is designated as a PK for the target model (needed for hasOne relation). It is to save time to label it as an id property that is not generated. It makes the UX better to not manually write those out.

raymondfeng

Please address my comments.

raymondfeng · 2018-12-04T06:22:28Z

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

+    @property()
+    registeredOn: Date;
+  }
+


It's not so obvious that RegistrationDate deserves a hasOne relation from the Customer. Something like A customer has a public profile is probably better.

raymondfeng · 2018-12-04T06:27:05Z

packages/repository/src/relations/belongs-to/belongs-to.decorator.ts

+  const propertyMetaDefaults: Partial<PropertyDefinition> = {
+    id: true,
+    generated: false,
+  };


Is the intention to make the FK unique? If so, I think it's wrong to set id to true as it will add the FK to the PK. For example, (id, customerId) becomes the PK.

I think we need to mark it as unique. IIRC, we can mark it uniquely indexed.

Is the intention to make the FK unique?

Yes.

If so, I think it's wrong to set id to true as it will add the FK to the PK. For example, (id, customerId) becomes the PK.

The way we have envisioned the current hasOne implementation, there is only one PK property, and that's customerId, there is no property called id. I.e. the primary key is a foreign key too.

I think we need to mark it as unique. IIRC, we can mark it uniquely indexed.

I am not sure if all databases support unique constraint. I am also pretty sure our auto-migration implementation does not correctly define unique constraint either.

As a result, by using unique instead of id, the default implementation of hasOne in LB4 would not work correctly out of the box.

On the other hand, all databases are enforcing the uniqueness constraint on primary keys (id property), otherwise primary keys could not be used as unique identifiers.

I think it's too restrictive to promote the FK for hasOne to be part of PK. At least we should check if there is an existing PK.

BTW, marking a property id: true does not guarantee it will be enforced by the database. I think we should generate an index with {unique: true} option per https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#indexes.

Not only we are promoting FK to be the only PK, but we also require the target od HasOne to have the same PK as the model it relates to. E.g. when a Customer has one PublicProfile, then both the customer instance and the profile instance share the same primary key value.

This is the only way I am aware of that offers strong guarantees about consistency and avoid possible race conditions in all databases we support in LoopBack.

I am more than happy to consider different approaches, as long as they work similarly well across the databases we have connectors for.

Should we open a new issue to continue this discussion?

raymondfeng · 2018-12-04T06:29:44Z

packages/repository/src/relations/belongs-to/belongs-to.decorator.ts

+ * @param definition Optional metadata for setting up a belongsTo relation
+ * @returns {(target: Object, key:string)}
+ */
+export function belongsToUniquely<T extends Entity>(


The name is not easy to understand. The purpose is to say the FK should be unique.

Maybe it's a warning sign that we should not overload @belongTo for responsibility of @property.

I agree that ideally, relation decorators like @belongsTo and @hasMany should not call @property decorator under the hood. To get there, we would have to change the current design in a non-trivial way and break backwards compatibility along the way.

Do we want to delay hasOne implementation?

Personally, I'd prefer to finish hasOne implementation using the current approach, and then look into ways how to change the overall design.

The name is not easy to understand. The purpose is to say the FK should be unique.

Can you propose a better name that's easier to understand?

Our intention here is to provide a syntactic sugar that will allow users to tell what they want to accomplish (setup the other side of hasOne relation) without going into low-level details (setting up a PK property as a way to get uniqueness constraint + make queries fast).

RaphaelDrai · 2018-12-05T06:17:09Z

Hello @b-admike ,
Great work with the merge of hasOne, thanks you so much !
As discussed in #1879 we are currently working on hasOne example and documentation as per todos requirement above.
Is there an estimation when the missing PATCH and DELETE operations will be covered? If not, and that no one is actually working on PATCH and DELETE, we can contribute to provide these features. Let me know if we can start this task?
Raphael

b-admike · 2018-12-06T04:00:19Z

hey @RaphaelDrai, sorry just saw your comment; based on the comments from @raymondfeng and @bajtos and discussion with @raymondfeng, I've decided to try the unique index approach for the FK relation property on the target model for a hasOne relation. With it, I plan to add the PATCH and DELETE operations and docs for the hasOne feature. Are you able to open a PR with your work on the example and its documentation? If so, we can definitely help you land it 👍

RaphaelDrai · 2018-12-06T05:43:33Z

Hello @b-admike,
thanks you for the update about the DELETE and PATCH operations, this is great... 🥇
Just for our internal knowledge, do you have a time estimation when they will be available?
Yes, we will be very happy to get your help of how creating a PR with our work and how to publish our hasOne example and documentation.

b-admike · 2018-12-10T13:23:40Z

Hello @b-admike,
thanks you for the update about the DELETE and PATCH operations, this is great... 🥇
Just for our internal knowledge, do you have a time estimation when they will be available?
Yes, we will be very happy to get your help of how creating a PR with our work and how to publish our hasOne example and documentation.

@RaphaelDrai Right now I'm using #2126 for enforcement of uniqueness on the target model instead of marking the foreign key as a primary key in the current design. @bajtos @raymondfeng and I had a discussion and since the memory connector and cloudant connector do not support unique indexes, we've decided to have the first release of hasOne provide "weak" relations i.e. ease of navigation and aggregation instead of enforcement of referential integrity and document it so users are aware of the limitations. We will set the metadata for the foreign key to be unique index, but it is up to users to run automigrate and if not supported, to set up the referential integrity in their collections/tables. I'm working on DELETE and PATCH operations at the moment in another branch as well as supporting unique indexes for memory connector (see loopbackio/loopback-datasource-juggler#1672) and I would like to ideally wrap them up this week. Can you let us know a bit more about your use case for the hasOne relation? I.e. what are your tables defined as and what is your expectation to enforce unique constraint?

RaphaelDrai · 2018-12-10T16:16:24Z

Hello @b-admike,
thanks you for the update about the DELETE and PATCH operations, this is great... 🥇
Just for our internal knowledge, do you have a time estimation when they will be available?
Yes, we will be very happy to get your help of how creating a PR with our work and how to publish our hasOne example and documentation.

@RaphaelDrai Right now I'm using #2126 for enforcement of uniqueness on the target model instead of marking the foreign key as a primary key in the current design. @bajtos @raymondfeng and I had a discussion and since the memory connector and cloudant connector do not support unique indexes, we've decided to have the first release of hasOne provide "weak" relations i.e. ease of navigation and aggregation instead of enforcement of referential integrity and document it so users are aware of the limitations. We will set the metadata for the foreign key to be unique index, but it is up to users to run automigrate and if not supported, to set up the referential integrity in their collections/tables. I'm working on DELETE and PATCH operations at the moment in another branch as well as supporting unique indexes for memory connector (see strongloop/loopback-datasource-juggler#1672) and I would like to ideally wrap them up this week. Can you let us know a bit more about your use case for the hasOne relation? I.e. what are your tables defined as and what is your expectation to enforce unique constraint?

Thanks you for your detailed and helpful answer.
Our particular use case for the 'hasOne' relation is creating a connection from a model let's say "Country" with another model let's say "Capital City". The relation should follow the "one-to-one" relation where a given "Country" can have only one "Capital City" and vice versa, a given "Capital City" can have (belongs to) only one "Country".

I read the previous messages about "belongsToUnique" decorator. I think that it could be helpful to have the option to use "belongsTo" or "belongsToUnique" in 'hasOne' relation.
For example if we use the decorator "belongsToUnique" it will address our use case above where in both directions the one-to-one relation is preserved and ensured.
But if we use the decorator "belongsTo" it will address the use case of one-to-one in one direction, and in the opposite direction we can allow one-to-many.
Example of this use case is a data model "Customer" connected to a data model "Orders" using the 'hasOne' relation. A given "Order" can have only one Customer, this is enforced with the 'belongsToUnique', but in the opposite direction, "Customer" can have many orders.

b-admike · 2018-12-10T22:03:16Z

Thanks you for your detailed and helpful answer.
Our particular use case for the 'hasOne' relation is creating a connection from a model let's say "Country" with another model let's say "Capital City". The relation should follow the "one-to-one" relation where a given "Country" can have only one "Capital City" and vice versa, a given "Capital City" can have (belongs to) only one "Country".

I read the previous messages about "belongsToUnique" decorator. I think that it could be helpful to have the option to use "belongsTo" or "belongsToUnique" in 'hasOne' relation.
For example if we use the decorator "belongsToUnique" it will address our use case above where in both directions the one-to-one relation is preserved and ensured.
But if we use the decorator "belongsTo" it will address the use case of one-to-one in one direction, and in the opposite direction we can allow one-to-many.
Example of this use case is a data model "Customer" connected to a data model "Orders" using the 'hasOne' relation. A given "Order" can have only one Customer, this is enforced with the 'belongsToUnique', but in the opposite direction, "Customer" can have many orders.

Thanks for your feedback @RaphaelDrai. Those sound like good use cases. Unfortunately, I'm going to revert belongsToUnique implementation since it marks the foreign key as a primary key on the target model (in this case Capital City). We have agreed to let our users/DBAs set up the unique constraint(s) for the relation and provide the "weak" relation I've mentioned above. I have a PR on the way and I will ping you as well to give your feedback/review.

b-admike requested review from bajtos and raymondfeng as code owners November 9, 2018 16:46

b-admike changed the base branch from master to feat/hasone-relation November 9, 2018 16:49

b-admike changed the title ~~[WIP Spike PoC]: Use target model PK for hasOne relation to enforce uniqueness~~ [WIP PoC]: Use target model PK for hasOne relation to enforce uniqueness Nov 9, 2018

b-admike mentioned this pull request Nov 21, 2018

[Roadmap] 🚀1Q2019 🚀 #1839

Closed

20 tasks

b-admike force-pushed the feat/hasone-relation branch from 2b6221b to fd03a78 Compare November 22, 2018 06:29

bajtos reviewed Nov 22, 2018

View reviewed changes

packages/repository/test/acceptance/has-one.relation.acceptance.ts Outdated Show resolved Hide resolved

packages/repository/test/fixtures/models/address.model.ts Outdated Show resolved Hide resolved

b-admike changed the base branch from feat/hasone-relation to master November 22, 2018 18:51

b-admike force-pushed the spike/hasone-relation branch from ca49f91 to 3c96fea Compare November 22, 2018 18:59

bajtos reviewed Nov 23, 2018

View reviewed changes

packages/repository/src/relations/has-one/index.ts Outdated Show resolved Hide resolved

This was referenced Nov 27, 2018

Atomic "find or create" Repository API #1956

Closed

feat(repository): initial AtomicCrudRepository implementation #1974

Closed

b-admike force-pushed the spike/hasone-relation branch from 3c96fea to 93384d9 Compare November 27, 2018 16:58

b-admike mentioned this pull request Nov 29, 2018

feat(repository): hasOne relation #1879

Closed

7 tasks

b-admike changed the title ~~[WIP PoC]: Use target model PK for hasOne relation to enforce uniqueness~~ feat(repository): hasOne relation Nov 29, 2018

bajtos reviewed Nov 29, 2018

View reviewed changes

bajtos reviewed Nov 30, 2018

View reviewed changes

examples/todo-list/src/controllers/todo-list-image.controller.ts Outdated Show resolved Hide resolved

bajtos approved these changes Nov 30, 2018

View reviewed changes

b-admike force-pushed the spike/hasone-relation branch 2 times, most recently from 3e3a9ac to 9dea156 Compare December 3, 2018 07:34

bajtos approved these changes Dec 3, 2018

View reviewed changes

jannyHou reviewed Dec 4, 2018

View reviewed changes

biniam added 2 commits December 4, 2018 00:18

feat(repository): hasOne relation

7f70e02

chore: add has one relation for todo-list example

28e9ff1

b-admike force-pushed the spike/hasone-relation branch from a971555 to e128d86 Compare December 4, 2018 06:09

b-admike force-pushed the spike/hasone-relation branch from e128d86 to b9c51b3 Compare December 4, 2018 06:20

feat(repository): add belongsToUniquely sugar syntax method

e57d9c1

b-admike force-pushed the spike/hasone-relation branch from b9c51b3 to e57d9c1 Compare December 4, 2018 06:40

b-admike merged commit 1b5b66a into master Dec 4, 2018

b-admike deleted the spike/hasone-relation branch December 4, 2018 06:55

raymondfeng reviewed Dec 4, 2018

View reviewed changes

b-admike mentioned this pull request Dec 6, 2018

fix(repository): use unique indexes for hasOne FK constraint #2126

Closed

7 tasks

b-admike mentioned this pull request Dec 12, 2018

fix(repository): revert hasOne target FK as PK implementation #2147

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository): hasOne relation #2005

feat(repository): hasOne relation #2005

b-admike commented Nov 9, 2018 •

edited

Loading

bajtos commented Nov 12, 2018

bajtos left a comment

bajtos Nov 29, 2018

b-admike Dec 4, 2018

bajtos left a comment

bajtos Dec 3, 2018

bajtos Dec 3, 2018

bajtos Dec 3, 2018

b-admike Dec 3, 2018

bajtos Dec 3, 2018

jannyHou left a comment

b-admike commented Dec 4, 2018

raymondfeng left a comment

raymondfeng Dec 4, 2018

raymondfeng Dec 4, 2018

bajtos Dec 4, 2018

raymondfeng Dec 4, 2018

bajtos Dec 4, 2018

raymondfeng Dec 4, 2018

raymondfeng Dec 4, 2018

bajtos Dec 4, 2018

RaphaelDrai commented Dec 5, 2018

b-admike commented Dec 6, 2018

RaphaelDrai commented Dec 6, 2018

b-admike commented Dec 10, 2018

RaphaelDrai commented Dec 10, 2018

b-admike commented Dec 10, 2018

feat(repository): hasOne relation #2005

feat(repository): hasOne relation #2005

Conversation

b-admike commented Nov 9, 2018 • edited Loading

Checklist

bajtos commented Nov 12, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

b-admike commented Dec 4, 2018

raymondfeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaphaelDrai commented Dec 5, 2018

b-admike commented Dec 6, 2018

RaphaelDrai commented Dec 6, 2018

b-admike commented Dec 10, 2018

RaphaelDrai commented Dec 10, 2018

b-admike commented Dec 10, 2018

b-admike commented Nov 9, 2018 •

edited

Loading