Add middleware/handler to throttle requests to conform to API rate limit

[holtkamp]

The API employs a rate limit, which is understandable.

next to the (outdated?) documentation the following seems to apply:

• endpoint: device-server/*
  • HTTP GET: 9 requests per 9 consecutive seconds
• endpoint: user/*
  • HTTP POST ??: 5 requests per 5 consecutive seconds

A way to deal with this is:

• use the API,
• detect a 429 exception
• sleep a bit and try again

This is a rather "reactive" approach and unintelligent (the minimum number of seconds to sleep seems not available in the exception?). It would be nice to have a middleware in the API client that keeps track of the number of requests submitted to each endpoint and applies throttling when required.

Since the client performs some requests when connecting. Such a throttling mechanism can not solely be implemented in userland code. A throttling approach also prevents sleeping during a test 😨 .

[OGKevin]

@holtkamp thanks for this suggestion. Ill tackle this in 0.13.0.

[holtkamp]

@OGKevin ok, nice, in case you need inspiration, implemented something which currently "does the job"...

[OGKevin]

@holtkamp What do you mean with implemented something which currently "does the job". The way the ApiClient Is build it would not be possible to remake a call after a 429 has been caught. The only way to achieve this is to add a middleware before the request is sent and keep the count of requests per seconds set in BunqContext.

Then calculate if this request may trigger a 429 or not. And based on that sleep or not. But of course im open to other suggestions.

[holtkamp]

The only way to achieve this is to add a middleware before the request is sent and keep the count of requests per seconds set in BunqContext.

Indeed, that approach is what I implemented. Will check coming week if I can share some details

[holtkamp]

Currently using this approach: https://gist.github.com/holtkamp/bae00c495f80eace6aa49d12b46f5bca, maybe not very elegant, but does the job

[holtkamp]

Apparently what we are looking for is a "LeakyBucket" implementation. Look at this PSR-7 implementation of it:
https://github.com/robwittman/leaky-bucket-rate-limiter

[holtkamp]

@OGKevin any update / progress on this aspect?

[OGKevin]

No not really 😭. Im packed with internal projects and therefore didn't get time to work on github projects. PR's are highly appreciated tho. Next week ill be on holiday so i might do some hobby coding and work on github then.

Some one made a PR for c# bunq/sdk_csharp#106 (review) which looks good.

[holtkamp]

Happy anniversary for this issue! 🎂

While Bunq is pushing a lot of "crazy awesome nice amazing" new features in well marketed "product updates", it seems some basic functionality like request throttling of the official client of a throttled API is not a priority.

@OGKevin are sufficient resources assigned to API (client) development?

[OGKevin]

While Bunq is pushing a lot of "crazy awesome nice amazing" new features in well marketed "product updates", it seems some basic functionality like request throttling of the official client of a throttled API is not a priority.

thats exactly the case :P and has always been. Sorry i could not have had a proper look at this.

I no longer work at bunq, hence i've been removed as maintainer. @kojoru took over, but it looks like he is running in the same issue as i was 😉. Im happy to dive into this and do open source contribution tho. ill try to have a look this weekend before i go on holiday. If not afterwards.

I haven't used this SDK anymore, i've switched to golang.

[holtkamp]

@OGKevin thanks for the response

@kojoru maybe you can convince decision makers to assign some of the "marketing budget" (for example, this: https://twitter.com/bunq/status/1090923033223745537) to sponsoring OSS contributions. There are plenty skilled developers who can help improve the API client once they get some support to be able to spend time on this.

[OGKevin]

So i took a look at this, and it seems that the current way the request and response handlers are created: https://github.com/bunq/sdk_php/tree/e9511e1c158a2c8d768d167bc05a66d7d88ea5d5/src/Http/Handler

The response handlers do not have the request 🤔 So, the first task would be is to rework the response handlers to so that they get the request as well. Once this is done, then we can create a new response handler that checks for 429, and if that is the case then returns a recursive call to the client again to execute that request.

I'll work on a pr to give the response handlers the request.

[OGKevin]

I've created a PR. the only thing that we need to figure out now is how we're going to test an actual request. I hate the fact that the SDK is making real requests which makes the test suite super slow :( never got the chance to implement a nice request mocker.

Nevertheless, how can we easily trigger a 429 that goes through the api client 🤔

sdk_php/tests/Http/ResponseHandlerRateLimitTest.php

Lines 60 to 66 in a1c1ba1

	/**
	*/
	public function testActualRequest()
	{
	static::markTestSkipped('how do we want to test this');
	}
	}

[OGKevin]

just to clarify my retry approach, as this SDK is build based on static methods and singletons. It makes no sense to keep a global mapping to throttle the requests instead of retrying on failures. If we had a long living api client, then this client could have kept a map of uri's and throttle the requests accordingly.

[holtkamp]

If we had a long living api client, then this client could have kept a map of uri's and throttle the requests accordingly.

Well, in my case the client gets initialized on a daily basis to perform a batch of operations. So this client is "long living".

I think the "retry approach" to deal with 429 errors is fundamental to have, but can be extended with the "request throttling" approach to (try to) prevent even receiving those 429 errors...

[holtkamp]

the only thing that we need to figure out now is how we're going to test an actual request. I hate the fact that the SDK is making real requests which makes the test suite super slow :( never got the chance to implement a nice request mocker.

Maybe we can have a look at https://github.com/picqer/moneybird-php-client/tree/master/tests for inspiration. I contributed to that library a few times and if I remember correct the testing of the connection with a mocked connection was quite decent...

Also see:

• Add FinancialMutation::linkToBooking() picqer/moneybird-php-client#85 (comment)
• https://phpunit.de/manual/6.5/en/test-doubles.html
• https://github.com/bernardphp/bernard/blob/master/tests/Driver/Sqs/DriverTest.php

[OGKevin]

Well, in my case the client gets initialized on a daily basis to perform a batch of operations. So this client is "long living".

Thats not what i meant, what i meant was that for each Api call, a new client is being made. E.g.

sdk_php/src/Model/Generated/Endpoint/MonetaryAccountBank.php

Line 409 in e9511e1

$apiClient = new ApiClient(static::getApiContext());

and

sdk_php/src/Model/Generated/Endpoint/Payment.php

Line 355 in e9511e1

$apiClient = new ApiClient(static::getApiContext());

This makes the client super short lived and therefore it can not nicely keep track of all the previous requests it made and therefore nicely throttle if it suspects a 429 to happen.

Maybe we can have a look at https://github.com/picqer/moneybird-php-client/tree/master/tests for inspiration.

I see that they indeed use mocks and not make actual requests which is nice. But, in our case, mocking is kind of hard. As bunq's approach is to make everything static. And the fact that you have this

sdk_php/src/Model/Generated/Endpoint/MonetaryAccountBank.php

Lines 443 to 457 in e9511e1

	public static function get(int $monetaryAccountBankId, array $customHeaders = []): BunqResponseMonetaryAccountBank
	{
	$apiClient = new ApiClient(static::getApiContext());
	$responseRaw = $apiClient->get(
	vsprintf(
	self::ENDPOINT_URL_READ,
	[static::determineUserId(), $monetaryAccountBankId]
	),
	[],
	$customHeaders
	);
	return BunqResponseMonetaryAccountBank::castFromBunqResponse(
	static::fromJson($responseRaw, self::OBJECT_TYPE_GET)
	);

Instead of the class having a client as field, makes it even harder to have a test double client that fakes the requests. What i normally try to do is use interfaces and make 2 implementations. One for test and one for prod, as you could see here

sdk_php/src/Http/RequestRetryer.php

Lines 12 to 18 in 4c13a41

	interface RequestRetryer
	{
	/**
	* @param RequestInterface $request
	* @return ResponseInterface
	*/
	public function retryRequest(RequestInterface $request): ResponseInterface;

However, IMO there should not be an interface for this, however there should be an interface that defines the

sdk_php/src/Http/ApiClient.php

Lines 212 to 218 in e9511e1

	private function request(
	string $method,
	string $uri,
	$body,
	array $params,
	array $customHeaders
	): BunqResponseRaw {

method. This way, we can have 1 "requester" for production that makes actual requests, and 1 "requester" for test that fakes requests. This makes it so that you don't even need to use a mock, just an implementation for testing.

[OGKevin]

I guess we can drop all hopes of getting my PR looked at and merged 🤷‍♂ Im really sorry @holtkamp that I could not have done this while I still was a maintainer😔.

[holtkamp]

Additional inspiration for the Bunq team:

https://github.com/spatie/guzzle-rate-limiter-middleware

[bosd]

Seems like another anniversary 🎂 for this issue.
Anyone found an alternative solution or user implementation?