[WEB-4668] fix: LabelDetailAPIEndpoint from LabelListCreateAPIEndpoint #7571
Conversation
…or passing the get_queryset method to inherited class
…validation - Introduced comprehensive tests for creating, retrieving, updating, and deleting labels. - Enhanced validation logic to prevent duplicate external IDs in label creation. - Updated the existing LabelDetailAPIEndpoint to exclude the current label when checking for duplicates.
WalkthroughLabel detail endpoint now inherits from LabelListCreateAPIEndpoint, and its PATCH conflict check targets Label records excluding the current label. A new test suite validates list/create/detail/update/delete behavior for label endpoints, including external_id/external_source handling and duplicate constraint responses. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as Label API Endpoint
participant DB as Label Store
C->>API: POST /labels (name, color, [external_id, source])
API->>DB: Check conflict by external_id+source
alt Conflict
API-->>C: 409 CONFLICT
else No conflict
API->>DB: Create Label
API-->>C: 201 Created
end
sequenceDiagram
participant C as Client
participant API as Label Detail Endpoint
participant DB as Label Store
C->>API: PATCH /labels/{id} (updates)
API->>DB: Check conflict excluding {id}
alt Conflict
API-->>C: 409 CONFLICT
else No conflict
API->>DB: Update Label
API-->>C: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/plane/api/views/issue.py (1)
950-957: Good inheritance change; consider removing redundant attribute re-declarationsInheriting from LabelListCreateAPIEndpoint is the right move to reuse get_queryset and permissions. The explicit re-declarations of serializer_class, model, permission_classes, and use_read_replica here are redundant (they’re identical to the parent) and can be dropped to keep things DRY and prevent divergence in future edits.
Suggested diff:
class LabelDetailAPIEndpoint(LabelListCreateAPIEndpoint): """Label Detail Endpoint""" - serializer_class = LabelSerializer - model = Label - permission_classes = [ProjectMemberPermission] - use_read_replica = True + # Inherit serializer_class, model, permission_classes, and use_read_replica + # from LabelListCreateAPIEndpoint to avoid duplication.apps/api/plane/tests/contract/api/test_labels.py (3)
3-3: Remove unused importIntegrityError is not used in this test module.
Apply this diff:
-from django.db import IntegrityError
176-186: Drop redundant local import of uuid4uuid4 is already imported at the top. The local import is unnecessary.
Apply this diff:
- from uuid import uuid4 - fake_id = uuid4()
188-204: Add PATCH external_id conflict and unchanged-value tests to cover edge casesTo validate the updated conflict logic (including exclude(pk=...) and guarding on provided external_id), add tests for:
- 409 when PATCH attempts to set external_id to one already used by another label
- 200 when PATCH keeps the same external_id on the same label (no conflict)
I can open a follow-up PR, but here are ready-to-use tests:
@pytest.mark.django_db def test_update_label_duplicate_external_id_conflict(api_key_client, workspace, project): l1 = Label.objects.create( name="L1", project=project, workspace=workspace, color="#111111", external_id="dup-1", external_source="github" ) l2 = Label.objects.create( name="L2", project=project, workspace=workspace, color="#222222", external_id="dup-2", external_source="github" ) url = f"/api/v1/workspaces/{workspace.slug}/projects/{project.id}/labels/{l2.id}/" response = api_key_client.patch(url, {"external_id": "dup-1", "external_source": "github"}, format="json") assert response.status_code == status.HTTP_409_CONFLICT @pytest.mark.django_db def test_update_label_external_id_unchanged_no_conflict(api_key_client, workspace, project): lbl = Label.objects.create( name="L1", project=project, workspace=workspace, color="#111111", external_id="keep-1", external_source="github" ) url = f"/api/v1/workspaces/{workspace.slug}/projects/{project.id}/labels/{lbl.id}/" response = api_key_client.patch(url, {"external_id": "keep-1"}, format="json") assert response.status_code == status.HTTP_200_OK
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/api/views/issue.py(2 hunks)apps/api/plane/tests/contract/api/test_labels.py(1 hunks)
| str(request.data.get("external_id")) | ||
| and (label.external_id != str(request.data.get("external_id"))) | ||
| and Issue.objects.filter( | ||
| and Label.objects.filter( | ||
| project_id=project_id, | ||
| workspace__slug=slug, | ||
| external_source=request.data.get( | ||
| "external_source", label.external_source | ||
| ), | ||
| external_id=request.data.get("external_id"), | ||
| ).exists() | ||
| ) | ||
| .exclude(id=pk) | ||
| .exists() | ||
| ): |
There was a problem hiding this comment.
Bug: str(None) truthiness causes false 409 conflicts on PATCH
Using str(request.data.get("external_id")) in the guard means that when external_id is missing or null, it becomes the string "None" (truthy), which incorrectly triggers the duplicate check and can return 409 for valid requests. Also, exclude(pk=pk) is more idiomatic than exclude(id=pk).
Apply this diff:
- if (
- str(request.data.get("external_id"))
- and (label.external_id != str(request.data.get("external_id")))
- and Label.objects.filter(
- project_id=project_id,
- workspace__slug=slug,
- external_source=request.data.get(
- "external_source", label.external_source
- ),
- external_id=request.data.get("external_id"),
- )
- .exclude(id=pk)
- .exists()
- ):
+ if (
+ request.data.get("external_id")
+ and (label.external_id != str(request.data.get("external_id")))
+ and Label.objects.filter(
+ project_id=project_id,
+ workspace__slug=slug,
+ external_source=request.data.get(
+ "external_source", label.external_source
+ ),
+ external_id=request.data.get("external_id"),
+ )
+ .exclude(pk=pk)
+ .exists()
+ ):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| str(request.data.get("external_id")) | |
| and (label.external_id != str(request.data.get("external_id"))) | |
| and Issue.objects.filter( | |
| and Label.objects.filter( | |
| project_id=project_id, | |
| workspace__slug=slug, | |
| external_source=request.data.get( | |
| "external_source", label.external_source | |
| ), | |
| external_id=request.data.get("external_id"), | |
| ).exists() | |
| ) | |
| .exclude(id=pk) | |
| .exists() | |
| ): | |
| if ( | |
| request.data.get("external_id") | |
| and (label.external_id != str(request.data.get("external_id"))) | |
| and Label.objects.filter( | |
| project_id=project_id, | |
| workspace__slug=slug, | |
| external_source=request.data.get( | |
| "external_source", label.external_source | |
| ), | |
| external_id=request.data.get("external_id"), | |
| ) | |
| .exclude(pk=pk) | |
| .exists() | |
| ): |
🤖 Prompt for AI Agents
In apps/api/plane/api/views/issue.py around lines 1013 to 1025, the guard is
using str(request.data.get("external_id")) which converts None to "None"
(truthy) and causes false 409 conflicts; change the logic to read the raw
external_id = request.data.get("external_id"), ensure you only run the duplicate
check when external_id is not None and not an empty string (e.g., if external_id
is not None and external_id != ""), use that raw value (or str() only after
confirming it's not None) in the filter, and replace .exclude(id=pk) with the
idiomatic .exclude(pk=pk).
Description
fix: LabelDetailAPIEndpoint from LabelListCreateAPIEndpoint or passing the
get_querysetmethod to inherited classType of Change
Test Scenarios
v1apisReferences
WEB-4668
Summary by CodeRabbit
Bug Fixes
Tests