diff --git a/core/dr-catalog/src/merge.rs b/core/dr-catalog/src/merge.rs index 08cf47b..cfcb71f 100644 --- a/core/dr-catalog/src/merge.rs +++ b/core/dr-catalog/src/merge.rs @@ -63,7 +63,7 @@ //! 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; +use rusqlite::{Connection, OptionalExtension}; use crate::error::CatalogError; @@ -101,6 +101,19 @@ pub struct MergeReport { pub keywords_deleted: usize, /// Keywords where this device's revision was at least as high. pub keywords_kept_local: usize, + /// People the remote had and this device did not. + pub people_inserted: usize, + /// People the remote had renamed, set aside, or brought back. + pub people_updated: usize, + /// People where this device's revision was at least as high. + pub people_kept_local: usize, + /// Faces this device now agrees belong to somebody. + pub faces_assigned: usize, + /// Faces whose local confirmation outranked the remote's. + pub faces_kept_local: usize, + /// "Not this person" judgements taken from the remote. + pub faces_rejected: usize, + /// Redundant identities for one word, retired by /// [`crate::keywords::fuse_duplicates`]. pub keywords_fused: usize, @@ -179,6 +192,19 @@ pub fn merge_all(conn: &Connection) -> Result { let mut report = MergeReport::default(); merge_collections_within(&tx, &mut report)?; merge_keywords_within(&tx, &mut report)?; + merge_people_within(&tx, &mut report)?; + tx.commit()?; + Ok(report) +} + +/// Merge people and identity judgements from an attached catalog. +/// +/// The people half of [`merge_all`], on its own, for the same reason the other +/// two have one: the rules are independent and worth exercising alone. +pub fn merge_people(conn: &Connection) -> Result { + let tx = conn.unchecked_transaction()?; + let mut report = MergeReport::default(); + merge_people_within(&tx, &mut report)?; tx.commit()?; Ok(report) } @@ -685,6 +711,381 @@ fn attached_has_table(conn: &Connection, schema: &str, table: &str) -> Result 0) } +// ── people, and who the user said they are ──────────────────────────────── + +/// Merge people and the user's identity judgements from an attached catalog. +/// +/// # Why this is here at all +/// +/// Face *data* syncs as sealed shards ([`crate::face_shard`]) — boxes, +/// landmarks, embeddings, the run marker. What the shards deliberately do not +/// carry is who anybody **is**: the person rows, their names, and the +/// assignments joining the two. Those were supposed to travel in the catalog +/// snapshot, which is a whole-file copy and therefore does contain them — but +/// the snapshot is *merged*, not adopted, and this merge only ever looked at +/// collections and keywords. So a second device received every face and no +/// people at all, and drew an empty People screen over a full catalog. +/// +/// # What travels, and what is recomputed +/// +/// The rule this module already follows for the rest of the catalog: user +/// judgements travel, inference is rebuilt. Concretely (docs/faces.md, and the +/// asymmetry `crate::faces` opens with): +/// +/// - **People** — uuid, name, and whether the user set them aside. Merged by +/// uuid on `revision`, exactly as a collection is. +/// - **Confirmations** — the user said this face is this person. +/// - **Rejections** — the user said it is *not*, which is equally a fact and +/// is why re-clustering does not put it back. +/// - **The assignments inside an ignored group** — carried even though they are +/// only suggestions, because they are what anchors the ignore. Without them +/// a group set aside on one device reappears on the other, which is the same +/// fault that made "Not interested" not stick locally. +/// +/// Ordinary suggestions are *not* carried. They are this pass's own output, +/// clustering is deterministic, and both devices hold the same embeddings — so +/// each recomputes them and arrives at the same answer. Shipping them would +/// double the merge for no new information. +/// +/// # Faces have no cross-device identity, so one is derived +/// +/// `faces.id` is a local row id and means nothing in another catalog; there is +/// no uuid to fall back on. What both devices *do* agree on is `oc:fileid` and +/// the box, so a remote face is matched to the local face on the same +/// photograph whose box overlaps it most, above a floor of 0.5 IoU. +/// +/// That is not a new rule: it is the one +/// [`crate::faces::record_detections`] already uses to carry a confirmation +/// across a re-index, and it is loose on purpose — the question is "is this the +/// same face in the frame", not "is this the same rectangle", and a device +/// running a newer detector is entitled to have moved the box a little. +fn merge_people_within(tx: &Connection, report: &mut MergeReport) -> Result<(), CatalogError> { + // A remote written before faces existed has none of these tables, and one + // written before V10 has no `ignored`. Both are ordinary — `remote_is_ + // mergeable` admits any catalog at or below this schema version — so they + // are probed for rather than assumed, and an absent one skips this half + // instead of aborting a merge that would otherwise have succeeded. + if !remote_has(tx, "people")? || !remote_has(tx, "faces")? { + return Ok(()); + } + let ignored_col = remote_has_column(tx, "people", "ignored")?; + // Spelled into the SQL rather than branched around it: the two queries + // below would otherwise each need a second copy. + let ignored_sel = if ignored_col { "r.ignored" } else { "0" }; + + // ---- the people themselves ------------------------------------------- + struct Incoming { + uuid: String, + name: String, + ignored: bool, + created: i64, + revision: i64, + modified: i64, + /// Resolved after every person exists, since a merge target can arrive + /// after the person redirecting to it. + merged_into_uuid: Option, + verdict: MergeVerdict, + } + + let rows: Vec = { + let mut stmt = tx.prepare(&format!( + "SELECT r.uuid, r.name, {ignored_sel}, r.created, r.revision, r.modified, + rm.uuid, l.revision, l.modified + FROM remote_cat.people r + LEFT JOIN main.people l ON l.uuid = r.uuid + LEFT JOIN remote_cat.people rm ON rm.id = r.merged_into" + ))?; + let mapped = stmt.query_map([], |r| { + let revision: i64 = r.get(4)?; + let modified: i64 = r.get(5)?; + let local_rev: Option = r.get(7)?; + let local_mod: Option = r.get(8)?; + Ok(Incoming { + uuid: r.get(0)?, + name: r.get(1)?, + ignored: r.get(2)?, + created: r.get(3)?, + revision, + modified, + merged_into_uuid: r.get(6)?, + // People have no tombstone: a person is merged away rather + // than deleted, and `merged_into` is that redirect. + verdict: verdict(local_rev.zip(local_mod), (revision, modified), false), + }) + })?; + mapped.collect::>()? + }; + + for p in &rows { + match p.verdict { + MergeVerdict::InsertedFromRemote => { + tx.execute( + "INSERT INTO people (uuid, name, ignored, created, revision, modified) + VALUES (?1, ?2, ?3, ?4, ?5, ?6)", + rusqlite::params![p.uuid, p.name, p.ignored, p.created, p.revision, p.modified], + )?; + report.people_inserted += 1; + } + MergeVerdict::UpdatedFromRemote | MergeVerdict::DeletedByRemote => { + tx.execute( + "UPDATE people + SET name = ?2, ignored = ?3, revision = ?4, modified = ?5 + WHERE uuid = ?1", + rusqlite::params![p.uuid, p.name, p.ignored, p.revision, p.modified], + )?; + report.people_updated += 1; + } + MergeVerdict::KeptLocal => report.people_kept_local += 1, + } + } + + // Redirects, once every person on both sides exists locally. + for p in rows.iter().filter(|p| p.merged_into_uuid.is_some()) { + if p.verdict == MergeVerdict::KeptLocal { + continue; + } + tx.execute( + "UPDATE people + SET merged_into = (SELECT id FROM people WHERE uuid = ?2) + WHERE uuid = ?1", + rusqlite::params![p.uuid, p.merged_into_uuid], + )?; + } + + // ---- match the remote's faces onto this device's ---------------------- + let face_map = match_faces(tx)?; + if face_map.is_empty() { + return Ok(()); + } + + // ---- confirmations, and the anchors under an ignored group ----------- + { + let mut stmt = tx.prepare(&format!( + "SELECT fp.face_id, p.uuid, fp.probability, fp.confirmed + FROM remote_cat.face_person fp + JOIN remote_cat.people p ON p.id = fp.person_id + WHERE fp.confirmed = 1 OR {} = 1", + if ignored_col { "p.ignored" } else { "0" } + ))?; + let incoming: Vec<(i64, String, f64, bool)> = stmt + .query_map([], |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?)))? + .collect::>()?; + + for (remote_face, uuid, probability, confirmed) in incoming { + let Some(&local_face) = face_map.get(&remote_face) else { + continue; + }; + let person: Option = tx + .query_row("SELECT id FROM people WHERE uuid = ?1", [&uuid], |r| { + r.get(0) + }) + .optional()?; + let Some(person) = person else { continue }; + + // A local confirmation is never overwritten, in either direction. + // Two devices confirming the same face as different people is a + // genuine disagreement and there is no revision on an assignment to + // settle it with; silently taking the remote's answer would let a + // sync undo something the user did here. It stays as it is, and the + // user can change it on the device they are looking at. + let locally_confirmed: bool = tx.query_row( + "SELECT EXISTS(SELECT 1 FROM face_person + WHERE face_id = ?1 AND confirmed = 1)", + [local_face], + |r| r.get(0), + )?; + if locally_confirmed { + report.faces_kept_local += 1; + continue; + } + + // A rejection here outranks an assignment from elsewhere: it is + // this user's judgement about this pair, and re-suggesting what + // they pushed away is the behaviour that makes the feature feel + // broken. + let rejected: bool = tx.query_row( + "SELECT EXISTS(SELECT 1 FROM face_person_rejected + WHERE face_id = ?1 AND person_id = ?2)", + [local_face, person], + |r| r.get(0), + )?; + if rejected { + continue; + } + + tx.execute( + "INSERT INTO face_person (face_id, person_id, probability, confirmed) + VALUES (?1, ?2, ?3, ?4) + ON CONFLICT(face_id) DO UPDATE SET + person_id = excluded.person_id, + probability = excluded.probability, + confirmed = excluded.confirmed", + rusqlite::params![local_face, person, probability, confirmed], + )?; + report.faces_assigned += 1; + } + } + + // ---- rejections, as a set union -------------------------------------- + { + let mut stmt = tx.prepare( + "SELECT fr.face_id, p.uuid + FROM remote_cat.face_person_rejected fr + JOIN remote_cat.people p ON p.id = fr.person_id", + )?; + let incoming: Vec<(i64, String)> = stmt + .query_map([], |r| Ok((r.get(0)?, r.get(1)?)))? + .collect::>()?; + + for (remote_face, uuid) in incoming { + let Some(&local_face) = face_map.get(&remote_face) else { + continue; + }; + let person: Option = tx + .query_row("SELECT id FROM people WHERE uuid = ?1", [&uuid], |r| { + r.get(0) + }) + .optional()?; + let Some(person) = person else { continue }; + + let n = tx.execute( + "INSERT OR IGNORE INTO face_person_rejected (face_id, person_id) + VALUES (?1, ?2)", + [local_face, person], + )?; + report.faces_rejected += n; + + // A rejection that lands on a face currently *suggested* to be + // that person has to take the suggestion with it, or the screen + // keeps offering exactly what the other device just refused. + tx.execute( + "DELETE FROM face_person + WHERE face_id = ?1 AND person_id = ?2 AND confirmed = 0", + [local_face, person], + )?; + } + } + + Ok(()) +} + +/// Whether the attached remote holds a table. +fn remote_has(tx: &Connection, table: &str) -> Result { + Ok(tx.query_row( + "SELECT EXISTS(SELECT 1 FROM remote_cat.sqlite_master + WHERE type = 'table' AND name = ?1)", + [table], + |r| r.get::<_, bool>(0), + )?) +} + +/// Whether a table in the attached remote holds a column. +fn remote_has_column(tx: &Connection, table: &str, column: &str) -> Result { + // `pragma_table_info` takes the schema as a second argument, which is the + // only way to ask about an attached database rather than the main one. + let mut stmt = + tx.prepare("SELECT 1 FROM pragma_table_info(?1, 'remote_cat') WHERE name = ?2")?; + Ok(stmt.exists(rusqlite::params![table, column])?) +} + +/// Remote face row id to local face row id, by photograph and box overlap. +/// +/// See [`merge_people_within`] for why a face has no shared identity and this +/// has to be derived. Only faces from the same model are compared: boxes from +/// two different detectors are not the same measurement, and matching across +/// them would attach a judgement to a face nobody looked at. +fn match_faces(tx: &Connection) -> Result, CatalogError> { + /// Loose on purpose — "the same face in the frame", not "the same + /// rectangle". The figure `record_detections` uses for the same job. + const MIN_IOU: f32 = 0.5; + + type Boxed = (i64, f32, f32, f32, f32); + + // Local faces, grouped by the photograph's cross-device id. + let mut local: std::collections::HashMap<(i64, String), Vec> = + std::collections::HashMap::new(); + { + let mut stmt = tx.prepare( + "SELECT f.id, r.file_id, f.model_id, f.x, f.y, f.w, f.h + FROM main.faces f + JOIN main.remote r ON r.image_id = f.image_id + WHERE r.file_id IS NOT NULL", + )?; + let rows = stmt.query_map([], |r| { + Ok(( + r.get::<_, i64>(1)?, + r.get::<_, String>(2)?, + ( + r.get::<_, i64>(0)?, + r.get::<_, f64>(3)? as f32, + r.get::<_, f64>(4)? as f32, + r.get::<_, f64>(5)? as f32, + r.get::<_, f64>(6)? as f32, + ), + )) + })?; + for row in rows { + let (file_id, model, boxed) = row?; + local.entry((file_id, model)).or_default().push(boxed); + } + } + if local.is_empty() { + return Ok(Default::default()); + } + + let mut map = std::collections::HashMap::new(); + let mut stmt = tx.prepare( + "SELECT f.id, r.file_id, f.model_id, f.x, f.y, f.w, f.h + FROM remote_cat.faces f + JOIN remote_cat.remote r ON r.image_id = f.image_id + WHERE r.file_id IS NOT NULL", + )?; + let rows = stmt.query_map([], |r| { + Ok(( + r.get::<_, i64>(0)?, + r.get::<_, i64>(1)?, + r.get::<_, String>(2)?, + ( + r.get::<_, f64>(3)? as f32, + r.get::<_, f64>(4)? as f32, + r.get::<_, f64>(5)? as f32, + r.get::<_, f64>(6)? as f32, + ), + )) + })?; + + for row in rows { + let (remote_id, file_id, model, rbox) = row?; + let Some(candidates) = local.get(&(file_id, model)) else { + continue; + }; + let best = candidates + .iter() + .map(|&(id, x, y, w, h)| (id, iou(rbox, (x, y, w, h)))) + .filter(|&(_, score)| score >= MIN_IOU) + .max_by(|a, b| a.1.total_cmp(&b.1)); + if let Some((local_id, _)) = best { + map.insert(remote_id, local_id); + } + } + Ok(map) +} + +/// Intersection over union of two `(x, y, w, h)` boxes. +fn iou(a: (f32, f32, f32, f32), b: (f32, f32, f32, f32)) -> f32 { + let x0 = a.0.max(b.0); + let y0 = a.1.max(b.1); + let x1 = (a.0 + a.2).min(b.0 + b.2); + let y1 = (a.1 + a.3).min(b.1 + b.3); + let inter = (x1 - x0).max(0.0) * (y1 - y0).max(0.0); + let union = a.2 * a.3 + b.2 * b.3 - inter; + if union <= 0.0 { + 0.0 + } else { + inter / union + } +} + #[cfg(test)] mod tests { use super::*; @@ -1490,4 +1891,286 @@ mod tests { assert_eq!(report.kept_local, 1); assert!(report.should_upload()); } + + // ── people, and who the user said they are ──────────────────────────── + + /// An image present in `db` and carrying the cross-device file id both + /// catalogs agree on. + fn add_synced_image(c: &Connection, db: &str, id: i64, file_id: i64) { + add_image_without_hash(c, db, id); + c.execute( + &format!("INSERT INTO {db}.remote(image_id, file_id) VALUES (?1, ?2)"), + rusqlite::params![id, file_id], + ) + .unwrap(); + } + + /// A face on `image`, at a box the caller can nudge to test the matching. + fn add_face(c: &Connection, db: &str, id: i64, image: i64, x: f64) -> i64 { + c.execute( + &format!( + "INSERT INTO {db}.faces + (id, image_id, x, y, w, h, landmarks, detector_confidence, + embedding, crop_px, model_id, detected_at) + VALUES (?1, ?2, ?3, 0.2, 0.2, 0.2, X'00', 0.9, X'00', 150.0, + 'w600k_mbf', 0)" + ), + rusqlite::params![id, image, x], + ) + .unwrap(); + id + } + + fn add_person(c: &Connection, db: &str, id: i64, uuid: &str, name: &str, ignored: bool) { + c.execute( + &format!( + "INSERT INTO {db}.people(id, uuid, name, ignored, created, revision, modified) + VALUES (?1, ?2, ?3, ?4, 0, 1, 1)" + ), + rusqlite::params![id, uuid, name, ignored], + ) + .unwrap(); + } + + fn assign(c: &Connection, db: &str, face: i64, person: i64, confirmed: bool) { + c.execute( + &format!( + "INSERT INTO {db}.face_person(face_id, person_id, probability, confirmed) + VALUES (?1, ?2, 0.9, ?3)" + ), + rusqlite::params![face, person, confirmed], + ) + .unwrap(); + } + + fn person_of(c: &Connection, face: i64) -> Option<(String, bool)> { + c.query_row( + "SELECT p.name, fp.confirmed + FROM main.face_person fp JOIN main.people p ON p.id = fp.person_id + WHERE fp.face_id = ?1", + [face], + |r| Ok((r.get(0)?, r.get(1)?)), + ) + .optional() + .unwrap() + } + + /// The bug: a second device received every face through the shards and no + /// people at all, because this merge only ever looked at collections and + /// keywords. It drew an empty People screen over a full catalog. + #[test] + fn a_named_person_and_their_confirmed_face_cross_over() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + // The same face, found independently on each device, so the row ids + // differ — which is the whole difficulty. + let local = add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.31); + add_person(&c, "remote_cat", 3, "u-anna", "Anna", false); + assign(&c, "remote_cat", remote, 3, true); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.people_inserted, 1); + assert_eq!(report.faces_assigned, 1); + assert_eq!(person_of(&c, local), Some(("Anna".to_string(), true))); + } + + /// Boxes from two devices are close but not identical. Matching has to be + /// by overlap, not equality, or nothing ever lines up. + #[test] + fn a_face_in_a_different_photograph_is_not_matched() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + add_synced_image(&c, db, 2, 6000); + } + let elsewhere = add_face(&c, "main", 7, 2, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "remote_cat", 3, "u-anna", "Anna", false); + assign(&c, "remote_cat", remote, 3, true); + + merge_all(&c).unwrap(); + assert_eq!(person_of(&c, elsewhere), None, "matched across photographs"); + } + + /// Two faces in one frame, and the judgement must land on the right one. + #[test] + fn the_overlapping_face_is_the_one_that_gets_the_name() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + let left = add_face(&c, "main", 7, 1, 0.10); + let right = add_face(&c, "main", 8, 1, 0.70); + let remote = add_face(&c, "remote_cat", 42, 1, 0.71); + add_person(&c, "remote_cat", 3, "u-bob", "Bob", false); + assign(&c, "remote_cat", remote, 3, true); + + merge_all(&c).unwrap(); + assert_eq!(person_of(&c, right), Some(("Bob".to_string(), true))); + assert_eq!(person_of(&c, left), None); + } + + /// A group set aside on one device stays set aside on the other — which + /// needs its *suggestions* to travel, since that is what anchors it. + #[test] + fn a_group_set_aside_stays_set_aside_on_the_other_device() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + let local = add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "remote_cat", 3, "u-stranger", "", true); + // Only ever a suggestion, which is exactly why it needs carrying. + assign(&c, "remote_cat", remote, 3, false); + + merge_all(&c).unwrap(); + let ignored: bool = c + .query_row( + "SELECT ignored FROM main.people WHERE uuid = 'u-stranger'", + [], + |r| r.get(0), + ) + .unwrap(); + assert!(ignored, "the set-aside flag did not travel"); + assert_eq!(person_of(&c, local), Some((String::new(), false))); + } + + /// An ordinary suggestion is this pass's own output. Both devices hold the + /// same embeddings and clustering is deterministic, so each recomputes it — + /// shipping it would double the merge for no new information. + #[test] + fn an_ordinary_suggestion_does_not_travel() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + let local = add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "remote_cat", 3, "u-guess", "", false); + assign(&c, "remote_cat", remote, 3, false); + + merge_all(&c).unwrap(); + assert_eq!(person_of(&c, local), None); + } + + /// A sync must not undo what the user did on the device they are holding. + #[test] + fn a_local_confirmation_outranks_the_remotes() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + let local = add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "main", 1, "u-anna", "Anna", false); + add_person(&c, "remote_cat", 3, "u-bob", "Bob", false); + assign(&c, "main", local, 1, true); + assign(&c, "remote_cat", remote, 3, true); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.faces_kept_local, 1); + assert_eq!(person_of(&c, local), Some(("Anna".to_string(), true))); + } + + /// "Not this person" is a judgement too, and it has to outrank an + /// assignment arriving from elsewhere. + #[test] + fn a_rejection_travels_and_removes_the_suggestion_it_contradicts() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + let local = add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "main", 1, "u-anna", "Anna", false); + add_person(&c, "remote_cat", 3, "u-anna", "Anna", false); + // Locally suggested; the other device says it is not her. + assign(&c, "main", local, 1, false); + c.execute( + "INSERT INTO remote_cat.face_person_rejected(face_id, person_id) + VALUES (?1, 3)", + [remote], + ) + .unwrap(); + + merge_all(&c).unwrap(); + assert_eq!(person_of(&c, local), None, "the refused suggestion stayed"); + let rejected: bool = c + .query_row( + "SELECT EXISTS(SELECT 1 FROM main.face_person_rejected WHERE face_id = ?1)", + [local], + |r| r.get(0), + ) + .unwrap(); + assert!(rejected); + } + + /// A rename on the other device wins on revision, like everything else. + #[test] + fn a_higher_revision_renames_a_person() { + let c = two_catalogs(); + add_person(&c, "main", 1, "u-anna", "Ana", false); + add_person(&c, "remote_cat", 3, "u-anna", "Anna", false); + c.execute( + "UPDATE remote_cat.people SET revision = 5, modified = 5 WHERE uuid = 'u-anna'", + [], + ) + .unwrap(); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.people_updated, 1); + let name: String = c + .query_row( + "SELECT name FROM main.people WHERE uuid = 'u-anna'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(name, "Anna"); + } + + #[test] + fn a_lower_revision_does_not_rename_a_person() { + let c = two_catalogs(); + add_person(&c, "main", 1, "u-anna", "Anna", false); + add_person(&c, "remote_cat", 3, "u-anna", "Ana", false); + c.execute("UPDATE main.people SET revision = 9, modified = 9", []) + .unwrap(); + + let report = merge_all(&c).unwrap(); + assert_eq!(report.people_kept_local, 1); + let name: String = c + .query_row( + "SELECT name FROM main.people WHERE uuid = 'u-anna'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(name, "Anna"); + } + + /// Merging twice must not double anything — a sync runs on every pass. + #[test] + fn merging_people_twice_changes_nothing_the_second_time() { + let c = two_catalogs(); + for db in ["main", "remote_cat"] { + add_synced_image(&c, db, 1, 5000); + } + add_face(&c, "main", 7, 1, 0.30); + let remote = add_face(&c, "remote_cat", 42, 1, 0.30); + add_person(&c, "remote_cat", 3, "u-anna", "Anna", false); + assign(&c, "remote_cat", remote, 3, true); + + merge_all(&c).unwrap(); + let second = merge_all(&c).unwrap(); + assert_eq!(second.people_inserted, 0); + let people: i64 = c + .query_row("SELECT COUNT(*) FROM main.people", [], |r| r.get(0)) + .unwrap(); + assert_eq!(people, 1); + } } diff --git a/core/dr-catalog/src/schema.rs b/core/dr-catalog/src/schema.rs index e98bf0e..98094f9 100644 --- a/core/dr-catalog/src/schema.rs +++ b/core/dr-catalog/src/schema.rs @@ -206,10 +206,17 @@ pub fn v1_for_attached(schema_name: &str) -> String { /// lost by its absence — it exists to make the *grid* page quickly, and the /// grid never reads across an attachment. pub fn for_attached(schema_name: &str) -> String { + // V10 is `ALTER TABLE`, which the textual rewrite cannot qualify, so its + // columns are spelled out. A remote genuinely older than V10 is a real + // case and `merge::merge_people_within` probes for them; this is the + // *current* shape, which is what the tests want. format!( - "{}\n{}", + "{}\n{}\n{}\n\ + ALTER TABLE {schema_name}.people ADD COLUMN ignored INTEGER NOT NULL DEFAULT 0;\n\ + ALTER TABLE {schema_name}.faces ADD COLUMN crop BLOB;", rewrite_for_attached(V1, schema_name), - rewrite_for_attached(V6, schema_name) + rewrite_for_attached(V6, schema_name), + rewrite_for_attached(V8, schema_name), ) }