fix: route v2 callables through wrapV2 when platform is gcfv2 - #311
fix: route v2 callables through wrapV2 when platform is gcfv2#311IzaakGough wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for identifying and wrapping GCFv2 callable functions by checking the platform field in the endpoint metadata. Key changes include exporting the isCallableV2Function utility, updating the wrap function to prioritize v2 callable detection, and adding a corresponding test case in spec/main.spec.ts to ensure correct routing. There are no review comments to address, and I have no further feedback to provide.
cabljac
left a comment
There was a problem hiding this comment.
This looks great. The __endpoint.platform === 'gcfv2' check is a much more stable signal than counting function parameters, and the regression test covers the routing path nicely. A couple of small questions below, neither blocking.
cabljac
left a comment
There was a problem hiding this comment.
The callable fix itself looks right to me: platform check first with the arity heuristic as fallback is the correct order, and the v1-callable negative test plus the realistic rest-param shape in the regression test address everything from my last review. Ran this against both firebase-functions 4.9.0 and 7.2.5 and the routing behaves. One behavioral concern inline before this goes in, plus a couple of smaller ones.
| function isV2CloudFunction<T extends CloudEvent<unknown>>( | ||
| cloudFunction: any | ||
| ): cloudFunction is CloudFunctionV2<T> { | ||
| if (cloudFunction?.__endpoint?.platform === 'gcfv2') { |
There was a problem hiding this comment.
This check routes everything with platform gcfv2 to wrapV2, not just callables. That silently changes behavior for v2 onSchedule and v2 blocking functions: from what I can see they have outer length 0 even on firebase-functions 4.9.0, so today they go through wrapV1 (onSchedule handlers get {eventId, timestamp, ...} from the scheduleTrigger path, blocking handlers get exactly the event object passed in), and after this change they get the generic CloudEvent mock shape instead, with keys like specversion and source merged in. Anyone asserting on the old shapes breaks silently. Can you confirm this repro on your side?
If it holds: either add scheduleTrigger/blockingTrigger handling in wrapV2 so those families get sensible events, or pin the new behavior with tests and call the change out in the release notes. Transferable rule: when a dispatch condition widens from "this specific kind" to "this whole platform", every function family on that platform changes route, so each one needs a test proving its behavior stayed intact or a deliberate note that it changed.
There was a problem hiding this comment.
Confirmed this repro on my end. I dug into this a bit more and I think the widening isn't actually needed. It looks like only v2 callables have the arity misrouting problem. I think just dropping the isV2CloudFunction and relying on the new isCallableV2Function is cleaner way to go. Let me know what you think.
| const v2Callable = cloudFunction as CallableFunction<any, T>; | ||
| return wrapV2(v2Callable); | ||
| } | ||
| if (isV2CloudFunction<V>(cloudFunction)) { |
There was a problem hiding this comment.
Smaller regression: wrap(v2 onRequest) previously threw the clear "Wrap function is only available for 'onCall' HTTP functions, not 'onRequest'" from the v1 path. Now it routes to wrapV2 and dies in assertIsCloudFunction with "This library can only be used with functions written with firebase-functions v3.20.0 and above", which points users in exactly the wrong direction. Please check for __endpoint.httpsTrigger and rethrow the onRequest-specific message.
| }); | ||
|
|
||
| describe('v2 callable functions', () => { | ||
| it('should route to v2 path when outer and run have length 0', () => { |
There was a problem hiding this comment.
The regression test is well built, the hand-rolled rest-param shape matches what v7 actually emits. One gap worth knowing about: because devDeps pin firebase-functions ^4.9.0, CI only ever runs against 4.9.0, so this test encodes our belief about the v7 shape rather than checking the real thing. Mocks like this go stale silently: if v8 changes the wrapper shape, every test still passes while wrap() breaks for users, which is exactly the failure mode that made #310 possible long before your change. I've raised #333 for testing against multiple firebase-functions majors in CI. Transferable rule: when a bug comes from a dependency changing shape underneath us, the fix wants two parts, the code change and a test that runs against the real dependency so the next shape change can't land silently.
| | WrappedFunction<T> | ||
| | WrappedV2Function<V> | ||
| | WrappedV2CallableFunction<T> { | ||
| if (isCallableV2Function(cloudFunction)) { |
There was a problem hiding this comment.
nit: this branch is behaviorally redundant now, anything passing it also passes the platform check below, and wrapV2 re-checks it internally. I believe it only exists to select the TS overload. A one-line comment saying so would stop the next reader trying to delete it.
Fixes #310
Summary
Fixes
wrap()misrouting v2onCallfunctions to the v1 test helper when used withfirebase-functions@7.x.wrap()previously chose betweenwrapV2andwrapV1usingFunction.lengthi.e.In
firebase-functions@7, v2 callables are wrapped with rest-parameter helpers (withInit,wrapTraceContext), so both lengths are0.wrap()fell through towrapV1, which invokesrun(data, context)instead ofrun({ data, auth, ... }), breaking auth and otherCallableRequestfields in unit tests.Affected versions:
firebase-functions-test@3.4.1,3.5.0(and any release using the arity-only heuristic without this fix).Issue reproduced here: MRE
Problem
With
firebase-functions@7+firebase-functions-test@3.4.1/3.5.0:onCall()that checksrequest.authwrap()and call it with an auth contextrequest.auth→ spuriouspermission-deniederrorsRoot cause:
isV2CloudFunctionreturnsfalsefor real v2 callables, sowrapV1is used for a v2 handler.Solution
wrapV2before the arity check, using stable SDK metadata:__endpoint.callableTrigger__endpoint.platform === 'gcfv2'(v1 callables also havecallableTriggerbut usegcfv1)isCallableV2Functionfromsrc/v2.tsfor reuse and testsisV2CloudFunctionto also returntruewhenplatform === 'gcfv2', so other v2 triggers (Pub/Sub, alerts, etc.) are not misrouted when.length === 0What changed vs original
wrap()Original behavior
isV2CloudFunctionwas only:New behavior
isCallableV2Function(new, exported fromsrc/v2.ts):isV2CloudFunction(updated):Routing flow
onCall+wrap(fn)({ data, auth })wrapV1→run(data, context)→ auth brokenwrapV2→run({ data, auth })→ auth worksonCall(callableTrigger,gcfv1)wrapV1→(data, { auth })length === 0wrapV1platform === 'gcfv2'→wrapV2For v2 callables,
wrapV2invokes:The
as CallableFunctioncast is for TypeScript only (wrap()accepts a broad union;wrapV2has separate overloads for callables vs event functions).Files changed
src/main.tsisCallableV2Functionbranch inwrap();isV2CloudFunctionchecksplatform === 'gcfv2'src/v2.tsisCallableV2FunctionwithcallableTrigger+gcfv2platform guardspec/main.spec.tslength === 0routes to v2 pathTesting
npm test(all 119 tests pass)Backward compatibility
wrapV1(platformisgcfv1or absent)wrapV2single-argumentCallableRequestAPIplatform === 'gcfv2'and arity heuristic fails