diff --git a/deploy/consolidated/README.md b/deploy/consolidated/README.md index 57adc6e84..dc731f37c 100644 --- a/deploy/consolidated/README.md +++ b/deploy/consolidated/README.md @@ -273,9 +273,9 @@ applies the numbered `.sql` files in [`one_d4/migrations/`](../../domains/games/apis/one_d4/migrations/) and exits. Both `one_d4` and `one_d4_worker` gate on it with `service_completed_successfully` — the worker so it no longer waits for the -Java service to boot, the service so its own boot-time migration (which -still runs, until #1426 demotes it to a verifier) is serialized behind this -one rather than racing it. +Java service to boot, the service so the schema exists by the time its boot +check runs. This one-shot is the only thing that writes that schema; one_d4 +verifies at boot and crash-loops rather than repairing what it finds. Two things about `shared_postgres` are load-bearing and easy to undo by accident: diff --git a/deploy/consolidated/compose.yaml b/deploy/consolidated/compose.yaml index ce015131e..376fedb92 100644 --- a/deploy/consolidated/compose.yaml +++ b/deploy/consolidated/compose.yaml @@ -401,11 +401,11 @@ services: # The schema step (#1419): applies one_d4's migrations/ .sql files and # exits, before anything that needs the tables. Idempotent — safe on every - # deploy, like golf_hub_db_init above. The Java service still runs the - # same migrations at boot, after this (#1426 is the demotion to a - # verifier); what the step already buys is one_d4_worker starting without - # waiting for the Java service (#1418 measured that coupling as an - # error-loop until one_d4 came up). + # deploy, like golf_hub_db_init above. The only writer of that schema: + # one_d4 checks its work at boot rather than repeating it (#1426), and + # one_d4_worker creates nothing, so it starts without waiting for the Java + # service (#1418 measured that coupling as an error-loop until one_d4 + # came up). one_d4_migrate: image: ghcr.io/muchq/one_d4_migrate:${ONE_D4_MIGRATE_SHA:-${DEPLOY_SHA:-latest}} labels: @@ -454,12 +454,10 @@ services: - one-d4 depends_on: # The migrate gate is a strict superset of the old shared_postgres one - # (the one-shot itself waits for Postgres), and it serializes the two - # migration runners: this service still applies the same files at boot - # (#1426 is the demotion to a verifier), and released together the two - # would race — CREATE TABLE/INDEX IF NOT EXISTS is idempotent but not - # concurrency-safe, and the one-shot losing that race gates the worker - # off until the next deploy. + # (the one-shot itself waits for Postgres). This service verifies at + # boot that the migrations were applied and refuses to serve otherwise, + # so releasing the two together without the gate crash-loops it against + # a schema the one-shot has not finished writing. one_d4_migrate: condition: service_completed_successfully deploy: diff --git a/deploy/consolidated/deploy_config_test.go b/deploy/consolidated/deploy_config_test.go index 870893704..0c6de60b2 100644 --- a/deploy/consolidated/deploy_config_test.go +++ b/deploy/consolidated/deploy_config_test.go @@ -1393,18 +1393,16 @@ func TestTheWorkerGatesOnTheMigrateStepNotTheJavaService(t *testing.T) { } } -// The Java service gates on the same one-shot — not for the schema (it still -// applies the migrations at boot until #1426), but for serialization: released -// together, the two runners execute identical DDL concurrently, and -// CREATE TABLE/INDEX IF NOT EXISTS is idempotent yet not concurrency-safe on -// Postgres. The loser of that race under restart:"no" is one_d4_migrate, and -// its failure gates one_d4_worker off until the next deploy. +// The Java service gates on the same one-shot, and now needs to: since #1426 its +// boot checks the migrations were applied rather than applying them, and refuses +// to serve otherwise. Released together without the gate it crash-loops against a +// schema the one-shot has not finished writing. func TestTheJavaServiceAlsoGatesOnTheMigrateStep(t *testing.T) { if !gatesOnCompletedMigrate(t, "one_d4") { t.Errorf("one_d4 does not gate on one_d4_migrate with "+ - "service_completed_successfully (depends_on: %v) — its boot-time migration then "+ - "runs concurrently with the one-shot's, and the one-shot losing that race blocks "+ - "the worker.", dependsOn(t, "one_d4")) + "service_completed_successfully (depends_on: %v) — its boot-time schema check then "+ + "races the one-shot, and fails against the tables it has not created yet.", + dependsOn(t, "one_d4")) } } diff --git a/domains/games/apis/one_d4/BUILD.bazel b/domains/games/apis/one_d4/BUILD.bazel index 6fd0f9841..b401c294d 100644 --- a/domains/games/apis/one_d4/BUILD.bazel +++ b/domains/games/apis/one_d4/BUILD.bazel @@ -58,7 +58,7 @@ java_library( # The schema itself (#1419): numbered idempotent .sql files, manifest-ordered. # One copy of the DDL, three readers — Migration at boot (both engines), the -# one_d4_migrate deploy step, and one_d4_worker's schema_contract_test via +# one_d4_migrate deploy step, and one_d4_worker's Postgres suites via # :migrations_sql. Each file is named exactly once, in one of the three # groups below; the shipping and test targets compose the groups, so a new # step is a line in manifest.txt plus a line in one group. @@ -129,9 +129,10 @@ java_library( visibility = ["//visibility:public"], ) -# Every migration file plus the manifest, as plain files: data for -# one_d4_worker's schema_contract_test, which scrapes the Postgres DDL and -# checks that no .sql file is unreachable from the manifest. +# Every migration file plus the manifest, as plain files: what +# one_d4_worker's Postgres suites run to build the schema they test against, +# and what schema_contract_test walks to check that no .sql file is +# unreachable from the manifest. filegroup( name = "migrations_sql", testonly = True, @@ -378,12 +379,6 @@ filegroup( visibility = ["//domains/games:__subpackages__"], ) -# The DDL itself lives in :migrations_sql (#1419); what schema_contract_test -# still scrapes from Java source is the attempt budget, and nothing else. The -# lease and retention windows reach it as data, from :retention_policy_json -# (#1424), so RetentionPolicy.java does not belong here — listing a source -# nothing opens would re-run the contract test on every edit to it. - # The retention windows, as data rather than as a constant in either language (#1424). # # Both readers load this at startup — the C++ worker out of its image's runfiles, Java off its @@ -649,6 +644,7 @@ java_test_suite( "src/test/java/com/muchq/games/one_d4/db/PostgresAggregateCompatTest.java", "src/test/java/com/muchq/games/one_d4/db/PostgresConcurrentWriteTest.java", "src/test/java/com/muchq/games/one_d4/db/PostgresMigrationRunnerTest.java", + "src/test/java/com/muchq/games/one_d4/db/PostgresMigrationVerifyTest.java", "src/test/java/com/muchq/games/one_d4/db/PostgresPlayerIndexTest.java", "src/test/java/com/muchq/games/one_d4/db/PostgresReadTimeoutTest.java", "src/test/java/com/muchq/games/one_d4/db/PostgresRetentionIndexTest.java", diff --git a/domains/games/apis/one_d4/README.md b/domains/games/apis/one_d4/README.md index b0d0c1ab4..d8f72d186 100644 --- a/domains/games/apis/one_d4/README.md +++ b/domains/games/apis/one_d4/README.md @@ -108,6 +108,11 @@ disappears with the process. docker run -d --name one_d4_dev -p 5432:5432 \ -e POSTGRES_USER=indexer -e POSTGRES_PASSWORD=indexer -e POSTGRES_DB=indexer postgres:18 +INDEXER_DB_URL="jdbc:postgresql://localhost:5432/indexer" \ + INDEXER_DB_USERNAME=indexer \ + INDEXER_DB_PASSWORD=indexer \ + bazel run //domains/games/apis/one_d4:one_d4_migrate + INDEXER_DB_URL="jdbc:postgresql://localhost:5432/indexer" \ INDEXER_DB_USERNAME=indexer \ INDEXER_DB_PASSWORD=indexer \ @@ -115,13 +120,16 @@ INDEXER_DB_URL="jdbc:postgresql://localhost:5432/indexer" \ ``` `postgres:18` is the image the deploy runs (`shared_postgres` in `compose.yaml`), so local -dev and production speak the same dialect. Migrations run at startup against an empty -database, so nothing else is needed to bring one up. +dev and production speak the same dialect. + +The migrate step is not optional: the service creates no schema of its own. It checks at +boot that the migrations were applied and refuses to serve otherwise (#1426), naming what +is missing. Re-run it after pulling a new `V` step. -The schema itself is the numbered `.sql` files in [`migrations/`](migrations/) (#1419) — -`Migration` applies them at boot, and the deploy runs the same files first as the -`one_d4_migrate` one-shot (`compose.yaml`), which is what lets `one_d4_worker` start -without waiting for this service. `migrations/README.md` has the authoring rules. +The schema itself is the numbered `.sql` files in [`migrations/`](migrations/) (#1419). +`one_d4_migrate` is the same one-shot the deploy runs before the services start +(`compose.yaml`), which is what lets `one_d4_worker` start without waiting for this +service. `migrations/README.md` has the authoring rules. Credentials in the URL still work, but only if the password survives URL decoding — pgjdbc decodes query values, so `+` becomes a space, `&` truncates the rest, and a bare diff --git a/domains/games/apis/one_d4/migrations/README.md b/domains/games/apis/one_d4/migrations/README.md index e9fd3941f..c2c532cd7 100644 --- a/domains/games/apis/one_d4/migrations/README.md +++ b/domains/games/apis/one_d4/migrations/README.md @@ -1,10 +1,10 @@ # one_d4 schema migrations The schema as numbered, idempotent SQL files (#1419). This directory is the -one copy of the DDL: the Java service applies it at boot, the -`one_d4_migrate` deploy step applies it before the services start, and -`one_d4_worker`'s `schema_contract_test` reads it to hold the C++ fixtures -to it. +one copy of the DDL: the `one_d4_migrate` deploy step applies it before the +services start, the Java service verifies at boot that it did, and +`one_d4_worker`'s Postgres suites apply it to build the schema they test +against. ## Layout @@ -25,32 +25,34 @@ notes in `V009__dedupe_key.sql` for why that is load-bearing). - **Idempotent, always.** Every statement must be safe to re-run: `IF NOT EXISTS`, `IF EXISTS`, `DO $$ ... EXCEPTION` blocks, guarded UPDATEs. There is no tracking table; re-running everything *is* the - mechanism, and the Java service re-runs it on every boot (#1426 tracks - demoting that boot-time run to a verifier — and it is where the tracking - table question reopens, if boot time ever grows with the step count or a - step arrives that cannot be written idempotently). + mechanism, and it is what lets the deploy step run on every deploy. What + would end that: boot/deploy time growing with the step count, or a step + that cannot be written idempotently (a backfill too expensive to guard). + Neither exists yet. + + Idempotence is also what `Migration.verify()` leans on — it applies the + steps to an empty scratch schema and compares, so a step that only works + against a populated database breaks boot verification, not just re-runs. - **Append, don't edit.** A schema change is a new `V` step: the next number, a line in `manifest.txt`, and the file named in *each* applicable `BUILD.bazel` list — `:migrations` (pg + shared, ships with the service), - `:h2_migrations` (h2, test-only), `:migrations_sql` (all of them, the C++ - contract test's data). A file unlisted in BUILD neither ships nor runs, - and no test can see it. Editing an old step is for comments only. -- **Plain SQL, executed one statement at a time.** Each `pg/` and shared - file also works under `psql -f`; nothing here depends on the runner (the - `h2/` files are H2 syntax and are not psql-compatible). Statements are - split on top-level semicolons — dollar-quoting, `''` escapes and comments - respected — and executed individually, exactly as the old Java constants - were: whole-file execute would trade that for the driver's own - multi-statement semantics and lose which step failed. The splitter does - not model double-quoted identifiers or `E''` strings, so don't use them - (none of the schema needs either). + `:h2_migrations` (h2, test-only), `:migrations_sql` (all of them, what + the C++ suites run and walk). A file unlisted in BUILD neither ships nor + runs, and no test can see it. Editing an old step is for comments only. +- **Plain SQL, and a whole file has to work as one script.** Each `pg/` and + shared file also works under `psql -f`; nothing here depends on the runner + (the `h2/` files are H2 syntax and are not psql-compatible). Java splits + on top-level semicolons — dollar-quoting, `''` escapes and comments + respected — and executes them individually, so a failure names its step; + the splitter does not model double-quoted identifiers or `E''` strings, so + don't use them (none of the schema needs either). `one_d4_worker`'s + Postgres suites send each `pg/` or shared file whole through libpq + instead (`migration_files`), which puts its statements in one implicit + transaction — so no step may depend on an earlier statement in the same + file having committed. - **Forked steps stay in step.** Both engines run the same step list in the same order; only a step's SQL may differ. If you add a partial index on Postgres, add the H2 stand-in with the same name (see `V016`, `V017`). -- **The C++ fixtures are held to `pg/` + shared.** - `schema_contract_test` scrapes `CREATE TABLE` bodies and - `ALTER TABLE ... ADD COLUMN IF NOT EXISTS` lines, so keep one column per - line in CREATE bodies and each ALTER on a single line. ## Running them by hand @@ -64,6 +66,6 @@ done The deploy step (`//domains/games/apis/one_d4:one_d4_migrate`, a one-shot compose service) does the same through the Java `Migration` class, so the statements production runs are the ones the tests ran. Both `one_d4` and -`one_d4_worker` gate on it — the service so the two migration runners are -serialized rather than concurrent, the worker so it can start without the -Java service at all. +`one_d4_worker` gate on it — the service because its boot check would +otherwise run against a schema the one-shot has not finished writing, the +worker because it can then start without the Java service at all. diff --git a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/IndexerModule.java b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/IndexerModule.java index 6318d738a..aed58cc15 100644 --- a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/IndexerModule.java +++ b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/IndexerModule.java @@ -128,10 +128,15 @@ static String resolveJdbcUrl(@Nullable String configuredUrl) { return configuredUrl == null || configuredUrl.isBlank() ? readJdbcUrl() : configuredUrl.strip(); } + /** + * The schema is {@code one_d4_migrate}'s to write (#1426); boot's job is to refuse to serve + * against one that step did not finish. The H2 test path has no such step and applies the + * migrations itself. + */ @Context public Migration migration(DataSource dataSource, SqlDialect dialect) { Migration migration = new Migration(dataSource, dialect); - migration.run(); + migration.atBoot(); return migration; } diff --git a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/Migration.java b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/Migration.java index 5151254fa..c2084cdad 100644 --- a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/Migration.java +++ b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/Migration.java @@ -1,31 +1,64 @@ package com.muchq.games.one_d4.db; import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; import java.sql.SQLException; import java.sql.Statement; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.UUID; import javax.sql.DataSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Applies the schema: the numbered, idempotent .sql files under {@code migrations/} (#1419), in - * manifest order, resolved for this dialect's engine by {@link MigrationFiles} and split into - * statements by {@link SqlStatements}. The files are the one copy of the DDL; this class carries - * none. + * The schema: the numbered, idempotent .sql files under {@code migrations/} (#1419), in manifest + * order, resolved for this dialect's engine by {@link MigrationFiles} and split into statements by + * {@link SqlStatements}. The files are the one copy of the DDL; this class carries none. * - *

Runs as the standalone {@code one_d4_migrate} deploy step ({@link MigrationRunner}) and - * then again at service boot (an {@code IndexerModule} {@code @Context} bean) — the compose - * gate serializes the two, since the statements are idempotent but not concurrency-safe. Re-running - * everything is the whole mechanism: there is no tracking table. #1426 tracks demoting the - * boot-time run to a verifier. See {@code migrations/README.md} for the authoring rules. + *

{@link #run} applies them — the standalone {@code one_d4_migrate} deploy step ({@link + * MigrationRunner}), and the H2 test path, which has no deploy step in front of it. There is no + * tracking table: re-running everything is the whole mechanism, which is what makes the two callers + * interchangeable. See {@code migrations/README.md} for the authoring rules. * - *

No transaction wraps the run, matching the statement-at-a-time behavior the DDL has always had - * here: a failure stops at the failing step, leaves the earlier idempotent steps applied, and names - * the step it died in. + *

{@link #verify} checks they have already been applied, and writes nothing. That is what the + * service runs at boot (#1426), so nothing but {@code one_d4_migrate} writes the deployed schema. + * + *

No transaction wraps {@link #run}, matching the statement-at-a-time behavior the DDL has + * always had here: a failure stops at the failing step, leaves the earlier idempotent steps + * applied, and names the step it died in. */ public class Migration { private static final Logger LOG = LoggerFactory.getLogger(Migration.class); + /** + * Everything a schema holds that a migration step can create, by name. Types are deliberately not + * compared: both runners read one set of files, and a boot that refuses to serve is too blunt an + * answer to a column somebody widened by hand. + */ + private static final String OBJECTS_IN_SCHEMA = + """ + SELECT 'table ' || table_name FROM information_schema.tables WHERE table_schema = ? + UNION ALL + SELECT 'column ' || table_name || '.' || column_name FROM information_schema.columns + WHERE table_schema = ? + UNION ALL + SELECT 'index ' || indexname FROM pg_indexes WHERE schemaname = ? + UNION ALL + SELECT 'constraint ' || conname FROM pg_constraint c + JOIN pg_namespace n ON n.oid = c.connamespace WHERE n.nspname = ? + """; + + /** Enough of the list to act on; the count above it says how much was elided. */ + private static final int NAMED_IN_FAILURE = 20; + + /** Missing tables first, then the columns of tables that do exist, then what indexes them. */ + private static final List KINDS = List.of("table", "column", "index", "constraint"); + private final DataSource dataSource; private final SqlDialect dialect; @@ -34,22 +67,150 @@ public Migration(DataSource dataSource, SqlDialect dialect) { this.dialect = dialect; } + /** + * What the service does about the schema when it starts: check it, or build it. Postgres has + * {@code one_d4_migrate} in front of it and gets {@link #verify}; the H2 test path has nothing in + * front of it and gets {@link #run}. Here rather than in {@code IndexerModule} so the choice is + * reachable from a test that can watch what it wrote. + */ + public void atBoot() { + if (dialect.migratedBeforeBoot()) { + verify(); + } else { + run(); + } + } + public void run() { - String engine = dialect.migrationsEngine(); try (Connection conn = dataSource.getConnection(); Statement stmt = conn.createStatement()) { - for (String step : MigrationFiles.steps()) { - for (String sql : SqlStatements.split(MigrationFiles.sqlFor(step, engine))) { - try { - stmt.execute(sql); - } catch (SQLException e) { - throw new RuntimeException("Migration step " + step + " failed on " + engine, e); - } - } - } + apply(stmt); LOG.info("Database migration completed successfully"); } catch (SQLException e) { throw new RuntimeException("Failed to run database migration", e); } } + + /** + * Refuses to return if the deployed schema is missing anything the migrations create. + * + *

What to expect comes from the files rather than from a list anyone maintains: the migrations + * run into a scratch schema, and what they build there is compared with what the service's own + * schema holds. It happens inside a transaction that is always rolled back, so the scratch schema + * is never committed and the live tables are never touched — every statement runs against the + * empty copies. + * + *

Postgres only: it needs DDL to be transactional. The H2 path calls {@link #run} instead + * ({@link SqlDialect#migratedBeforeBoot}). + */ + public void verify() { + String scratch = "one_d4_verify_" + UUID.randomUUID().toString().replace("-", ""); + try (Connection conn = dataSource.getConnection()) { + boolean autoCommit = conn.getAutoCommit(); + conn.setAutoCommit(false); + try { + List missing = missingFrom(conn, scratch); + if (!missing.isEmpty()) { + throw new IllegalStateException( + "The database is missing " + + missing.size() + + " object(s) the migrations create, so one_d4_migrate has not completed against" + + " it: " + + String.join( + ", ", missing.subList(0, Math.min(missing.size(), NAMED_IN_FAILURE))) + + (missing.size() > NAMED_IN_FAILURE ? ", ..." : "")); + } + } finally { + // Undoes the scratch schema and the search_path with it, whichever way this leaves. + conn.rollback(); + conn.setAutoCommit(autoCommit); + } + LOG.info("Database schema verified against migrations/"); + } catch (SQLException e) { + throw new RuntimeException("Failed to verify database schema", e); + } + } + + /** Objects the migrations build that the connection's own schema does not have, sorted. */ + private List missingFrom(Connection conn, String scratch) throws SQLException { + String live; + try (Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery("SELECT current_schema()")) { + rs.next(); + live = rs.getString(1); + } + Set present = objectsIn(conn, live); + + try (Statement stmt = conn.createStatement()) { + stmt.execute("CREATE SCHEMA " + scratch); + } + setLocalSearchPath(conn, scratch); + try (Statement stmt = conn.createStatement()) { + apply(stmt); + } + + List missing = new ArrayList<>(); + for (String object : objectsIn(conn, scratch)) { + if (!present.contains(object)) { + missing.add(object); + } + } + // A table nobody created is missing every column too, and listing them buries the one line + // that says what to do about it. + Set absentTables = new HashSet<>(); + for (String object : missing) { + if (object.startsWith("table ")) { + absentTables.add(object.substring("table ".length())); + } + } + missing.removeIf( + object -> + object.startsWith("column ") + && absentTables.contains( + object.substring("column ".length(), object.lastIndexOf('.')))); + missing.sort( + Comparator.comparingInt(Migration::kindOf).thenComparing(Comparator.naturalOrder())); + return missing; + } + + private static int kindOf(String object) { + return KINDS.indexOf(object.substring(0, object.indexOf(' '))); + } + + private void apply(Statement stmt) throws SQLException { + String engine = dialect.migrationsEngine(); + for (String step : MigrationFiles.steps()) { + for (String sql : SqlStatements.split(MigrationFiles.sqlFor(step, engine))) { + try { + stmt.execute(sql); + } catch (SQLException e) { + throw new RuntimeException("Migration step " + step + " failed on " + engine, e); + } + } + } + } + + private static Set objectsIn(Connection conn, String schema) throws SQLException { + Set objects = new HashSet<>(); + try (PreparedStatement stmt = conn.prepareStatement(OBJECTS_IN_SCHEMA)) { + for (int i = 1; i <= 4; i++) { + stmt.setString(i, schema); + } + try (ResultSet rs = stmt.executeQuery()) { + while (rs.next()) { + objects.add(rs.getString(1)); + } + } + } + return objects; + } + + /** Transaction-scoped, so the rollback that ends verify() puts the pooled connection back. */ + private static void setLocalSearchPath(Connection conn, String schema) throws SQLException { + try (PreparedStatement stmt = + conn.prepareStatement("SELECT set_config('search_path', ?, true)")) { + stmt.setString(1, schema); + stmt.execute(); + } + } } diff --git a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/MigrationRunner.java b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/MigrationRunner.java index e937bb456..f290178b7 100644 --- a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/MigrationRunner.java +++ b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/MigrationRunner.java @@ -10,9 +10,9 @@ * a one-shot compose service before {@code one_d4_worker} and {@code one_d4} start, so the C++ * poller never races the Java service for a schema neither of them has created yet. * - *

Same statements, same order, same code as the service's own boot-time {@link Migration} — the - * two paths run in sequence (the compose gate serializes them) until #1426 demotes the boot-time - * run to a verifier. + *

The only writer of the deployed schema. Both gated services boot behind it: one_d4_worker + * because it creates nothing, one_d4 because its boot checks this step finished ({@link + * Migration#verify}) rather than applying anything itself. * *

Reads {@code $INDEXER_DB_URL} (plus {@code $INDEXER_DB_USERNAME}/{@code $INDEXER_DB_PASSWORD}) * exactly as the service does, and like the service it has no fallback: a migrate step that diff --git a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/SqlDialect.java b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/SqlDialect.java index 937370a0c..28bd62b47 100644 --- a/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/SqlDialect.java +++ b/domains/games/apis/one_d4/src/main/java/com/muchq/games/one_d4/db/SqlDialect.java @@ -24,4 +24,16 @@ public interface SqlDialect { * differs. See {@code migrations/README.md}. */ String migrationsEngine(); + + /** + * Whether something has already applied the migrations by the time this service boots. Postgres + * has the {@code one_d4_migrate} one-shot in front of it, so boot runs {@link Migration#verify} + * and writes no schema; the H2 test path has nothing in front of it and applies them itself. + * + *

Defaults to true so a dialect added for a deployed engine cannot become a second writer of + * the schema by omission. + */ + default boolean migratedBeforeBoot() { + return true; + } } diff --git a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/H2SqlDialect.java b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/H2SqlDialect.java index aa91d3b6d..e0bac82f7 100644 --- a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/H2SqlDialect.java +++ b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/H2SqlDialect.java @@ -40,4 +40,10 @@ public String upsertIndexedPeriod() { public String migrationsEngine() { return "h2"; } + + /** No deploy step runs ahead of an in-memory database, so the H2 path migrates at boot. */ + @Override + public boolean migratedBeforeBoot() { + return false; + } } diff --git a/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/PostgresMigrationVerifyTest.java b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/PostgresMigrationVerifyTest.java new file mode 100644 index 000000000..22ea17fe4 --- /dev/null +++ b/domains/games/apis/one_d4/src/test/java/com/muchq/games/one_d4/db/PostgresMigrationVerifyTest.java @@ -0,0 +1,156 @@ +package com.muchq.games.one_d4.db; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.Statement; +import javax.sql.DataSource; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * What one_d4 runs at boot instead of migrating (#1426). Needs the deployment engine: the check + * builds the expected schema inside a transaction it rolls back, which only holds where DDL is + * transactional. + */ +public class PostgresMigrationVerifyTest { + + private static final String SCHEMA = "migration_verify_test"; + + private String rawUrl; + private String jdbcUrl; + + @BeforeEach + public void setUp() throws Exception { + rawUrl = System.getenv("PG_TEST_DB_URL"); + assumeTrue( + rawUrl != null && !rawUrl.isBlank(), + "PG_TEST_DB_URL is not set; skipping the real-postgres verify suite"); + jdbcUrl = PgTestUrls.jdbcUrl(rawUrl, SCHEMA); + + exec("DROP SCHEMA IF EXISTS " + SCHEMA + " CASCADE", null); + exec("CREATE SCHEMA " + SCHEMA, null); + } + + @AfterEach + public void tearDown() throws Exception { + if (rawUrl == null || rawUrl.isBlank()) { + return; + } + exec("DROP SCHEMA IF EXISTS " + SCHEMA + " CASCADE", null); + } + + @Test + public void passesOnceTheMigrateStepHasRun() { + assertThat(MigrationRunner.run(jdbcUrl, null, null)).isZero(); + + assertThatCode(() -> verifier().verify()).doesNotThrowAnyException(); + } + + @Test + public void failsOnADatabaseNobodyMigrated() { + assertThatThrownBy(() -> verifier().verify()) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("one_d4_migrate") + .hasMessageContaining("table indexing_requests"); + } + + @Test + public void namesTheColumnAMissingStepWouldHaveAdded() throws Exception { + assertThat(MigrationRunner.run(jdbcUrl, null, null)).isZero(); + exec("ALTER TABLE indexing_requests DROP COLUMN dedupe_key CASCADE", SCHEMA); + + assertThatThrownBy(() -> verifier().verify()) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("column indexing_requests.dedupe_key"); + } + + @Test + public void namesTheIndexAMissingStepWouldHaveBuilt() throws Exception { + assertThat(MigrationRunner.run(jdbcUrl, null, null)).isZero(); + exec("DROP INDEX idx_game_features_white_username", SCHEMA); + + assertThatThrownBy(() -> verifier().verify()) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("index idx_game_features_white_username"); + } + + /** + * The wiring, not just the method: what the service calls at startup on the dialect it ships + * checks the schema rather than creating it. + */ + @Test + public void bootDoesNotCreateTheSchemaOnTheDeploymentDialect() throws Exception { + assertThatThrownBy(() -> verifier().atBoot()).isInstanceOf(IllegalStateException.class); + + assertThat( + scalar( + "SELECT count(*) FROM information_schema.tables WHERE table_schema = '" + + SCHEMA + + "'")) + .as("boot built the schema instead of refusing to serve without it") + .isEqualTo("0"); + } + + /** + * The point of the demotion: boot stops being a writer. A verify that repaired what it found, or + * that left its scratch schema behind, would be a second writer wearing a different name. + */ + @Test + public void writesNothing() throws Exception { + assertThat(MigrationRunner.run(jdbcUrl, null, null)).isZero(); + exec("ALTER TABLE indexing_requests DROP COLUMN dedupe_key CASCADE", SCHEMA); + exec( + "INSERT INTO indexing_requests (player, platform, start_month, end_month)" + + " VALUES ('alice', 'chess.com', '2026-01', '2026-01')", + SCHEMA); + + assertThatThrownBy(() -> verifier().verify()).isInstanceOf(IllegalStateException.class); + assertThatThrownBy(() -> verifier().verify()).isInstanceOf(IllegalStateException.class); + + assertThat( + scalar( + "SELECT count(*) FROM information_schema.columns WHERE table_schema = '" + + SCHEMA + + "' AND table_name = 'indexing_requests'" + + " AND column_name = 'dedupe_key'")) + .as("the failed verify put the column back") + .isEqualTo("0"); + assertThat( + scalar( + "SELECT count(*) FROM information_schema.schemata" + + " WHERE schema_name LIKE 'one_d4_verify_%'")) + .as("a scratch schema outlived the transaction that built it") + .isEqualTo("0"); + assertThat(scalar("SELECT count(*) FROM " + SCHEMA + ".indexing_requests")) + .as("the rows the verify ran over") + .isEqualTo("1"); + } + + private Migration verifier() { + DataSource dataSource = DataSourceFactory.create(jdbcUrl, null, null); + return new Migration(dataSource, new PostgresSqlDialect()); + } + + private void exec(String sql, String schema) throws Exception { + try (Connection conn = DriverManager.getConnection(PgTestUrls.jdbcUrl(rawUrl, schema)); + Statement stmt = conn.createStatement()) { + stmt.execute(sql); + } + } + + private String scalar(String sql) throws Exception { + try (Connection conn = DriverManager.getConnection(PgTestUrls.jdbcUrl(rawUrl, null)); + Statement stmt = conn.createStatement(); + ResultSet rs = stmt.executeQuery(sql)) { + rs.next(); + return rs.getString(1); + } + } +} diff --git a/domains/games/apis/one_d4_worker/BUILD.bazel b/domains/games/apis/one_d4_worker/BUILD.bazel index caab4ebcd..74d5cc716 100644 --- a/domains/games/apis/one_d4_worker/BUILD.bazel +++ b/domains/games/apis/one_d4_worker/BUILD.bazel @@ -132,6 +132,25 @@ cc_library( ], ) +# The deployed schema, for the suites that need one (#1401). Applies +# one_d4's migrations/ .sql files rather than a hand-copy of the DDL, so a +# Postgres suite here runs against the tables the one_d4_migrate step +# creates. Test-only: this worker polls those tables and never creates them. +cc_library( + name = "migration_files", + testonly = True, + srcs = ["migration_files.cc"], + hdrs = ["migration_files.h"], + data = ["//domains/games/apis/one_d4:migrations_sql"], + visibility = ["//domains/games:__subpackages__"], + deps = [ + "//domains/platform/libs/pg", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings", + ], +) + cc_test( name = "retention_policy_test", size = "small", @@ -190,6 +209,7 @@ cc_test( env_inherit = ["PG_TEST_DB_URL"], tags = ["requires-postgres"], deps = [ + ":migration_files", ":retention", "//domains/platform/libs/pg", "@com_google_absl//absl/strings", @@ -223,6 +243,7 @@ cc_test( srcs = ["pg_queue_test.cc"], env_inherit = ["PG_TEST_DB_URL"], deps = [ + ":migration_files", ":pg_queue", "//domains/platform/libs/pg", "@com_google_absl//absl/strings", @@ -297,6 +318,7 @@ cc_test( srcs = ["pg_reanalysis_test.cc"], env_inherit = ["PG_TEST_DB_URL"], deps = [ + ":migration_files", ":pg_reanalysis", "//domains/games/libs/one_d4_motifs:occurrence", "@com_google_absl//absl/strings", @@ -359,6 +381,7 @@ cc_test( srcs = ["reanalysis_queue_test.cc"], env_inherit = ["PG_TEST_DB_URL"], deps = [ + ":migration_files", ":reanalysis_queue", "//domains/platform/libs/pg", "@com_google_absl//absl/strings", @@ -374,18 +397,15 @@ cc_test( name = "schema_contract_test", size = "small", srcs = ["schema_contract_test.cc"], - data = [ - "pg_game_sink_test.cc", - "pg_queue_test.cc", - "pg_reanalysis_test.cc", - "reanalysis_queue_test.cc", - "//domains/games/apis/one_d4:migrations_sql", - "//domains/games/apis/one_d4:retention_policy_json", - ], + data = ["//domains/games/apis/one_d4:retention_policy_json"], deps = [ + ":migration_files", ":poller", ":poller_options", ":retention_policy", + "//domains/platform/libs/pg", + "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@googletest//:gtest_main", @@ -549,6 +569,7 @@ cc_test( srcs = ["pg_game_sink_test.cc"], env_inherit = ["PG_TEST_DB_URL"], deps = [ + ":migration_files", ":pg_game_sink", "//domains/platform/libs/pg", "@com_google_absl//absl/status", @@ -643,15 +664,6 @@ cc_binary( linux_amd64_oci_binary(bin_name = "one_d4_worker") -# The service name this worker reports as. prom_proxy's indexing selectors -# have to cover it, and nothing else ties the two together. -filegroup( - name = "worker_service_name", - testonly = True, - srcs = ["worker_main.cc"], - visibility = ["//domains/platform:__subpackages__"], -) - # The ceiling is a relationship between the run's checkpoints and the # lease's renewal, so it is tested through the real lease rather than a # LeaseKeeper that answers as told — a fake that decides both cannot show diff --git a/domains/games/apis/one_d4_worker/migration_files.cc b/domains/games/apis/one_d4_worker/migration_files.cc new file mode 100644 index 000000000..3d6a11eb2 --- /dev/null +++ b/domains/games/apis/one_d4_worker/migration_files.cc @@ -0,0 +1,106 @@ +#include "domains/games/apis/one_d4_worker/migration_files.h" + +#include +#include +#include +#include +#include + +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/ascii.h" +#include "absl/strings/str_cat.h" + +namespace one_d4_worker { +namespace { + +constexpr char kRoot[] = "domains/games/apis/one_d4/migrations"; + +absl::StatusOr Read(const std::string& path) { + std::ifstream file(path); + if (!file.good()) return absl::NotFoundError(absl::StrCat("no migration file at ", path)); + std::ostringstream contents; + contents << file.rdbuf(); + return contents.str(); +} + +/// Schema names reach the DDL below by concatenation, so only the shape a +/// suite name has is accepted. +bool IsPlainIdentifier(const std::string& name) { + if (name.empty() || absl::ascii_isdigit(name.front())) return false; + for (const char c : name) { + if (!absl::ascii_islower(c) && !absl::ascii_isdigit(c) && c != '_') return false; + } + return true; +} + +} // namespace + +std::string MigrationsRoot() { return kRoot; } + +absl::StatusOr> MigrationSteps() { + const absl::StatusOr manifest = Read(absl::StrCat(kRoot, "/manifest.txt")); + if (!manifest.ok()) return manifest.status(); + + std::istringstream lines(*manifest); + std::vector steps; + std::string line; + while (std::getline(lines, line)) { + absl::StripAsciiWhitespace(&line); + if (!line.empty() && line[0] != '#') steps.push_back(line); + } + if (steps.empty()) return absl::NotFoundError("manifest.txt names no steps"); + return steps; +} + +absl::StatusOr MigrationSqlPath(const std::string& step, const std::string& engine) { + const std::string engine_path = absl::StrCat(kRoot, "/", engine, "/", step, ".sql"); + const std::string shared_path = absl::StrCat(kRoot, "/", step, ".sql"); + const bool engine_exists = std::filesystem::exists(engine_path); + const bool shared_exists = std::filesystem::exists(shared_path); + if (engine_exists && shared_exists) { + return absl::FailedPreconditionError(absl::StrCat(step, " has both ", engine_path, " and ", + shared_path, + " — a forked step has no shared file")); + } + if (!engine_exists && !shared_exists) { + return absl::NotFoundError(absl::StrCat(step, " has no SQL for ", engine, " — expected ", + engine_path, " or ", shared_path)); + } + return engine_exists ? engine_path : shared_path; +} + +absl::Status ResetToMigratedSchema(pg::Client& client, const std::string& schema) { + if (!IsPlainIdentifier(schema)) { + return absl::InvalidArgumentError(absl::StrCat("not a plain schema name: ", schema)); + } + if (const absl::Status dropped = client.ExecScript( + absl::StrCat("DROP SCHEMA IF EXISTS ", schema, " CASCADE; CREATE SCHEMA ", schema, ";")); + !dropped.ok()) { + return dropped; + } + + const absl::StatusOr current = client.Exec("SELECT current_schema()"); + if (!current.ok()) return current.status(); + if (current->Get(0, 0) != schema) { + return absl::FailedPreconditionError( + absl::StrCat("unqualified names on this connection resolve to ", + current->Get(0, 0).value_or("(none)"), ", not ", schema)); + } + + const absl::StatusOr> steps = MigrationSteps(); + if (!steps.ok()) return steps.status(); + for (const std::string& step : *steps) { + const absl::StatusOr path = MigrationSqlPath(step, "pg"); + if (!path.ok()) return path.status(); + const absl::StatusOr sql = Read(*path); + if (!sql.ok()) return sql.status(); + if (const absl::Status applied = client.ExecScript(*sql); !applied.ok()) { + return absl::Status(applied.code(), + absl::StrCat("migration step ", step, ": ", applied.message())); + } + } + return absl::OkStatus(); +} + +} // namespace one_d4_worker diff --git a/domains/games/apis/one_d4_worker/migration_files.h b/domains/games/apis/one_d4_worker/migration_files.h new file mode 100644 index 000000000..a6100b1f7 --- /dev/null +++ b/domains/games/apis/one_d4_worker/migration_files.h @@ -0,0 +1,45 @@ +#ifndef DOMAINS_GAMES_APIS_ONE_D4_WORKER_MIGRATION_FILES_H +#define DOMAINS_GAMES_APIS_ONE_D4_WORKER_MIGRATION_FILES_H + +#include +#include + +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "domains/platform/libs/pg/pg.h" + +namespace one_d4_worker { + +// The one_d4 schema, read from the numbered .sql files under +// one_d4/migrations (#1419) — the same files the Java Migration and the +// one_d4_migrate deploy step run against production, reached through +// runfiles instead of a classpath. MigrationFiles.java is the twin, and the +// resolution rule below is its rule. +// +// Test-only: the worker polls these tables and never creates them. What it +// buys is that a suite's tables are the deployed tables, so a column a +// migration changes fails the suite that reads it instead of passing +// against a fixture's copy. + +/// The directory the files are read from, relative to the runfiles root. +std::string MigrationsRoot(); + +/// The manifest's step names, in the order they run. +absl::StatusOr> MigrationSteps(); + +/// The file one step resolves to for one engine ("pg" or "h2"): +/// `/.sql` when the engines fork, `.sql` when they +/// agree. Both or neither is refused rather than guessed around. +absl::StatusOr MigrationSqlPath(const std::string& step, const std::string& engine); + +/// Drops `schema`, recreates it, and runs the Postgres migrations into it in +/// manifest order. `client` is the connection the caller goes on to test +/// against; it must resolve unqualified names to `schema`, and that is +/// checked rather than assumed — a suite whose search_path went missing +/// would otherwise build its tables in public alongside every sibling +/// suite's rows, and answer counting questions for all of them. +absl::Status ResetToMigratedSchema(pg::Client& client, const std::string& schema); + +} // namespace one_d4_worker + +#endif // DOMAINS_GAMES_APIS_ONE_D4_WORKER_MIGRATION_FILES_H diff --git a/domains/games/apis/one_d4_worker/pg_game_sink_test.cc b/domains/games/apis/one_d4_worker/pg_game_sink_test.cc index fcf362b83..a3ffe5f11 100644 --- a/domains/games/apis/one_d4_worker/pg_game_sink_test.cc +++ b/domains/games/apis/one_d4_worker/pg_game_sink_test.cc @@ -14,6 +14,7 @@ #include "absl/strings/str_cat.h" #include "absl/synchronization/notification.h" #include "absl/time/clock.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { @@ -30,9 +31,9 @@ using ::testing::ElementsAre; constexpr char kRequest[] = "00000000-0000-4000-8000-000000000001"; constexpr char kOwner[] = "worker-1"; -/// This suite's own schema, so its DDL cannot race pg_queue_test's: both -/// drop and recreate indexing_requests, and bazel runs them at the same -/// time against one database. +/// A schema of this suite's own. Every suite here gets the one database CI +/// runs and bazel runs them in parallel, so the tables are shared mutable +/// state otherwise. constexpr char kSchema[] = "one_d4_game_sink_test"; /// The connection options carried in the conninfo rather than set with a @@ -54,92 +55,9 @@ class PgGameSinkTest : public testing::Test { void SetUp() override { const char* url = std::getenv("PG_TEST_DB_URL"); if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; - { - pg::Client bootstrap(url); - ASSERT_TRUE(bootstrap.Exec(absl::StrCat("CREATE SCHEMA IF NOT EXISTS ", kSchema)).ok()); - } conninfo_ = Conninfo(url); client_ = std::make_unique(conninfo_); - - // Column for column what PostgresSqlDialect creates and Migration adds. - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS indexed_periods").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS motif_occurrences").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS game_features").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS indexing_requests").ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE indexing_requests ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - player VARCHAR(255) NOT NULL, - platform VARCHAR(50) NOT NULL, - start_month VARCHAR(7) NOT NULL, - end_month VARCHAR(7) NOT NULL, - status VARCHAR(20) NOT NULL DEFAULT 'PENDING', - created_at TIMESTAMP NOT NULL DEFAULT now(), - updated_at TIMESTAMP NOT NULL DEFAULT now(), - owner_id VARCHAR(128), - lease_expires_at TIMESTAMP - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE game_features ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - request_id UUID NOT NULL REFERENCES indexing_requests(id), - game_url VARCHAR(1024) NOT NULL UNIQUE, - platform VARCHAR(50) NOT NULL, - white_username VARCHAR(255), - black_username VARCHAR(255), - white_elo INT, - black_elo INT, - white_title VARCHAR(10), - black_title VARCHAR(10), - time_class VARCHAR(50), - eco VARCHAR(10), - opening_name VARCHAR(255), - opening_family VARCHAR(255), - result VARCHAR(20), - played_at TIMESTAMP, - num_moves INT, - indexed_at TIMESTAMP NOT NULL DEFAULT now(), - pgn TEXT - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE motif_occurrences ( - id VARCHAR(36) NOT NULL PRIMARY KEY, - game_url VARCHAR(1024) NOT NULL - REFERENCES game_features(game_url) ON DELETE CASCADE, - motif VARCHAR(50) NOT NULL, - ply INT NOT NULL, - side VARCHAR(5) NOT NULL, - move_number INT NOT NULL, - description TEXT, - moved_piece VARCHAR(20), - attacker VARCHAR(20), - target VARCHAR(20), - is_discovered BOOLEAN NOT NULL DEFAULT FALSE, - is_mate BOOLEAN NOT NULL DEFAULT FALSE, - pin_type VARCHAR(8) - ))") - .ok()); - - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE indexed_periods ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - player VARCHAR(255) NOT NULL, - platform VARCHAR(50) NOT NULL, - year_month VARCHAR(7) NOT NULL, - fetched_at TIMESTAMP NOT NULL, - is_complete BOOLEAN NOT NULL, - games_count INT NOT NULL, - exclude_bullet BOOLEAN NOT NULL DEFAULT FALSE, - CONSTRAINT indexed_periods_unique - UNIQUE (player, platform, year_month, exclude_bullet) - ))") - .ok()); + ASSERT_TRUE(ResetToMigratedSchema(*client_, kSchema).ok()); ASSERT_TRUE(client_ ->Exec("INSERT INTO indexing_requests (id, player, platform, start_month," diff --git a/domains/games/apis/one_d4_worker/pg_queue_test.cc b/domains/games/apis/one_d4_worker/pg_queue_test.cc index e9c288ca7..864011d75 100644 --- a/domains/games/apis/one_d4_worker/pg_queue_test.cc +++ b/domains/games/apis/one_d4_worker/pg_queue_test.cc @@ -8,6 +8,7 @@ #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { @@ -24,41 +25,27 @@ namespace { /// reached, not what the deployment sets it to. constexpr int kMaxAttempts = 3; +/// A schema of this suite's own. Every suite here gets the one database CI +/// runs and bazel runs them in parallel, so the tables are shared mutable +/// state otherwise. +constexpr char kSchema[] = "one_d4_pg_queue_test"; + +/// search_path on the connection rather than a qualified name on every +/// statement: PgQueue's SQL is the production SQL, and production does not +/// qualify. +std::string Conninfo(const std::string& url) { + return absl::StrCat(url, url.find('?') == std::string::npos ? "?" : "&", + "options=-c%20search_path%3D", kSchema); +} + class PgQueueTest : public testing::Test { protected: void SetUp() override { const char* url = std::getenv("PG_TEST_DB_URL"); if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; - conninfo_ = url; + conninfo_ = Conninfo(url); client_ = std::make_unique(conninfo_); - - // Column for column what PostgresSqlDialect creates and Migration adds, - // types included — schema_contract_test is what keeps this copy honest. - // The id being UUID rather than text is the reason that test exists: a - // fixture that invents VARCHAR ids passes happily and tells you nothing - // about production. - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS indexing_requests").ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE indexing_requests ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - player VARCHAR(255) NOT NULL, - platform VARCHAR(50) NOT NULL, - start_month VARCHAR(7) NOT NULL, - end_month VARCHAR(7) NOT NULL, - status VARCHAR(20) NOT NULL DEFAULT 'PENDING', - created_at TIMESTAMP NOT NULL DEFAULT now(), - updated_at TIMESTAMP NOT NULL DEFAULT now(), - error_message TEXT, - games_indexed INT DEFAULT 0, - exclude_bullet BOOLEAN NOT NULL DEFAULT FALSE, - owner_id VARCHAR(128), - lease_expires_at TIMESTAMP, - skip_cache BOOLEAN DEFAULT FALSE, - attempts INT DEFAULT 0, - dedupe_key VARCHAR(600) - ))") - .ok()); + ASSERT_TRUE(ResetToMigratedSchema(*client_, kSchema).ok()); queue_ = std::make_unique(*client_, kMaxAttempts); } diff --git a/domains/games/apis/one_d4_worker/pg_reanalysis_test.cc b/domains/games/apis/one_d4_worker/pg_reanalysis_test.cc index c0f502428..cb3fb46be 100644 --- a/domains/games/apis/one_d4_worker/pg_reanalysis_test.cc +++ b/domains/games/apis/one_d4_worker/pg_reanalysis_test.cc @@ -13,6 +13,7 @@ #include "absl/strings/str_format.h" #include "absl/synchronization/notification.h" #include "absl/time/clock.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" namespace one_d4_worker { namespace { @@ -26,9 +27,13 @@ using ::testing::IsEmpty; std::string Url(int n) { return absl::StrFormat("https://chess.com/game/%04d", n); } -/// This suite's own schema. It drops and recreates three tables two other -/// suites also use, and bazel runs them at the same time against one -/// database — see pg_game_sink_test, which hit this first. +/// The indexing request every game here belongs to. game_features.request_id +/// is NOT NULL onto it; nothing in this pass reads the request itself. +constexpr char kRequest[] = "00000000-0000-4000-8000-000000000001"; + +/// A schema of this suite's own. Every suite here gets the one database CI +/// runs and bazel runs them in parallel, so the tables are shared mutable +/// state otherwise. constexpr char kSchema[] = "one_d4_pg_reanalysis_test"; /// Not UTC, deliberately: the fence compares lease_expires_at to NOW(), and @@ -45,72 +50,22 @@ class PgReanalysisTest : public testing::Test { void SetUp() override { const char* url = std::getenv("PG_TEST_DB_URL"); if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; - { - pg::Client bootstrap(url); - ASSERT_TRUE(bootstrap.Exec(absl::StrCat("CREATE SCHEMA IF NOT EXISTS ", kSchema)).ok()); - } conninfo_ = Conninfo(url); client_ = std::make_unique(conninfo_); - - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS motif_occurrences").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS game_features").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS reanalysis_requests").ok()); - - // Only the columns this pass reads or writes — schema_contract_test - // keeps the shape honest against the Java DDL. - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE game_features ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - game_url VARCHAR(1024) NOT NULL UNIQUE, - pgn TEXT - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE motif_occurrences ( - id VARCHAR(36) PRIMARY KEY, - game_url VARCHAR(1024) NOT NULL, - motif VARCHAR(50) NOT NULL, - ply INT NOT NULL, - side VARCHAR(5) NOT NULL, - move_number INT NOT NULL, - description TEXT, - moved_piece VARCHAR(20), - attacker VARCHAR(20), - target VARCHAR(20), - is_discovered BOOLEAN NOT NULL DEFAULT FALSE, - is_mate BOOLEAN NOT NULL DEFAULT FALSE, - pin_type VARCHAR(8) - ))") - .ok()); + ASSERT_TRUE(ResetToMigratedSchema(*client_, kSchema).ok()); ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE reanalysis_requests ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - status VARCHAR(20) NOT NULL DEFAULT 'PENDING', - created_at TIMESTAMP NOT NULL DEFAULT now(), - updated_at TIMESTAMP NOT NULL DEFAULT now(), - owner_id VARCHAR(128), - lease_expires_at TIMESTAMP, - attempts INT NOT NULL DEFAULT 0, - error_message TEXT, - cursor_game_url VARCHAR(1024), - games_processed INT NOT NULL DEFAULT 0, - games_failed INT NOT NULL DEFAULT 0 - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec("CREATE UNIQUE INDEX idx_reanalysis_requests_single_live ON " - "reanalysis_requests ((true)) WHERE status IN ('PENDING', " - "'PROCESSING')") + ->Exec("INSERT INTO indexing_requests (id, player, platform, start_month," + " end_month) VALUES ($1, 'alice', 'chess.com', '2026-01', '2026-01')", + {kRequest}) .ok()); } void AddGame(const std::string& url, const std::string& pgn) { - ASSERT_TRUE( - client_->Exec("INSERT INTO game_features (game_url, pgn) VALUES ($1, $2)", {url, pgn}) - .ok()); + ASSERT_TRUE(client_ + ->Exec("INSERT INTO game_features (request_id, game_url, platform, pgn)" + " VALUES ($1, $2, 'chess.com', $3)", + {kRequest, url, pgn}) + .ok()); } void AddOccurrence(const std::string& url, const std::string& motif) { @@ -207,8 +162,11 @@ TEST_F(PgReanalysisTest, AnEmptyCorpusPagesToNothing) { } TEST_F(PgReanalysisTest, ANullPgnComesBackEmptyRatherThanMissing) { - ASSERT_TRUE( - client_->Exec("INSERT INTO game_features (game_url, pgn) VALUES ($1, NULL)", {Url(0)}).ok()); + ASSERT_TRUE(client_ + ->Exec("INSERT INTO game_features (request_id, game_url, platform, pgn)" + " VALUES ($1, $2, 'chess.com', NULL)", + {kRequest, Url(0)}) + .ok()); PgGameCorpus corpus(*client_); auto page = corpus.After("", 10); diff --git a/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc b/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc index c4c37ed84..b1ac27bae 100644 --- a/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc +++ b/domains/games/apis/one_d4_worker/reanalysis_queue_test.cc @@ -9,6 +9,7 @@ #include "absl/strings/str_cat.h" #include "absl/time/time.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { @@ -48,36 +49,8 @@ class ReanalysisQueueTest : public testing::Test { void SetUp() override { const char* url = std::getenv("PG_TEST_DB_URL"); if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; - { - pg::Client bootstrap(url); - ASSERT_TRUE(bootstrap.Exec(absl::StrCat("CREATE SCHEMA IF NOT EXISTS ", kSchema)).ok()); - } client_ = std::make_unique(Conninfo(url)); - - // Column for column what PostgresSqlDialect creates — - // schema_contract_test is what keeps this copy honest. - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS reanalysis_requests").ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE reanalysis_requests ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - status VARCHAR(20) NOT NULL DEFAULT 'PENDING', - created_at TIMESTAMP NOT NULL DEFAULT now(), - updated_at TIMESTAMP NOT NULL DEFAULT now(), - owner_id VARCHAR(128), - lease_expires_at TIMESTAMP, - attempts INT NOT NULL DEFAULT 0, - error_message TEXT, - cursor_game_url VARCHAR(1024), - games_processed INT NOT NULL DEFAULT 0, - games_failed INT NOT NULL DEFAULT 0 - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec("CREATE UNIQUE INDEX idx_reanalysis_requests_single_live ON " - "reanalysis_requests ((true)) WHERE status IN ('PENDING', " - "'PROCESSING')") - .ok()); + ASSERT_TRUE(ResetToMigratedSchema(*client_, kSchema).ok()); queue_ = std::make_unique(*client_, kMaxAttempts); } diff --git a/domains/games/apis/one_d4_worker/retention_test.cc b/domains/games/apis/one_d4_worker/retention_test.cc index 6f4d960d8..b52039dc5 100644 --- a/domains/games/apis/one_d4_worker/retention_test.cc +++ b/domains/games/apis/one_d4_worker/retention_test.cc @@ -6,9 +6,12 @@ #include #include #include +#include +#include #include "absl/strings/str_cat.h" #include "absl/time/time.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { @@ -18,22 +21,10 @@ namespace { // anywhere holds a live lease", "a request outlives the games pointing at it" // — so they are tested against a real Postgres. The CI job supplies one, and // without PG_TEST_DB_URL these skip. -// -// The four tables the sweep touches, declared with the columns it keys on and -// not the rest — game_features has fifteen more in production that no arm here -// reads. So this is not a mirror of the migrations and schema_contract_test -// does not compare it against them the way it does pg_queue_test's. What it -// does pin is the part that would silently invalidate these tests: -// TheMigrationSchemaHasTheColumnsTheSweepKeysOn checks that every column named -// below still exists and that every timestamp compared against is still a -// naive TIMESTAMP. - -/// A schema of this suite's own, as pg_game_sink_test does. Every suite here -/// gets the one database CI runs, and bazel runs the targets in parallel, so -/// the public schema is shared mutable state: this suite's game_features -/// carries a foreign key onto indexing_requests, which is enough to make -/// pg_queue_test's `DROP TABLE indexing_requests` fail outright — a real -/// failure in a suite that changed nothing, blaming a table it does not own. + +/// A schema of this suite's own. Every suite here gets the one database CI +/// runs and bazel runs them in parallel, so the tables are shared mutable +/// state otherwise. constexpr char kSchema[] = "one_d4_retention_test"; /// search_path on the connection rather than a qualified name on every @@ -49,62 +40,8 @@ class RetentionTest : public testing::Test { void SetUp() override { const char* url = std::getenv("PG_TEST_DB_URL"); if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; - { - pg::Client bootstrap(url); - ASSERT_TRUE(bootstrap.Exec(absl::StrCat("CREATE SCHEMA IF NOT EXISTS ", kSchema)).ok()); - } client_ = std::make_unique(Conninfo(url)); - - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS motif_occurrences").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS game_features").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS indexed_periods").ok()); - ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS indexing_requests").ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE indexing_requests ( - id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - player VARCHAR(255) NOT NULL, - platform VARCHAR(50) NOT NULL, - start_month VARCHAR(7) NOT NULL, - end_month VARCHAR(7) NOT NULL, - status VARCHAR(20) NOT NULL DEFAULT 'PENDING', - created_at TIMESTAMP NOT NULL DEFAULT now(), - updated_at TIMESTAMP NOT NULL DEFAULT now(), - error_message TEXT, - games_indexed INT DEFAULT 0, - exclude_bullet BOOLEAN NOT NULL DEFAULT FALSE, - owner_id VARCHAR(128), - lease_expires_at TIMESTAMP, - skip_cache BOOLEAN DEFAULT FALSE, - attempts INT DEFAULT 0, - dedupe_key VARCHAR(600) - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE game_features ( - game_url VARCHAR(1024) PRIMARY KEY, - request_id UUID REFERENCES indexing_requests(id), - indexed_at TIMESTAMP NOT NULL DEFAULT now() - ))") - .ok()); - // The cascade is why the sweep does not delete motifs itself, and why the - // metric has no label for them. - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE motif_occurrences ( - id SERIAL PRIMARY KEY, - game_url VARCHAR(1024) NOT NULL - REFERENCES game_features(game_url) ON DELETE CASCADE - ))") - .ok()); - ASSERT_TRUE(client_ - ->Exec(R"( - CREATE TABLE indexed_periods ( - id SERIAL PRIMARY KEY, - fetched_at TIMESTAMP NOT NULL DEFAULT now() - ))") - .ok()); + ASSERT_TRUE(ResetToMigratedSchema(*client_, kSchema).ok()); } /// A request, with everything the arms key on stated explicitly. @@ -172,29 +109,27 @@ class RetentionTest : public testing::Test { } }; -/// The isolation itself, because nothing else here would notice losing it. -/// Without the search_path this suite builds its tables in public, where -/// game_features' foreign key onto indexing_requests makes pg_queue_test's -/// unqualified `DROP TABLE indexing_requests` fail — in a suite that changed -/// nothing, naming a table it does not own. These targets share the one -/// database CI runs and bazel schedules them in parallel, so the only version -/// of that bug that shows up locally is the one that already shipped. -TEST_F(RetentionTest, RunsInItsOwnSchemaRatherThanPublic) { - const auto schema = client_->Exec("SELECT current_schema()"); - ASSERT_TRUE(schema.ok()) << schema.status(); - EXPECT_EQ(schema->Get(0, 0).value_or("(null)"), kSchema); - - // And the unqualified name production uses resolves here, which - // current_schema() alone does not say — a search_path naming a schema that - // does not exist falls through to public with no error. Sibling suites keep - // an indexing_requests of their own, so the question is never "does this - // table exist" but "which one does Sweep's SQL reach". - const auto resolved = client_->Exec( - "SELECT n.nspname FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace" - " WHERE c.oid = to_regclass('indexing_requests')"); - ASSERT_TRUE(resolved.ok()) << resolved.status(); - ASSERT_EQ(resolved->rows(), 1) << "unqualified indexing_requests resolves to nothing"; - EXPECT_EQ(resolved->Get(0, 0).value_or("(null)"), kSchema); +// The sweep binds naive UTC literals to every timestamp it compares. A column +// migrated to TIMESTAMPTZ would be compared through the session's timezone +// instead and cut a different set of rows — identically to this one under the +// UTC stamps below, so it is asked of the schema rather than of a result. +TEST_F(RetentionTest, TheTimestampsTheSweepComparesAreNaive) { + for (const auto& [table, column] : + std::vector>{{"indexing_requests", "updated_at"}, + {"indexing_requests", "created_at"}, + {"indexing_requests", "lease_expires_at"}, + {"game_features", "indexed_at"}, + {"indexed_periods", "fetched_at"}}) { + const auto declared = client_->Exec( + "SELECT data_type FROM information_schema.columns WHERE table_schema = $1" + " AND table_name = $2 AND column_name = $3", + {kSchema, table, column}); + ASSERT_TRUE(declared.ok()) << declared.status(); + ASSERT_EQ(declared->rows(), 1) + << table << "." << column << " is gone, and the sweep keys on it"; + EXPECT_EQ(declared->Get(0, 0).value_or("(null)"), "timestamp without time zone") + << table << "." << column << " is not a naive TIMESTAMP, but the sweep binds one"; + } } TEST_F(RetentionTest, ReleasesALapsedClaimWithoutFailingIt) { @@ -319,15 +254,22 @@ TEST_F(RetentionTest, ReleasingDoesNotHideStalenessFromTheSameSweep) { TEST_F(RetentionTest, DeletesGamesAndPeriodsPastTheWindowAndCascadesMotifs) { Request("owner", "COMPLETE", kNow, "", absl::InfinitePast(), 0, kNow); ASSERT_TRUE(client_ - ->Exec(R"(INSERT INTO game_features (game_url, request_id, indexed_at) - VALUES ('old', $1::uuid, $2::timestamp), ('new', $1::uuid, $3::timestamp))", + ->Exec(R"(INSERT INTO game_features (game_url, platform, request_id, indexed_at) + VALUES ('old', 'chess.com', $1::uuid, $2::timestamp), + ('new', 'chess.com', $1::uuid, $3::timestamp))", {Id("owner"), Stamp(kNow - absl::Hours(24 * 8)), Stamp(kNow)}) .ok()); ASSERT_TRUE( - client_->Exec("INSERT INTO motif_occurrences (game_url) VALUES ('old'), ('new')").ok()); + client_ + ->Exec("INSERT INTO motif_occurrences (id, game_url, motif, ply, side, move_number)" + " VALUES (gen_random_uuid()::text, 'old', 'fork', 1, 'WHITE', 1)," + " (gen_random_uuid()::text, 'new', 'fork', 1, 'WHITE', 1)") + .ok()); ASSERT_TRUE(client_ - ->Exec(R"(INSERT INTO indexed_periods (fetched_at) - VALUES ($1::timestamp), ($2::timestamp))", + ->Exec(R"(INSERT INTO indexed_periods + (player, platform, year_month, is_complete, games_count, fetched_at) + VALUES ('alice', 'chess.com', '2026-01', TRUE, 1, $1::timestamp), + ('bob', 'chess.com', '2026-01', TRUE, 1, $2::timestamp))", {Stamp(kNow - absl::Hours(24 * 8)), Stamp(kNow)}) .ok()); @@ -348,8 +290,8 @@ TEST_F(RetentionTest, KeepsARequestWhileAnyGameStillPointsAtIt) { Request("referenced", "COMPLETE", kNow - absl::Hours(24 * 31), "", absl::InfinitePast(), 0, kNow - absl::Hours(24 * 31)); ASSERT_TRUE(client_ - ->Exec(R"(INSERT INTO game_features (game_url, request_id, indexed_at) - VALUES ('fresh', $1::uuid, $2::timestamp))", + ->Exec(R"(INSERT INTO game_features (game_url, platform, request_id, indexed_at) + VALUES ('fresh', 'chess.com', $1::uuid, $2::timestamp))", {Id("referenced"), Stamp(kNow)}) .ok()); @@ -366,8 +308,8 @@ TEST_F(RetentionTest, ARequestAndItsGamesClearInOnePass) { Request("aged", "COMPLETE", kNow - absl::Hours(24 * 31), "", absl::InfinitePast(), 0, kNow - absl::Hours(24 * 31)); ASSERT_TRUE(client_ - ->Exec(R"(INSERT INTO game_features (game_url, request_id, indexed_at) - VALUES ('stale', $1::uuid, $2::timestamp))", + ->Exec(R"(INSERT INTO game_features (game_url, platform, request_id, indexed_at) + VALUES ('stale', 'chess.com', $1::uuid, $2::timestamp))", {Id("aged"), Stamp(kNow - absl::Hours(24 * 31))}) .ok()); diff --git a/domains/games/apis/one_d4_worker/schema_contract_test.cc b/domains/games/apis/one_d4_worker/schema_contract_test.cc index cff5ee84e..2bf56de4f 100644 --- a/domains/games/apis/one_d4_worker/schema_contract_test.cc +++ b/domains/games/apis/one_d4_worker/schema_contract_test.cc @@ -1,145 +1,56 @@ -#include #include #include -#include -#include -#include #include -#include #include -#include #include -#include "absl/strings/ascii.h" +#include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/time/time.h" +#include "domains/games/apis/one_d4_worker/migration_files.h" #include "domains/games/apis/one_d4_worker/poller.h" #include "domains/games/apis/one_d4_worker/poller_options.h" #include "domains/games/apis/one_d4_worker/retention_policy.h" +#include "domains/platform/libs/pg/pg.h" namespace one_d4_worker { namespace { -// The schema lives in one_d4's migrations/ .sql files (#1419); this worker -// is a second poller on the tables they create. Nothing but agreement makes -// that safe, and the agreement is invisible from either side alone: -// pg_queue_test builds its own copy of the table, so a column a migration -// changes would leave the C++ tests green and production broken. +// What this worker shares with one_d4 and cannot see from its own side: the +// migration files that create the tables it polls, and the policy file both +// processes read. // -// The id column is why this exists. It is UUID, and a fixture that invents -// VARCHAR ids passes every test while telling you nothing. +// Column types are not checked here. The Postgres suites run the migration +// files themselves (migration_files), so a column that changed is a query +// that fails in the suite that reads it. What is left is whether every file +// runs at all, and what this worker does with the policy it loads. -constexpr char kMigrationsDir[] = "domains/games/apis/one_d4/migrations"; - -std::string Read(const std::string& path) { - std::ifstream file(path); - EXPECT_TRUE(file.good()) << "missing " << path; - std::ostringstream contents; - contents << file.rdbuf(); - return contents.str(); -} - -/// manifest.txt's step names, in order — the same list Migration runs. -std::vector ManifestSteps() { - std::istringstream lines(Read(absl::StrCat(kMigrationsDir, "/manifest.txt"))); - std::vector steps; - std::string line; - while (std::getline(lines, line)) { - absl::StripAsciiWhitespace(&line); - if (!line.empty() && line[0] != '#') steps.push_back(line); - } - return steps; -} - -/// One step's file for one engine, resolved the way Migration resolves it: -/// the engine directory when the engines fork, the shared file when they -/// agree — and never both, which would be two sources of truth. -std::string ResolveStep(const std::string& step, const std::string& engine) { - const std::string engine_path = absl::StrCat(kMigrationsDir, "/", engine, "/", step, ".sql"); - const std::string shared_path = absl::StrCat(kMigrationsDir, "/", step, ".sql"); - const bool engine_exists = std::filesystem::exists(engine_path); - const bool shared_exists = std::filesystem::exists(shared_path); - EXPECT_FALSE(engine_exists && shared_exists) - << step << " has both an " << engine << " file and a shared file"; - EXPECT_TRUE(engine_exists || shared_exists) << step << " has no file for " << engine; - return engine_exists ? engine_path : shared_path; -} - -/// Every statement the Postgres migration runs, concatenated in order. -const std::string& PostgresMigrationSql() { - static const std::string* const sql = [] { - auto* all = new std::string; - for (const std::string& step : ManifestSteps()) { - absl::StrAppend(all, Read(ResolveStep(step, "pg")), "\n"); - } - return all; - }(); - return *sql; -} - -/// column name -> declared type, from a CREATE TABLE body plus any ALTER -/// TABLE ... ADD COLUMN for the same table. -std::map Columns(const std::string& source, const std::string& table) { - std::map columns; - - const std::regex create( - absl::StrCat("CREATE TABLE (IF NOT EXISTS )?", table, R"(\s*\(([\s\S]*?)\n\s*\))")); - std::smatch body; - if (std::regex_search(source, body, create)) { - std::istringstream lines(body[2].str()); - std::string line; - while (std::getline(lines, line)) { - const std::regex column(R"(^\s*([a-z_]+)\s+([A-Za-z]+(\(\d+\))?))"); - std::smatch parsed; - if (std::regex_search(line, parsed, column)) columns[parsed[1]] = parsed[2]; - } - } - - const std::regex added(absl::StrCat( - "ALTER TABLE ", table, R"( ADD COLUMN IF NOT EXISTS ([a-z_]+) ([A-Za-z]+(\(\d+\))?))")); - for (std::sregex_iterator it(source.begin(), source.end(), added), end; it != end; ++it) { - columns[(*it)[1]] = (*it)[2]; - } - return columns; -} - -/// The DDL for one table, as the migrations create and then alter it. -std::map MigrationSchemaFor(const std::string& table) { - return Columns(PostgresMigrationSql(), table); -} - -std::map MigrationSchema() { - return MigrationSchemaFor("indexing_requests"); -} - -// The migration files are read blind off the manifest, so an unreachable -// file — in :migrations_sql but missing from the manifest, or shadowed by -// the resolution rule — is one this test silently never scrapes and -// production never runs. Walks the runfiles copy of :migrations_sql only: -// a file on disk but absent from that filegroup (or from BUILD entirely) -// is invisible to every bazel test, which migrations/README.md documents -// as this repo's standing trap. TEST(SchemaContract, EveryMigrationFileIsReachableFromTheManifest) { - std::set reachable = {"manifest.txt"}; - for (const std::string& step : ManifestSteps()) { + const absl::StatusOr> steps = MigrationSteps(); + ASSERT_TRUE(steps.ok()) << steps.status(); + + std::set reachable = {absl::StrCat(MigrationsRoot(), "/manifest.txt")}; + for (const std::string& step : *steps) { for (const std::string& engine : {"pg", "h2"}) { - if (std::filesystem::exists(absl::StrCat(kMigrationsDir, "/", engine, "/", step, ".sql"))) { - reachable.insert(absl::StrCat(engine, "/", step, ".sql")); - } - } - if (std::filesystem::exists(absl::StrCat(kMigrationsDir, "/", step, ".sql"))) { - reachable.insert(absl::StrCat(step, ".sql")); + const absl::StatusOr path = MigrationSqlPath(step, engine); + if (path.ok()) reachable.insert(*path); } } + + // Walks the runfiles copy of :migrations_sql only: a file on disk but + // absent from that filegroup is invisible to every bazel test, which + // migrations/README.md documents as this repo's standing trap. int seen = 0; - for (const auto& entry : std::filesystem::recursive_directory_iterator(kMigrationsDir)) { + for (const auto& entry : std::filesystem::recursive_directory_iterator(MigrationsRoot())) { if (!entry.is_regular_file()) continue; // lexically_relative, not relative: runfiles are symlinks, and relative() // canonicalizes through them to somewhere outside the tree. - const std::string relative = entry.path().lexically_relative(kMigrationsDir).generic_string(); - EXPECT_TRUE(reachable.count(relative) == 1) - << relative << " is not reachable from manifest.txt — it never runs anywhere"; + const std::string path = absl::StrCat( + MigrationsRoot(), "/", entry.path().lexically_relative(MigrationsRoot()).generic_string()); + EXPECT_TRUE(reachable.count(path) == 1) + << path << " is not reachable from manifest.txt — it never runs anywhere"; ++seen; } // A loose floor: only there to catch the walk finding nothing at all, so a @@ -147,166 +58,27 @@ TEST(SchemaContract, EveryMigrationFileIsReachableFromTheManifest) { EXPECT_GT(seen, 10) << "the directory walk found almost nothing — the data moved"; } -// Both engines resolve every step, checked here as well as in -// MigrationFilesTest because this test's view of the schema is built from -// the same resolution rule: a step it cannot resolve is a step it is not -// checking the fixtures against. +// Both engines resolve every step. The Postgres half is exercised by every +// suite that migrates; H2 is one_d4's own path, and a step it cannot resolve +// is one the Java service dies on at boot. TEST(SchemaContract, EveryManifestStepResolvesForBothEngines) { - const std::vector steps = ManifestSteps(); - ASSERT_FALSE(steps.empty()) << "read no steps at all — the manifest moved"; - for (const std::string& step : steps) { + const absl::StatusOr> steps = MigrationSteps(); + ASSERT_TRUE(steps.ok()) << steps.status(); + for (const std::string& step : *steps) { for (const std::string& engine : {"pg", "h2"}) { - ResolveStep(step, engine); // EXPECTs inside + const absl::StatusOr path = MigrationSqlPath(step, engine); + EXPECT_TRUE(path.ok()) << path.status(); } } } -TEST(SchemaContract, TheMigrationSchemaHasEveryColumnThisWorkerReadsOrWrites) { - const std::map java = MigrationSchema(); - ASSERT_FALSE(java.empty()) << "read no columns at all — the DDL moved"; - - for (const std::string& column : - {"id", "player", "platform", "start_month", "end_month", "status", "created_at", - "updated_at", "error_message", "games_indexed", "exclude_bullet", "skip_cache", "attempts", - "owner_id", "lease_expires_at", "dedupe_key"}) { - EXPECT_TRUE(java.count(column) == 1) << column << " is gone from the migration schema"; - } -} - -TEST(SchemaContract, TheTestFixtureDeclaresTheSameTypes) { - const std::map java = MigrationSchema(); - const std::map fixture = - Columns(Read("domains/games/apis/one_d4_worker/pg_queue_test.cc"), "indexing_requests"); - ASSERT_FALSE(fixture.empty()) << "read no columns from the fixture"; - - for (const auto& [name, type] : fixture) { - const auto declared = java.find(name); - ASSERT_TRUE(declared != java.end()) << name << " is not in the migration schema"; - EXPECT_EQ(type, declared->second) << name << " is declared differently in the fixture"; - } -} - -TEST(SchemaContract, IdIsAUuid) { - // Named on its own because it is the one a text fixture gets wrong - // silently: 'job-1' is a fine VARCHAR and not a UUID at all. - EXPECT_EQ(MigrationSchema()["id"], "UUID"); -} - -// The same argument, for the three tables the sink writes. Its fixture -// hand-copies their DDL too, so a column a migration changes leaves these -// tests green and production broken — and unlike indexing_requests, these -// are tables the C++ worker writes rows into rather than just claims from. - -TEST(SchemaContract, TheMigrationSchemaHasEveryColumnTheSinkWrites) { - for (const auto& [table, wanted] : std::vector>>{ - {"game_features", - {"id", "request_id", "game_url", "platform", "white_username", "black_username", - "white_elo", "black_elo", "white_title", "black_title", "time_class", "eco", - "opening_name", "opening_family", "result", "played_at", "num_moves", "indexed_at", - "pgn"}}, - {"motif_occurrences", - {"id", "game_url", "motif", "ply", "side", "move_number", "description", "moved_piece", - "attacker", "target", "is_discovered", "is_mate", "pin_type"}}, - {"indexed_periods", - {"id", "player", "platform", "year_month", "fetched_at", "is_complete", "games_count", - "exclude_bullet"}}}) { - const std::map java = MigrationSchemaFor(table); - ASSERT_FALSE(java.empty()) << "read no columns of " << table << " — the DDL moved"; - for (const std::string& column : wanted) { - EXPECT_TRUE(java.count(column) == 1) << column << " is gone from " << table; - } - } -} - -TEST(SchemaContract, TheSinkFixtureDeclaresTheSameTypes) { - const std::string fixture_source = Read("domains/games/apis/one_d4_worker/pg_game_sink_test.cc"); - int checked = 0; - for (const std::string& table : {"game_features", "motif_occurrences", "indexed_periods"}) { - const std::map java = MigrationSchemaFor(table); - const std::map fixture = Columns(fixture_source, table); - ASSERT_FALSE(fixture.empty()) << "read no columns of " << table << " from the fixture"; - - for (const auto& [name, type] : fixture) { - const auto declared = java.find(name); - ASSERT_TRUE(declared != java.end()) << name << " is not in the migration " << table; - EXPECT_EQ(type, declared->second) - << name << " is declared differently in the " << table << " fixture"; - ++checked; - } - } - EXPECT_GT(checked, 30) << "the fixture parse found almost nothing to compare"; -} - -TEST(SchemaContract, TheOccurrenceIdIsNotAUuidColumn) { - // motif_occurrences.id is a VARCHAR holding a UUID, unlike every other - // id here — which is why the sink generates it with gen_random_uuid() - // cast to text. A fixture that made it UUID would accept a cast the real - // column rejects. - EXPECT_EQ(MigrationSchemaFor("motif_occurrences")["id"], "VARCHAR(36)"); - EXPECT_EQ(MigrationSchemaFor("game_features")["id"], "UUID"); -} - -// The reanalysis fixture is a third hand-copy of the two tables the pass -// reads and writes, so it drifts on the same terms as the sink's. -TEST(SchemaContract, TheReanalysisFixtureDeclaresTheSameTypes) { - const std::string fixture_source = Read("domains/games/apis/one_d4_worker/pg_reanalysis_test.cc"); - int checked = 0; - for (const std::string& table : {"game_features", "motif_occurrences"}) { - const std::map java = MigrationSchemaFor(table); - const std::map fixture = Columns(fixture_source, table); - ASSERT_FALSE(fixture.empty()) << "read no columns of " << table << " from the fixture"; - - for (const auto& [name, type] : fixture) { - const auto declared = java.find(name); - ASSERT_TRUE(declared != java.end()) << name << " is not in the migration " << table; - EXPECT_EQ(type, declared->second) - << name << " is declared differently in the reanalysis " << table << " fixture"; - ++checked; - } - } - EXPECT_GT(checked, 12) << "the fixture parse found almost nothing to compare"; -} - -// reanalysis_requests is not a shared table — the indexers never touch it, -// which is the point of it existing (#1389 phase 5). But the migrations -// still own its DDL and this worker still hand-copies it into a fixture, so -// the same drift is available: a column renamed in pg/V017 leaves -// reanalysis_queue_test green against a table production does not have. - -TEST(SchemaContract, TheMigrationSchemaHasEveryColumnTheReanalysisQueueTouches) { - const std::map java = MigrationSchemaFor("reanalysis_requests"); - ASSERT_FALSE(java.empty()) << "read no columns at all — the DDL moved"; - - for (const std::string& column : - {"id", "status", "created_at", "updated_at", "owner_id", "lease_expires_at", "attempts", - "error_message", "cursor_game_url", "games_processed", "games_failed"}) { - EXPECT_TRUE(java.count(column) == 1) << column << " is gone from reanalysis_requests"; - } -} - -TEST(SchemaContract, TheReanalysisRequestFixtureDeclaresTheSameTypes) { - const std::map java = MigrationSchemaFor("reanalysis_requests"); - // Both copies. pg_reanalysis_test carries one too, and it is the one - // behind the fence — a lease_expires_at that drifted to TIMESTAMPTZ - // there would compare against NOW() differently than production does. - int checked = 0; - for (const std::string& path : {"domains/games/apis/one_d4_worker/reanalysis_queue_test.cc", - "domains/games/apis/one_d4_worker/pg_reanalysis_test.cc"}) { - const std::map fixture = Columns(Read(path), "reanalysis_requests"); - ASSERT_FALSE(fixture.empty()) << "read no columns from " << path; - - for (const auto& [name, type] : fixture) { - const auto declared = java.find(name); - ASSERT_TRUE(declared != java.end()) << name << " is not in the migration schema"; - EXPECT_EQ(type, declared->second) << name << " is declared differently in " << path; - ++checked; - } - } - EXPECT_GT(checked, 20) << "one of the two fixtures was not parsed"; -} - -TEST(SchemaContract, TheReanalysisIdIsAUuidToo) { - EXPECT_EQ(MigrationSchemaFor("reanalysis_requests")["id"], "UUID"); +// The schema name is concatenated into the DDL that drops and recreates it, +// so it is refused before anything is sent — which is what makes this +// answerable without a server. +TEST(SchemaContract, ASchemaNameThatIsNotAPlainIdentifierIsRefused) { + pg::Client unreachable("postgresql://127.0.0.1:1/nope?connect_timeout=2"); + EXPECT_EQ(ResetToMigratedSchema(unreachable, "one_d4_test; DROP SCHEMA public CASCADE").code(), + absl::StatusCode::kInvalidArgument); } /// The shipped windows, loaded the way the worker loads them at startup. @@ -344,43 +116,5 @@ TEST(SchemaContract, ThePollerRunsTheShippedLeaseVocabulary) { EXPECT_LE(options.renew_every * 4, options.lease); } -/// The columns the sweep keys on, against the real DDL. -/// -/// retention_test's fixture is deliberately minimal — it declares what the -/// sweep touches and not the other fifteen columns of game_features — so it -/// cannot be compared column-for-column the way pg_queue_test's is. What can -/// be checked is that the columns the sweep names still exist and still hold -/// what it binds: every timestamp it compares is written as a naive UTC -/// TIMESTAMP, so one drifted to TIMESTAMPTZ would compare against a bound -/// literal differently than production does, and delete a different set of -/// rows than the tests say it deletes. -TEST(SchemaContract, TheMigrationSchemaHasTheColumnsTheSweepKeysOn) { - const std::vector>> keyed = { - {"indexing_requests", - {"id", "status", "attempts", "owner_id", "lease_expires_at", "updated_at", "created_at", - "dedupe_key", "error_message"}}, - {"game_features", {"request_id", "indexed_at"}}, - {"indexed_periods", {"fetched_at"}}}; - - for (const auto& [table, wanted] : keyed) { - const std::map java = MigrationSchemaFor(table); - ASSERT_FALSE(java.empty()) << "read no columns of " << table << " — the DDL moved"; - for (const std::string& column : wanted) { - EXPECT_TRUE(java.count(column) == 1) - << column << " is gone from " << table << ", and the sweep keys on it"; - } - } - - for (const auto& [table, column] : - std::vector>{{"indexing_requests", "updated_at"}, - {"indexing_requests", "created_at"}, - {"indexing_requests", "lease_expires_at"}, - {"game_features", "indexed_at"}, - {"indexed_periods", "fetched_at"}}) { - EXPECT_EQ(MigrationSchemaFor(table)[column], "TIMESTAMP") - << table << "." << column << " is not a naive TIMESTAMP, but the sweep binds one"; - } -} - } // namespace } // namespace one_d4_worker diff --git a/domains/games/apis/one_d4_worker/worker_main.cc b/domains/games/apis/one_d4_worker/worker_main.cc index 44d1f3aeb..39276d0af 100644 --- a/domains/games/apis/one_d4_worker/worker_main.cc +++ b/domains/games/apis/one_d4_worker/worker_main.cc @@ -98,6 +98,10 @@ int main(int /*argc*/, char** argv) { } futility::otel::OtelConfig otel_config{ + // prom_proxy's indexing selectors are service_name=~"one_d4(_worker)?" + // (domains/platform/apis/prom_proxy/registry.go). Nothing enforces the + // match: renaming this leaves every test green and every indexing chart + // Java-only. .service_name = "one_d4_worker", .service_version = "1.0.0", // An index run takes minutes and a month holds hundreds of games. diff --git a/domains/platform/apis/prom_proxy/BUILD.bazel b/domains/platform/apis/prom_proxy/BUILD.bazel index c2b98d68a..8c465eb21 100644 --- a/domains/platform/apis/prom_proxy/BUILD.bazel +++ b/domains/platform/apis/prom_proxy/BUILD.bazel @@ -32,9 +32,6 @@ go_test( "registry_test.go", "service_handlers_test.go", ], - data = [ - "//domains/games/apis/one_d4_worker:worker_service_name", - ], embed = [":prom_proxy_lib"], deps = [ "@com_github_prometheus_client_golang//api", diff --git a/domains/platform/apis/prom_proxy/registry.go b/domains/platform/apis/prom_proxy/registry.go index 11a5aed17..39729e20e 100644 --- a/domains/platform/apis/prom_proxy/registry.go +++ b/domains/platform/apis/prom_proxy/registry.go @@ -390,10 +390,13 @@ var serviceRegistry = map[string]serviceEntry{ // Counts, outcomes and motif names only — the emitter never labels by // player or by game, so no series here is per-user. // - // The indexing series come from one_d4_worker. The - // service_name=~"one_d4(_worker)?" selectors also match stored series - // recorded under service_name="one_d4"; both names are one timeline, and - // narrowing the selector would cut every chart off where they meet. + // The indexing series come from one_d4_worker, the name worker_main.cc + // reports as. The service_name=~"one_d4(_worker)?" selectors also match + // stored series recorded under service_name="one_d4"; both names are one + // timeline, and narrowing the selector would cut every chart off where + // they meet. Nothing enforces the match with the worker: renaming the + // service there leaves every test in this repo green and every indexing + // chart Java-only. // The probes tile stays scoped to one_d4: the worker serves no HTTP. "one_d4": { CustomScalars: []customScalarDef{ diff --git a/domains/platform/apis/prom_proxy/registry_test.go b/domains/platform/apis/prom_proxy/registry_test.go index daa02ec7e..4ecfd8dbf 100644 --- a/domains/platform/apis/prom_proxy/registry_test.go +++ b/domains/platform/apis/prom_proxy/registry_test.go @@ -2,7 +2,6 @@ package prom_proxy import ( "fmt" - "os" "regexp" "strconv" "strings" @@ -509,39 +508,6 @@ var oneD4ExportedNames = map[string]bool{ "retention_requests_settled_total": true, } -// The selector above covers both indexers by name, and the C++ worker's -// name is set in its own main. Nothing else ties the two together: rename -// the service there and every test in this repo still passes while all -// seventeen indexing selectors quietly revert to Java-only — which is the -// failure the widened selector exists to prevent. -func TestOneD4SelectorCoversTheServiceTheCppWorkerReportsAs(t *testing.T) { - // rules_go runs the test in its own package directory inside the - // runfiles tree, so the data dep is reached from the root. - source, err := os.ReadFile("../../../../domains/games/apis/one_d4_worker/worker_main.cc") - require.NoError(t, err, "worker_main.cc is not where this test looks") - - match := regexp.MustCompile(`\.service_name = "([a-z0-9_]+)"`).FindSubmatch(source) - require.NotNil(t, match, "worker_main.cc names no service") - name := string(match[1]) - - entry := serviceRegistry["one_d4"] - selector := regexp.MustCompile(`service_name=~"([^"]+)"`) - checked := 0 - for _, def := range entry.CustomScalars { - for _, query := range def.AllQueries() { - for _, found := range selector.FindAllStringSubmatch(query, -1) { - pattern, err := regexp.Compile("^" + found[1] + "$") - require.NoError(t, err, "selector %q is not a regexp", found[1]) - assert.True(t, pattern.MatchString(name), - "selector %q does not match %q, the service the C++ worker reports as: %s", - found[1], name, query) - checked++ - } - } - } - assert.NotZero(t, checked, "no indexing selector names both indexers") -} - func TestOneD4QueriesNameRealInstrumentsAndScopeThem(t *testing.T) { entry := serviceRegistry["one_d4"] require.NotEmpty(t, entry.CustomScalars) diff --git a/domains/platform/libs/pg/pg.cc b/domains/platform/libs/pg/pg.cc index 68639495f..37441539f 100644 --- a/domains/platform/libs/pg/pg.cc +++ b/domains/platform/libs/pg/pg.cc @@ -100,6 +100,22 @@ absl::StatusOr Client::Exec(const std::string& sql, return ExecLocked(sql, params); } +absl::Status Client::ExecScript(const std::string& sql) { + const std::lock_guard lock(mu_); + if (absl::Status connected = EnsureConnectedLocked(); !connected.ok()) return connected; + pg_result* raw = PQexec(conn_, sql.c_str()); + // Owns the handle, so the returns below still clear it. + Result result(raw); + const ExecStatusType status = raw == nullptr ? PGRES_FATAL_ERROR : PQresultStatus(raw); + if (status == PGRES_TUPLES_OK || status == PGRES_COMMAND_OK) return absl::OkStatus(); + if (status == PGRES_EMPTY_QUERY) { + return absl::InvalidArgumentError("postgres script contained no statements"); + } + return absl::InternalError(absl::StrCat( + "postgres script failed: ", + TrimmedError(raw == nullptr ? PQerrorMessage(conn_) : PQresultErrorMessage(raw)))); +} + absl::StatusOr Transaction::Exec(const std::string& sql, const std::vector& params) { // No lock: InTransaction holds it for the whole callback. diff --git a/domains/platform/libs/pg/pg.h b/domains/platform/libs/pg/pg.h index 47d826bc5..d4f2137aa 100644 --- a/domains/platform/libs/pg/pg.h +++ b/domains/platform/libs/pg/pg.h @@ -93,6 +93,15 @@ class Client { /// deleted reports zero rows). absl::StatusOr Exec(const std::string& sql, const std::vector& params = {}); + /// Runs a script of one or more statements, as `psql -f` would: the + /// simple query protocol, so no parameters and no rows come back. + /// Postgres wraps a multi-statement script in one implicit transaction, + /// so a statement that fails takes the whole script down with it and + /// leaves nothing behind — which is why this does not retry the way + /// Exec does. A script with no statements in it is an error, not a + /// no-op: it is what an empty or all-comments file looks like from here. + absl::Status ExecScript(const std::string& sql); + /// Runs `body` between BEGIN and COMMIT, holding the connection for /// the whole callback so no other caller can interleave a statement. /// A non-ok return from `body`, a failed transaction statement, or a diff --git a/domains/platform/libs/pg/pg_test.cc b/domains/platform/libs/pg/pg_test.cc index a8e54ae0c..8af89b2fa 100644 --- a/domains/platform/libs/pg/pg_test.cc +++ b/domains/platform/libs/pg/pg_test.cc @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "absl/status/status.h" @@ -187,4 +188,91 @@ TEST_F(PgTransactionTest, OtherCallersCannotInterleaveOnTheSameConnection) { EXPECT_EQ(Count(), 2) << "the outsider's write lands once the transaction releases"; } +// The .sql files one_d4's migrations ship as, run the way psql runs them. +class PgScriptTest : public ::testing::Test { + protected: + void SetUp() override { + const char* url = std::getenv("PG_TEST_DB_URL"); + if (url == nullptr || *url == '\0') GTEST_SKIP() << "PG_TEST_DB_URL unset"; + client_ = std::make_unique(url); + ASSERT_TRUE(client_->Exec("DROP TABLE IF EXISTS pg_script_test").ok()); + } + + std::unique_ptr client_; +}; + +// Why this is a second method rather than a use of Exec: Exec binds +// parameters, which puts it on the extended protocol, and that protocol +// carries one statement per message however few parameters are passed. +TEST_F(PgScriptTest, ExecRefusesTheScriptExecScriptRuns) { + const std::string script = + "CREATE TABLE pg_script_test (n integer); INSERT INTO pg_script_test VALUES (1);"; + EXPECT_FALSE(client_->Exec(script).ok()); + EXPECT_TRUE(client_->ExecScript(script).ok()); +} + +TEST_F(PgScriptTest, RunsEveryStatementInTheScript) { + ASSERT_TRUE(client_ + ->ExecScript(R"( + CREATE TABLE pg_script_test (n integer); + INSERT INTO pg_script_test VALUES (1); + INSERT INTO pg_script_test VALUES (2); + )") + .ok()); + + const auto rows = client_->Exec("SELECT n FROM pg_script_test ORDER BY n"); + ASSERT_TRUE(rows.ok()) << rows.status(); + EXPECT_EQ(rows->rows(), 2); +} + +// Statement-at-a-time is what Exec offers, and a script is not that: the +// implicit transaction Postgres wraps around a multi-statement script means +// the earlier statements do not survive a later failure. +TEST_F(PgScriptTest, LeavesNothingBehindWhenAStatementFails) { + const absl::Status status = client_->ExecScript(R"( + CREATE TABLE pg_script_test (n integer); + INSERT INTO pg_script_test VALUES (1); + INSERT INTO pg_script_test VALUES ('not a number'); + )"); + EXPECT_FALSE(status.ok()); + + const auto exists = client_->Exec("SELECT to_regclass('pg_script_test')"); + ASSERT_TRUE(exists.ok()) << exists.status(); + EXPECT_EQ(exists->Get(0, 0), std::nullopt) << "the failed script left its table behind"; +} + +// Dollar-quoted bodies carry semicolons, so a script split on ';' by hand +// would send half a DO block. libpq is what parses this one. +TEST_F(PgScriptTest, RunsADollarQuotedBlockWholeAndItsErrorsAreHandled) { + ASSERT_TRUE(client_ + ->ExecScript(R"( + CREATE TABLE pg_script_test (n integer); + DO $$ BEGIN + ALTER TABLE pg_script_test ADD CONSTRAINT pg_script_test_n_unique UNIQUE (n); + EXCEPTION WHEN duplicate_table OR duplicate_object THEN NULL; + END $$; + DO $$ BEGIN + ALTER TABLE pg_script_test ADD CONSTRAINT pg_script_test_n_unique UNIQUE (n); + EXCEPTION WHEN duplicate_table OR duplicate_object THEN NULL; + END $$; + )") + .ok()); + + const auto constraints = + client_->Exec("SELECT conname FROM pg_constraint WHERE conname = 'pg_script_test_n_unique'"); + ASSERT_TRUE(constraints.ok()) << constraints.status(); + EXPECT_EQ(constraints->rows(), 1); +} + +TEST_F(PgScriptTest, RefusesAScriptWithNoStatementsInIt) { + // What an empty or all-comments file reaches here as. Reporting success + // would make a migration that never ran indistinguishable from one that did. + EXPECT_EQ(client_->ExecScript("-- nothing to do\n").code(), absl::StatusCode::kInvalidArgument); +} + +TEST(PgScriptOfflineTest, ExecScriptOnUnreachableServerReturnsUnavailable) { + pg::Client client("postgresql://127.0.0.1:1/nope?connect_timeout=2"); + EXPECT_EQ(client.ExecScript("SELECT 1; SELECT 2;").code(), absl::StatusCode::kUnavailable); +} + } // namespace