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

[sfmskywalker]

Fixes #255

Cause

FluentMigrator 7.2 AddPostgres() registers Postgres15_0Processor. In 7.2.0 the postgres processors report:

Processor	DatabaseType	DatabaseTypeAliases
PostgresProcessor	Postgres	PostgreSQL
Postgres10_0Processor	PostgreSQL10_0	PostgreSQL10_0, PostgreSQL
Postgres11_0Processor	PostgreSQL11_0	PostgreSQL11_0, PostgreSQL
Postgres15_0Processor (AddPostgres())	PostgreSQL15_0	PostgreSQL15_0, PostgreSQL
Postgres92Processor	Postgres92	Postgres92, PostgreSQL92

IfDatabase matching in 7.2:

• IfDatabase(params string[]): exact OrdinalIgnoreCase against DatabaseType or aliases. This is the overload we use.
• IfDatabase(Predicate<string>): receives only DatabaseType. Do not use it here.

A second blocker appears after migrate: FluentMigrator force-quotes PG identifiers ("ActivityExecutionRecords"), 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 ("IsPublished" = 0 → 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.

Migrations

Shared MigrationDatabases.DateTimeOffsetProviders passed to the alias-aware params overload:

SqlServer, Oracle, MySql, Postgres, PostgreSQL, PostgreSQL92

Applied at Management/Initial.cs (10001), Runtime/Initial.cs (20001), and Runtime/V3_3.cs (20004).

Store (hooks in shared code; only PG overrides)

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(string name) — default returns the input unchanged. PostgreSqlDialect double-quotes.
• BooleanLiteral(bool value) — 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") (same spelling as definition search; no dialect sniff).

PostgreSqlDialect still quotes From / And / Count / Delete / Upsert (PK included, statement terminated with ;) and registers a PG DateTimeOffset handler.

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. That is accepted.

Compatibility position: the supported path is the FluentMigrator-created mixed-case quoted schema. Handmade lowercase unquoted schemas are unsupported. They never worked with the Dapper PG store: every upsert omitted the PK (23502) from 3.0 through 3.8.

Rename-path caveat: migrating against an un-renamed lowercase table creates an empty mixed-case duplicate ("WorkflowDefinitions" next to workflowdefinitions) because FluentMigrator's TableExists is case-sensitive. Renamed tables still need matching VersionInfo rows, or later migrations fail. Otherwise, recreate via the migrations on an empty database and re-import any data. 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. These databases cannot hold workflow data, so that one-line repair is enough.
• Instance search now uses Id instead of ID. That fixes instance search on case-sensitive SQL Server collations, where the column is Id.

Tests

• Real FluentMigrator processors from AddSqlServer(), AddSQLite(), AddPostgres(), AddMySql(), and AddOracleManaged().
• SQLite VersionInfo replay of 10001 / 20001.
• Dialect SQL-shape tests; non-PG query-builder snapshot (from/and, OrderBy, VersionOptions, search, LessThan, paged delete, count, upsert), including the ID → Id token.
• Fresh PostgreSQL (Testcontainers, CI): migrate, persist, WriteLine to completion, publish + FindWorkflowGraphAsync(Published), VersionOptions Latest / Published / LatestOrPublished, Studio lists, journal and activity-execution reads, bookmark-queue paging, BeforeLastUpdated, TryMarkInterruptedAsync, and a paged delete built in the test so inner parameters are not dropped (Store.DeleteAsync is unchanged; filed separately for 3.10).

CI (ubuntu-latest) has Docker. This agent VM does not.

Main counterpart: #259

  

[sfmskywalker]

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

The PG side of the diagnosis is right. With AddPostgres() (DatabaseType PostgreSQL15_0, aliases PostgreSQL15_0,PostgreSQL), IfDatabase("Postgres") never matches. The predicate does fix fresh PG migrations: all 15 versions and 13 tables on PG17. Two problems block merge, though. One is a new regression the predicate introduces for SQL Server. The other is CI: it is red on the PR's own fresh-PG test.

Blockers

B1: The predicate breaks SQL Server (and MySql/Oracle variants), the same bug moved to another provider.

• IfDatabase(Predicate<string>) only receives DatabaseType. IsDateTimeOffsetProvider exact-matches "SqlServer", but no FluentMigrator 7.2 SQL Server builder reports that DatabaseType:
  • AddSqlServer() and AddSqlServer2016() register SqlServer2016Processor, with DatabaseType SqlServer2016 and alias SqlServer.
  • AddSqlServer2008/2012/2014() report SqlServer20xx, also with alias SqlServer.
• The old IfDatabase("SqlServer", …) matched through the alias. The new predicate doesn't, and IfDatabase("Sqlite") doesn't match either. So on SQL Server neither branch runs: 10001, 20001 and 20004 are recorded with no WorkflowDefinitions, WorkflowInstances, Bookmarks, WorkflowExecutionLogRecords, ActivityExecutionRecords, WorkflowInboxMessages or BookmarkQueueItems tables.
• This hits fresh installs, and also SQL Server DBs upgraded from before 3.3, which would skip BookmarkQueueItems. The workbench uses AddSqlServer().
• The same happens for AddMySql()/AddMySql8() (MySql8), AddMySql5() (MySql5), AddOracleManaged() (OracleManaged), AddOracle12C() (Oracle12c) and AddOracle12CManaged() (Oracle12cManaged). All of them matched before through the MySql/Oracle aliases. Only AddOracle() (Oracle) still matches.
• Verified two ways:
  • Resolving each builder's processor and comparing old vs new matching.
  • Running the real migrations on PG through a processor that reports SQL Server's identifiers (SqlServer2016 + alias SqlServer). Base: OK, 13 tables. This PR: 42P01 relation "public.WorkflowDefinitions" does not exist at 10002, the exact #255 symptom.
• DateTimeOffsetProvider_DoesNotChangeOtherProviders asserts SqlServer2008, MySql8 and Oracle12c → false. That locks the regression in: those processors did take this branch before.
• Fix: keep the alias-aware params overload and just add the PG names. For example, a single MigrationDatabases.DateTimeOffsetProviders = ["SqlServer", "Oracle", "MySql", "Postgres", "PostgreSQL", "PostgreSQL92"] used as IfDatabase(MigrationDatabases.DateTimeOffsetProviders). Every 7.2 PG processor has Postgres, PostgreSQL or PostgreSQL92 in DatabaseType or aliases. If you keep a predicate, it must prefix-match every family (SqlServer*, MySql*, Oracle*, Postgres*). Either way, fix the theory data.
• Regression test: resolve the real processor for each builder the repo references (AddSqlServer(), AddSQLite(), AddPostgres(); MySql/Oracle optional). Assert that exactly one of the two create branches applies, using FluentMigrator's real semantics (DatabaseType ∪ aliases for params, DatabaseType only for a predicate). That test catches future provider renames in either direction.

B2: CI is red. The fresh-PG Testcontainers test fails in CI on this head.

• CI has Docker, so the test runs; it is not skipped. ubuntu-latest result: A fresh PostgreSQL DB applies every Dapper migration and can persist SerializedMetadata + AggregateFaultCount [FAIL], Npgsql.PostgresException : 42P01: relation "activityexecutionrecords" does not exist at DapperPostgreSqlMigrationTests.cs:70 (store.SaveAsync). Summary: Elsa.Persistence.Dapper.UnitTests Failed 1 / Passed 40, overall Test Failed // Failed: 1, Passed: 465, Skipped: 2.
• Reproduced identically against a local PostgreSQL 17. The migrations succeed and every table exists. The failure is in the store:
  • FluentMigrator's PG quoter (default ForceQuote = true) creates "ActivityExecutionRecords".
  • The Dapper PG dialect emits unquoted identifiers, which PG folds to activityexecutionrecords.
• There is a second problem behind it. With Force Quote=false, the next failure is 23502: null value in column "id", because PostgreSqlDialect.Upsert leaves the primary key out of the insert column list (fields excludes the PK). SqlDialectBase/SqliteDialect don't have this problem.
• Fix, preferred: quote identifiers in the PG dialect/query builder, and include primaryKeyField in PostgreSqlDialect.Upsert's insert list. This keeps FluentMigrator's defaults, and it keeps the Schema.Table(...).Exists() guards working. PostgresProcessor.TableExists compares table_name case-sensitively, so with Force Quote=false the guards would silently miss the lowercased tables.
• Alternative: narrow this PR's test to migration-only assertions and track the store fixes as a separate 3.9 blocker. Then say explicitly that Dapper + PostgreSQL still does not work end-to-end after this PR.

Non-blocking

• Already-broken PG DBs aren't healed.
  • Released 3.6–3.8 (FluentMigrator 7.2 since 3.6.0) fresh PG installs stop with 10001 recorded and only VersionInfo present. 10001 never re-runs, so this PR still fails on them at 10002 (verified).
  • Deleting the row, DELETE FROM "VersionInfo" WHERE "Version" = 10001;, then migrating up completes: all 15 versions and 13 tables (verified).
  • These DBs can't hold data, so a release note with that line is enough. An automated repair is optional and would need its own test.
• The Exists guards are fine for normal upgrades.
  • Recorded migrations never re-run, and supported providers (SQLite, SQL Server, PG) apply each migration transactionally, so a partially applied 10001 isn't a realistic state.
  • Indexes are inline .Indexed() within the guarded Create.Table, and there are no separate Create.Index/FK calls, so indexes stay consistent with their table.
  • Caveat: a table-exists check passes even when the columns differ, so schema drift is silently accepted. Consider at least a warning log.
  • The guards cover only the IfDatabase tables. Runtime/V3_1 (20002) and Identity/Initial (30001) still create tables unguarded, so "DBs without VersionInfo rows" is only partly handled. Either guard those too or narrow the rationale.
• Fail fast on an unknown provider. Unmatched providers silently create nothing and still record the version, which is what turned both #255 and B1 into silent failures. For example, IfDatabase(t => !IsDateTimeOffset(t) && !IsSqlite(t)).Delegate(() => throw new NotSupportedException(...)), with the Sqlite name in the same helper so there is one source of truth.
• Naming and docs. IsDateTimeOffsetProvider describes the column type rather than the provider set; DateTimeOffsetProviders or UsesDateTimeOffsetSchema would read better. The XML doc lists PostgreSQL and PostgreSQL92 as DatabaseType values, but they are aliases only.
• FYI, out of scope: a full MigrateDown(0) on PG fails in Runtime/V3_3.Down (Rename.Column("Id")…To("Key"), 42701). This also happens on the base branch.

Verification

• Tests:
  • Elsa.Persistence.Dapper.UnitTests: 40/41 locally. The only failure is the Testcontainers test, because the box has no Docker.
  • The same test pointed at local PG17 fails as in CI (42P01 activityexecutionrecords).
  • Elsa.Dapper.UnitTests: 16/16.
• Probes:
  • SQLite: fresh migrate OK; base→20006→PR OK; base full→PR OK; down 20006/20005 and back up OK. All reach 15 versions and 13 tables.
  • PG17 with AddPostgres(): fresh OK; old DB at 20006 → PR OK; down 20006/20005 and back up OK.
• Revert checks:
  • Restoring IfDatabase("SqlServer","Oracle","MySql","Postgres") makes the fresh-PG test fail earlier, at MigrateUp (42P01 WorkflowDefinitions). That is correct revert-proofing, once B2 is resolved.
  • Removing the Exists guards makes Replaying 10001/20001… fail (table "WorkflowDefinitions" already exists).
• Scope: all IfDatabase call sites are in the three changed files (10001, 20001, 20004). No other migration, including Identity 30001+, uses IfDatabase. The SQLite branch (IfDatabase("Sqlite") vs DatabaseType SQLite) is unchanged and correct.

[cursor]

Addressing Round 1 REQUEST_CHANGES. There were no line-level review threads on this PR, so this is the reply for both blockers.

B1 — SQL Server / MySql / Oracle regression. The predicate overload only sees DatabaseType, so AddSqlServer() (SqlServer2016), AddMySql() (MySql8) and AddOracleManaged() (OracleManaged) no longer took the DateTimeOffset branch. Reverted to the alias-aware IfDatabase(params string[]) overload with a shared MigrationDatabases.DateTimeOffsetProviders list: "SqlServer", "Oracle", "MySql", "Postgres", "PostgreSQL", "PostgreSQL92". Dropped IsDateTimeOffsetProvider and the theory data that asserted SqlServer2008 / MySql8 / Oracle12c should not match.

New tests resolve the real processors from AddSqlServer(), AddSQLite(), AddPostgres(), AddMySql(), and AddOracleManaged() (no database needed) and assert that exactly one create branch applies, using DatabaseType ∪ aliases. A separate test shows SqlServer2016 matches only through the SqlServer alias.

B2 — CI red on persist. The Testcontainers test now asserts migrate + expected tables/columns only. I did not quote identifiers in the PG dialect in this PR. Doing it properly means quoting every table/column the store emits (From, And, Insert, Update, Delete, Count, Upsert, …). Several of those ISqlDialect methods are not virtual on SqlDialectBase, so a complete fix either changes the shared base (all providers) or reimplements the whole dialect. That is more than a focused migration fix. PostgreSqlDialect.Upsert also omits the PK from the insert list (23502 after unquoting). Filing that as its own 3.9 store blocker.

Release note (verified by review): 3.6–3.8 PG DBs stuck at 10002 heal with DELETE FROM "VersionInfo" WHERE "Version" = 10001; then migrate up. Added to the PR body.

[sfmskywalker]

Code Review, Round 2/4: REQUEST_CHANGES + HIGH @ 39ae028

The head moved twice during this review (23fcce2 → 9b14229 → 39ae028). I covered all three; the findings below are against 39ae028.

Both R1 blockers are resolved:

• B1: the alias-aware list is back. I verified it with real processors and with a spoofed-processor migration run.
• B2: CI is green. The fresh-PG Testcontainers test ran and passed: Dapper UnitTests 43/43, 0 skipped. I reproduced it on local PG17.

What still blocks: outside the narrow path the test exercises, most read paths of the Dapper PG store fail, including publish and run-by-definition. Dapper + PostgreSQL is still not usable, and "Fixes #255" overstates what lands.

Blocker

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

I ran 90 store and runtime operations through the real Dapper stores against PG17 at this head, and 36 fail. 9 of them also fail on SQLite, so they are pre-existing (see FYI). That leaves 27 PG-only failures, all 42703 column "<lowercased>" does not exist:

• Management:
  • IWorkflowDefinitionPublisher.PublishAsync.
  • IWorkflowDefinitionService.FindWorkflowGraphAsync(id, VersionOptions.Published), which is the run-by-definition lookup.
  • FindAsync with any VersionOptions flag (Latest, Published, LatestOrPublished, Draft).
  • TryUpdateLatestAsync and FindLastVersionAsync.
  • FindSummariesAsync/FindManyAsync when paged (the Studio definitions list, default order CreatedAt), when ordered, and with SearchTerm.
• Instances:
  • Paged FindManyAsync.
  • SummarizeManyAsync (the Studio instance list), with and without order or search.
  • The BeforeLastUpdated filter.
  • TryMarkInterruptedAsync.
• Runtime:
  • BookmarkQueueStore.PageAsync, used by the queue processor.
  • Paged BookmarkStore and TriggerStore finds.
  • Ordered ActivityExecutionStore.FindManyAsync.
  • Journal reads: WorkflowExecutionLogStore.FindAsync(order) and FindManyAsync(page).

These pass:

• The migrations.
• Upsert, insert, update and delete by key, and FindAsync by Id.
• Unordered filters.
• The identity stores.
• WriteLine and Event-bookmark workflows run via IWorkflowHost.

Causes: none are in the dialect.

• ParameterizedQueryBuilderExtensions inlines the field name in OrderBy(string) and OrderBy(OrderField[]) (neither calls ISqlDialect.OrderBy). It does the same in Is(VersionOptions) (and IsLatest = 1), both search-term helpers, LessThan, StartsWith, and the paged Delete(table, pk, inner).
• DapperWorkflowInstanceStore.TryMarkInterruptedAsync appends the raw and not Status = @FinishedStatus.
• For VersionOptions, quoting alone is not enough: "IsPublished" = 0 fails on PG with 42883 operator does not exist: boolean = integer.

Fix (preferred; only PG behavior changes): add a dialect hook and route those call sites through it.

• ISqlDialect: add string QuoteIdentifier(string identifier) => identifier; and string BooleanLiteral(bool value) => value ? "1" : "0"; as default interface methods. For zero public-API change, use an internal interface that only PostgreSqlDialect implements instead. PostgreSqlDialect returns Quote(...) and true/false.
• Builder: call query.Dialect.QuoteIdentifier(...) in both OrderBy overloads, LessThan, StartsWith, both search-term helpers and the paged Delete. Is(VersionOptions) uses the quoted names plus BooleanLiteral.
• Store: in TryMarkInterruptedAsync, use $"and not {q.Dialect.QuoteIdentifier("Status")} = @FinishedStatus".

I prototyped exactly this on this head: +25/−13 across 4 files.

• All 27 PG-only failures pass. Publish, run-by-definition, both Studio lists, the journal and the bookmark queue all work, and the remaining PG failures are the same set as on SQLite.
• The identity defaults leave SQLite and SQL Server SQL byte-identical. SQLite probe results are unchanged.
• Dapper UnitTests pass 43/43, including the fresh-PG test on local PG17.

Tests to add with the fix:

• A golden-SQL test showing that SQLite/SQL Server builder output is unchanged.
• In the fresh-PG test: publish followed by FindWorkflowGraphAsync(Published), a paged FindSummariesAsync(VersionOptions.Latest), SummarizeManyAsync with an order, and a journal FindManyAsync(page).

Fallback, if shared files stay off-limits for 3.9:

• Merge as a migrations plus partial-store fix, and change "Fixes #255" to "Refs #255".
• Say in the PR and the release notes that the Dapper + PostgreSQL store is not functional in 3.9: publish, lists and version lookups fail.
• Open a 3.10 issue with the list above.

Conditions

• C1: clean. None of these changed: SqlDialectBase, ISqlDialect, SqliteDialect, SqlServerDialect, ParameterizedQueryBuilderExtensions, Store, the module stores, the SQLite/SQL Server providers, or TypeHandlers/Sqlite.

  • Outside PostgreSqlDialect.cs, the migrations and the tests, two things changed. PostgreSqlDbConnectionProvider gained a static ctor that registers a DateTimeOffset handler, and there is a new TypeHandlers/PostgreSql/DateTimeOffsetHandler.cs. Both are PG-only code, but the handler registration is process-global (see non-blocking).
  • Packages: Directory.Packages.props only adds PackageVersion rows for FluentMigrator.Runner.MySql/Oracle/Postgres. Only the test csproj references them, and it has IsPackable=false.
  • The Migrations csproj only adds InternalsVisibleTo. Shipped dependencies are unchanged.
  • A -w diff of the migrations shows only the IfDatabase argument and the Exists() wrappers. No columns changed.

• C2: met at this head.

  • CI ubuntu-latest passed. Dapper UnitTests: 43/43 passed, 0 skipped. Overall: 469 passed, 2 skipped in other assemblies.
  • 23fcce2 failed with Error parsing column 11 (StartedAt … DateTime): the SQLite handler leaked into the PG test. 9b14229 fixed that.
  • 9b14229 then failed with 42601 syntax error at or near "insert": a multi-row SaveManyAsync hit the unterminated upsert. 39ae028 fixed that.
  • On local PG17 the test passes both on its own and with the full assembly.

• C3: met. DapperPostgresProcessorNameTests resolves the real processors for AddSqlServer/AddSQLite/AddPostgres/AddMySql/AddOracleManaged through DI; none of the names are hand-typed. For the spoof probe I ran the real migrations on PG17 through a processor reporting SqlServer2016 with alias SqlServer. Base and this head both reach 15 versions and 13 tables.

• C4: I agree with the unsupported position. A working hand-made lowercase DB realistically does not exist:

  • The PG upsert has left the PK out since the dialect was added to elsa-core in Sep 2023 (cd2591430/72d855090, 3.0), and the builder has always left the PK out of fields. So every SaveAsync on PG failed with 23502 in 3.0–3.8, lowercase or not.
  • FluentMigrator's PG quoter force-quotes, so migration-created schemas were always mixed-case, and the unquoted store got 42P01.
  • A working lowercase setup would have needed both a hand-made schema and a custom dialect.

  The rename path in the PR body is incomplete, though:

  • FluentMigrator's TableExists is case-sensitive. Running the migrations against lowercase tables that were not renamed creates empty mixed-case duplicates next to them.
  • Renamed tables without matching VersionInfo rows fail at 10002+ (Alter.Table…AddColumn, the column already exists) or at 20002/30001 (unguarded creates).

  Suggested release-note line: "Dapper + PostgreSQL now uses the FluentMigrator-created, quoted mixed-case schema. Hand-made lowercase/unquoted schemas are not supported. They never worked with the Dapper PG store, because every upsert failed. Recreate the schema by running the migrations on an empty database and re-import any data. For installs stuck after 10001 on 3.6–3.8, run DELETE FROM "VersionInfo" WHERE "Version" = 10001; and restart."

Non-blocking

• Maintainability. PostgreSqlDialect re-implements 14 ISqlDialect members and duplicates the SqlDialectBase templates.
  • Delete/Count/IsNull/IsNotNull/Insert/Update are non-virtual in the base. A call through a SqlDialectBase- or PostgreSqlDialect-typed reference gets unquoted SQL; a call through ISqlDialect gets quoted SQL. That works today because every caller uses ISqlDialect, but it is a trap.
  • A protected virtual string QuoteIdentifier(string) (identity by default), used by the base templates, would shrink the PG dialect to QuoteIdentifier plus Upsert, with byte-identical SQLite/SQL Server output.
  • If B3 takes the hook route, fold this in now, since it is the same hook. Otherwise do it in 3.10.
• DateTimeOffset handler. Dapper type handlers are process-global, and the last one registered wins. If a process initializes SqliteDbConnectionProvider after PostgreSqlDbConnectionProvider, PG reads break again, which is the 23fcce2 CI failure. Single-provider apps are fine. For the tests, put the PG test in its own non-parallel collection, or share one tolerant handler between providers (3.10).
• Revert-proofing.
  • Each of these reverts fails the fresh-PG test: restoring the base dialect (42P01 activityexecutionrecords), dropping the upsert ; (42601), dropping the PK from the upsert (23502).
  • Dropping PostgreSQL/PostgreSQL92 from the list fails 4 processor tests plus the fresh-PG test.
  • Gap: the processor test checks the constant, not the migrations. If a call site inlines a different list, nothing catches it for SQL Server/MySQL; only PG is caught, by the container test. A Docker-free test would close this: run the real migrations through a processor reporting AddSqlServer()'s DatabaseType and aliases, and assert that all tables exist.
• R1 items.
  • Done: release note (in the PR body); naming and XML doc.
  • Not done, all optional: a drift warning; guards for 20002/30001 (or narrow the rationale); failing fast on an unknown provider.
• FYI, pre-existing on every provider (same on SQLite; worth separate issues):
  • DapperKeyValueStore uses the table KeyValues, but the migrations create KeyValuePairs, so the whole KV store fails.
  • BookmarkQueueItemRecord.SerializedOptions has no column, so BookmarkQueueStore.SaveAsync/AddAsync fail.
  • CountDistinctAsync emits two selects.
  • The paged Store.DeleteAsync does not copy the inner query's parameters.

Verification

• Tests: Dapper UnitTests 42/43 locally; the only failure is that there is no Docker. 43/43 with the fresh-PG test pointed at local PG17. Elsa.Dapper.UnitTests 16/16.
• PG17 migrations:
  • Fresh: 15 versions and 13 tables.
  • Down to 20005 and back up: OK.
  • A stuck 3.6–3.8 DB (base fresh stops at 10001) is not healed by this head. The DELETE plus migrate heals it.
• SQLite: fresh; base@20006 → head; base full → head; down to 20005 → up. All OK, each ending at 15 versions and 13 tables.

[cursor]

Addressing Round 2 REQUEST_CHANGES B3. There are no line-level review threads.

B3 — unquoted identifiers and integer booleans on shared builder paths. Landed in eec9581 (BooleanLiteral + QuoteIdentifier routing) on top of the earlier QuoteIdentifier hook.

• ISqlDialect.QuoteIdentifier defaults to identity; BooleanLiteral defaults to 1/0. Only PostgreSqlDialect overrides them ("Name" and true/false).
• ParameterizedQueryBuilderExtensions routes every inlined identifier through the quote hook, including 3.9 LessThan (BeforeLastUpdated). Is(VersionOptions) uses BooleanLiteral. TryMarkInterruptedAsync quotes Status.
• Non-PG SQL is byte-identical: NonPgQuerySqlSnapshotTests asserts the pre-hook SQLite and SQL Server strings, including = 1 / = 0.
• Fresh-PG Testcontainers test now covers publish, FindWorkflowGraphAsync(Published), VersionOptions Latest/Published/LatestOrPublished, Studio definition and instance lists, journal and activity-execution reads, bookmark-queue paging, BeforeLastUpdated, TryMarkInterruptedAsync, and paged delete.

Non-test size for this increment: 4 files, +25 / −6. No non-PG behaviour change.

[sfmskywalker]

Code Review, Round 3/4: REQUEST_CHANGES + HIGH @ 8526e90

Addendum, latest push 3a58d186 (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 eec9581 to 8526e90. The new commit only touches the test: it populates the activity registry and registers WriteLine before publishing. The non-test code is byte-identical at both SHAs, so every runtime check below applies to 8526e90. CI results are for 8526e90.

R2's B3 is mostly fixed, and the hook design is what we asked for:

• QuoteIdentifier/BooleanLiteral default to the identity and 1/0. Only PostgreSqlDialect overrides them.
• Every inlined identifier in the builder goes through the hook, and so does TryMarkInterruptedAsync.
• SQLite and SQL Server SQL is byte-identical to release/3.9.0.
• 26 of R2's 27 PG-only failures now pass.

Two things block merge. The fix emits one wrong identifier, which breaks PG instance search. And the fresh-PG test is red in CI: it cannot pass as written.

Blockers

B4: Instance search emits "ID" on PostgreSQL, which fails with 42703.

• AndWorkflowInstanceSearchTerm passes the literal "ID" through QuoteIdentifier. Quoted PG identifiers are case-sensitive and the column is "Id", so any WorkflowInstanceFilter.SearchTerm fails with 42703: column "ID" does not exist. That covers Studio instance-list search (SummarizeManyAsync) and FindManyAsync.
• This is the current CI failure: DapperPostgreSqlMigrationTests.cs:242.
• The 90-operation run on PG17 at this head fails only this operation beyond the SQLite baseline (details below).
• Fix: use query.QuoteIdent("Id"), the same spelling the definition-search helper already uses.
  • Side effect: the SQLite/SQL Server instance-search snapshot changes one token (ID → Id). On SQLite, MySQL and Oracle, and on SQL Server with its default case-insensitive collation, the two are equivalent.
  • On a SQL Server database with a case-sensitive collation, ID does not resolve to the migration-created Id column today, so this is a latent fix, not a regression.
  • Update the snapshot with a one-line comment. Call out the token in the PR body, because it is the only exception to "byte-identical" and the release lead should sign off on it explicitly.
• Add a PG shape assertion to QueryBuilder_QuotesInlinedIdentifiers: Assert.Contains("\"Id\" like @SearchTerm", queries["instance-search"]) and Assert.DoesNotContain("\"ID\"", queries["instance-search"]).
• Verified: with only this change, PG17 failures equal the SQLite baseline exactly (81/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. It failed on eec9581 with an NRE at PublishAsync: the activity registry was never populated, which 8526e90 fixes. On 8526e90 it fails on B4. I ran the whole chain against a local PG17, and two more failures sit behind B4:

• (a) Wrong status value. The rec-pg-1 activity record, at line 104, has Status = "Finished". ActivityStatus has no Finished, so the ordered IActivityExecutionStore.FindManyAsync (orderedActivities) throws ArgumentException: Requested value 'Finished' was not found. Use "Completed".
• (b) Paged delete (line 351). Store.DeleteAsync(filter, pageArgs, orderFields) drops the inner query's parameters. This is the pre-existing all-provider bug being filed separately; SQLite fails the same way with "Must add values for the following parameters".
  • On PG, the unbound @DefinitionId parses as a prefix operator on an unquoted identifier: 42703: column "definitionid" does not exist. So this assertion cannot pass without fixing that bug.
  • To keep coverage of the PG-specific part (quoted "Id" in (…) around a quoted, ordered, paged subquery) without changing other providers, build the statement in the test and merge the inner parameters:

    var provider = new PostgreSqlDbConnectionProvider(_connectionString);
    var inner = provider.CreateQuery()
        .From("WorkflowInstances", "Id")
        .Is(nameof(WorkflowInstanceRecord.DefinitionId), "def-pg-page")
        .OrderBy(new OrderField(nameof(WorkflowInstanceRecord.CreatedAt), OrderDirection.Descending))
        .Page(PageArgs.FromRange(0, 1));
    var delete = provider.CreateQuery().Delete("WorkflowInstances", "Id", inner);
    delete.Parameters.AddDynamicParams(inner.Parameters); // Store.DeleteAsync drops these today (tracked separately)
    using var connection = provider.GetConnection();
    Assert.Equal(1, await connection.ExecuteAsync(delete.Sql.ToString(), delete.Parameters));

    Keep the remaining assertion after it. I checked this pattern on PG17 and on SQLite, and it deletes exactly one row on both.
  • Fixing Store.DeleteAsync itself (deleteQuery.Parameters.AddDynamicParams(selectQuery.Parameters)) is one line and emits the same SQL. But it changes SQLite/SQL Server behavior (from throwing to working), so leave it to the separate issue unless the release lead opts in.
• Verified: with B4 + (a) + (b) applied locally, the test passes on PG17.

Revert-proofing

All runs used the test with B4, (a) and (b) applied locally, and a full rebuild each time:

• Revert PG QuoteIdentifier to return the name unchanged: the test fails with 42703: column "name" does not exist.
• Revert PG BooleanLiteral to 1/0: the test fails with 42883: operator does not exist: boolean = integer.
• Make SqlDialectBase.QuoteIdentifier quote: the SQLite and SQL Server snapshot tests fail, along with the no-op test. Making the default BooleanLiteral emit true/false also fails them.

Release-lead bar

1. Hooks only: met. The builder changes only through QuoteIdent/BoolLit, which route to ISqlDialect. Only PostgreSqlDialect overrides. The only store-level change is TryMarkInterruptedAsync, also through the hook.
2. Byte-identical, captured from base: met at this head. I ran all 18 snapshot queries on release/3.9.0 with SqliteDialect and SqlServerDialect. The output is identical to the snapshots and to this head's output.
   • Coverage: both OrderBy overloads, every VersionOptions flag, both search helpers, LessThan, StartsWith, paged delete, booleans, from/and/count/upsert.
   • The repo has only the SQLite, SQL Server and PostgreSQL dialects; there are no MySQL/Oracle dialect classes.
   • B4 will change one token here, see above.
3. Fresh-PG coverage: not met.
   • The list is complete: publish → FindWorkflowGraphAsync(Published), list/search/OrderBy, VersionOptions, paged delete, both Studio lists, BookmarkQueueStore.PageAsync, the journal, BeforeLastUpdated, TryMarkInterruptedAsync. But the test is red (B5).
   • Most assertions check returned data; the queue page asserts order, for example. Three are weak, see N4.
4. Release notes: met. The rename-path caveat (empty duplicate "WorkflowDefinitions", matching VersionInfo rows, recreate and re-import) and DELETE FROM "VersionInfo" WHERE "Version" = 10001; are both in the body.
5. R2 holds: met.
   • Migrations, the migrations csproj, packages, the PG provider and the handler are byte-identical to R2's head (39ae028).
   • The alias list is intact. Processor-name and migration tests pass 18/18.
   • Running the same 90 operations on SQLite gives identical results on release/3.9.0 and on this head.

Non-blocking

• N1: Dialect duplication and the base-typed trap (recommended in this round).
  • PostgreSqlDialect still re-implements 14 members with its own Quote(...). Meanwhile SqlDialectBase now has a virtual QuoteIdentifier that its own templates don't use. The result is two parallel quoting mechanisms.
  • Delete, Count (both overloads), IsNull, IsNotNull, Insert and Update (both overloads) are non-virtual in the base. So new PostgreSqlDialect().Delete("Bookmarks"), or any call through a SqlDialectBase/PostgreSqlDialect-typed reference, returns unquoted SQL.
  • Not a blocker: every caller in the repo goes through ISqlDialect (ParameterizedQuery.Dialect, IDbConnectionProvider.Dialect), so nothing is broken at runtime.
  • But the fix is now cheap and guarded. Route the base templates through QuoteIdentifier(...); with the identity default, SQLite and SQL Server stay byte-identical, and the snapshot test proves it. PostgreSqlDialect then shrinks to QuoteIdentifier, BooleanLiteral, Upsert (PK plus ;) and the COUNT(distinct …) expression handling.
  • Do it now since another push is needed anyway, or open a 3.10 issue.
• N2: Two definitions of each default.
  • ISqlDialect has default interface methods, and SqlDialectBase re-declares both as virtuals.
  • The snapshot tests only exercise the base virtuals. I flipped the ISqlDialect default to quoting and all three snapshot tests stayed green.
  • Either drop one definition, or pin the interface defaults with a test on a stub that implements ISqlDialect directly (Elsa.Dapper.UnitTests already has StubSqlDialect).
• N3: PG DateTimeOffset handler and static ctor: PG-only, justified, no risk to SQLite/SQL Server.
  • The handler is internal and registered only from PostgreSqlDbConnectionProvider's static ctor. That ctor runs only when a PG provider is instantiated; DapperFeature's default factory is SQLite, so SQLite- and SQL Server-only apps never touch it.
  • Dapper keeps one handler per type, and the last registration wins.
  • PG registered after SQLite: SQLite reads go through the PG handler. Its string branch is the same DateTimeOffset.Parse(s) call, and SetValue is the same parameter.Value = value. SQL Server returns DateTimeOffset, which passes straight through; the SQLite handler would have thrown there, so PG-last is strictly more tolerant.
  • SQLite registered after PG: PG reads break again, exactly R2's 23fcce2 failure. That only happens in a process that instantiates both providers, and it affects only PG.
  • Optional hardening: have SqliteDbConnectionProvider register the same tolerant handler, which is identical for strings. In the test assembly the order is timing-dependent today; it is fine in CI because Testcontainers startup makes SQLite register first.
• N4: Test quality.
  • The single [Fact] runs about 30 checks in sequence, so the NRE hid B4, (a) and (b). Split them into facts over a shared migrated-DB fixture.
  • Add an environment-variable connection-string override so the test can run without Docker. That is how I ran it locally, and it would have caught all of this before push.
  • Tighten three assertions: the Studio instance list (TotalCount >= 1; assert ids and order with two or more rows), the Studio definitions list (Contains; assert order and page size), and the journal page (one row cannot show desc order).
  • Also cover VersionOptions.Draft and the journal's FindAsync(order), and assert that TryMarkInterruptedAsync refuses a Finished row.
• N5: Small cleanups.
  • The builder's OrderBy(string, …) could call query.Dialect.OrderBy(...); the output is byte-identical and PG already overrides it.
  • A misindented /// line sits in the PostgreSqlDialect remarks.
  • PR body: "The store fix is not PostgreSQL-dialect-only" reads as if other providers change. Say instead that the hooks live in shared code, only PG overrides them, and other providers' SQL is byte-identical (plus the B4 token).
• FYI, pre-existing, out of scope: StartsWith emits @SearchTermLike but binds @{field}, so the KV StartsWith query will fail on every provider even after the KeyValues/KeyValuePairs table fix. Add it to the separate issue with the four known items.

Verification

• Build and unit tests. Release build of both SHAs.
  • Elsa.Persistence.Dapper.UnitTests: 46/47; the fresh-PG test is the only failure, as in CI. Elsa.Dapper.UnitTests: 16/16.
  • I ran the fresh-PG test against a local PostgreSQL 17 cluster (the box has no Docker), with a local-only patch that swaps the Testcontainers connection string for a fresh database. Results: eec9581 NRE at publish; 8526e90 42703 "ID".
• 90-operation store/runtime run on PG17 (R2's checklist, real Dapper stores plus IWorkflowHost/publisher end-to-end):
  • PG17: 80/90. The 10 failures are the 9 SQLite-baseline items plus SummarizeManyAsync(SearchTerm) (B4). R2 had 54/90.
  • SQLite: 81/90 on both this head and release/3.9.0, with identical per-operation results.
  • With B4 applied, PG17 is 81/90 and its failure set equals SQLite's.
• CI ubuntu-latest at 8526e90: failure.
  • Elsa.Persistence.Dapper.UnitTests: Failed 1, Passed 46, Skipped 0, Total 47 (R2's 43 plus 4 new). The PG test ran: it was reported at 9.7 s, and the assembly took 9 s.
  • Overall: Failed 1, Passed 472, Skipped 2. The 2 skips are in other assemblies, as in R2.
  • eec9581 failed the same way at publish.

[cursor]

Addressing Round 3 REQUEST_CHANGES B4/B5. There are no line-level review threads.

Landed in dc596ed.

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

B5a — activity status. rec-pg-1 now uses Status = "Completed" (ActivityStatus has no Finished).

B5b — paged delete. Did not change Store.DeleteAsync. The test now builds the delete through the dialect and merges the inner parameters with AddDynamicParams, then asserts the remaining rows.

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.

#258 a7a0bf6 / #259 counterpart.

[sfmskywalker]

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

Delta since Round 3 (8526e90e to a7a0bf60; commits 3a58d186, dc596edf, a7a0bf60): it contains exactly what Round 3 asked for, and nothing else.

• B4 fixed, in the plain form. AndWorkflowInstanceSearchTerm now uses query.QuoteIdent("Id"), with no dialect sniff left anywhere in src. This one line is the only non-test change in the delta.

• Snapshot. The only change in NonPgQuerySqlSnapshotTests is the instance-search token (ID → Id) plus a two-line comment, in the same commits as the code change. The PG shape assertions are added: "Id" like @SearchTerm is present and "ID" is absent.

• B5 fixed.

  • (a) The activity record now uses Status = "Completed".
  • (b) The paged delete is built in the test with the inner parameters merged, and the remaining assertion is kept.
  • Store.DeleteAsync is untouched, byte-identical to release/3.9.0.

• PR body and release note. Both carry:

  • the accepted ID → Id exception, noting that it also fixes instance search on case-sensitive SQL Server collations
  • the lowercase-schema rename caveat: an empty duplicate "WorkflowDefinitions", the matching VersionInfo rows, and recreate-and-re-import
  • DELETE FROM "VersionInfo" WHERE "Version" = 10001;
  • Fixes #255

  The wording change asked for in Round 3 (hooks live in shared code, only PG overrides them) is in as well.

Verification (Release build, local PostgreSQL 17.11)

• Fresh-PG test. Passes when pointed at a fresh PG17 database; the only local change swaps in the connection string, since the box has no Docker.
  • Elsa.Persistence.Dapper.UnitTests: 47/47. Elsa.Dapper.UnitTests: 16/16.
• Revert-proof. Putting "ID" back fails 4 tests:
  • the fresh-PG test, with 42703: column "ID" does not exist
  • the PG shape assertion
  • both the SQLite and SQL Server snapshots
• 88-operation store/runtime run (the Round 2 checklist, rebuilt; real Dapper stores plus IWorkflowHost/publisher end-to-end):
  • PG17: 79/88. SQLite: 79/88. The per-operation results are identical.
  • The 9 shared failures are the known pre-existing items, all out of scope: CountDistinctAsync, Store.DeleteAsync paged, 3 bookmark-queue ops (#264), and 4 key-value ops (#263).
  • With the Id fix reverted, PG drops to 78/88. The only extra failure is SummarizeManyAsync(SearchTerm) with 42703, so the run detects exactly this regression.
• CI at a7a0bf60: green. ubuntu-latest, 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 against Testcontainers rather than being skipped: it is a plain [Fact] that fails without Docker, and the assembly takes about 1 s locally without the container.
  • Overall: Passed 473, Skipped 2. The skips are the Slack and Azure Service Bus tests, the same as Round 3.

Non-blocking

• Round 3's N1 (fold PostgreSqlDialect's 14 re-implemented members into SqlDialectBase templates via QuoteIdentifier) and N3 (register the tolerant DateTimeOffset handler from SQLite too) weren't taken up. That's fine; they're worth a 3.10 issue.
• Merge interplay with #266:
  • #266's R1 fix will touch the same line in ParameterizedQueryBuilderExtensions.StartsWith (the key-value StartsWith binding). This PR changes that line to {query.QuoteIdent(field)} like @SearchTermLike but leaves the binding as is. Whichever lands second has a small textual conflict; keep the quoting and the new parameter name together.
  • Both PRs also add package references to the Dapper test csproj, a trivial conflict; keep both.
• Fixes #255 doesn't auto-close from a non-default base branch, so close #255 manually once #258 and #259 are both merged.