Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Callback collision #269

Closed
wants to merge 3 commits into from

5 participants

@mandrews

Add unit test to demonstrate issue with similar callback methods overriding each other in distinct contexts

@phene

I think this also affects after_commit callbacks. Should we add a test for that, too?

@josevalim josevalim was assigned
@bgentry

Can we please get this merged in prior to the 3.1 release?

@josevalim
Owner

Thanks for the patch with the fix but it does not make sense to hardcode options[:on]. The :on option just makes sense for a few callbacks and does not belong conceptually in AS::Callbacks. I am still unsure on how to properly solve the issue. Maybe we should allow :on to receive an array?

@bgentry

I would be even happier if I could do :on => [ :create, :update ]

@josevalim
Owner

@bgentry that is easier to implement. You just need to rewrite this:

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/validations.rb#L136

As something like:

options[:if].unshift("#{Array.wrap(options[:on]).inspect}.include?(validation_context)")

Also in a couple other places like:

https://github.com/rails/rails/blob/master/activemodel/lib/active_model/validations/callbacks.rb
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/transactions.rb

@mandrews

@josevalim- I agree that hard coding :on is not appropriate. How about instead of white listing options that imply difference, we black list options that determine equivalence.

@josevalim
Owner

@mandrews I believe black listing would work fine. +1

@sikachu
Collaborator

Note: the target of this commit is currently goes to 3.0.6, which is so old. I believe there's no way to change it, so maybe @josevalim can merge it into master instead.

I believe this is a bugfix, so targets are for 3-0-stable, 3-0-9, 3-1-stable and master. And please don't forget the CHANGELOG entry. ;)

@mandrews

I rebased to master, ran the unit tests and created a new pull request:
#1777

@josevalim
Owner

Closed in favor of #1777

@josevalim josevalim closed this
@mandrews mandrews referenced this pull request
Closed

Callback collision #1777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 14, 2011
  1. @mandrews

    Add unit test to demonstrate issue with similar callback methods over…

    mandrews authored
    …riding each other in distinct contexts
  2. @mandrews
Commits on May 12, 2011
  1. @mandrews
This page is out of date. Refresh to see the latest.
View
51 activerecord/test/cases/callbacks_test.rb
@@ -138,6 +138,34 @@ def history
end
end
+class ContextualCallbacksDeveloper < ActiveRecord::Base
+ set_table_name 'developers'
+
+ before_validation { history << :before_validation }
+ before_validation :before_validation_on_create_and_update, :on => :create
+ before_validation :before_validation_on_create_and_update, :on => :update
+
+ validate do
+ history << :validate
+ end
+
+ after_validation { history << :after_validation }
+ after_validation :after_validation_on_create_and_update, :on => :create
+ after_validation :after_validation_on_create_and_update, :on => :update
+
+ def before_validation_on_create_and_update
+ history << "before_validation_on_#{self.validation_context}".to_sym
+ end
+
+ def after_validation_on_create_and_update
+ history << "after_validation_on_#{self.validation_context}".to_sym
+ end
+
+ def history
+ @history ||= []
+ end
+end
+
class CallbackCancellationDeveloper < ActiveRecord::Base
set_table_name 'developers'
@@ -286,6 +314,17 @@ def test_validate_on_create
], david.history
end
+ def test_validate_on_contextual_create
+ david = ContextualCallbacksDeveloper.create('name' => 'David', 'salary' => 1000000)
+ assert_equal [
+ :before_validation,
+ :before_validation_on_create,
+ :validate,
+ :after_validation,
+ :after_validation_on_create
+ ], david.history
+ end
+
def test_update
david = CallbackDeveloper.find(1)
david.save
@@ -345,6 +384,18 @@ def test_validate_on_update
], david.history
end
+ def test_validate_on_contextual_update
+ david = ContextualCallbacksDeveloper.find(1)
+ david.save
+ assert_equal [
+ :before_validation,
+ :before_validation_on_update,
+ :validate,
+ :after_validation,
+ :after_validation_on_update
+ ], david.history
+ end
+
def test_destroy
david = CallbackDeveloper.find(1)
david.destroy
View
12 activesupport/lib/active_support/callbacks.rb
@@ -147,8 +147,12 @@ def next_id
@@_callback_sequence += 1
end
- def matches?(_kind, _filter)
- @kind == _kind && @filter == _filter
+ ISOMORPHIC_OPTIONS = [:if, :unless, :per_key]
+
+ def matches?(_kind, _filter, _options = {})
+ @kind == _kind && @filter == _filter &&
+ @options.reject { |k,v| ISOMORPHIC_OPTIONS.include?(k) } ==
+ _options.reject { |k,v| ISOMORPHIC_OPTIONS.include?(k) }
end
def _update_filter(filter_options, new_options)
@@ -485,7 +489,7 @@ def set_callback(name, *filter_list, &block)
end
filters.each do |filter|
- chain.delete_if {|c| c.matches?(type, filter) }
+ chain.delete_if {|c| c.matches?(type, filter, options) }
end
options[:prepend] ? chain.unshift(*(mapped.reverse)) : chain.push(*mapped)
@@ -497,7 +501,7 @@ def set_callback(name, *filter_list, &block)
def skip_callback(name, *filter_list, &block)
__update_callbacks(name, filter_list, block) do |chain, type, filters, options|
filters.each do |filter|
- filter = chain.find {|c| c.matches?(type, filter) }
+ filter = chain.find {|c| c.matches?(type, filter, options) }
if filter && options.any?
new_filter = filter.clone(chain, self)
Something went wrong with that request. Please try again.