diff --git a/ZB.MOM.WW.LocalDb/README.md b/ZB.MOM.WW.LocalDb/README.md index f0ba7a4..5b853a0 100644 --- a/ZB.MOM.WW.LocalDb/README.md +++ b/ZB.MOM.WW.LocalDb/README.md @@ -174,6 +174,8 @@ builder.Services.AddZbLocalDb(builder.Configuration, db => "CREATE TABLE IF NOT EXISTS widget (id INTEGER PRIMARY KEY, name TEXT, qty INTEGER)") .GetAwaiter().GetResult(); db.RegisterReplicated("widget"); // PK required; installs capture triggers + // The application assigns widget ids. A key SQLite assigns per node (AUTOINCREMENT, or a NULL + // rowid) collides across a pair: AUTOINCREMENT is refused, prefer TEXT GUIDs. See Limitations. }); // Consume ILocalDb anywhere: @@ -295,6 +297,14 @@ All options are validated at startup (`ZB.MOM.WW.Configuration` validators). - **Exactly two nodes.** No mesh / N-node topologies. - **LWW only.** No per-table pluggable conflict resolvers. +- **Replicated primary keys must be unique across both nodes.** LWW keys on the primary key, + so two different rows inserted on the two nodes under the same key are read as two versions + of one row, and one is lost on **both** nodes with no dead-letter and no metric. A key each + node assigns on its own cannot replicate: `RegisterReplicated` **refuses an `AUTOINCREMENT` + key** (`0.3.6`), and a plain `INTEGER PRIMARY KEY` is accepted only because the application + may assign it from a node-scoped range — left `NULL` on insert it loses rows the same way. + Use a TEXT GUID minted by the writer (what ScadaBridge's `site_events` and OtOpcUa's alarm + store-and-forward did when they moved onto LocalDb). - **BLOB columns are rejected at registration** (a column declared `BLOB` fails `RegisterReplicated`, because `json_object` cannot capture it). A *typeless* column can still hold a BLOB value at runtime, which then surfaces as a write-time error, not a diff --git a/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/ILocalDb.cs b/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/ILocalDb.cs index ca95f67..8568fb0 100644 --- a/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/ILocalDb.cs +++ b/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/ILocalDb.cs @@ -38,11 +38,22 @@ public interface ILocalDb /// schema digest, and installs the three AFTER triggers that capture every change into the /// oplog + row-version tables within the same transaction as the consumer's write. Idempotent /// (re-registering regenerates the triggers). Throws - /// if the name is unsafe, the table is missing, it has no primary key, or it is the parent of a - /// foreign key whose ON DELETE action mutates the child (CASCADE / SET NULL / - /// SET DEFAULT) — see the remarks for why that last one cannot be replicated. + /// if the name is unsafe, the table is missing, it has no primary key, its primary key is + /// AUTOINCREMENT (0.3.6), or it is the parent of a foreign key whose ON DELETE action + /// mutates the child (CASCADE / SET NULL / SET DEFAULT) — see the remarks for why + /// the last two cannot be replicated. /// /// + /// + /// Replicated primary keys must be unique across BOTH nodes. Last-writer-wins keys on the + /// primary key, so two different rows that two nodes insert under the same key are read as two + /// versions of ONE row and one of them is silently lost on both nodes. A key each node assigns on + /// its own therefore cannot replicate: an AUTOINCREMENT key is refused outright, and a plain + /// INTEGER PRIMARY KEY (a rowid alias) is accepted only because the application may assign + /// it from a node-scoped range — left NULL on insert it loses rows the same way. Prefer a TEXT GUID + /// minted by the writer. + /// + /// /// Replicated tables may not be the parent of a cascading foreign key. The apply path /// deletes rows for remote tombstones (and, before 0.3.6, wrote remote rows with /// INSERT OR REPLACE, which deleted the conflicting row first); with @@ -52,6 +63,7 @@ public interface ILocalDb /// runs at registration and inspects the whole database's foreign keys; SQLite cannot add a /// foreign key to an existing table, so a later-created child table is caught on the next /// registration (i.e. the next startup), not at the moment of the DDL. + /// /// /// Secondary UNIQUE keys must be unique across BOTH nodes. Since 0.3.6 a remote row /// is upserted on its primary key only; if it collides with a different local row on another diff --git a/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs b/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs index 455d7a1..d02c58c 100644 --- a/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs +++ b/ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs @@ -199,6 +199,7 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable throw new LocalDbRegistrationException( $"Cannot register table '{tableName}': replication requires an explicit primary key."); + EnsureNoAutoincrementKey(tableName); EnsureNoInboundActionOnDelete(tableName); var allColumns = columns.Select(c => (c.Name, c.Type)).ToList(); @@ -295,6 +296,46 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable } } + /// + /// Refuses to register a table whose primary key is declared AUTOINCREMENT (LocalDb-005). + /// + /// + /// + /// Why. LWW keys on pk_json. AUTOINCREMENT exists to have SQLite assign the + /// key, and each node assigns its own 1, 2, 3 … — so two DIFFERENT rows inserted on the two nodes + /// (both nodes of a pair logging during a split, say) carry the same pk_json, LWW reads them + /// as two versions of one row, and one row is lost on BOTH nodes with no dead-letter and no metric. + /// That is never what a replicated table wants, so it is refused rather than warned about. + /// + /// + /// A plain INTEGER PRIMARY KEY is still allowed, because the application may assign + /// it itself from a globally unique or node-scoped range. Left NULL on insert it has exactly the + /// same hazard (it is the same rowid alias, minus the never-reuse guarantee); the remarks on + /// say so, and the safe choice is a TEXT GUID. + /// + /// + /// Detection reads SQLite's own column metadata (sqlite3_table_column_metadata, surfaced as + /// IsAutoIncrement in the reader's schema table) rather than parsing the CREATE text, + /// so a column named or defaulted to the word is not mistaken for the keyword. + /// + /// + private void EnsureNoAutoincrementKey(string tableName) + { + using var cmd = _master.CreateCommand(); + cmd.CommandText = $"SELECT * FROM {TriggerSqlGenerator.Ident(tableName)} LIMIT 0"; + using var reader = cmd.ExecuteReader(); + var schema = reader.GetSchemaTable(); + foreach (System.Data.DataRow row in schema.Rows) + { + if (row["IsAutoIncrement"] is true) + throw new LocalDbRegistrationException( + $"Cannot register table '{tableName}': its primary key '{row["BaseColumnName"]}' is declared " + + "AUTOINCREMENT. Each node would assign the same keys to different rows, and last-writer-wins " + + "would silently keep only one of them on both nodes. Use a globally unique key instead " + + "(a TEXT GUID, or an INTEGER the application assigns from a node-scoped range)."); + } + } + /// /// Refuses to register a table that is the PARENT of a foreign key whose ON DELETE action /// mutates the child (CASCADE, SET NULL, SET DEFAULT). diff --git a/ZB.MOM.WW.LocalDb/tests/ZB.MOM.WW.LocalDb.Tests/RegistrationTests.cs b/ZB.MOM.WW.LocalDb/tests/ZB.MOM.WW.LocalDb.Tests/RegistrationTests.cs index a60fa0a..81b2062 100644 --- a/ZB.MOM.WW.LocalDb/tests/ZB.MOM.WW.LocalDb.Tests/RegistrationTests.cs +++ b/ZB.MOM.WW.LocalDb/tests/ZB.MOM.WW.LocalDb.Tests/RegistrationTests.cs @@ -70,6 +70,58 @@ public sealed class RegistrationTests : IDisposable Assert.Contains("BLOB", ex.Message); } + // ---- LocalDb-005 (scadaproj#46): AUTOINCREMENT keys are assigned independently on each node ---- + + [Fact] + public async Task Register_AutoincrementKey_Throws() + { + // Each node hands out 1, 2, 3 … on its own, so two DIFFERENT rows inserted on the two nodes + // share a pk_json; LWW reads them as two versions of one row and one is lost on both nodes. + using var db = Create(); + await db.ExecuteAsync("CREATE TABLE ev (id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)"); + + var ex = Assert.Throws(() => db.RegisterReplicated("ev")); + Assert.Contains("ev", ex.Message); + Assert.Contains("AUTOINCREMENT", ex.Message); + // Refused before anything was installed. + Assert.Empty(await db.QueryAsync( + "SELECT name FROM sqlite_master WHERE type='trigger' AND tbl_name='ev'", r => r.GetString(0))); + } + + [Fact] + public async Task Register_AutoincrementKey_Throws_WithBaselineToo() + { + using var db = Create(); + await db.ExecuteAsync("CREATE TABLE ev (id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)"); + + Assert.Throws(() => db.RegisterReplicated("ev", baselineExistingRows: true)); + } + + [Fact] + public async Task Register_PlainIntegerPrimaryKey_IsStillAllowed() + { + // Compatibility guard: an INTEGER PRIMARY KEY the application assigns itself (node-scoped or + // otherwise globally unique ids) is legitimate, and refusing it would break existing schemas. + using var db = Create(); + await CreateOrders(db); + + db.RegisterReplicated("orders"); + Assert.True(db.ReplicatedTables.ContainsKey("orders")); + } + + [Fact] + public async Task Register_TheWordAutoincrementOutsideTheKey_IsNotMistakenForIt() + { + // Detection reads SQLite's own column metadata, not the CREATE text, so a column NAMED + // autoincrement or a default value spelling it does not trip the check. + using var db = Create(); + await db.ExecuteAsync( + "CREATE TABLE notes (id TEXT PRIMARY KEY, \"autoincrement\" INTEGER, note TEXT DEFAULT 'AUTOINCREMENT')"); + + db.RegisterReplicated("notes"); + Assert.True(db.ReplicatedTables.ContainsKey("notes")); + } + [Fact] public async Task Register_CascadeDeleteParent_Throws() { diff --git a/code-reviews/LocalDb/findings.md b/code-reviews/LocalDb/findings.md index e947b3a..efe1bc8 100644 --- a/code-reviews/LocalDb/findings.md +++ b/code-reviews/LocalDb/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude | | Commit reviewed | `91fb2b4` | -| Open findings | 2 | +| Open findings | 1 | ## Summary @@ -362,7 +362,7 @@ applied and that no dead-letter is written. README §Semantics is updated. | Severity | Low | | Priority | P3 | | Category | Documentation & XML docs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs:197` | **Description** @@ -393,7 +393,28 @@ at registration when the sole PK column is an `INTEGER PRIMARY KEY` rowid alias. **Resolution** -_Unresolved._ +Resolved 2026-11-09 (scadaproj#46, LocalDb `0.3.6`, unpublished). Re-verified with a throwaway 2-node probe: +`ev(id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)`, `'from A'` inserted on A and `'from B'` on B, both nodes ended +with `1:from B` only and no dead-letter. Fix, taking the review's safer option: `RegisterReplicated` (both overloads) +now **refuses** a table whose primary key is `AUTOINCREMENT` with a `LocalDbRegistrationException` that names the +column and the remedy, before any trigger is installed (`SqliteLocalDb.EnsureNoAutoincrementKey`). Detection reads +SQLite's column metadata (`IsAutoIncrement` from `sqlite3_table_column_metadata`), not the `CREATE` text. A refusal +rather than a warning, because `AUTOINCREMENT` only exists to have SQLite assign the key, which is never safe on a +replicated table, and `SqliteLocalDb` has no logger to warn through. + +A plain `INTEGER PRIMARY KEY` is still accepted, because an application may assign it from a node-scoped range; +refusing it would break schemas that are correct today. Its hazard when left `NULL` is documented instead: the +`RegisterReplicated` XML remarks and README §Limitations now say replicated primary keys must be unique across both +nodes and recommend writer-minted TEXT GUIDs, and the quickstart notes it. + +Compatibility: checked read-only on 2026-11-09. ScadaBridge's 12 replicated tables (`SiteLocalDbSetup.ReplicatedTables`) +and OtOpcUa's 5 (`LocalDbSetup.ReplicatedTables`: the two deployment-cache tables, the alarm S&F events table, and +the alarm condition state and comments tables) all use TEXT or composite keys, and none is `AUTOINCREMENT`. Both apps had already replaced their historical +`INTEGER PRIMARY KEY AUTOINCREMENT` keys with GUIDs for this reason, so neither boot is affected. + +Tests (`RegistrationTests`): `Register_AutoincrementKey_Throws` and `..._WithBaselineToo` fail pre-fix. Two guards pass +both ways: a plain `INTEGER PRIMARY KEY` still registers, and a column named `autoincrement` with a `DEFAULT +'AUTOINCREMENT'` is not mistaken for the keyword. ### LocalDb-006 — Column names containing `'` are captured but break the apply and snapshot SQL