ActionDispatch request helpers don't properly rewrite positional arguments to keyword arguments

[derekprior]

The code that rewrites deprecated positional argument version of get, post, put, etc does not properly rewrite positional arguments to keyword arguments. This appears to be an issue with nested hashes of params.

Here is the bug template to reproduce this:

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'rails', github: 'rails/rails'
  gem 'arel', github: 'rails/arel'
  gem 'rack', github: 'rack/rack'
  gem 'sprockets', github: 'rails/sprockets'
  gem 'sprockets-rails', github: 'rails/sprockets-rails'
  gem 'sass-rails', github: 'rails/sass-rails'
end

require 'action_controller/railtie'

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: 'cookie_store_key'
  secrets.secret_token    = 'secret_token'
  secrets.secret_key_base = 'secret_key_base'

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    resources :test, only: [:index]
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    render plain: 'Home'

    if params[:session][:password].nil?
      raise "parameter not present"
    end
  end
end

puts Rails.application.routes

require 'minitest/autorun'
require 'rack/test'

class TestControllerTest < ActionController::TestCase
  def setup
    @routes = Rails.application.routes
  end

  def test_with_positional
    get :index, session: { password: "foo" }
  end

  def test_with_kwargs
    get :index, params: { session: { password: "foo" } }
  end

  private
    def app
      Rails.application
    end
end

The test_with_positional method will blow up because params[:session] is nil in the controller.

[maclover7]

Looking into this.

[diego-silva]

It looks to me that its working as designed, since params session flash method body xhr are all valid kwargs according to the new syntax.

So there is actually no way to now that you were in fact wanting to use the deprecated syntax...

[maclover7]

I think this is an error on account of the test -- if you look at the first test, you are passing in session[:password], but in the second test, you are passing in params[:session][:password]. Based on your above tests, it seems like these variables are completely different (session and params[:session] wouldn't ever be equal), and nothing is behaving out of the ordinary here.

[maclover7]

Side note: ActionController::TestCase and ActionDispatch::IntegrationTest are meant to have the same process_with_kwargs methods, but there is a slight deviation which could be responsible for this. If you look here, the behavior is a little different regarding non-kwargs arguments -- the method places whatever's in the first argument inside params. I feel like this may be the culprit, but if I change the behavior in ActionController::TestCase to match that, I get a bunch of other test failures 😬

[maclover7]

Also, cc @kirs -- this behavior was introduced via #18323.

[derekprior]

I think this is an error on account of the test -- if you look at the first test, you are passing in session[:password], but in the second test, you are passing in params[:session][:password]

In Rails 4.2 and older, session: { password: "foo" } would be interpreted as the params because positional arguments were used. If I were to use params: { session: { password: "foo" } } as the argument in Rails 4.2, that entire thing would be interpreted as the params hash, so in the controller, accessing the password would be: params[:params][:session][:password].

and nothing is behaving out of the ordinary here

Note that the first test passes in Rails 4.2, 4.1, 4.0, and 3.2 but fails in Rails 5.

[maclover7]

Ahh, that makes more sense now -- I apologize -- I don't think I was fully understanding what you were saying before. :( I think the original PR that introduced this behavior was too aggressive in ripping out the old behavior. I think a partial reverting of the PR may be in order, but I'm not really sure how we'll be able to tell if the session key you are passing is for the kwargs side of things, or whether it's for the params hash...

[senny]

This is a tough one. Don't think there's much we can do about it. The two APIs conflict in this regard and we won't be able to tell the difference. Reverting would mean a full revert... I don't see a partial solution that expands on the existing method signatures.

@rafaelfranca any thoughts?

[derekprior]

I can't think of a great solution here either. In my library I've overridden the http methods to explicitly use positional arguments and then rewrite those to keyword arguments myself.

It's a corner case, for sure -- it seems to only crop up if you're using a parameter name that maps to one of the new keyword argument names -- but I'd be tempted to remove the deprecation and make it a breaking change all-together so people have to make the move to kwargs in their tests.

In that case, I'd say maybe rails-deprecated-http-actions or something like that would be in order as a gem supported until 5.1.

[maclover7]

^ working on an extraction gem for this.

[rafaelfranca]

For this specific case I'm fine with not working.

but I'd be tempted to remove the deprecation and make it a breaking change all-together so people have to make the move to kwargs in their tests.

We consider deprecated features already a breaking change, this is why we only add them in minor versions. The deprecation is there only to help you with the upgrade path but you must change your code.

I understand that in this case it is for a gem that want to maintain compatibility with old Rails versions and I agree it is not good to drop support for old Rails versions only because some tests but removing deprecation only because this case or extracting a gem that we would have to maintain I believe it is too much.