From 7b9eee4805c12ae8a3139b06e34722d4202b2148 Mon Sep 17 00:00:00 2001 From: GGThed Date: Tue, 11 Aug 2026 15:00:18 -0400 Subject: [PATCH] feat(db): mesurer la derive du schema, au lieu de la supposer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DB-001. create_all() cree les tables manquantes et ne fait jamais d ALTER. Une colonne ajoutee a un modele il y a six mois est donc absente de toute base qui possedait deja la table, et rien ne le dit : l application demarre, et la premiere requete qui touche cette colonne echoue a l execution. C est la raison d etre de migrations/add_tryout_coaches.py, ecrit a la main pour rattraper un cas. Personne ne sait combien il y en a d autres. app/supporting_scripts/schema_report.py compare le catalogue d une base vivante aux modeles : tables, colonnes, types, nullabilite, contraintes d unicite, cles etrangeres, index. En **lecture seule** — il ouvre une connexion, lit, imprime, sort. Aucun DDL, aucun DML. Les constats sont classes par ce qu ils coutent, pas par ce qu ils sont : - BLOCKING : les modeles l attendent, la base ne l a pas. C est la derive ; - RISK : la base l a, aucun modele ne le decrit. Inoffensif tant que rien ne bouge — et **un alembic --autogenerate proposera de le supprimer**, avec ses donnees. C est la classe qu on lit en entier ; - DIFFERENCE : types, nullabilite, contraintes. Chacune demande un humain. Les types sont compares apres compilation vers le meme dialecte : opposer String(200) a VARCHAR(200) en chaines aurait signale chaque colonne comme differente, et un rapport qui crie partout ne se lit plus. --check-seed-accounts repond a la question de SEC-003 a laquelle le depot ne peut pas repondre : le compte admin/password seme par clear_db.py existe-t-il encore, et son mot de passe est-il toujours celui-la. 13 tests le pilotent contre des bases SQLite fabriquees pour diverger d une facon connue. Le cas qui compte le plus est la base propre : un rapport qui crie sur une base saine ne sera pas lu, et un rapport qui dit « aucun ecart » sur une base derivee est pire que pas de rapport — c est un feu vert pour laisser autogenerate ecrire la difference en DROP. docs/database-schema.md donne la suite, etape par etape, avec le piege de DB-002 en toutes lettres : la migration initiale doit decrire la base telle qu elle est, pas telle que les modeles la decrivent. Generer depuis les modeles puis estampiller revient a declarer que la derive n existe pas. Alembic n est pas ajoute aux dependances : rien ne l utilise encore, et une dependance que rien n utilise est exactement ce que l audit reprochait ailleurs. Le document dit a quelle etape l ajouter. 530 tests. --- app/supporting_scripts/schema_report.py | 384 ++++++++++++++++++++++++ docs/database-schema.md | 121 ++++++++ tests/test_schema_report.py | 259 ++++++++++++++++ 3 files changed, 764 insertions(+) create mode 100644 app/supporting_scripts/schema_report.py create mode 100644 docs/database-schema.md create mode 100644 tests/test_schema_report.py diff --git a/app/supporting_scripts/schema_report.py b/app/supporting_scripts/schema_report.py new file mode 100644 index 0000000..7942786 --- /dev/null +++ b/app/supporting_scripts/schema_report.py @@ -0,0 +1,384 @@ +"""Compare a live database against the models (DB-001). + +Why this exists +--------------- +`db.create_all()` creates missing tables and never ALTERs an existing one. A +column added to a model months ago is therefore simply absent from any +database that already had the table, and nothing says so: the application +starts, and the first query touching that column fails at runtime. The audit +called the accumulated result "les dérives" and could not measure it, because +measuring it needs the production database. + +This script measures it. It is **read-only** — it opens a connection, reads +the catalogue, prints a report and exits. It issues no DDL and no DML, and +takes no locks beyond what reading `information_schema` takes. + +It is the prerequisite for everything in the DB wave: `DB-002` asks for an +initial Alembic migration describing the **real** schema rather than the +models', and this is what tells you what the real schema is. + +Usage +----- + # Against whatever DATABASE_URL points at + python app/supporting_scripts/schema_report.py + + # Against a restored copy, which is the safe way to do it first + python app/supporting_scripts/schema_report.py \ + --url postgresql://user:pass@host:5432/restored_copy + + # Also look for the seeded admin/password account (SEC-003) + python app/supporting_scripts/schema_report.py --check-seed-accounts + +Exit codes +---------- + 0 the live schema matches the models + 1 drift found — the report says what + 2 could not connect or read the catalogue + +Reading the output +------------------ +Findings are grouped by what they cost you: + + BLOCKING the application will fail at runtime — a table or column the + models use and the database does not have. + RISK the database has something the models do not describe. Harmless + to the running application, but an Alembic autogenerate would + propose to DROP it, which is how a corrective migration deletes + a column somebody still needed. + DIFFERENCE type, nullability, default or constraint disagreements. Each one + needs a human: some are dialect spelling, some are real. +""" + +import argparse +import os +import sys + +# Importable as a script from the project root, like the other supporting +# scripts: `python app/supporting_scripts/schema_report.py`. +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) + +from sqlalchemy import create_engine, inspect # noqa: E402 +from sqlalchemy.exc import SQLAlchemyError # noqa: E402 + +BLOCKING = 'BLOCKING' +RISK = 'RISK' +DIFFERENCE = 'DIFFERENCE' + + +class Finding: + """One disagreement between the models and the live database.""" + + def __init__(self, severity, table, detail, consequence=''): + self.severity = severity + self.table = table + self.detail = detail + self.consequence = consequence + + def __str__(self): + line = f' [{self.severity:10}] {self.table}: {self.detail}' + if self.consequence: + line += f'\n → {self.consequence}' + return line + + def __repr__(self): # pragma: no cover — debugging aid + return f'' + + +def model_metadata(): + """The schema the models describe. + + Imports app.models for its side effect: importing the modules is what + registers every table on the shared metadata. + """ + from app.extensions import db + from app.models import User # noqa: F401 — registers the whole model package + + return db.metadata + + +def _type_of(column_type, dialect): + """A type as this dialect spells it, so the two sides are comparable. + + Comparing `String(200)` with `VARCHAR(200)` as strings would report every + column as different. Compiling both against the same dialect makes the + comparison mean something. + """ + try: + return column_type.compile(dialect=dialect) + except Exception: # noqa: BLE001 — an uncompilable type is still reportable + return str(column_type) + + +def compare_tables(metadata, inspector): + """Tables the models expect against tables the database has.""" + findings = [] + model_tables = set(metadata.tables) + live_tables = set(inspector.get_table_names()) + + for name in sorted(model_tables - live_tables): + findings.append( + Finding( + BLOCKING, + name, + 'table is missing from the database', + 'every query against this model fails. create_all() would ' + 'create it — which is why the absence can survive unnoticed ' + 'on a machine where AUTO_CREATE_TABLES is on.', + ) + ) + + for name in sorted(live_tables - model_tables): + findings.append( + Finding( + RISK, + name, + 'table exists in the database and in no model', + 'an Alembic autogenerate would propose to DROP it. Decide ' + 'before running one: it may be a leftover, or it may be the ' + 'only copy of something.', + ) + ) + + return findings, sorted(model_tables & live_tables) + + +def compare_columns(metadata, inspector, table_name, dialect): + """Column-by-column, for one table.""" + findings = [] + model_columns = {c.name: c for c in metadata.tables[table_name].columns} + live_columns = {c['name']: c for c in inspector.get_columns(table_name)} + + for name in sorted(set(model_columns) - set(live_columns)): + column = model_columns[name] + findings.append( + Finding( + BLOCKING, + table_name, + f'column "{name}" is in the model and not in the database', + 'this is exactly what create_all() cannot fix: it never ' + 'ALTERs. Any query selecting or writing this column fails.' + + ( + '' + if column.nullable + else ' The column is NOT NULL, so the ' + 'corrective migration needs a default or a backfill.' + ), + ) + ) + + for name in sorted(set(live_columns) - set(model_columns)): + findings.append( + Finding( + RISK, + table_name, + f'column "{name}" is in the database and not in any model', + 'an autogenerated migration would propose to DROP it, taking ' + 'its data. Check whether something outside the application ' + 'reads it before agreeing.', + ) + ) + + for name in sorted(set(model_columns) & set(live_columns)): + model_column, live_column = model_columns[name], live_columns[name] + + model_type = _type_of(model_column.type, dialect) + live_type = _type_of(live_column['type'], dialect) + if model_type != live_type: + findings.append( + Finding( + DIFFERENCE, + table_name, + f'column "{name}" type: model says {model_type}, database says {live_type}', + 'a narrower column in the database silently truncates or ' + 'rejects; a wider one is usually harmless.', + ) + ) + + if bool(model_column.nullable) != bool(live_column.get('nullable', True)): + findings.append( + Finding( + DIFFERENCE, + table_name, + f'column "{name}" nullability: model says ' + f'{"NULL" if model_column.nullable else "NOT NULL"}, database says ' + f'{"NULL" if live_column.get("nullable", True) else "NOT NULL"}', + 'a NOT NULL the database does not enforce is a constraint ' + 'the application only believes it has.', + ) + ) + + return findings + + +def compare_constraints(metadata, inspector, table_name): + """Unique constraints, indexes and foreign keys. + + Named constraints are compared by the columns they cover rather than by + name: the same rule declared under two names is the same rule, and + reporting it as a difference would bury the ones that matter. + """ + findings = [] + table = metadata.tables[table_name] + + def column_sets(entries, key): + return {tuple(sorted(entry[key] or [])) for entry in entries} + + model_unique = { + tuple(sorted(c.name for c in constraint.columns)) + for constraint in table.constraints + if constraint.__class__.__name__ == 'UniqueConstraint' + } + live_unique = column_sets(inspector.get_unique_constraints(table_name), 'column_names') + for columns in sorted(model_unique - live_unique): + findings.append( + Finding( + DIFFERENCE, + table_name, + f'unique constraint on {list(columns)} is declared and absent from the database', + 'the application believes duplicates are impossible here. ' + 'They are not, and two concurrent requests will prove it.', + ) + ) + + model_fks = { + tuple(sorted(fk.parent.name for fk in constraint.elements)) + for constraint in table.foreign_key_constraints + } + live_fks = column_sets(inspector.get_foreign_keys(table_name), 'constrained_columns') + for columns in sorted(model_fks - live_fks): + findings.append( + Finding( + DIFFERENCE, + table_name, + f'foreign key on {list(columns)} is declared and absent from the database', + 'orphan rows are possible, and ON DELETE behaviour is not ' + 'being enforced by the database at all.', + ) + ) + + model_indexes = {tuple(sorted(c.name for c in index.columns)) for index in table.indexes} + live_indexes = column_sets(inspector.get_indexes(table_name), 'column_names') + for columns in sorted(model_indexes - live_indexes): + findings.append( + Finding( + DIFFERENCE, + table_name, + f'index on {list(columns)} is declared and absent from the database', + 'correctness is unaffected; the queries that rely on it are ' + 'doing sequential scans.', + ) + ) + + return findings + + +def collect_findings(engine): + """Every disagreement between the models and this database.""" + metadata = model_metadata() + inspector = inspect(engine) + + findings, shared_tables = compare_tables(metadata, inspector) + for table_name in shared_tables: + findings.extend(compare_columns(metadata, inspector, table_name, engine.dialect)) + findings.extend(compare_constraints(metadata, inspector, table_name)) + return findings + + +def find_seed_accounts(engine): + """Accounts matching the credentials clear_db.py used to seed (SEC-003). + + The script was removed from the deployment, but it had already been run: + the audit could not tell whether an `admin` account with the password + `password` still exists in production, and that question cannot be + answered from the repository. + + Returns: + list[tuple]: (username, role, whether the known password matches). + """ + from sqlalchemy import text + + from app.extensions import check_password + + with engine.connect() as connection: + rows = connection.execute( + text('SELECT username, role, password_hash FROM users WHERE username = :name'), + {'name': 'admin'}, + ).fetchall() + + results = [] + for username, role, password_hash in rows: + try: + matches = check_password(password_hash, 'password') + except Exception: # noqa: BLE001 — an unreadable hash is not a match + matches = False + results.append((username, role, matches)) + return results + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.split('\n')[0]) + parser.add_argument( + '--url', + default=os.getenv('DATABASE_URL'), + help='Database URL. Defaults to DATABASE_URL. Point it at a restored copy the first time.', + ) + parser.add_argument( + '--check-seed-accounts', + action='store_true', + help='Also look for the admin/password account seeded by clear_db.py (SEC-003).', + ) + args = parser.parse_args(argv) + + if not args.url: + print('No database URL. Pass --url or set DATABASE_URL.', file=sys.stderr) + return 2 + + from app.app import normalise_database_url + + try: + engine = create_engine(normalise_database_url(args.url)) + findings = collect_findings(engine) + except SQLAlchemyError as exc: + print(f'Could not read the schema: {exc}', file=sys.stderr) + return 2 + + print('=' * 78) + print('Schema report — models vs live database (DB-001)') + print('=' * 78) + + if not findings: + print('\nNo drift. The live schema matches the models.') + for severity in (BLOCKING, RISK, DIFFERENCE): + group = [f for f in findings if f.severity == severity] + if not group: + continue + print(f'\n{severity} — {len(group)} finding(s)') + for finding in group: + print(finding) + + if args.check_seed_accounts: + print('\n' + '=' * 78) + print('Seeded accounts (SEC-003)') + print('=' * 78) + try: + accounts = find_seed_accounts(engine) + except SQLAlchemyError as exc: + print(f'Could not check: {exc}') + else: + if not accounts: + print('No account named "admin".') + for username, role, matches in accounts: + verdict = ( + 'PASSWORD IS STILL "password" — change it now' + if matches + else 'password has been changed' + ) + print(f' {username} ({role}): {verdict}') + + blocking = sum(1 for f in findings if f.severity == BLOCKING) + print(f'\n{len(findings)} finding(s), {blocking} blocking.') + return 1 if findings else 0 + + +if __name__ == '__main__': # pragma: no cover + sys.exit(main()) diff --git a/docs/database-schema.md b/docs/database-schema.md new file mode 100644 index 0000000..b65ff54 --- /dev/null +++ b/docs/database-schema.md @@ -0,0 +1,121 @@ +# Le schéma réel, et comment sortir de `create_all()` + +> **État au 2026-08-11** : l'outil de relevé existe et est testé. Le relevé +> lui-même n'a pas été exécuté — il demande un accès à la base de production, +> qui ne peut pas venir du dépôt. Tout ce qui suit attend cette exécution. + +## Pourquoi c'est le nœud + +`db.create_all()` crée les tables manquantes et **ne fait jamais d'`ALTER`**. + +Une colonne ajoutée à un modèle il y a six mois est donc absente de toute base +qui possédait déjà la table, et rien ne le dit : l'application démarre +normalement, et la première requête qui touche cette colonne échoue à +l'exécution. C'est la raison d'être de `migrations/add_tryout_coaches.py`, un +script écrit à la main pour rattraper un cas. + +Personne ne sait combien il y en a d'autres. C'est ce que `DB-001` mesure, et +c'est pourquoi **huit tâches en dépendent** : `DB-002` à `DB-009`, plus +`ARCH-001` (fusion coach/équipe) et `SEC-012` (identité Discord avec +`unique=True`). + +## Étape 1 — sauvegarder, et vérifier la sauvegarde + +**Rien de ce qui suit ne se fait avant qu'une sauvegarde ait été restaurée +avec succès.** Pas « prise » : *restaurée*. Une sauvegarde qu'on n'a jamais +restaurée est une hypothèse. + +Procédure dans `docs/database-restore.md`. La copie restaurée sert aussi de +terrain pour les étapes 2 et 3. + +## Étape 2 — relever l'écart + +```bash +# D'abord sur la copie restaurée, jamais directement sur la production +python app/supporting_scripts/schema_report.py \ + --url postgresql://user:pass@host:5432/copie_restauree +``` + +L'outil est **en lecture seule** : il ouvre une connexion, lit le catalogue, +imprime et sort. Aucun DDL, aucun DML. + +Codes de sortie : `0` aucun écart, `1` écart trouvé, `2` connexion impossible. + +Le rapport classe ce qu'il trouve par ce que ça coûte : + +| Classe | Ce que c'est | Ce que ça coûte | +|---|---|---| +| `BLOCKING` | Les modèles l'attendent, la base ne l'a pas | L'application échoue à l'exécution. C'est la dérive de `create_all()` | +| `RISK` | La base l'a, aucun modèle ne le décrit | Inoffensif tant que rien ne bouge. **Un `alembic --autogenerate` proposera de le supprimer**, avec ses données | +| `DIFFERENCE` | Types, nullabilité, contraintes qui divergent | Chacune demande un humain : certaines sont de l'orthographe de dialecte, d'autres sont réelles | + +**La classe `RISK` est celle qu'on lit en entier.** C'est par là qu'une +migration corrective détruit une colonne dont quelqu'un se servait encore. + +Pendant qu'on y est, la question que l'audit posait et à laquelle le dépôt ne +peut pas répondre — le compte `admin`/`password` semé par `clear_db.py` +existe-t-il encore ? (`SEC-003`) : + +```bash +python app/supporting_scripts/schema_report.py --check-seed-accounts +``` + +## Étape 3 — Alembic, décrivant le schéma **réel** (`DB-002`) + +Le piège de cette étape tient en une phrase : **la migration initiale doit +décrire la base telle qu'elle est, pas telle que les modèles la décrivent.** + +Générer la migration initiale depuis les modèles puis estampiller la +production revient à déclarer que la dérive n'existe pas. Elle reste là, +invisible, et la première migration suivante s'appuie sur un état faux. + +1. Ajouter `alembic` à `requirements.txt` — et seulement à ce moment : une + dépendance que rien n'utilise est exactement ce que l'audit reprochait + ailleurs. +2. `alembic init migrations/alembic`, en pointant `sqlalchemy.url` sur + `DATABASE_URL` plutôt qu'en le codant en dur. +3. Générer la révision initiale **contre la copie restaurée** : + `alembic revision --autogenerate -m "schéma existant"`. +4. **Relire la révision ligne par ligne** contre le rapport de l'étape 2. Tout + `op.drop_*` correspond à une ligne `RISK` : ou bien on l'assume, ou bien on + le retire de la migration. +5. Estampiller : `alembic stamp head`. La révision initiale ne s'exécute + jamais ; elle décrit le point de départ. + +## Étape 4 — la migration corrective (`DB-003`) + +Une seconde révision qui rattrape les écarts relevés. À exécuter d'abord sur +la copie restaurée, et à vérifier avec : + +```bash +alembic upgrade head && alembic downgrade -1 && alembic upgrade head +python app/supporting_scripts/schema_report.py --url # doit sortir 0 +``` + +Le critère d'acceptation est celui-là : le relevé ne trouve plus rien. + +Attention aux colonnes `BLOCKING` déclarées `NOT NULL` : les ajouter à une +table peuplée échoue sans valeur par défaut ni remplissage. Le rapport le +signale dans la conséquence. + +## Étape 5 — ce que la migration débloque + +Dans cet ordre, parce qu'ils dépendent tous de `DB-002` : + +| Tâche | Ce qu'elle fait | Pourquoi ça attendait | +|---|---|---| +| `DB-004` | Retirer `create_all()` de `create_app()` | Tant qu'il est là, deux mécanismes décrivent le schéma | +| `DB-005` | Cascades de suppression au niveau base | Les cascades ORM sont en place ; PostgreSQL ne les connaît pas | +| `DB-006` | Unicité sur `TryoutRegistration(tryout_id, player_id)` | Le plafond d'inscriptions est aujourd'hui un `count()` suivi d'un `add()` : deux requêtes simultanées passent toutes les deux | +| `DB-007` | Index, `CheckConstraint` sur les statuts, `server_default` | — | +| `DB-008` | Trancher `attendance_confirmed` côté tryout | `discord_bot.py` écrit un attribut fantôme ; aujourd'hui journalisé en avertissement | +| `DB-009` | Horodatages avec fuseau | `datetime.utcnow` partout, déprécié en 3.12 | +| `ARCH-001` | Fusionner coach/équipe sur la relation m2m | Migration de données ; `app/permissions.py` rend la duplication inoffensive **en lecture** seulement, l'écriture crée toujours les deux | +| `SEC-012` | Identité Discord côté serveur, `unique=True` | La colonne doit être unique, donc dédoublonnée d'abord | + +## Ce qu'on ne fait pas + +Générer la migration initiale depuis les modèles « pour avancer en +attendant ». Ça produit un dépôt qui a l'air d'avoir des migrations, une +production estampillée sur un état qu'elle n'a pas, et la dérive préservée +sous une couche de plus. diff --git a/tests/test_schema_report.py b/tests/test_schema_report.py new file mode 100644 index 0000000..8b2fd51 --- /dev/null +++ b/tests/test_schema_report.py @@ -0,0 +1,259 @@ +"""The schema drift report actually finds drift (DB-001). + +`db.create_all()` creates missing tables and never ALTERs one. Every column +added to a model since a table was first created is therefore absent from +any database that already had it, and the application says nothing until a +query touches it. Measuring that needs the production database — which is +why the audit could only name the problem. + +These tests drive the report against SQLite databases built to disagree with +the models in a specific way, so that the tool's answer can be checked +against a known truth. A report that returns "no drift" on a drifted +database is worse than no report: it is a green light to run +`alembic revision --autogenerate` and let it write the difference as DROPs. + +The tool is read-only; nothing here asserts otherwise, because nothing here +gives it the chance to be anything else. +""" + +import sqlite3 + +import pytest +from sqlalchemy import create_engine + +from app.supporting_scripts.schema_report import ( + BLOCKING, + DIFFERENCE, + RISK, + collect_findings, + main, +) + + +@pytest.fixture +def live_db(tmp_path, app): + """A database created from the models, then tampered with. + + Built through the application so it starts out identical to the models — + the baseline the report has to call clean before its disagreements mean + anything. + """ + path = tmp_path / 'live.sqlite' + engine = create_engine(f'sqlite:///{path}') + + from app.extensions import db + + with app.app_context(): + db.metadata.create_all(engine) + + def tamper(*statements): + connection = sqlite3.connect(path) + for statement in statements: + connection.execute(statement) + connection.commit() + connection.close() + return create_engine(f'sqlite:///{path}') + + return engine, tamper + + +def _by_severity(findings, severity): + return [f for f in findings if f.severity == severity] + + +class TestCleanDatabase: + def test_a_database_built_from_the_models_reports_no_drift(self, live_db, app): + engine, _tamper = live_db + + with app.app_context(): + findings = collect_findings(engine) + + assert findings == [], f'false positives make the report unusable: {findings}' + + +class TestMissingThings: + """The BLOCKING class: the models expect it, the database has not got it.""" + + def test_a_dropped_table_is_blocking(self, live_db, app): + engine, tamper = live_db + engine = tamper('DROP TABLE evaluations') + + with app.app_context(): + findings = collect_findings(engine) + + blocking = _by_severity(findings, BLOCKING) + assert any(f.table == 'evaluations' for f in blocking) + + def test_a_column_the_model_added_later_is_blocking(self, live_db, app): + """The exact shape of the create_all() drift: the table exists, so + create_all() leaves it alone for ever.""" + engine, tamper = live_db + engine = tamper( + 'ALTER TABLE tryouts RENAME TO tryouts_old', + 'CREATE TABLE tryouts (id INTEGER PRIMARY KEY, title VARCHAR(200) NOT NULL)', + ) + + with app.app_context(): + findings = collect_findings(engine) + + missing = [f for f in _by_severity(findings, BLOCKING) if f.table == 'tryouts'] + assert any('"game"' in f.detail for f in missing) + assert any('"date"' in f.detail for f in missing) + + def test_a_missing_not_null_column_says_the_migration_needs_a_default(self, live_db, app): + """A corrective migration that adds a NOT NULL column to a populated + table fails unless it carries a default or a backfill. The report has + to say so, because that is where the migration breaks.""" + engine, tamper = live_db + engine = tamper( + 'ALTER TABLE tryouts RENAME TO tryouts_old', + 'CREATE TABLE tryouts (id INTEGER PRIMARY KEY, title VARCHAR(200) NOT NULL)', + ) + + with app.app_context(): + findings = collect_findings(engine) + + game = next(f for f in findings if '"game"' in f.detail) + assert 'NOT NULL' in game.consequence + assert 'backfill' in game.consequence + + +class TestExtraThings: + """The RISK class: the database has it, no model describes it. + + Harmless while running, dangerous the moment somebody autogenerates a + migration — which will propose to DROP it. + """ + + def test_an_unknown_table_is_reported_as_a_risk(self, live_db, app): + engine, tamper = live_db + engine = tamper('CREATE TABLE legacy_import (id INTEGER PRIMARY KEY)') + + with app.app_context(): + findings = collect_findings(engine) + + risks = _by_severity(findings, RISK) + assert any(f.table == 'legacy_import' for f in risks) + assert any('DROP' in f.consequence for f in risks) + + def test_an_unknown_column_is_reported_as_a_risk(self, live_db, app): + engine, tamper = live_db + engine = tamper('ALTER TABLE tryouts ADD COLUMN old_notes TEXT') + + with app.app_context(): + findings = collect_findings(engine) + + assert any(f.severity == RISK and 'old_notes' in f.detail for f in findings) + + +class TestDifferences: + def test_a_nullability_disagreement_is_reported(self, live_db, app): + """A NOT NULL the database does not enforce is a constraint the + application only believes it has.""" + engine, tamper = live_db + engine = tamper( + 'ALTER TABLE tryouts RENAME TO tryouts_old', + 'CREATE TABLE tryouts (' + ' id INTEGER PRIMARY KEY, title VARCHAR(200), game VARCHAR(50) NOT NULL,' + ' date DATE NOT NULL, created_by INTEGER NOT NULL)', + ) + + with app.app_context(): + findings = collect_findings(engine) + + assert any( + f.severity == DIFFERENCE and 'nullability' in f.detail and '"title"' in f.detail + for f in findings + ) + + def test_a_type_disagreement_is_reported(self, live_db, app): + engine, tamper = live_db + engine = tamper( + 'ALTER TABLE tryouts RENAME TO tryouts_old', + 'CREATE TABLE tryouts (' + ' id INTEGER PRIMARY KEY, title VARCHAR(20) NOT NULL, game VARCHAR(50) NOT NULL,' + ' date DATE NOT NULL, created_by INTEGER NOT NULL)', + ) + + with app.app_context(): + findings = collect_findings(engine) + + narrowed = [f for f in findings if 'type' in f.detail and '"title"' in f.detail] + assert narrowed, 'a column narrowed from 200 to 20 characters truncates silently' + assert 'VARCHAR(20)' in narrowed[0].detail + + +class TestExitCodes: + """The report is meant to be runnable from a check, so the code matters.""" + + def test_a_clean_database_exits_zero(self, live_db, app, capsys): + engine, _tamper = live_db + + with app.app_context(): + assert main(['--url', str(engine.url)]) == 0 + + assert 'No drift' in capsys.readouterr().out + + def test_a_drifted_database_exits_one(self, live_db, app, capsys): + engine, tamper = live_db + engine = tamper('DROP TABLE evaluations') + + with app.app_context(): + assert main(['--url', str(engine.url)]) == 1 + + assert 'BLOCKING' in capsys.readouterr().out + + def test_no_url_at_all_exits_two_rather_than_guessing(self, monkeypatch, capsys): + """Defaulting to something would eventually point this at the wrong + database.""" + monkeypatch.delenv('DATABASE_URL', raising=False) + + assert main([]) == 2 + + +class TestSeedAccountCheck: + def test_it_finds_an_admin_whose_password_is_still_the_seeded_one(self, live_db, app, capsys): + """SEC-003 asked whether clear_db.py's admin/password account still + exists in production. That cannot be answered from the repository.""" + engine, tamper = live_db + + from app.extensions import hash_password + + with app.app_context(): + weak = hash_password('password') + connection = sqlite3.connect(str(engine.url).replace('sqlite:///', '')) + connection.execute( + 'INSERT INTO users (username, password_hash, role, full_name, email, is_active_account)' + " VALUES ('admin', ?, 'admin', 'Seeded', 'admin@example.test', 1)", + (weak,), + ) + connection.commit() + connection.close() + + with app.app_context(): + main(['--url', str(engine.url), '--check-seed-accounts']) + + assert 'PASSWORD IS STILL' in capsys.readouterr().out + + def test_a_changed_password_reports_as_changed(self, live_db, app, capsys): + engine, _tamper = live_db + + from app.extensions import hash_password + + with app.app_context(): + strong = hash_password('a-real-password-9X') + connection = sqlite3.connect(str(engine.url).replace('sqlite:///', '')) + connection.execute( + 'INSERT INTO users (username, password_hash, role, full_name, email, is_active_account)' + " VALUES ('admin', ?, 'admin', 'Real', 'admin@example.test', 1)", + (strong,), + ) + connection.commit() + connection.close() + + with app.app_context(): + main(['--url', str(engine.url), '--check-seed-accounts']) + + output = capsys.readouterr().out + assert 'password has been changed' in output + assert 'PASSWORD IS STILL' not in output