Skip to content

fix: keep valid tags when a bulk upsert hits a bad record (#134) - #146

Open
coreyphillips wants to merge 1 commit into
masterfrom
issue-134
Open

fix: keep valid tags when a bulk upsert hits a bad record (#134)#146
coreyphillips wants to merge 1 commit into
masterfrom
issue-134

Conversation

@coreyphillips

Copy link
Copy Markdown
Collaborator

Closes #134

Upsert_tags now skips and reports unwritable records instead of aborting the batch, returning UpsertTagsResult { inserted, skipped }.

ActivityDB::upsert_tags ran the whole batch in one transaction and returned Err on the first record it could not write, so a single tag whose parent activity was missing (FOREIGN KEY constraint failed) rolled back every other tag in the call. An empty activity ID or wallet ID did the same, returning before the transaction even reached the remaining records. On the app side that turned one orphan tag into a failed restore of the entire activity/tags/closed-channels category, and it forced every consumer to write a per-record loop just to get resilience.

What changed

  • upsert_tags in src/modules/activity/implementation.rs now continues past a record it cannot write: an empty activity ID, an empty wallet ID, an empty tag, or a failing INSERT (a constraint violation rolls back only its own statement, so the surrounding transaction still commits the rest).
  • It returns UpsertTagsResult { inserted, skipped } instead of (); inserted counts tags persisted (including ones already present, so idempotent re-runs report the same count), and each SkippedTag in skipped carries wallet ID, activity ID, the offending tag (None when the whole record was rejected before any tag was tried) and the reason.
  • New SkippedTag and UpsertTagsResult UniFFI records in src/modules/activity/types.rs, re-exported from src/lib.rs; the upsert_tags FFI export returns the result type.
  • upsert_tags with only bad records is no longer an error: test_upsert_tags_empty_activity_id now asserts the record comes back in skipped rather than as Err.

How to test

  • cargo test modules::activity, 190 passed, including the new test_upsert_tags_keeps_valid_records_when_one_is_unusable, which sends a batch of valid record / missing parent activity / empty activity ID / empty wallet ID / valid record and asserts both valid records are persisted and all three failures are reported with their reasons.
  • cargo test, 494 passed, 11 failed. All 11 failures are the blocktank tests that call the live staging API (api.stag.blocktank.to) and regtest; they fail identically without this change because the sandbox has no network.
  • cargo clippy --all-targets, no new warnings from the touched files.

Notes

Generated bindings under bindings/ are not regenerated here: this repo does that in a separate version-bump commit via build.sh -r (needs Xcode/NDK), so Swift/Kotlin/Python callers pick up the new return type on the next release build. The signature change is source-compatible for callers that ignored the old empty return, though Swift will warn about the unused result. Empty tags, previously skipped silently, are now reported in skipped too, on the grounds that "which records were skipped and why" should not have a silent exception.

upsert_tags aborted the whole batch on the first record it could not
write, so a single tag whose parent activity was missing discarded every
other tag in the call (and, for the apps, the rest of the restore
category with it).

Bad records are now skipped and reported instead: upsert_tags returns
UpsertTagsResult { inserted, skipped }, where each SkippedTag carries the
wallet ID, activity ID, the offending tag (if any) and the reason. Only
batch-level failures (transaction, statement or commit) still return an
error. Closes #134.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: keep valid tags when a bulk upsert hits a bad record

1 participant