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 issue with schema usage with FROM clause #4268

Merged
merged 1 commit into from Feb 3, 2021
Merged

Fix issue with schema usage with FROM clause #4268

merged 1 commit into from Feb 3, 2021

Conversation

Saka7
Copy link
Contributor

@Saka7 Saka7 commented Feb 2, 2021

Fixes issue: #1368
Related issues: #2054, #3654

Description

Prevent adding a schema (specified by .withSchema) to a .from caluse containing a QueryBuilder, Raw or function.

@Saka7 Saka7 changed the title Fix issue schema usage with FROM clause Fix issue with schema usage with FROM clause Feb 2, 2021
@Saka7
Copy link
Contributor Author

Saka7 commented Feb 2, 2021

IMHO, In case this PR is merged we should implement #3654 as well for consistency

const isRawQuery = tableName instanceof Raw;
const isFunction = typeof tableName === 'function';

if (tableName && schemaName && !isQueryBuilder && !isRawQuery && !isFunction) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since last three checks are only relevant if first three pass, I would split checks in two blocks and only resolve these conditions if first two pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

@kibertoad
Copy link
Collaborator

@Saka7 I agree. Can you either add the change to this PR or open a separate one?

@kibertoad
Copy link
Collaborator

Also there are conflicts now.

@Saka7
Copy link
Contributor Author

Saka7 commented Feb 3, 2021

@Saka7 I agree. Can you either add the change to this PR or open a separate one?

I've added the fix into this PR

@Saka7 Also there are conflicts now.

Resolved

@kibertoad
Copy link
Collaborator

Thank you!

@kibertoad kibertoad merged commit 09e9fa4 into knex:master Feb 3, 2021
mike183 added a commit to status200/knex that referenced this pull request Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants