-
Notifications
You must be signed in to change notification settings - Fork 166
LG-9449: Prevent Location Search slowdown and errors from ArcGIS auth #8579
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.
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
Changes from all commits
df007b7
c253ded
144f2c5
d96e22d
1dfabbc
0c4e05c
ec69df9
8535941
73a8ee9
187bc92
ef74c4b
7857c2f
29196e7
ef8c07c
4a1154a
6562861
7d4f6ae
a6a8caa
2d7e545
feb10fa
69b8f4f
b980494
18a931f
6379866
fe2428e
d7024d6
5fb9933
d9ad542
b903397
df19d25
70cd2b7
0f6d3a8
d932d18
d7a98f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| class ArcgisTokenJob < ApplicationJob | ||
| queue_as :default | ||
|
|
||
| def perform | ||
| analytics.idv_arcgis_token_job_started | ||
| token_entry = token_keeper.retrieve_token | ||
| token_keeper.save_token(token_entry, token_entry.expires_at) | ||
| return true | ||
| ensure | ||
| analytics.idv_arcgis_token_job_completed | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def token_keeper | ||
| ArcgisApi::TokenKeeper.new | ||
| end | ||
|
|
||
| def analytics | ||
| @analytics ||= Analytics.new( | ||
| user: AnonymousUser.new, | ||
| request: nil, | ||
| session: {}, | ||
| sp: nil, | ||
| ) | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| module ArcgisApi | ||
| # Stateless and thread-safe factory object that create a new Faraday::Connection object. | ||
| class ConnectionFactory | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this class? If there a reason we couldn't reuse
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To easily inject a mock one that stub request on connection for testing purpose, also customize faraday middleware behaviours.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW there is test support that allows you to simulate HTTP interactions, making mocks of HTTP libraries like Faraday unnecessary (see the ArcgisApiHelper). That said I like the separation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @allthesignals , good point, this was to mock on faraday level, webmock covers much more. It was adapted to the mock geocoder used in feature test. Let me c whether I can handle it better.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ambivalent to the existence of this class. There are two effective usages of this (excluding |
||
| # @param [String|URI] url | ||
| # @options [Hash] Faraday connection options | ||
| # @return Faraday::Connection | ||
| def connection(url = nil, options = {}) | ||
| conn_options = options ? options.dup : {} | ||
| Faraday.new(url, conn_options) do |conn| | ||
| # Log request metrics | ||
| conn.request :instrumentation, name: 'request_metric.faraday' | ||
| conn.options.timeout = IdentityConfig.store.arcgis_api_request_timeout_seconds | ||
| # Parse JSON responses | ||
| conn.response :json, content_type: 'application/json' | ||
| # Raise an error subclassing Faraday::Error on 4xx, 5xx, and malformed responses | ||
| # Note: The order of this matters for parsing the error response body. | ||
| conn.response :raise_error | ||
| yield conn if block_given? | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| module ArcgisApi | ||
| # Faraday middleware to raise exception when response status is 200 but with error in body | ||
| class ResponseValidation < Faraday::Middleware | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to keep the response logic closer to where the request is made?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mitchellhenke when a new connection is created on demand, this middleware is registered with the connection for response processing.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, sorry, I understand how it works, but if this is specific to one API, could we pull it into the class that's making those requests rather than separating it across files?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mitchellhenke , we use farady-retry for automatic retrying, if we handle this after response returned to client, I think it's too late to trigger retry with farady-retry.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's possible to have similar logic for retries when making the request, and I think it'd be better to move it there instead of having a separate class.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mitchellhenke not exactly, for invalid response with 200 http status code, once the response is returned to caller, it's out of faraday middleware chain, and wont be able to utilize faraday-retry gem which works as a faraday middleware and we have to write our own retry mechanisms(bookkeeping, counting, making new requests the whole nine yard that's implemented by farady-retry).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mitchellhenke I think that using this as a separate middleware class is acceptable given the way ESRI itself handles this in its arcgis-rest-js library. That said, I think it could have somewhat better documentation. |
||
| def on_complete(env) | ||
| return unless env[:status] == 200 && env.body | ||
| body = env.body.is_a?(String) ? JSON.parse(env.body) : env.body | ||
| return unless body.fetch('error', false) | ||
| handle_api_errors(body) | ||
| end | ||
|
|
||
| def handle_api_errors(response_body) | ||
| # response_body is in this format: | ||
| # {"error"=>{"code"=>400, "message"=>"", "details"=>[""]}} | ||
| error_code = response_body.dig('error', 'code') | ||
| error_message = response_body.dig('error', 'message') || "Received error code #{error_code}" | ||
| # log an error | ||
| raise ArcgisApi::InvalidResponseError.new( | ||
| Faraday::Error.new(error_message), | ||
| { | ||
| status: error_code, | ||
| body: { details: response_body.dig('error', 'details')&.join(', ') }, | ||
| }, | ||
| ) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be more accurate to call this ArcgisRefreshTokenJob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svalexander, will in effect it's refresh the token, but it does not check whether the token need to be refreshed and replace with a new one anyway.