diff --git a/src/django_smartbase_admin/actions/admin_action_list.py b/src/django_smartbase_admin/actions/admin_action_list.py index ee2b9eaf..67bd74b3 100644 --- a/src/django_smartbase_admin/actions/admin_action_list.py +++ b/src/django_smartbase_admin/actions/admin_action_list.py @@ -149,24 +149,19 @@ def init_column_fields(self) -> None: def get_filters(self): return [field for field in self.column_fields if field.filter_widget] - def get_tabulator_columns_add_id_column_if_missing(self, add_id_column=True): + def get_tabulator_columns(self): + """Serialized Tabulator columns + the pk field name (the frontend's + ``tableIdColumnName``).""" columns_serialized = [] id_column_name = None for field in self.column_fields: if getattr(field.model_field, "primary_key", False): id_column_name = field.field columns_serialized.append(field.serialize_tabulator()) - # Add ID column in case there is none - if add_id_column and not id_column_name: - model_id_field = self.get_pk_field() - id_column_name = model_id_field.name - id_field = self.view.auto_create_field_from_model_field(model_id_field) - id_field.title = "ID" - id_field.list_visible = False - id_field.init_field_static( - self.view, self.threadsafe_request.request_data.configuration - ) - columns_serialized = [id_field.serialize_tabulator()] + columns_serialized + if id_column_name is None: + # No column is the pk (e.g. an annotation addresses it instead); + # the pk value is still in every row, so name it directly. + id_column_name = self.get_pk_field().name return columns_serialized, id_column_name def get_excel_columns(self): @@ -208,7 +203,7 @@ def get_template_data(self): "AJAX_NOTIFICATIONS_KEY": SB_ADMIN_AJAX_NOTIFICATIONS_KEY, } - columns, id_column_name = self.get_tabulator_columns_add_id_column_if_missing() + columns, id_column_name = self.get_tabulator_columns() row_actions = self.view.get_sbadmin_row_actions_processed( self.threadsafe_request ) diff --git a/src/django_smartbase_admin/audit/manager.py b/src/django_smartbase_admin/audit/manager.py index 9f72cd8b..d2b9ef56 100644 --- a/src/django_smartbase_admin/audit/manager.py +++ b/src/django_smartbase_admin/audit/manager.py @@ -142,15 +142,17 @@ def _extract_fk_affected(model, changes: dict) -> list[dict]: old_repr = None new_repr = None - # Add old value if present + # Add old value if present. ``old_val`` / ``new_val`` arrive already + # JSON-safe from serialization (int PKs stay int, UUID/char PKs are + # str) — use them as-is. Casting to int() crashed on non-int PKs. if old_val: obj_repr = old_repr or _get_object_repr(related_model, old_val) - affected.append({"ct": ct_label, "id": int(old_val), "repr": obj_repr}) + affected.append({"ct": ct_label, "id": old_val, "repr": obj_repr}) # Add new value if present and different if new_val and new_val != old_val: obj_repr = new_repr or _get_object_repr(related_model, new_val) - affected.append({"ct": ct_label, "id": int(new_val), "repr": obj_repr}) + affected.append({"ct": ct_label, "id": new_val, "repr": obj_repr}) return affected diff --git a/src/django_smartbase_admin/audit/tests/test_action_processing.py b/src/django_smartbase_admin/audit/tests/test_action_processing.py index 2d2b06bc..21474e7b 100644 --- a/src/django_smartbase_admin/audit/tests/test_action_processing.py +++ b/src/django_smartbase_admin/audit/tests/test_action_processing.py @@ -115,7 +115,7 @@ def get_pk_field(self): def get_filters(self): return [] - def get_tabulator_columns_add_id_column_if_missing(self, add_id_column=True): + def get_tabulator_columns(self): return [], "id" diff --git a/src/django_smartbase_admin/audit/tests/test_fk_affected.py b/src/django_smartbase_admin/audit/tests/test_fk_affected.py new file mode 100644 index 00000000..b23716ea --- /dev/null +++ b/src/django_smartbase_admin/audit/tests/test_fk_affected.py @@ -0,0 +1,44 @@ +"""``_extract_fk_affected`` must keep FK pks as they arrive (already +JSON-safe from serialization): int pks stay int, and non-int pks (UUID / +char) are kept as their string form instead of crashing on ``int(...)`` — +the regression that logged a noisy "Failed to create audit log" whenever a +FK pointed at a UUID-keyed model. +""" + +from __future__ import annotations + +from django.contrib.auth.models import Permission +from django.test import SimpleTestCase + +from django_smartbase_admin.audit.manager import _extract_fk_affected + +# ``Permission.content_type`` is a ForeignKey; the related model's own pk +# type is irrelevant to the bug — only the value handed in matters. + + +class ExtractFkAffectedTests(SimpleTestCase): + def test_uuid_string_pk_kept_not_cast_to_int(self): + uid = "aa57f521-1c2b-4f3a-9d4e-5f6a7b8c9d0e" + affected = _extract_fk_affected( + Permission, + {"content_type": {"old": None, "new": uid, "new_display": "Queue X"}}, + ) + self.assertEqual( + affected, + [{"ct": "contenttypes.contenttype", "id": uid, "repr": "Queue X"}], + ) + + def test_integer_pk_preserved_as_int(self): + affected = _extract_fk_affected( + Permission, + { + "content_type": { + "old": 7, + "new": 9, + "old_display": "A", + "new_display": "B", + } + }, + ) + self.assertEqual([a["id"] for a in affected], [7, 9]) + self.assertTrue(all(isinstance(a["id"], int) for a in affected)) diff --git a/src/django_smartbase_admin/engine/admin_base_view.py b/src/django_smartbase_admin/engine/admin_base_view.py index ee20a2fa..1d03f887 100644 --- a/src/django_smartbase_admin/engine/admin_base_view.py +++ b/src/django_smartbase_admin/engine/admin_base_view.py @@ -290,7 +290,7 @@ def init_view_dynamic(self, request, request_data=None, **kwargs): def get_field_map(self, request) -> dict[str, "SBAdminField"]: return self.init_fields_cache( - self.get_sbadmin_list_display(request), + self.get_effective_list_display(request), request.request_data.configuration, request=request, ) @@ -826,6 +826,69 @@ def init_view_dynamic(self, request, request_data=None, **kwargs) -> None: def get_sbadmin_list_display(self, request) -> list[str] | list: return self.sbadmin_list_display or self.list_display or [] + def get_effective_list_display(self, request) -> list: + """``get_sbadmin_list_display`` augmented with a synthetic, + read-only primary-key column when the admin hasn't declared one. + + The list pipeline already emits every row's pk under a normalized + ``"id"`` key, but without a matching field nothing can *select*, + *sort*, or *filter* by it — the field map rejects the name (this is + what made MCP ``list_rows(fields=["id"])`` fail, and why the browser + only ever got a display-only id column). Promoting the pk to a real, + hidden column closes that round-trip through the normal column / + sort / filter machinery, for the UI and MCP alike. Hidden by default + (``list_visible=False``) so it doesn't clutter the grid. With the pk + now a real column, :meth:`get_tabulator_columns` reports its id-column + name directly instead of grafting on a display-only one. + """ + list_display = list(self.get_sbadmin_list_display(request) or []) + pk_field = self._build_synthetic_pk_field(list_display) + if pk_field is not None: + list_display.append(pk_field) + return list_display + + def _build_synthetic_pk_field(self, list_display): + """Synthetic read-only pk column for the list — the canonical ``id`` + column the agent uses for select / sort / filter, and the frontend for + row identity. + + Returns ``None`` when a declared column already addresses the pk — by + name, by an explicit ``filter_field`` pointing at it, or via a method + ``@admin.display(ordering="id")`` — since the author has already + exposed it and a second column would be redundant; or when the pk + can't be resolved (e.g. a composite pk). + """ + from django_smartbase_admin.engine.filter_widgets import ( + PrimaryKeyFilterWidget, + ) + + model = getattr(self, "model", None) + if model is None: + return None + pk_model_field = getattr(model._meta, "pk", None) + pk_name = getattr(pk_model_field, "name", None) + if not pk_name: + return None + + for entry in list_display: + name = getattr(entry, "name", entry) + method = getattr(self, name, None) if isinstance(name, str) else None + if ( + name == pk_name + or getattr(entry, "filter_field", None) == pk_name + or ( + callable(method) + and getattr(method, "admin_order_field", None) == pk_name + ) + ): + return None + + field = self.auto_create_field_from_model_field(pk_model_field) + field.title = "ID" + field.list_visible = False # off by default; still select/sort/filterable + field.filter_widget = PrimaryKeyFilterWidget() + return field + def _register_list_filter_autocomplete(self, request) -> None: field_map = self.init_fields_cache( self.get_sbadmin_list_display(request), @@ -840,7 +903,7 @@ def _register_list_filter_autocomplete(self, request) -> None: def get_list_display(self, request) -> list[str]: return [ getattr(field, "name", field) - for field in self.get_sbadmin_list_display(request) + for field in self.get_effective_list_display(request) ] def get_search_fields(self, request): diff --git a/src/django_smartbase_admin/engine/filter_widgets.py b/src/django_smartbase_admin/engine/filter_widgets.py index 82ed3066..28de17b0 100644 --- a/src/django_smartbase_admin/engine/filter_widgets.py +++ b/src/django_smartbase_admin/engine/filter_widgets.py @@ -1,8 +1,9 @@ import json +import re from datetime import datetime, timedelta from django import forms -from django.core.exceptions import ImproperlyConfigured +from django.core.exceptions import ImproperlyConfigured, ValidationError from django.contrib.postgres.fields import ArrayField from django.db.models import Q, fields, FilteredRelation, Count from django.http import JsonResponse @@ -386,6 +387,85 @@ def get_advanced_filter_operators(self): ] +class PrimaryKeyFilterWidget(SBAdminFilterWidget): + """Exact-match / ``IN`` filter on a primary key. + + Accepts a single pk or a list of pks and filters + ``__in=[...]``. Attached to the synthetic primary-key + column the list view exposes when the admin hasn't declared one, so a + row can be re-fetched by the ``id`` it already shows — in the browser + (a text box: one id, or several comma-separated) and over MCP (a pk or + a list of pks). Not auto-detected for any model field type; only used + when set explicitly. + """ + + # A pk is typed, not picked — reuse the plain text-input template. + template_name = "sb_admin/filter_widgets/string_field.html" + close_dropdown_on_change = True + + _SEPARATORS = re.compile(r"[\s,;]+") + + def parse_value_from_input(self, request, filter_value): + """Flat list of pks. Accepts a native scalar/list (MCP) or a string of + pks separated by commas, whitespace, or semicolons (the UI text box). + Values are coerced through the pk field; ones that can't be this pk + (e.g. ``"abc"`` for an int pk) are dropped, not handed to the ORM.""" + value = filter_value + if isinstance(value, str): + value = [part for part in self._SEPARATORS.split(value.strip()) if part] + if not isinstance(value, list): + value = [value] + parsed = [] + for item in value: + if item in forms.Field.empty_values: + continue + if self.model_field is not None: + try: + item = self.model_field.get_prep_value(item) + except (ValueError, TypeError, ValidationError): + continue + parsed.append(item) + return parsed + + def get_base_filter_query_for_parsed_value(self, request, parsed_value): + if not parsed_value: + # Truly-empty values are dropped before the widget runs, so an + # empty list here is non-empty input that coerced to nothing — + # invalid. Fail closed so an active filter can't silently widen to + # the whole table (which would also widen bulk actions / exports). + return Q(pk__in=[]) + return Q(**{f"{self.field.filter_field}__in": parsed_value}) + + def validate_value(self, value) -> None: + items = value if isinstance(value, list) else [value] + for item in items: + # ``bool`` is an ``int`` subclass but never a valid pk here. + if isinstance(item, bool) or not isinstance(item, (int, str)): + raise ValueError( + "PrimaryKeyFilterWidget expects a primary key or a list of " + "primary keys (int or str), got " + f"{type(item).__name__}: {item!r}" + ) + # Reject up front so MCP gets a clear error, not an ORM failure + # mid-query. + if self.model_field is not None: + try: + self.model_field.get_prep_value(item) + except (ValueError, TypeError, ValidationError): + raise ValueError( + f"PrimaryKeyFilterWidget got {item!r}, not a valid " + f"{self.model_field.get_internal_type()} id" + ) + + def get_advanced_filter_operators(self): + return [ + AllOperators.IN, + AllOperators.NOT_IN, + AllOperators.IS_NULL, + AllOperators.IS_NOT_NULL, + ] + + class NumberRangeFilterWidget(AutocompleteParseMixin, SBAdminFilterWidget): template_name = "sb_admin/filter_widgets/number_range_field.html" diff --git a/src/django_smartbase_admin/mcp/mcp.py b/src/django_smartbase_admin/mcp/mcp.py index 00b83067..7c49bf5d 100644 --- a/src/django_smartbase_admin/mcp/mcp.py +++ b/src/django_smartbase_admin/mcp/mcp.py @@ -161,7 +161,9 @@ def _validate_sort(admin, request, sort, field_map=None) -> None: field_map = admin.get_field_map(request) field_map = field_map or {} for entry in sort: - if not isinstance(entry, dict) or "field" not in entry: + # ``dir`` is required, not defaulted — the list pipeline reads + # ``sort['dir']`` directly, so a missing key must fail clearly here. + if not isinstance(entry, dict) or "field" not in entry or "dir" not in entry: raise ValueError( "Each sort entry must be {'field': , 'dir': " f"'asc'|'desc'}}, got {entry!r}" @@ -172,9 +174,8 @@ def _validate_sort(admin, request, sort, field_map=None) -> None: f"Admin {admin.get_id()!r} cannot sort by {name!r}; " f"sortable fields: {sorted(field_map)}." ) - direction = entry.get("dir", "asc") - if direction not in ("asc", "desc"): - raise ValueError(f"sort dir must be 'asc' or 'desc', got {direction!r}") + if entry["dir"] not in ("asc", "desc"): + raise ValueError(f"sort dir must be 'asc' or 'desc', got {entry['dir']!r}") def _decode_preset_url_params(url_params) -> dict: @@ -517,6 +518,9 @@ def list_rows( ``list_admins["admin_views"][].fields[].name``. Every row also carries a normalized ``"id"`` key for row identity, regardless of the model's pk field name (so ``row["id"]`` is always safe). + ``"id"`` is itself a selectable, sortable, and filterable column + — filter it with one id or a list of ids to re-fetch the exact + rows behind ids you already saw. filter_data: ``{field: value}`` mapping. Each **key** is a column ``name`` from ``list_admins["admin_views"][].fields[].name`` — the same identifier ``fields`` and ``sort`` use. (Keys returned @@ -590,6 +594,25 @@ def list_rows( # Built once and shared — ``get_field_map`` rebuilds + clones on # every call. field_map = admin.get_field_map(request) + # Accept ``"id"`` as an alias for a differently-named pk on input, so + # the documented refetch works (output always mirrors the pk to + # ``"id"``). No-op for the usual ``id``-pk model. + pk_name = admin.model._meta.pk.name + if pk_name != "id" and "id" not in field_map: + fields = [pk_name if f == "id" else f for f in fields] + if filter_data: + filter_data = { + (pk_name if k == "id" else k): v for k, v in filter_data.items() + } + if sort: + sort = [ + ( + {**s, "field": pk_name} + if isinstance(s, dict) and s.get("field") == "id" + else s + ) + for s in sort + ] filter_data = _normalize_filter_keys(filter_data, field_map) _validate_filter_data(admin, request, filter_data, field_map) _validate_sort(admin, request, sort, field_map) diff --git a/src/django_smartbase_admin/mcp/schema.py b/src/django_smartbase_admin/mcp/schema.py index 76fff554..4b5cf3fa 100644 --- a/src/django_smartbase_admin/mcp/schema.py +++ b/src/django_smartbase_admin/mcp/schema.py @@ -24,6 +24,7 @@ DateFilterWidget, MultipleChoiceFilterWidget, NumberRangeFilterWidget, + PrimaryKeyFilterWidget, ) from django_smartbase_admin.mcp.actions import collect_action_entries from django_smartbase_admin.mcp.service import SBAdminMCPDetailService @@ -46,6 +47,7 @@ # Order matters: ``MultipleChoiceFilterWidget`` subclasses # ``ChoiceFilterWidget``, so it must be checked first. _WIDGET_CATEGORIES: tuple[tuple[type, str], ...] = ( + (PrimaryKeyFilterWidget, "PrimaryKeyFilterWidget"), (DateFilterWidget, "DateFilterWidget"), (NumberRangeFilterWidget, "NumberRangeFilterWidget"), (BooleanFilterWidget, "BooleanFilterWidget"), @@ -55,6 +57,13 @@ ) WIDGET_SHAPES: dict[str, dict] = { + "PrimaryKeyFilterWidget": { + "value_shape": ( + "an id, or a list of ids, matched exactly (SQL IN); pass the " + "ids you already saw in the data to re-fetch those exact rows" + ), + "example": [42, 7, 13], + }, "DateFilterWidget": { "value_shape": ( "[start, end] — list of two ISO-8601 dates ('YYYY-MM-DD'); " diff --git a/src/django_smartbase_admin/mcp/tests/test_filter_validation.py b/src/django_smartbase_admin/mcp/tests/test_filter_validation.py index a929b213..840fa891 100644 --- a/src/django_smartbase_admin/mcp/tests/test_filter_validation.py +++ b/src/django_smartbase_admin/mcp/tests/test_filter_validation.py @@ -246,3 +246,28 @@ def q(value): ("uploaded_at__lte", d2 + timedelta(days=1)), ], ) + + +class ValidateSortTests(TestCase): + """``dir`` is required (not defaulted): the list pipeline reads + ``sort['dir']`` directly, so a bad/missing one must be caught in + validation, not surface as a ``KeyError`` mid-query.""" + + def _validate(self, sort): + from django_smartbase_admin.mcp.mcp import _validate_sort + + # ``field_map`` provided, so ``admin``/``request`` go unused. + _validate_sort(None, None, sort, field_map={"name": object()}) + + def test_missing_dir_raises_clear_error(self): + with self.assertRaises(ValueError) as ctx: + self._validate([{"field": "name"}]) + self.assertIn("dir", str(ctx.exception)) + + def test_invalid_dir_raises(self): + with self.assertRaises(ValueError) as ctx: + self._validate([{"field": "name", "dir": "up"}]) + self.assertIn("asc", str(ctx.exception)) + + def test_valid_sort_passes(self): + self._validate([{"field": "name", "dir": "desc"}]) # no raise diff --git a/src/django_smartbase_admin/mcp/tests/test_pk_field.py b/src/django_smartbase_admin/mcp/tests/test_pk_field.py new file mode 100644 index 00000000..548e85f7 --- /dev/null +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -0,0 +1,414 @@ +"""A model's primary key is exposed as a real, filterable/sortable column +for MCP even when the admin doesn't declare it — closing the round-trip +between the ``id`` every row carries and what ``list_rows`` accepts as input. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock + +from django.contrib import admin +from django.contrib.sessions.models import Session +from django.db.models import F +from django.test import TestCase, override_settings +from django.urls import path +from django.utils import timezone +from filer.models import Folder + +from django_smartbase_admin.actions.admin_action_list import SBAdminListAction +from django_smartbase_admin.admin.admin_base import SBAdmin +from django_smartbase_admin.admin.site import sb_admin_site +from django_smartbase_admin.engine.field import SBAdminField +from django_smartbase_admin.mcp.mcp import SBAdminTools +from django_smartbase_admin.mcp.tests._common import ( + MCPToolTestConfig, + build_mcp_request, +) + +urlpatterns = [path("sb-admin/", sb_admin_site.urls)] + + +class _PkOmittedAdmin(SBAdmin): + """Pk deliberately absent from ``list_display`` — the MCP layer + should synthesize it.""" + + model = Folder + sbadmin_list_display = ("name",) + + +class _PkDeclaredAdmin(SBAdmin): + """Pk explicitly declared — no synthetic column should be added.""" + + model = Folder + list_display = ("id", "name") + + +class _PkAliasFilterFieldAdmin(SBAdmin): + """Pk exposed under a different name but filtered via ``filter_field="id"`` + — the synthetic column must not duplicate that filter_field.""" + + model = Folder + sbadmin_list_display = ( + SBAdminField(name="public_id", annotate=F("id"), filter_field="id"), + "name", + ) + + +class _PkMethodOrderingAdmin(SBAdmin): + """Pk reached through a display method ordered on it — same collision.""" + + model = Folder + sbadmin_list_display = ("id_label", "name") + + @admin.display(ordering="id") + def id_label(self, obj): + return f"#{obj.id}" + + +@override_settings( + ROOT_URLCONF=__name__, + SB_ADMIN_CONFIGURATION="tests.sbadmin_config.MCPSBAdminConfiguration", +) +class PrimaryKeyFieldTests(TestCase): + admin_class = _PkOmittedAdmin + + def setUp(self): + super().setUp() + self._original = sb_admin_site._registry.pop(Folder, None) + sb_admin_site.register(Folder, self.admin_class) + MCPToolTestConfig().init_view_map() + MCPToolTestConfig.view_permission_for = None + + def tearDown(self): + MCPToolTestConfig.view_permission_for = None + sb_admin_site._registry.pop(Folder, None) + if self._original is not None: + sb_admin_site._registry[Folder] = self._original + super().tearDown() + + def _tools(self): + user = MagicMock(is_authenticated=True, is_superuser=True) + return SBAdminTools(request=build_mcp_request(user)) + + def test_schema_surfaces_synthetic_pk_with_widget_and_shape(self): + result = self._tools().list_admins() + entry = next(e for e in result["admin_views"] if e["view_id"] == "filer_folder") + fields_by_name = {f["name"]: f for f in entry["fields"]} + + self.assertIn("id", fields_by_name) + pk_field = fields_by_name["id"] + # Synthetic, so it's hidden by default but real enough to filter. + self.assertEqual(pk_field.get("list_visible"), False) + self.assertEqual(pk_field["filter"]["widget"], "PrimaryKeyFilterWidget") + + shape = result["widget_shapes"]["PrimaryKeyFilterWidget"] + self.assertEqual(shape["example"], [42, 7, 13]) + self.assertIn("IN", shape["value_shape"]) + + def test_select_by_id_does_not_raise(self): + """The originally-reported failure: ``fields=['id']`` rejected with + 'has no fields'. It must now resolve and return the pk.""" + folder = Folder.objects.create(name="alpha") + rows = self._tools().list_rows("filer_folder", fields=["id"])["data"] + self.assertEqual([r["id"] for r in rows], [folder.pk]) + + def test_sort_by_id_descending(self): + a = Folder.objects.create(name="a") + b = Folder.objects.create(name="b") + c = Folder.objects.create(name="c") + rows = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + sort=[{"field": "id", "dir": "desc"}], + )["data"] + self.assertEqual([r["id"] for r in rows], [c.pk, b.pk, a.pk]) + + def test_filter_by_id_list_refetches_exact_rows(self): + a = Folder.objects.create(name="a") + Folder.objects.create(name="b") + c = Folder.objects.create(name="c") + rows = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + filter_data={"id": [a.pk, c.pk]}, + )["data"] + self.assertEqual({r["id"] for r in rows}, {a.pk, c.pk}) + + def test_filter_by_single_id_scalar(self): + a = Folder.objects.create(name="a") + Folder.objects.create(name="b") + rows = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + filter_data={"id": a.pk}, + )["data"] + self.assertEqual([r["id"] for r in rows], [a.pk]) + + def test_wrong_shape_id_filter_raises(self): + with self.assertRaises(ValueError) as ctx: + self._tools().list_rows( + "filer_folder", + fields=["id"], + filter_data={"id": {"not": "a pk"}}, + ) + self.assertIn("PrimaryKeyFilterWidget", str(ctx.exception)) + + def test_non_numeric_id_rejected_before_query(self): + """A non-numeric id for an integer pk is rejected up front (clear + error), not passed to ``id__in`` where the ORM would 500.""" + with self.assertRaises(ValueError) as ctx: + self._tools().list_rows( + "filer_folder", + fields=["id"], + filter_data={"id": "abc"}, + ) + self.assertIn("abc", str(ctx.exception)) + + +class PrimaryKeyFilterWidgetParseTests(TestCase): + """The widget reads a single pk or several pks separated by commas, + whitespace, or semicolons (the text-input UI), plus native scalar / + list / autocomplete-shaped values (MCP).""" + + def _widget(self): + from types import SimpleNamespace + + from django_smartbase_admin.engine.filter_widgets import ( + PrimaryKeyFilterWidget, + ) + + widget = PrimaryKeyFilterWidget() + widget.field = SimpleNamespace(filter_field="id") + return widget + + def test_separated_string_inputs(self): + widget = self._widget() + for raw, expected in [ + ("5", ["5"]), + ("5,9", ["5", "9"]), + ("5, 9", ["5", "9"]), + ("5 9", ["5", "9"]), + ("5;9", ["5", "9"]), + ("5\n9", ["5", "9"]), + (" 5 , 9 ; 13 ", ["5", "9", "13"]), + ("", []), + ]: + self.assertEqual(widget.parse_value_from_input(None, raw), expected, raw) + + def test_native_inputs(self): + widget = self._widget() + self.assertEqual(widget.parse_value_from_input(None, 5), [5]) + self.assertEqual(widget.parse_value_from_input(None, [5, 9]), [5, 9]) + + def test_coerces_and_drops_invalid_for_integer_pk(self): + widget = self._widget() + widget.model_field = Folder._meta.pk # integer AutoField + # Coerced to int, and "abc" dropped instead of reaching the ORM. + self.assertEqual(widget.parse_value_from_input(None, "5, abc, 9"), [5, 9]) + self.assertEqual(widget.parse_value_from_input(None, "abc"), []) + self.assertEqual(widget.parse_value_from_input(None, [5, "9"]), [5, 9]) + + def test_non_numeric_pks_are_allowed(self): + """Non-integer pks pass through; only values that can't be *that* pk + type are dropped (UUIDField raises ValidationError, not ValueError).""" + from uuid import UUID + + from django.db.models import CharField, UUIDField + + char = self._widget() + char.model_field = CharField() + self.assertEqual(char.parse_value_from_input(None, "abc"), ["abc"]) + self.assertEqual(char.parse_value_from_input(None, "a, b ; c"), ["a", "b", "c"]) + + uid = self._widget() + uid.model_field = UUIDField() + u = "550e8400-e29b-41d4-a716-446655440000" + self.assertEqual(uid.parse_value_from_input(None, u), [UUID(u)]) + self.assertEqual(uid.parse_value_from_input(None, "not-a-uuid"), []) + + def test_query_is_in_lookup(self): + widget = self._widget() + q = widget.get_base_filter_query_for_parsed_value(None, ["5", "9"]) + self.assertEqual(q.children, [("id__in", ["5", "9"])]) + # Invalid input that coerced to nothing → fail closed (match nothing). + self.assertEqual( + widget.get_base_filter_query_for_parsed_value(None, []).children, + [("pk__in", [])], + ) + + +@override_settings( + ROOT_URLCONF=__name__, + SB_ADMIN_CONFIGURATION="tests.sbadmin_config.MCPSBAdminConfiguration", +) +class DeclaredPrimaryKeyTests(TestCase): + """When the admin already declares the pk, nothing is synthesized — the + author's column (and its own filter widget) is left untouched.""" + + def setUp(self): + super().setUp() + self._original = sb_admin_site._registry.pop(Folder, None) + sb_admin_site.register(Folder, _PkDeclaredAdmin) + MCPToolTestConfig().init_view_map() + MCPToolTestConfig.view_permission_for = None + + def tearDown(self): + MCPToolTestConfig.view_permission_for = None + sb_admin_site._registry.pop(Folder, None) + if self._original is not None: + sb_admin_site._registry[Folder] = self._original + super().tearDown() + + def _tools(self): + user = MagicMock(is_authenticated=True, is_superuser=True) + return SBAdminTools(request=build_mcp_request(user)) + + def test_declared_pk_is_not_duplicated(self): + result = self._tools().list_admins() + entry = next(e for e in result["admin_views"] if e["view_id"] == "filer_folder") + names = [f["name"] for f in entry["fields"]] + self.assertEqual(names, ["id", "name"]) # exactly one "id", no dup + + def test_declared_pk_select_and_sort_still_work(self): + a = Folder.objects.create(name="a") + b = Folder.objects.create(name="b") + rows = self._tools().list_rows( + "filer_folder", + fields=["id", "name"], + sort=[{"field": "id", "dir": "desc"}], + )["data"] + self.assertEqual([r["id"] for r in rows], [b.pk, a.pk]) + + +@override_settings( + ROOT_URLCONF=__name__, + SB_ADMIN_CONFIGURATION="tests.sbadmin_config.MCPSBAdminConfiguration", +) +class PrimaryKeyAliasTests(TestCase): + """The synthetic pk column is skipped only when a declared column already + *is* the pk — not merely because a column's ``filter_field`` points at it.""" + + def setUp(self): + super().setUp() + self._original = sb_admin_site._registry.pop(Folder, None) + + def tearDown(self): + MCPToolTestConfig.view_permission_for = None + sb_admin_site._registry.pop(Folder, None) + if self._original is not None: + sb_admin_site._registry[Folder] = self._original + super().tearDown() + + def _fields(self, admin_class): + sb_admin_site._registry.pop(Folder, None) + sb_admin_site.register(Folder, admin_class) + MCPToolTestConfig().init_view_map() + MCPToolTestConfig.view_permission_for = None + user = MagicMock(is_authenticated=True, is_superuser=True) + result = SBAdminTools(request=build_mcp_request(user)).list_admins() + entry = next(e for e in result["admin_views"] if e["view_id"] == "filer_folder") + return {f["name"]: f for f in entry["fields"]} + + def test_explicit_filter_field_alias_suppresses_synthetic(self): + # ``public_id`` already addresses the pk (filter_field="id"), so no + # redundant synthetic "id" is added. + fields = self._fields(_PkAliasFilterFieldAdmin) + self.assertEqual(list(fields), ["public_id", "name"]) # no synthetic "id" + + def test_display_method_ordering_suppresses_synthetic(self): + # ``id_label`` orders on the pk → addresses it; no synthetic "id". + fields = self._fields(_PkMethodOrderingAdmin) + self.assertEqual(list(fields), ["id_label", "name"]) # no synthetic "id" + + +class TabulatorIdColumnNameTests(TestCase): + """``get_tabulator_columns`` reports the row-id column name from the pk + column (``model_field.primary_key``); when no column is the pk — e.g. the + synthetic was skipped for an annotation alias — it falls back to the pk + field name (only the name; no column is grafted on).""" + + def _id_column_name(self, column_fields, pk_name="id"): + action = SimpleNamespace( + column_fields=column_fields, + get_pk_field=lambda: SimpleNamespace(name=pk_name), + ) + _, id_column_name = SBAdminListAction.get_tabulator_columns(action) + return id_column_name + + def test_uses_pk_column_field_when_present(self): + pk_col = SimpleNamespace( + model_field=SimpleNamespace(primary_key=True), + field="id", + serialize_tabulator=lambda: {"field": "id"}, + ) + self.assertEqual(self._id_column_name([pk_col], pk_name="UNUSED"), "id") + + def test_falls_back_to_pk_name_when_no_pk_column(self): + # An annotation that addresses the pk has no ``model_field.primary_key``. + alias = SimpleNamespace( + model_field=None, + field="public_id__annotate", + serialize_tabulator=lambda: {"field": "public_id__annotate"}, + ) + self.assertEqual(self._id_column_name([alias]), "id") + + +class _SessionAdmin(SBAdmin): + """A model whose pk is a non-``id`` CharField (``session_key``).""" + + model = Session + sbadmin_list_display = ("expire_date",) + + +@override_settings( + ROOT_URLCONF=__name__, + SB_ADMIN_CONFIGURATION="tests.sbadmin_config.MCPSBAdminConfiguration", +) +class CustomPkIdAliasTests(TestCase): + """For a model whose pk isn't named ``id``, ``"id"`` is accepted as an + alias on input — so the documented exact-row refetch works even though the + column is registered under the real pk name.""" + + def setUp(self): + super().setUp() + self._original = sb_admin_site._registry.pop(Session, None) + sb_admin_site.register(Session, _SessionAdmin) + MCPToolTestConfig().init_view_map() + MCPToolTestConfig.view_permission_for = None + + def tearDown(self): + MCPToolTestConfig.view_permission_for = None + sb_admin_site._registry.pop(Session, None) + if self._original is not None: + sb_admin_site._registry[Session] = self._original + super().tearDown() + + def _tools(self): + user = MagicMock(is_authenticated=True, is_superuser=True) + return SBAdminTools(request=build_mcp_request(user)) + + def _view_id(self, tools): + result = tools.list_admins() + return next( + e["view_id"] for e in result["admin_views"] if e["model"] == "Session" + ) + + def test_id_alias_selects_sorts_and_filters_the_real_pk(self): + Session.objects.create( + session_key="abc123", session_data="", expire_date=timezone.now() + ) + tools = self._tools() + view_id = self._view_id(tools) + + # ``fields=["id"]`` resolves (alias → ``session_key``); row mirrors it. + rows = tools.list_rows( + view_id, fields=["id"], sort=[{"field": "id", "dir": "asc"}] + )["data"] + self.assertEqual([r["id"] for r in rows], ["abc123"]) + + # ``filter_data={"id": ...}`` refetches the exact row. + filtered = tools.list_rows( + view_id, fields=["id"], filter_data={"id": "abc123"} + )["data"] + self.assertEqual([r["id"] for r in filtered], ["abc123"])