From d992d7ceff53f213d570fd7db7cb8b6d648aa5cc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 13 Nov 2026 00:48:08 +0000 Subject: [PATCH] perf(canon): Reconcile parses the documents seven times, not twelve Reconcile is a row of steps from bytes to bytes, and each parsed both documents for itself: twelve whole-document parses for one write, 23 s of a 77 s settled re-run of 600 pages. Five of them served steps that only read the stored document or needed no tree at all. - The transplant's pairing and the final comparison share one parse of the stored document. The carries between them copy parts of the stored tree into the new one, so they keep a parse of their own. - The id substitution reads the document's ids off the raw bytes instead of parsing it before and after it patches. - The PersistentId carry is skipped for a document that does not hold the bytes of a binary PersistentId element; only a workflow does. The old Reconcile is kept as reconcileInPasses and compared byte for byte, with the verdict, on every PedApp unit against itself, a rebuild with new ids, rebuilds with properties left out, and the reverse: 2,214 comparisons. A workflow-shaped pair covers the PersistentId path the fixture does not. Not reading ids under arrays, and always skipping the carry, each fail the tests. A script changing all 600 pages, run with the previous binary and this one, leaves 1,719 byte-identical units. Settled page re-run: 74.3 s -> 61.6 s. Refs #2160 Co-Authored-By: Claude Fable 5.2 Claude-Session: https://claude.ai/code/session_011oK82u4HRCKfX2BBvhgsNm --- ...oncile-parses-both-documents-per-step.json | 1 + CHANGELOG.md | 1 + modelsdk/canon/canon.go | 8 +- modelsdk/canon/identity.go | 103 +++++++++- modelsdk/canon/persistentid.go | 19 ++ modelsdk/canon/reconcile_passes_test.go | 186 ++++++++++++++++++ modelsdk/canon/transplant.go | 118 ++++++++++- 7 files changed, 427 insertions(+), 9 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/modelsdk/2026-11-13-reconcile-parses-both-documents-per-step.json create mode 100644 modelsdk/canon/reconcile_passes_test.go diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-11-13-reconcile-parses-both-documents-per-step.json b/.claude/skills/fix-issue/findings/modelsdk/2026-11-13-reconcile-parses-both-documents-per-step.json new file mode 100644 index 000000000..3597631fd --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-11-13-reconcile-parses-both-documents-per-step.json @@ -0,0 +1 @@ +{"area": "modelsdk/canon", "date": "2026-11-13", "symptom": "canon.Reconcile was 23 s of a 77 s settled re-run of 600 pages, about 38 ms for each rebuilt page compared with its stored unit (ako/mxcli#2160)", "cause": "Reconcile is a row of steps from bytes to bytes - carry translations, transplant ids, carry PersistentIds, carry what MDL cannot state, compare - and each parses both documents for itself: twelve whole-document parses for one write. Five of them served steps that either only read the stored document or did not need a tree at all: the id substitution parsed the document before and after patching to read its ids, the PersistentId carry parsed both documents to find that a page has no PersistentId, and the transplant and the final comparison each parsed the stored document", "file": "modelsdk/canon/identity.go (Reconcile, parseStored, equalToStored, reconcileInPasses), modelsdk/canon/transplant.go (transplantIDs, idMappingWith, elementIDSetOf), modelsdk/canon/persistentid.go (persistentIDElement), modelsdk/canon/canon.go (digestOfDoc)", "insight": "The obvious change - parse each document once and hand the trees to every step - is unsafe here, and reading the steps showed why before any test did: the translation carry and the unexpressed carry copy parts of the stored tree into the new one, so a shared stored tree could be changed under the steps after them, and the new document changes between steps. Only the steps that provably just read the stored document share its parse (the transplant's pairing and the final digest); the carries keep a parse of their own. The other two savings need no tree: the ids are read off the raw bytes, and a document that does not contain the bytes of a binary PersistentId element has nothing to carry. Twelve parses become seven. The old Reconcile is kept verbatim as reconcileInPasses and compared byte for byte, with the verdict, on every PedApp unit against itself, a rebuild with new ids, rebuilds with properties left out, and the reverse: 2,214 comparisons. The fixture has no workflow, so the PersistentId path was unexercised on both sides; a workflow-shaped pair is compared separately, and always skipping the carry fails three tests. A first version of that control did not compile (the mutation left an import unused) and printed nothing, which reads like a pass: a control has to be seen to fail. End to end, a script changing all 600 pages run with the old and the new binary leaves 1,719 byte-identical units, 600 of which differ from before the run. Measured: settled page re-run 74.3 s -> 61.6 s, changed-pages run 72.1 s -> 65.6 s."} diff --git a/CHANGELOG.md b/CHANGELOG.md index a4cfd7492..c3f9853a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - **`check` resolves an `alter page` target against the building blocks the page holds** (ako/mxcli#2182, mendixlabs/mxcli#1835) — an `alter page` / `alter snippet` on a document the same script creates was never resolved, so `set … on a_container99` after `use building block PlatformDesignSystem.Alert_Info as a_` passed `check -p` and exec then stopped on `widget "a_container99" not found`, with the page already created. A plain misspelt widget passed the same way. Check now replays the documents the script creates, expanding each `use building block` and `use fragment` as exec does (prefix included), and refuses a target they do not hold at that point as MDL-ALTER02, suggesting the nearest name (`did you mean a_container1`). On a stored page, the names a block inserted by `alter page … insert` are now its expanded names: `insert after s_container2` after inserting a block `as s_` is no longer refused, and `set … on s_text99` is no longer waived. Without a project a block cannot be expanded, so every name under its prefix is accepted (every name, for an unprefixed block), and a page holding DataGrid 2 columns or object-list items, whose addresses are derived, is not judged. The duplicate-name side (ako/mxcli#559) was already fixed. - **A pluggable widget's template is prepared once per widget type** (ako/mxcli#2160) — every data grid, filter and other pluggable widget a script builds loaded its template by deep-cloning the built-in one and merging the project's widget package into the clone, the same work each time. The merged template is now made once for each widget type and package and reused; each widget still gets IDs of its own. Creating 600 pages: 29.5 s before, 26.5 s after; re-running them settled: 83.1 s before, 73.9 s after. The pages written are the same: all 1,719 units of the two projects compare equal once IDs are set aside. - **`check` no longer decodes every microflow of the project before it starts** (mendixlabs/mxcli#1842, ako/mxcli#2160) — two of its checks, flow access (who may run a flow) and entity-access calls (MDL-SEC22), each read every microflow in full once per run, so a `check` of five statements cost seconds on a large project whatever the script held. They now read the few values they need from each stored flow, and decode a flow's body only when it contains a call. What `check` reports is unchanged. On a project of 5,373 units, with files already read once: `check` of 5 `create or modify microflow` statements 2.0 s before, 0.9 s after; of 200, 3.9 s before, 2.8 s after. +- **Comparing a write with the stored unit parses the documents seven times where it parsed them twelve** (ako/mxcli#2160) — every unit mxcli writes is first reconciled with the stored one: stored element ids and translations are carried over, and a write that changes nothing is dropped. Each step of that parsed both documents for itself. The steps that only read the stored document now share one parse, the ids are read without parsing, and the workflow-only step is skipped for a document that is not a workflow. The bytes written are the same: a script that changes 600 pages leaves the same 1,719 unit files, byte for byte, with the previous build and this one. Re-running 600 settled pages: 74.3 s before, 61.6 s after. - **A pluggable widget's own `caption`, `label`, `buttonStyle` and `icon` are the widget's, not the core properties of that name** (ako/mxcli#2180, mendixlabs/mxcli#1833) — PlatformDesignSystem's Button declares property keys named `caption`, `buttonStyle` and `icon`, its Form label one named `label`, and mxcli read those names as the core widget properties first. `alter page … set (caption: 'Save') on btn` failed with "widget has no Caption property"; `set (buttonStyle: 'outline')` printed "Altered page" and wrote nothing; `set (label: …)` wrote a core label beside the widget's own one, and `create page` did the same, so `describe` printed two labels; `check` refused the widget's own `buttonStyle: 'fillBlue'` as an unknown Mendix button style (MDL-WIDGET02) and its `icon: 'bookmark'` as "not a qualified reference"; and `create page` — and a `use building block` copy of a block holding one — dropped the `icon`. Where the widget's definition declares the key (not as a system property, as a combo box declares `Label`), the name now means that key in all three: ALTER writes it, `create page` stores it, and `check` judges the value against the widget's own enumeration. Core buttons, and pluggable widgets without such a key, are unchanged. Measured on Mendix 11.12.1 with a widget package shaped like the PDS Button: before, two `check` errors, a dropped icon and a second LabelTemplate per widget; after, `check` clean, every value stored in the widget's own property, `mx check` 0 errors. - **`describe page` loads the page it describes** (ako/mxcli#2160) — it loaded every page with the same local name, in whatever module, before picking the one asked for, and read the page's document twice. `Overview`, `NewEdit` and `Home` repeat across the modules of most projects, and rewriting a page describes the stored one up to three times, so a script that re-runs its pages paid for it per statement. Output is unchanged. Re-running a settled script of 600 `create or modify page` statements: 87.8 s before, 77.2 s after. - **The decode cache compares a unit's bytes once per run** (ako/mxcli#2160) — the cache that spares a script decoding the same unit again recognises the unit by its bytes. It kept the buffer of an earlier pass, so in the next pass every listing compared every unit of the type byte for byte: all of a project's pages, twice per page statement. It now keeps the buffer it matched. Re-running a settled script of 600 `create or modify page` statements: 103.7 s before, 86.8 s after. diff --git a/modelsdk/canon/canon.go b/modelsdk/canon/canon.go index f51c4a646..9e4efde70 100644 --- a/modelsdk/canon/canon.go +++ b/modelsdk/canon/canon.go @@ -67,11 +67,17 @@ func DigestMasking(raw []byte, mask map[string]bool) (string, error) { if err := bson.Unmarshal(raw, &doc); err != nil { return "", fmt.Errorf("unmarshal BSON (%d bytes): %w", len(raw), err) } + return digestOfDoc(doc, mask), nil +} + +// digestOfDoc is DigestMasking for a document already parsed. It only reads +// doc. +func digestOfDoc(doc bson.D, mask map[string]bool) string { ids := collectElementIDs(doc) var b strings.Builder renderValue(&b, doc, ids, mask) sum := sha256.Sum256([]byte(b.String())) - return hex.EncodeToString(sum[:]), nil + return hex.EncodeToString(sum[:]) } // collectElementIDs walks containment in a deterministic order and numbers every diff --git a/modelsdk/canon/identity.go b/modelsdk/canon/identity.go index 33c4dff79..6e1f4e47b 100644 --- a/modelsdk/canon/identity.go +++ b/modelsdk/canon/identity.go @@ -4,9 +4,12 @@ package canon import ( "bytes" + "fmt" "os" "go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore" + + "go.mongodb.org/mongo-driver/v2/bson" ) // Reconcile is ADR-0008 decision 1, as a single decision both storage engines @@ -131,8 +134,14 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b // version control as a whole-document replacement. TransplantIDs puts the // stored ids back on the elements that still correspond, rewriting every // reference with them. + // + // The stored document is parsed once here for the two steps that only read + // it, this pairing and the comparison at the end (parsedStored). The carries + // in between copy parts of the stored document into the new one and keep a + // parse of their own. + storedDoc := parseStored(stored) if !o.contentsOwnElementIDs { - contents = TransplantIDs(contents, stored) + contents = transplantIDs(contents, storedDoc) } // And the nested identity property the transplant does not cover: every @@ -152,6 +161,70 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b // (ako/mxcli#1897). contents = CarryUnexpressed(contents, stored) + if alwaysWrite() { + return contents, false + } + eq, err := equalToStored(contents, storedDoc) + if err != nil { + return contents, false + } + return contents, eq +} + +// reconcileInPasses is Reconcile as it was before its read-only steps shared a +// parse of the stored document: every step on the bytes alone. It is what +// Reconcile is defined by and what its test compares it with. +func reconcileInPasses(contents, stored []byte, opts ...Option) (out []byte, unchanged bool) { + if len(stored) == 0 { + return contents, false + } + var o reconcileOpts + for _, fn := range opts { + fn(&o) + } + + // Identity is carried unconditionally. MXCLI_ALWAYS_WRITE turns off *eliding* + // the write, not preserving what the document is: a forced write that + // re-minted StableId would renumber the deployed model's operation ids, which + // is a change to the app rather than a debugging aid. + contents = CarryIdentity(contents, stored) + + // The translations the statement had no way to say. MDL carries ONE string + // per text, so a rebuild drops every other language the document held — a + // describe→exec round-trip deleted a page's Dutch title from a real project. + // Runs before the transplant because it is the one carry that changes the + // document's length; the transplant patches fixed-width binaries in place and + // must see the final framing. + if !o.contentsOwnTranslations { + contents = CarryTranslations(contents, stored) + } + + // The same reasoning one level down, for the writes that do land: a rebuild + // mints a fresh $ID for every sub-element, so a two-line change reads in + // version control as a whole-document replacement. TransplantIDs puts the + // stored ids back on the elements that still correspond, rewriting every + // reference with them. + if !o.contentsOwnElementIDs { + contents = transplantIDsInPasses(contents, stored) + } + + // And the nested identity property the transplant does not cover: every + // Workflows$* element carries a PersistentId that both engines re-mint on + // every write, so without this a workflow document never equals itself and + // no-op elision could never fire for one (issue #949). + contents = carryPersistentIDsUnfiltered(contents, stored) + + // A property MDL cannot state: a tab container's default tab. The writer + // points it at the first tab, so a round trip rewrote a page whose stored + // default was null and reset any other choice (ako/mxcli#1897). Needs the + // transplanted $IDs, so it runs after TransplantIDs. + contents = CarryDefaultTabPages(contents, stored) + + // The rest of what MDL cannot state: an expression's surrounding whitespace + // and an empty screen-reader label Studio Pro stores as a ClientTemplate + // (ako/mxcli#1897). + contents = CarryUnexpressed(contents, stored) + if alwaysWrite() { return contents, false } @@ -162,6 +235,34 @@ func Reconcile(contents, stored []byte, opts ...Option) (out []byte, unchanged b return contents, eq } +// parsedStored is the stored document parsed for the steps of Reconcile that +// only read it. ok is false for bytes that are not a document, which those +// steps treat as they treat a failed parse of their own. +type parsedStored struct { + doc bson.D + ok bool +} + +func parseStored(stored []byte) parsedStored { + var doc bson.D + if err := bson.Unmarshal(stored, &doc); err != nil { + return parsedStored{} + } + return parsedStored{doc: doc, ok: true} +} + +// equalToStored is Equal(contents, stored) with the stored side already parsed. +func equalToStored(contents []byte, stored parsedStored) (bool, error) { + da, err := Digest(contents) + if err != nil { + return false, fmt.Errorf("canonicalise left: %w", err) + } + if !stored.ok { + return false, fmt.Errorf("canonicalise right: not a document") + } + return da == digestOfDoc(stored.doc, nil), nil +} + // alwaysWrite disables no-op elision. An escape hatch for bisecting a suspected // elision bug ("does it still reproduce if every write lands?") without building // a patched binary — not a supported knob. diff --git a/modelsdk/canon/persistentid.go b/modelsdk/canon/persistentid.go index 99f84eeaa..014b8a801 100644 --- a/modelsdk/canon/persistentid.go +++ b/modelsdk/canon/persistentid.go @@ -3,6 +3,7 @@ package canon import ( + "bytes" "go.mongodb.org/mongo-driver/v2/bson" "go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore" ) @@ -43,6 +44,24 @@ const persistentIDKey = "PersistentId" // // A document that cannot be read on either side passes through untouched. func CarryPersistentIDs(contents, stored []byte) []byte { + // Only a workflow's elements have the property. A pair needs it on the new + // side as a 16-byte binary, stored as this byte sequence: without it there + // is nothing to carry, and the two documents need not be parsed to find + // that out — as they were for every page, microflow and domain model + // written (ako/mxcli#2160). + if !bytes.Contains(contents, persistentIDElement) { + return contents + } + return carryPersistentIDsUnfiltered(contents, stored) +} + +// persistentIDElement is how a binary PersistentId property starts in a stored +// document: the binary type byte, the key, and the key's terminator. +var persistentIDElement = []byte("\x05" + persistentIDKey + "\x00") + +// carryPersistentIDsUnfiltered is CarryPersistentIDs without the check that +// the document has the property at all. +func carryPersistentIDsUnfiltered(contents, stored []byte) []byte { m := persistentIDMapping(contents, stored) if len(m) == 0 { return contents diff --git a/modelsdk/canon/reconcile_passes_test.go b/modelsdk/canon/reconcile_passes_test.go new file mode 100644 index 000000000..6a9b7cd4e --- /dev/null +++ b/modelsdk/canon/reconcile_passes_test.go @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "bytes" + "crypto/sha256" + "reflect" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// rebuilt is raw as a rebuild of the same document would encode it: every +// element with a new $ID, and every 16-byte binary that held one of those ids +// (a pointer to the element) following it. The new ids are derived from the +// old, so the result is the same on every run. +func rebuilt(t *testing.T, raw []byte) []byte { + t.Helper() + var doc bson.D + if err := bson.Unmarshal(raw, &doc); err != nil { + t.Fatalf("unmarshal: %v", err) + } + ids := map[string][]byte{} + var collect func(v any) + collect = func(v any) { + switch x := v.(type) { + case bson.D: + for _, e := range x { + if b, ok := e.Value.(bson.Binary); ok && e.Key == "$ID" && len(b.Data) == 16 { + sum := sha256.Sum256(append([]byte("rebuilt"), b.Data...)) + ids[string(b.Data)] = sum[:16] + } + collect(e.Value) + } + case bson.A: + for _, e := range x { + collect(e) + } + } + } + collect(doc) + var remint func(v any) any + remint = func(v any) any { + switch x := v.(type) { + case bson.D: + for i := range x { + x[i].Value = remint(x[i].Value) + } + case bson.A: + for i := range x { + x[i] = remint(x[i]) + } + case bson.Binary: + if to, ok := ids[string(x.Data)]; ok { + return bson.Binary{Subtype: x.Subtype, Data: append([]byte(nil), to...)} + } + } + return v + } + out, err := bson.Marshal(remint(doc)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return out +} + +// Reconcile is defined as its steps run on the bytes alone, each parsing what +// it reads (reconcileInPasses). It now parses the stored document once for the +// steps that only read it, skips the PersistentId carry for a document without +// the property, and checks the id substitution without parsing. It must return +// the same bytes and the same verdict: compared on every PedApp unit against +// itself, against a rebuild of itself (new ids throughout), against a rebuild +// that left properties out, and the other way round. +func TestReconcile_ReturnsWhatItsStepsReturnOnTheBytesAlone(t *testing.T) { + units := pedAppUnits(t) + compared, unchangedCount, changedBytes := 0, 0, 0 + for name, stored := range units { + pairs := [][2][]byte{ + {stored, stored}, + {rebuilt(t, stored), stored}, + {thinned(t, rebuilt(t, stored), 7), stored}, + {stored, thinned(t, rebuilt(t, stored), 7)}, + {withoutUserRoleGUIDs(t, rebuilt(t, stored)), stored}, + } + for i, pair := range pairs { + contents, against := pair[0], pair[1] + // The translations option is compared on the rebuild, where a carry + // has something to do. + optionSets := [][]Option{nil} + if i == 1 { + optionSets = append(optionSets, []Option{ContentsOwnTranslations()}) + } + for _, opts := range optionSets { + want, wantSame := reconcileInPasses(append([]byte(nil), contents...), against, opts...) + got, gotSame := Reconcile(append([]byte(nil), contents...), against, opts...) + compared++ + if gotSame != wantSame { + t.Fatalf("%s pair %d: unchanged %v, the steps on the bytes alone give %v", name, i, gotSame, wantSame) + } + if !bytes.Equal(got, want) { + t.Fatalf("%s pair %d: %d bytes, the steps on the bytes alone give %d different ones", name, i, len(got), len(want)) + } + if wantSame { + unchangedCount++ + } + if !bytes.Equal(want, contents) { + changedBytes++ + } + } + } + } + t.Logf("%d comparisons: %d judged unchanged, %d where the reconcile rewrote the contents", compared, unchangedCount, changedBytes) + // Controls: unchanged and changed verdicts, and reconciles that carried + // something onto the contents, must all have been compared. + if unchangedCount == 0 || unchangedCount == compared || changedBytes == 0 { + t.Fatalf("the comparison did not cover every outcome: %d unchanged of %d, %d rewritten", unchangedCount, compared, changedBytes) + } +} + +// The fixture holds no workflow, and a workflow's elements are the only ones +// with a PersistentId, so the carry Reconcile now skips for a document without +// the property is compared here on one that has it: a rebuild that minted new +// PersistentIds must get the stored ones back, by both routes. +func TestReconcile_StillCarriesPersistentIDs(t *testing.T) { + bin := func(b byte) bson.Binary { + return bson.Binary{Data: bytes.Repeat([]byte{b}, 16)} + } + workflow := func(rootID, actID, actPersistent byte) []byte { + raw, err := bson.Marshal(bson.D{ + {Key: "$ID", Value: bin(rootID)}, + {Key: "$Type", Value: "Workflows$Workflow"}, + {Key: "Name", Value: "W"}, + {Key: "Activities", Value: bson.A{int32(3), bson.D{ + {Key: "$ID", Value: bin(actID)}, + {Key: "$Type", Value: "Workflows$UserTask"}, + {Key: "Name", Value: "Task"}, + {Key: "PersistentId", Value: bin(actPersistent)}, + }}}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return raw + } + stored := workflow(1, 2, 3) + contents := workflow(11, 12, 13) + + want, wantSame := reconcileInPasses(append([]byte(nil), contents...), stored) + got, gotSame := Reconcile(append([]byte(nil), contents...), stored) + if !bytes.Equal(got, want) || gotSame != wantSame { + t.Fatalf("Reconcile gives %d bytes (unchanged %v), the steps on the bytes alone %d (unchanged %v)", len(got), gotSame, len(want), wantSame) + } + if !wantSame { + t.Fatal("control: a rebuild that differs only in minted ids was not judged unchanged, so the carry did not run") + } + if !bytes.Contains(got, bytes.Repeat([]byte{3}, 16)) || bytes.Contains(got, bytes.Repeat([]byte{13}, 16)) { + t.Error("the stored PersistentId was not carried onto the rebuild") + } +} + +// The ids the substitution check reads where they lie are the ids it read by +// parsing the document. +func TestElementIDSetOf_ReadsTheIDsParsingRead(t *testing.T) { + units := pedAppUnits(t) + total := 0 + for name, stored := range units { + for _, raw := range [][]byte{stored, rebuilt(t, stored), thinned(t, stored, 5)} { + want, wantOK := elementIDSetByParsing(raw) + got, gotOK := elementIDSetOf(raw) + if gotOK != wantOK || !reflect.DeepEqual(got, want) { + t.Fatalf("%s: read %d ids (ok %v), parsing reads %d (ok %v)", name, len(got), gotOK, len(want), wantOK) + } + total += len(want) + } + } + if total == 0 { + t.Fatal("control: no ids were read, so the comparison is of empty sets") + } + for _, junk := range [][]byte{nil, {}, []byte("not a document")} { + _, wantOK := elementIDSetByParsing(junk) + if _, gotOK := elementIDSetOf(junk); gotOK != wantOK { + t.Errorf("%d bytes that are not a document: ok %v, parsing gives ok %v", len(junk), gotOK, wantOK) + } + } +} diff --git a/modelsdk/canon/transplant.go b/modelsdk/canon/transplant.go index 6c92a1ad1..f727d4412 100644 --- a/modelsdk/canon/transplant.go +++ b/modelsdk/canon/transplant.go @@ -64,12 +64,35 @@ import ( // mapping and the contents pass through untouched, which is the behaviour that // existed before this function. func TransplantIDs(contents, stored []byte) []byte { + return transplantIDs(contents, parseStored(stored)) +} + +// transplantIDs is TransplantIDs with the stored document already parsed. It +// only reads the parse. +func transplantIDs(contents []byte, stored parsedStored) []byte { + if !stored.ok { + return contents + } + m := idMappingWith(contents, stored.doc) + if len(m) == 0 { + return contents + } + out := append([]byte(nil), contents...) + if !substituteIDs(out, m, elementIDSetOf) { + return contents + } + return out +} + +// transplantIDsInPasses is TransplantIDs as it was: both documents parsed +// here, and the substitution checked by parsing the document before and after. +func transplantIDsInPasses(contents, stored []byte) []byte { m := idMapping(contents, stored) if len(m) == 0 { return contents } out := append([]byte(nil), contents...) - if !substituteIDs(out, m) { + if !substituteIDs(out, m, elementIDSetByParsing) { return contents } return out @@ -79,11 +102,17 @@ func TransplantIDs(contents, stored []byte) []byte { // for every pair that is safe to apply. Entries that would be no-ops are kept so // the collision check can see them, and skipped when applied. func idMapping(contents, stored []byte) map[string][]byte { - var newDoc, oldDoc bson.D - if err := bson.Unmarshal(contents, &newDoc); err != nil { + var oldDoc bson.D + if err := bson.Unmarshal(stored, &oldDoc); err != nil { return nil } - if err := bson.Unmarshal(stored, &oldDoc); err != nil { + return idMappingWith(contents, oldDoc) +} + +// idMappingWith is idMapping with the stored document already parsed. +func idMappingWith(contents []byte, oldDoc bson.D) map[string][]byte { + var newDoc bson.D + if err := bson.Unmarshal(contents, &newDoc); err != nil { return nil } w := &pairer{pairs: map[string][]byte{}, claimed: map[string]bool{}} @@ -339,8 +368,8 @@ func dropCollisions(pairs map[string][]byte, newIDs map[string]int) map[string][ // copying out of it — a property of the library, not of this package. Rather // than assume it, the result is read back and checked against the id set the // substitution was supposed to produce. -func substituteIDs(raw []byte, m map[string][]byte) bool { - before, ok := elementIDsOf(raw) +func substituteIDs(raw []byte, m map[string][]byte, idsOf func([]byte) (map[string]bool, bool)) bool { + before, ok := idsOf(raw) if !ok { return false } @@ -361,7 +390,7 @@ func substituteIDs(raw []byte, m map[string][]byte) bool { return false } - after, ok := elementIDsOf(raw) + after, ok := idsOf(raw) if !ok || len(after) != len(want) { return false } @@ -373,6 +402,81 @@ func substituteIDs(raw []byte, m map[string][]byte) bool { return true } +// elementIDSetByParsing is the set of element ids of a document, read by +// parsing it: what elementIDSetOf is compared with. +func elementIDSetByParsing(raw []byte) (map[string]bool, bool) { + ids, ok := elementIDsOf(raw) + if !ok { + return nil, false + } + out := make(map[string]bool, len(ids)) + for id := range ids { + out[id] = true + } + return out, true +} + +// elementIDSetOf is the set of element ids of a document, read where they lie: +// every $ID of the document and of every document under it, a 16-byte binary +// as its UUID and a string as itself (elementID). ok is false for bytes that +// are not a document. +// +// The substitution check reads the ids before and after it patches. Parsing +// the whole document for each was two of the twelve parses a reconcile made +// (ako/mxcli#2160); the ids are all it needs, and they can be read without +// building the tree. +func elementIDSetOf(raw []byte) (map[string]bool, bool) { + if bson.Raw(raw).Validate() != nil { + return nil, false + } + out := map[string]bool{} + var walkDoc func(doc bsoncore.Document, isArray bool) + walkDoc = func(doc bsoncore.Document, isArray bool) { + elems, err := doc.Elements() + if err != nil { + return + } + id, hasID := "", false + for _, e := range elems { + v := e.Value() + switch v.Type { + case bsoncore.TypeEmbeddedDocument: + if d, ok := v.DocumentOK(); ok { + walkDoc(d, false) + } + case bsoncore.TypeArray: + if a, ok := v.ArrayOK(); ok { + walkDoc(bsoncore.Document(a), true) + } + } + if isArray || e.Key() != "$ID" { + continue + } + // The last $ID of a document wins, as it does in the map a parsed + // document becomes. + switch v.Type { + case bsoncore.TypeBinary: + if _, data, ok := v.BinaryOK(); ok && len(data) == 16 { + id, hasID = blobToUUID(data), true + } else { + hasID = false + } + case bsoncore.TypeString: + if str, ok := v.StringValueOK(); ok { + id, hasID = str, true + } + default: + hasID = false + } + } + if hasID { + out[id] = true + } + } + walkDoc(bsoncore.Document(raw), false) + return out, true +} + func elementIDsOf(raw []byte) (map[string]int, bool) { var d bson.D if err := bson.Unmarshal(raw, &d); err != nil {