Permalink
Browse files

Remove Api::BaseController#error_during_processing

Previously Api::BaseController would swallow all exceptions (or all
StandardError as of Solidus 1.0) and silently render an exception using
error_during_processing.

This has long hid errors in specs (see the last few commits for
examples) as well as in production usage.

This removes the rescue_from, the error_during_processing method, and
the configurable error_notifier. Instead, errors bubble up and are
handled by Rails middleware, as they should be.

This allows exceptions to be seen in API specs, stack traces to be seen
in development, and errors to be reported to error collection services
in production.
  • Loading branch information...
jhawthorn committed Aug 9, 2017
1 parent 9fbf271 commit 4c57b317004b17ff46dd97d1ac3d33e9f1661bd1
@@ -16,14 +16,11 @@ class BaseController < ActionController::Base

attr_accessor :current_api_user

class_attribute :error_notifier

before_action :load_user
before_action :authorize_for_order, if: proc { order_token.present? }
before_action :authenticate_user
before_action :load_user_roles

rescue_from StandardError, with: :error_during_processing
rescue_from ActionController::ParameterMissing, with: :parameter_missing_error
rescue_from ActiveRecord::RecordNotFound, with: :not_found
rescue_from CanCan::AccessDenied, with: :unauthorized
@@ -68,15 +65,6 @@ def unauthorized
render "spree/api/errors/unauthorized", status: 401
end

def error_during_processing(exception)
Rails.logger.error exception.message
Rails.logger.error exception.backtrace.join("\n")

error_notifier.call(exception, self) if error_notifier

render json: { exception: exception.message }, status: 422
end

def gateway_error(exception)
@order.errors.add(:base, exception.message)
invalid_resource!(@order)
@@ -62,66 +62,10 @@ def index
end
end

it 'chatches StandardError' do
expect(subject).to receive(:authenticate_user).and_return(true)
expect(subject).to receive(:load_user_roles).and_return(true)
expect(subject).to receive(:index).and_raise("no joy")
get :index, params: { token: "fake_key" }
expect(json_response).to eq({ "exception" => "no joy" })
expect(response.content_type).to eq("application/json")
end

it 'raises Exception' do
expect(subject).to receive(:authenticate_user).and_return(true)
expect(subject).to receive(:load_user_roles).and_return(true)
expect(subject).to receive(:index).and_raise(Exception.new("no joy"))
expect {
get :index, params: { token: "fake_key" }
}.to raise_error(Exception, "no joy")
end

it "lets a subclass override the product associations that are eager-loaded" do
expect(controller.respond_to?(:product_includes, true)).to be
end

describe '#error_during_processing' do
controller(FakesController) do
# GET /foo
# Simulates a failed API call.
def foo
raise StandardError
end
end

# What would be placed in config/initializers/spree.rb
Spree::Api::BaseController.error_notifier = proc do |e, controller|
MockHoneybadger.notify_or_ignore(e, rack_env: controller.request.env)
end

##
# Fake HB alert class
class MockHoneybadger
# https://github.com/honeybadger-io/honeybadger-ruby/blob/master/lib/honeybadger.rb#L136
def self.notify_or_ignore(_exception, _opts = {})
end
end

before do
user = double(email: "spree@example.com")
allow(user).to receive_message_chain :spree_roles, pluck: []
allow(Spree.user_class).to receive_messages find_by: user
@routes = ActionDispatch::Routing::RouteSet.new.tap do |r|
r.draw { get 'foo' => 'fakes#foo' }
end
end

it 'should notify notify_error_during_processing' do
expect(MockHoneybadger).to receive(:notify_or_ignore).once.with(kind_of(Exception), rack_env: kind_of(Hash))
get :foo, params: { token: 123 }
expect(response.status).to eq(422)
end
end

context 'insufficient stock' do
before do
expect(subject).to receive(:authenticate_user).and_return(true)

0 comments on commit 4c57b31

Please sign in to comment.