Skip to content

Require project_id on list endpoints to prevent full table scans #1133

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.

Sign up for GitHub

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

Draft
mihow wants to merge 1 commit into
base: main
Choose a base branch
from
+44 −43
Draft

Require project_id on list endpoints to prevent full table scans #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions ami/base/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,23 +64,33 @@ class ProjectMixin:
Mixin to help views extract the active project from the request.
"""

require_project = False # Project is optional
require_project = False # Project required for all actions
require_project_for_list = True # Project required for list actions
Comment on lines +67 to +68
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The inline comment on require_project is misleading: the attribute is False (project optional), but the comment says it's required for all actions. Please update the comment (or the default) so it matches the actual behavior to avoid confusion when configuring views.

Suggested change
require_project = False # Project required for all actions
require_project_for_list = True # Project required for list actions
require_project = False # If True, project is required for all actions; otherwise project is optional
require_project_for_list = True # If True, project is required for list actions

Copilot uses AI. Check for mistakes.
request: rest_framework.request.Request
kwargs: dict # This is is generated by DRF from the URL pattern /api/projects/{project_id:int}/

def get_active_project(self) -> Project | None:
"""
Instance method wrapper around the standalone get_active_project function.
Raises Http404 if project_id is required but not provided or the project does not exist.

Enforcement logic:
- If require_project is True, project_id is required for all actions.
- If require_project_for_list is True (default), project_id is required for list actions.
- Missing project_id when required raises ValidationError (400).
- Valid project_id pointing to nonexistent project raises Http404 (404).
"""
action = getattr(self, "action", None)
required = self.require_project or (self.require_project_for_list and action == "list")

project = get_active_project(
Comment on lines 72 to 85
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

ProjectMixin.get_active_project() docstring/behavior suggests require_project_for_list=True enforces project_id for list actions, but that enforcement only happens if the view actually calls self.get_active_project() during request handling. For example, ProjectViewSet inherits ProjectMixin but its list/get_queryset path never calls get_active_project(), so no 400 will be raised. Consider either (a) clarifying the docstring to state this is enforced only when get_active_project() is invoked, or (b) moving the enforcement into a hook that always runs for list actions (e.g., initial() / get_queryset() in the mixin) if the intent is truly "all list endpoints".

Copilot uses AI. Check for mistakes.
request=self.request,
kwargs=self.kwargs,
required=self.require_project,
required=required,
)

if not project and self.require_project:
# project_id was required but not provided or invalid
raise Http404("Project ID is required but was not provided or is invalid")
if not project and required:
# Missing project_id is already caught by ValidationError in get_active_project().
# This handles: valid project_id was provided but the project doesn't exist.
raise Http404("Project not found.")

return project
65 changes: 28 additions & 37 deletions ami/main/api/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,6 +1330,7 @@ class TaxonViewSet(DefaultViewSet, ProjectMixin):
API endpoint that allows taxa to be viewed or edited.
"""

require_project_for_list = False # Taxonomy is global, not per-project
queryset = Taxon.objects.all().defer("notes")
serializer_class = TaxonSerializer
filter_backends = DefaultViewSetMixin.filter_backends + [
Expand Down Expand Up @@ -1609,6 +1610,7 @@ def list(self, request, *args, **kwargs):


class TaxaListViewSet(viewsets.ModelViewSet, ProjectMixin):
require_project_for_list = False # Taxa lists are global
queryset = TaxaList.objects.all()

def get_queryset(self):
Expand All @@ -1622,6 +1624,7 @@ def get_queryset(self):


class TagViewSet(DefaultViewSet, ProjectMixin):
require_project_for_list = False # Tags include global tags
queryset = Tag.objects.all()
serializer_class = TagSerializer
filterset_fields = ["taxa"]
Expand Down Expand Up @@ -1678,6 +1681,7 @@ def get_serializer_class(self):

class SummaryView(GenericAPIView, ProjectMixin):
permission_classes = [IsActiveStaffOrReadOnly]
require_project = True # Unfiltered summary queries are too expensive
Comment on lines 1682 to +1684
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if ProjectViewSet or its parent classes call get_active_project in any list-related code path
echo "=== ProjectViewSet definition ==="
ast-grep --pattern 'class ProjectViewSet($_$$) {
  $$$
}'

echo ""
echo "=== Search for get_active_project calls in views.py ==="
rg -n 'get_active_project' ami/main/api/views.py

echo ""
echo "=== Check if pagination or other mixins call get_active_project ==="
rg -n 'get_active_project' ami/base/

Repository: RolnickLab/antenna

Length of output: 1472

🏁 Script executed:

#!/bin/bash
echo "=== Find ProjectViewSet class definition ==="
rg -n 'class ProjectViewSet' ami/main/api/views.py -A 5

echo ""
echo "=== Examine pagination code that calls get_active_project ==="
sed -n '25,35p' ami/base/pagination.py

echo ""
echo "=== Look for require_project_for_list in views.py ==="
rg -n 'require_project' ami/main/api/views.py | head -20

echo ""
echo "=== Check ProjectViewSet's parent classes and methods ==="
rg -n 'class ProjectViewSet' ami/main/api/views.py -A 20

Repository: RolnickLab/antenna

Length of output: 2070

🏁 Script executed:

#!/bin/bash
echo "=== Check DefaultViewSet definition ==="
rg -n 'class DefaultViewSet' ami/main/api/views.py -A 10

echo ""
echo "=== Check ProjectMixin and its get_active_project method ==="
rg -n 'class ProjectMixin' ami/base/views.py -A 20

echo ""
echo "=== Look at ProjectPagination ==="
rg -n 'class ProjectPagination' ami/base/pagination.py -B 2 -A 15

echo ""
echo "=== Check the full pagination _get_project method ==="
sed -n '27,35p' ami/base/pagination.py

echo ""
echo "=== Search for require_project usage pattern in the code ==="
rg -n 'require_project' ami/base/views.py

Repository: RolnickLab/antenna

Length of output: 2993

🏁 Script executed:

#!/bin/bash
echo "=== Look for ProjectViewSet URL routing ==="
rg -n 'ProjectViewSet' ami/main/api/ | grep -E '(router|urlpatterns|register)'

echo ""
echo "=== Check URL patterns file ==="
fd -e py 'url' ami/main/api/ | head -5

echo ""
echo "=== Check the get_active_project full implementation ==="
sed -n '72,100p' ami/base/views.py

echo ""
echo "=== Look at how require_project_for_list is used ==="
sed -n '67,95p' ami/base/views.py

Repository: RolnickLab/antenna

Length of output: 2710

🏁 Script executed:

#!/bin/bash
echo "=== Check how ProjectViewSet is registered in router ==="
fd -type f -name '*.py' ami/main/api/ -exec grep -l 'ProjectViewSet' {} \;

echo ""
echo "=== Look for router registration ==="
fd -type f -name 'urls.py' ami/ 2>/dev/null

echo ""
echo "=== Check if there's an api router file ==="
find ami/main/api -name '*.py' -type f | head -10

Repository: RolnickLab/antenna

Length of output: 463

🏁 Script executed:

#!/bin/bash
echo "=== Check if ProjectViewSet sets require_project or require_project_for_list ==="
sed -n '143,170p' ami/main/api/views.py

echo ""
echo "=== Check if there are any Django URL patterns ==="
rg -n 'path|url' ami/main/api/views.py | head -10

echo ""
echo "=== Check the ami/main/urls.py ==="
cat ami/main/urls.py

echo ""
echo "=== Check the api/schemas file for routing ==="
rg -n 'ProjectViewSet' ami/main/api/schemas.py

Repository: RolnickLab/antenna

Length of output: 1554

🏁 Script executed:

#!/bin/bash
echo "=== Find urls.py files ==="
find ami -name 'urls.py' -type f

echo ""
echo "=== Check if ProjectViewSet overrides require_project_for_list ==="
sed -n '143,180p' ami/main/api/views.py

echo ""
echo "=== Look for RestRouter in schemas ==="
rg -n 'Router\|ProjectViewSet' ami/main/api/schemas.py -A 2

echo ""
echo "=== Check if ProjectViewSet calls list action ==="
sed -n '143,200p' ami/main/api/views.py | grep -E 'def list|require_project'

Repository: RolnickLab/antenna

Length of output: 1544

🏁 Script executed:

#!/bin/bash
echo "=== Check if ProjectViewSet is at project-level endpoint or root endpoint ==="
rg -n 'register.*ProjectViewSet' ami/ -r '' 2>/dev/null || echo "Not found in rg"

echo ""
echo "=== Look at how DRF routes work - check if ProjectViewSet is nested under project ==="
cat ami/main/api/schemas.py | head -50

echo ""
echo "=== Check pagination more carefully - when does _get_project get called? ==="
sed -n '1,40p' ami/base/pagination.py

echo ""
echo "=== Look for list method in ProjectViewSet or DefaultViewSet ==="
sed -n '103,180p' ami/main/api/views.py

Repository: RolnickLab/antenna

Length of output: 4246

🏁 Script executed:

#!/bin/bash
echo "=== Check the get_active_project logic when required=True ==="
sed -n '12,60p' ami/base/views.py

echo ""
echo "=== Check if ProjectViewSet overrides require_project_for_list anywhere ==="
sed -n '143,250p' ami/main/api/views.py | grep -A 5 -B 5 'require_project'

echo ""
echo "=== Confirm how the kwargs are used for project_id extraction ==="
sed -n '12,50p' ami/base/views.py | head -40

Repository: RolnickLab/antenna

Length of output: 3557

🏁 Script executed:

#!/bin/bash
echo "=== Check if ProjectViewSet needs to override require_project_for_list ==="
echo "Looking at the actual behavior - does ProjectViewSet list need project_id?"
echo ""
echo "Show ProjectViewSet context again with more lines:"
sed -n '143,180p' ami/main/api/views.py

echo ""
echo "=== Check if there are any tests for ProjectViewSet list ==="
find . -name '*test*' -type f -name '*.py' -exec grep -l 'ProjectViewSet' {} \; 2>/dev/null | head -3

Repository: RolnickLab/antenna

Length of output: 1604

🏁 Script executed:

#!/bin/bash
echo "=== Look at similar root-level viewsets that set require_project_for_list = False ==="
rg -B 5 'require_project_for_list = False' ami/main/api/views.py

echo ""
echo "=== Understand the difference - where are these viewsets used? ==="
rg -B 15 'require_project_for_list = False' ami/main/api/views.py | grep 'class ' | head -5

echo ""
echo "=== Check if ProjectViewSet should also override this ==="
echo "ProjectViewSet doesn't override require_project_for_list, so it inherits the default."
echo "Let's verify pagination actually gets called during list:"
sed -n '10,15p' ami/base/pagination.py

Repository: RolnickLab/antenna

Length of output: 1495

ProjectViewSet is broken by the new require_project_for_list = True default and requires explicit override.

While ProjectViewSet's get_queryset() doesn't directly call get_active_project(), the pagination code does—LimitOffsetPaginationWithPermissions._get_project() explicitly calls it during get_paginated_response(). During a list action with the new default, required=True is passed to get_active_project(). Since ProjectViewSet is a root-level endpoint (not nested under a project_id), the project extraction fails and raises Http404.

ProjectViewSet should set require_project_for_list = False (like TaxonViewSet, TaxaListViewSet, and TagViewSet do) since it lists all projects globally, not per-project resources.

🧰 Tools
🪛 Ruff (0.15.0)

[warning] 1683-1683: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@ami/main/api/views.py` around lines 1682 - 1684, ProjectViewSet is broken by
the new default require_project_for_list = True causing
LimitOffsetPaginationWithPermissions._get_project() to call
get_active_project(required=True) and raise Http404 for root-level project
listing; to fix, explicitly set require_project_for_list = False on the
ProjectViewSet class (same pattern as TaxonViewSet/TaxaListViewSet/TagViewSet)
so list actions do not force project extraction, and ensure pagination via
LimitOffsetPaginationWithPermissions._get_project() and get_paginated_response()
will skip required project resolution for this view.


@extend_schema(parameters=[project_id_doc_param])
def get(self, request):
Comment on lines 1682 to 1687
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

SummaryView now requires project_id (require_project = True), but the OpenAPI parameter used here (project_id_doc_param) is defined with required=False, so the generated docs will still show the query param as optional. Consider overriding the schema for this endpoint with a required=True parameter (or introducing a separate required_project_id_doc_param) so the docs match the new 400 behavior.

Copilot uses AI. Check for mistakes.
Expand All @@ -1686,43 +1690,30 @@ def get(self, request):
"""
user = request.user
project = self.get_active_project()
if project:
data = {
"projects_count": Project.objects.visible_for_user( # type: ignore
user
).count(), # @TODO filter by current user, here and everywhere!
"deployments_count": Deployment.objects.visible_for_user(user) # type: ignore
.filter(project=project)
.count(),
"events_count": Event.objects.visible_for_user(user) # type: ignore
.filter(deployment__project=project, deployment__isnull=False)
.count(),
"captures_count": SourceImage.objects.visible_for_user(user) # type: ignore
.filter(deployment__project=project)
.count(),
# "detections_count": Detection.objects.filter(occurrence__project=project).count(),
"occurrences_count": Occurrence.objects.visible_for_user(user) # type: ignore
.apply_default_filters(project=project, request=self.request) # type: ignore
.valid()
.filter(project=project)
.count(), # type: ignore
"taxa_count": Occurrence.objects.visible_for_user(user) # type: ignore
.apply_default_filters(project=project, request=self.request) # type: ignore
.unique_taxa(project=project)
.count(),
}
else:
data = {
"projects_count": Project.objects.visible_for_user(user).count(), # type: ignore
"deployments_count": Deployment.objects.visible_for_user(user).count(), # type: ignore
"events_count": Event.objects.visible_for_user(user) # type: ignore
.filter(deployment__isnull=False)
.count(),
"captures_count": SourceImage.objects.visible_for_user(user).count(), # type: ignore
"occurrences_count": Occurrence.objects.valid().visible_for_user(user).count(), # type: ignore
"taxa_count": Occurrence.objects.visible_for_user(user).unique_taxa().count(), # type: ignore
"last_updated": timezone.now(),
}
data = {
"projects_count": Project.objects.visible_for_user( # type: ignore
user
).count(), # @TODO filter by current user, here and everywhere!
"deployments_count": Deployment.objects.visible_for_user(user) # type: ignore
.filter(project=project)
.count(),
"events_count": Event.objects.visible_for_user(user) # type: ignore
.filter(deployment__project=project, deployment__isnull=False)
.count(),
"captures_count": SourceImage.objects.visible_for_user(user) # type: ignore
.filter(deployment__project=project)
.count(),
# "detections_count": Detection.objects.filter(occurrence__project=project).count(),
"occurrences_count": Occurrence.objects.visible_for_user(user) # type: ignore
.apply_default_filters(project=project, request=self.request) # type: ignore
.valid()
.filter(project=project)
.count(), # type: ignore
"taxa_count": Occurrence.objects.visible_for_user(user) # type: ignore
.apply_default_filters(project=project, request=self.request) # type: ignore
.unique_taxa(project=project)
.count(),
}

aliases = {
"num_sessions": data["events_count"],
Expand Down
Loading