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

cast json values on write to be consistent with reading from the db. #12643

Merged
merged 1 commit into from Oct 25, 2013

Conversation

@severin
Copy link
Contributor

severin commented Oct 25, 2013

This is analog to what commit 5ac2341 did with hstore fields...

/cc @senny

@senny
senny reviewed Oct 25, 2013
View changes
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,18 @@
* Type cast hstore values on write, so that the value is consistent

This comment has been minimized.

Copy link
@senny

senny Oct 25, 2013

Member

hstore should be json I guess.

@senny
senny reviewed Oct 25, 2013
View changes
activerecord/lib/active_record/connection_adapters/postgresql/oid.rb Outdated
@@ -249,6 +249,11 @@ def type_cast(value)
end

class Json < Type

This comment has been minimized.

Copy link
@senny

senny Oct 25, 2013

Member

✂️ (only the blank line after class)

@senny
senny reviewed Oct 25, 2013
View changes
activerecord/test/cases/adapters/postgresql/json_test.rb Outdated
@@ -49,6 +49,13 @@ def test_change_table_supports_json
JsonDataType.reset_column_information
end

def test_cast_value_on_write
x = JsonDataType.new payload: {:symbol => :foo, "bool" => true, "number" => 5}
assert_equal({"symbol" => "foo", "bool" => true, "number" => 5}, x.payload)

This comment has been minimized.

Copy link
@senny

senny Oct 25, 2013

Member

I think it might be more expressive to only compare the keys:

assert_equal ["symbol", "bool", "number"], x.payload.keys

This comment has been minimized.

Copy link
@severin

severin Oct 25, 2013

Author Contributor

symbol values are also converted to strings, I rewrote the test as follows:

x = JsonDataType.new payload: {"string" => "foo", :symbol => :bar}
assert_equal({"string" => "foo", "symbol" => "bar"}, x.payload)
@senny
Copy link
Member

senny commented Oct 25, 2013

Does #12490 also completely apply for this change?

@severin
Copy link
Contributor Author

severin commented Oct 25, 2013

In senny@557b8b6 you added a test that verifies that store_accessor works with json columns and the test still passes

@senny
Copy link
Member

senny commented Oct 25, 2013

looks great! Thank you for your contribution 💛

senny added a commit that referenced this pull request Oct 25, 2013
cast json values on write to be consistent with reading from the db.
@senny senny merged commit dc8fac1 into rails:master Oct 25, 2013
@severin
Copy link
Contributor Author

severin commented Oct 25, 2013

You're very welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.