-
Notifications
You must be signed in to change notification settings - Fork 22k
Allow deprecated non-symbol access to nested config_for
hashes
#35198
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
Conversation
0ce8747
to
bdb499d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be worth mentioning in the changelog as a bugfix for the next release. Can you add a changelog entry?
c3cb377
to
6ac7aff
Compare
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. |
railties/lib/rails/application.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation seems similar to HWA, why not inheriting from it and just override the convert_key
method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly what I tried first and decided against later, for 2 reasons:
- HWIA stores keys as strings, but we want symbols. So it felt like I was resisting the base class.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 👍
6ac7aff
to
699ebef
Compare
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.
699ebef
to
de96628
Compare
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 theOrderedOptions
object returned fromconfig_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 fromconfig_for
has indifferent access semantics due to the nature of howOrderedOptions
works. This functionality is the same forRails.config
andsecrets
, 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.