Coercion scope bug

[maxlinc]

Found a nasty bug.

The coercions are being stored on the superclass rather than the current class. This means that two classes that share the same superclass can't have conflicting coercions. This PR has I test, I don't have a fix yet.

I can't even mark the test as pending. Just defining OtherNestedCoercableHash breaks NestedCoercableHash even if it's never used. The problem is that it defines the coercion on RootCoercableHash so the definition from one class leaks into the scope of the other. Since the type was different that breaks other specs.

[maxlinc]

You can also see this by calling key_coercions:

instance[:nested].class
=> NestedCoercableHash
[5] pry(#<RSpec::ExampleGroups::HashieExtensionsCoercion::CoerceKey::Nesting>)> instance[:nested].class.key_coercions
=> {:foo=>Symbol, :bar=>Integer, :nested=>NestedCoercableHash, :other=>OtherNestedCoercableHash, :nested_list=>[NestedCoercableHash], :nested_hash=>{String=>NestedCoercableHash}}

It would make sense for coercions in BaseCoercableHash to show up in NestedCoercableHash since that's the superclass, but it's actually picking up coercions from NestedCoercableHash.

[dblock]

Yep :(

[maxlinc]

I think it's fixed. Double check it for me... I was expecting it to be something trickier.

In fact, I'm not sure this is 100% fixed. It perhaps it still has issues with deeply nested coercable structures. Is there a way to kill off the inherited method completely? It seems like a hack.

[michaelherold]

There was a bug that was similar to this one, only in reverse, for indifferent_access (see #195). I think the takeaway is that book-keeping for this type of stuff is hard.

In this case, the #dup makes a lot of sense. We want to be able to add to the coercions in the new class, but still maintain the coercions in the parent class. Without the dup, if we add to the list of coercions, we add to both the parent and the child.

What feels like a hack about the inherited hook to you? It makes a lot of domain sense to me. Without changing the way we do book-keeping -- which I'm not sure how we could, given the modular nature -- I can't think of a better way to express this.

[martinstreicher]

I found this bug too. My rspec test to show it is at martinstreicher@b0d2f96.

[martinstreicher]

I submitted a PR for my spec. Perhaps it is another demonstration that the fix works in a form more likely to occur in an app.

[dblock]

I like #288 as well, I would take both tests, either needs a CHANGELOG, please.

[martinstreicher]

Sure, I will add one.

[martinstreicher]

CHANGELOG updated in my branch.

[dblock]

Merged spec via fd07b80.

[martinstreicher]

It was part of #288 #288

On Mar 31, 2015, at 10:57 AM, Daniel Doubrovkine (dB.) @dblockdotorg [email protected] wrote:

Can I have a PR please?

—
Reply to this email directly or view it on GitHub #282 (comment).