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

Serialize JSON attribute value nil as SQL NULL, not JSON 'null' #25670

Merged
merged 1 commit into from Sep 23, 2016

Conversation

Projects
None yet
9 participants
@tdtran
Contributor

tdtran commented Jul 3, 2016

Assume payload is a table column of JSON type. A Ruby nil value of payload can be stored in the database in one of two ways

  • SQL NULL
  • JSON value 'null'

When loading data from the database both NULL and 'null' are deserialized correctly into Ruby nil. This part is fine.

When data are persisted into the DB nil is translated to 'null', not SQL NULL. This leads to several problems:

Although x.update_attributes(payload: nil) translates to SQL UPDATE products SET payload = 'null', a query Product.where(payload: nil) translates to SELECT * from products WHERE payload IS NULL. Such a SQL query returns nothing because 'null' IS NOT NULL. This is demonstrated by the failed test I added in this PR.

One way to fix this bug is to translate where(payload: nil) to WHERE payload IS NULL OR payload = 'null'. We'd have to check if payload type is JSON first and such a SQL statement is slower than the simpler payload IS NOT NULL.

Support for JSON types in Postgresql in Rails 4.x translates nil to SQL NULL, not JSON 'null'. This change of behavior was introduced when AR added JSON support for MySQL (#21110). In our code base we have a few complicated SQL statements which rely on the old behavior. They are broken when we upgrade to Rails 5.0. Queries for nil, not nil are pretty common, I suspect we are not the only ones affected.

I think the preferred fix is to map nil to SQL NULL, exactly as how it used to work in Rails 4.x.

@rails-bot

This comment has been minimized.

rails-bot commented Jul 3, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @senny (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@agrobbin

This comment has been minimized.

Contributor

agrobbin commented Jul 28, 2016

We just ran into this bug in a newly upgraded Rails 5 application. Previously nil was coerced to SQL NULL, but now it's coerced to a JSON string of "null", causing issues when attempting to search for records that have no data in that column. Using this patch successfully changed all "null" strings to SQL NULL.

Would be great to get this fix included in 5.0.1!

@senny

This comment has been minimized.

Member

senny commented Aug 5, 2016

This looks good. @sgrif may have some insights to share.

@tdtran can you squash your commits and add an entry to the changelog?

@tdtran tdtran force-pushed the tdtran:fix-nil-json branch Aug 5, 2016

@tdtran

This comment has been minimized.

Contributor

tdtran commented Aug 5, 2016

@senny changelog entry added, commits squashed, branch rebased against latest rails[master]

@tenderlove

This comment has been minimized.

Member

tenderlove commented Sep 23, 2016

@tdtran can you rebase and I'll merge?

Serialize JSON attribute value nil as SQL NULL, not JSON 'null'
Test: JSON attribute value nil can be used in where(attr: nil)

Add changelog entry

@tdtran tdtran force-pushed the tdtran:fix-nil-json branch to 21675fd Sep 23, 2016

@tenderlove tenderlove merged commit da8cd51 into rails:master Sep 23, 2016

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@connorshea

This comment has been minimized.

Contributor

connorshea commented Sep 23, 2016

Should this be backported to 5-0-stable and/or added to the 5.0.1 milestone?

@tenderlove

This comment has been minimized.

Member

tenderlove commented Sep 23, 2016

@connorshea yes, I'll backport to 5-0-stable

tenderlove added a commit that referenced this pull request Sep 23, 2016

Merge pull request #25670 from tdtran/fix-nil-json
Serialize JSON attribute value nil as SQL NULL, not JSON 'null'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment