Fix 500 error for xml format when method is not allowed

304 · 304 · commit 27a14c96f33f · 2016-02-22T16:46:20.000+01:00
Problem:

All requests to Not Allowed methods are returning 500 error instead of
405 when the format is xml.

Cause:

Route definitions for not allowed methods are returning an empty string
as a response body. But the empty string fails to convert to xml and
raises InvalidFormatter error.

Solution:

Define an exception for "method is not allowed" case.
Return an error message "405 Not Allowed" instead of empty string.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,7 +11,6 @@
 * [#1252](https://github.com/ruby-grape/grape/pull/1252): Allow default to be a subset or equal to allowed values without raising IncompatibleOptionValues - [@jeradphelps](https://github.com/jeradphelps).
 * [#1255](https://github.com/ruby-grape/grape/pull/1255): Allow param type definition in `route_param` - [@namusyaka](https://github.com/namusyaka).
 * [#1257](https://github.com/ruby-grape/grape/pull/1257): Allow Proc, Symbol or String in `rescue_from with: ...` - [@namusyaka](https://github.com/namusyaka).
-* [#1282](https://github.com/ruby-grape/grape/pull/1282): Fix specs circular dependency - [@304](https://github.com/304).
 * Your contribution here.
 
 #### Fixes
@@ -22,6 +21,8 @@
 * [#1263](https://github.com/ruby-grape/grape/pull/1263): Fix `route :any, '*path'` breaking generated `OPTIONS`, Method Not Allowed routes - [@arempe93](https://github.com/arempe93).
 * [#1266](https://github.com/ruby-grape/grape/pull/1266): Fix `Allow` header including `OPTIONS` when `do_not_route_options!` is active - [@arempe93](https://github.com/arempe93).
 * [#1270](https://github.com/ruby-grape/grape/pull/1270): Fix `param` versioning with a custom parameter - [@wshatch](https://github.com/wshatch).
+* [#1282](https://github.com/ruby-grape/grape/pull/1282): Fix specs circular dependency - [@304](https://github.com/304).
+* [#1283](https://github.com/ruby-grape/grape/pull/1283): Fix 500 error for xml format when method is not allowed - [@304](https://github.com/304).
 
 0.14.0 (12/07/2015)
 ===================
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -72,6 +72,7 @@ module Exceptions
     autoload :InvalidMessageBody
     autoload :InvalidAcceptHeader
     autoload :InvalidVersionHeader
+    autoload :MethodNotAllowed
   end
 
   module ErrorFormatter
diff --git a/lib/grape/api.rb b/lib/grape/api.rb
@@ -185,9 +185,7 @@ def generate_not_allowed_method(path, allowed_methods, allow_header)
       return if not_allowed_methods.empty?
 
       self.class.route(not_allowed_methods, path) do
-        header 'Allow', allow_header
-        status 405
-        ''
+        fail Grape::Exceptions::MethodNotAllowed, header.merge('Allow' => allow_header)
       end
 
       # move options endpoint to top of defined endpoints
diff --git a/lib/grape/exceptions/method_not_allowed.rb b/lib/grape/exceptions/method_not_allowed.rb
@@ -0,0 +1,10 @@
+# encoding: utf-8
+module Grape
+  module Exceptions
+    class MethodNotAllowed < Base
+      def initialize(headers)
+        super(message: '405 Not Allowed', status: 405, headers: headers)
+      end
+    end
+  end
+end
diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -523,10 +523,28 @@ def subject.enable_root_route!
       end
       put '/example'
       expect(last_response.status).to eql 405
-      expect(last_response.body).to eql ''
+      expect(last_response.body).to eql '405 Not Allowed'
       expect(last_response.headers['X-Custom-Header']).to eql 'foo'
     end
 
+    context 'when format is xml' do
+      it 'returns a 405 for an unsupported method' do
+        subject.format :xml
+        subject.get 'example' do
+          'example'
+        end
+
+        put '/example'
+        expect(last_response.status).to eql 405
+        expect(last_response.body).to eq <<-XML
+<?xml version="1.0" encoding="UTF-8"?>
+<error>
+  <message>405 Not Allowed</message>
+</error>
+XML
+      end
+    end
+
     specify '405 responses includes an Allow header specifying supported methods' do
       subject.get 'example' do
         'example'
@@ -602,8 +620,8 @@ def subject.enable_root_route!
         expect(last_response.status).to eql 405
       end
 
-      it 'has an empty body' do
-        expect(last_response.body).to be_blank
+      it 'contains error message in body' do
+        expect(last_response.body).to eq '405 Not Allowed'
       end
 
       it 'has an Allow header' do
