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

Introduce ActiveSupport Disallowed Deprecations #37940

Merged

Conversation

@cpruitt
Copy link
Contributor

cpruitt commented Dec 11, 2019

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 a string 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 GitHub::Deprecation.disallowed_warnings= which allows the configuration of rules to match deprecation warnings that should not be allowed within the app and ActiveSupport::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 if string is a substring of message
Symbol - Matches if symbol.to_s is a substring of message
Regexp - Matches if expression.match(message)

ActiveSupport::Deprecation.disallowed_warnings = [
  "bad_method",
   :worse_method,
  /(horrible|unsafe)_method/,
]

Disallowed deprecations can also be configured to the symbol :all which will raise an exception for all deprecation warnings.

ActiveSupport::Deprecation.disallowed_warnings = :all

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=.

if Rails.env.production?
  ActiveSupport::Deprecation.disallowed_behavior = [:log]
else
  ActiveSupport::Deprecation.disallowed_behavior = [:raise]
end

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 optional allowed_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:

ActiveSupport::Deprecation.disallowed_behavior = :raise
ActiveSupport::Deprecation.disallowed_warnings = [
  "bad_method is deprecated",
  :worse_method,
  /(horrible/unsafe)_method/
]

All deprecations can be temporarily re-allowed using:

ActiveSupport::Deprecation.allow do
  User.do_thing_that_calls_bad_and_worse_method
end

One or more specific deprecations can be temporarily re-allowed using:

ActiveSupport::Deprecation.allow [:bad_method, "worse_method"] do
  User.do_thing_that_calls_bad_and_worse_method
end

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:

if Rails.env.production?
  ActiveSupport::Deprecation.allow [:bad_method] do
    User.do_thing_that_calls_bad_method
  end
else
  User.do_thing_that_calls_bad_method
endf

Instead the if: kwarg will bypass the allow behavior if not true and yield the block immediately.

ActiveSupport::Deprecation.allow [:bad_method], if: Rails.env.production? do
  User.do_thing_that_calls_bad_method
end

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.

@eileencodes

This comment has been minimized.

Copy link
Member

eileencodes commented Dec 11, 2019

@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".

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 11, 2019

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.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 11, 2019

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 app/models/user.rb line 27 to call case_sensitive: true but not other parts of the app.

@casperisfine

This comment has been minimized.

Copy link

casperisfine commented Dec 11, 2019

@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 (Namespace::Class#method).

Also both case mean getting a stackframe which ideally wouldn't run in production.

@dylanahsmith

This comment has been minimized.

Copy link
Contributor

dylanahsmith commented Dec 11, 2019

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 caller_locations. For MRI, it could also be optimized using the rb_profile_frames C API.

@Edouard-chin

This comment has been minimized.

Copy link
Member

Edouard-chin commented Dec 12, 2019

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:

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' }
]
@cpruitt

This comment has been minimized.

Copy link
Contributor Author

cpruitt commented Dec 12, 2019

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?

@casperisfine

This comment has been minimized.

Copy link

casperisfine commented Dec 12, 2019

I do realize that this would require editing code in each location instead of recording all existing deprecation instances at once

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.

@eileencodes

This comment has been minimized.

Copy link
Member

eileencodes commented Dec 12, 2019

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.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 12, 2019

@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?

I like this approach and could be well paired with the current code in this PR.

@eileencodes eileencodes added this to the 6.1.0 milestone Dec 12, 2019
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Dec 12, 2019

Is the intention to upstream the deprecation toolkit?

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.

@casperisfine

This comment has been minimized.

Copy link

casperisfine commented Dec 12, 2019

I suppose both the proposed block method and the external file can be backed by the same underlying stack trace checking code.

@Edouard-chin

This comment has been minimized.

Copy link
Member

Edouard-chin commented Dec 12, 2019

👍 For me for the idea allow_deprecations do ... I agree with Rafael that it's not the best solution but I think it will fit a lot of application use cases so we should include it in the framework

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.

@cpruitt

This comment has been minimized.

Copy link
Contributor Author

cpruitt commented Dec 12, 2019

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 allow_deprecations do ... to this PR. Thanks all!

@cpruitt

This comment has been minimized.

Copy link
Contributor Author

cpruitt commented Dec 19, 2019

I've added ActiveSupport::Deprecation.allow do ... end and updated the examples in the PR to describe it's use. If anything else can be added to improve this I'm happy to take care of it. 🙂

@cpruitt cpruitt force-pushed the cpruitt:activesupport-disallowed-deprecations branch from abd4394 to a9a15a2 Dec 19, 2019
@@ -37,6 +41,44 @@ def silence(&block)
@silenced_thread.bind(true, &block)
end

# Allow previously disallowd deprecation warnings within the block.

This comment has been minimized.

Copy link
@peterb

peterb Dec 21, 2019

Contributor

There's a typo here which ought to be "disallowed".

This comment has been minimized.

Copy link
@cpruitt

cpruitt Jan 9, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@peterb

peterb Dec 21, 2019

Contributor

To me, the spelling "falsy" reads a little better.

This comment has been minimized.

Copy link
@cpruitt

cpruitt Jan 9, 2020

Author Contributor

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
Comment on lines 73 to 75

This comment has been minimized.

Copy link
@peterb

peterb Dec 21, 2019

Contributor

Would renaming if_conditional to conditional make this method body easier to read?

This comment has been minimized.

Copy link
@cpruitt

cpruitt Jan 9, 2020

Author Contributor

I'm not opposed to this. Changed in df0959b

@cpruitt cpruitt force-pushed the cpruitt:activesupport-disallowed-deprecations branch from a9a15a2 to df0959b Jan 9, 2020
@cpruitt

This comment has been minimized.

Copy link
Contributor Author

cpruitt commented Jan 9, 2020

@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.

@cpruitt cpruitt force-pushed the cpruitt:activesupport-disallowed-deprecations branch 4 times, most recently from 5628ebb to df8e690 Jan 10, 2020
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.
@cpruitt cpruitt force-pushed the cpruitt:activesupport-disallowed-deprecations branch from df8e690 to 10754f7 Jan 15, 2020
@eileencodes eileencodes merged commit 464a86f into rails:master Jan 15, 2020
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #66480 passed (19 minutes, 20 seconds)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.