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/.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/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/.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-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 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-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 b00376e009..c0ce3e42a1 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. @@ -854,16 +881,14 @@ 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 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 +896,36 @@ 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 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 && 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 +// 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_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/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 e23c0989b1..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 { @@ -1103,14 +1112,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 +1461,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 +1475,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/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_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 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..9ad10bf23c --- /dev/null +++ b/mdl/executor/flow_modify_stated_positions_1508_test.go @@ -0,0 +1,258 @@ +// 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()) + } + // 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()) + } + 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 +}