From 1b88baf809086321bb1a2a769b493c99371612f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:19:08 +0000 Subject: [PATCH 1/4] fix: keep a @curve written inside a loop or while body (mendixlabs/mxcli#1597) A `@curve` on a statement inside a `loop` or `while` body passed check and exec and was not stored; `describe` printed the body without it, while the same annotation after the loop was kept. The body is built by its own flowBuilder, which recorded the curve, and the loop hoisted that builder's flows to the parent without the curves that belong on them. applyFlowCurves runs once, on the builder that owns the flows, so the body's curves were never applied. The error-handler builder had the same merge (#991); it is now one helper, adoptCurves, which the loop, while and error-handler merges all call. Refs ako/mxcli#1507 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HjYj5GTj2in3ckFYaDBAuN --- ...-21-curve-inside-loop-body-not-stored.json | 1 + .../bug-tests/1597-curve-inside-loop-body.mdl | 41 ++++++ .../cmd_microflows_builder_annotations.go | 15 +++ .../cmd_microflows_builder_control.go | 5 + mdl/executor/cmd_microflows_builder_flows.go | 7 +- .../cmd_microflows_loop_curve_test.go | 119 ++++++++++++++++++ 6 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-21-curve-inside-loop-body-not-stored.json create mode 100644 mdl-examples/bug-tests/1597-curve-inside-loop-body.mdl create mode 100644 mdl/executor/cmd_microflows_loop_curve_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-curve-inside-loop-body-not-stored.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-curve-inside-loop-body-not-stored.json new file mode 100644 index 0000000000..db22df0650 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-curve-inside-loop-body-not-stored.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-21", "symptom": "mendixlabs/mxcli#1597 (case A): `@curve(from: (39, 0), to: (0, 61))` on a statement inside a `loop` or `while` body passed check and exec with no warning, and `describe` printed the body with no `@curve` — the same annotation after the loop was kept", "cause": "a loop body is built by its own flowBuilder; applyPendingAnnotations recorded the curve on that sub-builder's curveByOrigin, and addLoopStatement / addWhileStatement hoisted flows, annotationFlows, errors and the label registry to the parent but not the curves. applyFlowCurves runs once, on the builder that owns the flows, so the body's curves were never applied", "file": "mdl/executor/cmd_microflows_builder_annotations.go (adoptCurves); cmd_microflows_builder_control.go (addLoopStatement, addWhileStatement); cmd_microflows_builder_flows.go (addErrorHandlerFlow now calls the same helper)", "test": "TestLoopBody_KeepsCurve, TestWhileBody_KeepsCurve, TestNestedLoopBody_KeepsCurve (cmd_microflows_loop_curve_test.go)", "insight": "#991 fixed exactly this for the error-handler builder a month earlier, at one of the three sites that hoist a sub-builder's flows — each site copies a hand-picked list of fields, so a fix at one is not a fix at the others. When a child builder's state is lost, grep `&flowBuilder{` for every sub-builder and diff the field lists their merge tails copy, rather than following the reported statement. The merge is now one helper (adoptCurves) the three sites share, and a nested loop test proves it chains through two levels. The main-line curve after the loop is the in-test control: it passed before the fix, so the test isolates the body", "refs": ["mendixlabs/mxcli#1597", "mendixlabs/mxcli#991", "ako/mxcli#1507"]} diff --git a/mdl-examples/bug-tests/1597-curve-inside-loop-body.mdl b/mdl-examples/bug-tests/1597-curve-inside-loop-body.mdl new file mode 100644 index 0000000000..0a4c60e101 --- /dev/null +++ b/mdl-examples/bug-tests/1597-curve-inside-loop-body.mdl @@ -0,0 +1,41 @@ +-- mendixlabs/mxcli#1597 (case A): a `@curve` on a statement inside a `loop` or +-- `while` body passed check and exec and was not stored — `describe` printed the +-- body with no `@curve`, while the same annotation after the loop was kept. +-- After the fix the body's curve is written on the flow leaving that statement, +-- and `describe` prints it inside the loop as it does outside. +mdl 1; + +create module BugTest1597; + +create persistent entity BugTest1597.Item (Code: String(20)); + +create microflow BugTest1597.ACT_Loop () +begin + retrieve $L from BugTest1597.Item; + loop $I in $L + begin + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'inside-a'; + log info node 'n' 'inside-b'; + end loop; + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'outside-c'; + log info node 'n' 'outside-d'; +end; + +create microflow BugTest1597.ACT_While () +begin + declare $N integer = 0; + while $N < 3 + begin + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'inside-a'; + log info node 'n' 'inside-b'; + set $N = $N + 1; + end while; +end; + +-- Expect: `@curve(from: (39, 0), to: (0, 61))` above `log … 'inside-a'` in both, +-- and above `log … 'outside-c'` in ACT_Loop. +describe microflow BugTest1597.ACT_Loop; +describe microflow BugTest1597.ACT_While; diff --git a/mdl/executor/cmd_microflows_builder_annotations.go b/mdl/executor/cmd_microflows_builder_annotations.go index 045c81d09c..8402bacc8b 100644 --- a/mdl/executor/cmd_microflows_builder_annotations.go +++ b/mdl/executor/cmd_microflows_builder_annotations.go @@ -182,6 +182,21 @@ func mergePosition(ann *ast.ActivityAnnotations, computedX, computedY int) (int, return ann.Merge.X, ann.Merge.Y } +// adoptCurves takes over the @curves a sub-builder recorded, for a caller that +// is taking over its flows. applyFlowCurves runs once, on the builder that owns +// the flows when the graph is complete, so a curve left behind on a loop body's +// or an error handler's builder was never applied: check and exec passed and +// the edge came out straight (mendixlabs/mxcli#991, mendixlabs/mxcli#1597). +// Every site that appends a sub-builder's flows calls this alongside. +func (fb *flowBuilder) adoptCurves(sub *flowBuilder) { + for id, c := range sub.curveByOrigin { + if fb.curveByOrigin == nil { + fb.curveByOrigin = map[model.ID]*ast.FlowCurve{} + } + fb.curveByOrigin[id] = c + } +} + // applyFlowCurves stamps each recorded @curve onto the flows leaving that // activity. Runs once, after the graph is built. // diff --git a/mdl/executor/cmd_microflows_builder_control.go b/mdl/executor/cmd_microflows_builder_control.go index 616b932216..a032231ff0 100644 --- a/mdl/executor/cmd_microflows_builder_control.go +++ b/mdl/executor/cmd_microflows_builder_control.go @@ -760,6 +760,10 @@ func (fb *flowBuilder) addLoopStatement(s *ast.LoopStmt) model.ID { // Add the internal flows to the parent's flows (top-level), not inside loop // This is how Mendix stores them - all flows at the microflow level fb.flows = append(fb.flows, loopBuilder.flows...) + // The body's @curves belong on those flows, and are applied by whichever + // builder ends up owning them; left on loopBuilder they were never applied + // (mendixlabs/mxcli#1597). + fb.adoptCurves(loopBuilder) fb.annotationFlows = append(fb.annotationFlows, loopBuilder.annotationFlows...) // Refusals raised in the body were collected into the sub-builder and // dropped on the floor, so exec reported success on a flow it had silently @@ -1111,6 +1115,7 @@ func (fb *flowBuilder) addWhileStatement(s *ast.WhileStmt) model.ID { fb.objects = append(fb.objects, loop) fb.flows = append(fb.flows, loopBuilder.flows...) + fb.adoptCurves(loopBuilder) // see addLoopStatement (mendixlabs/mxcli#1597) fb.annotationFlows = append(fb.annotationFlows, loopBuilder.annotationFlows...) // Refusals raised in the body were collected into the sub-builder and // dropped on the floor, so exec reported success on a flow it had silently diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index 9cd46edd97..3f159724ee 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -845,12 +845,7 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in // builder that owns the flows once its graph is complete — this one, since // the handler's flows (and the edge that rejoins the main flow) land here. // Left on errBuilder it was never applied (mendixlabs/mxcli#991). - for id, c := range errBuilder.curveByOrigin { - if fb.curveByOrigin == nil { - fb.curveByOrigin = map[model.ID]*ast.FlowCurve{} - } - fb.curveByOrigin[id] = c - } + fb.adoptCurves(errBuilder) if fb.annotationsByLabel == nil { fb.annotationsByLabel = errBuilder.annotationsByLabel } diff --git a/mdl/executor/cmd_microflows_loop_curve_test.go b/mdl/executor/cmd_microflows_loop_curve_test.go new file mode 100644 index 0000000000..ce7dc0303d --- /dev/null +++ b/mdl/executor/cmd_microflows_loop_curve_test.go @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// mendixlabs/mxcli#1597. A `@curve` on a statement inside a `loop` or `while` +// body passed check and exec and was not stored: the body is built by its own +// flowBuilder, which recorded the curve, and the loop handed its flows to the +// parent without the curves that belong on them. applyFlowCurves runs once, on +// the builder that owns the flows, so a curve left on the sub-builder was never +// applied. The same statement after the loop kept its curve. + +// loopBodyIDs returns the IDs of every object inside a loop's collection, at +// any depth, so a test can tell a body flow from a main-line one. +func loopBodyIDs(oc *microflows.MicroflowObjectCollection) map[model.ID]bool { + ids := map[model.ID]bool{} + var walk func(objs []microflows.MicroflowObject, inLoop bool) + walk = func(objs []microflows.MicroflowObject, inLoop bool) { + for _, o := range objs { + if inLoop { + ids[o.GetID()] = true + } + if l, ok := o.(*microflows.LoopedActivity); ok && l.ObjectCollection != nil { + walk(l.ObjectCollection.Objects, true) + } + } + } + walk(oc.Objects, false) + return ids +} + +// curvedFlows splits the flows carrying the given control vectors by whether +// they leave an activity inside a loop body. +func curvedFlows(oc *microflows.MicroflowObjectCollection, from, to string) (inBody, outside int) { + body := loopBodyIDs(oc) + for _, f := range oc.Flows { + if f.OriginControlVector != from || f.DestinationControlVector != to { + continue + } + if body[f.OriginID] { + inBody++ + } else { + outside++ + } + } + return inBody, outside +} + +func TestLoopBody_KeepsCurve(t *testing.T) { + src := `mdl 1; +create microflow Probe.ACT_Do ($L: List of Probe.Item) +begin + loop $I in $L + begin + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'inside-a'; + log info node 'n' 'inside-b'; + end loop; + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'outside-c'; + log info node 'n' 'outside-d'; +end;` + oc := buildCollectionFromMDL(t, false, src) + inBody, outside := curvedFlows(oc, "39;0", "0;61") + if outside != 1 { + t.Fatalf("%d main-line flows carry the curve, want 1 (the control: outside-c → outside-d)", outside) + } + if inBody != 1 { + t.Errorf("%d loop-body flows carry the curve, want 1 (inside-a → inside-b) — "+ + "the @curve inside the loop was recorded on the body's builder and never applied", inBody) + } +} + +func TestWhileBody_KeepsCurve(t *testing.T) { + src := `mdl 1; +create microflow Probe.ACT_While () +begin + declare $N integer = 0; + while $N < 3 + begin + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'inside-a'; + log info node 'n' 'inside-b'; + set $N = $N + 1; + end while; +end;` + oc := buildCollectionFromMDL(t, false, src) + if inBody, _ := curvedFlows(oc, "39;0", "0;61"); inBody != 1 { + t.Errorf("%d while-body flows carry the curve, want 1 (inside-a → inside-b)", inBody) + } +} + +// A loop inside a loop is built by a sub-builder of a sub-builder, so the +// curve has to be handed up twice to reach the builder that applies it. +func TestNestedLoopBody_KeepsCurve(t *testing.T) { + src := `mdl 1; +create microflow Probe.ACT_Nested ($L: List of Probe.Item) +begin + loop $I in $L + begin + loop $J in $L + begin + @curve(from: (39, 0), to: (0, 61)) + log info node 'n' 'inner-a'; + log info node 'n' 'inner-b'; + end loop; + end loop; +end;` + oc := buildCollectionFromMDL(t, false, src) + if inBody, _ := curvedFlows(oc, "39;0", "0;61"); inBody != 1 { + t.Errorf("%d nested-loop-body flows carry the curve, want 1 (inner-a → inner-b)", inBody) + } +} From 36d1c13158f0758c8fb231755b3154da3b7fb80f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:24:18 +0000 Subject: [PATCH 2/4] fix: check the annotations in an alter microflow fragment (mendixlabs/mxcli#1597) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@annotation('Why this commit is here.')` in an `alter microflow … insert` fragment passed check and exec ("Altered microflow") and no note was written; the same line in `create or modify microflow` is refused as MDL079. The annotation rules — MDL059, MDL060, MDL079, MDL092 — ran over create bodies only. A fragment is built by the same flowBuilder, which drops an annotation the visitor could not read, so all four reached the builder unchecked in an alter. check now runs them over every insert and replace fragment, and execAlterFlow refuses the exec-enforced subset as create does, for `-c` and the REPL, which skip the check preflight. Refs ako/mxcli#1507 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HjYj5GTj2in3ckFYaDBAuN --- ...r-flow-fragment-annotations-unchecked.json | 1 + .../bug-tests/1597-alter-annotation-forms.mdl | 26 +++++ .../1597-alter-annotation-positional.fail.mdl | 30 +++++ mdl/executor/cmd_alter_flow.go | 4 + mdl/executor/validate_alter_flow.go | 56 +++++++++ .../validate_alter_flow_annotations_test.go | 106 ++++++++++++++++++ mdl/executor/validate_program.go | 5 + 7 files changed, 228 insertions(+) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-21-alter-flow-fragment-annotations-unchecked.json create mode 100644 mdl-examples/bug-tests/1597-alter-annotation-forms.mdl create mode 100644 mdl-examples/bug-tests/1597-alter-annotation-positional.fail.mdl create mode 100644 mdl/executor/validate_alter_flow.go create mode 100644 mdl/executor/validate_alter_flow_annotations_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-alter-flow-fragment-annotations-unchecked.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-alter-flow-fragment-annotations-unchecked.json new file mode 100644 index 0000000000..0f42fb8455 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-21-alter-flow-fragment-annotations-unchecked.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-21", "symptom": "mendixlabs/mxcli#1597 (case B): `@annotation('Why this commit is here.')` in an `alter microflow … insert after $Result begin … end;` fragment passed check and exec (\"Altered microflow\") and no note was written; the same line in `create or modify microflow` is refused as MDL079", "cause": "the annotation rules (MDL059 unknown name, MDL060 bad curve, MDL079 bad note, MDL092 bad anchor) ran only over create bodies — ValidateProgram had no AlterFlowStmt case, and execAlterFlow ran no statement validation — while the fragment goes through the same flowBuilder, which drops what the visitor could not read", "file": "mdl/executor/validate_alter_flow.go (ValidateAlterFlow, validateAlterFlowRules); validate_program.go (AlterFlowStmt case); cmd_alter_flow.go (execAlterFlow refuses the exec-enforced subset before loading)", "test": "TestValidateProgram_ChecksAnnotationsInAnAlterFlowFragment (all four rules, insert and replace, nested in an if), TestValidateProgram_AlterFlowFragmentValidNotesStayClean (control), TestExecAlterFlow_RefusesADroppedAnnotation", "insight": "The reported rule was one of four with the same hole: probing `@postion`, `@curve(from: (a, b))` and `@anchor(from: sideways)` in the same fragment showed all four passing, which is what moved the fix from one rule to walkAnnotations. Only the annotation rules are run on a fragment — the rest of the microflow rule set reads parameters, declares and later uses of the whole flow, which a fragment cannot see, so running ValidateMicroflow on it would trade a silent drop for false refusals. MDL079 is still not exec-enforced for create or alter, so `-c` and the REPL write past it in both; exec of a file is covered by its check preflight", "refs": ["mendixlabs/mxcli#1597", "ako/mxcli#1507"], "rules": ["MDL059", "MDL060", "MDL079", "MDL092"]} diff --git a/mdl-examples/bug-tests/1597-alter-annotation-forms.mdl b/mdl-examples/bug-tests/1597-alter-annotation-forms.mdl new file mode 100644 index 0000000000..44953f9bde --- /dev/null +++ b/mdl-examples/bug-tests/1597-alter-annotation-forms.mdl @@ -0,0 +1,26 @@ +-- mendixlabs/mxcli#1597 (case B, the control): the two spellings of a note that +-- an `alter microflow … insert` fragment writes. The positional +-- `@annotation('text')` is refused as MDL079 — see +-- 1597-alter-annotation-positional.fail.mdl. +mdl 1; + +create module BugTest1597c; + +create persistent entity BugTest1597c.Item (Code: String(50)); + +create microflow BugTest1597c.ACT_Do () returns Boolean begin + $Result = create BugTest1597c.Item (Code = 'a'); + return true; +end; + +alter microflow BugTest1597c.ACT_Do { + insert after $Result begin + @annotation 'Why this commit is here.' + commit $Result; + @annotation(text: 'And why this log is.') + log info node 'n' 'committed'; + end; +}; + +-- Expect: both notes, attached to the commit and the log. +describe microflow BugTest1597c.ACT_Do; diff --git a/mdl-examples/bug-tests/1597-alter-annotation-positional.fail.mdl b/mdl-examples/bug-tests/1597-alter-annotation-positional.fail.mdl new file mode 100644 index 0000000000..0bb8b8a3d7 --- /dev/null +++ b/mdl-examples/bug-tests/1597-alter-annotation-positional.fail.mdl @@ -0,0 +1,30 @@ +-- ============================================================================ +-- mendixlabs/mxcli#1597 (case B, the refusal): `@annotation('text')` in an +-- `alter microflow … insert` fragment +-- ============================================================================ +-- +-- A positional parameter is not one the visitor reads, so the note is not +-- written at all. `create or modify microflow` refused the line as MDL079; +-- `alter` passed check and exec ("Altered microflow") and wrote the commit +-- without the note. See the companion 1597-alter-annotation-forms.mdl for the +-- two spellings that do write it. +-- +-- This file is expected to FAIL `mxcli check` (MDL079). +-- ============================================================================ +mdl 1; + +create module BugTest1597b; + +create persistent entity BugTest1597b.Item (Code: String(50)); + +create microflow BugTest1597b.ACT_Do () returns Boolean begin + $Result = create BugTest1597b.Item (Code = 'a'); + return true; +end; + +alter microflow BugTest1597b.ACT_Do { + insert after $Result begin + @annotation('Why this commit is here.') + commit $Result; + end; +}; diff --git a/mdl/executor/cmd_alter_flow.go b/mdl/executor/cmd_alter_flow.go index adbfe888b5..fe9a1aa944 100644 --- a/mdl/executor/cmd_alter_flow.go +++ b/mdl/executor/cmd_alter_flow.go @@ -31,6 +31,10 @@ func execAlterFlow(ctx *ExecContext, s *ast.AlterFlowStmt) error { if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // An annotation the fragment's builder would drop (mendixlabs/mxcli#1597). + if err := validateAlterFlowRules(s); err != nil { + return err + } a, err := loadAlterFlow(ctx, s) if err != nil { return err diff --git a/mdl/executor/validate_alter_flow.go b/mdl/executor/validate_alter_flow.go new file mode 100644 index 0000000000..66b6eccdf0 --- /dev/null +++ b/mdl/executor/validate_alter_flow.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateAlterFlow runs the annotation rules (MDL059, MDL060, MDL079, MDL092) +// over the fragments an `alter microflow|nanoflow` inserts or replaces. +// +// A fragment is built by the same flowBuilder as a create body, which drops an +// annotation it cannot read — the visitor records why, and only the validator +// says so. The rules were run over create bodies alone, so +// `@annotation('text')` in an `insert` passed check and exec ("Altered +// microflow") and no note was written, while the same line in `create or +// modify` was refused (mendixlabs/mxcli#1597). +// +// Only the annotation rules: the rest of the microflow rule set reads the whole +// flow — its parameters, its declares, what a later statement uses — and a +// fragment is a piece of a stored flow that this pass cannot see. +func ValidateAlterFlow(s *ast.AlterFlowStmt) []linter.Violation { + v := µflowValidator{mfName: s.Name.String(), docType: s.Kind()} + for _, op := range s.Operations { + v.walkAnnotations(op.Body) + } + return v.violations +} + +// validateAlterFlowRules is validateMicroflowRules for an alter: exec refuses +// what ValidateAlterFlow reports, restricted to the same verified allowlist, so +// `exec -c` and the REPL — which skip the check preflight — refuse what a +// create body would have. +func validateAlterFlowRules(s *ast.AlterFlowStmt) error { + var msgs []string + for _, v := range ValidateAlterFlow(s) { + if v.Severity != linter.SeverityError || !execEnforcedMicroflowRules[v.RuleID] { + continue + } + msg := fmt.Sprintf("[%s] %s", v.RuleID, v.Message) + if v.Suggestion != "" { + msg += "\n " + v.Suggestion + } + msgs = append(msgs, msg) + } + if len(msgs) == 0 { + return nil + } + return mdlerrors.NewValidationf("alter %s '%s' has validation errors:\n - %s", + s.Kind(), s.Name.String(), strings.Join(msgs, "\n - ")) +} diff --git a/mdl/executor/validate_alter_flow_annotations_test.go b/mdl/executor/validate_alter_flow_annotations_test.go new file mode 100644 index 0000000000..13df184ae9 --- /dev/null +++ b/mdl/executor/validate_alter_flow_annotations_test.go @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// mendixlabs/mxcli#1597 (case B). `@annotation('Why this commit is here.')` in +// an `alter microflow … insert` fragment passed check and exec ("Altered +// microflow"), and the note was not written — the visitor cannot read a +// positional parameter, so it drops the whole note and records why. The same +// line in `create or modify microflow` is refused as MDL079. The annotation +// rules (MDL059/MDL060/MDL079/MDL092) were run over create bodies only; an +// alter fragment is a body too, and reached the builder unchecked. + +func parseAlterFlow(t *testing.T, src string) (*ast.Program, *ast.AlterFlowStmt) { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + s, ok := prog.Statements[len(prog.Statements)-1].(*ast.AlterFlowStmt) + if !ok { + t.Fatalf("last statement is %T, want *ast.AlterFlowStmt", prog.Statements[len(prog.Statements)-1]) + } + return prog, s +} + +func alterFlowRuleCounts(t *testing.T, prog *ast.Program) map[string]int { + t.Helper() + ids := map[string]int{} + for _, v := range ValidateProgram(prog, "") { + ids[v.RuleID]++ + } + return ids +} + +func TestValidateProgram_ChecksAnnotationsInAnAlterFlowFragment(t *testing.T) { + cases := []struct { + name, fragment, rule string + }{ + {"positional note text (the report)", "@annotation('Why this commit is here.')\n commit $Result;", "MDL079"}, + {"typo'd annotation name", "@postion(10, 20)\n commit $Result;", "MDL059"}, + {"curve that is not a pair", "@curve(from: (a, b))\n commit $Result;", "MDL060"}, + {"anchor side that does not exist", "@anchor(from: sideways)\n commit $Result;", invalidAnchorRule}, + // Nested inside a fragment's own control flow: the walk has to enter it. + {"inside an if in the fragment", "if $Result != empty then\n @annotation('nested')\n commit $Result;\n end if;", "MDL079"}, + } + for _, op := range []string{"insert after $Result", "replace $Result with"} { + for _, c := range cases { + t.Run(op+"/"+c.name, func(t *testing.T) { + prog, _ := parseAlterFlow(t, "mdl 1;\nalter microflow Probe.ACT_Do {\n "+op+ + " begin\n "+c.fragment+"\n end;\n};") + if got := alterFlowRuleCounts(t, prog)[c.rule]; got == 0 { + t.Errorf("check passed an alter fragment carrying %s — the annotation is dropped by "+ + "the builder, so exec writes the flow without it", c.rule) + } + }) + } + } +} + +// The control: the two spellings of a note that DO work in an alter fragment +// must stay clean, or the check would refuse what the reporter's workaround is. +func TestValidateProgram_AlterFlowFragmentValidNotesStayClean(t *testing.T) { + prog, _ := parseAlterFlow(t, `mdl 1; +alter microflow Probe.ACT_Do { + insert after $Result begin + @annotation 'Why this commit is here.' + @curve(from: (39, 0), to: (0, 61)) + @anchor(from: right, to: left) + commit $Result; + @annotation(text: 'And this log.') + log info node 'n' 'x'; + end; +};`) + ids := alterFlowRuleCounts(t, prog) + for _, rule := range []string{"MDL059", "MDL060", "MDL079", invalidAnchorRule} { + if ids[rule] != 0 { + t.Errorf("%s reported on a valid alter fragment (%d times)", rule, ids[rule]) + } + } +} + +// exec without the preflight (`-c`, the REPL, --no-check) refuses what create +// refuses: the exec-enforced annotation rules, before anything is loaded. +func TestExecAlterFlow_RefusesADroppedAnnotation(t *testing.T) { + _, s := parseAlterFlow(t, `mdl 1; +alter microflow Probe.ACT_Do { + insert after $Result begin + @postion(10, 20) + commit $Result; + end; +};`) + ctx, _ := newMockCtx(t, withBackend(&mock.MockBackend{IsConnectedFunc: func() bool { return true }})) + err := execAlterFlow(ctx, s) + if err == nil || !strings.Contains(err.Error(), "MDL059") { + t.Fatalf("execAlterFlow = %v, want a refusal naming MDL059", err) + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 9e26be466d..6b269effe8 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -115,6 +115,11 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { violations = append(violations, ValidateFlowParameterAnnotations("microflow '"+mfStmt.Name.String()+"'", mfStmt.Parameters)...) } + // An alter fragment is a body too: an annotation its builder drops was + // refused in create and passed here (mendixlabs/mxcli#1597). + if afStmt, ok := stmt.(*ast.AlterFlowStmt); ok { + violations = append(violations, ValidateAlterFlow(afStmt)...) + } // Parameter annotations for the two flow flavours that do not go through // ValidateMicroflow but share the parameter grammar. if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { From ca5f4773ae6b1555fab0704f2a61bb6d52e4c471 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:54:43 +0000 Subject: [PATCH 3/4] fix: set a Java action parameter's description and the return value name (mendixlabs/mxcli#1588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create java action` had no way to set a parameter's description or the return value's name. `describe` printed both as `--` comments (`Input: String not null -- The text to convert.`, `-- return NAME: 'UpperText'`), which read like syntax: written in a script they passed check and exec and were dropped, leaving the description empty and the return name `ReturnValueName`. Both now use the spelling the language already has for the same thing elsewhere: - a `/** … */` doc comment before a parameter is its description, as on an entity attribute; - `returns T as $Name` names the return value, as on a microflow. Java and JavaScript actions share these grammar rules, so both visitors read them through one helper and both describers print them through one emitter; describe output now re-executes as "Unchanged". On `create or modify` an omitted description or name still keeps what is stored. A multi-line description, of which the `--` form printed only the first line, now round-trips whole; one holding `*/`, which a doc comment cannot carry, is still shown as a comment so the output parses. Refs ako/mxcli#1496 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HjYj5GTj2in3ckFYaDBAuN --- ...m-description-return-name-unspellable.json | 1 + .claude/skills/mendix/java-actions/SKILL.md | 35 ++- .../skills/mendix/javascript-actions/SKILL.md | 21 +- cmd/mxcli/syntax/features_integration.go | 6 +- docs-site/src/appendixes/quick-reference.md | 1 + .../reference/microflow/create-java-action.md | 33 ++- .../microflow/create-javascript-action.md | 9 +- docs/01-project/MDL_QUICK_REFERENCE.md | 1 + ...a-action-param-description-return-name.mdl | 40 +++ mdl/ast/ast_javaaction.go | 11 +- mdl/executor/cmd_javaactions.go | 161 +++++++---- .../cmd_javaactions_param_description_test.go | 252 ++++++++++++++++++ mdl/executor/cmd_javascript_actions.go | 48 +--- mdl/executor/cmd_javascript_actions_write.go | 18 +- mdl/grammar/domains/MDLMicroflow.g4 | 10 +- mdl/visitor/visitor_javaaction.go | 91 ++++--- 16 files changed, 567 insertions(+), 171 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-grammar/2026-10-21-java-action-param-description-return-name-unspellable.json create mode 100644 mdl-examples/bug-tests/1588-java-action-param-description-return-name.mdl create mode 100644 mdl/executor/cmd_javaactions_param_description_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-21-java-action-param-description-return-name-unspellable.json b/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-21-java-action-param-description-return-name-unspellable.json new file mode 100644 index 0000000000..7cbe940f75 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-21-java-action-param-description-return-name-unspellable.json @@ -0,0 +1 @@ +{"area": "mdl/grammar", "date": "2026-10-21", "symptom": "mendixlabs/mxcli#1588: `create java action` had no way to set a parameter's description or the return value's name; `describe` printed both as `--` comments (`Input: String not null -- The text to convert.`, `-- return NAME: 'UpperText'`), and a script copying that form passed check and exec with the description empty and the return name `ReturnValueName`", "cause": "no grammar slot for either: javaActionParameter had no docComment and javaActionReturnType no `AS VARIABLE`, so the only spelling describe could offer was a comment the lexer discards. The writers carried the stored values (#705, #731), which hid the gap on round trips of Studio Pro-authored actions and made it total on new ones", "file": "mdl/grammar/domains/MDLMicroflow.g4 (javaActionParameter: docComment?, javaActionReturnType: AS VARIABLE); mdl/visitor/visitor_javaaction.go (buildJavaActionParams, buildJavaActionReturn — one helper for the Java and JavaScript handlers); mdl/executor/cmd_javaactions.go + cmd_javascript_actions_write.go (carriedDocumentation per parameter, stated ReturnName wins); cmd_javaactions.go writeCodeActionParams / describeCodeActionReturnName shared by both describers", "test": "cmd_javaactions_param_description_test.go: create, rewrite (stated replaces, unstated carries) and describe round trip, for Java and JavaScript actions", "insight": "When describe prints a value as a comment, ask whether create has a slot for it — a comment in describe output is either decoration or an unspellable property, and only the second is a bug. Pick the spelling the language already uses for the same thing elsewhere (a member's `/** */` as on an attribute, `returns T as $Name` as on a microflow): the reporter had tried exactly those two first. Java and JavaScript actions share the grammar rules, so a field read by only one handler would have been a fresh silent drop in the other — both visitors now go through one helper and both describers through one emitter. The `--` form had also kept only the first line of a multi-line description; the doc comment carries all of it, and carriedDocumentation's normal form keeps a CRLF description from churning on describe → exec", "refs": ["mendixlabs/mxcli#1588", "ako/mxcli#1496"]} diff --git a/.claude/skills/mendix/java-actions/SKILL.md b/.claude/skills/mendix/java-actions/SKILL.md index e86a3faec4..c800a453be 100644 --- a/.claude/skills/mendix/java-actions/SKILL.md +++ b/.claude/skills/mendix/java-actions/SKILL.md @@ -13,12 +13,12 @@ description: "Create and call custom Java actions — extending Mendix with serv | 28-39 | Reference files | | 40-48 | When to Use This Skill | | 49-55 | Overview | -| 56-332 | Part 2.5: Creating Java Actions in MDL | -| 333-526 | Part 3: Calling Java Actions from MDL | -| 527-540 | Validation Checklist | -| 541-551 | Common Errors | -| 552-559 | Related Documentation | -| 560-641 | Quick Reference | +| 56-355 | Part 2.5: Creating Java Actions in MDL | +| 356-549 | Part 3: Calling Java Actions from MDL | +| 550-563 | Validation Checklist | +| 564-574 | Common Errors | +| 575-582 | Related Documentation | +| 583-664 | Quick Reference | # Mendix Java Actions Skill @@ -78,6 +78,29 @@ return false; $$; ``` +### Parameter Descriptions and the Return Value Name + +What a modeler sees when dragging the action into a microflow: each parameter's +description, and the return value's name (the default output variable name). +A description is a `/** … */` doc comment **before** the parameter, as on an entity +attribute; the return value's name is `returns T as $Name`, as on a microflow: + +```mdl +mdl 1; +/** Converts text to upper case. */ +create java action Module.ToUpper( + /** The text to convert. */ + Input: string not null +) returns string as $UpperText +as $$ +return Input.toUpperCase(); +$$; +``` + +A trailing `-- text` after a parameter is an ordinary comment and is **not stored**. +On `create or modify`, leaving either out keeps what is stored; a new action gets +no descriptions and `ReturnValueName`. The same forms apply to JavaScript actions. + ### Type Parameters (Generics) Type parameters let Java actions accept any entity type dynamically. Use `entity ` in a parameter type to declare the type parameter inline. That parameter becomes the **entity type selector** (receives the entity type name, e.g., `'Module.Entity'`). Bare `pEntity` parameters become **parameterized entity** params (receive entity instances, e.g., `$Variable`). diff --git a/.claude/skills/mendix/javascript-actions/SKILL.md b/.claude/skills/mendix/javascript-actions/SKILL.md index 7d893fd9f7..62311b2d5b 100644 --- a/.claude/skills/mendix/javascript-actions/SKILL.md +++ b/.claude/skills/mendix/javascript-actions/SKILL.md @@ -12,14 +12,14 @@ description: "Create, call and drop JavaScript actions — client-side logic cal |---|---| | 31-39 | When to Use This Skill | | 40-51 | Overview | -| 52-94 | Syntax | -| 95-156 | Examples | -| 157-168 | Calling from a nanoflow | -| 169-193 | The generated .js file | -| 194-202 | Inspecting | -| 203-211 | Validation Checklist | -| 212-221 | Common Errors | -| 222-226 | Related Documentation | +| 52-97 | Syntax | +| 98-159 | Examples | +| 160-171 | Calling from a nanoflow | +| 172-196 | The generated .js file | +| 197-205 | Inspecting | +| 206-214 | Validation Checklist | +| 215-224 | Common Errors | +| 225-229 | Related Documentation | # Mendix JavaScript Actions Skill @@ -53,7 +53,7 @@ normally returns a `Promise`. ```sql CREATE [OR MODIFY] JAVASCRIPT ACTION Module.Name ( parameters ) - RETURNS type + RETURNS type [AS $ReturnName] [EXPOSED AS 'caption' IN 'category'] [PLATFORM Web | Native | Hybrid | All] -- default Web AS $$ @@ -82,6 +82,9 @@ Same type system as Java actions: - Type parameter (generics): declare `EntityType: ENTITY `, then use the bare `pEntity` for instance parameters - Append `NOT NULL` to mark a parameter required. +- A `/** … */` doc comment **before** a parameter is its description; a trailing + `-- text` is an ordinary comment and is not stored. `RETURNS type AS $Name` + names the return value. Left out on `OR MODIFY`, both keep what is stored. ### Parameter names in the body diff --git a/cmd/mxcli/syntax/features_integration.go b/cmd/mxcli/syntax/features_integration.go index b17148de70..b2422f2565 100644 --- a/cmd/mxcli/syntax/features_integration.go +++ b/cmd/mxcli/syntax/features_integration.go @@ -646,8 +646,8 @@ DESCRIBE DATABASE CONNECTION Ops.Erp;`, "create java action", "or modify java action", "type parameter", "entity parameter", "exposed as", "returns", "generics", "drop java action", }, - Syntax: "CREATE [OR MODIFY] JAVA ACTION Module.Name[] [FOLDER 'path'](\n Param: Type [NOT NULL],\n EntityType: ENTITY NOT NULL,\n Obj: pEntity\n) RETURNS ReturnType\n[EXPOSED AS 'Label' IN 'Category'\n [ICON 'icon.png'] [ICON DARK 'icon-dark.png']\n [IMAGE 'image.png'] [IMAGE DARK 'image-dark.png']]\n[NOT EXPOSED]\nAS $$\n// Java code — AS $$ ... $$ is mandatory, cannot be omitted\n$$;\n\nOR MODIFY: updates signature/body in-place, preserves UUID.\n\nEXPOSED AS puts the action in Studio Pro's toolbox. The icon is a 64x64 PNG\nand the image a 256x192 PNG, read from disk relative to the .mdl file's own directory.\nAn OMITTED clause preserves what is stored — including bitmaps set in Studio\nPro — so removing an entry is NOT EXPOSED, and clearing one bitmap is\nDROP ICON / DROP IMAGE [DARK].\n\nType parameters: an ENTITY parameter declares T. A type parameter no\nENTITY parameter names is declared in the header, Module.Name; an\nunqualified type that is neither is refused (MDL-JA01), since it would be\nwritten as an entity with no module (CE1613).", - Example: "mdl 1;\nCREATE JAVA ACTION Utils.FormatCurrency(\n Amount: Decimal NOT NULL\n) RETURNS String\nEXPOSED AS 'Format Currency' IN 'Formatting'\n ICON 'assets/currency-64.png'\n IMAGE 'assets/currency-256.png'\nAS $$\nreturn String.format(\"%.2f\", Amount);\n$$;\n\n-- Generic entity validator with type parameter\nCREATE JAVA ACTION Utils.IsValid(\n EntityType: ENTITY NOT NULL,\n Obj: pEntity NOT NULL\n) RETURNS Boolean\nAS $$\nreturn Obj != null;\n$$;\n\n-- Type parameter with no entity selector: the argument's entity binds it\nCREATE JAVA ACTION Utils.Describe(\n Obj: T NOT NULL\n) RETURNS String\nAS $$\nreturn Obj.toString();\n$$;\n\n-- Idempotent update (preserves UUID)\nCREATE OR MODIFY JAVA ACTION Utils.FormatCurrency(\n Amount: Decimal NOT NULL,\n Decimals: Integer NOT NULL\n) RETURNS String\nAS $$\nreturn String.format(\"%.\" + Decimals + \"f\", Amount);\n$$;", + Syntax: "CREATE [OR MODIFY] JAVA ACTION Module.Name[] [FOLDER 'path'](\n [/** description */] Param: Type [NOT NULL],\n EntityType: ENTITY NOT NULL,\n Obj: pEntity\n) RETURNS ReturnType [AS $ReturnName]\n[EXPOSED AS 'Label' IN 'Category'\n [ICON 'icon.png'] [ICON DARK 'icon-dark.png']\n [IMAGE 'image.png'] [IMAGE DARK 'image-dark.png']]\n[NOT EXPOSED]\nAS $$\n// Java code — AS $$ ... $$ is mandatory, cannot be omitted\n$$;\n\nOR MODIFY: updates signature/body in-place, preserves UUID.\n\nEXPOSED AS puts the action in Studio Pro's toolbox. The icon is a 64x64 PNG\nand the image a 256x192 PNG, read from disk relative to the .mdl file's own directory.\nAn OMITTED clause preserves what is stored — including bitmaps set in Studio\nPro — so removing an entry is NOT EXPOSED, and clearing one bitmap is\nDROP ICON / DROP IMAGE [DARK].\n\nA parameter's description is a /** ... */ doc comment before it, as on an\nentity attribute; AS $ReturnName names the return value (the default name of\nthe variable a microflow call stores it in), as on a microflow. Leaving either\nout keeps what is stored; a new action gets no description and ReturnValueName.\n\nType parameters: an ENTITY parameter declares T. A type parameter no\nENTITY parameter names is declared in the header, Module.Name; an\nunqualified type that is neither is refused (MDL-JA01), since it would be\nwritten as an entity with no module (CE1613).", + Example: "mdl 1;\nCREATE JAVA ACTION Utils.FormatCurrency(\n /** The amount to format. */\n Amount: Decimal NOT NULL\n) RETURNS String AS $FormattedAmount\nEXPOSED AS 'Format Currency' IN 'Formatting'\n ICON 'assets/currency-64.png'\n IMAGE 'assets/currency-256.png'\nAS $$\nreturn String.format(\"%.2f\", Amount);\n$$;\n\n-- Generic entity validator with type parameter\nCREATE JAVA ACTION Utils.IsValid(\n EntityType: ENTITY NOT NULL,\n Obj: pEntity NOT NULL\n) RETURNS Boolean\nAS $$\nreturn Obj != null;\n$$;\n\n-- Type parameter with no entity selector: the argument's entity binds it\nCREATE JAVA ACTION Utils.Describe(\n Obj: T NOT NULL\n) RETURNS String\nAS $$\nreturn Obj.toString();\n$$;\n\n-- Idempotent update (preserves UUID)\nCREATE OR MODIFY JAVA ACTION Utils.FormatCurrency(\n Amount: Decimal NOT NULL,\n Decimals: Integer NOT NULL\n) RETURNS String\nAS $$\nreturn String.format(\"%.\" + Decimals + \"f\", Amount);\n$$;", SeeAlso: []string{"java-action", "javascript-action"}, }) @@ -658,7 +658,7 @@ DESCRIBE DATABASE CONNECTION Ops.Erp;`, "javascript action", "javascript", "js action", "call javascript", "platform", "exposed as", "javascriptaction", }, - Syntax: "LIST JAVASCRIPT ACTIONS [IN Module];\nDESCRIBE JAVASCRIPT ACTION Module.Name;\nCREATE [OR MODIFY] JAVASCRIPT ACTION Module.Name [FOLDER 'path'](...) RETURNS Type [PLATFORM Web|Native|Hybrid|All] AS $$ ... $$;\nDROP JAVASCRIPT ACTION Module.Name;\n\nWrites the unit plus javascriptsource//actions/.js. AS $$ ... $$ is mandatory. PLATFORM defaults to Web. The body sees each parameter with its first character lower-cased (ReadOnly is readOnly, URL is uRL), as mxbuild generates the signature.", + Syntax: "LIST JAVASCRIPT ACTIONS [IN Module];\nDESCRIBE JAVASCRIPT ACTION Module.Name;\nCREATE [OR MODIFY] JAVASCRIPT ACTION Module.Name [FOLDER 'path'](...) RETURNS Type [AS $ReturnName] [PLATFORM Web|Native|Hybrid|All] AS $$ ... $$;\nDROP JAVASCRIPT ACTION Module.Name;\n\nParameters, their /** ... */ descriptions and AS $ReturnName are written as for a Java action (see java-action.create).\n\nWrites the unit plus javascriptsource//actions/.js. AS $$ ... $$ is mandatory. PLATFORM defaults to Web. The body sees each parameter with its first character lower-cased (ReadOnly is readOnly, URL is uRL), as mxbuild generates the signature.", Example: "mdl 1;\nCREATE JAVASCRIPT ACTION Utils.IsStrictMode() RETURNS Boolean\nPLATFORM Web\nAS $$\nreturn Promise.resolve((function(){ return !this; })());\n$$;\n\n-- Exposed, native, with parameters\nCREATE JAVASCRIPT ACTION Utils.ShowToast(\n Message: String NOT NULL,\n Duration: Integer\n) RETURNS Boolean\nEXPOSED AS 'Show Toast' IN 'UI'\nPLATFORM Native\nAS $$\nreturn Promise.resolve(true);\n$$;", SeeAlso: []string{"java-action"}, }) diff --git a/docs-site/src/appendixes/quick-reference.md b/docs-site/src/appendixes/quick-reference.md index 1d6998bcd0..f13ca22cfe 100644 --- a/docs-site/src/appendixes/quick-reference.md +++ b/docs-site/src/appendixes/quick-reference.md @@ -330,6 +330,7 @@ CREATE OR MODIFY NAVIGATION Responsive | Describe Java action | `DESCRIBE JAVA ACTION Module.Name;` | Full MDL output with signature | | Create Java action | `CREATE JAVA ACTION Module.Name(params) RETURNS type AS $$ ... $$;` | Inline Java code | | Create with type params | `CREATE JAVA ACTION Module.Name(EntityType: ENTITY , Obj: pEntity) ...;` | Generic type parameters | +| Describe a parameter, name the return value | `CREATE JAVA ACTION Module.Name(/** The text. */ Input: String) RETURNS String AS $Result AS $$ ... $$;` | `/** … */` before a parameter is its description; `AS $Name` the return value's name | | Create exposed action | `... EXPOSED AS 'Caption' IN 'Category' AS $$ ... $$;` | Toolbox-visible in Studio Pro | | Drop Java action | `DROP JAVA ACTION Module.Name;` | Delete a Java action | | Call from microflow | `$Result = CALL JAVA ACTION Module.Name(Param = value);` | Inside BEGIN...END | diff --git a/docs-site/src/reference/microflow/create-java-action.md b/docs-site/src/reference/microflow/create-java-action.md index 3f45325899..4e4bb9dff3 100644 --- a/docs-site/src/reference/microflow/create-java-action.md +++ b/docs-site/src/reference/microflow/create-java-action.md @@ -4,7 +4,7 @@ ```sql CREATE [ OR MODIFY ] JAVA ACTION module.Name [ < T [, ...] > ] [ FOLDER 'folder_path' ] ( parameters ) - RETURNS type + RETURNS type [ AS $return_name ] [ EXPOSED AS 'caption' IN 'category' ] AS $$ java_code $$ @@ -27,6 +27,19 @@ Java actions support generic entity handling through type parameters. A paramete A type parameter that no `ENTITY ` parameter names -- one bound by the object the caller passes in -- is declared in the header after the action name: `module.Name`. The header also fixes the order of the type parameters when it differs from the order the `ENTITY ` parameters would give. An unqualified type that is neither a declared type parameter nor `Microflow`/`Nanoflow` is refused (`MDL-JA01`): it would be written as an entity with no module, which Mendix reports as CE1613. +### Parameter Descriptions and the Return Value Name + +A parameter's description is a `/** ... */` doc comment written before it -- the +same form an entity attribute's documentation takes. `RETURNS type AS $Name` +names the return value: the default name of the variable a microflow call stores +the result in, as `RETURNS type AS $Name` does on a microflow. Both are what Studio +Pro shows when a modeler drags the action into a microflow, and what `DESCRIBE JAVA +ACTION` prints. + +On `CREATE OR MODIFY`, leaving either out keeps what is stored; an empty `/** */` +clears a description. A new action gets no descriptions and the return value name +`ReturnValueName`, as in Studio Pro. + ### Exposed Actions The optional `EXPOSED AS` clause makes the action visible in the Studio Pro toolbox under the specified category, allowing modelers to use it directly in microflows. @@ -43,7 +56,7 @@ The optional `EXPOSED AS` clause makes the action visible in the Studio Pro tool : Optional. Places the document in the named module folder, creating missing folders in the path. On `CREATE OR MODIFY` this **moves** an existing document; omitting the clause leaves placement alone rather than returning the document to the module root. See [MOVE](../organization/move.md). `parameters` -: Comma-separated parameter declarations. Each parameter has a name, colon, and type. Supported types: +: Comma-separated parameter declarations. Each parameter has an optional `/** description */`, a name, colon, and type. Supported types: - Primitives: `String`, `Integer`, `Long`, `Decimal`, `Boolean`, `DateTime` - Entity: `Module.EntityName` - List: `List of Module.EntityName` @@ -56,6 +69,9 @@ The optional `EXPOSED AS` clause makes the action visible in the Studio Pro tool `RETURNS type` : The return type. Same type options as parameters. +`AS $return_name` +: Optional. The return value's name. Omitted, the stored name is kept (`ReturnValueName` on a new action). + `EXPOSED AS 'caption' IN 'category'` : Optional. Makes the action visible in the toolbox with the given caption and category. @@ -81,6 +97,19 @@ AS $$ $$; ``` +Java action with parameter descriptions and a named return value: + +```sql +/** Converts text to upper case. */ +CREATE JAVA ACTION MyModule.JA_ToUpper ( + /** The text to convert. */ + Input: String NOT NULL +) RETURNS String AS $UpperText +AS $$ + return Input.toUpperCase(); +$$; +``` + Java action with type parameters for generic entity handling: ```sql diff --git a/docs-site/src/reference/microflow/create-javascript-action.md b/docs-site/src/reference/microflow/create-javascript-action.md index 109c9a4afa..49f8b1bdad 100644 --- a/docs-site/src/reference/microflow/create-javascript-action.md +++ b/docs-site/src/reference/microflow/create-javascript-action.md @@ -4,7 +4,7 @@ ```sql CREATE [ OR MODIFY ] JAVASCRIPT ACTION module.Name [ FOLDER 'folder_path' ] ( parameters ) - RETURNS type + RETURNS type [ AS $return_name ] [ EXPOSED AS 'caption' IN 'category' ] [ PLATFORM Web | Native | Hybrid | All ] AS $$ javascript_code $$ @@ -42,6 +42,13 @@ nothing else. The body is written against that name. `ReadOnly` is `readOnly`, body that uses the declared spelling (`ReadOnly`) throws a `ReferenceError` when the action runs. `DESCRIBE` still prints the declared names. +### Parameter Descriptions and the Return Value Name + +As for a Java action, a parameter's description is a `/** ... */` doc comment +before it, and `RETURNS type AS $Name` names the return value. Leaving either out +on `CREATE OR MODIFY` keeps what is stored. See +[CREATE JAVA ACTION](create-java-action.md#parameter-descriptions-and-the-return-value-name). + ### Platform The optional `PLATFORM` clause restricts where the action runs: `Web`, `Native`, diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 0d0dd07c89..e0c311a5e0 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -1442,6 +1442,7 @@ Module.OrderResponse_CustomerInfo/Module.CustomerInfo as customer { | Describe Java action | `describe java action Module.Name;` | Full MDL output with signature | | Create Java action | `create [or modify] java action Module.Name [folder 'path'](params) returns type as $$ ... $$;` | OR MODIFY updates signature/body, preserves UUID | | Create with type params | `create java action Module.Name(EntityType: entity , Obj: pEntity) ...;` | Generic type parameters | +| Describe a parameter, name the return value | `create java action Module.Name(/** The text. */ Input: String) returns String as $Result as $$ ... $$;` | `/** … */` before a parameter is its description; `as $Name` the return value's name. Omitted on `or modify`, both keep what is stored | | Create exposed action | `... exposed as 'caption' in 'Category' as $$ ... $$;` | Toolbox-visible in Studio Pro | | Exposed action with bitmaps | `... exposed as 'caption' in 'Category' icon 'i.png' icon dark 'id.png' image 'm.png' image dark 'md.png' as $$ ... $$;` | Icon 64x64 PNG, image 256x192; paths relative to the .mdl file's own directory. A wrong size warns and is written; a non-PNG is refused | | Remove a toolbox entry | `... not exposed as $$ ... $$;` | An **omitted** clause preserves the stored entry (bitmaps included), so removal is explicit | diff --git a/mdl-examples/bug-tests/1588-java-action-param-description-return-name.mdl b/mdl-examples/bug-tests/1588-java-action-param-description-return-name.mdl new file mode 100644 index 0000000000..a563ab22c9 --- /dev/null +++ b/mdl-examples/bug-tests/1588-java-action-param-description-return-name.mdl @@ -0,0 +1,40 @@ +-- mendixlabs/mxcli#1588: `create java action` had no way to set a parameter's +-- description or the return value's name. `describe` printed both as `--` +-- comments (`Input: String not null -- The text to convert.` and +-- `-- return NAME: 'UpperText'`); written in a script they passed check and +-- exec and were dropped — the description empty, the return name the default +-- `ReturnValueName`. +-- After the fix a `/** … */` before a parameter is its description (as on an +-- entity attribute) and `returns T as $Name` names the return value (as on a +-- microflow), for Java and JavaScript actions alike; `describe` prints those +-- forms, so its output re-executes as "Unchanged". +mdl 1; + +create module BugTest1588; + +/** Converts text to upper case. */ +create java action BugTest1588.ToUpper( + /** The text to convert. */ + Input: String not null, + /** + * How to convert it. + * A description of two lines keeps both. + */ + Mode: String +) returns String as $UpperText +as $$ +return Input.toUpperCase(); +$$; + +create javascript action BugTest1588.JS_ToUpper( + /** The text to convert. */ + Input: String not null +) returns String as $UpperText +as $$ +return Input.toUpperCase(); +$$; + +-- Expect: both descriptions as `/** … */` before their parameters, and +-- `returns String as $UpperText`, in both actions. +describe java action BugTest1588.ToUpper; +describe javascript action BugTest1588.JS_ToUpper; diff --git a/mdl/ast/ast_javaaction.go b/mdl/ast/ast_javaaction.go index 9560510e97..162e43e640 100644 --- a/mdl/ast/ast_javaaction.go +++ b/mdl/ast/ast_javaaction.go @@ -11,6 +11,11 @@ type JavaActionParam struct { Name string // Parameter name Type DataType // Parameter type IsRequired bool // NOT NULL constraint + // Description is the `/** … */` doc comment written before the parameter. + // DescriptionSet says one was written: an absent comment keeps the stored + // description, an empty one clears it (mendixlabs/mxcli#1588). + Description string + DescriptionSet bool } // CreateJavaActionStmt represents: @@ -18,7 +23,7 @@ type JavaActionParam struct { // CREATE JAVA ACTION Module.Name( // EntityType: ENTITY NOT NULL, // Source: pEntity NOT NULL -// ) RETURNS type +// ) RETURNS type [AS $ReturnName] // EXPOSED AS 'caption' IN 'category' // AS $$ ... $$; type CreateJavaActionStmt struct { @@ -27,6 +32,7 @@ type CreateJavaActionStmt struct { Name QualifiedName // Qualified name (Module.ActionName) Parameters []JavaActionParam // Input parameters ReturnType DataType // Return type (can be nil for void) + ReturnName string // `returns T as $Name`: the return value's name; "" keeps the stored one (mendixlabs/mxcli#1588) JavaCode string // The executeAction() body ExtraCode string // Optional extra code section Imports []string // Optional additional imports @@ -57,7 +63,7 @@ func (s *DropJavaActionStmt) isStatement() {} // // CREATE JAVASCRIPT ACTION Module.Name( // Param: type NOT NULL -// ) RETURNS type +// ) RETURNS type [AS $ReturnName] // EXPOSED AS 'caption' IN 'category' // PLATFORM Web // AS $$ ... $$; @@ -70,6 +76,7 @@ type CreateJavaScriptActionStmt struct { Name QualifiedName // Qualified name (Module.ActionName) Parameters []JavaActionParam // Input parameters ReturnType DataType // Return type (can be nil for void) + ReturnName string // `returns T as $Name`; "" keeps the stored one (mendixlabs/mxcli#1588) JavaScriptCode string // The exported function body (user code) Documentation string // Optional documentation comment DocumentationSet bool // see mendixlabs/mxcli#1018: absent preserves, empty clears diff --git a/mdl/executor/cmd_javaactions.go b/mdl/executor/cmd_javaactions.go index 949f9fce3e..8938518b44 100644 --- a/mdl/executor/cmd_javaactions.go +++ b/mdl/executor/cmd_javaactions.go @@ -97,62 +97,15 @@ func describeJavaAction(ctx *ExecContext, name ast.QualifiedName) error { sb.WriteString(describeFolderClause(ctx, ja.ContainerID)) sb.WriteString("(") - // Parameters — one per line when descriptions are present - hasParamDescriptions := false - for _, p := range ja.Parameters { - if p.Description != "" { - hasParamDescriptions = true - break - } - } - - for i, param := range ja.Parameters { - if hasParamDescriptions { - sb.WriteString("\n ") - } - sb.WriteString(param.Name) - sb.WriteString(": ") - if param.ParameterType != nil { - sb.WriteString(formatJavaActionType(param.ParameterType)) - } else { - sb.WriteString("Object") - } - if param.IsRequired { - sb.WriteString(" not null") - } - // The separator precedes any `--` doc comment: written after it, the - // comment swallows the comma and the next parameter fails to parse (#1412). - if i < len(ja.Parameters)-1 { - sb.WriteString(",") - if !hasParamDescriptions { - sb.WriteString(" ") - } - } - if param.Description != "" { - paramDoc := strings.ReplaceAll(param.Description, "\r\n", "\n") - paramDoc = strings.ReplaceAll(paramDoc, "\r", "\n") - firstLine, _, _ := strings.Cut(paramDoc, "\n") - sb.WriteString(" -- ") - sb.WriteString(firstLine) - } - } - if hasParamDescriptions { - sb.WriteString("\n") - } + writeCodeActionParams(&sb, ja.Parameters, formatJavaActionType) sb.WriteString(")") - // Return type + // Return type, and the return value's name if ja.ReturnType != nil { sb.WriteString(" returns ") sb.WriteString(formatJavaActionReturnType(ja.ReturnType)) } - - // RETURN NAME metadata - if ja.ActionDefaultReturnName != "" { - sb.WriteString("\n-- return NAME: '") - sb.WriteString(ja.ActionDefaultReturnName) - sb.WriteString("'") - } + sb.WriteString(describeCodeActionReturnName(ja.ActionDefaultReturnName, ja.ReturnType != nil)) // EXPOSED AS clause if ja.MicroflowActionInfo != nil && ja.MicroflowActionInfo.Caption != "" { @@ -346,6 +299,94 @@ func formatJavaActionType(t javaactions.CodeActionParameterType) string { } // formatJavaActionReturnType formats a Java action return type. +// writeCodeActionParams prints a Java or JavaScript action's parameters, each +// description as the `/** … */` doc comment that create reads before a +// parameter — the form an entity attribute's documentation takes. It was a +// trailing `--` comment, which the lexer discards: a script copying describe's +// output lost every description, and only the first line was printed anyway +// (mendixlabs/mxcli#1588). One parameter per line when any is described. +func writeCodeActionParams(sb *strings.Builder, params []*javaactions.JavaActionParameter, + formatType func(javaactions.CodeActionParameterType) string) { + described := false + for _, p := range params { + if p.Description != "" { + described = true + break + } + } + for i, param := range params { + if described { + sb.WriteString("\n") + if param.Description != "" { + doc := strings.ReplaceAll(param.Description, "\r\n", "\n") + doc = strings.ReplaceAll(doc, "\r", "\n") + // A `*/` would end the doc comment mid-text and the rest would not + // parse. Shown as a comment, the parameter states none, so a + // rewrite carries the stored one. + if strings.Contains(doc, "*/") { + for line := range strings.SplitSeq(doc, "\n") { + fmt.Fprintf(sb, " -- %s\n", line) + } + } else if !strings.Contains(doc, "\n") && !strings.HasPrefix(doc, "*") { + // The inline form holds one line that does not itself start with + // `*`, which the parser strips as the gutter (mendixlabs/mxcli#1428). + fmt.Fprintf(sb, " /** %s */\n", doc) + } else { + outputJavadocIndented(sb, doc, " ") + } + } + sb.WriteString(" ") + } + sb.WriteString(param.Name) + sb.WriteString(": ") + if param.ParameterType != nil { + sb.WriteString(formatType(param.ParameterType)) + } else { + sb.WriteString("Object") + } + if param.IsRequired { + sb.WriteString(" not null") + } + if i < len(params)-1 { + sb.WriteString(",") + if !described { + sb.WriteString(" ") + } + } + } + if described { + sb.WriteString("\n") + } +} + +// describeCodeActionReturnName is the ` as $Name` that follows `returns T`, +// naming the return value as create reads it (mendixlabs/mxcli#1588). A name +// the `$Name` token cannot spell, or one with no `returns` to follow, is still +// shown, as a comment on its own line. +func describeCodeActionReturnName(name string, hasReturns bool) string { + if name == "" { + return "" + } + if hasReturns && isVariableTokenBody(name) { + return " as $" + name + } + return "\n-- return NAME: '" + name + "'" +} + +// isVariableTokenBody reports whether `$` + s lexes as one VARIABLE token +// ('$' ID_BODY+). +func isVariableTokenBody(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if !(r == '_' || r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9') { + return false + } + } + return true +} + func formatJavaActionReturnType(t javaactions.CodeActionReturnType) string { if t == nil { return "Void" @@ -455,11 +496,12 @@ func execCreateJavaAction(ctx *ExecContext, s *ast.CreateJavaActionStmt) error { var existingJADoc string haveExistingJA := false var existingActionInfo *javaactions.MicroflowActionInfo - // A parameter's Description and Category have no MDL spelling either — - // DESCRIBE prints the description as a comment — so a rewrite carries them - // from the stored parameter of the same name. + // A parameter's Category has no MDL spelling, and its Description is set + // only by a `/** … */` before it, so a rewrite carries both from the stored + // parameter of the same name unless the statement says otherwise. storedParams := map[string]*javaactions.JavaActionParameter{} - // Neither has an MDL spelling, so a rewrite carries them. The defaults are + // The export level has no MDL spelling and the return value's name is set + // only by `returns T as $Name`, so a rewrite carries them. The defaults are // what Studio Pro writes on a new action; "Public" — what this used to // hardcode — is not a member of JavaActionsExportLevel (API | Hidden), and // every rewrite turned a Hidden action into it (ako/mxcli#705). @@ -505,6 +547,11 @@ func execCreateJavaAction(ctx *ExecContext, s *ast.CreateJavaActionStmt) error { newID = existingJAID } + // `returns T as $Name` names the return value (mendixlabs/mxcli#1588). + if s.ReturnName != "" { + defaultReturnName = s.ReturnName + } + // Create the Java action ja := &javaactions.JavaAction{ BaseElement: model.BaseElement{ @@ -581,10 +628,14 @@ func execCreateJavaAction(ctx *ExecContext, s *ast.CreateJavaActionStmt) error { } else { jaParam.ParameterType = astDataTypeToJavaActionParamType(param.Type) } + var storedDescription string if sp := storedParams[param.Name]; sp != nil { - jaParam.Description = sp.Description + storedDescription = sp.Description jaParam.Category = sp.Category } + // A doc comment before the parameter sets its description; none keeps + // the stored one (mendixlabs/mxcli#1588). + jaParam.Description = carriedDocumentation(param.DescriptionSet, param.Description, storedDescription) ja.Parameters = append(ja.Parameters, jaParam) } diff --git a/mdl/executor/cmd_javaactions_param_description_test.go b/mdl/executor/cmd_javaactions_param_description_test.go new file mode 100644 index 0000000000..b06e3b58ff --- /dev/null +++ b/mdl/executor/cmd_javaactions_param_description_test.go @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/javaactions" +) + +// mendixlabs/mxcli#1588. `create java action` had no way to set a parameter's +// description or the return value's name. `describe` printed both as `--` +// comments (`Input: String not null -- The text to convert.` and `-- return +// NAME: 'UpperText'`), which read like syntax: written in a script they passed +// check and exec and were dropped, the description left empty and the return +// name the default `ReturnValueName`. +// +// They are now spelled the way the language already spells the same two things +// elsewhere — a parameter's description is a `/** … */` doc comment before it, +// as on an entity attribute, and the return value's name is `returns T as +// $Name`, as on a microflow — and describe prints those forms, so a describe → +// exec round trip carries them. Both forms are shared with JavaScript actions. + +const toUpperScript = `create java action MyModule.ToUpper( + /** The text to convert. */ + Input: String not null, + /** + * How to convert it. + * Two lines, both kept. + */ + Mode: String +) returns String as $UpperText +as $$ +return Input.toUpperCase(); +$$;` + +func parseOne[T ast.Statement](t *testing.T, src string) T { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse: %v\n%s", errs, src) + } + return findStmt[T](t, prog, src) +} + +func TestCreateJavaAction_WritesStatedParameterDescriptionAndReturnName(t *testing.T) { + s := parseOne[*ast.CreateJavaActionStmt](t, toUpperScript) + + mod := mkModule("MyModule") + var captured *javaactions.JavaAction + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListJavaActionsFunc: func() ([]*types.JavaAction, error) { return nil, nil }, + CreateJavaActionFunc: func(ja *javaactions.JavaAction) error { + captured = ja + return nil + }, + WriteJavaSourceFileFunc: func(string, string, string, []*javaactions.JavaActionParameter, + javaactions.CodeActionReturnType, []string, string) error { + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + assertNoError(t, execCreateJavaAction(ctx, s)) + + if captured == nil || len(captured.Parameters) != 2 { + t.Fatalf("CreateJavaAction not called with two parameters: %+v", captured) + } + if got := captured.Parameters[0].Description; got != "The text to convert." { + t.Errorf("Input description = %q, want %q", got, "The text to convert.") + } + if got, want := captured.Parameters[1].Description, "How to convert it.\nTwo lines, both kept."; got != want { + t.Errorf("Mode description = %q, want %q", got, want) + } + if captured.ActionDefaultReturnName != "UpperText" { + t.Errorf("return value name = %q, want UpperText — the default is ReturnValueName, "+ + "which is what the report saw", captured.ActionDefaultReturnName) + } +} + +// A stated value replaces the stored one; an unstated one is still carried +// (TestJavaActionRewriteCarriesParameterDescriptionAndCategory), so the +// existing preserve rule and the new spelling do not fight. +func TestJavaActionRewrite_StatedDescriptionAndReturnNameReplaceStored(t *testing.T) { + mod := mkModule("MyModule") + stored := storedExposedAction(mod) + stored.ActionDefaultReturnName = "Old" + stored.Parameters = []*javaactions.JavaActionParameter{ + {Name: "Input", Description: "old text", Category: "Kept"}, + {Name: "Other", Description: "kept, the statement does not say"}, + } + ctx, captured := exposeRewriteCtx(t, stored, mod) + + s := rewriteStmt(false) + s.ReturnType = ast.DataType{Kind: ast.TypeString} + s.ReturnName = "New" + s.Parameters = []ast.JavaActionParam{ + {Name: "Input", Type: ast.DataType{Kind: ast.TypeString}, Description: "new text", DescriptionSet: true}, + {Name: "Other", Type: ast.DataType{Kind: ast.TypeString}}, + } + assertNoError(t, execCreateJavaAction(ctx, s)) + + got := *captured + if got == nil { + t.Fatal("UpdateJavaAction was never called") + } + if p := got.Parameters[0]; p.Description != "new text" || p.Category != "Kept" { + t.Errorf("Input: description %q, category %q — want the stated description and the carried category", p.Description, p.Category) + } + if p := got.Parameters[1]; p.Description != "kept, the statement does not say" { + t.Errorf("Other: description %q — an unstated description must be carried", p.Description) + } + if got.ActionDefaultReturnName != "New" { + t.Errorf("return value name = %q, want the stated New", got.ActionDefaultReturnName) + } +} + +// Describe prints the forms create reads, so its output re-executes into the +// same values — including a description of several lines, of which the `--` +// form kept only the first. +func TestDescribeJavaAction_DescriptionAndReturnNameRoundTrip(t *testing.T) { + mod := mkModule("MyModule") + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ReadJavaActionByNameFunc: func(string) (*javaactions.JavaAction, error) { + return &javaactions.JavaAction{ + BaseElement: model.BaseElement{ID: nextID("ja")}, + ContainerID: mod.ID, + Name: "ToUpper", + ActionDefaultReturnName: "UpperText", + ReturnType: &types.StringType{}, + Parameters: describedParams(), + }, nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb)) + assertNoError(t, describeJavaAction(ctx, ast.QualifiedName{Module: "MyModule", Name: "ToUpper"})) + out := buf.String() + stmt := findStmt[*ast.CreateJavaActionStmt](t, reparse(t, out), out) + assertDescribedForms(t, out, stmt.Parameters, stmt.ReturnName) +} + +func TestCreateJavaScriptAction_WritesStatedParameterDescriptionAndReturnName(t *testing.T) { + s := parseOne[*ast.CreateJavaScriptActionStmt](t, + strings.Replace(toUpperScript, "java action", "javascript action", 1)) + + mod := mkModule("MyModule") + var captured *types.JavaScriptAction + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListJavaScriptActionsFunc: func() ([]*types.JavaScriptAction, error) { return nil, nil }, + CreateJavaScriptActionFunc: func(jsa *types.JavaScriptAction) error { + captured = jsa + return nil + }, + WriteJavaScriptSourceFileFunc: func(string, string, string, []*types.JavaActionParameter, types.CodeActionReturnType) error { + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + assertNoError(t, execCreateJavaScriptAction(ctx, s)) + + if captured == nil || len(captured.Parameters) != 2 { + t.Fatalf("CreateJavaScriptAction not called with two parameters: %+v", captured) + } + if got := captured.Parameters[0].Description; got != "The text to convert." { + t.Errorf("Input description = %q", got) + } + if got, want := captured.Parameters[1].Description, "How to convert it.\nTwo lines, both kept."; got != want { + t.Errorf("Mode description = %q, want %q", got, want) + } + if captured.ActionDefaultReturnName != "UpperText" { + t.Errorf("return value name = %q, want UpperText", captured.ActionDefaultReturnName) + } +} + +func TestDescribeJavaScriptAction_DescriptionAndReturnNameRoundTrip(t *testing.T) { + mod := mkModule("WebMod") + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ReadJavaScriptActionByNameFunc: func(string) (*types.JavaScriptAction, error) { + return &types.JavaScriptAction{ + BaseElement: model.BaseElement{ID: nextID("jsa")}, + ContainerID: mod.ID, + Name: "ToUpper", + Platform: "Web", + ActionDefaultReturnName: "UpperText", + ReturnType: &types.StringType{}, + Parameters: describedParams(), + }, nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb)) + assertNoError(t, describeJavaScriptAction(ctx, ast.QualifiedName{Module: "WebMod", Name: "ToUpper"})) + out := buf.String() + stmt := findStmt[*ast.CreateJavaScriptActionStmt](t, reparse(t, out), out) + assertDescribedForms(t, out, stmt.Parameters, stmt.ReturnName) +} + +// describedParams covers the description shapes describe has to print: one +// line, several lines (CRLF, as Studio Pro stores it), a line that starts with +// `*` (which the doc-comment gutter would eat inline), one holding `*/` (which +// would end the doc comment early, so it cannot be one), and none. +func describedParams() []*types.JavaActionParameter { + return []*types.JavaActionParameter{ + {Name: "Input", Description: "The text to convert.", IsRequired: true, ParameterType: &types.StringType{}}, + {Name: "Mode", Description: "How to convert it.\r\nTwo lines, both kept.", ParameterType: &types.StringType{}}, + {Name: "Bullet", Description: "* a bullet", ParameterType: &types.StringType{}}, + {Name: "Glob", Description: "Files matching /tmp/*/ or */logs", ParameterType: &types.StringType{}}, + {Name: "Plain", ParameterType: &types.StringType{}}, + } +} + +func assertDescribedForms(t *testing.T, out string, params []ast.JavaActionParam, returnName string) { + t.Helper() + if strings.Contains(out, "-- return NAME") || strings.Contains(out, " -- The text") { + t.Errorf("describe still prints a value as a `--` comment, which create drops:\n%s", out) + } + want := []struct { + name, desc string + set bool + }{ + {"Input", "The text to convert.", true}, + {"Mode", "How to convert it.\nTwo lines, both kept.", true}, + {"Bullet", "* a bullet", true}, + // Not stated, so a rewrite carries the stored description — and the + // output still parses, which a doc comment holding `*/` would not. + {"Glob", "", false}, + {"Plain", "", false}, + } + if len(params) != len(want) { + t.Fatalf("re-parsed %d parameters, want %d\n%s", len(params), len(want), out) + } + for i, w := range want { + p := params[i] + if p.Name != w.name || p.Description != w.desc || p.DescriptionSet != w.set { + t.Errorf("parameter %d re-parsed as %q description %q (set %v), want %q %q (set %v)\n%s", + i, p.Name, p.Description, p.DescriptionSet, w.name, w.desc, w.set, out) + } + } + if returnName != "UpperText" { + t.Errorf("return value name re-parsed as %q, want UpperText\n%s", returnName, out) + } +} diff --git a/mdl/executor/cmd_javascript_actions.go b/mdl/executor/cmd_javascript_actions.go index 113b29f44b..2216f4c0c3 100644 --- a/mdl/executor/cmd_javascript_actions.go +++ b/mdl/executor/cmd_javascript_actions.go @@ -102,48 +102,8 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { sb.WriteString(describeFolderClause(ctx, jsa.ContainerID)) sb.WriteString("(") - // Parameters - hasParamDescriptions := false - for _, p := range jsa.Parameters { - if p.Description != "" { - hasParamDescriptions = true - break - } - } - - for i, param := range jsa.Parameters { - if hasParamDescriptions { - sb.WriteString("\n ") - } - sb.WriteString(param.Name) - sb.WriteString(": ") - if param.ParameterType != nil { - sb.WriteString(formatJavaScriptActionType(param.ParameterType)) - } else { - sb.WriteString("Object") - } - if param.IsRequired { - sb.WriteString(" not null") - } - // The separator precedes any `--` doc comment: written after it, the - // comment swallows the comma and the next parameter fails to parse (#1412). - if i < len(jsa.Parameters)-1 { - sb.WriteString(",") - if !hasParamDescriptions { - sb.WriteString(" ") - } - } - if param.Description != "" { - paramDoc := strings.ReplaceAll(param.Description, "\r\n", "\n") - paramDoc = strings.ReplaceAll(paramDoc, "\r", "\n") - firstLine, _, _ := strings.Cut(paramDoc, "\n") - sb.WriteString(" -- ") - sb.WriteString(firstLine) - } - } - if hasParamDescriptions { - sb.WriteString("\n") - } + // Parameters, each description as the doc comment create reads (see the Java twin). + writeCodeActionParams(&sb, jsa.Parameters, formatJavaScriptActionType) sb.WriteString(")") // Clauses are emitted in grammar order (returns, exposed, platform, body) so @@ -154,6 +114,7 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { sb.WriteString("\n returns ") sb.WriteString(formatJavaActionReturnType(jsa.ReturnType)) } + sb.WriteString(describeCodeActionReturnName(jsa.ActionDefaultReturnName, jsa.ReturnType != nil)) // EXPOSED AS clause if jsa.MicroflowActionInfo != nil && jsa.MicroflowActionInfo.Caption != "" { @@ -197,9 +158,6 @@ func describeJavaScriptAction(ctx *ExecContext, name ast.QualifiedName) error { fmt.Fprintln(ctx.Output, stmt) // Additional info as comments. - if jsa.ActionDefaultReturnName != "" { - fmt.Fprintf(ctx.Output, "-- return NAME: '%s'\n", jsa.ActionDefaultReturnName) - } if jsa.MicroflowActionInfo != nil && len(jsa.MicroflowActionInfo.IconData) > 0 { fmt.Fprintf(ctx.Output, "-- icon: %d bytes\n", len(jsa.MicroflowActionInfo.IconData)) } diff --git a/mdl/executor/cmd_javascript_actions_write.go b/mdl/executor/cmd_javascript_actions_write.go index 6de9667fe4..a59ce88b67 100644 --- a/mdl/executor/cmd_javascript_actions_write.go +++ b/mdl/executor/cmd_javascript_actions_write.go @@ -55,13 +55,15 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS var existingActionInfo *types.MicroflowActionInfo var existingJSDoc string haveExistingJS := false - // A parameter's Description and Category have no MDL spelling — DESCRIBE - // prints the description as a comment — so a rewrite carries them from the - // stored parameter of the same name, as the Java twin does (#731). + // A parameter's Category has no MDL spelling, and its Description is set + // only by a `/** … */` before it, so a rewrite carries both from the stored + // parameter of the same name unless the statement says otherwise, as the + // Java twin does (#731, mendixlabs/mxcli#1588). storedParams := map[string]*types.JavaActionParameter{} // The stored parameter names in order, for sourceUnchanged below. var storedParamNames []string - // No MDL spelling for either, so a rewrite carries them; the defaults are + // The export level has no MDL spelling and the return value's name is set + // only by `returns T as $Name`, so a rewrite carries them; the defaults are // Studio Pro's. "Public", which this hardcoded, is not a member of // JavaScriptActionsExportLevel (API | Hidden) — see the Java twin. exportLevel, defaultReturnName := "Hidden", "ReturnValueName" @@ -110,6 +112,10 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS newID = existingID } + if s.ReturnName != "" { + defaultReturnName = s.ReturnName + } + jsa := &types.JavaScriptAction{ BaseElement: model.BaseElement{ID: newID, TypeName: "JavaScriptActions$JavaScriptAction"}, ContainerID: containerID, @@ -170,10 +176,12 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS p.ParameterType = astDataTypeToJavaActionParamType(param.Type) } } + var storedDescription string if sp := storedParams[param.Name]; sp != nil { - p.Description = sp.Description + storedDescription = sp.Description p.Category = sp.Category } + p.Description = carriedDocumentation(param.DescriptionSet, param.Description, storedDescription) jsa.Parameters = append(jsa.Parameters, p) } diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index 852344669b..07584f508c 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/mdl/grammar/domains/MDLMicroflow.g4 @@ -73,12 +73,18 @@ javaActionTypeParameters : LESS_THAN identifierOrKeyword (COMMA identifierOrKeyword)* GREATER_THAN ; +// A parameter's description is a `/** … */` doc comment before it, as on an +// entity attribute. DESCRIBE printed it as a trailing `--` comment, which the +// lexer discards, so a script copying that form lost it (mendixlabs/mxcli#1588). javaActionParameter - : parameterName COLON dataType NOT_NULL? + : docComment? parameterName COLON dataType NOT_NULL? ; +// `returns T as $Name` names the return value — the default name of the +// variable a microflow call stores it in — with the spelling a microflow's +// return variable uses (mendixlabs/mxcli#1588). javaActionReturnType - : RETURNS dataType + : RETURNS dataType (AS VARIABLE)? ; // EXPOSED AS 'Caption' IN 'Category' — appear in Studio Pro's toolbox diff --git a/mdl/visitor/visitor_javaaction.go b/mdl/visitor/visitor_javaaction.go index 0b28eb6a78..5a080bda2c 100644 --- a/mdl/visitor/visitor_javaaction.go +++ b/mdl/visitor/visitor_javaaction.go @@ -22,22 +22,7 @@ func (b *Builder) ExitCreateJavaActionStatement(ctx *parser.CreateJavaActionStat } // Get parameters - if paramList := ctx.JavaActionParameterList(); paramList != nil { - for _, paramCtx := range paramList.AllJavaActionParameter() { - param := ast.JavaActionParam{} - if pn := paramCtx.ParameterName(); pn != nil { - param.Name = parameterNameText(pn) - } - if dt := paramCtx.DataType(); dt != nil { - param.Type = buildDataType(dt) - } - // Check for NOT NULL constraint - if paramCtx.NOT_NULL() != nil { - param.IsRequired = true - } - stmt.Parameters = append(stmt.Parameters, param) - } - } + stmt.Parameters = buildJavaActionParams(ctx.JavaActionParameterList()) // Type parameters: those the `Name` header declares, in its order, // then any ENTITY parameter declares that the header did not @@ -64,11 +49,7 @@ func (b *Builder) ExitCreateJavaActionStatement(ctx *parser.CreateJavaActionStat } // Get return type - if retType := ctx.JavaActionReturnType(); retType != nil { - if dt := retType.DataType(); dt != nil { - stmt.ReturnType = buildJavaActionReturnType(dt) - } - } + stmt.ReturnType, stmt.ReturnName = buildJavaActionReturn(ctx.JavaActionReturnType()) // Get exposed clause (EXPOSED AS 'caption' IN 'category') if exposed := ctx.JavaActionExposedClause(); exposed != nil { @@ -134,21 +115,7 @@ func (b *Builder) ExitCreateJavaScriptActionStatement(ctx *parser.CreateJavaScri stmt.Folder = unquoteStringLit(lit) } - if paramList := ctx.JavaActionParameterList(); paramList != nil { - for _, paramCtx := range paramList.AllJavaActionParameter() { - param := ast.JavaActionParam{} - if pn := paramCtx.ParameterName(); pn != nil { - param.Name = parameterNameText(pn) - } - if dt := paramCtx.DataType(); dt != nil { - param.Type = buildDataType(dt) - } - if paramCtx.NOT_NULL() != nil { - param.IsRequired = true - } - stmt.Parameters = append(stmt.Parameters, param) - } - } + stmt.Parameters = buildJavaActionParams(ctx.JavaActionParameterList()) // Extract type parameters from ENTITY parameter declarations. for _, param := range stmt.Parameters { @@ -166,11 +133,7 @@ func (b *Builder) ExitCreateJavaScriptActionStatement(ctx *parser.CreateJavaScri } } - if retType := ctx.JavaActionReturnType(); retType != nil { - if dt := retType.DataType(); dt != nil { - stmt.ReturnType = buildJavaActionReturnType(dt) - } - } + stmt.ReturnType, stmt.ReturnName = buildJavaActionReturn(ctx.JavaActionReturnType()) if exposed := ctx.JavaActionExposedClause(); exposed != nil { allStrings := exposed.AllSTRING_LITERAL() @@ -285,3 +248,49 @@ func extractJavaImports(code string) (body string, imports []string) { } return strings.TrimSpace(strings.Join(bodyLines, "\n")), imports } + +// buildJavaActionParams reads a Java or JavaScript action's parameter list — +// one helper for both, since they share the grammar rule and a field read by +// one handler only is a silent drop in the other. +func buildJavaActionParams(list parser.IJavaActionParameterListContext) []ast.JavaActionParam { + if list == nil { + return nil + } + var params []ast.JavaActionParam + for _, paramCtx := range list.AllJavaActionParameter() { + param := ast.JavaActionParam{} + if pn := paramCtx.ParameterName(); pn != nil { + param.Name = parameterNameText(pn) + } + if dt := paramCtx.DataType(); dt != nil { + param.Type = buildDataType(dt) + } + // Check for NOT NULL constraint + if paramCtx.NOT_NULL() != nil { + param.IsRequired = true + } + // The parameter's description (mendixlabs/mxcli#1588). + if doc := paramCtx.DocComment(); doc != nil { + param.Description, param.DescriptionSet = extractDocComment(doc.GetText()), true + } + params = append(params, param) + } + return params +} + +// buildJavaActionReturn reads `returns T [as $Name]`: the return type, and the +// return value's name without its `$` ("" when the clause names none). +func buildJavaActionReturn(ret parser.IJavaActionReturnTypeContext) (ast.DataType, string) { + if ret == nil { + return ast.DataType{}, "" + } + var dt ast.DataType + if d := ret.DataType(); d != nil { + dt = buildJavaActionReturnType(d) + } + var name string + if v := ret.VARIABLE(); v != nil { + name = strings.TrimPrefix(v.GetText(), "$") + } + return dt, name +} From e9bfd00164768895a485c8283827d1794ca70d46 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 05:07:40 +0000 Subject: [PATCH 4/4] =?UTF-8?q?ci(deps):=20bump=20Go=20toolchain=201.27.2?= =?UTF-8?q?=20=E2=86=92=201.27.3=20for=20GO-2026-6809?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit govulncheck flagged GO-2026-6809 (distrust compromised intermediate certificates in crypto/x509), present in crypto/x509@go1.27.2 and fixed in go1.27.3. mxcli reaches the vulnerable code through marketplace.Client.Download (io.Copy over TLS). This is a new advisory against the standard library: build-and-test is red on main for the same reason. Bump the toolchain in go.mod and the go-version in every workflow, so CI installs the fixed standard library. Locally, govulncheck ./... reports GO-2026-6809 on go1.27.2 (exit 3) and "No vulnerabilities found" on go1.27.3. make build, make test and make vet pass on go1.27.3. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AeAyg3qx2my26Q8cvGyW5z (cherry picked from commit 21be71b6f53838b1481370da13ed1dca1020fee3) --- .github/workflows/nightly.yml | 8 ++++---- .github/workflows/push-test.yml | 8 ++++---- .github/workflows/release.yml | 2 +- go.mod | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 5bdbdda112..da3b5719ea 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -40,7 +40,7 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Cache ANTLR4 JAR uses: actions/cache@v6 @@ -89,7 +89,7 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' # mxbuild 9 draws through System.Drawing; without libgdiplus every mx call # dies with "The type initializer for 'Gdip' threw an exception." @@ -144,7 +144,7 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Cache ANTLR4 JAR uses: actions/cache@v6 with: @@ -175,7 +175,7 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - uses: oven-sh/setup-bun@v2 diff --git a/.github/workflows/push-test.yml b/.github/workflows/push-test.yml index 1b876f965d..f9feb55789 100644 --- a/.github/workflows/push-test.yml +++ b/.github/workflows/push-test.yml @@ -23,7 +23,7 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Test the tunnel seam shell: bash # Scoped with -run to the seam's own tests. The full test binaries are still @@ -78,7 +78,7 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Test the Windows process helpers shell: bash # Scoped with -run: the rest of the docker package's tests assert POSIX @@ -122,7 +122,7 @@ jobs: submodules: true - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Cache ANTLR4 JAR uses: actions/cache@v6 with: @@ -256,7 +256,7 @@ jobs: submodules: ${{ matrix.suite == 'roundtrip' || matrix.suite == 'parity' }} - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - name: Cache ANTLR4 JAR uses: actions/cache@v6 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2f0b8d4021..5bdcf8eb98 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -17,7 +17,7 @@ jobs: fetch-depth: 0 - uses: actions/setup-go@v7 with: - go-version: '1.27.2' + go-version: '1.27.3' - uses: oven-sh/setup-bun@v2 - name: Cache ANTLR4 JAR uses: actions/cache@v6 diff --git a/go.mod b/go.mod index 48d47e5da9..73f39f6398 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/mendixlabs/mxcli go 1.26.0 -toolchain go1.27.2 +toolchain go1.27.3 require ( github.com/alecthomas/chroma/v2 v2.26.1