Skip to content

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

Open
IzaakGough wants to merge 4 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 4 commits into
mainfrom
@invertase/feat-add-retry-for-storage-firestore-database

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jun 22, 2026

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

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
IzaakGough marked this pull request as ready for review June 22, 2026 14:52
@cabljac

cabljac commented Jul 16, 2026

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.

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