fix(dapper): match PostgreSQL provider names and quote identifiers via dialect hook

[sfmskywalker]

Port of #258 onto main. Tracking issue: #255.

Cause

FluentMigrator 7.2 AddPostgres() registers Postgres15_0Processor (DatabaseType PostgreSQL15_0, aliases PostgreSQL15_0, PostgreSQL). IfDatabase("Postgres") never matches.

IfDatabase(params string[]) is exact OrdinalIgnoreCase against DatabaseType or aliases. The predicate overload sees only DatabaseType — do not use it here.

After migrate, FluentMigrator force-quotes PG identifiers while the Dapper store and shared query builder emitted unquoted names (42P01 / 42703). PostgreSqlDialect.Upsert omitted the PK (23502). VersionOptions compared boolean columns to integers (42883). Instance search inlined ID, which quoted on PG is 42703 against "Id". After persist, Find failed because the SQLite string-only DateTimeOffset handler is registered on SqlMapper and Npgsql returns DateTime.

Fix

Migrations and the store fix stay together in this PR (same as #258).

Shared MigrationDatabases.DateTimeOffsetProviders used with the alias-aware params overload:

SqlServer, Oracle, MySql, Postgres, PostgreSQL, PostgreSQL92

at 10001, 20001 and 20004.

The hooks live in shared ISqlDialect / the query builder. Only PostgreSqlDialect overrides them. Other providers' SQL is byte-identical to the pre-hook capture, except one token (see below).

• QuoteIdentifier — default is a no-op; PostgreSqlDialect double-quotes.
• BooleanLiteral — default emits 1/0; PostgreSqlDialect emits true/false.

Every identifier ParameterizedQueryBuilderExtensions inlines goes through QuoteIdentifier. Is(VersionOptions) uses BooleanLiteral. TryMarkInterruptedAsync quotes Status. AndWorkflowInstanceSearchTerm uses query.QuoteIdent("Id") (no dialect sniff). Main has no LessThan (that call site exists only on 3.9).

Single intentional non-PG SQL change (release-lead sign-off)

AndWorkflowInstanceSearchTerm used to emit unquoted ID. It now emits Id via plain query.QuoteIdent("Id"). The golden snapshot is updated in the same commit.

This is the only exception to "byte-identical" non-PG SQL. It also fixes instance search on case-sensitive SQL Server collations, where the column is Id.

Handmade lowercase, unquoted PG schemas

Yes, quoted identifiers break those databases. Accepted.

Official Dapper+PG is the FluentMigrator mixed-case quoted schema. Handmade lowercase schemas are unsupported — they never worked (upsert omitted the PK since 3.0).

Rename-path caveat: migrating against an un-renamed lowercase table creates an empty mixed-case duplicate ("WorkflowDefinitions" next to workflowdefinitions) because TableExists is case-sensitive. Renamed tables need matching VersionInfo rows. Otherwise, recreate via the migrations and re-import. FluentMigrator quoting is not turned off.

Release note

• 3.6–3.8 PG installs stuck at 10002: DELETE FROM "VersionInfo" WHERE "Version" = 10001; then migrate up.
• Instance search now uses Id instead of ID. That fixes instance search on case-sensitive SQL Server collations, where the column is Id.

Tests

Same as #258 except LessThan / BeforeLastUpdated (main does not have them): real processors; SQLite VersionInfo replay; dialect SQL-shape tests; non-PG snapshot including the ID → Id token; Testcontainers fresh-PG migrate + persist + WriteLine + publish + FindWorkflowGraphAsync(Published) + VersionOptions + Studio lists + journal/activity-execution reads + bookmark-queue paging + paged delete (built in the test; Store.DeleteAsync unchanged, filed separately for 3.10) + TryMarkInterruptedAsync.

3.9 counterpart: #258

  

[sfmskywalker]

Code Review, Round 1/4: REQUEST_CHANGES + HIGH @ 5c3b5c3

This is the main port of #258. Its diff is identical to #258's except for the Directory.Packages.props index line and hunk offset. The full analysis is in the #258 Round 1 review, and the same findings apply here.

Blockers

• B1: SQL Server regression from the predicate. FluentMigrator 7.2 AddSqlServer() reports DatabaseType SqlServer2016, alias SqlServer, and the predicate only sees DatabaseType. IsDateTimeOffsetProvider's exact "SqlServer" match therefore fails, and so does IfDatabase("Sqlite"). The result is that 10001, 20001 and 20004 create no tables on SQL Server, and on AddMySql*, AddOracleManaged and AddOracle12C* too. All of these matched through aliases before.
  • Verified: running the real migrations with SQL Server's processor identifiers gives 13 tables on base and 42P01 WorkflowDefinitions at 10002 with this PR.
  • Fix: use the alias-aware params overload with the PG names added, e.g. ["SqlServer", "Oracle", "MySql", "Postgres", "PostgreSQL", "PostgreSQL92"], and correct the DoesNotChangeOtherProviders theory data.
  • Test: resolve the real processors for AddSqlServer(), AddSQLite() and AddPostgres(), and assert that exactly one create branch applies.
• B2: CI is red. The fresh-PG Testcontainers test runs in CI (it is not skipped) and fails: 42P01: relation "activityexecutionrecords" does not exist at DapperPostgreSqlMigrationTests.cs:70. Summary: Dapper UnitTests Failed 1 / Passed 40; overall Failed: 1, Passed: 438, Skipped: 2. It reproduces on local PG17.
  • The migrations succeed. The store fails because the Dapper PG dialect uses unquoted identifiers while FluentMigrator force-quotes. Behind that, PostgreSqlDialect.Upsert leaves the primary key out of the insert list (23502 null id).
  • Fix: quote PG identifiers and include the PK in the PG upsert. Alternatively, narrow the test to migrations only and track the store fix as a separate blocker.

Non-blocking

Same as the #258 Round 1 review:

• Add a release note for PG DBs already stuck at 10001. DELETE FROM "VersionInfo" WHERE "Version" = 10001; then migrating up heals them (verified).
• The Exists guards accept schema drift, and they don't cover 20002/30001.
• Fail fast on an unknown provider.
• Naming, and the XML doc listing aliases as DatabaseType values.

Verification

• Tests:
  • Elsa.Persistence.Dapper.UnitTests: 40/41 locally. The only failure is the Testcontainers test, because there is no Docker locally.
  • Pointed at local PG17, it fails exactly as in CI.
  • Elsa.Dapper.UnitTests: 11/11.
• Probes and revert checks: identical code to #258, so they carry over. The SQLite and PG17 fresh, upgrade and down/up probes pass. Reverting the predicate makes the fresh-PG test fail at MigrateUp. Removing the guards makes the replay test fail.

[cursor]

Addressing Round 1 REQUEST_CHANGES (same findings as #258). There were no line-level review threads on this PR.

B1. Reverted to IfDatabase(params string[]) with MigrationDatabases.DateTimeOffsetProviders = "SqlServer", "Oracle", "MySql", "Postgres", "PostgreSQL", "PostgreSQL92". Dropped the DatabaseType-only predicate that broke AddSqlServer() / AddMySql() / AddOracleManaged(). Tests now resolve those real processors (plus AddSQLite() and AddPostgres()) and assert exactly one create branch applies via DatabaseType ∪ aliases.

B2. Narrowed the Testcontainers test to migrate + expected tables. Quoting the whole PG dialect plus putting the PK in PostgreSqlDialect.Upsert is a store-wide change (several ISqlDialect methods are not virtual on the shared base). Tracking that as its own 3.9 blocker so this port stays a migrate fix.

Release note: DELETE FROM "VersionInfo" WHERE "Version" = 10001; then migrate heals 3.6–3.8 PG DBs stuck at 10002. Added to the PR body.

Cherry-pick of #258 @ f3631ec.

[sfmskywalker]

Code Review, Round 2/4: REQUEST_CHANGES + HIGH @ 7198d69

This is the main port of #258. The head moved twice during this review (f358cd3 → b7702d5 → 7198d69). At 7198d69 the diff is identical to #258's at 39ae028, apart from the Directory.Packages.props index line and hunk offset. The full analysis is in the #258 Round 2 review.

R1 B1 and B2 are resolved. CI is green, and the fresh-PG Testcontainers test ran and passed.

Blocker

B3: Core PG store paths still fail on a fresh, fully migrated PG DB. Same as #258.

• I ran 90 store and runtime operations against PG17 at this head, and 35 fail.
• 9 of them also fail on SQLite (pre-existing), which leaves 26 PG-only 42703 failures. That is the #258 list minus BeforeLastUpdated, which main's store does not map.
• Among them:
  • IWorkflowDefinitionPublisher.PublishAsync.
  • FindWorkflowGraphAsync(id, Published).
  • Every VersionOptions flag query.
  • The Studio definition and instance lists (paged, ordered, search).
  • Journal reads.
  • BookmarkQueueStore.PageAsync.
  • TryMarkInterruptedAsync.
• Causes: the builder inlines unquoted identifiers in both OrderBy overloads, Is(VersionOptions), both search-term helpers, StartsWith and the paged Delete, and TryMarkInterruptedAsync appends raw SQL. VersionOptions also compares a boolean column with 1/0 (42883).
• Fix: the same dialect hook described in the #258 Round 2 review (QuoteIdentifier plus BooleanLiteral, identity/1/0 by default, overridden by PostgreSqlDialect). Main has no LessThan, so it has one call site fewer.
  • I prototyped it on this head: +24/−12 across 4 files.
  • All 26 PG-only failures pass, and the remaining failures are the same set as on SQLite.
  • SQLite/SQL Server SQL is unchanged, and Dapper UnitTests pass 43/43 with the fresh-PG test on local PG17.
  • Add the same golden-SQL and extended fresh-PG assertions.
• Fallback, if shared files stay off-limits:
  • Change "Fixes #255" to "Refs #255".
  • Document the Dapper + PostgreSQL store as not functional in 3.9.
  • Track the store half in 3.10.

Conditions

• C1: clean. The changed files are the same as #258's. No shared or other-provider code changed. The package changes are test-only, and shipped dependencies are unchanged.
• C2: met at this head.
  • CI ubuntu-latest passed. Dapper UnitTests: 43/43 passed, 0 skipped. Overall: 442 passed, 2 skipped elsewhere. CodeQL passed.
  • f358cd3 failed with the SQLite-handler Error parsing column 11 (StartedAt … DateTime).
  • b7702d5 failed with 42601 from the unterminated upsert.
  • On local PG17, the test passes both on its own and with the full assembly.
• C3: met. The processor test uses real processors, and the migrations are identical to #258's, where the spoof probe (SqlServer2016 + alias SqlServer on PG17) gives 15 versions and 13 tables on both base and head.
• C4: same position and release-note line as the #258 Round 2 review. Hand-made lowercase schemas never worked with the Dapper PG store: the upsert has omitted the PK since 3.0. The PR body's rename path needs the VersionInfo/duplicate-table caveat, or should be replaced with "recreate via migrations and re-import".

Non-blocking

Same as the #258 Round 2 review:

• Fold a QuoteIdentifier hook into SqlDialectBase instead of re-implementing 14 ISqlDialect members (now, if B3 uses the hook; otherwise 3.10).
• The process-global DateTimeOffset handler is order-dependent in mixed-provider processes.
• The processor test checks the constant, not the migrations' call sites. Add a Docker-free spoofed SQL Server migration test.
• Open R1 items: drift warning, guards for 20002/30001, failing fast on an unknown provider.
• Pre-existing on every provider: KV table name KeyValues vs KeyValuePairs; BookmarkQueueItems.SerializedOptions has no column; the CountDistinctAsync double select; paged delete parameters.

Verification

• Tests: Dapper UnitTests 42/43 locally (no Docker), 43/43 with local PG17. Elsa.Dapper.UnitTests 11/11.
• Probes: migrations are identical to #258's, so the PG17 and SQLite fresh, upgrade and down/up probes and the revert checks carry over.

[cursor]

Addressing Round 2 REQUEST_CHANGES B3 (same findings as #258). There are no line-level review threads.

B3. Landed in f74a282.

• ISqlDialect.QuoteIdentifier (identity default) + BooleanLiteral (1/0 default). Only PostgreSqlDialect overrides ("Name", true/false).
• Shared builder inlines (OrderBy, VersionOptions, both search helpers, StartsWith, paged delete) go through the hooks. TryMarkInterruptedAsync quotes Status. Main has no LessThan.
• Non-PG SQL is byte-identical (NonPgQuerySqlSnapshotTests on SqliteDialect / SqlServerDialect).
• Fresh-PG test covers publish, FindWorkflowGraphAsync(Published), VersionOptions, Studio lists, journal/activity-execution reads, bookmark-queue paging, paged delete, and TryMarkInterruptedAsync.

Non-test size for this increment: 4 files, +24 / −5. No non-PG behaviour change. 3.9 counterpart: #258 @ eec9581.

[sfmskywalker]

Code Review, Round 3/4: REQUEST_CHANGES + HIGH @ 1a5c38c

Addendum, latest push d01de904 (arrived while this review was being finalised): B4 is addressed, but via query.Dialect.QuoteIdentifier("Id") == "Id" ? "ID" : "Id". That produces correct SQL, but it sniffs the dialect's behaviour to keep one historical token, and a reader has to reverse-engineer why. Please replace it with the plain query.QuoteIdent("Id") as described under B4. The one-token snapshot change (ID to Id) is harmless on SQLite and on default-collation SQL Server, and it fixes case-sensitive SQL Server collations; I'm asking the release lead to sign off on it explicitly. The new PG shape assertions are good; keep them. B5 (a) and (b) are not in that push, so the fresh-PG test is still expected to fail. The next round is the last one in the review loop (4/4), so please address B4 (plain form) and B5 together.

The head moved during this review, from f74a282 to 1a5c38c. As on #258, the new commit only touches the test: it populates the activity registry and registers WriteLine before publishing. Non-test code is byte-identical at both SHAs.

The non-test diff matches #258's exactly, minus the LessThan call site, which main doesn't have. So the findings, fixes and reasoning are the same as in the #258 Round 3 review. Apply the same changes here. What follows is specific to this branch.

Blockers (details in the #258 Round 3 review)

• B4: Instance search emits "ID" on PG, which fails with 42703.
  • AndWorkflowInstanceSearchTerm quotes the literal "ID", but the column is "Id". Studio instance search and FindManyAsync(SearchTerm) fail on PG. This is the current CI failure (DapperPostgreSqlMigrationTests.cs:242).
  • Fix: query.QuoteIdent("Id"), update the instance-search snapshot (one token, ID → Id), flag the token in the PR body for the release lead, and add the PG shape assertion.
  • With that change, PG17 failures equal SQLite's (80/90 on both).
• B5: The fresh-PG test cannot pass as written, and ubuntu-latest is red. The test ran in CI; it was not skipped.
  • f74a282: NRE at publish. 1a5c38c: 42703 "ID". Two more failures sit behind B4:
    • (a) The rec-pg-1 activity record, at line 104, has Status = "Finished". ActivityStatus has no Finished, so the ordered FindManyAsync at line 277 throws ArgumentException. Use "Completed".
    • (b) The paged delete at line 346 hits the pre-existing dropped-inner-parameters bug: 42703 column "definitionid" on PG, "Must add values…" on SQLite. Use the builder-plus-AddDynamicParams snippet from the #258 Round 3 review. I checked it on PG17 and SQLite.
  • With B4 + (a) + (b) applied locally, the test passes on PG17.

Revert-proofing

Same results as on #258, with a full rebuild each time:

• Reverting the PG QuoteIdentifier override fails the test with 42703 column "name".
• Reverting the PG BooleanLiteral override fails it with 42883 boolean = integer.
• A quoting SqlDialectBase.QuoteIdentifier default fails the SQLite/SQL Server snapshot tests.
• Flipping only the ISqlDialect default does not fail anything (N2).

Release-lead bar

1. Hooks only: met.
2. Byte-identical, captured from base: met at this head.
   • All 17 snapshot queries, run on main with SqliteDialect/SqlServerDialect, produce output identical to the snapshots and to this head.
   • Coverage: OrderBy ×2, every VersionOptions flag, both search helpers, StartsWith, paged delete, booleans. There are no MySQL/Oracle dialect classes.
   • B4 changes one token.
3. Fresh-PG coverage: not met. The coverage list is complete, but the test is red (B5). The same weak assertions apply (N4).
4. Release notes: met. The rename-path caveat and the DELETE FROM "VersionInfo" WHERE "Version" = 10001; line are both present.
5. R2 holds: met.
   • Migrations, packages, the PG provider and the handler are byte-identical to R2's head (7198d69). The alias list is intact, and processor-name/migration tests pass.
   • Running the same 90 operations on SQLite gives identical results on main and on this head.

Non-blocking

• N1 to N5 as in the #258 Round 3 review:
  • N1: fold PostgreSqlDialect's 14 re-implemented members into SqlDialectBase templates that call QuoteIdentifier. This removes the trap where a base- or concrete-typed call returns unquoted SQL.
  • N2: two definitions of each default, and the ISqlDialect one is untested.
  • N3: the DateTimeOffset handler is PG-only and justified, and it cannot break SQLite/SQL Server. A later SQLite registration can break PG in a mixed-provider process.
  • N4: split the test, add a no-Docker connection override, tighten assertions.
  • N5: small cleanups and PR-body wording.
• FYI, main only, pre-existing, out of scope:
  • main's DapperWorkflowInstanceStore ignores WorkflowInstanceFilter.BeforeLastUpdated. The 3.9 LessThan filter was never forward-ported, so the filter is silently dropped on every provider; my run fails it identically on SQLite and PG.
  • RestartInterruptedWorkflowsTask relies on that filter for its cutoff. Please open a forward-port issue.

Verification

• Build and unit tests. Release build of both SHAs.
  • Elsa.Persistence.Dapper.UnitTests: 46/47; the fresh-PG test is the only failure. Elsa.Dapper.UnitTests: 11/11.
  • I ran the fresh-PG test against a local PostgreSQL 17 cluster (the box has no Docker), with a local-only patch that swaps in the connection string.
• 90-operation store/runtime run on PG17:
  • PG17: 79/90. The 11 failures are the 10 SQLite failures on this branch (the 9 known items plus BeforeLastUpdated, see FYI) plus SummarizeManyAsync(SearchTerm) (B4).
  • SQLite: 80/90 on both this head and main, with identical per-operation results.
  • With B4 applied, PG17 is 80/90 and its failure set equals SQLite's.
• CI ubuntu-latest at 1a5c38c: failure.
  • Elsa.Persistence.Dapper.UnitTests: Failed 1, Passed 46, Skipped 0, Total 47. The PG test ran: it was reported at 10.8 s, and the assembly took 10 s.
  • Overall: Failed 1, Passed 445, Skipped 2. The 2 skips are in other assemblies.
  • CodeQL/Analyze are green. f74a282 failed the same way at publish.

[cursor]

Addressing Round 3 REQUEST_CHANGES B4/B5. There are no line-level review threads. Same changes as #258.

Landed in be037bf.

B4 — instance search ID. Replaced the dialect sniff with plain query.QuoteIdent("Id"). The SQLite/SQL Server instance-search snapshot changes one token (ID → Id); SQLite and default-collation SQL Server are case-insensitive. Called out in the PR body as the only intentional non-PG SQL change. Kept the PG shape assertions.

B5a — activity status. rec-pg-1 now uses Status = "Completed".

B5b — paged delete. Did not change Store.DeleteAsync. The test builds the delete through the dialect and merges the inner parameters with AddDynamicParams.

Did not take N1 (SqlDialectBase template fold) or the optional SQLite DateTimeOffset handler change.

[cursor]

B4 wording correction (Crew Lead/CR): the ID → Id token is accepted in plain query.QuoteIdent("Id") form. The snapshot comment and release note no longer say "no behaviour change". They now say this also fixes instance search on case-sensitive SQL Server collations, where the column is Id.

#259 ad6b219 / #258 counterpart.

[sfmskywalker]

Code Review, Round 4/4: APPROVE + HIGH @ ad6b219

Delta since Round 3 (1a5c38cf to ad6b219a; commits d01de904, be037bfd, ad6b219a): identical to #258's, line for line; only context lines differ. The same findings apply as in the #258 Round 4 review.

• B4 fixed, in the plain form. query.QuoteIdent("Id"), with no dialect sniff. It is the only non-test change.
• Snapshot. The only change is the instance-search token (ID → Id) plus a comment, in the same commits as the code change. The PG shape assertions are added.
• B5 fixed. (a) Status = "Completed"; (b) the paged delete is built in the test with the inner parameters merged. Store.DeleteAsync is byte-identical to main.
• PR body. It carries:
  • the accepted ID → Id exception, noting that it also fixes case-sensitive SQL Server collations
  • the lowercase-schema rename caveat (duplicate "WorkflowDefinitions", VersionInfo rows, recreate and re-import)
  • the VersionInfo 10001 DELETE line
  • "Tracking issue: #255" instead of a closing keyword, the same split as #251/#252

Verification (Release build, local PostgreSQL 17.11)

• Fresh-PG test. Passes on PG17. Elsa.Persistence.Dapper.UnitTests: 47/47. Elsa.Dapper.UnitTests: 11/11.
• Revert-proof. Putting "ID" back fails the fresh-PG test (42703 "ID"), the PG shape assertion, and both non-PG snapshots.
• 88-operation run. PG17: 78/88. SQLite: 78/88. The per-operation results are identical.
  • The 10 shared failures are the 9 pre-existing items from the #258 Round 4 review, plus BeforeLastUpdated. main still ignores that filter on every provider: the forward-port FYI from Round 3.
• CI at ad6b219a: green. ubuntu-latest, CodeQL/Analyze, submit-nuget, GitGuardian and the CLA check all pass.
  • Elsa.Persistence.Dapper.UnitTests: Failed 0, Passed 47, Skipped 0, Total 47, in 10 s. The fresh-PG test ran rather than being skipped.
  • Overall: Passed 446, Skipped 2 (Slack and Azure Service Bus, unrelated).

Non-blocking

• The same as the #258 Round 4 review: N1 and N3 are deferred, and there is a small StartsWith-line and csproj conflict with #266's eventual main port.
• Open the BeforeLastUpdated forward-port issue for main, if it isn't filed yet.