Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
### Next Release

* [#406](https://github.com/ruby-grape/grape-entity/pull/406): Handle symbol-to-proc wrappers (`&:method_name`) where the method uses `delegate` or `method_missing` - [@marcrohloff](https://github.com/marcrohloff).
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entry is placed above the #### Features / #### Fixes headers rather than under one of them. Please move the line under #### Fixes.


#### Features

* Your contribution here.
Expand Down
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ Implement your feature or bug fix.

Ruby style is enforced with [Rubocop](https://github.com/bbatsov/rubocop), run `bundle exec rubocop` and fix any style issues highlighted.

Make sure that `bundle exec rake` completes without errors.
Make sure that `bundle exec rake` and `rubocop` completes without errors.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Make sure that `bundle exec rake` and `rubocop` completes without errors.
Make sure that `bundle exec rake` and `rubocop` complete without errors.

Subject-verb agreement: two subjects -> complete. Also the line above already instructs users to run bundle exec rubocop, so this becomes a near-duplicate 🤷


#### Write Documentation

Expand Down
3 changes: 2 additions & 1 deletion lib/grape_entity/entity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,8 @@ def ensure_block_arity!(block)
end

arity = object.method(origin_method_name).arity
return if arity.zero?
# functions defined using `delegate` or `method_missing` have an arity of -1
return if arity <= 0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too permissive and reintroduces the bug class #389 was meant to prevent. Ruby's negative arities encode -(required + 1), so def foo(a, *rest) has arity -2 and def foo(a, b, *rest) has arity -3 - both have required positional
args, but both now sail through this guard and blow up later at instance_exec(object, &block) with a "wrong number of arguments" raised from inside user code, instead of the friendly ArgumentError this method was designed to produce.

Worth noting: def method_missing(method, ...) itself has arity -2, so even the stated motivating case is partially mishandled by <= 0.

Suggested fix is match the actual intent ("no required positional args"):

arity = object.method(origin_method_name).arity
required = arity >= 0 ? arity : -arity - 1
return if required.zero?


raise ArgumentError, <<~MSG
Cannot use `&:#{origin_method_name}` because that method expects #{arity} argument#{'s' if arity != 1}.
Expand Down
47 changes: 47 additions & 0 deletions spec/grape_entity/entity_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,14 @@ class BogusEntity < Grape::Entity

describe 'blocks' do
class SomeObject
class SomeObjectDelegate
def method_using_delegation
'delegated-result'
end
end

delegate :method_using_delegation, to: :delegate_object

def method_without_args
'result'
end
Expand All @@ -415,6 +423,23 @@ def method_with_multiple_args(_object, _options)
def raises_argument_error
raise ArgumentError, 'something different'
end

def method_missing(method, ...)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding method_missing directly to SomeObject, could you extract the new fixtures into subclasses? That way the shared class stays clean and the existing tests (especially the "undefined method" one) don't depend on your
respond_to_missing? being correct to pass.

Something like:

SomeObjectWithMissing = Class.new(SomeObject) do
  def method_missing(method, ...)
    return 'missing-result' if method == :method_using_missing
    super
  end

  def respond_to_missing?(method, include_private = false)
    method == :method_using_missing || super
  end
end

SomeObjectWithDelegation = Class.new(SomeObject) do
  InnerDelegate = Class.new { def delegated_method = 'delegated-result' }
  delegate :delegated_method, to: :inner
  def inner = @inner ||= InnerDelegate.new
end

and specs kind of:

context 'with &: referencing a method_missing-backed method' do
  specify do
    subject.expose :using_missing, &:method_using_missing

    object = SomeObjectWithMissing.new
    expect(subject.represent(object).value_for(:using_missing)).to eq('missing-result')
  end
end

context 'with &: referencing a delegated method' do
  specify do
    subject.expose :using_delegation, &:delegated_method

    object = SomeObjectWithDelegation.new
    expect(subject.represent(object).value_for(:using_delegation)).to eq('delegated-result')
  end
end

with the regression case that pins the boundary:

context 'with &: referencing a method with required args and a splat (arity -2)' do
  specify do
    subject.expose :x, &:method_with_required_and_splat

    object = SomeObjectWithMissing.new  # or any object with such a method
    expect do
      subject.represent(object).value_for(:x)
    end.to raise_error(ArgumentError, match(/method expects/))
  end
end

return 'missing-result' if method.to_sym == :method_using_missing

super
end

def delegate_object
@delegate_object ||= SomeObjectDelegate.new
end

private

def respond_to_missing?(method, include_private = false) # rubocop:disable Style/OptionalBooleanParameter
method.to_sym == :method_using_missing ||
super
end
end

describe 'with block passed in' do
Expand Down Expand Up @@ -459,6 +484,28 @@ def raises_argument_error
end
end

context 'with block passed in via & that uses `missing_method`' do
specify do
subject.expose :using_missing, &:method_using_missing

object = SomeObject.new
expect(object.method(:method_using_missing).arity).to eq(-1)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value assertion is the one that proves grape-entity handles it correctly. The arity line is testing Ruby.

Suggested change
expect(object.method(:method_using_missing).arity).to eq(-1)

value = subject.represent(object).value_for(:using_missing)
expect(value).to eq('missing-result')
end
end

context 'with block passed in via & that uses `delegate`' do
specify do
subject.expose :using_delegation, &:method_using_delegation

object = SomeObject.new
expect(object.method(:method_using_delegation).arity).to eq(-1)
value = subject.represent(object).value_for(:using_delegation)
expect(value).to eq('delegated-result')
end
end

context 'with block passed in via &' do
specify do
subject.expose :that_method_with_one_arg, &:method_with_one_arg
Expand Down
Loading