Skip to content
This repository

composed_of freezing prevents ActiveModel::Validations from working #1513

Closed
bradphelan opened this Issue June 06, 2011 · 19 comments
Brad Phelan

composed_of objects should be able to take advantage of ActiveModel::Validations. However composed_of uses value
semantics and freezes the objects.

The solution is to ensure that ActiveModel::Validations use variables with "reference" semantics so that if the object is frozen you can still add errors to the validation. The following monkey patch works for me and doesn't add any performance overhead or space overhead for creating Errors objects if they are not needed.


module ActiveModel
  module Validations

    def self.included(base)
      super base
      base.class_eval do
        def errors
          @errors ||= []
          @errors[0] ||= ActiveModel::Errors.new(self) 
          @errors[0]
        end

        def validation_context=(val)
          @validation_context ||= []
          @validation_context[0] = val
        end

        def validation_context
          @validation_context ||= []
          @validation_context[0]
        end

        def freeze
          @errors ||= []
          @validation_context ||= []
          super
        end
      end
    end
  end

end


module MySugr
  # This module is mainly a hack to get around a bug with composed_of freezing objects
  # and ActiveRecord::Validations requiring unfrozen objects
  module Composable

    def self.included(base)
      base.send :include, ActiveModel::Validations
    end

    def new_record?
      true
    end

  end

end

and is used like

class BolusPumpSetting
    include MySugr::Composable

    attr_reader :normal_units, :square_units, :square_duration

    def initialize(normal_units, square_units, square_duration)
      @normal_units = normal_units
      @square_units = square_units
      @square_duration = square_duration
    end

    validate do |record|
      if record.square_units.nil? ^ record.square_duration.nil?
        record.errors.add :base, "Must set both square_units and square duration"
      end
    end

end
class Log < ActiveRecord::Base

  composed_of :bolus_pump_setting,
    :allow_nil => true,
    :mapping => [
      %w[bolus_pump_setting_normal_units normal_units],
      %w[bolus_pump_setting_square_units square_units],
      %w[bolus_pump_setting_square_duration square_duration]
    ]
  validates_associated :bolus_pump_setting

end
lichtamberg

Yeah that would be cool... Does someone know if this becomes applied to the master branch?

Eric Schwartz

This is still causing us problems... it would be helpful if someone could look at it.

Steve Klabnik
Collaborator

While I don't have the authority to pull in code, these things, in order of awesome, help move fixes along:

  1. A report (✓)
  2. A failing test
  3. A failing test + code that makes it pass.
Brad Phelan

Responding to bug reports in order of awesome

(1) thank-you
(2) do you please have time to add more information?
(3) do you please have time to add a test case?
(4) I see you have provided a patch, thanks! However could you please fork and then provide a pull request as it would make things for the core devs easier.
(5) if you don't have time thanks anyway for your contribution.

Just sayin!

Steve Klabnik
Collaborator

Word. I was mostly trying to point @emschwar in the right direction. Regardless of that, thanks for your report, absolutely.

Mark McSpadden markmcspadden referenced this issue from a commit in markmcspadden/rails May 18, 2012
Mark McSpadden Failing test for issue #1513 eb7eb0a
Mark McSpadden

Created two failing tests and then implemented the initial proposed solution: https://github.com/markmcspadden/rails/compare/issue_1513

Didn't seem to solve the issue, but it's there if someone else wants to pick it up and give it some more attention.

Angelo Capilleri

@rafaelfranca this is considered opened?

Rafael Mendonça França
Owner

yes

Angelo Capilleri

If the object is frozen this means that it should not change. I don't understand why you try to change the errors instance in your test @markmcspadden.
Maybe everything could be solved forsing the validation process when freeze is called.
cc / @rafaelfranca

Mark McSpadden

@acapilleri - Looking back, it's probably a misunderstanding of the problem on my part.

Rafael Mendonça França
Owner

The problem is that @bradphelan want to use ActiveModel::Validation with composed_of, and composed_of freeze the object after the initialization. So you can't add errors, or change any value on it.

Solve this problem can be difficult and I personally don't think that it will be worth.

I'll leave this issue open but I'll not work on it. If someone want to provide a pull request without monkey patches to change a frozen object I'll be glad to review.

Angelo Capilleri

actually a test like this fails :

class Book < Hash
  include ActiveModel::Validations
  attr_accessor :title
end

def test_validate_no_validated_frozen_object 
    Book.validates_presence_of :title
    book = Book.new
    book.title = "Litterature"
    assert book.valid?

    book_freeze = Book.new
    book_freeze.title = "Litterature"
    book_freeze.freeze
    assert book_freeze.valid?

    book = Book.new
    assert book.invalid?

    book_freeze2 = Book.new
    book_freeze2.freeze
    assert book_freeze2.invalid?
end
Mark McSpadden

@acapilleri - Yeah that's a much better test of the behavior. Nice.

Angelo Capilleri

When an object is freezed you don't know if is valid or not.This happens because the validation method try to change some stuff inside an instance.This behaviour is not consistent because an object is valid and invalid also after his freezing. To solve this, I forced to call valid? method when the object if freezed considering that if an obejct if valid/invalid before freezing it will be also when is frozen. With the following changes:

# Valid the object when freeze is called so the object
def freeze
  self.valid?
  super
end

# Runs all the specified validations and returns true if no errors were added
# otherwise false. Context can optionally be supplied to define which callbacks
# to test against (the context is defined on the validations using :on).
def valid?(context = nil)
  unless self.frozen?
     current_context, self.validation_context = validation_context, context
     errors.clear
     @valid = run_validations!
   else
     @valid
   end
end

the test above not fails but activerecord https://github.com/rails/rails/blob/master/activerecord/test/cases/transactions_test.rb#L365 fails.
I want add an exception like FrozenRecord when active record try to save frozen object, because maybe at the moment it fails because the validation can't proceed.
Any suggestion?
cc / @carlosantoniodasilva

Carlos Antonio da Silva

@acapilleri I don't think there's a need to add an exception like that to AR, because AR does not suffer from this problem when calling valid?. The difference is that when you call freeze in an AR object, it actually freezes the @attributes hash, and not the AR object itself, so running valid? just works.

Now about the issue with ActiveModel, I kinda agree that we can't handle all the cases where someone tries to change a frozen object. In case of a model using AMo::Validations, I get this:

>> class Foo; include ActiveModel::Validations; end
=> Foo
>> f = Foo.new
=> #<Foo:0x007f81447457a8>
>> f.validation_context
=> nil
>> f.freeze
=> #<Foo:0x007f81447457a8>
>> f.valid?
RuntimeError: can't modify frozen Foo
    from ./activemodel-3.2.5/lib/active_model/validations.rb:195:in `ensure in valid?'

which means that the problem happens when self.validation_context= is executed.

In any case, I think that forcing validations to happen without the user expecting it may not be the best in some scenarios when calling freeze. It should just return the same result as prior to calling freeze, ie just errors.empty?, without running validations.. Still unsure.. @rafaelfranca thoughts?

Angelo Capilleri

Sorry, I forgot a line of code :

# Runs all the specified validations and returns true if no errors were added
# otherwise false. Context can optionally be supplied to define which callbacks
# to test against (the context is defined on the validations using :on).
def valid?(context = nil)
  unless self.frozen?
     current_context, self.validation_context = validation_context, context
     errors.clear
     @valid = run_validations!
   else
     @valid
   end
ensure
  self.validation_context = current_context unless self.frozen?
end
Mark McSpadden

Looks like #6743 would "solve" the composed_of part of this discussion.

I think the AM considerations raise here are still worth keeping it open for.

Angelo Capilleri

maybe We should open an issue about validation of freeze object that includes ActiveModel::Validation

Rafael Mendonça França rafaelfranca closed this June 18, 2012
Rafael Mendonça França
Owner

Closed by #6743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.