before_validation callbacks being run AFTER validation [ActveModel 3.0.0.beta3]

I think it's because is a before_validate.

The validate method is not a normal method. In ActiveRecord, there are no callback on validate. The callback is in valid? method and call before/after_validation.

For me it's not a really bug.

@Cyril

This isn't ActiveRecord, though. The whole idea of ActiveModel is that callbacks, validations, etc should be able to be used independently of ActiveRecord. If you look on lines 150-154 of activemodel/validations.rb, you'll see that valid? is indeed calling the validation callbacks, and it's only the name that's been changed to be more consistent with other callbacks, which use verbs (on_create) rather than nouns (on_creation). before_validation isn't in ActiveModel, and therefore no longer exists in Rails 3, having been replaced by before_validate.

Regardless of internal implementation, this still seems like a bug in ActiveModel, because the only thing I'm changing between examples is the order of the declaration, which historically (in ActiveRecord) hasn't mattered. Nor am I calling validate directly: I'm calling valid? as you suggest. Once again, I've only declared the callback with before_validate because that is what it is called in ActiveModel.

M

I check again the ActiveModel code.

We can see that ActiveModel::Validation use the validate callback to chain his validation.

So like callback are call by chaining in order of define, if you define a validate before your before_validate, the validate methode add a callback :validate. But if you define a before_validate is add to chain. So if it's before all validation is define in first, instead if call in order.

So it's really not a bug. the validate callback is define by ActiveModel::Validation, so it's reserved callback. we can't use it if you use ActiveModel::Validation.

The solution is change ActiveModel::Validations and avoid using ActiveSupport::Callback. Or avoid it.

I prefer avoiding it.

@Matt I created a patch that applies to master to demonstrate the issue. It's just a rewrite of your spec. I'm pretty confused as to the right way to use the new validation api. Here's 3 ways of defining a validation, all of which behave differently:

class Person
  include ActiveModel::Validations
  extend ActiveModel::Callbacks
  define_model_callbacks :validate

  attr_accessor :name

  def default_name
    @name = "John Smith"
  end
end

# Doesn't run callback before validation
class A < Person
  validate { validates_presence_of :name }
  before_validate :default_name
end

# Works as expected
class B < Person
  def validate
    validates_presence_of :name
  end
  before_validate :default_name
end

# NoMethodError: undefined method `before_validate' for #<ActiveModel::Validations::PresenceValidator>
class C < Person
  validates_presence_of :name
  before_validate :default_name
end

Just to recap and to make it easier for others to understand please take a look at following code.

class Developer
  include ActiveModel::Validations
  extend ActiveModel::Callbacks
  define_model_callbacks :validate

  before_validate :set_default_name

  validates_presence_of :name
  attr_accessor :name

  def set_default_name
    @name = 'neeraj'
  end
end

If you do

Developer.new.valid?
# exception 
#NoMethodError: undefined method `before_validate' for #<ActiveModel::Validations::PresenceValidator:0x104359c60>

Please note that in the above code if you remove the validates_presence_of :name code then Developer.new.valid? will not blow up.

The fix for above case is to understand how include ActiveModel::Validations works. When a class includes that module then a call similar to
define_model_callbacks :validate is already being made. All you need to do is to wire up the callbacks.

Here is the modified code that works.

class Developer
  include ActiveModel::Validations

  validate :before, :set_default_name

  validates_presence_of :name
  attr_accessor :name

  def set_default_name
    @name = 'neeraj'
  end
end

@Ben this takes care of the example that your provided with class C. I have not looked at example with A and B in great detail but do let me know if solution I provided does not work.

I am going to update the rails guide to reflect this style of usage ( given that it is right ). I will try to get sign off from a rails core team on this one. :-)

@neeraj thanks for the explanation. I updated the test case i wrote to use your syntax. Unfortunately it doesn't seem to solve the original problem. Here's the updated version of the test case:

    require "cases/helper"

    class ValidationCallbackTest < ActiveModel::TestCase

      class Person
        include ActiveModel::Validations

        attr_accessor :name

        def default_name
          @name = "John Smith"
        end
      end

      class PersonWithCallbackFirst < Person
        validate :before, :default_name
        validates_presence_of :name
      end

      class PersonWithCallbackLast < Person
        validates_presence_of :name
        validate :before, :default_name
      end

      test "should run callback before validation when callback defined before" do
        assert PersonWithCallbackFirst.new.valid?
      end

      test "should run callback before validation when callback defined after" do
        assert PersonWithCallbackLast.new.valid?
      end
    end

@Ben. Thanks for recreating the test cases.

And you are right that order of declaration matters. Test related to PersonWithCallbackFirst is passing. However test related to PersonWithCallbackLast is failing.

I will try to dig into the matter tomorrow.

@Ben : Can you try this patch. It works for me. Have not written any test etc. Just want to get a quick feel if it works for others. This is still work in progress.

Active Model executes all the callbacks in the order they are defined. And that is great. This ensures that in Active Record I can define three before_save callbacks and the second callback rely on the value set by first callback.

PersonWithCallbackLast class in Ben's example fails and ,I think, that is okay.

Neeraj: I disagree: I think it's terrible.

A callback defined to be run before validation should be run before validation, not after if that happens to be the order in which it was defined. Otherwise, what's the point of callbacks?

It's all very well saying "you just have to define the callbacks before the validations", but what if that's impossible? What if a parent class defines a validation and a child class defines a callback that needs to be run before it? I know it happens, because that's how I stumbled across this behaviour in the first place.

I agree that all the callbacks of the same type ought to be run in the order they were defined, but I can't think of any circumstance in which it would be sensible behaviour for a before_validate callback (however it's defined) to be called after validation. It just makes no sense. What if you defined an after_save callback before a before_save callback? Would you expect them to be run in that order?

I would expect all the after_save callbacks to run in the same order.

I did some more testing with my patch and it seems to be working. However right now I add every single before callback as the topmost callback which means all the last before callback would be the first one to run.

In order to fix it, I need to ensure that order of before callbacks are maintained and at the same time the later declared callbacks are inserted just before the validation callbacks. And that is possible.

However at this point of time I would like to get feedback from a core team member to ensure my sanity.

Take a look at http://github.com/neerajdotname/r_4653/blob/1afe74156456ceb54efd186... .

I moved before_validation from ActiveRecord to ActiveModel and now ActiveModel has before_validation http://github.com/neerajdotname/r_4653/blob/master/app/models/user.... .

Also now it does not matter whether before_validation is define before or after validates_presence_of.

With this change only three tests are failing in ActiveRecord which I can take a look at. No test is failing in ActiveModel.

I would say that before_validation and after_validation callbacks should be moved from ActiveRecord to ActiveModel.

I will dig more into it later. Just wanted to chime in what I found so far.

@Valim, so this would be the proper way to do it? (These test pass on master):

    require "cases/helper"

    class ValidationCallbackTest < ActiveModel::TestCase

      class Person
        include ActiveModel::Validations
        extend ActiveModel::Callbacks
        define_model_callbacks :validation

        attr_accessor :name

        def default_name
          @name = "John Smith"
        end

        def valid?
          _run_validation_callbacks { super }
        end
      end

      class PersonWithCallbackFirst < Person
        before_validation :default_name
        validates_presence_of :name
      end

      class PersonWithCallbackLast < Person
        validates_presence_of :name
        before_validation :default_name
      end

      test "should run callback before validation when callback defined before" do
        assert PersonWithCallbackFirst.new.valid?
      end

      test "should run callback before validation when callback defined after" do
        assert PersonWithCallbackLast.new.valid?
      end
    end

Yes, that would be one way to do it! Take a look at ActiveRecord::Callbacks for more examples!