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

Fixing SQL in queries #4991

Merged
merged 3 commits into from
Mar 22, 2022
Merged

Fixing SQL in queries #4991

merged 3 commits into from
Mar 22, 2022

Conversation

mike12345567
Copy link
Collaborator

@mike12345567 mike12345567 commented Mar 21, 2022

Description

Fixing issue #4978 - fixing an issue with using the keyword 'in' as part of an SQL query.

The fix for this has to make some changes to the SQL query itself, switching to a where id in ($1, $2, $3) syntax, this utilises some of the work that was put in place around string concatenation to find instances that need swapped. Currently this will look for the keyword in (any casing) before a binding, if found it will switch it to use this syntax. This is also dependent on the inserted value being a string of comma separated values that were to be used as in parameters, however this should always be true as that is the format that would have been used if the bindings were directly interpolated into the SQL query itself.

I've tested this with multiple in queries as well, as well as using in for a single string value (no comma separation). If there are no commas it will just generate a syntax like in ($1) where the string value is the only one used.

I've also added a SQL_MAX_ROWS env variable, so that self hosters can set the max number of rows returned by SQL queries (greater than 5000) if they desire to export large SQL tables.

@mike12345567 mike12345567 self-assigned this Mar 21, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2022

Codecov Report

Merging #4991 (114c9cf) into master (a555659) will decrease coverage by 0.32%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #4991      +/-   ##
==========================================
- Coverage   69.09%   68.77%   -0.33%     
==========================================
  Files         145      146       +1     
  Lines        5019     5054      +35     
  Branches      769      773       +4     
==========================================
+ Hits         3468     3476       +8     
- Misses       1096     1120      +24     
- Partials      455      458       +3     
Impacted Files Coverage Δ
packages/server/src/threads/query.js 63.88% <0.00%> (-6.35%) ⬇️
...ackages/server/src/api/controllers/row/external.js 16.36% <22.22%> (+0.67%) ⬆️
...ackages/server/src/api/controllers/row/internal.js 77.61% <25.00%> (-1.70%) ⬇️
packages/server/src/middleware/publicApi.js 70.00% <70.00%> (ø)
packages/server/src/automations/utils.js 72.72% <0.00%> (-0.96%) ⬇️
packages/server/src/threads/automation.js 82.19% <0.00%> (+0.37%) ⬆️

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 a9154e1...114c9cf. Read the comment docs.

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

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

LGTM - great job getting this sorted so quickly.

packages/server/src/threads/query.js Outdated Show resolved Hide resolved
@mike12345567 mike12345567 merged commit 3811436 into master Mar 22, 2022
@mike12345567 mike12345567 deleted the fix/sql-query-in branch March 22, 2022 13:00
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants