feat(repository): hasOne relation #1879

b-admike · 2018-10-19T07:21:44Z

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

Fixes #1422

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

Great start!

bajtos · 2018-10-19T12:45:33Z

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

+   * @param options Options for the operations
+   * @returns A promise of the target object or null if not found.
+   */
+  findOne(filter?: Filter<Target>, options?: Options): Promise<Target | null>;


Let's replace find and findOne methods with a single method called get.

get(options?: Options): Promise<Target | undefined>;

Since there is always at most one target model instance associated with the source model instance, it feels more natural for me to get that single instance than findOne. I am also not sure what the filter argument would be good for, because I see very little value in filtering a set of at most single item.

On the second thought, maybe we should use find(option) instead of get(options) to indicate that the method may return undefined when the target does not exist, as opposed to throwing an EntityNotFound exception.

I think we should also consider the alternative where get(options) actually throws when the target model was not found, because that's what CRUD repository methods findById, patchById, etc. do.

Yeah I thought about not needing filter argument too, then thought that someone might want to use the fields key in filter to get the instance with certain properties (maybe not a good use case).

I think we should also consider the alternative where get(options) actually throws when the target model was not found, because that's what CRUD repository methods findById, patchById, etc. do.

These methods throw the EntityNotFound error which would require an id to be present and in this case we won't be given one for the method. For that reason, I think naming it get(options) and returning undefined sounds like a solid approach IMO.

Yeah I thought about not needing filter argument too, then thought that someone might want to use the fields key in filter to get the instance with certain properties (maybe not a good use case).

That's a good use case. Can we mark filter as accepting Filter properties without where? See Exclude<T, U> in TypeScript's Advanced Types.

These methods throw the EntityNotFound error which would require an id to be present

I have run into this problem while encountering belongsTo relation too. I think it's a clear sign that we need a way how to improve EntityNotFoundError to support a where filter in addition to the id value. Such change would be out of scope of this pull request though.

That's a good use case. Can we mark filter as accepting Filter properties without where? See Exclude<T, U> in TypeScript's Advanced Types.

Yes, this approach sounds good to me 👍 and for EntityNotFound, we can create a follow-up task for those enhancements.

bajtos · 2018-10-19T12:46:41Z

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

+  ): Promise<TargetEntity> {
+    const targetRepository = await this.getTargetRepository();
+    return targetRepository.create(
+      constrainDataObject(targetModelData, this.constraint),


I am afraid this is not enough to ensure there is never more than one model linked.

Please add a test that calls .create() two times and verifies that the second create is rejected.

This is the main difference between HasOne and HasMany.

Great catch; I'm working on this right now, and was wondering, where we should have validation in place to reject subsequent requests. If we do it at this level, then we'd probably have to make a query first to see if there is an existing instance IIUC. Thoughts?

Could you please check how is HasOne implemented in LB 3.x?

https://github.com/strongloop/loopback-datasource-juggler/blob/f7036b2837578348207999d8c31d88d624af4251/lib/relation-definition.js#L1791-L1834

bajtos · 2018-10-19T12:47:00Z

packages/repository/src/relations/relation.types.ts

+  /**
+   * The foreign key used by the target model.
+   *
+   * E.g. when a Customer has many Order instances, then keyTo is "customerId".


"has one", not "has many"

bajtos · 2018-10-19T12:47:53Z

packages/repository/src/repositories/repository.ts

+   * @param options Options for the operations
+   * @returns A promise of the target object or null if not found.
+   */
+  findOne(filter?: Filter<T>, options?: Options): Promise<T | null>;


We should be using undefined instead of null because it works better with ES6 default parameters.

bajtos · 2018-10-19T12:50:46Z

coverage/coveralls — Coverage decreased (-55.1%) to 34.7%

This is scary! I don't think your changes would account for such a huge drop in code coverage numbers, but at the same time we will need to fix this before landing your PR.

b-admike · 2018-10-19T13:57:31Z

coverage/coveralls — Coverage decreased (-55.1%) to 34.7%

This is scary! I don't think your changes would account for such a huge drop in code coverage numbers, but at the same time we will need to fix this before landing your PR.

Yeah that is huge indeed 😨. I will take a look at the details.

bajtos · 2018-10-23T12:21:44Z

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

+        'HasOne relation does not allow creation of more than one target model instance',
+      );
+    } else {
+      return await targetRepository.create(


It is crucial to leverage an atomic implementation of findOrCreate provided by our connectors. The current proposal is prone to race conditions, where to instances of the target "hasOne" model can be created.

Consider the following scenario: the LB4 server is handling two incoming HTTP requests to create a hasOne target and the code is executed in the following way by Node.js runtime:

Request 2 arrives, DefaultHasOneRepository#create is invoked

targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.

Request 1 arrives, DefaultHasOneRepository#create is invoked.

targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.

targetRepository.find for Request 1 returns, no model was found. targetRepository.create in called.

targetRepository.find for Request 2 returns, no model was found. targetRepository.create in called.

Both create requests eventually finish. We have two models that are a target of hasOne relation. 💥 💣 💥

I am proposing to put this pull request on hold and open a new pull request to add findOrCreate method to EntityCrudRepository (and the implementations).

Thank you for explaining the need for an atomic implementation with a great example. I agree, and I agree with doing that first.

I opened a new issue to keep track of "findOrCreate" story, see #1956

jannyHou · 2018-10-23T19:53:46Z

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

+
+  const sourceModel = relationMeta.source;
+  if (!sourceModel || !sourceModel.modelName) {
+    const reason = 'source model must be defined';


Is it ok to define reason twice in one function?
The first one is on https://github.com/strongloop/loopback-next/pull/1879/files#diff-a42257687a98a97022f7eb63398d2269R68

Yes, it's block-scoped.

jannyHou · 2018-10-23T20:50:10Z

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

+    throw new InvalidRelationError(reason, relationMeta);
+  }
+
+  return Object.assign(relationMeta, {keyTo: defaultFkName});


Do we allow override the default fk in the relationMeta? This line implies we don't?

Good catch. I believe we should allow users to provide their own keyTo value. @b-admike please add a test to ensure this use-case is supported.

Actually: this line is executed only when relationMeta.keyTo was falsey, see lines 72-75 above.

if (relationMeta.keyTo) { // The explict cast is needed because of a limitation of type inference return relationMeta as HasOneResolvedDefinition; }

jannyHou

@b-admike Great effort! The implementation looks similar to hasMany relation, from the design's perspective, would it be possible to make hasOne a more constrained version of hasMany? IMO essentially hasOne = hasMany + one more constraint in the create method.

Some proposal:

Add a configure property in the hasMany relation metadata to specify one-to-one relation.
Apply additional check in the hasManyRepository's create method to make sure one source instance only HAS ONE target instance.

In this case hasOne is like a sugar relation of hasMany.
Thought?

b-admike · 2018-10-25T03:51:19Z

@jannyHou Thank you for your valuable feedback. I do intend to refactor the code since these two relations have lots of similarities, but thought it would be better to start off like this and think about ways we can re-use common code.

Some proposal:

Add a configure property in the hasMany relation metadata to specify one-to-one relation.

Apply additional check in the hasManyRepository's create method to make sure one source instance only HAS ONE target instance.

I like this proposal. Are you thinking that the hasOne decorator set this configuration property and the rest of the flow is the same as hasMany relation? The only thing that is separate is the repository interface which would be its own standalone interface which would call on findOrCreate for create for instance.

bajtos · 2018-10-25T10:00:09Z

The implementation looks similar to hasMany relation, from the design's perspective, would it be possible to make hasOne a more constrained version of hasMany? IMO essentially hasOne = hasMany + one more constraint in the create method.

Let's explore!

Few constraints to keep in mind:

The constraint in create must be enforced in an atomic way (e.g. findOrCreate)
Many HasOneRepository methods have different signature from HasManyRepository, because there is always at most one model on the other side of the relation. Where has-many relation is loading/updating/deleting possibly multiple target instances, has-one always returns/updates a single instance only.

raymondfeng · 2018-11-06T19:36:36Z

Cross-posting from #1956 (comment)

RaphaelDrai · 2018-11-28T09:35:41Z

Hello @b-admike,
I have tested the "hasOne" feature from the github branch "feat/hasone-relation" in order to evaluate what is working and missing. Below my report:

I simulated a use case where we have a "hasOne" relation connection from a source model (parent) to a target model (child).
The relation indicates that a source model of a given instance (parent_id) cannot have more than one instance to the target model (child_id).
The feature is working as expected and we get an error if we attempt to create two children bind to a same parent.

Missing features:
When we attempt to create two targets to a same source instance the application generates response and log with following error:
Server side -> 500 Error: HasOne relation does not allow creation of more than one target model instance
Client side -> receives response with "statusCode": 500 and "message": "Internal Server Error"
I recommend to remove the server log and replace the current client error as following:
error code: 400 "Bad request"
message: source model cannot have two targets

_CRUD operations and features missing_:
	FIND
	DELETE
	PATCH

I am currently preparing a short documentation that details how to code an application example of "hasOne" relation. Please let me know where do you think that I can post it.
I need also to know if the missing features are completed in any given github branch and if not what is the plan for it.
Thanks you,
Raphael

b-admike · 2018-11-29T09:02:27Z

hi @RaphaelDrai thank you for taking the time to test out this branch and compiling your results. I've actually continued my work on the spike/hasone-relation branch (see #2005) instead of here since we decided to take the approach from there and it was easier for me to compare the changes there to master.

When we attempt to create two targets to a same source instance the application generates response and log with following error:
Server side -> 500 Error: HasOne relation does not allow creation of more than one target model instance
Client side -> receives response with "statusCode": 500 and "message": "Internal Server Error"

This should now be a Duplicate Entry Error since we use the database to determine the uniqueness of the foreign key for us (it is also the primary key on the target model) and marked with a 409 error code.

CRUD operations and features missing:
FIND
DELETE
PATCH

find or get is currently there, but PATCH and DELETE are not. I think we can add those two APIs. I've added a hasOne relation to the todo-list example to demonstrate the changes, so you can also test with that.

I am currently preparing a short documentation that details how to code an application example of "hasOne" relation. Please let me know where do you think that I can post it.

That's awesome. We can possibly use the documentation as a follow-up or part of #2005 for the relation I added to todo-list.

b-admike · 2018-11-29T09:05:28Z

Closing this in favour of #2005. Feel free to also copy/link your comment there @RaphaelDrai.

RaphaelDrai · 2018-12-02T11:32:46Z

Hello @b-admike ,
It sounds great, thanks you :-).
I will follow-up the todo-list documentation part that you have added in #2005 case and we will publish the documentation and code example of hasOne relation.
Two questions please:

Actually hasOne feature is considered as a show stopper in our loopback-4 application. Can you provide an estimation when the missing CRUD operations will be implemented and then merged ?
For help, I will be glad to contribute with the hasOne CRUD coding on any required part of the code that it is not covered yet. If it is possible, can you suggest me what contribution do you need that I can help ?
Kind regards,
Raphael

b-admike requested review from bajtos and raymondfeng as code owners October 19, 2018 07:21

bajtos suggested changes Oct 19, 2018

View reviewed changes

b-admike force-pushed the feat/hasone-relation branch 3 times, most recently from c358770 to 2b6221b Compare October 22, 2018 07:15

bajtos suggested changes Oct 23, 2018

View reviewed changes

jannyHou reviewed Oct 23, 2018

View reviewed changes

This was referenced Oct 25, 2018

Define models relations in JSON file or in JavaScript code #695

Closed

Atomic "find or create" Repository API #1956

Closed

bajtos mentioned this pull request Nov 9, 2018

feat: add authorization component #1205

Merged

7 tasks

b-admike mentioned this pull request Nov 21, 2018

[Roadmap] 🚀1Q2019 🚀 #1839

Closed

20 tasks

biniam added 3 commits November 22, 2018 01:29

feat(repository): hasOne relation

323221a

fixup! apply review feedback

d945369

fixup! apply feedback

fd03a78

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

b-admike mentioned this pull request Nov 29, 2018

feat(repository): hasOne relation #2005

Merged

9 tasks

b-admike closed this Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository): hasOne relation #1879

feat(repository): hasOne relation #1879

b-admike commented Oct 19, 2018 •

edited

Loading

bajtos left a comment

bajtos Oct 19, 2018

b-admike Oct 19, 2018

bajtos Oct 19, 2018

b-admike Oct 19, 2018

bajtos Oct 19, 2018

b-admike Oct 19, 2018

bajtos Oct 19, 2018

bajtos Oct 19, 2018

bajtos Oct 19, 2018

bajtos commented Oct 19, 2018

b-admike commented Oct 19, 2018

bajtos Oct 23, 2018 •

edited

Loading

b-admike Oct 25, 2018

bajtos Nov 1, 2018

jannyHou Oct 23, 2018 •

edited

Loading

raymondfeng Oct 23, 2018

jannyHou Oct 23, 2018

bajtos Oct 25, 2018

bajtos Oct 25, 2018

jannyHou left a comment

b-admike commented Oct 25, 2018

bajtos commented Oct 25, 2018

raymondfeng commented Nov 6, 2018

RaphaelDrai commented Nov 28, 2018 •

edited

Loading

b-admike commented Nov 29, 2018

b-admike commented Nov 29, 2018

RaphaelDrai commented Dec 2, 2018

feat(repository): hasOne relation #1879

feat(repository): hasOne relation #1879

Conversation

b-admike commented Oct 19, 2018 • edited Loading

Checklist

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

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

bajtos commented Oct 19, 2018

b-admike commented Oct 19, 2018

bajtos Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Oct 23, 2018 • edited Loading

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 Oct 25, 2018

bajtos commented Oct 25, 2018

raymondfeng commented Nov 6, 2018

RaphaelDrai commented Nov 28, 2018 • edited Loading

b-admike commented Nov 29, 2018

b-admike commented Nov 29, 2018

RaphaelDrai commented Dec 2, 2018

b-admike commented Oct 19, 2018 •

edited

Loading

bajtos Oct 23, 2018 •

edited

Loading

jannyHou Oct 23, 2018 •

edited

Loading

RaphaelDrai commented Nov 28, 2018 •

edited

Loading