Repository navigation
feat(fcm): Migrate topic management to FCM v1 API - #980
Merged
Merged
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 the topic management functionality in the Firebase Cloud Messaging module from the legacy Instance ID (IID) API to the FCM v1 API, introducing new synchronous and asynchronous methods (subscribe_to_topic, subscribe_to_topic_async, unsubscribe_from_topic, and unsubscribe_from_topic_async) while deprecating the legacy versions. The feedback highlights a potential concurrency race condition in the multi-threaded request execution where a shared headers dictionary is mutated, and suggests extracting duplicated error-handling logic into a single helper method to improve maintainability.
lahirumaramba
force-pushed
the
lm-fcm-topics
branch
from
September 15, 2026 14:40
e065d10 to
be27016
Compare
lahirumaramba
force-pushed
the
lm-fcm-topics
branch
from
September 17, 2026 17:22
b4e98ba to
3c654a0
Compare
Member
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the topic subscription and unsubscription features to the FCM v1 API, introducing both synchronous and asynchronous implementations (subscribe_to_topic, subscribe_to_topic_async, unsubscribe_from_topic, and unsubscribe_from_topic_async). The previous Instance ID API implementations have been renamed to subscribe_to_topic_legacy and unsubscribe_from_topic_legacy and marked as deprecated. Comprehensive unit tests have been added to cover the new functionality, validation rules, and error handling. There are no review comments to address, and I have no additional feedback to provide.
OrlandriaH-G
approved these changes
Sep 21, 2026
jonathanedey
approved these changes
Sep 21, 2026
jonathanedey
left a comment
jonathanedey
left a comment
Contributor
There was a problem hiding this comment.
LGTM with one comment on error parsing which isn't blocking.
|
|
||
| return 'UNKNOWN_ERROR' | ||
|
|
||
| def _build_topic_subscription_result(self, response, is_subscribe): |
Contributor
There was a problem hiding this comment.
Not blocking but is there a reason we don't reuse the _build_fcm_error and _build_fcm_error_httpx methods to parse the responses here. I think I follow that there needs to be a pre-check based on is_subscribe. Is there anything else?
Member
Author
There was a problem hiding this comment.
I think it was because we had to map the error codes to keep backward compatibility with the legacy subscription methods. I will add a task to clean this up in the next major version. Thanks!
…ic error parsing - Pass a copy of self._fcm_headers in topic management requests to prevent concurrent mutation race conditions in ThreadPoolExecutor. - Extract common error parsing logic from _build_topic_subscription_result_from_requests_error and _build_topic_subscription_result_from_httpx_error into _build_topic_subscription_result.
- Hoist URL-encoded topic computation out of the per-token request loops. - Mount an HTTPAdapter with a connection pool size of 100 on the FCM client session. - Fix line length and method signature override lint warnings in test_messaging.py.
- Added type annotations to public topic management functions and internal methods - Updated HTTP status code mapping (401->UNAUTHENTICATED, 503->UNAVAILABLE, 408/504->DEADLINE_EXCEEDED) and prioritized status codes over free-form message strings - Updated topic regex with \Z to reject trailing newlines - Passed header copies in send, send_each, and send_each_async to prevent concurrency mutations - Added test coverage for status code mapping, async argument validation, prefixed topics, and async batch unsubscribe
lahirumaramba
force-pushed
the
lm-fcm-topics
branch
from
September 23, 2026 15:29
3c654a0 to
86ddab0
Compare
Merged
akalex
added a commit
to healthjoy/async-firebase
that referenced
this pull request
Oct 4, 2026
* feat(messaging): Migrate topic management to FCM v1 API subscribe_devices_to_topic() and unsubscribe_devices_from_topic() now call the FCM v1 topic subscriptions endpoint instead of the Instance ID API, following firebase/firebase-admin-python#980. One request is sent per device token, at most 100 in flight (or max_connections if lower). Request-level failures (401, timeouts, connection errors) are now reported per token in TopicManagementResponse.errors instead of TopicManagementResponse.exception. Subscribing an already subscribed token (ALREADY_EXISTS / CONFLICT) counts as success; 409 ABORTED does not. Arguments are validated: up to 1000 non-empty device tokens and a well-formed topic name, optionally prefixed with /topics/. The previous IID behavior is kept as the deprecated subscribe_devices_to_topic_legacy() and unsubscribe_devices_from_topic_legacy(). Bump version to 6.3.0. * fix(messaging): Address review findings on FCM v1 topic management - Prepare headers once per call instead of once per device token. A failed access token fetch is now reported for every token without sending requests, instead of being retried serially by each task. - Share the concurrency limit across concurrent calls on a client. - Escape the device token in the request path, so a "/" in a token can no longer change the path. - Tolerate non-dict JSON error bodies and malformed "details" when resolving error reasons, instead of raising AttributeError and aborting the whole batch. * fix: Raise on OAuth token errors and deduplicate topic device tokens - CredentialManager.get_access_token() now calls raise_for_status(), so an error response from the token endpoint (e.g. a revoked key) is handled as an HTTP error instead of escaping as KeyError('expires_in'). send() and send_each() return an FCMResponse with exception set, and topic management reports the error for every device token. - Topic management sends one request per unique device token and maps each outcome back to every index where the token appears, avoiding concurrent requests for the same subscription. * fix(messaging): Use a plain string error field as the topic error reason Bodies like {"error": "invalid_grant"} from the OAuth token endpoint, or {"error": "INVALID_REGISTRATION"} from legacy-style APIs, now report the string as the per-token reason instead of the generic HTTP status code. The send() error path is unchanged. * refactor(messaging): Make FCM v1 topic management backward compatible Restore subscribe_devices_to_topic() and unsubscribe_devices_from_topic() to the Instance ID implementation, unchanged apart from a DeprecationWarning, and expose the FCM v1 implementation as the new subscribe_to_topic() and unsubscribe_from_topic(). Remove the *_legacy methods. Revert the changes to shared code paths so existing behavior matches 6.2.2: CredentialManager no longer calls raise_for_status(), and _parse_platform_error(), _get_fcm_error_type() and the IID response parser are back to their original code. The v1 error handler now has its own lenient body parsing. * refactor(messaging): Keep topic management helpers out of the public API - Make TOPIC_PREFIX, TOPIC_NAME_PATTERN and TopicManagementResponse.from_error_reasons() private, so 6.3.0 adds only the public names callers need. - Reword the subscribe_to_topic()/unsubscribe_from_topic() docstrings and README so they don't imply a rejected access token is reported per token. - Move the v1 error helpers above the public API section in responses, fix the public function count, and note why the FCM error code lookup is not shared with _get_fcm_error_type(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * refactor(messaging): Make the topic concurrency cap private and pin legacy behavior - Rename TOPIC_MANAGEMENT_MAX_CONCURRENCY to _TOPIC_MANAGEMENT_MAX_CONCURRENCY; it is an internal tuning value. - List the remaining public additions (TOPIC_MANAGEMENT_MAX_DEVICE_TOKENS and AsyncClientBase.FCM_REGISTRATIONS_ENDPOINT) in CHANGES. - Add a test that the deprecated IID methods still send input the new v1 methods reject, guarding backward compatibility. * fix(messaging): Harden FCM v1 topic management per review - Decide "already subscribed" from the HTTP status (409) or a status of ALREADY_EXISTS/CONFLICT, before resolving the reason, matching the official Firebase Admin SDKs (Python, Node, Go, Java, .NET). An FCM errorCode in the body can no longer override it, and 409 ABORTED now counts as already subscribed, as in those SDKs. - Use only non-empty string error codes, so a malformed error body (e.g. a list status) falls back to the HTTP status instead of raising TypeError and aborting the call. - Create the concurrency semaphore per call. A semaphore cached on the client bound to the first event loop and raised RuntimeError when the client was reused from another loop. - Stop logging the request URL, which contains the device token. - Return the HTTP error from _send_topic_subscription_request and let each per-token step apply its own rule. - Clarify in CHANGES that the deprecated methods now emit a warning. * refactor(messaging): Address Codacy findings in topic management - Build TopicManagementResponse from per-token outcomes in a private client function that sets only public fields, instead of calling the private TopicManagementResponse._from_error_reasons() from another module. messages.py is back to its 6.2.2 code. - Use the fake_device_token fixture in the logging test and check both the raw and URL-encoded token, replacing the hardcoded token literal that Bandit reported as B105. * test: Add FCM v1 topic management integration tests Exercise subscribe_to_topic()/unsubscribe_from_topic() against the real FCM project with a made-up registration token. FCM may accept or reject it, so the tests assert that each request is routed, authorized and answered with a Google API error or success, and that the public methods report exactly one outcome per token. This catches a wrong endpoint, HTTP method or missing permission, but cannot cover the success or "already subscribed" paths for a real device. Move the shared credential check into tests/integration.py and the fcm_client fixture into conftest.py so both integration modules use them. * ci: Rename the integration test step The step runs every test marked integration, which now includes the topic management tests, not only FCM payload validation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrates
subscribe_to_topicandunsubscribe_from_topicin the messaging module from the legacy Instance ID (IID) API to the FCM v1 Topic Subscriptions API.Key changes:
subscribe_to_topicandunsubscribe_from_topicto call the FCM v1 endpoints (/v1/projects/{projectId}/registrations/{token}/topicSubscriptions).subscribe_to_topic_asyncandunsubscribe_from_topic_asyncutilizingHttpxAsyncClient(HTTP/2).subscribe_to_topic_legacyandunsubscribe_from_topic_legacywith deprecation warnings.ALREADY_EXISTS(HTTP 409) is treated as success for topic subscriptions, whileNOT_FOUND(HTTP 404) is recorded as a failure for topic unsubscriptions.