Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Module#delegate_missing_to #23824

Closed
dhh opened this issue Feb 23, 2016 · 13 comments
Closed

Add Module#delegate_missing_to #23824

dhh opened this issue Feb 23, 2016 · 13 comments

Comments

@dhh
Copy link
Member

@dhh dhh commented Feb 23, 2016

Here's a common pattern if you want to build a decorator:

    class Partition
      def initialize(first_event)
        @events = [ first_event ]
      end

      def people
        if @events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end

      private
        def respond_to_missing?(name, include_private = false)
          @events.respond_to?(name, include_private)
        end

        def method_missing(method, *args, &block)
          @events.send(method, *args, &block)
        end
    end

Would be nice to have it as:

    class Partition
      delegate_missing_to :@events

      def initialize(first_event)
        @events = [ first_event ]
      end

      def people
        if @events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end
    end
@dhh dhh added the activesupport label Feb 23, 2016
@dhh dhh added this to the 5.1.0 milestone Feb 24, 2016
@gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Feb 27, 2016

I have a free afternoon, I can take a stab at this.

gsamokovarov added a commit to gsamokovarov/rails that referenced this issue Feb 27, 2016
When building decorators, a common pattern may emerge:

    class Partition
      def initialize(first_event)
        @Events = [ first_event ]
      end

      def people
        if @Events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end

      private
        def respond_to_missing?(name, include_private = false)
          @events.respond_to?(name, include_private)
        end

        def method_missing(method, *args, &block)
          @events.send(method, *args, &block)
        end
    end

With `Module#delegate_missing_to`, the above is condensed to:

    class Partition
      delegate_missing_to :@Events

      def initialize(first_event)
        @Events = [ first_event ]
      end

      def people
        if @Events.first.detail.people.any?
          @events.collect { |e| Array(e.detail.people) }.flatten.uniq
        else
          @events.collect(&:creator).uniq
        end
      end
    end

David suggested it in rails#23824.
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 24, 2016

Is not this the same that SimpleDelegator does?

class Partition < SimpleDelegator
  def initialize(first_event)
    @events = [ first_event ]
    super(@events)
  end

  def people
    if @events.first.detail.people.any?
      @events.collect { |e| Array(e.detail.people) }.flatten.uniq
    else
      @events.collect(&:creator).uniq
    end
  end
end
@dhh
Copy link
Member Author

@dhh dhh commented May 24, 2016

I prefer not having to hijack the inheritance tree for such a simple feature. I find the use of inheritance + super odd as well. "delegate_missing_to" reveals intentions better and clearer.

On May 24, 2016, at 03:07, Rafael França notifications@github.com wrote:

This is not the same that SimpleDelegator does?

class Partition < SimpleDelegator
def initialize(first_event)
@Events = [ first_event ]
super(@Events)
end

  def people
    if @events.first.detail.people.any?
      @events.collect { |e| Array(e.detail.people) }.flatten.uniq
    else
      @events.collect(&:creator).uniq
    end
  end
end


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 24, 2016

👍. Agree it reveals the intention better.

@dhh
Copy link
Member Author

@dhh dhh commented May 24, 2016

I also like the parity with delegate_to. It feels like a natural extension
of it. Yes, the functionality is the same, but it's in a much nicer package.

On Tue, May 24, 2016 at 6:26 PM, Rafael França notifications@github.com
wrote:

👍. Agree it reveals the intention better.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#23824 (comment)

@connorjacobsen
Copy link

@connorjacobsen connorjacobsen commented May 24, 2016

Why not just use the already existing delegate method? Then you can explicitly define the interface for the decorator instead of just passing anything through.

It makes the decorator interface well-defined, and far less permissive.

class Partition
  def initialize(first_event)
    @events = [ first_event ]
  end

  delegate :some_decorator_method, :another_decorator_method, to: :@events
end
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 24, 2016

There are cases where you want a permissive decorator, and for these cases I believe the proposed API is good.

@connorjacobsen
Copy link

@connorjacobsen connorjacobsen commented May 24, 2016

Permissiveness aside, that functionality already exists in a much better way. The existing method forces the author to be explicit, better conveys intent, and provides an interface instead of just blanket allowing all of the things.

delegates_missing_to may reveal intention better than SimpleDelegator, but delegate on its own reveals intention far better than delegates_missing_to.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 24, 2016

delegates_missing_to may reveal intention better than SimpleDelegator, but delegate on its own reveals intention far better than delegates_missing_to.

That is true. And now we have a spectrum of choices for people form the more permissive to the more explicit.

Even with the existing methods, people often use the method_missing + respond_to_missing? pattern. This new method is to make this common pattern easier.

You could agree that "explicit, better conveys intent, and provides an interface instead of just blanket allowing all of the things" is the best approach but people may think the best approach is what they want to use and Rails is just providing tools so people can choose.

#23930 was merged, so I'm closing this one, but this doesn't mean that the discussion is over. If you have more inputs to give, please let us know.

@connorjacobsen
Copy link

@connorjacobsen connorjacobsen commented May 24, 2016

What you just described appears to be the complete opposite of http://david.heinemeierhansson.com/2012/rails-is-omakase.html.

I also don't agree with the idea that "people want to do this not so good thing, so let's make it easier to do that" is the best approach.

I personally use the method_missing + respond_to_missing? pattern outside of rails often, but the methods that the target object will respond_to_missing? are always limited with some mechanism or another. The proposed pattern doesn't allow for that. As written, if the target object responds to the thing, we can delegate to it. There is no way to constrain it.

I understand that we have differing opinions on this, and @dhh wants it, so it's probably going to stay in, but it should be reverted. It encourages bad practices, it doesn't add anything new despite adding extra code, and the whole idea that it is giving a number of options to do the same thing seems to be very against the Rails way.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented May 24, 2016

I personally use the method_missing + respond_to_missing? pattern outside of rails often, but the methods that the target object will respond_to_missing? are always limited with some mechanism or another. The proposed pattern doesn't allow for that. As written, if the target object responds to the thing, we can delegate to it. There is no way to constrain it.

I also limited the delegation with this mechanism but I also know that there are cases where you want a full delegation to the decorated object, and this is exactly what this method introduces.

It encourages bad practices

Who encourage bad practices are the people, not tools. You can perfectly use this tool to do best practices.

it doesn't add anything new despite adding extra code

Yes, it introduces a more intention revealing implementation for a common pattern.

the whole idea that it is giving a number of options to do the same thing seems to be very against the Rails way.

I think you are confusing with the Zen of Python here. There is nothing in the Rails way that says we can't have a number of options to do something. We have many ways to find a record in Active Record (where, find_by, find), we have many ways to create a column, we have many ways to create a text field.

@dhh
Copy link
Member Author

@dhh dhh commented May 25, 2016

Conor, just the other day I added this ninth tenet about sharp knives to the Rails Doctrine: http://rubyonrails.org/doctrine/#provide-sharp-knives. It applies well here and so does the tenet on beautiful code.

We've taken a common pattern and boiled it down to a single class method that's in line with existing behavior we expose around delegation. That satisfies both of these tenets and thus in my view is very much "the Rails way".

That's exactly why the doctrine was created, such that we can all have a clearer understanding of exactly what "the Rails way" is.

My use of this pattern is for decorators. Give me this object as it is with a handful of methods decorated on top of it. The intent is indeed to reveal all of the delegated object and then a bit extra.

On May 25, 2016, at 00:46, Connor Jacobsen notifications@github.com wrote:

What you just described appears to be the complete opposite of http://david.heinemeierhansson.com/2012/rails-is-omakase.html.

I also don't agree with the idea that "people want to do this not so good thing, so let's make it easier to do that" is the best approach.

I personally use the method_missing + respond_to_missing? pattern outside of rails often, but the methods that the target object will respond_to_missing? are always limited with some mechanism or another. The proposed pattern doesn't allow for that. As written, if the target object responds to the thing, we can delegate to it. There is no way to constrain it.

I understand that we have differing opinions on this, and @dhh wants it, so it's probably going to stay in, but it should be reverted. It encourages bad practices, it doesn't add anything new despite adding extra code, and the whole idea that it is giving a number of options to do the same thing seems to be very against the Rails way.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

jeremy referenced this issue May 25, 2016
And make sure that it doesn't even try to call the method in the target.
@alipman88
Copy link
Contributor

@alipman88 alipman88 commented Jul 15, 2019

Hi Rails Team, I've found a bug/edge case in the way the delegate_missing_to functionality collides with marshallization:

When an object using delegate_missing_to is marshalled (e.g. during caching), Ruby checks to see if the object responds to marshal_dump or _dump, which in turn calls the object's respond_to_missing? function. This in turn calls the object's delegation target - if the delegation target is a method which adds an instance variable to another object currently being marshalled, this may trigger a Ruby bug where the instance variable "overflows" into a subsequent object:

user = User.first
user.avatar
original_array = [user, nil]
serialized_array = Marshal.dump(original_array)
deserialized_array = Marshal.load(serialized_array)

original_array[1]
# => nil

deserialized_array[1]
# => :@attachment_changes

What's happening is that Ruby marshallizes objects recursively, starting by counting the number of instance variables, and then marshallizing each instance variable. If, during the marshallization of an instance variable, an instance variable is added to the parent object, the extra instance variable (which wasn't originally counted) overflows upon demarshallization: https://bugs.ruby-lang.org/issues/15968. (Future versions of Ruby will raise an "instance variable added" error, but that doesn't completely resolve my issue.)

I encountered this in the wild when loading caching an ActiveRecord object after loading its attachment (#36522).

Right now, the scope of this behavior is limited, but I think it's likely to confound the occasional Rails developer, and arise more frequently if the delegate_missing_to functionality becomes more widely used in Rails apps & throughout the Rails codebase.

Apologies for bumping an old thread and/or being impatient, but I'm hoping to attract some reviewers to a pull request I submitted that resolves this issue: #36623. I feel the correct solution is somewhat subjective (either immediately return false if :marshal_dump or :_dump is passed to responds_to_missing?, or allow an exclude: [:marshal_dump, :_dump] hash parameter to be passed to delegate_missing_to), so I'm hoping to collect some community input as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants