LG-9103: capture error when ArcGIS service is not returning a token

[racingspider]

When ArcGIS is unable to return a token it causes an exception in retrieve_token and throws HTTP 500 responses. The UI has no indication of error and the http 500 responses cause the on-call engineer to be notified.

This change catches the missing token, logs the issue, and returns a 400 error which allows a retry.

changelog: Internal, PO-Search, Return 400 instead of 500 and logs ArcGIS token issues

[aduth]

Can we add some test coverage for these changes?

[sheldon-b]

I have a few thoughts here

1. I don't think deleting the cache is necessary. The cached value has an expiration included so it'll automatically expire when it needs to. I expect that when this method gets called the cache will already be cleared
2. I like that we log an analytics event here, that seems to satisfy the ticket's AC. I think we should name the event something more specific though since we're only handling auth request failures. Maybe analytics.idv_in_person_arcgis_auth_token_failure? Also, you'll need to go define that method on the analytics class in the app/services/analytics_events.rb file
3. We should try to log data about the error response we got from ArcGIS. Right now we're just logging hard-coded values of 400, even if the actual response was a 503 or a timeout error. I expect something like this (note you'll need to rescue the error to a variable, like rescue StandardError => err)

analytics.idv_in_person_arcgis_auth_token_failure(
        exception_class: err.class.to_s,
        exception_message: err.message,
        response_status_code: err?.response_status,
      )

5. I think it would be good to re-raise the exception after we log the error message. That way execution will stop (right now this will continue to try to make a request without an auth token) and roll back up to the controller, which can return a 400 to the UI and display an error to the user. You can re-raise the exception by using raise

[sheldon-b]

Oh, another thing occurred to me. The production issue that led to this ticket revealed that when ArcGIS rejects our authentication it doesn't actually return any sort of error HTTP status code. It returns a successful status code but the body contains error information. And then this method raised an exception because it was trying to access the token key in the JSON response, but there was no token key

All this to say: we should continue catching exceptions (that will handle 4xx and 5xx errors) but we should also inspect the payload and log a message if there is not a token key. These two cases probably deserve different error handling logic; when there is no token key we can also log message and details from the payload

I'm basing this all off of that one slack thread. I wonder if the ArcGIS docs define what keys we can expect from error payloads -- that would be a good thing to check when implementing this error handling. I know Matt has encountered quite a few ArcGIS errors and is familiar with the docs

[NavaTim]

@sheldon-b @racingspider I noticed while looking over this that the retrieve_token! function directly uses the expiration sent back from ArcGIS (without subtracting any time), which means that some calls will inevitably be made after the token expires. I'm going to create another story to handle that scenario.

I wonder if the ArcGIS docs define what keys we can expect from error payloads -- that would be a good thing to check when implementing this error handling.

The error handling page for ArcGIS REST JS suggests that ArcGIS has relatively stable forms of output which indicate errors in the ArcGIS REST APIs. Some of the details can be inferred by looking at the arcgis-rest-request package code. I expect the codes returned may be those in the ArcGIS REST APIs / Error codes list.

I don't think deleting the cache is necessary.

I agree, it shouldn't be necessary to delete cache for most kinds of errors that occur here. If we're doing a re-auth request, then the cache should already be empty.

I think we should name the event something more specific though since we're only handling auth request failures.

That or add the request URL path the the data logged here. I do agree about making sure we can narrow this down to which type of call is associated with the error.

[zachmargolis]

parse_token does not seem like a particularly useful method, since it just returns what it takes in, we could inline it with a tap since it seems to only use side effects

Suggested change

	parse_token(
	faraday.post(
	IdentityConfig.store.arcgis_api_generate_token_url, URI.encode_www_form(body)
	) do |req|
	req.options.context = { service_name: 'arcgis_token' }
	end.body,
	)
	faraday.post(
	IdentityConfig.store.arcgis_api_generate_token_url, URI.encode_www_form(body)
	) do |req|
	req.options.context = { service_name: 'arcgis_token' }
	end.body.tap do |body|
	handle_api_errors(body)
	end

[racingspider]

Taking both bits of feedback, I'm going to essentially re-raise the error and have the controller properly handle the error instead of gobbling it up at this lower level. I generally err to resolving errors at the lower levels, but in this case at the controller level should suffice ('talking' out loud here).

[NavaTim]

@racingspider Can you inspect this cache clear more closely to verify if it's needed for these cases, and open a PR to remove it if not?