diff --git a/apps/api/plane/api/serializers/project.py b/apps/api/plane/api/serializers/project.py index 644b5ba1076..aa98dd439fe 100644 --- a/apps/api/plane/api/serializers/project.py +++ b/apps/api/plane/api/serializers/project.py @@ -114,13 +114,20 @@ def validate(self, data): if project_identifier is not None and re.match(Project.FORBIDDEN_IDENTIFIER_CHARS_PATTERN, project_identifier): raise serializers.ValidationError("Project identifier cannot contain special characters.") - if data.get("project_lead", None) is not None: - # Check if the project lead is a member of the workspace - if not WorkspaceMember.objects.filter( + project_lead = data.get("project_lead") + if ( + project_lead + and not WorkspaceMember.objects.filter( workspace_id=self.context["workspace_id"], - member_id=data.get("project_lead"), - ).exists(): - raise serializers.ValidationError("Project lead should be a user in the workspace") + member=project_lead, + is_active=True, + ).exists() + ): + # Field-shaped error so DRF surfaces it under the specific key + # rather than as non_field_errors. Also requires the membership + # to be active so that revoked / removed members can't slip + # through and trigger the FK error downstream. + raise serializers.ValidationError({"project_lead": "The provided user is not a member of this workspace."}) if data.get("default_assignee", None) is not None: # Check if the default assignee is a member of the workspace diff --git a/apps/api/plane/api/views/project.py b/apps/api/plane/api/views/project.py index 5ab0fd1c1fa..bc478d1bb10 100644 --- a/apps/api/plane/api/views/project.py +++ b/apps/api/plane/api/views/project.py @@ -6,7 +6,7 @@ import json # Django imports -from django.db import IntegrityError +from django.db import IntegrityError, transaction from django.db.models import Exists, F, Func, OuterRef, Prefetch, Q, Subquery, Count from django.db.models.functions import Coalesce from django.utils import timezone @@ -38,6 +38,7 @@ ProjectPage, ) from plane.bgtasks.webhook_task import model_activity, webhook_activity +from plane.utils.exception_logger import log_exception from .base import BaseAPIView from plane.utils.host import base_host from plane.api.serializers import ( @@ -223,48 +224,72 @@ def post(self, request, slug): serializer = ProjectCreateSerializer(data={**request.data}, context={"workspace_id": workspace.id}) if serializer.is_valid(): - serializer.save() - - # Add the user as Administrator to the project - _ = ProjectMember.objects.create(project_id=serializer.instance.id, member=request.user, role=20) + with transaction.atomic(): + serializer.save() + + # Add the creator as Administrator of the project. + _ = ProjectMember.objects.create(project_id=serializer.instance.id, member=request.user, role=20) + + # If a different project_lead was provided, add them as + # Administrator too. Use project_lead_id (the FK column) + # rather than project_lead (the related descriptor, which + # would resolve to a User instance and break UUID coercion + # downstream in ProjectMember.objects.create). + if ( + serializer.instance.project_lead_id is not None + and serializer.instance.project_lead_id != request.user.id + ): + ProjectMember.objects.create( + project_id=serializer.instance.id, + member_id=serializer.instance.project_lead_id, + role=20, + ) - if serializer.instance.project_lead is not None and str(serializer.instance.project_lead) != str( - request.user.id - ): - ProjectMember.objects.create( - project_id=serializer.instance.id, - member_id=serializer.instance.project_lead, - role=20, + State.objects.bulk_create( + [ + State( + name=state["name"], + color=state["color"], + project=serializer.instance, + sequence=state["sequence"], + workspace=serializer.instance.workspace, + group=state["group"], + default=state.get("default", False), + created_by=request.user, + ) + for state in DEFAULT_STATES + ] ) - State.objects.bulk_create( - [ - State( - name=state["name"], - color=state["color"], - project=serializer.instance, - sequence=state["sequence"], - workspace=serializer.instance.workspace, - group=state["group"], - default=state.get("default", False), - created_by=request.user, + project = self.get_queryset().filter(pk=serializer.instance.id).first() + + # Defer the activity-log task until the surrounding + # transaction commits, so it never fires on a rolled-back + # creation. + # robust=True so broker / dispatch failures are logged + # internally by Django and don't surface as 500 after a + # successful commit (the inverse of the rollback path + # covered by test_model_activity_not_called_on_rollback). + # A nested function (rather than functools.partial) is + # used here because Django's robust on_commit logging + # path reads ``func.__qualname__`` to format the error + # message; ``partial`` objects don't have that dunder + # by default and the workaround is brittle when the + # wrapped callable is a mock. The closure captures + # the locals at construction time and they are never + # rebound, so late-binding is not a hazard here. + def _dispatch_model_activity(): + model_activity.delay( + model_name="project", + model_id=str(project.id), + requested_data=request.data, + current_instance=None, + actor_id=request.user.id, + slug=slug, + origin=base_host(request=request, is_app=True), ) - for state in DEFAULT_STATES - ] - ) - project = self.get_queryset().filter(pk=serializer.instance.id).first() - - # Model activity - model_activity.delay( - model_name="project", - model_id=str(project.id), - requested_data=request.data, - current_instance=None, - actor_id=request.user.id, - slug=slug, - origin=base_host(request=request, is_app=True), - ) + transaction.on_commit(_dispatch_model_activity, robust=True) serializer = ProjectSerializer(project) return Response(serializer.data, status=status.HTTP_201_CREATED) @@ -275,6 +300,17 @@ def post(self, request, slug): {"name": "The project name is already taken"}, status=status.HTTP_409_CONFLICT, ) + # Any other IntegrityError is unexpected: log it the same way + # the catch-all `except Exception` below would and return the + # same generic 500 so the client gets a uniform error shape. + # `raise` here would not fall through to a sibling except + # clause — it would exit the try/except entirely and bypass + # both the logging and the JSON response. + log_exception(e) + return Response( + {"error": "An unexpected error occurred"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) except Workspace.DoesNotExist: return Response({"error": "Workspace does not exist"}, status=status.HTTP_404_NOT_FOUND) except ValidationError: @@ -282,6 +318,16 @@ def post(self, request, slug): {"identifier": "The project identifier is already taken"}, status=status.HTTP_409_CONFLICT, ) + except Exception as e: + # Unexpected server-side failure: log the traceback and return a + # generic 500 so the client can distinguish it from a 4xx caused + # by bad input. Returning 400 here was the anti-pattern that + # masked the original ghost-create bug. + log_exception(e) + return Response( + {"error": "An unexpected error occurred"}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) class ProjectDetailAPIEndpoint(BaseAPIView): diff --git a/apps/api/plane/tests/contract/api/test_projects.py b/apps/api/plane/tests/contract/api/test_projects.py new file mode 100644 index 00000000000..329097e60cd --- /dev/null +++ b/apps/api/plane/tests/contract/api/test_projects.py @@ -0,0 +1,216 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +from unittest import mock +from uuid import uuid4 + +import pytest +from rest_framework import status + +from plane.db.models import Project, ProjectMember, State, User, WorkspaceMember + + +@pytest.fixture +def other_workspace_member(db, workspace): + """Create another user that is a member of the workspace, distinct from the creator.""" + unique_id = uuid4().hex[:8] + other = User.objects.create( + email=f"other-{unique_id}@plane.so", + username=f"other_user_{unique_id}", + first_name="Other", + last_name="User", + ) + other.set_password("test-password") + other.save() + WorkspaceMember.objects.create(workspace=workspace, member=other, role=20) + return other + + +@pytest.fixture +def outsider_user(db): + """Create a user that is NOT a member of any workspace under test.""" + unique_id = uuid4().hex[:8] + outsider = User.objects.create( + email=f"outsider-{unique_id}@plane.so", + username=f"outsider_{unique_id}", + first_name="Out", + last_name="Sider", + ) + outsider.set_password("test-password") + outsider.save() + return outsider + + +@pytest.mark.contract +class TestProjectListCreateAPIEndpoint: + """Contract tests for POST /api/v1/workspaces/{slug}/projects/.""" + + def get_url(self, workspace_slug): + return f"/api/v1/workspaces/{workspace_slug}/projects/" + + @pytest.mark.django_db + def test_create_project_with_lead_as_creator(self, api_key_client, workspace, create_user): + """Regression for the ghost-create bug. + + When project_lead points to the creator's own user_id, the endpoint + must return 201 and create a fully-populated project (single + ProjectMember as admin, default workflow states). + + Before the fix, the endpoint returned 400 "Please provide valid detail" + but had already persisted the Project row without states or members, + leaving an unusable orphan. + """ + url = self.get_url(workspace.slug) + payload = { + "name": "Self Lead Project", + "identifier": "SL", + "project_lead": str(create_user.id), + } + + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_201_CREATED, f"Got {response.status_code}: {response.data!r}" + + # Look up the project we just created instead of relying on + # ordering-sensitive Project.objects.first(). + project = Project.objects.get(id=response.data["id"]) + # Creator is registered as admin (single membership; lead == creator + # should not produce a duplicate row). + assert ProjectMember.objects.filter(project=project, member=create_user, role=20).count() == 1 + # Default workflow states must be created. + assert State.objects.filter(project=project).count() == 5 + + @pytest.mark.django_db + def test_create_project_with_lead_as_other_user( + self, api_key_client, workspace, create_user, other_workspace_member + ): + """When project_lead is a different workspace member, both creator + and lead become admins of the project.""" + url = self.get_url(workspace.slug) + payload = { + "name": "Other Lead Project", + "identifier": "OL", + "project_lead": str(other_workspace_member.id), + } + + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_201_CREATED, f"Got {response.status_code}: {response.data!r}" + project = Project.objects.get(id=response.data["id"]) + + # Both creator and other_workspace_member are admins. + assert ProjectMember.objects.filter(project=project, member=create_user, role=20).exists() + assert ProjectMember.objects.filter(project=project, member=other_workspace_member, role=20).exists() + assert State.objects.filter(project=project).count() == 5 + + @pytest.mark.django_db + def test_create_project_without_lead(self, api_key_client, workspace, create_user): + """Baseline regression: omitting project_lead must succeed and the + creator becomes the sole admin.""" + url = self.get_url(workspace.slug) + payload = { + "name": "Basic Project", + "identifier": "BP", + } + + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_201_CREATED, f"Got {response.status_code}: {response.data!r}" + project = Project.objects.get(id=response.data["id"]) + assert ProjectMember.objects.filter(project=project, member=create_user, role=20).count() == 1 + assert State.objects.filter(project=project).count() == 5 + + @pytest.mark.django_db + def test_create_project_with_lead_not_in_workspace_returns_400(self, api_key_client, workspace, outsider_user): + """When project_lead refers to a user that is NOT a member of the + target workspace, the endpoint must reject the request with a 400 + carrying a field-shaped error and must not persist the Project.""" + url = self.get_url(workspace.slug) + payload = { + "name": "Outsider Lead Project", + "identifier": "OUT", + "project_lead": str(outsider_user.id), + } + + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST, f"Got {response.status_code}: {response.data!r}" + assert "project_lead" in response.data, ( + f"Expected field-shaped error under 'project_lead', got {response.data!r}" + ) + # No project should have been persisted. + assert Project.objects.count() == 0 + + @pytest.mark.django_db + def test_model_activity_not_called_on_rollback(self, api_key_client, workspace, create_user): + """If anything inside the transaction.atomic() block raises, the + whole creation must roll back (no Project, no ProjectMember, no + State) and the deferred model_activity.delay() task must not fire, + because it is registered with transaction.on_commit(). + + Force the failure inside State.objects.bulk_create — past the point + where the original ghost-create bug would have committed a partial + Project — and verify the response is 500 with no side effects. + """ + url = self.get_url(workspace.slug) + payload = { + "name": "Rollback Probe", + "identifier": "RB", + "project_lead": str(create_user.id), + } + + forced_error = RuntimeError("forced failure for rollback test") + + with ( + mock.patch( + "plane.api.views.project.State.objects.bulk_create", + side_effect=forced_error, + ), + mock.patch("plane.api.views.project.model_activity") as mocked_activity, + ): + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR, ( + f"Got {response.status_code}: {response.data!r}" + ) + # Transaction must have rolled back: no Project, no ProjectMember, + # no State persisted. + assert Project.objects.count() == 0 + assert ProjectMember.objects.count() == 0 + assert State.objects.count() == 0 + # And the deferred Celery task must not have been dispatched — + # transaction.on_commit() callbacks only fire on a successful commit. + mocked_activity.delay.assert_not_called() + + @pytest.mark.django_db(transaction=True) + def test_response_still_201_when_broker_dispatch_fails(self, api_key_client, workspace, create_user): + """If model_activity.delay raises *after* the atomic block has + committed (e.g., the Celery broker is down), the project, member + rows and states are already persisted — the response must remain + 201 and the failure must be absorbed by Django's robust=True + on_commit handling, not surface as a 500. + + Uses ``transaction=True`` so the surrounding test transaction is + actually committed and the ``on_commit`` callback fires (the + default ``django_db`` wrapper would suppress it via rollback).""" + url = self.get_url(workspace.slug) + payload = { + "name": "Broker Down", + "identifier": "BD", + "project_lead": str(create_user.id), + } + + with mock.patch("plane.api.views.project.model_activity") as mocked_activity: + mocked_activity.delay.side_effect = RuntimeError("broker unavailable") + response = api_key_client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_201_CREATED, f"Got {response.status_code}: {response.data!r}" + # Project and its scaffolding are persisted (commit happened + # before the on_commit callback fired). + project = Project.objects.get(id=response.data["id"]) + assert ProjectMember.objects.filter(project=project).count() == 1 + assert State.objects.filter(project=project).count() == 5 + # The dispatch was attempted but its failure was swallowed by + # transaction.on_commit(robust=True). + mocked_activity.delay.assert_called_once()