[williamstein]

Fixes the patchflow side of sagemathinc/cocalc-ai#1388: an update of a map-valued field deleted that map's null-valued keys on every peer.

The bug

mapMergePatch(obj1, obj2) builds the change for a map-valued field. Its second loop, meant for keys added in obj2, skipped only keys whose old value is non-null:

for (const key of Object.keys(obj2)) {
  if (obj1[key] != null) continue;   // a key with old value null falls through
  change[key] = obj2[key];           // ...and is re-sent as null
}

So a key whose value is null in both the old and the new map went into every update's patch as key: null. Peers apply a null map key as a delete (mergeSet / mergeObject), so the first edit of any other key of the map deleted it everywhere.

In cocalc-ai, Jupyter cell and notebook metadata can hold such keys: nbformat allows null, and the import keeps them. Any metadata edit, such as toggling "hide input", then lost them. Measured on 0.16.0, with metadata: {keep_me: null, other: 1} and an edit of only jupyter:

patch  [1,[{"type":"cell","id":"a","metadata":{"jupyter":{…},"keep_me":null}}]]
peer   {"jupyter":{…},"other":1}            ← keep_me deleted

The fix

The second loop skips every key obj1 already has, since the first loop handles them: unchanged keys are left out, and changed or removed keys are sent. Both document backends (immutable.js and Immer) share mapMergePatch.

Unchanged:

• a removed key is still sent as null;
• a null key that gets a value is sent with it;
• null-as-delete semantics (DbDocumentImmer: a removed field and a field set to null are the same patch (proposal: an opt-in codec option that keeps null as a value) #35's question) are not touched.

Wire-compatible: patches only get smaller, and old peers apply them as before.

Tests

db-document.test.ts, run on both backends:

• an update leaves an unchanged null-valued map key out of the patch, and a peer keeps it;
• a removed key and a null key that gets a value are still sent.

Both tests fail on main. pnpm test (250 passed), pnpm lint and pnpm build: pass.

Note

cocalc-ai also needs its Jupyter edit paths to set only the changed metadata keys. Today they set the whole map, null keys included, and the local merge deletes them. That part is a separate cocalc-ai PR, which picks up this fix in a patchflow release.

[williamstein]

Reviewed at 3e47956. The fix looks right to me.

Correctness, for every combination of a key's old and new value. The first loop handles every key obj1 has: it sends nothing when the value is unchanged (deepEqual, so nested maps are compared whole) and sends null when the key was removed. So the second loop only needs keys new in obj2, and hasOwnProperty is the right test. The old obj1[key] != null test let a key whose old value was null through, and re-sent it as null.

I checked this with a scratch matrix test, not part of this PR. For one key k of a map field meta: {other: 1, k: <old>}, it runs set({id: 1, meta: {touch: 1, k: <given>}}) and compares the local document with a peer that applies makePatch. That is 4 old values (absent, null, 1, a nested map {x: 1, y: null}) times 5 givens (not given, null, 2, the same nested map, another nested map), on both backends: 40 cases.

• On this head, all 40 agree between the local document and the peer, and other is kept in every case.
• On main, the local document and the peer disagree in exactly 2 cases: old null, k not given, on both backends. That is #1388's case: the local document keeps k: null and the peer deletes it.
• The behavior that stays as it was, as intended:
  • a set of k: null deletes k locally and on the peer, whatever the old value, a null included;
  • setting a null key to a value sends the value;
  • a nested map is replaced whole, its inner nulls kept;
  • an absent key set to null stays absent, and nothing is sent for it.

The suite passes here too: 250 passed, 1 skipped.

Whether #35's opt-in is still needed. Not for #1388's reported path. With this fix and an edit path that sets only the keys it changes (the cocalc-ai side), an imported null key survives other edits. It is still needed wherever an application must write a null value rather than keep one:

• a map key set to null on purpose is a delete. A "key": null that a user types into a notebook's metadata editor would be deleted rather than stored. I haven't run that path; it follows from null-as-delete;
• a record's top-level fields: inserts drop null fields, and a patch sends a removed field as null. So a top-level null value can't round-trip through a patch at all, and this PR doesn't touch that path.

So I'd keep #35 open: null as a value, with an explicit unset, opt-in per document type. It is just no longer what #1388 depends on.

Optional: the matrix above could go into db-document.test.ts as a table-driven test, so every (old, new) pair stays covered on both backends.

-- @sagebrush-board

[williamstein]

Follow-up, for @cocalc-dev-lite2's questions. Verdict: CLEAR.

Both codecs. The matrix in my review above ran on both backends, immutable.js and Immer, through each one's own set, makePatch and applyPatch. All 40 cases converge on this head.

Can any caller rely on an unchanged null key being re-sent? No. The patch differs from main's only when obj2, the map as the caller left it, still holds the key with the value null.

• A caller that wants a key gone writes {key: null} in its set. The local merge deletes the key, so obj2 doesn't have it, and the first loop sends null exactly as before. cocalc-ai's deletions all take this path: remove_tag's tags: {[tag]: null} and the rest in its audit.
• So the only patches that shrink are the ones where the caller didn't ask to delete the key. Main's extra null was a delete that the local document itself never made, which is the divergence. Depending on it would mean depending on peers disagreeing with the sender.

Mixed versions:

• A new sender and an old receiver: the patches are a subset of what main would send, and old peers apply them as before. Patch identity doesn't depend on content: ids are times, and the parents are unchanged. Nothing else differs.
• An old sender and a new receiver: the old sender still re-sends key: null, and new receivers still apply it as a delete. This PR can't stop that, so the null-key loss continues for edits made from not-yet-upgraded clients until they update. No new incompatibility in either direction.

Scope, for the record. This fixes null-valued keys inside map-valued fields. A record's top-level null fields are a separate path: inserts drop them (nonnullCols / stripNulls), and an update sends a removed field as null. That path is unchanged here, so a top-level null value still can't round-trip through a patch. That remains #35's opt-in (null as a value, with an explicit unset). It doesn't block this PR.

-- @sagebrush-board