From 01c3a846e40665af9f3e6cd1aad526b8d2f058e5 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:38:43 +0000 Subject: [PATCH 01/13] fix: a restated if outweighs a new one whose condition differs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create or modify` pairing a new `if` and a restated one against the same stored decision weighed "the same if but for its condition" exactly like "the same if with a change in a branch", and lcsStatements breaks a tie toward the earlier pair. So in if $Out then …; end if; -- new, no @position @position(-299, 200) if $Flag then …; end if; -- restated, branch edited the new `if $Out` took over the stored decision (its condition set in place) and the restated `if $Flag` was inserted as a new node at its stated position, on top of it: two decisions at (-299, 200), both merges at (186, 200). The weights are doubled and a changed condition now weighs 1, below every other partial match, so the restated if pairs with its stored decision and the new one is an insert. Every other ratio is unchanged. Part of ako/mxcli#1508 case 6 (mendixlabs/mxcli#1598). Co-Authored-By: Claude Opus 5.5 --- mdl/executor/cmd_flow_modify.go | 30 +++++++++++++++-------- mdl/executor/flow_declared_match_test.go | 31 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index e23c0989b1..3648906d85 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -1103,14 +1103,16 @@ func (pd *patchDiff) conditionEdit(declared, stored *ast.IfStmt) error { // insert (#859). A loop is paired this way only when its body can be edited in // place (editableLoopBody, ako/mxcli#1106); any other change inside a loop is // refused either way. -// -// An `if` whose condition changes is paired the same way (ako/mxcli#888): the -// condition is set on the stored decision, and a change around it must not -// merge with it into a replace of the decision. func sameBlockShell(declared, stored ast.MicroflowStatement) bool { - if _, _, ok := sameIfShell(declared, stored); ok { - return true - } + _, _, ok := sameIfShell(declared, stored) + return ok +} + +// conditionChanged reports whether two statements are the same `if` but for +// its condition (ifConditionChanged). Such a pair is matched as a pair too +// (ako/mxcli#888): the condition is set on the stored decision, and a change +// around it must not merge with it into a replace of the decision. +func conditionChanged(declared, stored ast.MicroflowStatement) bool { _, _, ok := ifConditionChanged(declared, stored) return ok } @@ -1450,6 +1452,12 @@ func insertAnchor(loc *storedLocator, stored []ast.MicroflowStatement, gapStart, // it too — it is that node, moved (ako/mxcli#818) — but an exact match weighs // more, so of two alike statements the one the script left in place keeps its // node. +// +// An `if` whose condition changes weighs least: of a new `if` and a restated +// one before the same stored decision, the restated one — the same condition +// — is that decision. Weighed alike, the tie went to the earlier pair: the new +// if took the stored decision over and the restated one was inserted as a new +// node, drawn on top of it (mendixlabs/mxcli#1598, case 6). func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { n, m := len(declared), len(stored) w := make([][]int, n) @@ -1458,14 +1466,16 @@ func lcsStatements(declared, stored []ast.MicroflowStatement) [][2]int { for j := range stored { switch { case declaredMatches(declared[i], stored[j]): - w[i][j] = 2 + w[i][j] = 4 case sameExceptPositions(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case sameBlockShell(declared[i], stored[j]): - w[i][j] = 1 + w[i][j] = 2 case editableLoopBody(declared[i], stored[j]): // A loop edited in place inside is that loop: a change beside // it must not merge with it into a replace (ako/mxcli#1106). + w[i][j] = 2 + case conditionChanged(declared[i], stored[j]): w[i][j] = 1 } } diff --git a/mdl/executor/flow_declared_match_test.go b/mdl/executor/flow_declared_match_test.go index 4e0882af7d..87b777ed5a 100644 --- a/mdl/executor/flow_declared_match_test.go +++ b/mdl/executor/flow_declared_match_test.go @@ -169,6 +169,37 @@ func TestLCSStatements_ExactMatchWinsOverAMove(t *testing.T) { } } +// mendixlabs/mxcli#1598, case 6: a new `if` inserted before a stored one that +// is restated with a change in its branch. The new if is the stored one but +// for its condition, and the restated if is the stored one but for its +// branch; both counted the same, and the tie went to the earlier pair, so the +// new if took over the stored decision (its condition set in place) and the +// restated if was inserted as a new node — on top of it, at the position the +// script states. The same condition is the closer match. +func TestLCSStatements_TheSameConditionWinsOverAChangedOne(t *testing.T) { + stored := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'a'; + end if;`).Body + declared := parseFlowBody(t, ` @position(-500, 200) + log info node 'N' 'first'; + if $In = 'y' then + log info node 'N' 'new'; + end if; + @position(-299, 200) + if $In = 'x' then + @position(-124, 100) + log info node 'N' 'b'; + end if;`).Body + pairs := lcsStatements(declared, stored) + if len(pairs) != 2 || pairs[0] != [2]int{0, 0} || pairs[1] != [2]int{2, 1} { + t.Fatalf("pairs %v, want [[0 0] [2 1]]: the restated if paired with the stored one, the new if inserted", pairs) + } +} + // mendixlabs/mxcli#1505: Studio Pro stores a multi-line note with CRLF line // breaks and describe prints those bytes as they are. An editor that turns // them into LF changed nothing anyone can see on the canvas, yet the free From a4ed9b61a5180dbe0457adb84c305fc991fc7e5d Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:38:56 +0000 Subject: [PATCH 02/13] fix: create or modify does not move a node the script places (mdl 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `create or modify` that inserts statements without @position into a flow whose later statements state theirs made room by moving everything past the gap along. The stored flow then said something other than the script: in ako/mxcli#1508 case 6 `if $Flag` went -299 -> 40, its merge 186 -> 525 and the return 530 -> 869, while the calls replaced in its branch — placed at their own stated positions, never shifted — stayed at (-124, 100) and (60, 100), left of their decision. Re-running the script was refused (the @merge no longer matched), and shifting "consistently" cannot fix that: the next run must move the nodes back. Under mdl 1 every point the script states a node at — @position, @merge, @start, a note's position:, a parameter's @position; loop bodies skipped, their coordinates are relative — is pinned on the mutator (MicroflowMutator.Pin), and a make-room shift (an insert, or a replace longer than what it replaces) that would move a pinned node is refused, naming the room it needs and the node in the way, with the two ways out: state @position on the new statements, or leave the positions after them unstated so they can move along. Pinning is by point, so a node a placed fragment puts at its stated position is pinned too. mdl 0 is unchanged (ADR-0011): there the refusal would fall back to the whole-flow rebuild, losing the merges, curves and $IDs the shift keeps — TestFlowModify_InsertIsSpliced, ADR-0012's headerless describe+insert acceptance test, caught exactly that. Fixes ako/mxcli#1508 case 6 (mendixlabs/mxcli#1598). Co-Authored-By: Claude Opus 5.5 --- ...e-shift-leaves-placed-fragment-behind.json | 1 + .../skills/mendix/write-microflows/SKILL.md | 2 +- .../write-microflows/reference/pitfalls.md | 16 ++ .../skills/mendix/write-nanoflows/SKILL.md | 35 +-- docs-wiki/bug-patterns/flow-graph-geometry.md | 9 + ...try-1508-splice-keeps-stated-positions.mdl | 110 ++++++++ mdl/backend/mfmutator/splice.go | 60 ++++- mdl/backend/mfmutator/splice_pin_test.go | 115 ++++++++ mdl/backend/microflow_mutation.go | 6 + mdl/executor/cmd_flow_modify.go | 11 +- mdl/executor/cmd_flow_modify_patch.go | 82 +++++- .../flow_modify_stated_positions_1508_test.go | 249 ++++++++++++++++++ 12 files changed, 669 insertions(+), 27 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl create mode 100644 mdl/backend/mfmutator/splice_pin_test.go create mode 100644 mdl/executor/flow_modify_stated_positions_1508_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json new file mode 100644 index 0000000000..b3a9cdf987 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-leaves-placed-fragment-behind.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "create or modify of a nanoflow drawn with @position on every statement, adding an if/else after the declare without @position and editing the two calls in the next if's branch: `spliced: 1 inserted, 1 replaced, 1 dropped`, with `if $Flag` -299 -> 40, its merge 186 -> 525 and the return 530 -> 869, while the replaced calls stayed at (-124, 100)/(60, 100), left of their own decision. Re-running the same script was refused (`the connectors of the If at (40, 200) are redrawn`). With a plain if (no else) the new if took over the stored decision and the restated `if $Flag` was drawn on top of it at (-299, 200), both merges at (186, 200).", "cause": "Three rules that each held alone. makeRoom shifts every top-level node past the cut by the deficit (339); a Replace whose first statement states @position is Placed and written at its stated coordinates, never shifted; movedNode records a move only when the stated position differs from the stored one, so the shifted nodes were never moved back, and the merge is not a move candidate at all. On re-run @merge is geometry but not a position field, so the pair fell to `redrawn`. No-else: lcsStatements weighed ifConditionChanged the same as sameIfShell and the tie went to the earlier pair, so the new if became a condition edit of the stored decision and the restated if a Placed insert, which never runs checkRoom.", "file": "mdl/backend/mfmutator/splice.go (Pin, pinnedPast, makeRoom, Replace), mdl/backend/microflow_mutation.go (MicroflowMutator.Pin), mdl/executor/cmd_flow_modify_patch.go (statedPoints, patch), mdl/executor/cmd_flow_modify.go (lcsStatements weights, mdl 1 gate in planFlowModify)", "insight": "Shift-consistently cannot satisfy idempotence: once the script states -299 and the store says 40, the re-run must move it back or refuse — measured even for a plain one-activity insert into a described flow (+104, then the re-run refused on @merge). So a node at a stated point is pinned and a shift that would move one is refused, naming the room and the node; the mutator pins by point rather than $ID because the node at a stated point may be one a later Placed fragment puts there. Collect stated points from @position, @merge, @start, note and parameter positions, skipping loop bodies (relative coordinates). The control is the same script with the positions after the insert left out: spliced, everything moved by one delta, re-run Unchanged. Gate the pins on mdl 1 (flowRebuildRefused.Applies): under mdl 0 the refusal falls back to the whole-flow rebuild, which deletes the Studio Pro merges and $IDs the shift kept — TestFlowModify_InsertIsSpliced (ADR-0012's describe+insert acceptance, headerless) caught exactly that, and ADR-0011 forbids the change of meaning. Not touched: the shift still moves parameters (y 53) along with the row.", "refs": ["ako/mxcli#1508", "mendixlabs/mxcli#1598"]} diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index af58d4d850..4cf24d5edd 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -69,7 +69,7 @@ Choose the mode by who owns the microflow ([choose-edit-mode](../choose-edit-mod (targets from `describe microflow X with handles`). `create or modify` of `describe` output patches too: unchanged writes nothing; a statement (even one whose handler returns), guard clause, `return` value, `if` condition, header clause, parameter (added/retyped; removed only if unused) or stated `@position`/`@start` change is - patched in place (a move keeps the node's flows), as is a loop-body activity with new values. A redrawn + patched in place (a move keeps the node's flows), as is a loop-body activity with new values ([an insert needing room past a stated `@position`](reference/pitfalls.md#13-inserting-where-the-script-states-every-position)). A redrawn `@anchor`/`@curve`, other loop-body change, error handler or other `return` added/taken away rebuilds under mdl 0 (`MDL-V1-REBUILD`: IDs renumbered, merges and curves lost) and is refused under `mdl 1;`. **To add, remove or reorder in a loop body, `alter … replace` the whole loop** ([pitfalls](reference/pitfalls.md#11-changing-something-inside-a-loop-body)). **To rebuild deliberately, `drop` and `create` in ONE script** — the grants carry only within it ([pitfalls](reference/pitfalls.md#drop--create-is-still-a-new-document)). diff --git a/.claude/skills/mendix/write-microflows/reference/pitfalls.md b/.claude/skills/mendix/write-microflows/reference/pitfalls.md index b1df1bbddc..1848349607 100644 --- a/.claude/skills/mendix/write-microflows/reference/pitfalls.md +++ b/.claude/skills/mendix/write-microflows/reference/pitfalls.md @@ -376,6 +376,22 @@ alter microflow MyModule.GetCode { }; ``` +### 13. Inserting Where the Script States Every Position + +`describe` output states `@position` on every statement. Under `mdl 1;`, `create or +modify` never moves a node the script draws at a stated position: a statement inserted +**without** `@position` into a gap too small for it is refused, naming the room it needs +and the node in the way (mendixlabs/mxcli#1598). Moving the rest of the row along would +leave the stored flow saying something other than the script, so the next run would move +it back. Either: + +- give the new statements an `@position` where there is room for them, or +- leave the positions of what comes after them unstated (`@position`, `@merge`, a note's + `position:`), so the splice can move it along — the second run of that script is then + Unchanged. + +A headerless (mdl 0) script still moves the row along, as it always did. + ## Implicit Variable Creation (CE0111 Duplicate Variable) These statements **implicitly create a new variable** with the name on the left side: diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index 3aadc22379..6740be6af0 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -11,21 +11,21 @@ description: "Nanoflow syntax in MDL — shared with microflows, but client-side | lines | section | |---|---| | 35-44 | When to Use This Skill | -| 45-78 | Changing an Existing Nanoflow | -| 79-97 | When to Use a Nanoflow vs a Microflow | -| 98-112 | Key Differences from Microflows | -| 113-136 | Nanoflow Structure | -| 137-145 | Naming Convention | -| 146-214 | Supported Activities | -| 215-481 | Real-World Patterns | -| 482-552 | NanoflowCommons JavaScript Actions Reference | -| 553-591 | Disallowed Activities | -| 592-595 | Return Type Restrictions | -| 596-640 | Error Handling in Nanoflows | -| 641-648 | Security (GRANT/REVOKE) | -| 649-661 | Management Commands | -| 662-672 | Common Mistakes | -| 673-687 | Validation Checklist | +| 45-81 | Changing an Existing Nanoflow | +| 82-100 | When to Use a Nanoflow vs a Microflow | +| 101-115 | Key Differences from Microflows | +| 116-139 | Nanoflow Structure | +| 140-148 | Naming Convention | +| 149-217 | Supported Activities | +| 218-484 | Real-World Patterns | +| 485-555 | NanoflowCommons JavaScript Actions Reference | +| 556-594 | Disallowed Activities | +| 595-598 | Return Type Restrictions | +| 599-643 | Error Handling in Nanoflows | +| 644-651 | Security (GRANT/REVOKE) | +| 652-664 | Management Commands | +| 665-675 | Common Mistakes | +| 676-690 | Validation Checklist | # Mendix Nanoflow Skill @@ -58,7 +58,10 @@ Choose the mode by who owns the nanoflow ([choose-edit-mode](../choose-edit-mode the stored end event or decision, and a body without a trailing `return` means the stored end. The header (documentation, return type, parameters added, retyped or — when unused — removed) is set on the stored document, and a stated `@position` or `@start` moves the - stored node, keeping its flows. A change it cannot patch — anything inside a loop body + stored node, keeping its flows. Under `mdl 1;` a node at a stated position is never moved + to make room: a statement inserted without `@position` where the gap is too small is refused, naming + the node in the way — give the new statements an `@position`, or leave `@position` and + `@merge` off the statements after them. A change it cannot patch — anything inside a loop body or error handler, a redrawn `@anchor`/`@curve`, any other `return` added or taken away — rebuilds the whole nanoflow under mdl 0 (warning `MDL-V1-REBUILD`: element IDs renumbered, merges removed, curves reset) and diff --git a/docs-wiki/bug-patterns/flow-graph-geometry.md b/docs-wiki/bug-patterns/flow-graph-geometry.md index 5303e9f98c..3a1d12a62b 100644 --- a/docs-wiki/bug-patterns/flow-graph-geometry.md +++ b/docs-wiki/bug-patterns/flow-graph-geometry.md @@ -94,6 +94,15 @@ translated into the gap; anything that is not on the line (a note) must not size the gap, and anything the script placed absolutely must not be translated. +**Making room is a move the script did not ask for.** Shifting the row after +a gap is consistent only while nothing past the gap has a stated position: +once the script says -299 and the store says 40, the next run must move it back +or refuse, so "shift everything consistently" can never re-run Unchanged. A +node at a stated point is pinned instead, and a shift that would move one is +refused with the room it needs — under mdl 1 only, because under mdl 0 a +refusal becomes a whole-flow rebuild, which loses more than the shift did. +Check a splice fix by re-running the same script, not by describing once. + ## See also - [fix-issue findings](../../.claude/skills/fix-issue/findings/) — the formulas, diff --git a/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl new file mode 100644 index 0000000000..3d7471644d --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1508-splice-keeps-stated-positions.mdl @@ -0,0 +1,110 @@ +mdl 1; +-- ============================================================================ +-- Bug: an inserted if moved every stated node after it, but not the +-- statements replaced in its neighbour's branch +-- (ako/mxcli#1508, case 6; mendixlabs/mxcli#1598) +-- ============================================================================ +-- +-- NF_Do is drawn with a @position on every statement. The reported +-- restatement (below, commented out) keeps every @position, adds an if/else +-- after the declare without one, changes the JS argument and binds the call's +-- result. Before the fix it was spliced in as +-- +-- Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +-- +-- with `if $Flag` moved -299 -> 40, its merge 186 -> 525 and the return +-- 530 -> 869 to make room, while the two replaced calls stayed at the +-- positions the script states, (-124, 100) and (60, 100): left of their own +-- decision. Re-running the script was refused ("the connectors of the If at +-- (40, 200) are redrawn"). With a plain `if` (no else) the new if took over the +-- stored decision and the restated `if $Flag` was drawn on top of it. +-- +-- Fixed: under mdl 1 a node the script draws at a stated position is not +-- moved to make room. The reported restatement is refused, naming the room it +-- needs and what is in the way (a headerless script still makes room as +-- before: under mdl 0 the refusal would fall back to a whole-flow rebuild, +-- ADR-0011): +-- +-- the inserted statements need 339px more room than there is between the +-- ActionActivity at (-500, 200) and the ExclusiveSplit '$Flag' at +-- (-299, 200), which the script draws where it is; state @position on the +-- new statements, where there is room for them, or leave unstated the +-- positions of what comes after them (@position, @merge, a note's +-- position:) so that it can move along. +-- +-- The restatement that leaves those positions unstated (the last statement of +-- this file) is spliced: everything after the declare moves along by the same +-- amount, the replaced calls with their decision. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe nanoflow MyFirstModule.NF_Do" +-- Expected: both calls are drawn right of `if $Flag` and left of its merge, +-- and the last statement, run again on its own, reports +-- Unchanged nanoflow: MyFirstModule.NF_Do +-- Uncomment the reported restatement to see the refusal. +-- ============================================================================ + +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; + +create or modify microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; + +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; + +-- The reported restatement: refused, nothing written. +-- create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +-- begin +-- @position(-500, 200) +-- declare $Out Boolean = false; +-- if $Out then +-- log info node 'Probe' 'x'; +-- else +-- log info node 'Probe' 'y'; +-- end if; +-- @position(-299, 200) +-- @merge(186, 200) +-- if $Flag then +-- @position(-124, 100) +-- call javascript action MyFirstModule.JS_Do(Msg = 'b'); +-- @position(60, 100) +-- $Ok = call microflow MyFirstModule.SUB_Check(); +-- end if; +-- @position(530, 200) +-- return $Out; +-- end; + +-- The same change with the positions after the insert left unstated: spliced. +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + if $Flag then + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + return $Out; +end; diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index b00376e009..b28ff3e354 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -69,6 +69,8 @@ type Mutator struct { // removed holds the $IDs of every element an operation took out, so Save // can prove that nothing still points at one of them. removed map[string]bool + // pinned are the points the script states a node is drawn at (Pin). + pinned map[point]bool } var _ backend.MicroflowMutator = (*Mutator)(nil) @@ -341,7 +343,16 @@ func (m *Mutator) makeRoom(g *graph, fb *fragmentBox, f flowRef, x, y *node) (en b := y.pos.get(ax) - s*y.size.get(ax)/2 need := fb.length(ax) + 2*minGap if deficit := need - s*(b-a); deficit > 0 { - m.shift(g, ax, s, float64(a+b)/2, s*deficit, "") + cut := float64(a+b) / 2 + if p := m.pinnedPast(g, ax, s, cut, ""); p != nil { + blocked := ", and making it would move " + describeNode(p) + "," + if p == y { + blocked = "," + } + return 0, 0, fmt.Errorf("the inserted statements need %dpx more room than there is between %s and %s%s "+ + "which the script draws where it is; %s", deficit, describeNode(x), describeNode(y), blocked, roomAdvice) + } + m.shift(g, ax, s, cut, s*deficit, "") b += s * deficit } @@ -422,7 +433,12 @@ func (m *Mutator) Replace(target model.ID, frag *backend.MicroflowFragment) erro // past target moves along by the difference. near := x.pos.get(ax) - s*x.size.get(ax)/2 if extra := fb.length(ax) - x.size.get(ax); extra > 0 { - m.shift(g, ax, s, float64(x.pos.get(ax)), s*extra, x.id) + cut := float64(x.pos.get(ax)) + if p := m.pinnedPast(g, ax, s, cut, x.id); p != nil { + return fmt.Errorf("the replacing statements are %dpx longer than %s, and making room for them would move %s, "+ + "which the script draws where it is; %s", extra, describeNode(x), describeNode(p), roomAdvice) + } + m.shift(g, ax, s, cut, s*extra, x.id) } centre := point{} centre = centre.set(ax, near+s*fb.length(ax)/2) @@ -700,6 +716,17 @@ func (m *Mutator) Move(target model.ID, to model.Point) error { return nil } +// Pin records that the script states a node is drawn at at +// (mendixlabs/mxcli#1598, case 6). A point rather than a node: the node there +// may be one a later fragment places, at the position its statement states. +func (m *Mutator) Pin(at model.Point) error { + if m.pinned == nil { + m.pinned = map[point]bool{} + } + m.pinned[point{at.X, at.Y}] = true + return nil +} + // bodyFlows returns the flows that run inside loop's body, at any depth. They // are stored in the unit's Flows list, not in the loop, so taking the loop out // has to take them too; left behind they would point at removed objects. @@ -860,10 +887,7 @@ func (m *Mutator) markRemoved(v any) { // so no curve changes. skip is a node that is about to be removed. func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { for _, n := range g.order { - if n.loop != "" || n.id == skip { - continue - } - if float64(s)*(float64(n.pos.get(ax))-cut) <= 0 { + if !shifted(n, ax, s, cut, skip) { continue } n.pos = n.pos.set(ax, n.pos.get(ax)+delta) @@ -871,6 +895,30 @@ func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip s } } +// shifted reports whether shift moves n: a top-level node past the cut. +func shifted(n *node, ax axis, s int, cut float64, skip string) bool { + return n.loop == "" && n.id != skip && float64(s)*(float64(n.pos.get(ax))-cut) > 0 +} + +// pinnedPast returns the first node shift would move that is drawn at a point +// the script states (Pin), or nil. Moving it would make the stored flow say +// something other than the script — a later fragment placed at its own stated +// position would be left behind on the wrong side of a node that moved, and +// re-running the script would move the node back (mendixlabs/mxcli#1598, +// case 6) — so the caller refuses instead. +func (m *Mutator) pinnedPast(g *graph, ax axis, s int, cut float64, skip string) *node { + for _, n := range g.order { + if m.pinned[n.pos] && shifted(n, ax, s, cut, skip) { + return n + } + } + return nil +} + +// roomAdvice is the way out of a refused shift. +const roomAdvice = "state @position on the new statements, where there is room for them, or leave unstated the positions " + + "of what comes after them (@position, @merge, a note's position:) so that it can move along" + // checkIntegrity is the rule-1 guard: no binary anywhere in the unit may // still name an element that was removed, and no two elements may share an // $ID. Either would make the document unopenable, and neither is caught by diff --git a/mdl/backend/mfmutator/splice_pin_test.go b/mdl/backend/mfmutator/splice_pin_test.go new file mode 100644 index 0000000000..b8728bba8d --- /dev/null +++ b/mdl/backend/mfmutator/splice_pin_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// A node whose position the script states is not moved to make room +// (mendixlabs/mxcli#1598, case 6). The splice moved every node past the gap +// along by what the fragment lacked, including ones the script draws at a +// stated @position — so the stored flow no longer said what the script said, +// a fragment placed at its own stated position was left behind on the wrong +// side of its decision, and re-running the same script moved the nodes back +// (or was refused). Such a shift is refused now, saying which node is in the +// way; the controls show the same splice still makes room when nothing past +// the gap is pinned. +func TestSplice_MakingRoomRefusesToMoveAPinnedNode(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + // b at (420, 200) sits 50px right of a; a 120px activity needs 200. + t.Run("an insert, the node after the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 420, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "the ActionActivity at (420, 200)") || + !strings.Contains(err.Error(), "150px") { + t.Fatalf("got %v, want a refusal naming the pinned activity at (420, 200) and the 150px lacking", err) + } + if p := positions(m)[uid("b")]; p.X != 420 { + t.Errorf("b moved to x=%d before the refusal", p.X) + } + }) + t.Run("control: nothing pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatal(err) + } + if p := positions(m)[uid("b")]; p.X != 570 { + t.Errorf("b is at x=%d, want it moved along by 150 to 570", p.X) + } + }) + t.Run("control: only nodes before the gap pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + for _, p := range []model.Point{{X: 100, Y: 200}, {X: 250, Y: 200}} { + if err := m.Pin(p); err != nil { + t.Fatal(err) + } + } + if err := m.InsertAfter(model.ID(uid("a")), oneActivity()); err != nil { + t.Fatalf("a pin before the gap refused the insert: %v", err) + } + }) + t.Run("a replace longer than what it replaces, the end pinned", func(t *testing.T) { + m, _ := newMutator(t, unit(line(), lineFlows())) + if err := m.Pin(model.Point{X: 600, Y: 200}); err != nil { + t.Fatal(err) + } + err := m.Replace(model.ID(uid("a")), twoActivities()) + if err == nil || !strings.Contains(err.Error(), "the EndEvent at (600, 200)") { + t.Fatalf("got %v, want a refusal naming the pinned end event", err) + } + }) + t.Run("an earlier fragment drawn at a stated point", func(t *testing.T) { + // The script states where the first fragment goes, so it is pinned + // like a stored node: a later insert cannot slide it along. + m, _ := newMutator(t, unit(line(), lineFlows())) + placed := oneActivity() + placed.Placed = true + placed.Objects[0].SetPosition(model.Point{X: 600, Y: 400}) + if err := m.Pin(model.Point{X: 600, Y: 400}); err != nil { + t.Fatal(err) + } + if err := m.InsertAfter(model.ID(uid("b")), placed); err != nil { + t.Fatal(err) + } + err := m.InsertAfter(model.ID(uid("a")), oneActivity()) + if err == nil || !strings.Contains(err.Error(), "(600, 400)") { + t.Fatalf("got %v, want a refusal naming the placed activity at (600, 400)", err) + } + }) +} + +// twoActivities is a two-activity fragment in builder coordinates, 290px long. +func twoActivities() *backend.MicroflowFragment { + act := func(x int) *microflows.ActionActivity { + return µflows.ActionActivity{BaseActivity: microflows.BaseActivity{BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Position: model.Point{X: x, Y: 200}, + Size: model.Size{Width: 120, Height: 60}, + }}} + } + a, b := act(360), act(530) + return &backend.MicroflowFragment{ + Objects: []microflows.MicroflowObject{a, b}, + Flows: []*microflows.SequenceFlow{{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + OriginID: a.ID, DestinationID: b.ID, + }}, + Entry: a.ID, Exit: b.ID, + } +} diff --git a/mdl/backend/microflow_mutation.go b/mdl/backend/microflow_mutation.go index 4217ff5d09..d381554916 100644 --- a/mdl/backend/microflow_mutation.go +++ b/mdl/backend/microflow_mutation.go @@ -66,6 +66,12 @@ type MicroflowMutator interface { // Move sets where the top-level node target is drawn, and nothing else: // its flows keep their ends, sides and curves (ako/mxcli#818). Move(target model.ID, to model.Point) error + // Pin says the script states that a node is drawn at the point at — a + // stored node, or one a fragment places there. An insert or replace that + // would move the node drawn there to make room is refused instead, so the + // stored flow keeps saying what the script says (mendixlabs/mxcli#1598, + // case 6). + Pin(at model.Point) error // SetHeader writes the document properties of declared — a // *microflows.Microflow or *microflows.Nanoflow built from the statement, // with every property it does not state carried from the stored one — onto diff --git a/mdl/executor/cmd_flow_modify.go b/mdl/executor/cmd_flow_modify.go index 3648906d85..d3fd5a958b 100644 --- a/mdl/executor/cmd_flow_modify.go +++ b/mdl/executor/cmd_flow_modify.go @@ -326,9 +326,18 @@ func planFlowModify(ctx *ExecContext, d *flowDecl) (*flowPlan, error) { // a member of one was written bare (ako/mxcli#885). a.declaredVarTypes = varTypes + // A node the script draws at a stated position is pinned under mdl 1 only + // (ADR-0011: a new refusal applies under the version that introduces it). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow, losing the merges, curves and $IDs the shift keeps, so a headerless + // script makes room as it did (mendixlabs/mxcli#1598, case 6). + var stated []model.Point + if flowRebuildRefused.Applies(ctx.LanguageVersion) { + stated = statedPoints(d) + } p := &flowPlan{a: a, ops: ops, moves: moves, storedFolder: storedFolderOf(stored)} if len(ops) > 0 || len(moves) > 0 || headerChanged { - if p.mut, err = patch(ctx, a, ops, targets, moves); err != nil { + if p.mut, err = patch(ctx, a, ops, targets, moves, stated); err != nil { return nil, cannotSplice("%v", err) } if headerChanged { diff --git a/mdl/executor/cmd_flow_modify_patch.go b/mdl/executor/cmd_flow_modify_patch.go index dcd6cf6eaf..908016b05a 100644 --- a/mdl/executor/cmd_flow_modify_patch.go +++ b/mdl/executor/cmd_flow_modify_patch.go @@ -5,6 +5,7 @@ package executor import ( "bytes" "fmt" + "reflect" "strings" "go.mongodb.org/mongo-driver/v2/bson" @@ -32,6 +33,10 @@ import ( // moves that node, and only it: its flows keep their ends, sides and // curves (no reconnection), and nothing around it moves. A position the // script leaves out keeps what is stored, as before; +// - a node the script draws at a stated position stays there: a fragment +// that needs more room than the stated positions leave is refused, rather +// than everything after it moved along, which left the stored flow saying +// something other than the script (mendixlabs/mxcli#1598, case 6); // - two nodes nobody placed follow the node they were derived from, because // mxcli's own layout put them there and nothing else says where they go: // a start event where the layout derives it follows the first statement @@ -56,10 +61,13 @@ const endSpacing = HorizontalSpacing + HorizontalSpacing/2 // patch opens the stored flow and applies the moves and the splice // operations. The moves go first, so a fragment the script places next to a // moved node is joined to it where it now is, and again after the operations, -// so a node an insert shifted to make room is still drawn where the script -// says (a move sets a position, so applying it twice is applying it once). +// so a node no one placed that follows another (followingEnd) is drawn where +// the script says even if an insert shifted it to make room (a move sets a +// position, so applying it twice is applying it once). Between the two, every +// point the script states a node at is pinned, so an operation that would +// have to move one to make room is refused (mendixlabs/mxcli#1598, case 6). func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, targets []mfmutator.Candidate, - moves []flowMove) (backend.MicroflowMutator, error) { + moves []flowMove, stated []model.Point) (backend.MicroflowMutator, error) { mut, err := ctx.Backend.OpenMicroflowForMutation(a.mf.ID) if err != nil { return nil, err @@ -75,6 +83,11 @@ func patch(ctx *ExecContext, a *alterFlowContext, ops []*ast.AlterFlowOperation, if err := move(); err != nil { return nil, err } + for _, p := range stated { + if err := mut.Pin(p); err != nil { + return nil, err + } + } if err := a.applyTo(ctx, mut, ops, targets); err != nil { return nil, err } @@ -138,6 +151,69 @@ func (pd *patchDiff) movedNode(d, s ast.MicroflowStatement) error { return nil } +// statedPoints are the canvas points the declared flow states a node is drawn +// at: each statement's @position, an if's @merge, the @start, every note's +// position and every parameter's. The splice pins whatever is drawn at one, so +// that making room for a fragment does not move a node away from where the +// script draws it (mendixlabs/mxcli#1598, case 6). A loop body is left out: +// its positions are relative to the loop box, and the splice moves no node +// inside one. +func statedPoints(d *flowDecl) []model.Point { + var out []model.Point + add := func(p *ast.Position) { + if p != nil { + out = append(out, model.Point{X: p.X, Y: p.Y}) + } + } + for _, p := range d.params { + add(p.Position) + } + var walk func(rv reflect.Value) + walk = func(rv reflect.Value) { + switch rv.Kind() { + case reflect.Interface: + if !rv.IsNil() { + walk(rv.Elem()) + } + case reflect.Pointer: + if rv.IsNil() { + return + } + switch v := rv.Interface().(type) { + case *ast.ActivityAnnotations: + add(v.Position) + add(v.Merge) + add(v.Start) + for _, notes := range [][]ast.MicroflowAnnotation{v.Notes, v.FreeNotes} { + for _, n := range notes { + add(n.Position) + } + } + return + case *ast.LoopStmt: + walk(reflect.ValueOf(v.Annotations)) + return + case *ast.WhileStmt: + walk(reflect.ValueOf(v.Annotations)) + return + } + walk(rv.Elem()) + case reflect.Struct: + for i := 0; i < rv.NumField(); i++ { + if rv.Type().Field(i).IsExported() { + walk(rv.Field(i)) + } + } + case reflect.Slice, reflect.Array: + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i)) + } + } + } + walk(reflect.ValueOf(d.body)) + return out +} + // redrawn is the refusal for a statement whose connectors the script draws // differently: the patch moves nodes, and does not reroute flows. func redrawn(st ast.MicroflowStatement) error { diff --git a/mdl/executor/flow_modify_stated_positions_1508_test.go b/mdl/executor/flow_modify_stated_positions_1508_test.go new file mode 100644 index 0000000000..bb4a3cf40d --- /dev/null +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -0,0 +1,249 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// mendixlabs/mxcli#1598 (ako/mxcli#1508), case 6, on a real project write. +// +// The flow is drawn with a @position on every statement. Restated with a new +// if/else after the declare and no @position on it, the splice made room by +// moving everything after the declare along by 339px — while the branch's two +// calls, replaced at the positions the script states, stayed where they were, +// left of their own decision: +// +// Modified nanoflow: MyFirstModule.NF_Do (spliced: 1 inserted, 1 replaced, 1 dropped) +// @position(40, 200) if $Flag was -299 +// @merge(525, 200) its merge was 186 +// @position(-124, 100) call javascript action … +// @position(869, 200) return was 530 +// +// and re-running the same script was refused ("the connectors of the If at +// (40, 200) are redrawn"). The same script with a plain `if` and no else put +// the new if on top of the stored decision. Either way the stored flow said +// something other than the script, so under mdl 1 the splice now refuses to +// move a node the script states, and the script that leaves those positions +// unstated is the control: it is applied, and its re-run is Unchanged. + +const nf1508Setup = `mdl 1; +create javascript action MyFirstModule.JS_Do(Msg: String) returns Boolean platform web as $$ +return Promise.resolve(true); +$$; +create microflow MyFirstModule.SUB_Check() returns Boolean as $R +begin + declare $R Boolean = true; + return $R; +end; +create nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'a'); + @position(60, 100) + call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +// nf1508Modify is the reported restatement: every @position kept, an if/else +// added after the declare without one, the JS argument changed and the call's +// result bound. +const nf1508Modify = `mdl 1; +create or modify nanoflow MyFirstModule.NF_Do ($Flag: Boolean) returns Boolean as $Out +begin + @position(-500, 200) + declare $Out Boolean = false; + if $Out then + log info node 'Probe' 'x'; + else + log info node 'Probe' 'y'; + end if; + @position(-299, 200) + @merge(186, 200) + if $Flag then + @position(-124, 100) + call javascript action MyFirstModule.JS_Do(Msg = 'b'); + @position(60, 100) + $Ok = call microflow MyFirstModule.SUB_Check(); + end if; + @position(530, 200) + return $Out; +end; +` + +func TestCreateOrModify_InsertDoesNotMoveStatedNodes1508(t *testing.T) { + // Each case starts from the flow as the setup draws it. + setup := func(t *testing.T) (*Executor, *bytes.Buffer, func() string) { + t.Helper() + exec, out := openPedAppFixture(t) + if err := agreeExec(t, exec, nf1508Setup); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + describe := func() string { + out.Reset() + if err := afRun(t, exec, "describe nanoflow MyFirstModule.NF_Do;"); err != nil { + return err.Error() + } + return out.String() + } + return exec, out, describe + } + refused := func(t *testing.T, src string) { + exec, out, describe := setup(t) + before := nf1508Unit(t, exec) + out.Reset() + err := agreeExec(t, exec, src) + said := out.String() + if err != nil { + said += err.Error() + } + if err == nil || !strings.Contains(said, "which the script draws where it is") || + !strings.Contains(said, "the ExclusiveSplit '$Flag' at (-299, 200)") { + t.Errorf("want a refusal naming the decision the script draws at (-299, 200), got %v\n%s", err, said) + } + if !bytes.Equal(nf1508Unit(t, exec), before) { + t.Errorf("the refused statement wrote the nanoflow:\n%s", describe()) + } + } + t.Run("the reported if/else", func(t *testing.T) { refused(t, nf1508Modify) }) + // A refusal is new meaning, so it applies under mdl 1 only (ADR-0011). + // Under mdl 0 the refused splice would fall back to rebuilding the whole + // flow — losing the merges, curves and $IDs the shift keeps — so a + // headerless script is spliced as it was. + t.Run("mdl 0 splices as before", func(t *testing.T) { + exec, out, _ := setup(t) + out.Reset() + if err := agreeExec(t, exec, strings.TrimPrefix(nf1508Modify, "mdl 1;\n")); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if said := out.String(); strings.Contains(said, "MDL-V1-REBUILD") || + !strings.Contains(said, "Modified nanoflow: MyFirstModule.NF_Do (spliced:") { + t.Errorf("want the headerless script spliced, not rebuilt, got:\n%s", said) + } + }) + // With no else the new if took over the stored decision (the same if but + // for its condition) and the restated `if $Flag` was inserted on top of it. + t.Run("a plain if", func(t *testing.T) { + refused(t, strings.Replace(nf1508Modify, " else\n log info node 'Probe' 'y';\n", "", 1)) + }) + + // The way out the refusal names: leave the positions after the insert + // unstated, so they can move along. Everything moves by the same amount — + // the replaced calls with their decision — and re-running reports Unchanged. + t.Run("control: no position stated after the insert", func(t *testing.T) { + exec, out, describe := setup(t) + free := nf1508Modify + for _, line := range []string{" @position(-299, 200)\n", " @merge(186, 200)\n", " @position(-124, 100)\n", + " @position(60, 100)\n", " @position(530, 200)\n"} { + free = strings.Replace(free, line, "", 1) + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("%v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Modified nanoflow: MyFirstModule.NF_Do") { + t.Errorf("want the nanoflow modified, got:\n%s", out.String()) + } + v := parseFlowView(t, nf1508Unit(t, exec)) + if v.objectAt(t, -500, 200, "Microflows$ActionActivity") == "" { + t.Errorf("the declare the script draws at (-500, 200) moved:\n%s", describe()) + } + split, merge, branch := nf1508Branch(t, v, "$Flag") + if len(branch) != 2 { + t.Fatalf("`if $Flag` has %d activities in its branch, want 2:\n%s", len(branch), describe()) + } + for _, x := range branch { + if x <= split || x >= merge { + t.Errorf("a branch activity of `if $Flag` is at x=%d, outside its decision (%d) and merge (%d):\n%s", + x, split, merge, describe()) + } + } + out.Reset() + if err := agreeExec(t, exec, free); err != nil { + t.Fatalf("re-run: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Unchanged nanoflow: MyFirstModule.NF_Do") { + t.Errorf("re-run: want Unchanged, got:\n%s", out.String()) + } + }) +} + +// nf1508Unit returns MyFirstModule.NF_Do's stored bytes. +func nf1508Unit(t *testing.T, exec *Executor) []byte { + t.Helper() + ctx := exec.newExecContext(t.Context()) + h, err := getHierarchy(ctx) + if err != nil { + t.Fatal(err) + } + all, err := ctx.Backend.ListNanoflows() + if err != nil { + t.Fatal(err) + } + for _, n := range all { + if n.Name == "NF_Do" && h.GetModuleName(h.FindModuleID(n.ContainerID)) == "MyFirstModule" { + raw, err := ctx.Backend.GetRawUnitBytes(n.ID) + if err != nil { + t.Fatal(err) + } + return append([]byte(nil), raw...) + } + } + t.Fatal("MyFirstModule.NF_Do not found") + return nil +} + +// nf1508Branch returns the x of the decision on cond, of the merge its paths +// join at, and of every activity on those paths. +func nf1508Branch(t *testing.T, v flowView, cond string) (split, merge int, branch []int) { + t.Helper() + var splitID string + for _, id := range v.order { + o := v.objs[id] + if afGet(o, "$Type") != "Microflows$ExclusiveSplit" { + continue + } + if sc, _ := afGet(o, "SplitCondition").(bson.D); sc != nil && afGet(sc, "Expression") == cond { + splitID = id + split, _ = afPoint(o) + } + } + if splitID == "" { + t.Fatalf("no decision on %s", cond) + } + seen := map[string]bool{splitID: true} + for frontier := []string{splitID}; len(frontier) > 0; frontier = frontier[1:] { + for _, f := range v.flows { + dest := afIDOf(afGet(f, "DestinationPointer")) + if afIDOf(afGet(f, "OriginPointer")) != frontier[0] || seen[dest] { + continue + } + seen[dest] = true + o := v.objs[dest] + x, _ := afPoint(o) + switch afGet(o, "$Type") { + case "Microflows$ExclusiveMerge": + merge = x + case "Microflows$ActionActivity": + branch = append(branch, x) + frontier = append(frontier, dest) + } + } + } + if merge == 0 { + t.Fatalf("the decision on %s reaches no merge", cond) + } + return split, merge, branch +} From c4f43e80dc3904c93c3a1266aef6767bed59a431 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 10:40:00 +0000 Subject: [PATCH 03/13] docs: list the stated-position refusal among mdl 1's splice refusals versions.md enumerates what `create or modify` refuses under mdl 1 where mdl 0 rebuilds; add the insert that needs room past a node the script places. The review command gains the pattern that #1508 case 6 hit: a new splice refusal changes mdl 0 into a rebuild unless it is gated on the language version. Co-Authored-By: Claude Opus 5.5 --- .claude/commands/mxcli-dev/review.md | 1 + docs-site/src/language/versions.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/commands/mxcli-dev/review.md b/.claude/commands/mxcli-dev/review.md index 3ef98554b9..76fa4079d5 100644 --- a/.claude/commands/mxcli-dev/review.md +++ b/.claude/commands/mxcli-dev/review.md @@ -52,6 +52,7 @@ proactively. Add a row after every review that surfaces something new. | 21 | A registry that reads only one source (an `embed.FS`) has no extension point, so the only way to customise its output is to hand-edit the generated block — which a digest fence then refuses to touch on the next run. The feature ends up hostile to the exact case it was built for | API design | When a guard refuses a user's edit, ask whether the thing being edited should have been authorable. Introduce a source abstraction (`fs.FS` + root) and let a project-local directory shadow the embedded set — the walk functions usually already take a root, so the change is contained | | 22 | A test helper that expands a template cross-products every list against every reference (e.g. every `@each $weight` list against every `url()`), inventing artefacts no asset ever shipped and failing on correct input | Test coverage | Scan positionally: a reference must be expanded against the loop it actually sits under. Prove the helper both fires on a real break (delete one shipped file) and stays quiet on correct input — a helper only checked against green code has not been shown to detect anything | | 23 | A copy-to-scaffold path renames files but not the identifiers built from the name (`@mixin mxcli--`, `@import "mxcli-"`), so two artefacts collide the moment both exist — and the symptom is a rule that silently compiles to nothing | Code correctness | Assert the structural contract on the *generated* artefact, not just the shipped ones: factor the built-in's contract test into a helper and run the scaffold through it. Verify once end to end against the real toolchain and record it | +| 24 | A new splice refusal routed through the shared `cannotSplice` path changes **mdl 0** too: there a refusal falls back to the whole-flow rebuild, so a headerless script that used to splice now loses its merges, curves and `$ID`s — a silent change of meaning ADR-0011 rules out (#1508 case 6, caught by `TestFlowModify_InsertIsSpliced`) | Language versioning | Gate the new refusal on `flowRebuildRefused.Applies(ctx.LanguageVersion)` so mdl 0 keeps its old splice; add an mdl 0 case asserting "spliced, no MDL-V1-REBUILD" beside the mdl 1 refusal test | | 24 | A stacked PR's base merged at an older head: commits pushed to the base branch after its merge never reach `main`, while both PRs read as fine (ako/mxcli#1108 lost its last two commits this way) | Merge hygiene | Before merging any stacked PR, and after pushing to a branch whose PR may already be merged, check each commit with `git merge-base --is-ancestor origin/main`; retarget the next PR to `main` so it carries the stranded commits, and say so in its description | | 24 | A writer leaves out a property Studio Pro always writes (an empty `Documentation`, the empty half of an attribute/association pair) or writes one Mendix does not declare. `mx check`, the build and the runtime stay green, but Mendix's merge/diff engine compares property NAMES and throws, so the document becomes unmergeable once Studio Pro saves it (mendixlabs/mxcli#1373) | BSON writing | The doctype gate's `checkPropertySets` catches it per script: `mx convert -p` re-serializes what is not canonical. Fix the writer, or record Mendix's value with `MXCLI_PROPERTY_SETS_RECORD` + `internal/propertysets/cmd/propdefaults`; never guess a default | | 24 | A skill or command still instructs work into a package the repo has deleted (`sdk/mpr` test locations, a symptom table moved to `findings/*.jsonl` years prior) — the doc reads as authoritative and every instruction in it is a compile error or a no-op | Docs quality | When a package is deleted or a doc is restructured, grep `.claude/` for its name in the same PR. A deletion that leaves the guidance behind is worse than no guidance, because the reader trusts it | diff --git a/docs-site/src/language/versions.md b/docs-site/src/language/versions.md index 0fa153f21a..01e6dd4a62 100644 --- a/docs-site/src/language/versions.md +++ b/docs-site/src/language/versions.md @@ -196,7 +196,7 @@ A construct with no mechanical rewrite is reported with the reason, and `fmt` re `-p app.mpr` also settles a bare `commit $X;` in a `create or modify microflow|nanoflow`, with or without the header. Since mendixlabs/mxcli#895 a bare commit means *with* events, Studio Pro's default; an older mxcli stored the same statement *without* events. Where the stored flow commits the variable without events, `fmt --upgrade -p app.mpr` writes `commit $X without events;`, so re-running the script keeps what is stored instead of turning the event handlers on (or, inside a loop under `mdl 1`, being refused). A new flow, a new commit, or a stored commit with events is left as written; where the stored flow commits the variable both ways, the statement is left and reported. Without `-p`, `fmt` prints a note (`MDL067`) for each flow with a bare commit. -`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. +`-p app.mpr` also tells the upgrade which statements `exec` would refuse once the header is there. Under `mdl 1` a `create or modify microflow|nanoflow` of a stored flow is applied as a patch, and a change the patch cannot make (inside a loop body anything but an activity replaced by one of the same kind, an error handler, a redrawn connector, a `return` added, a statement inserted without `@position` that needs room past a node the script places) is refused with nothing written; without the header the same statement rebuilds the whole flow (`MDL-V1-REBUILD`). No rewrite keeps that meaning, so `fmt` names each such statement and leaves the header off that file, applying the rest of the upgrade. Change those flows with `alter`, or drop and create them, and upgrade again; `--force-header` adds the header regardless. `mxcli check script.mdl -p app.mpr` reports the same statements (an `MDL-V1-REBUILD` error under `mdl 1`, the warning without the header), computed by the same code as `exec` and `diff`. ## Behaviour changes to know about From 9442043f133f857a012d251f0de24445dc465779 Mon Sep 17 00:00:00 2001 From: Ako Date: Wed, 21 Oct 2026 13:00:34 +0000 Subject: [PATCH 04/13] fix: a splice making room leaves the flow's parameters where they are MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shift moved every top-level node past the cut, and a parameter is a top-level object of the stored object collection: one if/else inserted into NF_Do moved `$Flag` from its derived (200, 53) to (539, 53), a longer replace then to (699, 53), and from then on describe printed a parameter @position no script stated. mxcli's derived parameter row is absolute and right of the start event, so almost every insert's cut falls left of it. A parameter is not on the line the room is made on. Studio Pro authors draw it above the start event — 63 of 71 in PedApp and JTSBootLogboek sit at the start's x, none within 60px of the flow line — and an insert in Studio Pro moves nothing. shifted() now leaves parameters out, and with it pinnedPast, so a stated parameter position no longer blocks a splice it is not in the way of. Fixes ako/mxcli#1547. Co-Authored-By: Claude Opus 5.5 --- ...6-10-21-splice-shift-moves-parameters.json | 1 + ...geometry-1547-splice-leaves-parameters.mdl | 35 +++++++++++++ mdl/backend/mfmutator/splice.go | 19 ++++--- .../mfmutator/splice_parameters_test.go | 50 +++++++++++++++++++ .../flow_modify_stated_positions_1508_test.go | 9 ++++ 5 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json create mode 100644 mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl create mode 100644 mdl/backend/mfmutator/splice_parameters_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json new file mode 100644 index 0000000000..c7daa32352 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-21-splice-shift-moves-parameters.json @@ -0,0 +1 @@ +{"area": "mdl/backend", "date": "2026-10-21", "symptom": "A splice that makes room for an inserted fragment (create or modify / alter … insert) moved the flow's parameters along with the row: one if/else inserted into NF_Do moved $Flag from its derived (200, 53) to (539, 53), a longer replace then to (699, 53), and from then on describe printed a parameter @position no script stated. Inserting before the first statement of a two-parameter microflow moved $Count from (300, 53) to (410, 53).", "cause": "mfmutator shift (splice.go) moves every top-level node past the cut, and a Microflows$MicroflowParameter is a top-level object of the unit's object collection (the decoded model keeps parameters apart, so nothing in the executor sees them as nodes). mxcli's derived parameter row is absolute (200 + idx*pitch, 53), right of the start event, so almost every insert's cut falls left of it.", "file": "mdl/backend/mfmutator/splice.go (shifted)", "insight": "Measure the authored convention before deciding what 'the row' is: 63 of 71 Studio Pro-authored parameters (PedApp 19/20, JTSBootLogboek 44/51) sit exactly at the start event's x, all above every other node, none within 60px of the flow line — a parameter belongs to the flow, not to the stretch of line past an insert, and Studio Pro moves nothing on insert. The decoded model hides this: microflows.Microflow carries Parameters apart from ObjectCollection, so a probe that type-switches the collection for *MicroflowParameter does not compile, while the BSON the mutator walks has them inline. Excluding them in shifted also keeps them out of pinnedPast, so a stated parameter position can no longer block a splice it is not in the way of.", "refs": ["ako/mxcli#1547", "ako/mxcli#1508"]} diff --git a/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl new file mode 100644 index 0000000000..5b6f8b2c9b --- /dev/null +++ b/mdl-examples/bug-tests/flow-geometry-1547-splice-leaves-parameters.mdl @@ -0,0 +1,35 @@ +mdl 1; +-- ============================================================================ +-- Bug: a splice making room moved the flow's parameters along with the row +-- (ako/mxcli#1547) +-- ============================================================================ +-- +-- The second statement inserts a log before the first one, so the splice +-- makes room by moving everything after the start event along. Before the fix +-- that included the two parameters, which mxcli draws on a row above the flow +-- (x 200 and 300, y 53): both moved right, and from then on describe printed a +-- @position on each that no script ever stated, the row drifting further with +-- every insert. +-- +-- Fixed: a parameter is not on the line the room is made on, so it stays +-- where it is. Studio Pro authors draw a parameter above the start event, and +-- inserting an activity in Studio Pro moves nothing. +-- +-- Verify: exec, then +-- mxcli -p app.mpr -c "describe microflow MyFirstModule.ACT_ParamRow" +-- Expected: the parameters carry no @position (they are still on the derived +-- row), and the new log is first. +-- ============================================================================ + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; + +create or modify microflow MyFirstModule.ACT_ParamRow ($Name: String, $Count: Integer) +begin + log info node 'Probe' 'inserted'; + log info node 'Probe' 'first ' + $Name; + log info node 'Probe' 'second ' + toString($Count); +end; diff --git a/mdl/backend/mfmutator/splice.go b/mdl/backend/mfmutator/splice.go index b28ff3e354..c0ce3e42a1 100644 --- a/mdl/backend/mfmutator/splice.go +++ b/mdl/backend/mfmutator/splice.go @@ -881,10 +881,11 @@ func (m *Mutator) markRemoved(v any) { } // shift moves every top-level node past the cut (along axis ax, in the -// direction of s) by delta. The sweep keeps everything on either side of the -// cut exactly as it was relative to its neighbours, so only the flows that -// cross the cut get longer; a flow's control vectors are relative to its ends, -// so no curve changes. skip is a node that is about to be removed. +// direction of s) by delta, but the parameters (shifted). The sweep keeps +// everything on either side of the cut exactly as it was relative to its +// neighbours, so only the flows that cross the cut get longer; a flow's +// control vectors are relative to its ends, so no curve changes. skip is a +// node that is about to be removed. func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip string) { for _, n := range g.order { if !shifted(n, ax, s, cut, skip) { @@ -895,9 +896,15 @@ func (m *Mutator) shift(g *graph, ax axis, s int, cut float64, delta int, skip s } } -// shifted reports whether shift moves n: a top-level node past the cut. +// shifted reports whether shift moves n: a top-level node past the cut that +// is not a parameter. A parameter is not on the line the room is made on: it +// belongs to the flow as a whole, and Studio Pro authors draw it above the +// start event (63 of 71 in PedApp and JTSBootLogboek at the start's x, none +// within 60px of the line). Moved, it drifted right with every insert and +// describe printed a @position the script never stated (ako/mxcli#1547). func shifted(n *node, ax axis, s int, cut float64, skip string) bool { - return n.loop == "" && n.id != skip && float64(s)*(float64(n.pos.get(ax))-cut) > 0 + return n.loop == "" && n.id != skip && n.typ != "Microflows$MicroflowParameter" && + float64(s)*(float64(n.pos.get(ax))-cut) > 0 } // pinnedPast returns the first node shift would move that is drawn at a point diff --git a/mdl/backend/mfmutator/splice_parameters_test.go b/mdl/backend/mfmutator/splice_parameters_test.go new file mode 100644 index 0000000000..96c2b74ee4 --- /dev/null +++ b/mdl/backend/mfmutator/splice_parameters_test.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mfmutator + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" +) + +// A parameter is not on the line a fragment is spliced into, so making room +// does not move it (ako/mxcli#1547). shift moved every top-level node past the +// cut, parameters included: one inserted if/else moved a nanoflow's `$Flag` +// from its derived (200, 53) to (539, 53), and from then on describe printed a +// @position the script never stated, the row drifting right with every insert. +// Studio Pro authors draw a parameter above the start event — 63 of 71 in +// PedApp and JTSBootLogboek sit at the start's x, none within 60px of the line +// — and an insert in Studio Pro moves nothing. The control is the activity +// after the gap, which does move. +func TestSplice_MakingRoomLeavesTheParameters(t *testing.T) { + positions := func(m *Mutator) map[string]point { + out := map[string]point{} + for _, n := range m.graph().order { + out[n.id] = n.pos + } + return out + } + objs := append(line(), obj("param", "Microflows$MicroflowParameter", 500, 53)) + for _, c := range []struct { + name string + splice func(m *Mutator) error + }{ + {"an insert", func(m *Mutator) error { return m.InsertAfter(model.ID(uid("a")), oneActivity()) }}, + {"a replace longer than what it replaces", func(m *Mutator) error { return m.Replace(model.ID(uid("a")), twoActivities()) }}, + } { + t.Run(c.name, func(t *testing.T) { + m, _ := newMutator(t, unit(objs, lineFlows())) + if err := c.splice(m); err != nil { + t.Fatal(err) + } + got := positions(m) + if p := got[uid("b")]; p.X <= 420 { + t.Fatalf("control: b is at x=%d, want it moved along past 420 to make room", p.X) + } + if p := got[uid("param")]; p != (point{500, 53}) { + t.Errorf("the parameter moved to (%d, %d), want it left at (500, 53)", p.X, p.Y) + } + }) + } +} diff --git a/mdl/executor/flow_modify_stated_positions_1508_test.go b/mdl/executor/flow_modify_stated_positions_1508_test.go index bb4a3cf40d..9ad10bf23c 100644 --- a/mdl/executor/flow_modify_stated_positions_1508_test.go +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -160,6 +160,15 @@ func TestCreateOrModify_InsertDoesNotMoveStatedNodes1508(t *testing.T) { if v.objectAt(t, -500, 200, "Microflows$ActionActivity") == "" { t.Errorf("the declare the script draws at (-500, 200) moved:\n%s", describe()) } + // The parameter is not on the line the room was made on: it stays on + // its derived row (ako/mxcli#1547), where it moved to (539, 53). + for _, id := range v.order { + if o := v.objs[id]; afGet(o, "$Type") == "Microflows$MicroflowParameter" { + if x, y := afPoint(o); x != 200 || y != 53 { + t.Errorf("$Flag moved to (%d, %d), want it left at (200, 53):\n%s", x, y, describe()) + } + } + } split, merge, branch := nf1508Branch(t, v, "$Flag") if len(branch) != 2 { t.Fatalf("`if $Flag` has %d activities in its branch, want 2:\n%s", len(branch), describe()) From 756bcfb4a98c76b3f1b41b9a354097ff7e1bdb77 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 14:55:30 +0000 Subject: [PATCH 05/13] refactor: move the object-list body-slot rule into mdl/types bodyItemSlotKey (content, else the first widget slot) decides where an item's loose children go for CREATE and DESCRIBE. ALTER PAGE is about to need the same answer for a bare item address (`acc1 item@1`), and the page mutator cannot import the executor, so the rule moves to mdl/types beside WidgetSlotKeyword. No behaviour change; the executor name stays as a thin alias. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- mdl/executor/widget_engine.go | 20 ++------------------ mdl/types/widget_slot_keywords.go | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index 23007401bc..9a133b1b40 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -2393,24 +2393,8 @@ func defaultItemSlotKey(mapping *ObjectListMapping) string { return bodyItemSlotKey(keys) } -// bodyItemSlotKey picks an object-list item's BODY among its widget slots -// (property keys, in schema order): `content` when the item has one, else the -// first. CREATE routes an item's loose children there; DESCRIBE prints that -// slot as the item's children and every other slot as a ` { … }` -// block. One function, so the two cannot disagree about which slot loose -// widgets belong to — they did, and an Accordion group's headerContent -// round-tripped into its content (mendixlabs/mxcli#1453). -func bodyItemSlotKey(keys []string) string { - for _, k := range keys { - if k == "content" { - return k - } - } - if len(keys) == 0 { - return "" - } - return keys[0] -} +// bodyItemSlotKey is types.BodyItemSlotKey, which the page mutator reads too. +func bodyItemSlotKey(keys []string) string { return types.BodyItemSlotKey(keys) } // lookupProperty does a case-insensitive lookup against an AST property map. // MDL property names ignore case (e.g. `HeaderText` and `headerText` both match). diff --git a/mdl/types/widget_slot_keywords.go b/mdl/types/widget_slot_keywords.go index 6429503823..9c75bafd12 100644 --- a/mdl/types/widget_slot_keywords.go +++ b/mdl/types/widget_slot_keywords.go @@ -42,3 +42,23 @@ func WidgetSlotKeyword(widgetID, propertyKey string) string { } return strings.ToUpper(propertyKey) } + +// BodyItemSlotKey picks an object-list item's BODY among its widget slots +// (property keys, in schema order): `content` when the item has one, else the +// first. CREATE routes an item's loose children there; DESCRIBE prints that +// slot as the item's children and every other slot as a ` { … }` +// block; ALTER PAGE writes there for a bare item address (`acc1 item@1`). One +// function, so the three cannot disagree about which slot loose widgets belong +// to — two of them did, and an Accordion group's headerContent round-tripped +// into its content (mendixlabs/mxcli#1453). +func BodyItemSlotKey(keys []string) string { + for _, k := range keys { + if k == "content" { + return k + } + } + if len(keys) == 0 { + return "" + } + return keys[0] +} From 66a4a1b3133c8e54c105182c3941f720262f5fe3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 14:57:32 +0000 Subject: [PATCH 06/13] feat(grammar): an item address takes a slot, `acc1 item@1.headercontent` alterTarget's two `item` alternatives gain `(DOT itemSlot=identifierOrKeyword)?`. The visitor carries the slot as WidgetRef.Column, the way `grid.controlbar` carries a widget's slot, so every operation and the alter-target resolver receive it as the dotted member they already understand. The slot is taken off the identifiers before the selector is read: `item@1.headercontent` has two identifiers, the shape `item(t1)` was recognised by, and the first version of the test read it as `item(headercontent)@1`. PROPOSAL_alter_object_list_item_address.md step 3. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- mdl/grammar/MDLParser.g4 | 10 ++++-- mdl/visitor/visitor_alter_item_slot_test.go | 40 +++++++++++++++++++++ mdl/visitor/visitor_alter_page.go | 8 +++++ 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 mdl/visitor/visitor_alter_item_slot_test.go diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 63fda217df..b85e3eacb9 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -441,10 +441,16 @@ alterFragment // holding the widget named t1, `nav item('Home')` for the one whose text is // Home, and `nav item@2` for the second item. With a selector, @n chooses among // the items that match it. +// +// The same address reaches a pluggable widget's object-list item — an +// Accordion group, a Pop-up menu item — which stores no name either +// (PROPOSAL_alter_object_list_item_address.md): `acc1 item@1` is the group, +// INSERT INTO it fills its body slot, and `acc1 item@1.headercontent` names +// another of its slots, as `grid.controlbar` names a widget's. alterTarget : identifierOrKeyword COLUMN LPAREN (attributePathV3 | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? - | identifierOrKeyword ITEM LPAREN (identifierOrKeyword | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? - | identifierOrKeyword ITEM AT NUMBER_LITERAL + | identifierOrKeyword ITEM LPAREN (identifierOrKeyword | STRING_LITERAL) RPAREN (AT NUMBER_LITERAL)? (DOT itemSlot=identifierOrKeyword)? + | identifierOrKeyword ITEM AT NUMBER_LITERAL (DOT itemSlot=identifierOrKeyword)? | identifierOrKeyword (DOT identifierOrKeyword)? (AT NUMBER_LITERAL)? | STRING_LITERAL (AT NUMBER_LITERAL)? ; diff --git a/mdl/visitor/visitor_alter_item_slot_test.go b/mdl/visitor/visitor_alter_item_slot_test.go new file mode 100644 index 0000000000..96dbe197e3 --- /dev/null +++ b/mdl/visitor/visitor_alter_item_slot_test.go @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// PROPOSAL_alter_object_list_item_address.md: an item address takes a slot, +// `acc1 item@1.headercontent`, carried as the target's Column exactly as +// `grid.controlbar` carries a widget's slot. The slot must not be read as the +// selector's widget — `item@1.headercontent` has two identifiers, the shape +// `item(t1)` used to be recognised by. +func TestAlterTarget_ItemSlot(t *testing.T) { + stmt := buildAlterPage(t, `alter page M.P { + insert into acc1 item@1.headercontent { dynamictext t1 (Content: 'x') }; + insert into acc1 item(btnIn).headercontent { dynamictext t2 (Content: 'x') }; + insert into acc1 item('One')@2.headercontent { dynamictext t3 (Content: 'x') }; + insert into acc1 item@1 { dynamictext t4 (Content: 'x') }; + };`) + check := func(label string, got ast.WidgetRef, want ast.NavItemRef, spelled, slot string) { + t.Helper() + if got.Item == nil || *got.Item != want { + t.Errorf("%s: Item = %+v, want %+v", label, got.Item, want) + } + if got.Widget != spelled || got.Column != slot || got.Ordinal != 0 { + t.Errorf("%s: Widget %q Column %q Ordinal %d, want %q, %q, 0", label, got.Widget, got.Column, got.Ordinal, spelled, slot) + } + } + ins := func(i int) ast.WidgetRef { return stmt.Operations[i].(*ast.InsertWidgetOp).Target } + check("item@1.slot", ins(0), ast.NavItemRef{List: "acc1", Ordinal: 1}, "acc1 item@1", "headercontent") + check("item(w).slot", ins(1), ast.NavItemRef{List: "acc1", Widget: "btnIn"}, "acc1 item(btnIn)", "headercontent") + check("item('t')@2.slot", ins(2), ast.NavItemRef{List: "acc1", Text: "One", Ordinal: 2}, "acc1 item('One')@2", "headercontent") + check("bare item", ins(3), ast.NavItemRef{List: "acc1", Ordinal: 1}, "acc1 item@1", "") + if got := ins(0).Name(); got != "acc1 item@1.headercontent" { + t.Errorf("Name() = %q, want the address as written", got) + } +} diff --git a/mdl/visitor/visitor_alter_page.go b/mdl/visitor/visitor_alter_page.go index ce0d7d30c6..fac82bc64a 100644 --- a/mdl/visitor/visitor_alter_page.go +++ b/mdl/visitor/visitor_alter_page.go @@ -281,6 +281,14 @@ func (b *Builder) buildAlterTarget(ctx parser.IAlterTargetContext) ast.WidgetRef // the item's, so it moves off the target, where it would mean a choice // among widgets. ids := tc.AllIdentifierOrKeyword() + // `acc1 item@1.headercontent` names a slot of the item. It is carried + // as Column, the way `grid.controlbar` carries a widget's slot, and is + // taken off the identifiers first: `item@1.slot` has two of them, the + // shape `item(t1)` is otherwise recognised by. + if slot := tc.GetItemSlot(); slot != nil { + ref.Column = identifierOrKeywordText(slot) + ids = ids[:len(ids)-1] + } item := &ast.NavItemRef{List: identifierOrKeywordText(ids[0]), Ordinal: ref.Ordinal} if sl := tc.STRING_LITERAL(); sl != nil { item.Text = unquoteStringLit(sl) From 3002233fa2d7bc4cf8368109b3f0595f7ba7dee3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:07:36 +0000 Subject: [PATCH 07/13] feat(alter page): address a pluggable widget's object-list items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Accordion group (any CustomWidgets$WidgetObject item) stores no Name, so `insert into grp1 { … }` never resolved, and an empty group could not be filled at all (ako/mxcli#1326, mendixlabs/mxcli#1453). Before: insert into acc1 item@2 { … } -> acc1 item@2: "acc1" is a CustomWidgets$CustomWidget, not a navigation list The navigation-list item address now reaches object-list items too: insert into acc1 item@2 { … } -- the item's body slot insert into acc1 item('One') { … } -- by its text (headerText) insert into acc1 item(btnIn) { … } -- the item holding a widget insert into acc1 item@2.headercontent { … } -- another slot replace / drop acc1 item@1.content -- swap / empty a slot drop acc1 item@2 -- remove the item set (headerText: 'First') on acc1 item@1 -- an item property - resolveObjectListItem finds the widget's one object list (refusing, by name, a widget with several), matches by held widget, by TextTemplate/String text, or by position, and shares @n, the ambiguity and the miss messages with navigation lists (pickItem). - The result carries the item schema, so SET reuses the column setter and `item.slot` reuses the pluggable-slot code from #1462. - The body slot is types.BodyItemSlotKey, the slot CREATE and DESCRIBE use. - BEFORE/AFTER an item, and REPLACE of a whole item with widgets, are refused with the form that works: adding a new item is not part of this change, and widgets written into an item list are the #935 unloadable shape. Control: the new tests fail on main with the refusal above. Fixed, on a blank 11.13.0 app (Accordion inside a data view) the four statements apply, describe shows each widget in its slot, the text box binds to the data view's attribute, and mx check reports 0 errors. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- mdl/backend/pagemutator/alter_target.go | 2 +- mdl/backend/pagemutator/mutator.go | 19 ++ mdl/backend/pagemutator/navlist.go | 9 +- .../object_list_item_address_test.go | 292 ++++++++++++++++++ mdl/backend/pagemutator/objectlist_items.go | 287 +++++++++++++++++ mdl/backend/pagemutator/pluggableslot.go | 3 + 6 files changed, 609 insertions(+), 3 deletions(-) create mode 100644 mdl/backend/pagemutator/object_list_item_address_test.go create mode 100644 mdl/backend/pagemutator/objectlist_items.go diff --git a/mdl/backend/pagemutator/alter_target.go b/mdl/backend/pagemutator/alter_target.go index 1ffd1890ef..a72a8889df 100644 --- a/mdl/backend/pagemutator/alter_target.go +++ b/mdl/backend/pagemutator/alter_target.go @@ -45,7 +45,7 @@ func (m *Mutator) ResolveAlterTarget(t backend.AlterTarget) (backend.AlterTarget if result == nil { return backend.AlterTargetMatch{}, m.widgetNotFoundError(name) } - if widgetTypeName(result.widget) == navListItemType { + if widgetTypeName(result.widget) == navListItemType || result.item != nil { return backend.AlterTargetMatch{Kind: "item", Name: name}, nil } if bsonnav.DGetString(result.widget, "$Type") != objectListItemType && len(result.colPropKeys) == 0 { diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index 161c3c6dc1..e33c0e6935 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -516,6 +516,9 @@ func (m *Mutator) InsertWidget(widgetRef string, columnRef string, position back if result == nil { return m.widgetNotFoundError(widgetRef) } + if result.item != nil { + return m.insertIntoObjectListItem(result, widgetRef, position, widgets) + } if err := refuseObjectListItemTarget(result, widgetRef); err != nil { return err } @@ -543,6 +546,9 @@ func (m *Mutator) InsertWidget(widgetRef string, columnRef string, position back if strings.EqualFold(string(position), "into") { newContainer, err := appendChildrenToContainer(result.widget, widgetRef, newBsonWidgets) if err != nil { + if hint := objectListItemHint(result, widgetRef); hint != "" { + return fmt.Errorf("cannot INSERT INTO %q: %s", widgetRef, hint) + } if hint := pluggableSlotHint(result, widgetRef); hint != "" { return fmt.Errorf("cannot INSERT INTO %q: a pluggable widget keeps its child widgets in named slots, "+ "not in a list of its own — use %s", widgetRef, hint) @@ -704,6 +710,15 @@ func (m *Mutator) ReplaceWidget(widgetRef string, columnRef string, widgets []pa if result == nil { return m.widgetNotFoundError(widgetRef) } + if result.item != nil { + // Widgets in place of an item would be written into the item list, + // which the loader cannot read (the #935 shape). The item's content is + // a slot, and replacing that is what was meant. + body := strings.ToLower(types.BodyItemSlotKey(result.item.slotKeys)) + return fmt.Errorf("cannot replace %q with widgets: it is a %s item, not a widget — replace its "+ + "content with `replace %s.%s with { … }`, or remove the item with `drop %s`", + widgetRef, result.item.listKey, widgetRef, body, widgetRef) + } if err := refuseObjectListItemTarget(result, widgetRef); err != nil { return err } @@ -1460,6 +1475,10 @@ type bsonWidgetResult struct { // declares (Expression, TextTemplate, Boolean, …). Without it a setter // cannot tell which field of a WidgetValue to write — see columnValueField. colPropKinds map[string]string + // item is set when the result is a pluggable widget's object-list item + // addressed as `owner item…` (an Accordion group): its slots are reached + // through it, and it is never a widget to insert beside or replace. + item *objectListItemInfo } // widgetFinder is a function type for locating widgets in a raw BSON tree. diff --git a/mdl/backend/pagemutator/navlist.go b/mdl/backend/pagemutator/navlist.go index 87037f546f..106a7d002f 100644 --- a/mdl/backend/pagemutator/navlist.go +++ b/mdl/backend/pagemutator/navlist.go @@ -175,8 +175,13 @@ func resolveNavItem(rawData bson.D, find widgetFinder, sel backend.NavItemSelect return nil, fmt.Errorf("%s: navigation list %q not found", sel, sel.List) } if !isNavigationList(list.widget) { - return nil, fmt.Errorf("%s: %q is a %s, not a navigation list — `item(…)` and `item@n` address a navigation list's items", - sel, sel.List, widgetTypeName(list.widget)) + // A pluggable widget's object-list item (an Accordion group) has no + // name either, and is addressed the same way. + if bsonnav.DGetString(list.widget, "$Type") == "CustomWidgets$CustomWidget" { + return resolveObjectListItem(list, sel) + } + return nil, fmt.Errorf("%s: %q is a %s, not a navigation list — `item(…)` and `item@n` address a navigation list's "+ + "items or a pluggable widget's object-list items", sel, sel.List, widgetTypeName(list.widget)) } items := bsonnav.DGetArrayElements(bsonnav.DGet(list.widget, "Items")) var matches []int diff --git a/mdl/backend/pagemutator/object_list_item_address_test.go b/mdl/backend/pagemutator/object_list_item_address_test.go new file mode 100644 index 0000000000..7fd91474ee --- /dev/null +++ b/mdl/backend/pagemutator/object_list_item_address_test.go @@ -0,0 +1,292 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// PROPOSAL_alter_object_list_item_address.md (ako/mxcli#1326, +// mendixlabs/mxcli#1453): an Accordion group stores no Name, so +// `insert into grp1 { … }` never resolved. It is addressed as an item of the +// accordion — `acc1 item@1`, `acc1 item('One')`, `acc1 item(btnIn)` — and a +// slot of it as `acc1 item@1.headercontent`. + +func tpID(b byte) bson.Binary { + return bson.Binary{Subtype: 0x04, Data: []byte{b, b, b, b, b, b, b, b, b, b, b, b, b, b, b, b}} +} + +var ( + tpGroups = tpID(0x10) + tpRenderMode = tpID(0x11) + tpHeaderText = tpID(0x12) + tpHeaderSlot = tpID(0x13) + tpContent = tpID(0x14) +) + +func textTemplate(text string) bson.D { + return bson.D{ + {Key: "$Type", Value: "Forms$ClientTemplate"}, + {Key: "Parameters", Value: bson.A{int32(2)}}, + {Key: "Template", Value: bson.D{ + {Key: "$Type", Value: "Texts$Text"}, + {Key: "Items", Value: bson.A{int32(3), bson.D{ + {Key: "$Type", Value: "Texts$Translation"}, + {Key: "LanguageCode", Value: "en_US"}, + {Key: "Text", Value: text}, + }}}, + }}, + } +} + +func namedWidget(name string) bson.D { + return bson.D{{Key: "$Type", Value: "Forms$DynamicText"}, {Key: "Name", Value: name}} +} + +func widgetList(names ...string) bson.A { + out := bson.A{int32(2)} + for _, n := range names { + out = append(out, namedWidget(n)) + } + return out +} + +// accordionGroup is one CustomWidgets$WidgetObject of the `groups` list. +func accordionGroup(header string, headerWidgets, bodyWidgets []string) bson.D { + prop := func(tp bson.Binary, value bson.D) bson.D { + return bson.D{{Key: "$Type", Value: "CustomWidgets$WidgetProperty"}, {Key: "TypePointer", Value: tp}, {Key: "Value", Value: value}} + } + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetObject"}, + {Key: "Properties", Value: bson.A{int32(2), + prop(tpRenderMode, bson.D{{Key: "PrimitiveValue", Value: "custom"}, {Key: "Widgets", Value: bson.A{int32(2)}}}), + prop(tpHeaderText, bson.D{{Key: "TextTemplate", Value: textTemplate(header)}, {Key: "Widgets", Value: bson.A{int32(2)}}}), + prop(tpHeaderSlot, bson.D{{Key: "Widgets", Value: widgetList(headerWidgets...)}}), + prop(tpContent, bson.D{{Key: "Widgets", Value: widgetList(bodyWidgets...)}}), + }}, + } +} + +// accordionWithGroups is acc1 with the given groups, its schema declaring +// headerContent BEFORE content — so the body is chosen by name, not position. +func accordionWithGroups(groups ...bson.D) bson.D { + pt := func(id bson.Binary, key, kind string) bson.D { + return bson.D{{Key: "$ID", Value: id}, {Key: "PropertyKey", Value: key}, {Key: "ValueType", Value: bson.D{{Key: "Type", Value: kind}}}} + } + objects := bson.A{int32(2)} + for _, g := range groups { + objects = append(objects, g) + } + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidget"}, + {Key: "Name", Value: "acc1"}, + {Key: "Type", Value: bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidgetType"}, + {Key: "WidgetId", Value: "com.mendix.widget.web.accordion.Accordion"}, + {Key: "ObjectType", Value: bson.D{{Key: "PropertyTypes", Value: bson.A{int32(2), bson.D{ + {Key: "$ID", Value: tpGroups}, + {Key: "PropertyKey", Value: "groups"}, + {Key: "ValueType", Value: bson.D{ + {Key: "Type", Value: "Object"}, + {Key: "IsList", Value: true}, + {Key: "ObjectType", Value: bson.D{{Key: "PropertyTypes", Value: bson.A{int32(2), + pt(tpRenderMode, "headerRenderMode", "Enumeration"), + pt(tpHeaderText, "headerText", "TextTemplate"), + pt(tpHeaderSlot, "headerContent", "Widgets"), + pt(tpContent, "content", "Widgets"), + }}}}, + }}, + }}}}}, + }}, + {Key: "Object", Value: bson.D{{Key: "Properties", Value: bson.A{int32(2), bson.D{ + {Key: "$Type", Value: "CustomWidgets$WidgetProperty"}, + {Key: "TypePointer", Value: tpGroups}, + {Key: "Value", Value: bson.D{{Key: "Objects", Value: objects}, {Key: "Widgets", Value: bson.A{int32(2)}}}}, + }}}}}, + } +} + +func twoGroupMutator() *Mutator { + return New(makeRawPage(accordionWithGroups( + accordionGroup("One", nil, []string{"btnIn"}), + accordionGroup("Two", nil, nil), + )), model.ID("u"), &stubWidgetDeps{}) +} + +// groupsOf returns acc1's stored groups. +func groupsOf(t *testing.T, m *Mutator) []any { + t.Helper() + acc := findWidgetForTest(t, m.rawData, "acc1") + prop := bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(acc, "Object"), "Properties"))[0].(bson.D) + return bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(prop, "Value"), "Objects")) +} + +// slotNames returns the widget names in group g's property tp, checking the +// list keeps its [2] marker. +func slotNames(t *testing.T, m *Mutator, g int, tp bson.Binary) []string { + t.Helper() + group := groupsOf(t, m)[g].(bson.D) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(group, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) != bsonnav.ExtractBinaryIDFromDoc(tp) { + continue + } + raw := bsonnav.ToBsonA(bsonnav.DGet(bsonnav.DGetDoc(pd, "Value"), "Widgets")) + if len(raw) == 0 || raw[0] != int32(2) { + t.Fatalf("slot lost its [2] marker: %v", raw) + } + var names []string + for _, w := range raw[1:] { + names = append(names, bsonnav.DGetString(w.(bson.D), "Name")) + } + return names + } + t.Fatalf("group %d has no such property", g) + return nil +} + +func itemButton(name string) []pages.Widget { + b := &pages.ActionButton{} + b.Name = name + return []pages.Widget{b} +} + +var into = backend.InsertPosition("into") + +func TestInsertIntoItemBody_EverySelector(t *testing.T) { + for _, c := range []struct { + ref string + group int + want string + }{ + {"acc1 item@1", 0, "btnIn,btnNew"}, + {"acc1 item@2", 1, "btnNew"}, // an EMPTY body — the case nothing could reach + {"acc1 item('Two')", 1, "btnNew"}, + {"acc1 item(btnIn)", 0, "btnIn,btnNew"}, + } { + m := twoGroupMutator() + if err := m.InsertWidget(c.ref, "", into, itemButton("btnNew")); err != nil { + t.Fatalf("insert into %s: %v", c.ref, err) + } + if got := strings.Join(slotNames(t, m, c.group, tpContent), ","); got != c.want { + t.Errorf("insert into %s: group %d content = %s, want %s", c.ref, c.group+1, got, c.want) + } + if got := slotNames(t, m, c.group, tpHeaderSlot); len(got) != 0 { + t.Errorf("insert into %s wrote into headerContent too: %v", c.ref, got) + } + } +} + +func TestInsertIntoItemSlot(t *testing.T) { + m := twoGroupMutator() + if err := m.InsertWidget("acc1 item@1", "headercontent", into, itemButton("btnHead")); err != nil { + t.Fatalf("insert into acc1 item@1.headercontent: %v", err) + } + if got := slotNames(t, m, 0, tpHeaderSlot); len(got) != 1 || got[0] != "btnHead" { + t.Errorf("headerContent = %v, want [btnHead]", got) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnIn" { + t.Errorf("content = %s, want btnIn untouched", got) + } +} + +func TestReplaceAndDropItemSlot(t *testing.T) { + m := twoGroupMutator() + if err := m.ReplaceWidget("acc1 item@1", "content", itemButton("btnRep")); err != nil { + t.Fatalf("replace acc1 item@1.content: %v", err) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnRep" { + t.Errorf("after replace content = %s, want btnRep", got) + } + if err := m.DropWidget([]backend.WidgetRef{{Widget: "acc1 item@1", Column: "content"}}); err != nil { + t.Fatalf("drop acc1 item@1.content: %v", err) + } + if got := slotNames(t, m, 0, tpContent); len(got) != 0 { + t.Errorf("after drop content = %v, want empty", got) + } + if n := len(groupsOf(t, m)); n != 2 { + t.Errorf("dropping a slot removed a group: %d groups left", n) + } +} + +func TestDropItem(t *testing.T) { + m := twoGroupMutator() + if err := m.DropWidget([]backend.WidgetRef{{Widget: "acc1 item@2"}}); err != nil { + t.Fatalf("drop acc1 item@2: %v", err) + } + groups := groupsOf(t, m) + if len(groups) != 1 { + t.Fatalf("%d groups left, want 1", len(groups)) + } + if got := strings.Join(slotNames(t, m, 0, tpContent), ","); got != "btnIn" { + t.Errorf("the wrong group was dropped: remaining content %s", got) + } +} + +func TestSetItemProperty(t *testing.T) { + m := twoGroupMutator() + if err := m.SetWidgetProperty("acc1 item@1", "headerText", "Renamed"); err != nil { + t.Fatalf("set headerText on acc1 item@1: %v", err) + } + if err := m.InsertWidget("acc1 item('Renamed')", "", into, itemButton("btnNew")); err != nil { + t.Fatalf("the new header text does not address the group: %v", err) + } +} + +func TestResolveAlterTargetItem(t *testing.T) { + m := twoGroupMutator() + for _, c := range []struct { + path []string + kind string + }{ + {[]string{"acc1 item@1"}, "item"}, + {[]string{"acc1 item@1", "headercontent"}, "region"}, + } { + got, err := m.ResolveAlterTarget(backend.AlterTarget{Path: c.path}) + if err != nil { + t.Fatalf("ResolveAlterTarget(%v): %v", c.path, err) + } + if got.Kind != c.kind { + t.Errorf("ResolveAlterTarget(%v) kind = %q, want %q", c.path, got.Kind, c.kind) + } + } +} + +func TestItemAddressMissAndAmbiguity(t *testing.T) { + m := New(makeRawPage(accordionWithGroups( + accordionGroup("Same", nil, nil), + accordionGroup("Same", nil, nil), + )), model.ID("u"), &stubWidgetDeps{}) + err := m.InsertWidget("acc1 item('Same')", "", into, itemButton("b")) + if err == nil || !strings.Contains(err.Error(), "ambiguous") || !strings.Contains(err.Error(), "@1") { + t.Errorf("two groups titled Same: want an ambiguity error naming @n, got %v", err) + } + err = m.InsertWidget("acc1 item@3", "", into, itemButton("b")) + if err == nil || !strings.Contains(err.Error(), "item@1") || !strings.Contains(err.Error(), "item@2") { + t.Errorf("item@3 of two: want a miss listing the addresses that resolve, got %v", err) + } + if err := m.InsertWidget("acc1 item('Same')@2", "", into, itemButton("b")); err != nil { + t.Errorf("@n among matches: %v", err) + } +} + +// Adding a new group is not part of this change; BEFORE/AFTER an item must be +// refused, not treated as INTO or written into the Objects list as a widget. +func TestInsertBesideItemIsRefused(t *testing.T) { + m := twoGroupMutator() + err := m.InsertWidget("acc1 item@1", "", backend.InsertPosition("after"), itemButton("b")) + if err == nil { + t.Fatal("insert after acc1 item@1 { widget } must be refused") + } + if n := len(groupsOf(t, m)); n != 2 { + t.Errorf("a refused insert changed the group count to %d", n) + } +} diff --git a/mdl/backend/pagemutator/objectlist_items.go b/mdl/backend/pagemutator/objectlist_items.go new file mode 100644 index 0000000000..1ee906b61b --- /dev/null +++ b/mdl/backend/pagemutator/objectlist_items.go @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "fmt" + "strings" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// A pluggable widget's object-list item — an Accordion group, a Pop-up menu +// item — is a CustomWidgets$WidgetObject and stores no Name, so `insert into +// grp1` never resolved: the name a script wrote on `group grp1` was parsed and +// dropped (ako/mxcli#1326, mendixlabs/mxcli#1453). It is addressed the way a +// navigation list item is (#1388), by what it holds, what it shows or where it +// is: `acc1 item(btnIn)`, `acc1 item('One')`, `acc1 item@1`. A bare item address +// means the item's BODY slot (types.BodyItemSlotKey, the slot CREATE routes loose +// children to); `acc1 item@1.headercontent` names another slot, as +// `grid.controlbar` names a widget's. PROPOSAL_alter_object_list_item_address.md. + +// objectListItemInfo is what a resolved item carries beyond its position: the +// widget that owns the list, and the item's widget slots in schema order. +type objectListItemInfo struct { + owner bson.D + listKey string + slotKeys []string // Widgets-typed item property keys, schema order +} + +// itemList is one object-list property of a pluggable widget. +type itemList struct { + key string + typeID string + value bson.D // the property's WidgetValue, holding Objects + slotKeys []string +} + +// objectItemLists returns the widget's object-list properties: its schema's +// `Object`-typed, IsList properties that are stored with an Objects list. +func objectItemLists(widget bson.D) []itemList { + objType := bsonnav.DGetDoc(bsonnav.DGetDoc(widget, "Type"), "ObjectType") + var lists []itemList + for _, pt := range bsonnav.DGetArrayElements(bsonnav.DGet(objType, "PropertyTypes")) { + ptDoc, ok := pt.(bson.D) + if !ok { + continue + } + vt := bsonnav.DGetDoc(ptDoc, "ValueType") + if bsonnav.DGetString(vt, "Type") != "Object" || bsonnav.DGet(vt, "IsList") != true { + continue + } + l := itemList{ + key: bsonnav.DGetString(ptDoc, "PropertyKey"), + typeID: bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(ptDoc, "$ID")), + } + for _, ipt := range bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(vt, "ObjectType"), "PropertyTypes")) { + iptDoc, ok := ipt.(bson.D) + if ok && bsonnav.DGetString(bsonnav.DGetDoc(iptDoc, "ValueType"), "Type") == "Widgets" { + l.slotKeys = append(l.slotKeys, bsonnav.DGetString(iptDoc, "PropertyKey")) + } + } + for _, prop := range bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(widget, "Object"), "Properties")) { + propDoc, ok := prop.(bson.D) + if ok && bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(propDoc, "TypePointer")) == l.typeID { + l.value = bsonnav.DGetDoc(propDoc, "Value") + } + } + if l.value != nil && hasKey(l.value, "Objects") { + lists = append(lists, l) + } + } + return lists +} + +// resolveObjectListItem finds the one item of a pluggable widget a selector +// addresses, with the same matching, @n and messages as a navigation list. +func resolveObjectListItem(owner *bsonWidgetResult, sel backend.NavItemSelector) (*bsonWidgetResult, error) { + lists := objectItemLists(owner.widget) + switch { + case len(lists) == 0: + return nil, fmt.Errorf("%s: %q (%s) has no list of items — `item(…)` and `item@n` address a navigation "+ + "list's items or a pluggable widget's object-list items (an Accordion's groups)", + sel, sel.List, widgetTypeName(owner.widget)) + case len(lists) > 1: + var keys []string + for _, l := range lists { + keys = append(keys, l.key) + } + return nil, fmt.Errorf("%s: %q has more than one list of items (%s), so `item` does not say which — "+ + "not supported yet", sel, sel.List, strings.Join(keys, ", ")) + } + l := lists[0] + keys, kinds := buildColumnPropKeyMap(owner.widget, l.typeID) + items := bsonnav.DGetArrayElements(bsonnav.DGet(l.value, "Objects")) + var matches []int + for i, el := range items { + itemDoc, ok := el.(bson.D) + if !ok { + continue + } + switch { + case sel.Widget != "": + if !itemHoldsWidget(itemDoc, sel.Widget) { + continue + } + case sel.Text != "": + if !containsString(objectItemTexts(itemDoc, kinds), sel.Text) { + continue + } + } + matches = append(matches, i) + } + pick, err := pickItem(sel, matches, len(items), func() string { return objectItemList(sel.List, items, kinds) }) + if err != nil { + return nil, err + } + return &bsonWidgetResult{ + widget: items[pick].(bson.D), + parentArr: items, + parentKey: "Objects", + parentDoc: l.value, + index: pick, + colPropKeys: keys, + colPropKinds: kinds, + item: &objectListItemInfo{owner: owner.widget, listKey: l.key, slotKeys: l.slotKeys}, + }, nil +} + +// pickItem applies a selector's @n to the indexes that match it. listing names +// every item's addresses, for a message that says what would hit. +func pickItem(sel backend.NavItemSelector, matches []int, total int, listing func() string) (int, error) { + switch { + case sel.Ordinal > 0 && sel.Ordinal <= len(matches): + return matches[sel.Ordinal-1], nil + case sel.Ordinal > 0: + return -1, fmt.Errorf("%s: %s, so there is no @%d. %s", sel, countMatches(len(matches), sel), sel.Ordinal, listing()) + case len(matches) == 1: + return matches[0], nil + case len(matches) == 0: + return -1, fmt.Errorf("%s: no item matches. %s", sel, listing()) + default: + return -1, fmt.Errorf("%s is ambiguous: %s — add @n to choose one (@1 … @%d). %s", + sel, countMatches(len(matches), sel), len(matches), listing()) + } +} + +// itemHoldsWidget reports whether any slot of the item holds the named widget, +// at any depth. +func itemHoldsWidget(itemDoc bson.D, name string) bool { + for _, val := range objectListItemPropValues(itemDoc) { + if findInWidgetArray(val, "Widgets", name) != nil { + return true + } + } + return false +} + +// objectListItemPropValues returns the Value document of every property of an +// item. +func objectListItemPropValues(itemDoc bson.D) []bson.D { + var out []bson.D + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(itemDoc, "Properties")) { + if pd, ok := p.(bson.D); ok { + if v := bsonnav.DGetDoc(pd, "Value"); v != nil { + out = append(out, v) + } + } + } + return out +} + +// objectItemTexts returns the texts an item shows: its TextTemplate properties in +// every language, and its String properties — an Accordion group's headerText. +func objectItemTexts(itemDoc bson.D, kinds map[string]string) []string { + var out []string + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(itemDoc, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + val := bsonnav.DGetDoc(pd, "Value") + switch kinds[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] { + case "TextTemplate": + tmpl := bsonnav.DGetDoc(bsonnav.DGetDoc(val, "TextTemplate"), "Template") + for _, it := range bsonnav.DGetArrayElements(bsonnav.DGet(tmpl, "Items")) { + if d, ok := it.(bson.D); ok { + if t := bsonnav.DGetString(d, "Text"); t != "" && !containsString(out, t) { + out = append(out, t) + } + } + } + case "String": + if t := bsonnav.DGetString(val, "PrimitiveValue"); t != "" && !containsString(out, t) { + out = append(out, t) + } + } + } + return out +} + +// objectItemList lists each item's addresses, so a miss shows what would hit. +func objectItemList(list string, items []any, kinds map[string]string) string { + if len(items) == 0 { + return fmt.Sprintf("%q has no items", list) + } + var parts []string + for i, el := range items { + itemDoc, _ := el.(bson.D) + addrs := []string{fmt.Sprintf("item@%d", i+1)} + if texts := objectItemTexts(itemDoc, kinds); len(texts) > 0 { + addrs = append(addrs, "item('"+strings.ReplaceAll(texts[0], "'", "''")+"')") + } + parts = append(parts, strings.Join(addrs, " = ")) + } + return fmt.Sprintf("Items of %s: %s", list, strings.Join(parts, "; ")) +} + +// itemSlots returns a resolved item's widget slots, keyed as an item slot is +// spelled in MDL: the property key, lower-cased (`headercontent`), the keyword +// CREATE's item builder routes. The first returned slot is not special; the +// body is chosen with types.BodyItemSlotKey. +func itemSlots(result *bsonWidgetResult) []pluggableSlot { + var out []pluggableSlot + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(result.widget, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + tp := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) + if result.colPropKinds[tp] != "Widgets" { + continue + } + val := bsonnav.DGetDoc(pd, "Value") + if val == nil || !hasKey(val, "Widgets") { + continue + } + out = append(out, pluggableSlot{owner: result, keyword: strings.ToLower(result.colPropKeys[tp]), value: val}) + } + return out +} + +// itemBodySlot returns the item's body slot. +func itemBodySlot(result *bsonWidgetResult) (pluggableSlot, bool) { + body := strings.ToLower(types.BodyItemSlotKey(result.item.slotKeys)) + for _, s := range itemSlots(result) { + if s.keyword == body { + return s, true + } + } + return pluggableSlot{}, false +} + +// insertIntoObjectListItem handles a bare item target: INTO fills its body; +// BEFORE/AFTER would add a new item, which ALTER does not do yet. +func (m *Mutator) insertIntoObjectListItem(result *bsonWidgetResult, ref string, position backend.InsertPosition, widgets []pages.Widget) error { + if !strings.EqualFold(string(position), "into") { + return fmt.Errorf("cannot insert %s %q: adding a new %s item is not supported by ALTER yet — "+ + "to put widgets in this item, write `insert into %s { … }`", strings.ToLower(string(position)), + ref, result.item.listKey, ref) + } + body, ok := itemBodySlot(result) + if !ok { + return fmt.Errorf("%q has no widget slot to insert into", ref) + } + newBson, err := m.serializeWidgets(widgets) + if err != nil { + return fmt.Errorf("serialize widgets: %w", err) + } + body.set(append(append([]any{}, body.widgets()...), newBson...)) + return nil +} + +// objectListItemHint names the item addresses of a widget that has an item +// list, for the refusal of `insert into ` on the widget itself. +func objectListItemHint(owner *bsonWidgetResult, name string) string { + lists := objectItemLists(owner.widget) + if len(lists) != 1 || len(lists[0].slotKeys) == 0 { + return "" + } + return fmt.Sprintf("its widgets live in its %s: `insert into %s item@1 { … }` fills the first one, "+ + "`%s item('')` addresses one by its text", lists[0].key, name, name) +} diff --git a/mdl/backend/pagemutator/pluggableslot.go b/mdl/backend/pagemutator/pluggableslot.go index eec127ebc3..8730e81d55 100644 --- a/mdl/backend/pagemutator/pluggableslot.go +++ b/mdl/backend/pagemutator/pluggableslot.go @@ -39,6 +39,9 @@ type pluggableSlot struct { // pluggableSlots lists the widget's own widgets-typed properties (not those of // its object-list items — an Accordion group's content is not the Accordion's). func pluggableSlots(owner *bsonWidgetResult) []pluggableSlot { + if owner.item != nil { + return itemSlots(owner) // `acc1 item@1.headercontent` + } if bsonnav.DGetString(owner.widget, "$Type") != "CustomWidgets$CustomWidget" { return nil } From a50ac0ac3ebb06646780ea2334a46bf33fa5f149 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:10:12 +0000 Subject: [PATCH 08/13] fix(executor): refuse a page following a view entity's association from its target (CE8812, mendixlabs/mxcli#1611) A view entity's association, derived from an `.ID as ` OQL column, is one-way for a page. A data source that follows it from the entity it points at passed check --references and exec. One example: `DATABASE FROM $currentObject/M.Stats_Owner/M.Stats` on a page over M.Owner. mx check then failed with CE8812 "Entity 'M.Owner' has no association 'M.Stats_Owner'". MDL-ASSOCDS02 (validate_view_assoc_reverse.go) runs in the attribute-scope walk beside MDL-ASSOCDS01. It follows the association or `database from` path hop by hop from the widget's context entity, and reports the first hop that follows a view association from its TO end. The hint is the query that builds: DATABASE WHERE [ = $currentObject]. The association index now carries a view flag, from Source == OqlViewAssociationSource. A script's view associations were known to no checker, so that same remedy was refused as naming "neither an attribute nor an association" when the view entity came from the same script. This was true on a page and in a microflow retrieve, both of which build at 0 errors. scriptContext.recordViewAssociations now registers each view association column the way a CREATE ASSOCIATION is registered. Measured on 11.14.0, one page per shape, with uniquely named widgets: - CE8812 for a list view, data grid, gallery and data view, in the database and in-memory forms, and as a second hop. - 0 errors from the view side, for the constrained query, for a persistent association reversed, and for a microflow association retrieve. - check now reports exactly the six refused shapes, and nothing else. - The bug-test file gives 0 errors from check and from mxbuild. With its data source switched to the reverse: MDL-ASSOCDS02 from check, CE8812 from mxbuild. Control: with checkViewAssocReverse unhooked, the 7 refusal cases fail with no error reported. Before the registration fix, the remedy test failed on both the page and the microflow. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HxAn3VqxUnZ7ZGUnzAYpbT --- ...sociation-followed-from-target-ce8812.json | 1 + .../mendix/create-page/reference/widgets.md | 2 + cmd/mxcli/syntax/features_domain_model.go | 5 + .../1611-view-association-reverse.mdl | 72 +++++++++ mdl/executor/validate.go | 26 ++++ mdl/executor/validate_assoc_list_source.go | 10 +- mdl/executor/validate_view_assoc_reverse.go | 113 ++++++++++++++ .../validate_view_assoc_reverse_test.go | 144 ++++++++++++++++++ .../validate_widget_attribute_scope.go | 1 + 9 files changed, 372 insertions(+), 2 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json create mode 100644 mdl-examples/bug-tests/1611-view-association-reverse.mdl create mode 100644 mdl/executor/validate_view_assoc_reverse.go create mode 100644 mdl/executor/validate_view_assoc_reverse_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json new file mode 100644 index 0000000000..76b0c3b320 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-view-entity-association-followed-from-target-ce8812.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-22","symptom":"A page data source following a view entity's association from the entity it points at — `listview (DataSource: DATABASE FROM $currentObject/M.Stats_Owner/M.Stats)` on a page over M.Owner, where M.Stats selects `o.ID as Stats_Owner` — passed check --references and exec, then mx check failed CE8812 \"Entity 'M.Owner' has no association 'M.Stats_Owner'\" (mendixlabs/mxcli#1611 A). Separately, its remedy `DATABASE M.Stats WHERE [M.Stats_Owner = $currentObject]` (and the microflow `retrieve … where M.Stats_Owner = $x`) was refused by check as naming 'neither an attribute nor an association' when the view entity came from the same script.","cause":"Nothing modelled that a view entity's OQL-derived association (OqlViewAssociationSource) is one-way for a page; MDL-ASSOCDS01's association index had no view flag. And scriptContext recorded only CREATE ASSOCIATION statements, so a script's view associations (its `.ID as ` columns) were invisible to every name-resolving checker.","fix":"MDL-ASSOCDS02 (validate_view_assoc_reverse.go), hooked into attributeScopeValidator.walk beside MDL-ASSOCDS01: follow the association or `database from` path hop by hop from the widget's context entity and report the first hop that follows a view association from its TO end, with the constrained query as the fix. assocShape.view from Association/CrossAssociation Source == OqlViewAssociationSource; scriptContext.recordViewAssociations registers each view association column through recordAssociation (ends, name index, shape) and marks it view.","file":"mdl/executor/validate_view_assoc_reverse.go","insight":"Measure the remedy you are about to recommend in the same shape the user works in: the constrained query built at 0 errors against a stored view entity, but the check refused it when the view was created in the same script — the reporter's flow — so the new hint would have sent users into a second false refusal. Measured on 11.14.0, one page per shape with uniquely named widgets so mx check's 'at ' attributes each CE8812: refused in list view, data grid, gallery and data view, database and in-memory form, at any hop; accepted from the view side, constrained, and in a microflow association retrieve (so the rule is page data sources only). An entity named Owner is itself CE7247 — rename repro entities before reading the result.","refs":["mendixlabs/mxcli#1611","ako/mxcli#1539"],"ce":["CE8812"],"rules":["MDL-ASSOCDS02"]} diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index 7d24ce3668..c0be15e8ea 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -410,6 +410,8 @@ column (caption: 'Actions') { > **A list widget needs an association that yields a list.** A list view, data grid or gallery over `$currentObject/M.Assoc` is CE8812 ("A grid association path must result in a list") when the association is a Reference followed from its FROM entity, or a Reference with owner Both from either end (one-to-one). Use a data view for the single object, or a ReferenceSet. `check` reports it as MDL-ASSOCDS01. The reverse of a default Reference (from the TO entity) is a list and fine. +> **A view entity's association is one-way for a page.** The association a view entity derives from its OQL (`select c.ID as Stats_Customer`) can be followed from the view entity only. A data source that follows it from the entity it points at, in any widget and at any hop (`database from $currentObject/M.Stats_Customer/M.Stats`, `$currentObject/M.Stats_Customer`), is CE8812 "Entity 'M.Customer' has no association 'M.Stats_Customer'". Query the view entity instead: `datasource: database M.Stats where [M.Stats_Customer = $currentObject]`. `check` reports it as MDL-ASSOCDS02. A microflow `retrieve … from $Cust/M.Stats_Customer` is accepted. + **With WHERE and SORT BY (inline in DataSource):** ```sql datagrid dgActive ( diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index 65fe54fdc6..03f4e6dd92 100644 --- a/cmd/mxcli/syntax/features_domain_model.go +++ b/cmd/mxcli/syntax/features_domain_model.go @@ -254,6 +254,11 @@ func init() { "- THE ALIAS IS A MODULE-LEVEL NAME. It cannot collide with an entity or\n" + " enumeration in the same module, case-insensitively — Mendix: \"Duplicate\n" + " name … Entities, associations and enumerations cannot share names.\"\n\n" + + "On a page the association is ONE-WAY: a data source can follow it from the\n" + + "view entity only. From the entity it points at (`database from\n" + + "$currentObject/M.Totals_Meter/M.Totals`) mxbuild reports CE8812 \"Entity\n" + + "'M.Meter' has no association 'M.Totals_Meter'\" (MDL-ASSOCDS02). Query the\n" + + "view entity instead: `database M.Totals where [M.Totals_Meter = $currentObject]`.\n\n" + "The alternative is often better: CAST the id to a string and keep it as a\n" + "plain attribute. That is one SQL statement instead of two and materialises\n" + "no objects in the client, and the id is still there to look the real object\n" + diff --git a/mdl-examples/bug-tests/1611-view-association-reverse.mdl b/mdl-examples/bug-tests/1611-view-association-reverse.mdl new file mode 100644 index 0000000000..ed2e8b6c87 --- /dev/null +++ b/mdl-examples/bug-tests/1611-view-association-reverse.mdl @@ -0,0 +1,72 @@ +mdl 1; +-- ============================================================================ +-- mendixlabs/mxcli#1611 A — a page following a view entity's association from +-- the entity it points at (CE8812) +-- ============================================================================ +-- +-- Reported (v0.27.0, Mendix 11.12.5): a view entity M.Stats selects `e.ID as +-- Stats_Owner`; on a page over an Owner, +-- listview lv (DataSource: DATABASE FROM $currentObject/M.Stats_Owner/M.Stats) +-- passed `check --references` and exec, then mx check failed CE8812 "Entity +-- 'M.Owner' has no association 'M.Stats_Owner'". +-- +-- A view entity's association is one-way for a page. Measured on 11.14.0, one +-- page per shape: following it from the entity it points at is CE8812 in a list +-- view, data grid, gallery and data view, in the database form and the +-- in-memory form, and as a second hop. Following it from the view entity, the +-- constrained query below, and a microflow association retrieve all built at +-- 0 errors. `check` now reports the refused shapes as MDL-ASSOCDS02. +-- +-- This file is the shape that must PASS, with the view entity created in the +-- same script. The constrained query used to be refused by check in that case, +-- because the script's view associations were known to no checker. This file: +-- 0 errors from check and from mxbuild. With the list view's data source +-- changed to `DATABASE FROM $currentObject/Bug1611.Totals_Customer/Bug1611.Totals`: +-- MDL-ASSOCDS02 from check, CE8812 from mxbuild. +-- ============================================================================ + +CREATE MODULE Bug1611; + +CREATE OR MODIFY PERSISTENT ENTITY Bug1611.Customer ( Name: String(100) ); +CREATE OR MODIFY PERSISTENT ENTITY Bug1611.Purchase ( Amount: Integer ); +CREATE OR MODIFY ASSOCIATION Bug1611.Purchase_Customer FROM Bug1611.Purchase TO Bug1611.Customer; + +CREATE OR MODIFY VIEW ENTITY Bug1611.Totals ( Total: Integer ) AS ( + from Bug1611.Purchase as p + join p/Bug1611.Purchase_Customer/Bug1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, sum(p.Amount) as Total +); + +-- From the customer: query the view entity, constrained on its association. +CREATE OR MODIFY PAGE Bug1611.Customer_Totals ( + Params: ($Customer: Bug1611.Customer), + Title: 'Totals', + Layout: Atlas_Core.Atlas_Default +) { + dataview dvCustomer (DataSource: $Customer) { + listview lvTotals (DataSource: DATABASE Bug1611.Totals WHERE [Bug1611.Totals_Customer = $currentObject]) { + dynamictext txtTotal (Content: 'total') + } + } +}; + +-- From the view entity: the association followed the way it points. +CREATE OR MODIFY PAGE Bug1611.Totals_Customer ( + Params: ($Totals: Bug1611.Totals), + Title: 'Customer', + Layout: Atlas_Core.Atlas_Default +) { + dataview dvTotals (DataSource: $Totals) { + dataview dvCustomer (DataSource: $currentObject/Bug1611.Totals_Customer) { + dynamictext txtName (Content: 'customer') + } + } +}; + +-- A microflow may retrieve over it from the customer. +CREATE OR MODIFY MICROFLOW Bug1611.ACT_CustomerTotals ($Customer: Bug1611.Customer) +BEGIN + RETRIEVE $Totals FROM $Customer/Bug1611.Totals_Customer; + RETRIEVE $Constrained FROM Bug1611.Totals WHERE Bug1611.Totals_Customer = $Customer; +END; diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 43a994e59a..659f2b8284 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -182,6 +182,31 @@ func (sc *scriptContext) recordEntityAttrs(s *ast.CreateEntityStmt) { // switches over the same statement types, so a case added to one and not the // other is collected on one path only — which is how this rule first shipped // firing against stored associations but not script-declared ones. +func (sc *scriptContext) recordViewAssociations(s *ast.CreateViewEntityStmt) { + // A view entity's associations are its `.ID as ` columns: the + // view entity is the FROM end, the selected entity the TO end, and the + // association lives in the view's module (oql_view_associations.go). + // + // They are registered exactly as a script's CREATE ASSOCIATION is, so every + // checker that resolves association names sees them. Without that, the + // constraint `[M.Stats_Customer = $currentObject]` on the view entity — the + // remedy MDL-ASSOCDS02 names — was refused as naming "neither an attribute + // nor an association" whenever the view came from the same script. + for _, col := range viewAssociationColumns(s.Query.RawQuery) { + to := parseQN(col.Entity) + if to.Module == "" { + continue + } + name := ast.QualifiedName{Module: s.Name.Module, Name: col.Name} + sc.recordAssociation(&ast.CreateAssociationStmt{ + Name: name, Parent: s.Name, Child: to, Type: ast.AssocReference, Owner: ast.OwnerDefault, + }) + shape := sc.associationShapes[strings.ToLower(name.String())] + shape.view = true + sc.associationShapes[strings.ToLower(name.String())] = shape + } +} + func (sc *scriptContext) recordAssociation(s *ast.CreateAssociationStmt) { if s.Name.Module == "" || s.Name.Name == "" { return @@ -246,6 +271,7 @@ func (sc *scriptContext) collectSingle(stmt ast.Statement) { if s.Name.Module != "" { sc.entities[s.Name.String()] = true sc.viewEntities[s.Name.String()] = true + sc.recordViewAssociations(s) } case *ast.CreateExternalEntityStmt: if s.Name.Module != "" { diff --git a/mdl/executor/validate_assoc_list_source.go b/mdl/executor/validate_assoc_list_source.go index a9b7c35888..456a759caa 100644 --- a/mdl/executor/validate_assoc_list_source.go +++ b/mdl/executor/validate_assoc_list_source.go @@ -36,6 +36,10 @@ type assocShape struct { from, to string refSet bool owner string // "Default", "Both", or "" when not one of the measured two + // view marks an association a view entity derives from its OQL (an + // OqlViewAssociationSource): `from` is the view entity, and a page cannot + // follow it from `to` (MDL-ASSOCDS02, validate_view_assoc_reverse.go). + view bool } func astOwnerName(o ast.OwnerType) string { @@ -91,14 +95,16 @@ func checkAssociationShapes(ctx *ExecContext, sc *scriptContext) map[string]asso if from, to := byID[a.ParentID], byID[a.ChildID]; from != "" && to != "" { qn := mod + "." + a.Name out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: to, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner)} + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} } } for _, a := range dm.CrossAssociations { if from := byID[a.ParentID]; from != "" && a.ChildRef != "" { qn := mod + "." + a.Name out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: a.ChildRef, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner)} + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} } } } diff --git a/mdl/executor/validate_view_assoc_reverse.go b/mdl/executor/validate_view_assoc_reverse.go new file mode 100644 index 0000000000..d45951001c --- /dev/null +++ b/mdl/executor/validate_view_assoc_reverse.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// MDL-ASSOCDS02 — a page data source that follows a view entity's association +// from the entity it points at. +// +// A view entity's association (an `.ID as ` column of its OQL) is +// one-way for a page: the persistent entity at its far end does not own it. +// mxbuild refuses a data source that follows it from there with CE8812 +// "Entity 'M.Customer' has no association 'M.Stats_Customer'". `check` passed +// it and exec wrote it (mendixlabs/mxcli#1611 A). +// +// Measured on mxbuild 11.14.0, one page per shape: +// +// DATABASE FROM $currentObject/M.Stats_Customer/M.Stats list view, data grid, gallery → CE8812 +// $currentObject/M.Stats_Customer list view, data view → CE8812 +// … the same hop second in a path (from an Item) → CE8812 +// the same, followed from the view entity → ok +// DATABASE M.Stats WHERE [M.Stats_Customer = $currentObject] → ok +// a microflow `retrieve $L from $Cust/M.Stats_Customer` → ok (not judged) +// +// The walk follows each hop from the widget's context entity, so it judges only +// a path it can place: an unknown association or an entity it cannot follow +// ends the walk silently. +func (v *attributeScopeValidator) checkViewAssocReverse(w *ast.WidgetV3, enclosing dataContext) { + if v.assocs == nil { + return + } + ds := w.GetDataSource() + if ds == nil { + return + } + var path string + switch { + case ds.Type == "association": + path = ds.Reference + case ds.Type == "database" && ds.AssociationPath != "": + path = ds.AssociationPath + default: + return + } + cur := v.dataSourceContextEntity(ds, enclosing) + if cur == "" || path == "" { + return + } + for _, seg := range strings.Split(path, "/") { + seg = strings.TrimSpace(seg) + if seg == "" { + return + } + qn := seg + if !strings.Contains(qn, ".") { + mod, _, _ := strings.Cut(cur, ".") + qn = mod + "." + qn + } + shape, isAssoc := v.assocs[strings.ToLower(qn)] + if !isAssoc { + // An entity step (`/M.Stats` after the association) names where the + // hop arrived; anything else ends what can be judged. + if strings.EqualFold(qn, cur) { + continue + } + return + } + switch { + case strings.EqualFold(cur, shape.from): + cur = shape.to + case strings.EqualFold(cur, shape.to): + if shape.view { + v.errs = append(v.errs, fmt.Sprintf( + "%s data source %s: %s is an association of view entity %s, and a page cannot follow it from %s — "+ + "mxbuild rejects this with CE8812 \"Entity '%s' has no association '%s'\". Query the view entity "+ + "instead: DATABASE %s WHERE [%s = $currentObject] [MDL-ASSOCDS02]", + widgetLabel(w.Name, w.Type), dataSourceDisplay(ds), shape.qn, shape.from, cur, + cur, shape.qn, shape.from, shape.qn)) + return + } + cur = shape.from + default: + return + } + } +} + +// dataSourceContextEntity is the entity a context-relative data source starts +// from: the enclosing data container's for $currentObject, else the page or +// snippet parameter's. "" when it cannot be established. +func (v *attributeScopeValidator) dataSourceContextEntity(ds *ast.DataSourceV3, enclosing dataContext) string { + switch cv := strings.ToLower(ds.ContextVariable); cv { + case "", "currentobject": + if enclosing.unresolved || len(enclosing.entities) == 0 { + return "" + } + return enclosing.entities[len(enclosing.entities)-1] + default: + return v.pageParams[cv] + } +} + +func dataSourceDisplay(ds *ast.DataSourceV3) string { + if ds.Type == "database" { + return "DATABASE FROM $" + ds.ContextVariable + "/" + ds.AssociationPath + } + return dataSourceText(ds) +} diff --git a/mdl/executor/validate_view_assoc_reverse_test.go b/mdl/executor/validate_view_assoc_reverse_test.go new file mode 100644 index 0000000000..2b4069a237 --- /dev/null +++ b/mdl/executor/validate_view_assoc_reverse_test.go @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// viewAssocCtx is a project with a persistent C1611.Customer, a C1611.Item +// pointing at it, and a view entity C1611.Stats whose OQL selects the +// customer's id as Stats_Customer — the association Mendix derives from it. +func viewAssocCtx(t *testing.T) *ExecContext { + t.Helper() + mod := &model.Module{BaseElement: model.BaseElement{ID: "mod-1611"}, Name: "C1611"} + b := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { + return []*domainmodel.DomainModel{{ + BaseElement: model.BaseElement{ID: "dm-1611"}, ContainerID: mod.ID, + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: "e-cust"}, Name: "Customer", Persistable: true}, + {BaseElement: model.BaseElement{ID: "e-item"}, Name: "Item", Persistable: true}, + {BaseElement: model.BaseElement{ID: "e-stats"}, Name: "Stats", Persistable: true, + Source: "DomainModels$OqlViewEntitySource"}, + }, + Associations: []*domainmodel.Association{ + {BaseElement: model.BaseElement{ID: "a-item"}, Name: "Item_Customer", ParentID: "e-item", ChildID: "e-cust", + Type: domainmodel.AssociationTypeReference, Owner: domainmodel.AssociationOwnerDefault}, + {BaseElement: model.BaseElement{ID: "a-stats"}, Name: "Stats_Customer", ParentID: "e-stats", ChildID: "e-cust", + Type: domainmodel.AssociationTypeReference, Owner: domainmodel.AssociationOwnerDefault, + Source: domainmodel.OqlViewAssociationSource, ViewSourceReference: "Stats_Customer"}, + }, + }}, nil + }, + } + ctx, _ := newMockCtx(t, withBackend(b)) + ctx.widgetRegistry = &WidgetRegistry{byMDLName: map[string]*WidgetDefinition{}, byWidgetID: map[string]*WidgetDefinition{}} + ctx.widgetRegistryLoaded = true + return ctx +} + +func viewAssocPage(param, widget, ds string) string { + return `create page C1611.P (title: 'P', layout: Atlas_Core.Atlas_Default, params: ($P: C1611.` + param + `)) { + dataview dv (datasource: $P) { + ` + widget + ` w (datasource: ` + ds + `) { } + } +}` +} + +// mendixlabs/mxcli#1611 A: a page data source that follows a view entity's +// association from the entity it points at passed check and exec, then mxbuild +// failed CE8812 "Entity 'M.Owner' has no association 'M.Stats_Owner'". Each +// case is one measured shape (11.14.0, one page per shape); the controls built +// at 0 errors. +func TestViewAssociationReverse_MeasuredShapes(t *testing.T) { + ctx := viewAssocCtx(t) + for _, tc := range []struct { + name, param, widget, ds string + ce8812 bool + }{ + {"database from, the report", "Customer", "listview", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"in-memory association source", "Customer", "listview", "$currentObject/C1611.Stats_Customer", true}, + {"data grid", "Customer", "datagrid", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"gallery", "Customer", "gallery", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Stats", true}, + {"data view", "Customer", "dataview", "$currentObject/C1611.Stats_Customer", true}, + {"second hop", "Item", "listview", "DATABASE FROM $currentObject/C1611.Item_Customer/C1611.Customer/C1611.Stats_Customer/C1611.Stats", true}, + {"unqualified association", "Customer", "listview", "$currentObject/Stats_Customer", true}, + // controls + {"persistent association reversed", "Customer", "listview", "DATABASE FROM $currentObject/C1611.Item_Customer/C1611.Item", false}, + {"view association forward", "Stats", "dataview", "$currentObject/C1611.Stats_Customer", false}, + {"forward then a persistent reverse", "Stats", "listview", "DATABASE FROM $currentObject/C1611.Stats_Customer/C1611.Customer/C1611.Item_Customer/C1611.Item", false}, + {"the reporter's workaround", "Customer", "listview", "DATABASE C1611.Stats WHERE [C1611.Stats_Customer = $currentObject]", false}, + } { + t.Run(tc.name, func(t *testing.T) { + errs := assocListErrs(t, ctx, viewAssocPage(tc.param, tc.widget, tc.ds)) + var hits []string + for _, e := range errs { + if strings.Contains(e, "MDL-ASSOCDS02") { + hits = append(hits, e) + } + } + if !tc.ce8812 { + if len(errs) != 0 { + t.Errorf("want no error, got %q", errs) + } + return + } + if len(hits) != 1 || len(errs) != 1 { + t.Fatalf("want one MDL-ASSOCDS02, got %q", errs) + } + for _, want := range []string{"CE8812", "C1611.Stats_Customer", "view entity C1611.Stats", + "DATABASE C1611.Stats WHERE [C1611.Stats_Customer = "} { + if !strings.Contains(hits[0], want) { + t.Errorf("want %q in %q", want, hits[0]) + } + } + }) + } +} + +// The reporter's flow creates the view entity and the page in one script: the +// association comes from the view's OQL, not from a CREATE ASSOCIATION. +func TestViewAssociationReverse_ScriptDeclaredView(t *testing.T) { + src := `create view entity C1611.Totals (Total: Integer) as ( + from C1611.Item as i + join i/C1611.Item_Customer/C1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, count(i.ID) as Total +); +` + viewAssocPage("Customer", "listview", "DATABASE FROM $currentObject/C1611.Totals_Customer/C1611.Totals") + errs := assocListErrs(t, viewAssocCtx(t), src) + if len(errs) != 1 || !strings.Contains(errs[0], "MDL-ASSOCDS02") || !strings.Contains(errs[0], "C1611.Totals_Customer") { + t.Errorf("want one MDL-ASSOCDS02 for the script's view association, got %q", errs) + } +} + +// The remedy MDL-ASSOCDS02 names — query the view entity, constrained on its +// association — must itself pass when the view entity is created by the same +// script. It did not: the script's view associations were known to no checker, +// so `[C1611.Totals_Customer = $currentObject]` was refused as naming "neither +// an attribute nor an association", on a page and in a microflow retrieve, +// while mxbuild builds both at 0 errors (measured, 11.14.0). +func TestViewAssociationReverse_RemedyPassesForScriptDeclaredView(t *testing.T) { + src := `create view entity C1611.Totals (Total: Integer) as ( + from C1611.Item as i + join i/C1611.Item_Customer/C1611.Customer as c + group by c.ID + select c.ID as Totals_Customer, count(i.ID) as Total +); +` + viewAssocPage("Customer", "listview", "DATABASE C1611.Totals WHERE [C1611.Totals_Customer = $currentObject]") + `; +create microflow C1611.MF ($C: C1611.Customer) begin retrieve $L from C1611.Totals where C1611.Totals_Customer = $C; end;` + prog := parseMDL(t, src) + for _, err := range validateProgram(viewAssocCtx(t), prog) { + if strings.Contains(err.Error(), "Totals_Customer") { + t.Errorf("the remedy was refused: %v", err) + } + } +} diff --git a/mdl/executor/validate_widget_attribute_scope.go b/mdl/executor/validate_widget_attribute_scope.go index 877889afd5..e4a46fa189 100644 --- a/mdl/executor/validate_widget_attribute_scope.go +++ b/mdl/executor/validate_widget_attribute_scope.go @@ -114,6 +114,7 @@ func (v *attributeScopeValidator) walk(w *ast.WidgetV3, enclosing dataContext) { inner := childContext(enclosing, w.GetDataSource(), v.sigs, v.pageParams) v.checkInputBinding(w, enclosing) v.checkAssocListSource(w, enclosing) + v.checkViewAssocReverse(w, enclosing) v.checkReactUnsupported(w) if v.registry == nil { // No registry: the pluggable half cannot run, and a pluggable widget's From b39bd6b987b01428614622157fd462f87c70fd19 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:28:48 +0000 Subject: [PATCH 09/13] fix(executor): refuse a String-returning widget expression given a non-String literal (CE0117, mendixlabs/mxcli#1611) `column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') }` passed check and exec, then mx check failed with CE0117 "Error(s) in expression.". The filter's defaultValue is an Expression property that returns a String. A quoted MDL value on an expression property is the expression's text, so 'true' stored the Boolean expression `true`. The writer follows the convention that describe prints and every expression property relies on. What was missing is a check of the expression against the property's declared return type. MDL-WIDGET49 (validate_widget_expression_return.go) runs beside MDL-WIDGET42. On a String-returning Expression property it refuses a value that is a literal Boolean, Integer or Decimal (exprcheck.InferSourceKind), or a single bare name. The hint gives the String literal: `defaultValue: '''true'''`. The embedded schema now records each Expression property's ReturnType. A value that infers no kind is left alone (an attribute path, a variable, an untyped function), and so is DateTime, which the inference gets wrong. It runs with and without a project. Measured on 11.14.0, one filter per value. CE0117: 'true', 'false', '1' (in a String and in an Integer column), '1.5' and 'abc'. Builds at 0 errors: '''true''', '''abc''', '''tru''', 'empty' and '''a'' + ''b'''. check now reports exactly the six refused values. The bug-test file gives 0 errors from check and from mxbuild as written; with defaultValue 'true', MDL-WIDGET49 from check and CE0117 from mxbuild. Control: with the check unhooked, the 6 refusal cases fail with no error reported. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HxAn3VqxUnZ7ZGUnzAYpbT --- ...ter-default-value-not-a-string-ce0117.json | 1 + .../mendix/create-page/reference/widgets.md | 12 ++ .../1611-dropdownfilter-default-value.mdl | 40 ++++++ .../validate_widget_embedded_schema.go | 10 +- .../validate_widget_expression_return.go | 114 ++++++++++++++++++ .../validate_widget_expression_return_test.go | 76 ++++++++++++ mdl/executor/validate_widgets.go | 2 + 7 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json create mode 100644 mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl create mode 100644 mdl/executor/validate_widget_expression_return.go create mode 100644 mdl/executor/validate_widget_expression_return_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json new file mode 100644 index 0000000000..d3cc0c0265 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-22-dropdownfilter-default-value-not-a-string-ce0117.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-22","symptom":"`column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') }` passed check and exec, then mx check failed CE0117 \"Error(s) in expression.\" at the drop-down filter (mendixlabs/mxcli#1611 B).","cause":"defaultValue is a pluggable Expression property whose template declares ReturnType String, and a quoted MDL value on an Expression property is the expression's text, so 'true' stored the Boolean expression `true`. No check typed an authored expression against the property's declared return type; the embedded schema read each property's ValueType.Type but not its ReturnType.","fix":"MDL-WIDGET49 (validate_widget_expression_return.go), beside MDL-WIDGET42 in validateWidgetProperties: for a String-returning Expression property, refuse a value that infers as a literal Boolean, Integer or Decimal (exprcheck.InferSourceKind) or is one bare name, with the String-literal spelling as the hint (`defaultValue: '''true'''`). embeddedWidgetSchema.returns records ReturnType.Type per Expression key. Runs with and without a project (the DROPDOWNFILTER definition is built in).","file":"mdl/executor/validate_widget_expression_return.go","insight":"The writer was right and the check was missing: the MDL convention (a quoted value IS the expression text) is what describe prints and what every other expression property relies on, so 'wrap the string for a String-returning property' would have silently changed the meaning of every existing script. Measure the column type separately from the property's return type: '1' failed in an Integer column too — the return type is the widget's, fixed at String, not the attribute's. Judge only what InferSourceKind is sure of: it calls [%CurrentDateTime%] a String, so DateTime is not trusted; Unknown (paths, variables, untyped functions) is never refused.","refs":["mendixlabs/mxcli#1611","ako/mxcli#1539"],"ce":["CE0117"],"rules":["MDL-WIDGET49"]} diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index 7d24ce3668..b898f4542b 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -710,6 +710,18 @@ custom attribute (Studio Pro's "Attribute config: Custom"); listing two is refus attribute of the Data grid 2 column it sits in ("Auto"), so in a **gallery's** `filter { … }` bar, which has no column, the attribute must be named. +`defaultValue:` preselects an option. It is an **expression returning a String**, and a +quoted MDL value is the expression's text, so the option goes in as a String literal +inside the quotes, even for a Boolean column: + +```sql +column (Attribute: IsActive) { dropdownfilter ddfActive (defaultValue: '''true''') } +``` + +`defaultValue: 'true'` stores the Boolean expression `true`, and `'1'` or a bare name +like `'Open'` fail the same way: mxbuild reports CE0117 "Error(s) in expression." `check` +reports it as MDL-WIDGET49. `'empty'` (no preselection) is fine. + Filter by an **association** instead of an attribute — the options are the associated objects. Giving the filter a `datasource:` (the OPTION list) selects this mode; all three parts are required: diff --git a/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl b/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl new file mode 100644 index 0000000000..2689cf288c --- /dev/null +++ b/mdl-examples/bug-tests/1611-dropdownfilter-default-value.mdl @@ -0,0 +1,40 @@ +mdl 1; +-- ============================================================================ +-- mendixlabs/mxcli#1611 B — a drop-down filter's defaultValue of the wrong kind +-- (CE0117) +-- ============================================================================ +-- +-- Reported (v0.27.0, Mendix 11.12.5): +-- column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') } +-- passed check and exec, then mx check failed CE0117 "Error(s) in expression." +-- +-- defaultValue is an Expression returning a String, and a quoted MDL value on +-- an expression property IS the expression's text: 'true' stores the Boolean +-- expression `true`. Measured on 11.14.0, one filter per value: 'true', +-- 'false', '1' (in a String and in an Integer column), '1.5' and 'abc' are +-- CE0117; '''true''', '''abc''', '''tru''', 'empty' and '''a'' + ''b''' build. +-- check now reports the refused values as MDL-WIDGET49, with or without a +-- project. +-- +-- This file is the shape that must PASS: the option as a String literal inside +-- the quotes. As written: 0 errors from check and from mxbuild. With the first +-- filter's defaultValue changed to 'true': MDL-WIDGET49 from check, CE0117 from +-- mxbuild. +-- ============================================================================ + +CREATE MODULE Bug1611Ddf; + +CREATE OR MODIFY PERSISTENT ENTITY Bug1611Ddf.Task ( + Title: String(100), + IsActive: Boolean DEFAULT true +); + +CREATE OR MODIFY PAGE Bug1611Ddf.Task_Overview ( + Title: 'Tasks', + Layout: Atlas_Core.Atlas_Default +) { + datagrid dgTasks (DataSource: DATABASE Bug1611Ddf.Task) { + column (Attribute: IsActive) { dropdownfilter ddfActive (defaultValue: '''true''') } + column (Attribute: Title) { textfilter tfTitle } + } +}; diff --git a/mdl/executor/validate_widget_embedded_schema.go b/mdl/executor/validate_widget_embedded_schema.go index 1e8737a83b..116f058df0 100644 --- a/mdl/executor/validate_widget_embedded_schema.go +++ b/mdl/executor/validate_widget_embedded_schema.go @@ -44,6 +44,9 @@ type embeddedWidgetSchema struct { // types maps a lower-cased key to its declared type ("Expression", // "TextTemplate", "Boolean", …). types map[string]string + // returns maps a lower-cased Expression key to its declared return type + // ("String", "Boolean", …); absent when the template declares none. + returns map[string]string // slotAccepts are the widget types an item's widgets slot routes, by slot // key (a column's `filter` → textfilter, …); empty for a widget's own schema. slotAccepts map[string][]string @@ -143,7 +146,7 @@ func loadEmbeddedSchema(widgetID, listKey string) *embeddedWidgetSchema { var schema *embeddedWidgetSchema if tmpl, err := widgets.GetTemplate(widgetID); err == nil && tmpl != nil { if propTypes := embeddedTemplatePropertyTypes(tmpl.Type, listKey); len(propTypes) > 0 { - schema = &embeddedWidgetSchema{Title: tmpl.Name, Version: tmpl.Version, types: map[string]string{}} + schema = &embeddedWidgetSchema{Title: tmpl.Name, Version: tmpl.Version, types: map[string]string{}, returns: map[string]string{}} if listKey != "" { schema.Title += " column" schema.slotAccepts = itemSlotAcceptedChildTypes[widgetID][listKey] @@ -156,6 +159,11 @@ func loadEmbeddedSchema(widgetID, listKey string) *embeddedWidgetSchema { kind := "" if vt, ok := pt["ValueType"].(map[string]any); ok { kind, _ = vt["Type"].(string) + if rt, ok := vt["ReturnType"].(map[string]any); ok && kind == "Expression" { + if ret, _ := rt["Type"].(string); ret != "" { + schema.returns[strings.ToLower(key)] = ret + } + } } schema.Names = append(schema.Names, key) schema.types[strings.ToLower(key)] = kind diff --git a/mdl/executor/validate_widget_expression_return.go b/mdl/executor/validate_widget_expression_return.go new file mode 100644 index 0000000000..f4b374a427 --- /dev/null +++ b/mdl/executor/validate_widget_expression_return.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "regexp" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/exprcheck" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// validateWidgetExpressionReturnTypes is MDL-WIDGET49: a pluggable widget's +// Expression property, declared to return a String, given an expression of +// another kind. +// +// A quoted MDL value on an Expression property IS the expression's text, so +// `dropdownfilter ddf (defaultValue: 'true')` stores the expression `true` — a +// Boolean where the drop-down filter's `defaultValue` returns a String. check +// and exec passed it and mxbuild failed CE0117 "Error(s) in expression." +// (mendixlabs/mxcli#1611 B). Measured on 11.14.0, one filter per value: +// +// 'true', 'false', '1', '1.5', 'abc' → CE0117 (Boolean, Integer, Decimal, a bare name) +// '''true''', '''abc''', 'empty' → ok +// +// Only what is certain is judged: a String-returning property, and a value whose +// kind is a literal Boolean, Integer or Decimal, or that is one bare name (which +// is no Mendix expression at all). Anything that infers no kind — an attribute +// path, a variable, a function the checker does not type — is left alone. +func validateWidgetExpressionReturnTypes(w *ast.WidgetV3, def *WidgetDefinition, + embedded *embeddedWidgetSchema, locationPrefix string) []linter.Violation { + if w == nil || len(w.Properties) == 0 { + return nil + } + schema := embedded + if def != nil { + schema = loadEmbeddedSchema(def.WidgetID, "") + } + if schema == nil || len(schema.returns) == 0 { + return nil + } + keys := make([]string, 0, len(w.Properties)) + for k := range w.Properties { + keys = append(keys, k) + } + sort.Strings(keys) + + var out []linter.Violation + for _, key := range keys { + lk := strings.ToLower(key) + if schema.types[lk] != "Expression" || schema.returns[lk] != "String" { + continue + } + var text string + switch v := w.Properties[key].(type) { + case string: + text = v + case *ast.WidgetExpressionV3: + text = v.Text + default: + continue + } + text = strings.TrimSpace(text) + got := "" + switch { + case bareExpressionNameRe.MatchString(text) && !isExpressionKeyword(text): + got = "a bare name, which is no expression" + default: + switch exprcheck.InferSourceKind(text, nil) { + case exprcheck.KindBoolean: + got = "a Boolean" + case exprcheck.KindInteger, exprcheck.KindLong: + got = "an Integer" + case exprcheck.KindDecimal: + got = "a Decimal" + } + } + if got == "" { + continue + } + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET49", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` property `%s` is an expression returning a String, and `%s` is %s — "+ + "mxbuild rejects it with CE0117 \"Error(s) in expression.\" A quoted value on an expression "+ + "property is the expression's text, not a string", + locationPrefix, widgetLabelForError(w), key, text, got), + Suggestion: fmt.Sprintf("Write the value as a String literal inside the quotes: `%s: %s`.", + key, mdlStringOfExpressionLiteral(text)), + }) + } + return out +} + +var bareExpressionNameRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + +func isExpressionKeyword(s string) bool { + switch strings.ToLower(s) { + case "true", "false", "empty": + return true + } + return false +} + +// mdlStringOfExpressionLiteral is the MDL spelling of the expression 'text': the +// Mendix string literal, quoted again for MDL with every apostrophe doubled. +func mdlStringOfExpressionLiteral(text string) string { + literal := "'" + strings.ReplaceAll(text, "'", "''") + "'" + return "'" + strings.ReplaceAll(literal, "'", "''") + "'" +} diff --git a/mdl/executor/validate_widget_expression_return_test.go b/mdl/executor/validate_widget_expression_return_test.go new file mode 100644 index 0000000000..8421fb0567 --- /dev/null +++ b/mdl/executor/validate_widget_expression_return_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// mendixlabs/mxcli#1611 B: `dropdownfilter ddf (defaultValue: 'true')` in a +// Boolean column passed check and exec, then mxbuild failed CE0117 "Error(s) in +// expression." The property is an Expression returning String, and a quoted MDL +// value IS the expression text, so `'true'` stores the expression `true` — a +// Boolean. Each case is one measured value (11.14.0, one filter per value, the +// column type beside it); the controls built at 0 errors. +func TestWidgetExpressionReturnType_DropdownFilterDefault(t *testing.T) { + for _, tc := range []struct { + name, attr, value string + ce0117 bool + }{ + {"the report: Boolean literal", "IsActive", `'true'`, true}, + {"false", "IsActive", `'false'`, true}, + {"bare name", "Label", `'abc'`, true}, + {"Integer literal", "Label", `'1'`, true}, + {"Integer literal in an Integer column", "Size", `'1'`, true}, + {"Decimal literal", "Label", `'1.5'`, true}, + // controls + {"the workaround: a String literal", "IsActive", `'''true'''`, false}, + {"String literal", "Label", `'''abc'''`, false}, + {"a String matching no option", "IsActive", `'''tru'''`, false}, + {"empty", "Label", `'empty'`, false}, + {"a String expression", "Label", `'''a'' + ''b'''`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + src := `create page F.P (Title: 'P', Layout: Atlas_Core.Atlas_Default) { + datagrid dg (DataSource: DATABASE F.Thing) { + column (Attribute: ` + tc.attr + `) { dropdownfilter ddf (defaultValue: ` + tc.value + `) } + } +};` + var hits []string + for _, v := range ValidateWidgetProperties(parseMDL(t, src), "") { + if v.RuleID == "MDL-WIDGET49" { + hits = append(hits, v.Message+" | "+v.Suggestion) + } + } + if !tc.ce0117 { + if len(hits) != 0 { + t.Errorf("want no MDL-WIDGET49, got %q", hits) + } + return + } + if len(hits) != 1 { + t.Fatalf("want one MDL-WIDGET49, got %q", hits) + } + for _, want := range []string{"defaultValue", "String", "CE0117"} { + if !strings.Contains(hits[0], want) { + t.Errorf("want %q in %q", want, hits[0]) + } + } + }) + } +} + +// The hint gives the spelling that builds: the value as a String literal. +func TestWidgetExpressionReturnType_HintQuotesTheLiteral(t *testing.T) { + src := `create page F.P (Title: 'P', Layout: Atlas_Core.Atlas_Default) { + datagrid dg (DataSource: DATABASE F.Thing) { + column (Attribute: IsActive) { dropdownfilter ddf (defaultValue: 'true') } + } +};` + for _, v := range ValidateWidgetProperties(parseMDL(t, src), "") { + if v.RuleID == "MDL-WIDGET49" && !strings.Contains(v.Suggestion, `defaultValue: '''true'''`) { + t.Errorf("hint does not give the String literal: %q", v.Suggestion) + } + } +} diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 0693d99bdc..a6d82e32d3 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -337,6 +337,8 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc out = append(out, validateWidgetExpressionValues(w, nil, mapping, true, locationPrefix)...) } else { out = append(out, validateWidgetExpressionValuesIn(w, def, nil, isObjectListItem, embedded, locationPrefix)...) + // An Expression property's value of the wrong kind (MDL-WIDGET49). + out = append(out, validateWidgetExpressionReturnTypes(w, def, embedded, locationPrefix)...) } // A generic widget type that resolved to nothing is already reported as // MDL-WIDGET25 (the kind is wrong). Validating its properties on top of From f1ccb59cd2df122d57994e268c3f4c18e96aea96 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:28:51 +0000 Subject: [PATCH 10/13] fix(alter page): keep an item's hidden slots and texts as CREATE stores them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on a blank 11.13.0 app, project checked as stored (`docker check --no-update-widgets`; the normalising default hides all of these): 1. `insert into acc1 item@1.headercontent { … }` on a group whose headerRenderMode is 'text' (the default) stored widgets the Accordion hides: CE0463 at the Accordion. It is now refused, at check and at exec, naming the property that shows the slot. A SET earlier in the same statement can be what shows it, so check stands down then and exec decides on the values after the SET. 2. `set (headerRenderMode: 'custom') on acc1 item@1` hid headerText and left its template filled: CE0463. CREATE stores a hidden template null (#1198), so the post-SET reset now nulls it. 3. `set (headerRenderMode: 'text')` on a group made in custom mode revealed a null template: CE0463, and an empty one is CE4899. The 'Header' text CREATE writes lives in the widget package, not the stored schema, so such a SET is refused unless it also sets headerText; `set (headerRenderMode: 'text', headerText: 'Two')` creates the template (Deps.SerializeClientTemplate, codec-built) and writes the text. The reset is ResetHiddenColumnProperties' pass, generalised to items; templates are handled for items only, since the column case is not measured. check's dry run now applies an item SET as a whole, as it does for a column's. Control: each new test fails before its change with the measured symptom; on the app, scenarios 1-3 give 1 error before and 0 after (3 by refusal, the project left unchanged), and set-then-insert gives 0. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- .../modelsdk/widget_pluggable_write.go | 6 + mdl/backend/object_list_item_values.go | 17 ++ .../pagemutator/column_hidden_reset.go | 100 ++++++++++- .../pagemutator/column_hidden_reset_test.go | 8 +- mdl/backend/pagemutator/mutator.go | 7 + .../object_list_item_address_test.go | 167 +++++++++++++++++- mdl/backend/pagemutator/objectlist_items.go | 70 ++++++++ mdl/backend/pagemutator/probe_test.go | 2 + mdl/executor/alter_hidden_item_slot.go | 114 ++++++++++++ mdl/executor/alter_hidden_item_slot_test.go | 53 ++++++ mdl/executor/cmd_alter_page.go | 50 +++++- mdl/executor/validate_alter_page_refs.go | 13 ++ mdl/executor/validate_alter_set.go | 2 +- mdl/executor/validate_alter_set_test.go | 2 + 14 files changed, 601 insertions(+), 10 deletions(-) create mode 100644 mdl/backend/object_list_item_values.go create mode 100644 mdl/executor/alter_hidden_item_slot.go create mode 100644 mdl/executor/alter_hidden_item_slot_test.go diff --git a/mdl/backend/modelsdk/widget_pluggable_write.go b/mdl/backend/modelsdk/widget_pluggable_write.go index 8fad2a6b05..99a2710e37 100644 --- a/mdl/backend/modelsdk/widget_pluggable_write.go +++ b/mdl/backend/modelsdk/widget_pluggable_write.go @@ -240,6 +240,12 @@ func (codecChildSerializer) SerializeCustomWidgetDataSource(ds pages.DataSource) return genToV1BSON(el) } +// SerializeClientTemplate builds an empty Forms$ClientTemplate through the +// codec, for a pluggable text-template property ALTER has to fill. +func (codecChildSerializer) SerializeClientTemplate() bson.D { + return genToV1BSON(clientTemplateToGen(nil)) +} + // genToV1BSON encodes a gen element with the codec (bson v2 bytes) and decodes it // back into a v1 bson.D for the shared (v1) builder. func genToV1BSON(el element.Element) bson.D { diff --git a/mdl/backend/object_list_item_values.go b/mdl/backend/object_list_item_values.go new file mode 100644 index 0000000000..a067d82d53 --- /dev/null +++ b/mdl/backend/object_list_item_values.go @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: Apache-2.0 + +package backend + +// ObjectListItemValues is what a page mutator reports about a stored +// object-list item addressed as `owner item…` (an Accordion group), so the +// executor can evaluate the widget's visibility rules against the values the +// item actually holds. Item and Widget map lower-cased property keys to their +// stored primitive or expression value. +type ObjectListItemValues struct { + WidgetID string + ListKey string + // BodySlot is the property key of the item's body slot (BodyItemSlotKey). + BodySlot string + Item map[string]string + Widget map[string]string +} diff --git a/mdl/backend/pagemutator/column_hidden_reset.go b/mdl/backend/pagemutator/column_hidden_reset.go index a2f04da0fa..6a62984595 100644 --- a/mdl/backend/pagemutator/column_hidden_reset.go +++ b/mdl/backend/pagemutator/column_hidden_reset.go @@ -53,14 +53,38 @@ func (m *Mutator) ResetHiddenColumnProperties(gridRef, columnRef string, rules [ } } return resetHiddenItemProps(col.widget, col.colPropKeys, col.colPropKinds, defaults, - primitiveValuesByKey(grid.widget, buildPropKeyMap(grid.widget)), listKey, rules, explicit) + primitiveValuesByKey(grid.widget, buildPropKeyMap(grid.widget)), listKey, rules, explicit, nil) +} + +// ResetHiddenItemProperties is ResetHiddenColumnProperties for an object-list +// item addressed as `owner item…` (an Accordion group). It also nulls a +// TextTemplate the SET now hides, because that is how CREATE stores one +// (#1198): `set (headerRenderMode: 'custom') on acc1 item@1` hid headerText, +// left its template filled, and the project as stored failed with CE0463 at +// the Accordion (11.13.0). Column templates are left as they were — that case +// is not measured. +func (m *Mutator) ResetHiddenItemProperties(itemRef string, rules []types.WidgetVisibilityRule, setProps []string) ([]string, error) { + item := m.widgetFinder(m.rawData, itemRef) + if item == nil || item.item == nil { + return nil, fmt.Errorf("%q is not an object-list item", itemRef) + } + owner := item.item.owner + explicit := make(map[string]bool, len(setProps)) + for _, p := range setProps { + if key := resolveColumnPropertyKey(p, item.colPropKeys); key != "" { + explicit[key] = true + } + } + return resetHiddenItemProps(item.widget, item.colPropKeys, item.colPropKinds, + columnPropDefaults(owner, item.item.listKey), + primitiveValuesByKey(owner, buildPropKeyMap(owner)), item.item.listKey, rules, explicit, m.newTemplate) } // resetHiddenItemProps is the document-level half, separate so it can be tested // without a page. itemKeys/itemKinds map TypePointer ids to schema keys and // value kinds; defaults and widgetValues are keyed by schema key. func resetHiddenItemProps(item bson.D, itemKeys, itemKinds, defaults, widgetValues map[string]string, - listKey string, rules []types.WidgetVisibilityRule, explicit map[string]bool) ([]string, error) { + listKey string, rules []types.WidgetVisibilityRule, explicit map[string]bool, newTemplate func() bson.D) ([]string, error) { reset := map[string]bool{} // A reset can hide another property in turn (`minWidth` back to "auto" // hides `minWidthLimit`), so repeat until nothing changes. Each pass only @@ -80,10 +104,48 @@ func resetHiddenItemProps(item bson.D, itemKeys, itemKinds, defaults, widgetValu v, ok := src[c.PropertyKey] return v, ok }) + key := rule.PropertyKey + isTemplate := newTemplate != nil && keyKind(itemKeys, itemKinds, key) == "TextTemplate" + if isTemplate && determinable && !fires && !templateFilled(item, itemKeys, key) { + // Visible, stored null. With a declared default it gets a + // template holding that; without one there is no text to + // write — an empty required text is CE4899, measured on + // 11.13.0 ("Property 'Groups/2/Text' is required") — and the + // default CREATE writes ('Header') lives in the widget + // package, not in the stored schema. So the author is asked + // for it, in the statement that reveals it. + if defaults[key] == "" { + return nil, fmt.Errorf( + "property %q is shown under this item's new configuration but holds no text — set it in the "+ + "same statement, e.g. `set (…, %s: '…') on `", key, key) + } + if tmpl := newTemplate(); tmpl != nil { + if val := itemPropValue(item, itemKeys, key); val != nil { + updateClientTemplateText(tmpl, defaults[key]) + bsonnav.DSet(val, "TextTemplate", tmpl) + reset[key] = true + changed = true + } + } + continue + } if !determinable || !fires { continue } - key := rule.PropertyKey + if isTemplate { + if !templateFilled(item, itemKeys, key) { + continue + } + if explicit[key] { + return nil, fmt.Errorf( + "property %q is hidden under this item's configuration (%s), so it cannot be set — "+ + "a hidden text with a value fails the build with CE0463", key, describeConditions(rule)) + } + nullTemplate(item, itemKeys, key) + reset[key] = true + changed = true + continue + } def, hasDefault := defaults[key] cur, isPrimitive := values[key] if !hasDefault || !isPrimitive || cur == def { @@ -204,3 +266,35 @@ func columnPropDefaults(widgetDoc bson.D, listKey string) map[string]string { } return out } + +// keyKind returns the value kind of the item property with schema key key. +func keyKind(keys, kinds map[string]string, key string) string { + for id, k := range keys { + if k == key { + return kinds[id] + } + } + return "" +} + +// itemPropValue returns the Value document of the item property key. +func itemPropValue(item bson.D, keys map[string]string, key string) bson.D { + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(item, "Properties")) { + if pd, ok := p.(bson.D); ok && keys[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] == key { + return bsonnav.DGetDoc(pd, "Value") + } + } + return nil +} + +// templateFilled reports whether the item property key holds a TextTemplate. +func templateFilled(item bson.D, keys map[string]string, key string) bool { + return bsonnav.DGetDoc(itemPropValue(item, keys, key), "TextTemplate") != nil +} + +// nullTemplate stores the item property key's TextTemplate as null. +func nullTemplate(item bson.D, keys map[string]string, key string) { + if val := itemPropValue(item, keys, key); val != nil { + bsonnav.DSet(val, "TextTemplate", nil) + } +} diff --git a/mdl/backend/pagemutator/column_hidden_reset_test.go b/mdl/backend/pagemutator/column_hidden_reset_test.go index 007d595459..1e11f66385 100644 --- a/mdl/backend/pagemutator/column_hidden_reset_test.go +++ b/mdl/backend/pagemutator/column_hidden_reset_test.go @@ -62,7 +62,7 @@ func widthRules() []types.WidgetVisibilityRule { func TestResetHiddenColumnProps1519_SizeGoesBackToDefault(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "auto") - got, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"width": true}) + got, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"width": true}, nil) if err != nil { t.Fatalf("reset: %v", err) } @@ -78,7 +78,7 @@ func TestResetHiddenColumnProps1519_SizeGoesBackToDefault(t *testing.T) { // minWidth, and minWidth back at "auto" hides minWidthLimit. func TestResetHiddenColumnProps1519_CascadesThroughDependentRules(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "manual") - if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil); err != nil { + if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil, nil); err != nil { t.Fatalf("reset: %v", err) } if v := fieldOf(t, col, idMinWidth, "PrimitiveValue"); v != "auto" { @@ -92,7 +92,7 @@ func TestResetHiddenColumnProps1519_CascadesThroughDependentRules(t *testing.T) // CONTROL: under "manual" size is visible and keeps the author's value. func TestResetHiddenColumnProps1519_VisibleSizeIsKept(t *testing.T) { col, keys, kinds := widthColumn("manual", "auto") - if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil); err != nil { + if _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), nil, nil); err != nil { t.Fatalf("reset: %v", err) } if v := fieldOf(t, col, idSize, "PrimitiveValue"); v != "220" { @@ -105,7 +105,7 @@ func TestResetHiddenColumnProps1519_VisibleSizeIsKept(t *testing.T) { // refuses it on create. Its default is still accepted. func TestResetHiddenColumnProps1519_NamedHiddenPropertyIsRefused(t *testing.T) { col, keys, kinds := widthColumn("autoFill", "auto") - _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"size": true}) + _, err := resetHiddenItemProps(col, keys, kinds, columnDefaults, nil, "columns", widthRules(), map[string]bool{"size": true}, nil) if err == nil || !strings.Contains(err.Error(), "CE0463") || !strings.Contains(err.Error(), `"size"`) { t.Fatalf("want a refusal naming size and CE0463, got %v", err) } diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index e33c0e6935..8517c02fab 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -38,6 +38,10 @@ type Deps interface { SerializeClientAction(a pages.ClientAction) bson.D // SerializeCustomWidgetDataSource converts a pluggable-widget data source to raw bson.D. SerializeCustomWidgetDataSource(ds pages.DataSource) bson.D + // SerializeClientTemplate builds an empty Forms$ClientTemplate, for a + // text-template property that is stored null and has to hold text — the + // codec's shape, not one assembled here. + SerializeClientTemplate() bson.D // BuildDataGrid2Column builds a DataGrid2 column object (raw bson.D) from a // column spec, given the column object's type ID and per-property type IDs. // Returns an error if the engine does not support DataGrid2 column ALTER (so @@ -122,6 +126,9 @@ func (m *Mutator) SetWidgetProperty(widgetRef string, prop string, value any) er if n := m.columnMatchCount(widgetRef); n > 1 { return columnAmbiguityError(widgetRef, n) } + if result.item != nil { + m.ensureItemTemplate(result, prop) + } return setColumnPropertyMut(result.widget, result.colPropKeys, result.colPropKinds, prop, value) } return setRawWidgetPropertyMut(result.widget, prop, value) diff --git a/mdl/backend/pagemutator/object_list_item_address_test.go b/mdl/backend/pagemutator/object_list_item_address_test.go index 7fd91474ee..bdc56bc58d 100644 --- a/mdl/backend/pagemutator/object_list_item_address_test.go +++ b/mdl/backend/pagemutator/object_list_item_address_test.go @@ -10,6 +10,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/backend" "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/types" "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/pages" ) @@ -99,7 +100,8 @@ func accordionWithGroups(groups ...bson.D) bson.D { {Key: "IsList", Value: true}, {Key: "ObjectType", Value: bson.D{{Key: "PropertyTypes", Value: bson.A{int32(2), pt(tpRenderMode, "headerRenderMode", "Enumeration"), - pt(tpHeaderText, "headerText", "TextTemplate"), + append(pt(tpHeaderText, "headerText", "TextTemplate")[:2], bson.E{Key: "ValueType", Value: bson.D{ + {Key: "Type", Value: "TextTemplate"}, {Key: "DefaultValue", Value: "Header"}}}), pt(tpHeaderSlot, "headerContent", "Widgets"), pt(tpContent, "content", "Widgets"), }}}}, @@ -290,3 +292,166 @@ func TestInsertBesideItemIsRefused(t *testing.T) { t.Errorf("a refused insert changed the group count to %d", n) } } + +// The values the hidden-slot refusal evaluates come from the stored item. +func TestObjectListItemValues(t *testing.T) { + m := twoGroupMutator() + v, ok := m.ObjectListItemValues("acc1 item@1") + if !ok { + t.Fatal("ObjectListItemValues(acc1 item@1) found nothing") + } + if v.WidgetID != "com.mendix.widget.web.accordion.Accordion" || v.ListKey != "groups" || v.BodySlot != "content" { + t.Errorf("got %+v", v) + } + if v.Item["headerrendermode"] != "custom" { + t.Errorf("item values = %v, want headerrendermode=custom", v.Item) + } + if _, ok := m.ObjectListItemValues("acc1"); ok { + t.Error("a widget that is not an item must report ok=false") + } +} + +// accordionHeaderRules is the Accordion's own rule pair: headerContent hidden +// while headerRenderMode is "text", headerText hidden while it is not. +func accordionHeaderRules() []types.WidgetVisibilityRule { + cond := func(op string) *types.WidgetVisibilityCondition { + return &types.WidgetVisibilityCondition{PropertyKey: "headerRenderMode", Operator: op, Value: "text", Scope: types.ConditionScopeItem} + } + return []types.WidgetVisibilityRule{ + {PropertyKey: "headerContent", ListPropertyKey: "groups", HiddenWhen: cond("eq")}, + {PropertyKey: "headerText", ListPropertyKey: "groups", HiddenWhen: cond("ne")}, + } +} + +// headerTextTemplate returns group g's stored headerText template, or nil. +func headerTextTemplate(t *testing.T, m *Mutator, g int) any { + t.Helper() + group := groupsOf(t, m)[g].(bson.D) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(group, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) == bsonnav.ExtractBinaryIDFromDoc(tpHeaderText) { + return bsonnav.DGet(bsonnav.DGetDoc(pd, "Value"), "TextTemplate") + } + } + t.Fatal("no headerText property") + return nil +} + +// `set (headerRenderMode: 'custom') on acc1 item@1` hides headerText. A hidden +// template must be null — what CREATE stores for a custom-mode group (#1198); +// leaving it filled was CE0463 at the Accordion on 11.13.0 (project as stored). +func TestSetItemPropertyNullsTheTemplateItHides(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(accordionGroup("One", nil, nil))), model.ID("u"), &stubWidgetDeps{}) + // Start in text mode, as a group created with headerText is. + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "custom"); err != nil { + t.Fatal(err) + } + if _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}); err != nil { + t.Fatalf("ResetHiddenItemProperties: %v", err) + } + if tmpl := headerTextTemplate(t, m, 0); tmpl != nil { + t.Errorf("headerText is hidden in custom mode and must be null, got %v", tmpl) + } +} + +// Naming the hidden property in the same SET is refused, as on create. +func TestSetItemPropertyRefusesAHiddenTemplateItNames(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(accordionGroup("One", nil, nil))), model.ID("u"), &stubWidgetDeps{}) + _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode", "headerText"}) + if err == nil || !strings.Contains(err.Error(), "headerText") { + t.Fatalf("want a refusal naming headerText, got %v", err) + } +} + +// templateDeps builds an empty Forms$ClientTemplate, as the codec does. +type templateDeps struct{ stubWidgetDeps } + +func (templateDeps) SerializeClientTemplate() bson.D { + return bson.D{ + {Key: "$Type", Value: "Forms$ClientTemplate"}, + {Key: "Fallback", Value: bson.D{{Key: "$Type", Value: "Texts$Text"}, {Key: "Items", Value: bson.A{int32(3)}}}}, + {Key: "Parameters", Value: bson.A{int32(2)}}, + {Key: "Template", Value: bson.D{{Key: "$Type", Value: "Texts$Text"}, {Key: "Items", Value: bson.A{int32(3)}}}}, + } +} + +// customGroup is a group as CREATE stores one in custom mode: headerText null. +func customGroup() bson.D { + g := accordionGroup("unused", nil, nil) + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(g, "Properties")) { + pd := p.(bson.D) + if bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer")) == bsonnav.ExtractBinaryIDFromDoc(tpHeaderText) { + bsonnav.DSet(bsonnav.DGetDoc(pd, "Value"), "TextTemplate", nil) + } + } + return g +} + +// Switching a custom-mode group back to 'text' makes headerText visible with +// a null template — CE0463 the other way round. With a declared default the +// reset fills it with that, as CREATE writes a visible headerText. +func TestSetItemPropertyFillsTheTemplateItReveals(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(customGroup())), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}); err != nil { + t.Fatalf("ResetHiddenItemProperties: %v", err) + } + if headerTextTemplate(t, m, 0) == nil { + t.Fatal("a visible headerText must not stay null") + } + if err := m.InsertWidget("acc1 item('Header')", "", into, itemButton("b")); err != nil { + t.Errorf("the revealed headerText should hold the schema default 'Header': %v", err) + } +} + +// Setting the text of an item whose template is null creates the template. +func TestSetItemTextCreatesANullTemplate(t *testing.T) { + m := New(makeRawPage(accordionWithGroups(customGroup())), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + if err := m.SetWidgetProperty("acc1 item@1", "headerText", "Two"); err != nil { + t.Fatalf("set headerText on a null template: %v", err) + } + if err := m.InsertWidget("acc1 item('Two')", "", into, itemButton("b")); err != nil { + t.Errorf("headerText 'Two' was not written: %v", err) + } +} + +// The real Accordion declares DefaultValue "" for headerText, so there is no +// text to fill in, and an empty required text is CE4899. The SET that reveals +// it must say so rather than write an empty template. +func TestSetItemPropertyRefusesARevealedTemplateWithNoText(t *testing.T) { + acc := accordionWithGroups(customGroup()) + // Blank out the fixture's declared default. + var clear func(v any) + clear = func(v any) { + switch d := v.(type) { + case bson.D: + for i := range d { + if d[i].Key == "DefaultValue" { + d[i].Value = "" + } + clear(d[i].Value) + } + case bson.A: + for _, e := range d { + clear(e) + } + } + } + clear(acc) + m := New(makeRawPage(acc), model.ID("u"), &templateDeps{}) + if err := m.SetWidgetProperty("acc1 item@1", "headerRenderMode", "text"); err != nil { + t.Fatal(err) + } + _, err := m.ResetHiddenItemProperties("acc1 item@1", accordionHeaderRules(), []string{"headerRenderMode"}) + if err == nil || !strings.Contains(err.Error(), "headerText") { + t.Fatalf("want a refusal asking for headerText, got %v", err) + } +} diff --git a/mdl/backend/pagemutator/objectlist_items.go b/mdl/backend/pagemutator/objectlist_items.go index 1ee906b61b..4369233d3c 100644 --- a/mdl/backend/pagemutator/objectlist_items.go +++ b/mdl/backend/pagemutator/objectlist_items.go @@ -285,3 +285,73 @@ func objectListItemHint(owner *bsonWidgetResult, name string) string { return fmt.Sprintf("its widgets live in its %s: `insert into %s item@1 { … }` fills the first one, "+ "`%s item('')` addresses one by its text", lists[0].key, name, name) } + +// ObjectListItemValues reports a stored item's values for visibility rules. +// ok is false when itemRef does not resolve to an object-list item. +func (m *Mutator) ObjectListItemValues(itemRef string) (backend.ObjectListItemValues, bool) { + result := m.widgetFinder(m.rawData, itemRef) + if result == nil || result.item == nil { + return backend.ObjectListItemValues{}, false + } + ownerKeys := buildPropKeyMap(result.item.owner) + return backend.ObjectListItemValues{ + WidgetID: pluggableWidgetID(result.item.owner), + ListKey: result.item.listKey, + BodySlot: types.BodyItemSlotKey(result.item.slotKeys), + Item: storedScalarValues(result.widget, result.colPropKeys), + Widget: storedScalarValues(bsonnav.DGetDoc(result.item.owner, "Object"), ownerKeys), + }, true +} + +// storedScalarValues maps each property of a WidgetObject (keys: TypePointer +// id -> property key) to its stored PrimitiveValue, or its Expression when it +// has no primitive one. +func storedScalarValues(obj bson.D, keys map[string]string) map[string]string { + out := map[string]string{} + for _, p := range bsonnav.DGetArrayElements(bsonnav.DGet(obj, "Properties")) { + pd, ok := p.(bson.D) + if !ok { + continue + } + key := keys[bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(pd, "TypePointer"))] + val := bsonnav.DGetDoc(pd, "Value") + if key == "" || val == nil { + continue + } + v := bsonnav.DGetString(val, "PrimitiveValue") + if v == "" { + v = bsonnav.DGetString(val, "Expression") + } + if v != "" { + out[strings.ToLower(key)] = v + } + } + return out +} + +// newTemplate returns an empty client template from the engine, or nil when +// the engine has none to give. +func (m *Mutator) newTemplate() bson.D { + if m.deps == nil { + return nil + } + return m.deps.SerializeClientTemplate() +} + +// ensureItemTemplate gives an item's text-template property a template when +// it is stored null, so `set (headerText: 'Two') on acc1 item@2` can write a +// group CREATE made in custom mode. Whether the text may be set at all — it +// may be hidden — is the reset pass's to say afterwards, with the refusal it +// gives a hidden property the statement named. +func (m *Mutator) ensureItemTemplate(result *bsonWidgetResult, prop string) { + key := resolveColumnPropertyKey(prop, result.colPropKeys) + if key == "" || keyKind(result.colPropKeys, result.colPropKinds, key) != "TextTemplate" || + templateFilled(result.widget, result.colPropKeys, key) { + return + } + if tmpl := m.newTemplate(); tmpl != nil { + if val := itemPropValue(result.widget, result.colPropKeys, key); val != nil { + bsonnav.DSet(val, "TextTemplate", tmpl) + } + } +} diff --git a/mdl/backend/pagemutator/probe_test.go b/mdl/backend/pagemutator/probe_test.go index b7a8b0c630..780b182054 100644 --- a/mdl/backend/pagemutator/probe_test.go +++ b/mdl/backend/pagemutator/probe_test.go @@ -17,6 +17,8 @@ type recordingDeps struct{ saves int } func (d *recordingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *recordingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } +func (d *recordingDeps) SerializeClientTemplate() bson.D { return nil } + func (d *recordingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil } diff --git a/mdl/executor/alter_hidden_item_slot.go b/mdl/executor/alter_hidden_item_slot.go new file mode 100644 index 0000000000..6c55479639 --- /dev/null +++ b/mdl/executor/alter_hidden_item_slot.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/types" +) + +// objectListItemValuer is the page mutator capability the hidden-slot check +// needs; engines without it are not checked. +type objectListItemValuer interface { + ObjectListItemValues(itemRef string) (backend.ObjectListItemValues, bool) +} + +// refuseHiddenItemSlot refuses an INSERT or REPLACE that writes widgets into a +// slot of an object-list item that the widget's own configuration hides — an +// Accordion group's headerContent while headerRenderMode is 'text'. Such +// widgets are stored but never rendered, and the project as stored fails +// with CE0463 at the widget (PROPOSAL_alter_object_list_item_address.md). +// A bare item address writes into the body slot, so that is checked too. +// +// Called by exec and by check's target validation, so the refusal reaches +// the author before anything is written. +func refuseHiddenItemSlot(ctx *ExecContext, mutator any, ref ast.WidgetRef) error { + if ref.Item == nil { + return nil + } + valuer, ok := mutator.(objectListItemValuer) + if !ok { + return nil + } + vals, ok := valuer.ObjectListItemValues(ref.Widget) + if !ok { + return nil + } + registry := ctx.GetWidgetRegistry() + if registry == nil { + return nil + } + def, ok := registry.GetByWidgetID(vals.WidgetID) + if !ok { + return nil + } + slot := ref.Column + if slot == "" { + slot = vals.BodySlot + } + return itemSlotHiddenError(visibilityRulesFor(def, registry), ref.Widget, vals.ListKey, slot, vals.Item, vals.Widget) +} + +// itemSlotHiddenError evaluates the nested visibility rules for one item slot. +// A rule that cannot be decided from the stored values refuses nothing. +func itemSlotHiddenError(rules []types.WidgetVisibilityRule, itemRef, listKey, slot string, + itemValues, widgetValues map[string]string) error { + for _, rule := range rules { + if !rule.Nested() || rule.HiddenWhen == nil || + !strings.EqualFold(rule.ListPropertyKey, listKey) || !strings.EqualFold(rule.PropertyKey, slot) { + continue + } + fires, determinable := rule.Fires(func(c types.WidgetVisibilityCondition) (string, bool) { + values := widgetValues + if c.Scope == types.ConditionScopeItem { + values = itemValues + } + v, ok := values[strings.ToLower(c.PropertyKey)] + return v, ok + }) + if !determinable || !fires { + continue + } + var why []string + for _, c := range rule.Conditions() { + v := itemValues[strings.ToLower(c.PropertyKey)] + if c.Scope != types.ConditionScopeItem { + v = widgetValues[strings.ToLower(c.PropertyKey)] + } + why = append(why, fmt.Sprintf("%s is '%s'", c.PropertyKey, v)) + } + return fmt.Errorf("cannot write widgets into %s.%s: %s is hidden while %s, so the widgets would be stored "+ + "but not rendered, and the widget fails with CE0463 — change %s on %s first (`set (…) on %s`)", + itemRef, strings.ToLower(slot), rule.PropertyKey, strings.Join(why, " and "), + conditionKeys(rule), itemRef, itemRef) + } + return nil +} + +func conditionKeys(rule types.WidgetVisibilityRule) string { + var keys []string + for _, c := range rule.Conditions() { + keys = append(keys, c.PropertyKey) + } + return strings.Join(keys, ", ") +} + +// refuseHiddenItemSlotFor is refuseHiddenItemSlot for the operations that +// write widgets into a target: INSERT INTO and REPLACE of an item slot. +func refuseHiddenItemSlotFor(ctx *ExecContext, mutator any, op ast.AlterPageOperation, ref ast.WidgetRef) error { + switch o := op.(type) { + case *ast.InsertWidgetOp: + if strings.EqualFold(o.Position, "INTO") { + return refuseHiddenItemSlot(ctx, mutator, ref) + } + case *ast.ReplaceWidgetOp: + if ref.Column != "" { + return refuseHiddenItemSlot(ctx, mutator, ref) + } + } + return nil +} diff --git a/mdl/executor/alter_hidden_item_slot_test.go b/mdl/executor/alter_hidden_item_slot_test.go new file mode 100644 index 0000000000..1fc485dc4b --- /dev/null +++ b/mdl/executor/alter_hidden_item_slot_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// An Accordion hides a group's headerContent while headerRenderMode is +// 'text', the default (`"text"===n.headerRenderMode?hidePropertyIn(…, +// "groups",…,"headerContent")`). Writing widgets there stored content Studio +// Pro does not render, and the project as stored failed with CE0463 at the +// Accordion — measured on a blank 11.13.0 app with --no-update-widgets: 0 +// errors before the write, 1 after; the normalising default check hid it +// (PROPOSAL_alter_object_list_item_address.md, "a slot the item's +// configuration hides is refused"). +func TestHiddenItemSlotIsRefused(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", + map[string]string{"headerrendermode": "text"}, nil) + if err == nil { + t.Fatal("a write into headerContent of a text-mode group was not refused") + } + for _, want := range []string{"headerContent", "headerRenderMode", "'text'", "acc1 item@1"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the refusal should name %s: %v", want, err) + } + } +} + +func TestVisibleItemSlotIsAccepted(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", + map[string]string{"headerrendermode": "custom"}, nil); err != nil { + t.Errorf("headerContent of a custom-mode group must be writable: %v", err) + } + // The body is never hidden by this rule, whatever the mode. + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "content", + map[string]string{"headerrendermode": "text"}, nil); err != nil { + t.Errorf("content must be writable in text mode: %v", err) + } +} + +// A value the stored item does not carry is undeterminable, and an +// undeterminable rule refuses nothing — the same stance as check's +// validateWidgetItemVisibility. +func TestUndeterminableItemSlotIsAccepted(t *testing.T) { + rules, _ := extractVisibilityRulesFromJS(accordionGetProperties) + if err := itemSlotHiddenError(rules, "acc1 item@1", "groups", "headercontent", nil, nil); err != nil { + t.Errorf("no stored headerRenderMode: %v", err) + } +} diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index c94154fbf1..27bdcbd781 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -350,17 +350,55 @@ func applySetPropertyMutator(ctx *ExecContext, mutator backend.PageMutator, op * if err := applySetProperties(ctx, mutator, op, moduleName, moduleID); err != nil { return err } - if op.Target.IsColumn() { + if op.Target.IsColumn() || op.Target.Item != nil { names := make([]string, 0, len(op.Properties)) for k := range op.Properties { names = append(names, k) } sort.Strings(names) + if op.Target.Item != nil { + return resetHiddenItemProperties(ctx, mutator, op.Target, names) + } return resetHiddenColumnProperties(ctx, mutator, op.Target, names) } return nil } +// itemHiddenPropertyResetter is the page mutator capability behind +// resetHiddenItemProperties; engines without it are left as they were. +type itemHiddenPropertyResetter interface { + ResetHiddenItemProperties(itemRef string, rules []types.WidgetVisibilityRule, setProps []string) ([]string, error) +} + +// resetHiddenItemProperties is resetHiddenColumnProperties for an object-list +// item (`acc1 item@1`): after the SET, every item property the widget's +// editorConfig now hides goes back to how CREATE stores a hidden one. +// `set (headerRenderMode: 'custom')` hid the group's headerText and left its +// template filled: CE0463 at the Accordion. The rules are the installed +// package's, found from the item's own widget id. +func resetHiddenItemProperties(ctx *ExecContext, mutator backend.PageMutator, target ast.WidgetRef, setProps []string) error { + resetter, ok := mutator.(itemHiddenPropertyResetter) + if !ok { + return nil + } + valuer, ok := mutator.(objectListItemValuer) + if !ok { + return nil + } + vals, ok := valuer.ObjectListItemValues(target.Widget) + if !ok { + return nil + } + rules := resolveWidgetVisibilityRules(ctx.MprPath, vals.WidgetID) + if len(rules) == 0 { + return nil + } + if _, err := resetter.ResetHiddenItemProperties(target.Widget, rules, setProps); err != nil { + return mdlerrors.NewBackend("set on "+target.Name(), err) + } + return nil +} + // applySetProperties writes each property of a SET, without the column // follow-up — what check's per-property dry run exercises, since a column's // hidden-property rules can only be judged on the statement as a whole. @@ -545,6 +583,11 @@ func convertASTAction(ctx *ExecContext, mutator backend.PageMutator, value any, // ============================================================================ func applyInsertWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op *ast.InsertWidgetOp, moduleName string, moduleID model.ID) error { + if strings.EqualFold(op.Position, "INTO") { + if err := refuseHiddenItemSlot(ctx, mutator, op.Target); err != nil { + return err + } + } opWidgets, err := expandAlterFragments(ctx, op.Widgets, moduleName, moduleID) if err != nil { return err @@ -686,6 +729,11 @@ func applyDropWidgetMutator(mutator backend.PageMutator, op *ast.DropWidgetOp) e // ============================================================================ func applyReplaceWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op *ast.ReplaceWidgetOp, moduleName string, moduleID model.ID, storedWidgets func() map[string]*ast.WidgetV3) error { + if op.Target.Item != nil && op.Target.Column != "" { + if err := refuseHiddenItemSlot(ctx, mutator, op.Target); err != nil { + return err + } + } newWidgets, err := expandAlterFragments(ctx, op.NewWidgets, moduleName, moduleID) if err != nil { return err diff --git a/mdl/executor/validate_alter_page_refs.go b/mdl/executor/validate_alter_page_refs.go index 0f955d2989..9c2d3b6df2 100644 --- a/mdl/executor/validate_alter_page_refs.go +++ b/mdl/executor/validate_alter_page_refs.go @@ -124,7 +124,11 @@ func validateAlterPageReferences(ctx *ExecContext, prog *ast.Program, sc *script // The targets, against the stored document, in the order exec // resolves them. + itemSetEarlier := map[string]bool{} for _, op := range s.Operations { + if set, ok := op.(*ast.SetPropertyOp); ok && set.Target.Item != nil { + itemSetEarlier[set.Target.Widget] = true + } // A SET's target is validateAlterSetProperties' finding. if _, isSet := op.(*ast.SetPropertyOp); mutator != nil && !isSet { for _, ref := range alterPageOperationTargets(op) { @@ -136,6 +140,15 @@ func validateAlterPageReferences(ctx *ExecContext, prog *ast.Program, sc *script } if _, err := mutator.ResolveAlterTarget(alterTargetOf(ref)); err != nil { errs = append(errs, mdlerrors.NewValidationf("%s: %v", label, err)) + } else if !itemSetEarlier[ref.Widget] { + // An earlier SET on the item in this statement may be what + // makes the slot visible (`set (headerRenderMode: 'custom')`, + // the very step the refusal recommends). This pass reads the + // stored document, so it cannot see that; exec checks again + // against the values after the SET. + if err := refuseHiddenItemSlotFor(ctx, mutator, op, ref); err != nil { + errs = append(errs, mdlerrors.NewValidationf("%s: %v", label, err)) + } } } } diff --git a/mdl/executor/validate_alter_set.go b/mdl/executor/validate_alter_set.go index 0700d008ff..0f85f9403d 100644 --- a/mdl/executor/validate_alter_set.go +++ b/mdl/executor/validate_alter_set.go @@ -171,7 +171,7 @@ func checkSetOp(ctx *ExecContext, p pageProbe, label string, op *ast.SetProperty // fine one property at a time and refused as a whole, since the width hides // the size, and `ColumnWidth: manual, Size: 220` is the reverse — judged // one at a time, Size alone would be hidden (mendixlabs/mxcli#1519). - if len(errs) == 0 && op.Target.IsColumn() { + if len(errs) == 0 && (op.Target.IsColumn() || op.Target.Item != nil) { if probe, err := p.Probe(); err == nil { if err := applySetPropertyMutator(ctx, probe, op, modName, modID); err != nil && !scriptDeclaresMissing(sc, err, modName) { errs = append(errs, mdlerrors.NewValidation(fmt.Sprintf("%s: %v", label, err))) diff --git a/mdl/executor/validate_alter_set_test.go b/mdl/executor/validate_alter_set_test.go index e40d0a93ef..07186bf6d2 100644 --- a/mdl/executor/validate_alter_set_test.go +++ b/mdl/executor/validate_alter_set_test.go @@ -72,6 +72,8 @@ type countingDeps struct{ saves int } func (d *countingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *countingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } +func (d *countingDeps) SerializeClientTemplate() bson.D { return nil } + func (d *countingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil } From faa20ca18188944bf84f41b35d12c8b2fd34855b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:31:41 +0000 Subject: [PATCH 11/13] docs: document object-list item addressing; regression script; finding - alter-page reference and skill: `acc1 item@n` / `item('text')` / `item(widget)` / `.headercontent`, the hidden-slot refusal and its fix, and what is not supported (adding a new item). - mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl: exec on a blank 11.13.0 app puts each widget in its slot and `docker check --no-update-widgets` reports 0 errors. - Proposal status -> partial, with what is done and what is not; proposals index regenerated. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- .../2026-10-22-object-list-item-address.json | 1 + .claude/skills/mendix/alter-page/SKILL.md | 9 +++++ docs-site/src/reference/page/alter-page.md | 40 +++++++++++++++++++ ...PROPOSAL_alter_object_list_item_address.md | 17 +++++++- docs/11-proposals/README.md | 6 +-- ...453-alter-accordion-group-item-address.mdl | 39 ++++++++++++++++++ 6 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json create mode 100644 mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl diff --git a/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json new file mode 100644 index 0000000000..20f300dc3f --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-backend/2026-10-22-object-list-item-address.json @@ -0,0 +1 @@ +{"area": "mdl/backend/pagemutator", "date": "2026-10-22", "symptom": "`insert into grp1 { … }` for an Accordion group: widget \"grp1\" not found — the group stores no Name; `insert into acc1 item@2 { … }` was refused as \"not a navigation list\"; an empty group could not be filled at all", "cause": "object-list items (CustomWidgets$WidgetObject) had no address; resolveNavItem accepted only navigation lists", "file": "`mdl/backend/pagemutator/objectlist_items.go` (resolveObjectListItem, item slots, ObjectListItemValues), `column_hidden_reset.go` (ResetHiddenItemProperties, template nulling/creation), `mdl/executor/alter_hidden_item_slot.go`; grammar `alterTarget` itemSlot", "insight": "The default `docker check` normalises widgets on a temp copy and HID every defect here — run `--no-update-widgets` (the project as stored, what MxBuild and run --local see) or a hidden-slot write and a mode-switching SET both look clean while each is CE0463. Diffing a SET-switched group against a CREATE-made one (TypePointer -> PropertyKey in the JSON dump) found the cause in one step: headerText template filled vs null. Two plausible wrong turns: assuming the schema DefaultValue holds the visible default (it is \"\" — 'Header' lives in the widget package, so a revealed text with no value is refused, not invented), and a check-time hidden-slot refusal that blocked `set (headerRenderMode: 'custom'); insert into …headercontent` — the very fix its own message recommended — because check reads the stored document; it stands down when the statement sets the item first, and exec decides on live values. The item result carries the item schema like a DataGrid2 column, so SET and `.slot` reused existing paths rather than growing new ones", "refs": ["mendixlabs/mxcli#1453", "ako/mxcli#1326"], "ce": ["CE0463", "CE4899"]} diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index 3538b76230..30d3fa4b5a 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -678,6 +678,15 @@ page and bind the buttons at creation time instead of rewiring afterwards: view: `replace dvMain.footer with { footer { … } }`, `insert into dvMain.footer { … }`, `drop dvMain.footer`. +**An Accordion group (any pluggable object-list item) has no name either.** + `group g1 (…)` keeps no `g1`, so address the group on its widget: + `insert into acc1 item@2 { … }` fills its body, `acc1 item('One')` finds it + by its headerText, `acc1 item(btnIn)` by a widget inside it, and + `acc1 item@1.headercontent` names its header slot. The header slot is hidden + — and refused — while `headerRenderMode` is `'text'`: write + `set (headerRenderMode: 'custom') on acc1 item@1;` first. Adding a new group + with `insert after acc1 item@1` is not supported. + **A pluggable widget's slot has no name either.** A Data grid 2's control bar (`controlbar { … }` in describe) is a property of the grid, so address it by the grid: `insert into dataGrid1.controlbar { actionbutton … }`, `replace diff --git a/docs-site/src/reference/page/alter-page.md b/docs-site/src/reference/page/alter-page.md index ca8dae3c00..39caf9304c 100644 --- a/docs-site/src/reference/page/alter-page.md +++ b/docs-site/src/reference/page/alter-page.md @@ -159,6 +159,46 @@ alter snippet Shop.Menu { Only items go next to an item or in its place: `INSERT BEFORE` / `AFTER` an item and `REPLACE` of one take `item` blocks, and widgets go `INTO` an item. +### Pluggable Widget Item Operations + +A pluggable widget's object-list item — an Accordion group, a Pop-up menu +item — stores no name either (a name written on `group g1` is not kept), and is +addressed the same way, on the widget that owns the list: + +| Address | Means | +|---------|-------| +| `acc1 item@2` | the second group — `INSERT INTO` fills its body | +| `acc1 item('One')` | the group whose text (`headerText`) is `One` | +| `acc1 item(btnIn)` | the group holding the widget `btnIn` | +| `acc1 item@1.headercontent` | another slot of the group, by its keyword | + +The body is the slot `describe` prints as the item's children (`content`, or +the item's first widget slot); any other slot is named with `.slot`, spelled as +`describe` prints its block. + +```sql +mdl 1; +alter page Shop.Faq { + insert into acc1 item@2 { dynamictext txtAnswer (Content: 'Yes.') }; + set (headerRenderMode: 'custom') on acc1 item@1; + insert into acc1 item@1.headercontent { dynamictext txtHead (Content: 'Shipping') }; + replace acc1 item('Returns').content with { dynamictext txtRet (Content: '30 days') }; + drop acc1 item@3; +}; +``` + +- A slot the widget's configuration hides is refused, naming the property that + shows it: an Accordion hides a group's `headercontent` while + `headerRenderMode` is `'text'` (the default). Widgets written there are not + rendered and fail the build with CE0463. Set the property first — in the same + statement is fine, as above. +- A SET that switches a group's mode keeps the texts it hides or shows as + CREATE stores them. Switching back to `'text'` needs the text in the same + statement — `set (headerRenderMode: 'text', headerText: 'Shipping') on acc1 item@1` + — since the stored schema has no default to fill in. +- `INSERT BEFORE` / `AFTER` an item (adding a new group) and `REPLACE` of a + whole item are not supported; use `drop` and `create or modify page`. + ### DROP Removes one or more widgets by name. The widget and all its children are removed from the tree. diff --git a/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md b/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md index 716008b437..96f3475174 100644 --- a/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md +++ b/docs/11-proposals/PROPOSAL_alter_object_list_item_address.md @@ -1,12 +1,12 @@ --- title: ALTER PAGE Addressing of Pluggable Object-List Items -status: proposed +status: partial date: 2026-10-17 --- # Proposal: ALTER PAGE Addressing of Pluggable Object-List Items -**Status:** Proposed +**Status:** Partial **Date:** 2026-10-17 Refs ako/mxcli#1326, mendixlabs/mxcli#1453. @@ -120,6 +120,19 @@ Shared with the existing forms, so nothing new to learn: **item** inserts an item (`insert after acc1 item@1 { group (…) { … } }`), mirroring navigation-list items. +## Implementation Status + +Done: steps 1–3, 5–7 (describe slots #1565; shared body rule; grammar; the +resolver for every selector form; INSERT INTO an item's body or slot, +REPLACE / DROP of a slot, DROP of an item, SET of an item property; the +hidden-slot refusal, Open Question 2 answered as "refuse"). Measuring the SET +path found that a mode switch must also keep the texts it hides or shows as +CREATE stores them, and does. + +Not done: inserting a NEW item (`insert after acc1 item@1 { group … }`) is +refused; step 4's selector rename was not needed; legacy table cells stay +deferred. + ## Implementation Plan Order matters: describe has to stop misplacing header widgets before an address diff --git a/docs/11-proposals/README.md b/docs/11-proposals/README.md index eb01524dd5..5bcacf7d94 100644 --- a/docs/11-proposals/README.md +++ b/docs/11-proposals/README.md @@ -30,11 +30,12 @@ for display in this README): ## Active Proposals (118) -### In Progress (partial) (15) +### In Progress (partial) (16) | Proposal | Status | Summary | |----------|--------|---------| | [Add Styling Support to MDL Pages](page-styling-support.md) | Partial | Mendix has four styling mechanisms on every widget, all stored in a BSON Forms$Appearance object: | +| [ALTER PAGE Addressing of Pluggable Object-List Items](PROPOSAL_alter_object_list_item_address.md) | Partial | Refs ako/mxcli#1326, mendixlabs/mxcli#1453. | | [Atlas Design System — a skill for visually appealing Mendix apps](PROPOSAL_atlas_design_system.md) | Partial | mxcli can generate a fully-functional Mendix app, but the default output looks bland — it leans | | [Default styling — generated apps that look designed on first boot](PROPOSAL_default_styling.md) | Partial | mxcli new produces a blank Atlas app. | | [Eval Framework for mxcli + Claude Code](proposal-eval-framework.md) | Partial | An evaluation framework that systematically tests how well Claude Code + mxcli handles real-world Mendix app generation tasks. | @@ -59,11 +60,10 @@ for display in this README): | [MDL Language Critique and Beta Syntax Freeze](PROPOSAL_mdl_beta_syntax_freeze.md) | Accepted | Related: ADR-0003; resulting decisions ADR-0010, ADR-0011, ADR-0012; design-mdl-syntax skill, PROPOSAL_workflow_microflow_syntax_alignment.m | | [Size containers from their contents, not from a statement count](PROPOSAL_container_autosize.md) | Accepted | Closes the last open item of upstream #884 — | -### Proposed (41) +### Proposed (40) | Proposal | Status | Summary | |----------|--------|---------| -| [ALTER PAGE Addressing of Pluggable Object-List Items](PROPOSAL_alter_object_list_item_address.md) | Proposed | Refs ako/mxcli#1326, mendixlabs/mxcli#1453. | | [Bootstrap hooks that fetch the mxcli the project was built with](PROPOSAL_bootstrap_source.md) | Proposed | mxcli init writes .claude/bootstrap-mxcli.sh with a hard-coded download of | | [Built-in file database for run --local (HSQLDB)](PROPOSAL_run_local_builtin_database.md) | Proposed | Depends on: the Windows process-helper fix (PR #1147) — this builds on that branch. | | [Comprehensive MDL Syntax and Grammar Improvements (v2)](PROPOSAL_mdl_syntax_improvements_v2.md) | Proposed | This document consolidates and expands upon previous proposals, offering a unified set of syntax and grammar improvements for all major Mend | diff --git a/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl b/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl new file mode 100644 index 0000000000..c13ba803ba --- /dev/null +++ b/mdl-examples/bug-tests/1453-alter-accordion-group-item-address.mdl @@ -0,0 +1,39 @@ +mdl 1; +-- mendixlabs/mxcli#1453 / ako/mxcli#1326: an Accordion group stores no Name, +-- so `insert into grp1 { … }` never resolved and an empty group could not be +-- filled. Groups are now addressed on their widget, like navigation list +-- items: `acc1 item@n`, `acc1 item('')`, `acc1 item()`, +-- with `.headercontent` for the header slot +-- (PROPOSAL_alter_object_list_item_address.md). +-- +-- Expected (describe below): the first group's body holds txtOne and +-- txtOne2, its header slot txtHead; the second group holds tbName; and +-- `mxcli docker check --no-update-widgets` reports 0 errors. +-- +-- The header slot is hidden while headerRenderMode is 'text', so the SET +-- comes first; inserting into it without that SET is refused. + +create module Bug1453b; + +create persistent entity Bug1453b.Item (Name: string(50)); + +create page Bug1453b.P (title: 'P', layout: Atlas_Core.Atlas_Default, params: ( $Item: Bug1453b.Item )) { + dataview dv (DataSource: $Item) { + accordion acc1 (advancedMode: false, collapsible: true, expandBehavior: 'singleExpanded', animate: true, showIcon: 'right', animateIcon: true) { + group (headerRenderMode: 'text', headerText: 'One') { + dynamictext txtOne (Content: 'one') + } + group (headerRenderMode: 'custom') { + } + } + } +}; + +alter page Bug1453b.P { + insert into acc1 item@2 { textbox tbName (Label: 'Name', Attribute: Name) }; + insert into acc1 item('One') { dynamictext txtOne2 (Content: 'one more') }; + set (headerRenderMode: 'custom') on acc1 item@1; + insert into acc1 item@1.headercontent { dynamictext txtHead (Content: 'Header') } +}; + +describe page Bug1453b.P; From ba672029c9a854f3c8b1542a17651c157ed62bd0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:32:03 +0000 Subject: [PATCH 12/13] docs(skill): regenerate the alter-page skill index after the new section Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- .claude/skills/mendix/alter-page/SKILL.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index 30d3fa4b5a..f29ba43b5f 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -17,10 +17,10 @@ description: "Modify an existing page or snippet's widget tree in place with ALT | 482-560 | Examples | | 561-651 | DataGrid 2 columns: how to address them, and what you can set | | 652-663 | Common Mistakes | -| 664-692 | Limitations — prefer binding at page creation (ledger finding #45) | -| 693-700 | Validation Checklist | -| 701-710 | Related Commands | -| 711-715 | Related Skills | +| 664-701 | Limitations — prefer binding at page creation (ledger finding #45) | +| 702-709 | Validation Checklist | +| 710-719 | Related Commands | +| 720-724 | Related Skills | # ALTER PAGE / ALTER SNIPPET - Modify Existing Pages and Snippets From 17b896717e4818d973520c4e1d020276d4568937 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 15:43:12 +0000 Subject: [PATCH 13/13] chore: gofmt the test stubs; keep the alter-page skill under its 700-line budget TestLargeSkillsWereSplit refused the skill at 703 body lines after the item-address paragraph; it is now three lines, with the detail in the alter-page reference. The test-stub methods are gofmt-clean. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019httGxDh8UXt1Zoyh3Gh52 --- .claude/skills/mendix/alter-page/SKILL.md | 19 +++++++------------ mdl/backend/pagemutator/probe_test.go | 2 +- mdl/executor/validate_alter_set_test.go | 2 +- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index f29ba43b5f..250b5f3915 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -17,10 +17,10 @@ description: "Modify an existing page or snippet's widget tree in place with ALT | 482-560 | Examples | | 561-651 | DataGrid 2 columns: how to address them, and what you can set | | 652-663 | Common Mistakes | -| 664-701 | Limitations — prefer binding at page creation (ledger finding #45) | -| 702-709 | Validation Checklist | -| 710-719 | Related Commands | -| 720-724 | Related Skills | +| 664-696 | Limitations — prefer binding at page creation (ledger finding #45) | +| 697-704 | Validation Checklist | +| 705-714 | Related Commands | +| 715-719 | Related Skills | # ALTER PAGE / ALTER SNIPPET - Modify Existing Pages and Snippets @@ -678,14 +678,9 @@ page and bind the buttons at creation time instead of rewiring afterwards: view: `replace dvMain.footer with { footer { … } }`, `insert into dvMain.footer { … }`, `drop dvMain.footer`. -**An Accordion group (any pluggable object-list item) has no name either.** - `group g1 (…)` keeps no `g1`, so address the group on its widget: - `insert into acc1 item@2 { … }` fills its body, `acc1 item('One')` finds it - by its headerText, `acc1 item(btnIn)` by a widget inside it, and - `acc1 item@1.headercontent` names its header slot. The header slot is hidden - — and refused — while `headerRenderMode` is `'text'`: write - `set (headerRenderMode: 'custom') on acc1 item@1;` first. Adding a new group - with `insert after acc1 item@1` is not supported. +**An Accordion group has no name either** (`group g1` keeps no `g1`): use + `acc1 item@2`, `acc1 item('One')` or `acc1 item(btnIn)`; `.headercontent` is its + header slot, refused until `set (headerRenderMode: 'custom') on acc1 item@1`. **A pluggable widget's slot has no name either.** A Data grid 2's control bar (`controlbar { … }` in describe) is a property of the grid, so address it by diff --git a/mdl/backend/pagemutator/probe_test.go b/mdl/backend/pagemutator/probe_test.go index 780b182054..3d7cda40fc 100644 --- a/mdl/backend/pagemutator/probe_test.go +++ b/mdl/backend/pagemutator/probe_test.go @@ -17,7 +17,7 @@ type recordingDeps struct{ saves int } func (d *recordingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *recordingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } -func (d *recordingDeps) SerializeClientTemplate() bson.D { return nil } +func (d *recordingDeps) SerializeClientTemplate() bson.D { return nil } func (d *recordingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil diff --git a/mdl/executor/validate_alter_set_test.go b/mdl/executor/validate_alter_set_test.go index 07186bf6d2..654abb69a7 100644 --- a/mdl/executor/validate_alter_set_test.go +++ b/mdl/executor/validate_alter_set_test.go @@ -72,7 +72,7 @@ type countingDeps struct{ saves int } func (d *countingDeps) SerializeWidget(pages.Widget) bson.D { return nil } func (d *countingDeps) SerializeClientAction(pages.ClientAction) bson.D { return nil } -func (d *countingDeps) SerializeClientTemplate() bson.D { return nil } +func (d *countingDeps) SerializeClientTemplate() bson.D { return nil } func (d *countingDeps) SerializeCustomWidgetDataSource(pages.DataSource) bson.D { return nil