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

Add strict argument checking to ActiveRecord callbacks #30919

Merged
merged 1 commit into from Jul 22, 2018

Conversation

Projects
None yet
8 participants
@seanlinsley
Copy link
Contributor

seanlinsley commented Oct 18, 2017

Summary

This fixes #17622 by adding strict argument checking to ActiveRecord callbacks.

This ends up adding it to all save-related callbacks defined in ActiveRecord::DefineCallbacks, including e.g. after_create. Which should be fine: they didn't support :on in the first place.

Other Information

Should this go in the changelog? It seems like it should be backported to 5.0.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Oct 18, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

seanlinsley added a commit to seanlinsley/paper_trail that referenced this pull request Oct 18, 2017

Fix callback to only run before update
`before_save` doesn't accept `:on` as an argument, so this callback was actually running on all save events, not just on update.

rails/rails#17622

Depending on what `reset_timestamp_attrs_for_update_if_needed` does, this might need to be backported to older versions.

I've submitted a PR to Rails to raise an exception when unexpected arguments are passed... unsure if an exception is the right way to go when there might be quite a few gems with this bug.

rails/rails#30919
@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 18, 2017

As I discovered when using this patch in my application, Paper Trail has been calling before_save on: :update: paper-trail-gem/paper_trail#1001

That makes the roll-out for this a bit more complex... how many other gems does this affect?

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 19, 2017

The only failure was:

Failure:

EventedRedisAdapterTest#test_multiple_broadcast [/home/travis/build/rails/rails/actioncable/test/subscription_adapter/common.rb:35]:

Expected false to be truthy.

Re-running CI.

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 19, 2017

Oh, it looks like I'm not able to... (I thought Travis let PR authors to OSS re-run their own builds). Could someone else re-run them?

@eugeneius

This comment has been minimized.

Copy link
Member

eugeneius commented Oct 19, 2017

Maybe these options should be verified in ActiveSupport::Callbacks.set_callback, since that's where they're used (and documented)?

I think this needs to be a deprecation warning rather than an error, at least initially.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Oct 19, 2017

Agree we should probably just do this globally in set_callback... though going that universal does potentially limit what we can safely say in the exception message. Just stating the options supported by set_callback is probably sufficient, I guess. (In contrast, I think it would be overreach for us to claim to know all the options supported by the caller [e.g. before_save].)

I don't think we need a deprecation, though: AFAICS, there's no sensible reason for someone to currently be deliberately passing unknown options.

seanlinsley added a commit to modernmsg/paper_trail that referenced this pull request Oct 19, 2017

Fix callback to only run before update
`before_save` doesn't accept `:on` as an argument, so this callback was actually running on all save events, not just on update.

rails/rails#17622

Depending on what `reset_timestamp_attrs_for_update_if_needed` does, this might need to be backported to older versions.

I've submitted a PR to Rails to raise an exception when unexpected arguments are passed... unsure if an exception is the right way to go when there might be quite a few gems with this bug.

rails/rails#30919
@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 19, 2017

I would've defined it in set_callback except that would cause before_validation to break:

def before_validation(*args, &block)
options = args.last
if options.is_a?(Hash) && options[:on]
options[:if] = Array(options[:if])
options[:on] = Array(options[:on])
options[:if].unshift ->(o) {
options[:on].include? o.validation_context
}
end
set_callback(:validation, :before, *args, &block)
end

One option would be to update before_validation and after_validation to delete :on since it's wrapped into an :if lambda, but I don't know what side effects that might have.

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Oct 19, 2017

Ah, interesting. Yeah, let's try that and see what happens.

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 19, 2017

Okay. Should I add those changes & remove the previous changes as a new commit, or is it okay to rewrite the history on this branch?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Oct 19, 2017

Rewrite history 👍

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 19, 2017

This sort of approach won't work because of the various arguments passed to validations, for example validates_inclusion_of in:

def set_callback(name, *filter_list, &block)
  # ...

  if (unknown = options.keys - VALID_OPTIONS).any?
    raise ArgumentError, <<-MSG.squish
      Callbacks don't accept #{unknown.map(&:inspect).join(', ')} as an argument.
      Valid keys are #{VALID_OPTIONS.map(&:inspect).join(', ')}.
    MSG
  end

Given that validate implements its own argument checking, it appears that the original implementation in this PR follows the current trend in the Rails source code (even if it's sub-optimal).

@rafaelfranca rafaelfranca assigned matthewd and unassigned pixeltrix Oct 19, 2017

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 20, 2017

The only other option would be to provide a common interface that each callback setter could use to define its expected attributes, essentially:

set_callback(:validation, *args, allowed_options: ALLOWED_OPTIONS, **options, &block)
@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 20, 2017

But that seems like too much code for this simple PR.

We're going to go ahead and start using this patch internally, though I'll of course make any changes necessary for this PR to be mergeable.

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 20, 2017

In our application there were 8 places where :on was incorrectly passed, affecting 6 models. If that's any indication, a lot of people are affected by this. What's the best way communicate this change to prevent friction?

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 21, 2017

It's made worse by gems who may also have this bug, e.g. Paper Trail.

Perhaps 5.0 and 5.1 should show a warning, while 6 (I presume that's the next major version) should raise an exception.

activemodel/lib/active_model/callbacks.rb Outdated
end
end

def verify_options(method, options)

This comment has been minimized.

@matthewd

matthewd Oct 22, 2017

Member

This name is waaaay too generic for us to add everywhere that AMo::Callbacks gets included.

It doesn't actually use any instance state, though, so instead of giving it a big ugly name, let's just move it onto the module itself.

This comment has been minimized.

@seanlinsley

seanlinsley Oct 22, 2017

Author Contributor

Do you mean as a class method (on the module) so it'd be called like this?

Callback.verify_options(__method__, options)

This comment has been minimized.

@matthewd

matthewd Oct 22, 2017

Member

Yep 👍

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 22, 2017

I wonder if the error message could be improved. Perhaps it should link to #17622, for example.

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 24, 2017

Is there someone on the Rails team we should ask about:

What's the best way communicate this change to prevent friction?

?

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 27, 2017

Happily, only a handful of gems appear to have this bug: (note that PaperTrail has a new version released with paper-trail-gem/paper_trail#1001)

{
  "music_blender"=>["lib/music_blender/track.rb"],
  "versioned_record"=>["lib/versioned_record.rb"],
  "activerecord-userstamp"=>["lib/active_record/userstamp/stampable.rb"],
  "rivendell-import"=>["lib/rivendell/import/tasking/destination.rb"],
  "scaffold_plus"=>["lib/generators/scaffold_plus/geodesic/geodesic_generator.rb"],
  "my_todo"=>["lib/my_todo/models/note.rb"],
}

That is assuming the script I wrote is correct: (e.g. is finding reverse dependencies of ActiveRecord the right option?)

auth = 'Authorization:YOUR_API_KEY'

gems = JSON.parse(`curl -H '#{auth}' https://rubygems.org/api/v1/gems/activerecord/reverse_dependencies.json`); nil

gems.size
# => 4479

dir = Dir.mktmpdir

gems.each_with_index do |name, index|
  next if Dir["#{dir}/#{name}/source/*"].any?

  print "\r"
  print "#{index + 1} / #{gems.size}"

  sleep 1

  download_link = JSON.parse(`curl -s -H '#{auth}' https://rubygems.org/api/v1/gems/#{name}.json`)['gem_uri']

  `curl -s -o #{dir}/#{name}.gem #{download_link}`
  `mkdir -p #{dir}/#{name}/source`
  `tar zxf #{dir}/#{name}.gem -C #{dir}/#{name}`
  `tar zxf #{dir}/#{name}/data.tar.gz -C #{dir}/#{name}/source`
end


# ** is recursive in Ruby, but not in Bash
gems.map do |name|
  files = Dir["#{dir}/#{name}/source/**/*.rb"].select do |file|
    begin
      File.read(file) =~ /(before|after|around)_save .*(on:|:on) /
    rescue => e
      puts "failed to read #{file}"
    end
  end
  files.map!{ |file| file.sub "#{dir}/#{name}/source/", '' }
  [name, files] if files.any?
end.compact.to_h
@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Oct 29, 2017

There's also the possibility that the regular expression isn't detecting all uses (e.g. if it's split onto multiple lines)

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Nov 20, 2017

Other than wordsmithing the error message, this should be good to go AFAICT.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Jan 23, 2018

Hm... Is there a reason we can't just change the code to use Ruby keyword arguments rather than crafting our own error message here?

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jan 23, 2018

I believe that'd require code like this:

def _define_before_model_callback(klass, callback)
  klass.define_singleton_method("before_#{callback}") do |*args, if: nil, unless: nil, prepend: nil, &block|
    options = {if: if, unless: unless, prepend: prepend}
    set_callback(:"#{callback}", :before, *args, **options, &block)
  end
end

Which results in a syntax error.

Additionally, keyword arguments aren't great because the exception doesn't tell you what arguments you could've passed instead.

2.5.0 :001 > def a(b: nil, c: nil); end
 => :a 
2.5.0 :002 > a h: 4
ArgumentError (unknown keyword: h)
@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jan 23, 2018

If we want to cut down on the total number of lines in this PR we could use assert_valid_keys:

# Validates all keys in a hash match <tt>*valid_keys</tt>, raising
# +ArgumentError+ on a mismatch.
#
# Note that keys are treated differently than HashWithIndifferentAccess,
# meaning that string and symbol keys will not match.
#
# { name: 'Rob', years: '28' }.assert_valid_keys(:name, :age) # => raises "ArgumentError: Unknown key: :years. Valid keys are: :name, :age"
# { name: 'Rob', age: '28' }.assert_valid_keys('name', 'age') # => raises "ArgumentError: Unknown key: :name. Valid keys are: 'name', 'age'"
# { name: 'Rob', age: '28' }.assert_valid_keys(:name, :age) # => passes, raises nothing
def assert_valid_keys(*valid_keys)
valid_keys.flatten!
each_key do |k|
unless valid_keys.include?(k)
raise ArgumentError.new("Unknown key: #{k.inspect}. Valid keys are: #{valid_keys.map(&:inspect).join(', ')}")
end
end
end

But that'd remove flexibility to customize the error message.

@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Jan 23, 2018

I do think it makes sense to at least use the Active Support method for this that is used elsewhere.

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jan 23, 2018

That'll change the exception from:

before_save does not accept :on. Valid keys are :if, :unless, :prepend.

to:

Unknown key: :on. Valid keys are: :if, :unless, :prepend

That'll make the error a bit more difficult for application developers to identify the issue, but I can update the PR to do that.

@seanlinsley seanlinsley force-pushed the seanlinsley:17622-before_save_strict_arguments branch Jan 23, 2018

activemodel/lib/active_model/callbacks.rb Outdated
@@ -68,6 +68,10 @@ def self.extended(base) #:nodoc:
end
end

def self.verify_options(method, options) # :nodoc:

This comment has been minimized.

@eugeneius

eugeneius Jan 23, 2018

Member

The method argument is no longer used here.

This code is now simple enough that it could probably just be inlined.

This comment has been minimized.

@sgrif

sgrif Jan 30, 2018

Member

Agreed

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jan 23, 2018

I personally prefer the original implementation b/c it's more forgiving to application developers that run into this error.

It's made even more important b/c it appears that Ruby 2.5 is doing something weird with stack traces.

My application running the previous version of this PR, on Ruby 2.4.2:

$ bundle exec rails c
gems/rails-b2a1f5f8afe6/activemodel/lib/active_model/callbacks.rb:155:in `verify_options': before_save does not accept :on. Valid keys are :if, :unless, :prepend. (ArgumentError)
  from gems/rails-b2a1f5f8afe6/activemodel/lib/active_model/callbacks.rb:129:in `block in _define_before_model_callback'
  from app/models/user.rb:119:in `<class:User>'
  from app/models/user.rb:1:in `<main>'

My application running the previous version of this PR, on Ruby 2.5.0:

$ bundle exec rails c
Traceback (most recent call last):
bin/rails: before_save does not accept :on. Valid keys are :if, :unless, :prepend. (ArgumentError)

Ruby 2.5 claims it's including a stack trace, but doesn't actually include one. If we were to use assert_valid_keys, the error becomes completely inscrutable:

$ bundle exec rails c
Traceback (most recent call last):
bin/rails: Unknown key: :on. Valid keys are: :if, :unless, :prepend (ArgumentError)
@sgrif

This comment has been minimized.

Copy link
Member

sgrif commented Jan 30, 2018

I think there's an argument to be made for a more general improvement to assert_valid_keys (e.g. method_name_for_error: :before_save), but I'd prefer to move forward with the smaller implementation for the time being

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jan 30, 2018

Would that sort of thing be considered a breaking change? I'd be happy to open a separate PR to improve assert_valid_keys.

For now I'll simplify this PR.

@seanlinsley seanlinsley force-pushed the seanlinsley:17622-before_save_strict_arguments branch Jan 31, 2018

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Feb 1, 2018

Hmm

`load_defaults': Unknown version "6.0" (RuntimeError)

That seems like something that's likely been fixed on master.

@eugeneius

This comment has been minimized.

Copy link
Member

eugeneius commented Feb 3, 2018

It has: 71a392c

The problem you described with missing backtraces on Ruby 2.5.0 was caused by a bug in the binding_of_caller gem, which has been fixed: banister/binding_of_caller#69

@seanlinsley seanlinsley force-pushed the seanlinsley:17622-before_save_strict_arguments branch Feb 4, 2018

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Feb 4, 2018

👍 thanks for the links @eugeneius. I just rebased this branch so hopefully it'll pass now.

@kamipo kamipo added the activemodel label May 27, 2018

@ybakos

This comment has been minimized.

Copy link
Contributor

ybakos commented Jul 22, 2018

Bump.

ybakos added a commit to osu-cascades/bda-explorer that referenced this pull request Jul 22, 2018

Project: Refactor before_save callback and tests.
The before_save on: :create callback doesn't do what one might think.
Rails silently ignores the on: :create option for before_save, so this
is no different than a plain before_save. See:

rails/rails#17622
rails/rails#30919

Refactor the tests, to make them a little more explicit and descriptive.
Add latitude and longitude to the factory, so that new instances are
valid.
activerecord/test/cases/callbacks_test.rb Outdated
after_save(on: :create) {}
end
end
assert_equal exception.message, "Unknown key: :on. Valid keys are: :if, :unless, :prepend"

This comment has been minimized.

@eugeneius

eugeneius Jul 22, 2018

Member

These assertions are backwards: the expected value should be passed first, and the actual value second.

This comment has been minimized.

@seanlinsley

seanlinsley Jul 22, 2018

Author Contributor

Oh, okay. I'm usually use Rspec, which AFAIK tends to follow the opposite pattern.

Is there a style guide documenting things like this?

This comment has been minimized.

@ybakos

ybakos Jul 22, 2018

Contributor

@seanlinsley It's an xUnit idiom. All assertion methods are of the form assert(expected, actual). Rspec just does it the other way, for the semantics of their api, eg expect(actual).to be(expected). FWIW, some JS test suites also use the order actual, expected. 😮

@seanlinsley seanlinsley force-pushed the seanlinsley:17622-before_save_strict_arguments branch Jul 22, 2018

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jul 22, 2018

Ran into this error on CI:

$ sudo -E apt-get -yq --no-install-suggests --no-install-recommends $TRAVIS_APT_OPTS install ffmpeg mupdf mupdf-tools poppler-utils
Reading package lists...
Building dependency tree...
Reading state information...
Package ffmpeg is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source

rebase in progress...

add strict argument checking to ActiveRecord callbacks
This ends up adding it to all save-related callbacks defined in `ActiveRecord::DefineCallbacks`, including e.g. `after_create`. Which should be fine: they didn't support `:on` in the first place.

@seanlinsley seanlinsley force-pushed the seanlinsley:17622-before_save_strict_arguments branch to dfb0e4b Jul 22, 2018

@kamipo kamipo merged commit dfb0e4b into rails:master Jul 22, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details

kamipo added a commit that referenced this pull request Jul 22, 2018

Merge pull request #30919 from seanlinsley/17622-before_save_strict_a…
…rguments

Add strict argument checking to ActiveRecord callbacks

@seanlinsley seanlinsley deleted the seanlinsley:17622-before_save_strict_arguments branch Jul 22, 2018

@seanlinsley

This comment has been minimized.

Copy link
Contributor Author

seanlinsley commented Jul 22, 2018

Thanks for merging this. Thoughts on whether this should have a changelog entry, and if it should be backported?

I just came across #33342 which proposes showing a deprecation warning for a similar scenario, then changing that to an argument error in a following version. Should this PR have done that as well?

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Jul 22, 2018

I've merged this PR as an improvement/kaizen for a future version of Rails, so basically isn't backported according to our maintenance policy. It is probably a good idea to have a changelog entry.

This is similar to #33347 and #33029, but is not similar to #33342.
This and #33347 improves that silently ignored unspported options, #33029 improves that raised unhelpful error, by making error message more helpful.

But #33342 proposes deprecating one of the use case which may any apps depending on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.