From f886fe2d8ccc900cde2629577e5c0be8c7d4c67f Mon Sep 17 00:00:00 2001 From: Guillermo Iguaran Date: Sat, 2 Nov 2013 14:30:03 -0500 Subject: [PATCH 1/5] Revert "Merge pull request #9660 from sebasoga/change_strong_parameters_require_behaviour" This reverts commit c2b5a8e61ba0f35015e6ac949a5c8fce2042a1f2, reversing changes made to 1918b12c0429caec2a6134ac5e5b42ade103fe90. See: https://github.com/rails/rails/pull/9660#issuecomment-27627493 --- .../metal/strong_parameters.rb | 32 ++++++------------- .../middleware/exception_wrapper.rb | 3 +- .../parameters/parameters_require_test.rb | 8 +---- .../test/controller/required_params_test.rb | 19 ----------- .../test/dispatch/debug_exceptions_test.rb | 6 ---- 5 files changed, 11 insertions(+), 57 deletions(-) diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 8ae7e474a3..fcc76f6225 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -10,6 +10,8 @@ module ActionController # params = ActionController::Parameters.new(a: {}) # params.fetch(:b) # # => ActionController::ParameterMissing: param not found: b + # params.require(:a) + # # => ActionController::ParameterMissing: param not found: a class ParameterMissing < KeyError attr_reader :param # :nodoc: @@ -19,20 +21,6 @@ module ActionController end end - # Raised when a required parameter value is empty. - # - # params = ActionController::Parameters.new(a: {}) - # params.require(:a) - # # => ActionController::EmptyParameter: value is empty for required key: a - class EmptyParameter < IndexError - attr_reader :param - - def initialize(param) - @param = param - super("value is empty for required key: #{param}") - end - end - # Raised when a supplied parameter is not expected. # # params = ActionController::Parameters.new(a: "123", b: "456") @@ -169,22 +157,20 @@ module ActionController self end - # Ensures that a parameter is present. If it's present and not empty, - # returns the parameter at the given +key+, if it's empty raises - # an ActionController::EmptyParameter error, otherwise - # raises an ActionController::ParameterMissing error. + # Ensures that a parameter is present. If it's present, returns + # the parameter at the given +key+, otherwise raises an + # ActionController::ParameterMissing error. # # ActionController::Parameters.new(person: { name: 'Francesco' }).require(:person) # # => {"name"=>"Francesco"} # - # ActionController::Parameters.new(person: {}).require(:person) - # # => ActionController::EmptyParameter: value is empty for required key: person + # ActionController::Parameters.new(person: nil).require(:person) + # # => ActionController::ParameterMissing: param not found: person # - # ActionController::Parameters.new(name: {}).require(:person) + # ActionController::Parameters.new(person: {}).require(:person) # # => ActionController::ParameterMissing: param not found: person def require(key) - raise(ActionController::ParameterMissing.new(key)) unless self.key?(key) - self[key].presence || raise(ActionController::EmptyParameter.new(key)) + self[key].presence || raise(ParameterMissing.new(key)) end # Alias of #require. diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb index daddaccadd..37bf9c8c9f 100644 --- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb +++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb @@ -15,8 +15,7 @@ module ActionDispatch 'ActionController::InvalidAuthenticityToken' => :unprocessable_entity, 'ActionDispatch::ParamsParser::ParseError' => :bad_request, 'ActionController::BadRequest' => :bad_request, - 'ActionController::ParameterMissing' => :bad_request, - 'ActionController::EmptyParameter' => :bad_request + 'ActionController::ParameterMissing' => :bad_request ) cattr_accessor :rescue_templates diff --git a/actionpack/test/controller/parameters/parameters_require_test.rb b/actionpack/test/controller/parameters/parameters_require_test.rb index 21b3eaa6b5..bdaba8d2d8 100644 --- a/actionpack/test/controller/parameters/parameters_require_test.rb +++ b/actionpack/test/controller/parameters/parameters_require_test.rb @@ -2,14 +2,8 @@ require 'abstract_unit' require 'action_controller/metal/strong_parameters' class ParametersRequireTest < ActiveSupport::TestCase - test "required parameters must be present" do + test "required parameters must be present not merely not nil" do assert_raises(ActionController::ParameterMissing) do - ActionController::Parameters.new(name: {}).require(:person) - end - end - - test "required parameters can't be blank" do - assert_raises(ActionController::EmptyParameter) do ActionController::Parameters.new(person: {}).require(:person) end end diff --git a/actionpack/test/controller/required_params_test.rb b/actionpack/test/controller/required_params_test.rb index b27069140b..343d57c300 100644 --- a/actionpack/test/controller/required_params_test.rb +++ b/actionpack/test/controller/required_params_test.rb @@ -5,11 +5,6 @@ class BooksController < ActionController::Base params.require(:book).require(:name) head :ok end - - def update - params.require(:book) - head :ok - end end class ActionControllerRequiredParamsTest < ActionController::TestCase @@ -25,20 +20,6 @@ class ActionControllerRequiredParamsTest < ActionController::TestCase end end - test "empty required parameters will raise an exception" do - assert_raise ActionController::EmptyParameter do - put :update, {book: {}} - end - - assert_raise ActionController::EmptyParameter do - put :update, {book: ''} - end - - assert_raise ActionController::EmptyParameter do - put :update, {book: nil} - end - end - test "required parameters that are present will not raise" do post :create, { book: { name: "Mjallo!" } } assert_response :ok diff --git a/actionpack/test/dispatch/debug_exceptions_test.rb b/actionpack/test/dispatch/debug_exceptions_test.rb index 6e28e4a982..3045a07ad6 100644 --- a/actionpack/test/dispatch/debug_exceptions_test.rb +++ b/actionpack/test/dispatch/debug_exceptions_test.rb @@ -43,8 +43,6 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest raise ActionController::UrlGenerationError, "No route matches" when "/parameter_missing" raise ActionController::ParameterMissing, :missing_param_key - when "/required_key_empty_value" - raise ActionController::EmptyParameter, :empty_param_key else raise "puke!" end @@ -128,10 +126,6 @@ class DebugExceptionsTest < ActionDispatch::IntegrationTest get "/parameter_missing", {}, {'action_dispatch.show_exceptions' => true} assert_response 400 assert_match(/ActionController::ParameterMissing/, body) - - get "/required_key_empty_value", {}, {'action_dispatch.show_exceptions' => true} - assert_response 400 - assert_match(/ActionController::EmptyParameter/, body) end test "rescue with text error for xhr request" do From e474e47bfaefea2c2f72cb2317dcdfc372cae484 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Sat, 2 Nov 2013 17:30:23 -0200 Subject: [PATCH 2/5] Update guides welcome page to point to version 3.2.15 [ci skip] --- guides/source/_welcome.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guides/source/_welcome.html.erb b/guides/source/_welcome.html.erb index 0a0a958e30..93c177905c 100644 --- a/guides/source/_welcome.html.erb +++ b/guides/source/_welcome.html.erb @@ -15,7 +15,7 @@

<% end %>

- The guides for Rails 3.2.x are available at http://guides.rubyonrails.org/v3.2.14/. + The guides for Rails 3.2.x are available at http://guides.rubyonrails.org/v3.2.15/.

The guides for Rails 2.3.x are available at http://guides.rubyonrails.org/v2.3.11/. From e3e7786b0fde0c7807608e4d51c63e97a2a59550 Mon Sep 17 00:00:00 2001 From: Guillermo Iguaran Date: Sat, 2 Nov 2013 14:39:40 -0500 Subject: [PATCH 3/5] Improve wording in AC::ParameterMissing error message --- actionpack/lib/action_controller/metal/strong_parameters.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index fcc76f6225..b4948d99a8 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -17,7 +17,7 @@ module ActionController def initialize(param) # :nodoc: @param = param - super("param not found: #{param}") + super("param is missing or the value is empty: #{param}") end end From 6963e8959f67029903e305d29c2856a481418065 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Sat, 2 Nov 2013 18:08:36 -0200 Subject: [PATCH 4/5] Fix to work on Ruby 1.9.3, example and changelog improvements --- activerecord/CHANGELOG.md | 44 +++++++++++++------------- activerecord/lib/active_record/enum.rb | 10 +++--- activerecord/test/cases/enum_test.rb | 6 ++-- activerecord/test/models/book.rb | 4 +-- 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5716cc4b5d..16376b0b89 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,27 +1,27 @@ -* Added ActiveRecord::Base#enum for declaring enum attributes where the values map to integers in the database, but can be queried by name. +* Added ActiveRecord::Base#enum for declaring enum attributes where the values map to integers in the database, but can be queried by name. - Example: - class Conversation < ActiveRecord::Base - enum status: %i( active archived ) - end - - Conversation::STATUS # => { active: 0, archived: 1 } - - # conversation.update! status: 0 - conversation.active! - conversation.active? # => true - conversation.status # => :active - - # conversation.update! status: 1 - conversation.archived! - conversation.archived? # => true - conversation.status # => :archived - - # conversation.update! status: 1 - conversation.status = :archived + Example: - *DHH* + class Conversation < ActiveRecord::Base + enum status: [:active, :archived] + end + Conversation::STATUS # => { active: 0, archived: 1 } + + # conversation.update! status: 0 + conversation.active! + conversation.active? # => true + conversation.status # => :active + + # conversation.update! status: 1 + conversation.archived! + conversation.archived? # => true + conversation.status # => :archived + + # conversation.update! status: 1 + conversation.status = :archived + + *DHH* * ActiveRecord::Base#attribute_for_inspect now truncates long arrays (more than 10 elements) @@ -35,6 +35,7 @@ Fixed bug when providing `includes` in through association scope, and fetching targets. Example: + class Vendor < ActiveRecord::Base has_many :relationships, -> { includes(:user) } has_many :users, through: :relationships @@ -50,7 +51,6 @@ vendor.users.to_a # => No exception is raised - Fixes #12242, #9517, #10240. *Paul Nikitochkin* diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 13b843ff4f..60af6b4178 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -2,7 +2,7 @@ module ActiveRecord # Declare an enum attribute where the values map to integers in the database, but can be queried by name. Example: # # class Conversation < ActiveRecord::Base - # enum status: %i( active archived ) + # enum status: [:active, :archived] # end # # Conversation::STATUS # => { active: 0, archived: 1 } @@ -11,21 +11,21 @@ module ActiveRecord # conversation.active! # conversation.active? # => true # conversation.status # => :active - # + # # # conversation.update! status: 1 # conversation.archived! # conversation.archived? # => true # conversation.status # => :archived - # + # # # conversation.update! status: 1 # conversation.status = :archived # # You can set the default value from the database declaration, like: # - # create_table :conversation do + # create_table :conversations do |t| # t.column :status, :integer, default: 0 # end - # + # # Good practice is to let the first declared status be the default. module Enum def enum(definitions) diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 9855261e4d..7f5ddc92d4 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -13,7 +13,7 @@ class StoreTest < ActiveRecord::TestCase assert_not @book.written? assert_not @book.published? end - + test "query state with symbol" do assert_equal :proposed, @book.status end @@ -22,12 +22,12 @@ class StoreTest < ActiveRecord::TestCase @book.written! assert @book.written? end - + test "update by setter" do @book.update! status: :written assert @book.written? end - + test "constant" do assert_equal 0, Book::STATUS[:proposed] assert_equal 1, Book::STATUS[:written] diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index 997c088176..a527e41a8a 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -6,6 +6,6 @@ class Book < ActiveRecord::Base has_many :subscriptions has_many :subscribers, through: :subscriptions - - enum status: %i( proposed written published ) + + enum status: [:proposed, :written, :published] end From a2f27385f60292c77e5e29a9568f1888a7c3d214 Mon Sep 17 00:00:00 2001 From: Carlos Antonio da Silva Date: Sat, 2 Nov 2013 18:13:02 -0200 Subject: [PATCH 5/5] Use an already existing fixture --- activerecord/test/cases/enum_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activerecord/test/cases/enum_test.rb b/activerecord/test/cases/enum_test.rb index 7f5ddc92d4..ad8494d9a1 100644 --- a/activerecord/test/cases/enum_test.rb +++ b/activerecord/test/cases/enum_test.rb @@ -5,7 +5,7 @@ class StoreTest < ActiveRecord::TestCase fixtures :books setup do - @book = Book.create! name: 'REMOTE' + @book = books(:awdr) end test "query state by predicate" do