Merge pull request #525 from dblock/default-error-status-500

dblock · dblock · commit 608191e66a08 · 2013-12-04T07:07:13.000-08:00
Default error status 500
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,8 @@ Next Release
 
 * [#510](https://github.com/intridea/grape/pull/510): Support lambda-based default values for params - [@myitcv](https://github.com/myitcv).
 * [#511](https://github.com/intridea/grape/pull/511): Add `required` option for OAuth2 middleware - [@bcm](https://github.com/bcm).
+* [#520](https://github.com/intridea/grape/pull/520): Use `default_error_status` to specify the default status code returned from `error!` - [@salimane](https://github.com/salimane).
+* [#525](https://github.com/intridea/grape/pull/525): The default status code returned from `error!` has been changed from 403 to 500 - [@dblock](https://github.com/dblock).
 * Your contribution here.
 
 #### Fixes
diff --git a/README.md b/README.md
@@ -688,6 +688,19 @@ instead of a message.
 error!({ "error" => "unexpected error", "detail" => "missing widget" }, 500)
 ```
 
+### Default Error HTTP Status Code
+
+By default Grape returns a 500 status code from `error!`. You can change this with `default_error_status`.
+
+``` ruby
+class API < Grape::API
+  default_error_status 400
+  get '/example' do
+    error! "This should have http status code 400"
+  end
+end
+```
+
 ### Handling 404
 
 For Grape to handle all the 404s for your API, it can be useful to use a catch-all.
diff --git a/lib/grape/endpoint.rb b/lib/grape/endpoint.rb
@@ -202,8 +202,9 @@ def version
     # end user with the specified message.
     #
     # @param message [String] The message to display.
-    # @param status [Integer] the HTTP Status Code. Defaults to 403.
-    def error!(message, status = 403)
+    # @param status [Integer] the HTTP Status Code. Defaults to default_error_status, 500 if not set.
+    def error!(message, status = nil)
+      status = settings[:default_error_status] unless status
       throw :error, message: message, status: status
     end
 
@@ -405,7 +406,7 @@ def build_middleware
       b.use Rack::Head
       b.use Grape::Middleware::Error,
             format: settings[:format],
-            default_status: settings[:default_error_status] || 403,
+            default_status: settings[:default_error_status] || 500,
             rescue_all: settings[:rescue_all],
             rescued_errors: aggregate_setting(:rescued_errors),
             default_error_formatter: settings[:default_error_formatter],
diff --git a/lib/grape/middleware/error.rb b/lib/grape/middleware/error.rb
@@ -5,7 +5,7 @@ module Middleware
     class Error < Base
       def default_options
         {
-          default_status: 403, # default status returned on error
+          default_status: 500, # default status returned on error
           default_message: "",
           format: :txt,
           formatters: {},
diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb
@@ -993,7 +993,7 @@ def three
         raise "rain!"
       end
       get '/exception'
-      last_response.status.should eql 403
+      last_response.status.should eql 500
     end
 
     it 'rescues only certain errors if rescue_from is called with specific errors' do
@@ -1002,7 +1002,7 @@ def three
       subject.get('/unrescued') { raise "beefcake" }
 
       get '/rescued'
-      last_response.status.should eql 403
+      last_response.status.should eql 500
 
       lambda { get '/unrescued' }.should raise_error
     end
@@ -1477,7 +1477,16 @@ def self.call(object, env)
         raise "rain!"
       end
       get '/exception'
-      last_response.status.should eql 403
+      last_response.status.should eql 500
+    end
+    it 'uses the default error status in error!' do
+      subject.rescue_from :all
+      subject.default_error_status 400
+      subject.get '/exception' do
+        error! "rain!"
+      end
+      get '/exception'
+      last_response.status.should eql 400
     end
   end
 
diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb
@@ -464,7 +464,7 @@ def app
       end
 
       get '/hey'
-      last_response.status.should == 403
+      last_response.status.should == 500
       last_response.body.should == "This is not valid."
     end
 
diff --git a/spec/grape/middleware/error_spec.rb b/spec/grape/middleware/error_spec.rb
@@ -31,10 +31,10 @@ def app
     last_response.body.should == 'Awesome stuff.'
   end
 
-  it 'defaults to a 403 status' do
+  it 'defaults to a 500 status' do
     ErrApp.error = {}
     get '/'
-    last_response.status.should == 403
+    last_response.status.should == 500
   end
 
   it 'has a default message' do
diff --git a/spec/grape/middleware/exception_spec.rb b/spec/grape/middleware/exception_spec.rb
@@ -15,7 +15,7 @@ def call(env)
   # raises a hash error
   class ErrorHashApp
     class << self
-      def error!(message, status = 403)
+      def error!(message, status)
         throw :error, message: { error: message, detail: "missing widget" }, status: status
       end
 
@@ -28,7 +28,7 @@ def call(env)
   # raises an error!
   class AccessDeniedApp
     class << self
-      def error!(message, status = 403)
+      def error!(message, status)
         throw :error, message: message, status: status
       end
 
@@ -70,13 +70,13 @@ def call(env)
       last_response.body.should == "rain!"
     end
 
-    it 'defaults to a 403 status' do
+    it 'defaults to a 500 status' do
       @app ||= Rack::Builder.app do
         use Grape::Middleware::Error, rescue_all: true
         run ExceptionApp
       end
       get '/'
-      last_response.status.should == 403
+      last_response.status.should == 500
     end
 
     it 'is possible to specify a different default status code' do

Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@ module Middleware`
`5`	`5`	`class Error < Base`
`6`	`6`	`def default_options`
`7`	`7`	`{`
`8`		`- default_status: 403, # default status returned on error`
	`8`	`+ default_status: 500, # default status returned on error`
`9`	`9`	`default_message: "",`
`10`	`10`	`format: :txt,`
`11`	`11`	`formatters: {},`