From 96a1efc1901d922d4a37c43ce928de263408cd88 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:05:39 +0000 Subject: [PATCH] fix(mcp): ALTER MICROFLOW insert lands on Studio Pro 11.15 Every insert over --mcp failed on 11.15.0-rc.4 with `undefined is not an object (evaluating 'u.$Type')`. The splice sent the whole insert as one ped_update_document and addressed the elements it added in that same update by index. PED resolves a path against the document as it was before the update, so none of those sets reach a new element: a set below a new flow's line fails the whole update, and any other set (an activity's action, a flow's isErrorHandler) is accepted and lost. Separately, several index-less adds to one array are stored in reverse and $id() resolves against that final order, so an insert of more than one element was wired to the wrong objects. The insert is now two updates. The first adds the new elements (constructor properties only, each at the stored length as its index, wired with $id()) and moves the stored ones. The second fills in the new elements: actions, conditions, loop sources, isErrorHandler and control vectors (0;0 is the schema default and is not sent). A failed second update leaves the insert incomplete in Studio Pro, the same contract as CreateMicroflow's create-then-set; the error says so. Verified live on 11.15.0-rc.4: plain, IF/ELSE and note inserts match the default engine on every flow's ends, sides and control vectors and every object's position, and ped_check_errors is clean on two successive calls. Co-Authored-By: Claude Opus 5.5 --- ...mcp-splice-addresses-elements-it-adds.json | 1 + docs/03-development/PED_MCP_CAPABILITIES.md | 73 ++++++- ...cp-alter-insert-addresses-new-elements.mdl | 73 +++++++ mdl/backend/mcp/microflow_mutator.go | 108 ++++++---- mdl/backend/mcp/microflow_mutator_test.go | 192 +++++++++++++++++- 5 files changed, 396 insertions(+), 51 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-21-mcp-splice-addresses-elements-it-adds.json create mode 100644 mdl-examples/bug-tests/mcp-alter-insert-addresses-new-elements.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-mcp-splice-addresses-elements-it-adds.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-mcp-splice-addresses-elements-it-adds.json new file mode 100644 index 0000000000..1fa0d67e30 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-mcp-splice-addresses-elements-it-adds.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "On Studio Pro 11.15.0-rc.4 every `alter microflow … { insert after … begin … end; }` over --mcp failed: `ped_update_document … undefined is not an object (evaluating 'u.$Type')`, with or without a note; a plain one-activity insert reproduced it. With that set removed, the insert landed but ped_check_errors reported `No action defined` on the new activity, and an inserted IF/ELSE came back wired to the wrong objects (the activity before it led into the else-branch log).", "cause": "flowPatchOps sent the whole insert as one update and addressed the elements it added in that same update by index: /flows/N/line/ControlVector, /objectCollection/objects/N/action, …/relativeMiddlePoint, /flows/N/isErrorHandler. PED resolves a path against the document as it was before the update, so none of those reach the new element. A set below a new flow's line dereferences a line that is not there yet and fails the whole update. Any other set is accepted and lost, and so is the same property put in the constructor value (an ActionActivity's constructor takes only x/y/caption). Separately, several index-less appends to one array in one update are stored in reverse, and $id(/…/N) resolves against that final order, so with more than one new object every flow reference pointed at the wrong one.", "file": "mdl/backend/mcp/microflow_mutator.go (SaveUnit, flowPatchOps, newFlowLineOps)", "insight": "Two direct ped_update_document probes settle what an update can address, and they are cheaper than any reading of the code. First, add an element and set one of its properties in the same update, then read it back. Second, send the same set in a separate update. The difference is the whole bug. The separate-update set always landed: control vectors, action and isErrorHandler alike. So the fix is two updates. The first is all or nothing: the new elements, wired with $id(), and the stored elements moved to make room. The second fills in the new elements. A failed second update leaves an incomplete insert in Studio Pro, the same contract as CreateMicroflow's create-then-set. That is the price of 11.15, and the error says so. For ordering, give every add the array's stored length as its index; PED keeps adds at one shared index in listed order (its documented rule for inserting several elements at a position). The 1534 finding's 'an AnnotationFlow added before a SequenceFlow is stored after it' was this reversal, not something about annotation flows. A one-activity insert hides both bugs, because it adds one object and one flow, so verify with an insert of several elements (an IF/ELSE) and compare every flow's ends, sides and vectors against the same script run locally.", "refs": ["ako/mxcli#1534"]} diff --git a/docs/03-development/PED_MCP_CAPABILITIES.md b/docs/03-development/PED_MCP_CAPABILITIES.md index a880bf7a8d..b2e24eae81 100644 --- a/docs/03-development/PED_MCP_CAPABILITIES.md +++ b/docs/03-development/PED_MCP_CAPABILITIES.md @@ -597,15 +597,70 @@ servers. - The pre-11.14 (full) constructor's flows carry no `$Type`, so a note's line could only be read as a `SequenceFlow`. The backend refuses notes there rather than risk an execution edge out of a note. -- Within one `ped_update_document`, an `AnnotationFlow` added *before* a - `SequenceFlow` is stored *after* it. So the splice adds its new sequence flows - (and the sets that address them by index) first, and note lines last. -- **Open: ALTER MICROFLOW insert over MCP fails on 11.15.** Setting - `/flows/N/line/originControlVector` on a flow added in the same update fails the - whole update with `undefined is not an object (evaluating 'u.$Type')`. This - happened without any note: a plain one-activity insert failed, and so did a lone - `SequenceFlow` add followed by that set. Setting the flow's whole `line` instead - is accepted but **not applied**: the read-back has no control vectors. +- ~~Within one `ped_update_document`, an `AnnotationFlow` added *before* a + `SequenceFlow` is stored *after* it.~~ This was the general reversal described + below, not something about note lines. + +### Microflow canvas: what one update can address (measured live 2026-10-21, 11.15.0-rc.4) + +**Fixed: ALTER MICROFLOW insert over MCP failed on 11.15.** Every insert failed +with `undefined is not an object (evaluating 'u.$Type')`, with or without a note. +Direct `ped_update_document` probes on bare test documents found two rules behind +it. Both follow from the tool's own description: *"every `index` you provide refers +to the array as it exists BEFORE this call"*. + +- **An update cannot address an element it adds.** A path resolves against the + document as it was before the update, so `/flows/N/…` or `/objectCollection/objects/N/…` + for a new N does not reach the new element: + + | Same update as the add | Result | + |---|---| + | set `/flows/N/line/originControlVector` | **fails the whole update** (`u.$Type`): the line is not there yet | + | set `/flows/N/line` (whole `BezierCurve`), then a vector | fails the same way | + | set `/flows/N/line` (whole `BezierCurve`) alone | accepted, **not applied** | + | `line` or `isErrorHandler` in the `SequenceFlow` constructor value | accepted, **ignored** (the constructor has neither) | + | set `/flows/N/isErrorHandler` | accepted, **not applied**: the flow reads back as an ordinary one | + | set `/objectCollection/objects/N/action` or `…/relativeMiddlePoint` | accepted, **not applied**: "No action defined"; the position is the constructor's `x`/`y` | + | `action` in the `ActionActivity` constructor value | accepted, **ignored** (the constructor takes `x`, `y`, `caption` only) | + | add the flow in its element shape (`origin`, `line`, `isErrorHandler`) | refused: `NodeUnableToConvertElement, String, System.Guid`, so `$id()` is not resolved on that path | + + **Every one of these sets lands in a later update**: control vectors, + `isErrorHandler` and `action` alike, on a flow or activity the previous update + created. `$id(/…/N)` references *do* resolve to elements added earlier in the + same update. +- **Several adds to one array without an `index` are stored in reverse**, and + `$id()` resolves against that final order. Three objects added in one update came + back as 3, 2, 1, and two flows came back as 2, 1. An inserted IF/ELSE was + therefore wired to the wrong objects, while a one-activity insert (one object, one + flow) hid the problem. Adds given **one shared `index`** (the array's length before + the update) are stored in listed order, which is PED's documented rule for adding + several elements at one position. + +**The fix** (`flowPatchOps` / `SaveUnit`) sends an insert as **two updates**. The +first adds the new elements, constructor properties only, each at the stored length +as its `index` and wired with `$id()`, and moves the stored elements to make room. +The second fills in the new elements: actions, split conditions, loop sources, +`isErrorHandler` and control vectors (a `0;0` vector is the schema default and is +not sent). + +This gives up some of the one-update atomicity the splice relied on. The first +update is still all or nothing, but **a failed second update leaves the insert in +Studio Pro incomplete**. That is the same contract as `CreateMicroflow`, which +creates and then sets activities. The error says so, and `ped_check_errors` names +what is missing. There is no write that avoids it on 11.15: nothing in the update +that adds an activity can give it its action. A retry is refused by the live/local +index check, so it cannot insert twice. + +Verified live: plain, IF/ELSE and note inserts, each compared with the same script +run with the default engine. All flows matched on ends, sides and control vectors +(4, 9 and 6 flows), as did every object's position. The fills read back: actions, +the condition, `true`/`false` cases, and the note's caption and size. +`ped_check_errors` was clean on two successive calls. The script is +`mdl-examples/bug-tests/mcp-alter-insert-addresses-new-elements.mdl`. + +Still refused upstream: a custom error handler (`on error … begin … end error`) +over MCP. `isErrorHandler` is settable in a later update, so the splice marks such a +flow in its second update, but MDL cannot reach that path yet. ## Capability gaps (established 11.11, status re-checked each release) diff --git a/mdl-examples/bug-tests/mcp-alter-insert-addresses-new-elements.mdl b/mdl-examples/bug-tests/mcp-alter-insert-addresses-new-elements.mdl new file mode 100644 index 0000000000..a896bea361 --- /dev/null +++ b/mdl-examples/bug-tests/mcp-alter-insert-addresses-new-elements.mdl @@ -0,0 +1,73 @@ +mdl 1; +-- ============================================================================ +-- ALTER MICROFLOW insert over --mcp lands on Studio Pro 11.15 +-- ============================================================================ +-- +-- The insert reads the stored flow from the local project, so the local .mpr +-- must hold the same base document as Studio Pro. Run part 1 over --mcp +-- (`--mcp http://localhost/mcp --mcp-dial :`, with -p pointing at +-- the local project) BEFORE running it locally: the MCP create checks the +-- local project for an existing document. Then run part 2 the same two ways +-- and compare every flow's ends, connection sides and control vectors (the +-- local .mpr via `mxcli bson dump`, Studio Pro's document via +-- ped_read_document). They must match, and ped_check_errors must be clean. +-- +-- Before the fix, on 11.15.0-rc.4: +-- * every insert failed with `undefined is not an object (evaluating +-- 'u.$Type')` — the update set a control vector on a flow it added itself; +-- * without that set, the new activity had "No action defined": PED drops a +-- set on an element added in the same update; +-- * the IF/ELSE was wired to the wrong objects, because several adds to one +-- array in one update are stored in reverse. +-- After the fix, all 4 + 9 + 6 flows below match the default engine. +-- +-- Studio Pro's MCP server has no delete, so the documents stay in the open +-- project after a run. +-- ============================================================================ + +-- Part 1: the base documents. + +create microflow MyFirstModule.MCPA_XPlain () +begin + log info node 'MCPA' 'one'; + log info node 'MCPA' 'two'; +end; + +create microflow MyFirstModule.MCPA_XIf () +begin + log info node 'MCPA' 'one'; + log info node 'MCPA' 'two'; +end; + +create microflow MyFirstModule.MCPA_XNote () +begin + log info node 'MCPA' 'one'; + log info node 'MCPA' 'two'; +end; + +-- Part 2: the inserts. + +alter microflow MyFirstModule.MCPA_XPlain { + insert after log node 'MCPA' 'one' begin + log info node 'MCPA' 'inserted'; + end; +}; + +alter microflow MyFirstModule.MCPA_XIf { + insert after log node 'MCPA' 'one' begin + declare $N INTEGER = 1; + if $N > 0 then + log info node 'MCPA' 'positive'; + else + log info node 'MCPA' 'not positive'; + end if; + end; +}; + +alter microflow MyFirstModule.MCPA_XNote { + insert after log node 'MCPA' 'one' begin + @annotation 'Inserted with a note' + log info node 'MCPA' 'inserted'; + log info node 'MCPA' 'and another'; + end; +}; diff --git a/mdl/backend/mcp/microflow_mutator.go b/mdl/backend/mcp/microflow_mutator.go index 730569ce4f..d8cf0b029a 100644 --- a/mdl/backend/mcp/microflow_mutator.go +++ b/mdl/backend/mcp/microflow_mutator.go @@ -25,7 +25,8 @@ import ( // takes no unit bytes, so Save diffs the spliced document against the stored // one and sends the difference as ped_update_document path operations on the // live document: positions set, new objects and flows added, rewired flow ends -// set, removed elements removed. PED addresses list entries by index, and those +// set, removed elements removed — and, in a second update, the new elements +// filled in (flowPatchOps says why). PED addresses list entries by index, and those // indexes are the stored order — so before anything is sent, the live document // is read back and compared with the local one, and a mismatch (the .mpr is // behind Studio Pro) refuses the write rather than patching the wrong element. @@ -203,7 +204,7 @@ func (d *mcpFlowDeps) SaveUnit(_ string, contents []byte) error { if err := bson.Unmarshal(contents, &spliced); err != nil { return err } - ops, err := d.b.flowPatchOps(d, spliced) + ops, fill, err := d.b.flowPatchOps(d, spliced) if err != nil { return err } @@ -221,10 +222,21 @@ func (d *mcpFlowDeps) SaveUnit(_ string, contents []byte) error { return fmt.Errorf("%s: refusing to send a removal over MCP", d.qn) } } - // One update: PED applies it all or, on a failure, nothing. + // Two updates, because PED cannot address an element in the update that + // adds it (see flowPatchOps). The first is all or nothing: the new + // elements, wired in, and the stored ones moved to make room. The second + // fills in the new elements. If it fails, the first is not rolled back: + // the insert is left in Studio Pro without what the second carried, as a + // create is when setting its activities fails (CreateMicroflow), and + // ped_check_errors names what is missing. if err := d.b.pedUpdateDoc(d.docType, d.qn, ops...); err != nil { return err } + if len(fill) > 0 { + if err := d.b.pedUpdateDoc(d.docType, d.qn, fill...); err != nil { + return fmt.Errorf("the insert into %s was added in Studio Pro, but setting up its new activities and flows failed (the insert is left in Studio Pro without them): %w", d.qn, err) + } + } return d.b.pedCheckDocument(d.docType, d.qn) } @@ -280,6 +292,22 @@ func pedVector(s string) map[string]int { return map[string]int{"width": p["x"], "height": p["y"]} } +// newFlowLineOps sets the control vectors of a sequence flow the insert added, +// at fp. A 0;0 vector is the line's schema default and is left out. +func newFlowLineOps(fp string, f *microflows.SequenceFlow) []pedOpEntry { + var ops []pedOpEntry + for _, v := range []struct{ key, vec string }{ + {"originControlVector", f.OriginControlVector}, + {"destinationControlVector", f.DestinationControlVector}, + } { + if v.vec == "" || v.vec == "0;0" { + continue + } + ops = append(ops, pedOpEntry{Path: fp + "/line/" + v.key, Operation: pedOperation{Type: "set", Value: pedVector(v.vec)}}) + } + return ops +} + // pedQualifiedFlowType maps the storage $Type of a flow object to the // qualified name PED reports for it, where the two differ (CLAUDE.md, "BSON // Storage Names vs Qualified Names"). @@ -292,8 +320,26 @@ var pedQualifiedFlowType = map[string]string{ // everything that uses a stored index (positions, rewired flows) and every // append goes first, while indexes are still the stored ones; removals go // last, highest index first. -func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, error) { - var ops []pedOpEntry +// +// They come in two updates. PED resolves a path against the document as it +// was before the update — its own rule for indexes — so an element the update +// adds cannot be addressed in it (measured on 11.15.0-rc.4). A set below a new +// flow's line fails the whole update with `undefined is not an object +// (evaluating 'u.$Type')`; any other set on a new element — an activity's +// action, its position, a flow's isErrorHandler or whole line — is accepted +// and then missing on read-back, and so is the same property put in the +// constructor value. The same sets land in a later update. So ops adds the new +// elements (constructor properties only, references as $id(), which do +// resolve) and changes the stored ones; fill sets the rest on the new elements, +// for SaveUnit to send once they exist. +// +// Every add carries the array's stored length as its index. Appended without +// one, several elements added to the same array in one update are stored in +// reverse (measured on 11.15.0-rc.4: three objects, two flows), and $id() +// resolves against that final order — so an insert of more than one element +// was wired to the wrong objects. Added at one shared index they are stored as +// listed, which is PED's own rule for adding several elements at a position. +func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) (ops, fill []pedOpEntry, err error) { set := func(path string, v any) { ops = append(ops, pedOpEntry{Path: path, Operation: pedOperation{Type: "set", Value: v}}) } @@ -325,25 +371,25 @@ func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, er for j, id := range added { path[id] = fmt.Sprintf("/objectCollection/objects/%d", len(oldObjIDs)+j) } + objectsEnd, flowsEnd := len(oldObjIDs), len(oldFlowIDs) for _, id := range added { obj, ok := d.objects[id] if !ok { - return nil, fmt.Errorf("internal: spliced object %s was not serialized", id) + return nil, nil, fmt.Errorf("internal: spliced object %s was not serialized", id) } idPath := map[model.ID]string{} m, err := b.mapObjectTree(obj, path[id], idPath) if err != nil { - return nil, err + return nil, nil, err } - var sets []pedOpEntry skeletonObject(m, path[id], func(p string, v any) { - sets = append(sets, pedOpEntry{Path: p, Operation: pedOperation{Type: "set", Value: v}}) + fill = append(fill, pedOpEntry{Path: p, Operation: pedOperation{Type: "set", Value: v}}) }) - ops = append(ops, pedOpEntry{Path: "/objectCollection/objects", Operation: pedOperation{Type: "add", Value: m}}) - ops = append(ops, sets...) - // The skeleton constructor ignores a position; set it on the stored element. + ops = append(ops, pedOpEntry{Path: "/objectCollection/objects", Operation: pedOperation{Type: "add", Value: m, Index: &objectsEnd}}) + // The constructor's x/y place it on 11.15; the set is for a server + // whose skeleton constructor ignores them. p := obj.GetPosition() - set(path[id]+"/relativeMiddlePoint", map[string]int{"x": p.X, "y": p.Y}) + fill = append(fill, pedOpEntry{Path: path[id] + "/relativeMiddlePoint", Operation: pedOperation{Type: "set", Value: map[string]int{"x": p.X, "y": p.Y}}}) for nested, np := range idPath { path[string(nested)] = np } @@ -376,7 +422,7 @@ func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, er if pointerID(o, end+"Pointer") != pointerID(n, end+"Pointer") { r, err := ref(pointerID(n, end+"Pointer")) if err != nil { - return nil, err + return nil, nil, err } set(fp+"/"+key, r) } @@ -390,10 +436,9 @@ func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, er } } } - // Sequence flows are added before note lines. Within one update PED stores - // an AnnotationFlow after a SequenceFlow added later (measured on - // 11.15.0-rc.4), so the sets below address a new sequence flow by the - // right index only when no note line was added ahead of it. + // Sequence flows are added before note lines, the order a stored document + // has them in, so fill addresses the new sequence flows from the stored + // length up. var addedNotes []*microflows.AnnotationFlow next := len(oldFlowIDs) for _, id := range addedFlows { @@ -405,46 +450,41 @@ func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, er next++ f, ok := d.flows[id] if !ok { - return nil, fmt.Errorf("internal: spliced flow %s was not serialized", id) + return nil, nil, fmt.Errorf("internal: spliced flow %s was not serialized", id) } o, err := ref(string(f.OriginID)) if err != nil { - return nil, err + return nil, nil, err } dst, err := ref(string(f.DestinationID)) if err != nil { - return nil, err + return nil, nil, err } v := map[string]any{"$Type": "Microflows$SequenceFlow", "originId": o, "destinationId": dst} setPEDSides(v, f) cv, err := mapCaseValue(f.CaseValue) if err != nil { - return nil, err + return nil, nil, err } if cv != nil { v["caseValue"] = cv } - ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: v}}) + ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: v, Index: &flowsEnd}}) if f.IsErrorHandler { - set(fp+"/isErrorHandler", true) - } - if f.OriginControlVector != "" { - set(fp+"/line/originControlVector", pedVector(f.OriginControlVector)) - } - if f.DestinationControlVector != "" { - set(fp+"/line/destinationControlVector", pedVector(f.DestinationControlVector)) + fill = append(fill, pedOpEntry{Path: fp + "/isErrorHandler", Operation: pedOperation{Type: "set", Value: true}}) } + fill = append(fill, newFlowLineOps(fp, f)...) } for _, af := range addedNotes { o, err := ref(string(af.OriginID)) if err != nil { - return nil, err + return nil, nil, err } dst, err := ref(string(af.DestinationID)) if err != nil { - return nil, err + return nil, nil, err } - ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: pedAnnotationFlow(af, o, dst)}}) + ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: pedAnnotationFlow(af, o, dst), Index: &flowsEnd}}) } for i := len(oldFlowIDs) - 1; i >= 0; i-- { @@ -459,7 +499,7 @@ func (b *Backend) flowPatchOps(d *mcpFlowDeps, spliced bson.D) ([]pedOpEntry, er ops = append(ops, pedOpEntry{Path: "/objectCollection/objects", Operation: pedOperation{Type: "remove", Index: &idx}}) } } - return ops, nil + return ops, fill, nil } // checkLiveFlowMatches compares the live document's objects and flows with the diff --git a/mdl/backend/mcp/microflow_mutator_test.go b/mdl/backend/mcp/microflow_mutator_test.go index 72b3f6825b..afce262f30 100644 --- a/mdl/backend/mcp/microflow_mutator_test.go +++ b/mdl/backend/mcp/microflow_mutator_test.go @@ -103,7 +103,8 @@ func logFragment() *backend.MicroflowFragment { // The splice becomes PED path operations addressed by the stored indexes: // the moved end, the new activity (added bare, then its action and position // set, as PED's skeleton constructor requires), the rewired flow's end, and -// the new flow — and no removal. +// the new flow — and no removal. Which of them wait for the second update is +// TestFlowPatchOps_NewElementsAreFilledInOnceTheyExist. func TestFlowPatchOps_InsertAfter(t *testing.T) { var sent []any f := newFakePED(t, func(name string, args map[string]any) (string, bool) { @@ -131,10 +132,10 @@ func TestFlowPatchOps_InsertAfter(t *testing.T) { if err := m.Save(); err != nil { t.Fatalf("save: %v", err) } - if len(sent) != 1 { - t.Fatalf("want one ped_update_document, got %d", len(sent)) + if len(sent) != 2 { + t.Fatalf("want the insert and its fill, got %d updates", len(sent)) } - js, _ := json.Marshal(sent[0]) + js, _ := json.Marshal(append(sent[0].([]any), sent[1].([]any)...)) got := string(js) var ops []pedOpEntry if err := json.Unmarshal(js, &ops); err != nil { @@ -209,8 +210,8 @@ func TestFlowPatchOps_InsertNote(t *testing.T) { if err := m.Save(); err != nil { t.Fatalf("save: %v", err) } - if len(sent) != 1 { - t.Fatalf("want one ped_update_document, got %d", len(sent)) + if len(sent) != 2 { + t.Fatalf("want the insert and its fill, got %d updates", len(sent)) } js, _ := json.Marshal(sent[0]) var ops []pedOpEntry @@ -231,8 +232,8 @@ func TestFlowPatchOps_InsertNote(t *testing.T) { } } // PED stores a note line after a sequence flow added later in the same - // update, so the new sequence flow (and the sets addressing it as - // /flows/3) must come first. + // update, so the new sequence flow must come first: the fill that follows + // addresses it as /flows/3. if lineAt < lastSequenceOp { t.Errorf("the note line (op %d) is added before the new sequence flow's ops (last at %d):\n%s", lineAt, lastSequenceOp, js) } @@ -254,6 +255,181 @@ func TestFlowPatchOps_InsertNote(t *testing.T) { } } +// reducePED fakes Studio Pro holding storedReduce, and records each +// ped_update_document's operations. +func reducePED(t *testing.T, sent *[][]pedOpEntry) *Backend { + t.Helper() + f := newFakePED(t, func(name string, args map[string]any) (string, bool) { + switch name { + case "ped_read_document": + return `{"results":[{"path":"/objectCollection/objects","result":[` + + `{"$Type":"Microflows$StartEvent","relativeMiddlePoint":{"x":-98,"y":200}},` + + `{"$Type":"Microflows$EndEvent","relativeMiddlePoint":{"x":564,"y":200}},` + + `{"$Type":"Microflows$ActionActivity","relativeMiddlePoint":{"x":214,"y":200}},` + + `{"$Type":"Microflows$ActionActivity","relativeMiddlePoint":{"x":404,"y":200}}]},` + + `{"path":"/flows","result":[{},{},{}]}]}`, false + case "ped_update_document": + js, _ := json.Marshal(args["operations"]) + var ops []pedOpEntry + if err := json.Unmarshal(js, &ops); err != nil { + t.Fatal(err) + } + *sent = append(*sent, ops) + return "SUCCESS: All operations have been performed successfully.", false + case "ped_check_errors": + return "No errors found.", false + } + return "SUCCESS", false + }) + return &Backend{client: f.connectClient(t)} +} + +// opsAt maps the paths an update sets to their JSON values. +func opsAt(ops []pedOpEntry) map[string]string { + got := map[string]string{} + for _, op := range ops { + if op.Operation.Type == "set" { + v, _ := json.Marshal(op.Operation.Value) + got[op.Path] = string(v) + } + } + return got +} + +// PED resolves a path against the document as it was before the update, so +// an element the update adds cannot be addressed in it (measured on +// 11.15.0-rc.4): a set below a new flow's line fails the whole update with +// `undefined is not an object (evaluating 'u.$Type')`, which made every insert +// fail, and a set of a new activity's action is accepted and lost, which left +// it with "No action defined". So the update that adds an element addresses +// nothing on it, and a second update fills it in. +func TestFlowPatchOps_NewElementsAreFilledInOnceTheyExist(t *testing.T) { + var sent [][]pedOpEntry + m, _ := openFlowMutator(t, reducePED(t, &sent)) + if err := m.InsertAfter(model.ID(mfUUID("a")), logFragment()); err != nil { + t.Fatal(err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + if len(sent) != 2 { + t.Fatalf("want the insert and then its fill, got %d updates: %v", len(sent), sent) + } + // The stored document has four objects and three flows, so the insert + // adds /objectCollection/objects/4 and /flows/3. + for _, op := range sent[0] { + if strings.HasPrefix(op.Path, "/objectCollection/objects/4/") || strings.HasPrefix(op.Path, "/flows/3/") { + t.Errorf("the update that adds the element also sets %s on it", op.Path) + } + } + got := opsAt(sent[1]) + if !strings.Contains(got["/objectCollection/objects/4/action"], `"Microflows$LogMessageAction"`) { + t.Errorf("the fill does not set the new activity's action: %v", got) + } + for p, v := range map[string]string{ + // The new flow leaves the activity on the right… + "/flows/3/line/originControlVector": `{"height":0,"width":15}`, + // …and keeps the end the rewired flow had at b. + "/flows/3/line/destinationControlVector": `{"height":0,"width":-30}`, + } { + if got[p] != v { + t.Errorf("%s = %q, want %s", p, got[p], v) + } + } + for p := range got { + if !strings.HasPrefix(p, "/objectCollection/objects/4/") && !strings.HasPrefix(p, "/flows/3/") { + t.Errorf("the fill sets %s, which the first update could address", p) + } + } +} + +// Several elements appended to one array in one update are stored in reverse, +// and $id() resolves against that final order (measured on 11.15.0-rc.4): an +// IF inserted over MCP was wired to the wrong objects. Every add therefore +// carries the stored length as its index, which keeps them in listed order. +func TestFlowPatchOps_AddsKeepTheirListedOrder(t *testing.T) { + var sent [][]pedOpEntry + m, _ := openFlowMutator(t, reducePED(t, &sent)) + frag := logFragment() + second := logFragment().Objects[0] + second.SetPosition(model.Point{X: 520, Y: 200}) + frag.Objects = append(frag.Objects, second) + frag.Flows = []*microflows.SequenceFlow{{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: frag.Entry, DestinationID: second.GetID(), + OriginConnectionIndex: 1, DestinationConnectionIndex: 3, + }} + frag.Exit = second.GetID() + if err := m.InsertAfter(model.ID(mfUUID("a")), frag); err != nil { + t.Fatal(err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + adds := map[string]int{} + for _, op := range sent[0] { + if op.Operation.Type != "add" { + continue + } + adds[op.Path]++ + want := map[string]int{"/objectCollection/objects": 4, "/flows": 3}[op.Path] + if op.Operation.Index == nil || *op.Operation.Index != want { + t.Errorf("add to %s has index %v, want the stored length %d", op.Path, op.Operation.Index, want) + } + } + if adds["/objectCollection/objects"] != 2 || adds["/flows"] != 2 { + t.Errorf("adds = %v, want two objects and two flows", adds) + } +} + +// A 0;0 control vector is the line's schema default, which PED asks not to be +// sent. +func TestNewFlowLineOps_LeavesOutDefaults(t *testing.T) { + ops := newFlowLineOps("/flows/4", µflows.SequenceFlow{OriginControlVector: "0;0", DestinationControlVector: "0;-15"}) + if len(ops) != 1 || ops[0].Path != "/flows/4/line/destinationControlVector" { + t.Errorf("ops = %+v, want only the destination vector", ops) + } + if ops := newFlowLineOps("/flows/4", µflows.SequenceFlow{OriginControlVector: "0;0"}); len(ops) != 0 { + t.Errorf("default vectors produced %+v", ops) + } +} + +// An error handler flow is marked in the fill too. Set in the update that adds +// the flow, isErrorHandler is accepted and the flow reads back as an ordinary +// one (measured on 11.15.0-rc.4) — a second normal exit from the activity. +func TestFlowPatchOps_NewErrorHandlerFlowIsMarkedOnceItExists(t *testing.T) { + var sent [][]pedOpEntry + m, _ := openFlowMutator(t, reducePED(t, &sent)) + frag := logFragment() + handler := logFragment().Objects[0] + handler.SetPosition(model.Point{X: 360, Y: 300}) + frag.Objects = append(frag.Objects, handler) + frag.Flows = []*microflows.SequenceFlow{{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: frag.Entry, DestinationID: handler.GetID(), + OriginConnectionIndex: 2, IsErrorHandler: true, + }} + if err := m.InsertAfter(model.ID(mfUUID("a")), frag); err != nil { + t.Fatal(err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + if len(sent) != 2 { + t.Fatalf("want the insert and then its fill, got %d updates", len(sent)) + } + for _, op := range sent[0] { + if strings.HasSuffix(op.Path, "/isErrorHandler") { + t.Errorf("the update that adds the flow also marks it at %s", op.Path) + } + } + // The fragment's own flow is added first (/flows/3), then the one leaving + // the fragment (/flows/4). + if got := opsAt(sent[1]); got["/flows/3/isErrorHandler"] != "true" { + t.Errorf("the error handler flow is not marked in the fill: %v", got) + } +} + // PED addresses by index, so a live document that differs from the stored one // must refuse the write before anything is sent. func TestFlowPatchOps_StaleLiveDocumentIsRefused(t *testing.T) {