fix: preserve crashlytics options#279
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 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.
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
lgtm - i think Gemini comments are mostly cleanup nits, probably worth doing
| **option_values, | ||
| alert_type=alert_type, | ||
| app_id=app_id, | ||
| ) |
There was a problem hiding this comment.
I think there can be a possibility of a future regression here. If a developer adds a custom options field to a specific subclass (e.g., AppDistributionOptions) that is not present on FirebaseAlertOptions, the current implementation will include it in option_values and attempt to pass it to the FirebaseAlertOptions constructor. This will raise a runtime error. Maybe we can try to filter option_values to only include fields defined in firebaseAlertOptions?
Ai suggested something like this:
def _alert_options_to_firebase_alert_options(
options: EventHandlerOptions,
alert_type: str | AlertType,
) -> FirebaseAlertOptions:
app_id = getattr(options, "app_id", None)
# Restrict to fields supported by FirebaseAlertOptions
allowed_fields = {f.name for f in _dataclasses.fields(FirebaseAlertOptions)}
option_values = {
field.name: getattr(options, field.name)
for field in _dataclasses.fields(options)
if field.name in allowed_fields and field.name not in {"alert_type", "app_id"}
}
return FirebaseAlertOptions(
**option_values,
alert_type=alert_type,
app_id=app_id,
)There was a problem hiding this comment.
Good catch! I think the suggested snippet here looks good. Will add this.
|
Thanks for making the changes Izaak. Approved |
3 participants
Fixes #265
Summary
Alert-based function decorators were reconstructing
FirebaseAlertOptionswith onlyalert_typeandapp_id, which dropped shared handler settings likeregion,max_instances,retry, andsecrets. This change preserves the full option set when building alert endpoints and adds coverage across the alert wrappers that delegate toFirebaseAlertOptions.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 freshFirebaseAlertOptionswith just the alert identifiers, so the inheritedEventHandlerOptionsfields 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 onlyalert_typeandapp_idwhere needed. Update the App Distribution, Crashlytics, Performance, and Billing option wrappers to use that helper before generating the endpoint. Expandtests/test_options.pywith endpoint assertions that verify shared options are preserved for each alert wrapper and for directalerts_fn.on_alert_published(...)usage.Testing
uv run pytest tests/test_options.py