From 06d2c2d15bd520bdb3126fda63e5154d395240fb Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 8 May 2023 10:10:27 +0900 Subject: [PATCH 1/3] Refactor ActionView::Template::Types to avoid delegation The `Type` class was introduced in https://github.com/rails/rails/pull/23085 for the sole purpose of breaking the dependency of Action View on Action Dispatch. Unless you are somehow running Action View standalone, this is actually never used. So instead of delegating, we can use constant swapping, this saves us a useless layer. Ultimately we could consider moving `Mime::Types` into Active Support but it requires some more thoughts. --- actionmailer/lib/action_mailer.rb | 2 +- actionpack/lib/action_dispatch.rb | 2 +- .../lib/action_dispatch/http/mime_type.rb | 4 ++ actionview/lib/action_view/template.rb | 12 ++++ actionview/lib/action_view/template/types.rb | 65 +++++++------------ .../test/template/asset_tag_helper_test.rb | 2 +- 6 files changed, 44 insertions(+), 43 deletions(-) diff --git a/actionmailer/lib/action_mailer.rb b/actionmailer/lib/action_mailer.rb index 996740408b..891fe11101 100644 --- a/actionmailer/lib/action_mailer.rb +++ b/actionmailer/lib/action_mailer.rb @@ -73,6 +73,6 @@ def self.eager_load! ActiveSupport.on_load(:action_view) do ActionView::Base.default_formats ||= Mime::SET.symbols - ActionView::Template::Types.delegate_to Mime + ActionView::Template.mime_types_implementation = Mime ActionView::LookupContext::DetailsKey.clear end diff --git a/actionpack/lib/action_dispatch.rb b/actionpack/lib/action_dispatch.rb index 712e9a4014..37b2b54edc 100644 --- a/actionpack/lib/action_dispatch.rb +++ b/actionpack/lib/action_dispatch.rb @@ -147,6 +147,6 @@ def eager_load! ActiveSupport.on_load(:action_view) do ActionView::Base.default_formats ||= Mime::SET.symbols - ActionView::Template::Types.delegate_to Mime + ActionView::Template.mime_types_implementation = Mime ActionView::LookupContext::DetailsKey.clear end diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 8553655d73..1bcf3007a2 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -42,6 +42,10 @@ def [](type) Type.lookup_by_extension(type) end + def symbols + SET.symbols + end + def fetch(type, &block) return type if type.is_a?(Type) EXTENSION_LOOKUP.fetch(type.to_s, &block) diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 607cdb71f2..50f2b547cf 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -109,6 +109,7 @@ class Template autoload :Handlers autoload :HTML autoload :Inline + autoload :Types autoload :Sources autoload :Text autoload :Types @@ -119,6 +120,17 @@ class Template singleton_class.attr_accessor :frozen_string_literal @frozen_string_literal = false + class << self # :nodoc: + def mime_types_implementation=(implementation) + # This method isn't thread-safe, but it's not supposed + # to be called after initialization + if self::Types != implementation + remove_const(:Types) + const_set(:Types, implementation) + end + end + end + attr_reader :identifier, :handler attr_reader :variable, :format, :variant, :virtual_path diff --git a/actionview/lib/action_view/template/types.rb b/actionview/lib/action_view/template/types.rb index dfb6c0999f..07bda733ce 100644 --- a/actionview/lib/action_view/template/types.rb +++ b/actionview/lib/action_view/template/types.rb @@ -4,53 +4,38 @@ module ActionView class Template # :nodoc: - module Types - class Type - SET = Struct.new(:symbols).new([ :html, :text, :js, :css, :xml, :json ]) + class SimpleType # :nodoc: + SET = Struct.new(:symbols).new([ :html, :text, :js, :css, :xml, :json ]) - def self.[](type) - if type.is_a?(self) - type - else - new(type) - end - end - - attr_reader :symbol - - def initialize(symbol) - @symbol = symbol.to_sym - end - - def to_s - @symbol.to_s - end - alias to_str to_s - - def ref - @symbol - end - alias to_sym ref - - def ==(type) - @symbol == type.to_sym unless type.blank? + def self.[](type) + if type.is_a?(self) + type + else + new(type) end end - class << self - attr_reader :symbols + attr_reader :symbol - def delegate_to(klass) - @symbols = klass::SET.symbols - @type_klass = klass - end - - def [](type) - @type_klass[type] - end + def initialize(symbol) + @symbol = symbol.to_sym end - delegate_to Type + def to_s + @symbol.to_s + end + alias to_str to_s + + def ref + @symbol + end + alias to_sym ref + + def ==(type) + @symbol == type.to_sym unless type.blank? + end end + + Types = SimpleType # :nodoc: end end diff --git a/actionview/test/template/asset_tag_helper_test.rb b/actionview/test/template/asset_tag_helper_test.rb index 11fe27869f..ae61e42fec 100644 --- a/actionview/test/template/asset_tag_helper_test.rb +++ b/actionview/test/template/asset_tag_helper_test.rb @@ -4,7 +4,7 @@ require "active_support/ordered_options" require "action_dispatch" -ActionView::Template::Types.delegate_to Mime +ActionView::Template.mime_types_implementation = Mime module AssetTagHelperTestHelpers def with_preload_links_header(new_preload_links_header = true) From 583afa1404e7d1a1c4d166f1e02292c9d30872f2 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 8 May 2023 10:34:55 +0900 Subject: [PATCH 2/3] Optimize mime types validation in ActionView::LookupContext Fix: https://github.com/rails/rails/issues/48156 The assumption here is that in the overwhelming majority of cases, all formats are valid. So we first check if any of the formats is invalid before duping the details hash and filtering them. Additonally, by exposing a (non-public) `valid_symbols?` method, we can check symbols are valid without resporting to `Array#%` which would needlessly duplicate the `formats` array. --- .../lib/action_dispatch/http/mime_type.rb | 17 +++++++++++++-- actionview/lib/action_view/lookup_context.rb | 10 +++++---- actionview/lib/action_view/template/types.rb | 21 +++++++++++++------ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 1bcf3007a2..55da88cf96 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -11,6 +11,7 @@ class Mimes def initialize @mimes = [] @symbols = [] + @symbols_set = Set.new end def each(&block) @@ -19,17 +20,25 @@ def each(&block) def <<(type) @mimes << type - @symbols << type.to_sym + sym_type = type.to_sym + @symbols << sym_type + @symbols_set << sym_type end def delete_if @mimes.delete_if do |x| if yield x - @symbols.delete(x.to_sym) + sym_type = x.to_sym + @symbols.delete(sym_type) + @symbols_set.delete(sym_type) true end end end + + def valid_symbols?(symbols) # :nodoc + symbols.all? { |s| @symbols_set.include?(s) } + end end SET = Mimes.new @@ -46,6 +55,10 @@ def symbols SET.symbols end + def valid_symbols?(symbols) # :nodoc: + SET.valid_symbols?(symbols) + end + def fetch(type, &block) return type if type.is_a?(Type) EXTENSION_LOOKUP.fetch(type.to_s, &block) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 952da0b9a5..e8dcb26e44 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -63,9 +63,11 @@ def self.digest_cache(details) end def self.details_cache_key(details) - if details[:formats] - details = details.dup - details[:formats] &= Template::Types.symbols + if formats = details[:formats] + unless Template::Types.valid_symbols?(formats) + details = details.dup + details[:formats] &= Template::Types.symbols + end end @details_keys[details] ||= TemplateDetails::Requested.new(**details) end @@ -262,7 +264,7 @@ def formats=(values) values.concat(default_formats) if values.delete "*/*" values.uniq! - unless values.all? { |v| Template::Types.symbols.include?(v) } + unless Template::Types.valid_symbols?(values) invalid_values = values - Template::Types.symbols raise ArgumentError, "Invalid formats: #{invalid_values.map(&:inspect).join(", ")}" end diff --git a/actionview/lib/action_view/template/types.rb b/actionview/lib/action_view/template/types.rb index 07bda733ce..fdddb20a54 100644 --- a/actionview/lib/action_view/template/types.rb +++ b/actionview/lib/action_view/template/types.rb @@ -4,14 +4,23 @@ module ActionView class Template # :nodoc: + # SimpleType is mostly just a stub implementation for when Action View + # is used without Action Dispatch. class SimpleType # :nodoc: - SET = Struct.new(:symbols).new([ :html, :text, :js, :css, :xml, :json ]) + @symbols = [ :html, :text, :js, :css, :xml, :json ] + class << self + attr_reader :symbols - def self.[](type) - if type.is_a?(self) - type - else - new(type) + def [](type) + if type.is_a?(self) + type + else + new(type) + end + end + + def valid_symbols?(symbols) # :nodoc + symbols.all? { |s| @symbols.include?(s) } end end From 4c6bc6c908371f1c887d8595108558d071dce567 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Mon, 8 May 2023 10:49:33 +0900 Subject: [PATCH 3/3] Optimize ActionView::LookupContext::DetailsKey Assuming the overwhelming majority of the time `details[:formats]` is all valid, we can first check the cache with the unmofidied `details` and then only if we miss we filter the valid formats. --- actionview/lib/action_view/lookup_context.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index e8dcb26e44..04e0207a0f 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -63,13 +63,15 @@ def self.digest_cache(details) end def self.details_cache_key(details) - if formats = details[:formats] - unless Template::Types.valid_symbols?(formats) - details = details.dup - details[:formats] &= Template::Types.symbols + @details_keys.fetch(details) do + if formats = details[:formats] + unless Template::Types.valid_symbols?(formats) + details = details.dup + details[:formats] &= Template::Types.symbols + end end + @details_keys[details] ||= TemplateDetails::Requested.new(**details) end - @details_keys[details] ||= TemplateDetails::Requested.new(**details) end def self.clear