diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index 9c186ab..9a9b608 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -265,33 +265,171 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } } } else { - let max_len = std::cmp::max(sl.len(), tl.len()); - for i in 0..max_len { - let step = if let Some(sv) = sl.get(i) { - label(sv) - .or_else(|| tl.get(i).and_then(&label)) - .unwrap_or_else(|| i.to_string()) - } else { - tl.get(i).and_then(&label).unwrap_or_else(|| i.to_string()) - }; - path.push(step); - match (sl.get(i), tl.get(i)) { - (Some(sv), Some(tv)) => inner(path, None, sv, tv, opts, out), - (Some(sv), None) => out.push(Delta { - path: path.clone(), - op: DeltaOp::Remove, - old: Some(sv.to_json()), - new: None, - }), - (None, Some(tv)) => out.push(Delta { - path: path.clone(), - op: DeltaOp::Add, - old: None, - new: Some(tv.to_json()), - }), - (None, None) => {} + // Keyless list: items carry no stable identifier, so a + // positional (by-index) diff mis-attributes shifts — deleting + // or inserting a row makes every later row look edited and, on + // patch, touches the wrong one (WI193). + // + // Diff it as a sequence instead: an LCS over structural + // equality, turned into edit opcodes per gap between matched + // anchors — equal (skip), replace (field-level Update), delete + // (Remove), insert (Add). + // + // Constraint: the patcher overwrites a list slot by index or + // appends, applies index-addressed Removes in descending + // order, and has *no* insert-at-index. An inserted row + // therefore only round-trips if it lands at the very end. When + // an insert would have to go mid-list we fall back to the + // positional overwrite+append encoding the patcher can replay. + let n = sl.len(); + let m = tl.len(); + let eq = |i: usize, j: usize| sl[i].equals(&tl[j], opts.treat_missing_as_null); + + // Positional overwrite/append/remove encoding — the patcher's + // native shape. Used as the fallback for mid-list inserts and + // for lists too large to diff in O(n*m). + let run_positional = |path: &mut Vec, out: &mut Vec| { + let max_len = std::cmp::max(n, m); + for i in 0..max_len { + let step = if let Some(sv) = sl.get(i) { + label(sv) + .or_else(|| tl.get(i).and_then(&label)) + .unwrap_or_else(|| i.to_string()) + } else { + tl.get(i).and_then(&label).unwrap_or_else(|| i.to_string()) + }; + path.push(step); + match (sl.get(i), tl.get(i)) { + (Some(sv), Some(tv)) => inner(path, None, sv, tv, opts, out), + (Some(sv), None) => out.push(Delta { + path: path.clone(), + op: DeltaOp::Remove, + old: Some(sv.to_json()), + new: None, + }), + (None, Some(tv)) => out.push(Delta { + path: path.clone(), + op: DeltaOp::Add, + old: None, + new: Some(tv.to_json()), + }), + (None, None) => {} + } + path.pop(); } - path.pop(); + }; + + // The LCS table is O(n*m); cap it so a pathologically large + // list can't blow up memory/time. Above the cap, fall back to + // the linear positional diff. + const MAX_LCS_CELLS: usize = 1 << 20; // ~8 MB as a flat usize grid + let mut used_lcs = false; + if n.saturating_mul(m) <= MAX_LCS_CELLS { + // LCS-length DP in a flat (n+1)*(m+1) grid, filled from the + // bottom-right corner. + let cols = m + 1; + let at = |i: usize, j: usize| i * cols + j; + let mut dp = vec![0usize; (n + 1) * cols]; + for i in (0..n).rev() { + for j in (0..m).rev() { + dp[at(i, j)] = if eq(i, j) { + dp[at(i + 1, j + 1)] + 1 + } else { + dp[at(i + 1, j)].max(dp[at(i, j + 1)]) + }; + } + } + + // Backtrack into matched (source, target) anchor pairs. + let mut anchors: Vec<(usize, usize)> = Vec::new(); + let (mut i, mut j) = (0usize, 0usize); + while i < n && j < m { + if eq(i, j) { + anchors.push((i, j)); + i += 1; + j += 1; + } else if dp[at(i + 1, j)] >= dp[at(i, j + 1)] { + i += 1; + } else { + j += 1; + } + } + + // Gaps = runs of unmatched rows before/between/after + // anchors, as half-open (src_lo, src_hi, tgt_lo, tgt_hi). + let mut gaps: Vec<(usize, usize, usize, usize)> = Vec::new(); + let (mut ps, mut pt) = (0usize, 0usize); + for &(si, tj) in &anchors { + gaps.push((ps, si, pt, tj)); + ps = si + 1; + pt = tj + 1; + } + gaps.push((ps, n, pt, m)); // trailing gap + + // Per gap: paired positions are field-level Updates + // (recurse), excess source rows are Removes, excess target + // rows are Adds. An Add only round-trips if it appends, so a + // gap with excess target rows must be the trailing gap; + // otherwise fall back to the positional encoding. + // + // Whether a same-position content change is an "edit" or a + // "replace" (delete+add) is intentionally NOT decided here — + // that is a UI concern resolved from tracked add/remove ops + // higher up the chain. The engine just reports the change. + let last_gap = gaps.len() - 1; + let clean_safe = + gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { + g == last_gap || (thi - tlo) <= (shi - slo) + }); + + if clean_safe { + // Emit replaces + adds first and removes last so every + // index is still valid when the patcher applies it + // (removes go descending, adds append). + let mut pending_removes: Vec<(usize, JsonValue)> = Vec::new(); + let mut add_idx = n; + let mut emit_add = + |path: &mut Vec, out: &mut Vec, v: JsonValue| { + path.push(add_idx.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Add, + old: None, + new: Some(v), + }); + path.pop(); + add_idx += 1; + }; + for &(slo, shi, tlo, thi) in &gaps { + let paired = (shi - slo).min(thi - tlo); + for k in 0..paired { + let si = slo + k; + path.push(si.to_string()); + inner(path, None, &sl[si], &tl[tlo + k], opts, out); + path.pop(); + } + for (si, sv) in sl.iter().enumerate().take(shi).skip(slo + paired) { + pending_removes.push((si, sv.to_json())); + } + for tv in tl.iter().take(thi).skip(tlo + paired) { + emit_add(path, out, tv.to_json()); + } + } + for (si, old) in pending_removes { + path.push(si.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Remove, + old: Some(old), + new: None, + }); + path.pop(); + } + used_lcs = true; + } + } + if !used_lcs { + run_positional(path, out); } } } diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index 7a3f767..cd3bb55 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -549,3 +549,160 @@ fn diff_and_patch_multiple_removes_from_scalar_list() { "patched source should equal target after multi-remove" ); } + +/// Regression (WI193): lists of inlined objects with **no** key/identifier +/// slot (e.g. config-driven repeat tables) must be diffed as a sequence, not +/// positionally. Deleting the first row used to shift every later row into a +/// false `Update` and remove the wrong one. An LCS keeps untouched rows quiet, +/// handles duplicate rows by multiplicity, and still reports an in-place edit +/// as a single field-level `Update` rather than a remove+add. All three cases +/// must also round-trip through `patch`. +#[test] +fn diff_and_patch_keyless_object_list_shifts() { + let schema = from_yaml(Path::new(&info_path("personinfo.yaml"))).unwrap(); + let mut sv = SchemaView::new(); + sv.add_schema(schema.clone()).unwrap(); + let conv = converter_from_schema(&schema); + let person = class_in_schema(&sv, &schema, "Person"); + + // MedicalEvent has no key/identifier slot, so `has_medical_history` is a + // keyless inlined-object list. Rows are distinguished by `started_at_time`. + let e1 = r#"{"started_at_time":"2020-01-01","duration":1.0}"#; + let e2 = r#"{"started_at_time":"2021-02-02","duration":2.0}"#; + let e2_edited = r#"{"started_at_time":"2021-02-02","duration":9.0}"#; + let person_with = |events: &str| -> String { + format!(r#"{{"id":"P:001","name":"fred","has_medical_history":[{events}]}}"#) + }; + let load = |json: &str| load_json_instance(json, &sv, &person, &conv); + + let roundtrip = |src: &LinkMLInstance, tgt: &LinkMLInstance| { + let deltas = diff(src, tgt, DiffOptions::new(false)); + let (patched, trace) = patch( + src, + &deltas, + linkml_runtime::diff::PatchOptions { + ignore_no_ops: true, + treat_missing_as_null: false, + }, + ) + .unwrap(); + assert!( + trace.failed.is_empty(), + "no delta should fail to apply, got failed: {:?}", + trace.failed + ); + assert_eq!( + patched.to_json(), + tgt.to_json(), + "patched source must equal target" + ); + deltas + }; + + // Case 1: delete the first row [E1, E2] -> [E2]. Exactly one Remove, no + // Update — the surviving row E2 must NOT be flagged as edited. + let src = load(&person_with(&format!("{e1},{e2}"))); + let tgt = load(&person_with(e2)); + let deltas = roundtrip(&src, &tgt); + assert_eq!( + deltas.iter().filter(|d| d.op == DeltaOp::Remove).count(), + 1, + "delete-first-row: expected exactly one Remove, got: {deltas:?}" + ); + assert!( + deltas.iter().all(|d| d.op == DeltaOp::Remove), + "delete-first-row: only a Remove is allowed, got: {deltas:?}" + ); + + // Case 2: duplicate rows [E1, E1, E2] -> [E1, E2]. One copy of E1 is gone; + // multiplicity-aware diff must emit exactly one Remove and nothing else. + let src = load(&person_with(&format!("{e1},{e1},{e2}"))); + let tgt = load(&person_with(&format!("{e1},{e2}"))); + let deltas = roundtrip(&src, &tgt); + assert_eq!( + deltas.len(), + 1, + "duplicate-row: expected a single delta, got: {deltas:?}" + ); + assert_eq!( + deltas[0].op, + DeltaOp::Remove, + "duplicate-row: the single delta must be a Remove, got: {deltas:?}" + ); + + // Case 3: in-place edit [E1, E2] -> [E1, E2'] (duration changed). Must be a + // single field-level Update, not a remove+add of the whole row. + let src = load(&person_with(&format!("{e1},{e2}"))); + let tgt = load(&person_with(&format!("{e1},{e2_edited}"))); + let deltas = roundtrip(&src, &tgt); + assert_eq!( + deltas.len(), + 1, + "in-place-edit: expected a single delta, got: {deltas:?}" + ); + assert_eq!( + deltas[0].op, + DeltaOp::Update, + "in-place-edit: the single delta must be an Update, got: {deltas:?}" + ); + assert_eq!( + deltas[0].path, + vec![ + "has_medical_history".to_string(), + "1".to_string(), + "duration".to_string() + ], + "in-place-edit: Update must address the changed field of the edited row, got: {deltas:?}" + ); + + // Case 4: mid-list insert [E1, E2] -> [E1, E3, E2]. The index-based patcher + // has no insert-at-index, so this falls back to positional overwrite+append; + // it must still round-trip exactly (regression guard for the hybrid). + let e3 = r#"{"started_at_time":"2022-03-03","duration":3.0}"#; + let src = load(&person_with(&format!("{e1},{e2}"))); + let tgt = load(&person_with(&format!("{e1},{e3},{e2}"))); + roundtrip(&src, &tgt); + + // Case 5: combined delete-first + append [E1, E2] -> [E2, E3]. The added row + // E3 must read as an Add (not the deleted E1 mis-attributed as an edit), the + // dropped E1 as a Remove, and the survivor E2 stays quiet. Round-trips + // because the insert lands at the tail. + let src = load(&person_with(&format!("{e1},{e2}"))); + let tgt = load(&person_with(&format!("{e2},{e3}"))); + let deltas = roundtrip(&src, &tgt); + assert_eq!( + deltas.iter().filter(|d| d.op == DeltaOp::Remove).count(), + 1, + "combined: exactly one Remove (E1), got: {deltas:?}" + ); + assert_eq!( + deltas.iter().filter(|d| d.op == DeltaOp::Add).count(), + 1, + "combined: exactly one Add (E3), got: {deltas:?}" + ); + assert!( + deltas.iter().all(|d| d.op != DeltaOp::Update), + "combined: no row should be mis-reported as an Update, got: {deltas:?}" + ); + + // Case 6: same-position content change [E1, E2] -> [E1, X]. The LCS keeps E1 + // and pairs E2/X in the trailing gap. The engine reports this as field-level + // Updates on row 1 — it does NOT guess whether it was an in-place edit or a + // delete+add (that is a UI concern resolved from tracked ops). Must + // round-trip and touch only index 1. + let x = r#"{"started_at_time":"2099-09-09","duration":99.0}"#; + let src = load(&person_with(&format!("{e1},{x}"))); + let tgt = load(&person_with(&format!("{e1},{e2}"))); + let deltas = roundtrip(&src, &tgt); + assert!( + deltas.iter().all(|d| d.op == DeltaOp::Update), + "same-position change: all deltas are field Updates, got: {deltas:?}" + ); + assert!( + deltas.iter().all( + |d| d.path.first().map(|s| s.as_str()) == Some("has_medical_history") + && d.path.get(1).map(|s| s.as_str()) == Some("1") + ), + "same-position change: Updates address only row 1, got: {deltas:?}" + ); +}