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 inclusion resolver for hasOne relation #3764

Merged
merged 1 commit into from Sep 24, 2019

Conversation

@agnes512
Copy link
Contributor

agnes512 commented Sep 19, 2019

resolves #3449

This PR implements the inclusionResolver for hasOne relation:

  • Add a new function createHasOneInclusionResolver to hasOne factory.
  • Add tests for hasOneInclusionResolver in repository-tests.
  • Add docs to explain the idea of hasOneInclusionResolver, and the basic setup/usages.

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 hasone-resolver branch 4 times, most recently from 1dcc829 to 72415c4 Sep 20, 2019
@agnes512 agnes512 marked this pull request as ready for review Sep 21, 2019
@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Sep 21, 2019
docs/site/hasOne-relation.md Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the hasone-resolver branch 2 times, most recently from 48b9193 to 357bb70 Sep 23, 2019
docs/site/hasOne-relation.md Outdated Show resolved Hide resolved
docs/site/hasOne-relation.md Outdated Show resolved Hide resolved

## Querying related models

We introduce the concept of `inclusion resolver` in `Has Many Relation` and

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Sep 24, 2019

Contributor

Should we have the same intro as the other relations for consistency:

LoopBack 4 has the concept of an `inclusion resolver` in relations, which helps
to query data through an `include` filter. An inclusion resolver is a function
that can fetch target models for the given list of source model instances.
LoopBack 4 creates a different inclusion resolver for each relation type.

?

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 24, 2019

Author Contributor

I don't want to repeat (copy) it three times in three different relations 馃槀 I was thinking that they might check Has Many and BelongsTo already.

This comment has been minimized.

Copy link
@dhmlau

dhmlau Sep 24, 2019

Contributor

If it's the same content, should we move the content to the top level? https://loopback.io/doc/en/lb4/Relations.html.
IMHO, it might not be the right assumption that users would read the docs for all the relations page. :) Besides, when we have more relations, it might not be a good idea to copy and paste the same paragraph everywhere too. Just my 2 cents.

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 24, 2019

Author Contributor

I thought about that before. But I found that the content in Relation.html are high-level stuff. That's why I put them inside of each relation.

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Sep 24, 2019

Contributor

IMHO, it might not be the right assumption that users would read the docs for all the relations page. :)

I agree that it might not be the right assumption that users will read the docs for all of them (they might just be interested in hasOne).

Besides, when we have more relations, it might not be a good idea to copy and paste the same paragraph everywhere too. Just my 2 cents.

I think for the meantime since we only have three, it should be fine?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 24, 2019

Member

+1 to introduce the InclusionResolver concept at relations level. Each of the specific relations can have its own page. We can have links to refer to the InclusionResolver.

@agnes512 agnes512 force-pushed the hasone-resolver branch 2 times, most recently from 3f95dd6 to 9b5eaf5 Sep 24, 2019
Copy link
Contributor

nabdelgadir left a comment

one minor comment but LGTM

@@ -31,7 +31,10 @@ introduction of [repositories](Repositories.md), we aim to simplify the approach
to relations by creating constrained repositories. This means that certain
constraints need to be honoured by the target model repository based on the
relation definition, and thus we produce a constrained version of it as a
navigational property on the source repository.
navigational property on the source repository. Besides, we also introduce the

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Sep 24, 2019

Contributor

suggestion: Additionally, we introduce...?

Co-authored-by: Miroslav <mbajtos@cz.ibm.com>
@agnes512 agnes512 force-pushed the hasone-resolver branch from 9b5eaf5 to 8682168 Sep 24, 2019
@agnes512 agnes512 merged commit 8dfdf58 into master Sep 24, 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.01%) to 91.73%
Details
@agnes512 agnes512 deleted the hasone-resolver branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.