feat(repository): implement inclusionResolver for hasMany relation #3595

agnes512 · 2019-08-23T18:24:13Z

resolves #3447

This PR implements the inclusionResolver for hasMany relation:

Add a helper flattenTargetsOfOneToManyRelation and its unit tests for hasMany
Add a new function createHasManyInclusionResolver to hasMany factory.
Add tests for hasManyInclusionResolver in repository-tests.
Add docs to explain the idea of hasManyInclusionResolver, and the basic setup/usages.

Suggested way to review:
-> start with the document file and hasmany-inclusion-resolver.acceptance.ts to get the idea of the inclusion resolver
-> review has-many.inclusion-resolver.ts to see the implementation details.
-> review the rest of files.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

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

👉 Check out how to submit a PR 👈

packages/repository-tests/src/crud-test-suite.ts

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

docs/site/HasMany-relation.md

emonddr · 2019-09-11T13:56:38Z

docs/site/HasMany-relation.md

+]
+```
+
+- You can delete a relation from `inclusionResolvers` to disable the inclusion


What is this doing exactly? It is not clear to me.

Just to show the way to disable a certain inclusion resolver. Because not all models are traversable.

...ges/repository/src/__tests__/unit/repositories/relations-helpers/build-lookup-map.helpers.ts

bajtos

The changes look reasonable at very high level, let's improve implementation details now.

docs/site/HasMany-relation.md

bajtos · 2019-09-12T14:56:07Z

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

+            `Invalid "filter.include" entries: {"relation":"orders"}`,
+          );
+          // reset
+          customerRepo.inclusionResolvers.set(


This won't work. If await expect above throws, then this line is never called.

I believe it should be safe to simply remove this line, because orders resolver is always initialized from the beforeEach hook (see lines 74-77 above).

This won't work. If await expect above throws, then this line is never called.

I believe it should be safe to simply remove this line, because orders resolver is always initialized from the beforeEach hook (see lines 74-77 above).

☝️

This comment haven't been addressed yet.

packages/repository-tests/src/crud/relations/fixtures/repositories/customer.repository.ts

...ges/repository/src/__tests__/unit/repositories/relations-helpers/build-lookup-map.helpers.ts

...sts__/unit/repositories/relations-helpers/flatten-targets-of-one-to-many-relation.helpers.ts

...ges/repository/src/__tests__/unit/repositories/relations-helpers/build-lookup-map.helpers.ts

...sts__/unit/repositories/relations-helpers/flatten-targets-of-one-to-many-relation.helpers.ts

packages/repository/src/relations/has-many/has-many.helpers.ts

agnes512 · 2019-09-13T14:57:22Z

Helpers for the inclusion resolver #3727

keyFrom is implemented in PR #3726

bajtos

The implementation looks good, let's improve the tests please.

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

bajtos · 2019-09-19T14:07:09Z

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

+            `Invalid "filter.include" entries: {"relation":"orders"}`,
+          );
+          // reset
+          customerRepo.inclusionResolvers.set(


This won't work. If await expect above throws, then this line is never called.

I believe it should be safe to simply remove this line, because orders resolver is always initialized from the beforeEach hook (see lines 74-77 above).

☝️

This comment haven't been addressed yet.

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

bajtos · 2019-09-19T14:16:03Z

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

+          expect(toJSON(result)).to.deepEqual(toJSON(expected));
+        });
+
+        it('returns related instances to target models via findById() method', async () => {


Please introduce a new describe block to group findById tests and apply the comments I posted for find tests.

I removed find* from all titles as my intention was to show different use cases instead of testing find* here.

bajtos · 2019-09-19T14:17:37Z

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

+          ];
+          expect(toJSON(result)).to.deepEqual(toJSON(expected));
+        });
+


I am missing the following test case for find method:

Create two customers

Create one order for each customer

Run find with a filter limiting the results to only one of the customers

Verify that data from the other customer (customer, order) did not leak into the results

I test this case in that findById() test case. Checks the result has correct instance and its related models.
Since I don't want the test cases in this file look like testing find* methods, I will reword the titles to show the intention of each test case.

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts

bajtos · 2019-09-20T12:33:14Z

packages/repository-tests/src/crud/relations/helpers.ts

  // get the repository class and create a new instance of it
  const customerRepoClass = createCustomerRepo(repositoryClass);
  const customerRepo: CustomerRepository = new customerRepoClass(
    db,
    async () => orderRepo,
    async () => addressRepo,
  );
+  // register the inclusionResolvers here for customerRepo
+


Let's add the newline before the comment. I feel the comment belongs to registration of inclusion resolvers below, not to new customerRepoClass code above.

const customerRepo: CustomerRepository = new customerRepoClass( db, async () => orderRepo, async () => addressRepo, ); // register the inclusionResolvers here for customerRepo customerRepo.inclusionResolvers.set( 'orders', customerRepo.orders.inclusionResolver, );

raymondfeng · 2019-09-20T22:05:27Z

docs/site/HasMany-relation.md

+    );
+
+    // add this line to register inclusion resolver
+    this.registerInclusion('orders', this.orders.inclusionResolver);


I'm wondering if we can make it even simpler as we can use orders to get the inclusionResolver (this['orders'].inclusionResolver).

this.registerInclusion('orders');

Good point. I think it's doable.
Let me land this PR and open a new one for it.

raymondfeng · 2019-09-20T22:07:39Z

@agnes512 Please fix CI failures.

Co-authored-by: Nora <[email protected]> Co-authored-by: Miroslav <[email protected]>

samarpanB · 2019-09-27T14:17:58Z

@agnes512 when can we expect this to be released ? I have taken a latest update but the inclusion resolver is not available there.

agnes512 · 2019-09-27T14:53:10Z

@samarpanB it will probably be released later today, worst case in 3-4 days 😄

samarpanB · 2019-09-27T15:29:42Z

Is it possible to release today? Actually I need to use it in one of my ongoing project. I don’t want to use the poor mans inclusion resolver 😊

agnes512 · 2019-09-27T20:06:15Z

@samarpanB we've just released! thanks for waiting~!

agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from 6bf98c6 to c74d6b6 Compare August 29, 2019 14:22

agnes512 force-pushed the hasmany-resolver branch 5 times, most recently from e2ba1c4 to fe46284 Compare September 6, 2019 02:34

bajtos reviewed Sep 6, 2019

View reviewed changes

packages/repository-tests/src/crud-test-suite.ts Outdated Show resolved Hide resolved

bajtos reviewed Sep 6, 2019

View reviewed changes

...itory-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts Outdated Show resolved Hide resolved

agnes512 force-pushed the hasmany-resolver branch 6 times, most recently from c545033 to 6866dd7 Compare September 10, 2019 23:49

agnes512 marked this pull request as ready for review September 10, 2019 23:50

agnes512 requested a review from raymondfeng as a code owner September 10, 2019 23:50

emonddr suggested changes Sep 11, 2019

View reviewed changes

agnes512 force-pushed the hasmany-resolver branch 4 times, most recently from 951730a to 105385f Compare September 11, 2019 19:50

nabdelgadir force-pushed the hasmany-resolver branch from 105385f to 90ecdb9 Compare September 11, 2019 22:12

agnes512 force-pushed the hasmany-resolver branch from e1c19f9 to b77f8a9 Compare September 12, 2019 14:49

bajtos suggested changes Sep 12, 2019

View reviewed changes

agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from ff03493 to aeda4b0 Compare September 17, 2019 21:55

agnes512 force-pushed the hasmany-resolver branch from aeda4b0 to 7243075 Compare September 17, 2019 23:10

agnes512 requested review from emonddr and bajtos September 18, 2019 14:25

agnes512 force-pushed the hasmany-resolver branch 2 times, most recently from 7786594 to 3ca0cc6 Compare September 18, 2019 21:48

bajtos suggested changes Sep 19, 2019

View reviewed changes

agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from 061af6b to d22962d Compare September 20, 2019 02:31

bajtos approved these changes Sep 20, 2019

View reviewed changes

agnes512 force-pushed the hasmany-resolver branch 6 times, most recently from ecca712 to 2b208b7 Compare September 20, 2019 21:11

raymondfeng reviewed Sep 20, 2019

View reviewed changes

raymondfeng approved these changes Sep 20, 2019

View reviewed changes

feat(repository): implement inclusionResolver for hasMany

b99c96b

Co-authored-by: Nora <[email protected]> Co-authored-by: Miroslav <[email protected]>

agnes512 force-pushed the hasmany-resolver branch from 888bdcd to b99c96b Compare September 20, 2019 23:22

agnes512 merged commit 4cf9a70 into master Sep 20, 2019

agnes512 deleted the hasmany-resolver branch September 20, 2019 23:37

agnes512 mentioned this pull request Jul 16, 2020

Implement InclusionResolver for hasManyThrough relation #5946

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository): implement inclusionResolver for hasMany relation #3595

feat(repository): implement inclusionResolver for hasMany relation #3595

agnes512 commented Aug 23, 2019 •

edited

Loading

emonddr Sep 11, 2019

agnes512 Sep 11, 2019

bajtos left a comment

bajtos Sep 12, 2019

bajtos Sep 19, 2019

agnes512 commented Sep 13, 2019 •

edited

Loading

bajtos left a comment

bajtos Sep 19, 2019

bajtos Sep 19, 2019

agnes512 Sep 20, 2019

bajtos Sep 19, 2019

agnes512 Sep 20, 2019 •

edited

Loading

bajtos Sep 20, 2019

raymondfeng Sep 20, 2019

agnes512 Sep 20, 2019

raymondfeng commented Sep 20, 2019

samarpanB commented Sep 27, 2019

agnes512 commented Sep 27, 2019

samarpanB commented Sep 27, 2019

agnes512 commented Sep 27, 2019

feat(repository): implement inclusionResolver for hasMany relation #3595

feat(repository): implement inclusionResolver for hasMany relation #3595

Conversation

agnes512 commented Aug 23, 2019 • edited Loading

Checklist

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

agnes512 commented Sep 13, 2019 • edited Loading

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

agnes512 Sep 20, 2019 • 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

raymondfeng commented Sep 20, 2019

samarpanB commented Sep 27, 2019

agnes512 commented Sep 27, 2019

samarpanB commented Sep 27, 2019

agnes512 commented Sep 27, 2019

agnes512 commented Aug 23, 2019 •

edited

Loading

agnes512 commented Sep 13, 2019 •

edited

Loading

agnes512 Sep 20, 2019 •

edited

Loading