feat(fcm): Migrate topic management to FCM v1 API - #1240
lahirumaramba wants to merge 3 commits into
Conversation
fdfe8ea to
51005bd
Compare
There was a problem hiding this comment.
Code Review
This pull request migrates topic subscription and unsubscription operations in FirebaseMessaging from the legacy Instance ID API to the FCM v1 API, while retaining the legacy methods as deprecated. The new implementation in FirebaseMessagingClientImpl handles these operations concurrently using CompletableFuture. However, the current design creates and shuts down a new thread pool for every topic management request, which is highly inefficient and can cause thread exhaustion under high load. It is recommended to initialize a single shared ExecutorService in the constructor with core thread timeout enabled, and reuse it across requests to improve performance and simplify the request handling logic.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates topic subscription and unsubscription operations to the FCM v1 API, introducing new asynchronous methods in FirebaseMessagingClientImpl using CompletableFuture and deprecating the legacy Instance ID API methods. The feedback recommends using daemon threads in the default executor to prevent JVM shutdown delays in short-lived applications, and wrapping task submissions in a try-catch block to gracefully handle RejectedExecutionException when custom bounded executors are used.
d9277eb to
5511d97
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates topic management operations (subscribe/unsubscribe) from the legacy Instance ID API to the FCM v1 API. It introduces a configurable FCM host, adds a dedicated executor service for concurrent topic management requests, and updates the internal client implementation and corresponding tests to support these changes. The review identified a potential issue where a trailing slash in a custom FCM host configuration could lead to malformed URLs, and a code suggestion was provided to normalize the host string.
| this.projectId = builder.projectId; | ||
| this.fcmHost = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost; | ||
| this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId); |
There was a problem hiding this comment.
If builder.fcmHost is configured with a trailing slash (e.g., https://custom.host.com/), it can result in URLs with double slashes (e.g., https://custom.host.com//v1/projects/...). This can cause unexpected 404 errors or authentication failures with some strict API gateways or proxies. Consider normalizing fcmHost by stripping any trailing slash.
| this.projectId = builder.projectId; | |
| this.fcmHost = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost; | |
| this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId); | |
| this.projectId = builder.projectId; | |
| String host = Strings.isNullOrEmpty(builder.fcmHost) ? DEFAULT_FCM_HOST : builder.fcmHost; | |
| if (host.endsWith("/")) { | |
| host = host.substring(0, host.length() - 1); | |
| } | |
| this.fcmHost = host; | |
| this.fcmSendUrl = String.format(FCM_URL, this.fcmHost, builder.projectId); |
Migrates topic subscription and management from the legacy Instance ID (IID) service (
iid.googleapis.com) to the FCM v1 REST API (fcm.googleapis.com/v1/projects/{projectId}/registrations/{token}/topicSubscriptions).