fix(webhook/signal): Optimise historical record writes (#5041)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

api/core/models.py

@@ -42,6 +42,16 @@ def natural_key(self):
         return (str(self.uuid),)
 
 
+class SoftDeleteAwareHistoricalRecords(HistoricalRecords):
+    def create_historical_record(
+        self, instance: models.Model, history_type: str, using: str = None
+    ) -> None:
+        if getattr(instance, "deleted_at", None) is not None and history_type == "~":
+            # Don't create `update` historical record for soft-deleted objects
+            return
+        super().create_historical_record(instance, history_type, using)
+
+
 class SoftDeleteExportableManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager):
     pass
 
@@ -198,7 +208,7 @@ def abstract_base_auditable_model_factory(
     show_change_details_for_create: bool = False,
 ) -> typing.Type[AbstractBaseAuditableModel]:
     class Base(AbstractBaseAuditableModel):
-        history = HistoricalRecords(
+        history = SoftDeleteAwareHistoricalRecords(
             bases=[
                 base_historical_model_factory(
                     change_details_excluded_fields or [],

api/core/signals.py

@@ -34,6 +34,8 @@ def create_audit_log_from_historical_record(
         if settings.TASK_RUN_METHOD == TaskRunMethod.TASK_PROCESSOR
         else None
     )
+    if instance.get_skip_create_audit_log():
+        return
 
     try:
         environment, project = instance.get_environment_and_project()

api/features/signals.py

@@ -12,6 +12,6 @@
 
 @receiver(post_save, sender=FeatureState)
 def trigger_feature_state_change_webhooks_signal(instance, **kwargs):
-    if instance.environment_feature_version_id:
+    if instance.environment_feature_version_id or instance.deleted_at:
         return
     trigger_feature_state_change_webhooks(instance)

api/pyproject.toml

@@ -140,7 +140,7 @@ drf-yasg = "~1.21.6"
 django-debug-toolbar = "~3.2.1"
 sentry-sdk = "~2.8.0"
 environs = "~9.2.0"
-django-lifecycle = "~1.0.0"
+django-lifecycle = "~1.2.4"
 drf-writable-nested = "~0.6.2"
 django-filter = "~2.4.0"
 flagsmith-flag-engine = "^5.3.0"

api/segments/migrations/0027_historicalsegmentrule.py

@@ -0,0 +1,42 @@
+# Generated by Django 3.2.23 on 2025-01-23 12:19
+
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+import simple_history.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api_keys', '0003_masterapikey_is_admin'),
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('segments', '0026_add_change_request_to_segments'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='HistoricalSegmentRule',
+            fields=[
+                ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')),
+                ('deleted_at', models.DateTimeField(blank=True, db_index=True, default=None, editable=False, null=True)),
+                ('type', models.CharField(choices=[('ALL', 'all'), ('ANY', 'any'), ('NONE', 'none')], max_length=50)),
+                ('created_at', models.DateTimeField(blank=True, editable=False, null=True)),
+                ('updated_at', models.DateTimeField(blank=True, editable=False, null=True)),
+                ('history_id', models.AutoField(primary_key=True, serialize=False)),
+                ('history_date', models.DateTimeField()),
+                ('history_change_reason', models.CharField(max_length=100, null=True)),
+                ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)),
+                ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)),
+                ('master_api_key', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.DO_NOTHING, to='api_keys.masterapikey')),
+                ('rule', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='segments.segmentrule')),
+                ('segment', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='segments.segment')),
+            ],
+            options={
+                'verbose_name': 'historical segment rule',
+                'ordering': ('-history_date', '-history_id'),
+                'get_latest_by': 'history_date',
+            },
+            bases=(simple_history.models.HistoricalChanges, models.Model),
+        ),
+    ]

api/segments/models.py

@@ -29,7 +29,6 @@
 from metadata.models import Metadata
 from projects.models import Project
 
-from .helpers import segment_audit_log_helper
 from .managers import LiveSegmentManager, SegmentManager
 
 logger = logging.getLogger(__name__)
@@ -92,10 +91,6 @@ def __str__(self):
         return "Segment - %s" % self.name
 
     def get_skip_create_audit_log(self) -> bool:
-        skip = segment_audit_log_helper.should_skip_audit_log(self.id)
-        if skip is not None:
-            return skip
-
         try:
             if self.version_of_id and self.version_of_id != self.id:
                 return True
@@ -146,10 +141,8 @@ def set_version_of_to_self_if_none(self):
         This allows the segment model to reference all versions of
         itself including itself.
         """
-        segment_audit_log_helper.set_skip_audit_log(self.id)
         self.version_of = self
-        self.save()
-        segment_audit_log_helper.unset_skip_audit_log(self.id)
+        self.save_without_historical_record()
 
     def shallow_clone(
         self,
@@ -178,10 +171,8 @@ def deep_clone(self) -> "Segment":
         cloned_segment.version_of = self
         cloned_segment.save()
 
-        segment_audit_log_helper.set_skip_audit_log(self.id)
         self.version += 1
-        self.save()
-        segment_audit_log_helper.unset_skip_audit_log(self.id)
+        self.save_without_historical_record()
 
         cloned_rules = []
         for rule in self.rules.all():
@@ -211,7 +202,10 @@ def _get_project(self):
         return self.project
 
 
-class SegmentRule(SoftDeleteExportableModel):
+class SegmentRule(
+    SoftDeleteExportableModel,
+    abstract_base_auditable_model_factory(["uuid"]),
+):
     ALL_RULE = "ALL"
     ANY_RULE = "ANY"
     NONE_RULE = "NONE"
@@ -230,6 +224,8 @@ class SegmentRule(SoftDeleteExportableModel):
     created_at = models.DateTimeField(null=True, auto_now_add=True)
     updated_at = models.DateTimeField(null=True, auto_now=True)
 
+    history_record_class_path = "segments.models.HistoricalSegmentRule"
+
     def clean(self):
         super().clean()
         parents = [self.segment, self.rule]
@@ -246,8 +242,17 @@ def __str__(self):
         )
 
     def get_skip_create_audit_log(self) -> bool:
-        segment = self.get_segment()
-        return segment.version_of_id != segment.id
+        try:
+            segment = self.get_segment()
+            if segment.deleted_at:
+                return True
+            return segment.version_of_id != segment.id
+        except (Segment.DoesNotExist, SegmentRule.DoesNotExist):
+            # handle hard delete
+            return True
+
+    def _get_project(self) -> typing.Optional[Project]:
+        return self.get_segment().project
 
     def get_segment(self):
         """
@@ -361,8 +366,19 @@ def __str__(self):
         )
 
     def get_skip_create_audit_log(self) -> bool:
-        segment = self.rule.get_segment()
-        return segment.version_of_id != segment.id
+        try:
+
+            if self.rule.deleted_at:
+                return True
+
+            segment = self.rule.get_segment()
+            if segment.deleted_at:
+                return True
+
+            return segment.version_of_id != segment.id
+        except (Segment.DoesNotExist, SegmentRule.DoesNotExist):
+            # handle hard delete
+            return True
 
     def get_update_log_message(self, history_instance) -> typing.Optional[str]:
         return f"Condition updated on segment '{self._get_segment().name}'."

api/tests/unit/features/test_unit_features_models.py

@@ -470,6 +470,23 @@ def test_feature_state_save_calls_trigger_webhooks(
     mock_trigger_webhooks.assert_called_with(feature_state)
 
 
+def test_delete_feature_should_not_trigger_fs_change_webhooks(
+    mocker: MockerFixture,
+    feature: Feature,
+    environment: Environment,
+) -> None:
+    # Given
+    mock_trigger_webhooks = mocker.patch(
+        "features.signals.trigger_feature_state_change_webhooks"
+    )
+
+    # When
+    feature.delete()
+
+    # Then
+    mock_trigger_webhooks.assert_not_called()
+
+
 def test_feature_state_type_environment(
     feature: Feature,
     environment: Environment,