Repository navigation
fix(functions): make the v2 callable handlers actually work 7352ce2 fix(functions): hand addNumbers/addMessage the whole v2 request 8f85835 fix(functions): put back the sanitizer call and the admin imports - #2845
Open
rootkiller6788 wants to merge 3 commits into
Open
rootkiller6788 wants to merge 3 commits into
rootkiller6788 wants to merge 3 commits into
Conversation
firebase-functions v2 calls an onCall handler with a single
CallableRequest ({data, auth, instanceIdToken}), not the old
(data, context) pair. firebase#1701 moved the sample to the v2 import path
but kept the v1 handler shape, so addNumbers read data.firstNumber
off the request wrapper and always answered invalid-argument, and
addMessage blew up on context.auth.
The androidTest TestAddNumber expects 32 + 16 = 48, which never
came back.
Two things got lost in the same v2 pass. The require for firebase-admin went away while addMessage still called admin.database(), and the qualified call turned into a bare sanitizeText() that is not defined in this file. Both are ReferenceErrors at request time, so addMessage never once wrote a message. Switched to the modular admin entry points (firebase-admin/database, firebase-admin/messaging) to match the initializeApp import that is already here.
rootkiller6788
marked this pull request as ready for review
September 27, 2026 13:10
Contributor
There was a problem hiding this comment.
Code Review
This pull request migrates Firebase Cloud Functions from v1 to v2, updating the onCall handlers to accept a single request parameter and replacing deprecated admin SDK calls with modular Firebase Admin SDK imports. Feedback on these changes highlights a critical issue where request.instanceIdToken is no longer available directly in v2 and must be retrieved from the raw request headers. Additionally, it is recommended to use optional chaining when accessing properties on request.data to prevent potential runtime TypeError crashes when functions are invoked with empty payloads.
A callable invoked without a body leaves data undefined, and reading firstNumber/text off it threw a TypeError instead of the intended invalid-argument HttpsError. Optional chaining lets the existing checks reject the call cleanly.
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 free
to 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.
The Cloud Functions sample for the callable-functions quickstart has been dead since #1701 moved it to the v2 imports. That pass swapped the require to firebase-functions/v2/https but left the handler in the v1 shape, so addNumbers was reading firstNumber off the CallableRequest wrapper and answering invalid-argument for every call. TestAddNumber in the androidTest sources waits 60s for "48" and would never see it.
The same pass also dropped
const admin = require('firebase-admin')while addMessage still called admin.database(), and turned sanitizer.sanitizeText(text) into a bare sanitizeText(text) that is not defined in the file. addMessage therefore threw a ReferenceError before it ever reached the database write, so it had never written a single message.I moved the handlers to the single
requestargument, switched to the modular admin entry points (firebase-admin/database, firebase-admin/messaging) to match the initializeApp import that is already there, and restored the qualified sanitizer call.How I checked it: no Android SDK on this box, so I installed the exact versions pinned in package-lock.json (firebase-functions 6.1.0, firebase-admin 12.7.0) and drove the real handlers through a small harness, faking only the network boundary (RTDB/FCM/initializeApp). Before: addNumbers threw invalid-argument and addMessage threw ReferenceError: sanitizeText is not defined, then ReferenceError: admin is not defined once the qualifier was back. After: 13 checks pass, including 32 + 16 = 48, the author fields coming off the auth token, the two validation errors, and the push path with and without an instance ID token.
CI does not catch this because the workflow only runs
assemble; it never deploys the functions or runs the androidTest.One unrelated thing I noticed but did not touch:
capitalize-sentence: ^0.1.2in functions/functions/package.json cannot be installed right now. The registry only has 1.0.0, and the 0.1.5 pinned in package-lock.json 404s, so npm install/npm ci fails for that folder before anything else runs. Probably worth a separate dependency bump.