Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.3k
Scopes with foreign key#9735
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Scopes with foreign key#9735
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedJul 29, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #9735 +/- ##==========================================+ Coverage 96.3% 96.32% +0.01%========================================== Files 63 63 Lines 9413 9408 -5 ==========================================- Hits 9065 9062 -3+ Misses 348 346 -2
Continue to review full report at Codecov.
|
Thanks for this fix. I want to do some tests on my local setup after that I will merge this PR |
Tested this locally, this wont work with @u9r52sld I can merge this in current state or would you like to fix |
@sushantdhiman Thank you for testing. Of course I'll fix |
Someone please tell me why the |
You can ignore it in this case. LGTM, I'll again test a few things locally and merge |
Finally got the chance to test this (basically was not active with Sequelize for past week). Looks like attributes mixing is broken after this, consider this case constModel=sequelize.define('model',{name:Sequelize.STRING,value:Sequelize.INTEGER,password:Sequelize.STRING},{defaultScope:{attributes:{exclude:['password']}},scopes:{aScope:{attributes:{exclude:['value']}}}});console.log(Model._scope);console.log(Model.scope('aScope','defaultScope')._scope); |
I think we should conform scope just before merging them that will solve all concerns I have raised so far. Just conform scopes before merging with other scopes and keep merge/assign logic same as before (as much as possible), otherwise it can become an unintended breaking change. |
Could you please tell me the reason? I think we should not conform scopes like |
May be we can merge attributes in that style, but not for Conforming will convert both Conform standardise includes and other properties of scope so merge is predictable |
@u9r52sld Thanks, now it looks almost ready. Some concerns related to merge strategy changes, we have discussed it in#9087 for a few months. I have asked participants from that discussion to share their ideas here. I don't use scopes in my projects so not sure what merge strategy works well. I hope we can figure out something that works for everyone. P.S. I'll be out for a few days, after that we can complete our discussion and merge this to v5 |
Hello, I'm the one who opened#9087, I believe sushantdhiman wanted me to take a look here because this is also related to scopes. I will try to take a look as soon as possible (in less than 48 hours probably). In the meantime, if you could read my concerns on#9087 as well that would be great :) I'll be happy to discuss further and answer questions. |
@u9r52sld@papb wrt to changes in this PR we should only target how scopes are merged among themselves. That is under scope of this PR. For overall scope overhaul next we need to decide how scopes are merged with finder options, but that should be a different PR. This PR is changing both scope's internal representation (like attributes are no longer converted from |
Oh, the way that scopes are merged among themselves does concern my issue as well. Currently, two scopes with different includes do not merge correctly. I show this in my issue#9087. Once I have more time, I can prepare an specific test that fails. But I believe this should be straightforward from my issue. If anyone has questions let me know. In the weekend I'll have time to write the failing test. |
Hello, I didn't have time to write the test as I was thinking, but please take a look atthis other test that I wrote. It is about a merge of a scope with the findAll options, but the same idea for merging two scopes fails the exact same way. Let me know if you have any other questions or want clarifications. |
@sushantdhiman@papb I'm sorry for the late reply. Thank you for your concern! |
@u9r52sld Thank you very much! Let me know if you need any further clarifications. |
test/unit/model/scope.test.js Outdated
| }, { override: true }); | ||
| expect(Company._scope).to.deep.equal({ | ||
| include: [{ model: Project }] | ||
| }); | ||
| }); | ||
| it('works with exclude and include attributes', () => { | ||
| it('should not work with exclude and include attributes', () => { |
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.
should keep exclude and include attributes
| // Reverse so we consider the latest include first. | ||
| // This is used if several scopes specify the same include - the last scope should take precedence | ||
| for (const scopeInclude of scope.include.reverse()) { |
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.
What is the new merge strategy now (wrthttps://github.com/sequelize/sequelize/blob/master/docs/scopes.md#merging), as far as I can tell it is still the same?
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.
Yeah, it's almost the same. I just updated for issues#9087,#6245
For example, current merge strategy is as follows.
Company.addScope('scopeA', { where: { name: 'A', something: true }, attributes: { include: ['name'] }, include: [ { model: Project, where: { name: 'A', something: true }, }, Project ], limit: 5, order: [ ['name', 'DESC'], ]})Company.addScope('scopeB', { where: { name: 'B' }, attributes: { exclude: ['something'] }, include: [ { model: Project, as: 'Pj' }, { model: Project, where: { name: 'B' } }, ], limit: 10, order: [ ['something', 'ASC'], ]})expect(Company.scope(['scopeA', 'scopeB'])._scope).to.deep.equal({ where: { name: 'B', something: true }, // 'where' is shallowly merged attributes: { // just override exclude: ['something'] }, include: [ // 'include' is shallowly merged with the same model and alias(as) { model: Project, where: { name: 'B', something: true }, }, { model: Project, as: 'Pj' }, ], limit: 10, // just override order: [ // Array is united ['name', 'DESC'], ['something', 'ASC'] ]})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.
Hmm I thought we discussed to haveattributes.include / exclude merge#9735 (comment)
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.
Oh, i'm sorry. I fixed it.e31be36
Company.addScope('scopeA', { attributes: { include: ['name'] }})Company.addScope('scopeB', { attributes: { include: ['something'], exclude: ['password'] }})expect(Company.scope(['scopeA', 'scopeB'])._scope).to.deep.equal({ attributes: { // merged include: ['name', 'something'], exclude: ['password'] }})an unnecessary file
sushantdhiman commentedOct 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
harpcio commentedNov 24, 2018
Are there any chances for this bugfix to be in v4? |
@harpcio I'd guess that probably not, because although I agree that the previous behavior was wrong and that this can be seen as a "bug fix", in fact this is a breaking change, and breaking changes should not be added without a change in the major version number (following semver). |
Extra documentation for this PR was added in#10312 |
Uh oh!
There was an error while loading.Please reload this page.
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Confirming options after foreign key is added.
See issue:#6245