From 6e075ca6985bfe70737f5d35d8eb279e2652912f Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 12:56:53 +0200 Subject: [PATCH 1/6] refactor: simplify tabulator column handling and introduce primary key filter widget - Renamed `get_tabulator_columns_add_id_column_if_missing` to `get_tabulator_columns` for clarity. - Removed the logic for conditionally adding an ID column, now handled by `get_effective_list_display`. - Introduced `PrimaryKeyFilterWidget` to allow filtering by primary key, enhancing the usability of the list view. - Updated tests to ensure proper functionality of the new primary key handling and filtering capabilities. --- .../actions/admin_action_list.py | 19 +- .../audit/tests/test_action_processing.py | 2 +- .../engine/admin_base_view.py | 50 +++- .../engine/filter_widgets.py | 62 +++++ src/django_smartbase_admin/mcp/mcp.py | 3 + src/django_smartbase_admin/mcp/schema.py | 9 + .../mcp/tests/test_pk_field.py | 218 ++++++++++++++++++ 7 files changed, 347 insertions(+), 16 deletions(-) create mode 100644 src/django_smartbase_admin/mcp/tests/test_pk_field.py diff --git a/src/django_smartbase_admin/actions/admin_action_list.py b/src/django_smartbase_admin/actions/admin_action_list.py index ee2b9eaf..0afd44e3 100644 --- a/src/django_smartbase_admin/actions/admin_action_list.py +++ b/src/django_smartbase_admin/actions/admin_action_list.py @@ -149,24 +149,17 @@ 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 column's field name (the + frontend's ``tableIdColumnName``, or ``None`` if the column set has + no single pk). The pk is exposed as a real column upstream by + ``get_effective_list_display``, so it isn't grafted on here.""" 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 return columns_serialized, id_column_name def get_excel_columns(self): @@ -208,7 +201,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/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/engine/admin_base_view.py b/src/django_smartbase_admin/engine/admin_base_view.py index ee20a2fa..98ee4df7 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,52 @@ 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, or ``None`` when the pk is already + declared / 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 + declared = {getattr(field, "name", field) for field in list_display} + if pk_name in declared: + return None + field = self.auto_create_field_from_model_field(pk_model_field) + field.title = "ID" + # Hidden by default — present for select/sort/filter, not forced + # into every projection (the pk is emitted regardless). + field.list_visible = False + 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 +886,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..8338bd22 100644 --- a/src/django_smartbase_admin/engine/filter_widgets.py +++ b/src/django_smartbase_admin/engine/filter_widgets.py @@ -1,4 +1,5 @@ import json +import re from datetime import datetime, timedelta from django import forms @@ -386,6 +387,67 @@ 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): + """Normalise input to a flat list of pks, dropping empties. + + Accepts a native scalar or list (MCP) or a string of pks separated + by commas, whitespace, or semicolons (the text-input UI: ``"5"``, + ``"5, 9"``, ``"5 9"``, ``"5;9"``). Values stay strings — the ORM + coerces them per the pk field's type, so this works for integer and + non-integer (uuid/char) pks alike. + """ + 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] + return [item for item in value if item not in forms.Field.empty_values] + + def get_base_filter_query_for_parsed_value(self, request, parsed_value): + if not parsed_value: + # No usable pk → no constraint (matches how the other widgets + # treat an empty value: the filter simply doesn't narrow). + return Q() + 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}" + ) + + 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..360eb41e 100644 --- a/src/django_smartbase_admin/mcp/mcp.py +++ b/src/django_smartbase_admin/mcp/mcp.py @@ -517,6 +517,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 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_pk_field.py b/src/django_smartbase_admin/mcp/tests/test_pk_field.py new file mode 100644 index 00000000..8e7a8799 --- /dev/null +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -0,0 +1,218 @@ +"""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 unittest.mock import MagicMock + +from django.test import TestCase, override_settings +from django.urls import path +from filer.models import Folder + +from django_smartbase_admin.admin.admin_base import SBAdmin +from django_smartbase_admin.admin.site import sb_admin_site +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") + + +@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)) + + +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_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"])]) + # No usable pk → no constraint. + self.assertEqual( + widget.get_base_filter_query_for_parsed_value(None, []).children, [] + ) + + +@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]) From 5a5095d0ddde3585d309839844e3d09c8c104e18 Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 13:44:02 +0200 Subject: [PATCH 2/6] formatting --- src/django_smartbase_admin/mcp/tests/test_pk_field.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/django_smartbase_admin/mcp/tests/test_pk_field.py b/src/django_smartbase_admin/mcp/tests/test_pk_field.py index 8e7a8799..231dd231 100644 --- a/src/django_smartbase_admin/mcp/tests/test_pk_field.py +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -64,9 +64,7 @@ def _tools(self): 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" - ) + 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) @@ -201,9 +199,7 @@ def _tools(self): 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" - ) + 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 From 6f1c1768396eca8c5eecacbdf5e9792a2356d142 Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 14:08:16 +0200 Subject: [PATCH 3/6] fixes --- src/django_smartbase_admin/audit/manager.py | 8 +- .../engine/admin_base_view.py | 20 +++- .../engine/filter_widgets.py | 34 +++++-- .../mcp/tests/test_pk_field.py | 94 +++++++++++++++++++ 4 files changed, 144 insertions(+), 12 deletions(-) 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/engine/admin_base_view.py b/src/django_smartbase_admin/engine/admin_base_view.py index 98ee4df7..07253662 100644 --- a/src/django_smartbase_admin/engine/admin_base_view.py +++ b/src/django_smartbase_admin/engine/admin_base_view.py @@ -861,9 +861,23 @@ def _build_synthetic_pk_field(self, list_display): pk_name = getattr(pk_model_field, "name", None) if not pk_name: return None - declared = {getattr(field, "name", field) for field in list_display} - if pk_name in declared: - return None + # Skip if a declared column already targets the pk — by name, by an + # explicit ``filter_field="id"``, or via a method's + # ``@admin.display(ordering="id")``. A second column on the same + # filter_field would collide, and the W001 duplicate check can't see + # one synthesized at runtime. + 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" # Hidden by default — present for select/sort/filter, not forced diff --git a/src/django_smartbase_admin/engine/filter_widgets.py b/src/django_smartbase_admin/engine/filter_widgets.py index 8338bd22..059230a0 100644 --- a/src/django_smartbase_admin/engine/filter_widgets.py +++ b/src/django_smartbase_admin/engine/filter_widgets.py @@ -3,7 +3,7 @@ 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 @@ -406,20 +406,32 @@ class PrimaryKeyFilterWidget(SBAdminFilterWidget): _SEPARATORS = re.compile(r"[\s,;]+") def parse_value_from_input(self, request, filter_value): - """Normalise input to a flat list of pks, dropping empties. + """Normalise input to a flat list of pks, dropping empties and any + value that can't be this pk. Accepts a native scalar or list (MCP) or a string of pks separated by commas, whitespace, or semicolons (the text-input UI: ``"5"``, - ``"5, 9"``, ``"5 9"``, ``"5;9"``). Values stay strings — the ORM - coerces them per the pk field's type, so this works for integer and - non-integer (uuid/char) pks alike. + ``"5, 9"``, ``"5 9"``, ``"5;9"``). Each value is coerced through the + pk field, so an id that doesn't fit (e.g. ``"abc"`` for an integer + pk) is dropped rather than handed to the ORM — where it would raise + and 500 the list query for the browser. """ 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] - return [item for item in value if item not in forms.Field.empty_values] + 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: @@ -438,6 +450,16 @@ def validate_value(self, value) -> None: "primary keys (int or str), got " f"{type(item).__name__}: {item!r}" ) + # Reject an id that can't be this pk up front, so MCP gets a clear + # error instead of the ORM's coercion 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 [ diff --git a/src/django_smartbase_admin/mcp/tests/test_pk_field.py b/src/django_smartbase_admin/mcp/tests/test_pk_field.py index 231dd231..6537a448 100644 --- a/src/django_smartbase_admin/mcp/tests/test_pk_field.py +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -7,12 +7,15 @@ from unittest.mock import MagicMock +from django.contrib import admin +from django.db.models import F from django.test import TestCase, override_settings from django.urls import path from filer.models import Folder 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, @@ -37,6 +40,28 @@ class _PkDeclaredAdmin(SBAdmin): 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", @@ -125,6 +150,17 @@ def test_wrong_shape_id_filter_raises(self): ) 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, @@ -161,6 +197,32 @@ def test_native_inputs(self): 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"]) @@ -212,3 +274,35 @@ def test_declared_pk_select_and_sort_still_work(self): 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): + """A column that already targets the pk filter_field (without being named + after it) suppresses the synthetic column — no duplicate filter.""" + + def tearDown(self): + MCPToolTestConfig.view_permission_for = None + sb_admin_site._registry.pop(Folder, None) + super().tearDown() + + def _filter_field_names(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"] for f in entry["fields"]] + + def test_explicit_filter_field_alias_suppresses_synthetic(self): + names = self._filter_field_names(_PkAliasFilterFieldAdmin) + self.assertEqual(names, ["public_id", "name"]) # no extra "id" + + def test_display_method_ordering_suppresses_synthetic(self): + names = self._filter_field_names(_PkMethodOrderingAdmin) + self.assertEqual(names, ["id_label", "name"]) # no extra "id" From ff37499a112ab7b0ba1c5204f339ca6875ed174e Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 14:09:05 +0200 Subject: [PATCH 4/6] fixes --- .../audit/tests/test_fk_affected.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 src/django_smartbase_admin/audit/tests/test_fk_affected.py 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)) From 8cc6bc8b1a82aca6d4b077fd3f97d320b8173137 Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 14:58:49 +0200 Subject: [PATCH 5/6] fixes --- .../actions/admin_action_list.py | 10 +- .../engine/admin_base_view.py | 23 ++-- .../engine/filter_widgets.py | 21 ++-- src/django_smartbase_admin/mcp/mcp.py | 28 ++++- .../mcp/tests/test_filter_validation.py | 25 ++++ .../mcp/tests/test_pk_field.py | 115 ++++++++++++++++-- 6 files changed, 182 insertions(+), 40 deletions(-) diff --git a/src/django_smartbase_admin/actions/admin_action_list.py b/src/django_smartbase_admin/actions/admin_action_list.py index 0afd44e3..67bd74b3 100644 --- a/src/django_smartbase_admin/actions/admin_action_list.py +++ b/src/django_smartbase_admin/actions/admin_action_list.py @@ -150,16 +150,18 @@ def get_filters(self): return [field for field in self.column_fields if field.filter_widget] def get_tabulator_columns(self): - """Serialized Tabulator columns + the pk column's field name (the - frontend's ``tableIdColumnName``, or ``None`` if the column set has - no single pk). The pk is exposed as a real column upstream by - ``get_effective_list_display``, so it isn't grafted on here.""" + """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()) + 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): diff --git a/src/django_smartbase_admin/engine/admin_base_view.py b/src/django_smartbase_admin/engine/admin_base_view.py index 07253662..1d03f887 100644 --- a/src/django_smartbase_admin/engine/admin_base_view.py +++ b/src/django_smartbase_admin/engine/admin_base_view.py @@ -848,8 +848,16 @@ def get_effective_list_display(self, request) -> list: return list_display def _build_synthetic_pk_field(self, list_display): - """Synthetic read-only pk column, or ``None`` when the pk is already - declared / can't be resolved (e.g. a composite pk).""" + """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, ) @@ -861,11 +869,7 @@ def _build_synthetic_pk_field(self, list_display): pk_name = getattr(pk_model_field, "name", None) if not pk_name: return None - # Skip if a declared column already targets the pk — by name, by an - # explicit ``filter_field="id"``, or via a method's - # ``@admin.display(ordering="id")``. A second column on the same - # filter_field would collide, and the W001 duplicate check can't see - # one synthesized at runtime. + for entry in list_display: name = getattr(entry, "name", entry) method = getattr(self, name, None) if isinstance(name, str) else None @@ -878,11 +882,10 @@ def _build_synthetic_pk_field(self, list_display): ) ): return None + field = self.auto_create_field_from_model_field(pk_model_field) field.title = "ID" - # Hidden by default — present for select/sort/filter, not forced - # into every projection (the pk is emitted regardless). - field.list_visible = False + field.list_visible = False # off by default; still select/sort/filterable field.filter_widget = PrimaryKeyFilterWidget() return field diff --git a/src/django_smartbase_admin/engine/filter_widgets.py b/src/django_smartbase_admin/engine/filter_widgets.py index 059230a0..361cdfe5 100644 --- a/src/django_smartbase_admin/engine/filter_widgets.py +++ b/src/django_smartbase_admin/engine/filter_widgets.py @@ -406,16 +406,10 @@ class PrimaryKeyFilterWidget(SBAdminFilterWidget): _SEPARATORS = re.compile(r"[\s,;]+") def parse_value_from_input(self, request, filter_value): - """Normalise input to a flat list of pks, dropping empties and any - value that can't be this pk. - - Accepts a native scalar or list (MCP) or a string of pks separated - by commas, whitespace, or semicolons (the text-input UI: ``"5"``, - ``"5, 9"``, ``"5 9"``, ``"5;9"``). Each value is coerced through the - pk field, so an id that doesn't fit (e.g. ``"abc"`` for an integer - pk) is dropped rather than handed to the ORM — where it would raise - and 500 the list query for the browser. - """ + """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] @@ -435,8 +429,7 @@ def parse_value_from_input(self, request, filter_value): def get_base_filter_query_for_parsed_value(self, request, parsed_value): if not parsed_value: - # No usable pk → no constraint (matches how the other widgets - # treat an empty value: the filter simply doesn't narrow). + # No usable pk → no constraint, like the other widgets on empty. return Q() return Q(**{f"{self.field.filter_field}__in": parsed_value}) @@ -450,8 +443,8 @@ def validate_value(self, value) -> None: "primary keys (int or str), got " f"{type(item).__name__}: {item!r}" ) - # Reject an id that can't be this pk up front, so MCP gets a clear - # error instead of the ORM's coercion failure mid-query. + # 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) diff --git a/src/django_smartbase_admin/mcp/mcp.py b/src/django_smartbase_admin/mcp/mcp.py index 360eb41e..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: @@ -593,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/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 index 6537a448..5892d9a0 100644 --- a/src/django_smartbase_admin/mcp/tests/test_pk_field.py +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -5,14 +5,18 @@ 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 @@ -281,15 +285,15 @@ def test_declared_pk_select_and_sort_still_work(self): SB_ADMIN_CONFIGURATION="tests.sbadmin_config.MCPSBAdminConfiguration", ) class PrimaryKeyAliasTests(TestCase): - """A column that already targets the pk filter_field (without being named - after it) suppresses the synthetic column — no duplicate filter.""" + """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 tearDown(self): MCPToolTestConfig.view_permission_for = None sb_admin_site._registry.pop(Folder, None) super().tearDown() - def _filter_field_names(self, admin_class): + def _fields(self, admin_class): sb_admin_site._registry.pop(Folder, None) sb_admin_site.register(Folder, admin_class) MCPToolTestConfig().init_view_map() @@ -297,12 +301,107 @@ def _filter_field_names(self, admin_class): 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"] for f in entry["fields"]] + return {f["name"]: f for f in entry["fields"]} def test_explicit_filter_field_alias_suppresses_synthetic(self): - names = self._filter_field_names(_PkAliasFilterFieldAdmin) - self.assertEqual(names, ["public_id", "name"]) # no extra "id" + # ``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): - names = self._filter_field_names(_PkMethodOrderingAdmin) - self.assertEqual(names, ["id_label", "name"]) # no extra "id" + # ``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"]) From 1956cef1820d96a89e9209e5c4c10b950faf5a2e Mon Sep 17 00:00:00 2001 From: Ondrej Kolimar Date: Fri, 19 Jun 2026 15:18:59 +0200 Subject: [PATCH 6/6] fixes --- src/django_smartbase_admin/engine/filter_widgets.py | 7 +++++-- src/django_smartbase_admin/mcp/tests/test_pk_field.py | 11 +++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/django_smartbase_admin/engine/filter_widgets.py b/src/django_smartbase_admin/engine/filter_widgets.py index 361cdfe5..28de17b0 100644 --- a/src/django_smartbase_admin/engine/filter_widgets.py +++ b/src/django_smartbase_admin/engine/filter_widgets.py @@ -429,8 +429,11 @@ def parse_value_from_input(self, request, filter_value): def get_base_filter_query_for_parsed_value(self, request, parsed_value): if not parsed_value: - # No usable pk → no constraint, like the other widgets on empty. - return Q() + # 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: diff --git a/src/django_smartbase_admin/mcp/tests/test_pk_field.py b/src/django_smartbase_admin/mcp/tests/test_pk_field.py index 5892d9a0..548e85f7 100644 --- a/src/django_smartbase_admin/mcp/tests/test_pk_field.py +++ b/src/django_smartbase_admin/mcp/tests/test_pk_field.py @@ -231,9 +231,10 @@ 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"])]) - # No usable pk → no constraint. + # Invalid input that coerced to nothing → fail closed (match nothing). self.assertEqual( - widget.get_base_filter_query_for_parsed_value(None, []).children, [] + widget.get_base_filter_query_for_parsed_value(None, []).children, + [("pk__in", [])], ) @@ -288,9 +289,15 @@ 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):