From 31fa4793db5cc534318d4edc595bc487e354866c Mon Sep 17 00:00:00 2001 From: Cliff Braton Date: Mon, 3 Apr 2017 11:24:01 -0500 Subject: [PATCH] Don't do runtime delegation, delegate explicitly (#789) * Don't do runtime delegation, delegate explicitly If someone accidentally calls `super` instead of `object` inside a decorator, all hell breaks loose. ```rb class Post < ActiveRecord::Base def title "title" end end class PostDecorator < Draper::Decorator delegate_all def title super and "overriden title" end end post = PostDecorator.new(Post.new) post.title #=> "overriden title" post.title #=> "title" ``` This patch makes both `super` and `object` work. I'm not sure why runtime delegation was added in the first place, but it seems very wrong to add methods on the fly. * Add documentation and more tests. * Consistent line spacing in test. --- README.md | 5 +++- lib/draper/automatic_delegation.rb | 8 +++--- spec/draper/decorator_spec.rb | 41 ++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2b1068c..6989090 100644 --- a/README.md +++ b/README.md @@ -496,7 +496,10 @@ end When your decorator calls `delegate_all`, any method called on the decorator not defined in the decorator itself will be delegated to the decorated object. This -is a very permissive interface. +includes calling `super` from within the decorator. A call to `super` from within +the decorator will first try to call the method on the parent decorator class. If +the method does not exist on the parent decorator class, it will then try to call +the method on the decorated `object`. This is a very permissive interface. If you want to strictly control which methods are called within views, you can choose to only delegate certain methods from the decorator to the source model: diff --git a/lib/draper/automatic_delegation.rb b/lib/draper/automatic_delegation.rb index 8f8d65e..3457d48 100644 --- a/lib/draper/automatic_delegation.rb +++ b/lib/draper/automatic_delegation.rb @@ -2,12 +2,14 @@ module Draper module AutomaticDelegation extend ActiveSupport::Concern - # Delegates missing instance methods to the source object. + # Delegates missing instance methods to the source object. Note: This will delegate `super` + # method calls to `object` as well. Calling `super` will first try to call the method on + # the parent decorator class. If no method exists on the parent class, it will then try + # to call the method on the `object`. def method_missing(method, *args, &block) return super unless delegatable?(method) - self.singleton_class.delegate method - send(method, *args, &block) + object.send(method, *args, &block) end # Checks if the decorator responds to an instance method, or is able to diff --git a/spec/draper/decorator_spec.rb b/spec/draper/decorator_spec.rb index 3a2f606..f4f8e0a 100644 --- a/spec/draper/decorator_spec.rb +++ b/spec/draper/decorator_spec.rb @@ -572,12 +572,43 @@ module Draper expect(decorator.hello_world).to be :delegated end - it "adds delegated methods to the decorator when they are used" do - decorator = Decorator.new(double(hello_world: :delegated)) + it 'delegates `super` to parent class first' do + parent_decorator_class = Class.new(Decorator) do + def hello_world + "parent#hello_world" + end + end - expect(decorator.methods).not_to include :hello_world - decorator.hello_world - expect(decorator.methods).to include :hello_world + child_decorator_class = Class.new(parent_decorator_class) do + def hello_world + super + end + end + + decorator = child_decorator_class.new(double(hello_world: 'object#hello_world')) + expect(decorator.hello_world).to eq 'parent#hello_world' + end + + it 'delegates `super` to object if method does not exist on parent class' do + decorator_class = Class.new(Decorator) do + def hello_world + super + end + end + + decorator = decorator_class.new(double(hello_world: 'object#hello_world')) + expect(decorator.hello_world).to eq 'object#hello_world' + end + + it 'raises `NoMethodError` when `super` is called on for method that does not exist' do + decorator_class = Class.new(Decorator) do + def hello_world + super + end + end + + decorator = decorator_class.new(double) + expect{decorator.hello_world}.to raise_error NoMethodError end it "allows decorator to decorate different classes of objects" do