diff --git a/CHANGELOG.md b/CHANGELOG.md index e5fd3e1ee..3e450bc1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,6 +95,7 @@ * [#3038](https://github.com/ruby-grape/grape/pull/3038): Hold every method to RuboCop's default ABC size - [@ericproulx](https://github.com/ericproulx). * [#3039](https://github.com/ruby-grape/grape/pull/3039): Hold every module to RuboCop's default length - [@ericproulx](https://github.com/ericproulx). * [#3031](https://github.com/ruby-grape/grape/pull/3031): Define `InheritableSetting`'s scalar settings and scope flags by key - [@ericproulx](https://github.com/ericproulx). +* [#3032](https://github.com/ruby-grape/grape/pull/3032): Keep each API's content type order when negotiating a browser's `*/*` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.1 (2026-09-15) diff --git a/lib/grape/content_types.rb b/lib/grape/content_types.rb index bc008e065..6669af7a5 100644 --- a/lib/grape/content_types.rb +++ b/lib/grape/content_types.rb @@ -19,10 +19,16 @@ def content_types_for(from_settings) from_settings.presence || DEFAULTS end + # Identity rather than +==+ for the defaults, and the pairs rather than + # the registry for the cache key, because Hash equality ignores order and + # this table's order is the API's preference: the +*/*+ every browser + # sends picks the first media type declared. Compared by value, the first + # registry built with a set of content types fixed that order for every + # later one declaring the same set in another order. def mime_types_for(from_settings) - return MIME_TYPES if from_settings == Grape::ContentTypes::DEFAULTS + return MIME_TYPES if from_settings.equal?(Grape::ContentTypes::DEFAULTS) - MimeTypesCache[from_settings] + MimeTypesCache[from_settings.to_a] end # Every format under both spellings in one plain Hash, so a lookup is a @@ -53,8 +59,9 @@ def media_type(content_type) # Both tables below are derived from nothing but the content-type registry, # and every endpoint builds its own content-type-aware middlewares — so an # app held one copy per endpoint of tables it only ever reads. Keying the - # cache on the registry collapses them: Hash keys compare by value, so - # every registry with the same content types shares one table. + # cache on the registry collapses them: keys compare by value, so every + # registry with the same content types shares one table -- declared in + # the same order, for MimeTypesCache (see #mime_types_for). # # Both the key and the table are frozen: the caller's registry stays # reachable (through +middleware.options[:content_types]+, among others) @@ -64,8 +71,8 @@ def media_type(content_type) 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 + @cache = Hash.new do |h, content_types| + h[content_types.freeze] = content_types.to_h.invert.transform_keys! { |mime_type| Grape::ContentTypes.media_type(mime_type) }.freeze end end end diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 0a7ea7706..9acd20455 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -3407,6 +3407,25 @@ def self.call(error:, **) expect(last_response.status).to eq(406) expect(last_response.body).to eq(Rack::Utils.escape_html("The requested format 'txt' is not supported.")) end + + it "answers a browser's */* in the type it declares first, whatever order another API declared the same types in" do + browser_accept = 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' + json_first = Class.new(described_class) do + content_type :json, 'application/json' + content_type :txt, 'text/plain' + get(:page) { 'page' } + end + txt_first = Class.new(described_class) do + content_type :txt, 'text/plain' + content_type :json, 'application/json' + get(:page) { 'page' } + end + + content_types = [json_first, txt_first].map do |api| + Rack::MockRequest.new(api).get('/page', 'HTTP_ACCEPT' => browser_accept).content_type + end + expect(content_types).to eq(%w[application/json text/plain]) + end end describe '.formatter' do diff --git a/spec/grape/content_types_spec.rb b/spec/grape/content_types_spec.rb index c2aa7d72b..985d3cb4d 100644 --- a/spec/grape/content_types_spec.rb +++ b/spec/grape/content_types_spec.rb @@ -51,20 +51,20 @@ describe '.mime_types_for' do subject { described_class.mime_types_for(from_settings) } - context 'when from_settings is equal to Grape::ContentTypes::DEFAULTS' do - let(:from_settings) do - { - xml: 'application/xml', - serializable_hash: 'application/json', - json: 'application/json', - binary: 'application/octet-stream', - txt: 'text/plain' - }.freeze - end + context 'when from_settings is Grape::ContentTypes::DEFAULTS' do + let(:from_settings) { described_class::DEFAULTS } it { is_expected.to be(described_class::MIME_TYPES) } end + context 'when from_settings holds the default content types in another order' do + let(:from_settings) { described_class::DEFAULTS.to_a.reverse.to_h } + + it 'keeps that order' do + expect(subject.keys).to eq(%w[text/plain application/octet-stream application/json application/xml]) + end + end + context 'when from_settings is not equal to Grape::ContentTypes::DEFAULTS' do let(:from_settings) do { @@ -79,6 +79,12 @@ 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 + + it 'keeps the order of each registry declaring the same content types' do + txt_first = described_class.mime_types_for({ txt: 'text/plain', json: 'application/json' }) + json_first = described_class.mime_types_for({ json: 'application/json', txt: 'text/plain' }) + expect([txt_first.keys, json_first.keys]).to eq([%w[text/plain application/json], %w[application/json text/plain]]) + end end end