From 7d3b4d889d3846485791a200c16ad7ebd02287fd Mon Sep 17 00:00:00 2001 From: Henrique Costa Date: Thu, 23 Apr 2026 17:38:30 +0200 Subject: [PATCH] fix: traverse through unpaired ExclusiveMerge in describe (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DESCRIBE MICROFLOW` silently truncated its output at an `ExclusiveMerge` that wasn't the matching end-of-branch point of an `ExclusiveSplit`. Every activity downstream of the merge was dropped. The issue reporter hit this on a manual retry-loop pattern — setup activities feed into a merge, the merge feeds into a REST call, and a back-edge from a "change retry count + delay" branch loops back into the same merge — so the describer stopped right after the retry-count declaration and never emitted the REST call, the retry branch, or anything else on the diagram. Root cause: `traverseFlow` returned unconditionally on every `ExclusiveMerge`, assuming the node was going to be handled by the split's branch traversal. That's true when the merge closes an IF/ELSE, but not when it's a pure junction for converging flows outside of a split. Fix: early-return only when the merge appears as a value in `splitMergeMap` (i.e. it is the paired end point of some split). When the merge isn't paired, walk through it as a pass-through — the same behaviour `traverseFlowUntilMerge` already has for intermediate merges. Added three focused regression tests: - `TestTraverseFlow_UnpairedMergeIsPassThrough` — the minimal case; also asserts no stray `end if;` is emitted. - `TestTraverseFlow_PairedMergeStillClosesIfElse` — regression guard for normal IF/ELSE; still closes with exactly one `end if;`. - `TestTraverseFlow_ManualRetryLoopPatternEmitsEverything` — closer shape of the user's microflow (setup → merge → call → decide → change → delay → merge loop back), verifies every activity shows up. Fixes #281. Co-Authored-By: Claude Opus 4.7 --- mdl/executor/cmd_microflows_show_helpers.go | 35 ++- .../cmd_microflows_unpaired_merge_test.go | 215 ++++++++++++++++++ 2 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 mdl/executor/cmd_microflows_unpaired_merge_test.go diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index 21200f44..6c646abb 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -455,8 +455,23 @@ func traverseFlow( return } - // Check if this is a merge point - if so, don't process it here (it will be handled by the split) + // Merge points are normally processed by the matching split's traversal — + // returning here avoids emitting an `end if;` twice. But a merge can also + // appear as a pure junction (multiple incoming flows converging outside + // of an IF), e.g. a manual retry loop where a "Call REST → decide → loop + // back" pattern routes the back-edge into a merge. Those merges are not + // values in splitMergeMap, and stopping there drops every activity after + // them (see issue #281). When that happens, walk through as a pass- + // through — same as traverseFlowUntilMerge already does for intermediate + // merges. if _, isMerge := obj.(*microflows.ExclusiveMerge); isMerge { + if isMergePairedWithSplit(currentID, splitMergeMap) { + return + } + visited[currentID] = true + for _, flow := range flowsByOrigin[currentID] { + traverseFlow(ctx, flow.DestinationID, activityMap, flowsByOrigin, flowsByDest, splitMergeMap, visited, entityNames, microflowNames, lines, indent, sourceMap, headerLineCount, annotationsByTarget) + } return } @@ -877,6 +892,24 @@ func emitLoopBody( } } +// isMergePairedWithSplit reports whether an ExclusiveMerge appears as the +// matching end-of-branch point for some ExclusiveSplit recorded in +// splitMergeMap (i.e., it closes an IF/ELSE block). Merges that aren't paired +// — e.g. a junction used as the loop-back target of a manual retry pattern — +// must be traversed as pass-through, otherwise every activity after them is +// dropped from describe output (issue #281). +// +// splitMergeMap is split-ID → merge-ID, so the merge is paired iff it appears +// as a value in the map. +func isMergePairedWithSplit(mergeID model.ID, splitMergeMap map[model.ID]model.ID) bool { + for _, v := range splitMergeMap { + if v == mergeID { + return true + } + } + return false +} + // findBranchFlows separates flows from a split into TRUE and FALSE branches based on CaseValue. // Returns (trueFlow, falseFlow). Either may be nil if the branch doesn't exist. func findBranchFlows(flows []*microflows.SequenceFlow) (trueFlow, falseFlow *microflows.SequenceFlow) { diff --git a/mdl/executor/cmd_microflows_unpaired_merge_test.go b/mdl/executor/cmd_microflows_unpaired_merge_test.go new file mode 100644 index 00000000..58d5f973 --- /dev/null +++ b/mdl/executor/cmd_microflows_unpaired_merge_test.go @@ -0,0 +1,215 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Regression tests for issue #281: DESCRIBE MICROFLOW silently truncated the +// output at an ExclusiveMerge used as a pure junction (e.g., the loop-back +// target of a manual retry-loop pattern, visible in the screenshot attached +// to the issue). The describer used to early-return on every merge, but +// merges that aren't the matching end point of an IF/ELSE block must be +// traversed as pass-through or their successors are dropped. +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func TestTraverseFlow_UnpairedMergeIsPassThrough(t *testing.T) { + // Flow graph: start → act1 → merge → act2 → end. + // The merge has no matching split (splitMergeMap is empty), so it's a + // pure junction, not the end of an IF/ELSE. + e := newTestExecutor() + activityMap := map[model.ID]microflows.MicroflowObject{ + mkID("start"): µflows.StartEvent{BaseMicroflowObject: mkObj("start")}, + mkID("act1"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("act1")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "before merge"}}, + }, + }, + mkID("merge"): µflows.ExclusiveMerge{BaseMicroflowObject: mkObj("merge")}, + mkID("act2"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("act2")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "after merge"}}, + }, + }, + mkID("end"): µflows.EndEvent{BaseMicroflowObject: mkObj("end")}, + } + + flowsByOrigin := map[model.ID][]*microflows.SequenceFlow{ + mkID("start"): {mkFlow("start", "act1")}, + mkID("act1"): {mkFlow("act1", "merge")}, + mkID("merge"): {mkFlow("merge", "act2")}, + mkID("act2"): {mkFlow("act2", "end")}, + } + + var lines []string + visited := make(map[model.ID]bool) + // Empty splitMergeMap — the merge isn't paired with any split. + e.traverseFlow(mkID("start"), activityMap, flowsByOrigin, nil, visited, nil, nil, &lines, 0, nil, 0, nil) + + out := strings.Join(lines, "\n") + if !strings.Contains(out, "before merge") { + t.Errorf("missing `before merge` activity (should always be emitted):\n%s", out) + } + if !strings.Contains(out, "after merge") { + t.Errorf("issue #281: `after merge` activity was dropped — describer stopped at unpaired merge:\n%s", out) + } + // The merge itself must not produce an `end if;` here — it's a + // junction, not the closing bracket of an IF/ELSE. + if strings.Contains(out, "end if;") { + t.Errorf("unpaired merge must not emit `end if;`:\n%s", out) + } +} + +func TestTraverseFlow_PairedMergeStillClosesIfElse(t *testing.T) { + // Regression guard for the partnered case: a merge that IS paired with + // a split must still be handled by the split's branch logic (not by the + // new pass-through path). Builds a standard IF/ELSE: + // + // start → split(true→thenAct, false→elseAct) → merge → end + e := newTestExecutor() + activityMap := map[model.ID]microflows.MicroflowObject{ + mkID("start"): µflows.StartEvent{BaseMicroflowObject: mkObj("start")}, + mkID("split"): µflows.ExclusiveSplit{ + BaseMicroflowObject: mkObj("split"), + SplitCondition: µflows.ExpressionSplitCondition{Expression: "$x > 0"}, + }, + mkID("thenAct"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("thenAct")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "yes"}}, + }, + }, + mkID("elseAct"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("elseAct")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "no"}}, + }, + }, + mkID("merge"): µflows.ExclusiveMerge{BaseMicroflowObject: mkObj("merge")}, + mkID("end"): µflows.EndEvent{BaseMicroflowObject: mkObj("end")}, + } + + flowsByOrigin := map[model.ID][]*microflows.SequenceFlow{ + mkID("start"): {mkFlow("start", "split")}, + mkID("split"): {mkBranchFlow("split", "thenAct", microflows.EnumerationCase{Value: "true"}), mkBranchFlow("split", "elseAct", microflows.EnumerationCase{Value: "false"})}, + mkID("thenAct"): {mkFlow("thenAct", "merge")}, + mkID("elseAct"): {mkFlow("elseAct", "merge")}, + mkID("merge"): {mkFlow("merge", "end")}, + } + + splitMergeMap := map[model.ID]model.ID{ + mkID("split"): mkID("merge"), + } + + var lines []string + visited := make(map[model.ID]bool) + e.traverseFlow(mkID("start"), activityMap, flowsByOrigin, splitMergeMap, visited, nil, nil, &lines, 0, nil, 0, nil) + + out := strings.Join(lines, "\n") + if !strings.Contains(out, "if $x > 0 then") { + t.Errorf("missing IF header:\n%s", out) + } + if !strings.Contains(out, "else") { + t.Errorf("missing else branch:\n%s", out) + } + if !strings.Contains(out, "end if;") { + t.Errorf("missing `end if;` — paired merge must still close the IF:\n%s", out) + } + // The merge should produce exactly one `end if;`, not two. + if count := strings.Count(out, "end if;"); count != 1 { + t.Errorf("expected exactly one `end if;`, got %d:\n%s", count, out) + } +} + +func TestTraverseFlow_ManualRetryLoopPatternEmitsEverything(t *testing.T) { + // Closer reproduction of the user's microflow from issue #281. The retry + // pattern wires a back-edge from a "change retry count + delay" branch + // into a merge that sits between the setup activities and the REST call, + // so control re-enters the REST call every retry iteration. + // + // start → setup → merge → call → decide + // ↑ ↓ (retry) + // delay ← change ←──┘ + // + // Before the fix the describer emitted setup and stopped at the merge, + // dropping call, decide, change and delay — exactly what the issue + // reported. + e := newTestExecutor() + activityMap := map[model.ID]microflows.MicroflowObject{ + mkID("start"): µflows.StartEvent{BaseMicroflowObject: mkObj("start")}, + mkID("setup"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("setup")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "setup"}}, + }, + }, + mkID("merge"): µflows.ExclusiveMerge{BaseMicroflowObject: mkObj("merge")}, + mkID("call"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("call")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "call-rest"}}, + }, + }, + mkID("decide"): µflows.ExclusiveSplit{ + BaseMicroflowObject: mkObj("decide"), + SplitCondition: µflows.ExpressionSplitCondition{Expression: "$retry"}, + }, + mkID("change"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("change")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "change-retry-count"}}, + }, + }, + mkID("delay"): µflows.ActionActivity{ + BaseActivity: microflows.BaseActivity{BaseMicroflowObject: mkObj("delay")}, + Action: µflows.LogMessageAction{ + LogLevel: "Info", + LogNodeName: "'App'", + MessageTemplate: &model.Text{Translations: map[string]string{"en_US": "delay"}}, + }, + }, + mkID("end"): µflows.EndEvent{BaseMicroflowObject: mkObj("end")}, + } + + flowsByOrigin := map[model.ID][]*microflows.SequenceFlow{ + mkID("start"): {mkFlow("start", "setup")}, + mkID("setup"): {mkFlow("setup", "merge")}, + mkID("merge"): {mkFlow("merge", "call")}, + mkID("call"): {mkFlow("call", "decide")}, + mkID("decide"): {mkBranchFlow("decide", "change", microflows.EnumerationCase{Value: "true"}), mkBranchFlow("decide", "end", microflows.EnumerationCase{Value: "false"})}, + mkID("change"): {mkFlow("change", "delay")}, + mkID("delay"): {mkFlow("delay", "merge")}, + } + + var lines []string + visited := make(map[model.ID]bool) + // The `decide` split has no merge target (both branches leave the loop), + // so splitMergeMap is empty. + e.traverseFlow(mkID("start"), activityMap, flowsByOrigin, nil, visited, nil, nil, &lines, 0, nil, 0, nil) + + out := strings.Join(lines, "\n") + for _, want := range []string{"setup", "call-rest", "if $retry then", "change-retry-count", "delay"} { + if !strings.Contains(out, want) { + t.Errorf("issue #281: missing %q from describe output:\n%s", want, out) + } + } +}