fix(kits): restore the extensions' event payload shapes - #3098
Conversation
The 2nd gen migration renamed what the Eventarc payloads carry.
`firestore-counter` and `firestore-translate-text` published
`{change, context}` on `onStart` and `{context}` on `onCompletion`; the kits
published `{data, params}` and `{params}`, following the 2nd gen handler
signature. That drops `context.eventId`, `context.timestamp`,
`context.eventType` and `context.resource` outright and moves the trigger
wildcards, so every subscriber reading them breaks.
Nothing about 2nd gen forces this. A custom event's payload is whatever the
kit hands `channel.publish`, and `FirestoreEvent` carries all five 1st gen
`EventContext` fields under different names, so `toEventContext` rebuilds the
object and the handlers publish the original shape. `event.data` and the 1st
gen `change` are the same `Change<DocumentSnapshot>` and serialize
identically, so the `change` key costs nothing.
`speech-to-text` published the caught `Error` itself, which serializes to
`{"error":{}}` because `message` and `stack` are not enumerable; the kit
published `{message, stack}` instead. That is an upgrade rather than parity,
and it dropped the `name` that `errorFromAny` puts on a thrown non-error, so
the error is published as-is again.
Refs #3027
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the firestore-counter, firestore-translate-text, and speech-to-text kits to maintain backward compatibility with 1st generation event payloads. For the Firestore kits, a new toEventContext helper is introduced to rebuild the 1st gen EventContext from 2nd gen events, ensuring onStart and onCompletion events retain their original shapes. For the speech-to-text kit, the error event payload is reverted to publish the raw error directly, preserving parity with the extension's behavior where genuine Error objects serialize to empty objects. I have no feedback to provide as there are no review comments.
|
Review of this PR's own changes — finding 1 of 2, The event fixtures I added in The assertions pass either way, so the reconstruction is not wrong — but the fixtures pin a |
|
Review of this PR's own changes — finding 2 of 2, no test asserts the serialized The whole point of the fix is what a subscriber reads off the wire, and That leaves the parity claim in the READMEs — |
Two problems in the parity tests. The `firestore-counter` fixtures used the
internal-state path with only a `shardId`, but `handleShardWrite` serves the
`{collection}/{counter=**}/_counter_shards_/{shardId}` trigger, so they pinned
a `resource.name` and `params` map no subscriber receives.
And nothing asserted the serialized payload for either Firestore kit, even
though `firebase-admin` puts it on the wire as `JSON.stringify(data)` and that
round-trip is the whole point of the fix. Both `events.test.ts` suites now
build the context with `toEventContext` and assert what a subscriber reads.
Refs #3027
|
Review of this PR's own changes — finding 3, an unrelated reformat slipped into The - const actual = await vi.importActual<typeof import("../src/controller")>(
- "../src/controller"
- );
+ const actual =
+ await vi.importActual<typeof import("../src/controller")>(
+ "../src/controller"
+ );That is prettier 3 output. The repo pins |
The pre-commit hook shells out to a bare `prettier`, which resolved to a global 3.7.4 in a worktree with no root `node_modules` and reflowed the `vi.importActual` call in the counter handler tests. The repo pins 2.8.8. Refs #3027
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
The investigation in the description is the right shape - checking what v1 actually emits rather than trusting the yaml, and pinning it with a deploy, is what makes this reviewable. eventType, resource.name and the time/id mapping all check out against firebase-functions@4.9.0.
Two things I'd want resolved before merge: the publisher role (none of these three kits can publish at all today, and the README now tells users these payloads arrive), and the params claim, which I think is wrong in a way the README repeats.
The rest are smaller. Detail inline.
Note on provenance: this was an AI-led review, run against the branch and the original extension sources, and I've read the findings rather than re-derived every one of them myself. The file:line references and the SDK behaviour claims are worth checking against the source before you act on them, particularly the params one. Push back where it's wrong.
| - The three functions, the event types they publish and their payloads: | ||
| `onStart` still carries `{change, context}` and `onCompletion` still carries | ||
| `{context}`, with `context.eventId`, `context.timestamp`, `context.eventType`, | ||
| `context.resource` and the trigger wildcards under `context.params`. Aside | ||
| from the worker point above. |
There was a problem hiding this comment.
Anchoring here because src/index.ts isn't in the diff, but this is the finding I'd most want closed before merge.
All three kits declare roles/eventarc.eventReceiver and no roles/eventarc.publisher (firestore-counter/src/index.ts:40, firestore-translate-text/src/index.ts:36, speech-to-text/src/index.ts:38). I think that's why you saw the 403 on every publish.
It isn't only that the payload is invisible. handleShardWrite awaits recordStartEvent as its first action (handlers.ts:67), so a user who sets EVENTARC_CHANNEL gets a function that throws before it aggregates anything. Same for translate at handlers.ts:75.
That matters more now than it did before this PR, because this bullet tells users these payloads arrive. Right now they don't arrive at all.
I'd add the publisher role (one line per kit, same class as the roles already declared) and re-run the deploy test. That should also get you the subscriber-side view you couldn't capture. If there's a reason the Extensions install flow is meant to grant this rather than extension.yaml, say so and let's file the issue and caveat the README, rather than leaving it unlinked.
Small thing on the same bullet: it ends on the fragment "Aside from the worker point above."
| service: FIRESTORE_SERVICE, | ||
| name: `projects/${event.project}/databases/${event.database}/documents/${event.document}`, | ||
| }, | ||
| params: event.params, |
There was a problem hiding this comment.
I don't think the extension published any params here, so "still ... under context.params" in the README isn't parity, it's a superset.
In v1, context.params isn't supplied by the platform. The SDK computes it from the code-side trigger path (_makeParams, firebase-functions/lib/v1/cloud-functions.js:150). The extensions registered document(process.env.INTERNAL_STATE_PATH) (firestore-counter/functions/src/index.ts:80) and document(process.env.COLLECTION_PATH) (firestore-translate-text/functions/src/index.ts:47). Neither has a {wildcard} segment, so WILDCARD_REGEX matches nothing and params comes out {}. The yaml wildcards registered the trigger, they never reached the SDK.
Worth verifying yourself before you change anything, since you already have the deploy set up. Harmless either way, a superset breaks nobody, but the README and the description both claim a parity I don't think holds.
The more general point: the deploy test ran the kit's own toEventContext over 2nd gen events, so it can only confirm the kit is self-consistent. Nothing in it observes a real 1st gen context. If you can get one out of the emulator against the old extension, commit it as a fixture and assert event-context.test.ts against it. Then the parity claim is pinned rather than argued.
There was a problem hiding this comment.
One precision after reading _makeParams properly: the only way the extension's params was non-empty is if a user put their own {wildcard} segments inside COLLECTION_PATH itself (the validation regex allows braces, and the code-side path is that env var verbatim). Even then messageId never appears, because it only exists in the yaml resource. So the exact statement is "empty by default, and never the yaml wildcards". The rest of the comment stands.
| // Parity with the extension: `message` and `stack` are not enumerable, so | ||
| // subscribers receive `{"error":{}}` for a genuine `Error`. | ||
| const payload = publish.mock.calls[0][0]; | ||
| expect(JSON.parse(JSON.stringify(payload)).data).toEqual({ error: {} }); |
There was a problem hiding this comment.
The {"error":{}} framing holds for new Error(...), but I don't think it's what this path actually produces.
The catch-all at handlers.ts:199 wraps remoteFile.download() (handlers.ts:133), which throws ApiError from @google-cloud/storage. code, errors, response and message are all enumerable own properties on it, so a subscriber gets something like {"error":{"code":404,"errors":[...],"message":"..."}}. Your own deploy log shows ApiError: No such object, so that's the realistic case.
Parity with the extension is unaffected either way, it published the raw error too. But this test and the README at :217-221 both describe the rare case as the general one. I'd reword to something like "carries whatever enumerable fields the thrown error has; a plain Error gives {}", and add an ApiError-shaped case so the realistic payload is pinned too.
One thing I did check: the response object isn't a leak risk, teeny-request gives it a toJSON that returns headers only.
| }); | ||
| }); | ||
|
|
||
| test("puts the whole 1st gen context on the wire", async () => { |
There was a problem hiding this comment.
A few small ones, none blocking:
- This test mocks
firebase-admin/eventarc, so the real serializer (toCloudEventProtoFormat) never runs, andrecordStartEventis a pass-through. It ends up being theevent-context.test.tsassertion again with a hand-rolledJSON.stringify. Not wrong, but the comment claims more than it proves. Either drop it, or publish something where JSON semantics actually differ fromtoEqual(an error-bearing payload would). - No failure-mode coverage.
strictis off, sotimestamp: undefinedcompiles fine and the key just vanishes from the wire. One test for a missingevent.timewould catch that. SHARD_WRITEhere (:21-28) and the fixture inevent-context.test.ts:25-32are bothas any. Translate centralises this inmakeEvent(tests/helpers.ts:102), still a cast but only in one place. Worth giving counter the same.expectedEventContext(tests/helpers.ts:122) rebuilds theprojects/.../databases/.../documents/...template at:131from the same parts the source uses, so the same mistake in both places passes. A literal expected string is stronger.firestore-translate-text/README.md:216says "The four event payloads:" and then describes two.
2 participants
Closes #3027.
Investigation
The decision in #3027 was whether the payload rename is intentional 2nd gen design. It is not, and nothing about 2nd gen forces it:
channel.publish—firebase-adminjustJSON.stringifys it. There is no 2nd gen wire format for custom events, so{data, params}is only the 2nd gen handler's local variable names leaking into the payload.FirestoreEventcarries every 1st genEventContextfield under a different name (id,time,project/database/document,params), so the 1st gencontextis fully reconstructible.changeand 2nd genevent.dataare the samefirebase-adminChange<DocumentSnapshot>and serialize identically, so keeping thechangekey costs nothing.context.eventId,context.timestamp,context.eventTypeandcontext.resourcewere dropped from the payload entirely, so this was a regression on top of a consumer break.For
speech-to-text, the extension published the caughtErroritself.messageandstackare not enumerable, so subscribers received{"error":{}}; the kit published{message, stack}instead. That is an upgrade, not parity, and it also dropped thenamethaterrorFromAnyputs on a thrown non-error. Restored to parity, with the improvement left to be tracked separately.Changes
src/event-context.tstofirestore-counterandfirestore-translate-text, withtoEventContextrebuilding the 1st genEventContext(eventId,timestamp,eventType,resource.service/resource.name,params) from aFirestoreEvent.eventTypeis the 1st gen constantgoogle.firestore.document.writethat every 1st gen FirestoreonWritetrigger reported.firestore-counterhandleShardWritepublishesonStartas{change, context}andonCompletionas{context}again, instead of{data, params}and{params}.firestore-translate-texthandleDocumentWritedoes the same, across all threeonCompletioncall sites.speech-to-textrecordErrorEventpublishes{ error }again instead of{ error: { message, stack } }.firestore-counterandfirestore-translate-textREADMEs and the "failevents for unexpected errors now say what went wrong" note from thespeech-to-textREADME, and recorded the payloads under each kit's "Unchanged" list instead.event-context.test.tsto both kits; pinned the rebuilt payloads in thehandlersandeventssuites, including a JSON round-trip assertion on what a subscriber reads off the wire; added aspeech-to-textcase for the thrown-non-error payload keeping itsname, and reworked the raw-Errorcase to assert the{"error":{}}the extension produced.Testing
Unit: 81 tests in
firestore-counter, 94 infirestore-translate-text, 34 inspeech-to-text, all passing.tsc -bclean for all three, andprettier@2.8.8 --list-differentclean over every file touched. The new wire-level assertion was checked as non-vacuous by breakingeventTypeand confirming it fails.Deployed: all three kits were
npm packed intoproject/function-kits/*/source/vendorand the installed bundles confirmed to contain the new code. The four overlappingkit-firestore-translate-text-*-fstranslatefunctions incorie-testingwere deleted and confirmed gone fromfunctions:listbefore deployingfirestore-counter-a91f6c2e,firestore-translate-text-a91f6c2eandspeech-to-text-a91f6c2e.Payload, from real platform events. Eventarc publishing 403s in this project, so a subscriber cannot see the payload (see below); instead a temporary codebase held Firestore triggers on the same document patterns, running the kit's own compiled
toEventContexton the real events.firestore-counter, on a shard write:{"change":{"before":{...},"after":{"_fieldsProto":{"counter":{"integerValue":"1"}},...}}, "context":{"eventId":"84b2826b-9cbb-488c-8db3-098edb1b02b0", "timestamp":"2026-09-02T14:53:29.897039Z", "eventType":"google.firestore.document.write", "resource":{"service":"firestore.googleapis.com", "name":"projects/corie-testing/databases/(default)/documents/fxkits_range_counters/page-1788360801506/_counter_shards_/0000"}, "params":{"collection":"fxkits_range_counters","counter":"page-1788360801506","shardId":"0000"}}}firestore-translate-text, on a document write: same fivecontextfields,resource.nameprojects/corie-testing/databases/(default)/documents/fxkits_range_tr_gemini/smoke-1788360897518,params{"messageId":"smoke-1788360897518"}.Indirectly affected behaviour, with the channel unset as it was before: the translate handler still translates end to end —
{"input":"hello world","translated":{"en":"hello world","es":"hola mundo"}}— and the counter still aggregates, the shard write landing as{"counter":1}on the counter document.speech-to-text: its changed path is the catch-all handler, provoked by finalizing anaudio/wavobject and deleting it so the download throws inside thetry.ApiError: No such object: …was logged fromlogs.error(error)and the invocation completed, so the error path is intact. The payload itself is covered only by the unit round-trip, for the reason below.Afterwards the temporary codebase and its functions were deleted, the three other
firestore-translate-textinstances redeployed, and the smoke documents removed.Out of scope
corie-testingfor every kit that publishes events: the kits declareroles/eventarc.eventReceiverbut no publisher role, and the extensions got publish rights on the channel from the Extensions install flow instead ofextension.yaml. It is pre-existing and unrelated to the payload shape — the handlers reachrecordStartEventand fail there — but it is worth its own issue, since withEVENTARC_CHANNELset the awaited publish also takes the handler down with it.firestore-bigquery-exportkeeps the extension'scontextkey ononStart, but fills it with the 2nd gen event (context.id,context.time) rather than a 1st genEventContext. Same class of break, different payload; not currently tracked in Kits parity issues #2974.firestore-vector-searchpublishes{params}, but the extension published nothing at all from that path (it declared the event types and never used them), so there is no extension payload to be at parity with.changesnapshots serialize withfirebase-admininternals in both the extension and the kit, but the kit is onfirebase-admin@14where the extension was on@12, so the internal field set differs slightly. That is an SDK-upgrade artifact this PR does not change — before it, the same snapshots were serialized underdata.