Skip to content

fix: route v2 callables through wrapV2 when platform is gcfv2 - #311

Open
IzaakGough wants to merge 7 commits into
masterfrom
@invertase/fix-wrap-v2-callable-detection
Open

fix: route v2 callables through wrapV2 when platform is gcfv2#311
IzaakGough wants to merge 7 commits into
masterfrom
@invertase/fix-wrap-v2-callable-detection

Conversation

@IzaakGough

@IzaakGough IzaakGough commented May 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #310

Summary

Fixes wrap() misrouting v2 onCall functions to the v1 test helper when used with firebase-functions@7.x.

wrap() previously chose between wrapV2 and wrapV1 using Function.length i.e.

cloudFunction.length === 1 && cloudFunction.run.length === 1

In firebase-functions@7, v2 callables are wrapped with rest-parameter helpers (withInit, wrapTraceContext), so both lengths are 0. wrap() fell through to wrapV1, which invokes run(data, context) instead of
run({ data, auth, ... }), breaking auth and other CallableRequest fields 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:

  1. Define a v2 callable with onCall() that checks request.auth
  2. Wrap it with wrap() and call it with an auth context
  3. The handler never receives request.auth → spurious permission-denied errors

Root cause: isV2CloudFunction returns false for real v2 callables, so wrapV1 is used for a v2 handler.

Solution

  • Route v2 callables to wrapV2 before the arity check, using stable SDK metadata:
    • __endpoint.callableTrigger
    • __endpoint.platform === 'gcfv2' (v1 callables also have callableTrigger but use gcfv1)
  • Export isCallableV2Function from src/v2.ts for reuse and tests
  • Broaden isV2CloudFunction to also return true when platform === 'gcfv2', so other v2 triggers (Pub/Sub, alerts, etc.) are not misrouted when .length === 0

What changed vs original wrap()

Original behavior

if (isV2CloudFunction(cloudFunction)) {
  return wrapV2(cloudFunction);  // v2 path
}
return wrapV1(cloudFunction);    // everything else, including misrouted v2 callables

isV2CloudFunction was only:

cloudFunction.length === 1 && cloudFunction.run?.length === 1

New behavior

if (isCallableV2Function(cloudFunction)) {
  const v2Callable = cloudFunction as CallableFunction<any, T>;
  return wrapV2(v2Callable);
}
if (isV2CloudFunction(cloudFunction)) {
  return wrapV2(cloudFunction as CloudFunctionV2<V>);
}
return wrapV1(cloudFunction);

isCallableV2Function (new, exported from src/v2.ts):

!!cf?.__endpoint?.callableTrigger && cf.__endpoint.platform === 'gcfv2'

isV2CloudFunction (updated):

if (cloudFunction?.__endpoint?.platform === 'gcfv2') {
  return true;
}
return cloudFunction.length === 1 && cloudFunction?.run?.length === 1;

Routing flow

Scenario Original After fix
v2 onCall + wrap(fn)({ data, auth }) Often wrapV1run(data, context) → auth broken wrapV2run({ data, auth }) → auth works
v1 onCall (callableTrigger, gcfv1) wrapV1(data, { auth }) Unchanged
v2 Pub/Sub / alerts with length === 0 Could hit wrapV1 platform === 'gcfv2'wrapV2

For v2 callables, wrapV2 invokes:

(req: CallableRequest) => cloudFunction.run(req)

The as CallableFunction cast is for TypeScript only (wrap() accepts a broad union; wrapV2 has separate overloads for callables vs event functions).

Files changed

File Change
src/main.ts Early isCallableV2Function branch in wrap(); isV2CloudFunction checks platform === 'gcfv2'
src/v2.ts Export isCallableV2Function with callableTrigger + gcfv2 platform guard
spec/main.spec.ts Regression test: v2 callable with outer/run length === 0 routes to v2 path

Testing

  • npm test (all 119 tests pass)

Backward compatibility

  • v1 callables: unchanged — still routed via wrapV1 (platform is gcfv1 or absent)
  • v2 callables: now correctly use wrapV2 single-argument CallableRequest API
  • v2 event functions: improved routing when platform === 'gcfv2' and arity heuristic fails

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@IzaakGough
IzaakGough marked this pull request as ready for review May 15, 2026 14:00
@IzaakGough
IzaakGough requested a review from cabljac May 15, 2026 14:00

@cabljac cabljac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spec/main.spec.ts
Comment thread src/main.ts
Comment thread src/main.ts

@cabljac cabljac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/main.ts
function isV2CloudFunction<T extends CloudEvent<unknown>>(
cloudFunction: any
): cloudFunction is CloudFunctionV2<T> {
if (cloudFunction?.__endpoint?.platform === 'gcfv2') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/main.ts
const v2Callable = cloudFunction as CallableFunction<any, T>;
return wrapV2(v2Callable);
}
if (isV2CloudFunction<V>(cloudFunction)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spec/main.spec.ts
});

describe('v2 callable functions', () => {
it('should route to v2 path when outer and run have length 0', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/main.ts
| WrappedFunction<T>
| WrappedV2Function<V>
| WrappedV2CallableFunction<T> {
if (isCallableV2Function(cloudFunction)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

firebase-functions-test@3.4.1 misdetects v2 callables

2 participants