Skip to content
Open
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("experimentation", "0002_add_config_and_created_status"),
]

operations = [
migrations.RemoveConstraint(
model_name="warehouseconnection",
name="unique_active_warehouse_per_type_and_env",
),
migrations.AddConstraint(
model_name="warehouseconnection",
constraint=models.UniqueConstraint(
condition=models.Q(("deleted_at__isnull", True)),
fields=("environment",),
name="unique_active_warehouse_per_env",
),
),
Comment on lines +15 to +22
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This migration will fail if any environment currently has multiple active warehouse connections of different types. Since the previous constraint (unique_active_warehouse_per_type_and_env) allowed one connection per type, an environment could have both a Snowflake and a Flagsmith connection active. You should include a data migration to resolve these duplicates (e.g., by soft-deleting all but the most recent one) before applying this new constraint.

]
4 changes: 2 additions & 2 deletions api/experimentation/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class WarehouseConnection(LifecycleModelMixin, SoftDeleteExportableModel): # ty
class Meta:
constraints = [
models.UniqueConstraint(
fields=["warehouse_type", "environment"],
fields=["environment"],
condition=models.Q(deleted_at__isnull=True),
name="unique_active_warehouse_per_type_and_env",
name="unique_active_warehouse_per_env",
),
]
3 changes: 2 additions & 1 deletion api/experimentation/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,14 @@ def create(
WarehouseConnection.objects.all_with_deleted()
.filter(
environment=environment,
warehouse_type=warehouse_type,
deleted_at__isnull=False,
)
.order_by("-deleted_at")
.first()
)
if existing:
existing.deleted_at = None
existing.warehouse_type = warehouse_type
Comment on lines 57 to +65
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Resurrecting a soft-deleted connection and changing its warehouse_type is risky. If the connection's ID is referenced by other models (e.g., experiments), those references will now point to a connection of a different type, leading to data inconsistency. Since the new unique constraint only applies to active records (deleted_at__isnull=True), it is safer to only resurrect records that match the requested warehouse_type and create a new record otherwise.

Suggested change
environment=environment,
warehouse_type=warehouse_type,
deleted_at__isnull=False,
)
.order_by("-deleted_at")
.first()
)
if existing:
existing.deleted_at = None
existing.warehouse_type = warehouse_type
environment=environment,
warehouse_type=warehouse_type,
deleted_at__isnull=False,
)
.order_by("-deleted_at")
.first()
)
if existing:
existing.deleted_at = None

existing.status = WarehouseConnectionStatus.CREATED
existing.name = validated_data["name"]
existing.config = validated_data.get("config")
Expand Down
6 changes: 3 additions & 3 deletions api/experimentation/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ def create(self, request: Request, *args: object, **kwargs: object) -> Response:
serializer = self.get_serializer(data=request.data)
serializer.is_valid(raise_exception=True)

warehouse_type = serializer.validated_data["warehouse_type"]
if WarehouseConnection.objects.filter(
environment=environment,
warehouse_type=warehouse_type,
).exists():
return Response(
{"detail": "Warehouse connection already exists."},
{
"detail": "This environment already has an active warehouse connection."
},
status=409,
)

Expand Down
34 changes: 33 additions & 1 deletion api/tests/unit/experimentation/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,39 @@ def test_post__already_exists__returns_409(

# Then
assert response.status_code == status.HTTP_409_CONFLICT
assert response.json()["detail"] == "Warehouse connection already exists."
assert (
response.json()["detail"]
== "This environment already has an active warehouse connection."
)


def test_post__different_type_already_exists__returns_409(
admin_client: APIClient,
environment: Environment,
enable_features: EnableFeaturesFixture,
warehouse_connection: WarehouseConnection,
warehouse_connection_url: str,
) -> None:
# Given
enable_features("experimentation_warehouse_connection")

# When
response = admin_client.post(
warehouse_connection_url,
data={
"warehouse_type": "snowflake",
"name": "My Snowflake",
"config": {"account_identifier": "xy12345.us-east-1"},
},
format="json",
)

# Then
assert response.status_code == status.HTTP_409_CONFLICT
assert (
response.json()["detail"]
== "This environment already has an active warehouse connection."
)


def test_post__soft_deleted_exists__resurrects_and_returns_201(
Expand Down
Loading