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

RaphaelDrai · 2019-02-06T12:43:14Z

implemented delete new feature on hasone relation. created an example of one to one application into
examples folder

Implemented the DELETE operation in hasOne relation. which is part of the issue #2233.
I have added an example application for the hasOne relation under the folder:
examples/one-2-one
The next step, I will implement the PATCH then the PUT operations new features in the hasOne relation.

Fixes #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

implemeted delete new feature on hasone relation. created an example of one to one application into examples folder "fix loopbackio#2233"

bajtos

Thank you @RaphaelDrai for the pull request.

First of all, please add tests to cover your new delete method in HasOne repository.

I have mixed feelings about your new one-2-one example. IIRC, our intention was to add another model to examples/todo-list, for example "TodoList has one {some new model}". What are your arguments for adding an entirely new example? We must be cautious about maintenance costs - the more example apps we have, the more expensive it is to make changes related to project layout.

@b-admike you were working on HasOne relation in the past, please take a look at this pull request too.

bajtos · 2019-02-07T14:03:46Z

examples/one-2-one/package.json

+{
+  "name": "@loopback/example-one-2-one",
+  "version": "1.3.1",
+  "description": "Continuation of the hasOne example using relations in LoopBack 4.",


The description says "Continuation of the hasOne example" - which example do you mean by "hasOne example"?

Hello @bajtos , thanks you for your comments. I will add shortly the test of the delete operation. Just for information I already completed the PATCH operation as well, and I will commit it on next week. About the one-2-one example: the example implementation is fitting the documentation in LB4 for hasOne (https://loopback.io/doc/en/lb4/hasOne-relation.html) that describes Supplier and Account (by the way same as in LB3). "Todo-List has one" could make confusion with the existing "Todo-List" that implement hasMany relation. In my opinion and from my own experience as loopback new learner, better to have each relation with its own example to ease the learning and fit the documentation.

Makes sense 👍

Let's find a new better example name please. How about @loopback/example-hasOne-relation or maybe just @loopback/example-hasOne?

Or maybe @loopback/example-supplier-account?

Let's use a clearer description. How about "An example showing hasOne relation in LoopBack 4"?

bajtos · 2019-02-07T14:10:09Z

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

@@ -39,6 +40,14 @@ export interface HasOneRepository<Target extends Entity> {
    filter?: Pick<Filter<Target>, Exclude<keyof Filter<Target>, 'where'>>,
    options?: Options,
  ): Promise<Target>;
+
+  /**
+   * Delete multiple target model instances


This does not make sense to me. In HasOne relation, there is always either no target model or one target model. I don't see why would the caller want to delete multiple instances or provide a filter?

IMO, the API should look like this:

delete(options?: Options): Promise<Count>;

I am also not sure whether to return Count (0 = not found; 1 = deleted), or whether to use Boolean (false = not found; true = deleted). I guess it's good to stay consistent with other delete methods and return Count.

I agree hasOne is always either no target model or one target model. The problem is that actually there is no any check to avoid multiple targets as we had in the past. Adding multiple targets check was removed from the initial version that I tested a coupe of months ago. This is the reason why I kept the Count. Let me know if you want me to add the check that avoid to add multiple targets then I will return true or false as per your recommendation.
Do you agree with a separated example one-2-one application as I did?

I agree hasOne is always either no target model or one target model. The problem is that actually there is no any check to avoid multiple targets as we had in the past. Adding multiple targets check was removed from the initial version that I tested a coupe of months ago. This is the reason why I kept the Count.

I think we need to decouple the delete API (what parameters is the method accepting) from the value it returns.

Do we both agree that it does not make sense to provide a filter or where argument where deleting the other side of hasOne relation?

As for the return value, let's keep returning Count for consistency with other delete methods.

bajtos

Hi @RaphaelDrai, I think this pull request is a great start 👍

I see three major areas to improve:

Remove where parameter from the repository delete API.
Add tests to cover the new feature.
Improve the example, add tests.

It may be easier to split these changes into two pull requests: one PR to add delete method, another PR to add the new example app. If you agree, then please keep this PR for the example app and open a new PR for delete implementation. That way most of my comments below stay preserved and will be easier to verify.

bajtos · 2019-02-12T14:07:28Z

examples/one-2-one/.gitignore

@@ -0,0 +1,64 @@
+# Logs


We don't keep package-level .gitignore files and use a top-level .gitignore file in loopback-next monorepo instead. Please remove this file.

bajtos · 2019-02-12T14:08:15Z

examples/one-2-one/.yo-rc.json

@@ -0,0 +1 @@
+{}


We don't have .yo-rc.json in other example packages, can you please remove this file too?

bajtos · 2019-02-12T14:09:35Z

examples/one-2-one/package.json

+{
+  "name": "@loopback/example-one-2-one",
+  "version": "1.3.1",
+  "description": "Continuation of the hasOne example using relations in LoopBack 4.",


Let's use a clearer description. How about "An example showing hasOne relation in LoopBack 4"?

bajtos · 2019-02-12T14:10:26Z

examples/one-2-one/README.md

@@ -0,0 +1,3 @@
+# one2one


The README needs to be improved before this patch is landed. See our README guidelines.

I will remove my hasOne example as per your notification below.

bajtos · 2019-02-12T14:10:48Z

examples/one-2-one/package.json

+    "@loopback/repository": "^1.1.1",
+    "@loopback/rest": "^1.5.1",
+    "@loopback/rest-explorer": "^1.1.4",
+    "@loopback/service-proxy": "^1.0.5",


I think this dependency is not needed by this example app, can you remove it please?

bajtos · 2019-02-12T14:17:06Z

examples/one-2-one/src/controllers/supplier-account.controller.ts

+      },
+    },
+  })
+  async find(


Let's call this method get for consistency with the method name used by the repository.

bajtos · 2019-02-12T14:17:52Z

examples/one-2-one/src/controllers/supplier-account.controller.ts

+  })
+  async delete(
+    @param.path.string('id') id: string,
+    @param.query.object('where', getWhereSchemaFor(Account)) where?: Where,


As commented earlier, I think HasOne's delete should not accept any where condition. Let's simplify the REST API too.

bajtos · 2019-02-12T14:18:58Z

examples/one-2-one/test/acceptance/test-helper.ts

+export interface AppWithClient {
+  app: One2oneApplication;
+  client: Client;
+}


Please add acceptance-level tests for each controller you are adding to this example application:

SupplierController

SupplierAccountController

AccountController (if you decide to keep it)

bajtos · 2019-02-12T14:22:35Z

examples/one-2-one/tsconfig.json

+  "include": [
+    "src",
+    "test",
+    "index.ts"


We have recently reworked our project layout so that test files are kept in src/__tests__. Please rebase your pull request on top of the latest master and update your new example app to follow the new convention. See the following pull request for the changes you need to make: https://github.com/strongloop/loopback-next/pull/2316/files.

It basically boils down to:

move test to src/__tests__

update package.json scripts & files

update tsconfig.build.json

fix paths in import statements

I apologize for inconvenience I caused here.

bajtos · 2019-02-12T14:25:14Z

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

@@ -87,4 +96,12 @@ export class DefaultHasOneRepository<
    }
    return found[0];
  }
+
+  async delete(where?: Where<TargetEntity>, options?: Options): Promise<Count> {


Please add tests to cover this new repository method, see packages/repository/src/__tests__/acceptance/has-one.relation.acceptance.ts

bajtos · 2019-02-12T14:49:12Z

@RaphaelDrai I just found that our example/todo-list already comes with a has-one relation: a TodoList has one Image, see e.g. here:

https://github.com/strongloop/loopback-next/blob/d1d1995615c354e1ebf11062a4bced1b1a57f03c/examples/todo-list/src/repositories/todo-list.repository.ts#L43-L46

Given that we already have an example for HasOne, I am taking back my earlier comments where I said it's ok to create a new example for HasOne-only. Let's keep building on top of the existing example please.

implement hasone relation operations delete, patch and put "fix loopbackio#2233"

implemeted delete new feature on hasone relation. created an example of one to one application into examples folder "fix loopbackio#2233"

implement hasone relation operations delete, patch and put "fix loopbackio#2233"

…k-next into hasOneDelete

jannyHou

@RaphaelDrai Thank you for the contribution! Your branch contains several commits not related to this PR, it's a little bit hard to find the real code change, could you rebase and fix them?

bajtos · 2019-02-15T09:11:24Z

@RaphaelDrai I see a lot of unrelated commits from our master in your feature branch, it looks like GitHub thinks your pull request (feature branch) was started from an older commit in master and the history somehow diverged. I suspect you have incorrectly rebased your branch. Could you please fix it? See our instructions in How to rebase your branch.

Since most of the review comments were related to the new example app that's no longer part of this pull request, it may be easier for you to start fresh, create a new feature branch and open a new pull request. You can use git cherry-pick <sha> to move the relevant commits from this old branch to the new one. In general, we don't like when an old PR is closed a new one is opened, because it loses discussion history, but I feel this case is different because most of the comments are no longer relevant.

RaphaelDrai · 2019-02-17T07:53:24Z

Hi @bajtos and @jannyHou,
I will try again to to proceed with a new rebase of my branch.
If it will fail again I will create a new PR.
Thanks,
Raphael

RaphaelDrai · 2019-02-17T17:13:35Z

Hi @bajtos,
I created a new PR and committed the implementation.
Reference: #2414

RaphaelDrai · 2019-02-25T06:58:19Z

This PR was replaced with : #2414

bajtos · 2019-03-05T15:52:17Z

Closing in favor of #2414

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

9093ae0

implemeted delete new feature on hasone relation. created an example of one to one application into examples folder "fix loopbackio#2233"

RaphaelDrai requested review from bajtos and raymondfeng as code owners February 6, 2019 12:43

bajtos reviewed Feb 7, 2019

View reviewed changes

bajtos added feature community contribution Relations Model relations (has many, etc.) labels Feb 7, 2019

bajtos reviewed Feb 7, 2019

View reviewed changes

bajtos assigned b-admike and bajtos Feb 7, 2019

raymondfeng added 2 commits February 7, 2019 09:31

fix(rest): sanitize json for JSON.parse()

5042698

chore: update @types/debug to version 4.1.0

b5f12be

bajtos added community-contribution and removed community-contribution labels Feb 8, 2019

bajtos and others added 4 commits February 8, 2019 07:55

chore: exclude "good first issue" from stalebot

6ef5d85

fix: update to the most recent lodash version

65ee865

fix(docs): broken link in Crafting in LB

7707fa5

bajtos suggested changes Feb 12, 2019

View reviewed changes

raymondfeng and others added 9 commits February 12, 2019 07:33

chore: update p-event to version 2.3.1

914ad47

refactor(rest): use on-finished module instead of hand-written solution

49aeac9

feat(context): introduce async context observers for bind/unbind events

e5e5fc4

fix(rest): only return matched trie nodes with values

669ede1

fix(cli): do not install deps for built-in connectors

2e035a5

fix(docs): remove review questions section for Controller page

93f7bbf

feat(repository): implement crud operations: delete, patch and put

5b926e6

implement hasone relation operations delete, patch and put "fix loopbackio#2233"

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

6d9db67

implemeted delete new feature on hasone relation. created an example of one to one application into examples folder "fix loopbackio#2233"

feat(repository): implement crud operations: delete, patch and put

b55d55a

implement hasone relation operations delete, patch and put "fix loopbackio#2233"

Merge branch 'hasOneDelete' of https://github.com/RaphaelDrai/loopbac…

b266abb

…k-next into hasOneDelete

RaphaelDrai requested review from b-admike, hacksparrow, jannyHou and marioestradarosa as code owners February 13, 2019 14:41

jannyHou reviewed Feb 13, 2019

View reviewed changes

bajtos closed this Mar 5, 2019

NorthDecoder mentioned this pull request Jan 15, 2021

Delete with no query removes all records by default #7017

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RaphaelDrai commented Feb 6, 2019 •

edited

Loading

bajtos left a comment •

edited

Loading

bajtos Feb 7, 2019

RaphaelDrai Feb 7, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 7, 2019

RaphaelDrai Feb 7, 2019

bajtos Feb 12, 2019

bajtos left a comment •

edited

Loading

bajtos Feb 12, 2019

RaphaelDrai Feb 13, 2019

bajtos Feb 12, 2019

RaphaelDrai Feb 13, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

RaphaelDrai Feb 13, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

bajtos Feb 12, 2019

RaphaelDrai Feb 13, 2019

bajtos commented Feb 12, 2019

jannyHou left a comment

bajtos commented Feb 15, 2019

RaphaelDrai commented Feb 17, 2019

RaphaelDrai commented Feb 17, 2019

RaphaelDrai commented Feb 25, 2019

bajtos commented Mar 5, 2019

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

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

Conversation

RaphaelDrai commented Feb 6, 2019 • edited Loading

Checklist

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

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

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 Feb 12, 2019

jannyHou left a comment

Choose a reason for hiding this comment

bajtos commented Feb 15, 2019

RaphaelDrai commented Feb 17, 2019

RaphaelDrai commented Feb 17, 2019

RaphaelDrai commented Feb 25, 2019

bajtos commented Mar 5, 2019

RaphaelDrai commented Feb 6, 2019 •

edited

Loading

bajtos left a comment •

edited

Loading

bajtos left a comment •

edited

Loading