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 hasMany relation #3595

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@agnes512
Copy link
Contributor

agnes512 commented Aug 23, 2019

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

@agnes512 agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from 6bf98c6 to c74d6b6 Aug 23, 2019
@agnes512 agnes512 force-pushed the hasmany-resolver branch 5 times, most recently from e2ba1c4 to fe46284 Sep 3, 2019
@agnes512 agnes512 force-pushed the hasmany-resolver branch 6 times, most recently from c545033 to 6866dd7 Sep 9, 2019
@agnes512 agnes512 marked this pull request as ready for review Sep 10, 2019
@agnes512 agnes512 requested a review from raymondfeng as a code owner Sep 10, 2019
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
]
```

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

This comment has been minimized.

Copy link
@emonddr

emonddr Sep 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 11, 2019

Author Contributor

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

@agnes512 agnes512 force-pushed the hasmany-resolver branch 4 times, most recently from 951730a to 105385f Sep 11, 2019
@nabdelgadir nabdelgadir force-pushed the hasmany-resolver branch from 105385f to 90ecdb9 Sep 11, 2019
@agnes512 agnes512 force-pushed the hasmany-resolver branch from e1c19f9 to b77f8a9 Sep 12, 2019
Copy link
Member

bajtos left a comment

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

docs/site/HasMany-relation.md Outdated Show resolved Hide resolved
`Invalid "filter.include" entries: {"relation":"orders"}`,
);
// reset
customerRepo.inclusionResolvers.set(

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 12, 2019

Member

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 has been minimized.

Copy link
@bajtos

bajtos Sep 19, 2019

Member

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.

@agnes512

This comment has been minimized.

Copy link
Contributor Author

agnes512 commented Sep 13, 2019

Helpers for the inclusion resolver #3727

keyFrom is implemented in PR #3726

@agnes512 agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from ff03493 to aeda4b0 Sep 13, 2019
@agnes512 agnes512 force-pushed the hasmany-resolver branch from aeda4b0 to 7243075 Sep 17, 2019
@agnes512 agnes512 requested review from emonddr and bajtos Sep 18, 2019
@agnes512 agnes512 force-pushed the hasmany-resolver branch 2 times, most recently from 7786594 to 3ca0cc6 Sep 18, 2019
Copy link
Member

bajtos left a comment

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

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

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 19, 2019

Member

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.

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

it('returns related instances to target models via findById() method', async () => {

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 19, 2019

Member

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

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 19, 2019

Member

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

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

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.

@agnes512 agnes512 force-pushed the hasmany-resolver branch 3 times, most recently from 061af6b to d22962d Sep 20, 2019
@bajtos
bajtos approved these changes Sep 20, 2019
// 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

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 20, 2019

Member

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,
  );
@agnes512 agnes512 force-pushed the hasmany-resolver branch 6 times, most recently from ecca712 to 2b208b7 Sep 20, 2019
);
// add this line to register inclusion resolver
this.registerInclusion('orders', this.orders.inclusionResolver);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 20, 2019

Member

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');

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 20, 2019

Author Contributor

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

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Sep 20, 2019

@agnes512 Please fix CI failures.

Co-authored-by: Nora <nora.abdelgadir@ibm.com>
Co-authored-by: Miroslav <mbajtos@cz.ibm.com>
@agnes512 agnes512 force-pushed the hasmany-resolver branch from 888bdcd to b99c96b Sep 20, 2019
@agnes512 agnes512 merged commit 4cf9a70 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.05%) to 91.682%
Details
@agnes512 agnes512 deleted the hasmany-resolver branch Sep 20, 2019
@samarpanB

This comment has been minimized.

Copy link
Contributor

samarpanB commented Sep 27, 2019

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

@agnes512

This comment has been minimized.

Copy link
Contributor Author

agnes512 commented Sep 27, 2019

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

@samarpanB

This comment has been minimized.

Copy link
Contributor

samarpanB commented Sep 27, 2019

Is it possible to release today? Actually I need to use it in one of my ongoing project. I don鈥檛 want to use the poor mans inclusion resolver 馃槉

@agnes512

This comment has been minimized.

Copy link
Contributor Author

agnes512 commented Sep 27, 2019

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

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.