Fix caching behavior in CollectionDecorator#decorated_collection #388

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@tovodeverett

I think I wrote enough tests to ensure the new logic in CollectionDecorator#decorated_collection is correct. My goals in this implementation were:

  • Check for any scenarios where the cached decorators no longer match the underlying source objects.
  • Not replace @decorated_collection with a new array (my initial approach did this, and several tests blew up that were placing mocks on @decorated_collection).
  • Have fast behavior for calls where there are no existing decorators.
  • Preserve existing decorators wherever possible.
@tovodeverett

I'm not really happy with DecoratedAssociation#decorator_class mutating options[:with], but I'm not quite ready to excise the behavior and refactor it somewhere else.

Never the less, it turns out there's a bug in #decorator_class that predates both this update and #385. Specifically, if options[:with] isn't set, the first time #decorator_class gets called it will set options[:with] to :infer and then return CollectionDecorator. But the second time it gets called, it will just return :infer. This leads to occasional error messages like this:

NoMethodError: undefined method `decorate' for :infer:Symbol

The tovodeverett@c602a37 commit is a quick fix, although I agree with @haines that a full refactoring of #decorator_class and #decorator_options is probably a good idea.

@tovodeverett

Is there a reason we can't get rid of :infer completely in DecoratedAssociation and move that behavior into CollectionDecorator? Right now the behavior in CollectionDecorator is that if you don't specify :with, it tries to infer it from the class name. Of course, that only makes sense if the class name isn't Draper::CollectionDecorator. So why can't CollectionDecorator.inferred_decorator_class return :infer if self == Draper::CollectionDecorator? If someone does indeed subclass CollectionDecorator, then the class name mangling behavior would remain, but otherwise it can just do the right thing. If we move this behavior into CollectionDecorator (where it will simplify the interface to CollectionDecorator as well), then we can easily clean up the logic in DecoratedAssociation.

@haines
Member
haines commented Dec 14, 2012

I don't like this approach (sorry, @tovodeverett!). The problem is that every time we call a method on the decorated collection, it has to iterate over the entire collection first, and then do an array comparison... at least 2N operations. Yikes.

I don't see a way to fix it neatly. I think the take-home message from issues like #378 and #387 is not that we need to fix the caching, but that modifying collections after decorating them is bad news. It's particularly bad for methods like build that are delegated to the source collection; for methods like reverse! the behaviour is less harmful because it just acts on our cached decorated array (although it still causes a mismatch between source_collection and decorated_collection).

The moral is that decorating should be pretty much the last thing that happens before handing off to the view. I think we maybe need to make that clearer.

The decorator_class stuff is a good catch, although I don't actually think it was an issue before b2f6388 because it would only ever have been called once. But in any case it goes to show that those methods need to be reworked! I will tackle that tomorrow, it's time for bed here :)

@tovodeverett

I was getting ready to make a case for doing some actual performance testing so we know the actual cost, but then I noticed that you pointed out a bug in the code I developed. Because reverse! gets delegated to the cached decorated array, it will reverse the array, but then the source collection doesn't get reversed and the next call will simply redo the decorated array. The fix for that wouldn't be as easy. In the end, the only workaround I really see is coming up with some sort of on-demand cached decorating (i.e. hooking just the methods that return elements and attaching those to some sort of hash-based cache), and that's a lot of work to get just right.

I'm reminded of Terje Mathisen quote, "Almost all programming can be viewed as an exercise in caching."

So I'd say we should just make it really clear that Decorators (both collections and singulars) should be treated as read-only and only used by views for displaying information.

@haines
Member
haines commented Dec 14, 2012

So I'd say we should just make it really clear that Decorators (both collections and singulars) should be treated as read-only and only used by views for displaying information.

Yep, agreed. I think this is one of the problems with the overloaded terminology. Draper::Decorators aren't general-purpose decorators but are special cases which are variously dubbed "view models", "presenters", "exhibits"... I think I'm right in saying that classically they should really just be called "views" but of course Rails uses that for what should be called "templates". Since we're on the subject of quotes, perhaps this sums it up :)

@haines haines closed this Dec 14, 2012
@haines
Member
haines commented Dec 19, 2012

Is there a reason we can't get rid of :infer completely in DecoratedAssociation and move that behavior into CollectionDecorator?

@tovodeverett I've gone one step further and removed with: :infer completely, and fixed the issue you spotted. See #394.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment