Skip to content

Handle unexpected ArcGIS response body #8970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tomas-nava merged 6 commits into from
Aug 9, 2023
Merged

Handle unexpected ArcGIS response body #8970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ac41738
return 422 instead of 400 on ArcGIS errors
Aug 9, 2023
4981f53
re-indent case conditionals
Aug 9, 2023
a9fa4a4
handle a malformed response body
Aug 9, 2023
ee58a91
status code should be nil not false when missing
Aug 9, 2023
45e1df6
add new test for invalid token
Aug 9, 2023
a46cced
use try
Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions app/controllers/idv/in_person/address_search_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ def geocoder

def report_errors(error)
remapped_error = case error
when Faraday::Error,
ActionController::InvalidAuthenticityToken
:bad_request
else
:internal_server_error
end
when Faraday::Error,
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally, I like the less-indented version of this the case/when statements, I'm guessing our rubocop allows both

Copy link
Copy Markdown
Contributor Author

@tomas-nava tomas-nava Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote the statements I'm changing here :) Yes, rubocop allows both, and I find the left-justfied version confusing.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if rubocop allows both, author's choice I think (until we can figure out how to get it to enforce one or the other)

ActionController::InvalidAuthenticityToken
:unprocessable_entity
else
:internal_server_error
end

errors = if error.respond_to?(:response_body)
error.response_body && error.response_body[:details]
error.response_body.is_a?(Hash) && error.response_body[:details]
end

errors ||= 'ArcGIS error performing operation'
Expand All @@ -50,7 +50,7 @@ def report_errors(error)
api_status_code: Rack::Utils.status_code(remapped_error),
exception_class: error.class,
exception_message: error.message,
response_status_code: error.respond_to?(:response_status) && error.response_status,
response_status_code: error.try(:response_status),
)

analytics.idv_arcgis_request_failure(
Expand All @@ -59,7 +59,7 @@ def report_errors(error)
exception_message: error.message,
response_body_present: error.respond_to?(:response_body) && error.response_body.present?,
response_body: error.respond_to?(:response_body) && error.response_body,
response_status_code: error.respond_to?(:response_status) && error.response_status,
response_status_code: error.try(:response_status),
)
render json: [], status: remapped_error
end
Expand Down
12 changes: 6 additions & 6 deletions app/controllers/idv/in_person/usps_locations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,12 @@ def add_proofing_component

def handle_error(err)
remapped_error = case err
when ActionController::InvalidAuthenticityToken,
Faraday::Error
:unprocessable_entity
else
:internal_server_error
end
when ActionController::InvalidAuthenticityToken,
Faraday::Error
:unprocessable_entity
else
:internal_server_error
end

analytics.idv_in_person_locations_request_failure(
api_status_code: Rack::Utils.status_code(remapped_error),
Expand Down
67 changes: 58 additions & 9 deletions spec/controllers/idv/in_person/address_search_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,14 @@
'message' => 'Unable to complete operation.',
} }
end
let(:parsed_response_body) { { details: response_body['error']['details'].join(', ') } }

before do
exception = Faraday::ClientError.new(
RuntimeError.new(response_body['error']['message']),
{
status: response_body['error']['code'],
body: { details: response_body['error']['details'].join(', ') },
body: parsed_response_body,
},
)
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
Expand All @@ -86,7 +87,7 @@
expect(addresses.length).to eq 0
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 400,
api_status_code: 422,
success: false,
errors: 'request is too many characters',
result_total: 0,
Expand All @@ -95,18 +96,38 @@
response_status_code: 400,
)
end

context 'with malformed response body details' do
let(:parsed_response_body) { response_body['error']['details'] }

it 'logs analytics event' do
response = get :index
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 422,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
exception_class: Faraday::ClientError,
exception_message: 'Unable to complete operation.',
response_status_code: 400,
)
end
end
end
end

context 'with unsuccessful fetch' do
context 'with connection failed exception' do
before do
exception = Faraday::ConnectionFailed.new('connection failed')
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
end

it 'gets an empty pilot response' do
response = get :index
expect(response.status).to eq(400)
expect(response.status).to eq(422)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
end
Expand All @@ -115,7 +136,7 @@
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 400,
api_status_code: 422,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
Expand All @@ -126,6 +147,34 @@
end
end

context 'with invalid authenticity token exception' do
before do
exception = ActionController::InvalidAuthenticityToken.new('invalid token')
allow(geocoder).to receive(:find_address_candidates).and_raise(exception)
end

it 'gets an empty pilot response' do
response = get :index
expect(response.status).to eq(422)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0
end

it 'logs search analytics' do
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 422,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
exception_class: ActionController::InvalidAuthenticityToken,
exception_message: 'invalid token',
response_status_code: nil,
)
end
end

context 'with a timeout error' do
let(:server_error) { Faraday::TimeoutError.new }

Expand All @@ -137,7 +186,7 @@

it 'returns an error code' do
response = get :index
expect(response.status).to eq(400)
expect(response.status).to eq(422)
addresses = JSON.parse(response.body)
expect(addresses.length).to eq 0

Expand All @@ -157,7 +206,7 @@
response
expect(@analytics).to have_logged_event(
'IdV: in person proofing location search submitted',
api_status_code: 400,
api_status_code: 422,
success: false,
errors: 'ArcGIS error performing operation',
result_total: 0,
Expand Down Expand Up @@ -191,7 +240,7 @@
result_total: 0,
exception_class: StandardError,
exception_message: 'error',
response_status_code: false,
response_status_code: nil,
)
end

Expand All @@ -202,7 +251,7 @@
api_status_code: 500,
exception_class: StandardError,
exception_message: 'error',
response_status_code: false,
response_status_code: nil,
response_body_present: false,
response_body: false,
)
Expand Down