Skip to content
Closed
85 changes: 85 additions & 0 deletions ddpui/api/admin_portal/organizations_api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
from ninja import Router
from ninja.errors import HttpError

from ddpui.auth import has_permission
from ddpui.models.org import Org
from ddpui.schemas.admin_org_schema import UpdateOrganizationSchema

admin_org_router = Router()


@admin_org_router.get("/v1/organizations")
@admin_org_router.get("/v1/organizations/")
@has_permission(["can_manage_organization"])
def get_admin_organizations(request, name: str | None = None):
"""List organizations with optional filtering."""
queryset = Org.objects.all()

if name:
queryset = queryset.filter(name__icontains=name)

orgs = list(queryset.values("id", "name", "slug"))

return {
"success": True,
"count": len(orgs),
"data": orgs,
}


@admin_org_router.get("/v1/organizations/{org_id}")
@admin_org_router.get("/v1/organizations/{org_id}/")
@has_permission(["can_manage_organization"])
def get_single_org(request, org_id: int):
"""Get single organization by ID."""
org = (
Org.objects.filter(id=org_id)
.values("id", "name", "slug")
.first()
)

if not org:
raise HttpError(404, "Organization not found")

return {
"success": True,
"data": org,
}


@admin_org_router.put("/v1/organizations/{org_id}")
@admin_org_router.put("/v1/organizations/{org_id}/")
@has_permission(["can_manage_organization"])
def update_organization(
request,
org_id: int,
payload: UpdateOrganizationSchema,
):
"""Update organization details."""
org = Org.objects.filter(id=org_id).first()

if not org:
raise HttpError(404, "Organization not found")

existing_slug = (
Org.objects.filter(slug=payload.slug)
.exclude(id=org_id)
.exists()
)

if existing_slug:
raise HttpError(400, "Slug already exists")

org.name = payload.name
org.slug = payload.slug
org.save()
Comment on lines +65 to +81

@coderabbitai coderabbitai Bot May 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Org model field definitions (name/slug) =="
fd -i 'org.py' | xargs -r rg -n -C2 'class\s+Org|name\s*=|slug\s*='

echo
echo "== Uniqueness constraints involving slug =="
rg -n -C2 'slug|UniqueConstraint|unique_together|unique\s*=\s*True' --type py

Repository: DalgoT4D/DDP_backend

Length of output: 50377


Slug uniqueness check is non-atomic and race-prone.

The Org.slug field has no database-level uniqueness constraint (unique=True or UniqueConstraint). The pre-check with .exists() and the write with .save() are separate operations, allowing concurrent requests to both pass validation and create duplicate slugs.

Add a database constraint and wrap the write in atomic transaction handling with IntegrityError catching. Also update the HTTP status code to 409 (Conflict) to properly reflect the error semantics.

💡 Suggested hardening
+from django.db import IntegrityError, transaction
 from ninja import Router
 from ninja.errors import HttpError
@@
     existing_slug = (
         Org.objects.filter(slug=payload.slug)
         .exclude(id=org_id)
         .exists()
     )
 
     if existing_slug:
-        raise HttpError(400, "Slug already exists")
+        raise HttpError(409, "Slug already exists")
 
     org.name = payload.name
     org.slug = payload.slug
-    org.save()
+    try:
+        with transaction.atomic():
+            org.save(update_fields=["name", "slug"])
+    except IntegrityError:
+        raise HttpError(409, "Slug already exists")

Also add to the Org model (ddpui/models/org.py):

 class Org(models.Model):
     name = models.CharField(max_length=50)
-    slug = models.CharField(max_length=20, null=True)
+    slug = models.CharField(max_length=20, null=True, unique=True)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ddpui/api/admin_portal/organizations_api.py` around lines 64 - 75, The slug
uniqueness check is race-prone because it lacks a DB constraint and the exists()
pre-check is separate from the save; add a uniqueness constraint to the Org
model (Org.slug -> unique=True or a UniqueConstraint in ddpui/models/org.py and
generate a migration), then in organizations_api.py wrap the write in a
transaction.atomic block around org.save() and catch django.db.IntegrityError
(or DatabaseError) to translate it into HttpError(409, "Slug already exists");
you can keep the existing_slug pre-check for early feedback but must still
handle IntegrityError on org.save() to ensure atomicity and return 409 on
conflict.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review. I’ve noted the race-condition concern around slug uniqueness validation. Since this PR focuses on introducing the initial admin update APIs, I’d prefer to keep database-level uniqueness constraints and transactional handling as a follow-up improvement to avoid expanding the migration scope significantly in this change set.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@sachin9919, understood — thanks for the context. Deferring database-level constraints and transactional handling to a follow-up is a reasonable call to keep this PR focused.

Would you like me to open a GitHub issue to track the follow-up work? It would cover:

  1. Adding unique=True (or a UniqueConstraint) to Org.slug along with the corresponding migration.
  2. Wrapping org.save() in transaction.atomic() and catching IntegrityError to return HTTP 409 on slug conflicts in the update endpoint.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated the schema validation to reject whitespace-only values for name and slug using field_validator, ensuring inputs are properly stripped and validated before update operations.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -i 'admin_org_schema.py' | xargs -r cat -n

Length of output: 683


@sachin9919, the implementation looks solid. A few notes from reviewing the updated file:

  1. Correct layeringField(..., min_length=1) rejects empty strings at Pydantic's field level, and must_not_be_blank then catches whitespace-only inputs (e.g. " "). The two guards complement each other well.

  2. Silent normalisation — because the validator returns value.strip(), inputs like " admin " will be silently saved as "admin". This is usually desirable, but worth being aware of (and potentially documenting) so callers know the API normalises values rather than rejecting them.

  3. Classmethod pattern@field_validator + @classmethod is the correct Pydantic v2 pattern; no issues there.


The race-condition / atomicity concern from the earlier review thread is still tracked as a follow-up. Would you still like me to open a GitHub issue to track:

  • Adding unique=True to Org.slug with a migration.
  • Wrapping org.save() in transaction.atomic() and catching IntegrityErrorHTTP 409?


return {
"success": True,
"message": "Organization updated successfully",
"data": {
"id": org.id,
"name": org.name,
"slug": org.slug,
},
}
5 changes: 4 additions & 1 deletion ddpui/models/airbyte.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ class SyncStats(models.Model):
job_id = models.IntegerField(null=True)
attempt = models.IntegerField(default=0)
status = models.TextField()
sync_type = models.CharField(choices=[("manual", "manual"), ("orchestrate", "orchestrate")])
sync_type = models.CharField(
max_length=20,
choices=[("manual", "manual"), ("orchestrate", "orchestrate")],
)
sync_time = models.DateTimeField()
sync_duration_s = models.BigIntegerField(default=0)
sync_records = models.BigIntegerField(default=0)
Expand Down
9 changes: 7 additions & 2 deletions ddpui/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@
from ddpui.api.filter_api import filter_router
from ddpui.api.public_api import public_router
from ddpui.api.report_api import report_router
from ddpui.api.admin_portal.organizations_api import admin_org_router


src_api = NinjaAPI(
urls_namespace="api",
title="Dalgo backend apis",
description="Open source ELT orchestrator",
docs_url="/api/docs",
# auth=auth.CustomAuthMiddleware(),
auth=auth.CustomJwtAuthMiddleware(),
)

Expand Down Expand Up @@ -87,6 +87,8 @@ def ninja_default_error_handler(
filter_router.tags = ["Filters"]
public_router.tags = ["Public"]
report_router.tags = ["Reports"]
admin_org_router.tags = ["Admin Organizations"]


# mount all the module routes
src_api.add_router("/api/airbyte/", airbyte_router)
Expand All @@ -108,6 +110,8 @@ def ninja_default_error_handler(
src_api.add_router("/api/dashboards/", dashboard_native_router)
src_api.add_router("/api/filters/", filter_router)
src_api.add_router("/api/reports/", report_router)
src_api.add_router("/api/admin/", admin_org_router)


# Public API without authentication
public_api = NinjaAPI(
Expand All @@ -116,4 +120,5 @@ def ninja_default_error_handler(
description="Public endpoints for shared dashboards - no authentication required",
docs_url="/api/v1/public/docs",
)
public_api.add_router("/api/v1/public/", public_router)

public_api.add_router("/api/v1/public/", public_router)
8 changes: 8 additions & 0 deletions ddpui/schemas/admin_org_schema.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from ninja import Schema


class UpdateOrganizationSchema(Schema):
"""Schema for updating organization details."""

name: str
slug: str
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated