Skip to content

Commit 11a9bc2

Browse files
hjothamendixclaude
andcommitted
fix: address merged-PR ako follow-up polish items
Batches the remaining polish items ako flagged in PRs that already merged: - PR mendixlabs#357 (Java return-type inference): remove the dead `case javaactions.ListType:` value form, replace the exprToString→strip→lookup string-mangling with a direct `*ast.VariableExpr` type assertion, and add the missing unit test for `*javaactions.EntityType` return registration. - PR mendixlabs#363 (change refresh modifier): restore the CE0032 reference in the `addChangeObjectAction` comment so developers searching for the Studio Pro error code find the inference logic. - PR mendixlabs#364 (enum split): collapse the dual `return true` branches of `isTerminalStmt`'s EnumSplitStmt arm into one and expand the comment documenting the intentional divergence from `bodyReturns`. - PR mendixlabs#364 minor: add a doc comment on `hasExplicitFalseBranchAnchor` explaining the Top→Bottom heuristic, and add TestHasExplicitFalseBranchAnchor covering nil, default, and single-sided cases. Validation: make build, make test, make lint-go. Related: mendixlabs#463 (walkStatements refactor), mendixlabs#468 (TitleOverride nil). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent db1b1e9 commit 11a9bc2

6 files changed

Lines changed: 128 additions & 27 deletions

mdl/executor/cmd_microflows_builder_actions.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,13 @@ func (fb *flowBuilder) addRollbackAction(s *ast.RollbackStmt) model.ID {
242242

243243
// addChangeObjectAction creates a CHANGE statement.
244244
func (fb *flowBuilder) addChangeObjectAction(s *ast.ChangeObjectStmt) model.ID {
245-
// Empty non-committing changes need RefreshInClient to satisfy Studio Pro
246-
// consistency checks; explicit `refresh` keeps the same flag for all changes.
245+
// CE0032 rejects change actions with no items that do not commit the
246+
// object. The published error text only mentions items/commit, but
247+
// `mx check` also accepts RefreshInClient=true as a third valid escape.
248+
// The builder auto-promotes empty changes to refresh-only so describe →
249+
// exec of such actions stays valid without requiring authored MDL to say
250+
// `refresh` explicitly; when the author wrote `refresh`, we keep the
251+
// same flag for non-empty changes too.
247252
action := &microflows.ChangeObjectAction{
248253
BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())},
249254
ErrorHandlingType: fb.ehType(nil),

mdl/executor/cmd_microflows_builder_calls.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -373,28 +373,28 @@ func javaActionReturnVarType(returnType javaactions.CodeActionReturnType) string
373373
return ""
374374
}
375375

376+
// inferGenericJavaActionReturnType infers the element type of a generic
377+
// `ListType{Entity: ""}` Java action return by inspecting the caller's
378+
// variable-typed arguments. When the action parameters don't determine an
379+
// element type, the function returns "" and the caller records the declaration
380+
// as Unknown.
376381
func (fb *flowBuilder) inferGenericJavaActionReturnType(jaDef *javaactions.JavaAction, s *ast.CallJavaActionStmt) string {
377382
if jaDef == nil || fb.varTypes == nil || s == nil {
378383
return ""
379384
}
380-
switch t := jaDef.ReturnType.(type) {
381-
case *javaactions.ListType:
382-
if t.Entity != "" {
383-
return ""
384-
}
385-
case javaactions.ListType:
386-
if t.Entity != "" {
387-
return ""
388-
}
389-
default:
385+
// ListType is always stored as a pointer by the parser; there is no value
386+
// form in the SDK. Only the generic (Entity == "") case reaches the
387+
// variable-type lookup below.
388+
t, ok := jaDef.ReturnType.(*javaactions.ListType)
389+
if !ok || t.Entity != "" {
390390
return ""
391391
}
392392
for _, arg := range s.Arguments {
393-
valueExpr := strings.TrimPrefix(strings.Trim(fb.exprToString(arg.Value), "'"), "$")
394-
if valueExpr == "" {
393+
varExpr, ok := arg.Value.(*ast.VariableExpr)
394+
if !ok {
395395
continue
396396
}
397-
if typ := fb.varTypes[valueExpr]; strings.HasPrefix(typ, "List of ") {
397+
if typ := fb.varTypes[varExpr.Name]; strings.HasPrefix(typ, "List of ") {
398398
return typ
399399
}
400400
}

mdl/executor/cmd_microflows_builder_flows.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,12 +279,15 @@ func isTerminalStmt(stmt ast.MicroflowStatement) bool {
279279
return false
280280
}
281281
}
282-
// Both paths return true intentionally. isTerminalStmt diverges from
283-
// bodyReturns here: bodyReturns requires an ELSE to guarantee all paths
284-
// return (a valid Mendix requirement), but isTerminalStmt only needs to
285-
// know whether the flow builder must thread a continuation edge past this
286-
// statement. When every explicit case terminates the split has no outgoing
287-
// merge edge regardless of whether an ELSE exists, so we are always terminal.
282+
// Every reachable branch terminates, so the split has no continuation
283+
// to thread into the parent flow. This intentionally diverges from
284+
// `bodyReturns` in validate_microflow.go: that predicate treats an
285+
// enum split without an `else` as non-terminal because authored MDL
286+
// is expected to supply a default branch covering missing values.
287+
// Here we also accept described-from-MPR graphs where Studio Pro
288+
// produced an exhaustive set of value cases without a default flow —
289+
// in both no-else and with-else forms the split terminates once we
290+
// reach this point.
288291
return true
289292
default:
290293
return false

mdl/executor/cmd_microflows_builder_java_return_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,37 @@ func TestAddJavaAction_ConcreteListReturnRegistersListType(t *testing.T) {
6363
}
6464
}
6565

66+
// TestAddJavaAction_EntityReturnRegistersEntityType pins ako's review follow-up
67+
// for PR #357: the most common Java action return shape — a bare
68+
// `*javaactions.EntityType{Entity: "Mod.Ent"}` — must register the output
69+
// variable as the entity's qualified name so downstream attribute access
70+
// resolves against the right domain-model entry.
71+
func TestAddJavaAction_EntityReturnRegistersEntityType(t *testing.T) {
72+
backend := &mock.MockBackend{
73+
ReadJavaActionByNameFunc: func(qualifiedName string) (*javaactions.JavaAction, error) {
74+
return &javaactions.JavaAction{
75+
ReturnType: &javaactions.EntityType{Entity: "Orders.Order"},
76+
}, nil
77+
},
78+
}
79+
80+
fb := &flowBuilder{
81+
backend: backend,
82+
varTypes: map[string]string{},
83+
declaredVars: map[string]string{},
84+
measurer: &layoutMeasurer{},
85+
}
86+
87+
fb.addCallJavaActionAction(&ast.CallJavaActionStmt{
88+
OutputVariable: "CreatedOrder",
89+
ActionName: ast.QualifiedName{Module: "Orders", Name: "CreateFromPayload"},
90+
})
91+
92+
if got := fb.varTypes["CreatedOrder"]; got != "Orders.Order" {
93+
t.Fatalf("CreatedOrder type = %q, want Orders.Order", got)
94+
}
95+
}
96+
6697
func TestAddJavaAction_GenericListReturnInheritsInputListType(t *testing.T) {
6798
backend := &mock.MockBackend{
6899
ReadJavaActionByNameFunc: func(qualifiedName string) (*javaactions.JavaAction, error) {
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
3+
package executor
4+
5+
import (
6+
"testing"
7+
8+
"github.com/mendixlabs/mxcli/sdk/microflows"
9+
)
10+
11+
// TestHasExplicitFalseBranchAnchor pins ako's review follow-up for PR #364:
12+
// the heuristic that decides whether a false-branch flow carries an explicit
13+
// anchor annotation should fire only on the AnchorTop→AnchorBottom pair and
14+
// must stay dormant for nil flows, builder defaults, and any other side
15+
// combination.
16+
func TestHasExplicitFalseBranchAnchor(t *testing.T) {
17+
tests := []struct {
18+
name string
19+
flow *microflows.SequenceFlow
20+
want bool
21+
}{
22+
{name: "nil flow", flow: nil, want: false},
23+
{
24+
name: "builder default right→left",
25+
flow: &microflows.SequenceFlow{OriginConnectionIndex: AnchorRight, DestinationConnectionIndex: AnchorLeft},
26+
want: false,
27+
},
28+
{
29+
name: "split default bottom→top",
30+
flow: &microflows.SequenceFlow{OriginConnectionIndex: AnchorBottom, DestinationConnectionIndex: AnchorTop},
31+
want: false,
32+
},
33+
{
34+
name: "authored top→bottom",
35+
flow: &microflows.SequenceFlow{OriginConnectionIndex: AnchorTop, DestinationConnectionIndex: AnchorBottom},
36+
want: true,
37+
},
38+
{
39+
name: "only origin customised",
40+
flow: &microflows.SequenceFlow{OriginConnectionIndex: AnchorTop, DestinationConnectionIndex: AnchorTop},
41+
want: false,
42+
},
43+
{
44+
name: "only destination customised",
45+
flow: &microflows.SequenceFlow{OriginConnectionIndex: AnchorBottom, DestinationConnectionIndex: AnchorBottom},
46+
want: false,
47+
},
48+
}
49+
50+
for _, tc := range tests {
51+
t.Run(tc.name, func(t *testing.T) {
52+
got := hasExplicitFalseBranchAnchor(tc.flow)
53+
if got != tc.want {
54+
t.Fatalf("hasExplicitFalseBranchAnchor(%s) = %v, want %v", tc.name, got, tc.want)
55+
}
56+
})
57+
}
58+
}

mdl/executor/cmd_microflows_show_helpers.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1436,12 +1436,16 @@ func branchFlowStartsAtTerminal(flow *microflows.SequenceFlow, activityMap map[m
14361436
}
14371437
}
14381438

1439-
// hasExplicitFalseBranchAnchor reports whether a flow is the false-branch anchor
1440-
// of a boolean ExclusiveSplit (origin=top, destination=bottom). This heuristic
1441-
// distinguishes boolean false-branch flows from enum-split flows inside isGuard,
1442-
// because both share a nil CaseValue but differ in their anchor positions.
1443-
// Limitation: a custom @anchor that happens to use (top, bottom) would be
1444-
// misclassified; this is accepted because it is an unlikely user choice.
1439+
// hasExplicitFalseBranchAnchor reports whether a false-branch sequence flow
1440+
// carries anchor metadata that the user explicitly authored. Top→Bottom is
1441+
// the non-default pair produced by `@anchor(false: (from: top, to: bottom))`;
1442+
// any other combination is either a builder default or a different author
1443+
// intention that should not trigger the guard-pattern describer.
1444+
//
1445+
// Used by the `isGuard` paths in traverseFlow / traverseFlowUntilMerge to
1446+
// distinguish a real guard continuation from a branch whose layout should
1447+
// stay visible as an explicit `else` in the described MDL. See
1448+
// TestHasExplicitFalseBranchAnchor for the exhaustive cases.
14451449
func hasExplicitFalseBranchAnchor(flow *microflows.SequenceFlow) bool {
14461450
if flow == nil {
14471451
return false

0 commit comments

Comments
 (0)