Feed back API rate limits requests to the application. Demonstrate how to handle rate limit exceptions with Guzzle Middleware

[mogilvie]

PR for issue #806

Pass the API Request Rate Limits back up to the Application to allow the user of the bundle to decide if they throttle api requests.

Have also added an option to throttle requests from within the bundle itself.

Set a configurable variable in the Application, which sets the maximum wait time that the application should delay if it hits an API limit. (Can be set false if no intention to retry requests)

Catch Xero 429 Response issues where the API limit has been reached.

Get the 'Retry-After' time from the Xero response header.

If the Retry-After period is less than the permitted maximum duration in the config, then sleep for the retry period, and retry the request.

[calcinai]

@mogilvie just reading through this–I'm wondering if this is the correct place to be handling the throttling. I can see that you've got to be intentional with that config parameter, but potentially just sleeping in execution of the request could lead to some potentially undesirable timeouts, especially as the rest of the application will have no idea that it's caught/sleeping for that reason.

Is there a way we could move that handling out of the middle of the request while keeping it simple? For instance, you could inject a middleware into the Guzzle constructor and leverage the "retry" middleware? See https://github.com/guzzle/guzzle/blob/7.7/src/RetryMiddleware.php

[mogilvie]

Thanks @calcinai, wasn't aware of the middleware option. I'll do that.

[mogilvie]

So, looking further, we can achieve the retry using the current Application::setTransport, where the parent bundle can construct a Client with the the RetryMiddleware in the HanderStack.

Shall I just document this, instead of modifying the Application? Or should I add a method for that replaces the Client Transport with a RetryMiddleware constructed one?

[mogilvie]

Have updated PR to allow user to skip the default guzzle client, and provide their own Client as a transport.
Added example and explanation in ReadMe.

[calcinai]

@mogilvie Yeah that's what I was getting at! Sorry I didn't quite get here until you'd already written the code..

[mogilvie]

I think I'm done. Review at your leisure.

[calcinai]

@mogilvie I think this way is much cleaner, thanks for your work. The only thing I think would improve would be to either extend or duplicate the guzzle middleware to catch the errors and increment those counters. It'd also remove the need to do all the plumbing in user/app space.

Functionally I think it's great as is, so this would really just shuffling some code around. Thoughts?

[mogilvie]

There are three feature aspects to be considered here.

1. Introducing a method of throttling when the user hits rate limits. The middleware works well for this.
2. Providing feedback to the application on the current status of the call limits. That's an information type function that the end application package can use to make decisions on. They might trend this information, or provide GUI feedback, modify their query pagination, slow down calls. But it's not a retry function, this shouldn't be in the RetryMiddleware.
3. Handling of exceptions, I would think that the Application continues to handle the exceptions. The middleware should only be handling the 429 response code. Anything else fails up to the exception catching in the Application.

The call count status is available in the normal response headers from the Xero API. Item 2 is just a way of exposing those to the application. Should we be looking at a different method of doing that? Any preference?

[calcinai]

Cool, well let's stick with how it is. The thing that got me thinking about it was really just the getAppRateLimits() method where properties are transposed into an associative array, which is a nightmare for any sort of static analysis. I didn't want to suggest doing something differently there if another approach would remove that anywhere.

[mogilvie]

I could change that getAppRateLimits() array response to individual getters?

[calcinai]

@mogilvie If you don't mind, I think that would be great. Really appreciate the back and forward on this.

[mogilvie]

swapped out getAppRateLimits for individual getters.