Skip to content

feat(messaging): migrate topic subscriptions to FCM v1 API#3211

Draft
lahirumaramba wants to merge 1 commit into
mainfrom
lm-fcm-topics
Draft

feat(messaging): migrate topic subscriptions to FCM v1 API#3211
lahirumaramba wants to merge 1 commit into
mainfrom
lm-fcm-topics

Conversation

@lahirumaramba

Copy link
Copy Markdown
Member

Migrate subscribeToTopic and unsubscribeFromTopic methods in admin.messaging.Messaging from legacy Instance ID (IID) endpoints (iid.googleapis.com/iid/v1) to the modernized single-token Firebase Cloud Messaging (FCM) v1 Topics Subscription REST API (fcm.googleapis.com/v1).

  • Update subscribeToTopic (CreateTopicSubscription) and unsubscribeFromTopic (DeleteTopicSubscription) to use HTTP/2 transport by default (or HTTP/1.1 via enableLegacyHttpTransport()).
  • Implement client-side concurrency pooling (runWithConcurrencyLimit) bounded to 100 concurrent requests per batch to avoid HTTP/2 stream exhaustion.
  • Preserve idempotency by treating ALREADY_EXISTS (HTTP 409) as success for subscription requests, and NOT_FOUND (HTTP 404) as REGISTRATION_TOKEN_NOT_REGISTERED for unsubscription requests.
  • Add legacy backup/escape-hatch methods subscribeToTopicLegacy and unsubscribeFromTopicLegacy annotated with @deprecated Use {@link Messaging.subscribeToTopic} instead. and @deprecated Use {@link Messaging.unsubscribeFromTopic} instead..

Migrate `subscribeToTopic` and `unsubscribeFromTopic` methods in `admin.messaging.Messaging` from legacy Instance ID (IID) endpoints (`iid.googleapis.com/iid/v1`) to the modernized single-token Firebase Cloud Messaging (FCM) v1 Topics Subscription REST API (`fcm.googleapis.com/v1`).

- Update `subscribeToTopic` (`CreateTopicSubscription`) and `unsubscribeFromTopic` (`DeleteTopicSubscription`) to use HTTP/2 transport by default (or HTTP/1.1 via `enableLegacyHttpTransport()`).
- Implement client-side concurrency pooling (`runWithConcurrencyLimit`) bounded to 100 concurrent requests per batch to avoid HTTP/2 stream exhaustion.
- Preserve idempotency by treating `ALREADY_EXISTS` (HTTP 409) as success for subscription requests, and `NOT_FOUND` (HTTP 404) as `REGISTRATION_TOKEN_NOT_REGISTERED` for unsubscription requests.
- Add legacy backup/escape-hatch methods `subscribeToTopicLegacy` and `unsubscribeFromTopicLegacy` annotated with `@deprecated Use {@link Messaging.subscribeToTopic} instead.` and `@deprecated Use {@link Messaging.unsubscribeFromTopic} instead.`.

TAG=agy
CONV=a2576aae-730b-4fb1-a151-9e6913f97be4
@wiz-9635d3485b

Copy link
Copy Markdown

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings 1 Low
Software Management Finding Software Management Findings -
Total 1 Low

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request migrates the topic subscription and unsubscription management in the Firebase Messaging service to the FCM v1 API, while preserving the old behavior under deprecated legacy methods. It introduces concurrent request handling with a limit for both HTTP/1.1 and HTTP/2 transports. The review feedback highlights a potential runtime TypeError when parsing server error responses if the error field is null, and identifies an opportunity to reduce code duplication in the task mapping logic for HTTP/1.1 and HTTP/2 requests.

Comment on lines +263 to +265
const errorMessage = (validator.isNonNullObject(json) && 'error' in json && validator.isNonEmptyString((json as any).error.message))
? (json as any).error.message
: undefined;

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.

high

If the server returns a JSON response where the error field is explicitly null (e.g., { "error": null }), evaluating (json as any).error.message will throw a TypeError: Cannot read properties of null (reading 'message') and crash the process. Adding a check to ensure json.error is a non-null object before accessing its properties prevents this potential runtime exception.

Suggested change
const errorMessage = (validator.isNonNullObject(json) && 'error' in json && validator.isNonEmptyString((json as any).error.message))
? (json as any).error.message
: undefined;
const errorMessage = (validator.isNonNullObject(json) && 'error' in json && validator.isNonNullObject((json as any).error) && validator.isNonEmptyString((json as any).error.message))
? (json as any).error.message
: undefined;

Comment on lines +580 to +604
return this.getProjectId().then((projectId) => {
if (http2SessionHandler) {
const tasks = registrationTokensArray.map((token) => async () => {
const path = methodName === 'subscribeToTopic'
? `/v1/projects/${projectId}/registrations/${encodeURIComponent(token)}/topicSubscriptions?topic_name=${encodeURIComponent(topicName)}`
: `/v1/projects/${projectId}/registrations/${encodeURIComponent(token)}/topicSubscriptions/${encodeURIComponent(topicName)}?allow_missing=true`;
const requestData = methodName === 'subscribeToTopic' ? {} : undefined;
return this.messagingRequestHandler.invokeHttp2RequestHandlerForTopicSubscriptionResponse(
FCM_SEND_HOST, path, methodName, requestData, http2SessionHandler,
);
});
return runWithConcurrencyLimit(tasks, 100);
} else {
const tasks = registrationTokensArray.map((token) => async () => {
const path = methodName === 'subscribeToTopic'
? `/v1/projects/${projectId}/registrations/${encodeURIComponent(token)}/topicSubscriptions?topic_name=${encodeURIComponent(topicName)}`
: `/v1/projects/${projectId}/registrations/${encodeURIComponent(token)}/topicSubscriptions/${encodeURIComponent(topicName)}?allow_missing=true`;
const requestData = methodName === 'subscribeToTopic' ? {} : undefined;
return this.messagingRequestHandler.invokeHttpRequestHandlerForTopicSubscriptionResponse(
FCM_SEND_HOST, path, methodName, requestData,
);
});
return runWithConcurrencyLimit(tasks, 100);
}
})

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.

medium

The path construction and task mapping logic is identical between the HTTP/2 and HTTP/1.1 code paths. We can refactor this to construct the tasks array once and conditionally invoke the appropriate request handler inside the task function. This eliminates significant code duplication and improves maintainability.

        return this.getProjectId().then((projectId) => {
          const tasks = registrationTokensArray.map((token) => async () => {
            const path = methodName === 'subscribeToTopic'
              ? '/v1/projects/' + projectId + '/registrations/' + encodeURIComponent(token) + '/topicSubscriptions?topic_name=' + encodeURIComponent(topicName)
              : '/v1/projects/' + projectId + '/registrations/' + encodeURIComponent(token) + '/topicSubscriptions/' + encodeURIComponent(topicName) + '?allow_missing=true';
            const requestData = methodName === 'subscribeToTopic' ? {} : undefined;

            if (http2SessionHandler) {
              return this.messagingRequestHandler.invokeHttp2RequestHandlerForTopicSubscriptionResponse(
                FCM_SEND_HOST, path, methodName, requestData, http2SessionHandler,
              );
            } else {
              return this.messagingRequestHandler.invokeHttpRequestHandlerForTopicSubscriptionResponse(
                FCM_SEND_HOST, path, methodName, requestData,
              );
            }
          });
          return runWithConcurrencyLimit(tasks, 100);
        })

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.

1 participant