Uh oh!
There was an error while loading. Please reload this page.
Send the idempotency key as an Idempotency-Key header instead of a query parameter - #132
Open
eastagiletracker wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes sending the idempotency key as an
Idempotency-Keyheader instead of a query parameter on every resourcecreate()that accepts one. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/336. You can sign in with your GitHub ID to claim ownership of the project.What's happening
Resource._createis declared as_create(data, params?, requestConfig = {}), and it already builds theIdempotency-Keyheader out ofrequestConfig.idempotency_key. Eleven publiccreate()methods call it with only two arguments —super._create(opts, requestConfig)— so the request config lands in theparamsslot andrequestConfigsilently falls back to its{}default. The compiler cannot see it:Paramsdefaults to{}, andIRequestConfigis structurally assignable to{}, sotscaccepts the call.The effect is that the key each of those methods documents (
@param requestConfig Idempotency key: { idempotency_key: string}) is serialized into the query string and noIdempotency-Keyheader is ever sent.payments.createis where that costs the most: a retried create looks like a brand new request to the API, so the retry can post a second payment. It also places the key in the request URL, where it lands in proxy and access logs.Reproduced on
masterat d0aa5f6Affected today:
accounts.create,entities.create,entities(id).subscriptions.create,entities(id).manualConnect.create,payments.create,reports.create,secrets.create,forwardingRequests.create,webhooks.create,teams.createandteams.publicKeys.create.entities(id).connect.createwas already passing all three arguments and is correct as-is.The change
Each of those eleven call sites now passes
requestConfigin its own positional slot (super._create(opts, undefined, requestConfig)). No public method signature changes,Resource._createis untouched, and the only difference on the wire is the one that was documented all along: the key moves out of the query string and into theIdempotency-Keyheader. Running the same script above against this branch printsrequest url : /paymentsandIdempotency-Key hdr : my-idem-key-123.How it was verified
test/resources/RequestConfig.tests.tsis new and wired intotest/index.ts. It stands up a local HTTP server, points a client at it through thebaseURLoption, and asserts three things for each of the eleven methods: the key is sent as anIdempotency-Keyheader, it is absent from the query string, and no such header is sent when no request config is passed. It needs no API credentials, so it runs anywhere:Against
master'ssrcthose tests report12 passing, 22 failing— the twelve that pass are the controls asserting no header when no request config is given, which is correct on both trees. With this change they report34 passing. There is also a control forentities(id).connect.create, confirming it still sends itsexpandquery parameters and the header, so the one already-correct caller does not move.npx tsc -p tsconfig-test.json(yourpretest) andnpm run buildare green before and after.npm run lintfails identically on both trees on a clean checkout, since.eslintrc.jsreferences@typescript-eslint/eslint-pluginand it is not indevDependencies; that is untouched here. The credentialed suite needsTEST_CLIENT_KEY, which I do not have, so I could not run it — hence the new file being self-contained.One merge note: #127 also adds a line to
test/index.ts, so that file may want a trivial rebase depending on merge order.How this was managed
This work was tracked on a board imported from this repository's own issues and pull requests — 131 stories in all. The story for this change is Idempotency key is sent as a query param instead of the Idempotency-Key header, on the board at https://eastagiletracker.com/projects/336.
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com