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

Improve key formatting for nested hashes and disable by default #497

Merged
merged 2 commits into from
Jan 27, 2021

Conversation

tf
Copy link
Contributor

@tf tf commented Jan 27, 2021

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"}}}

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 and disable it by default.

 json.key_format! camelize: :lower
 json.deep_format_keys!
 json.settings({some_value: "abc"})

 # => { "settings": { "someValue": "abc" }}

Moreover, as a side effect of #486, key format was 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.

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.
@tf tf changed the title [WIP] Fix changing key format in child elements Improve key formatting for nested hashes and disable by default Jan 27, 2021
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" }}
@dhh dhh merged commit 761da5c into rails:master Jan 27, 2021
@dhh
Copy link
Member

dhh commented Jan 27, 2021

Great work @tf! One question: Why did you update jbuilder for your app in the first place? By explicitly calling bundle update jbuilder or by accident?

@dhh
Copy link
Member

dhh commented Jan 27, 2021

Released as 2.11.2.

@tf
Copy link
Contributor Author

tf commented Jan 27, 2021

jbuilder comes as a dependency of our pageflow engine gem using a (SemVer)-optimistic version constraint. Updating the engine in one of the host applications during development also triggered the minor version update. So things only ever broke on my local machine.

@tf tf deleted the nested-key-format-fix branch January 27, 2021 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants