From 3e8d0a60cb17522760a3f7865a085991da5c2d74 Mon Sep 17 00:00:00 2001 From: Henrique Costa Date: Thu, 30 Apr 2026 09:40:43 +0200 Subject: [PATCH] fix: address merged microflow review followups 1. Manual while-true detection over-triggered when the only continue lived inside a nested loop. The continue scan crossed loop boundaries while the break scan did not, so the outer while could be rebuilt as a manual back-edge. Stop the continue scan at nested loops and add a regression test. 2. Owner-both reverse retrieves feeding add/remove-to-list were still treated as object-only consumers. The list pre-scan ignored AddToListStmt and RemoveFromListStmt target lists, so AssociationRetrieveSource could be suppressed. Track those list consumers and add coverage for the helper. 3. Direct nanoflow describe did not set the return-value render context used by EndEvent formatting. Thread the wrapped nanoflow return type through the formatter so value-returning nanoflows do not emit bare return; for empty EndEvents. Also folds in low-risk review followups: commit-action writer coverage, change-object refresh negative coverage, download-file formatter coverage, reverse-retrieve name validation tightening, and documentation for MXCLI_EXEC_TIMEOUT. Tests: make build Tests: make test Tests: make lint-go Closes #404. Closes #405. Closes #406. --- docs/01-project/MDL_QUICK_REFERENCE.md | 2 + mdl/executor/bugfix_regression_test.go | 22 ++++++ mdl/executor/cmd_microflows_builder.go | 2 +- .../cmd_microflows_builder_actions.go | 3 +- ...croflows_builder_collectlistinputs_test.go | 31 ++++++++ .../cmd_microflows_builder_control.go | 18 ++--- mdl/executor/cmd_microflows_builder_graph.go | 10 +++ ...oflows_builder_manual_while_nested_test.go | 74 +++++++++++++++++++ mdl/executor/cmd_microflows_format_action.go | 51 +++++++------ .../cmd_microflows_format_action_test.go | 16 ++++ mdl/executor/cmd_microflows_show.go | 13 +++- .../cmd_microflows_show_helpers_test.go | 2 +- mdl/executor/cmd_nanoflows_mock_test.go | 31 ++++++++ sdk/mpr/writer_microflow_action_items_test.go | 13 ++++ sdk/mpr/writer_microflow_version_test.go | 6 +- 15 files changed, 252 insertions(+), 42 deletions(-) create mode 100644 mdl/executor/cmd_microflows_builder_collectlistinputs_test.go create mode 100644 mdl/executor/cmd_microflows_builder_manual_while_nested_test.go diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 2197cad2..ab015310 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -1047,6 +1047,8 @@ Cross-reference commands require `refresh catalog full` to populate reference da | Setup mxcli | `mxcli setup mxcli [--os linux]` | Download platform-specific mxcli binary | | LSP server | `mxcli lsp --stdio` | Language server for VS Code | +Set `MXCLI_EXEC_TIMEOUT` to override the per-statement execution timeout used by `mxcli exec` (for example `MXCLI_EXEC_TIMEOUT=12m` or `MXCLI_EXEC_TIMEOUT=900`). + ## ANTLR4 Parser Architecture The MDL parser uses ANTLR4 for grammar definition, enabling cross-language grammar sharing (Go, TypeScript, Java, Python). diff --git a/mdl/executor/bugfix_regression_test.go b/mdl/executor/bugfix_regression_test.go index 44356e92..cfee2304 100644 --- a/mdl/executor/bugfix_regression_test.go +++ b/mdl/executor/bugfix_regression_test.go @@ -694,6 +694,28 @@ func TestEmptyChangeObjectRefreshesInClient(t *testing.T) { if !action.RefreshInClient { t.Fatal("empty change object must refresh in client to remain valid without member changes or commit") } + + id = fb.addChangeObjectAction(&ast.ChangeObjectStmt{ + Variable: "Object", + Changes: []ast.ChangeItem{{ + Attribute: "Name", + Value: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "changed"}, + }}, + }) + if id == "" || len(fb.objects) != 2 { + t.Fatalf("expected second change object activity, got id=%q objects=%d", id, len(fb.objects)) + } + activity, ok = fb.objects[1].(*microflows.ActionActivity) + if !ok { + t.Fatalf("object type = %T, want *microflows.ActionActivity", fb.objects[1]) + } + action, ok = activity.Action.(*microflows.ChangeObjectAction) + if !ok { + t.Fatalf("action type = %T, want *microflows.ChangeObjectAction", activity.Action) + } + if action.RefreshInClient { + t.Fatal("non-empty change object must not infer refresh in client") + } } func TestCallMicroflowUnknownResultTypeStillDeclaresVariable(t *testing.T) { diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 99b5d724..f46c971b 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -53,7 +53,7 @@ type flowBuilder struct { microflowsCacheLoaded bool nanoflowsCache []*microflows.Nanoflow nanoflowsCacheLoaded bool - manualLoopBackTarget model.ID + manualLoopBackTarget model.ID } // addError records a validation error during flow building. diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index d3129851..2aab1f3c 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -308,13 +308,14 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { outputUsedAsObject := fb.objectInputVariables != nil && fb.objectInputVariables[s.Variable] // Owner-both Reference associations need later usage context: the same // compact retrieve can be consumed as either a list or a single object. + // Owner="" means metadata was unavailable, so keep the association source. expandReverseReference := assocInfo != nil && assocInfo.Type == domainmodel.AssociationTypeReference && assocInfo.Owner != "" && assocInfo.parentPersistable && assocInfo.childEntityQN != "" && startVarType == assocInfo.childEntityQN && - (assocInfo.Owner != domainmodel.AssociationOwnerBoth || outputUsedAsList && !outputUsedAsObject) + (assocInfo.Owner != domainmodel.AssociationOwnerBoth || (outputUsedAsList && !outputUsedAsObject)) if expandReverseReference { // Reverse traversal on Reference: child → parent (one-to-many) diff --git a/mdl/executor/cmd_microflows_builder_collectlistinputs_test.go b/mdl/executor/cmd_microflows_builder_collectlistinputs_test.go new file mode 100644 index 00000000..ade44df7 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_collectlistinputs_test.go @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// TestCollectListInputVariables_AddRemoveFromList pins issue #405: +// `add $X to $List` and `remove $Y from $List` consume the target list, so +// the list variable must be tracked as a list input. Without it, the output +// of an Owner=Both reverse retrieve fed straight into add/remove was +// misclassified as object-only and the AssociationRetrieveSource was +// suppressed, re-introducing the original #383 bug for this usage shape. +func TestCollectListInputVariables_AddRemoveFromList(t *testing.T) { + stmts := []ast.MicroflowStatement{ + &ast.AddToListStmt{Item: "NewItem", List: "Items"}, + &ast.RemoveFromListStmt{Item: "OldItem", List: "Backlog"}, + } + + got := collectListInputVariables(stmts) + + if !got["Items"] { + t.Errorf("AddToListStmt target `Items` must be marked as list input; got %v", got) + } + if !got["Backlog"] { + t.Errorf("RemoveFromListStmt target `Backlog` must be marked as list input; got %v", got) + } +} diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index e47ca4f7..dce3e663 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -441,7 +441,7 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { } func isManualWhileTrueCandidate(s *ast.WhileStmt) bool { - if s == nil || containsBreakForCurrentLoop(s.Body) || (!containsContinueStmt(s.Body) && !containsTerminalStmt(s.Body)) { + if s == nil || containsBreakForCurrentLoop(s.Body) || (!containsContinueForCurrentLoop(s.Body) && !containsTerminalStmt(s.Body)) { return false } lit, ok := s.Condition.(*ast.LiteralExpr) @@ -470,23 +470,19 @@ func containsBreakForCurrentLoop(stmts []ast.MicroflowStatement) bool { return false } -func containsContinueStmt(stmts []ast.MicroflowStatement) bool { +// containsContinueForCurrentLoop mirrors containsBreakForCurrentLoop: +// a continue inside a nested loop targets that nested loop, not this one. +func containsContinueForCurrentLoop(stmts []ast.MicroflowStatement) bool { for _, stmt := range stmts { switch s := stmt.(type) { case *ast.ContinueStmt: return true case *ast.IfStmt: - if containsContinueStmt(s.ThenBody) || containsContinueStmt(s.ElseBody) { - return true - } - case *ast.LoopStmt: - if containsContinueStmt(s.Body) { - return true - } - case *ast.WhileStmt: - if containsContinueStmt(s.Body) { + if containsContinueForCurrentLoop(s.ThenBody) || containsContinueForCurrentLoop(s.ElseBody) { return true } + case *ast.LoopStmt, *ast.WhileStmt: + continue } } return false diff --git a/mdl/executor/cmd_microflows_builder_graph.go b/mdl/executor/cmd_microflows_builder_graph.go index f2c421c7..7caef5f2 100644 --- a/mdl/executor/cmd_microflows_builder_graph.go +++ b/mdl/executor/cmd_microflows_builder_graph.go @@ -116,6 +116,8 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a // Handle leftover pending annotations (free-floating annotation text) if fb.pendingAnnotations != nil { + // Free annotations before a statement stay unattached; trailing free + // annotations are drained after the statement loop below. for _, text := range freeAnnotationTexts(fb.pendingAnnotations) { fb.attachFreeAnnotation(text) } @@ -185,6 +187,14 @@ func collectListInputVariables(stmts []ast.MicroflowStatement) map[string]bool { inputs[s.ListVariable] = true } walk(s.Body) + case *ast.AddToListStmt: + if s.List != "" { + inputs[s.List] = true + } + case *ast.RemoveFromListStmt: + if s.List != "" { + inputs[s.List] = true + } case *ast.WhileStmt: walk(s.Body) case *ast.IfStmt: diff --git a/mdl/executor/cmd_microflows_builder_manual_while_nested_test.go b/mdl/executor/cmd_microflows_builder_manual_while_nested_test.go new file mode 100644 index 00000000..111d3162 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_manual_while_nested_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// TestBuildFlowGraph_ManualWhileTrueIgnoresNestedLoopContinue pins issue #404: +// a `while true` whose only `continue` lives inside a nested collection loop +// must NOT be classified as a manual back-edge candidate. The outer flow +// should be built as a regular LoopedActivity (with a WhileLoopCondition). +// +// Before the fix, containsContinueStmt recursed into nested LoopStmt bodies +// asymmetrically with containsBreakForCurrentLoop, so isManualWhileTrueCandidate +// returned true and the outer while was rebuilt as an ExclusiveMerge back-edge, +// creating an unconditional infinite loop in the BSON graph. +func TestBuildFlowGraph_ManualWhileTrueIgnoresNestedLoopContinue(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.WhileStmt{ + Condition: &ast.LiteralExpr{Kind: ast.LiteralBoolean, Value: true}, + Body: []ast.MicroflowStatement{ + &ast.LoopStmt{ + LoopVariable: "item", + ListVariable: "items", + Body: []ast.MicroflowStatement{ + &ast.ContinueStmt{}, + }, + }, + // No outer-scope continue / return / raise: the outer while + // has no terminal signal of its own. + }, + }, + } + + fb := &flowBuilder{ + posX: 100, + posY: 100, + spacing: HorizontalSpacing, + measurer: &layoutMeasurer{}, + varTypes: map[string]string{"items": "List of Sample.Item"}, + declaredVars: map[string]string{"items": "List of Sample.Item"}, + } + oc := fb.buildFlowGraph(body, nil) + + var ( + outerLoop *microflows.LoopedActivity + mergeCount int + ) + for _, obj := range oc.Objects { + switch o := obj.(type) { + case *microflows.LoopedActivity: + // The first looped activity at this scope is the outer while. + if outerLoop == nil { + outerLoop = o + } + case *microflows.ExclusiveMerge: + mergeCount++ + } + } + + if outerLoop == nil { + t.Fatal("outer `while true` must be built as a LoopedActivity, not an ExclusiveMerge back-edge") + } + if outerLoop.LoopSource == nil { + t.Fatal("outer LoopedActivity must have a LoopSource (WhileLoopCondition for `while true`)") + } + if mergeCount != 0 { + t.Errorf("manual back-edge ExclusiveMerge must not be emitted; got %d ExclusiveMerge node(s)", mergeCount) + } +} diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index dd9dc270..5047d05b 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -79,6 +79,7 @@ func formatActivity( if ctx != nil && ctx.DescribingMicroflowHasReturnValue { return "" } + // Without render context, default to the void-flow form. return "return;" case *microflows.ActionActivity: @@ -351,30 +352,30 @@ func formatAction( stmt := fmt.Sprintf("retrieve $%s from %s", outputVar, entityName) - if dbSource.XPathConstraint != "" { - constraint := strings.TrimSpace(dbSource.XPathConstraint) - // XPath may contain multiple predicates like [a][b] or [a]\n[b]. - // Split them and join with MDL 'and' so the parser sees - // separate xpathConstraint nodes. - if strings.HasPrefix(constraint, "[") && strings.HasSuffix(constraint, "]") { - // Split on "][" boundary (possibly separated by \n literals), - // then re-wrap each predicate. - inner := constraint[1 : len(constraint)-1] - // Normalise real newlines between predicates: ]\n[ → ][ - inner = strings.ReplaceAll(inner, "]\n[", "][") - parts := strings.Split(inner, "][") - if len(parts) > 1 { - var wrapped []string - for _, p := range parts { - wrapped = append(wrapped, "["+strings.TrimSpace(p)+"]") + if dbSource.XPathConstraint != "" { + constraint := strings.TrimSpace(dbSource.XPathConstraint) + // XPath may contain multiple predicates like [a][b] or [a]\n[b]. + // Split them and join with MDL 'and' so the parser sees + // separate xpathConstraint nodes. + if strings.HasPrefix(constraint, "[") && strings.HasSuffix(constraint, "]") { + // Split on "][" boundary (possibly separated by \n literals), + // then re-wrap each predicate. + inner := constraint[1 : len(constraint)-1] + // Normalise real newlines between predicates: ]\n[ to ][ + inner = strings.ReplaceAll(inner, "]\n[", "][") + parts := strings.Split(inner, "][") + if len(parts) > 1 { + var wrapped []string + for _, p := range parts { + wrapped = append(wrapped, "["+strings.TrimSpace(p)+"]") + } + constraint = strings.Join(wrapped, "\n ") + } else { + constraint = parts[0] } - constraint = strings.Join(wrapped, "\n ") - } else { - constraint = parts[0] } + stmt += fmt.Sprintf("\n where %s", constraint) } - stmt += fmt.Sprintf("\n where %s", constraint) - } // Output SORT BY clause if present if len(dbSource.Sorting) > 0 { @@ -1378,7 +1379,13 @@ func isSimpleMendixName(name string) bool { return false } for i, r := range name { - if r == '_' || r >= 'A' && r <= 'Z' || r >= 'a' && r <= 'z' || i > 0 && r >= '0' && r <= '9' { + if i == 0 { + if r >= 'A' && r <= 'Z' || r >= 'a' && r <= 'z' { + continue + } + return false + } + if r == '_' || r >= 'A' && r <= 'Z' || r >= 'a' && r <= 'z' || r >= '0' && r <= '9' { continue } return false diff --git a/mdl/executor/cmd_microflows_format_action_test.go b/mdl/executor/cmd_microflows_format_action_test.go index 20660a5b..814f324f 100644 --- a/mdl/executor/cmd_microflows_format_action_test.go +++ b/mdl/executor/cmd_microflows_format_action_test.go @@ -507,6 +507,19 @@ func TestFormatAction_DownloadFile(t *testing.T) { } } +func TestFormatAction_DownloadFileWithoutBrowserFlag(t *testing.T) { + e := newTestExecutor() + action := µflows.DownloadFileAction{ + FileDocument: "GeneratedReport", + } + + got := e.formatAction(action, nil, nil) + want := "download file $GeneratedReport;" + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} + func TestFormatAction_ValidationFeedback(t *testing.T) { e := newTestExecutor() action := µflows.ValidationFeedbackAction{ @@ -752,6 +765,9 @@ func TestParseReverseAssociationXPathRejectsComplexPredicates(t *testing.T) { "[SampleRuntime.Domain_Runtime != $Runtime]", "[SampleRuntime.Domain_Runtime = $Runtime/Other.Assoc]", "[SampleRuntime.Domain_Runtime = 'literal']", + "[_SampleRuntime.Domain_Runtime = $Runtime]", + "[SampleRuntime._Domain_Runtime = $Runtime]", + "[SampleRuntime.Domain_Runtime = $_Runtime]", "SampleRuntime.Domain_Runtime = $Runtime", } diff --git a/mdl/executor/cmd_microflows_show.go b/mdl/executor/cmd_microflows_show.go index 55e26373..2edab189 100644 --- a/mdl/executor/cmd_microflows_show.go +++ b/mdl/executor/cmd_microflows_show.go @@ -427,10 +427,17 @@ func describeNanoflow(ctx *ExecContext, name ast.QualifiedName) error { lines = append(lines, "begin") // Wrap nanoflow in a Microflow to reuse formatMicroflowActivities + wrapperMf := µflows.Microflow{ + ReturnType: targetNf.ReturnType, + ObjectCollection: targetNf.ObjectCollection, + } + prevDescribingReturnValue := ctx.DescribingMicroflowHasReturnValue + ctx.DescribingMicroflowHasReturnValue = microflowHasReturnValue(wrapperMf) + defer func() { + ctx.DescribingMicroflowHasReturnValue = prevDescribingReturnValue + }() + if targetNf.ObjectCollection != nil && len(targetNf.ObjectCollection.Objects) > 0 { - wrapperMf := µflows.Microflow{ - ObjectCollection: targetNf.ObjectCollection, - } activityLines := formatMicroflowActivities(ctx, wrapperMf, entityNames, microflowNames) for _, line := range activityLines { lines = append(lines, " "+line) diff --git a/mdl/executor/cmd_microflows_show_helpers_test.go b/mdl/executor/cmd_microflows_show_helpers_test.go index 48731f06..d00636ed 100644 --- a/mdl/executor/cmd_microflows_show_helpers_test.go +++ b/mdl/executor/cmd_microflows_show_helpers_test.go @@ -362,7 +362,7 @@ func TestFormatActivity_StartEvent(t *testing.T) { } } -func TestFormatActivity_EndEvent_NoReturn(t *testing.T) { +func TestFormatActivity_EndEvent_VoidOrUnknownContext(t *testing.T) { e := newTestExecutor() obj := µflows.EndEvent{BaseMicroflowObject: mkObj("1")} got := e.formatActivity(obj, nil, nil) diff --git a/mdl/executor/cmd_nanoflows_mock_test.go b/mdl/executor/cmd_nanoflows_mock_test.go index 307d79b9..f08beb7b 100644 --- a/mdl/executor/cmd_nanoflows_mock_test.go +++ b/mdl/executor/cmd_nanoflows_mock_test.go @@ -114,6 +114,37 @@ func TestDescribeNanoflow_Mock_WithReturnType(t *testing.T) { assertContainsStr(t, out, "nanoflow MyModule.NF_GetName") } +func TestDescribeNanoflow_ReturningFlowSkipsEmptyEndEvent(t *testing.T) { + mod := mkModule("MyModule") + nf := mkNanoflow(mod.ID, "NF_Value") + nf.ReturnType = µflows.StringType{} + nf.ObjectCollection = µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{ + µflows.StartEvent{BaseMicroflowObject: mkObj("start")}, + µflows.EndEvent{BaseMicroflowObject: mkObj("end")}, + }, + Flows: []*microflows.SequenceFlow{mkFlow("start", "end")}, + } + + h := mkHierarchy(mod) + withContainer(h, nf.ContainerID, mod.ID) + + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { return nil, nil }, + ListNanoflowsFunc: func() ([]*microflows.Nanoflow, error) { return []*microflows.Nanoflow{nf}, nil }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { return nil, nil }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + } + + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) + assertNoError(t, describeNanoflow(ctx, ast.QualifiedName{Module: "MyModule", Name: "NF_Value"})) + + out := buf.String() + assertContainsStr(t, out, "returns String") + assertNotContainsStr(t, out, "return;") +} + // --- DROP NANOFLOW --- func TestDropNanoflow_Mock(t *testing.T) { diff --git a/sdk/mpr/writer_microflow_action_items_test.go b/sdk/mpr/writer_microflow_action_items_test.go index a0f5c277..8fce7db6 100644 --- a/sdk/mpr/writer_microflow_action_items_test.go +++ b/sdk/mpr/writer_microflow_action_items_test.go @@ -71,3 +71,16 @@ func TestSerializeChangeObjectActionItemsUseStorageListMarkerAndDefaultErrorHand t.Fatalf("Items marker = %#v, want int32(2)", items[0]) } } + +func TestSerializeCommitActionAlwaysWritesDefaultErrorHandling(t *testing.T) { + action := µflows.CommitObjectsAction{ + BaseElement: model.BaseElement{ID: "commit-1"}, + CommitVariable: "Order", + } + + doc := serializeMicroflowAction(action) + + if got := getBSONField(doc, "ErrorHandlingType"); got != "Rollback" { + t.Fatalf("ErrorHandlingType = %#v, want Rollback", got) + } +} diff --git a/sdk/mpr/writer_microflow_version_test.go b/sdk/mpr/writer_microflow_version_test.go index fd3c298a..356927fc 100644 --- a/sdk/mpr/writer_microflow_version_test.go +++ b/sdk/mpr/writer_microflow_version_test.go @@ -86,12 +86,12 @@ func TestSerializeEndEvent_EmptyReturnValueHasNoTrailingLineBreak(t *testing.T) Position: model.Point{X: 10, Y: 20}, Size: model.Size{Width: 20, Height: 20}, }, - ReturnValue: "empty", + ReturnValue: "", } doc := serializeMicroflowObject(end) - if got := bsonGetKey(doc, "ReturnValue"); got != "empty" { - t.Fatalf("ReturnValue = %q, want %q", got, "empty") + if got := bsonGetKey(doc, "ReturnValue"); got != "" { + t.Fatalf("ReturnValue = %q, want empty string", got) } }