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(model): add options.include[].right option #11537

Merged
merged 1 commit into from Oct 18, 2019
Merged

feat(model): add options.include[].right option #11537

merged 1 commit into from Oct 18, 2019

Conversation

ckeboss
Copy link
Contributor

@ckeboss ckeboss commented Oct 11, 2019

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Add ability to specify a right join for associations

Addresses #4187, should we reopen it?

test/integration/model/count.test.js was changed due to the linter. I can revert it if you would like.

@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #11537 into master will increase coverage by 3.78%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11537      +/-   ##
==========================================
+ Coverage   92.47%   96.26%   +3.78%     
==========================================
  Files          91       94       +3     
  Lines        8895     9182     +287     
==========================================
+ Hits         8226     8839     +613     
+ Misses        669      343     -326
Impacted Files Coverage Δ
lib/dialects/sqlite/index.js 100% <ø> (ø)
lib/model.js 96.46% <ø> (ø) ⬆️
lib/dialects/abstract/index.js 100% <ø> (ø) ⬆️
lib/dialects/abstract/query-generator.js 97.18% <100%> (+0.51%) ⬆️
lib/dialects/sqlite/query.js 98.65% <0%> (ø)
lib/dialects/sqlite/connection-manager.js 100% <0%> (ø)
lib/data-types.js 91.34% <0%> (+0.34%) ⬆️
lib/dialects/abstract/query.js 91.98% <0%> (+0.96%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f295a74...a2bd850. Read the comment docs.

@ckeboss ckeboss marked this pull request as ready for review October 11, 2019 03:57
Add ability to specify a right join for associations

Closes #4187
@papb
Copy link
Member

papb commented Oct 11, 2019

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!

I have reopened the issue 👍

I don't have time to look into your PR now though

@papb
Copy link
Member

papb commented Oct 11, 2019

Do you consider this PR ready for merge or is it still a work in progress?

@papb papb added hard For issues and PRs. status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action type: feature For issues and PRs. For new features. Never breaking changes. labels Oct 11, 2019
@ckeboss
Copy link
Contributor Author

ckeboss commented Oct 11, 2019

Yes, finished it up last night. Had to make some updates in regards to sqlite not supporting right joins.

@papb
Copy link
Member

papb commented Oct 11, 2019

This looks good, but I am not sure about the current sqlite behavior. Currently, if I pass right: true to an SQLite call, it will silently ignore it and perform a left join... I'm not sure this is the best way... What do you think about this? Perhaps an error instead or at least a warning?

@ckeboss
Copy link
Contributor Author

ckeboss commented Oct 11, 2019

Is there any precedence for this? How does the library currently handle unsupported features in dialects? The only one I found was UNION ALL which just fallbacks to UNION.

I am open to other ways.

@papb
Copy link
Member

papb commented Oct 13, 2019

I'm not sure really... Let's see what @sushantdhiman thinks about it.

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Oct 13, 2019
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

For now I don't think we need to warn users about right being ignored for given dialect, they should be aware of SQL joins available in that dialects. If there are some complaints regarding this, we can add a warning when parsing include options

@sushantdhiman sushantdhiman merged commit 2949a63 into sequelize:master Oct 18, 2019
This was referenced Oct 18, 2019
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 5.20.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@papb
Copy link
Member

papb commented Oct 18, 2019

Thank you very much for the help @ckeboss!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hard For issues and PRs. released type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants