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

fix: Long DELETE project call #3360

Merged
merged 19 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions api/core/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ def create_audit_log_from_historical_record(
else None
)

environment, project = history_instance.instance.get_environment_and_project()
if project != history_instance.instance and (
(project and project.deleted_at)
or (environment and environment.project.deleted_at)
):
# don't trigger audit log records in deleted projects
return
Comment on lines +33 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

This is arguably unrelated to this PR but I think it's a good improvement here to prevent unnecessary data in the AuditLog table.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's sorta unrelated but it's a good addition.


tasks.create_audit_log_from_historical_record.delay(
kwargs={
"history_instance_id": history_instance.history_id,
Expand Down
20 changes: 20 additions & 0 deletions api/environments/migrations/0034_alter_environment_project.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Generated by Django 3.2.23 on 2024-01-31 18:47

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('projects', '0021_add_identity_overrides_migration_status'),
('environments', '0033_add_environment_feature_state_version_logic'),
]

operations = [
migrations.AlterField(
Copy link
Contributor

Choose a reason for hiding this comment

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

sqlmigrate output:

❯ python manage.py sqlmigrate environments 0034
BEGIN;
--
-- Alter field project on environment
--
COMMIT;

Copy link
Contributor

Choose a reason for hiding this comment

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

Django migrations for this silly stuff is always a bit of an eye roll

model_name='environment',
name='project',
field=models.ForeignKey(help_text='Changing the project selected will remove all previous Feature States for the previously associated projects Features that are related to this Environment. New default Feature States will be created for the new selected projects Features for this Environment.', on_delete=django.db.models.deletion.DO_NOTHING, related_name='environments', to='projects.project'),
),
]
4 changes: 3 additions & 1 deletion api/environments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ class Environment(
" Environment. New default Feature States will be created for the new"
" selected projects Features for this Environment."
),
on_delete=models.CASCADE,
# Cascade deletes are decouple from the Django ORM. See this PR for details.
# https://github.com/Flagsmith/flagsmith/pull/3360/
on_delete=models.DO_NOTHING,
)

api_key = models.CharField(
Expand Down
5 changes: 5 additions & 0 deletions api/environments/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,8 @@ def delete_environment_from_dynamo(api_key: str, environment_id: str):

# Delete environment_v2 documents
environment_v2_wrapper.delete_environment(environment_id)


@register_task_handler()
def delete_environment(environment_id: int) -> None:
Environment.objects.get(id=environment_id).delete()
1 change: 1 addition & 0 deletions api/features/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ def ready(self):

# noinspection PyUnresolvedReferences
import features.signals # noqa
import features.tasks # noqa
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Generated by Django 3.2.23 on 2024-02-01 12:12

from django.db import migrations, models
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the sqlmigrate output for these migrations

Copy link
Contributor

Choose a reason for hiding this comment

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

❯ python manage.py sqlmigrate features 0063
BEGIN;
--
-- Alter field project on feature
--
COMMIT;
❯ python manage.py sqlmigrate features 0064
BEGIN;
--
-- Alter field project on feature
--
--
-- Alter field project on historicalfeature
--
COMMIT;

import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('projects', '0021_add_identity_overrides_migration_status'),
('features', '0062_alter_feature_segment_unique_together'),
]

operations = [
migrations.AlterField(
model_name='feature',
name='project',
field=models.ForeignKey(help_text='Changing the project selected will remove previous Feature States for the previouslyassociated projects Environments that are related to this Feature. New default Feature States will be created for the new selected projects Environments for this Feature. Also this will remove any Tags associated with a feature as Tags are Project defined', on_delete=django.db.models.deletion.DO_NOTHING, related_name='features', to='projects.project'),
),
]
25 changes: 25 additions & 0 deletions api/features/migrations/0064_fix_feature_help_text_typo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Generated by Django 3.2.23 on 2024-02-02 10:10

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('projects', '0021_add_identity_overrides_migration_status'),
('features', '0063_detach_feature_from_project_cascade_delete'),
]

operations = [
migrations.AlterField(
model_name='feature',
name='project',
field=models.ForeignKey(help_text='Changing the project selected will remove previous Feature States for the previously associated projects Environments that are related to this Feature. New default Feature States will be created for the new selected projects Environments for this Feature. Also this will remove any Tags associated with a feature as Tags are Project defined', on_delete=django.db.models.deletion.DO_NOTHING, related_name='features', to='projects.project'),
),
migrations.AlterField(
model_name='historicalfeature',
name='project',
field=models.ForeignKey(blank=True, db_constraint=False, help_text='Changing the project selected will remove previous Feature States for the previously associated projects Environments that are related to this Feature. New default Feature States will be created for the new selected projects Environments for this Feature. Also this will remove any Tags associated with a feature as Tags are Project defined', null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='projects.project'),
),
]
6 changes: 4 additions & 2 deletions api/features/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,14 @@ class Feature(
Project,
related_name="features",
help_text=_(
"Changing the project selected will remove previous Feature States for the previously"
"Changing the project selected will remove previous Feature States for the previously "
"associated projects Environments that are related to this Feature. New default "
"Feature States will be created for the new selected projects Environments for this "
"Feature. Also this will remove any Tags associated with a feature as Tags are Project defined"
),
on_delete=models.CASCADE,
# Cascade deletes are decouple from the Django ORM. See this PR for details.
# https://github.com/Flagsmith/flagsmith/pull/3360/
on_delete=models.DO_NOTHING,
)
initial_value = models.CharField(
max_length=20000, null=True, default=None, blank=True
Expand Down
8 changes: 7 additions & 1 deletion api/features/tasks.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from environments.models import Webhook
from features.models import FeatureState
from features.models import Feature, FeatureState
from task_processor.decorators import register_task_handler
from webhooks.constants import WEBHOOK_DATETIME_FORMAT
from webhooks.webhooks import (
WebhookEventType,
Expand Down Expand Up @@ -79,3 +80,8 @@ def _get_feature_state_webhook_data(feature_state, previous=False):
identity_identifier=getattr(feature_state.identity, "identifier", None),
feature_segment=feature_state.feature_segment,
)


@register_task_handler()
def delete_feature(feature_id: int) -> None:
Feature.objects.get(pk=feature_id).delete()
5 changes: 5 additions & 0 deletions api/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
)
from projects.managers import ProjectManager
from projects.tasks import (
handle_cascade_delete,
migrate_project_environments_to_v2,
write_environments_to_dynamodb,
)
Expand Down Expand Up @@ -158,6 +159,10 @@ def write_to_dynamo(self):
def clean_up_dynamo(self):
DynamoProjectMetadata(self.id).delete()

@hook(AFTER_DELETE)
def handle_cascade_delete(self) -> None:
handle_cascade_delete.delay(kwargs={"project_id": self.id})

@property
def is_edge_project_by_default(self) -> bool:
return bool(
Expand Down
26 changes: 26 additions & 0 deletions api/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,29 @@ def migrate_project_environments_to_v2(project_id: int) -> None:
IdentityOverridesV2MigrationStatus.COMPLETE
)
project.save()


@register_task_handler()
def handle_cascade_delete(project_id: int) -> None:
"""
Due to the combination of soft deletes and audit log functionality,
cascade deletes for a project can take a long time for large projects.
This task delegates the cascade delete to tasks for the main related
entities for a project.
"""

from environments.tasks import delete_environment
from features.tasks import delete_feature
from projects.models import Project
from segments.tasks import delete_segment
Comment on lines +36 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the local import for the Project import, but I'm surprised that the tasks were necessary to import this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, yeah, I tried to resolve this but I ended up just diving into a hell hole of circular dependencies. It annoyingly made testing a little harder because I can't mock the imports, but I do feel like it's not a major issue for tasks to have local imports given their place in the stack.


project = Project.objects.all_with_deleted().get(id=project_id)

for environment_id in project.environments.values_list("id", flat=True):
delete_environment.delay(kwargs={"environment_id": environment_id})

for segment_id in project.segments.values_list("id", flat=True):
delete_segment.delay(kwargs={"segment_id": segment_id})

for feature_id in project.features.values_list("id", flat=True):
delete_feature.delay(kwargs={"feature_id": feature_id})
5 changes: 5 additions & 0 deletions api/segments/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,8 @@

class SegmentsConfig(BaseAppConfig):
name = "segments"

def ready(self) -> None:
super().ready()

import segments.tasks # noqa
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Generated by Django 3.2.23 on 2024-02-01 13:45

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('projects', '0021_add_identity_overrides_migration_status'),
('segments', '0019_add_audit_to_condition'),
]

operations = [
migrations.AlterField(
Copy link
Contributor

Choose a reason for hiding this comment

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

sqlmigrate output:

❯ python manage.py sqlmigrate segments 0020
BEGIN;
--
-- Alter field project on segment
--
COMMIT;

model_name='segment',
name='project',
field=models.ForeignKey(on_delete=django.db.models.deletion.DO_NOTHING, related_name='segments', to='projects.project'),
),
]
6 changes: 5 additions & 1 deletion api/segments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ class Segment(
name = models.CharField(max_length=2000)
description = models.TextField(null=True, blank=True)
project = models.ForeignKey(
Project, on_delete=models.CASCADE, related_name="segments"
Project,
# Cascade deletes are decouple from the Django ORM. See this PR for details.
# https://github.com/Flagsmith/flagsmith/pull/3360/
on_delete=models.DO_NOTHING,
related_name="segments",
)
feature = models.ForeignKey(
Feature, on_delete=models.CASCADE, related_name="segments", null=True
Expand Down
7 changes: 7 additions & 0 deletions api/segments/tasks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from segments.models import Segment
from task_processor.decorators import register_task_handler


@register_task_handler()
def delete_segment(segment_id: int) -> None:
Segment.objects.get(pk=segment_id).delete()
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
import pytest
from pytest_django.fixtures import SettingsWrapper
from pytest_mock import MockerFixture

from environments.dynamodb.types import IdentityOverridesV2Changeset
from environments.models import Environment
from features.models import Feature
from projects.models import IdentityOverridesV2MigrationStatus, Project
from projects.tasks import migrate_project_environments_to_v2
from projects.tasks import (
handle_cascade_delete,
migrate_project_environments_to_v2,
)
from segments.models import Segment
from task_processor.task_run_method import TaskRunMethod


@pytest.fixture
Expand Down Expand Up @@ -88,3 +96,22 @@ def test_migrate_project_environments_to_v2__expected_status_on_error(
assert project_v2_migration_in_progress.identity_overrides_v2_migration_status == (
IdentityOverridesV2MigrationStatus.IN_PROGRESS
)


def test_handle_cascade_delete(
project: Project,
environment: Environment,
feature: Feature,
segment: Segment,
settings: SettingsWrapper,
) -> None:
# Given
settings.TASK_RUN_METHOD = TaskRunMethod.SYNCHRONOUSLY

# When
handle_cascade_delete(project_id=project.id)

# Then
assert not Environment.objects.filter(id=environment.id).exists()
assert not Feature.objects.filter(id=feature.id).exists()
assert not Segment.objects.filter(id=segment.id).exists()
25 changes: 25 additions & 0 deletions api/tests/unit/projects/test_unit_projects_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
import pytest
from django.urls import reverse
from django.utils import timezone
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

Expand All @@ -30,6 +32,7 @@
VIEW_PROJECT,
)
from segments.models import Segment
from task_processor.task_run_method import TaskRunMethod
from users.models import FFAdminUser, UserPermissionGroup

now = timezone.now()
Expand Down Expand Up @@ -783,3 +786,25 @@ def test_get_project_data_by_id(
assert response_json["total_features"] == num_features
assert response_json["total_segments"] == num_segments
assert response_json["show_edge_identity_overrides_for_feature"] is False


def test_delete_project_delete_handles_cascade_delete(
admin_client: APIClient,
project: Project,
mocker: MockerFixture,
settings: SettingsWrapper,
) -> None:
# Given
settings.TASK_RUN_METHOD = TaskRunMethod.SYNCHRONOUSLY

url = reverse("api-v1:projects:project-detail", args=[project.id])
mocked_handle_cascade_delete = mocker.patch("projects.models.handle_cascade_delete")

# When
response = admin_client.delete(url)

# Then
assert response.status_code == status.HTTP_204_NO_CONTENT
mocked_handle_cascade_delete.delay.assert_called_once_with(
kwargs={"project_id": project.id}
)
Original file line number Diff line number Diff line change
@@ -1,15 +1,29 @@
from datetime import timedelta
from datetime import datetime, time, timedelta
from decimal import Decimal

import pytest
from django.utils import timezone
from freezegun import freeze_time
from pytest_lazyfixture import lazy_fixture

from task_processor.decorators import register_task_handler
from task_processor.models import RecurringTask, Task

now = timezone.now()
one_hour_ago = now - timedelta(hours=1)
one_hour_from_now = now + timedelta(hours=1)

@pytest.fixture
def current_datetime() -> datetime:
with freeze_time("2024-01-31T09:45:16.758115"):
yield timezone.now()


@pytest.fixture
def one_hour_ago(current_datetime: datetime) -> time:
return (current_datetime - timedelta(hours=1)).time()


@pytest.fixture
def one_hour_from_now(current_datetime: datetime) -> time:
return (current_datetime + timedelta(hours=1)).time()


@register_task_handler()
Expand Down Expand Up @@ -50,7 +64,7 @@ def test_serialize_data_handles_decimal_objects(input, expected_output):

@pytest.mark.parametrize(
"first_run_time, expected",
((one_hour_ago.time(), True), (one_hour_from_now.time(), False)),
((lazy_fixture("one_hour_ago"), True), (lazy_fixture("one_hour_from_now"), False)),
)
def test_recurring_task_run_should_execute_first_run_at(first_run_time, expected):
assert (
Expand Down
Loading