Merge pull request #1765 from jdmurphy/415_status

dblock · web-flow · commit 2dcef9af4dd4 · 2018-06-06T09:29:35.000-04:00
Use 415 status for request bodies of unsupported media type
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,4 +1,4 @@
-### 1.0.4 (Next)
+### 1.1.0 (Next)
 
 #### Features
 
@@ -10,6 +10,7 @@
 * [#1762](https://github.com/ruby-grape/grape/pull/1763): Fix unsafe HTML rendering on errors - [@ctennis](https://github.com/ctennis).
 * [#1759](https://github.com/ruby-grape/grape/pull/1759): Update appraisal for rails_edge - [@zvkemp](https://github.com/zvkemp).
 * [#1758](https://github.com/ruby-grape/grape/pull/1758): Fix expanding load_path in gemspec - [@2maz](https://github.com/2maz).
+* [#1765](https://github.com/ruby-grape/grape/pull/1765): Use 415 when request body is of an unsupported media type - [@jdmurphy](https://github.com/jdmurphy).
 * Your contribution here.
 
 ### 1.0.3 (4/23/2018)
diff --git a/README.md b/README.md
@@ -145,7 +145,7 @@ content negotiation, versioning and much more.
 
 ## Stable Release
 
-You're reading the documentation for the next release of Grape, which should be **1.0.4**.
+You're reading the documentation for the next release of Grape, which should be **1.1.0**.
 Please read [UPGRADING](UPGRADING.md) when upgrading from a previous version.
 The current stable release is [1.0.3](https://github.com/ruby-grape/grape/blob/v1.0.3/README.md).
 
@@ -2551,6 +2551,9 @@ Built-in formatters are the following.
 * `:serializable_hash`: use object's `serializable_hash` when available, otherwise fallback to `:json`
 * `:binary`: data will be returned "as is"
 
+If a body is present in a request to an API, with a Content-Type header value that is of an unsupported type a
+"415 Unsupported Media Type" error code will be returned by Grape.
+
 Response statuses that indicate no content as defined by [Rack](https://github.com/rack)
 [here](https://github.com/rack/rack/blob/master/lib/rack/utils.rb#L567)
 will bypass serialization and the body entity - though there should be none -
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,12 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 1.1.0
+
+#### Changes in HTTP Response Code for Unsupported Content Type
+
+For PUT, POST, PATCH, and DELETE requests where a non-empty body and a "Content-Type" header is supplied that is not supported by the Grape API, Grape will no longer return a 406 "Not Acceptable" HTTP status code and will instead return a 415 "Unsupported Media Type" so that the usage of HTTP status code falls more in line with the specification of [RFC 2616](https://www.ietf.org/rfc/rfc2616.txt).
+
 ### Upgrading to >= 1.0.0
 
 #### Changes in XML and JSON Parsers
diff --git a/gemfiles/multi_json.gemfile b/gemfiles/multi_json.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/multi_xml.gemfile b/gemfiles/multi_xml.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rack_1.5.2.gemfile b/gemfiles/rack_1.5.2.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rack_edge.gemfile b/gemfiles/rack_edge.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rails_3.gemfile b/gemfiles/rails_3.gemfile
@@ -23,7 +23,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rails_4.gemfile b/gemfiles/rails_4.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rails_5.gemfile b/gemfiles/rails_5.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/gemfiles/rails_edge.gemfile b/gemfiles/rails_edge.gemfile
@@ -22,7 +22,7 @@ end
 group :test do
   gem 'cookiejar'
   gem 'coveralls', '~> 0.8.17', require: false
-  gem 'danger-toc', '~> 0.1.0'
+  gem 'danger-toc', '~> 0.1.2'
   gem 'grape-entity', '~> 0.6'
   gem 'maruku'
   gem 'mime-types'
diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb
@@ -95,7 +95,7 @@ def read_rack_input(body)
         fmt = request.media_type ? mime_types[request.media_type] : options[:default_format]
 
         unless content_type_for(fmt)
-          throw :error, status: 406, message: "The requested content-type '#{request.media_type}' is not supported."
+          throw :error, status: 415, message: "The provided content-type '#{request.media_type}' is not supported."
         end
         parser = Grape::Parser.parser_for fmt, options
         if parser
diff --git a/lib/grape/version.rb b/lib/grape/version.rb
@@ -1,4 +1,4 @@
 module Grape
   # The current version of Grape.
-  VERSION = '1.0.4'.freeze
+  VERSION = '1.1.0'.freeze
 end
diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb
@@ -941,15 +941,15 @@ def app
       end
     end
 
-    it 'responds with a 406 for an unsupported content-type' do
+    it 'responds with a 415 for an unsupported content-type' do
       subject.format :json
       # subject.content_type :json, "application/json"
       subject.put '/request_body' do
         params[:user]
       end
       put '/request_body', '<user>Bobby T.</user>', 'CONTENT_TYPE' => 'application/xml'
-      expect(last_response.status).to eq(406)
-      expect(last_response.body).to eq('{"error":"The requested content-type \'application/xml\' is not supported."}')
+      expect(last_response.status).to eq(415)
+      expect(last_response.body).to eq('{"error":"The provided content-type \'application/xml\' is not supported."}')
     end
 
     it 'does not accept text/plain in JSON format if application/json is specified as content type' do
@@ -960,8 +960,8 @@ def app
       end
       put '/request_body', ::Grape::Json.dump(user: 'Bob'), 'CONTENT_TYPE' => 'text/plain'
 
-      expect(last_response.status).to eq(406)
-      expect(last_response.body).to eq('{"error":"The requested content-type \'text/plain\' is not supported."}')
+      expect(last_response.status).to eq(415)
+      expect(last_response.body).to eq('{"error":"The provided content-type \'text/plain\' is not supported."}')
     end
 
     context 'content type with params' do
diff --git a/spec/grape/middleware/formatter_spec.rb b/spec/grape/middleware/formatter_spec.rb
@@ -224,6 +224,80 @@ def to_xml
 
   context 'input' do
     %w[POST PATCH PUT DELETE].each do |method|
+      context 'when body is not nil or empty' do
+        context 'when Content-Type is supported' do
+          let(:io) { StringIO.new('{"is_boolean":true,"string":"thing"}') }
+          let(:content_type) { 'application/json' }
+
+          it "parses the body from #{method} and copies values into rack.request.form_hash" do
+            subject.call(
+              'PATH_INFO' => '/info',
+              'REQUEST_METHOD' => method,
+              'CONTENT_TYPE' => content_type,
+              'rack.input' => io,
+              'CONTENT_LENGTH' => io.length
+            )
+            expect(subject.env['rack.request.form_hash']['is_boolean']).to be true
+            expect(subject.env['rack.request.form_hash']['string']).to eq('thing')
+          end
+        end
+
+        context 'when Content-Type is not supported' do
+          let(:io) { StringIO.new('{"is_boolean":true,"string":"thing"}') }
+          let(:content_type) { 'application/atom+xml' }
+
+          it 'returns a 415 HTTP error status' do
+            error = catch(:error) {
+              subject.call(
+                'PATH_INFO' => '/info',
+                'REQUEST_METHOD' => method,
+                'CONTENT_TYPE' => content_type,
+                'rack.input' => io,
+                'CONTENT_LENGTH' => io.length
+              )
+            }
+            expect(error[:status]).to eq(415)
+            expect(error[:message]).to eq("The provided content-type 'application/atom+xml' is not supported.")
+          end
+        end
+      end
+
+      context 'when body is nil' do
+        let(:io) { double }
+        before do
+          allow(io).to receive_message_chain(:rewind, :read).and_return(nil)
+        end
+
+        it 'does not read and parse the body' do
+          expect(subject).not_to receive(:read_rack_input)
+          subject.call(
+            'PATH_INFO' => '/info',
+            'REQUEST_METHOD' => method,
+            'CONTENT_TYPE' => 'application/json',
+            'rack.input' => io,
+            'CONTENT_LENGTH' => 0
+          )
+        end
+      end
+
+      context 'when body is empty' do
+        let(:io) { double }
+        before do
+          allow(io).to receive_message_chain(:rewind, :read).and_return('')
+        end
+
+        it 'does not read and parse the body' do
+          expect(subject).not_to receive(:read_rack_input)
+          subject.call(
+            'PATH_INFO' => '/info',
+            'REQUEST_METHOD' => method,
+            'CONTENT_TYPE' => 'application/json',
+            'rack.input' => io,
+            'CONTENT_LENGTH' => 0
+          )
+        end
+      end
+
       ['application/json', 'application/json; charset=utf-8'].each do |content_type|
         context content_type do
           it "parses the body from #{method} and copies values into rack.request.form_hash" do
