Skip to content

Commit a5f7496

Browse files
committed
fix(nimbus): catch FmlError when FmlClient fails to parse a manifest
Because * The FmlClient constructor can raise FmlError.ValidationError when a feature manifest has validation errors (e.g. a variable with both gecko-pref and default, which are now mutually exclusive) * NimbusFmlLoader.fml_client() did not catch this, causing a 500 on any page that calls get_invalid_fields_errors() for affected experiments * This was discovered via failing Fenix integration tests on PR #15002 This commit * Wraps the FmlClient() constructor in a try/except FmlError so invalid manifests are logged and skipped rather than crashing the page * Adds unit tests for the graceful degradation in both fml_client() and get_fml_errors() Fixes #15018
1 parent e9f8ccd commit a5f7496

File tree

2 files changed

+45
-6
lines changed

2 files changed

+45
-6
lines changed

experimenter/experimenter/features/manifests/nimbus_fml_loader.py

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
from typing import Optional
55

66
from django.conf import settings
7-
from nimbus_megazord.fml import FmlClient
7+
from nimbus_megazord.fml import FmlClient, FmlError
88

99
from experimenter.experiments.constants import NimbusConstants
1010
from experimenter.experiments.models import NimbusFeatureVersion
@@ -55,10 +55,16 @@ def fml_client(self, version: Optional[NimbusFeatureVersion] = None) -> FmlClien
5555
"""
5656
file_path = self.file_path(version)
5757
if file_path is not None:
58-
return FmlClient(
59-
str(file_path),
60-
self.channel,
61-
)
58+
try:
59+
return FmlClient(
60+
str(file_path),
61+
self.channel,
62+
)
63+
except FmlError:
64+
logger.exception(
65+
f"Nimbus FML Loader: FmlClient failed to parse manifest: {file_path}"
66+
)
67+
return None
6268
else:
6369
logger.error("Nimbus FML Loader: Failed to get FmlClient.")
6470
return None

experimenter/experimenter/features/tests/test_nimbus_fml_loader.py

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
from unittest.mock import patch
33

44
from django.test import TestCase
5-
from nimbus_megazord.fml import FmlClient
5+
from nimbus_megazord.fml import FmlClient, FmlError
66

77
from experimenter.experiments.constants import NimbusConstants
88
from experimenter.features.manifests.nimbus_fml_loader import NimbusFmlLoader
@@ -210,3 +210,36 @@ def test_return_no_errors_for_invalid_application(
210210
self.assertEqual(loader.application, None)
211211
self.assertEqual(result, [])
212212
self.assertIn("Nimbus FML Loader: Invalid application", log.output[0])
213+
214+
@patch(
215+
"nimbus_megazord.fml.FmlClient.__init__",
216+
side_effect=FmlError("gecko-pref and default are mutually exclusive"),
217+
)
218+
@mock_fml_features
219+
def test_fml_client_returns_none_when_manifest_fails_to_parse(self, _mock_client):
220+
loader = self.create_loader()
221+
with self.assertLogs(level="ERROR") as log:
222+
result = loader.fml_client()
223+
self.assertIsNone(result)
224+
self.assertIn(
225+
"Nimbus FML Loader: FmlClient failed to parse manifest",
226+
log.output[0],
227+
)
228+
229+
@patch(
230+
"nimbus_megazord.fml.FmlClient.__init__",
231+
side_effect=FmlError("gecko-pref and default are mutually exclusive"),
232+
)
233+
@mock_fml_features
234+
def test_get_fml_errors_returns_empty_when_manifest_fails_to_parse(
235+
self, _mock_client
236+
):
237+
loader = self.create_loader()
238+
test_blob = json.dumps({"enabled": True})
239+
with self.assertLogs(level="ERROR") as log:
240+
result = loader.get_fml_errors(test_blob, "cookie-banners")
241+
self.assertEqual(result, [])
242+
self.assertIn(
243+
"Nimbus FML Loader: FmlClient failed to parse manifest",
244+
log.output[0],
245+
)

0 commit comments

Comments
 (0)