Add the initial schema script (task 44) #17

Merged

rob merged 2 commits from feat/initial-schema-script into main 2026-08-03 11:38:19 +00:00

Conversation 2 Commits 2 Files changed 12 +832 -62

rob commented 2026-08-03 11:15:44 +00:00

Owner

Copy link

New PlaceMark.Database project holding the DDL and the DbUp upgrade call. 0001-initial-schema.sql creates the ICU collation, the five tables, eight foreign keys each stating ON DELETE, and exactly three foreign-key indexes.

Separate project because the journal key is the full embedded resource name — the project name, root namespace and Scripts\ folder are all part of it, so they are chosen deliberately rather than inherited. Scripts included by wildcard so a new .sql cannot be silently omitted. Keeps DbUp out of the API's dependency graph.

Verification queries the database, not the script

Every structural test reads pg_catalog on the container the fixture just upgraded:

• Eight foreign keys — one row each with referencing column, referenced column and confdeltype decoded, compared row-for-row against the data model. A constraint named for one column but declared on another fails.
• Three and three — one row per indexed column (12 rows, 10 indexes) compared in full, so an unasked-for index appears as an extra row; plus a test that no index covers the three deliberately unindexed audit columns.

Mutation-checked: RESTRICT→CASCADE plus one added index fails exactly four tests.

One real finding. PostgreSQL raises 23001 restrict_violation for RESTRICT, not 23503. The test asserts the SQLSTATE, because NO ACTION — the omitted-clause case — also refuses the delete, just with 23503. The code is the only thing distinguishing the constraint we wanted from the one an omission would have given us.

Flagged

WithTransactionPerScript() is not tested. Proving it needs a deliberately failing script and therefore a second scripts assembly. With one script and no failure mode to trigger, that machinery seemed worse than the gap — held up by review and the comment beside the call. Say if you want it.

PlaceMark.Database has no test project, breaking the one-per-project convention. Its tests live in PlaceMark.Infrastructure.Tests, which already owns the fixture applying the same scripts; a second project means a second container asserting the same schema. Recorded in the README beside the existing PlaceMark.Contracts exception.

Also deletes the provisional round_trip_probe table and inline collation from the fixture, as task 43 asked; those tests now run against users. Corrects the README and docs/data-model.md where this made them untrue — note nothing yet applies the scripts to the Compose database; that is task 45.

New `PlaceMark.Database` project holding the DDL and the DbUp upgrade call. `0001-initial-schema.sql` creates the ICU collation, the five tables, eight foreign keys each stating `ON DELETE`, and exactly three foreign-key indexes. Separate project because **the journal key is the full embedded resource name** — the project name, root namespace and `Scripts\` folder are all part of it, so they are chosen deliberately rather than inherited. Scripts included by wildcard so a new `.sql` cannot be silently omitted. Keeps DbUp out of the API's dependency graph. ## Verification queries the database, not the script Every structural test reads `pg_catalog` on the container the fixture just upgraded: - **Eight foreign keys** — one row each with referencing column, referenced column and `confdeltype` decoded, compared row-for-row against the data model. A constraint *named* for one column but *declared* on another fails. - **Three and three** — one row per indexed column (12 rows, 10 indexes) compared in full, so an unasked-for index appears as an extra row; plus a test that no index covers the three deliberately unindexed audit columns. Mutation-checked: `RESTRICT`→`CASCADE` plus one added index fails exactly four tests. **One real finding.** PostgreSQL raises **`23001 restrict_violation`** for `RESTRICT`, not `23503`. The test asserts the SQLSTATE, because `NO ACTION` — the omitted-clause case — *also* refuses the delete, just with `23503`. The code is the only thing distinguishing the constraint we wanted from the one an omission would have given us. ## Flagged **`WithTransactionPerScript()` is not tested.** Proving it needs a deliberately failing script and therefore a second scripts assembly. With one script and no failure mode to trigger, that machinery seemed worse than the gap — held up by review and the comment beside the call. Say if you want it. **`PlaceMark.Database` has no test project**, breaking the one-per-project convention. Its tests live in `PlaceMark.Infrastructure.Tests`, which already owns the fixture applying the same scripts; a second project means a second container asserting the same schema. Recorded in the README beside the existing `PlaceMark.Contracts` exception. Also deletes the provisional `round_trip_probe` table and inline collation from the fixture, as task 43 asked; those tests now run against `users`. Corrects the README and `docs/data-model.md` where this made them untrue — note **nothing yet applies the scripts to the Compose database**; that is task 45.

rob added 1 commit 2026-08-03 11:15:44 +00:00

Add the initial schema script ... 3df2b3a5f9

New PlaceMark.Database project holding only the DDL and the upgrade
call. It exists so the journal key is stable: DbUp keys on the full
embedded resource name, so the project name, root namespace and Scripts
folder are all part of it. Scripts are included by wildcard so a new .sql
cannot be silently omitted, and DbUp stays out of the API's graph.

0001-initial-schema.sql creates the ICU collation first, then the five
tables with every constraint named, the eight foreign keys each stating
ON DELETE, and exactly three foreign-key indexes.

The structural tests query pg_catalog on the upgraded container rather
than reading the script: one row per foreign key with its columns and
decoded delete action compared against the data model row for row, and
one row per indexed column so an unasked-for index shows up as an extra.
Mutating RESTRICT to CASCADE and adding one omitted index fails exactly
four tests.

PostgreSQL raises 23001 restrict_violation for RESTRICT, not 23503. The
test asserts the SQLSTATE, because NO ACTION — the omitted-clause case —
would also refuse the delete, but with 23503. The code is what
distinguishes them.

Deletes the provisional round_trip_probe table and inline collation from
the fixture, which now applies the real scripts. The round-trip tests
move to users, the table they were standing in for.

rob force-pushed feat/initial-schema-script from 3df2b3a5f9 to 96dbdf5e66 2026-08-03 11:16:44 +00:00 Compare

rob reviewed 2026-08-03 11:24:19 +00:00

rob left a comment

Copy link

Verdict: mergeable

Independent review at 96dbdf5. I did not take the script's word for anything: I applied 0001-initial-schema.sql to a fresh postgres:18.4-trixie and diffed the resulting catalogue against docs/data-model.md column by column, then mutated the script to find out what the tests actually hold down.

The central check: the schema matches the specification

It matches exactly. All 32 columns across the five tables agree with the data model on type, nullability and default — including the details that are easy to get subtly wrong: external_identities carrying linked_at alone rather than the created_at/updated_at pair; role having no default while status defaults to 'pending'; the three audit columns nullable because they are SET NULL; description varchar(2000) nullable and password_hash nullable.

Eight foreign keys, eight correct delete actions. contype = 'f' returns exactly eight rows and every confdeltype matches the table in the data model — the two CASCADEs on groups/external_identities, the three SET NULL audit references, the two group cascades, and fk_group_memberships_users_user_id as RESTRICT.

Your 23001 finding is correct, and I confirmed it is load-bearing. Against the same server:

Declaration	SQLSTATE
ON DELETE RESTRICT	23001 restrict_violation
ON DELETE NO ACTION	23503 foreign_key_violation
clause omitted entirely	23503 foreign_key_violation

Asserting PostgresErrorCodes.RestrictViolation rather than merely "it threw" is therefore the only thing in the suite separating the constraint you wanted from the one an omitted clause would have given. I mutated ON DELETE RESTRICT to the omitted-clause form — the actual hazard the ticket and ADR-0005 warn about — and it fails two tests, the delete-action row and the SQLSTATE. That is the single most valuable assertion in the PR and it earns its keep.

Exactly the intended indexes. Ten indexes over twelve column-rows, matching your list precisely; groups.created_by_user_id, places.created_by_user_id and group_memberships.invited_by_user_id carry none.

The collation is right and does its job. collprovider = i, collisdeterministic = f, colllocale = und-u-ks-level2. WHERE email = 'ada@example.com' matches a row stored as Ada@Example.COM, and a second insert differing only in case is refused by ux_users_email.

Applying twice is a no-op — confirmed, and the assertion is stronger than it looks: replaying the raw SQL against the same database fails at line 23 with collation "case_insensitive" already exists, so an empty second run really does prove the journal suppressed it.

Mutation claim verified. RESTRICT→CASCADE plus CREATE INDEX ix_places_created_by_user_id fails exactly four tests, the four named in your description.

dotnet build, -c Release, dotnet test (23 passed, 0 failed) and dotnet format --verify-no-changes are all clean, with zero warnings.

Non-blocking: the coverage is narrower than the description implies

"Every structural test reads pg_catalog" is true, but only two dimensions are read exhaustively — foreign keys and indexes. I mutated fourteen schema properties; these survived with all 18 tests green:

Mutation	Result
latitude/longitude double precision → numeric(9,6)	all pass
description varchar(2000) → text, display_name varchar(100) → varchar(50)	all pass
drop DEFAULT 'pending' from status	all pass
drop ck_places_latitude_range	all pass
drop ck_users_display_name_not_blank	all pass
drop ck_group_memberships_inviter_is_not_self	all pass
widen ck_group_memberships_role to admit 'admin'	all pass

So no check constraint is asserted anywhere, and no column type is asserted anywhere. Nullability and defaults are caught only incidentally, by insert-based tests that happen to omit the column — password_hash NULL → NOT NULL fails four tests, but none of them is about nullability, so that safety net moves the day those inserts change.

I am not treating this as blocking, because the schema you have delivered is correct — I verified that by hand rather than inferring it from the tests — and because the two hazards the ticket and ADR-0021 single out are precisely the two you covered. But two of these are worth more than the rest:

• ck_group_memberships_role is the database's only guarantee that the role vocabulary is what the authorisation policies will expect. Widening it passes silently today, and the per-group roles epic is the next thing to lean on it.
• double precision is an ADR-0016 decision the data model calls out as expensive to reverse once rows exist. Nothing would notice it becoming numeric.

A single extra test reading pg_constraint for contype = 'c' and comparing conname plus pg_get_constraintdef row-for-row would close the first, in the same style as the foreign key test. A second reading format_type(atttypid, atttypmod) per column closes the rest. Both are cheap and neither needs another container.

The four judgement calls

1. The separate PlaceMark.Database project is justified, and not over-structured. The journal-key argument is real and I confirmed it: the embedded name is exactly PlaceMark.Database.Scripts.0001-initial-schema.sql, so the project name and folder are genuinely part of the database's state, and choosing them deliberately beats inheriting them from PlaceMark.Infrastructure. The second benefit is concrete too — there is no DbUp assembly in the API's build output, so a migration runner is not in the deployed API's dependency graph. ADR-0021 explicitly leaves "where the runner project lives" to #45, so this is within your gift. The wildcard EmbeddedResource and the comment explaining why it is a wildcard are the right call given the silent-skip failure mode.

2. WithTransactionPerScript() untested — right trade, but the stated reason is not quite right. I agree it should not block: the behaviour was already measured and recorded in ADR-0021 against this exact package version, and re-proving it here would test DbUp rather than SchemaUpgrader.

But "proving it needs a second scripts assembly" is not so. Upgrade could take an optional Assembly parameter defaulting to typeof(SchemaUpgrader).Assembly; a test then passes its own assembly with a deliberately-failing two-statement .sql embedded in PlaceMark.Infrastructure.Tests, and asserts that the first statement's object is absent afterwards. No second project, one optional parameter, and it would pin the configuration rather than the vendor. Worth knowing the option exists if this ever matters; I would not hold the PR for it.

3. No PlaceMark.Database.Tests is acceptable and the exception is recorded properly. The README change rewrites the single-exception sentence into two and gives the reasoning — that the scripts are only observable through a real PostgreSQL, and PlaceMark.Infrastructure.Tests already owns the fixture that applies them. That is a better justification than the convention it bends, and a second container asserting the same schema would be pure cost.

4. Things that will bite later.

• The collation's version dependency is the one I would write down. pg_collation.collversion is 153.128 here and it is recorded against users.email, which carries the unique index ux_users_email. When the base image's ICU is upgraded, PostgreSQL will warn that the collation has a version mismatch and the index may need REINDEX — a btree over a collated column can silently mis-order after an ICU change. Nothing in the schema or the docs mentions this, and the first person to bump postgres:18.4-trixie will not be expecting it. A note in docs/data-model.md beside the collation, or a line in the script header, would be enough; it does not need solving now.
• Naming, ordering and idempotency are all sound. The four-digit prefix gives correct lexical ordering, the collation precedes users as the only ordering constraint, and the resource name survived MSBuild's logical-name mangling intact despite beginning with a digit — which the journal test pins.
• The PR correctly notes that nothing yet applies the scripts to the Compose database, and the README says so plainly rather than leaving \dt a mystery. Good that this was corrected rather than left aspirational.

Nothing here blocks the merge. The delivered schema is a faithful implementation of the specification, and I checked that against a running database rather than against the diff.

Verdict: mergeable Independent review at `96dbdf5`. I did not take the script's word for anything: I applied `0001-initial-schema.sql` to a fresh `postgres:18.4-trixie` and diffed the resulting catalogue against `docs/data-model.md` column by column, then mutated the script to find out what the tests actually hold down. ## The central check: the schema matches the specification **It matches exactly.** All 32 columns across the five tables agree with the data model on type, nullability and default — including the details that are easy to get subtly wrong: `external_identities` carrying `linked_at` alone rather than the `created_at`/`updated_at` pair; `role` having no default while `status` defaults to `'pending'`; the three audit columns nullable *because* they are `SET NULL`; `description varchar(2000)` nullable and `password_hash` nullable. **Eight foreign keys, eight correct delete actions.** `contype = 'f'` returns exactly eight rows and every `confdeltype` matches the table in the data model — the two `CASCADE`s on `groups`/`external_identities`, the three `SET NULL` audit references, the two group cascades, and `fk_group_memberships_users_user_id` as `RESTRICT`. **Your `23001` finding is correct, and I confirmed it is load-bearing.** Against the same server: | Declaration | SQLSTATE | | --- | --- | | `ON DELETE RESTRICT` | `23001` `restrict_violation` | | `ON DELETE NO ACTION` | `23503` `foreign_key_violation` | | clause omitted entirely | `23503` `foreign_key_violation` | Asserting `PostgresErrorCodes.RestrictViolation` rather than merely "it threw" is therefore the only thing in the suite separating the constraint you wanted from the one an omitted clause would have given. I mutated `ON DELETE RESTRICT` to the omitted-clause form — the actual hazard the ticket and ADR-0005 warn about — and it fails two tests, the delete-action row and the SQLSTATE. That is the single most valuable assertion in the PR and it earns its keep. **Exactly the intended indexes.** Ten indexes over twelve column-rows, matching your list precisely; `groups.created_by_user_id`, `places.created_by_user_id` and `group_memberships.invited_by_user_id` carry none. **The collation is right and does its job.** `collprovider = i`, `collisdeterministic = f`, `colllocale = und-u-ks-level2`. `WHERE email = 'ada@example.com'` matches a row stored as `Ada@Example.COM`, and a second insert differing only in case is refused by `ux_users_email`. **Applying twice is a no-op** — confirmed, and the assertion is stronger than it looks: replaying the raw SQL against the same database fails at line 23 with `collation "case_insensitive" already exists`, so an empty second run really does prove the journal suppressed it. **Mutation claim verified.** `RESTRICT`→`CASCADE` plus `CREATE INDEX ix_places_created_by_user_id` fails exactly four tests, the four named in your description. `dotnet build`, `-c Release`, `dotnet test` (23 passed, 0 failed) and `dotnet format --verify-no-changes` are all clean, with zero warnings. ## Non-blocking: the coverage is narrower than the description implies "Every structural test reads `pg_catalog`" is true, but only two dimensions are read exhaustively — foreign keys and indexes. I mutated fourteen schema properties; these survived with all 18 tests green: | Mutation | Result | | --- | --- | | `latitude`/`longitude` `double precision` → `numeric(9,6)` | all pass | | `description varchar(2000)` → `text`, `display_name varchar(100)` → `varchar(50)` | all pass | | drop `DEFAULT 'pending'` from `status` | all pass | | drop `ck_places_latitude_range` | all pass | | drop `ck_users_display_name_not_blank` | all pass | | drop `ck_group_memberships_inviter_is_not_self` | all pass | | widen `ck_group_memberships_role` to admit `'admin'` | all pass | So **no check constraint is asserted anywhere**, and **no column type is asserted anywhere**. Nullability and defaults are caught only incidentally, by insert-based tests that happen to omit the column — `password_hash NULL → NOT NULL` fails four tests, but none of them is about nullability, so that safety net moves the day those inserts change. I am not treating this as blocking, because the schema you have delivered is correct — I verified that by hand rather than inferring it from the tests — and because the two hazards the ticket and ADR-0021 single out are precisely the two you covered. But two of these are worth more than the rest: - **`ck_group_memberships_role` is the database's only guarantee that the role vocabulary is what the authorisation policies will expect.** Widening it passes silently today, and the per-group roles epic is the next thing to lean on it. - **`double precision` is an ADR-0016 decision the data model calls out as expensive to reverse once rows exist.** Nothing would notice it becoming `numeric`. A single extra test reading `pg_constraint` for `contype = 'c'` and comparing `conname` plus `pg_get_constraintdef` row-for-row would close the first, in the same style as the foreign key test. A second reading `format_type(atttypid, atttypmod)` per column closes the rest. Both are cheap and neither needs another container. ## The four judgement calls **1. The separate `PlaceMark.Database` project is justified, and not over-structured.** The journal-key argument is real and I confirmed it: the embedded name is exactly `PlaceMark.Database.Scripts.0001-initial-schema.sql`, so the project name and folder are genuinely part of the database's state, and choosing them deliberately beats inheriting them from `PlaceMark.Infrastructure`. The second benefit is concrete too — there is no DbUp assembly in the API's build output, so a migration runner is not in the deployed API's dependency graph. ADR-0021 explicitly leaves "where the runner project lives" to #45, so this is within your gift. The wildcard `EmbeddedResource` and the comment explaining why it is a wildcard are the right call given the silent-skip failure mode. **2. `WithTransactionPerScript()` untested — right trade, but the stated reason is not quite right.** I agree it should not block: the behaviour was already measured and recorded in ADR-0021 against this exact package version, and re-proving it here would test DbUp rather than `SchemaUpgrader`. But "proving it needs a second scripts assembly" is not so. `Upgrade` could take an optional `Assembly` parameter defaulting to `typeof(SchemaUpgrader).Assembly`; a test then passes its own assembly with a deliberately-failing two-statement `.sql` embedded in `PlaceMark.Infrastructure.Tests`, and asserts that the first statement's object is absent afterwards. No second project, one optional parameter, and it would pin the configuration rather than the vendor. Worth knowing the option exists if this ever matters; I would not hold the PR for it. **3. No `PlaceMark.Database.Tests` is acceptable and the exception is recorded properly.** The README change rewrites the single-exception sentence into two and gives the reasoning — that the scripts are only observable through a real PostgreSQL, and `PlaceMark.Infrastructure.Tests` already owns the fixture that applies them. That is a better justification than the convention it bends, and a second container asserting the same schema would be pure cost. **4. Things that will bite later.** - **The collation's version dependency is the one I would write down.** `pg_collation.collversion` is `153.128` here and it is recorded against `users.email`, which carries the unique index `ux_users_email`. When the base image's ICU is upgraded, PostgreSQL will warn that the collation has a version mismatch and the index may need `REINDEX` — a `btree` over a collated column can silently mis-order after an ICU change. Nothing in the schema or the docs mentions this, and the first person to bump `postgres:18.4-trixie` will not be expecting it. A note in `docs/data-model.md` beside the collation, or a line in the script header, would be enough; it does not need solving now. - Naming, ordering and idempotency are all sound. The four-digit prefix gives correct lexical ordering, the collation precedes `users` as the only ordering constraint, and the resource name survived MSBuild's logical-name mangling intact despite beginning with a digit — which the journal test pins. - The PR correctly notes that nothing yet applies the scripts to the Compose database, and the README says so plainly rather than leaving `\dt` a mystery. Good that this was corrected rather than left aspirational. Nothing here blocks the merge. The delivered schema is a faithful implementation of the specification, and I checked that against a running database rather than against the diff.

rob added 1 commit 2026-08-03 11:33:08 +00:00

Assert column types and check constraints too ... 5d312998bf

Review proved the gap by mutation: widening the role constraint to admit
'admin', dropping the latitude range check and changing coordinates to
numeric(9,6) all left every test green. The role vocabulary is what the
roles epic leans on and the coordinate type is ADR-0016's headline
decision, so both were drifting silently.

Three tests added, read from the catalogue like the foreign keys were:
eight check constraints compared by PostgreSQL's own rendering of the
parsed expression, so a widened value set shows however it was written;
32 columns compared as type, length, collation, nullability and default;
and the role vocabulary observed as behaviour, with 'admin' refused.

Each mutation now fails at least one test.

Tests WithTransactionPerScript, and drops the claim that it needed a
second scripts assembly — an optional assembly parameter on Upgrade lets
the test supply its own embedded failing script. It asserts 22P02, which
is only reachable if the CREATE TABLE already ran, so the rollback claim
is not vacuous.

Documents the collation's version dependency at the script: the recorded
collversion, the index at risk, and the two-statement remedy in order.

rob referenced this pull request 2026-08-03 11:33:33 +00:00

Global exception handling and Problem Details (task 58) #18

rob reviewed 2026-08-03 11:37:53 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 5d31299, superseding my comment at 96dbdf5. All three findings are properly closed, and I re-ran the mutation battery rather than reading the new tests and believing them.

My count was wrong and yours is right: eight check constraints, not seven. I miscounted group_memberships when I listed them. The test asserts eight and eight is what the data model specifies.

The coverage gap is closed — 12 of 12 mutations now caught

Everything that escaped last time now fails, including mutations I did not raise:

Mutation	Was	Now fails	Caught by
ck_group_memberships_role admits 'admin'	passed	2	constraint list + the behavioural test
coordinates → numeric(9,6)	passed	2	column list + constraint list
drop ck_places_latitude_range	passed	1	constraint list
drop ck_users_display_name_not_blank	passed	1	constraint list
drop ck_group_memberships_inviter_is_not_self	passed	1	constraint list
widen ck_group_memberships_status to admit 'declined'	—	1	constraint list
drop DEFAULT 'pending' from status	passed	2	column list + behaviour
display_name varchar(100) → varchar(50)	passed	1	column list
description varchar(2000) → text	passed	1	column list
drop COLLATE case_insensitive from email	—	3	column list + both collation tests
password_hash NULL → NOT NULL	4 collateral	6	now including the column list deliberately

Two things I liked more than I expected. Rendering the columns through format_type and comparing whole lines means the type, length, collation, nullability and default all fail as one readable diff rather than five assertions — and text COLLATE case_insensitive being in that line is why dropping the collation now fails three tests instead of two. And using pg_get_constraintdef rather than the typed text is what makes the 'admin' widening fail however it is spelled: I wrote it as a fourth IN element and PostgreSQL rendered it back as = ANY (ARRAY[...]), which is exactly the point. The coordinate mutation failing the constraint test as well is a free bonus — the range checks re-render against the new type.

InsertMembership_RoleOutsideTheThreeThePolicyKnows_IsRefused asserting 23514 plus the constraint name is the right complement to the catalogue test: one pins the declaration, the other pins that it bites.

WithTransactionPerScript() — now tested, and the test is not vacuous

I checked this two ways.

Removing .WithTransactionPerScript() from SchemaUpgrader fails exactly one test, the new one. So it pins the configuration rather than DbUp's behaviour, which was the whole question.

And I confirmed the 22P02 reasoning is sound at the database level, because the comment's claim is the load-bearing part:

without a transaction → probe table survives: true
with a transaction    → probe table survives: false
failing statement SQLSTATE → 22P02 (execution time, after CREATE TABLE ran)

So the assertion really is only reachable with the table already created, and ShouldBeFalse() would genuinely fail under the default strategy. The rollback claim is not vacuous.

The isolation is done properly too, which is the part that could have gone wrong quietly. I checked the built assemblies: PlaceMark.Database.dll embeds exactly one resource, PlaceMark.Database.Scripts.0001-initial-schema.sql, and the failing script exists only as PlaceMark.Infrastructure.Tests.Database.FailingScripts.0001-creates-a-table-then-fails.sql. Putting it outside PlaceMark.Database's wildcard rather than trying to exclude it there is the right instinct — a script that must never reach the schema should not live where the wildcard could ever reach it.

The optional Assembly? defaulting to the declaring assembly is a minimal seam, documented as to why it exists, and production has one caller passing nothing.

Collation version note

The remedy is correct and I ran it verbatim against the schema:

REINDEX INDEX ux_users_email;          -- REINDEX
ALTER COLLATION case_insensitive REFRESH VERSION;   -- NOTICE: version has not changed

Both statements are valid and the order is the important half — reindexing first, then refreshing, because refreshing first would clear the warning while the btree was still ordered under the old ICU. The note says "in this order" and that emphasis earns its place. Naming ux_users_email as the index at risk and recording 153.128 makes it actionable rather than a caution.

Verified at this head

• dotnet build and dotnet build -c Release — succeeded, 0 warnings, 0 errors.
• dotnet test -c Release — 27 passed, 0 failed (4 Api, 1 WebUI, 22 Infrastructure).
• dotnet format --verify-no-changes — exit 0.
• The DDL is unchanged from 96dbdf5 apart from the collation comment block, so my exhaustive catalogue diff from the first review still stands: 32 columns, 8 foreign keys with the specified delete actions, 10 indexes, both composite primary keys, case-insensitive matching and 23001 on the RESTRICT — all still matching docs/data-model.md.

One factual correction, immaterial

The note about a mid-flight rebase onto main for task 57 does not match the branch: 5d31299's parent is still 96dbdf5, whose parent is still df1308d ("Establish the Minimal API endpoint structure (#16)") — the same base my first review ran against. PlaceMark.Api.Tests reported 4 tests at 96dbdf5 too, not 3, so nothing changed there. No rebase appears to have happened, and none was needed. Flagging only so the record is accurate; it has no bearing on the verdict.

Nothing outstanding. The schema matches the specification, and the tests now hold down every dimension of it that the specification states.

Verdict: mergeable Re-reviewed at `5d31299`, superseding my comment at `96dbdf5`. All three findings are properly closed, and I re-ran the mutation battery rather than reading the new tests and believing them. **My count was wrong and yours is right: eight check constraints, not seven.** I miscounted `group_memberships` when I listed them. The test asserts eight and eight is what the data model specifies. ## The coverage gap is closed — 12 of 12 mutations now caught Everything that escaped last time now fails, including mutations I did not raise: | Mutation | Was | Now fails | Caught by | | --- | --- | --- | --- | | `ck_group_memberships_role` admits `'admin'` | passed | **2** | constraint list + the behavioural test | | coordinates → `numeric(9,6)` | passed | **2** | column list + constraint list | | drop `ck_places_latitude_range` | passed | **1** | constraint list | | drop `ck_users_display_name_not_blank` | passed | **1** | constraint list | | drop `ck_group_memberships_inviter_is_not_self` | passed | **1** | constraint list | | widen `ck_group_memberships_status` to admit `'declined'` | — | **1** | constraint list | | drop `DEFAULT 'pending'` from `status` | passed | **2** | column list + behaviour | | `display_name varchar(100)` → `varchar(50)` | passed | **1** | column list | | `description varchar(2000)` → `text` | passed | **1** | column list | | drop `COLLATE case_insensitive` from `email` | — | **3** | column list + both collation tests | | `password_hash NULL` → `NOT NULL` | 4 collateral | **6** | now including the column list *deliberately* | Two things I liked more than I expected. Rendering the columns through `format_type` and comparing whole lines means the type, length, collation, nullability and default all fail as one readable diff rather than five assertions — and `text COLLATE case_insensitive` being in that line is why dropping the collation now fails three tests instead of two. And using `pg_get_constraintdef` rather than the typed text is what makes the `'admin'` widening fail *however it is spelled*: I wrote it as a fourth `IN` element and PostgreSQL rendered it back as `= ANY (ARRAY[...])`, which is exactly the point. The coordinate mutation failing the *constraint* test as well is a free bonus — the range checks re-render against the new type. `InsertMembership_RoleOutsideTheThreeThePolicyKnows_IsRefused` asserting `23514` plus the constraint name is the right complement to the catalogue test: one pins the declaration, the other pins that it bites. ## `WithTransactionPerScript()` — now tested, and the test is not vacuous I checked this two ways. **Removing `.WithTransactionPerScript()` from `SchemaUpgrader` fails exactly one test**, the new one. So it pins the configuration rather than DbUp's behaviour, which was the whole question. **And I confirmed the `22P02` reasoning is sound at the database level**, because the comment's claim is the load-bearing part: ``` without a transaction → probe table survives: true with a transaction → probe table survives: false failing statement SQLSTATE → 22P02 (execution time, after CREATE TABLE ran) ``` So the assertion really is only reachable with the table already created, and `ShouldBeFalse()` would genuinely fail under the default strategy. The rollback claim is not vacuous. The isolation is done properly too, which is the part that could have gone wrong quietly. I checked the built assemblies: `PlaceMark.Database.dll` embeds exactly one resource, `PlaceMark.Database.Scripts.0001-initial-schema.sql`, and the failing script exists only as `PlaceMark.Infrastructure.Tests.Database.FailingScripts.0001-creates-a-table-then-fails.sql`. Putting it outside `PlaceMark.Database`'s wildcard rather than trying to exclude it there is the right instinct — a script that must never reach the schema should not live where the wildcard could ever reach it. The optional `Assembly?` defaulting to the declaring assembly is a minimal seam, documented as to why it exists, and production has one caller passing nothing. ## Collation version note The remedy is correct and I ran it verbatim against the schema: ``` REINDEX INDEX ux_users_email; -- REINDEX ALTER COLLATION case_insensitive REFRESH VERSION; -- NOTICE: version has not changed ``` Both statements are valid and the order is the important half — reindexing first, then refreshing, because refreshing first would clear the warning while the btree was still ordered under the old ICU. The note says "in this order" and that emphasis earns its place. Naming `ux_users_email` as the index at risk and recording `153.128` makes it actionable rather than a caution. ## Verified at this head - `dotnet build` and `dotnet build -c Release` — succeeded, **0 warnings, 0 errors**. - `dotnet test -c Release` — **27 passed, 0 failed** (4 Api, 1 WebUI, 22 Infrastructure). - `dotnet format --verify-no-changes` — exit 0. - The DDL is unchanged from `96dbdf5` apart from the collation comment block, so my exhaustive catalogue diff from the first review still stands: 32 columns, 8 foreign keys with the specified delete actions, 10 indexes, both composite primary keys, case-insensitive matching and `23001` on the `RESTRICT` — all still matching `docs/data-model.md`. ## One factual correction, immaterial The note about a mid-flight rebase onto `main` for task 57 does not match the branch: `5d31299`'s parent is still `96dbdf5`, whose parent is still `df1308d` ("Establish the Minimal API endpoint structure (#16)") — the same base my first review ran against. `PlaceMark.Api.Tests` reported 4 tests at `96dbdf5` too, not 3, so nothing changed there. No rebase appears to have happened, and none was needed. Flagging only so the record is accurate; it has no bearing on the verdict. Nothing outstanding. The schema matches the specification, and the tests now hold down every dimension of it that the specification states.

rob merged commit 9c84303a80 into main 2026-08-03 11:38:19 +00:00

rob deleted branch feat/initial-schema-script 2026-08-03 11:38:20 +00:00

rob referenced this pull request from a commit 2026-08-03 11:38:22 +00:00

Add the initial schema script (#17)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!17

No description provided.