Skip to content

Commit bb99b2b

Browse files
committed
fix(api): handle DoesNotExist in audit log signals and webhooks (#6879)
When a related FeatureState or FeatureSegment is deleted before the async create_audit_log_from_historical_record task runs, the signals and mappers crash with DoesNotExist errors. Handle these gracefully: - audit/signals.py: catch FeatureState.DoesNotExist in send_feature_flag_went_live_signal and skip the signal - integrations/grafana/mappers.py: catch ObjectDoesNotExist in _get_instance_tags_from_audit_log_record and return empty tags - features/tasks.py: catch ObjectDoesNotExist when accessing feature_state.feature_segment and fall back to None
1 parent e7328d8 commit bb99b2b

File tree

6 files changed

+155
-39
lines changed

6 files changed

+155
-39
lines changed

api/audit/signals.py

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,11 +217,20 @@ def send_feature_flag_went_live_signal(sender, instance, **kwargs): # type: ign
217217

218218
# Handle FeatureState, FeatureStateValue, and MultivariateFeatureStateValue audit logs
219219
# All these types have related_object_type=FEATURE_STATE
220-
if isinstance(audited_instance, (FeatureStateValue, MultivariateFeatureStateValue)):
221-
feature_state = audited_instance.feature_state
222-
elif isinstance(audited_instance, FeatureState):
223-
feature_state = audited_instance
224-
else:
220+
try:
221+
if isinstance(
222+
audited_instance, (FeatureStateValue, MultivariateFeatureStateValue)
223+
):
224+
feature_state = audited_instance.feature_state
225+
elif isinstance(audited_instance, FeatureState):
226+
feature_state = audited_instance
227+
else:
228+
return
229+
except FeatureState.DoesNotExist:
230+
logger.debug(
231+
"FeatureState no longer exists for audit log %d, skipping.",
232+
instance.pk,
233+
)
225234
return
226235

227236
if feature_state.is_scheduled:

api/features/tasks.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import logging
22
from typing import Any
33

4+
from django.core.exceptions import ObjectDoesNotExist
45
from task_processor.decorators import (
56
register_task_handler,
67
)
@@ -131,14 +132,20 @@ def _get_feature_state_webhook_data(
131132
)
132133

133134
assert feature_state.environment is not None
135+
136+
try:
137+
feature_segment = feature_state.feature_segment
138+
except ObjectDoesNotExist:
139+
feature_segment = None
140+
134141
return Webhook.generate_webhook_feature_state_data(
135142
feature_state.feature,
136143
environment=feature_state.environment,
137144
enabled=feature_state.enabled,
138145
value=value,
139146
identity_id=feature_state.identity_id,
140147
identity_identifier=getattr(feature_state.identity, "identifier", None),
141-
feature_segment=feature_state.feature_segment,
148+
feature_segment=feature_segment,
142149
multivariate_feature_state_values=mv_values,
143150
)
144151

api/integrations/grafana/mappers.py

Lines changed: 35 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
from django.core.exceptions import ObjectDoesNotExist
2+
13
from audit.models import AuditLog
24
from audit.services import get_audited_instance_from_audit_log_record
35
from features.models import (
@@ -20,41 +22,44 @@ def _get_feature_tags(
2022
def _get_instance_tags_from_audit_log_record(
2123
audit_log_record: AuditLog,
2224
) -> list[str]:
23-
if instance := get_audited_instance_from_audit_log_record(audit_log_record):
24-
if isinstance(instance, Feature):
25-
return [
26-
f"feature:{instance.name}",
27-
*_get_feature_tags(instance),
28-
]
25+
try:
26+
if instance := get_audited_instance_from_audit_log_record(audit_log_record):
27+
if isinstance(instance, Feature):
28+
return [
29+
f"feature:{instance.name}",
30+
*_get_feature_tags(instance),
31+
]
2932

30-
if isinstance(instance, FeatureState):
31-
return [
32-
f"feature:{(feature := instance.feature).name}",
33-
f"flag:{'enabled' if instance.enabled else 'disabled'}",
34-
*_get_feature_tags(feature), # type: ignore[has-type]
35-
]
33+
if isinstance(instance, FeatureState):
34+
return [
35+
f"feature:{(feature := instance.feature).name}",
36+
f"flag:{'enabled' if instance.enabled else 'disabled'}",
37+
*_get_feature_tags(feature), # type: ignore[has-type]
38+
]
3639

37-
if isinstance(instance, FeatureStateValue):
38-
return [
39-
f"feature:{(feature := instance.feature_state.feature).name}",
40-
*_get_feature_tags(feature),
41-
]
40+
if isinstance(instance, FeatureStateValue):
41+
return [
42+
f"feature:{(feature := instance.feature_state.feature).name}",
43+
*_get_feature_tags(feature),
44+
]
4245

43-
if isinstance(instance, Segment):
44-
return [f"segment:{instance.name}"]
46+
if isinstance(instance, Segment):
47+
return [f"segment:{instance.name}"]
4548

46-
if isinstance(instance, FeatureSegment):
47-
return [
48-
f"feature:{(feature := instance.feature).name}",
49-
f"segment:{instance.segment.name}",
50-
*_get_feature_tags(feature),
51-
]
49+
if isinstance(instance, FeatureSegment):
50+
return [
51+
f"feature:{(feature := instance.feature).name}",
52+
f"segment:{instance.segment.name}",
53+
*_get_feature_tags(feature),
54+
]
5255

53-
if isinstance(instance, EnvironmentFeatureVersion):
54-
return [
55-
f"feature:{instance.feature.name}",
56-
*_get_feature_tags(instance.feature),
57-
]
56+
if isinstance(instance, EnvironmentFeatureVersion):
57+
return [
58+
f"feature:{instance.feature.name}",
59+
*_get_feature_tags(instance.feature),
60+
]
61+
except ObjectDoesNotExist:
62+
return []
5863

5964
return []
6065

api/tests/unit/audit/test_unit_audit_signals.py

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
trigger_feature_state_change_webhooks,
1717
)
1818
from environments.models import Environment
19-
from features.models import Feature, FeatureState
19+
from features.models import Feature, FeatureState, FeatureStateValue
2020
from features.versioning.models import EnvironmentFeatureVersion
2121
from integrations.dynatrace.dynatrace import EVENTS_API_URI
2222
from integrations.dynatrace.models import DynatraceConfiguration
@@ -596,3 +596,40 @@ def test_send_feature_flag_went_live_signal__non_feature_state_instance__skips_s
596596

597597
# Then
598598
mock_signal_send.assert_not_called()
599+
600+
601+
def test_send_feature_flag_went_live_signal__deleted_feature_state__skips_signal(
602+
environment: Environment,
603+
feature: Feature,
604+
mocker: MockerFixture,
605+
) -> None:
606+
# Given
607+
feature_state = FeatureState.objects.get(
608+
environment=environment, feature=feature, feature_segment__isnull=True
609+
)
610+
feature_state_value = feature_state.feature_state_value
611+
612+
audit_log = AuditLog.objects.create(
613+
environment=environment,
614+
related_object_id=feature_state.id,
615+
related_object_type=RelatedObjectType.FEATURE_STATE.name,
616+
history_record_id=feature_state_value.history.first().history_id,
617+
history_record_class_path=feature_state_value.history_record_class_path,
618+
)
619+
620+
# Simulate the FeatureState being deleted before the signal runs.
621+
mock_audited = mocker.MagicMock(spec=FeatureStateValue)
622+
type(mock_audited).feature_state = mocker.PropertyMock(
623+
side_effect=FeatureState.DoesNotExist
624+
)
625+
mocker.patch(
626+
"audit.signals.get_audited_instance_from_audit_log_record",
627+
return_value=mock_audited,
628+
)
629+
mock_signal_send = mocker.patch("audit.signals.feature_state_change_went_live.send")
630+
631+
# When
632+
send_feature_flag_went_live_signal(sender=AuditLog, instance=audit_log)
633+
634+
# Then
635+
mock_signal_send.assert_not_called()

api/tests/unit/features/test_unit_features_tasks.py

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
import pytest
2+
from django.core.exceptions import ObjectDoesNotExist
23
from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped]
34
from pytest_mock import MockerFixture
45

56
from api_keys.models import MasterAPIKey
67
from environments.models import Environment
78
from features.models import Feature, FeatureState
8-
from features.tasks import trigger_feature_state_change_webhooks
9+
from features.tasks import (
10+
_get_feature_state_webhook_data,
11+
trigger_feature_state_change_webhooks,
12+
)
913
from organisations.models import Organisation
1014
from projects.models import Project
1115
from users.models import FFAdminUser
@@ -162,3 +166,26 @@ def test_trigger_feature_state_change_webhooks_for_deleted_flag_uses_fs_instance
162166

163167
assert data["previous_state"]["feature"]["id"] == feature_state.feature.id
164168
assert event_type == WebhookEventType.FLAG_DELETED.value
169+
170+
171+
def test_get_feature_state_webhook_data__deleted_feature_segment__returns_data(
172+
mocker: MockerFixture,
173+
environment: Environment,
174+
feature: Feature,
175+
) -> None:
176+
# Given
177+
feature_state = FeatureState.objects.get(feature=feature, environment=environment)
178+
179+
# Simulate the FeatureSegment being deleted after the FeatureState was loaded.
180+
mocker.patch.object(
181+
type(feature_state),
182+
"feature_segment",
183+
new_callable=mocker.PropertyMock,
184+
side_effect=ObjectDoesNotExist,
185+
)
186+
187+
# When
188+
data = _get_feature_state_webhook_data(feature_state)
189+
190+
# Then - should not raise; feature_segment should be None in the result.
191+
assert data["feature_segment"] is None

api/tests/unit/integrations/grafana/test_mappers.py

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
from audit.models import AuditLog
77
from environments.models import Environment
8-
from features.models import Feature, FeatureSegment, FeatureState
8+
from features.models import Feature, FeatureSegment, FeatureState, FeatureStateValue
99
from integrations.grafana.mappers import (
1010
map_audit_log_record_to_grafana_annotation,
1111
)
@@ -199,6 +199,37 @@ def test_map_audit_log_record_to_grafana_annotation__feature_segment__return_exp
199199
}
200200

201201

202+
def test_map_audit_log_record_to_grafana_annotation__deleted_feature_state__returns_no_instance_tags(
203+
mocker: MockerFixture,
204+
audit_log_record: AuditLog,
205+
) -> None:
206+
# Given
207+
mock_instance = mocker.MagicMock(spec=FeatureStateValue)
208+
type(mock_instance).feature_state = mocker.PropertyMock(
209+
side_effect=FeatureState.DoesNotExist
210+
)
211+
mocker.patch(
212+
"integrations.grafana.mappers.get_audited_instance_from_audit_log_record",
213+
return_value=mock_instance,
214+
)
215+
216+
# When
217+
annotation = map_audit_log_record_to_grafana_annotation(audit_log_record)
218+
219+
# Then
220+
assert annotation == {
221+
"tags": [
222+
"flagsmith",
223+
"project:Test Project",
224+
"environment:unknown",
225+
"by:superuser@example.com",
226+
],
227+
"text": "Test event",
228+
"time": 1719220187325,
229+
"timeEnd": 1719220187325,
230+
}
231+
232+
202233
@pytest.mark.django_db
203234
def test_map_audit_log_record_to_grafana_annotation__generic__return_expected(
204235
mocker: MockerFixture,

0 commit comments

Comments
 (0)