Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions src/firebase_functions/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,23 @@ def _endpoint(
)


def _alert_options_to_firebase_alert_options(
options: EventHandlerOptions,
alert_type: str | AlertType,
app_id: str | None = None,
) -> FirebaseAlertOptions:
option_values = {
field.name: getattr(options, field.name)
for field in _dataclasses.fields(options)
if field.name not in {"alert_type", "app_id"}
}
return FirebaseAlertOptions(
**option_values,
alert_type=alert_type,
app_id=app_id,
)
Comment on lines +655 to +669
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.

medium

The helper function _alert_options_to_firebase_alert_options can automatically extract app_id from the options object if it exists (using getattr(options, "app_id", None)). This simplifies the function signature and removes the need for callers to manually pass self.app_id in every _endpoint implementation.

Suggested change
def _alert_options_to_firebase_alert_options(
options: EventHandlerOptions,
alert_type: str | AlertType,
app_id: str | None = None,
) -> FirebaseAlertOptions:
option_values = {
field.name: getattr(options, field.name)
for field in _dataclasses.fields(options)
if field.name not in {"alert_type", "app_id"}
}
return FirebaseAlertOptions(
**option_values,
alert_type=alert_type,
app_id=app_id,
)
def _alert_options_to_firebase_alert_options(
options: EventHandlerOptions,
alert_type: str | AlertType,
) -> FirebaseAlertOptions:
app_id = getattr(options, "app_id", None)
option_values = {
field.name: getattr(options, field.name)
for field in _dataclasses.fields(options)
if field.name not in {"alert_type", "app_id"}
}
return FirebaseAlertOptions(
**option_values,
alert_type=alert_type,
app_id=app_id,
)



@_dataclasses.dataclass(frozen=True, kw_only=True)
class AppDistributionOptions(EventHandlerOptions):
"""
Expand All @@ -669,9 +686,10 @@ def _endpoint(
**kwargs,
) -> _manifest.ManifestEndpoint:
assert kwargs["alert_type"] is not None
return FirebaseAlertOptions(
alert_type=kwargs["alert_type"],
app_id=self.app_id,
return _alert_options_to_firebase_alert_options(
self,
kwargs["alert_type"],
self.app_id,
)._endpoint(**kwargs)
Comment on lines +689 to 693
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.

medium

Since _alert_options_to_firebase_alert_options can automatically extract app_id from self, passing self.app_id explicitly is redundant and can be removed.

        return _alert_options_to_firebase_alert_options(
            self,
            kwargs["alert_type"],
        )._endpoint(**kwargs)



Expand All @@ -692,9 +710,10 @@ def _endpoint(
**kwargs,
) -> _manifest.ManifestEndpoint:
assert kwargs["alert_type"] is not None
return FirebaseAlertOptions(
alert_type=kwargs["alert_type"],
app_id=self.app_id,
return _alert_options_to_firebase_alert_options(
self,
kwargs["alert_type"],
self.app_id,
)._endpoint(**kwargs)
Comment on lines +713 to 717
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.

medium

Since _alert_options_to_firebase_alert_options can automatically extract app_id from self, passing self.app_id explicitly is redundant and can be removed.

        return _alert_options_to_firebase_alert_options(
            self,
            kwargs["alert_type"],
        )._endpoint(**kwargs)



Expand All @@ -715,9 +734,10 @@ def _endpoint(
**kwargs,
) -> _manifest.ManifestEndpoint:
assert kwargs["alert_type"] is not None
return FirebaseAlertOptions(
alert_type=kwargs["alert_type"],
app_id=self.app_id,
return _alert_options_to_firebase_alert_options(
self,
kwargs["alert_type"],
self.app_id,
)._endpoint(**kwargs)
Comment on lines +737 to 741
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.

medium

Since _alert_options_to_firebase_alert_options can automatically extract app_id from self, passing self.app_id explicitly is redundant and can be removed.

        return _alert_options_to_firebase_alert_options(
            self,
            kwargs["alert_type"],
        )._endpoint(**kwargs)



Expand All @@ -733,8 +753,9 @@ def _endpoint(
**kwargs,
) -> _manifest.ManifestEndpoint:
assert kwargs["alert_type"] is not None
return FirebaseAlertOptions(
alert_type=kwargs["alert_type"],
return _alert_options_to_firebase_alert_options(
self,
kwargs["alert_type"],
)._endpoint(**kwargs)


Expand Down
111 changes: 110 additions & 1 deletion tests/test_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,19 @@

from pytest import raises

from firebase_functions import https_fn, options, params
from firebase_functions import alerts_fn, https_fn, options, params
from firebase_functions.alerts import (
app_distribution_fn,
billing_fn,
crashlytics_fn,
performance_fn,
)
from firebase_functions.private.serving import functions_as_yaml, merge_required_apis

# pylint: disable=protected-access

ALERT_SECRET = params.SecretParam("GITLAB_PERSONAL_ACCESS_TOKEN")


@https_fn.on_call()
def asamplefunction(_):
Expand Down Expand Up @@ -196,3 +204,104 @@ def test_invoker_with_no_element_throws():
AssertionError, match="HttpsOptions: Invalid option for invoker - must be a non-empty list."
):
options.HttpsOptions(invoker=[])._endpoint(func_name="test")


def _assert_alert_endpoint_options(endpoint, expected_alert_type, expect_app_id: str | None = None):
assert endpoint.region == ["europe-west1"]
assert endpoint.maxInstances == 1
assert endpoint.secretEnvironmentVariables == [{"key": "GITLAB_PERSONAL_ACCESS_TOKEN"}]
assert endpoint.eventTrigger["retry"] is True
assert endpoint.eventTrigger["eventFilters"]["alerttype"] == expected_alert_type
if expect_app_id is None:
assert "appid" not in endpoint.eventTrigger["eventFilters"]
else:
assert endpoint.eventTrigger["eventFilters"]["appid"] == expect_app_id


def test_crashlytics_options_preserved_in_alert_endpoint():
@crashlytics_fn.on_new_fatal_issue_published(
secrets=[ALERT_SECRET],
region="europe-west1",
max_instances=1,
retry=True,
app_id="app-123",
)
def sample(_event):
return None

_assert_alert_endpoint_options(
sample.__firebase_endpoint__,
"crashlytics.newFatalIssue",
expect_app_id="app-123",
)


def test_app_distribution_options_preserved_in_alert_endpoint():
@app_distribution_fn.on_new_tester_ios_device_published(
secrets=[ALERT_SECRET],
region="europe-west1",
max_instances=1,
retry=True,
app_id="app-123",
)
def sample(_event):
return None

_assert_alert_endpoint_options(
sample.__firebase_endpoint__,
"appDistribution.newTesterIosDevice",
expect_app_id="app-123",
)


def test_performance_options_preserved_in_alert_endpoint():
@performance_fn.on_threshold_alert_published(
secrets=[ALERT_SECRET],
region="europe-west1",
max_instances=1,
retry=True,
app_id="app-123",
)
def sample(_event):
return None

_assert_alert_endpoint_options(
sample.__firebase_endpoint__,
"performance.threshold",
expect_app_id="app-123",
)


def test_billing_options_preserved_in_alert_endpoint():
@billing_fn.on_plan_update_published(
secrets=[ALERT_SECRET],
region="europe-west1",
max_instances=1,
retry=True,
)
def sample(_event):
return None

_assert_alert_endpoint_options(
sample.__firebase_endpoint__,
"billing.planUpdate",
)


def test_firebase_alert_options_preserved_in_alert_endpoint():
@alerts_fn.on_alert_published(
alert_type=alerts_fn.AlertType.CRASHLYTICS_NEW_FATAL_ISSUE,
secrets=[ALERT_SECRET],
region="europe-west1",
max_instances=1,
retry=True,
app_id="app-123",
)
def sample(_event):
return None

_assert_alert_endpoint_options(
sample.__firebase_endpoint__,
"crashlytics.newFatalIssue",
expect_app_id="app-123",
)
Loading