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

Strip whitespace in disallow_raw_sql! #41326

Merged
merged 1 commit into from Feb 5, 2021

Conversation

gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Feb 4, 2021

Summary

Closes #41307

Support proper SQL parsing for multiple column strings in pluck.

Before:

User.pluck("first_name, last_name") #=> SELECT first_name, last_name FROM "users"
User.pluck("first_name, last_name\n") #=> ActiveRecord::UnknownAttributeReference

After:

User.pluck("first_name, last_name") #=> SELECT first_name, last_name FROM "users"
User.pluck("first_name, last_name\n") #=> SELECT first_name, last_name\n FROM "users"

Simply strips whitespace from the SQL sanitization checks of pluck.

@kaspth
Copy link
Contributor

kaspth commented Feb 4, 2021

I'd vote for deprecating, since pluck("first_name, last_name") won't fit with the Enumerable version of pluck.

@gmcgibbon
Copy link
Member Author

Hm, this also seems to conflict with invoking SQL functions in pluck. I'll go ahead and try to deprecate instead.

@rafaelfranca
Copy link
Member

pluck(Arel.sql("some_sql_transformation(first_name)")) should still be supported. So I don't think not matching Enumerator version of pluck is enough reason to deprecate that.

@gmcgibbon gmcgibbon force-pushed the multiple_column_strings branch 3 times, most recently from 1d6afc9 to 8cee3f4 Compare February 4, 2021 21:51
Removing trailing whitespace when matching columns in
`ActiveRecord::Sanitization.disallow_raw_sql!`.

Co-authored-by: Adrian Hirt <aedu.hirt@gmail.com>
@gmcgibbon
Copy link
Member Author

gmcgibbon commented Feb 4, 2021

Given I can't deprecate string / comma delimited strings, I've decided to simply use strip in the SQL sanitizing method (like the issue originally suggested). Parsing SQL into an AST isn't worth it with all the edge cases and potential speed regressions.

@gmcgibbon gmcgibbon changed the title Sanitize SQL columns Strip whitespace in disallow_raw_sql! Feb 5, 2021
@gmcgibbon gmcgibbon merged commit bf1fbfd into rails:main Feb 5, 2021
@gmcgibbon gmcgibbon deleted the multiple_column_strings branch February 5, 2021 01:03
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.

Whitespace at start/end of query string triggers ActiveRecord::UnknownAttributeReference
3 participants