-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(repository): add more helpers for HasManyThrough #5605
Conversation
0b3a74b
to
9fb04db
Compare
Added two helpers. They are all for deleting through models as we decided to have the delete method this way https://github.com/strongloop/loopback-next/pull/4438/files#r402105078 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as far as I am concerned.
I am not familiar enough with the code in packages/repository/src/repositories/constraint-utils.ts
to be able to review the changes properly. Please get the approval from FA owners before landing.
packages/repository/src/__tests__/unit/repositories/constraint-utils.unit.ts
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.helpers.ts
Outdated
Show resolved
Hide resolved
* keyTo: 'productId', | ||
* }, | ||
* }; | ||
* createThroughConstraint(resolvedMetadata, {id: 3, name: 'a product'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional query field reminds me sth(sorry don't remember where is the original chat :p) :
We want to distinguish the query on through model(CategoryProductLink) and target model(Product), how do we achieve that?
For example, {name: 'a product'}
seems for target model, and createThroughConstraint
applies for through model, and that's why name
is ignored here. But:
- where is the place that processes the target model's filter?
- how to specify the through model's filter?
(I started from the helper function, might missing the code/docs if they are created other places.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jannyHou I think these helpers are just for handling the relation metadata. And filters are handled in the hasManyThrough repository. Here is an example in the original design: find method. They won't be included in this PR. But I'd love to discuss it:
So far from those two community PRs(#2359 , #4438 ), I only see the target model is capable of applying filters. Not sure if we need it for through models ( I don't see the reason to allow filtering through models tho). Do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice progress!
Add two more helpers for HasManyThrough relation
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈