diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 184371bbf..f6718bfc7 100644 --- a/.github/workflows/pr.yml +++ b/.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. diff --git a/CHANGELOG.md b/CHANGELOG.md index fa1b1ac6f..981f36aea 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Makefile b/Makefile index 97184b723..b45d4186b 100644 --- a/Makefile +++ b/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() in a test without `# intentional-sleep: `; +# 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: diff --git a/docs/modules/tenants.md b/docs/modules/tenants.md index 17f465040..be679be6b 100644 --- a/docs/modules/tenants.md +++ b/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). diff --git a/framework/core/simple_module_core/permission_denied.py b/framework/core/simple_module_core/permission_denied.py new file mode 100644 index 000000000..b41156945 --- /dev/null +++ b/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 diff --git a/framework/core/simple_module_core/stored_settings.py b/framework/core/simple_module_core/stored_settings.py index 4f0b16839..c5ea52f2a 100644 --- a/framework/core/simple_module_core/stored_settings.py +++ b/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. diff --git a/framework/core/tests/test_events.py b/framework/core/tests/test_events.py index 3dfe88704..342c9f36f 100644 --- a/framework/core/tests/test_events.py +++ b/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): diff --git a/framework/db/simple_module_db/_core_walk.py b/framework/db/simple_module_db/_core_walk.py new file mode 100644 index 000000000..9c69ddfc4 --- /dev/null +++ b/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() diff --git a/framework/db/simple_module_db/core_scope.py b/framework/db/simple_module_db/core_scope.py index a927df09e..dc8742b3f 100644 --- a/framework/db/simple_module_db/core_scope.py +++ b/framework/db/simple_module_db/core_scope.py @@ -6,7 +6,7 @@ a join's ``ON``), a member of a UNION / INTERSECT / EXCEPT, the extra FROM of an UPDATE / DELETE, or the SELECT feeding an ``INSERT ... FROM SELECT``. -One cheap walk decides whether the statement holds any such reference; ORM +One cheap walk (``_core_walk``) decides whether the statement holds any such reference; ORM statements usually do not and are returned untouched. Otherwise the statement is cloned once (``cloned_traverse``, a single clone map, so a correlated subquery keeps pointing at the same outer FROM) and every SELECT @@ -26,72 +26,10 @@ from typing import Any from sqlalchemy.sql import visitors -from sqlalchemy.sql.selectable import Alias, Join, Select, TableClause -from sqlalchemy.sql.visitors import HasTraverseInternals - -from simple_module_db._core_joins import ( - Predicates, - _base_name, - _is_orm, - is_plain_leaf, - same_from, - scope_joins, -) -from simple_module_db._outer_joins import is_excluded, orm_outer_join_tables - - -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 _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 froms - -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. - """ - stack = [stmt] - while stack: - el = stack.pop() - if _is_orm(el): - continue - target = el if isinstance(el, (TableClause, Alias)) else getattr(el, "table", None) - if ( - isinstance(target, (TableClause, Alias)) - and is_plain_leaf(target) - and _base_name(target) in names - ): - return True - stack.extend(_children(el)) - return False - - -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. - if isinstance(el, Select): - omit = ("_correlate", "_correlate_except") - return [*HasTraverseInternals.get_children(el, omit_attrs=omit), *_froms_read(el)] - if el.__visit_name__ in ("update", "delete"): - return [*el.get_children(), *_froms_read(el)] - return list(el.get_children()) +from simple_module_db._core_joins import Predicates, is_plain_leaf, same_from, scope_joins +from simple_module_db._core_walk import froms_read, references_plain +from simple_module_db._outer_joins import is_excluded, orm_outer_join_tables def _executable_options(stmt: Any) -> list[Any]: @@ -99,10 +37,6 @@ def _executable_options(stmt: Any) -> list[Any]: return [o for el in visitors.iterate(stmt) for o in getattr(el, "_with_options", ())] -def _where_froms(stmt: Any) -> list[Any]: - return [f for crit in stmt._where_criteria for f in crit._from_objects] - - def _add_where(stmt: Any, preds: list[Any]) -> None: if preds: # ``stmt`` is this traversal's own clone: mutating it is safe stmt._where_criteria = (*stmt._where_criteria, *preds) @@ -130,12 +64,12 @@ def select(self, stmt: Any) -> None: # A bare table an ORM entity LEFT-joins in is filtered by that # join's ON; a WHERE on it would make the join inner (#417). outer = orm_outer_join_tables(stmt, self.orm_covered) if self.orm_covered else [] - candidates = [f for f in _froms_read(stmt) if not (outer and is_excluded(f, outer))] + candidates = [f for f in froms_read(stmt) if not (outer and is_excluded(f, outer))] _add_where(stmt, [*joined.where, *self._preds(candidates, joined.covered)]) def dml(self, stmt: Any) -> None: """The tables an UPDATE / DELETE joins in through its ``WHERE``.""" - _add_where(stmt, self._preds(_froms_read(stmt), [])) + _add_where(stmt, self._preds(froms_read(stmt), [])) def scope_core( @@ -146,7 +80,7 @@ def scope_core( ``names`` are the scoped table names. ``orm_covered`` are the classes the caller gives loader criteria (for the #417 exclusion), or ``None``. """ - if not names or not _references_plain(stmt, names): + if not names or not references_plain(stmt, names): return stmt visitor = _Visitor(predicates_for, orm_covered) return visitors.cloned_traverse( diff --git a/framework/db/tests/test_db_core_in_orm_select.py b/framework/db/tests/test_db_core_in_orm_select.py new file mode 100644 index 000000000..36d71595f --- /dev/null +++ b/framework/db/tests/test_db_core_in_orm_select.py @@ -0,0 +1,142 @@ +"""Core references hidden inside an otherwise-ORM SELECT are still scoped (#559). + +``scope_core``'s walk skips ORM-annotated elements and never expands most of +an ORM select's FROMs, so these pin that a plain ``Model.__table__`` column +or subquery anywhere in an ORM statement — its columns, ``WHERE``, +``HAVING``, ``ORDER BY`` or a join — keeps reaching the filters. Tenant A +sees one link (id 1, on tag ``used``); tenant B's links 7, 8 and 9 also +point at ``used`` and must never be counted. +""" + +from __future__ import annotations + +import pytest +from _core_scope_models import Link, Plain, Tag, _rows, link, make_strict, seeded, tag +from simple_module_db.tenancy import TenantIsolationError +from sqlalchemy import func, or_, select +from sqlalchemy.orm import aliased + +__all__ = ["seeded"] + + +def _links_of_tag(): + """Correlated: tenant A's links on the outer ``Tag`` row.""" + return select(func.count()).where(link.c.tag_id == Tag.id).scalar_subquery() + + +def _b_article_links(): + """Uncorrelated: links on B's article 7 — none for tenant A.""" + return select(func.count()).select_from(link).where(link.c.article_id == 7).scalar_subquery() + + +SHAPES = { + "core column": ( + lambda: select(Tag.name, link.c.id).where(link.c.tag_id == Tag.id), + [("used", 1)], + ), + "core column, entity first": ( + lambda: select(Tag, link.c.article_id).where(link.c.tag_id == Tag.id), + "one", + ), + "scalar subquery column": ( + lambda: select(Tag.name, _links_of_tag()), + [("unused", 0), ("used", 1)], + ), + "core column in WHERE": ( + lambda: select(Tag.name).where(link.c.tag_id == Tag.id, link.c.article_id == 7), + [], + ), + "core column in WHERE, aliased entity": ( + lambda: select((ta := aliased(Tag)).name).where( + link.c.tag_id == ta.id, link.c.article_id == 7 + ), + [], + ), + # Not leaks: the entity's own table through a Core column, and a Core + # column on an outer-joined entity, whose ON already holds its criteria + # (a WHERE predicate there would turn the LEFT join inner, #417). + "entity's own table in WHERE": ( + lambda: select(Tag.name).where(tag.c.id == Tag.id, tag.c.name == "used"), + [("used",)], + ), + "core column on outer-joined entity": ( + lambda: ( + select(Tag.name, Link.id) + .outerjoin(Link, Link.tag_id == Tag.id) + .where(or_(link.c.id.is_(None), link.c.article_id == 1)) + ), + [("unused", None), ("used", 1)], + ), + "core IN subquery in WHERE": ( + lambda: select(Tag.name).where( + Tag.id.in_(select(link.c.tag_id).where(link.c.article_id == 7)) + ), + [], + ), + "core scalar in WHERE": ( + lambda: select(Tag.name).where(_links_of_tag() > 1), + [], + ), + "core scalar in HAVING": ( + lambda: select(Tag.name).group_by(Tag.name).having(_b_article_links() > 0), + [], + ), + "core scalar in ORDER BY": ( + # Tenant A: ``used`` has 1 link, so it sorts first. Counting B's + # links (4) would leave the alphabetical order. + lambda: select(Tag.name).order_by((_links_of_tag() == 1).desc(), Tag.name), + "ordered", + ), + "core inner join": ( + lambda: select(Tag.name).join(link, link.c.tag_id == Tag.id), + [("used",)], + ), + "core outer join": ( + lambda: select(Tag.name, link.c.id).outerjoin(link, link.c.tag_id == Tag.id), + [("unused", None), ("used", 1)], + ), + "core subquery join": ( + lambda: select(Tag.name).join( + (sq := select(link.c.tag_id).subquery()), sq.c.tag_id == Tag.id + ), + [("used",)], + ), + "core exists in ORM join ON": ( + lambda: select(Tag.name).join(Link, (Link.tag_id == Tag.id) & (_b_article_links() == 0)), + [("used",)], + ), +} + + +@pytest.mark.parametrize("shape", list(SHAPES)) +async def test_core_inside_orm_select_is_scoped(seeded, shape): + build, expected = SHAPES[shape] + if expected == "ordered": + names = [r[0] for r in (await seeded.execute(build())).all()] + assert names == ["used", "unused"] + elif expected == "one": + rows = (await seeded.execute(build())).all() + assert [(r[0].name, r[1]) for r in rows] == [("used", 1)] + else: + assert await _rows(seeded, build()) == expected + + +# ``Plain`` carries no criteria of its own, so the refusal can only come +# from the Core reference to ``link``. +STRICT = { + "column": lambda: select(Plain.name, link.c.id).where(link.c.tag_id == Plain.id), + "WHERE": lambda: select(Plain.name).where(link.c.tag_id == Plain.id), + "WHERE subquery": lambda: select(Plain.name).where(Plain.id.in_(select(link.c.tag_id))), + "HAVING": lambda: select(Plain.name).group_by(Plain.name).having(_b_article_links() > 0), + "ORDER BY": lambda: select(Plain.name).order_by( + select(func.count()).where(link.c.tag_id == Plain.id).scalar_subquery() + ), + "join": lambda: select(Plain.name).join(link, link.c.tag_id == Plain.id), +} + + +@pytest.mark.parametrize("shape", list(STRICT)) +async def test_core_inside_orm_select_refuses_without_tenant(seeded, shape): + make_strict(seeded) + with pytest.raises(TenantIsolationError): + await seeded.execute(STRICT[shape]()) diff --git a/framework/hosting/simple_module_hosting/_error_handlers.py b/framework/hosting/simple_module_hosting/_error_handlers.py index a4c05571a..8a19b4db3 100644 --- a/framework/hosting/simple_module_hosting/_error_handlers.py +++ b/framework/hosting/simple_module_hosting/_error_handlers.py @@ -10,6 +10,7 @@ from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse from simple_module_core.exceptions import NotFoundError +from simple_module_core.permission_denied import PermissionDenied from simple_module_inertia import ( Inertia, InertiaConfig, @@ -22,7 +23,6 @@ from simple_module_hosting._inertia_shared import _INERTIA_HEADER from simple_module_hosting._observability import CorrelationIdMiddleware -from simple_module_hosting.permissions import PERMISSION_DENIED_PREFIX logger = logging.getLogger(__name__) @@ -55,26 +55,6 @@ def _specific_detail(status_code: int, message: str) -> str: return "" if message.strip().casefold() == phrase.casefold() else message -def _required_permission(message: str) -> str | None: - """The permission named by a ``RequiresPermission`` denial, if this is one. - - The 403 page says "Your role doesn't include ````. Ask an admin to - grant it." — it needs the bare permission name, not the sentence the guard - wrote for logs and API clients. Reading it back off the detail (using the - guard's own exported prefix) keeps ``RequiresPermission`` as the single - place that knows the name; the alternative, stashing it on - ``request.state``, only works when the exception travels the one code path - that set it, and 403s are also raised by hand. - - Anything else — a role-gated ``/admin`` 403, a hand-raised - ``HTTPException(403, "Administrator access required")`` — yields ``None``, - and the page falls back to copy that names no permission. - """ - if not message.startswith(PERMISSION_DENIED_PREFIX): - return None - return message[len(PERMISSION_DENIED_PREFIX) :].strip() or None - - def _login_url(request: Request) -> str | None: """Best-effort login URL for the sign-in statuses. @@ -159,9 +139,13 @@ async def render_error_page( status_code: int, message: str, headers: Mapping[str, str] | None = None, + required_permission: str | None = None, ) -> Response: """Render the Inertia error page for *status_code*. + ``required_permission`` names the permission a 403 lacked (from + :class:`PermissionDenied`); the page words its own sentence around it. + ``headers`` carries an ``HTTPException``'s own headers through to the response. A rendered page is still the same status as the JSON body it replaces, so ``WWW-Authenticate`` on a 401 and ``Retry-After`` on a @@ -208,7 +192,7 @@ async def render_error_page( # Always present, null when no permission is involved: the page # branches on it, and an absent prop is indistinguishable there # from one that was never wired. - "required_permission": _required_permission(message), + "required_permission": required_permission, "login_url": (_login_url(request) if status_code in _SIGN_IN_STATUSES else None), # Set by MaintenanceMiddleware. A planned outage reads very # differently from the same status code arriving unbidden. @@ -249,7 +233,8 @@ async def http_exception_handler(request: Request, exc: HTTPException) -> Respon headers = getattr(exc, "headers", None) if exc.status_code in _INERTIA_ERROR_STATUSES and not _wants_json(request): detail = str(exc.detail) if exc.detail else "" - return await render_error_page(request, exc.status_code, detail, headers) + required = exc.permission if isinstance(exc, PermissionDenied) else None + return await render_error_page(request, exc.status_code, detail, headers, required) return JSONResponse( status_code=exc.status_code, content={"detail": exc.detail}, diff --git a/framework/hosting/simple_module_hosting/permissions.py b/framework/hosting/simple_module_hosting/permissions.py index 2ceb40c40..208ce275e 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING, Any from fastapi import HTTPException, Request +from simple_module_core.permission_denied import PERMISSION_DENIED_PREFIX, PermissionDenied from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants from simple_module_hosting._anonymous import demote_to_anonymous @@ -20,6 +21,7 @@ "DEFAULT_ROLE_PERMISSIONS", "PERMISSION_DENIED_PREFIX", "WILDCARD", + "PermissionDenied", "RequiresPermission", "apply_scope_cap", "ensure_resolved_permissions", @@ -31,13 +33,6 @@ "resolved_permissions_for", ] -# How a denial spells the missing permission in ``HTTPException.detail``. -# Exported because the error-page handler parses the name back out of it to -# tell the visitor what to ask an admin for; two copies of this literal in two -# files is exactly the drift that would leave the 403 page with no permission -# to name and no test to notice. -PERMISSION_DENIED_PREFIX = "Permission required: " - def resolve_permissions( roles: list[str], @@ -259,7 +254,4 @@ async def __call__(self, request: Request) -> None: if getattr(request.state, "user", None) is None: # demoted while resolving raise HTTPException(status_code=401, detail="Authentication required") if not grants(permissions, self.permission): - raise HTTPException( - status_code=403, - detail=f"{PERMISSION_DENIED_PREFIX}{self.permission}", - ) + raise PermissionDenied(self.permission) diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index 49189e868..6df2bb71b 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -16,7 +16,8 @@ import httpx import pytest -from fastapi import Depends, Request +from fastapi import Depends, HTTPException, Request +from simple_module_core.permission_denied import PermissionDenied from simple_module_hosting.permissions import RequiresPermission _NOT_FOUND = 404 @@ -24,6 +25,7 @@ _MISSING_PATH = "/definitely/not/a/real/route" _GUARDED_PERMISSION = "settings.manage" _GUARDED_PATH = "/permission-guard-probe" +_DENIED_PATH = "/permission-denied-probe" def _without_the_guarded_permission(request: Request) -> None: @@ -139,7 +141,8 @@ class TestRequiredPermissionProp: as its own prop. The guard already puts the name in the exception detail ("Permission required: settings.manage"), but that string is a *message* for a human, and rendering it verbatim would leak the wording of a Python - exception into the UI. Parsing it back out here keeps one source of truth + exception into the UI. The handler reads PermissionDenied.permission, keeping + one source of truth for the permission name and lets the page own the sentence. """ @@ -170,6 +173,35 @@ async def test_other_errors_carry_no_permission( assert "required_permission" in props, f"prop missing entirely; props={sorted(props)}" assert props["required_permission"] is None + async def test_page_names_permission_even_when_message_is_reworded( + self, app, authenticated_client: httpx.AsyncClient + ) -> None: + """The permission comes from the exception, not from its text.""" + + async def _probe() -> None: + raise PermissionDenied("reports.export", "Vous n'avez pas le droit") + + app.add_api_route(_DENIED_PATH, _probe, methods=["GET"]) + resp = await authenticated_client.get(_DENIED_PATH) + assert resp.status_code == 403 + props = _inertia_page(resp.text)["props"] + assert props["required_permission"] == "reports.export" + assert props["message"] == "Vous n'avez pas le droit" + + api = await authenticated_client.get(_DENIED_PATH, headers={"Accept": "application/json"}) + assert api.status_code == 403 + assert api.json() == {"detail": "Vous n'avez pas le droit"} + + async def test_message_that_merely_looks_like_the_old_prefix_names_nothing( + self, app, authenticated_client: httpx.AsyncClient + ) -> None: + async def _probe() -> None: + raise HTTPException(403, "Permission required: sneaky.perm") + + app.add_api_route(_DENIED_PATH + "-plain", _probe, methods=["GET"]) + resp = await authenticated_client.get(_DENIED_PATH + "-plain") + assert _inertia_page(resp.text)["props"]["required_permission"] is None + class TestStatusPhraseIsNotAMessage: """The default ``HTTPException.detail`` is the status name, not copy. diff --git a/framework/testing/simple_module_test/__init__.py b/framework/testing/simple_module_test/__init__.py index 934cc16fe..7066efde4 100644 --- a/framework/testing/simple_module_test/__init__.py +++ b/framework/testing/simple_module_test/__init__.py @@ -30,6 +30,7 @@ from simple_module_test.routes import effective_route_paths from simple_module_test.session_cookie import forge_session_cookie from simple_module_test.tenant_client import TenantClient +from simple_module_test.waiting import wait_for, wait_for_sync __all__ = [ "FakeEventBus", @@ -38,4 +39,6 @@ "build_test_app", "effective_route_paths", "forge_session_cookie", + "wait_for", + "wait_for_sync", ] diff --git a/framework/testing/simple_module_test/waiting.py b/framework/testing/simple_module_test/waiting.py new file mode 100644 index 000000000..9b9ff590e --- /dev/null +++ b/framework/testing/simple_module_test/waiting.py @@ -0,0 +1,83 @@ +"""Polling helpers that replace fixed ``sleep()`` waits in tests. + +A fixed sleep is a bet on timing: too short and the test flakes on a loaded +runner, too long and the suite is slow on a fast one. ``wait_for`` polls a +predicate until it is truthy and fails with the last value it saw. +""" + +import asyncio +import inspect +import time +from collections.abc import Awaitable, Callable +from typing import Any + + +def _timeout_error( + predicate: Callable[..., Any], timeout: float, last: Any, error: BaseException | None +) -> AssertionError: + name = getattr(predicate, "__name__", repr(predicate)) + detail = f"last error: {error!r}" if error is not None else f"last value: {last!r}" + return AssertionError(f"wait_for({name}) not satisfied within {timeout}s; {detail}") + + +def _poll_once(predicate: Callable[[], Any]) -> tuple[Any, BaseException | None]: + """Call ``predicate`` once; an exception it raises is returned, not raised.""" + try: + return predicate(), None + except Exception as exc: + return None, exc + + +async def wait_for( + predicate: Callable[[], Any] | Callable[[], Awaitable[Any]], + timeout: float = 2.0, + interval: float = 0.01, +) -> Any: + """Poll ``predicate`` (sync or async) until truthy; return its value. + + Raises ``AssertionError`` with the last value seen on timeout. An exception + raised by the predicate counts as "not yet" and is reported if it is the + last thing seen. + """ + deadline = time.monotonic() + timeout + while True: + value, error = _poll_once(predicate) + if inspect.isawaitable(value): + value, error = await _settle(value, max(deadline - time.monotonic(), 0.001)) + if value: + return value + if time.monotonic() >= deadline: + raise _timeout_error(predicate, timeout, value, error) + await asyncio.sleep(interval) + + +async def _settle(awaitable: Awaitable[Any], budget: float) -> tuple[Any, BaseException | None]: + """Await an async predicate's result, with :func:`_poll_once`'s contract. + + ``budget`` bounds the await, so a predicate that hangs fails the wait at its + timeout instead of blocking the test forever. + """ + try: + return await asyncio.wait_for(awaitable, budget), None + except Exception as exc: + return None, exc + + +def wait_for_sync( + predicate: Callable[[], Any], + timeout: float = 2.0, + interval: float = 0.01, +) -> Any: + """Blocking twin of :func:`wait_for` for sync tests (sync predicates only).""" + deadline = time.monotonic() + timeout + while True: + value, error = _poll_once(predicate) + if inspect.isawaitable(value): + if inspect.iscoroutine(value): + value.close() # never awaited: avoid the RuntimeWarning + raise TypeError("wait_for_sync takes a sync predicate; use wait_for for async ones") + if value: + return value + if time.monotonic() >= deadline: + raise _timeout_error(predicate, timeout, value, error) + time.sleep(interval) # intentional-sleep: the polling interval itself diff --git a/framework/testing/tests/test_waiting_helpers.py b/framework/testing/tests/test_waiting_helpers.py new file mode 100644 index 000000000..17e2e3937 --- /dev/null +++ b/framework/testing/tests/test_waiting_helpers.py @@ -0,0 +1,88 @@ +import asyncio + +import pytest +from simple_module_test import wait_for, wait_for_sync + + +async def test_returns_value_when_already_true(): + assert await wait_for(lambda: "ok") == "ok" + + +async def test_polls_sync_predicate_until_true(): + calls = {"n": 0} + + def pred(): + calls["n"] += 1 + return calls["n"] >= 3 + + assert await wait_for(pred, interval=0.001) is True + assert calls["n"] == 3 + + +async def test_async_predicate(): + state = {"done": False} + + async def pred(): + return state["done"] + + async def flip(): + await asyncio.sleep(0.02) # intentional-sleep: simulate work finishing later + state["done"] = True + + task = asyncio.create_task(flip()) + assert await wait_for(pred, interval=0.005) is True + await task + + +async def test_timeout_reports_last_value(): + with pytest.raises(AssertionError, match=r"last value: 0"): + await wait_for(lambda: 0, timeout=0.05, interval=0.005) + + +async def test_predicate_exception_is_retried_then_reported(): + calls = {"n": 0} + + def pred(): + calls["n"] += 1 + if calls["n"] < 3: + raise KeyError("missing") + return True + + assert await wait_for(pred, interval=0.001) + + def boom(): + raise ValueError("x") + + with pytest.raises(AssertionError, match="last error: ValueError"): + await wait_for(boom, timeout=0.05, interval=0.005) + + +def test_sync_returns_and_polls(): + calls = {"n": 0} + + def pred(): + calls["n"] += 1 + return calls["n"] >= 2 + + assert wait_for_sync(pred, interval=0.001) is True + + +def test_sync_timeout(): + with pytest.raises(AssertionError, match=r"last value: \[\]"): + wait_for_sync(list, timeout=0.05, interval=0.005) + + +async def test_hung_async_predicate_times_out(): + async def hangs(): + await asyncio.sleep(30) # intentional-sleep: never returns on its own + + with pytest.raises(AssertionError, match="not satisfied"): + await wait_for(hangs, timeout=0.05, interval=0.005) + + +def test_sync_rejects_an_async_predicate(): + async def pred(): + return True + + with pytest.raises(TypeError, match="sync predicate"): + wait_for_sync(pred) diff --git a/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx b/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx new file mode 100644 index 000000000..4d3a69256 --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx @@ -0,0 +1,119 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const reload = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { reload, visit: vi.fn(), get: vi.fn(), post: vi.fn() }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import Admin from '../api_tokens/pages/Admin'; +import { stubFetch, token } from './fixtures'; + +const SECRET = 'smpy_svc_adminsecret'; + +const SERVICE = token({ + id: 's1', + kind: 'service', + name: 'ingest-bot', + prefix: 'smpy_zz99', + roles: ['uploader'], + owner_user_id: null, + owner_email: null, + tenant_id: 'acme', +}); + +function setProps(over: Record = {}) { + state.props = { + tokens: [SERVICE], + scopes: ['files.read'], + roles: ['uploader', 'reader'], + tenant_required: false, + max_lifetime_days: 365, + expiry_presets: [7, 30], + csrf_token: 'csrf-xyz', + api_url: '/api/api_tokens', + ...over, + }; +} + +describe('api_tokens Admin', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); + }); + + it('lists every token with its kind and role-based owner', () => { + render(); + expect(screen.getByText('ingest-bot')).toBeInTheDocument(); + expect(screen.getByText(keys.api_tokens.kind.service)).toBeInTheDocument(); + expect(screen.getByText('uploader')).toBeInTheDocument(); + }); + + it('shows the empty state when there are no tokens', () => { + setProps({ tokens: [] }); + render(); + expect(screen.getByText(keys.api_tokens.list.empty_title)).toBeInTheDocument(); + }); + + it('replaces the table with a banner and disables creation without a tenant', () => { + setProps({ tenant_required: true }); + render(); + expect(screen.getByText(keys.api_tokens.admin.tenant_required)).toBeInTheDocument(); + expect(screen.queryByText('ingest-bot')).toBeNull(); + expect( + screen.getByRole('button', { name: keys.api_tokens.admin.new_service_key }), + ).toBeDisabled(); + }); + + it('creates a service key with roles and reveals its secret', async () => { + const fetchMock = stubFetch(201, { token: SECRET, item: SERVICE }); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.admin.new_service_key })); + fireEvent.change(await screen.findByLabelText(keys.api_tokens.form.name), { + target: { value: 'robot' }, + }); + fireEvent.click(screen.getByLabelText('files.read')); + fireEvent.click(screen.getByLabelText('reader')); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.form.submit })); + + expect(await screen.findByText(SECRET)).toBeInTheDocument(); + const [url, init] = fetchMock.mock.calls[0] as unknown as [string, RequestInit]; + expect(url).toBe('/api/api_tokens/admin/service-keys'); + expect(JSON.parse(init.body as string)).toMatchObject({ name: 'robot', roles: ['reader'] }); + }); + + it('confirms before revoking via the admin endpoint, and not when cancelled', async () => { + const fetchMock = stubFetch(200, SERVICE); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: keys.api_tokens.revoke.cancel })); + await waitFor(() => expect(screen.queryByText(keys.api_tokens.revoke.description)).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: keys.api_tokens.revoke.confirm })); + await waitFor(() => expect(toast.success).toHaveBeenCalled()); + expect(fetchMock.mock.calls[0][0]).toBe('/api/api_tokens/admin/tokens/s1/revoke'); + }); + + it('toasts the generic error when revoke fails without a detail', async () => { + stubFetch(500, {}); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: keys.api_tokens.revoke.confirm })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(keys.api_tokens.toasts.failed)); + }); +}); diff --git a/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx b/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx new file mode 100644 index 000000000..30879b1c3 --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx @@ -0,0 +1,69 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +import { CreatedTokenDialog } from '../api_tokens/pages/components/CreatedTokenDialog'; + +const SECRET = 'smpy_pat_0123456789abcdef'; +const c = keys.api_tokens.created; + +function stubClipboard(writeText: ReturnType) { + Object.defineProperty(navigator, 'clipboard', { value: { writeText }, configurable: true }); +} + +describe('CreatedTokenDialog', () => { + beforeEach(() => stubClipboard(vi.fn().mockResolvedValue(undefined))); + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('renders nothing while there is no token', () => { + render(); + expect(screen.queryByText(c.title)).toBeNull(); + }); + + it('shows the secret with the one-time warning', () => { + render(); + expect(screen.getByText(SECRET)).toBeInTheDocument(); + expect(screen.getByText(c.warning)).toBeInTheDocument(); + }); + + it('copies the secret to the clipboard and acknowledges it', async () => { + const writeText = vi.fn().mockResolvedValue(undefined); + stubClipboard(writeText); + render(); + fireEvent.click(screen.getByRole('button', { name: c.copy })); + await waitFor(() => expect(screen.getByRole('button', { name: c.copied })).toBeInTheDocument()); + expect(writeText).toHaveBeenCalledWith(SECRET); + }); + + it('survives a rejected clipboard and keeps offering Copy', async () => { + const writeText = vi.fn().mockRejectedValue(new Error('denied')); + stubClipboard(writeText); + render(); + fireEvent.click(screen.getByRole('button', { name: c.copy })); + await waitFor(() => expect(writeText).toHaveBeenCalled()); + expect(screen.getByRole('button', { name: c.copy })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: c.copied })).toBeNull(); + expect(screen.getByText(SECRET)).toBeInTheDocument(); + }); + + it('closes through Done', () => { + const onClose = vi.fn(); + render(); + fireEvent.click(screen.getByRole('button', { name: c.done })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('forgets the copied state after being closed and reopened', async () => { + const { rerender } = render(); + fireEvent.click(screen.getByRole('button', { name: c.copy })); + await waitFor(() => expect(screen.getByRole('button', { name: c.copied })).toBeInTheDocument()); + fireEvent.click(screen.getByRole('button', { name: c.done })); + rerender(); + rerender(); + expect(screen.getByRole('button', { name: c.copy })).toBeInTheDocument(); + }); +}); diff --git a/modules/api_tokens/tests-js/api_tokens_Index.test.tsx b/modules/api_tokens/tests-js/api_tokens_Index.test.tsx new file mode 100644 index 000000000..21a9fc05c --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_Index.test.tsx @@ -0,0 +1,204 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const reload = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { reload, visit: vi.fn(), get: vi.fn(), post: vi.fn() }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import Index from '../api_tokens/pages/Index'; +import { stubFetch, token } from './fixtures'; + +const SECRET = 'smpy_pat_topsecretvalue'; + +function setProps(over: Record = {}) { + state.props = { + tokens: [token()], + scopes: ['files.read', 'files.write'], + max_lifetime_days: 365, + expiry_presets: [7, 30, 90], + csrf_token: 'csrf-abc', + api_url: '/api/api_tokens', + ...over, + }; +} + +const f = keys.api_tokens.form; +const rv = keys.api_tokens.revoke; +const ra = keys.api_tokens.revoke_all; + +async function createToken() { + fireEvent.click(screen.getByRole('button', { name: f.new_token })); + fireEvent.change(await screen.findByLabelText(f.name), { target: { value: 'deploy' } }); + fireEvent.click(screen.getByLabelText('files.read')); + fireEvent.click(screen.getByRole('button', { name: f.submit })); +} + +describe('api_tokens Index', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); + }); + + it('lists the tokens by name and prefix, never a secret', () => { + render(); + expect(screen.getByText('ci-token')).toBeInTheDocument(); + expect(screen.getByText('smpy_ab12…')).toBeInTheDocument(); + expect(screen.getByText(keys.api_tokens.status.active)).toBeInTheDocument(); + }); + + it('shows the empty state and disables Revoke all with no tokens', () => { + setProps({ tokens: [] }); + render(); + expect(screen.getByText(keys.api_tokens.list.empty_title)).toBeInTheDocument(); + expect(screen.getByRole('button', { name: ra.button })).toBeDisabled(); + }); + + it('disables New token when the caller holds no scopes', () => { + setProps({ scopes: [] }); + render(); + expect(screen.getByRole('button', { name: f.new_token })).toBeDisabled(); + }); + + it('offers no Revoke action on revoked or expired rows', () => { + setProps({ + tokens: [ + token({ id: 'r', name: 'old', revoked_at: '2026-02-01T00:00:00Z' }), + token({ id: 'e', name: 'lapsed', expires_at: '2020-01-01T00:00:00Z' }), + ], + }); + render(); + expect(screen.queryByRole('button', { name: keys.api_tokens.list.revoke })).toBeNull(); + expect(screen.getByRole('button', { name: ra.button })).toBeDisabled(); + }); + + it('creates a token, posting JSON with the CSRF header, and reveals the secret once', async () => { + const fetchMock = stubFetch(201, { token: SECRET, item: token({ id: 't2' }) }); + render(); + await createToken(); + + expect(await screen.findByText(SECRET)).toBeInTheDocument(); + const [url, init] = fetchMock.mock.calls[0] as unknown as [string, RequestInit]; + expect(url).toBe('/api/api_tokens/tokens/'); + expect(init.method).toBe('POST'); + expect((init.headers as Record)['X-CSRF-Token']).toBe('csrf-abc'); + expect(JSON.parse(init.body as string)).toMatchObject({ + name: 'deploy', + scopes: ['files.read'], + expires_in_days: 30, + }); + expect(reload).toHaveBeenCalledWith({ only: ['tokens'] }); + }); + + it('removes the secret once dismissed and never renders it after a reload', async () => { + stubFetch(201, { token: SECRET, item: token({ id: 't2' }) }); + const { rerender } = render(); + await createToken(); + await screen.findByText(SECRET); + + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.created.done })); + await waitFor(() => expect(screen.queryByText(SECRET)).toBeNull()); + + // The reloaded `tokens` prop carries only the prefix. + setProps({ tokens: [token(), token({ id: 't2', name: 'deploy', prefix: 'smpy_cd34' })] }); + rerender(); + expect(screen.getByText('smpy_cd34…')).toBeInTheDocument(); + expect(document.body.textContent).not.toContain(SECRET); + }); + + it('does not bring the secret back after navigating away and returning', async () => { + stubFetch(201, { token: SECRET, item: token({ id: 't2' }) }); + const first = render(); + await createToken(); + await screen.findByText(SECRET); + first.unmount(); + + render(); + expect(document.body.textContent).not.toContain(SECRET); + expect(screen.queryByText(keys.api_tokens.created.title)).toBeNull(); + }); + + it('toasts the server detail and shows no secret when creation is refused', async () => { + stubFetch(400, { detail: 'Name already in use' }); + render(); + await createToken(); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Name already in use')); + expect(screen.queryByText(keys.api_tokens.created.title)).toBeNull(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('falls back to the generic toast when the network fails', async () => { + vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('offline'))); + render(); + await createToken(); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(keys.api_tokens.toasts.failed)); + }); + + it('asks for confirmation before revoking and calls nothing when cancelled', async () => { + const fetchMock = stubFetch(200, {}); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + expect(await screen.findByText(rv.description)).toBeInTheDocument(); + expect(fetchMock).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole('button', { name: rv.cancel })); + await waitFor(() => expect(screen.queryByText(rv.description)).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + expect(toast.success).not.toHaveBeenCalled(); + }); + + it('revokes after confirmation, toasts and reloads the list', async () => { + const fetchMock = stubFetch(200, token({ revoked_at: '2026-03-01T00:00:00Z' })); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: rv.confirm })); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith(keys.api_tokens.toasts.revoked)); + expect(fetchMock.mock.calls[0][0]).toBe('/api/api_tokens/tokens/t1/revoke'); + expect(reload).toHaveBeenCalledWith({ only: ['tokens'] }); + }); + + it('toasts an error and does not reload when revoke fails', async () => { + stubFetch(403, { detail: 'Not yours' }); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.api_tokens.list.revoke })); + fireEvent.click(await screen.findByRole('button', { name: rv.confirm })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Not yours')); + expect(toast.success).not.toHaveBeenCalled(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('revoke-all confirms first, then posts to the bulk endpoint', async () => { + const fetchMock = stubFetch(200, { revoked: 3 }); + render(); + fireEvent.click(screen.getByRole('button', { name: ra.button })); + expect(await screen.findByText(ra.description)).toBeInTheDocument(); + expect(fetchMock).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole('button', { name: ra.confirm })); + await waitFor(() => expect(toast.success).toHaveBeenCalled()); + expect(fetchMock.mock.calls[0][0]).toBe('/api/api_tokens/tokens/revoke-all'); + }); + + it('revoke-all does nothing when cancelled', async () => { + const fetchMock = stubFetch(200, { revoked: 3 }); + render(); + fireEvent.click(screen.getByRole('button', { name: ra.button })); + fireEvent.click(await screen.findByRole('button', { name: ra.cancel })); + await waitFor(() => expect(screen.queryByText(ra.description)).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + }); +}); diff --git a/modules/api_tokens/tests-js/fixtures.ts b/modules/api_tokens/tests-js/fixtures.ts new file mode 100644 index 000000000..1428b526f --- /dev/null +++ b/modules/api_tokens/tests-js/fixtures.ts @@ -0,0 +1,27 @@ +import { vi } from 'vitest'; +import type { ApiToken } from '../api_tokens/pages/types'; + +export function token(over: Partial = {}): ApiToken { + return { + id: 't1', + kind: 'personal', + name: 'ci-token', + prefix: 'smpy_ab12', + scopes: ['files.read'], + roles: [], + owner_user_id: 'u1', + owner_email: 'a@example.com', + tenant_id: null, + created_at: '2026-01-01T00:00:00Z', + expires_at: '2999-01-01T00:00:00Z', + last_used_at: null, + revoked_at: null, + ...over, + }; +} + +export function stubFetch(status: number, body: unknown) { + const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} diff --git a/modules/background_tasks/background_tasks/settings.py b/modules/background_tasks/background_tasks/settings.py index 98fac859f..a015e7aae 100644 --- a/modules/background_tasks/background_tasks/settings.py +++ b/modules/background_tasks/background_tasks/settings.py @@ -278,10 +278,17 @@ def _handler_timeout_fits_the_lease( return timeout ceiling = max_outbox_handler_timeout(lease) if timeout > ceiling: - raise ValueError( - f"outbox_handler_timeout_seconds ({timeout}) must leave a safety margin " - f"inside outbox_lease_seconds ({lease}): at most " - f"{ceiling:g}. Raise the lease or lower the timeout." + raise PydanticCustomError( + "handler_timeout_exceeds_lease", + "outbox_handler_timeout_seconds ({timeout}) must leave a safety margin " + "inside outbox_lease_seconds ({lease}): at most " + "{ceiling}. Raise the lease or lower the timeout.", + { + "timeout": timeout, + "lease": lease, + "ceiling": f"{ceiling:g}", + RELATED_FIELDS_CTX: ["outbox_lease_seconds"], + }, ) return timeout diff --git a/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx new file mode 100644 index 000000000..63609a313 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx @@ -0,0 +1,138 @@ +import '@testing-library/jest-dom/vitest'; +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const visit = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { reload: vi.fn(), get: vi.fn(), visit }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import type { TaskDetail } from '../background_tasks/pages/constants'; +import Detail from '../background_tasks/pages/Detail'; +import { stubFetch } from './stub-fetch'; + +const rd = keys.background_tasks.retry_dialog; + +function execution(over: Partial = {}): TaskDetail { + return { + id: 'e1', + celery_task_id: 'c1', + task_name: 'demo.task', + status: 'failed', + queue: 'default', + tenant_id: null, + args: [1, 'two'], + kwargs: { flag: true }, + result: null, + traceback: 'Traceback (most recent call last)', + exception_type: 'ValueError', + worker: 'w1', + retries: 2, + retried_from_id: null, + queued_at: '2026-01-01T00:00:00Z', + started_at: '2026-01-01T00:00:01Z', + finished_at: '2026-01-01T00:00:02Z', + heartbeat_at: null, + ...over, + }; +} + +function setProps(over: Partial = {}, permissions = ['background_tasks.manage']) { + state.props = { execution: execution(over), max_retries: 3, auth: { permissions } }; +} + +describe('background_tasks Detail page', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); + }); + + it('renders the task and its traceback', () => { + render(); + expect(screen.getAllByText('demo.task').length).toBeGreaterThan(0); + expect(screen.getByText('Traceback (most recent call last)')).toBeInTheDocument(); + }); + + it('offers no retry to a viewer without the manage permission', () => { + setProps({}, []); + render(); + expect( + screen.queryByRole('button', { name: keys.background_tasks.detail.retry_button }), + ).toBeNull(); + }); + + it('offers no retry for a task that already succeeded', () => { + setProps({ status: 'success' }); + render(); + expect( + screen.queryByRole('button', { name: keys.background_tasks.detail.retry_button }), + ).toBeNull(); + }); + + it('confirms with the payload and retry count before re-enqueueing', async () => { + const fetchMock = stubFetch(200, { id: 'e2' }); + render(); + fireEvent.click( + screen.getAllByRole('button', { name: keys.background_tasks.detail.retry_button })[0], + ); + + expect(await screen.findByText(rd.args_label, { exact: false })).toBeInTheDocument(); + expect(screen.getByText(rd.kwargs_label, { exact: false })).toBeInTheDocument(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('does not call the API when the confirm is cancelled', async () => { + const fetchMock = stubFetch(200, { id: 'e2' }); + render(); + fireEvent.click( + screen.getAllByRole('button', { name: keys.background_tasks.detail.retry_button })[0], + ); + fireEvent.click(await screen.findByRole('button', { name: rd.cancel })); + + await waitFor(() => expect(screen.queryByText(rd.description, { exact: false })).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + expect(visit).not.toHaveBeenCalled(); + }); + + it('retries on confirm, toasts and navigates to the new execution', async () => { + const fetchMock = stubFetch(200, { id: 'e2' }); + render(); + fireEvent.click( + screen.getAllByRole('button', { name: keys.background_tasks.detail.retry_button })[0], + ); + fireEvent.click(await screen.findByRole('button', { name: rd.confirm })); + + await waitFor(() => expect(visit).toHaveBeenCalledWith('/admin/background-tasks/e2')); + expect(fetchMock).toHaveBeenCalledWith( + '/api/background_tasks/admin/executions/e1/retry', + expect.objectContaining({ method: 'POST' }), + ); + expect(toast.success).toHaveBeenCalledTimes(1); + }); + + it('shows the server message and stays put when the retry fails', async () => { + stubFetch(409, { detail: 'Already retried' }); + render(); + fireEvent.click( + screen.getAllByRole('button', { name: keys.background_tasks.detail.retry_button })[0], + ); + fireEvent.click(await screen.findByRole('button', { name: rd.confirm })); + + await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Already retried')); + expect(visit).not.toHaveBeenCalled(); + expect(toast.success).not.toHaveBeenCalled(); + }); +}); diff --git a/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx new file mode 100644 index 000000000..34ae50357 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx @@ -0,0 +1,179 @@ +import '@testing-library/jest-dom/vitest'; +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const reload = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { reload, get: vi.fn(), visit: vi.fn() }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import Index from '../background_tasks/pages/Index'; +import type { Execution } from '../background_tasks/pages/retry'; +import { stubFetch } from './stub-fetch'; + +const rd = keys.background_tasks.retry_dialog; +const ra = keys.background_tasks.retry_all_dialog; +const RELOAD = { only: ['executions', 'pagination', 'status_counts', 'queues'] }; + +function execution(over: Partial = {}): Execution { + return { + id: 'e1', + celery_task_id: 'c1', + task_name: 'demo.task', + status: 'failed', + queue: 'default', + tenant_id: null, + args: [], + kwargs: {}, + retries: 0, + worker: null, + queued_at: '2026-01-01T00:00:00Z', + started_at: null, + finished_at: null, + exception_type: 'ValueError', + retried_from_id: null, + ...over, + }; +} + +function setProps(over: Record = {}, permissions = ['background_tasks.manage']) { + state.props = { + executions: [execution()], + pagination: { page: 1, per_page: 20, total: 1 }, + filters: { status: '', task_name: '', queue: '', tenant: '' }, + status_counts: { failed: 1 }, + queues: ['default'], + tenant_ids: [], + platform_tenant_value: '__platform__', + worker_presence: null, + has_executions: true, + auth: { permissions }, + ...over, + }; +} + +describe('background_tasks Index page', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.unstubAllGlobals(); + vi.clearAllMocks(); + }); + + it('lists executions and offers retry controls to managers only', () => { + render(); + expect(screen.getByText('demo.task')).toBeInTheDocument(); + expect( + screen.getByRole('button', { name: keys.background_tasks.table.retry }), + ).toBeInTheDocument(); + cleanup(); + + setProps({}, []); + render(); + expect(screen.queryByRole('button', { name: keys.background_tasks.table.retry })).toBeNull(); + expect( + screen.queryByRole('button', { name: keys.background_tasks.index.retry_all_button }), + ).toBeNull(); + }); + + it('retries one row only after the confirm, then reloads the list and strip', async () => { + const fetchMock = stubFetch(200, { id: 'e2' }); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.background_tasks.table.retry })); + expect(await screen.findByText(rd.no_args)).toBeInTheDocument(); + expect(fetchMock).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole('button', { name: rd.confirm })); + await waitFor(() => expect(reload).toHaveBeenCalledWith(RELOAD)); + expect(fetchMock).toHaveBeenCalledWith( + '/api/background_tasks/admin/executions/e1/retry', + expect.objectContaining({ method: 'POST' }), + ); + expect(toast.success).toHaveBeenCalledTimes(1); + }); + + it('does nothing when the single retry is cancelled', async () => { + const fetchMock = stubFetch(200, { id: 'e2' }); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.background_tasks.table.retry })); + fireEvent.click(await screen.findByRole('button', { name: rd.cancel })); + await waitFor(() => expect(screen.queryByText(rd.no_args)).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('toasts the failure and skips the reload when a single retry errors', async () => { + stubFetch(500, { detail: 'Broker down' }); + render(); + fireEvent.click(screen.getByRole('button', { name: keys.background_tasks.table.retry })); + fireEvent.click(await screen.findByRole('button', { name: rd.confirm })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Broker down')); + expect(reload).not.toHaveBeenCalled(); + }); + + it('retry-all states its scope, then posts with the active filters', async () => { + setProps({ filters: { status: 'failed', task_name: 'demo', queue: 'default', tenant: '' } }); + const fetchMock = stubFetch(200, { queued: 4, remaining: 0 }); + render(); + fireEvent.click( + screen.getByRole('button', { name: keys.background_tasks.index.retry_all_button }), + ); + expect(await screen.findByText(ra.description)).toBeInTheDocument(); + expect(fetchMock).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByRole('button', { name: ra.confirm })); + await waitFor(() => expect(reload).toHaveBeenCalledWith(RELOAD)); + expect(fetchMock).toHaveBeenCalledWith( + '/api/background_tasks/admin/executions/retry-failed?status=failed&q=demo&queue=default', + expect.objectContaining({ method: 'POST' }), + ); + expect(toast.success).toHaveBeenCalledTimes(1); + }); + + it('retry-all mentions the backlog when the server capped the pass', async () => { + stubFetch(200, { queued: 2, remaining: 7 }); + render(); + fireEvent.click( + screen.getByRole('button', { name: keys.background_tasks.index.retry_all_button }), + ); + fireEvent.click(await screen.findByRole('button', { name: ra.confirm })); + await waitFor(() => expect(toast.success).toHaveBeenCalled()); + expect(toast.success).toHaveBeenCalledWith( + keys.background_tasks.toasts.bulk_retried, + expect.objectContaining({ description: keys.background_tasks.toasts.bulk_remaining }), + ); + }); + + it('retry-all does not call the API when cancelled', async () => { + const fetchMock = stubFetch(200, { queued: 1, remaining: 0 }); + render(); + fireEvent.click( + screen.getByRole('button', { name: keys.background_tasks.index.retry_all_button }), + ); + fireEvent.click(await screen.findByRole('button', { name: ra.cancel })); + await waitFor(() => expect(screen.queryByText(ra.description)).toBeNull()); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('retry-all toasts an error and does not reload on failure', async () => { + stubFetch(403, { detail: 'Forbidden' }); + render(); + fireEvent.click( + screen.getByRole('button', { name: keys.background_tasks.index.retry_all_button }), + ); + fireEvent.click(await screen.findByRole('button', { name: ra.confirm })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith('Forbidden')); + expect(reload).not.toHaveBeenCalled(); + }); +}); diff --git a/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx new file mode 100644 index 000000000..b719a2011 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx @@ -0,0 +1,174 @@ +import '@testing-library/jest-dom/vitest'; +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const reload = vi.hoisted(() => vi.fn()); +const get = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { reload, get, visit: vi.fn() }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import Outbox from '../background_tasks/pages/Outbox'; +import type { OutboxMessage } from '../background_tasks/pages/outbox'; +import { stubFetch } from './stub-fetch'; + +const ob = keys.background_tasks.outbox; + +function message(over: Partial = {}): OutboxMessage { + return { + id: 'm1', + event_key: 'orders.contracts.events.OrderPlaced', + handler_name: 'billing.handlers.charge', + payload: {}, + tenant_id: null, + status: 'dead', + attempts: 5, + next_attempt_at: '2026-01-01T00:00:00Z', + locked_until: null, + last_error: 'Boom\nTraceback...', + created_at: '2026-01-01T00:00:00Z', + delivered_at: null, + ...over, + }; +} + +function setProps(over: Record = {}, permissions = ['background_tasks.manage']) { + state.props = { + messages: [message()], + counts: { pending: 2, running: 0, dead: 1 }, + pagination: { page: 1, per_page: 20, total: 1 }, + filters: { status: '' }, + auth: { permissions }, + ...over, + }; +} + +describe('background_tasks Outbox page', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + vi.clearAllMocks(); + }); + + it('lists messages with the short event name and first error line', () => { + render(); + expect(screen.getByText('OrderPlaced')).toBeInTheDocument(); + expect(screen.getByText('billing.handlers.charge')).toBeInTheDocument(); + expect(screen.getByText('Boom')).toBeInTheDocument(); + }); + + it('shows the empty state', () => { + setProps({ messages: [], pagination: { page: 1, per_page: 20, total: 0 } }); + render(); + expect(screen.getByText(ob.empty_title)).toBeInTheDocument(); + }); + + it('hides row actions without the manage permission', () => { + setProps({}, []); + render(); + expect(screen.queryByRole('button', { name: ob.retry })).toBeNull(); + expect(screen.queryByRole('button', { name: ob.discard })).toBeNull(); + }); + + it('offers no action on a running message and no retry on a pending one', () => { + setProps({ + messages: [message({ id: 'a', status: 'running' }), message({ id: 'b', status: 'pending' })], + }); + render(); + expect(screen.queryByRole('button', { name: ob.retry })).toBeNull(); + expect(screen.getAllByRole('button', { name: ob.discard })).toHaveLength(1); + }); + + it('replays a dead message with a POST, toasts and reloads the list', async () => { + const fetchMock = stubFetch(200, { id: 'm1', status: 'pending' }); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.retry })); + + await waitFor(() => expect(reload).toHaveBeenCalled()); + expect(fetchMock).toHaveBeenCalledWith( + '/api/background_tasks/admin/outbox/m1/retry', + expect.objectContaining({ method: 'POST' }), + ); + expect(toast.success).toHaveBeenCalledWith(ob.retried); + expect(reload).toHaveBeenCalledWith({ only: ['messages', 'counts', 'pagination'] }); + }); + + it('shows the translated conflict and still reloads when replay is refused', async () => { + stubFetch(409, { detail: 'English server text' }); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.retry })); + + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(ob.conflict)); + expect(toast.success).not.toHaveBeenCalled(); + expect(reload).toHaveBeenCalled(); + }); + + it('reports a vanished row on 404 and a generic failure otherwise', async () => { + stubFetch(404); + const { unmount } = render(); + fireEvent.click(screen.getByRole('button', { name: ob.retry })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(ob.gone)); + unmount(); + + stubFetch(500); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.retry })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(ob.retry_failed)); + }); + + it('asks before discarding and calls nothing when the operator declines', () => { + const fetchMock = stubFetch(204); + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.discard })); + + expect(confirmSpy).toHaveBeenCalledTimes(1); + expect(fetchMock).not.toHaveBeenCalled(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('discards with a DELETE once confirmed', async () => { + const fetchMock = stubFetch(204); + vi.spyOn(window, 'confirm').mockReturnValue(true); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.discard })); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith(ob.discarded)); + expect(fetchMock).toHaveBeenCalledWith( + '/api/background_tasks/admin/outbox/m1', + expect.objectContaining({ method: 'DELETE' }), + ); + expect(reload).toHaveBeenCalled(); + }); + + it('toasts a discard failure', async () => { + stubFetch(500); + vi.spyOn(window, 'confirm').mockReturnValue(true); + render(); + fireEvent.click(screen.getByRole('button', { name: ob.discard })); + await waitFor(() => expect(toast.error).toHaveBeenCalledWith(ob.discard_failed)); + }); + + it('filters by status through a page-1 visit', () => { + render(); + fireEvent.click(screen.getByRole('button', { name: new RegExp(ob.status_dead) })); + expect(get).toHaveBeenCalledWith( + '/admin/background-tasks/outbox', + { status: 'dead' }, + expect.objectContaining({ preserveState: true }), + ); + }); +}); diff --git a/modules/background_tasks/tests-js/outbox.test.ts b/modules/background_tasks/tests-js/outbox.test.ts index e56f726f1..f27369e9d 100644 --- a/modules/background_tasks/tests-js/outbox.test.ts +++ b/modules/background_tasks/tests-js/outbox.test.ts @@ -8,14 +8,7 @@ import { retryOutboxMessage, shortName, } from '../background_tasks/pages/outbox'; - -function stubFetch(status: number, body: unknown = {}) { - const fetchMock = vi.fn( - async () => new Response(status === 204 ? null : JSON.stringify(body), { status }), - ); - vi.stubGlobal('fetch', fetchMock); - return fetchMock; -} +import { stubFetch } from './stub-fetch'; describe('outbox actions', () => { afterEach(() => { diff --git a/modules/background_tasks/tests-js/stub-fetch.ts b/modules/background_tasks/tests-js/stub-fetch.ts new file mode 100644 index 000000000..c60911756 --- /dev/null +++ b/modules/background_tasks/tests-js/stub-fetch.ts @@ -0,0 +1,10 @@ +import { vi } from 'vitest'; + +// A 204 must carry no body: `new Response('{}', { status: 204 })` throws. +export function stubFetch(status: number, body: unknown = {}) { + const fetchMock = vi.fn( + async () => new Response(status === 204 ? null : JSON.stringify(body), { status }), + ); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} diff --git a/modules/background_tasks/tests/test_bg_invalidation.py b/modules/background_tasks/tests/test_bg_invalidation.py index 65942fa42..8ef48c3a6 100644 --- a/modules/background_tasks/tests/test_bg_invalidation.py +++ b/modules/background_tasks/tests/test_bg_invalidation.py @@ -48,7 +48,7 @@ async def get_message(self, ignore_subscribe_messages: bool = False, timeout=Non """ if self._frames: return self._frames.pop(0) - await asyncio.sleep(timeout if timeout else 0) + await asyncio.sleep(timeout or 0) # intentional-sleep: fake idle-channel poll interval return None async def aclose(self) -> None: diff --git a/modules/background_tasks/tests/test_bg_invalidation_degraded.py b/modules/background_tasks/tests/test_bg_invalidation_degraded.py index 84eebb9b6..c882906a3 100644 --- a/modules/background_tasks/tests/test_bg_invalidation_degraded.py +++ b/modules/background_tasks/tests/test_bg_invalidation_degraded.py @@ -20,6 +20,7 @@ from background_tasks.invalidation import RedisInvalidationTransport from simple_module_core.invalidation import Invalidation, InvalidationBus +from simple_module_test import wait_for _CHANNEL = "qa.invalidation" @@ -72,16 +73,19 @@ async def hold(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> No ) await transport.start() try: - deadline = asyncio.get_running_loop().time() + 15 - while asyncio.get_running_loop().time() < deadline: - if any("keep cached values until they expire" in r.message for r in caplog.records): - break - await asyncio.sleep(0.05) - else: + try: + await wait_for( + lambda: any( + "keep cached values until they expire" in r.message for r in caplog.records + ), + timeout=15, + interval=0.05, + ) + except AssertionError as exc: raise AssertionError( "the listener never complained about a mute broker — it is wedged " "in an unbounded read and will never reconnect" - ) + ) from exc finally: await transport.stop() for writer in writers: @@ -111,7 +115,7 @@ async def hold(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> No InvalidationBus(), f"redis://127.0.0.1:{port}/0", _CHANNEL ) await transport.start() - await asyncio.sleep(0.2) + await wait_for(lambda: bool(writers)) # the listener is connected and mid-read try: await asyncio.wait_for(transport.stop(), timeout=20) finally: diff --git a/modules/background_tasks/tests/test_bg_invalidation_redis.py b/modules/background_tasks/tests/test_bg_invalidation_redis.py index db0d6286d..ee69b7513 100644 --- a/modules/background_tasks/tests/test_bg_invalidation_redis.py +++ b/modules/background_tasks/tests/test_bg_invalidation_redis.py @@ -22,6 +22,7 @@ import pytest from background_tasks.invalidation import RedisInvalidationTransport from simple_module_core.invalidation import Invalidation, InvalidationBus +from simple_module_test import wait_for # Per process: under pytest-xdist every worker shares one Redis (CI points them # all at a single service container), and pub/sub has no isolation but the @@ -38,11 +39,7 @@ async def _await_delivery(seen: list[Invalidation], count: int = 1) -> None: sub-millisecond in practice but a loaded CI runner is not, and a sleep long enough to be safe there would be a second added to every run. """ - deadline = asyncio.get_running_loop().time() + _DELIVERY_TIMEOUT_SECONDS - while len(seen) < count: - if asyncio.get_running_loop().time() > deadline: - raise AssertionError(f"delivered {len(seen)} of {count} messages before timing out") - await asyncio.sleep(0.01) + await wait_for(lambda: len(seen) >= count, timeout=_DELIVERY_TIMEOUT_SECONDS) async def _subscribed(bus: InvalidationBus, url: str, channel: str = _CHANNEL): @@ -62,15 +59,17 @@ async def _subscribed(bus: InvalidationBus, url: str, channel: str = _CHANNEL): probe = aioredis.from_url(url) try: - deadline = asyncio.get_running_loop().time() + _DELIVERY_TIMEOUT_SECONDS - while True: + + async def subscribed() -> bool: counts = await probe.pubsub_numsub(channel) - if counts and counts[0][1] >= 1: - return transport - if asyncio.get_running_loop().time() > deadline: - await transport.stop() - raise AssertionError(f"listener never subscribed to {channel!r}") - await asyncio.sleep(0.01) + return bool(counts and counts[0][1] >= 1) + + try: + await wait_for(subscribed, timeout=_DELIVERY_TIMEOUT_SECONDS) + except AssertionError: + await transport.stop() + raise + return transport finally: await probe.aclose() @@ -118,9 +117,7 @@ async def capture(message): client = aioredis.from_url(redis_server) try: await client.publish(_CHANNEL, Invalidation(channel=_CHANNEL, key="k").to_wire()) - deadline = asyncio.get_running_loop().time() + _DELIVERY_TIMEOUT_SECONDS - while not raw and asyncio.get_running_loop().time() < deadline: - await asyncio.sleep(0.01) + await wait_for(lambda: bool(raw), timeout=_DELIVERY_TIMEOUT_SECONDS) finally: await client.aclose() await listener.stop() @@ -147,14 +144,27 @@ async def test_the_publisher_does_not_reapply_its_own_broadcast(self, redis_serv bus.set_transport(transport) try: await bus.publish(_CHANNEL, key="once") - # Give the echo time to arrive and be rejected. There is no - # positive signal for "a message was correctly ignored", so this - # waits out the delivery window instead of polling for one. - await asyncio.sleep(0.3) + # There is no positive signal for "a message was correctly + # ignored", so follow it with a sentinel from another origin. + # Pub/sub preserves order, so once the sentinel is applied the echo + # ahead of it has been seen and rejected. + import redis.asyncio as aioredis + + client = aioredis.from_url(redis_server) + try: + wire = Invalidation(channel=_CHANNEL, key="sentinel", origin="elsewhere").to_wire() + await client.publish(_CHANNEL, wire) + finally: + await client.aclose() + await wait_for( + lambda: any(i.key == "sentinel" for i in seen), timeout=_DELIVERY_TIMEOUT_SECONDS + ) finally: await transport.stop() - assert len(seen) == 1, "the publisher applied its own echo a second time" + assert [i.key for i in seen] == ["once", "sentinel"], ( + "the publisher applied its own echo a second time" + ) async def test_a_second_worker_does_apply_it(self, redis_server): """The same message, to a bus that did not publish it — must apply.""" @@ -199,12 +209,16 @@ async def test_only_the_configured_channel_is_subscribed(self, redis_server): try: wire = Invalidation(channel=_CHANNEL, key="k", origin="elsewhere").to_wire() await client.publish("app-two.invalidation", wire) - await asyncio.sleep(0.3) + # Ordered after the foreign-channel message: once the sentinel on + # our own channel lands, the other app's message would have too. + sentinel = Invalidation(channel=_CHANNEL, key="sentinel", origin="elsewhere").to_wire() + await client.publish("app-one.invalidation", sentinel) + await wait_for(lambda: bool(seen), timeout=_DELIVERY_TIMEOUT_SECONDS) finally: await client.aclose() await transport.stop() - assert seen == [] + assert [i.key for i in seen] == ["sentinel"] class TestTeardown: @@ -224,14 +238,12 @@ async def test_stop_releases_the_subscription_on_the_server(self, redis_server): probe = aioredis.from_url(redis_server) try: - deadline = asyncio.get_running_loop().time() + _DELIVERY_TIMEOUT_SECONDS - while True: + + async def released() -> bool: counts = await probe.pubsub_numsub(_CHANNEL) - if not counts or counts[0][1] == 0: - break - if asyncio.get_running_loop().time() > deadline: - raise AssertionError("the subscription outlived stop()") - await asyncio.sleep(0.05) + return not counts or counts[0][1] == 0 + + await wait_for(released, timeout=_DELIVERY_TIMEOUT_SECONDS) finally: await probe.aclose() diff --git a/modules/background_tasks/tests/test_bg_outbox_eager_platform.py b/modules/background_tasks/tests/test_bg_outbox_eager_platform.py index c6364be16..87343a4dd 100644 --- a/modules/background_tasks/tests/test_bg_outbox_eager_platform.py +++ b/modules/background_tasks/tests/test_bg_outbox_eager_platform.py @@ -22,6 +22,7 @@ from simple_module_db import all_tenants, current_tenant_id, finalize_session, tenant_context from simple_module_db.tenancy import is_all_tenants from simple_module_db.transaction import SESSION_START_KEY +from simple_module_test import wait_for pytestmark = pytest.mark.usefixtures("_stub_celery") @@ -31,15 +32,6 @@ class EagerPing(Event): n: int -async def _wait_for(predicate, timeout: float = 3.0) -> bool: - deadline = time.monotonic() + timeout - while time.monotonic() < deadline: - if await predicate(): - return True - await asyncio.sleep(0.02) - return False - - async def test_eager_publish_from_a_plain_session_is_delivered(app, outbox_rows): """A CLI script or startup hook publishing in single-process dev.""" app.state.background_tasks.settings.task_always_eager = True @@ -57,7 +49,7 @@ async def delivered() -> bool: rows = await outbox_rows() return bool(rows) and all(r.status == OutboxStatus.DELIVERED for r in rows) - assert await _wait_for(delivered) + await wait_for(delivered, timeout=3.0) assert seen == [7] app.state.background_tasks.celery.send_task.assert_not_called() @@ -72,7 +64,8 @@ async def handler(event: EagerPing, ctx: DurableContext) -> None: ... await app.state.sm.event_bus.publish_durable(EagerPing(8), session) await session.rollback() - await asyncio.sleep(0.1) + # Negative assertion: nothing was scheduled, so there is no event to wait for. + await asyncio.sleep(0.1) # intentional-sleep: window for a wrongly scheduled delivery assert await outbox_rows() == [] diff --git a/modules/background_tasks/tests/test_bg_outbox_loop_interrupt.py b/modules/background_tasks/tests/test_bg_outbox_loop_interrupt.py index d01659ac0..9bc4e9a85 100644 --- a/modules/background_tasks/tests/test_bg_outbox_loop_interrupt.py +++ b/modules/background_tasks/tests/test_bg_outbox_loop_interrupt.py @@ -21,7 +21,7 @@ def test_interrupt_cancels_the_coroutine_on_the_loop(): async def slow() -> None: started.set() try: - await asyncio.sleep(30) + await asyncio.sleep(30) # intentional-sleep: blocks until the interrupt cancels it except asyncio.CancelledError: cancelled.set() raise diff --git a/modules/background_tasks/tests/test_bg_outbox_retry_policy.py b/modules/background_tasks/tests/test_bg_outbox_retry_policy.py index 5ab4340ab..e29d07e18 100644 --- a/modules/background_tasks/tests/test_bg_outbox_retry_policy.py +++ b/modules/background_tasks/tests/test_bg_outbox_retry_policy.py @@ -108,7 +108,7 @@ def test_timeout_leaves_a_margin_inside_the_lease(self): async def test_a_hung_handler_is_a_failed_attempt(self, app, outbox_rows): async def hangs(event: PolicyPing, ctx: DurableContext) -> None: - await asyncio.sleep(30) + await asyncio.sleep(30) # intentional-sleep: hangs until the handler timeout cancels it app.state.sm.event_bus.subscribe_durable(PolicyPing, hangs, name="hangs") await _publish(app, PolicyPing(1)) @@ -132,7 +132,9 @@ def test_settings_refuse_a_timeout_that_outlives_the_lease(self): async def test_backoff_is_measured_from_the_failure(app, outbox_rows): async def slow_fail(event: PolicyPing, ctx: DurableContext) -> None: - await asyncio.sleep(0.6) + await asyncio.sleep( + 0.6 + ) # intentional-sleep: simulated slow handler; backoff is measured after it raise RuntimeError("slow fail") app.state.sm.event_bus.subscribe_durable(PolicyPing, slow_fail, name="slowfail") diff --git a/modules/background_tasks/tests/test_bg_outbox_swallowed_cancel.py b/modules/background_tasks/tests/test_bg_outbox_swallowed_cancel.py index eb4420fa8..42fb51896 100644 --- a/modules/background_tasks/tests/test_bg_outbox_swallowed_cancel.py +++ b/modules/background_tasks/tests/test_bg_outbox_swallowed_cancel.py @@ -34,7 +34,7 @@ class SwallowPing(Event): async def test_a_swallowed_cancellation_is_a_timed_out_attempt(app, outbox_rows, effect_names): async def handler(event: SwallowPing, ctx: DurableContext) -> None: with contextlib.suppress(asyncio.CancelledError): # wrong, but it happens - await asyncio.sleep(5) + await asyncio.sleep(5) # intentional-sleep: hangs until the handler timeout cancels it ctx.db.add(TaskExecution(task_name="effect:late", status=TaskStatus.SUCCESS)) app.state.sm.event_bus.subscribe_durable(SwallowPing, handler, name="swallow") diff --git a/modules/background_tasks/tests/test_bg_outbox_thread_loops.py b/modules/background_tasks/tests/test_bg_outbox_thread_loops.py index b1959efaa..9ff2a4d18 100644 --- a/modules/background_tasks/tests/test_bg_outbox_thread_loops.py +++ b/modules/background_tasks/tests/test_bg_outbox_thread_loops.py @@ -38,7 +38,7 @@ def test_threads_get_their_own_loops_and_do_not_serialise(): stop_worker_loop() async def blocking() -> int: - time.sleep(0.5) # a handler doing blocking I/O on the loop + time.sleep(0.5) # intentional-sleep: a handler doing blocking I/O on the loop return id(asyncio.get_running_loop()) try: diff --git a/modules/background_tasks/tests/test_outbox_concurrency.py b/modules/background_tasks/tests/test_outbox_concurrency.py index f4610c938..a19b308c2 100644 --- a/modules/background_tasks/tests/test_outbox_concurrency.py +++ b/modules/background_tasks/tests/test_outbox_concurrency.py @@ -25,6 +25,7 @@ from simple_module_core.events import Event, EventBus from simple_module_db import init_db from simple_module_db.listeners import register_listeners +from simple_module_test import wait_for from simple_module_test.database import ( database_url_for_tests, init_db_kwargs, @@ -170,10 +171,12 @@ async def test_a_late_finisher_after_reclaim_is_fenced_off(factory) -> None: """Worker A is slow past its lease; B reclaims and delivers. A's handler effects must not commit: the delivered mark is fenced on A's attempt.""" gate = asyncio.Event() + entered = asyncio.Event() async def handler(event: StockMoved, ctx: DurableContext) -> None: ctx.db.add(TaskExecution(task_name=f"attempt-{ctx.attempt}", status=TaskStatus.SUCCESS)) if ctx.attempt == 1: + entered.set() await gate.wait() bus = EventBus() @@ -182,7 +185,7 @@ async def handler(event: StockMoved, ctx: DurableContext) -> None: t0 = datetime.now(UTC) (claimed_by_a,) = await claim(factory, now=t0, lease_seconds=60) worker_a = asyncio.create_task(deliver_one(factory, bus, claimed_by_a, policy=POLICY)) - await asyncio.sleep(0.05) + await wait_for(entered.is_set) worker_b = await dispatch(factory, bus, POLICY, now=t0 + timedelta(seconds=61)) gate.set() diff --git a/modules/background_tasks/tests/test_worker_inspector.py b/modules/background_tasks/tests/test_worker_inspector.py index 2b784107c..a2744926d 100644 --- a/modules/background_tasks/tests/test_worker_inspector.py +++ b/modules/background_tasks/tests/test_worker_inspector.py @@ -138,10 +138,13 @@ def test_probes_run_concurrently_not_sequentially(): """ from background_tasks.worker_inspector import WorkerInspector + def _slow_probe(*_: object, **__: object) -> None: + time.sleep(PROBE_DELAY) # intentional-sleep: simulated slow probe + celery, _ = _celery_with_live_broker_no_workers() inspect = celery.control.inspect.return_value for name in ("ping", "stats", "active_queues", "active"): - getattr(inspect, name).side_effect = lambda *_, **__: (time.sleep(PROBE_DELAY), None)[1] + getattr(inspect, name).side_effect = _slow_probe start = time.perf_counter() snapshot = WorkerInspector(celery, timeout=PROBE_DELAY).snapshot() diff --git a/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx b/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx new file mode 100644 index 000000000..ce00627ce --- /dev/null +++ b/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx @@ -0,0 +1,194 @@ +import { configureI18n, keys } from '@simple-module-py/i18n'; +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +configureI18n({ locale: 'en', messages: {} }); + +const state = vi.hoisted(() => ({ props: {} as Record })); +const post = vi.hoisted(() => vi.fn()); +const visit = vi.hoisted(() => vi.fn()); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, href }: { children: React.ReactNode; href: string }) => ( + {children} + ), + router: { post, visit }, + usePage: () => ({ props: state.props }), +})); +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); + +import { toast } from 'sonner'; +import Browse from '../feature_flags/pages/Browse'; +import type { FeatureFlag } from '../feature_flags/pages/types'; + +const FLAGS: FeatureFlag[] = [ + { + name: 'beta_search', + description: 'New search', + default_enabled: false, + enabled: false, + overridden: false, + system_enabled: null, + }, + { + name: 'dark_mode', + description: 'Dark theme', + default_enabled: false, + enabled: true, + overridden: true, + system_enabled: null, + }, +]; + +function setProps(over: Record = {}, permissions = ['feature_flags.manage']) { + state.props = { + flags: FLAGS, + tenant_id: null, + tenants: ['acme'], + audit_log_url: null, + auth: { permissions }, + ...over, + }; +} + +type PostOptions = { onSuccess: () => void; onError: () => void; preserveScroll: boolean }; + +const confirm = keys.feature_flags.confirm; +const table = keys.feature_flags.table; +const browse = keys.feature_flags.browse; + +describe('feature_flags Browse', () => { + beforeEach(() => setProps()); + afterEach(() => { + vi.clearAllMocks(); + }); + + it('renders every flag with its description and override badge', () => { + render(); + expect(screen.getByText('beta_search')).toBeInTheDocument(); + expect(screen.getByText('dark_mode')).toBeInTheDocument(); + expect(screen.getByText('Dark theme')).toBeInTheDocument(); + expect(screen.getAllByText(table.overridden)).toHaveLength(1); + }); + + it('shows the empty state when no module registers flags', () => { + setProps({ flags: [] }); + render(); + expect(screen.getByText(browse.empty_title)).toBeInTheDocument(); + }); + + it('links to the audit history only when an audit log is installed', () => { + const { unmount } = render(); + expect(screen.queryByText(browse.view_history)).toBeNull(); + unmount(); + setProps({ audit_log_url: '/admin/audit?x=1' }); + render(); + expect(screen.getByText(browse.view_history)).toHaveAttribute('href', '/admin/audit?x=1'); + }); + + it('makes the switches read-only without the manage permission', () => { + setProps({}, []); + render(); + expect(screen.getByRole('switch', { name: 'beta_search' })).toBeDisabled(); + expect(screen.queryByText(table.clear_override)).toBeNull(); + }); + + it('stages a toggle behind a confirm and writes nothing until confirmed', async () => { + render(); + fireEvent.click(screen.getByRole('switch', { name: 'beta_search' })); + expect(await screen.findByText(confirm.scope_system)).toBeInTheDocument(); + expect(post).not.toHaveBeenCalled(); + }); + + it('cancelling the confirm leaves the flag and the server untouched', async () => { + render(); + fireEvent.click(screen.getByRole('switch', { name: 'beta_search' })); + fireEvent.click(await screen.findByRole('button', { name: confirm.cancel })); + await waitFor(() => expect(screen.queryByText(confirm.scope_system)).toBeNull()); + expect(post).not.toHaveBeenCalled(); + expect(screen.getByRole('switch', { name: 'beta_search' })).not.toBeChecked(); + }); + + it('enables a flag on confirm, posting enabled=true to the system scope', async () => { + render(); + fireEvent.click(screen.getByRole('switch', { name: 'beta_search' })); + fireEvent.click(await screen.findByRole('button', { name: confirm.enable_action })); + + expect(post).toHaveBeenCalledTimes(1); + const [url, data, options] = post.mock.calls[0] as [string, unknown, PostOptions]; + expect(url).toBe('/admin/feature-flags/beta_search/toggle'); + expect(data).toEqual({ enabled: true }); + expect(options.preserveScroll).toBe(true); + options.onSuccess(); + expect(toast.success).toHaveBeenCalledWith(keys.feature_flags.toasts.enabled); + }); + + it('disables a flag on confirm and toasts a failure from the server', async () => { + render(); + fireEvent.click(screen.getByRole('switch', { name: 'dark_mode' })); + fireEvent.click(await screen.findByRole('button', { name: confirm.disable_action })); + + const [, data, options] = post.mock.calls[0] as [string, unknown, PostOptions]; + expect(data).toEqual({ enabled: false }); + options.onSuccess(); + expect(toast.success).toHaveBeenCalledWith(keys.feature_flags.toasts.disabled); + options.onError(); + expect(toast.error).toHaveBeenCalledWith(keys.feature_flags.toasts.toggle_failed); + }); + + it('scopes the toggle URL and the confirm text to the viewed tenant', async () => { + setProps({ tenant_id: 'acme corp' }); + render(); + fireEvent.click(screen.getByRole('switch', { name: 'beta_search' })); + expect(await screen.findByText(confirm.scope_tenant)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: confirm.enable_action })); + expect(post.mock.calls[0][0]).toBe( + '/admin/feature-flags/beta_search/toggle?tenant_id=acme%20corp', + ); + }); + + it('clears an override without a confirm and reports the result', () => { + render(); + fireEvent.click(screen.getAllByText(table.clear_override)[0]); + const [url, data, options] = post.mock.calls[0] as [string, unknown, PostOptions]; + expect(url).toBe('/admin/feature-flags/dark_mode/clear'); + expect(data).toEqual({}); + options.onSuccess(); + expect(toast.success).toHaveBeenCalledWith(keys.feature_flags.toasts.cleared); + options.onError(); + expect(toast.error).toHaveBeenCalledWith(keys.feature_flags.toasts.toggle_failed); + }); + + it('explains which value a non-overridden flag follows', () => { + render(); + expect(screen.getAllByText(table.following_default).length).toBeGreaterThan(0); + cleanup(); + setProps({ tenant_id: 'acme' }); + render(); + expect(screen.getAllByText(table.following_system).length).toBeGreaterThan(0); + }); + + it('switches scope by visiting the tenant URL, or the system URL', () => { + render(); + fireEvent.click(screen.getByRole('radio', { name: 'acme' })); + expect(visit).toHaveBeenCalledWith('/admin/feature-flags/?tenant_id=acme'); + cleanup(); + setProps({ tenant_id: 'acme' }); + render(); + fireEvent.click(screen.getByRole('radio', { name: browse.scope_system })); + expect(visit).toHaveBeenLastCalledWith('/admin/feature-flags/'); + }); + + it('lets an operator name a tenant with no overrides yet', () => { + render(); + fireEvent.click(screen.getByRole('radio', { name: browse.scope_custom })); + const go = screen.getByRole('button', { name: browse.go }); + expect(go).toBeDisabled(); + fireEvent.change(screen.getByLabelText(browse.tenant_id_label), { + target: { value: ' new-co ' }, + }); + fireEvent.click(go); + expect(visit).toHaveBeenCalledWith('/admin/feature-flags/?tenant_id=new-co'); + }); +}); diff --git a/modules/file_storage/file_storage/settings.py b/modules/file_storage/file_storage/settings.py index a8f10c621..fcc4edeff 100644 --- a/modules/file_storage/file_storage/settings.py +++ b/modules/file_storage/file_storage/settings.py @@ -16,6 +16,7 @@ from pydantic import Field, field_validator, model_validator from pydantic_settings import SettingsConfigDict from simple_module_core.settings_base import DbBackedSettings +from simple_module_core.stored_settings import field_error from file_storage import constants @@ -148,7 +149,17 @@ def _validate_backend_config(self) -> FileStorageSettings: if not value ] if missing: - raise ValueError(f"S3 backend selected but {', '.join(missing)} is not set.") + # Located on the first missing field (declared after ``backend``) so + # the form shows it there; ``backend`` is related so lenient boot + # can fall back to the default backend instead of failing. + raise field_error( + type(self), + missing[0], + "s3_config_incomplete", + f"S3 backend selected but {', '.join(missing)} is not set.", + value=getattr(self, missing[0]), + related=["backend", "s3_bucket", "s3_region"], + ) return self def presign_endpoint_url(self) -> str: diff --git a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py index cedb7eaaa..5ff5fd55d 100644 --- a/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py +++ b/modules/file_storage/tests/test_file_storage_thumbnail_hardening.py @@ -10,6 +10,7 @@ import pytest from file_storage import constants, thumbnails from PIL import Image +from simple_module_test import wait_for API = constants.ROUTE_PREFIX_API @@ -183,12 +184,12 @@ def request(): await asyncio.get_running_loop().run_in_executor(None, entered.wait, 5) first.cancel() # the client went away second = request() # retry joins the same decode (single flight) - await asyncio.sleep(0.05) + await asyncio.gather(first, return_exceptions=True) # the cancellation has fully propagated assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY - 1 assert len(calls) == 1 release.set() assert await second == b"webp" - await asyncio.sleep(0.05) + await wait_for(lambda: thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY) assert thumbnails._slots()._value == constants.THUMBNAIL_MAX_CONCURRENCY assert not thumbnails._INFLIGHT diff --git a/modules/file_storage/tests/test_file_storage_upload_guards.py b/modules/file_storage/tests/test_file_storage_upload_guards.py new file mode 100644 index 000000000..5b844a686 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_upload_guards.py @@ -0,0 +1,271 @@ +"""Direct unit tests for ``upload_guards.UploadGuards`` (#454), without the HTTP layer.""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta +from types import SimpleNamespace +from typing import Any + +import pytest +from file_storage import constants +from file_storage import upload_guards as guards_module +from file_storage.contracts.service import StorageError +from file_storage.models import StoredFile, UploadSession +from file_storage.upload_guards import UploadGuards +from file_storage.upload_support import DirectUploadUnsupportedError, QuotaExceededError +from simple_module_db import tenant_context +from sqlalchemy import select + +OWNER = "user-1" +OTHER = "user-2" + + +class _Backend: + backend_id = "fake" + supports_direct_upload = True + + def __init__(self) -> None: + self.aborted: list[tuple[str, str]] = [] + self.deleted: list[str] = [] + self.fail = False + + async def abort_multipart(self, key: str, upload_id: str) -> None: + if self.fail: + raise StorageError("boom") + self.aborted.append((key, upload_id)) + + async def delete(self, key: str) -> None: + if self.fail: + raise StorageError("boom") + self.deleted.append(key) + + +class _Guards(UploadGuards): + def __init__(self, db: Any, backend: Any, **settings: Any) -> None: + self.db = db + self.files = SimpleNamespace(backend=backend) + defaults = {"quota_bytes": None, "upload_session_retention_days": 7} + self.settings = SimpleNamespace(**{**defaults, **settings}) + + +@pytest.fixture +def backend() -> _Backend: + return _Backend() + + +def _session( + *, + status: str = constants.UploadStatus.ACTIVE, + total: int = 100, + expires_in: timedelta = timedelta(hours=1), + owner: str = OWNER, +) -> UploadSession: + return UploadSession( + key=f"t1/{uuid.uuid4()}.bin", + filename="f.bin", + content_type="application/octet-stream", + total_size=total, + part_size=10, + backend_upload_id="up-1", + status=status, + expires_at=datetime.now(UTC) + expires_in, + created_by=owner, + ) + + +def _stored(size: int) -> StoredFile: + return StoredFile( + key=f"t1/{uuid.uuid4()}.bin", + filename="s.bin", + content_type="application/octet-stream", + size_bytes=size, + backend="fake", + ) + + +class TestBackendProperty: + def test_returns_a_direct_upload_capable_backend(self, backend): + assert _Guards(None, backend).backend is backend + + def test_refuses_a_backend_without_direct_upload(self): + plain = SimpleNamespace(backend_id="local") + with pytest.raises(DirectUploadUnsupportedError, match="local"): + _Guards(None, plain).backend # noqa: B018 + + def test_refuses_a_backend_that_declares_it_unsupported(self): + off = SimpleNamespace(backend_id="local", supports_direct_upload=False) + with pytest.raises(DirectUploadUnsupportedError): + _Guards(None, off).backend # noqa: B018 + + +class TestCleanupHelpers: + async def test_abort_backend_forwards_the_upload_id(self, backend): + await _Guards(None, backend)._abort_backend("k", "u") + assert backend.aborted == [("k", "u")] + + async def test_abort_backend_swallows_storage_errors(self, backend, caplog): + backend.fail = True + await _Guards(None, backend)._abort_backend("k", "u") + assert "file_storage.abort_failed" in caplog.text + + async def test_drop_object_deletes_and_swallows_storage_errors(self, backend, caplog): + guards = _Guards(None, backend) + await guards._drop_object("k") + assert backend.deleted == ["k"] + backend.fail = True + await guards._drop_object("k2") + assert "file_storage.cleanup_failed" in caplog.text + + async def test_reject_drops_the_object_and_aborts_the_session(self, backend, db_session): + with tenant_context("t1"): + row = _session() + db_session.add(row) + await db_session.flush() + await _Guards(db_session, backend)._reject(row) + assert backend.deleted == [row.key] + assert row.status == constants.UploadStatus.ABORTED + + async def test_reject_still_aborts_when_the_object_cannot_be_deleted(self, backend, db_session): + backend.fail = True + with tenant_context("t1"): + row = _session() + db_session.add(row) + await db_session.flush() + await _Guards(db_session, backend)._reject(row) + assert row.status == constants.UploadStatus.ABORTED + + +class TestPurgeFinished: + @pytest.mark.parametrize("owner", [None, ""]) + async def test_without_an_owner_nothing_is_deleted(self, backend, db_session, owner): + assert await _Guards(db_session, backend).purge_finished(owner=owner) == 0 + + async def test_deletes_only_this_owners_finished_rows_past_retention(self, backend, db_session): + old = -timedelta(days=30) + with tenant_context("t1"): + doomed = [ + _session(status=constants.UploadStatus.COMPLETED, expires_in=old), + _session(status=constants.UploadStatus.ABORTED, expires_in=old), + ] + kept = [ + _session(expires_in=old), # still ACTIVE, however stale + _session(status=constants.UploadStatus.COMPLETED, expires_in=-timedelta(days=1)), + _session(status=constants.UploadStatus.COMPLETED, expires_in=old, owner=OTHER), + ] + db_session.add_all([*doomed, *kept]) + await db_session.flush() + + removed = await _Guards(db_session, backend).purge_finished(owner=OWNER) + + remaining = set((await db_session.scalars(select(UploadSession.id))).all()) + assert removed == 2 + assert remaining == {row.id for row in kept} + + async def test_zero_day_retention_purges_anything_already_expired(self, backend, db_session): + with tenant_context("t1"): + db_session.add( + _session(status=constants.UploadStatus.COMPLETED, expires_in=-timedelta(seconds=5)) + ) + await db_session.flush() + removed = await _Guards( + db_session, backend, upload_session_retention_days=0 + ).purge_finished(owner=OWNER) + assert removed == 1 + + +class TestCheckQuota: + async def test_no_quota_never_refuses(self, backend, db_session): + with tenant_context("t1"): + await _Guards(db_session, backend)._check_quota(10**15, reserve=True) + + async def test_refuses_stored_bytes_plus_the_new_size_past_the_quota(self, backend, db_session): + with tenant_context("t1"): + db_session.add(_stored(60)) + await db_session.flush() + guards = _Guards(db_session, backend, quota_bytes=100) + await guards._check_quota(40, reserve=False) # exactly at the limit + with pytest.raises(QuotaExceededError, match="100"): + await guards._check_quota(41, reserve=False) + + async def test_open_sessions_only_count_when_reserving(self, backend, db_session): + with tenant_context("t1"): + db_session.add(_session(total=80)) + await db_session.flush() + guards = _Guards(db_session, backend, quota_bytes=100) + await guards._check_quota(50, reserve=False) + with pytest.raises(QuotaExceededError): + await guards._check_quota(50, reserve=True) + + async def test_expired_and_finished_sessions_release_their_reservation( + self, backend, db_session + ): + with tenant_context("t1"): + db_session.add_all( + [ + _session(total=80, expires_in=-timedelta(minutes=1)), + _session(total=80, status=constants.UploadStatus.COMPLETED), + _session(total=80, status=constants.UploadStatus.ABORTED), + ] + ) + await db_session.flush() + await _Guards(db_session, backend, quota_bytes=100)._check_quota(100, reserve=True) + + async def test_stored_and_reserved_bytes_add_up(self, backend, db_session): + with tenant_context("t1"): + db_session.add_all([_stored(30), _session(total=30)]) + await db_session.flush() + guards = _Guards(db_session, backend, quota_bytes=100) + await guards._check_quota(40, reserve=True) + with pytest.raises(QuotaExceededError): + await guards._check_quota(41, reserve=True) + + async def test_usage_is_per_tenant(self, backend, db_session): + with tenant_context("t2"): + db_session.add(_stored(1000)) + await db_session.flush() + with tenant_context("t1"): + await _Guards(db_session, backend, quota_bytes=100)._check_quota(100, reserve=True) + + async def test_reserve_takes_the_lock_before_reading_usage( + self, backend, db_session, monkeypatch + ): + order: list[str] = [] + guards = _Guards(db_session, backend, quota_bytes=100) + + async def fake_lock() -> None: + order.append("lock") + + monkeypatch.setattr(guards, "_serialize_reservations", fake_lock) + with tenant_context("t1"): + await guards._check_quota(1, reserve=True) + await guards._check_quota(1, reserve=False) + assert order == ["lock"] + + +def _db_on(dialect: str, calls: list[tuple[str, Any]]) -> SimpleNamespace: + """A session stub on ``dialect`` that records every statement it executes.""" + + async def execute(statement: object, params: Any = None) -> None: + calls.append((str(statement), params)) + + bind = SimpleNamespace(dialect=SimpleNamespace(name=dialect)) + return SimpleNamespace(get_bind=lambda: bind, execute=execute) + + +class TestSerializeReservations: + async def test_is_a_no_op_off_postgres(self, backend, db_session): + executed: list[tuple[str, Any]] = [] + await _Guards(_db_on("sqlite", executed), backend)._serialize_reservations() + assert executed == [] + + async def test_takes_a_per_tenant_advisory_lock_on_postgres(self, backend, monkeypatch): + calls: list[tuple[str, Any]] = [] + monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "acme") + await _Guards(_db_on("postgresql", calls), backend)._serialize_reservations() + monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "globex") + await _Guards(_db_on("postgresql", calls), backend)._serialize_reservations() + + assert all("pg_advisory_xact_lock" in sql for sql, _ in calls) + assert calls[0][1]["key"] != calls[1][1]["key"] + assert calls[0][1]["key"] >> 32 == 0x4653544F diff --git a/modules/keycloak/keycloak/settings.py b/modules/keycloak/keycloak/settings.py index fa83e8134..4c34ae4d5 100644 --- a/modules/keycloak/keycloak/settings.py +++ b/modules/keycloak/keycloak/settings.py @@ -8,6 +8,7 @@ from simple_module_core.environments import NON_PROD_ENVIRONMENTS from simple_module_core.redirect_safety import non_empty_redirect from simple_module_core.settings_base import DbBackedSettings +from simple_module_core.stored_settings import field_error DEFAULT_LOGIN_REDIRECT_URL = "/dashboard/" @@ -60,16 +61,22 @@ def _check_required_in_production(self) -> KeycloakSettings: env = os.environ.get("SM_ENVIRONMENT", "development") if env in NON_PROD_ENVIRONMENTS: return self - missing = [] - if not self.server_url: - missing.append("SM_KEYCLOAK_SERVER_URL") - if not self.realm: - missing.append("SM_KEYCLOAK_REALM") - if not self.client_id: - missing.append("SM_KEYCLOAK_CLIENT_ID") - if not self.client_secret: - missing.append("SM_KEYCLOAK_CLIENT_SECRET") + required = ( + ("server_url", "SM_KEYCLOAK_SERVER_URL"), + ("realm", "SM_KEYCLOAK_REALM"), + ("client_id", "SM_KEYCLOAK_CLIENT_ID"), + ("client_secret", "SM_KEYCLOAK_CLIENT_SECRET"), + ) + missing = [(field, var) for field, var in required if not getattr(self, field)] if missing: - msg = f"Keycloak settings required in production: {', '.join(missing)}" - raise ValueError(msg) + # Located on the first empty field so the form shows it there. + msg = f"Keycloak settings required in production: {', '.join(v for _, v in missing)}" + raise field_error( + type(self), + missing[0][0], + "keycloak_settings_required", + msg, + value="", + related=[field for field, _ in required], + ) return self diff --git a/modules/settings/tests/test_lenient_related_fields_validators.py b/modules/settings/tests/test_lenient_related_fields_validators.py new file mode 100644 index 000000000..f447e5af5 --- /dev/null +++ b/modules/settings/tests/test_lenient_related_fields_validators.py @@ -0,0 +1,110 @@ +"""Every cross-field settings validator declares RELATED_FIELDS_CTX (#558). + +Seed a stored value that violates the rule and check lenient hydration drops +only the stored culprit, keeping the module's other overrides. +""" + +from __future__ import annotations + +import pytest +from background_tasks.settings import ENV_PREFIX, BackgroundTasksSettings +from file_storage.settings import FileStorageSettings +from keycloak.settings import KeycloakSettings +from pydantic import ValidationError +from settings.hydrate import RELATED_FIELDS_CTX, build_settings +from simple_module_core.stored_settings import construct_leniently +from simple_module_hosting.host_settings import HostSettings +from site_lock.settings import SiteLockSettings +from users.settings import UsersSettings + + +def _int(v: int) -> tuple[str, str]: + return (str(v), "int") + + +def _str(v: str) -> tuple[str, str]: + return (v, "string") + + +def _bool(v: bool) -> tuple[str, str]: + return (str(v).lower(), "bool") + + +def test_bg_handler_timeout_over_lease_drops_only_the_timeout() -> None: + stored = { + "outbox_lease_seconds": _int(100), + "outbox_handler_timeout_seconds": _int(500), + "max_retries": _int(7), + } + cfg = build_settings(BackgroundTasksSettings, stored, lenient=True) + assert cfg.outbox_handler_timeout_seconds is None + assert (cfg.outbox_lease_seconds, cfg.max_retries) == (100, 7) + + +def test_bg_handler_timeout_pinned_by_env_drops_the_stored_lease(monkeypatch) -> None: + monkeypatch.setenv(f"{ENV_PREFIX}OUTBOX_HANDLER_TIMEOUT_SECONDS", "200") + stored = {"outbox_lease_seconds": _int(100), "max_retries": _int(7)} + cfg = build_settings(BackgroundTasksSettings, stored, lenient=True) + assert cfg.outbox_handler_timeout_seconds == 200 + assert cfg.max_retries == 7 + assert cfg.outbox_lease_seconds != 100 + + +def test_file_storage_s3_missing_config_drops_the_stored_backend() -> None: + stored = { + "backend": _str("s3"), + "s3_bucket": _str("b"), + "max_file_size_bytes": _int(1234), + } + cfg = build_settings(FileStorageSettings, stored, lenient=True) + assert cfg.backend != "s3" + assert cfg.max_file_size_bytes == 1234 + + +def test_file_storage_error_is_located_and_message_unchanged() -> None: + with pytest.raises(ValidationError) as exc: + FileStorageSettings(backend="s3", s3_bucket="b") + err = exc.value.errors()[0] + assert err["loc"] == ("s3_region",) + assert "S3 backend selected but s3_region is not set." in err["msg"] + assert "backend" in err["ctx"][RELATED_FIELDS_CTX] + + +def test_site_lock_enabled_without_password_drops_only_enabled() -> None: + stored = {"enabled": _bool(True), "password": _str(""), "message": _str("maintenance")} + cfg = build_settings(SiteLockSettings, stored, lenient=True) + assert cfg.enabled is False + assert cfg.message == "maintenance" + + +def test_host_default_locale_unsupported_drops_only_the_stored_locale() -> None: + cfg = construct_leniently( + HostSettings, + {"i18n_default_locale": "zz", "maintenance_message": "Keep me"}, + ) + assert cfg.i18n_default_locale in cfg.i18n_supported_locales + assert cfg.maintenance_message == "Keep me" + + +def test_users_placeholder_secret_in_production_is_located_and_still_refused(monkeypatch) -> None: + monkeypatch.setenv("SM_ENVIRONMENT", "production") + stored = {"reset_password_token_lifetime_seconds": _int(99)} + with pytest.raises(ValidationError) as exc: + build_settings(UsersSettings, stored, lenient=True) + err = exc.value.errors()[0] + assert err["loc"] == ("reset_password_token_secret",) + assert set(err["ctx"][RELATED_FIELDS_CTX]) == { + "reset_password_token_secret", + "verification_token_secret", + } + + +def test_keycloak_missing_config_in_production_is_located_and_still_refused(monkeypatch) -> None: + monkeypatch.setenv("SM_ENVIRONMENT", "production") + stored = {"realm": _str("r"), "client_id": _str("c")} + with pytest.raises(ValidationError) as exc: + build_settings(KeycloakSettings, stored, lenient=True) + err = exc.value.errors()[0] + assert err["loc"] == ("server_url",) + assert "SM_KEYCLOAK_SERVER_URL" in err["msg"] + assert "realm" in err["ctx"][RELATED_FIELDS_CTX] diff --git a/modules/site_lock/site_lock/settings.py b/modules/site_lock/site_lock/settings.py index 5043c8473..01afc3f6b 100644 --- a/modules/site_lock/site_lock/settings.py +++ b/modules/site_lock/site_lock/settings.py @@ -7,10 +7,9 @@ from __future__ import annotations from pydantic import model_validator -from pydantic_core import InitErrorDetails, PydanticCustomError -from pydantic_core import ValidationError as CoreValidationError from pydantic_settings import SettingsConfigDict from simple_module_core.settings_base import DbBackedSettings +from simple_module_core.stored_settings import field_error _BLANK_PASSWORD_MSG = "password must be set before enabling the site lock" @@ -45,16 +44,12 @@ def _password_required_when_enabled(self) -> SiteLockSettings: locked when it was still wide open. """ if self.enabled and not self.password.strip(): - raise CoreValidationError.from_exception_data( - title=type(self).__name__, - line_errors=[ - InitErrorDetails( - type=PydanticCustomError( - "site_lock_password_required", _BLANK_PASSWORD_MSG - ), - loc=("enabled",), - input=self.enabled, - ) - ], + raise field_error( + type(self), + "enabled", + "site_lock_password_required", + _BLANK_PASSWORD_MSG, + value=self.enabled, + related=["enabled", "password"], ) return self diff --git a/modules/tenants/README.md b/modules/tenants/README.md index 13d03483a..6b75daf1a 100644 --- a/modules/tenants/README.md +++ b/modules/tenants/README.md @@ -78,5 +78,7 @@ 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). diff --git a/modules/tenants/tenants/_deletion.py b/modules/tenants/tenants/_deletion.py new file mode 100644 index 000000000..ed4718def --- /dev/null +++ b/modules/tenants/tenants/_deletion.py @@ -0,0 +1,36 @@ +"""Removing a tenant and the rows that only make sense inside it.""" + +from __future__ import annotations + +from simple_module_core.events import Event +from sqlalchemy import delete, select +from sqlalchemy.ext.asyncio import AsyncSession + +from tenants.contracts.events import MembershipRemoved, TenantDeleted +from tenants.models import Invitation, Membership, Tenant + + +async def delete_tenant_rows(db: AsyncSession, tenant: Tenant) -> list[Event]: + """Delete memberships, invitations and the tenant itself, then flush. + + Returns the events for the caller to queue on commit: one + ``MembershipRemoved`` per member — the same event a single removal + publishes, so seat counts and per-user state follow — then ``TenantDeleted``. + + Done explicitly rather than leaning on ``ON DELETE CASCADE``, which only + fires where the database enforces foreign keys. + """ + user_ids = ( + await db.scalars( + select(Membership.user_id) + .where(Membership.tenant_id == tenant.id) + .order_by(Membership.user_id) + ) + ).all() + for model in (Membership, Invitation): + await db.execute(delete(model).where(model.tenant_id == tenant.id)) + events: list[Event] = [MembershipRemoved(tenant.id, user_id) for user_id in user_ids] + events.append(TenantDeleted(tenant.id, tenant.slug, tenant.name)) + await db.delete(tenant) + await db.flush() + return events diff --git a/modules/tenants/tenants/contracts/events.py b/modules/tenants/tenants/contracts/events.py index 136c5a4f4..c15b003fa 100644 --- a/modules/tenants/tenants/contracts/events.py +++ b/modules/tenants/tenants/contracts/events.py @@ -29,6 +29,20 @@ class TenantStatusChanged(Event): previous: str +@dataclass +class TenantDeleted(Event): + """The tenant and its memberships/invitations are gone. + + Modules keeping per-tenant state of their own (rows, external + subscriptions) clean it up here. Carries the slug and name because the + tenant row no longer exists to look them up. + """ + + tenant_id: str + slug: str + name: str + + @dataclass class MembershipAdded(Event): tenant_id: str diff --git a/modules/tenants/tenants/service.py b/modules/tenants/tenants/service.py index f646a8d49..5b1f9dd10 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -22,6 +22,7 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession +from tenants._deletion import delete_tenant_rows from tenants._owner_guard import demote_owner, remove_owner from tenants.constants import ( ENTITLEMENT_SEATS, @@ -143,6 +144,10 @@ async def set_status(self, tenant_id: str, status: TenantStatus) -> Tenant: self._after_commit(TenantStatusChanged(tenant_id, status, previous), invalidate=[None]) return tenant + async def delete_tenant(self, tenant_id: str) -> None: + tenant = await self.lock(tenant_id) + self._after_commit(*await delete_tenant_rows(self.db, tenant), invalidate=[None]) + async def list_all(self, *, search: str = "", limit: int = 50, offset: int = 0) -> list[Tenant]: stmt = select(Tenant).order_by(Tenant.created_at.desc()).limit(limit).offset(offset) if search: diff --git a/modules/tenants/tests/test_tenant_deleted_event.py b/modules/tenants/tests/test_tenant_deleted_event.py new file mode 100644 index 000000000..cbaff683f --- /dev/null +++ b/modules/tenants/tests/test_tenant_deleted_event.py @@ -0,0 +1,126 @@ +"""``TenantService.delete_tenant`` publishes its events and invalidates after commit only.""" + +from __future__ import annotations + +import pytest +from simple_module_core.events import EventBus +from simple_module_db.transaction import finalize_session, rollback_session +from sqlalchemy import func, select +from tenants import resolver +from tenants.constants import MembershipRole +from tenants.contracts.events import MembershipRemoved, TenantDeleted +from tenants.contracts.schemas import TenantCreate +from tenants.models import Membership, Tenant +from tenants.service import TenantService + + +async def _tenant_with_owner(db_session, bus: EventBus) -> tuple[TenantService, Tenant]: + svc = TenantService(db_session, bus=bus) + tenant = await svc.create_tenant(TenantCreate(name="Acme"), owner_user_id="u1") + await finalize_session(db_session) + return svc, tenant + + +async def test_delete_publishes_after_commit(db_session): + bus = EventBus() + seen: list[TenantDeleted] = [] + + async def handler(event: TenantDeleted) -> None: + # The delete is already durable when the handler runs. + assert await db_session.get(Tenant, event.tenant_id) is None + seen.append(event) + + bus.subscribe(TenantDeleted, handler) + svc, tenant = await _tenant_with_owner(db_session, bus) + + await svc.delete_tenant(tenant.id) + assert seen == [] # nothing before the commit + + await finalize_session(db_session) + assert seen == [TenantDeleted(tenant.id, "acme", "Acme")] + members = await db_session.scalar(select(func.count()).select_from(Membership)) + assert members == 0 + + +async def test_rolled_back_delete_publishes_nothing(db_session): + bus = EventBus() + seen: list[TenantDeleted] = [] + + async def handler(event: TenantDeleted) -> None: + seen.append(event) + + bus.subscribe(TenantDeleted, handler) + svc, tenant = await _tenant_with_owner(db_session, bus) + + await svc.delete_tenant(tenant.id) + await rollback_session(db_session) + + assert seen == [] + remaining = await db_session.scalar(select(func.count()).select_from(Tenant)) + assert remaining == 1 + + +@pytest.fixture(autouse=True) +def _clear_membership_cache(): + yield + resolver.forget(None) + + +async def _tenant_with_members(db_session, bus: EventBus, invalidated: list[str | None]): + async def invalidate(key: str | None) -> None: + resolver.forget(key) + invalidated.append(key) + + svc = TenantService(db_session, bus=bus, invalidate=invalidate) + tenant = await svc.create_tenant(TenantCreate(name="Acme"), owner_user_id="u1") + await svc.add_member(tenant.id, "u2", MembershipRole.MEMBER) + await finalize_session(db_session) + invalidated.clear() + for user_id in ("u1", "u2"): # both members' tenant lists are cached + resolver._MEMBERSHIPS[user_id] = [] + return svc, tenant + + +async def test_delete_removes_every_member_after_commit(db_session): + bus = EventBus() + removed: list[MembershipRemoved] = [] + + async def handler(event: MembershipRemoved) -> None: + removed.append(event) + + bus.subscribe(MembershipRemoved, handler) + invalidated: list[str | None] = [] + svc, tenant = await _tenant_with_members(db_session, bus, invalidated) + + await svc.delete_tenant(tenant.id) + assert removed == [] and invalidated == [] # nothing before the commit + assert resolver.cached_memberships("u2") is not None + + await finalize_session(db_session) + assert sorted(removed, key=lambda e: e.user_id) == [ + MembershipRemoved(tenant.id, "u1"), + MembershipRemoved(tenant.id, "u2"), + ] + assert invalidated == [None] # one broadcast drops every worker's cache + assert resolver.cached_memberships("u1") is None + assert resolver.cached_memberships("u2") is None + + +async def test_rolled_back_delete_removes_no_member(db_session): + bus = EventBus() + removed: list[MembershipRemoved] = [] + + async def handler(event: MembershipRemoved) -> None: + removed.append(event) + + bus.subscribe(MembershipRemoved, handler) + invalidated: list[str | None] = [] + svc, tenant = await _tenant_with_members(db_session, bus, invalidated) + + await svc.delete_tenant(tenant.id) + await rollback_session(db_session) + + assert removed == [] and invalidated == [] + assert resolver.cached_memberships("u2") is not None + members = await db_session.scalar(select(func.count()).select_from(Membership)) + assert members == 2 diff --git a/modules/tenants/tests/test_tenants_shared_props.py b/modules/tenants/tests/test_tenants_shared_props.py new file mode 100644 index 000000000..dbbe3c2c1 --- /dev/null +++ b/modules/tenants/tests/test_tenants_shared_props.py @@ -0,0 +1,101 @@ +"""Direct tests for the ``tenant`` shared Inertia prop provider.""" + +from __future__ import annotations + +from datetime import UTC, datetime +from types import SimpleNamespace +from typing import Any, cast + +from starlette.requests import Request +from tenants import resolver +from tenants.constants import TenantStatus +from tenants.contracts.schemas import MyTenantView +from tenants.shared_props import tenant_shared_props + +CREATED = datetime(2026, 1, 2, 3, 4, 5, tzinfo=UTC) + + +def _view(tenant_id: str, role: str = "member") -> MyTenantView: + return MyTenantView( + id=tenant_id, + slug=tenant_id, + name=f"Org {tenant_id}", + status=TenantStatus.ACTIVE, + created_at=CREATED, + role=role, + ) + + +def _request(**state: Any) -> Request: + # The provider only reads ``request.state``; a real ASGI scope isn't needed. + return cast("Request", SimpleNamespace(state=SimpleNamespace(**state))) + + +def test_anonymous_request_contributes_nothing(): + assert tenant_shared_props(_request()) == {} + assert tenant_shared_props(_request(user=None)) == {} + + +def test_user_with_no_cached_memberships_gets_empty_lists(): + request = _request(user=SimpleNamespace(id="u1"), tenant_id=None) + assert tenant_shared_props(request) == { + "tenant": {"active": None, "memberships": [], "suspended": False} + } + + +def test_active_tenant_is_the_membership_matching_request_tenant_id(): + resolver._MEMBERSHIPS["u1"] = [_view("acme", "owner"), _view("globex")] + request = _request(user=SimpleNamespace(id="u1"), tenant_id="globex") + + tenant = tenant_shared_props(request)["tenant"] + + assert tenant["active"]["id"] == "globex" + assert tenant["active"]["role"] == "member" + assert [m["id"] for m in tenant["memberships"]] == ["acme", "globex"] + + +def test_active_is_none_when_the_resolved_tenant_is_not_a_membership(): + resolver._MEMBERSHIPS["u1"] = [_view("acme")] + request = _request(user=SimpleNamespace(id="u1"), tenant_id="elsewhere") + + tenant = tenant_shared_props(request)["tenant"] + + assert tenant["active"] is None + assert len(tenant["memberships"]) == 1 + + +def test_memberships_are_json_safe(): + resolver._MEMBERSHIPS["u1"] = [_view("acme", "admin")] + request = _request(user=SimpleNamespace(id="u1"), tenant_id="acme") + + tenant = tenant_shared_props(request)["tenant"] + + # ``mode="json"`` turns the datetime into a string Inertia can serialise. + assert tenant["active"]["created_at"] == CREATED.isoformat().replace("+00:00", "Z") + assert isinstance(tenant["memberships"][0]["created_at"], str) + + +def test_suspended_flag_follows_request_state(): + resolver._MEMBERSHIPS["u1"] = [_view("acme")] + request = _request(user=SimpleNamespace(id="u1"), tenant_id="acme", tenant_suspended=True) + assert tenant_shared_props(request)["tenant"]["suspended"] is True + + +def test_memberships_are_looked_up_by_the_users_own_id_only(): + resolver._MEMBERSHIPS["someone-else"] = [_view("secret-org")] + request = _request(user=SimpleNamespace(id="u1"), tenant_id="secret-org") + + tenant = tenant_shared_props(request)["tenant"] + + assert tenant["memberships"] == [] + assert tenant["active"] is None + + +def test_user_id_is_stringified_for_the_cache_lookup(): + import uuid + + uid = uuid.uuid4() + resolver._MEMBERSHIPS[str(uid)] = [_view("acme")] + request = _request(user=SimpleNamespace(id=uid), tenant_id="acme") + + assert tenant_shared_props(request)["tenant"]["active"]["id"] == "acme" diff --git a/modules/users/users/settings.py b/modules/users/users/settings.py index 293991ca9..2fb0773b7 100644 --- a/modules/users/users/settings.py +++ b/modules/users/users/settings.py @@ -19,6 +19,7 @@ from simple_module_core.environments import NON_PROD_ENVIRONMENTS from simple_module_core.redirect_safety import non_empty_redirect from simple_module_core.settings_base import DbBackedSettings +from simple_module_core.stored_settings import field_error from users.session_version_cache import SESSION_VERSION_TTL_SECONDS @@ -226,14 +227,22 @@ def _forbid_placeholder_token_secrets_in_production(self) -> UsersSettings: return self bad = [] if self.reset_password_token_secret == _PLACEHOLDER_RESET_SECRET: - bad.append("RESET_PASSWORD_TOKEN_SECRET") + bad.append(("reset_password_token_secret", "RESET_PASSWORD_TOKEN_SECRET")) if self.verification_token_secret == _PLACEHOLDER_VERIFY_SECRET: - bad.append("VERIFICATION_TOKEN_SECRET") + bad.append(("verification_token_secret", "VERIFICATION_TOKEN_SECRET")) if bad: - names = ", ".join(bad) - raise ValueError( + names = ", ".join(label for _, label in bad) + # Located on the first placeholder field. Dropping a stored + # placeholder falls back to the same placeholder, so lenient boot + # still refuses: the check cannot be sidestepped. + raise field_error( + type(self), + bad[0][0], + "placeholder_token_secret", f"users.{names} must be set to non-default value(s) when " f"SM_ENVIRONMENT={env!r}. Generate with " - "`python -c 'import secrets; print(secrets.token_urlsafe(48))'`." + "`python -c 'import secrets; print(secrets.token_urlsafe(48))'`.", + value="", + related=["reset_password_token_secret", "verification_token_secret"], ) return self diff --git a/pyproject.toml b/pyproject.toml index 8fed39acc..2b45c7f67 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,9 +67,11 @@ ignore = [ [tool.ty.environment] extra-paths = [ + "framework/cli", "framework/core", "framework/db", "framework/hosting", + "framework/inertia", "framework/testing", "modules/auth", "modules/dashboard", diff --git a/scripts/bench_core_scope.py b/scripts/bench_core_scope.py new file mode 100644 index 000000000..50e7fb08b --- /dev/null +++ b/scripts/bench_core_scope.py @@ -0,0 +1,105 @@ +"""Micro-benchmark for ``core_scope.scope_core`` (GH #559). Not a test. + + uv run python scripts/bench_core_scope.py + +Times the per-statement tenancy scope walk on representative ORM selects +(the hot path: every ORM query pays it while tenancy names are set) and a +few Core statements (which are cloned and rewritten). Statements are built +once, as ``filter_statements`` sees them, and ``scope_core`` is called the +way ``query_filter`` calls it, with tenant ``A`` bound. + +No ``from __future__ import annotations``: SQLModel resolves the +relationship annotations at class creation. +""" + +import sys +import timeit +from collections.abc import Callable +from typing import Any + +from simple_module_db.base import create_module_base +from simple_module_db.core_scope import scope_core +from simple_module_db.mixins import MultiTenantMixin, SoftDeleteMixin +from simple_module_db.query_filter import _orm_covered, _scoped_names, _subquery_predicates +from sqlalchemy import exists, func, select +from sqlalchemy.orm import aliased, configure_mappers, selectinload +from sqlmodel import Field, Relationship + +_Base = create_module_base("bench559") + + +class Author(_Base, MultiTenantMixin, SoftDeleteMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "bench559_author" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(default="", max_length=100) + posts: list["Post"] = Relationship(back_populates="author") + + +class Post(_Base, MultiTenantMixin, SoftDeleteMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "bench559_post" + id: int | None = Field(default=None, primary_key=True) + title: str = Field(default="", max_length=100) + author_id: int | None = Field(default=None, foreign_key="bench559_author.id") + author: Author | None = Relationship(back_populates="posts") + + +post, author = Post.__table__, Author.__table__ + + +def _orm_statements() -> dict[str, Any]: + page = select(Post).where(Post.title.like("a%")).order_by(Post.id) + return { + "orm get": select(Post).where(Post.id == 1), + "orm join + selectinload": select(Post) + .join(Post.author) + .where(Author.name == "x") + .options(selectinload(Post.author)), + "orm subquery in WHERE": select(Post).where( + Post.author_id.in_(select(Author.id).where(Author.name == "x")) + ), + "orm exists in WHERE": select(Author).where( + exists().where(Post.author_id == Author.id, Post.title == "t") + ), + "orm pagination page": page.limit(20).offset(40), + "orm pagination count": select(func.count()).select_from(page.subquery()), + "orm aliased join": select(Post).join(aliased(Author), Post.author), + } + + +def _core_statements() -> dict[str, Any]: + return { + "core select": select(post.c.id).where(post.c.title == "t"), + "core join": select(post.c.id).select_from( + post.outerjoin(author, author.c.id == post.c.author_id) + ), + "core in ORM WHERE": select(Post).where(Post.author_id.in_(select(author.c.id))), + } + + +def _bench(label: str, stmt: Any, call: Callable[[Any], Any], number: int) -> float: + best = min(timeit.repeat(lambda: call(stmt), number=number, repeat=5)) + micros = best / number * 1e6 + print(f" {label:<28} {micros:8.2f} µs") + return micros + + +def main(number: int = 2000) -> None: + from simple_module_db.tenancy import current_tenant_id + + configure_mappers() # fills model_registry's scoped table names + current_tenant_id.set("A") + preds = _subquery_predicates(False, False, "A", False) + names = _scoped_names(False, False) + covered = _orm_covered(False, False, "A") + + def call(stmt: Any) -> Any: + return scope_core(stmt, preds, names, covered) + + for title, stmts in (("ORM", _orm_statements()), ("Core", _core_statements())): + print(f"{title} statements (best of 5 x {number}):") + times = [_bench(label, stmt, call, number) for label, stmt in stmts.items()] + print(f" {'mean':<28} {sum(times) / len(times):8.2f} µs") + + +if __name__ == "__main__": + main(int(sys.argv[1]) if len(sys.argv) > 1 else 2000) diff --git a/scripts/check_metadata.py b/scripts/check_metadata.py index 30032e287..6b3c71f44 100644 --- a/scripts/check_metadata.py +++ b/scripts/check_metadata.py @@ -5,6 +5,9 @@ publish-pypi matrix of `.github/workflows/release.yml` (and vice versa) — a workspace package missing from the matrix builds a wheel on every release and then silently never reaches PyPI. + * The root pyproject's `[tool.ty.environment] extra-paths` and + `[tool.pytest.ini_options] testpaths` must list every framework/* and + modules/* package (and its tests/ dir) on disk, and nothing stale (#520). * Every `pyproject.toml` under framework/* and modules/* must have: - name starting with "simple_module_" - non-placeholder description (not "Add your description here" or empty) @@ -194,6 +197,32 @@ def check_release_matrix(root: Path) -> list[str]: return errors +def check_tooling_lists(root: Path) -> list[str]: + """Assert ty `extra-paths` and pytest `testpaths` track the packages on disk.""" + root_toml = root / "pyproject.toml" + if not root_toml.exists(): + return [] + data = tomlkit.parse(root_toml.read_text(encoding="utf-8")) + tool = data.get("tool", {}) + extra = {str(p) for p in tool.get("ty", {}).get("environment", {}).get("extra-paths", [])} + tests = {str(p) for p in tool.get("pytest", {}).get("ini_options", {}).get("testpaths", [])} + + pkgs = [p.parent.relative_to(root).as_posix() for p in discover_python_packages(root)] + want_extra = set(pkgs) + want_tests = {f"{p}/tests" for p in pkgs if (root / p / "tests").is_dir()} + + errors: list[str] = [] + for label, have, want in ( + ("[tool.ty.environment] extra-paths", extra, want_extra), + ("[tool.pytest.ini_options] testpaths", tests, want_tests), + ): + for entry in sorted(want - have): + errors.append(f"pyproject.toml {label}: missing '{entry}' (exists on disk)") + for entry in sorted(e for e in have - want if e.startswith(("framework/", "modules/"))): + errors.append(f"pyproject.toml {label}: stale '{entry}' (not on disk)") + return errors + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( @@ -211,6 +240,7 @@ def main(argv: list[str] | None = None) -> int: for pkg in discover_npm_packages(root): all_errors.extend(check_npm_package(pkg)) all_errors.extend(check_release_matrix(root)) + all_errors.extend(check_tooling_lists(root)) if all_errors: for e in all_errors: diff --git a/scripts/check_test_sleeps.py b/scripts/check_test_sleeps.py new file mode 100644 index 000000000..15704fa14 --- /dev/null +++ b/scripts/check_test_sleeps.py @@ -0,0 +1,138 @@ +"""Flag fixed ``sleep()`` calls in test files. + +A fixed sleep standing in for "until X happened" is a bet on timing: it flakes +on a loaded CI runner and slows the suite on a fast one. Poll with +``simple_module_test.wait_for`` instead. A sleep that is genuinely the point of +the test (a stub that blocks until cancelled, a simulated slow handler, a real +timing measurement) stays, marked on the same line: + + await asyncio.sleep(30) # intentional-sleep: hangs until the timeout cancels it + +``sleep(0)`` (a bare event-loop yield) is always allowed. Covers ``test_*.py``, +``conftest.py`` and anything under a ``tests`` directory. + +Usage: + uv run python scripts/check_test_sleeps.py +""" + +from __future__ import annotations + +import ast +import re +import subprocess +import sys +from collections.abc import Iterable +from pathlib import Path + +MARKER = "# intentional-sleep:" +# Its fixtures are sleep() calls written as strings. +SELF_TEST = Path("scripts/tests/test_check_test_sleeps.py") +_SLEEP_CALL = re.compile(r"\bsleep\(\s*([^)]*)\)") +_ZERO = re.compile(r"^0+(\.0*)?$") + + +def is_test_file(path: Path) -> bool: + """Return True for ``test_*.py``, ``conftest.py`` and files under ``tests/``.""" + if path.suffix != ".py": + return False + return path.name.startswith("test_") or path.name == "conftest.py" or "tests" in path.parts + + +def _is_zero(node: ast.expr) -> bool: + return ( + isinstance(node, ast.Constant) + and isinstance(node.value, (int, float)) + and not isinstance(node.value, bool) + and node.value == 0 + ) + + +def _is_sleep_call(node: ast.AST) -> bool: + if not isinstance(node, ast.Call): + return False + func = node.func + return (isinstance(func, ast.Name) and func.id == "sleep") or ( + isinstance(func, ast.Attribute) and func.attr == "sleep" + ) + + +def _find_by_ast(tree: ast.AST, lines: list[str]) -> list[tuple[int, str]]: + found: list[tuple[int, str]] = [] + for node in ast.walk(tree): + if not _is_sleep_call(node): + continue + assert isinstance(node, ast.Call) + if node.args and _is_zero(node.args[0]): + continue + # A formatter may split the call over several lines; the marker may sit on any. + end = node.end_lineno or node.lineno + if any(MARKER in line for line in lines[node.lineno - 1 : end]): + continue + found.append((node.lineno, lines[node.lineno - 1].strip())) + return sorted(found) + + +def _find_by_regex(lines: list[str]) -> list[tuple[int, str]]: + found: list[tuple[int, str]] = [] + for number, line in enumerate(lines, start=1): + if MARKER in line: + continue + code = line.split("#", 1)[0] + for match in _SLEEP_CALL.finditer(code): + if not _ZERO.match(match.group(1).strip()): + found.append((number, line.strip())) + break + return found + + +def find_unmarked_sleeps(text: str) -> list[tuple[int, str]]: + """Return ``(line_number, stripped_line)`` for each unmarked non-zero sleep. + + Parsed with ``ast`` so a call split across lines by the formatter, or a + ``#`` inside a string, cannot hide one; a file that does not parse falls + back to a per-line regex. + """ + lines = text.splitlines() + try: + tree = ast.parse(text) + except SyntaxError: + return _find_by_regex(lines) + return _find_by_ast(tree, lines) + + +def _list_git_files(root: Path) -> list[Path]: + result = subprocess.run( + ["git", "ls-files", "--cached", "--others", "--exclude-standard"], + cwd=root, + capture_output=True, + text=True, + check=True, + ) + return [Path(line) for line in result.stdout.splitlines() if line] + + +def check(root: Path, paths: Iterable[Path]) -> list[tuple[Path, int, str]]: + violations: list[tuple[Path, int, str]] = [] + for rel in paths: + if rel == SELF_TEST or not is_test_file(rel) or not (root / rel).is_file(): + continue + text = (root / rel).read_text(encoding="utf-8", errors="replace") + violations.extend((rel, n, line) for n, line in find_unmarked_sleeps(text)) + return violations + + +def main() -> int: + root = Path.cwd() + violations = check(root, _list_git_files(root)) + if not violations: + print("OK: no unmarked fixed sleeps in tests.") + return 0 + print(f"FAIL: {len(violations)} fixed sleep(s) in tests without `{MARKER} `.") + print("Poll with simple_module_test.wait_for, or mark a deliberate one.") + for path, number, line in violations: + print(f" {path.as_posix()}:{number}: {line}") + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/test_check_test_sleeps.py b/scripts/tests/test_check_test_sleeps.py new file mode 100644 index 000000000..4d8eea6c0 --- /dev/null +++ b/scripts/tests/test_check_test_sleeps.py @@ -0,0 +1,50 @@ +"""Tests for the fixed-sleep lint in test files.""" + +from __future__ import annotations + +from pathlib import Path + +from check_test_sleeps import check, find_unmarked_sleeps, is_test_file + + +def test_flags_nonzero_sleep(): + src = ( + "async def t():\n await asyncio.sleep(0.3)\n time.sleep(1)\n await sleep(delay)\n" + ) + assert [n for n, _ in find_unmarked_sleeps(src)] == [2, 3, 4] + + +def test_allows_zero_yield_and_marked_sleeps(): + src = ( + "await asyncio.sleep(0)\n" + "await asyncio.sleep(0.0)\n" + "await asyncio.sleep(30) # intentional-sleep: blocks until cancelled\n" + "x = 1 # sleep(5) in a comment\n" + ) + assert find_unmarked_sleeps(src) == [] + + +def test_is_test_file(): + assert is_test_file(Path("modules/a/tests/helpers.py")) + assert is_test_file(Path("modules/a/test_x.py")) + assert is_test_file(Path("conftest.py")) + assert not is_test_file(Path("modules/a/service.py")) + assert not is_test_file(Path("modules/a/tests/data.json")) + + +def test_check_reports_only_test_files(tmp_path): + (tmp_path / "tests").mkdir() + (tmp_path / "tests" / "test_a.py").write_text("time.sleep(2)\n") + (tmp_path / "service.py").write_text("time.sleep(2)\n") + found = check(tmp_path, [Path("tests/test_a.py"), Path("service.py")]) + assert [(p.as_posix(), n) for p, n, _ in found] == [("tests/test_a.py", 1)] + + +def test_flags_sleep_split_across_lines_and_after_a_hash_in_a_string(): + src = "await asyncio.sleep(\n 0.5\n)\nx = '#'; time.sleep(2)\n" + assert [n for n, _ in find_unmarked_sleeps(src)] == [1, 4] + + +def test_marker_on_any_line_of_a_split_call_allows_it(): + src = "await asyncio.sleep(\n 0.5\n) # intentional-sleep: simulated slow work\n" + assert find_unmarked_sleeps(src) == [] diff --git a/scripts/tests/test_check_tooling_lists.py b/scripts/tests/test_check_tooling_lists.py new file mode 100644 index 000000000..25da1dc72 --- /dev/null +++ b/scripts/tests/test_check_tooling_lists.py @@ -0,0 +1,47 @@ +"""check_tooling_lists: ty extra-paths / pytest testpaths vs packages on disk (#520).""" + +from __future__ import annotations + +from pathlib import Path + +from scripts.check_metadata import check_tooling_lists + +OK_EXTRA = ["framework/core", "modules/blog", "host"] +OK_TESTS = ["framework/core/tests", "modules/blog/tests", "tests"] + + +def _quote(items: list[str]) -> str: + return ", ".join(f'"{x}"' for x in items) + + +def _repo(tmp_path: Path, extra: list[str], tests: list[str]) -> Path: + for pkg in ("framework/core", "modules/blog"): + (tmp_path / pkg / "tests").mkdir(parents=True) + (tmp_path / pkg / "pyproject.toml").write_text("[project]\nname = 'x'\n") + (tmp_path / "pyproject.toml").write_text( + f"[tool.ty.environment]\nextra-paths = [{_quote(extra)}]\n" + f"[tool.pytest.ini_options]\ntestpaths = [{_quote(tests)}]\n" + ) + return tmp_path + + +def test_matching_lists_pass(tmp_path: Path) -> None: + assert check_tooling_lists(_repo(tmp_path, OK_EXTRA, OK_TESTS)) == [] + + +def test_missing_entry_fails(tmp_path: Path) -> None: + errors = check_tooling_lists(_repo(tmp_path, ["framework/core", "host"], OK_TESTS[:1])) + assert any("missing 'modules/blog'" in e for e in errors) + assert any("missing 'modules/blog/tests'" in e for e in errors) + + +def test_stale_entry_fails(tmp_path: Path) -> None: + errors = check_tooling_lists( + _repo(tmp_path, [*OK_EXTRA, "modules/gone"], [*OK_TESTS, "modules/gone/tests"]) + ) + assert any("stale 'modules/gone'" in e for e in errors) + assert any("stale 'modules/gone/tests'" in e for e in errors) + + +def test_real_repo_lists_match_disk() -> None: + assert check_tooling_lists(Path(__file__).resolve().parents[2]) == [] diff --git a/tests/integration/test_invalidation_cross_process.py b/tests/integration/test_invalidation_cross_process.py index 7ad36763e..3546fd2bf 100644 --- a/tests/integration/test_invalidation_cross_process.py +++ b/tests/integration/test_invalidation_cross_process.py @@ -185,7 +185,7 @@ def _boot(stack: _Stack, *, broadcast: bool, channel: str) -> _Worker: return worker except httpx.HTTPError: pass - time.sleep(0.2) + time.sleep(0.2) # intentional-sleep: boot poll that also watches for process death worker.stop() raise AssertionError("worker did not become ready in time") @@ -244,7 +244,7 @@ def _seconds_until_rejected(client: httpx.Client, limit: float) -> float | None: while time.monotonic() - start < limit: if client.get(_GUARDED, follow_redirects=False).status_code in (302, 401): return time.monotonic() - start - time.sleep(0.2) + time.sleep(0.2) # intentional-sleep: measuring time-to-reject is a real timing test return None