feat(repository): implement inclusionResolver for belongsTo relation #3721

agnes512 · 2019-09-12T13:44:57Z

resolves #3448

This PR implements the inclusionResolver for belongsTo relation:

Add a helper flattenTargetsOfOneToOneRelation and its unit tests for belongsTo and hasOne relation ( since they both use this helper to implement inclusion resolver)
Add a new function createBelongsToInclusionResolver into the belongsTo factory createBelongsToAccessorFor
Add tests for createBelongsToInclusionResolver in repository-tests.
- introduce a new test-suite flag supportsInclusionResolvers.
Add docs to explain the idea of belongsToInclusionResolver, and the basic setup/usages.

Suggested way to review:
-> start with the document file and belongs-to-inclusion-resolver.acceptance.ts to get the idea of the inclusion resolver.
-> review belongs-to.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/relations/acceptance/unit.test.relation.acceptance.ts

packages/repository/src/__tests__/unit/repositories/relations-helpers/uniq.helpers.unit.ts

bajtos

Most of my comments posted for hasMany relations apply to this pull request too, please read them here: #3595 (review)

packages/repository-tests/src/crud/relations/acceptance/unit.test.relation.acceptance.ts

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

Helpers for the inclusion resolver #3727

raymondfeng · 2019-09-18T15:42:20Z

docs/site/BelongsTo-relation.md

+through the following code:
+
+```ts
+addressRepo.find({include: [{relation: 'customer'}]});


Let's be consistent here, either use order belongsTo customer or address belongsTo customer. Personally I prefer order belongsTo customer.

Yeah I think that would be more consistent with other tests.

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

packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts

docs/site/BelongsTo-relation.md

...ory-tests/src/crud/relations/acceptance/belongs-to-inclusion-resolver.relation.acceptance.ts

...s/repository/src/__tests__/unit/repositories/relations-helpers/relations-helpers-fixtures.ts

packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts

bajtos

Most of the comments posted in #3595 (review) apply to this pull request too.

bajtos

I quickly skimmed through the changes and don't see any obvious problems. Please get at least one more approval before landing.

...ory-tests/src/crud/relations/acceptance/belongs-to.inclusion-resolver.relation.acceptance.ts

nabdelgadir · 2019-09-20T17:14:53Z

...s/repository/src/__tests__/unit/repositories/relations-helpers/relations-helpers-fixtures.ts

+  }
+}
+interface ManufacturerRelations {
+  products?: Product;


should be product?: ProductWithRelations

...s/repository/src/__tests__/unit/repositories/relations-helpers/relations-helpers-fixtures.ts

packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts

raymondfeng · 2019-09-20T19:03:42Z

...ory-tests/src/crud/relations/acceptance/belongs-to.inclusion-resolver.relation.acceptance.ts

+        description: "Odin's Coffee Maker",
+        customerId: odin.id,
+      });
+


Can we add a test case to cover scope? Such as:

{include: {relation: 'customer', scope: {fields: ['name']}}}

jannyHou · 2019-09-20T19:23:31Z

...s/repository/src/__tests__/unit/repositories/relations-helpers/relations-helpers-fixtures.ts

@@ -100,3 +161,7 @@ export function createCategory(properties: Partial<Category>) {
 export function createProduct(properties: Partial<Product>) {
  return new Product(properties as Product);
 }
+
+export function createManufacturer(properties: Partial<Manufacturer>) {
+  return new Manufacturer(properties as Manufacturer);


The constructor of Manufacturer class takes in a partial manufacturer on line, any reason why specify the type as Manufacturer again?

Before we were passing in a single value. Will remove

raymondfeng · 2019-09-20T19:31:33Z

...ory-tests/src/crud/relations/acceptance/belongs-to.inclusion-resolver.relation.acceptance.ts

+      });
+      expect(
+        orderRepo.find({include: [{relation: 'customer'}], limit: 1}),
+      ).to.be.rejectedWith(`Invalid "scope is not supported`);


Oh, so the initial version does not support scope?

BTW, I was referring to the scope inside include.

No. See #3443

jannyHou · 2019-09-20T19:40:23Z

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

+      // the order of sourceIds matters
+      const result = flattenTargetsOfOneToOneRelation(
+        [erasers.categoryId, pencil.categoryId, pen.categoryId],
+        [book, stationery, stationery],


Hmm, a question for flattenTargetsOfOneToOneRelation, does it mean if I provide the second parameter in a random order, like [stationery, book, stationery], it still returns an array of the results according to the order of [erasers.categoryId, pencil.categoryId, pen.categoryId]?

I am very confused about what this function does, could you elaborate more about it?

Sure. This API doc of this function:

Returns an array of instances. The order of arrays is based on the order of sourceIds

The second parameter is just a group of entities that we found by the foreign key (all related models), the order doesn't matter. So this function basically just matches each the passed in sourceId with its related instance.
In this case, if you pass in [pencil.categoryId, erasers.categoryId, pen.categoryId] as the first param, the result will be [stationery, book, stationery].
And you will also find a function called flattenTargetsOfOneToManyRelation in has many relation, it does the same thing except that one sourceId might have more than one instances.

jannyHou

👏 Great effort @agnes512 and @nabdelgadir on implementing the inclusion story! Mostly LGTM just a few nitpick and question.

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

agnes512 commented Sep 12, 2019

View reviewed changes

packages/repository-tests/src/crud/relations/acceptance/unit.test.relation.acceptance.ts Outdated Show resolved Hide resolved

agnes512 force-pushed the belongsto-resolver branch from ee9b621 to a2817b0 Compare September 12, 2019 15:03

agnes512 commented Sep 12, 2019

View reviewed changes

packages/repository/src/__tests__/unit/repositories/relations-helpers/uniq.helpers.unit.ts Outdated Show resolved Hide resolved

agnes512 marked this pull request as ready for review September 12, 2019 15:09

agnes512 requested review from bajtos and raymondfeng as code owners September 12, 2019 15:09

bajtos suggested changes Sep 12, 2019

View reviewed changes

packages/repository-tests/src/crud/relations/acceptance/unit.test.relation.acceptance.ts Outdated Show resolved Hide resolved

agnes512 force-pushed the belongsto-resolver branch 3 times, most recently from 9e4f4e5 to 4225c24 Compare September 17, 2019 21:49

raymondfeng reviewed Sep 18, 2019

View reviewed changes

packages/repository/src/relations/relation.helpers.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Sep 18, 2019

View reviewed changes

packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts Show resolved Hide resolved

nabdelgadir reviewed Sep 18, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch from c407cbf to e09dfd9 Compare September 18, 2019 18:43

nabdelgadir reviewed Sep 18, 2019

View reviewed changes

packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts Outdated Show resolved Hide resolved

agnes512 force-pushed the belongsto-resolver branch 4 times, most recently from 61895ff to bb6974a Compare September 18, 2019 21:47

agnes512 requested a review from bajtos September 19, 2019 01:02

bajtos suggested changes Sep 19, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch 2 times, most recently from 7471e50 to f424c2a Compare September 20, 2019 02:29

bajtos approved these changes Sep 20, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch from f424c2a to f71c96f Compare September 20, 2019 14:25

nabdelgadir approved these changes Sep 20, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch 2 times, most recently from 02d5f7f to 200f54a Compare September 20, 2019 18:16

raymondfeng reviewed Sep 20, 2019

View reviewed changes

jannyHou reviewed Sep 20, 2019

View reviewed changes

raymondfeng reviewed Sep 20, 2019

View reviewed changes

jannyHou reviewed Sep 20, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch from ea98efe to 8e860a5 Compare September 20, 2019 19:51

jannyHou approved these changes Sep 20, 2019

View reviewed changes

agnes512 force-pushed the belongsto-resolver branch from 8e860a5 to 2a39041 Compare September 20, 2019 20:20

raymondfeng approved these changes Sep 20, 2019

View reviewed changes

feat(repository): implement inclusion resolver for belongsTo relation

aa13a8b

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

agnes512 force-pushed the belongsto-resolver branch from 2a39041 to aa13a8b Compare September 20, 2019 20:26

agnes512 merged commit fc3d5b6 into master Sep 20, 2019

agnes512 deleted the belongsto-resolver branch September 20, 2019 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository): implement inclusionResolver for belongsTo relation #3721

feat(repository): implement inclusionResolver for belongsTo relation #3721

agnes512 commented Sep 12, 2019 •

edited

Loading

bajtos left a comment

agnes512 commented Sep 13, 2019

raymondfeng Sep 18, 2019

agnes512 Sep 18, 2019

bajtos left a comment

bajtos left a comment

nabdelgadir Sep 20, 2019

raymondfeng Sep 20, 2019

jannyHou Sep 20, 2019

agnes512 Sep 20, 2019

raymondfeng Sep 20, 2019

raymondfeng Sep 20, 2019

agnes512 Sep 20, 2019

jannyHou Sep 20, 2019

agnes512 Sep 20, 2019 •

edited

Loading

jannyHou left a comment

feat(repository): implement inclusionResolver for belongsTo relation #3721

feat(repository): implement inclusionResolver for belongsTo relation #3721

Conversation

agnes512 commented Sep 12, 2019 • edited Loading

Checklist

bajtos left a comment

Choose a reason for hiding this comment

agnes512 commented Sep 13, 2019

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

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

agnes512 Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

agnes512 commented Sep 12, 2019 •

edited

Loading

agnes512 Sep 20, 2019 •

edited

Loading