Fix 10 issues found in high-effort review of main (#141)

* Fix 10 issues found in high-effort review of main

Backend:
- SQLAlchemy: coerce relationship (m2m) filter values to int for integer
  pks (500 on PostgreSQL/asyncpg) and reject unsupported relation lookups
  with 422 instead of silently collapsing them to pk equality
- PonyORM: only rewrite <relation>_<pk> filter keys when the base attribute
  is a real relation, so plain columns ending in _id filter correctly
- PonyORM: run list search as a single OR'd WHERE clause instead of
  materializing every matching row once per search field
- Flask: pass through HTTPExceptions with an attached response, preserve
  error headers on JSON error responses, log unhandled errors with traceback
- Filtering: text columns support IS NULL again via __exact=null while
  substring lookups keep the literal string

Frontend:
- only coerce "true"/"false" strings to booleans for boolean widgets, so
  text content that spells a boolean survives the change form
- treat fields without a change widget as known non-date (null) instead of
  falling back to shape-based date detection
- transformDataFromServer takes the model configuration directly, removing
  the duplicated getChangeWidgetTypes incantation at every call site

* Add coverage for relation-filter rejection and Flask error passthrough

Covers the three new defensive branches: the service-layer 422 for
substring lookups on m2m filter fields, the SQLAlchemy adapter's loud
failure for unsupported relation conditions, and the Flask handler's
passthrough of HTTPExceptions with an attached response (plus header
preservation on JSON error responses).

Author: vsdudakov

CHANGELOG.md

@@ -2,6 +2,31 @@
 
 All notable changes to FastAdmin are documented in this file.
 
+## Unreleased
+
+Bug-fix follow-up to the 0.8.1 audit. No public API changes.
+
+- **SQLAlchemy**: relationship (m2m) filter values are coerced to `int` for
+  integer primary keys (previously raised HTTP 500 on strictly typed drivers
+  such as PostgreSQL/asyncpg), and unsupported lookups on relation fields
+  (e.g. `icontains`) are rejected with HTTP 422 instead of silently matching
+  by primary-key equality.
+- **Pony ORM**: the foreign-key filter rewrite no longer corrupts filters on
+  plain columns whose name merely ends with `_id`, and list-view search now
+  runs as a single OR'd WHERE clause instead of materializing every matching
+  row once per search field.
+- **Flask**: `HTTPException`s with an attached response (`abort(Response(...))`)
+  are passed through unchanged, error headers (`Allow`, `WWW-Authenticate`,
+  `Retry-After`, ...) are preserved on JSON error responses, and unhandled
+  errors are logged with their traceback.
+- **Filtering**: `IS NULL` is expressible again on text columns via
+  `field__exact=null` (substring lookups keep the literal string `"null"`).
+- **Frontend**: stop corrupting text fields whose content is the literal word
+  `"true"`/`"false"` on the change form (booleans are now only coerced for
+  boolean widgets), stop shape-based date detection for fields known to have
+  no date widget, and pass the model configuration directly to
+  `transformDataFromServer` so every call site gets the safe behavior.
+
 ## 0.8.1
 
 Bug-fix release from a full-source audit of `main`. No public API changes.

fastadmin/api/frameworks/flask/app.py

@@ -33,11 +33,19 @@ def default(self, o):
 @app.errorhandler(Exception)
 def exception_handler(exc):
     if isinstance(exc, HTTPException):
+        if exc.response is not None:
+            # abort(Response(...)) attached a fully-formed response; pass it
+            # through untouched instead of replacing it with a generic error.
+            return exc
         # Return API errors as JSON {"detail": ...} with the proper HTTP status
         # so the shared React frontend can read the message (matching the Django
         # and FastAPI integrations), instead of werkzeug's default HTML page.
-        return {"detail": exc.description}, exc.code or 500
-    # Unhandled server error: log it server-side but never leak internals to the
-    # client, and return a real HTTP 500 (a bare dict would be sent as HTTP 200).
-    logger.error("Unhandled admin error: %s", exc)
+        # Keep the exception's headers (Allow, WWW-Authenticate, Retry-After, ...)
+        # apart from its text/html Content-Type, which the JSON body replaces.
+        headers = [(k, v) for k, v in exc.get_headers() if k.lower() != "content-type"]
+        return {"detail": exc.description}, exc.code or 500, headers
+    # Unhandled server error: log it (with traceback — this is the only place it
+    # is captured) but never leak internals to the client, and return a real
+    # HTTP 500 (a bare dict would be sent as HTTP 200).
+    logger.error("Unhandled admin error: %s", exc, exc_info=exc)
     return {"detail": "Internal server error."}, 500

fastadmin/api/helpers.py

@@ -24,17 +24,26 @@
 def sanitize_filter_value(
     value: str | list,
     field: ModelFieldWidgetSchema | None = None,
+    condition: str = "exact",
 ) -> bool | None | str | list:
     """Sanitize value (string or list for __in filters).
 
     :params value: a value (str or list of str for __in).
     :params field: the field being filtered, used to decide whether the
         "true"/"false"/"null" literals should be coerced (skipped for text fields).
+    :params condition: the filter lookup (exact/in/icontains/...), used to keep
+        IS NULL expressible on text fields.
     :return: A sanitized value.
     """
     if isinstance(value, list):
-        return [sanitize_filter_value(v, field) for v in value]
+        return [sanitize_filter_value(v, field, condition) for v in value]
     if field is not None and field.filter_widget_type in TEXT_FILTER_WIDGET_TYPES:
+        # Text content may legitimately be the words "true"/"false"/"null", so
+        # they are kept literal — except "null" on an exact lookup, which stays
+        # the only way to express IS NULL on a text column (match the literal
+        # word via contains/icontains instead).
+        if value == "null" and condition == "exact":
+            return None
         return value
     match value:
         case "false":
@@ -109,7 +118,8 @@ def build_query_filters(
             continue
         field_name = key.partition("__")[0]
         field = next((f for f in fields if f.name == field_name), None)
-        result[sanitize_filter_key(key, fields)] = sanitize_filter_value(value, field)
+        sanitized_key = sanitize_filter_key(key, fields)
+        result[sanitized_key] = sanitize_filter_value(value, field, sanitized_key[1])
     return result
 
 

fastadmin/api/service.py

@@ -103,10 +103,13 @@ def _validate_filters(admin_model: Any, filters: dict, exclude_filter_fields: tu
 
         The filterable field set is ``list_filter`` when the admin defines it,
         otherwise the serialized field set; the lookup suffix must be one of
-        ALLOWED_FILTER_CONDITIONS.
+        ALLOWED_FILTER_CONDITIONS. Relation (m2m) fields only support pk
+        equality/membership, so other lookups are rejected here instead of
+        failing (or silently degrading) inside the ORM adapters.
         """
         list_filter = getattr(admin_model, "list_filter", ())
         allowlist = set(list_filter) if list_filter else fields
+        m2m_fields = {f.name for f in admin_model.get_model_fields_with_widget_types() if f.is_m2m}
         for k in filters:
             if k in exclude_filter_fields:
                 continue
@@ -115,6 +118,8 @@ def _validate_filters(admin_model: Any, filters: dict, exclude_filter_fields: tu
                 raise AdminApiException(422, detail=f"Filter by {k} is not allowed")
             if field not in allowlist:
                 raise AdminApiException(422, detail=f"Filter by {k} is not allowed")
+            if field in m2m_fields and condition and condition not in ("exact", "in"):
+                raise AdminApiException(422, detail=f"Filter by {k} is not allowed")
 
     @staticmethod
     def _bind_admin_context(

fastadmin/models/orms/ponyorm.py

@@ -259,8 +259,16 @@ def orm_get_list(
                 field = field_with_condition[0]
                 condition = field_with_condition[1]
                 model_pk_name = self.get_model_pk_name(self.model_cls)
-                if field.endswith(f"_{model_pk_name}"):
-                    field = field.replace(f"_{model_pk_name}", f".{model_pk_name}")
+                # A "<relation>_<pk>" key (added by sanitize_filter_key for fk
+                # fields) is rewritten to the "<relation>.<pk>" path — but only
+                # when the base attribute really is a relation, so a plain
+                # column that merely ends with "_id" is left untouched.
+                pk_suffix = f"_{model_pk_name}"
+                if field.endswith(pk_suffix):
+                    base_name = field[: -len(pk_suffix)]
+                    base_attr = getattr(self.model_cls, base_name, None) if base_name else None
+                    if base_attr is not None and getattr(base_attr, "is_relation", False):
+                        field = f"{base_name}.{model_pk_name}"
                 # Bind the user-supplied value as a local (`fval`) so Pony
                 # resolves it as a query parameter. Only the field path — built
                 # from the admin's validated/allowlisted field names — is
@@ -272,21 +280,16 @@ def orm_get_list(
 
         search_fields = list(self.search_fields)
         if search and search_fields:
-            model_pk_name = self.get_model_pk_name(self.model_cls)
-            ids = []
             # Bind the user-supplied search term as a local so Pony resolves it
-            # as a parameter. Only the field path (from the admin's trusted
-            # search_fields config) is interpolated — never the search value.
+            # as a parameter. Only the field paths (from the admin's trusted
+            # search_fields config) are interpolated — never the search value.
+            # All fields are OR-ed into a single filter so the search runs as
+            # one WHERE clause instead of materializing matching rows per field.
             search_term = search.lower()  # noqa: F841 (referenced by Pony filter string)
-            for search_field in search_fields:
-                pony_search_field = search_field.replace("__", ".")
-                qs_ids = qs.filter(f"search_term in m.{pony_search_field}.lower()")
-                objs = list(qs_ids)
-                ids += [getattr(o, model_pk_name) for o in objs]
-            # Bind the collected pks as a local so Pony resolves it as a
-            # parameter; only the trusted pk field path is interpolated.
-            pk_ids = set(ids)  # noqa: F841 (referenced by Pony filter string)
-            qs = qs.filter(f"m.{model_pk_name} in pk_ids")
+            search_expr = " or ".join(
+                f"search_term in m.{search_field.replace('__', '.')}.lower()" for search_field in search_fields
+            )
+            qs = qs.filter(search_expr)
 
         ordering = [sort_by] if sort_by else self.ordering
         if ordering:

fastadmin/models/orms/sqlalchemy.py

@@ -340,9 +340,17 @@ def order_column(ordering_field: str):
                     if related_mapper is not None:
                         # Relationship field (e.g. m2m): SQLAlchemy rejects a
                         # scalar comparison against a collection, so match on the
-                        # related row's pk via any()/has() instead.
+                        # related row's pk via any()/has() instead. Only pk
+                        # equality/membership is meaningful here — anything else
+                        # must fail loudly rather than degrade to an equality
+                        # match (the service layer rejects these with a 422).
+                        if condition not in ("exact", "in"):
+                            raise ValueError(f"Filter condition {condition!r} is not supported for relation {field!r}")
                         related_cls = related_mapper.class_
                         related_pk = getattr(related_cls, self.get_model_pk_name(related_cls))
+                        if isinstance(related_pk.expression.type, BIGINT | Integer):
+                            with contextlib.suppress(ValueError, TypeError):
+                                value = [int(x) for x in value] if condition == "in" else int(value)
                         match_expr = related_pk.in_(value) if condition == "in" else related_pk == value
                         if getattr(rel_property, "uselist", True):
                             q.append(model_field.any(match_expr))

frontend/src/components/async-select/index.tsx

@@ -28,10 +28,7 @@ import { getFetcher, patchFetcher, postFetcher } from "@/fetchers/fetchers";
 import { getConfigurationModel } from "@/helpers/configuration";
 import { handleError } from "@/helpers/forms";
 import { getTitleFromModel } from "@/helpers/title";
-import {
-  getChangeWidgetTypes,
-  transformDataFromServer,
-} from "@/helpers/transform";
+import { transformDataFromServer } from "@/helpers/transform";
 import { EModelPermission } from "@/interfaces/configuration";
 import { ConfigurationContext } from "@/providers/ConfigurationProvider";
 
@@ -90,10 +87,7 @@ export const AsyncSelect: React.FC<IAsyncSelect> = ({
   const asyncSelectChangeInitialValues = useMemo(
     () =>
       initialChangeValues != null
-        ? transformDataFromServer(
-            initialChangeValues,
-            getChangeWidgetTypes(modelConfiguration),
-          )
+        ? transformDataFromServer(initialChangeValues, modelConfiguration)
         : undefined,
     [initialChangeValues, modelConfiguration],
   );

frontend/src/components/inline-widget/index.tsx

@@ -30,7 +30,6 @@ import {
 import { handleError } from "@/helpers/forms";
 import { getTitleFromModel } from "@/helpers/title";
 import {
-  getChangeWidgetTypes,
   transformDataFromServer,
   transformDataToServer,
   transformFiltersToServer,
@@ -152,10 +151,7 @@ export const InlineWidget: React.FC<IInlineWidget> = ({
   const inlineChangeInitialValues = useMemo(
     () =>
       initialChangeValues != null
-        ? transformDataFromServer(
-            initialChangeValues,
-            getChangeWidgetTypes(modelConfiguration),
-          )
+        ? transformDataFromServer(initialChangeValues, modelConfiguration)
         : undefined,
     [initialChangeValues, modelConfiguration],
   );

frontend/src/containers/change/index.tsx

@@ -28,7 +28,6 @@ import { getConfigurationModel } from "@/helpers/configuration";
 import { handleError } from "@/helpers/forms";
 import { getTitleFromModel } from "@/helpers/title";
 import {
-  getChangeWidgetTypes,
   transformDataFromServer,
   transformDataToServer,
 } from "@/helpers/transform";
@@ -62,10 +61,7 @@ export const Change: React.FC = () => {
   const initialValues = useMemo(
     () =>
       initialChangeValues != null
-        ? transformDataFromServer(
-            initialChangeValues,
-            getChangeWidgetTypes(modelConfiguration),
-          )
+        ? transformDataFromServer(initialChangeValues, modelConfiguration)
         : undefined,
     [initialChangeValues, modelConfiguration],
   );

frontend/src/helpers/transform.test.tsx

@@ -232,21 +232,71 @@ describe("transform", () => {
         "12:00:00",
       );
     });
+    it("keeps a boolean-looking string as-is for a non-boolean widget", () => {
+      expect(transformValueFromServer("false", EFieldWidgetType.Input)).toBe(
+        "false",
+      );
+      expect(transformValueFromServer("True", EFieldWidgetType.TextArea)).toBe(
+        "True",
+      );
+    });
+    it("coerces boolean strings for boolean widgets", () => {
+      expect(transformValueFromServer("false", EFieldWidgetType.Switch)).toBe(
+        false,
+      );
+      expect(transformValueFromServer("true", EFieldWidgetType.Checkbox)).toBe(
+        true,
+      );
+    });
+    it("keeps real booleans regardless of widget type", () => {
+      expect(transformValueFromServer(true, EFieldWidgetType.Input)).toBe(true);
+      expect(transformValueFromServer(false, null)).toBe(false);
+    });
+    it("skips shape detection when the widget is known to be absent", () => {
+      expect(transformValueFromServer("2024-01-15", null)).toBe("2024-01-15");
+      expect(transformValueFromServer("false", null)).toBe("false");
+    });
   });
 
   describe("transformDataFromServer", () => {
     it("transforms all values", () => {
       const r = transformDataFromServer({ d: "2024-01-15" });
       expect(r).toHaveProperty("d");
     });
-    it("respects per-field widget types", () => {
+    it("respects per-field widget types from the model configuration", () => {
       const r = transformDataFromServer(
         { at: "2024-01-15", code: "2024-01-15" },
-        { at: EFieldWidgetType.DatePicker, code: EFieldWidgetType.Input },
+        {
+          fields: [
+            {
+              name: "at",
+              change_configuration: {
+                form_widget_type: EFieldWidgetType.DatePicker,
+              },
+            },
+            {
+              name: "code",
+              change_configuration: {
+                form_widget_type: EFieldWidgetType.Input,
+              },
+            },
+          ],
+        } as any,
       );
       expect(dayjs.isDayjs(r.at)).toBe(true);
       expect(r.code).toBe("2024-01-15");
     });
+    it("does not shape-detect fields without a change widget when a configuration is given", () => {
+      const r = transformDataFromServer(
+        { code: "2024-01-15", flag: "false", unknown: "2024-01-15" },
+        {
+          fields: [{ name: "code", change_configuration: {} }],
+        } as any,
+      );
+      expect(r.code).toBe("2024-01-15");
+      expect(r.flag).toBe("false");
+      expect(r.unknown).toBe("2024-01-15");
+    });
   });
 
   describe("getChangeWidgetTypes", () => {
@@ -262,7 +312,7 @@ describe("transform", () => {
           { name: "code", change_configuration: {} },
         ],
       } as any);
-      expect(map).toEqual({ at: EFieldWidgetType.DatePicker, code: undefined });
+      expect(map).toEqual({ at: EFieldWidgetType.DatePicker, code: null });
     });
     it("returns an empty map when configuration is missing", () => {
       expect(getChangeWidgetTypes()).toEqual({});