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

Prevent making bind param if casted value is nil #29282

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented May 30, 2017

If casted value is nil, generated SQL should be IS NULL. But currently
it is generated as = NULL. To prevent this behavior, avoid making bind
param if casted value is nil.

Fixes #28945.

sql = Topic.where(last_read: "").to_sql

assert_no_match %r{["`]last_read["`] = NULL}, sql
assert_match %r{["`]last_read["`] IS NULL}, sql
Copy link
Member

Choose a reason for hiding this comment

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

Let's use assert_equal 1, Task.where(starting: "").count instead. Trying to match quoted identifiers seems tricky, because we'd have to account for SQL Server, etc. (And just in general, if we can prove what we need without being specific about the SQL, so much the better.)

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

Should we also add a CHANGELOG.md entry?

If casted value is nil, generated SQL should be `IS NULL`. But currently
it is generated as `= NULL`. To prevent this behavior, avoid making bind
param if casted value is nil.

Fixes rails#28945.
@kamipo kamipo force-pushed the prevent_making_bind_param_if_casted_value_is_nil branch from 7f055ce to 67a4a9f Compare May 31, 2017 09:28
@@ -48,6 +48,10 @@ def test_where_copies_arel_bind_params
assert_equal [chef], chefs.to_a
end

def test_where_with_casted_value_is_nil
assert_equal 4, Topic.where(last_read: "").count
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the test without assert_match against generated SQL.

@@ -1,3 +1,7 @@
* Prevent making bind param if casted value is nil.

*Ryuta Kamizono*
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a CHANGELOG.md entry.

@eileencodes eileencodes merged commit 6bc1468 into rails:master Jun 2, 2017
@kamipo kamipo deleted the prevent_making_bind_param_if_casted_value_is_nil branch June 2, 2017 19:07
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

4 participants