Merge pull request #3183 from riking/json-errors-2

Consolidate custom exception handling

app/controllers/application_controller.rb

@@ -76,7 +76,7 @@ class ApplicationController < ActionController::Base
   # Some exceptions
   class RenderEmpty < Exception; end
 
-  # Render nothing unless we are an xhr request
+  # Render nothing
   rescue_from RenderEmpty do
     render 'default/empty'
   end
@@ -93,40 +93,43 @@ class ApplicationController < ActionController::Base
       time_left = I18n.t("rate_limiter.hours", count: (e.available_in / 1.hour.to_i))
     end
 
-    render json: {errors: [I18n.t("rate_limiter.too_many_requests", time_left: time_left)]}, status: 429
+    render_json_error I18n.t("rate_limiter.too_many_requests", time_left: time_left), type: :rate_limit, status: 429
   end
 
   rescue_from Discourse::NotLoggedIn do |e|
     raise e if Rails.env.test?
 
-    if request.get?
+    if (request.format && request.format.json?) || request.xhr? || !request.get?
-      redirect_to "/"
+      rescue_discourse_actions(:not_logged_in, 403, true)
     else
-      render status: 403, json: failed_json.merge(message: I18n.t(:not_logged_in))
+      redirect_to "/"
     end
 
   end
 
   rescue_from Discourse::NotFound do
-    rescue_discourse_actions("[error: 'not found']", 404) # TODO: this breaks json responses
+    rescue_discourse_actions(:not_found, 404)
   end
 
   rescue_from Discourse::InvalidAccess do
-    rescue_discourse_actions("[error: 'invalid access']", 403, true) # TODO: this breaks json responses
+    rescue_discourse_actions(:invalid_access, 403, true)
   end
 
   rescue_from Discourse::ReadOnly do
-    render status: 405, json: failed_json.merge(message: I18n.t("read_only_mode_enabled"))
+    render_json_error I18n.t('read_only_mode_enabled'), type: :read_only, status: 405
   end
 
-  def rescue_discourse_actions(message, error, include_ember=false)
+  def rescue_discourse_actions(type, status_code, include_ember=false)
-    if request.format && request.format.json?
+
-      # TODO: this doesn't make sense. Stuffing an html page into a json response will cause
+    if (request.format && request.format.json?) || (request.xhr?)
-      #       $.parseJSON to fail in the browser. Also returning text like "[error: 'invalid access']"
+      # HACK: do not use render_json_error for topics#show
-      #       from the above rescue_from blocks will fail because that isn't valid json.
+      if request.params[:controller] == 'topics' && request.params[:action] == 'show'
-      render status: error, layout: false, text: (error == 404) ? build_not_found_page(error) : message
+        return render status: status_code, layout: false, text: (status_code == 404) ? build_not_found_page(status_code) : I18n.t(type)
+      end
+
+      render_json_error I18n.t(type), type: type, status: status_code
     else
-      render text: build_not_found_page(error, include_ember ? 'application' : 'no_ember')
+      render text: build_not_found_page(status_code, include_ember ? 'application' : 'no_ember')
     end
   end
 
@@ -318,8 +321,17 @@ class ApplicationController < ActionController::Base
       MultiJson.dump(serializer)
     end
 
-    def render_json_error(obj)
+    # Render action for a JSON error.
-      render json: MultiJson.dump(create_errors_json(obj)), status: 422
+    #
+    # obj      - a translated string, an ActiveRecord model, or an array of translated strings
+    # opts:
+    #   type   - a machine-readable description of the error
+    #   status - HTTP status code to return
+    def render_json_error(obj, opts={})
+      if opts.is_a? Fixnum
+        opts = {status: opts}
+      end
+      render json: MultiJson.dump(create_errors_json(obj, opts[:type])), status: opts[:status] || 422
     end
 
     def success_json

config/locales/server.en.yml

@@ -118,6 +118,8 @@ en:
     not_enough_space_on_disk: "There is not enough space on disk to upload this backup."
 
   not_logged_in: "You need to be logged in to do that."
+  not_found: "The requested URL or resource could not be found."
+  invalid_access: "You are not permitted to view the requested resource."
   read_only_mode_enabled: "The site is in read only mode. Interactions are disabled."
 
   too_many_replies:

lib/json_error.rb

@@ -1,6 +1,14 @@
 module JsonError
 
-  def create_errors_json(obj)
+  def create_errors_json(obj, type=nil)
+    errors = create_errors_array obj
+    errors[:error_type] = type if type
+    errors
+  end
+
+  private
+
+  def create_errors_array(obj)
 
     # If we're passed a string, assume that is the error message
     return {errors: [obj]} if obj.is_a?(String)
@@ -21,8 +29,6 @@ module JsonError
     JsonError.generic_error
   end
 
-  private
-
   def self.generic_error
     {errors: [I18n.t('js.generic_error')]}
   end