From a66aeb64e47917bffba3ea187944c96ed3a851e7 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:09:42 +0200 Subject: [PATCH 01/12] chore(tooling): check ty extra-paths and pytest testpaths against disk (#520) Add framework/cli and framework/inertia to ty extra-paths; check_metadata.py now fails when extra-paths or testpaths drift from framework/* and modules/*. Runs in make lint and the python-lint CI job. new_module.py already appends to both lists. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .github/workflows/pr.yml | 4 ++ pyproject.toml | 2 + scripts/check_metadata.py | 30 +++++++++++++++ scripts/tests/test_check_tooling_lists.py | 47 +++++++++++++++++++++++ 4 files changed, 83 insertions(+) create mode 100644 scripts/tests/test_check_tooling_lists.py diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 184371bbf..d2bfeb71b 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -37,6 +37,10 @@ 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 # 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/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/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/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]) == [] From 93dc884cad177579f414092e232783dfc23f87e2 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:16:46 +0200 Subject: [PATCH 02/12] test: cover api_tokens, feature_flags and background_tasks pages, tenants shared props, upload guards (#514) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests-js/api_tokens_Admin.test.tsx | 132 +++++++++ .../api_tokens_CreatedTokenDialog.test.tsx | 71 +++++ .../tests-js/api_tokens_Index.test.tsx | 232 +++++++++++++++ .../background_tasks_DetailPage.test.tsx | 144 +++++++++ .../background_tasks_IndexPage.test.tsx | 185 ++++++++++++ .../background_tasks_OutboxPage.test.tsx | 182 ++++++++++++ .../tests-js/feature_flags_Browse.test.tsx | 196 +++++++++++++ .../tests/test_file_storage_upload_guards.py | 277 ++++++++++++++++++ .../tests/test_tenants_shared_props.py | 109 +++++++ 9 files changed, 1528 insertions(+) create mode 100644 modules/api_tokens/tests-js/api_tokens_Admin.test.tsx create mode 100644 modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx create mode 100644 modules/api_tokens/tests-js/api_tokens_Index.test.tsx create mode 100644 modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx create mode 100644 modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx create mode 100644 modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx create mode 100644 modules/feature_flags/tests-js/feature_flags_Browse.test.tsx create mode 100644 modules/file_storage/tests/test_file_storage_upload_guards.py create mode 100644 modules/tenants/tests/test_tenants_shared_props.py 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..fc9b80b10 --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx @@ -0,0 +1,132 @@ +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, 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 type { ApiToken } from '../api_tokens/pages/types'; + +const SECRET = 'smpy_svc_adminsecret'; + +const SERVICE: ApiToken = { + id: 's1', + kind: 'service', + name: 'ingest-bot', + prefix: 'smpy_zz99', + scopes: ['files.read'], + roles: ['uploader'], + owner_user_id: null, + owner_email: null, + tenant_id: 'acme', + created_at: '2026-01-01T00:00:00Z', + expires_at: '2999-01-01T00:00:00Z', + last_used_at: null, + revoked_at: null, +}; + +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, + }; +} + +function stubFetch(status: number, body: unknown) { + const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +describe('api_tokens Admin', () => { + beforeEach(() => setProps()); + afterEach(() => { + cleanup(); + 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..9cd6391a2 --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx @@ -0,0 +1,71 @@ +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: {} }); + +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(() => { + cleanup(); + 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..0f4651418 --- /dev/null +++ b/modules/api_tokens/tests-js/api_tokens_Index.test.tsx @@ -0,0 +1,232 @@ +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, 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 type { ApiToken } from '../api_tokens/pages/types'; + +const SECRET = 'smpy_pat_topsecretvalue'; +const FUTURE = '2999-01-01T00:00:00Z'; + +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: FUTURE, + last_used_at: null, + revoked_at: null, + ...over, + }; +} + +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, + }; +} + +function stubFetch(status: number, body: unknown) { + const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +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(() => { + cleanup(); + 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/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..439073239 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx @@ -0,0 +1,144 @@ +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 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'; + +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 } }; +} + +function stubFetch(status: number, body: unknown) { + const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +describe('background_tasks Detail page', () => { + beforeEach(() => setProps()); + afterEach(() => { + cleanup(); + 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..763fb8b38 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx @@ -0,0 +1,185 @@ +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'; + +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, + }; +} + +function stubFetch(status: number, body: unknown) { + const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +describe('background_tasks Index page', () => { + beforeEach(() => setProps()); + afterEach(() => { + cleanup(); + 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..069b7b648 --- /dev/null +++ b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx @@ -0,0 +1,182 @@ +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()); +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'; + +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, + }; +} + +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; +} + +describe('background_tasks Outbox page', () => { + beforeEach(() => setProps()); + afterEach(() => { + cleanup(); + 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/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..69382c23c --- /dev/null +++ b/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx @@ -0,0 +1,196 @@ +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 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(() => { + cleanup(); + 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/tests/test_file_storage_upload_guards.py b/modules/file_storage/tests/test_file_storage_upload_guards.py new file mode 100644 index 000000000..c1e76e253 --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_upload_guards.py @@ -0,0 +1,277 @@ +"""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"] + + +class TestSerializeReservations: + async def test_is_a_no_op_off_postgres(self, backend, db_session): + executed: list[object] = [] + + class _Db: + def get_bind(self): + return SimpleNamespace(dialect=SimpleNamespace(name="sqlite")) + + async def execute(self, *args: object, **kwargs: object) -> None: + executed.append(args) + + await _Guards(_Db(), backend)._serialize_reservations() + assert executed == [] + + async def test_takes_a_per_tenant_advisory_lock_on_postgres(self, backend, monkeypatch): + calls: list[tuple[str, dict]] = [] + + class _Db: + def get_bind(self): + return SimpleNamespace(dialect=SimpleNamespace(name="postgresql")) + + async def execute(self, statement: object, params: dict) -> None: + calls.append((str(statement), params)) + + monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "acme") + await _Guards(_Db(), backend)._serialize_reservations() + monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "globex") + await _Guards(_Db(), 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/tenants/tests/test_tenants_shared_props.py b/modules/tenants/tests/test_tenants_shared_props.py new file mode 100644 index 000000000..e9c140afd --- /dev/null +++ b/modules/tenants/tests/test_tenants_shared_props.py @@ -0,0 +1,109 @@ +"""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 + +import pytest +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))) + + +@pytest.fixture(autouse=True) +def _clean_cache(): + resolver._MEMBERSHIPS.clear() + yield + resolver._MEMBERSHIPS.clear() + + +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" From eeacebce5e2a7fc9b915c45ca3b17174be49f1c9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:17:19 +0200 Subject: [PATCH 03/12] fix(settings): cross-field validators declare related_fields (#558) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_core/stored_settings.py | 32 ++++- .../background_tasks/settings.py | 15 ++- modules/file_storage/file_storage/settings.py | 13 ++- modules/keycloak/keycloak/settings.py | 29 +++-- .../test_lenient_related_fields_validators.py | 110 ++++++++++++++++++ modules/site_lock/site_lock/settings.py | 21 ++-- modules/users/users/settings.py | 19 ++- 7 files changed, 204 insertions(+), 35 deletions(-) create mode 100644 modules/settings/tests/test_lenient_related_fields_validators.py 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/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/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/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/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 From 544a4bd71be6a494a67ab8730406691ea3cee99a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:09:22 +0200 Subject: [PATCH 04/12] refactor(hosting): PermissionDenied carries the missing permission (#518) RequiresPermission raises PermissionDenied(HTTPException) with .permission; the 403 page reads it via isinstance instead of slicing the message. The JSON body and page props are unchanged. PERMISSION_DENIED_PREFIX is kept (now in simple_module_core.permissions, still re-exported from simple_module_hosting.permissions) for external importers; the parser is gone. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_core/permission_denied.py | 28 +++++++++++ .../simple_module_hosting/_error_handlers.py | 31 +++--------- .../simple_module_hosting/permissions.py | 20 +++----- .../tests/test_error_page_shared_props.py | 3 +- .../tests/test_permission_denied_exception.py | 50 +++++++++++++++++++ 5 files changed, 96 insertions(+), 36 deletions(-) create mode 100644 framework/core/simple_module_core/permission_denied.py create mode 100644 framework/hosting/tests/test_permission_denied_exception.py 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/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..e0eb2a5b1 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -6,7 +6,12 @@ from typing import TYPE_CHECKING, Any from fastapi import HTTPException, Request -from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants +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 from simple_module_hosting.token_guard import GATE_PERMISSION @@ -20,6 +25,7 @@ "DEFAULT_ROLE_PERMISSIONS", "PERMISSION_DENIED_PREFIX", "WILDCARD", + "PermissionDenied", "RequiresPermission", "apply_scope_cap", "ensure_resolved_permissions", @@ -31,13 +37,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 +258,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..303929164 100644 --- a/framework/hosting/tests/test_error_page_shared_props.py +++ b/framework/hosting/tests/test_error_page_shared_props.py @@ -139,7 +139,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. """ diff --git a/framework/hosting/tests/test_permission_denied_exception.py b/framework/hosting/tests/test_permission_denied_exception.py new file mode 100644 index 000000000..20290dc9b --- /dev/null +++ b/framework/hosting/tests/test_permission_denied_exception.py @@ -0,0 +1,50 @@ +"""The 403 page names the missing permission from the exception, not its text.""" + +from __future__ import annotations + +import html +import json +import re + +import httpx +from fastapi import HTTPException +from simple_module_core.permission_denied import PermissionDenied + +_PATH = "/permission-denied-probe" + + +def _props(body: str) -> dict: + match = re.search( + r'', body, re.DOTALL + ) or re.search(r"data-page='([^']+)'", body) + assert match is not None, "error page did not render via Inertia" + return json.loads(html.unescape(match.group(1)))["props"] + + +async def test_page_names_permission_even_when_message_is_reworded( + app, authenticated_client: httpx.AsyncClient +) -> None: + async def _probe() -> None: + raise PermissionDenied("reports.export", "Vous n'avez pas le droit") + + app.add_api_route(_PATH, _probe, methods=["GET"]) + resp = await authenticated_client.get(_PATH) + assert resp.status_code == 403 + props = _props(resp.text) + assert props["required_permission"] == "reports.export" + assert props["message"] == "Vous n'avez pas le droit" + + api = await authenticated_client.get(_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( + app, authenticated_client: httpx.AsyncClient +) -> None: + async def _probe() -> None: + raise HTTPException(403, "Permission required: sneaky.perm") + + app.add_api_route(_PATH + "-plain", _probe, methods=["GET"]) + resp = await authenticated_client.get(_PATH + "-plain") + assert _props(resp.text)["required_permission"] is None From e720646304b49d62aa9f2be50f825a7060a15439 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:11:03 +0200 Subject: [PATCH 05/12] feat(tenants): publish TenantDeleted after commit (#491) Adds TenantService.delete_tenant (there was no delete path), which removes the tenant with its memberships and invitations and queues TenantDeleted (tenant_id, slug, name) on commit like TenantStatusChanged. Documented with the other tenant events. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/modules/tenants.md | 2 + modules/tenants/README.md | 2 + modules/tenants/tenants/_deletion.py | 25 ++++++++ modules/tenants/tenants/contracts/events.py | 14 +++++ modules/tenants/tenants/service.py | 5 ++ .../tests/test_tenant_deleted_event.py | 57 +++++++++++++++++++ 6 files changed, 105 insertions(+) create mode 100644 modules/tenants/tenants/_deletion.py create mode 100644 modules/tenants/tests/test_tenant_deleted_event.py 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/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..02345b9de --- /dev/null +++ b/modules/tenants/tenants/_deletion.py @@ -0,0 +1,25 @@ +"""Removing a tenant and the rows that only make sense inside it.""" + +from __future__ import annotations + +from sqlalchemy import delete +from sqlalchemy.ext.asyncio import AsyncSession + +from tenants.contracts.events import TenantDeleted +from tenants.models import Invitation, Membership, Tenant + + +async def delete_tenant_rows(db: AsyncSession, tenant: Tenant) -> TenantDeleted: + """Delete memberships, invitations and the tenant itself, then flush. + + Returns the ``TenantDeleted`` event for the caller to queue on commit. + + Done explicitly rather than leaning on ``ON DELETE CASCADE``, which only + fires where the database enforces foreign keys. + """ + for model in (Membership, Invitation): + await db.execute(delete(model).where(model.tenant_id == tenant.id)) + event = TenantDeleted(tenant.id, tenant.slug, tenant.name) + await db.delete(tenant) + await db.flush() + return event 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..9b9538627 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..a45226865 --- /dev/null +++ b/modules/tenants/tests/test_tenant_deleted_event.py @@ -0,0 +1,57 @@ +"""``TenantService.delete_tenant`` publishes ``TenantDeleted`` after commit only.""" + +from __future__ import annotations + +from simple_module_core.events import EventBus +from simple_module_db.transaction import finalize_session, rollback_session +from sqlalchemy import func, select +from tenants.contracts.events import 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 From 61bbe3ca621cd43706122519b5fb892223be38f1 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:30:25 +0200 Subject: [PATCH 06/12] test: replace fixed sleeps with a polling wait_for helper and lint new ones (#512) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .github/workflows/pr.yml | 2 + Makefile | 9 +- framework/core/tests/test_events.py | 7 +- .../testing/simple_module_test/__init__.py | 3 + .../testing/simple_module_test/waiting.py | 73 +++++++++++++++ .../testing/tests/test_waiting_helpers.py | 72 +++++++++++++++ .../tests/test_bg_invalidation.py | 4 +- .../tests/test_bg_invalidation_degraded.py | 20 +++-- .../tests/test_bg_invalidation_redis.py | 72 ++++++++------- .../tests/test_bg_outbox_eager_platform.py | 15 +--- .../tests/test_bg_outbox_loop_interrupt.py | 2 +- .../tests/test_bg_outbox_retry_policy.py | 6 +- .../tests/test_bg_outbox_swallowed_cancel.py | 2 +- .../tests/test_bg_outbox_thread_loops.py | 2 +- .../tests/test_outbox_concurrency.py | 5 +- .../tests/test_worker_inspector.py | 5 +- .../test_file_storage_thumbnail_hardening.py | 5 +- scripts/check_test_sleeps.py | 89 +++++++++++++++++++ scripts/tests/test_check_test_sleeps.py | 43 +++++++++ .../test_invalidation_cross_process.py | 4 +- 20 files changed, 375 insertions(+), 65 deletions(-) create mode 100644 framework/testing/simple_module_test/waiting.py create mode 100644 framework/testing/tests/test_waiting_helpers.py create mode 100644 scripts/check_test_sleeps.py create mode 100644 scripts/tests/test_check_test_sleeps.py diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 184371bbf..a128857f3 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -37,6 +37,8 @@ jobs: cache-dependency-glob: ${{ env.UV_CACHE_GLOB }} - run: make install-py - run: make ci-python-lint + # 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/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/framework/core/tests/test_events.py b/framework/core/tests/test_events.py index 3dfe88704..6ace31fff 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,7 @@ 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) assert len(received) == 1 assert received[0].order_id == 99 @@ -161,7 +162,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/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..059fda823 --- /dev/null +++ b/framework/testing/simple_module_test/waiting.py @@ -0,0 +1,73 @@ +"""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 value: {last!r}" + if error is not None: + detail = f"last error: {error!r}" + return AssertionError(f"wait_for({name}) not satisfied within {timeout}s; {detail}") + + +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 + last: Any = None + error: BaseException | None = None + while True: + try: + value = predicate() + if inspect.isawaitable(value): + value = await value + last, error = value, None + if value: + return value + except Exception as exc: + error = exc + if time.monotonic() >= deadline: + raise _timeout_error(predicate, timeout, last, error) + await asyncio.sleep(interval) + + +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 + last: Any = None + error: BaseException | None = None + while True: + try: + value = predicate() + last, error = value, None + if value: + return value + except Exception as exc: + error = exc + if time.monotonic() >= deadline: + raise _timeout_error(predicate, timeout, last, 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..655f1210c --- /dev/null +++ b/framework/testing/tests/test_waiting_helpers.py @@ -0,0 +1,72 @@ +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) diff --git a/modules/background_tasks/tests/test_bg_invalidation.py b/modules/background_tasks/tests/test_bg_invalidation.py index 65942fa42..36f2164ad 100644 --- a/modules/background_tasks/tests/test_bg_invalidation.py +++ b/modules/background_tasks/tests/test_bg_invalidation.py @@ -48,7 +48,9 @@ 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 if timeout else 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/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/scripts/check_test_sleeps.py b/scripts/check_test_sleeps.py new file mode 100644 index 000000000..1dc1d718e --- /dev/null +++ b/scripts/check_test_sleeps.py @@ -0,0 +1,89 @@ +"""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 re +import subprocess +import sys +from collections.abc import Iterable, Sequence +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 find_unmarked_sleeps(text: str) -> list[tuple[int, str]]: + """Return ``(line_number, stripped_line)`` for each unmarked non-zero sleep.""" + found: list[tuple[int, str]] = [] + for number, line in enumerate(text.splitlines(), 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 _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(argv: Sequence[str] | None = None) -> int: + root = Path(argv[0]) if argv else 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(sys.argv[1:])) diff --git a/scripts/tests/test_check_test_sleeps.py b/scripts/tests/test_check_test_sleeps.py new file mode 100644 index 000000000..44ee2cfa5 --- /dev/null +++ b/scripts/tests/test_check_test_sleeps.py @@ -0,0 +1,43 @@ +"""Tests for the fixed-sleep lint in test files.""" + +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +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)] 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 From 1a2906fd5d831d538a90b92ebdb2b6d5fbb55e63 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:35:44 +0200 Subject: [PATCH 07/12] perf(db): cut the tenancy scope walk's per-statement cost (#559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk that decides whether a statement holds a Core reference runs on every ORM query while tenancy names are set. It now expands fewer nodes, more cheaply, without changing which elements are reachable — so the set of statements that get scoped is unchanged: * each element is expanded once (a FROM named by columns and joins); * bound parameters, which have no clause children, are not expanded; * a SELECT's declared children skip its empty collections, and no longer repeat its FROM list or (under the ORM plugin, with an all-entity columns clause) its columns-clause FROMs, which the walk prunes anyway; * `table` is looked up without the AttributeError a column expression's __getattr__ raises and formats for it. The walk moves to `_core_walk.py`. Checked against the previous walk on every statement the db and tenants suites execute (2347 calls, no disagreement). `scripts/bench_core_scope.py` times it: an uncloned ORM select drops from 25-86 µs to 14-44 µs (get 26 -> 15, pagination page 34 -> 17, pagination count 83 -> 44). New tests pin that Core columns and subqueries inside an otherwise-ORM select (columns, WHERE, HAVING, ORDER BY, joins) stay filtered, and refuse under strict mode with no tenant. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/_core_walk.py | 164 ++++++++++++++++++ framework/db/simple_module_db/core_scope.py | 80 +-------- .../db/tests/test_db_core_in_orm_select.py | 130 ++++++++++++++ scripts/bench_core_scope.py | 105 +++++++++++ 4 files changed, 406 insertions(+), 73 deletions(-) create mode 100644 framework/db/simple_module_db/_core_walk.py create mode 100644 framework/db/tests/test_db_core_in_orm_select.py create mode 100644 scripts/bench_core_scope.py 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..2affcfd3d --- /dev/null +++ b/framework/db/simple_module_db/_core_walk.py @@ -0,0 +1,164 @@ +"""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 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 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. + 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..7dc8837fc --- /dev/null +++ b/framework/db/tests/test_db_core_in_orm_select.py @@ -0,0 +1,130 @@ +"""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 + +__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)], + ), + # 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 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/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) From 47b3ee393826962830145c0d891271aa419013c5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:36:11 +0200 Subject: [PATCH 08/12] fix(db): scope Core columns in an ORM select's WHERE (#559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing tenant leak found by the #559 coverage tests. An ORM select that reads a raw `Model.__table__` only through its WHERE — `select(Tag.name).where(link.c.tag_id == Tag.id, link.c.article_id == 7)`, also with an `aliased()` entity — pulls that table into FROM, but the scope pass took WHERE FROMs only for ORM-less selects. The table was detected, the statement cloned, and no predicate added: tenant A saw a row through tenant B's link, and strict mode with no tenant did not refuse. An ORM select now also gets WHERE predicates for the FROMs its WHERE pulls in through plain columns. ORM-annotated elements and nested selects are pruned (an entity column's `_from_objects` is its bare table, and an aliased entity's would gain a stray cartesian FROM). The #417 outer-join exclusion and the joins' covered set still apply. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/_core_walk.py | 28 +++++++++++++++++-- .../db/tests/test_db_core_in_orm_select.py | 12 ++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/framework/db/simple_module_db/_core_walk.py b/framework/db/simple_module_db/_core_walk.py index 2affcfd3d..9c69ddfc4 100644 --- a/framework/db/simple_module_db/_core_walk.py +++ b/framework/db/simple_module_db/_core_walk.py @@ -20,7 +20,7 @@ from typing import Any -from sqlalchemy.sql.elements import ColumnElement +from sqlalchemy.sql.elements import ColumnClause, ColumnElement from sqlalchemy.sql.selectable import Alias, Join, Select, TableClause from sqlalchemy.sql.traversals import _get_children @@ -48,7 +48,31 @@ def froms_read(stmt: Any) -> list[Any]: 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 + 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: diff --git a/framework/db/tests/test_db_core_in_orm_select.py b/framework/db/tests/test_db_core_in_orm_select.py index 7dc8837fc..36d71595f 100644 --- a/framework/db/tests/test_db_core_in_orm_select.py +++ b/framework/db/tests/test_db_core_in_orm_select.py @@ -14,6 +14,7 @@ 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"] @@ -41,6 +42,16 @@ def _b_article_links(): 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). @@ -114,6 +125,7 @@ async def test_core_inside_orm_select_is_scoped(seeded, shape): # 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( From a21621253bf55eb2adb005b0dd77f806322cd68b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:36:42 +0200 Subject: [PATCH 09/12] docs: changelog for #491 #512 #514 #518 #520 #558 #559 Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) 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 From 98ec11d0506f9770d241daf704ce3487c7a8c069 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:48:55 +0200 Subject: [PATCH 10/12] =?UTF-8?q?refactor(optimize):=20duplication=20?= =?UTF-8?q?=E2=80=94=20shared=20JS=20test=20fixtures,=20fold=20permission-?= =?UTF-8?q?denied=20tests,=20one=20db=20stub?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - background_tasks/tests-js: one 204-aware stubFetch in stub-fetch.ts - api_tokens/tests-js: token()/stubFetch in fixtures.ts - drop per-file afterEach cleanup() (RTL auto-cleanup, globals: true) and the jest-dom import where the module tsconfig does not typecheck tests-js - move the PermissionDenied tests into TestRequiredPermissionProp - drop duplicated tenants cache fixture and scripts sys.path insert - file_storage upload guards: one _db_on() session stub Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_error_page_shared_props.py | 33 +++++++++++- .../tests/test_permission_denied_exception.py | 50 ------------------- .../tests-js/api_tokens_Admin.test.tsx | 21 ++------ .../api_tokens_CreatedTokenDialog.test.tsx | 4 +- .../tests-js/api_tokens_Index.test.tsx | 32 +----------- modules/api_tokens/tests-js/fixtures.ts | 27 ++++++++++ .../background_tasks_DetailPage.test.tsx | 10 +--- .../background_tasks_IndexPage.test.tsx | 8 +-- .../background_tasks_OutboxPage.test.tsx | 12 +---- .../background_tasks/tests-js/outbox.test.ts | 9 +--- .../background_tasks/tests-js/stub-fetch.ts | 10 ++++ .../tests-js/feature_flags_Browse.test.tsx | 2 - .../tests/test_file_storage_upload_guards.py | 34 ++++++------- .../tests/test_tenants_shared_props.py | 8 --- scripts/tests/test_check_test_sleeps.py | 3 -- 15 files changed, 96 insertions(+), 167 deletions(-) delete mode 100644 framework/hosting/tests/test_permission_denied_exception.py create mode 100644 modules/api_tokens/tests-js/fixtures.ts create mode 100644 modules/background_tasks/tests-js/stub-fetch.ts diff --git a/framework/hosting/tests/test_error_page_shared_props.py b/framework/hosting/tests/test_error_page_shared_props.py index 303929164..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: @@ -171,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/hosting/tests/test_permission_denied_exception.py b/framework/hosting/tests/test_permission_denied_exception.py deleted file mode 100644 index 20290dc9b..000000000 --- a/framework/hosting/tests/test_permission_denied_exception.py +++ /dev/null @@ -1,50 +0,0 @@ -"""The 403 page names the missing permission from the exception, not its text.""" - -from __future__ import annotations - -import html -import json -import re - -import httpx -from fastapi import HTTPException -from simple_module_core.permission_denied import PermissionDenied - -_PATH = "/permission-denied-probe" - - -def _props(body: str) -> dict: - match = re.search( - r'', body, re.DOTALL - ) or re.search(r"data-page='([^']+)'", body) - assert match is not None, "error page did not render via Inertia" - return json.loads(html.unescape(match.group(1)))["props"] - - -async def test_page_names_permission_even_when_message_is_reworded( - app, authenticated_client: httpx.AsyncClient -) -> None: - async def _probe() -> None: - raise PermissionDenied("reports.export", "Vous n'avez pas le droit") - - app.add_api_route(_PATH, _probe, methods=["GET"]) - resp = await authenticated_client.get(_PATH) - assert resp.status_code == 403 - props = _props(resp.text) - assert props["required_permission"] == "reports.export" - assert props["message"] == "Vous n'avez pas le droit" - - api = await authenticated_client.get(_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( - app, authenticated_client: httpx.AsyncClient -) -> None: - async def _probe() -> None: - raise HTTPException(403, "Permission required: sneaky.perm") - - app.add_api_route(_PATH + "-plain", _probe, methods=["GET"]) - resp = await authenticated_client.get(_PATH + "-plain") - assert _props(resp.text)["required_permission"] is None diff --git a/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx b/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx index fc9b80b10..4d3a69256 100644 --- a/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx +++ b/modules/api_tokens/tests-js/api_tokens_Admin.test.tsx @@ -1,6 +1,5 @@ -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 { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; configureI18n({ locale: 'en', messages: {} }); @@ -20,25 +19,20 @@ vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); import { toast } from 'sonner'; import Admin from '../api_tokens/pages/Admin'; -import type { ApiToken } from '../api_tokens/pages/types'; +import { stubFetch, token } from './fixtures'; const SECRET = 'smpy_svc_adminsecret'; -const SERVICE: ApiToken = { +const SERVICE = token({ id: 's1', kind: 'service', name: 'ingest-bot', prefix: 'smpy_zz99', - scopes: ['files.read'], roles: ['uploader'], owner_user_id: null, owner_email: null, tenant_id: 'acme', - created_at: '2026-01-01T00:00:00Z', - expires_at: '2999-01-01T00:00:00Z', - last_used_at: null, - revoked_at: null, -}; +}); function setProps(over: Record = {}) { state.props = { @@ -54,16 +48,9 @@ function setProps(over: Record = {}) { }; } -function stubFetch(status: number, body: unknown) { - const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); - vi.stubGlobal('fetch', fetchMock); - return fetchMock; -} - describe('api_tokens Admin', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); }); diff --git a/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx b/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx index 9cd6391a2..30879b1c3 100644 --- a/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx +++ b/modules/api_tokens/tests-js/api_tokens_CreatedTokenDialog.test.tsx @@ -1,6 +1,5 @@ -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 { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; configureI18n({ locale: 'en', messages: {} }); @@ -17,7 +16,6 @@ function stubClipboard(writeText: ReturnType) { describe('CreatedTokenDialog', () => { beforeEach(() => stubClipboard(vi.fn().mockResolvedValue(undefined))); afterEach(() => { - cleanup(); vi.restoreAllMocks(); }); diff --git a/modules/api_tokens/tests-js/api_tokens_Index.test.tsx b/modules/api_tokens/tests-js/api_tokens_Index.test.tsx index 0f4651418..21a9fc05c 100644 --- a/modules/api_tokens/tests-js/api_tokens_Index.test.tsx +++ b/modules/api_tokens/tests-js/api_tokens_Index.test.tsx @@ -1,6 +1,5 @@ -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 { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; configureI18n({ locale: 'en', messages: {} }); @@ -20,29 +19,9 @@ vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })); import { toast } from 'sonner'; import Index from '../api_tokens/pages/Index'; -import type { ApiToken } from '../api_tokens/pages/types'; +import { stubFetch, token } from './fixtures'; const SECRET = 'smpy_pat_topsecretvalue'; -const FUTURE = '2999-01-01T00:00:00Z'; - -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: FUTURE, - last_used_at: null, - revoked_at: null, - ...over, - }; -} function setProps(over: Record = {}) { state.props = { @@ -56,12 +35,6 @@ function setProps(over: Record = {}) { }; } -function stubFetch(status: number, body: unknown) { - const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); - vi.stubGlobal('fetch', fetchMock); - return fetchMock; -} - const f = keys.api_tokens.form; const rv = keys.api_tokens.revoke; const ra = keys.api_tokens.revoke_all; @@ -76,7 +49,6 @@ async function createToken() { describe('api_tokens Index', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); }); 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/tests-js/background_tasks_DetailPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx index 439073239..63609a313 100644 --- a/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx +++ b/modules/background_tasks/tests-js/background_tasks_DetailPage.test.tsx @@ -1,6 +1,6 @@ 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 { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; configureI18n({ locale: 'en', messages: {} }); @@ -21,6 +21,7 @@ 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; @@ -52,16 +53,9 @@ function setProps(over: Partial = {}, permissions = ['background_tas state.props = { execution: execution(over), max_retries: 3, auth: { permissions } }; } -function stubFetch(status: number, body: unknown) { - const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); - vi.stubGlobal('fetch', fetchMock); - return fetchMock; -} - describe('background_tasks Detail page', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); }); diff --git a/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx index 763fb8b38..34ae50357 100644 --- a/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx +++ b/modules/background_tasks/tests-js/background_tasks_IndexPage.test.tsx @@ -21,6 +21,7 @@ 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; @@ -63,16 +64,9 @@ function setProps(over: Record = {}, permissions = ['background }; } -function stubFetch(status: number, body: unknown) { - const fetchMock = vi.fn(async () => new Response(JSON.stringify(body), { status })); - vi.stubGlobal('fetch', fetchMock); - return fetchMock; -} - describe('background_tasks Index page', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); }); diff --git a/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx index 069b7b648..b719a2011 100644 --- a/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx +++ b/modules/background_tasks/tests-js/background_tasks_OutboxPage.test.tsx @@ -1,6 +1,6 @@ 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 { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; configureI18n({ locale: 'en', messages: {} }); @@ -22,6 +22,7 @@ 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; @@ -54,18 +55,9 @@ function setProps(over: Record = {}, permissions = ['background }; } -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; -} - describe('background_tasks Outbox page', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.unstubAllGlobals(); vi.restoreAllMocks(); vi.clearAllMocks(); 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/feature_flags/tests-js/feature_flags_Browse.test.tsx b/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx index 69382c23c..ce00627ce 100644 --- a/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx +++ b/modules/feature_flags/tests-js/feature_flags_Browse.test.tsx @@ -1,4 +1,3 @@ -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'; @@ -62,7 +61,6 @@ const browse = keys.feature_flags.browse; describe('feature_flags Browse', () => { beforeEach(() => setProps()); afterEach(() => { - cleanup(); vi.clearAllMocks(); }); diff --git a/modules/file_storage/tests/test_file_storage_upload_guards.py b/modules/file_storage/tests/test_file_storage_upload_guards.py index c1e76e253..5b844a686 100644 --- a/modules/file_storage/tests/test_file_storage_upload_guards.py +++ b/modules/file_storage/tests/test_file_storage_upload_guards.py @@ -243,34 +243,28 @@ async def fake_lock() -> None: assert order == ["lock"] -class TestSerializeReservations: - async def test_is_a_no_op_off_postgres(self, backend, db_session): - executed: list[object] = [] +def _db_on(dialect: str, calls: list[tuple[str, Any]]) -> SimpleNamespace: + """A session stub on ``dialect`` that records every statement it executes.""" - class _Db: - def get_bind(self): - return SimpleNamespace(dialect=SimpleNamespace(name="sqlite")) + async def execute(statement: object, params: Any = None) -> None: + calls.append((str(statement), params)) - async def execute(self, *args: object, **kwargs: object) -> None: - executed.append(args) + bind = SimpleNamespace(dialect=SimpleNamespace(name=dialect)) + return SimpleNamespace(get_bind=lambda: bind, execute=execute) - await _Guards(_Db(), backend)._serialize_reservations() + +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, dict]] = [] - - class _Db: - def get_bind(self): - return SimpleNamespace(dialect=SimpleNamespace(name="postgresql")) - - async def execute(self, statement: object, params: dict) -> None: - calls.append((str(statement), params)) - + calls: list[tuple[str, Any]] = [] monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "acme") - await _Guards(_Db(), backend)._serialize_reservations() + await _Guards(_db_on("postgresql", calls), backend)._serialize_reservations() monkeypatch.setattr(guards_module, "owning_tenant", lambda db: "globex") - await _Guards(_Db(), backend)._serialize_reservations() + 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"] diff --git a/modules/tenants/tests/test_tenants_shared_props.py b/modules/tenants/tests/test_tenants_shared_props.py index e9c140afd..dbbe3c2c1 100644 --- a/modules/tenants/tests/test_tenants_shared_props.py +++ b/modules/tenants/tests/test_tenants_shared_props.py @@ -6,7 +6,6 @@ from types import SimpleNamespace from typing import Any, cast -import pytest from starlette.requests import Request from tenants import resolver from tenants.constants import TenantStatus @@ -32,13 +31,6 @@ def _request(**state: Any) -> Request: return cast("Request", SimpleNamespace(state=SimpleNamespace(**state))) -@pytest.fixture(autouse=True) -def _clean_cache(): - resolver._MEMBERSHIPS.clear() - yield - resolver._MEMBERSHIPS.clear() - - def test_anonymous_request_contributes_nothing(): assert tenant_shared_props(_request()) == {} assert tenant_shared_props(_request(user=None)) == {} diff --git a/scripts/tests/test_check_test_sleeps.py b/scripts/tests/test_check_test_sleeps.py index 44ee2cfa5..9b1ef7b64 100644 --- a/scripts/tests/test_check_test_sleeps.py +++ b/scripts/tests/test_check_test_sleeps.py @@ -2,11 +2,8 @@ from __future__ import annotations -import sys from pathlib import Path -sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) - from check_test_sleeps import check, find_unmarked_sleeps, is_test_file From 454666b7554d4b46840dd68d067ef7880bc4a43f Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 16:50:41 +0200 Subject: [PATCH 11/12] =?UTF-8?q?refactor(optimize):=20over-engineering=20?= =?UTF-8?q?=E2=80=94=20shared=20wait=5Ffor=20poll=20step,=20simpler=20slee?= =?UTF-8?q?p=20lint=20entry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - permissions.py: one-line simple_module_core.permissions import - waiting.py: single-expression detail; wait_for/wait_for_sync share _poll_once - check_test_sleeps.py: drop the unused argv root option, use Path.cwd() - test_bg_invalidation: asyncio.sleep(timeout or 0) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../simple_module_hosting/permissions.py | 6 +-- .../testing/simple_module_test/waiting.py | 52 ++++++++++--------- .../tests/test_bg_invalidation.py | 4 +- scripts/check_test_sleeps.py | 8 +-- 4 files changed, 33 insertions(+), 37 deletions(-) diff --git a/framework/hosting/simple_module_hosting/permissions.py b/framework/hosting/simple_module_hosting/permissions.py index e0eb2a5b1..208ce275e 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -7,11 +7,7 @@ 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_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants from simple_module_hosting._anonymous import demote_to_anonymous from simple_module_hosting.token_guard import GATE_PERMISSION diff --git a/framework/testing/simple_module_test/waiting.py b/framework/testing/simple_module_test/waiting.py index 059fda823..3b0e52e3f 100644 --- a/framework/testing/simple_module_test/waiting.py +++ b/framework/testing/simple_module_test/waiting.py @@ -16,12 +16,18 @@ def _timeout_error( predicate: Callable[..., Any], timeout: float, last: Any, error: BaseException | None ) -> AssertionError: name = getattr(predicate, "__name__", repr(predicate)) - detail = f"last value: {last!r}" - if error is not None: - detail = f"last error: {error!r}" + 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, @@ -34,23 +40,25 @@ async def wait_for( last thing seen. """ deadline = time.monotonic() + timeout - last: Any = None - error: BaseException | None = None while True: - try: - value = predicate() - if inspect.isawaitable(value): - value = await value - last, error = value, None - if value: - return value - except Exception as exc: - error = exc + value, error = _poll_once(predicate) + if inspect.isawaitable(value): + value, error = await _settle(value) + if value: + return value if time.monotonic() >= deadline: - raise _timeout_error(predicate, timeout, last, error) + raise _timeout_error(predicate, timeout, value, error) await asyncio.sleep(interval) +async def _settle(awaitable: Awaitable[Any]) -> tuple[Any, BaseException | None]: + """Await an async predicate's result, with :func:`_poll_once`'s contract.""" + try: + return await awaitable, None + except Exception as exc: + return None, exc + + def wait_for_sync( predicate: Callable[[], Any], timeout: float = 2.0, @@ -58,16 +66,10 @@ def wait_for_sync( ) -> Any: """Blocking twin of :func:`wait_for` for sync tests (sync predicates only).""" deadline = time.monotonic() + timeout - last: Any = None - error: BaseException | None = None while True: - try: - value = predicate() - last, error = value, None - if value: - return value - except Exception as exc: - error = exc + value, error = _poll_once(predicate) + if value: + return value if time.monotonic() >= deadline: - raise _timeout_error(predicate, timeout, last, error) + raise _timeout_error(predicate, timeout, value, error) time.sleep(interval) # intentional-sleep: the polling interval itself diff --git a/modules/background_tasks/tests/test_bg_invalidation.py b/modules/background_tasks/tests/test_bg_invalidation.py index 36f2164ad..8ef48c3a6 100644 --- a/modules/background_tasks/tests/test_bg_invalidation.py +++ b/modules/background_tasks/tests/test_bg_invalidation.py @@ -48,9 +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 - ) # intentional-sleep: fake idle-channel poll interval + await asyncio.sleep(timeout or 0) # intentional-sleep: fake idle-channel poll interval return None async def aclose(self) -> None: diff --git a/scripts/check_test_sleeps.py b/scripts/check_test_sleeps.py index 1dc1d718e..e00161e5e 100644 --- a/scripts/check_test_sleeps.py +++ b/scripts/check_test_sleeps.py @@ -20,7 +20,7 @@ import re import subprocess import sys -from collections.abc import Iterable, Sequence +from collections.abc import Iterable from pathlib import Path MARKER = "# intentional-sleep:" @@ -72,8 +72,8 @@ def check(root: Path, paths: Iterable[Path]) -> list[tuple[Path, int, str]]: return violations -def main(argv: Sequence[str] | None = None) -> int: - root = Path(argv[0]) if argv else Path.cwd() +def main() -> int: + root = Path.cwd() violations = check(root, _list_git_files(root)) if not violations: print("OK: no unmarked fixed sleeps in tests.") @@ -86,4 +86,4 @@ def main(argv: Sequence[str] | None = None) -> int: if __name__ == "__main__": - sys.exit(main(sys.argv[1:])) + sys.exit(main()) From 1d3b6b77d325ef5ff5ac085199495a6ceec35345 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 19 Oct 2026 17:03:09 +0200 Subject: [PATCH 12/12] fix: address code review findings (round 1, pass 1) - tenants: delete_tenant now publishes MembershipRemoved for every member (collected before the bulk delete) alongside TenantDeleted, after commit only; the existing global membership-cache invalidation still runs post- commit, and a rollback publishes and invalidates nothing. Tests cover both. - core: test_publish_nowait yields to the loop after the wait so a duplicate delivery would be caught by the exactly-one assertion. - testing: wait_for bounds an async predicate's await by the remaining timeout; wait_for_sync rejects async predicates. - scripts: check_test_sleeps parses with ast, so a sleep split over lines or after a '#' inside a string is still flagged (regex fallback on bad syntax). Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/core/tests/test_events.py | 2 + .../testing/simple_module_test/waiting.py | 16 +++- .../testing/tests/test_waiting_helpers.py | 16 ++++ modules/tenants/tenants/_deletion.py | 23 ++++-- modules/tenants/tenants/service.py | 2 +- .../tests/test_tenant_deleted_event.py | 73 ++++++++++++++++++- scripts/check_test_sleeps.py | 55 +++++++++++++- scripts/tests/test_check_test_sleeps.py | 10 +++ 8 files changed, 181 insertions(+), 16 deletions(-) diff --git a/framework/core/tests/test_events.py b/framework/core/tests/test_events.py index 6ace31fff..342c9f36f 100644 --- a/framework/core/tests/test_events.py +++ b/framework/core/tests/test_events.py @@ -109,6 +109,8 @@ async def handler(e): bus.subscribe(OrderCreated, handler) bus.publish_nowait(OrderCreated(order_id=99)) 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 diff --git a/framework/testing/simple_module_test/waiting.py b/framework/testing/simple_module_test/waiting.py index 3b0e52e3f..9b9ff590e 100644 --- a/framework/testing/simple_module_test/waiting.py +++ b/framework/testing/simple_module_test/waiting.py @@ -43,7 +43,7 @@ async def wait_for( while True: value, error = _poll_once(predicate) if inspect.isawaitable(value): - value, error = await _settle(value) + value, error = await _settle(value, max(deadline - time.monotonic(), 0.001)) if value: return value if time.monotonic() >= deadline: @@ -51,10 +51,14 @@ async def wait_for( await asyncio.sleep(interval) -async def _settle(awaitable: Awaitable[Any]) -> tuple[Any, BaseException | None]: - """Await an async predicate's result, with :func:`_poll_once`'s contract.""" +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 awaitable, None + return await asyncio.wait_for(awaitable, budget), None except Exception as exc: return None, exc @@ -68,6 +72,10 @@ def wait_for_sync( 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: diff --git a/framework/testing/tests/test_waiting_helpers.py b/framework/testing/tests/test_waiting_helpers.py index 655f1210c..17e2e3937 100644 --- a/framework/testing/tests/test_waiting_helpers.py +++ b/framework/testing/tests/test_waiting_helpers.py @@ -70,3 +70,19 @@ def pred(): 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/tenants/tenants/_deletion.py b/modules/tenants/tenants/_deletion.py index 02345b9de..ed4718def 100644 --- a/modules/tenants/tenants/_deletion.py +++ b/modules/tenants/tenants/_deletion.py @@ -2,24 +2,35 @@ from __future__ import annotations -from sqlalchemy import delete +from simple_module_core.events import Event +from sqlalchemy import delete, select from sqlalchemy.ext.asyncio import AsyncSession -from tenants.contracts.events import TenantDeleted +from tenants.contracts.events import MembershipRemoved, TenantDeleted from tenants.models import Invitation, Membership, Tenant -async def delete_tenant_rows(db: AsyncSession, tenant: Tenant) -> TenantDeleted: +async def delete_tenant_rows(db: AsyncSession, tenant: Tenant) -> list[Event]: """Delete memberships, invitations and the tenant itself, then flush. - Returns the ``TenantDeleted`` event for the caller to queue on commit. + 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)) - event = TenantDeleted(tenant.id, tenant.slug, tenant.name) + 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 event + return events diff --git a/modules/tenants/tenants/service.py b/modules/tenants/tenants/service.py index 9b9538627..5b1f9dd10 100644 --- a/modules/tenants/tenants/service.py +++ b/modules/tenants/tenants/service.py @@ -146,7 +146,7 @@ async def set_status(self, tenant_id: str, status: TenantStatus) -> 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]) + 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) diff --git a/modules/tenants/tests/test_tenant_deleted_event.py b/modules/tenants/tests/test_tenant_deleted_event.py index a45226865..cbaff683f 100644 --- a/modules/tenants/tests/test_tenant_deleted_event.py +++ b/modules/tenants/tests/test_tenant_deleted_event.py @@ -1,11 +1,14 @@ -"""``TenantService.delete_tenant`` publishes ``TenantDeleted`` after commit only.""" +"""``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.contracts.events import TenantDeleted +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 @@ -55,3 +58,69 @@ async def handler(event: TenantDeleted) -> None: 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/scripts/check_test_sleeps.py b/scripts/check_test_sleeps.py index e00161e5e..15704fa14 100644 --- a/scripts/check_test_sleeps.py +++ b/scripts/check_test_sleeps.py @@ -17,6 +17,7 @@ from __future__ import annotations +import ast import re import subprocess import sys @@ -37,10 +38,43 @@ def is_test_file(path: Path) -> bool: return path.name.startswith("test_") or path.name == "conftest.py" or "tests" in path.parts -def find_unmarked_sleeps(text: str) -> list[tuple[int, str]]: - """Return ``(line_number, stripped_line)`` for each unmarked non-zero sleep.""" +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 number, line in enumerate(text.splitlines(), start=1): + 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] @@ -51,6 +85,21 @@ def find_unmarked_sleeps(text: str) -> list[tuple[int, str]]: 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"], diff --git a/scripts/tests/test_check_test_sleeps.py b/scripts/tests/test_check_test_sleeps.py index 9b1ef7b64..4d8eea6c0 100644 --- a/scripts/tests/test_check_test_sleeps.py +++ b/scripts/tests/test_check_test_sleeps.py @@ -38,3 +38,13 @@ def test_check_reports_only_test_files(tmp_path): (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) == []