Skip to content
This repository

ActiveRecord::Store does not work properly with a nullable db column. #4840

Closed
iHiD opened this Issue February 01, 2012 · 7 comments

3 participants

Jeremy Walker Aaron Patterson Rafael Mendonça França
Jeremy Walker

The following code works fine if there is a db column called 'properties' which is text and nullable:

class Tutorial
  store :properties, accessors: [:width, :height]
end
t = Tutorial.new
t.width = 100

However, it fails if the column is not null, with:

TypeError: can't convert Symbol into Integer
    from /Users/iHiD/.rvm/gems/ruby-1.9.3-p0/gems/activerecord-3.2.1/lib/active_record/store.rb:39:in `[]='

This can be reproduced in ActiveRecord tests by amending activerecord/test/schema/schema.rb:40 to

t.text :settings, :null => false, :default => ""

The issue is clearly that the property is being initialized to a string, rather than an empty hash.

I think the fix can be written into activerecord/lib/active_record/attribute_methods/serialization.rb in 'initialize_attributes'.

A quick, dirty hack of changing line 67 to the following fixes the problem, but breaks other tests.

current_value = attributes[key]
if coder.is_a?(Coders::YAMLColumn) && !current_value.is_a?(coder.object_class)
    current_value = coder.object_class.new
end
attributes[key] = Attribute.new(coder, current_value, :serialized)

I am happy to look into this further, but I figure someone who knows this chunk of code will understand the full initialization process much more fully and so may be able to come up with a more robust solution.

Aaron Patterson
Owner

It looks like store always assumes the serialized column is initialized. Rather than guaranteeing it is initialized, I think a well placed ||= {} in this code would fix the problem.

Jeremy Walker

That's a good place for the code. However, by that point store already been initialized to "" so ||={} won't change it.
I think we could check that it's a Hash, as line 32 specifies that datatype. How about a simple...

store_attribute = {} unless store_attribute.is_a?(Hash)

I'll put together a pull request if you're happy with that?

Aaron Patterson
Owner

Ya, that seems fine to me! As long as you add a test! ;-)

Jeremy Walker

Of course! ;)

Changing the test migration (as per my first post) causes the existing test to fail. Is that enough (with maybe a comment), or do you want two columns in the migration, each with specific tests?

The Rails tests don't seem to generally have that, but I'm happy to start a new precedent :-)

Aaron Patterson
Owner

If changing the column doesn't cause other tests to break, go ahead and do that. Otherwise, add a new column. TBH, I don't particularly care which route you go as long as there is a test. :-)

Rafael Mendonça França

I think that this can be closed

Jeremy Walker iHiD closed this February 02, 2012
Jeremy Walker

Yep. Thanks, @tenderlove!

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.