refactor(api): move from Postgres+RLS to single-engine SQLite
CI / Repo hygiene (pull_request) Successful in 2s
CI / Web (lint, typecheck, build) (pull_request) Successful in 13s
CI / Migrations reversible (pull_request) Successful in 6s
CI / API (lint, types, tests) (pull_request) Successful in 52s

Reverses a shipped, tested, merged decision (D4/PR #2) rather than building
on it — see docs/DECISIONS.md D15 for the full record: what was rejected
(Postgres as a second container; Postgres+PostGIS bundled inside the single
container via a supervisor), what this costs (no database-level RLS, no
PostGIS, procrastinate needs replacing — all stated as a concern before this
was decided, and reaffirmed anyway, which is the user's call to make about
their own instance).

The one invariant-critical consequence: isolation between users now rests
entirely on the repository-layer scope (db.py's `Scope.select()`), not two
layers. CLAUDE.md's invariant #4 is revised accordingly. This is not a
downgrade-and-hope — `Scope` is built so an unfiltered query against a
user-owned table is structurally harder to write than a scoped one (there is
no method on `Scope` that returns one), and
tests/test_auth.py::test_scoped_session_blocks_cross_user_reads replaces the
old RLS proof with the same empirical standard: it doesn't trust the query
builder filters correctly because the code reads correctly, it registers two
real users and checks.
test_unscoped_session_can_see_every_user_when_misused is the deliberately
alarming companion — it demonstrates exactly what a reviewer must now catch,
since nothing else will.

Six real, non-obvious SQLite behaviours found and fixed by actually running
this against a real file, not assumed from docs:

- Foreign keys, ON DELETE CASCADE included, are OFF by default per
  connection — deleting a user silently left orphaned sessions/api_tokens,
  no error either way. Fixed with PRAGMA foreign_keys=ON on every connect.
- Transactions default to DEFERRED, which only takes a write lock on the
  first actual write — a real check-then-act race for invite redemption
  (two concurrent redemptions could both read used_count < max_uses as true
  before either commits). Fixed by disabling the driver's implicit BEGIN and
  issuing BEGIN IMMEDIATE ourselves — SQLAlchemy's own documented recipe for
  this, not improvised.
- DateTime(timezone=True) does NOT round-trip tzinfo on SQLite — a tz-aware
  datetime goes in, a naive one comes back out, and every
  `expires_at < datetime.now(UTC)` comparison in auth/service.py then raises
  TypeError. Fixed once at the Base level with a UTCDateTime TypeDecorator
  rather than per-column.
- Uuid(as_uuid=True) stores as 32-char hex with NO hyphens on SQLite, not
  str(uuid)'s hyphenated form. A test fixture that raw-inserted the
  hyphenated form left rows the ORM's own later UPDATE (via
  invite.used_count += 1's autoflush) could never match by primary key,
  updating zero rows and raising StaleDataError. Fixed by using .hex to
  match exactly what the ORM itself writes.
- BEGIN IMMEDIATE applies to every transaction, reads included — a
  long-lived test fixture that autobegins a transaction via a bare read and
  never explicitly closes it holds SQLite's exclusive write lock for the
  rest of the test, and a later scoped_session() call fails with "database
  is locked". Not an app-code bug (every real session block closes cleanly
  on exit), but real enough to document since the next person writing a
  test against the db_auth fixture will hit it too.
- Python's sqlite3 module deprecates its own implicit datetime adapter as of
  3.12 — silent today, warns on every raw-SQL datetime bind. Only ever hit
  test fixture code (the ORM path never uses it, confirmed by running the
  ORM-only health test with warnings promoted to errors and it stayed
  clean); fixed there with an explicit .isoformat() rather than left for a
  future Python version to turn into a real failure.

Also, since with_for_update() silently no-ops on SQLite (confirmed —
SQLAlchemy emits no SQL for it, no error either) rather than actually
locking anything: removed it from register()'s invite-redemption query and
corrected the comment to attribute the concurrency guarantee to BEGIN
IMMEDIATE, where it now actually lives.

One PR, not several, for the same reason PR #2 was: the migration, the
models, db.py, and the docs recording why are five views of one decision —
splitting them wouldn't make review easier, just disconnected. 552
insertions / 548 deletions across 17 files, most of it necessarily touching
what PR #2 shipped rather than net-new code.

Deliberately deferred, not solved here: PostGIS's replacement for spatial
storage, procrastinate's replacement for background jobs, and the
EXCLUDE USING gist constraint's replacement for component_installs — none
of those tables exist yet (Phase 1-2), so none of it is broken, and
docs/DECISIONS.md D15 records exactly what each future phase needs to
decide before it can be built. .gitea/workflows/deploy pipeline (PR #4,
built for the old 3-container Postgres compose stack) was closed as
superseded rather than merged; the single-container image build is
follow-up work, not part of this change.

Verified: ruff check, ruff format --check, and mypy --strict all clean.
13/13 pytest passing against a real SQLite file, including with
DeprecationWarning promoted to an error (confirms the sqlite3 adapter
deprecation fix actually holds, not just that it's quiet by default). Full
alembic upgrade -> downgrade -1 -> upgrade cycle run clean. alembic check
clean with no include_object filter needed at all now (SQLite starts with
nothing but what our own migrations create — no PostGIS/TIGER noise to
filter out in the first place). CI's exact migration command sequence
reproduced locally end to end before touching the workflow file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-09-21 15:42:44 -04:00
co-authored by Claude Opus 5
parent 1832059e03
commit e7392a5723
17 changed files with 548 additions and 544 deletions
+12 -39
View File
@@ -1,16 +1,15 @@
"""Alembic environment.
Deliberately independent of velodrome.config.Settings: migrations run as a privileged/owner
connection (VELODROME_DATABASE_URL_MIGRATE — a superuser or schema-owning role, which trivially
satisfies "BYPASSRLS" since superusers always bypass RLS), never as either of the two runtime
roles the app itself uses. Reading env vars directly here, rather than importing the app's
settings, keeps a migration-only CI job from needing every runtime env var the app requires.
Reads VELODROME_DATABASE_URL directly from the environment rather than importing
velodrome.config.Settings — keeps a migration-only invocation from needing every other runtime
env var the app requires, even though today they'd resolve to the same value. There's no more
separate migration/owner role to reason about here (docs/DECISIONS.md D15): SQLite has no roles,
so migrations run against the exact same file and connection the app itself uses.
"""
import asyncio
import os
import sqlalchemy as sa
from sqlalchemy.engine import Connection
from sqlalchemy.ext.asyncio import async_engine_from_config
@@ -21,60 +20,34 @@ config = context.config
target_metadata = Base.metadata
# postgis/postgis ships an entire pre-installed schema of its own (PostGIS core tables plus the
# TIGER geocoder's tiger/topology schemas — dozens of tables) that Alembic never created and
# doesn't manage. Without this filter, `alembic check`/autogenerate sees every single one as
# "should be dropped" simply because it's not in our SQLAlchemy metadata — which would make CI's
# `alembic check` step permanently useless (always red, for reasons that have nothing to do with
# an actual drift).
#
# A denylist keyed on schema name is NOT reliable here: reflected foreign tables can come back
# with `schema=None` on their Table object regardless of which schema they actually live in on
# the server (confirmed against a real postgis/postgis:16-3.4 container — tables that `\dt`
# clearly shows under the `tiger` schema still reflect with schema=None). An allowlist is the
# robust version of the same idea: only ever compare tables OUR metadata declares, so a future
# PostGIS/TIGER version adding more foreign tables can never cause a false positive here.
def include_object(
object: sa.schema.SchemaItem, name: str | None, type_: str, reflected: bool, compare_to: object
) -> bool:
if type_ == "table":
return name in target_metadata.tables
return True
def _migrate_url() -> str:
url = os.environ.get("VELODROME_DATABASE_URL_MIGRATE")
def _database_url() -> str:
url = os.environ.get("VELODROME_DATABASE_URL")
if not url:
# Local-dev convenience only — every real environment (CI, deploy/) sets this explicitly.
url = "postgresql+asyncpg://postgres:postgres@localhost:5432/velodrome"
# Local-dev convenience only — CI and deploy/ both set this explicitly.
url = "sqlite+aiosqlite:///./velodrome.db"
return url
def run_migrations_offline() -> None:
context.configure(
url=_migrate_url(),
url=_database_url(),
target_metadata=target_metadata,
literal_binds=True,
dialect_opts={"paramstyle": "named"},
include_object=include_object,
)
with context.begin_transaction():
context.run_migrations()
def _do_run_migrations(connection: Connection) -> None:
context.configure(
connection=connection,
target_metadata=target_metadata,
include_object=include_object,
)
context.configure(connection=connection, target_metadata=target_metadata)
with context.begin_transaction():
context.run_migrations()
async def run_migrations_online() -> None:
configuration = config.get_section(config.config_ini_section) or {}
configuration["sqlalchemy.url"] = _migrate_url()
configuration["sqlalchemy.url"] = _database_url()
connectable = async_engine_from_config(configuration, prefix="sqlalchemy.")
async with connectable.connect() as connection: