diff --git a/CHANGELOG.md b/CHANGELOG.md index d4b51316a..a7de01391 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ * [#2882](https://github.com/ruby-grape/grape/pull/2882): Strip mount path and prefix in `Grape::Middleware::Versioner::Path` with `each` instead of `Enumerable#reduce` - [@ericproulx](https://github.com/ericproulx). * [#2881](https://github.com/ruby-grape/grape/pull/2881): Decide whether a request carries a body from the env, so `Grape::Middleware::Formatter` no longer builds a `Rack::Request` for GET, HEAD and OPTIONS - [@ericproulx](https://github.com/ericproulx). * [#2883](https://github.com/ruby-grape/grape/pull/2883): Resolve a format's content type through a plain Hash carrying both spellings instead of a `HashWithIndifferentAccess` that converts the key on every read - [@ericproulx](https://github.com/ericproulx). +* [#2885](https://github.com/ruby-grape/grape/pull/2885): Share the content-type lookup and mime-type tables between middleware instances that register the same content types, instead of building a copy per API - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/content_types.rb b/lib/grape/content_types.rb index ae11c6d32..c84734f09 100644 --- a/lib/grape/content_types.rb +++ b/lib/grape/content_types.rb @@ -22,7 +22,20 @@ def content_types_for(from_settings) def mime_types_for(from_settings) return MIME_TYPES if from_settings == Grape::ContentTypes::DEFAULTS - from_settings.invert.transform_keys! { |k| media_type(k) } + MimeTypesCache[from_settings] + end + + # Every format under both spellings in one plain Hash, so a lookup is a + # single +Hash#[]+. +HashWithIndifferentAccess+ converted the key on every + # read instead, and this is read two or three times per request — to + # negotiate the format, and again to set the response content type. + # + # Keys arrive as Symbols: the +content_type+ DSL symbolizes what it is + # given and the defaults are Symbols. The key is stored as it came too, + # so a middleware constructed directly with String keys still answers to + # either spelling, as the indifferent hash did. + def lookup_for(from_settings) + LookupCache[from_settings] end # The media type of a content-type header: the part before any `;` @@ -36,5 +49,38 @@ def media_type(content_type) base = content_type.include?(';') ? content_type.split(';', 2).first : content_type base.strip end + + # Both tables below are derived from nothing but the content-type registry, + # and one content-type-aware middleware is built per API instance — so an + # app mounting N APIs held N copies of tables it only ever reads. Keying + # the cache on the registry itself collapses them: Hash keys compare by + # value, so every API that registers the same content types shares one + # table. + # + # Both the key and the table are frozen: the caller's registry stays + # reachable (through +middleware.options[:content_types]+, among others) + # and mutating a live key would corrupt a cache that is now shared + # process-wide. + + class MimeTypesCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, from_settings| + h[from_settings.dup.freeze] = from_settings.invert.transform_keys! { |mime_type| Grape::ContentTypes.media_type(mime_type) }.freeze + end + end + end + + class LookupCache < Grape::Util::Cache + def initialize + super + @cache = Hash.new do |h, from_settings| + h[from_settings.dup.freeze] = from_settings.each_with_object({}) do |(format, media_type), lookup| + lookup[format] = media_type + lookup[format.is_a?(String) ? format.to_sym : format.to_s] = media_type + end.freeze + end + end + end end end diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index f40c8bc39..75eda0349 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -27,6 +27,14 @@ def initialize(content_types: nil, default_format: :txt, format: nil, formatters def_delegators :config, :default_format, :format, :formatters, :parsers + # The formatter is the only middleware that maps an incoming media type + # back to a format, so it warms +mime_types+ itself rather than making + # every content-type-aware middleware build a table none of them read. + def initialize(app, **options) + super + mime_types + end + def before negotiate_content_type read_body_input diff --git a/lib/grape/middleware/precomputed_content_types.rb b/lib/grape/middleware/precomputed_content_types.rb index 828c6ab50..1ce79e0e3 100644 --- a/lib/grape/middleware/precomputed_content_types.rb +++ b/lib/grape/middleware/precomputed_content_types.rb @@ -10,13 +10,18 @@ module Middleware # at initialization so per-request +dup+s inherit them rather than # rebuilding them. # + # +mime_types+ is not warmed here: Formatter is the only middleware that + # reads it, and it warms it itself. The tables behind +mime_types+ and + # +content_type_for+ are shared process-wide per content-type registry + # (see Grape::ContentTypes), so the ivars below memoize a lookup, not a + # copy. + # # Opt-in: plain +Grape::Middleware::Base+ subclasses that don't need # content-type-aware helpers don't pay for them. module PrecomputedContentTypes def initialize(app, **options) super content_types - mime_types content_types_lookup end @@ -38,20 +43,8 @@ def content_type private - # Every format under both spellings in one plain Hash, so a lookup is a - # single +Hash#[]+. +HashWithIndifferentAccess+ converted the key on every - # read instead, and this is read two or three times per request — to - # negotiate the format, and again to set the response content type. - # - # Keys arrive as Symbols: the +content_type+ DSL symbolizes what it is - # given and the defaults are Symbols. The key is stored as it came too, - # so a middleware constructed directly with String keys still answers to - # either spelling, as the indifferent hash did. def content_types_lookup - @content_types_lookup ||= content_types.each_with_object({}) do |(format, media_type), lookup| - lookup[format] = media_type - lookup[format.is_a?(String) ? format.to_sym : format.to_s] = media_type - end.freeze + @content_types_lookup ||= Grape::ContentTypes.lookup_for(content_types) end end end diff --git a/spec/grape/content_types_spec.rb b/spec/grape/content_types_spec.rb index e148dc883..c2aa7d72b 100644 --- a/spec/grape/content_types_spec.rb +++ b/spec/grape/content_types_spec.rb @@ -73,6 +73,46 @@ end it { is_expected.to eq('application/xml' => :xml) } + + it { is_expected.to be_frozen } + + it 'returns the same table for an equal registry' do + expect(subject).to be(described_class.mime_types_for({ xml: 'application/xml;charset=utf-8' })) + end + end + end + + describe '.lookup_for' do + subject { described_class.lookup_for(from_settings) } + + let(:from_settings) { { json: 'application/json' } } + + it 'holds every format under both spellings' do + expect(subject).to eq(json: 'application/json', 'json' => 'application/json') + end + + it { is_expected.to be_frozen } + + context 'with String keys' do + let(:from_settings) { { 'json' => 'application/json' } } + + it 'answers to either spelling' do + expect(subject).to eq('json' => 'application/json', json: 'application/json') + end + end + + context 'when another registry has the same content types' do + it 'returns the same table' do + expect(subject).to be(described_class.lookup_for({ json: 'application/json' })) + end + end + + context 'when the registry is mutated after being looked up' do + it 'keeps answering the original registry' do + subject + from_settings[:xml] = 'application/xml' + expect(described_class.lookup_for({ json: 'application/json' })).to eq(json: 'application/json', 'json' => 'application/json') + end end end