Keyless-list diff: gap-based opcodes instead of positional matching

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<String>, out: &mut Vec<Delta>| {
+                        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<String>, out: &mut Vec<Delta>, 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);
                     }
                 }
             }

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:?}"
+    );
+}