forgejo_admin/basketball-api

Fork 1

infra: migration 048 — GRANT tournament tables to westside_streamlit_ro #515

Open

forgejo_admin wants to merge 1 commit from 510-grant-tournament-tables-to-ro-role into main

forgejo_admin commented

2026-04-25 13:55:13 +00:00

Owner

Summary

Adds migration 048 extending the westside_streamlit_ro allowlist (created in 044) to include tournaments and tournament_products. Enables the upcoming Streamlit tournament section (forgejo_admin/westside-streamlit#15) to query tournament data joined through the already-granted orders table. ACL only — no schema changes.

Closes #510

Changes

alembic/versions/048_grant_tournament_tables_to_ro_role.py — new migration. revision="048", down_revision="047". upgrade() GRANTs SELECT on tournaments and tournament_products to westside_streamlit_ro; downgrade() REVOKEs.

Style mirrors migration 044: per-table loop with TABLE public.{table} qualifier, ROLE_NAME + GRANT_TABLES constants. registrations intentionally NOT included — tournament payments write to orders (already granted in 044), not registrations.

Test Plan

Round-trip verified locally against a Postgres DB stamped to 047 with the role + tournament tables seeded:

$ alembic upgrade head
INFO  [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro.

$ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';"
        grantee        |     table_name      | privilege_type
-----------------------+---------------------+----------------
 westside_streamlit_ro | tournament_products | SELECT
 westside_streamlit_ro | tournaments         | SELECT

$ alembic downgrade 047
INFO  [alembic.runtime.migration] Running downgrade 048 -> 047, GRANT SELECT on tournament tables to westside_streamlit_ro.

$ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';"
 grantee | table_name | privilege_type
---------+------------+----------------
(0 rows)

$ alembic upgrade head   # idempotent re-apply
INFO  [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro.
# GRANTs reapplied

ruff check alembic/versions/048_grant_tournament_tables_to_ro_role.py — clean
ruff format --check — clean
pytest tests/ — 474 pass; 1 pre-existing failure (test_jersey_reminder.py::test_plain_text_has_deadline_copy asserts "March 30" but copy now reads "April 10") confirmed to also fail on main and is unrelated to this migration

Review Checklist

Revision is exactly "048", down_revision is exactly "047" (no slot collision)
Role name westside_streamlit_ro matches migration 044
upgrade() GRANTs SELECT on tournaments and tournament_products only — registrations intentionally excluded
downgrade() REVOKEs SELECT on the same two tables
Migration style/header matches 044 (per-table loop with TABLE public.{table} qualifier)
No schema changes; ACL only
No changes to src/basketball_api/models.py
ruff check clean
ruff format --check clean
Round-trip (upgrade head / downgrade 047 / upgrade head) verified locally
Existing test suite still passes (modulo the unrelated pre-existing test_jersey_reminder failure on main)

Notes / Surprises

A clean alembic upgrade head from an empty DB hits a pre-existing issue at migration 029_add_schedule_tables (type "division" already exists on the second Enum(..., create_type=False) usage in the same migration with current SQLAlchemy 2.0.48). This is unrelated to migration 048 and is reproducible on main. Round-trip for 048 was therefore validated against a DB stamped at 047 with the role and tournament tables seeded — verifying the actual SQL emitted by 048 works correctly. Worth filing a separate ticket for the 029 enum issue.
Alembic head was 047 at branch time — slot 048 was not contended.

Forgejo issue: #510
Review verdict note: review-1072-2026-04-22 (APPROVED)
project-westside-streamlit — consumer project
story-westside-streamlit-tournament — user story this serves
feedback_migration_slot_coordination — slot-collision rule
feedback_basketball_hands_off — narrow-allowlist rationale
Migration 044_add_westside_streamlit_ro_role — defines the role being extended
Migration 045_add_tournament_tables — source of the tables being GRANTed
Consumer ticket: forgejo_admin/westside-streamlit#15 (C3 — Tournament section)

## Summary Adds migration `048` extending the `westside_streamlit_ro` allowlist (created in `044`) to include `tournaments` and `tournament_products`. Enables the upcoming Streamlit tournament section (`forgejo_admin/westside-streamlit#15`) to query tournament data joined through the already-granted `orders` table. ACL only — no schema changes. Closes #510 ## Changes - `alembic/versions/048_grant_tournament_tables_to_ro_role.py` — new migration. `revision="048"`, `down_revision="047"`. `upgrade()` GRANTs SELECT on `tournaments` and `tournament_products` to `westside_streamlit_ro`; `downgrade()` REVOKEs. Style mirrors migration `044`: per-table loop with `TABLE public.{table}` qualifier, `ROLE_NAME` + `GRANT_TABLES` constants. `registrations` intentionally NOT included — tournament payments write to `orders` (already granted in 044), not `registrations`. ## Test Plan Round-trip verified locally against a Postgres DB stamped to `047` with the role + tournament tables seeded: ``` $ alembic upgrade head INFO [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro. $ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';" grantee | table_name | privilege_type -----------------------+---------------------+---------------- westside_streamlit_ro | tournament_products | SELECT westside_streamlit_ro | tournaments | SELECT $ alembic downgrade 047 INFO [alembic.runtime.migration] Running downgrade 048 -> 047, GRANT SELECT on tournament tables to westside_streamlit_ro. $ psql -c "SELECT grantee, table_name, privilege_type FROM information_schema.role_table_grants WHERE grantee = 'westside_streamlit_ro';" grantee | table_name | privilege_type ---------+------------+---------------- (0 rows) $ alembic upgrade head # idempotent re-apply INFO [alembic.runtime.migration] Running upgrade 047 -> 048, GRANT SELECT on tournament tables to westside_streamlit_ro. # GRANTs reapplied ``` - `ruff check alembic/versions/048_grant_tournament_tables_to_ro_role.py` — clean - `ruff format --check` — clean - `pytest tests/` — 474 pass; 1 pre-existing failure (`test_jersey_reminder.py::test_plain_text_has_deadline_copy` asserts "March 30" but copy now reads "April 10") confirmed to also fail on `main` and is unrelated to this migration ## Review Checklist - [x] Revision is exactly `"048"`, down_revision is exactly `"047"` (no slot collision) - [x] Role name `westside_streamlit_ro` matches migration 044 - [x] `upgrade()` GRANTs SELECT on `tournaments` and `tournament_products` only — `registrations` intentionally excluded - [x] `downgrade()` REVOKEs SELECT on the same two tables - [x] Migration style/header matches 044 (per-table loop with `TABLE public.{table}` qualifier) - [x] No schema changes; ACL only - [x] No changes to `src/basketball_api/models.py` - [x] `ruff check` clean - [x] `ruff format --check` clean - [x] Round-trip (`upgrade head` / `downgrade 047` / `upgrade head`) verified locally - [x] Existing test suite still passes (modulo the unrelated pre-existing `test_jersey_reminder` failure on `main`) ## Notes / Surprises - A clean `alembic upgrade head` from an empty DB hits a pre-existing issue at migration `029_add_schedule_tables` (`type "division" already exists` on the second `Enum(..., create_type=False)` usage in the same migration with current SQLAlchemy 2.0.48). This is unrelated to migration 048 and is reproducible on `main`. Round-trip for 048 was therefore validated against a DB stamped at `047` with the role and tournament tables seeded — verifying the actual SQL emitted by 048 works correctly. Worth filing a separate ticket for the 029 enum issue. - Alembic head was 047 at branch time — slot 048 was not contended. ## Related Notes - Forgejo issue: #510 - Review verdict note: `review-1072-2026-04-22` (APPROVED) - `project-westside-streamlit` — consumer project - `story-westside-streamlit-tournament` — user story this serves - `feedback_migration_slot_coordination` — slot-collision rule - `feedback_basketball_hands_off` — narrow-allowlist rationale - Migration `044_add_westside_streamlit_ro_role` — defines the role being extended - Migration `045_add_tournament_tables` — source of the tables being GRANTed - Consumer ticket: `forgejo_admin/westside-streamlit#15` (C3 — Tournament section)

forgejo_admin added 1 commit

2026-04-25 13:55:13 +00:00

infra: migration 048 - GRANT tournament tables to westside_streamlit_ro

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

65199be6a0

Extends migration 044's allowlist for the westside_streamlit_ro role to
include `tournaments` and `tournament_products` (added in migration 045).
Tournament payments flow through the existing `orders` table, which is
already granted in 044, so only these two tables need to be added.
`registrations` is intentionally NOT granted: it is for tryouts/practices
and tournament checkout writes to `orders`, not `registrations`.

ACL only - no schema changes. Defense-in-depth principle from 044 holds:
new tables stay denied by default; this extends the allowlist deliberately
and minimally.

Closes #510

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-04-25 13:58:09 +00:00

Author

Owner

PR #515 Review

DOMAIN REVIEW

Stack: Python / Alembic / Postgres ACL. ACL-only migration, no schema or model changes.

Migration slot coordination (feedback_migration_slot_coordination)

revision = "048", down_revision = "047" — slot correct.
PR body confirms head was 047 at branch time, no contention. Single migration, no parallel-agent collision risk.
File name 048_grant_tournament_tables_to_ro_role.py matches the {slot}_{snake_case}.py convention used by 044/045.

Style match with migration 044

Per-table loop with TABLE public.{table} qualifier — matches 044 exactly.
ROLE_NAME and GRANT_TABLES constants — same pattern as 044.
Module docstring documents revision/down_revision/create_date in the same shape as 044.
One stylistic divergence (in the right direction): 044's downgrade() uses REVOKE ALL PRIVILEGES (because it also drops the role); 048 uses targeted REVOKE SELECT because it must NOT touch the grants 044 owns. This is the correct narrowing — a REVOKE ALL here would clobber 044's allowlist on tournaments/tournament_products if those tables had been added to 044 retroactively. Targeted REVOKE is safer and idempotent.

Allowlist correctness (deliberate registrations exclusion)

GRANT_TABLES = ("tournaments", "tournament_products") — exactly what westside-streamlit#15 needs for the join path.
Docstring explicitly documents why registrations is NOT included: tournament checkout writes to orders (already granted in 044), registrations is for tryouts/practices. Correct per feedback_basketball_hands_off defense-in-depth: extend allowlist deliberately and minimally.
Verified against migration 045: tournament_products.product_id -> products.id (granted in 044), tournament_products.tournament_id -> tournaments.id (granted here). Join path closes without registrations.

Round-trip evidence

PR body shows three-step round-trip: upgrade head -> grants present, downgrade 047 -> grants removed (0 rows), upgrade head -> grants re-applied. This is the right test for an ACL migration.
information_schema.role_table_grants is the correct source-of-truth view to verify GRANTs. Output shows exactly the two expected rows and nothing else.
Re-upgrade is idempotent because GRANT SELECT on an already-granted privilege is a no-op in Postgres (no IF NOT EXISTS gymnastics needed).

Code quality (generic checklist)

f-string interpolation of {table} and {ROLE_NAME} into raw SQL — both are module-local constants under code review, not user input. No injection risk. Acceptable here for the same reason 044 does it.
No secrets in diff. No env var reads (correctly — role already exists from 044, password not needed).
No magic numbers, no hardcoded URLs, no swallowed errors.
ruff check / ruff format clean per PR body.

BLOCKERS

None.

NITS

None blocking. One observation for the record: the upgrade log message text ("GRANT SELECT on tournament tables to westside_streamlit_ro.") is supplied by Alembic from the docstring's first line, which matches. No action.

SOP COMPLIANCE

Branch named 510-grant-tournament-tables-to-ro-role — matches {issue}-{kebab-case} convention
PR body has Summary / Changes / Test Plan / Notes / Related — full template
Related references parent issue #510, consumer ticket westside-streamlit#15, conventions (feedback_migration_slot_coordination, feedback_basketball_hands_off), and predecessor migrations 044/045
Round-trip test evidence inline in Test Plan
No secrets, .env files, or credentials committed
Single-file change, no scope creep
Closes #510

PROCESS OBSERVATIONS

DORA notes:

Deployment frequency: low-risk ACL migration, single file, 48 LOC. Fast-merge candidate once approved.
Change failure risk: minimal. Round-trip verified, downgrade is targeted REVOKE (won't clobber sibling grants), no schema mutation, no model coupling.
Lead time: cleanly scoped — one ticket, one PR, one slot. Mirrors 044 architecture so reviewers can pattern-match instantly.

Discovered scope (do NOT block this PR — flag for separate tickets per feedback_discovered_scope_always_tracked):

Migration 029 enum bug — clean alembic upgrade head from empty DB fails at 029_add_schedule_tables with type "division" already exists. Reproducible on main. Confirmed in source: lines 34 and 77 both reference Enum("boys", "girls", name="division", create_type=False) without an explicit division.create() call, so SQLAlchemy 2.0.48 emits the type creation on the first hit and again on the second. Should be a separate Forgejo issue.
tests/test_jersey_reminder.py::test_plain_text_has_deadline_copy — asserts "March 30" deadline copy, but template now reads "April 10". Pre-existing failure on main, unrelated to this migration. Tracking ticket may already exist (#491 mentions "two pre-existing test failures keep CI red on main"); worth checking before filing a new one.

Neither blocks #515. Both warrant their own board items.

VERDICT: APPROVED

## PR #515 Review ### DOMAIN REVIEW Stack: Python / Alembic / Postgres ACL. ACL-only migration, no schema or model changes. **Migration slot coordination (`feedback_migration_slot_coordination`)** - `revision = "048"`, `down_revision = "047"` — slot correct. - PR body confirms head was `047` at branch time, no contention. Single migration, no parallel-agent collision risk. - File name `048_grant_tournament_tables_to_ro_role.py` matches the `{slot}_{snake_case}.py` convention used by 044/045. **Style match with migration 044** - Per-table loop with `TABLE public.{table}` qualifier — matches 044 exactly. - `ROLE_NAME` and `GRANT_TABLES` constants — same pattern as 044. - Module docstring documents revision/down_revision/create_date in the same shape as 044. - One stylistic divergence (in the right direction): 044's `downgrade()` uses `REVOKE ALL PRIVILEGES` (because it also drops the role); 048 uses targeted `REVOKE SELECT` because it must NOT touch the grants 044 owns. This is the correct narrowing — a `REVOKE ALL` here would clobber 044's allowlist on `tournaments`/`tournament_products` if those tables had been added to 044 retroactively. Targeted REVOKE is safer and idempotent. **Allowlist correctness (deliberate `registrations` exclusion)** - `GRANT_TABLES = ("tournaments", "tournament_products")` — exactly what `westside-streamlit#15` needs for the join path. - Docstring explicitly documents why `registrations` is NOT included: tournament checkout writes to `orders` (already granted in 044), `registrations` is for tryouts/practices. Correct per `feedback_basketball_hands_off` defense-in-depth: extend allowlist deliberately and minimally. - Verified against migration 045: `tournament_products.product_id` -> `products.id` (granted in 044), `tournament_products.tournament_id` -> `tournaments.id` (granted here). Join path closes without `registrations`. **Round-trip evidence** - PR body shows three-step round-trip: `upgrade head` -> grants present, `downgrade 047` -> grants removed (0 rows), `upgrade head` -> grants re-applied. This is the right test for an ACL migration. - `information_schema.role_table_grants` is the correct source-of-truth view to verify GRANTs. Output shows exactly the two expected rows and nothing else. - Re-upgrade is idempotent because `GRANT SELECT` on an already-granted privilege is a no-op in Postgres (no `IF NOT EXISTS` gymnastics needed). **Code quality (generic checklist)** - f-string interpolation of `{table}` and `{ROLE_NAME}` into raw SQL — both are module-local constants under code review, not user input. No injection risk. Acceptable here for the same reason 044 does it. - No secrets in diff. No env var reads (correctly — role already exists from 044, password not needed). - No magic numbers, no hardcoded URLs, no swallowed errors. - ruff check / ruff format clean per PR body. ### BLOCKERS None. ### NITS None blocking. One observation for the record: the upgrade log message text (`"GRANT SELECT on tournament tables to westside_streamlit_ro."`) is supplied by Alembic from the docstring's first line, which matches. No action. ### SOP COMPLIANCE - [x] Branch named `510-grant-tournament-tables-to-ro-role` — matches `{issue}-{kebab-case}` convention - [x] PR body has Summary / Changes / Test Plan / Notes / Related — full template - [x] Related references parent issue #510, consumer ticket `westside-streamlit#15`, conventions (`feedback_migration_slot_coordination`, `feedback_basketball_hands_off`), and predecessor migrations 044/045 - [x] Round-trip test evidence inline in Test Plan - [x] No secrets, .env files, or credentials committed - [x] Single-file change, no scope creep - [x] Closes #510 ### PROCESS OBSERVATIONS DORA notes: - Deployment frequency: low-risk ACL migration, single file, 48 LOC. Fast-merge candidate once approved. - Change failure risk: minimal. Round-trip verified, downgrade is targeted REVOKE (won't clobber sibling grants), no schema mutation, no model coupling. - Lead time: cleanly scoped — one ticket, one PR, one slot. Mirrors 044 architecture so reviewers can pattern-match instantly. Discovered scope (do NOT block this PR — flag for separate tickets per `feedback_discovered_scope_always_tracked`): 1. **Migration 029 enum bug** — clean `alembic upgrade head` from empty DB fails at `029_add_schedule_tables` with `type "division" already exists`. Reproducible on `main`. Confirmed in source: lines 34 and 77 both reference `Enum("boys", "girls", name="division", create_type=False)` without an explicit `division.create()` call, so SQLAlchemy 2.0.48 emits the type creation on the first hit and again on the second. Should be a separate Forgejo issue. 2. **`tests/test_jersey_reminder.py::test_plain_text_has_deadline_copy`** — asserts "March 30" deadline copy, but template now reads "April 10". Pre-existing failure on `main`, unrelated to this migration. Tracking ticket may already exist (#491 mentions "two pre-existing test failures keep CI red on main"); worth checking before filing a new one. Neither blocks #515. Both warrant their own board items. ### VERDICT: APPROVED

forgejo_admin referenced this pull request

2026-04-25 14:03:52 +00:00

Two pre-existing test failures keep CI red on main #491

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 510-grant-tournament-tables-to-ro-role:510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff 510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

git rebase main

git switch main

git merge --ff-only 510-grant-tournament-tables-to-ro-role

git switch 510-grant-tournament-tables-to-ro-role

git rebase main

git switch main

git merge --no-ff 510-grant-tournament-tables-to-ro-role

git switch main

git merge --squash 510-grant-tournament-tables-to-ro-role