Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Create versioning for segments #4138

Merged
merged 31 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
2ad4381
Create versioning for segments migration
zachaysan Jun 10, 2024
f6746e1
Create versioning of segments
zachaysan Jun 10, 2024
8df7f91
Create segments manager for versioned segments
zachaysan Jun 10, 2024
a582253
Switch to nullable integer
zachaysan Jun 10, 2024
730d064
Deep clone the default segment fixture
zachaysan Jun 10, 2024
42f93a9
Deep clone during update
zachaysan Jun 10, 2024
c7eb529
Create default for newly created segments
zachaysan Jun 10, 2024
f06cb3d
Test deep cloning of segments
zachaysan Jun 10, 2024
641c02d
Update unit segments views
zachaysan Jun 10, 2024
d40d5ed
Merge branch 'main' into feat/create_versioning_for_segments
zachaysan Jun 10, 2024
7341357
Update audit log tests for segments versioning
zachaysan Jun 10, 2024
41f086d
Switch to assertion error, since it's closer to intent
zachaysan Jun 10, 2024
ed56775
Add tests for edge cases
zachaysan Jun 10, 2024
922001d
Remove historical from sql migration
zachaysan Jun 11, 2024
4842576
Switch to deepcopy for deep cloned objects
zachaysan Jun 11, 2024
b6bbf72
Update to copying from instances and add in skips for audit log
zachaysan Jun 17, 2024
8ed85c4
Add try and except block to gaurd model updates
zachaysan Jun 17, 2024
a37def6
Create audit log helper
zachaysan Jun 17, 2024
934d2cd
Update audit log tests and add view test for versioned segments
zachaysan Jun 17, 2024
17b2c9d
Update clone tests and add deep delete tests
zachaysan Jun 17, 2024
558bc40
Fix version of check and test for model existence during delete
zachaysan Jun 17, 2024
c76211d
Switch to existence check
zachaysan Jun 18, 2024
1eb9469
Add tests for skipping audit log
zachaysan Jun 18, 2024
7bb7384
Add test for when an exception is thrown by the update
zachaysan Jun 18, 2024
ec6f40b
Merge branch 'main' into feat/create_versioning_for_segments
zachaysan Jun 18, 2024
4481e4b
Create WIP code for Matt
zachaysan Jun 21, 2024
669a079
Minor optimisation
matthewelwell Jun 24, 2024
34e7e46
Handle exception generating audit log
matthewelwell Jun 24, 2024
8f38dcd
Add test for audit log on segment not existing
zachaysan Jun 25, 2024
e5c7b47
Fix conflicts and merge branch 'main' into feat/create_versioning_for…
zachaysan Jun 25, 2024
70e974a
Merge branch 'main' into feat/create_versioning_for_segments
zachaysan Jun 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions api/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
11 changes: 11 additions & 0 deletions api/segments/managers.py
Original file line number Diff line number Diff line change
@@ -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"))
58 changes: 58 additions & 0 deletions api/segments/migrations/0023_add_versioning_to_segments.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Generated by Django 3.2.25 on 2024-06-10 15:31
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(null=True),
),
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(null=True),
),
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,
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
UPDATE segments_segment SET version_of_id = id;
UPDATE segments_historicalsegment SET version_of_id = id;
112 changes: 112 additions & 0 deletions api/segments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,20 @@
from copy import deepcopy

from core.models import (
SoftDeleteExportableManager,
SoftDeleteExportableModel,
abstract_base_auditable_model_factory,
)
from django.conf import settings
from django.contrib.contenttypes.fields import GenericRelation
from django.core.exceptions import ValidationError
from django.db import models
from django_lifecycle import (
AFTER_CREATE,
BEFORE_CREATE,
LifecycleModelMixin,
hook,
)
from flag_engine.segments import constants

from audit.constants import (
Expand All @@ -22,10 +29,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"]),
):
Expand All @@ -45,8 +55,25 @@ class Segment(
Feature, on_delete=models.CASCADE, related_name="segments", null=True
)

# 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,
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

Expand Down Expand Up @@ -84,6 +111,46 @@ 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):
"""
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,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have clone functions in other models in the code base where we take a slightly different approach, e.g.:

def deep_clone(self) -> "Segment":
    new_segment = copy.deepcopy(self)

    new_segment.id = None  # this is what forces django to think it's creating a new model object

    ...

The benefit of this approach is that it maintains itself when new fields are added to those model classes. With the approach that you're using here, if a new field is added to the segment, we'll need to remember to add it here I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll implement that strategy. Just to double check, the timestamps will still be set to now for the new model, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this is done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check, the timestamps will still be set to now for the new model, right?

Good question - I don't know. We should verify in a test. Although I guess the question is whether we want that to be the case. Since we're backfilling the history, wouldn't we want e.g. the created_at not to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it turns out that none of the models involved in this PR have created_at or updated_at timestamps, although they probably should, at least on the segments. Should I add them to this PR? If so, what should the default be for existing objects None or now()? Should I include conditions and rules in having timestamps as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add these fields to all models in the segments app, yes, but let's do it in a separate PR. Regarding the default, we could be smart and look in the objects history but that might end up being a fairly mammoth migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good I've made a new ticket for this.

#4211

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is up for the timestamps:

#4236


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

Expand Down Expand Up @@ -140,6 +207,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():
assert False, "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"])
Expand Down
6 changes: 5 additions & 1 deletion api/segments/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions api/tests/integration/audit/test_audit_logs.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down Expand Up @@ -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"]
Expand Down
Loading
Loading