Xero-Java icon indicating copy to clipboard operation
Xero-Java copied to clipboard

Create a configurable API retry strategy

Open U3R1YXJ0 opened this issue 4 years ago • 11 comments

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

Is your feature request related to a problem? Please describe. We regularly get unspecified "com.xero.api.XeroServerErrorException: Internal Server Errors" on API calls. In the last 2 weeks we've had 13 instances. They are not related to a specific call - when we manually retry we normally succeed second time.

This probably wouldn't matter for many atomic use cases, but we have functionality that runs a series of calls over the space of 30 minutes. If we get an internal server error we stop, because there is an unknown problem. We're are now looking at creating a retry solution within our client.

Describe the solution you'd like The Xero client could include a default and configurable retry strategy for failed API calls.

Describe alternatives you've considered We're planning on building our own solution.

Additional context Ideally the Xero API would be more specific about any issues. At the moment we could retry on all internal server errors, and our experience suggest this is sensible. However if there was some specific information at Xero that could indicate if the client should retry again or not, that would be ideal.

U3R1YXJ0 avatar Feb 18 '21 09:02 U3R1YXJ0

@U3R1YXJ0, this is already supported by the SDK. All you need to do is override the default http client in the Xero API class with one that is configured with retries. For example you can configure an Apache http client with a retry policy, and set the Apache http client onto the respective Xero API class.

Once I get a free moment I will send you a example to get you going.

lancedfr avatar Feb 19 '21 14:02 lancedfr

Hi Lance, awesome, that sounds great! Thanks, a example will be really useful.

U3R1YXJ0 avatar Feb 19 '21 14:02 U3R1YXJ0

Hi Stuart, please see an example below, refactor to your requirement. We configure the AccountingApi instance as a managed bean so its not created every request.

  void someOperation() {
      ApiClient apiClient = new ApiClient();
      apiClient.setHttpTransport(new ApacheHttpTransport(newRetryHttpClient()));
      //ConnectionTimeout defaults to 20 seconds if not set
      apiClient.setConnectionTimeout(CONNECTION_TIMEOUT);
      //ReadTimeout defaults to 20 seconds if not set
      apiClient.setReadTimeout(READ_TIMEOUT);
      AccountingApi accountingApi = AccountingApi.getInstance(apiClient);
      //or AssetApi.getInstance(apiClient), etc
  }

  DefaultHttpClient newRetryHttpClient() {
    DefaultHttpClient defaultHttpClient = new DefaultHttpClient();
    defaultHttpClient.setHttpRequestRetryHandler(new DefaultHttpRequestRetryHandler(RETRY_COUNT, false));
    return defaultHttpClient;
  }

One last thing to note, the Xero SDK uses google-http-client to facilitate http connections. The google-http-client can be configured to use Apache http client, pure Java http client (or others like okhttp, not sure), the point is you can configure whichever underlaying http client you want with its retry policy, connection managers, SSL config, http schema config, etc. You not forced to use Apache http client like my above example does.

lancedfr avatar Feb 19 '21 16:02 lancedfr

Ok, got it, that's great. Yes, I remember configuring the google client now (it was quite a while ago). Thanks for the example, we will give that a go.

U3R1YXJ0 avatar Feb 19 '21 16:02 U3R1YXJ0

@lancedfr - thanks for helping on this issue.

I'll leave this open so I can add this info to the README.

SidneyAllen avatar Feb 19 '21 23:02 SidneyAllen

@lancedfr Just to mention that we've found DefaultHttpClient is deprecated in favour of HttpClient. The Xero SDK currently uses google-api-client 1.23.0. In this version, the ApacheHttpTransport client is not compatible with HttpClient, but we think it is in later versions of google-api-client.

We're going to stick with the deprecated DefaultHttpClient for now.

Perhaps you could update google-api-client to a more recent version? 1.23.0 is from October 2017. Looks like this would make integration with ApacheHttpTransport easier whilst using the recomended HttpClient.

EDIT: Just realised you don't work for Xero @lancedfr! So the suggestion is for @SidneyAllen

U3R1YXJ0 avatar Feb 23 '21 12:02 U3R1YXJ0

We declared a more recent version of google-api-client as a dependency in our application POM to override the default version specified in the Xero SDK, and that seems to work fine.

U3R1YXJ0 avatar Feb 23 '21 12:02 U3R1YXJ0

Correct, I do not work for Xero. I only contribute to the SDK and did some work on the "first oauth 1" SDK 😁

I wanted to create a request to update the google-api-client to a newer version as a result of the deprecated dependency on Apache http code. I speak under correction I think the latest version of google-api-client still depends on the deprecated Apache code??? If so, we'll need to update to google-http-client-apache-v2, which I think is a breaking change for those who already use Apache Http client.

EDIT: just realised you said you declared a more recent version of google-api-client, meaning its probably not dependant on deprecated Apache http code. Maybe we can create a separate request to update to the latest version of google-api-client 🙂

lancedfr avatar Feb 23 '21 15:02 lancedfr

Hey @lancedfr and @U3R1YXJ0

I recently did a spike to see what the impact of updating the google-api-client from 1.23.0 to 1.31.2

I found the methods for interacting with Xero API endpoints all functioned properly, but the methods used during our authentication flow required modification.

For example in the Callback servlet under 1.23.0 we use the following code.

final JsonFactory JSON_FACTORY = new JacksonFactory();

DataStoreFactory DATA_STORE_FACTORY = new MemoryDataStoreFactory();

AuthorizationCodeFlow flow = new AuthorizationCodeFlow.Builder(BearerToken.authorizationHeaderAccessMethod(),
                    HTTP_TRANSPORT, JSON_FACTORY, new GenericUrl(TOKEN_SERVER_URL),
                    new ClientParametersAuthentication(clientId, clientSecret), clientId, AUTHORIZATION_SERVER_URL)
                    .setScopes(scopeList).setDataStoreFactory(DATA_STORE_FACTORY).build();

TokenResponse tokenResponse = flow.newTokenRequest(authResponseCode).setRedirectUri(redirectURI).execute();

Under version 1.31.2 JacksonFactory(); has been deprecated and the callback code uses GsonFactory

TokenResponse tokenResponse = new AuthorizationCodeTokenRequest(new NetHttpTransport(), new GsonFactory(),
        			 new GenericUrl(TOKEN_SERVER_URL), clientId)
        			 	.setRedirectUri(redirectURI)
        			 	.setCode(authResponseCode)
        			 	.setScopes(scopeList)
        			 	.setGrantType("authorization_code")
        			 	.setClientAuthentication(new ClientParametersAuthentication(clientId, clientSecret))
        			 	.execute();

The code is 1.31.2 is cleaner and will be better as we move into the future but from what I can tell this is a breaking change and will require a major version update from 4.x to 5.x. I was holding off as I wanted to get the Files API support added in 4.7 of the SDK. The Files API work is complete now.

Any objections or support of this move to 1.31.2 for google-api-client?

SidneyAllen avatar Feb 23 '21 18:02 SidneyAllen

Hi @SidneyAllen, I support the move. I do not think anyone is in need of this upgrade, with that said I'd say it's low priority and it can for Files API work (or even longer for that matter).

PS. I believe #261 can be resolved as retry is supported in the SDK. Let me know if you want me to resolve this one and open a new feature request for the google-api-client upgrade?

lancedfr avatar Feb 28 '21 20:02 lancedfr

@SidneyAllen We also support the move. I also agree with @lancedfr that there is no hurry as individual clients can upgrade themselves by specifying a more recent version in their application POMs.

U3R1YXJ0 avatar Mar 01 '21 08:03 U3R1YXJ0