Skip to content
This repository

Evil cache bug with composed_of #2084

Closed
bradphelan opened this Issue July 15, 2011 · 5 comments

4 participants

Brad Phelan Steve Klabnik Rafael Mendonça França Jon Kessler
Brad Phelan

The method

----------------------
activerecord-3.1.0.rc4/lib/active_record/aggregations.rb
----------------------
235         def writer_method(name, class_name, mapping, allow_nil, converter)
236           define_method("#{name}=") do |part|
237             if part.nil? && allow_nil
238               mapping.each { |pair| self[pair.first] = nil }
239               @aggregation_cache[name] = nil
240             else
241               unless part.is_a?(class_name.constantize) || converter.nil?
242                 part = converter.respond_to?(:call) ?
243                   converter.call(part) :
244                   class_name.constantize.send(converter, part)
245               end
246  
247               mapping.each { |pair| self[pair.first] = part.send(pair.last) }
248               @aggregation_cache[name] = part.freeze
249             end
250           end
251         end

is broken at line 247 due to caching. The problem is that rails does type conversions on activerecord fields. So for example if I have an AR class Foo with a field bar declared as an integer in the db then

foo = new Foo
foo.bar = "10.0"
puts foo.bar.class

=> Integer

However composed_of assumes that the there is no conversion during setting the object. Thus the aggregate objects will be returned with a string field rather than an integer field because the converted object was not read back and put in the cache.
The solution is to add an extra line in the above method that reads back the attribute after writing and caches the converted value and not the original value.

Something like


        def writer_method(name, class_name, mapping, allow_nil, converter)
          define_method("#{name}=") do |part|
            if part.nil? && allow_nil
              mapping.each { |pair| self[pair.first] = nil }
              @aggregation_cache[name] = nil
            else
              unless part.is_a?(class_name.constantize) || converter.nil?
                part = converter.respond_to?(:call) ?
                  converter.call(part) :
                  class_name.constantize.send(converter, part)
              end

              mapping.each { |pair| 
                  self[pair.first] = part.send(pair.last)
                  # Cache the converted attribute in the aggregate part
                  part.send("#{pair.last}=", self[pair.first]
               }
              @aggregation_cache[name] = part.freeze
            end
          end
        end
Jon Kessler

@bradphelan Is this still an issue, or can it be closed?

Brad Phelan

Has the suggested fix been made in the code. I don't use composed of anymore. I doubt it has been fixed by some other vector if the change I suggested has not been made.

Steve Klabnik
Collaborator

Since @bradphelan isn't sure if this is still a problem, I'm closing. If anyone wants to produce a test case and/or a patch, I will gladly re-open.

Steve Klabnik steveklabnik closed this April 28, 2012
Rafael Mendonça França rafaelfranca reopened this April 29, 2012
Rafael Mendonça França
Owner

Reopening since we got a patch. So it is still an issue

Steve Klabnik
Collaborator

If #6743 gets merged, this bug will go away.

Steve Klabnik steveklabnik referenced this issue from a commit June 18, 2012
Commit has since been removed from the repository and is no longer available.
Steve Klabnik steveklabnik referenced this issue from a commit June 18, 2012
Commit has since been removed from the repository and is no longer available.
Steve Klabnik steveklabnik closed this issue from a commit June 15, 2012
Steve Klabnik Removing composed_of from ActiveRecord.
This feature adds a lot of complication to ActiveRecord for dubious
value. Let's talk about what it does currently:

class Customer < ActiveRecord::Base
  composed_of :balance, :class_name => "Money", :mapping => %w(balance amount)
end

Instead, you can do something like this:

    def balance
      @balance ||= Money.new(value, currency)
    end

    def balance=(balance)
      self[:value] = balance.value
      self[:currency] = balance.currency
      @balance = balance
    end

Since that's fairly easy code to write, and doesn't need anything
extra from the framework, if you use composed_of today, you'll
have to add accessors/mutators like that.

Closes #1436
Closes #2084
Closes #3807
14fc8b3
Steve Klabnik steveklabnik closed this in 14fc8b3 June 18, 2012
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.