Use Dapper and DbUp instead of EF Core (ADR-0020, ADR-0021) #14

Merged

rob merged 2 commits from docs/dapper-decision into main 2026-08-03 09:38:24 +00:00

Conversation 2 Commits 2 Files changed 7 +721 -141

rob commented 2026-08-03 09:20:48 +00:00

Owner

Copy link

Records two decisions taken by the product owner, and follows the consequences. Documentation only.

• ADR-0020 — data access is Dapper over Npgsql, hand-written SQL, no ORM.
• ADR-0021 — schema applied by DbUp from journalled .sql scripts.

Nothing is superseded. EF Core was never an ADR — it was asserted in CLAUDE.md and assumed by the data model, both freely edited. Three accepted records mention it in frozen bodies (0005, 0009, 0019), but their decisions stand; only the technology their consequence sections assumed has changed. ADR-0020 records the translation for each ("read DeleteBehavior.Restrict as ON DELETE RESTRICT").

ADR-0016 point 4 reopened — ICU collation wins over citext

Its deciding argument was EF migration ergonomics, and there are no migrations. Measured against postgres:18.4-trixie and 18-alpine:

• ICU is one line of ordinary DDL; citext needs CREATE EXTENSION and rights a managed application role may not have.
• On PG 18 LIKE works under a non-deterministic collation. Regex and text_pattern_ops still refuse — ADR-0006 excludes search, and the failure is an error, not a wrong answer.
• One of ADR-0016's own claims was disproved: it said citext rules out the Alpine image. It does not — Alpine ships it in contrib.
• New cost recorded: the collation carries a version, so an ICU change on an image bump can require REINDEX + REFRESH VERSION.

Other four points unchanged. UUIDv7's rationale strengthens under Dapper — registration inserts user, personal group and membership in one transaction, and the last two need an id the first has not returned.

Two DbUp behaviours that are load-bearing and silent

Both measured, both in ADR-0021's Decision rather than left to implementation:

• No transaction unless told. A two-statement script whose second statement failed left the first behind, unjournalled — so the next run reapplies and fails on the object that now exists. WithTransactionPerScript() rolls it back.
• The journal key is the full embedded resource name, so renaming the project, its root namespace or the scripts folder re-runs every script. Invisible in a diff.

Needs your attention

Three tickets need retitling (43, 44, 45 all name EF Core). One acceptance criterion does not survive: #44's "Migration is reversible (Down method implemented/tested)" — ADR-0021 argues against down scripts explicitly.

Verification

dotnet build 0/0, dotnet format clean, links 311/39 anchors/0 problems. README.md had four EF references, all fixed; CONTRIBUTING.md had none.

Records two decisions taken by the product owner, and follows the consequences. Documentation only. - **ADR-0020** — data access is Dapper over Npgsql, hand-written SQL, no ORM. - **ADR-0021** — schema applied by DbUp from journalled `.sql` scripts. **Nothing is superseded.** EF Core was never an ADR — it was asserted in `CLAUDE.md` and assumed by the data model, both freely edited. Three accepted records mention it in frozen bodies (0005, 0009, 0019), but their decisions stand; only the technology their consequence sections assumed has changed. ADR-0020 records the translation for each ("read `DeleteBehavior.Restrict` as `ON DELETE RESTRICT`"). ## ADR-0016 point 4 reopened — ICU collation wins over `citext` Its deciding argument was EF migration ergonomics, and there are no migrations. Measured against `postgres:18.4-trixie` and `18-alpine`: - ICU is one line of ordinary DDL; `citext` needs `CREATE EXTENSION` and rights a managed application role may not have. - On PG 18 `LIKE` works under a non-deterministic collation. Regex and `text_pattern_ops` still refuse — ADR-0006 excludes search, and the failure is an error, not a wrong answer. - **One of ADR-0016's own claims was disproved:** it said `citext` rules out the Alpine image. It does not — Alpine ships it in contrib. - New cost recorded: the collation carries a version, so an ICU change on an image bump can require `REINDEX` + `REFRESH VERSION`. Other four points unchanged. UUIDv7's rationale *strengthens* under Dapper — registration inserts user, personal group and membership in one transaction, and the last two need an id the first has not returned. ## Two DbUp behaviours that are load-bearing and silent Both measured, both in ADR-0021's **Decision** rather than left to implementation: - **No transaction unless told.** A two-statement script whose second statement failed left the first behind, unjournalled — so the next run reapplies and fails on the object that now exists. `WithTransactionPerScript()` rolls it back. - **The journal key is the full embedded resource name**, so renaming the project, its root namespace *or* the scripts folder re-runs every script. Invisible in a diff. ## Needs your attention **Three tickets need retitling** (43, 44, 45 all name EF Core). One acceptance criterion does not survive: **#44's "Migration is reversible (`Down` method implemented/tested)"** — ADR-0021 argues against down scripts explicitly. ## Verification `dotnet build` 0/0, `dotnet format` clean, links 311/39 anchors/0 problems. `README.md` had four EF references, all fixed; `CONTRIBUTING.md` had none.

rob added 1 commit 2026-08-03 09:20:49 +00:00

Use Dapper and DbUp instead of EF Core ... b647dd50b2

Records two decisions taken by the product owner: data access is Dapper
over Npgsql with hand-written SQL, and schema changes are applied by DbUp
from journalled .sql scripts.

Two records rather than one: they are separable in both directions, and
keeping EF Core solely to generate migrations was a live option a
combined record could not have expressed.

Nothing is superseded. EF Core was never an ADR — it was asserted in
CLAUDE.md and assumed by the data model, both freely edited. Three
accepted records name it in frozen bodies, but hard delete, the cascade,
RESTRICT and the contracts boundary all stand; only the technology their
consequence sections assumed has changed. ADR-0020 records the
translation for each instead.

Reopens ADR-0016 point 4, which is still Proposed. citext beat an ICU
collation on EF migration ergonomics alone, and there are no migrations.
An ICU non-deterministic collation is one line of ordinary DDL where
citext needs CREATE EXTENSION and rights a managed application role may
not have. One of ADR-0016's own claims was disproved in the process:
citext does not rule out the Alpine image, which ships it in contrib. The
new cost is recorded — the collation carries a version, so an image bump
can require a REINDEX.

Replaces the EF-specific guidance in the data model rather than deleting
it. An omitted ON DELETE is valid SQL meaning NO ACTION, so all eight
constraints must state their clause; PostgreSQL indexes a referencing
column only when told, so three FK indexes are written and three
deliberately are not. The mirror failure is named: an index nobody writes
does not exist.

rob reviewed 2026-08-03 09:30:48 +00:00

rob left a comment

Copy link

Verdict: changes needed

One blocking item, and it is a typo rather than an argument: a sentence in docs/data-model.md stops mid-clause. Everything else below is optional.

I re-ran the measurements rather than taking them on trust. Every claim in the PR body that I could execute held, including the two that were hardest to believe.

Blocking

docs/data-model.md:231 — truncated sentence in the users constraint table.

| `ux_users_email` | `UNIQUE (email)` — case-insensitive, because the column's collation is |

The cell ends on "is". It reads as a dropped edit from the citext → collation rewrite (the old cell ended "because the column is citext"). It is also the one line in the document a reader checks to find out why the unique constraint is case-insensitive, so it is worth more than the character count suggests. Something like "because the column's collation is non-deterministic" closes it.

Verification of the four claims the PR stakes itself on

1. Supersession: I agree nothing is superseded. I read all three mentions in place. ADR-0005's is in Consequences, ADR-0019's is in Consequences, ADR-0009's is in Context (the hazard motivating the contracts boundary). None is in a Decision section, and none of the three decisions changes: hard delete, the cascade, RESTRICT on group_memberships.user_id, and the contracts boundary all read correctly today. ADR-0001 reserves partial supersession for a record "replacing part of an old decision", and no decision has been replaced — only the mechanism a consequence named. Adding Partially superseded by 0020 would misreport all three, exactly as ADR-0020 argues. The obligation in ADR-0019 ("must be told DeleteBehavior.Restrict") is the closest call, because it is phrased as an instruction; but the instruction it encodes — this relationship is RESTRICT, not CASCADE — survives verbatim, and the row in the foreign key table is unchanged.

The residual cost is discoverability, and it is not fixable in this PR: a reader who lands on ADR-0019 directly gets no forward pointer, because ADR-0001 permits only status-field edits. The index prose covers it for anyone who arrives that way. Worth noting that ADR-0019 already flagged the mechanism that would fix this — an Answered by metadata field — and explicitly deferred it. That deferral now has a second customer.

2. ICU collation over citext — every claim reproduced, including the disproof. Against postgres:18.4-trixie:

• CREATE COLLATION case_insensitive (provider = icu, locale = 'und-u-ks-level2', deterministic = false) succeeds with no extension.
• Equality is case-insensitive; ux_users_email rejected ROB@example.com against a stored Rob@Example.COM with duplicate key value violates unique constraint.
• LIKE works, and is case-insensitive under it — both anchored (LIKE 'rob@%') and mid-string (LIKE '%EXAMPLE%') matched.
• Regex is refused with precisely the message quoted: nondeterministic collations are not supported for regular expressions. text_pattern_ops likewise: nondeterministic collations are not supported for operator class "text_pattern_ops".
• pg_collation.collversion is 153.128, the figure recorded.
• The index is genuinely usable for the lookup: with 20k rows and ANALYZE, EXPLAIN gives Index Scan using ux_users_email, not a seq scan. This is the claim I most expected to fail, and it did not.

On citext: CREATE TABLE t (email citext) fails with type "citext" does not exist until CREATE EXTENSION citext — the quoted error, verbatim.

The Alpine correction is right, and ADR-0016's original claim was wrong. postgres:18-alpine lists citext 1.8 in pg_available_extensions and CREATE EXTENSION citext succeeds on it. Withdrawing that reason rather than quietly deleting it is the right call. The collation also works on Alpine — and, incidentally, its collversion there is 153.136 against trixie's 153.128, so the two official variants of the same PostgreSQL 18.4 already carry different ICU. That does not create a mismatch in practice (each database creates its own collation against its own library), but it is a rather better illustration of the version cost ADR-0016 records than the hypothetical image bump.

ADR-0016's enum aside also holds: BEGIN; ALTER TYPE role ADD VALUE 'viewer'; SELECT 'viewer'::role; fails with unsafe use of new value "viewer" of enum type role, as quoted.

3. Both DbUp claims reproduced. dbup-postgresql 7.0.1 (still the current version) against postgres:18.4-trixie, two scripts, the second creating a table twice:

• Default strategy: run failed at the second statement, and beta was left behind in the database, unjournalled — journal contained only 0001. The re-run then failed with 42P07: relation "beta" already exists, which is the exact failure mode the ADR describes.

• WithTransactionPerScript(): same script, nothing left behind — only alpha and schemaversions — and 0001 stayed journalled.

• Journal key: the entry is Rev14Probe.Scripts.0001-good.sql. Changing only <RootNamespace> to PlaceMark.Database, touching no script, re-discovered it as PlaceMark.Database.Scripts.0001-good.sql and re-ran it against a database that already had it, failing on alpha. The claim is understated if anything: the journal keeps the old key, so the failure is permanent until someone edits the journal.

I also confirmed the ordering claim in ADR-0016/0021 end to end — CREATE COLLATION followed by a table declaring COLLATE case_insensitive followed by an insert, in one transactional script, applies cleanly and matches case-insensitively afterwards.

4. UUIDv7 — conclusion right, one clause overstated. "A key the application cannot know before it writes is a key it cannot use to write the rows that reference it" is sound, and "every insert would have to read its key back with a RETURNING clause" is exactly right. But the registration example goes one step further than the evidence: "each of the last two needs an identifier the first has not returned yet." A data-modifying CTE chains them in a single statement —

WITH new_user AS (INSERT INTO u2(name) VALUES ('rob') RETURNING id)
INSERT INTO g2(owner_id, name) SELECT id, 'personal' FROM new_user RETURNING id, owner_id;

— which I ran successfully against uuidv7() column defaults on 18.4. So database generation is awkward here, not impossible: the real costs are the extra round trip per insert and the inability to build the graph before writing, both of which the preceding sentence already states correctly. ADR-0016 is still Proposed, so this is cheap to sharpen now and expensive later. Softening it to something like "each of the last two needs an identifier that only exists once the first has been written and read back" keeps the argument and drops the part that is not quite true.

Non-blocking

docs/adr/README.md:88 — "the initial migration is no longer blocked on either." The sweep caught every equivalent phrase in docs/data-model.md (four of them, all now "first schema script") but missed this one. Index prose is editable, and this is the one place a reader is told the schema is unblocked.

ADR-0021 does not record what WithTransactionPerScript() costs. It is put in the Decision as a blanket rule, correctly, and the record calls the builder call load-bearing — but transactional DDL is not universal. Measured on the same setup: a script containing CREATE INDEX CONCURRENTLY fails under WithTransactionPerScript() with 25001: CREATE INDEX CONCURRENTLY cannot run inside a transaction block, and succeeds under the default strategy. ADR-0016's enum paragraph already handles a cousin of this ("has to be two scripts"), so the record clearly knows the family; the general statement is missing. At ADR-0007's scale nothing needs a concurrent index build and the failure is loud, so this is a sentence in Consequences rather than a change of decision — but it is the sort of thing whose absence costs an hour on the day someone hits it.

docs/data-model.md:496 asserts an equivalence it does not argue. "Five of the eight are the ones an ORM's conventions would have got wrong, and they are worth knowing as the five most likely to be got wrong by hand too." Plausible — those five are the ones deviating from an intuitive cascade — but with hand-written DDL any of the eight can simply be omitted, and the paragraph two above already gives the better instruction ("a count, not a search"). The ORM framing is doing rhetorical work here in a document that has just declared there are no conventions left to fight.

ADR-0020 calls account deletion "three ordered statements". It is three ordered steps; step 3 is two statements and step 1 is a read. The linked section is authoritative and correct, so this is cosmetic.

Everything else checked

• Index contiguous 0001–0021, no gaps, and every status in the table matches the Status field in the file it links to (0016 correctly still Proposed in both).
• 313 relative links and heading anchors across 27 Markdown files resolve; 0 problems.
• dotnet build: 0 warnings, 0 errors. dotnet format --verify-no-changes: clean.
• No EF Core or "migration" residue outside frozen ADR bodies, other than the index line noted above. CONTRIBUTING.md had none, as stated.
• British English throughout the changed files; the only hits on an American-spelling sweep are SERIALIZABLE, DeleteBehavior and a Forgejo scope string, all external names.
• Dapper.DefaultTypeMap.MatchNamesWithUnderscores is a settable static on the current Dapper (2.1.79), so the consequence about global start-up state is accurate; Guid.CreateVersion7() is present on the pinned SDK (10.0.110), as ADR-0016 says.

The two documents are unusually well evidenced for documentation-only records — the DbUp transaction default in particular is the kind of thing normally discovered in production — and the decision to correct ADR-0016's Alpine claim in the open, at the cost of admitting the original reasoning was unchecked, is the right instinct. Fix the truncated line and this is mergeable.

Verdict: changes needed One blocking item, and it is a typo rather than an argument: a sentence in `docs/data-model.md` stops mid-clause. Everything else below is optional. I re-ran the measurements rather than taking them on trust. Every claim in the PR body that I could execute held, including the two that were hardest to believe. ## Blocking **`docs/data-model.md:231` — truncated sentence in the `users` constraint table.** ``` | `ux_users_email` | `UNIQUE (email)` — case-insensitive, because the column's collation is | ``` The cell ends on "is". It reads as a dropped edit from the `citext` → collation rewrite (the old cell ended "because the column is `citext`"). It is also the one line in the document a reader checks to find out *why* the unique constraint is case-insensitive, so it is worth more than the character count suggests. Something like "because the column's collation is non-deterministic" closes it. ## Verification of the four claims the PR stakes itself on **1. Supersession: I agree nothing is superseded.** I read all three mentions in place. ADR-0005's is in **Consequences**, ADR-0019's is in **Consequences**, ADR-0009's is in **Context** (the hazard motivating the contracts boundary). None is in a Decision section, and none of the three decisions changes: hard delete, the cascade, `RESTRICT` on `group_memberships.user_id`, and the contracts boundary all read correctly today. ADR-0001 reserves partial supersession for a record "replacing part of an old decision", and no decision has been replaced — only the mechanism a consequence named. Adding `Partially superseded by 0020` would misreport all three, exactly as ADR-0020 argues. The obligation in ADR-0019 ("must be told `DeleteBehavior.Restrict`") is the closest call, because it is phrased as an instruction; but the instruction it encodes — this relationship is `RESTRICT`, not `CASCADE` — survives verbatim, and the row in the foreign key table is unchanged. The residual cost is discoverability, and it is not fixable in this PR: a reader who lands on ADR-0019 directly gets no forward pointer, because ADR-0001 permits only status-field edits. The index prose covers it for anyone who arrives that way. Worth noting that ADR-0019 already flagged the mechanism that would fix this — an `Answered by` metadata field — and explicitly deferred it. That deferral now has a second customer. **2. ICU collation over `citext` — every claim reproduced, including the disproof.** Against `postgres:18.4-trixie`: - `CREATE COLLATION case_insensitive (provider = icu, locale = 'und-u-ks-level2', deterministic = false)` succeeds with no extension. - Equality is case-insensitive; `ux_users_email` rejected `ROB@example.com` against a stored `Rob@Example.COM` with `duplicate key value violates unique constraint`. - **`LIKE` works**, and is case-insensitive under it — both anchored (`LIKE 'rob@%'`) and mid-string (`LIKE '%EXAMPLE%'`) matched. - Regex is refused with precisely the message quoted: `nondeterministic collations are not supported for regular expressions`. `text_pattern_ops` likewise: `nondeterministic collations are not supported for operator class "text_pattern_ops"`. - `pg_collation.collversion` is **`153.128`**, the figure recorded. - The index is genuinely usable for the lookup: with 20k rows and `ANALYZE`, `EXPLAIN` gives `Index Scan using ux_users_email`, not a seq scan. This is the claim I most expected to fail, and it did not. On `citext`: `CREATE TABLE t (email citext)` fails with `type "citext" does not exist` until `CREATE EXTENSION citext` — the quoted error, verbatim. **The Alpine correction is right, and ADR-0016's original claim was wrong.** `postgres:18-alpine` lists `citext` 1.8 in `pg_available_extensions` and `CREATE EXTENSION citext` succeeds on it. Withdrawing that reason rather than quietly deleting it is the right call. The collation also works on Alpine — and, incidentally, its `collversion` there is **`153.136`** against trixie's `153.128`, so the two official variants of the same PostgreSQL 18.4 already carry different ICU. That does not create a mismatch in practice (each database creates its own collation against its own library), but it is a rather better illustration of the version cost ADR-0016 records than the hypothetical image bump. ADR-0016's enum aside also holds: `BEGIN; ALTER TYPE role ADD VALUE 'viewer'; SELECT 'viewer'::role;` fails with `unsafe use of new value "viewer" of enum type role`, as quoted. **3. Both DbUp claims reproduced.** `dbup-postgresql` 7.0.1 (still the current version) against `postgres:18.4-trixie`, two scripts, the second creating a table twice: - **Default strategy:** run failed at the second statement, and `beta` was **left behind in the database, unjournalled** — journal contained only `0001`. The re-run then failed with `42P07: relation "beta" already exists`, which is the exact failure mode the ADR describes. - **`WithTransactionPerScript()`:** same script, nothing left behind — only `alpha` and `schemaversions` — and `0001` stayed journalled. - **Journal key:** the entry is `Rev14Probe.Scripts.0001-good.sql`. Changing **only** `<RootNamespace>` to `PlaceMark.Database`, touching no script, re-discovered it as `PlaceMark.Database.Scripts.0001-good.sql` and re-ran it against a database that already had it, failing on `alpha`. The claim is understated if anything: the journal keeps the old key, so the failure is permanent until someone edits the journal. I also confirmed the ordering claim in ADR-0016/0021 end to end — `CREATE COLLATION` followed by a table declaring `COLLATE case_insensitive` followed by an insert, in one transactional script, applies cleanly and matches case-insensitively afterwards. **4. UUIDv7 — conclusion right, one clause overstated.** "A key the application cannot know before it writes is a key it cannot use to write the rows that reference it" is sound, and "every insert would have to read its key back with a `RETURNING` clause" is exactly right. But the registration example goes one step further than the evidence: *"each of the last two needs an identifier the first has not returned yet."* A data-modifying CTE chains them in a single statement — ```sql WITH new_user AS (INSERT INTO u2(name) VALUES ('rob') RETURNING id) INSERT INTO g2(owner_id, name) SELECT id, 'personal' FROM new_user RETURNING id, owner_id; ``` — which I ran successfully against `uuidv7()` column defaults on 18.4. So database generation is awkward here, not impossible: the real costs are the extra round trip per insert and the inability to build the graph before writing, both of which the preceding sentence already states correctly. ADR-0016 is still Proposed, so this is cheap to sharpen now and expensive later. Softening it to something like "each of the last two needs an identifier that only exists once the first has been written and read back" keeps the argument and drops the part that is not quite true. ## Non-blocking **`docs/adr/README.md:88` — "the initial migration is no longer blocked on either."** The sweep caught every equivalent phrase in `docs/data-model.md` (four of them, all now "first schema script") but missed this one. Index prose is editable, and this is the one place a reader is told the schema is unblocked. **ADR-0021 does not record what `WithTransactionPerScript()` costs.** It is put in the Decision as a blanket rule, correctly, and the record calls the builder call load-bearing — but transactional DDL is not universal. Measured on the same setup: a script containing `CREATE INDEX CONCURRENTLY` fails under `WithTransactionPerScript()` with `25001: CREATE INDEX CONCURRENTLY cannot run inside a transaction block`, and succeeds under the default strategy. ADR-0016's enum paragraph already handles a cousin of this ("has to be two scripts"), so the record clearly knows the family; the general statement is missing. At ADR-0007's scale nothing needs a concurrent index build and the failure is loud, so this is a sentence in Consequences rather than a change of decision — but it is the sort of thing whose absence costs an hour on the day someone hits it. **`docs/data-model.md:496` asserts an equivalence it does not argue.** "Five of the eight are the ones an ORM's conventions would have got wrong, and they are worth knowing as the five most likely to be got wrong by hand too." Plausible — those five are the ones deviating from an intuitive cascade — but with hand-written DDL any of the eight can simply be omitted, and the paragraph two above already gives the better instruction ("a count, not a search"). The ORM framing is doing rhetorical work here in a document that has just declared there are no conventions left to fight. **ADR-0020 calls account deletion "three ordered statements".** It is three ordered *steps*; step 3 is two statements and step 1 is a read. The linked section is authoritative and correct, so this is cosmetic. ## Everything else checked - Index contiguous 0001–0021, no gaps, and every status in the table matches the `Status` field in the file it links to (0016 correctly still `Proposed` in both). - 313 relative links and heading anchors across 27 Markdown files resolve; 0 problems. - `dotnet build`: 0 warnings, 0 errors. `dotnet format --verify-no-changes`: clean. - No EF Core or "migration" residue outside frozen ADR bodies, other than the index line noted above. `CONTRIBUTING.md` had none, as stated. - British English throughout the changed files; the only hits on an American-spelling sweep are `SERIALIZABLE`, `DeleteBehavior` and a Forgejo scope string, all external names. - `Dapper.DefaultTypeMap.MatchNamesWithUnderscores` is a settable static on the current Dapper (2.1.79), so the consequence about global start-up state is accurate; `Guid.CreateVersion7()` is present on the pinned SDK (10.0.110), as ADR-0016 says. The two documents are unusually well evidenced for documentation-only records — the DbUp transaction default in particular is the kind of thing normally discovered in production — and the decision to correct ADR-0016's Alpine claim in the open, at the cost of admitting the original reasoning was unchecked, is the right instinct. Fix the truncated line and this is mergeable.

rob added 1 commit 2026-08-03 09:35:17 +00:00

Action review on the Dapper and DbUp records ... b8e9b81ad2

Fixes a truncated cell left by the citext-to-collation rewrite.

Records a cost of the blanket WithTransactionPerScript rule that was
missing: CREATE INDEX CONCURRENTLY fails inside a transaction block with
25001. The remedy is a separate upgrader for that one script rather than
dropping the strategy, and the escape hatch's own cost is measured too —
a concurrent index build that fails outside a transaction leaves an
invalid index behind that must be dropped by hand.

Concedes that the UUIDv7 clause was wrong as written: a data-modifying
CTE does chain uuidv7() defaults through RETURNING in one statement. The
weight moves to the reason that decides it — the client cannot name a row
it has not created.

Makes the five-of-eight foreign key claim argue itself rather than assert
an equivalence with what an ORM gets wrong: the shared cause is inferring
delete behaviour from nullability rather than from what should happen to
the referencing row. Corrects the arithmetic while rewriting it — the
four optional references are three SET NULL plus one that cascades.

rob reviewed 2026-08-03 09:38:00 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at b8e9b81. My earlier verdict at b647dd5 is superseded by this one. All five points are actioned, and the two that added new factual claims I re-measured rather than accepted.

The blocking item is closed

docs/data-model.md:231 now reads "case-insensitive, because that is what the column's collation makes it". Complete sentence, and it says the thing the cell exists to say.

The four non-blocking points

Index prose. "first schema script". I re-ran the sweep against the new head: every remaining migration in docs/ is either deliberate (ADR-0020 and ADR-0021 discussing EF migrations by name, ADR-0016 and the data model quoting the reason that was withdrawn) or in a frozen body using the word generically — ADR-0006:38 and ADR-0017:209, the two you named. Nothing left to sweep.

CREATE INDEX CONCURRENTLY. Recorded in both the Decision ("it is not free either") and the Consequences, which is the right split — a reader who only skims the Decision now knows the strategy has a cost. The new claim in it is correct: a CREATE UNIQUE INDEX CONCURRENTLY that fails on duplicate data left ix_dup behind with pg_index.indisvalid = f, needing a manual drop, exactly as written. Recording the escape hatch's own cost is what makes the paragraph worth having — "just run it outside a transaction" is the advice someone would otherwise reach for at the worst moment.

One refinement, entirely optional and not worth a revision on its own: "the strategy is chosen per run, not per script" is right about DbUp's API, and it is the reason the escape hatch works — but it also means the separate run must be filtered to that one script, or every other pending script in the same run loses its transaction too. The record's "for one script" implies this; spelling it out would close the gap for whoever reaches for it under pressure.

UUIDv7. The concession is better than what I asked for. Rather than softening the clause it states the wrong version explicitly, says why it is wrong, and moves the weight onto Client-side creation — which is the argument that was always doing the work and does not depend on what the database can chain. Naming the cost that survives (the statement's shape being dictated by where keys come from) keeps the alternative's real merit visible instead of pretending it has none.

Five of eight. The rewrite argues the shared cause — inferring delete behaviour from nullability — and the ORM equivalence now follows from it rather than being asserted alongside it, which is the right order. That also explains why EF Core would have got the same five wrong: same inference, performed by a tool. The arithmetic is right, and the correction matters: the four optional references are three SET NULL plus groups.personal_for_user_id, which is nullable (Null: yes in the groups field table) and cascades. Four optional and four required, five in the at-risk set, eight rows. Catching that mid-rewrite is the kind of thing that would have read as authoritative and been wrong.

ADR-0020's "three ordered statements" is now "three ordered steps — several statements, since step 3 alone deletes memberships and then the user". Matches the data model.

Re-verified at the new head

• 314 relative links, 39 with anchors, 0 problems — matching your figures.
• The delta from b647dd5 touches seven Markdown files and nothing else, so the build and format results cannot have moved from the 0/0 and clean I measured on the previous revision; your re-run agrees.
• Failed concurrent index leaves indisvalid = f: measured on postgres:18.4-trixie.
• Everything I verified on the previous revision — the ICU collation behaviour and its collversion, citext on Alpine, both DbUp behaviours and the journal key — is untouched by these edits.

Nothing outstanding. Container and scratch directories from both passes are cleaned up.

Verdict: mergeable Re-reviewed at `b8e9b81`. My earlier verdict at `b647dd5` is superseded by this one. All five points are actioned, and the two that added new factual claims I re-measured rather than accepted. ## The blocking item is closed `docs/data-model.md:231` now reads "case-insensitive, because that is what the column's collation makes it". Complete sentence, and it says the thing the cell exists to say. ## The four non-blocking points **Index prose.** "first schema script". I re-ran the sweep against the new head: every remaining `migration` in `docs/` is either deliberate (ADR-0020 and ADR-0021 discussing EF migrations by name, ADR-0016 and the data model quoting the reason that was withdrawn) or in a frozen body using the word generically — ADR-0006:38 and ADR-0017:209, the two you named. Nothing left to sweep. **`CREATE INDEX CONCURRENTLY`.** Recorded in both the Decision ("it is not free either") and the Consequences, which is the right split — a reader who only skims the Decision now knows the strategy has a cost. The new claim in it is correct: a `CREATE UNIQUE INDEX CONCURRENTLY` that fails on duplicate data left `ix_dup` behind with `pg_index.indisvalid = f`, needing a manual drop, exactly as written. Recording the escape hatch's own cost is what makes the paragraph worth having — "just run it outside a transaction" is the advice someone would otherwise reach for at the worst moment. One refinement, entirely optional and not worth a revision on its own: "the strategy is chosen per run, not per script" is right about DbUp's API, and it is the reason the escape hatch works — but it also means the separate run must be filtered to that one script, or every other pending script in the same run loses its transaction too. The record's "for one script" implies this; spelling it out would close the gap for whoever reaches for it under pressure. **UUIDv7.** The concession is better than what I asked for. Rather than softening the clause it states the wrong version explicitly, says why it is wrong, and moves the weight onto *Client-side creation* — which is the argument that was always doing the work and does not depend on what the database can chain. Naming the cost that survives (the statement's shape being dictated by where keys come from) keeps the alternative's real merit visible instead of pretending it has none. **Five of eight.** The rewrite argues the shared cause — inferring delete behaviour from nullability — and the ORM equivalence now follows from it rather than being asserted alongside it, which is the right order. That also explains why EF Core would have got the same five wrong: same inference, performed by a tool. The arithmetic is right, and the correction matters: the four optional references are three `SET NULL` plus `groups.personal_for_user_id`, which is nullable (`Null: yes` in the `groups` field table) and cascades. Four optional and four required, five in the at-risk set, eight rows. Catching that mid-rewrite is the kind of thing that would have read as authoritative and been wrong. **ADR-0020's "three ordered statements"** is now "three ordered **steps** — several statements, since step 3 alone deletes memberships and then the user". Matches the data model. ## Re-verified at the new head - 314 relative links, 39 with anchors, 0 problems — matching your figures. - The delta from `b647dd5` touches seven Markdown files and nothing else, so the build and format results cannot have moved from the 0/0 and clean I measured on the previous revision; your re-run agrees. - Failed concurrent index leaves `indisvalid = f`: measured on `postgres:18.4-trixie`. - Everything I verified on the previous revision — the ICU collation behaviour and its `collversion`, `citext` on Alpine, both DbUp behaviours and the journal key — is untouched by these edits. Nothing outstanding. Container and scratch directories from both passes are cleaned up.

rob merged commit 4de51223ab into main 2026-08-03 09:38:24 +00:00

rob deleted branch docs/dapper-decision 2026-08-03 09:38:25 +00:00

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

Use Dapper and DbUp instead of EF Core (#14)

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!14

No description provided.