Repository navigation
fix: replace the association on the same event type when attaching - #38
hideokamoto wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. Summary
Merge Risk:
|
…ersion
attatchEdgeFunction only removes an association whose ARN matches exactly
before appending, so attaching edge-fn:3 while edge-fn:2 is on
viewer-request leaves two viewer-request associations (Quantity 2).
CloudFront allows only one function per event type.
The existing fixtures ('edge-lambda-arn') have no version, so they did not
cover this case. These tests use versioned ARN fixtures and fail on the
current code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bmzxkp9hit6dRSrgRbVgx4
CloudFront allows only one function per event type in a cache behavior. attatchEdgeFunction now drops every association whose EventType equals the target event type of the default cache behavior before appending the new one, instead of dropping only an exact ARN match. Attaching edge-fn:3 while edge-fn:2 (or another function) is on viewer-request now leaves a single viewer-request association. Associations for other event types and the other cache behaviors are left unchanged. detatchEdgeFunction still removes only the exact ARN. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bmzxkp9hit6dRSrgRbVgx4
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bmzxkp9hit6dRSrgRbVgx4
b7c7591 to
41bfc74
Compare
Summary
attatchEdgeFunctiononly removes an association whose ARN matches exactly before it appends the new one. When another version of the same function (or another function) is already on the target event type, the config sent toUpdateDistributionends up with two associations for that event type. CloudFront allows only one function per event type.This PR makes attach replace whatever is associated with the target event type of the default cache behavior. Detach is unchanged.
Cause
At
1d57cc8(master;libs/configUpdator.tsis unchanged since999a8e8):libs/configUpdator.ts:136-153attatchEdgeFunctioncallsdetatchEdgeFunction(L137) and then pushes the new association (L145-149).libs/configUpdator.ts:115-121detatchEdgeFunctiondrops an item only whenitem.EventType === this.eventTypeandisTargetLambdaArn(item.LambdaFunctionARN)is true.libs/configUpdator.ts:96-99isTargetLambdaArnis a strict===comparison, so...:function:edge-fn:2is not treated as the target when the controller holds...:function:edge-fn:3.Result: starting from
viewer-request -> edge-fn:2and attachingedge-fn:3gives twoviewer-requestitems andQuantity: 2(see the Red output below).The existing tests use the fixture
'edge-lambda-arn', which has no ARN shape and no version, and only re-attach the same ARN. That is why they did not cover this case.The CloudFront rule
From
node_modules/@aws-sdk/client-cloudfront/dist-types/models/models_0.d.ts:717(@aws-sdk/client-cloudfront3.879.0, the version inpnpm-lock.yaml), the doc comment ofLambdaFunctionAssociations, quoted verbatim:Unconfirmed: whether real CloudFront actually rejects an
UpdateDistributionrequest with two associations for one event type. I did not test against real AWS. An earlier local run against an AWS API emulator produced the duplicatedviewer-requestassociation with this library, and the emulator accepted the update, so the emulator does not tell us either way. I did not re-run that emulator check for this PR.Design choice
Two options were considered:
Why (A):
README.md:21,README.md:26). A controller is built with one ARN and one event type, andattachEdgeFunctionis the only way to put a function on an event type. With (B), switching to a different function would throw. The caller would first have to build a second controller with the old function's exact versioned ARN and detach it. That is twoUpdateDistributioncalls (two deployments) with a window where nothing runs on that event type.$LATESTor alias ARNs, and (B) would need extra rules for those.What (B) would have given us: protection against replacing a function that was put there by someone else by mistake. If you prefer that safety, the change is small and I can rework the PR.
Behaviour change
The output changes only when the target event type of the default cache behavior already has an association with a different ARN:
Quantity+1QuantityunchangedQuantity+1Quantityunchangedfix:without aBREAKING CHANGEfooter, so under the README's Release Process (semantic-release,fix:-> patch) this would ship as a patch. If you consider the second row a breaking change, the fix commit needsfix!:or aBREAKING CHANGE:footer to get a major release; tell me and I will reword it.CacheBehaviorsare not touched (same as before). The new association is created withoutIncludeBody, as before. When it replaces an association that hadIncludeBody: trueon the same event type, that flag is not carried over (re-attaching the same ARN already behaved this way).README.md and README_ja.md now describe this for
attachEdgeFunctionanddetachEdgeFunction.Tests (Red -> Green)
New tests use versioned, ARN-shaped fixtures (
arn:aws:lambda:us-east-1:123456789012:function:edge-fn:3, the AWS documentation placeholder account also used in the README):configUpdator.test.ts, attach: replaces another version of the same function; replaces a different function; keepsorigin-request(withIncludeBody) andviewer-responseassociations, at most one association per event type, andQuantityequalsItems.length; leavesCacheBehaviorsunchanged; re-attaching the same version keeps one association.configUpdator.test.ts, detach: removes only the exact version.controller.test.ts:attachEdgeFunctionwith a stubbedCloudFrontClientsends oneviewer-requestassociation inUpdateDistribution.Red
Commit 3123eb5 (tests only, on top of master
1d57cc8).pnpm run test --ciexited 1:Tests: 5 failed, 14 passed, 19 total.Full output
Green
Commit 060a363 (the fix).
pnpm run test --ci --verboseexited 0:Tests: 19 passed, 19 total.Full output
Other checks (head of this branch
41bfc74, Node 22.23.4,pnpm install --frozen-lockfile)The same steps as the GitHub Actions workflow:
pnpm run lint: exit 0pnpm run typecheck: exit 0pnpm run test --ci: 19 passedpnpm run build: exit 0This branch was rebased onto #37 (yarn -> pnpm) after the PR was opened. The Red and Green output above is from the rebased commits (each run used a
git archiveexport of that commit, which is why the paths in the output are not a repository checkout). Before the rebase, the same tests gave the same Red result (5 failed, 14 passed) and the same Green result (19 passed) with yarn.Notes on existing tooling (not changed in this PR):
pnpm run lintonly coverslibs/*.ts.__tests__/.eslintrc.ymlis not valid YAML (bad indentation of a mapping entry (2:46)), so ESLint cannot run on the tests directory as is. Running ESLint on the tests with the root config reports nothing on the new lines.commitlint.config.jsfails to load becausepackage.jsonhas"type": "module". I checked the three commit messages with the same rules (@commitlint/config-conventional) from a.cjsconfig outside the repo: no problems..releaserc.jsonlistsmainas the release branch, while this repository's default branch (and the base of this PR) ismaster.Not in this PR
🤖 Generated with Claude Code
https://claude.ai/code/session_01Bmzxkp9hit6dRSrgRbVgx4