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

fix(model): count() code generation for distinct rows #11946

Merged
merged 1 commit into from Feb 21, 2020

Conversation

TimMensch
Copy link
Contributor

@TimMensch TimMensch commented Feb 20, 2020

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: #11894, #2713, #6404, #6418, #7344

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)? See Below
  • 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)? N/A
  • Did you update the typescript typings accordingly (if applicable)? N/A
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

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.

#11893 was this PR against v5. This PR is against master.

Notes:
Tests: Tested against Postgres 11 only. There are failures, but they seem to be failures in master, not in anything that I changed. The only change I made is in the count() function, and none of the failures hits that function. 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.

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
@TimMensch TimMensch requested a review from sushantdhiman Feb 20, 2020
@sushantdhiman sushantdhiman merged commit 8a5ecc1 into sequelize:master Feb 21, 2020
1 of 2 checks passed
@papb
Copy link
Member

@papb papb commented Feb 21, 2020

Nice, thank you very much! 🎉

@TimMensch TimMensch deleted the fix-count-distinct-v6 branch Feb 24, 2020
adzialocha added a commit to adzialocha/hoffnung3000 that referenced this issue Mar 25, 2020
We have a problem with Sequelize and how it handles distinct SQL queries causing slots (JOIN table) to be empty
when fetching many events. This breaks the calendar.

Related issue: sequelize/sequelize#7344
Possible PR/Fix: sequelize/sequelize#11946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants