diff --git a/core/dr-catalog/src/merge.rs b/core/dr-catalog/src/merge.rs index 710553b..7a1e50c 100644 --- a/core/dr-catalog/src/merge.rs +++ b/core/dr-catalog/src/merge.rs @@ -1,5 +1,5 @@ -//! TRACES: FR-CAT-7 | FR-NC-9 -//! Merging a remote catalog's collections into the local one. +//! TRACES: FR-CAT-7 | FR-CAT-5 | FR-NC-9 +//! Merging a remote catalog's collections and keywords into the local one. //! //! # Why this is a merge and not a copy //! @@ -33,6 +33,35 @@ //! against a device that still holds it would otherwise resurrect it. The //! tombstone carries a revision like any other edit, so deletion competes on //! the same footing as a rename. +//! +//! # Keywords merge on the same three rules +//! +//! [`merge_keywords`] reuses all of the above rather than inventing a second +//! set of rules, because a keyword is the same shape of problem as a +//! collection: a named thing with a device-independent identity, and a +//! many-to-many join to images. +//! +//! - The **vocabulary** (`keyword_terms`) is decided per row by [`verdict`], +//! exactly as collections are. +//! - The **assignments** (`keywords`) are a set union, exactly as membership +//! is: two devices each keywording different photographs "puffin" keep both +//! sets, and two devices each keywording the *same* photograph converge on +//! one row rather than one of them winning. +//! - **Deletion** tombstones, and takes the assignments with it. +//! +//! Two things are genuinely different, and both are consequences of assignments +//! storing the *word* rather than a row id: +//! +//! 1. A tombstone deletes assignments **by name**, so a deletion still lands on +//! a device that had minted its own identity for the same word. The union +//! then refuses to readmit a word a winning tombstone has just removed — +//! without that filter, the other device's live assignments would resurrect +//! it on the very same pass. +//! 2. Two devices that independently typed the same word arrive with two uuids +//! for one keyword. [`crate::keywords::fuse_duplicates`] collapses them onto +//! the lexicographically smaller one, which both devices compute identically. +//! A unique index on the name would instead abort the merge transaction at +//! that moment, which is the ordinary case rather than a corner one. use rusqlite::Connection; @@ -59,18 +88,44 @@ pub struct MergeReport { pub kept_local: usize, pub deleted: usize, pub members_added: usize, + + // Keywords are counted separately from collections rather than summed into + // the same fields. The report is shown to the user — "3 collections, 11 + // keywords" is a sentence; "14 things" is not — and a merge that went wrong + // is far easier to place when the counts say which half it went wrong in. + /// Keywords the remote had and this device did not. + pub keywords_inserted: usize, + /// Keywords the remote had renamed, or brought back from a tombstone. + pub keywords_updated: usize, + /// Keywords the remote deleted, and this device has now deleted too. + pub keywords_deleted: usize, + /// Keywords where this device's revision was at least as high. + pub keywords_kept_local: usize, + /// Redundant identities for one word, retired by + /// [`crate::keywords::fuse_duplicates`]. + pub keywords_fused: usize, + /// Keyword assignments taken from the remote. + pub keywords_assigned: usize, } impl MergeReport { /// Whether the local catalog changed, and so needs re-uploading. pub fn local_changed(&self) -> bool { - self.inserted > 0 || self.updated > 0 || self.deleted > 0 || self.members_added > 0 + self.inserted > 0 + || self.updated > 0 + || self.deleted > 0 + || self.members_added > 0 + || self.keywords_inserted > 0 + || self.keywords_updated > 0 + || self.keywords_deleted > 0 + || self.keywords_fused > 0 + || self.keywords_assigned > 0 } /// Whether the local catalog holds anything the remote did not, and so /// must be uploaded even if nothing was taken from the remote. pub fn should_upload(&self) -> bool { - self.kept_local > 0 || self.local_changed() + self.kept_local > 0 || self.keywords_kept_local > 0 || self.local_changed() } } @@ -108,16 +163,55 @@ pub fn verdict( } } -/// Merge collections and membership from an attached catalog. +/// Merge everything that syncs, from an attached catalog. /// /// The remote catalog must already be attached under the schema name -/// `remote_cat`; [`crate::Catalog::merge_attached_collections`] handles that. +/// `remote_cat`; [`crate::sync::merge_remote`] handles that. +/// +/// **One transaction over both halves.** Keywords and collections are +/// independent as data, but a merge that landed the collections and then failed +/// on the keywords would leave a catalog that has already taken the remote's +/// revisions for half of itself — and the next attempt, seeing those revisions, +/// would decline to take them again. Half a merge is not a state that can be +/// resumed, so it is not a state that can be reached. +pub fn merge_all(conn: &Connection) -> Result { + let tx = conn.unchecked_transaction()?; + let mut report = MergeReport::default(); + merge_collections_within(&tx, &mut report)?; + merge_keywords_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} + +/// Merge collections and membership from an attached catalog. +/// +/// The collections half of [`merge_all`], on its own. Kept as a public entry +/// point because the two halves are genuinely independent, and because the +/// rules for this one are worth being able to exercise without a keyword in +/// sight. /// /// Runs in one transaction: a merge either lands whole or not at all. pub fn merge_collections(conn: &Connection) -> Result { let tx = conn.unchecked_transaction()?; let mut report = MergeReport::default(); + merge_collections_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} +/// Merge the keyword vocabulary and its assignments from an attached catalog. +/// +/// The keywords half of [`merge_all`], on its own. See the module header for +/// the three rules and the two places keywords differ from collections. +pub fn merge_keywords(conn: &Connection) -> Result { + let tx = conn.unchecked_transaction()?; + let mut report = MergeReport::default(); + merge_keywords_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} + +fn merge_collections_within(tx: &Connection, report: &mut MergeReport) -> Result<(), CatalogError> { // ---- collections ------------------------------------------------------ { let mut stmt = tx.prepare( @@ -310,8 +404,245 @@ pub fn merge_collections(conn: &Connection) -> Result )?; report.members_added = added; - tx.commit()?; - Ok(report) + Ok(()) +} + +/// Schema name the downloaded remote catalog is attached under. +/// +/// Repeated from [`crate::sync`] rather than shared, because the SQL below +/// spells it inline and a constant that only half the file used would be worse +/// than no constant at all. +const REMOTE: &str = "remote_cat"; + +/// The keyword half. See the module header. +fn merge_keywords_within(tx: &Connection, report: &mut MergeReport) -> Result<(), CatalogError> { + // ---- the vocabulary --------------------------------------------------- + // + // A remote written before schema v6 has no `keyword_terms` at all, and + // `remote_is_mergeable` deliberately admits it: the check is that the + // remote is not *newer* than us. So the table's absence is a normal state + // and not an error. Its assignments still merge below — those have been in + // the schema since v1 — and its words gain identities on that device the + // next time it opens the catalog and backfills. + if attached_has_table(tx, REMOTE, "keyword_terms")? { + struct Incoming { + uuid: String, + name: String, + /// What this device currently calls the same identity, if it has + /// it. A rename is applied to the assignment rows by rewriting this + /// text, so it has to be read before the term row is overwritten. + local_name: Option, + created: i64, + revision: i64, + modified: i64, + verdict: MergeVerdict, + } + + let rows: Vec = { + let mut stmt = tx.prepare( + "SELECT r.uuid, r.name, r.created, r.revision, r.modified, r.deleted, + l.name, l.revision, l.modified + FROM remote_cat.keyword_terms r + LEFT JOIN main.keyword_terms l ON l.uuid = r.uuid", + )?; + let found = stmt + .query_map([], |r| { + let deleted: i64 = r.get(5)?; + let local_rev: Option = r.get(7)?; + let local_mod: Option = r.get(8)?; + let revision: i64 = r.get(3)?; + let modified: i64 = r.get(4)?; + Ok(Incoming { + uuid: r.get(0)?, + name: r.get(1)?, + local_name: r.get(6)?, + created: r.get(2)?, + revision, + modified, + verdict: verdict( + local_rev.zip(local_mod), + (revision, modified), + deleted != 0, + ), + }) + })? + .collect::, _>>()?; + found + }; + + for row in rows { + match row.verdict { + MergeVerdict::KeptLocal => { + report.keywords_kept_local += 1; + } + MergeVerdict::InsertedFromRemote => { + tx.execute( + "INSERT INTO main.keyword_terms + (uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, ?3, ?4, ?5, 0)", + rusqlite::params![ + row.uuid, + row.name, + row.created, + row.revision, + row.modified, + ], + )?; + report.keywords_inserted += 1; + } + MergeVerdict::UpdatedFromRemote => { + // The assignments carry the *word*, so taking a new name + // for an identity we already hold means rewriting every row + // spelt the old way. Without this the vocabulary would show + // the new spelling and the search would only find the old. + if let Some(old) = row.local_name.filter(|n| *n != row.name) { + tx.execute( + "INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT version_id, ?2 FROM main.keywords WHERE keyword = ?1", + rusqlite::params![old, row.name], + )?; + tx.execute("DELETE FROM main.keywords WHERE keyword = ?1", [&old])?; + } + tx.execute( + "UPDATE main.keyword_terms + SET name = ?2, revision = ?3, modified = ?4, deleted = 0 + WHERE uuid = ?1", + rusqlite::params![row.uuid, row.name, row.revision, row.modified], + )?; + report.keywords_updated += 1; + } + MergeVerdict::DeletedByRemote => { + // Tombstone rather than DELETE, or a third device + // reintroduces the keyword through us. + tx.execute( + "INSERT INTO main.keyword_terms + (uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, ?3, ?4, ?5, 1) + ON CONFLICT(uuid) DO UPDATE SET + deleted = 1, revision = ?4, modified = ?5", + rusqlite::params![ + row.uuid, + row.name, + row.created, + row.revision, + row.modified, + ], + )?; + // **By name, not by identity.** This device may well have + // minted its own uuid for the same word before the two ever + // synced, in which case deleting by uuid would tombstone a + // row that nothing is assigned to and leave every + // photograph still carrying the word. + tx.execute("DELETE FROM main.keywords WHERE keyword = ?1", [&row.name])?; + report.keywords_deleted += 1; + } + } + } + + // Two devices that each typed "Iceland" now hold two identities for one + // word. Collapse them before the assignments arrive, so the vocabulary + // the user sees after a sync has one row per word. + report.keywords_fused = crate::keywords::fuse_duplicates(tx)?; + } + + // ---- assignments ------------------------------------------------------ + // + // Set union, and the union is the whole point: FR-NC-9's principle applied + // to metadata rather than to edit nodes. Two devices that keyworded + // different frames "puffin" both keep their work, and neither loses it to + // whichever synced second. + // + // A removal therefore does not propagate — the remote's assignment simply + // reappears. That is the same trade-off collection membership makes above, + // and for the same reason: an unwanted keyword is removed again in a + // second, and a silently lost afternoon of keywording is not recoverable at + // all. Making removal propagate needs a tombstone per assignment, which is + // a schema change and a merge rule of its own. + // + // An incoming keyword lands on the local default version, so an image that + // has not got one yet would silently drop it. That is not a rare state: the + // invariant is maintained by a backfill on open, and a scan that ran since + // has added rows it has not covered. Losing a word the user typed on + // another device, for a bookkeeping reason, would be the wrong answer — + // this is idempotent and writes nothing once the invariant holds. + crate::rating::ensure_default_versions_within(tx)?; + + // Two passes rather than one statement with an `OR`, because they resolve + // *different identities* for the same photograph and each wants its own + // index. See [`ASSIGN_BY_FILE_ID`] for why there are two at all. + for sql in [ASSIGN_BY_FILE_ID, ASSIGN_BY_CONTENT_HASH] { + report.keywords_assigned += tx.execute(sql, [])?; + } + + Ok(()) +} + +/// Take assignments for images both devices know by the server's file id. +/// +/// **Preferred over the content hash**, and the reason is that `content_hash` +/// is expensive — the schema says so, and it is computed only when import +/// dedup or a reconnect asks for it, which for most libraries is never. Keying +/// keywords on it alone would mean the union quietly did nothing for the +/// ordinary image, which is the exact failure this merge exists to prevent. +/// +/// `oc:fileid` is the opposite: it is recorded for every image the moment a +/// remote scan sees it, it is stable across server-side renames and moves, and +/// it is the same integer on every device pointed at the same Nextcloud — which +/// is precisely the situation where two devices are keywording one library. +/// +/// The keyword lands on the local image's **default version**, not on the +/// version it came from. Version uuids do not reconcile across devices in the +/// catalog: [`crate::rating::ensure_default_versions`] mints a fresh one per +/// device, so the same photograph's default versions have different uuids on +/// two machines and a uuid-keyed join would union nothing at all. Version +/// identity is reconciled in the *sidecar* (FR-NC-8), and until a merged +/// version arrives through there, the default version is both where +/// [`crate::keywords::assign`] writes and where the panel reads — so it is the +/// one place the word can land and be seen. +const ASSIGN_BY_FILE_ID: &str = " + INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT lv.id, rk.keyword + FROM remote_cat.keywords rk + JOIN remote_cat.versions rv ON rv.id = rk.version_id + JOIN remote_cat.remote rr ON rr.image_id = rv.image_id + JOIN main.remote lr ON lr.file_id = rr.file_id + JOIN main.versions lv ON lv.image_id = lr.image_id AND lv.is_default = 1 + WHERE NOT EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 1) + OR EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 0)"; + +/// The same union for a library with no server behind it. +/// +/// A local-only library has no `remote` rows at all, so [`ASSIGN_BY_FILE_ID`] +/// matches nothing and this is the only identity available — and it is the one +/// collection membership already uses, so a library where membership merges +/// has keywords that merge too. +const ASSIGN_BY_CONTENT_HASH: &str = " + INSERT OR IGNORE INTO main.keywords(version_id, keyword) + SELECT lv.id, rk.keyword + FROM remote_cat.keywords rk + JOIN remote_cat.versions rv ON rv.id = rk.version_id + JOIN remote_cat.images ri ON ri.id = rv.image_id + JOIN main.images li ON li.content_hash = ri.content_hash + JOIN main.versions lv ON lv.image_id = li.id AND lv.is_default = 1 + WHERE ri.content_hash IS NOT NULL + AND (NOT EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 1) + OR EXISTS (SELECT 1 FROM main.keyword_terms t + WHERE t.name = rk.keyword AND t.deleted = 0))"; + +/// Whether an attached database holds a table of this name. +/// +/// The schema name and the table name are both literals from this file, never +/// user text — but they are still bound rather than formatted where SQLite +/// allows it, because the habit is what keeps the one that eventually is user +/// text from being formatted by accident. +fn attached_has_table(conn: &Connection, schema: &str, table: &str) -> Result { + let sql = + format!("SELECT count(*) FROM {schema}.sqlite_master WHERE type = 'table' AND name = ?1"); + let n: i64 = conn.query_row(&sql, [table], |r| r.get(0))?; + Ok(n > 0) } #[cfg(test)] @@ -391,12 +722,21 @@ mod tests { // ---- integration over two real catalogs ------------------------------ fn two_catalogs() -> Connection { + attached_remote(schema::for_attached("remote_cat")) + } + + /// The same pair, but with the remote stopped at v1 — a device running a + /// build from before keywords had identities. + fn two_catalogs_with_a_v1_remote() -> Connection { + attached_remote(schema::v1_for_attached("remote_cat")) + } + + fn attached_remote(remote_schema: String) -> Connection { let c = Connection::open_in_memory().unwrap(); schema::configure(&c).unwrap(); schema::migrate(&c).unwrap(); // A second in-memory database standing in for the downloaded remote. c.execute_batch("ATTACH ':memory:' AS remote_cat").unwrap(); - let remote_schema = super::super::schema::v1_for_attached("remote_cat"); c.execute_batch(&remote_schema).unwrap(); c } @@ -656,6 +996,330 @@ mod tests { assert!(!second.local_changed(), "merge must be idempotent"); } + // ---- keywords -------------------------------------------------------- + + /// Give an image a default version, as every write path assumes it has. + fn add_version(c: &Connection, db: &str, image: i64, uuid: &str) -> i64 { + c.execute( + &format!( + "INSERT INTO {db}.versions(image_id, uuid, name, is_default) + VALUES (?1, ?2, 'Default', 1)" + ), + rusqlite::params![image, uuid], + ) + .unwrap(); + c.last_insert_rowid() + } + + /// Put a word on an image's default version, creating the version. + /// + /// The version uuid is derived from the database *and* the image, so the + /// two catalogs never accidentally agree on one — which is the real + /// situation, and the reason the assignment union cannot key on it. + fn keyword(c: &Connection, db: &str, image: i64, word: &str) { + let existing: Option = c + .query_row( + &format!("SELECT id FROM {db}.versions WHERE image_id = ?1 AND is_default = 1"), + [image], + |r| r.get(0), + ) + .ok(); + let version = + existing.unwrap_or_else(|| add_version(c, db, image, &format!("v-{db}-{image}"))); + c.execute( + &format!("INSERT OR IGNORE INTO {db}.keywords(version_id, keyword) VALUES (?1, ?2)"), + rusqlite::params![version, word], + ) + .unwrap(); + } + + fn add_term(c: &Connection, db: &str, uuid: &str, name: &str, rev: i64, deleted: i64) { + c.execute( + &format!( + "INSERT INTO {db}.keyword_terms(uuid, name, created, revision, modified, deleted) + VALUES (?1, ?2, 0, ?3, ?3, ?4)" + ), + rusqlite::params![uuid, name, rev, deleted], + ) + .unwrap(); + } + + /// Map an image to a server file id, as a remote scan does. + fn add_file_id(c: &Connection, db: &str, image: i64, file_id: i64) { + c.execute( + &format!("INSERT INTO {db}.remote(image_id, file_id) VALUES (?1, ?2)"), + rusqlite::params![image, file_id], + ) + .unwrap(); + } + + /// Every word on an image locally, sorted. + fn words_on(c: &Connection, image: i64) -> Vec { + let mut stmt = c + .prepare( + "SELECT DISTINCT k.keyword FROM main.keywords k + JOIN main.versions v ON v.id = k.version_id + WHERE v.image_id = ?1 ORDER BY k.keyword", + ) + .unwrap(); + let rows = stmt.query_map([image], |r| r.get(0)).unwrap(); + rows.collect::, _>>().unwrap() + } + + fn live_terms(c: &Connection) -> Vec { + let mut stmt = c + .prepare("SELECT name FROM main.keyword_terms WHERE deleted = 0 ORDER BY name") + .unwrap(); + let rows = stmt.query_map([], |r| r.get(0)).unwrap(); + rows.collect::, _>>().unwrap() + } + + #[test] + fn two_devices_keywording_different_photographs_both_survive() { + // FR-NC-9's principle applied to metadata: disjoint work merges to the + // union, and neither device loses an afternoon to whoever synced last. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + add_image(&c, db, 2, "hash-b"); + } + keyword(&c, "main", 1, "puffin"); + keyword(&c, "remote_cat", 2, "gannet"); + + merge_keywords(&c).unwrap(); + + assert_eq!(words_on(&c, 1), ["puffin"]); + assert_eq!(words_on(&c, 2), ["gannet"]); + } + + #[test] + fn two_devices_keywording_one_photograph_keep_both_words() { + // The case the union is really for: the same frame, two different + // words, and last-writer-wins would silently drop one of them. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "main", 1, "puffin"); + keyword(&c, "remote_cat", 1, "Iceland"); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 1); + assert_eq!(words_on(&c, 1), ["Iceland", "puffin"]); + } + + #[test] + fn a_word_both_devices_already_had_is_not_duplicated() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + keyword(&c, db, 1, "puffin"); + } + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 0); + assert_eq!(words_on(&c, 1), ["puffin"]); + } + + #[test] + fn keywords_reach_an_image_the_server_names_but_no_one_has_hashed() { + // `content_hash` is computed only when import dedup or a reconnect asks + // for it, so for most images it is NULL — and a union keyed on it alone + // would quietly do nothing for the ordinary photograph. The file id is + // recorded by every remote scan, which is exactly the situation where + // two devices are keywording one library. + let c = two_catalogs(); + c.execute( + "INSERT INTO main.roots(id, kind, label) VALUES (1, 'remote', 'r')", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO remote_cat.roots(id, kind, label) VALUES (1, 'remote', 'r')", + [], + ) + .unwrap(); + // Different row ids for one photograph, and no hash on either side. + c.execute( + "INSERT INTO main.images(id, root_id, source_ref, added_at) + VALUES (77, 1, 'IMG_1.CR3', 0)", + [], + ) + .unwrap(); + c.execute( + "INSERT INTO remote_cat.images(id, root_id, source_ref, added_at) + VALUES (3, 1, 'IMG_1.CR3', 0)", + [], + ) + .unwrap(); + add_file_id(&c, "main", 77, 9001); + add_file_id(&c, "remote_cat", 3, 9001); + add_version(&c, "main", 77, "v-main"); + keyword(&c, "remote_cat", 3, "puffin"); + + merge_keywords(&c).unwrap(); + assert_eq!(words_on(&c, 77), ["puffin"]); + } + + #[test] + fn keywords_map_across_devices_by_content_hash_where_there_is_no_server() { + // A local-only library has no `remote` rows at all, so the hash is the + // only identity available — and it is the one membership already uses. + let c = two_catalogs(); + add_image(&c, "main", 77, "same-photo"); + add_image(&c, "remote_cat", 3, "same-photo"); + add_version(&c, "main", 77, "v-main"); + keyword(&c, "remote_cat", 3, "puffin"); + + merge_keywords(&c).unwrap(); + assert_eq!(words_on(&c, 77), ["puffin"]); + } + + #[test] + fn the_vocabulary_merges_by_uuid_and_a_skewed_clock_cannot_win() { + let c = two_catalogs(); + add_term(&c, "main", "u-1", "Iceland", 9, 0); + add_term(&c, "remote_cat", "u-1", "iceland", 2, 0); + add_term(&c, "remote_cat", "u-2", "puffin", 1, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_kept_local, 1); + assert_eq!(report.keywords_inserted, 1); + assert_eq!(live_terms(&c), ["Iceland", "puffin"]); + } + + #[test] + fn a_remote_rename_moves_this_device_s_assignments_too() { + // The failure this exists to stop: the vocabulary shows the corrected + // spelling and the search still only finds the old one. + let c = two_catalogs(); + add_image(&c, "main", 1, "hash-a"); + add_term(&c, "main", "u-1", "Icland", 1, 0); + keyword(&c, "main", 1, "Icland"); + add_term(&c, "remote_cat", "u-1", "Iceland", 4, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_updated, 1); + assert_eq!(live_terms(&c), ["Iceland"]); + assert_eq!(words_on(&c, 1), ["Iceland"]); + } + + #[test] + fn a_remote_deletion_takes_the_word_off_every_photograph() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "main", "u-1", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "u-1", "blurry", 5, 1); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_deleted, 1); + assert!(live_terms(&c).is_empty()); + assert!(words_on(&c, 1).is_empty()); + } + + #[test] + fn a_deletion_is_not_undone_by_the_union_on_the_same_pass() { + // The remote deleted the word *and* still carries assignments for it — + // it has not yet had the chance to sweep them, or a third device put + // them there. Without the tombstone filter the union would put the word + // straight back on the photograph the deletion had just cleared. + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "main", "u-1", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "u-1", "blurry", 5, 1); + keyword(&c, "remote_cat", 1, "blurry"); + + merge_keywords(&c).unwrap(); + assert!(words_on(&c, 1).is_empty(), "a deleted keyword came back"); + } + + #[test] + fn a_deletion_lands_even_when_the_two_devices_minted_different_uuids() { + // Both typed "blurry" before they ever synced, so this device's row has + // a uuid the remote has never heard of. Deleting by identity would + // tombstone nothing and leave every photograph still carrying the word. + let c = two_catalogs(); + add_image(&c, "main", 1, "hash-a"); + add_term(&c, "main", "mine", "blurry", 1, 0); + keyword(&c, "main", 1, "blurry"); + add_term(&c, "remote_cat", "theirs", "blurry", 5, 1); + + merge_keywords(&c).unwrap(); + assert!(words_on(&c, 1).is_empty()); + } + + #[test] + fn two_devices_that_typed_one_word_end_up_with_one_keyword() { + // Neither is wrong until they meet, which is why the name carries no + // unique index — a constraint would abort the merge at this moment. + let c = two_catalogs(); + add_term(&c, "main", "zzzz", "Iceland", 3, 0); + add_term(&c, "remote_cat", "aaaa", "Iceland", 1, 0); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_fused, 1); + assert_eq!(live_terms(&c), ["Iceland"]); + + let survivor: String = c + .query_row("SELECT uuid FROM main.keyword_terms", [], |r| r.get(0)) + .unwrap(); + assert_eq!( + survivor, "aaaa", + "both devices must pick the same survivor without asking each other" + ); + } + + #[test] + fn merging_keywords_twice_changes_nothing_the_second_time() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + add_term(&c, "remote_cat", "u-1", "puffin", 1, 0); + keyword(&c, "remote_cat", 1, "puffin"); + + let first = merge_keywords(&c).unwrap(); + assert!(first.local_changed()); + let second = merge_keywords(&c).unwrap(); + assert!(!second.local_changed(), "merge must be idempotent"); + } + + #[test] + fn a_remote_from_before_keyword_identities_still_contributes_its_words() { + // `remote_is_mergeable` admits an older remote on purpose — the check + // is that it is not *newer* than us. A missing table is therefore a + // normal state and must not fail the merge. + let c = two_catalogs_with_a_v1_remote(); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "remote_cat", 1, "puffin"); + + let report = merge_keywords(&c).unwrap(); + assert_eq!(report.keywords_assigned, 1); + assert_eq!(words_on(&c, 1), ["puffin"]); + } + + #[test] + fn merge_all_lands_both_halves() { + let c = two_catalogs(); + add_collection(&c, "remote_cat", 1, "u-coll", "Portugal", 1); + for db in ["main", "remote_cat"] { + add_image(&c, db, 1, "hash-a"); + } + keyword(&c, "remote_cat", 1, "puffin"); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.inserted, 1); + assert_eq!(report.keywords_assigned, 1); + } + #[test] fn keeping_local_still_marks_the_catalog_for_upload() { // We hold something the remote does not, so the remote is stale even diff --git a/core/dr-catalog/src/rating.rs b/core/dr-catalog/src/rating.rs index 1c90595..139acd9 100644 --- a/core/dr-catalog/src/rating.rs +++ b/core/dr-catalog/src/rating.rs @@ -75,6 +75,24 @@ impl Judgement { /// The UUID is per row and generated here — it is the merge identity across /// devices (FR-NC-8), so two images must never share one. pub fn ensure_default_versions(conn: &Connection) -> Result { + // One transaction for the batch. A backfill over a 24k-image library is + // 24k inserts, and per-statement commits would make it minutes rather + // than seconds. + let tx = conn.unchecked_transaction()?; + let n = ensure_default_versions_within(&tx)?; + tx.commit()?; + Ok(n) +} + +/// [`ensure_default_versions`] without opening a transaction. +/// +/// Separate because SQLite has no nested `BEGIN`: [`crate::merge`] needs the +/// invariant restored *inside* the merge transaction — an incoming keyword +/// lands on a default version, so an image without one would silently drop it — +/// and calling the public form there fails at runtime with "cannot start a +/// transaction within a transaction". The same split, for the same reason, as +/// `collections::add_within`. +pub fn ensure_default_versions_within(conn: &Connection) -> Result { let ids: Vec = { let mut stmt = conn.prepare( "SELECT i.id FROM images i @@ -89,12 +107,8 @@ pub fn ensure_default_versions(conn: &Connection) -> Result return Ok(0); } - // One transaction for the batch. A backfill over a 24k-image library is - // 24k inserts, and per-statement commits would make it minutes rather - // than seconds. - let tx = conn.unchecked_transaction()?; { - let mut insert = tx.prepare( + let mut insert = conn.prepare( "INSERT INTO versions(image_id, uuid, name, is_default, rating, flag) VALUES (?1, ?2, ?3, 1, 0, 0)", )?; @@ -102,7 +116,6 @@ pub fn ensure_default_versions(conn: &Connection) -> Result insert.execute(rusqlite::params![id, new_uuid(), DEFAULT_VERSION_NAME])?; } } - tx.commit()?; Ok(ids.len()) } diff --git a/core/dr-catalog/src/sync.rs b/core/dr-catalog/src/sync.rs index 7cabf36..01d302d 100644 --- a/core/dr-catalog/src/sync.rs +++ b/core/dr-catalog/src/sync.rs @@ -16,10 +16,11 @@ //! //! # What is actually synced //! -//! Only collections merge (see [`crate::merge`]). The rest of the catalog is a -//! *local index* of *local* storage — folder mtimes, cache paths, job rows — -//! and copying another device's version of those in would be actively wrong. -//! The remote file is read for its collections and then discarded. +//! Only the *user's judgements about their library* merge: collections, and the +//! keyword vocabulary with its assignments (see [`crate::merge`]). The rest of +//! the catalog is a *local index* of *local* storage — folder mtimes, cache +//! paths, job rows — and copying another device's version of those in would be +//! actively wrong. The remote file is read for those two and then discarded. //! //! This is why the catalog remains disposable in the ARCH §6.12 sense: nothing //! here makes the local database authoritative for anything a rebuild could @@ -96,7 +97,7 @@ pub fn merge_remote(conn: &Connection, remote: &Path) -> Result