Skip to content

Commit 6c675b0

Browse files
iancooperclaude
andcommitted
fix: PR #4039 F2-1 follow-up — PG lowercase-then-quote across DDL/DML/detection
Closes the F2-1 reviewer comment "the SQLite / MySQL / PG V1 baselines deserve the same scan" properly for PG, which F2 had deferred as a chain-wide limitation. PG case-folds unquoted identifiers to lowercase at parse time, so reserved-keyword table names (Order, User, Group, …) failed under the legacy unquoted DDL chain. This commit makes the PG path quote-safe end-to-end while preserving backward compatibility with existing folded tables. Approach: lowercase the configured identifier first (matching PG's natural fold of the legacy unquoted form), then double-quote at every emission site. A new `PgIdentifier` helper centralizes this in the shared PG assembly. DDL emitters (Wave A) - PostgreSqlOutboxBuilder.GetDDL/GetExistsQuery and PostgreSqlInboxBuilder GetDDL/GetExistsQuery now emit `"schema"."table"` via PgIdentifier. - PostgreSqlOutboxMigrationCatalog V1 historical DDL + V2..V7 ALTERs and PostgreSqlInboxMigrationCatalog V1 historical DDL apply the same convention. Detection / validator / lock / history (Wave B) - PostgreSqlBoxDetectionHelper user-table lookups (information_schema.tables, information_schema.columns, history-row params) PgIdentifier.Normalize the configured values so mixed-case defaults (e.g. "Outbox") match PG's stored folded form. This also closes a latent bug: pre-change detection passed the mixed-case configured name verbatim and would have missed `outbox` for any user on the framework default — only undetectable because tests use lowercase / GUID names exclusively. - PostgreSqlBoxDetectionHelper.DoesHistoryExistAsync inlines the literal `__BrighterMigrationHistory` existence check so it bypasses the fold path — the system-table CREATE quotes the name and pg_class case-preserves it. - PostgreSqlPayloadModeValidator normalizes its parameter values. - PostgreSqlBoxMigrationRunner stores history rows under lowercase BoxTableName/SchemaName and hashes the advisory-lock key off the PG-normalized lock resource string. Runtime DML (Wave C) - RelationalDatabaseInbox.GenerateSqlText lifted from private to protected virtual to match the existing Outbox shape. - PostgreSqlOutbox + PostgreSqlInbox override GenerateSqlText to lowercase-quote the configured table name before string.Format substitutes into the SQL templates — closes the runtime path for reserved-keyword names. Tests + docs (Wave D) - New end-to-end test pins the contract by configuring `OutBoxTableName = "Order"` (PG reserved keyword): asserts fresh install succeeds against the folded `order` table, history is keyed under lowercase, chain replay is a no-op, and PostgreSqlOutbox AddAsync/GetAsync round-trips a message through the reserved-keyword table. - Full PG suite (182 tests) passes. - docs/guides/box-provisioning-new-backend.md "Operational notes" section documents the PG fold-then-quote convention and the upgrade-safety story for existing deployments (history rows with mixed-case BoxTableName values become unreachable on read, but the ALTER chain is idempotent so the worst case is one no-op re-run). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e040c7a commit 6c675b0

13 files changed

Lines changed: 335 additions & 55 deletions

File tree

docs/guides/box-provisioning-new-backend.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,12 @@ The MySQL migration runner mutates its own connection string to set `AllowUserVa
267267

268268
`BoxProvisioningOptions.MigrationLockTimeout` is honoured at millisecond resolution on MSSQL (`sp_getapplock`) and PG (`pg_try_advisory_lock` retry loop with a monotonic deadline), but MySQL's `GET_LOCK` takes an integer-second argument and SQLite's runner falls back to the same whole-second granularity. Sub-second timeouts on MySQL/SQLite are rounded **up** to 1 second — `TimeSpan.Zero` is **not** fail-fast on these backends. Cross-backend callers expecting fail-fast semantics should size their probe/deploy budgets accordingly.
269269

270+
### PostgreSQL — identifiers are lowercased then quoted
271+
272+
PostgreSQL case-folds unquoted identifiers to lowercase at parse time. Brighter's PG layer (DDL builders, migration catalogs, runtime DML, detection helpers, advisory-lock keys, and `__BrighterMigrationHistory` parameter values) all route configured table and schema names through `PgIdentifier.Normalize` / `PgIdentifier.Quote` — i.e. lowercased, then double-quoted at the emission site. This preserves continuity with historical Brighter PG deployments (where unquoted DDL produced lowercase physical tables regardless of the configured casing) **and** allows reserved-keyword table names (`Order`, `User`, `Group`, …) which the legacy unquoted form rejects as syntax errors. The internal `__BrighterMigrationHistory` table is created via quoted DDL and therefore remains case-preserved — its existence check bypasses the fold-normalization that user-table lookups go through.
273+
274+
If you previously had a PG deployment with a mixed-case `OutBoxTableName` configured (e.g. the default `"Outbox"`), the upgrade is non-disruptive: the existing physical table is already named `outbox` in `pg_class`, and the new quoted form `"outbox"` resolves to the same table. History-table rows written before this normalization (with mixed-case `BoxTableName` values) become unreachable on read; the ALTER chain is idempotent (`ADD COLUMN IF NOT EXISTS`) so the worst case is a single re-run against an existing table — a no-op.
275+
270276
## Identifier safety
271277

272278
Brighter's `Identifiers.AssertSafe` chokepoint validates table and schema names against `^[A-Za-z][A-Za-z0-9_]*$`. This is the **strictest backend's** rule (Spanner rejects `_`-prefixed names as reserved) applied framework-wide. If your new backend allows characters outside this set inside quoted/backticked identifiers, the chokepoint will still reject them — this is deliberate defence-in-depth. Do not loosen the regex; if your backend has a legitimate use case for an unusual identifier shape, rename the configuration value or document the constraint.

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ THE SOFTWARE. */
2929
using Microsoft.Extensions.Logging.Abstractions;
3030
using Npgsql;
3131
using Paramore.Brighter.Logging;
32+
using Paramore.Brighter.PostgreSql;
3233

3334
namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
3435

@@ -80,8 +81,14 @@ public async Task<bool> DoesTableExistAsync(
8081
command.CommandText = @"
8182
SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
8283
WHERE TABLE_SCHEMA = @SchemaName AND TABLE_NAME = @TableName)";
83-
command.Parameters.AddWithValue("@SchemaName", schemaName ?? DefaultSchemaName);
84-
command.Parameters.AddWithValue("@TableName", tableName);
84+
// information_schema.tables stores PG-folded (lowercase) names. Normalize so a
85+
// mixed-case configured value (e.g. default "Outbox") matches the stored "outbox".
86+
// The internal __BrighterMigrationHistory system-table existence check does NOT
87+
// route through this method — its CREATE DDL quotes the name and therefore
88+
// case-preserves it in pg_class, so DoesHistoryExistAsync inlines a literal-name
89+
// lookup that bypasses the fold normalization done here.
90+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
91+
command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
8592

8693
// Pattern-match rather than (bool)raw! so a driver returning null surfaces as a named
8794
// InvalidOperationException instead of a bare NullReferenceException — Npgsql has
@@ -103,18 +110,32 @@ public async Task<bool> DoesHistoryExistAsync(
103110
CancellationToken cancellationToken = default,
104111
NpgsqlTransaction? transaction = null)
105112
{
106-
var historyTableExists = await DoesTableExistAsync(
107-
connection, "__BrighterMigrationHistory", DefaultSchemaName, cancellationToken, transaction);
108-
if (!historyTableExists)
109-
return false;
113+
// System-table existence: __BrighterMigrationHistory is created via quoted DDL
114+
// (`CREATE TABLE IF NOT EXISTS "public"."__BrighterMigrationHistory"`), so its name
115+
// is case-PRESERVED in pg_class. Routing through DoesTableExistAsync would lowercase
116+
// the lookup to `'__brightermigrationhistory'` and miss. Inline the literal check.
117+
using (var existsCmd = connection.CreateCommand())
118+
{
119+
if (transaction != null) existsCmd.Transaction = transaction;
120+
existsCmd.CommandText = @"
121+
SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
122+
WHERE TABLE_SCHEMA = 'public' AND TABLE_NAME = '__BrighterMigrationHistory')";
123+
var existsRaw = await existsCmd.ExecuteScalarAsync(cancellationToken);
124+
var historyTableExists = existsRaw is bool b && b;
125+
if (!historyTableExists)
126+
return false;
127+
}
110128

111129
using var command = connection.CreateCommand();
112130
if (transaction != null) command.Transaction = transaction;
113131
command.CommandText = @"
114132
SELECT COUNT(1) FROM ""public"".""__BrighterMigrationHistory""
115133
WHERE ""BoxTableName"" = @BoxTableName AND ""SchemaName"" = @SchemaName";
116-
command.Parameters.AddWithValue("@BoxTableName", tableName);
117-
command.Parameters.AddWithValue("@SchemaName", schemaName ?? DefaultSchemaName);
134+
// History rows are stored with PG-folded (lowercase) identifiers by the runner so
135+
// that lookups remain consistent across mixed-case configured values; normalize
136+
// here too. See PostgreSqlBoxMigrationRunner.InsertHistory.
137+
command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
138+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
118139

119140
try
120141
{
@@ -160,8 +181,8 @@ public async Task<int> GetMaxVersionAsync(
160181
command.CommandText = @"
161182
SELECT COALESCE(MAX(""MigrationVersion""), 0) FROM ""public"".""__BrighterMigrationHistory""
162183
WHERE ""BoxTableName"" = @BoxTableName AND ""SchemaName"" = @SchemaName";
163-
command.Parameters.AddWithValue("@BoxTableName", tableName);
164-
command.Parameters.AddWithValue("@SchemaName", schemaName ?? DefaultSchemaName);
184+
command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
185+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
165186

166187
var raw = await command.ExecuteScalarAsync(cancellationToken);
167188
return raw is int i
@@ -242,8 +263,8 @@ internal async Task<HashSet<string>> GetTableColumnsAsHashSetAsync(
242263
command.CommandText = @"
243264
SELECT column_name FROM information_schema.columns
244265
WHERE table_schema = @SchemaName AND table_name = @TableName";
245-
command.Parameters.AddWithValue("@SchemaName", schemaName ?? DefaultSchemaName);
246-
command.Parameters.AddWithValue("@TableName", tableName);
266+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
267+
command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
247268

248269
var columns = new HashSet<string>(StringComparer.Ordinal);
249270
using var reader = await command.ExecuteReaderAsync(cancellationToken);

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxMigrationRunner.cs

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ THE SOFTWARE. */
3030
using Npgsql;
3131
using Paramore.Brighter.Logging;
3232
using Paramore.Brighter.Observability;
33+
using Paramore.Brighter.PostgreSql;
3334

3435
namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
3536

@@ -105,9 +106,12 @@ protected override Task<IAmAProvisioningUnitOfWork<NpgsqlTransaction>> CreateUni
105106
// Include the schema in the lock key so two same-named tables in different schemas
106107
// (e.g. public.Outbox and billing.Outbox) acquire distinct advisory locks. Without
107108
// the schema qualifier they would share a lock and serialize unnecessarily — matches
108-
// the MSSQL runner's lockResource shape.
109+
// the MSSQL runner's lockResource shape. Lowercase the identifiers so callers that
110+
// configure the same physical table with different casings (e.g. "Outbox" vs
111+
// "outbox" — both folded to `outbox` by PG) hash to the same lock key.
109112
protected override string LockResourceFor(string? schemaName, string tableName)
110-
=> $"BrighterMigration_{schemaName ?? HISTORY_TABLE_SCHEMA}.{tableName}";
113+
=> $"BrighterMigration_{PgIdentifier.Normalize(schemaName ?? HISTORY_TABLE_SCHEMA)}.{PgIdentifier.Normalize(tableName)}";
114+
111115

112116
protected override async Task EnsureHistoryTableAsync(
113117
NpgsqlConnection connection, NpgsqlTransaction? transaction, string? schemaName,
@@ -290,8 +294,14 @@ private static async Task InsertHistoryRowAsync(
290294
INSERT INTO ""{HISTORY_TABLE_SCHEMA}"".""{MIGRATION_HISTORY_TABLE}"" (""MigrationVersion"", ""SchemaName"", ""BoxTableName"", ""Description"")
291295
VALUES (@Version, @SchemaName, @BoxTableName, @Description)";
292296
command.Parameters.AddWithValue("@Version", version);
293-
command.Parameters.AddWithValue("@SchemaName", schemaName);
294-
command.Parameters.AddWithValue("@BoxTableName", tableName);
297+
// Store PG-folded (lowercase) identifiers so history rows match the case of the
298+
// physical table PG actually created — and so lookups via the detection helper
299+
// (which normalizes the same way) hit the same row regardless of the configured
300+
// casing. Existing rows written before this normalization remain in the table but
301+
// become unreachable on read; the ALTER chain is idempotent (`ADD COLUMN IF NOT
302+
// EXISTS`) so a re-run against an existing table is a no-op.
303+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName));
304+
command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
295305
command.Parameters.AddWithValue("@Description", description);
296306

297307
await command.ExecuteNonQueryAsync(cancellationToken);

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlInboxMigrationCatalog.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ THE SOFTWARE. */
2424
using System;
2525
using System.Collections.Generic;
2626
using Paramore.Brighter.Inbox.Postgres;
27+
using Paramore.Brighter.PostgreSql;
2728

2829
namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
2930

@@ -55,12 +56,12 @@ public class PostgreSqlInboxMigrationCatalog : IAmABoxMigrationCatalog
5556
// Literal historical PostgreSQL inbox DDL extracted from commit 1cdc04b60 (Nov 2023, PR
5657
// #2560). First-shipped state already carried ContextKey with a composite PRIMARY KEY on
5758
// (CommandId, ContextKey) — see the V1-only/born-past-V1 note in the class remarks.
58-
// {0} = table name (validated).
59-
// Intentionally NOT double-quoted — PG identifiers are case-folded when unquoted and the
60-
// PG inbox is V1-only (no V2+ chain). Quoting V1 here would change runtime semantics for
61-
// existing deployments (case-folded `outbox`-style lookups would miss a now case-sensitive
62-
// table). Reserved-keyword identifiers (e.g. "Order") are a chain-wide PG limitation, not
63-
// a V1-specific gap. Per PR #4039 reviewer item F2-1.
59+
// {0} = quoted, lowercased table identifier (via PgIdentifier).
60+
// Lowercase-then-quote: PG case-folds unquoted identifiers to lowercase at parse time, so
61+
// the legacy unquoted form created e.g. `inbox` from a configured `"Inbox"`. Quoting the
62+
// already-lowercased form ("inbox") resolves to the same physical table as the legacy
63+
// unquoted form, AND lets reserved-keyword names (User, Order, …) parse cleanly. All
64+
// PG catalogs and builders share this fold-then-quote convention via PgIdentifier.
6465
private const string V1HistoricalDdl =
6566
"""
6667
CREATE TABLE {0}
@@ -114,7 +115,7 @@ public IReadOnlyList<IAmABoxMigration> All(IAmARelationalDatabaseConfiguration c
114115
new BoxMigration(
115116
Version: 1,
116117
Description: "Create inbox table",
117-
UpScript: string.Format(V1HistoricalDdl, configuration.InBoxTableName),
118+
UpScript: string.Format(V1HistoricalDdl, PgIdentifier.Quote(configuration.InBoxTableName)),
118119
LogicalColumns: new HashSet<string>(StringComparer.Ordinal)
119120
{
120121
"commandid", "commandtype", "commandbody", "timestamp", "contextkey"

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlOutboxMigrationCatalog.cs

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ THE SOFTWARE. */
2525
using System.Collections.Generic;
2626
using System.Linq;
2727
using Paramore.Brighter.Outbox.PostgreSql;
28+
using Paramore.Brighter.PostgreSql;
2829

2930
namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
3031

@@ -74,13 +75,14 @@ public class PostgreSqlOutboxMigrationCatalog : IAmABoxMigrationCatalog
7475

7576
// Literal historical PostgreSQL outbox DDL extracted from commit 3c30343fa (July 2019).
7677
// First-shipped state already carried Dispatched — see the born-past-V1 note in the
77-
// class remarks. {0} = table name (validated).
78-
// Intentionally NOT double-quoted — V2..V7 ALTERs in this catalog also use unquoted
79-
// identifiers and rely on PG's identifier case-folding. Adding "{0}" to V1 would create
80-
// a case-sensitive table that the unquoted V2..V7 ALTERs could not reach, so V1 stays
81-
// symmetric with the rest of the chain. Reserved-keyword identifiers (e.g. "Order") are
82-
// a chain-wide limitation on PG, not a V1-specific gap — discussed under PR #4039
83-
// reviewer item F2-1 and tracked separately if it ever becomes a real constraint.
78+
// class remarks. {0} = quoted, lowercased table identifier (via PgIdentifier).
79+
// Lowercase-then-quote: PG case-folds unquoted identifiers to lowercase at parse time,
80+
// so historical tables created via the unquoted builder DDL live as e.g. `outbox` in
81+
// pg_class regardless of the configured `OutBoxTableName = "Outbox"`. Quoting the
82+
// already-lowercased form ("outbox") resolves to the same physical table as the legacy
83+
// unquoted form would have, AND unlocks reserved-keyword names (Order, User, Group, …)
84+
// that the unquoted V2..V7 ALTERs would otherwise reject as syntax errors. All catalogs
85+
// and builders in this chain apply the same fold-then-quote rule via PgIdentifier.
8486
private const string V1HistoricalDdl =
8587
"""
8688
CREATE TABLE {0}
@@ -137,7 +139,7 @@ public IReadOnlyList<IAmABoxMigration> All(IAmARelationalDatabaseConfiguration c
137139
new BoxMigration(
138140
Version: 1,
139141
Description: "Create outbox table",
140-
UpScript: string.Format(V1HistoricalDdl, table),
142+
UpScript: string.Format(V1HistoricalDdl, PgIdentifier.Quote(table)),
141143
LogicalColumns: Cumulative(1)),
142144

143145
new BoxMigration(
@@ -215,5 +217,9 @@ private static string AddColumns(string schema, string table, params (string Col
215217
string.Join(Environment.NewLine, columns.Select(c => AddColumn(schema, table, c.Column, c.Type)));
216218

217219
private static string AddColumn(string schema, string table, string column, string type) =>
218-
$"ALTER TABLE {schema}.{table} ADD COLUMN IF NOT EXISTS {column} {type} NULL;";
220+
// schema/table are lowercase-then-quoted via PgIdentifier so the ALTER targets the
221+
// same physical table that V1's CREATE produced — and so reserved-keyword names
222+
// (Order, User, Group, …) parse cleanly. Column names are sourced from the catalog
223+
// and are already ADR 0057 §1 lowercase, so they are emitted bare.
224+
$"ALTER TABLE {PgIdentifier.QuoteQualified(schema, table)} ADD COLUMN IF NOT EXISTS {column} {type} NULL;";
219225
}

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlPayloadModeValidator.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ THE SOFTWARE. */
2424
using System.Threading;
2525
using System.Threading.Tasks;
2626
using Npgsql;
27+
using Paramore.Brighter.PostgreSql;
2728

2829
namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
2930

@@ -60,9 +61,11 @@ public async Task ValidateAsync(
6061
command.CommandText = @"
6162
SELECT data_type FROM information_schema.columns
6263
WHERE table_schema = @SchemaName AND table_name = @TableName AND column_name = @ColumnName";
63-
command.Parameters.AddWithValue("@SchemaName", resolvedSchema);
64-
command.Parameters.AddWithValue("@TableName", tableName);
65-
command.Parameters.AddWithValue("@ColumnName", columnName);
64+
// information_schema.columns stores PG-folded (lowercase) identifiers. Normalize
65+
// configured values so mixed-case defaults match the stored folded form.
66+
command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(resolvedSchema));
67+
command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
68+
command.Parameters.AddWithValue("@ColumnName", PgIdentifier.Normalize(columnName));
6669

6770
var dataType = (string?)await command.ExecuteScalarAsync(cancellationToken);
6871
if (dataType == null) return;

src/Paramore.Brighter.Inbox.Postgres/PostgreSqlInbox.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ THE SOFTWARE. */
2525

2626
using System;
2727
using System.Data;
28+
using System.Linq;
2829
using Npgsql;
2930
using NpgsqlTypes;
3031
using Paramore.Brighter.Logging;
@@ -63,4 +64,15 @@ protected override IDbDataParameter CreateJsonSqlParameter(string parameterName,
6364
{
6465
return new NpgsqlParameter { ParameterName = parameterName, NpgsqlDbType = DatabaseConfiguration.BinaryMessagePayload ? NpgsqlDbType.Jsonb : NpgsqlDbType.Json,Value = value ?? DBNull.Value };
6566
}
67+
68+
/// <summary>
69+
/// Lowercase-then-quote the configured inbox table name so reserved-keyword names
70+
/// (Order, User, Group, …) parse cleanly and mixed-case configured values resolve to
71+
/// the same physical table as PG's natural case-fold of the legacy unquoted form would
72+
/// have produced.
73+
/// </summary>
74+
protected override string GenerateSqlText(string sqlFormat, params string[] orderedParams)
75+
=> string.Format(
76+
sqlFormat,
77+
orderedParams.Prepend(PgIdentifier.Quote(DatabaseConfiguration.InBoxTableName)).ToArray());
6678
}

0 commit comments

Comments
 (0)