Skip to content

Commit 3aaec53

Browse files
authored
Fix 12 bugs found in full-source audit of main (#140)
* Fix 12 bugs found in full-source audit of main Backend (Python): - ponyorm: use get_model_pk_name in search instead of hardcoded `id` (search 500'd for models with a non-`id` primary key) - ponyorm: build ordering in a single order_by() call so mixed asc/desc fields keep their declared priority (Pony prepends per-call) - sqlalchemy: filter m2m/relationship fields via any()/has() on the related pk instead of a scalar comparison that raised InvalidRequestError (500) - flask: return a real HTTP 500 with a generic message (was HTTP 200 leaking the exception text), and emit API errors as JSON {"detail": ...} matching the Django/FastAPI integrations instead of werkzeug HTML pages - api: make sanitize_filter_value type-aware so text fields keep the literal "true"/"false"/"null" strings; add shared build_query_filters helper - base: correct the self-contradictory sortable_by docstring Frontend (React/TS): - transform: detect dayjs with dayjs.isDayjs() instead of truthy `value.date` (a plain object with a `date` key crashed form submission) - transform: parse date/time strings to dayjs only for date widgets, threaded through change/inline/async-select so text fields are no longer corrupted - form-container: Checkbox.Group binds via `value`, not `checked` - dashboard-action-widget: handle errors on run/refresh actions - change/add: use the react-query v5 invalidateQueries({ queryKey }) signature Adds unit tests for the type-aware filter value, build_query_filters, the widget-aware transforms, and the updated invalidateQueries calls. * Add SQLAlchemy relationship-filter test to restore 100% coverage Covers the new m2m/to-one relationship filter branch in orm_get_list (any() for collections with exact/in, has() for to-one relations).
1 parent c274620 commit 3aaec53

20 files changed

Lines changed: 338 additions & 73 deletions

File tree

fastadmin/api/frameworks/flask/app.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,11 @@ def default(self, o):
3333
@app.errorhandler(Exception)
3434
def exception_handler(exc):
3535
if isinstance(exc, HTTPException):
36-
return exc
37-
return {
38-
"status_code": 500,
39-
"content": {"exception": str(exc)},
40-
}
36+
# Return API errors as JSON {"detail": ...} with the proper HTTP status
37+
# so the shared React frontend can read the message (matching the Django
38+
# and FastAPI integrations), instead of werkzeug's default HTML page.
39+
return {"detail": exc.description}, exc.code or 500
40+
# Unhandled server error: log it server-side but never leak internals to the
41+
# client, and return a real HTTP 500 (a bare dict would be sent as HTTP 200).
42+
logger.error("Unhandled admin error: %s", exc)
43+
return {"detail": "Internal server error."}, 500

fastadmin/api/helpers.py

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,41 @@
11
from pathlib import Path
22
from uuid import UUID
33

4-
from fastadmin.models.schemas import ModelFieldWidgetSchema
5-
6-
7-
def sanitize_filter_value(value: str | list) -> bool | None | str | list:
4+
from fastadmin.models.schemas import ModelFieldWidgetSchema, WidgetType
5+
6+
# Text-like filter widgets whose values are free-form strings. For these the
7+
# literals "true"/"false"/"null" are legitimate content and must NOT be coerced
8+
# to bool/None (otherwise a Char column can never be filtered for those words).
9+
TEXT_FILTER_WIDGET_TYPES = frozenset(
10+
{
11+
WidgetType.Input,
12+
WidgetType.TextArea,
13+
WidgetType.RichTextArea,
14+
WidgetType.JsonTextArea,
15+
WidgetType.SlugInput,
16+
WidgetType.EmailInput,
17+
WidgetType.PhoneInput,
18+
WidgetType.UrlInput,
19+
WidgetType.PasswordInput,
20+
}
21+
)
22+
23+
24+
def sanitize_filter_value(
25+
value: str | list,
26+
field: ModelFieldWidgetSchema | None = None,
27+
) -> bool | None | str | list:
828
"""Sanitize value (string or list for __in filters).
929
1030
:params value: a value (str or list of str for __in).
31+
:params field: the field being filtered, used to decide whether the
32+
"true"/"false"/"null" literals should be coerced (skipped for text fields).
1133
:return: A sanitized value.
1234
"""
1335
if isinstance(value, list):
14-
return [sanitize_filter_value(v) for v in value]
36+
return [sanitize_filter_value(v, field) for v in value]
37+
if field is not None and field.filter_widget_type in TEXT_FILTER_WIDGET_TYPES:
38+
return value
1539
match value:
1640
case "false":
1741
return False
@@ -64,6 +88,31 @@ def sanitize_filter_key(key: str, fields: list[ModelFieldWidgetSchema]) -> tuple
6488
return field_name, condition
6589

6690

91+
def build_query_filters(
92+
filters: dict,
93+
fields: list[ModelFieldWidgetSchema],
94+
exclude: tuple[str, ...],
95+
) -> dict[tuple[str, str], bool | None | str | list]:
96+
"""Build the sanitized ``{(field_name, condition): value}`` filter dict.
97+
98+
Resolves each key's field so value coercion (true/false/null) can be applied
99+
in a type-aware way (see :func:`sanitize_filter_value`).
100+
101+
:param filters: raw filters mapping ``key -> value``.
102+
:param fields: model fields with widget types.
103+
:param exclude: keys to skip (search, sort_by, offset, limit).
104+
:return: sanitized filters dict.
105+
"""
106+
result: dict[tuple[str, str], bool | None | str | list] = {}
107+
for key, value in filters.items():
108+
if key in exclude:
109+
continue
110+
field_name = key.partition("__")[0]
111+
field = next((f for f in fields if f.name == field_name), None)
112+
result[sanitize_filter_key(key, fields)] = sanitize_filter_value(value, field)
113+
return result
114+
115+
67116
def is_valid_uuid(uuid_to_test: str) -> bool:
68117
"""Check if uuid_to_test is a valid uuid.
69118

fastadmin/api/service.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from asgiref.sync import sync_to_async
1111

1212
from fastadmin.api.exceptions import AdminApiException
13-
from fastadmin.api.helpers import sanitize_filter_key, sanitize_filter_value
13+
from fastadmin.api.helpers import build_query_filters
1414
from fastadmin.api.schemas import (
1515
ChangePasswordInputSchema,
1616
ExportFormat,
@@ -233,11 +233,11 @@ async def list(
233233
query_filters: dict[tuple[str, str], bool | str | None | list] | None = None
234234
if query_params.filters:
235235
self._validate_filters(admin_model, query_params.filters, exclude_filter_fields, fields)
236-
query_filters = {
237-
sanitize_filter_key(k, admin_model.get_model_fields_with_widget_types()): sanitize_filter_value(v)
238-
for k, v in query_params.filters.items()
239-
if k not in exclude_filter_fields
240-
}
236+
query_filters = build_query_filters(
237+
query_params.filters,
238+
admin_model.get_model_fields_with_widget_types(),
239+
exclude_filter_fields,
240+
)
241241

242242
if query_params.sort_by:
243243
if query_params.sort_by.strip("-") not in fields:
@@ -456,11 +456,11 @@ async def export(
456456
query_filters: dict[tuple[str, str], bool | str | None | list] | None = None
457457
if query_params.filters:
458458
self._validate_filters(admin_model, query_params.filters, exclude_filter_fields, fields)
459-
query_filters = {
460-
sanitize_filter_key(k, admin_model.get_model_fields_with_widget_types()): sanitize_filter_value(v)
461-
for k, v in query_params.filters.items()
462-
if k not in exclude_filter_fields
463-
}
459+
query_filters = build_query_filters(
460+
query_params.filters,
461+
admin_model.get_model_fields_with_widget_types(),
462+
exclude_filter_fields,
463+
)
464464

465465
if query_params.sort_by:
466466
if query_params.sort_by.strip("-") not in fields:

fastadmin/models/base.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,9 @@ class BaseModelAdmin:
202202
# Example of usage: show_full_result_count = True
203203
show_full_result_count: bool = False
204204

205-
# By default, the list page allows sorting by all model fields
205+
# By default (an empty collection), the list page allows sorting by all model fields.
206206
# If you want to disable sorting for some columns, set sortable_by to a collection (e.g. list, tuple, or set)
207-
# of the subset of list_display that you want to be sortable.
208-
# An empty collection disables sorting for all columns.
207+
# of the subset of list_display that you want to be sortable; columns not listed become non-sortable.
209208
# Example of usage: sortable_by = ("mobile_number", "email")
210209
sortable_by: Sequence[str] = ()
211210

fastadmin/models/orms/ponyorm.py

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,7 @@ def orm_get_list(
272272

273273
search_fields = list(self.search_fields)
274274
if search and search_fields:
275+
model_pk_name = self.get_model_pk_name(self.model_cls)
275276
ids = []
276277
# Bind the user-supplied search term as a local so Pony resolves it
277278
# as a parameter. Only the field path (from the admin's trusted
@@ -281,17 +282,24 @@ def orm_get_list(
281282
pony_search_field = search_field.replace("__", ".")
282283
qs_ids = qs.filter(f"search_term in m.{pony_search_field}.lower()")
283284
objs = list(qs_ids)
284-
ids += [o.id for o in objs]
285-
qs = qs.filter(lambda m: m.id in set(ids))
285+
ids += [getattr(o, model_pk_name) for o in objs]
286+
# Bind the collected pks as a local so Pony resolves it as a
287+
# parameter; only the trusted pk field path is interpolated.
288+
pk_ids = set(ids) # noqa: F841 (referenced by Pony filter string)
289+
qs = qs.filter(f"m.{model_pk_name} in pk_ids")
286290

287291
ordering = [sort_by] if sort_by else self.ordering
288292
if ordering:
289-
desc_fields = [o[1:] for o in ordering if o.startswith("-")]
290-
asc_fields = [o for o in ordering if not o.startswith("-")]
291-
if asc_fields:
292-
qs = qs.order_by(*(getattr(self.model_cls, o) for o in asc_fields))
293-
if desc_fields:
294-
qs = qs.order_by(*(desc(getattr(self.model_cls, o)) for o in desc_fields))
293+
# Build a single order_by() call that preserves the declared field
294+
# order. Pony prepends the fields of each separate order_by() call,
295+
# so applying asc and desc fields in two calls would invert their
296+
# relative priority (a later desc field wrongly outranks an earlier
297+
# asc one).
298+
order_exprs = [
299+
desc(getattr(self.model_cls, o[1:])) if o.startswith("-") else getattr(self.model_cls, o)
300+
for o in ordering
301+
]
302+
qs = qs.order_by(*order_exprs)
295303

296304
total = qs.count()
297305

fastadmin/models/orms/sqlalchemy.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,21 @@ def order_column(ordering_field: str):
335335
condition = field_with_condition[1]
336336
model_field = getattr(self.model_cls, field)
337337

338+
rel_property = getattr(model_field, "property", None)
339+
related_mapper = getattr(rel_property, "mapper", None)
340+
if related_mapper is not None:
341+
# Relationship field (e.g. m2m): SQLAlchemy rejects a
342+
# scalar comparison against a collection, so match on the
343+
# related row's pk via any()/has() instead.
344+
related_cls = related_mapper.class_
345+
related_pk = getattr(related_cls, self.get_model_pk_name(related_cls))
346+
match_expr = related_pk.in_(value) if condition == "in" else related_pk == value
347+
if getattr(rel_property, "uselist", True):
348+
q.append(model_field.any(match_expr))
349+
else:
350+
q.append(model_field.has(match_expr))
351+
continue
352+
338353
if condition != "in" and isinstance(model_field.expression.type, BIGINT | Integer):
339354
with contextlib.suppress(ValueError, TypeError):
340355
value = int(value)

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ vi.mock("@/helpers/forms", () => ({
163163
}));
164164

165165
vi.mock("@/helpers/transform", () => ({
166+
getChangeWidgetTypes: () => ({}),
166167
transformDataFromServer: (v: unknown) => v,
167168
}));
168169

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ import { getFetcher, patchFetcher, postFetcher } from "@/fetchers/fetchers";
2828
import { getConfigurationModel } from "@/helpers/configuration";
2929
import { handleError } from "@/helpers/forms";
3030
import { getTitleFromModel } from "@/helpers/title";
31-
import { transformDataFromServer } from "@/helpers/transform";
31+
import {
32+
getChangeWidgetTypes,
33+
transformDataFromServer,
34+
} from "@/helpers/transform";
3235
import { EModelPermission } from "@/interfaces/configuration";
3336
import { ConfigurationContext } from "@/providers/ConfigurationProvider";
3437

@@ -87,9 +90,12 @@ export const AsyncSelect: React.FC<IAsyncSelect> = ({
8790
const asyncSelectChangeInitialValues = useMemo(
8891
() =>
8992
initialChangeValues != null
90-
? transformDataFromServer(initialChangeValues)
93+
? transformDataFromServer(
94+
initialChangeValues,
95+
getChangeWidgetTypes(modelConfiguration),
96+
)
9197
: undefined,
92-
[initialChangeValues],
98+
[initialChangeValues, modelConfiguration],
9399
);
94100

95101
const {

frontend/src/components/dashboard-action-widget/index.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { useCallback, useMemo, useRef, useState } from "react";
1616
import { useTranslation } from "react-i18next";
1717

1818
import { postFetcher } from "@/fetchers/fetchers";
19+
import { handleError } from "@/helpers/forms";
1920
import { getTitleFromFieldName } from "@/helpers/title";
2021
import { transformValueToServer } from "@/helpers/transform";
2122
import { getWidgetCls } from "@/helpers/widgets";
@@ -211,6 +212,8 @@ export const DashboardActionWidget: React.FC<DashboardActionWidgetProps> = ({
211212
);
212213
setActionResult(result as IWidgetActionResponse);
213214
setResultsView("json");
215+
} catch (error) {
216+
handleError(error);
214217
} finally {
215218
setIsActionRunning(false);
216219
}
@@ -229,6 +232,8 @@ export const DashboardActionWidget: React.FC<DashboardActionWidgetProps> = ({
229232
},
230233
);
231234
setActionResult(result as IWidgetActionResponse);
235+
} catch (error) {
236+
handleError(error);
232237
} finally {
233238
setIsActionRefreshing(false);
234239
}

frontend/src/components/form-container/index.tsx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -185,11 +185,12 @@ export const FormContainer: React.FC<IFormContainer> = ({
185185
] as any
186186
}
187187
valuePropName={
188-
[
189-
EFieldWidgetType.Checkbox,
190-
EFieldWidgetType.Switch,
191-
EFieldWidgetType.CheckboxGroup,
192-
].includes(getConf(field).form_widget_type as EFieldWidgetType)
188+
// Only single boolean widgets use `checked`. Checkbox.Group is
189+
// controlled via `value` (an array); binding it to `checked` would
190+
// stop stored selections from rendering.
191+
[EFieldWidgetType.Checkbox, EFieldWidgetType.Switch].includes(
192+
getConf(field).form_widget_type as EFieldWidgetType,
193+
)
193194
? "checked"
194195
: undefined
195196
}

0 commit comments

Comments
 (0)