From 4580e851ceae9e0a30ab1b7c752f4a21dc2f2c1b Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 08:49:04 +0000 Subject: [PATCH 1/7] canon: a patch of the stored unit owns its element $IDs (ContentsOwnElementIDs, Writer.UpdateRawUnitPatch) TransplantIDs pairs elements by type and position, which re-pairs the surviving flows of a patched microflow onto their neighbours' $IDs after a drop. A write that started from the stored bytes skips it; elision and the storage-GUID guard still apply. Co-Authored-By: Claude Opus 5.5 --- modelsdk/canon/identity.go | 21 +++++++++++- modelsdk/canon/patch_ids_test.go | 56 ++++++++++++++++++++++++++++++++ modelsdk/mpr/writer_core.go | 11 +++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 modelsdk/canon/patch_ids_test.go diff --git a/modelsdk/canon/identity.go b/modelsdk/canon/identity.go index 8c9143d9b..8a8cb0c96 100644 --- a/modelsdk/canon/identity.go +++ b/modelsdk/canon/identity.go @@ -36,6 +36,23 @@ type Option func(*reconcileOpts) type reconcileOpts struct { contentsOwnTranslations bool contentsOwnStorageGUIDs bool + contentsOwnElementIDs bool +} + +// ContentsOwnElementIDs tells Reconcile that the write is a PATCH of the stored +// document, not a rebuild: every element that survived kept its stored $ID, and +// every new element carries a fresh one on purpose. So the structural transplant +// must not run. +// +// The transplant exists for rebuilds, whose elements all arrive with random +// $IDs and are paired back by type and position. On a patch that pairing is not +// a no-op but a hazard: drop one sequence flow and every flow after it pairs with +// its predecessor, taking that flow's $ID — identities move onto other nodes, +// which is what ADR-0012 measured the microflow rebuild doing (51 of 161). The +// graph splice of `alter microflow` (mfmutator) is the caller; it has its own +// guard that no reference is left dangling. Elision still applies. +func ContentsOwnElementIDs() Option { + return func(o *reconcileOpts) { o.contentsOwnElementIDs = true } } // ContentsOwnTranslations tells Reconcile that the write already accounts for @@ -114,7 +131,9 @@ 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. - contents = TransplantIDs(contents, stored) + if !o.contentsOwnElementIDs { + contents = TransplantIDs(contents, stored) + } // And the nested identity property the transplant does not cover: every // Workflows$* element carries a PersistentId that both engines re-mint on diff --git a/modelsdk/canon/patch_ids_test.go b/modelsdk/canon/patch_ids_test.go new file mode 100644 index 000000000..cae29e2f3 --- /dev/null +++ b/modelsdk/canon/patch_ids_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package canon + +import ( + "bytes" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// seqFlows builds a flow document whose Flows list holds one sequence flow per +// id in ids, each from origin i to origin i+1. Every flow has the same $Type +// and shape, which is exactly what makes the structural pairing positional. +func seqFlows(t *testing.T, ids ...byte) []byte { + t.Helper() + flows := bson.A{int32(3)} + for i, id := range ids { + flows = append(flows, bson.D{ + {Key: "$Type", Value: "Microflows$SequenceFlow"}, + {Key: "$ID", Value: bin(id)}, + {Key: "OriginPointer", Value: bin(byte(100 + i))}, + {Key: "DestinationPointer", Value: bin(byte(101 + i))}, + }) + } + return marshal(t, bson.D{ + {Key: "$Type", Value: "Microflows$Microflow"}, + {Key: "$ID", Value: bin(1)}, + {Key: "Flows", Value: flows}, + }) +} + +// A patch of the stored document — an `alter microflow` drop — keeps every +// surviving element's $ID already. The structural transplant must not then +// re-pair those elements: with one flow gone, the flows after it pair +// positionally with their predecessors and each takes its neighbour's $ID, +// moving identities onto other nodes (ADR-0012's "51 of 161 $IDs changed"). +func TestReconcile_PatchOwnsElementIDs(t *testing.T) { + stored := seqFlows(t, 10, 20, 30, 40) + patched := seqFlows(t, 10, 30, 40) // flow 20 dropped; 30 and 40 keep their $IDs + + // Control: the rebuild path pairs by position and moves $IDs, which is + // the defect the option exists for. + rebuilt, _ := Reconcile(patched, stored) + if ids := idSet(t, rebuilt); ids[blobToUUID(bin(40).Data)] { + t.Fatalf("control: expected the transplant to move $ID 40 onto another flow; the test would prove nothing") + } + + out, unchanged := Reconcile(patched, stored, ContentsOwnElementIDs()) + if unchanged { + t.Fatal("a patch that dropped a flow reported no change") + } + if !bytes.Equal(out, patched) { + t.Errorf("a patch that owns its element $IDs was rewritten by Reconcile") + } +} diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index 9d9e0b23c..ec92cb7b8 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -828,6 +828,17 @@ func (w *Writer) UpdateRawUnitOwningTranslations(unitID string, contents []byte) return w.updateUnit(unitID, contents, canon.ContentsOwnTranslations()) } +// UpdateRawUnitPatch is UpdateRawUnit for a write that PATCHED the stored bytes +// in place rather than rebuilding them — the graph splice of `alter microflow`. +// Such a write already carries every stored $ID and translation it means to +// keep, so neither is carried back: re-pairing element $IDs structurally would +// move identities onto other elements after a drop (canon.ContentsOwnElementIDs), +// and carrying translations would undo a deliberate removal. Elision and the +// storage-GUID guard apply as for every write. +func (w *Writer) UpdateRawUnitPatch(unitID string, contents []byte) error { + return w.updateUnit(unitID, contents, canon.ContentsOwnElementIDs(), canon.ContentsOwnTranslations()) +} + // UpdateRawUnitOwningStorageGUIDs is UpdateRawUnit for a write that deliberately // transplants storage GUIDs onto elements that keep their $ID — the marketplace // module update, which carries a module's existing GUIDs onto the documents From 85ac9cd9e7585af60e03ead5556457a93c7d2e07 Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 08:49:04 +0000 Subject: [PATCH 2/7] mfmutator: graph splice into the stored microflow (insert after/before, replace, drop) Splices a fragment into the raw stored unit: appends only the fragment's objects and flows, rewires the flow around the target by its pointers and the rewired end, moves nodes past the insertion point to make room, and never rewrites an $ID. Save refuses a unit in which anything still points at a removed element or two elements share an $ID. Refuses what it cannot do safely: after a decision, before a join, inside a loop body, drop/replace of a decision or of an activity with an error handler, and a placement that would overlap an object. The modelsdk backend writes through UpdateRawUnitPatch (canon.Reconcile). Co-Authored-By: Claude Opus 5.5 --- mdl/backend/backend.go | 1 + mdl/backend/mfmutator/splice.go | 939 ++++++++++++++++++ mdl/backend/mfmutator/splice_test.go | 354 +++++++ mdl/backend/microflow_mutation.go | 49 + mdl/backend/mock/backend.go | 3 + mdl/backend/mock/mock_mutation.go | 11 + .../modelsdk/microflow_mutator_write.go | 91 ++ mdl/backend/modelsdk/unimplemented_gen.go | 14 + 8 files changed, 1462 insertions(+) create mode 100644 mdl/backend/mfmutator/splice.go create mode 100644 mdl/backend/mfmutator/splice_test.go create mode 100644 mdl/backend/microflow_mutation.go create mode 100644 mdl/backend/modelsdk/microflow_mutator_write.go diff --git a/mdl/backend/backend.go b/mdl/backend/backend.go index a57e80e3a..f9a0cd760 100644 --- a/mdl/backend/backend.go +++ b/mdl/backend/backend.go @@ -37,5 +37,6 @@ type FullBackend interface { AgentEditorBackend PageMutationBackend WorkflowMutationBackend + MicroflowMutationBackend WidgetBuilderBackend } diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go new file mode 100644 index 000000000..afc303b30 --- /dev/null +++ b/mdl/backend/mfmutator/splice.go @@ -0,0 +1,939 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +// # The graph splice (plan item 4.2b) +// +// An `alter microflow` operation edits the STORED document, never a rebuild of +// it. The rebuild (`UpdateMicroflow`, then re-pairing IDs by type and position) +// is what deleted merges, reset curves and moved $IDs onto other nodes on a +// Studio Pro-authored flow (ADR-0012, Context). So the splice works on the raw +// BSON tree of the unit as read from storage: +// +// - the fragment's objects are the only elements it builds, and they are +// appended to the collection that holds the target; +// - the sequence flows around the target are rewired by changing their +// pointers, connection sides and the control vector at the rewired end — +// every other property of a rewired flow (its $ID, case value, error-handler +// flag, the curve at its other end) is kept; +// - nodes are moved only to make room, and only by a translation; +// - nothing else is touched, and no $ID is ever rewritten. A removed element +// leaves no reference behind: Save refuses a document in which any binary +// still names an element that was removed (CLAUDE.md rule 1), and one in +// which two elements share an $ID. +// +// What the splice cannot do safely it refuses, with the reason: an insert +// after a decision (which branch?), before an activity several flows enter +// (which path?), a drop or replace of an activity with an error handler (the +// handler would be orphaned), and — for now — anything inside a loop body, +// whose coordinates are relative to the loop box. + +import ( + "bytes" + "fmt" + "strconv" + "strings" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// Deps supplies the engine-specific steps: turning a new object or flow into +// its stored BSON form, and writing the unit back. Everything between — which +// flows to rewire, where to place, what to remove — is decided here, once, for +// every storage engine. +type Deps interface { + SerializeObject(obj microflows.MicroflowObject) (bson.D, error) + SerializeSequenceFlow(f *microflows.SequenceFlow) (bson.D, error) + SerializeAnnotationFlow(f *microflows.AnnotationFlow) (bson.D, error) + // SaveUnit writes the patched unit. The implementation must go through the + // storage engine's reconciling write (canon.Reconcile), like every write. + SaveUnit(unitID string, contents []byte) error +} + +// Mutator splices fragments into one microflow or nanoflow unit. +type Mutator struct { + deps Deps + unitID model.ID + doc bson.D + // removed holds the $IDs of every element an operation took out, so Save + // can prove that nothing still points at one of them. + removed map[string]bool +} + +var _ backend.MicroflowMutator = (*Mutator)(nil) + +// New returns a Mutator over a decoded Microflows$Microflow or +// Microflows$Nanoflow unit. +func New(doc bson.D, unitID model.ID, deps Deps) (*Mutator, error) { + switch t := dString(doc, "$Type"); t { + case "Microflows$Microflow", "Microflows$Nanoflow": + default: + return nil, fmt.Errorf("unit %s is a %s, not a microflow or nanoflow", unitID, t) + } + if dDoc(doc, "ObjectCollection") == nil { + return nil, fmt.Errorf("unit %s has no object collection", unitID) + } + return &Mutator{deps: deps, unitID: unitID, doc: doc, removed: map[string]bool{}}, nil +} + +// Bytes returns the patched unit, after the integrity checks Save applies. +func (m *Mutator) Bytes() ([]byte, error) { + if err := m.checkIntegrity(); err != nil { + return nil, err + } + return bson.Marshal(m.doc) +} + +// Save writes the patched unit through Deps. +func (m *Mutator) Save() error { + out, err := m.Bytes() + if err != nil { + return err + } + return m.deps.SaveUnit(string(m.unitID), out) +} + +// --------------------------------------------------------------------------- +// The graph view +// --------------------------------------------------------------------------- + +// node is one object of the flow as stored. +type node struct { + id string // model ID form (types.BlobToUUID of the $ID) + typ string + doc bson.D + pos point + size point + // loop is the $ID of the loop whose body holds the node, "" at top level. + loop string +} + +// flowRef is one entry of the unit's Flows list. +type flowRef struct { + id string + typ string + doc bson.D + origin string + dest string + isErr bool +} + +func (f flowRef) isSequence() bool { return f.typ == "Microflows$SequenceFlow" } + +type graph struct { + nodes map[string]*node + // order is the nodes in storage order, so a shift visits them the same + // way every run. + order []*node + flows []flowRef +} + +func (m *Mutator) graph() *graph { + g := &graph{nodes: map[string]*node{}} + var walk func(oc bson.D, loop string) + walk = func(oc bson.D, loop string) { + for _, el := range arrayElements(dGet(oc, "Objects")) { + d, ok := el.(bson.D) + if !ok { + continue + } + n := &node{ + id: binaryID(dGet(d, "$ID")), + typ: dString(d, "$Type"), + doc: d, + pos: parsePoint(dString(d, "RelativeMiddlePoint")), + size: parsePoint(dString(d, "Size")), + loop: loop, + } + if n.id != "" { + g.nodes[n.id] = n + g.order = append(g.order, n) + } + if n.typ == "Microflows$LoopedActivity" { + if inner := dDoc(d, "ObjectCollection"); inner != nil { + walk(inner, n.id) + } + } + } + } + walk(dDoc(m.doc, "ObjectCollection"), "") + for _, el := range arrayElements(dGet(m.doc, "Flows")) { + d, ok := el.(bson.D) + if !ok { + continue + } + isErr, _ := dGet(d, "IsErrorHandler").(bool) + g.flows = append(g.flows, flowRef{ + id: binaryID(dGet(d, "$ID")), + typ: dString(d, "$Type"), + doc: d, + origin: binaryID(dGet(d, "OriginPointer")), + dest: binaryID(dGet(d, "DestinationPointer")), + isErr: isErr, + }) + } + return g +} + +// outgoing returns the sequence flows leaving id, split into normal and +// error-handler flows. +func (g *graph) outgoing(id string) (normal, errs []flowRef) { + for _, f := range g.flows { + if f.isSequence() && f.origin == id { + if f.isErr { + errs = append(errs, f) + } else { + normal = append(normal, f) + } + } + } + return normal, errs +} + +// incoming returns the sequence flows entering id. +func (g *graph) incoming(id string) []flowRef { + var out []flowRef + for _, f := range g.flows { + if f.isSequence() && f.dest == id { + out = append(out, f) + } + } + return out +} + +// annotationFlows returns the annotation flows attached to id. +func (g *graph) annotationFlows(id string) []flowRef { + var out []flowRef + for _, f := range g.flows { + if !f.isSequence() && (f.origin == id || f.dest == id) { + out = append(out, f) + } + } + return out +} + +func (g *graph) node(id model.ID) (*node, error) { + n, ok := g.nodes[string(id)] + if !ok { + return nil, fmt.Errorf("activity %s is not in the stored flow (was it dropped by an earlier operation?)", id) + } + if n.loop != "" { + return nil, fmt.Errorf("%s is inside a loop body; alter does not splice inside a loop yet — "+ + "address the loop itself, or rewrite the loop with create or modify", describeNode(n)) + } + return n, nil +} + +// --------------------------------------------------------------------------- +// Operations +// --------------------------------------------------------------------------- + +// InsertAfter splices frag onto the flow leaving target. +func (m *Mutator) InsertAfter(target model.ID, frag *backend.MicroflowFragment) error { + g := m.graph() + x, err := g.node(target) + if err != nil { + return err + } + normal, _ := g.outgoing(x.id) + switch { + case len(normal) == 0: + return fmt.Errorf("nothing follows %s: it ends the flow; insert before it instead", describeNode(x)) + case len(normal) > 1: + return fmt.Errorf("%s has %d outgoing flows (it is a decision): insert after it would not say which branch; "+ + "insert before the first activity of the branch instead", describeNode(x), len(normal)) + } + return m.spliceOnFlow(g, normal[0], frag) +} + +// InsertBefore splices frag onto the flow entering target. +func (m *Mutator) InsertBefore(target model.ID, frag *backend.MicroflowFragment) error { + g := m.graph() + y, err := g.node(target) + if err != nil { + return err + } + in := g.incoming(y.id) + switch { + case len(in) == 0: + return fmt.Errorf("no flow enters %s; there is no path to insert on", describeNode(y)) + case len(in) > 1: + return fmt.Errorf("%d flows enter %s: insert before it would not say on which path; "+ + "insert after one of its predecessors instead", len(in), describeNode(y)) + } + return m.spliceOnFlow(g, in[0], frag) +} + +// spliceOnFlow puts frag on flow f (origin X, destination Y): f keeps its +// origin end and now enters the fragment's entry; a new flow leaves the +// fragment's exit and enters Y where f used to. +func (m *Mutator) spliceOnFlow(g *graph, f flowRef, frag *backend.MicroflowFragment) error { + x, y := g.nodes[f.origin], g.nodes[f.dest] + if x == nil || y == nil { + return fmt.Errorf("flow %s points at an object that is not in the flow", f.id) + } + if x.loop != "" || y.loop != "" { + return fmt.Errorf("the flow runs inside a loop body; alter does not splice inside a loop yet") + } + fb, err := fragmentGeometry(frag) + if err != nil { + return err + } + ax, s, err := flowAxis(x, y) + if err != nil { + return err + } + + // Make room: the fragment plus a gap on either side has to fit between + // X's and Y's facing edges; what is missing is added by moving everything + // past the midpoint of that gap further along the axis. + a := x.pos.get(ax) + s*x.size.get(ax)/2 + b := y.pos.get(ax) - s*y.size.get(ax)/2 + need := fb.length(ax) + 2*minGap + if deficit := need - s*(b-a); deficit > 0 { + m.shift(g, ax, s, float64(a+b)/2, s*deficit, "") + b += s * deficit + } + + // Place the fragment centred in the gap, its entry level with the flow. + cross := ax.other() + mid := point{} + mid = mid.set(ax, (a+b)/2) + mid = mid.set(cross, (x.pos.get(cross)+y.pos.get(cross))/2) + fb.placeCentred(ax, mid) + if err := fb.checkRoom(g, ""); err != nil { + return err + } + + entrySide, exitSide := side(ax, -s), side(ax, s) + destIdx, destVec := flowEnd(f.doc, "Destination") + if err := m.addFragment(frag); err != nil { + return err + } + // f now enters the fragment. + setPointer(f.doc, "DestinationPointer", frag.Entry) + setInt(f.doc, "DestinationConnectionIndex", entrySide) + setVector(f.doc, "DestinationControlVector", sideVector(entrySide)) + // And a new flow carries on from the fragment to Y. + return m.addFlow(µflows.SequenceFlow{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: frag.Exit, + DestinationID: model.ID(y.id), + OriginConnectionIndex: exitSide, + DestinationConnectionIndex: destIdx, + OriginControlVector: sideVector(exitSide), + DestinationControlVector: destVec, + }) +} + +// Replace puts frag where target is: every flow that entered target enters +// the fragment, the flow that left it leaves the fragment's exit, and +// annotations attached to target are attached to the fragment's entry. +func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) error { + g := m.graph() + x, err := g.node(target) + if err != nil { + return err + } + out, err := removable(g, x, "replace") + if err != nil { + return err + } + y := g.nodes[out.dest] + if y == nil { + return fmt.Errorf("flow %s points at an object that is not in the flow", out.id) + } + fb, err := fragmentGeometry(frag) + if err != nil { + return err + } + ax, s, err := flowAxis(x, y) + if err != nil { + return err + } + // The fragment starts where target started; if it is longer, everything + // past target moves along by the difference. + near := x.pos.get(ax) - s*x.size.get(ax)/2 + if extra := fb.length(ax) - x.size.get(ax); extra > 0 { + m.shift(g, ax, s, float64(x.pos.get(ax)), s*extra, x.id) + } + centre := point{} + centre = centre.set(ax, near+s*fb.length(ax)/2) + centre = centre.set(ax.other(), x.pos.get(ax.other())) + fb.placeCentred(ax, centre) + if err := fb.checkRoom(g, x.id); err != nil { + return err + } + + if err := m.addFragment(frag); err != nil { + return err + } + for _, in := range g.incoming(x.id) { + setPointer(in.doc, "DestinationPointer", frag.Entry) + } + setPointer(out.doc, "OriginPointer", frag.Exit) + for _, af := range g.annotationFlows(x.id) { + key := "DestinationPointer" + if af.origin == x.id { + key = "OriginPointer" + } + setPointer(af.doc, key, frag.Entry) + } + return m.removeObject(x) +} + +// Drop removes target and joins the flows that entered it to the object it +// led to. Annotation lines attached to it go with it; the annotations stay. +func (m *Mutator) Drop(target model.ID) error { + g := m.graph() + x, err := g.node(target) + if err != nil { + return err + } + out, err := removable(g, x, "drop") + if err != nil { + return err + } + destIdx, destVec := flowEnd(out.doc, "Destination") + destPtr := dGet(out.doc, "DestinationPointer") + for _, in := range g.incoming(x.id) { + if in.origin == out.dest { + return fmt.Errorf("dropping %s would leave a flow from an object to itself", describeNode(x)) + } + } + for _, in := range g.incoming(x.id) { + setBinary(in.doc, "DestinationPointer", destPtr) + setInt(in.doc, "DestinationConnectionIndex", destIdx) + setVector(in.doc, "DestinationControlVector", destVec) + } + drop := map[string]bool{out.id: true} + for _, af := range g.annotationFlows(x.id) { + drop[af.id] = true + } + m.removeFlows(drop) + return m.removeObject(x) +} + +// removable checks that x can be taken out of the flow and returns the one +// flow that leaves it. +func removable(g *graph, x *node, verb string) (flowRef, error) { + switch x.typ { + case "Microflows$ActionActivity", "Microflows$LoopedActivity": + default: + return flowRef{}, fmt.Errorf("cannot %s %s: only an activity or a loop can be, because a decision or an end event "+ + "changes the shape of the flow", verb, describeNode(x)) + } + normal, errs := g.outgoing(x.id) + if len(errs) > 0 { + return flowRef{}, fmt.Errorf("cannot %s %s: it has an error handler, which would be left with no activity; "+ + "rewrite the flow with create or modify instead", verb, describeNode(x)) + } + if len(normal) != 1 { + return flowRef{}, fmt.Errorf("cannot %s %s: it has %d outgoing flows, not one", verb, describeNode(x), len(normal)) + } + return normal[0], nil +} + +// --------------------------------------------------------------------------- +// Tree edits +// --------------------------------------------------------------------------- + +// addFragment serializes the fragment and appends it: its objects to the top +// level collection (appended, so no stored object changes list position), its +// flows to the Flows list. +func (m *Mutator) addFragment(frag *backend.MicroflowFragment) error { + oc := dDoc(m.doc, "ObjectCollection") + objs := arrayElements(dGet(oc, "Objects")) + for _, obj := range frag.Objects { + d, err := m.deps.SerializeObject(obj) + if err != nil { + return fmt.Errorf("serialize %T: %w", obj, err) + } + if d == nil { + return fmt.Errorf("cannot write a %T into a flow", obj) + } + objs = append(objs, d) + } + if !setArray(oc, "Objects", objs) { + return fmt.Errorf("the object collection has no Objects list") + } + for _, f := range frag.Flows { + if err := m.addFlow(f); err != nil { + return err + } + } + for _, af := range frag.AnnotationFlows { + d, err := m.deps.SerializeAnnotationFlow(af) + if err != nil { + return fmt.Errorf("serialize annotation flow: %w", err) + } + if err := m.appendFlowDoc(d); err != nil { + return err + } + } + return nil +} + +func (m *Mutator) addFlow(f *microflows.SequenceFlow) error { + d, err := m.deps.SerializeSequenceFlow(f) + if err != nil { + return fmt.Errorf("serialize sequence flow: %w", err) + } + return m.appendFlowDoc(d) +} + +func (m *Mutator) appendFlowDoc(d bson.D) error { + if d == nil { + return fmt.Errorf("a flow serialized to nothing") + } + flows := arrayElements(dGet(m.doc, "Flows")) + flows = append(flows, d) + if !setArray(m.doc, "Flows", flows) { + return fmt.Errorf("the unit has no Flows list") + } + return nil +} + +func (m *Mutator) removeFlows(ids map[string]bool) { + var keep []any + for _, el := range arrayElements(dGet(m.doc, "Flows")) { + if d, ok := el.(bson.D); ok && ids[binaryID(dGet(d, "$ID"))] { + m.markRemoved(d) + continue + } + keep = append(keep, el) + } + setArray(m.doc, "Flows", keep) +} + +// removeObject takes x out of the top-level collection. +func (m *Mutator) removeObject(x *node) error { + oc := dDoc(m.doc, "ObjectCollection") + var keep []any + found := false + for _, el := range arrayElements(dGet(oc, "Objects")) { + if d, ok := el.(bson.D); ok && binaryID(dGet(d, "$ID")) == x.id { + m.markRemoved(d) + found = true + continue + } + keep = append(keep, el) + } + if !found { + return fmt.Errorf("%s is not in the top-level collection", describeNode(x)) + } + setArray(oc, "Objects", keep) + return nil +} + +// markRemoved records the $ID of d and of every element nested in it. +func (m *Mutator) markRemoved(v any) { + switch t := v.(type) { + case bson.D: + for _, e := range t { + if e.Key == "$ID" { + if b, ok := e.Value.(primitive.Binary); ok { + m.removed[string(b.Data)] = true + } + continue + } + m.markRemoved(e.Value) + } + case bson.A: + for _, el := range t { + m.markRemoved(el) + } + } +} + +// shift moves every top-level node past the cut (along axis ax, in the +// direction of s) by delta. The sweep keeps everything on either side of the +// cut exactly as it was relative to its neighbours, so only the flows that +// cross the cut get longer; a flow's control vectors are relative to its ends, +// so no curve changes. skip is a node that is about to be removed. +func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { + for _, n := range g.order { + if n.loop != "" || n.id == skip { + continue + } + if float64(s)*(float64(n.pos.get(ax))-cut) <= 0 { + continue + } + n.pos = n.pos.set(ax, n.pos.get(ax)+delta) + dSet(n.doc, "RelativeMiddlePoint", n.pos.String()) + } +} + +// checkIntegrity is the rule-1 guard: no binary anywhere in the unit may +// still name an element that was removed, and no two elements may share an +// $ID. Either would make the document unopenable, and neither is caught by +// anything cheaper than Studio Pro. +func (m *Mutator) checkIntegrity() error { + seen := map[string]bool{} + var dup, dangling string + var walk func(v any, key string) + walk = func(v any, key string) { + switch t := v.(type) { + case bson.D: + for _, e := range t { + walk(e.Value, e.Key) + } + case bson.A: + for _, el := range t { + walk(el, key) + } + case primitive.Binary: + if len(t.Data) != 16 { + return + } + k := string(t.Data) + if key == "$ID" { + if seen[k] && dup == "" { + dup = types.BlobToUUID(t.Data) + } + seen[k] = true + return + } + if m.removed[k] && dangling == "" { + dangling = key + " -> " + types.BlobToUUID(t.Data) + } + } + } + walk(m.doc, "") + if dup != "" { + return fmt.Errorf("refusing to write: two elements would share $ID %s", dup) + } + if dangling != "" { + return fmt.Errorf("refusing to write: a reference still points at a removed element (%s)", dangling) + } + return nil +} + +// --------------------------------------------------------------------------- +// Geometry +// --------------------------------------------------------------------------- + +// minGap is the free space kept on each side of an inserted fragment, the +// edge-to-edge distance mxcli's own layout leaves between activities. +const minGap = 40 + +type axis int + +const ( + axisX axis = iota + axisY +) + +func (a axis) other() axis { return 1 - a } + +type point struct{ X, Y int } + +func (p point) get(a axis) int { + if a == axisX { + return p.X + } + return p.Y +} + +func (p point) set(a axis, v int) point { + if a == axisX { + p.X = v + } else { + p.Y = v + } + return p +} + +func (p point) String() string { return strconv.Itoa(p.X) + ";" + strconv.Itoa(p.Y) } + +func parsePoint(s string) point { + x, y, ok := strings.Cut(s, ";") + if !ok { + return point{} + } + px, _ := strconv.Atoi(strings.TrimSpace(x)) + py, _ := strconv.Atoi(strings.TrimSpace(y)) + return point{px, py} +} + +// flowAxis is the direction a flow from x to y runs: the axis along which the +// centres are further apart, and the sign along it. +func flowAxis(x, y *node) (axis, int, error) { + dx, dy := y.pos.X-x.pos.X, y.pos.Y-x.pos.Y + switch { + case dx == 0 && dy == 0: + return 0, 0, fmt.Errorf("%s and %s are drawn on the same spot; cannot tell which way the flow runs", describeNode(x), describeNode(y)) + case abs(dx) >= abs(dy): + return axisX, sign(dx), nil + default: + return axisY, sign(dy), nil + } +} + +// Connection indexes, as Mendix numbers the sides of a box. +const ( + sideTop = 0 + sideRight = 1 + sideBottom = 2 + sideLeft = 3 +) + +// side is the side of a box that faces direction s along ax. +func side(ax axis, s int) int { + switch { + case ax == axisX && s > 0: + return sideRight + case ax == axisX: + return sideLeft + case s > 0: + return sideBottom + default: + return sideTop + } +} + +// sideVector is the control vector Studio Pro draws a straight flow end with: +// perpendicular to the side, pointing out of the box. +func sideVector(sd int) string { + switch sd { + case sideTop: + return "0;-15" + case sideRight: + return "15;0" + case sideBottom: + return "0;15" + default: + return "-15;0" + } +} + +// fragmentBox is the fragment's layout as the builder produced it, to be +// translated into place. +type fragmentBox struct { + frag *backend.MicroflowFragment + min, max point +} + +func fragmentGeometry(frag *backend.MicroflowFragment) (*fragmentBox, error) { + if frag == nil || len(frag.Objects) == 0 || frag.Entry == "" || frag.Exit == "" { + return nil, fmt.Errorf("the fragment is empty") + } + fb := &fragmentBox{frag: frag} + first := true + for _, obj := range frag.Objects { + p, sz := obj.GetPosition(), objectSize(obj) + lo := point{p.X - sz.X/2, p.Y - sz.Y/2} + hi := point{p.X + sz.X/2, p.Y + sz.Y/2} + if first { + fb.min, fb.max, first = lo, hi, false + continue + } + fb.min = point{min(fb.min.X, lo.X), min(fb.min.Y, lo.Y)} + fb.max = point{max(fb.max.X, hi.X), max(fb.max.Y, hi.Y)} + } + return fb, nil +} + +func (fb *fragmentBox) length(ax axis) int { return fb.max.get(ax) - fb.min.get(ax) } + +// placeCentred translates the fragment so that its box is centred on c along +// ax, and its entry object sits on c across it. +func (fb *fragmentBox) placeCentred(ax axis, c point) { + var entry point + for _, obj := range fb.frag.Objects { + if obj.GetID() == fb.frag.Entry { + entry = point{obj.GetPosition().X, obj.GetPosition().Y} + } + } + d := point{} + d = d.set(ax, c.get(ax)-(fb.min.get(ax)+fb.max.get(ax))/2) + d = d.set(ax.other(), c.get(ax.other())-entry.get(ax.other())) + for _, obj := range fb.frag.Objects { + p := obj.GetPosition() + obj.SetPosition(model.Point{X: p.X + d.X, Y: p.Y + d.Y}) + } + fb.min = point{fb.min.X + d.X, fb.min.Y + d.Y} + fb.max = point{fb.max.X + d.X, fb.max.Y + d.Y} +} + +// checkRoom refuses a placement that would draw the fragment over an object +// that stays where it is. The shift clears the space past the cut; an object +// that already sat inside the gap (a branch drawn below the main line, say) +// is not moved, and drawing over it would hide it in Studio Pro. skip is the +// object a replace removes. +func (fb *fragmentBox) checkRoom(g *graph, skip string) error { + for _, n := range g.order { + if n.loop != "" || n.id == skip { + continue + } + lo := point{n.pos.X - n.size.X/2, n.pos.Y - n.size.Y/2} + hi := point{n.pos.X + n.size.X/2, n.pos.Y + n.size.Y/2} + if lo.X < fb.max.X && fb.min.X < hi.X && lo.Y < fb.max.Y && fb.min.Y < hi.Y { + return fmt.Errorf("there is no free room for the fragment: at (%d, %d)-(%d, %d) it would be drawn over %s; "+ + "move that object aside in Studio Pro first", fb.min.X, fb.min.Y, fb.max.X, fb.max.Y, describeNode(n)) + } + } + return nil +} + +func objectSize(obj microflows.MicroflowObject) point { + if s, ok := obj.(interface{ GetSize() model.Size }); ok { + sz := s.GetSize() + return point{sz.Width, sz.Height} + } + return point{} +} + +// flowEnd reads a flow's connection index and control vector at one end +// ("Origin" or "Destination"). +func flowEnd(d bson.D, end string) (int, string) { + idx := 0 + switch v := dGet(d, end+"ConnectionIndex").(type) { + case int32: + idx = int(v) + case int64: + idx = int(v) + case int: + idx = v + } + vec := "" + if line := dDoc(d, "Line"); line != nil { + vec = dString(line, end+"ControlVector") + } + return idx, vec +} + +func abs(v int) int { + if v < 0 { + return -v + } + return v +} + +func sign(v int) int { + if v < 0 { + return -1 + } + return 1 +} + +func describeNode(n *node) string { + name := strings.TrimPrefix(n.typ, "Microflows$") + if c := dString(n.doc, "Caption"); c != "" && name != "ActionActivity" { + name += " '" + c + "'" + } + return fmt.Sprintf("the %s at (%d, %d)", name, n.pos.X, n.pos.Y) +} + +// --------------------------------------------------------------------------- +// bson.D helpers. The unit is decoded with bson v1 into bson.D, whose nested +// documents are bson.D and whose arrays are bson.A with Mendix's leading +// int32 list marker. +// --------------------------------------------------------------------------- + +func dGet(d bson.D, key string) any { + for _, e := range d { + if e.Key == key { + return e.Value + } + } + return nil +} + +func dDoc(d bson.D, key string) bson.D { + v, _ := dGet(d, key).(bson.D) + return v +} + +func dString(d bson.D, key string) string { + v, _ := dGet(d, key).(string) + return v +} + +func dSet(d bson.D, key string, v any) bool { + for i := range d { + if d[i].Key == key { + d[i].Value = v + return true + } + } + return false +} + +// arrayElements returns the elements of a Mendix list, without its marker. +func arrayElements(v any) []any { + a, ok := v.(bson.A) + if !ok || len(a) == 0 { + return nil + } + if _, isMarker := a[0].(int32); isMarker { + return append([]any(nil), a[1:]...) + } + return append([]any(nil), a...) +} + +// setArray replaces a list's elements, keeping its stored marker. +func setArray(d bson.D, key string, elements []any) bool { + a, ok := dGet(d, key).(bson.A) + if !ok { + return false + } + out := bson.A{} + if len(a) > 0 { + if marker, isMarker := a[0].(int32); isMarker { + out = append(out, marker) + } + } + return dSet(d, key, append(out, elements...)) +} + +func binaryID(v any) string { + b, ok := v.(primitive.Binary) + if !ok || len(b.Data) != 16 { + return "" + } + return types.BlobToUUID(b.Data) +} + +// setPointer points key at the element id, keeping the binary subtype the +// stored pointer uses. +func setPointer(d bson.D, key string, id model.ID) { + sub := byte(0) + if b, ok := dGet(d, key).(primitive.Binary); ok { + sub = b.Subtype + } + dSet(d, key, primitive.Binary{Subtype: sub, Data: types.UUIDToBlob(string(id))}) +} + +func setBinary(d bson.D, key string, v any) { + if b, ok := v.(primitive.Binary); ok { + dSet(d, key, primitive.Binary{Subtype: b.Subtype, Data: bytes.Clone(b.Data)}) + } +} + +// setInt writes an integer property with the width it is stored in. +func setInt(d bson.D, key string, v int) { + switch dGet(d, key).(type) { + case int64: + dSet(d, key, int64(v)) + default: + dSet(d, key, int32(v)) + } +} + +// setVector sets a control vector on the flow's line. A flow without a +// Bezier line (a pre-10 document) has nothing to set. +func setVector(d bson.D, key, v string) { + if v == "" { + return + } + if line := dDoc(d, "Line"); line != nil { + dSet(line, key, v) + } +} diff --git a/mdl/backend/mfmutator/splice_test.go b/mdl/backend/mfmutator/splice_test.go new file mode 100644 index 000000000..34bc09358 --- /dev/null +++ b/mdl/backend/mfmutator/splice_test.go @@ -0,0 +1,354 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "bytes" + "fmt" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// These tests run the splice on hand-built documents, for the shapes the +// Studio Pro fixture does not have (loops, error handlers, vertical flows). +// The acceptance test on a Studio Pro-drawn flow is in the executor package +// (cmd_alter_flow_pedapp_test.go). + +// uid returns a deterministic element id for a short name. +func uid(name string) string { + b := make([]byte, 16) + copy(b, name) + return types.BlobToUUID(b) +} + +func bin(name string) primitive.Binary { + return primitive.Binary{Subtype: 0, Data: types.UUIDToBlob(uid(name))} +} + +func obj(name, typ string, x, y int) bson.D { + w, h := 120, 60 + if typ != "Microflows$ActionActivity" && typ != "Microflows$LoopedActivity" { + w, h = 20, 20 + } + return bson.D{ + {Key: "$ID", Value: bin(name)}, + {Key: "$Type", Value: typ}, + {Key: "RelativeMiddlePoint", Value: fmt.Sprintf("%d;%d", x, y)}, + {Key: "Size", Value: fmt.Sprintf("%d;%d", w, h)}, + } +} + +func flow(name, from, to string, fromSide, toSide int32, isErr bool) bson.D { + return bson.D{ + {Key: "$ID", Value: bin(name)}, + {Key: "$Type", Value: "Microflows$SequenceFlow"}, + {Key: "CaseValues", Value: bson.A{int32(2), bson.D{{Key: "$ID", Value: bin(name + "c")}, {Key: "$Type", Value: "Microflows$NoCase"}}}}, + {Key: "DestinationConnectionIndex", Value: toSide}, + {Key: "DestinationPointer", Value: bin(to)}, + {Key: "IsErrorHandler", Value: isErr}, + {Key: "Line", Value: bson.D{ + {Key: "$ID", Value: bin(name + "l")}, + {Key: "$Type", Value: "Microflows$BezierCurve"}, + {Key: "DestinationControlVector", Value: "-30;0"}, + {Key: "OriginControlVector", Value: "30;0"}, + }}, + {Key: "OriginConnectionIndex", Value: fromSide}, + {Key: "OriginPointer", Value: bin(from)}, + } +} + +func unit(objects []bson.D, flows []bson.D) bson.D { + objs := bson.A{int32(3)} + for _, o := range objects { + objs = append(objs, o) + } + fl := bson.A{int32(3)} + for _, f := range flows { + fl = append(fl, f) + } + return bson.D{ + {Key: "$ID", Value: bin("unit")}, + {Key: "$Type", Value: "Microflows$Microflow"}, + {Key: "Flows", Value: fl}, + {Key: "ObjectCollection", Value: bson.D{ + {Key: "$ID", Value: bin("oc")}, + {Key: "$Type", Value: "Microflows$MicroflowObjectCollection"}, + {Key: "Objects", Value: objs}, + }}, + } +} + +// fakeDeps serializes the way the codec does for the properties the splice +// reads, and records what was saved. +type fakeDeps struct{ saved []byte } + +func (d *fakeDeps) SerializeObject(o microflows.MicroflowObject) (bson.D, error) { + typ := "Microflows$ActionActivity" + if _, ok := o.(*microflows.ExclusiveMerge); ok { + typ = "Microflows$ExclusiveMerge" + } + p := o.GetPosition() + sz := objectSize(o) + return bson.D{ + {Key: "$ID", Value: primitive.Binary{Data: types.UUIDToBlob(string(o.GetID()))}}, + {Key: "$Type", Value: typ}, + {Key: "RelativeMiddlePoint", Value: fmt.Sprintf("%d;%d", p.X, p.Y)}, + {Key: "Size", Value: fmt.Sprintf("%d;%d", sz.X, sz.Y)}, + }, nil +} + +func (d *fakeDeps) SerializeSequenceFlow(f *microflows.SequenceFlow) (bson.D, error) { + return bson.D{ + {Key: "$ID", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.ID))}}, + {Key: "$Type", Value: "Microflows$SequenceFlow"}, + {Key: "DestinationConnectionIndex", Value: int32(f.DestinationConnectionIndex)}, + {Key: "DestinationPointer", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.DestinationID))}}, + {Key: "IsErrorHandler", Value: f.IsErrorHandler}, + {Key: "Line", Value: bson.D{ + {Key: "DestinationControlVector", Value: f.DestinationControlVector}, + {Key: "OriginControlVector", Value: f.OriginControlVector}, + }}, + {Key: "OriginConnectionIndex", Value: int32(f.OriginConnectionIndex)}, + {Key: "OriginPointer", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.OriginID))}}, + }, nil +} + +func (d *fakeDeps) SerializeAnnotationFlow(f *microflows.AnnotationFlow) (bson.D, error) { + return bson.D{ + {Key: "$ID", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.ID))}}, + {Key: "$Type", Value: "Microflows$AnnotationFlow"}, + {Key: "DestinationPointer", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.DestinationID))}}, + {Key: "OriginPointer", Value: primitive.Binary{Data: types.UUIDToBlob(string(f.OriginID))}}, + }, nil +} + +func (d *fakeDeps) SaveUnit(_ string, contents []byte) error { + d.saved = contents + return nil +} + +func newMutator(t *testing.T, doc bson.D) (*Mutator, *fakeDeps) { + t.Helper() + raw, err := bson.Marshal(doc) + if err != nil { + t.Fatal(err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatal(err) + } + deps := &fakeDeps{} + m, err := New(d, "unit", deps) + if err != nil { + t.Fatal(err) + } + return m, deps +} + +// oneActivity is a single-activity fragment in builder coordinates. +func oneActivity() *backend.MicroflowFragment { + id := model.ID(types.GenerateID()) + act := µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: id}, + Position: model.Point{X: 360, Y: 200}, + Size: model.Size{Width: 120, Height: 60}, + }}} + return &backend.MicroflowFragment{Objects: []microflows.MicroflowObject{act}, Entry: id, Exit: id} +} + +func line() []bson.D { + return []bson.D{ + obj("start", "Microflows$StartEvent", 100, 200), + obj("a", "Microflows$ActionActivity", 250, 200), + obj("b", "Microflows$ActionActivity", 420, 200), + obj("end", "Microflows$EndEvent", 600, 200), + } +} + +func lineFlows() []bson.D { + return []bson.D{ + flow("f1", "start", "a", 1, 3, false), + flow("f2", "a", "b", 1, 3, false), + flow("f3", "b", "end", 1, 3, false), + } +} + +// The control every splice test leans on: decoding a unit and encoding it +// again with nothing spliced gives back the stored bytes exactly. +func TestSplice_UntouchedUnitRoundTripsExactly(t *testing.T) { + raw, _ := bson.Marshal(unit(line(), lineFlows())) + m, _ := newMutator(t, unit(line(), lineFlows())) + out, err := m.Bytes() + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(out, raw) { + t.Error("an unmodified unit did not round-trip byte for byte") + } +} + +func TestSplice_InsertAfterKeepsTheErrorHandlerFlow(t *testing.T) { + objs := append(line(), obj("handler", "Microflows$ActionActivity", 250, 350)) + flows := append(lineFlows(), flow("err", "a", "handler", 2, 0, true)) + m, _ := newMutator(t, unit(objs, flows)) + errBefore, _ := bson.Marshal(flow("err", "a", "handler", 2, 0, true)) + + frag := oneActivity() + if err := m.InsertAfter(model.ID(uid("a")), frag); err != nil { + t.Fatal(err) + } + g := m.graph() + for _, f := range g.flows { + switch f.id { + case uid("err"): + got, _ := bson.Marshal(f.doc) + if !bytes.Equal(got, errBefore) { + t.Error("the error-handler flow changed") + } + case uid("f2"): + if f.dest != string(frag.Entry) { + t.Error("the normal flow out of a was not rewired to the fragment") + } + } + } +} + +func TestSplice_VerticalFlowUsesTopAndBottom(t *testing.T) { + objs := []bson.D{ + obj("a", "Microflows$ActionActivity", 200, 100), + obj("b", "Microflows$ActionActivity", 200, 200), + obj("below", "Microflows$ActionActivity", 200, 300), + obj("beside", "Microflows$ActionActivity", 500, 100), + } + flows := []bson.D{flow("f", "a", "b", 2, 0, false), flow("g", "b", "below", 2, 0, false)} + m, _ := newMutator(t, unit(objs, flows)) + frag := oneActivity() + if err := m.InsertAfter(model.ID(uid("a")), frag); err != nil { + t.Fatal(err) + } + g := m.graph() + if p := g.nodes[uid("b")].pos; p.X != 200 || p.Y <= 200 { + t.Errorf("b should have moved down, is at %v", p) + } + if p := g.nodes[uid("beside")].pos; p.X != 500 || p.Y != 100 { + t.Errorf("an object above the cut moved to %v", p) + } + for _, f := range g.flows { + if f.id == uid("f") { + if idx, _ := flowEnd(f.doc, "Destination"); idx != sideTop { + t.Errorf("the rewired flow enters the fragment on side %d, want top", idx) + } + } + if f.origin == string(frag.Exit) { + if idx, _ := flowEnd(f.doc, "Origin"); idx != sideBottom { + t.Errorf("the new flow leaves the fragment on side %d, want bottom", idx) + } + } + } +} + +func TestSplice_Refusals(t *testing.T) { + loop := obj("loop", "Microflows$LoopedActivity", 420, 200) + loop = append(loop, bson.E{Key: "ObjectCollection", Value: bson.D{ + {Key: "$ID", Value: bin("loopoc")}, + {Key: "$Type", Value: "Microflows$MicroflowObjectCollection"}, + {Key: "Objects", Value: bson.A{int32(3), obj("inner", "Microflows$ActionActivity", 100, 60), obj("inner2", "Microflows$ActionActivity", 260, 60)}}, + }}) + withLoop := []bson.D{obj("start", "Microflows$StartEvent", 100, 200), obj("a", "Microflows$ActionActivity", 250, 200), loop} + loopFlows := []bson.D{flow("f1", "start", "a", 1, 3, false), flow("f2", "a", "loop", 1, 3, false), flow("fi", "inner", "inner2", 1, 3, false)} + + twoIn := append(line(), obj("c", "Microflows$ActionActivity", 250, 350)) + twoInFlows := append(lineFlows(), flow("f4", "c", "b", 0, 2, false)) + + withHandler := append(line(), obj("handler", "Microflows$ActionActivity", 250, 350)) + handlerFlows := append(lineFlows(), flow("err", "a", "handler", 2, 0, true)) + + cases := []struct { + name string + objs []bson.D + flows []bson.D + op func(m *Mutator) error + want string + }{ + {"insert inside a loop", withLoop, loopFlows, + func(m *Mutator) error { return m.InsertAfter(model.ID(uid("inner")), oneActivity()) }, "inside a loop"}, + {"drop inside a loop", withLoop, loopFlows, + func(m *Mutator) error { return m.Drop(model.ID(uid("inner"))) }, "inside a loop"}, + {"insert before a join", twoIn, twoInFlows, + func(m *Mutator) error { return m.InsertBefore(model.ID(uid("b")), oneActivity()) }, "2 flows enter"}, + {"insert after the end", line(), lineFlows(), + func(m *Mutator) error { return m.InsertAfter(model.ID(uid("end")), oneActivity()) }, "ends the flow"}, + {"drop an activity with an error handler", withHandler, handlerFlows, + func(m *Mutator) error { return m.Drop(model.ID(uid("a"))) }, "error handler"}, + {"replace an activity with an error handler", withHandler, handlerFlows, + func(m *Mutator) error { return m.Replace(model.ID(uid("a")), oneActivity()) }, "error handler"}, + {"drop the end event", line(), lineFlows(), + func(m *Mutator) error { return m.Drop(model.ID(uid("end"))) }, "cannot drop"}, + {"an empty fragment", line(), lineFlows(), + func(m *Mutator) error { return m.InsertAfter(model.ID(uid("a")), &backend.MicroflowFragment{}) }, "empty"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m, _ := newMutator(t, unit(tc.objs, tc.flows)) + before, _ := bson.Marshal(m.doc) + err := tc.op(m) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("want an error containing %q, got %v", tc.want, err) + } + after, _ := bson.Marshal(m.doc) + if !bytes.Equal(before, after) { + t.Error("a refused operation changed the document") + } + }) + } +} + +// CLAUDE.md rule 1: an element is never removed while anything still points +// at it. A pointer the splice does not know about (here, a made-up property +// on another object) is found by value, and the write is refused. +func TestSplice_DropRefusesADanglingReference(t *testing.T) { + objs := line() + objs[3] = append(objs[3], bson.E{Key: "SomePointer", Value: bin("a")}) + m, deps := newMutator(t, unit(objs, lineFlows())) + if err := m.Drop(model.ID(uid("a"))); err != nil { + t.Fatalf("drop: %v", err) + } + err := m.Save() + if err == nil || !strings.Contains(err.Error(), "still points at a removed element") { + t.Fatalf("want the dangling-reference refusal, got %v", err) + } + if deps.saved != nil { + t.Error("a unit with a dangling reference was saved") + } +} + +// Control for the test above: the same drop without the stray pointer saves. +func TestSplice_DropJoinsTheFlows(t *testing.T) { + m, deps := newMutator(t, unit(line(), lineFlows())) + if err := m.Drop(model.ID(uid("a"))); err != nil { + t.Fatalf("drop: %v", err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + if bytes.Contains(deps.saved, types.UUIDToBlob(uid("a"))) { + t.Error("the dropped activity's $ID is still in the unit") + } + g := m.graph() + for _, f := range g.flows { + if f.id == uid("f1") && f.dest != uid("b") { + t.Errorf("the flow into the dropped activity now enters %s, want b", f.dest) + } + if f.id == uid("f2") { + t.Error("the flow out of the dropped activity is still there") + } + } +} diff --git a/mdl/backend/microflow_mutation.go b/mdl/backend/microflow_mutation.go new file mode 100644 index 000000000..2737d66c9 --- /dev/null +++ b/mdl/backend/microflow_mutation.go @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: Apache-2.0 + +package backend + +import ( + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// MicroflowFragment is what an `alter microflow` insert or replace splices in: +// the objects and flows a fragment of MDL builds to, written exactly as +// `create microflow` writes the same statements, and the two objects the +// surrounding flow connects to. Entry is where the flow into the fragment +// ends; Exit is where the flow out of it starts. For a one-activity fragment +// they are the same object. +// +// Positions are the builder's own; the mutator translates the fragment into +// place (plan item 4.2c) before writing it. +type MicroflowFragment struct { + Objects []microflows.MicroflowObject + Flows []*microflows.SequenceFlow + AnnotationFlows []*microflows.AnnotationFlow + Entry, Exit model.ID +} + +// MicroflowMutator splices into one stored microflow or nanoflow (ADR-0012 +// decision 3). Targets are activity IDs, already resolved from their content +// address by mfmutator.Resolve. Every operation edits the stored document in +// place: untouched elements stay byte-identical, and nothing is rebuilt. +// Call Save to persist. +type MicroflowMutator interface { + // InsertAfter splices frag onto the one flow that leaves target. + InsertAfter(target model.ID, frag *MicroflowFragment) error + // InsertBefore splices frag onto the one flow that enters target. + InsertBefore(target model.ID, frag *MicroflowFragment) error + // Replace puts frag in target's place and removes target. + Replace(target model.ID, frag *MicroflowFragment) error + // Drop removes target and joins its incoming flows to its successor. + Drop(target model.ID) error + // Save writes the patched unit. + Save() error +} + +// MicroflowMutationBackend opens a microflow or nanoflow for splicing. +type MicroflowMutationBackend interface { + // OpenMicroflowForMutation loads a Microflows$Microflow or + // Microflows$Nanoflow unit and returns a mutator over its stored form. + OpenMicroflowForMutation(unitID model.ID) (MicroflowMutator, error) +} diff --git a/mdl/backend/mock/backend.go b/mdl/backend/mock/backend.go index b5fa2477a..cef40cf4a 100644 --- a/mdl/backend/mock/backend.go +++ b/mdl/backend/mock/backend.go @@ -341,6 +341,9 @@ type MockBackend struct { // WorkflowMutationBackend OpenWorkflowForMutationFunc func(unitID model.ID) (backend.WorkflowMutator, error) + // MicroflowMutationBackend + OpenMicroflowForMutationFunc func(unitID model.ID) (backend.MicroflowMutator, error) + // WidgetSerializationBackend // WidgetBuilderBackend diff --git a/mdl/backend/mock/mock_mutation.go b/mdl/backend/mock/mock_mutation.go index f9d9f53c6..a0ee642bc 100644 --- a/mdl/backend/mock/mock_mutation.go +++ b/mdl/backend/mock/mock_mutation.go @@ -32,6 +32,17 @@ func (m *MockBackend) OpenWorkflowForMutation(unitID model.ID) (backend.Workflow return nil, fmt.Errorf("MockBackend.OpenWorkflowForMutation not configured") } +// --------------------------------------------------------------------------- +// MicroflowMutationBackend +// --------------------------------------------------------------------------- + +func (m *MockBackend) OpenMicroflowForMutation(unitID model.ID) (backend.MicroflowMutator, error) { + if m.OpenMicroflowForMutationFunc != nil { + return m.OpenMicroflowForMutationFunc(unitID) + } + return nil, fmt.Errorf("MockBackend.OpenMicroflowForMutation not configured") +} + // --------------------------------------------------------------------------- // WidgetSerializationBackend // --------------------------------------------------------------------------- diff --git a/mdl/backend/modelsdk/microflow_mutator_write.go b/mdl/backend/modelsdk/microflow_mutator_write.go new file mode 100644 index 000000000..21f8a2810 --- /dev/null +++ b/mdl/backend/modelsdk/microflow_mutator_write.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "fmt" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/codec" + "github.com/mendixlabs/mxcli/modelsdk/element" + genMf "github.com/mendixlabs/mxcli/modelsdk/gen/microflows" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// OpenMicroflowForMutation loads a microflow or nanoflow unit and returns the +// shared graph splice (mfmutator) over its stored bytes. New objects and flows +// are encoded with the same converters `create microflow` uses, so a fragment +// is written exactly as the same statements would be in a create; the patched +// unit is written through the reconciling writer as a patch. +func (b *Backend) OpenMicroflowForMutation(unitID model.ID) (backend.MicroflowMutator, error) { + if b.writer == nil { + return nil, fmt.Errorf("OpenMicroflowForMutation: not connected for writing") + } + raw, err := b.reader.GetRawUnitBytes(string(unitID)) + if err != nil { + return nil, fmt.Errorf("OpenMicroflowForMutation: load unit: %w", err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + return nil, fmt.Errorf("OpenMicroflowForMutation: unmarshal: %w", err) + } + return mfmutator.New(d, unitID, codecMicroflowDeps{b: b}) +} + +// codecMicroflowDeps implements mfmutator.Deps for the modelsdk (codec) backend. +type codecMicroflowDeps struct{ b *Backend } + +var _ mfmutator.Deps = codecMicroflowDeps{} + +func (d codecMicroflowDeps) SerializeObject(obj microflows.MicroflowObject) (bson.D, error) { + el := microflowObjectToGen(obj) + if el == nil { + return nil, nil + } + assignFlowObjectIDs(el) + return encodeFlowElementToD(el) +} + +func (d codecMicroflowDeps) SerializeSequenceFlow(f *microflows.SequenceFlow) (bson.D, error) { + el := sequenceFlowToGen(f, d.b.majorVersion()) + assignID(el) + if sf, ok := el.(*genMf.SequenceFlow); ok { + for _, cv := range sf.CaseValuesItems() { + assignID(cv) + } + assignID(sf.Line()) + } + return encodeFlowElementToD(el) +} + +func (d codecMicroflowDeps) SerializeAnnotationFlow(f *microflows.AnnotationFlow) (bson.D, error) { + el := annotationFlowToGen(f, d.b.majorVersion()) + assignID(el) + if af, ok := el.(*genMf.AnnotationFlow); ok { + assignID(af.Line()) + } + return encodeFlowElementToD(el) +} + +// SaveUnit writes the spliced unit as a patch: the reconciling writer elides +// an unchanged unit and guards storage GUIDs as for every write, but does not +// re-pair element $IDs the splice deliberately kept (canon.ContentsOwnElementIDs). +func (d codecMicroflowDeps) SaveUnit(unitID string, contents []byte) error { + return d.b.writer.UpdateRawUnitPatch(unitID, contents) +} + +func encodeFlowElementToD(el element.Element) (bson.D, error) { + raw, err := (&codec.Encoder{}).Encode(el) + if err != nil { + return nil, err + } + var out bson.D + if err := bson.Unmarshal(raw, &out); err != nil { + return nil, err + } + return out, nil +} diff --git a/mdl/backend/modelsdk/unimplemented_gen.go b/mdl/backend/modelsdk/unimplemented_gen.go index 232eed4a2..b7b3f4f78 100644 --- a/mdl/backend/modelsdk/unimplemented_gen.go +++ b/mdl/backend/modelsdk/unimplemented_gen.go @@ -891,6 +891,11 @@ func (unimplemented) MoveViewEntitySourceDocument(_ string, _ model.ID, _ string return errUnimplemented("MoveViewEntitySourceDocument") } +func (unimplemented) OpenMicroflowForMutation(_ model.ID) (backend.MicroflowMutator, error) { + var r0 backend.MicroflowMutator + return r0, errUnimplemented("OpenMicroflowForMutation") +} + func (unimplemented) OpenPageForMutation(_ model.ID) (backend.PageMutator, error) { var r0 backend.PageMutator return r0, errUnimplemented("OpenPageForMutation") @@ -1118,6 +1123,10 @@ func (unimplemented) UpdateJsonStructure(_ *types.JsonStructure) error { return errUnimplemented("UpdateJsonStructure") } +func (unimplemented) UpdateLayout(_ *pages.Layout) error { + return errUnimplemented("UpdateLayout") +} + func (unimplemented) UpdateMenuDocument(_ *types.MenuDocument) error { return errUnimplemented("UpdateMenuDocument") } @@ -1224,3 +1233,8 @@ func (unimplemented) WriteJavaScriptSourceFile(_ string, _ string, _ string, _ [ func (unimplemented) WriteJavaSourceFile(_ string, _ string, _ string, _ []*types.JavaActionParameter, _ types.CodeActionReturnType, _ []string, _ string) error { return errUnimplemented("WriteJavaSourceFile") } + +func (unimplemented) WriteViewEntitySourceDocument(_ model.ID, _ string, _ string, _ string, _ string) (model.ID, error) { + var r0 model.ID + return r0, errUnimplemented("WriteViewEntitySourceDocument") +} From ab4e9e2dd59fada4fde4f892702a4ea5d4d37f0f Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 08:49:04 +0000 Subject: [PATCH 3/7] alter microflow/nanoflow: grammar, fragment building and scope checks (#736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alter microflow|nanoflow M.F { insert after|before { … } replace with { … } drop ; }. Targets are the #713 content addresses, resolved against the stored flow before any operation runs. Fragments are built with the create-microflow builder, seeded with the flow's variables, and cut out of their start and end events. A fragment variable that clashes with one the flow has, or one it reads that is not declared upstream of the insertion point, is an error; so is dropping an activity whose output is still read. Acceptance on PedApp VAL_Feedback: only the new log, the two flows around it and the shifted positions differ; an empty alter writes nothing. Co-Authored-By: Claude Opus 5.5 --- cmd/mxcli/syntax/features_microflow.go | 35 ++ docs/01-project/MDL_QUICK_REFERENCE.md | 2 + mdl/ast/ast_alter_flow.go | 54 ++ mdl/executor/cmd_alter_flow.go | 445 ++++++++++++++++ mdl/executor/cmd_alter_flow_pedapp_test.go | 571 +++++++++++++++++++++ mdl/executor/register_stubs.go | 3 + mdl/executor/registry_test.go | 1 + mdl/executor/stmt_summary.go | 3 + mdl/grammar/MDLParser.g4 | 35 ++ mdl/visitor/visitor_alter.go | 4 + mdl/visitor/visitor_alter_flow.go | 53 ++ mdl/visitor/visitor_alter_flow_test.go | 60 +++ 12 files changed, 1266 insertions(+) create mode 100644 mdl/ast/ast_alter_flow.go create mode 100644 mdl/executor/cmd_alter_flow.go create mode 100644 mdl/executor/cmd_alter_flow_pedapp_test.go create mode 100644 mdl/visitor/visitor_alter_flow.go create mode 100644 mdl/visitor/visitor_alter_flow_test.go diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index bc7dc3a74..118337dc1 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -400,6 +400,41 @@ func init() { Example: "LOG INFO NODE 'OrderService' 'Order created successfully';\nLOG WARNING 'Customer not found';\nLOG ERROR 'Failed to process {1}' WITH (\n {1} = $OrderNumber\n);", }) + Register(SyntaxFeature{ + Path: "microflow.alter", + Summary: "Patch a stored microflow or nanoflow: insert, replace or drop activities in place", + Keywords: []string{ + "alter microflow", "alter nanoflow", "insert after", "insert before", + "replace", "drop activity", "patch microflow", "splice", "handle", + }, + Syntax: "ALTER MICROFLOW|NANOFLOW Module.Name {\n" + + " INSERT AFTER|BEFORE { }\n" + + " REPLACE WITH { }\n" + + " DROP ;\n" + + "};\n\n" + + "-- addresses one activity by content, as `describe microflow ... with handles` prints it:\n" + + "-- $Var the activity that outputs $Var\n" + + "-- 'Caption' a decision or an activity with a custom caption\n" + + "-- a statement pattern; * matches any run of tokens\n" + + "-- followed by @n when it matches more than one. Targets are resolved against the\n" + + "-- stored flow before any operation runs; an ambiguous or unknown target is an error.\n" + + "-- Only the new activities, the rewired flows and the objects moved to make room change;\n" + + "-- every other element keeps its $ID, position and curve.\n" + + "-- Refused: insert after a decision, insert before an activity several flows enter,\n" + + "-- drop/replace of a decision or of an activity with an error handler, anything inside\n" + + "-- a loop body, a fragment that returns, and a fragment variable that clashes with one\n" + + "-- the flow has or reads one not declared on the path. Over --mcp only insert is supported.", + Example: "alter microflow FeedbackModule.VAL_Feedback {\n" + + " insert after $IsValidEmail { log info node 'Feedback' 'Email checked'; }\n" + + " replace set $ValidFeedback = false @3 with {\n" + + " set $ValidFeedback = false;\n" + + " log warning node 'Feedback' 'Email rejected';\n" + + " }\n" + + " drop log debug node 'Feedback' *;\n" + + "};", + SeeAlso: []string{"microflow"}, + }) + Register(SyntaxFeature{ Path: "microflow.show-page", Summary: "Open and close pages from microflows", diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 1abf19730..6ef196712 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -483,6 +483,8 @@ rather than updating the first. | Describe microflow | `describe microflow Module.Name;` | Full MDL with activities | | Describe microflow (normalized) | `describe microflow Module.Name normalized;` | Folds crossed branches into one condition instead of flattening them. Opt-in: the output re-executes to an equivalent graph with fewer nodes and a different layout | | Describe microflow (with handles) | `describe microflow Module.Name with handles;` | Prints `-- handle: ` above each activity: its content address for `alter microflow` — output `$Var`, `'Caption'`, or a statement pattern with `*` wildcards (anchored at both ends), plus `@n` when several match. Comments only; cannot be combined with `normalized` | +| Insert into a stored microflow | `alter microflow Module.Name { insert after { } };` | Also `insert before`, and `alter nanoflow`. A graph splice into the stored flow, not a rebuild: only the new activities, the two rewired flows and the objects moved to make room change; every other element keeps its `$ID`, position and curve. `` is a handle from `describe … with handles`, resolved before any operation runs. Refused: after a decision, before an activity several flows enter, inside a loop body, a fragment that returns, a variable the flow already has or one not declared on the path | +| Replace or drop an activity | `alter microflow Module.Name { replace with { } drop ; };` | Flows into the activity are re-pointed at the replacement (or at its successor, for `drop`). Refused for a decision, an end event, an activity with an error handler, and an activity whose output variable is still read. Over `--mcp` only `insert` is supported | | Describe nanoflow | `describe nanoflow Module.Name;` | Full MDL with activities | | Rename microflow | `rename microflow Module.Old to New;` | Updates all references | | Rename nanoflow | `rename nanoflow Module.Old to New;` | Updates all references | diff --git a/mdl/ast/ast_alter_flow.go b/mdl/ast/ast_alter_flow.go new file mode 100644 index 000000000..b6534dbd4 --- /dev/null +++ b/mdl/ast/ast_alter_flow.go @@ -0,0 +1,54 @@ +// SPDX-License-Identifier: Apache-2.0 + +package ast + +// ============================================================================ +// ALTER MICROFLOW / ALTER NANOFLOW — a graph splice into the stored flow +// ============================================================================ + +// AlterFlowStmt represents: +// +// alter microflow|nanoflow Module.Name { +// insert after|before { } +// replace with { } +// drop ; +// } +// +// (ADR-0012 decision 3). Targets are content addresses, resolved against the +// flow as stored before any operation applies. +type AlterFlowStmt struct { + Nanoflow bool + Name QualifiedName + Operations []*AlterFlowOperation +} + +func (s *AlterFlowStmt) isStatement() {} + +// Kind is "microflow" or "nanoflow". +func (s *AlterFlowStmt) Kind() string { + if s.Nanoflow { + return "nanoflow" + } + return "microflow" +} + +// AlterFlowOpKind names an operation of an AlterFlowStmt. +type AlterFlowOpKind string + +const ( + AlterFlowInsertAfter AlterFlowOpKind = "insert after" + AlterFlowInsertBefore AlterFlowOpKind = "insert before" + AlterFlowReplace AlterFlowOpKind = "replace" + AlterFlowDrop AlterFlowOpKind = "drop" +) + +// AlterFlowOperation is one operation of an AlterFlowStmt. +type AlterFlowOperation struct { + Op AlterFlowOpKind + // Target is the content address as written (`$IsValidEmail`, + // `'Email is Valid?'`, `log * node 'Debug' *`, with an optional `@n`). + // mfmutator.ParseTarget reads it. + Target string + // Body is the fragment, for insert and replace. + Body []MicroflowStatement +} diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go new file mode 100644 index 000000000..69f5897a2 --- /dev/null +++ b/mdl/executor/cmd_alter_flow.go @@ -0,0 +1,445 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// execAlterFlow handles `alter microflow|nanoflow Module.Name { … }`: a patch +// of the stored flow (ADR-0012 decision 3), never a rebuild. +// +// Every target is resolved against the flow AS STORED, before any operation +// runs, so an address means what `describe … with handles` showed: an +// ambiguity or a miss refuses the whole statement before anything changes, and +// a later operation cannot address what an earlier one inserted. +func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { + if !ctx.Connected() { + return mdlerrors.NewNotConnected() + } + if !ctx.ConnectedForWrite() { + return mdlerrors.NewNotConnectedWrite() + } + a, err := loadAlterFlow(ctx, s) + if err != nil { + return err + } + + targets := make([]mfmutator.Candidate, len(s.Operations)) + for i, op := range s.Operations { + c, err := mfmutator.ResolveText(a.cands, op.Target) + if err != nil { + return mdlerrors.NewValidation(fmt.Sprintf("alter %s %s: %s %s: %v", s.Kind(), s.Name, op.Op, op.Target, err)) + } + targets[i] = c + } + + mut, err := ctx.Backend.OpenMicroflowForMutation(a.mf.ID) + if err != nil { + return mdlerrors.NewBackend("open "+s.Kind()+" for alter", err) + } + for i, op := range s.Operations { + target := targets[i] + fail := func(err error) error { + return mdlerrors.NewValidation(fmt.Sprintf("alter %s %s: %s %s: %v", s.Kind(), s.Name, op.Op, op.Target, err)) + } + if op.Op == ast.AlterFlowDrop { + if err := a.checkOutputUnused(target, nil); err != nil { + return fail(err) + } + if err := mut.Drop(target.ID); err != nil { + return fail(err) + } + continue + } + frag, err := a.buildFragment(ctx, op.Body) + if err != nil { + return fail(err) + } + if err := a.checkFragmentScope(ctx, op, target, frag); err != nil { + return fail(err) + } + switch op.Op { + case ast.AlterFlowInsertAfter: + err = mut.InsertAfter(target.ID, frag) + case ast.AlterFlowInsertBefore: + err = mut.InsertBefore(target.ID, frag) + case ast.AlterFlowReplace: + if err = a.checkOutputUnused(target, frag); err == nil { + err = mut.Replace(target.ID, frag) + } + default: + err = fmt.Errorf("unknown operation") + } + if err != nil { + return fail(err) + } + } + if err := mut.Save(); err != nil { + return mdlerrors.NewBackend("save altered "+s.Kind(), err) + } + fmt.Fprintf(ctx.Output, "Altered %s %s\n", s.Kind(), s.Name) + return nil +} + +// alterFlowContext is what the operations of one statement share: the stored +// flow (a nanoflow wrapped as a microflow, the way describe renders one), its +// addressable activities, and the name maps rendering needs. +type alterFlowContext struct { + stmt *ast.AlterFlowStmt + mf *microflows.Microflow + cands []mfmutator.Candidate + entityNames map[model.ID]string + microflowNames map[model.ID]string +} + +func loadAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) (*alterFlowContext, error) { + h, err := getHierarchy(ctx) + if err != nil { + return nil, mdlerrors.NewBackend("build hierarchy", err) + } + a := &alterFlowContext{stmt: s, entityNames: getEntityNames(ctx, h)} + // A copy: nanoflow names are added below, and the cached map is shared. + a.microflowNames = map[model.ID]string{} + for id, n := range getMicroflowNames(ctx, h) { + a.microflowNames[id] = n + } + inModule := func(container model.ID, name string) bool { + return h.GetModuleName(h.FindModuleID(container)) == s.Name.Module && name == s.Name.Name + } + if s.Nanoflow { + nfs, err := ctx.Backend.ListNanoflows() + if err != nil { + return nil, mdlerrors.NewBackend("list nanoflows", err) + } + for _, nf := range nfs { + a.microflowNames[nf.ID] = h.GetQualifiedName(nf.ContainerID, nf.Name) + } + nf, ok := pickLive(nfs, + func(nf *microflows.Nanoflow) bool { return inModule(nf.ContainerID, nf.Name) }, + func(nf *microflows.Nanoflow) bool { return nf.Excluded }) + if !ok { + return nil, mdlerrors.NewNotFound("nanoflow", s.Name.String()) + } + a.mf = µflows.Microflow{ + BaseElement: nf.BaseElement, + ContainerID: nf.ContainerID, + Name: nf.Name, + Parameters: nf.Parameters, + ReturnType: nf.ReturnType, + ReturnVariableName: nf.ReturnVariableName, + ObjectCollection: nf.ObjectCollection, + } + } else { + mfs, err := ctx.Backend.ListMicroflows() + if err != nil { + return nil, mdlerrors.NewBackend("list microflows", err) + } + mf, ok := pickLive(mfs, + func(mf *microflows.Microflow) bool { return inModule(mf.ContainerID, mf.Name) }, + func(mf *microflows.Microflow) bool { return mf.Excluded }) + if !ok { + return nil, mdlerrors.NewNotFound("microflow", s.Name.String()) + } + a.mf = mf + } + if a.mf.ObjectCollection == nil { + return nil, mdlerrors.NewValidation(fmt.Sprintf("%s %s has no flow to alter", s.Kind(), s.Name)) + } + a.cands, _, _, _ = microflowTargets(ctx, a.mf, a.entityNames, a.microflowNames) + return a, nil +} + +// buildFragment builds a fragment's statements with the builder `create +// microflow` uses, seeded with the variables the stored flow declares, and +// cuts it out of the start and end events the builder wraps it in. +func (a *alterFlowContext) buildFragment(ctx *ExecContext, body []ast.MicroflowStatement) (*backend.MicroflowFragment, error) { + if len(body) == 0 { + return nil, fmt.Errorf("the fragment is empty; use drop to remove an activity") + } + varTypes, declared := a.storedVariables(ctx) + hierarchy, _ := getHierarchy(ctx) + restServices, _ := loadRestServices(ctx) + fb := &flowBuilder{ + textLang: authoringLanguage(ctx), + posX: 200, + posY: 200, + baseY: 200, + spacing: HorizontalSpacing, + varTypes: varTypes, + declaredVars: declared, + measurer: &layoutMeasurer{varTypes: varTypes}, + backend: ctx.Backend, + hierarchy: hierarchy, + restServices: restServices, + isNanoflow: a.stmt.Nanoflow, + } + oc := fb.buildFlowGraph(body, nil) + if errs := fb.GetErrors(); len(errs) > 0 { + return nil, fmt.Errorf("the fragment has errors:\n - %s", strings.Join(errs, "\n - ")) + } + if fb.endsWithReturn { + return nil, fmt.Errorf("the fragment ends the flow with a return, so nothing would lead on to the rest of it; " + + "a return inside an inserted fragment is not supported yet") + } + return cutFragment(oc) +} + +// cutFragment removes the builder's start event and final end event, and says +// where the fragment is entered and left. Several paths reaching the end (an +// if without a merge before it, an error handler that rejoins at the end) are +// joined by a merge, which becomes the exit. +func cutFragment(oc *microflows.MicroflowObjectCollection) (*backend.MicroflowFragment, error) { + var start, end microflows.MicroflowObject + ends := 0 + for _, obj := range oc.Objects { + switch obj.(type) { + case *microflows.StartEvent: + start = obj + case *microflows.EndEvent: + ends++ + end = obj + } + } + if start == nil || end == nil { + return nil, fmt.Errorf("the fragment does not continue: its last statement ends the flow, so nothing would lead on to the rest of it") + } + if ends > 1 { + return nil, fmt.Errorf("the fragment returns; a return inside an inserted fragment is not supported yet") + } + frag := &backend.MicroflowFragment{} + var intoEnd []*microflows.SequenceFlow + for _, f := range oc.Flows { + switch { + case f.OriginID == start.GetID(): + if frag.Entry != "" { + return nil, fmt.Errorf("the fragment starts with more than one flow") + } + frag.Entry = f.DestinationID + case f.DestinationID == end.GetID(): + intoEnd = append(intoEnd, f) + default: + frag.Flows = append(frag.Flows, f) + } + } + for _, obj := range oc.Objects { + if obj != start && obj != end { + frag.Objects = append(frag.Objects, obj) + } + } + frag.AnnotationFlows = oc.AnnotationFlows + switch { + case frag.Entry == "" || frag.Entry == end.GetID() || len(frag.Objects) == 0: + return nil, fmt.Errorf("the fragment builds no activity") + case len(intoEnd) == 0: + return nil, fmt.Errorf("no path through the fragment leads on to the rest of the flow") + case len(intoEnd) == 1: + frag.Exit = intoEnd[0].OriginID + default: + p := end.GetPosition() + merge := µflows.ExclusiveMerge{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Position: p, + Size: model.Size{Width: MergeSize, Height: MergeSize}, + }} + for _, f := range intoEnd { + f.DestinationID = merge.ID + frag.Flows = append(frag.Flows, f) + } + frag.Objects = append(frag.Objects, merge) + frag.Exit = merge.ID + } + return frag, nil +} + +// storedVariables returns the variables the stored flow declares, in the two +// maps the builder keeps: entity-typed ones with their entity (a change or a +// member access resolves attributes through it), and the rest as declared. +func (a *alterFlowContext) storedVariables(ctx *ExecContext) (varTypes, declared map[string]string) { + varTypes, declared = map[string]string{}, map[string]string{} + add := func(name string, dt microflows.DataType) { + if name == "" { + return + } + t := "Unknown" + if dt != nil { + t = formatMicroflowDataType(ctx, dt, a.entityNames) + } + switch dt.(type) { + case *microflows.ObjectType, *microflows.ListType: + varTypes[name] = t + default: + declared[name] = t + } + } + for _, p := range a.mf.Parameters { + add(p.Name, p.Type) + } + for _, c := range a.cands { + act, ok := c.Object.(*microflows.ActionActivity) + if !ok || c.OutputVariable == "" { + continue + } + switch x := act.Action.(type) { + case *microflows.CreateVariableAction: + add(c.OutputVariable, x.DataType) + case *microflows.CreateObjectAction: + if x.EntityQualifiedName != "" { + varTypes[c.OutputVariable] = x.EntityQualifiedName + } else if n, ok := a.entityNames[x.EntityID]; ok { + varTypes[c.OutputVariable] = n + } else { + declared[c.OutputVariable] = "Object" + } + default: + declared[c.OutputVariable] = "Unknown" + } + } + return varTypes, declared +} + +// systemVariables are in scope everywhere they exist at all; the platform +// reports a misuse (a $latestError outside an error handler) itself. +var systemVariables = map[string]bool{ + "currentUser": true, "currentSession": true, "currentObject": true, "currentDeviceType": true, + "latestError": true, "latestHttpResponse": true, "latestSoapFault": true, +} + +var variableRef = regexp.MustCompile(`\$([A-Za-z_][A-Za-z0-9_]*)`) + +// checkFragmentScope is plan item 4.2d: the fragment is checked in the scope +// of its insertion point. A variable it declares that the flow already has is +// an error (it would shadow or clash with the stored one); a variable it uses +// that is not declared upstream of where it goes, nor by the fragment itself, +// is an error too, since the fragment would read something that does not exist +// yet on that path. +func (a *alterFlowContext) checkFragmentScope(ctx *ExecContext, op *ast.AlterFlowOperation, target mfmutator.Candidate, frag *backend.MicroflowFragment) error { + existing := map[string]bool{} + for _, p := range a.mf.Parameters { + existing[p.Name] = true + } + for _, c := range a.cands { + if c.OutputVariable != "" { + existing[c.OutputVariable] = true + } + } + own := map[string]bool{} + for _, obj := range frag.Objects { + act, ok := obj.(*microflows.ActionActivity) + if !ok { + continue + } + v := mfmutator.OutputVariable(act.Action) + if v == "" { + continue + } + replacingSame := op.Op == ast.AlterFlowReplace && v == target.OutputVariable + if existing[v] && !replacingSame { + return fmt.Errorf("the fragment declares $%s, which the %s already has; choose another name", v, a.stmt.Kind()) + } + own[v] = true + } + + inScope := map[string]bool{} + for _, p := range a.mf.Parameters { + inScope[p.Name] = true + } + for id := range a.upstreamOf(target.ID, op.Op == ast.AlterFlowInsertAfter) { + for _, c := range a.cands { + if c.ID == id && c.OutputVariable != "" { + inScope[c.OutputVariable] = true + } + } + } + var missing []string + seen := map[string]bool{} + for _, obj := range frag.Objects { + for _, m := range variableRef.FindAllStringSubmatch(formatActivity(ctx, obj, a.entityNames, a.microflowNames), -1) { + v := m[1] + if seen[v] || systemVariables[v] || inScope[v] || own[v] { + continue + } + seen[v] = true + missing = append(missing, "$"+v) + } + } + if len(missing) > 0 { + sort.Strings(missing) + where := "before " + op.Target + if op.Op == ast.AlterFlowInsertAfter { + where = "after " + op.Target + } + return fmt.Errorf("the fragment uses %s, which is not declared on the path %s", strings.Join(missing, ", "), where) + } + return nil +} + +// upstreamOf returns every object from which id can be reached along the +// stored flows — the activities whose outputs exist when the flow gets there. +// id itself is included only when including says so (an insert after it runs +// once it has). +func (a *alterFlowContext) upstreamOf(id model.ID, including bool) map[model.ID]bool { + preds := map[model.ID][]model.ID{} + for _, f := range a.mf.ObjectCollection.Flows { + preds[f.DestinationID] = append(preds[f.DestinationID], f.OriginID) + } + out := map[model.ID]bool{} + queue := append([]model.ID(nil), preds[id]...) + for len(queue) > 0 { + n := queue[0] + queue = queue[1:] + if out[n] { + continue + } + out[n] = true + queue = append(queue, preds[n]...) + } + if including { + out[id] = true + } + return out +} + +// checkOutputUnused refuses to take away an activity whose output variable +// another activity still reads — unless the replacement declares it again. +func (a *alterFlowContext) checkOutputUnused(target mfmutator.Candidate, replacement *backend.MicroflowFragment) error { + v := target.OutputVariable + if v == "" { + return nil + } + if replacement != nil { + for _, obj := range replacement.Objects { + if act, ok := obj.(*microflows.ActionActivity); ok && mfmutator.OutputVariable(act.Action) == v { + return nil + } + } + } + ref := regexp.MustCompile(`\$` + regexp.QuoteMeta(v) + `\b`) + var users []string + for _, c := range a.cands { + if c.ID == target.ID { + continue + } + for _, text := range append([]string{c.Statement}, c.Alternates...) { + if ref.MatchString(text) { + users = append(users, c.Statement) + break + } + } + } + if len(users) > 0 { + return fmt.Errorf("$%s is still used by: %s", v, strings.Join(users, "; ")) + } + return nil +} diff --git a/mdl/executor/cmd_alter_flow_pedapp_test.go b/mdl/executor/cmd_alter_flow_pedapp_test.go new file mode 100644 index 000000000..fc14786ea --- /dev/null +++ b/mdl/executor/cmd_alter_flow_pedapp_test.go @@ -0,0 +1,571 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "context" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + modelsdkbackend "github.com/mendixlabs/mxcli/mdl/backend/modelsdk" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// The acceptance test of plan item 4.2 (ako/mxcli#736) runs on the Studio +// Pro-authored PedApp fixture, because only a flow Studio Pro drew can show +// what a rebuild loses: its merges, its curves, its object order, its $IDs. + +// openPedAppFixture opens a private copy of testdata/pedapp. +func openPedAppFixture(t *testing.T) (*Executor, *bytes.Buffer) { + t.Helper() + src := filepath.Join("..", "..", "testdata", "pedapp") + if _, err := os.Stat(filepath.Join(src, "PedApp.mpr")); err != nil { + t.Skipf("PedApp fixture not found: %v", err) + } + dir := t.TempDir() + if err := copyPedAppFile(filepath.Join(src, "PedApp.mpr"), filepath.Join(dir, "PedApp.mpr")); err != nil { + t.Fatal(err) + } + if err := copyPedAppTree(filepath.Join(src, "mprcontents"), filepath.Join(dir, "mprcontents")); err != nil { + t.Fatal(err) + } + out := &bytes.Buffer{} + exec := New(out) + exec.SetBackendFactory(func() backend.FullBackend { return modelsdkbackend.New() }) + if err := exec.Execute(&ast.ConnectStmt{Path: filepath.Join(dir, "PedApp.mpr")}); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = exec.Execute(&ast.DisconnectStmt{}) }) + return exec, out +} + +func afRun(t *testing.T, exec *Executor, src string) error { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse %q: %v", src, errs[0]) + } + for _, s := range prog.Statements { + if err := exec.Execute(s); err != nil { + return err + } + } + return nil +} + +// valFeedbackUnit returns VAL_Feedback's unit ID and stored bytes. +func valFeedbackUnit(t *testing.T, exec *Executor) (model.ID, []byte) { + t.Helper() + ctx := exec.newExecContext(context.Background()) + h, err := getHierarchy(ctx) + if err != nil { + t.Fatal(err) + } + all, err := ctx.Backend.ListMicroflows() + if err != nil { + t.Fatal(err) + } + for _, m := range all { + if m.Name == "VAL_Feedback" && h.GetModuleName(h.FindModuleID(m.ContainerID)) == "FeedbackModule" { + raw, err := ctx.Backend.GetRawUnitBytes(m.ID) + if err != nil { + t.Fatal(err) + } + return m.ID, append([]byte(nil), raw...) + } + } + t.Fatal("FeedbackModule.VAL_Feedback not found") + return "", nil +} + +// flowView indexes a stored flow's top-level objects and flows by $ID. +type flowView struct { + doc bson.D + objs map[string]bson.D + flows map[string]bson.D + order []string // object ids in storage order +} + +func parseFlowView(t *testing.T, raw []byte) flowView { + t.Helper() + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + t.Fatal(err) + } + v := flowView{doc: d, objs: map[string]bson.D{}, flows: map[string]bson.D{}} + oc, _ := afGet(d, "ObjectCollection").(bson.D) + for _, el := range afList(afGet(oc, "Objects")) { + o := el.(bson.D) + id := afIDOf(afGet(o, "$ID")) + v.objs[id] = o + v.order = append(v.order, id) + } + for _, el := range afList(afGet(d, "Flows")) { + f := el.(bson.D) + v.flows[afIDOf(afGet(f, "$ID"))] = f + } + return v +} + +func afGet(d bson.D, k string) any { + for _, e := range d { + if e.Key == k { + return e.Value + } + } + return nil +} + +func afList(v any) []any { + a, _ := v.(bson.A) + if len(a) > 0 { + if _, ok := a[0].(int32); ok { + return a[1:] + } + } + return a +} + +func afIDOf(v any) string { + b, _ := v.(primitive.Binary) + return types.BlobToUUID(b.Data) +} + +func afMarshal(t *testing.T, v any) []byte { + t.Helper() + b, err := bson.Marshal(v) + if err != nil { + t.Fatal(err) + } + return b +} + +// without returns d minus the named keys, for comparing the rest. +func afWithout(d bson.D, keys ...string) bson.D { + var out bson.D + for _, e := range d { + skip := false + for _, k := range keys { + skip = skip || e.Key == k + } + if !skip { + out = append(out, e) + } + } + return out +} + +// changedKeys lists the keys whose values differ between two elements (one +// level deep, plus the Line's vectors, which is where a flow keeps its curve). +func afChangedKeys(t *testing.T, a, b bson.D) []string { + t.Helper() + var out []string + keys := map[string]bool{} + for _, e := range a { + keys[e.Key] = true + } + for _, e := range b { + keys[e.Key] = true + } + for k := range keys { + av, bv := afGet(a, k), afGet(b, k) + if k == "Line" { + al, _ := av.(bson.D) + bl, _ := bv.(bson.D) + for _, lk := range []string{"OriginControlVector", "DestinationControlVector"} { + if fmt.Sprint(afGet(al, lk)) != fmt.Sprint(afGet(bl, lk)) { + out = append(out, "Line."+lk) + } + } + if !bytes.Equal(afMarshal(t, afWithout(al, "OriginControlVector", "DestinationControlVector")), + afMarshal(t, afWithout(bl, "OriginControlVector", "DestinationControlVector"))) { + out = append(out, "Line") + } + continue + } + if !bytes.Equal(afMarshal(t, bson.D{{Key: "v", Value: av}}), afMarshal(t, bson.D{{Key: "v", Value: bv}})) { + out = append(out, k) + } + } + return afSort(out) +} + +func afSort(s []string) []string { + for i := 1; i < len(s); i++ { + for j := i; j > 0 && s[j] < s[j-1]; j-- { + s[j], s[j-1] = s[j-1], s[j] + } + } + return s +} + +func afPoint(d bson.D) (int, int) { + s, _ := afGet(d, "RelativeMiddlePoint").(string) + x, y, _ := strings.Cut(s, ";") + px, _ := strconv.Atoi(x) + py, _ := strconv.Atoi(y) + return px, py +} + +func afSize(d bson.D) (int, int) { + s, _ := afGet(d, "Size").(string) + x, y, _ := strings.Cut(s, ";") + px, _ := strconv.Atoi(x) + py, _ := strconv.Atoi(y) + return px, py +} + +// objectAt returns the id of the stored top-level object at (x, y). +func (v flowView) objectAt(t *testing.T, x, y int, typ string) string { + t.Helper() + for _, id := range v.order { + o := v.objs[id] + if px, py := afPoint(o); px == x && py == y && afGet(o, "$Type") == typ { + return id + } + } + t.Fatalf("no %s at (%d, %d)", typ, x, y) + return "" +} + +// The acceptance test of plan item 4.2 (ako/mxcli#736): one `log` inserted +// after $IsValidEmail in the Studio Pro-drawn VAL_Feedback. Only the new +// activity, the two flows around it and the positions moved to make room may +// differ; every other element — its $ID, its curve, every merge — must come +// through byte-identical. +func TestAlterMicroflow_PedApp_InsertAfterChangesOnlyTheSplice(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + before := parseFlowView(t, raw) + javaCall := before.objectAt(t, 980, 200, "Microflows$ActionActivity") + split := before.objectAt(t, 1155, 200, "Microflows$ExclusiveSplit") + + script := "alter microflow FeedbackModule.VAL_Feedback {\n" + + " insert after $IsValidEmail { log info node 'Feedback' 'Email checked'; }\n" + + "};" + if n := strings.Count(script, "\n") + 1; n > 5 { + t.Fatalf("the acceptance script is %d lines; the plan allows 5", n) + } + if err := afRun(t, exec, script); err != nil { + t.Fatalf("alter: %v", err) + } + _, rawAfter := valFeedbackUnit(t, exec) + after := parseFlowView(t, rawAfter) + + // The document around the flow is untouched. + if !bytes.Equal(afMarshal(t, afWithout(before.doc, "ObjectCollection", "Flows")), + afMarshal(t, afWithout(after.doc, "ObjectCollection", "Flows"))) { + t.Error("a property of the microflow document itself changed") + } + ocBefore, _ := afGet(before.doc, "ObjectCollection").(bson.D) + ocAfter, _ := afGet(after.doc, "ObjectCollection").(bson.D) + if !bytes.Equal(afMarshal(t, afWithout(ocBefore, "Objects")), afMarshal(t, afWithout(ocAfter, "Objects"))) { + t.Error("a property of the object collection changed") + } + + // Objects: every stored one survives with its $ID and in its place in the + // list; exactly one is new, a log activity; the rest differ at most in + // position, and only by the shift that made room. + for i, id := range before.order { + if after.order[i] != id { + t.Fatalf("stored object %d moved in the list: %s became %s", i, id, after.order[i]) + } + } + if got := len(after.order) - len(before.order); got != 1 { + t.Fatalf("want exactly one new object, got %d", got) + } + newID := after.order[len(after.order)-1] + newObj := after.objs[newID] + if action, _ := afGet(newObj, "Action").(bson.D); afGet(newObj, "$Type") != "Microflows$ActionActivity" || + afGet(action, "$Type") != "Microflows$LogMessageAction" { + t.Fatalf("the new object is not a log activity: %v", afGet(newObj, "$Type")) + } + shifted := 0 + javaX, _ := afPoint(before.objs[javaCall]) + for _, id := range before.order { + b, a := before.objs[id], after.objs[id] + changed := afChangedKeys(t, b, a) + if len(changed) == 0 { + continue + } + if len(changed) != 1 || changed[0] != "RelativeMiddlePoint" { + t.Errorf("object %s (%v) changed more than its position: %v", id, afGet(b, "$Type"), changed) + continue + } + bx, by := afPoint(b) + ax, ay := afPoint(a) + if ay != by || ax <= bx || bx <= javaX { + t.Errorf("object %s moved from (%d,%d) to (%d,%d): only objects past the insertion point may move, and only along the flow", + id, bx, by, ax, ay) + } + shifted++ + } + if shifted == 0 { + t.Error("nothing was shifted, yet the gap after $IsValidEmail is too narrow for an activity: placement did not run") + } + // The new activity overlaps nothing. + nx, ny := afPoint(newObj) + nw, nh := afSize(newObj) + for _, id := range after.order { + if id == newID { + continue + } + o := after.objs[id] + ox, oy := afPoint(o) + ow, oh := afSize(o) + if abs(nx-ox)*2 < nw+ow && abs(ny-oy)*2 < nh+oh { + t.Errorf("the new activity at (%d,%d) overlaps %v at (%d,%d)", nx, ny, afGet(o, "$Type"), ox, oy) + } + } + + // Flows: every stored flow survives; one is new (log -> split); one is + // rewired (java call -> log), and only at its destination end. + var added []string + for id := range after.flows { + if _, ok := before.flows[id]; !ok { + added = append(added, id) + } + } + if len(added) != 1 { + t.Fatalf("want exactly one new flow, got %d", len(added)) + } + nf := after.flows[added[0]] + if afIDOf(afGet(nf, "OriginPointer")) != newID || afIDOf(afGet(nf, "DestinationPointer")) != split { + t.Error("the new flow does not run from the log to 'Email is Valid?'") + } + rewired := 0 + for id, b := range before.flows { + a, ok := after.flows[id] + if !ok { + t.Errorf("stored flow %s is gone", id) + continue + } + changed := afChangedKeys(t, b, a) + if len(changed) == 0 { + continue + } + rewired++ + if afIDOf(afGet(b, "OriginPointer")) != javaCall || afIDOf(afGet(a, "DestinationPointer")) != newID { + t.Errorf("flow %s changed but is not the flow out of $IsValidEmail: %v", id, changed) + } + for _, k := range changed { + switch k { + case "DestinationPointer", "DestinationConnectionIndex", "Line.DestinationControlVector": + default: + t.Errorf("the rewired flow changed %s; only its destination end may change", k) + } + } + } + if rewired != 1 { + t.Errorf("want exactly one rewired flow, got %d", rewired) + } + // The new flow ends where the rewired one used to. + var oldIn bson.D + for id, b := range before.flows { + if afIDOf(afGet(b, "OriginPointer")) == javaCall { + oldIn = b + _ = id + } + } + if fmt.Sprint(afGet(nf, "DestinationConnectionIndex")) != fmt.Sprint(afGet(oldIn, "DestinationConnectionIndex")) { + t.Error("the new flow does not enter 'Email is Valid?' on the side the old flow did") + } + + // And the result reads back: describe shows the log between the two. + var buf bytes.Buffer + exec.output = &buf + if err := afRun(t, exec, "describe microflow FeedbackModule.VAL_Feedback;"); err != nil { + t.Fatal(err) + } + body := buf.String() + iCall := strings.Index(body, "$IsValidEmail = call java action") + iLog := strings.Index(body, "log info node 'Feedback' 'Email checked';") + iSplit := strings.Index(body, "@caption 'Email is Valid?'") + if iCall < 0 || iLog < iCall || iSplit < iLog { + t.Errorf("describe does not show the log between the call and the decision:\n%s", body) + } +} + +// The control: an alter with no operations reads the unit, patches nothing +// and writes nothing — the stored bytes survive the decode/encode round trip +// exactly, so any difference the test above sees is the splice's. +func TestAlterMicroflow_PedApp_EmptyAlterChangesNothing(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + if err := afRun(t, exec, "alter microflow FeedbackModule.VAL_Feedback { };"); err != nil { + t.Fatalf("alter: %v", err) + } + _, rawAfter := valFeedbackUnit(t, exec) + if !bytes.Equal(raw, rawAfter) { + t.Error("an empty alter changed the stored unit") + } +} + +// Drop joins the flow into the dropped activity to the one after it, and +// takes away only the activity and the flow that left it. +func TestAlterMicroflow_PedApp_Drop(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + before := parseFlowView(t, raw) + target := before.objectAt(t, 1305, 460, "Microflows$ActionActivity") + mergeAfter := before.objectAt(t, 1460, 460, "Microflows$ExclusiveMerge") + + if err := afRun(t, exec, "alter microflow FeedbackModule.VAL_Feedback { drop set $ValidFeedback = false @3; };"); err != nil { + t.Fatalf("alter: %v", err) + } + _, rawAfter := valFeedbackUnit(t, exec) + after := parseFlowView(t, rawAfter) + + if _, ok := after.objs[target]; ok { + t.Fatal("the dropped activity is still there") + } + if len(after.objs) != len(before.objs)-1 || len(after.flows) != len(before.flows)-1 { + t.Fatalf("want one object and one flow fewer, got %d->%d objects, %d->%d flows", + len(before.objs), len(after.objs), len(before.flows), len(after.flows)) + } + for id, b := range before.objs { + if id == target { + continue + } + if !bytes.Equal(afMarshal(t, b), afMarshal(t, after.objs[id])) { + t.Errorf("object %s changed", id) + } + } + rewired := 0 + for id, b := range before.flows { + a, ok := after.flows[id] + if !ok { + if afIDOf(afGet(b, "OriginPointer")) != target { + t.Errorf("flow %s is gone but did not leave the dropped activity", id) + } + continue + } + if changed := afChangedKeys(t, b, a); len(changed) > 0 { + rewired++ + if afIDOf(afGet(b, "DestinationPointer")) != target || afIDOf(afGet(a, "DestinationPointer")) != mergeAfter { + t.Errorf("flow %s changed but is not the flow into the dropped activity: %v", id, changed) + } + } + } + if rewired != 1 { + t.Errorf("want one rewired flow, got %d", rewired) + } + if bytes.Contains(rawAfter, uuidBytes(target)) { + t.Error("the unit still contains the dropped activity's $ID") + } +} + +func uuidBytes(id string) []byte { return types.UUIDToBlob(id) } + +// Replace puts a two-activity fragment where one activity was: the flows in +// and out are re-pointed, everything past it moves along to make room. +func TestAlterMicroflow_PedApp_Replace(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + before := parseFlowView(t, raw) + target := before.objectAt(t, 1305, 460, "Microflows$ActionActivity") + + err := afRun(t, exec, `alter microflow FeedbackModule.VAL_Feedback { + replace set $ValidFeedback = false @3 with { + set $ValidFeedback = false; + log warning node 'Feedback' 'Email rejected'; + } + };`) + if err != nil { + t.Fatalf("alter: %v", err) + } + _, rawAfter := valFeedbackUnit(t, exec) + after := parseFlowView(t, rawAfter) + if _, ok := after.objs[target]; ok { + t.Fatal("the replaced activity is still there") + } + if got := len(after.objs) - len(before.objs); got != 1 { + t.Errorf("want one object more (two in, one out), got %+d", got) + } + if got := len(after.flows) - len(before.flows); got != 1 { + t.Errorf("want one flow more (the fragment's own), got %+d", got) + } + for id := range before.flows { + if _, ok := after.flows[id]; !ok { + t.Errorf("stored flow %s is gone; replace keeps the flows in and out", id) + } + } + if bytes.Contains(rawAfter, uuidBytes(target)) { + t.Error("the unit still contains the replaced activity's $ID") + } + var buf bytes.Buffer + exec.output = &buf + if err := afRun(t, exec, "describe microflow FeedbackModule.VAL_Feedback;"); err != nil { + t.Fatal(err) + } + if !strings.Contains(buf.String(), "log warning node 'Feedback' 'Email rejected';") { + t.Errorf("describe does not show the replacement:\n%s", buf.String()) + } +} + +// What the splice cannot do safely, it refuses — before writing anything. +func TestAlterMicroflow_PedApp_Refusals(t *testing.T) { + cases := []struct{ name, op, want string }{ + {"insert after a decision", `insert after 'Email is Valid?' { log info 'x'; }`, "which branch"}, + {"drop a decision", `drop 'Email is Valid?';`, "cannot drop"}, + {"drop a variable still read", `drop $IsValidEmail;`, "still used"}, + {"drop the end", `drop return $ValidFeedback;`, "cannot drop"}, + {"declare an existing variable", `insert after $IsValidEmail { declare $ValidFeedback Boolean = true; }`, "already has"}, + {"use a variable not yet declared", `insert after $ValidFeedback { log info 'x {1}' with ({1} = toString($IsValidEmail)); }`, "not declared on the path"}, + {"ambiguous target", `drop set $ValidFeedback = false;`, "add an ordinal"}, + {"unknown target", `drop $Nope;`, "no activity matches"}, + {"a fragment that returns", `insert after $IsValidEmail { return false; }`, "ends the flow"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + err := afRun(t, exec, "alter microflow FeedbackModule.VAL_Feedback { "+tc.op+" };") + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("want an error containing %q, got %v", tc.want, err) + } + if _, rawAfter := valFeedbackUnit(t, exec); !bytes.Equal(raw, rawAfter) { + t.Error("a refused alter changed the stored unit") + } + }) + } +} + +var _ = microflows.ActionActivity{} + +// alter nanoflow goes through the same splice: the unit is a +// Microflows$Nanoflow with the same object collection and flows. +func TestAlterNanoflow_PedApp_InsertBefore(t *testing.T) { + exec, _ := openPedAppFixture(t) + err := afRun(t, exec, `alter nanoflow FeedbackModule.ACT_Feedback_ClearImage { + insert before call javascript action * { declare $Cleared Boolean = true; } + };`) + if err != nil { + t.Fatalf("alter: %v", err) + } + var buf bytes.Buffer + exec.output = &buf + if err := afRun(t, exec, "describe nanoflow FeedbackModule.ACT_Feedback_ClearImage;"); err != nil { + t.Fatal(err) + } + body := buf.String() + iChange := strings.Index(body, "change $Feedback") + iDeclare := strings.Index(body, "declare $Cleared Boolean = true;") + iCall := strings.Index(body, "call javascript action") + if iChange < 0 || iDeclare < iChange || iCall < iDeclare { + t.Errorf("describe does not show the declare between the change and the call:\n%s", body) + } +} diff --git a/mdl/executor/register_stubs.go b/mdl/executor/register_stubs.go index 130cefb93..aac52f5ba 100644 --- a/mdl/executor/register_stubs.go +++ b/mdl/executor/register_stubs.go @@ -519,6 +519,9 @@ func registerLintHandlers(r *Registry) { } func registerAlterPageHandlers(r *Registry) { + r.Register(&ast.AlterFlowStmt{}, func(ctx *ExecContext, stmt ast.Statement) error { + return execAlterFlow(ctx, stmt.(*ast.AlterFlowStmt)) + }) r.Register(&ast.AlterPageStmt{}, func(ctx *ExecContext, stmt ast.Statement) error { return execAlterPage(ctx, stmt.(*ast.AlterPageStmt)) }) diff --git a/mdl/executor/registry_test.go b/mdl/executor/registry_test.go index 9340826a3..a59e8e787 100644 --- a/mdl/executor/registry_test.go +++ b/mdl/executor/registry_test.go @@ -177,6 +177,7 @@ func allKnownStatements() []ast.Statement { &ast.AlterODataClientStmt{}, &ast.AlterODataServiceStmt{}, &ast.AlterPageStmt{}, + &ast.AlterFlowStmt{}, &ast.AlterPagesLayoutStmt{}, &ast.AlterPagesStylingStmt{}, &ast.AlterProjectSecurityStmt{}, diff --git a/mdl/executor/stmt_summary.go b/mdl/executor/stmt_summary.go index 362f91603..bcb135d79 100644 --- a/mdl/executor/stmt_summary.go +++ b/mdl/executor/stmt_summary.go @@ -198,6 +198,9 @@ func stmtSummary(stmt ast.Statement) string { case *ast.AlterStylingStmt: return fmt.Sprintf("alter styling on %s %s widget %s", s.ContainerType, s.ContainerName, s.WidgetName) + case *ast.AlterFlowStmt: + return fmt.Sprintf("alter %s %s", s.Kind(), s.Name) + // ALTER PAGE / ALTER SNIPPET case *ast.AlterPageStmt: ct := s.ContainerType diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 472d38961..2aab0c61f 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -173,6 +173,11 @@ alterStatement // there; a scroll-container region is addressed as `layoutContainer.top`, // because a region has no Name of its own. | ALTER alterDocumentType qualifiedName LBRACE alterOperation+ RBRACE + // The generic ALTER on a microflow or nanoflow (ADR-0012 decision 3): a + // graph splice into the stored flow. Its targets are content addresses + // (`$Var`, `'Caption'`, a statement pattern) and its fragments are + // microflow statements, so it has its own operation rule. + | ALTER (MICROFLOW | NANOFLOW) qualifiedName LBRACE alterFlowOperation* RBRACE | alterPagesLayoutStatement | alterPagesStylingStatement | ALTER WORKFLOW qualifiedName alterWorkflowAction+ SEMICOLON? @@ -312,6 +317,36 @@ alterTarget | STRING_LITERAL (AT NUMBER_LITERAL)? ; +/** + * `alter microflow` / `alter nanoflow` operations (ADR-0012 decision 3, + * ako/mxcli#736): + * + * ```mdl + * alter microflow FeedbackModule.VAL_Feedback { + * insert after $IsValidEmail { log info node 'Feedback' 'Email checked'; } + * insert before 'Email is Valid?' { … } + * replace commit $Order with { commit $Order with events; } + * drop log * node 'Debug' *; + * } + * ``` + * + * A fragment is written exactly as the same statements are in `create + * microflow`. A target is a content address, resolved by mfmutator: `$Var` + * (the activity that outputs it), `'Caption'`, or a statement pattern with `*` + * wildcards, each optionally followed by `@n`. A pattern is any run of tokens, + * so the target is taken as raw text up to the `{`, `with` or `;` that ends it; + * that is why `drop` needs its semicolon. + */ +alterFlowOperation + : INSERT (AFTER | BEFORE) alterFlowTarget LBRACE microflowBody RBRACE SEMICOLON? + | REPLACE alterFlowTarget WITH LBRACE microflowBody RBRACE SEMICOLON? + | DROP alterFlowTarget SEMICOLON + ; + +alterFlowTarget + : ~(LBRACE | RBRACE | SEMICOLON | WITH)+ + ; + // ALTER PAGES [IN ] SET LAYOUT = Module.Layout [MAP (...)] [WHERE LAYOUT = Module.Old] // // The bulk form is the real one: an app has one layout and many pages, so diff --git a/mdl/visitor/visitor_alter.go b/mdl/visitor/visitor_alter.go index c878412ef..859174a7f 100644 --- a/mdl/visitor/visitor_alter.go +++ b/mdl/visitor/visitor_alter.go @@ -17,6 +17,10 @@ func (b *Builder) ExitAlterStatement(ctx *parser.AlterStatementContext) { b.exitAlterDocumentStatement(ctx) return } + if ctx.MICROFLOW() != nil || ctx.NANOFLOW() != nil { + b.exitAlterFlowStatement(ctx) + return + } // Handle ALTER PAGES … SET LAYOUT (the bulk repoint) if sub := ctx.AlterPagesStylingStatement(); sub != nil { diff --git a/mdl/visitor/visitor_alter_flow.go b/mdl/visitor/visitor_alter_flow.go new file mode 100644 index 000000000..784848fda --- /dev/null +++ b/mdl/visitor/visitor_alter_flow.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/grammar/parser" +) + +// exitAlterFlowStatement builds an AlterFlowStmt from +// ALTER MICROFLOW|NANOFLOW Module.Name { insert / replace / drop }. +func (b *Builder) exitAlterFlowStatement(ctx *parser.AlterStatementContext) { + stmt := &ast.AlterFlowStmt{Nanoflow: ctx.NANOFLOW() != nil} + if qn := ctx.QualifiedName(); qn != nil { + stmt.Name = buildQualifiedName(qn) + } + for _, opCtx := range ctx.AllAlterFlowOperation() { + op := opCtx.(*parser.AlterFlowOperationContext) + o := &ast.AlterFlowOperation{Target: alterFlowTargetText(op.AlterFlowTarget())} + switch { + case op.INSERT() != nil && op.BEFORE() != nil: + o.Op = ast.AlterFlowInsertBefore + case op.INSERT() != nil: + o.Op = ast.AlterFlowInsertAfter + case op.REPLACE() != nil: + o.Op = ast.AlterFlowReplace + default: + o.Op = ast.AlterFlowDrop + } + if body := op.MicroflowBody(); body != nil { + o.Body = buildMicroflowBody(body) + } + stmt.Operations = append(stmt.Operations, o) + } + b.statements = append(b.statements, stmt) +} + +// alterFlowTargetText returns a target as the author wrote it — whitespace and +// quoting included — since a statement pattern is matched token by token +// against describe's rendering, and the grammar only knows it as a run of +// arbitrary tokens. +func alterFlowTargetText(ctx parser.IAlterFlowTargetContext) string { + if ctx == nil { + return "" + } + start, stop := ctx.GetStart(), ctx.GetStop() + if start == nil || stop == nil { + return strings.TrimSpace(ctx.GetText()) + } + return strings.TrimSpace(start.GetInputStream().GetText(start.GetStart(), stop.GetStop())) +} diff --git a/mdl/visitor/visitor_alter_flow_test.go b/mdl/visitor/visitor_alter_flow_test.go new file mode 100644 index 000000000..2391ffc99 --- /dev/null +++ b/mdl/visitor/visitor_alter_flow_test.go @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// alter microflow / nanoflow (ADR-0012 decision 3, ako/mxcli#736): targets are +// content addresses kept as written, fragments are microflow statements. +func TestAlterFlow_OperationsAndTargets(t *testing.T) { + prog, errs := Build(`alter microflow FeedbackModule.VAL_Feedback { + insert after $IsValidEmail { log info node 'Feedback' 'Email checked'; } + insert before 'Email is Valid?' @2 { declare $n Integer = 1; set $n = 2; } + replace log * node 'Debug' * with { log warning node 'Debug' 'x'; } + drop set $ValidFeedback = false @3; + };`) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + stmt, ok := prog.Statements[0].(*ast.AlterFlowStmt) + if !ok { + t.Fatalf("want *ast.AlterFlowStmt, got %T", prog.Statements[0]) + } + if stmt.Nanoflow || stmt.Name.String() != "FeedbackModule.VAL_Feedback" { + t.Errorf("header: nanoflow=%v name=%s", stmt.Nanoflow, stmt.Name) + } + want := []struct { + op ast.AlterFlowOpKind + target string + body int + }{ + {ast.AlterFlowInsertAfter, "$IsValidEmail", 1}, + {ast.AlterFlowInsertBefore, "'Email is Valid?' @2", 2}, + {ast.AlterFlowReplace, "log * node 'Debug' *", 1}, + {ast.AlterFlowDrop, "set $ValidFeedback = false @3", 0}, + } + if len(stmt.Operations) != len(want) { + t.Fatalf("want %d operations, got %d", len(want), len(stmt.Operations)) + } + for i, w := range want { + got := stmt.Operations[i] + if got.Op != w.op || got.Target != w.target || len(got.Body) != w.body { + t.Errorf("op %d: got %q %q body=%d, want %q %q body=%d", i, got.Op, got.Target, len(got.Body), w.op, w.target, w.body) + } + } +} + +func TestAlterFlow_Nanoflow(t *testing.T) { + prog, errs := Build(`alter nanoflow M.NF { drop $X; }`) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + stmt := prog.Statements[0].(*ast.AlterFlowStmt) + if !stmt.Nanoflow || stmt.Operations[0].Op != ast.AlterFlowDrop || stmt.Operations[0].Target != "$X" { + t.Errorf("got %+v %+v", stmt, stmt.Operations[0]) + } +} From e184384d34e7290dfa48c919805641802470d0fc Mon Sep 17 00:00:00 2001 From: Ako Date: Sun, 27 Sep 2026 08:49:04 +0000 Subject: [PATCH 4/7] describe with handles: the error-handler block's { is not part of the handle A handle has to parse as an alter target, and a target ends at the { of a fragment. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/mdl-executor.jsonl | 1 + mdl/executor/cmd_microflows_handles.go | 8 +++++++- mdl/executor/cmd_microflows_handles_test.go | 15 +++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 92a698438..129a64784 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -728,3 +728,4 @@ {"area": "mdl/executor", "date": "2026-09-26", "symptom": "`describe fragment from page M.P widget w` (and `from snippet`) fails for every container and every widget — including ones `describe page` prints — with `not found in page M.P`, a message that does not even name the widget", "cause": "Visitor stores DescribeFragmentFromStmt.ContainerType as \"PAGE\"/\"SNIPPET\"; describeFragmentFrom switched on \"page\"/\"snippet\" with no default, so neither branch ran, the widget list stayed empty, and the fall-through reported the widget missing. Third instance of this split: ALTER PAGE (#402), DESCRIBE/ALTER STYLING (#631)", "file": "`mdl/executor/cmd_fragments.go` (`describeFragmentFrom`)", "insight": "The mismatch hid behind a plausible error because a switch on the discriminator had no default: an unmatched container type looked like an empty container, and an empty container looks like a missing widget. Normalise with strings.ToLower where the discriminator is consumed (the house convention — cmd_styling, cmd_alter_page, validate_alter_* all do) AND make the default an error, so the next casing drift fails loudly instead of reporting the wrong thing. The existing mock tests hand-built the AST and so agreed with the handler; only a test that goes visitor.Build → NewRegistry().Dispatch pins the contract between the two layers (cmd_fragments_from_test.go). Verified on Evora: Administration.Account_Edit/textBox6 and AgentCommons.Snippet_Agent_Details/dataView7 now describe", "refs": ["#402", "#631"]} {"area": "mdl/executor", "date": "2026-09-26", "symptom": "describe output that does not re-parse or loses data (ako/mxcli#707): an entity string default or validation message containing ' was emitted unescaped; so were module-role descriptions, published OData/REST Path/Version/Namespace/Summary/Folder, and REST client BaseUrl/Path/header values; an agent `mcp service` block with a Description lacked the comma after `Enabled`; workflow decision / parallel split captions came back only as `-- caption` comments (replay reset them to 'Decision' / 'Parallel split'); `describe demo user` emitted `password '***'`, which replay stored as the password; `describe settings` printed `DatabasePassword = ''`.", "cause": "Hand-rolled `'%s'` emit sites that put the quotes and the escaping in different places (the #1006 source-scan guard covered only cmd_workflows.go); a block emitter with no separator logic, unlike its sibling; captions treated as commentary although the grammar has `comment '…'` for both activities; secrets printed as data, with a placeholder the writer took literally.", "file": "mdl/executor/cmd_entities_describe.go, cmd_security.go, cmd_security_write.go, cmd_odata.go, cmd_published_rest.go, cmd_rest_clients.go, cmd_agenteditor_agents.go, cmd_workflows.go, cmd_settings.go", "fix": "Every emit site uses mdlQuoted; TestDescribers_HaveNoHandRolledStringLiterals now scans all seven describer files. MCP block writes the comma like the tool block. workflowCaptionClauses emits `comment '…'` for a non-default caption and computes the name clause against the caption the writer will store. DatabasePassword is omitted with a comment (create or modify is a patch, so replay keeps it). Demo users are described as `create or modify … password '***'`, and the executor treats '***' as 'keep the stored password', refusing it for a user that does not exist.", "insight": "Assert round trips by reparsing describe output with the real visitor and comparing the AST value to the stored one, not by substring. For secrets the right placeholder is one the WRITER understands: omission works where the create is a patch (configuration); where the grammar requires the value (demo user) give the placeholder a meaning (keep stored) and refuse it where that meaning is empty, so a replay can neither leak nor silently set a credential.", "test": "mdl/executor/issue707_describe_roundtrip_test.go"} {"area": "mdl/executor", "date": "2026-09-26", "symptom": "describe microflow \u2026 with handles (ako/mxcli#713) printed no handle for an activity inside an `on error { \u2026 }` block, and the alter-target resolver counted such activities after the whole main flow: on SUB_Feedback_PostToAppInsights `return * @1` picked `return $Response`, although describe prints the handler's `return empty` first. Also `$Response` did not address a REST call whose output is on its result handling (and cast, create list, web service, workflow, XML/JSON, database-query outputs).", "cause": "Error-handler bodies are rendered by collectErrorHandlerStatements, a second describer that returned bare strings and never wrote the source map, so those nodes had no line to rank or print a handle at; unranked candidates were appended last. The output-variable switch was copied from actionOutputVariableName, which had drifted from the formatter.", "file": "mdl/executor/cmd_microflows_show_helpers.go; mdl/backend/mfmutator/target.go", "fix": "collectErrorHandlerStatementSpans reports each handler-body object's statement span; emitActivityStatement and emitCommentedErrorHandler record them in the source map (additive entries in ELK sourceMap too). mfmutator.OutputVariable reads the variable where the formatter does, for every action it prints as `$X = \u2026`. A comment rendering (`-- Unsupported \u2026`) is no statement, so it never becomes a handle.", "insight": "Any node the describer prints through a side path must enter the source map, or everything keyed on print order (ordinals, handles, ELK highlighting) silently disagrees with the text. Check ranking against a Studio Pro flow that has a handler body, not just VAL_Feedback.", "test": "TestDescribeWithHandles_ErrorHandlerBody, TestMicroflowTargets_PedAppEveryFlowRanksAndResolves, TestOutputVariable_EveryActionDescribePrintsAnAssignmentFor, TestCandidate_CommentRenderingIsNoStatement"} +{"area": "mdl/executor", "date": "2026-09-27", "symptom": "describe microflow \u2026 with handles printed `-- handle: commit $Order on error {` for an activity with a custom error handler (TestApp Services.SaveOrder). The handle could not be used: an `alter microflow` target ends at the `{` that opens a fragment, so `insert after commit $Order on error { \u2026 }` parsed the handler brace as the fragment.", "cause": "printedStatement ended an action's statement at a line ending in `;` or `{` and kept the `{`, which belongs to the error-handler block describe opens, not to the statement.", "file": "mdl/executor/cmd_microflows_handles.go", "fix": "printedStatement strips the trailing `{` of an error-handler block opener, so the handle is `commit $Order on error`, which parses as a target and still matches the activity.", "insight": "A handle is only useful if it can be written back as a target in the grammar that consumes it; test a printed handle by parsing it, not only by resolving it.", "test": "TestPrintedStatement_ErrorHandlerBlockOpenerIsNotPartOfTheStatement"} diff --git a/mdl/executor/cmd_microflows_handles.go b/mdl/executor/cmd_microflows_handles.go index 2172169fa..1c871e241 100644 --- a/mdl/executor/cmd_microflows_handles.go +++ b/mdl/executor/cmd_microflows_handles.go @@ -88,9 +88,15 @@ func printedStatement(obj microflows.MicroflowObject, body []string, r elkSource return strings.Join(parts, " ") } default: - if strings.HasSuffix(line, ";") || strings.HasSuffix(line, "{") { + if strings.HasSuffix(line, ";") { return strings.Join(parts, " ") } + if strings.HasSuffix(line, "{") { + // The `{` opens the error handler block. It is not part of + // the statement: a handle is written as an alter target, and a + // target ends where a fragment's `{` begins. + return strings.TrimSpace(strings.TrimSuffix(strings.Join(parts, " "), "{")) + } } if len(parts) >= 50 { break diff --git a/mdl/executor/cmd_microflows_handles_test.go b/mdl/executor/cmd_microflows_handles_test.go index 056cdd56e..10ae2ce01 100644 --- a/mdl/executor/cmd_microflows_handles_test.go +++ b/mdl/executor/cmd_microflows_handles_test.go @@ -308,3 +308,18 @@ func TestDescribeWithHandles_ErrorHandlerBody(t *testing.T) { t.Errorf("with handles minus the handle lines differs from plain describe:\n%s\n---\n%s", strings.Join(stripped, "\n"), strings.Join(plain, "\n")) } } + +// A handle has to be writable as an `alter microflow` target, and a target +// ends at the `{` that opens a fragment. So the `{` describe prints after an +// activity with a custom error handler is not part of its statement. +func TestPrintedStatement_ErrorHandlerBlockOpenerIsNotPartOfTheStatement(t *testing.T) { + body := []string{" commit $Order on error {", " return false;", " };"} + obj := &microflows.ActionActivity{} + got := printedStatement(obj, body, elkSourceRange{StartLine: 0, EndLine: 2}) + if got != "commit $Order on error" { + t.Errorf("printed statement %q, want %q", got, "commit $Order on error") + } + if _, err := mfmutator.ParseTarget(got); err != nil { + t.Errorf("the handle does not parse as a target: %v", err) + } +} From 02979d99b1263f04cadf7857c9cdfc928c66507a Mon Sep 17 00:00:00 2001 From: Ako <andrej@koelewijn.net> Date: Sun, 27 Sep 2026 08:49:04 +0000 Subject: [PATCH 5/7] mcp: alter microflow insert over the Studio Pro MCP backend Runs the shared splice on the stored flow and sends the difference as ped_update_document path operations in one update, after checking the live document still matches the .mpr (PED addresses entries by index). Drop and replace are refused: PED does not roll back a removal when an update fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- mdl/backend/mcp/microflow_mutator.go | 499 ++++++++++++++++++++++ mdl/backend/mcp/microflow_mutator_test.go | 207 +++++++++ mdl/backend/mcp/unsupported_gen.go | 5 + 3 files changed, 711 insertions(+) create mode 100644 mdl/backend/mcp/microflow_mutator.go create mode 100644 mdl/backend/mcp/microflow_mutator_test.go diff --git a/mdl/backend/mcp/microflow_mutator.go b/mdl/backend/mcp/microflow_mutator.go new file mode 100644 index 000000000..4bbe61ad1 --- /dev/null +++ b/mdl/backend/mcp/microflow_mutator.go @@ -0,0 +1,499 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "encoding/json" + "fmt" + "strconv" + "strings" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// ALTER MICROFLOW / NANOFLOW over MCP (plan item 4.2f). +// +// The splice itself is the shared engine (mfmutator), run on the stored form of +// the flow as the local .mpr holds it — the same decisions, placement and +// refusals as on the modelsdk backend. What differs is the write: Studio Pro +// 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 +// 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. + +// OpenMicroflowForMutation opens a microflow or nanoflow for splicing. +func (b *Backend) OpenMicroflowForMutation(unitID model.ID) (backend.MicroflowMutator, error) { + raw, err := b.reader.GetRawUnitBytes(unitID) + if err != nil { + return nil, fmt.Errorf("alter over MCP reads the stored flow from the local project: %w", err) + } + var d bson.D + if err := bson.Unmarshal(raw, &d); err != nil { + return nil, err + } + docType, _ := docValue(d, "$Type").(string) + name, _ := docValue(d, "Name").(string) + qn, err := b.flowQualifiedName(unitID, docType, name) + if err != nil { + return nil, err + } + deps := &mcpFlowDeps{b: b, docType: docType, qn: qn, stored: d, objects: map[string]microflows.MicroflowObject{}, + flows: map[string]*microflows.SequenceFlow{}, annotations: map[string]*microflows.AnnotationFlow{}} + var work bson.D + if err := bson.Unmarshal(raw, &work); err != nil { + return nil, err + } + m, err := mfmutator.New(work, unitID, deps) + if err != nil { + return nil, err + } + return &mcpFlowMutator{Mutator: m, qn: qn}, nil +} + +// mcpFlowMutator is the shared splice, limited to what PED applies safely: +// inserts. A PED update that fails is rolled back EXCEPT for its removals, +// which persist (PED's own warning, and measured: a failed update left the +// removed activity gone together with the flows attached to it). A drop or a +// replace needs a removal, so over MCP they are refused rather than risked on +// the live model; run them against the .mpr instead. +type mcpFlowMutator struct { + *mfmutator.Mutator + qn string +} + +func (m *mcpFlowMutator) Replace(model.ID, *backend.MicroflowFragment) error { + return fmt.Errorf("replace is not supported by the MCP backend yet: Studio Pro does not roll back a removal when an update fails; run without --mcp to alter %s in the .mpr", m.qn) +} + +func (m *mcpFlowMutator) Drop(model.ID) error { + return fmt.Errorf("drop is not supported by the MCP backend yet: Studio Pro does not roll back a removal when an update fails; run without --mcp to alter %s in the .mpr", m.qn) +} + +func (b *Backend) flowQualifiedName(id model.ID, docType, name string) (string, error) { + var container model.ID + switch docType { + case microflowDocType: + mf, err := b.GetMicroflow(id) + if err != nil { + return "", err + } + container = mf.ContainerID + case "Microflows$Nanoflow": + nfs, err := b.reader.ListNanoflows() + if err != nil { + return "", err + } + for _, nf := range nfs { + if nf.ID == id { + container = nf.ContainerID + } + } + default: + return "", fmt.Errorf("unit %s is a %s, not a microflow or nanoflow", id, docType) + } + mod, err := b.moduleNameForContainer(container) + if err != nil { + return "", err + } + return mod + "." + name, nil +} + +// mcpFlowDeps is mfmutator.Deps for the MCP backend. Serialize produces only +// what the splice reads (identity, type, geometry, pointers) and keeps the +// domain object, which is what PED is sent; SaveUnit turns the spliced +// document into PED operations. +type mcpFlowDeps struct { + b *Backend + docType string + qn string + stored bson.D + + objects map[string]microflows.MicroflowObject + flows map[string]*microflows.SequenceFlow + annotations map[string]*microflows.AnnotationFlow +} + +func binaryOf(id model.ID) primitive.Binary { + return primitive.Binary{Subtype: 0, Data: types.UUIDToBlob(string(id))} +} + +func (d *mcpFlowDeps) SerializeObject(obj microflows.MicroflowObject) (bson.D, error) { + d.objects[string(obj.GetID())] = obj + p := obj.GetPosition() + w, h := 0, 0 + if s, ok := obj.(interface{ GetSize() model.Size }); ok { + w, h = s.GetSize().Width, s.GetSize().Height + } + return bson.D{ + {Key: "$ID", Value: binaryOf(obj.GetID())}, + {Key: "$Type", Value: "mcp-new-object"}, + {Key: "RelativeMiddlePoint", Value: fmt.Sprintf("%d;%d", p.X, p.Y)}, + {Key: "Size", Value: fmt.Sprintf("%d;%d", w, h)}, + }, nil +} + +func (d *mcpFlowDeps) SerializeSequenceFlow(f *microflows.SequenceFlow) (bson.D, error) { + d.flows[string(f.ID)] = f + return bson.D{ + {Key: "$ID", Value: binaryOf(f.ID)}, + {Key: "$Type", Value: "Microflows$SequenceFlow"}, + {Key: "DestinationConnectionIndex", Value: int32(f.DestinationConnectionIndex)}, + {Key: "DestinationPointer", Value: binaryOf(f.DestinationID)}, + {Key: "IsErrorHandler", Value: f.IsErrorHandler}, + {Key: "Line", Value: bson.D{ + {Key: "$Type", Value: "Microflows$BezierCurve"}, + {Key: "DestinationControlVector", Value: f.DestinationControlVector}, + {Key: "OriginControlVector", Value: f.OriginControlVector}, + }}, + {Key: "OriginConnectionIndex", Value: int32(f.OriginConnectionIndex)}, + {Key: "OriginPointer", Value: binaryOf(f.OriginID)}, + }, nil +} + +func (d *mcpFlowDeps) SerializeAnnotationFlow(f *microflows.AnnotationFlow) (bson.D, error) { + d.annotations[string(f.ID)] = f + return bson.D{ + {Key: "$ID", Value: binaryOf(f.ID)}, + {Key: "$Type", Value: "Microflows$AnnotationFlow"}, + {Key: "DestinationPointer", Value: binaryOf(f.DestinationID)}, + {Key: "OriginPointer", Value: binaryOf(f.OriginID)}, + }, nil +} + +// SaveUnit sends the difference between the stored and the spliced document. +func (d *mcpFlowDeps) SaveUnit(_ string, contents []byte) error { + var spliced bson.D + if err := bson.Unmarshal(contents, &spliced); err != nil { + return err + } + ops, err := d.b.flowPatchOps(d, spliced) + if err != nil { + return err + } + if len(ops) == 0 { + return nil + } + if err := d.b.checkLiveFlowMatches(d.docType, d.qn, d.stored); err != nil { + return err + } + // Only inserts reach here (mcpFlowMutator refuses the rest), so there is + // nothing to remove; a removal would mean the splice did something this + // path was not built for, and PED would not roll it back on a failure. + for _, op := range ops { + if op.Operation.Type == "remove" { + return fmt.Errorf("%s: refusing to send a removal over MCP", d.qn) + } + } + // One update: PED applies it all or, on a failure, nothing. + if err := d.b.pedUpdateDoc(d.docType, d.qn, ops...); err != nil { + return err + } + return d.b.pedCheckDocument(d.docType, d.qn) +} + +// flowList returns a unit's top-level objects or flows, in stored order, with +// their $IDs. +func flowList(d bson.D, objects bool) (ids []string, docs []bson.D) { + list := docValue(d, "Flows") + if objects { + oc, _ := docValue(d, "ObjectCollection").(bson.D) + list = docValue(oc, "Objects") + } + a, _ := list.(bson.A) + for i, el := range a { + if i == 0 { + if _, marker := el.(int32); marker { + continue + } + } + e, ok := el.(bson.D) + if !ok { + continue + } + b, _ := docValue(e, "$ID").(primitive.Binary) + ids = append(ids, types.BlobToUUID(b.Data)) + docs = append(docs, e) + } + return ids, docs +} + +func docValue(d bson.D, key string) any { + for _, e := range d { + if e.Key == key { + return e.Value + } + } + return nil +} + +func pointerID(d bson.D, key string) string { + b, _ := docValue(d, key).(primitive.Binary) + return types.BlobToUUID(b.Data) +} + +func storedPointToPED(s string) map[string]int { + x, y, _ := strings.Cut(s, ";") + px, _ := strconv.Atoi(x) + py, _ := strconv.Atoi(y) + return map[string]int{"x": px, "y": py} +} + +func pedVector(s string) map[string]int { + p := storedPointToPED(s) + return map[string]int{"width": p["x"], "height": p["y"]} +} + +var pedSides = []string{"Top", "Right", "Bottom", "Left"} + +// 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"). +var pedQualifiedFlowType = map[string]string{ + "Microflows$MicroflowParameter": "Microflows$MicroflowParameterObject", +} + +// flowPatchOps derives the PED operations that turn the stored document into +// the spliced one. Order matters, because PED addresses entries by index: +// 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 + set := func(path string, v any) { + ops = append(ops, pedOpEntry{Path: path, Operation: pedOperation{Type: "set", Value: v}}) + } + oldObjIDs, oldObjs := flowList(d.stored, true) + newObjIDs, newObjs := flowList(spliced, true) + oldFlowIDs, oldFlows := flowList(d.stored, false) + newFlowIDs, newFlows := flowList(spliced, false) + + path := map[string]string{} + oldObjIndex := map[string]int{} + for i, id := range oldObjIDs { + oldObjIndex[id] = i + path[id] = fmt.Sprintf("/objectCollection/objects/%d", i) + } + survivingObj := map[string]bool{} + var added []string + for i, id := range newObjIDs { + if idx, ok := oldObjIndex[id]; ok { + survivingObj[id] = true + before, _ := docValue(oldObjs[idx], "RelativeMiddlePoint").(string) + after, _ := docValue(newObjs[i], "RelativeMiddlePoint").(string) + if before != after { + set(path[id]+"/relativeMiddlePoint", storedPointToPED(after)) + } + continue + } + added = append(added, id) + } + for j, id := range added { + path[id] = fmt.Sprintf("/objectCollection/objects/%d", len(oldObjIDs)+j) + } + for _, id := range added { + obj, ok := d.objects[id] + if !ok { + return 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 + } + var sets []pedOpEntry + skeletonObject(m, path[id], func(p string, v any) { + sets = append(sets, 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. + p := obj.GetPosition() + set(path[id]+"/relativeMiddlePoint", map[string]int{"x": p.X, "y": p.Y}) + for nested, np := range idPath { + path[string(nested)] = np + } + } + ref := func(id string) (string, error) { + p, ok := path[id] + if !ok { + return "", fmt.Errorf("a flow points at %s, which is not a top-level object PED can address", id) + } + return "$id(" + p + ")", nil + } + + oldFlowIndex := map[string]int{} + for i, id := range oldFlowIDs { + oldFlowIndex[id] = i + } + survivingFlow := map[string]bool{} + var addedFlows []string + for i, id := range newFlowIDs { + idx, ok := oldFlowIndex[id] + if !ok { + addedFlows = append(addedFlows, id) + continue + } + survivingFlow[id] = true + fp := fmt.Sprintf("/flows/%d", idx) + o, n := oldFlows[idx], newFlows[i] + for _, end := range []string{"Origin", "Destination"} { + key := strings.ToLower(end) + if pointerID(o, end+"Pointer") != pointerID(n, end+"Pointer") { + r, err := ref(pointerID(n, end+"Pointer")) + if err != nil { + return nil, err + } + set(fp+"/"+key, r) + } + if fmt.Sprint(docValue(o, end+"ConnectionIndex")) != fmt.Sprint(docValue(n, end+"ConnectionIndex")) { + set(fp+"/"+key+"ConnectionIndex", docValue(n, end+"ConnectionIndex")) + } + ol, _ := docValue(o, "Line").(bson.D) + nl, _ := docValue(n, "Line").(bson.D) + if ov, nv := fmt.Sprint(docValue(ol, end+"ControlVector")), fmt.Sprint(docValue(nl, end+"ControlVector")); ov != nv && nv != "" { + set(fp+"/line/"+key+"ControlVector", pedVector(nv)) + } + } + } + for j, id := range addedFlows { + fp := fmt.Sprintf("/flows/%d", len(oldFlowIDs)+j) + if af, ok := d.annotations[id]; ok { + o, err := ref(string(af.OriginID)) + if err != nil { + return nil, err + } + dst, err := ref(string(af.DestinationID)) + if err != nil { + return nil, err + } + ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: map[string]any{ + "$Type": "Microflows$AnnotationFlow", "originId": o, "destinationId": dst}}}) + continue + } + f, ok := d.flows[id] + if !ok { + return nil, fmt.Errorf("internal: spliced flow %s was not serialized", id) + } + o, err := ref(string(f.OriginID)) + if err != nil { + return nil, err + } + dst, err := ref(string(f.DestinationID)) + if err != nil { + return nil, err + } + v := map[string]any{"$Type": "Microflows$SequenceFlow", "originId": o, "destinationId": dst} + if f.OriginConnectionIndex >= 0 && f.OriginConnectionIndex < 4 { + v["originConnectionSide"] = pedSides[f.OriginConnectionIndex] + } + if f.DestinationConnectionIndex >= 0 && f.DestinationConnectionIndex < 4 { + v["destinationConnectionSide"] = pedSides[f.DestinationConnectionIndex] + } + cv, err := mapCaseValue(f.CaseValue) + if err != nil { + return nil, err + } + if cv != nil { + v["caseValue"] = cv + } + ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "add", Value: v}}) + 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)) + } + } + + for i := len(oldFlowIDs) - 1; i >= 0; i-- { + if !survivingFlow[oldFlowIDs[i]] { + idx := i + ops = append(ops, pedOpEntry{Path: "/flows", Operation: pedOperation{Type: "remove", Index: &idx}}) + } + } + for i := len(oldObjIDs) - 1; i >= 0; i-- { + if !survivingObj[oldObjIDs[i]] { + idx := i + ops = append(ops, pedOpEntry{Path: "/objectCollection/objects", Operation: pedOperation{Type: "remove", Index: &idx}}) + } + } + return ops, nil +} + +// checkLiveFlowMatches compares the live document's objects and flows with the +// stored ones the splice ran on: the same count, and each object the same type +// at the same position. PED addresses them by index, so a live document that +// has moved on since the .mpr was saved would have the patch applied to other +// elements than the ones the splice chose. +func (b *Backend) checkLiveFlowMatches(docType, qn string, stored bson.D) error { + res, err := b.client.CallTool("ped_read_document", map[string]any{ + "documentType": docType, + "documentName": qn, + "paths": []string{"/objectCollection/objects", "/flows"}, + }) + if err != nil { + return err + } + if res.IsError { + return fmt.Errorf("ped_read_document %s: %s", qn, pedStripReminder(res.Text)) + } + var body struct { + Results []struct { + Path string `json:"path"` + Result json.RawMessage `json:"result"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(pedStripReminder(res.Text)), &body); err != nil { + return fmt.Errorf("read %s from Studio Pro: %w", qn, err) + } + stale := func(what string) error { + return fmt.Errorf("%s in Studio Pro no longer matches the local project (%s); save in Studio Pro so the .mpr is current, then retry", qn, what) + } + _, objs := flowList(stored, true) + _, flows := flowList(stored, false) + for _, r := range body.Results { + var live []map[string]any + if err := json.Unmarshal(r.Result, &live); err != nil { + return fmt.Errorf("read %s %s from Studio Pro: %w", qn, r.Path, err) + } + switch r.Path { + case "/flows": + if len(live) != len(flows) { + return stale(fmt.Sprintf("%d flows live, %d stored", len(live), len(flows))) + } + case "/objectCollection/objects": + if len(live) != len(objs) { + return stale(fmt.Sprintf("%d objects live, %d stored", len(live), len(objs))) + } + for i, o := range objs { + typ, _ := docValue(o, "$Type").(string) + if q, ok := pedQualifiedFlowType[typ]; ok { + typ = q + } + if live[i]["$Type"] != typ { + return stale(fmt.Sprintf("object %d is a %v live, a %s stored", i, live[i]["$Type"], typ)) + } + rp, _ := docValue(o, "RelativeMiddlePoint").(string) + want := storedPointToPED(rp) + pt, _ := live[i]["relativeMiddlePoint"].(map[string]any) + if fmt.Sprint(pt["x"]) != strconv.Itoa(want["x"]) || fmt.Sprint(pt["y"]) != strconv.Itoa(want["y"]) { + return stale(fmt.Sprintf("object %d is at %v live, at %s stored", i, pt, rp)) + } + } + } + } + return nil +} diff --git a/mdl/backend/mcp/microflow_mutator_test.go b/mdl/backend/mcp/microflow_mutator_test.go new file mode 100644 index 000000000..7fad8b990 --- /dev/null +++ b/mdl/backend/mcp/microflow_mutator_test.go @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "encoding/json" + "fmt" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mfmutator" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func mfID(name string) primitive.Binary { + b := make([]byte, 16) + copy(b, name) + return primitive.Binary{Data: b} +} + +func mfUUID(name string) string { return types.BlobToUUID(mfID(name).Data) } + +func mfObj(name, typ string, x int) bson.D { + return bson.D{ + {Key: "$ID", Value: mfID(name)}, + {Key: "$Type", Value: typ}, + {Key: "RelativeMiddlePoint", Value: fmt.Sprintf("%d;200", x)}, + {Key: "Size", Value: "120;60"}, + } +} + +func mfFlow(name, from, to string) bson.D { + return bson.D{ + {Key: "$ID", Value: mfID(name)}, + {Key: "$Type", Value: "Microflows$SequenceFlow"}, + {Key: "DestinationConnectionIndex", Value: int32(3)}, + {Key: "DestinationPointer", Value: mfID(to)}, + {Key: "IsErrorHandler", Value: false}, + {Key: "Line", Value: bson.D{{Key: "$Type", Value: "Microflows$BezierCurve"}, + {Key: "DestinationControlVector", Value: "-30;0"}, {Key: "OriginControlVector", Value: "30;0"}}}, + {Key: "OriginConnectionIndex", Value: int32(1)}, + {Key: "OriginPointer", Value: mfID(from)}, + } +} + +// storedReduce is the shape of TestApp's Microflows.MicroflowReduce, the flow +// the live check ran on: a start, three activities 170 apart, an end. +func storedReduce() bson.D { + return bson.D{ + {Key: "$ID", Value: mfID("unit")}, + {Key: "$Type", Value: "Microflows$Microflow"}, + {Key: "Flows", Value: bson.A{int32(3), mfFlow("f0", "start", "a"), mfFlow("f1", "a", "b"), mfFlow("f2", "b", "end")}}, + {Key: "Name", Value: "Reduce"}, + {Key: "ObjectCollection", Value: bson.D{ + {Key: "$ID", Value: mfID("oc")}, + {Key: "$Type", Value: "Microflows$MicroflowObjectCollection"}, + {Key: "Objects", Value: bson.A{int32(3), + mfObj("start", "Microflows$StartEvent", -98), + mfObj("end", "Microflows$EndEvent", 564), + mfObj("a", "Microflows$ActionActivity", 214), + mfObj("b", "Microflows$ActionActivity", 404), + }}, + }}, + } +} + +func openFlowMutator(t *testing.T, b *Backend) (*mcpFlowMutator, *mcpFlowDeps) { + t.Helper() + stored := storedReduce() + raw, _ := bson.Marshal(stored) + var storedD, work bson.D + _ = bson.Unmarshal(raw, &storedD) + _ = bson.Unmarshal(raw, &work) + deps := &mcpFlowDeps{b: b, docType: microflowDocType, qn: "M.Reduce", stored: storedD, + objects: map[string]microflows.MicroflowObject{}, flows: map[string]*microflows.SequenceFlow{}, + annotations: map[string]*microflows.AnnotationFlow{}} + m, err := mfmutator.New(work, "unit", deps) + if err != nil { + t.Fatal(err) + } + return &mcpFlowMutator{Mutator: m, qn: "M.Reduce"}, deps +} + +func logFragment() *backend.MicroflowFragment { + id := model.ID(types.GenerateID()) + act := &microflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: id}, + Position: model.Point{X: 360, Y: 200}, + Size: model.Size{Width: 120, Height: 60}, + }}, + Action: &microflows.LogMessageAction{LogLevel: "Info", LogNodeName: "'Reduce'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "reduced"}}}, + } + return &backend.MicroflowFragment{Objects: []microflows.MicroflowObject{act}, Entry: id, Exit: id} +} + +// 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. +func TestFlowPatchOps_InsertAfter(t *testing.T) { + var sent []any + 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": + sent = append(sent, args["operations"]) + return "SUCCESS: All operations have been performed successfully.", false + case "ped_check_errors": + return "No errors found.", false + } + return "SUCCESS", false + }) + b := &Backend{client: f.connectClient(t)} + m, _ := openFlowMutator(t, b) + 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) != 1 { + t.Fatalf("want one ped_update_document, got %d", len(sent)) + } + js, _ := json.Marshal(sent[0]) + got := string(js) + var ops []pedOpEntry + if err := json.Unmarshal(js, &ops); err != nil { + t.Fatal(err) + } + has := func(path, typ, value string) bool { + for _, op := range ops { + v, _ := json.Marshal(op.Operation.Value) + if op.Path == path && op.Operation.Type == typ && strings.Contains(string(v), value) { + return true + } + } + return false + } + for _, w := range []struct{ path, typ, value string }{ + {"/objectCollection/objects/3/relativeMiddlePoint", "set", `"x":534`}, // b moved along + {"/objectCollection/objects", "add", `"Microflows$ActionActivity"`}, + {"/objectCollection/objects/4/action", "set", `"Microflows$LogMessageAction"`}, + {"/flows/1/destination", "set", `$id(/objectCollection/objects/4)`}, + {"/flows", "add", `"destinationId":"$id(/objectCollection/objects/3)"`}, + {"/flows", "add", `"originId":"$id(/objectCollection/objects/4)"`}, + } { + if !has(w.path, w.typ, w.value) { + t.Errorf("no %s %s with %s in:\n%s", w.typ, w.path, w.value, got) + } + } + if strings.Contains(got, `"remove"`) { + t.Errorf("an insert sent a removal:\n%s", 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) { + updates := 0 + f := newFakePED(t, func(name string, _ map[string]any) (string, bool) { + switch name { + case "ped_read_document": + return `{"results":[{"path":"/objectCollection/objects","result":[{},{},{},{},{}]},{"path":"/flows","result":[{},{},{}]}]}`, false + case "ped_update_document": + updates++ + } + return "SUCCESS", false + }) + b := &Backend{client: f.connectClient(t)} + m, _ := openFlowMutator(t, b) + if err := m.InsertAfter(model.ID(mfUUID("a")), logFragment()); err != nil { + t.Fatal(err) + } + err := m.Save() + if err == nil || !strings.Contains(err.Error(), "no longer matches the local project") { + t.Fatalf("want the stale-document refusal, got %v", err) + } + if updates != 0 { + t.Error("an update was sent to a document that did not match") + } +} + +// Drop and replace need a removal, which PED does not roll back when an +// update fails; over MCP they are refused. +func TestMCPFlowMutator_RefusesRemovals(t *testing.T) { + m, _ := openFlowMutator(t, &Backend{}) + if err := m.Drop(model.ID(mfUUID("a"))); err == nil || !strings.Contains(err.Error(), "not supported by the MCP backend") { + t.Errorf("drop: %v", err) + } + if err := m.Replace(model.ID(mfUUID("a")), logFragment()); err == nil || !strings.Contains(err.Error(), "not supported by the MCP backend") { + t.Errorf("replace: %v", err) + } +} diff --git a/mdl/backend/mcp/unsupported_gen.go b/mdl/backend/mcp/unsupported_gen.go index a1dc96a9c..abae5e3a3 100644 --- a/mdl/backend/mcp/unsupported_gen.go +++ b/mdl/backend/mcp/unsupported_gen.go @@ -992,6 +992,11 @@ func (unsupportedBackend) MoveViewEntitySourceDocument(_ string, _ model.ID, _ s return } +func (unsupportedBackend) OpenMicroflowForMutation(_ model.ID) (r0 backend.MicroflowMutator, err1 error) { + err1 = errUnsupported("OpenMicroflowForMutation") + return +} + func (unsupportedBackend) OpenPageForMutation(_ model.ID) (r0 backend.PageMutator, err1 error) { err1 = errUnsupported("OpenPageForMutation") return From 534a59542676a9d3b6b358d0a72fc68ef94c61e4 Mon Sep 17 00:00:00 2001 From: Ako <andrej@koelewijn.net> Date: Sun, 27 Sep 2026 09:04:00 +0000 Subject: [PATCH 6/7] mfmutator: drop/replace of a loop removes its body flows Loop body flows are stored in the unit's Flows list, not in the loop, so removing a Studio Pro loop with two or more body activities left them pointing at removed objects and Save refused the unit (TestApp ACT_ConflictedWorkflowHelper_ApplyJumpTo). mx check 11.14 on the dropped and replaced copies gives the baseline error list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- mdl/backend/mfmutator/splice.go | 25 +++++++++++++- mdl/backend/mfmutator/splice_test.go | 50 ++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index afc303b30..011cf3e1e 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -385,6 +385,7 @@ func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) erro } setPointer(af.doc, key, frag.Entry) } + m.removeFlows(g.bodyFlows(x.id)) return m.removeObject(x) } @@ -412,7 +413,8 @@ func (m *Mutator) Drop(target model.ID) error { setInt(in.doc, "DestinationConnectionIndex", destIdx) setVector(in.doc, "DestinationControlVector", destVec) } - drop := map[string]bool{out.id: true} + drop := g.bodyFlows(x.id) + drop[out.id] = true for _, af := range g.annotationFlows(x.id) { drop[af.id] = true } @@ -420,6 +422,27 @@ func (m *Mutator) Drop(target model.ID) error { return m.removeObject(x) } +// bodyFlows returns the flows that run inside loop's body, at any depth. They +// are stored in the unit's Flows list, not in the loop, so taking the loop out +// has to take them too; left behind they would point at removed objects. +func (g *graph) bodyFlows(loop string) map[string]bool { + inside := func(id string) bool { + for n := g.nodes[id]; n != nil && n.loop != ""; n = g.nodes[n.loop] { + if n.loop == loop { + return true + } + } + return false + } + out := map[string]bool{} + for _, f := range g.flows { + if inside(f.origin) || inside(f.dest) { + out[f.id] = true + } + } + return out +} + // removable checks that x can be taken out of the flow and returns the one // flow that leaves it. func removable(g *graph, x *node, verb string) (flowRef, error) { diff --git a/mdl/backend/mfmutator/splice_test.go b/mdl/backend/mfmutator/splice_test.go index 34bc09358..a665bff52 100644 --- a/mdl/backend/mfmutator/splice_test.go +++ b/mdl/backend/mfmutator/splice_test.go @@ -352,3 +352,53 @@ func TestSplice_DropJoinsTheFlows(t *testing.T) { } } } + +// A loop's body flows are stored in the unit's Flows list, not in the loop. +// Dropping or replacing the loop takes them with it; left behind, they point at +// the removed body objects and Save refuses the unit (a Studio Pro-authored +// loop with two body activities, ACT_ConflictedWorkflowHelper_ApplyJumpTo in +// TestApp, hit exactly that). +func TestSplice_DropOrReplaceALoopTakesItsBodyFlows(t *testing.T) { + build := func() ([]bson.D, []bson.D) { + loop := obj("loop", "Microflows$LoopedActivity", 420, 200) + loop = append(loop, bson.E{Key: "ObjectCollection", Value: bson.D{ + {Key: "$ID", Value: bin("loopoc")}, + {Key: "$Type", Value: "Microflows$MicroflowObjectCollection"}, + {Key: "Objects", Value: bson.A{int32(3), obj("inner", "Microflows$ActionActivity", 100, 60), obj("inner2", "Microflows$ActionActivity", 260, 60)}}, + }}) + objs := []bson.D{ + obj("start", "Microflows$StartEvent", 100, 200), + obj("a", "Microflows$ActionActivity", 250, 200), + loop, + obj("end", "Microflows$EndEvent", 700, 200), + } + flows := []bson.D{ + flow("f1", "start", "a", 1, 3, false), + flow("f2", "a", "loop", 1, 3, false), + flow("fi", "inner", "inner2", 1, 3, false), + flow("f3", "loop", "end", 1, 3, false), + } + return objs, flows + } + ops := map[string]func(m *Mutator) error{ + "drop": func(m *Mutator) error { return m.Drop(model.ID(uid("loop"))) }, + "replace": func(m *Mutator) error { return m.Replace(model.ID(uid("loop")), oneActivity()) }, + } + for name, op := range ops { + t.Run(name, func(t *testing.T) { + objs, flows := build() + m, deps := newMutator(t, unit(objs, flows)) + if err := op(m); err != nil { + t.Fatalf("%s: %v", name, err) + } + if err := m.Save(); err != nil { + t.Fatalf("save: %v", err) + } + for _, gone := range []string{"loop", "inner", "inner2", "fi"} { + if bytes.Contains(deps.saved, types.UUIDToBlob(uid(gone))) { + t.Errorf("%s is still in the unit", gone) + } + } + }) + } +} From 7abe76bb6a212bafb59b85fa970821aa1233d22f Mon Sep 17 00:00:00 2001 From: Ako <andrej@koelewijn.net> Date: Sun, 27 Sep 2026 09:04:00 +0000 Subject: [PATCH 7/7] alter microflow: scope checks span the whole statement Each operation was checked against the flow as stored only. Measured with mx check 11.14 on TestApp: two fragments declaring the same variable gave CE0111, and a fragment reading a variable a drop in the same statement removed gave CE0109, after "Altered microflow". The context now tracks what earlier operations declared, read and removed. Also count a fragment loop's iterator as the fragment's own, so loop fragments are no longer refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .../fix-issue/findings/mdl-executor.jsonl | 1 + mdl/executor/cmd_alter_flow.go | 77 ++++++++++++++++--- mdl/executor/cmd_alter_flow_pedapp_test.go | 74 ++++++++++++++++++ 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 129a64784..4947fb831 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -729,3 +729,4 @@ {"area": "mdl/executor", "date": "2026-09-26", "symptom": "describe output that does not re-parse or loses data (ako/mxcli#707): an entity string default or validation message containing ' was emitted unescaped; so were module-role descriptions, published OData/REST Path/Version/Namespace/Summary/Folder, and REST client BaseUrl/Path/header values; an agent `mcp service` block with a Description lacked the comma after `Enabled`; workflow decision / parallel split captions came back only as `-- caption` comments (replay reset them to 'Decision' / 'Parallel split'); `describe demo user` emitted `password '***'`, which replay stored as the password; `describe settings` printed `DatabasePassword = '<plaintext>'`.", "cause": "Hand-rolled `'%s'` emit sites that put the quotes and the escaping in different places (the #1006 source-scan guard covered only cmd_workflows.go); a block emitter with no separator logic, unlike its sibling; captions treated as commentary although the grammar has `comment '…'` for both activities; secrets printed as data, with a placeholder the writer took literally.", "file": "mdl/executor/cmd_entities_describe.go, cmd_security.go, cmd_security_write.go, cmd_odata.go, cmd_published_rest.go, cmd_rest_clients.go, cmd_agenteditor_agents.go, cmd_workflows.go, cmd_settings.go", "fix": "Every emit site uses mdlQuoted; TestDescribers_HaveNoHandRolledStringLiterals now scans all seven describer files. MCP block writes the comma like the tool block. workflowCaptionClauses emits `comment '…'` for a non-default caption and computes the name clause against the caption the writer will store. DatabasePassword is omitted with a comment (create or modify is a patch, so replay keeps it). Demo users are described as `create or modify … password '***'`, and the executor treats '***' as 'keep the stored password', refusing it for a user that does not exist.", "insight": "Assert round trips by reparsing describe output with the real visitor and comparing the AST value to the stored one, not by substring. For secrets the right placeholder is one the WRITER understands: omission works where the create is a patch (configuration); where the grammar requires the value (demo user) give the placeholder a meaning (keep stored) and refuse it where that meaning is empty, so a replay can neither leak nor silently set a credential.", "test": "mdl/executor/issue707_describe_roundtrip_test.go"} {"area": "mdl/executor", "date": "2026-09-26", "symptom": "describe microflow \u2026 with handles (ako/mxcli#713) printed no handle for an activity inside an `on error { \u2026 }` block, and the alter-target resolver counted such activities after the whole main flow: on SUB_Feedback_PostToAppInsights `return * @1` picked `return $Response`, although describe prints the handler's `return empty` first. Also `$Response` did not address a REST call whose output is on its result handling (and cast, create list, web service, workflow, XML/JSON, database-query outputs).", "cause": "Error-handler bodies are rendered by collectErrorHandlerStatements, a second describer that returned bare strings and never wrote the source map, so those nodes had no line to rank or print a handle at; unranked candidates were appended last. The output-variable switch was copied from actionOutputVariableName, which had drifted from the formatter.", "file": "mdl/executor/cmd_microflows_show_helpers.go; mdl/backend/mfmutator/target.go", "fix": "collectErrorHandlerStatementSpans reports each handler-body object's statement span; emitActivityStatement and emitCommentedErrorHandler record them in the source map (additive entries in ELK sourceMap too). mfmutator.OutputVariable reads the variable where the formatter does, for every action it prints as `$X = \u2026`. A comment rendering (`-- Unsupported \u2026`) is no statement, so it never becomes a handle.", "insight": "Any node the describer prints through a side path must enter the source map, or everything keyed on print order (ordinals, handles, ELK highlighting) silently disagrees with the text. Check ranking against a Studio Pro flow that has a handler body, not just VAL_Feedback.", "test": "TestDescribeWithHandles_ErrorHandlerBody, TestMicroflowTargets_PedAppEveryFlowRanksAndResolves, TestOutputVariable_EveryActionDescribePrintsAnAssignmentFor, TestCandidate_CommentRenderingIsNoStatement"} {"area": "mdl/executor", "date": "2026-09-27", "symptom": "describe microflow \u2026 with handles printed `-- handle: commit $Order on error {` for an activity with a custom error handler (TestApp Services.SaveOrder). The handle could not be used: an `alter microflow` target ends at the `{` that opens a fragment, so `insert after commit $Order on error { \u2026 }` parsed the handler brace as the fragment.", "cause": "printedStatement ended an action's statement at a line ending in `;` or `{` and kept the `{`, which belongs to the error-handler block describe opens, not to the statement.", "file": "mdl/executor/cmd_microflows_handles.go", "fix": "printedStatement strips the trailing `{` of an error-handler block opener, so the handle is `commit $Order on error`, which parses as a target and still matches the activity.", "insight": "A handle is only useful if it can be written back as a target in the grammar that consumes it; test a printed handle by parsing it, not only by resolving it.", "test": "TestPrintedStatement_ErrorHandlerBlockOpenerIsNotPartOfTheStatement"} +{"area": "mdl/executor", "date": "2026-09-27", "symptom": "alter microflow (ako/mxcli#736) reported \"Altered microflow\" for statements mx check then rejected: two fragments in one statement declaring the same variable gave CE0111 Duplicate variable name; a fragment reading $X in the same statement as `drop $X` (either order) gave CE0109 Undefined variable. A loop fragment was refused as reading its own iterator, and drop/replace of a Studio Pro loop with more than one body activity was refused by the dangling-reference guard.", "cause": "The scope checks compared each operation with the flow as stored, never with what earlier operations of the same statement had declared, read or removed. The iterator of a fragment's loop is on its LoopSource, not an action output. Loop body flows are stored in the unit's Flows list, not in the loop, so removing the loop left them pointing at removed objects.", "file": "mdl/executor/cmd_alter_flow.go; mdl/backend/mfmutator/splice.go", "fix": "alterFlowContext tracks declaredByOps / readByOps / removedByOps across the statement's operations and checks each later operation against them; checkFragmentScope counts a fragment loop's iterator as its own; Drop and Replace remove graph.bodyFlows of a loop with it.", "insight": "A per-operation check against the stored document is only sound for a one-operation statement; every multi-operation test needs a case where operation 2 depends on operation 1. mx check on a copied TestApp is the cheap oracle: the CE numbers appear the moment a hygiene hole is hit.", "test": "TestAlterMicroflow_PedApp_ScopeSpansTheStatement; TestAlterMicroflow_PedApp_LoopFragmentDeclaresItsIterator; TestSplice_DropOrReplaceALoopTakesItsBodyFlows"} diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index 69f5897a2..d8aac4309 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -61,6 +61,7 @@ func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { if err := mut.Drop(target.ID); err != nil { return fail(err) } + a.noteRemoved(target, nil) continue } frag, err := a.buildFragment(ctx, op.Body) @@ -85,6 +86,10 @@ func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { if err != nil { return fail(err) } + if op.Op == ast.AlterFlowReplace { + a.noteRemoved(target, frag) + } + a.noteFragment(ctx, frag) } if err := mut.Save(); err != nil { return mdlerrors.NewBackend("save altered "+s.Kind(), err) @@ -102,6 +107,52 @@ type alterFlowContext struct { cands []mfmutator.Candidate entityNames map[model.ID]string microflowNames map[model.ID]string + + // What the statement's earlier operations did to the variables, so a later + // one is checked against the flow as it will be written, not as stored: + // the variables their fragments declare, the ones their fragments read + // (with who reads them), and the stored outputs they took away. + declaredByOps map[string]bool + readByOps map[string][]string + removedByOps map[string]bool +} + +// noteRemoved records that target's output is gone, unless the fragment that +// replaces it declares it again. +func (a *alterFlowContext) noteRemoved(target mfmutator.Candidate, replacement *backend.MicroflowFragment) { + v := target.OutputVariable + if v == "" || fragmentDeclares(replacement, v) { + return + } + a.removedByOps[v] = true +} + +// noteFragment records what an inserted or replacing fragment declares and +// reads. +func (a *alterFlowContext) noteFragment(ctx *ExecContext, frag *backend.MicroflowFragment) { + for _, obj := range frag.Objects { + if act, ok := obj.(*microflows.ActionActivity); ok { + if v := mfmutator.OutputVariable(act.Action); v != "" { + a.declaredByOps[v] = true + } + } + text := formatActivity(ctx, obj, a.entityNames, a.microflowNames) + for _, m := range variableRef.FindAllStringSubmatch(text, -1) { + a.readByOps[m[1]] = append(a.readByOps[m[1]], text) + } + } +} + +func fragmentDeclares(frag *backend.MicroflowFragment, v string) bool { + if frag == nil { + return false + } + for _, obj := range frag.Objects { + if act, ok := obj.(*microflows.ActionActivity); ok && mfmutator.OutputVariable(act.Action) == v { + return true + } + } + return false } func loadAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) (*alterFlowContext, error) { @@ -109,7 +160,8 @@ func loadAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) (*alterFlowContext, e if err != nil { return nil, mdlerrors.NewBackend("build hierarchy", err) } - a := &alterFlowContext{stmt: s, entityNames: getEntityNames(ctx, h)} + a := &alterFlowContext{stmt: s, entityNames: getEntityNames(ctx, h), + declaredByOps: map[string]bool{}, readByOps: map[string][]string{}, removedByOps: map[string]bool{}} // A copy: nanoflow names are added below, and the cached map is shared. a.microflowNames = map[model.ID]string{} for id, n := range getMicroflowNames(ctx, h) { @@ -336,6 +388,14 @@ func (a *alterFlowContext) checkFragmentScope(ctx *ExecContext, op *ast.AlterFlo } own := map[string]bool{} for _, obj := range frag.Objects { + // A loop's iterator exists only inside the loop, which the fragment + // brings along; it reads it there, so it is the fragment's own. + if loop, ok := obj.(*microflows.LoopedActivity); ok { + if src, ok := loop.LoopSource.(*microflows.IterableList); ok && src.VariableName != "" { + own[src.VariableName] = true + } + continue + } act, ok := obj.(*microflows.ActionActivity) if !ok { continue @@ -348,6 +408,9 @@ func (a *alterFlowContext) checkFragmentScope(ctx *ExecContext, op *ast.AlterFlo if existing[v] && !replacingSame { return fmt.Errorf("the fragment declares $%s, which the %s already has; choose another name", v, a.stmt.Kind()) } + if a.declaredByOps[v] { + return fmt.Errorf("the fragment declares $%s, which an earlier operation of this alter already declares; choose another name", v) + } own[v] = true } @@ -367,7 +430,7 @@ func (a *alterFlowContext) checkFragmentScope(ctx *ExecContext, op *ast.AlterFlo for _, obj := range frag.Objects { for _, m := range variableRef.FindAllStringSubmatch(formatActivity(ctx, obj, a.entityNames, a.microflowNames), -1) { v := m[1] - if seen[v] || systemVariables[v] || inScope[v] || own[v] { + if seen[v] || systemVariables[v] || own[v] || (inScope[v] && !a.removedByOps[v]) { continue } seen[v] = true @@ -415,15 +478,11 @@ func (a *alterFlowContext) upstreamOf(id model.ID, including bool) map[model.ID] // another activity still reads — unless the replacement declares it again. func (a *alterFlowContext) checkOutputUnused(target mfmutator.Candidate, replacement *backend.MicroflowFragment) error { v := target.OutputVariable - if v == "" { + if v == "" || fragmentDeclares(replacement, v) { return nil } - if replacement != nil { - for _, obj := range replacement.Objects { - if act, ok := obj.(*microflows.ActionActivity); ok && mfmutator.OutputVariable(act.Action) == v { - return nil - } - } + if readers := a.readByOps[v]; len(readers) > 0 { + return fmt.Errorf("$%s is read by what an earlier operation of this alter adds: %s", v, strings.Join(readers, "; ")) } ref := regexp.MustCompile(`\$` + regexp.QuoteMeta(v) + `\b`) var users []string diff --git a/mdl/executor/cmd_alter_flow_pedapp_test.go b/mdl/executor/cmd_alter_flow_pedapp_test.go index fc14786ea..791677917 100644 --- a/mdl/executor/cmd_alter_flow_pedapp_test.go +++ b/mdl/executor/cmd_alter_flow_pedapp_test.go @@ -569,3 +569,77 @@ func TestAlterNanoflow_PedApp_InsertBefore(t *testing.T) { t.Errorf("describe does not show the declare between the change and the call:\n%s", body) } } + +// The scope check covers the whole statement, not each operation against the +// stored flow alone. Measured with mx check 11.14 on TestApp before the fix: +// two inserts declaring the same variable gave CE0111 "Duplicate variable +// name", and an insert reading a variable a drop in the same statement took +// away gave CE0109 "Undefined variable" — both after "Altered microflow". +func TestAlterMicroflow_PedApp_ScopeSpansTheStatement(t *testing.T) { + t.Run("two fragments declare the same variable", func(t *testing.T) { + exec, _ := openPedAppFixture(t) + _, raw := valFeedbackUnit(t, exec) + err := afRun(t, exec, `alter microflow FeedbackModule.VAL_Feedback { + insert after $IsValidEmail { declare $Dup Boolean = true; } + insert after $ValidFeedback { declare $Dup Boolean = false; } + };`) + if err == nil || !strings.Contains(err.Error(), "$Dup") { + t.Fatalf("want the clash on $Dup refused, got %v", err) + } + if _, after := valFeedbackUnit(t, exec); !bytes.Equal(raw, after) { + t.Error("a refused alter changed the stored unit") + } + }) + + // SUB_Feedback_Sanitize reads its nine $Sanitized* outputs in one change; + // replacing that change first leaves $SanitizedPageName unread, so only a + // fragment of the second statement reads it. + const unread = `alter microflow FeedbackModule.SUB_Feedback_Sanitize { + replace change $Feedback (Subject = $SanitizedSubject, Description = $SanitizedDescription, SubmitterUUID = $SanitizedSubmitterUUID, SubmitterEmail = $SanitizedSubmitterEmail, SubmitterDisplayName = $SanitizedSubmitterDisplayName, ActiveUserRoles = $SanitizedActiveUserRoles, PageName = $SanitizedPageName, Browser = $SanitizedBrowser, EnvironmentURL = $SanitizedEnvironmentURL) with { log info 'sanitized'; } + };` + const use = `insert after $SanitizedSubmitterUUID { log info 'page {1}' with ({1} = $SanitizedPageName); }` + const drop = `drop $SanitizedPageName;` + for name, ops := range map[string]string{ + "insert a reader, then drop the producer": use + "\n" + drop, + "drop the producer, then insert a reader": drop + "\n" + use, + } { + t.Run(name, func(t *testing.T) { + exec, _ := openPedAppFixture(t) + if err := afRun(t, exec, unread); err != nil { + t.Fatalf("setup: %v", err) + } + err := afRun(t, exec, "alter microflow FeedbackModule.SUB_Feedback_Sanitize {\n"+ops+"\n};") + if err == nil || !strings.Contains(err.Error(), "$SanitizedPageName") { + t.Fatalf("want the read of a dropped $SanitizedPageName refused, got %v", err) + } + }) + } + // Control: each operation on its own is accepted. + t.Run("control", func(t *testing.T) { + for _, op := range []string{use, drop} { + exec, _ := openPedAppFixture(t) + if err := afRun(t, exec, unread); err != nil { + t.Fatalf("setup: %v", err) + } + if err := afRun(t, exec, "alter microflow FeedbackModule.SUB_Feedback_Sanitize {\n"+op+"\n};"); err != nil { + t.Errorf("%s: %v", op, err) + } + } + }) +} + +// A loop in the fragment declares its iterator; the scope check must count it +// as the fragment's own, or every loop fragment is refused as reading an +// undeclared variable. +func TestAlterMicroflow_PedApp_LoopFragmentDeclaresItsIterator(t *testing.T) { + exec, _ := openPedAppFixture(t) + err := afRun(t, exec, `alter microflow FeedbackModule.VAL_Feedback { + insert after $IsValidEmail { + $Items = create list of FeedbackModule.Feedback; + loop $Item in $Items begin log info 'item'; end loop; + } + };`) + if err != nil { + t.Fatalf("alter: %v", err) + } +}