Tenancy walk perf + WHERE-only leak fix, PermissionDenied, TenantDeleted, validator related fields, wait_for, tooling checks, test gaps (#491 #512 #514 #518 #520 #558 #559)

.github/workflows/pr.yml

@@ -37,6 +37,12 @@ jobs:
           cache-dependency-glob: ${{ env.UV_CACHE_GLOB }}
       - run: make install-py
       - run: make ci-python-lint
+      # Package metadata, release matrix, and the hand-maintained ty
+      # extra-paths / pytest testpaths lists (#520) must match what is on disk.
+      - name: Package metadata and tooling lists
+        run: uv run python scripts/check_metadata.py
+      # Fixed sleeps in tests are timing bets; poll with simple_module_test.wait_for.
+      - run: make ci-check-test-sleeps
       # The checked-in typed-key files must match what the installed modules'
       # catalogs emit. Catches a catalog edit committed without regenerating, and
       # (with the namespace guard in gen_i18n.py) a regeneration that dropped keys.

CHANGELOG.md

@@ -12,6 +12,15 @@ All notable changes to this project are documented in this file. The format is b
 ## [Unreleased]
 
 ### Changed
+- `RequiresPermission` raises `simple_module_core.permission_denied.PermissionDenied`
+  (an `HTTPException` carrying `.permission`); the 403 handler reads the
+  attribute instead of parsing the message (#518). `PERMISSION_DENIED_PREFIX`
+  stays importable.
+- Settings cross-field validators (background_tasks, file_storage, site_lock,
+  users, keycloak) name their related fields, so lenient hydration drops only
+  the stored culprit instead of resetting the whole module (#558).
+- The tenancy scope walk is about 2× cheaper for statements that pass through
+  unchanged (get 26→15 µs, pagination count 83→44 µs) (#559).
 - **Behaviour change (#494):** a pre-app settings read that fails (connection,
   query, import error) is no longer treated as a fresh install. Outside
   development and testing the host and the Celery worker refuse to boot with
@@ -96,6 +105,14 @@ All notable changes to this project are documented in this file. The format is b
   previously served, and anonymous, in production.
 
 ### Added
+- `TenantDeleted` event and `TenantService.delete_tenant` (#491). No endpoint
+  yet: what deleting a tenant means for other modules' rows is still open.
+- `simple_module_test.wait_for` / `wait_for_sync`; `make ci-check-test-sleeps`
+  flags unmarked fixed sleeps in tests (#512).
+- `scripts/check_metadata.py` asserts the ty `extra-paths` and pytest
+  `testpaths` lists match the packages on disk, in `make lint` and CI (#520).
+- Tests for the api_tokens, feature_flags and background_tasks pages,
+  `tenants/shared_props.py` and `file_storage/upload_guards.py` (#514).
 - `PATCH /api/users/admin/{id}/unlock` lifts a login lockout early (audited).
   `UserListItem` carries `locked_until`; the users list shows a "locked" badge
   and the edit page a "Locked until" row with an Unlock action (#530).
@@ -368,6 +385,10 @@ All notable changes to this project are documented in this file. The format is b
   *complete* verdict now keeps answering while one refresh runs.
 
 ### Security
+- An ORM select that reads a raw `Model.__table__` (or an `aliased()` entity's
+  table) only in its WHERE clause now gets the tenant filter, and strict mode
+  refuses it without a tenant; such a select could read another tenant's link
+  rows (#559, extends #506).
 - Tenant and soft-delete filters now cover Core statements that used to leak
   other tenants' or deleted rows (#506): outer and FULL joins over
   `Model.__table__`, table aliases, subqueries, CTEs, scalar/correlated

Makefile

@@ -1,4 +1,4 @@
-.PHONY: install install-py install-js dev dev-api dev-ui build test test-py test-py-pg test-js test-e2e bench memray-run memray-flamegraph loadtest loadtest-seed loadtest-memray bench-nav lint doctor migrate migration downgrade migration-history migrations-roundtrip-pg docker-up docker-down kill new-module gen-pages openapi gen-i18n docker-build docker-app docker-compose-app sync-module-deps ci-python-lint ci-python-typecheck ci-js-lint ci-js-typecheck ci-check-file-size ci-check-hardcoded-strings ci-check-untranslated ci-build-packages worker beat worker-docker
+.PHONY: install install-py install-js dev dev-api dev-ui build test test-py test-py-pg test-js test-e2e bench memray-run memray-flamegraph loadtest loadtest-seed loadtest-memray bench-nav lint doctor migrate migration downgrade migration-history migrations-roundtrip-pg docker-up docker-down kill new-module gen-pages openapi gen-i18n docker-build docker-app docker-compose-app sync-module-deps ci-python-lint ci-python-typecheck ci-js-lint ci-js-typecheck ci-check-file-size ci-check-test-sleeps ci-check-hardcoded-strings ci-check-untranslated ci-build-packages worker beat worker-docker
 
 # Install
 install:
@@ -121,7 +121,7 @@ loadtest:                   ## Run locust against a server already on $(LOCUST_H
 loadtest-memray:            ## Start uvicorn under memray, load-test, emit flamegraph
 	scripts/loadtest_memray.sh $(LOCUST_ARGS)
 
-lint: ci-python-lint ci-python-typecheck ci-js-lint ci-js-typecheck ci-check-file-size ci-check-hardcoded-strings ci-check-untranslated
+lint: ci-python-lint ci-python-typecheck ci-js-lint ci-js-typecheck ci-check-file-size ci-check-test-sleeps ci-check-hardcoded-strings ci-check-untranslated
 	uv run python scripts/check_metadata.py
 	uv run python scripts/check_readmes.py
 
@@ -158,6 +158,11 @@ ci-js-typecheck:
 ci-check-file-size:
 	uv run python scripts/check_file_size.py
 
+# Fail on a fixed sleep(<non-zero>) in a test without `# intentional-sleep: <reason>`;
+# poll with simple_module_test.wait_for instead.
+ci-check-test-sleeps:
+	uv run python scripts/check_test_sleeps.py
+
 # Enforce that permissions, role names, Inertia page ids, and module dependency
 # names are declared as named constants rather than hardcoded string literals.
 ci-check-hardcoded-strings:

docs/modules/tenants.md

@@ -85,6 +85,8 @@ The module ships no billing, but a billing module needs nothing more from it:
 - `TenantService.set_status(tenant_id, TenantStatus.SUSPENDED | ACTIVE)` for
   dunning.
 - Events, published after commit: `TenantCreated`, `TenantStatusChanged`,
+  `TenantDeleted` (`tenant_id`, `slug`, `name`; from
+  `TenantService.delete_tenant` — subscribe to clean up per-tenant state),
   `MembershipAdded`, `MembershipRemoved`, `InvitationCreated` (also the hook a
   mailer uses to deliver the invitation link).
 

framework/core/simple_module_core/permission_denied.py

@@ -0,0 +1,28 @@
+"""The 403 raised when a principal lacks a named permission."""
+
+from __future__ import annotations
+
+from fastapi import HTTPException
+
+__all__ = ["PERMISSION_DENIED_PREFIX", "PermissionDenied"]
+
+PERMISSION_DENIED_PREFIX = "Permission required: "
+"""How the default 403 message spells the missing permission (human text only).
+
+Nothing parses it: handlers read :attr:`PermissionDenied.permission`.
+"""
+
+
+class PermissionDenied(HTTPException):
+    """A 403 that knows which permission was missing.
+
+    ``detail`` is a human message; the 403 error page and any other consumer
+    read :attr:`permission` rather than parsing that text.
+    """
+
+    def __init__(self, permission: str, detail: str | None = None) -> None:
+        super().__init__(
+            status_code=403,
+            detail=detail if detail is not None else f"{PERMISSION_DENIED_PREFIX}{permission}",
+        )
+        self.permission = permission

framework/core/simple_module_core/stored_settings.py

@@ -15,12 +15,14 @@
 from typing import Any
 
 from pydantic import ValidationError
-from pydantic_core import ErrorDetails
+from pydantic_core import ErrorDetails, InitErrorDetails, PydanticCustomError
+from pydantic_core import ValidationError as CoreValidationError
 from pydantic_settings import BaseSettings
 
 __all__ = [
     "RELATED_FIELDS_CTX",
     "construct_leniently",
+    "field_error",
     "parse_stored_value",
 ]
 
@@ -35,6 +37,34 @@
 _FALSE = frozenset({"0", "false", "no", "off"})
 
 
+def field_error(
+    model: type,
+    field: str,
+    error_type: str,
+    message: str,
+    *,
+    value: Any = None,
+    related: list[str] | tuple[str, ...] = (),
+) -> CoreValidationError:
+    """A cross-field failure from a ``model_validator``, located on ``field``.
+
+    A bare ``ValueError`` from a model validator has ``loc=()``: the settings
+    form cannot show it and lenient hydration cannot tell which stored field to
+    drop (#558). This pins the error on ``field`` and names every field the rule
+    reads under ``ctx[RELATED_FIELDS_CTX]``. ``message`` is used verbatim.
+    """
+    return CoreValidationError.from_exception_data(
+        title=model.__name__,
+        line_errors=[
+            InitErrorDetails(
+                type=PydanticCustomError(error_type, message, {RELATED_FIELDS_CTX: list(related)}),
+                loc=(field,),
+                input=value,
+            )
+        ],
+    )
+
+
 def parse_stored_value(raw: str, value_type: str) -> Any:
     """Decode a stored value according to its ``value_type`` label.
 

framework/core/tests/test_events.py

@@ -6,6 +6,7 @@
 from dataclasses import dataclass
 
 from simple_module_core.events import Event, EventBus
+from simple_module_test import wait_for
 
 
 @dataclass
@@ -107,7 +108,9 @@ async def handler(e):
 
         bus.subscribe(OrderCreated, handler)
         bus.publish_nowait(OrderCreated(order_id=99))
-        await asyncio.sleep(0.05)
+        await wait_for(lambda: len(received) == 1)
+        for _ in range(10):  # let any duplicate delivery land before counting
+            await asyncio.sleep(0)
         assert len(received) == 1
         assert received[0].order_id == 99
 
@@ -161,7 +164,9 @@ async def test_handlers_dispatched_concurrently(self):
         order: list[str] = []
 
         async def slow(e):
-            await asyncio.sleep(0.02)
+            await asyncio.sleep(
+                0.02
+            )  # intentional-sleep: "fast" must finish while this is still pending
             order.append("slow")
 
         async def fast(e):

framework/db/simple_module_db/_core_walk.py

@@ -0,0 +1,188 @@
+"""The cheap walk ``core_scope`` runs on every statement (#506, #559).
+
+``references_plain`` answers one question — does this statement read a
+scoped table outside the ORM's annotations? — and ORM statements usually
+answer no, so this runs on nearly every query while tenancy is on. It is a
+reachability search over the clause tree, pruned at ORM-annotated elements;
+everything here is about expanding fewer nodes, or expanding them more
+cheaply, without changing which elements are reachable (#559):
+
+* each element is expanded once, however many parents name it;
+* a bound parameter has no clause children and is not expanded;
+* a SELECT's declared children skip its empty collections, and the FROMs
+  ``froms_read`` would add are added only when its internals do not
+  already hold them (or they are ORM-annotated, which the walk prunes);
+* ``table`` is looked up without the costly ``AttributeError`` that a
+  column expression's ``__getattr__`` raises for it.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from sqlalchemy.sql.elements import ColumnClause, ColumnElement
+from sqlalchemy.sql.selectable import Alias, Join, Select, TableClause
+from sqlalchemy.sql.traversals import _get_children
+
+from simple_module_db._core_joins import _base_name, _is_orm, is_plain_leaf
+
+
+def orm_less(stmt: Any) -> bool:
+    """A SELECT naming no ORM entity in its columns, FROM list or joins.
+
+    Loader criteria have nothing to attach to there — ``exists().where(
+    Doc.x == ...)`` — so the tables its ``WHERE`` reads must be scoped like
+    Core ones. In an ORM select those tables are the entities' own.
+    """
+    targets = [entry[0] for entry in stmt._setup_joins]
+    return not any(_is_orm(el) for el in (*stmt._raw_columns, *stmt._from_obj, *targets))
+
+
+def where_froms(stmt: Any) -> list[Any]:
+    return [f for crit in stmt._where_criteria for f in crit._from_objects]
+
+
+def froms_read(stmt: Any) -> list[Any]:
+    """The FROMs a SELECT reads besides its joins, and the extra FROMs of a DML."""
+    if not isinstance(stmt, Select):
+        target = stmt.table._deannotate()
+        return [f for f in where_froms(stmt) if f is not target]
+    froms = [*stmt.columns_clause_froms, *(f for f in stmt._from_obj if not isinstance(f, Join))]
+    return [*froms, *(where_froms(stmt) if orm_less(stmt) else core_where_froms(stmt))]
+
+
+def core_where_froms(stmt: Select) -> list[Any]:
+    """The FROMs a SELECT's ``WHERE`` pulls in through plain (non-ORM) columns.
+
+    ``select(Tag.name).where(link.c.tag_id == Tag.id)`` reads ``link`` only
+    through its ``WHERE``. ``where_froms`` cannot be used for an ORM select:
+    an entity column's ``_from_objects`` is its bare table too, and an
+    ``aliased()`` entity's would gain a stray cartesian FROM. So the criteria
+    are walked down to their columns, pruning ORM-annotated elements and
+    anything that adds no FROM to this SELECT (a nested select's own
+    references are scoped where that select is visited).
+    """
+    found: list[Any] = []
+    stack = list(stmt._where_criteria)
+    while stack:
+        el = stack.pop()
+        if _is_orm(el) or not el._from_objects:
+            continue
+        if isinstance(el, ColumnClause):
+            found.extend(el._from_objects)
+        else:
+            stack.extend(el.get_children())
+    return found
+
+
+def references_plain(stmt: Any, names: set[str]) -> bool:
+    """Whether ``stmt`` reads a scoped table outside the ORM's annotations.
+
+    ORM-annotated elements are not descended into: their tables are the
+    loader criteria's business, and every ORM column leads to a plain table.
+    The answer depends only on *which* elements are reachable, so an element
+    met twice is expanded once. ``seen`` keeps the objects, not just their
+    ids, so a temporary child list cannot free one and recycle its id.
+    """
+    stack = [stmt]
+    seen: dict[int, Any] = {}
+    while stack:
+        el = stack.pop()
+        if id(el) in seen or _is_orm(el):
+            continue
+        seen[id(el)] = el
+        target = el if isinstance(el, (TableClause, Alias)) else _table_of(el)
+        if (
+            isinstance(target, (TableClause, Alias))
+            and is_plain_leaf(target)
+            and _base_name(target) in names
+        ):
+            return True
+        if el.__visit_name__ != "bindparam":  # a bound value: no clause children
+            stack.extend(_children(el))
+    return False
+
+
+_SELECT_OMIT = ("_correlate", "_correlate_except")
+_EMPTYABLE = (tuple, list)
+
+
+def _children(el: Any) -> list[Any]:
+    # ``Select.get_children`` adds every FROM it derives from the columns and
+    # WHERE, deannotated, so every ORM statement would look Core. Its declared
+    # internals plus the FROMs ``froms_read`` would scope are what counts —
+    # less what the internals already hold: the FROM list itself, and the
+    # columns-clause FROMs of an all-entity ORM columns clause (#559).
+    if isinstance(el, Select):
+        kids = _declared_children(el)
+        if not _orm_columns_only(el):
+            kids.extend(el.columns_clause_froms)
+        if orm_less(el):
+            kids.extend(where_froms(el))
+        return kids
+    if el.__visit_name__ in ("update", "delete"):
+        return [*el.get_children(), *froms_read(el)]
+    return list(el.get_children())
+
+
+def _declared_children(stmt: Select) -> list[Any]:
+    """``HasTraverseInternals.get_children(stmt, omit_attrs=_SELECT_OMIT)``, as a list.
+
+    The same generated dispatch, minus the call per empty collection: most
+    of a SELECT's ~30 declared attributes are ``()`` on any given statement.
+    """
+    kids: list[Any] = []
+    dispatch = _get_children.run_generated_dispatch(
+        stmt, stmt._traverse_internals, "_generated_get_children_traversal"
+    )
+    for attrname, obj, meth in dispatch:
+        if obj is None or (obj.__class__ in _EMPTYABLE and not obj) or attrname in _SELECT_OMIT:
+            continue
+        kids.extend(meth(obj))
+    return kids
+
+
+def _orm_columns_only(stmt: Select) -> bool:
+    """Every ``columns_clause_froms`` entry is ORM-annotated, so the walk prunes it.
+
+    Under the ORM compile plugin an annotated column's FROM is its entity's
+    ``__clause_element__()`` — a ``Mapper``'s or ``AliasedInsp``'s selectable,
+    annotated with ``parententity`` itself. The Core plugin would give the
+    column's own ``_from_objects`` instead, which may be a plain table.
+    """
+    return stmt._propagate_attrs.get("compile_state_plugin") == "orm" and all(
+        _is_orm(c) for c in stmt._raw_columns
+    )
+
+
+# Per class: does attribute lookup of ``table`` end in ``ColumnElement.__getattr__``?
+_COMPARATOR_LOOKUP: dict[type, bool] = {}
+
+
+def _table_of(el: Any) -> Any:
+    """``getattr(el, "table", None)``, exactly — minus the ``AttributeError`` it costs.
+
+    When no class in the MRO defines ``table``, Python tries the instance
+    ``__dict__`` and then ``__getattr__``; for a ``ColumnElement`` that is
+    ``getattr(self.comparator, "table")``, which this calls directly instead
+    of letting ``__getattr__`` format an error message to be thrown away.
+    Any other class (one that defines ``table``, has no ``__dict__`` or its
+    own ``__getattr__``) takes the plain ``getattr``.
+    """
+    cls = el.__class__
+    via_comparator = _COMPARATOR_LOOKUP.get(cls)
+    if via_comparator is None:
+        via_comparator = _COMPARATOR_LOOKUP[cls] = (
+            cls.__getattr__ is ColumnElement.__getattr__  # type: ignore[attr-defined]
+            if issubclass(cls, ColumnElement)
+            and cls.__dictoffset__ != 0
+            and not any("table" in vars(k) for k in cls.__mro__)
+            else False
+        )
+    if not via_comparator:
+        return getattr(el, "table", None)
+    found = el.__dict__.get("table", _MISSING)
+    return getattr(el.comparator, "table", None) if found is _MISSING else found
+
+
+_MISSING = object()