feat: UpsertObjectAsync reports Inserted vs Updated - #33
Conversation
Closes #32. WriteObjectAsync only says whether the write succeeded. A caller maintaining a derived view of the store (a queue count, an added/removed signal) needs to know whether the row was created or replaced, and a read-then-write pair of its own is not atomic against other writers - in PFM.Mobile the outer lock that made it atomic wedged the whole TPR queue once in the field. UpsertObjectAsync<T>(obj, [keySelector], partition, withTransaction, ct) returns UpsertResult.Inserted / Updated. It runs INSERT OR IGNORE and, only when that affected no row, an in-place UPDATE of Data - both inside the existing connection gate and serializable transaction, one statement on the insert path and two on the update path, no pre-read. Anything other than exactly one affected row throws TychoException like the other write paths. Stored contents match WriteObjectAsync; the registered-id overload and the strict-mode key-divergence guard behave the same. Tests cover first/second write, rows written by WriteObjectAsync, the three primary-key axes (key, type, partition), the empty partition, the registered overload, withTransaction: false, and null arguments. Full suite passes in Debug and Encrypted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6e7babe to
33c84d8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The upsert implementation can throw on idempotent updates because SQLite may report 0 affected rows for no-op UPDATEs, and the changelog now contains multiple “unreleased” version headings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds an outcome-reporting upsert API to TychoDB so callers can distinguish “created row” vs “replaced existing row” without a separate read-then-write guarded by an external lock.
Changes:
- Introduces
UpsertObjectAsync<T>overloads that returnUpsertResult.Inserted/UpsertResult.UpdatedusingINSERT OR IGNOREfollowed byUPDATEwhen needed. - Adds SQL query constants to support the two-step upsert implementation.
- Adds a new
UpsertObjectTestssuite plus README and CHANGELOG documentation updates.
File summaries
| File | Description |
|---|---|
| TychoDB/UpsertResult.cs | Adds the UpsertResult enum used by the new API. |
| TychoDB/Tycho.cs | Implements UpsertObjectAsync overloads and upsert logic. |
| TychoDB/Queries.cs | Adds InsertOrIgnore and UpdateDataWithKeyAndFullTypeName SQL snippets. |
| TychoDB.UnitTests/UpsertObjectTests.cs | Adds coverage for inserted/updated outcomes and failure semantics. |
| README.md | Documents when/why to use UpsertObjectAsync and how to interpret results. |
| CHANGELOG.md | Adds an entry describing the new API and semantics. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| affected = updateCommand.ExecuteNonQuery(); | ||
| result = UpsertResult.Updated; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not the case in SQLite: the change count an UPDATE reports is the number of rows the WHERE clause matched and the statement wrote, not the number whose bytes differed (unlike MySQL's affected-rows). PRAGMA count_changes only controlled whether the count came back as a result row and is deprecated; it never affected the value. So an idempotent rewrite reports 1 and comes back Updated. Pinned that with a test in 1e24553 (RewritingIdenticalData_IsUpdated_NotAFailure, passes in Debug and Encrypted) and noted it inline where the check lives, so the affected == 0 → throw path stays as the guard for the insert-ignored-for-another-constraint case.
| @@ -1,5 +1,21 @@ | |||
| # Changelog | |||
|
|
|||
| ## 5.3.0 (unreleased) | |||
There was a problem hiding this comment.
Agreed. 5.0.0 through 5.2.1 are all tagged, so that heading was stale; 1e24553 marks 5.0.0 with its tag date (2026-07-21), leaving 5.3.0 as the only unreleased section. (5.1.x/5.2.x never got changelog entries; left that as-is rather than backfilling here.)
| /// Stored contents are identical to <see cref="WriteObjectAsync{T}(T, string?, bool, CancellationToken)"/>; | ||
| /// the difference is only the answer. The insert/update decision is made inside the | ||
| /// connection gate and 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 was a problem hiding this comment.
Fair. Reworded in 1e24553: the decision is made inside the connection gate and, when withTransaction is true, the transaction. The row-left-as-it-was guarantee on failure holds either way because only one of the two statements ever modifies data — with a transaction it is rolled back, without one the single failing statement is atomic on its own. README and CHANGELOG say the same.
… section, transaction wording - Test that rewriting identical JSON reports Updated: SQLite's change count is rows matched by the UPDATE, not rows whose bytes differed, so the exactly-one-row check cannot misfire on an idempotent upsert. - Remarks/README/CHANGELOG no longer promise a transaction when withTransaction is false; the row-left-as-it-was guarantee holds either way because only one of the two statements ever modifies data. - CHANGELOG: 5.0.0 marked released (tag date) so 5.3.0 is the only unreleased section. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new UpsertObjectTests file is missing a required using TychoDB; directive (no global using found), which will break compilation in the unit test project.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
TychoDB/Tycho.cs:695
- The thrown
TychoExceptionwhenaffected != 1lacks the key/type/partition details that are needed to diagnose which upsert failed (especially when many upserts run in a batch/loop). Include the values already computed in this scope in the message.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
Implementation matches the stated design and is backed by thorough tests and documentation, with only minor naming cleanup suggested.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
TychoDB/Queries.cs:151
UpdateDataWithKeyAndFullTypeNameis a misleading name for this new query: it also constrainsPartitionin the WHERE clause. That makes future call sites/searches harder and increases the chance someone reuses it incorrectly assuming partition is not part of the predicate.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Closes #32.
What
UpsertObjectAsync<T>(obj, [keySelector], partition, withTransaction, ct)returnsUpsertResult.InsertedorUpsertResult.Updated, so a caller that keeps a derived view of the store (a queue count, an added/removed signal) can learn whether a write created the row or replaced one without a read-then-write pair guarded by a lock of its own. In PFM.Mobile the outer lock that made that pair atomic wedged the whole TPR queue once in the field; this is the database-side answer.How
INSERT OR IGNORE, then — only when that affected no row — an in-placeUPDATEofData. Both run inside the existing connection gate and serializable transaction: one statement on the insert path, two on the update path, no pre-read.WriteObjectAsync. The registered-id overload and the strict-mode key-divergence guard behave the same.TychoExceptionafter rolling back, leaving the row as it was.Tests
13 new tests in
UpsertObjectTests: first/second write, rows previously written byWriteObjectAsync, the three primary-key axes (key, type, partition), the empty partition, the registered overload,withTransaction: false, null arguments, and the failure paths (serializer throws → exception and no row; aBEFORE UPDATEtrigger aborting the update → exception and the original row untouched; the next upsert after a failure still works). Full suite: 309 passed / 4 skipped in both Debug and Encrypted.README section beside the write examples; CHANGELOG entry under 5.3.0.
🤖 Generated with Claude Code