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(mssql): Generate valid SQL for withRecursive() [#4514] #4639

Merged

Conversation

jeremy-w
Copy link
Contributor

@jeremy-w jeremy-w commented Aug 23, 2021

mssql forbids use of the RECURSIVE keyword required by some other
dialects.

The approach taken here maximizes subclass reuse and leaking
changes to the query outside with generation. If we skipped
restoring the recursive labels using undoList, the SQL would
generate fine here, but the test suite fails:
The next-tested driver fails its test, because clone() preserves the
top-level statement's recursive = false change, so it's shy one
RECURSIVE in the generated SQL.

This addresses the mssql portion of [#4514], but not the oracledb issues.
If this approach to omitting RECURSIVE looks good, I can apply the same to the
oracledb dialect. (Supporting a column alias list as separate argument, as Oracle
requires for recursive CTEs, would require deeper changes.)

@jeremy-w jeremy-w force-pushed the jeremy-w/mssql/fix-withRecursive-4514 branch from bf06fb6 to c1aa2ed Compare August 25, 2021 01:19
mssql forbids use of the RECURSIVE keyword required by some other
dialects.

The approach taken here maximizes subclass reuse and leaking
changes to the query outside `with` generation. If we skipped
restoring the recursive labels using `undoList`, the SQL would
generate fine here, but the test suite fails:
The next-tested driver fails its test, because clone() preserves the
top-level statement's `recursive = false` change, so it's shy one
`RECURSIVE` in the generated SQL.
@jeremy-w jeremy-w force-pushed the jeremy-w/mssql/fix-withRecursive-4514 branch from c1aa2ed to 0ffc4b6 Compare August 25, 2021 01:28
@jeremy-w
Copy link
Contributor Author

first pass at integration test had errors on a couple platforms i wasn't testing locally:

  • postgres: the format of returning must vary across drivers - pg was returning [{id: 1}] rather than just [1]
  • mysql: its increments() column is apparently unsigned, and so it refused to let a (signed) integer column reference the id column.

i revised it to omit the keys and fkeys (they weren't the point of the test - just superfluous "realism"), and everything passed the next time around. 👍

@kibertoad kibertoad merged commit 40576ce into knex:master Aug 25, 2021
@kibertoad
Copy link
Collaborator

Thanks a lot! Could you also add same for Oracle?

@jeremy-w
Copy link
Contributor Author

Started poking at it - got Oracle working locally so I can iterate on it.

Oracle requires a column list to be valid (with “cteName” (“col1”, “col2”) as …), so it might take larger changes, unless using a raw value in the first argument works as a workaround.

But while trying to check that workaround, I somehow got the query compiler to blow the stack with recursion - haven’t had a chance to untangle that unexpected diversion just yet.

@jeremy-w
Copy link
Contributor Author

Recursion was due to code like this.select().unionAll(this.select()). wrapping the unionAll arg in a callback resolved it.

@jeremy-w
Copy link
Contributor Author

Opened #4652 to apply the parallel change made here. We'll need to work out a querybuilder API change to be able to successfully issue rCTE queries on Oracle, though, in order to accommodate its requirement of a column list.

@jeremy-w jeremy-w deleted the jeremy-w/mssql/fix-withRecursive-4514 branch August 27, 2021 01:29
@kibertoad
Copy link
Collaborator

Released in 0.95.11

OlivierCavadenti pushed a commit to AbeonaPascha/knex that referenced this pull request Nov 4, 2021
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