Permalink
Browse files

pg, keep `hstore` and `json` attributes as `Hash` in @attributes.

The solution presented in this patch is not efficient. We should replace it
in the near future. The following needs to be worked out:
  * Is `@attributes` storing the Ruby or SQL representation?
  * `cacheable_column?` is broken but `hstore` and `json` rely on that behavior

Refs #15369.

/cc @sgrif @rafaelfranca
  • Loading branch information...
1 parent 7f73b91 commit bdbf00dc78869f91a8af4d56a19213faf2b8d9e5 @senny senny committed May 28, 2014
@@ -1,3 +1,10 @@
+* Keep PostgreSQL `hstore` and `json` attributes as `Hash` in `@attributes`.
+ Fixes duplication in combination with `store_accessor`.
+
+ Fixes #15369.
+
+ *Yves Senn*
+
* `rake railties:install:migrations` respects the order of railties.
*Arun Agrawal*
@@ -8,7 +8,13 @@ def type
end
def type_cast_for_write(value)
- ConnectionAdapters::PostgreSQLColumn.hstore_to_string value
+ # roundtrip to ensure uniform uniform types
+ # TODO: This is not an efficient solution.
+ cast_value(type_cast_for_database(value))
+ end
+
+ def type_cast_for_database(value)
+ ConnectionAdapters::PostgreSQLColumn.hstore_to_string(value)
end
def cast_value(value)
@@ -8,7 +8,13 @@ def type
end
def type_cast_for_write(value)
- ConnectionAdapters::PostgreSQLColumn.json_to_string value
+ # roundtrip to ensure uniform uniform types
+ # TODO: This is not an efficient solution.
+ cast_value(type_cast_for_database(value))
+ end
+
+ def type_cast_for_database(value)
+ ConnectionAdapters::PostgreSQLColumn.json_to_string(value)
end
def cast_value(value)
@@ -142,6 +142,16 @@ def test_with_store_accessors
assert_equal "GMT", x.timezone
end
+ def test_duplication_with_store_accessors
+ x = Hstore.new(language: "fr", timezone: "GMT")
+ assert_equal "fr", x.language
+ assert_equal "GMT", x.timezone
+
+ y = x.dup
+ assert_equal "fr", y.language
+ assert_equal "GMT", y.timezone
+ end
+
def test_gen1
assert_equal(%q(" "=>""), @column.class.hstore_to_string({' '=>''}))
end
@@ -139,6 +139,14 @@ def test_with_store_accessors
assert_equal "640×1136", x.resolution
end
+ def test_duplication_with_store_accessors
+ x = JsonDataType.new(resolution: "320×480")
+ assert_equal "320×480", x.resolution
+
+ y = x.dup
+ assert_equal "320×480", y.resolution
+ end
+
def test_update_all
json = JsonDataType.create! payload: { "one" => "two" }

8 comments on commit bdbf00d

Member

sgrif replied May 28, 2014

Is @attributes storing the Ruby or SQL representation?

From what I could tell last time I took a swing at the behavior around that code, it's neither. It stores the string representation intended for the form builder. However, that doesn't make sense when the type in question in mutable, since it mutates the values in @attributes_cache, which is nonsensical. I think we should decide what, if anything, _before_type_cast means for types that will not be represented in a form builder such as these.

Member

sgrif replied May 28, 2014

Is the call to type_cast_for_database necessary? Would doing alias type_cast_for_write type_cast work?

Member

senny replied May 28, 2014

@sgrif also, we don't have to store the form inputs in @attributes to provide _before_type_cast. I think we need to decide what representation is most effective to work with. That should be stored in @attributes. Then we can provide the necessary feature set around that. Currently it's type casting madness everywhere. Specifically with more complex PG types and serialized attributes.

Member

senny replied May 28, 2014

@sgrif type_cast_for_database ensures that we have consistent keys across reloads:

model.settings = { key: "value" }
model.settings # => { "key" => "value" }
model.reload
model.settings # => { "key" => "value" }

If we don't cast to the DB representation we get:

model.settings = { key: "value" }
model.settings # => { key: "value" }
model.reload
model.settings # => { "key" => "value" }
Member

sgrif replied May 28, 2014

also, we don't have to store the form inputs in @attributes to provide _before_type_cast.

Agreed. If nothing else, that variable as it is used now should be renamed.

I think we need to decide what representation is most effective to work with. That should be stored in @attributes. Then we can provide the necessary feature set around that.

Sure. We should also pin down what "before type cast" means in cases like this. Does that concept even make sense when the type cast version is mutable?

Currently it's type casting madness everywhere. Specifically with more complex PG types and serialized attributes.

madness-this-is-sparta

Member

sgrif replied May 28, 2014

type_cast_for_database ensures that we have consistent keys across reloads:

Would adding this to type_cast?

when Hash then value.stringify_keys
Member

senny replied May 28, 2014

@sgrif it depends wether hstore_to_string performs additional casting or not. As far as I can tell it converts values as well.

Member

sgrif replied May 28, 2014

Yeah, the added complexity is probably not worth the minimal performance gains.

Please sign in to comment.