Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix: ActiveRecord::Store TypeError conversion when using YAML coder #13593

Merged
merged 1 commit into from Jan 6, 2014

Conversation

Projects
None yet
3 participants
Contributor

oliveiraethales commented Jan 4, 2014

As on PR #13575, after GitHub trolling us hehe (: cc @senny

Closes #13570

@senny senny and 1 other commented on an outdated diff Jan 5, 2014

activerecord/test/cases/store_test.rb
@@ -162,4 +162,8 @@ class StoreTest < ActiveRecord::TestCase
assert_equal [:color], first_model.stored_attributes[:data]
assert_equal [:width, :height], second_model.stored_attributes[:data]
end
+
+ test "using YAML coder with a store must not return nil" do
+ assert_not_nil @john.params
@senny

senny Jan 5, 2014

Member

can't we assert on what it should return?

@oliveiraethales

oliveiraethales Jan 5, 2014

Contributor

Well observed, will do (:

Contributor

oliveiraethales commented Jan 5, 2014

@senny There it is! (:

@senny senny and 1 other commented on an outdated diff Jan 6, 2014

activerecord/test/cases/store_test.rb
@@ -162,4 +162,11 @@ class StoreTest < ActiveRecord::TestCase
assert_equal [:color], first_model.stored_attributes[:data]
assert_equal [:width, :height], second_model.stored_attributes[:data]
end
+
+ test "correctly store YAML coder" do
+ assert_equal Hash.new, @john.params
+
@senny

senny Jan 6, 2014

Member

can we also add an assertion before the assignment? This will illustrate the initial state:

assert_equal ..., @john.params
@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

I thought that this exact line was doing that (assert_equal Hash.new, @john.params)!
Could you explain? Sorry for not getting it yet hehe

@senny senny and 1 other commented on an outdated diff Jan 6, 2014

activerecord/test/cases/store_test.rb
@@ -162,4 +162,11 @@ class StoreTest < ActiveRecord::TestCase
assert_equal [:color], first_model.stored_attributes[:data]
assert_equal [:width, :height], second_model.stored_attributes[:data]
end
+
+ test "correctly store YAML coder" do
@senny

senny Jan 6, 2014

Member

just a nitpick but we don't actually store the coder. The coder is used to serialize the data. So maybe we can reword this to:

store works with YAML coder
@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

Hmmm, nice! Thanks for the clarification man :D Will do it!

@senny

senny Jan 6, 2014

Member

if we are only going to verify that it works with nil without the saves, we should name the test accordingly:

YAML coder initializes the store when a Nil value is given
@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

Done it! <3

@senny senny and 1 other commented on an outdated diff Jan 6, 2014

activerecord/test/cases/store_test.rb
@@ -162,4 +162,11 @@ class StoreTest < ActiveRecord::TestCase
assert_equal [:color], first_model.stored_attributes[:data]
assert_equal [:width, :height], second_model.stored_attributes[:data]
end
+
+ test "correctly store YAML coder" do
+ assert_equal Hash.new, @john.params
+
+ @john.params['color'] = 'blue'
+ assert_equal @john.params['color'], 'blue'
@senny

senny Jan 6, 2014

Member

as there were no other YAML tests we should also make a DB roundtrip and verify that deserialization using YAML works as expected. Maybe even save twice.

1.) with blank state
2.) with the color assignment

@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

I see! But isn't the default behaviour (that is, when no coder is specified) to use YAML coder?

As on lines 169~173 of store.rb:

if coder_or_class_name.respond_to?(:load) && coder_or_class_name.respond_to?(:dump)
coder_or_class_name
else
ActiveRecord::Coders::YAMLColumn.new(coder_or_class_name || Object)
end

Please correct me if I'm wrong (:

@senny

senny Jan 6, 2014

Member

that's true. Does this problem only happen when you specify the coder explicitly (eg: coder: YAML) or also when you don't specify a coder at all? If it happens always there is no need to define a new store called params we could simply use one of the existing ones (like settings), no?

@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

Yeah, I considered using settings too (: But with some local tests, I've found that the error only happened when the YAML coder was specified manually, as in the original issue. So I added the new store 'params'. Is there a better alternative?

@senny senny and 1 other commented on an outdated diff Jan 6, 2014

activerecord/test/cases/store_test.rb
@@ -162,4 +162,8 @@ class StoreTest < ActiveRecord::TestCase
assert_equal [:color], first_model.stored_attributes[:data]
assert_equal [:width, :height], second_model.stored_attributes[:data]
end
+
+ test "YAML coder initializes the store when a Nil value is given" do
+ assert_equal Hash.new, @john.params
@senny

senny Jan 6, 2014

Member

Don't like Hash.new let's use assert_equal({}, @john.params)

@oliveiraethales

oliveiraethales Jan 6, 2014

Contributor

Perfect, was in doubt which one to use, thanks for clarifying :D

@senny senny and 1 other commented on an outdated diff Jan 6, 2014

activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+* Fixed ActiveRecord::Store nil conversion TypeError when using YAML coder.
+ In case the YAML passed as paramter is nil, uses an empty string
+
+ Fixes #13570
@senny

senny Jan 6, 2014

Member

Just a nitpick: can you add a . after Fixes XXX.. It should look like:

Fixes #13570.
Member

senny commented Jan 6, 2014

@oliveiraethales thanks for sticking with me. I added some final comments but this is looking good.

Contributor

oliveiraethales commented Jan 6, 2014

No, thank you for the patience! :D Awesome to work with guys like you (:

I'll make the improvements, let me know anything else that can be improved and when to squash.

Member

senny commented Jan 6, 2014

@oliveiraethales please squash after you've made the changes.

Contributor

oliveiraethales commented Jan 6, 2014

@senny Done! (:

@senny senny commented on an outdated diff Jan 6, 2014

activerecord/test/cases/store_test.rb
end
+
@senny

senny Jan 6, 2014

Member

let's keep the newline at the end of this file.

@senny senny commented on an outdated diff Jan 6, 2014

activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+* Fixed ActiveRecord::Store nil conversion TypeError when using YAML coder.
+ In case the YAML passed as paramter is nil, uses an empty string
@senny

senny Jan 6, 2014

Member

also missing a dot at the end of the sentence.

Member

senny commented Jan 6, 2014

@oliveiraethales two tiny bits more. you can simply commit --amend them and then force push again.

@oliveiraethales oliveiraethales Fix: ActiveRecord::Store TypeError conversion when using YAML coder
Renaming the test accordingly to its behaviour

Adding 'Fixes' statement to changelog

Improving tests legibility & changelog

Undoing mistakenly removed empty line & further improving changelog
901a0c8
Contributor

oliveiraethales commented Jan 6, 2014

Done :3 Next time will use --amend, I was doing the 'old' way, pushing and then rebasing

@senny senny added a commit that referenced this pull request Jan 6, 2014

@senny senny Merge pull request #13593 from oliveiraethales/store_yaml_coder
Fix: ActiveRecord::Store TypeError conversion when using YAML coder
f2b80a4

@senny senny merged commit f2b80a4 into rails:master Jan 6, 2014

Member

senny commented Jan 6, 2014

@oliveiraethales thank you for your contribution 💛

Contributor

laurocaetano commented Jan 6, 2014

💛 💚

Contributor

oliveiraethales commented Jan 6, 2014

Thanks a LOT @senny! <3 <3 ❤️ 😻
Looking forward to the next one :D

@senny senny added a commit that referenced this pull request Jan 6, 2014

@senny senny Merge pull request #13593 from oliveiraethales/store_yaml_coder
Fix: ActiveRecord::Store TypeError conversion when using YAML coder
Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/store.rb
	activerecord/test/schema/schema.rb
7678ab6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment