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: Separate queries are not subQueries #12144

Merged
merged 6 commits into from Apr 22, 2020

Conversation

JacobLey
Copy link
Contributor

@JacobLey JacobLey commented Apr 21, 2020

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)? (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

Resolves #12141

As stated in issue, I believe "separate" queries are being negatively impacted if their "parent" is required, even if that parent is not part of the current query.

The generated SQL expects the parent to be generating a literal subquery (wrapped in parentheses) but due to the nature of "separate" queries, that isn't actually happening.

This change fixes things locally for me.

I don't believe this portion of code is currently covered by unit-tests, and I'm not entirely sure where to start on a higher level integration test...
Added unit test

@JacobLey JacobLey changed the title Parent required separate Separate queries are not subQueries Apr 21, 2020
@JacobLey
Copy link
Contributor Author

@JacobLey JacobLey commented Apr 21, 2020

Bleh keep failing on unrelated (and inconsistent) other tests... closing/re-opening again to retrigger build

@JacobLey JacobLey closed this Apr 21, 2020
@JacobLey JacobLey reopened this Apr 21, 2020
@JacobLey
Copy link
Contributor Author

@JacobLey JacobLey commented Apr 21, 2020

One more try...

@JacobLey JacobLey closed this Apr 21, 2020
@JacobLey JacobLey reopened this Apr 21, 2020
@JacobLey JacobLey changed the title Separate queries are not subQueries fix: Separate queries are not subQueries Apr 21, 2020
@codecov
Copy link

@codecov codecov bot commented Apr 21, 2020

Codecov Report

Merging #12144 into master will increase coverage by 21.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12144       +/-   ##
===========================================
+ Coverage   74.85%   95.87%   +21.01%     
===========================================
  Files          86       92        +6     
  Lines        8373     9019      +646     
===========================================
+ Hits         6268     8647     +2379     
+ Misses       2105      372     -1733     
Impacted Files Coverage Δ
lib/model.js 96.60% <100.00%> (+1.06%) ⬆️
lib/dialects/sqlite/query.js 98.65% <0.00%> (ø)
lib/dialects/postgres/index.js 100.00% <0.00%> (ø)
lib/dialects/sqlite/index.js 100.00% <0.00%> (ø)
lib/dialects/postgres/connection-manager.js 95.83% <0.00%> (ø)
lib/dialects/sqlite/connection-manager.js 100.00% <0.00%> (ø)
lib/dialects/postgres/query.js 97.84% <0.00%> (ø)
lib/utils.js 98.36% <0.00%> (+4.50%) ⬆️
lib/sequelize.js 95.44% <0.00%> (+7.29%) ⬆️
... and 24 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 a1ec8a1...9fa083e. Read the comment docs.

});

expect(options.subQuery).to.equal(false);
expect(options.include[0].subQuery).to.equal(false);
Copy link
Contributor Author

@JacobLey JacobLey Apr 21, 2020

Choose a reason for hiding this comment

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

This assertion will fail if you remove the !include.separate check

@@ -405,6 +405,26 @@ describe(Support.getTestDialectTeaser('Model'), () => {
expect(options.include[0].subQueryFilter).to.equal(false);
});

it('should not tag a separate hasMany association with subQuery true', function() {
Copy link
Contributor

@sushantdhiman sushantdhiman Apr 22, 2020

Choose a reason for hiding this comment

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

In addition to this test, please add an integration test here

Copy link
Contributor Author

@JacobLey JacobLey Apr 22, 2020

Choose a reason for hiding this comment

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

Added integration test

const user = await task.createUser({ id: 2 });
await user.createCompany({ id: 3 });

const results = await Task.findAll({
Copy link
Contributor Author

@JacobLey JacobLey Apr 22, 2020

Choose a reason for hiding this comment

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

@JacobLey
Copy link
Contributor Author

@JacobLey JacobLey commented Apr 22, 2020

Updated SSCCE to use this custom fork sequelize/sequelize-sscce#71

Helps visualize the change in SQL, shows it is now valid

@sushantdhiman sushantdhiman merged commit 2dcd198 into sequelize:master Apr 22, 2020
3 checks passed
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Apr 22, 2020

Thanks for this fix. If you want to have this ported to v5, Please open a PR targeting that branch

JacobLey added a commit to JacobLey/sequelize that referenced this issue Apr 22, 2020
Port of master PR: sequelize#12144

Only significant change is integration test was altered to use promise.then() rather than async/await
@JacobLey
Copy link
Contributor Author

@JacobLey JacobLey commented Apr 22, 2020

If you want to have this ported to v5, Please open a PR targeting that branch

#12152

I had to rebase + squash commits because branches are so different, but hopefully this is enough

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.

2 participants