Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

3.1: params_wrapper raises error on anynomous controllers #1090

Merged
merged 4 commits into from

3 participants

@dchelimsky

Given this code:

Class.new(ApplicationController)

I get the following error:

+/Users/david/.rvm/gems/ruby-1.9.2-p180@rspec-dev/gems/activesupport-3.1.0.beta1/lib/active_support/whiny_nil.rb:48:in `method_missing': undefined method `sub' for nil:NilClass (NoMethodError)
+   from /Users/david/.rvm/gems/ruby-1.9.2-p180@rspec-dev/gems/actionpack-3.1.0.beta1/lib/action_controller/metal/params_wrapper.rb:140:in `_default_wrap_model'

This is caused by model_name = self.name.sub(/Controller$/, '').singularize on https://github.com/rails/rails/blob/8b0262f9535cb9ad2215e5ed672150e2c52cb4b4/actionpack/lib/action_controller/metal/params_wrapper.rb#L143

I've added a failing test case, and will offer up a solution in the next day or so.

@alindeman

What's the expected behavior here? Should param wrapping simply be ignored?

@josevalim
Owner

@alindeman we should just not try to wrap things automatically then. you would have to be explicit.

@alindeman

Awesome @dchelimsky, would love to get this merged for beta2. Adding a comment because GitHub doesn't notify pull request participants simply when new commits are added.

@josevalim josevalim merged commit f1e1e76 into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
6 actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -140,6 +140,8 @@ def inherited(klass)
# This method also does namespace lookup. Foo::Bar::UsersController will
# try to find Foo::Bar::User, Foo::User and finally User.
def _default_wrap_model #:nodoc:
+ return nil if self.name.nil?
+
model_name = self.name.sub(/Controller$/, '').singularize
begin
@@ -168,7 +170,7 @@ def _set_wrapper_defaults(options, model=nil)
end
end
- unless options[:name]
+ unless options[:name] || self.name.nil?
model ||= _default_wrap_model
options[:name] = model ? model.to_s.demodulize.underscore :
controller_name.singularize
@@ -226,7 +228,7 @@ def _wrap_parameters(parameters)
# Checks if we should perform parameters wrapping.
def _wrapper_enabled?
ref = request.content_mime_type.try(:ref)
- _wrapper_formats.include?(ref) && !request.request_parameters[_wrapper_key]
+ _wrapper_formats.include?(ref) && _wrapper_key && !request.request_parameters[_wrapper_key]
end
end
end
View
6 actionpack/lib/action_controller/test_case.rb
@@ -413,7 +413,11 @@ def process(action, parameters = nil, session = nil, flash = nil, http_method =
@request.env['REQUEST_METHOD'] = http_method
parameters ||= {}
- @request.assign_parameters(@routes, @controller.class.name.underscore.sub(/_controller$/, ''), action.to_s, parameters)
+ controller_class_name = @controller.class.name ?
+ @controller.class.name.underscore.sub(/_controller$/, '') :
+ "anonymous_controller"
+
+ @request.assign_parameters(@routes, controller_class_name, action.to_s, parameters)
@request.session = ActionController::TestSession.new(session) if session
@request.session["flash"] = @request.flash.update(flash || {})
View
63 actionpack/test/controller/params_wrapper_test.rb
@@ -2,7 +2,21 @@
module Admin; class User; end; end
+module ParamsWrapperTestHelp
+ def with_default_wrapper_options(&block)
+ @controller.class._wrapper_options = {:format => [:json]}
+ @controller.class.inherited(@controller.class)
+ yield
+ end
+
+ def assert_parameters(expected)
+ assert_equal expected, self.class.controller_class.last_parameters
+ end
+end
+
class ParamsWrapperTest < ActionController::TestCase
+ include ParamsWrapperTestHelp
+
class UsersController < ActionController::Base
class << self
attr_accessor :last_parameters
@@ -166,20 +180,11 @@ def test_not_wrapping_abstract_model
assert_parameters({ 'username' => 'sikachu', 'title' => 'Developer', 'user' => { 'username' => 'sikachu', 'title' => 'Developer' }})
end
end
-
- private
- def with_default_wrapper_options(&block)
- @controller.class._wrapper_options = {:format => [:json]}
- @controller.class.inherited(@controller.class)
- yield
- end
-
- def assert_parameters(expected)
- assert_equal expected, UsersController.last_parameters
- end
end
class NamespacedParamsWrapperTest < ActionController::TestCase
+ include ParamsWrapperTestHelp
+
module Admin
module Users
class UsersController < ActionController::Base;
@@ -247,14 +252,36 @@ def test_hierarchy_namespace_lookup_from_model
end
end
- private
- def with_default_wrapper_options(&block)
- @controller.class._wrapper_options = {:format => [:json]}
- @controller.class.inherited(@controller.class)
- yield
+end
+
+class AnonymousControllerParamsWrapperTest < ActionController::TestCase
+ include ParamsWrapperTestHelp
+
+ tests(Class.new(ActionController::Base) do
+ class << self
+ attr_accessor :last_parameters
end
- def assert_parameters(expected)
- assert_equal expected, Admin::Users::UsersController.last_parameters
+ def parse
+ self.class.last_parameters = request.params.except(:controller, :action)
+ head :ok
+ end
+ end)
+
+ def test_does_not_implicitly_wrap_params
+ with_default_wrapper_options do
+ @request.env['CONTENT_TYPE'] = 'application/json'
+ post :parse, { 'username' => 'sikachu' }
+ assert_parameters({ 'username' => 'sikachu' })
end
+ end
+
+ def test_does_wrap_params_if_name_provided
+ with_default_wrapper_options do
+ @controller.class.wrap_parameters(:name => "guest")
+ @request.env['CONTENT_TYPE'] = 'application/json'
+ post :parse, { 'username' => 'sikachu' }
+ assert_parameters({ 'username' => 'sikachu', 'guest' => { 'username' => 'sikachu' }})
+ end
+ end
end
Something went wrong with that request. Please try again.