Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• 

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

  • no changes were found...

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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
  

  • validation-with-callback-test.patch 1.5 KB
• 

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

• You flagged this item as spam.
  

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

  • 4653_work_in_progress.diff 1.1 KB
• 

  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.

• You flagged this item as spam.
  

  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 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 June 15th, 2010 @ 03:17 AM

  • Assigned user set to “Pratik”

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

• 

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

• You flagged this item as spam.
  

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

  • 4653_fix_against_master.diff 10.3 KB
• 

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

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

  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 October 15th, 2010 @ 11:01 PM

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

  [bulk edit]