diff --git a/CHANGELOG.md b/CHANGELOG.md index d412978..8d0930a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,23 @@ # Changelog -## 5.0.0 (unreleased) — Security & performance hardening +## 5.3.0 (unreleased) + +### Added + +- **`UpsertObjectAsync` reports whether the write inserted or updated.** Returns + `UpsertResult.Inserted` when no row existed for the key in that partition and + `UpsertResult.Updated` when one did and its data was replaced; stored contents are identical + to `WriteObjectAsync`. Implemented as `INSERT OR IGNORE` plus, only when nothing was inserted, + an in-place `UPDATE`, both under the connection gate and the transaction — so callers that keep + an incremental view of the store (a queue count, an added/removed signal) no longer need a + read-then-write pair guarded by a lock of their own. Registered-id and explicit key-selector + overloads, same strict-mode divergence guard. There is deliberately no failure value: any + failure (insert ignored for another constraint, update not affecting exactly one row, a + serializer or SQLite error) throws `TychoException` and leaves the row as it was — with a + transaction it is rolled back, without one the single failing statement is atomic on its own. + (#32) + +## 5.0.0 — 2026-07-21 — Security & performance hardening This release closes a critical SQL-injection vector and a data-integrity bug, and adds proven write/startup performance improvements. It is a **major** version because diff --git a/README.md b/README.md index 4e3ce25..4e3aa37 100644 --- a/README.md +++ b/README.md @@ -192,6 +192,34 @@ the call site. > cannot reach. Types registered with `AddTypeRegistrationWithCustomKeySelector` have no id > property to compare against and are unaffected. +## Knowing Whether a Write Inserted or Updated + +`WriteObjectAsync` reports only success. When you keep something derived from the store — a +queue count, an added/removed signal — you need to know whether the write **created** the row +or **replaced** one, and a read-then-write pair of your own is not atomic against other +writers. `UpsertObjectAsync` answers that from inside the connection gate and transaction: + +```csharp +var result = await db.UpsertObjectAsync(queuedItem, x => x.Key, "queue"); + +if (result == UpsertResult.Inserted) +{ + queueCount++; // exact: an edit of an already-queued item returns Updated +} +``` + +Stored contents are identical to `WriteObjectAsync`; the row is keyed by +`(Key, FullTypeName, Partition)`, so the same key in another partition or for another type is +`Inserted`. The registered-id overload `UpsertObjectAsync(obj, partition)` and the strict-mode +key-divergence guard behave exactly as they do for `WriteObjectAsync`. + +There is no third result. A call that returns has written the object; anything else — the +insert ignored for a reason other than an existing row, the update touching anything but one +row, a serializer or SQLite error on either statement — throws `TychoException` and leaves the +stored row exactly what it was before the call (only one of the two statements ever modifies +data, so this holds with `withTransaction: false` too). Callers that count on the result should +let that exception surface rather than treat it as "updated". + ## Querying Objects TychoDB offers rich querying capabilities. diff --git a/TychoDB.UnitTests/UpsertObjectTests.cs b/TychoDB.UnitTests/UpsertObjectTests.cs new file mode 100644 index 0000000..f2d8c64 --- /dev/null +++ b/TychoDB.UnitTests/UpsertObjectTests.cs @@ -0,0 +1,295 @@ +#nullable enable + +using System; +using System.IO; +using System.Threading.Tasks; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Shouldly; +using TychoDB; + +namespace TychoDB.UnitTests; + +/// +/// UpsertObjectAsync exists so a caller can learn whether a write created a row or +/// replaced one without a read-then-write pair of its own. The tests pin the answer on the +/// three axes of the primary key (key, type, partition) and check the stored data matches the +/// last write. +/// +[TestClass] +public class UpsertObjectTests +{ + private const string PartitionA = "partitionA"; + private const string PartitionB = "partitionB"; + + [TestMethod] + public async Task FirstWriteOfAKey_IsInserted() + { + using var db = Connect(); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA); + + result.ShouldBe(UpsertResult.Inserted); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("one"); + } + + [TestMethod] + public async Task SecondWriteOfTheSameKey_IsUpdated_AndReplacesTheData() + { + using var db = Connect(); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key, PartitionA); + + result.ShouldBe(UpsertResult.Updated); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("two"); + (await db.CountObjectsAsync(PartitionA)).ShouldBe(1); + } + + [TestMethod] + public async Task RewritingIdenticalData_IsUpdated_NotAFailure() + { + // SQLite's change count is the number of rows the UPDATE matched, not the number whose + // bytes differed, so an idempotent rewrite reports one affected row and must come back + // Updated rather than tripping the exactly-one-row check. + using var db = Connect(); + var doc = new Doc { Key = "k1", Description = "same" }; + await db.UpsertObjectAsync(doc, x => x.Key, PartitionA); + + var result = await db.UpsertObjectAsync(doc, x => x.Key, PartitionA); + + result.ShouldBe(UpsertResult.Updated); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("same"); + } + + [TestMethod] + public async Task ARowWrittenByWriteObjectAsync_CountsAsExisting() + { + using var db = Connect(); + await db.WriteObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key, PartitionA); + + result.ShouldBe(UpsertResult.Updated); + } + + [TestMethod] + public async Task SameKeyInAnotherPartition_IsInserted() + { + using var db = Connect(); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "a" }, x => x.Key, PartitionA); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "b" }, x => x.Key, PartitionB); + + result.ShouldBe(UpsertResult.Inserted); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("a"); + (await db.ReadObjectAsync("k1", PartitionB))!.Description.ShouldBe("b"); + } + + [TestMethod] + public async Task SameKeyForAnotherType_IsInserted() + { + using var db = Connect(); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "doc" }, x => x.Key, PartitionA); + + var result = await db.UpsertObjectAsync(new Other { Key = "k1", Name = "other" }, x => x.Key, PartitionA); + + result.ShouldBe(UpsertResult.Inserted); + } + + [TestMethod] + public async Task NoPartition_BehavesLikeTheEmptyPartition() + { + using var db = Connect(); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key); + + result.ShouldBe(UpsertResult.Updated); + (await db.ReadObjectAsync("k1"))!.Description.ShouldBe("two"); + } + + [TestMethod] + public async Task RegisteredIdOverload_UsesTheRegisteredKey() + { + using var db = Connect(register: true); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, PartitionA); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, PartitionA); + + result.ShouldBe(UpsertResult.Updated); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("two"); + } + + [TestMethod] + public async Task WithoutATransaction_StillReportsTheOutcome() + { + using var db = Connect(); + await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA, withTransaction: false); + + var result = await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key, PartitionA, withTransaction: false); + + result.ShouldBe(UpsertResult.Updated); + } + + [TestMethod] + public async Task NullObject_Throws() + { + using var db = Connect(); + + await Should.ThrowAsync( + async () => await db.UpsertObjectAsync(null!, x => x.Key, PartitionA)); + } + + [TestMethod] + public async Task NullKeySelector_Throws() + { + using var db = Connect(); + + await Should.ThrowAsync( + async () => await db.UpsertObjectAsync(new Doc { Key = "k1" }, null!, PartitionA)); + } + + [TestMethod] + public async Task InsertPathFailure_Throws_AndWritesNothing() + { + // The serializer blows up, so nothing reaches SQLite. That must surface as a + // TychoException (never as an outcome) and leave no row behind. + using var db = Connect(serializer: new ThrowingSerializer(failFromCall: 1)); + + await Should.ThrowAsync( + async () => await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA)); + + (await db.CountObjectsAsync(PartitionA)).ShouldBe(0); + } + + [TestMethod] + public async Task UpdatePathFailure_Throws_AndLeavesTheExistingRowUntouched() + { + // The row exists, so INSERT OR IGNORE is ignored and the follow-up UPDATE runs into a + // trigger that aborts it. The failure must be an exception, and the transaction must + // roll back to the original data - the "ignored, then failed" case from the review. + var (db, path) = ConnectWithPath(persistConnection: false); + using (db) + { + (await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA)) + .ShouldBe(UpsertResult.Inserted); + + AbortEveryUpdate(path); + + await Should.ThrowAsync( + async () => await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key, PartitionA)); + + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("one"); + (await db.CountObjectsAsync(PartitionA)).ShouldBe(1); + } + } + + [TestMethod] + public async Task AfterAFailure_TheNextUpsertStillWorks() + { + using var db = Connect(serializer: new ThrowingSerializer(failFromCall: 1, failCount: 1)); + + await Should.ThrowAsync( + async () => await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "one" }, x => x.Key, PartitionA)); + + (await db.UpsertObjectAsync(new Doc { Key = "k1", Description = "two" }, x => x.Key, PartitionA)) + .ShouldBe(UpsertResult.Inserted); + (await db.ReadObjectAsync("k1", PartitionA))!.Description.ShouldBe("two"); + } + + private static Tycho Connect(bool register = false, IJsonSerializer? serializer = null) + { + return ConnectWithPath(register, serializer).Db; + } + + private static (Tycho Db, string Path) ConnectWithPath(bool register = false, IJsonSerializer? serializer = null, bool persistConnection = true) + { + var dir = Path.GetTempPath(); + var name = $"{Guid.NewGuid()}.db"; + + // Pooling keeps a closed connection's handle alive; a test that installs DDL from a + // second connection needs the first one really closed, so pooling follows persistence. + var db = new Tycho(dir, serializer ?? new NewtonsoftJsonSerializer(), dbName: name, persistConnection: persistConnection, rebuildCache: true, requireTypeRegistration: false, useConnectionPooling: persistConnection); + + if (register) + { + db.AddTypeRegistrationWithCustomKeySelector(x => x.Key); + } + + return (db.Connect(), Path.Combine(dir, name)); + } + + /// + /// Installs a trigger through a second connection so every UPDATE on JsonValue aborts. + /// Needs a Tycho opened with persistConnection: false, otherwise Tycho's held connection + /// keeps the writer lock and the DDL fails with "database is locked". + /// + private static void AbortEveryUpdate(string path) + { + using var connection = new Microsoft.Data.Sqlite.SqliteConnection($"Data Source={path}"); + connection.Open(); + + using var command = connection.CreateCommand(); + command.CommandText = + """ + CREATE TRIGGER abort_every_update BEFORE UPDATE ON JsonValue + BEGIN + SELECT RAISE(ABORT, 'update refused by test trigger'); + END; + """; + command.ExecuteNonQuery(); + } + + public class Doc + { + public string Key { get; set; } = string.Empty; + + public string Description { get; set; } = string.Empty; + } + + /// + /// Delegates to the real serializer except for a window of calls (1-based, counted per + /// serialize) during which it throws, standing in for any failure ahead of the INSERT. + /// + private sealed class ThrowingSerializer(int failFromCall, int failCount = int.MaxValue) : IJsonSerializer + { + private readonly NewtonsoftJsonSerializer _inner = new(); + + private int _calls; + + public string DateTimeSerializationFormat => _inner.DateTimeSerializationFormat; + + public object Serialize(T obj) + { + return ShouldFail() ? throw new InvalidOperationException("serializer refused") : _inner.Serialize(obj); + } + + public void Serialize(T obj, System.Buffers.IBufferWriter bufferWriter) + { + if (ShouldFail()) + { + throw new InvalidOperationException("serializer refused"); + } + + _inner.Serialize(obj, bufferWriter); + } + + public System.Threading.Tasks.ValueTask DeserializeAsync(Stream stream, System.Threading.CancellationToken cancellationToken) + { + return _inner.DeserializeAsync(stream, cancellationToken); + } + + private bool ShouldFail() + { + var call = ++_calls; + return call >= failFromCall && call - failFromCall < failCount; + } + } + + public class Other + { + public string Key { get; set; } = string.Empty; + + public string Name { get; set; } = string.Empty; + } +} diff --git a/TychoDB/Queries.cs b/TychoDB/Queries.cs index d3a7c79..3ac92fe 100644 --- a/TychoDB/Queries.cs +++ b/TychoDB/Queries.cs @@ -132,6 +132,33 @@ INSERT OR REPLACE INTO JsonValue(Key, FullTypeName, Data, Partition) SELECT last_insert_rowid(); """; + /// + /// First half of the outcome-reporting upsert: inserts only when the + /// (Key, FullTypeName, Partition) row is absent. One affected row means the object was + /// inserted; zero means it already existed and + /// runs next. + /// + public const string InsertOrIgnore = + """ + INSERT OR IGNORE INTO JsonValue(Key, FullTypeName, Data, Partition) + VALUES ($key, $fullTypeName, json($json), $partition); + """; + + /// + /// Second half of the outcome-reporting upsert: replaces the stored JSON of an existing row. + /// + public const string UpdateDataWithKeyAndFullTypeName = + """ + UPDATE JsonValue + SET Data = json($json) + WHERE + Key = $key + AND + FullTypeName = $fullTypeName + AND + Partition = $partition; + """; + private const string BatchInsertPrefix = "INSERT OR REPLACE INTO JsonValue(Key, FullTypeName, Data, Partition) VALUES "; diff --git a/TychoDB/Tycho.cs b/TychoDB/Tycho.cs index 3086ac1..82fcfa7 100644 --- a/TychoDB/Tycho.cs +++ b/TychoDB/Tycho.cs @@ -556,6 +556,168 @@ public ValueTask WriteObjectsAsync(IEnumerable objs, Func cancellationToken); } + /// + /// Writes a single object and reports whether the write created the row or replaced an + /// existing one, using registered type information to determine the ID. + /// + /// The type of the object to write. + /// The object to write. + /// Optional partition key to organize objects. + /// Whether to use a transaction for the operation. + /// A token to cancel the asynchronous operation. + /// + /// when no row existed for the key in that partition, + /// when one did and its data was replaced. + /// + /// + /// Stored contents are identical to ; + /// the difference is only the answer. The insert/update decision is made inside the + /// connection gate (and, when is true, the + /// transaction), so a caller keeping an incremental view of the store (a queue count, an + /// added/removed signal) can rely on it without a read-then-write pair and an outer lock + /// of its own. + /// + /// There is no failure value: the result is only ever one of the two outcomes, and a call + /// that returns has written the object. Every failure throws + /// and leaves the row exactly as it was - only one of the two statements ever modifies + /// data, so with a transaction it is rolled back and without one the failed statement is + /// atomic on its own. That covers the insert being ignored for a reason other than an + /// existing row (the follow-up update then affects nothing), the update affecting anything + /// other than one row, and any serializer or SQLite error on either statement. + /// + /// + public ValueTask UpsertObjectAsync(T obj, string? partition = null, bool withTransaction = true, + CancellationToken cancellationToken = default) + { + return UpsertObjectAsync(obj, GetIdSelectorFor(), partition, withTransaction, cancellationToken); + } + + /// + /// Writes a single object using a custom key selector and reports whether the write created + /// the row or replaced an existing one. + /// + /// The type of the object to write. + /// The object to write. + /// A function that extracts the key from the object. + /// Optional partition key to organize objects. + /// Whether to use a transaction for the operation. + /// A token to cancel the asynchronous operation. + /// + /// when no row existed for the key in that partition, + /// when one did and its data was replaced. + /// + /// + /// The key selector follows the same rules as + /// , + /// including the strict-mode divergence guard. Failure semantics are those of + /// : never a third + /// result value, always a with the row left as it was. + /// + public ValueTask UpsertObjectAsync(T obj, Func keySelector, string? partition = null, + bool withTransaction = true, CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(obj); + ArgumentNullException.ThrowIfNull(keySelector); + ArgumentNullException.ThrowIfNull(_connection); + + keySelector = GuardAgainstKeyDivergence(keySelector); + + return _connection + .WithConnectionBlockAsync( + _connectionGate, + (obj, keySelector, partition, withTransaction, _commandTimeout, _jsonSerializer), + static (conn, state) => + { + SqliteTransaction? transaction = null; + + if (state.withTransaction) + { + transaction = conn.BeginTransaction(IsolationLevel.Serializable); + } + + try + { + var keyValue = state.keySelector(state.obj); + var fullTypeNameValue = TypeCache.FullName; + var partitionValue = state.partition.AsValueOrEmptyString(); + + using var serializationStream = _memoryStreamManager.GetStream("TychoDB.UpsertObject"); + state._jsonSerializer.Serialize(state.obj, serializationStream); + var json = serializationStream.ToArray(); + + // INSERT OR IGNORE affects one row only when the key was absent, which is + // exactly the answer; if it affected nothing the row exists and the data + // is replaced in place. Both statements run under the same gate and + // transaction, so no other writer can slip between them. + using var insertCommand = conn.CreateCommand(); + if (transaction is not null) + { + insertCommand.Transaction = transaction; + } + + insertCommand.CommandTimeout = state._commandTimeout; + insertCommand.CommandText = Queries.InsertOrIgnore; + insertCommand.Parameters.Add(ParameterKey, SqliteType.Text).Value = keyValue; + insertCommand.Parameters.Add(ParameterFullTypeName, SqliteType.Text).Value = fullTypeNameValue; + insertCommand.Parameters.Add(ParameterJson, SqliteType.Blob).Value = json; + insertCommand.Parameters.Add(ParameterPartition, SqliteType.Text).Value = partitionValue; + + var result = UpsertResult.Inserted; + var affected = insertCommand.ExecuteNonQuery(); + + if (affected == 0) + { + using var updateCommand = conn.CreateCommand(); + if (transaction is not null) + { + updateCommand.Transaction = transaction; + } + + updateCommand.CommandTimeout = state._commandTimeout; + updateCommand.CommandText = Queries.UpdateDataWithKeyAndFullTypeName; + updateCommand.Parameters.Add(ParameterKey, SqliteType.Text).Value = keyValue; + updateCommand.Parameters.Add(ParameterFullTypeName, SqliteType.Text).Value = fullTypeNameValue; + updateCommand.Parameters.Add(ParameterJson, SqliteType.Blob).Value = json; + updateCommand.Parameters.Add(ParameterPartition, SqliteType.Text).Value = partitionValue; + + affected = updateCommand.ExecuteNonQuery(); + result = UpsertResult.Updated; + } + + // Two outcomes only. INSERT OR IGNORE can be ignored for a constraint + // other than the existing-row case, and then the UPDATE finds nothing; + // either way anything but exactly one affected row is a failure, and a + // failure is an exception plus rollback - never a quiet "Updated". + // (SQLite's change count is the number of rows the UPDATE matched, so a + // rewrite with identical JSON still reports 1 - see the idempotent test.) + if (affected != 1) + { + throw new TychoException($"Upsert affected {affected} rows; expected exactly one ({result})"); + } + + transaction?.Commit(); + + return result; + } + catch (TychoException) + { + transaction?.Rollback(); + throw; + } + catch (Exception ex) + { + transaction?.Rollback(); + throw new TychoException("Failed Upserting Object", ex); + } + finally + { + transaction?.Dispose(); + } + }, + _persistConnection, + cancellationToken); + } + /// /// Counts objects matching the optional filter criteria. /// diff --git a/TychoDB/UpsertResult.cs b/TychoDB/UpsertResult.cs new file mode 100644 index 0000000..6038974 --- /dev/null +++ b/TychoDB/UpsertResult.cs @@ -0,0 +1,14 @@ +namespace TychoDB; + +/// +/// What +/// did to the row for the object's key. +/// +public enum UpsertResult +{ + /// No row existed for the key in that partition; one was created. + Inserted, + + /// A row already existed for the key in that partition; its data was replaced. + Updated, +}