-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: merge scope where conditions using the and operator #14110
feat: merge scope where conditions using the and operator #14110
Conversation
Using the flag approach is really stunning here and nice to have, anyway, can you also provide the usage with the same description as the model definition how to enable the flag option in Sequelize manual? Might be part of Applying WHERE clauses |
Wouldn't it be easier and less error prone to combine where conditions with So: // Scope 1
whereAttributeIs1: {
where: { field: 1 },
},
// Scope 2
whereAttributeIs2: {
where: { field: 2 },
}
myModel.scope('whereAttributeIs1', 'whereAttributeIs2')._scope ===
{
where: {
[Op.and]: [
{ field: 1 },
{ field: 2 },
]
}
} |
Thanks for the reply and the suggestion. Indeed, it would be nice to update the manual documentation with this new feature. I assume it would be best to add a new sub section in the merging section in https://sequelize.org/v6/manual/scopes.html (https://github.com/EtienneTurc/sequelize/blob/add-option-to-merge-where-scope-with-and-operator/docs/manual/other-topics/scopes.md).
Thanks for the reply and the suggestion. Indeed, it looks easier and thus the previous examples will look like these: // Attributes merging
// Scope 1
whereAttributeIs1: {
where: {
field: 1,
},
},
// Scope 2
whereAttributeIs2: {
where: {
field: 2,
},
}
// New expected behavior
myModel.scope('whereAttributeIs1', 'whereAttributeIs2')._scope ===
{
where: {
[Op.and]: [
{field: 1},
{field: 2}
]
}
} // Op.and merging
// Scope 1
whereOpAnd1: {
where: {
[Op.and]: [{ field: 1 }],
},
},
// Scope 2
whereOpAnd2: {
where: {
[Op.and]: [{ field: 2 }],
},
},
// New expected behavior
myModel.scope('whereOpAnd1', 'whereOpAnd2')._scope ===
{
where: {
[Op.and]: [
{ field: 1 },
{ field: 2 }
],
},
} // Op.or merging
// Scope 1
whereOpOr1: {
where: {
[Op.or]: [{ field: 1 }],
},
},
// Scope 2
whereOpOr2: {
where: {
[Op.or]: [{ field: 2 }],
},
},
// New expected behavior
myModel.scope('whereOpAnd1', 'whereOpAnd2')._scope ===
{
where: {
[Op.and]: [
{ [Op.or]: [{ field: 1 }] },
{ [Op.or]: [{ field: 2 }] }
],
},
} But let's also note that this implementation will also impact those cases (even if this should not be an issue): // Scope 1
whereAttributeIs1: {
where: {
field1: 1,
},
},
// Scope 2
whereAttributeIs2: {
where: {
field2: 2,
},
}
// Current behavior
myModel.scope('whereAttributeIs1', 'whereAttributeIs2')._scope ===
{
where: {
field1: 1,
field2: 2
},
}
// New expected behavior
myModel.scope('whereAttributeIs1', 'whereAttributeIs2')._scope ===
{
where: {
[Op.and]: [
{field1: 1},
{field2: 2}
]
}
} (Currently making the modifications) |
I have a few more comments but I'll let you change this major change first and do a thorough review once you're ready :) |
I've made the requested changes. Note that it slightly changes the behavior of merging // Op.and merging
// Scope 1
whereOpAnd1: {
where: {
[Op.and]: [{ field: 1 }],
},
},
// Scope 2
whereOpAnd2: {
where: {
[Op.and]: [{ field: 2 }],
},
},
// New behavior
myModel.scope('whereOpAnd1', 'whereOpAnd2')._scope ===
{
where: {
[Op.and]: [
{ [Op.and]: [{ field: 1 }] },
{ [Op.and]: [{ field: 2 }] }
],
},
} It drastically simplifies the code and makes debugging easier because where clauses from different scopes are not mixed together. I am waiting for you to approve these changes before adding a new sub-section in the manual documentation :) |
0e31df8
to
773a10f
Compare
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.
From what I can see, mergeWhereScopesWithAndOperator
is in the Model
's options. I think that's fine because IMO the current merge behavior is just plain wrong and should be replaced with the new one.
This is targeting main
(v7 alpha). I think it would be better to only introduce this flag in v6 to avoid a breaking change, and use the new behavior as the default behavior in v7.
That would mean:
- Open a new PR that targets the v6 branch and copy-paste the current code there.
- Change this PR to make merging with AND the only behavior in v7, without a flag
Can you add a few tests that verify that Model.findAll
merges the scopes using the right strategy? (you will need to use the beforeFindAfterOptions
hook to acesss the transformed options)
Thanks for the review. I also believe that this behavior should be the default one, so I am glad that you are willing to use it for the v7. I will make the two PRs in the following days :) |
d1332c5
to
9c486df
Compare
I've made the requested modifications. So now this pr (for the v7) changes the merging behavior from the For the v6, I've opened this pr: NB: I took the liberty to name the current behavior |
9c486df
to
4649979
Compare
Thanks! I'll try to not take too long before reviewing both PRs. I'm not the most knowledgeable user of scopes (never used them, actually) so I'd like another member of @sequelize/maintainers to review this too, but based on my understanding of scopes, these changes look good. In-depth review as soon as I can |
c980457
to
63fc59a
Compare
I've made the requested changes (on both prs). I was reluctant at first to unpack |
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.
Looking good :)
I agree with your concern, if we didn't use _.merge we could have enough information to only add [Op.and] when necessary without unpacking existing [Op.and] but I think this is good enough
Only 3 nits and it's good on my end
The concept was also approved by other team members
63fc59a
to
0b681a4
Compare
Great ! I have done the few changes. |
0b681a4
to
0695256
Compare
And here we go! 🎊 Thanks for the PR :) |
Thank you ! |
🎉 This PR is included in version 7.0.0-alpha.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
…14110) BREAKING CHANGE: the `where` property of a scope is now merged using `Op.and`
Pull Request Checklist
yarn test
oryarn test-DIALECT
pass with this change (including linting)?Description Of Change
Issues related:
Pull request that tried to solve these issues:
This pr adds the following elements:
AND
operator instead of overwriting it with the last value. It supports merging attributes,[Op.and]
and[Op.or]
.mergeWhereScopesWithAndOperator
to enable this behavior for a model or all the models.Examples:
Why should this be in Sequelize ?
The current behavior for merging different attributes already acts as an
AND
operator. Thus it is natural to expect thesame behavior for different conditions on the same attribute.