Skip to content

fix: preserve crashlytics options#279

Open
IzaakGough wants to merge 6 commits into
mainfrom
@invertase/fix-preserve-crashlytics-options
Open

fix: preserve crashlytics options#279
IzaakGough wants to merge 6 commits into
mainfrom
@invertase/fix-preserve-crashlytics-options

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #265

Summary

Alert-based function decorators were reconstructing FirebaseAlertOptions with only alert_type and app_id, which dropped shared handler settings like region, max_instances, retry, and secrets. This change preserves the full option set when building alert endpoints and adds coverage across the alert wrappers that delegate to FirebaseAlertOptions.

Problem/Root Cause

Issue #265 reports that Crashlytics alert handlers ignored configuration such as region, max instances, and secrets. The same pattern also applied to the other alert-specific option wrappers. In each _endpoint() implementation, the wrapper created a fresh FirebaseAlertOptions with just the alert identifiers, so the inherited EventHandlerOptions fields on the original options object never made it into the generated endpoint manifest.

Solution/Changes

Add a shared helper that copies the dataclass fields from the original alert options object into a new FirebaseAlertOptions, while overriding only alert_type and app_id where needed. Update the App Distribution, Crashlytics, Performance, and Billing option wrappers to use that helper before generating the endpoint. Expand tests/test_options.py with endpoint assertions that verify shared options are preserved for each alert wrapper and for direct alerts_fn.on_alert_published(...) usage.

Testing

  • uv run pytest tests/test_options.py

@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 introduces a helper function _alert_options_to_firebase_alert_options to streamline the conversion of event handler options to Firebase alert options, refactoring several _endpoint methods to use it, and adds comprehensive unit tests to verify option preservation. The reviewer suggested simplifying the helper function's signature by dynamically extracting app_id from the options object using getattr, which would eliminate the need to explicitly pass self.app_id in multiple _endpoint implementations.

Comment thread src/firebase_functions/options.py
Comment thread src/firebase_functions/options.py
Comment thread src/firebase_functions/options.py
Comment thread src/firebase_functions/options.py
@IzaakGough IzaakGough marked this pull request as ready for review June 3, 2026 11:34
@cabljac cabljac self-requested a review June 8, 2026 09:35

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

lgtm - i think Gemini comments are mostly cleanup nits, probably worth doing

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.

Crashlytics options are ignored

2 participants