Fix for key_format! to handle nested hashes #486
Conversation
Converts the keys of all hashes within arrays and ones being merged in to follow the specified key format. Fixes rails#473
blackjack26
force-pushed
the
473-key-format-merge
branch
from
b3122e0 to
3c0c882
Compare
blackjack26
changed the title
|
+1 looking for this feature as well |
|
Another +1 here. |
|
I agree that this is a good idea in general. Still, it is a breaking change that is now breaking my app. |
|
tf, your app was expecting that the key_format wasn't being honored below the first level? |
|
Yes - or at least it worked with the previous behavior. For example, here we include some translations in a JSON seed that are then passed along to |
|
Gotcha. Goes to the idea that every bug that isn't fixed becomes a feature that's depended upon 😄. I'd take a PR that adds a compatibility mode that can be configured to bring the bug back. |
Since rails#486, key format is applied when a hash is merged into the currently built object: json.key_format! :upcase json.merge!({some: "value"}) # => {"SOME": "value"} As a side effect of the change, key format is also applied to the result of nested blocks [1]: json.key_format! camelize: :lower json.level_one do json.key_format! :upcase json.value 'two' end # => jbuilder 2.10.0: {"levelOne": {"VALUE": "two"}} # => jbuilder 2.11.0: {"levelOne": {"vALUE": "two"}} The string "vALUE" results from calling `"value".upcase.camelize(:lower)`. This makes it impossible to override key format in child elements. [1] https://github.com/blackjack26/jbuilder/blob/3c0c882f23e892dbe51be625bb71adc42febae09/lib/jbuilder.rb#L39
|
I'd be ok with changing my app in this case and making things more explicit. I would have expected to be able to change the key format in child elements. To stick with my example: # before
json.key_format! camelize: :lower
json.translations get_translations
# after
json.key_format! camelize: :lower
json.translations do
json.key_format! :underscore
json.merge! get_translations
endThis no longer works, though, since with the change in this PR the I pushed a test that passes for 2.10 and fails for 2.11: tf@136be93 |
|
I would expect that to work as well. If @blackjack26 wants to have a go, have at it. Otherwise you might also want to take a look. |
Since rails#486, key format is applied deeply to hashes and arrays that are passed to `set!`, `merge!` and `array!`: json.key_format! :upcase json.set!(:foo, {some: "value"}) # => {"FOO": {"SOME": "value"}} json.key_format! :upcase json.merge!({some: "value"}) # => {"SOME": "value"} json.key_format! :upcase json.array!([{some: "value"}]) # => [{"SOME": "value"}] This also works for arrays and hashes extracted from objects: comment = Struct.new(:author).new({ first_name: 'John', last_name: 'Doe' }) json.key_format! camlize: :lower json.set!(:comment, comment, :author) # => {"comment": {"author": {"firstName": "John", "lastName": "Doe"}}} As a side effect of the change, key format is also applied to the result of nested blocks, making it impossible to change key format in the scope of a block: json.key_format! camelize: :lower json.level_one do json.key_format! :upcase json.value 'two' end # => jbuilder 2.10.0: {"levelOne": {"VALUE": "two"}} # => jbuilder 2.11.0: {"levelOne": {"vALUE": "two"}} The string "vALUE" results from calling `"value".upcase.camelize(:lower)`. The same happens when trying to change the key format inside of an `array!` block. This happens since key transformation was added in the `_merge_values` method, which is used both by `merge!` but also when `set!` is called with a block. To restore the previous behavior, we pull the `_transform_keys` call up into `merge!` itself. To make sure extracted hashes and arrays keep being transformed (see comment/author example above), we apply `_transform_keys` in `_extract_hash_values` and `_extract_method_values`. This also aligns the behavior of `extract!` which was left out in rails#486: comment = {author: { first_name: 'John', last_name: 'Doe' }} result = jbuild do |json| json.key_format! camelize: :lower json.extract! comment, :author end # => jbuilder 2.10 and 2.11: {"author": { :first_name => "John", :last_name => "Doe" }} # => now: {"author": { "firstName": "John", "lastName": "Doe" }} Finally, to fix `array!`, we make it call `_merge_values` directly instead of relying on `merge!`. `array!` then has to transform keys itself when a collection is passed to preserve the new behavior introduced by rails#486.
|
I've opened #497 to address the issues. I'll test it now some more in the context of my project before removing the WIP prefix. |
Since rails#486, key format is also applied to nested hashes that are passed as values: json.key_format! camelize: :lower json.settings({some_value: "abc"}) # => { "settings": { "someValue": "abc" }} This breaks code that relied on the previous behavior. Add a `deep_format_keys!` directive that can be used to opt out of the new behavior.
|
Given the current state, #497 looks like an improvement to me. Still, I found even more places in my code that rely on the old behavior in subtle ways. Moreover, my PR now also aligns the behavior of model = Model.new(some_serialized_attribute: {some_key: => 'some value'})
json.key_format! camelize: :lower
# with 2.11
json.some_serialized_attribue model.serialized_attribute_attribute
# => {"someSerializedAttribute": {"someKey": "some value"}}
json.extract!(model, :some_serialized_attribute)
# => {"someSerializedAttribute": {"some_key": "some value"}}
# with #497
json.some_serialized_attribue model.serialized_attribute
json.extract!(model, :some_serialized_attribute)
# => both times: {"someSerializedAttribute": {"someKey": "some value"}}If deep key formatting is supposed to be enabled by default, those two should work the same IMO. Still, for me, this makes things even worse since I have a lot of serialized attributes that would now be transformed. I therefore continued with your initial proposal, to allow opting out of the new behavior via a new The commit builds on #497 at the moment, but could also be turned into its own PR. I would need to remove the test about changing the setting per scope then, though, since this does not work with current Given how disruptive this whole change has been for me, I'd be very much in favor of changing my commit to set |
|
@tf I concur with your analysis. We should turn deep formatting off by default, then change it in the next major. |
|
I've updated #497 to include a reworked commit to disable deep formatting by default. Using the branch, my app no longer breaks. |
Since rails#486, key format was also applied to nested hashes that are passed as values: json.key_format! camelize: :lower json.settings({some_value: "abc"}) # => { "settings": { "someValue": "abc" }} This breaks code that relied on the previous behavior. Add a `deep_format_keys!` directive that can be used to opt into this new behavior: json.key_format! camelize: :lower json.deep_format_keys! json.settings({some_value: "abc"}) # => { "settings": { "someValue": "abc" }}
* Fix changing key format in child elements Since #486, key format is applied deeply to hashes and arrays that are passed to `set!`, `merge!` and `array!`: json.key_format! :upcase json.set!(:foo, {some: "value"}) # => {"FOO": {"SOME": "value"}} json.key_format! :upcase json.merge!({some: "value"}) # => {"SOME": "value"} json.key_format! :upcase json.array!([{some: "value"}]) # => [{"SOME": "value"}] This also works for arrays and hashes extracted from objects: comment = Struct.new(:author).new({ first_name: 'John', last_name: 'Doe' }) json.key_format! camlize: :lower json.set!(:comment, comment, :author) # => {"comment": {"author": {"firstName": "John", "lastName": "Doe"}}} As a side effect of the change, key format is also applied to the result of nested blocks, making it impossible to change key format in the scope of a block: json.key_format! camelize: :lower json.level_one do json.key_format! :upcase json.value 'two' end # => jbuilder 2.10.0: {"levelOne": {"VALUE": "two"}} # => jbuilder 2.11.0: {"levelOne": {"vALUE": "two"}} The string "vALUE" results from calling `"value".upcase.camelize(:lower)`. The same happens when trying to change the key format inside of an `array!` block. This happens since key transformation was added in the `_merge_values` method, which is used both by `merge!` but also when `set!` is called with a block. To restore the previous behavior, we pull the `_transform_keys` call up into `merge!` itself. To make sure extracted hashes and arrays keep being transformed (see comment/author example above), we apply `_transform_keys` in `_extract_hash_values` and `_extract_method_values`. This also aligns the behavior of `extract!` which was left out in #486: comment = {author: { first_name: 'John', last_name: 'Doe' }} result = jbuild do |json| json.key_format! camelize: :lower json.extract! comment, :author end # => jbuilder 2.10 and 2.11: {"author": { :first_name => "John", :last_name => "Doe" }} # => now: {"author": { "firstName": "John", "lastName": "Doe" }} Finally, to fix `array!`, we make it call `_merge_values` directly instead of relying on `merge!`. `array!` then has to transform keys itself when a collection is passed to preserve the new behavior introduced by #486. * Disable deeply formatting keys of nested hashes by default Since #486, key format was also applied to nested hashes that are passed as values: json.key_format! camelize: :lower json.settings({some_value: "abc"}) # => { "settings": { "someValue": "abc" }} This breaks code that relied on the previous behavior. Add a `deep_format_keys!` directive that can be used to opt into this new behavior: json.key_format! camelize: :lower json.deep_format_keys! json.settings({some_value: "abc"}) # => { "settings": { "someValue": "abc" }}
Converts the keys of hashes nested within other hashes and arrays.
Fixes #473