From 60a35a8d848d4777a6bd70cf891f6cdfe56f6fdd Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 10:34:46 +0200 Subject: [PATCH 1/8] fix(diff): LCS over keyless lists to stop shift mis-attribution Positional by-index diff of keyless inlined lists turned a row delete into shifted Updates + a wrong-row Remove (WI193). Replace the else branch with an LCS over structural equality: unchanged rows match by subsequence, leftovers pair (in-place field Updates) then Remove/Add. Keyed branch and patch-side index handling unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 105 +++++++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 24 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index 9c186ab..da1fa8e 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -265,33 +265,90 @@ 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()) + // Keyless list: items carry no stable identifier, so + // positional (by-index) matching mis-attributes shifts — + // deleting an element makes every later element look edited + // and, on patch, removes the wrong one. Diff as a sequence + // via LCS over structural equality instead. + // + // Phase 1: rows in the longest common subsequence are + // unchanged. Phase 2: pair the leftover source/target rows + // in residual order and recurse, so an in-place edit stays a + // field-level Update rather than a remove+add; any surplus + // source row is a Remove, any surplus target row an Add. + let n = sl.len(); + let m = tl.len(); + let eq = |i: usize, j: usize| sl[i].equals(&tl[j], opts.treat_missing_as_null); + + // LCS-length DP filled from the bottom-right corner. + let mut dp = vec![vec![0usize; m + 1]; n + 1]; + for i in (0..n).rev() { + for j in (0..m).rev() { + dp[i][j] = if eq(i, j) { + dp[i + 1][j + 1] + 1 + } else { + dp[i + 1][j].max(dp[i][j + 1]) + }; + } + } + + // Backtrack to flag matched (unchanged) rows on each side. + let mut matched_src = vec![false; n]; + let mut matched_tgt = vec![false; m]; + let (mut i, mut j) = (0usize, 0usize); + while i < n && j < m { + if eq(i, j) { + matched_src[i] = true; + matched_tgt[j] = true; + i += 1; + j += 1; + } else if dp[i + 1][j] >= dp[i][j + 1] { + i += 1; } 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) => {} + j += 1; } + } + + let leftover_src: Vec = (0..n).filter(|&i| !matched_src[i]).collect(); + let leftover_tgt: Vec = (0..m).filter(|&j| !matched_tgt[j]).collect(); + let paired = leftover_src.len().min(leftover_tgt.len()); + + // Paired leftovers: recurse for field-level deltas, addressed + // by the source index (Updates patch in place). + for k in 0..paired { + let si = leftover_src[k]; + path.push(si.to_string()); + inner(path, None, &sl[si], &tl[leftover_tgt[k]], opts, out); + path.pop(); + } + // Surplus source rows are removed, addressed by source index. + // These are the highest leftover indices, so they sit above + // every paired Update and (applied descending by the patcher) + // never shift a lower index. + for &si in &leftover_src[paired..] { + path.push(si.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Remove, + old: Some(sl[si].to_json()), + new: None, + }); + path.pop(); + } + // Surplus target rows are added past the source end so the + // patcher appends them (it overwrites for in-range indices + // and appends otherwise), preserving target order. + let mut add_idx = n; + for &tj in &leftover_tgt[paired..] { + path.push(add_idx.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Add, + old: None, + new: Some(tl[tj].to_json()), + }); path.pop(); + add_idx += 1; } } } From 07e0e8f4775304361eaff059ad8b3ee9be6cdd06 Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 10:36:36 +0200 Subject: [PATCH 2/8] test(diff): keyless inlined-object list shift/dup/edit + round-trip Covers WI193: delete-first-row -> single Remove (survivor not flagged), duplicate rows -> one Remove by multiplicity, in-place edit -> single field-level Update. Each round-trips through patch. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/tests/diff.rs | 106 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index 7a3f767..e8f83bc 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -549,3 +549,109 @@ 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:?}" + ); +} From 7668932179c81a4ea347eceb5147ace74f4ffc09 Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 10:38:03 +0200 Subject: [PATCH 3/8] style(diff): iterator form for keyless-list add counter (clippy) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index da1fa8e..b88f4f2 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -338,8 +338,7 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) // Surplus target rows are added past the source end so the // patcher appends them (it overwrites for in-range indices // and appends otherwise), preserving target order. - let mut add_idx = n; - for &tj in &leftover_tgt[paired..] { + for (add_idx, &tj) in (n..).zip(leftover_tgt[paired..].iter()) { path.push(add_idx.to_string()); out.push(Delta { path: path.clone(), @@ -348,7 +347,6 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) new: Some(tl[tj].to_json()), }); path.pop(); - add_idx += 1; } } } From d382abf9eb2c79d72eef47cf0edebe4ef0b991ba Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 10:49:32 +0200 Subject: [PATCH 4/8] =?UTF-8?q?fix(diff):=20hybrid=20keyless-list=20diff?= =?UTF-8?q?=20=E2=80=94=20clean=20removes=20only=20for=20pure=20deletion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The index-based patcher can overwrite-by-index or append but has no insert-at-index, so emitting a clean Add for a mid-list insert would not round-trip. Restrict the LCS path to pure deletions (target is an ordered subsequence of source) where clean Removes round-trip via descending-index application; everything else (insert/reorder/edit/mixed) keeps the positional overwrite+append encoding the patcher can replay. Fixes WI193 (row delete no longer flags survivors) without regressing insert round-trips. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 141 +++++++++++++++++++++----------------- src/runtime/tests/diff.rs | 8 +++ 2 files changed, 85 insertions(+), 64 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index b88f4f2..e81c5f1 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -265,17 +265,24 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } } } else { - // Keyless list: items carry no stable identifier, so - // positional (by-index) matching mis-attributes shifts — - // deleting an element makes every later element look edited - // and, on patch, removes the wrong one. Diff as a sequence - // via LCS over structural equality instead. + // Keyless list: items carry no stable identifier. Positional + // (by-index) matching mis-attributes shifts — deleting an + // element makes every later element look edited and, on + // patch, removes the wrong one (WI193). // - // Phase 1: rows in the longest common subsequence are - // unchanged. Phase 2: pair the leftover source/target rows - // in residual order and recurse, so an in-place edit stays a - // field-level Update rather than a remove+add; any surplus - // source row is a Remove, any surplus target row an Add. + // We can only reorder the diff safely within what the patcher + // can replay: it overwrites a list slot by index or appends, + // and applies index-addressed Removes in descending order — + // it has *no* insert-at-index. So: + // + // * Pure deletion (target is an ordered subsequence of the + // source): emit clean Removes for the dropped rows, found + // via an LCS over structural equality. Remove addresses + // any index and round-trips, so this is safe and fixes the + // shift mis-attribution. + // * Anything else (insert / reorder / edit / mixed): fall + // back to positional overwrite+append, the only encoding + // the index-based patcher can faithfully 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); @@ -292,61 +299,67 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } } - // Backtrack to flag matched (unchanged) rows on each side. - let mut matched_src = vec![false; n]; - let mut matched_tgt = vec![false; m]; - let (mut i, mut j) = (0usize, 0usize); - while i < n && j < m { - if eq(i, j) { - matched_src[i] = true; - matched_tgt[j] = true; - i += 1; - j += 1; - } else if dp[i + 1][j] >= dp[i][j + 1] { - i += 1; - } else { - j += 1; + // `dp[0][0] == m` means every target row lies on a common + // subsequence — i.e. the target is the source with some rows + // dropped (same order): a pure deletion. + if dp[0][0] == m && m < n { + // Backtrack to flag the surviving (matched) source rows; + // the rest were deleted. + let mut matched_src = vec![false; n]; + let (mut i, mut j) = (0usize, 0usize); + while i < n && j < m { + if eq(i, j) { + matched_src[i] = true; + i += 1; + j += 1; + } else if dp[i + 1][j] >= dp[i][j + 1] { + i += 1; + } else { + j += 1; + } + } + for (i, sv) in sl.iter().enumerate() { + if matched_src[i] { + continue; + } + path.push(i.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Remove, + old: Some(sv.to_json()), + new: None, + }); + path.pop(); + } + } else { + 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(); } - } - - let leftover_src: Vec = (0..n).filter(|&i| !matched_src[i]).collect(); - let leftover_tgt: Vec = (0..m).filter(|&j| !matched_tgt[j]).collect(); - let paired = leftover_src.len().min(leftover_tgt.len()); - - // Paired leftovers: recurse for field-level deltas, addressed - // by the source index (Updates patch in place). - for k in 0..paired { - let si = leftover_src[k]; - path.push(si.to_string()); - inner(path, None, &sl[si], &tl[leftover_tgt[k]], opts, out); - path.pop(); - } - // Surplus source rows are removed, addressed by source index. - // These are the highest leftover indices, so they sit above - // every paired Update and (applied descending by the patcher) - // never shift a lower index. - for &si in &leftover_src[paired..] { - path.push(si.to_string()); - out.push(Delta { - path: path.clone(), - op: DeltaOp::Remove, - old: Some(sl[si].to_json()), - new: None, - }); - path.pop(); - } - // Surplus target rows are added past the source end so the - // patcher appends them (it overwrites for in-range indices - // and appends otherwise), preserving target order. - for (add_idx, &tj) in (n..).zip(leftover_tgt[paired..].iter()) { - path.push(add_idx.to_string()); - out.push(Delta { - path: path.clone(), - op: DeltaOp::Add, - old: None, - new: Some(tl[tj].to_json()), - }); - path.pop(); } } } diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index e8f83bc..c66be32 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -654,4 +654,12 @@ fn diff_and_patch_keyless_object_list_shifts() { ], "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); } From fc0ff260ce1c3a373c91214d214deaf7f93c6e05 Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 12:14:53 +0200 Subject: [PATCH 5/8] fix(diff): gap-based opcode diff for keyless lists (combined edits) Replace the pure-deletion-only hybrid with a full LCS gap diff: per gap between matched anchors, pair rows as replaces (field Updates), excess source rows as Removes, excess target rows as Adds. A combined delete+add (e.g. [P,Q]->[Q,R]) now yields remove P + add R instead of mis-attributed positional Updates. Stays round-trip-safe: clean opcodes only when inserts append (suffix); mid-list inserts fall back to positional overwrite+append. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 119 +++++++++++++++++++++++++------------- src/runtime/tests/diff.rs | 22 +++++++ 2 files changed, 102 insertions(+), 39 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index e81c5f1..8addd89 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -265,24 +265,22 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } } } else { - // Keyless list: items carry no stable identifier. Positional - // (by-index) matching mis-attributes shifts — deleting an - // element makes every later element look edited and, on - // patch, removes the wrong one (WI193). + // 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). // - // We can only reorder the diff safely within what the patcher - // can replay: it overwrites a list slot by index or appends, - // and applies index-addressed Removes in descending order — - // it has *no* insert-at-index. So: + // 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). // - // * Pure deletion (target is an ordered subsequence of the - // source): emit clean Removes for the dropped rows, found - // via an LCS over structural equality. Remove addresses - // any index and round-trips, so this is safe and fixes the - // shift mis-attribution. - // * Anything else (insert / reorder / edit / mixed): fall - // back to positional overwrite+append, the only encoding - // the index-based patcher can faithfully replay. + // 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); @@ -299,34 +297,77 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } } - // `dp[0][0] == m` means every target row lies on a common - // subsequence — i.e. the target is the source with some rows - // dropped (same order): a pure deletion. - if dp[0][0] == m && m < n { - // Backtrack to flag the surviving (matched) source rows; - // the rest were deleted. - let mut matched_src = vec![false; n]; - let (mut i, mut j) = (0usize, 0usize); - while i < n && j < m { - if eq(i, j) { - matched_src[i] = true; - i += 1; - j += 1; - } else if dp[i + 1][j] >= dp[i][j + 1] { - i += 1; - } else { - 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[i + 1][j] >= dp[i][j + 1] { + i += 1; + } else { + j += 1; } - for (i, sv) in sl.iter().enumerate() { - if matched_src[i] { - continue; + } + + // Gaps = runs of unmatched rows before/between/after anchors, + // as half-open ranges (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 + + // An insert round-trips only if it appends, i.e. no gap other + // than the last carries excess target rows. + let last_gap = gaps.len() - 1; + let insert_is_suffix = + gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { + g == last_gap || (thi - tlo) <= (shi - slo) + }); + + if insert_is_suffix { + // Per gap: pair rows positionally as replaces (recurse for + // field-level Updates), excess source rows are Removes, + // excess target rows are Adds. Emit replaces + adds first + // and removes last so every index is still valid when the + // patcher applies it (removes go in descending order). + let mut pending_removes: Vec<(usize, JsonValue)> = Vec::new(); + let mut add_idx = n; + 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(); } - path.push(i.to_string()); + 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) { + path.push(add_idx.to_string()); + out.push(Delta { + path: path.clone(), + op: DeltaOp::Add, + old: None, + new: Some(tv.to_json()), + }); + path.pop(); + add_idx += 1; + } + } + for (si, old) in pending_removes { + path.push(si.to_string()); out.push(Delta { path: path.clone(), op: DeltaOp::Remove, - old: Some(sv.to_json()), + old: Some(old), new: None, }); path.pop(); diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index c66be32..4ee9f3a 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -662,4 +662,26 @@ fn diff_and_patch_keyless_object_list_shifts() { 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:?}" + ); } From 99376b41507515f0618c845a7b09d093ac9861ad Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 13:02:07 +0200 Subject: [PATCH 6/8] fix(diff): similarity-gate keyless replaces (delete+add vs edit) A gap pairing two leftover rows was always a field-level Update, so a deleted row replaced in place by an unrelated new row read as an edit (WI193 combined case). Pair as an Update only when the rows are similar (>= half their fields unchanged); dissimilar rows become Remove + Add. Such an Add must append, so a dissimilar pair outside the trailing gap falls back to the positional encoding for round-trip safety. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 101 +++++++++++++++++++++++++++++--------- src/runtime/tests/diff.rs | 24 +++++++++ 2 files changed, 101 insertions(+), 24 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index 8addd89..61a0602 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -20,6 +20,36 @@ fn slot_is_ignored(slot: &SlotView) -> bool { .unwrap_or(false) } +/// Heuristic: are two keyless-list rows the *same row edited* (so a diff should +/// pair them as a field-level update) or *different rows* (a delete + add)? +/// +/// Keyless rows have no identifier, so we judge by content: two objects are +/// "the same edited row" when at least half of their fields are unchanged. A +/// genuine field edit keeps most fields; a freshly added row that displaced a +/// deleted one typically shares almost nothing. Non-objects (e.g. scalars) are +/// always treated as edits — a scalar value change is an update in place. +fn rows_similar(s: &LinkMLInstance, t: &LinkMLInstance, treat_missing_as_null: bool) -> bool { + match (s, t) { + (LinkMLInstance::Object { values: sm, .. }, LinkMLInstance::Object { values: tm, .. }) => { + let mut keys: std::collections::HashSet<&String> = sm.keys().collect(); + keys.extend(tm.keys()); + if keys.is_empty() { + return true; + } + let mut equal = 0usize; + for k in &keys { + match (sm.get(*k), tm.get(*k)) { + (Some(a), Some(b)) if a.equals(b, treat_missing_as_null) => equal += 1, + (None, None) => equal += 1, + _ => {} + } + } + equal * 2 >= keys.len() + } + _ => true, + } +} + /// Operation applied by a [`Delta`]. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] @@ -323,43 +353,66 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } gaps.push((ps, n, pt, m)); // trailing gap - // An insert round-trips only if it appends, i.e. no gap other - // than the last carries excess target rows. + // Within each gap, a paired position is a *replace* only when + // the two rows are similar (a field edit); dissimilar rows are + // a delete + add. A gap emits an Add when it has excess target + // rows or a dissimilar pair. Such an Add only round-trips if it + // appends, so it must be confined to the trailing gap; + // otherwise fall back to the positional encoding. let last_gap = gaps.len() - 1; - let insert_is_suffix = - gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { - g == last_gap || (thi - tlo) <= (shi - slo) - }); + let gap_emits_add = |slo: usize, shi: usize, tlo: usize, thi: usize| { + let paired = (shi - slo).min(thi - tlo); + (thi - tlo) > (shi - slo) + || (0..paired).any(|k| { + !rows_similar( + &sl[slo + k], + &tl[tlo + k], + opts.treat_missing_as_null, + ) + }) + }; + let clean_safe = gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { + g == last_gap || !gap_emits_add(slo, shi, tlo, thi) + }); - if insert_is_suffix { - // Per gap: pair rows positionally as replaces (recurse for - // field-level Updates), excess source rows are Removes, - // excess target rows are Adds. Emit replaces + adds first - // and removes last so every index is still valid when the - // patcher applies it (removes go in descending order). + if clean_safe { + // Emit replaces + adds first and removes last so every index + // is still valid when the patcher applies it (removes go in + // descending order, 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(); + let tj = tlo + k; + if rows_similar(&sl[si], &tl[tj], opts.treat_missing_as_null) { + path.push(si.to_string()); + inner(path, None, &sl[si], &tl[tj], opts, out); + path.pop(); + } else { + // Different rows: delete the source, add the target. + pending_removes.push((si, sl[si].to_json())); + emit_add(path, out, tl[tj].to_json()); + } } 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) { - path.push(add_idx.to_string()); - out.push(Delta { - path: path.clone(), - op: DeltaOp::Add, - old: None, - new: Some(tv.to_json()), - }); - path.pop(); - add_idx += 1; + emit_add(path, out, tv.to_json()); } } for (si, old) in pending_removes { diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index 4ee9f3a..2b33822 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -684,4 +684,28 @@ fn diff_and_patch_keyless_object_list_shifts() { deltas.iter().all(|d| d.op != DeltaOp::Update), "combined: no row should be mis-reported as an Update, got: {deltas:?}" ); + + // Case 6: same-position delete + add of an UNRELATED row [E1, E2] -> [E1, X], + // where X shares no field values with E2. The LCS keeps E1 and leaves E2/X in + // the same gap; they must NOT be paired into a field-level Update (that is the + // reported bug where the added row reads as an edit of the deleted one). With + // no shared fields they are dissimilar -> Remove E2 + Add X. + let x = r#"{"started_at_time":"2099-09-09","duration":99.0}"#; + let src = load(&person_with(&format!("{e1},{e2}"))); + let tgt = load(&person_with(&format!("{e1},{x}"))); + let deltas = roundtrip(&src, &tgt); + assert!( + deltas.iter().all(|d| d.op != DeltaOp::Update), + "dissimilar replace must not be an Update, got: {deltas:?}" + ); + assert_eq!( + deltas.iter().filter(|d| d.op == DeltaOp::Remove).count(), + 1, + "dissimilar replace: one Remove (E2), got: {deltas:?}" + ); + assert_eq!( + deltas.iter().filter(|d| d.op == DeltaOp::Add).count(), + 1, + "dissimilar replace: one Add (X), got: {deltas:?}" + ); } From a1aaa1db12d91b85af7ea6043f3daf144c6829f3 Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 15:26:56 +0200 Subject: [PATCH 7/8] fix(diff): respect treat_missing_as_null in rows_similar; cap LCS table Address PR review: - rows_similar now counts a null-vs-missing field as equal when treat_missing_as_null, matching LinkMLInstance::equals, so that difference no longer flips a field edit into Remove+Add. - Cap the O(n*m) LCS table (flat usize grid, ~8MB) and fall back to the linear positional diff for lists above the cap, bounding memory/time. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 251 ++++++++++++++++++++++------------------ 1 file changed, 140 insertions(+), 111 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index 61a0602..8823604 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -41,6 +41,16 @@ fn rows_similar(s: &LinkMLInstance, t: &LinkMLInstance, treat_missing_as_null: b match (sm.get(*k), tm.get(*k)) { (Some(a), Some(b)) if a.equals(b, treat_missing_as_null) => equal += 1, (None, None) => equal += 1, + // With treat_missing_as_null, a field present-as-null on one + // side and absent on the other is equal (matches + // LinkMLInstance::equals null/missing normalisation), so a + // null-vs-missing difference doesn't flip the similarity. + (Some(LinkMLInstance::Null { .. }), None) + | (None, Some(LinkMLInstance::Null { .. })) + if treat_missing_as_null => + { + equal += 1 + } _ => {} } } @@ -315,117 +325,10 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) let m = tl.len(); let eq = |i: usize, j: usize| sl[i].equals(&tl[j], opts.treat_missing_as_null); - // LCS-length DP filled from the bottom-right corner. - let mut dp = vec![vec![0usize; m + 1]; n + 1]; - for i in (0..n).rev() { - for j in (0..m).rev() { - dp[i][j] = if eq(i, j) { - dp[i + 1][j + 1] + 1 - } else { - dp[i + 1][j].max(dp[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[i + 1][j] >= dp[i][j + 1] { - i += 1; - } else { - j += 1; - } - } - - // Gaps = runs of unmatched rows before/between/after anchors, - // as half-open ranges (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 - - // Within each gap, a paired position is a *replace* only when - // the two rows are similar (a field edit); dissimilar rows are - // a delete + add. A gap emits an Add when it has excess target - // rows or a dissimilar pair. Such an Add only round-trips if it - // appends, so it must be confined to the trailing gap; - // otherwise fall back to the positional encoding. - let last_gap = gaps.len() - 1; - let gap_emits_add = |slo: usize, shi: usize, tlo: usize, thi: usize| { - let paired = (shi - slo).min(thi - tlo); - (thi - tlo) > (shi - slo) - || (0..paired).any(|k| { - !rows_similar( - &sl[slo + k], - &tl[tlo + k], - opts.treat_missing_as_null, - ) - }) - }; - let clean_safe = gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { - g == last_gap || !gap_emits_add(slo, shi, tlo, thi) - }); - - if clean_safe { - // Emit replaces + adds first and removes last so every index - // is still valid when the patcher applies it (removes go in - // descending order, 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; - let tj = tlo + k; - if rows_similar(&sl[si], &tl[tj], opts.treat_missing_as_null) { - path.push(si.to_string()); - inner(path, None, &sl[si], &tl[tj], opts, out); - path.pop(); - } else { - // Different rows: delete the source, add the target. - pending_removes.push((si, sl[si].to_json())); - emit_add(path, out, tl[tj].to_json()); - } - } - 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(); - } - } else { + // 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) { @@ -454,6 +357,132 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } 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 + + // A paired position is a *replace* only when the rows are + // similar (a field edit); dissimilar rows are a delete + + // add. A gap emits an Add when it has excess target rows or + // a dissimilar pair, which only round-trips if it appends — + // so it must be the trailing gap, else fall back. + let last_gap = gaps.len() - 1; + let gap_emits_add = |slo: usize, shi: usize, tlo: usize, thi: usize| { + let paired = (shi - slo).min(thi - tlo); + (thi - tlo) > (shi - slo) + || (0..paired).any(|k| { + !rows_similar( + &sl[slo + k], + &tl[tlo + k], + opts.treat_missing_as_null, + ) + }) + }; + let clean_safe = + gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { + g == last_gap || !gap_emits_add(slo, shi, tlo, thi) + }); + + 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; + let tj = tlo + k; + if rows_similar(&sl[si], &tl[tj], opts.treat_missing_as_null) { + path.push(si.to_string()); + inner(path, None, &sl[si], &tl[tj], opts, out); + path.pop(); + } else { + // Different rows: delete source, add target. + pending_removes.push((si, sl[si].to_json())); + emit_add(path, out, tl[tj].to_json()); + } + } + 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); } } } From 13fbe417fd49c47cc54bde5a0f7cf6a993afc7a1 Mon Sep 17 00:00:00 2001 From: Alexander Date: Fri, 19 Jun 2026 16:27:11 +0200 Subject: [PATCH 8/8] refactor(diff): drop rows_similar heuristic from keyless-list diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edit-vs-replace is a UI concern, not the diff engine's: deciding whether a same-position content change is an in-place edit or a delete+add can't be done from row content without a key. Remove the rows_similar guess — the engine now reports a same-position change as a field-level Update; consumers resolve add/remove intent from tracked ops. LCS stays (correct deletes), clean opcodes only when inserts append (else positional). Tests updated: same-position replace is an Update; pure delete/append/reorder/combined unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/runtime/src/diff.rs | 81 +++++++-------------------------------- src/runtime/tests/diff.rs | 33 ++++++++-------- 2 files changed, 29 insertions(+), 85 deletions(-) diff --git a/src/runtime/src/diff.rs b/src/runtime/src/diff.rs index 8823604..9a9b608 100644 --- a/src/runtime/src/diff.rs +++ b/src/runtime/src/diff.rs @@ -20,46 +20,6 @@ fn slot_is_ignored(slot: &SlotView) -> bool { .unwrap_or(false) } -/// Heuristic: are two keyless-list rows the *same row edited* (so a diff should -/// pair them as a field-level update) or *different rows* (a delete + add)? -/// -/// Keyless rows have no identifier, so we judge by content: two objects are -/// "the same edited row" when at least half of their fields are unchanged. A -/// genuine field edit keeps most fields; a freshly added row that displaced a -/// deleted one typically shares almost nothing. Non-objects (e.g. scalars) are -/// always treated as edits — a scalar value change is an update in place. -fn rows_similar(s: &LinkMLInstance, t: &LinkMLInstance, treat_missing_as_null: bool) -> bool { - match (s, t) { - (LinkMLInstance::Object { values: sm, .. }, LinkMLInstance::Object { values: tm, .. }) => { - let mut keys: std::collections::HashSet<&String> = sm.keys().collect(); - keys.extend(tm.keys()); - if keys.is_empty() { - return true; - } - let mut equal = 0usize; - for k in &keys { - match (sm.get(*k), tm.get(*k)) { - (Some(a), Some(b)) if a.equals(b, treat_missing_as_null) => equal += 1, - (None, None) => equal += 1, - // With treat_missing_as_null, a field present-as-null on one - // side and absent on the other is equal (matches - // LinkMLInstance::equals null/missing normalisation), so a - // null-vs-missing difference doesn't flip the similarity. - (Some(LinkMLInstance::Null { .. }), None) - | (None, Some(LinkMLInstance::Null { .. })) - if treat_missing_as_null => - { - equal += 1 - } - _ => {} - } - } - equal * 2 >= keys.len() - } - _ => true, - } -} - /// Operation applied by a [`Delta`]. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] @@ -406,26 +366,20 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) } gaps.push((ps, n, pt, m)); // trailing gap - // A paired position is a *replace* only when the rows are - // similar (a field edit); dissimilar rows are a delete + - // add. A gap emits an Add when it has excess target rows or - // a dissimilar pair, which only round-trips if it appends — - // so it must be the trailing gap, else fall back. + // 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 gap_emits_add = |slo: usize, shi: usize, tlo: usize, thi: usize| { - let paired = (shi - slo).min(thi - tlo); - (thi - tlo) > (shi - slo) - || (0..paired).any(|k| { - !rows_similar( - &sl[slo + k], - &tl[tlo + k], - opts.treat_missing_as_null, - ) - }) - }; let clean_safe = gaps.iter().enumerate().all(|(g, &(slo, shi, tlo, thi))| { - g == last_gap || !gap_emits_add(slo, shi, tlo, thi) + g == last_gap || (thi - tlo) <= (shi - slo) }); if clean_safe { @@ -450,16 +404,9 @@ pub fn diff(source: &LinkMLInstance, target: &LinkMLInstance, opts: DiffOptions) let paired = (shi - slo).min(thi - tlo); for k in 0..paired { let si = slo + k; - let tj = tlo + k; - if rows_similar(&sl[si], &tl[tj], opts.treat_missing_as_null) { - path.push(si.to_string()); - inner(path, None, &sl[si], &tl[tj], opts, out); - path.pop(); - } else { - // Different rows: delete source, add target. - pending_removes.push((si, sl[si].to_json())); - emit_add(path, out, tl[tj].to_json()); - } + 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())); diff --git a/src/runtime/tests/diff.rs b/src/runtime/tests/diff.rs index 2b33822..cd3bb55 100644 --- a/src/runtime/tests/diff.rs +++ b/src/runtime/tests/diff.rs @@ -685,27 +685,24 @@ fn diff_and_patch_keyless_object_list_shifts() { "combined: no row should be mis-reported as an Update, got: {deltas:?}" ); - // Case 6: same-position delete + add of an UNRELATED row [E1, E2] -> [E1, X], - // where X shares no field values with E2. The LCS keeps E1 and leaves E2/X in - // the same gap; they must NOT be paired into a field-level Update (that is the - // reported bug where the added row reads as an edit of the deleted one). With - // no shared fields they are dissimilar -> Remove E2 + Add X. + // 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},{e2}"))); - let tgt = load(&person_with(&format!("{e1},{x}"))); + 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), - "dissimilar replace must not be an Update, got: {deltas:?}" + deltas.iter().all(|d| d.op == DeltaOp::Update), + "same-position change: all deltas are field Updates, got: {deltas:?}" ); - assert_eq!( - deltas.iter().filter(|d| d.op == DeltaOp::Remove).count(), - 1, - "dissimilar replace: one Remove (E2), got: {deltas:?}" - ); - assert_eq!( - deltas.iter().filter(|d| d.op == DeltaOp::Add).count(), - 1, - "dissimilar replace: one Add (X), 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:?}" ); }