Formatting from header acts like Versioning from header (#2548)

ericproulx · web-flow · commit 2b35fede4088 · 2025-03-29T17:41:19.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@
 * [#2543](https://github.com/ruby-grape/grape/pull/2543): Fix array allocation on mount - [@ericproulx](https://github.com/ericproulx).
 * [#2546](https://github.com/ruby-grape/grape/pull/2546): Fix middleware with keywords - [@ericproulx](https://github.com/ericproulx).
 * [#2547](https://github.com/ruby-grape/grape/pull/2547): Remove jsonapi related code - [@ericproulx](https://github.com/ericproulx).
+* [#2548](https://github.com/ruby-grape/grape/pull/2548): Formatting from header acts like versioning from header - [@ericproulx](https://github.com/ericproulx).
 * Your contribution here.
 
 ### 2.3.0 (2025-02-08)
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -6,6 +6,25 @@ Upgrading Grape
 - Passing a class to `build_with` or `Grape.config.param_builder` has been deprecated in favor of a symbolized short_name. See `SHORTNAME_LOOKUP` in [params_builder](lib/grape/params_builder.rb).
 - Including Grape's extensions like `Grape::Extensions::Hashie::Mash::ParamBuilder` has been deprecated in favor of using `build_with` at the route level.
 
+#### Accept Header Negotiation Harmonized
+
+[Accept](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept) header is now fully interpreted through `Rack::Utils.best_q_match` which is following [RFC2616 14.1](https://datatracker.ietf.org/doc/html/rfc2616#section-14.1). Since [Grape 2.1.0](https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md#210-20240615), the [header versioning strategy](https://github.com/ruby-grape/grape?tab=readme-ov-file#header) was adhering to it, but `Grape::Middleware::Formatter` never did.
+
+Your API might act differently since it will strictly follow the [RFC2616 14.1](https://datatracker.ietf.org/doc/html/rfc2616#section-14.1) when interpreting the `Accept` header. Here are the differences:
+
+###### Invalid or missing quality ranking
+The following used to yield `application/xml` and now will yield `application/json` as the preferred media type:
+- `application/json;q=invalid,application/xml;q=0.5`
+- `application/json,application/xml;q=1.0`
+
+For the invalid case, the value `invalid` was automatically `to_f` and `invalid.to_f` equals `0.0`. Now, since it doesn't match [Rack's regex](https://github.com/rack/rack/blob/3-1-stable/lib/rack/utils.rb#L138), its interpreted as non provided and its quality ranking equals 1.0.
+
+For the non provided case, 1.0 was automatically assigned and in a case of multiple best matches, the first was returned based on Ruby's sort_by `quality`. Now, 1.0 is still assigned and the last is returned in case of multiple best matches. See [Rack's implementation](https://github.com/rack/rack/blob/e8f47608668d507e0f231a932fa37c9ca551c0a5/lib/rack/utils.rb#L167) of the RFC.
+
+###### Considering the closest generic when vendor tree
+Excluding the [header versioning strategy](https://github.com/ruby-grape/grape?tab=readme-ov-file#header), whenever a media type with the [vendor tree](https://datatracker.ietf.org/doc/html/rfc6838#section-3.2) leading facet `vnd.` like `application/vnd.api+json` was provided, Grape would also consider its closest generic when negotiating. In that case, `application/json` was added to the negotiation. Now, it will just consider the provided media types without considering any closest generics, and you'll need to [register](https://github.com/ruby-grape/grape?tab=readme-ov-file#api-formats) it.
+You can find the official vendor tree registrations on [IANA](https://www.iana.org/assignments/media-types/media-types.xhtml)
+
 ### Upgrading to >= 2.4.0
 
 #### Custom Validators
diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb
@@ -122,56 +122,31 @@ def read_rack_input(body)
       def negotiate_content_type
         fmt = format_from_extension || format_from_params || options[:format] || format_from_header || options[:default_format]
         if content_type_for(fmt)
-          env[Grape::Env::API_FORMAT] = fmt
+          env[Grape::Env::API_FORMAT] = fmt.to_sym
         else
           throw :error, status: 406, message: "The requested format '#{fmt}' is not supported."
         end
       end
 
       def format_from_extension
-        parts = request.path.split('.')
+        request_path = request.path.try(:scrub)
+        dot_pos = request_path.rindex('.')
+        return unless dot_pos
 
-        if parts.size > 1
-          extension = parts.last
-          # avoid symbol memory leak on an unknown format
-          return extension.to_sym if content_type_for(extension)
-        end
-        nil
+        extension = request_path[dot_pos + 1..]
+        extension if content_type_for(extension)
       end
 
       def format_from_params
-        fmt = Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[FORMAT]
-        # avoid symbol memory leak on an unknown format
-        return fmt.to_sym if content_type_for(fmt)
-
-        fmt
+        Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING])[FORMAT]
       end
 
       def format_from_header
-        mime_array.each do |t|
-          return mime_types[t] if mime_types.key?(t)
-        end
-        nil
-      end
-
-      def mime_array
-        accept = env[Grape::Http::Headers::HTTP_ACCEPT]
-        return [] unless accept
-
-        accept_into_mime_and_quality = %r{
-          (
-            \w+/[\w+.-]+)     # eg application/vnd.example.myformat+xml
-          (?:
-           (?:;[^,]*?)?       # optionally multiple formats in a row
-           ;\s*q=([\w.]+)     # optional "quality" preference (eg q=0.5)
-          )?
-        }x
-
-        vendor_prefix_pattern = /vnd\.[^+]+\+/
+        accept_header = env[Grape::Http::Headers::HTTP_ACCEPT].try(:scrub)
+        return if accept_header.blank?
 
-        accept.scan(accept_into_mime_and_quality)
-              .sort_by { |_, quality_preference| -(quality_preference ? quality_preference.to_f : 1.0) }
-              .flat_map { |mime, _| [mime, mime.sub(vendor_prefix_pattern, '')] }
+        media_type = Rack::Utils.best_q_match(accept_header, mime_types.keys)
+        mime_types[media_type] if media_type
       end
     end
   end
diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb
@@ -82,6 +82,12 @@ def to_xml
   end
 
   context 'detection' do
+    context 'when path contains invalid byte sequence' do
+      it 'does not raise an exception' do
+        expect { subject.call(Rack::PATH_INFO => "/info.\x80") }.not_to raise_error
+      end
+    end
+
     it 'uses the xml extension if one is provided' do
       subject.call(Rack::PATH_INFO => '/info.xml')
       expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
@@ -95,8 +101,6 @@ def to_xml
     it 'uses the format parameter if one is provided' do
       subject.call(Rack::PATH_INFO => '/info', Rack::QUERY_STRING => 'format=json')
       expect(subject.env[Grape::Env::API_FORMAT]).to eq(:json)
-      subject.call(Rack::PATH_INFO => '/info', Rack::QUERY_STRING => 'format=xml')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
     end
 
     it 'uses the default format if none is provided' do
@@ -116,6 +120,12 @@ def to_xml
   end
 
   context 'accept header detection' do
+    context 'when header contains invalid byte sequence' do
+      it 'does not raise an exception' do
+        expect { subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => "Hello \x80") }.not_to raise_error
+      end
+    end
+
     it 'detects from the Accept header' do
       subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/xml')
       expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
@@ -131,10 +141,10 @@ def to_xml
 
     it 'handles quality rankings mixed with nothing' do
       subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/json,application/xml; q=1.0')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:json)
+      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
 
       subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/xml; q=1.0,application/json')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
+      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:json)
     end
 
     it 'handles quality rankings that have a default 1.0 value' do
@@ -156,30 +166,21 @@ def to_xml
       expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
     end
 
-    it 'ignores invalid quality rankings' do
-      subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/json;q=invalid,application/xml;q=0.5')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
-      subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/xml;q=0.5,application/json;q=invalid')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
-
-      subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/json;q=,application/xml;q=0.5')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:json)
-
-      subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/json;q=nil,application/xml;q=0.5')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
-    end
-
-    it 'parses headers with vendor and api version' do
-      subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test-v1+xml')
-      expect(subject.env[Grape::Env::API_FORMAT]).to eq(:xml)
-    end
-
     context 'with custom vendored content types' do
-      subject { described_class.new(app, content_types: { custom: 'application/vnd.test+json' }) }
+      context 'when registered' do
+        subject { described_class.new(app, content_types: { custom: 'application/vnd.test+json' }) }
 
-      it 'uses the custom type' do
-        subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')
-        expect(subject.env[Grape::Env::API_FORMAT]).to eq(:custom)
+        it 'uses the custom type' do
+          subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')
+          expect(subject.env[Grape::Env::API_FORMAT]).to eq(:custom)
+        end
+      end
+
+      context 'when unregistered' do
+        it 'returns the default content type text/plain' do
+          r = Rack::MockResponse[*subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')]
+          expect(r.headers[Rack::CONTENT_TYPE]).to eq('text/plain')
+        end
       end
     end
 
@@ -216,11 +217,6 @@ def to_xml
       _, headers, = s.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')
       expect(headers[Rack::CONTENT_TYPE]).to eq('application/vnd.test+json')
     end
-
-    it 'is set to closest generic for custom vendored/versioned without registered type' do
-      _, headers, = subject.call(Rack::PATH_INFO => '/info', Grape::Http::Headers::HTTP_ACCEPT => 'application/vnd.test+json')
-      expect(headers[Rack::CONTENT_TYPE]).to eq('application/json')
-    end
   end
 
   context 'format' do
