draper breaks #== on persisted ActiveRecord models, if you aren't using delegate_all, results in NoMethodError on #id #859
Comments
|
If there are any currently active Draper maintainers, it would be lovely to get some feedback! As I think about it, it seems clear the 'right' answer is that Decorators should not over-ride instance_of to claim they are instances of their decorated object's class unless I could prepare a PR to do that, but before investing that time it would be awesome to get feedback:
|
I agree that
Now, there is.
Though I'm quite new here, I can dig through it easily. Unfortunately, core authors seem to be accessible no more. |
|
Starting from 30d209f, I found that I'll try to remove both and look for what to be broken. |
|
That's exciting there is some maintenance energy on this again! Thank you for your work! I have stopped using Draper in the ensueing 5 years, so no longer have a stake in this. |
@jrochkind, I'm just wondering to consider what to do with Draper in a perspective: Do you use any alternatives now and, if you do, what advantages do they have over Draper? |
|
I found that embracing ViewComponent has largely eliminated my desire for Draper. But I think different people use Draper in somewhat different ways. At the time I stopped using it, the advantage of antyhing else was simply that Draper was unmaintained, and that I kept running into weird edge cases causing me problems, like this. I think Draper perhaps required over-complicated implementation to work with Rails how it wanted, making it somewhat fragile. |
Decorators shouldn't pretend to be instances of the underlying model classes for the following reasons: 1. It's terribly wrong to pretend being a class without implementing it's interface. It can break third party code. 2. Hacking Ruby core methods can lead to unexpected and hard to understand behavior. It should be avoided without a strong reason behind. 3. I'd prefer compatibility patches to be narrow-scoped and live in there own modules. Resolves drapergem#859. Reverts drapergem#72, drapergem#110, drapergem#417, drapergem#497.
If you aren't using
delegate_allin your decorator, draper breaks calling #== to compare an ActiveRecord model to a Decorator decorating an instance of the same class as the model.Why does this matter, why would you ever do it? Not sure, but rspec is trying to do it when reporting a spec failure.
Reproduction
Will raise:
Why does it happen?
widget#==is from Draper::Decorator::Equality. The first thing it does is callsuper.The super method is ActiveRecord::Core#==
That method first calls
super(and gets false from BasicObject#==). Then it checks to see ifwidget_decorator.instance_of?(widget.class). Because draper overrides instance_of on decorators, it says thatwidget_decoratoris an instance ofWidget.Then ActiveRecord will try to compare widget_decorator.id to widget.id and get the NoMethodException, because
WidgetDecoratordoes notdelegate_all(or specifically delegateid), so widget_decorator has no#id.Why does it matter, why would you do this?
I'm not totally sure, but in my actual rspec suite, a failing spec triggered something in rspec to try to compare a Widget to a WidgetDecorator, raising and interrupting rspec before it even got to give me details of failing test spec.
I'm not totally sure what's going on, but here's an actual exception backtrace. (This one involves LazyHelpers because that's in my actual app, but LazyHelpers is NOT necessary to reproduce the problem).
stack trace
How to fix it?
I have no idea, it's the result of a strange interaction of several odd meta-programming functions in both Draper and ActiveRecord. Pretty unclear how to cleanly fix it.
In general, overriding
instance_of?to say that the decorator is an ActiveRecord, when if it doesn't usedelegate_allit doesn't really have the API/type of an ActiveRecord... is a very dangerous thing to do.But it's probably there for a reason? Or perhaps you should only get the
instance_ofoverride if you havedelegate_all-- that would fix this issue, not sure if it would break something else.Workaround?
If you aren't using
delegate_all, you probably still want todelegate :idjust to avoid messing things up in this case, or any other case that might use==where the operands include a Decorator and an ActiveRecord.That's what I did, tested, it resolves the issue.
The draper README examples of not using
delegate_alland selectively delegating methods do not show delegating:id-- but you probably always should to avoid this issue.The text was updated successfully, but these errors were encountered: