Skip to content
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

SDK exception codes do not match API response codes #267

Open
U3R1YXJ0 opened this issue Mar 8, 2021 · 4 comments
Open

SDK exception codes do not match API response codes #267

U3R1YXJ0 opened this issue Mar 8, 2021 · 4 comments
Labels
Status: In Discussion Tag individuals in the comments you wish to discuss issue with Type: Feature Request

Comments

@U3R1YXJ0
Copy link

U3R1YXJ0 commented Mar 8, 2021

SDK you're using (please complete the following information):

  • Version 4.1.4

Describe the bug
For background purposes, the specific thing I am working on is creating a retry solution for "Internal Server Error" messages. We see a lot of these in our logs.

We created a solution in which errors with a code == 500 are retried. I assumed this was the correct code to catch because the API response codes page says Internal Errors are a 500. However, this do not seem to work. On closer inspection, it appears "Internal Server Error" messages are actually error with code > 500.

The issue raised is that error codes greater than 500 from the API are not mapped well into Java exceptions.

I have 4 sources of information on error codes:
XeroApiExceptionHandler.java
README.md
https://developer.xero.com/documentation/oauth2/limits
https://developer.xero.com/documentation/api/http-response-codes

  1. The response codes page says rate limit exception is a 503, but OAuth2 limits, Java SDK code and readame say its a 429. This is likely an issues for the API team on the response codes page?

  2. The Java SDK maps all errors > 500 to the same message and the exception codes are not logged by default. 501 (not implemented), 503 (rate limit?), 503 (not available) and 503 (organsation offline) all get mapped to a static message "Internal Server Error". We are therefore losing valuable error information. This also explains why we're regularly seeing Internal Server Error from the API - they are most likely actually 503 not available messages from the Xero API, which are suitable to retry (unlike internal server errors).

  3. Mapping error codes > 500 to a static message "Internal Server Error" is particularly confusing because the the Xero API "Internal Error" is actually a 500 code. The 500 code then maps to a different message (no mention of internal server error) asking the client to check the Xero APi status page.

To Reproduce
N/A

Expected behavior
The Java SDK exception messages for codes > 500 should match the API exception messages. The Java SDK should not merge multiple API messages into one Java exception message. Perhaps the API code should be included in the Java exception message, as its not included by default.

Screenshots
NA

Additional context
NA

@U3R1YXJ0
Copy link
Author

U3R1YXJ0 commented Mar 8, 2021

I can see a 503 from the accounting API is a rate limit exception, and a 429 from the oauth2 api is a rate limit exception. I'm now wondering whether the Java SDK differentiates these? Could I actually be getting 503 rate limits exception on the accounting API that are being converted into "Internal Server Error" in the SDK?

Is the accounting API rate limit still a valid response if I am using OAuth2? Or will I only get OAuth2 rate limits errors?

@SidneyAllen
Copy link
Contributor

Hi @U3R1YXJ0

In your last comment you use the term "the accounting API" and the "ouath2 api". I believe you are confusing the API set and the authentication methods. OAuth 1.0a (scheduled for deprecation) and OAuth 2.0.

429 is the correct status code for OAuth 2.0 rate limit exceptions with header information about which rate limit was hit.
503 was the status code for OAuth 1.0a for rate limits - but is not true for OAuth 2.0

You call out two separate issues.

  1. The SDK should handle errors > 500 with more granularity. Def. something we can do - this is a feature request and will need to be prioritized against other work. Unless the amazing @lancedfr can lend a hand to get it done quicker 😄
  2. The API shouldn't return multiple error types with the same status code. - Is this accurate? If so, this is feedback for the API engineering team and can't be solved in the SDK.

@SidneyAllen SidneyAllen added Status: In Discussion Tag individuals in the comments you wish to discuss issue with Type: Feature Request labels Mar 16, 2021
@SidneyAllen SidneyAllen self-assigned this Mar 16, 2021
@U3R1YXJ0
Copy link
Author

I raised the 429/503 issue with the API team and they have updated https://developer.xero.com/documentation/api/http-response-codes. The page now has both codes and specifies that one is relevant to OAuth2 and the other OAuth1.

The updates I suggest to the SDK (XeroApiExceptionHandler.java) are:

  1. Handle/Map 501 and 503 API codes
  2. Remove mapping of >500 codes to "Internal Server Error" message (this removes useful info from API).

@evelinasmit
Copy link
Contributor

I've submitted a pull request @SidneyAllen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Discussion Tag individuals in the comments you wish to discuss issue with Type: Feature Request
Projects
None yet
Development

No branches or pull requests

3 participants