feat(repository): delete & patch operations for HasOne relation #2414

RaphaelDrai · 2019-02-17T17:11:23Z

implemented the missing crud operations delete, patch and put operations
in relation hasOne.

"fix #2233"

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

elv1s

My comments are mainly around handling primary keys. Thanks @RaphaelDrai

packages/repository/src/model.ts

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

bajtos

Great start!

I am proposing to remove the PUT endpoint from this pull request, it requires more discussion and non-trivial implementation.

packages/repository/src/__tests__/acceptance/has-one.relation.acceptance.ts

packages/repository/src/model.ts

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

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

bajtos

Nice progress!

packages/repository/src/__tests__/acceptance/has-one.relation.acceptance.ts

packages/repository/src/model.ts

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

bajtos · 2019-03-14T15:09:29Z

Actually, the hasOne relation allows to create two targets for a same source. This is what I wrote in my previous comments. In my opinion, delete operation shall remove all targets associated to a same source. When we will fix this issue to avoid to create two targets for a same source (new PR) then we can remove the Where as you recommend.
What do you think, can we keep the where till the fix?

We were discussing referential integrity for relations in #2127, with two conclusions:

There are two kinds of relations: weak and strong. Weak relations are for navigating between models only (think of GraphQL). Strong relations enforce referential integrity.
It's too difficult (if possible at all) to enforce referential integrity at LoopBack side. Our relations require the database to provide integrity guarantees. To support different programming models (especially in NoSQL databases), we want to investigate different relation types (HasMany vs. ReferencesMany, etc.). See Strong relations with referential integrity #2331 and the related issue (if you don't have ZenHub extension, then you can view the Epic stories here). I think the following issues may be related for this pull request:
- Memory connector should enforce referential integrity Memory connector should enforce referential integrity #2333
- Database migration should create SQL constraints for relations Database migration should create SQL constraints for relations #2332
- Document hasMany/belongsTo/hasOne limitations for NoSQL databases Document hasMany/belongsTo/hasOne limitations for NoSQL databases #2340

Now if you are using HasOne with PostgreSQL (or any other SQL database), then we require you to configure FOREIGN KEY and UNIQUE constraints to enforce HasOne constraints.

In my opinion, delete operation shall remove all targets associated to a same source.

I guess this makes sense. However, I don't understand how is the where parameter need for that? How do you envision its usage? IMO, we should simple delete all target models belonging to the source model instance.

When we will fix this issue to avoid to create two targets for a same source (new PR) then we can remove the Where as you recommend.

This won't be really fixed in LoopBack. We want to improve our auto-migration to automatically define SQL constraints as needed by relations (see #2332), and also improve our memory connector to enforce such constraints too (see #2333), but I am not sure if that can be considered as a fix?

bajtos · 2019-03-14T15:12:02Z

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

+    const targetRepository = await this.getTargetRepository();
+    return await targetRepository.updateAll(
+      constrainDataObject(dataObject, this.constraint),
+      constrainWhere({}, this.constraint as Where<TargetEntity>),


Similarly here, why is the explicit cast to Where<TargetEntity> needed? Can we find a way that would not require it?

@RaphaelDrai PTAL, can we remove this explicit type cast here?

Do we need to call constrainWhere at all? I think we can use this.constraint directly.

Suggested change

constrainWhere({}, this.constraint as Where<TargetEntity>),

this.constraint,

bajtos

The patch looks mostly good now. I'd like to see two changes before we land it:

Remove the where argument from delete method.
Check if we can remove as Were<TargetEntity> cast, it looks like a code smell to me.

packages/repository/src/__tests__/acceptance/has-one.relation.acceptance.ts

bajtos · 2019-03-18T12:25:39Z

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

+import { Getter } from '@loopback/context';
+import { DataObject, Options, Count } from '../../common-types';
+import { Entity } from '../../model';
+import { Filter, Where } from '../../query';


We don't add spaces around braces, please run npm run lint:fix.

bajtos · 2019-03-18T12:27:11Z

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

+    const targetRepository = await this.getTargetRepository();
+    return await targetRepository.updateAll(
+      constrainDataObject(dataObject, this.constraint),
+      constrainWhere({}, this.constraint as Where<TargetEntity>),


@RaphaelDrai PTAL, can we remove this explicit type cast here?

bajtos · 2019-03-18T12:28:14Z

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

@@ -87,4 +103,19 @@ export class DefaultHasOneRepository<
    }
    return found[0];
  }
+  async delete(options?: Options): Promise<Count> {
+    const targetRepository = await this.getTargetRepository();
+    return targetRepository.deleteAll(options);


This will delete ALL target model instances, not only the instance associated with the source model instance.

Please add a test to verify that delete preserves instances that are not related to the source model and then fix the implementation.

I think this is something we should verify for the PATCH operation too. Please add another test to check that PATCH is updating only the single instance on the other end of the relation.

Yet another two tests to consider: what happens when there is no target model instance? IMO, both delete and patch should run successfully and return {count: 0}.

Hi,
I did what you requested to remove the where argument etc... Note that I implemented hasOne same as hasMany is implemented and in hasMany there is no any test to verify the associated target model, the relation already handle it. Please see how hasMany is implement. If your assumption is right the same issue is existing in hasMany. But, when I made tests in hasOne with my initial code including PUT, hasOne worked as expected.
Just to keep you informed, someone else will continue the updates of hasOne with you.
I am moving to other responsibilities.
Kind regards
Raphael

I am afraid we misunderstood each other. I was asking to remove where argument from the public API, not from the underlying implementation...

Just to keep you informed, someone else will continue the updates of hasOne with you.
I am moving to other responsibilities.

Thank you for the update, I wish you good luck on your new assignment 🍀

bajtos · 2019-03-19T13:46:06Z

To speed up landing of this pull request, I made the remaining changes myself in 1e199e8.

@raymondfeng @b-admike LGTY?

elv1s

LGTM

bajtos · 2019-03-21T14:41:55Z

Landed, thank you for the contribution!

RaphaelDrai requested review from bajtos and raymondfeng as code owners February 17, 2019 17:11

RaphaelDrai mentioned this pull request Feb 17, 2019

feat(repository): delete operation in hasOne relation and example hasOne #2347

Closed

7 tasks

dhmlau added the community-contribution label Feb 20, 2019

dhmlau requested a review from b-admike February 20, 2019 20:31

elv1s suggested changes Feb 26, 2019

View reviewed changes

bajtos suggested changes Mar 1, 2019

View reviewed changes

dhmlau assigned RaphaelDrai Mar 2, 2019

bajtos added feature Relations Model relations (has many, etc.) labels Mar 5, 2019

bajtos suggested changes Mar 11, 2019

View reviewed changes

bajtos changed the title ~~feat(repository): implemented crud operations in hasone relation~~ feat(repository): delete & patch operations for HasOne relation Mar 12, 2019

bajtos reviewed Mar 14, 2019

View reviewed changes

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

bajtos suggested changes Mar 18, 2019

View reviewed changes

bajtos force-pushed the hasOneCRUD branch from 7c5163d to 1e199e8 Compare March 19, 2019 13:45

bajtos approved these changes Mar 19, 2019

View reviewed changes

bajtos requested a review from elv1s March 19, 2019 13:49

bajtos self-assigned this Mar 19, 2019

elv1s approved these changes Mar 19, 2019

View reviewed changes

feat(repository): add PATCH and DELETE for HasOne relation

f130dbb

bajtos force-pushed the hasOneCRUD branch from 1f8df53 to f130dbb Compare March 21, 2019 12:52

bajtos merged commit 5936fb9 into loopbackio:master Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository): delete & patch operations for HasOne relation #2414

feat(repository): delete & patch operations for HasOne relation #2414

RaphaelDrai commented Feb 17, 2019 •

edited

Loading

elv1s left a comment

bajtos left a comment

bajtos left a comment

bajtos commented Mar 14, 2019

bajtos Mar 14, 2019

RaphaelDrai Mar 17, 2019

bajtos Mar 18, 2019

bajtos Mar 18, 2019

bajtos left a comment •

edited

Loading

bajtos Mar 18, 2019

bajtos Mar 18, 2019

bajtos Mar 18, 2019

RaphaelDrai Mar 19, 2019

bajtos Mar 19, 2019

bajtos commented Mar 19, 2019

elv1s left a comment

bajtos commented Mar 21, 2019

	constrainWhere({}, this.constraint as Where<TargetEntity>),
	this.constraint,

feat(repository): delete & patch operations for HasOne relation #2414

feat(repository): delete & patch operations for HasOne relation #2414

Conversation

RaphaelDrai commented Feb 17, 2019 • edited Loading

Checklist

elv1s left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Mar 14, 2019

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 left a comment • 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

Choose a reason for hiding this comment

bajtos commented Mar 19, 2019

elv1s left a comment

Choose a reason for hiding this comment

bajtos commented Mar 21, 2019

RaphaelDrai commented Feb 17, 2019 •

edited

Loading

bajtos left a comment •

edited

Loading