fix(api): resolve via ModelAdmin.get_object (honour consumer overrides) (#187)

`load_object_or_none` (used by detail GET, update PATCH, bulk PATCH,
delete) resolved the target via `get_queryset().get(pk=pk)`. Django's
own change view resolves via `ModelAdmin.get_object(request, object_id)`
— and `get_object` is a documented extension point consumers override.

Django's default `get_object` *is* `get_queryset().get(...)`, so the
default security posture is unchanged. But a consumer that overrides
`get_object` to bypass a list-only filter (so an individual record
stays openable even when hidden from the list) was not honoured: the
SPA 404'd a row the legacy admin opens.

Observed in the laminr pilot: `LoanPackageAdmin.get_queryset` excludes
test-tenant packages (list scoping), but its `get_object` deliberately
bypasses that filter for the change view. The SPA detail 404'd those
packages; the legacy admin opens them.

Fix: `load_object_or_none` now calls `model_admin.get_object(request,
str(pk))`. The views still gate the returned object on
`has_view_permission` / `has_change_permission` / `has_delete_permission`,
so using `get_object` does NOT widen access — it only fixes *which
object resolves*, consistent with Django. Defensive except still
collapses DoesNotExist / ValidationError / ValueError / TypeError to
None → 404 (never 500).

Test (test_detail.py::test_detail_resolves_via_get_object_not_get_queryset):
get_queryset returns none() while get_object returns the row; detail
must 200. 52/52 detail+update+bulk+delete tests pass.

Co-authored-by: Martin Castro Laminrs <mcastro@laminr.ai>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 3 people

django_admin_react/api/writes.py

@@ -38,6 +38,7 @@
 from typing import Any
 
 from django.contrib.admin.options import ModelAdmin
+from django.core.exceptions import ValidationError
 from django.db.models import ForeignKey
 from django.db.models import ManyToManyField
 from django.db.models import Model
@@ -138,24 +139,34 @@ def load_object_or_none(
     request: HttpRequest,
     pk: Any,
 ) -> Model | None:
-    """Fetch one row through the admin's queryset, or ``None`` on miss.
-
-    Three lookup failures collapse to ``None`` (callers convert to
-    404, never to 500):
-
-    - ``DoesNotExist`` — pk valid but no matching row, or the row was
-      filtered out by ``ModelAdmin.get_queryset(request)``.
-    - ``ValueError`` — pk could not be parsed for the field's type
-      (e.g., ``"abc"`` against an ``IntegerField``).
-    - ``TypeError`` — similar parse failure for a custom pk type.
-
-    Centralizing this here means every "load or 404" call site has
-    the same exception surface and the same security posture
-    (queryset never bypassed, parse errors never leak).
+    """Fetch one row through ``ModelAdmin.get_object``, or ``None`` on miss.
+
+    Uses ``ModelAdmin.get_object(request, pk)`` — exactly what Django's
+    own change view calls — rather than ``get_queryset().get(pk=pk)``.
+    Django's default ``get_object`` *is* ``get_queryset().get(...)``, so
+    the default security posture is unchanged. But a consumer that
+    overrides ``get_object`` (a documented Django extension point) gets
+    that override honoured here too — matching the legacy admin. A real
+    example: an admin whose ``get_queryset`` hides rows for list
+    performance / scoping but whose ``get_object`` deliberately bypasses
+    that filter so an individual record is still openable. Resolving
+    detail via ``get_queryset`` would 404 such a row even though the
+    legacy admin opens it.
+
+    The view still gates the returned object on ``has_view_permission``
+    (see ``detail.py`` / ``update.py`` / ``destroy.py``), so using
+    ``get_object`` does not widen access — it only fixes *which object
+    resolves*, consistent with Django.
+
+    Failures collapse to ``None`` (callers convert to 404, never 500):
+    ``DoesNotExist`` (no row / filtered out), ``ValidationError`` /
+    ``ValueError`` / ``TypeError`` (pk unparseable for the field type).
+    Django's stock ``get_object`` already returns ``None`` on these, but
+    a consumer override might raise, so we stay defensive.
     """
     try:
-        return model_admin.get_queryset(request).get(pk=pk)
-    except (model.DoesNotExist, ValueError, TypeError):
+        return model_admin.get_object(request, str(pk))
+    except (model.DoesNotExist, ValidationError, ValueError, TypeError):
         return None
 
 

tests/test_detail.py

@@ -18,6 +18,38 @@ def _url(pk: object) -> str:
     return f"/admin-react/api/v1/auth/group/{pk}/"
 
 
+@pytest.mark.django_db
+def test_detail_resolves_via_get_object_not_get_queryset(superuser_client: Client) -> None:
+    """The detail view must resolve the object through
+    ``ModelAdmin.get_object`` (what Django's change view uses), not
+    ``get_queryset().get()``.
+
+    A consumer may override ``get_object`` to bypass a filter that
+    ``get_queryset`` applies for list-view scoping/performance — so an
+    individual record stays openable even though it's hidden from the
+    list. Observed in the laminr pilot: ``LoanPackageAdmin.get_queryset``
+    excludes test-tenant packages, but its ``get_object`` deliberately
+    bypasses that so the change view still opens them. Resolving detail
+    via ``get_queryset`` 404'd such rows.
+
+    Here: ``get_queryset`` returns ``none()`` (hides everything) while
+    ``get_object`` returns the real row. The detail view must 200.
+    """
+    g = Group.objects.create(name="hidden-from-list")
+
+    with admin_override(
+        Group,
+        get_queryset=lambda self, request: Group.objects.none(),
+        get_object=lambda self, request, object_id, from_field=None: Group.objects.filter(
+            pk=object_id
+        ).first(),
+    ):
+        response = superuser_client.get(_url(g.pk))
+
+    assert response.status_code == 200, response.content
+    assert response.json()["pk"] == g.pk
+
+
 @pytest.mark.django_db
 def test_detail_calls_get_form_with_change_true(superuser_client: Client) -> None:
     """Regression: the detail view must call ``get_form(..., change=True)``