FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: add retry support for firestore, storage and realtime database by IzaakGough · Pull Request #298 · firebase/firebase-functions-python · GitHub

feat: add retry support for firestore, storage and realtime database - #298

Open
IzaakGough wants to merge 9 commits into
mainfrom
@invertase/feat-add-retry-for-storage-firestore-database
Open

feat: add retry support for firestore, storage and realtime database#298
IzaakGough wants to merge 9 commits into
mainfrom
@invertase/feat-add-retry-for-storage-firestore-database

Conversation

IzaakGough commented Jun 22, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #165

This PR maintains parity between the Python and JavaScript SDKs by adding retry support for Storage, Firestore, and Realtime Database.

gemini-code-assist Bot left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code Review

This pull request updates StorageOptions, DatabaseOptions, and FirestoreOptions to inherit from EventHandlerOptions instead of RuntimeOptions, allowing the retry configuration to be dynamically set. Corresponding unit tests have been added to verify this behavior. Feedback suggests simplifying the _endpoint method in StorageOptions by leveraging super()._endpoint directly now that it inherits from EventHandlerOptions.

Comment thread src/firebase_functions/options.py Outdated
IzaakGough marked this pull request as ready for review June 22, 2026 14:52

cabljac commented Jul 16, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Nice work covering all three providers, and the tests for both retry=True and the default path are solid.

One structural ask: DatabaseOptions._endpoint and FirestoreOptions._endpoint skip EventHandlerOptions._endpoint (they call RuntimeOptions._endpoint(self, ...) directly, I assume because the parent does not handle eventFilterPathPatterns) and re-implement the retry=... line inline. That leaves three copies of the retry logic, and any future change to how EventHandlerOptions._endpoint builds the event trigger (new trigger fields, changed defaults) would silently not apply to Database and Firestore. You already moved Storage over to delegating in 23bd29e, please do the same for these two, e.g. by letting the parent accept optional path patterns:

# EventHandlerOptions._endpoint
event_trigger = _manifest.EventTrigger(
    eventType=kwargs["event_type"],
    retry=self.retry if self.retry is not None else False,
    eventFilters=kwargs["event_filters"],
    eventFilterPathPatterns=kwargs.get("event_filters_path_patterns"),
)

then both subclasses build their filters and call super()._endpoint(...). Worth checking the manifest asdict output stays identical for the no-path-patterns providers (a None there may need omitting rather than serialising). If there is a reason the bypass is deliberate that I am missing, happy to hear it.

IzaakGough requested a review from a team July 23, 2026 12:06
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing retry in storage and firestore trigger decorators

2 participants


Back | FazBrowse Home | New Git URL