Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Draper::Decoratable#== #391

Closed
tovodeverett opened this Issue · 9 comments

3 participants

Toby Ovod-Everett Steve Klabnik Andrew Haines
Toby Ovod-Everett

For some reason, Draper::Decoratable#== isn't working in my test suite, but it seems to work fine from rails c. I can't figure out why. In the test suite, the first works and the second does not:

it { assigns(:roll).should == roll }
it { roll.should == assigns(:roll) }

It's not a big deal for assigns, but it does cause problems when working with the shoulda assign_to(:foo).with(bar) matcher (which reverses the test).

I'm still figuring out my way around the debugger, but here's part of a debugger session:

/home/toby/.gem/ruby/1.9.1/bundler/gems/draper-c602a37cea2e/lib/draper/decoratable.rb:25
super || (other.respond_to?(:source) && super(other.source))
(rdb:1) p super
false
(rdb:1) p other.respond_to?(:source)
true
(rdb:1) p super(other.source)
false
(rdb:1) p self
#<Roll id: 1, name: "2034-NAME-123", authcode: "CRPDCGDF", date_A: "2012-11-02", date_Z: "2012-11-03", desc: "Description of Roll", desc_ext: "Extended description of Roll", photographers: "Toby, Corina", camera: "Carol's 35mm", picturecd_proc_date: "2012-11-04", picturecd_roll_num: "123456", created_at: "2012-12-15 00:57:06", updated_at: "2012-12-15 00:57:06">
(rdb:1) p other.source
#<Roll id: 1, name: "2034-NAME-123", authcode: "CRPDCGDF", date_A: "2012-11-02", date_Z: "2012-11-03", desc: "Description of Roll", desc_ext: "Extended description of Roll", photographers: "Toby, Corina", camera: "Carol's 35mm", picturecd_proc_date: "2012-11-04", picturecd_roll_num: "123456", created_at: "2012-12-15 00:57:06", updated_at: "2012-12-15 00:57:06">
(rdb:1) p self == other.source
true
(rdb:1) p super(other.source)
false

The current body of Draper::Decoratable#== is:

super || (other.respond_to?(:source) && super(other.source))

Everything works fine if I replace that with:

super || (other.respond_to?(:source) && self == other.source)

Thoughts? BTW, the reason for the initial screw up on the title was that I thought it was related to SimpleCov, but I think I just got confused.

Steve Klabnik

I have no idea what's right here, I'll have to give it some thought.

Andrew Haines
Collaborator

That's surprising. The only difference should be that self == other.source recurses; it still calls super(other.source) indirectly when it re-enters the function (in the first branch of the ||).

Do you have something else overriding #==?

Steve Klabnik

@tovodeverett PING! Can you give us any more information about this?

Toby Ovod-Everett

Sorry for dropping off the face of the planet for a while - I've been trying to make progress on the code that uses Draper in between trying to be present for my family during the holidays (great for family, not so good for being productive).

I'll do my best to do a more in depth investigation tomorrow, but I can report that the issue still appears to be present as of right now using the package at https://github.com/drapergem/draper.

Also, I had to modify Draper::CollectionDecorator#initialize like so to force the source object to load and convert to an array before freezing the thing:

module Draper
  class CollectionDecorator
    def initialize(source, options = {})
      options.assert_valid_keys(:with, :context)
      @source = source.to_a.dup.freeze
      @decorator_class = options[:with]
      @context = options.fetch(:context, {})
    end
  end
end

Otherwise, code like Draper::CollectionDraper.new(Model.order('foo')) results in RuntimeError: can't modify frozen ActiveRecord::Relation when the source object tries to load values.

Steve Klabnik

It's all good! Holidays are hard. This is just the last issue we know about, and I'd like to get 1.0 out the door.

Andrew Haines
Collaborator

Yep, good call on the to_a, I've added that now.

Toby Ovod-Everett

I think I've found the scenario that raises the issue. If you have two undecorated model objects that point to the same record, Rails overrides == to return true. If you decorate one of these and call decorated_1 == undecorated_2, you'll get true. But if you reverse that call, you'll get false!

I can demonstrate this from rails c - I'll append the session to this post. With 4 objects, there are 12 possible intra-object comparisons, and 2 of those return false.

From snooping around in the debugger, it looks to me like ActiveRecord::Base#== gets called before the code in Draper::Decoratable, which makes sense! After all, we're mixing in to ActiveRecord::Base#==, so its code gets called before anything we add via extend. So it tries super first, we fail (because the logic to equate two different model objects that point to the same record is above us, not below us), and then it promptly fails because the two are of different classes.

I see two solutions. One is the one I suggested at the top (reissuing == so we can re-enter ActiveRecord::Base#== instead of using super). The other would be to somehow mix Draper::Decoratable#== into every class that inherits from ActiveRecord::Base by hooking inherited. The latter one strikes me as fraught with peril.

<TL;DR>
Here's my rails c session:

vin:toby 2:17pm ~/ruby/photos (drying_out_controllers)$ rails c
Loading development environment (Rails 3.2.9)
irb(main):001:0> undecorated_1 = Photo.limit(1)[0]
  Photo Load (0.4ms)  SELECT "photos".* FROM "photos" LIMIT 1
=> #<Photo id: 30129, roll_id: nil, idx: 1, . . .>
irb(main):002:0> undecorated_2 = Photo.limit(1)[0]
  Photo Load (0.2ms)  SELECT "photos".* FROM "photos" LIMIT 1
=> #<Photo id: 30129, roll_id: nil, idx: 1, . . .>
irb(main):003:0> decorated_1 = undecorated_1.decorate
=> #<PhotoDecorator:0x00000006a76e98 @source=#<Photo id: 30129, roll_id: nil, idx: 1, . . .>, @context={}>
irb(main):004:0> decorated_2 = undecorated_2.decorate
=> #<PhotoDecorator:0x00000006a79fd0 @source=#<Photo id: 30129, roll_id: nil, idx: 1, . . .>, @context={}>
irb(main):005:0> undecorated_1 == undecorated_2
=> true
irb(main):006:0> undecorated_2 == undecorated_1
=> true
irb(main):007:0> undecorated_1 == decorated_1
=> true
irb(main):008:0> decorated_1 == undecorated_1
=> true
irb(main):009:0> undecorated_2 == decorated_2
=> true
irb(main):010:0> decorated_2 == undecorated_2
=> true
irb(main):011:0> decorated_1 == decorated_2
=> true
irb(main):012:0> decorated_2 == decorated_1
=> true
irb(main):013:0> decorated_1 == undecorated_2
=> true
irb(main):014:0> undecorated_2 == decorated_1
=> false
irb(main):015:0> decorated_2 == undecorated_1
=> true
irb(main):016:0> undecorated_1 == decorated_2
=> false
Toby Ovod-Everett

Some more thoughts on this. Because of the way equality is checked and the order of the || subclauses in the various == implementations, it would probably be preferable for us to sit before ActiveRecord::Base#== rather than after it. Where we are currently means that the Draper::Decoratable code to check for source gets called before ActiveRecord::Base#== tries its magic. The code path follows the super calls on the left hand side of the || in each implementation of == and then when the one built into Ruby returns false, it will start unwinding back up the stack trying each alternative test one by one in reverse order of the initial calls. I think the intent is that we'd like ActiveRecord::Base#== to take its stab first, and only if it fails to then check for source. If that's the case, we need to sit before it.

From that perspective, I'd like to propose that instead of mixing in an implementation of ==, we consider chaining == using alias_method_chain.

If we do that, we should be able to dispense with reissuing == and rely on super. However, we still might want to reissue == in order to provide a more robust implementation should some other module decide to chain == as well. That way we would ensure access to the other modules' code after we extract source independent of the order of the chaining.

Andrew Haines
Collaborator

Thanks for the test case @tovodeverett, I get the same in our dummy app. I would prefer to avoid alias_method_chain, and just change the implementation from super(other.source) to self == other.source, since that works. See #404.

Andrew Haines haines closed this issue from a commit
Andrew Haines haines Use `self == other.source` in Decoratable#==
Apparently AR::Base does something tricky with #== which causes
`super(other.source)` to break

Closes #391
b83a95f
Andrew Haines haines closed this in b83a95f
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.