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(repository): adding hasManyThrough (through model) to hasMany and its helpers #5354

Merged
merged 2 commits into from
May 15, 2020

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented May 7, 2020

Extract hasManyThrough relation definition and its helpers from #4438

Adding unit tests for hasManyThrough.help

Changes:

  • Move the through member to HasManyDefinition and get rid of hasManyThroughDefinition.

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 requested a review from hacksparrow as a code owner May 7, 2020 01:44
@agnes512 agnes512 requested review from bajtos and raymondfeng May 7, 2020 01:44
@agnes512 agnes512 marked this pull request as draft May 7, 2020 01:45
@agnes512 agnes512 force-pushed the hmt-def branch 4 times, most recently from 29a7602 to 1500021 Compare May 7, 2020 16:33
@agnes512 agnes512 marked this pull request as ready for review May 7, 2020 16:38
@agnes512 agnes512 force-pushed the hmt-def branch 2 times, most recently from dbf9cbd to 03c2547 Compare May 7, 2020 19:57
@dhmlau dhmlau added the Relations Model relations (has many, etc.) label May 8, 2020
@dhmlau dhmlau added this to the May 2020 milestone May 8, 2020
@agnes512 agnes512 requested a review from derdeka May 8, 2020 15:34
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@agnes512 Appreciate your effort for helping on move the PR forward! Nice start 👍

>(
relationMeta: HasManyThroughResolvedDefinition,
fkValue: ForeignKeyType,
targetInstance?: Target,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, why the 3rd parameter is a target instance instead of id/ids?
IIUC, usually hasManyThrough works in this way:

  • e.g. Find all products belong to a category by category1.products()
  • the through constraint will be {categoryId: 1}
  • all categoryProducts with categoryId = 1 are found, generate a collection of target ids according to records' productId
  • then Products.find(id: {inq: [1,2,3]})

If the product instance is already known, why would people search for it?
If the constraint is to perform PATCH/PUT/DELETE, why it doesn't take in id/ids but one(and only one) instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following 2359 and 4438. And I agree that createThroughConstraint should only take in Through models and resolved relationMatadata and only generate {categoryId: 1} as the constraint. I will fix this first.

Besides, as @bajtos mentioned in comment #2359 (comment), we should improve the process by converting the constraint to a SQL JOIN query (quote from the comment):

SELECT target.*
   FROM CategoryProductLink as through
   LEFT JOIN Product as target ON through.productId = target.id
   # this condition is built from "through" constraint 
   WHERE through.categoryId = 1 
   # this condition is built from "target" constraint
   AND target.country = 'us';

I don't know if this is doable with current connectors, but hopefully it is achievable with function createThroughConstraint in the future as it has all of the pieces to build such queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the discussion in #2359 (comment) 👍

A good summary is here #2359 (comment)

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I quickly skimmed through the proposed changes, they look pretty good 👍

The code coverage has decreased. AFAICT, unit tests are not covering all error paths in has-many-through.helpers.ts, see https://coveralls.io/builds/30686736/source?filename=packages/repository/src/relations/has-many/has-many-through.helpers.ts Could you PTAL?

I'll leave the detailed review and final approvals to CODEOWNERS of repository & relations.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍 Nice start! LGTM

@agnes512 agnes512 merged commit 1168c5f into master May 15, 2020
@agnes512 agnes512 deleted the hmt-def branch May 15, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Relations Model relations (has many, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants