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

Add idempotency support #639

Merged
merged 6 commits into from
Sep 13, 2022

Conversation

sandervanhooft
Copy link
Collaborator

@sandervanhooft sandervanhooft commented Sep 12, 2022

This PR adds idempotency support to the MollieApiClient.

Using a (Default)IdempotencyKeyGenerator, an idempotency key is automatically added to the request headers on each mutating action (POST, PATCH, DELETE).

On retries, the same key is reused.

New methods:

  • $mollieApiClient->setIdempotencyKey(string $key): MollieApiClient
  • $mollieApiClient->getIdempotencyKey(): string
  • $mollieApiClient->resetIdempotencyKey(): MollieApiClient
  • $mollieApiClient->setIdempotencyKeyGenerator(IdempotencyKeyGeneratorContract $generator): MollieApiClient
  • $mollieApiClient->clearIdempotencyKeyGenerator(IdempotencyKeyGeneratorContract $generator): MollieApiClient

Note that it's now possible to inject the IdempotencyKeyGenerator when instantiating the MollieApiClient.

@sandervanhooft sandervanhooft changed the base branch from master to v2-idempotency September 13, 2022 09:43
@sandervanhooft sandervanhooft merged commit 35c872e into mollie:v2-idempotency Sep 13, 2022
@sandervanhooft sandervanhooft deleted the idempotency branch September 13, 2022 09:45
@mollierobbert
Copy link
Contributor

mollierobbert commented Sep 14, 2022

Thanks Sander! I like the implementation, however we do not currently want to offer idempotency out-of-the-box. It's a feature that should remain optional for now for two reasons.

Firstly, it's a beta feature that we have yet to test at full scale (and adding library support is a critical part of even being able to test it at scale). Secondly, the feature will change the behavior of the API for the user in subtle ways. This may not be what the user expects.

As such, I suggest we launch it as optional first, and then review after a while.

@sandervanhooft
Copy link
Collaborator Author

Hi @mollierobbert ,

Thanks for the ❤️ . This is exactly why it was prepared for a separate v2-idempotency branch, so it can easily be piloted in specific integrations, while being kept up to date with the master branch.

@Pimm
Copy link
Contributor

Pimm commented Nov 28, 2022

Hi @sandervanhooft,

We've implemented this same functionality in the Node.js client, and had a question. In this PR, idempotency keys are added to POST, DELETE, and PATCH requests. Are there any PATCH endpoints in the Mollie API which are not idempotent by nature? We can't think of any, but you may know something we don't.

@sandervanhooft
Copy link
Collaborator Author

Hi @Pimm !

Good question. As far as I know all PATCH endpoints are idempotent, but best to double check with @mollierobbert .

What do you think Robbert?

@mollierobbert
Copy link
Contributor

I silently launched the documentation with a 'beta' flag, since the closed beta was a bit hard to test without coverage by the PHP/Node/etc. packages anyway.

https://docs.mollie.com/overview/api-idempotency

As stated in the docs, we will ignore keys for DELETE PATCH GET, since all of those are considered idempotent by definition.

DELETE is irreversible — doing it twice will not delete something twice.

PATCH will update the specified fields, but if you run the same PATCH again the fields will already be updated, so no changes will be made.

Hope this helps!

One more thing Sander — in Pimm's package we now decided to allow configuring a key per request. Do you think we can do something similar for PHP? It makes a bit more sense to me than configuring it on client-level, since each request should have its own key.

(Perhaps I'm misinterpreting the interface though — correct me if I'm wrong.)

@Pimm
Copy link
Contributor

Pimm commented Dec 6, 2022

This is getting a little off-topic, as it concerns not just the PHP client.

DELETE is irreversible — doing it twice will not delete something twice.

In the Node.js client, we're adding idempotency keys to DELETE requests as well. Not because we want to prevent double-deleting (super deleting?) something, but because we want to report the correct outcome.

Deleting an existing customer causes Mollie core to respond 204; deleting an already deleted customer causes it to respond 410. Consequently, if a client (PHP, Node.js, or otherwise) re-attempts a deletion, it may receive the 410 on the second attempt and report that, which is not ideal. With the idempotency key, Mollie core could replay the initial 204, causing that to be reported instead.

In other words: DELETE requests are by nature safe to retry, but the outcome could be misleading.

Do you have any thoughts on this @mollierobbert?

@sandervanhooft
Copy link
Collaborator Author

Agreeing with @Pimm here on the DELETE idempotency keys.

@sandervanhooft
Copy link
Collaborator Author

One more thing Sander — in Pimm's package we now decided to allow configuring a key per request. Do you think we can do something similar for PHP? It makes a bit more sense to me than configuring it on client-level, since each request should have its own key.

(Perhaps I'm misinterpreting the interface though — correct me if I'm wrong.)

It's already here!

/**
* Set the idempotency key used on the next request. The idempotency key is a unique string ensuring a request to a
* mutating Mollie endpoint is processed only once. The idempotency key resets to null after each request. Using
* the setIdempotencyKey method supersedes the IdempotencyKeyGenerator.
*
* @param $key
* @return $this
*/
public function setIdempotencyKey($key)
{
$this->idempotencyKey = $key;
return $this;
}

@mollierobbert
Copy link
Contributor

I reasoned that since DELETE endpoints will already respond with either a No Content or Gone, people can already easily implement idempotency for all DELETE calls without this Idempotency-Key feature. Hence I did not consider these calls in scope for the feature.

I don't have a strong preference to include or exclude it though. But, I'm a bit hesitant to further increase scope for now, since we're not entirely sure how big of a storage footprint the Idempotency-Key feature will generate for us. So let's reconsider it a few weeks/months from now as we evaluate the first results of the feature.

@sandervanhooft
Copy link
Collaborator Author

I reasoned that since DELETE endpoints will already respond with either a No Content or Gone, people can already easily implement idempotency for all DELETE calls without this Idempotency-Key feature. Hence I did not consider these calls in scope for the feature.

For clarity:

  • No Content is a 2** success status
  • Gone is a 410 error status, the SDK should throw an exception. I cannot assume that the merchant wants the SDK to catch these as a convenience.

@mollierobbert
Copy link
Contributor

Fair enough! We just added support to DELETE endpoints as well. Let me know if this works as expected for you guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants