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

feat(repository): hasOne relation #1879

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@b-admike
Copy link
Member

b-admike commented Oct 19, 2018

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

@b-admike b-admike requested review from bajtos and raymondfeng as code owners Oct 19, 2018

@bajtos
Copy link
Member

bajtos left a comment

Great start!

* @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>;

This comment has been minimized.

@bajtos

bajtos Oct 19, 2018

Member

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.

This comment has been minimized.

@b-admike

b-admike Oct 19, 2018

Member

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.

This comment has been minimized.

@bajtos

bajtos Oct 19, 2018

Member

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.

This comment has been minimized.

@b-admike

b-admike Oct 19, 2018

Member

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.

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

This comment has been minimized.

@bajtos

bajtos Oct 19, 2018

Member

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.

This comment has been minimized.

@b-admike

b-admike Oct 19, 2018

Member

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?

This comment has been minimized.

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

This comment has been minimized.

@bajtos

bajtos Oct 19, 2018

Member

"has one", not "has many"

* @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>;

This comment has been minimized.

@bajtos

bajtos Oct 19, 2018

Member

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

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Member

b-admike commented Oct 19, 2018

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.

@b-admike b-admike force-pushed the feat/hasone-relation branch 3 times, most recently from c358770 to 2b6221b Oct 22, 2018

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

This comment has been minimized.

@bajtos

bajtos Oct 23, 2018

Member

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:

  1. Request 2 arrives, DefaultHasOneRepository#create is invoked
  2. targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.
  3. Request 1 arrives, DefaultHasOneRepository#create is invoked.
  4. targetRepository.find is called. It's an async function so other stuff happens while the query is in progress.
  5. targetRepository.find for Request 1 returns, no model was found. targetRepository.create in called.
  6. targetRepository.find for Request 2 returns, no model was found. targetRepository.create in called.
  7. 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).

This comment has been minimized.

@b-admike

b-admike Oct 25, 2018

Member

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

This comment has been minimized.

@bajtos

bajtos Nov 1, 2018

Member

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


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

This comment has been minimized.

@jannyHou

jannyHou Oct 23, 2018

Contributor

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

This comment has been minimized.

@raymondfeng

raymondfeng Oct 23, 2018

Member

Yes, it's block-scoped.

throw new InvalidRelationError(reason, relationMeta);
}

return Object.assign(relationMeta, {keyTo: defaultFkName});

This comment has been minimized.

@jannyHou

jannyHou Oct 23, 2018

Contributor

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

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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.

This comment has been minimized.

@bajtos

bajtos Oct 25, 2018

Member

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

jannyHou left a comment

@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

This comment has been minimized.

Copy link
Member

b-admike commented Oct 25, 2018

@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

This comment has been minimized.

Copy link
Member

bajtos commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member

raymondfeng commented Nov 6, 2018

Cross-posting from #1956 (comment)

@bajtos bajtos referenced this pull request Nov 9, 2018

Open

[WIP PoC] feat: add authorization component #1205

3 of 7 tasks complete

@b-admike b-admike referenced this pull request Nov 21, 2018

Open

[Roadmap] 🚀1Q2019 🚀 #1839

11 of 21 tasks complete

b-admike added some commits Oct 17, 2018

@b-admike b-admike force-pushed the feat/hasone-relation branch from 2b6221b to fd03a78 Nov 22, 2018

@RaphaelDrai

This comment has been minimized.

Copy link

RaphaelDrai commented Nov 28, 2018

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

This comment has been minimized.

Copy link
Member

b-admike commented Nov 29, 2018

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 b-admike referenced this pull request Nov 29, 2018

Merged

feat(repository): hasOne relation #2005

6 of 9 tasks complete
@b-admike

This comment has been minimized.

Copy link
Member

b-admike commented Nov 29, 2018

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

@b-admike b-admike closed this Nov 29, 2018

@RaphaelDrai

This comment has been minimized.

Copy link

RaphaelDrai commented Dec 2, 2018

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:

  1. 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 ?
  2. 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment