fix(api): pass project_lead_id (not User instance) when creating ProjectMember (#8966)
* test(api): add regression tests for create-project endpoint Cover three scenarios: - project_lead set to the creator's own user_id - project_lead set to a different workspace member - project_lead omitted (baseline) The first two currently fail on preview because of a UUID coercion bug in ProjectMember.objects.create — see follow-up commit. * fix(api): pass project_lead_id (not User instance) when creating ProjectMember The create-project endpoint built a ProjectMember row with member_id=serializer.instance.project_lead, which resolves to a User instance via Django's related descriptor instead of a UUID. Django's UUIDField coercion then fails with AttributeError: 'User' object has no attribute 'replace', which the generic exception handler converts to a 400 "Please provide valid detail" — but only after the Project row was already persisted, leaving an orphaned project without default states. Fix: - Use project_lead_id (FK ID, no descriptor lookup) on both the guard comparison and the ProjectMember creation. - Wrap the post-save flow in transaction.atomic() so any future exception triggers a clean rollback. - Defer model_activity.delay() with transaction.on_commit() so the activity log only fires after a successful commit. - Capture the exception with log_exception() in the generic catch so future regressions surface in api logs. Note: a related data integrity issue exists where ProjectCreateSerializer doesn't create a ProjectIdentifier row (unlike its frontend counterpart). Out of scope here, will follow up in a separate PR. * fix(api): return 500 on unexpected errors and harden project create Address review feedback from @sriramveeraghanta on PR #8966: - The catch-all `except Exception` now returns 500 instead of 400. Reusing the generic 400 response on a server-side crash was the anti-pattern that hid the original ghost-create bug for nine months; a 500 lets clients distinguish between "bad input" and "server fault". - The `IntegrityError` branch no longer falls through silently when the message is unrecognised. It re-raises so the catch-all `except` logs the exception and returns a 500. - `transaction.on_commit()` now schedules `model_activity.delay` via `functools.partial` instead of a lambda, avoiding late-binding closure semantics. - `ProjectCreateSerializer.validate()` now rejects `project_lead` values that are not active workspace members, surfacing the error under the `project_lead` field key (rather than as `non_field_errors`) so API clients can react programmatically. * test(api): harden assertions and cover rollback / workspace-membership Address review feedback from @sriramveeraghanta on PR #8966: - The three existing tests now look up the created project via `Project.objects.get(id=response.data["id"])` instead of `.first()`. The assertion now fails for the right reason if the wrong project is returned by the endpoint. - New `test_create_project_with_lead_not_in_workspace_returns_400` guards the workspace-membership validation added to `ProjectCreateSerializer.validate()`. Expects a 400 with a field-shaped error and zero rows persisted. - New `test_model_activity_not_called_on_rollback` locks in the `transaction.on_commit()` semantics: when an exception is raised inside the atomic block (forced via mocking `State.objects.bulk_create`), the response is 500, no Project / ProjectMember / State rows are persisted, and the deferred `model_activity.delay` task is never dispatched. This prevents a future refactor from silently regressing the rollback contract. * fix(api): mark on_commit dispatch as robust against broker failures Address coderabbit re-review feedback on PR #8966. Without robust=True, an exception raised by model_activity.delay (e.g., a Celery broker outage) propagates out of the on_commit callback and is caught by the outer `except Exception` handler, which returns a 500 despite the project, ProjectMember rows and default States having already been committed. The client sees a 500 and assumes the create failed — the same class of mismatch between actual state and reported status that the original bug exhibited, just at the post-commit phase. Set robust=True so Django logs the dispatch failure internally via the standard transaction logger and the response stays 201, reflecting the persisted state. Switch from `functools.partial` to a nested function (`_dispatch_model_activity`) for the on_commit callable. Django's robust on_commit logging path reads `func.__qualname__` to format the error message; `partial` objects lack that dunder by default, and the `functools.update_wrapper` workaround turns out to be brittle when the wrapped callable is replaced by a Mock (which the new regression test relies on). A nested function exposes `__qualname__` natively, and the locals it closes over are bound at definition time and never rebound before the callback fires, so the late-binding-closure motivation for `partial` over `lambda` does not apply here. A new test, test_response_still_201_when_broker_dispatch_fails, mirrors test_model_activity_not_called_on_rollback to lock in the post-commit branch. It uses `@pytest.mark.django_db(transaction=True)` so the surrounding test transaction is actually committed and the `on_commit` callback fires (the default wrapper suppresses it via rollback). * fix(api): handle unrecognised IntegrityError consistently Address coderabbit re-review feedback on PR #8966. The previous fix used `raise` inside the IntegrityError handler with the intent of "letting the catch-all `except Exception` below log it and return 500". Coderabbit correctly flagged that `raise` exits the try/except entirely — sibling except clauses don't fire — so unrecognised integrity errors actually skipped `log_exception` and the consistent 500 JSON shape, contradicting the stated intent. Replicate the catch-all behaviour inline: log the exception via `log_exception(e)` and return the same generic 500 response with `{"error": "An unexpected error occurred"}`. The client now gets a uniform error shape regardless of which `except` branch handled it. --------- Co-authored-by: Jose Antonio Martinez <257598434+jamartineztelecoengineer84-dotcom@users.noreply.github.com>
This commit is contained in:
jamartineztelecoengineer84-dotcom
committed by
GitHub
GitHub
parent
65d6a94b0a
commit
50a7b47b31
@@ -114,13 +114,20 @@ class ProjectCreateSerializer(BaseSerializer):
|
||||
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
|
||||
|
||||
@@ -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 @@ from plane.db.models import (
|
||||
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 @@ class ProjectListCreateAPIEndpoint(BaseAPIView):
|
||||
serializer = ProjectCreateSerializer(data={**request.data}, context={"workspace_id": workspace.id})
|
||||
|
||||
if serializer.is_valid():
|
||||
serializer.save()
|
||||
with transaction.atomic():
|
||||
serializer.save()
|
||||
|
||||
# Add the user as Administrator to the project
|
||||
_ = ProjectMember.objects.create(project_id=serializer.instance.id, member=request.user, role=20)
|
||||
# Add the creator as Administrator of the project.
|
||||
_ = ProjectMember.objects.create(project_id=serializer.instance.id, member=request.user, 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,
|
||||
# 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,
|
||||
)
|
||||
|
||||
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 @@ class ProjectListCreateAPIEndpoint(BaseAPIView):
|
||||
{"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 @@ class ProjectListCreateAPIEndpoint(BaseAPIView):
|
||||
{"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):
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user