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 10 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
4 changes: 3 additions & 1 deletion .github/workflows/api-pull-request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ jobs:
run: make django-make-migrations

- name: Restore cached testmon data
if: ${{ github.event_name == 'pull_request' }}
if:
${{ github.event_name == 'pull_request' && !contains(github.event.pull_request.labels.*.name, 'skip-testmon')
}}
id: cache-testmon-restore
uses: actions/cache/restore@v3
with:
Expand Down
15 changes: 10 additions & 5 deletions api/audit/signals.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging

from django.conf import settings
from django.db.models.signals import post_save
from django.dispatch import receiver

Expand All @@ -16,8 +17,6 @@

@receiver(post_save, sender=AuditLog)
def call_webhooks(sender, instance, **kwargs):
data = AuditLogListSerializer(instance=instance).data

if not (instance.project or instance.environment):
logger.warning("Audit log without project or environment. Not sending webhook.")
return
Expand All @@ -27,9 +26,15 @@ def call_webhooks(sender, instance, **kwargs):
if instance.project
else instance.environment.project.organisation
)
call_organisation_webhooks.delay(
args=(organisation.id, data, WebhookEventType.AUDIT_LOG_CREATED.value)
)

if (
not settings.DISABLE_WEBHOOKS
and organisation.webhooks.filter(enabled=True).exists()
):
data = AuditLogListSerializer(instance=instance).data
call_organisation_webhooks.delay(
args=(organisation.id, data, WebhookEventType.AUDIT_LOG_CREATED.value)
)


def _get_integration_config(instance, integration_name):
Expand Down
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'),
),
]
9 changes: 8 additions & 1 deletion api/environments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,14 @@ class Environment(
" Environment. New default Feature States will be created for the new"
" selected projects Features for this Environment."
),
on_delete=models.CASCADE,
# Deleting large environments take significant amount of queries due to
# audit log signals and lifecycle events for related objects.
# For now, decouple environment deletion from project deletion
# to be able to perform it asynchronously.
#
# We choose `DO_NOTHING` here as we're only taking project soft deletes
# into account
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'),
),
]
2 changes: 1 addition & 1 deletion api/features/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class Feature(
"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,
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})
3 changes: 0 additions & 3 deletions api/projects/views.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.conf import settings
from django.utils.decorators import method_decorator
from drf_yasg import openapi
Expand Down
3 changes: 3 additions & 0 deletions api/segments/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@

class SegmentsConfig(BaseAppConfig):
name = "segments"

def ready(self) -> None:
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'),
),
]
2 changes: 1 addition & 1 deletion api/segments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ 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, 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