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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: adds support ReferencesMany relation #8271

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

mikeevstropov
Copy link
Contributor

@mikeevstropov mikeevstropov commented Jan 30, 2022

Definition in Model.

@referencesMany(() => Account)
accountIds: string[]; // or number[]

Include in query filter.

{"include": ["accounts"]}

Response will contain resolved relations in accounts field. E.g.

{
  "id": "customer-1",
  "accountIds": [
    "account-1",
    "account-2"
  ],
  "accounts": [
    {"id": "account-1", "balance": 10},
    {"id": "account-2", "balance": 20},
  ]
}

Related
#2488
#1450
#1920

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • 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

@mikeevstropov mikeevstropov changed the title feat(repository): support ReferencesMany relation feat(repository): adds support ReferencesMany relation Feb 1, 2022
@mikeevstropov
Copy link
Contributor Author

Done

@mikeevstropov mikeevstropov changed the title feat(repository): adds support ReferencesMany relation feat: adds support ReferencesMany relation Feb 2, 2022
@coveralls
Copy link

coveralls commented Feb 2, 2022

Pull Request Test Coverage Report for Build 1940408176

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 257 of 300 (85.67%) changed or added relevant lines in 26 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 76.409%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/repository/src/repositories/legacy-juggler-bridge.ts 2 3 66.67%
packages/repository/src/model.ts 0 2 0.0%
packages/repository/src/relations/references-many/references-many.decorator.ts 15 19 78.95%
packages/repository/src/relations/references-many/references-many.repository.ts 3 11 27.27%
packages/repository/src/relations/references-many/references-many.accessor.ts 11 20 55.0%
packages/repository/src/relations/references-many/references-many.helpers.ts 23 32 71.88%
examples/references-many/src/index.ts 4 14 28.57%
Totals Coverage Status
Change from base Build 1937893134: 0.4%
Covered Lines: 17144
Relevant Lines: 18024

💛 - Coveralls

@edgency
Copy link

edgency commented Feb 9, 2022

Looks like no one needs this add-on, sadness

@achrinza achrinza self-requested a review February 10, 2022 06:06
@achrinza achrinza added feature Repository Issues related to @loopback/repository package labels Feb 10, 2022
Copy link
Member

@achrinza achrinza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM 👍 Will see if any of the other maintainers have any objection.

@mikeevstropov
Copy link
Contributor Author

Redundant property itemType has been removed from the relation decorator. Example updated.

@achrinza
Copy link
Member

Thanks, @mikeevstropov. I'll try to set some time to review the latest changes soon.

@dhmlau
Copy link
Member

dhmlau commented Mar 29, 2022

@mikeevstropov, your PR is almost good to go. Could you please squash the commits, esp the ones with Merge branch master...? Thanks.

cc @achrinza

@mikeevstropov
Copy link
Contributor Author

mikeevstropov commented Mar 30, 2022

@dhmlau Hi. I'm not sure if I did it right, just squashed the commits into one (including "merge" commits).

@dhmlau
Copy link
Member

dhmlau commented Mar 30, 2022

@mikeevstropov, hm.. as you pointed out, it has the merged commits, so doesn't look quite right.
I git clone your repo and noticed that you didn't set the upstream. Here is what I normally do to rebase community PRs:

git remote add upstream https://github.com/loopbackio/loopback-next.git
git remote -v // optional - used to verify remotes you have setup
git fetch upstream
git rebase -i upstream/master 
git push origin +references-many-relation

The caveat is... I'm not sure if the commands above could fix where we're at. (I tend to make another local copy of the changes before pushing again, because there're quite a lot of changes in this PR). Please give it a try. Hope it works! Thanks.

@achrinza
Copy link
Member

achrinza commented Mar 30, 2022

@mikeevstropov It seems like most of the changes are localised in the @loopback/repository and @loopback/example-references-many packages. You can select only those specific set of files:

gitCommitHash="$(git log -1 --pretty=%H)"

# Take note of the hash in case you need to reset back to the current state:
echo "$gitCommitHash"

git reset --hard master

git checkout "$gitCommitHash" -- \
    packages/repository/src \
    examples/references-many \
    docs/site/MONOREPO.md \
    docs/site/Relations.md \
    docs/site/Relation-generator.md \
    docs/site/ReferencesMany-relation.md

git status # See the staged files

git commit -s

The above commands should cherry-pick all of the files needed. Please confirm everything is in order before doing a git commit -s and pushing it as this is a destructive action to the Git History. You can also choose to checkout these changes to a new branch and create a new PR.

In case of a SNAFU, we can get back to this current state by resetting back to the original hash:

git reset --hard "$gitCommitHash"

As a safety net, me (and I assume @dhmlau as well) have a local copy of this PR in its current state on our machines.

If you're not comfortable with this, the maintainers can help make the necessary changes from our end.

--

As for the commit message itself, as suggestion may be:

feat(repository): support `ReferencesMany` relation

closes: https://github.com/loopbackio/loopback-next/issues/2488

@achrinza
Copy link
Member

achrinza commented Mar 30, 2022

#8455 is my attempt at "recovering" the original PR using the steps above. If it looks correct, you can try the above steps against this PR, or we can use that new PR if that's preferred.

@mikeevstropov
Copy link
Contributor Author

@dhmlau @achrinza Sorry, got it, the PR should not contain the merged commits. It's fixed now.

@mikeevstropov
Copy link
Contributor Author

@achrinza The commit is renamed as you suggest.

@achrinza achrinza merged commit 371a6dc into loopbackio:master Mar 31, 2022
@achrinza
Copy link
Member

Thanks for your contributions, @mikeevstropov! They have been merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants