Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@agnes512
Copy link
Contributor

agnes512 commented Sep 12, 2019

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 馃憟

@agnes512 agnes512 force-pushed the belongsto-resolver branch from ee9b621 to a2817b0 Sep 12, 2019
@agnes512 agnes512 marked this pull request as ready for review Sep 12, 2019
@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Sep 12, 2019
Copy link
Member

bajtos left a comment

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

@agnes512

This comment has been minimized.

Copy link
Contributor Author

agnes512 commented Sep 13, 2019

Helpers for the inclusion resolver #3727

@agnes512 agnes512 force-pushed the belongsto-resolver branch 3 times, most recently from 9e4f4e5 to 4225c24 Sep 13, 2019
through the following code:

```ts
addressRepo.find({include: [{relation: 'customer'}]});

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 18, 2019

Member

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

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 18, 2019

Author Contributor

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

@agnes512 agnes512 force-pushed the belongsto-resolver branch from c407cbf to e09dfd9 Sep 18, 2019
@agnes512 agnes512 force-pushed the belongsto-resolver branch 4 times, most recently from 61895ff to bb6974a Sep 18, 2019
@agnes512 agnes512 requested a review from bajtos Sep 19, 2019
Copy link
Member

bajtos left a comment

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

@agnes512 agnes512 force-pushed the belongsto-resolver branch 2 times, most recently from 7471e50 to f424c2a Sep 20, 2019
@bajtos
bajtos approved these changes Sep 20, 2019
Copy link
Member

bajtos left a comment

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

@agnes512 agnes512 force-pushed the belongsto-resolver branch from f424c2a to f71c96f Sep 20, 2019
}
}
interface ManufacturerRelations {
products?: Product;

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Sep 20, 2019

Contributor

should be product?: ProductWithRelations

@agnes512 agnes512 force-pushed the belongsto-resolver branch 2 times, most recently from 02d5f7f to 200f54a Sep 20, 2019
description: "Odin's Coffee Maker",
customerId: odin.id,
});

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 20, 2019

Member

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

{include: {relation: 'customer', scope: {fields: ['name']}}}
@@ -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);

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 20, 2019

Contributor

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

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

Before we were passing in a single value. Will remove

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 20, 2019

Member

Oh, so the initial version does not support scope?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 20, 2019

Member

BTW, I was referring to the scope inside include.

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

No. See #3443

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

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 20, 2019

Contributor

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?

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

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.

@agnes512 agnes512 force-pushed the belongsto-resolver branch from ea98efe to 8e860a5 Sep 20, 2019
Copy link
Contributor

jannyHou left a comment

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

@agnes512 agnes512 force-pushed the belongsto-resolver branch from 8e860a5 to 2a39041 Sep 20, 2019
Co-authored-by: Miroslav <mbajtos@cz.ibm.com>
@agnes512 agnes512 force-pushed the belongsto-resolver branch from 2a39041 to aa13a8b Sep 20, 2019
@agnes512 agnes512 merged commit fc3d5b6 into master Sep 20, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.02%) to 91.633%
Details
@agnes512 agnes512 deleted the belongsto-resolver branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.