Treat Middleware::Base#initialize options as Hash.

[skarger]

Addresses #1992.

Before this PR, Grape::Middleware::Base is treating the options param as keyword args, but it's actually meant as a Hash positional argument.

grape/lib/grape/middleware/base.rb

Line 17 in 536622a

def initialize(app, **options)

The former style, **options, prompts a deprecation warning in Ruby 2.7.

gems/2.7.0/gems/rack-2.2.2/lib/rack/builder.rb:158: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
gems/2.7.0/gems/grape-1.3.3/lib/grape/middleware/base.rb:17: warning: The called method 'initialize' is defined here.

For a middleware subclass of Grape::Middleware::Base, Rack's use method will instantiate it with middleware.new(app, *args, &block), passing the options parameter in *args:

https://github.com/rack/rack/blob/a5e80f01947954af76b14c1d1fdd8e79dd8337f3/lib/rack/builder.rb#L158

My assumption is that, contrary to the suggestion by the deprecation warning, it's not feasible to change Rack to add ** to the call where it passes *args. Plus, Grape's code implies that options really is meant as a positional Hash argument, so I changed it to be one.

The tests pass after this change, including
https://github.com/ruby-grape/grape/blob/v1.3.3/spec/grape/middleware/base_spec.rb.

The main risk I see here is if there's some behavior or usage style that the **options version supported that will now break.

[grape-bot]

	1 Warning
⚠️	There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

[dblock]

Thanks! Can we please 1) add a test that makes sure options are properly assigned 2) fix the changelog

[dblock]

It would be nice if tests failed on warnings, too. If you find out how to enable that part of this PR, I'd appreciate it!

[dim]

I think the backwards-compatible workaround solution would be:

def initialize(app, options = nil)
  @app = app
  @options = default_options.dup
  @options.update(options) if options.is_a?(Hash)
  @app_response = nil
end

This retains the optional argument character of options while resolving the deprecation warning.

[dblock]

And do we need it to be backwards compatible here?

[skarger]

Defaulting the options parameter to an empty hash is backwards compatible. When declaring the parameter with **options, the options variable inside the method will default to an empty hash.

See this example, run with Ruby 2.7.

def example_method(positional, **options)
  puts "  positional: #{positional}"
  puts "  options class: #{options.class}"
  puts "  options: #{options}\n"
end

Given positional argument only:
example_method("arg1")
  positional: arg1
  options class: Hash
  options: {}

Given positional argument followed by hash argument:
example_method("arg1", a_key: "a value")
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Like the existing initialize method in Grape::Middleware::Base, the fact that example_method has options with the ** in front declares it a keyword arguments, not as a (positional) hash argument. However the logic inside Grape::Middleware::Base#initialize treats options as a hash - it just merges it with default_options.

When passing a hash as a positional argument, Ruby 2.7 prints the deprecation warning because it's treating the given hash as keyword arguments in accordance with the method signature:

Given positional argument followed by positional hash:
example_method("arg1", { a_key: "a value" })
test.rb:15: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
test.rb:1: warning: The called method `example_method' is defined here
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Changing the signature to def example_method(positional, options = {}) makes the deprecation warning go away, but otherwise matches the same behavior for all of these examples. This is what I've done in this PR.

[dim]

Actually, I think this needs to be fixed in Rack::Builder. I will submit an issue.

[dim]

Looking at rack/rack@8dad949, I think just using https://github.com/ruby/ruby2_keywords in Middleware::Base may also solve this issue

[skarger]

Looking at rack/rack@8dad949, I think just using https://github.com/ruby/ruby2_keywords in Middleware::Base may also solve this issue

@dim even if that works, I think the existing PR is a better solution, because ruby2_keywords is meant to serve as a bridge for code that can't be converted yet, not to be used permanently.

Given that the logic inside Middleware::Base already treats options just as a Hash argument, it seems reasonable to change the method signature to declare that in the Ruby 3-correct way.

[skarger]

Defaulting the options parameter to an empty hash is backwards compatible. When declaring the parameter with **options, the options variable inside the method will default to an empty hash.

See this example, run with Ruby 2.7.

def example_method(positional, **options)
  puts "  positional: #{positional}"
  puts "  options class: #{options.class}"
  puts "  options: #{options}\n"
end

Given positional argument only:
example_method("arg1")
  positional: arg1
  options class: Hash
  options: {}

Given positional argument followed by hash argument:
example_method("arg1", a_key: "a value")
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Like the existing initialize method in Grape::Middleware::Base, the fact that example_method has options with the ** in front declares it a keyword arguments, not as a (positional) hash argument. However the logic inside Grape::Middleware::Base#initialize treats options as a hash - it just merges it with default_options.

When passing a hash as a positional argument, Ruby 2.7 prints the deprecation warning because it's treating the given hash as keyword arguments in accordance with the method signature:

Given positional argument followed by positional hash:
example_method("arg1", { a_key: "a value" })
test.rb:15: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
test.rb:1: warning: The called method `example_method' is defined here
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Changing the signature to def example_method(positional, options = {}) makes the deprecation warning go away, but otherwise matches the same behavior for all of these examples. This is what I've done in this PR.

[skarger]

Can we please 1) add a test that makes sure options are properly assigned

@dblock the existing tests do that, and they still pass after this change:
https://github.com/ruby-grape/grape/blob/v1.3.3/spec/grape/middleware/base_spec.rb#L127

I'm happy to add another test, if there's some detail not covered in the existing tests that you're thinking about?

[dblock]

The CHANGELOG line is missing a # in the PR number. Danger is nitpicky ;)

[dblock]

I'm satisfied with this, will merge on 🍏

[skarger]

@dblock fixed, thanks for looking.

[dim]

Any chance for a v1.3.4 release?

[dblock]

Please add a +1 to #2030