-
Notifications
You must be signed in to change notification settings - Fork 99
feat(admin): add organization APIs and fix SyncStats migration issue #1330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,42 @@ | ||||||||||||||||||||
| from ninja import Router | ||||||||||||||||||||
| from django.http import Http404 | ||||||||||||||||||||
| from ddpui.models.org import Org | ||||||||||||||||||||
|
|
||||||||||||||||||||
| admin_org_router = Router() | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| @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") | ||||||||||||||||||||
|
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
💡 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:
🌐 Web query:
💡 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 Citations:
🌐 Web query:
💡 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 Citations:
🌐 Web query:
💡 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:
Replace Django Ninja's Use 🛠️ 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
Suggested change
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| return { | ||||||||||||||||||||
| "success": True, | ||||||||||||||||||||
| "data": org | ||||||||||||||||||||
|
Comment on lines
+8
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 Secondary issues:
🧰 Tools🪛 Ruff (0.15.12)[warning] 10-10: PEP 484 prohibits implicit Convert to (RUF013) 🤖 Prompt for AI Agents |
||||||||||||||||||||
| } | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,10 +25,11 @@ class Migration(migrations.Migration): | |
| ("connection_id", models.CharField(max_length=36)), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing migration for This migration still defines 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 |
||
| ("attempt", models.IntegerField(default=0)), | ||
| ("status", models.TextField()), | ||
| ( | ||
| ( | ||
| "sync_type", | ||
| models.CharField( | ||
| choices=[("manual", "manual"), ("orchestrate", "orchestrate")] | ||
| max_length=20, | ||
| choices=[("manual", "manual"), ("orchestrate", "orchestrate")], | ||
| ), | ||
| ), | ||
| ("sync_time", models.DateTimeField()), | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,11 +8,17 @@ class SyncStats(models.Model): | |||||||||||
| """single table to track connection sync stats""" | ||||||||||||
|
|
||||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Airbyte connection IDs are standard UUIDs (
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 🔧 Proposed revert- connection_id = models.CharField(max_length=50)
+ connection_id = models.CharField(max_length=36)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| job_id = models.IntegerField(null=True) | ||||||||||||
| attempt = models.IntegerField(default=0) | ||||||||||||
| status = models.TextField() | ||||||||||||
| sync_type = models.CharField(choices=[("manual", "manual"), ("orchestrate", "orchestrate")]) | ||||||||||||
|
|
||||||||||||
| # ✅ FIXED LINE (added max_length) | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the development-artifact comment before merging.
🧹 Proposed fix- # ✅ FIXED LINE (added max_length)
sync_type = models.CharField(
max_length=20,
choices=[("manual", "manual"), ("orchestrate", "orchestrate")]
)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| 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) | ||||||||||||
|
|
@@ -66,7 +72,7 @@ class AirbyteJob(models.Model): | |||||||||||
| started_at = models.DateTimeField(null=True) # because the api spec says this will be optional | ||||||||||||
| ended_at = models.DateTimeField( | ||||||||||||
| null=True | ||||||||||||
| ) # when the job ended; can be null if we pull or sync an ongonig job | ||||||||||||
| ) # when the job ended; can be null if we pull or sync an ongoing job | ||||||||||||
| created_at = models.DateTimeField() # when the job was created in airbyte | ||||||||||||
| updated_at = models.DateTimeField(auto_now=True) # when the django record was last updated | ||||||||||||
|
|
||||||||||||
|
|
@@ -109,4 +115,4 @@ def latest_failed_attempt_id(self): | |||||||||||
| if failed_attempts: | ||||||||||||
| return max(failed_attempts, key=lambda x: x["id"])["id"] | ||||||||||||
|
|
||||||||||||
| return None | ||||||||||||
| return None | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit
Optionalviolates PEP 484 (Ruff RUF013)🔧 Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 10-10: PEP 484 prohibits implicit
OptionalConvert to
T | None(RUF013)
🤖 Prompt for AI Agents