diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 9f453c80d2..b26492ffbb 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -109,3 +109,5 @@ {"area": "cmd/mxcli", "date": "2026-09-13", "symptom": "`build-and-test` fails in CI on `TestSettleSourceReturnsPromptlyForOneChange` \u2014 \"a quiet source took 196.975373ms to settle, want under 100ms\" \u2014 while the SAME tree passes in another run of the same workflow minutes earlier", "cause": "The test bounded elapsed wall-clock time as a multiple of the poll interval (`poll * (sourceSettleWindow + 3)`, 100ms against a nominal 40ms). settleSource waits on `time.After(poll)`, which guarantees AT LEAST the duration and nothing about the upper bound, so a loaded runner blows the budget with no defect present.", "file": "cmd/mxcli/docker/runlocal.go (settleSourceWith, the injected tick), cmd/mxcli/docker/runlocal_settle_test.go", "insight": "The property being guarded was a POLL COUNT, not a duration \u2014 'a quiet source costs one extra poll' \u2014 so the fix is to make polls countable (inject the timer) rather than to widen the budget, which only moves the flake threshold. Diagnosis shortcut worth reusing: the same workflow ran twice on the same tree, once from the push event and once from the pull_request merge commit, and disagreed \u2014 two runs of one tree is direct evidence of nondeterminism and cheaper than reading the test. Two things the controls settled that reasoning did not: (1) the assertions are written in terms of `sourceSettleWindow`, so WIDENING that constant leaves both tests green \u2014 they assert the loop honours whatever window is declared, never the number itself, and the real control is a loop that costs one poll MORE than it declares (both fail). (2) Each tick call must return a freshly-armed channel; returning one shared channel makes the multi-file test HANG rather than miscount, so the re-arm is load-bearing and not a style choice. The seam also made a previously untestable guarantee expressible: the window must be sourceSettleWindow CONSECUTIVE quiet polls, and dropping `quiet = 0` from the change branch was green against every pre-existing test in the file.", "refs": ["ako/mxcli#449"]} {"area":"cmd/mxcli","date":"2026-09-15","symptom":"Porting cmd/mxcli/docker off sdk/mpr moved two WRITE paths (ensureDemoUsers, applyHarvest) onto the codec backend. A baseline diff of `docker check` showed the project byte-identical across 421 files — which proved nothing, because the run had not written anything.","cause":"docker check's widget-update harvest is a no-op on an already-clean fixture, so an output+filetree diff against a pre-port binary exercises only the READ paths. Coverage then showed ensureDemoUsers at 0.0% — a write path the port touched that no test in the package ran.","file":"cmd/mxcli/docker/build.go","fix":"Added TestEnsureDemoUsers_CreatesAdminWhenNoneExist and _SkipsWhenUsersExist, plus a clearDemoUsers helper that establishes the precondition. Coverage 0.0% -> 76.5%. The read paths keep the baseline-diff evidence; applyHarvest was already at 76.9% via TestRunUpdateWidgets_RestoresV2AfterConversion.","insight":"A byte-identical baseline diff is strong evidence for a READ port and near-worthless for a WRITE port, because the natural control (nothing changed) is also what a no-op produces. The two need different instruments, and the cheap way to tell which you have is `go test -coverprofile` + `go tool cover -func` grepped for the functions you touched: it answers 'did my port's code even run' in one command, where a passing suite does not. Here it separated applyHarvest (76.9%, genuinely exercised including its UpdateRawUnit) from ensureDemoUsers (0.0%) inside the same package, so the gap was specific rather than a general absence of tests. Second trap, hit while fixing it: the shared v2 fixture ALREADY HAS two demo users, so the create-path test skipped and the idempotence test asserted the wrong count. Skipping on an unmet precondition is the #808 shape — set the precondition up instead (RemoveDemoUser in a helper, then assert the helper actually emptied it before proceeding). Third: read back through a FRESH connection, since asserting on the value the writer still holds passes against a write that never reached disk."} {"area":"cmd/mxcli","date":"2026-09-15","symptom":"Porting the last cmd/mxcli readers off sdk/mpr, cmd_extract_templates.go compiled with a type error (RawType/RawObject are bson.D on sdk/mpr, any on types.RawCustomWidgetType). Casting past it would have compiled — and broken the command at runtime, because FindCustomWidgetType is UNIMPLEMENTED on the codec backend.","cause":"mdl/backend/modelsdk/unimplemented_gen.go carries FindCustomWidgetType; measured at runtime it returns 'FindCustomWidgetType is not implemented on the model engine. This should be unreachable'. cmd_extract_templates.go was calling it through a concrete *mpr.Reader, so it was reachable only by NOT going through the backend.","file":"cmd/mxcli/cmd_extract_templates.go","fix":"Left this one file on sdk/mpr with a comment saying why and what would fix it (implement FindCustomWidgetType on the codec backend), and ported the other five. cmd/mxcli is otherwise clean; importers 13 -> 8.","insight":"The type error was the lucky part. A compile error is the ONLY reason this did not ship as a runtime failure — the cast that silences it is one line, and nothing else would have objected. When a port hits a type mismatch at a backend boundary, check whether the backend method is implemented at all before reconciling the types: `grep -n '' mdl/backend/modelsdk/unimplemented_gen.go` answers it in one command, and a runtime probe (connect read-only, call it, log the error) confirms it in under a minute. Note the direction of the trap: the unimplemented method's own error says 'This should be unreachable', and porting a caller to the backend is precisely what MAKES it reachable — so the #477 census blind spot (callers holding a concrete reader are invisible) cuts both ways. Second, smaller measurement trap in the same slice: a baseline diff of `check --post-migration` showed 50 lines vanishing, which looked like a regression and was not — the FIRST run built and cached a catalog inside the project, so the second run reused it. Two binaries must each get their own fresh copy of the fixture, exactly as for a write port; a command that caches into the project directory makes consecutive runs non-independent even when nothing is being written on purpose."} +{"area": "cmd/mxcli", "date": "2026-09-16", "symptom": "mendixlabs/mxcli#1103: a RETRIEVE with LIMIT inside a .test.mdl block was reported as `mismatched input 'LIMIT' expecting {GROUP_BY, SELECT, HAVING}` — the OQL follow set — on the statement `mxcli syntax microflow.retrieve` prints as its own example. The reporter concluded the test-block path routes microflow statements into the OQL parser.", "cause": "It does not. The generated MxTest.Test_* microflow parses fine (measured end-to-end against a real 11.6.6 project: the flow was created with the LIMIT intact). The message came from `mxcli check`/the LSP being pointed at the .test.mdl file itself, which they parsed as top-level MDL. A test block is a MICROFLOW BODY: DECLARE is not a top-level statement, the parser resyncs, RETRIEVE is a NON-RESERVED keyword so it is swallowed as an identifier, and the leftover `FROM …` starts oqlQueryTerm's FROM-first alternative (mdl/grammar/domains/MDLCatalog.g4), whose follow set is exactly {GROUP_BY, SELECT, HAVING}.", "file": "cmd/mxcli/testrunner/check_source.go", "fix": "testrunner.CheckSource renders each block as the microflow it becomes, padded so every body keeps its SOURCE line numbers (wrapper fragments go on the lines the doc comment and the '/' separator occupied). cmd_check.go and lsp_diagnostics.go translate before parsing, so all downstream rules apply unchanged and no diagnostic needs remapping. .test.mdl files joined `make check-mdl`; `.fail.test.mdl` names one whose annotations are deliberately unusable.", "insight": "Two lessons. First: the reporter's diagnosis was precise, confident and wrong, and the fastest way to find that out was to run the pipeline rather than read it — dumping GenerateTestFlows' output and feeding it to visitor.Build took one throwaway test and settled in seconds what an hour of grepping had not. Their error message was real; the command that produced it was not the one they named. Second, the general shape: a tool that OWNS a file format must not hand that format to a parser for a different one. The VS Code extension binds MDL to `.mdl`, which `.test.mdl` matches, so every test file in the editor was a wall of squiggles — 9 of this repo's 10 test files reported errors, one of them 392, and nobody had noticed because nobody runs `mxcli check` on a test file. When adding a derived file format, check what the EXISTING tooling makes of it; the answer is rarely 'nothing'. Line-preserving padding is what makes the translation honest: render into a slice of the source's own length and place wrapper fragments only on lines the original spent on comments or separators, and a diagnostic's line:col is the author's without a mapping table to drift."} +{"area": "cmd/mxcli", "date": "2026-09-16", "symptom": "mendixlabs/mxcli#1104: `mxcli test --attach` reported only 'build failed: The project cannot be deployed, because it contains errors.' on an injection failure, and afterwards EVERY later run of ANY test file failed the same way until a leftover document was found by hand.", "cause": "Two independent defects. (1) The parsed problems were in hand and discarded: runner_attach.go and LocalApp.Rebuild both built their error with `fmt.Errorf(\"build failed: %s\", build.Message)`, and Message is identical for every failing build. Attribution (build_attribution.go, BuildResult.ErrorSummary) existed but was wired only into the --local BOOT, so --attach and every --watch rebuild lost it. (2) Generated names are positional — MxTest.Test_test_1, _2, … from the test's index in its file — and every test file reuses them, while cleanup dropped only the CURRENT suite's names. A run with fewer tests than the last one therefore left the surplus behind, and under --attach the MxTest module always pre-exists (the dev loop installed it) so the whole-module drop never fires.", "file": "cmd/mxcli/testrunner/cleanup_leftovers.go", "fix": "buildFailure()/resultsForBuildFailure() shared by both runners; cleanup keys on what the project HOLDS (SHOW MICROFLOWS IN MxTest, filtered by the generated prefix) with the suite only as a fallback; reportCleanup names every surviving document and prints its DROP.", "insight": "Measured, not reasoned: planted one bad MxTest.Test_test_2 in a project, ran a known-good one-test suite, and watched it fail and leave the leftover in place — then after the fix watched run 1 fail and CLEAN, and run 2 pass. A self-healing sequence is the control that 'cleanup works' cannot be argued into. The general rule for generated artefacts: derive what to remove from what EXISTS, never from what you intended to create. Keying on the suite was wrong in both directions at once — it missed leftovers AND issued DROPs for flows a part-way injection never created, and those failures made cleanup report 'the project has been left modified' for a project it had just cleaned, which is a false alarm that sends the reader hunting for damage. Positional names (index-in-file) guarantee collisions across files and are worth avoiding, but as long as they exist the prefix is the only safe key. Also worth pinning: once BuildFailedError.Error() renders the errors, a hint that repeats them prints everything twice — assert on the message the READER sees, not on the hint in isolation."} diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index 911a51a7e7..3135561123 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -111,4 +111,5 @@ {"area": "mdl/backend", "date": "2026-09-15", "symptom": "`create or modify workflow` over `--mcp` (Studio Pro 11.14) stored the flow's activities reversed, with an old activity left in and a new one missing ([Start, a, b, End] rewritten as A, B, C → Start, B, A, a, End; a larger flow also lost its parallel split and held a name twice); `alter workflow … replace activity X` left X in place. The update calls all reported SUCCESS", "cause": "One `ped_update_document` batch is not applied in the order sent. Every measured batch fits: ops run highest index first, and at one index the adds go in as a block in op order before the removes — so a remove at an index where something was just added removes the added element. `UpdateWorkflow` sent the flow's removes plus its middles in reverse at index 1 in one batch (and index-less adds for event sub-processes/handlers, which come out reversed); `ReplaceActivity` sent remove @k plus adds at k, k+1; `InsertAfterActivity` sent incrementing indices", "file": "`mdl/backend/mcp/workflow.go` (`UpdateWorkflow`, `InsertAfterActivity`, `ReplaceActivity`, `addAtOp`); simulator `pedListSim` in `mdl/backend/mcp/workflow_listops_test.go`", "insight": "**The code comment claimed the reverse-at-index-1 trick worked, and no fake PED modelled batch semantics, so the unit tests asserted the ops sent rather than the list stored.** Fix tests by simulating the server's list semantics and asserting the resulting ORDER, and keep a table test that replays each raw-PED measurement through the simulator — that is what makes the simulator trustworthy and each control meaningful. The first guess at the rule (\"adds first, then removes\") fit two measurements and failed the third; fit the model to every data point before building on it. Also: a live update-path probe needs a workflow the executor can see — either on disk, or created earlier in the same exec (the backend's session list)", "fix": "Never add to and remove from the same list in one batch: add the statement's elements at a single index in their own order (flow middles @1, event sub-processes and handlers @0, replacement activities @k+1), then remove the stored/replaced ones in a second update. Adding first leaves duplicates, not a gutted workflow, if the second update fails"} {"area":"mdl/backend","date":"2026-09-15","symptom":"Wiring FindCustomWidgetType from modelsdk/mpr.Reader onto the codec Backend by straight delegation made `mxcli extract-templates` extract 0 of 6 templates, reporting for each widget: '[SKIP] Combo box: widget type is bson.D, want bson.D'. The type assertion in the caller names the same type on both sides of 'want'.","cause":"modelsdk/mpr builds RawType/RawObject with the v2 BSON driver (go.mongodb.org/mongo-driver/v2/bson) while sdk/mpr and every caller use v1 (go.mongodb.org/mongo-driver/bson). They are unrelated Go types that both print as 'bson.D', so the mismatch is invisible in the error text. types.RawCustomWidgetType declares the fields as `any` to avoid a BSON dependency, which removes the compiler's ability to catch it too.","file":"mdl/backend/modelsdk/widget_custom_find.go","fix":"Convert at the backend boundary with the package's existing v2ToV1BSON helper, so RawType/RawObject always hold v1 bson.D — the currency sdk/mpr established and callers assert. Verified by extracting all 6 templates byte-for-byte identically to the pre-change binary (1.2MB datagrid.json included); reverting the conversion fails the new test with 'RawType is bson.D, want v1 bson.D'.","insight":"An `any` field crossing an engine boundary can carry the RIGHT type name and the WRONG package, and the error message will look like a tautology. When a type assertion fails with identical type names on both sides, the question is which import path each came from, not what the type is — the two BSON drivers coexist in this repo on purpose (modelsdk is v2, sdk/mpr and the CLI are v1) and widget_pluggable_write.go's v2ToV1BSON already existed for the write direction. A cast written to silence that compile/assert error panics at runtime instead. Two process notes from the same change. (1) GREP FOR AN EXISTING IMPLEMENTATION BEFORE WRITING ONE: the walker had been in modelsdk/mpr all along (FindAllCustomWidgetTypes + collectCustomWidgets, and it populates UnitName/WidgetName which a fresh implementation would omit); only the backend wiring was missing, which is exactly what 'this should be unreachable' in the unimplemented error meant. (2) unimplemented_gen.go still emits the stub after a method is implemented — the generator writes a complete fallback set and Backend's own method shadows it — so the thing to update is the unreachableUnimplemented map in unimplemented_reachability_test.go, which fails loudly if a listed method becomes implemented."} {"area":"mdl/backend","date":"2026-09-15","symptom":"Phase 4a took sdk/mpr from 27 importers to 0, but nothing stopped the count from creeping back — there was no build or test guard, only the plan document and a habit.","cause":"The invariant lived in prose. A single new `import \"github.com/mendixlabs/mxcli/sdk/mpr\"` compiles, passes every test, and reintroduces exactly the blind spot Phase 4a existed to close: the unimplemented-method census in mdl/backend/modelsdk lists methods with NO implementation, so a caller reaching one through a concrete *sdk/mpr.Reader never appears in it. That is what hid project_tree.go's 36 semantic reads (#477) and cmd_extract_templates.go's FindCustomWidgetType (#484) until each was found by hand.","file":"mdl/backend/sdkmpr_import_guard_test.go","fix":"TestNothingImportsTheLegacyEngine parses every .go file's imports (go/parser, ImportsOnly) and fails naming any file that imports sdk/mpr, with the remedy in the message. Controlled by dropping a one-line file importing sdk/mpr into examples/ — it fails and names the file.","insight":"A zero-count invariant needs TWO positive controls or it passes vacuously forever, and the failure mode is silent by construction: a walk rooted at the wrong directory, a skipped-dir rule that is too broad, or an import-parsing mistake all report '0 importers' and read as success. So assert (1) a plausible number of files was actually scanned (here >500; it sees 2551) and (2) the detector can see imports AT ALL, by counting a package the repo definitely does import (mdl/backend, 120 files). Only then does 0 mean zero. This is scripts/check-tunnel-deps.sh's pattern — it asserts chisel IS in the linux graph before asserting it is absent from windows/darwin — and the same reasoning as a bug-fix control: a test that only ever passes has not been shown to detect anything. Practical note: skip sdk/mpr's own directory by comparing the path to the repo root rather than by basename, or a directory named mpr elsewhere is skipped too."} +{"area": "mdl/backend/modelsdk", "date": "2026-09-16", "symptom": "`describe enumeration System.WorkflowActivityType` -> \"enumeration not found\" while `describe entity System.WorkflowActivityRecord` prints `ActivityType: Enumeration(System.WorkflowActivityType)` in the same session. `show enumerations` omits every System enum; `check --references` REJECTS a valid attribute typed against one (a false positive that blocks correct scripts); `CATALOG.attributes LEFT JOIN CATALOG.enumerations` resolves 0 of 19 on a blank app. Values could only be guessed at until the build rejected one with CE1613", "cause": "`modelsdk/meta.SystemEnumerations` — all 15 System enumerations with their values — had been in the tree since #889 with ZERO non-test consumers. The System module is not stored in the .mpr at all (measured: the string `WorkflowActivityType` occurs in 0 of 370 mprcontents units and 0 bytes of the .mpr sqlite), so its entities/associations/Java actions are each synthesized by a `Build*` helper and appended to a listing. The enumeration half had the data table and neither the helper nor the wiring, so the entity attribute printed a type naming a document nothing could produce", "file": "`modelsdk/meta/system_enumerations.go` (new, `BuildSystemEnumerations`); wired in `mdl/backend/modelsdk/enumeration.go` (`ListEnumerations`, `GetEnumeration`)", "insight": "**A data table with no consumer looks exactly like a missing feature.** Before opening any file, `grep -rn --include=*.go | grep -v _test` — zero hits is the whole diagnosis, and it took one command. The virtual System module has one wiring point PER LISTING, so the question for any new System doctype is \"which listings must it appear in\", not \"is the data there\". **One append fixed four of the five reported symptoms at once** (describe, show, check --references, catalog) because the catalog builder takes `ctx.Backend` as its reader — so `ListEnumerations` is the single choke point. Two traps. (1) `search` was NOT fixed by it: the strings index is built from value CAPTIONS (`builder_modules.go`), and the meta table carries names only, so System enums produce 0 rows in `CATALOG.strings` while user enums produce 36. Defaulting a caption to the value name would be inventing text indistinguishable from a developer's own — left unfixed and filed instead. (2) A stale `.mxcli/catalog.db` made the catalog look unfixed for three measurements; delete it before concluding anything about catalog output. Diagnosis tip: the sibling guard `TestModelerSystemEntities_HaveResolvableGeneralizations` existed and its enumeration twin did not — when one half of a synthesized module has a resolvability test, check whether the others do", "refs": "mendixlabs/mxcli#1102, mendixlabs/mxcli#1071, #889", "ce": "CE1613"} {"area":"mdl/backend","date":"2026-09-16","symptom":"With sdk/mpr at zero importers, `rm -rf sdk/mpr` still would not have been safe: it had two live dependencies that an import-based check cannot see. sdk/mpr/version has six importers (two of them shipping code, cmd/mxcli/docker/build.go and patch.go), and cmd/mxcli/docker/update_widgets_test.go reads sdk/mpr/testdata/v1-project by FILESYSTEM PATH.","cause":"The zero-importer guard matched the exact string \"github.com/mendixlabs/mxcli/sdk/mpr\". A SUBPACKAGE is a different import path, and a testdata directory is not an import at all — it is an os.DirFS string. Neither shape appears in a check written against the parent package's path.","file":"sdk/mpr","fix":"Repointed the six version importers at mdl/types (sdk/mpr/version.ProjectVersion is `type ProjectVersion = types.ProjectVersion`, an ALIAS, so it is the same type rather than a compatible one — modelsdk/mpr/version declares a duplicate struct and would NOT have been), moved the v1-project fixture to modelsdk/mpr/testdata/, verified both with the package still present, and only then deleted. 163 files, 41,674 lines.","insight":"Before deleting a package, search for THREE things, not one: the package's own import path, its subpackages' import paths (`.../pkg/`), and its directory as a literal string (testdata read through os.DirFS, go:embed, scripts). The last two are invisible to any importer census. Repoint everything FIRST and prove the build and tests green while the package still exists — that separates 'the repoint was wrong' from 'the deletion was wrong', which a single combined commit cannot distinguish. Two measurements worth keeping: the shipped binary is byte-identical in SIZE before and after, confirming the linker had already dropped the package, so this deletion removes source weight and not runtime behaviour; and `sdk/widgets` dropped to zero importers as a side effect but must NOT be deleted, because modelsdk/widgets/dirty_template_test.go reads sdk/widgets/templates/mendix-11.6 by path — the same path-not-import trap, found by grepping for the directory name rather than the import."} diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 64b2543910..af8f7dc2e7 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -627,3 +627,7 @@ {"area":"mdl/executor","date":"2026-09-15","symptom":"Two check-time validators (validate_import_mapping_find.go, validate_offline_paths.go) imported sdk/mpr directly — which CLAUDE.md's backend-abstraction checklist forbids for the executor. After porting them to the backend, coverage of every ported function was 0.0%: offlineProfilesIn, projectEntityFacts and the new openProjectForValidation were never executed by the suite.","cause":"Both validators FAIL OPEN by design: an unreadable project returns nil and silences the rule rather than failing the check on something it could not inspect. The existing tests only exercised the pure helpers (offlinePathViolations) or passed an empty projectPath, so the reader half never ran.","file":"mdl/executor/validate_project_reader.go","fix":"Added openProjectForValidation as the package's one read-only connect, and tests that exercise all three against a real fixture. Coverage 0.0% -> 72%/78%/100%.","insight":"A FAIL-OPEN validator is the worst shape for an unverified port: break its reader and the rule stops firing, which looks exactly like a project the rule does not apply to. Nothing goes red. So for any fail-open code path, the test must assert the rule FIRES on a project that should trigger it — asserting it stays quiet proves nothing, since quiet is also the failure mode. Two setup details that decide whether such a test is real. (1) The fixture ships only an ONLINE navigation profile, so offlineProfilesIn returns empty on it either way; the test has to SEED an offline profile, and Mendix fixes the legal names (Responsive/Phone/Tablet + the *Offline variants) — an invented name is refused by the executor, which is how the first attempt failed. (2) Assert the control first: the stock fixture reports zero offline profiles, so a reader that invented one is caught before the positive assertion runs. Also worth noting the signature constraint that shaped the fix: ValidateProgram takes a project PATH, not a backend, because `check --references` validates a script against a project it never connects an executor to — so these open their own short-lived read-only connection rather than threading ctx.Backend through a public signature and every caller."} {"area":"mdl/executor","date":"2026-09-15","symptom":"TestRoundtripPage_MicroflowButtonWithCurrentObject failed on main and on every branch cut from it: 'Expected Target: $currentObject parameter mapping in describe output', while the printed output plainly contained the mapping as \"Target\": $currentObject. Unit tests were green; only the integration suite (-tags integration) caught it.","cause":"Not a describe regression at all. mdl/executor/identifier_quoting.go's mdlIdent quotes any identifier that does not LEX as a bare identifier, running the real ANTLR lexer. #476 (notify workflow ... TARGET) added `TARGET: T A R G E T;` to MDLLexer.g4, so the parameter named Target began lexing as a keyword token and DESCRIBE started quoting it. The output became MORE correct; the test's exact-substring assertion went stale.","file":"mdl/executor/roundtrip_page_test.go","fix":"Made the assertion quoting-agnostic (accepts Target: or \\\"Target\\\":). Controlled by renaming the expected parameter to a name that is absent, which still fails — so the assertion continues to detect a genuinely dropped mapping rather than passing on anything.","insight":"Adding a keyword to MDLLexer.g4 silently reformats DESCRIBE output for every existing element whose NAME matches that keyword, anywhere mdlIdent is used — the grammar change and the broken test are in different packages with no compile-time link, so nothing points from one to the other. When adding a token, grep the test tree for exact-substring assertions containing that word: here `grep -rn '\"Target: '` found the single collision in seconds, where reading the #476 diff never would have. The deeper rule is that an exact-substring assertion on DESCRIBE output encodes a quoting decision the test does not care about; assert the mapping quoting-agnostically, or re-parse the output, since what a roundtrip test means to check is that the mapping survived. Note the input side did NOT break: TARGET was added to the non-reserved-keyword rule, so scripts writing `Target:` unquoted still parse — which is why check-mdl's 544 scripts stayed green and only this one output assertion moved."} {"area": "mdl/executor", "date": "2026-09-15", "symptom": "check --references rejected a page that mxbuild builds at 0 errors: 'the constraint on Administration.Account names \"System.UserRoles\", which is neither an attribute nor an association of it'. Administration.Account extends System.User and System.UserRoles is declared from System.User, so it IS an association of it, by inheritance. Every inherited association was a false positive, and separately so was every cross-module one. Inherited ATTRIBUTES were fine throughout, which is what made it look like a narrow bug rather than a whole axis.", "cause": "associationTargetFrom matched the start entity against the association's two ends by exact equality and scanned only dm.Associations. Its doc comment said a specialisation deliberately returns false, 'the cost of being wrong is a false error on a working script' - sound while its only caller (resolveMemberOnEntity, typing an association retrieve) treated false as silence. The new XPath constraint checker's noteQualified then called the same helper and treated false as EVIDENCE, so the exact case the comment declined to chase became the finding. noteQualified did carry a three-valued guard, but it tested whether the BASE ENTITY was known - the wrong axis; Administration.Account is known.", "file": "mdl/executor/validate_member_refs.go", "fix": "Replaced the boolean with assocResolution (resolved / missing / notAnEnd / unknown). The start entity is matched through its generalization chain (generalizationChain, built on findEntityByQN), and a chain that could not be walked to its root yields assocUnknown = silence. associationEndsByName also reads dm.CrossAssociations, whose far end is held by NAME not by element ID. noteQualified reports only assocMissing and assocNotAnEnd.", "insight": "A helper whose contract is 'false when it cannot establish the answer' is safe only while every caller treats false as silence; the moment one caller treats it as evidence, the comment promising restraint becomes the specification of a false positive. A boolean cannot carry 'no' and 'don't know' to two callers that need to tell them apart - if the codebase already has a three-valued resolver next door (memberResolution, ten lines up), reusing the two-valued sibling is the smell. Two cheap guards would have caught it: a fixture entity with a GENERALIZATION carrying an association, and one CROSS-MODULE association - a fixture of flat single-module entities cannot distinguish a correct resolver from one comparing two names. Establish the verdict with the build, not by reading: mx check on the rejected page said 0 errors, which settles it in one run."} +{"area": "mdl/executor", "date": "2026-09-16", "symptom": "mendixlabs/mxcli#1103's real defect: `retrieve $reqs from Mod.E where … limit 1;` followed by `head($reqs)` passed `mxcli check --references` and failed the build with CE0097 'The selected reqs variable must be of type List'. Inside a .test.mdl file it was worse — the injected test just failed to build, with no error text at all on the --attach path.", "cause": "cmd_microflows_builder_actions.go maps `limit \"1\"` with no offset to microflows.RangeTypeFirst — Mendix's 'First object' range — so the output variable is an OBJECT, not a one-element list. That is deliberate and documented (MDL_QUICK_REFERENCE), but nothing between the author and mxbuild said so: describe re-emits `limit 1`, so an object retrieve and a list retrieve are byte-identical MDL.", "file": "mdl/executor/validate_microflow_retrieve_single.go", "fix": "MDL-RETRIEVE01: track variables bound by a limit-1-no-offset retrieve in statement order (a rebinding clears them) and flag a later list use — list operation, aggregate, loop, ADD/REMOVE. Keyed on exactly the writer's condition. The message names CE0097 and both working spellings.", "insight": "A silent type change is the expensive kind, and this one had every property that makes it hard: the source text is identical on both sides, DESCRIBE round-trips it unchanged, and the only signal is a CE code from a tool at the far end of a build. When a clause changes a variable's CARDINALITY rather than its value, the check that catches it has to key on exactly the same condition as the writer — `limit == \"1\" && offset == \"\"` here, copied from the builder — or the diagnostic and the model disagree, which is worse than neither. The confusion is also structural, not carelessness: the SAME word means the opposite elsewhere in MDL, since `import from mapping … first` binds an object and `… limit 1` a one-element list. Where a language contradicts itself, the message must name the working spelling rather than only refuse. Cheap control worth copying: run the whole mdl-examples corpus (`make check-mdl`, 558 scripts) after adding a rule — zero new failures is a real statement about false positives that unit tests cannot make."} +{"area": "mdl/executor", "date": "2026-09-16", "symptom": "`CREATE ENUMERATION System.BrandNewThing (A 'a');` prints **\"Created enumeration: System.BrandNewThing\"** and writes an ORPHANED unit — measured 369 -> 370 units, the new unit's ContainerID is the synthetic module id `00000000-0000-0000-0000-000000000001`, which is present in no Unit row. Silent model corruption on a success message. `ALTER`/`DROP`/`MOVE ENUMERATION System.X` instead leak a raw `open …/00000000-…-0002.mxunit: no such file or directory`", "cause": "The System module is virtual — synthesized in code, no stored unit — but `findOrCreateModule(\"System\")` resolves it out of `ListModules` and hands its SYNTHETIC id over as the new enumeration's container. Entities happen to fail safe because they must load the virtual domain-model unit first; an enumeration is a unit in its own right, so nothing stopped the write. Making the System enums readable (#1102) also made them addressable by the four write verbs, which is what forced the guard", "file": "`mdl/executor/cmd_enumerations.go` (`refuseSystemEnumerationWrite`, called from create/alter/drop); `mdl/executor/cmd_move.go` (`moveEnumeration`, both ends); `mdl/executor/cmd_rename.go` (`execRenameEnumeration`); `mdl/executor/cmd_modules.go` (`execDropModule` refuses System outright)", "insight": "**Making a synthesized element readable makes it writable — audit EVERY write verb in the same change.** The read fix is one append; enumerating what it exposed took three passes and kept growing: create, alter, drop, MOVE (both ends — guarding only the source still lets a user enum be moved INTO System, the same orphan by another route), rename, and DROP MODULE System, whose cascade walked the newly-visible documents and printed \"unit not found\" 15 times. `grep -rn 'Backend.CreateX\\|Backend.UpdateX\\|Backend.DeleteX\\|Backend.MoveX'` over the executor is the census that ends the guessing — do it BEFORE writing the guard, not after each test failure. **Key the guard on the module NAME, not the container**: a brand-new enumeration has no container yet, and that is exactly the case that corrupted. This is the opposite of `isMarketplaceModule`, which deliberately distrusts names because a user may name a module Atlas_Core — `System` is platform-reserved and cannot be a user module, so the name IS the signal. Assert the refusal never reached the backend, not just that an error came back: a refusal that still wrote would leave the orphan. **A read-only doctype must not DESCRIBE as re-executable MDL** — emitting `create or modify enumeration System.X` hands the reader (or an LLM) a statement the guard rejects, so System enums describe as `--` comment lines the way DESCRIBE BUILDING BLOCK does, with a control test that USER enums still round-trip. And do not name the statement's own element in the hint: on a CREATE that name does not exist, so \"use `describe enumeration System.BrandNewThing`\" sends the reader after nothing", "refs": "mendixlabs/mxcli#1102"} +{"area": "mdl/executor", "date": "2026-09-16", "symptom": "A DataGrid2 column holding BOTH a custom-content widget and a filter widget (`column IsActive { checkbox cbActive (Editable: Never) dropdownfilter ddfActive }`) is written correctly \u2014 measured on 11.6.6: showContentAs=customContent, content=[cbActive], filter=[ddfActive], mx check 0 errors, checkbox cells AND a working Yes/No filter in the browser \u2014 but `describe page` emits only the checkbox, so describe -> exec DELETES the filter. Every signal is green at both ends. Reported upstream (mendixlabs/mxcli#1111) as 'DataGrid2 cannot combine content and a filter', i.e. as a missing capability rather than a describe bug.", "cause": "extractDataGrid2Column took the FIRST Widgets-typed property it met (`if len(col.ContentWidgets) == 0`) instead of keying on the resolved propKey, and rawDataGridColumn had no filter field at all. A column has TWO Widgets-typed slots; the writer emits column properties alphabetically, so `content` precedes `filter` and won. A filter-only column round-tripped by ACCIDENT: with content empty the filter landed in ContentWidgets and was re-emitted in the column body, where the builder routes it back to the filter slot by widget type (itemSlotAcceptedChildTypes) \u2014 which is why the common shape looked correct.", "file": "mdl/executor/cmd_pages_describe_pluggable.go", "fix": "rawDataGridColumn gains FilterWidgets; route on propKey (content -> ContentWidgets, filter -> FilterWidgets) with the first-wins heuristic kept only for propKey == \"\" (no key map); outputDataGrid2ColumnV3 opens a body when either list is non-empty and emits content widgets then filter widgets. ako/mxcli#489.", "insight": "Two sibling slots of the same TYPE need routing by KEY, not by shape \u2014 a reader that asks 'is this a Widgets array?' cannot tell `content` from `filter`, and the writer's alphabetical order silently decides which one survives. Worth checking wherever a describer matches on value shape: the key map is already in hand (WidgetProperty.TypePointer -> PropertyType $ID; note the inner WidgetValue.TypePointer points at the ValueType $ID instead, so probing at the wrong node level makes the map look broken when it is not). The accidental-success case is the real trap: filter-only columns worked, so the gap only appeared in the combination, and the reporter rationalised it as a platform limitation \u2014 the widget XML says otherwise (`content` and `filter` are independent `widgets` properties with no dependency on showContentAs). Control that settles it in one step: re-exec the describe output and read the verb \u2014 `Unchanged page` means the description reproduces the document, `Replaced page` means it does not."} +{"area": "mdl/executor", "date": "2026-09-16", "symptom": "`checkbox cb (Attribute: A, Editable: Never, ReadOnlyStyle: Control)` parses, passes `mxcli check`, is reported as executed \u2014 and the stored document keeps ReadOnlyStyle \"Inherit\". DESCRIBE *does* read and emit the property, so describe -> exec on a Studio Pro-authored page silently downgraded Control to Inherit. Visible only in the browser: with Inherit a read-only check box renders the TEXT \"Yes\"/\"No\", with Control the (disabled) checkbox glyph \u2014 measured on 11.6.6 by patching the stored string by hand ('Inherit' and 'Control' are both 7 bytes, so a byte substitution in the .mxunit is a valid document; mx check 0 errors both ways).", "cause": "Three silent layers. sdk/pages.CheckBox had no ReadOnlyStyle field, so buildCheckBoxV3 had nowhere to put the property; the codec writer hardcoded g.SetReadOnlyStyle(\"Inherit\") (and a per-type constant for TextBox/TextArea/DatePicker/RadioButtons, \"Control\" for DataView); and `ReadOnlyStyle` sits in validate_widgets.go's known-property allowlist under the comment \"vocabulary describe page emits\", which silenced the MDL-WIDGET check that flags a property no builder consumes.", "file": "mdl/executor/cmd_pages_builder_v3_widgets.go", "fix": "Add ReadOnlyStyle to pages.CheckBox; read and canonicalise it in buildCheckBoxV3 (Inherit/Control/Text, case-insensitive in, Mendix casing out, unknown value refused); write orDefaultStr(x.ReadOnlyStyle, \"Inherit\") in the codec so an omitted property still produces the document it always did. ako/mxcli#490.", "insight": "An allowlist added so DESCRIBE output re-parses will also silence the warning that a property is going nowhere \u2014 the two uses are indistinguishable from inside the checker, so entries added for the first reason need a consuming builder or they become a licence to drop. The asymmetry is the tell: a describer that READS a property whose writer hardcodes a constant is a round trip that quietly rewrites the user's model, and it is worth grepping for that pairing directly (`grep SetX(\"literal\")` against what extract*/describe reads). Measurement trick worth reusing: when the model layer cannot express a value, patch the stored BSON to the candidate value and boot \u2014 a same-length string substitution keeps the document valid, which settles 'is this the property that changes the rendering?' before writing any Go. The end-to-end control afterwards is the elision verb: re-authoring through MDL over the hand-patched document reported `Unchanged page`, i.e. what mxcli now writes is byte-for-byte the document that was verified in the browser."} diff --git a/.claude/skills/fix-issue/findings/mdl-other.jsonl b/.claude/skills/fix-issue/findings/mdl-other.jsonl index df5b3e0db7..3ced6780e2 100644 --- a/.claude/skills/fix-issue/findings/mdl-other.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-other.jsonl @@ -63,3 +63,4 @@ {"area": "mdl/catalog", "date": "2026-09-09", "symptom": "A new CATALOG view is queryable by name but absent from `show catalog tables`, so nobody can discover it — caught by TestTables_CoversAllViews, not by any query", "cause": "Adding a view to tables.go creates it in SQLite, but Catalog.Tables() is a separate hand-maintained list and is what SHOW CATALOG TABLES prints. The two are not derived from each other", "file": "`mdl/catalog/catalog.go` (Tables), `mdl/catalog/tables.go`", "insight": "Two hand-maintained lists of the same thing, with a guard test comparing them — the guard is the only reason this is a five-second fix instead of a view nobody finds for months. When adding a catalog view, expect to touch both. The general shape recurs in this codebase (stmtCreateInfo vs projectNameSets.setFor under-reported conflicts for the same reason), so the question to ask of any list is what compares it to its twin", "refs": ["ako/mxcli#413"]} {"area":"mdl/catalog","date":"2026-09-13","symptom":"A user asks 'are there circular dependencies between my modules?', reads CATALOG.GRAPH_CYCLES, gets 0 rows, and concludes there are none — while CATALOG.GRAPH_MODULE_COUPLING lists the same module pair in both directions (mendixlabs/mxcli#1060)","cause":"GRAPH_CYCLES is SCCs of the ASSET graph. Modules A and B reference each other through DIFFERENT documents, which form no cycle among themselves, so the asset table is correctly empty for a genuinely circular module pair. Compounded by scope: the asset graph admits only graphRefKinds, while coupling counts every kind — 110 of 316 edges on a stock 11.14 app, and Administration -> Atlas_Core there is `layout`-only","file":"`mdl/catalog/builder_graph.go` (`buildModuleCycles`, `loadModuleEdges`, `graphRefKindsSQL`), `mdl/catalog/tables.go` (`graph_module_cycles_data`, `graph_analysis_scope`), `mdl/linter/starlark_graph.go` (`module_cycles()`)","insight":"**When a report says two tables disagree, check whether they answer different questions before checking whether one is broken — then make the difference queryable.** Tarjan was fine; the granularity and the edge filter were both undocumented and invisible from SQL. Two traps in the implementation: (1) the new module pass sat after `if len(edges) == 0 { return }` on the STRUCTURAL edge set, so a project whose only cross-module refs are navigational — the reported case exactly — got nothing; a test with only `layout` edges caught it. (2) A module-cycle table built on the structural subset would have shipped green and still answered 'none' for the reported pair, so the edge set has to match graph_module_coupling, the table it is read beside. Generate the scope view's IN list from graphRefKinds rather than restating it, or the documentation of the filter drifts from the filter","fix":"Add graph_module_cycles (module SCCs, all kinds, with a RefKinds column naming the edges inside the cycle) and graph_analysis_scope (per-kind edge counts + InAssetGraph), plus a module_cycles() Starlark builtin"} {"area": "mdl/ast", "date": "2026-09-15", "symptom": "Rewriting a microflow from its own `describe microflow` output moves its workflow actions: after `create or modify`, `open workflow` / `notify workflow` (and every other workflow action) sit ~800px further right, the first one on top of the end event; a second rewrite is stable. Five `log` statements round-trip identically", "cause": "`@position` (and `@caption`, `@color`, `@anchor`) was parsed and dropped for these statements: the visitor's `setStatementAnnotations` and the builder's `getStatementAnnotations` were hand-written type switches with no case for any of the eleven workflow statements or the three mapping statements (import/export mapping, transform json). The setter also had an EMPTY `case *ast.EnumSplitStmt:` — a no-op in Go, which does not fall through — and the getter lacked `SendRestRequestStmt`. With no annotation the builder auto-placed each action after the start event, which the stored start position had moved right", "file": "`mdl/ast/annotations.go` (new `SetStatementAnnotations`), `mdl/visitor/visitor_microflow_statements.go` (`setStatementAnnotations`), `mdl/executor/cmd_microflows_builder_annotations.go` (`getStatementAnnotations`)", "insight": "**This is #884's lesson, unapplied one level up.** #884 introduced the reflective `ast.StatementAnnotations` precisely because a type switch over the annotated statements silently skips the one added later — but only the validator used it, while the two switches that actually carry `@position` from source to BSON stayed hand-written and had already missed 15 types. When a class-level fix lands, grep for every other copy of the pattern it replaces. Both now delegate to reflection, so a new statement type with an `Annotations` field is covered on declaration; `TestEveryAnnotatedStatementIsReachable` pins the field shape. **Diagnose with the source, not the symptom list**: a go/parser scan of the AST structs against the switch case labels found all 15 gaps (and the empty case) in one run, where the report named two statements. Control: with both switches restored, `TestPositionAnnotationPlacesEveryActionStatement` fails 17 subtests at the builder default (100,100) while its `log` subtest passes. Measured on mx-test-projects/i956 (11.13): describe→rewrite→describe diffs every position before, identical after. Tests `mdl/executor/microflow_statement_position_test.go`, `mdl/ast/annotations_coverage_test.go`", "refs": [], "rules": []} +{"area": "mdl/exprcheck", "date": "2026-09-16", "symptom": "`$out = $out + $r/Status` inside `LOOP $r IN $reqs` (Enumeration into a String) passes `mxcli check -p --references`, is written by `exec`, and fails the native build with **CE0117** at the Change variable activity. The same mistake on a PARAMETER is refused as E004, so the checker looks like it is skipped inside LOOP bodies (mendixlabs/mxcli#1100)", "cause": "The loop BODY was walked and checked all along \u2014 the control that proves it is `'status=' + $T/Status` on a parameter written one line INSIDE the loop, which was refused before the fix. Two holes in the variable scope, in series, produced the asymmetry: (a) `buildVarEntityScope` recorded CREATE, database RETRIEVE and parameters but never `LoopStmt.LoopVariable`, so `$r/Status` resolved to no attribute and inferred KindUnknown, which every rule tolerates by design; (b) `CheckAdapter` never set `Context.Scope` at all, so a DECLARE'd `$out String` was Unknown too \u2014 and E004 needs BOTH operands typed, so closing (a) alone still reported nothing on the reported script. (b) also meant `$out = $out + $Req/Status` with no loop in sight was equally silent; the report's own case A hides that by using a string literal on the left", "file": "`mdl/exprcheck/adapters/adapter_scope.go` (`buildFlowScope` replacing `buildVarEntityScope`, `recordRetrieve`/`recordListOperation`/`recordDeclare`, `kindScope`, `StatementErrorHandling`, `DataTypeKind`), `mdl/exprcheck/adapters/check.go` (`walkFlow` passes Scope; `checkListOperationCondition`; ON ERROR bodies walked), `mdl/exprcheck/slot_resolver.go` + `slot_to_context.go` (`ListOperation.Condition`), `mdl/executor/validate_microflow.go` (delegates `astKindToExprKind` and `stmtErrorHandling`)", "insight": "**Separate \"was the walk there\" from \"did the variable resolve\" before believing a skipped-construct report.** The title said LOOP bodies were not checked; one control \u2014 the same expression on a parameter, one line deeper \u2014 showed the walk was fine and the scope was not, which changed the fix from a walk to a resolver. **A silence can need two fixes to break**: typing the loop variable alone left the reported script still reporting nothing, because the rule needs both operands. Fix one, re-measure, and do not conclude the fix failed. **The same walk already existed, correct, next door**: `mdl/executor/validate_member_refs.go` typed loop variables from the list; the expression checker's walk did not \u2014 duplicate-resolver drift, which is why `stmtErrorHandling` and the DataTypeKind table are now single copies in adapters with the executor delegating. **Order is load-bearing and silent when wrong**: parameters must seed the scope BEFORE the body walk, or an association retrieve off a parameter (and every loop over its result) stays untyped \u2014 this was written the old way first and only a test caught it. **False-positive control**: exec-then-type-check over 591 mdl-examples scripts, 11 violations before and 11 after, same rules. It earned its keep \u2014 the first cut fired E009 on `set $At = find($Hay, $Needle)`, Mendix's STRING find, which the visitor still builds as a ListOperationStmt (the flow builder disambiguates it later, ledger #63). Requiring a KNOWN element entity before checking a FIND/FILTER predicate applies the same disambiguation. Controls: each of the four scope sources reverted in turn fails a distinct test with the reported symptom (empty violations). **Still open**: a bare attribute name in a FILTER predicate resolves to nothing, `retrieve \u2026 limit 1` is typed as a list like any other retrieve, and `LOOP $r IN $T/Mod.Assoc` cannot be typed because the visitor drops the association path (`ListVariable` is empty)"} diff --git a/.claude/skills/fix-issue/findings/mdl-visitor.jsonl b/.claude/skills/fix-issue/findings/mdl-visitor.jsonl index 9c40768ba2..7c861ed290 100644 --- a/.claude/skills/fix-issue/findings/mdl-visitor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-visitor.jsonl @@ -30,3 +30,4 @@ {"area": "mdl/visitor", "date": "2026-09-08", "symptom": "`call microflow M.ACT with (Ctx = $WorkflowContext)` — an UNQUOTED value in a workflow parameter mapping — crashed the binary with SIGSEGV (nil pointer) in buildWorkflowCallMicroflow, on `check`, `check --references` and `exec` alike, with no diagnostic beyond the Go panic. Reported as mendixlabs/mxcli#1023.", "cause": "The grammar rule workflowParameterMapping requires STRING_LITERAL, but visitor.Build() walks the parse tree even when the parse FAILED (deliberately — that is what lets check report more than the first error). Under ANTLR error recovery the rule context exists with a nil STRING_LITERAL child, and the visitor read it unguarded. Same bug at the CALL WORKFLOW site.", "fix": "Factor both sites into buildWorkflowParameterMappings, nil-checking QualifiedName() and STRING_LITERAL() and skipping the mapping. The syntax error the listener already recorded ('mismatched input ... expecting STRING_LITERAL') becomes what the author sees.", "file": "mdl/visitor/visitor_workflow.go", "insight": "In this codebase a required grammar child is NOT a guarantee inside the visitor, because Build() walks a failed parse on purpose. Every ctx.X().GetText() on a required child is therefore a latent crash reachable from ordinary malformed input — grep 'STRING_LITERAL().GetText()' for the ones still unguarded. The other half of the finding is documentation-shaped: `mxcli syntax workflow call-microflow` omitted the WITH clause entirely, so an author had nothing to copy and reached for the bare-variable spelling used everywhere else in MDL. A crash on input the tool's own docs do not cover is a docs bug with a segfault attached."} {"area": "mdl/visitor", "date": "2026-09-07", "symptom": "`create or modify snippet M.S (params: { $T: Mod.\"Thing\" })` failed at execution with `failed to resolve entity Mod.\"Thing\": entity not found`, while the identical quoted form in a PAGE parameter resolved fine (ako/CapTrackV4 019).", "cause": "buildSnippetParameterListAsPage re-split the parse node's TEXT (`parseQualifiedName(dt.GetText())`), and GetText() returns the source verbatim, quotes included. The page path walks the parse tree, where buildQualifiedName unquotes each part via identifierOrKeywordText. Fixed by walking the tree; the dead duplicate buildSnippetParameters — a correct implementation nothing called — was removed.", "file": "`mdl/visitor/visitor_page_v3.go` (buildSnippetParameterListAsPage); `mdl/visitor/visitor_page.go` (removed buildSnippetParameters); tests `mdl/visitor/snippet_param_quoted_entity_test.go`", "insight": "GetText() on an ANTLR context is the source text, not the resolved value, so any conversion built on it silently keeps quoting, whitespace and casing that the tree-walking helpers strip. Grep for `parseQualifiedName(.*GetText())` when a name resolves in one statement and not in a sibling. The asymmetry is also the diagnosis: when two statements accept the same syntax and only one works, compare their VISITORS before their executors — here both executor paths were identical and called the same resolveEntity. Two copies of one conversion with one of them dead is how they drifted, so the dead one is deleted rather than fixed."} {"area": "mdl/visitor", "date": "2026-09-15", "symptom": "`placeholder Main { }` inside a CREATE LAYOUT passes `mxcli check`, then fails at exec with `layout \"X\" declares no placeholder` \u2014 a message that flatly contradicts the script, which says `placeholder Main`. The failed exec has already created the module.", "cause": "One grammar rule (placeholderBlockV3) serves two opposite jobs, told apart by shape: `if c.LBRACE() == nil` makes a bodiless placeholder a DECLARATION widget, and the braced form is routed to buildPagePlaceholdersV3 \u2014 the page-side job of FILLING a layout slot. In a layout there is no such job, so the braced form was dropped on the floor and the layout ended up with zero placeholders.", "fix": "Builder gained `inLayout` (saved/restored around the layout body build, since the same body builder serves pages) and collects the dropped names into ast.CreateLayoutStmt.BracedPlaceholders; MDL083 reports them at check time. The braced form is still dropped \u2014 recording it is a diagnostic, not a decision to honour it.", "insight": "When one parse rule serves two documents and is disambiguated by SHAPE rather than by context, the wrong shape has no error path by construction \u2014 it just silently means the other thing. The tell is a runtime message that contradicts the source text. Note the mistake is the natural one: every other layout element takes a body, and `alter page` uses the braced form for real, so the author is generalising correctly from the rest of the language. DESCRIBE emits the bodiless form, so round-tripping never produces it and no existing test covered it.", "controls": "A page's braced placeholder must still fill a slot (TestBuildPageV3_BracedPlaceholderStillFillsASlot) and a layout's bodiless form must still produce a real widget \u2014 the flag is save/restored precisely so a page later in the same script is not flagged.", "refs": "mendixlabs/mxcli#1063", "file": "mdl/visitor/visitor_page_v3.go, mdl/visitor/visitor.go, mdl/ast/ast_page_v3.go"} +{"area": "mdl/visitor", "date": "2026-09-16", "symptom": "`$n = COUNT(FILTER($reqs, $currentObject/Status = Mod.E.Approved))` passed `mxcli check`, execed with \"Created microflow\", and then failed the build with CE0012 \"The 'List' property is required.\" at Aggregate list activity 'Count' (mendixlabs/mxcli#1101). `describe` read the stored flow back as `$n = count($)`.", "cause": "buildSetStatementNode converts a SET whose value is a list/aggregate call into an activity statement, and every arm took its list operand through extractVariableName, which handles *ast.VariableExpr and *ast.IdentifierExpr and returns \"\" for anything else with no default branch. A nested call is 'anything else', so the inner call was discarded (list AND predicate) and the activity written with an empty AggregateVariableName. Fixed by recording the dropped operand on the statement (ast.UnresolvedOperand) and refusing it at check time as MDL-LISTOP02.", "file": "`mdl/visitor/visitor_microflow_statements.go` (buildListOrAggregateStatement, recordUnresolvedOperands); `mdl/ast/ast_microflow.go` (UnresolvedOperand); `mdl/executor/validate_microflow_listop_source.go`; tests `mdl/executor/validate_microflow_listop_source_test.go`, `mdl-examples/bug-tests/1101-nested-list-operand-dropped.fail.mdl`", "insight": "A type switch with no default over AST nodes is a silent data-loss site, and `extractVariableName`-shaped helpers ('return the name, or \"\"') hide it behind a value that reads as absence. The measurement that mapped the blast radius was not reading code: exec each spelling into its own copy of a project and run mxbuild, one microflow per project so the error count is unambiguous. That turned a COUNT bug into six — head/tail/filter/sort/union and a bare string literal — and found one strictly worse than the report: `sort(filter(…), Name)` makes mxbuild ABORT with InvalidOperationException rather than report an error, because the sort attribute resolves against the absent list's entity, so the document cannot be loaded at all. Put the control in the SAME project as the defect where possible: the reporter's two-statement workaround next to the nested form gave `The app contains: 1 errors`, which proves the rule's scope in one build. When several arms each do the same bookkeeping inline, move them to one tail rather than adding the bookkeeping N times — this file's own buildSetAggregate comment already says that is how the attribute went missing before.", "refs": "mendixlabs/mxcli#1101; sibling rule MDL-LISTOP01 (#1002); related surface-syntax issue mendixlabs/mxcli#750 (expressions vs. what the model can store)"} diff --git a/.claude/skills/mendix/patterns-data-processing/SKILL.md b/.claude/skills/mendix/patterns-data-processing/SKILL.md index ac6fa53476..8512289c96 100644 --- a/.claude/skills/mendix/patterns-data-processing/SKILL.md +++ b/.claude/skills/mendix/patterns-data-processing/SKILL.md @@ -232,6 +232,37 @@ $MaxPrice = maximum($Products.Price); ## List Operations +### One statement per operation — they do not nest + +Every list operation and aggregate is a separate **activity** in Mendix, and an +activity stores its list as a **variable reference**. There is no slot for a +nested computation, so this is not a shorter spelling — it is a list argument the +model cannot hold: + +```mdl +-- WRONG. mxcli check refuses this as MDL-LISTOP02. +$n = count(filter($Requests, $currentObject/Status = Module.ENUM_Status.Approved)); +``` + +Before the rule existed it parsed, passed `check`, and execed with +`Created microflow` — then dropped the inner call entirely and wrote an activity +with an empty list, which mxbuild rejected with **CE0012** (`The 'List' property +is required.`) for an aggregate or **CE0096** for a list operation. The +`sort(filter(…), Attr)` shape was worse still: with the list gone the sort +attribute has no entity to resolve against, and mxbuild aborts rather than +reporting an error. + +Give the inner operation its own statement and pass the variable: + +```mdl +-- RIGHT +$Approved = filter($Requests, $currentObject/Status = Module.ENUM_Status.Approved); +$n = count($Approved); +``` + +The same applies to both operands of `union`/`intersect`/`subtract`, and to any +non-variable list argument — `count('nonsense')` fails the same way. + ### Add to List ```mdl diff --git a/.claude/skills/mendix/system-module/SKILL.md b/.claude/skills/mendix/system-module/SKILL.md index cdc9e3da2a..709c004c6d 100644 --- a/.claude/skills/mendix/system-module/SKILL.md +++ b/.claude/skills/mendix/system-module/SKILL.md @@ -353,43 +353,68 @@ HTTP proxy settings (internal use). ## 8. Enumerations -### WorkflowState -`InProgress`, `Paused`, `Completed`, `Aborted`, `Incompatible`, `Failed` +Inspect any of these from the CLI rather than copying from here — the values are +**case-sensitive**, and a wrong one is only caught at build time as **CE1613** +"The selected enumeration value no longer exists": -### WorkflowUserTaskState -`created`, `InProgress`, `Completed`, `Paused`, `Aborted`, `Failed` +```bash +mxcli -p app.mpr describe enumeration System.WorkflowActivityType +mxcli -p app.mpr show enumerations # includes the System module +``` -### WorkflowUserTaskCompletionType -`single`, `Veto`, `Consensus`, `Majority`, `Threshold`, `microflow` +Two of these names are easy to confuse, and mixing them up is the mistake that +CE1613 usually reports: **`WorkflowActivityState`** has `Finished`, while +**`WorkflowActivityExecutionState`** has `Completed`. They are different +enumerations on different entities. -### WorkflowActivityType -`Start`, `end`, `ExclusiveSplit`, `ParallelSplit`, `ParallelSplitBranchStopper`, `ParallelSplitMerge`, `UserTask`, `CallMicroflow`, `CallWorkflow`, `JumpTo`, `MultiInputUserTask`, `WaitForNotification`, `WaitForTimer`, `EndOfBoundaryEventPath`, `NonInterruptingTimerEvent`, `InterruptingTimerEvent` +System enumerations are **read-only** — they are built into the platform, not +stored in the project, so `create`/`alter`/`drop`/`move enumeration System.…` is +refused. `describe` prints them as `--` comment lines for that reason. -### WorkflowActivityExecutionState -`created`, `InProgress`, `Completed`, `Paused`, `Aborted`, `Failed` +### ContextType +`System`, `User`, `Anonymous`, `ScheduledEvent` -### WorkflowCurrentActivityAction -`DoNothing`, `JumpTo` +### DeviceType +`Phone`, `Tablet`, `Desktop` -### WorkflowEventType -`WorkflowCompleted`, `WorkflowInitiated`, `WorkflowRestarted`, `WorkflowFailed`, `WorkflowAborted`, `WorkflowPaused`, `WorkflowUnpaused`, `WorkflowRetried`, `WorkflowUpdated`, `WorkflowUpgraded`, `WorkflowConflicted`, `WorkflowResolved`, `WorkflowJumpToOptionApplied`, `StartEventExecuted`, `EndEventExecuted`, `DecisionExecuted`, `JumpExecuted`, `ParallelSplitExecuted`, `ParallelMergeExecuted`, `CallWorkflowStarted`, `CallWorkflowEnded`, `CallMicroflowStarted`, `CallMicroflowEnded`, `WaitForNotificationStarted`, `WaitForNotificationEnded`, `WaitForTimerStarted`, `WaitForTimerEnded`, `UserTaskStarted`, `MultiUserTaskOutcomeSelected`, `UserTaskEnded`, `NonInterruptingTimerEventExecuted`, `InterruptingTimerEventExecuted` +### EventStatus +`Running`, `Completed`, `Error`, `Stopped` + +### ProxyConfiguration +`UseAppSettings`, `Override`, `NoProxy` ### QueueTaskStatus `Idle`, `Running`, `Completed`, `Failed`, `Retrying`, `Aborted`, `Incompatible` -### EventStatus -`Running`, `Completed`, `error`, `Stopped` - -### ContextType -`System`, `user`, `Anonymous`, `ScheduledEvent` +### UnreferencedFileState +`New`, `Obsolete`, `Deleted` ### UserType -`Internal`, `external` +`Internal`, `External` -### DeviceType -`Phone`, `Tablet`, `Desktop` +### WorkflowActivityExecutionState +`Created`, `InProgress`, `Completed`, `Paused`, `Aborted`, `Failed` ---- +### WorkflowActivityState +`Started`, `Suspended`, `Finished`, `Replaced`, `Aborted`, `Failed` + +### WorkflowActivityType +`Start`, `End`, `ExclusiveSplit`, `ParallelSplit`, `ParallelSplitBranchStopper`, `ParallelSplitMerge`, `UserTask`, `CallMicroflow`, `CallWorkflow`, `JumpTo`, `MultiInputUserTask`, `WaitForNotification`, `WaitForTimer`, `EndOfBoundaryEventPath`, `NonInterruptingTimerEvent`, `InterruptingTimerEvent` + +### WorkflowCurrentActivityAction +`DoNothing`, `JumpTo` + +### WorkflowEventType +`WorkflowCompleted`, `WorkflowInitiated`, `WorkflowRestarted`, `WorkflowFailed`, `WorkflowAborted`, `WorkflowPaused`, `WorkflowUnpaused`, `WorkflowRetried`, `WorkflowUpdated`, `WorkflowUpgraded`, `WorkflowConflicted`, `WorkflowResolved`, `WorkflowJumpToOptionApplied`, `StartEventExecuted`, `EndEventExecuted`, `DecisionExecuted`, `JumpExecuted`, `ParallelSplitExecuted`, `ParallelMergeExecuted`, `CallWorkflowStarted`, `CallWorkflowEnded`, `CallMicroflowStarted`, `CallMicroflowEnded`, `WaitForNotificationStarted`, `WaitForNotificationEnded`, `WaitForTimerStarted`, `WaitForTimerEnded`, `UserTaskStarted`, `MultiUserTaskOutcomeSelected`, `UserTaskEnded`, `NonInterruptingTimerEventExecuted`, `InterruptingTimerEventExecuted` + +### WorkflowState +`InProgress`, `Paused`, `Completed`, `Aborted`, `Incompatible`, `Failed` + +### WorkflowUserTaskCompletionType +`Single`, `Veto`, `Consensus`, `Majority`, `Threshold`, `Microflow` + +### WorkflowUserTaskState +`Created`, `InProgress`, `Completed`, `Paused`, `Aborted`, `Failed` ## 9. Inheritance Hierarchies diff --git a/.claude/skills/mendix/test-microflows/SKILL.md b/.claude/skills/mendix/test-microflows/SKILL.md index cf55472aef..75d0b5c33d 100644 --- a/.claude/skills/mendix/test-microflows/SKILL.md +++ b/.claude/skills/mendix/test-microflows/SKILL.md @@ -381,8 +381,33 @@ when deployed anywhere else. The project's **Security Level is not modified**. The after-startup microflow runs in an administrative context and is not subject to it, and forcing it off breaks projects whose published REST/OData services use custom authentication. If a -cleanup step fails the run reports an error and names what was left changed — -the project is modified, so it must not read as a clean pass. +cleanup step fails the run reports an error, **names every generated document +still in the project and prints the `DROP` that removes it** — the project is +modified, so it must not read as a clean pass. + +Cleanup removes **every** generated `MxTest.Test_*` microflow the project holds, +not only the ones this run created. The names are positional (`Test_test_1`, +`_2`, … from the test's index in its file) and every test file reuses them, so +keying cleanup on the current suite left a flow behind whenever a later run had +fewer tests than an earlier one — and a leftover that does not build fails +**every subsequent run of every test file**, with a message about the project +rather than about any test (mendixlabs/mxcli#1104). + +## Check a test file before you run it + +`mxcli check suite.test.mdl` works, and is much faster than a run. A test block +is a **microflow body**, and `check` renders it as the microflow it becomes, on +the file's own lines — so a diagnostic points at the statement you wrote. + +That includes the semantic rules, which is where most of the value is: a test +whose body would not compile is reported here instead of failing the injection +with nothing but "the project cannot be deployed". An `@expect` or `@verify` that +cannot be evaluated is reported here too, as `MDL-TEST01`. + +One rule to know about, because its symptom is confusing and its shape is common +in tests: `retrieve $x … limit 1` binds a **single object**, not a one-element +list, so `head($x)` is `CE0097` at build time and `MDL-RETRIEVE01` at check time. +Drop the `limit` to get a list, or use the variable as the object it is. --- diff --git a/.claude/skills/mendix/write-workflows/SKILL.md b/.claude/skills/mendix/write-workflows/SKILL.md index 74cc5e743f..da0f9d7e1c 100644 --- a/.claude/skills/mendix/write-workflows/SKILL.md +++ b/.claude/skills/mendix/write-workflows/SKILL.md @@ -369,13 +369,26 @@ name.** A task declared `user task "ReviewAndPlan" 'Review and plan'` stores `Name = 'Review and plan'`, so routing an inbox on the activity name silently never matches. Route on your own entity's status instead. -## System-module documents are read from the runtime, not the .mpr +## System-module enumerations are synthesized, not stored -`describe enumeration System.WorkflowUserTaskState` and `show enumerations in System` -return nothing — the System module's **enumerations** are not in the project file, so -mxcli cannot resolve them. Constrain on an attribute instead (`[EndTime = empty]` -selects open tasks) rather than naming a System enum value. System **entities** are -documented in `system-module`. +The System module's enumerations are **not in the project file** — Mendix ships +them with the platform — so mxcli synthesizes them from its own table of platform +definitions. `describe enumeration System.WorkflowUserTaskState` and +`show enumerations` report them, read-only: + +```bash +mxcli -p app.mpr describe enumeration System.WorkflowUserTaskState +``` + +They used to return nothing, which is why guessing a value and hitting **CE1613** +"The selected enumeration value no longer exists" was the only way to find out +(mendixlabs/mxcli#1102). Check the values before branching on one — they are +case-sensitive, and `WorkflowActivityState` (`Finished`) is a different +enumeration from `WorkflowActivityExecutionState` (`Completed`). + +Constraining on an attribute (`[EndTime = empty]` selects open tasks) is still +often the better XPath, but it is no longer a workaround for not knowing the +values. The full list and the System **entities** are in `system-module`. ## Platform rules diff --git a/CHANGELOG.md b/CHANGELOG.md index c6110181a6..1424974070 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,26 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **The expression type checker was silent on a LOOP variable, and on every locally declared variable** (mendixlabs/mxcli#1100). `$out = $out + $r/Status` inside `LOOP $r IN $reqs` — an Enumeration concatenated into a String — passed `check --references`, was written by `exec`, and failed the build with **CE0117** at the Change variable activity, while the same mistake on a parameter was refused as **E004**. + + The report reads as "the checker is skipped inside a `LOOP` body". It is not, and the control says so: `'status=' + $T/Status` on a *parameter*, written one line **inside** the loop, was refused before this change. The body was always walked; the **variable scope** had two holes, and both had to close before the reported script reported anything. `LOOP $r IN $reqs` never recorded `$r`, so `$r/Status` resolved to no attribute and inferred Unknown — which every rule tolerates by design. And `Context.Scope` was never set at all, so a `DECLARE $out String` was Unknown too; E004 needs **both** operands typed, so typing the iterator alone still reported nothing. That second hole is why `$out = $out + $Req/Status` was equally silent with no loop in sight — the report's case A hides it by putting a string literal on the left. + + The list sources a loop can iterate are typed with it: a database retrieve, an **association** retrieve (the far end resolved through the association index, which needs parameters seeded before the body walk), a `CREATE LIST`, and the list operations that carry their input's element type through. Two other block-scoped positions the report asked about are covered: an **ON ERROR handler body**, which was not walked at all, so moving a statement into one exempted it from every rule; and a **FIND/FILTER predicate**, where `$currentObject` is bound to the element type of the list under test. A bare attribute name in a predicate (`FILTER($L, Status = 'Open')`) still resolves to nothing — binding bare names would change what a bare identifier means everywhere in an expression. + + Measured: exec-then-type-check over 591 `mdl-examples/` scripts gives **11 violations before and 11 after**, the same rules on the same lines. The sweep earned its keep — a first cut reported Mendix's **string** `find($Hay, $Needle)` as a non-Boolean predicate, because the visitor still builds it as a list operation and the flow builder disambiguates it later (ledger #63); a FIND/FILTER predicate is now checked only when the input list's element entity is known, which applies the same disambiguation. + +- **`mxcli check` and the editor could not parse a `.test.mdl` file at all** (mendixlabs/mxcli#1103). A test block is a **microflow body** — that is what the runner turns it into — and both were handing the file to the top-level grammar instead. `DECLARE` is not a top-level statement, so the parser resynced; `RETRIEVE` is a non-reserved keyword, so it was swallowed as an identifier; and the leftover `FROM …` started an OQL query, whose follow set is `{GROUP_BY, SELECT, HAVING}`. The reported error therefore told the author their `RETRIEVE` needed a `SELECT`, on a statement `mxcli syntax microflow.retrieve` prints as its own example. + + This was not a corner: the VS Code extension binds MDL to `.mdl`, which `.test.mdl` matches, so every test file open in the editor was a wall of squiggles — 9 of this repository's 10 test files reported errors, one of them 392, now all 0. Each block is rendered as the microflow it becomes, padded so it keeps its **source line numbers**, which is what lets every existing rule apply with no remapping: `mxcli check suite.test.mdl` now reports an uncompilable body, an unusable `@expect` or `@verify` (`MDL-TEST01`), and everything else, at the line the author wrote it on. `make check-mdl` sweeps test files too, with `.fail.test.mdl` for one whose annotations are deliberately unusable. + +- **`retrieve … limit 1` silently binds a single object, and nothing said so until the build** (mendixlabs/mxcli#1103). It is Mendix's "First object" range, so `head()`, `count()` or a `loop` over that variable is **CE0097** — but `check --references` passed, and `describe` re-emits `limit 1`, so an object retrieve and a list retrieve are identical MDL text. **MDL-RETRIEVE01** reports it at check time, naming the CE code and both working spellings. The behaviour itself is unchanged and still documented; only the silence is fixed. (The same word means the opposite on `import from mapping`, where `first` binds the object and `limit 1` a one-element list — which is why reading it as a list is a reasonable mistake.) + +- **`mxcli test` reported a rejected build as one sentence, and one leftover document then failed every later run of any test file** (mendixlabs/mxcli#1104). + + MxBuild puts the same text in `Message` for every failing build, so "build failed: The project cannot be deployed, because it contains errors." cannot distinguish "your test does not compile" from "an unrelated document is broken". The parsed problems were in hand and discarded one line before they were needed: `--attach` and every `--watch` rebuild built their error with `fmt.Errorf`, while the attribution that turns a build error into the failing test's row was wired only into the `--local` boot. Both paths now carry the problems, so a build error in a generated test microflow becomes that test's ERROR row and one in the project names its document. + + Cleanup now removes **every** generated `MxTest.Test_*` microflow the project holds rather than the current suite's. The names are positional (`Test_test_1`, `_2`, … from the test's index in its file) and every test file reuses them, so a run with fewer tests than the last one left the surplus behind — and under `--attach` the `MxTest` module always pre-exists, so the whole-module drop never fired. A single leftover that does not build then failed every subsequent run of every file, reporting a problem in the project rather than in any test. Keying on the suite was also wrong the other way: it issued `DROP` for flows a part-way injection never created, and those failures made cleanup announce "the project has been left modified" for a project it had just cleaned. What cleanup genuinely cannot remove is now named, with the `DROP` that removes it. + - **`check --references` rejected every XPath constraint that hops an INHERITED or a CROSS-MODULE association** (ako/mxcli-sudoku FINDINGS #57). The constraint-member check added in `3aa2ee0e` reported the stock `Administration.Account_Overview` page — `Administration.Account extends System.User`, so `System.UserRoles` (declared from `System.User`) is an association of it by inheritance, and `mx check` on the rejected page says 0 errors. Because the false positive lands on a Marketplace module almost every app has, `check` stopped being usable as a gate for anyone whose entities inherit, which is the normal case for anything extending `System.User`, `System.Image` or `System.FileDocument`. The lookup matched the start entity against the association's two ends by exact equality and read only `dm.Associations`. Its comment said the specialisation case was deliberately not chased — "the cost of being wrong is a false error on a working script" — and that was sound while its only caller treated `false` as *silence*; the new check treated the same `false` as *evidence*, so the precise case the comment declined to chase became the finding. It is three-valued now (resolved / missing / not-an-end / unknown): the start entity is matched through its generalization chain, a cross-module association is found where it is actually stored (`CrossAssociations`, far end held by name), and a chain that could not be walked to its root is silence rather than a report. The check still fires on an association the entity genuinely lacks, including a specialisation's association named on its generalization. diff --git a/CLAUDE.md b/CLAUDE.md index cb16d4d533..0de38ed30c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -812,8 +812,10 @@ These rules apply whenever generating microflow or nanoflow MDL. Violations are 1. **NEVER create empty list variables as loop sources.** If processing imported data, accept the list as a microflow parameter — `declare $Items list of ... = empty` followed by `loop $item in $Items` is always wrong. 2. **NEVER use nested LOOPs for list matching.** Loop over the primary list and use `$match = FIND($TargetList, key = $item/key)` for an O(N) in-memory lookup. A plain `retrieve … where` **cannot** filter a list variable (only a database/association source), so `retrieve $match from $TargetList where …` is a parse error — use `FIND`/`FILTER`. Nested loops are O(N^2). The `$item` there is the enclosing loop's iterator and stays valid — MDL-LISTOP01 flags a predicate variable that is *not in scope*, not the name. Inside the predicate itself, the item under test is `$currentObject` (a bare attribute name resolves to it). -3. **Use append logic when merging**, not overwrite: `$Existing/Field + '\n' + $New/Field` inside an `if $New/Field != empty` guard. -4. **Read `.claude/skills/patterns-data-processing.md`** for delta merge, batch processing, and list operation patterns. +3. **NEVER nest one list operation inside another.** Each of HEAD/TAIL/FIND/FILTER/SORT/UNION/INTERSECT/SUBTRACT/RANGE and the aggregates is a separate **activity**, and an activity stores its list as a **variable reference** — there is no slot for a nested computation. `$n = COUNT(FILTER($reqs, …))` parses, and used to drop the inner call entirely and write an activity with an empty list: `check` clean, `exec` reporting "Created microflow", then CE0012 / CE0096 at build time — and `sort(filter(…), Attr)` made mxbuild abort outright, because the sort attribute resolves against the now-absent list's entity. One statement each: `$approved = FILTER($reqs, …); $n = COUNT($approved);`. `mxcli check` now refuses the nested form as MDL-LISTOP02 (mendixlabs/mxcli#1101). +4. **Use append logic when merging**, not overwrite: `$Existing/Field + '\n' + $New/Field` inside an `if $New/Field != empty` guard. +5. **`retrieve … limit 1` binds a single OBJECT, not a one-element list** — it is Mendix's "First object" range, so `head()`, `count()` or a `loop` over that variable is **CE0097**. Drop the `limit` for a list; `limit 1 offset n` and every other `limit` ARE lists. Note the same word means the opposite on `import from mapping`, where `first` binds the object and `limit 1` a one-element list. `describe` re-emits `limit 1` either way, so the source of an object retrieve and a list retrieve are identical text and only MDL-RETRIEVE01 distinguishes them before a build (mendixlabs/mxcli#1103). +6. **Read `.claude/skills/patterns-data-processing.md`** for delta merge, batch processing, and list operation patterns. **Always validate before presenting to user:** ```bash @@ -848,7 +850,7 @@ Full syntax tables for all MDL statements (microflows, pages, security, navigati - LSP server with hover, go-to-definition, completion, diagnostics, symbols, folding - VS Code extension (`vscode-mdl`) with context menu commands (Run/Check/Selection) - Docker build integration (`mxcli docker build`) with PAD patching (Phase 1) -- Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`): local test runs go through a **token-guarded HTTP endpoint** registered by a generated Java custom request handler, instead of compiling the suite into the project's after-startup microflow. Boot registers the endpoint and then **chains the project's own after-startup microflow**, so tests see the app as it really boots (`--skip-app-startup` opts out) — without that, a suite depending on startup state passed under `--attach` and failed under `--local`. One microflow per test, resolved by name at request time from `Core.getMicroflowNames()` and invoked with `Core.microflowCall(...).execute(...)` — so a throwing test fails only itself (not the boot), results are returned rather than scraped from the runtime log, and each test has its own variable scope. Owning the `IContext` is also what finally makes **`@cleanup rollback`** (the annotation's documented default, previously parsed and ignored) real: the handler wraps the call in `startTransaction()`/`rollbackTransaction()`, so a test's writes do not survive it — verified against Postgres, with `@cleanup none` as the in-run control. A rollback that fails is reported per test and summarised, never silent; an unknown strategy is a parse error. The handler **survives `reload_model`** (after-startup does not re-run, the JVM is unchanged), which is what makes `--watch` possible: ~30s first run, then ~2s from an edit — to a test *or* to the microflow under test — to a verdict. `--attach` skips even that boot by running against an app already up under `run --local --test-endpoint`, driving that process's serve + admin APIs over loopback; it uses **that app's database**, only ever adds/removes its own test microflows, and refuses a change needing a restart. Security: the handler is **not registered at all** without `MXCLI_TEST_TOKEN` in the runtime env (so a project that kept the `MxTest` module through a failed cleanup is inert in production), the token is constant-time compared, non-loopback callers are refused, `/list` is clamped to the test namespace, and only `MxTest.Test_*` may be invoked. The token reaches the runtime via its environment and is never written into the project. Docker keeps the after-startup runner (`--legacy-runner` selects it locally). Packages: `cmd/mxcli/testrunner/` (`endpoint.go`, `client.go`, `watch.go`, `host.go`, `handshake.go`). See `docs/15-testing/SPIKE_test_endpoint_request_handler.md` +- Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`): local test runs go through a **token-guarded HTTP endpoint** registered by a generated Java custom request handler, instead of compiling the suite into the project's after-startup microflow. Boot registers the endpoint and then **chains the project's own after-startup microflow**, so tests see the app as it really boots (`--skip-app-startup` opts out) — without that, a suite depending on startup state passed under `--attach` and failed under `--local`. One microflow per test, resolved by name at request time from `Core.getMicroflowNames()` and invoked with `Core.microflowCall(...).execute(...)` — so a throwing test fails only itself (not the boot), results are returned rather than scraped from the runtime log, and each test has its own variable scope. Owning the `IContext` is also what finally makes **`@cleanup rollback`** (the annotation's documented default, previously parsed and ignored) real: the handler wraps the call in `startTransaction()`/`rollbackTransaction()`, so a test's writes do not survive it — verified against Postgres, with `@cleanup none` as the in-run control. A rollback that fails is reported per test and summarised, never silent; an unknown strategy is a parse error. The handler **survives `reload_model`** (after-startup does not re-run, the JVM is unchanged), which is what makes `--watch` possible: ~30s first run, then ~2s from an edit — to a test *or* to the microflow under test — to a verdict. `--attach` skips even that boot by running against an app already up under `run --local --test-endpoint`, driving that process's serve + admin APIs over loopback; it uses **that app's database**, only ever adds/removes its own test microflows, and refuses a change needing a restart. Security: the handler is **not registered at all** without `MXCLI_TEST_TOKEN` in the runtime env (so a project that kept the `MxTest` module through a failed cleanup is inert in production), the token is constant-time compared, non-loopback callers are refused, `/list` is clamped to the test namespace, and only `MxTest.Test_*` may be invoked. The token reaches the runtime via its environment and is never written into the project. A `.test.mdl` file is **checkable**: each block is a microflow body, so `mxcli check` (and the LSP, hence VS Code) renders the blocks as the microflows they become, on the file's own lines — before #1103 the top-level grammar was applied instead, and since `RETRIEVE` is a non-reserved keyword the leftover `FROM …` started an OQL query, so the reader was told their retrieve needed a SELECT; 9 of this repo's 10 test files reported errors that way, one of them 392. `make check-mdl` now sweeps them, with `.fail.test.mdl` for a file whose annotations are deliberately unusable. Two things a failed run must not do, both reported as #1104: **a rejected build is reported with MxBuild's own errors** — `BuildResult.ErrorSummary()` was in hand and discarded by `fmt.Errorf("build failed: %s", build.Message)` on the `--attach` and rebuild paths, and that sentence is identical for every failing build, so it could not tell "your test does not compile" from "an unrelated document is broken"; and **cleanup removes every generated `MxTest.Test_*` the project holds**, not just this suite's. The names are positional and every file reuses them, so keying cleanup on the suite left a flow behind whenever a later run had fewer tests than an earlier one — and one leftover that does not build fails every later run of every test file. What cleanup could not remove is named, with the `DROP` that removes it. Docker keeps the after-startup runner (`--legacy-runner` selects it locally). Packages: `cmd/mxcli/testrunner/` (`endpoint.go`, `client.go`, `watch.go`, `host.go`, `handshake.go`, `check_source.go`, `cleanup_leftovers.go`). See `docs/15-testing/SPIKE_test_endpoint_request_handler.md` - Warm local dev loop (`mxcli run --local [--watch] [--screenshot]`): Docker-free `mxbuild --serve` + standalone runtime, hot `reload_model` for behavioural changes and restart+DDL for structural ones (chosen from the serve build's `restartRequired`). Bundles the browser client (`web/dist/` via mxbuild's rollup runner, which the serve Deploy target skips) so Mendix 11.x apps render in a browser. `--watch` keeps an incremental rollup bundler hot (CHOKIDAR_USEPOLLING for container fs; ~3-4s page re-bundle, skipped for model-only edits) and watches only model source (`.mpr`+`mprcontents/`). `--ensure-db` provisions the local Postgres + app database if missing; `--setup` does the non-blocking prerequisites (cache mxbuild+runtime, ensure DB) and exits — `mxcli init` wires it into a Claude Code SessionStart hook so a fresh/reaped web session self-bootstraps, and `docs-site/src/tools/bootstrap-prompt.md` is the empty-repo seed prompt. `--screenshot` captures a Playwright PNG each change (pixel-perfect page loop), with `--screenshot-url` deep links (repeatable for multi-page sets, one PNG per page) and `--screenshot-user`/`--screenshot-password` form login (session saved as Playwright storage state, reused via `screenshot --load-storage`). See `docs/11-proposals/PROPOSAL_mxcli_dev_warm_loop.md` - External browser preview (`mxcli run --hub ` + `mxcli tunnel-hub`): the app stays local and reverse-tunnels out over a single 443 connection (embedded chisel) to a static relay, so it is reachable in a browser at a public URL — works from egress-only environments (Claude Code web), verified live through the session's MITM egress proxy. `run --hub` implies `--local`, boots the runtime with `ApplicationRootUrl` set to the assigned URL (so the SPA/`originURI` work under the public origin), resolves the control proxy honouring `NO_PROXY`, and retries forever. `mxcli tunnel-hub --domain ` is the **multi-tenant** relay: a registry keyed by prefix/project/solution/branch/worktree (stable URLs on reconnect) fronts many previews at per-subdomain hosts (`[prefix-]project[-branch].`; main collapses to the project) over one 443 with per-subdomain autocert, a registration API (`/api/register|status|deregister|backends|sessions`), and an availability overview at `hub./` **grouped by Claude Code session** (`/api/sessions`): each session lists the endpoints it exposed and links back to its `claude.ai/code` conversation. Client identity flags: `--hub-prefix`/`--hub-project`/`--hub-solution`/`--hub-branch`/`--hub-worktree` (project + branch auto-detected); `--hub-session` groups a session's endpoints (auto-detected from `CLAUDE_CODE_REMOTE_SESSION_ID`). Past sessions are retained: a durable per-session endpoint history (`--sessions-file`, default `~/.mxcli/hub-sessions.json`) survives restarts and reaping, and is pruned after `--session-retention` (default 30d) — so the overview shows offline sessions too (`SessionLog` in `cmd/mxcli/tunnelhub/sessions.go`). Package: `cmd/mxcli/tunnelhub/`. See `docs/11-proposals/PROPOSAL_mxcli_dev_warm_loop.md` (slices 3–4) - Tunnel-hub GitHub authentication (opt-in, gated on `--github-oauth-client-id`; absent = today's open hub): **viewer plane** — GitHub OAuth web flow + HMAC-signed SSO session cookie (`Domain=.`), owner-checked previews (`--require-auth` default on → 302 to login / 403 non-owner; soft mode filters the listing only), `/api/backends` filtered to the viewer (unauthenticated → 401), admin "signed in as" via `/api/whoami`. **Registration plane** — durable, hashed hub API keys (`--keys-file`, default `~/.mxcli/hub-keys.json`, survive restarts) presented as `X-Hub-Key` → stamps `Backend.Owner`; shared `X-Hub-Secret` still works as an owner-less fallback. **Key issuance** — the hub's `/cli` browser page mints a key from the session cookie (no PAT; the device flow was removed as Claude Code containers block GitHub's device endpoints), rotate-by-default + count + revoke-all; `mxcli auth hub login --token ` is the headless path; `run --hub` reads `MXCLI_HUB_KEY` (env → `~/.mxcli/auth.json`) and degrades to local-only if registration fails. Append-only JSONL audit trail (`--audit-log`, no secrets). Packages: `cmd/mxcli/tunnelhub/` (+`audit/`), `cmd/mxcli/hubauth/`. See `docs/11-proposals/PROPOSAL_hub_authentication.md` diff --git a/Makefile b/Makefile index 4bf9b66553..533490c103 100644 --- a/Makefile +++ b/Makefile @@ -183,6 +183,16 @@ test: grammar sync-all # rule rejects). The runner inverts the exit code for these: an unexpected # pass is treated as a regression of the rule. # +# A test file whose ANNOTATIONS are deliberately unusable is a negative test like +# any other and is named `.fail.test.mdl`; both fixtures of that kind exist to +# prove the runner reports an ERROR rather than a PASS. +# +# `.test.mdl` files are swept too. They used to be skipped because `check` could +# not parse one at all — a test block is a microflow body, not a top-level +# statement, so every test file reported errors about the grammar rather than +# about itself (mendixlabs/mxcli#1103). Now that `check` renders them, the sweep +# is what keeps that true. +# # `check` runs here WITHOUT a project, so only CHECK-TIME rules can be tested # this way. A guard living in the executor or a backend needs a model before it # can decide anything, so its repro is valid MDL, `check` exits 0, and naming @@ -192,7 +202,6 @@ test: grammar sync-all check-mdl: build @FAILED=0; \ for f in mdl-examples/doctype-tests/*.mdl mdl-examples/bug-tests/*.mdl; do \ - case "$$f" in *.test.mdl) continue ;; esac; \ case "$$f" in \ */116-datagrid2-column-name-mismatch.mdl|\ */343-list-attribute-find-filter.mdl|\ @@ -207,7 +216,7 @@ check-mdl: build continue ;; \ esac; \ NAME=$$(basename "$$f"); \ - case "$$f" in *.fail.mdl) \ + case "$$f" in *.fail.mdl|*.fail.test.mdl) \ if ./$(BUILD_DIR)/$(BINARY_NAME) check "$$f" > /dev/null 2>&1; then \ echo "FAIL (negative test unexpectedly passed): $$NAME"; \ FAILED=1; \ diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 4864bd5075..db2e19a84c 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -7,6 +7,7 @@ import ( "os" "strings" + "github.com/mendixlabs/mxcli/cmd/mxcli/testrunner" "github.com/mendixlabs/mxcli/mdl/executor" "github.com/mendixlabs/mxcli/mdl/linter" "github.com/mendixlabs/mxcli/mdl/visitor" @@ -115,7 +116,31 @@ Examples: if !isStructured { fmt.Printf("Checking syntax: %s\n", mdlSourceLabel(filePath)) } - prog, errs := visitor.Build(string(content)) + + // A .test.mdl / .test.md file is not top-level MDL: each block is a + // microflow body. Render it as the microflows it becomes, on the source's + // own lines, so every rule below applies to what the author actually wrote + // (mendixlabs/mxcli#1103). + source := string(content) + var testProblems []linter.Violation + if testrunner.IsTestFile(filePath) { + checked, terr := testrunner.CheckSource(source, filePath) + if terr != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", terr) + os.Exit(1) + } + source = checked.MDL + for _, p := range checked.Problems { + testProblems = append(testProblems, linter.Violation{ + RuleID: "MDL-TEST01", + Severity: linter.SeverityError, + Message: fmt.Sprintf("test %q: %s", p.Test, p.Message), + Location: linter.Location{DocumentType: "test", DocumentName: p.Test}, + }) + } + } + + prog, errs := visitor.Build(source) if len(errs) > 0 { if isStructured { var parseViolations []linter.Violation @@ -133,7 +158,7 @@ Examples: fmt.Fprintf(os.Stderr, " - %v\n", err) } // Hint: if script contains IMPORT/QUERY with single $ but not $$, suggest dollar-quoting - src := string(content) + src := source if (strings.Contains(src, "IMPORT") || strings.Contains(src, "import")) && (strings.Contains(src, "QUERY") || strings.Contains(src, "query")) && strings.Contains(src, "$") && !strings.Contains(src, "$$") { @@ -150,7 +175,7 @@ Examples: // Every semantic check lives in executor.ValidateProgram, so `mxcli exec` // refuses exactly what `mxcli check` reports. Adding a check there gives // both commands it at once. - violations := executor.ValidateProgram(prog, projectPath) + violations := append(testProblems, executor.ValidateProgram(prog, projectPath)...) if isStructured { // Always emit structured output (even when clean) diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index 0950e772ec..058bee2d54 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -249,7 +249,10 @@ func (a *LocalApp) Rebuild(projectPath string) (ApplyAction, *BuildResult, error return ActionReload, nil, err } if !build.OK() { - return ActionReload, build, fmt.Errorf("build failed: %s", build.Message) + // The same error type the cold boot returns, so a caller can attribute the + // problems rather than re-parse a sentence. Message alone is identical for + // every failing build (mendixlabs/mxcli#1104). + return ActionReload, build, &BuildFailedError{Result: build} } action, err := a.Runtime.Controller().ApplyBuild(build, a.Runtime.Restart) return action, build, err diff --git a/cmd/mxcli/lsp_diagnostics.go b/cmd/mxcli/lsp_diagnostics.go index 5c42544ea6..7dc7f535a2 100644 --- a/cmd/mxcli/lsp_diagnostics.go +++ b/cmd/mxcli/lsp_diagnostics.go @@ -8,6 +8,7 @@ import ( "strconv" "strings" + "github.com/mendixlabs/mxcli/cmd/mxcli/testrunner" "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/executor" "github.com/mendixlabs/mxcli/mdl/linter" @@ -19,6 +20,46 @@ import ( // errLineRegexp parses error messages in the format "line N:M msg". var errLineRegexp = regexp.MustCompile(`^line (\d+):(\d+) (.+)$`) +// checkableDocument renders a document as the MDL to parse, plus the diagnostics +// that come from the document's own format rather than from the grammar. +// +// The extension binds the MDL language to `.mdl`, which `.test.mdl` matches, so +// until now every test file in the editor was diagnosed against the top-level +// grammar it is not written in — a wall of squiggles saying nothing true +// (mendixlabs/mxcli#1103). A test block is a microflow body; testrunner renders +// it as one, on the same lines, so the positions below need no adjustment. +func checkableDocument(docURI uri.URI, text string) (string, []protocol.Diagnostic) { + path := docURI.Filename() + if !testrunner.IsTestFile(path) { + return text, nil + } + checked, err := testrunner.CheckSource(text, path) + if err != nil { + // The file is not a usable test file at all. Report that, at the top, + // rather than whatever the grammar makes of it. + return "", []protocol.Diagnostic{{ + Range: protocol.Range{Start: protocol.Position{Line: 0}, End: protocol.Position{Line: 0}}, + Severity: protocol.DiagnosticSeverityError, + Source: "mdl-test", + Message: err.Error(), + }} + } + var diags []protocol.Diagnostic + for _, p := range checked.Problems { + line := uint32(0) + if p.Line > 0 { + line = uint32(p.Line - 1) + } + diags = append(diags, protocol.Diagnostic{ + Range: protocol.Range{Start: protocol.Position{Line: line}, End: protocol.Position{Line: line}}, + Severity: protocol.DiagnosticSeverityError, + Source: "mdl-test", + Message: p.Message, + }) + } + return checked.MDL, diags +} + // parseMDLDiagnostics runs the MDL parser on text and converts errors to LSP diagnostics. func parseMDLDiagnostics(text string) []protocol.Diagnostic { _, errs := visitor.Build(text) @@ -60,7 +101,8 @@ func parseMDLDiagnostics(text string) []protocol.Diagnostic { // publishDiagnostics parses the document and sends diagnostics to the client. func (s *mdlServer) publishDiagnostics(ctx context.Context, docURI uri.URI, text string) { - diags := parseMDLDiagnostics(text) + text, diags := checkableDocument(docURI, text) + diags = append(diags, parseMDLDiagnostics(text)...) // If no parse errors, run semantic validation inline if len(diags) == 0 { diags = append(diags, s.runSemanticValidation(text)...) @@ -130,7 +172,8 @@ func (s *mdlServer) DidSave(ctx context.Context, params *protocol.DidSaveTextDoc s.mu.Unlock() // If there are parse errors, don't run semantic checks - if diags := parseMDLDiagnostics(text); len(diags) > 0 { + checkable, diags := checkableDocument(docURI, text) + if len(diags) > 0 || len(parseMDLDiagnostics(checkable)) > 0 { return nil } diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index b96ed8a90e..f32ca52817 100644 --- a/cmd/mxcli/syntax/features_domain_model.go +++ b/cmd/mxcli/syntax/features_domain_model.go @@ -241,7 +241,7 @@ func init() { "caption", "show enumerations", "describe enumeration", "drop enumeration", }, - Syntax: "CREATE ENUMERATION Module.Name (\n ValueName 'Display Caption',\n ...\n);\n\nALTER ENUMERATION Module.Name ADD VALUE [IF NOT EXISTS] NewValue [CAPTION 'Display Caption'];\nALTER ENUMERATION Module.Name RENAME VALUE OldName TO NewName;\nALTER ENUMERATION Module.Name MODIFY VALUE ValueName CAPTION 'New Caption';\nALTER ENUMERATION Module.Name DROP VALUE [IF EXISTS] ValueName;\n\nIF NOT EXISTS / IF EXISTS make a script RE-RUNNABLE. Without them the second\nrun errors and exec STOPS THERE, so one already-present value leaves every\nlater statement unapplied. A defensive drop-then-add is not a substitute: the\ndrop fails when the value is absent and the add when it is present.\n\nSHOW ENUMERATIONS;\nSHOW ENUMERATIONS IN ;\nDESCRIBE ENUMERATION Module.Name;\nDROP ENUMERATION Module.Name;\n\nUsing in entity:\n AttrName: Enumeration(Module.EnumName)", + Syntax: "CREATE ENUMERATION Module.Name (\n ValueName 'Display Caption',\n ...\n);\n\nALTER ENUMERATION Module.Name ADD VALUE [IF NOT EXISTS] NewValue [CAPTION 'Display Caption'];\nALTER ENUMERATION Module.Name RENAME VALUE OldName TO NewName;\nALTER ENUMERATION Module.Name MODIFY VALUE ValueName CAPTION 'New Caption';\nALTER ENUMERATION Module.Name DROP VALUE [IF EXISTS] ValueName;\n\nIF NOT EXISTS / IF EXISTS make a script RE-RUNNABLE. Without them the second\nrun errors and exec STOPS THERE, so one already-present value leaves every\nlater statement unapplied. A defensive drop-then-add is not a substitute: the\ndrop fails when the value is absent and the add when it is present.\n\nSHOW ENUMERATIONS;\nSHOW ENUMERATIONS IN ;\nDESCRIBE ENUMERATION Module.Name;\nDROP ENUMERATION Module.Name;\n\nUsing in entity:\n AttrName: Enumeration(Module.EnumName)\n\nThe System module's enumerations are platform built-ins with no stored\nunit. SHOW ENUMERATIONS and DESCRIBE ENUMERATION report them anyway, so\ntheir values can be read instead of guessed at until the build rejects one\nwith CE1613. They are READ-ONLY: DESCRIBE prints them as -- comment lines,\nand CREATE / ALTER / DROP / MOVE naming System is refused.\n mxcli -p app.mpr describe enumeration System.WorkflowActivityType", Example: "CREATE ENUMERATION MyModule.OrderStatus (\n Pending 'Pending Approval',\n Processing 'Being Processed',\n Shipped 'Shipped to Customer'\n);\n\n-- Using in an entity\nCREATE PERSISTENT ENTITY MyModule.Order (\n OrderNumber: String(20) NOT NULL,\n Status: Enumeration(MyModule.OrderStatus)\n);", SeeAlso: []string{"domain-model.enumeration", "domain-model.entity.attributes"}, }) diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index 6b46e15ad0..bd8198162b 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -66,8 +66,20 @@ func init() { }, // Retrieve-by-association was missing here, so it read as unsupported // even though it works and the write-microflows skill documents it. - Syntax: "-- From the database\nRETRIEVE $Var FROM Module.Entity\n [WHERE condition]\n [SORT BY attr ASC|DESC]\n [LIMIT n] [OFFSET n];\n\n-- Over an association, from an object you already have\nRETRIEVE $Var FROM $Object/Module.Association;", - Example: "RETRIEVE $Customer FROM MyModule.Customer\n WHERE Code = $CustomerCode\n LIMIT 1;\n\nRETRIEVE $Orders FROM MyModule.Order\n WHERE Status = 'Pending'\n SORT BY CreateDate DESC\n LIMIT 10 OFFSET 0;\n\n-- Follow an association rather than querying the database\nRETRIEVE $Orders FROM $Customer/MyModule.Order_Customer;\nRETRIEVE $Customer FROM $Order/MyModule.Order_Customer;", + Syntax: "-- From the database\nRETRIEVE $Var FROM Module.Entity\n [WHERE condition]\n [SORT BY attr ASC|DESC]\n [LIMIT n] [OFFSET n];\n\n-- Over an association, from an object you already have\nRETRIEVE $Var FROM $Object/Module.Association;", + Example: "-- LIMIT 1 binds a single OBJECT (Mendix's \"First object\" range), not a\n" + + "-- one-element list — hence the singular variable name here.\n" + + "RETRIEVE $Customer FROM MyModule.Customer\n WHERE Code = $CustomerCode\n LIMIT 1;\n\n" + + "-- Any other LIMIT is a bounded range, which is a list.\n" + + "RETRIEVE $Orders FROM MyModule.Order\n WHERE Status = 'Pending'\n SORT BY CreateDate DESC\n LIMIT 10 OFFSET 0;\n\n" + + "-- Follow an association rather than querying the database\nRETRIEVE $Orders FROM $Customer/MyModule.Order_Customer;\nRETRIEVE $Customer FROM $Order/MyModule.Order_Customer;\n\n" + + "-- Notes:\n" + + "-- * LIMIT 1 with no OFFSET is the one form that binds an object. HEAD(),\n" + + "-- COUNT() or a LOOP over it is CE0097 at build time; mxcli reports it as\n" + + "-- MDL-RETRIEVE01 at check time.\n" + + "-- * LIMIT 1 OFFSET n is a bounded range, so that one IS a list.\n" + + "-- * `import from mapping … limit 1` means the opposite — a one-element\n" + + "-- list — and `… first` is its object form.", SeeAlso: []string{"microflow.object-operations", "xpath"}, }) @@ -305,8 +317,8 @@ func init() { // debugging the build error back to the right topic (issue #1002). "$currentObject", "predicate", "CE0117", "CE0109", "MDL-LISTOP01", }, - Syntax: "$List = CREATE LIST OF Module.Entity;\nADD $Item TO $List;\nREMOVE $Item FROM $List;\n$Result = HEAD($List);\n$Result = TAIL($List);\n$Result = FIND($List, predicate);\n$Result = FILTER($List, predicate);\n$Result = SORT($List, attr ASC);\n$Result = UNION($L1, $L2);\n$Result = INTERSECT($L1, $L2);\n$Result = SUBTRACT($L1, $L2);\n$Result = RANGE($List, offset, amount);\n$Result = RANGE($List, offset);\n\n-- Aggregates. Mendix has eight; each takes an attribute or an expression\n-- over $currentObject.\n$Count = COUNT($List);\n$Sum = SUM($List.Attr);\n$Sum = SUM($List, expression);\n$Avg = AVERAGE($List.Attr);\n$Min = MINIMUM($List.Attr);\n$Max = MAXIMUM($List.Attr);\n$AllMatch = ALL($List, boolean-expression);\n$AnyMatch = ANY($List, boolean-expression);\n\n-- REDUCE folds the list into one value. $currentResult is the running\n-- total; both INITIAL and RETURNS are required and cannot be inferred.\n$Folded = REDUCE($List, expression, initial: value, returns: Type);\n\n-- RANGE takes OFFSET first, then AMOUNT, and needs at least ONE of them:\n-- RANGE($L, $Offset, $Amount) page: skip $Offset, take $Amount\n-- RANGE($L, 0, $Amount) first $Amount\n-- RANGE($L, $Offset) skip $Offset, take the rest\n-- RANGE($L) with no bound is CE6520 at build time (mxcli check: MDL068).\n\nA FIND/FILTER predicate is evaluated once per item, and Mendix binds the\nitem to $currentObject -- the same variable the aggregate expressions above\nuse, and the only iterator name there is:\n\n FILTER($Orders, $currentObject/Amount > 0)\n\nA bare attribute name means the same thing; mxcli resolves it against the\nlist's entity and writes $currentObject/Attr. A name that is not a member\nof that entity is refused, and naming any other variable is MDL-LISTOP01.\n\nSORT is not an expression -- it takes attribute names directly, so a bare\nattribute is the only spelling there.", - Example: "$AllOrders = CREATE LIST OF MyModule.Order;\nADD $NewOrder TO $AllOrders;\n$First = HEAD($AllOrders);\n\n-- The item under test is $currentObject\n$Pending = FILTER($AllOrders, $currentObject/Status = 'Pending');\n$Large = FILTER($AllOrders, $currentObject/Amount > 1000);\n\n-- A bare attribute name is resolved against the list's entity\n$Open = FILTER($AllOrders, Status != 'Closed');\n\n-- SORT takes attribute names, not an expression\n$Sorted = SORT($Pending, CreateDate DESC);\n$Page = RANGE($Sorted, $Offset, $PageSize);\n$Total = SUM($AllOrders.Amount);\n$AllPaid = ALL($AllOrders, $currentObject/Paid);\n$AnyLate = ANY($AllOrders, $currentObject/DueDate < [%CurrentDateTime%]);\n$Discounted = REDUCE(\n $AllOrders,\n $currentResult + $currentObject/Amount * 0.9,\n initial: 0,\n returns: Decimal\n);", + Syntax: "$List = CREATE LIST OF Module.Entity;\nADD $Item TO $List;\nREMOVE $Item FROM $List;\n$Result = HEAD($List);\n$Result = TAIL($List);\n$Result = FIND($List, predicate);\n$Result = FILTER($List, predicate);\n$Result = SORT($List, attr ASC);\n$Result = UNION($L1, $L2);\n$Result = INTERSECT($L1, $L2);\n$Result = SUBTRACT($L1, $L2);\n$Result = RANGE($List, offset, amount);\n$Result = RANGE($List, offset);\n\n-- Aggregates. Mendix has eight; each takes an attribute or an expression\n-- over $currentObject.\n$Count = COUNT($List);\n$Sum = SUM($List.Attr);\n$Sum = SUM($List, expression);\n$Avg = AVERAGE($List.Attr);\n$Min = MINIMUM($List.Attr);\n$Max = MAXIMUM($List.Attr);\n$AllMatch = ALL($List, boolean-expression);\n$AnyMatch = ANY($List, boolean-expression);\n\n-- REDUCE folds the list into one value. $currentResult is the running\n-- total; both INITIAL and RETURNS are required and cannot be inferred.\n$Folded = REDUCE($List, expression, initial: value, returns: Type);\n\n-- RANGE takes OFFSET first, then AMOUNT, and needs at least ONE of them:\n-- RANGE($L, $Offset, $Amount) page: skip $Offset, take $Amount\n-- RANGE($L, 0, $Amount) first $Amount\n-- RANGE($L, $Offset) skip $Offset, take the rest\n-- RANGE($L) with no bound is CE6520 at build time (mxcli check: MDL068).\n\nA FIND/FILTER predicate is evaluated once per item, and Mendix binds the\nitem to $currentObject -- the same variable the aggregate expressions above\nuse, and the only iterator name there is:\n\n FILTER($Orders, $currentObject/Amount > 0)\n\nA bare attribute name means the same thing; mxcli resolves it against the\nlist's entity and writes $currentObject/Attr. A name that is not a member\nof that entity is refused, and naming any other variable is MDL-LISTOP01.\n\nSORT is not an expression -- it takes attribute names directly, so a bare\nattribute is the only spelling there.\n\nEvery list operation above is a separate ACTIVITY, and an activity stores\nits list as a VARIABLE. They do not nest: COUNT(FILTER($L, ...)) is not a\nshorter spelling of two statements, it is a list argument Mendix cannot\nstore. mxcli refuses it as MDL-LISTOP02; give the inner operation its own\nstatement and pass the variable:\n\n $Approved = FILTER($Orders, $currentObject/Status = 'Approved');\n $Count = COUNT($Approved);", + Example: "$AllOrders = CREATE LIST OF MyModule.Order;\nADD $NewOrder TO $AllOrders;\n$First = HEAD($AllOrders);\n\n-- The item under test is $currentObject\n$Pending = FILTER($AllOrders, $currentObject/Status = 'Pending');\n$Large = FILTER($AllOrders, $currentObject/Amount > 1000);\n\n-- A bare attribute name is resolved against the list's entity\n$Open = FILTER($AllOrders, Status != 'Closed');\n\n-- SORT takes attribute names, not an expression\n$Sorted = SORT($Pending, CreateDate DESC);\n$Page = RANGE($Sorted, $Offset, $PageSize);\n$Total = SUM($AllOrders.Amount);\n$AllPaid = ALL($AllOrders, $currentObject/Paid);\n$AnyLate = ANY($AllOrders, $currentObject/DueDate < [%CurrentDateTime%]);\n\n-- List operations do not nest -- one statement each (MDL-LISTOP02)\n-- WRONG: $Count = COUNT(FILTER($AllOrders, $currentObject/Paid));\n$Paid = FILTER($AllOrders, $currentObject/Paid);\n$Count = COUNT($Paid);\n$Discounted = REDUCE(\n $AllOrders,\n $currentResult + $currentObject/Amount * 0.9,\n initial: 0,\n returns: Decimal\n);", SeeAlso: []string{"microflow.retrieve"}, }) diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index acd97a1e4c..0d8e938b64 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -127,7 +127,16 @@ CREATE PAGE Sales.Detail (Title: 'Detail', Layout: Atlas_Core.Atlas_Default) { "-- Data grid 2 column filters go INSIDE the column's own braces\nDATAGRID dg (...) { COLUMN c (Attribute: A) { TEXTFILTER tf (Attribute: A) } }\nTEXTFILTER | NUMBERFILTER | DATEFILTER | DROPDOWNFILTER | DROPDOWNSORT\n" + "-- Match the filter to the column's type, or MxBuild refuses it: String ->\n" + "-- TEXTFILTER, Integer/Long/Decimal -> NUMBERFILTER, Date and time -> DATEFILTER,\n" + - "-- Enumeration -> DROPDOWNFILTER. A Boolean column takes no filter at all.\n" + + "-- Enumeration AND Boolean -> DROPDOWNFILTER (the drop-down filter's own\n" + + "-- attribute types are Enum and Boolean; a Boolean column filters Yes/No).\n" + + "-- A column may carry BOTH a custom-content widget and a filter — `content`\n" + + "-- and `filter` are separate slots, so a read-only CHECKBOX cell and a\n" + + "-- DROPDOWNFILTER live in the same braces:\n" + + "DATAGRID dg (...) { COLUMN Active (Attribute: IsActive) {\n" + + " CHECKBOX cb (Attribute: IsActive, Editable: Never, ReadOnlyStyle: Control)\n" + + " DROPDOWNFILTER ddf } }\n" + + "-- ReadOnlyStyle (Inherit | Control | Text) is what makes a read-only check\n" + + "-- box render as the checkbox glyph instead of the text Yes/No.\n" + "-- The grid-wide filter bar is CONTROLBAR; a GALLERY spells that same slot\n" + "-- FILTER, so `FILTER f { ... }` belongs to a gallery and not to a datagrid:\n" + "GALLERY g (...) { FILTER f { TEXTFILTER tf (Attribute: A) } }\n" + diff --git a/cmd/mxcli/testrunner/build_attribution.go b/cmd/mxcli/testrunner/build_attribution.go index b67523e37a..f49a66b335 100644 --- a/cmd/mxcli/testrunner/build_attribution.go +++ b/cmd/mxcli/testrunner/build_attribution.go @@ -25,13 +25,51 @@ package testrunner import ( + "errors" "fmt" "regexp" "strings" + "time" "github.com/mendixlabs/mxcli/cmd/mxcli/docker" ) +// buildFailure is the error every path in this package returns for a build +// MxBuild rejected. +// +// The type is load-bearing, not decoration. MxBuild puts the same sentence in +// Message for every failing build — "The project cannot be deployed, because it +// contains errors." — so an error carrying only that cannot tell "your test does +// not compile" from "an unrelated document in the project is broken". Two paths +// built their error with fmt.Errorf and threw the parsed problems away, which is +// the whole of mendixlabs/mxcli#1104's first half: the detail was already in +// hand and discarded one line before it was needed. +func buildFailure(build *docker.BuildResult) error { + return &docker.BuildFailedError{Result: build} +} + +// resultsForBuildFailure turns a failed build into something the reader can act +// on, and passes any other error through untouched. +// +// A build error belonging to a generated test microflow is that test's problem: +// it becomes an ERROR row and the rest become SKIP. Anything else is in the +// project, and the returned error names the documents so the reader is not sent +// looking through their own model for a message that came from mxcli's. +// +// Shared by both runners on purpose. It was wired into the --local boot only, so +// --attach and every rebuild under --watch reported the bare sentence. +func resultsForBuildFailure(err error, suite *TestSuite) (*SuiteResult, error) { + var bf *docker.BuildFailedError + if !errors.As(err, &bf) { + return nil, err + } + if results := resultsFromFailedBuild(bf.BuildErrors(), suite); results != nil { + return &SuiteResult{Name: suite.Name, Tests: results, Started: time.Now()}, nil + } + _, other := attributeBuildProblems(bf.BuildErrors(), suite) + return nil, fmt.Errorf("%w%s", err, buildFailureHint(other)) +} + // testFlowDocumentPattern matches the `document` MxBuild reports for a generated // test microflow. // @@ -138,17 +176,51 @@ func resultsFromFailedBuild(problems []docker.BuildProblem, suite *TestSuite) [] // buildFailureHint is appended to the error when a build failure could not be // attributed to any test, which means it is in the project rather than in the // suite. +// +// The errors themselves are already rendered by BuildFailedError.Error(), so +// this says what the reader cannot work out from them: that none of them belongs +// to a test in this run, and — for a generated microflow no current test owns — +// that it is a leftover from an earlier run, with the command to remove it. That +// last case is the one that fails every subsequent run of every test file until +// someone finds it by hand (mendixlabs/mxcli#1104). func buildFailureHint(other []docker.BuildProblem) string { if len(other) == 0 { return "" } var b strings.Builder - b.WriteString("\n The build errors are in the project, not in the tests:") - for _, p := range other { - b.WriteString(fmt.Sprintf("\n %s %s", p.ErrorCode, p.Message)) - if w := p.Where(); w != "" { - b.WriteString(" — at " + w) - } + b.WriteString("\n These errors are in the project, not in this run's tests.") + for _, name := range leftoverFlowsIn(other) { + b.WriteString(fmt.Sprintf( + "\n %s is a microflow an earlier `mxcli test` run left behind. Remove it with:"+ + "\n mxcli -p -c \"DROP MICROFLOW %s\"", name, name)) } return b.String() } + +// leftoverFlowsIn names the generated test microflows among a set of build +// problems, deduplicated and in the order MxBuild reported them. +// +// A document matching the generated prefix, in the generated module, that no +// test in this run owns, can only have come from an earlier run: the names are +// positional and nothing else in the project is allowed to use them. +func leftoverFlowsIn(problems []docker.BuildProblem) []string { + var names []string + seen := map[string]bool{} + for _, p := range problems { + for _, loc := range p.Locations { + if !strings.EqualFold(loc.Module, mxTestModule) { + continue + } + m := testFlowDocumentPattern.FindStringSubmatch(loc.Document) + if m == nil { + continue + } + name := mxTestModule + "." + m[1] + if !seen[name] { + seen[name] = true + names = append(names, name) + } + } + } + return names +} diff --git a/cmd/mxcli/testrunner/build_attribution_test.go b/cmd/mxcli/testrunner/build_attribution_test.go index ed626abfbc..dd2cec8404 100644 --- a/cmd/mxcli/testrunner/build_attribution_test.go +++ b/cmd/mxcli/testrunner/build_attribution_test.go @@ -130,11 +130,20 @@ func TestResultsFromFailedBuildDeclinesUnattributableErrors(t *testing.T) { t.Fatalf("expected no results for a project-level failure, got %v", results) } - _, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) - hint := buildFailureHint(other) - for _, want := range []string{"in the project, not in the tests", "CE0109", "SUB_Deal"} { - if !strings.Contains(hint, want) { - t.Errorf("hint %q does not mention %q", hint, want) + // Asserted on the whole message the reader sees, not on the hint alone: the + // errors are rendered once by BuildFailedError.Error() and the hint adds only + // what cannot be read off them. Testing the hint in isolation is what made an + // earlier version print every error twice. + _, err := resultsForBuildFailure(buildFailure(failedBuild(p)), suite) + if err == nil { + t.Fatal("err = nil, want the build failure") + } + for _, want := range []string{"in the project, not in this run's tests", "CE0109", "SUB_Deal"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("message %q does not mention %q", err.Error(), want) } } + if strings.Count(err.Error(), "CE0109") != 1 { + t.Errorf("CE0109 is reported %d times, want once:\n%s", strings.Count(err.Error(), "CE0109"), err.Error()) + } } diff --git a/cmd/mxcli/testrunner/build_failure_surfacing_test.go b/cmd/mxcli/testrunner/build_failure_surfacing_test.go new file mode 100644 index 0000000000..61eec25aac --- /dev/null +++ b/cmd/mxcli/testrunner/build_failure_surfacing_test.go @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "errors" + "fmt" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/cmd/mxcli/docker" +) + +// failedBuild is the shape a serve /build response has when MxBuild rejected the +// model: the generic Message, with the real detail only in Problems. +func failedBuild(problems ...docker.BuildProblem) *docker.BuildResult { + return &docker.BuildResult{ + Status: "Failure", + Message: "The project cannot be deployed, because it contains errors.", + Problems: docker.BuildProblems{Problems: problems}, + } +} + +// TestBuildFailureCarriesTheProblems is the guard for mendixlabs/mxcli#1104's +// first half: a rebuild that fails must hand the caller the parsed problems, not +// only the one sentence MxBuild puts in Message. +// +// The sentence is identical for every failing build, so an error carrying only +// that is indistinguishable between "your test does not compile" and "an +// unrelated document in the project is broken" — which is exactly the report. +func TestBuildFailureCarriesTheProblems(t *testing.T) { + build := failedBuild(problem("CE0097", "The selected 'accs' variable must be of type List.", + "MxTest", "Microflow 'Test_test_2'", "List operation activity 'Head'")) + + err := buildFailure(build) + + var bf *docker.BuildFailedError + if !errors.As(err, &bf) { + t.Fatalf("error is %T, want *docker.BuildFailedError — the caller cannot attribute what it cannot inspect", err) + } + if len(bf.BuildErrors()) != 1 { + t.Fatalf("BuildErrors() = %d, want 1", len(bf.BuildErrors())) + } + if msg := err.Error(); !strings.Contains(msg, "CE0097") || !strings.Contains(msg, "Test_test_2") { + t.Errorf("Error() = %q, want the code and the document it was found in", msg) + } +} + +// TestResultsForBuildFailure covers the shared handling both runners now use: a +// build error belonging to a generated test microflow becomes that test's ERROR +// row, and one belonging to the project is reported with the documents named. +func TestResultsForBuildFailure(t *testing.T) { + suite := suiteOf("test_1", "test_2") + + t.Run("an error in a test microflow becomes that test's row", func(t *testing.T) { + err := buildFailure(failedBuild(problem("CE0097", "must be of type List.", + "MxTest", "Microflow 'Test_test_2'", "List operation activity 'Head'"))) + + result, outErr := resultsForBuildFailure(err, suite) + if outErr != nil { + t.Fatalf("outErr = %v, want nil — the run reports per-test rows", outErr) + } + if result == nil { + t.Fatal("result = nil, want one row per test") + } + byID := map[string]TestResult{} + for _, r := range result.Tests { + byID[r.ID] = r + } + if byID["test_2"].Status != StatusError { + t.Errorf("test_2 = %v, want ERROR", byID["test_2"].Status) + } + if byID["test_1"].Status != StatusSkip { + t.Errorf("test_1 = %v, want SKIP — it was never run", byID["test_1"].Status) + } + }) + + t.Run("an error in the project names the document", func(t *testing.T) { + // The leftover case: a generated microflow from an EARLIER run is not in + // this suite, so nothing here can be blamed for it — but the reader still + // has to be told which document to go and remove. + err := buildFailure(failedBuild(problem("CE0097", "must be of type List.", + "MxTest", "Microflow 'Test_test_7'", "List operation activity 'Head'"))) + + result, outErr := resultsForBuildFailure(err, suite) + if result != nil { + t.Errorf("result = %v, want nil — no test in this suite is at fault", result) + } + if outErr == nil { + t.Fatal("outErr = nil, want the build failure") + } + if !strings.Contains(outErr.Error(), "Test_test_7") { + t.Errorf("Error() = %q, want the leftover document named", outErr.Error()) + } + // A Test_* microflow in MxTest that no current test owns can only be a + // leftover from an earlier run. Saying so — and how to remove it — is the + // difference between one command and a hunt through the model. + if !strings.Contains(outErr.Error(), "DROP MICROFLOW MxTest.Test_test_7") { + t.Errorf("Error() = %q, want a runnable DROP for the leftover", outErr.Error()) + } + }) + + t.Run("an error in the user's own model is not called a leftover", func(t *testing.T) { + err := buildFailure(failedBuild(problem("CE0109", "Undefined variable 'x'.", + "Sudoku", "Microflow 'SUB_Deal'", "End event"))) + + _, outErr := resultsForBuildFailure(err, suite) + if outErr == nil { + t.Fatal("outErr = nil, want the build failure") + } + if strings.Contains(outErr.Error(), "DROP MICROFLOW") { + t.Errorf("Error() = %q, must not offer to drop the user's own document", outErr.Error()) + } + }) + + t.Run("a non-build error is passed through untouched", func(t *testing.T) { + want := fmt.Errorf("the runtime is not running") + result, outErr := resultsForBuildFailure(want, suite) + if result != nil || !errors.Is(outErr, want) { + t.Errorf("resultsForBuildFailure(%v) = %v, %v; want nil, the same error", want, result, outErr) + } + }) +} diff --git a/cmd/mxcli/testrunner/check_source.go b/cmd/mxcli/testrunner/check_source.go new file mode 100644 index 0000000000..de4e6be223 --- /dev/null +++ b/cmd/mxcli/testrunner/check_source.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Rendering a .test.mdl file as something `mxcli check` and the LSP can check. +// +// A test file is not a sequence of top-level MDL statements. Each block is a +// MICROFLOW BODY — that is literally what the runner turns it into — so feeding +// one to the top-level grammar produces errors about the grammar rather than +// about the file. Measured on the block in mendixlabs/mxcli#1103: `DECLARE` is +// not a statement, the parser resyncs, `RETRIEVE` is swallowed as a non-reserved +// keyword, and the remaining `FROM …` starts an OQL query whose follow set is +// {GROUP_BY, SELECT, HAVING}. The reader is then told their RETRIEVE needs a +// SELECT — on a statement `mxcli syntax microflow.retrieve` prints as its own +// example. +// +// That is not a niche path. The VS Code extension binds the MDL language to +// `.mdl`, which `.test.mdl` matches, so every test file open in the editor was a +// wall of red squiggles; 9 of the 10 test files in this repository report errors +// this way, one of them 392. +// +// The rendering keeps every body on the line the author wrote it on, by padding +// with blank lines and putting the wrapper on the lines the doc comment and the +// '/' separator occupied. A diagnostic then needs no remapping — which is what +// makes this a translation of the source rather than a second parser for it. +package testrunner + +import ( + "fmt" + "path/filepath" + "strings" +) + +// CheckedSource is a test file rendered for checking. +type CheckedSource struct { + // MDL is one microflow per test block, laid out on the source's own lines. + MDL string + // Problems are the things the MDL cannot carry: annotations that claim to + // assert something and cannot. + Problems []SourceProblem +} + +// SourceProblem is one problem found in a test file's annotations. +type SourceProblem struct { + Line int + Test string + Message string +} + +// IsTestFile reports whether a path is one of the test file formats. +func IsTestFile(name string) bool { return isTestFile(name) } + +// CheckSource renders a test file's blocks as microflows. +// +// It returns an error when the file cannot be parsed as a test file at all — two +// @test comments with no separator between them, say. That is a real problem +// with the file and is reported as itself, rather than as whatever the MDL +// grammar makes of the result. +func CheckSource(content, path string) (CheckedSource, error) { + var tests []TestCase + var err error + if strings.EqualFold(filepath.Ext(path), ".md") { + tests, err = parseMarkdownTests(content, path) + } else { + tests, err = parseMDLTests(content, path) + } + if err != nil { + return CheckedSource{}, err + } + + lines := strings.Split(content, "\n") + // One slot per source line, blank unless something is placed on it. A + // rendered line is only ever the body verbatim or a wrapper fragment, so + // columns survive too. The spare slot is for a closing fragment on a file + // whose last test runs to EOF with no '/' after it; appending past the end + // shifts nothing. + out := make([]string, len(lines)+1) + + var problems []SourceProblem + for i, tc := range tests { + for _, msg := range tc.AssertionErrors { + problems = append(problems, SourceProblem{Line: tc.Line, Test: tc.Name, Message: msg}) + } + body := strings.Split(tc.MDL, "\n") + if tc.MDL == "" || tc.BodyLine <= 0 { + continue + } + first := tc.BodyLine - 1 // 0-based + if first >= len(out) { + continue + } + for j := range body { + if k := first + j; k < len(out) && k < len(lines) { + out[k] = lines[k] + } + } + // A void microflow needs no RETURN, so the wrapper is two fragments and + // the body between them is exactly what the author typed. + place(out, first-1, fmt.Sprintf("CREATE OR REPLACE MICROFLOW %s.%s () BEGIN", mxTestModule, checkFlowName(tc, i))) + place(out, first+len(body), "END; /") + } + + return CheckedSource{MDL: strings.Join(out, "\n"), Problems: problems}, nil +} + +// checkFlowName names the wrapper after the test, because that name is what a +// semantic violation is reported against — linter locations carry a document, +// not a line. "at MxTest.Check_retrieve_with_a_limit" is the author's own words; +// "at MxTest.Check_1" is a number they never wrote and cannot search for. +func checkFlowName(tc TestCase, index int) string { + var b strings.Builder + b.WriteString("Check_") + for _, r := range tc.Name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + b.WriteRune(r) + default: + b.WriteByte('_') + } + } + if tc.Name == "" { + fmt.Fprintf(&b, "%d", index+1) + } + return b.String() +} + +// place writes a wrapper fragment onto a line, appending when the line is +// already taken. +// +// Two tests separated by a single-line doc comment want the same line — one for +// its END, the next for its header — and both fragments are complete statements, +// so sharing the line costs nothing and keeps every later line where it was. +// A fragment with no slot left is dropped rather than shifting every line after +// it: the point of this rendering is that a diagnostic's line number is the +// author's, and a missing END is reported on the line it is missing from. +func place(out []string, idx int, fragment string) { + if idx < 0 || idx >= len(out) { + return + } + if strings.TrimSpace(out[idx]) == "" { + out[idx] = fragment + return + } + out[idx] += " " + fragment +} diff --git a/cmd/mxcli/testrunner/check_source_test.go b/cmd/mxcli/testrunner/check_source_test.go new file mode 100644 index 0000000000..bd14088970 --- /dev/null +++ b/cmd/mxcli/testrunner/check_source_test.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestCheckSourceParsesATestFile is mendixlabs/mxcli#1103. +// +// A .test.mdl file is not a sequence of top-level MDL statements — each block is +// a MICROFLOW BODY, and that is what it becomes when the runner injects it. Fed +// to the top-level grammar, DECLARE is not a statement at all, the parser +// resyncs, RETRIEVE is swallowed as a non-reserved keyword, and `FROM …` starts +// an OQL query whose follow set is {GROUP_BY, SELECT, HAVING} — which is the +// error the reporter chased, on a statement mxcli's own syntax help prints. +func TestCheckSourceParsesATestFile(t *testing.T) { + src := `/** + * @test retrieve with a limit + * @cleanup none + */ +DECLARE $result Boolean = false; +RETRIEVE $reqs FROM Probe.Request WHERE Status = Probe.ENUM_Status.Approved LIMIT 1; +$req = HEAD($reqs); +$result = $req != empty; +/ +` + got, err := CheckSource(src, "x.test.mdl") + if err != nil { + t.Fatalf("CheckSource: %v", err) + } + if _, errs := visitor.Build(got.MDL); len(errs) > 0 { + t.Fatalf("a valid test file still does not parse:\n%v\n--- rendered ---\n%s", errs, got.MDL) + } +} + +// TestCheckSourceKeepsSourceLineNumbers is what makes the diagnostics usable: a +// rendered block must sit on the lines the author wrote it on, or every error +// points somewhere else and the reader is worse off than with no check. +func TestCheckSourceKeepsSourceLineNumbers(t *testing.T) { + src := `/** + * @test broken + */ +DECLARE $ok Boolean = true; +SET $ok = ; +/ +` + got, err := CheckSource(src, "x.test.mdl") + if err != nil { + t.Fatalf("CheckSource: %v", err) + } + _, errs := visitor.Build(got.MDL) + if len(errs) == 0 { + t.Fatal("the broken statement was not reported at all") + } + if !strings.Contains(errs[0].Error(), "line 5:") { + t.Errorf("error %q is not on line 5, where the bad statement is:\n%s", errs[0], got.MDL) + } +} + +// TestCheckSourceReportsAnnotationProblems: an @expect that cannot be compiled is +// already an ERROR at run time. `mxcli check` is where the author would rather +// hear about it. +func TestCheckSourceReportsAnnotationProblems(t *testing.T) { + src := `/** + * @test bad expect + * @expect count($ok) + */ +DECLARE $ok Boolean = true; +/ +` + got, err := CheckSource(src, "x.test.mdl") + if err != nil { + t.Fatalf("CheckSource: %v", err) + } + if len(got.Problems) == 0 { + t.Fatalf("an unusable @expect was not reported: %+v", got) + } +} + +// TestCheckSourceRejectsAMalformedFile: a file the test parser refuses is not +// checkable, and saying so beats reporting the grammar's confusion about it. +func TestCheckSourceRejectsAMalformedFile(t *testing.T) { + src := `/** + * @test one + */ +/** + * @test two + */ +DECLARE $ok Boolean = true; +/ +` + if _, err := CheckSource(src, "x.test.mdl"); err == nil { + t.Error("a file with two @test comments and no separator was accepted") + } +} + +// TestIsTestFile pins what the translation applies to. A plain .mdl script must +// keep going through the top-level grammar unchanged. +func TestIsTestFile(t *testing.T) { + cases := map[string]bool{ + "suite.test.mdl": true, + "suite.test.md": true, + "/a/b/SUITE.TEST.MDL": true, + "script.mdl": false, + "notes.md": false, + "-": false, + } + for name, want := range cases { + if got := IsTestFile(name); got != want { + t.Errorf("IsTestFile(%q) = %v, want %v", name, got, want) + } + } +} diff --git a/cmd/mxcli/testrunner/cleanup_leftovers.go b/cmd/mxcli/testrunner/cleanup_leftovers.go new file mode 100644 index 0000000000..6f68ac1608 --- /dev/null +++ b/cmd/mxcli/testrunner/cleanup_leftovers.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Deciding what cleanup has to remove. +// +// The generated test microflows are named positionally — MxTest.Test_test_1, +// _2, … from the test's index in its file — so every test file reuses the same +// names. Cleaning up the CURRENT suite's names is therefore wrong in both +// directions, and both were reported as mendixlabs/mxcli#1104: +// +// - A run with fewer tests than the last one leaves the surplus behind, and a +// leftover that does not compile fails the build of every later run of any +// file. Cleanup never looks at it again, because it is not in any suite. +// - An injection that failed part-way created only some of them, so DROPping +// the whole suite fails on the rest and cleanup reports the project left +// modified when it had just been cleaned. +// +// What is in the project is the authority on what to remove. The suite is only +// the fallback for when the project cannot be read. +package testrunner + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + "strings" +) + +// generatedTestFlowNames qualifies the microflow names mxcli generated, and only +// those. +// +// A pre-existing MxTest module is the user's — `mxcli test` adds documents to it +// and takes those documents back out, never the module and never anything else +// in it. The prefix is the whole of that distinction, so it is applied in one +// place. +func generatedTestFlowNames(names []string) []string { + bare := strings.TrimPrefix(testFlowPrefix, mxTestModule+".") + out := make([]string, 0, len(names)) + for _, n := range names { + if strings.HasPrefix(n, bare) { + out = append(out, mxTestModule+"."+n) + } + } + return out +} + +// listGeneratedTestFlows asks the project which generated test microflows it +// currently holds. +func listGeneratedTestFlows(projectPath string) ([]string, error) { + mxcliPath, err := findMxcli() + if err != nil { + return nil, err + } + cmd := exec.Command(mxcliPath, "-p", projectPath, "-c", "SHOW MICROFLOWS IN "+mxTestModule, "--json") + cmd.Env = append(os.Environ(), "MXCLI_QUIET=1") + output, err := cmd.Output() + if err != nil { + return nil, err + } + var flows []struct { + Name string `json:"Name"` + } + if err := json.Unmarshal(output, &flows); err != nil { + return nil, fmt.Errorf("parsing microflow list: %w", err) + } + names := make([]string, 0, len(flows)) + for _, f := range flows { + names = append(names, f.Name) + } + return generatedTestFlowNames(names), nil +} + +// testFlowsToDrop returns the generated microflows cleanup should remove. +// +// Discovery can fail — no MxTest module yet, an unreadable project, no mxcli on +// PATH — and a cleanup that removes nothing is worse than one that tries the +// names it knows. So the suite is the fallback, which is exactly the old +// behaviour and no worse than it. +func testFlowsToDrop(projectPath string, suite *TestSuite) []string { + if projectPath == "" { + return suiteTestFlowNames(suite) + } + if flows, err := listGeneratedTestFlows(projectPath); err == nil { + return flows + } + return suiteTestFlowNames(suite) +} + +// suiteTestFlowNames is the fallback: the names this run would have created. +func suiteTestFlowNames(suite *TestSuite) []string { + if suite == nil { + return nil + } + names := make([]string, 0, len(suite.Tests)) + for _, tc := range suite.Tests { + names = append(names, testFlowName(tc)) + } + return names +} + +// survivingTestFlows reports what is still in the project after a failed +// cleanup, best effort. Nothing is reported rather than something guessed: a +// list the reader cannot trust is worse than no list, because the whole point +// is to save them the hunt. +func survivingTestFlows(projectPath string) []string { + if projectPath == "" { + return nil + } + flows, err := listGeneratedTestFlows(projectPath) + if err != nil { + return nil + } + return flows +} diff --git a/cmd/mxcli/testrunner/cleanup_leftovers_test.go b/cmd/mxcli/testrunner/cleanup_leftovers_test.go new file mode 100644 index 0000000000..95bbd8666a --- /dev/null +++ b/cmd/mxcli/testrunner/cleanup_leftovers_test.go @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "errors" + "strings" + "testing" +) + +// TestEndpointCleanupDropsEveryGeneratedFlow is the guard for the second half of +// mendixlabs/mxcli#1104. +// +// Generated names are positional — MxTest.Test_test_1, _2, … — and are reused by +// every test file. Cleaning up only the CURRENT suite's names therefore leaves a +// flow behind whenever a later run has fewer tests than an earlier one, and that +// leftover fails the build of every subsequent run of any file. What is in the +// project is the authority on what to drop; the suite is not. +func TestEndpointCleanupDropsEveryGeneratedFlow(t *testing.T) { + st := projectState{afterStartup: "MyModule.Startup"} + // This run has one test; Test_test_7 is an earlier run's leftover. + present := []string{"MxTest.Test_test_1", "MxTest.Test_test_7"} + + cmds := endpointCleanupCommands(st, present, true) + joined := strings.Join(cmds, "\n") + + for _, want := range []string{ + "DROP MICROFLOW MxTest.Test_test_1", + "DROP MICROFLOW MxTest.Test_test_7", + } { + if !strings.Contains(joined, want) { + t.Errorf("cleanup does not %s:\n%s", want, joined) + } + } +} + +// TestCleanupNeverDropsWhatWasNotCreated is the other half of keying cleanup on +// the project rather than on the suite. +// +// An injection that failed part-way leaves some flows created and some not. +// Issuing DROP for every test in the suite makes the missing ones fail, so +// cleanup reported "the project has been left modified" for a project it had +// just cleaned — a false alarm that sends the reader looking for damage. +func TestCleanupNeverDropsWhatWasNotCreated(t *testing.T) { + st := projectState{} + cmds := endpointCleanupCommands(st, []string{"MxTest.Test_test_1"}, true) + if strings.Contains(strings.Join(cmds, "\n"), "Test_test_2") { + t.Errorf("cleanup drops a flow that was never created:\n%s", strings.Join(cmds, "\n")) + } +} + +// TestGeneratedTestFlowNames keeps the prefix filter honest: a user's own +// microflow in a pre-existing MxTest module is not mxcli's to delete. +func TestGeneratedTestFlowNames(t *testing.T) { + got := generatedTestFlowNames([]string{"Test_test_1", "MyOwnFlow", "RegisterEndpoint", "Test_test_12"}) + want := []string{"MxTest.Test_test_1", "MxTest.Test_test_12"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("generatedTestFlowNames = %v, want %v", got, want) + } +} + +// TestReportCleanupNamesWhatWasLeft covers the reporter's second ask: a cleanup +// failure that does not say WHICH document survived leaves them to find it by +// hand, which is the step that cost them the debugging cycle. +func TestReportCleanupNamesWhatWasLeft(t *testing.T) { + var b strings.Builder + reportCleanup(&b, errors.New("DROP MICROFLOW MxTest.Test_test_2: exit status 1"), []string{"MxTest.Test_test_2"}) + out := b.String() + + if !strings.Contains(out, "MxTest.Test_test_2") { + t.Errorf("the surviving document is not named:\n%s", out) + } + if !strings.Contains(out, "DROP MICROFLOW MxTest.Test_test_2") { + t.Errorf("no runnable DROP was offered:\n%s", out) + } +} diff --git a/cmd/mxcli/testrunner/generator_endpoint_test.go b/cmd/mxcli/testrunner/generator_endpoint_test.go index 64f7b644f4..3bbbbbc74f 100644 --- a/cmd/mxcli/testrunner/generator_endpoint_test.go +++ b/cmd/mxcli/testrunner/generator_endpoint_test.go @@ -197,7 +197,7 @@ func TestEndpointCleanupCommands(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := endpointCleanupCommands(tc.state, suite, tc.present) + got := endpointCleanupCommands(tc.state, suiteTestFlowNames(suite), tc.present) if len(got) != len(tc.want) { t.Fatalf("got %d commands %q, want %d %q", len(got), got, len(tc.want), tc.want) } @@ -219,7 +219,7 @@ func TestEndpointCleanupRestoreIsAlwaysFirst(t *testing.T) { {createdMxTest: false}, {afterStartup: "Mod.ASU", createdMxTest: true}, } { - cmds := endpointCleanupCommands(st, suite, true) + cmds := endpointCleanupCommands(st, suiteTestFlowNames(suite), true) if !strings.HasPrefix(cmds[0], "ALTER SETTINGS MODEL AfterStartupMicroflow") { t.Errorf("state %+v: first command is %q, want the after-startup restore", st, cmds[0]) } diff --git a/cmd/mxcli/testrunner/handshake_test.go b/cmd/mxcli/testrunner/handshake_test.go index 26d5ad5033..6748770ea6 100644 --- a/cmd/mxcli/testrunner/handshake_test.go +++ b/cmd/mxcli/testrunner/handshake_test.go @@ -148,7 +148,7 @@ func TestGenerateEndpointMDLNoChainWhenNone(t *testing.T) { } func TestDropTestFlows(t *testing.T) { - got := dropTestFlows(&TestSuite{Tests: []TestCase{{ID: "test_1"}, {ID: "test_2"}}}) + got := dropTestFlows("", &TestSuite{Tests: []TestCase{{ID: "test_1"}, {ID: "test_2"}}}) want := []string{"DROP MICROFLOW MxTest.Test_test_1", "DROP MICROFLOW MxTest.Test_test_2"} if len(got) != len(want) { t.Fatalf("got %q, want %q", got, want) @@ -164,7 +164,7 @@ func TestDropTestFlows(t *testing.T) { // attach adds only test microflows, so it must remove only those. The endpoint // and the after-startup setting belong to the dev loop hosting them. func TestDropTestFlowsNeverTouchesTheEndpoint(t *testing.T) { - for _, cmd := range dropTestFlows(&TestSuite{Tests: []TestCase{{ID: "test_1"}}}) { + for _, cmd := range dropTestFlows("", &TestSuite{Tests: []TestCase{{ID: "test_1"}}}) { for _, forbidden := range []string{"DROP MODULE", endpointStartupFlow, endpointRegisterAction, "AfterStartupMicroflow"} { if strings.Contains(cmd, forbidden) { t.Errorf("attach cleanup would remove %q, which the hosting dev loop owns: %q", forbidden, cmd) diff --git a/cmd/mxcli/testrunner/parser.go b/cmd/mxcli/testrunner/parser.go index 1062fb47bc..65e31b0c18 100644 --- a/cmd/mxcli/testrunner/parser.go +++ b/cmd/mxcli/testrunner/parser.go @@ -38,6 +38,11 @@ type TestCase struct { Throws string // @throws expected error message, "" when written bare SourceFile string // Original file path Line int // Line number in source file + // BodyLine is the 1-based source line the MDL body starts on, which is not + // Line: that one points at the doc comment. Checking a test file needs the + // body's own line, so a diagnostic can be reported where the author wrote the + // statement rather than where the annotation is — see check_source.go. + BodyLine int } // expectsThrow reports whether the test expects its body to raise an error. @@ -175,7 +180,7 @@ func parseMDLTests(content string, sourcePath string) ([]TestCase, error) { for _, block := range blocks { // Extract javadoc comment and MDL body - doc, body, line, err := extractDocAndBody(block) + doc, body, line, bodyLine, err := extractDocAndBody(block) if err != nil { return nil, fmt.Errorf("%s: %w", sourcePath, err) } @@ -213,6 +218,7 @@ func parseMDLTests(content string, sourcePath string) ([]TestCase, error) { Throws: annotations.Throws, SourceFile: sourcePath, Line: line, + BodyLine: bodyLine, }) } @@ -249,7 +255,7 @@ func parseMarkdownTests(content string, sourcePath string) ([]TestCase, error) { blockContent := strings.Join(blockLines, "\n") // Parse the block as a single test - doc, body, _, err := extractDocAndBody(testBlock{Text: blockContent, Line: blockStart}) + doc, body, _, bodyLine, err := extractDocAndBody(testBlock{Text: blockContent, Line: blockStart}) if err != nil { return nil, fmt.Errorf("%s: %w", sourcePath, err) } @@ -280,6 +286,7 @@ func parseMarkdownTests(content string, sourcePath string) ([]TestCase, error) { Throws: annotations.Throws, SourceFile: sourcePath, Line: blockStart, + BodyLine: bodyLine, }) } else { blockLines = append(blockLines, line) @@ -397,7 +404,7 @@ func splitTestBlocks(content string) []testBlock { // with no message. Scanning for the delimiters by raw substring search is bug // 1b: a `--` line whose prose spelled them out was read as a doc comment, so // describing the bug in a comment re-triggered it. -func extractDocAndBody(block testBlock) (string, string, int, error) { +func extractDocAndBody(block testBlock) (string, string, int, int, error) { docs := scanDocComments(block.Text, block.Line) // More than one @test in a chunk means a '/' separator is missing. Silently @@ -410,7 +417,7 @@ func extractDocAndBody(block testBlock) (string, string, int, error) { } } if len(named) > 1 { - return "", "", 0, fmt.Errorf( + return "", "", 0, 0, fmt.Errorf( "test %q is followed by another @test doc comment (%q) with no '/' separator "+ "between them, so only one of the two could run: add a line containing "+ "just '/' after the first test's statements", named[0], named[1]) @@ -425,9 +432,36 @@ func extractDocAndBody(block testBlock) (string, string, int, error) { } } if doc == nil { - return "", strings.TrimSpace(block.Text), block.Line, nil + return "", strings.TrimSpace(block.Text), block.Line, bodyStartLine(block.Text, 0, block.Line), nil } - return doc.Text, strings.TrimSpace(block.Text[doc.End:]), doc.Line, nil + return doc.Text, strings.TrimSpace(block.Text[doc.End:]), doc.Line, + bodyStartLine(block.Text, doc.End, block.Line), nil +} + +// bodyStartLine is the 1-based file line the body's first non-blank character +// sits on, counting from the chunk's own first line. +// +// Computed here rather than derived from the doc comment's length, because the +// body is TrimSpace'd: leading blank lines belong to neither, and a body that +// starts on the same line as the comment's closing delimiter has no leading line +// of its own at all. +func bodyStartLine(chunk string, from, chunkLine int) int { + line := chunkLine + for i := 0; i < from && i < len(chunk); i++ { + if chunk[i] == '\n' { + line++ + } + } + for i := from; i < len(chunk); i++ { + switch chunk[i] { + case '\n': + line++ + case ' ', '\t', '\r': + default: + return line + } + } + return line } // docComment is one `/** … */` comment found in a chunk. diff --git a/cmd/mxcli/testrunner/runner.go b/cmd/mxcli/testrunner/runner.go index 54c017f014..939ca86a62 100644 --- a/cmd/mxcli/testrunner/runner.go +++ b/cmd/mxcli/testrunner/runner.go @@ -284,7 +284,7 @@ func runEndpoint(opts RunOptions, suite *TestSuite, timeout time.Duration, w io. cleanupErr := cleanupEndpoint(opts.ProjectPath, state, cleanupSuite, w) removeGeneratedJavaSource(opts.ProjectPath, w) restoreProjectFile(state, cleanupErr, w) - reportCleanup(w, cleanupErr) + reportCleanup(w, cleanupErr, survivingTestFlows(opts.ProjectPath)) if cleanupErr == nil { fmt.Fprintln(w, " project restored") } @@ -374,7 +374,7 @@ func runAfterStartup(opts RunOptions, suite *TestSuite, timeout time.Duration, w if err != nil { cleanupErr := cleanup(opts.ProjectPath, state, w) restoreProjectFile(state, cleanupErr, w) - reportCleanup(w, cleanupErr) + reportCleanup(w, cleanupErr, survivingTestFlows(opts.ProjectPath)) return nil, err } @@ -384,7 +384,7 @@ func runAfterStartup(opts RunOptions, suite *TestSuite, timeout time.Duration, w fmt.Fprintln(w, "Cleaning up...") cleanupErr := cleanup(opts.ProjectPath, state, w) restoreProjectFile(state, cleanupErr, w) - reportCleanup(w, cleanupErr) + reportCleanup(w, cleanupErr, survivingTestFlows(opts.ProjectPath)) PrintResults(w, result, opts.Color) @@ -752,11 +752,25 @@ func cleanup(projectPath string, st projectState, w io.Writer) error { // reportCleanup prints a cleanup failure prominently. The project is left mutated, // so this must not read as a passing run. -func reportCleanup(w io.Writer, err error) { +// left names the generated documents still in the project, so the reader does +// not have to find them by hand — which is the step mendixlabs/mxcli#1104 says +// cost them a debugging cycle. A surviving test microflow is not inert: it fails +// the build of every later run of any test file. +func reportCleanup(w io.Writer, err error, left []string) { if err == nil { return } fmt.Fprintf(w, "\nERROR: cleanup failed — the project has been left modified:\n%v\n", err) + if len(left) > 0 { + fmt.Fprintf(w, "\nStill in the project (generated by this run or an earlier one):\n") + for _, name := range left { + fmt.Fprintf(w, " %s\n", name) + } + fmt.Fprintf(w, "\nRemove them with:\n") + for _, name := range left { + fmt.Fprintf(w, " mxcli -p -c \"DROP MICROFLOW %s\"\n", name) + } + } fmt.Fprintf(w, "Check the after-startup microflow and the %s module before committing.\n", mxTestModule) } diff --git a/cmd/mxcli/testrunner/runner_attach.go b/cmd/mxcli/testrunner/runner_attach.go index 9d817f391b..800ed11eda 100644 --- a/cmd/mxcli/testrunner/runner_attach.go +++ b/cmd/mxcli/testrunner/runner_attach.go @@ -81,7 +81,11 @@ func (a *attachedApp) applyModelChange(projectPath string) (string, error) { return "", fmt.Errorf("rebuilding through the attached app's build server on port %d: %w", a.hs.ServePort, err) } if !build.OK() { - return "", fmt.Errorf("build failed: %s", build.Message) + // Carries the parsed problems; the caller turns them into per-test rows or + // names the document they were found in. Returning only build.Message here + // is what made an --attach run say nothing but "the project cannot be + // deployed" (mendixlabs/mxcli#1104). + return "", buildFailure(build) } // No restart callback: the runtime belongs to the other process. A structural // change is refused rather than half-applied — see the error below. @@ -113,7 +117,7 @@ func runAttached(opts RunOptions, suite *TestSuite, timeout time.Duration, w io. injected := suite finish := func(result *SuiteResult, runErr error) (*SuiteResult, error) { fmt.Fprintln(w, "Cleaning up...") - cleanupErr := runMDLCommands(opts.ProjectPath, dropTestFlows(injected)) + cleanupErr := runMDLCommands(opts.ProjectPath, dropTestFlows(opts.ProjectPath, injected)) if cleanupErr == nil { // Leave the app serving a model that matches the project on disk; // otherwise the developer's next page load still runs the test flows. @@ -122,7 +126,7 @@ func runAttached(opts RunOptions, suite *TestSuite, timeout time.Duration, w io. } fmt.Fprintln(w, " test microflows removed") } else { - reportCleanup(w, cleanupErr) + reportCleanup(w, cleanupErr, survivingTestFlows(opts.ProjectPath)) } if runErr != nil { return nil, runErr @@ -133,12 +137,28 @@ func runAttached(opts RunOptions, suite *TestSuite, timeout time.Duration, w io. return result, nil } + // Printing is shared because a build MxBuild rejected now produces results + // too: the failing test gets an ERROR row instead of the run getting a bare + // "the project cannot be deployed". + report := func(result *SuiteResult, err error) (*SuiteResult, error) { + if result != nil { + PrintResults(w, result, opts.Color) + if jerr := writeJUnit(opts, result, w); jerr != nil && err == nil { + err = jerr + } + } + return result, err + } + fmt.Fprintln(w, "Injecting test microflows...") if err := execMDLScript(opts.ProjectPath, GenerateTestFlows(suite), "mxtest-flows-*.mdl"); err != nil { return finish(nil, fmt.Errorf("injecting test microflows: %w", err)) } if _, err := app.applyModelChange(opts.ProjectPath); err != nil { - return finish(nil, err) + // Same treatment the --local boot gives a rejected build: attribute each + // error to the generated microflow it was found in, and name the document + // when it belongs to the project instead (mendixlabs/mxcli#1104). + return report(finish(resultsForBuildFailure(err, suite))) } if opts.Watch { @@ -149,24 +169,18 @@ func runAttached(opts RunOptions, suite *TestSuite, timeout time.Duration, w io. if err != nil { return finish(nil, err) } - result, err = finish(result, nil) - if result != nil { - PrintResults(w, result, opts.Color) - if jerr := writeJUnit(opts, result, w); jerr != nil && err == nil { - err = jerr - } - } - return result, err + return report(finish(result, nil)) } // dropTestFlows returns the DROP statements for a suite's generated microflows. -func dropTestFlows(suite *TestSuite) []string { - if suite == nil { - return nil - } - cmds := make([]string, 0, len(suite.Tests)) - for _, tc := range suite.Tests { - cmds = append(cmds, "DROP MICROFLOW "+testFlowName(tc)) +func dropTestFlows(projectPath string, suite *TestSuite) []string { + // Keyed on what the project holds, not on this suite: an attach always runs + // against a project whose MxTest module pre-exists, so nothing else ever + // removes a flow an earlier run left behind (mendixlabs/mxcli#1104). + flows := testFlowsToDrop(projectPath, suite) + cmds := make([]string, 0, len(flows)) + for _, name := range flows { + cmds = append(cmds, "DROP MICROFLOW "+name) } return cmds } diff --git a/cmd/mxcli/testrunner/runner_cleanup_test.go b/cmd/mxcli/testrunner/runner_cleanup_test.go index 0c1f0fbf58..3117f52369 100644 --- a/cmd/mxcli/testrunner/runner_cleanup_test.go +++ b/cmd/mxcli/testrunner/runner_cleanup_test.go @@ -113,8 +113,8 @@ func TestNoSecurityLevelManipulation(t *testing.T) { all := append(setupCommands(mxTestRunner), setupCommands(endpointStartupFlow)...) all = append(all, cleanupCommands(projectState{}, true)...) all = append(all, cleanupCommands(projectState{afterStartup: "Mod.Flow", createdMxTest: true}, true)...) - all = append(all, endpointCleanupCommands(projectState{}, suite, true)...) - all = append(all, endpointCleanupCommands(projectState{afterStartup: "Mod.Flow", createdMxTest: true}, suite, true)...) + all = append(all, endpointCleanupCommands(projectState{}, suiteTestFlowNames(suite), true)...) + all = append(all, endpointCleanupCommands(projectState{afterStartup: "Mod.Flow", createdMxTest: true}, suiteTestFlowNames(suite), true)...) for _, cmd := range all { if strings.Contains(strings.ToUpper(cmd), "SECURITY LEVEL") { t.Errorf("the runner still alters the project Security Level: %q (#802)", cmd) diff --git a/cmd/mxcli/testrunner/runner_endpoint.go b/cmd/mxcli/testrunner/runner_endpoint.go index f0d03069a5..064f08feed 100644 --- a/cmd/mxcli/testrunner/runner_endpoint.go +++ b/cmd/mxcli/testrunner/runner_endpoint.go @@ -3,7 +3,6 @@ package testrunner import ( - "errors" "fmt" "io" "os" @@ -104,17 +103,7 @@ func runViaEndpoint(opts RunOptions, suite *TestSuite, token string, timeout tim // test's problem, not the run's. Reporting it as an ERROR row — and the // rest as SKIP — says which assertion broke, where the bare failure said // only that the project would not deploy (FINDINGS #46 follow-up). - var bf *docker.BuildFailedError - if errors.As(err, &bf) { - if results := resultsFromFailedBuild(bf.BuildErrors(), suite); results != nil { - return &SuiteResult{Name: suite.Name, Tests: results, Started: time.Now()}, nil - } - // Not the tests' doing: the model itself does not build. Say so - // rather than letting the reader assume a test is at fault. - _, other := attributeBuildProblems(bf.BuildErrors(), suite) - return nil, fmt.Errorf("%w%s", err, buildFailureHint(other)) - } - return nil, err + return resultsForBuildFailure(err, suite) } defer sess.stop() return runSuite(sess.client, sess.adminOptions(), suite, opts, w) @@ -221,7 +210,7 @@ func endpointReadyTimeout(suiteTimeout time.Duration) time.Duration { // the MxTest module, dropping the module removes all of it in one statement; // when the module was already the user's, each generated document is named // explicitly so nothing of theirs is touched. -func endpointCleanupCommands(st projectState, suite *TestSuite, mxTestPresent bool) []string { +func endpointCleanupCommands(st projectState, flows []string, mxTestPresent bool) []string { restore := "ALTER SETTINGS MODEL AfterStartupMicroflow = ''" if st.afterStartup != "" { restore = "ALTER SETTINGS MODEL AfterStartupMicroflow = " + quoteMDLString(st.afterStartup) @@ -233,8 +222,10 @@ func endpointCleanupCommands(st projectState, suite *TestSuite, mxTestPresent bo if st.createdMxTest { return append(cmds, "DROP MODULE "+mxTestModule) } - for _, tc := range suite.Tests { - cmds = append(cmds, "DROP MICROFLOW "+testFlowName(tc)) + // Every generated flow the project holds, not just this suite's — see + // cleanup_leftovers.go. + for _, name := range flows { + cmds = append(cmds, "DROP MICROFLOW "+name) } return append(cmds, "DROP MICROFLOW "+endpointStartupFlow, @@ -255,7 +246,7 @@ func cleanupEndpoint(projectPath string, st projectState, suite *TestSuite, w io if mxTestPresent && !st.createdMxTest { fmt.Fprintf(w, " %s module already existed; dropping only the generated documents\n", mxTestModule) } - return runMDLCommands(projectPath, endpointCleanupCommands(st, suite, mxTestPresent)) + return runMDLCommands(projectPath, endpointCleanupCommands(st, testFlowsToDrop(projectPath, suite), mxTestPresent)) } // removeGeneratedJavaSource deletes the .java file the Java action generated. diff --git a/docs-site/src/appendixes/error-messages.md b/docs-site/src/appendixes/error-messages.md index fe9a642e2e..6244249eb7 100644 --- a/docs-site/src/appendixes/error-messages.md +++ b/docs-site/src/appendixes/error-messages.md @@ -144,6 +144,36 @@ The rule keys on **scope, not on the name**. `$item` is perfectly valid in a pre ## mxcli Parser Errors + +### MDL-LISTOP02: A list operation nested inside another one + +``` +count(…): the list argument is `filter($reqs, $currentObject/Status = Mod.E.Approved)`, +which is not a variable. A Mendix aggregate list activity stores its list as a +variable reference and has no slot for a nested computation, so the argument is +dropped and the activity is written with an empty list — mxbuild then rejects it +with CE0012 "The 'List' property is required.". [MDL-LISTOP02] +``` + +**Cause:** Every list operation and aggregate — `HEAD`, `TAIL`, `FIND`, `FILTER`, `SORT`, `UNION`, `INTERSECT`, `SUBTRACT`, `RANGE`, `COUNT`, `SUM`, `AVERAGE`, `MINIMUM`, `MAXIMUM`, `REDUCE`, `ALL`, `ANY` — is a separate **activity** in Mendix, and an activity stores its list as a **variable reference**. MDL's expression grammar makes them look composable, but there is nowhere in the model to put a nested call. + +Before this rule existed the inner call was dropped, list and predicate together, and the activity was written with an empty list. That passes `mxcli check`, execs with `Created microflow`, and fails only at build time — `CE0012 "The 'List' property is required."` for an aggregate, `CE0096` for a list operation. `sort(filter(…), Attr)` was worse: with the list gone the sort attribute has no entity to resolve against, and mxbuild aborts with an `InvalidOperationException` instead of reporting an error. + +The rule keys on the operand not reducing to a variable, so it also covers a non-list argument: `count('nonsense')` failed the same way. + +**Solution:** Give the inner operation its own statement and pass the variable. + +```mdl +-- WRONG +$n = count(filter($Requests, $currentObject/Status = Module.ENUM_Status.Approved)); + +-- RIGHT +$Approved = filter($Requests, $currentObject/Status = Module.ENUM_Status.Approved); +$n = count($Approved); +``` + +The same applies to both operands of `union`/`intersect`/`subtract`. + ### Mismatched input ``` diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 0cd17016cd..25eb48a807 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -68,14 +68,14 @@ create persistent entity Module.Photo ( | Create external entities | `create [or modify] external entities from Module.Client [into module] [entities (...)];` | Bulk from $metadata | | Drop entity | `drop entity Module.Name;` | | | Describe entity | `describe entity Module.Name;` | Full MDL output | -| Describe enumeration | `describe enumeration Module.Name;` | Full MDL output | +| Describe enumeration | `describe enumeration Module.Name;` | Full MDL output. **`System.*` enumerations are included** — they are platform built-ins with no stored unit, synthesized so their values are discoverable instead of guessed at until **CE1613**. They are read-only: `describe` prints them as `--` comment lines, and `create`/`alter`/`drop`/`move` naming the System module is refused | | Rename entity | `rename entity Module.Old to New;` | Updates all references | | Rename enumeration | `rename enumeration Module.Old to New;` | Updates attribute type refs | | Rename association | `rename association Module.Old to New;` | Updates all references | | Show entities | `show entities [in module];` | List all or filter by module | | Create enumeration | `create [or modify] enumeration Module.Name (Value1 'caption', ...);` | | | Alter enumeration values | `alter enumeration Module.Name add value [if not exists] X [caption '..'] \| rename value X to Y \| modify value X caption '..' \| drop value [if exists] X;` | `modify value … caption` re-captions in place (works while referenced). `if not exists` / `if exists` make the script re-runnable — the bare forms error and stop the run | -| Drop enumeration | `drop enumeration Module.Name;` | | +| Drop enumeration | `drop enumeration Module.Name;` | Refused for `System.*` (read-only platform module) | | Create association | `create [or modify] association Module.Name from Parent to Child type reference\|ReferenceSet [owner default\|both] [delete_behavior ...];` | OR MODIFY updates existing association in-place. **The FROM entity must live in `Module`** — Mendix stores an association in its FROM entity's module, so a remote FROM writes a dangling pointer and the project stops OPENING (**MDL070**). The TO entity may be remote; that direction is stored BY NAME | | Drop association | `drop association Module.Name;` | | | Association line anchors | `@anchor(from: (0, 54), to: (100, 54))` above `create association …` | Where the connector attaches to each entity box, as a **percentage** of the box (0..100, whole numbers). `from` = the FROM entity's box, `to` = the TO entity's. Omitting an end preserves what is stored, so a `create or modify` about something else never flattens a hand-tuned line. Cross-module associations have no anchors — Mendix stores none | @@ -597,7 +597,12 @@ and `mxbuild` were all clean. Only the running app showed it. | `TRY ... CATCH ... end TRY` | `on error { ... }` blocks | Use error handlers on specific activities | **Notes:** -- `retrieve ... limit n` IS supported. `limit 1` returns a single entity, otherwise returns a list. +- `retrieve ... limit n` IS supported. **`limit 1` with no `offset` binds a single OBJECT**, not a + one-element list: it is Mendix's "First object" range. Every other `limit` (including + `limit 1 offset n`) is a bounded range, which is a list. Using a `limit 1` variable as a list — + `head()`, `count()`, a `loop` — is **CE0097** at build time and **MDL-RETRIEVE01** at check time. + Note this is the opposite of the import-mapping clause above, where `first` binds an object and + `limit 1` a one-element list. - `rollback $entity [refresh];` IS supported. Rolls back uncommitted changes to an object. ## Project Organization @@ -1372,7 +1377,7 @@ MDL uses explicit property declarations for pages: | Page CSS class / style | `Class: 'css-class', Style: 'css: rule'` | `(Title: 'Home', Class: 'container-fluid bg-light', Style: 'min-height: 100vh')` — the page's Appearance | | Page variables | `variables: { $name: type = 'expr' }` | `variables: { $show: boolean = 'true' }` | | Repeated widget entries | ` ( … )` **in the widget body** | A repeatable property (FileUploader `allowedFileFormats`, HTML Element `attributes`, a chart's `series`) is a block, never a property value. `attributes: [(attributeName: 'x')]` is **MDL-WIDGET27** — it used to check clean, exec, and vanish from storage. `describe widget -p app.mpr` lists the container keywords | -| Data grid 2 column filter | `column c (attribute: A) { textfilter f }` | **Inside the column's braces.** `column c (…) filter f { … }` is the GALLERY form — the grammar reads it as a column with no body plus a sibling `filter` widget, which the grid has nowhere to put; it used to be dropped on write and is now **MDL-WIDGET30**. A grid-wide filter bar is `controlbar`; a gallery spells that same slot `filter`. Match the filter to the column's type (String → `textfilter`, number → `numberfilter`, DateTime → `datefilter`, Enumeration → `dropdownfilter`, Boolean → none) | +| Data grid 2 column filter | `column c (attribute: A) { textfilter f }` | **Inside the column's braces.** `column c (…) filter f { … }` is the GALLERY form — the grammar reads it as a column with no body plus a sibling `filter` widget, which the grid has nowhere to put; it used to be dropped on write and is now **MDL-WIDGET30**. A grid-wide filter bar is `controlbar`; a gallery spells that same slot `filter`. Match the filter to the column's type (String → `textfilter`, number → `numberfilter`, DateTime → `datefilter`, Enumeration **and Boolean** → `dropdownfilter` — the drop-down filter's own attribute types are Enum and Boolean, and a Boolean column filters Yes/No). A column may carry a **custom-content widget AND a filter**: `content` and `filter` are separate slots, so `column Active (attribute: IsActive) { checkbox cb (Editable: Never, ReadOnlyStyle: Control) dropdownfilter ddf }` renders checkbox cells and still filters | | Widget with nowhere to go | any widget in a pluggable widget's body | A child matching no container, slot or `template` catch-all is **MDL-WIDGET30** at check time and refused by `exec`. `describe widget -p app.mpr` lists what the parent declares. Needs the parent's definition, so it is silent without `-p` | | Inspect a widget | `describe widget ;` | `describe widget combobox;` — properties, enum values, defaults and the editor rules that HIDE properties under some configurations. **Body containers** names what the widget's body takes, and for an object list the widgets-typed slots *inside one item* plus the widget types that route into each — that is where `column … { textfilter }` is spelled out. Works with no project open; with one, reads the installed `.mpk` (version-accurate, and the only place a Marketplace widget appears). Same output as `mxcli widget describe` | | Widget name | Required after type | `textbox txtName (...)` | diff --git a/mdl-examples/bug-tests/1100-loop-variable-typing.mdl b/mdl-examples/bug-tests/1100-loop-variable-typing.mdl new file mode 100644 index 0000000000..72115f757f --- /dev/null +++ b/mdl-examples/bug-tests/1100-loop-variable-typing.mdl @@ -0,0 +1,124 @@ +-- mendixlabs/mxcli#1100 — a LOOP variable was untyped, so every expression rule +-- was silently off inside the construct where list processing happens. +-- +-- REPORTED SHAPE. The same mistake fires outside a loop and is silent inside it: +-- +-- -- A: refused, E004 +-- $out = 'status=' + $Req/Status; +-- +-- -- B: passes check, execs, fails the build +-- LOOP $r IN $reqs BEGIN +-- $out = $out + $r/Status; -- [CE0117] at Change variable activity +-- END LOOP; +-- +-- WHAT WAS ACTUALLY BROKEN — not what the title says. The loop BODY was walked +-- and its expressions were checked all along: the same `'status=' + $T/Status` +-- written against a PARAMETER inside the loop was refused before the fix. Two +-- separate holes in the variable scope produced the asymmetry, and both had to +-- close before the reported script reported anything: +-- +-- 1. `LOOP $r IN $reqs` never recorded `$r`, so `$r/Status` resolved to no +-- attribute and inferred Unknown — which every rule tolerates by design. +-- 2. `DECLARE $out String` never recorded `$out`, so the accumulator was +-- Unknown too. E004 needs BOTH operands typed, so fixing (1) alone still +-- reported nothing on `$out + $r/Status`. +-- +-- Fixing (2) also closed the same silence one level up: `$out = $out + $T/Status` +-- on a plain parameter, with no loop anywhere, was equally unreported. +-- +-- The list sources a loop can iterate are typed with it — a database retrieve, +-- an association retrieve (the far end resolved through the association index), +-- a CREATE LIST, and the list operations that carry their input's element type +-- through (FILTER, SORT, RANGE, UNION, …). +-- +-- REPRODUCING: these rules are the catalog-backed tier, so they need a project: +-- +-- mxcli check 1100-loop-variable-typing.mdl -p app.mpr --references +-- +-- `mxcli check` with no project (what `make check-mdl` runs) cannot resolve an +-- attribute's type at all, so this file is written in its CORRECTED form and +-- must PASS. The failing forms are in the comments above and the regression +-- coverage is TestTypeCheckProgramTypesLoopVariables in mdl/executor. + +create module Probe; + +create enumeration Probe.ENUM_Status ( + Open 'Open', + Closed 'Closed' +); + +create persistent entity Probe.Request ( + Code: String(20), + Status: Enumeration(Probe.ENUM_Status) +); + +create persistent entity Probe.Reporter ( + Email: String(200) +); + +create association Probe.Request_Reporter + from Probe.Request to Probe.Reporter; + +-- The reported microflow, written correctly. Before the fix the toString() was +-- optional as far as mxcli was concerned; now leaving it out is E004. +create or replace microflow Probe.SUB_LoopNormal () +returns String +begin + declare $out String = ''; + retrieve $reqs from Probe.Request; + loop $r in $reqs begin + $out = $out + toString($r/Status); + end loop; + return $out; +end; + +-- The control from the report: the same expression on a parameter. It was +-- refused before the fix and must stay refused after it. +create or replace microflow Probe.SUB_Param ($Req: Probe.Request) +returns String +begin + declare $out String = ''; + $out = 'status=' + toString($Req/Status); + return $out; +end; + +-- A loop over an association retrieve. The iterator is typed from the entity at +-- the far end of Request_Reporter, which is resolved rather than read off the +-- statement — an expression path does not spell its intermediate entity. +create or replace microflow Probe.SUB_LoopAssoc ($Req: Probe.Request) +returns String +begin + declare $emails String = ''; + retrieve $reps from $Req/Probe.Request_Reporter; + loop $rep in $reps begin + $emails = $emails + $rep/Email; + end loop; + return $emails; +end; + +-- A loop over a filtered list: FILTER carries the element type through, so the +-- iterator is still a Request. +create or replace microflow Probe.SUB_LoopFiltered () +returns String +begin + declare $codes String = ''; + retrieve $reqs from Probe.Request; + $open = FILTER($reqs, $currentObject/Status = Probe.ENUM_Status.Open); + loop $r in $open begin + $codes = $codes + $r/Code; + end loop; + return $codes; +end; + +-- An ON ERROR handler body was not walked at all, so moving a statement into +-- one exempted it from every rule. +create or replace microflow Probe.SUB_Handler () +returns String +begin + declare $out String = ''; + retrieve $reqs from Probe.Request + on error { + $out = $out + 'retrieve failed'; + }; + return $out; +end; diff --git a/mdl-examples/bug-tests/1101-nested-list-operand-dropped.fail.mdl b/mdl-examples/bug-tests/1101-nested-list-operand-dropped.fail.mdl new file mode 100644 index 0000000000..e8a99d0def --- /dev/null +++ b/mdl-examples/bug-tests/1101-nested-list-operand-dropped.fail.mdl @@ -0,0 +1,75 @@ +-- mendixlabs/mxcli#1101 — a list operation nested inside another one. +-- +-- This file is expected to FAIL `mxcli check` (.fail.mdl). Every statement below +-- used to pass check, exec with "Created microflow", and write an activity whose +-- List property was empty. +-- +-- MDL's expression grammar makes list operations look composable. Mendix's model +-- is not: each one is a separate ACTIVITY whose list is stored as a VARIABLE +-- reference, with no slot for a nested computation. The inner call therefore had +-- nowhere to go and was dropped — list and predicate together — and the describe +-- of the result reads `$n = count($)`. +-- +-- Measured on mxbuild 11.6.6, one microflow per project. The reporter's own +-- workaround (the two-statement form) is the control and passes at 0 errors: +-- +-- count(filter(…)) CE0012 "The 'List' property is required." +-- head(filter(…)) CE0096 — the list-operation flavour of the same +-- sum(filter(…), 1) CE0012 + CE0117 +-- sort(filter(…), Name) mxbuild ABORTS with InvalidOperationException. The +-- sort attribute resolves against the (now absent) +-- list's entity, so the document cannot be loaded at +-- all — no error code, no line. +-- count('nonsense') CE0012 — nesting is not required to lose the list +-- +-- Expected: MDL-LISTOP02 (error) on each of the six microflows below. + +-- 1. The reporter's exact form. +create or replace microflow BugTest1101.CountOfFilter() returns Integer +begin + retrieve $reqs from BugTest1101.Request; + $n = count(filter($reqs, $currentObject/Name != '')); + return $n; +end; + +-- 2. Same hole reached through a list operation rather than an aggregate. +create or replace microflow BugTest1101.HeadOfFilter() returns BugTest1101.Request +begin + retrieve $reqs from BugTest1101.Request; + $h = head(filter($reqs, $currentObject/Name != '')); + return $h; +end; + +-- 3. The nesting can go the other way round — an inner sort under a filter. +create or replace microflow BugTest1101.FilterOfSort() returns List of BugTest1101.Request +begin + retrieve $reqs from BugTest1101.Request; + $f = filter(sort($reqs, Name), $currentObject/Name != ''); + return $f; +end; + +-- 4. The worst one: with the list gone, the sort attribute has no entity to +-- resolve against and mxbuild cannot load the document at all. +create or replace microflow BugTest1101.SortOfFilter() returns List of BugTest1101.Request +begin + retrieve $reqs from BugTest1101.Request; + $s = sort(filter($reqs, $currentObject/Name != ''), Name); + return $s; +end; + +-- 5. The dropped operand can be the SECOND list of a two-list operation. +create or replace microflow BugTest1101.UnionOfFilter() returns List of BugTest1101.Request +begin + retrieve $reqs from BugTest1101.Request; + retrieve $others from BugTest1101.Request; + $u = union($others, filter($reqs, $currentObject/Name != '')); + return $u; +end; + +-- 6. Nesting is not required. Anything that is not a variable is dropped the +-- same way, so the rule keys on the operand not reducing to one. +create or replace microflow BugTest1101.CountOfLiteral() returns Integer +begin + $n = count('nonsense'); + return $n; +end; diff --git a/mdl-examples/bug-tests/1102-system-enumerations-readable.mdl b/mdl-examples/bug-tests/1102-system-enumerations-readable.mdl new file mode 100644 index 0000000000..c5ce92fce5 --- /dev/null +++ b/mdl-examples/bug-tests/1102-system-enumerations-readable.mdl @@ -0,0 +1,44 @@ +-- mendixlabs/mxcli#1102 — System enumerations are invisible to show/describe/search +-- +-- `describe entity` printed attributes typed against System enumerations while +-- the enumerations themselves could not be inspected through any command, so +-- their values could only be guessed at until the build rejected a guess with +-- CE1613 "The selected enumeration value no longer exists". +-- +-- $ mxcli -p app.mpr describe enumeration System.WorkflowActivityType +-- Error: enumeration not found: System.WorkflowActivityType +-- +-- Cause: modelsdk/meta.SystemEnumerations (15 enumerations, values and all) had +-- no consumer. The System module is not stored in the .mpr — its entities, +-- associations and Java actions are each synthesized and appended to a listing; +-- the enumeration half had the data table and no wiring. +-- +-- Run against any project (the System module is present in all of them): +-- mxcli exec mdl-examples/bug-tests/1102-system-enumerations-readable.mdl -p app.mpr + +-- The statement from the report. Before the fix: "enumeration not found". +DESCRIBE ENUMERATION System.WorkflowActivityType; + +-- The reporter's CE1613 was `System.WorkflowActivityExecutionState.Finished`. +-- Finished is real — but on System.WorkflowActivityState, a different +-- enumeration with a confusingly similar name. These two statements are what +-- makes that mistake obvious in one step instead of one build. +DESCRIBE ENUMERATION System.WorkflowActivityExecutionState; +DESCRIBE ENUMERATION System.WorkflowActivityState; + +-- Both of these are read-only: System enumerations describe as `--` comment +-- lines rather than as `create or modify …`, because every enumeration write +-- naming the System module is refused. The refusals are covered by unit tests +-- (mdl/executor/cmd_enumerations_system_test.go), not here: this file is checked +-- without a project, and a guard that needs a model cannot decide anything at +-- check time. Before the guard, the first of these REPORTED SUCCESS and wrote a +-- unit whose parent does not exist: +-- +-- CREATE ENUMERATION System.BrandNewThing (A 'a'); -- orphaned unit +-- ALTER ENUMERATION System.WorkflowActivityType ADD VALUE Invented CAPTION 'x'; +-- DROP ENUMERATION System.WorkflowActivityType; + +-- System enumerations are also listed now, so an attribute typed against one +-- resolves under `mxcli check --references` instead of being reported missing +-- (the #1071 direction of the same gap). +SHOW ENUMERATIONS; diff --git a/mdl-examples/bug-tests/1103-retrieve-limit-one-is-an-object.fail.mdl b/mdl-examples/bug-tests/1103-retrieve-limit-one-is-an-object.fail.mdl new file mode 100644 index 0000000000..467ffe1c16 --- /dev/null +++ b/mdl-examples/bug-tests/1103-retrieve-limit-one-is-an-object.fail.mdl @@ -0,0 +1,26 @@ +-- mendixlabs/mxcli#1103 — `retrieve … limit 1` binds a single OBJECT. +-- +-- The executor maps `limit 1` with no offset to Mendix's "First object" range, +-- so $Requests is an object and head() cannot take it. Nothing said so before +-- the build: `mxcli check --references` passed and `describe` re-emits +-- `limit 1`, so an object retrieve and a list retrieve are identical text. +-- mxbuild rejected it as CE0097 at the far end of a build — and inside a +-- .test.mdl file, not even that: the injected test simply failed to build. +-- +-- MDL-RETRIEVE01 reports it at check time. This script must FAIL check. +create module Probe; + +create entity Probe.Request ( + Code: String(20) +); + +create or replace microflow Probe.M_LimitOneAsList () +returns Boolean as $Found +begin + declare $Found Boolean = false; + retrieve $Requests from Probe.Request where Code = 'X' limit 1; + $Request = head($Requests); + set $Found = $Request != empty; + return $Found; +end; +/ diff --git a/mdl-examples/bug-tests/1103-test-file-checks-as-microflow-bodies.test.mdl b/mdl-examples/bug-tests/1103-test-file-checks-as-microflow-bodies.test.mdl new file mode 100644 index 0000000000..68c23818c0 --- /dev/null +++ b/mdl-examples/bug-tests/1103-test-file-checks-as-microflow-bodies.test.mdl @@ -0,0 +1,33 @@ +-- mendixlabs/mxcli#1103 — a .test.mdl file is checkable. +-- +-- Every block here is a MICROFLOW BODY, which is what the runner turns it into. +-- Checked against the top-level grammar instead, `declare` is not a statement, +-- the parser resyncs, `retrieve` is swallowed as a non-reserved keyword, and the +-- remaining `from …` starts an OQL query whose follow set is +-- {GROUP_BY, SELECT, HAVING} — so the reporter was told their retrieve needed a +-- SELECT. `mxcli check` renders the blocks as microflows on these same lines. + +/** + * @test limit 1 binds one object, so use it as one + * @expect $Found = true + */ +retrieve $Request from Probe.Request where Code = 'X' limit 1; +$Found = $Request != empty; +/ + +/** + * @test without a limit it is a list, and head() takes it + * @expect $Found = true + */ +retrieve $Requests from Probe.Request where Code = 'X'; +$Request = head($Requests); +$Found = $Request != empty; +/ + +/** + * @test a bounded range above one is still a list + * @expect $Count = 2 + */ +retrieve $Requests from Probe.Request where Code = 'X' limit 2; +$Count = count($Requests); +/ diff --git a/mdl-examples/bug-tests/927-test-mdl-leading-comment-and-count.test.mdl b/mdl-examples/bug-tests/927-test-mdl-leading-comment-and-count.fail.test.mdl similarity index 100% rename from mdl-examples/bug-tests/927-test-mdl-leading-comment-and-count.test.mdl rename to mdl-examples/bug-tests/927-test-mdl-leading-comment-and-count.fail.test.mdl diff --git a/mdl-examples/bug-tests/datagrid-489-column-content-and-filter.mdl b/mdl-examples/bug-tests/datagrid-489-column-content-and-filter.mdl new file mode 100644 index 0000000000..93ec405d4d --- /dev/null +++ b/mdl-examples/bug-tests/datagrid-489-column-content-and-filter.mdl @@ -0,0 +1,59 @@ +-- ============================================================================ +-- ako/mxcli#489 — a DataGrid2 column with BOTH custom content and a filter +-- ============================================================================ +-- +-- Reported upstream as a missing capability (mendixlabs/mxcli#1111: "the column +-- block accepts either a content widget or a filter widget, not both"). The +-- write path already supported it; DESCRIBE did not, so a describe → exec round +-- trip deleted the filter. +-- +-- Measured on 11.6.6 before the fix: +-- * stored BSON: showContentAs=customContent, content=[cbActive], +-- filter=[ddfActive] -- both slots filled +-- * `mx check`: 0 errors +-- * browser: checkbox cells render AND the Yes/No/(all) filter works +-- * `describe page`: the dropdownfilter is ABSENT +-- * re-exec of that description: the `filter` slot is gone +-- +-- A filter-only column round-tripped by accident: with `content` empty the +-- filter landed in the content list and was re-emitted in the column body, where +-- the builder routes it back to the filter slot by widget type. That accident is +-- why this went unnoticed — the common shape looked fine. +-- +-- After the fix, describing this page and re-executing the output reports +-- `Unchanged page`: the description reproduces the stored document exactly. +-- +-- Note for the "visual checkbox" half of the upstream report: a read-only check +-- box renders as the text "Yes"/"No" unless ReadOnlyStyle is Control — see +-- ako/mxcli#490 and readonlystyle-490-checkbox-control.mdl. +-- ============================================================================ + +create entity BugTests.Customer ( + Name: string(200), + IsActive: boolean +); + +create or replace page BugTests.P_489_ColumnContentAndFilter ( + Title: 'Column with content and filter', + Layout: Atlas_Core.Atlas_Default +) { + datagrid dgCustomers ( + DataSource: database from BugTests.Customer, + Selection: None + ) { + column Name (Attribute: Name, Caption: 'Name') { + textfilter tfName + } + -- The column under test: a custom-content cell AND a filter in one block. + column IsActive (Attribute: IsActive, Caption: 'Active') { + checkbox cbActive (Attribute: IsActive, Editable: Never, ReadOnlyStyle: Control) + dropdownfilter ddfActive + } + } +}; + +-- Verify by hand: +-- mxcli exec datagrid-489-column-content-and-filter.mdl -p app.mpr +-- mxcli -p app.mpr -c "describe page BugTests.P_489_ColumnContentAndFilter" +-- -> the column body must list BOTH cbActive and ddfActive +-- re-exec that description -> "Unchanged page", not "Replaced page" diff --git a/mdl-examples/bug-tests/expect-vacuous-assertions.test.mdl b/mdl-examples/bug-tests/expect-vacuous-assertions.fail.test.mdl similarity index 100% rename from mdl-examples/bug-tests/expect-vacuous-assertions.test.mdl rename to mdl-examples/bug-tests/expect-vacuous-assertions.fail.test.mdl diff --git a/mdl-examples/bug-tests/readonlystyle-490-checkbox-control.mdl b/mdl-examples/bug-tests/readonlystyle-490-checkbox-control.mdl new file mode 100644 index 0000000000..c9cfba5876 --- /dev/null +++ b/mdl-examples/bug-tests/readonlystyle-490-checkbox-control.mdl @@ -0,0 +1,65 @@ +-- ============================================================================ +-- ako/mxcli#490 — ReadOnlyStyle on a check box reaches the document +-- ============================================================================ +-- +-- `ReadOnlyStyle:` parsed, passed `mxcli check` (it is in the known-property +-- allowlist as "vocabulary describe page emits"), was reported as executed — +-- and the stored document kept "Inherit". DESCRIBE *did* read and emit it, so +-- describe → exec on a Studio Pro-authored page silently downgraded Control to +-- Inherit. +-- +-- Not cosmetic. Measured on 11.6.6, same document otherwise, `mx check` 0 +-- errors both ways: +-- ReadOnlyStyle: Inherit -> a read-only check box renders the TEXT "Yes"/"No" +-- ReadOnlyStyle: Control -> it renders the (disabled) CHECKBOX glyph +-- +-- That is the whole of "show a Boolean as a checkbox" in a DataGrid2 cell, and +-- it is why mendixlabs/mxcli#1111 asked Mendix for a new column type. +-- +-- Scope: check box only, matching what DESCRIBE reads back today. The other +-- input widgets (textbox, textarea, datepicker, radiobuttons) still write a +-- hardcoded Inherit and are not read back either. +-- ============================================================================ + +create entity BugTests.Task ( + Description: string(200), + Done: boolean +); + +create or replace page BugTests.P_490_ReadOnlyStyle ( + Title: 'Read-only style', + Layout: Atlas_Core.Atlas_Default +) { + layoutgrid lgMain { + row rowMain { + column colMain (DesktopWidth: 12) { + datagrid dgTasks (DataSource: database from BugTests.Task, Selection: None) { + column Description (Attribute: Description, Caption: 'Task') { + textfilter tfDescription + } + -- Control: the cell renders a disabled checkbox, and the column still + -- filters (Yes / No) — the two halves ako/mxcli#489 and #490 together. + column Done (Attribute: Done, Caption: 'Done') { + checkbox cbDoneControl (Attribute: Done, Editable: Never, ReadOnlyStyle: Control) + dropdownfilter ddfDone + } + } + } + } + } +}; + +-- Verify by hand: +-- mxcli exec readonlystyle-490-checkbox-control.mdl -p app.mpr +-- mxcli bson dump page -p app.mpr --object BugTests.P_490_ReadOnlyStyle \ +-- | grep -A1 '"ReadOnlyStyle"' -> Control (was Inherit before the fix) +-- mxcli -p app.mpr -c "describe page BugTests.P_490_ReadOnlyStyle" +-- -> ReadOnlyStyle: Control on cbDoneControl; re-exec reports +-- "Unchanged page", so the description reproduces the document. +-- Drop the `ReadOnlyStyle:` clause and re-exec -> "Replaced page", and the +-- stored value goes back to Inherit: the control that shows the value is the +-- authored one rather than a constant. +-- +-- An unknown value is refused rather than written — a member Studio Pro cannot +-- resolve is a project that will not open, and mxbuild does not complain: +-- checkbox cbBad (Attribute: Done, ReadOnlyStyle: ReadOnly) -- refused diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index d0e5dcd735..84b423b918 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -834,10 +834,31 @@ type ListOperationStmt struct { // ListOperationsAction has no ErrorHandlingType, so an ON ERROR here has // nowhere to go; parsing it and reporting it beats dropping it silently. ErrorHandling *ErrorHandlingClause + // UnresolvedOperands holds the list operands that did not reduce to a + // variable name — see UnresolvedOperand. Empty on every well-formed statement. + UnresolvedOperands []UnresolvedOperand } func (s *ListOperationStmt) isMicroflowStatement() {} +// UnresolvedOperand is a list operand that the visitor could not reduce to a +// variable name. +// +// A Mendix list-operation or aggregate activity stores its list as a VARIABLE +// REFERENCE — there is no slot for a nested computation. So MDL's expression +// grammar accepts `count(filter($l, …))`, which looks composable, but the model +// has nowhere to put the inner call. The conversion used to drop it silently and +// write the activity with an empty List, which passes `check`, execs with a +// success message, and fails the build with CE0012 / CE0096 (mendixlabs/mxcli#1101). +// +// Recording what was dropped — rather than leaving an empty InputVariable behind +// — is what lets the validator name the operand and print the two-statement +// rewrite. Expr is nil when the operand was absent altogether. +type UnresolvedOperand struct { + Index int // 0 = the list; 1 = the second list of UNION/INTERSECT/SUBTRACT/CONTAINS/EQUALS + Expr Expression // what was written there, for the diagnostic +} + // AggregateListOperationType represents the type of aggregate operation. type AggregateListOperationType int @@ -897,6 +918,9 @@ type AggregateListStmt struct { // ErrorHandling is recorded only so the clause can be REFUSED — Mendix's // AggregateAction has no ErrorHandlingType. See ListOperationStmt. ErrorHandling *ErrorHandlingClause + // UnresolvedOperands holds the list operand that did not reduce to a variable + // name — see UnresolvedOperand. Empty on every well-formed statement. + UnresolvedOperands []UnresolvedOperand } func (s *AggregateListStmt) isMicroflowStatement() {} diff --git a/mdl/backend/modelsdk/enumeration.go b/mdl/backend/modelsdk/enumeration.go index 6b83034350..a6206e33b5 100644 --- a/mdl/backend/modelsdk/enumeration.go +++ b/mdl/backend/modelsdk/enumeration.go @@ -4,6 +4,7 @@ package modelsdkbackend import ( genEnum "github.com/mendixlabs/mxcli/modelsdk/gen/enumerations" + "github.com/mendixlabs/mxcli/modelsdk/meta" "github.com/mendixlabs/mxcli/modelsdk/mprread" "github.com/mendixlabs/mxcli/model" @@ -18,21 +19,30 @@ func (b *Backend) ListEnumerations() ([]*model.Enumeration, error) { if err != nil { return nil, err } - out := make([]*model.Enumeration, 0, len(units)) + out := make([]*model.Enumeration, 0, len(units)+len(meta.SystemEnumerations)) for _, u := range units { out = append(out, enumToModel(u.Element, u.ContainerID)) } + // The System module's enumerations are platform built-ins with no stored + // unit, so they have to be synthesized or they vanish — same reason as + // ListJavaActions. Without them `describe enumeration System.X` reports + // "not found" and `check --references` rejects an attribute typed against + // one, while `describe entity` prints that very type (#1102). + out = append(out, meta.BuildSystemEnumerations()...) return out, nil } func (b *Backend) GetEnumeration(id model.ID) (*model.Enumeration, error) { - units, err := mprread.ListUnitsWithContainer[*genEnum.Enumeration](b.reader) + // Resolved against the same set ListEnumerations returns, so a caller + // holding an ID from the listing can always look it up again — the System + // module's synthesized enumerations included. + enums, err := b.ListEnumerations() if err != nil { return nil, err } - for _, u := range units { - if model.ID(u.Element.ID()) == id { - return enumToModel(u.Element, u.ContainerID), nil + for _, e := range enums { + if e.ID == id { + return e, nil } } return nil, nil diff --git a/mdl/backend/modelsdk/enumeration_system_test.go b/mdl/backend/modelsdk/enumeration_system_test.go new file mode 100644 index 0000000000..a22c31a027 --- /dev/null +++ b/mdl/backend/modelsdk/enumeration_system_test.go @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/meta" +) + +// The System module is not stored in the project — its enumerations exist only +// in modelsdk/meta — so a reader that decodes stored units alone reports them as +// absent. That is what made `describe enumeration System.WorkflowActivityType` +// fail, `show enumerations` omit them, and `check --references` reject a valid +// attribute typed against one (mendixlabs/mxcli#1102). Same shape as the System +// Java actions synthesized in java.go. + +// valuesOf returns an enumeration's value names. +func valuesOf(e *model.Enumeration) []string { + out := make([]string, 0, len(e.Values)) + for _, v := range e.Values { + out = append(out, v.Name) + } + return out +} + +func hasValue(e *model.Enumeration, name string) bool { + for _, v := range e.Values { + if v.Name == name { + return true + } + } + return false +} + +// findSystemEnum returns the synthesized System enumeration of that local name. +func findSystemEnum(enums []*model.Enumeration, name string) *model.Enumeration { + for _, e := range enums { + if e.Name == name && string(e.ContainerID) == meta.SystemModuleID { + return e + } + } + return nil +} + +func TestListEnumerations_IncludesSystemModule(t *testing.T) { + b := New() + if err := b.Connect(fixture); err != nil { + t.Fatalf("Connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + enums, err := b.ListEnumerations() + if err != nil { + t.Fatalf("ListEnumerations: %v", err) + } + + // The control: the STORED enumerations must still come back. Without it this + // passes against a build that returned the synthesized ones only. + stored := 0 + for _, e := range enums { + if string(e.ContainerID) != meta.SystemModuleID { + stored++ + } + } + if stored != 7 { + t.Errorf("stored enumerations = %d, want 7 (the fixture's own)", stored) + } + + if got, want := len(enums)-stored, len(meta.SystemEnumerations); got != want { + t.Errorf("synthesized System enumerations = %d, want %d", got, want) + } + + activityType := findSystemEnum(enums, "WorkflowActivityType") + if activityType == nil { + t.Fatal("System.WorkflowActivityType not returned by ListEnumerations") + } + if !hasValue(activityType, "UserTask") { + t.Errorf("System.WorkflowActivityType values = %v, want UserTask among them", valuesOf(activityType)) + } + + // The reporter's CE1613 was `System.WorkflowActivityExecutionState.Finished`. + // Finished is real, but on System.WorkflowActivityState — this one has + // Completed. Pinning both directions is what makes the listing answer the + // question that was actually being asked. + execState := findSystemEnum(enums, "WorkflowActivityExecutionState") + if execState == nil { + t.Fatal("System.WorkflowActivityExecutionState not returned by ListEnumerations") + } + if hasValue(execState, "Finished") { + t.Error("System.WorkflowActivityExecutionState must not carry Finished (that is WorkflowActivityState)") + } + if !hasValue(execState, "Completed") { + t.Errorf("System.WorkflowActivityExecutionState values = %v, want Completed", valuesOf(execState)) + } + if activityState := findSystemEnum(enums, "WorkflowActivityState"); activityState == nil { + t.Error("System.WorkflowActivityState not returned by ListEnumerations") + } else if !hasValue(activityState, "Finished") { + t.Errorf("System.WorkflowActivityState values = %v, want Finished", valuesOf(activityState)) + } +} + +// TestGetEnumeration_FindsSystemModule keeps the by-ID lookup consistent with +// the listing: a caller holding an ID from ListEnumerations must be able to +// resolve it again. +func TestGetEnumeration_FindsSystemModule(t *testing.T) { + b := New() + if err := b.Connect(fixture); err != nil { + t.Fatalf("Connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + + enums, err := b.ListEnumerations() + if err != nil { + t.Fatalf("ListEnumerations: %v", err) + } + want := findSystemEnum(enums, "WorkflowActivityType") + if want == nil { + t.Fatal("System.WorkflowActivityType not in ListEnumerations") + } + + got, err := b.GetEnumeration(want.ID) + if err != nil { + t.Fatalf("GetEnumeration(%q): %v", want.ID, err) + } + if got == nil { + t.Fatalf("GetEnumeration(%q) = nil — the listing offers an ID the getter cannot resolve", want.ID) + } + if got.Name != "WorkflowActivityType" { + t.Errorf("GetEnumeration returned %q, want WorkflowActivityType", got.Name) + } + + // Control: a stored enumeration still resolves by ID. + for _, e := range enums { + if string(e.ContainerID) == meta.SystemModuleID { + continue + } + stored, err := b.GetEnumeration(e.ID) + if err != nil || stored == nil { + t.Fatalf("GetEnumeration(%q) for stored %s: %v / nil=%v", e.ID, e.Name, err, stored == nil) + } + break + } +} diff --git a/mdl/backend/modelsdk/enumeration_test.go b/mdl/backend/modelsdk/enumeration_test.go index 9b9ea7b2a0..928bbee049 100644 --- a/mdl/backend/modelsdk/enumeration_test.go +++ b/mdl/backend/modelsdk/enumeration_test.go @@ -2,11 +2,21 @@ package modelsdkbackend -import "testing" +import ( + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/meta" +) // TestReadSlice_Enumerations checks the enum adapter: values are converted (for // the Values count) and captions decode via textElementToModel. SHOW // ENUMERATIONS is cross-checked byte-for-byte against legacy in the plan. +// +// The count is of STORED enumerations. ListEnumerations also returns the System +// module's synthesized ones (#1102), which have no stored unit and no caption to +// decode, so counting the whole listing here would stop testing the adapter and +// start tracking the size of a hardcoded table. func TestReadSlice_Enumerations(t *testing.T) { b := New() if err := b.Connect(fixture); err != nil { @@ -14,12 +24,18 @@ func TestReadSlice_Enumerations(t *testing.T) { } t.Cleanup(func() { _ = b.Disconnect() }) - enums, err := b.ListEnumerations() + all, err := b.ListEnumerations() if err != nil { t.Fatalf("ListEnumerations: %v", err) } + var enums []*model.Enumeration + for _, e := range all { + if string(e.ContainerID) != meta.SystemModuleID { + enums = append(enums, e) + } + } if len(enums) != 7 { - t.Fatalf("ListEnumerations count = %d, want 7", len(enums)) + t.Fatalf("stored enumeration count = %d, want 7", len(enums)) } for _, e := range enums { if e.Name == "Filter_Operators" { diff --git a/mdl/backend/modelsdk/widget_readonlystyle_test.go b/mdl/backend/modelsdk/widget_readonlystyle_test.go new file mode 100644 index 0000000000..f0d82ca0dd --- /dev/null +++ b/mdl/backend/modelsdk/widget_readonlystyle_test.go @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// ako/mxcli#490 — `ReadOnlyStyle:` parsed, passed check, was emitted by DESCRIBE, +// and was never written: the codec hardcoded "Inherit" on every check box. +// +// Not cosmetic. With "Inherit" a read-only check box in a DataGrid2 cell renders +// as the text "Yes"/"No"; with "Control" it renders the (disabled) checkbox +// glyph — measured on 11.6.6 by patching the stored string by hand. +func TestCheckBoxReadOnlyStyle_Written(t *testing.T) { + tests := []struct { + name string + style string + want string + }{ + // An omitted property keeps the stored default, so scripts that never + // mention it produce the same document as before. + {"unset keeps the default", "", "Inherit"}, + {"control", "Control", "Control"}, + {"text", "Text", "Text"}, + {"inherit", "Inherit", "Inherit"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cb := &pages.CheckBox{ReadOnlyStyle: tc.style} + cb.Name = "cbActive" + doc := encodeWidget(t, cb) + if got := docGet(doc, "ReadOnlyStyle"); got != tc.want { + t.Errorf("ReadOnlyStyle = %v, want %q — the authored value never reached "+ + "the document (ako/mxcli#490)", got, tc.want) + } + }) + } +} diff --git a/mdl/backend/modelsdk/widget_write.go b/mdl/backend/modelsdk/widget_write.go index 242dac4242..cba094814c 100644 --- a/mdl/backend/modelsdk/widget_write.go +++ b/mdl/backend/modelsdk/widget_write.go @@ -462,7 +462,10 @@ func widgetToGen(w pages.Widget) (element.Element, error) { } g.SetOnChangeAction(onChangeCB) g.SetOnEnterAction(noActionGen()) - g.SetReadOnlyStyle("Inherit") + // Unset keeps Mendix's default; an authored Control/Text is what decides + // whether a read-only check box renders as the glyph or as "Yes"/"No" + // text (ako/mxcli#490). The value is canonicalised at build time. + g.SetReadOnlyStyle(orDefaultStr(x.ReadOnlyStyle, "Inherit")) g.SetValidation(widgetValidationToGen()) return g, nil diff --git a/mdl/executor/cmd_enumerations.go b/mdl/executor/cmd_enumerations.go index 683b5968c3..0542ea51b0 100644 --- a/mdl/executor/cmd_enumerations.go +++ b/mdl/executor/cmd_enumerations.go @@ -15,6 +15,38 @@ import ( "github.com/mendixlabs/mxcli/model" ) +// refuseSystemEnumerationWrite rejects any write that names the System module. +// +// The System module's enumerations are platform built-ins that the backend +// SYNTHESIZES so they can be read (#1102) — there is no stored unit for the +// module, so there is nothing for a write to live in. Before this guard the four +// write verbs did not fail cleanly: CREATE ENUMERATION System.X reported +// "Created enumeration: System.X" and wrote a unit whose ContainerID was the +// synthetic module ID 00000000-…-0001, which is not a unit in the project — an +// orphan with a dangling parent, on disk, with no error. The others surfaced a +// raw .mxunit path instead. +// +// The signal is the module NAME, not the container: a brand-new enumeration has +// no container yet, and that is precisely the case that used to corrupt (the +// module lookup resolves "System" to the virtual module and hands its synthetic +// ID over as the parent). Unlike the Marketplace guard, which deliberately does +// not trust a name because a user may name a module Atlas_Core, "System" is +// reserved by the platform and cannot be a user module. +func refuseSystemEnumerationWrite(verb string, name ast.QualifiedName) error { + if name.Module != "System" { + return nil + } + // The hint must not name the statement's own enumeration: on a CREATE that + // name usually does not exist, and telling the reader to describe it would + // send them after nothing. + return mdlerrors.NewValidation(fmt.Sprintf( + "cannot %s %s: the System module is owned by the Mendix platform and is read-only — "+ + "its enumerations are built in, not stored in the project. "+ + "Define your own enumeration in one of your modules; "+ + "`show enumerations` lists the built-in ones and `describe enumeration System.` reports their values.", + verb, name.String())) +} + // execCreateEnumeration handles CREATE ENUMERATION statements. func execCreateEnumeration(ctx *ExecContext, s *ast.CreateEnumerationStmt) error { @@ -22,6 +54,14 @@ func execCreateEnumeration(ctx *ExecContext, s *ast.CreateEnumerationStmt) error return mdlerrors.NewNotConnected() } + verb := "create enumeration" + if s.CreateOrModify { + verb = "create or modify enumeration" + } + if err := refuseSystemEnumerationWrite(verb, s.Name); err != nil { + return err + } + // Validate enumeration values for reserved words if violations := ValidateEnumeration(s); len(violations) > 0 { var msgs []string @@ -145,6 +185,9 @@ func findEnumeration(ctx *ExecContext, moduleName, enumName string) *model.Enume // execAlterEnumeration handles ALTER ENUMERATION ADD/DROP/RENAME VALUE by // read-modify-writing the enumeration through the backend (engine-agnostic). func execAlterEnumeration(ctx *ExecContext, s *ast.AlterEnumerationStmt) error { + if err := refuseSystemEnumerationWrite("alter enumeration", s.Name); err != nil { + return err + } enum := findEnumeration(ctx, s.Name.Module, s.Name.Name) if enum == nil { return mdlerrors.NewNotFound("enumeration", s.Name.String()) @@ -240,6 +283,10 @@ func execDropEnumeration(ctx *ExecContext, s *ast.DropEnumerationStmt) error { return mdlerrors.NewNotConnected() } + if err := refuseSystemEnumerationWrite("drop enumeration", s.Name); err != nil { + return err + } + // Find enumeration enums, err := ctx.Backend.ListEnumerations() if err != nil { @@ -367,6 +414,21 @@ func describeEnumeration(ctx *ExecContext, name ast.QualifiedName) error { fmt.Fprintf(ctx.Output, "/**\n * %s\n */\n", enum.Documentation) } + // A System enumeration is a platform built-in: the write paths refuse + // it, so emitting `create or modify …` would hand the reader a + // statement mxcli rejects. Report it informationally instead, the way + // DESCRIBE BUILDING BLOCK does for the other read-only doctype. + if modName == "System" { + fmt.Fprintf(ctx.Output, "-- Enumeration: %s.%s\n", modName, enum.Name) + fmt.Fprintf(ctx.Output, "-- Values (%d):\n", len(enum.Values)) + for _, v := range enum.Values { + fmt.Fprintf(ctx.Output, "-- %s\n", v.Name) + } + fmt.Fprintf(ctx.Output, "-- The System module is owned by the Mendix platform: this enumeration is\n") + fmt.Fprintf(ctx.Output, "-- built in and read-only, so this output is informational, not re-executable.\n") + return nil + } + fmt.Fprintf(ctx.Output, "create or modify enumeration %s.%s (\n", modName, enum.Name) for i, v := range enum.Values { comma := "," diff --git a/mdl/executor/cmd_enumerations_system_test.go b/mdl/executor/cmd_enumerations_system_test.go new file mode 100644 index 0000000000..ab0128c81b --- /dev/null +++ b/mdl/executor/cmd_enumerations_system_test.go @@ -0,0 +1,274 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/meta" +) + +// The System module's enumerations are platform built-ins with no stored unit: +// the backend synthesizes them so they can be READ (mendixlabs/mxcli#1102). +// Making them visible also makes them addressable by the write paths, and the +// System module has no stored unit to contain anything — so every write has to +// be refused at the statement, not discovered as a disk error underneath. +// +// On the enumeration path the pre-fix behaviour was worse than a bad message: +// `CREATE ENUMERATION System.BrandNewThing` REPORTED SUCCESS and wrote a unit +// whose ContainerID was the synthetic module ID 00000000-…-0001, which is not a +// unit in the project — an orphan with a dangling parent. (Measured on the +// expr-checker fixture: 369 → 370 units, container present in no Unit row.) +// Entities happen to fail safe because they need the virtual domain-model unit +// loaded first; enumerations are units in their own right, so nothing stopped +// them. + +// systemEnumCtx builds a context whose backend exposes a user module plus the +// virtual System module and one synthesized System enumeration, and records +// whether any write reached the backend. +func systemEnumCtx(t *testing.T) (*ExecContext, *[]string) { + t.Helper() + user := mkModule("Sales") + system := &model.Module{ + BaseElement: model.BaseElement{ID: model.ID(meta.SystemModuleID)}, + Name: "System", + } + + userEnum := mkEnumeration(user.ID, "OrderStatus", "Draft", "Shipped") + sysEnum := mkEnumeration(system.ID, "WorkflowActivityType", "UserTask", "CallMicroflow") + + h := mkHierarchy(user, system) + withContainer(h, userEnum.ContainerID, user.ID) + withContainer(h, sysEnum.ContainerID, system.ID) + + var writes []string + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{user, system}, nil + }, + ListEnumerationsFunc: func() ([]*model.Enumeration, error) { + return []*model.Enumeration{userEnum, sysEnum}, nil + }, + CreateEnumerationFunc: func(e *model.Enumeration) error { + writes = append(writes, "create:"+e.Name) + return nil + }, + UpdateEnumerationFunc: func(e *model.Enumeration) error { + writes = append(writes, "update:"+e.Name) + return nil + }, + DeleteEnumerationFunc: func(id model.ID) error { + writes = append(writes, "delete:"+string(id)) + return nil + }, + MoveEnumerationFunc: func(e *model.Enumeration) error { + writes = append(writes, "move:"+e.Name) + return nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + return ctx, &writes +} + +func sysQN(name string) ast.QualifiedName { + return ast.QualifiedName{Module: "System", Name: name} +} + +// TestDescribeEnumeration_System is the symptom from the issue: the values have +// to be reportable, because nothing else in mxcli can tell you what they are. +func TestDescribeEnumeration_System(t *testing.T) { + ctx, _ := systemEnumCtx(t) + var out strings.Builder + ctx.Output = &out + + if err := describeEnumeration(ctx, sysQN("WorkflowActivityType")); err != nil { + t.Fatalf("describe enumeration System.WorkflowActivityType: %v", err) + } + got := out.String() + for _, want := range []string{"System.WorkflowActivityType", "UserTask", "CallMicroflow"} { + if !strings.Contains(got, want) { + t.Errorf("describe output missing %q:\n%s", want, got) + } + } + // The write paths refuse System, so DESCRIBE must not emit a statement that + // mxcli would reject if pasted back — a describe → exec round trip that + // cannot work is worse than one that is plainly marked read-only. + if strings.Contains(got, "create or modify enumeration") { + t.Errorf("describe emits a CREATE statement for a read-only System enumeration:\n%s", got) + } + if !strings.Contains(got, "read-only") { + t.Errorf("describe output does not say the enumeration is read-only:\n%s", got) + } +} + +// TestDescribeEnumeration_UserStillRoundTrips is the control for the branch +// above: an ordinary enumeration must still describe as re-executable MDL. +func TestDescribeEnumeration_UserStillRoundTrips(t *testing.T) { + ctx, _ := systemEnumCtx(t) + var out strings.Builder + ctx.Output = &out + + if err := describeEnumeration(ctx, ast.QualifiedName{Module: "Sales", Name: "OrderStatus"}); err != nil { + t.Fatalf("describe enumeration Sales.OrderStatus: %v", err) + } + got := out.String() + if !strings.Contains(got, "create or modify enumeration Sales.OrderStatus") { + t.Errorf("user enumeration no longer describes as re-executable MDL:\n%s", got) + } +} + +// TestSystemEnumerationWrites_AreRefused covers all four write verbs. Each one +// must refuse BEFORE the backend is touched — the assertion on `writes` is the +// point, since a refusal that still wrote would leave the orphan behind. +func TestSystemEnumerationWrites_AreRefused(t *testing.T) { + cases := []struct { + name string + run func(ctx *ExecContext) error + }{ + {"create", func(ctx *ExecContext) error { + return execCreateEnumeration(ctx, &ast.CreateEnumerationStmt{ + Name: sysQN("BrandNewThing"), + Values: []ast.EnumValue{{Name: "A", Caption: "a"}}, + }) + }}, + {"create or modify", func(ctx *ExecContext) error { + return execCreateEnumeration(ctx, &ast.CreateEnumerationStmt{ + Name: sysQN("WorkflowActivityType"), + CreateOrModify: true, + Values: []ast.EnumValue{{Name: "A", Caption: "a"}}, + }) + }}, + {"alter add value", func(ctx *ExecContext) error { + return execAlterEnumeration(ctx, &ast.AlterEnumerationStmt{ + Name: sysQN("WorkflowActivityType"), + Operation: ast.AlterEnumAdd, + ValueName: "Invented", + Caption: "Invented", + }) + }}, + {"drop", func(ctx *ExecContext) error { + return execDropEnumeration(ctx, &ast.DropEnumerationStmt{ + Name: sysQN("WorkflowActivityType"), + }) + }}, + {"move out", func(ctx *ExecContext) error { + return moveEnumeration(ctx, sysQN("WorkflowActivityType"), model.ID("mod-sales"), "Sales") + }}, + {"rename", func(ctx *ExecContext) error { + return execRenameEnumeration(ctx, &ast.RenameStmt{ + Name: sysQN("WorkflowActivityType"), + NewName: "Renamed", + }) + }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx, writes := systemEnumCtx(t) + err := tc.run(ctx) + if err == nil { + t.Fatalf("%s on a System enumeration was accepted, want a refusal", tc.name) + } + msg := err.Error() + if !strings.Contains(msg, "System") { + t.Errorf("refusal does not name the System module: %q", msg) + } + // The pre-fix failures were a success message (create) or a raw + // mxunit path (the rest). Neither is an explanation. + if strings.Contains(msg, ".mxunit") || strings.Contains(msg, "no such file") { + t.Errorf("refusal leaks a storage error instead of explaining: %q", msg) + } + if len(*writes) != 0 { + t.Errorf("refusal still reached the backend: %v", *writes) + } + }) + } +} + +// TestMoveEnumerationIntoSystem_IsRefused covers the other end of MOVE: the +// enumeration being moved is an ordinary one, but the destination is System. +// Guarding only the source would let a user enumeration be moved INTO a module +// with no stored unit, which is the same orphan by another route. +func TestMoveEnumerationIntoSystem_IsRefused(t *testing.T) { + ctx, writes := systemEnumCtx(t) + err := moveEnumeration(ctx, + ast.QualifiedName{Module: "Sales", Name: "OrderStatus"}, + model.ID(meta.SystemModuleID), "System") + if err == nil { + t.Fatal("moving a user enumeration into System was accepted, want a refusal") + } + if !strings.Contains(err.Error(), "System") { + t.Errorf("refusal does not name the System module: %q", err) + } + if len(*writes) != 0 { + t.Errorf("refusal still reached the backend: %v", *writes) + } +} + +// TestUserEnumerationWrites_StillWork is the control. Without it the guard could +// be refusing every enumeration write and every test above would still pass. +func TestUserEnumerationWrites_StillWork(t *testing.T) { + userQN := ast.QualifiedName{Module: "Sales", Name: "OrderStatus"} + + t.Run("create or modify", func(t *testing.T) { + ctx, writes := systemEnumCtx(t) + if err := execCreateEnumeration(ctx, &ast.CreateEnumerationStmt{ + Name: userQN, + CreateOrModify: true, + Values: []ast.EnumValue{{Name: "Draft", Caption: "Draft"}}, + }); err != nil { + t.Fatalf("create or modify on a user enumeration: %v", err) + } + if len(*writes) == 0 { + t.Error("user enumeration write did not reach the backend") + } + }) + + t.Run("alter add value", func(t *testing.T) { + ctx, writes := systemEnumCtx(t) + if err := execAlterEnumeration(ctx, &ast.AlterEnumerationStmt{ + Name: userQN, + Operation: ast.AlterEnumAdd, + ValueName: "Cancelled", + Caption: "Cancelled", + }); err != nil { + t.Fatalf("alter on a user enumeration: %v", err) + } + if len(*writes) == 0 { + t.Error("user enumeration alter did not reach the backend") + } + }) + + t.Run("drop", func(t *testing.T) { + ctx, writes := systemEnumCtx(t) + if err := execDropEnumeration(ctx, &ast.DropEnumerationStmt{Name: userQN}); err != nil { + t.Fatalf("drop on a user enumeration: %v", err) + } + if len(*writes) == 0 { + t.Error("user enumeration drop did not reach the backend") + } + }) +} + +// TestDropModuleSystem_IsRefused: DROP MODULE cascades over the module's +// documents, and the System module's are all synthesized. Before the refusal it +// reported "unit not found" once per document — 15 warnings and no change — which +// is noise the enumeration fix would otherwise have introduced (#1102). +func TestDropModuleSystem_IsRefused(t *testing.T) { + ctx, writes := systemEnumCtx(t) + err := execDropModule(ctx, &ast.DropModuleStmt{Name: "System"}) + if err == nil { + t.Fatal("DROP MODULE System was accepted, want a refusal") + } + if !strings.Contains(err.Error(), "System") { + t.Errorf("refusal does not name the module: %q", err) + } + if len(*writes) != 0 { + t.Errorf("refusal still reached the backend: %v", *writes) + } +} diff --git a/mdl/executor/cmd_modules.go b/mdl/executor/cmd_modules.go index 99cb3e454e..f84de602d9 100644 --- a/mdl/executor/cmd_modules.go +++ b/mdl/executor/cmd_modules.go @@ -85,6 +85,18 @@ func execDropModule(ctx *ExecContext, s *ast.DropModuleStmt) error { return mdlerrors.NewNotFound("module", s.Name) } + // The System module is virtual: it is synthesized from modelsdk/meta, not + // stored, so there is nothing here to drop. The cascade below would walk its + // synthesized documents and report a "unit not found" warning for each one + // (15 of them once the enumerations became visible — #1102) while changing + // nothing. Refusing says that in one line instead. + if targetModule.Name == "System" { + return mdlerrors.NewValidation( + "cannot drop module System: it is owned by the Mendix platform and is not stored in the " + + "project — its entities, associations, enumerations and Java actions are built in. " + + "Every Mendix app has it and no app can remove it.") + } + // Build set of all container IDs belonging to this module (including nested folders) moduleContainers := getModuleContainers(ctx, targetModule.ID) diff --git a/mdl/executor/cmd_move.go b/mdl/executor/cmd_move.go index 99ab6458e9..57c970799e 100644 --- a/mdl/executor/cmd_move.go +++ b/mdl/executor/cmd_move.go @@ -357,6 +357,15 @@ func moveEntity(ctx *ExecContext, name ast.QualifiedName, sourceModule, targetMo // moveEnumeration moves an enumeration to a new container. // For cross-module moves, updates all EnumerationAttributeType references across all domain models. func moveEnumeration(ctx *ExecContext, name ast.QualifiedName, targetContainerID model.ID, targetModuleName string) error { + // Neither end may be System: its enumerations are platform built-ins with no + // stored unit, so there is nothing to move out and nowhere to move in (#1102). + if err := refuseSystemEnumerationWrite("move enumeration", name); err != nil { + return err + } + if targetModuleName == "System" { + return refuseSystemEnumerationWrite("move enumeration into", + ast.QualifiedName{Module: "System", Name: name.Name}) + } enum := findEnumeration(ctx, name.Module, name.Name) if enum == nil { return mdlerrors.NewNotFound("enumeration", name.String()) diff --git a/mdl/executor/cmd_pages_builder_readonlystyle_test.go b/mdl/executor/cmd_pages_builder_readonlystyle_test.go new file mode 100644 index 0000000000..5803ba5f41 --- /dev/null +++ b/mdl/executor/cmd_pages_builder_readonlystyle_test.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#490 — the builder half: `ReadOnlyStyle:` on a checkbox has to reach +// the semantic model, or the codec has nothing to write. The property parsed, +// `mxcli check` accepted it (it is in the known-property allowlist as +// "vocabulary describe page emits") and every layer below dropped it. +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" +) + +func TestBuildCheckBox_ReadOnlyStyle(t *testing.T) { + tests := []struct { + name string + value any + want string + }{ + {"unset stays unset (writer keeps the stored default)", nil, ""}, + {"control", "Control", "Control"}, + {"text", "Text", "Text"}, + {"inherit", "Inherit", "Inherit"}, + // MDL property values are matched case-insensitively everywhere else; + // the enum value stored must still be Mendix's own casing, since an + // unknown member is a document Studio Pro cannot load. + {"lowercase is canonicalised", "control", "Control"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + pb := &pageBuilder{widgetScope: map[string]model.ID{}} + w := &ast.WidgetV3{Name: "cbActive", Type: "checkbox", Properties: map[string]any{}} + if tc.value != nil { + w.Properties["ReadOnlyStyle"] = tc.value + } + cb, err := pb.buildCheckBoxV3(w) + if err != nil { + t.Fatalf("buildCheckBoxV3: %v", err) + } + if cb.ReadOnlyStyle != tc.want { + t.Errorf("ReadOnlyStyle = %q, want %q (ako/mxcli#490)", cb.ReadOnlyStyle, tc.want) + } + }) + } +} + +// An unknown member must be refused rather than written: a value outside +// Inherit/Control/Text is a property Studio Pro cannot resolve, and mxbuild +// tolerates it — so the build stays green and the project will not open. +func TestBuildCheckBox_ReadOnlyStyleRejectsUnknownValue(t *testing.T) { + pb := &pageBuilder{widgetScope: map[string]model.ID{}} + w := &ast.WidgetV3{ + Name: "cbActive", + Type: "checkbox", + Properties: map[string]any{"ReadOnlyStyle": "ReadOnly"}, + } + _, err := pb.buildCheckBoxV3(w) + if err == nil { + t.Fatal("an unknown ReadOnlyStyle was accepted — it would be written into the document") + } + if !strings.Contains(err.Error(), "Control") { + t.Errorf("the error should name the accepted values, got: %v", err) + } +} diff --git a/mdl/executor/cmd_pages_builder_v3_widgets.go b/mdl/executor/cmd_pages_builder_v3_widgets.go index 0aee2b63b1..8dd2940427 100644 --- a/mdl/executor/cmd_pages_builder_v3_widgets.go +++ b/mdl/executor/cmd_pages_builder_v3_widgets.go @@ -594,6 +594,16 @@ func (pb *pageBuilder) buildCheckBoxV3(w *ast.WidgetV3) (*pages.CheckBox, error) cb.Label = label } + // Handle ReadOnlyStyle ("Read-only style": Inherit / Control / Text). An + // omitted property stays empty and the writer keeps the stored default — + // what decides whether a read-only check box renders as "Yes"/"No" text or + // as the checkbox glyph (ako/mxcli#490). + style, err := readOnlyStyleValue(w.GetStringProp("ReadOnlyStyle"), w.Name) + if err != nil { + return nil, err + } + cb.ReadOnlyStyle = style + // Handle OnChange (the "On change" client action) if err := pb.applyOnChangeV3(w, &cb.OnChangeAction); err != nil { return nil, err @@ -606,6 +616,26 @@ func (pb *pageBuilder) buildCheckBoxV3(w *ast.WidgetV3) (*pages.CheckBox, error) return cb, nil } +// readOnlyStyleValue canonicalises an authored `ReadOnlyStyle:` to the member +// Mendix stores. MDL matches property values case-insensitively, but the value +// written has to be one of the metamodel's members (generated/metamodel's +// PagesReadOnlyStyle): an unknown one is a property Studio Pro cannot resolve, +// and mxbuild tolerates it — so the build stays green and the project does not +// open. Empty in, empty out: unset keeps the stored default. +func readOnlyStyleValue(raw, widgetName string) (string, error) { + if raw == "" { + return "", nil + } + for _, member := range []string{"Inherit", "Control", "Text"} { + if strings.EqualFold(raw, member) { + return member, nil + } + } + return "", mdlerrors.NewValidationf( + "checkbox %q: ReadOnlyStyle %q is not a Mendix read-only style — use Inherit, Control or Text", + widgetName, raw) +} + // buildRadioButtonsV3 creates RadioButtons from V3 syntax. func (pb *pageBuilder) buildRadioButtonsV3(w *ast.WidgetV3) (*pages.RadioButtons, error) { rb := &pages.RadioButtons{ diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index e1c0f6c06b..c8ef55b83b 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -577,7 +577,8 @@ type rawDataGridColumn struct { Caption string CaptionParams []string // Parameters for template placeholders in caption ShowContentAs string // "attribute", "customContent", or "dynamicText" - ContentWidgets []rawWidget // Widgets inside the column (for custom content) + ContentWidgets []rawWidget // Widgets in the column's `content` slot (custom content) + FilterWidgets []rawWidget // Widgets in the column's `filter` slot (text/number/date/dropdown filter) DynamicText string // Template text for dynamicText mode DynamicTextParams []string // Parameters for dynamicText template Alignment string // "left", "center", or "right" (empty = default "left") diff --git a/mdl/executor/cmd_pages_describe_column_filter_test.go b/mdl/executor/cmd_pages_describe_column_filter_test.go new file mode 100644 index 0000000000..a4cf7c30aa --- /dev/null +++ b/mdl/executor/cmd_pages_describe_column_filter_test.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: Apache-2.0 + +// ako/mxcli#489: DESCRIBE PAGE drops a DataGrid2 column's filter widget when the +// column also carries custom content. +// +// A column's `content` and `filter` are two independent Widgets-typed slots, and +// the write path fills both (measured on 11.6.6: showContentAs=customContent, +// content=[cbActive], filter=[ddfActive], mx check 0 errors, both working in the +// browser). The reader took the FIRST widgets-typed property it met instead of +// keying on the resolved property key, and the writer emits column properties +// alphabetically — so `content` won and the filter was dropped. A filter-only +// column survived by accident: with `content` empty the filter landed in the +// content list and was re-emitted in the column body, where the builder routes +// it back to the filter slot by widget type. +// +// The round trip is what makes this more than cosmetic: describing such a page +// and re-executing the output deletes the filter, with mxcli check, exec and +// mx check clean at both ends. +package executor + +import ( + "bytes" + "strings" + "testing" +) + +// buildDataGridWithContentAndFilterColumn mirrors the shape Mendix stores for a +// DataGrid2 whose single column holds BOTH a custom-content widget and a filter +// widget. Property keys resolve through the widget's own PropertyTypes, keyed by +// the WidgetProperty's TypePointer (the PropertyType $ID — the inner WidgetValue +// points at the ValueType $ID instead, which is a different node). +func buildDataGridWithContentAndFilterColumn() map[string]any { + const ( + idColumns = "type-id-columns" + idHeader = "type-id-header" + idShowContentAs = "type-id-showcontentas" + idContent = "type-id-content" + idFilter = "type-id-filter" + ) + + colProp := func(typePointer string, value map[string]any) map[string]any { + return map[string]any{"TypePointer": typePointer, "Value": value} + } + + return map[string]any{ + "Name": "dgTest", + "Type": map[string]any{ + "WidgetId": "com.mendix.widget.web.datagrid.Datagrid", + "ObjectType": map[string]any{ + "PropertyTypes": []any{ + map[string]any{ + "$ID": idColumns, "PropertyKey": "columns", + "ValueType": map[string]any{ + "ObjectType": map[string]any{ + "PropertyTypes": []any{ + map[string]any{"$ID": idHeader, "PropertyKey": "header", + "ValueType": map[string]any{"Type": "TextTemplate"}}, + map[string]any{"$ID": idShowContentAs, "PropertyKey": "showContentAs", + "ValueType": map[string]any{"Type": "Enumeration"}}, + map[string]any{"$ID": idContent, "PropertyKey": "content", + "ValueType": map[string]any{"Type": "Widgets"}}, + map[string]any{"$ID": idFilter, "PropertyKey": "filter", + "ValueType": map[string]any{"Type": "Widgets"}}, + }, + }, + }, + }, + }, + }, + }, + "Object": map[string]any{ + "Properties": []any{ + map[string]any{ + "TypePointer": idColumns, + "Value": map[string]any{ + "Objects": []any{ + map[string]any{ + // Alphabetical, as the writer emits them: content before filter. + "Properties": []any{ + colProp(idContent, map[string]any{ + "Widgets": []any{ + map[string]any{ + "$Type": "Forms$CheckBox", + "Name": "cbActive", + }, + }, + }), + colProp(idFilter, map[string]any{ + "Widgets": []any{ + map[string]any{ + "$Type": "CustomWidgets$CustomWidget", + "Name": "ddfActive", + "Type": map[string]any{ + "WidgetId": "com.mendix.widget.web.datagriddropdownfilter.DatagridDropdownFilter", + }, + }, + }, + }), + colProp(idHeader, map[string]any{ + "TextTemplate": map[string]any{ + "Template": map[string]any{ + "Items": []any{ + map[string]any{"Text": "Active"}, + }, + }, + }, + }), + colProp(idShowContentAs, map[string]any{ + "PrimitiveValue": "customContent", + }), + }, + }, + }, + }, + }, + }, + }, + } +} + +// The read half: both slots must survive extraction, in their own lists. +func TestDataGrid2Column_KeepsContentAndFilterWidgets(t *testing.T) { + cols := extractDataGrid2Columns(nil, buildDataGridWithContentAndFilterColumn()) + if len(cols) != 1 { + t.Fatalf("expected 1 column, got %d", len(cols)) + } + col := cols[0] + if len(col.ContentWidgets) != 1 || col.ContentWidgets[0].Name != "cbActive" { + t.Errorf("content widgets = %+v, want one widget named cbActive", col.ContentWidgets) + } + if len(col.FilterWidgets) != 1 || col.FilterWidgets[0].Name != "ddfActive" { + t.Fatalf("filter widgets = %+v, want one widget named ddfActive — the column's "+ + "filter was dropped, so describe→exec deletes it (ako/mxcli#489)", col.FilterWidgets) + } +} + +// A filter-only column keeps working: its filter belongs in FilterWidgets now +// rather than riding along in the content list, and DESCRIBE must still emit it. +func TestDataGrid2Column_FilterOnlyColumnStillRoundTrips(t *testing.T) { + w := buildDataGridWithContentAndFilterColumn() + // Drop the content property, leaving filter + header + showContentAs. + cols := w["Object"].(map[string]any)["Properties"].([]any)[0].(map[string]any) + objects := cols["Value"].(map[string]any)["Objects"].([]any) + colProps := objects[0].(map[string]any)["Properties"].([]any) + objects[0].(map[string]any)["Properties"] = colProps[1:] + + got := extractDataGrid2Columns(nil, w) + if len(got) != 1 { + t.Fatalf("expected 1 column, got %d", len(got)) + } + if len(got[0].ContentWidgets) != 0 { + t.Errorf("content widgets = %+v, want none", got[0].ContentWidgets) + } + if len(got[0].FilterWidgets) != 1 || got[0].FilterWidgets[0].Name != "ddfActive" { + t.Errorf("filter widgets = %+v, want one widget named ddfActive", got[0].FilterWidgets) + } +} + +// The emit half: both widgets must appear in the column body. Testing through +// outputWidgetMDLV3 rather than the column helper means removing the emit change +// fails this test — asserting on the helper alone would prove the helper works +// and nothing about the wiring. +func TestDataGrid2Column_EmitsContentAndFilterWidgets(t *testing.T) { + cols := extractDataGrid2Columns(nil, buildDataGridWithContentAndFilterColumn()) + if len(cols) == 0 { + t.Fatal("fixture produced no columns") + } + + var buf bytes.Buffer + ctx := &ExecContext{Output: &buf} + outputWidgetMDLV3(ctx, rawWidget{ + Type: "CustomWidgets$CustomWidget", + RenderMode: "datagrid2", + Name: "dgTest", + WidgetID: "com.mendix.widget.web.datagrid.Datagrid", + DataGridColumns: cols, + }, 0) + + out := buf.String() + if !strings.Contains(out, "cbActive") { + t.Errorf("custom-content widget missing from DESCRIBE output:\n%s", out) + } + if !strings.Contains(out, "ddfActive") { + t.Errorf("filter widget missing from DESCRIBE output — re-executing this output "+ + "deletes the filter (ako/mxcli#489):\n%s", out) + } + // A body, not a bare column line, or the output cannot re-parse. + if !strings.Contains(out, "column Active") || !strings.Contains(out, "{") { + t.Errorf("column should be emitted with a body:\n%s", out) + } +} diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 5dc16b38df..25ca8c3c44 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -978,7 +978,11 @@ func outputDataGrid2ColumnV3(ctx *ExecContext, prefix, colName string, col rawDa // named Title/Description), mirroring the general widget-name path so DESCRIBE // output re-parses. #619 added mdlIdent for widgets but missed columns (#638). header := fmt.Sprintf("column %s", mdlIdent(colName)) - hasContent := len(col.ContentWidgets) > 0 + // A column body carries the `content` slot's widgets AND the `filter` slot's; + // the builder routes a filter back to its own slot by widget type. Emitting + // only the content widgets deleted the filter of every custom-content column + // on a describe→exec round trip (ako/mxcli#489). + hasContent := len(col.ContentWidgets) > 0 || len(col.FilterWidgets) > 0 if hasContent { // Output column with content block @@ -986,6 +990,9 @@ func outputDataGrid2ColumnV3(ctx *ExecContext, prefix, colName string, col rawDa for _, widget := range col.ContentWidgets { outputWidgetMDLV3(ctx, widget, len(prefix)/2+1) } + for _, widget := range col.FilterWidgets { + outputWidgetMDLV3(ctx, widget, len(prefix)/2+1) + } fmt.Fprintf(ctx.Output, "%s}\n", prefix) } else { // Output simple column line diff --git a/mdl/executor/cmd_pages_describe_pluggable.go b/mdl/executor/cmd_pages_describe_pluggable.go index 28add25efa..3479741627 100644 --- a/mdl/executor/cmd_pages_describe_pluggable.go +++ b/mdl/executor/cmd_pages_describe_pluggable.go @@ -481,16 +481,25 @@ func extractDataGrid2Column(ctx *ExecContext, colObj map[string]any, colPropKeyM } } - // Check for Widgets array (content property for custom widgets) - if len(col.ContentWidgets) == 0 { - widgets := getBsonArrayElements(value["Widgets"]) - if len(widgets) > 0 { - for _, w := range widgets { - if wMap, ok := w.(map[string]any); ok { - col.ContentWidgets = append(col.ContentWidgets, parseRawWidget(ctx, wMap, entityContext)...) - } - } + // A column has TWO Widgets-typed slots — `content` (custom content) and + // `filter` — so route on the property key. Taking the first Widgets array + // instead dropped the filter of any column that also had custom content: + // the writer emits properties alphabetically, so `content` came first and + // won, and describe→exec then deleted the filter (ako/mxcli#489). + if propKey == "content" || propKey == "filter" { + widgets := parseColumnSlotWidgets(ctx, value, entityContext) + if propKey == "filter" { + col.FilterWidgets = append(col.FilterWidgets, widgets...) + } else { + col.ContentWidgets = append(col.ContentWidgets, widgets...) } + continue + } + + // Fallback for a document whose property keys did not resolve (no key map): + // the first Widgets array is the column's content. + if propKey == "" && len(col.ContentWidgets) == 0 { + col.ContentWidgets = append(col.ContentWidgets, parseColumnSlotWidgets(ctx, value, entityContext)...) } // Check for TextTemplate (could be header or dynamicText property) @@ -1387,3 +1396,17 @@ func anyCustomWidgetDataSource(w map[string]any) *rawDataSource { } return nil } + +// parseColumnSlotWidgets reads the widgets stored in one of a DataGrid2 column's +// Widgets-typed slots (`content` or `filter`). +func parseColumnSlotWidgets(ctx *ExecContext, value map[string]any, entityContext string) []rawWidget { + var out []rawWidget + for _, w := range getBsonArrayElements(value["Widgets"]) { + wMap, ok := w.(map[string]any) + if !ok { + continue + } + out = append(out, parseRawWidget(ctx, wMap, entityContext)...) + } + return out +} diff --git a/mdl/executor/cmd_rename.go b/mdl/executor/cmd_rename.go index bdb6e774ba..863e9b32d4 100644 --- a/mdl/executor/cmd_rename.go +++ b/mdl/executor/cmd_rename.go @@ -277,6 +277,11 @@ func execRenameDocument(ctx *ExecContext, s *ast.RenameStmt, docType string) err // execRenameEnumeration renames an enumeration and updates all references. func execRenameEnumeration(ctx *ExecContext, s *ast.RenameStmt) error { + // Platform built-in, no stored unit to rename (#1102). + if err := refuseSystemEnumerationWrite("rename enumeration", s.Name); err != nil { + return err + } + oldQualifiedName := s.Name.Module + "." + s.Name.Name newQualifiedName := s.Name.Module + "." + s.NewName diff --git a/mdl/executor/typecheck_test.go b/mdl/executor/typecheck_test.go index eb989d6a0c..9c1967d496 100644 --- a/mdl/executor/typecheck_test.go +++ b/mdl/executor/typecheck_test.go @@ -287,3 +287,240 @@ END; t.Errorf("an untypeable variable produced %+v", got) } } + +// TestTypeCheckProgramTypesLoopVariables pins mendixlabs/mxcli#1100. +// +// The report's title says the checker is skipped inside a LOOP body. It is not: +// the body is walked, and the same expression written against a PARAMETER +// inside the loop was refused before the fix — that control is the third case +// below, and it distinguishes "the walk does not reach here" from "the variable +// resolves to nothing". It was the second: `LOOP $r IN $reqs` never recorded +// `$r`, so `$r/Status` inferred Unknown, and Unknown is tolerated by every rule +// by design. +func TestTypeCheckProgramTypesLoopVariables(t *testing.T) { + exec := typeCheckFixture(t) + + // The reported script, verbatim in shape. Before the fix: Check passed!, + // exec wrote it, mxbuild reported CE0117 at the Change variable activity. + loopVar := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_LoopNormal () RETURNS String +BEGIN + DECLARE $out String = ''; + RETRIEVE $reqs FROM MyFirstModule.Ticket; + LOOP $r IN $reqs BEGIN + $out = $out + $r/Status; + END LOOP; + RETURN $out; +END; +`) + if len(loopVar) != 1 || loopVar[0].RuleID != "E004" { + t.Errorf("an Enumeration concatenated inside a LOOP produced %+v, want one E004", loopVar) + } + + // The report's case A, which already worked and must keep working. + param := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_Param ($Req: MyFirstModule.Ticket) RETURNS String +BEGIN + DECLARE $out String = ''; + $out = 'status=' + $Req/Status; + RETURN $out; +END; +`) + if len(param) != 1 || param[0].RuleID != "E004" { + t.Errorf("the parameter control produced %+v, want one E004", param) + } + + // The control that says the LOOP BODY was never the problem: a parameter + // referenced one line deeper is checked, and was before the fix too. + paramInLoop := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_ParamInLoop ($Req: MyFirstModule.Ticket) +BEGIN + RETRIEVE $reqs FROM MyFirstModule.Ticket; + LOOP $r IN $reqs BEGIN + LOG 'x {1}' WITH ({1} = 'status=' + $Req/Status); + END LOOP; +END; +`) + if len(paramInLoop) != 1 || paramInLoop[0].RuleID != "E004" { + t.Errorf("a parameter inside a LOOP produced %+v, want one E004", paramInLoop) + } + + // Every rule was off for a loop variable, not just E004. + enumCompare := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_LoopEnumCompare () +BEGIN + RETRIEVE $reqs FROM MyFirstModule.Ticket; + LOOP $r IN $reqs BEGIN + IF $r/Status = 'Open' THEN + LOG 'x'; + END IF; + END LOOP; +END; +`) + if len(enumCompare) != 1 || enumCompare[0].RuleID != "E001" { + t.Errorf("an enum compared to a string inside a LOOP produced %+v, want one E001", enumCompare) + } + + // The failure direction. A correct loop must stay silent — a checker that + // reports the fixed form is worse than one that reported nothing. + clean := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_LoopClean () RETURNS String +BEGIN + DECLARE $out String = ''; + RETRIEVE $reqs FROM MyFirstModule.Ticket; + LOOP $r IN $reqs BEGIN + $out = $out + toString($r/Status) + $r/Title; + IF $r/Status = MyFirstModule.OrderStatus.Open THEN + LOG 'open'; + END IF; + END LOOP; + RETURN $out; +END; +`) + if len(clean) != 0 { + t.Errorf("a correct loop produced %+v, want none", clean) + } +} + +// TestTypeCheckProgramTypesDeclaredVariables pins the second half of #1100. +// +// Typing the loop variable alone does NOT make the reported script report +// anything: E004 needs both operands known, and the accumulator `$out` was +// Unknown too. That is also why the same mistake was silent with no loop in +// sight — `$out = $out + $Req/Status` on a parameter was unreported before the +// fix, which the report's own case A hides by using a string literal. +func TestTypeCheckProgramTypesDeclaredVariables(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_Accumulate ($Req: MyFirstModule.Ticket) RETURNS String +BEGIN + DECLARE $out String = ''; + $out = $out + $Req/Status; + RETURN $out; +END; +`) + if len(got) != 1 || got[0].RuleID != "E004" { + t.Errorf("a declared String accumulator produced %+v, want one E004", got) + } + + // Mendix auto-converts a numeric operand in a String concat, so a declared + // Integer must not be reported. This is the boundary the rule already had; + // typing the variable is what puts it in reach of being crossed. + numeric := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_Numeric () RETURNS String +BEGIN + DECLARE $n Integer = 1; + DECLARE $out String = 'n=' + $n; + RETURN $out; +END; +`) + if len(numeric) != 0 { + t.Errorf("a numeric concat produced %+v, want none (Mendix auto-converts)", numeric) + } +} + +// TestTypeCheckProgramTypesDerivedLists pins the list sources a LOOP can +// iterate. The iterator is only as typed as the list it walks, so a retrieve +// over an association and a list operation have to carry their element type or +// the fix above covers one spelling of the same loop. +func TestTypeCheckProgramTypesDerivedLists(t *testing.T) { + exec := typeCheckFixture(t) + + // An association retrieve names the association, not the entity, so the far + // end is resolved through the association index. + assoc := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_LoopAssoc ($T: MyFirstModule.Ticket) +BEGIN + RETRIEVE $reps FROM $T/MyFirstModule.Ticket_Reporter; + LOOP $r IN $reps BEGIN + LOG 'x {1}' WITH ({1} = $r); + END LOOP; +END; +`) + if len(assoc) != 1 || assoc[0].RuleID != "E009" { + t.Errorf("a loop over an association retrieve produced %+v, want one E009", assoc) + } + + // FILTER carries the input's element type through. + filtered := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_LoopFiltered () +BEGIN + RETRIEVE $reqs FROM MyFirstModule.Ticket; + $open = FILTER($reqs, $currentObject/Title != ''); + LOOP $r IN $open BEGIN + LOG 'x {1}' WITH ({1} = 'status=' + $r/Status); + END LOOP; +END; +`) + if len(filtered) != 1 || filtered[0].RuleID != "E004" { + t.Errorf("a loop over a FILTER result produced %+v, want one E004", filtered) + } +} + +// TestTypeCheckProgramChecksBlockScopedBodies covers the two other block-scoped +// positions the report asked about: an ON ERROR handler's body, which was not +// walked at all, and a FIND/FILTER predicate, where $currentObject is now bound +// to the element type of the list under test. +func TestTypeCheckProgramChecksBlockScopedBodies(t *testing.T) { + exec := typeCheckFixture(t) + + handler := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_Handler ($T: MyFirstModule.Ticket) RETURNS String +BEGIN + DECLARE $out String = ''; + RETRIEVE $reqs FROM MyFirstModule.Ticket + ON ERROR { + $out = $out + $T/Status; + }; + RETURN $out; +END; +`) + if len(handler) != 1 || handler[0].RuleID != "E004" { + t.Errorf("an ON ERROR handler body produced %+v, want one E004", handler) + } + + predicate := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_Predicate () +BEGIN + RETRIEVE $reqs FROM MyFirstModule.Ticket; + $open = FILTER($reqs, $currentObject/Status = 'Open'); +END; +`) + if len(predicate) != 1 || predicate[0].RuleID != "E001" { + t.Errorf("a FILTER predicate produced %+v, want one E001", predicate) + } + + // The control: the same predicate written correctly stays silent, and so + // does the bare-attribute spelling the skills recommend, which resolves to + // nothing rather than to a wrong answer. + clean := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_PredicateClean () +BEGIN + RETRIEVE $reqs FROM MyFirstModule.Ticket; + $open = FILTER($reqs, $currentObject/Status = MyFirstModule.OrderStatus.Open); + $named = FILTER($reqs, "Title" != ''); +END; +`) + if len(clean) != 0 { + t.Errorf("a correct FILTER predicate produced %+v, want none", clean) + } + + // Mendix's STRING find(haystack, needle) still arrives as a + // ListOperationStmt — the visitor does not disambiguate it, the flow + // builder does, by looking at whether the input is a declared String + // (mdl-examples/bug-tests/ledger-63-string-find.mdl). Checking its second + // argument as a Boolean predicate reported the needle on a script that + // builds at 0 errors, which the corpus sweep caught and this pins. + stringFind := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.SUB_StringFind ($Hay: String, $Needle: String) RETURNS Integer +BEGIN + DECLARE $At Integer = 0; + SET $At = find($Hay, $Needle); + RETURN $At; +END; +`) + if len(stringFind) != 0 { + t.Errorf("Mendix's string find() produced %+v, want none", stringFind) + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index f9e4f1e49f..dc88e3f30d 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -9,6 +9,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/exprcheck" + "github.com/mendixlabs/mxcli/mdl/exprcheck/adapters" "github.com/mendixlabs/mxcli/mdl/linter" ) @@ -101,6 +102,8 @@ func (v *microflowValidator) addViolation(ruleID string, severity linter.Severit // validate runs all checks on the microflow body. func (v *microflowValidator) validate(body []ast.MicroflowStatement) { v.checkListOperationIterator(body) + v.checkRetrieveLimitOneAsList(body) + v.checkListOperationSource(body) v.checkMergeJoinLabels(body) v.checkAnnotationLabels(body) @@ -889,27 +892,11 @@ func microflowExprSource(expr ast.Expression) string { // astKindToExprKind maps an MDL primitive data-type kind to an exprcheck kind. // Returns false for non-primitive / unmappable kinds (entities, lists, void). +// +// The table lives in mdl/exprcheck/adapters because the catalog-backed checker +// needs the same answer; a second copy here is how the two would drift apart. func astKindToExprKind(k ast.DataTypeKind) (exprcheck.TypeKind, bool) { - switch k { - case ast.TypeString, ast.TypeStringTemplate: - return exprcheck.KindString, true - case ast.TypeInteger, ast.TypeAutoNumber: - return exprcheck.KindInteger, true - case ast.TypeLong: - return exprcheck.KindLong, true - case ast.TypeDecimal: - return exprcheck.KindDecimal, true - case ast.TypeBoolean: - return exprcheck.KindBoolean, true - case ast.TypeDateTime, ast.TypeDate: - return exprcheck.KindDateTime, true - case ast.TypeBinary: - return exprcheck.KindBinary, true - case ast.TypeEnumeration: - return exprcheck.KindEnumeration, true - default: - return exprcheck.KindUnknown, false - } + return adapters.DataTypeKind(k) } // checkErrorHandlingInLoop warns if custom error handling is used inside a loop. @@ -1340,57 +1327,12 @@ func exprVarRefs(expr ast.Expression) []string { } // stmtErrorHandling returns the ErrorHandlingClause for statements that support it. +// +// The table lives in mdl/exprcheck/adapters so the expression checker's walk and +// this one cannot disagree about which statements carry a handler: a statement +// missing from one copy is silently skipped by whichever walk holds it. func stmtErrorHandling(stmt ast.MicroflowStatement) *ast.ErrorHandlingClause { - switch s := stmt.(type) { - case *ast.CreateObjectStmt: - return s.ErrorHandling - case *ast.DeleteObjectStmt: - return s.ErrorHandling - case *ast.MfCommitStmt: - return s.ErrorHandling - case *ast.RetrieveStmt: - return s.ErrorHandling - case *ast.CallMicroflowStmt: - return s.ErrorHandling - case *ast.CallNanoflowStmt: - return s.ErrorHandling - case *ast.CallJavaActionStmt: - return s.ErrorHandling - case *ast.DownloadFileStmt: - return s.ErrorHandling - case *ast.SynchronizeStmt: - return s.ErrorHandling - case *ast.CallJavaScriptActionStmt: - return s.ErrorHandling - case *ast.CallWebServiceStmt: - return s.ErrorHandling - case *ast.ExecuteDatabaseQueryStmt: - return s.ErrorHandling - // The eight statements #1078 gave an onErrorClause. Without them here, MDL076 - // cannot see a clause these statements now accept, and MDL077 cannot refuse - // one on a list operation or aggregate. - case *ast.DeclareStmt: - return s.ErrorHandling - case *ast.MfSetStmt: - return s.ErrorHandling - case *ast.ChangeObjectStmt: - return s.ErrorHandling - case *ast.LogStmt: - return s.ErrorHandling - case *ast.ShowPageStmt: - return s.ErrorHandling - case *ast.ClosePageStmt: - return s.ErrorHandling - case *ast.ShowMessageStmt: - return s.ErrorHandling - case *ast.ValidationFeedbackStmt: - return s.ErrorHandling - case *ast.ListOperationStmt: - return s.ErrorHandling - case *ast.AggregateListStmt: - return s.ErrorHandling - } - return nil + return adapters.StatementErrorHandling(stmt) } // isEmptyInit checks if a variable initializer is empty/nil (used to detect "DECLARE $List List of ... = empty"). diff --git a/mdl/executor/validate_microflow_listop_source.go b/mdl/executor/validate_microflow_listop_source.go new file mode 100644 index 0000000000..313a68d335 --- /dev/null +++ b/mdl/executor/validate_microflow_listop_source.go @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// checkListOperationSource flags a list-operation or aggregate whose list +// operand is not a variable — MDL-LISTOP02, mendixlabs/mxcli#1101. +// +// MDL's expression grammar makes list operations look composable: +// +// $n = count(filter($reqs, $currentObject/Status = Mod.E.Approved)); +// +// Mendix's model is not. Each of those is a separate ACTIVITY, and an activity +// stores its list as a variable reference — Microflows$AggregateAction's +// AggregateVariableName, Microflows$ListOperationsAction's list property. There +// is no slot for a nested computation, so the inner call had nowhere to go and +// was dropped, list and predicate together. The activity was then written with +// an empty list, which is the defect's whole signature: `check` clean, `exec` +// printing "Created microflow", and the failure only at build time. +// +// Measured on mxbuild 11.6.6, one microflow per project: +// +// count(filter(…)) CE0012 "The 'List' property is required." +// head(filter(…)) CE0096, the list-operation flavour of the same +// sum(filter(…), 1) CE0012 + CE0117 +// sort(filter(…), Name) mxbuild ABORTS — InvalidOperationException on the +// sort attribute, which resolves against the (now +// absent) list's entity. No error code, no line: the +// document cannot be loaded at all. +// count('nonsense') CE0012 — nesting is not required to lose the list +// +// The control for all of them is the reporter's own workaround, which builds +// the same two activities explicitly and passes at 0 errors. +// +// Why refuse rather than materialise an implicit variable: the refusal covers +// every spelling from one rule, including the literal operand, which no amount +// of materialising would fix. And it needs no project — the answer is in the +// statement's own text — so plain `mxcli check` reports it. +func (v *microflowValidator) checkListOperationSource(body []ast.MicroflowStatement) { + forEachMicroflowStatement(body, func(s ast.MicroflowStatement) { + switch stmt := s.(type) { + case *ast.ListOperationStmt: + op := strings.ToLower(stmt.Operation.String()) + for _, u := range stmt.UnresolvedOperands { + v.reportUnresolvedListOperand(op, stmt.OutputVariable, u, "CE0096") + } + case *ast.AggregateListStmt: + op := strings.ToLower(stmt.Operation.String()) + for _, u := range stmt.UnresolvedOperands { + v.reportUnresolvedListOperand(op, stmt.OutputVariable, u, "CE0012") + } + } + }) +} + +// reportUnresolvedListOperand emits one MDL-LISTOP02 violation, naming what was +// written where a list variable belongs and printing the rewrite that works. +func (v *microflowValidator) reportUnresolvedListOperand(op, outputVar string, u ast.UnresolvedOperand, ce string) { + which := "list argument" + if u.Index == 1 { + which = "second list argument" + } + + src := microflowExprSource(u.Expr) + if src == "" { + v.addViolation("MDL-LISTOP02", linter.SeverityError, + fmt.Sprintf("%s(…): the %s is missing. A Mendix %s activity stores its list as a "+ + "variable reference, so mxbuild rejects an empty one with %s "+ + "\"The 'List' property is required.\".", op, which, activityNoun(op), ce), + fmt.Sprintf("Pass a list variable, e.g. $%s = %s($MyList).", displayVar(outputVar), op)) + return + } + + v.addViolation("MDL-LISTOP02", linter.SeverityError, + fmt.Sprintf("%s(…): the %s is `%s`, which is not a variable. A Mendix %s activity stores "+ + "its list as a variable reference and has no slot for a nested computation, so the "+ + "argument is dropped and the activity is written with an empty list — mxbuild then "+ + "rejects it with %s \"The 'List' property is required.\".", + op, which, src, activityNoun(op), ce), + unresolvedListOperandRemedy(op, outputVar, u, src)) +} + +// unresolvedListOperandRemedy prints the rewrite. It names the variable to +// introduce and which argument to put it in, rather than reconstructing the whole +// corrected statement: the operand's position differs per operation (filter and +// sort carry a predicate or a sort spec after the list, union carries a second +// list), so a reconstructed example would be wrong for most of them. +func unresolvedListOperandRemedy(op, outputVar string, u ast.UnresolvedOperand, src string) string { + name := "$" + displayVar(outputVar) + "_source" + if u.Index == 1 { + name = "$" + displayVar(outputVar) + "_second" + } + which := "list argument" + if u.Index == 1 { + which = "second list argument" + } + if isListOperationCall(u.Expr) { + return fmt.Sprintf("Give the inner operation its own statement and pass its variable: "+ + "`%s = %s;` then use `%s` as the %s of %s(…). Each list operation is a separate "+ + "activity in Mendix, so they cannot be nested in one expression.", + name, src, name, which, op) + } + return fmt.Sprintf("Assign a list to a variable and pass the variable as the %s of %s(…), "+ + "e.g. `%s = ;`.", which, op, name) +} + +// isListOperationCall reports whether an expression is a call to one of the list +// operations — the case where the remedy can name the exact rewrite. +func isListOperationCall(expr ast.Expression) bool { + call, ok := expr.(*ast.FunctionCallExpr) + if !ok { + return false + } + switch strings.ToUpper(call.Name) { + case "HEAD", "TAIL", "FIND", "FILTER", "SORT", "UNION", "INTERSECT", + "SUBTRACT", "RANGE", "COUNT", "SUM", "AVERAGE", "MINIMUM", "MAXIMUM": + return true + } + return false +} + +// activityNoun names the activity Mendix would build, for the diagnostic. +func activityNoun(op string) string { + switch op { + case "count", "sum", "average", "minimum", "maximum", "reduce", "all", "any": + return "aggregate list" + default: + return "list operation" + } +} + +// displayVar keeps the message readable when the statement has no output +// variable (a parse that got far enough to build the activity but not to name +// its result). +func displayVar(outputVar string) string { + if outputVar == "" { + return "Result" + } + return outputVar +} diff --git a/mdl/executor/validate_microflow_listop_source_test.go b/mdl/executor/validate_microflow_listop_source_test.go new file mode 100644 index 0000000000..582fa3d41c --- /dev/null +++ b/mdl/executor/validate_microflow_listop_source_test.go @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// listopSourceViolations parses real MDL, validates the microflow it declares +// and returns only the MDL-LISTOP02 messages. +// +// It goes through the visitor rather than hand-building the AST on purpose: the +// defect in mendixlabs/mxcli#1101 IS the conversion, so a test that writes the +// AST it wants would assert against the wrong side of the bug. +func listopSourceViolations(t *testing.T, src string) []string { + t.Helper() + prog := parseMDL(t, src) + var out []string + for _, stmt := range prog.Statements { + mf, ok := stmt.(*ast.CreateMicroflowStmt) + if !ok { + continue + } + for _, v := range ValidateMicroflow(mf) { + if v.RuleID == "MDL-LISTOP02" { + out = append(out, v.Message) + } + } + } + return out +} + +// microflowSrc wraps a body in a microflow that retrieves two lists, so each +// case below is only the statement under test. +func microflowSrc(name, body string) string { + return "create or replace microflow Shop." + name + `() +begin + retrieve $reqs from Shop.Request; + retrieve $others from Shop.Request; + ` + body + ` +end;` +} + +// TestNestedListOperandIsReported is mendixlabs/mxcli#1101 exactly as reported: +// COUNT over an inline FILTER. Measured before the fix — `check` clean, `exec` +// printed "Created microflow", and mxbuild 11.6.6 then failed the build with +// CE0012 "The 'List' property is required." at Aggregate list activity 'Count'. +func TestNestedListOperandIsReported(t *testing.T) { + got := listopSourceViolations(t, microflowSrc("CountFilter", + `$n = COUNT(FILTER($reqs, $currentObject/Status = Shop.ENUM_Status.Approved));`)) + if len(got) != 1 { + t.Fatalf("expected 1 MDL-LISTOP02 violation, got %d: %v", len(got), got) + } + for _, want := range []string{"count", "filter($reqs", "CE0012"} { + if !strings.Contains(got[0], want) { + t.Errorf("message %q does not mention %q", got[0], want) + } + } +} + +// TestNestedListOperandInEveryShape covers the spellings measured against +// mxbuild 11.6.6 while investigating #1101. The list operand is dropped by the +// same line in every one of them, so a fix that only handles COUNT leaves the +// rest silent. +func TestNestedListOperandInEveryShape(t *testing.T) { + cases := []struct { + name string + body string + // buildSymptom is what mxbuild did with the document before the fix. + buildSymptom string + }{ + {"CountOfFilter", `$n = COUNT(FILTER($reqs, $currentObject/Name != ''));`, "CE0012"}, + {"HeadOfFilter", `$h = HEAD(FILTER($reqs, $currentObject/Name != ''));`, "CE0096"}, + {"SumOfFilter", `$n = SUM(FILTER($reqs, $currentObject/Name != ''), 1);`, "CE0012 + CE0117"}, + {"SortOfFilter", `$s = SORT(FILTER($reqs, $currentObject/Name != ''), Name);`, "mxbuild abort"}, + {"FilterOfSort", `$f = FILTER(SORT($reqs, Name), $currentObject/Name != '');`, "CE0096"}, + {"TailOfFilter", `$t = TAIL(FILTER($reqs, $currentObject/Name != ''));`, "CE0096"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := listopSourceViolations(t, microflowSrc(tc.name, tc.body)) + if len(got) != 1 { + t.Fatalf("%s (mxbuild: %s): expected 1 MDL-LISTOP02 violation, got %d: %v", + tc.body, tc.buildSymptom, len(got), got) + } + }) + } +} + +// TestLiteralListOperandIsReported: nesting is not required to lose the list. +// `COUNT('nonsense')` reached mxbuild as an aggregate with an empty List and the +// same CE0012 — so the rule keys on "did not reduce to a variable", not on the +// operand being a call. +func TestLiteralListOperandIsReported(t *testing.T) { + got := listopSourceViolations(t, microflowSrc("CountLiteral", `$n = COUNT('nonsense');`)) + if len(got) != 1 { + t.Fatalf("expected 1 MDL-LISTOP02 violation, got %d: %v", len(got), got) + } +} + +// TestSecondListOperandIsReported covers the two-list operations, where the +// dropped operand is the SECOND one — `union($others, filter(…))` stored +// $others and an empty second list. +func TestSecondListOperandIsReported(t *testing.T) { + got := listopSourceViolations(t, microflowSrc("UnionFilter", + `$u = UNION($others, FILTER($reqs, $currentObject/Name != ''));`)) + if len(got) != 1 { + t.Fatalf("expected 1 MDL-LISTOP02 violation, got %d: %v", len(got), got) + } + if !strings.Contains(got[0], "second") { + t.Errorf("message %q does not say which operand was dropped", got[0]) + } +} + +// TestResolvedListOperandsAreNotReported is the control. Every one of these is a +// spelling mxbuild accepts at 0 errors (the two-variable form is the workaround +// the reporter found), so a rule that fires here would be worse than the bug. +func TestResolvedListOperandsAreNotReported(t *testing.T) { + bodies := []string{ + // The reporter's own workaround. + `$approved = FILTER($reqs, $currentObject/Name != ''); + $n = COUNT($approved);`, + `$n = COUNT($reqs);`, + `$h = HEAD($reqs);`, + `$s = SORT($reqs, Name);`, + `$u = UNION($reqs, $others);`, + `$i = INTERSECT($reqs, $others);`, + `$r = RANGE($reqs, 0, 10);`, + `$f = FILTER($reqs, $currentObject/Name != '');`, + // Aggregates over an attribute path and over a per-item expression: + // buildSetAggregate resolves both, so neither is a dropped operand. + `$n = SUM($reqs.Amount);`, + `$n = SUM($reqs, $currentObject/Amount * 2);`, + // String functions that share a name with a list operation must stay + // value expressions — they never become a list activity at all. + `$p = find('haystack', 'needle');`, + `$b = contains($reqs/Name, 'x');`, + } + for i, body := range bodies { + got := listopSourceViolations(t, microflowSrc("Control", body)) + if len(got) != 0 { + t.Errorf("case %d %q: expected no MDL-LISTOP02 violation, got %v", i, body, got) + } + } +} diff --git a/mdl/executor/validate_microflow_retrieve_single.go b/mdl/executor/validate_microflow_retrieve_single.go new file mode 100644 index 0000000000..d9edf2a2c4 --- /dev/null +++ b/mdl/executor/validate_microflow_retrieve_single.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// retrieveSingleRule is the rule ID for "a LIMIT 1 retrieve used as a list". +const retrieveSingleRule = "MDL-RETRIEVE01" + +// checkRetrieveLimitOneAsList flags a variable that `RETRIEVE … LIMIT 1` bound to +// a single OBJECT and a later statement uses as a LIST. +// +// `limit 1` is not a one-element list here. The executor maps it to Mendix's +// "First object" range (RangeTypeFirst, cmd_microflows_builder_actions.go), which +// makes the output variable an object — and that is deliberate and documented +// (MDL_QUICK_REFERENCE.md), not something to change under anyone's feet. +// +// What was missing is any sign of it before the build. Nothing in the MDL says +// the variable changed shape: `mxcli check --references` passed, and DESCRIBE +// re-emits `limit 1`, so the source of an object retrieve and a list retrieve are +// identical text. The first thing the author saw was CE0097 "The selected 'x' +// variable must be of type List" from mxbuild — and inside a .test.mdl file, not +// even that: the injected test simply failed to build (mendixlabs/mxcli#1103). +// +// The clause is also spelled the other way round elsewhere in the same language — +// `import from mapping … first` binds an object and `… limit 1` a one-element +// list — so reading it as a list is a reasonable mistake rather than a careless +// one. The message therefore names the working spelling instead of only refusing. +// +// Keyed on exactly the condition the writer uses (limit "1", no offset), because +// a check that disagrees with the writer it describes is worse than no check. +func (v *microflowValidator) checkRetrieveLimitOneAsList(body []ast.MicroflowStatement) { + // single holds the variables currently bound to one object by a LIMIT 1 + // retrieve. Maintained in statement order so a rebinding clears it: a name + // reused for a real list further down is not this rule's business. + single := map[string]bool{} + + forEachMicroflowStatement(body, func(s ast.MicroflowStatement) { + if name, op := listUseOf(s); name != "" && single[name] { + v.addViolation(retrieveSingleRule, linter.SeverityError, + fmt.Sprintf("$%s was retrieved with LIMIT 1, which binds a single object rather than a "+ + "one-element list, so %s cannot take it — mxbuild rejects this with CE0097 "+ + "\"The selected '%s' variable must be of type List\".", name, op, name), + fmt.Sprintf("Drop the LIMIT to retrieve a list and keep %s, or keep LIMIT 1 and use "+ + "$%s as the object it already is.", op, name)) + } + + // Rebinding first, so a statement that both consumes and produces the + // name is judged on what it consumed. + for _, p := range statementProducedVars(s) { + delete(single, p.name) + } + if r, ok := s.(*ast.RetrieveStmt); ok && r.Limit == "1" && r.Offset == "" && r.Variable != "" { + single[r.Variable] = true + } + }) +} + +// listUseOf reports the list variable a statement consumes, and a phrase naming +// what consumes it. ("", "") when the statement takes no list. +func listUseOf(s ast.MicroflowStatement) (string, string) { + switch st := s.(type) { + case *ast.ListOperationStmt: + if st.InputVariable != "" { + return st.InputVariable, st.Operation.String() + "()" + } + if st.SecondVariable != "" { + return st.SecondVariable, st.Operation.String() + "()" + } + case *ast.AggregateListStmt: + if st.InputVariable != "" { + return st.InputVariable, st.Operation.String() + "()" + } + case *ast.LoopStmt: + if st.ListVariable != "" { + return st.ListVariable, "a loop" + } + case *ast.AddToListStmt: + if st.List != "" { + return st.List, "ADD … TO" + } + case *ast.RemoveFromListStmt: + if st.List != "" { + return st.List, "REMOVE … FROM" + } + } + return "", "" +} diff --git a/mdl/executor/validate_microflow_retrieve_single_test.go b/mdl/executor/validate_microflow_retrieve_single_test.go new file mode 100644 index 0000000000..e034d0165f --- /dev/null +++ b/mdl/executor/validate_microflow_retrieve_single_test.go @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +func retrieveSingleViolations(t *testing.T, stmt *ast.CreateMicroflowStmt) []string { + t.Helper() + var out []string + for _, v := range ValidateMicroflow(stmt) { + if v.RuleID == retrieveSingleRule { + out = append(out, v.Message) + } + } + return out +} + +func mfWith(body ...ast.MicroflowStatement) *ast.CreateMicroflowStmt { + return &ast.CreateMicroflowStmt{ + Name: ast.QualifiedName{Module: "Probe", Name: "M"}, + Body: body, + } +} + +func retrieveLimit(variable, limit, offset string) *ast.RetrieveStmt { + return &ast.RetrieveStmt{ + Variable: variable, + Source: ast.QualifiedName{Module: "Probe", Name: "Request"}, + Limit: limit, + Offset: offset, + } +} + +// TestRetrieveLimitOneUsedAsList is mendixlabs/mxcli#1103's real defect. +// +// `RETRIEVE $x … LIMIT 1` is compiled to Mendix's "First object" range, so $x is +// an OBJECT, not a one-element list. Nothing between the author and mxbuild said +// so: `mxcli check --references` passed, and `describe` re-emits `limit 1`, so +// the source looks identical to a list retrieve. The first sign was CE0097 at +// the far end of a build — and, in a .test.mdl file, a test that failed to +// inject with no explanation at all. +func TestRetrieveLimitOneUsedAsList(t *testing.T) { + t.Run("HEAD of a LIMIT 1 retrieve is flagged", func(t *testing.T) { + got := retrieveSingleViolations(t, mfWith( + retrieveLimit("reqs", "1", ""), + &ast.ListOperationStmt{Operation: ast.ListOpHead, InputVariable: "reqs", OutputVariable: "req"}, + )) + if len(got) != 1 { + t.Fatalf("got %d violations %q, want 1", len(got), got) + } + for _, want := range []string{"$reqs", "LIMIT 1", "CE0097"} { + if !strings.Contains(got[0], want) { + t.Errorf("message %q does not mention %q", got[0], want) + } + } + }) + + t.Run("looping over a LIMIT 1 retrieve is flagged", func(t *testing.T) { + got := retrieveSingleViolations(t, mfWith( + retrieveLimit("reqs", "1", ""), + &ast.LoopStmt{LoopVariable: "r", ListVariable: "reqs"}, + )) + if len(got) != 1 { + t.Fatalf("got %d violations %q, want 1", len(got), got) + } + }) + + t.Run("aggregating a LIMIT 1 retrieve is flagged", func(t *testing.T) { + got := retrieveSingleViolations(t, mfWith( + retrieveLimit("reqs", "1", ""), + &ast.AggregateListStmt{Operation: ast.AggregateCount, InputVariable: "reqs", OutputVariable: "n"}, + )) + if len(got) != 1 { + t.Fatalf("got %d violations %q, want 1", len(got), got) + } + }) +} + +// TestRetrieveLimitOneControls are the cases that must NOT be flagged. Each is a +// shape the rule would swallow if it keyed on the wrong thing. +func TestRetrieveLimitOneControls(t *testing.T) { + cases := map[string]*ast.CreateMicroflowStmt{ + // The reporter's own control: without LIMIT the retrieve is a list, and + // this is the single most common shape in every test suite. + "no limit": mfWith( + retrieveLimit("reqs", "", ""), + &ast.ListOperationStmt{Operation: ast.ListOpHead, InputVariable: "reqs", OutputVariable: "req"}, + ), + // LIMIT 2 is a CustomRange, which is a list however small. + "limit 2": mfWith( + retrieveLimit("reqs", "2", ""), + &ast.ListOperationStmt{Operation: ast.ListOpHead, InputVariable: "reqs", OutputVariable: "req"}, + ), + // An offset forces CustomRange even at limit 1 — the executor's own + // condition, so the rule must use the same one or it will disagree with + // the writer it is describing. + "limit 1 with offset": mfWith( + retrieveLimit("reqs", "1", "5"), + &ast.ListOperationStmt{Operation: ast.ListOpHead, InputVariable: "reqs", OutputVariable: "req"}, + ), + // Using it as an object is exactly right and must stay silent. + "used as an object": mfWith( + retrieveLimit("req", "1", ""), + &ast.MfCommitStmt{Variable: "req"}, + ), + // Rebound to a real list before the list use. + "rebound to a list": mfWith( + retrieveLimit("reqs", "1", ""), + &ast.CreateListStmt{Variable: "reqs", EntityType: ast.QualifiedName{Module: "Probe", Name: "Request"}}, + &ast.ListOperationStmt{Operation: ast.ListOpHead, InputVariable: "reqs", OutputVariable: "req"}, + ), + } + for name, stmt := range cases { + t.Run(name, func(t *testing.T) { + if got := retrieveSingleViolations(t, stmt); len(got) != 0 { + t.Errorf("flagged a valid microflow: %q", got) + } + }) + } +} diff --git a/mdl/exprcheck/adapters/adapter_scope.go b/mdl/exprcheck/adapters/adapter_scope.go index 53b3c68c2a..87e32511d9 100644 --- a/mdl/exprcheck/adapters/adapter_scope.go +++ b/mdl/exprcheck/adapters/adapter_scope.go @@ -9,41 +9,277 @@ import ( "github.com/mendixlabs/mxcli/mdl/exprcheck" ) -// buildVarEntityScope walks a microflow body and records every variable -// known to hold an entity instance, mapping varName → entity QN. +// flowScope is one flow's variable type environment: which entity a variable +// holds, and — for a variable holding a primitive — which kind. // -// Sources covered: -// - CreateObjectStmt (Variable ← EntityType) -// - RetrieveStmt with $var = retrieve … from (Variable ← EntityType) +// The two halves are separate because exprcheck consumes them through separate +// seams (EntityScope and Scope), and because the questions differ: an object +// variable is resolved against the catalog per attribute, while a primitive is +// its own answer. +type flowScope struct { + entities map[string]string + kinds map[string]exprcheck.TypeKind +} + +func newFlowScope() *flowScope { + return &flowScope{ + entities: map[string]string{}, + kinds: map[string]exprcheck.TypeKind{}, + } +} + +// buildFlowScope walks a microflow body in statement order and records every +// variable it can type. +// +// Order matters: a variable is typed from what introduced it, and what +// introduced it is always an earlier statement — `LOOP $r IN $reqs` can only +// type `$r` once `RETRIEVE $reqs` has been seen. The walk descends into nested +// bodies at the point they appear, so an inner loop over a list built inside an +// outer loop resolves too. // -// The map is best-effort. An empty entry means "unknown" and the caller -// should fall back to a slot path without entity.attr enrichment. -func buildVarEntityScope(body []ast.MicroflowStatement) map[string]string { - scope := map[string]string{} +// The map is best-effort: an absent entry means "unknown", and every exprcheck +// rule tolerates Unknown by design. It is never a licence to guess — a wrong +// entry produces a false positive on code that builds, which costs more than +// the silence it replaces. +func buildFlowScope(body []ast.MicroflowStatement, params []ast.MicroflowParam, assoc associationResolver) *flowScope { + s := newFlowScope() + // Parameters are seeded BEFORE the walk, not after it. They are in scope + // from the first statement, and a body statement can be typed FROM one — + // `RETRIEVE $reps FROM $T/Mod.Ticket_Reporter` resolves only if `$T` is + // already known. Appending them afterwards left every such retrieve, and + // every loop over its result, untyped. + addParamTypes(s, params) var walk func([]ast.MicroflowStatement) walk = func(stmts []ast.MicroflowStatement) { - for _, s := range stmts { - switch n := s.(type) { + for _, st := range stmts { + switch n := st.(type) { case *ast.CreateObjectStmt: if n.Variable != "" { - scope[n.Variable] = n.EntityType.String() + s.entities[n.Variable] = n.EntityType.String() + } + case *ast.CreateListStmt: + if n.Variable != "" { + s.entities[n.Variable] = n.EntityType.String() } case *ast.RetrieveStmt: - if n.Variable != "" && n.StartVariable == "" && n.Source.Name != "" { - scope[n.Variable] = n.Source.String() + s.recordRetrieve(n, assoc) + case *ast.ListOperationStmt: + s.recordListOperation(n) + case *ast.DeclareStmt: + s.recordDeclare(n) + case *ast.LoopStmt: + // The iterator's type is the element type of the list it walks. + // Without this every rule the checker enforces is silently off + // for `$r/Attr` inside the body — not because the body is + // skipped (it is walked), but because the variable resolves to + // nothing and Unknown is tolerated everywhere + // (mendixlabs/mxcli#1100). + if n.LoopVariable != "" && n.ListVariable != "" { + if qn, ok := s.entities[strings.TrimPrefix(n.ListVariable, "$")]; ok && qn != "" { + s.entities[n.LoopVariable] = qn + } } + walk(n.Body) case *ast.IfStmt: walk(n.ThenBody) walk(n.ElseBody) case *ast.WhileStmt: walk(n.Body) - case *ast.LoopStmt: - walk(n.Body) + } + // A custom ON ERROR body is a block of ordinary statements, so the + // variables it introduces are typed the same way. It is walked last + // because it runs after the statement that carries it. + if eb := errorHandlerBody(st); eb != nil { + walk(eb) } } } walk(body) - return scope + return s +} + +// recordRetrieve types a RETRIEVE's output variable. +// +// A database retrieve names its entity outright. An association retrieve +// (`RETRIEVE $reps FROM $T/Mod.Ticket_Reporter`) names only the association, so +// the entity at the far end has to be resolved through the association index — +// the same hop an expression path makes, and the reason the resolver is passed +// down here rather than being consulted only at expression level. +func (s *flowScope) recordRetrieve(n *ast.RetrieveStmt, assoc associationResolver) { + if n.Variable == "" || n.Source.Name == "" { + return + } + if n.StartVariable == "" { + s.entities[n.Variable] = n.Source.String() + return + } + if assoc == nil { + return + } + from, ok := s.entities[strings.TrimPrefix(n.StartVariable, "$")] + if !ok || from == "" { + return + } + if target, ok := assoc.AssociationTarget(n.Source.String(), from); ok && target != "" { + s.entities[n.Variable] = target + } +} + +// recordListOperation types a list operation's output. +// +// The operations split three ways: most carry the input's element type through +// (a filtered list of Orders is still Orders), CONTAINS and EQUALS answer a +// Boolean, and the rest are left alone. Nothing is inferred for an operation +// whose input was never typed. +func (s *flowScope) recordListOperation(n *ast.ListOperationStmt) { + if n.OutputVariable == "" { + return + } + switch n.Operation { + case ast.ListOpContains, ast.ListOpEquals: + s.kinds[n.OutputVariable] = exprcheck.KindBoolean + return + case ast.ListOpHead, ast.ListOpTail, ast.ListOpFind, ast.ListOpFilter, + ast.ListOpSort, ast.ListOpUnion, ast.ListOpIntersect, ast.ListOpSubtract, + ast.ListOpRange: + if n.InputVariable == "" { + return + } + if qn, ok := s.entities[strings.TrimPrefix(n.InputVariable, "$")]; ok && qn != "" { + s.entities[n.OutputVariable] = qn + } + } +} + +// recordDeclare types a DECLARE'd variable from the type it was written with. +// +// This is what makes `$out + $Order/Status` reportable: E004 and the slot rules +// need BOTH operands typed, and a locally declared String was Unknown, so the +// most ordinary shape in the language — accumulate into a String — was exempt +// from every rule (mendixlabs/mxcli#1100). +func (s *flowScope) recordDeclare(n *ast.DeclareStmt) { + if n.Variable == "" { + return + } + switch { + case n.Type.EntityRef != nil: + s.entities[n.Variable] = n.Type.EntityRef.String() + case n.Type.Kind == ast.TypeEnumeration && n.Type.EnumRef != nil: + // A bare qualified name parses as TypeEnumeration with EnumRef set and + // cannot be told from an entity (see CLAUDE.md). The ENTITY guess is + // free — a name that is really an enumeration resolves no attributes — + // but the KIND is not, so it is recorded only for the unambiguous + // spelling ExplicitEnum marks. Calling an entity variable an + // Enumeration would put the wrong type name in a rule's message. + s.entities[n.Variable] = n.Type.EnumRef.String() + if n.Type.ExplicitEnum { + s.kinds[n.Variable] = exprcheck.KindEnumeration + } + default: + if k, ok := DataTypeKind(n.Type.Kind); ok { + s.kinds[n.Variable] = k + } + } +} + +// StatementErrorHandling returns the ON ERROR clause a statement carries, or +// nil when it carries none. +// +// The clause is declared per statement type rather than on an interface, so +// this is a type switch — and a statement missing from it is invisible to every +// caller at once, which is why there is one table rather than one per walk. +// mdl/executor's validators call it too. +func StatementErrorHandling(stmt ast.MicroflowStatement) *ast.ErrorHandlingClause { + switch s := stmt.(type) { + case *ast.CreateObjectStmt: + return s.ErrorHandling + case *ast.DeleteObjectStmt: + return s.ErrorHandling + case *ast.MfCommitStmt: + return s.ErrorHandling + case *ast.RetrieveStmt: + return s.ErrorHandling + case *ast.CallMicroflowStmt: + return s.ErrorHandling + case *ast.CallNanoflowStmt: + return s.ErrorHandling + case *ast.CallJavaActionStmt: + return s.ErrorHandling + case *ast.DownloadFileStmt: + return s.ErrorHandling + case *ast.SynchronizeStmt: + return s.ErrorHandling + case *ast.CallJavaScriptActionStmt: + return s.ErrorHandling + case *ast.CallWebServiceStmt: + return s.ErrorHandling + case *ast.ExecuteDatabaseQueryStmt: + return s.ErrorHandling + // The eight statements #1078 gave an onErrorClause. Without them here, MDL076 + // cannot see a clause these statements now accept, and MDL077 cannot refuse + // one on a list operation or aggregate. + case *ast.DeclareStmt: + return s.ErrorHandling + case *ast.MfSetStmt: + return s.ErrorHandling + case *ast.ChangeObjectStmt: + return s.ErrorHandling + case *ast.LogStmt: + return s.ErrorHandling + case *ast.ShowPageStmt: + return s.ErrorHandling + case *ast.ClosePageStmt: + return s.ErrorHandling + case *ast.ShowMessageStmt: + return s.ErrorHandling + case *ast.ValidationFeedbackStmt: + return s.ErrorHandling + case *ast.ListOperationStmt: + return s.ErrorHandling + case *ast.AggregateListStmt: + return s.ErrorHandling + } + return nil +} + +// errorHandlerBody returns a statement's custom ON ERROR body, or nil when it +// has no clause or the clause is one of the bodyless forms (CONTINUE/ROLLBACK). +func errorHandlerBody(stmt ast.MicroflowStatement) []ast.MicroflowStatement { + eh := StatementErrorHandling(stmt) + if eh == nil || len(eh.Body) == 0 { + return nil + } + return eh.Body +} + +// DataTypeKind maps an MDL primitive data-type kind to an exprcheck kind, +// reporting false for kinds that have no primitive answer (entities, lists, +// void, type parameters). +// +// It lives here rather than in mdl/executor because both the scope-local +// validator and this adapter need the same answer, and two copies of a type +// mapping is how a resolver drifts. +func DataTypeKind(k ast.DataTypeKind) (exprcheck.TypeKind, bool) { + switch k { + case ast.TypeString, ast.TypeStringTemplate: + return exprcheck.KindString, true + case ast.TypeInteger, ast.TypeAutoNumber: + return exprcheck.KindInteger, true + case ast.TypeLong: + return exprcheck.KindLong, true + case ast.TypeDecimal: + return exprcheck.KindDecimal, true + case ast.TypeBoolean: + return exprcheck.KindBoolean, true + case ast.TypeDateTime, ast.TypeDate: + return exprcheck.KindDateTime, true + case ast.TypeBinary: + return exprcheck.KindBinary, true + case ast.TypeEnumeration: + return exprcheck.KindEnumeration, true + default: + return exprcheck.KindUnknown, false + } } // entityScope adapts a variable→entity map plus an association resolver to @@ -74,9 +310,19 @@ func (e entityScope) AssociationTarget(assocQN, fromEntityQN string) (string, bo return e.assoc.AssociationTarget(assocQN, fromEntityQN) } -// addParamEntities records the entity a parameter holds. +// kindScope adapts a variable→kind map to exprcheck.Scope. +type kindScope map[string]exprcheck.TypeKind + +var _ exprcheck.Scope = kindScope{} + +func (k kindScope) Lookup(name string) (exprcheck.TypeKind, bool) { + v, ok := k[strings.TrimPrefix(name, "$")] + return v, ok && v != exprcheck.KindUnknown +} + +// addParamTypes records what a parameter holds. // -// buildVarEntityScope walks only the body, so it sees a variable a CREATE or +// buildFlowScope walks only the body, so it sees a variable a CREATE or // RETRIEVE introduced and misses every parameter — and a microflow that takes // its object as a parameter is the ordinary case, not an edge one. // @@ -85,16 +331,30 @@ func (e entityScope) AssociationTarget(assocQN, fromEntityQN string) (string, bo // CLAUDE.md). Both spellings are recorded rather than guessed between — a name // that turns out to be an enumeration simply resolves no attributes, so the // wrong guess costs nothing. -func addParamEntities(scope map[string]string, params []ast.MicroflowParam) { +// +// It is called from buildFlowScope before the body walk; see the note there on +// why the order is load-bearing. +// +// A `list of Mod.Entity` parameter records the ELEMENT entity, which is what a +// LOOP over it needs; the list itself is not an object and nothing resolves an +// attribute against it. +func addParamTypes(s *flowScope, params []ast.MicroflowParam) { for _, p := range params { if p.Name == "" { continue } switch { case p.Type.EntityRef != nil: - scope[p.Name] = p.Type.EntityRef.String() + s.entities[p.Name] = p.Type.EntityRef.String() case p.Type.Kind == ast.TypeEnumeration && p.Type.EnumRef != nil: - scope[p.Name] = p.Type.EnumRef.String() + s.entities[p.Name] = p.Type.EnumRef.String() + if p.Type.ExplicitEnum { + s.kinds[p.Name] = exprcheck.KindEnumeration + } + default: + if k, ok := DataTypeKind(p.Type.Kind); ok { + s.kinds[p.Name] = k + } } } } diff --git a/mdl/exprcheck/adapters/adapter_scope_test.go b/mdl/exprcheck/adapters/adapter_scope_test.go new file mode 100644 index 0000000000..8019ea03a8 --- /dev/null +++ b/mdl/exprcheck/adapters/adapter_scope_test.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 + +package adapters + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/exprcheck" +) + +func qn(mod, name string) ast.QualifiedName { return ast.QualifiedName{Module: mod, Name: name} } + +// assocStub answers one association, in one direction. +type assocStub struct{ assoc, from, to string } + +func (a assocStub) AssociationTarget(assocQN, fromEntityQN string) (string, bool) { + if assocQN == a.assoc && fromEntityQN == a.from { + return a.to, true + } + return "", false +} + +// TestBuildFlowScopeTypesLoopVariable is the unit-level statement of +// mendixlabs/mxcli#1100: the iterator takes the element type of the list it +// walks, or nothing downstream of it can be typed. +func TestBuildFlowScopeTypesLoopVariable(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.RetrieveStmt{Variable: "reqs", Source: qn("Probe", "Request")}, + &ast.LoopStmt{LoopVariable: "r", ListVariable: "reqs"}, + } + s := buildFlowScope(body, nil, nil) + if got := s.entities["r"]; got != "Probe.Request" { + t.Errorf("loop variable typed %q, want Probe.Request", got) + } +} + +// TestBuildFlowScopeSeedsParametersFirst pins an ordering that is easy to get +// wrong and silent when it is: parameters must be in scope before the body +// walk, because a body statement can be typed FROM one. Appending them +// afterwards left every association retrieve off a parameter untyped, and with +// it every loop over the result. +func TestBuildFlowScopeSeedsParametersFirst(t *testing.T) { + params := []ast.MicroflowParam{ + {Name: "T", Type: ast.DataType{Kind: ast.TypeEntity, EntityRef: &ast.QualifiedName{Module: "Probe", Name: "Request"}}}, + } + body := []ast.MicroflowStatement{ + &ast.RetrieveStmt{Variable: "reps", Source: qn("Probe", "Request_Reporter"), StartVariable: "T"}, + &ast.LoopStmt{LoopVariable: "rep", ListVariable: "reps"}, + } + s := buildFlowScope(body, params, assocStub{"Probe.Request_Reporter", "Probe.Request", "Probe.Reporter"}) + if got := s.entities["reps"]; got != "Probe.Reporter" { + t.Errorf("association retrieve typed %q, want Probe.Reporter", got) + } + if got := s.entities["rep"]; got != "Probe.Reporter" { + t.Errorf("loop over an association retrieve typed %q, want Probe.Reporter", got) + } +} + +// TestBuildFlowScopeTypesDeclaredPrimitives pins the other half of #1100: a +// rule that needs both operands typed (E004) was silent on `$out + ` +// because a locally declared String was Unknown. +func TestBuildFlowScopeTypesDeclaredPrimitives(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.DeclareStmt{Variable: "out", Type: ast.DataType{Kind: ast.TypeString}}, + &ast.DeclareStmt{Variable: "n", Type: ast.DataType{Kind: ast.TypeInteger}}, + &ast.DeclareStmt{Variable: "obj", Type: ast.DataType{Kind: ast.TypeEntity, EntityRef: &ast.QualifiedName{Module: "Probe", Name: "Request"}}}, + } + s := buildFlowScope(body, nil, nil) + if got := s.kinds["out"]; got != exprcheck.KindString { + t.Errorf("declared String typed %v, want KindString", got) + } + if got := s.kinds["n"]; got != exprcheck.KindInteger { + t.Errorf("declared Integer typed %v, want KindInteger", got) + } + // An entity-typed DECLARE belongs to the entity half, not the kind half. + if got := s.entities["obj"]; got != "Probe.Request" { + t.Errorf("declared entity typed %q, want Probe.Request", got) + } + if _, ok := s.kinds["obj"]; ok { + t.Errorf("an entity-typed DECLARE should have no primitive kind") + } +} + +// TestBuildFlowScopeCarriesElementTypeThroughListOperations pins which +// operations preserve the element type and which answer a Boolean instead. A +// loop is only as typed as the list it walks, so a FILTER that lost the entity +// would leave the fix covering one spelling of the same loop. +func TestBuildFlowScopeCarriesElementTypeThroughListOperations(t *testing.T) { + for _, tc := range []struct { + op ast.ListOperationType + wantQN string + wantKind exprcheck.TypeKind + hasEntity bool + }{ + {ast.ListOpFilter, "Probe.Request", exprcheck.KindUnknown, true}, + {ast.ListOpSort, "Probe.Request", exprcheck.KindUnknown, true}, + {ast.ListOpHead, "Probe.Request", exprcheck.KindUnknown, true}, + {ast.ListOpRange, "Probe.Request", exprcheck.KindUnknown, true}, + {ast.ListOpContains, "", exprcheck.KindBoolean, false}, + {ast.ListOpEquals, "", exprcheck.KindBoolean, false}, + } { + body := []ast.MicroflowStatement{ + &ast.RetrieveStmt{Variable: "reqs", Source: qn("Probe", "Request")}, + &ast.ListOperationStmt{OutputVariable: "out", Operation: tc.op, InputVariable: "reqs"}, + } + s := buildFlowScope(body, nil, nil) + if got := s.entities["out"]; got != tc.wantQN { + t.Errorf("%v: entity %q, want %q", tc.op, got, tc.wantQN) + } + if tc.wantKind != exprcheck.KindUnknown { + if got := s.kinds["out"]; got != tc.wantKind { + t.Errorf("%v: kind %v, want %v", tc.op, got, tc.wantKind) + } + } + } +} + +// TestBuildFlowScopeWalksErrorHandlerBodies pins that a variable introduced in +// an ON ERROR handler is typed like any other. The handler's body is ordinary +// statements; leaving it out made moving a statement into one an exemption. +func TestBuildFlowScopeWalksErrorHandlerBodies(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.RetrieveStmt{ + Variable: "reqs", Source: qn("Probe", "Request"), + ErrorHandling: &ast.ErrorHandlingClause{Body: []ast.MicroflowStatement{ + &ast.CreateObjectStmt{Variable: "fallback", EntityType: qn("Probe", "Request")}, + }}, + }, + } + s := buildFlowScope(body, nil, nil) + if got := s.entities["fallback"]; got != "Probe.Request" { + t.Errorf("a variable created in an ON ERROR body typed %q, want Probe.Request", got) + } +} + +// TestBuildFlowScopeLeavesAnUnresolvableLoopAlone is the failure direction. A +// loop over a list nothing typed, or over an association path the AST does not +// record (`LOOP $r IN $T/Mod.Assoc` leaves ListVariable empty), must produce no +// entry — a guess here is a false positive on code that builds. +func TestBuildFlowScopeLeavesAnUnresolvableLoopAlone(t *testing.T) { + body := []ast.MicroflowStatement{ + &ast.LoopStmt{LoopVariable: "r", ListVariable: "unknown"}, + &ast.LoopStmt{LoopVariable: "p", ListVariable: ""}, + } + s := buildFlowScope(body, nil, nil) + for _, v := range []string{"r", "p"} { + if qn, ok := s.entities[v]; ok { + t.Errorf("an unresolvable loop variable %q was typed %q", v, qn) + } + } +} + +// TestBuildFlowScopeDoesNotCallAnAmbiguousNameAnEnumeration pins the one place +// a guess would be wrong in a way that shows. `DECLARE $o Mod.Person` parses as +// TypeEnumeration with EnumRef set — the visitor cannot tell it from an +// enumeration (see CLAUDE.md) — so the ENTITY guess is recorded (it resolves no +// attributes if wrong, costing nothing) but the KIND is not, or a rule would +// report an object as an "Enumeration". `ENUM Mod.Status` sets ExplicitEnum and +// is unambiguous, so it does get a kind. +func TestBuildFlowScopeDoesNotCallAnAmbiguousNameAnEnumeration(t *testing.T) { + ambiguous := ast.DataType{Kind: ast.TypeEnumeration, EnumRef: &ast.QualifiedName{Module: "Probe", Name: "Person"}} + explicit := ast.DataType{Kind: ast.TypeEnumeration, EnumRef: &ast.QualifiedName{Module: "Probe", Name: "Status"}, ExplicitEnum: true} + + s := buildFlowScope([]ast.MicroflowStatement{ + &ast.DeclareStmt{Variable: "maybe", Type: ambiguous}, + &ast.DeclareStmt{Variable: "sure", Type: explicit}, + }, nil, nil) + + if got := s.entities["maybe"]; got != "Probe.Person" { + t.Errorf("the entity guess was dropped: %q", got) + } + if k, ok := s.kinds["maybe"]; ok { + t.Errorf("an ambiguous bare name was typed %v; it must have no kind", k) + } + if got := s.kinds["sure"]; got != exprcheck.KindEnumeration { + t.Errorf("an explicit ENUM typed %v, want KindEnumeration", got) + } +} diff --git a/mdl/exprcheck/adapters/check.go b/mdl/exprcheck/adapters/check.go index 13410cdbcd..a2982bcd8e 100644 --- a/mdl/exprcheck/adapters/check.go +++ b/mdl/exprcheck/adapters/check.go @@ -19,8 +19,9 @@ type CheckAdapter struct { // assoc is the catalog when it can also answer association questions; the // interface does not require it, so this is nil for a reader that cannot. assoc associationResolver - // entities is set for the duration of one flow's walk. + // entities and kinds are set for the duration of one flow's walk. entities exprcheck.EntityScope + kinds exprcheck.Scope } // Option configures a CheckAdapter. @@ -95,13 +96,16 @@ func (c *CheckAdapter) CheckNanoflow(stmt *ast.CreateNanoflowStmt) *Result { // The variable→entity map used to be built here and used only to label a // CHANGE's slot path; it was never handed to the checker, so `$obj/Attr` had // nothing to resolve against and every rule that depends on an attribute path -// stayed quiet. It is now also the EntityScope for the whole walk. +// stayed quiet. It is now the EntityScope for the whole walk, and the primitive +// half is the Scope beside it — a locally declared String was Unknown until +// then, and a rule that needs both operands typed (E004) stayed quiet on the +// most ordinary shape in the language (mendixlabs/mxcli#1100). func (c *CheckAdapter) walkFlow(body []ast.MicroflowStatement, params []ast.MicroflowParam, mf string, r *Result) { - scope := buildVarEntityScope(body) - addParamEntities(scope, params) - c.entities = entityScope{vars: scope, assoc: c.assoc} - defer func() { c.entities = nil }() - c.walkBodyWithScope(body, mf, scope, r) + scope := buildFlowScope(body, params, c.assoc) + c.entities = entityScope{vars: scope.entities, assoc: c.assoc} + c.kinds = kindScope(scope.kinds) + defer func() { c.entities, c.kinds = nil, nil }() + c.walkBodyWithScope(body, mf, scope.entities, r) } func (c *CheckAdapter) walkBodyWithScope(body []ast.MicroflowStatement, mf string, scope map[string]string, r *Result) { @@ -116,6 +120,8 @@ func (c *CheckAdapter) walkBodyWithScope(body []ast.MicroflowStatement, mf strin c.walkBodyWithScope(n.Body, mf, scope, r) case *ast.LoopStmt: c.walkBodyWithScope(n.Body, mf, scope, r) + case *ast.ListOperationStmt: + c.checkListOperationCondition(n, mf, scope, r) case *ast.ReturnStmt: c.checkExpr(n.Value, "ReturnStmt.Value", mf, r) case *ast.DeclareStmt: @@ -153,7 +159,59 @@ func (c *CheckAdapter) walkBodyWithScope(body []ast.MicroflowStatement, mf strin c.checkExpr(a.Value, "CallArgument.Value", mf, r) } } + // A custom ON ERROR body is a block of ordinary statements and its + // expressions are as checkable as any other. It was not walked at all, + // so moving a statement into a handler exempted it from every rule. It + // is walked after the statement that carries it, which is when it runs. + if eb := errorHandlerBody(s); eb != nil { + c.walkBodyWithScope(eb, mf, scope, r) + } + } +} + +// checkListOperationCondition checks a FIND/FILTER predicate with +// $currentObject bound to the element type of the list under test. +// +// The predicate is block-scoped in the same way a loop body is: $currentObject +// exists only inside it, and it is named per statement rather than once per +// flow — two FILTERs over different lists in one microflow mean two different +// entities — so the binding is made for the duration of this one expression +// rather than folded into the flow scope. +// +// A BARE attribute name in the predicate (`FILTER($L, Status = 'Open')`, the +// spelling the skills recommend) still resolves to nothing: the parser reads it +// as a variable, not as an attribute of the item. That gap is deliberate here — +// binding bare names to the element entity would change what a bare identifier +// means everywhere in an expression, which is a larger decision than this one. +// +// A KNOWN element entity is the condition for checking at all, not just for +// binding $currentObject. `set $At = find($Hay, $Needle)` is Mendix's STRING +// find, and the visitor still builds it as a ListOperationStmt — the ambiguity +// is resolved later, in the flow builder, by looking at whether the input is a +// declared String (mdl-examples/bug-tests/ledger-63-string-find.mdl). Checking +// its second argument as a predicate reported the needle as a non-Boolean, on a +// script that builds at 0 errors. Requiring the entity applies the same +// disambiguation the builder already makes. +func (c *CheckAdapter) checkListOperationCondition(n *ast.ListOperationStmt, mf string, scope map[string]string, r *Result) { + if n.Condition == nil { + return + } + if n.Operation != ast.ListOpFind && n.Operation != ast.ListOpFilter { + return + } + elem := scope[strings.TrimPrefix(n.InputVariable, "$")] + if elem == "" { + return + } + vars := make(map[string]string, len(scope)+1) + for k, v := range scope { + vars[k] = v } + vars["currentObject"] = elem + saved := c.entities + c.entities = entityScope{vars: vars, assoc: c.assoc} + c.checkExpr(n.Condition, "ListOperation.Condition", mf, r) + c.entities = saved } func (c *CheckAdapter) checkExpr(expr ast.Expression, slot, mf string, r *Result) { @@ -170,6 +228,7 @@ func (c *CheckAdapter) checkExpr(expr ast.Expression, slot, mf string, r *Result Slots: c.slots, Catalog: c.catalog, Entities: c.entities, + Scope: c.kinds, }) r.Hints = append(r.Hints, hints...) } diff --git a/mdl/exprcheck/slot_resolver.go b/mdl/exprcheck/slot_resolver.go index 7a243e489d..05512e33a5 100644 --- a/mdl/exprcheck/slot_resolver.go +++ b/mdl/exprcheck/slot_resolver.go @@ -6,8 +6,12 @@ package exprcheck // Add a new entry whenever a new MDL statement slot is added to the executor. // Slot paths mirror the AST node + field name, e.g. "IfStmt.Condition". var staticExpectations = map[string]SlotConstraint{ - "IfStmt.Condition": {Kind: KindBoolean}, - "WhileStmt.Condition": {Kind: KindBoolean}, + "IfStmt.Condition": {Kind: KindBoolean}, + "WhileStmt.Condition": {Kind: KindBoolean}, + // A FIND/FILTER predicate is a Boolean expression over the item under test, + // the same shape as a WHILE condition. Mendix reports a non-Boolean one as + // CE0117 on the list-operation activity. + "ListOperation.Condition": {Kind: KindBoolean}, "RetrieveStmt.LimitExpr": {Kind: KindInteger}, "RetrieveStmt.OffsetExpr": {Kind: KindInteger}, "ChangeItem.Value": {Kind: KindUnknown, ResolveBy: "AttributeOf:Parent"}, diff --git a/mdl/exprcheck/slot_to_context.go b/mdl/exprcheck/slot_to_context.go index ac9dd32489..03705921f8 100644 --- a/mdl/exprcheck/slot_to_context.go +++ b/mdl/exprcheck/slot_to_context.go @@ -12,6 +12,7 @@ func SlotToContext(slotPath string) string { var slotContext = map[string]string{ "IfStmt.Condition": "IF condition", "WhileStmt.Condition": "WHILE condition", + "ListOperation.Condition": "FIND/FILTER predicate", "ChangeItem.Value": "field of CHANGE", "CreateItem.Value": "field of CREATE", "ReturnStmt.Value": "RETURN value", diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index 5c42501419..db4eed1f9d 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/mdl/visitor/visitor_microflow_statements.go @@ -802,146 +802,209 @@ func buildSetStatementNode(ctx parser.ISetStatementContext) ast.MicroflowStateme valueExpr = buildExpression(expr) } - // Check if the expression is a list operation or aggregate function + // Check if the expression is a list operation or aggregate function. if funcCall, ok := valueExpr.(*ast.FunctionCallExpr); ok { - funcName := strings.ToUpper(funcCall.Name) + if stmt := buildListOrAggregateStatement(targetVar, funcCall); stmt != nil { + return recordUnresolvedOperands(stmt, funcCall.Arguments) + } + } - // Check for list operations: HEAD, TAIL, FIND, FILTER, SORT, UNION, INTERSECT, SUBTRACT, CONTAINS, EQUALS - switch funcName { - case "HEAD": - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpHead, - InputVariable: extractVariableName(funcCall.Arguments, 0), - } - case "TAIL": - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpTail, - InputVariable: extractVariableName(funcCall.Arguments, 0), - } - case "FIND": - // `find` is overloaded: the LIST operation find(list, condition) — which - // filters a list by a boolean condition — and the STRING function - // find(haystack, needle) → the index of a substring. A STRING-LITERAL - // second argument is unambiguously the string function (you never filter - // a list by a bare string literal); it must stay a value expression, not - // a lossy List operation activity whose output variable collides - // (CE0111). Ledger #63. When both arguments are plain variables the kind - // is ambiguous here; the flow builder disambiguates String-typed inputs. - if !isStringLiteralArg(funcCall.Arguments, 1) { - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpFind, - InputVariable: extractVariableName(funcCall.Arguments, 0), - Condition: getArgumentExpression(funcCall.Arguments, 1), - } - } - // Falls through to the default MfSetStmt (string find expression). - case "FILTER": + if valueExprCtx != nil { + valueExpr = buildSourceExpression(valueExprCtx) + valueExpr = appendStatementExpressionTrailingWhitespace(valueExprCtx, valueExpr) + } + + // Default: regular SET statement + return &ast.MfSetStmt{ + Target: targetVar, + Value: valueExpr, + } +} + +// buildListOrAggregateStatement converts a SET whose value is a list-operation +// or aggregate call into the matching activity statement, or returns nil when +// the call is not one of those (or is the string-function reading of an +// overloaded name, which must stay a value expression). +// +// It is a separate function so that every arm funnels through one tail in the +// caller — recordUnresolvedOperands. Doing the same bookkeeping inline in each +// arm is how the list operand went missing in the first place; see the note on +// buildSetAggregate about two conversions for one syntax. +func buildListOrAggregateStatement(targetVar string, funcCall *ast.FunctionCallExpr) ast.MicroflowStatement { + funcName := strings.ToUpper(funcCall.Name) + + // Check for list operations: HEAD, TAIL, FIND, FILTER, SORT, UNION, INTERSECT, SUBTRACT, CONTAINS, EQUALS + switch funcName { + case "HEAD": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpHead, + InputVariable: extractVariableName(funcCall.Arguments, 0), + } + case "TAIL": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpTail, + InputVariable: extractVariableName(funcCall.Arguments, 0), + } + case "FIND": + // `find` is overloaded: the LIST operation find(list, condition) — which + // filters a list by a boolean condition — and the STRING function + // find(haystack, needle) → the index of a substring. A STRING-LITERAL + // second argument is unambiguously the string function (you never filter + // a list by a bare string literal); it must stay a value expression, not + // a lossy List operation activity whose output variable collides + // (CE0111). Ledger #63. When both arguments are plain variables the kind + // is ambiguous here; the flow builder disambiguates String-typed inputs. + if !isStringLiteralArg(funcCall.Arguments, 1) { return &ast.ListOperationStmt{ OutputVariable: targetVar, - Operation: ast.ListOpFilter, + Operation: ast.ListOpFind, InputVariable: extractVariableName(funcCall.Arguments, 0), Condition: getArgumentExpression(funcCall.Arguments, 1), } - case "SORT": - stmt := &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpSort, - InputVariable: extractVariableName(funcCall.Arguments, 0), - } - // Parse sort specifications from remaining arguments - stmt.SortSpecs = extractSortSpecs(funcCall.Arguments[1:]) - return stmt - case "UNION": - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpUnion, - InputVariable: extractVariableName(funcCall.Arguments, 0), - SecondVariable: extractVariableName(funcCall.Arguments, 1), - } - case "INTERSECT": - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpIntersect, - InputVariable: extractVariableName(funcCall.Arguments, 0), - SecondVariable: extractVariableName(funcCall.Arguments, 1), - } - case "SUBTRACT": - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpSubtract, - InputVariable: extractVariableName(funcCall.Arguments, 0), - SecondVariable: extractVariableName(funcCall.Arguments, 1), - } - case "CONTAINS": - // `contains` is overloaded: the LIST operation contains(list, object) - // and the STRING function contains(haystack, needle). A List operation - // activity requires two plain list/object variables; if either argument - // is a literal or a computed expression it is unambiguously the string - // function, which must stay a value expression (a Change Variable - // action) — serializing it as a List operation fails the build - // (CE0023/CE0097/CE0111). Ledger finding #53. When both arguments are - // plain variables the kind is still ambiguous here (no type info); the - // flow builder disambiguates String-typed inputs downstream. - if isPlainVariableArg(funcCall.Arguments, 0) && isPlainVariableArg(funcCall.Arguments, 1) { - return &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpContains, - InputVariable: extractVariableName(funcCall.Arguments, 0), - SecondVariable: extractVariableName(funcCall.Arguments, 1), - } - } - // Falls through to the default MfSetStmt (string contains expression). - case "EQUALS": + } + // Falls through to the default MfSetStmt (string find expression). + case "FILTER": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpFilter, + InputVariable: extractVariableName(funcCall.Arguments, 0), + Condition: getArgumentExpression(funcCall.Arguments, 1), + } + case "SORT": + stmt := &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpSort, + InputVariable: extractVariableName(funcCall.Arguments, 0), + } + // Parse sort specifications from remaining arguments + stmt.SortSpecs = extractSortSpecs(funcCall.Arguments[1:]) + return stmt + case "UNION": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpUnion, + InputVariable: extractVariableName(funcCall.Arguments, 0), + SecondVariable: extractVariableName(funcCall.Arguments, 1), + } + case "INTERSECT": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpIntersect, + InputVariable: extractVariableName(funcCall.Arguments, 0), + SecondVariable: extractVariableName(funcCall.Arguments, 1), + } + case "SUBTRACT": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpSubtract, + InputVariable: extractVariableName(funcCall.Arguments, 0), + SecondVariable: extractVariableName(funcCall.Arguments, 1), + } + case "CONTAINS": + // `contains` is overloaded: the LIST operation contains(list, object) + // and the STRING function contains(haystack, needle). A List operation + // activity requires two plain list/object variables; if either argument + // is a literal or a computed expression it is unambiguously the string + // function, which must stay a value expression (a Change Variable + // action) — serializing it as a List operation fails the build + // (CE0023/CE0097/CE0111). Ledger finding #53. When both arguments are + // plain variables the kind is still ambiguous here (no type info); the + // flow builder disambiguates String-typed inputs downstream. + if isPlainVariableArg(funcCall.Arguments, 0) && isPlainVariableArg(funcCall.Arguments, 1) { return &ast.ListOperationStmt{ OutputVariable: targetVar, - Operation: ast.ListOpEquals, + Operation: ast.ListOpContains, InputVariable: extractVariableName(funcCall.Arguments, 0), SecondVariable: extractVariableName(funcCall.Arguments, 1), } - case "RANGE": - stmt := &ast.ListOperationStmt{ - OutputVariable: targetVar, - Operation: ast.ListOpRange, - InputVariable: extractVariableName(funcCall.Arguments, 0), - } - if len(funcCall.Arguments) > 1 { - stmt.OffsetExpr = funcCall.Arguments[1] - } - if len(funcCall.Arguments) > 2 { - stmt.LimitExpr = funcCall.Arguments[2] - } - return stmt - // Check for aggregate operations: COUNT, SUM, AVERAGE, MINIMUM, MAXIMUM - case "COUNT": - return &ast.AggregateListStmt{ - OutputVariable: targetVar, - Operation: ast.AggregateCount, - InputVariable: extractVariableName(funcCall.Arguments, 0), - } - case "SUM": - return buildSetAggregate(targetVar, ast.AggregateSum, funcCall.Arguments) - case "AVERAGE": - return buildSetAggregate(targetVar, ast.AggregateAverage, funcCall.Arguments) - case "MINIMUM": - return buildSetAggregate(targetVar, ast.AggregateMinimum, funcCall.Arguments) - case "MAXIMUM": - return buildSetAggregate(targetVar, ast.AggregateMaximum, funcCall.Arguments) } + // Falls through to the default MfSetStmt (string contains expression). + case "EQUALS": + return &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpEquals, + InputVariable: extractVariableName(funcCall.Arguments, 0), + SecondVariable: extractVariableName(funcCall.Arguments, 1), + } + case "RANGE": + stmt := &ast.ListOperationStmt{ + OutputVariable: targetVar, + Operation: ast.ListOpRange, + InputVariable: extractVariableName(funcCall.Arguments, 0), + } + if len(funcCall.Arguments) > 1 { + stmt.OffsetExpr = funcCall.Arguments[1] + } + if len(funcCall.Arguments) > 2 { + stmt.LimitExpr = funcCall.Arguments[2] + } + return stmt + // Check for aggregate operations: COUNT, SUM, AVERAGE, MINIMUM, MAXIMUM + case "COUNT": + return &ast.AggregateListStmt{ + OutputVariable: targetVar, + Operation: ast.AggregateCount, + InputVariable: extractVariableName(funcCall.Arguments, 0), + } + case "SUM": + return buildSetAggregate(targetVar, ast.AggregateSum, funcCall.Arguments) + case "AVERAGE": + return buildSetAggregate(targetVar, ast.AggregateAverage, funcCall.Arguments) + case "MINIMUM": + return buildSetAggregate(targetVar, ast.AggregateMinimum, funcCall.Arguments) + case "MAXIMUM": + return buildSetAggregate(targetVar, ast.AggregateMaximum, funcCall.Arguments) } + return nil +} - if valueExprCtx != nil { - valueExpr = buildSourceExpression(valueExprCtx) - valueExpr = appendStatementExpressionTrailingWhitespace(valueExprCtx, valueExpr) +// recordUnresolvedOperands notes every list operand that did not reduce to a +// variable name, so the validator can refuse the statement instead of writing an +// activity with an empty List (mendixlabs/mxcli#1101). +// +// It keys on the RESULT of the conversion rather than on the argument's node +// type, which is what makes it uniform across the arms: buildSetAggregate reads +// an attribute path (`sum($List.Price)`) that extractVariableName cannot, so a +// predicate written over node types would have to differ per arm and would drift +// apart again. An empty InputVariable means the conversion found nothing to +// store, whatever the reason. +func recordUnresolvedOperands(stmt ast.MicroflowStatement, args []ast.Expression) ast.MicroflowStatement { + operand := func(index int) ast.UnresolvedOperand { + op := ast.UnresolvedOperand{Index: index} + if index < len(args) { + op.Expr = args[index] + } + return op + } + switch s := stmt.(type) { + case *ast.ListOperationStmt: + if s.InputVariable == "" { + s.UnresolvedOperands = append(s.UnresolvedOperands, operand(0)) + } + if listOperationTakesSecondList(s.Operation) && s.SecondVariable == "" { + s.UnresolvedOperands = append(s.UnresolvedOperands, operand(1)) + } + case *ast.AggregateListStmt: + if s.InputVariable == "" { + s.UnresolvedOperands = append(s.UnresolvedOperands, operand(0)) + } } + return stmt +} - // Default: regular SET statement - return &ast.MfSetStmt{ - Target: targetVar, - Value: valueExpr, +// listOperationTakesSecondList reports whether the operation's SECOND argument is +// another list. It is not "has a second argument": SORT's is a sort spec, +// FILTER/FIND's is a predicate and RANGE's is an offset, none of which belong in +// the list-operand check. +func listOperationTakesSecondList(op ast.ListOperationType) bool { + switch op { + case ast.ListOpUnion, ast.ListOpIntersect, ast.ListOpSubtract, + ast.ListOpContains, ast.ListOpEquals: + return true } + return false } // extractVariableName extracts a variable name from an argument at the given index. diff --git a/modelsdk/meta/system_enumerations.go b/modelsdk/meta/system_enumerations.go new file mode 100644 index 0000000000..896a7aa26c --- /dev/null +++ b/modelsdk/meta/system_enumerations.go @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 + +package meta + +// The System module's built-in enumerations, as model elements. +// +// Like the entities, associations and Java actions beside them, these are NOT +// stored in the .mpr — Mendix ships them with the platform — so a reader that +// only decodes stored units reports them as absent. The definitions in +// SystemEnumerations had been here since #889 with no consumer at all, which is +// why `describe entity` could print `ActivityType: Enumeration(System. +// WorkflowActivityType)` while `describe enumeration System.WorkflowActivityType` +// answered "enumeration not found": the entity half was wired and the +// enumeration half never was (mendixlabs/mxcli#1102). +// +// Read-only. The System module has no stored unit to contain anything, so the +// executor refuses every write that names it — see refuseSystemEnumerationWrite +// in mdl/executor/cmd_enumerations.go, which exists because making these +// visible also makes them addressable. + +import ( + "strings" + + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" +) + +// BuildSystemEnumerations returns the System module's enumerations as semantic +// model elements, ready to append to a backend's enumeration listing. +// +// IDs are deterministic, so two readers describing the same project agree about +// an enumeration's identity even though neither read it from storage, and the +// catalog (which keys enumerations_data on Id) stays stable run to run. Mirrors +// BuildSystemJavaActions. +// +// Captions are deliberately left unset: SystemEnumerations carries value NAMES +// only, and Mendix's own captions for these are not recorded here. Defaulting a +// caption to the value name would put invented text in DESCRIBE output and in +// the catalog's translation rows, where nothing could tell it from a caption a +// developer actually wrote. The cost is that `search` — which indexes captions — +// still does not match these; that needs real caption data, not a guess. +func BuildSystemEnumerations() []*model.Enumeration { + out := make([]*model.Enumeration, 0, len(SystemEnumerations)) + for _, def := range SystemEnumerations { + e := &model.Enumeration{ + ContainerID: model.ID(SystemModuleID), + // The model carries the LOCAL name; the module comes from + // ContainerID, exactly as it does for a stored enumeration. Keeping + // the qualified name here would make DESCRIBE emit + // "System.System.WorkflowActivityType". + Name: strings.TrimPrefix(def.Name, "System."), + } + e.ID = model.ID(types.GenerateDeterministicID(def.Name)) + e.TypeName = "Enumerations$Enumeration" + for _, v := range def.Values { + ev := model.EnumerationValue{Name: v} + ev.ID = model.ID(types.GenerateDeterministicID(def.Name + "." + v)) + ev.TypeName = "Enumerations$EnumerationValue" + e.Values = append(e.Values, ev) + } + out = append(out, e) + } + return out +} diff --git a/modelsdk/meta/system_enumerations_test.go b/modelsdk/meta/system_enumerations_test.go new file mode 100644 index 0000000000..c84ab5de1a --- /dev/null +++ b/modelsdk/meta/system_enumerations_test.go @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: Apache-2.0 + +package meta + +import ( + "os" + "slices" + "strings" + "testing" +) + +// The System module's enumerations were defined in SystemEnumerations and then +// never exposed: `describe enumeration System.WorkflowActivityType` reported +// "enumeration not found" while `describe entity` happily printed attributes +// typed against it, so the values could only be guessed at until the build +// rejected one with CE1613 (mendixlabs/mxcli#1102). +// +// The entity and Java-action halves of the virtual System module each have a +// Build* helper; this is the enumeration one, plus the resolvability guard that +// the entity half has had all along (TestModelerSystemEntities_ +// HaveResolvableGeneralizations) and this half did not. + +// TestModelerSystemEntities_HaveResolvableEnumerations is the sibling of +// TestModelerSystemEntities_HaveResolvableGeneralizations: an attribute typed +// against a System enumeration mxcli cannot produce is an attribute whose valid +// values nothing can report. It passes on today's table — the 15 definitions +// cover all 14 enumerations the modeler-view entities reference — and exists so +// that adding an enum-typed System attribute without its enumeration fails here +// rather than at a user's build. +func TestModelerSystemEntities_HaveResolvableEnumerations(t *testing.T) { + known := make(map[string]bool, len(SystemEnumerations)) + for _, e := range SystemEnumerations { + known[e.Name] = true + } + for _, ent := range ModelerSystemEntities() { + for _, a := range ent.Attributes { + if a.Type != "Enumeration" { + continue + } + if a.EnumQN == "" { + t.Errorf("System.%s.%s is an Enumeration with no EnumQN", ent.Name, a.Name) + continue + } + if !known[a.EnumQN] { + t.Errorf("System.%s.%s references %s, which is not in SystemEnumerations — "+ + "describe enumeration %s will report it missing", + ent.Name, a.Name, a.EnumQN, a.EnumQN) + } + } + } +} + +// TestBuildSystemEnumerations_CoversTheTable checks the helper exposes every +// definition, with the module stripped off the Name (the model carries the local +// name; the module comes from ContainerID) and every value carried over. +func TestBuildSystemEnumerations_CoversTheTable(t *testing.T) { + built := BuildSystemEnumerations() + if len(built) != len(SystemEnumerations) { + t.Fatalf("BuildSystemEnumerations() = %d enumerations, want %d", len(built), len(SystemEnumerations)) + } + byName := make(map[string]int, len(built)) + for _, e := range built { + if string(e.ContainerID) != SystemModuleID { + t.Errorf("%s ContainerID = %q, want the System module ID", e.Name, e.ContainerID) + } + if e.TypeName != "Enumerations$Enumeration" { + t.Errorf("%s TypeName = %q", e.Name, e.TypeName) + } + byName[e.Name] = len(e.Values) + } + for _, def := range SystemEnumerations { + local := def.Name[len("System."):] + got, ok := byName[local] + if !ok { + t.Errorf("%s missing from BuildSystemEnumerations() (looked for local name %q)", def.Name, local) + continue + } + if got != len(def.Values) { + t.Errorf("%s value count = %d, want %d", def.Name, got, len(def.Values)) + } + } +} + +// TestBuildSystemEnumerations_IDsAreDeterministicAndUnique matters because the +// catalog keys enumerations_data on Id: a colliding or run-varying ID either +// breaks the insert or makes the catalog differ run to run. Mirrors the Java +// action helper's scheme. +func TestBuildSystemEnumerations_IDsAreDeterministicAndUnique(t *testing.T) { + first, second := BuildSystemEnumerations(), BuildSystemEnumerations() + seen := map[string]string{} + for i, e := range first { + if e.ID != second[i].ID { + t.Errorf("%s ID varies between calls: %q vs %q", e.Name, e.ID, second[i].ID) + } + if e.ID == "" { + t.Errorf("%s has an empty ID", e.Name) + } + if prev, dup := seen[string(e.ID)]; dup { + t.Errorf("%s and %s share ID %q", prev, e.Name, e.ID) + } + seen[string(e.ID)] = e.Name + vals := map[string]bool{} + for _, v := range e.Values { + if v.ID == "" { + t.Errorf("%s.%s has an empty value ID", e.Name, v.Name) + } + if vals[string(v.ID)] { + t.Errorf("%s has duplicate value ID %q", e.Name, v.ID) + } + vals[string(v.ID)] = true + } + } +} + +// TestSystemEnumerations_NamesAreQualified guards the assumption the builder +// makes when it strips the prefix. +func TestSystemEnumerations_NamesAreQualified(t *testing.T) { + for _, def := range SystemEnumerations { + if len(def.Name) <= len("System.") || def.Name[:len("System.")] != "System." { + t.Errorf("SystemEnumerations entry %q is not a System-qualified name", def.Name) + } + if len(def.Values) == 0 { + t.Errorf("%s has no values", def.Name) + } + } +} + +// TestSkillDocumentsTheSameValues pins the system-module skill's enumeration +// section to this table. +// +// It exists because the section had DRIFTED into wrong casing — `created`, +// `end`, `single`, `microflow`, `error`, `user`, `external` — and was missing +// three enumerations entirely, WorkflowActivityState among them. Enumeration +// value names are case-sensitive and a wrong one is only caught at build time as +// CE1613, so a developer copying `created` out of the skill hit exactly the +// failure the skill was there to prevent (mendixlabs/mxcli#1102). A hand-kept +// list of platform values is only as good as the thing that compares it. +func TestSkillDocumentsTheSameValues(t *testing.T) { + const skill = "../../.claude/skills/mendix/system-module/SKILL.md" + raw, err := os.ReadFile(skill) + if err != nil { + t.Skipf("skill not readable (%v) — nothing to compare", err) + } + + // Section 8 lists one `### ` heading per enumeration, followed by + // its values as `backticked`, comma-separated names. + body := string(raw) + start := strings.Index(body, "## 8. Enumerations") + if start < 0 { + t.Fatalf("%s no longer has an '## 8. Enumerations' section — update this test with it", skill) + } + section := body[start:] + if end := strings.Index(section, "\n## 9."); end > 0 { + section = section[:end] + } + + documented := map[string][]string{} + var current string + for _, line := range strings.Split(section, "\n") { + if after, ok := strings.CutPrefix(line, "### "); ok { + current = strings.TrimSpace(after) + continue + } + if current == "" || !strings.HasPrefix(strings.TrimSpace(line), "`") { + continue + } + for _, part := range strings.Split(line, ",") { + if v := strings.Trim(strings.TrimSpace(part), "`"); v != "" { + documented[current] = append(documented[current], v) + } + } + current = "" + } + + for _, def := range SystemEnumerations { + local := strings.TrimPrefix(def.Name, "System.") + got, ok := documented[local] + if !ok { + t.Errorf("%s is not documented in the system-module skill's section 8", def.Name) + continue + } + if !slices.Equal(got, def.Values) { + t.Errorf("%s: skill documents %v, table has %v", def.Name, got, def.Values) + } + delete(documented, local) + } + for extra := range documented { + t.Errorf("the skill documents System.%s, which is not in SystemEnumerations", extra) + } +} diff --git a/sdk/pages/pages_widgets_input.go b/sdk/pages/pages_widgets_input.go index 167cf7b75c..e9880e802e 100644 --- a/sdk/pages/pages_widgets_input.go +++ b/sdk/pages/pages_widgets_input.go @@ -112,9 +112,18 @@ type ReferenceSetSelector struct { // CheckBox represents a checkbox widget. type CheckBox struct { BaseWidget - Label string `json:"label,omitempty"` - AttributePath string `json:"attributePath,omitempty"` - ReadOnly bool `json:"readOnly,omitempty"` + Label string `json:"label,omitempty"` + AttributePath string `json:"attributePath,omitempty"` + ReadOnly bool `json:"readOnly,omitempty"` + // ReadOnlyStyle is Mendix's "Read-only style": Inherit, Control or Text. + // Empty means unset — the writer keeps the stored default (Inherit), so a + // script that never mentions it produces the document it always did. + // + // It decides how a READ-ONLY check box renders, and the difference is not + // cosmetic: Text renders the words "Yes"/"No", Control renders the (disabled) + // checkbox glyph. That is the whole of "show a Boolean as a checkbox" in a + // DataGrid2 cell (ako/mxcli#490). + ReadOnlyStyle string `json:"readOnlyStyle,omitempty"` OnChangeAction ClientAction `json:"onChangeAction,omitempty"` }