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

[feature] idempotency header middleware #3186

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NyaaaWhatsUpDoc
Copy link
Member

Description

Adds a piece of middleware supporting the idempotency key. Though probably worth you looking through this PR first before we do anything with it @VyrCossont as you have more experience working with this client-side. I specifically wanted to avoid caching anything more than the status code for each response, as it would either require buffering and caching of response bodies which could end up quite memory intensive, or it will require a unique solution for every single endpoint which i'm also really not a fan of. Is responding with the previous status code + a "this already happened" http response body good enough for clients to work with? 🤔

closes #2723

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the feature/idempotency-header-middleware branch from d75fb3e to f3e68e9 Compare August 9, 2024 12:16
@VyrCossont
Copy link
Contributor

So unfortunately (and this is on me, I didn't fully investigate in the feature request) we do need to store the original response data or have some way of fetching it on demand. If we send the previous response code and the "already happened" JSON message instead of response data in the expected format (or an error code and response), no existing client is going to understand what to do with that data. If we send data in the expected format, there's no such issue.

The Mastodon implementation is status-specific and makes a DB query for whatever status or scheduled status was created with that idempotency key. We could do the same (requiring specific support for each idempotency-key-enabled endpoint to cache the relevant model type and ID for each idempotency key for future lookup), or we could cache the entire response data as well as the response code.

I think the scenario where a client makes a request with an idempotency key and it has already made that request before will be rare, compared to the case where a client makes a request with an idempotency key and it succeeds and is not repeated, so it would make more sense to not cache the full response data, just enough info that we can look it up later if needed.

@NyaaaWhatsUpDoc
Copy link
Member Author

hmmm i shall have a think about how best to do this 🤔

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.

[feature] Implement Idempotency-Key for POSTing statuses
2 participants