From 7e872091e104be09deb8b39ec73ab4792a44d4e9 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 24 Feb 2020 19:17:38 -0800 Subject: [PATCH 01/44] pull template keys up --- .../action_view/renderer/partial_renderer.rb | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index b008b1fdd2..5150d7df53 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -296,12 +296,21 @@ module ActionView setup(context, options, as, block) if @path + @variable = nil + @variable_counter = nil + @variable_iteration = nil + @template_keys = @locals.keys + if @has_object || @collection @variable, @variable_counter, @variable_iteration = retrieve_variable(@path, as) - @template_keys = retrieve_template_keys(@variable) - else - @template_keys = @locals.keys + @template_keys << @variable + + if @collection + @template_keys << @variable_counter + @template_keys << @variable_iteration + end end + template = find_template(@path, @template_keys) @variable ||= template.variable else @@ -517,16 +526,6 @@ module ActionView end end - def retrieve_template_keys(variable) - keys = @locals.keys - keys << variable - if @collection - keys << @variable_counter - keys << @variable_iteration - end - keys - end - def retrieve_variable(path, as) variable = as || begin base = path[-1] == "/" ? "" : File.basename(path) From dc1633090e6689d53e688c7bc8357ac3b31f4c70 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 24 Feb 2020 19:43:28 -0800 Subject: [PATCH 02/44] extract template keys to a method --- .../action_view/renderer/partial_renderer.rb | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 5150d7df53..e8faef27a3 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -291,27 +291,28 @@ module ActionView @context_prefix = @lookup_context.prefixes.first end + def template_keys + if @has_object || @collection + @locals.keys + retrieve_variable(@path, @as) + else + @locals.keys + end + end + def render(context, options, block) - as = as_variable(options) + as = @as = as_variable(options) setup(context, options, as, block) if @path @variable = nil @variable_counter = nil @variable_iteration = nil - @template_keys = @locals.keys if @has_object || @collection @variable, @variable_counter, @variable_iteration = retrieve_variable(@path, as) - @template_keys << @variable - - if @collection - @template_keys << @variable_counter - @template_keys << @variable_iteration - end end - template = find_template(@path, @template_keys) + template = find_template(@path, template_keys) @variable ||= template.variable else if options[:cached] @@ -357,7 +358,7 @@ module ActionView object, as = @object, @variable if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, @template_keys) + layout = find_template(layout.to_s, template_keys) end object = locals[as] if object.nil? # Respect object when object is false @@ -443,7 +444,7 @@ module ActionView as, counter, iteration = @variable, @variable_counter, @variable_iteration if layout = @options[:layout] - layout = find_template(layout, @template_keys) + layout = find_template(layout, template_keys) end partial_iteration = PartialIteration.new(@collection.size) From 96f89fbf5dd345116ead5e88fa99723985c7a4ff Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 10:05:42 -0800 Subject: [PATCH 03/44] collection with/without template are more similar --- .../action_view/renderer/partial_renderer.rb | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index e8faef27a3..461976fd0b 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -300,20 +300,12 @@ module ActionView end def render(context, options, block) - as = @as = as_variable(options) - setup(context, options, as, block) + @as = as_variable(options) + setup(context, options, @as, block) if @path - @variable = nil - @variable_counter = nil - @variable_iteration = nil - - if @has_object || @collection - @variable, @variable_counter, @variable_iteration = retrieve_variable(@path, as) - end - template = find_template(@path, template_keys) - @variable ||= template.variable + @variable = template.variable else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -395,6 +387,11 @@ module ActionView @object = options[:object] @collection = collection_from_options @path = partial + + if @collection + paths = @collection_data = @collection.map { |o| @path } + paths.map! { |path| retrieve_variable(path, as).unshift(path) } + end else @has_object = true @object = partial @@ -404,6 +401,7 @@ module ActionView paths = @collection_data = @collection.map { |o| partial_path(o, context) } if paths.uniq.length == 1 @path = paths.first + paths.map! { |path| retrieve_variable(path, as).unshift(path) } else paths.map! { |path| retrieve_variable(path, as).unshift(path) } @path = nil @@ -440,19 +438,21 @@ module ActionView end def collection_with_template(view, template) - locals = @locals - as, counter, iteration = @variable, @variable_counter, @variable_iteration + locals, collection_data = @locals, @collection_data if layout = @options[:layout] layout = find_template(layout, template_keys) end partial_iteration = PartialIteration.new(@collection.size) - locals[iteration] = partial_iteration @collection.map do |object| + index = partial_iteration.index + path, as, counter, iteration = collection_data[index] + locals[as] = object - locals[counter] = partial_iteration.index + locals[counter] = index + locals[iteration] = partial_iteration content = template.render(view, locals) content = layout.render(view, locals) { content } if layout From 867c4c2e601ea204a59ed020e440df9b58af802f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 10:19:19 -0800 Subject: [PATCH 04/44] collection with/without template are the same --- .../action_view/renderer/partial_renderer.rb | 30 ++++--------------- .../collection_cache_association_loading.rb | 5 ---- 2 files changed, 5 insertions(+), 30 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 461976fd0b..e3f54e0603 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -338,7 +338,7 @@ module ActionView collection_with_template(view, template) end else - collection_without_template(view) + collection_with_template(view, nil) end build_rendered_collection(collection_body, spacer) end @@ -439,6 +439,7 @@ module ActionView def collection_with_template(view, template) locals, collection_data = @locals, @collection_data + cache = {} if layout = @options[:layout] layout = find_template(layout, template_keys) @@ -454,32 +455,11 @@ module ActionView locals[counter] = index locals[iteration] = partial_iteration - content = template.render(view, locals) + _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) + content = _template.render(view, locals) content = layout.render(view, locals) { content } if layout partial_iteration.iterate! - build_rendered_template(content, template) - end - end - - def collection_without_template(view) - locals, collection_data = @locals, @collection_data - cache = {} - keys = @locals.keys - - partial_iteration = PartialIteration.new(@collection.size) - - @collection.map do |object| - index = partial_iteration.index - path, as, counter, iteration = collection_data[index] - - locals[as] = object - locals[counter] = index - locals[iteration] = partial_iteration - - template = (cache[path] ||= find_template(path, keys + [as, counter, iteration])) - content = template.render(view, locals) - partial_iteration.iterate! - build_rendered_template(content, template) + build_rendered_template(content, _template) end end diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index 6d1bc8e3c4..f529f601bc 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -22,11 +22,6 @@ module ActiveRecord end end - def collection_without_template(*) - @relation.preload_associations(@collection) if @relation - super - end - def collection_with_template(*) @relation.preload_associations(@collection) if @relation super From 64b3bcb0b9dd4f778cf84eb41c9ea321c7be2417 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 10:26:43 -0800 Subject: [PATCH 05/44] block does not need to be an ivar --- .../lib/action_view/renderer/partial_renderer.rb | 11 +++++------ .../railties/collection_cache_association_loading.rb | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index e3f54e0603..bede112a36 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -301,7 +301,7 @@ module ActionView def render(context, options, block) @as = as_variable(options) - setup(context, options, @as, block) + setup(context, options, @as) if @path template = find_template(@path, template_keys) @@ -316,7 +316,7 @@ module ActionView if @collection render_collection(context, template) else - render_partial(context, template) + render_partial(context, template, block) end end @@ -344,9 +344,9 @@ module ActionView end end - def render_partial(view, template) + def render_partial(view, template, block) instrument(:partial, identifier: template.identifier) do |payload| - locals, block = @locals, @block + locals = @locals object, as = @object, @variable if !block && (layout = @options[:layout]) @@ -373,9 +373,8 @@ module ActionView # If +options[:partial]+ is a string, then the @path instance variable is # set to that string. Otherwise, the +options[:partial]+ object must # respond to +to_partial_path+ in order to set up the path. - def setup(context, options, as, block) + def setup(context, options, as) @options = options - @block = block @locals = options[:locals] || {} @details = extract_details(options) diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index f529f601bc..a8cc6fe9ae 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -3,7 +3,7 @@ module ActiveRecord module Railties # :nodoc: module CollectionCacheAssociationLoading #:nodoc: - def setup(context, options, as, block) + def setup(context, options, as) @relation = nil return super unless options[:cached] From 99d74cac990981357ae902bcffd38869cae05405 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 13:31:49 -0800 Subject: [PATCH 06/44] Don't rely on the collection ivar We're going to turn this in to a more complex iterator --- .../lib/action_view/renderer/partial_renderer.rb | 12 ++++++------ .../partial_renderer/collection_caching.rb | 14 +++++++------- .../collection_cache_association_loading.rb | 4 ++-- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index bede112a36..b3d8e63e42 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -334,11 +334,11 @@ module ActionView end collection_body = if template - cache_collection_render(payload, view, template) do - collection_with_template(view, template) + cache_collection_render(payload, view, template, @collection) do |collection| + collection_with_template(view, template, collection) end else - collection_with_template(view, nil) + collection_with_template(view, nil, @collection) end build_rendered_collection(collection_body, spacer) end @@ -436,7 +436,7 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end - def collection_with_template(view, template) + def collection_with_template(view, template, collection) locals, collection_data = @locals, @collection_data cache = {} @@ -444,9 +444,9 @@ module ActionView layout = find_template(layout, template_keys) end - partial_iteration = PartialIteration.new(@collection.size) + partial_iteration = PartialIteration.new(collection.size) - @collection.map do |object| + collection.map do |object| index = partial_iteration.index path, as, counter, iteration = collection_data[index] diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index c12a25a5ec..c863ede182 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -11,13 +11,13 @@ module ActionView end private - def cache_collection_render(instrumentation_payload, view, template) - return yield unless @options[:cached] && view.controller.respond_to?(:perform_caching) && view.controller.perform_caching + def cache_collection_render(instrumentation_payload, view, template, collection) + return yield(collection) unless @options[:cached] && view.controller.respond_to?(:perform_caching) && view.controller.perform_caching # Result is a hash with the key represents the # key used for cache lookup and the value is the item # on which the partial is being rendered - keyed_collection, ordered_keys = collection_by_cache_keys(view, template) + keyed_collection, ordered_keys = collection_by_cache_keys(view, template, collection) # Pull all partials from cache # Result is a hash, key matches the entry in @@ -29,7 +29,7 @@ module ActionView # Extract the items for the keys that are not found # Set the uncached values to instance variable @collection # which is used by the caller - @collection = keyed_collection.reject { |key, _| cached_partials.key?(key) }.values + collection = keyed_collection.reject { |key, _| cached_partials.key?(key) }.values # If all elements are already in cache then # rendered partials will be an empty array @@ -37,7 +37,7 @@ module ActionView # If the cache is missing elements then # the block will be called against the remaining items # in the @collection. - rendered_partials = @collection.empty? ? [] : yield + rendered_partials = collection.empty? ? [] : yield(collection) index = 0 keyed_partials = fetch_or_cache_partial(cached_partials, template, order_by: keyed_collection.each_key) do @@ -55,12 +55,12 @@ module ActionView @options[:cached].respond_to?(:call) end - def collection_by_cache_keys(view, template) + def collection_by_cache_keys(view, template, collection) seed = callable_cache_key? ? @options[:cached] : ->(i) { i } digest_path = view.digest_path_from_template(template) - @collection.each_with_object([{}, []]) do |item, (hash, ordered_keys)| + collection.each_with_object([{}, []]) do |item, (hash, ordered_keys)| key = expanded_cache_key(seed.call(item), view, template, digest_path) ordered_keys << key hash[key] = item diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index a8cc6fe9ae..2fb44713d5 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -22,8 +22,8 @@ module ActiveRecord end end - def collection_with_template(*) - @relation.preload_associations(@collection) if @relation + def collection_with_template(_, _, collection) + @relation.preload_associations(collection) if @relation super end end From 7a9c90184a932f6a2765248f14597bc37568e534 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 13:42:05 -0800 Subject: [PATCH 07/44] variable ivar is not needed anymore --- actionview/lib/action_view/renderer/partial_renderer.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index b3d8e63e42..1ac2ad12f5 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -305,7 +305,6 @@ module ActionView if @path template = find_template(@path, template_keys) - @variable = template.variable else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -347,7 +346,8 @@ module ActionView def render_partial(view, template, block) instrument(:partial, identifier: template.identifier) do |payload| locals = @locals - object, as = @object, @variable + as = template.variable + object = @object if !block && (layout = @options[:layout]) layout = find_template(layout.to_s, template_keys) @@ -388,6 +388,7 @@ module ActionView @path = partial if @collection + @has_object = true paths = @collection_data = @collection.map { |o| @path } paths.map! { |path| retrieve_variable(path, as).unshift(path) } end From 67b1a7942e8b0bc2f30af78bbcb81c535a87e5dc Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 13:52:45 -0800 Subject: [PATCH 08/44] remove useless conditional --- actionview/lib/action_view/renderer/partial_renderer.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 1ac2ad12f5..6c7715de36 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -353,7 +353,6 @@ module ActionView layout = find_template(layout.to_s, template_keys) end - object = locals[as] if object.nil? # Respect object when object is false locals[as] = object if @has_object content = template.render(view, locals) do |*name| From c52fa675b8784c1bcdc996a353a44922443b0b9f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 14:12:38 -0800 Subject: [PATCH 09/44] Find the layout earlier Remove duplication in finding a layout. Remove `@as` ivar. --- .../action_view/renderer/partial_renderer.rb | 50 +++++++++---------- .../collection_cache_association_loading.rb | 2 +- 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 6c7715de36..3672169adb 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -291,20 +291,12 @@ module ActionView @context_prefix = @lookup_context.prefixes.first end - def template_keys - if @has_object || @collection - @locals.keys + retrieve_variable(@path, @as) - else - @locals.keys - end - end - def render(context, options, block) - @as = as_variable(options) - setup(context, options, @as) + as = as_variable(options) + setup(context, options, as) if @path - template = find_template(@path, template_keys) + template = find_template(@path, template_keys(@path, as)) else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -312,15 +304,27 @@ module ActionView template = nil end + if !block && (layout = @options[:layout]) + layout = find_template(layout.to_s, template_keys(@path, as)) + end + if @collection - render_collection(context, template) + render_collection(context, template, layout) else - render_partial(context, template, block) + render_partial(context, template, layout, block) end end private - def render_collection(view, template) + def template_keys(path, as) + if @has_object || @collection + @locals.keys + retrieve_variable(path, as) + else + @locals.keys + end + end + + def render_collection(view, template, layout) identifier = (template && template.identifier) || @path instrument(:collection, identifier: identifier, count: @collection.size) do |payload| return RenderedCollection.empty(@lookup_context.formats.first) if @collection.blank? @@ -334,25 +338,21 @@ module ActionView collection_body = if template cache_collection_render(payload, view, template, @collection) do |collection| - collection_with_template(view, template, collection) + collection_with_template(view, template, layout, collection) end else - collection_with_template(view, nil, @collection) + collection_with_template(view, nil, layout, @collection) end build_rendered_collection(collection_body, spacer) end end - def render_partial(view, template, block) + def render_partial(view, template, layout, block) instrument(:partial, identifier: template.identifier) do |payload| locals = @locals as = template.variable object = @object - if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys) - end - locals[as] = object if @has_object content = template.render(view, locals) do |*name| @@ -436,13 +436,9 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end - def collection_with_template(view, template, collection) + def collection_with_template(view, template, layout, collection) locals, collection_data = @locals, @collection_data - cache = {} - - if layout = @options[:layout] - layout = find_template(layout, template_keys) - end + cache = template || {} partial_iteration = PartialIteration.new(collection.size) diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index 2fb44713d5..2c54647c25 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -22,7 +22,7 @@ module ActiveRecord end end - def collection_with_template(_, _, collection) + def collection_with_template(_, _, _, collection) @relation.preload_associations(collection) if @relation super end From 5fa2c69e4b33452020cb4de7d202b3fc787868ea Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 14:42:52 -0800 Subject: [PATCH 10/44] Don't to_a things when we don't need to. If the partial renderer is passed a relation or collection proxy, it will be good for us to resolve that relation or collection proxy as late as possible. --- .../lib/action_view/renderer/partial_renderer.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 3672169adb..fec2f49f42 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -423,12 +423,12 @@ module ActionView def collection_from_options if @options.key?(:collection) collection = @options[:collection] - collection ? collection.to_a : [] + collection || [] end end def collection_from_object - @object.to_ary if @object.respond_to?(:to_ary) + @object if @object.respond_to?(:to_ary) end def find_template(path, locals) @@ -475,16 +475,12 @@ module ActionView end if view.prefix_partial_path_with_controller_namespace - prefixed_partial_names[path] ||= merge_prefix_into_object_path(@context_prefix, path.dup) + PREFIXED_PARTIAL_NAMES[@context_prefix][path] ||= merge_prefix_into_object_path(@context_prefix, path.dup) else path end end - def prefixed_partial_names - @prefixed_partial_names ||= PREFIXED_PARTIAL_NAMES[@context_prefix] - end - def merge_prefix_into_object_path(prefix, object_path) if prefix.include?(?/) && object_path.include?(?/) prefixes = [] From 638cc381b11d421f30670ea3cf9aa780d710b7bf Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 15:17:05 -0800 Subject: [PATCH 11/44] rely less on the collection ivar --- actionview/lib/action_view/renderer/partial_renderer.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index fec2f49f42..79b21da623 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -327,8 +327,6 @@ module ActionView def render_collection(view, template, layout) identifier = (template && template.identifier) || @path instrument(:collection, identifier: identifier, count: @collection.size) do |payload| - return RenderedCollection.empty(@lookup_context.formats.first) if @collection.blank? - spacer = if @options.key?(:spacer_template) spacer_template = find_template(@options[:spacer_template], @locals.keys) build_rendered_template(spacer_template.render(view, @locals), spacer_template) @@ -343,6 +341,9 @@ module ActionView else collection_with_template(view, nil, layout, @collection) end + + return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? + build_rendered_collection(collection_body, spacer) end end From 347094856caa872117721b0f753a0eb8bdd862a8 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 25 Feb 2020 16:07:36 -0800 Subject: [PATCH 12/44] Only iterate over collections once when a path is provided We'll later reuse this object to speed up the case when a relation is passed in. --- .../action_view/renderer/partial_renderer.rb | 68 +++++++++++++++++-- .../partial_renderer/collection_caching.rb | 10 ++- .../collection_cache_association_loading.rb | 2 +- 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 79b21da623..87d9a620f7 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -366,6 +366,61 @@ module ActionView end end + class CollectionIterator # :nodoc: + include Enumerable + + attr_reader :collection + + def initialize(collection, path, variables) + @collection = collection + @path = path + @variables = variables + end + + def from_collection(collection) + return collection if collection == self + + self.class.new(collection, @path, @variables) + end + + def each(&blk) + @collection.each(&blk) + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + + variables = [@path] + @variables + @collection.each { |o| yield(o, variables) } + end + + def size + @collection.size + end + end + + class MixedCollectionIterator # :nodoc: + include Enumerable + + def initialize(collection, paths) + @collection = collection + @paths = paths + end + + def each(&blk) + @collection.each(&blk) + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + @collection.each_with_index { |o, i| yield(o, @paths[i]) } + end + + def size + @collection.size + end + end + # Sets up instance variables needed for rendering a partial. This method # finds the options and details and extracts them. The method also contains # logic that handles the type of object passed in as the partial. @@ -389,8 +444,7 @@ module ActionView if @collection @has_object = true - paths = @collection_data = @collection.map { |o| @path } - paths.map! { |path| retrieve_variable(path, as).unshift(path) } + @collection = CollectionIterator.new(@collection, @path, retrieve_variable(@path, as)) end else @has_object = true @@ -398,13 +452,14 @@ module ActionView @collection = collection_from_object || collection_from_options if @collection - paths = @collection_data = @collection.map { |o| partial_path(o, context) } + paths = @collection.map { |o| partial_path(o, context) } if paths.uniq.length == 1 @path = paths.first - paths.map! { |path| retrieve_variable(path, as).unshift(path) } + @collection = CollectionIterator.new(@collection, @path, retrieve_variable(@path, as)) else paths.map! { |path| retrieve_variable(path, as).unshift(path) } @path = nil + @collection = MixedCollectionIterator.new(@collection, paths) end else @path = partial_path(@object, context) @@ -438,14 +493,13 @@ module ActionView end def collection_with_template(view, template, layout, collection) - locals, collection_data = @locals, @collection_data + locals = @locals cache = template || {} partial_iteration = PartialIteration.new(collection.size) - collection.map do |object| + collection.each_with_info.map do |object, (path, as, counter, iteration)| index = partial_iteration.index - path, as, counter, iteration = collection_data[index] locals[as] = object locals[counter] = index diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index c863ede182..4cf94cd6e2 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -11,8 +11,14 @@ module ActionView end private + def will_cache?(options, view) + options[:cached] && view.controller.respond_to?(:perform_caching) && view.controller.perform_caching + end + def cache_collection_render(instrumentation_payload, view, template, collection) - return yield(collection) unless @options[:cached] && view.controller.respond_to?(:perform_caching) && view.controller.perform_caching + return yield(collection) unless will_cache?(@options, view) + + collection_iterator = collection # Result is a hash with the key represents the # key used for cache lookup and the value is the item @@ -37,7 +43,7 @@ module ActionView # If the cache is missing elements then # the block will be called against the remaining items # in the @collection. - rendered_partials = collection.empty? ? [] : yield(collection) + rendered_partials = collection.empty? ? [] : yield(collection_iterator.from_collection(collection)) index = 0 keyed_partials = fetch_or_cache_partial(cached_partials, template, order_by: keyed_collection.each_key) do diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index 2c54647c25..a729e474ad 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -23,7 +23,7 @@ module ActiveRecord end def collection_with_template(_, _, _, collection) - @relation.preload_associations(collection) if @relation + @relation.preload_associations(collection.collection) if @relation super end end From 6539657a3c7bb205e9c0812d6c73a2326699ec52 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 08:18:47 -0800 Subject: [PATCH 13/44] pull iterator allocation in to a factory method --- .../action_view/renderer/partial_renderer.rb | 80 ++++++++++--------- 1 file changed, 42 insertions(+), 38 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 87d9a620f7..f505c3afcf 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -316,8 +316,25 @@ module ActionView end private + def build_collection_iterator(collection, path, as, context) + if path + SameCollectionIterator.new(collection, path, retrieve_variable(path, as)) + else + paths = collection.map { |o| partial_path(o, context) } + + if paths.uniq.length == 1 + @path = paths.first + build_collection_iterator(collection, @path, as, context) + else + paths.map! { |path| retrieve_variable(path, as).unshift(path) } + @path = nil + MixedCollectionIterator.new(collection, paths) + end + end + end + def template_keys(path, as) - if @has_object || @collection + if @has_object @locals.keys + retrieve_variable(path, as) else @locals.keys @@ -371,53 +388,47 @@ module ActionView attr_reader :collection - def initialize(collection, path, variables) + def initialize(collection) @collection = collection - @path = path - @variables = variables - end - - def from_collection(collection) - return collection if collection == self - - self.class.new(collection, @path, @variables) end def each(&blk) @collection.each(&blk) end - def each_with_info - return enum_for(:each_with_info) unless block_given? - - variables = [@path] + @variables - @collection.each { |o| yield(o, variables) } - end - def size @collection.size end end - class MixedCollectionIterator # :nodoc: - include Enumerable - - def initialize(collection, paths) - @collection = collection - @paths = paths + class SameCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, path, variables) + super(collection) + @path = path + @variables = variables end - def each(&blk) - @collection.each(&blk) + def from_collection(collection) + return collection if collection == self + self.class.new(collection, @path, @variables) end def each_with_info return enum_for(:each_with_info) unless block_given? - @collection.each_with_index { |o, i| yield(o, @paths[i]) } + variables = [@path] + @variables + @collection.each { |o| yield(o, variables) } + end + end + + class MixedCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, paths) + super(collection) + @paths = paths end - def size - @collection.size + def each_with_info + return enum_for(:each_with_info) unless block_given? + collection.each_with_index { |o, i| yield(o, @paths[i]) } end end @@ -444,23 +455,16 @@ module ActionView if @collection @has_object = true - @collection = CollectionIterator.new(@collection, @path, retrieve_variable(@path, as)) + @collection = build_collection_iterator(@collection, @path, as, context) end + else @has_object = true @object = partial @collection = collection_from_object || collection_from_options if @collection - paths = @collection.map { |o| partial_path(o, context) } - if paths.uniq.length == 1 - @path = paths.first - @collection = CollectionIterator.new(@collection, @path, retrieve_variable(@path, as)) - else - paths.map! { |path| retrieve_variable(path, as).unshift(path) } - @path = nil - @collection = MixedCollectionIterator.new(@collection, paths) - end + @collection = build_collection_iterator(@collection, nil, as, context) else @path = partial_path(@object, context) end From 0c1d14a810dac369c2032e9df6df89cc69209fef Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 08:34:03 -0800 Subject: [PATCH 14/44] a collection is never rendered with an object --- actionview/lib/action_view/renderer/partial_renderer.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index f505c3afcf..32eb5224c9 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -460,12 +460,12 @@ module ActionView else @has_object = true - @object = partial - @collection = collection_from_object || collection_from_options + @collection = collection_from_object(partial) || collection_from_options if @collection @collection = build_collection_iterator(@collection, nil, as, context) else + @object = partial @path = partial_path(@object, context) end end @@ -487,8 +487,8 @@ module ActionView end end - def collection_from_object - @object if @object.respond_to?(:to_ary) + def collection_from_object(partial) + partial if partial.respond_to?(:to_ary) end def find_template(path, locals) From 0c1fbc7e04063e63894a87ad7d90ec225d87e7fd Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 14:51:31 -0800 Subject: [PATCH 15/44] collection rendering has its own class --- actionview/lib/action_view.rb | 1 + .../action_view/renderer/partial_renderer.rb | 263 ++++++++++-------- .../lib/action_view/renderer/renderer.rb | 50 +++- .../activerecord/multifetch_cache_test.rb | 2 +- activerecord/lib/active_record/railtie.rb | 2 +- .../collection_cache_association_loading.rb | 22 +- 6 files changed, 213 insertions(+), 127 deletions(-) diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index 497e0d43c9..8d8a1be56d 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -51,6 +51,7 @@ module ActionView autoload :Renderer autoload :AbstractRenderer autoload :PartialRenderer + autoload :CollectionRenderer, "action_view/renderer/partial_renderer" autoload :TemplateRenderer autoload :StreamingTemplateRenderer end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 32eb5224c9..279c1c47cd 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -296,7 +296,7 @@ module ActionView setup(context, options, as) if @path - template = find_template(@path, template_keys(@path, as)) + template = find_template(@path, template_keys(@path, as, @collection)) else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -305,66 +305,21 @@ module ActionView end if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(@path, as)) + layout = find_template(layout.to_s, template_keys(@path, as, @collection)) end - if @collection - render_collection(context, template, layout) - else - render_partial(context, template, layout, block) - end + render_partial(context, template, layout, block) end private - def build_collection_iterator(collection, path, as, context) - if path - SameCollectionIterator.new(collection, path, retrieve_variable(path, as)) - else - paths = collection.map { |o| partial_path(o, context) } - - if paths.uniq.length == 1 - @path = paths.first - build_collection_iterator(collection, @path, as, context) - else - paths.map! { |path| retrieve_variable(path, as).unshift(path) } - @path = nil - MixedCollectionIterator.new(collection, paths) - end - end - end - - def template_keys(path, as) + def template_keys(path, as, collection) if @has_object - @locals.keys + retrieve_variable(path, as) + @locals.keys + retrieve_variable(path, as, collection) else @locals.keys end end - def render_collection(view, template, layout) - identifier = (template && template.identifier) || @path - instrument(:collection, identifier: identifier, count: @collection.size) do |payload| - spacer = if @options.key?(:spacer_template) - spacer_template = find_template(@options[:spacer_template], @locals.keys) - build_rendered_template(spacer_template.render(view, @locals), spacer_template) - else - RenderedTemplate::EMPTY_SPACER - end - - collection_body = if template - cache_collection_render(payload, view, template, @collection) do |collection| - collection_with_template(view, template, layout, collection) - end - else - collection_with_template(view, nil, layout, @collection) - end - - return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? - - build_rendered_collection(collection_body, spacer) - end - end - def render_partial(view, template, layout, block) instrument(:partial, identifier: template.identifier) do |payload| locals = @locals @@ -383,55 +338,6 @@ module ActionView end end - class CollectionIterator # :nodoc: - include Enumerable - - attr_reader :collection - - def initialize(collection) - @collection = collection - end - - def each(&blk) - @collection.each(&blk) - end - - def size - @collection.size - end - end - - class SameCollectionIterator < CollectionIterator # :nodoc: - def initialize(collection, path, variables) - super(collection) - @path = path - @variables = variables - end - - def from_collection(collection) - return collection if collection == self - self.class.new(collection, @path, @variables) - end - - def each_with_info - return enum_for(:each_with_info) unless block_given? - variables = [@path] + @variables - @collection.each { |o| yield(o, variables) } - end - end - - class MixedCollectionIterator < CollectionIterator # :nodoc: - def initialize(collection, paths) - super(collection) - @paths = paths - end - - def each_with_info - return enum_for(:each_with_info) unless block_given? - collection.each_with_index { |o, i| yield(o, @paths[i]) } - end - end - # Sets up instance variables needed for rendering a partial. This method # finds the options and details and extracts them. The method also contains # logic that handles the type of object passed in as the partial. @@ -450,17 +356,11 @@ module ActionView if String === partial @has_object = options.key?(:object) @object = options[:object] - @collection = collection_from_options + @collection = collection_from_options(options) @path = partial - - if @collection - @has_object = true - @collection = build_collection_iterator(@collection, @path, as, context) - end - else @has_object = true - @collection = collection_from_object(partial) || collection_from_options + @collection = collection_from_object(partial) || collection_from_options(options) if @collection @collection = build_collection_iterator(@collection, nil, as, context) @@ -473,6 +373,17 @@ module ActionView self end + def collection_from_options(options) + if options.key?(:collection) + collection = options[:collection] + collection || [] + end + end + + def collection_from_object(object) + object if object.respond_to?(:to_ary) + end + def as_variable(options) if as = options[:as] raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) @@ -480,17 +391,6 @@ module ActionView end end - def collection_from_options - if @options.key?(:collection) - collection = @options[:collection] - collection || [] - end - end - - def collection_from_object(partial) - partial if partial.respond_to?(:to_ary) - end - def find_template(path, locals) prefixes = path.include?(?/) ? [] : @lookup_context.prefixes @lookup_context.find_template(path, prefixes, true, locals, @details) @@ -557,13 +457,13 @@ module ActionView end end - def retrieve_variable(path, as) + def retrieve_variable(path, as, collection) variable = as || begin base = path[-1] == "/" ? "" : File.basename(path) raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ $1.to_sym end - if @collection + if collection variable_counter = :"#{variable}_counter" variable_iteration = :"#{variable}_iteration" end @@ -585,4 +485,127 @@ module ActionView raise ArgumentError.new(OPTION_AS_ERROR_MESSAGE % (as)) end end + + class CollectionRenderer < PartialRenderer + class CollectionIterator # :nodoc: + include Enumerable + + attr_reader :collection + + def initialize(collection) + @collection = collection + end + + def each(&blk) + @collection.each(&blk) + end + + def size + @collection.size + end + end + + class SameCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, path, variables) + super(collection) + @path = path + @variables = variables + end + + def from_collection(collection) + return collection if collection == self + self.class.new(collection, @path, @variables) + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + variables = [@path] + @variables + @collection.each { |o| yield(o, variables) } + end + end + + class MixedCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, paths) + super(collection) + @paths = paths + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + collection.each_with_index { |o, i| yield(o, @paths[i]) } + end + end + + def render_collection_with_partial(collection, partial, context, options, block) + as = as_variable(options) + + @options = options + + @locals = options[:locals] || {} + @details = extract_details(options) + @path = partial + + @has_object = true + @collection = build_collection_iterator(collection, partial, as, context) + + if options[:cached] && !partial + raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" + end + + template = find_template(partial, template_keys(partial, as, collection)) if partial + + if !block && (layout = options[:layout]) + layout = find_template(layout.to_s, template_keys(partial, as, collection)) + end + + render_collection(context, template, layout) + end + + def render_collection_derive_partial(collection, context, options, block) + paths = collection.map { |o| partial_path(o, context) } + + if paths.uniq.length == 1 + # Homogeneous + render_collection_with_partial(collection, paths.first, context, options, block) + else + render_collection_with_partial(collection, nil, context, options, block) + end + end + + private + def build_collection_iterator(collection, path, as, context) + if path + SameCollectionIterator.new(collection, path, retrieve_variable(path, as, collection)) + else + paths = collection.map { |o| partial_path(o, context) } + paths.map! { |path| retrieve_variable(path, as, collection).unshift(path) } + @path = nil + MixedCollectionIterator.new(collection, paths) + end + end + + def render_collection(view, template, layout) + identifier = (template && template.identifier) || @path + instrument(:collection, identifier: identifier, count: @collection.size) do |payload| + spacer = if @options.key?(:spacer_template) + spacer_template = find_template(@options[:spacer_template], @locals.keys) + build_rendered_template(spacer_template.render(view, @locals), spacer_template) + else + RenderedTemplate::EMPTY_SPACER + end + + collection_body = if template + cache_collection_render(payload, view, template, @collection) do |collection| + collection_with_template(view, template, layout, collection) + end + else + collection_with_template(view, nil, layout, @collection) + end + + return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? + + build_rendered_collection(collection_body, spacer) + end + end + end end diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index 75f235fe63..7e34f6cbf8 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -65,7 +65,55 @@ module ActionView end def render_partial_to_object(context, options, &block) #:nodoc: - PartialRenderer.new(@lookup_context).render(context, options, block) + make_renderer(options, context, block) end + + private + + def make_renderer(options, context, block) + partial = options[:partial] + if String === partial + collection = collection_from_options(options) + + if collection + # Collection + Partial + renderer = CollectionRenderer.new(@lookup_context) + renderer.render_collection_with_partial(collection, partial, context, options, block) + else + if options.key?(:object) + # Object + Partial + renderer = PartialRenderer.new(@lookup_context) + renderer.render(context, options, block) + else + # Partial + renderer = PartialRenderer.new(@lookup_context) + renderer.render(context, options, block) + end + end + else + collection = collection_from_object(partial) || collection_from_options(options) + + if collection + # Collection + Derived Partial + renderer = CollectionRenderer.new(@lookup_context) + renderer.render_collection_derive_partial(collection, context, options, block) + else + # Object + Derived Partial + renderer = PartialRenderer.new(@lookup_context) + renderer.render(context, options, block) + end + end + end + + def collection_from_options(options) + if options.key?(:collection) + collection = options[:collection] + collection || [] + end + end + + def collection_from_object(object) + object if object.respond_to?(:to_ary) + end end end diff --git a/actionview/test/activerecord/multifetch_cache_test.rb b/actionview/test/activerecord/multifetch_cache_test.rb index ab4bb4dc3e..3e4782bc86 100644 --- a/actionview/test/activerecord/multifetch_cache_test.rb +++ b/actionview/test/activerecord/multifetch_cache_test.rb @@ -3,7 +3,7 @@ require "active_record_unit" require "active_record/railties/collection_cache_association_loading" -ActionView::PartialRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) +ActionView::CollectionRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) class MultifetchCacheTest < ActiveRecordTestCase fixtures :topics, :replies diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 239ce54d87..9bae89a7ae 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -224,7 +224,7 @@ To keep using the current cache store, you can turn off cache versioning entirel initializer "active_record.collection_cache_association_loading" do require "active_record/railties/collection_cache_association_loading" ActiveSupport.on_load(:action_view) do - ActionView::PartialRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) + ActionView::CollectionRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) end end diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index a729e474ad..d1301a2c06 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -13,13 +13,27 @@ module ActiveRecord super end + def render_collection_with_partial(collection, partial, context, options, block) + @relation = nil + + return super unless options[:cached] + + @relation = get_relation(collection) + + super + end + + def get_relation(relation) + if relation && relation.is_a?(ActiveRecord::Relation) && !relation.loaded? + relation.skip_preloading! + relation + end + end + def relation_from_options(partial, collection) relation = partial if partial.is_a?(ActiveRecord::Relation) relation ||= collection if collection.is_a?(ActiveRecord::Relation) - - if relation && !relation.loaded? - relation.skip_preloading! - end + get_relation(relation) end def collection_with_template(_, _, _, collection) From 974ddb035c6fdba92883309b25fdd25befd74bd0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 14:59:33 -0800 Subject: [PATCH 16/44] remove collection references from the superclass --- .../action_view/renderer/partial_renderer.rb | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 279c1c47cd..87aff8742f 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -292,11 +292,11 @@ module ActionView end def render(context, options, block) - as = as_variable(options) - setup(context, options, as) + @as = as_variable(options) + setup(context, options, @as) if @path - template = find_template(@path, template_keys(@path, as, @collection)) + template = find_template(@path, template_keys(@path, @as)) else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -305,16 +305,16 @@ module ActionView end if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(@path, as, @collection)) + layout = find_template(layout.to_s, template_keys(@path, @as)) end render_partial(context, template, layout, block) end private - def template_keys(path, as, collection) + def template_keys(path, as) if @has_object - @locals.keys + retrieve_variable(path, as, collection) + @locals.keys + retrieve_variable(path, as) else @locals.keys end @@ -457,17 +457,13 @@ module ActionView end end - def retrieve_variable(path, as, collection) + def retrieve_variable(path, as) variable = as || begin base = path[-1] == "/" ? "" : File.basename(path) raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ $1.to_sym end - if collection - variable_counter = :"#{variable}_counter" - variable_iteration = :"#{variable}_iteration" - end - [variable, variable_counter, variable_iteration] + [variable] end IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ @@ -552,10 +548,10 @@ module ActionView raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" end - template = find_template(partial, template_keys(partial, as, collection)) if partial + template = find_template(partial, template_keys(partial, as)) if partial if !block && (layout = options[:layout]) - layout = find_template(layout.to_s, template_keys(partial, as, collection)) + layout = find_template(layout.to_s, template_keys(partial, as)) end render_collection(context, template, layout) @@ -573,12 +569,20 @@ module ActionView end private + def retrieve_variable(path, as) + vars = super + variable = vars.first + vars << :"#{variable}_counter" + vars << :"#{variable}_iteration" + vars + end + def build_collection_iterator(collection, path, as, context) if path - SameCollectionIterator.new(collection, path, retrieve_variable(path, as, collection)) + SameCollectionIterator.new(collection, path, retrieve_variable(path, as)) else paths = collection.map { |o| partial_path(o, context) } - paths.map! { |path| retrieve_variable(path, as, collection).unshift(path) } + paths.map! { |path| retrieve_variable(path, as).unshift(path) } @path = nil MixedCollectionIterator.new(collection, paths) end From 10f7201fc5a43460c0b4015b056f5ece4709e335 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 15:04:18 -0800 Subject: [PATCH 17/44] remove more collection references from the superclass --- .../action_view/renderer/partial_renderer.rb | 65 +++++++------------ 1 file changed, 24 insertions(+), 41 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 87aff8742f..3843621a6e 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -347,7 +347,6 @@ module ActionView # respond to +to_partial_path+ in order to set up the path. def setup(context, options, as) @options = options - @locals = options[:locals] || {} @details = extract_details(options) @@ -356,34 +355,17 @@ module ActionView if String === partial @has_object = options.key?(:object) @object = options[:object] - @collection = collection_from_options(options) @path = partial else @has_object = true - @collection = collection_from_object(partial) || collection_from_options(options) - if @collection - @collection = build_collection_iterator(@collection, nil, as, context) - else - @object = partial - @path = partial_path(@object, context) - end + @object = partial + @path = partial_path(@object, context) end self end - def collection_from_options(options) - if options.key?(:collection) - collection = options[:collection] - collection || [] - end - end - - def collection_from_object(object) - object if object.respond_to?(:to_ary) - end - def as_variable(options) if as = options[:as] raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) @@ -396,27 +378,6 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end - def collection_with_template(view, template, layout, collection) - locals = @locals - cache = template || {} - - partial_iteration = PartialIteration.new(collection.size) - - collection.each_with_info.map do |object, (path, as, counter, iteration)| - index = partial_iteration.index - - locals[as] = object - locals[counter] = index - locals[iteration] = partial_iteration - - _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) - content = _template.render(view, locals) - content = layout.render(view, locals) { content } if layout - partial_iteration.iterate! - build_rendered_template(content, _template) - end - end - # Obtains the path to where the object's partial is located. If the object # responds to +to_partial_path+, then +to_partial_path+ will be called and # will provide the path. If the object does not respond to +to_partial_path+, @@ -611,5 +572,27 @@ module ActionView build_rendered_collection(collection_body, spacer) end end + + def collection_with_template(view, template, layout, collection) + locals = @locals + cache = template || {} + + partial_iteration = PartialIteration.new(collection.size) + + collection.each_with_info.map do |object, (path, as, counter, iteration)| + index = partial_iteration.index + + locals[as] = object + locals[counter] = index + locals[iteration] = partial_iteration + + _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) + content = _template.render(view, locals) + content = layout.render(view, locals) { content } if layout + partial_iteration.iterate! + build_rendered_template(content, _template) + end + end + end end From 844568c23195a4061400c69129d327ba315bddd3 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 15:06:29 -0800 Subject: [PATCH 18/44] remove at ivar again --- actionview/lib/action_view/renderer/partial_renderer.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 3843621a6e..e6fbc73570 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -292,11 +292,11 @@ module ActionView end def render(context, options, block) - @as = as_variable(options) - setup(context, options, @as) + as = as_variable(options) + setup(context, options, as) if @path - template = find_template(@path, template_keys(@path, @as)) + template = find_template(@path, template_keys(@path, as)) else if options[:cached] raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -305,7 +305,7 @@ module ActionView end if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(@path, @as)) + layout = find_template(layout.to_s, template_keys(@path, as)) end render_partial(context, template, layout, block) From 8b6c5850a5fe3b28c9d8c72d089dcabdd17386a0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 15:37:45 -0800 Subject: [PATCH 19/44] introduce an object renderer --- actionview/lib/action_view.rb | 1 + .../action_view/renderer/partial_renderer.rb | 36 ++++++++++++------- .../lib/action_view/renderer/renderer.rb | 8 ++--- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index 8d8a1be56d..fd14094792 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -52,6 +52,7 @@ module ActionView autoload :AbstractRenderer autoload :PartialRenderer autoload :CollectionRenderer, "action_view/renderer/partial_renderer" + autoload :ObjectRenderer, "action_view/renderer/partial_renderer" autoload :TemplateRenderer autoload :StreamingTemplateRenderer end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index e6fbc73570..46f9db3442 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -308,7 +308,7 @@ module ActionView layout = find_template(layout.to_s, template_keys(@path, as)) end - render_partial(context, template, layout, block) + render_partial(context, @locals, template, layout, block) end private @@ -320,14 +320,8 @@ module ActionView end end - def render_partial(view, template, layout, block) + def render_partial(view, locals, template, layout, block) instrument(:partial, identifier: template.identifier) do |payload| - locals = @locals - as = template.variable - object = @object - - locals[as] = object if @has_object - content = template.render(view, locals) do |*name| view._layout_for(*name, &block) end @@ -354,12 +348,8 @@ module ActionView if String === partial @has_object = options.key?(:object) - @object = options[:object] @path = partial else - @has_object = true - - @object = partial @path = partial_path(@object, context) end @@ -593,6 +583,28 @@ module ActionView build_rendered_template(content, _template) end end + end + class ObjectRenderer < PartialRenderer + def render_object_with_partial(object, partial, context, options, block) + @has_object = true + @object = object + + render(context, options, block) + end + + def render_object_derive_partial(object, context, options, block) + path = partial_path(object, context) + + render_object_with_partial(object, path, context, options, block) + end + + private + + def render_partial(view, locals, template, layout, block) + as = template.variable + locals[as] = @object + super(view, locals, template, layout, block) + end end end diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index 7e34f6cbf8..ac22a2bb61 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -82,8 +82,8 @@ module ActionView else if options.key?(:object) # Object + Partial - renderer = PartialRenderer.new(@lookup_context) - renderer.render(context, options, block) + renderer = ObjectRenderer.new(@lookup_context) + renderer.render_object_with_partial(options[:object], partial, context, options, block) else # Partial renderer = PartialRenderer.new(@lookup_context) @@ -99,8 +99,8 @@ module ActionView renderer.render_collection_derive_partial(collection, context, options, block) else # Object + Derived Partial - renderer = PartialRenderer.new(@lookup_context) - renderer.render(context, options, block) + renderer = ObjectRenderer.new(@lookup_context) + renderer.render_object_derive_partial(partial, context, options, block) end end end From feec68436c921eb8b4e8dfba89eaa98eb93a7035 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 15:39:57 -0800 Subject: [PATCH 20/44] remove has_object ivar --- .../action_view/renderer/partial_renderer.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 46f9db3442..b81d8e0368 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -313,11 +313,7 @@ module ActionView private def template_keys(path, as) - if @has_object - @locals.keys + retrieve_variable(path, as) - else - @locals.keys - end + @locals.keys end def render_partial(view, locals, template, layout, block) @@ -347,7 +343,6 @@ module ActionView partial = options[:partial] if String === partial - @has_object = options.key?(:object) @path = partial else @path = partial_path(@object, context) @@ -492,7 +487,6 @@ module ActionView @details = extract_details(options) @path = partial - @has_object = true @collection = build_collection_iterator(collection, partial, as, context) if options[:cached] && !partial @@ -520,6 +514,10 @@ module ActionView end private + def template_keys(path, as) + super + retrieve_variable(path, as) + end + def retrieve_variable(path, as) vars = super variable = vars.first @@ -587,20 +585,21 @@ module ActionView class ObjectRenderer < PartialRenderer def render_object_with_partial(object, partial, context, options, block) - @has_object = true @object = object - render(context, options, block) end def render_object_derive_partial(object, context, options, block) path = partial_path(object, context) - render_object_with_partial(object, path, context, options, block) end private + def template_keys(path, as) + super + retrieve_variable(path, as) + end + def render_partial(view, locals, template, layout, block) as = template.variable locals[as] = @object From 9187f7482c40bfcb090e2107b274384f12db4f0b Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 15:57:59 -0800 Subject: [PATCH 21/44] partial renderer is specialized for rendering partials now --- .../action_view/renderer/partial_renderer.rb | 50 +++++-------------- .../lib/action_view/renderer/renderer.rb | 2 +- .../collection_cache_association_loading.rb | 10 ---- 3 files changed, 13 insertions(+), 49 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index b81d8e0368..90d223e9ae 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -291,24 +291,21 @@ module ActionView @context_prefix = @lookup_context.prefixes.first end - def render(context, options, block) - as = as_variable(options) - setup(context, options, as) + def render(partial, context, options, block) + @options = options + @locals = options[:locals] || {} + @details = extract_details(options) + @path = partial - if @path - template = find_template(@path, template_keys(@path, as)) - else - if options[:cached] - raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" - end - template = nil - end + as = as_variable(options) + + template = find_template(@path, template_keys(@path, as)) if !block && (layout = @options[:layout]) layout = find_template(layout.to_s, template_keys(@path, as)) end - render_partial(context, @locals, template, layout, block) + render_partial_template(context, @locals, template, layout, block) end private @@ -316,7 +313,7 @@ module ActionView @locals.keys end - def render_partial(view, locals, template, layout, block) + def render_partial_template(view, locals, template, layout, block) instrument(:partial, identifier: template.identifier) do |payload| content = template.render(view, locals) do |*name| view._layout_for(*name, &block) @@ -328,29 +325,6 @@ module ActionView end end - # Sets up instance variables needed for rendering a partial. This method - # finds the options and details and extracts them. The method also contains - # logic that handles the type of object passed in as the partial. - # - # If +options[:partial]+ is a string, then the @path instance variable is - # set to that string. Otherwise, the +options[:partial]+ object must - # respond to +to_partial_path+ in order to set up the path. - def setup(context, options, as) - @options = options - @locals = options[:locals] || {} - @details = extract_details(options) - - partial = options[:partial] - - if String === partial - @path = partial - else - @path = partial_path(@object, context) - end - - self - end - def as_variable(options) if as = options[:as] raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) @@ -586,7 +560,7 @@ module ActionView class ObjectRenderer < PartialRenderer def render_object_with_partial(object, partial, context, options, block) @object = object - render(context, options, block) + render(partial, context, options, block) end def render_object_derive_partial(object, context, options, block) @@ -600,7 +574,7 @@ module ActionView super + retrieve_variable(path, as) end - def render_partial(view, locals, template, layout, block) + def render_partial_template(view, locals, template, layout, block) as = template.variable locals[as] = @object super(view, locals, template, layout, block) diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index ac22a2bb61..bf619c03db 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -87,7 +87,7 @@ module ActionView else # Partial renderer = PartialRenderer.new(@lookup_context) - renderer.render(context, options, block) + renderer.render(partial, context, options, block) end end else diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index d1301a2c06..766f25beed 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -3,16 +3,6 @@ module ActiveRecord module Railties # :nodoc: module CollectionCacheAssociationLoading #:nodoc: - def setup(context, options, as) - @relation = nil - - return super unless options[:cached] - - @relation = relation_from_options(options[:partial], options[:collection]) - - super - end - def render_collection_with_partial(collection, partial, context, options, block) @relation = nil From f0a15484335341f14f36d6d2e98f447f1b4e24fe Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:13:12 -0800 Subject: [PATCH 22/44] as: doesn't make sense without an object parameter The local variable name in `as:` may not be a valid local variable name, but if there is no object specified to be assigned to the parameter, then why supply the `as:`? This commit adds an object for the as param --- actionview/test/template/render_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index eab73d622c..c22bdd6659 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -265,14 +265,14 @@ module RenderTestCases end def test_render_partial_with_invalid_option_as - e = assert_raises(ArgumentError) { @view.render(partial: "test/partial_only", as: "a-in") } + e = assert_raises(ArgumentError) { @view.render(partial: "test/partial_only", as: "a-in", object: nil) } assert_equal "The value (a-in) of the option `as` is not a valid Ruby identifier; " \ "make sure it starts with lowercase letter, " \ "and is followed by any combination of letters, numbers and underscores.", e.message end def test_render_partial_with_hyphen_and_invalid_option_as - e = assert_raises(ArgumentError) { @view.render(partial: "test/a-in", as: "a-in") } + e = assert_raises(ArgumentError) { @view.render(partial: "test/a-in", as: "a-in", object: nil) } assert_equal "The value (a-in) of the option `as` is not a valid Ruby identifier; " \ "make sure it starts with lowercase letter, " \ "and is followed by any combination of letters, numbers and underscores.", e.message From f8f2f9ce32965c06a7af588582ed2f0d03b4c543 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:17:49 -0800 Subject: [PATCH 23/44] remove the as_variable method --- .../action_view/renderer/partial_renderer.rb | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 90d223e9ae..d6f206c46f 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -297,19 +297,17 @@ module ActionView @details = extract_details(options) @path = partial - as = as_variable(options) - - template = find_template(@path, template_keys(@path, as)) + template = find_template(@path, template_keys(@path)) if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(@path, as)) + layout = find_template(layout.to_s, template_keys(@path)) end render_partial_template(context, @locals, template, layout, block) end private - def template_keys(path, as) + def template_keys(path) @locals.keys end @@ -325,13 +323,6 @@ module ActionView end end - def as_variable(options) - if as = options[:as] - raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) - as.to_sym - end - end - def find_template(path, locals) prefixes = path.include?(?/) ? [] : @lookup_context.prefixes @lookup_context.find_template(path, prefixes, true, locals, @details) @@ -377,11 +368,16 @@ module ActionView end end - def retrieve_variable(path, as) - variable = as || begin - base = path[-1] == "/" ? "" : File.basename(path) - raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ - $1.to_sym + def retrieve_variable(path) + variable = if as = @options[:as] + raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) + as.to_sym + else + begin + base = path[-1] == "/" ? "" : File.basename(path) + raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ + $1.to_sym + end end [variable] end @@ -453,24 +449,22 @@ module ActionView end def render_collection_with_partial(collection, partial, context, options, block) - as = as_variable(options) - @options = options @locals = options[:locals] || {} @details = extract_details(options) @path = partial - @collection = build_collection_iterator(collection, partial, as, context) + @collection = build_collection_iterator(collection, partial, context) if options[:cached] && !partial raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" end - template = find_template(partial, template_keys(partial, as)) if partial + template = find_template(partial, template_keys(partial)) if partial if !block && (layout = options[:layout]) - layout = find_template(layout.to_s, template_keys(partial, as)) + layout = find_template(layout.to_s, template_keys(partial)) end render_collection(context, template, layout) @@ -488,11 +482,11 @@ module ActionView end private - def template_keys(path, as) - super + retrieve_variable(path, as) + def template_keys(path) + super + retrieve_variable(path) end - def retrieve_variable(path, as) + def retrieve_variable(path) vars = super variable = vars.first vars << :"#{variable}_counter" @@ -500,12 +494,12 @@ module ActionView vars end - def build_collection_iterator(collection, path, as, context) + def build_collection_iterator(collection, path, context) if path - SameCollectionIterator.new(collection, path, retrieve_variable(path, as)) + SameCollectionIterator.new(collection, path, retrieve_variable(path)) else paths = collection.map { |o| partial_path(o, context) } - paths.map! { |path| retrieve_variable(path, as).unshift(path) } + paths.map! { |path| retrieve_variable(path).unshift(path) } @path = nil MixedCollectionIterator.new(collection, paths) end @@ -570,8 +564,8 @@ module ActionView private - def template_keys(path, as) - super + retrieve_variable(path, as) + def template_keys(path) + super + retrieve_variable(path) end def render_partial_template(view, locals, template, layout, block) From f3dfd288214120bcdb9fd405cd0f2e8f842d4ea8 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:19:19 -0800 Subject: [PATCH 24/44] remove extra ivar set --- actionview/lib/action_view/renderer/partial_renderer.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index d6f206c46f..4f658a65ae 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -500,7 +500,6 @@ module ActionView else paths = collection.map { |o| partial_path(o, context) } paths.map! { |path| retrieve_variable(path).unshift(path) } - @path = nil MixedCollectionIterator.new(collection, paths) end end From ef6e03d5b120e4b263755fe88ea9983608eaf1ba Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:33:27 -0800 Subject: [PATCH 25/44] Move options ivar assignment in to the superclass --- .../action_view/renderer/abstract_renderer.rb | 21 ++++++ .../action_view/renderer/partial_renderer.rb | 66 +++++++------------ .../lib/action_view/renderer/renderer.rb | 20 +++--- .../collection_cache_association_loading.rb | 4 +- 4 files changed, 56 insertions(+), 55 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 3738ac8c26..4b2d4854ae 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -27,6 +27,27 @@ module ActionView raise NotImplementedError end + module ObjectRendering # :nodoc: + private + def template_keys(path) + super + retrieve_variable(path) + end + + def retrieve_variable(path) + variable = if as = @options[:as] + raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) + as.to_sym + else + begin + base = path[-1] == "/" ? "" : File.basename(path) + raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ + $1.to_sym + end + end + [variable] + end + end + class RenderedCollection # :nodoc: def self.empty(format) EmptyCollection.new format diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 4f658a65ae..d266eaff61 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -286,15 +286,15 @@ module ActionView h[k] = Concurrent::Map.new end - def initialize(*) - super + def initialize(lookup_context, options) + super(lookup_context) + @options = options @context_prefix = @lookup_context.prefixes.first end - def render(partial, context, options, block) - @options = options - @locals = options[:locals] || {} - @details = extract_details(options) + def render(partial, context, block) + @locals = @options[:locals] || {} + @details = extract_details(@options) @path = partial template = find_template(@path, template_keys(@path)) @@ -307,7 +307,7 @@ module ActionView end private - def template_keys(path) + def template_keys(_) @locals.keys end @@ -368,20 +368,6 @@ module ActionView end end - def retrieve_variable(path) - variable = if as = @options[:as] - raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) - as.to_sym - else - begin - base = path[-1] == "/" ? "" : File.basename(path) - raise_invalid_identifier(path) unless base =~ /\A_?(.*?)(?:\.\w+)*\z/ - $1.to_sym - end - end - [variable] - end - IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ "make sure your partial name starts with underscore." @@ -399,6 +385,8 @@ module ActionView end class CollectionRenderer < PartialRenderer + include ObjectRendering + class CollectionIterator # :nodoc: include Enumerable @@ -448,44 +436,38 @@ module ActionView end end - def render_collection_with_partial(collection, partial, context, options, block) - @options = options - - @locals = options[:locals] || {} - @details = extract_details(options) + def render_collection_with_partial(collection, partial, context, block) + @locals = @options[:locals] || {} + @details = extract_details(@options) @path = partial @collection = build_collection_iterator(collection, partial, context) - if options[:cached] && !partial + if @options[:cached] && !partial raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" end template = find_template(partial, template_keys(partial)) if partial - if !block && (layout = options[:layout]) + if !block && (layout = @options[:layout]) layout = find_template(layout.to_s, template_keys(partial)) end render_collection(context, template, layout) end - def render_collection_derive_partial(collection, context, options, block) + def render_collection_derive_partial(collection, context, block) paths = collection.map { |o| partial_path(o, context) } if paths.uniq.length == 1 # Homogeneous - render_collection_with_partial(collection, paths.first, context, options, block) + render_collection_with_partial(collection, paths.first, context, block) else - render_collection_with_partial(collection, nil, context, options, block) + render_collection_with_partial(collection, nil, context, block) end end private - def template_keys(path) - super + retrieve_variable(path) - end - def retrieve_variable(path) vars = super variable = vars.first @@ -551,22 +533,20 @@ module ActionView end class ObjectRenderer < PartialRenderer - def render_object_with_partial(object, partial, context, options, block) + include ObjectRendering + + def render_object_with_partial(object, partial, context, block) @object = object - render(partial, context, options, block) + render(partial, context, block) end - def render_object_derive_partial(object, context, options, block) + def render_object_derive_partial(object, context, block) path = partial_path(object, context) - render_object_with_partial(object, path, context, options, block) + render_object_with_partial(object, path, context, block) end private - def template_keys(path) - super + retrieve_variable(path) - end - def render_partial_template(view, locals, template, layout, block) as = template.variable locals[as] = @object diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index bf619c03db..b30306388f 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -77,17 +77,17 @@ module ActionView if collection # Collection + Partial - renderer = CollectionRenderer.new(@lookup_context) - renderer.render_collection_with_partial(collection, partial, context, options, block) + renderer = CollectionRenderer.new(@lookup_context, options) + renderer.render_collection_with_partial(collection, partial, context, block) else if options.key?(:object) # Object + Partial - renderer = ObjectRenderer.new(@lookup_context) - renderer.render_object_with_partial(options[:object], partial, context, options, block) + renderer = ObjectRenderer.new(@lookup_context, options) + renderer.render_object_with_partial(options[:object], partial, context, block) else # Partial - renderer = PartialRenderer.new(@lookup_context) - renderer.render(partial, context, options, block) + renderer = PartialRenderer.new(@lookup_context, options) + renderer.render(partial, context, block) end end else @@ -95,12 +95,12 @@ module ActionView if collection # Collection + Derived Partial - renderer = CollectionRenderer.new(@lookup_context) - renderer.render_collection_derive_partial(collection, context, options, block) + renderer = CollectionRenderer.new(@lookup_context, options) + renderer.render_collection_derive_partial(collection, context, block) else # Object + Derived Partial - renderer = ObjectRenderer.new(@lookup_context) - renderer.render_object_derive_partial(partial, context, options, block) + renderer = ObjectRenderer.new(@lookup_context, options) + renderer.render_object_derive_partial(partial, context, block) end end end diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb index 766f25beed..28e84c0563 100644 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb @@ -3,10 +3,10 @@ module ActiveRecord module Railties # :nodoc: module CollectionCacheAssociationLoading #:nodoc: - def render_collection_with_partial(collection, partial, context, options, block) + def render_collection_with_partial(collection, partial, context, block) @relation = nil - return super unless options[:cached] + return super unless @options[:cached] @relation = get_relation(collection) From 7210adb7ecec40ef9a7e149bda23a3d905f684e9 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:35:03 -0800 Subject: [PATCH 26/44] move locals and details ivar set in to initialize --- actionview/lib/action_view/renderer/partial_renderer.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index d266eaff61..48bfd89a47 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -289,12 +289,12 @@ module ActionView def initialize(lookup_context, options) super(lookup_context) @options = options + @locals = @options[:locals] || {} + @details = extract_details(@options) @context_prefix = @lookup_context.prefixes.first end def render(partial, context, block) - @locals = @options[:locals] || {} - @details = extract_details(@options) @path = partial template = find_template(@path, template_keys(@path)) @@ -437,8 +437,6 @@ module ActionView end def render_collection_with_partial(collection, partial, context, block) - @locals = @options[:locals] || {} - @details = extract_details(@options) @path = partial @collection = build_collection_iterator(collection, partial, context) From 17fb6f0503d8c8c8f72ef6d3358611f181dec4ad Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:42:42 -0800 Subject: [PATCH 27/44] Move `partial_path` to ObjectRendering Partial paths are only required for deriving template info for rendering objects, which is only the case for the object renderer and the collection renderer --- .../action_view/renderer/abstract_renderer.rb | 34 +++++++++++++++++++ .../action_view/renderer/partial_renderer.rb | 33 ++---------------- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 4b2d4854ae..6aa3b4d166 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "concurrent/map" + module ActionView # This class defines the interface for a renderer. Each class that # subclasses +AbstractRenderer+ is used by the base +Renderer+ class to @@ -28,6 +30,15 @@ module ActionView end module ObjectRendering # :nodoc: + PREFIXED_PARTIAL_NAMES = Concurrent::Map.new do |h, k| + h[k] = Concurrent::Map.new + end + + def initialize(lookup_context, options) + super + @context_prefix = lookup_context.prefixes.first + end + private def template_keys(path) super + retrieve_variable(path) @@ -46,6 +57,29 @@ module ActionView end [variable] end + + # Obtains the path to where the object's partial is located. If the object + # responds to +to_partial_path+, then +to_partial_path+ will be called and + # will provide the path. If the object does not respond to +to_partial_path+, + # then an +ArgumentError+ is raised. + # + # If +prefix_partial_path_with_controller_namespace+ is true, then this + # method will prefix the partial paths with a namespace. + def partial_path(object, view) + object = object.to_model if object.respond_to?(:to_model) + + path = if object.respond_to?(:to_partial_path) + object.to_partial_path + else + raise ArgumentError.new("'#{object.inspect}' is not an ActiveModel-compatible object. It must implement :to_partial_path.") + end + + if view.prefix_partial_path_with_controller_namespace + PREFIXED_PARTIAL_NAMES[@context_prefix][path] ||= merge_prefix_into_object_path(@context_prefix, path.dup) + else + path + end + end end class RenderedCollection # :nodoc: diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 48bfd89a47..6f408d2494 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true -require "concurrent/map" require "action_view/renderer/partial_renderer/collection_caching" module ActionView @@ -282,25 +281,20 @@ module ActionView class PartialRenderer < AbstractRenderer include CollectionCaching - PREFIXED_PARTIAL_NAMES = Concurrent::Map.new do |h, k| - h[k] = Concurrent::Map.new - end - def initialize(lookup_context, options) super(lookup_context) @options = options @locals = @options[:locals] || {} @details = extract_details(@options) - @context_prefix = @lookup_context.prefixes.first end def render(partial, context, block) @path = partial - template = find_template(@path, template_keys(@path)) + template = find_template(partial, template_keys(partial)) if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(@path)) + layout = find_template(layout.to_s, template_keys(partial)) end render_partial_template(context, @locals, template, layout, block) @@ -328,29 +322,6 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end - # Obtains the path to where the object's partial is located. If the object - # responds to +to_partial_path+, then +to_partial_path+ will be called and - # will provide the path. If the object does not respond to +to_partial_path+, - # then an +ArgumentError+ is raised. - # - # If +prefix_partial_path_with_controller_namespace+ is true, then this - # method will prefix the partial paths with a namespace. - def partial_path(object, view) - object = object.to_model if object.respond_to?(:to_model) - - path = if object.respond_to?(:to_partial_path) - object.to_partial_path - else - raise ArgumentError.new("'#{object.inspect}' is not an ActiveModel-compatible object. It must implement :to_partial_path.") - end - - if view.prefix_partial_path_with_controller_namespace - PREFIXED_PARTIAL_NAMES[@context_prefix][path] ||= merge_prefix_into_object_path(@context_prefix, path.dup) - else - path - end - end - def merge_prefix_into_object_path(prefix, object_path) if prefix.include?(?/) && object_path.include?(?/) prefixes = [] From 51f50a923e4ae73235f7ae3c846b56332d53ca89 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 16:44:35 -0800 Subject: [PATCH 28/44] path ivar is not required --- .../lib/action_view/renderer/partial_renderer.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 6f408d2494..45cd12ac41 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -289,8 +289,6 @@ module ActionView end def render(partial, context, block) - @path = partial - template = find_template(partial, template_keys(partial)) if !block && (layout = @options[:layout]) @@ -408,8 +406,6 @@ module ActionView end def render_collection_with_partial(collection, partial, context, block) - @path = partial - @collection = build_collection_iterator(collection, partial, context) if @options[:cached] && !partial @@ -422,7 +418,7 @@ module ActionView layout = find_template(layout.to_s, template_keys(partial)) end - render_collection(context, template, layout) + render_collection(context, template, partial, layout) end def render_collection_derive_partial(collection, context, block) @@ -455,8 +451,8 @@ module ActionView end end - def render_collection(view, template, layout) - identifier = (template && template.identifier) || @path + def render_collection(view, template, path, layout) + identifier = (template && template.identifier) || path instrument(:collection, identifier: identifier, count: @collection.size) do |payload| spacer = if @options.key?(:spacer_template) spacer_template = find_template(@options[:spacer_template], @locals.keys) From 2105fec0176940f1f202dbf2250d7ca8b3d595d5 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 17:14:18 -0800 Subject: [PATCH 29/44] Merging path prefixes is only done in object / collection rendering --- .../action_view/renderer/abstract_renderer.rb | 17 +++++++++++++++++ .../action_view/renderer/partial_renderer.rb | 17 ----------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 6aa3b4d166..ded47da68b 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -80,6 +80,23 @@ module ActionView path end end + + def merge_prefix_into_object_path(prefix, object_path) + if prefix.include?(?/) && object_path.include?(?/) + prefixes = [] + prefix_array = File.dirname(prefix).split("/") + object_path_array = object_path.split("/")[0..-3] # skip model dir & partial + + prefix_array.each_with_index do |dir, index| + break if dir == object_path_array[index] + prefixes << dir + end + + (prefixes << object_path).join("/") + else + object_path + end + end end class RenderedCollection # :nodoc: diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 45cd12ac41..077a372f67 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -320,23 +320,6 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end - def merge_prefix_into_object_path(prefix, object_path) - if prefix.include?(?/) && object_path.include?(?/) - prefixes = [] - prefix_array = File.dirname(prefix).split("/") - object_path_array = object_path.split("/")[0..-3] # skip model dir & partial - - prefix_array.each_with_index do |dir, index| - break if dir == object_path_array[index] - prefixes << dir - end - - (prefixes << object_path).join("/") - else - object_path - end - end - IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ "make sure your partial name starts with underscore." From f0429dbc303b43c73558a4866f66db7bf9250a5c Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Wed, 26 Feb 2020 17:14:37 -0800 Subject: [PATCH 30/44] Make extracting details cheaper Details are only used if the user specifies a format or local, etc when rendering a template. This is a fairly uncommon thing to do, so lets make it cheaper when there are no details specified --- .../lib/action_view/renderer/abstract_renderer.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index ded47da68b..0604176323 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -146,12 +146,18 @@ module ActionView end private + NO_DETAILS = {}.freeze + def extract_details(options) # :doc: - @lookup_context.registered_details.each_with_object({}) do |key, details| + details = nil + @lookup_context.registered_details.each do |key| value = options[key] - details[key] = Array(value) if value + if value + (details ||= {})[key] = Array(value) + end end + details || NO_DETAILS end def instrument(name, **options) # :doc: From dbd8860096461c38f62e5e8ca846513a1246e4d0 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 12:49:53 -0800 Subject: [PATCH 31/44] move object specific stuff in to the object processing code --- .../lib/action_view/renderer/abstract_renderer.rb | 15 +++++++++++++++ .../lib/action_view/renderer/partial_renderer.rb | 15 --------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 0604176323..d86265b6c2 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -58,6 +58,21 @@ module ActionView [variable] end + IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ + "make sure your partial name starts with underscore." + + OPTION_AS_ERROR_MESSAGE = "The value (%s) of the option `as` is not a valid Ruby identifier; " \ + "make sure it starts with lowercase letter, " \ + "and is followed by any combination of letters, numbers and underscores." + + def raise_invalid_identifier(path) + raise ArgumentError, IDENTIFIER_ERROR_MESSAGE % path + end + + def raise_invalid_option_as(as) + raise ArgumentError, OPTION_AS_ERROR_MESSAGE % as + end + # Obtains the path to where the object's partial is located. If the object # responds to +to_partial_path+, then +to_partial_path+ will be called and # will provide the path. If the object does not respond to +to_partial_path+, diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 077a372f67..3c1d1f610c 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -319,21 +319,6 @@ module ActionView prefixes = path.include?(?/) ? [] : @lookup_context.prefixes @lookup_context.find_template(path, prefixes, true, locals, @details) end - - IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ - "make sure your partial name starts with underscore." - - OPTION_AS_ERROR_MESSAGE = "The value (%s) of the option `as` is not a valid Ruby identifier; " \ - "make sure it starts with lowercase letter, " \ - "and is followed by any combination of letters, numbers and underscores." - - def raise_invalid_identifier(path) - raise ArgumentError.new(IDENTIFIER_ERROR_MESSAGE % (path)) - end - - def raise_invalid_option_as(as) - raise ArgumentError.new(OPTION_AS_ERROR_MESSAGE % (as)) - end end class CollectionRenderer < PartialRenderer From 665a355ed8d2b366d2a5fb96ffd9e4c26f361d87 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 12:50:02 -0800 Subject: [PATCH 32/44] We don't need this private method --- .../lib/action_view/renderer/renderer.rb | 68 +++++++++---------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index b30306388f..d9973c7ebe 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -65,46 +65,42 @@ module ActionView end def render_partial_to_object(context, options, &block) #:nodoc: - make_renderer(options, context, block) + partial = options[:partial] + if String === partial + collection = collection_from_options(options) + + if collection + # Collection + Partial + renderer = CollectionRenderer.new(@lookup_context, options) + renderer.render_collection_with_partial(collection, partial, context, block) + else + if options.key?(:object) + # Object + Partial + renderer = ObjectRenderer.new(@lookup_context, options) + renderer.render_object_with_partial(options[:object], partial, context, block) + else + # Partial + renderer = PartialRenderer.new(@lookup_context, options) + renderer.render(partial, context, block) + end + end + else + collection = collection_from_object(partial) || collection_from_options(options) + + if collection + # Collection + Derived Partial + renderer = CollectionRenderer.new(@lookup_context, options) + renderer.render_collection_derive_partial(collection, context, block) + else + # Object + Derived Partial + renderer = ObjectRenderer.new(@lookup_context, options) + renderer.render_object_derive_partial(partial, context, block) + end + end end private - def make_renderer(options, context, block) - partial = options[:partial] - if String === partial - collection = collection_from_options(options) - - if collection - # Collection + Partial - renderer = CollectionRenderer.new(@lookup_context, options) - renderer.render_collection_with_partial(collection, partial, context, block) - else - if options.key?(:object) - # Object + Partial - renderer = ObjectRenderer.new(@lookup_context, options) - renderer.render_object_with_partial(options[:object], partial, context, block) - else - # Partial - renderer = PartialRenderer.new(@lookup_context, options) - renderer.render(partial, context, block) - end - end - else - collection = collection_from_object(partial) || collection_from_options(options) - - if collection - # Collection + Derived Partial - renderer = CollectionRenderer.new(@lookup_context, options) - renderer.render_collection_derive_partial(collection, context, block) - else - # Object + Derived Partial - renderer = ObjectRenderer.new(@lookup_context, options) - renderer.render_object_derive_partial(partial, context, block) - end - end - end - def collection_from_options(options) if options.key?(:collection) collection = options[:collection] From a85494dc0cba9f29e40ee910005b343ba8225434 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 12:55:04 -0800 Subject: [PATCH 33/44] Move renderers in to their own files We don't need to keep all of these in the partial renderer file --- actionview/lib/action_view.rb | 4 +- .../renderer/collection_renderer.rb | 176 ++++++++++++++++ .../action_view/renderer/object_renderer.rb | 25 +++ .../action_view/renderer/partial_renderer.rb | 193 ------------------ 4 files changed, 203 insertions(+), 195 deletions(-) create mode 100644 actionview/lib/action_view/renderer/collection_renderer.rb create mode 100644 actionview/lib/action_view/renderer/object_renderer.rb diff --git a/actionview/lib/action_view.rb b/actionview/lib/action_view.rb index fd14094792..a7876e4cfa 100644 --- a/actionview/lib/action_view.rb +++ b/actionview/lib/action_view.rb @@ -51,8 +51,8 @@ module ActionView autoload :Renderer autoload :AbstractRenderer autoload :PartialRenderer - autoload :CollectionRenderer, "action_view/renderer/partial_renderer" - autoload :ObjectRenderer, "action_view/renderer/partial_renderer" + autoload :CollectionRenderer + autoload :ObjectRenderer autoload :TemplateRenderer autoload :StreamingTemplateRenderer end diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb new file mode 100644 index 0000000000..c074a762de --- /dev/null +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -0,0 +1,176 @@ +# frozen_string_literal: true + +require "action_view/renderer/partial_renderer" + +module ActionView + class PartialIteration + # The number of iterations that will be done by the partial. + attr_reader :size + + # The current iteration of the partial. + attr_reader :index + + def initialize(size) + @size = size + @index = 0 + end + + # Check if this is the first iteration of the partial. + def first? + index == 0 + end + + # Check if this is the last iteration of the partial. + def last? + index == size - 1 + end + + def iterate! # :nodoc: + @index += 1 + end + end + + class CollectionRenderer < PartialRenderer # :nodoc: + include ObjectRendering + + class CollectionIterator # :nodoc: + include Enumerable + + attr_reader :collection + + def initialize(collection) + @collection = collection + end + + def each(&blk) + @collection.each(&blk) + end + + def size + @collection.size + end + end + + class SameCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, path, variables) + super(collection) + @path = path + @variables = variables + end + + def from_collection(collection) + return collection if collection == self + self.class.new(collection, @path, @variables) + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + variables = [@path] + @variables + @collection.each { |o| yield(o, variables) } + end + end + + class MixedCollectionIterator < CollectionIterator # :nodoc: + def initialize(collection, paths) + super(collection) + @paths = paths + end + + def each_with_info + return enum_for(:each_with_info) unless block_given? + collection.each_with_index { |o, i| yield(o, @paths[i]) } + end + end + + def render_collection_with_partial(collection, partial, context, block) + @collection = build_collection_iterator(collection, partial, context) + + if @options[:cached] && !partial + raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" + end + + template = find_template(partial, template_keys(partial)) if partial + + if !block && (layout = @options[:layout]) + layout = find_template(layout.to_s, template_keys(partial)) + end + + render_collection(context, template, partial, layout) + end + + def render_collection_derive_partial(collection, context, block) + paths = collection.map { |o| partial_path(o, context) } + + if paths.uniq.length == 1 + # Homogeneous + render_collection_with_partial(collection, paths.first, context, block) + else + render_collection_with_partial(collection, nil, context, block) + end + end + + private + def retrieve_variable(path) + vars = super + variable = vars.first + vars << :"#{variable}_counter" + vars << :"#{variable}_iteration" + vars + end + + def build_collection_iterator(collection, path, context) + if path + SameCollectionIterator.new(collection, path, retrieve_variable(path)) + else + paths = collection.map { |o| partial_path(o, context) } + paths.map! { |path| retrieve_variable(path).unshift(path) } + MixedCollectionIterator.new(collection, paths) + end + end + + def render_collection(view, template, path, layout) + identifier = (template && template.identifier) || path + instrument(:collection, identifier: identifier, count: @collection.size) do |payload| + spacer = if @options.key?(:spacer_template) + spacer_template = find_template(@options[:spacer_template], @locals.keys) + build_rendered_template(spacer_template.render(view, @locals), spacer_template) + else + RenderedTemplate::EMPTY_SPACER + end + + collection_body = if template + cache_collection_render(payload, view, template, @collection) do |collection| + collection_with_template(view, template, layout, collection) + end + else + collection_with_template(view, nil, layout, @collection) + end + + return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? + + build_rendered_collection(collection_body, spacer) + end + end + + def collection_with_template(view, template, layout, collection) + locals = @locals + cache = template || {} + + partial_iteration = PartialIteration.new(collection.size) + + collection.each_with_info.map do |object, (path, as, counter, iteration)| + index = partial_iteration.index + + locals[as] = object + locals[counter] = index + locals[iteration] = partial_iteration + + _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) + content = _template.render(view, locals) + content = layout.render(view, locals) { content } if layout + partial_iteration.iterate! + build_rendered_template(content, _template) + end + end + end +end diff --git a/actionview/lib/action_view/renderer/object_renderer.rb b/actionview/lib/action_view/renderer/object_renderer.rb new file mode 100644 index 0000000000..f0e73a8b7e --- /dev/null +++ b/actionview/lib/action_view/renderer/object_renderer.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module ActionView + class ObjectRenderer < PartialRenderer # :nodoc: + include ObjectRendering + + def render_object_with_partial(object, partial, context, block) + @object = object + render(partial, context, block) + end + + def render_object_derive_partial(object, context, block) + path = partial_path(object, context) + render_object_with_partial(object, path, context, block) + end + + private + + def render_partial_template(view, locals, template, layout, block) + as = template.variable + locals[as] = @object + super(view, locals, template, layout, block) + end + end +end diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index 3c1d1f610c..5a3af00454 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -3,33 +3,6 @@ require "action_view/renderer/partial_renderer/collection_caching" module ActionView - class PartialIteration - # The number of iterations that will be done by the partial. - attr_reader :size - - # The current iteration of the partial. - attr_reader :index - - def initialize(size) - @size = size - @index = 0 - end - - # Check if this is the first iteration of the partial. - def first? - index == 0 - end - - # Check if this is the last iteration of the partial. - def last? - index == size - 1 - end - - def iterate! # :nodoc: - @index += 1 - end - end - # = Action View Partials # # There's also a convenience method for rendering sub templates within the current controller that depends on a @@ -320,170 +293,4 @@ module ActionView @lookup_context.find_template(path, prefixes, true, locals, @details) end end - - class CollectionRenderer < PartialRenderer - include ObjectRendering - - class CollectionIterator # :nodoc: - include Enumerable - - attr_reader :collection - - def initialize(collection) - @collection = collection - end - - def each(&blk) - @collection.each(&blk) - end - - def size - @collection.size - end - end - - class SameCollectionIterator < CollectionIterator # :nodoc: - def initialize(collection, path, variables) - super(collection) - @path = path - @variables = variables - end - - def from_collection(collection) - return collection if collection == self - self.class.new(collection, @path, @variables) - end - - def each_with_info - return enum_for(:each_with_info) unless block_given? - variables = [@path] + @variables - @collection.each { |o| yield(o, variables) } - end - end - - class MixedCollectionIterator < CollectionIterator # :nodoc: - def initialize(collection, paths) - super(collection) - @paths = paths - end - - def each_with_info - return enum_for(:each_with_info) unless block_given? - collection.each_with_index { |o, i| yield(o, @paths[i]) } - end - end - - def render_collection_with_partial(collection, partial, context, block) - @collection = build_collection_iterator(collection, partial, context) - - if @options[:cached] && !partial - raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" - end - - template = find_template(partial, template_keys(partial)) if partial - - if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(partial)) - end - - render_collection(context, template, partial, layout) - end - - def render_collection_derive_partial(collection, context, block) - paths = collection.map { |o| partial_path(o, context) } - - if paths.uniq.length == 1 - # Homogeneous - render_collection_with_partial(collection, paths.first, context, block) - else - render_collection_with_partial(collection, nil, context, block) - end - end - - private - def retrieve_variable(path) - vars = super - variable = vars.first - vars << :"#{variable}_counter" - vars << :"#{variable}_iteration" - vars - end - - def build_collection_iterator(collection, path, context) - if path - SameCollectionIterator.new(collection, path, retrieve_variable(path)) - else - paths = collection.map { |o| partial_path(o, context) } - paths.map! { |path| retrieve_variable(path).unshift(path) } - MixedCollectionIterator.new(collection, paths) - end - end - - def render_collection(view, template, path, layout) - identifier = (template && template.identifier) || path - instrument(:collection, identifier: identifier, count: @collection.size) do |payload| - spacer = if @options.key?(:spacer_template) - spacer_template = find_template(@options[:spacer_template], @locals.keys) - build_rendered_template(spacer_template.render(view, @locals), spacer_template) - else - RenderedTemplate::EMPTY_SPACER - end - - collection_body = if template - cache_collection_render(payload, view, template, @collection) do |collection| - collection_with_template(view, template, layout, collection) - end - else - collection_with_template(view, nil, layout, @collection) - end - - return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? - - build_rendered_collection(collection_body, spacer) - end - end - - def collection_with_template(view, template, layout, collection) - locals = @locals - cache = template || {} - - partial_iteration = PartialIteration.new(collection.size) - - collection.each_with_info.map do |object, (path, as, counter, iteration)| - index = partial_iteration.index - - locals[as] = object - locals[counter] = index - locals[iteration] = partial_iteration - - _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) - content = _template.render(view, locals) - content = layout.render(view, locals) { content } if layout - partial_iteration.iterate! - build_rendered_template(content, _template) - end - end - end - - class ObjectRenderer < PartialRenderer - include ObjectRendering - - def render_object_with_partial(object, partial, context, block) - @object = object - render(partial, context, block) - end - - def render_object_derive_partial(object, context, block) - path = partial_path(object, context) - render_object_with_partial(object, path, context, block) - end - - private - - def render_partial_template(view, locals, template, layout, block) - as = template.variable - locals[as] = @object - super(view, locals, template, layout, block) - end - end end From 76ea105ca1e9c14d0bce9ddb330369792f720438 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 13:14:09 -0800 Subject: [PATCH 34/44] remove collection ivar --- .../lib/action_view/renderer/collection_renderer.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index c074a762de..8b5274a562 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -83,7 +83,7 @@ module ActionView end def render_collection_with_partial(collection, partial, context, block) - @collection = build_collection_iterator(collection, partial, context) + collection = build_collection_iterator(collection, partial, context) if @options[:cached] && !partial raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" @@ -95,7 +95,7 @@ module ActionView layout = find_template(layout.to_s, template_keys(partial)) end - render_collection(context, template, partial, layout) + render_collection(collection, context, template, partial, layout) end def render_collection_derive_partial(collection, context, block) @@ -128,9 +128,9 @@ module ActionView end end - def render_collection(view, template, path, layout) + def render_collection(collection, view, template, path, layout) identifier = (template && template.identifier) || path - instrument(:collection, identifier: identifier, count: @collection.size) do |payload| + instrument(:collection, identifier: identifier, count: collection.size) do |payload| spacer = if @options.key?(:spacer_template) spacer_template = find_template(@options[:spacer_template], @locals.keys) build_rendered_template(spacer_template.render(view, @locals), spacer_template) @@ -139,11 +139,11 @@ module ActionView end collection_body = if template - cache_collection_render(payload, view, template, @collection) do |collection| + cache_collection_render(payload, view, template, collection) do |collection| collection_with_template(view, template, layout, collection) end else - collection_with_template(view, nil, layout, @collection) + collection_with_template(view, nil, layout, collection) end return RenderedCollection.empty(@lookup_context.formats.first) if collection_body.empty? From 266e34973e1e78e487cf035b319ce8e7e3e7e4bd Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 13:15:56 -0800 Subject: [PATCH 35/44] Remove conditional Only one case can possibly be missing a partial. We can move the cache check to that branch and eliminate the conditional --- .../lib/action_view/renderer/collection_renderer.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 8b5274a562..82e61bdf1a 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -85,10 +85,6 @@ module ActionView def render_collection_with_partial(collection, partial, context, block) collection = build_collection_iterator(collection, partial, context) - if @options[:cached] && !partial - raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" - end - template = find_template(partial, template_keys(partial)) if partial if !block && (layout = @options[:layout]) @@ -105,6 +101,10 @@ module ActionView # Homogeneous render_collection_with_partial(collection, paths.first, context, block) else + if @options[:cached] + raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" + end + render_collection_with_partial(collection, nil, context, block) end end From 7c0dea18ce6593416b1242973b0695e06b64d657 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 13:20:34 -0800 Subject: [PATCH 36/44] Reduce iterations for non-homogeneous collections We only need to iterate over the paths twice (this was doing it 3 times) --- .../renderer/collection_renderer.rb | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 82e61bdf1a..76bb6b6b13 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -83,15 +83,9 @@ module ActionView end def render_collection_with_partial(collection, partial, context, block) - collection = build_collection_iterator(collection, partial, context) + collection = SameCollectionIterator.new(collection, partial, retrieve_variable(partial)) - template = find_template(partial, template_keys(partial)) if partial - - if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(partial)) - end - - render_collection(collection, context, template, partial, layout) + render_collection(collection, context, partial, block) end def render_collection_derive_partial(collection, context, block) @@ -105,7 +99,9 @@ module ActionView raise NotImplementedError, "render caching requires a template. Please specify a partial when rendering" end - render_collection_with_partial(collection, nil, context, block) + paths.map! { |path| retrieve_variable(path).unshift(path) } + collection = MixedCollectionIterator.new(collection, paths) + render_collection(collection, context, nil, block) end end @@ -118,17 +114,13 @@ module ActionView vars end - def build_collection_iterator(collection, path, context) - if path - SameCollectionIterator.new(collection, path, retrieve_variable(path)) - else - paths = collection.map { |o| partial_path(o, context) } - paths.map! { |path| retrieve_variable(path).unshift(path) } - MixedCollectionIterator.new(collection, paths) - end - end + def render_collection(collection, view, path, block) + template = find_template(path, template_keys(path)) if path + + if !block && (layout = @options[:layout]) + layout = find_template(layout.to_s, template_keys(path)) + end - def render_collection(collection, view, template, path, layout) identifier = (template && template.identifier) || path instrument(:collection, identifier: identifier, count: collection.size) do |payload| spacer = if @options.key?(:spacer_template) From 78a28b7c4706e65de29e9744e6eecbce10baf633 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 13:52:53 -0800 Subject: [PATCH 37/44] always lookup template from path cache --- actionview/lib/action_view/renderer/collection_renderer.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 76bb6b6b13..39ab7e20b4 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -146,7 +146,7 @@ module ActionView def collection_with_template(view, template, layout, collection) locals = @locals - cache = template || {} + cache = {} partial_iteration = PartialIteration.new(collection.size) @@ -157,7 +157,8 @@ module ActionView locals[counter] = index locals[iteration] = partial_iteration - _template = template || (cache[path] ||= find_template(path, @locals.keys + [as, counter, iteration])) + _template = (cache[path] ||= (template || find_template(path, @locals.keys + [as, counter, iteration]))) + content = _template.render(view, locals) content = layout.render(view, locals) { content } if layout partial_iteration.iterate! From 374c1b29409bd34aea22c38fab4356b7909f93c7 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 14:12:09 -0800 Subject: [PATCH 38/44] fix require --- actionview/test/template/partial_iteration_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actionview/test/template/partial_iteration_test.rb b/actionview/test/template/partial_iteration_test.rb index 1c3c566667..0ed995f7f2 100644 --- a/actionview/test/template/partial_iteration_test.rb +++ b/actionview/test/template/partial_iteration_test.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "abstract_unit" -require "action_view/renderer/partial_renderer" +require "action_view/renderer/collection_renderer" class PartialIterationTest < ActiveSupport::TestCase def test_has_size_and_index From 8d539bde7b7bd2d2347b804f935f7df7a7ce2e15 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 15:01:52 -0800 Subject: [PATCH 39/44] Move around local variable derivation This reduces object allocations in collection rendering --- .../action_view/renderer/abstract_renderer.rb | 9 ++---- .../renderer/collection_renderer.rb | 28 +++++++++---------- .../action_view/renderer/object_renderer.rb | 3 ++ 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index d86265b6c2..797f23b092 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -40,12 +40,8 @@ module ActionView end private - def template_keys(path) - super + retrieve_variable(path) - end - - def retrieve_variable(path) - variable = if as = @options[:as] + def local_variable(path) + if as = @options[:as] raise_invalid_option_as(as) unless /\A[a-z_]\w*\z/.match?(as.to_s) as.to_sym else @@ -55,7 +51,6 @@ module ActionView $1.to_sym end end - [variable] end IDENTIFIER_ERROR_MESSAGE = "The partial name (%s) is not a valid Ruby identifier; " \ diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 39ab7e20b4..5e37efa641 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -83,9 +83,16 @@ module ActionView end def render_collection_with_partial(collection, partial, context, block) - collection = SameCollectionIterator.new(collection, partial, retrieve_variable(partial)) + iter_vars = retrieve_variable(partial) + collection = SameCollectionIterator.new(collection, partial, iter_vars) - render_collection(collection, context, partial, block) + template = find_template(partial, @locals.keys + iter_vars) + + layout = if !block && (layout = @options[:layout]) + find_template(layout.to_s, @locals.keys + iter_vars) + end + + render_collection(collection, context, partial, template, layout, block) end def render_collection_derive_partial(collection, context, block) @@ -101,26 +108,17 @@ module ActionView paths.map! { |path| retrieve_variable(path).unshift(path) } collection = MixedCollectionIterator.new(collection, paths) - render_collection(collection, context, nil, block) + render_collection(collection, context, nil, nil, nil, block) end end private def retrieve_variable(path) - vars = super - variable = vars.first - vars << :"#{variable}_counter" - vars << :"#{variable}_iteration" - vars + variable = local_variable(path) + [variable, :"#{variable}_counter", :"#{variable}_iteration"] end - def render_collection(collection, view, path, block) - template = find_template(path, template_keys(path)) if path - - if !block && (layout = @options[:layout]) - layout = find_template(layout.to_s, template_keys(path)) - end - + def render_collection(collection, view, path, template, layout, block) identifier = (template && template.identifier) || path instrument(:collection, identifier: identifier, count: collection.size) do |payload| spacer = if @options.key?(:spacer_template) diff --git a/actionview/lib/action_view/renderer/object_renderer.rb b/actionview/lib/action_view/renderer/object_renderer.rb index f0e73a8b7e..b6ba1cbc7a 100644 --- a/actionview/lib/action_view/renderer/object_renderer.rb +++ b/actionview/lib/action_view/renderer/object_renderer.rb @@ -15,6 +15,9 @@ module ActionView end private + def template_keys(path) + super + [local_variable(path)] + end def render_partial_template(view, locals, template, layout, block) as = template.variable From 4a4a8be06400f166b0a4062b9ca952a35e62f8d2 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 15:38:20 -0800 Subject: [PATCH 40/44] make the cops happy --- actionview/lib/action_view/renderer/collection_renderer.rb | 6 +++--- actionview/lib/action_view/renderer/renderer.rb | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 5e37efa641..9c4e4409c4 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -89,7 +89,7 @@ module ActionView template = find_template(partial, @locals.keys + iter_vars) layout = if !block && (layout = @options[:layout]) - find_template(layout.to_s, @locals.keys + iter_vars) + find_template(layout.to_s, @locals.keys + iter_vars) end render_collection(collection, context, partial, template, layout, block) @@ -129,8 +129,8 @@ module ActionView end collection_body = if template - cache_collection_render(payload, view, template, collection) do |collection| - collection_with_template(view, template, layout, collection) + cache_collection_render(payload, view, template, collection) do |filtered_collection| + collection_with_template(view, template, layout, filtered_collection) end else collection_with_template(view, nil, layout, collection) diff --git a/actionview/lib/action_view/renderer/renderer.rb b/actionview/lib/action_view/renderer/renderer.rb index d9973c7ebe..b9278adafd 100644 --- a/actionview/lib/action_view/renderer/renderer.rb +++ b/actionview/lib/action_view/renderer/renderer.rb @@ -100,7 +100,6 @@ module ActionView end private - def collection_from_options(options) if options.key?(:collection) collection = options[:collection] From 72e0d04a4bdb56db85e7c5fe4bd61d2162c7ffe5 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Thu, 27 Feb 2020 16:54:50 -0800 Subject: [PATCH 41/44] Fix issue in ActionText ActionText was using `render` in a way that ActionView didn't have tests for. This is a fix for it. --- .../lib/action_view/renderer/object_renderer.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/object_renderer.rb b/actionview/lib/action_view/renderer/object_renderer.rb index b6ba1cbc7a..12c5bb59dc 100644 --- a/actionview/lib/action_view/renderer/object_renderer.rb +++ b/actionview/lib/action_view/renderer/object_renderer.rb @@ -4,8 +4,15 @@ module ActionView class ObjectRenderer < PartialRenderer # :nodoc: include ObjectRendering + def initialize(lookup_context, options) + super + @object = nil + @local_name = nil + end + def render_object_with_partial(object, partial, context, block) - @object = object + @object = object + @local_name = local_variable(partial) render(partial, context, block) end @@ -16,12 +23,11 @@ module ActionView private def template_keys(path) - super + [local_variable(path)] + super + [@local_name] end def render_partial_template(view, locals, template, layout, block) - as = template.variable - locals[as] = @object + locals[@local_name || template.variable] = @object super(view, locals, template, layout, block) end end From 49adb7f4c61de8cbb02c4a207dbd56093d99e795 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 28 Feb 2020 09:23:22 -0800 Subject: [PATCH 42/44] pull preloading behavior in to the collection renderer --- .../renderer/collection_renderer.rb | 26 +++++++++++++- .../activerecord/multifetch_cache_test.rb | 3 -- activerecord/lib/active_record/railtie.rb | 7 ---- .../collection_cache_association_loading.rb | 35 ------------------- 4 files changed, 25 insertions(+), 46 deletions(-) delete mode 100644 activerecord/lib/active_record/railties/collection_cache_association_loading.rb diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index 9c4e4409c4..b175088640 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -70,6 +70,24 @@ module ActionView end end + class PreloadCollectionIterator < SameCollectionIterator # :nodoc: + def initialize(collection, path, variables, relation) + super(collection, path, variables) + relation.skip_preloading! unless relation.loaded? + @relation = relation + end + + def from_collection(collection) + self.class.new(collection, @path, @variables, @relation) + end + + def each_with_info + return super unless block_given? + @relation.preload_associations(@collection) + super + end + end + class MixedCollectionIterator < CollectionIterator # :nodoc: def initialize(collection, paths) super(collection) @@ -84,7 +102,13 @@ module ActionView def render_collection_with_partial(collection, partial, context, block) iter_vars = retrieve_variable(partial) - collection = SameCollectionIterator.new(collection, partial, iter_vars) + + collection = if collection.respond_to?(:preload_associations) + PreloadCollectionIterator.new(collection, partial, iter_vars, collection) + else + SameCollectionIterator.new(collection, partial, iter_vars) + end + template = find_template(partial, @locals.keys + iter_vars) diff --git a/actionview/test/activerecord/multifetch_cache_test.rb b/actionview/test/activerecord/multifetch_cache_test.rb index 3e4782bc86..577615361a 100644 --- a/actionview/test/activerecord/multifetch_cache_test.rb +++ b/actionview/test/activerecord/multifetch_cache_test.rb @@ -1,9 +1,6 @@ # frozen_string_literal: true require "active_record_unit" -require "active_record/railties/collection_cache_association_loading" - -ActionView::CollectionRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) class MultifetchCacheTest < ActiveRecordTestCase fixtures :topics, :replies diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 9bae89a7ae..cef94e3d1c 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -221,13 +221,6 @@ To keep using the current cache store, you can turn off cache versioning entirel end end - initializer "active_record.collection_cache_association_loading" do - require "active_record/railties/collection_cache_association_loading" - ActiveSupport.on_load(:action_view) do - ActionView::CollectionRenderer.prepend(ActiveRecord::Railties::CollectionCacheAssociationLoading) - end - end - initializer "active_record.set_reloader_hooks" do ActiveSupport.on_load(:active_record) do ActiveSupport::Reloader.before_class_unload do diff --git a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb b/activerecord/lib/active_record/railties/collection_cache_association_loading.rb deleted file mode 100644 index 28e84c0563..0000000000 --- a/activerecord/lib/active_record/railties/collection_cache_association_loading.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module ActiveRecord - module Railties # :nodoc: - module CollectionCacheAssociationLoading #:nodoc: - def render_collection_with_partial(collection, partial, context, block) - @relation = nil - - return super unless @options[:cached] - - @relation = get_relation(collection) - - super - end - - def get_relation(relation) - if relation && relation.is_a?(ActiveRecord::Relation) && !relation.loaded? - relation.skip_preloading! - relation - end - end - - def relation_from_options(partial, collection) - relation = partial if partial.is_a?(ActiveRecord::Relation) - relation ||= collection if collection.is_a?(ActiveRecord::Relation) - get_relation(relation) - end - - def collection_with_template(_, _, _, collection) - @relation.preload_associations(collection.collection) if @relation - super - end - end - end -end From a15339ff6527ba2310cb578da2d8e7e0d6b6014e Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Fri, 28 Feb 2020 09:25:25 -0800 Subject: [PATCH 43/44] remove usless attr_reader --- actionview/lib/action_view/renderer/collection_renderer.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/actionview/lib/action_view/renderer/collection_renderer.rb b/actionview/lib/action_view/renderer/collection_renderer.rb index b175088640..c5f69e83d0 100644 --- a/actionview/lib/action_view/renderer/collection_renderer.rb +++ b/actionview/lib/action_view/renderer/collection_renderer.rb @@ -36,8 +36,6 @@ module ActionView class CollectionIterator # :nodoc: include Enumerable - attr_reader :collection - def initialize(collection) @collection = collection end @@ -59,7 +57,6 @@ module ActionView end def from_collection(collection) - return collection if collection == self self.class.new(collection, @path, @variables) end @@ -96,7 +93,7 @@ module ActionView def each_with_info return enum_for(:each_with_info) unless block_given? - collection.each_with_index { |o, i| yield(o, @paths[i]) } + @collection.each_with_index { |o, i| yield(o, @paths[i]) } end end From d7bd84bc1148539b9d5c7a72d7b2c535986e6192 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 23 Mar 2020 17:08:17 -0700 Subject: [PATCH 44/44] Remove out-of-date comments --- .../renderer/partial_renderer/collection_caching.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index 4cf94cd6e2..49011d2f43 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -33,16 +33,8 @@ module ActionView instrumentation_payload[:cache_hits] = cached_partials.size # Extract the items for the keys that are not found - # Set the uncached values to instance variable @collection - # which is used by the caller collection = keyed_collection.reject { |key, _| cached_partials.key?(key) }.values - # If all elements are already in cache then - # rendered partials will be an empty array - # - # If the cache is missing elements then - # the block will be called against the remaining items - # in the @collection. rendered_partials = collection.empty? ? [] : yield(collection_iterator.from_collection(collection)) index = 0