Skip to content

LG-8508: ArcGIS API: Use full URL paths instead of shared domain for greater flexibility#7777

Merged
allthesignals merged 4 commits intomainfrom
wmg/8508-make-paths-configurable
Feb 6, 2023
Merged

LG-8508: ArcGIS API: Use full URL paths instead of shared domain for greater flexibility#7777
allthesignals merged 4 commits intomainfrom
wmg/8508-make-paths-configurable

Conversation

@allthesignals
Copy link
Contributor

🎫 Ticket

🛠 Summary of changes

This change uses fully qualified URL paths for each endpoint.

Before, we were using a single root URL and sharing it across paths. This works for GSA-hosted ArcGIS, but it doesn't work for ESRI-hosted (cloud) ArcGIS Online because the host domains are different for generateToken and the geocoding services (see https://developers.arcgis.com/rest/users-groups-and-items/root.htm and https://developers.arcgis.com/rest/geocode/api-reference/geocoding-suggest.htm — one uses a subdomain arcgis, the other does not).

After, this change would depend on fully-qualified URLs so that we can swap in an alternative ArcGIS service if needed.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Local spot checking
  • Dev environment check

@allthesignals allthesignals force-pushed the wmg/8508-make-paths-configurable branch from 005eb26 to b6a30a0 Compare February 6, 2023 19:09
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@allthesignals allthesignals force-pushed the wmg/8508-make-paths-configurable branch from b6a30a0 to 84f5a58 Compare February 6, 2023 19:13
Comment on lines 38 to 40
Copy link
Contributor

Choose a reason for hiding this comment

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

my comment might have gotten lost, but I think we can remove these constants and just inline usage of the configs (since we removed Figaro a long time ago, reading from the config is a very cheap struct property read)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right! Okay make sense.

I'll need to update how the mock works too, but that should be easy enough. Glad to get rid of some code...

@allthesignals allthesignals force-pushed the wmg/8508-make-paths-configurable branch from 84f5a58 to b7a1963 Compare February 6, 2023 19:24
@allthesignals allthesignals force-pushed the wmg/8508-make-paths-configurable branch from 3780e81 to d314dfd Compare February 6, 2023 19:59
@allthesignals
Copy link
Contributor Author

Just want to note an important step:

When you switch over to another service, it'll still keep the old token from the previous service (naturally). In case of a failure, someone'll need to bust the cache manually.

@zachmargolis
Copy link
Contributor

When you switch over to another service, it'll still keep the old token from the previous service (naturally). In case of a failure, someone'll need to bust the cache manually.

Could we include the domain as part of the cache key?

@allthesignals
Copy link
Contributor Author

allthesignals commented Feb 6, 2023

When you switch over to another service, it'll still keep the old token from the previous service (naturally). In case of a failure, someone'll need to bust the cache manually.

Could we include the domain as part of the cache key?

Oh, interesting! I was thinking about just implementing the token retry on 401 approach we talked about a while back, but this could be quicker.

So, this would be a dynamic cache keyname, right? Something like

FULL_API_CACHE_TOKEN_KEY = "#{URI(SUGGEST_TOKEN_URL).host}}_API_TOKEN_CACHE_KEY"

# ...

Rails.cache.write(FULL_API_CACHE_TOKEN_KEY, token, expires_at: expires_at)

?

@zachmargolis
Copy link
Contributor

So, this would be a dynamic cache keyname, right? Something like

Yup exactly that!

@allthesignals
Copy link
Contributor Author

allthesignals commented Feb 6, 2023

@zachmargolis love it!

@NavaTim would there be any issues in redis with having a key name that includes periods? so, something like: arcgis_api_token_apis:google.com?

Looking here, I saw some other conventions that could work better. I settled on:

arcgis_api_token:geocoder.arcgis.com

🤷

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@allthesignals allthesignals merged commit 1e6b815 into main Feb 6, 2023
@allthesignals allthesignals deleted the wmg/8508-make-paths-configurable branch February 6, 2023 22:17
@aduth aduth mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants