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

Allow deprecated non-symbol access to nested config_for hashes #35198

Merged
merged 2 commits into from Feb 11, 2019

Conversation

@paracycle
Copy link
Contributor

@paracycle paracycle commented Feb 8, 2019

Summary

A change to config_for in #33815 and #33882 has altered the behaviour of the returned object in a breaking manner.

Before that change, nested hashes returned from config_for could be accessed using non-symbol keys. After the change, all keys are recursively symbolized so non-symbol access fails to read the expected values.

This is a breaking change for any app that might be relying on the nested hashes returned from config_for calls, and thus should be deprecated before being removed from the codebase.

Solution

This PR introduces a temporary NonSymbolAccessDeprecatedHash class that recursively wraps any nested hashes inside the OrderedOptions object returned from config_for and issues a deprecation notice when a non-symbol based access is performed.

This way, any apps that are still relying on the ability to access these nested hashes using non-symbol keys will be able to observe the deprecation notices and have time to implement fixes before non-symbol access is removed for good.

Other Information

Note that the top-level access to the OrderedOptions object returned from config_for has indifferent access semantics due to the nature of how OrderedOptions works. This functionality is the same for Rails.config and secrets, so is not changed nor is any deprecation notice issued.

The deprecation is only for nested keys below the top-level, since that is the functionality that is potentially broken.

@rails-bot rails-bot bot added the railties label Feb 8, 2019
@paracycle paracycle force-pushed the uk-change-config-for-behaviour branch from 0ce8747 to bdb499d Feb 8, 2019
railties/lib/rails/application.rb Outdated Show resolved Hide resolved
railties/lib/rails/application.rb Outdated Show resolved Hide resolved
railties/lib/rails/application.rb Show resolved Hide resolved
Copy link
Member

@gmcgibbon gmcgibbon left a comment

This might also be worth mentioning in the changelog as a bugfix for the next release. Can you add a changelog entry?

railties/lib/rails/application.rb Outdated Show resolved Hide resolved
railties/lib/rails/application.rb Show resolved Hide resolved
@paracycle paracycle force-pushed the uk-change-config-for-behaviour branch 3 times, most recently from c3cb377 to 6ac7aff Feb 10, 2019
@paracycle
Copy link
Contributor Author

@paracycle paracycle commented Feb 10, 2019

I think I've addressed all PR comments and squashed my commits, so this PR should be ready to go if there are no more comments.

Additionally, I've added an extra commit to fix the order of expected/actual parameters of some of the assertions added in #33882. Since that is logically separate from the main work in this PR, I've opted to leave that as a separate commit, and didn't squash it with my other commits.

@@ -590,5 +590,81 @@ def build_request(env)
def build_middleware
config.app_middleware + super
end

class NonSymbolAccessDeprecatedHash < Hash # :nodoc:
Copy link
Member

@Edouard-chin Edouard-chin Feb 11, 2019

Implementation seems similar to HWA, why not inheriting from it and just override the convert_key method ?

Copy link
Contributor Author

@paracycle paracycle Feb 11, 2019

That's exactly what I tried first and decided against later, for 2 reasons:

  1. HWIA stores keys as strings, but we want symbols. So it felt like I was resisting the base class.
  2. Even more importantly, convert_key is called during both read and write, and there is no way to know from which it is called. That would mean we would show deprecation notices as we are interning the YAML parsed hashes and thus showing notices to people who don't even access hashes via string keys. That would be confusing.

Basically HWIA just was not flexible enough for this purpose. But I did take most of the implementation straight from it.

Hope this addresses your concern.

Copy link
Member

@gmcgibbon gmcgibbon Feb 11, 2019

I applied this change to your branch.

1. HWIA stores keys as strings, but we want symbols. So it felt like I was resisting the base class.

This seems to work as expected in my tests, and passes the test suite: Rails.application.config.my_custom_config[:foo] => {:bar=>["baz"] }

2. Even more importantly, convert_key is called during both read and write, and there is no way to know from which it is called. That would mean we would show deprecation notices as we are interning the YAML parsed hashes and thus showing notices to people who don't even access hashes via string keys. That would be confusing.

I'm not seeing this behaviour. Can you explain how this would happen?

We can revert this to not extend from HWIA if need be, but it seems to work as expected AFAICT.

Copy link
Contributor Author

@paracycle paracycle Feb 11, 2019

So the []= method of HWIA calls convert_key here: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/hash_with_indifferent_access.rb#L94 which would be called ultimately from a source that has nested hashes. If those hashes don't have string keys, we would get a deprecation notice. I guess what's protecting the code from doing that is the fact that you are calling .to_sym before passing key to super, so that writes always get a symbol key.

I guess that solves it more elegantly than I had it. Thanks for the rework and the simplification. 👍

@gmcgibbon gmcgibbon force-pushed the uk-change-config-for-behaviour branch from 6ac7aff to 699ebef Feb 11, 2019
paracycle added 2 commits Feb 11, 2019
A change to `Rails::Application.config_for` in
rails#33815 and
rails#33882 has altered the behaviour of
the returned object in a breaking manner. Before that change, nested
hashes returned from `config_for` could be accessed using non-symbol keys.
After the change, all keys are recursively symbolized so non-symbol access
fails to read the expected values.

This is a breaking change for any app that might be relying on the
nested hashes returned from `config_for` calls, and thus should be
deprecated before being removed from the codebase.

This commit introduces a temporary `NonSymbolAccessDeprecatedHash` class
that recursively wraps any nested hashes inside the `OrderedOptions`
object returned from `config_for` and issues a deprecation notice when a
non-symbol based access is performed.

This way, apps that are still relying on the ability to access these
nested hashes using non-symbol keys will be able to observe the
deprecation notices and have time to implement changes before non-symbol
access is removed for good.

A CHANGELOG entry is also added to note that non-symbol access to nested
`config_for` hashes is deprecated.
The assertion from the previous PR had the expected and the actual
values in the wrong order, so when a test failed the error message was
confusing.

This commit fixes the problem by switching the order.
@gmcgibbon gmcgibbon force-pushed the uk-change-config-for-behaviour branch from 699ebef to de96628 Feb 11, 2019
@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Feb 11, 2019
@gmcgibbon gmcgibbon merged commit f9e6819 into rails:master Feb 11, 2019
2 checks passed
pudiva added a commit to alphagov/datagovuk_find that referenced this issue Jul 17, 2020
pudiva added a commit to alphagov/datagovuk_find that referenced this issue Jul 22, 2020
pudiva added a commit to alphagov/datagovuk_find that referenced this issue Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants