From 2fb487be547bfd26a3c2faa74fc04693fd71b84b Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 24 Jul 2024 16:28:40 +0100 Subject: [PATCH 1/4] Handle more scenarios for priority setting --- api/features/feature_segments/serializers.py | 43 +++++- .../test_unit_feature_segments_serializers.py | 139 +++++++++++++++++- 2 files changed, 175 insertions(+), 7 deletions(-) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index e7aa85afe5ce..ddbfd79f83f8 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -46,18 +46,49 @@ class CustomCreateSegmentOverrideFeatureSegmentSerializer( ): # Since the `priority` field on the FeatureSegment model is set to editable=False # (to adhere to the django-ordered-model functionality), we redefine the priority - # field here, and use it manually in the save method. + # field here, and use it manually in the create / update methods. priority = serializers.IntegerField(min_value=0, required=False) - def save(self, **kwargs: typing.Any) -> FeatureSegment: - priority: int | None = self.initial_data.pop("priority", None) + def create(self, validated_data: dict[str, typing.Any]) -> FeatureSegment: + priority: int | None = validated_data.get("priority", None) - feature_segment: FeatureSegment = super().save(**kwargs) + if ( + priority is not None + and ( + collision := FeatureSegment.objects.filter( + environment=validated_data["environment"], + feature=validated_data["feature"], + priority=priority, + ).first() + ) + is not None + ): + # Since there is no unique clause on the priority field, if a priority + # is set, it will just save the feature segment and not move others + # down. We can't just move the created feature segment like we do in + # update() because the to() method just reads that it's already at the + # given priority and early returns. + collision.to(priority + 1) + + feature_segment = super().create(validated_data) + + if priority is None: + feature_segment.bottom() + + return feature_segment + + def update( + self, instance: FeatureSegment, validated_data: dict[str, typing.Any] + ) -> FeatureSegment: + priority: int | None = validated_data.pop("priority", None) + + feature_segment = super().update(instance, validated_data) - if priority: + if priority is not None: + # Note that 0 is a valid priority feature_segment.to(priority) else: - feature_segment.bottom(priority) + feature_segment.bottom() return feature_segment diff --git a/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py b/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py index e710a70197af..be219cb89eb0 100644 --- a/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py +++ b/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py @@ -61,7 +61,7 @@ def test_feature_segment_change_priorities_serializer_validate_fails_if_non_uniq assert serializer.errors -def test_feature_segment_serializer_save_sets_lowest_priority_if_none_given( +def test_feature_segment_serializer_save_new_feature_segment_sets_lowest_priority_if_none_given( feature: Feature, segment_featurestate: FeatureState, feature_segment: FeatureSegment, @@ -78,5 +78,142 @@ def test_feature_segment_serializer_save_sets_lowest_priority_if_none_given( new_feature_segment = serializer.save(feature=feature, environment=environment) # Then + feature_segment.refresh_from_db() assert feature_segment.priority == 0 assert new_feature_segment.priority == 1 + + +def test_feature_segment_serializer_save_new_feature_segment_sets_highest_priority_if_0_given( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, +) -> None: + # Given + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + data={"segment": another_segment.id, "priority": 0} + ) + serializer.is_valid(raise_exception=True) + + # When + new_feature_segment = serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 1 + assert new_feature_segment.priority == 0 + + +def test_feature_segment_serializer_save_new_feature_segment_moves_others_if_needed( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, +) -> None: + # Given + feature_segment.to(1) + + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + data={"segment": another_segment.id, "priority": 1} + ) + serializer.is_valid(raise_exception=True) + + # When + new_feature_segment = serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 2 + assert new_feature_segment.priority == 1 + + +def test_feature_segment_serializer_save_existing_feature_segment_does_nothing_if_no_priority_given( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, +) -> None: + # Given + feature_segment_to_update = FeatureSegment.objects.create( + environment=environment, + feature=feature, + segment=another_segment, + priority=1, + ) + + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + instance=feature_segment_to_update, data={"segment": another_segment.id} + ) + serializer.is_valid(raise_exception=True) + + # When + updated_feature_segment = serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 0 + assert updated_feature_segment.priority == 1 + + +def test_feature_segment_serializer_save_existing_feature_segment_sets_highest_priority_if_0_given( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, +) -> None: + # Given + feature_segment_to_update = FeatureSegment.objects.create( + environment=environment, + feature=feature, + segment=another_segment, + priority=1, + ) + + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + instance=feature_segment_to_update, + data={"segment": another_segment.id, "priority": 0}, + ) + serializer.is_valid(raise_exception=True) + + # When + updated_feature_segment = serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 1 + assert updated_feature_segment.priority == 0 + + +def test_feature_segment_serializer_save_existing_feature_segment_moves_others_if_needed( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, +) -> None: + # Given + feature_segment.to(1) + + feature_segment_to_update = FeatureSegment.objects.create( + environment=environment, + feature=feature, + segment=another_segment, + ) + + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + instance=feature_segment_to_update, + data={"segment": another_segment.id, "priority": 1}, + ) + serializer.is_valid(raise_exception=True) + + # When + updated_feature_segment = serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 2 + assert updated_feature_segment.priority == 1 From 1931a5b732633b6c572279fde8febdba037fda29 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 24 Jul 2024 16:38:09 +0100 Subject: [PATCH 2/4] Simplify `create` --- api/features/feature_segments/serializers.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index ddbfd79f83f8..92ba037d0eed 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -70,12 +70,7 @@ def create(self, validated_data: dict[str, typing.Any]) -> FeatureSegment: # given priority and early returns. collision.to(priority + 1) - feature_segment = super().create(validated_data) - - if priority is None: - feature_segment.bottom() - - return feature_segment + return super().create(validated_data) def update( self, instance: FeatureSegment, validated_data: dict[str, typing.Any] From 3f1fb19e1bd49ba73eecccd371ed3158dc97d96f Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 24 Jul 2024 17:48:51 +0100 Subject: [PATCH 3/4] Update to always just move any conflict out of the way instead of operating on current segment --- api/features/feature_segments/serializers.py | 41 +++++++------------ .../test_unit_feature_segments_serializers.py | 35 ++++++++++++++++ 2 files changed, 50 insertions(+), 26 deletions(-) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 92ba037d0eed..58323f60af3d 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -1,8 +1,7 @@ -import typing - from common.features.serializers import ( CreateSegmentOverrideFeatureSegmentSerializer, ) +from django.db import transaction from rest_framework import serializers from rest_framework.exceptions import PermissionDenied @@ -46,18 +45,25 @@ class CustomCreateSegmentOverrideFeatureSegmentSerializer( ): # Since the `priority` field on the FeatureSegment model is set to editable=False # (to adhere to the django-ordered-model functionality), we redefine the priority - # field here, and use it manually in the create / update methods. + # field here, and use it manually in the save method. priority = serializers.IntegerField(min_value=0, required=False) - def create(self, validated_data: dict[str, typing.Any]) -> FeatureSegment: - priority: int | None = validated_data.get("priority", None) + @transaction.atomic() + def save(self, **kwargs) -> FeatureSegment: + """ + Note that this method is marked as atomic since a lot of additional validation is + performed in the call to super. If that fails, we want to roll the changes made by + `collision.to` back. + """ + + priority: int | None = self.validated_data.get("priority", None) if ( priority is not None and ( collision := FeatureSegment.objects.filter( - environment=validated_data["environment"], - feature=validated_data["feature"], + environment_id=kwargs["environment"], + feature=kwargs["feature"], priority=priority, ).first() ) @@ -65,27 +71,10 @@ def create(self, validated_data: dict[str, typing.Any]) -> FeatureSegment: ): # Since there is no unique clause on the priority field, if a priority # is set, it will just save the feature segment and not move others - # down. We can't just move the created feature segment like we do in - # update() because the to() method just reads that it's already at the - # given priority and early returns. + # down. This ensures that the incoming priority space is 'free'. collision.to(priority + 1) - return super().create(validated_data) - - def update( - self, instance: FeatureSegment, validated_data: dict[str, typing.Any] - ) -> FeatureSegment: - priority: int | None = validated_data.pop("priority", None) - - feature_segment = super().update(instance, validated_data) - - if priority is not None: - # Note that 0 is a valid priority - feature_segment.to(priority) - else: - feature_segment.bottom() - - return feature_segment + return super().save(**kwargs) class FeatureSegmentQuerySerializer(serializers.Serializer): diff --git a/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py b/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py index be219cb89eb0..848783dcfb2e 100644 --- a/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py +++ b/api/tests/unit/features/feature_segments/test_unit_feature_segments_serializers.py @@ -1,5 +1,8 @@ from unittest.mock import MagicMock +import pytest +from pytest_mock import MockerFixture + from environments.models import Environment from features.feature_segments.serializers import ( CustomCreateSegmentOverrideFeatureSegmentSerializer, @@ -217,3 +220,35 @@ def test_feature_segment_serializer_save_existing_feature_segment_moves_others_i feature_segment.refresh_from_db() assert feature_segment.priority == 2 assert updated_feature_segment.priority == 1 + + +def test_feature_segment_serializer_save_new_feature_segment_does_nothing_on_error( + feature: Feature, + segment_featurestate: FeatureState, + feature_segment: FeatureSegment, + another_segment: Segment, + environment: Environment, + mocker: MockerFixture, +) -> None: + # Given + feature_segment.to(1) + serializer = CustomCreateSegmentOverrideFeatureSegmentSerializer( + data={"segment": another_segment.id, "priority": 1}, + ) + serializer.is_valid(raise_exception=True) + + class MockException(Exception): + pass + + mocker.patch( + "features.feature_segments.serializers.CustomCreateSegmentOverrideFeatureSegmentSerializer.create", + side_effect=MockException, + ) + + # When + with pytest.raises(MockException): + serializer.save(feature=feature, environment=environment) + + # Then + feature_segment.refresh_from_db() + assert feature_segment.priority == 1 From 33c6a96087828c5c54c33f25bd9e98341c85affa Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 24 Jul 2024 18:10:09 +0100 Subject: [PATCH 4/4] Fix missing environment_feature_version --- api/features/feature_segments/serializers.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/api/features/feature_segments/serializers.py b/api/features/feature_segments/serializers.py index 58323f60af3d..9052eac8264d 100644 --- a/api/features/feature_segments/serializers.py +++ b/api/features/feature_segments/serializers.py @@ -58,12 +58,20 @@ def save(self, **kwargs) -> FeatureSegment: priority: int | None = self.validated_data.get("priority", None) + if kwargs["environment"].use_v2_feature_versioning: # pragma: no cover + assert ( + kwargs["environment_feature_version"] is not None + ), "Must provide environment_feature_version for environment using v2 versioning" + if ( priority is not None and ( collision := FeatureSegment.objects.filter( - environment_id=kwargs["environment"], + environment=kwargs["environment"], feature=kwargs["feature"], + environment_feature_version=kwargs.get( + "environment_feature_version" + ), priority=priority, ).first() )