This project is archived and is in readonly mode.

#4653 ✓resolved
Matt Powell

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

Reported by Matt Powell | May 20th, 2010 @ 01:41 AM | in 3.0.2

Take a look at this gist.

Both specs should pass: the before_validate callback should always be run before the validators. Instead, what appears to be happening is that the order of the declaration matters: if I define the before_validate callback after the validation rule, it is run after the validation, which is bad news.

It doesn't seem to matter what type the validation is.

In summary: the first spec passes because the callback is hit first; the second fails, because the validation on #something is run before the callback. If you declare any validators before you call before_validate, your poor callback is left out in the cold.

I have tested this both with and without an ORM (Mongoid, though not ActiveRecord), within and without an application, and also by shifting the exceptions inside the actual validation methods, so that I could be sure it was the validation firing that was causing the error. In each case, the same error occurred. I started trying to look at stack traces and so on, but I wound up in the middle of generated code, and backed away slowly, trying not to blink.

I'd still love some independent verification, though.

Comments and changes to this ticket

  • Rizwan Reza

    Rizwan Reza May 21st, 2010 @ 12:43 AM

    • no changes were found...
  • Cyril Mougel

    Cyril Mougel May 27th, 2010 @ 10:41 PM

    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.

  • Matt Powell

    Matt Powell May 27th, 2010 @ 11:10 PM

    @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

  • Cyril Mougel

    Cyril Mougel May 28th, 2010 @ 11:35 AM

    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.

  • Ben Marini

    Ben Marini June 13th, 2010 @ 11:03 PM

    @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
    
  • Neeraj Singh

    Neeraj Singh June 14th, 2010 @ 03:23 AM

    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. :-)

  • Ben Marini

    Ben Marini June 14th, 2010 @ 04:06 AM

    @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
    
  • Neeraj Singh

    Neeraj Singh June 14th, 2010 @ 06:59 AM

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

  • Neeraj Singh

    Neeraj Singh June 14th, 2010 @ 11:58 PM

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

  • Neeraj Singh

    Neeraj Singh June 15th, 2010 @ 01:37 AM

    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.

  • Matt Powell

    Matt Powell June 15th, 2010 @ 01:48 AM

    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?

  • Neeraj Singh

    Neeraj Singh June 15th, 2010 @ 03:10 AM

    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.

  • Neeraj Singh

    Neeraj Singh June 15th, 2010 @ 03:17 AM

    • Assigned user set to “Pratik”

    Assigning it to Pratik to get his take on this issue.

  • José Valim

    José Valim June 15th, 2010 @ 02:14 PM

    • Milestone cleared.
    • State changed from “new” to “open”
    • Assigned user changed from “Pratik” to “José Valim”

    It's indeed a bug. Does anyone mind adding a small test case for ActiveModel test suite? I will fix it ASAP.

  • José Valim

    José Valim June 15th, 2010 @ 02:24 PM

    • State changed from “open” to “invalid”

    Sorry, I'm wrong. This is not a bug in Rails. ActiveModel already reserves the :validate callbacks for validations, so you should not use it. Take as example ActiveRecord, it uses before_validation to ensure it does not conflict with validate.

  • Neeraj Singh

    Neeraj Singh June 15th, 2010 @ 03:22 PM

    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.

  • Ben Marini

    Ben Marini June 15th, 2010 @ 04:30 PM

    @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
    
  • José Valim

    José Valim June 15th, 2010 @ 04:31 PM

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

  • Neeraj Singh

    Neeraj Singh June 16th, 2010 @ 04:58 PM

    • Tag changed from rails3 validations, activemodel, callbacks, rails3, validations to rails3 validations, activemodel, callbacks, patch, rails3, validations

    Attached is patch with test which moves the before_validation and after_validation logic from ActiveRecord to ActiveModel.

    Because of this change validating a plain class is as simple as this one.

    class Dog
      include ActiveModel::Validations
    
      attr_accessor :name
      validates_presence_of :name
    
      before_validation :set_default_name
    
      private
    
      def set_default_name
        @name = 'super_cool_dog'
      end
    
    end
    
    Dog.new.valid?
    

    Without the patch one needs to do a lot of heavy lifting to get validation + before_validation callback working. http://gist.github.com/440882

    Thanks to José for providing most of code and all of guidance :-)

  • José Valim

    José Valim June 18th, 2010 @ 01:37 PM

    • State changed from “invalid” to “open”
  • Repository

    Repository June 19th, 2010 @ 11:19 PM

    • State changed from “open” to “resolved”

    (from [51739d3228d12907d60fb1b0a2b1ef96c55f66a3]) moving before_validation and after_validation functionality from ActiveRecord to ActiveModel

    [#4653 state:resolved]

    Signed-off-by: José Valim jose.valim@gmail.com
    http://github.com/rails/rails/commit/51739d3228d12907d60fb1b0a2b1ef...

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “High”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>