Merge AND conditions into one when chaining scopes

[olzv]

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

Closes #11588

Making it possible to chain scopes without overriding.

For example without this change only the last scope is applied while chaining if they both map condition to Op.and operator.

[codecov]

Codecov Report

Merging #11805 into master will increase coverage by 6.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #11805      +/-   ##
==========================================
+ Coverage   90.11%   96.25%   +6.13%     
==========================================
  Files          93       95       +2     
  Lines        9106     9233     +127     
==========================================
+ Hits         8206     8887     +681     
+ Misses        900      346     -554     

Impacted Files	Coverage Δ
lib/dialects/mssql/index.js	100.00% <0.00%> (ø)
lib/dialects/mssql/connection-manager.js	87.01% <0.00%> (ø)
lib/model.js	96.61% <0.00%> (+0.12%)	⬆️
lib/query-interface.js	92.19% <0.00%> (+0.48%)	⬆️
lib/sequelize.js	95.93% <0.00%> (+0.62%)	⬆️
lib/dialects/abstract/query-generator.js	97.12% <0.00%> (+1.26%)	⬆️
...dialects/abstract/query-generator/helpers/quote.js	100.00% <0.00%> (+6.66%)	⬆️
lib/dialects/mssql/data-types.js	100.00% <0.00%> (+69.87%)	⬆️
lib/dialects/mssql/resource-lock.js	100.00% <0.00%> (+77.77%)	⬆️
lib/dialects/mssql/query.js	95.53% <0.00%> (+89.94%)	⬆️
... and 2 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 b2a2134...e86d663. Read the comment docs.

[papb]

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 see WIP in the title; please let me know when you think this PR is ready for review. Thanks.

[olzv]

Hello, thanks.
Yes, it's WIP because I wanted to add some tests to cover this. Will let you know as soon as I am done.

[olzv]

Hi @papb basically the PR is ready for review in my opinion but I don't understand why code coverage decreased so much.
Could you help me please to figure out why it happened?

[papb]

Hello, don't worry about code coverage, it gives some weird results some times...

[papb]

As is, this PR introduces a breaking change, since what would previously be overwritten now won't. I think this feature should be added in a way that is not a breaking change. I suggest you add a new boolean option to the model definition, called perhaps enableExtendedScopeWhereMerges, which defaults to false and which you must check for being true before applying your change.

Also, for a richer feature, in that case you should also enable merging for [Op.or] and [Op.not]. What do you think?

[papb]

@olzv, this PR will fix #11588, I have edited your first post so that GitHub knows it.

😬

[olzv]

Hi @papb

I agree about this being a breaking change. I will add the boolean flag as you suggested. It makes sense to me.

Also, for a richer feature, in that case you should also enable merging for [Op.or] and [Op.not]. What do you think?

In my implementation it merges only Op.and but it is probably better to also merge Op.or and Op.not. I will improve that.

[sushantdhiman]

Actually this is related to #10873, perhaps we can go with merge strategy approach. I think where queries should be merged for all operators, deciding how to merge them properly may be tricky though

[loban]

I have a problem with this PR. It selectively only merges [Op.and] clauses but over-writes everything else. My ticket which was linked is not about that.

I am proposing to combine ALL where clauses by ANDing them together (perhaps with a flag so as to be backwards-compatible). This allows all the scenarios I outlined in my ticket. However, this PR does highlight the area of code I may want to modify to fulfill my intention.

[loban]

I am proposing to combine ALL where clauses by ANDing them together (perhaps with a flag so as to be backwards-compatible).

The key to understanding why I am proposing this is to realize that

where: {
    field1: value1,
    field2: value2
}

is really shorthand for

where: {
    [Op.and]: [
        { field1: value1 },
        { field2: value2 }
    ] 
}

[olzv]

Hi everyone,

the suggestion to merge all WHERE clauses sounds reasonable to me. I'll try to implement it.

[olzv]

Hey guys,

just a short update on this PR.
I am still working on it and have had already some progress. Now I want to make the code a bit prettier and maybe cover some edge cases.
Hope to send for the review soon.

[olzv]

Hi guys.

For now I think I've handled most of the cases but still few left. Also need to do some refactoring.
But I am close.
Will let you know as soon as it's ready.

[loban]

Thanks for working on this olzv. I'm excited to see the final results.

[olzv]

Hi guys,

I think it is ready for the review. Please take a look.
To see different possible merging cases you can refer to tests which I added.

[sdepold]

Hey there :)

First of all: Thanks a bunch for your contribution to Sequelize! Much appreciated!
Second: Unfortunately we haven't had the chance to look into this ever since you created it. Sorry for that!

A couple of months ago, we have switched from master to main branch as our primary development branch and hence this PR is now outdated :(

If you still think this change is making sense, please consider recreating the PR against main. Thanks in advance and sorry for the additional work.

✌️

[WikiRik]

Closing this since it is still against master. Can be reopened if that changes.