diff options
20 files changed, 101 insertions, 72 deletions
diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb index 89c3545323..07265be3fe 100644 --- a/actionpack/lib/action_controller/metal/url_for.rb +++ b/actionpack/lib/action_controller/metal/url_for.rb @@ -27,7 +27,7 @@ module ActionController :host => request.host, :port => request.optional_port, :protocol => request.protocol, - :_recall => request.symbolized_path_parameters + :_recall => request.path_parameters }.merge(super).freeze if (same_origin = _routes.equal?(env["action_dispatch.routes".freeze])) || diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index c6a8f581de..e6695ffc90 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -199,7 +199,7 @@ module ActionController value = value.dup end - if extra_keys.include?(key.to_sym) + if extra_keys.include?(key) non_path_parameters[key] = value else if value.is_a?(Array) @@ -208,7 +208,7 @@ module ActionController value = value.to_param end - path_parameters[key.to_s] = value + path_parameters[key] = value end end @@ -583,6 +583,7 @@ module ActionController end parameters, session, flash = args + parameters ||= {} # Ensure that numbers and symbols passed as params are converted to # proper params, as is the case when engaging rack. @@ -601,7 +602,6 @@ module ActionController @request.env['REQUEST_METHOD'] = http_method - parameters ||= {} controller_class_name = @controller.class.anonymous? ? "anonymous" : @controller.class.controller_path @@ -695,7 +695,7 @@ module ActionController :only_path => true, :action => action, :relative_url_root => nil, - :_recall => @request.symbolized_path_parameters) + :_recall => @request.path_parameters) url, query_string = @routes.url_for(options).split("?", 2) @@ -706,7 +706,7 @@ module ActionController end def html_format?(parameters) - return true unless parameters.is_a?(Hash) + return true unless parameters.key?(:format) Mime.fetch(parameters[:format]) { Mime['html'] }.html? end end diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 70e960488e..5b22cd1fcd 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -24,14 +24,13 @@ module ActionDispatch alias :params :parameters def path_parameters=(parameters) #:nodoc: - @symbolized_path_params = nil @env.delete('action_dispatch.request.parameters') @env[Routing::RouteSet::PARAMETERS_KEY] = parameters end # The same as <tt>path_parameters</tt> with explicitly symbolized keys. def symbolized_path_parameters - @symbolized_path_params ||= path_parameters.symbolize_keys + path_parameters end # Returns a hash with the \parameters used to form the \path of the request. diff --git a/actionpack/lib/action_dispatch/journey/router.rb b/actionpack/lib/action_dispatch/journey/router.rb index 3bfa03713d..1632c4490d 100644 --- a/actionpack/lib/action_dispatch/journey/router.rb +++ b/actionpack/lib/action_dispatch/journey/router.rb @@ -20,60 +20,38 @@ module ActionDispatch # :nodoc: VERSION = '2.0.0' - class NullReq # :nodoc: - attr_reader :env - def initialize(env) - @env = env - end - - def request_method - env['REQUEST_METHOD'] - end - - def path_info - env['PATH_INFO'] - end - - def ip - env['REMOTE_ADDR'] - end - - def [](k) - env[k] - end - end - attr_reader :request_class, :formatter attr_accessor :routes def initialize(routes, options) @options = options - @params_key = options[:parameters_key] - @request_class = options[:request_class] || NullReq + @request_class = options[:request_class] @routes = routes end def call(env) - env['PATH_INFO'] = Utils.normalize_path(env['PATH_INFO']) + req = request_class.new(env) - find_routes(env).each do |match, parameters, route| - script_name, path_info, set_params = env.values_at('SCRIPT_NAME', - 'PATH_INFO', - @params_key) + req.path_info = Utils.normalize_path(req.path_info) + + find_routes(req).each do |match, parameters, route| + set_params = req.path_parameters + path_info = req.path_info + script_name = req.script_name unless route.path.anchored - env['SCRIPT_NAME'] = (script_name.to_s + match.to_s).chomp('/') - env['PATH_INFO'] = match.post_match + req.script_name = (script_name.to_s + match.to_s).chomp('/') + req.path_info = match.post_match end - env[@params_key] = (set_params || {}).merge parameters + req.path_parameters = set_params.merge parameters status, headers, body = route.app.call(env) if 'pass' == headers['X-Cascade'] - env['SCRIPT_NAME'] = script_name - env['PATH_INFO'] = path_info - env[@params_key] = set_params + req.script_name = script_name + req.path_info = path_info + req.path_parameters = set_params next end @@ -84,10 +62,12 @@ module ActionDispatch end def recognize(req) - find_routes(req.env).each do |match, parameters, route| + rails_req = request_class.new(req.env) + + find_routes(rails_req).each do |match, parameters, route| unless route.path.anchored - req.env['SCRIPT_NAME'] = match.to_s - req.env['PATH_INFO'] = match.post_match.sub(/^([^\/])/, '/\1') + rails_req.script_name = match.to_s + rails_req.path_info = match.post_match.sub(/^([^\/])/, '/\1') end yield(route, parameters) @@ -124,9 +104,7 @@ module ActionDispatch simulator.memos(path) { [] } end - def find_routes env - req = request_class.new(env) - + def find_routes req routes = filter_routes(req.path_info).concat custom_routes.find_all { |r| r.path.match(req.path_info) } diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 58c7f5330e..7e78b417fa 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -47,7 +47,7 @@ module ActionDispatch private def constraint_args(constraint, request) - constraint.arity == 1 ? [request] : [request.symbolized_path_parameters, request] + constraint.arity == 1 ? [request] : [request.path_parameters, request] end end diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb index b08e62543b..f8ed0cbe6a 100644 --- a/actionpack/lib/action_dispatch/routing/redirection.rb +++ b/actionpack/lib/action_dispatch/routing/redirection.rb @@ -19,13 +19,13 @@ module ActionDispatch # If any of the path parameters has an invalid encoding then # raise since it's likely to trigger errors further on. - req.symbolized_path_parameters.each do |key, value| + req.path_parameters.each do |key, value| unless value.valid_encoding? raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}" end end - uri = URI.parse(path(req.symbolized_path_parameters, req)) + uri = URI.parse(path(req.path_parameters, req)) unless uri.host if relative_path?(uri.path) diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 8c2e821573..8b4fd26ce2 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -303,7 +303,6 @@ module ActionDispatch @set = Journey::Routes.new @router = Journey::Router.new(@set, { - :parameters_key => PARAMETERS_KEY, :request_class => request_class}) @formatter = Journey::Formatter.new @set end @@ -707,8 +706,8 @@ module ActionDispatch params[key] = URI.parser.unescape(value) end end - old_params = env[::ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] - env[::ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] = (old_params || {}).merge(params) + old_params = req.path_parameters + req.path_parameters = old_params.merge params dispatcher = route.app while dispatcher.is_a?(Mapper::Constraints) && dispatcher.matches?(env) do dispatcher = dispatcher.app diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index fbc10baf21..18a5d8b094 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -662,7 +662,7 @@ XML def test_id_converted_to_string get :test_params, :id => 20, :foo => Object.new - assert_kind_of String, @request.path_parameters['id'] + assert_kind_of String, @request.path_parameters[:id] end def test_array_path_parameter_handled_properly @@ -673,17 +673,17 @@ XML end get :test_params, :path => ['hello', 'world'] - assert_equal ['hello', 'world'], @request.path_parameters['path'] - assert_equal 'hello/world', @request.path_parameters['path'].to_param + assert_equal ['hello', 'world'], @request.path_parameters[:path] + assert_equal 'hello/world', @request.path_parameters[:path].to_param end end def test_assert_realistic_path_parameters get :test_params, :id => 20, :foo => Object.new - # All elements of path_parameters should use string keys + # All elements of path_parameters should use Symbol keys @request.path_parameters.keys.each do |key| - assert_kind_of String, key + assert_kind_of Symbol, key end end diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 835abc1c7a..ccc3839212 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3368,12 +3368,13 @@ end class TestAltApp < ActionDispatch::IntegrationTest class AltRequest + attr_accessor :path_parameters, :path_info, :script_name + def initialize(env) + @path_parameters = {} @env = env - end - - def path_info - "/" + @path_info = "/" + @script_name = "" end def request_method diff --git a/actionpack/test/journey/router_test.rb b/actionpack/test/journey/router_test.rb index 9a8d644f7b..b95f4c169b 100644 --- a/actionpack/test/journey/router_test.rb +++ b/actionpack/test/journey/router_test.rb @@ -12,7 +12,9 @@ module ActionDispatch def setup @app = StubDispatcher.new @routes = Routes.new - @router = Router.new(@routes, {}) + @router = Router.new(@routes, { + :request_class => ActionDispatch::Request + }) @formatter = Formatter.new(@routes) end @@ -39,7 +41,7 @@ module ActionDispatch end def test_dashes - router = Router.new(routes, {}) + router = Router.new(routes, { :request_class => ActionDispatch::Request }) exp = Router::Strexp.new '/foo-bar-baz', {}, ['/.?'] path = Path::Pattern.new exp @@ -55,7 +57,7 @@ module ActionDispatch end def test_unicode - router = Router.new(routes, {}) + router = Router.new(routes, { :request_class => ActionDispatch::Request }) #match the escaped version of /ほげ exp = Router::Strexp.new '/%E3%81%BB%E3%81%92', {}, ['/.?'] @@ -111,10 +113,14 @@ module ActionDispatch assert_equal env.env, klass.env end - class CustomPathRequest < Router::NullReq + class CustomPathRequest < ActionDispatch::Request def path_info env['custom.path_info'] end + + def path_info=(x) + env['custom.path_info'] = x + end end def test_request_class_overrides_path_info diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3dd973d64b..260bd063aa 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fixed serialization for records with an attribute named `format`. + + Fixes #15188. + + *Godfrey Chan* + * When a `group` is set, `sum`, `size`, `average`, `minimum` and `maximum` on a NullRelation should return a Hash. diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 8bd51dc71f..b9141b9b33 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -66,6 +66,7 @@ module ActiveRecord # Generates all the attribute related methods for columns in the database # accessors, mutators and query methods. def define_attribute_methods # :nodoc: + return false if @attribute_methods_generated # Use a mutex; we don't want two thread simultaneously trying to define # attribute methods. generated_attribute_methods.synchronize do @@ -200,6 +201,7 @@ module ActiveRecord # this is probably horribly slow, but should only happen at most once for a given AR class attribute_method.bind(self).call(*args, &block) else + return super unless respond_to_missing?(method, true) send(method, *args, &block) end else diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 4571cc0786..07eafef788 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -286,6 +286,8 @@ module ActiveRecord @new_record = false + self.class.define_attribute_methods + run_callbacks :find run_callbacks :initialize diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 416f2305d2..a607e9ac87 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -950,7 +950,6 @@ WARNING [@klass.send(:sanitize_sql, other.empty? ? opts : ([opts] + other))] when Hash opts = PredicateBuilder.resolve_column_aliases(klass, opts) - attributes = @klass.send(:expand_hash_conditions_for_aggregates, opts) bv_len = bind_values.length tmp_opts, bind_values = create_binds(opts, bv_len) diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index 495b43c9e5..4c96c2f4fd 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -843,6 +843,18 @@ class AttributeMethodsTest < ActiveRecord::TestCase assert_equal !real_topic.title?, klass.find(real_topic.id).title? end + def test_calling_super_when_parent_does_not_define_method_raises_error + klass = new_topic_like_ar_class do + def some_method_that_is_not_on_super + super + end + end + + assert_raise(NoMethodError) do + klass.new.some_method_that_is_not_on_super + end + end + private def new_topic_like_ar_class(&block) diff --git a/activerecord/test/cases/serialization_test.rb b/activerecord/test/cases/serialization_test.rb index c46060a646..7dd1f10ce9 100644 --- a/activerecord/test/cases/serialization_test.rb +++ b/activerecord/test/cases/serialization_test.rb @@ -1,8 +1,11 @@ require "cases/helper" require 'models/contact' require 'models/topic' +require 'models/book' class SerializationTest < ActiveRecord::TestCase + fixtures :books + FORMATS = [ :xml, :json ] def setup @@ -65,4 +68,20 @@ class SerializationTest < ActiveRecord::TestCase ensure ActiveRecord::Base.include_root_in_json = original_root_in_json end + + def test_read_attribute_for_serialization_with_format_after_init + klazz = Class.new(ActiveRecord::Base) + klazz.table_name = 'books' + + book = klazz.new(format: 'paperback') + assert_equal 'paperback', book.read_attribute_for_serialization(:format) + end + + def test_read_attribute_for_serialization_with_format_after_find + klazz = Class.new(ActiveRecord::Base) + klazz.table_name = 'books' + + book = klazz.find(books(:awdr).id) + assert_equal 'paperback', book.read_attribute_for_serialization(:format) + end end diff --git a/activerecord/test/fixtures/books.yml b/activerecord/test/fixtures/books.yml index fb48645456..abe56752c6 100644 --- a/activerecord/test/fixtures/books.yml +++ b/activerecord/test/fixtures/books.yml @@ -2,8 +2,10 @@ awdr: author_id: 1 id: 1 name: "Agile Web Development with Rails" + format: "paperback" rfr: author_id: 1 id: 2 name: "Ruby for Rails" + format: "ebook" diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 8c52ad2724..c15ee5022e 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -103,6 +103,7 @@ ActiveRecord::Schema.define do create_table :books, force: true do |t| t.integer :author_id + t.string :format t.column :name, :string t.column :status, :integer, default: 0 t.column :read_status, :integer, default: 0 diff --git a/activesupport/test/subscriber_test.rb b/activesupport/test/subscriber_test.rb index 8be8c51f07..21e4ba0cee 100644 --- a/activesupport/test/subscriber_test.rb +++ b/activesupport/test/subscriber_test.rb @@ -23,6 +23,7 @@ end # Monkey patch subscriber to test that only one subscriber per method is added. class TestSubscriber + remove_method :open_party def open_party(event) events << event end diff --git a/guides/source/active_record_querying.md b/guides/source/active_record_querying.md index 4b3d95a3e1..697ddd70cb 100644 --- a/guides/source/active_record_querying.md +++ b/guides/source/active_record_querying.md @@ -753,13 +753,15 @@ Article.find(10).comments.reorder('name') The SQL that would be executed: ```sql -SELECT * FROM articles WHERE id = 10 ORDER BY name +SELECT * FROM articles WHERE id = 10 +SELECT * FROM comments WHERE article_id = 10 ORDER BY name ``` In case the `reorder` clause is not used, the SQL executed would be: ```sql -SELECT * FROM articles WHERE id = 10 ORDER BY posted_at DESC +SELECT * FROM articles WHERE id = 10 +SELECT * FROM comments WHERE article_id = 10 ORDER BY posted_at DESC ``` ### `reverse_order` |