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

ActiveModel: adding validations dynamically fails #5449

Closed
bjoernbur opened this Issue Mar 15, 2012 · 15 comments

Comments

Projects
None yet
10 participants
@bjoernbur

bjoernbur commented Mar 15, 2012

I'm experiencing a strange bug using ActiveModel::Validations. When I add validations to a specific instance using class_eval. It works on the first instance but fails on the second. Following a small setup to illustrate the problem:

require 'active_model'

class Example
  include ActiveModel::Validations

  attr_accessor :name, :email
  attr_accessor :something, :else
end

e = Example.new
e.singleton_class.validates_presence_of :name, :email

e2 = Example.new
e2.singleton_class.validates_presence_of :something, :else

e.valid? # => false
e2.valid?

this results in:

/Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activesupport-3.2.2/lib/active_support/callbacks.rb:407:in `_run__3658204812670346400__validate__2880682970808709992__callbacks': undefined local variable or method `_callback_before_1' for #<Example:0x0000010292b7a8> (NameError)
    from /Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activesupport-3.2.2/lib/active_support/callbacks.rb:405:in `__run_callback'
    from /Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activesupport-3.2.2/lib/active_support/callbacks.rb:385:in `_run_validate_callbacks'
    from /Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activesupport-3.2.2/lib/active_support/callbacks.rb:81:in `run_callbacks'
    from /Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activemodel-3.2.2/lib/active_model/validations.rb:212:in `run_validations!'
    from /Users/bbu0103/.rvm/gems/ruby-1.9.2-p290@cookpit/gems/activemodel-3.2.2/lib/active_model/validations.rb:179:in `valid?'
    from validation_test.rb:17:in `<main>'

I used rails 3.1.x before and this worked as expected. I guess the problem was introduced in rails 3.2

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward Mar 15, 2012

Contributor

this is flawed code. 'e' and 'e2' are instances, not classes. you should be running 'class_eval' on 'Example'.

Contributor

ahoward commented Mar 15, 2012

this is flawed code. 'e' and 'e2' are instances, not classes. you should be running 'class_eval' on 'Example'.

@bjoernbur

This comment has been minimized.

Show comment
Hide comment
@bjoernbur

bjoernbur Mar 15, 2012

@ahoward I've reconstructed the example above. I called it explicitly on an instance to make the validation specific to a given instance. I can't add it to the Example Class otherwise all instances have the same validations...

bjoernbur commented Mar 15, 2012

@ahoward I've reconstructed the example above. I called it explicitly on an instance to make the validation specific to a given instance. I can't add it to the Example Class otherwise all instances have the same validations...

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward Mar 15, 2012

Contributor

an object's singleton_class doesn't magically inherit stuff from it's class - aka the singleton_class isn't prepared here on the basis of the previous

  include ActiveModel::Validations

that code never should have worked, and it's broken.

this will work as designed:

require 'rubygems'
require 'active_model'

class Example
  include ActiveModel::Validations

  attr_accessor :name, :email
  attr_accessor :something, :else

  def Example.dup
    super.tap{|dup| def dup.name() Example.name end }
  end
end

e = Example.dup.new 
e.class.class_eval do
  validates_presence_of :name, :email
end

p e.valid?
p e.errors


e2 = Example.dup.new 
e2.class.class_eval do
  validates_presence_of :name, :email
end

p e2.valid?
p e2.errors

__END__
false
#<ActiveModel::Errors:0x007f86c91ffd40 @base=#<#<Class:0x007f86c9201d70>:0x007f86c9201960 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f86c91ffd40 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>
#
false
#<ActiveModel::Errors:0x007f86c90d6e78 @base=#<#<Class:0x007f86c90d8f70>:0x007f86c90d8b88 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f86c90d6e78 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>
Contributor

ahoward commented Mar 15, 2012

an object's singleton_class doesn't magically inherit stuff from it's class - aka the singleton_class isn't prepared here on the basis of the previous

  include ActiveModel::Validations

that code never should have worked, and it's broken.

this will work as designed:

require 'rubygems'
require 'active_model'

class Example
  include ActiveModel::Validations

  attr_accessor :name, :email
  attr_accessor :something, :else

  def Example.dup
    super.tap{|dup| def dup.name() Example.name end }
  end
end

e = Example.dup.new 
e.class.class_eval do
  validates_presence_of :name, :email
end

p e.valid?
p e.errors


e2 = Example.dup.new 
e2.class.class_eval do
  validates_presence_of :name, :email
end

p e2.valid?
p e2.errors

__END__
false
#<ActiveModel::Errors:0x007f86c91ffd40 @base=#<#<Class:0x007f86c9201d70>:0x007f86c9201960 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f86c91ffd40 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>
#
false
#<ActiveModel::Errors:0x007f86c90d6e78 @base=#<#<Class:0x007f86c90d8f70>:0x007f86c90d8b88 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f86c90d6e78 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>
@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward Mar 15, 2012

Contributor

this permutation is probably what you want. in any case, this really is a flawed usage that never should have worked.

require 'rubygems'
require 'active_model'

class Example
  include ActiveModel::Validations

  attr_accessor :name, :email
  attr_accessor :something, :else

  def Example.dup(&block)
    super.tap do |dup|
      def dup.name() Example.name end
      dup.class_eval(&block) if block
    end
  end

  def Example.ish(*args, &block)
    dup(&block).new(*args)
  end

  def initialize(attributes = {})
    attributes.each{|k,v| send("#{ k }=", v)}
  end
end

e = Example.ish{ validates_presence_of :name, :email }
p e.valid?
p e.errors


e2 = Example.ish(:email => 'ara@dojo4.com'){ validates_presence_of :name, :email }
p e2.valid?
p e2.errors

__END__
false
#<ActiveModel::Errors:0x007f8ff4a21558 @base=#<#<Class:0x007f8ff4a15f50>:0x007f8ff4a215d0 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f8ff4a21558 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>

false
#<ActiveModel::Errors:0x007f8ff48ff238 @base=#<#<Class:0x007f8ff49014c0>:0x007f8ff48ff378 @email="ara@dojo4.com", @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f8ff48ff238 ...>>, @messages={:name=>["can't be blank"]}>
Contributor

ahoward commented Mar 15, 2012

this permutation is probably what you want. in any case, this really is a flawed usage that never should have worked.

require 'rubygems'
require 'active_model'

class Example
  include ActiveModel::Validations

  attr_accessor :name, :email
  attr_accessor :something, :else

  def Example.dup(&block)
    super.tap do |dup|
      def dup.name() Example.name end
      dup.class_eval(&block) if block
    end
  end

  def Example.ish(*args, &block)
    dup(&block).new(*args)
  end

  def initialize(attributes = {})
    attributes.each{|k,v| send("#{ k }=", v)}
  end
end

e = Example.ish{ validates_presence_of :name, :email }
p e.valid?
p e.errors


e2 = Example.ish(:email => 'ara@dojo4.com'){ validates_presence_of :name, :email }
p e2.valid?
p e2.errors

__END__
false
#<ActiveModel::Errors:0x007f8ff4a21558 @base=#<#<Class:0x007f8ff4a15f50>:0x007f8ff4a215d0 @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f8ff4a21558 ...>>, @messages={:name=>["can't be blank"], :email=>["can't be blank"]}>

false
#<ActiveModel::Errors:0x007f8ff48ff238 @base=#<#<Class:0x007f8ff49014c0>:0x007f8ff48ff378 @email="ara@dojo4.com", @validation_context=nil, @errors=#<ActiveModel::Errors:0x007f8ff48ff238 ...>>, @messages={:name=>["can't be blank"]}>
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 15, 2012

Member

@ahoward why do you think the code is flawed? I mean isn't the singleton class for exactly that type of use? putting stuff on a single instance that would otherwise end up in every instance? For example using attr_accessor it's pretty trivially to add a new field to just one instance. Would that be "flawed" too?

an object's singleton_class doesn't magically inherit stuff from it's class

Isn't this the same situation you say should not work or is flawed:

class Example
  def self.say_hello
    'hello'
  end
end
e = Example.new
e.singleton_class < Example # => true
e.singleton_class.say_hello # => hello

I'm really trying to understand why it should not work the way it was described in the ticket. I mean I understand that ActiveModel::Validations could be implemented in such a manner that it no longer works but from a ruby perspective where are the flaws?

Member

senny commented Mar 15, 2012

@ahoward why do you think the code is flawed? I mean isn't the singleton class for exactly that type of use? putting stuff on a single instance that would otherwise end up in every instance? For example using attr_accessor it's pretty trivially to add a new field to just one instance. Would that be "flawed" too?

an object's singleton_class doesn't magically inherit stuff from it's class

Isn't this the same situation you say should not work or is flawed:

class Example
  def self.say_hello
    'hello'
  end
end
e = Example.new
e.singleton_class < Example # => true
e.singleton_class.say_hello # => hello

I'm really trying to understand why it should not work the way it was described in the ticket. I mean I understand that ActiveModel::Validations could be implemented in such a manner that it no longer works but from a ruby perspective where are the flaws?

@ahoward

This comment has been minimized.

Show comment
Hide comment
@ahoward

ahoward Mar 15, 2012

Contributor

i'm not saying such an implementation is impossible but the assumption you are making, that method inheritance implies state inheritance is not right thinking

class Example
  class << Example
    attr :callbacks
  end

  @callbacks = [:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]
end


p Example.callbacks
p Example.new.singleton_class.callbacks

__END__

[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]

nil

it may. or it may not. it depends on the intention of the author and the code. one things is clear though: it is not ruby's intention that module state is magic someone based on an 'include' statement. it only gives guarantees about method inheritance.

in your case you are assuming that something clever will happen here wrt to state - probably you assuming every will be cloned and can then be appended too. but this requires dedicated code such as

class Example
  class << Example
    def callbacks
      unless defined?(@callbacks)
        @callbacks =
          ancestors.map{|a| a.callbacks if a.respond_to?(:callbacks)}.flatten.compact
      end

      @callbacks
    end
  end

  @callbacks = [:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]
end

a = Example
b = Example.new.singleton_class

p :a => a.callbacks
p :b => b.callbacks

b.callbacks.push(:what_happens_now?)

p :a => a.callbacks
p :b => b.callbacks

a.callbacks.push(:and_how_about_this?)

p :a => a.callbacks
p :b => b.callbacks

__END__

{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :what_happens_now?]}
{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :and_how_about_this?]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :what_happens_now?]}

so, to understand whether or not you should be able to do this you'd need to read the docs and test carefully. expecting it to just work though, isn't reasonable. in fact, you can see exactly the intention here:

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

that is to say it's a concern of ActiveModel to muck with inherited state. not the Validation module.

the alternative to manual copying is dynamic computation like

https://github.com/ahoward/dao/blob/master/lib/dao/validations/validator.rb#L166

summary: your usage is possible. but not directly supported or intentioned by the code.

Contributor

ahoward commented Mar 15, 2012

i'm not saying such an implementation is impossible but the assumption you are making, that method inheritance implies state inheritance is not right thinking

class Example
  class << Example
    attr :callbacks
  end

  @callbacks = [:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]
end


p Example.callbacks
p Example.new.singleton_class.callbacks

__END__

[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]

nil

it may. or it may not. it depends on the intention of the author and the code. one things is clear though: it is not ruby's intention that module state is magic someone based on an 'include' statement. it only gives guarantees about method inheritance.

in your case you are assuming that something clever will happen here wrt to state - probably you assuming every will be cloned and can then be appended too. but this requires dedicated code such as

class Example
  class << Example
    def callbacks
      unless defined?(@callbacks)
        @callbacks =
          ancestors.map{|a| a.callbacks if a.respond_to?(:callbacks)}.flatten.compact
      end

      @callbacks
    end
  end

  @callbacks = [:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]
end

a = Example
b = Example.new.singleton_class

p :a => a.callbacks
p :b => b.callbacks

b.callbacks.push(:what_happens_now?)

p :a => a.callbacks
p :b => b.callbacks

a.callbacks.push(:and_how_about_this?)

p :a => a.callbacks
p :b => b.callbacks

__END__

{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :what_happens_now?]}
{:a=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :and_how_about_this?]}
{:b=>[:method_inheritence_is_trivial, :state_inheritence_should_not_be_assumed, :what_happens_now?]}

so, to understand whether or not you should be able to do this you'd need to read the docs and test carefully. expecting it to just work though, isn't reasonable. in fact, you can see exactly the intention here:

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

that is to say it's a concern of ActiveModel to muck with inherited state. not the Validation module.

the alternative to manual copying is dynamic computation like

https://github.com/ahoward/dao/blob/master/lib/dao/validations/validator.rb#L166

summary: your usage is possible. but not directly supported or intentioned by the code.

@mcmire

This comment has been minimized.

Show comment
Hide comment
@mcmire

mcmire Mar 15, 2012

Contributor

I think what @ahoward is saying that is that instance variables defined on a class instance do not get inherited by the singleton class of a instance of that class, therefore the validators that are defined on a class when you include ActiveModel::Validations (because they are assigned to a class instance variable) will not be accessible by a singleton class of an instance of the model class. So you'd have to say something like:

c1 = Class.new(Example) do
  validates_presence_of :name, :email
end
e1 = c1.new

c2 = Class.new(Example) do
  validates_presence_of :something, :else
end
e2 = c2.new

e1.valid?
e2.valid?

The alternative (I guess) would be to define validators as class variables, but we all know that's not really an option anymore.

Contributor

mcmire commented Mar 15, 2012

I think what @ahoward is saying that is that instance variables defined on a class instance do not get inherited by the singleton class of a instance of that class, therefore the validators that are defined on a class when you include ActiveModel::Validations (because they are assigned to a class instance variable) will not be accessible by a singleton class of an instance of the model class. So you'd have to say something like:

c1 = Class.new(Example) do
  validates_presence_of :name, :email
end
e1 = c1.new

c2 = Class.new(Example) do
  validates_presence_of :something, :else
end
e2 = c2.new

e1.valid?
e2.valid?

The alternative (I guess) would be to define validators as class variables, but we all know that's not really an option anymore.

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus Mar 15, 2012

Member

I'm closing this because this is not a rails bug. It looks like inheritance would be better in this situation.

Member

drogus commented Mar 15, 2012

I'm closing this because this is not a rails bug. It looks like inheritance would be better in this situation.

@greyblake

This comment has been minimized.

Show comment
Hide comment
@greyblake

greyblake Dec 28, 2012

Contributor

Inheritance is not the solution for all cases.

Contributor

greyblake commented Dec 28, 2012

Inheritance is not the solution for all cases.

@jesseclark

This comment has been minimized.

Show comment
Hide comment
@jesseclark

jesseclark Feb 28, 2013

I am developing a system that contains multiple questionnaires which are created via an admin interface. The admin needs to be able to define a new property on the model that stores the end user's answers in addition to defining validations that need to be run by that instances of that model.

I've solved the defining a new property bit via Mongoid's allow_dynamic_fields and overriding method_missing to determine whether the incoming property value is within the admin defined set of properties and either setting the value or calling super.

In order to handle dynamic validations for the different questionnaires, it would be extremely helpful to be able to set validations at the instance level. I was familiar with the approach of setting the validations on the singleton_class which worked up till Rails 3.x and had been planning on using that method but now that seems to not be an option.

So, my question is: Is there any way to set validations on instances of models in Rails? And if so, how do I go about doing so?

Thanks.

jesseclark commented Feb 28, 2013

I am developing a system that contains multiple questionnaires which are created via an admin interface. The admin needs to be able to define a new property on the model that stores the end user's answers in addition to defining validations that need to be run by that instances of that model.

I've solved the defining a new property bit via Mongoid's allow_dynamic_fields and overriding method_missing to determine whether the incoming property value is within the admin defined set of properties and either setting the value or calling super.

In order to handle dynamic validations for the different questionnaires, it would be extremely helpful to be able to set validations at the instance level. I was familiar with the approach of setting the validations on the singleton_class which worked up till Rails 3.x and had been planning on using that method but now that seems to not be an option.

So, my question is: Is there any way to set validations on instances of models in Rails? And if so, how do I go about doing so?

Thanks.

@haggen

This comment has been minimized.

Show comment
Hide comment
@haggen

haggen Apr 11, 2013

@jesseclark I am having the same issue as you. A form's fields and validations are defined and stored in database, then I try to build up a model for that form. Have you found a solution ? I'm almost giving up to develop my own validation module. :(

Any help is appreciated! Thanks.

haggen commented Apr 11, 2013

@jesseclark I am having the same issue as you. A form's fields and validations are defined and stored in database, then I try to build up a model for that form. Have you found a solution ? I'm almost giving up to develop my own validation module. :(

Any help is appreciated! Thanks.

@haggen

This comment has been minimized.

Show comment
Hide comment
@haggen

haggen Apr 11, 2013

I got a workaround using #dup + #ish that @ahoward described above. But I find it quite smelly and should watch closely its performance and memory consumption.

haggen commented Apr 11, 2013

I got a workaround using #dup + #ish that @ahoward described above. But I find it quite smelly and should watch closely its performance and memory consumption.

@andreimoment

This comment has been minimized.

Show comment
Hide comment
@andreimoment

andreimoment Apr 21, 2015

Did anyone implementing the solution mentioned by @ahoward experience memory issues as pointed out by @haggen ? What would be a good way to measure this?

andreimoment commented Apr 21, 2015

Did anyone implementing the solution mentioned by @ahoward experience memory issues as pointed out by @haggen ? What would be a good way to measure this?

@Pithikos

This comment has been minimized.

Show comment
Hide comment
@Pithikos

Pithikos Sep 1, 2016

After countless hours fiddling with this, here's an example that actually works.

class MyModel
  include ActiveModel::Validations
  attr_accessor :attr1

  validate :instance_validations

  def initialize(use_instance_validation=false)
    @use_validation = use_instance_validation
  end

  def instance_validations
    validates_presence_of :attr1 if @use_validation
  end

end

m = MyModel.new(true)
ap m.valid? # false
m = MyModel.new
ap m.valid? # true

The trick is that we use a custom validator instance_validations which acts differently depending on the instance variable @use_validation.

Pithikos commented Sep 1, 2016

After countless hours fiddling with this, here's an example that actually works.

class MyModel
  include ActiveModel::Validations
  attr_accessor :attr1

  validate :instance_validations

  def initialize(use_instance_validation=false)
    @use_validation = use_instance_validation
  end

  def instance_validations
    validates_presence_of :attr1 if @use_validation
  end

end

m = MyModel.new(true)
ap m.valid? # false
m = MyModel.new
ap m.valid? # true

The trick is that we use a custom validator instance_validations which acts differently depending on the instance variable @use_validation.

@Pithikos

This comment has been minimized.

Show comment
Hide comment
@Pithikos

Pithikos Sep 1, 2016

And an other version of the same thing where we have everything in the initializer:

class MyModel
  include ActiveModel::Validations
  attr_accessor :attr1

  validate :instance_validations

  def initialize(use_instance_validation=false)
    define_singleton_method(:instance_validations) do
      if use_instance_validation
        validates_presence_of :attr1
      end
    end
  end

end

Tested on Rails 3 and Rails 4

Pithikos commented Sep 1, 2016

And an other version of the same thing where we have everything in the initializer:

class MyModel
  include ActiveModel::Validations
  attr_accessor :attr1

  validate :instance_validations

  def initialize(use_instance_validation=false)
    define_singleton_method(:instance_validations) do
      if use_instance_validation
        validates_presence_of :attr1
      end
    end
  end

end

Tested on Rails 3 and Rails 4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment