Skip to content

WIP feat(fcm): Migrate topic subscription APIs to new REST endpoints#3136

Open
lahirumaramba wants to merge 1 commit into
mainfrom
lm-fcm-topic-migration
Open

WIP feat(fcm): Migrate topic subscription APIs to new REST endpoints#3136
lahirumaramba wants to merge 1 commit into
mainfrom
lm-fcm-topic-migration

Conversation

@lahirumaramba
Copy link
Copy Markdown
Member

@lahirumaramba lahirumaramba commented May 12, 2026

Migrate topic subscription APIs to new REST endpoints via HTTP/2 multiplexing

DO_NOT_MERGE

Copy link
Copy Markdown
Contributor

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

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 refactors topic management to use the FCM v1 HTTP/2 API, replacing the legacy batch IID endpoint with individual requests per registration token. Feedback focuses on refactoring the asynchronous logic to avoid the new Promise constructor anti-pattern, ensuring the HTTP/2 session is established before requests are sent, and using defined constants instead of hardcoded host strings for better maintainability.

Comment on lines +415 to +479
let settledPromise: Promise<PromiseSettledResult<any>[]>;
return new Promise<MessagingTopicManagementResponse>((resolve, reject) => {
http2SessionHandler.invoke().catch((error) => {
reject(new FirebaseMessagingSessionError(error, undefined, undefined));
});

const requests = registrationTokensArray.map(async (registrationId) => {
let requestPath = `/v1/projects/${projectId}/registrations/${registrationId}/topicSubscriptions`;
if (isSubscribe) {
requestPath += `?topic_name=${topicName}`;
} else {
requestPath += `/${topicName}?allow_missing=true`;
}
return this.messagingRequestHandler.invokeHttp2RequestHandler(
'fcm.googleapis.com',
requestPath,
httpMethod,
isSubscribe ? {} : undefined,
http2SessionHandler
);
});

settledPromise = Promise.allSettled(requests);
settledPromise.then((results) => {
if (results.length > 0 && results.every((r) => r.status === 'rejected')) {
const firstReason = (results[0] as PromiseRejectedResult).reason;
if (firstReason instanceof RequestResponseError) {
reject(createFirebaseError(firstReason));
} else {
reject(firstReason);
}
return;
}

const response: MessagingTopicManagementResponse = {
successCount: 0,
failureCount: 0,
errors: [],
};

results.forEach((result, index) => {
if (result.status === 'fulfilled') {
response.successCount += 1;
} else {
response.failureCount += 1;
const err = result.reason;
const errorCode = err.response?.isJson() ? getErrorCode(err.response.data) : null;
const errorMessage = err.response?.isJson() ? err.response.data?.error?.message : err.message;
const newError = FirebaseMessagingError.fromTopicManagementServerError(
errorCode || 'UNKNOWN',
errorMessage,
err.response?.isJson() ? err.response.data : undefined
);
response.errors.push({
index,
error: newError,
});
}
});

resolve(response);
});
}).finally(() => {
http2SessionHandler.close();
});
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

The current implementation uses the new Promise constructor anti-pattern to wrap existing promise-returning logic. Additionally, it initiates all HTTP requests immediately, even if the HTTP/2 session invocation fails, which is inefficient and can lead to unhandled rejections.

Refactoring this into a clean promise chain ensures that requests are only started after the session is successfully established and simplifies error handling.

          return http2SessionHandler.invoke()
            .catch((error) => {
              throw new FirebaseMessagingSessionError(error, undefined, undefined);
            })
            .then(() => {
              const requests = registrationTokensArray.map(async (registrationId) => {
                let requestPath = `/v1/projects/${projectId}/registrations/${registrationId}/topicSubscriptions`;
                if (isSubscribe) {
                  requestPath += `?topic_name=${topicName}`;
                } else {
                  requestPath += `/${topicName}?allow_missing=true`;
                }
                return this.messagingRequestHandler.invokeHttp2RequestHandler(
                  FCM_SEND_HOST,
                  requestPath,
                  httpMethod,
                  isSubscribe ? {} : undefined,
                  http2SessionHandler
                );
              });

              return Promise.allSettled(requests);
            })
            .then((results) => {
              if (results.length > 0 && results.every((r) => r.status === 'rejected')) {
                const firstReason = (results[0] as PromiseRejectedResult).reason;
                if (firstReason instanceof RequestResponseError) {
                  throw createFirebaseError(firstReason);
                } else {
                  throw firstReason;
                }
              }

              const response: MessagingTopicManagementResponse = {
                successCount: 0,
                failureCount: 0,
                errors: [],
              };

              results.forEach((result, index) => {
                if (result.status === 'fulfilled') {
                  response.successCount += 1;
                } else {
                  response.failureCount += 1;
                  const err = result.reason;
                  const errorCode = err.response?.isJson() ? getErrorCode(err.response.data) : null;
                  const errorMessage = err.response?.isJson() ? err.response.data?.error?.message : err.message;
                  const newError = FirebaseMessagingError.fromTopicManagementServerError(
                    errorCode || 'UNKNOWN',
                    errorMessage,
                    err.response?.isJson() ? err.response.data : undefined
                  );
                  response.errors.push({
                    index,
                    error: newError,
                  });
                }
              });

              return response;
            })
            .finally(() => {
              http2SessionHandler.close();
            });

const isSubscribe = methodName === 'subscribeToTopic';
const httpMethod = isSubscribe ? 'POST' : 'DELETE';

const http2SessionHandler = new Http2SessionHandler('https://fcm.googleapis.com');
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

Avoid hardcoding the host string. Use the FCM_SEND_HOST constant defined at the top of the file for consistency.

Suggested change
const http2SessionHandler = new Http2SessionHandler('https://fcm.googleapis.com');
const http2SessionHandler = new Http2SessionHandler(`https://${FCM_SEND_HOST}`);

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