diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index ab67ebc8b..1f79a2e26 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -122,3 +122,6 @@ {"area": "cmd/mxcli/docker", "date": "2026-09-22", "symptom": "`windows-process-regression` fails intermittently on TestKillProcessGroup_ReapsGrandchildAndUnblocksWait: `cmd.Wait() did not return after killProcessGroup`, ~20.5s (the select deadline), and the GitHub runner then logs `Terminate orphan process: pid (NNNN) (PING)`. killProcessGroup reports no error. Reruns pass, so it reads as 'Windows CI is flaky' and gets attributed to whatever PR happened to be red.", "cause": "The test's readiness marker named the wrong process. The `spawn` helper mode started `cmd /c ping -n 60 127.0.0.1` and wrote `grandchild-started` immediately after `gc.Start()` returned — but Start() only guarantees `cmd.exe` was CREATED; `ping.exe`, which is what ends up holding the inherited stdout pipe, does not exist yet. The test raced ahead to killProcessGroup, `taskkill /F /T` enumerated a tree `ping.exe` had not joined, returned 0, and ping survived holding the write end, so cmd.Wait() never saw EOF.", "file": "`cmd/mxcli/docker/procgroup_windows_test.go` (helper gains a `grandchild` mode that announces ITSELF, with its pid, over the inherited pipe; the test then asserts processAlive on that pid before killing), parser split to `cmd/mxcli/docker/procgroup_marker_test.go` + TestGrandchildPID", "insight": "A readiness marker is only worth what it proves about the process the test is ABOUT. Emitting it from the parent after Start() proves the parent reached a line of code, which is the one thing never in doubt. Emit it from the process under test, over the channel under test — the unix half already did exactly this (`sh -c 'sleep 60 & echo $!; wait'` plus kill(gpid,0)) and the Windows half had silently diverged, so the fix was porting the sibling's handshake rather than inventing one. Two second-order traps: a deadline bump cannot fix this (the grandchild is never killed, so no amount of waiting helps) and would have buried it; and the marker parser must reject a line not yet terminated by \\n, since a truncated pid parses as a plausible different pid. The same-commit control that settled blame: sha 0710968d ran the identical workflow twice, `push` (35715042749) green and `pull_request` (35715076433) red — when a job is suspected flaky, look for two runs of one commit before reading the diff.", "refs": ["ako/mxcli#594", "ako/mxcli#597", "ako/mxcli#601"]} {"area": "cmd/mxcli/diag", "date": "2026-09-22", "symptom": "`mxcli diag loop-report --json` reports `\"failed\": 0` across a real 442-invocation log while runs were genuinely exiting non-zero. Read next to `\"unclosed\": 10` in the same object it says 'nothing failed', which is the opposite of the truth. The text report never printed the field at all, so the misreading was reachable only through --json, where no surrounding prose corrects it.", "cause": "The field counted a different population than its name claimed. `rep.Failed++` fires only for an invocation that CLOSED (wrote session_end) whose summary carried errors_count > 0, and errors_count is diaglog's STATEMENT-level counter \u2014 so the only path reaching it is a run that kept going after a failed statement, i.e. `exec --continue-on-error`. A run that actually fails exits through os.Exit, which skips the deferred Close() and PersistentPostRun, writes no session_end, and lands in `unclosed`. The two populations are disjoint by construction and `failed` is near-always zero. Nothing was broken; the name was.", "file": "`cmd/mxcli/diag_loop_report.go` (`loopReport.Failed` -> `StatementErrors`, json tag `failed` -> `runs_with_statement_errors`; renderLoopReport prints it only when non-zero and takes io.Writer so the text output is testable), tests `cmd/mxcli/diag_loop_report_test.go` (TestStatementErrorsIsDisjointFromUnclosed, TestJSONKeyNamesWhatItMeasures, TestStatementErrorLineIsPrintedOnlyWhenNonZero)", "insight": "A metric that is always zero fails silently in the one direction nobody checks: it is indistinguishable from good news, so it is never investigated. This one survived review and a whole feature PR because every local test run genuinely had no --continue-on-error invocations, so 0 was CORRECT in the test set and wrong in the field \u2014 only a 442-invocation log from a real project surfaced it. Two rules fell out. (1) Assert the JSON key literally, not the Go field: the key is the interface the wrong conclusion was drawn through, and a Go-side rename leaves the tag behind. (2) Never print a counter's zero beside a related non-zero counter; suppress it, or the pair reads as a comparison. The larger fix \u2014 making `failed` mean failed \u2014 needs an exit-code path through ~250 os.Exit sites in cmd/mxcli, since Go has no atexit; `unclosed` already carries that signal and the report explains it. Prove-by-revert done both ways: restoring the tag fails the key test with `\"failed\":2` in the payload, relaxing the guard to >= 0 prints `Finished with failed statements: 0` directly under `Did not close: 2`, which is the reported symptom exactly.", "refs": ["ako/mxcli#617", "ako/mxcli#620"]} {"area": "cmd/mxcli/check", "date": "2026-09-22", "symptom": "`make check-mdl` Error 1 in CI right after the #618 empty-script guard landed. The failing fixture, mdl-examples/doctype-tests/15-fragment-examples.test.mdl, got: 'produced no statements, but it is not empty. The parser could not begin reading it. First line that did not parse: create module FragTest;' \u2014 for a 416-line file that parses perfectly well (18 statements) when copied to a plain .mdl name. The filename was the whole difference.", "cause": "Two defects stacked. (1) MINE: cmd_check.go renders a .test.mdl through testrunner.CheckSource \u2014 a test block is a microflow BODY, so check parses the RENDERING, not the file \u2014 but the #618 guard was given `string(content)`, the file as read. A file with no @test block renders to nothing, so the guard saw zero statements against non-empty ORIGINAL text and quoted a source line the parser had never been handed. (2) PRE-EXISTING: that fixture declares no @test at all. It is a syntax demo misnamed .test.mdl (its sibling 15b-fragment-slots-examples.mdl is plain), so CheckSource rendered it to nothing, check printed 'Check passed!' on zero statements, and `make check-mdl` had been reporting PASS over 416 lines nothing ever read \u2014 concealing a real MDL-PAGE20 violation (page param $Customer, url with no {Customer} segment).", "file": "`cmd/mxcli/cmd_check.go` (guard takes `source`, the parsed text, not `content`; empty rendering from non-empty content gets its own branch), `cmd/mxcli/empty_script.go` (`noTestsDeclaredError`), fixture renamed to `mdl-examples/doctype-tests/15-fragment-examples.mdl` with the url fixed, tests `cmd/mxcli/empty_script_test.go`", "insight": "A guard that reports on text OTHER than what the parser consumed will eventually quote a line the parser never saw, and it reads as authoritative precisely because it names a line. Wherever a command transforms its input before parsing \u2014 a renderer, a preprocessor, a macro pass \u2014 every diagnostic downstream must be fed the transformed text, or it describes a file that was never compiled. The wider lesson is about what the guard FOUND: #618 is 'a silent no-op is the worst outcome', and the repo's own check-mdl suite contained an instance \u2014 a file passing because nothing read it. A suite that reports PASS per file cannot distinguish 'checked and clean' from 'not checked'; the tell was available all along in the statement count, which was 0. When a new guard fails CI, check whether it found a second instance of its own bug before assuming it is a false positive: here it was BOTH, and only fixing the diagnosis would have left the misnamed fixture green and unread. Prove-by-revert done end-to-end: restoring `string(content)` reproduces the CI message verbatim on the same bytes under the .test.mdl name.", "refs": ["ako/mxcli#618", "ako/mxcli#619", "ako/mxcli#1103"]} +{"area":"cmd/mxcli","date":"2026-09-23","symptom":"`mxcli test`, `mxcli check`, `mxcli exec` with no arguments wrote 0 session_start records (`mxcli check /nonexistent.mdl` wrote 1), so a run that failed cobra's Args validation was invisible to `diag loop-report`.","cause":"#617 put diaglog.Init in the root's PersistentPreRun and described that as 'before argument validation'. Cobra's execute() runs ValidateArgs, and answers --help/--version, BEFORE any hook, so arity failures returned before the session opened. The same move also meant the root's -c and REPL sessions were recorded as mode \"mxcli\" instead of \"batch\"/\"repl\", because PreRun reached the singleton first.","file":"cmd/mxcli/session_start.go","insight":"A cobra hook is not 'before anything': execute() order is ParseFlags → help/version → ValidateArgs → PersistentPreRun, so anything that must see every invocation belongs in main() before Execute, keyed on rootCmd.Find(os.Args[1:]), which needs no parsed command. Moving the open that early moves the close problem with it: --help/--version return nil without running PersistentPostRun, so a close left there turns every help lookup into an 'unclosed' (failed) run. That was the false failure #617 had already paid for once. Close in main() after a nil Execute instead. When a singleton is opened earlier, whichever caller reaches it first picks the mode, so resolve the mode the later callers would have passed (batch/repl) at the early call site. The regression test runs the real main() in a re-executed test binary with MXCLI_LOG_DIR. It is the only layer that sees os.Exit paths, and both controls fail at the right place: stubbing startSession gives 0/0/0 records with the /nonexistent.mdl control still passing, and moving the close back to PostRun gives 0 session_end records for --help/--version.","refs":["ako/mxcli#633","ako/mxcli#617"]} +{"area": "cmd/mxcli/diag", "date": "2026-09-23", "symptom": "`mxcli diag loop-report` showed all 5 `test` runs as 'did not close' although every test passed, and inflated the `-c` (111) and `exec` (180) counts in the same log. Reported as 'the command apparently skips the summary record on success too' \u2014 which is not what happens: `test` returns normally, PersistentPostRun fires, and the session_end IS written.", "cause": "mxcli runs mxcli. Measured from a real `mxcli test` with MXCLI_LOG_DIR pointed at a scratch dir: one parent session_start (pid 1071) followed by THREE child session_starts \u2014 `-c DESCRIBE SETTINGS`, `-c SHOW MODULES`, and an `exec` of the generated runner \u2014 before a single test executes. `new`, `eval`, `tui` and the LSP self-spawn the same way (six os.Executable() sites). buildInvocations segmented on 'next session_end OR next session_start, whichever comes first', so a child's start closed the parent's invocation and the parent's own end landed on whatever was open by then. session_end carried no pid, so pairing by process was impossible.", "file": "`mdl/diaglog/diaglog.go` (pid on session_end; parentPIDEnv marker set once in Init and inherited by every child), `cmd/mxcli/diag_loop_report.go` (buildInvocations pairs by pid with the positional rule as fallback; spawned runs excluded from the table and wall time, counted on their own line), tests `cmd/mxcli/diag_loop_report_test.go`", "insight": "The segmentation rule documented itself as exact 'for sequential invocations, which is what an agent loop produces' \u2014 and the thing that breaks that assumption is the tool itself, not concurrency by the user. When a tool can invoke itself, EVERY per-process measurement over it needs a parent link, not just a pid: a pid alone fixes the pairing but still counts three phantom agent calls per test run. The marker belongs on the ENVIRONMENT, not at each spawn site: exec.Command inherits the parent's environment (explicitly via os.Environ(), implicitly when Cmd.Env is nil), so one os.Setenv in Init covers all six self-spawn sites and any added later \u2014 six edits that would each have to be remembered become zero. Second-order trap: spawned runs must be excluded from WALL TIME too, not just the count, because a child's seconds are already inside its parent's; the test asserts 10s for a parent with three 1s children, and the reverted code says 3. Prove-by-revert done on the measured record shape: the positional rule gives Invocations=4 (want 1), Unclosed=1 for a parent whose tests all passed, Wall=3 (want 10).", "refs": ["ako/mxcli#617", "ako/mxcli#629"]} +{"area": "cmd/mxcli/syntax", "date": "2026-09-23", "symptom": "`mxcli syntax page datasource` documented `DataSource: MICROFLOW Module.MF($P)`. That form is a parse error: `dataview dv (datasource: microflow M.DS_X($State))` gives 'line 2:15 no viable alternative at input datasource'. Only the NAMED form `M.DS_X(State: $State)` parses. Hit in a real build, diagnosed from the error rather than the doc.", "cause": "The syntax entry was written from the intended shape rather than from something that had been run through the parser. Nothing checks it: the Syntax and Example fields are free text.", "file": "`cmd/mxcli/syntax/features_page.go` (page.datasource entry now shows `MICROFLOW Module.MF(Param: $P)` and states that the positional form is a parse error)", "insight": "CLAUDE.md deliberately points at `mxcli syntax` instead of restating syntax, so that it cannot go stale \u2014 which makes a wrong entry there worse than a wrong entry in prose, because it is the thing consulted INSTEAD of checking. The cost lands in the agent loop: read it, write it, fail to parse, diagnose, retry. Measured before reaching for the systemic guard: 42 of 164 syntax examples fail `mxcli check` today, but the large majority are fragments by design (a microflow body like `IF \u2026`, a widget snippet like `DATAGRID \u2026`, an OQL fragment) and are legitimately not standalone top-level MDL \u2014 so a blanket 'every example must parse' test would be mostly noise, and making it useful needs a way to mark which examples are standalone. Measuring that first is what stopped a plausible-sounding guard from being built wrong.", "refs": ["ako/mxcli#630"]} diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index a6322046e..7997f2b6f 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -125,3 +125,4 @@ {"area": "mdl/backend", "date": "2026-09-21", "symptom": "`CREATE OR MODIFY VIEW ENTITY` that changed ONLY the OQL printed `Unchanged view entity: \u2026` while `describe entity` showed the new query stored. Changing the attribute list as well reported `Modified` correctly, which is why it hid. Also: the OQL document's unit was replaced under a FRESH GUID on every run, even a byte-identical one, so an MDL-generated project could never come back clean in git (one of the four units #556 measured).", "cause": "A view entity's OQL lives in a separate `DomainModels$ViewEntitySourceDocument` unit, and the executor DELETED it and INSERTED a fresh one on every write. `ReportMutation` downgrades the verb when writes were offered and none landed, but the counters are incremented only at the update choke points (`writer_core.go` reconcileWithStored / MoveUnit) \u2014 `InsertUnit` is not counted at all. So the domain-model unit was offered and correctly elided, the OQL write was invisible, and the report believed the half it could see. Fixed with `WriteViewEntitySourceDocument`, which keeps the stored unit's id and goes through `UpdateRawUnit` \u2192 reconcile: an identical query is elided, a changed one lands and is counted, duplicates are still cleared.", "file": "`mdl/backend/modelsdk/move_view_write.go` (WriteViewEntitySourceDocument, encodeViewEntitySourceDocument), `mdl/executor/cmd_entities.go`", "insight": "**The first fix that comes to mind \u2014 count InsertUnit \u2014 would have swapped a false \"Unchanged\" for a false \"Modified\".** Measuring before changing is what caught it: re-running a BYTE-IDENTICAL script still re-minted the source document's unit id, so counting inserts would have made every view-entity statement report Modified forever. The right fix was the one ADR-0008 already mandates (wire the write path to canon.Reconcile), and it fixes the churn and the verb together. Generalisation worth remembering: any content that reaches storage through `InsertUnit` is invisible to the elision check, so a statement whose only landing write is a NEW unit can still be mis-reported \u2014 `MoveUnit` has a comment explaining it was counted for exactly this reason, and insert/delete were missed. Control the fix on the identical re-run, not just the changed one.", "refs": ["#583", "#556", "#910"]} {"area":"mdl/backend","date":"2026-09-22","symptom":"modelsdk/mpr/version.ProjectVersion declared its own struct with the same seven fields as mdl/types.ProjectVersion instead of aliasing it, so a *version.ProjectVersion could not be passed where a *types.ProjectVersion was wanted and vice versa — two unrelated Go types that both print as 'ProjectVersion'.","cause":"The deleted sdk/mpr/version aliased the canonical type (`type ProjectVersion = types.ProjectVersion`); this copy declared a duplicate. CLAUDE.md's shared-types rule asks for the alias, and nothing enforced it. The duplication survived the legacy-engine retirement because it compiles perfectly — the two declarations are field-for-field identical, so only an assignment ACROSS the boundary reveals them as different types.","file":"modelsdk/mpr/version/version.go","fix":"Made it an alias. The four methods it redeclared (IsAtLeast, IsAtLeastFull, String, IsMPRv2) were verified semantically identical to types' first — IsAtLeast differed only in early-return style, same truth table — and now come from types. IsSupported/SupportsFeature could not survive as methods on an aliased type and had ZERO callers anywhere (measured), so they went with Feature, MinVersion, featureVersions and SupportedVersionRange; that map called itself 'the fallback when the YAML registry is unavailable' and the live registry is sdk/versions/mendix-{9,10,11}.yaml via checkFeature.","insight":"A same-shape duplicate type is invisible to every signal except an assignment across the package boundary: it compiles, tests pass, and the error it eventually produces names the same type on both sides of 'want'. So the guard is a COMPILE-TIME assertion, not a runtime test — `var _ *types.ProjectVersion = (*version.ProjectVersion)(nil)` builds only under an alias and fails to build under a duplicate, which is strictly stronger than anything a test body can assert. Write it before the fix and watch it fail to compile; that failure IS the reproduction. Two measurements that made the cleanup safe rather than brave: diff the method BODIES before assuming the redeclarations are redundant (identical behaviour, different style, is the common case and the dangerous one is the near-miss), and count callers of anything the alias forces you to drop — here six exported symbols had zero. Unrelated trap hit while verifying: four cmd/mxcli tests that read skill files failed once in a full `go test ./...` interleaved with `make check-mdl`, which runs sync-skills (rsync --delete into cmd/mxcli/skills/). They pass in isolation, on clean main, and in an uninterleaved full run — do not attribute a skills-reading test failure to your change without re-running it alone."} {"area": "mdl/backend", "date": "2026-09-22", "symptom": "`create workflow … overview page X` reports `Created workflow` and exit 0 and stores NOTHING — the written unit carries no page reference and not even the page's qualified name as a string. `mx check` passes (a workflow with no overview page is valid) and `describe workflow` omits the clause, so nothing reveals the loss. Running `alter workflow … set overview page X` afterwards DOES write it, which is what makes the split visible", "cause": "Two fields for one concept, never joined: the executor set semantic `Workflow.OverviewPage` (`cmd_workflows_write.go:170`) and `workflowToGen` only ever read `Workflow.AdminPage`, which nothing set. The READ half was wrong in the mirror direction — `workflowFromGen` took `g.OverviewPageQualifiedName()`, so even the correctly-written ALTER read back empty and the catalog's overview-page reference edge never fired", "file": "`sdk/workflows/workflow.go` (the two fields collapsed to one), `mdl/backend/modelsdk/workflow_write.go` (`workflowToGen`), `mdl/backend/modelsdk/workflow_read.go` (`workflowOverviewPageName`)", "insight": "**The Model SDK's StructureVersionInfo settles which of two rival property names is real, in one grep**: `npm pack mendixmodelsdk` then `src/gen/workflows.js` gives `overviewPage: {deleted: \"9.11.0\"}` and `adminPage: {introduced: \"9.11.0\"}` — so AdminPage (a `Workflows$PageReference` CHILD, not a by-name string) is the stored property, and `generated/metamodel` agrees by declaring AdminPage and no OverviewPage. `modelsdk/gen` declares BOTH, which is how a reader and a writer ended up on opposite sides of a 9.11 rename inside one package. **The version branch CLAUDE.md's overlay rule would demand is dead here, and that is a measurement not an assumption**: `workflowToGen` writes `WorkflowV2`, introduced in 11.1.0, unconditionally — so no reachable project wants the pre-9.11 key. Write one spelling, READ both (a read fallback invents nothing). **The differential that proves it on a real build**: same script, same project, only the write suppressed — control 0 errors, fixed `CE7410 \"The selected page 'Overview' should accept a parameter of type 'Workflow'\"` on mxbuild 11.6.6. mxbuild can only validate a page it can see, so the error IS the evidence; with a valid overview page both variants are 0 errors, which is the usual weak-signal trap. Useful side-finding: an overview page takes **System.Workflow**, while a user task's page takes **System.WorkflowUserTask** — two pages, two parameters. NOT fixed: no check rule for CE7410 yet, and `WorkflowV2` being written unconditionally is questionable for a 10.x project. Same shape as the `create … comment 'text'` bug (findings/mdl-grammar.jsonl 2026-08-25): grep for `stmt.X = …` / `wf.X = …` with no matching read. Tests `mdl/backend/modelsdk/workflow_overview_page_test.go`; repro `mdl-examples/bug-tests/workflow-586b-overview-page-dropped.mdl`", "refs": ["ako/mxcli#586"], "ce": ["CE7410"]} +{"area": "mdl/backend", "date": "2026-09-23", "symptom": "`describe java action` prints `ContextObject: entity <>` for a parameter declared `entity not null`; a bare type-parameter reference (`Obj: pEntity`, `returns pEntity`) reads back nameless too. The description no longer round-trips", "cause": "The stored parameter type (`CodeActions$EntityTypeParameterType` / `ParameterizedEntityType`) holds only a BY_ID pointer to the `CodeActions$TypeParameter`. `javaActionFromGen` carried the ID into the semantic type but never resolved it to the name, and the name is all the describer prints. `javascript_read.go` had always done this resolution pass; the Java reader was ported without it", "file": "`mdl/backend/modelsdk/java_read.go` (`resolveJavaActionTypeParameterNames`)", "insight": "The executor's `entity <>` fallback in `formatJavaActionType` is the tell: an empty name at DESCRIBE means the *reader* dropped a by-ID resolution, not that the writer lost it \u2014 the write path sets both ID and name, so a create\u2192read unit test in the backend reproduces it without any project. When a JS and a Java reader cover the same `CodeActions$` shapes, diff their post-processing first; any pass one has and the other lacks is a candidate. The write was never wrong (replaying the fixed description into a fresh project describes identically), so no `mx check` run is needed. Issue mendixlabs/mxcli#1034", "refs": ["mendixlabs/mxcli#1034"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index f340793d9..cd31786cc 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -670,7 +670,13 @@ {"area": "mdl/executor", "date": "2026-09-21", "symptom": "`create or modify entity` drops an attribute a LATER script added, silently. Reported shape: entity created in 01-domain-core.mdl, a calculated attribute added in 03-logic.mdl (its microflow does not exist until then); re-running slice 01 ALONE rebuilt the entity from its own statement and removed the attribute, with `Modified entity: ServiceCore.LithoSystem` as the only output. It surfaced two slices later as `[CE1613] \"The selected attribute 'ServiceCore.LithoSystem.OpenRequestCount' no longer exists.\" at Text 'dtOpen'` — an error naming the PAGE, never the script that removed the attribute. `mxcli check … -p app.mpr --references` said \"Check passed!\".", "ce": "CE1613", "rules": ["MDL087"], "cause": "Half the ask was already shipped and half was not, and the report could not tell them apart. exec's warning (droppedEntityMembers, findings #24, landed 320a304 two weeks before the report) DOES fire — measured on a real 11.6.6 project re-running the reporter's slice 01, it prints the attribute by name — so the reporter was on an older binary. What genuinely did not exist was the issue's second ask: `check` had no project-aware pass for member loss at all, so the one command that runs BEFORE anything is written was the silent one. Added CheckEntityMemberDrops (MDL087, warning) to cmd_check.go's catalog-backed tier, and refactored droppedEntityMembers to share its comparison.", "file": "`mdl/executor/validate_entity_member_drops.go` (new: entityMemberSet, droppedMembers, CheckEntityMemberDrops), `mdl/executor/cmd_entities.go` (droppedEntityMembers now delegates), `cmd/mxcli/cmd_check.go` (projectViolations)", "insight": "**Reproduce before theorising when the report predates a fix in the same area** — exec already printed the exact line the issue asks for, so reading the issue text alone leads either to 'already fixed, close it' or to reimplementing the shipped half. Running the reporter's own sequence against a real project separated the two halves in one command each, and the isolated-slice check printing `Check passed!` is what identified the actual gap. **A check-time twin of an exec-time warning must NOT be the same computation.** exec is per-statement because it is applying statements; check sees the whole script, so it has to be the NET effect — a script that rebuilds an entity and then `alter entity … add attribute`s the members back loses nothing, and that is the IDIOMATIC full-script order, so a per-statement port would warn on every correct script and be switched off within a day. **Intent has to be tracked, not inferred from the outcome**: `drop attribute` / `rename attribute` / `drop entity` produce the same before/after diff as the accident, and a pure diff cannot separate them. Both of those are separate controls, and the naive implementation fails each one specifically (measured: stubbing the net/intent logic fails TestMDL087_ExplicitRemovalIsSilent on 3 of 4 spellings while the positive test still passes — so the positive test alone proves nothing). **One comparison, two layers**: the audit system fields and an omitted `extends` were reported by exec and would have been missed by a second hand-written diff, which is why droppedEntityMembers was refactored onto the shared entityMemberSet rather than copied. An audit pseudo-type (`AutoOwner`) is a FLAG, not an attribute — exec `continue`s past it — so counting it as one makes a faithful restatement read as a drop.", "refs": ["ako/mxcli#562", "findings #24", "findings #13"]} {"area": "mdl/executor", "date": "2026-09-21", "symptom": "`retrieve $AccountList from Administration.Account sort by System.Language.Code asc;` — MDL that `mxcli describe` had just emitted — passed `mxcli check` and was refused by `mxcli exec`: \"sort by attribute 'System.Language.Code' does not belong to entity 'Administration.Account'\". Reported as a check/exec inconsistency (mendixlabs/mxcli#1152); the real defect is that the round trip cannot replay its own output for any sort over an association reached from an ANCESTOR.", "cause": "inferSortEntityRefSteps searched ONE domain model — the retrieved entity's own module — for associations whose parent was the retrieved entity ITSELF, and qualified the association it found with the retrieved entity's module. All three assumptions hold only when the hop starts on the retrieved entity in its own module. Administration.Account reaches System.Language through System.User_Language, declared on System.User and stored in the System module: parent is an ancestor, the domain model is another module's, and the qualified name carries THAT module. Rewritten as a generalization-chain walk that looks each ancestor up in its own module and qualifies the association with the module storing it; the destination end is matched with entityIsSubtypeOf rather than by equality, since an association may point at a specialization of the entity that declares the attribute.", "file": "`mdl/executor/cmd_microflows_builder_actions.go` (inferSortEntityRefSteps); tests `mdl/executor/cmd_microflows_sort_association_test.go`, `mdl/backend/modelsdk/microflow_retrievesort_test.go`; example `mdl-examples/bug-tests/microflow-1152-sort-over-association.mdl`", "insight": "**The second control is the one that pays.** Reverting the fix reproduces the refusal, which only proves the test fires. The control that taught something was building a binary that DERIVES the hop and does not WRITE it — exec succeeds and mxbuild 11.12.3 answers CE7247 \"Cannot sort on attribute 'System.Language.Code'. Attribute 'System.Language.Code' is not an attribute of entity 'Administration.Account'\" — the executor's refusal message almost word for word, from the other end of the pipeline. That is what fixes the qualified name as load-bearing: the stored EntityRefStep must read System.User_Language, and the pre-existing code would have written Administration.User_Language had it found anything at all. **Skip the theory that check is missing a rule**: check has no sort-attribute rule at all and resolves no hops, so it was never going to disagree with exec here — the inconsistency in the report is a symptom of the false refusal, not a second defect. **Known residue, stated because the round trip rests on it**: DESCRIBE emits only the attribute's qualified name, so where several associations reach one entity the replay picks the nearest ancestor's first and can silently land on the other hop. Spelling the hop needs grammar (sortColumn is qualifiedName|IDENTIFIER, no `/` path) and is a language change, not a fix."} {"area": "mdl/executor", "date": "2026-09-21", "symptom": "Follow-up to the sort-hop inference fix: with the hop derivable but not SAYABLE, `describe → exec` still silently changed the program wherever two associations reach the same entity. Measured on 11.12.3 with Order_ShipTo and Order_BillTo (both Order -> Address): a microflow sorting by the BILLING address came back sorting by the SHIPPING one, `mx check` 0 errors on both sides. Same for a page datasource's sort bar.", "cause": "DESCRIBE emitted only the sort attribute's qualified name and the reader never looked at the hop at all — `sortItemsFromRaw` read AttributeRef.Attribute and skipped AttributeRef.EntityRef, so the association was written and never read back. MDL had no spelling for it either (`sortColumn : (qualifiedName | IDENTIFIER)`). Closed end to end: sortColumn takes `qualifiedName (SLASH qualifiedName)*` (the shape MDLCatalog.g4 already uses for Association/Entity), SortColumnDef/OrderByItemV3 carry the hops, the executor resolves the NAMED association instead of inferring, both readers reconstruct EntityRef.Steps, both describers emit `Assoc/.../Attr`, and the page writers moved from attributeRefToGen to inputAttributeRefToGen. Inference stays as the fallback, so every script written before still works.", "file": "`mdl/grammar/domains/MDLPage.g4` (sortColumn) + `mdl/ast/ast_page.go`/`ast_page_v3.go` + `mdl/visitor/visitor_microflow_statements.go` (sortColumnHops) + `visitor_page_v3.go` + `mdl/executor/cmd_microflows_builder_actions.go` (resolveSortAssociationPath, lookupSortHop, entityChainModules) + `cmd_microflows_format_action.go` + `cmd_pages_builder_v3.go` (resolveAssociationAttributePathForEntity) + `cmd_pages_describe_datasource.go` (sortAttributeHops, sortColumnPath) + `mdl/backend/modelsdk/microflow_read_actions.go` (entityRefStepsFromRaw) + `widget_write.go` + `sdk/pages/pages_datasources.go` (GridSort.AttributeRefSteps)", "insight": "**The measurement that decides whether a lossy describer is worth a language change is a CONSTRUCTED one.** The corpus agrees with the inference rule by construction — every document mxcli itself wrote stores the association inference would have picked, so the round trip is a fixed point on everything to hand and looks faithful. The case that matters had to be built: two associations to one entity, then the stored hop edited to the one inference does NOT pick. Byte-patching the .mxunit is enough and takes a minute — `Order_ShipTo` and `Order_BillTo` are the same length, so a `sed` on the BSON needs no resize — and the replay flipped it back immediately. **Control on a binary that drops the hop, not just on one that reverts the fix**: reverting only proves the test fires, while dropping the hop gets mxbuild to say CE7247 \"Cannot sort on attribute … is not an attribute of entity …\" — the executor's own refusal message from the other end of the pipeline, which is what proves the EntityRef load-bearing rather than cosmetic. **Two reads were missing, not one**: the microflow reader and the page reader each drop the hop separately, and fixing only the half named in the report would have shipped a describer that emits the path for microflows and silently drops it for pages. **The strongest round-trip evidence is 'Unchanged'** — with identity preservation and write elision, replaying DESCRIBE output on a correct implementation elides the write entirely, so `Unchanged microflow: …` is a stronger result than any byte comparison."} +{"area": "mdl/executor", "date": "2026-09-22", "symptom": "`call javascript action` with an entity-type parameter (NanoflowCommons.RefreshEntity's `EntityToRefresh: entity <>`) writes `Microflows$BasicCodeActionParameterValue{Argument: \"Mod.Entity\"}` where Studio Pro stores `Microflows$EntityTypeCodeActionParameterValue{Entity: \"Mod.Entity\"}`. Studio Pro's entity picker shows EMPTY; mxbuild fails **CE0115** \"The arguments that are passed to JavaScript action ... do not match the expected parameters and need to be refreshed\"", "cause": "`addCallJavaScriptActionAction` never looked the action up. It hardcoded Basic for every parameter, with the bug stated as a comment: `// JavaScript actions use BasicCodeActionParameterValue for all parameters`. The Java-action builder 15 lines above had resolved this correctly since ako/mxcli#656 via `ReadJavaActionByName` + an `entityTypeParams` set; the JS twin was never given the same treatment even though `ReadJavaScriptActionByName` was already on the backend interface and the read, write and DESCRIBE paths all already handled `EntityTypeCodeActionParameterValue`", "file": "`mdl/executor/cmd_microflows_builder_calls.go` (`addCallJavaScriptActionAction`)", "insight": "**A comment asserting a uniform rule is the tell.** The bug was a one-line claim (\"for all parameters\") that nobody had measured; everything needed to falsify it already existed in the tree. When a doctype has a twin (java/javascript, microflow/nanoflow), diff the two builders before theorising — the fixed twin is the spec. **Do not use DESCRIBE to detect this class**: describe renders the broken and the correct nanoflow as byte-identical MDL (both `EntityToRefresh = Administration.Account`), so a round-trip test passes against the bug. Assert on the BSON `$Type`. **The plausible wrong turn is trusting the report that mxbuild is silent** — it is not: two builds of `testdata/expr-checker/minimal.mpr` (which ships NanoflowCommons, so no `mxcli new` needed) gave 0 errors fixed vs CE0115 faulty on mxbuild 11.6.6. Probe the parameter's real kind before assuming: `EntityTypeParameterType` (the generic `entity <>` slot) takes the Entity value, while a CONCRETE `EntityType` (TakePicture's `Picture: System.Image`) takes an object expression and correctly stays Basic — a fix promoting every entity-ish parameter would be wrong, which is why that case is a test. Tests `mdl/executor/cmd_microflows_builder_js_action_test.go`, `mdl/backend/modelsdk/microflow_action_test.go`; example `mdl-examples/bug-tests/javascript-action-1137-entity-type-parameter.mdl`. Reported as mendixlabs/mxcli#1137", "ce": ["CE0115"]} {"area": "mdl/executor", "date": "2026-09-22", "symptom": "`CREATE OR REPLACE LAYOUT` re-run with an identical statement reported `Replaced layout …` and dirtied 3 files EVERY time. Measured on a blank 11.14.0 project, three runs produced three different .mxunit filenames (8aa37ee1… -> fd6e9c96… -> 6d2a…): the unit was deleted and re-inserted under a fresh GUID, so git shows a delete plus an untracked add rather than a modified file. Second, unreported symptom found by the control: a layout MOVEd into a folder was filed back into the module root on every rewrite (`show layouts` Folder column Layouts -> empty).", "cause": "execCreateLayout collected the stored layout's id into `toDelete`, deleted it, and called CreateLayout with a freshly built layout — CreateLayout goes through InsertUnit under a newly minted id, and InsertUnit is not a canon.Reconcile choke point. The #556 net (carryIdentityFromRemovedUnit) cannot cover it: that keys on the unit ID and this path re-mints it, so there is nothing to reconcile the re-insert against. The folder half has the same single cause: there is no FOLDER clause on CREATE LAYOUT, so buildLayoutV3 always sets ContainerID to the module root, and only an INSERT applies that to the unit's row.", "file": "`mdl/executor/cmd_pages_layout_v3.go` (execCreateLayout), `mdl/backend/modelsdk/layout_write.go` (UpdateLayout), `mdl/backend/page.go` + `mdl/backend/mock/`", "insight": "**When a `create or modify` handler churns, look for delete+create before looking at the codec.** This is the third instance in one week — REST client (#556), view entity OQL document (#583), layout (#600) — and all three were the same shape and took the same fix: rewrite the stored unit through UpdateRawUnit instead of replacing it. The tell is cheap: `ls` the .mxunit filenames across two runs. A CHANGED filename means delete+insert (fix the handler); a same filename with different bytes means the codec or a carry (fix canon). **A storage-layer net that keys on the unit ID cannot cover a path that re-mints the ID** — worth stating because #556's fix reads like it generalised, and it does not reach here. **The folder defect is the one the tests would not have found**: it only appears once a layout has been moved, which no unit test set up and no reported symptom mentioned; it surfaced from running the faulted binary through a MOVE, which is why the control is worth running on more than the reported case. **Do not trust the issue's severity**: #556 ties this to #553 (project unloadable). Measured with a real page bound to the churned layout, mxbuild reports 0 errors on both variants, because pages resolve layouts by qualified name and not by unit GUID — so `mx check` is not a control for this class at all and the version-control diff is the only signal.", "refs": ["ako/mxcli#600", "ako/mxcli#556", "ako/mxcli#583", "ako/mxcli#932", "mendixlabs/mxcli#1063"]} {"area":"mdl/executor","date":"2026-09-22","symptom":"`UPDATE WIDGETS` prints a per-property `Warning: Failed to set …` for every assignment and then reports `Updated 2 widget(s)`, plus `Note: Run 'refresh catalog full force' to update the catalog with changes`, and exits 0. `describe styling` afterwards shows nothing was written","cause":"`updated++` sat OUTSIDE the assignment loop and was unconditional, so the counter meant \"this widget was found\" and was reported as \"Updated\". The same counter gated `mutator.Save()`, so a container whose every assignment failed was still saved","file":"`mdl/executor/cmd_widgets.go` (`updateOutcome`, `updateWidgetsInContainer`, `execUpdateWidgets` summary)","insight":"**A success counter incremented in the wrong loop is invisible to every test that only checks the happy path** — the failures were already being printed correctly one line above the lie. Split the outcome into the three things that actually happen (changed / matched-but-unwritable / in-catalog-but-not-in-document) rather than adding a boolean: rounding the third into either of the others is how a stale catalog reads as success. **Bound the severity before writing it up**: the rebuilt document was semantically identical, so ADR-0008 elision skipped the write — measured, no `mprcontents/` unit changed mtime and `mx check` stayed at 0 errors, making this a reporting defect and not a data one. Worth saying, because \"claims success after failing\" otherwise reads as corruption. **The DRY RUN had the same defect one step earlier and is the worse half**, since the syntax help tells you to run it first: it printed `Would set …` without attempting anything. Fixed by running the assignments against `pagemutator.Probe()` — the discardable copy `mxcli check` already uses for ALTER PAGE SET — so the preview reports `Cannot set`. Reuse that seam rather than re-deriving what a setter accepts; a preview that re-implements the rule drifts from it in exactly the direction that hurts","refs":["ako/mxcli#520","ako/mxcli#515"]} {"area":"mdl/executor","date":"2026-09-22","symptom":"`alter page … set '' = on ` dead-ended — `set` reaches first-class properties and the stored widget's PLUGGABLE property bag, and a design property lives in `Appearance.DesignProperties`. The only spelling that worked was `alter styling`, a second statement for the same operation","cause":"No resolution from a STORED widget to its theme-registry key, so `set` could not tell a design property from a mistyped pluggable one and had to assume the latter","file":"`mdl/backend/pagemutator/probe.go` (`WidgetStorageType`); `mdl/executor/design_property_routing.go` (new); `cmd_alter_page.go` (`applySetPropertyMutator`); `mdl/backend/pagemutator/mutator.go` (the now-stale error message)","insight":"**The resolver the routing needed already existed with zero callers.** `bsonTypeToDesignPropsKey` ($Type → theme key) had never been referenced, so it had never been validated against anything; ako/mxcli#509 deliberately avoided standing up a third consumer of the concept before something needed it, and this was that something. **Do not assert the two key maps are consistent — they are not, and both directions have measured reasons.** $Type-only: `DataGrid`/`Gallery` are the NATIVE widgets, which the MDL keywords no longer produce (`datagrid`→Data grid 2's id via pluggableKeywordIDs), so the stored path resolves MORE than the inline one. Keyword-only: `header`/`footer` map to \"Header\"/\"Footer\" but MDL builds BOTH as `Forms$DivContainer`, and Atlas declares no such groups — so the inline design-property validation for a header widget misses and skips the widget silently, the same shape pluggableKeywordIDs records for combobox/gallery/image. A test that pins both exclusive SETS with their reasons is the useful shape; a consistency assertion fails on correct code. **Route only on a positive theme declaration for THIS widget's type** — routing on \"the theme says nothing, so it must be a design property\" turns a typo into a silently-written design property. **Prove the two statements are the same operation on bytes, not on reasoning**: write via `alter styling`, then run the `alter page` form and count rewritten units — 0 means elision found them semantically equal. `Altered page` is ALTER PAGE's fixed verb and is NOT the elision verb, so it proves nothing. Knock-on: the #1135 error message named `alter styling` as the route, which became stale the moment `set` learned the route — and a test asserted that wording, so it had to be inverted like the others","refs":["ako/mxcli#515","ako/mxcli#509","ako/mxcli#511","mendixlabs/mxcli#1135"]} {"area":"mdl/executor","date":"2026-09-22","symptom":"No way to set a design property across pages — \"every data grid compact and striped\" was one statement per page, and the bulk command that looked right (`update widgets`) writes only the pluggable property bag","cause":"ALTER PAGE's design-property SET (the singular half of ako/mxcli#515) had no plural sibling; MDL's only bulk page statement was `ALTER PAGES … SET LAYOUT`","file":"`mdl/grammar/MDLParser.g4` (`alterPagesStylingStatement`); `mdl/ast/ast_alter_page.go`; `mdl/visitor/visitor_alter_page.go`; `mdl/executor/cmd_alter_pages_styling.go` (new)","insight":"**The selector is the whole design problem, and a name cannot be it**: a widget name is unique only within its page (measured — `actionButton1` in 30 units of a blank project), so the predicate has to be a widget TYPE. Name it by the **MDL keyword**, resolved through the existing `pluggableKeywordIDs`, not by a `LIKE` over the stored id: `WidgetType LIKE '%datagrid%'` matches 20 widgets in 6 containers on a blank project because it sweeps in DatagridTextFilter/DateFilter/DropdownFilter, which do not carry the grid's design properties. **Reuse three things instead of growing a fourth of each** — `findMatchingWidgets` (the catalog query), the per-widget routing decision from the singular form, and `updateOutcome` from ako/mxcli#520 so a sweep that matches and writes nothing exits non-zero instead of claiming success. **Two ANTLR traps, both positional**: the rule has two `identifierOrKeyword` slots (optional module, WHERE value) returned as ONE list, so reading them positionally without checking `ctx.IN()` scopes a project-wide sweep to a module named after a widget type; and the sibling `ALTER PAGES … SET LAYOUT` shares the same prefix, so a test that the layout form still parses as itself is not optional. `ensureCatalog(ctx, true)` must be called before `findMatchingWidgets` or it nil-panics — a cold catalog otherwise reads as \"no such widgets\"","refs":["ako/mxcli#515","ako/mxcli#520"]} +{"area": "mdl/executor", "date": "2026-09-23", "symptom": "`currentDeviceType()` in a nanoflow passes `mxcli check` (\"All references valid.\") and `mxcli exec` (\"Created nanoflow: Test.NF_Dev\"), then the build fails `[error] [CE0117] \"Error(s) in expression.\" at Log message activity 'Log message (info)'`", "cause": "Two gaps stacked. MDL044 lived in ValidateMicroflow, which only CREATE MICROFLOW reaches — check (validate_program.go), the LSP and exec (buildNanoflowFromStmt) never ran it on a nanoflow, so the #828 fix covered half the flow types. And walkBody listed MDL044's expression sites inline per statement (return/if/declare/set/create/change) with no `log` case, so the reported repro — the call in a LOG message — was missed in a microflow too", "file": "`mdl/executor/validate_microflow.go` (`checkStmtExprFunctions`), `mdl/executor/validate_nanoflow.go` (new: `ValidateNanoflow`, `validateNanoflowRules`), `mdl/executor/cmd_microflows_build.go` (`buildNanoflowFromStmt`), `mdl/executor/validate_program.go`, `cmd/mxcli/lsp_diagnostics.go`", "insight": "**When a rule fix names a statement type, grep every entry point for the sibling type** — `CreateMicroflowStmt` and `CreateNanoflowStmt` share a body grammar but have separate wiring in check, LSP and exec, and #828 wired only the microflow side of all three. **Do not run ValidateMicroflow over a nanoflow to close it**: MDL057 refuses `synchronize`, which is nanoflow-only, so a wholesale reuse turns a gap into a false-positive write barrier; run only the rule that holds (one shared `checkStmtExprFunctions` site list, so the two walkers cannot drift). **Test the reported repro verbatim, not the prior fix's shape**: #828's test used `declare`, the report used `log`, and that difference was a second, independent gap. **Build the report's workaround before repeating it**: `[%CurrentDeviceType%]` is CE0117 on 11.13.0 in a nanoflow AND a microflow (control `[%CurrentDateTime%]` builds 0 errors) — and `check` does not validate token names at all, a separate gap. Operational trap: a project from `mxcli new` links `./mxcli` to `bin/mxcli` by shared inode, so a project created before rebuilding runs the PRE-fix binary — the first 'fixed' run here wrote the nanoflow; call `bin/mxcli` after `make build`", "refs": ["mendixlabs/mxcli#1033", "mendixlabs/mxcli#828"], "ce": ["CE0117"], "rules": ["MDL044"]} +{"area": "mdl/executor", "date": "2026-09-23", "symptom": "`jump to A;` inside a `boundary event … { }` body: `check --references` clean, `exec` written, DESCRIBE reads back `jump to A;`, then native `mx check`: [CE0495] \"Duplicate name 'A'.\" at User task 'A', Jump 'A' and [CE6680] \"The 'Target' property is required.\" at Jump 'A'", "cause": "Same as mendixlabs/mxcli#1005: `buildJumpTo` named the jump after its target, and in v0.20.0 name deduplication did not reach boundary-event bodies, so the jump kept the target's name. The report was filed against v0.20.0 (2026-08-28), three days before 825873d6 fixed it", "file": "`mdl/executor/cmd_workflows_write.go` (`buildJumpTo`, `deduplicateActivityNamesInFlow`); regression test `mdl/executor/issue1024_boundary_jump_test.go`", "insight": "**Before fixing, rebuild the reporter's version and main, and run the repro on both.** Main gave 0 errors on 11.14.0, and a build of `825873d6^` reproduced both errors verbatim, so the fix was a regression test, not a code change. It looked like a different bug because the report said the outcome-body form worked. That was a flow-order accident from #1005: dedup renames the SECOND activity with a name, and an outcome body comes after its task, while boundary bodies were then outside the dedup walk entirely. CE6680 'Target required' is Mendix's wording when the target name resolves to the jump itself, not a sign that TargetActivity was empty. It was set to `A` all along. **The #1005 fix has two independent guards** (jump named `JumpTo`, and jumps deduplicated last). Reverting either one alone leaves the test green, so a control has to stub both, or run the unchanged test against the pre-fix commit (with a shim for helpers added later). Otherwise the control 'passes' and proves nothing. mendixlabs/mxcli#1024", "refs": ["mendixlabs/mxcli#1005", "mendixlabs/mxcli#1024"], "ce": ["CE0495", "CE6680"]} +{"area": "mdl/executor", "date": "2026-09-23", "symptom": "`describe page` on a page with several galleries emits MDL that mxcli's own `check --references` rejects: `duplicate widget name 'template1' (used 4 times) — Mendix requires unique widget names per page (CE0495)` (and `filter1` when the galleries have filters). The row/col half of the same report was already fixed; this third name survived it", "cause": "`template`/`filter` inside a gallery are CHILD SLOTS of its definition (gallery.def.json childSlots). `applyChildSlots` builds only the block's children into the slot property and discards the block's name, so DESCRIBE has to synthesise `template1` for every gallery. `checkDuplicateWidgetNames` already exempted the parent's object-list containers but not its child-slot containers", "file": "`mdl/executor/validate_page_context.go` (`unstoredContainerKinds`, was `objectListContainerKinds`; `checkDuplicateWidgetNames`); test `mdl/executor/validate_page_slot_containers_test.go`", "insight": "**When a multi-symptom report is marked fixed, re-run every symptom in it, not the first** — the earlier fix covered `row1`/`col1`/`FullName` and its test never mentioned `template1`, so the issue stayed open with a third name nobody had reproduced. Cheapest proof a name is not stored: author a distinctive one (`template contentZebra`), `grep -rl` it in `mprcontents/` (0 files) and DESCRIBE it back (`template1`). Don't exempt the keyword `template` globally — outside a slot-declaring widget, `template` is `buildTemplateV3`, a `Forms$DivContainer` that DOES store its name; resolve it from the enclosing widget's def (both `ChildSlots` and every mode's), and cover the `container ` spelling that `applyChildSlots` routes by name. Measured on 11.12.2: four galleries each `template template1` → `mx check` 0 errors", "refs": ["mendixlabs/mxcli#978"], "ce": ["CE0495"]} +{"area":"mdl/executor","date":"2026-09-23","symptom":"\"List views are written with `Editable: false` by mxcli, so every input inside a list view renders disabled — even with `editable: Always` on the text box, and even inside a nested data view.\" Valid BSON, `mxcli check` clean, build succeeds; only the rendered app shows it","cause":"Not a writer bug: false IS Mendix's default for Pages$ListView.editable. The 2026-09-06 fix made an explicit `editable: true` reach the document, but nothing told an author who never wrote it that the list view's read-only context overrides the inputs' own `editable:`","file":"`mdl/executor/validate_listview_editable_inputs.go` (MDL-WIDGET31, hooked in `validate_widgets.go`); test `validate_listview_editable_inputs_test.go`; example `mdl-examples/bug-tests/631-listview-inputs-not-editable.mdl`","insight":"**Measure the default before changing it — and the measurement is cheap.** The issue parked \"default to true\" on a Studio Pro comparison it could not run; `npm pack mendixmodelsdk` and reading the class in `src/gen/pages.js` settles it in a minute: `new PrimitiveProperty(ListView, this, \"editable\", false, …)` and `_initializeDefaultProperties` never sets it, so flipping the default would have made mxcli disagree with every Studio Pro-authored list view. A Studio Pro project confirms it (ako/TestApp, 11.14.0): 30 of 32 list views stored false, none holding an input, and the one with inputs set to true by its author; `describe` of that page checks clean, deleting `Editable: true` from it fires MDL-WIDGET31, and describe -> exec preserves it. The fix is therefore a check for the COMBINATION (not-editable list view + input not marked `editable: Never`). Read editability with the SAME accessor the builder uses (`GetBoolProp`): a quoted `editable: 'true'` parses as a string, the builder writes false, and a rule that read it leniently would stay quiet on a page that renders disabled. Stop the descent at a nested list view — its own Editable governs its inputs and it is visited on its own — but NOT at a nested data view, which the reporter measured does not escape the context","refs":["ako/mxcli#631"],"rules":["MDL-WIDGET31"]} +{"area": "mdl/executor", "date": "2026-09-23", "symptom": "`captionparams` on an action button is accepted by `mxcli check` but not written; a container with `onclick` and a dynamic text does the job. Measured: `actionbutton b (caption: 'Save {1}', captionparams: [{1} = Title])` in a dataview wrote the LITERAL 'Title' (describe: `ContentParams: [{1} = 'Title']`), check --references and mx check both clean; and describe -> exec of any button with params dropped them all -> 4x CE0720 'Place holder index 1 is greater than 0' at mx check.", "cause": "buildButtonV3 carried its own copy of the template-parameter resolver (bare name without `.`/`$` -> quoted literal) instead of the shared buildClientTemplateParams that dynamictext/datagrid columns use; and DESCRIBE emitted a button's params as `ContentParams:`, a key the button builder never read. MDL-WIDGET04's orphan-placeholder check covered dynamictext only, so the round-tripped `{1}` with no parameter reached the build.", "file": "`mdl/executor/cmd_pages_builder_v3_widgets.go` (buildButtonV3 -> buildClientTemplateParams, ContentParams fallback), `mdl/executor/cmd_pages_describe_output.go` (ActionButton emits CaptionParams), `mdl/executor/validate_widgets.go` (validateButtonCaptionPlaceholders)", "insight": "**The report's workaround is the diagnosis**: 'a dynamic text does the job' means the SAME parameter text works on one widget and not another, so diff the two builders before touching grammar or the writer -- the writer was fine (clientTemplateToGen is shared). Two duplicate resolvers had drifted: the shared one gained SourceVariable, toString, association steps and format blocks over a year of fixes; the button copy got none. Do not trust 'check passed' for the literal case -- a literal holding the attribute's name is a valid model, so nothing short of describe (or looking at the rendered button) shows it. The describe->exec round trip is the cheap detector: run it once and the ContentParams/CaptionParams mismatch drops every param and mx check reports CE0720. Keep reading `ContentParams:` on buttons -- scripts dumped before this fix use it. Remaining gap, NOT this bug: `check --references` does not flag an unknown BARE attribute in contentparams/captionparams on either widget (`[{1} = Titel]` passes).", "refs": ["ako/mxcli#632"], "ce": ["CE0720"], "rules": ["MDL-WIDGET04"]} diff --git a/.claude/skills/fix-issue/findings/mdl-grammar.jsonl b/.claude/skills/fix-issue/findings/mdl-grammar.jsonl index 459df95ad..26304cc9b 100644 --- a/.claude/skills/fix-issue/findings/mdl-grammar.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-grammar.jsonl @@ -60,3 +60,4 @@ {"area": "mdl/grammar", "date": "2026-09-20", "symptom": "`create snippet Test.SNIPPET_Label (params: { $Label: string }) { dynamictext dt (content: $Label) }` — the spelling `mxcli syntax snippet.create` printed in its own Syntax line — passed `mxcli check` and failed at exec with \"failed to build snippet: failed to resolve entity string: entity not found: string\", naming a type nobody spelled (mendixlabs/mxcli#1028).", "cause": "`snippetParameter`/`snippetParameterList` in MDLPage.g4 were a byte-identical duplicate of `pageParameter`/`pageParameterList` with their own visitor, buildSnippetParameterListAsPage, which never called buildDataType — so a primitive type never reached the AST and buildSnippetV3 (which had no primitive branch either) took the source text for an entity name. Collapsed: a snippet's Params clause IS pageParameterList, and buildPageParameters is the only conversion. The primitive is then REFUSED, not written: mxbuild rejects a primitive snippet parameter with CE0046, so writing one the way a page parameter writes one would have traded an unreadable exec error for a build failure.", "file": "`mdl/grammar/domains/MDLPage.g4` (duplicate rule deleted); `mdl/visitor/visitor_page_v3.go` (buildSnippetParameterListAsPage deleted); `mdl/types/snippet_parameter_types.go` (SnippetParameterTypeRule, the measurements); `mdl/executor/validate_snippet_parameters.go` (MDL087); `mdl/executor/cmd_pages_builder_v3.go` (buildSnippetV3 refusal, pageParamBSONType Long fix); `cmd/mxcli/syntax/features_page.go`; tests `mdl/executor/snippet_param_primitive_test.go`, `mdl/executor/validate_snippet_parameters_test.go`, `mdl-examples/bug-tests/1028-snippet-primitive-parameter{,.fail}.mdl`", "insight": "Two byte-identical grammar rules with two visitors is a bug generator, not a duplication smell: this clause produced TWO reported bugs from the same duplication in a fortnight (the quoted entity name, then this), and the first fix — patching the copy — left the second live and silent, turning a loud wrong error into a parameter with no type at all. When a fix is 'make X agree with Y' and X and Y are the same grammar, delete X. Second, and the reason step 6 of fix-issue is not optional: the obvious repair here (write the primitive the way a page parameter writes one) is supported by every source of truth in the repo — generated/metamodel declares Forms$SnippetParameter.ParameterType as the polymorphic DataTypes$DataType, exactly as Forms$PageParameter's, and the codec encodes it happily — and mxbuild rejects it with CE0046. A shape argument from the metamodel cannot see a validator rule. The control that made the rule crisp was putting the SAME six primitives on a PAGE in the SAME mxbuild run: six CE0046 on the snippet, 0 errors on the page, so the restriction is on snippet parameters and not on primitives, which is exactly what the error message now has to say. Third, a bug like this is a documentation bug as much as a code one — the reporter reached it by following `mxcli syntax snippet.create`, so a fix that leaves that line printing `$Label: String` re-creates the report. Aside found on the way: pageParamBSONType returned \"DataTypes$LongType\", a $Type that does not exist in gen OR generated/metamodel (constant_write.go had the note, 'storage has no LongType'), and pageParamTypeToGen's default arm quietly rescued it into a String — so a `Long` page parameter had been silently stored as String.", "ce": "CE0046", "rules": "MDL087", "refs": "mendixlabs/mxcli#1028; the sibling quoted-name fix in the same clause (mdl/visitor/snippet_param_quoted_entity_test.go); ADR-0005 guard-don't-drop"} {"area": "mdl/grammar", "date": "2026-09-21", "symptom": "A new settings option list keyed on `IDENTIFIER` makes the feature's ONLY option a parse error: `alter settings workflows add group 'Approvers' (Description: '\u2026')` \u2192 \"mismatched input 'Description' expecting IDENTIFIER\"", "cause": "`Description` is an MDL lexer keyword (DESCRIPTION, from the security statements), so it never matches IDENTIFIER. The rule was copied from `languageOption`, whose keys (CheckCompleteness, CustomDateFormat\u2026) all happen to be plain identifiers \u2014 so the pattern looked safe and was not", "file": "`mdl/grammar/domains/MDLSettings.g4` (`settingsItemOption`) + `mdl/visitor/visitor_settings.go` (`collectSettingsItemOptions`)", "insight": "Any `( key: value )` option list must key on `identifierOrKeyword`, not IDENTIFIER, and the visitor must read it with `unquoteIdentifier(ctx.IdentifierOrKeyword().GetText())`. Before writing one, grep MDLLexer.g4 for each key you intend to accept \u2014 the check costs seconds and the failure lands on the single statement the feature exists for. Copying an existing option rule proves nothing about your key set. Control: reverting the rule to IDENTIFIER fails TestAlterSettings_WorkflowGroup with exactly that message. mendixlabs/mxcli#272", "refs": ["mendixlabs/mxcli#272"]} {"area": "mdl/grammar", "date": "2026-09-22", "symptom": "A `create workflow` clause written in the \"wrong\" position is a parse error — `on created microflow` anywhere but between the targeting clauses and `entity` gives `line 6:4 mismatched input 'ON' expecting ';'`, and a header clause out of place gives `mismatched input 'DISPLAY' expecting {ON, BEGIN, EXPORT, DUE, OVERVIEW}`. Neither names the clause or the rule, and one misplaced clause cascades into 3–7 more errors including a bogus `extraneous input 'END'`. The reporter reverse-engineered the order empirically and wrote it into their notes", "cause": "`createWorkflowStatement` and `workflowUserTaskStmt` were a fixed SEQUENCE of optional groups — each clause optional, its POSITION not — and the VISITOR depended on that: it read qualified names by COUNTING (`names[1]` or `names[2]` for the overview page depending on whether PARAMETER was present; `nameIdx` walked page → targeting → on-created → entity) and strings by index off `AllSTRING_LITERAL()`. So the grammar could not simply be relaxed", "file": "`mdl/grammar/domains/MDLWorkflow.g4` (new `workflowHeaderClause`, `workflowUserTaskClause`, `workflowMultiUserTaskClause`), `mdl/visitor/visitor_workflow.go` (`applyWorkflowUserTaskClause`), `mdl/visitor/visitor_workflow_clauses.go` (`checkWorkflowClausesAtMostOnce`)", "insight": "**Positional reading is what makes a clause order load-bearing, so the grammar fix is a visitor fix.** The tell is `names[idx++]` in an exit-listener: the rule already carried a comment warning that reading strings by position had nearly mis-assigned FOLDER, and the same hazard had simply been left standing for qualified names. **A clause set must re-add the at-most-once rule the sequence gave for free**, or `page M.A page M.B` starts parsing with the second silently winning — a worse failure than the parse error it replaces. Enforce it in the visitor, not the grammar: only there can the message say `duplicate PAGE clause on user task Review (already given on line 12)`. **Two spellings that fill one model slot are ONE clause**: `targeting microflow` + `targeting xpath` were both accepted and the LAST one won, though a user task stores one UserSource — order-dependence in its most damaging form, and now a duplicate. **Keep the MULTI alternative's own clause rule rather than collapsing to `MULTI?`** — relaxing the order must not relax the vocabulary, or a single user task starts accepting `decide by`. **Control that settles it**: build a `bin/mxcli` from HEAD in a `git worktree`, exec the canonical-order script with it, and compare the written `.mxunit` against the fixed binary's output for BOTH orders — 6,038 bytes each, identical in every string ≥8 chars, differing only in the randomly minted element `$ID`s. AST `reflect.DeepEqual` between the two orders is the unit-level version of the same claim; both-parse is not enough, since a relaxed grammar over a positional visitor parses and mis-assigns. Found in passing and NOT fixed here: `create workflow … overview page X` writes nothing (`mdl/backend/modelsdk/workflow_write.go` has no `OverviewPage`), while `alter workflow … set overview page` does. Tests `mdl/visitor/visitor_workflow_clause_order_test.go`; repro `mdl-examples/bug-tests/workflow-586-clause-order.mdl` with its `-canonical.mdl` control and `-duplicate-clause.fail.mdl` sibling", "refs": ["ako/mxcli#586"]} +{"area": "mdl/grammar", "date": "2026-09-23", "symptom": "`alter page M.P { set 'createFileAction' = microflow M.ACT_CreateFile on fileUploader1; };` (quoted or bare key) fails to parse: `line 2:37 extraneous input 'MyModule' expecting {DROP, ADD, SET, INSERT, REPLACE, '}'}` — a pluggable widget's NAMED action slot, writable on CREATE PAGE since #956, could only be retargeted by REPLACEing the whole widget", "cause": "`alterPageAssignment` special-cased `Action = actionExprV3` and sent every other key to `propertyValueV3`, which has no `microflow ` form. Below the grammar there was also no route: `SetWidgetProperty` would have stringified an action into `PrimitiveValue`, and `SetWidgetAction` writes the built-in click action, not a pluggable slot", "file": "`mdl/grammar/MDLParser.g4` (`alterPageAssignment`: `STRING_LITERAL|identifierOrKeyword EQUALS actionExprV3`), `mdl/visitor/visitor_alter_page.go` (keeps the author's key), `mdl/executor/cmd_alter_page.go` (`applySetPropertyMutator` routes any `*ast.ActionV3` not keyed `Action`), `mdl/backend/pagemutator/mutator.go` (`SetWidgetNamedAction`); tests `mdl/visitor/visitor_alter_page_named_action_test.go`, `mdl/backend/pagemutator/mutator_named_action_test.go`, `mdl/executor/alter_set_named_action_test.go`; example `mdl-examples/bug-tests/995-alter-page-set-named-action-slot.mdl`", "insight": "**Decide action-slot-ness by the stored PropertyType's `ValueType.Type == \"Action\"`, never by the presence of `Value.Action`** — every WidgetValue carries an Action (a NoAction by default) whatever its type, so a field-presence check would \"succeed\" writing into an Integer and change nothing. Unlike CREATE, ALTER has no datasource overlap to yield to (DataSource is its own alternative), so the action alternative goes before the scalar ones and `microflow M.X` parses straight to an action — no DataSourceV3 conversion. The check-time probe (validate_alter_set.go) dry-runs the same setter, so the wrong-type refusal surfaced at `check -p --references` for free. Measured on 11.12.1: set on a DataGrid 2 with Selection unset (slot hidden) also passed `mx check` at 0 errors — the CE0463 in #956's notes was 11.13.0, so ALTER does not yet run MDL-WIDGET10; don't assume either way without a build on the target version", "refs": ["mendixlabs/mxcli#995", "mendixlabs/mxcli#956"]} diff --git a/.claude/skills/mendix/alter-page/SKILL.md b/.claude/skills/mendix/alter-page/SKILL.md index 2c1fa2afd..cf43be8eb 100644 --- a/.claude/skills/mendix/alter-page/SKILL.md +++ b/.claude/skills/mendix/alter-page/SKILL.md @@ -125,6 +125,11 @@ set Action = microflow Module.ACT_Other on btnSave set Action = SAVE_CHANGES CLOSE_PAGE on btnSave set Action = SHOW_PAGE Module.DetailPage on btnEdit +-- Retarget ONE named action slot of a pluggable widget, by the widget's own +-- property key (the same key `create page` takes: `createFileAction: …`). +set 'createFileAction' = microflow Module.ACT_CreateFile on fileUploader1 +set 'onSelectionChange' = show_page Module.Detail on dgOrders + -- Rebind a data-bound widget set DataSource = $OrderParam on dvOrder set DataSource = microflow Module.MF_Get on dvOrder @@ -145,6 +150,7 @@ so a silent write would build cleanly and then fail to open. | Property | Widget Types | Value Type | Example | |----------|-------------|------------|---------| | `Action` | Widgets with an on-click action (ACTIONBUTTON, LINKBUTTON, clickable containers) | Any `create page` action expression | `set Action = microflow M.ACT_Go on btnSave` | +| `''` | Pluggable widgets — any **action-typed** property (File Uploader `createFileAction`, DataGrid 2 `onSelectionChange`, …) | Any `create page` action expression | `set 'createFileAction' = microflow M.ACT_Create on fileUploader1` — refused, naming the widget's action slots, if the key is not action-typed | | `caption` | ACTIONBUTTON, LINKBUTTON | String | `set caption = 'Submit' on btnSave` | | `content` | DYNAMICTEXT | String | `set content = 'New Heading' on txtTitle` | | `label` | TEXTBOX, TEXTAREA, DATEPICKER, COMBOBOX, CHECKBOX, RADIOBUTTONS | String | `set label = 'full Name' on txtName` | diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index b8df3c17b..c0d3911e3 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -834,6 +834,22 @@ actionbutton btnSubmit ( ) ``` +### Inputs in a List View Need `editable: true` on the List View + +A list view has an `Editable` of its own, default **false** (Mendix's default), +and its read-only context wins over `editable: Always` on an input inside it — +even inside a nested data view. Without it every input renders as a read-only +value, while `mx check` and the build stay clean. `mxcli check` reports this as +MDL-WIDGET31. + +```sql +listview lvRows (datasource: database Mod.Row, editable: true) { + textbox tName (label: 'Name', attribute: Name) +} +``` + +Write `true` unquoted: `editable: 'true'` is a string and is written false. + ### Only a CONTAINER (or a Button) Can Be Clicked `onclick:` is an alias for `action:`, and mxcli writes it for three widget kinds diff --git a/.claude/skills/mendix/write-microflows/SKILL.md b/.claude/skills/mendix/write-microflows/SKILL.md index a5e7e875c..a7252620f 100644 --- a/.claude/skills/mendix/write-microflows/SKILL.md +++ b/.claude/skills/mendix/write-microflows/SKILL.md @@ -388,8 +388,8 @@ toString($value) -- Convert to string > > **MDL044 also blocks `mxcli exec`**, not just `check`: a call to a name Mendix > has no built-in for is CE0117 at build time, so exec refuses to write the -> microflow rather than leaving you to find out from mxbuild. Two names that -> look plausible and are not real: `currentDeviceType()` and `trunc()` (use +> microflow or nanoflow (log messages included). Not real: `currentDeviceType()`, +> `[%CurrentDeviceType%]` (a CE0117 `check` misses) and `trunc()` (use > `round`/`floor`/`ceil`). If exec rejects a function you believe IS a Mendix > built-in, build it once and — if mxbuild accepts it — add it to `funcTable` in > `mdl/exprcheck/func_checker.go`; that table is the rule's only allow-list. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a4577f04..06ab4bc99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **An input inside a list view rendered disabled with every gate green** (ako/mxcli#631) — a `listview` that does not say `editable: true` is written `Editable: false`, and the list view's read-only context wins over `editable: Always` on the input, including inside a nested data view. The BSON is valid, `mx check` is clean and the build succeeds; only the running app shows it. The default is not changed: false is Mendix's own (mendixmodelsdk 4.115.0, `Pages$ListView.editable` defaults to false). `mxcli check` now warns **MDL-WIDGET31** on a list view that will be written read-only while it holds an input not marked `editable: Never`, and names `editable: true` as the fix. A quoted `editable: 'true'` is a string and is written false, so it warns too. + - **Two bundled lint rules reported nothing, on every project, since they were written** — `ARCH002` (entities without a data-change microflow) and `ARCH003` (entities without a business key) both skip an entity with `if entity.entity_type != "PERSISTENT"`. The catalog stores `PERSISTENT`, but `LintContext.Entities` normalizes the kind to `Persistent` before a Starlark rule reads it, so the guard held for every entity and the body never ran. There was nothing to notice: no error, no output, and `--list-rules` still listed both. Measured on a generated app of five persistent entities, `ARCH003` went from 0 findings to 5 once the spelling matched. The `write-lint-rules` skill had documented a third spelling, `"persistent"`, in its field table and in its worked example; both now say what the API returns, and a test loads the two shipped rule files and fails if either stops seeing a persistent entity. - **A Barcode Scanner authored by mxcli failed the build with CE0463** (mendixlabs/mxcli#1161) — `barcodescanner bsCode (datasource: Module.Entity.Code)` was accepted by `mxcli check`, accepted by `mx check`, and then rejected by headless `mxbuild` with `[CE0463] The definition of this widget has changed`, once per instance. The app would not deploy, and the only documented repair was Studio Pro's right-click → "Update widget". diff --git a/cmd/mxcli/diag_loop_report.go b/cmd/mxcli/diag_loop_report.go index a0954ad3e..865cf23d0 100644 --- a/cmd/mxcli/diag_loop_report.go +++ b/cmd/mxcli/diag_loop_report.go @@ -40,6 +40,10 @@ type logRecord struct { Mode string `json:"mode"` CommandsExecuted int `json:"commands_executed"` ErrorsCount int `json:"errors_count"` + PID int `json:"pid"` + // ParentPID is the mxcli process that spawned this one, when one did. It is + // a string because diaglog writes "" for a top-level run (ako/mxcli#629). + ParentPID string `json:"parent_pid"` } // invocation is one mxcli process: a session_start and the session_end that @@ -52,6 +56,11 @@ type invocation struct { Ended bool Commands int Errors int + PID int + // Spawned marks a run mxcli started itself. `mxcli test` runs three before a + // single test executes, so counting them as calls the agent made overstates + // the loop and understates `test` (ako/mxcli#629). + Spawned bool } // Duration is wall time for an invocation that closed. An invocation that did @@ -90,7 +99,11 @@ type loopReport struct { // path through all ~250 os.Exit sites. StatementErrors int `json:"runs_with_statement_errors"` // Unclosed is runs with no session_end. See the comment at its increment. - Unclosed int `json:"unclosed"` + Unclosed int `json:"unclosed"` + // Spawned is runs mxcli started itself, excluded from every other figure + // here. They are real processes and their time is already inside their + // parent's, so counting them again would double it (ako/mxcli#629). + Spawned int `json:"spawned_by_mxcli"` WallSeconds float64 `json:"wall_seconds"` ByVerb []verbStats `json:"by_verb"` CheckExecDup int `json:"check_then_exec_pairs"` @@ -167,22 +180,43 @@ func invocationScript(args []string) string { // so rather than pretending otherwise. func buildInvocations(records []logRecord) []invocation { var out []invocation + open := map[int]int{} // pid -> index of its open invocation cur := -1 for _, r := range records { switch r.Msg { case "session_start": out = append(out, invocation{ - Verb: invocationVerb(r.Args, r.Mode), - Script: invocationScript(r.Args), - Start: r.Time, + Verb: invocationVerb(r.Args, r.Mode), + Script: invocationScript(r.Args), + Start: r.Time, + PID: r.PID, + Spawned: r.ParentPID != "", }) cur = len(out) - 1 + if r.PID != 0 { + open[r.PID] = cur + } case "session_end": - if cur >= 0 && !out[cur].Ended { - out[cur].End = r.Time - out[cur].Ended = true - out[cur].Commands = r.CommandsExecuted - out[cur].Errors = r.ErrorsCount + // Pair by pid when the log has one. `mxcli test` spawns three mxcli + // processes before a single test runs, so their session_ends arrive + // between the parent's start and its own end; closing "the most + // recent open invocation" hands the parent's close to a child and + // reports the parent as a failure. Measured: every `test` run showed + // as unclosed although all its tests passed (ako/mxcli#629). + i := cur + if r.PID != 0 { + j, ok := open[r.PID] + if !ok { + continue // an end whose start is outside this window + } + i = j + delete(open, r.PID) + } + if i >= 0 && !out[i].Ended { + out[i].End = r.Time + out[i].Ended = true + out[i].Commands = r.CommandsExecuted + out[i].Errors = r.ErrorsCount } } } @@ -199,12 +233,21 @@ func analyzeLoop(records []logRecord) loopReport { for _, inv := range invs { // The report never counts itself. Since ako/mxcli#617 every command is - // recorded from PersistentPreRun, which excludes `diag` for this reason; + // recorded from startSession, which excludes `diag` for this reason; // the filter stays as the second guard, because a report whose numbers // depend on one exclusion staying in place would drift silently. if inv.Verb == "diag" || strings.HasPrefix(inv.Verb, "diag ") { continue } + // A run mxcli started itself is not a call anyone made. `mxcli test` + // spawns `-c DESCRIBE SETTINGS`, `-c SHOW MODULES` and an `exec` of the + // generated runner before the first test executes, so a session of 5 + // test runs carried 15 phantom entries in the table the report exists to + // rank. Counted on its own line instead (ako/mxcli#629). + if inv.Spawned { + rep.Spawned++ + continue + } s, ok := stats[inv.Verb] if !ok { s = &verbStats{Verb: inv.Verb} @@ -335,6 +378,12 @@ func renderLoopReport(rep loopReport, w io.Writer) { " unclosed runs above, which exited instead)\n", rep.StatementErrors) } + if rep.Spawned > 0 { + fmt.Fprintf(w, "Started by mxcli itself: %d (test, new, eval and the LSP run mxcli;\n"+ + " excluded above — their time is already inside the run\n"+ + " that started them)\n", rep.Spawned) + } + fmt.Fprintln(w, "\nBy command, most calls first:") fmt.Fprintf(w, " %-22s %6s %8s %10s %9s\n", "COMMAND", "CALLS", "UNCLOSED", "TOTAL", "MEDIAN") for _, s := range rep.ByVerb { diff --git a/cmd/mxcli/diag_loop_report_test.go b/cmd/mxcli/diag_loop_report_test.go index 4c50e8afe..5edf0ef0f 100644 --- a/cmd/mxcli/diag_loop_report_test.go +++ b/cmd/mxcli/diag_loop_report_test.go @@ -233,3 +233,108 @@ func TestStatementErrorLineIsPrintedOnlyWhenNonZero(t *testing.T) { t.Errorf("did not print the statement-error line at 1:\n%s", loud.String()) } } + +// pidStart / pidEnd carry the pid pairing that ako/mxcli#629 added. +func pidStart(sec, pid int, parent string, mode string, args ...string) logRecord { + r := startAt(sec, mode, args...) + r.PID = pid + r.ParentPID = parent + return r +} + +func pidEnd(sec, pid, cmds, errs int) logRecord { + r := endAt(sec, cmds, errs) + r.PID = pid + return r +} + +// mxcli runs mxcli. `mxcli test` spawns `-c DESCRIBE SETTINGS`, `-c SHOW +// MODULES` and an `exec` of the generated runner BEFORE the first test +// executes, and `new`, `eval`, `tui` and the LSP do the same. +// +// The records below are the shape measured from a real `mxcli test` run, not an +// invented one: one parent start, three child starts each naming the parent, and +// the children's ends arriving inside the parent's lifetime. +// +// That broke the report twice over. Closing "the most recent open invocation" +// handed the parent's close to a child, so every `test` run was reported as +// never having closed — a test project saw 5 of 5 unclosed while every test +// passed. And the children were counted as calls the agent made, putting 3 +// phantom entries per test run into the table the report exists to rank. +func TestSpawnedRunsDoNotSwallowTheirParentsClose(t *testing.T) { + rep := analyzeLoop([]logRecord{ + pidStart(0, 100, "", "subcommand", "test", "t.test.mdl", "-p", "x.mpr"), + pidStart(1, 101, "100", "batch", "-p", "x.mpr", "-c", "DESCRIBE SETTINGS"), + pidEnd(2, 101, 1, 0), + pidStart(3, 102, "100", "batch", "-p", "x.mpr", "-c", "SHOW MODULES"), + pidEnd(4, 102, 1, 0), + pidStart(5, 103, "100", "subcommand", "exec", "runner.mdl", "-p", "x.mpr"), + pidEnd(6, 103, 9, 0), + pidEnd(10, 100, 0, 0), // the parent closes LAST, long after its children + }) + + if rep.Invocations != 1 { + t.Errorf("Invocations = %d, want 1 — the three spawned runs are not calls "+ + "anyone made", rep.Invocations) + } + if rep.Spawned != 3 { + t.Errorf("Spawned = %d, want 3", rep.Spawned) + } + if rep.Unclosed != 0 { + t.Errorf("Unclosed = %d, want 0 — the parent DID close; a child's end was "+ + "being counted as its own", rep.Unclosed) + } + // 10s parent. The children's 1s each must NOT be added: their time is + // already inside the parent's, so counting both doubles it. + if rep.WallSeconds != 10 { + t.Errorf("WallSeconds = %v, want 10 (the parent alone; children are inside it)", + rep.WallSeconds) + } +} + +// CONTROL 1: a log written before #621 has no pid on either record, and must +// still pair positionally — otherwise the fix silently blanks older logs, which +// are exactly the ones a before/after comparison needs. +func TestLogsWithoutPidsStillPairPositionally(t *testing.T) { + rep := analyzeLoop([]logRecord{ + startAt(0, "subcommand", "exec", "a.mdl", "-p", "x.mpr"), + endAt(2, 3, 0), + startAt(3, "subcommand", "check", "b.mdl", "-p", "x.mpr"), + // no end + }) + if rep.Invocations != 2 || rep.Unclosed != 1 || rep.WallSeconds != 2 { + t.Errorf("old-format log: Invocations=%d Unclosed=%d Wall=%v, want 2, 1, 2", + rep.Invocations, rep.Unclosed, rep.WallSeconds) + } +} + +// CONTROL 2: a top-level run that happens to carry a pid is NOT spawned. Without +// this the fix could be "treat everything with a pid as a child", which would +// hide the whole loop rather than three calls per test run. +func TestPidAloneDoesNotMakeARunSpawned(t *testing.T) { + rep := analyzeLoop([]logRecord{ + pidStart(0, 200, "", "subcommand", "exec", "a.mdl", "-p", "x.mpr"), + pidEnd(1, 200, 2, 0), + }) + if rep.Spawned != 0 { + t.Errorf("Spawned = %d, want 0 — parent_pid is empty, so nothing spawned it", + rep.Spawned) + } + if rep.Invocations != 1 { + t.Errorf("Invocations = %d, want 1", rep.Invocations) + } +} + +// CONTROL 3: a session_end whose start is before the window (log rotation, or a +// --since cut) must be dropped, not applied to whichever invocation happens to +// be open. That mis-attribution is the same class of bug as the one above. +func TestEndWithoutItsStartIsDropped(t *testing.T) { + rep := analyzeLoop([]logRecord{ + pidEnd(0, 999, 5, 0), // its start is in yesterday's file + pidStart(1, 300, "", "subcommand", "check", "a.mdl", "-p", "x.mpr"), + }) + if rep.Unclosed != 1 { + t.Errorf("Unclosed = %d, want 1 — the orphan end must not close the check", + rep.Unclosed) + } +} diff --git a/cmd/mxcli/lsp_diagnostics.go b/cmd/mxcli/lsp_diagnostics.go index 7dc7f535a..10e06bda9 100644 --- a/cmd/mxcli/lsp_diagnostics.go +++ b/cmd/mxcli/lsp_diagnostics.go @@ -327,6 +327,7 @@ func (s *mdlServer) runSemanticValidation(text string) []protocol.Diagnostic { // The editor reports an unusable parameter annotation for the same // reason `check` does — a typo of @position parses and does nothing. if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { + violations = append(violations, executor.ValidateNanoflow(nfStmt)...) violations = append(violations, executor.ValidateFlowParameterAnnotations( "nanoflow '"+nfStmt.Name.String()+"'", nfStmt.Parameters)...) } diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index 9c4bf5e4b..e4db31239 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -32,10 +32,22 @@ func main() { fmt.Fprint(os.Stderr, warningBanner) } + // Open the process session before cobra sees argv. Cobra validates a + // command's Args before any hook runs, so a session started from + // PersistentPreRun missed every run with the wrong number of arguments + // (ako/mxcli#633). + startSession(os.Args[1:]) + if err := rootCmd.Execute(); err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + // Close only on a normal return. A failure that exits through os.Exit — + // almost every one — leaves no session_end, and that absence is what + // `diag loop-report` reads as a non-zero exit. Closing here rather than in + // PersistentPostRun also covers --help and --version, which cobra answers + // before any hook and which would otherwise read as failed runs. + diaglog.CloseCurrent() } // shouldSuppressWarning checks if the warning should be suppressed @@ -101,21 +113,6 @@ Examples: `, Version: version, PersistentPreRun: func(cmd *cobra.Command, args []string) { - // Record the invocation before anything can reject it. `diag loop-report` - // reports "mxcli invocations: N", and it used to count only the commands - // that happened to build a logged executor — 14 files of 53 registered - // commands — and only when the run survived long enough to reach that - // code. A `check` without -p was invisible, and so was every run that - // exited on a bad argument, which is exactly the agent failure the report - // exists to show (ako/mxcli#617). Init is a per-process singleton, so the - // commands that call it later get this same logger. - // - // diag is excluded: a report that counted its own runs would climb every - // time it was read. - if !strings.HasPrefix(cmd.CommandPath(), cmd.Root().Name()+" diag") { - _ = diaglog.Init(version, cmd.CommandPath()) - } - projectPath, _ := cmd.Flags().GetString("project") if projectPath == "" { if discovered := discoverProjectPath(); discovered != "" { @@ -135,12 +132,6 @@ Examples: globalMCPTrace, _ = cmd.Flags().GetBool("mcp-trace") globalEngineFlag, _ = cmd.Flags().GetString("engine") }, - PersistentPostRun: func(cmd *cobra.Command, args []string) { - // Closes the session opened in PersistentPreRun. Cobra skips this when a - // command exits through os.Exit, which is how almost every failure path - // ends — so "no session_end" stays the tell for a non-zero exit. - diaglog.CloseCurrent() - }, Run: func(cmd *cobra.Command, args []string) { // Get flags commands, _ := cmd.Flags().GetString("command") diff --git a/cmd/mxcli/session_start.go b/cmd/mxcli/session_start.go new file mode 100644 index 000000000..a42a7fade --- /dev/null +++ b/cmd/mxcli/session_start.go @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "strings" + + "github.com/mendixlabs/mxcli/mdl/diaglog" +) + +// startSession records the invocation before anything can reject it. +// `diag loop-report` reports "mxcli invocations: N", and burning calls on +// malformed invocations is exactly the agent failure it is opened to see +// (ako/mxcli#617). It runs from main() rather than PersistentPreRun because +// cobra validates Args before any hook: an arity failure used to return +// before anything was logged (ako/mxcli#633). +// +// Init is a per-process singleton, so the commands that call it later get this +// same logger — and whatever mode is chosen here is the one recorded. +func startSession(args []string) { + if mode, ok := sessionMode(args); ok { + _ = diaglog.Init(version, mode) + } +} + +// sessionMode names the session the way the command would once it ran: the +// subcommand's path, or for the root command the mode its own Run passes — +// "batch" for -c, "repl" otherwise — which invocationVerb falls back to when +// argv names no subcommand. ok is false for diag: a report that counted its +// own runs would climb every time it was read. +func sessionMode(args []string) (mode string, ok bool) { + cmd, _, err := rootCmd.Find(args) + if err != nil || cmd == nil { + // An unknown subcommand is still a wasted call; name it by the root. + return rootCmd.Name(), true + } + path := cmd.CommandPath() + if strings.HasPrefix(path, rootCmd.Name()+" diag") { + return "", false + } + if cmd != rootCmd { + return path, true + } + // --help and --version are answered by cobra without reaching Run, so the + // root is neither a one-shot nor a REPL there. + if hasFlag(args, "-h", "--help") { + return "help", true + } + if hasFlag(args, "", "--version") { + return "version", true + } + if hasCommandFlag(args) { + return "batch", true + } + return "repl", true +} + +// hasCommandFlag reports whether args pass the root's -c / --command, in any +// of the forms pflag accepts. +func hasCommandFlag(args []string) bool { + for _, a := range args { + if a == "--" { + return false + } + switch { + case a == "-c", a == "--command", strings.HasPrefix(a, "--command="): + return true + case strings.HasPrefix(a, "-c") && !strings.HasPrefix(a, "--"): + return true // -cVALUE, -c=VALUE + } + } + return false +} + +// hasFlag reports whether args pass a boolean flag by its short or long name. +func hasFlag(args []string, short, long string) bool { + for _, a := range args { + if a == "--" { + return false + } + if (short != "" && a == short) || a == long || strings.HasPrefix(a, long+"=") { + return true + } + } + return false +} diff --git a/cmd/mxcli/session_start_test.go b/cmd/mxcli/session_start_test.go new file mode 100644 index 000000000..147fcb957 --- /dev/null +++ b/cmd/mxcli/session_start_test.go @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// A command called with the wrong number of arguments wrote no session record +// at all (ako/mxcli#633). Cobra runs ValidateArgs BEFORE PersistentPreRun, so +// with diaglog.Init in PersistentPreRun an arity failure returned before +// anything was logged, and `diag loop-report` never saw the wasted call. +// +// These tests run the real main() in a child process: the failure path ends in +// os.Exit, and the point is what reaches the log file before it does. + +const runMainEnv = "MXCLI_TEST_RUN_MAIN" + +// TestRunMainHelper is not a test: it is main() for the child process. +func TestRunMainHelper(t *testing.T) { + raw := os.Getenv(runMainEnv) + if raw == "" { + t.Skip("helper process only") + } + var args []string + if err := json.Unmarshal([]byte(raw), &args); err != nil { + t.Fatalf("decode args: %v", err) + } + os.Args = append([]string{"mxcli"}, args...) + main() + os.Exit(0) +} + +// runMain runs mxcli with args in a child process, logging into a fresh +// directory, and returns the session_start records it wrote. +func runMain(t *testing.T, args ...string) []map[string]any { + t.Helper() + return runMainRecords(t, "session_start", args...) +} + +// runMainRecords is runMain for any record type. +func runMainRecords(t *testing.T, msg string, args ...string) []map[string]any { + t.Helper() + logDir := t.TempDir() + enc, _ := json.Marshal(args) + cmd := exec.Command(os.Args[0], "-test.run=^TestRunMainHelper$") + cmd.Dir = t.TempDir() // no .mpr to auto-discover + cmd.Env = append(os.Environ(), + runMainEnv+"="+string(enc), + "MXCLI_LOG_DIR="+logDir, + "MXCLI_LOG=1", + "MXCLI_QUIET=1", + ) + cmd.Stdin = strings.NewReader("") + _ = cmd.Run() // most probes exit non-zero; the log is what is asserted + + var recs []map[string]any + files, _ := filepath.Glob(filepath.Join(logDir, "*.log")) + for _, f := range files { + b, err := os.ReadFile(f) + if err != nil { + t.Fatalf("read log: %v", err) + } + for _, line := range strings.Split(string(b), "\n") { + var r map[string]any + if json.Unmarshal([]byte(line), &r) == nil && r["msg"] == msg { + recs = append(recs, r) + } + } + } + return recs +} + +func TestArityFailureWritesSessionStart(t *testing.T) { + for _, tc := range []struct { + args []string + mode string + }{ + {[]string{"test"}, "mxcli test"}, + {[]string{"check"}, "mxcli check"}, + {[]string{"exec"}, "mxcli exec"}, + // Control: a file error already reached PersistentPreRun and was logged. + {[]string{"check", "/nonexistent.mdl"}, "mxcli check"}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + starts := runMain(t, tc.args...) + if len(starts) != 1 { + t.Fatalf("mxcli %s: want 1 session_start record, got %d", + strings.Join(tc.args, " "), len(starts)) + } + if got := starts[0]["mode"]; got != tc.mode { + t.Errorf("mode = %v, want %q", got, tc.mode) + } + }) + } +} + +// The root command is the -c one-shot or the REPL, and those callers pass +// "batch" / "repl" to the singleton. Starting the session earlier must not +// replace that with a generic name, since invocationVerb falls back to mode +// exactly when argv names no subcommand. +func TestRootSessionKeepsBatchAndReplMode(t *testing.T) { + for _, tc := range []struct { + args []string + mode string + }{ + {[]string{"-c", "show version"}, "batch"}, + {[]string{"--command=show version"}, "batch"}, + {nil, "repl"}, + } { + t.Run(tc.mode+" "+strings.Join(tc.args, " "), func(t *testing.T) { + starts := runMain(t, tc.args...) + if len(starts) != 1 { + t.Fatalf("want 1 session_start record, got %d", len(starts)) + } + if got := starts[0]["mode"]; got != tc.mode { + t.Errorf("mode = %v, want %q", got, tc.mode) + } + }) + } +} + +// An arity failure exits non-zero, so it must stay unclosed — that absence is +// what the report reads as a failed run. +func TestArityFailureLeavesSessionUnclosed(t *testing.T) { + if n := len(runMainRecords(t, "session_end", "check")); n != 0 { + t.Fatalf("mxcli check (no args) wrote %d session_end records, want 0", n) + } +} + +// --help and --version succeed without reaching any cobra hook. With the +// session opened before Execute, a close left in PersistentPostRun would never +// run for them, and every help lookup would read as a failed run. +func TestHelpAndVersionCloseTheirSession(t *testing.T) { + for _, tc := range []struct { + args []string + mode string + }{ + {[]string{"check", "--help"}, "mxcli check"}, + {[]string{"--help"}, "help"}, + {[]string{"--version"}, "version"}, + } { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + starts := runMain(t, tc.args...) + if len(starts) != 1 { + t.Fatalf("want 1 session_start record, got %d", len(starts)) + } + if got := starts[0]["mode"]; got != tc.mode { + t.Errorf("mode = %v, want %q", got, tc.mode) + } + if n := len(runMainRecords(t, "session_end", tc.args...)); n != 1 { + t.Errorf("want 1 session_end record, got %d", n) + } + }) + } +} + +// diag stays excluded: a report that counted its own runs would climb every +// time it was read. +func TestDiagWritesNoSessionStart(t *testing.T) { + if n := len(runMain(t, "diag", "loop-report")); n != 0 { + t.Fatalf("diag loop-report wrote %d session_start records, want 0", n) + } +} diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index 828169068..3b1b8602d 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -244,7 +244,7 @@ CREATE PAGE Sales.Detail (Title: 'Detail', Layout: Atlas_Core.Atlas_Default) { "datasource", "data source", "database", "microflow", "selection", "variable", "binding", "binds", "association", "data from context", }, - Syntax: "DataSource: $Variable -- Parameter/variable binding\nDataSource: DATABASE Module.Entity -- Database query\nDataSource: DATABASE Module.Entity WHERE [Attr != ''] SORT BY Attr ASC\n -- ...optionally constrained and sorted\nDataSource: DATABASE Module.Entity SORT BY Module.Assoc/Attr ASC\n -- ...sorted over an association. Name the\n -- hop when two reach the same entity —\n -- the wrong one builds cleanly and sorts\n -- by the wrong thing.\nDataSource: DATABASE Module.Entity SEARCH BY Attr, Attr2\n -- LIST VIEW only: the attributes its\n -- search bar filters on. Mirrors SORT BY,\n -- but takes no direction.\nDataSource: MICROFLOW Module.MF -- Microflow datasource, no parameters\nDataSource: MICROFLOW Module.MF($P) -- ...one argument per PARAMETER, required:\n -- Mendix does NOT auto-map an object in\n -- scope, not even one of the exact type,\n -- so a missing argument is CE1571\nDataSource: SELECTION widgetName -- Selection from another widget\nDataSource: $currentObject/Module.Assoc -- Over an association (\"data from context\")\n -- list widget → to-many collection\n -- nested DATAVIEW → the to-one referenced object\nAttribute: AttributeName -- Attribute binding (inputs)", + Syntax: "DataSource: $Variable -- Parameter/variable binding\nDataSource: DATABASE Module.Entity -- Database query\nDataSource: DATABASE Module.Entity WHERE [Attr != ''] SORT BY Attr ASC\n -- ...optionally constrained and sorted\nDataSource: DATABASE Module.Entity SORT BY Module.Assoc/Attr ASC\n -- ...sorted over an association. Name the\n -- hop when two reach the same entity —\n -- the wrong one builds cleanly and sorts\n -- by the wrong thing.\nDataSource: DATABASE Module.Entity SEARCH BY Attr, Attr2\n -- LIST VIEW only: the attributes its\n -- search bar filters on. Mirrors SORT BY,\n -- but takes no direction.\nDataSource: MICROFLOW Module.MF -- Microflow datasource, no parameters\nDataSource: MICROFLOW Module.MF(Param: $P) -- ...one argument per PARAMETER, required,\n -- and NAMED: the positional form\n -- MF($P) is a parse error. Mendix does\n -- NOT auto-map an object in scope, not\n -- even one of the exact type, so a\n -- missing argument is CE1571\nDataSource: SELECTION widgetName -- Selection from another widget\nDataSource: $currentObject/Module.Assoc -- Over an association (\"data from context\")\n -- list widget → to-many collection\n -- nested DATAVIEW → the to-one referenced object\nAttribute: AttributeName -- Attribute binding (inputs)", Example: "-- Database datasource with grid\nDATAGRID grid (DataSource: DATABASE Module.Customer) {\n COLUMN colName (Attribute: Name, Caption: 'Name')\n}\n\n-- Microflow datasource\nDATAVIEW dv (DataSource: MICROFLOW Module.GetData) {\n TEXTBOX txtName (Label: 'Name', Attribute: Name)\n}\n\n-- Over an association: a nested DataView shows the referenced (to-one) object\nDATAVIEW dvOrder (DataSource: $Order) {\n DATAVIEW dvCustomer (DataSource: $currentObject/Order_Customer) {\n TEXTBOX txtCustName (Label: 'Name', Attribute: Name)\n }\n}\n\n-- Over an association: a list widget shows the (to-many) collection\nLISTVIEW lvLines (DataSource: $currentObject/Order_OrderLine) {\n DYNAMICTEXT dtLine (Content: 'Line')\n}", SeeAlso: []string{"page.widgets", "page.create"}, }) @@ -284,7 +284,7 @@ CREATE PAGE Sales.Detail (Title: 'Detail', Layout: Atlas_Core.Atlas_Default) { "popup width", "popup height", "popup resizable", "drop template", "insert template", "list view template", }, - Syntax: "ALTER PAGE Module.Name {\n SET property = value ON widgetName; -- widget property names: any casing\n SET 'Row size' = 'Small' ON lvOrders; -- an Atlas DESIGN property of that widget's\n -- type; quoted and case-sensitive.\n -- `show design properties for ` lists\n -- them. ON/OFF for a toggle, where OFF\n -- REMOVES the entry.\n -- A multi-select ('Hide on') or compound\n -- ('Spacing') one needs the inline\n -- DesignProperties: [...] form, because a\n -- SET assignment carries one value.\n SET Action = MICROFLOW Module.MF ON btnSave; -- any CREATE PAGE action form\n SET DataSource = $Param ON dvOrder; -- parameter/microflow/nanoflow/selection;\n -- DATABASE and association are REPLACE-only,\n -- and a data view takes no database source\n SET (prop1 = val1, prop2 = val2) ON widgetName;\n SET Title = 'New Title'; -- page-level (case-sensitive)\n SET Documentation = 'What this page is for.';\n SET Class = 'css-class'; -- page-level CSS class / style\n SET Style = 'css: rule';\n SET PopupWidth = 800; -- page-level pop-up dimensions\n SET PopupHeight = 480;\n SET PopupResizable = true;\n INSERT AFTER widgetName { };\n INSERT BEFORE widgetName { };\n INSERT INTO containerName { };\n DROP WIDGET name1, name2;\n DROP TEMPLATE FOR Module.Specialization IN listViewName;\n REPLACE widgetName WITH { };\n};\n\n-- The BULK form: one design property on every widget of a TYPE.\nALTER PAGES [IN Module]\n SET 'Compact' = ON, 'Striped' = ON\n WHERE WIDGETTYPE = datagrid -- the MDL keyword, which resolves to\n -- exactly one widget id. A full id in\n -- quotes works too. NOT a name: a widget\n -- name is unique only within its page.\n [DRY RUN]; -- run this FIRST. It reports the matches\n -- against a discardable copy and writes\n -- nothing.", + Syntax: "ALTER PAGE Module.Name {\n SET property = value ON widgetName; -- widget property names: any casing\n SET 'Row size' = 'Small' ON lvOrders; -- an Atlas DESIGN property of that widget's\n -- type; quoted and case-sensitive.\n -- `show design properties for ` lists\n -- them. ON/OFF for a toggle, where OFF\n -- REMOVES the entry.\n -- A multi-select ('Hide on') or compound\n -- ('Spacing') one needs the inline\n -- DesignProperties: [...] form, because a\n -- SET assignment carries one value.\n SET Action = MICROFLOW Module.MF ON btnSave; -- any CREATE PAGE action form\n SET 'createFileAction' = MICROFLOW Module.MF ON fileUploader1;\n -- a pluggable widget's NAMED action slot,\n -- by the widget's own key; refused on a\n -- key that is not action-typed\n SET DataSource = $Param ON dvOrder; -- parameter/microflow/nanoflow/selection;\n -- DATABASE and association are REPLACE-only,\n -- and a data view takes no database source\n SET (prop1 = val1, prop2 = val2) ON widgetName;\n SET Title = 'New Title'; -- page-level (case-sensitive)\n SET Documentation = 'What this page is for.';\n SET Class = 'css-class'; -- page-level CSS class / style\n SET Style = 'css: rule';\n SET PopupWidth = 800; -- page-level pop-up dimensions\n SET PopupHeight = 480;\n SET PopupResizable = true;\n INSERT AFTER widgetName { };\n INSERT BEFORE widgetName { };\n INSERT INTO containerName { };\n DROP WIDGET name1, name2;\n DROP TEMPLATE FOR Module.Specialization IN listViewName;\n REPLACE widgetName WITH { };\n};\n\n-- The BULK form: one design property on every widget of a TYPE.\nALTER PAGES [IN Module]\n SET 'Compact' = ON, 'Striped' = ON\n WHERE WIDGETTYPE = datagrid -- the MDL keyword, which resolves to\n -- exactly one widget id. A full id in\n -- quotes works too. NOT a name: a widget\n -- name is unique only within its page.\n [DRY RUN]; -- run this FIRST. It reports the matches\n -- against a discardable copy and writes\n -- nothing.", Example: "ALTER PAGE Module.EditPage {\n SET (Caption = 'Save & Close', ButtonStyle = Success) ON btnSave;\n INSERT AFTER txtName {\n TEXTBOX txtMiddleName (Label: 'Middle Name', Attribute: MiddleName)\n };\n DROP WIDGET txtUnused;\n};", SeeAlso: []string{"page.create", "page.show", "snippet.alter"}, }) diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index c18b30fbb..f90564b3e 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -1647,6 +1647,7 @@ Modify an existing page or snippet's widget tree in-place without full `create o | Drop widgets | `drop widget name1, name2` | Remove widgets by name | | Replace widget | `replace widgetName with { widgets }` | Replace widget subtree | | Pluggable prop | `set 'showLabel' = false on cbStatus` | Quoted name for pluggable widgets | +| Named action slot | `set 'createFileAction' = microflow M.ACT_Create on fileUploader1` | A pluggable widget's action-typed property, by its own key; any `create page` action form. Refused on a key that is not action-typed | | Set column prop | `set caption = 'New' on dgGrid.colName` | Dotted ref targets DataGrid column | | Drop column | `drop widget dgGrid.colName` | Remove a DataGrid column | | Insert column | `insert after dgGrid.colName { column ... }` | Add column to DataGrid | diff --git a/mdl-examples/bug-tests/1024-boundary-event-jump-to.mdl b/mdl-examples/bug-tests/1024-boundary-event-jump-to.mdl new file mode 100644 index 000000000..b58bb0199 --- /dev/null +++ b/mdl-examples/bug-tests/1024-boundary-event-jump-to.mdl @@ -0,0 +1,40 @@ +-- `jump to ` inside a boundary-event body (mendixlabs/mxcli#1024). +-- +-- Reported against v0.20.0 / Mendix 11.14.0: `check --references` clean, `exec` +-- written, DESCRIBE reads back `jump to A;`, then native `mx check`: +-- +-- [error] [CE0495] "Duplicate name 'A'." at User task 'A', Jump 'A' +-- [error] [CE6680] "The 'Target' property is required." at Jump 'A' +-- +-- The jump was NAMED after its target instead of only targeting it. Same root +-- cause as #1005, fixed by 825873d6 (a jump is now named JumpTo, and jumps are +-- deduplicated last); v0.20.0 predates that commit. Measured on 11.14.0: the +-- commit before 825873d6 reproduces both errors verbatim, 825873d6 and main give +-- 0 errors. An interrupting boundary event's path must end in `end workflow` or +-- `jump to`, so this is the only way to author one that loops back. +-- +-- Verify on a blank project (`mxcli new … --version 11.14.0`): +-- mxcli exec mdl-examples/bug-tests/1024-boundary-event-jump-to.mdl -p app.mpr +-- mxcli docker check -p app.mpr → 0 errors + +create module Probe; +create persistent entity Probe.Ctx ( Code: string(50) ); + +create page Probe."WF_TaskPage" ( + params: { $Task: System.WorkflowUserTask }, + title: 'Task', layout: Atlas_Core.Atlas_Default +) { + layoutgrid g { row r { column c (desktopwidth: 12) { + dataview dv (datasource: $Task) { textbox tb (attribute: Name, label: 'Task') } + } } } +}; + +create workflow Probe.WF_BoundaryJump + parameter $WorkflowContext: Probe.Ctx +begin + user task A 'A' page Probe.WF_TaskPage outcomes 'Done' { } + boundary event interrupting timer 'addDays([%CurrentDateTime%], 3)' { jump to A; }; + + wait for notification B + boundary event interrupting timer 'addDays([%CurrentDateTime%], 1)' { jump to A; }; +end workflow; diff --git a/mdl-examples/bug-tests/1033-nanoflow-log-expressions-ok.mdl b/mdl-examples/bug-tests/1033-nanoflow-log-expressions-ok.mdl new file mode 100644 index 000000000..3ac72240f --- /dev/null +++ b/mdl-examples/bug-tests/1033-nanoflow-log-expressions-ok.mdl @@ -0,0 +1,28 @@ +-- ============================================================================ +-- Issue #1033 — the accepted counterpart: real functions in nanoflow logs +-- ============================================================================ +-- +-- POSITIVE TEST: `mxcli check` MUST accept this file. +-- +-- The #1033 fix runs MDL044 over nanoflow bodies and over log messages, node +-- and template parameters, and MDL044 is an exec write barrier. Each flow +-- below exercises one of those new sites with a real built-in or token, so a +-- false positive shows up here as a refusal of valid MDL. +-- +-- Verified against mxbuild 11.13.0: each exec'd into a blank 11.13 app checks +-- with 0 errors. +-- +-- The rejected counterpart is 1033-nanoflow-unknown-expression-function.fail.mdl. +-- ============================================================================ + +create nanoflow MyFirstModule.NF_LogTemplate ($x: String) +begin + log info 'x: {1}' with ({1} = toUpperCase($x)); +end; +/ + +create nanoflow MyFirstModule.NF_LogToken () +begin + log info 'now: ' + toString([%CurrentDateTime%]); +end; +/ diff --git a/mdl-examples/bug-tests/1033-nanoflow-unknown-expression-function.fail.mdl b/mdl-examples/bug-tests/1033-nanoflow-unknown-expression-function.fail.mdl new file mode 100644 index 000000000..c55b82cba --- /dev/null +++ b/mdl-examples/bug-tests/1033-nanoflow-unknown-expression-function.fail.mdl @@ -0,0 +1,29 @@ +-- ============================================================================ +-- Issue #1033 — MDL044 ran for CREATE MICROFLOW only, not CREATE NANOFLOW +-- ============================================================================ +-- +-- NEGATIVE TEST (.fail.mdl): `mxcli check` MUST reject this file. An +-- unexpected pass means MDL044 no longer reaches nanoflow bodies (or log +-- messages). +-- +-- The reported repro. Two gaps let it through: MDL044 was wired to microflows +-- alone (#828), and a `log` message was never walked for MDL044 in either +-- flow type. `check` passed, `exec` wrote it, and the build failed: +-- +-- [error] [CE0117] "Error(s) in expression." +-- at Log message activity 'Log message (info)' +-- +-- Verified against mxbuild 11.13.0: exec'd by the pre-fix binary, the project +-- checks with that error; with the fix, check and exec both refuse it. +-- +-- NOT a workaround: `[%CurrentDeviceType%]`, suggested in the report, is also +-- CE0117 on 11.13.0, in a nanoflow and in a microflow. It is not a token. +-- +-- The accepted counterpart is 1033-nanoflow-log-expressions-ok.mdl. +-- ============================================================================ + +create nanoflow MyFirstModule.NF_Dev () +begin + log info 'device: ' + currentDeviceType(); +end; +/ diff --git a/mdl-examples/bug-tests/1034-describe-java-action-type-parameter-name.mdl b/mdl-examples/bug-tests/1034-describe-java-action-type-parameter-name.mdl new file mode 100644 index 000000000..64b3daabb --- /dev/null +++ b/mdl-examples/bug-tests/1034-describe-java-action-type-parameter-name.mdl @@ -0,0 +1,29 @@ +-- ============================================================================ +-- Bug #1034: DESCRIBE JAVA ACTION drops the type-parameter name +-- ============================================================================ +-- +-- Symptom (before fix): +-- A parameter declared `ContextObject: entity not null` was +-- described as `ContextObject: entity <>`. The stored parameter type holds +-- only a BY_ID pointer to the TypeParameter, and the modelsdk java-action +-- reader never resolved that pointer to its name, so the description could +-- not be fed back to recreate the action. +-- +-- After fix: +-- DESCRIBE prints `entity not null`, and a bare type-parameter +-- reference (`Obj: pEntity`, `returns pEntity`) prints its name too. +-- Replaying the description into a fresh project describes identically. +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/1034-describe-java-action-type-parameter-name.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe java action BugTest1034.JA_WithTypeParam" +-- Expected parameter line: ContextObject: entity not null +-- ============================================================================ + +create module BugTest1034; + +create java action BugTest1034.JA_WithTypeParam ( + ContextObject: entity not null +) returns boolean as $$ + return true; +$$; diff --git a/mdl-examples/bug-tests/631-listview-inputs-not-editable.mdl b/mdl-examples/bug-tests/631-listview-inputs-not-editable.mdl new file mode 100644 index 000000000..af83366bf --- /dev/null +++ b/mdl-examples/bug-tests/631-listview-inputs-not-editable.mdl @@ -0,0 +1,61 @@ +-- ako/mxcli#631 — "List views are written with `Editable: false` by mxcli, so +-- every input inside a list view renders disabled — even with `editable: +-- Always` on the text box, and even inside a nested data view." +-- +-- The BSON is valid, `mxcli check` was clean and the build succeeds; the +-- failure only shows in the rendered app. +-- +-- false IS Mendix's default (mendixmodelsdk 4.115.0: Pages$ListView `editable` +-- defaults to false), and Studio Pro agrees: in ako/TestApp (Mendix 11.14.0) +-- 30 of 32 list views are stored false with no input inside, and the only one +-- holding inputs (Pages.EditableLIstView) was set to true. So the writer is not +-- changed. What was missing is a diagnostic: `mxcli check` now warns +-- MDL-WIDGET31 on a list view that is not editable while it holds inputs that +-- are not `editable: Never`. +-- +-- Verify: +-- +-- mxcli check 631-listview-inputs-not-editable.mdl +-- -> MDL-WIDGET31 on lvDefault (textbox tDefault) and lvNested (checkbox +-- cbNested); nothing on lvEditable, lvReadOnly or lvDisplay. + +create module Lv631; +/ + +create or modify entity Lv631.Thing ( + Name: String(100), + Active: Boolean +); +/ + +create or replace page Lv631.P_Inputs +( title: 'List view inputs', layout: Atlas_Core.Atlas_Default ) +{ + -- WARNS: the reported case. The textbox's own `editable: Always` does not help. + listview lvDefault (datasource: database Lv631.Thing) { + textbox tDefault (label: 'Name', attribute: Name, editable: Always) + } + + -- WARNS: a nested data view does not escape the list view's read-only context. + listview lvNested (datasource: database Lv631.Thing) { + dataview dvNested (datasource: $currentObject) { + checkbox cbNested (label: 'Active', attribute: Active) + } + } + + -- The fix. + listview lvEditable (datasource: database Lv631.Thing, editable: true) { + textbox tEditable (label: 'Name', attribute: Name) + } + + -- Intentionally read-only inputs: quiet. + listview lvReadOnly (datasource: database Lv631.Thing) { + textbox tReadOnly (label: 'Name', attribute: Name, editable: Never) + } + + -- No inputs at all: quiet. + listview lvDisplay (datasource: database Lv631.Thing) { + dynamictext dtDisplay (content: '{1}', contentparams: [{1} = Name]) + } +} +/ diff --git a/mdl-examples/bug-tests/632-button-captionparams-bare-attribute.mdl b/mdl-examples/bug-tests/632-button-captionparams-bare-attribute.mdl new file mode 100644 index 000000000..5fd55c5d1 --- /dev/null +++ b/mdl-examples/bug-tests/632-button-captionparams-bare-attribute.mdl @@ -0,0 +1,64 @@ +-- ============================================================================ +-- ako/mxcli#632: captionparams on an action button passes check and is never +-- written. +-- ============================================================================ +-- +-- Reported: +-- +-- `captionparams` on an action button is accepted by `mxcli check` but not +-- written; a container with `onclick` and a dynamic text does the job. +-- +-- Two silent drops, same class: +-- +-- 1. A BARE attribute (`[{1} = Title]`) was written as the string literal +-- 'Title' -- the button had its own copy of the parameter resolver. The +-- same text on a dynamictext binds the attribute, which is why the +-- workaround worked. `check --references` passed and so did `mx check`: +-- a literal is perfectly valid, it just shows the attribute's NAME. +-- 2. DESCRIBE printed a button's parameters as `ContentParams:`, which the +-- button builder never read. describe -> exec passed `check` and wrote +-- every caption with its parameters gone. +-- +-- Measured on a blank Mendix 11.12.1 project: +-- +-- pre-fix describe -> b2 `ContentParams: [{1} = 'Title']` (a literal) +-- describe -> exec -> check passed; captions lose their params +-- mx check -> 4 x CE0720 "Place holder index 1 is greater than 0, +-- the number of parameter(s)" (one per button) +-- +-- fixed describe -> b2 `CaptionParams: [{1} = Title]` (bound) +-- describe -> exec -> "Unchanged page" (round trip stable) +-- mx check -> 0 errors +-- a caption `{1}` with no parameter is now MDL-WIDGET04 at check +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/632-button-captionparams-bare-attribute.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page Bug632.OrderDetail" # b2/b4 bound, not quoted +-- mx check app.mpr # 0 errors +-- ============================================================================ + +create module Bug632; + +create persistent entity Bug632.Order ( + Title: string(100), + Num: integer +); + +create page Bug632.OrderDetail ( + title: 'Order', + layout: Atlas_Core.Atlas_Default, + params: { $o: Bug632.Order } +) { + dataview dv (datasource: $o) { + -- quoted literal: stays a literal + actionbutton b1 (caption: 'Save {1}', captionparams: [{1} = 'order'], action: save_changes) + -- bare attribute: binds Bug632.Order.Title (was the literal 'Title') + actionbutton b2 (caption: 'Save {1}', captionparams: [{1} = Title], action: save_changes) + -- parameter-qualified attribute + actionbutton b3 (caption: 'Save {1}', captionparams: [{1} = $o.Title], action: save_changes) + -- non-string bare attribute + linkbutton b4 (caption: 'Order #{1}', captionparams: [{1} = Num], action: save_changes) + -- the workaround from the report, for comparison: binds the same way + dynamictext t1 (content: 'Save {1}', contentparams: [{1} = Title]) + } +} diff --git a/mdl-examples/bug-tests/978-gallery-slot-names-not-duplicates.mdl b/mdl-examples/bug-tests/978-gallery-slot-names-not-duplicates.mdl new file mode 100644 index 000000000..41e1ce3a7 --- /dev/null +++ b/mdl-examples/bug-tests/978-gallery-slot-names-not-duplicates.mdl @@ -0,0 +1,45 @@ +-- ============================================================================ +-- Bug 978 — DESCRIBE PAGE output rejected by mxcli's own check --references +-- ============================================================================ +-- +-- Symptom (before fix), on a page with several galleries: +-- - duplicate widget name 'template1' (used 4 times) — Mendix requires unique +-- widget names per page (CE0495) +-- +-- Root cause: +-- DESCRIBE writes every gallery's content as `template template1 { … }` (and +-- its filters as `filter filter1 { … }`). Those blocks are CHILD SLOTS of the +-- gallery definition; the pluggable engine stores only their children in the +-- slot property and discards the block's name. The duplicate-name check +-- counted them anyway. (Rows and columns, the other names in the report, were +-- fixed earlier via widgetKindsWithoutStoredNames.) +-- +-- After fix: +-- Child-slot blocks of the enclosing widget's definition are exempt, like +-- object-list items. Widgets INSIDE a slot still count. +-- +-- Verify (Mendix 11.12.2): +-- mxcli check 978-gallery-slot-names-not-duplicates.mdl -p app.mpr --references +-- mxcli exec 978-gallery-slot-names-not-duplicates.mdl -p app.mpr +-- mxcli docker check -p app.mpr -- 0 errors +-- ============================================================================ + +create persistent entity MyFirstModule.Bug978Item (Name: string(100)); + +create page MyFirstModule.Bug978Galleries ( + title: 'Galleries', + layout: Atlas_Core.Atlas_Default +) { + gallery g1 (datasource: database MyFirstModule.Bug978Item) { + template template1 { dynamictext t1 (content: '{1}', contentparams: [{1} = Name]) } + } + gallery g2 (datasource: database MyFirstModule.Bug978Item) { + template template1 { dynamictext t2 (content: '{1}', contentparams: [{1} = Name]) } + } + gallery g3 (datasource: database MyFirstModule.Bug978Item) { + template template1 { dynamictext t3 (content: '{1}', contentparams: [{1} = Name]) } + } + gallery g4 (datasource: database MyFirstModule.Bug978Item) { + template template1 { dynamictext t4 (content: '{1}', contentparams: [{1} = Name]) } + } +} diff --git a/mdl-examples/bug-tests/995-alter-page-set-named-action-slot.mdl b/mdl-examples/bug-tests/995-alter-page-set-named-action-slot.mdl new file mode 100644 index 000000000..7e3ac6ce8 --- /dev/null +++ b/mdl-examples/bug-tests/995-alter-page-set-named-action-slot.mdl @@ -0,0 +1,49 @@ +-- Bug: ALTER PAGE SET could not address a pluggable widget's named action slot. +-- Upstream mendixlabs/mxcli#995 (follow-up to #956). +-- +-- #956 gave named action slots a spelling on CREATE PAGE +-- (`createFileAction: microflow M.F`), but `alter page … { set … }` fell +-- through to a scalar value rule with no `microflow ` form: +-- +-- line 2:37 extraneous input 'MyModule' expecting {DROP, ADD, SET, INSERT, REPLACE, '}'} +-- +-- so retargeting one slot meant REPLACEing the whole widget and restating +-- every other property. +-- +-- Expected: `mxcli check` passes; after exec, DESCRIBE shows +-- `onSelectionChange: microflow M995.ACT_B` AND still `PageSize: 17` (the set +-- touched one slot, nothing else); `mx check` reports 0 errors (measured on +-- 11.12.1). +-- +-- A key that is not action-typed is refused, naming the widget's action slots: +-- set 'PageSize' = microflow M995.ACT_B on dg; +-- -> property "PageSize" of widget "dg" has type Integer, not Action — +-- its action slots are: onClick, onConfigurationChange, onSelectionChange + +create module M995; +create module role M995.User; +create persistent entity M995.Thing ( Name: string(100) ); +create microflow M995.ACT_A () begin log 'A'; end; +create microflow M995.ACT_B () begin log 'B'; end; + +create or replace page M995.P_Grid (Title: 'Grid', Layout: Atlas_Core.Atlas_Default) +{ + DATAGRID dg ( + DataSource: DATABASE M995.Thing, + Selection: Multiple, + PageSize: 17, + onSelectionChange: microflow M995.ACT_A + ) { + COLUMN c1 (Attribute: Name, Caption: 'Name') + } +} + +-- The quoted form (the pluggable-property convention) and the bare form both work. +alter page M995.P_Grid { + set 'onSelectionChange' = microflow M995.ACT_B on dg; +}; +alter page M995.P_Grid { + set onSelectionChange = microflow M995.ACT_B on dg; +}; + +describe page M995.P_Grid; diff --git a/mdl-examples/bug-tests/javascript-action-1137-entity-type-parameter.mdl b/mdl-examples/bug-tests/javascript-action-1137-entity-type-parameter.mdl new file mode 100644 index 000000000..0fe6f6d50 --- /dev/null +++ b/mdl-examples/bug-tests/javascript-action-1137-entity-type-parameter.mdl @@ -0,0 +1,38 @@ +-- mendixlabs/mxcli#1137 +-- CALL JAVASCRIPT ACTION with an entity-type parameter produced the wrong BSON +-- type: Microflows$BasicCodeActionParameterValue { Argument: "" } where +-- Studio Pro stores Microflows$EntityTypeCodeActionParameterValue { Entity: +-- "" }. The Basic shape leaves the entity picker EMPTY in Studio Pro. +-- +-- Measured on mxbuild 11.6.6 against testdata/expr-checker/minimal.mpr, the two +-- variants are: +-- fixed -> "The app contains: 0 errors." +-- faulty -> CE0115 "The arguments that are passed to JavaScript action +-- 'NanoflowCommons.RefreshEntity' do not match the expected +-- parameters and need to be refreshed." +-- So `mx check` DOES catch this one. What does not catch it is mxcli itself: +-- `mxcli check` passes, `exec` reports "Created nanoflow", and DESCRIBE renders +-- the broken and the correct document as identical MDL — both print +-- `EntityToRefresh = Administration.Account`. A describe-based round-trip test +-- therefore cannot detect this; assert on the BSON $Type. +-- +-- NanoflowCommons.RefreshEntity declares +-- EntityToRefresh: entity <> not null +-- i.e. a CodeActions$EntityTypeParameterType — the generic `<>` slot Studio Pro +-- renders as an entity picker. A parameter typed to a CONCRETE entity (e.g. +-- NanoflowCommons.TakePicture's `Picture: System.Image`) is a different case: +-- it takes an object-valued expression and keeps the Basic shape. +-- +-- Needs an app with the NanoflowCommons module (testdata/expr-checker/minimal.mpr). +-- +-- Verify after exec: +-- ./bin/mxcli -p app.mpr -c "describe nanoflow MyFirstModule.ACT_Refresh_NF" +-- and in the raw BSON the parameter value must carry +-- "$Type": "Microflows$EntityTypeCodeActionParameterValue" +-- "Entity": "Administration.Account" +-- and must NOT carry an "Argument" key. + +create or replace nanoflow MyFirstModule.ACT_Refresh_NF () +begin + call javascript action NanoflowCommons.RefreshEntity (EntityToRefresh = Administration.Account); +end; diff --git a/mdl/backend/mcp/page_mutator.go b/mdl/backend/mcp/page_mutator.go index 9829c396e..323e4142c 100644 --- a/mdl/backend/mcp/page_mutator.go +++ b/mdl/backend/mcp/page_mutator.go @@ -245,6 +245,13 @@ func (m *mcpPageMutator) SetWidgetAction(widgetRef string, action pages.ClientAc "(the pg LightPage does not expose widget actions) — widget %q", widgetRef) } +// SetWidgetNamedAction is refused for the same reason SetWidgetAction is: the +// pg LightPage exposes no widget actions to write into. +func (m *mcpPageMutator) SetWidgetNamedAction(widgetRef, propertyKey string, action pages.ClientAction) error { + return fmt.Errorf("setting a widget action is not supported by the MCP backend "+ + "(the pg LightPage does not expose widget actions) — widget %q, slot %q", widgetRef, propertyKey) +} + func (m *mcpPageMutator) SetWidgetDataSource(widgetRef string, ds pages.DataSource) error { _, _, _, w, ok := findWidget(m.content, widgetRef) if !ok { diff --git a/mdl/backend/mock/mock_page_mutator.go b/mdl/backend/mock/mock_page_mutator.go index 3d515b0cf..b0f44ab84 100644 --- a/mdl/backend/mock/mock_page_mutator.go +++ b/mdl/backend/mock/mock_page_mutator.go @@ -21,6 +21,7 @@ type MockPageMutator struct { SetWidgetPropertyFunc func(widgetRef string, prop string, value any) error SetWidgetDataSourceFunc func(widgetRef string, ds pages.DataSource) error SetWidgetActionFunc func(widgetRef string, action pages.ClientAction) error + SetWidgetNamedActionFunc func(widgetRef string, propertyKey string, action pages.ClientAction) error SetColumnPropertyFunc func(gridRef string, columnRef string, prop string, value any) error SetDesignPropertyFunc func(widgetRef string, key string, valueType string, option string) error RemoveDesignPropertyFunc func(widgetRef string, key string) error @@ -74,6 +75,13 @@ func (m *MockPageMutator) SetWidgetAction(widgetRef string, action pages.ClientA return nil } +func (m *MockPageMutator) SetWidgetNamedAction(widgetRef string, propertyKey string, action pages.ClientAction) error { + if m.SetWidgetNamedActionFunc != nil { + return m.SetWidgetNamedActionFunc(widgetRef, propertyKey, action) + } + return nil +} + func (m *MockPageMutator) SetColumnProperty(gridRef string, columnRef string, prop string, value any) error { if m.SetColumnPropertyFunc != nil { return m.SetColumnPropertyFunc(gridRef, columnRef, prop, value) diff --git a/mdl/backend/modelsdk/java_read.go b/mdl/backend/modelsdk/java_read.go index 5ef3ee7a4..0bd514eba 100644 --- a/mdl/backend/modelsdk/java_read.go +++ b/mdl/backend/modelsdk/java_read.go @@ -114,9 +114,32 @@ func javaActionFromGen(g *genJa.JavaAction, containerID model.ID) *javaactions.J out.MicroflowActionInfo = m } out.ReturnType = codeActionReturnTypeFromGen(g.JavaReturnType()) + resolveJavaActionTypeParameterNames(out) return out } +// resolveJavaActionTypeParameterNames fills the display names of type-parameter +// references. The stored types hold only a BY_ID pointer to the TypeParameter, +// and the name is all DESCRIBE prints — unresolved, `entity ` came back +// as `entity <>` and the description no longer round-tripped (#1034). +func resolveJavaActionTypeParameterNames(ja *javaactions.JavaAction) { + for _, p := range ja.Parameters { + switch pt := p.ParameterType.(type) { + case *javaactions.EntityTypeParameterType: + if pt.TypeParameterName == "" { + pt.TypeParameterName = ja.FindTypeParameterName(pt.TypeParameterID) + } + case *javaactions.TypeParameter: + if pt.TypeParameter == "" { + pt.TypeParameter = ja.FindTypeParameterName(pt.TypeParameterID) + } + } + } + if tp, ok := ja.ReturnType.(*javaactions.TypeParameter); ok && tp.TypeParameter == "" { + tp.TypeParameter = ja.FindTypeParameterName(tp.TypeParameterID) + } +} + // codeActionParamTypeFromGen converts a gen parameter-type element to the semantic // type (inverse of codeActionParamTypeToGen). func codeActionParamTypeFromGen(el element.Element) javaactions.CodeActionParameterType { diff --git a/mdl/backend/modelsdk/java_read_test.go b/mdl/backend/modelsdk/java_read_test.go index ad94050b7..05bb598f6 100644 --- a/mdl/backend/modelsdk/java_read_test.go +++ b/mdl/backend/modelsdk/java_read_test.go @@ -104,3 +104,61 @@ func TestReadJavaActionByName_MicroflowParameterType(t *testing.T) { got.Parameters[1].ParameterType) } } + +// DESCRIBE JAVA ACTION printed `ContextObject: entity <>` for a parameter +// declared `entity not null` (mendixlabs/mxcli#1034). The stored +// parameter type holds only a BY_ID pointer to the TypeParameter; the reader +// carried the ID across but never resolved it to the name, which is all the +// describer prints. The JavaScript-action reader already did this resolution. +func TestReadJavaActionByName_ResolvesTypeParameterNames(t *testing.T) { + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + mod, err := b.GetModuleByName("MyFirstModule") + if err != nil || mod == nil { + t.Fatalf("GetModuleByName: %v", err) + } + tp := &javaactions.TypeParameterDef{Name: "pEntity"} + tp.ID = model.ID("0b6f0d0e-1034-4a00-8000-000000000001") + ja := &javaactions.JavaAction{ + ContainerID: mod.ID, + Name: "ZzJaTypeParam", + TypeParameters: []*javaactions.TypeParameterDef{tp}, + Parameters: []*javaactions.JavaActionParameter{ + {Name: "ContextObject", IsRequired: true, + ParameterType: &javaactions.EntityTypeParameterType{TypeParameterID: tp.ID, TypeParameterName: "pEntity"}}, + {Name: "Obj", IsRequired: true, + ParameterType: &javaactions.TypeParameter{TypeParameterID: tp.ID, TypeParameter: "pEntity"}}, + }, + ReturnType: &javaactions.TypeParameter{TypeParameterID: tp.ID, TypeParameter: "pEntity"}, + } + if err := b.CreateJavaAction(ja); err != nil { + t.Fatalf("CreateJavaAction: %v", err) + } + + got, err := b.ReadJavaActionByName("MyFirstModule.ZzJaTypeParam") + if err != nil { + t.Fatalf("ReadJavaActionByName: %v", err) + } + if len(got.Parameters) != 2 { + t.Fatalf("params = %d, want 2", len(got.Parameters)) + } + etp, ok := got.Parameters[0].ParameterType.(*javaactions.EntityTypeParameterType) + if !ok { + t.Fatalf("param 0 type = %T, want *EntityTypeParameterType", got.Parameters[0].ParameterType) + } + if etp.TypeParameterName != "pEntity" { + t.Errorf("entity type parameter name = %q, want %q (describe prints `entity <%s>`)", + etp.TypeParameterName, "pEntity", etp.TypeParameterName) + } + if p, ok := got.Parameters[1].ParameterType.(*javaactions.TypeParameter); !ok || p.TypeParameter != "pEntity" { + t.Errorf("param 1 type = %#v, want TypeParameter{pEntity}", got.Parameters[1].ParameterType) + } + if r, ok := got.ReturnType.(*javaactions.TypeParameter); !ok || r.TypeParameter != "pEntity" { + t.Errorf("return type = %#v, want TypeParameter{pEntity}", got.ReturnType) + } +} diff --git a/mdl/backend/modelsdk/microflow_action_test.go b/mdl/backend/modelsdk/microflow_action_test.go index bb7f9e3da..bcaf045eb 100644 --- a/mdl/backend/modelsdk/microflow_action_test.go +++ b/mdl/backend/modelsdk/microflow_action_test.go @@ -300,3 +300,70 @@ func TestActionFromGen_WorkflowActions(t *testing.T) { t.Errorf("NotifyWorkflow WorkflowVariable lost: %+v", got) } } + +// TestMicroflowActionToGen_JavaScriptActionEntityTypeParameter pins the BSON the +// write path produces for a JavaScript-action entity-type parameter +// (mendixlabs/mxcli#1137). The reported symptom was an `mxcli bson dump` +// showing +// +// "$Type": "Microflows$BasicCodeActionParameterValue", +// "Argument": "CustomModule.BufferDefinition" +// +// where Studio Pro stores $Type Microflows$EntityTypeCodeActionParameterValue +// with the entity under the "Entity" key. The wrong shape leaves the Studio Pro +// entity picker empty and fails mxbuild with CE0115 (measured on 11.6.6). +// The builder chooses the value (mdl/executor); this asserts the chosen value +// survives to BSON under the right $Type and key rather than being flattened on +// the way out. +func TestMicroflowActionToGen_JavaScriptActionEntityTypeParameter(t *testing.T) { + g := microflowActionToGen(µflows.JavaScriptActionCallAction{ + JavaScriptAction: "NanoflowCommons.RefreshEntity", + ParameterMappings: []*microflows.JavaScriptActionParameterMapping{ + { + Parameter: "NanoflowCommons.RefreshEntity.EntityToRefresh", + Value: µflows.EntityTypeCodeActionParameterValue{Entity: "CustomModule.BufferDefinition"}, + }, + }, + }) + if g == nil { + t.Fatal("nil action") + } + encoded, err := (&codec.Encoder{}).Encode(g) + if err != nil { + t.Fatalf("encode: %v", err) + } + + mappings, err := bson.Raw(encoded).LookupErr("ParameterMappings") + if err != nil { + t.Fatalf("no ParameterMappings in encoded action: %v", err) + } + vals, err := mappings.Array().Values() + if err != nil { + t.Fatalf("ParameterMappings array: %v", err) + } + // Element 0 is Mendix's typed-array marker (an int), not a mapping. + var docs []bson.Raw + for _, v := range vals { + if doc, ok := v.DocumentOK(); ok { + docs = append(docs, doc) + } + } + if len(docs) != 1 { + t.Fatalf("ParameterMappings documents = %d, want 1", len(docs)) + } + value, err := docs[0].LookupErr("ParameterValue") + if err != nil { + t.Fatalf("mapping has no ParameterValue: %v", err) + } + valueDoc := value.Document() + + if got := valueDoc.Lookup("$Type").StringValue(); got != "Microflows$EntityTypeCodeActionParameterValue" { + t.Fatalf("$Type = %q, want Microflows$EntityTypeCodeActionParameterValue", got) + } + if got := valueDoc.Lookup("Entity").StringValue(); got != "CustomModule.BufferDefinition" { + t.Errorf("Entity = %q, want CustomModule.BufferDefinition", got) + } + if _, err := valueDoc.LookupErr("Argument"); err == nil { + t.Error("value carries an Argument key; that is the BasicCodeActionParameterValue shape") + } +} diff --git a/mdl/backend/mutation.go b/mdl/backend/mutation.go index f04e295cd..3c9db9400 100644 --- a/mdl/backend/mutation.go +++ b/mdl/backend/mutation.go @@ -84,6 +84,11 @@ type PageMutator interface { // Refuses a widget that has no Action property. SetWidgetAction(widgetRef string, action pages.ClientAction) error + // SetWidgetNamedAction writes an action into a pluggable widget's action + // slot addressed by the widget's own property key (e.g. the File Uploader's + // createFileAction). Refuses a key that is not an action-typed property. + SetWidgetNamedAction(widgetRef string, propertyKey string, action pages.ClientAction) error + // SetColumnProperty sets a property on a column within a grid widget. SetColumnProperty(gridRef string, columnRef string, prop string, value any) error diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index 0c8983e9f..669d00115 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -221,6 +221,91 @@ func (m *Mutator) SetWidgetAction(widgetRef string, action pages.ClientAction) e return nil } +// SetWidgetNamedAction writes an action into a pluggable widget's action slot +// addressed by the widget's own property key — the ALTER-level twin of CREATE +// PAGE's `createFileAction: microflow M.F` (#956), so one mis-wired slot can be +// retargeted without REPLACEing the widget and restating everything else +// (mendixlabs/mxcli#995). It writes the same Value.Action field +// widgetobj.Builder.SetAction does. +// +// The property TYPE decides, not the presence of the field: every stored +// WidgetValue carries an Action (a NoAction by default) whatever its type, so +// writing one into an Integer property would build clean and do nothing. +func (m *Mutator) SetWidgetNamedAction(widgetRef, propertyKey string, action pages.ClientAction) error { + result := m.widgetFinder(m.rawData, widgetRef) + if result == nil { + return m.widgetNotFoundError(widgetRef) + } + obj := bsonnav.DGetDoc(result.widget, "Object") + if obj == nil { + return fmt.Errorf("widget %q (%s) is not a pluggable widget and has no named action slots — "+ + "a built-in widget's click action is set with `set Action = … on %s`", + widgetRef, widgetTypeName(result.widget), widgetRef) + } + + keys, kinds := pluggablePropertyTypes(result.widget) + var actionSlots []string + for id, key := range keys { + if kinds[id] == "Action" { + actionSlots = append(actionSlots, key) + } + } + sort.Strings(actionSlots) + + for _, prop := range bsonnav.DGetArrayElements(bsonnav.DGet(obj, "Properties")) { + propDoc, ok := prop.(bson.D) + if !ok { + continue + } + id := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(propDoc, "TypePointer")) + if key := keys[id]; key == "" || !strings.EqualFold(key, propertyKey) { + continue + } + if kinds[id] != "Action" { + return fmt.Errorf("property %q of widget %q has type %s, not Action — "+ + "its action slots are: %s", propertyKey, widgetRef, kinds[id], slotList(actionSlots)) + } + valDoc := bsonnav.DGetDoc(propDoc, "Value") + if valDoc == nil { + return fmt.Errorf("property %q has no Value map", propertyKey) + } + serialized := m.deps.SerializeClientAction(action) + if serialized == nil { + return fmt.Errorf("unsupported action type %T", action) + } + bsonnav.DSet(valDoc, "Action", serialized) + return nil + } + return fmt.Errorf("widget %q has no action slot %q — its action slots are: %s", + widgetRef, propertyKey, slotList(actionSlots)) +} + +// pluggablePropertyTypes maps a pluggable widget's PropertyType IDs to their +// keys and to their value types ("Action", "Integer", …). +func pluggablePropertyTypes(widget bson.D) (keys, kinds map[string]string) { + keys = buildPropKeyMap(widget) + kinds = make(map[string]string, len(keys)) + objType := bsonnav.DGetDoc(bsonnav.DGetDoc(widget, "Type"), "ObjectType") + for _, pt := range bsonnav.DGetArrayElements(bsonnav.DGet(objType, "PropertyTypes")) { + ptDoc, ok := pt.(bson.D) + if !ok { + continue + } + id := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(ptDoc, "$ID")) + if vt := bsonnav.DGetDoc(ptDoc, "ValueType"); vt != nil && id != "" { + kinds[id] = bsonnav.DGetString(vt, "Type") + } + } + return keys, kinds +} + +func slotList(slots []string) string { + if len(slots) == 0 { + return "(none)" + } + return strings.Join(slots, ", ") +} + // widgetTypeName reports a widget's $Type for error messages, or "unknown type". func widgetTypeName(widget bson.D) string { if t, ok := bsonnav.DGet(widget, "$Type").(string); ok && t != "" { diff --git a/mdl/backend/pagemutator/mutator_named_action_test.go b/mdl/backend/pagemutator/mutator_named_action_test.go new file mode 100644 index 000000000..1f59d0c19 --- /dev/null +++ b/mdl/backend/pagemutator/mutator_named_action_test.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + "go.mongodb.org/mongo-driver/bson/primitive" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +func noAction() bson.D { + return bson.D{{Key: "$Type", Value: "Forms$NoAction"}, {Key: "DisabledDuringExecution", Value: true}} +} + +// makeFileUploader builds a CustomWidget shaped like the File Uploader: two +// action-typed slots and one integer property. Every WidgetValue carries an +// Action field whatever its type — the builder's default value writes a +// NoAction into all of them — which is why the setter has to consult the +// property TYPE rather than the presence of the field. +func makeFileUploader(name string) bson.D { + id := func(b byte) primitive.Binary { + return primitive.Binary{Subtype: 0x04, Data: []byte{b, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}} + } + propType := func(b byte, key, valueType string) bson.D { + return bson.D{ + {Key: "$ID", Value: id(b)}, + {Key: "PropertyKey", Value: key}, + {Key: "ValueType", Value: bson.D{{Key: "Type", Value: valueType}}}, + } + } + prop := func(b byte, primitiveValue string) bson.D { + return bson.D{ + {Key: "TypePointer", Value: id(b)}, + {Key: "Value", Value: bson.D{ + {Key: "Action", Value: noAction()}, + {Key: "PrimitiveValue", Value: primitiveValue}, + }}, + } + } + return bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidget"}, + {Key: "Name", Value: name}, + {Key: "Type", Value: bson.D{ + {Key: "$Type", Value: "CustomWidgets$CustomWidgetType"}, + {Key: "ObjectType", Value: bson.D{ + {Key: "PropertyTypes", Value: bson.A{ + int32(2), + propType(0xA1, "createFileAction", "Action"), + propType(0xA2, "createImageAction", "Action"), + propType(0xA3, "maxFileSize", "Integer"), + }}, + }}, + }}, + {Key: "Object", Value: bson.D{ + {Key: "Properties", Value: bson.A{ + int32(2), + prop(0xA1, ""), + prop(0xA2, ""), + prop(0xA3, "25"), + }}, + }}, + } +} + +// slotAction returns the Action stored on the pluggable property at index i. +func slotAction(t *testing.T, m *Mutator, widget string, i int) bson.D { + t.Helper() + result := m.widgetFinder(m.rawData, widget) + if result == nil { + t.Fatalf("widget %q not found", widget) + } + props := bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(result.widget, "Object"), "Properties")) + return bsonnav.DGetDoc(bsonnav.DGetDoc(props[i].(bson.D), "Value"), "Action") +} + +var microflowMarker = bson.D{ + {Key: "$Type", Value: "Forms$MicroflowClientAction"}, + {Key: "Microflow", Value: "MyModule.ACT_CreateFile"}, +} + +// TestSetWidgetNamedAction_WritesTheSlot is the storage half of +// mendixlabs/mxcli#995: `set 'createFileAction' = microflow M.F on +// fileUploader1` has to land in that slot's Value.Action, the same place the +// CREATE path's widgetobj.Builder.SetAction writes it, and nowhere else. +func TestSetWidgetNamedAction_WritesTheSlot(t *testing.T) { + for _, spelling := range []string{"createFileAction", "CreateFileAction"} { + t.Run(spelling, func(t *testing.T) { + m := New(makeRawPage(makeFileUploader("fileUploader1")), model.ID("u"), + &stubActionDeps{serialized: microflowMarker}) + + err := m.SetWidgetNamedAction("fileUploader1", spelling, + &pages.MicroflowClientAction{MicroflowName: "MyModule.ACT_CreateFile"}) + if err != nil { + t.Fatalf("SetWidgetNamedAction: %v", err) + } + if got := bsonnav.DGetString(slotAction(t, m, "fileUploader1", 0), "Microflow"); got != "MyModule.ACT_CreateFile" { + t.Errorf("createFileAction = %q, want MyModule.ACT_CreateFile", got) + } + if got := bsonnav.DGetString(slotAction(t, m, "fileUploader1", 1), "$Type"); got != "Forms$NoAction" { + t.Errorf("createImageAction = %q, want it untouched (Forms$NoAction)", got) + } + }) + } +} + +// TestSetWidgetNamedAction_RefusesNonActionProperty is the guard. Every +// WidgetValue has an Action field, so writing one into an integer property +// would "succeed" and be ignored by Mendix — a set that reports success and +// changes nothing. It has to be an error that names the action slots. +func TestSetWidgetNamedAction_RefusesNonActionProperty(t *testing.T) { + m := New(makeRawPage(makeFileUploader("fileUploader1")), model.ID("u"), + &stubActionDeps{serialized: microflowMarker}) + + err := m.SetWidgetNamedAction("fileUploader1", "maxFileSize", + &pages.MicroflowClientAction{MicroflowName: "M.F"}) + if err == nil { + t.Fatal("expected an error writing an action into an Integer property") + } + for _, want := range []string{"maxFileSize", "createFileAction", "createImageAction"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error should mention %q, got: %v", want, err) + } + } + if got := bsonnav.DGetString(slotAction(t, m, "fileUploader1", 2), "$Type"); got != "Forms$NoAction" { + t.Errorf("a refused set must not write: maxFileSize Action = %q", got) + } +} + +func TestSetWidgetNamedAction_UnknownKeyAndWidget(t *testing.T) { + m := New(makeRawPage(makeFileUploader("fileUploader1")), model.ID("u"), + &stubActionDeps{serialized: microflowMarker}) + if err := m.SetWidgetNamedAction("fileUploader1", "createFilAction", + &pages.MicroflowClientAction{MicroflowName: "M.F"}); err == nil || + !strings.Contains(err.Error(), "createFilAction") { + t.Errorf("unknown key: got %v, want an error naming it", err) + } + if err := m.SetWidgetNamedAction("noSuchWidget", "createFileAction", + &pages.MicroflowClientAction{MicroflowName: "M.F"}); err == nil { + t.Error("unknown widget: got nil error") + } +} + +// TestSetWidgetNamedAction_RefusesBuiltInWidget: a Forms$ActionButton has no +// pluggable slots; its click action is `set Action = …`. +func TestSetWidgetNamedAction_RefusesBuiltInWidget(t *testing.T) { + btn := bson.D{{Key: "$Type", Value: "Forms$ActionButton"}, {Key: "Name", Value: "btnGo"}, {Key: "Action", Value: noAction()}} + m := New(makeRawPage(btn), model.ID("u"), &stubActionDeps{serialized: microflowMarker}) + err := m.SetWidgetNamedAction("btnGo", "onClick", &pages.MicroflowClientAction{MicroflowName: "M.F"}) + if err == nil || !strings.Contains(err.Error(), "set Action") { + t.Errorf("got %v, want a refusal pointing at `set Action`", err) + } +} diff --git a/mdl/diaglog/diaglog.go b/mdl/diaglog/diaglog.go index c1ee30d08..20b21d310 100644 --- a/mdl/diaglog/diaglog.go +++ b/mdl/diaglog/diaglog.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "runtime" + "strconv" "strings" "sync" "time" @@ -22,17 +23,23 @@ import ( type Logger struct { slog *slog.Logger file *os.File + pid int cmdCount int errCount int startTime time.Time closed bool } +// parentPIDEnv names the mxcli process that spawned this one, when one did. +// Set by Init and inherited by every child, so a self-spawned run is +// distinguishable from a call the user or agent made (ako/mxcli#629). +const parentPIDEnv = "MXCLI_SESSION_PID" + // One session per process. // // `diag loop-report` counts session records, so a process that opened two would // count twice and one that opened none would be invisible. Init is called from -// PersistentPreRun for every command and again by each command that builds a +// mxcli's main() for every command and again by each command that builds a // logged executor; the second call has to return the SAME logger (ako/mxcli#617). var ( mu sync.Mutex @@ -40,7 +47,7 @@ var ( ) // CloseCurrent ends the process session, if one was started. It is called from -// PersistentPostRun, which cobra runs only when the command returned normally — +// main() only when the command returned normally — // so a run that exits through os.Exit leaves no session_end, and that absence is // what `diag loop-report` reads as a non-zero exit. func CloseCurrent() { @@ -96,6 +103,22 @@ func Init(version, mode string) *Logger { } current = l + l.pid = os.Getpid() + + // mxcli spawns mxcli: `test` runs `-c DESCRIBE SETTINGS`, `-c SHOW MODULES` + // and an `exec` of the generated runner before a single test executes, and + // `new`, `eval`, `tui` and the LSP do the same. Each child logs a session of + // its own, so without a marker the report counts them as calls the agent made + // — 3 phantom calls per `mxcli test` — and shows the parent as never having + // closed, because a session_start used to end the open invocation. + // + // The marker rides the ENVIRONMENT rather than each spawn site: a child + // started with exec.Command inherits this process's environment (explicitly + // via os.Environ(), or implicitly when Cmd.Env is nil), so setting it once + // here covers all six self-spawn sites and any added later without touching + // them (ako/mxcli#629). + parent := os.Getenv(parentPIDEnv) + os.Setenv(parentPIDEnv, strconv.Itoa(l.pid)) // Write session header l.slog.Info("session_start", @@ -105,7 +128,8 @@ func Init(version, mode string) *Logger { "arch", runtime.GOARCH, "mode", mode, "args", os.Args, - "pid", os.Getpid(), + "pid", l.pid, + "parent_pid", parent, ) return l @@ -128,6 +152,10 @@ func (l *Logger) Close() { "commands_executed", l.cmdCount, "errors_count", l.errCount, "duration_s", int(time.Since(l.startTime).Seconds()), + // The pid is what pairs this with its session_start. Without it a reader + // can only guess by position, which is wrong the moment one mxcli runs + // another — and mxcli runs itself routinely (ako/mxcli#629). + "pid", l.pid, ) l.file.Close() } diff --git a/mdl/executor/alter_set_named_action_test.go b/mdl/executor/alter_set_named_action_test.go new file mode 100644 index 000000000..f0dc6312d --- /dev/null +++ b/mdl/executor/alter_set_named_action_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// TestApplySetProperty_RoutesNamedActionSlot is the executor half of +// mendixlabs/mxcli#995: an action written against any key other than `Action` +// is a pluggable widget's named slot, and has to reach SetWidgetNamedAction +// with the author's key — not SetWidgetProperty, which would stringify the +// action into a PrimitiveValue, and not SetWidgetAction, which writes the +// built-in click action. +func TestApplySetProperty_RoutesNamedActionSlot(t *testing.T) { + var gotWidget, gotKey string + var gotAction pages.ClientAction + var wrongPath string + m := &mock.MockPageMutator{ + SetWidgetNamedActionFunc: func(w, key string, a pages.ClientAction) error { + gotWidget, gotKey, gotAction = w, key, a + return nil + }, + SetWidgetPropertyFunc: func(string, string, any) error { wrongPath = "SetWidgetProperty"; return nil }, + SetWidgetActionFunc: func(string, pages.ClientAction) error { wrongPath = "SetWidgetAction"; return nil }, + } + ctx, _ := newMockCtx(t) + op := &ast.SetPropertyOp{ + Target: ast.WidgetRef{Widget: "fileUploader1"}, + Properties: map[string]any{"createFileAction": &ast.ActionV3{Type: "save", ClosePage: true}}, + } + if err := applySetPropertyMutator(ctx, m, op, "MyModule", model.ID("mod")); err != nil { + t.Fatalf("set: %v", err) + } + if wrongPath != "" { + t.Fatalf("the named slot went through %s", wrongPath) + } + if gotWidget != "fileUploader1" || gotKey != "createFileAction" { + t.Errorf("SetWidgetNamedAction(%q, %q), want (fileUploader1, createFileAction)", gotWidget, gotKey) + } + if _, ok := gotAction.(*pages.SaveChangesClientAction); !ok { + t.Errorf("action = %T, want *pages.SaveChangesClientAction — built by the CREATE PAGE action builder", gotAction) + } +} + +// Control: `set Action = …` keeps its own route to the built-in click action. +func TestApplySetProperty_ActionKeepsItsRoute(t *testing.T) { + var named, click bool + m := &mock.MockPageMutator{ + SetWidgetNamedActionFunc: func(string, string, pages.ClientAction) error { named = true; return nil }, + SetWidgetActionFunc: func(string, pages.ClientAction) error { click = true; return nil }, + } + ctx, _ := newMockCtx(t) + op := &ast.SetPropertyOp{ + Target: ast.WidgetRef{Widget: "btnGo"}, + Properties: map[string]any{"Action": &ast.ActionV3{Type: "save"}}, + } + if err := applySetPropertyMutator(ctx, m, op, "MyModule", model.ID("mod")); err != nil { + t.Fatalf("set: %v", err) + } + if !click || named { + t.Errorf("SetWidgetAction called = %v, SetWidgetNamedAction called = %v; want true, false", click, named) + } +} + +// A named action slot on a column or at page level has no setter that can hold +// it — both would write the action as a scalar — so it is refused. +func TestApplySetProperty_NamedActionNeedsWidgetTarget(t *testing.T) { + for _, target := range []ast.WidgetRef{{}, {Widget: "dg", Column: "Name"}} { + m := &mock.MockPageMutator{} + ctx, _ := newMockCtx(t) + op := &ast.SetPropertyOp{ + Target: target, + Properties: map[string]any{"createFileAction": &ast.ActionV3{Type: "save"}}, + } + if err := applySetPropertyMutator(ctx, m, op, "MyModule", model.ID("mod")); err == nil { + t.Errorf("target %q: expected a refusal", target.Name()) + } + } +} diff --git a/mdl/executor/button_caption_params_632_test.go b/mdl/executor/button_caption_params_632_test.go new file mode 100644 index 000000000..adb836c0f --- /dev/null +++ b/mdl/executor/button_caption_params_632_test.go @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: Apache-2.0 + +// An action button's caption parameters (ako/mxcli#632). +// +// Reported: "`captionparams` on an action button is accepted by `mxcli check` +// but not written; a container with `onclick` and a dynamic text does the job." +// +// Two silent drops, both measured on a real 11.12.1 app: +// +// - The button carried its own copy of the parameter resolver, which read a +// bare attribute (`[{1} = Title]`) as the string literal 'Title'. The same +// text on a dynamictext binds the attribute. `check --references` passed, so +// the button rendered the word "Title". +// - DESCRIBE printed the button's parameters as `ContentParams:`, which the +// button builder never read. Re-executing a description passed `check` and +// wrote the caption with every parameter gone. +package executor + +import ( + "bytes" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" +) + +func buttonCaptionPB() *pageBuilder { + return &pageBuilder{ + paramEntityNames: map[string]string{"o": "Rp.Order"}, + widgetScope: map[string]model.ID{}, + entityContext: "Rp.Order", + } +} + +// A bare attribute name binds the attribute in the dataview's context, exactly +// as it does on a dynamictext — not a literal holding the attribute's name. +func TestButtonCaptionParams_BareAttributeBindsAttribute(t *testing.T) { + btn, err := buttonCaptionPB().buildButtonV3(&ast.WidgetV3{ + Type: "actionbutton", Name: "b", + Properties: map[string]any{ + "Caption": "Bare {1}", + "CaptionParams": []ast.ParamAssignmentV3{{Index: 1, Value: "Title"}}, + "Action": "SAVE_CHANGES", + }, + }) + if err != nil { + t.Fatalf("buildButtonV3: %v", err) + } + params := btn.CaptionTemplate.Parameters + if len(params) != 1 { + t.Fatalf("caption has %d parameter(s), want 1", len(params)) + } + p := params[0] + if p.Expression != "" { + t.Errorf("bare attribute written as expression %q — the button shows the attribute's NAME, not its value", p.Expression) + } + if p.AttributeRef != "Rp.Order.Title" { + t.Errorf("AttributeRef = %q, want Rp.Order.Title", p.AttributeRef) + } +} + +// A quoted literal stays a literal. +func TestButtonCaptionParams_QuotedLiteralStaysLiteral(t *testing.T) { + btn, err := buttonCaptionPB().buildButtonV3(&ast.WidgetV3{ + Type: "actionbutton", Name: "b", + Properties: map[string]any{ + "Caption": "Lit {1}", + "CaptionParams": []ast.ParamAssignmentV3{{Index: 1, Value: "'Hello'"}}, + }, + }) + if err != nil { + t.Fatalf("buildButtonV3: %v", err) + } + p := btn.CaptionTemplate.Parameters[0] + if p.Expression != "'Hello'" || p.AttributeRef != "" { + t.Errorf("param = {Expression:%q AttributeRef:%q}, want the literal 'Hello'", p.Expression, p.AttributeRef) + } +} + +// DESCRIBE's historical spelling for a button, `ContentParams:`, is honoured so +// that descriptions written before this fix still re-execute with their +// parameters. +func TestButtonCaptionParams_ContentParamsAliasIsRead(t *testing.T) { + btn, err := buttonCaptionPB().buildButtonV3(&ast.WidgetV3{ + Type: "actionbutton", Name: "b", + Properties: map[string]any{ + "Caption": "Dollar {1}", + "ContentParams": []ast.ParamAssignmentV3{{Index: 1, Value: "Title"}}, + }, + }) + if err != nil { + t.Fatalf("buildButtonV3: %v", err) + } + if n := len(btn.CaptionTemplate.Parameters); n != 1 { + t.Fatalf("caption has %d parameter(s) — `ContentParams:` on a button was dropped, leaving {1} unbound", n) + } +} + +// DESCRIBE names the property the language documents for a button. +func TestDescribeButtonEmitsCaptionParams(t *testing.T) { + var buf bytes.Buffer + outputWidgetMDLV3(&ExecContext{Output: &buf}, rawWidget{ + Type: "Forms$ActionButton", + Name: "b", + Caption: "Bare {1}", + Parameters: []string{"Title"}, + Action: "save_changes", + }, 0) + out := buf.String() + if !strings.Contains(out, "CaptionParams: [{1} = Title]") { + t.Errorf("button parameters not described as CaptionParams:\n%s", out) + } + if strings.Contains(out, "ContentParams") { + t.Errorf("button described with ContentParams (the dynamictext name):\n%s", out) + } +} + +// A caption placeholder with no parameter is CE0720 at build time. It is what an +// old description re-executed to, and `check` said nothing: the orphan check +// covered dynamictext only. +func TestButtonCaptionOrphanPlaceholderIsFlagged(t *testing.T) { + for _, kw := range []string{"actionbutton", "linkbutton"} { + orphan := &ast.WidgetV3{Type: kw, Name: "b", Properties: map[string]any{"Caption": "Save {1}"}} + if v := validateDynamicTextPlaceholders(orphan, "page X"); v == nil { + t.Errorf("%s: caption {1} with no parameter passed — mx check reports CE0720", kw) + } + for _, key := range []string{"CaptionParams", "ContentParams"} { + bound := &ast.WidgetV3{Type: kw, Name: "b", Properties: map[string]any{ + "Caption": "Save {1}", + key: []ast.ParamAssignmentV3{{Index: 1, Value: "'x'"}}, + }} + if v := validateDynamicTextPlaceholders(bound, "page X"); v != nil { + t.Errorf("%s with %s: false positive: %s", kw, key, v.Message) + } + } + } +} diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index 4b31ae52d..a15536ff2 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -160,6 +160,14 @@ func applySetPropertyMutator(ctx *ExecContext, mutator backend.PageMutator, op * for _, propName := range propNames { value := op.Properties[propName] + if _, isAction := value.(*ast.ActionV3); isAction && propName != "Action" && + (op.Target.Widget == "" || op.Target.IsColumn()) { + // A named action slot belongs to a pluggable widget. The column and + // page-level setters would stringify the action into a scalar. + return mdlerrors.NewValidationf( + "`set %s = ` needs a pluggable widget target: `set '%s' = … on `", + propName, propName) + } if op.Target.IsColumn() { if err := mutator.SetColumnProperty(op.Target.Widget, op.Target.Column, propName, value); err != nil { return mdlerrors.NewBackend("set "+propName+" on "+op.Target.Name(), err) @@ -183,6 +191,19 @@ func applySetPropertyMutator(ctx *ExecContext, mutator backend.PageMutator, op * if err := mutator.SetWidgetAction(op.Target.Widget, action); err != nil { return mdlerrors.NewBackend("set Action on "+op.Target.Name(), err) } + } else if _, isAction := value.(*ast.ActionV3); isAction { + // Any other key carrying an action is a pluggable widget's NAMED + // action slot — `set 'createFileAction' = microflow M.F` (#995). + // Same builder as `Action`; the mutator checks the key is an + // action-typed property of the stored widget. Through + // SetWidgetProperty it would be stringified into a PrimitiveValue. + action, err := convertASTAction(ctx, value, moduleName, moduleID) + if err != nil { + return err + } + if err := mutator.SetWidgetNamedAction(op.Target.Widget, propName, action); err != nil { + return mdlerrors.NewBackend("set "+propName+" on "+op.Target.Name(), err) + } } else if p := designPropertyForStoredWidget( ctx.GetThemeRegistry(), mutator, op.Target.Widget, propName); p != nil { // An Atlas design property of THIS stored widget. It lives in diff --git a/mdl/executor/cmd_microflows_build.go b/mdl/executor/cmd_microflows_build.go index 332b88456..0b9a9e68a 100644 --- a/mdl/executor/cmd_microflows_build.go +++ b/mdl/executor/cmd_microflows_build.go @@ -445,6 +445,14 @@ func buildNanoflowFromStmt(ctx *ExecContext, s *ast.CreateNanoflowStmt, opts bui return nil, err } + // The nanoflow half of buildMicroflowFromStmt's validateMicroflowRules call: + // without it, exec wrote what check reported (mendixlabs/mxcli#1033). + if opts.AllowCreate { + if err := validateNanoflowRules(s); err != nil { + return nil, err + } + } + // Find the module, and the folder, WITHOUT creating either on a dry run: // findOrCreateModule and resolveFolder both write, and `diff` must render a // proposed flow against an unmodified project. diff --git a/mdl/executor/cmd_microflows_builder_calls.go b/mdl/executor/cmd_microflows_builder_calls.go index e4f9c7f5d..d5a0aae07 100644 --- a/mdl/executor/cmd_microflows_builder_calls.go +++ b/mdl/executor/cmd_microflows_builder_calls.go @@ -485,17 +485,71 @@ func (fb *flowBuilder) inferGenericJavaActionReturnType(jaDef *javaactions.JavaA func (fb *flowBuilder) addCallJavaScriptActionAction(s *ast.CallJavaScriptActionStmt) model.ID { actionQN := s.ActionName.Module + "." + s.ActionName.Name + // Look up the JavaScript action definition to detect entity-type + // parameters — the `entity <>` slots Studio Pro renders as an entity + // picker (CodeActions$EntityTypeParameterType). Their value is the + // chosen entity's qualified name under + // Microflows$EntityTypeCodeActionParameterValue.Entity, NOT an + // expression under BasicCodeActionParameterValue.Argument. Writing the + // Basic shape leaves the picker empty in Studio Pro, and fails the build + // with CE0115 "the arguments ... do not match the expected parameters" + // (measured on mxbuild 11.6.6). What reports the nanoflow as correct is + // mxcli itself: `mxcli check` passes, exec says "Created nanoflow", and + // DESCRIBE renders the broken and the correct document as identical MDL + // (mendixlabs/mxcli#1137). The Java-action builder above has drawn this + // distinction since ako/mxcli#656; this path hardcoded Basic for every + // parameter. + // + // Note this is NOT the same as a parameter typed to a concrete entity + // (CodeActions$EntityType, e.g. NanoflowCommons.TakePicture's `Picture: + // System.Image`): that one takes an object-valued expression and keeps + // the Basic shape. + // + // Without a backend (mxcli check with no project) the signature cannot + // be resolved, so the prior Basic shape stands rather than guessing the + // parameter's kind from how the argument is spelled. + entityTypeParams := make(map[string]bool) + if fb.backend != nil { + jsaDef, err := fb.backend.ReadJavaScriptActionByName(actionQN) + if err != nil { + log.Printf("warning: could not look up JavaScript action %s: %v (entity type params will be empty)", actionQN, err) + } else if jsaDef != nil { + for _, p := range jsaDef.Parameters { + if _, ok := p.ParameterType.(*types.EntityTypeParameterType); ok { + entityTypeParams[p.Name] = true + } + } + } + } + // Build parameter mappings with Value structure var mappings []*microflows.JavaScriptActionParameterMapping for _, arg := range s.Arguments { // Parameter qualified name format: Module.JavaScriptAction.ParameterName paramQN := actionQN + "." + arg.Name - // JavaScript actions use BasicCodeActionParameterValue for all parameters + var value microflows.CodeActionParameterValue valueExpr := fb.exprToString(arg.Value) - value := µflows.BasicCodeActionParameterValue{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - Argument: valueExpr, + if entityTypeParams[arg.Name] { + // Entity-type parameter: the value is an entity qualified name. + // When the argument is a variable like $Order, resolve the entity + // it holds from varTypes, mirroring the Java-action builder. + entityName := strings.Trim(valueExpr, "'") + if strings.HasPrefix(entityName, "$") { + varName := strings.TrimPrefix(entityName, "$") + if resolvedType, ok := fb.varTypes[varName]; ok { + entityName = resolvedType + } + } + value = µflows.EntityTypeCodeActionParameterValue{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Entity: entityName, + } + } else { + value = µflows.BasicCodeActionParameterValue{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Argument: valueExpr, + } } mapping := µflows.JavaScriptActionParameterMapping{ diff --git a/mdl/executor/cmd_microflows_builder_js_action_test.go b/mdl/executor/cmd_microflows_builder_js_action_test.go new file mode 100644 index 000000000..4ca305572 --- /dev/null +++ b/mdl/executor/cmd_microflows_builder_js_action_test.go @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// jsActionActivity runs the CALL JAVASCRIPT ACTION builder and returns the +// action it produced. +func jsActionActivity(t *testing.T, fb *flowBuilder, stmt *ast.CallJavaScriptActionStmt) *microflows.JavaScriptActionCallAction { + t.Helper() + id := fb.addCallJavaScriptActionAction(stmt) + for _, obj := range fb.objects { + if obj.GetID() != id { + continue + } + activity, ok := obj.(*microflows.ActionActivity) + if !ok { + t.Fatalf("object = %T, want *ActionActivity", obj) + } + action, ok := activity.Action.(*microflows.JavaScriptActionCallAction) + if !ok { + t.Fatalf("action = %T, want *JavaScriptActionCallAction", activity.Action) + } + return action + } + t.Fatal("expected JavaScript action activity") + return nil +} + +// TestBuildJavaScriptAction_EntityTypeParameterEmitsEntityValue reproduces +// mendixlabs/mxcli#1137: a nanoflow calling a JavaScript action with an +// entity-type parameter (`NanoflowCommons.RefreshEntity`, whose EntityToRefresh +// is declared `entity <> not null`, i.e. a CodeActions$EntityTypeParameterType) +// was written as +// +// "$Type": "Microflows$BasicCodeActionParameterValue", +// "Argument": "CustomModule.BufferDefinition" +// +// where Studio Pro stores +// +// "$Type": "Microflows$EntityTypeCodeActionParameterValue", +// "Entity": "CustomModule.BufferDefinition" +// +// The Basic shape leaves the entity picker empty in Studio Pro. Measured on +// mxbuild 11.6.6, it is also CE0115 "The arguments that are passed to +// JavaScript action 'NanoflowCommons.RefreshEntity' do not match the expected +// parameters and need to be refreshed" — the same code finding #36 recorded for +// the microflow-typed Java action parameter. The Java-action builder has made +// this distinction since ako/mxcli#656; the JavaScript-action builder hardcoded +// Basic for every parameter. +func TestBuildJavaScriptAction_EntityTypeParameterEmitsEntityValue(t *testing.T) { + fb := &flowBuilder{ + posX: 100, + posY: 100, + spacing: HorizontalSpacing, + backend: &mock.MockBackend{ + ReadJavaScriptActionByNameFunc: func(qualifiedName string) (*types.JavaScriptAction, error) { + if qualifiedName != "NanoflowCommons.RefreshEntity" { + t.Fatalf("javascript action lookup = %q", qualifiedName) + } + return &types.JavaScriptAction{ + Name: "RefreshEntity", + Parameters: []*types.JavaActionParameter{ + {Name: "EntityToRefresh", ParameterType: &types.EntityTypeParameterType{}}, + }, + }, nil + }, + }, + } + stmt := &ast.CallJavaScriptActionStmt{ + ActionName: ast.QualifiedName{Module: "NanoflowCommons", Name: "RefreshEntity"}, + Arguments: []ast.CallArgument{ + {Name: "EntityToRefresh", Value: &ast.IdentifierExpr{Name: "CustomModule.BufferDefinition"}}, + }, + } + + action := jsActionActivity(t, fb, stmt) + if len(action.ParameterMappings) != 1 { + t.Fatalf("parameter mappings = %d, want 1", len(action.ParameterMappings)) + } + value, ok := action.ParameterMappings[0].Value.(*microflows.EntityTypeCodeActionParameterValue) + if !ok { + t.Fatalf("value = %T, want *EntityTypeCodeActionParameterValue (issue #1137: Basic leaves the Studio Pro entity picker empty)", action.ParameterMappings[0].Value) + } + if value.Entity != "CustomModule.BufferDefinition" { + t.Errorf("Entity = %q, want CustomModule.BufferDefinition", value.Entity) + } +} + +// TestBuildJavaScriptAction_ConcreteEntityParameterStaysBasic is the control for +// the test above: a parameter declared with a CONCRETE entity type +// (CodeActions$EntityType, e.g. NanoflowCommons.TakePicture's `Picture: +// System.Image`) is bound to an object-valued expression, not to an entity name, +// so it keeps the Basic shape. Without this case a fix that promoted every +// entity-ish parameter would pass. +func TestBuildJavaScriptAction_ConcreteEntityParameterStaysBasic(t *testing.T) { + fb := &flowBuilder{ + posX: 100, + posY: 100, + spacing: HorizontalSpacing, + backend: &mock.MockBackend{ + ReadJavaScriptActionByNameFunc: func(qualifiedName string) (*types.JavaScriptAction, error) { + return &types.JavaScriptAction{ + Name: "TakePicture", + Parameters: []*types.JavaActionParameter{ + {Name: "Picture", ParameterType: &types.EntityType{Entity: "System.Image"}}, + }, + }, nil + }, + }, + } + stmt := &ast.CallJavaScriptActionStmt{ + ActionName: ast.QualifiedName{Module: "NanoflowCommons", Name: "TakePicture"}, + Arguments: []ast.CallArgument{ + {Name: "Picture", Value: &ast.VariableExpr{Name: "Picture"}}, + }, + } + + action := jsActionActivity(t, fb, stmt) + value, ok := action.ParameterMappings[0].Value.(*microflows.BasicCodeActionParameterValue) + if !ok { + t.Fatalf("value = %T, want *BasicCodeActionParameterValue", action.ParameterMappings[0].Value) + } + if value.Argument != "$Picture" { + t.Errorf("Argument = %q, want $Picture", value.Argument) + } +} + +// TestBuildJavaScriptAction_NoBackendStaysBasic pins the offline path: with no +// backend to resolve the signature (mxcli check without -p), the builder cannot +// know the parameter is entity-typed and keeps the prior Basic shape rather than +// guessing from the argument's spelling. +func TestBuildJavaScriptAction_NoBackendStaysBasic(t *testing.T) { + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing} + stmt := &ast.CallJavaScriptActionStmt{ + ActionName: ast.QualifiedName{Module: "NanoflowCommons", Name: "RefreshEntity"}, + Arguments: []ast.CallArgument{ + {Name: "EntityToRefresh", Value: &ast.IdentifierExpr{Name: "CustomModule.BufferDefinition"}}, + }, + } + + action := jsActionActivity(t, fb, stmt) + if _, ok := action.ParameterMappings[0].Value.(*microflows.BasicCodeActionParameterValue); !ok { + t.Fatalf("value = %T, want *BasicCodeActionParameterValue without a backend", action.ParameterMappings[0].Value) + } +} diff --git a/mdl/executor/cmd_pages_builder_v3_widgets.go b/mdl/executor/cmd_pages_builder_v3_widgets.go index 2d2e8733b..374f58cd9 100644 --- a/mdl/executor/cmd_pages_builder_v3_widgets.go +++ b/mdl/executor/cmd_pages_builder_v3_widgets.go @@ -955,31 +955,17 @@ func (pb *pageBuilder) buildButtonV3(w *ast.WidgetV3) (*pages.ActionButton, erro }, } - // Handle CaptionParams (template parameters like {1}, {2}) - if params := w.GetCaptionParams(); params != nil { - for _, p := range params { - param := &pages.ClientTemplateParameter{ - BaseElement: model.BaseElement{ - ID: model.ID(types.GenerateID()), - TypeName: "Forms$ClientTemplateParameter", - }, - } - // Check if it's an attribute reference or literal - if strVal, ok := p.Value.(string); ok { - if strings.HasPrefix(strVal, "'") || strings.HasPrefix(strVal, "\"") { - // Already a quoted string literal - use as-is - param.Expression = strVal - } else if strings.HasPrefix(strVal, "$") || strings.Contains(strVal, ".") { - // Attribute reference - resolve widget references to entity paths - param.AttributeRef = pb.resolveTemplateAttributePath(strVal) - } else { - // Unquoted literal value - wrap in quotes for expression - param.Expression = "'" + strVal + "'" - } - } - btn.CaptionTemplate.Parameters = append(btn.CaptionTemplate.Parameters, param) - } + // CaptionParams bind through the same resolver as a dynamictext's + // ContentParams, so `[{1} = Title]` binds the attribute on both. The + // button used to carry its own copy that wrote a bare name as the literal + // 'Title' (#632). `ContentParams:` is what DESCRIBE printed for a button + // before #632, so it is read too — otherwise re-executing an old + // description drops every parameter and leaves {1} unbound. + params := w.GetCaptionParams() + if params == nil { + params = w.GetContentParams() } + btn.CaptionTemplate.Parameters = pb.buildClientTemplateParams(params) } // Handle ButtonStyle. Normalize case (so `primary` becomes `Primary`) and diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index c174cb389..8c131d324 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -451,7 +451,7 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = append(props, fmt.Sprintf("Caption: %s", mdlQuote(w.Caption))) } if len(w.Parameters) > 0 { - props = append(props, fmt.Sprintf("ContentParams: [%s]", strings.Join(formatParametersV3(w.Parameters), ", "))) + props = append(props, fmt.Sprintf("CaptionParams: [%s]", strings.Join(formatParametersV3(w.Parameters), ", "))) } if w.Action != "" { props = append(props, fmt.Sprintf("Action: %s", w.Action)) diff --git a/mdl/executor/issue1024_boundary_jump_test.go b/mdl/executor/issue1024_boundary_jump_test.go new file mode 100644 index 000000000..d4257eec6 --- /dev/null +++ b/mdl/executor/issue1024_boundary_jump_test.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// mendixlabs/mxcli#1024 — `jump to A;` inside a boundary-event body wrote a Jump +// NAMED A instead of one targeting A: +// +// [error] [CE0495] "Duplicate name 'A'." at User task 'A', Jump 'A' +// [error] [CE6680] "The 'Target' property is required." at Jump 'A' +// +// Same root cause as #1005 (buildJumpTo named the jump after its target, so the +// target resolved to the jump itself); reported against v0.20.0, which predates +// that fix. The #1005 tests only put jumps in outcome flows — this pins the +// boundary-event body, which is the only way to end an interrupting boundary +// event's path besides `end workflow`. + +// allActivities flattens the tree, boundary-event bodies included. +func allActivities(acts []workflows.WorkflowActivity) []workflows.WorkflowActivity { + var out []workflows.WorkflowActivity + for _, a := range acts { + out = append(out, a) + for _, f := range nestedFlows(a) { + out = append(out, allActivities(f.Activities)...) + } + } + return out +} + +func TestJumpTo_InBoundaryEventBody(t *testing.T) { + for name, src := range map[string]string{ + "user task": `create workflow Probe.WF + parameter $WorkflowContext: Probe.Ctx +begin + user task A 'A' page Probe.WF_TaskPage outcomes 'Done' { } + boundary event interrupting timer 'addDays([%CurrentDateTime%], 3)' { jump to A; }; +end workflow;`, + "wait for notification": `create workflow Probe.WF + parameter $WorkflowContext: Probe.Ctx +begin + wait for notification A + boundary event interrupting timer 'addDays([%CurrentDateTime%], 3)' { jump to A; }; +end workflow;`, + } { + t.Run(name, func(t *testing.T) { + var jump *workflows.JumpToActivity + seen := map[string]int{} + for _, a := range allActivities(buildWorkflowFrom(t, src)) { + if j, ok := a.(*workflows.JumpToActivity); ok { + jump = j + } + if n := a.GetName(); n != "" { + seen[n]++ + } + } + if jump == nil { + t.Fatal("no jump activity was built inside the boundary-event body") + } + if jump.TargetActivity != "A" { + t.Errorf("TargetActivity = %q, want A (CE6680)", jump.TargetActivity) + } + if jump.Name == jump.TargetActivity { + t.Errorf("jump is named after its target (%q) — it targets itself", jump.Name) + } + for n, c := range seen { + if c > 1 { + t.Errorf("Duplicate name %q (%d activities) — CE0495", n, c) + } + } + }) + } +} diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 8e4f59693..2b87a26bd 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -1285,7 +1285,8 @@ var execEnforcedMicroflowRules = map[string]bool{ // exprcheck's funcTable is now a write barrier, so a name missing from it // blocks valid MDL rather than merely warning about it: three genuine // built-ins (isNew/isSynced/isSyncing) were found missing and added — each - // built at 0 errors — before this line was added. + // built at 0 errors — before this line was added. validateNanoflowRules + // applies the same entry to nanoflow bodies (mendixlabs/mxcli#1033). "MDL044": true, // #884: an unknown annotation is silently dropped, so exec must refuse it too — // otherwise `check` catches the typo and the write that follows does not. diff --git a/mdl/executor/validate_listview_editable_inputs.go b/mdl/executor/validate_listview_editable_inputs.go new file mode 100644 index 000000000..09a16691b --- /dev/null +++ b/mdl/executor/validate_listview_editable_inputs.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// validateListViewEditableInputs (MDL-WIDGET31) reports a list view that will be +// written with Editable false while it holds input widgets that are meant to be +// editable. (ako/mxcli#631) +// +// Pages$ListView.Editable is what makes the inputs INSIDE a list view editable, +// and its read-only context wins over `editable: Always` on the input itself — +// including inside a nested data view. Without it every input renders as +//
, with a valid document, a clean `mx check` +// and a successful build: the failure only shows in the running app. +// +// The writer is right to default it to false: that is Mendix's own default +// (mendixmodelsdk 4.115.0, Pages$ListView `editable` defaults to false and +// _initializeDefaultProperties does not set it). Studio Pro agrees, measured on +// ako/TestApp (Mendix 11.14.0): 30 of its 32 list views are stored Editable false +// and none of those holds an input; the one list view with inputs +// (Pages.EditableLIstView, five textboxes at Editability Always) was set to +// Editable true by its author. So the fix is a diagnostic for the combination, +// not a different default. +// +// Editability is read with GetBoolProp, the same call buildListViewV3 uses, so +// the rule reports what will be written: a quoted `editable: 'true'` is a string +// and is written false. +// +// A warning, not an error: an input shown read-only in a list view is odd but +// legal. An input the author already marked `editable: Never` is not counted. +func validateListViewEditableInputs(w *ast.WidgetV3, locationPrefix string) []linter.Violation { + if w == nil || !strings.EqualFold(w.Type, "listview") || w.GetBoolProp("Editable") { + return nil + } + input := firstEditableInput(w.Children) + if input == nil { + return nil + } + return []linter.Violation{{ + RuleID: "MDL-WIDGET31", + Severity: linter.SeverityWarning, + Message: fmt.Sprintf( + "%s: list view `%s` is written with Editable false (the Mendix default), so input `%s` (%s) "+ + "inside it renders read-only — the list view's context wins over the input's own `editable:`", + locationPrefix, w.Name, input.Name, input.Type, + ), + Suggestion: fmt.Sprintf("Add `editable: true` to list view `%s`, or mark the inputs `editable: Never` "+ + "if they are meant to be read-only", w.Name), + }} +} + +// firstEditableInput finds an input widget below a list view that the author has +// not already made read-only. It does not descend into a nested list view, whose +// own Editable governs its inputs and which is reported on its own visit. +func firstEditableInput(widgets []*ast.WidgetV3) *ast.WidgetV3 { + for _, c := range widgets { + if c == nil { + continue + } + typ := strings.ToLower(c.Type) + if typ == "listview" { + continue + } + if editableWidgetTypes[typ] && typ != "dataview" && !strings.EqualFold(c.GetStringProp("Editable"), "Never") { + return c + } + if found := firstEditableInput(c.Children); found != nil { + return found + } + } + return nil +} diff --git a/mdl/executor/validate_listview_editable_inputs_test.go b/mdl/executor/validate_listview_editable_inputs_test.go new file mode 100644 index 000000000..22c7fbe26 --- /dev/null +++ b/mdl/executor/validate_listview_editable_inputs_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" +) + +// ako/mxcli#631. +// +// Reported with runtime evidence: "List views are written with `Editable: +// false` by mxcli, so every input inside a list view renders disabled — even +// with `editable: Always` on the text box, and even inside a nested data view." +// The BSON is valid, `mxcli check` was clean and the build succeeds; it only +// shows in the rendered app. +// +// false is Mendix's own default (mendixmodelsdk 4.115.0: Pages$ListView +// `editable` is a PrimitiveProperty defaulting to false, and +// _initializeDefaultProperties does not override it), so the writer is right to +// write it. Studio Pro agrees: in ako/TestApp (Mendix 11.14.0) 30 of 32 list +// views are stored false and hold no input, and the one with inputs was set to +// true by its author — the `control: editable true` case below is that page's +// shape. What was missing is a diagnostic for the combination that is never +// meant: inputs a list view will render read-only. +func TestMDLWIDGET31_ListViewInputsNotEditable(t *testing.T) { + cases := []struct { + name string + src string + want int + }{ + { + // The issue's reproduction, verbatim. + name: "issue repro: textbox editable Always in a default list view", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing) { + textbox t (label: 'N', attribute: Name, editable: Always) + } +}`, + want: 1, + }, + { + name: "input inside a nested data view", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing) { + dataview dv (datasource: $currentObject) { + checkbox cb (label: 'A', attribute: Active) + } + } +}`, + want: 1, + }, + { + name: "explicit editable false", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing, editable: false) { + textbox t (label: 'N', attribute: Name) + } +}`, + want: 1, + }, + { + // A quoted 'true' is a string, and buildListViewV3 reads the + // property with GetBoolProp — so it is written false, exactly as if + // it were absent. The rule reports what the writer does. + name: "quoted 'true' is written false", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing, editable: 'true') { + textbox t (label: 'N', attribute: Name) + } +}`, + want: 1, + }, + { + name: "control: editable true", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing, editable: true) { + textbox t (label: 'N', attribute: Name, editable: Always) + } +}`, + want: 0, + }, + { + name: "control: display-only content", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing) { + dynamictext dt (content: '{1}', contentparams: [{1} = Name]) + } +}`, + want: 0, + }, + { + name: "control: every input is explicitly Never", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database M.Thing) { + textbox t (label: 'N', attribute: Name, editable: Never) + } +}`, + want: 0, + }, + { + // A nested list view's inputs are governed by the NESTED list + // view's Editable, so the outer one is not the one to report. + name: "nested editable list view owns its own inputs", + src: `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + listview outer (datasource: database M.Thing) { + listview inner (datasource: database M.Thing, editable: true) { + textbox t (label: 'N', attribute: Name) + } + } +}`, + want: 0, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := widgetViolations(t, tc.src, "MDL-WIDGET31") + if len(got) != tc.want { + t.Fatalf("MDL-WIDGET31: got %d violation(s), want %d: %#v", len(got), tc.want, got) + } + if tc.want == 0 { + return + } + msg := got[0].Message + for _, s := range []string{"Editable", "read-only"} { + if !strings.Contains(msg, s) { + t.Errorf("message should mention %q: %s", s, msg) + } + } + if !strings.Contains(got[0].Suggestion, "editable: true") { + t.Errorf("suggestion should name the fix `editable: true`: %s", got[0].Suggestion) + } + }) + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 847e9628a..3c5c6880c 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -18,6 +18,7 @@ import ( func ValidateMicroflow(stmt *ast.CreateMicroflowStmt) []linter.Violation { v := µflowValidator{ mfName: stmt.Name.String(), + docType: "microflow", returnType: stmt.ReturnType, varKinds: map[string]exprcheck.TypeKind{}, } @@ -70,7 +71,10 @@ func (v *microflowValidator) checkQualifiedEntityRef(site string, entity ast.Qua // microflowValidator holds state for validating a single microflow. type microflowValidator struct { - mfName string + mfName string + // docType is the Location.DocumentType violations carry: "microflow", or + // "nanoflow" when ValidateNanoflow reuses the expression checks. + docType string returnType *ast.MicroflowReturnType // nil = void violations []linter.Violation loopDepth int // Track nesting depth inside loops @@ -92,7 +96,7 @@ func (v *microflowValidator) addViolation(ruleID string, severity linter.Severit Severity: severity, Message: message, Location: linter.Location{ - DocumentType: "microflow", + DocumentType: v.docType, DocumentName: v.mfName, }, Suggestion: suggestion, @@ -205,6 +209,7 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkUnknownAnnotations(s) v.checkErrorHandlingContinueSupported(s) v.checkErrorHandlingSupported(s) + v.checkStmtExprFunctions(s) switch stmt := s.(type) { case *ast.NotifyWorkflowStmt: // MDL-WF16. A notify reaches one named element of the workflow, and the @@ -231,12 +236,10 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { } case *ast.ReturnStmt: v.checkReturn(stmt) - v.checkExprFunctions("return", stmt.Value) v.checkQualifiedCallInExpression("return", stmt.Value) v.checkDivisionSlash("return", stmt.Value) v.checkDateTimeLiterals("return", stmt.Value) case *ast.IfStmt: - v.checkExprFunctions("if condition", stmt.Condition) v.checkDivisionSlash("if condition", stmt.Condition) v.checkDateTimeLiterals("if condition", stmt.Condition) v.walkBody(stmt.ThenBody) @@ -327,7 +330,6 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { } // #893 item 1: a Create Variable activity requires a value (CE0038). v.checkDeclareHasValue(stmt) - v.checkExprFunctions(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) v.checkQualifiedCallInExpression(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) v.checkDivisionSlash(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) v.checkDateTimeLiterals(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) @@ -339,7 +341,6 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkNumericAssignment("$"+stmt.Target, k, stmt.Value) } } - v.checkExprFunctions(fmt.Sprintf("set '%s'", stmt.Target), stmt.Value) v.checkQualifiedCallInExpression(fmt.Sprintf("set '%s'", stmt.Target), stmt.Value) v.checkDivisionSlash(fmt.Sprintf("set '%s'", stmt.Target), stmt.Value) v.checkDateTimeLiterals(fmt.Sprintf("set '%s'", stmt.Target), stmt.Value) @@ -421,16 +422,11 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkQualifiedEntityRef("create list of", stmt.EntityType) case *ast.CreateObjectStmt: v.checkQualifiedEntityRef("create", stmt.EntityType) - // Attribute values in a `create` are expressions too — an aggregate - // (sum/count/…) or an unknown function here fails the build with CE0117, - // but check previously only inspected return/if/declare/set (FINDINGS #17). for _, ch := range stmt.Changes { - v.checkExprFunctions(fmt.Sprintf("create %s attribute '%s'", stmt.EntityType.String(), ch.Attribute), ch.Value) v.checkQualifiedCallInExpression(fmt.Sprintf("create %s attribute '%s'", stmt.EntityType.String(), ch.Attribute), ch.Value) } case *ast.ChangeObjectStmt: for _, ch := range stmt.Changes { - v.checkExprFunctions(fmt.Sprintf("change '%s' attribute '%s'", stmt.Variable, ch.Attribute), ch.Value) v.checkQualifiedCallInExpression(fmt.Sprintf("change '%s' attribute '%s'", stmt.Variable, ch.Attribute), ch.Value) } } @@ -482,6 +478,42 @@ var mendixAggregateFuncs = map[string]bool{ "count": true, "sum": true, "average": true, "minimum": true, "maximum": true, } +// checkStmtExprFunctions runs MDL044 over every expression one statement +// carries. It is the single list of expression sites, shared by walkBody and by +// ValidateNanoflow — two lists were how nanoflows came to have none +// (mendixlabs/mxcli#1033). +func (v *microflowValidator) checkStmtExprFunctions(s ast.MicroflowStatement) { + switch stmt := s.(type) { + case *ast.ReturnStmt: + v.checkExprFunctions("return", stmt.Value) + case *ast.IfStmt: + v.checkExprFunctions("if condition", stmt.Condition) + case *ast.DeclareStmt: + v.checkExprFunctions(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) + case *ast.MfSetStmt: + v.checkExprFunctions(fmt.Sprintf("set '%s'", stmt.Target), stmt.Value) + case *ast.CreateObjectStmt: + // Attribute values in a `create` are expressions too — an aggregate + // (sum/count/…) or an unknown function here fails the build with CE0117, + // but check previously only inspected return/if/declare/set (FINDINGS #17). + for _, ch := range stmt.Changes { + v.checkExprFunctions(fmt.Sprintf("create %s attribute '%s'", stmt.EntityType.String(), ch.Attribute), ch.Value) + } + case *ast.ChangeObjectStmt: + for _, ch := range stmt.Changes { + v.checkExprFunctions(fmt.Sprintf("change '%s' attribute '%s'", stmt.Variable, ch.Attribute), ch.Value) + } + case *ast.LogStmt: + // The #1033 repro put the unknown call in a log message, which was never + // walked — in a microflow either. + v.checkExprFunctions("log node", stmt.Node) + v.checkExprFunctions("log message", stmt.Message) + for _, tp := range stmt.Template { + v.checkExprFunctions(fmt.Sprintf("log template {%d}", tp.Index), tp.Value) + } + } +} + // checkExprFunctions flags calls to names that are not Mendix expression // functions (e.g. a hallucinated randomInt()) — these parse and pass a naive // check but fail the build with CE0117. label describes where the expression diff --git a/mdl/executor/validate_microflow_expr_test.go b/mdl/executor/validate_microflow_expr_test.go index 68c235df5..84a35080c 100644 --- a/mdl/executor/validate_microflow_expr_test.go +++ b/mdl/executor/validate_microflow_expr_test.go @@ -63,6 +63,12 @@ func TestValidateMicroflow_UnknownFunction(t *testing.T) { // is how a write barrier stops catching anything. {"trunc is not a Mendix built-in", "declare $d Decimal = trunc($x);", true, ""}, {"currentDeviceType is not a Mendix built-in", "declare $b Boolean = currentDeviceType() = 'Phone';", true, ""}, + // mendixlabs/mxcli#1033: a log message is an expression too, and the + // reported repro put the unknown call there. Log statements were never + // walked for MDL044, in a microflow or a nanoflow. + {"unknown in log message", "log info 'device: ' + currentDeviceType();", true, ""}, + {"unknown in log template param", "log info 'device: {1}' with ({1} = currentDeviceType());", true, ""}, + {"known func in log message", "log info 'x: ' + toUpperCase($x);", false, ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/mdl/executor/validate_nanoflow.go b/mdl/executor/validate_nanoflow.go new file mode 100644 index 000000000..f2d65739f --- /dev/null +++ b/mdl/executor/validate_nanoflow.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/exprcheck" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ValidateNanoflow runs the MDL0xx rules that hold for a nanoflow body — today +// only MDL044, an expression calling a name that is not a Mendix function. +// +// MDL044 was wired to CREATE MICROFLOW alone (#828), so `currentDeviceType()` +// in a nanoflow passed check and exec and failed the build with CE0117 +// (mendixlabs/mxcli#1033). The microflow rule set is deliberately NOT run +// wholesale: several rules are microflow-specific and would be false positives +// here — MDL057 refuses `synchronize`, which only a nanoflow may contain. +func ValidateNanoflow(stmt *ast.CreateNanoflowStmt) []linter.Violation { + v := µflowValidator{ + mfName: stmt.Name.String(), + docType: "nanoflow", + returnType: stmt.ReturnType, + varKinds: map[string]exprcheck.TypeKind{}, + } + v.walkExprFunctions(stmt.Body) + return v.violations +} + +// walkExprFunctions applies checkStmtExprFunctions to every statement in the +// body, including branches, loops and error-handler bodies. +func (v *microflowValidator) walkExprFunctions(body []ast.MicroflowStatement) { + for _, s := range body { + v.checkStmtExprFunctions(s) + switch stmt := s.(type) { + case *ast.IfStmt: + v.walkExprFunctions(stmt.ThenBody) + v.walkExprFunctions(stmt.ElseBody) + case *ast.EnumSplitStmt: + for _, c := range stmt.Cases { + v.walkExprFunctions(c.Body) + } + v.walkExprFunctions(stmt.ElseBody) + case *ast.InheritanceSplitStmt: + for _, c := range stmt.Cases { + v.walkExprFunctions(c.Body) + } + v.walkExprFunctions(stmt.ElseBody) + case *ast.LoopStmt: + v.walkExprFunctions(stmt.Body) + case *ast.WhileStmt: + v.walkExprFunctions(stmt.Body) + } + if eh := stmtErrorHandling(s); eh != nil { + v.walkExprFunctions(eh.Body) + } + } +} + +// validateNanoflowRules is validateMicroflowRules for a nanoflow: exec refuses +// what ValidateNanoflow reports, restricted to the same verified allowlist, so +// check and exec cannot disagree. +func validateNanoflowRules(stmt *ast.CreateNanoflowStmt) error { + var msgs []string + for _, v := range ValidateNanoflow(stmt) { + if v.Severity != linter.SeverityError || !execEnforcedMicroflowRules[v.RuleID] { + continue + } + msg := fmt.Sprintf("[%s] %s", v.RuleID, v.Message) + if v.Suggestion != "" { + msg += "\n " + v.Suggestion + } + msgs = append(msgs, msg) + } + if len(msgs) == 0 { + return nil + } + return mdlerrors.NewValidationf("nanoflow '%s' has validation errors:\n - %s", + stmt.Name.String(), strings.Join(msgs, "\n - ")) +} diff --git a/mdl/executor/validate_nanoflow_test.go b/mdl/executor/validate_nanoflow_test.go new file mode 100644 index 000000000..56fca0c3f --- /dev/null +++ b/mdl/executor/validate_nanoflow_test.go @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func buildNF(t *testing.T, src string) *ast.CreateNanoflowStmt { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + return prog.Statements[0].(*ast.CreateNanoflowStmt) +} + +// TestValidateNanoflow_UnknownFunction guards mendixlabs/mxcli#1033: MDL044 +// (#828) ran only over CREATE MICROFLOW, so `currentDeviceType()` in a +// nanoflow passed `check` and `exec` and then failed the build with +// +// [error] [CE0117] "Error(s) in expression." at ... Test.NF_Dev ... +func TestValidateNanoflow_UnknownFunction(t *testing.T) { + cases := []struct { + name string + src string + wantMDL bool + }{ + { + // The reported repro, verbatim. + name: "issue repro: unknown call in a log message", + src: `create nanoflow Test.NF_Dev () +begin + log info 'device: ' + currentDeviceType(); +end;`, + wantMDL: true, + }, + { + name: "unknown call in a declare", + src: `create nanoflow Test.NF_Dev () returns Boolean +begin + declare $b Boolean = currentDeviceType() = 'Phone'; + return $b; +end;`, + wantMDL: true, + }, + { + name: "unknown call nested in an if branch", + src: `create nanoflow Test.NF_Dev ($x: String) +begin + if $x != empty then + set $x = trunc(1.5); + end if; +end;`, + wantMDL: true, + }, + { + // A token is not a function call, so MDL044 stays out of it. Measured + // on mxbuild 11.13.0: this nanoflow builds at 0 errors. (The report's + // suggested workaround, [%CurrentDeviceType%], does NOT — it is CE0117 + // in a nanoflow and a microflow alike, so it is not pinned here.) + name: "a real token is accepted", + src: `create nanoflow Test.NF_Dev () +begin + log info 'now: ' + toString([%CurrentDateTime%]); +end;`, + }, + { + name: "known functions are accepted", + src: `create nanoflow Test.NF_Dev ($x: String) returns String +begin + declare $s String = toUpperCase(trim($x)); + return $s; +end;`, + }, + { + // Only MDL044 runs over a nanoflow. MDL057 (`synchronize` is + // nanoflow-only) would be a false positive here, which is why the + // microflow rule set is not run wholesale. + name: "synchronize is not flagged in a nanoflow", + src: `create nanoflow Test.NF_Sync () +begin + synchronize all; +end;`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + nf := buildNF(t, tc.src) + vs := ValidateNanoflow(nf) + got := false + for _, v := range vs { + if v.RuleID == "MDL044" { + got = true + if v.Location.DocumentType != "nanoflow" { + t.Errorf("violation should locate a nanoflow, got %q", v.Location.DocumentType) + } + } else { + t.Errorf("only MDL044 runs over a nanoflow, got %s: %s", v.RuleID, v.Message) + } + } + if got != tc.wantMDL { + t.Fatalf("MDL044 fired=%v, want %v (violations %v)", got, tc.wantMDL, vs) + } + // The exec barrier must agree with check. + err := validateNanoflowRules(nf) + if tc.wantMDL { + if err == nil || !strings.Contains(err.Error(), "MDL044") { + t.Errorf("exec validation should refuse with MDL044, got: %v", err) + } + } else if err != nil { + t.Errorf("valid nanoflow rejected by exec validation: %v", err) + } + }) + } +} diff --git a/mdl/executor/validate_page_context.go b/mdl/executor/validate_page_context.go index 46ec9969c..6042dd923 100644 --- a/mdl/executor/validate_page_context.go +++ b/mdl/executor/validate_page_context.go @@ -61,13 +61,19 @@ var widgetKindsWithoutStoredNames = map[string]bool{ "column": true, } -// objectListContainerKinds returns the lowercased MDL container keywords the +// unstoredContainerKinds returns the lowercased MDL container keywords the // widget's definition declares as object lists (a BarChart's `series`, an -// Accordion's `group`). Children with one of those types are ITEMS, not widgets. +// Accordion's `group`) or as child slots (a Gallery's `template` and `filter`). +// Children with one of those types are ITEMS or SLOT BLOCKS, not widgets: an +// object-list item is a WidgetObject with no name, and applyChildSlots builds +// only a slot block's children into the slot property, discarding the block's +// own name. Either way the model holds no name for it — which is why DESCRIBE +// synthesises `series1` and `template1` — so it cannot be a CE0495 duplicate +// (upstream #978: four galleries described as `template template1 { … }`). // // Returns nil for anything that does not resolve — a built-in widget, an unknown // name, or no registry at all — so the caller's behaviour is unchanged there. -func objectListContainerKinds(registry *WidgetRegistry, w *ast.WidgetV3) map[string]bool { +func unstoredContainerKinds(registry *WidgetRegistry, w *ast.WidgetV3) map[string]bool { if registry == nil || w == nil { return nil } @@ -82,7 +88,20 @@ func objectListContainerKinds(registry *WidgetRegistry, w *ast.WidgetV3) map[str } } // WidgetMode carries no ObjectLists — object lists are declared once on the - // definition — so there is nothing mode-scoped to add here. + // definition — but it does carry ChildSlots. Every mode's slots count: which + // mode applies depends on properties the builder evaluates, and a slot + // keyword is a slot block in whichever mode declares it. + addSlots := func(slots []ChildSlotMapping) { + for _, s := range slots { + if s.MDLContainer != "" { + out[strings.ToLower(s.MDLContainer)] = true + } + } + } + addSlots(def.ChildSlots) + for i := range def.Modes { + addSlots(def.Modes[i].ChildSlots) + } return out } @@ -98,7 +117,9 @@ func objectListContainerKinds(registry *WidgetRegistry, w *ast.WidgetV3) map[str // `series series1 (…)`, because the stored WidgetObject carries no name and // DESCRIBE has to synthesise one. Measured on mxbuild 11.6.6, three charts on // one page each holding a `series s` is 0 errors; mxcli called it CE0495 and, -// since a reference error fails the run, refused to execute the script. +// since a reference error fails the run, refused to execute the script. And so +// are CHILD-SLOT BLOCKS — a gallery's `template` and `filter` — whose name +// applyChildSlots discards (see unstoredContainerKinds). // // registry may be nil (check runs with no project in CI). Then no parent // resolves, itemKinds is empty everywhere, and the rule behaves as it did @@ -115,13 +136,16 @@ func checkDuplicateWidgetNames(widgets []*ast.WidgetV3, registry *WidgetRegistry walk = func(ws []*ast.WidgetV3, itemKinds map[string]bool) { for _, w := range ws { kind := strings.ToLower(w.Type) - if w.Name != "" && !widgetKindsWithoutStoredNames[kind] && !itemKinds[kind] { + // `container { … }` is the other spelling of a slot block: + // applyChildSlots routes it by NAME, and stores no name for it either. + slotByName := kind == "container" && itemKinds[strings.ToLower(w.Name)] + if w.Name != "" && !widgetKindsWithoutStoredNames[kind] && !itemKinds[kind] && !slotByName { if counts[w.Name] == 0 { order = append(order, w.Name) } counts[w.Name]++ } - walk(w.Children, objectListContainerKinds(registry, w)) + walk(w.Children, unstoredContainerKinds(registry, w)) } } walk(widgets, nil) diff --git a/mdl/executor/validate_page_slot_containers_test.go b/mdl/executor/validate_page_slot_containers_test.go new file mode 100644 index 000000000..9c152977e --- /dev/null +++ b/mdl/executor/validate_page_slot_containers_test.go @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// upstream #978, the third symptom. The row/column half was fixed by +// widgetKindsWithoutStoredNames; the report also listed +// +// - duplicate widget name 'template1' (used 4 times) — Mendix requires unique widget names per page (CE0495) +// +// DESCRIBE writes every gallery's content as `template template1 { … }` and its +// filters as `filter filter1 { … }`, so a page with more than one gallery +// described into MDL that mxcli's own check rejected. +// +// Those blocks are CHILD SLOTS of the gallery's definition (gallery.def.json: +// TEMPLATE → `content`, FILTER → `filtersPlaceholder`). applyChildSlots builds +// only the block's children into the slot property; the block's own name is +// discarded, so the model holds nothing that could collide under CE0495. That is +// also why DESCRIBE has to synthesise `template1` — there is no stored name to +// read back. + +func galleryRegistry(t *testing.T) *WidgetRegistry { + t.Helper() + reg, err := NewWidgetRegistry() + if err != nil { + t.Fatalf("load embedded widget registry: %v", err) + } + if _, ok := reg.Get("GALLERY"); !ok { + t.Fatal("embedded registry has no gallery definition") + } + return reg +} + +// describedGallery is the shape DESCRIBE emits for a gallery with filters and +// content (cmd_pages_describe_output.go, the `widgetType == "gallery"` branch). +func describedGallery(name, text string) *ast.WidgetV3 { + return &ast.WidgetV3{Type: "gallery", Name: name, Children: []*ast.WidgetV3{ + {Type: "filter", Name: "filter1", Children: []*ast.WidgetV3{ + {Type: "textfilter", Name: "tf_" + name}, + }}, + {Type: "template", Name: "template1", Children: []*ast.WidgetV3{ + {Type: "dynamictext", Name: text}, + }}, + }} +} + +// The reported case: four galleries, as DESCRIBE renders them. +func TestCheckDuplicateWidgetNames_GallerySlotContainersAreNotWidgets(t *testing.T) { + page := []*ast.WidgetV3{ + describedGallery("g1", "t1"), + describedGallery("g2", "t2"), + describedGallery("g3", "t3"), + describedGallery("g4", "t4"), + } + if errs := checkDuplicateWidgetNames(page, galleryRegistry(t)); len(errs) != 0 { + t.Errorf("a child-slot container's name is not stored, so it cannot be a CE0495 duplicate; got:\n %s", + strings.Join(errs, "\n ")) + } +} + +// The control: the widgets INSIDE a slot are real, stored widgets. Two of them +// sharing a name across galleries is CE0495 and must still be reported — a fix +// that skipped the slot's whole subtree would pass the test above. +func TestCheckDuplicateWidgetNames_WidgetsInsideSlotContainersStillCount(t *testing.T) { + page := []*ast.WidgetV3{ + describedGallery("g1", "dup"), + describedGallery("g2", "dup"), + } + errs := checkDuplicateWidgetNames(page, galleryRegistry(t)) + if len(errs) != 1 || !strings.Contains(errs[0], "'dup'") { + t.Errorf("a duplicate widget inside a gallery template was not reported: %v", errs) + } +} + +// The second control: `template` outside a widget that declares it as a slot is +// buildTemplateV3 — a Forms$DivContainer that DOES store its name. Two of those +// sharing a name are a real duplicate. +func TestCheckDuplicateWidgetNames_StandaloneTemplateStillCounts(t *testing.T) { + page := []*ast.WidgetV3{ + {Type: "template", Name: "template1"}, + {Type: "container", Name: "c", Children: []*ast.WidgetV3{ + {Type: "template", Name: "template1"}, + }}, + } + errs := checkDuplicateWidgetNames(page, galleryRegistry(t)) + if len(errs) != 1 || !strings.Contains(errs[0], "'template1'") { + t.Errorf("a standalone template stores its name; a duplicate must be reported: %v", errs) + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index d6f36ac4f..d0ca4fba7 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -84,6 +84,8 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // Parameter annotations for the two flow flavours that do not go through // ValidateMicroflow but share the parameter grammar. if nfStmt, ok := stmt.(*ast.CreateNanoflowStmt); ok { + // MDL044 over the body (mendixlabs/mxcli#1033). + violations = append(violations, ValidateNanoflow(nfStmt)...) violations = append(violations, ValidateFlowParameterAnnotations("nanoflow '"+nfStmt.Name.String()+"'", nfStmt.Parameters)...) } diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 961ce40a6..93bb98cc8 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -187,6 +187,8 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc out = append(out, validateDynamicTextFormatting(w, locationPrefix)...) out = append(out, validateDatasourceXPathAssociationEmpty(w, locationPrefix)...) out = append(out, validateComboBoxAssociation(w, locationPrefix)...) + // #631: inputs inside a list view that will be written read-only. + out = append(out, validateListViewEditableInputs(w, locationPrefix)...) // A show_page argument naming anything but the context object is dropped. // The widget's OWN action is judged in the context IT establishes, not the // one it sits in — a list widget's onClick is row-scoped (ako/mxcli#552). @@ -1091,17 +1093,18 @@ var templatePlaceholderRe = regexp.MustCompile(`\{(\d+)\}`) // sources mirror buildDynamicTextV3: explicit ContentParams, a single Attribute // binding, or a whole-content reference (which carries no {N}, so is irrelevant // here). +// +// An action/link button's Caption is the same ClientTemplate and orphans the +// same way (CE0720). Its parameters mirror buildButtonV3: CaptionParams, or the +// ContentParams spelling DESCRIBE emitted for buttons before #632. func validateDynamicTextPlaceholders(w *ast.WidgetV3, locationPrefix string) *linter.Violation { + if isButtonKeyword(w.Type) { + return validateButtonCaptionPlaceholders(w, locationPrefix) + } if !strings.EqualFold(w.Type, "dynamictext") { return nil } - content := w.GetContent() - maxIdx := 0 - for _, m := range templatePlaceholderRe.FindAllStringSubmatch(content, -1) { - if n, err := strconv.Atoi(m[1]); err == nil && n > maxIdx { - maxIdx = n - } - } + maxIdx := maxTemplatePlaceholder(w.GetContent()) if maxIdx == 0 { return nil // no placeholders → nothing to orphan } @@ -1124,6 +1127,42 @@ func validateDynamicTextPlaceholders(w *ast.WidgetV3, locationPrefix string) *li } } +func isButtonKeyword(t string) bool { + return strings.EqualFold(t, "actionbutton") || strings.EqualFold(t, "linkbutton") +} + +func validateButtonCaptionPlaceholders(w *ast.WidgetV3, locationPrefix string) *linter.Violation { + maxIdx := maxTemplatePlaceholder(w.GetCaption()) + if maxIdx == 0 { + return nil + } + params := len(w.GetCaptionParams()) + if params == 0 { + params = len(w.GetContentParams()) + } + if maxIdx <= params { + return nil + } + return &linter.Violation{ + RuleID: "MDL-WIDGET04", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` (%s) caption references template placeholder {%d} but only %d parameter(s) are bound — bind it with `CaptionParams: [{%d} = ]`. An orphaned placeholder fails the build (CE0720).", + locationPrefix, w.Name, strings.ToLower(w.Type), maxIdx, params, maxIdx, + ), + } +} + +func maxTemplatePlaceholder(template string) int { + maxIdx := 0 + for _, m := range templatePlaceholderRe.FindAllStringSubmatch(template, -1) { + if n, err := strconv.Atoi(m[1]); err == nil && n > maxIdx { + maxIdx = n + } + } + return maxIdx +} + // validatePluggableWidgetProperties checks every AST property key on a // pluggable widget against the widget's def.json. Non-pluggable widgets are // skipped (those go through the static builder which already validates props). diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 28c7e2ddd..69048e9a7 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -314,6 +314,18 @@ alterPageAssignment | ACTION EQUALS actionExprV3 // Action = MICROFLOW Module.MF | SHOW_PAGE Module.Page | SAVE_CHANGES CLOSE_PAGE | VISIBLE EQUALS xpathConstraint // Visible = [Name != ''] (conditional visibility) | EDITABLE EQUALS xpathConstraint // Editable = [Status = 'Open'] (conditional editability) + // A pluggable widget's NAMED action slot, addressed by the widget's own key: + // `set 'createFileAction' = microflow M.F on fileUploader1`. The ALTER-level + // twin of widgetPropertyV3's `key: actionExprV3` (#956); without it the value + // fell to propertyValueV3, which has no `microflow ` form, and the only + // way to retarget one slot was to REPLACE the whole widget + // (mendixlabs/mxcli#995). Placed before the scalar alternatives, as on CREATE, + // so a bare action keyword (`close_page`) is an action; unlike CREATE there is + // no datasource overlap to yield to, since DataSource is its own alternative. + // Whether the key IS an action slot is the stored widget's call, not the + // grammar's — the mutator refuses one that is not. + | STRING_LITERAL EQUALS actionExprV3 // 'createFileAction' = MICROFLOW Module.MF + | identifierOrKeyword EQUALS actionExprV3 // createFileAction = MICROFLOW Module.MF | identifierOrKeyword EQUALS propertyValueV3 // Caption = 'Save' | STRING_LITERAL EQUALS propertyValueV3 // 'showLabel' = false ; diff --git a/mdl/visitor/visitor_alter_page.go b/mdl/visitor/visitor_alter_page.go index 604525da2..a45aecc06 100644 --- a/mdl/visitor/visitor_alter_page.go +++ b/mdl/visitor/visitor_alter_page.go @@ -135,6 +135,15 @@ func (b *Builder) buildAlterPageAssignment(ctx *parser.AlterPageAssignmentContex // previously only possible by REPLACEing the whole widget, which silently // drops any property the author did not restate. if acCtx := ctx.ActionExprV3(); acCtx != nil { + // A named pluggable action slot — `set 'createFileAction' = microflow + // M.F` — keeps the author's key; the executor routes any action value + // whose key is not `Action` to the slot of that name (#995). + if id := ctx.IdentifierOrKeyword(); id != nil { + return identifierOrKeywordText(id), buildActionV3(acCtx) + } + if sl := ctx.STRING_LITERAL(); sl != nil { + return unquoteString(sl.GetText()), buildActionV3(acCtx) + } return "Action", buildActionV3(acCtx) } diff --git a/mdl/visitor/visitor_alter_page_named_action_test.go b/mdl/visitor/visitor_alter_page_named_action_test.go new file mode 100644 index 000000000..08daf72f5 --- /dev/null +++ b/mdl/visitor/visitor_alter_page_named_action_test.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// TestAlterPage_SetNamedActionSlot_Parses is the grammar half of +// mendixlabs/mxcli#995. +// +// #956 gave a pluggable widget's named action slots a spelling on CREATE PAGE +// (`createFileAction: microflow M.F`), but `alterPageAssignment` fell through to +// propertyValueV3 for any name other than `Action`, and that rule has no +// `microflow ` form. The reported symptom, verbatim: +// +// line 2:37 extraneous input 'MyModule' expecting {DROP, ADD, SET, INSERT, REPLACE, '}'} +// +// so retargeting one slot meant REPLACEing the whole widget and restating every +// other property. Both the quoted (pluggable-property convention) and bare +// spellings from the report must reach the executor as an action, not a scalar. +func TestAlterPage_SetNamedActionSlot_Parses(t *testing.T) { + tests := []struct { + name string + src string + key string + wantType string + target string + }{ + {"quoted microflow", + "alter page MyModule.UploadPage {\n set 'createFileAction' = microflow MyModule.ACT_CreateFile on fileUploader1;\n};", + "createFileAction", "microflow", "MyModule.ACT_CreateFile"}, + {"bare microflow", + "alter page MyModule.UploadPage {\n set createFileAction = microflow MyModule.ACT_CreateFile on fileUploader1;\n};", + "createFileAction", "microflow", "MyModule.ACT_CreateFile"}, + {"quoted nanoflow", + "alter page M.P { set 'onUploadSuccessFile' = nanoflow M.NF_Done on fileUploader1; };", + "onUploadSuccessFile", "nanoflow", "M.NF_Done"}, + {"quoted show_page", + "alter page M.P { set 'onSelectionChange' = show_page M.Detail on dg; };", + "onSelectionChange", "showPage", "M.Detail"}, + {"inside a parenthesised list", + "alter page M.P { set ('createFileAction' = microflow M.A, 'createImageAction' = microflow M.B) on fileUploader1; };", + "createImageAction", "microflow", "M.B"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prog, errs := Build(tt.src) + if len(errs) > 0 { + t.Fatalf("Build: %v", errs) + } + op, ok := prog.Statements[0].(*ast.AlterPageStmt).Operations[0].(*ast.SetPropertyOp) + if !ok { + t.Fatalf("op type = %T, want *ast.SetPropertyOp", prog.Statements[0].(*ast.AlterPageStmt).Operations[0]) + } + raw, present := op.Properties[tt.key] + if !present { + t.Fatalf("no %q property; got %v", tt.key, op.Properties) + } + action, ok := raw.(*ast.ActionV3) + if !ok { + t.Fatalf("%s value type = %T, want *ast.ActionV3", tt.key, raw) + } + if action.Type != tt.wantType || action.Target != tt.target { + t.Errorf("action = %s %s, want %s %s", action.Type, action.Target, tt.wantType, tt.target) + } + }) + } +} + +// TestAlterPage_SetNamedActionSlot_ScalarsStayScalars is the control: adding an +// action alternative must not capture the scalar values the generic assignment +// already carried. +func TestAlterPage_SetNamedActionSlot_ScalarsStayScalars(t *testing.T) { + tests := []struct { + src string + key string + want any + }{ + {"alter page M.P { set 'showLabel' = false on w; };", "showLabel", false}, + {"alter page M.P { set 'pageSize' = 10 on w; };", "pageSize", 10}, + {"alter page M.P { set Caption = 'Save' on w; };", "Caption", "Save"}, + {"alter page M.P { set 'mode' = 'files' on w; };", "mode", "files"}, + } + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + prog, errs := Build(tt.src) + if len(errs) > 0 { + t.Fatalf("Build: %v", errs) + } + op := prog.Statements[0].(*ast.AlterPageStmt).Operations[0].(*ast.SetPropertyOp) + if got := op.Properties[tt.key]; got != tt.want { + t.Errorf("%s = %#v (%T), want %#v", tt.key, got, got, tt.want) + } + }) + } +}