Fix for key_format! to handle nested hashes

[blackjack26]

Converts the keys of hashes nested within other hashes and arrays.

Fixes #473

[venki09]

+1 looking for this feature as well

[daveslutzkin]

Another +1 here.

[tf]

I agree that this is a good idea in general. Still, it is a breaking change that is now breaking my app.

[dhh]

tf, your app was expecting that the key_format wasn't being honored below the first level?

[tf]

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 i18n-js in client side code. While the overall JSON seed uses camel case keys, the code relies on JBuilder preserving the snake case keys in the nested hash such that I18n.t calls can use the same key format in JS and on the server.

[dhh]

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.

[tf]

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
end

This no longer works, though, since with the change in this PR the camelize key format is applied to the result of the translations block, formatting the keys again that had been underscore formatted before.

I pushed a test that passes for 2.10 and fails for 2.11: https://github.com/tf/jbuilder/commit/136be93e7dad05bedbc8680d3bb53bcd5b55d2e4

[dhh]

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.

[tf]

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.

[tf]

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 extract! that was overlooked in the changes of this PR:

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 deep_format_keys! false directive in https://github.com/tf/jbuilder/commit/613931fdf0b90dea2ccabac81dacfc73260464b1. I'm open for suggestions regarding a better name if you can think of one.

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 master for the same reasons key_format! currently does not work in blocks.

Given how disruptive this whole change has been for me, I'd be very much in favor of changing my commit to set deep_format_keys to false by default, but that's probably a matter of different perspectives.

[dhh]

@tf I concur with your analysis. We should turn deep formatting off by default, then change it in the next major.

[tf]

I've updated #497 to include a reworked commit to disable deep formatting by default. Using the branch, my app no longer breaks.