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

feat: wrap subQuery with parenthesis when it appears as table name #3496

Merged
merged 2 commits into from Oct 28, 2019

Conversation

@edvardchen
Copy link
Contributor

edvardchen commented Oct 25, 2019

Fixes #3485

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 25, 2019

Could you please also add integration test that would actually execute the generated sql?

@edvardchen edvardchen force-pushed the edvardchen:master branch from 0c70ddf to d3409be Oct 28, 2019
@edvardchen edvardchen force-pushed the edvardchen:master branch from 5445986 to 7f7a82b Oct 28, 2019
@@ -1,6 +1,7 @@
/*global expect, d*/
'use strict';

const mysqlErrors = require('mysql/lib/protocol/constants/errors');

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 28, 2019

Collaborator

Why depend on non-public part of external dependency and not just hardcode the constant internally?

This comment has been minimized.

Copy link
@edvardchen

edvardchen Oct 28, 2019

Author Contributor

Emm, OK. I will change it

This comment has been minimized.

Copy link
@kibertoad

kibertoad Oct 28, 2019

Collaborator

My concern is that if there is a major change in the driver and both the constant is no longer there and field is no longer set, comparison is still going to be "undefined === undefined" and will pass. Thanks for changing it!

edvardchen
@edvardchen edvardchen force-pushed the edvardchen:master branch from 7f7a82b to 1e996f7 Oct 28, 2019
@kibertoad kibertoad merged commit 0560959 into knex:master Oct 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 28, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.