-
Notifications
You must be signed in to change notification settings - Fork 154
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
Comments
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 Care to PR a fix? |
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
With grape-entity, I can write code like the following:
This utilizes Ruby's somewhat obscure
&
operator to callSymbol#to_proc
and forwardfilename
topath
. 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 toself
), but Ruby 2 had a design issue whereto_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:Here,
block.parameters.count
(or,block.arity
) will return an incorrect value in Ruby 2: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:
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:
The text was updated successfully, but these errors were encountered: