From 20945fbe6347795d70ae73520b44b9c1e6ef564c Mon Sep 17 00:00:00 2001 From: coreyphillips Date: Wed, 26 Aug 2026 16:55:42 -0400 Subject: [PATCH] fix: keep valid tags when a bulk upsert hits a bad record 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. --- CHANGELOG.md | 2 + README.md | 4 + src/lib.rs | 4 +- src/modules/activity/README.md | 6 ++ src/modules/activity/implementation.rs | 62 ++++++++++---- src/modules/activity/tests.rs | 114 +++++++++++++++++++++++-- src/modules/activity/types.rs | 21 +++++ 7 files changed, 190 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c871842..fd457a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- `upsert_tags` no longer discards a whole batch when one record cannot be written. Records with an empty activity ID, an empty wallet ID, an empty tag, or a missing parent activity are now skipped and reported, and everything else in the batch is persisted. The function returns `UpsertTagsResult { inserted, skipped }` instead of nothing, 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 errors) still return an `ActivityError`. FFI signature change: callers that ignored the previous empty return keep working, and restore flows can now log which tags were dropped instead of losing the entire category. + - The Android AAR now ships targeted R8 consumer keep rules for the UniFFI/JNA FFI surface, so consuming apps can enable R8 full mode without extra keep rules for this library. - Add a generic hardware-wallet catalog with Foundation Passport support, multipart UR QR encoding and decoding, Passport single-signature account export parsing, and signed PSBT finalization across the UniFFI bindings. - Swap status updates now reconcile against Boltz's REST status whenever a swap is (re)subscribed, both on `boltz_start_swap_updates` and when `boltz_create_reverse_swap` adds a swap to a running stream. A confirmed reverse-swap lockup is therefore caught up and auto-claimed even when its live WebSocket event was missed (for example because the updates stream was down while the lockup confirmed), instead of the swap silently stalling until a manual claim. No FFI signature change. diff --git a/README.md b/README.md index 7df8a2e7..ae7f3e7f 100644 --- a/README.md +++ b/README.md @@ -180,6 +180,10 @@ ```rust fn get_all_unique_tags() -> Result, ActivityError> ``` + - [upsert_tags](src/modules/activity/README.md#usage-examples): Bulk insert tags, skipping and reporting records that cannot be written + ```rust + fn upsert_tags(activity_tags: Vec) -> Result + ``` - [get_activities_by_tag](src/modules/activity/README.md#usage-examples): Get activities with a specific tag and optional wallet scope ```rust fn get_activities_by_tag( diff --git a/src/lib.rs b/src/lib.rs index 0fddfce5..fac6efd1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -30,7 +30,7 @@ use crate::activity::{ derive_wallet_id as derive_wallet_id_inner, Activity, ActivityDB, ActivityError, ActivityFilter, ActivityTags, ClosedChannelDetails, DbError, LightningActivity, OnchainActivity, PaymentType, PreActivityMetadata, SortDirection, TransactionDetails, - DEFAULT_WALLET_ID, + UpsertTagsResult, DEFAULT_WALLET_ID, }; use crate::modules::blocktank::{ BlocktankDB, BlocktankError, BtOrderState2, CJitStateEnum, ChannelLiquidityOptions, @@ -753,7 +753,7 @@ pub fn get_all_activities_tags() -> Result, ActivityError> { } #[uniffi::export] -pub fn upsert_tags(activity_tags: Vec) -> Result<(), ActivityError> { +pub fn upsert_tags(activity_tags: Vec) -> Result { let mut guard = get_activity_db()?; let db = guard .activity_db diff --git a/src/modules/activity/README.md b/src/modules/activity/README.md index 7c3ad6df..4dba12f8 100644 --- a/src/modules/activity/README.md +++ b/src/modules/activity/README.md @@ -72,6 +72,12 @@ fn remove_tags(wallet_id: String, activity_id: String, tags: Vec) -> Res fn get_tags(wallet_id: String, activity_id: String) -> Result, ActivityError> fn get_all_unique_tags() -> Result, ActivityError> +// Bulk insert tags for multiple activities. Records that cannot be written (empty activity ID, +// empty wallet ID, empty tag, or a tag whose parent activity is missing) are skipped and +// reported in `UpsertTagsResult.skipped` instead of failing the batch. +fn upsert_tags(activity_tags: Vec) -> Result +fn get_all_activities_tags() -> Result, ActivityError> + // Pre-activity metadata fn add_pre_activity_metadata(pre_activity_metadata: PreActivityMetadata) -> Result<(), ActivityError> fn add_pre_activity_metadata_tags(wallet_id: String, payment_id: String, tags: Vec) -> Result<(), ActivityError> diff --git a/src/modules/activity/implementation.rs b/src/modules/activity/implementation.rs index f85cdb62..e9bce301 100644 --- a/src/modules/activity/implementation.rs +++ b/src/modules/activity/implementation.rs @@ -1,7 +1,7 @@ use crate::activity::{ Activity, ActivityError, ActivityFilter, ActivityTags, ClosedChannelDetails, LightningActivity, - OnchainActivity, PaymentState, PaymentType, PreActivityMetadata, SortDirection, - TransactionDetails, TxInput, TxOutput, + OnchainActivity, PaymentState, PaymentType, PreActivityMetadata, SkippedTag, SortDirection, + TransactionDetails, TxInput, TxOutput, UpsertTagsResult, }; use rusqlite::{Connection, OptionalExtension}; use serde_json; @@ -2202,10 +2202,21 @@ impl ActivityDB { Ok(result) } - /// Bulk upsert tags for multiple activities - pub fn upsert_tags(&mut self, activity_tags: &[ActivityTags]) -> Result<(), ActivityError> { + /// Bulk upsert tags for multiple activities. + /// + /// Records that cannot be written (empty activity ID, empty wallet ID, empty tag, or a + /// tag whose parent activity is missing) are skipped and reported in the result instead + /// of failing the batch, so one unusable record cannot discard the rest. Only failures + /// affecting the batch as a whole (transaction, statement or commit failures) return an + /// error. + pub fn upsert_tags( + &mut self, + activity_tags: &[ActivityTags], + ) -> Result { + let mut result = UpsertTagsResult::default(); + if activity_tags.is_empty() { - return Ok(()); + return Ok(result); } let tx = self @@ -2226,25 +2237,44 @@ impl ActivityDB { })?; for activity_tag in activity_tags { - if activity_tag.activity_id.is_empty() { - return Err(ActivityError::DataError { - error_details: "Activity ID cannot be empty".to_string(), + let mut skip_record = |tag: Option<&String>, reason: String| { + result.skipped.push(SkippedTag { + wallet_id: activity_tag.wallet_id.clone(), + activity_id: activity_tag.activity_id.clone(), + tag: tag.cloned(), + reason, }); + }; + + if activity_tag.activity_id.is_empty() { + skip_record(None, "Activity ID cannot be empty".to_string()); + continue; } - let wallet_id = Self::normalize_wallet_id(&activity_tag.wallet_id)?; + + let wallet_id = match Self::normalize_wallet_id(&activity_tag.wallet_id) { + Ok(wallet_id) => wallet_id, + Err(e) => { + skip_record(None, e.to_string()); + continue; + } + }; for tag in &activity_tag.tags { if tag.is_empty() { - continue; // Skip empty tags + skip_record(Some(tag), "Tag cannot be empty".to_string()); + continue; } - stmt.execute(rusqlite::params![ + + // A failing statement rolls back only itself, so the rest of the batch + // still commits. + match stmt.execute(rusqlite::params![ &wallet_id, &activity_tag.activity_id, tag - ]) - .map_err(|e| ActivityError::DataError { - error_details: format!("Failed to insert tag: {}", e), - })?; + ]) { + Ok(_) => result.inserted += 1, + Err(e) => skip_record(Some(tag), format!("Failed to insert tag: {}", e)), + } } } } @@ -2253,7 +2283,7 @@ impl ActivityDB { error_details: format!("Failed to commit transaction: {}", e), })?; - Ok(()) + Ok(result) } /// Add pre-activity metadata for an onchain address or lightning invoice. diff --git a/src/modules/activity/tests.rs b/src/modules/activity/tests.rs index f9a1ff5f..319c378c 100644 --- a/src/modules/activity/tests.rs +++ b/src/modules/activity/tests.rs @@ -3118,10 +3118,14 @@ mod tests { activity_id: activity.id.clone(), tags: vec!["payment".to_string(), "coffee".to_string()], }]; - assert!(db.upsert_tags(&activity_tags).is_ok()); + let result = db.upsert_tags(&activity_tags).unwrap(); + assert_eq!(result.inserted, 2); + assert!(result.skipped.is_empty()); // Second upsert with same tags (should be idempotent) - assert!(db.upsert_tags(&activity_tags).is_ok()); + let result = db.upsert_tags(&activity_tags).unwrap(); + assert_eq!(result.inserted, 2); + assert!(result.skipped.is_empty()); // Verify tags are still there and not duplicated let tags = db.get_tags(DEFAULT_WALLET_ID, &activity.id).unwrap(); @@ -3178,7 +3182,11 @@ mod tests { activity_id: activity.id.clone(), tags: vec!["payment".to_string(), "".to_string(), "coffee".to_string()], }]; - assert!(db.upsert_tags(&activity_tags).is_ok()); + let result = db.upsert_tags(&activity_tags).unwrap(); + assert_eq!(result.inserted, 2); + assert_eq!(result.skipped.len(), 1); + assert_eq!(result.skipped[0].tag, Some("".to_string())); + assert_eq!(result.skipped[0].reason, "Tag cannot be empty"); // Verify only non-empty tags were added let tags = db.get_tags(DEFAULT_WALLET_ID, &activity.id).unwrap(); @@ -3398,7 +3406,9 @@ mod tests { let (mut db, db_path) = setup(); // Test with empty vector - assert!(db.upsert_tags(&[]).is_ok()); + let result = db.upsert_tags(&[]).unwrap(); + assert_eq!(result.inserted, 0); + assert!(result.skipped.is_empty()); cleanup(&db_path); } @@ -3414,7 +3424,101 @@ mod tests { tags: vec!["payment".to_string()], }]; - assert!(db.upsert_tags(&activity_tags).is_err()); + let result = db.upsert_tags(&activity_tags).unwrap(); + assert_eq!(result.inserted, 0); + assert_eq!(result.skipped.len(), 1); + assert_eq!(result.skipped[0].activity_id, ""); + assert_eq!(result.skipped[0].tag, None); + assert_eq!(result.skipped[0].reason, "Activity ID cannot be empty"); + + cleanup(&db_path); + } + + #[test] + fn test_upsert_tags_keeps_valid_records_when_one_is_unusable() { + let (mut db, db_path) = setup(); + + let activity = create_test_onchain_activity(); + let mut other_activity = create_test_onchain_activity(); + other_activity.id = "test_onchain_2".to_string(); + other_activity.tx_id = "test_onchain_2_txid".to_string(); + + db.insert_onchain_activity(&activity).unwrap(); + db.insert_onchain_activity(&other_activity).unwrap(); + + // A batch where the second record has no parent activity, the third has an empty + // activity ID and the fourth has an empty wallet ID. + let activity_tags = vec![ + ActivityTags { + wallet_id: DEFAULT_WALLET_ID.to_string(), + activity_id: activity.id.clone(), + tags: vec!["payment".to_string(), "coffee".to_string()], + }, + ActivityTags { + wallet_id: DEFAULT_WALLET_ID.to_string(), + activity_id: "missing_activity".to_string(), + tags: vec!["orphan".to_string()], + }, + ActivityTags { + wallet_id: DEFAULT_WALLET_ID.to_string(), + activity_id: "".to_string(), + tags: vec!["nameless".to_string()], + }, + ActivityTags { + wallet_id: " ".to_string(), + activity_id: activity.id.clone(), + tags: vec!["walletless".to_string()], + }, + ActivityTags { + wallet_id: DEFAULT_WALLET_ID.to_string(), + activity_id: other_activity.id.clone(), + tags: vec!["food".to_string()], + }, + ]; + + let result = db.upsert_tags(&activity_tags).unwrap(); + + // The valid records on both sides of the bad ones are persisted. + assert_eq!(result.inserted, 3); + let mut tags = db.get_tags(DEFAULT_WALLET_ID, &activity.id).unwrap(); + tags.sort(); + assert_eq!(tags, vec!["coffee".to_string(), "payment".to_string()]); + assert_eq!( + db.get_tags(DEFAULT_WALLET_ID, &other_activity.id).unwrap(), + vec!["food".to_string()] + ); + + // And the caller can tell which records were skipped and why. + assert_eq!(result.skipped.len(), 3); + + let orphan = &result.skipped[0]; + assert_eq!(orphan.activity_id, "missing_activity"); + assert_eq!(orphan.tag, Some("orphan".to_string())); + assert!( + orphan.reason.contains("FOREIGN KEY constraint failed"), + "unexpected reason: {}", + orphan.reason + ); + + let nameless = &result.skipped[1]; + assert_eq!(nameless.activity_id, ""); + assert_eq!(nameless.tag, None); + assert_eq!(nameless.reason, "Activity ID cannot be empty"); + + let walletless = &result.skipped[2]; + assert_eq!(walletless.wallet_id, " "); + assert_eq!(walletless.tag, None); + assert!( + walletless.reason.contains("Wallet ID cannot be empty"), + "unexpected reason: {}", + walletless.reason + ); + + assert!(db + .get_all_unique_tags() + .unwrap() + .iter() + .all(|tag| tag != "orphan" && tag != "nameless" && tag != "walletless")); cleanup(&db_path); } diff --git a/src/modules/activity/types.rs b/src/modules/activity/types.rs index 29418497..e271b2e5 100644 --- a/src/modules/activity/types.rs +++ b/src/modules/activity/types.rs @@ -222,6 +222,27 @@ pub struct ActivityTags { pub tags: Vec, } +/// A tag record that a bulk upsert could not write. +#[derive(Debug, Clone, uniffi::Record, Serialize, Deserialize)] +pub struct SkippedTag { + pub wallet_id: String, + pub activity_id: String, + /// The individual tag that failed, or `None` when the whole record was + /// rejected before any of its tags were attempted. + pub tag: Option, + /// Why the record was skipped. + pub reason: String, +} + +/// Outcome of a bulk tag upsert. +#[derive(Debug, Clone, Default, uniffi::Record, Serialize, Deserialize)] +pub struct UpsertTagsResult { + /// Number of tags persisted, including tags that were already present. + pub inserted: u32, + /// Records that could not be written, with the reason for each. + pub skipped: Vec, +} + #[derive(Debug, Clone, uniffi::Record, Serialize, Deserialize)] pub struct PreActivityMetadata { #[serde(default = "default_wallet_id")]