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(selectQuery): Don't add empty HAVING clause #8931

Merged
merged 1 commit into from
Jan 22, 2018

Conversation

jorrit
Copy link
Contributor

@jorrit jorrit commented Jan 22, 2018

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 follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

When querying with empty having object, the query contains an empty HAVING clause. The code processing the where object checks if it contains any statements, the code processing having didn't. I added that.

@codecov
Copy link

codecov bot commented Jan 22, 2018

Codecov Report

Merging #8931 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@jorrit
Copy link
Contributor Author

jorrit commented Jan 22, 2018

The coverage of the patch is 75% because subqueries are not tested, just like before the patch. I can't find a unit test for subqueries either. Must I add one?

Never mind, added one.

@jorrit jorrit force-pushed the emptyhaving branch 2 times, most recently from e2047fd to f97fa72 Compare January 22, 2018 12:59
return {
subQuery: true,
tableAs: 'test',
having: { creationYear: { gt: 2002 } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Op.gt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. I didn't do this because none of the tests seem to use Op.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In v5 all tests will move to operators, this is something we are doing with new tests

@sushantdhiman sushantdhiman merged commit 335efd6 into sequelize:master Jan 22, 2018
@jorrit
Copy link
Contributor Author

jorrit commented Jan 22, 2018

Thanks for merging!

@jorrit jorrit deleted the emptyhaving branch January 22, 2018 14:25
@sushantdhiman
Copy link
Contributor

Thanks for contributing 👍

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.

None yet

2 participants