grape-entity icon indicating copy to clipboard operation
grape-entity copied to clipboard

Ruby 3: Entities exposing fields via Symbol#to_proc are broken

Open mttkay opened this issue 3 years ago • 3 comments

With grape-entity, I can write code like the following:

    class Blob < Grape::Entity
      expose :path
      expose :filename, &:path
    end

This utilizes Ruby's somewhat obscure & operator to call Symbol#to_proc and forward filename to path. The proc returned by this function takes the receiver of the method described by the symbol as its first argument, and the actual method parameters as the remaining arguments. Obviously the first argument is required (since it is equivalent to self), but Ruby 2 had a design issue where to_proc for symbols would report signature metadata that is not really correct. See https://rubyreferences.github.io/rubychanges/3.0.html#symbolto_proc-reported-as-lambda for a good summary.

The problem in grape-entity is this:

In Entity::exec_with_object, there is a test where grape reflects on the signature of such a block:

    def exec_with_object(options, &block)
      if block.parameters.count == 1
        instance_exec(object, &block)
      else
        instance_exec(object, options, &block)
      end
    rescue StandardError => e
      ...
    end

Here, block.parameters.count (or, block.arity) will return an incorrect value in Ruby 2:

[4] pry(#<API::Entities::Blob>)> block.parameters
=> [[:rest]]

[6] pry(#<API::Entities::Blob>)> block.arity
=> -1

This is Ruby lying about proc arity, since it tosses all possible arguments into a single optional argument (-1 means all arguments are optional, which is not true; you always need a receiver here.)

Ruby 3 fixes this:

[1] pry(#<API::Entities::Blob>)> block.parameters
=> [[:req], [:rest]]
[4] pry(#<API::Entities::Blob>)> block.arity
=> -2

This is correct: the first argument is now required, since it is the receiver of path. The second argument is the optional parameters (perhaps none.) The arity is also correct now: -2 means 1 required parameter, the rest optional.

What this means is that now grape-entity goes down the wrong code path in exec_with_object, since parameter count is now 2, not 1.

I'm not actually all that familiar with grape, I am just working on the Ruby 3 migration at GitLab so I wanted to raise awareness about this somewhat subtle issue.

A simple workaround is to rewrite the entity like so:

    class Blob < Grape::Entity
      expose :path
      expose :filename do |instance|
        instance.path
      end
    end

mttkay avatar Aug 19 '21 13:08 mttkay

For reference, we added this to our base entity class:

    def self.expose(*args, &block)
      # See https://github.com/ruby-grape/grape-entity/issues/354
      # If we pass in a lambda, we pass Grape a proc that accepts the canonical number
      # of arguments (2). Then we invoke the lambda in the appropriate manner.
      if block&.lambda?
        orig_block = block
        block = proc do |obj, opts|
          if orig_block.arity <= 0
            # For things like `expose :foo, &bar
            # Obviously you'd use :bar for this but there are cases you may have a 0-arity lambda.
            # Note that nested exposures (blocks with no args) are canonically done with a block,
            # so do not hit this branch.
            instance_exec(obj, &orig_block)
          elsif orig_block.arity == 1
            orig_block.call(obj)
          elsif orig_block.arity == 2
            orig_block.call(obj, opts)
          else
            raise RuntimeError, "expose blocks must take 0, 1, or 2 parameters"
          end
        end
      end
      super(*args, &block)
    end

It succeeds for this spec:

    it "can work around Symbol.to_proc issues with grape-entity in Ruby 3" do
      entity_cls = Class.new(MyProj::Service::Entities::Base) do
        expose :value, &:value
        expose :value, as: :nested do
          expose :value, as: :nested_value
        end
        expose :delegated, &self.delegate_to(:obj, :value)
        expose(:block1) { |o| o.value }
        expose(:block2) { |_o, opts| opts[:passed_option] }
        expose :lambda1, &lambda { |o| o.value }
        expose :lambda2, &lambda { |_o, opts| opts[:passed_option] }
      end

      entity_type = Struct.new('CustomObj', :value, :obj)

      x = entity_type.new(1,entity_type.new(2, nil))
      j = entity_cls.represent(x, passed_option: 5)
      expect(JSON.parse(j.to_json)).to eq({'block1'=>1, 'block2'=>5, 'delegated'=>2, 'lambda1'=>1, 'lambda2'=>5, 'value'=>1, "nested"=>{"nested_value"=>1}})
    end

rgalanakis avatar Mar 23 '22 19:03 rgalanakis

@rgalanakis Care to PR a fix?

dblock avatar Apr 16 '22 15:04 dblock

Yeah I will try to get to it soon (unfortunately we finished upgrading all of our Ruby 2.7 projects but I should still get a chance).

rgalanakis avatar Apr 19 '22 16:04 rgalanakis