Distinguish rate limit and network errors from invalid USDA keys#960
Open
munzzyy wants to merge 1 commit into
Open
Distinguish rate limit and network errors from invalid USDA keys#960munzzyy wants to merge 1 commit into
munzzyy wants to merge 1 commit into
Conversation
testApiKey() only ever returned true/false, so a rate-limited or network-failed request got treated the same as an actually bad key and the user saw "API Key Invalid" either way. Confirmed against the live USDA API that a valid key past its rate limit comes back as HTTP 429 / OVER_RATE_LIMIT, not the 403 / API_KEY_INVALID a bad key gets, and the old code couldn't tell the two apart. The non-ok branch also fell through to invalid-key for anything that wasn't a rate limit, which meant a 5xx USDA outage or a non-JSON response got the same "API Key Invalid" message as a genuinely bad key. The reason is now picked from the response status alongside the error code (403/API_KEY_INVALID for a bad key, 429/OVER_RATE_LIMIT for the rate limit, anything else falls back to network), so an outage or unexpected response reads as "couldn't verify" instead of telling the user their key is wrong. testApiKey now resolves an object with a reason (invalid-key, rate-limit, network) instead of a bare boolean, and saveUSDAKey shows a message that matches the actual failure. Fixes davidhealey#947.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #947.
testApiKey()only ever resolved true or false, so any non-2xx response got collapsed into the same "invalid" result. In practice that meant a rate-limited key and an actually-bad key showed the identical "API Key Invalid" toast. I hit the live USDA API to check what a rate limit actually looks like: a validDEMO_KEYpast its limit comes back as HTTP 429 witherror.code: "OVER_RATE_LIMIT", completely different from the 403 /API_KEY_INVALIDa bad key gets. The old code had no way to tell those apart.There was a second problem in the same branch: anything that wasn't a rate limit fell straight through to "invalid-key". That includes a USDA outage (5xx), a malformed/non-JSON response, or any other api.data.gov error code. So someone with a perfectly good key would get told their key is wrong during an outage, which is really the same bug as the rate-limit case, just a different trigger.
The fix picks the reason from both the response status and the error code: 403 or
API_KEY_INVALIDmeans the key itself is bad, 429 orOVER_RATE_LIMITmeans rate-limited, and anything else (including unknown or server errors) falls back to a "couldn't verify, check your connection" message instead of "API Key Invalid".testApiKeynow resolves{ valid, reason }instead of a bare boolean, andsaveUSDAKeyshows the message that matches the actual reason.Verified locally with a standalone Node script hitting the real USDA endpoint (not through Cordova, just plain fetch against the same URL testApiKey builds): a garbage key comes back invalid-key, hammering DEMO_KEY until it 429s comes back rate-limit, and pointing the request at a host that doesn't resolve comes back network. All three matched what the mapping is supposed to produce.
I ran this against Cordova's browser platform (
cordova platform add browser,cordova run browser) rather than a real Android device, so I can't confirm behavior on-device, just that the logic and messages are correct.