From 2ad43815117fa09af069b8c39589f5fa3711d738 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:11:09 +0000 Subject: [PATCH 01/27] Create versioning for segments migration --- .../0023_add_versioning_to_segments.py | 58 +++++++++++++++++++ .../sql/0023_add_versioning_to_segments.sql | 2 + 2 files changed, 60 insertions(+) create mode 100644 api/segments/migrations/0023_add_versioning_to_segments.py create mode 100644 api/segments/migrations/sql/0023_add_versioning_to_segments.sql diff --git a/api/segments/migrations/0023_add_versioning_to_segments.py b/api/segments/migrations/0023_add_versioning_to_segments.py new file mode 100644 index 000000000000..1b57dd5f54fd --- /dev/null +++ b/api/segments/migrations/0023_add_versioning_to_segments.py @@ -0,0 +1,58 @@ +# Generated by Django 3.2.25 on 2024-06-06 19:20 +import os + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("segments", "0022_add_soft_delete_to_segment_rules_and_conditions"), + ] + + operations = [ + migrations.AddField( + model_name="historicalsegment", + name="version", + field=models.IntegerField(default=1), + ), + migrations.AddField( + model_name="historicalsegment", + name="version_of", + field=models.ForeignKey( + blank=True, + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.DO_NOTHING, + related_name="+", + to="segments.segment", + ), + ), + migrations.AddField( + model_name="segment", + name="version", + field=models.IntegerField(default=1), + ), + migrations.AddField( + model_name="segment", + name="version_of", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="versioned_segments", + to="segments.segment", + ), + ), + migrations.RunSQL( + sql=open( + os.path.join( + os.path.dirname(__file__), + "sql", + "0023_add_versioning_to_segments.sql", + ) + ).read(), + reverse_sql=migrations.RunSQL.noop, + ), + ] diff --git a/api/segments/migrations/sql/0023_add_versioning_to_segments.sql b/api/segments/migrations/sql/0023_add_versioning_to_segments.sql new file mode 100644 index 000000000000..c67e00488910 --- /dev/null +++ b/api/segments/migrations/sql/0023_add_versioning_to_segments.sql @@ -0,0 +1,2 @@ +UPDATE segments_segment SET version_of_id = id; +UPDATE segments_historicalsegment SET version_of_id = id; From f6746e1414896e2783a29ba1ea69ec660934aba3 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:14:06 +0000 Subject: [PATCH 02/27] Create versioning of segments --- api/segments/models.py | 99 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/api/segments/models.py b/api/segments/models.py index dea6f980a7ea..4d87eab12c65 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -3,6 +3,7 @@ from copy import deepcopy from core.models import ( + SoftDeleteExportableManager, SoftDeleteExportableModel, abstract_base_auditable_model_factory, ) @@ -10,6 +11,7 @@ from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError from django.db import models +from django_lifecycle import AFTER_CREATE, LifecycleModelMixin, hook from flag_engine.segments import constants from audit.constants import ( @@ -22,10 +24,13 @@ from metadata.models import Metadata from projects.models import Project +from .managers import SegmentManager + logger = logging.getLogger(__name__) class Segment( + LifecycleModelMixin, SoftDeleteExportableModel, abstract_base_auditable_model_factory(["uuid"]), ): @@ -45,8 +50,22 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) + version = models.IntegerField(default=1) + version_of = models.ForeignKey( + "self", + on_delete=models.CASCADE, + related_name="versioned_segments", + null=True, + blank=True, + ) metadata = GenericRelation(Metadata) + # Only serves segments that are the canonical version. + objects = SegmentManager() + + # Includes versioned segments. + all_objects = SoftDeleteExportableManager() + class Meta: ordering = ("id",) # explicit ordering to prevent pagination warnings @@ -84,6 +103,41 @@ def id_exists_in_rules_data(rules_data: typing.List[dict]) -> bool: return False + @hook(AFTER_CREATE, when="version_of", is_now=None) + def set_version_of_to_self_if_none(self): + """ + This allows the segment model to reference all versions of + itself including itself. + """ + self.version_of = self + self.save() + + def deep_clone(self) -> "Segment": + new_segment = Segment.objects.create( + name=self.name, + description=self.description, + project=self.project, + feature=self.feature, + version=self.version, + version_of=self, + ) + + self.version += 1 + self.save() + + new_rules = [] + for rule in self.rules.all(): + new_rule = rule.deep_clone(new_segment) + new_rules.append(new_rule) + + new_segment.refresh_from_db() + + assert ( + len(self.rules.all()) == len(new_rules) == len(new_segment.rules.all()) + ), "Mismatch during rules creation" + + return new_segment + def get_create_log_message(self, history_instance) -> typing.Optional[str]: return SEGMENT_CREATED_MESSAGE % self.name @@ -140,6 +194,51 @@ def get_segment(self): rule = rule.rule return rule.segment + def deep_clone(self, versioned_segment: Segment) -> "SegmentRule": + if self.rule: + assert False, "Unexpected rule, expecting segment set not rule" + new_rule = SegmentRule.objects.create( + segment=versioned_segment, + type=self.type, + ) + + new_conditions = [] + for condition in self.conditions.all(): + new_condition = Condition( + operator=condition.operator, + property=condition.property, + value=condition.value, + description=condition.description, + created_with_segment=condition.created_with_segment, + rule=new_rule, + ) + new_conditions.append(new_condition) + Condition.objects.bulk_create(new_conditions) + + for sub_rule in self.rules.all(): + if sub_rule.rules.exists(): + logger.error("Expected two layers of rules, not more") + + new_sub_rule = SegmentRule.objects.create( + rule=new_rule, + type=sub_rule.type, + ) + + new_conditions = [] + for condition in sub_rule.conditions.all(): + new_condition = Condition( + operator=condition.operator, + property=condition.property, + value=condition.value, + description=condition.description, + created_with_segment=condition.created_with_segment, + rule=new_sub_rule, + ) + new_conditions.append(new_condition) + Condition.objects.bulk_create(new_conditions) + + return new_rule + class Condition( SoftDeleteExportableModel, abstract_base_auditable_model_factory(["uuid"]) From 8df7f91e84778d49e3c894b4034aa701a639634e Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:14:55 +0000 Subject: [PATCH 03/27] Create segments manager for versioned segments --- api/segments/managers.py | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 api/segments/managers.py diff --git a/api/segments/managers.py b/api/segments/managers.py new file mode 100644 index 000000000000..d1f977ca06ed --- /dev/null +++ b/api/segments/managers.py @@ -0,0 +1,11 @@ +from core.models import SoftDeleteExportableManager +from django.db.models import F + + +class SegmentManager(SoftDeleteExportableManager): + def get_queryset(self): + """ + Returns only the canonical segments, which will always be + the highest version. + """ + return super().get_queryset().filter(id=F("version_of")) From a5822538fa4604b2cd780b1bf848229af0798ba3 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:54:38 +0000 Subject: [PATCH 04/27] Switch to nullable integer --- api/segments/migrations/0023_add_versioning_to_segments.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/segments/migrations/0023_add_versioning_to_segments.py b/api/segments/migrations/0023_add_versioning_to_segments.py index 1b57dd5f54fd..0e7c8f0d5a03 100644 --- a/api/segments/migrations/0023_add_versioning_to_segments.py +++ b/api/segments/migrations/0023_add_versioning_to_segments.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.25 on 2024-06-06 19:20 +# Generated by Django 3.2.25 on 2024-06-10 15:31 import os import django.db.models.deletion @@ -15,7 +15,7 @@ class Migration(migrations.Migration): migrations.AddField( model_name="historicalsegment", name="version", - field=models.IntegerField(default=1), + field=models.IntegerField(null=True), ), migrations.AddField( model_name="historicalsegment", @@ -32,7 +32,7 @@ class Migration(migrations.Migration): migrations.AddField( model_name="segment", name="version", - field=models.IntegerField(default=1), + field=models.IntegerField(null=True), ), migrations.AddField( model_name="segment", From 730d064791c0f17c951f6dbd00392e6ba7016e90 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:55:01 +0000 Subject: [PATCH 05/27] Deep clone the default segment fixture --- api/conftest.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index a799dfbf43d0..7632761ce600 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -326,8 +326,13 @@ def project(organisation): @pytest.fixture() -def segment(project): - return Segment.objects.create(name="segment", project=project) +def segment(project: Project): + _segment = Segment.objects.create(name="segment", project=project) + # Deep clone the segment to ensure that any bugs around + # versioning get bubbled up through the test suite. + _segment.deep_clone() + + return _segment @pytest.fixture() From 42f93a92d99bdc68096be4fe50f08bb1d72bbe04 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:55:35 +0000 Subject: [PATCH 06/27] Deep clone during update --- api/segments/serializers.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index fccbeecf45aa..7bce1182c509 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -81,11 +81,15 @@ def create(self, validated_data): self._update_or_create_metadata(metadata_data, segment=segment) return segment - def update(self, instance, validated_data): + def update(self, instance: Segment, validated_data: dict[str, typing.Any]) -> None: # use the initial data since we need the ids included to determine which to update & which to create rules_data = self.initial_data.pop("rules", []) metadata_data = validated_data.pop("metadata", []) self.validate_segment_rules_conditions_limit(rules_data) + + # Create a version of the segment now that we're updating. + instance.deep_clone() + self._update_segment_rules(rules_data, segment=instance) self._update_or_create_metadata(metadata_data, segment=instance) # remove rules from validated data to prevent error trying to create segment with nested rules From c7eb529b9555955a1f5942f097c550d2b9055838 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 15:55:56 +0000 Subject: [PATCH 07/27] Create default for newly created segments --- api/segments/models.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 4d87eab12c65..8f2a26acd03c 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -11,7 +11,12 @@ from django.contrib.contenttypes.fields import GenericRelation from django.core.exceptions import ValidationError from django.db import models -from django_lifecycle import AFTER_CREATE, LifecycleModelMixin, hook +from django_lifecycle import ( + AFTER_CREATE, + BEFORE_CREATE, + LifecycleModelMixin, + hook, +) from flag_engine.segments import constants from audit.constants import ( @@ -50,7 +55,10 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) - version = models.IntegerField(default=1) + # This defaults to 1 for newly created segments. + version = models.IntegerField(null=True) + + # The related_name is not useful without specifying all_objects as a manager. version_of = models.ForeignKey( "self", on_delete=models.CASCADE, @@ -103,6 +111,11 @@ def id_exists_in_rules_data(rules_data: typing.List[dict]) -> bool: return False + @hook(BEFORE_CREATE, when="version_of", is_now=None) + def set_default_version_to_one_if_new_segment(self): + if self.version is None: + self.version = 1 + @hook(AFTER_CREATE, when="version_of", is_now=None) def set_version_of_to_self_if_none(self): """ From f06cb3da36f8a19e72c11190c792a13f4d4e8c6b Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 18:49:24 +0000 Subject: [PATCH 08/27] Test deep cloning of segments --- .../segments/test_unit_segments_models.py | 191 ++++++++++++++++++ 1 file changed, 191 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index de955abe95b5..17d340a55110 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -1,6 +1,8 @@ import pytest from flag_engine.segments.constants import EQUAL, PERCENTAGE_SPLIT +from features.models import Feature +from projects.models import Project from segments.models import Condition, Segment, SegmentRule @@ -167,3 +169,192 @@ def test_condition_get_update_log_message(segment, segment_rule, mocker): ) def test_segment_id_exists_in_rules_data(rules_data, expected_result): assert Segment.id_exists_in_rules_data(rules_data) == expected_result + + +def test_deep_clone_of_segment( + project: Project, + feature: Feature, +) -> None: + # Given + segment = Segment.objects.create( + name="SpecialSegment", + description="A lovely, special segment.", + project=project, + feature=feature, + ) + + # Check that the versioning is correct, since we'll testing + # against it later in the test. + assert segment.version == 1 + assert segment.version_of == segment + + parent_rule1 = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + parent_rule2 = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ANY_RULE + ) + + child_rule1 = SegmentRule.objects.create( + rule=parent_rule1, type=SegmentRule.ANY_RULE + ) + child_rule2 = SegmentRule.objects.create( + rule=parent_rule1, type=SegmentRule.ALL_RULE + ) + child_rule3 = SegmentRule.objects.create( + rule=parent_rule2, type=SegmentRule.NONE_RULE + ) + child_rule4 = SegmentRule.objects.create( + rule=parent_rule2, type=SegmentRule.ALL_RULE + ) + + condition1 = Condition.objects.create( + rule=parent_rule1, + property="parent_rule1", + operator=EQUAL, + value="condition1", + created_with_segment=True, + ) + condition2 = Condition.objects.create( + rule=parent_rule1, + property="parent_rule1", + operator=PERCENTAGE_SPLIT, + value="0.1", + created_with_segment=False, + ) + + condition3 = Condition.objects.create( + rule=child_rule1, + property="child_rule1", + operator=EQUAL, + value="condition3", + created_with_segment=True, + ) + Condition.objects.create( + rule=child_rule2, + property="child_rule2", + operator=PERCENTAGE_SPLIT, + value="0.2", + created_with_segment=False, + ) + Condition.objects.create( + rule=child_rule2, + property="child_rule2", + operator=EQUAL, + value="condition5", + created_with_segment=False, + ) + + Condition.objects.create( + rule=child_rule3, + property="child_rule3", + operator=EQUAL, + value="condition6", + created_with_segment=False, + ) + + Condition.objects.create( + rule=child_rule4, + property="child_rule4", + operator=EQUAL, + value="condition7", + created_with_segment=True, + ) + + # When + new_segment = segment.deep_clone() + + # Then + assert new_segment.name == segment.name + assert new_segment.description == segment.description + assert new_segment.project == project + assert new_segment.feature == feature + assert new_segment.version == 1 + assert new_segment.version_of == segment + + assert segment.version == 2 + + assert len(new_segment.rules.all()) == len(segment.rules.all()) == 2 + new_parent_rule1, new_parent_rule2 = list(new_segment.rules.all()) + + assert new_parent_rule1.segment == new_segment + assert new_parent_rule2.segment == new_segment + assert new_parent_rule1.type == parent_rule1.type + assert new_parent_rule2.type == parent_rule2.type + + assert len(new_parent_rule1.rules.all()) == len(parent_rule1.rules.all()) == 2 + assert len(new_parent_rule2.rules.all()) == len(parent_rule2.rules.all()) == 2 + new_child_rule1, new_child_rule2 = list(new_parent_rule1.rules.all()) + + assert new_child_rule1.type == child_rule1.type + assert new_child_rule2.type == child_rule2.type + + new_child_rule3, new_child_rule4 = list(new_parent_rule2.rules.all()) + + assert new_child_rule3.type == child_rule3.type + assert new_child_rule4.type == child_rule4.type + + assert ( + len(new_parent_rule1.conditions.all()) + == len(parent_rule1.conditions.all()) + == 2 + ) + + new_condition1, new_condition2 = list(new_parent_rule1.conditions.all()) + + assert new_condition1.property == condition1.property + assert new_condition1.operator == condition1.operator + assert new_condition1.value == condition1.value + assert new_condition1.created_with_segment is condition1.created_with_segment + + assert new_condition2.property == condition2.property + assert new_condition2.operator == condition2.operator + assert new_condition2.value == condition2.value + assert new_condition2.created_with_segment is condition2.created_with_segment + + assert ( + len(new_child_rule1.conditions.all()) == len(child_rule1.conditions.all()) == 1 + ) + new_condition3 = new_child_rule1.conditions.first() + + assert new_condition3.property == condition3.property + assert new_condition3.operator == condition3.operator + assert new_condition3.value == condition3.value + assert new_condition3.created_with_segment is condition3.created_with_segment + + # Since the conditions have been asserted at multiple levels, + # we leave the rest to basic size assertions. + assert ( + len(new_child_rule2.conditions.all()) == len(child_rule2.conditions.all()) == 2 + ) + assert ( + len(new_child_rule3.conditions.all()) == len(child_rule3.conditions.all()) == 1 + ) + assert ( + len(new_child_rule4.conditions.all()) == len(child_rule4.conditions.all()) == 1 + ) + + +def test_manager_returns_only_highest_version_of_segments( + segment: Segment, +) -> None: + # Given + # The built-in segment fixture is pre-versioned already. + assert segment.version == 2 + assert segment.version_of == segment + + new_segment = segment.deep_clone() + assert new_segment.version == 2 + assert segment.version == 3 + + # When + queryset1 = Segment.objects.filter(id=new_segment.id) + queryset2 = Segment.all_objects.filter(id=new_segment.id) + queryset3 = Segment.objects.filter(id=segment.id) + queryset4 = Segment.all_objects.filter(id=segment.id) + + # Then + assert not queryset1.exists() + assert queryset2.first() == new_segment + assert queryset3.first() == segment + assert queryset4.first() == segment From 641c02d1713ad6542eac3d9cc7c3d9738eb8c40d Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 18:50:08 +0000 Subject: [PATCH 09/27] Update unit segments views --- .../unit/segments/test_unit_segments_views.py | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index 1b1da0035ced..c66ac7bad194 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -158,6 +158,7 @@ def test_create_segments_reaching_max_limit(project, client, settings): def test_audit_log_created_when_segment_updated(project, segment, client): # Given segment = Segment.objects.create(name="Test segment", project=project) + url = reverse( "api-v1:projects:project-segments-detail", args=[project.id, segment.id], @@ -173,11 +174,14 @@ def test_audit_log_created_when_segment_updated(project, segment, client): # Then assert res.status_code == status.HTTP_200_OK + + # Three audit log records created, two for the primary + # segment and one for the newly versioned segment. assert ( AuditLog.objects.filter( related_object_type=RelatedObjectType.SEGMENT.name ).count() - == 1 + == 3 ) @@ -248,11 +252,14 @@ def test_audit_log_created_when_segment_created(project, client): # Then assert res.status_code == status.HTTP_201_CREATED + + # Two AuditLog instances are created because of the AFTER_CREATE hook + # of the Segment model which sets the version_of field to self. assert ( AuditLog.objects.filter( related_object_type=RelatedObjectType.SEGMENT.name ).count() - == 1 + == 2 ) @@ -468,11 +475,12 @@ def test_create_segments_with_description_condition(project, client): assert segment_condition_description_value == "test-description" -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) -def test_update_segment_add_new_condition(project, client, segment, segment_rule): +def test_update_segment_add_new_condition( + project: Project, + admin_client_new: APIClient, + segment: Segment, + segment_rule: SegmentRule, +) -> None: # Given url = reverse( "api-v1:projects:project-segments-detail", args=[project.id, segment.id] @@ -521,7 +529,9 @@ def test_update_segment_add_new_condition(project, client, segment, segment_rule } # When - response = client.put(url, data=json.dumps(data), content_type="application/json") + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) # Then assert response.status_code == status.HTTP_200_OK From 7341357951bbf7ea4cbd802e1991a4cd66fda4cb Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 19:12:20 +0000 Subject: [PATCH 10/27] Update audit log tests for segments versioning --- api/tests/integration/audit/test_audit_logs.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/tests/integration/audit/test_audit_logs.py b/api/tests/integration/audit/test_audit_logs.py index 9ee71f325486..7ea891ff8b68 100644 --- a/api/tests/integration/audit/test_audit_logs.py +++ b/api/tests/integration/audit/test_audit_logs.py @@ -250,7 +250,7 @@ def test_retrieve_audit_log_includes_changes_when_segment_override_created_and_d get_audit_logs_response_2 = admin_client.get(get_audit_logs_url) assert get_audit_logs_response_2.status_code == status.HTTP_200_OK results = get_audit_logs_response_2.json()["results"] - assert len(results) == 5 + assert len(results) == 6 # and the first one in the list should be for the deletion of the segment override delete_override_audit_log_id = results[0]["id"] @@ -308,9 +308,9 @@ def test_retrieve_audit_log_includes_changes_when_segment_override_created_for_f # and we should only have one audit log in the list related to the segment override # (since the FeatureState hasn't changed) - # 1 for creating the feature + 1 for creating the environment + 1 for creating the segment - # + 1 for the segment override = 4 - assert len(results) == 4 + # 1 for creating the feature + 1 for creating the environment + 2 for creating the segment + # + 1 for the segment override = 5 + assert len(results) == 5 # the first audit log in the list (i.e. most recent) should be the one that we want audit_log_id = results[0]["id"] From 41f086dd850036966ee96c8676e2d22c8fe7c46c Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 19:39:54 +0000 Subject: [PATCH 11/27] Switch to assertion error, since it's closer to intent --- api/segments/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/segments/models.py b/api/segments/models.py index 8f2a26acd03c..d7bc8476ebb7 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -230,7 +230,7 @@ def deep_clone(self, versioned_segment: Segment) -> "SegmentRule": for sub_rule in self.rules.all(): if sub_rule.rules.exists(): - logger.error("Expected two layers of rules, not more") + assert False, "Expected two layers of rules, not more" new_sub_rule = SegmentRule.objects.create( rule=new_rule, From ed56775e9290baf3317b248113f5c972c6f94e88 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 10 Jun 2024 19:40:28 +0000 Subject: [PATCH 12/27] Add tests for edge cases --- .../segments/test_unit_segments_models.py | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 17d340a55110..3e3200ed9dfd 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -358,3 +358,58 @@ def test_manager_returns_only_highest_version_of_segments( assert queryset2.first() == new_segment assert queryset3.first() == segment assert queryset4.first() == segment + + +def test_deep_clone_of_segment_with_sub_rule( + project: Project, + feature: Feature, +) -> None: + # Given + segment = Segment.objects.create( + name="SpecialSegment", + description="A lovely, special segment.", + project=project, + feature=feature, + ) + + sub_rule = SegmentRule.objects.create(type=SegmentRule.ALL_RULE) + + # Parent rule with invalid sub rule. + SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE, rule=sub_rule + ) + + with pytest.raises(AssertionError) as exception: + segment.deep_clone() + + assert ( + "AssertionError: Unexpected rule, expecting segment set not rule" + == exception.exconly() + ) + + +def test_deep_clone_of_segment_with_grandchild_rule( + project: Project, + feature: Feature, +) -> None: + # Given + segment = Segment.objects.create( + name="SpecialSegment", + description="A lovely, special segment.", + project=project, + feature=feature, + ) + + parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) + + child_rule = SegmentRule.objects.create(rule=parent_rule, type=SegmentRule.ANY_RULE) + + # Grandchild rule, which is invalid + SegmentRule.objects.create(rule=child_rule, type=SegmentRule.ANY_RULE) + + with pytest.raises(AssertionError) as exception: + segment.deep_clone() + + assert ( + "AssertionError: Expected two layers of rules, not more" == exception.exconly() + ) From 922001d56488c73b9c50cadade789303c530cd24 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 11 Jun 2024 18:46:25 +0000 Subject: [PATCH 13/27] Remove historical from sql migration --- api/segments/migrations/sql/0023_add_versioning_to_segments.sql | 1 - 1 file changed, 1 deletion(-) diff --git a/api/segments/migrations/sql/0023_add_versioning_to_segments.sql b/api/segments/migrations/sql/0023_add_versioning_to_segments.sql index c67e00488910..3e91d2ad844c 100644 --- a/api/segments/migrations/sql/0023_add_versioning_to_segments.sql +++ b/api/segments/migrations/sql/0023_add_versioning_to_segments.sql @@ -1,2 +1 @@ UPDATE segments_segment SET version_of_id = id; -UPDATE segments_historicalsegment SET version_of_id = id; From 4842576ebac9680d55581230c8af2873e51e2d5b Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 11 Jun 2024 18:48:49 +0000 Subject: [PATCH 14/27] Switch to deepcopy for deep cloned objects --- api/segments/models.py | 58 +++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index d7bc8476ebb7..2461162468cf 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -1,5 +1,6 @@ import logging import typing +import uuid from copy import deepcopy from core.models import ( @@ -126,14 +127,11 @@ def set_version_of_to_self_if_none(self): self.save() def deep_clone(self) -> "Segment": - new_segment = Segment.objects.create( - name=self.name, - description=self.description, - project=self.project, - feature=self.feature, - version=self.version, - version_of=self, - ) + new_segment = deepcopy(self) + new_segment.id = None + new_segment.uuid = uuid.uuid4() + new_segment.version_of = self + new_segment.save() self.version += 1 self.save() @@ -209,22 +207,21 @@ def get_segment(self): def deep_clone(self, versioned_segment: Segment) -> "SegmentRule": if self.rule: + # Since we're expecting a rule that is only belonging to a + # segment, we don't expect there also to be a rule associated. assert False, "Unexpected rule, expecting segment set not rule" - new_rule = SegmentRule.objects.create( - segment=versioned_segment, - type=self.type, - ) + new_rule = deepcopy(self) + new_rule.segment = versioned_segment + new_rule.uuid = uuid.uuid4() + new_rule.id = None + new_rule.save() new_conditions = [] for condition in self.conditions.all(): - new_condition = Condition( - operator=condition.operator, - property=condition.property, - value=condition.value, - description=condition.description, - created_with_segment=condition.created_with_segment, - rule=new_rule, - ) + new_condition = deepcopy(condition) + new_condition.uuid = uuid.uuid4() + new_condition.rule = new_rule + new_condition.id = None new_conditions.append(new_condition) Condition.objects.bulk_create(new_conditions) @@ -232,21 +229,18 @@ def deep_clone(self, versioned_segment: Segment) -> "SegmentRule": if sub_rule.rules.exists(): assert False, "Expected two layers of rules, not more" - new_sub_rule = SegmentRule.objects.create( - rule=new_rule, - type=sub_rule.type, - ) + new_sub_rule = deepcopy(sub_rule) + new_sub_rule.rule = new_rule + new_sub_rule.uuid = uuid.uuid4() + new_sub_rule.id = None + new_sub_rule.save() new_conditions = [] for condition in sub_rule.conditions.all(): - new_condition = Condition( - operator=condition.operator, - property=condition.property, - value=condition.value, - description=condition.description, - created_with_segment=condition.created_with_segment, - rule=new_sub_rule, - ) + new_condition = deepcopy(condition) + new_condition.rule = new_sub_rule + new_condition.uuid = uuid.uuid4() + new_condition.id = None new_conditions.append(new_condition) Condition.objects.bulk_create(new_conditions) From b6bbf726f8d53506964b23016037b81e2cf04576 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 15:20:30 +0000 Subject: [PATCH 15/27] Update to copying from instances and add in skips for audit log --- api/segments/models.py | 123 ++++++++++++++++++++++++++++------------- 1 file changed, 85 insertions(+), 38 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 2461162468cf..0adf3aa903f2 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -30,6 +30,7 @@ from metadata.models import Metadata from projects.models import Project +from .helpers import segment_audit_log_helper from .managers import SegmentManager logger = logging.getLogger(__name__) @@ -81,6 +82,16 @@ class Meta: 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 + + if self.version_of != self: + return True + + return False + @staticmethod def id_exists_in_rules_data(rules_data: typing.List[dict]) -> bool: """ @@ -123,31 +134,58 @@ 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) + + def deep_delete(self, hard: bool) -> None: + for rule in self.rules.all(): + for child_rule in rule.rules.all(): + for condition in child_rule.conditions.all(): + if hard: + condition.hard_delete() + else: + condition.delete() + if hard: + child_rule.hard_delete() + else: + child_rule.delete() + if hard: + rule.hard_delete() + else: + rule.delete() + if hard: + self.hard_delete() + else: + self.delete() def deep_clone(self) -> "Segment": - new_segment = deepcopy(self) - new_segment.id = None - new_segment.uuid = uuid.uuid4() - new_segment.version_of = self - new_segment.save() + cloned_segment = deepcopy(self) + cloned_segment.id = None + cloned_segment.uuid = uuid.uuid4() + 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) - new_rules = [] + cloned_rules = [] for rule in self.rules.all(): - new_rule = rule.deep_clone(new_segment) - new_rules.append(new_rule) + cloned_rule = rule.deep_clone(cloned_segment) + cloned_rules.append(cloned_rule) - new_segment.refresh_from_db() + cloned_segment.refresh_from_db() assert ( - len(self.rules.all()) == len(new_rules) == len(new_segment.rules.all()) + len(self.rules.all()) + == len(cloned_rules) + == len(cloned_segment.rules.all()) ), "Mismatch during rules creation" - return new_segment + return cloned_segment def get_create_log_message(self, history_instance) -> typing.Optional[str]: return SEGMENT_CREATED_MESSAGE % self.name @@ -193,6 +231,10 @@ def __str__(self): str(self.segment) if self.segment else str(self.rule), ) + def get_skip_create_audit_log(self) -> bool: + segment = self.get_segment() + return segment.version_of != segment + def get_segment(self): """ rules can be a child of a parent rule instead of a segment, this method iterates back up the tree to find the @@ -205,46 +247,47 @@ def get_segment(self): rule = rule.rule return rule.segment - def deep_clone(self, versioned_segment: Segment) -> "SegmentRule": + def deep_clone(self, cloned_segment: Segment) -> "SegmentRule": if self.rule: # Since we're expecting a rule that is only belonging to a - # segment, we don't expect there also to be a rule associated. + # segment, since a rule either belongs to a segment xor belongs + # to a rule, we don't expect there also to be a rule associated. assert False, "Unexpected rule, expecting segment set not rule" - new_rule = deepcopy(self) - new_rule.segment = versioned_segment - new_rule.uuid = uuid.uuid4() - new_rule.id = None - new_rule.save() + cloned_rule = deepcopy(self) + cloned_rule.segment = cloned_segment + cloned_rule.uuid = uuid.uuid4() + cloned_rule.id = None + cloned_rule.save() - new_conditions = [] + cloned_conditions = [] for condition in self.conditions.all(): - new_condition = deepcopy(condition) - new_condition.uuid = uuid.uuid4() - new_condition.rule = new_rule - new_condition.id = None - new_conditions.append(new_condition) - Condition.objects.bulk_create(new_conditions) + cloned_condition = deepcopy(condition) + cloned_condition.uuid = uuid.uuid4() + cloned_condition.rule = cloned_rule + cloned_condition.id = None + cloned_conditions.append(cloned_condition) + Condition.objects.bulk_create(cloned_conditions) for sub_rule in self.rules.all(): if sub_rule.rules.exists(): assert False, "Expected two layers of rules, not more" - new_sub_rule = deepcopy(sub_rule) - new_sub_rule.rule = new_rule - new_sub_rule.uuid = uuid.uuid4() - new_sub_rule.id = None - new_sub_rule.save() + cloned_sub_rule = deepcopy(sub_rule) + cloned_sub_rule.rule = cloned_rule + cloned_sub_rule.uuid = uuid.uuid4() + cloned_sub_rule.id = None + cloned_sub_rule.save() - new_conditions = [] + cloned_conditions = [] for condition in sub_rule.conditions.all(): - new_condition = deepcopy(condition) - new_condition.rule = new_sub_rule - new_condition.uuid = uuid.uuid4() - new_condition.id = None - new_conditions.append(new_condition) - Condition.objects.bulk_create(new_conditions) + cloned_condition = deepcopy(condition) + cloned_condition.rule = cloned_sub_rule + cloned_condition.uuid = uuid.uuid4() + cloned_condition.id = None + cloned_conditions.append(cloned_condition) + Condition.objects.bulk_create(cloned_conditions) - return new_rule + return cloned_rule class Condition( @@ -294,6 +337,10 @@ def __str__(self): self.value, ) + def get_skip_create_audit_log(self) -> bool: + segment = self.rule.get_segment() + return segment.version_of != segment + def get_update_log_message(self, history_instance) -> typing.Optional[str]: return f"Condition updated on segment '{self._get_segment().name}'." From 8ed85c4e28ab1297261ab0422cd10939ff91fbe7 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 15:22:11 +0000 Subject: [PATCH 16/27] Add try and except block to gaurd model updates --- api/segments/serializers.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index 7bce1182c509..df580254bc21 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -88,13 +88,24 @@ def update(self, instance: Segment, validated_data: dict[str, typing.Any]) -> No self.validate_segment_rules_conditions_limit(rules_data) # Create a version of the segment now that we're updating. - instance.deep_clone() - - self._update_segment_rules(rules_data, segment=instance) - self._update_or_create_metadata(metadata_data, segment=instance) - # remove rules from validated data to prevent error trying to create segment with nested rules - del validated_data["rules"] - return super().update(instance, validated_data) + cloned_segment = instance.deep_clone() + + try: + self._update_segment_rules(rules_data, segment=instance) + self._update_or_create_metadata(metadata_data, segment=instance) + + # remove rules from validated data to prevent error trying to create segment with nested rules + del validated_data["rules"] + response = super().update(instance, validated_data) + except Exception: + # Since there was a problem during the update we now delete the cloned segment, + # since we no longer need a versioned segment. + instance.refresh_from_db() + instance.version = cloned_segment.version + instance.save() + cloned_segment.deep_delete(hard=True) + raise + return response def validate_project_segment_limit(self, project: Project) -> None: if project.segments.count() >= project.max_segments_allowed: From a37def646c98e1949d93af49b81ecbcf45e2877b Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 15:22:58 +0000 Subject: [PATCH 17/27] Create audit log helper --- api/segments/helpers.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 api/segments/helpers.py diff --git a/api/segments/helpers.py b/api/segments/helpers.py new file mode 100644 index 000000000000..9d5797f512dd --- /dev/null +++ b/api/segments/helpers.py @@ -0,0 +1,16 @@ +class SegmentAuditLogHelper: + def __init__(self) -> None: + self.skip_audit_log = {} + + def should_skip_audit_log(self, segment_id: int) -> None | bool: + return self.skip_audit_log.get(segment_id) + + def set_skip_audit_log(self, segment_id: int) -> None: + self.skip_audit_log[segment_id] = True + + def unset_skip_audit_log(self, segment_id: int) -> None: + if segment_id in self.skip_audit_log: + del self.skip_audit_log[segment_id] + + +segment_audit_log_helper = SegmentAuditLogHelper() From 934d2cd04492bcf79e69fa020aa0153d819c9d6f Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 15:23:29 +0000 Subject: [PATCH 18/27] Update audit log tests and add view test for versioned segments --- .../integration/audit/test_audit_logs.py | 8 +- .../unit/segments/test_unit_segments_views.py | 100 ++++++++++++++++-- 2 files changed, 94 insertions(+), 14 deletions(-) diff --git a/api/tests/integration/audit/test_audit_logs.py b/api/tests/integration/audit/test_audit_logs.py index 7ea891ff8b68..9ee71f325486 100644 --- a/api/tests/integration/audit/test_audit_logs.py +++ b/api/tests/integration/audit/test_audit_logs.py @@ -250,7 +250,7 @@ def test_retrieve_audit_log_includes_changes_when_segment_override_created_and_d get_audit_logs_response_2 = admin_client.get(get_audit_logs_url) assert get_audit_logs_response_2.status_code == status.HTTP_200_OK results = get_audit_logs_response_2.json()["results"] - assert len(results) == 6 + assert len(results) == 5 # and the first one in the list should be for the deletion of the segment override delete_override_audit_log_id = results[0]["id"] @@ -308,9 +308,9 @@ def test_retrieve_audit_log_includes_changes_when_segment_override_created_for_f # and we should only have one audit log in the list related to the segment override # (since the FeatureState hasn't changed) - # 1 for creating the feature + 1 for creating the environment + 2 for creating the segment - # + 1 for the segment override = 5 - assert len(results) == 5 + # 1 for creating the feature + 1 for creating the environment + 1 for creating the segment + # + 1 for the segment override = 4 + assert len(results) == 4 # the first audit log in the list (i.e. most recent) should be the one that we want audit_log_id = results[0]["id"] diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index e50de8fbdb84..e6a9192a4e87 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -157,10 +157,9 @@ def test_create_segments_reaching_max_limit(project, client, settings): "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) -def test_audit_log_created_when_segment_updated(project, segment, client): +def test_audit_log_created_when_segment_updated(project, client): # Given segment = Segment.objects.create(name="Test segment", project=project) - url = reverse( "api-v1:projects:project-segments-detail", args=[project.id, segment.id], @@ -172,18 +171,16 @@ def test_audit_log_created_when_segment_updated(project, segment, client): } # When - res = client.put(url, data=json.dumps(data), content_type="application/json") + response = client.put(url, data=json.dumps(data), content_type="application/json") # Then - assert res.status_code == status.HTTP_200_OK + assert response.status_code == status.HTTP_200_OK - # Three audit log records created, two for the primary - # segment and one for the newly versioned segment. assert ( AuditLog.objects.filter( related_object_type=RelatedObjectType.SEGMENT.name ).count() - == 3 + == 1 ) @@ -255,13 +252,11 @@ def test_audit_log_created_when_segment_created(project, client): # Then assert res.status_code == status.HTTP_201_CREATED - # Two AuditLog instances are created because of the AFTER_CREATE hook - # of the Segment model which sets the version_of field to self. assert ( AuditLog.objects.filter( related_object_type=RelatedObjectType.SEGMENT.name ).count() - == 2 + == 1 ) @@ -558,6 +553,91 @@ def test_update_segment_add_new_condition( assert nested_rule.conditions.order_by("-id").first().value == new_condition_value +def test_update_segment_versioned_segment( + project: Project, + admin_client_new: APIClient, + segment: Segment, + segment_rule: SegmentRule, +) -> None: + # Given + url = reverse( + "api-v1:projects:project-segments-detail", args=[project.id, segment.id] + ) + nested_rule = SegmentRule.objects.create( + rule=segment_rule, type=SegmentRule.ANY_RULE + ) + existing_condition = Condition.objects.create( + rule=nested_rule, property="foo", operator=EQUAL, value="bar" + ) + + # Before updating the segment confirm pre-existing version count which is + # automatically set by the fixture. + assert Segment.all_objects.filter(version_of=segment).count() == 2 + + new_condition_property = "foo2" + new_condition_value = "bar" + data = { + "name": segment.name, + "project": project.id, + "rules": [ + { + "id": segment_rule.id, + "type": segment_rule.type, + "rules": [ + { + "id": nested_rule.id, + "type": nested_rule.type, + "rules": [], + "conditions": [ + # existing condition + { + "id": existing_condition.id, + "property": existing_condition.property, + "operator": existing_condition.operator, + "value": existing_condition.value, + }, + # new condition + { + "property": new_condition_property, + "operator": EQUAL, + "value": new_condition_value, + }, + ], + } + ], + "conditions": [], + } + ], + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + assert response.status_code == status.HTTP_200_OK + + # Now verify that a new versioned segment has been set. + assert Segment.all_objects.filter(version_of=segment).count() == 3 + + # Now check the previously versioned segment to match former count of conditions. + + versioned_segment = Segment.all_objects.filter( + version_of=segment, version=2 + ).first() + assert versioned_segment != segment + assert versioned_segment.rules.count() == 1 + versioned_rule = versioned_segment.rules.first() + assert versioned_rule.rules.count() == 1 + + nested_versioned_rule = versioned_rule.rules.first() + assert nested_versioned_rule.conditions.count() == 1 + versioned_condition = nested_versioned_rule.conditions.first() + assert versioned_condition != existing_condition + assert versioned_condition.property == existing_condition.property + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], From 17b2c9db50e801b768430fa916c126493d592d9a Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 15:24:23 +0000 Subject: [PATCH 19/27] Update clone tests and add deep delete tests --- .../segments/test_unit_segments_models.py | 192 +++++++++++------- 1 file changed, 118 insertions(+), 74 deletions(-) diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 3e3200ed9dfd..071910cb5222 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -1,4 +1,5 @@ import pytest +from django.utils import timezone from flag_engine.segments.constants import EQUAL, PERCENTAGE_SPLIT from features.models import Feature @@ -183,61 +184,41 @@ def test_deep_clone_of_segment( feature=feature, ) - # Check that the versioning is correct, since we'll testing + # Check that the versioning is correct, since we'll be testing # against it later in the test. assert segment.version == 1 assert segment.version_of == segment - parent_rule1 = SegmentRule.objects.create( - segment=segment, type=SegmentRule.ALL_RULE - ) - parent_rule2 = SegmentRule.objects.create( - segment=segment, type=SegmentRule.ANY_RULE - ) + parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) child_rule1 = SegmentRule.objects.create( - rule=parent_rule1, type=SegmentRule.ANY_RULE + rule=parent_rule, type=SegmentRule.ANY_RULE ) child_rule2 = SegmentRule.objects.create( - rule=parent_rule1, type=SegmentRule.ALL_RULE + rule=parent_rule, type=SegmentRule.NONE_RULE ) child_rule3 = SegmentRule.objects.create( - rule=parent_rule2, type=SegmentRule.NONE_RULE + rule=parent_rule, type=SegmentRule.NONE_RULE ) child_rule4 = SegmentRule.objects.create( - rule=parent_rule2, type=SegmentRule.ALL_RULE + rule=parent_rule, type=SegmentRule.ANY_RULE ) condition1 = Condition.objects.create( - rule=parent_rule1, - property="parent_rule1", - operator=EQUAL, - value="condition1", - created_with_segment=True, - ) - condition2 = Condition.objects.create( - rule=parent_rule1, - property="parent_rule1", - operator=PERCENTAGE_SPLIT, - value="0.1", - created_with_segment=False, - ) - - condition3 = Condition.objects.create( rule=child_rule1, property="child_rule1", operator=EQUAL, value="condition3", created_with_segment=True, ) - Condition.objects.create( + condition2 = Condition.objects.create( rule=child_rule2, property="child_rule2", operator=PERCENTAGE_SPLIT, value="0.2", created_with_segment=False, ) - Condition.objects.create( + condition3 = Condition.objects.create( rule=child_rule2, property="child_rule2", operator=EQUAL, @@ -245,7 +226,7 @@ def test_deep_clone_of_segment( created_with_segment=False, ) - Condition.objects.create( + condition4 = Condition.objects.create( rule=child_rule3, property="child_rule3", operator=EQUAL, @@ -253,7 +234,7 @@ def test_deep_clone_of_segment( created_with_segment=False, ) - Condition.objects.create( + condition5 = Condition.objects.create( rule=child_rule4, property="child_rule4", operator=EQUAL, @@ -262,77 +243,82 @@ def test_deep_clone_of_segment( ) # When - new_segment = segment.deep_clone() + cloned_segment = segment.deep_clone() # Then - assert new_segment.name == segment.name - assert new_segment.description == segment.description - assert new_segment.project == project - assert new_segment.feature == feature - assert new_segment.version == 1 - assert new_segment.version_of == segment + assert cloned_segment.name == segment.name + assert cloned_segment.description == segment.description + assert cloned_segment.project == project + assert cloned_segment.feature == feature + assert cloned_segment.version == 1 + assert cloned_segment.version_of == segment assert segment.version == 2 - assert len(new_segment.rules.all()) == len(segment.rules.all()) == 2 - new_parent_rule1, new_parent_rule2 = list(new_segment.rules.all()) + assert len(cloned_segment.rules.all()) == len(segment.rules.all()) == 1 + new_parent_rule = cloned_segment.rules.first() - assert new_parent_rule1.segment == new_segment - assert new_parent_rule2.segment == new_segment - assert new_parent_rule1.type == parent_rule1.type - assert new_parent_rule2.type == parent_rule2.type + assert new_parent_rule.segment == cloned_segment + assert new_parent_rule.type == parent_rule.type - assert len(new_parent_rule1.rules.all()) == len(parent_rule1.rules.all()) == 2 - assert len(new_parent_rule2.rules.all()) == len(parent_rule2.rules.all()) == 2 - new_child_rule1, new_child_rule2 = list(new_parent_rule1.rules.all()) + assert len(new_parent_rule.rules.all()) == len(parent_rule.rules.all()) == 4 + new_child_rule1, new_child_rule2, new_child_rule3, new_child_rule4 = list( + new_parent_rule.rules.all() + ) assert new_child_rule1.type == child_rule1.type assert new_child_rule2.type == child_rule2.type - - new_child_rule3, new_child_rule4 = list(new_parent_rule2.rules.all()) - assert new_child_rule3.type == child_rule3.type assert new_child_rule4.type == child_rule4.type assert ( - len(new_parent_rule1.conditions.all()) - == len(parent_rule1.conditions.all()) - == 2 + len(new_parent_rule.conditions.all()) == len(parent_rule.conditions.all()) == 0 ) - new_condition1, new_condition2 = list(new_parent_rule1.conditions.all()) + assert ( + len(new_child_rule1.conditions.all()) == len(child_rule1.conditions.all()) == 1 + ) + new_condition1 = new_child_rule1.conditions.first() assert new_condition1.property == condition1.property assert new_condition1.operator == condition1.operator assert new_condition1.value == condition1.value assert new_condition1.created_with_segment is condition1.created_with_segment + assert ( + len(new_child_rule2.conditions.all()) == len(child_rule2.conditions.all()) == 2 + ) + new_condition2, new_condition3 = list(new_child_rule2.conditions.all()) + assert new_condition2.property == condition2.property assert new_condition2.operator == condition2.operator assert new_condition2.value == condition2.value assert new_condition2.created_with_segment is condition2.created_with_segment - assert ( - len(new_child_rule1.conditions.all()) == len(child_rule1.conditions.all()) == 1 - ) - new_condition3 = new_child_rule1.conditions.first() - assert new_condition3.property == condition3.property assert new_condition3.operator == condition3.operator assert new_condition3.value == condition3.value assert new_condition3.created_with_segment is condition3.created_with_segment - # Since the conditions have been asserted at multiple levels, - # we leave the rest to basic size assertions. - assert ( - len(new_child_rule2.conditions.all()) == len(child_rule2.conditions.all()) == 2 - ) assert ( len(new_child_rule3.conditions.all()) == len(child_rule3.conditions.all()) == 1 ) + new_condition4 = new_child_rule3.conditions.first() + + assert new_condition4.property == condition4.property + assert new_condition4.operator == condition4.operator + assert new_condition4.value == condition4.value + assert new_condition4.created_with_segment is condition4.created_with_segment + assert ( len(new_child_rule4.conditions.all()) == len(child_rule4.conditions.all()) == 1 ) + new_condition5 = new_child_rule4.conditions.first() + + assert new_condition5.property == condition5.property + assert new_condition5.operator == condition5.operator + assert new_condition5.value == condition5.value + assert new_condition5.created_with_segment is condition5.created_with_segment def test_manager_returns_only_highest_version_of_segments( @@ -343,24 +329,24 @@ def test_manager_returns_only_highest_version_of_segments( assert segment.version == 2 assert segment.version_of == segment - new_segment = segment.deep_clone() - assert new_segment.version == 2 + cloned_segment = segment.deep_clone() + assert cloned_segment.version == 2 assert segment.version == 3 # When - queryset1 = Segment.objects.filter(id=new_segment.id) - queryset2 = Segment.all_objects.filter(id=new_segment.id) + queryset1 = Segment.objects.filter(id=cloned_segment.id) + queryset2 = Segment.all_objects.filter(id=cloned_segment.id) queryset3 = Segment.objects.filter(id=segment.id) queryset4 = Segment.all_objects.filter(id=segment.id) # Then assert not queryset1.exists() - assert queryset2.first() == new_segment + assert queryset2.first() == cloned_segment assert queryset3.first() == segment assert queryset4.first() == segment -def test_deep_clone_of_segment_with_sub_rule( +def test_deep_clone_of_segment_with_improper_sub_rule( project: Project, feature: Feature, ) -> None: @@ -372,13 +358,14 @@ def test_deep_clone_of_segment_with_sub_rule( feature=feature, ) - sub_rule = SegmentRule.objects.create(type=SegmentRule.ALL_RULE) - - # Parent rule with invalid sub rule. - SegmentRule.objects.create( - segment=segment, type=SegmentRule.ALL_RULE, rule=sub_rule + rule = SegmentRule.objects.create( + type=SegmentRule.ALL_RULE, + segment=segment, ) + # Rule with invalid relation to both segment and rule. + SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE, rule=rule) + with pytest.raises(AssertionError) as exception: segment.deep_clone() @@ -413,3 +400,60 @@ def test_deep_clone_of_segment_with_grandchild_rule( assert ( "AssertionError: Expected two layers of rules, not more" == exception.exconly() ) + + +def test_deep_delete_hard(segment: Segment) -> None: + # Given + parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) + + child_rule = SegmentRule.objects.create(rule=parent_rule, type=SegmentRule.ANY_RULE) + + condition = Condition.objects.create( + rule=child_rule, + property="child_rule", + operator=EQUAL, + value="condition", + created_with_segment=True, + ) + + # When + segment.deep_delete(hard=True) + + # Then + with pytest.raises(Segment.DoesNotExist): + segment.refresh_from_db() + with pytest.raises(SegmentRule.DoesNotExist): + parent_rule.refresh_from_db() + with pytest.raises(SegmentRule.DoesNotExist): + child_rule.refresh_from_db() + with pytest.raises(Condition.DoesNotExist): + condition.refresh_from_db() + + +def test_deep_delete_soft(segment: Segment) -> None: + # Given + parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) + + child_rule = SegmentRule.objects.create(rule=parent_rule, type=SegmentRule.ANY_RULE) + + condition = Condition.objects.create( + rule=child_rule, + property="child_rule", + operator=EQUAL, + value="condition", + created_with_segment=True, + ) + before_delete = timezone.now() + + # When + segment.deep_delete(hard=False) + + # Then + segment.refresh_from_db() + assert segment.deleted_at > before_delete + parent_rule.refresh_from_db() + assert parent_rule.deleted_at > before_delete + child_rule.refresh_from_db() + assert child_rule.deleted_at > before_delete + condition.refresh_from_db() + assert condition.deleted_at > before_delete From 558bc40bdc0542147bacfbcbedf116458a3ad372 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Mon, 17 Jun 2024 16:02:51 +0000 Subject: [PATCH 20/27] Fix version of check and test for model existence during delete --- api/segments/models.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/api/segments/models.py b/api/segments/models.py index 0adf3aa903f2..5641e9dc29b4 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -87,7 +87,10 @@ def get_skip_create_audit_log(self) -> bool: if skip is not None: return skip - if self.version_of != self: + try: + if self.version_of and self.version_of != self: + return True + except Segment.DoesNotExist: return True return False From c76211d6f42435070d092c13d3dfae821e3f19fe Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 18 Jun 2024 15:26:57 +0000 Subject: [PATCH 21/27] Switch to existence check --- api/segments/models.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 5641e9dc29b4..310f98cdebd5 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -262,14 +262,8 @@ def deep_clone(self, cloned_segment: Segment) -> "SegmentRule": cloned_rule.id = None cloned_rule.save() - cloned_conditions = [] - for condition in self.conditions.all(): - cloned_condition = deepcopy(condition) - cloned_condition.uuid = uuid.uuid4() - cloned_condition.rule = cloned_rule - cloned_condition.id = None - cloned_conditions.append(cloned_condition) - Condition.objects.bulk_create(cloned_conditions) + # Conditions are only part of the sub-rules. + assert self.conditions.exists() is False for sub_rule in self.rules.all(): if sub_rule.rules.exists(): From 1eb9469bdb9b281e74eb9f9e9d7a6dca8771c0ae Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 18 Jun 2024 15:27:30 +0000 Subject: [PATCH 22/27] Add tests for skipping audit log --- .../segments/test_unit_segments_models.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 071910cb5222..4ba132d07698 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -457,3 +457,35 @@ def test_deep_delete_soft(segment: Segment) -> None: assert child_rule.deleted_at > before_delete condition.refresh_from_db() assert condition.deleted_at > before_delete + + +def test_segment_rule_get_skip_create_audit_log_when_doesnt_skip( + segment: Segment, +) -> None: + # Given + assert segment == segment.version_of + segment_rule = SegmentRule.objects.create( + segment=segment, type=SegmentRule.ALL_RULE + ) + + # When + result = segment_rule.get_skip_create_audit_log() + + # Then + assert result is False + + +def test_segment_rule_get_skip_create_audit_log_when_skips(segment: Segment) -> None: + # Given + cloned_segment = segment.deep_clone() + assert cloned_segment != cloned_segment.version_of + + segment_rule = SegmentRule.objects.create( + segment=cloned_segment, type=SegmentRule.ALL_RULE + ) + + # When + result = segment_rule.get_skip_create_audit_log() + + # Then + assert result is True From 7bb73848e22579fce6b550ffdc3a94d26f85dcae Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 18 Jun 2024 15:28:14 +0000 Subject: [PATCH 23/27] Add test for when an exception is thrown by the update --- .../unit/segments/test_unit_segments_views.py | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index e6a9192a4e87..a5b67952ecc7 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -9,6 +9,7 @@ from pytest_django import DjangoAssertNumQueries from pytest_django.fixtures import SettingsWrapper from pytest_lazyfixture import lazy_fixture +from pytest_mock import MockerFixture from rest_framework import status from rest_framework.test import APIClient @@ -638,6 +639,82 @@ def test_update_segment_versioned_segment( assert versioned_condition.property == existing_condition.property +def test_update_segment_versioned_segment_with_thrown_exception( + project: Project, + admin_client_new: APIClient, + segment: Segment, + segment_rule: SegmentRule, + mocker: MockerFixture, +) -> None: + # Given + url = reverse( + "api-v1:projects:project-segments-detail", args=[project.id, segment.id] + ) + nested_rule = SegmentRule.objects.create( + rule=segment_rule, type=SegmentRule.ANY_RULE + ) + existing_condition = Condition.objects.create( + rule=nested_rule, property="foo", operator=EQUAL, value="bar" + ) + + assert ( + segment.version == 2 == Segment.all_objects.filter(version_of=segment).count() + ) + + new_condition_property = "foo2" + new_condition_value = "bar" + data = { + "name": segment.name, + "project": project.id, + "rules": [ + { + "id": segment_rule.id, + "type": segment_rule.type, + "rules": [ + { + "id": nested_rule.id, + "type": nested_rule.type, + "rules": [], + "conditions": [ + { + "id": existing_condition.id, + "property": existing_condition.property, + "operator": existing_condition.operator, + "value": existing_condition.value, + }, + { + "property": new_condition_property, + "operator": EQUAL, + "value": new_condition_value, + }, + ], + } + ], + "conditions": [], + } + ], + } + + update_super_patch = mocker.patch( + "rest_framework.serializers.ModelSerializer.update" + ) + update_super_patch.side_effect = Exception("Mocked exception") + + # When + with pytest.raises(Exception): + admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then + segment.refresh_from_db() + + # Now verify that the version of the segment has not been changed. + assert ( + segment.version == 2 == Segment.all_objects.filter(version_of=segment).count() + ) + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], From 4481e4ba661a13e21514b530000e4b59dbdaecfc Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Fri, 21 Jun 2024 13:28:12 +0000 Subject: [PATCH 24/27] Create WIP code for Matt --- api/segments/models.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 310f98cdebd5..72de61c57c88 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -88,7 +88,7 @@ def get_skip_create_audit_log(self) -> bool: return skip try: - if self.version_of and self.version_of != self: + if self.version_of_id and self.version_of_id != self.id: return True except Segment.DoesNotExist: return True @@ -143,6 +143,14 @@ def set_version_of_to_self_if_none(self): segment_audit_log_helper.unset_skip_audit_log(self.id) def deep_delete(self, hard: bool) -> None: + # WIP: This is the updated code to reflect what we want. + if hard: + self.hard_delete() + else: + self.delete() + + return + # This is the code the ran with success. for rule in self.rules.all(): for child_rule in rule.rules.all(): for condition in child_rule.conditions.all(): @@ -236,7 +244,7 @@ def __str__(self): def get_skip_create_audit_log(self) -> bool: segment = self.get_segment() - return segment.version_of != segment + return segment.version_of_id != segment.id def get_segment(self): """ @@ -336,7 +344,7 @@ def __str__(self): def get_skip_create_audit_log(self) -> bool: segment = self.rule.get_segment() - return segment.version_of != segment + return segment.version_of_id != segment.id def get_update_log_message(self, history_instance) -> typing.Optional[str]: return f"Condition updated on segment '{self._get_segment().name}'." From 669a0797a1b3bf33905e7885effb10a27295d0e4 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Mon, 24 Jun 2024 13:55:45 +0100 Subject: [PATCH 25/27] Minor optimisation --- api/segments/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/segments/models.py b/api/segments/models.py index 72de61c57c88..34ae3e5a4476 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -254,7 +254,7 @@ def get_segment(self): TODO: denormalise the segment information so that we don't have to make multiple queries here in complex cases """ rule = self - while not rule.segment: + while not rule.segment_id: rule = rule.rule return rule.segment From 34e7e46fc767c314311bdc61b36fc7f9101e0e4b Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Mon, 24 Jun 2024 14:29:17 +0100 Subject: [PATCH 26/27] Handle exception generating audit log --- api/core/signals.py | 18 +++++- api/segments/models.py | 29 ---------- api/segments/serializers.py | 2 +- .../segments/test_unit_segments_models.py | 58 ------------------- 4 files changed, 18 insertions(+), 89 deletions(-) diff --git a/api/core/signals.py b/api/core/signals.py index a77a6c7d1da3..87645ae97f39 100644 --- a/api/core/signals.py +++ b/api/core/signals.py @@ -1,5 +1,8 @@ +import logging + from core.models import _AbstractBaseAuditableModel from django.conf import settings +from django.core.exceptions import ObjectDoesNotExist from django.utils import timezone from simple_history.models import HistoricalRecords @@ -7,6 +10,8 @@ from task_processor.task_run_method import TaskRunMethod from users.models import FFAdminUser +logger = logging.getLogger(__name__) + def create_audit_log_from_historical_record( instance: _AbstractBaseAuditableModel, @@ -30,7 +35,18 @@ def create_audit_log_from_historical_record( else None ) - environment, project = instance.get_environment_and_project() + try: + environment, project = instance.get_environment_and_project() + except ObjectDoesNotExist: + logger.warning( + "Unable to create audit log for %s %s. " + "Parent model does not exist - this likely means it was hard deleted.", + instance.related_object_type, + getattr(instance, "id", "uuid"), + exc_info=True, + ) + return + if project != history_instance.instance and ( (project and project.deleted_at) or (environment and environment.project.deleted_at) diff --git a/api/segments/models.py b/api/segments/models.py index 34ae3e5a4476..992f37ea7023 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -142,35 +142,6 @@ def set_version_of_to_self_if_none(self): self.save() segment_audit_log_helper.unset_skip_audit_log(self.id) - def deep_delete(self, hard: bool) -> None: - # WIP: This is the updated code to reflect what we want. - if hard: - self.hard_delete() - else: - self.delete() - - return - # This is the code the ran with success. - for rule in self.rules.all(): - for child_rule in rule.rules.all(): - for condition in child_rule.conditions.all(): - if hard: - condition.hard_delete() - else: - condition.delete() - if hard: - child_rule.hard_delete() - else: - child_rule.delete() - if hard: - rule.hard_delete() - else: - rule.delete() - if hard: - self.hard_delete() - else: - self.delete() - def deep_clone(self) -> "Segment": cloned_segment = deepcopy(self) cloned_segment.id = None diff --git a/api/segments/serializers.py b/api/segments/serializers.py index df580254bc21..322357038eef 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -103,7 +103,7 @@ def update(self, instance: Segment, validated_data: dict[str, typing.Any]) -> No instance.refresh_from_db() instance.version = cloned_segment.version instance.save() - cloned_segment.deep_delete(hard=True) + cloned_segment.hard_delete() raise return response diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 4ba132d07698..6f6d4d2556b3 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -1,5 +1,4 @@ import pytest -from django.utils import timezone from flag_engine.segments.constants import EQUAL, PERCENTAGE_SPLIT from features.models import Feature @@ -402,63 +401,6 @@ def test_deep_clone_of_segment_with_grandchild_rule( ) -def test_deep_delete_hard(segment: Segment) -> None: - # Given - parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) - - child_rule = SegmentRule.objects.create(rule=parent_rule, type=SegmentRule.ANY_RULE) - - condition = Condition.objects.create( - rule=child_rule, - property="child_rule", - operator=EQUAL, - value="condition", - created_with_segment=True, - ) - - # When - segment.deep_delete(hard=True) - - # Then - with pytest.raises(Segment.DoesNotExist): - segment.refresh_from_db() - with pytest.raises(SegmentRule.DoesNotExist): - parent_rule.refresh_from_db() - with pytest.raises(SegmentRule.DoesNotExist): - child_rule.refresh_from_db() - with pytest.raises(Condition.DoesNotExist): - condition.refresh_from_db() - - -def test_deep_delete_soft(segment: Segment) -> None: - # Given - parent_rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) - - child_rule = SegmentRule.objects.create(rule=parent_rule, type=SegmentRule.ANY_RULE) - - condition = Condition.objects.create( - rule=child_rule, - property="child_rule", - operator=EQUAL, - value="condition", - created_with_segment=True, - ) - before_delete = timezone.now() - - # When - segment.deep_delete(hard=False) - - # Then - segment.refresh_from_db() - assert segment.deleted_at > before_delete - parent_rule.refresh_from_db() - assert parent_rule.deleted_at > before_delete - child_rule.refresh_from_db() - assert child_rule.deleted_at > before_delete - condition.refresh_from_db() - assert condition.deleted_at > before_delete - - def test_segment_rule_get_skip_create_audit_log_when_doesnt_skip( segment: Segment, ) -> None: From 8f38dcd62ad5e52c6837832fe9642ebfbeab73c6 Mon Sep 17 00:00:00 2001 From: Zach Aysan Date: Tue, 25 Jun 2024 18:50:23 +0000 Subject: [PATCH 27/27] Add test for audit log on segment not existing --- .../segments/test_unit_segments_models.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index 6f6d4d2556b3..351a2fc7cf90 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -1,5 +1,8 @@ +from unittest.mock import PropertyMock + import pytest from flag_engine.segments.constants import EQUAL, PERCENTAGE_SPLIT +from pytest_mock import MockerFixture from features.models import Feature from projects.models import Project @@ -431,3 +434,20 @@ def test_segment_rule_get_skip_create_audit_log_when_skips(segment: Segment) -> # Then assert result is True + + +def test_segment_get_skip_create_audit_log_when_exception( + mocker: MockerFixture, + segment: Segment, +) -> None: + # Given + patched_segment = mocker.patch.object( + Segment, "version_of_id", new_callable=PropertyMock + ) + patched_segment.side_effect = Segment.DoesNotExist("Segment missing") + + # When + result = segment.get_skip_create_audit_log() + + # Then + assert result is True