Skip to content

fix: [SDK-5336] reuse matching default Firebase app - #2762

Merged
fadi-george merged 2 commits into
firebase-installation-idsfrom
fadi/sdk-5336
Sep 23, 2026
Merged

fadi-george merged 2 commits into
firebase-installation-idsfrom
fadi/sdk-5336

Conversation

@fadi-george

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Reuse a conclusively matching default Firebase app for legacy FCM token registration.

Details

Motivation

Preserve the customer's existing FCM token and update the same OneSignal subscription when migrating to OneSignal, while retaining the named OneSignal Firebase app as the safe fallback.

Scope

Legacy FCM-token mode now reuses an already initialized default Firebase app only when both gcmSenderId and the project number parsed from applicationId match the resolved sender ID. Missing, malformed, contradictory, or mismatched defaults continue through ONESIGNAL_SDK_FCM_APP_NAME and emit a warning. FID mode and arbitrary token-request failure behavior are unchanged.

Testing

Unit testing

Added coverage for matching, programmatic, absent, malformed, contradictory, and mismatched Firebase configurations; runtime token failures; FID isolation; and updating an existing subscription after token changes.

Automated checks:

  • Focused notifications and subscription manager unit tests
  • Spotless
  • Detekt
  • Notifications release assembly

Manual testing

Not run on a physical device; behavior is covered by Robolectric/unit tests and a release build.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

@fadi-george
fadi-george requested a review from a team as a code owner September 22, 2026 23:50
@fadi-george
fadi-george changed the base branch from main to firebase-installation-ids September 23, 2026 00:00

@cursor cursor 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.

Multi-model review (Opus 5.5, GPT 5.6 Sol, Grok 4.7)

Threshold: 23 files, +1461/−69 = 1530 (no generated/lock/vendor/snapshots).

SDK-5336’s dual-field match for legacy default-app reuse looks correct: a token is not minted from a default app whose gcmSenderId or applicationId project number disagrees with the resolved sender.

Act on (2/3)

  • FID mode still trusts gcmSenderId alone (firebaseAppSenderId). The same contradictory options that fail legacy reuse are accepted and registered; the new test pins that. The comment above installationIdRegistration says sender, app id, and API key must be one project.
  • -32/-33 are not retryable. PushTokenManager starts each process with pushToken = null, so a tokenless permanent FID error overwrites a persisted SUBSCRIBED address. Same-instance tests hide this.

Consider

  • Reuse does not require projectId/apiKey; a matching-but-unusable default has no fallback (Opus).
  • App choice is not persisted, so late programmatic FirebaseApp init can flip tokens across sessions (Opus).
  • onRegistered still does not refresh a rotated FID (GPT; carry-forward).

Noted: warn on a missing default app (normal for the gms flavor); shared default-app token isolation. Dismissed: no-fallback-after-match as a blocker (stated contract); ERROR(9999) mapping as a new regression (lone Grok, documented).

Open in Web View Automation 

Sent by Cursor Automation: PR Reviews


internal object FCMTokenProvider {
fun firebaseAppSenderId(
senderId: String?,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

FID sender resolution returns gcmSenderId even when applicationId's project number disagrees. Legacy reuse rejects that pair (FCMLegacyAppSelector.matches). The new test does not apply legacy application id matching to installation id mode then registers that contradictory default app and uploads its installation id. validateSenderId only compares this derived sender, so the mismatch never becomes INVALID_FCM_SENDER_ID.

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 test is the isolation this PR is for. FID cannot fall back to ONESIGNAL_SDK_FCM_APP_NAME: installationIdRegistration says that app pairs the customer sender with OneSignal's shared credentials. firebaseAppSenderId returning gcmSenderId is the existing FID check, and this change does not touch it. A contradictory applicationId stays on the host default app on purpose.

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, but this is existing FID behavior on the base branch. SDK-5336 intentionally leaves FID unchanged, so this is out of scope here.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george

Copy link
Copy Markdown
Contributor Author

Checked the review findings:

  • FID accepting contradictory gcmSenderId/applicationId: confirmed, but it's existing FID behavior on the base branch. Out of scope, since SDK-5336 leaves FID unchanged.
  • -32/-33 overwriting a persisted SUBSCRIBED status after restart: confirmed, but it comes from the earlier FID status work. PushTokenManager isn't touched in this PR, so it's out of scope.
  • Consider items: they follow SDK-5336's stated rules (both IDs must match, no fallback after a matching app fails, warn on fallback). Leaving as is.

@abdulraqeeb33 abdulraqeeb33 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.

Legacy default-app reuse matches on both gcmSenderId and the applicationId project number, and FID stays on the host default app.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • PushRegistratorFCM.kt: 21/21 touched executable lines (100.0%) (47 touched lines in diff)

Overall (aggregate gate)

21/21 touched executable lines covered (100.0% — requires ≥ 80%)

📥 View workflow run

@fadi-george
fadi-george merged commit f2769c1 into firebase-installation-ids Sep 23, 2026
5 checks passed
@fadi-george
fadi-george deleted the fadi/sdk-5336 branch September 23, 2026 19:57
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.

2 participants