fix(model): count() code generation for distinct rows #11893
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add code to count() to cause it to create valid SQL when distinct=true and col is unspecified or "*". Otherwise the SQL generated is COUNT(DISTINCT(*)), which is invalid. Issues referenced below were already closed, but the issue hadn't actually been fixed, and it _keeps coming up_. Closes: sequelize#2713, sequelize#6404, sequelize#6418, sequelize#7344
|
Looking at the failed build, the failure is unrelated to my PR. |
7 tasks
|
Bump. Any chance we can get this merged? |
sushantdhiman
suggested changes
Feb 20, 2020
sushantdhiman
added
the
status: awaiting response
6 tasks
|
@sushantdhiman #11946 submitted as a PR to master. |
sushantdhiman
approved these changes
Feb 21, 2020
papb
removed
the
status: awaiting response
|
🎉 This PR is included in version 5.21.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Apr 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request check-list
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Add code to
count()to cause it to create valid SQL whendistinct=trueandcolis unspecified or"*". Otherwise the SQL generated isCOUNT(DISTINCT(*)), which is invalid. Issues referenced below were already closed, but the issue hadn't actually been fixed, and it keeps coming up.Made this change against v5, because I need it ASAP, but the identical change should work against v6. I can submit another PR against
masterNotes:
Tested against Postgres 11 only. Three time-based tests failed; it seems like there's a time zone issue? Some other Model/create test failed, also likely due to database version mismatch (permission denied to create extension "uuid-ossp"). Sorry, I don't have time to install the custom Docker Postgres; the related tests passed.
Related tickets: #2713, #6404, #6418, #7344 -- all were "closed", but none were previously actually fixed. Workarounds were listed, but those workarounds don't work at all if you're using Sequelize with Feathers-Sequelize and Sequelize-TypeScript. Besides, Sequelize shouldn't generate bad code when the options aren't perfect.