From 8cf21f236343d15d8eab2947da8d9ce9e3c89c87 Mon Sep 17 00:00:00 2001 From: riking Date: Sun, 8 Feb 2015 12:25:03 -0800 Subject: [PATCH 1/4] FEATURE: Refactor error returns in application_controller --- app/controllers/application_controller.rb | 36 ++++++++++++----------- config/locales/server.en.yml | 2 ++ lib/json_error.rb | 18 ++++++++---- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3a94fe59bdf..b876799c9c2 100644 --- a/app/controllers/application_controller.rb +++ b/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,37 @@ 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), 'rate_limit', 429 end rescue_from Discourse::NotLoggedIn do |e| raise e if Rails.env.test? - if request.get? - redirect_to "/" + if (request.format && request.format.json?) || (request.xhr?) + 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'), 'read_only', 405 end - def rescue_discourse_actions(message, error, 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 - # $.parseJSON to fail in the browser. Also returning text like "[error: 'invalid access']" - # from the above rescue_from blocks will fail because that isn't valid json. - render status: error, layout: false, text: (error == 404) ? build_not_found_page(error) : message + def rescue_discourse_actions(type, status_code, include_ember=false) + if (request.format && request.format.json?) || (request.xhr?) + render_json_error I18n.t(type), type, 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 @@ -317,8 +314,13 @@ class ApplicationController < ActionController::Base MultiJson.dump(serializer) end - def render_json_error(obj) - render json: MultiJson.dump(create_errors_json(obj)), status: 422 + # Render action for a JSON error. + # + # obj - a translated string, an ActiveRecord model, or an array of translated strings + # type - a machine-readable description of the error + # status - HTTP status code to return + def render_json_error(obj, type=nil, status=422) + render json: MultiJson.dump(create_errors_json(obj, type)), status: status end def success_json diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8e849dde853..13a82819e10 100644 --- a/config/locales/server.en.yml +++ b/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: diff --git a/lib/json_error.rb b/lib/json_error.rb index 2abc927a9eb..c882c2f472f 100644 --- a/lib/json_error.rb +++ b/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,10 +29,8 @@ module JsonError JsonError.generic_error end - private - - def self.generic_error - {errors: [I18n.t('js.generic_error')]} - end + def self.generic_error + {errors: [I18n.t('js.generic_error')]} + end end From a16aa9fde8378c8ab25693acc6667ab898f703ba Mon Sep 17 00:00:00 2001 From: riking Date: Sun, 8 Feb 2015 13:35:09 -0800 Subject: [PATCH 2/4] HACK: Keep old behavior for topics#show --- app/controllers/application_controller.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b876799c9c2..7436f01bf83 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -99,7 +99,7 @@ class ApplicationController < ActionController::Base rescue_from Discourse::NotLoggedIn do |e| raise e if Rails.env.test? - if (request.format && request.format.json?) || (request.xhr?) + if (request.format && request.format.json?) || request.xhr? || !request.get? rescue_discourse_actions('not_logged_in', 403, true) else redirect_to "/" @@ -120,7 +120,13 @@ class ApplicationController < ActionController::Base end def rescue_discourse_actions(type, status_code, include_ember=false) + if (request.format && request.format.json?) || (request.xhr?) + # HACK: do not use render_json_error for topics#show + if request.params[:controller] == 'topics' && request.params[:action] == 'show' + 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, status_code else render text: build_not_found_page(status_code, include_ember ? 'application' : 'no_ember') From 8d394808311e44e4ff129d6e7ab37d3c04aab64e Mon Sep 17 00:00:00 2001 From: riking Date: Mon, 9 Feb 2015 10:19:47 -0800 Subject: [PATCH 3/4] use symbols for error types (squash me) --- app/controllers/application_controller.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7436f01bf83..2b01001d43c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -93,14 +93,14 @@ class ApplicationController < ActionController::Base time_left = I18n.t("rate_limiter.hours", count: (e.available_in / 1.hour.to_i)) end - render_json_error I18n.t("rate_limiter.too_many_requests", time_left: time_left), 'rate_limit', 429 + render_json_error I18n.t("rate_limiter.too_many_requests", time_left: time_left), :rate_limit, 429 end rescue_from Discourse::NotLoggedIn do |e| raise e if Rails.env.test? if (request.format && request.format.json?) || request.xhr? || !request.get? - rescue_discourse_actions('not_logged_in', 403, true) + rescue_discourse_actions(:not_logged_in, 403, true) else redirect_to "/" end @@ -108,15 +108,15 @@ class ApplicationController < ActionController::Base end rescue_from Discourse::NotFound do - rescue_discourse_actions('not_found', 404) + rescue_discourse_actions(:not_found, 404) end rescue_from Discourse::InvalidAccess do - rescue_discourse_actions('invalid_access', 403, true) + rescue_discourse_actions(:invalid_access, 403, true) end rescue_from Discourse::ReadOnly do - render_json_error I18n.t('read_only_mode_enabled'), 'read_only', 405 + render_json_error I18n.t('read_only_mode_enabled'), :read_only, 405 end def rescue_discourse_actions(type, status_code, include_ember=false) From ecb911285dae65bc516067e26b90f65038e4fd03 Mon Sep 17 00:00:00 2001 From: riking Date: Sun, 22 Feb 2015 21:28:50 -0800 Subject: [PATCH 4/4] Fix the render_json_error api --- app/controllers/application_controller.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 2b01001d43c..e71e729b6f9 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -93,7 +93,7 @@ class ApplicationController < ActionController::Base time_left = I18n.t("rate_limiter.hours", count: (e.available_in / 1.hour.to_i)) end - render_json_error I18n.t("rate_limiter.too_many_requests", time_left: time_left), :rate_limit, 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| @@ -116,7 +116,7 @@ class ApplicationController < ActionController::Base end rescue_from Discourse::ReadOnly do - render_json_error I18n.t('read_only_mode_enabled'), :read_only, 405 + render_json_error I18n.t('read_only_mode_enabled'), type: :read_only, status: 405 end def rescue_discourse_actions(type, status_code, include_ember=false) @@ -127,7 +127,7 @@ class ApplicationController < ActionController::Base 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, status_code + render_json_error I18n.t(type), type: type, status: status_code else render text: build_not_found_page(status_code, include_ember ? 'application' : 'no_ember') end @@ -322,11 +322,15 @@ class ApplicationController < ActionController::Base # Render action for a JSON error. # - # obj - a translated string, an ActiveRecord model, or an array of translated strings - # type - a machine-readable description of the error - # status - HTTP status code to return - def render_json_error(obj, type=nil, status=422) - render json: MultiJson.dump(create_errors_json(obj, type)), status: status + # 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