Repository navigation
feat(messaging): migrate topic management to FCM v1 REST API - #327
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates FCM topic management (subscribeToTopic and unsubscribeFromTopic) to the FCM v1 REST API, deprecating the legacy IID methods. It introduces concurrent worker processing for token subscriptions and adds a 1000-token limit validation. Feedback on the changes highlights a potential double-encoding issue when constructing request URIs with pre-encoded path components, recommending the use of Uri with pathSegments instead. Additionally, the topic validation regex incorrectly allows a private/ segment, which contradicts the error message and is unsupported by the FCM v1 REST API.
Coverage Report✅ Coverage 74.34% meets 40% threshold Total Coverage: 74.34% Package Breakdown
Minimum threshold: 40% |
74666b5 to
92ddff0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates FCM topic management (subscribeToTopic and unsubscribeFromTopic) from the legacy IID API to the FCM v1 REST API, while deprecating the legacy methods as subscribeToTopicLegacy and unsubscribeFromTopicLegacy. The migration introduces concurrent requests using a worker pool, adds validation for a maximum of 1000 registration tokens, and updates topic format validation. Comprehensive unit tests have been added to verify the new REST API integrations, error handling, and legacy methods. There are no review comments to address, and no additional feedback is provided.
There was a problem hiding this comment.
Code Review
This pull request migrates FCM topic management (subscribeToTopic and unsubscribeFromTopic) from the legacy IID API to the FCM v1 REST API, while deprecating the legacy methods as subscribeToTopicLegacy and unsubscribeFromTopicLegacy. The new implementation processes registration tokens concurrently using a worker pool to interact with the FCM v1 REST API, adds validation to limit registration tokens to 1000, and updates error handling and unit tests accordingly. There are no review comments provided, and I have no additional feedback on these changes.
demolaf
left a comment
demolaf
left a comment
There was a problem hiding this comment.
Two notes. The CHANGELOG one blocks; the allow_missing one is a question rather than a change request.
| 'topicSubscriptions', | ||
| cleanTopic, | ||
| ], | ||
| queryParameters: {'allow_missing': 'true'}, |
There was a problem hiding this comment.
Does allow_missing=true also suppress NOT_FOUND when the token itself is unregistered? Legacy batchRemove reported those as per-token failures, so suppressing them would drop a signal callers may rely on.
There was a problem hiding this comment.
Hmm good question. It should not suppress NOT_FOUND when the token itself is unregistered.
Per Google API AIP-135, allow_missing=true only applies to the leaf resource being deleted (topicSubscriptions/{cleanTopic}). When the registration token is not registered, the parent resource (registrations/{token}) does not exist, and the FCM v1 endpoint still returns HTTP 404 NOT_FOUND.
The SDK maps that 404 to MessagingClientErrorCode.registrationTokenNotRegistered, preserving the per-token failure signal just like legacy batchRemove. What allow_missing=true does is make unsubscription idempotent when the token is valid/registered but was never subscribed to that topic (avoiding a false failure for an already-unsubscribed token).
demolaf
left a comment
demolaf
left a comment
There was a problem hiding this comment.
@lahirumaramba can you apply the fix to CHANGELOG here too?
- Migrate subscribeToTopic and unsubscribeFromTopic to FCM v1 REST API. - Concurrently process registration token requests with a worker pool (up to 100 concurrent requests). - Add deprecated subscribeToTopicLegacy and unsubscribeFromTopicLegacy methods for backward compatibility with the IID API. - Validate registration tokens list length (up to 1000 tokens) and topic format. - Handle 409 Conflict / ALREADY_EXISTS responses as success during subscription. - Update unit tests covering FCM v1 topic management endpoints, error handling, and legacy methods. - Update CHANGELOG.md.
- Use Uri constructor with pathSegments instead of Uri.https with pre-encoded components to prevent double-encoding. - Remove (private/)? from topic validation regex to match FCM v1 API and error format. - Add invalid topic test cases for paths containing slashes.
400626e to
58df2d7
Compare
3 participants
Description