Skip to content

fix(fcm): reject empty message batches before dispatch - #986

Open
Shubham-Padkonde wants to merge 2 commits into
firebase:mainfrom
Shubham-Padkonde:codex-fcm-empty-batch
Open

Shubham-Padkonde wants to merge 2 commits into
firebase:mainfrom
Shubham-Padkonde:codex-fcm-empty-batch

Conversation

@Shubham-Padkonde

Copy link
Copy Markdown

Fixes #792.

An empty message list currently reaches a zero-worker thread pool and raises UnknownError in the synchronous sender, while the asynchronous sender returns an empty response. Reject empty batches with a clear ValueError before either dispatch path starts. This also covers multicast messages with no recipients; non-empty token and FID batches retain their existing behavior.

Added regression tests for direct and multicast sending in both sync and async APIs, and documented the non-empty requirement. All six collected regression cases fail on the original implementation.

Validation on Windows / Python 3.13:

  • Full unit suite: 3,921 passed (200 deprecation warnings).
  • Messaging module: 954 passed.
  • Project-configured pylint on both changed files: 10.00/10, no findings.
  • git diff --check passed.
  • Live Firebase integration tests were not run (no configured project).

Prepared with Codex assistance.

Context Sources Used:

  • id: firebase-admin-python; repository path: AGENTS.md

Validate empty lists in both send paths and cover direct and multicast callers with regression tests.
@Shubham-Padkonde
Shubham-Padkonde requested a review from a team September 22, 2026 08:06

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

Copy link
Copy Markdown

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 introduces validation to ensure that message batches passed to send_each and send_each_async are not empty, raising a ValueError if they are, along with corresponding docstring updates and unit tests. The feedback points out that raising a ValueError('messages must not be empty.') when called via multicast methods results in a leaky abstraction, as the error message refers to an internal parameter (messages) rather than the user-provided multicast_message. It is recommended to validate that the MulticastMessage contains at least one token or fid inside _get_messages_from_multicast instead to provide a cleaner API experience.

Comment on lines +450 to +451
if not messages:
raise ValueError('messages must not be empty.')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Raising ValueError('messages must not be empty.') here results in a leaky abstraction when called via send_each_for_multicast or send_each_for_multicast_async. If a user passes a MulticastMessage with empty tokens/fids, they will receive an error message referring to messages, which is an internal parameter they did not directly provide.\n\nTo provide a cleaner API experience, consider validating that the MulticastMessage contains at least one token or fid inside _get_messages_from_multicast instead, and raising a more appropriate error message.\n\nFor example, in _get_messages_from_multicast:\npython\ndef _get_messages_from_multicast(multicast_message: MulticastMessage) -> List[Message]:\n # ... existing extraction logic ...\n if not messages:\n raise ValueError('multicast_message must contain at least one token or fid.')\n return messages\n

@Shubham-Padkonde

Copy link
Copy Markdown
Author

Addressed the multicast error-message feedback in ebf718a. Empty multicast calls now identify multicast_message and require at least one token or fid. Tests cover empty tokens, empty fids, and both, for synchronous and asynchronous methods. All 958 messaging tests pass; both modified modules pass pylint (10/10). Prepared with Codex assistance.

This branch has not been deployed

No deployments
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.

Better Error handling for empty token list in Messaging send_each_multicast

1 participant