feat(admin): add organization APIs and fix SyncStats migration issue#1330
feat(admin): add organization APIs and fix SyncStats migration issue#1330sachin9919 wants to merge 1 commit intoDalgoT4D:mainfrom
Conversation
WalkthroughThis PR adds a new admin organizations API module with endpoints to list and retrieve organizations, updates the SyncStats database schema to increase constraints on existing fields, and integrates the new API into the main router. ChangesAdmin Organizations API
SyncStats Schema Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
ddpui/api/admin_portal/organizations_api.py (1)
19-24: ⚡ Quick winDouble DB query:
queryset.count()fires a secondCOUNTafter the results are already fetched
list(orgs)evaluates theSELECTquery;queryset.count()then issues a separateCOUNT(*)query. Materialize once and uselen():⚡ Proposed fix
- orgs = queryset.values("id", "name", "slug") - - return { - "success": True, - "count": queryset.count(), - "data": list(orgs) - } + orgs = list(queryset.values("id", "name", "slug")) + + return { + "success": True, + "count": len(orgs), + "data": orgs + }🤖 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 19 - 24, Materialize the queryset once to avoid two DB hits: assign orgs_list = list(queryset.values("id", "name", "slug")) and return "count": len(orgs_list) instead of calling queryset.count(); update references to use orgs_list (or rename orgs) so the SELECT is executed only once and the count is derived from the in-memory list.ddpui/routes.py (1)
89-110: 💤 Low value
admin_org_routeris missing a.tagsassignment, unlike all other routersEvery other router sets
.tagsfor API documentation grouping (lines 69–88), butadmin_org_routeris not included. The admin organization endpoints will appear ungrouped in the Swagger docs.📄 Proposed fix
public_router.tags = ["Public"] report_router.tags = ["Reports"] +admin_org_router.tags = ["Admin"]🤖 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/routes.py` around lines 89 - 110, The admin_org_router is missing a .tags assignment so its endpoints are ungrouped in Swagger; add a tags list to the router (e.g., set admin_org_router.tags = ["Admin"] or whatever consistent group name your other routers use) before calling src_api.add_router("/api/admin/", admin_org_router) so the admin endpoints are grouped in the API docs.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@ddpui/api/admin_portal/organizations_api.py`:
- Line 10: The function get_admin_organizations uses a default None for
parameter name without an explicit Optional type which violates PEP 484; update
the signature to annotate name as Optional[str] (or str | None if the codebase
uses Python 3.10+ union syntax), and add the corresponding import (from typing
import Optional) if needed; specifically modify the get_admin_organizations
definition and ensure the file imports Optional so the default None is correctly
typed.
- Around line 8-41: Both endpoints (get_admin_organizations and get_single_org)
are missing an admin permission check and have a few other issues: add the
`@has_permission`("can_manage_organizations") decorator above both functions so
only admins can call them (request.permissions is set by CustomJwtAuthMiddleware
but does not enforce admin role), change the query parameter type to
Optional[str] (or str | None) for name in get_admin_organizations, materialize
the queryset results before counting by doing orgs =
list(Org.objects.filter(...).values("id","name","slug")) and then use len(orgs)
for the count instead of queryset.count(), and replace the Http404("Organization
not found") in get_single_org with HttpError(404, "Organization not found") so
the custom message is preserved; use the Org model and the has_permission symbol
to locate the code to update.
- Around line 36-37: Replace the Http404 raise with a ninja.errors HttpError so
the custom message is preserved in production: import HttpError from
ninja.errors and change the raise at the org existence check (the block that
checks "if not org") to raise HttpError(404, "Organization not found") instead
of Http404("Organization not found"), ensuring the error message is passed
through to callers.
In `@ddpui/migrations/0085_syncstats.py`:
- Line 25: Add a new migration that alters SyncStats.connection_id to match the
model change (max_length 50): create a migration (e.g., 0086) with an AlterField
operation targeting model_name="syncstats", name="connection_id" and set field
to models.CharField(max_length=50) so the DB schema matches
ddpui/models/airbyte.py's SyncStats.connection_id change; ensure the new
migration is included in the app migrations list and committed.
In `@ddpui/models/airbyte.py`:
- Line 11: Change the Airbyte model's connection_id field from max_length=50
back to max_length=36 to match the rest of the codebase; locate the
connection_id declaration in ddpui.models.airbyte (the connection_id =
models.CharField(...) line) and set max_length=36 so it stays consistent with
ConnectionMeta.connection_id, ConnectionJob.connection_id,
OrgSchemaChange.connection_id and OrgTask.connection_id.
- Line 16: Remove the development-artifact comment "# ✅ FIXED LINE (added
max_length)" from the top of the Airbyte model in ddpui/models/airbyte.py so
only meaningful docstrings/comments remain; ensure the actual change (adding
max_length) stays in the model definition and run the linter/formatting before
committing.
---
Nitpick comments:
In `@ddpui/api/admin_portal/organizations_api.py`:
- Around line 19-24: Materialize the queryset once to avoid two DB hits: assign
orgs_list = list(queryset.values("id", "name", "slug")) and return "count":
len(orgs_list) instead of calling queryset.count(); update references to use
orgs_list (or rename orgs) so the SELECT is executed only once and the count is
derived from the in-memory list.
In `@ddpui/routes.py`:
- Around line 89-110: The admin_org_router is missing a .tags assignment so its
endpoints are ungrouped in Swagger; add a tags list to the router (e.g., set
admin_org_router.tags = ["Admin"] or whatever consistent group name your other
routers use) before calling src_api.add_router("/api/admin/", admin_org_router)
so the admin endpoints are grouped in the API docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dc43e6c-e5af-4446-8519-f64db1767454
📒 Files selected for processing (5)
ddpui/api/admin_portal/__init__.pyddpui/api/admin_portal/organizations_api.pyddpui/migrations/0085_syncstats.pyddpui/models/airbyte.pyddpui/routes.py
| @admin_org_router.get("/v1/organizations") | ||
| @admin_org_router.get("/v1/organizations/") | ||
| def get_admin_organizations(request, name: str = None): | ||
| """ | ||
| List organizations with optional filtering | ||
| """ | ||
| queryset = Org.objects.all() | ||
|
|
||
| if name: | ||
| queryset = queryset.filter(name__icontains=name) | ||
|
|
||
| orgs = queryset.values("id", "name", "slug") | ||
|
|
||
| return { | ||
| "success": True, | ||
| "count": queryset.count(), | ||
| "data": list(orgs) | ||
| } | ||
|
|
||
|
|
||
| @admin_org_router.get("/v1/organizations/{org_id}") | ||
| @admin_org_router.get("/v1/organizations/{org_id}/") | ||
| 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 Http404("Organization not found") | ||
|
|
||
| return { | ||
| "success": True, | ||
| "data": org |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether any admin-level permission guard or decorator exists in the codebase
rg -rn --type=py "admin" --include="*.py" -l | xargs rg -l "permission|is_superuser|is_staff|admin_required" 2>/dev/null | head -20Repository: DalgoT4D/DDP_backend
Length of output: 1402
🏁 Script executed:
# 1. Check CustomJwtAuthMiddleware implementation
fd -t f "*.py" | xargs rg -l "CustomJwtAuthMiddleware" -A 20Repository: DalgoT4D/DDP_backend
Length of output: 426
🏁 Script executed:
# 2. Look at the actual organizations_api.py file to see full context
cat -n ddpui/api/admin_portal/organizations_api.py | head -50Repository: DalgoT4D/DDP_backend
Length of output: 1351
🏁 Script executed:
# 3. Find other admin endpoints to see how they handle authorization
rg -n "admin.*router" --type=py -A 5 | head -100Repository: DalgoT4D/DDP_backend
Length of output: 2103
🏁 Script executed:
# 4. Check for permission decorators or checking patterns
rg -n "permission|@admin|@require" --type=py | head -40Repository: DalgoT4D/DDP_backend
Length of output: 2997
🏁 Script executed:
# Check src_api auth configuration
cat -n ddpui/routes.py | sed -n '25,40p'Repository: DalgoT4D/DDP_backend
Length of output: 677
🏁 Script executed:
# Check CustomJwtAuthMiddleware
rg -A 30 "class CustomJwtAuthMiddleware" --type=pyRepository: DalgoT4D/DDP_backend
Length of output: 1776
🏁 Script executed:
# Check has_permission decorator implementation
rg -A 15 "def has_permission" --type=pyRepository: DalgoT4D/DDP_backend
Length of output: 981
🏁 Script executed:
# Check Django/Ninja Http404 behavior and if custom messages are preserved
rg -n "Http404\|HttpError" ddpui/api/admin_portal/organizations_api.pyRepository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Look at how Http404 is used elsewhere in the codebase to compare
rg -B 2 -A 2 "raise Http404" --type=py | head -40Repository: DalgoT4D/DDP_backend
Length of output: 1381
🏁 Script executed:
# Check if there are any admin permission checks or decorators for admin endpoints
rg -n "@admin|admin_required|is_admin|is_superuser" --type=py | head -20Repository: DalgoT4D/DDP_backend
Length of output: 442
🏁 Script executed:
# Verify the queryset.values().count() behavior - check if this is a known pattern
cat -n ddpui/api/admin_portal/organizations_api.py | sed -n '14,25p'Repository: DalgoT4D/DDP_backend
Length of output: 409
🏁 Script executed:
# Check if there's an admin permission or role that should protect these endpoints
rg -n "admin|superuser" ddpui/auth.py -A 3 -B 3 | head -60Repository: DalgoT4D/DDP_backend
Length of output: 259
🏁 Script executed:
# Look at permission definitions to see what permissions exist
cat -n seed/003_role_permissions.json | head -100Repository: DalgoT4D/DDP_backend
Length of output: 2264
🏁 Script executed:
# Check how Ninja handles Http404 exceptions - see if there's custom exception handling
rg -B 5 -A 10 "exception_handler.*404\|Http404" ddpui/routes.pyRepository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Verify the type hint issue exists - check if Ruff is configured and would flag this
fd -type f "pyproject.toml\|setup.cfg\|ruff.toml" | xargs cat 2>/dev/nullRepository: DalgoT4D/DDP_backend
Length of output: 235
🏁 Script executed:
# Find permission definitions to see what permissions exist
rg -n "Permission.*slug\|permission.*=" --type=py | grep -i "org\|admin" | head -30Repository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Look at actual permission model/data
rg -n "class Permission\|permission_slug" --type=py -A 5 | head -50Repository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Check how Http404 is handled by Ninja - search for exception handlers or HTTP status responses
rg -n "404\|HttpException" ddpui/routes.py -B 2 -A 5Repository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Check the type hint for name parameter - verify if it's a typing issue
python3 << 'PYEOF'
# Check if "name: str = None" violates PEP 484
# According to PEP 484, Optional type should be used
# name: str = None should be name: Optional[str] = None
# or in Python 3.10+: name: str | None = None
print("Type hint 'name: str = None' violates PEP 484")
print("Should be: 'name: Optional[str] = None' or 'name: str | None = None'")
PYEOFRepository: DalgoT4D/DDP_backend
Length of output: 179
🏁 Script executed:
# Check if pyproject.toml exists and has Ruff configuration
find . -name "pyproject.toml" -o -name "setup.cfg" -o -name "ruff.toml" 2>/dev/null | xargs ls -la 2>/dev/nullRepository: DalgoT4D/DDP_backend
Length of output: 615
🏁 Script executed:
# Check Ruff configuration
cat ruff.tomlRepository: DalgoT4D/DDP_backend
Length of output: 1179
🏁 Script executed:
# Check pyproject.toml for any tool.ruff or tool.ninja configuration
cat pyproject.toml | grep -A 20 "\[tool"Repository: DalgoT4D/DDP_backend
Length of output: 960
🏁 Script executed:
# Look at how other endpoints handle 404 responses - specifically compare Http404 vs HttpError
rg -n "HttpError.*404\|Http404" --type=py -B 1 -A 1 | head -50Repository: DalgoT4D/DDP_backend
Length of output: 46
🏁 Script executed:
# Verify database query behavior - check if queryset.values().count() is inefficient
python3 << 'PYEOF'
# In Django ORM:
# queryset.values("id", "name", "slug") returns a ValuesQuerySet
# queryset.count() on any queryset runs a SQL COUNT(*) query
# list(orgs) materializes the ValuesQuerySet, running a SELECT query
# So yes, this is 2 separate DB queries:
# 1. SELECT COUNT(*) FROM org WHERE ...
# 2. SELECT id, name, slug FROM org WHERE ...
#
# Better approach:
# orgs_list = list(queryset.values("id", "name", "slug"))
# Then use len(orgs_list) for count - only 1 query
print("Confirmed: queryset.count() + list(orgs) = 2 DB queries")
print("Better: orgs_list = list(queryset.values(...)) and use len(orgs_list)")
PYEOFRepository: DalgoT4D/DDP_backend
Length of output: 190
Add admin permission check—unauthenticated enumeration of all organizations
Both endpoints lack the @has_permission() decorator standard across the codebase. Although CustomJwtAuthMiddleware validates the JWT and populates request.permissions, it enforces no admin role check. Any authenticated user—including regular org members—can call GET /api/admin/v1/organizations and enumerate all organization IDs, names, and slugs. Add @has_permission() with an appropriate admin permission (e.g., can_manage_organizations) to both endpoints before shipping.
Secondary issues:
- Lines 23–24: Inefficient dual queries—
queryset.count()executes aCOUNT(*), thenlist(orgs)runs a separateSELECT. Materialize the list first:orgs = list(queryset.values(...)), then uselen(orgs)for the count. - Line 10:
name: str = Noneviolates PEP 484. Usename: Optional[str] = Noneorname: str | None = None. - Line 37: Custom message in
Http404("Organization not found")is silently replaced with"Not Found"in production. UseHttpError(404, "Organization not found")to preserve the message.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 10-10: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 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 8 - 41, Both
endpoints (get_admin_organizations and get_single_org) are missing an admin
permission check and have a few other issues: add the
`@has_permission`("can_manage_organizations") decorator above both functions so
only admins can call them (request.permissions is set by CustomJwtAuthMiddleware
but does not enforce admin role), change the query parameter type to
Optional[str] (or str | None) for name in get_admin_organizations, materialize
the queryset results before counting by doing orgs =
list(Org.objects.filter(...).values("id","name","slug")) and then use len(orgs)
for the count instead of queryset.count(), and replace the Http404("Organization
not found") in get_single_org with HttpError(404, "Organization not found") so
the custom message is preserved; use the Org model and the has_permission symbol
to locate the code to update.
|
|
||
| @admin_org_router.get("/v1/organizations") | ||
| @admin_org_router.get("/v1/organizations/") | ||
| def get_admin_organizations(request, name: str = None): |
There was a problem hiding this comment.
Implicit Optional violates PEP 484 (Ruff RUF013)
🔧 Proposed fix
-def get_admin_organizations(request, name: str = None):
+def get_admin_organizations(request, name: str | None = None):📝 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.
| def get_admin_organizations(request, name: str = None): | |
| def get_admin_organizations(request, name: str | None = None): |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 10-10: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 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` at line 10, The function
get_admin_organizations uses a default None for parameter name without an
explicit Optional type which violates PEP 484; update the signature to annotate
name as Optional[str] (or str | None if the codebase uses Python 3.10+ union
syntax), and add the corresponding import (from typing import Optional) if
needed; specifically modify the get_admin_organizations definition and ensure
the file imports Optional so the default None is correctly typed.
| if not org: | ||
| raise Http404("Organization not found") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and verify the code
fd -t f organizations_api.pyRepository: DalgoT4D/DDP_backend
Length of output: 108
🏁 Script executed:
# Search for the specific code location
cat -n ddpui/api/admin_portal/organizations_api.py | sed -n '30,45p'Repository: DalgoT4D/DDP_backend
Length of output: 453
🏁 Script executed:
# Check for imports in the file
head -50 ddpui/api/admin_portal/organizations_api.pyRepository: DalgoT4D/DDP_backend
Length of output: 1057
🌐 Web query:
Django Ninja Http404 custom message production behavior
💡 Result:
In Django Ninja, when Http404 is raised inside an API operation (e.g., via get_object_or_404), the default exception handler _default_404 produces a JSON 404 response with {"detail": msg}, where msg = "Not Found" in production (DEBUG=False) and msg = "Not Found: <custom message from Http404>" in DEBUG=True. The custom message passed to Http404 (e.g., Http404("My custom message")) is ignored in production and only appended in DEBUG mode.[1][2] Note: This applies only to Http404 raised within Ninja operations. URL routing 404s (non-existent endpoint) use Django's default handler (typically a 404 HTML page or template in production).[3]
Citations:
- 1: https://github.com/vitalik/django-ninja/blob/master/ninja/errors.py
- 2: https://django-ninja.dev/guides/errors/
- 3: [BUG] 404 handler override not working as expected vitalik/django-ninja#1390
🌐 Web query:
Django Ninja HttpError 404 message handling
💡 Result:
In Django Ninja, “404 message handling” depends on how the 404 is produced: 1) 404 raised inside a Ninja endpoint (recommended for custom JSON) - Django Ninja registers a default exception handler for django.http.Http404. By default it returns a JSON response with status 404 and a detail message “Not Found”; if settings.DEBUG is True it appends the exception text/representation to that message [1][2]. - You can override this behavior by registering an exception handler for Http404 using @api.exception_handler(Http404) [1][2]. Example (returning your own message): @api.exception_handler(Http404) def page_not_found(request, exc): return api.create_response( request, {"message": "Please retry later"}, status=404,) This override is consistent with Ninja’s documented “override default exception handlers” mechanism [1][2]. 2) “URL not found” (no matching route) vs Http404 raised from inside an endpoint - Ninja “fully relies on django url router to find the right callback”, so a request that doesn’t match any Ninja-defined endpoint will typically fall through to Django’s normal 404 handling, not Ninja’s Http404 handler [3]. - Ninja’s Http404 exception handler is mainly for cases where you explicitly raise Http404 from within an operation (e.g., get_object_or_404 or raising Http404 yourself) [3]. Practical options when you need a custom body for API 404s - Add a catch-all endpoint that matches your API prefix (so Ninja can return a controlled 404). A community-proposed approach is to register a very last operation like /{path:endpoint} (include_in_schema=False) and raise Http404 from there, so it gets handled by Ninja’s Http404 logic/overrides [3]. - Alternatively, customize Django’s global handler for 404 pages (outside Ninja) if you truly need to change behavior for unmatched URLs (this will affect non-Ninja routes too, unless you condition on path) [3]. What message you’ll see with the default handler - Default Http404 response uses “Not Found” as detail, and if settings.DEBUG is True, it includes the exception info in the detail string [2][4]. Bottom line - To control the “message” for 404s in Ninja, prefer raising django.http.Http404 within your operations and override it with @api.exception_handler(Http404) [1][2]. - For unmatched URLs (no endpoint matched), Django’s router/404 handling is in play; use a catch-all Ninja endpoint under your API prefix if you need consistent JSON for those cases [3].
Citations:
- 1: https://django-ninja.rest-framework.com/guides/errors/
- 2: https://django-ninja.dev/guides/errors/
- 3: [BUG] 404 handler override not working as expected vitalik/django-ninja#1390
- 4: https://github.com/vitalik/django-ninja/blob/master/ninja/errors.py
🌐 Web query:
Django Ninja HttpError exception handler custom message
💡 Result:
You can control the “custom message” for Django Ninja’s HttpError in two ways: 1) Pass the message directly to HttpError (simplest) - HttpError takes a status code and a message: raise HttpError(503, "Service Unavailable. Please retry later.") [1] - By default Ninja’s HttpError handler returns JSON with a detail field set to the string form of the exception (str(exc)) [2]. Since HttpError.str returns the stored message, the message you pass is what appears in the response body as detail. [2] Example: from ninja.errors import HttpError @api.get("/some/resource") def some_operation(request): raise HttpError(503, "Please retry later") With the default handler, the response will be along the lines of: { "detail": "Please retry later" } (status code 503) [2] 2) Override HttpError’s exception handler and build your own payload/message - Ninja supports custom exception handlers via @api.exception_handler, which can override the defaults for given exception types. [1] - The docs explicitly describe registering handlers with @api.exception_handler and note Ninja includes a default handler for ninja.errors.HttpError. [1][2] Example: from ninja.errors import HttpError @api.exception_handler(HttpError) def http_error_handler(request, exc): return api.create_response( request, {"message": str(exc), "status": exc.status_code}, status=exc.status_code,) This replaces the default behavior (detail=str(exc)). [2] Notes / gotchas - One common customization is to include your own field name (e.g., "message") instead of using the default "detail". That requires overriding the HttpError handler. [2] - There is also a reported issue where Ninja may treat the HttpError message as Python code in some situations; if you see unexpected behavior, ensure the message is a plain string (e.g., consider quoting/escaping) or adjust your error handling accordingly. [3] So, for “HttpError exception handler custom message”, the direct answer is: provide the message as the 2nd argument to HttpError for the default response, or override @api.exception_handler(HttpError) if you want a fully custom response shape/message field name. [1][2]
Citations:
- 1: https://django-ninja.dev/guides/errors/
- 2: https://github.com/vitalik/django-ninja/blob/master/ninja/errors.py
- 3: HttpError evaluates message string as python code. vitalik/django-ninja#166
🌐 Web query:
Django Ninja HttpError 404 vs Http404 difference
💡 Result:
In Django Ninja, HttpError(404, "message") and Django's Http404 serve different purposes for handling 404 errors, though both result in a 404 response when raised inside Ninja operations [1][2][3]. HttpError is a Ninja-specific exception from ninja.errors, designed for throwing HTTP errors with any status code and custom message from anywhere in the code. Example: raise HttpError(404, "Resource not found") returns {"detail": "Resource not found"} with status 404 via Ninja's default handler _default_http_error [1][2]. Http404 is Django's standard 404 exception, often raised by get_object_or_404 when an object is not found. In Ninja, it has a dedicated default handler _default_404 that returns {"detail": "Not Found"} (or with traceback in DEBUG mode), status 404 [1][2][3]. Key differences: - HttpError supports any status code (e.g., 503) and includes the full custom message in the response [1][2]. - Http404 is 404-specific; its default response uses a generic "Not Found" message unless DEBUG=True [1][2]. - Both are caught by Ninja's exception handlers when raised in API operations, but URL routing 404s (non-existent paths) bypass Ninja handlers and use Django's default behavior [4]. Use HttpError for flexible API error responses; Http404 integrates with Django utilities like get_object_or_404 [1][3].
Citations:
- 1: https://django-ninja.rest-framework.com/guides/errors/
- 2: https://github.com/vitalik/django-ninja/blob/master/ninja/errors.py
- 3: https://django-ninja.dev/guides/errors/
- 4: [BUG] 404 handler override not working as expected vitalik/django-ninja#1390
Replace Http404 with HttpError(404, ...) to preserve the error message in production
Django Ninja's Http404 handler ignores custom messages in non-DEBUG mode and always returns {"detail": "Not Found"} to callers. The message "Organization not found" is silently dropped in production.
Use HttpError(404, ...) from ninja.errors instead—it preserves the custom message string in all environments via the _default_http_error handler.
🛠️ Proposed fix
+from ninja.errors import HttpError
from ninja import Router
from django.http import Http404
...
- raise Http404("Organization not found")
+ raise HttpError(404, "Organization not found")📝 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.
| if not org: | |
| raise Http404("Organization not found") | |
| from ninja.errors import HttpError | |
| from ninja import Router | |
| from django.http import Http404 |
| if not org: | |
| raise Http404("Organization not found") | |
| if not org: | |
| raise HttpError(404, "Organization not found") |
🤖 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 36 - 37, Replace
the Http404 raise with a ninja.errors HttpError so the custom message is
preserved in production: import HttpError from ninja.errors and change the raise
at the org existence check (the block that checks "if not org") to raise
HttpError(404, "Organization not found") instead of Http404("Organization not
found"), ensuring the error message is passed through to callers.
| @@ -25,10 +25,11 @@ class Migration(migrations.Migration): | |||
| ("connection_id", models.CharField(max_length=36)), | |||
There was a problem hiding this comment.
Missing migration for connection_id max_length change (36 → 50).
This migration still defines connection_id with max_length=36, but SyncStats.connection_id in ddpui/models/airbyte.py (line 11) is changed in this PR to max_length=50. Without a follow-up migration (e.g., AlterField), Django will detect schema drift, makemigrations --check will fail in CI, and the database column won't be updated on existing deployments.
A new migration is needed:
# In a new migration file, e.g., 0086_syncstats_connection_id_max_length.py
operations = [
migrations.AlterField(
model_name="syncstats",
name="connection_id",
field=models.CharField(max_length=50),
),
]🤖 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/migrations/0085_syncstats.py` at line 25, Add a new migration that
alters SyncStats.connection_id to match the model change (max_length 50): create
a migration (e.g., 0086) with an AlterField operation targeting
model_name="syncstats", name="connection_id" and set field to
models.CharField(max_length=50) so the DB schema matches
ddpui/models/airbyte.py's SyncStats.connection_id change; ensure the new
migration is included in the app migrations list and committed.
|
|
||
| org = models.ForeignKey(Org, on_delete=models.CASCADE) | ||
| connection_id = models.CharField(max_length=36) | ||
| connection_id = models.CharField(max_length=50) |
There was a problem hiding this comment.
connection_id max_length=50 is inconsistent with the rest of the codebase and lacks motivation.
Airbyte connection IDs are standard UUIDs (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx, always 36 chars). Every other model that stores a connection_id uses max_length=36:
ConnectionMeta.connection_id(org.py:296) —max_length=36ConnectionJob.connection_id(org.py:306) —max_length=36OrgSchemaChange.connection_id(org.py:322) —max_length=36OrgTask.connection_id(tasks.py:93) —max_length=36
Increasing this field to 50 without a documented reason and without corresponding changes to the related models creates a silent schema inconsistency. Unless there's a concrete case for IDs exceeding 36 chars, revert to max_length=36 to stay consistent.
🔧 Proposed revert
- connection_id = models.CharField(max_length=50)
+ connection_id = models.CharField(max_length=36)📝 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.
| connection_id = models.CharField(max_length=50) | |
| connection_id = models.CharField(max_length=36) |
🤖 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/models/airbyte.py` at line 11, Change the Airbyte model's connection_id
field from max_length=50 back to max_length=36 to match the rest of the
codebase; locate the connection_id declaration in ddpui.models.airbyte (the
connection_id = models.CharField(...) line) and set max_length=36 so it stays
consistent with ConnectionMeta.connection_id, ConnectionJob.connection_id,
OrgSchemaChange.connection_id and OrgTask.connection_id.
| status = models.TextField() | ||
| sync_type = models.CharField(choices=[("manual", "manual"), ("orchestrate", "orchestrate")]) | ||
|
|
||
| # ✅ FIXED LINE (added max_length) |
There was a problem hiding this comment.
Remove the development-artifact comment before merging.
# ✅ FIXED LINE (added max_length) is a scratch note, not documentation. It adds noise and will be misleading to future readers.
🧹 Proposed fix
- # ✅ FIXED LINE (added max_length)
sync_type = models.CharField(
max_length=20,
choices=[("manual", "manual"), ("orchestrate", "orchestrate")]
)📝 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.
| # ✅ FIXED LINE (added max_length) | |
| sync_type = models.CharField( | |
| max_length=20, | |
| choices=[("manual", "manual"), ("orchestrate", "orchestrate")] | |
| ) |
🤖 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/models/airbyte.py` at line 16, Remove the development-artifact comment
"# ✅ FIXED LINE (added max_length)" from the top of the Airbyte model in
ddpui/models/airbyte.py so only meaningful docstrings/comments remain; ensure
the actual change (adding max_length) stays in the model definition and run the
linter/formatting before committing.
1 participant
Overview
This PR introduces initial Admin Organization APIs and fixes a critical migration issue in
SyncStats.Features
?name=...)Fixes
max_lengthtoSyncStats.sync_typeTechnical Details
Orgmodel/api/admin/v1/organizationsEndpoints
GET /api/admin/v1/organizationsGET /api/admin/v1/organizations/{id}Impact
Future Scope
Summary by CodeRabbit