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
Introduce ActiveSupport Disallowed Deprecations #37940
Introduce ActiveSupport Disallowed Deprecations #37940
Conversation
@rafaelfranca not sure whether you would benefit from this at Shopify but we'd like to get this merged into Rails. We often want to ban deprecations on our Rails next build but it can be hard to do that since we silence deprecations. We don't want all deprecations to be a hard fail so this would allow us to say "ok we no longer will allow this deprecation". |
We use https://github.com/Shopify/deprecation_toolkit to solve this problem. But the mechanism is different. It allow deprecations in some tests by recording the existent deprecations. Deprecations that are not recorded, either because a test that touches existent deprecated code was added, or because new deprecated code was added, will fail the tests. I feel the approach take by deprecation toolkit is better because in order to disallow one deprecation with this approach you first need to fix all occurrences of it, and while you are fixing one part of the app, people can still add new deprecated code. |
The ideal solution I'd like to see something similar to deprecation toolkit but instead of recording the deprecation message recording the deprecation call site. Something like: I allow the file |
@rafaelfranca the problem with using the callsite as identifier is that adding a simple blank line can change the identifier. Method fully qualified name might be an option ( Also both case mean getting a stackframe which ideally wouldn't run in production. |
I agree that the callsite is ideal to use, although I would recommend using file path and method name or label (to also include indication of block level, since they can easily be obtained from |
It's already possible in the deprecation toolkit to allow/forbid deprecation based on the call site. DeprecationToolkit::Configuration.allowed_deprecations = [
->(message, stack) { message ~= 'Foo' && stack.first.label == 'method_triggering_deprecation' }
] |
Thanks for the feedback. I think I understand the overall concern and it raises a good point, that we'll want to provide a way for a given block of code to allow deprecations while they are being fixed, but we're not entirely sold on the idea of recording that information to data files. I'd hoped to achieve a light, simple solution with minimal config, setup and maintenance requirements. I don't personally have any experience using DeprecationToolkit, but it does seem a good deal more complex. My opinion is that Rails can provide a solid, simple way of managing disallowed deprecations in dev/test/prod without adding much overhead (recorded files / stackframes). In a case where Rails doesn't meet the need of an app, then a gem like DeprecationToolkit would certainly be a great alternative. I do agree that the ability to provide time for a particular part of the app to have deprecated code removed is important. I would suggest that instead of recording that information outside of the code, that information be made part of the code itself using something like: allow_deprecations ["bad_method is going away"] do
User.code_that_calls_bad_method
end or allow_deprecations ["bad_method is going away"], if: Rails.env.production? do
User.code_that_calls_bad_method
end I do realize that this would require editing code in each location instead of recording all existing deprecation instances at once, but the advantage I see is that it makes it very clear in the code itself that there is a pending issue that needs to be addressed. It is not "out of sight out of mind". @rafaelfranca, I think this addresses the need that you've expressed, though goes about it in a very different way that might better accommodate GitHub's preferred work flow as well. Could I ask if you have any concerns about the alternative I described above? |
Yeah, but that's just doesn't really work with our workflow. When updating Rails we can have thousands of callsites throwing deprecations. Hence why it's recorded separately, that allow us to record them automatically, and then grind them down. Silencing them on each callsite like you suggest, very often it's almost as much work as solving the deprecation itself. |
Thanks everyone for their input. Since Shopify already has a gem that works our intention isn't to change your workflow. Is the intention to upstream the deprecation toolkit? I think for us and for smaller Rails apps Deprecation Toolkit is more complex than we need. This solution is lightweight, easy to use and implement, and doesn't require recording call sites. I'd like to focus on reviewing this solution if the intention isn't to implement deprecation toolkit in Rails. |
I like this approach and could be well paired with the current code in this PR. |
We don't have plans to upstream the toolkit yet, but we plan to release a gem called that will codify most of the process we use to upgrade Rails applications and that will include some sort of solution for deprecations, possibility similar to the deprecation toolkit, but focusing on recording the call site not the test. Maybe in the future this gem will be part of Rails, but I prefer to grow it outside for now. Like Jean mentioned, changing code to allow a deprecation will likely be as costly as changing the code to not call the deprecated method. While I see a simpler solution is good and we should include it, I think it is not the best solution for the problem, but I accept it as a temporary solution for the problem. |
I suppose both the proposed block method and the external file can be backed by the same underlying stack trace checking code. |
👍 For me for the idea From experience, a lot of deprecations calls usually come from few hot code path, so silencing each of them isn't too much of an issue since there isn't many. |
I appreciate everyone's input and thoughtful responses. 🙂 I can see some more detailed use cases (i.e. Shopify) and I'd be happy to see the deprecation handling evolve and improve to fit more workflows over time. I'm glad this will at least work as a starting point. I'll work on adding a |
I've added |
abd4394
to
a9a15a2
Compare
@@ -37,6 +41,44 @@ def silence(&block) | |||
@silenced_thread.bind(true, &block) | |||
end | |||
|
|||
# Allow previously disallowd deprecation warnings within the block. |
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.
There's a typo here which ought to be "disallowed".
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.
Thanks. Fixed in df0959b
# | ||
# The optional <tt>if:</tt> argument accepts a truthy/falsy value or an object that | ||
# responds to <tt>.call</tt>. If truthy, then matching warnings will be allowed. | ||
# If falsey then the method yields to the block without allowing the warning. |
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.
To me, the spelling "falsy" reads a little better.
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.
Looking at the Rails and Ruby source, both spellings are used. I'm going to leave this as-is just to avoid a little bike-shedding but thanks for the suggestion.
if_conditional = binding.local_variable_get(:if) | ||
if_conditional = if_conditional.call if if_conditional.respond_to?(:call) | ||
if if_conditional |
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.
Would renaming if_conditional
to conditional
make this method body easier to read?
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'm not opposed to this. Changed in df0959b
a9a15a2
to
df0959b
Compare
@rafaelfranca I think I've addressed the requested changes / improvements but if there's anything else you (or anyone else) sees as important to change let me know. |
5628ebb
to
df8e690
Compare
This allows deprecation messages to be matched by substring, symbol (treated as substring), or regular expression. If a warning is matched, the behaviors configured for disallowed deprecations will be used. The default behavior for disallowed deprecation warnings is `:raise`. Also adds `ActiveSupport::Deprecation.allow` for thread-local, block level ignoring of deprecation warnings which would otherwise be disallowed by ActiveSupport::Deprecation.disallowed_warnings.
df8e690
to
10754f7
Compare
This PR is a result of some of the work we're doing at GitHub with Rails updates and keeping up to date with Rails master.
Use Case
When we eliminate deprecation warnings that appear in our app we want to be sure that the deprecations are never re-introduced. Often this means adding a linting test to CI or a new Rubocop rule which is frequently less than ideal. For example, the default uniqueness validator in Rails 6.0 generates deprecation warnings for string columns indicating that 6.1 will no longer enforce case sensitive comparison. Adding a Rubocop rule to enforce
case_sensitive: true
for only attributes backed by astring
type column is difficult.Our solution to this problem is to allow specific deprecation warnings to be configured as disallowed after they have been removed from the codebase. If a disallowed deprecation is encountered, it will be treated as a hard failure and raise an exception in dev/test. In production we log the deprecation as though it were an error.
Rails Disallowed Deprecations
This PR introduces
ActiveSupport::Deprecation.disallowed_warnings=
which allows the configuration of rules to match deprecation warnings that should not be allowed within the app andActiveSupport::Deprecation.disallowed_behavior
which specifies the behavior to be used when a disallowed deprecation warning is matched.Configuration
Rules are defined as an array. Array elements can be a:
String
- Matches ifstring
is a substring of messageSymbol
- Matches ifsymbol.to_s
is a substring of messageRegexp
- Matches ifexpression.match(message)
Disallowed deprecations can also be configured to the symbol
:all
which will raise an exception for all deprecation warnings.The behavior for disallowed messages defaults to
:raise
if nothing else is set. Alternatively the behavior can be configured in the same way as.behavior=
.Temporarily re-allowing disallowed deprecations
This PR also introduces
ActiveSupport::Deprecation.allow do ... end
This method allows thread-local ignoring of deprecation warnings which would otherwise be disallowed by
ActiveSupport::Deprecation.disallowed_warnings
for the duration of the given block.allow
accepts an optionalallowed_warnings
argument which can be an array of string, symbol, or regular expression elements. If a single scalar value is passed it will be wrapped in an array. Alternatively the single symbol:all
can be passed to re-allow all deprecation messages.:all
is the default argument value.Example - Given disallowed deprecations configured as:
All deprecations can be temporarily re-allowed using:
One or more specific deprecations can be temporarily re-allowed using:
Conditionally re-allowing disallowed deprecations
It's possible to re-allow a disallowed deprecation only if a condition is true. For example, assuming that
bad_method
is disallowed in dev, test, and production but you wanted to allow it in one particular case, only in production. One option would be to duplicate the code causing the deprecation:Instead the
if:
kwarg will bypass the allow behavior if not true and yield the block immediately.The
if:
argument can also take an object that responds to.call
.Other Ways of Doing This
This workflow can be accomplished in a couple of different, albeit less convenient, ways without this PR. The first is to use
:notify
as a behavior. In our app, we always rescue exceptions raised in notifications, so using[:notify]
does not meet our needs.The second (which we implemented) is to use an custom behavior. That approach works well. The only real drawback is that it requires a bit more work in the application to set up.
In the interest of promoting Rails upgrades and application health, this seems like a useful addition to the
ActiveSupport::Deprecation
API.