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

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

Open
mttkay opened this issue Aug 19, 2021 · 3 comments
Open

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

mttkay opened this issue Aug 19, 2021 · 3 comments

Comments

@mttkay
Copy link

mttkay commented Aug 19, 2021

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
@rgalanakis
Copy link

rgalanakis commented Mar 23, 2022

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

@dblock
Copy link
Member

dblock commented Apr 16, 2022

@rgalanakis Care to PR a fix?

@rgalanakis
Copy link

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).

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

No branches or pull requests

3 participants