diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index aed1fbded..57f14c3a4 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -525,6 +525,16 @@ extracting `OffsetExpression`/`LimitExpression`. | Running `mxcli test` dirties the project: `git status` reports the `.mpr` modified after a run that changed nothing, so a "run the tests, assert the tree is clean" CI step fails and a pull request carries a meaningless diff. `mprcontents/` is byte-identical; only the `.mpr` differs, and a `.mpr` diff is opaque | The runner injects an `MxTest` module and takes it back out. Cleanup restores the model, but every unit write stamps a fresh UUID into the `.mpr`'s `_Transaction.LastTransactionID` (`updateTransactionID`, in both engines' writers) **and** the insert/delete cycle relays SQLite's pages. Restoring the row alone is measurably insufficient: three consecutive runs then hold the id stable and still produce a different file hash each time | `cmd/mxcli/testrunner/snapshot.go` (new — `projectSnapshot`, `documentTreeDigest`), `cmd/mxcli/testrunner/runner.go` (`captureProjectState`, `restoreProjectFile` at all three cleanup sites), `cmd/mxcli/testrunner/host.go` (`HostedEndpoint.Remove`) | Snapshot the whole `.mpr` before injecting and write it back after a successful cleanup — byte-exact by construction rather than by enumerating what might have changed. Two refusals are load-bearing: **cleanup failed** and **the `mprcontents/` tree moved**. In either case the project is not in the state the snapshot describes, and restoring would turn a visible harmless discrepancy into an invisible misleading one — a tree that reads as clean while the model is not. Write through a temp file + rename so an interrupted restore cannot truncate the `.mpr`. **Generalisable**: a tool that mutates a project to do its work owes a restore that is byte-exact, not semantically equivalent — every consumer downstream of it compares bytes. And when the obvious cause is a single row, measure the file hash before concluding it is the only one. Verified with the reporter's own harness: 3 runs, `.mpr` and `mprcontents` hashes unchanged throughout. mxcli-sudoku FINDINGS #47 | | `run --local --watch` deploys a **half-applied model** when an `mxcli exec` is running, and there is no way to recover: re-running the script writes nothing (it is byte-idempotent), so nothing re-triggers the watcher and the stale build stands | `watchAndApply` (`cmd/mxcli/docker/runlocal.go`) rebuilt on the **first** mtime bump. An exec rewrites the `.mpr` and many `mprcontents/*.mxunit` over seconds, so the build snapshots the tree mid-write. There was a settle for the web bundler and nothing for the model. Two behaviours each correct alone: idempotency removed a *recovery path* nobody had written down as one — "just run it again" was load-bearing | `cmd/mxcli/docker/runlocal.go` (`settleSource`, `sourceSettleWindow`, the `case <-ticker.C` branch) | Wait for the source mtime to stop advancing (two poll intervals of quiet) before building. The wait is unbounded on purpose — a long exec is the case it exists for, and building late beats building mid-write — but it still honours the interrupt channel so Ctrl-C is not swallowed. `touch` on the `.mpr` remains the force-rebuild hatch, now documented. **Generalisable, twice over**: (a) a change *signal* is not a change *event* — anything polling mtime must debounce or it samples a writer mid-flight; (b) when adding an optimisation that skips work, ask what informal recovery procedure depended on that work happening. **Test trap hit while fixing it**: the first version asserted the file count *after* `wg.Wait()`, which holds against a `settleSource` that returns immediately — the assertion has to read the writer's state at the moment it returned. mxcli-sudoku FINDINGS #45 | | An OQL `ORDER BY` on a **DateTime** looks ignored: `order by DealtAt desc limit 5` returns old rows while `order by Id desc` returns genuinely new ones, and the wrong answer is **stable across runs** — so it reads as a platform bug rather than a data problem, and the natural workaround becomes "order by Id for recency" | **Not an mxcli bug, and not a Mendix bug.** Mendix emits the ordering with no null placement, so the database default applies: on PostgreSQL `DESC` means **NULLS FIRST**, and rows whose attribute was never set head the result. Stability is what makes it convincing and wrong — a degenerate sort key is exactly as repeatable as a correct one, so "stable, therefore not a tie-break artifact" does not discriminate | Nothing to change in mxcli — `ExecuteOQL` passes the query verbatim and `parseOQLFeedback` preserves row order. Docs only: `docs-site/src/tools/oql.md`, `.claude/skills/mendix/write-oql-queries.md` | Measured on **Mendix 11.13.0** with PostgreSQL statement logging on, which is the method to reuse: `ALTER SYSTEM SET log_statement='all'`, run the query, read the SQL the runtime actually sent. With four distinct non-null timestamps the emitted SQL is `… ORDER BY "x"."dealtat" DESC LIMIT $1` and the result is correctly ordered — the ordering is **not** dropped. Adding two rows with an empty DealtAt reproduces the reported symptom exactly, and running Mendix's own emitted SQL by hand with `NULLS LAST` fixes it. **Generalisable**: when a query returns a wrong-but-stable answer, suspect a degenerate sort key before suspecting the engine; and when a bug report blames a layer, check whether that layer is even in the path — here the same finding also reported the column being *dropped from the projection*, whose known mechanism is a null in the first row, which was direct evidence for the real cause sitting unread in the report. mxcli-sudoku FINDINGS #39 (second half) | +| `ALTER PAGE SET ON .` fails with `column property "X" not found` for a property the very same tool writes happily inside `CREATE PAGE`. `check --references` passes the statement that `exec` rejects, so the disagreement only surfaces at write time | Two hand-written alias tables for one widget schema, drifted. The create path (`mdl/executor/widget_defs.go` `itemPropertyAliases`) aliased `DynamicCellClass` → `columnClass`; the ALTER path (`mdl/backend/pagemutator/mutator.go` `columnPropertyAliases`) kept its own copy and did not — the file contained zero occurrences of the string | `mdl/types/widget_item_aliases.go` (new — `ItemPropertyAliases` moved here), `mdl/executor/widget_defs.go` (now a reference), `mdl/backend/pagemutator/mutator.go` (`resolveColumnPropertyKey`, `settableColumnProperties`) | Delete the second table rather than adding the missing row: resolve the MDL name against the schema keys the **document itself declares** (case-insensitively), falling back to the one shared alias table for genuine renames. A property the create path can write is then one ALTER can write, by construction. Case-only differences (`Sortable`→`sortable`) need no entry at all, which keeps the shared table down to real aliases. The not-found error now lists the settable keys read off that grid, so it is right for the widget version installed rather than the one mxcli was built against. **Generalisable**: two tables describing one external schema will drift; the fix is one table or none, never a third. mendixlabs/mxcli#919 | +| `ALTER PAGE SET DynamicCellClass` / `SET Visible` on a DataGrid 2 column reports success and changes nothing visible: the value does not come back from `describe page`, and `mx check` stays at 0 errors | `setColumnPropertyMut` wrote every value to `PrimitiveValue`. `columnClass` and `visible` are **Expression**-valued, and a `CustomWidgets$WidgetValue` carries *every* field at once — Expression, PrimitiveValue, TextTemplate, AttributeRef — with the unused ones empty, so key-presence cannot tell you which one to write and the always-primitive default looked plausible. The value landed in a field Mendix does not read | `mdl/backend/pagemutator/mutator.go` (`columnValueField`, `buildColumnPropKeyMap` now also returns kinds, `bsonWidgetResult.colPropKinds`) | Take the kind from the schema — the column's `PropertyTypes[].ValueType.Type` — and write Expression, PrimitiveValue or the TextTemplate path accordingly; refuse the structured kinds (Attribute, DataSource, Action, Widgets) rather than writing a string where a reference belongs. Verified on Mendix 11.13.0: before, the write was invisible to DESCRIBE and `mx check` said 0 errors; after, it round-trips and mxbuild **validates** it — a valid expression builds at 0 errors and a bare identifier is CE0117, matching what CREATE produces for the same input. **Generalisable**: when a container type carries every variant field at once, presence is not a discriminator — the schema is. A silent wrong write is worse than the loud failure it was hiding behind | +| A DataGrid 2 column named in MDL (`column colLabel (...)`) cannot be addressed by that name: `ALTER PAGE … ON dg1.colLabel` says not found, and `describe page` shows a name the author never wrote | Working as designed and undiscoverable. Mendix stores no name on a pluggable DataGrid 2 column — the schema has no name key, and `backend.DataGridColumnSpec` has no field for one — so the MDL name is dropped and everything downstream uses a derived name (bound attribute → sanitized caption → `colN`) | `mdl/executor/validate_widgets.go` (`validateDataGrid2ColumnName`, `derivedDataGrid2ColumnName`) | Warn at authoring time (MDL-WIDGET16) naming the addressable name, instead of leaving the author to discover it from a failed ALTER. **Warn, not reject**: the name reads as documentation in the source and rejecting it would break every existing script — mxcli's own doctype tests name every column. Stay silent when the derivation cannot be known (no attribute, no caption → `colN`, which depends on position), because naming the wrong one is worse than naming none. Note this is specific to the **pluggable** DataGrid 2; legacy `Forms$DataGridColumn` does store a Name | +| `mxcli check script.mdl -p app.mpr` prints an unqualified `Check passed!` having resolved **nothing** against the project — a misspelled icon, entity or page name sails through the command that was handed the model. Reported as "check does not validate icon names", which it does | `cmd_check.go` gated the whole reference pass on `--references`, so supplying `-p` alone ran only the model-free rules. `validate_icon_refs.go` already existed and works (it names the offending icon and the `describe icon collection` command); it returns early when `!ctx.Connected()`, so it simply never ran | `cmd/mxcli/cmd_check.go` (`checkRefs = checkRefs || projectPath != ""`, the qualified pass message, help text), `cmd/mxcli/main.go` (flag help) | Make `-p` imply reference resolution — someone who hands the command a project has said what they want — and keep `--references` accepted so existing invocations survive. A run *without* a project now qualifies its verdict by naming what was not resolved. **Generalisable, and the third instance this month**: a clean report that cannot distinguish *checked and clean* from *not checked* is the same defect as a vacuous test assertion. When a feature needs two flags to do its job, the second one is usually a bug. Before adding a validator, check whether one exists and is merely unreachable. mxcli-dbreplication F5 | +| Four MDL scripts pass `mxcli check` with 0 errors and execute cleanly; `mx check` then reports CE0156 (user role cannot sign in) and CE5601 (page URL missing a parameter segment) | Both are decidable from the MDL alone and neither had a rule. A user role built only from application module roles has no System module role, so nobody holding it can sign in or read System entities; a page with parameters and a `Url` needs a `{Name}` segment per parameter or Mendix cannot bind it | `mdl/executor/validate_role_and_url.go` (new — `ValidateUserRoleSystemModuleRole` MDL-SEC20, `ValidatePageURLParameters` MDL-PAGE20, `urlBindsParameter`), `mdl/executor/validate_program.go` (wiring) | Add both as model-free rules so `check` catches them without a project. **The URL matcher is the subtle half**: Mendix binds a parameter by an attribute path, so `url: 'p006/{Customer/Name}'` is the ordinary shape and an exact `{Customer}` match would flag correct pages — match the segment's leading identifier instead. Verified against mxcli's own page examples at 0 false positives, which is the control that matters for a new rule. **MDL-SEC20 shipped too strict and CI caught it**: `make check-mdl` runs `mxcli check` over every example, and an error-severity rule broke two of them. Measured against mxbuild 11.13 rather than argued about — CE0156 fires at security level Prototype and **not at all** at Off, which a blank project ships, so the rule warns by default and is an error only when the script itself enables security. CE5601 fires either way, so MDL-PAGE20 stays an error and the one example it flagged was genuinely broken (fixed). **Generalisable**: `make test` is not the CI gate — `make check-mdl` runs the rules against real scripts, which is exactly where a new rule's false-positive rate shows up. Run it before shipping a rule. A role declared with no module roles at all is left alone: that is a placeholder for ALTER USER ROLE, not a missing System role. mxcli-dbreplication F10 | +| `mxcli marketplace search 'Database Replication'` returns **No results** for a module that is right there; `search replication` finds it | `filterItems` matched the packaged name (`DatabaseReplication`) and publisher verbatim. Packaged names have no spaces, and `Content` carries no display-name field, so the name as written everywhere matched nothing | `internal/marketplace/client.go` (`normalizeSearchTerm`) | Fold case and drop separators (space, hyphen, underscore, dot) on **both** sides before matching, so the written name and the packaged name meet in the middle. Adding a display-name field was not an option — the API does not return one. **Generalisable**: when a search matches an identifier that was mechanically derived from a human name, normalise to the derivation, or every user has to guess the derivation. mxcli-dbreplication F7 | +| `MDL-WIDGET16` fires 44 times on one project, once per DataGrid 2 column, all saying the same thing | The rule was written per column when the fact — this grid stores no column names — belongs to the grid. Correct but chatty enough to bury the rest of the report | `mdl/executor/validate_widgets.go` (`validateDataGrid2ColumnNames`, now called per widget rather than per object-list item) | Emit one violation per grid listing each `written → addressable` mapping. **Generalisable**: an advisory rule's unit should be the thing the advice is about, not the thing that triggered it — N identical infos train the reader to skip the category. Feedback on a rule shipped days earlier, from the project using it. mxcli-dbreplication F6 | +| `combobox cb (Association: …, Caption: Name)` passes `mxcli check`, executes, and the caption is **gone** — `describe page` does not show it and the build fails `CE0642 "Property 'Caption' is required."` The reporter concluded the association combo box was unusable and redesigned the UI around it | `Caption` is in `isBuiltinPropName`'s universal allow-list, so MDL-WIDGET01 never questions it on a widget the engine has no route for — while `OptionCaption` *is* correctly rejected. The working spelling is `CaptionAttribute:`, which round-trips cleanly, so the feature was never broken: the author was one property name away with nothing to tell them | `mdl/executor/validate_widgets.go` (`builtinPropertyMisuse`, `misusedBuiltinProperty`, rule MDL-WIDGET17) | Flag the builtin names that are wrong on a specific widget and name the one that works. Deliberately an **explicit list**, not inference: whether a builtin is routed lives in the engine's dispatch, not the .def.json (the combobox def declares `optionsSourceAssociationCaption{Type,Expression}` and nothing called `Caption`), so inferring it would mean reimplementing that dispatch and getting it wrong the other way — the universal allow-list exists precisely to stop false positives on `Label:`/`Class:`. **Generalisable**: a permissive allow-list added to prevent false positives converts every unrouted name into a silent drop; the cost is invisible until a build error names a property the author did write. mxcli-owid #38 | +| A skill pack mxcli ships teaches a guard that cannot fire: `references/patterns.md` shows a splice caller doing `IF $Q/Rejected THEN -- fail the request`, and a project wrote and then deleted a Java action for that branch | The pack's **own** `QueryObject.java` throws `IllegalArgumentException` when `r.rejected && rejectUnsupported` **before** `Core.instantiate`, so a caller that receives a Query at all always has `Rejected = false`. The code is right and the documentation is wrong — and two further places repeated the claim (`SKILL.md`'s field table said `both`, `failure-modes.md` said "Pass `true`: `Rejected` comes back set") | `.claude/skills/packs/mendix-odata-pushdown/references/patterns.md`, `SKILL.md`, `references/failure-modes.md` (embed under `cmd/mxcli/skillpacks/` is regenerated by `make build`) | Delete the dead branch from the pattern and say why it cannot run, quoting the throw; mark `Rejected` as a **bind**-caller field. **Generalisable**: when a doc and the code it documents disagree, grep the whole pack — the claim is rarely in one place, and the two secondary mentions were the ones that would have re-taught it. A documented safeguard that silently is not one is the same defect class as a vacuous assertion. mxcli-owid #30 | +| A two-column card layout renders as one column and the CSS is provably correct — it is on the wrong element | Mendix wraps a repeating widget's children in an intermediate element (`ul` for a list view, `.mx-dataview-content` for a data view), so `display: grid` on the widget's own class has exactly **one** grid item | `.claude/skills/mendix/migrate-design-prototype.md` | Put the grid on the element that holds the repeated children (`.my-cards > ul`), and `min-width: 0` on the child so a wide table does not push the track. The skill already used `> ul > li` in one example but never stated the rule, which is why the same project hit it **twice in two different widgets**. **Generalisable**: an example that happens to be right teaches nothing — if a selector's shape is load-bearing, say why. mxcli-owid #15, #41 | | `mxcli oql` fails on every Mendix older than 11.11 with *"Action not found ... upgrade mxcli"*, and separately reports a **rejected query as `0 rows`** rather than as an error | Two independent misreadings of the runtime's replies. (1) The 11.11+ REST route `/dev/preview_execute_oql` does not exist on older runtimes, and the admin API does not 404 it — it dispatches the POST as an ordinary admin request, finds no `action` field, and answers **HTTP 200** with `{"result":1,"message":"Action not found"}`. The fallback to the legacy M2EE action keyed on the 404 alone, so the legacy action — which works fine there — was never tried. (2) That legacy action reports a bad query as `{"feedback":{"error":"..."},"result":0}` — inside the feedback, with a **success** result code — so `M2EEError()` (which keys off the result) says nothing and the error body parses as an empty result | `cmd/mxcli/docker/oql.go` (`legacyOQL`, `oqlDevErrorKind`, the `error` field in `parseOQLFeedback`'s envelope) | Fall back to the legacy action on *either* absence signal, and surface `feedback.error` regardless of the result code. Measured on 11.6.6: before, `mxcli oql` could not run any query; after, `select count(*) as n from Mod.E` returns a row, and a bad query is an error instead of `0 rows`. **Generalisable**: when a fallback is keyed on one specific failure signal, check what the *other* end actually sends — an HTTP-level 404 and an application-level "not found" are different wires, and a success code next to an error message is common enough to assume it happens. Found while wiring @verify (FINDINGS #48); related to #39 | | Windows Defender flags the mxcli **Windows** release binary as `Trojan:Script/Sabsik.EN.A!ml`; enterprise EDR (Defender for Endpoint, CrowdStrike, SentinelOne) blocks it harder. Not the generic unsigned-Go-binary false positive of #185 | The binary genuinely embedded **chisel**, a dual-use tunnelling/pivoting tool (SSH over WebSocket), on every platform — although the tunnel only ever runs inside a Linux container. `run --hub` linked `chisel/client`, `tunnel-hub` linked `chisel/server`, so windows/darwin carried 32 packages incl. the whole `x/crypto/ssh` stack for a feature they cannot use | `cmd/mxcli/docker/tunnel_linux.go` + `tunnel_other.go` (client seam), `cmd/mxcli/tunnelhub/control_linux.go` + `control_other.go` (server seam), `scripts/check-tunnel-deps.sh` (guard) | **Never obfuscate, pack or rename to dodge the scanner** — attacker tradecraft, and it makes the binary less trustworthy, not more. **Code signing does not fix this class**: a signed binary containing chisel is still flagged behaviourally; signing only addresses #185's generic false positive. The fix is to stop shipping the capability where it is unused: one interface per seam, `_linux.go` impl + `!linux` stub, commands still registered everywhere but failing with an actionable message. **Prove absence three ways, and know that `go tool nm` is not one of them** — release ldflags `-s -w` strip the symbol table, so nm reports "no symbols" whether or not the code is linked and would give a false pass; use `go list -deps`, `go version -m`, and `strings` (nm only on a deliberately unstripped build). **Guard against the transitive path, not the name**: match the module list (`x/crypto/ssh`, `gorilla/websocket`, `armon/go-socks5`, `jpillora/*`) so re-entry without the word "chisel" still trips it, and assert a **positive control** (chisel IS in the linux graph) so the check cannot pass vacuously. Verified by re-adding the import and watching the guard fail on all four windows/darwin targets. Result: -13.5 MB (-14.7%) on windows+darwin, linux unchanged. See ADR-0009 | | CE7375 "must be published and be the key when associations are exposed as an associated object id" on a service publishing no associations | `PublishAssociations` is the representation, not a yes/no — `No` selects "as an associated object id", which needs the system ID as key | `mdl/executor/validate_odata_service_shape.go` | Set `PublishAssociations: Yes` ("as a link", and the default when omitted). MDL-ODATA06 warns at check time | @@ -537,6 +547,7 @@ extracting `OffsetExpression`/`LimitExpression`. | A skill claims a construct is unsupported that the grammar accepts (and documents the working form elsewhere in the same file) — an agent writes valid MDL, hits the contradicting claim, and undoes it. Seen for `case … end case` in `write-microflows.md` (supported at §CASE Statements, "not supported" under UNSUPPORTED Syntax), with `check-syntax.md`, `MDL_QUICK_REFERENCE.md`, three `docs-site` pages and the generated project CLAUDE.md (a third shape, `CASE $Var/Attr AS x WHEN Enum.Value`) all disagreeing | Nothing validates prose claims against the grammar. `scripts/check-skill-mdl.sh` extracts fenced blocks but deliberately **skips microflow bodies** (fragment-heavy), so every syntax claim about microflow statements is unguarded. The wrong "WRONG:" example also used string-literal case values, which fail for their own reason — making the claim look confirmed | Docs only: `.claude/skills/mendix/write-microflows.md` + `check-syntax.md`, `docs/01-project/MDL_QUICK_REFERENCE.md`, `docs-site/src/language/control-flow.md` + `appendixes/{quick-reference,common-mistakes}.md`, `cmd/mxcli/init_claudemd.go` (the generated table) | Settle it against the binary before editing, one variant per claim (`bin/mxcli check` on the documented form, the quoted form, the qualified form, the `AS` form, the `else` form) — a "WRONG" example that fails for an unrelated reason is not evidence. Then fix **every** surface: grep `end case\|END CASE` across `.claude/`, `docs/`, `docs-site/` and `cmd/mxcli/*.go`, not just the file in the report (7 surfaces, the issue named 3). Pin both directions in `mdl-examples/bug-tests/` — `907-case-enum-split-is-supported.mdl` (must pass) and `907-case-enum-split-invalid-forms.fail.mdl` (must fail) — so `make check-mdl` catches drift back. Issue #907 | | Docs disagree on whether an enumeration may be written as a string literal — the skills say "NEVER use a string literal", docs-site uses `Status = 'Draft'` in a dozen CREATE/CHANGE examples and calls it "plain strings when the context is unambiguous". Following either one wholesale is wrong, and `mxcli check` passes **both** the good and the bad form, so the build is where you find out | It is context-dependent and nobody had measured it. A string is accepted wherever the slot is **already enum-typed**; in an **expression** it is a String compared to an Enumeration | Docs only: `.claude/skills/mendix/write-microflows.md` (Enumeration Comparisons), `docs-site/src/language/{control-flow,expressions}.md`, `docs-site/src/reference/microflow/create-microflow.md` | Measure per context on a real project before touching a single example — one microflow per construct, `mxcli docker check` read per construct (`mxcli new EP --version 11.13.0 --skip-build`, then `exec` + `docker check`). Verified on 11.13.0: **CE0117** for `if $O/Status = 'Draft'` and for `'x' + $O/Status`; **clean** for `change`/`create` member values, attribute `DEFAULT 'Draft'`, XPath `[Status = 'Draft']`, and `getCaption()`/`toString()` in a concat. So fix **only** comparisons and concatenations — a sweep-and-replace over every `Status = '…'` would have "corrected" ~10 of 12 sites that are fine. No `.fail.mdl` is possible (check passes the failing form; the type checker is still a proposal — `docs/11-proposals/PROPOSAL_expression_type_checking.md`), so the verdicts live in `mdl-examples/bug-tests/enum-string-literal-contexts.mdl`, which pins the ACCEPTED forms and builds clean (control: adding it to a probe project left the error count unchanged at the 2 deliberate failures) | | `mxcli exec` applies a script that `mxcli check` rejects with an error — e.g. a page written with an invalid widget property, which is then silently dropped, so the widget renders but does nothing | `exec` ran no semantic checks at all. Parse errors stopped it; the ~24 `Validate*` checks lived inline in `cmd/mxcli/cmd_check.go` and had exactly one caller. "Run check before exec" was a convention nothing enforced, on a tool built for unattended agent use | `mdl/executor/validate_program.go` (the shared list), `cmd/mxcli/cmd_exec.go` (the gate), `cmd/mxcli/cmd_check.go` | `executor.ValidateProgram` is now the single definition of what `check` checks, and `exec` refuses to run when it reports an **error** (warnings print and do not block). `--no-check` opts out. **A check wired into nothing is invisible** — it reports no violations and reads as a clean project — so `TestValidateProgram_WiresEveryWholeProgramValidator` asserts structurally that every exported `Validate*(prog *ast.Program, …)` is called from `ValidateProgram`; per-statement helpers take a statement and are excluded. When adding a check, add it to `ValidateProgram` and both commands get it | +| The expression type checker reports nothing, ever. `mdl/exprcheck` is a complete checker — parser, type lattice, function table, hints registry, slot resolver, and a `Context` with two tiers — and a suite of semantic rules (E001 enum-vs-string-literal, E009 slot-type mismatch, …) that never fire on any real project. `mxcli check --references` is green on a script with obvious type errors | Three silent absences, each of which alone reduces the whole thing to a no-op. (a) **Nothing implemented `exprcheck.CatalogReader`** — `grep -rn "exprcheck.CatalogReader"` outside the package returned zero hits — so every invocation ran with a nil `Catalog` and `Context.IsSemanticEnabled()` was false. (b) Three of the five lookups had **no data in the catalog**: `attributes_data.DataType` is the bare kind (`"Enumeration"`, losing which one), `enumerations_data` stored `ValueCount` but not the values, `microflows_data` stored `ParameterCount` but not the parameters. (c) The adapter's `exprSource` read **only `ast.SourceExpr`**, and the visitor attaches one to some slots and not others — measured on a fixture, neither a CREATE's nor a CHANGE's enum value carried one, so the walk had nothing to parse | `mdl/exprcatalog/` (new), `mdl/catalog/tables.go` + `builder_modules.go` + `builder_microflows.go` (schema 10), `mdl/exprcheck/adapters/check.go` (`WithSourceFunc`, trim, `CheckNanoflow`), `mdl/executor/typecheck.go` (new), `cmd/mxcli/cmd_check.go` | Implement the seam as a **memoized index loaded once** (four queries), not SQL per lookup. **Bump `CatalogSchemaVersion`** when adding a column the checker reads: without it a cached catalog answers "unknown" for every lookup, which the checker reads as "cannot tell" and skips — a green run that checked nothing, i.e. the original bug wearing a fresh cache. Keep the **E0xx codes** rather than remapping: the code in the message must be the code you can look up. **Generalisable, and the reason this sat undetected**: a seam with zero implementations is invisible to the compiler and to tests that mock it — when auditing an interface, grep for *implementations* (`var _ Iface = `, or the interface name outside its own package), not for callers; and when a checker's failure mode is "unknown → skip", every missing input looks exactly like a clean project. **Verify against a real project, never a mock** — all three defects survive a mocked test. Controls: nil reader → 0 violations, default source func → 0 violations, fixed → 2 (CREATE and CHANGE). False-positive probe: all 21 microflows of a 11.13.0 app described back to MDL and re-checked → 0 findings. **Known depth limit**: `AttributePathExpr` still infers `KindUnknown`, so `if $obj/Status = 'Open'` is NOT caught — only slot-qualified positions (create/change members) are. That is the proposal's open Tier-2 item, not a regression | | A `show`/`list` command **exits 0, prints nothing, and does nothing** — `mxcli -c "show page Mod.Home"` looks like a page with no content, `show connections` looks like no connections are open. No error, no parse failure, no output. Indistinguishable from a legitimately empty result, which is why it survives: the answer is plausible | The grammar alternative exists (`showOrList PAGE qualifiedName`, `showOrList CONNECTIONS`, `showOrList NOTEBOOKS`) but `ExitShowStatement` has **no `else if` branch for it**, so the visitor appends no statement and the program runs zero statements. Not a handler bug — the AST node is never created. `create notebook … begin end` is the same shape (grammar with no AST/visitor/executor anywhere) | `mdl/visitor/visitor_query.go` (`ExitShowStatement`), `mdl/grammar/domains/MDLCatalog.g4`, `mdl/ast/ast_query.go`, `mdl/executor/executor_query.go` | Either wire a visitor branch or delete the grammar alternative — an unimplemented command must fail loudly. `SHOW PAGE X` became an alias for `DESCRIBE PAGE` (matching `SHOW ENTITY`), `SHOW CONNECTIONS` wired to `sql.Manager.List()`, `SHOW NOTEBOOKS` removed. **A structural grep cannot find these**: `ctx.PAGE()` *does* appear in `ExitShowStatement` — for the unrelated `SHOW ACCESS ON PAGE` branch — so a token search reports the broken alternative as covered. The guard (`TestEveryShowAlternativeProducesAStatement`) instead synthesizes a probe from each grammar alternative and asserts a statement comes out. **Ordering trap when adding a branch**: `SHOW DATABASE CONNECTIONS` also matches `CONNECTIONS()`, so a new `CONNECTIONS` branch placed first swallows it and answers a stored-document query with live session state — guard with `ctx.DATABASE() == nil`. Found while auditing mxcli against Studio Pro's document-type list. **Follow-up**: the notebook grammar (`createNotebookStatement`, `notebookOptions`, `notebookPage`, `alterNotebookAction`, the `SHOW NOTEBOOKS` alternative, and the `NOTEBOOK`/`NOTEBOOKS` lexer tokens) was removed outright — all four of `create`/`alter`/`drop`/`show notebook` were silent no-ops with no Go code behind any of them. Dead grammar is not inert: it *accepts input and discards it*. Removing the tokens also frees `notebook` as an ordinary identifier. **Control for a broad grammar change**: sweep every `mdl-examples/**/*.mdl` through `mxcli check` before and after and diff the failure sets — 15 scripts fail either way, so the raw count proves nothing and only the set difference does | | `mxcli check` reports **MDL003** *"microflow returns X but not all code paths have a return statement"* on a microflow that is correct and builds cleanly. Since `exec` began refusing scripts whose checks report an error, the script also will not run without `--no-check` — so a false positive became a hard block. Seven shipped `mdl-examples` scripts were in this state | The microflow uses the documented `RETURNS T AS $Var` form and assigns `$Var` instead of writing an explicit `RETURN`. `buildFlowGraph` sets the final EndEvent's `ReturnValue` to `"$"+Var` **whenever the AS clause is present** (`cmd_microflows_builder_graph.go`), so the return is synthesized either way — that is what the clause is for. The check demanded a `RETURN` statement in the source on top of it | `mdl/executor/validate_microflow.go` (Check 5 in `microflowValidator.validate`) | Skip MDL003 when `returnType.Variable != ""`, mirroring the builder's own condition exactly rather than inventing a second rule. **Verify against mxbuild, not intuition**: the flagged microflow builds with **0 errors** on 11.13.0, `describe microflow` renders the synthesized `return $Found;`, and the stored EndEvent carries a `ReturnValue` — three independent confirmations the check was wrong, not the script. Keep a companion test for the no-AS-clause case, or a fix that simply disables the rule passes. **Sweeping examples as a control**: exclude `*.test.mdl` (those are `mxcli test` files with `@test`/`@expect` blocks, unparseable by `check` by design) and `*.fail.mdl`, and diff failure SETS rather than counts | | A guard that walks a stored BSON document works on the **modelsdk** engine and is a **silent no-op on legacy** (or vice versa) — same project, same document, opposite behaviour. Here `checkNoQueuedCalls` refused a rewrite that would drop a task-queue binding under modelsdk, and let it through under `--engine legacy`, destroying the binding exactly as the guard existed to prevent | The two readers return **different Go shapes for the same BSON**: `modelsdk` yields `[]interface{}` for arrays, the legacy `mpr` reader yields **`bson.A`**. `bson.A` is a NAMED slice type (`type A []interface{}`), so a type switch `case []any:` does **not** match it and the walk never descends into `ObjectCollection.Objects` | `mdl/executor/validate_queued_calls.go` (`queuedCallTargets`, `storedQueueRetries`); the same blind spot exists in `mdl/backend/mcp/page.go`, `mdl/backend/mcp/page_mutator.go`, `mdl/executor/widget_sync_apply.go` | Add `case bson.A:` alongside `case []any:` in every raw-BSON walk. The house pattern already does this (`mdl/backend/bsonnav`, `mdl/catalog/builder_pages.go`, `mdl/xpathrefs/scan.go` and ~7 more) — the ones missing it are the outliers: `for f in $(grep -rln "case \[\]any:" --include=*.go mdl/); do grep -q "bson.A" $f \|\| echo $f; done`. **Test both shapes of the same document in one test**, not just the one your engine returns. **A unit test alone is not enough to find this**: it only surfaced by running the same command under `MXCLI_ENGINE=legacy` and diffing against modelsdk — engine parity has to be exercised, not assumed. Reported as FINDINGS #25 | @@ -547,3 +558,7 @@ extracting `OffsetExpression`/`LimitExpression`. | `create or replace` on a document whose module holds a same-named **excluded** twin edits the WRONG one — the live document keeps its old body and the script looks like a no-op — and the rewrite also clears the twin's `Excluded` flag, so a project that built at 0 errors fails **CE0122** "Duplicate document name". Also `describe` returns the excluded document's body | Studio Pro's "Exclude from project" makes a document name non-unique: Mendix allows the duplicate as long as at most one is active (measured on 11.13.0 — excluded pair = 0 errors, both active = CE0122). Every by-name lookup took the FIRST match, so which document it hit depended on enumeration order; and every rebuild wrote `Excluded` from the AST default (`false`) instead of carrying the stored value. `@excluded` exists and round-trips through DESCRIBE, so absence of the annotation must mean "the script does not say", never "make it active" | `mdl/executor/excluded_docs.go` (`pickLive`) + the create paths: `cmd_microflows_create.go`, `cmd_nanoflows_create.go`, `cmd_pages_create_v3.go` (pages **and** snippets), `cmd_enumerations.go`, `cmd_queues.go`, `cmd_workflows_write.go`, `cmd_javaactions.go`, `cmd_javascript_actions_write.go`, `cmd_businessevents.go`, `cmd_published_rest.go`, `cmd_dbconnection.go`, `cmd_datatransformer.go`, `cmd_import_mappings.go`, `cmd_export_mappings.go`, `cmd_jsonstructures.go`, `cmd_imagecollections.go`, `cmd_agenteditor_*.go`, plus `cmd_microflows_show.go` for DESCRIBE | Route every by-name lookup through `pickLive` (live match wins; an all-excluded set still resolves to the first, so lookups do not start reporting "not found") and carry the stored flag next to the ID/roles each path already preserves. Four types had **no** `Excluded` field to carry — `model.Enumeration`, `types.JavaAction`, `types.ImageCollection`, `pages.Snippet` — so the field had to be added and populated in BOTH engines' readers (`TestFieldCountDrift` catches the `mdl/types` half and requires `convert.go` + the expected counts to be updated). Backends that hardcoded `SetExcluded(false)` (enumeration, snippet, image collection) are the same bug wearing a different hat. **Measure, do not read**: queues looked correct (their reader and writer both carry the flag) and still dropped it, because the executor built a fresh struct — the matrix run is what found it. Pages/snippets additionally collected ALL name matches and DELETED the extras, which destroyed the excluded twin outright. Tests `TestPickLive`, `TestCreateOrModifyMicroflow_PreservesStoredExclusion`, `TestCreateOrModifyMicroflow_TargetsLiveTwin` (both fail with the reported symptoms when the fix is reverted); fixture `mdl-examples/bug-tests/914-excluded-document-preserved.mdl`. Issue #914 | | Lint CONV010 flags **every** `ACT_` microflow that shows a page, closes one, or calls a sub-microflow — 11 false positives out of 13 findings, burying the real ones | The rule's `ALLOWED_ACTIONS` held the Mendix **storage** names (`ShowFormAction`, `CloseFormAction`); the catalog labels an action with its **SDK** name, derived from the parsed Go type in `getMicroflowActionType` (`ShowPageAction`, `ClosePageAction`, `MicroflowCallAction`). The allowlist matched nothing | `.claude/lint-rules/conv010_act_microflow_content.star`, `mdl/catalog/builder_microflows.go` | List the SDK names (both spellings is cheap insurance). The storage-name split is the same one in CLAUDE.md's `$Type` table — it bites rule authors because the catalog deliberately does **not** use storage names. `mdl/catalog/lint_rule_vocabulary_test.go` pins the allowlist to what `getMicroflowActionType` actually returns, so the rule cannot drift from the labeller again. Copy the rule into the target project's `.claude/lint-rules/` when testing: `mxcli lint -p ` prefers the **project's** copy over the embedded one, so editing the repo's copy alone changes nothing | | Lint QUAL004 reports a live microflow as "not called from anywhere" (page datasource, widget button, calculated attribute), or a navigation-only page as orphaned | The rule counted only the `call` and `schedule` reference kinds. The builder emits `datasource`, `action` and `calculate` for microflows, and `home_page` / `login_page` / `menu_item` for pages — all ignored. The page half was masked by `ENTRY_PAGE_PATTERNS`, which happens to cover the pages most likely to be navigation targets | `.claude/lint-rules/orphaned_elements.star`, `mdl/catalog/builder_references.go` | Count every kind that means "this runs" / "this opens", via the `MICROFLOW_ENTRY_KINDS` / `PAGE_ENTRY_KINDS` lists. `TestQUAL004CountsEveryEntryPointKind` fails when one goes missing and `TestQUAL004EntryKindsAreRealRefKinds` when one is misspelled. Adding a new `RefKind` that means reachability means adding it to the right list | +| `mxcli check` reports `✓ Syntax OK`, `exec` writes the microflow, and the defect appears only when a human opens Studio Pro's Errors pane: CE0038 on a value-less `declare`, CE0068 on a `return` inside a loop, CE0111 on `declare $X` followed by `$X = call microflow …` | Nothing in the MDL rule set covered them — each is a Mendix consistency rule with no MDL counterpart. CE0111's real scope is far wider than the reported case: a microflow's variable namespace is **flat**, so parameters, loop iterators and every activity output share it, and neither a branch nor a loop body opens a scope (all seven combinations measured on mxbuild 11.6.6) | `mdl/executor/validate_microflow_ce_gaps.go` (MDL061/062/063), wired from `validate_microflow.go`; fixtures `mdl-examples/bug-tests/893-check-gaps-*.mdl` | Error severity alone closes the gap — `exec` pre-flights the whole script and refuses with nothing written; they are deliberately kept OUT of `execEnforcedMicroflowRules` so `--no-check` still works. **Run any new rule over `mdl-examples/` before wiring it up**: this one hit 4 of 374 files and 3 were FALSE positives, because the rule reads the AST while the outcome depends on what the BUILDER emits — `while true` becomes an ExclusiveMerge back-edge and not a loop object (#350), `returns T as $Var` routes the End event elsewhere, `set $x = contains($str,$str)` parses as a ListOperationStmt that the builder rewrites to a Change Variable (ledger #53/#63), and an `@excluded` document is never checked by mxbuild at all. The 4th was a genuine CE0111 in a shipped example. The shared predicate `stringOverloadedListOp` keeps rule and builder from drifting. Issue #893 items 1/2/6 | +| `calculated by Module.Microflow` on an attribute is accepted by `check` and by exec ("Added attribute"), but the stored document holds a plain `DomainModels$StoredValue` with no calculation link — the microflow name appears nowhere in the domain model unit, `mx check` reports **0 errors**, and the attribute is simply empty at runtime. Both the CREATE (inline) and ALTER (`ADD`/`MODIFY ATTRIBUTE`) paths. A microflow whose signature cannot work is accepted too, masked by the same drop | `attributeToGen` in the **modelsdk** writer had arms for OqlViewValue / ODataMappedValue / ODataMappedPrimitiveCollectionValue and a `default:` that emits StoredValue — no `CalculatedValue` arm — so the binding the executor had already resolved fell through and was discarded. The **legacy** writer had the arm all along (`sdk/mpr/writer_domainmodel.go`), which is why the feature read as implemented; modelsdk is the default engine (`--engine`), so everyone hit the broken path. The reader had no `CalculatedValue` case either, so an unrelated ALTER on the same entity destroyed a binding made in Studio Pro | `mdl/backend/modelsdk/domainmodel_write.go` (`attributeToGen`) + `domainmodel.go` (`attributeFromGen`) + `mdl/executor/calculated_attributes.go` (`resolveCalculatedValue`, called from the three sites in `cmd_entities.go`) | Add the write arm (`genDm.NewCalculatedValue`, `SetMicroflowQualifiedName` → the `Microflow` ByNameRef key, `SetPassEntity`) **and** the read arm — a write-only fix leaves the read-modify-write data loss in place, which is the worse half. Derive `PassEntity` from the signature rather than hardcoding it (legacy hardcoded `microflowRef != ""`): measured on 11.13.0, an entity-parameter microflow (`PassEntity=true`) and a parameterless one (`PassEntity=false`) BOTH build at 0 errors, so refusing the parameterless form would have been wrong. Signature rules are refused at exec (the #833 placement), and each was checked against mxbuild rather than assumed — wrong entity parameter and wrong return type are both **CE7247**, but the return-type message is *"should be Integer/Long"*, so **Integer and Long are one family** and a strict equality check refuses valid MDL (caught only by reading the CE text). To ask mxbuild about a binding mxcli now refuses, stub the check and rebuild — `--engine legacy` does NOT bypass it, because the validation lives in the engine-independent executor. Tests `TestAttributeToGen_CalculatedValue`, `TestAttributeFromGen_CalculatedValue`, `TestResolveCalculatedValue_*` (the backend three fail with `value is *domainmodels.StoredValue` when reverted); fixture `mdl-examples/bug-tests/917-calculated-attribute-binding.mdl`. Issue #917 | +| `DESCRIBE IMPORT MAPPING` output does not reproduce the script that made it: `Total = total` comes back as `Total = Total`, an array binding as `= ItemItem`, `LineId = id` as `LineId = _id` — and the output cannot be re-run at all, failing with "import mapping already exists". Export mappings identically (unreported) | DESCRIBE printed the element's **ExposedName** (Mendix's display name — capitalised initial, `Item` suffix on an array's item object) instead of the raw JSON key from `JsonPath`, and emitted a bare `create` header where every other DESCRIBE emits `create or modify` | `mdl/executor/cmd_import_mappings.go` (`mappingMemberName`, the four print sites, the header) + `cmd_export_mappings.go` (same four + header) | Print the raw key derived from `JsonPath` — strip a trailing `\|(Object)` first, because an array's mapping element sits at the ITEM object while the script addressed the array (that suffix is what produced `ItemItem`). Fall back to ExposedName when there is no JsonPath (XML-schema / message-definition mappings have none). Safe by construction: the raw path is `jsonSchemaIndex.resolve`'s FIRST lookup, so it cannot regress #882. **Do NOT "fix" ExposedName itself** — the capitalisation is Mendix's own, confirmed against a Studio Pro-authored document in the blank app (`ExposedName "Uuid"` vs `Path "(Object)|uuid"`); rewriting it would diverge from Studio Pro. The `Item` suffix could NOT be confirmed the same way (a blank app has no Studio Pro array structure) and was left alone — with a separate `ExposedItemName` property in the BSON, that is worth checking against a marketplace module before anyone touches storage. Note the issue's framing was half wrong: the mapping DID round-trip semantically (re-executing the old output rebuilt byte-identical JsonPaths), so this was a text/diff defect, not a broken mapping — measure before agreeing with a title. Tests `TestMappingMemberName`, `TestDescribe{Import,Export}Mapping_RoundTripsMemberNames` (fail with the reported symptoms when reverted); two existing header assertions needed updating with the intentional change; fixture `mdl-examples/bug-tests/915-mapping-describe-roundtrip.mdl` is a DESCRIBE fixed point. Issue #915 | +| `mxcli check -p app.mpr` passes a microflow containing `if $obj/Status = 'Open'` — comparing an enumeration attribute to a string literal, the first example in the type-checking proposal and the shape people actually write. The same mistake written as a create or change member *is* caught, which makes the gap look arbitrary | Two causes, both invisible. (a) **`inferKind` returned `KindUnknown` for every `AttributePathExpr`**, so `$obj/Attr` typed to nothing and every rule downstream of it stayed quiet. `exprcheck.Scope` is `Lookup(name) (TypeKind, bool)` — it can say "$P is an Object" but not *which* entity, so there was nowhere to put the answer; the adapter computed a variable→entity map (`buildVarEntityScope`) and used it only to label a slot path, never passing it to the checker, and it covered body-introduced variables but **not parameters** — the ordinary case. (b) Even resolved, nothing fired: **E001 keys off the SLOT** (`CreateItem.Value:Entity.Attr`), which exists for an assignment and not for a comparison | `mdl/exprcheck/interfaces.go` (`EntityScope`, `Context.Entities`), `mdl/exprcheck/parser.go` (`attributePathKind`, `pathTargetEntity`, `checkEnumComparedToString`), `mdl/exprcheck/adapters/` (`walkFlow`, `addParamEntities`, `entityScope`), `mdl/exprcatalog/` (`AssociationTarget`) | Put the object side in its **own seam** (`EntityScope`: `VariableEntity` + `AssociationTarget`) beside `Scope` rather than widening either it or `CatalogReader` — `CatalogReader`'s shape is what stays re-syncable from the upstream fork. **A multi-hop expression path is not an XPath path**: XPath spells the intermediate entity (`[Assoc/Entity/Attr]`) so a walk can read it off, while an expression does not (`$O/Mod.Assoc/Attr`), so every hop must resolve through the association index. For (b), emit the **same code and message** from the comparison as from the slot — one defect should not have two names depending on where it was spotted. **Generalisable**: when a checker is silent, separate "did the input resolve" from "is there a rule for this shape" before touching either — here resolution and detection were both missing, and fixing only one would have looked like the fix failed. Verify with a **probe corpus that actually exercises the construct**: 21 microflows described back to MDL contained 40 attribute-path lines including an association hop, and were clean before and after, which is what makes "no false positives" mean something. Controls: path→`KindUnknown` restored, `Entities` dropped from the Context, and `addParamEntities` removed each fail a distinct test. **Still open**: a *terminal* association step (`$Order/Mod.Order_Lines`) types to unknown — Object vs List depends on the association's kind and direction, and guessing costs false positives | diff --git a/.claude/skills/mendix/alter-page.md b/.claude/skills/mendix/alter-page.md index 9a9b8dacc..87bc8d972 100644 --- a/.claude/skills/mendix/alter-page.md +++ b/.claude/skills/mendix/alter-page.md @@ -358,6 +358,63 @@ alter page MyModule.Customer_Edit { }; ``` +## DataGrid 2 columns: how to address them, and what you can set + +**Mendix stores no column name.** A DataGrid 2 column's schema has no name or +identifier key — the only human-facing label is its caption — so the name you +write in MDL is dropped: + +```mdl +create or replace page Mod.P (...) { + datagrid dg1 (datasource: database Mod.Item) { + column colLabel (attribute: Label, caption: 'The Label') -- "colLabel" is not stored + } +}; +``` + +`describe page` shows that column as `Label`, and that is the name `ALTER PAGE` +answers to: + +```mdl +alter page Mod.P { SET Caption = 'Renamed' ON dg1.colLabel } -- WRONG: column not found +alter page Mod.P { SET Caption = 'Renamed' ON dg1.Label } -- correct +``` + +The derived name is, in order: **the bound attribute's short name**, else the +**sanitized caption**, else **`colN`** by position. `mxcli check` reports +**MDL-WIDGET16** when the name you wrote differs from the one that will address +the column, so you find out at authoring time rather than from a failed ALTER. + +Two columns that derive the same name are ambiguous and ALTER refuses rather than +picking one — give them distinct captions. + +### Setting column properties + +Property names resolve against the keys the installed widget declares, so both +the schema key and mxcli's MDL alias work (`DynamicCellClass` and `ColumnClass` +both reach `columnClass`). An unknown name lists what *is* settable on that grid. + +**Expression-valued properties take a Mendix expression, not a literal.** +`DynamicCellClass` and `Visible` are expressions, so a literal CSS class has to be +a quoted string *inside* the expression — doubled quotes in MDL: + +```mdl +-- WRONG: the expression becomes a bare identifier, mxbuild reports CE0117 +alter page Mod.P { SET DynamicCellClass = 'highlight' ON dg1.Label } + +-- correct: the expression is the string literal 'highlight' +alter page Mod.P { SET DynamicCellClass = '''highlight''' ON dg1.Label } +``` + +This applies equally to `create page`; the two paths behave identically. A bare +identifier is not a valid Mendix expression, and mxbuild reports CE0117 against +the column. + +Properties holding a **structured** value — `attribute`, `filter`, `content`, +actions — cannot be set by ALTER at all. It refuses them and points at +`create or replace page`, rather than writing a string where Mendix expects a +reference. + ## Common Mistakes | Mistake | Fix | diff --git a/.claude/skills/mendix/check-syntax.md b/.claude/skills/mendix/check-syntax.md index 01e05a7b4..7f1ca8a49 100644 --- a/.claude/skills/mendix/check-syntax.md +++ b/.claude/skills/mendix/check-syntax.md @@ -31,6 +31,24 @@ It also does not mean the script is *correct*. `mxcli check` validates MDL synta and mxcli's own rules; it does not validate the Mendix model. Run `mx check` (or `mxcli docker check -p app.mpr`) after applying a slice. +### `-p` resolves references — there is no separate opt-in + +`mxcli check script.mdl` alone checks syntax and the semantic rules that need no +model. **Pass `-p` and it also resolves every reference** — modules, entities, +pages, microflows and icons — against that project: + +```bash +mxcli check script.mdl # syntax + model-free rules +mxcli check script.mdl -p app.mpr # ... and every reference resolved +``` + +`--references` is implied by `-p` and is kept only so existing scripts keep +working. It used to be required, which meant `mxcli check script.mdl -p app.mpr` +printed an unqualified `Check passed!` having resolved nothing — a misspelled +icon or entity sailed through a command that had been handed the project. A run +without a project now says what it did not check, so a pass is never read as +more than it is. + ## Pre-Flight Validation Checklist Before writing any MDL, verify these requirements: @@ -137,6 +155,33 @@ Run `mxcli syntax keywords` for the full list of 320+ reserved keywords. | `page not found` | Page doesn't exist | Check qualified name with `--references` | | `entity not found` | Typo or wrong module | Use fully qualified name | +## Two rules that only real validation used to catch + +Both are decidable from the MDL alone and now fail `check`, because a project +found them the hard way — four scripts passed `check` with 0 errors, executed +cleanly, and `mx check` then reported them: + +| Rule | MxBuild | What it catches | +|---|---|---| +| `MDL-SEC20` | CE0156 | `CREATE USER ROLE` with no **System** module role — nobody holding it can sign in or read System entities. Add `System.User`. **Warning by default, error when the script enables security** (see below). | +| `MDL-PAGE20` | CE5601 | A page with **parameters and a `Url`** where the URL has no segment for a parameter. Mendix binds each parameter from the URL, so the page cannot be opened by link. | + +`MDL-SEC20`'s severity follows the security level, because the underlying error +does. Measured on Mendix 11.13: the same role is **CE0156 at security level +Prototype and no error at all at level Off**, where roles are stored but not +validated. A blank project ships `Off`. So the rule warns by default and is an +error only when the script itself contains `ALTER PROJECT SECURITY LEVEL` set to +something other than `Off` — at which point the author has said which world they +are in. + +`MDL-PAGE20` accepts an attribute path in the segment (`url: 'p006/{Customer/Name}'`), +which is the usual shape — it matches the segment's leading name, not the whole +segment. + +**`check` is still necessary, not sufficient.** Run `mx check` (or +`mxcli docker check`) after every `exec`; these two rules narrow the gap, they do +not close it. + ## Validation Workflow ### Before Writing MDL diff --git a/.claude/skills/mendix/generate-domain-model.md b/.claude/skills/mendix/generate-domain-model.md index 54ca53fcc..85c8c870e 100644 --- a/.claude/skills/mendix/generate-domain-model.md +++ b/.claude/skills/mendix/generate-domain-model.md @@ -488,6 +488,28 @@ create persistent entity Module.OrderLine ( - `calculated Module.Microflow` — also valid (`by` keyword is optional) - `calculated` — bare form, marks as calculated but requires manual microflow binding in Studio Pro +**The microflow's signature is checked, and mxcli refuses a mismatch before +writing** — Mendix reports these as **CE7247** at build time (verified on 11.13.0): + +| Microflow | Result | +|-----------|--------| +| takes the owning entity (`$Order: Module.Order`) | ✅ stored with `PassEntity = true` | +| takes **no** parameter | ✅ stored with `PassEntity = false` — equally valid | +| takes a *different* entity | ❌ refused: CE7247 *"Microflow parameter 'X' should be of type Module.Order."* | +| takes two or more parameters | ❌ refused | +| returns the wrong type | ❌ refused: CE7247 *"Microflow return type should be …"* | +| returns `Long` for an `integer` attribute (or vice versa) | ✅ accepted — Integer and Long are one family here | + +A microflow **created earlier in the same script** cannot be inspected yet, so +its signature is not checked; the build has the last word on those. + +> **Before mxcli 0.17 the binding was silently discarded** on the default +> engine: the attribute was written as an ordinary stored value, `mx check` +> reported 0 errors, and the attribute stayed empty at runtime (#917). If you +> have attributes that were declared `calculated by` and never calculated, they +> need re-running through a current mxcli — re-executing the same statement is +> enough. + ### Data Types | Type | Example | Description | diff --git a/.claude/skills/mendix/json-structures-and-mappings.md b/.claude/skills/mendix/json-structures-and-mappings.md index 119b12541..e0cd91b10 100644 --- a/.claude/skills/mendix/json-structures-and-mappings.md +++ b/.claude/skills/mendix/json-structures-and-mappings.md @@ -34,9 +34,14 @@ Consequences worth knowing: - **Either spelling works in MDL.** `Total = total` and `Total = Total` produce the same stored mapping. Write whichever you have. -- **`DESCRIBE` emits the exposed name**, because that is the name Studio Pro shows. - A describe → edit → exec cycle is therefore lossless, but the text you get back - will not match the raw JSON keys you wrote. +- **`DESCRIBE` emits the raw JSON key**, so its output reproduces the script that + produced the mapping — `Total = total` comes back as `Total = total`, and an + array binding as `= item` rather than `= ItemItem`. It also emits + `create or modify`, so the output re-runs against the project it was read from. + (Until #915 it printed the exposed name and a bare `create`: the text differed + from the input, making every script-vs-describe diff noise, and re-running it + failed with "import mapping already exists". The stored mapping was correct + either way.) - **A member matching neither spelling is refused**, listing what would have worked. It is never written with a guessed path: such a mapping passed `mxcli check` and failed later in mxbuild (CE5015) or at runtime. diff --git a/.claude/skills/mendix/migrate-design-prototype.md b/.claude/skills/mendix/migrate-design-prototype.md index be0a93446..cdb88eb32 100644 --- a/.claude/skills/mendix/migrate-design-prototype.md +++ b/.claude/skills/mendix/migrate-design-prototype.md @@ -512,6 +512,43 @@ for screenshotting the running app. Iterate ②–④ per screen until it matche --- +## Never put a grid on a Mendix widget's own class + +A layout that should be two columns comes out as one, and the CSS is right — it +is on the wrong element. Mendix wraps a repeating widget's children in an +intermediate element, so `display: grid` on the widget's own class has exactly +**one** grid item and every card stacks: + +``` +div.mx-listview.my-cards [689x2054] display=grid <- the class you wrote + ul. [334x2038] display=block <- ONE child + li.mx-name-index-0 [334x526] <- the things you meant to lay out +``` + +A data view does the same with `.mx-dataview-content`. One project hit this +twice in two different widgets before naming the rule (mxcli-owid, findings #15 +and #41). + +Put the grid on the element that actually holds the repeated children: + +```scss +/* list view */ +.my-cards > ul { display: grid; grid-template-columns: repeat(2, minmax(0, 1fr)); } +.my-cards > ul > li { min-width: 0; } + +/* data view */ +.my-page > .mx-dataview-content { display: grid; grid-template-columns: 240px 1fr; } +``` + +`min-width: 0` on the child matters: a grid item defaults to `min-width: auto`, +so a wide table or a long unbroken string inside a card pushes the column past +its track instead of scrolling within it. + +**How to find the right element** rather than guess: run the app, inspect the +widget, and walk down from the class you wrote until you reach the element with +one child per row. `mxcli run --local --screenshot` plus the browser inspector +settles it in one pass — see `.claude/skills/verify-in-runtime.md`. + ## Gotchas (learned building this app) - **Never put `Style:` (inline style) on a `DYNAMICTEXT`** — it crashes MxBuild with a diff --git a/.claude/skills/mendix/write-microflows.md b/.claude/skills/mendix/write-microflows.md index 77d0f8e57..83f95f83b 100644 --- a/.claude/skills/mendix/write-microflows.md +++ b/.claude/skills/mendix/write-microflows.md @@ -213,6 +213,9 @@ declare $ProductList list of Test.Product = empty; -- use a parameter, retrieve -- WRONG: Using AS keyword (not supported in mxcli) declare $Product as Test.Product; -- ERROR: parse error +-- WRONG: No value (CE0038, MDL061) +declare $X string; -- a Create Variable activity requires a value + -- WRONG: Missing type declare $Counter = 0; -- Type inference not always supported @@ -390,6 +393,65 @@ end; **Note**: Parameters are automatically declared by the parameter list. The `returns type as $Var` syntax names the return variable but does NOT declare it - you must still use `declare $Var type = value;` if you want to use SET on it. +### 8. RETURN Inside a Loop + +**Error**: CE0068 - "End events cannot be placed inside a loop." (MDL062) + +A `return` builds an End event, and Mendix does not allow one inside a loop — +whether the return sits in the loop body directly or inside a branch within it. + +❌ **INCORRECT:** +```mdl +loop $Part in $PartList +begin + if $Part/IsMatch then + return true; -- End event inside the loop + end if; +end loop; +``` + +✅ **CORRECT** — leave the loop with `break`, and return once after it: +```mdl +declare $Found boolean = false; +loop $Part in $PartList +begin + if $Part/IsMatch then + set $Found = true; + break; + end if; +end loop; +return $Found; +``` + +### 9. Two Activities Creating the Same Variable + +**Error**: CE0111 - "Duplicate variable name 'X'." (MDL063) + +A microflow's variable names are unique **flow-wide**. Branches and loop bodies +do not open a scope, and parameters and loop iterators share the same namespace. +The trap is that every activity with an output **creates** its variable — there +is no form in which a call, a retrieve, an aggregate or an import mapping writes +into one that already exists. + +❌ **INCORRECT:** +```mdl +declare $Session string = ''; +$Session = call microflow Mod.Login(); -- the call creates $Session too +``` + +✅ **CORRECT** — let the activity create it: +```mdl +$Session = call microflow Mod.Login(); +``` + +Assigning to an existing variable is fine, because `set` is a *Change Variable* +activity and creates nothing: + +```mdl +declare $Session string = ''; +set $Session = 'anonymous'; -- valid, any number of times +``` + ## Control Flow ### IF Statements diff --git a/.claude/skills/packs/mendix-odata-pushdown/SKILL.md b/.claude/skills/packs/mendix-odata-pushdown/SKILL.md index b98404a9a..89170c1af 100644 --- a/.claude/skills/packs/mendix-odata-pushdown/SKILL.md +++ b/.claude/skills/packs/mendix-odata-pushdown/SKILL.md @@ -55,7 +55,7 @@ builds the invocation for a resource backed by a stored routine. | `Top`, `Skip` | bind | the page, already clamped to `MaxTop` | | `SortColumn1/2`, `SortDirection1/2` | bind | the sort, as exposed names and `A`/`D` | | `WantsCount` | both | `$count=true` — the client wants the size of the set | -| `Rejected`, `RejectReason` | both | the request asked for something untranslatable | +| `Rejected`, `RejectReason` | bind | the request asked for something untranslatable. Only a **bind** caller sees these: with `RejectUnsupported = true` (what a splice caller passes) `Parse` throws instead of returning, so a splice caller always has `Rejected = false` | ## Two ways to spend it diff --git a/.claude/skills/packs/mendix-odata-pushdown/references/failure-modes.md b/.claude/skills/packs/mendix-odata-pushdown/references/failure-modes.md index 9673862c9..3d4215784 100644 --- a/.claude/skills/packs/mendix-odata-pushdown/references/failure-modes.md +++ b/.claude/skills/packs/mendix-odata-pushdown/references/failure-modes.md @@ -59,8 +59,15 @@ time it is asked for anyway, so the requirement is doing you a favour. **An untranslated filter, on a splice caller that passed `RejectUnsupported = false`.** The splice caller's `WHERE` *is* `FilterSql`. If the filter could not be -translated and was dropped, there is no `WHERE`. Pass `true`: `Rejected` comes -back set, and the caller is expected to fail the request. +translated and was dropped, there is no `WHERE` — every row in the table, under +a 200, in answer to a request for a handful. + +Pass `true`. `Parse` then **throws** rather than returning a Query, so the +request becomes a 500 with the reason in the runtime log. Do not write a +microflow branch on `Rejected` for this: a splice caller that receives a Query +at all always has `Rejected = false`, so the branch is dead code (mxcli-owid, +finding #30). Reading `Rejected` is for a **bind** caller, which passes `false` +and does get a Query back. `$orderby` is the one thing dropped rather than rejected. A wrong order is cosmetic; a wrong row count is not. diff --git a/.claude/skills/packs/mendix-odata-pushdown/references/patterns.md b/.claude/skills/packs/mendix-odata-pushdown/references/patterns.md index 08ee4d772..9dee5e0ec 100644 --- a/.claude/skills/packs/mendix-odata-pushdown/references/patterns.md +++ b/.claude/skills/packs/mendix-odata-pushdown/references/patterns.md @@ -19,10 +19,6 @@ $Q = CALL JAVA ACTION {{MODULE}}.Parse( KeyField = 'driverId', RejectUnsupported = true); -IF $Q/Rejected THEN - -- fail the request; do not answer it with unfiltered rows -END - DECLARE $Sql String = 'SELECT d.* FROM drivers d' + $Q/FilterSql + $Q/OrderBySql; ``` @@ -30,6 +26,27 @@ DECLARE $Sql String = 'SELECT d.* FROM drivers d' + $Q/FilterSql + $Q/OrderBySql `FilterSql`, so an untranslated filter means no `WHERE` at all — every row in the table, under a 200, in answer to a request for a handful. +**Do not write `IF $Q/Rejected THEN` in a splice caller — the branch cannot +run.** With `RejectUnsupported = true`, `Parse` throws before it builds the +Query: + +```java +if (r.rejected && Boolean.TRUE.equals(rejectUnsupported)) { + throw new IllegalArgumentException("cannot translate OData query: " + r.rejectReason); +} +IMendixObject o = Core.instantiate(context, ENTITY); // never reached when rejected +``` + +So a microflow that receives a `Query` at all always has `Rejected = false`. The +untranslatable request has already become a 500 with the reason in the runtime +log — which is the honest answer, and is why the throw is there. A guard +microflow written for that branch never executes; one project wrote and then +deleted a Java action for exactly this (mxcli-owid, finding #30). + +`Rejected` / `RejectReason` are still worth reading — by a **bind** caller +(`RejectUnsupported = false`), which does receive a Query and needs to log the +filter it was never going to apply. + --- ## Bind — the SQL is somebody else's diff --git a/cmd/mxcli/cmd_check.go b/cmd/mxcli/cmd_check.go index 2d6e506f4..634a28962 100644 --- a/cmd/mxcli/cmd_check.go +++ b/cmd/mxcli/cmd_check.go @@ -18,15 +18,28 @@ var checkCmd = &cobra.Command{ Short: "Check an MDL script for errors without executing it", Long: `Check an MDL script file for syntax errors and optionally validate references. -By default, only checks syntax (parsing). Use --references to also validate -that all referenced modules, entities, etc. exist in the project. +Without a project it checks syntax and the semantic rules that need no model. +Pass -p and it also resolves every reference — modules, entities, pages, +microflows and icons — against that project; --references is implied by -p and +is kept only for compatibility. Reference validation is smart: it automatically skips references to objects that are created within the script itself. For example, if your script creates a module "MyModule" and then creates entities in it, no error will be reported for the module reference. -Output includes structured rule IDs (MDL prefix) for each validation issue. +Given a project it also type-checks the expressions in the script's microflows +and nanoflows: comparing an enumeration attribute to a string literal (in a +create/change member, or in a condition such as: if $obj/Status = 'Open'), +operand and argument type mismatches, and the like. Attribute paths resolve +through associations too, so $Order/Sales.Order_Customer/Name is typed. +These need the project to answer what an attribute's type is and which values an +enumeration has, which is why they need -p. They report under exprcheck's own +E0xx codes, and — like every other check here — only an error severity fails the +run. + +Output includes structured rule IDs (MDL prefix for reference and script rules, +E0xx for expression type rules) for each validation issue. Use --post-migration to scan an existing project (independent of the script) for legacy native widgets that have pluggable replacements — Studio Pro does @@ -36,8 +49,8 @@ Examples: # Check syntax only (no project needed) mxcli check script.mdl - # Check syntax and validate references against a project - mxcli check script.mdl -p app.mpr --references + # Check syntax, resolve references, and type-check expressions + mxcli check script.mdl -p app.mpr # Scan the project for legacy native widgets after a Mendix upgrade mxcli check script.mdl -p app.mpr --post-migration @@ -53,7 +66,14 @@ Examples: Run: func(cmd *cobra.Command, args []string) { filePath := args[0] projectPath, _ := cmd.Flags().GetString("project") + // A project makes reference resolution possible, so it runs. It used to + // need --references as well, which meant `mxcli check script.mdl -p + // app.mpr` printed an unqualified "Check passed!" having resolved + // nothing — icons, entity and page references all silently unchecked. + // Someone who hands the command a project has said what they want; the + // flag stays accepted so existing invocations and scripts keep working. checkRefs, _ := cmd.Flags().GetBool("references") + checkRefs = checkRefs || projectPath != "" postMigration, _ := cmd.Flags().GetBool("post-migration") format := resolveFormat(cmd, "text") isStructured := format != "" && format != "text" @@ -177,6 +197,34 @@ Examples: if !isStructured { fmt.Printf("✓ All references valid\n") } + + // Expression type checking is the catalog-backed tier: the rules that + // need an attribute's type, an enumeration's cases or a microflow's + // return type. It runs here rather than in the unconditional pass + // because those answers only exist once a project is connected, and + // after the reference check because a script naming things that do + // not exist has a more basic problem than a mistyped operand — and + // because building the catalog for a run that already failed is + // wasted work. + // + // Like every other violation this command emits, only an error + // severity fails the run. Warnings and hints are advice, and a + // checker whose first outing turns advice into a broken build is a + // checker people turn off. + typeViolations := exec.TypeCheckProgram(prog) + if len(typeViolations) > 0 { + if isStructured { + formatter.Format(typeViolations, os.Stderr) + } else { + fmt.Fprintln(os.Stderr) + formatter.Format(typeViolations, os.Stderr) + } + if linter.Summarize(typeViolations).Errors > 0 { + os.Exit(1) + } + } else if !isStructured { + fmt.Printf("✓ Expression types OK\n") + } } // Post-migration scan: walk the project for native widgets that @@ -214,6 +262,15 @@ Examples: if !isStructured { fmt.Println("\nCheck passed!") + // Qualify the verdict when nothing was resolved against a model. A + // bare "Check passed!" reads as more than it is: without a project + // no icon, entity, page or microflow name in the script has been + // looked up, and those are exactly what this command gets reached + // for. Saying so beats leaving the reader to infer it. + if !checkRefs { + fmt.Println(" (no project given — icon, entity, page and microflow references were") + fmt.Println(" not resolved; re-run with -p for full coverage)") + } } }, } diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index 919703506..04d8ee3a6 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -134,7 +134,7 @@ Examples: rules.NewWeakPasswordPolicyRule(), rules.NewDemoUsersActiveRule(), rules.NewOverlappingActivitiesRule(), // MPR008 - requires BSON inspection - rules.NewLoopChildContainmentRule(), // MPR011 - requires BSON inspection + rules.NewLoopChildContainmentRule(), // MPR011 - requires BSON inspection rules.NewNoCommitInLoopRule(), // CONV011-CONV014 - require BSON inspection rules.NewExclusiveSplitCaptionRule(), rules.NewErrorHandlingOnCallsRule(), diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index e611ba9de..8f426f5bc 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -312,7 +312,7 @@ func init() { rootCmd.Flags().StringP("command", "c", "", "Execute MDL command(s) and exit") // Check command flags - checkCmd.Flags().BoolP("references", "r", false, "Validate references against the project") + checkCmd.Flags().BoolP("references", "r", false, "Validate references against the project (implied by -p; kept for compatibility)") checkCmd.Flags().String("format", "text", "Output format: text, json, sarif") checkCmd.Flags().Bool("post-migration", false, "Scan the project for legacy native widgets that survived a Mendix upgrade (requires -p)") diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 7a99efaaf..b1c2fcb19 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -441,7 +441,7 @@ it is for pages. | Statement | Syntax | Notes | |-----------|--------|-------| -| Variable declaration | `declare $Var type = value;` | Primitives: String, Integer, Boolean, Decimal, DateTime | +| Variable declaration | `declare $Var type = value;` | Primitives: String, Integer, Boolean, Decimal, DateTime. The value is **required** — Mendix has no uninitialized variable and a bare `declare` is CE0038 / MDL061. Lists (MDL040) and objects (MDL043) cannot be declared at all | | Entity declaration | `declare $entity Module.Entity;` | No AS keyword, no = empty | | List declaration | `declare $list list of Module.Entity = empty;` | | | Assignment | `set $Var = expression;` | Variable must be declared first | @@ -482,9 +482,9 @@ it is for pages. | Enum split | `case $Var when Value then ... end case;` | Enumeration decision branches. Bare enum values (never quoted or qualified), one branch per value **including `(empty)`** (MDL056), no `else` (MDL008), no `AS` alias | | Type split | `split type $Var case Module.Entity ... end split;` | Runtime specialization branches | | Cast | `cast $SpecificVar;` | Downcast inside a type split branch | -| LOOP | `loop $item in $list begin ... end loop;` | FOR EACH over list | +| LOOP | `loop $item in $list begin ... end loop;` | FOR EACH over list. No `return` inside — an End event cannot sit in a loop (CE0068 / MDL062); use `break` and return after the loop | | WHILE | `while condition begin ... end while;` | Condition-based loop | -| Return | `return $value;` | Required at end of every flow path | +| Return | `return $value;` | Required at end of every flow path, and never inside a loop (MDL062) | | Execute DB query | `$Result = execute database query Module.Conn.Query;` | 3-part name; supports DYNAMIC, params, CONNECTION override | | Import mapping | `[$Var =] import from mapping Module.IMM($SourceVar) [all\|first\|limit [offset ]];` | Apply import mapping to string variable. Trailing clause is Studio Pro's Range; omitted = infer from the mapping's root. `first` binds one OBJECT (`limit 1` is a one-element LIST). Mendix rejects `offset` on a non-list mapping (CE6100) | | Export mapping | `$Var = export to mapping Module.EMM($EntityVar);` | Apply export mapping to entity, returns string | diff --git a/docs/11-proposals/PROPOSAL_bootstrap_source.md b/docs/11-proposals/PROPOSAL_bootstrap_source.md new file mode 100644 index 000000000..d05ff2170 --- /dev/null +++ b/docs/11-proposals/PROPOSAL_bootstrap_source.md @@ -0,0 +1,81 @@ +--- +title: Bootstrap hooks that fetch the mxcli the project was built with +status: proposed +date: 2026-08-18 +related: + - .claude/skills/mendix/bootstrap-app.md + - docs-site/src/tools/bootstrap-prompt.md +--- + +# Bootstrap hooks that fetch the mxcli the project was built with + +## Problem + +`mxcli init` writes `.claude/bootstrap-mxcli.sh` with a hard-coded download of +`https://github.com/mendixlabs/mxcli/releases/download/nightly/mxcli--`. + +For a project pinned to a fork that is wrong, and wrong *silently*: after an idle +reap the next session comes back on a different mxcli than the one the app was +built with. Nothing announces the swap — the binary is there, it runs, and its +behaviour differs. A test project hit this and rewrote the generated script by +hand (mxcli-dbreplication, finding F2). + +The same script also conflates two audiences that want opposite things: + +- a **user** of a Mendix app wants a working `./mxcli` in seconds, from a + release, with no toolchain; +- a **contributor to mxcli** wants the binary built from *their* checkout, which + means a clone, an ANTLR jar, and `make build` — minutes, not seconds, and + worth it because the point is to exercise local changes. + +One script cannot be both. Today's is the first with no way to ask for the +second, so the fork case is served by hand-editing generated output — which the +next `mxcli init` overwrites. + +## Proposal + +Emit **two** bootstrap prompts rather than one parameterised script. + +### 1. User bootstrap (default) + +What `init` writes today, with one change: the release source is resolved rather +than hard-coded. In order of preference — + +1. an explicit `--bootstrap-source ` passed to `init`; +2. the origin remote of the repository the running mxcli was built from, when + that is discoverable and is not `mendixlabs/mxcli`; +3. `mendixlabs/mxcli` nightly, as now. + +Rule 2 is what makes the fork case work without anyone having to know about the +flag: a binary built from `ako/mxcli` writes a hook that fetches from +`ako/mxcli`. The version it pins should be the version that wrote it, not +`nightly`, so a reap restores the *same* binary rather than the newest one. + +### 2. Developer bootstrap (opt-in) + +`mxcli init --bootstrap developer` writes a script that clones and builds: +installs the pinned ANTLR jar if `antlr4` is absent, `make build`, and falls back +to a release download when the source build fails so a broken tree does not leave +the session with no mxcli at all. `MXCLI_REPO` / `MXCLI_REF` override the source; +this is essentially the script the reporting project wrote by hand, promoted to +something `init` can emit. + +## Open questions + +- **Is the running binary's origin discoverable?** `main.Version` carries a + commit, not a remote. This may need a build-time ldflag, which is cheap but is + a change to the release pipeline rather than to `init`. +- **Which default for a project created by a fork build?** Rule 2 says "the + fork", which is right for a fork's own test projects and wrong for someone who + built from a fork once and wants the upstream release afterwards. The flag + settles it; the question is which way the *unflagged* case should fall. +- **Cold-start cost.** The developer script takes minutes from a cold container. + Whether that fits inside a SessionStart hook's budget has not been measured — + the reporting project flagged the same worry as its OPEN-1. + +## Not doing + +Making one script switch on an environment variable. It reads as a single +supported path with a hidden mode, and the two paths differ in prerequisites, +runtime and failure modes — a user who accidentally triggers the developer path +gets a multi-minute clone and an ANTLR download they never asked for. diff --git a/docs/11-proposals/PROPOSAL_expression_type_checking.md b/docs/11-proposals/PROPOSAL_expression_type_checking.md index 817f3acf4..b60c4e802 100644 --- a/docs/11-proposals/PROPOSAL_expression_type_checking.md +++ b/docs/11-proposals/PROPOSAL_expression_type_checking.md @@ -695,10 +695,41 @@ refactors (`mprrepos`/`mxgraph`). Cherry-pickable like the modelsdk engine. superseding "Phase 1 — Type infrastructure" above, which `exprcheck` already delivers): -1. Provide our-catalog-backed `CatalogReader` + `SlotResolver` implementations and - finish Tier-2 depth (attribute types, microflow return types) — the - `AttributePathExpr → KindUnknown` gap. -2. Wire the `exprcheck` adapters into **our** `mxcli check` / `validate` path. +1. ~~Provide our-catalog-backed `CatalogReader`~~ **done** — `mdl/exprcatalog`, + a memoized index over the catalog. It needed three catalog additions first + (`attributes_data.EnumerationQualifiedName`, `enumeration_values_data`, + `microflow_parameters_data`): the seam had no implementation *and* three of + its five lookups had no data. Still open: **Tier-2 depth** — `inferKind` + returns `KindUnknown` for `AttributePathExpr`, so `$obj/Attr` resolves to + nothing and only slot-qualified positions (a create/change member, where the + adapter builds `CreateItem.Value:Entity.Attr`) reach the catalog. That is what + makes `if $obj/Status = 'Open'` still pass. **Also done** — an `EntityScope` + seam (`VariableEntity` + `AssociationTarget`) sits beside `Scope` rather than + inside it, because `Scope` speaks `TypeKind` and cannot carry "$P is + Mod.Person"; `CatalogReader`'s shape is left untouched so it stays + re-syncable. `inferKind` now resolves an attribute path, including multi-hop + ones — a Mendix expression, unlike XPath, does not name the intermediate + entity, so each hop resolves through the association index. Two further gaps + closed on the way: the adapter's variable→entity map was computed and never + handed to the checker, and it covered only body-introduced variables, never + **parameters** — the ordinary case. + + `if $obj/Status = 'Open'` needed one more thing than resolution: E001 fired + only from a **slot** (`CreateItem.Value:Entity.Attr`), which exists for an + assignment and not for a comparison. A comparison-side detection now emits the + same code and message from the other operand's resolved type — one defect + should not have two names depending on where it was spotted. + + Still open: a **terminal association** step (`$Order/Mod.Order_Lines`) types + to unknown rather than Object or List, because which one depends on the + association's kind and direction and guessing would cost false positives. +2. ~~Wire the `exprcheck` adapters into **our** `mxcli check` / `validate` path~~ + **done** — `Executor.TypeCheckProgram`, called by `mxcli check --references`. + Two things were wrong in the ported adapter and are worth knowing before + wiring the LSP to it: `exprSource` read only `ast.SourceExpr` (the visitor + attaches one to some slots and not others, so most expressions were invisible + — now injectable via `WithSourceFunc`), and the variable scope it builds is + never passed into the parse `Context`. 3. **Phase 3 LSP** — still to-do; the `Context`-based design makes it straightforward (run with `Scope` only for the inline, project-less path). 4. Decide the cosmetics: keep `exprcheck`'s `E0xx` codes (faithful port) vs remap diff --git a/internal/marketplace/client.go b/internal/marketplace/client.go index 5544c8eff..973e38e78 100644 --- a/internal/marketplace/client.go +++ b/internal/marketplace/client.go @@ -194,20 +194,41 @@ func (c *Client) fetchPages(ctx context.Context, startPage, n int) ([][]Content, // filterItems returns items whose name or publisher contains query // (case-insensitive substring match). func filterItems(items []Content, query string) []Content { - q := strings.ToLower(query) + q := normalizeSearchTerm(query) var matched []Content for _, item := range items { name := "" if item.LatestVersion != nil { - name = strings.ToLower(item.LatestVersion.Name) + name = normalizeSearchTerm(item.LatestVersion.Name) } - if strings.Contains(name, q) || strings.Contains(strings.ToLower(item.Publisher), q) { + if strings.Contains(name, q) || strings.Contains(normalizeSearchTerm(item.Publisher), q) { matched = append(matched, item) } } return matched } +// normalizeSearchTerm folds case and drops the separators that differ between +// how content is packaged and how it is written down. +// +// The API exposes only the packaged name — `Content` carries no display name — +// and packaged names have no spaces. So searching for the module as it is +// written everywhere, "Database Replication", matched nothing while +// "replication" found it. Someone who types the name they were given should not +// have to guess that the space is the problem. +func normalizeSearchTerm(s string) string { + var b strings.Builder + b.Grow(len(s)) + for _, r := range strings.ToLower(s) { + switch r { + case ' ', '-', '_', '.', '\t': + continue + } + b.WriteRune(r) + } + return b.String() +} + // Get returns the full detail for a single content item by ID. func (c *Client) Get(ctx context.Context, contentID int) (*Content, error) { var out Content diff --git a/internal/marketplace/search_test.go b/internal/marketplace/search_test.go new file mode 100644 index 000000000..16d9e4e5a --- /dev/null +++ b/internal/marketplace/search_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 + +package marketplace + +import "testing" + +func content(name, publisher string) Content { + return Content{Publisher: publisher, LatestVersion: &Version{Name: name}} +} + +// TestFilterItemsMatchesTheWrittenName is the regression test for a real search +// miss: the module everyone calls "Database Replication" is packaged as +// "DatabaseReplication", and the API exposes only the packaged name. Searching +// the name as written returned nothing, and the user had to guess that the space +// was the problem. +func TestFilterItemsMatchesTheWrittenName(t *testing.T) { + items := []Content{ + content("DatabaseReplication", "Mendix"), + content("MxModelReflection", "Mendix"), + content("Excel Importer", "Community"), + } + cases := []struct{ query, want string }{ + {"Database Replication", "DatabaseReplication"}, + {"database replication", "DatabaseReplication"}, + {"databasereplication", "DatabaseReplication"}, + {"replication", "DatabaseReplication"}, + {"database-replication", "DatabaseReplication"}, + {"excelimporter", "Excel Importer"}, + {"Excel Importer", "Excel Importer"}, + } + for _, c := range cases { + got := filterItems(items, c.query) + if len(got) != 1 { + t.Errorf("%q matched %d items, want 1", c.query, len(got)) + continue + } + if got[0].LatestVersion.Name != c.want { + t.Errorf("%q matched %q, want %q", c.query, got[0].LatestVersion.Name, c.want) + } + } +} + +// TestFilterItemsStillDiscriminates — normalising separators must not make the +// search match everything. +func TestFilterItemsStillDiscriminates(t *testing.T) { + items := []Content{content("DatabaseReplication", "Mendix"), content("Excel Importer", "Community")} + if got := filterItems(items, "workflow"); len(got) != 0 { + t.Errorf("unrelated query matched %d items", len(got)) + } + if got := filterItems(items, "mendix"); len(got) != 1 { + t.Errorf("publisher query matched %d items, want 1", len(got)) + } +} diff --git a/mdl-examples/bug-tests/295-showpage-null-variable.mdl b/mdl-examples/bug-tests/295-showpage-null-variable.mdl index bae761606..0fd2eb67f 100644 --- a/mdl-examples/bug-tests/295-showpage-null-variable.mdl +++ b/mdl-examples/bug-tests/295-showpage-null-variable.mdl @@ -20,7 +20,9 @@ create persistent entity bug295.Product ( -- Target page that accepts an entity parameter. CREATE PAGE bug295.Product_Detail ( Title: 'Product Detail', - Url: 'Product_Detail', + -- The URL needs a segment per page parameter or MxBuild reports CE5601 + -- (flagged by MDL-PAGE20); this page takes $Product. + Url: 'Product_Detail/{Product}', layout: Atlas_Core.Atlas_Default, Params: { $Product: bug295.Product } ) { diff --git a/mdl-examples/bug-tests/312-validate-skip-excluded-microflows.mdl b/mdl-examples/bug-tests/312-validate-skip-excluded-microflows.mdl index c15bacbd8..2632d90ae 100644 --- a/mdl-examples/bug-tests/312-validate-skip-excluded-microflows.mdl +++ b/mdl-examples/bug-tests/312-validate-skip-excluded-microflows.mdl @@ -44,7 +44,6 @@ end; create microflow BugTest312.MF_ExcludedWithBrokenCall () returns string as $msg begin - declare $msg string = empty; $msg = call microflow BugTest312.NoSuchTarget(); return $msg; end; @@ -56,7 +55,6 @@ end; create microflow BugTest312.MF_IncludedValid () returns string as $msg begin - declare $msg string = empty; $msg = call microflow BugTest312.MF_RealTarget(); return $msg; end; diff --git a/mdl-examples/bug-tests/893-check-gaps-ce0038-ce0068-ce0111.fail.mdl b/mdl-examples/bug-tests/893-check-gaps-ce0038-ce0068-ce0111.fail.mdl new file mode 100644 index 000000000..c2d1cdb03 --- /dev/null +++ b/mdl-examples/bug-tests/893-check-gaps-ce0038-ce0068-ce0111.fail.mdl @@ -0,0 +1,66 @@ +-- ============================================================================ +-- Bug #893 (items 1, 2 and 6): constructs that passed check AND exec, and were +-- then rejected by the build +-- ============================================================================ +-- +-- Symptom (before fix): this file reported `✓ Syntax OK`, `mxcli exec` wrote +-- all of it, and the defect surfaced only when a human opened Studio Pro's +-- Errors pane. Reproduced on mxbuild 11.6.6 against an app whose baseline is +-- 1 error: executing this script took it to 4. +-- +-- CE0038 "The 'Value' property is required." at ZZ_ProbeA +-- CE0068 "End events cannot be placed inside a loop." at ZZ_ProbeB +-- CE0111 "Duplicate variable name 'Session'." at ZZ_ProbeD +-- +-- After fix: MDL061, MDL062 and MDL063 report all three, and because they are +-- error severity `exec` refuses the script with nothing written. +-- +-- Usage (this file is expected to FAIL — .fail.mdl): +-- mxcli check mdl-examples/bug-tests/893-check-gaps-ce0038-ce0068-ce0111.fail.mdl +-- Expect exactly three errors: MDL061, MDL062, MDL063. +-- +-- The passing counterpart is 893-check-gaps-fixed.mdl, whose three fixes were +-- each measured to take the same app back to its 1-error baseline. +-- ============================================================================ + +create module BugTest893; + +create entity BugTest893.Part ( + Name : string +); +/ + +create microflow BugTest893.Helper () +returns string +begin + return 'hello'; +end; +/ + +-- Item 1: a Create Variable activity requires a value (CE0038). +create microflow BugTest893.ZZ_ProbeA () +begin + declare $X string; + log info node 'x' 'y'; +end; +/ + +-- Item 2: a return builds an End event, which cannot sit inside a loop (CE0068). +create microflow BugTest893.ZZ_ProbeB () +begin + retrieve $PartList from BugTest893.Part; + loop $Part in $PartList + begin + return; + end loop; +end; +/ + +-- Item 6: the call creates its OWN output variable, so the declare collides +-- with it (CE0111). A microflow's variable names are unique flow-wide. +create microflow BugTest893.ZZ_ProbeD () +begin + declare $Session string = ''; + $Session = call microflow BugTest893.Helper(); +end; +/ diff --git a/mdl-examples/bug-tests/893-check-gaps-fixed.mdl b/mdl-examples/bug-tests/893-check-gaps-fixed.mdl new file mode 100644 index 000000000..814c17de1 --- /dev/null +++ b/mdl-examples/bug-tests/893-check-gaps-fixed.mdl @@ -0,0 +1,60 @@ +-- ============================================================================ +-- Bug #893 (items 1, 2 and 6): the passing counterpart +-- ============================================================================ +-- +-- The three fixes MDL061/MDL062/MDL063 suggest, written out. Each was measured +-- on mxbuild 11.6.6 to take the app back to its 1-error baseline, so this file +-- is the evidence that the rules point at something that actually works rather +-- than merely refusing the original. +-- +-- Usage: +-- mxcli check mdl-examples/bug-tests/893-check-gaps-fixed.mdl # passes +-- mxcli exec mdl-examples/bug-tests/893-check-gaps-fixed.mdl -p app.mpr +-- mx check app.mpr # no new errors +-- +-- The failing counterpart is 893-check-gaps-ce0038-ce0068-ce0111.fail.mdl. +-- ============================================================================ + +create module BugTest893Fixed; + +create entity BugTest893Fixed.Part ( + Name : string +); +/ + +create microflow BugTest893Fixed.Helper () +returns string +begin + return 'hello'; +end; +/ + +-- Item 1 fixed: give the variable a starting value. Mendix has no +-- uninitialized variable — every one is created with a value. +create microflow BugTest893Fixed.ZZ_ProbeA () +begin + declare $X string = ''; + log info node 'x' 'y'; +end; +/ + +-- Item 2 fixed: `break` leaves the loop without placing an End event in it. +-- To return a value, assign it inside the loop and return once after it. +create microflow BugTest893Fixed.ZZ_ProbeB () +begin + retrieve $PartList from BugTest893Fixed.Part; + loop $Part in $PartList + begin + break; + end loop; +end; +/ + +-- Item 6 fixed: drop the declare. The call activity creates '$Session' itself; +-- there is no form in which it writes into an existing variable. +create microflow BugTest893Fixed.ZZ_ProbeD () +begin + $Session = call microflow BugTest893Fixed.Helper(); + log info node 'x' $Session; +end; +/ diff --git a/mdl-examples/bug-tests/915-mapping-describe-roundtrip.mdl b/mdl-examples/bug-tests/915-mapping-describe-roundtrip.mdl new file mode 100644 index 000000000..b7de03ebf --- /dev/null +++ b/mdl-examples/bug-tests/915-mapping-describe-roundtrip.mdl @@ -0,0 +1,80 @@ +-- ============================================================================ +-- #915 — DESCRIBE of a mapping reproduces the script that produced it +-- ============================================================================ +-- +-- POSITIVE TEST: `mxcli check` MUST accept this file. It also builds at 0 +-- errors on mxbuild 11.13.0, and is a DESCRIBE fixed point: exec it, describe +-- both mappings, and the output is this file's mapping bodies verbatim. +-- +-- DESCRIBE used to print Mendix's derived ExposedName instead of the JSON key, +-- so its own output no longer matched the input: +-- +-- Total = total -> Total = Total +-- CamelCase = camelCase -> CamelCase = CamelCase +-- ... = item -> ... = ItemItem (array item object) +-- LineId = id -> LineId = _id (Id is reserved) +-- +-- Two things are NOT bugs here, and were left alone: +-- +-- * The capitalisation itself. Mendix derives ExposedName by capitalising the +-- initial, and Studio Pro stores it that way too — a blank app's own +-- FeedbackModule JSON structure holds ExposedName "Uuid" against JsonPath +-- "(Object)|uuid". Rewriting ExposedName would diverge from Studio Pro. +-- * The stored mapping. JsonPath preserved the original keys all along, and +-- re-executing the old DESCRIBE output rebuilt identical paths (the +-- jsonSchemaIndex accepts either spelling since #882). The defect was that +-- the TEXT differed, which made every script-vs-describe diff noise. +-- +-- The second half of the fix is the header: DESCRIBE emitted a bare +-- `create import mapping`, so re-running its output against the project it came +-- from failed with "import mapping already exists". It now emits +-- `create or modify`, which is what makes the round trip actually runnable. +-- +-- Export mappings had both defects identically; the issue only reported import. + +create or modify json structure Bug915.JSON_Payload snippet $${ + "total": 3, + "camelCase": "x", + "item": [ + { + "id": 1, + "label": "a" + } + ] +}$$; + +create or modify persistent entity Bug915.Payload ( + "Total": Integer, + "CamelCase": String(50) +); + +create or modify persistent entity Bug915.Line ( + "LineId": Integer, + "Label": String(50) +); + +create or modify association Bug915.Line_Payload from Bug915.Line to Bug915.Payload; + +-- Every member below is a lowercase-initial JSON key, and one is an array whose +-- item object Mendix names "ItemItem". DESCRIBE must give these names back. +create or modify import mapping Bug915.IMM_Payload + with json structure Bug915.JSON_Payload +{ + create Bug915.Payload { + Total = total, + CamelCase = camelCase, + create Bug915.Line_Payload/Bug915.Line = item { + LineId = id, + Label = label + } + } +}; + +create or modify export mapping Bug915.EMM_Payload + with json structure Bug915.JSON_Payload +{ + Bug915.Payload { + total = Total, + camelCase = CamelCase + } +}; diff --git a/mdl-examples/bug-tests/917-calculated-attribute-binding.mdl b/mdl-examples/bug-tests/917-calculated-attribute-binding.mdl new file mode 100644 index 000000000..962a447a5 --- /dev/null +++ b/mdl-examples/bug-tests/917-calculated-attribute-binding.mdl @@ -0,0 +1,67 @@ +-- ============================================================================ +-- #917 — `calculated by` reaches the model, and the signature is checked +-- ============================================================================ +-- +-- POSITIVE TEST: `mxcli check` MUST accept this file, and every statement below +-- is verified to build at 0 errors on mxbuild 11.13.0. +-- +-- The binding used to be discarded on the DEFAULT engine: attributeToGen had no +-- CalculatedValue arm, so the attribute fell through to the default StoredValue. +-- `mxcli check` passed, exec reported "Added attribute", the microflow name +-- appeared nowhere in the domain model unit, and `mx check` reported 0 errors — +-- the attribute simply stayed empty at runtime. The legacy engine wrote it +-- correctly all along, which is why the feature looked implemented. +-- +-- Measured per shape (one project each, `mxcli docker check` read per shape): +-- +-- microflow takes the owning entity -> PassEntity=true, 0 errors +-- microflow takes no parameter -> PassEntity=false, 0 errors +-- microflow takes a DIFFERENT entity -> CE7247 "Microflow parameter 'Other' +-- should be of type MyFirstModule.Order." +-- microflow returns String, attr Int -> CE7247 "Microflow return type should +-- be Integer/Long." +-- microflow returns Long, attr Int -> 0 errors (one family) +-- +-- The last row is why the return-type check is not a strict equality: Mendix's +-- own message says "Integer/Long", and a strict rule refused valid MDL. +-- +-- The two CE7247 shapes are now refused by exec before anything is written, so +-- they cannot appear in this file; the refusals are pinned in +-- mdl/executor/calculated_attributes_test.go instead. + +CREATE OR MODIFY PERSISTENT ENTITY Bug917.Order ( + "Reference": String(50) +); + +-- Takes the owning entity: stored with PassEntity = true. +CREATE OR MODIFY MICROFLOW Bug917.CalcWithEntity ($Order: Bug917.Order) +RETURNS Integer +BEGIN + return 42; +END; + +-- Takes nothing: stored with PassEntity = false. Both are valid Mendix. +CREATE OR MODIFY MICROFLOW Bug917.CalcNoParam () +RETURNS Integer +BEGIN + return 7; +END; + +-- Returns Long for an Integer attribute — accepted, Integer and Long are one +-- family for this check. +CREATE OR MODIFY MICROFLOW Bug917.CalcLong ($Order: Bug917.Order) +RETURNS Long +BEGIN + return 42; +END; + +-- The ALTER path (the shape reported in the issue). +ALTER ENTITY Bug917.Order ADD ATTRIBUTE "TotalA": Integer CALCULATED BY Bug917.CalcWithEntity; +ALTER ENTITY Bug917.Order ADD ATTRIBUTE "TotalB": Integer CALCULATED BY Bug917.CalcNoParam; +ALTER ENTITY Bug917.Order ADD ATTRIBUTE "TotalC": Integer CALCULATED BY Bug917.CalcLong; + +-- The CREATE path: the same binding declared inline on a new entity. +CREATE OR MODIFY PERSISTENT ENTITY Bug917.Invoice ( + "Number": String(20), + "Amount": Integer CALCULATED BY Bug917.CalcNoParam +); diff --git a/mdl/backend/modelsdk/calculated_attribute_test.go b/mdl/backend/modelsdk/calculated_attribute_test.go new file mode 100644 index 000000000..846e580a6 --- /dev/null +++ b/mdl/backend/modelsdk/calculated_attribute_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// TestAttributeToGen_CalculatedValue is the #917 write half. Without the +// CalculatedValue arm the attribute fell through to the default StoredValue, so +// `calculated by` reported success and the binding never reached the model — +// an attribute that stays empty at runtime in a project that builds at 0 errors. +func TestAttributeToGen_CalculatedValue(t *testing.T) { + attr := &domainmodel.Attribute{ + Name: "Total", + Type: &domainmodel.IntegerAttributeType{}, + Value: &domainmodel.AttributeValue{ + Type: "CalculatedValue", + MicroflowName: "MyFirstModule.CalcTotal", + PassEntity: true, + }, + } + + out := attributeToGen(attr, false) + cv, ok := out.Value().(*genDm.CalculatedValue) + if !ok { + t.Fatalf("value is %T, want *genDm.CalculatedValue — the binding was discarded", out.Value()) + } + if got := cv.MicroflowQualifiedName(); got != "MyFirstModule.CalcTotal" { + t.Errorf("microflow = %q, want %q", got, "MyFirstModule.CalcTotal") + } + if !cv.PassEntity() { + t.Error("PassEntity = false, want true for a microflow that takes the owning entity") + } +} + +// TestAttributeToGen_CalculatedValue_NoPassEntity pins the parameterless shape. +// Both are accepted by mxbuild (measured on 11.13.0, 0 errors), so PassEntity +// must follow the signature rather than being hardcoded. +func TestAttributeToGen_CalculatedValue_NoPassEntity(t *testing.T) { + attr := &domainmodel.Attribute{ + Name: "Total", + Type: &domainmodel.IntegerAttributeType{}, + Value: &domainmodel.AttributeValue{ + Type: "CalculatedValue", + MicroflowName: "MyFirstModule.CalcNoParam", + }, + } + + cv, ok := attributeToGen(attr, false).Value().(*genDm.CalculatedValue) + if !ok { + t.Fatal("value is not a CalculatedValue") + } + if cv.PassEntity() { + t.Error("PassEntity = true for a parameterless microflow") + } +} + +// TestAttributeFromGen_CalculatedValue is the #917 read half: reading the +// binding back is what makes a read-modify-write safe. Without it an unrelated +// ALTER on the same entity silently converts the attribute to a stored one, +// destroying a binding the user made in Studio Pro. +func TestAttributeFromGen_CalculatedValue(t *testing.T) { + g := genDm.NewAttribute() + g.SetName("Total") + cv := genDm.NewCalculatedValue() + cv.SetMicroflowQualifiedName("MyFirstModule.CalcTotal") + cv.SetPassEntity(true) + g.SetValue(cv) + + attr := attributeFromGen(g) + if attr.Value == nil { + t.Fatal("attribute value is nil — the binding did not survive the read") + } + if attr.Value.Type != "CalculatedValue" { + t.Errorf("value type = %q, want CalculatedValue", attr.Value.Type) + } + if attr.Value.MicroflowName != "MyFirstModule.CalcTotal" { + t.Errorf("microflow = %q, want MyFirstModule.CalcTotal", attr.Value.MicroflowName) + } + if !attr.Value.PassEntity { + t.Error("PassEntity did not survive the read") + } +} diff --git a/mdl/backend/modelsdk/domainmodel.go b/mdl/backend/modelsdk/domainmodel.go index 142597962..ed335c9b1 100644 --- a/mdl/backend/modelsdk/domainmodel.go +++ b/mdl/backend/modelsdk/domainmodel.go @@ -263,6 +263,16 @@ func attributeFromGen(a *genDm.Attribute) *domainmodel.Attribute { switch v := a.Value().(type) { case *genDm.StoredValue: attr.Value = &domainmodel.AttributeValue{DefaultValue: v.DefaultValue()} + case *genDm.CalculatedValue: + // Reading the binding back is what makes a read-modify-write safe: without + // it every calculated attribute comes back as a plain value and the + // writer's CalculatedValue arm never fires, so an unrelated ALTER on the + // same entity silently converts the attribute to a stored one (#917). + attr.Value = &domainmodel.AttributeValue{ + Type: "CalculatedValue", + MicroflowName: v.MicroflowQualifiedName(), + PassEntity: v.PassEntity(), + } case *genDm.OqlViewValue: // View-entity attribute: the OQL column reference must survive a // read-modify-write (e.g. MOVE ENTITY) or the view goes out of sync (CE6770). diff --git a/mdl/backend/modelsdk/domainmodel_write.go b/mdl/backend/modelsdk/domainmodel_write.go index f8076cc91..83fcf656f 100644 --- a/mdl/backend/modelsdk/domainmodel_write.go +++ b/mdl/backend/modelsdk/domainmodel_write.go @@ -600,6 +600,21 @@ func attributeToGen(a *domainmodel.Attribute, isExternal bool) *genDm.Attribute vv := genDm.NewOqlViewValue() vv.SetReference(a.Value.ViewReference) out.SetValue(vv) + case a.Value != nil && a.Value.Type == "CalculatedValue": + // Calculated attribute: the value is computed by a microflow, not stored. + // Without this arm the attribute fell through to the default StoredValue + // below, so `calculated by` parsed, validated and reported success while + // the binding never reached the model — an attribute that stays empty at + // runtime, in a project that builds at 0 errors (#917). The legacy + // serializer had the arm; the default engine did not. + // + // Microflow is a ByNameReference (gen binds the "Microflow" key to the + // qualified name), and PassEntity says whether the microflow takes the + // owning entity as its parameter. + cv := genDm.NewCalculatedValue() + cv.SetMicroflowQualifiedName(a.Value.MicroflowName) + cv.SetPassEntity(a.Value.PassEntity) + out.SetValue(cv) case isExternal && a.IsPrimitiveCollection: // The single attribute of a primitive-collection NPE (e.g. TripTag.Tag) is // backed by a Rest$ODataMappedPrimitiveCollectionValue (issue #718). diff --git a/mdl/backend/pagemutator/column_property_test.go b/mdl/backend/pagemutator/column_property_test.go new file mode 100644 index 000000000..be03ba923 --- /dev/null +++ b/mdl/backend/pagemutator/column_property_test.go @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: Apache-2.0 + +package pagemutator + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/bsonutil" +) + +// Real TypePointers are BSON binary UUIDs, not strings — the setter reads them +// through ExtractBinaryIDFromDoc, so the fixture has to use the same encoding or +// it tests nothing. +var ( + idClass = "11111111-1111-1111-1111-111111111111" + idSortable = "22222222-2222-2222-2222-222222222222" + idAttr = "33333333-3333-3333-3333-333333333333" +) + +// columnFixture builds a column document with three properties whose schema +// kinds differ: an Expression, a primitive, and a TextTemplate. +func columnFixture() (bson.D, map[string]string, map[string]string) { + value := func() bson.D { + // A WidgetValue always carries every field at once — that is why the + // value's shape cannot be inferred from which keys are present. + return bson.D{ + {Key: "Expression", Value: ""}, + {Key: "PrimitiveValue", Value: ""}, + {Key: "TextTemplate", Value: nil}, + {Key: "AttributeRef", Value: nil}, + } + } + prop := func(id string) bson.D { + return bson.D{ + {Key: "TypePointer", Value: bsonutil.IDToBsonBinary(id)}, + {Key: "Value", Value: value()}, + } + } + col := bson.D{{Key: "Properties", Value: bson.A{ + prop(idClass), prop(idSortable), prop(idAttr), + }}} + keys := map[string]string{ + idClass: "columnClass", idSortable: "sortable", idAttr: "attribute", + } + kinds := map[string]string{ + idClass: "Expression", idSortable: "Boolean", idAttr: "Attribute", + } + return col, keys, kinds +} + +func fieldOf(t *testing.T, col bson.D, typePointer, field string) any { + t.Helper() + for _, p := range col { + if p.Key != "Properties" { + continue + } + for _, item := range p.Value.(bson.A) { + doc := item.(bson.D) + var tp string + var val bson.D + for _, e := range doc { + switch e.Key { + case "TypePointer": + tp = bsonnav.ExtractBinaryIDFromDoc(e.Value) + case "Value": + val, _ = e.Value.(bson.D) + } + } + if tp != typePointer { + continue + } + for _, e := range val { + if e.Key == field { + return e.Value + } + } + } + } + return nil +} + +// TestSetColumnPropertyResolvesTheCreatePathAlias is the regression test for +// mendixlabs/mxcli#919. The create path aliased DynamicCellClass → columnClass +// while the ALTER path kept its own hand-written table that did not, so a +// property mxcli could write it could not then edit. +func TestSetColumnPropertyResolvesTheCreatePathAlias(t *testing.T) { + col, keys, kinds := columnFixture() + if err := setColumnPropertyMut(col, keys, kinds, "DynamicCellClass", "my-class"); err != nil { + t.Fatalf("DynamicCellClass rejected: %v", err) + } + if got := fieldOf(t, col, idClass, "Expression"); got != "my-class" { + t.Errorf("Expression = %v, want my-class", got) + } +} + +// TestSetColumnPropertyIsCaseInsensitiveOnSchemaKeys — a property whose MDL name +// differs from the schema key only by case needs no alias entry, which is what +// keeps the shared table down to genuine renames. +func TestSetColumnPropertyIsCaseInsensitiveOnSchemaKeys(t *testing.T) { + for _, name := range []string{"Sortable", "sortable", "SORTABLE"} { + col, keys, kinds := columnFixture() + if err := setColumnPropertyMut(col, keys, kinds, name, "true"); err != nil { + t.Errorf("%s rejected: %v", name, err) + continue + } + if got := fieldOf(t, col, idSortable, "PrimitiveValue"); got != "true" { + t.Errorf("%s: PrimitiveValue = %v, want true", name, got) + } + } +} + +// TestSetColumnPropertyWritesTheFieldTheSchemaDeclares is the second defect: the +// setter always wrote PrimitiveValue, so an Expression-valued property got its +// value in a field Studio Pro does not read. It reported success, did not survive +// a DESCRIBE round trip, and left mx check at 0 errors. +func TestSetColumnPropertyWritesTheFieldTheSchemaDeclares(t *testing.T) { + col, keys, kinds := columnFixture() + if err := setColumnPropertyMut(col, keys, kinds, "columnClass", "cls"); err != nil { + t.Fatalf("columnClass rejected: %v", err) + } + if got := fieldOf(t, col, idClass, "Expression"); got != "cls" { + t.Errorf("Expression = %v, want cls", got) + } + if got := fieldOf(t, col, idClass, "PrimitiveValue"); got != "" { + t.Errorf("PrimitiveValue = %v, want it left empty — the value belongs in Expression", got) + } +} + +// TestSetColumnPropertyRefusesAStructuredValue. An attribute-valued property +// needs a structured AttributeRef, not a string. Writing a plausible-looking +// string into it is the same silent corruption in a different field. +func TestSetColumnPropertyRefusesAStructuredValue(t *testing.T) { + col, keys, kinds := columnFixture() + err := setColumnPropertyMut(col, keys, kinds, "attribute", "Mod.Ent.Name") + if err == nil { + t.Fatal("an Attribute-valued property was set from a plain string") + } + if !strings.Contains(err.Error(), "CREATE OR REPLACE PAGE") { + t.Errorf("error = %q, want it to point at the supported path", err) + } +} + +// TestSetColumnPropertyErrorListsWhatIsSettable — the old message named only the +// property that failed, which is no help when the name is nearly right. +func TestSetColumnPropertyErrorListsWhatIsSettable(t *testing.T) { + col, keys, kinds := columnFixture() + err := setColumnPropertyMut(col, keys, kinds, "NoSuchProperty", "x") + if err == nil { + t.Fatal("an unknown property was accepted") + } + for _, want := range []string{"columnClass", "sortable", "attribute"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not list %q", err, want) + } + } +} diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index b5591383d..94f16a2af 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -5,6 +5,7 @@ package pagemutator import ( "fmt" "math" + "sort" "strings" "go.mongodb.org/mongo-driver/bson" @@ -108,7 +109,7 @@ func (m *Mutator) SetWidgetProperty(widgetRef string, prop string, value any) er if n := m.columnMatchCount(widgetRef); n > 1 { return columnAmbiguityError(widgetRef, n) } - return setColumnPropertyMut(result.widget, result.colPropKeys, prop, value) + return setColumnPropertyMut(result.widget, result.colPropKeys, result.colPropKinds, prop, value) } return setRawWidgetPropertyMut(result.widget, prop, value) } @@ -183,7 +184,7 @@ func (m *Mutator) SetColumnProperty(gridRef string, columnRef string, prop strin if err != nil { return err } - return setColumnPropertyMut(result.widget, result.colPropKeys, prop, value) + return setColumnPropertyMut(result.widget, result.colPropKeys, result.colPropKinds, prop, value) } func (m *Mutator) SetDesignProperty(widgetRef, key, valueType, option string) error { @@ -1016,6 +1017,10 @@ type bsonWidgetResult struct { parentDoc bson.D index int colPropKeys map[string]string + // colPropKinds maps the same TypePointer ids to the value kind the schema + // declares (Expression, TextTemplate, Boolean, …). Without it a setter + // cannot tell which field of a WidgetValue to write — see columnValueField. + colPropKinds map[string]string } // widgetFinder is a function type for locating widgets in a raw BSON tree. @@ -1158,7 +1163,7 @@ func findInWidgetChildren(wDoc bson.D, widgetName string) *bsonWidgetResult { if valDoc == nil { break } - colPropKeyMap := buildColumnPropKeyMap(wDoc, typePointerID) + colPropKeyMap, colPropKindMap := buildColumnPropKeyMap(wDoc, typePointerID) columns := bsonnav.DGetArrayElements(bsonnav.DGet(valDoc, "Objects")) for i, colItem := range columns { colDoc, ok := colItem.(bson.D) @@ -1167,12 +1172,13 @@ func findInWidgetChildren(wDoc bson.D, widgetName string) *bsonWidgetResult { } if deriveColumnNameBson(colDoc, colPropKeyMap, i) == widgetName { return &bsonWidgetResult{ - widget: colDoc, - parentArr: columns, - parentKey: "Objects", - parentDoc: valDoc, - index: i, - colPropKeys: colPropKeyMap, + widget: colDoc, + parentArr: columns, + parentKey: "Objects", + parentDoc: valDoc, + index: i, + colPropKeys: colPropKeyMap, + colPropKinds: colPropKindMap, } } // Descend into the column's OWN content widgets. A column @@ -1256,7 +1262,7 @@ func findBsonColumn(rawData bson.D, gridName, columnName string, find widgetFind return nil, fmt.Errorf("column %q on grid %q not found", columnName, gridName) } - colPropKeyMap := buildColumnPropKeyMap(gridResult.widget, typePointerID) + colPropKeyMap, colPropKindMap := buildColumnPropKeyMap(gridResult.widget, typePointerID) columns := bsonnav.DGetArrayElements(bsonnav.DGet(valDoc, "Objects")) var matches []*bsonWidgetResult @@ -1270,12 +1276,13 @@ func findBsonColumn(rawData bson.D, gridName, columnName string, find widgetFind available = append(available, derived) if derived == columnName { matches = append(matches, &bsonWidgetResult{ - widget: colDoc, - parentArr: columns, - parentKey: "Objects", - parentDoc: valDoc, - index: i, - colPropKeys: colPropKeyMap, + widget: colDoc, + parentArr: columns, + parentKey: "Objects", + parentDoc: valDoc, + index: i, + colPropKeys: colPropKeyMap, + colPropKinds: colPropKindMap, }) } } @@ -1367,7 +1374,7 @@ func gridColumnNames(wDoc bson.D) []string { if valDoc == nil { return nil } - colPropKeyMap := buildColumnPropKeyMap(wDoc, typePointerID) + colPropKeyMap, _ := buildColumnPropKeyMap(wDoc, typePointerID) var names []string for i, colItem := range bsonnav.DGetArrayElements(bsonnav.DGet(valDoc, "Objects")) { if colDoc, ok := colItem.(bson.D); ok { @@ -1446,15 +1453,16 @@ func buildPropKeyMap(widgetDoc bson.D) map[string]string { } // buildColumnPropKeyMap builds a TypePointer ID -> PropertyKey map for column properties. -func buildColumnPropKeyMap(widgetDoc bson.D, columnsTypePointerID string) map[string]string { +func buildColumnPropKeyMap(widgetDoc bson.D, columnsTypePointerID string) (map[string]string, map[string]string) { m := make(map[string]string) + kinds := make(map[string]string) widgetType := bsonnav.DGetDoc(widgetDoc, "Type") if widgetType == nil { - return m + return m, kinds } objType := bsonnav.DGetDoc(widgetType, "ObjectType") if objType == nil { - return m + return m, kinds } for _, pt := range bsonnav.DGetArrayElements(bsonnav.DGet(objType, "PropertyTypes")) { ptDoc, ok := pt.(bson.D) @@ -1467,11 +1475,11 @@ func buildColumnPropKeyMap(widgetDoc bson.D, columnsTypePointerID string) map[st } valType := bsonnav.DGetDoc(ptDoc, "ValueType") if valType == nil { - return m + return m, kinds } colObjType := bsonnav.DGetDoc(valType, "ObjectType") if colObjType == nil { - return m + return m, kinds } for _, cpt := range bsonnav.DGetArrayElements(bsonnav.DGet(colObjType, "PropertyTypes")) { cptDoc, ok := cpt.(bson.D) @@ -1482,11 +1490,14 @@ func buildColumnPropKeyMap(widgetDoc bson.D, columnsTypePointerID string) map[st cid := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(cptDoc, "$ID")) if key != "" && cid != "" { m[cid] = key + if cvt := bsonnav.DGetDoc(cptDoc, "ValueType"); cvt != nil { + kinds[cid] = bsonnav.DGetString(cvt, "Type") + } } } - return m + return m, kinds } - return m + return m, kinds } // deriveColumnNameBson derives a column name from its BSON WidgetObject. @@ -1966,7 +1977,7 @@ func collectWidgetScopeInChildren(wDoc bson.D, scope map[string]model.ID) { if valDoc == nil { break } - colPropKeyMap := buildColumnPropKeyMap(wDoc, typePointerID) + colPropKeyMap, _ := buildColumnPropKeyMap(wDoc, typePointerID) columns := bsonnav.DGetArrayElements(bsonnav.DGet(valDoc, "Objects")) for i, colItem := range columns { colDoc, ok := colItem.(bson.D) @@ -1991,41 +2002,93 @@ func collectWidgetScopeInChildren(wDoc bson.D, scope map[string]model.ID) { // Property setting helpers // --------------------------------------------------------------------------- -// columnPropertyAliases maps user-facing property names to internal column property keys. -// MDL lookup is case-insensitive (see columnPropertyAliasesCI below); the values -// here are the BSON-internal PropertyKeys defined by the DataGrid2 widget schema -// and must stay case-sensitive. -var columnPropertyAliases = map[string]string{ - "Caption": "header", - "Attribute": "attribute", - "Visible": "visible", - "Alignment": "alignment", - "WrapText": "wrapText", - "Sortable": "sortable", - "Resizable": "resizable", - "Draggable": "draggable", - "Hidable": "hidable", - "ColumnWidth": "width", - "Size": "size", - "ShowContentAs": "showContentAs", - "ColumnClass": "columnClass", - "Tooltip": "tooltip", -} - -// columnPropertyAliasesCI is a lowercase-keyed view of columnPropertyAliases -// used for case-insensitive MDL lookup (set caption = … vs set Caption = …). -var columnPropertyAliasesCI = func() map[string]string { - m := make(map[string]string, len(columnPropertyAliases)) - for k, v := range columnPropertyAliases { - m[strings.ToLower(k)] = v +// resolveColumnPropertyKey maps a user-facing MDL property name onto the schema +// key the column document actually declares. +// +// It resolves against the keys in propKeyMap — read from the widget's own Type +// document — rather than a hand-written list. That is the whole point: the list +// this replaced was a second copy of the create path's alias table, and the two +// drifted (mendixlabs/mxcli#919). Matching what the document declares means a +// property the create path can write is one ALTER can write, by construction. +// +// Two ways to match, in order: the schema key itself, case-insensitively +// (`Sortable` → `sortable`), then the genuine renames in types.ItemPropertyAliases +// (`DynamicCellClass` → `columnClass`). +func resolveColumnPropertyKey(propName string, propKeyMap map[string]string) string { + want := strings.ToLower(propName) + + declared := make(map[string]bool, len(propKeyMap)) + for _, key := range propKeyMap { + declared[key] = true + if strings.EqualFold(key, propName) { + return key + } } - return m -}() -func setColumnPropertyMut(colDoc bson.D, propKeyMap map[string]string, propName string, value any) error { - internalKey := columnPropertyAliasesCI[strings.ToLower(propName)] + for schemaKey, aliases := range types.ItemPropertyAliasesFor(types.DataGridWidgetID, types.DataGridColumnsKey) { + if !declared[schemaKey] { + continue + } + for _, alias := range aliases { + if strings.ToLower(alias) == want { + return schemaKey + } + } + } + return "" +} + +// settableColumnProperties lists what ALTER can set on this column, for an error +// message. Derived from the document, so it is accurate for the widget version +// actually installed rather than for the one mxcli was built against. +func settableColumnProperties(propKeyMap map[string]string) string { + seen := make(map[string]bool, len(propKeyMap)) + names := make([]string, 0, len(propKeyMap)) + for _, key := range propKeyMap { + if seen[key] { + continue + } + seen[key] = true + names = append(names, key) + } + sort.Strings(names) + return strings.Join(names, ", ") +} + +// columnValueField decides which field of a WidgetValue a property's value +// belongs in, from the value kind the widget schema declares for it. +// +// This has to come from the schema and cannot be inferred from the stored +// document: a WidgetValue carries *every* field at once — Expression, +// PrimitiveValue, TextTemplate, AttributeRef and the rest — with the unused ones +// empty. So "which key is present" says nothing, and the previous code, which +// always wrote PrimitiveValue, silently put expression values in the wrong field. +// `SET DynamicCellClass` and `SET Visible` both reported success, wrote a value +// Studio Pro does not read, did not survive a DESCRIBE round trip, and left +// `mx check` at 0 errors. +// +// The second return reports whether the kind is settable at all. Attribute, +// datasource, action and widget-valued properties need a structured value, not a +// string, so ALTER refuses them rather than writing a plausible-looking wrong one. +func columnValueField(kind string) (string, bool) { + switch kind { + case "Expression": + return "Expression", true + case "TextTemplate": + return "TextTemplate", true + case "Attribute", "Association", "DataSource", "Action", "Widgets", "Object", "Form", "Image", "Icon", "Microflow", "Nanoflow", "Selection": + return "", false + default: + // Boolean, String, Integer, Decimal, Enumeration, … — the primitives. + return "PrimitiveValue", true + } +} + +func setColumnPropertyMut(colDoc bson.D, propKeyMap map[string]string, propKindMap map[string]string, propName string, value any) error { + internalKey := resolveColumnPropertyKey(propName, propKeyMap) if internalKey == "" { - internalKey = propName + return fmt.Errorf("column property %q not found — settable column properties on this grid are: %s", + propName, settableColumnProperties(propKeyMap)) } props := bsonnav.DGetArrayElements(bsonnav.DGet(colDoc, "Properties")) @@ -2035,27 +2098,37 @@ func setColumnPropertyMut(colDoc bson.D, propKeyMap map[string]string, propName continue } typePointerID := bsonnav.ExtractBinaryIDFromDoc(bsonnav.DGet(propDoc, "TypePointer")) - propKey := propKeyMap[typePointerID] - if propKey != internalKey { + if propKeyMap[typePointerID] != internalKey { continue } valDoc := bsonnav.DGetDoc(propDoc, "Value") if valDoc == nil { return fmt.Errorf("column property %q has no Value", propName) } + + field, settable := columnValueField(propKindMap[typePointerID]) + if !settable { + return fmt.Errorf( + "column property %q holds a value of kind %s, which ALTER cannot set from a plain value — "+ + "rewrite the column with CREATE OR REPLACE PAGE instead", + propName, propKindMap[typePointerID]) + } + strVal := fmt.Sprintf("%v", value) - // TextTemplate-valued properties (header, tooltip) store the text inside - // a nested Forms$ClientTemplate → Texts$Text → Items[Translation].Text. - if textTemplate := bsonnav.DGetDoc(valDoc, "TextTemplate"); textTemplate != nil { - if updateClientTemplateText(textTemplate, strVal) { - return nil + if field == "TextTemplate" { + // The text lives inside Forms$ClientTemplate → Texts$Text → + // Items[Translation].Text, not in the field itself. + if textTemplate := bsonnav.DGetDoc(valDoc, "TextTemplate"); textTemplate != nil { + if updateClientTemplateText(textTemplate, strVal) { + return nil + } } + return fmt.Errorf("column property %q has no text template to update", propName) } - // Primitive-valued properties (sortable, visible, alignment, etc.) - bsonnav.DSet(valDoc, "PrimitiveValue", strVal) + bsonnav.DSet(valDoc, field, strVal) return nil } - return fmt.Errorf("column property %q not found", propName) + return fmt.Errorf("column property %q not found on this column", propName) } // updateClientTemplateText replaces the Template.Items[*].Text of a diff --git a/mdl/catalog/builder_microflows.go b/mdl/catalog/builder_microflows.go index 2e32ba929..d46395bdf 100644 --- a/mdl/catalog/builder_microflows.go +++ b/mdl/catalog/builder_microflows.go @@ -34,6 +34,16 @@ func (b *Builder) buildMicroflows() error { } defer mfStmt.Close() + paramStmt, err := b.tx.Prepare(` + INSERT INTO microflow_parameters_data (Id, MicroflowId, MicroflowQualifiedName, + ModuleName, Name, ParameterType, Description, Ordinal, ProjectId, SnapshotId) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `) + if err != nil { + return err + } + defer paramStmt.Close() + // Prepare activity statement only in full mode var actStmt *sql.Stmt if b.fullMode { @@ -53,6 +63,37 @@ func (b *Builder) buildMicroflows() error { mfCount := 0 nfCount := 0 + paramCount := 0 + + // insertParams writes one row per parameter. Microflows and nanoflows share + // it: both carry []*MicroflowParameter and both land in microflows_data, so + // splitting them here would only invite the two paths to drift. + insertParams := func(flowID, qualifiedName, moduleName string, params []*microflows.MicroflowParameter) error { + for i, prm := range params { + if prm == nil { + continue + } + id := string(prm.ID) + if id == "" { + id = flowID + "/" + prm.Name + } + if _, err := paramStmt.Exec( + id, + flowID, + qualifiedName, + moduleName, + prm.Name, + getDataTypeName(prm.Type), + prm.Documentation, + i, + projectID, snapshotID, + ); err != nil { + return err + } + paramCount++ + } + return nil + } actCount := 0 // Process microflows @@ -92,6 +133,9 @@ func (b *Builder) buildMicroflows() error { if err != nil { return err } + if err := insertParams(string(mf.ID), qualifiedName, moduleName, mf.Parameters); err != nil { + return err + } mfCount++ // Insert activities only in full mode @@ -181,6 +225,9 @@ func (b *Builder) buildMicroflows() error { if err != nil { return err } + if err := insertParams(string(nf.ID), qualifiedName, moduleName, nf.Parameters); err != nil { + return err + } nfCount++ // Insert activities only in full mode @@ -236,6 +283,7 @@ func (b *Builder) buildMicroflows() error { b.report("Microflows", mfCount) b.report("Nanoflows", nfCount) + b.report("Flow parameters", paramCount) if b.fullMode { b.report("Activities", actCount) } diff --git a/mdl/catalog/builder_modules.go b/mdl/catalog/builder_modules.go index 438d25c5c..6f5d88f7a 100644 --- a/mdl/catalog/builder_modules.go +++ b/mdl/catalog/builder_modules.go @@ -4,6 +4,7 @@ package catalog import ( "fmt" + "sort" "strings" "github.com/mendixlabs/mxcli/sdk/domainmodel" @@ -80,9 +81,9 @@ func (b *Builder) buildEntities() error { attrStmt, err := b.tx.Prepare(` INSERT INTO attributes_data (Id, Name, EntityId, EntityQualifiedName, ModuleName, - DataType, Length, IsUnique, IsRequired, DefaultValue, IsCalculated, Description, - ProjectId, SnapshotId) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + DataType, EnumerationQualifiedName, Length, IsUnique, IsRequired, DefaultValue, + IsCalculated, Description, ProjectId, SnapshotId) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `) if err != nil { return err @@ -175,6 +176,7 @@ func (b *Builder) buildEntities() error { // Insert attributes for _, attr := range entity.Attributes { dataType := "" + enumQN := "" length := 0 if attr.Type != nil { dataType = attr.Type.GetTypeName() @@ -182,6 +184,13 @@ func (b *Builder) buildEntities() error { if st, ok := attr.Type.(*domainmodel.StringAttributeType); ok { length = st.Length } + // GetTypeName is the bare kind, so an enumeration attribute + // reports only "Enumeration". Which enumeration is a separate + // column; without it nothing downstream can check a value + // against the cases. + if et, ok := attr.Type.(*domainmodel.EnumerationAttributeType); ok { + enumQN = et.EnumerationRef + } } // Check for unique/required constraints by ID first, then by name @@ -210,6 +219,7 @@ func (b *Builder) buildEntities() error { qualifiedName, moduleName, dataType, + enumQN, length, isUnique, isRequired, @@ -258,7 +268,18 @@ func (b *Builder) buildEnumerations() error { } defer stmt.Close() + valueStmt, err := b.tx.Prepare(` + INSERT INTO enumeration_values_data (Id, EnumerationId, EnumerationQualifiedName, + ModuleName, Name, Caption, Ordinal, ProjectId, SnapshotId) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + `) + if err != nil { + return err + } + defer valueStmt.Close() + projectID, snapshotID := b.snapshotMeta() + valueCount := 0 for _, enum := range enums { // Get module name using hierarchy @@ -286,9 +307,52 @@ func (b *Builder) buildEnumerations() error { if err != nil { return err } + + for i, v := range enum.Values { + // A value's Id is not always populated on read, and the pair + // (enumeration, name) is what identifies it anyway — a synthetic key + // keeps the row insertable either way. + id := string(v.ID) + if id == "" { + id = string(enum.ID) + "/" + v.Name + } + caption := "" + if v.Caption != nil { + // Any translation is better than none for a display caption, and + // nothing downstream keys off it — the checker matches on Name. + caption = v.Caption.GetTranslation("en_US") + if caption == "" { + // Fall back to some other language rather than storing + // nothing, but pick it deterministically: iterating the map + // directly would make the catalog row vary run to run. + langs := make([]string, 0, len(v.Caption.Translations)) + for lang := range v.Caption.Translations { + langs = append(langs, lang) + } + sort.Strings(langs) + if len(langs) > 0 { + caption = v.Caption.Translations[langs[0]] + } + } + } + if _, err := valueStmt.Exec( + id, + string(enum.ID), + qualifiedName, + moduleName, + v.Name, + caption, + i, + projectID, snapshotID, + ); err != nil { + return err + } + valueCount++ + } } b.report("Enumerations", len(enums)) + b.report("Enumeration values", valueCount) return nil } diff --git a/mdl/catalog/catalog.go b/mdl/catalog/catalog.go index 0538e79b7..4fa61763b 100644 --- a/mdl/catalog/catalog.go +++ b/mdl/catalog/catalog.go @@ -112,6 +112,8 @@ func (c *Catalog) Tables() []string { "CATALOG.BUILDING_BLOCKS", "CATALOG.LAYOUTS", "CATALOG.ENUMERATIONS", + "CATALOG.ENUMERATION_VALUES", + "CATALOG.MICROFLOW_PARAMETERS", "CATALOG.JAVA_ACTIONS", "CATALOG.JAVA_ACTION_PARAMETERS", "CATALOG.JAVASCRIPT_ACTIONS", diff --git a/mdl/catalog/tables.go b/mdl/catalog/tables.go index c7746bcbc..7fd917b83 100644 --- a/mdl/catalog/tables.go +++ b/mdl/catalog/tables.go @@ -7,6 +7,13 @@ package catalog // // History: // +// 10 — the three lookups expression type checking needs and the catalog could +// not answer: attributes_data.EnumerationQualifiedName (DataType says only +// "Enumeration", losing which one), enumeration_values_data (the table +// stored ValueCount but not the values), and microflow_parameters_data +// (likewise ParameterCount but not the parameters). Without the bump a +// cached catalog answers "unknown" for every one, which the checker reads +// as "cannot tell" and silently skips — a green run that checked nothing. // 9 — java_action_parameters_data + view: a Java action's parameters, each // with its own Description. Without the bump a cached catalog silently // reports zero parameters, so QUAL002 would under-report rather than @@ -16,7 +23,7 @@ package catalog // SnapshotSource / SourceId / SourceBranch / SourceRevision columns // from every row (issue #576). // 1 — initial flat schema with denormalized snapshot columns on every row. -const CatalogSchemaVersion = "9" +const CatalogSchemaVersion = "10" // MetaSchemaVersion is the catalog_meta key that records the schema version // the cache was built against. @@ -137,6 +144,11 @@ func (c *Catalog) createTables() error { EntityQualifiedName TEXT, ModuleName TEXT, DataType TEXT, + -- DataType is the bare kind ("Enumeration"), so the enum's identity + -- needs its own column. Kept separate rather than folded into + -- DataType as "Enumeration:QN" because existing queries and lint + -- rules match DataType by equality. + EnumerationQualifiedName TEXT, Length INTEGER, IsUnique INTEGER DEFAULT 0, IsRequired INTEGER DEFAULT 0, @@ -171,6 +183,26 @@ func (c *Catalog) createTables() error { `CREATE VIEW IF NOT EXISTS nanoflows AS SELECT * FROM microflows WHERE MicroflowType = 'NANOFLOW'`, + // microflow_parameters: one row per parameter, for microflows and + // nanoflows alike. microflows_data carries only ParameterCount, so a + // caller could see that a flow takes three arguments but not what they + // are — which is what typing a CALL's arguments requires. ParameterType + // uses the same encoding as microflows_data.ReturnType ("String", + // "Object:Mod.Entity", "Enumeration:Mod.Enum", …). + `CREATE TABLE IF NOT EXISTS microflow_parameters_data ( + Id TEXT PRIMARY KEY, + MicroflowId TEXT, + MicroflowQualifiedName TEXT, + ModuleName TEXT, + Name TEXT, + ParameterType TEXT, + Description TEXT, + Ordinal INTEGER DEFAULT 0, + ProjectId TEXT, + SnapshotId TEXT + )`, + viewWithFullSnapshot("microflow_parameters"), + // pages `CREATE TABLE IF NOT EXISTS pages_data ( Id TEXT PRIMARY KEY, @@ -250,6 +282,23 @@ func (c *Catalog) createTables() error { )`, viewWithFullSnapshot("enumerations"), + // enumeration_values: one row per value. enumerations_data carries only + // ValueCount, which cannot answer "is 'Open' a case of this enum" — + // the check behind the most common expression bug (comparing an enum + // attribute to a string literal). + `CREATE TABLE IF NOT EXISTS enumeration_values_data ( + Id TEXT PRIMARY KEY, + EnumerationId TEXT, + EnumerationQualifiedName TEXT, + ModuleName TEXT, + Name TEXT, + Caption TEXT, + Ordinal INTEGER DEFAULT 0, + ProjectId TEXT, + SnapshotId TEXT + )`, + viewWithFullSnapshot("enumeration_values"), + // java_actions `CREATE TABLE IF NOT EXISTS java_actions_data ( Id TEXT PRIMARY KEY, @@ -1180,6 +1229,8 @@ func (c *Catalog) createTables() error { `CREATE INDEX IF NOT EXISTS idx_refs_kind ON refs(RefKind)`, `CREATE INDEX IF NOT EXISTS idx_attributes_entity ON attributes_data(EntityId)`, `CREATE INDEX IF NOT EXISTS idx_attributes_entity_qname ON attributes_data(EntityQualifiedName)`, + `CREATE INDEX IF NOT EXISTS idx_enum_values_enum ON enumeration_values_data(EnumerationQualifiedName)`, + `CREATE INDEX IF NOT EXISTS idx_microflow_params_flow ON microflow_parameters_data(MicroflowQualifiedName)`, `CREATE INDEX IF NOT EXISTS idx_java_actions_name ON java_actions_data(Name)`, `CREATE INDEX IF NOT EXISTS idx_java_actions_module ON java_actions_data(ModuleName)`, `CREATE INDEX IF NOT EXISTS idx_odata_clients_name ON odata_clients_data(Name)`, diff --git a/mdl/catalog/typecheck_tables_test.go b/mdl/catalog/typecheck_tables_test.go new file mode 100644 index 000000000..089e735b5 --- /dev/null +++ b/mdl/catalog/typecheck_tables_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +package catalog + +import "testing" + +// The three lookups expression type checking needs, and which the catalog could +// not answer before schema 10. Each is tested at the schema level — that the +// column or table exists and holds what a reader will select — because the +// failure mode is not an error but an empty answer, which a checker reads as +// "cannot tell" and silently skips. + +func TestAttributesCarryTheEnumerationQualifiedName(t *testing.T) { + cat, err := New() + if err != nil { + t.Fatalf("New: %v", err) + } + defer cat.Close() + + if _, err := cat.CatalogDB().Exec( + `INSERT INTO attributes_data (Id, Name, EntityQualifiedName, DataType, EnumerationQualifiedName) + VALUES ('a1', 'Status', 'Shop.Order', 'Enumeration', 'Shop.OrderStatus')`, + ); err != nil { + t.Fatalf("insert: %v", err) + } + + var got string + if err := cat.CatalogDB().QueryRow( + `SELECT EnumerationQualifiedName FROM attributes WHERE Id = 'a1'`, + ).Scan(&got); err != nil { + t.Fatalf("select: %v", err) + } + if got != "Shop.OrderStatus" { + t.Errorf("got %q, want the enum's qualified name", got) + } +} + +func TestEnumerationValuesTableIsQueryable(t *testing.T) { + cat, err := New() + if err != nil { + t.Fatalf("New: %v", err) + } + defer cat.Close() + + for _, v := range []struct { + name string + ordinal int + }{{"Open", 0}, {"Closed", 1}} { + if _, err := cat.CatalogDB().Exec( + `INSERT INTO enumeration_values_data (Id, EnumerationQualifiedName, Name, Caption, Ordinal) + VALUES (?, 'Shop.OrderStatus', ?, ?, ?)`, + "Shop.OrderStatus/"+v.name, v.name, v.name, v.ordinal, + ); err != nil { + t.Fatalf("insert: %v", err) + } + } + + rows, err := cat.CatalogDB().Query( + `SELECT Name FROM enumeration_values WHERE EnumerationQualifiedName = 'Shop.OrderStatus' ORDER BY Ordinal`) + if err != nil { + t.Fatalf("select: %v", err) + } + defer rows.Close() + + var got []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + t.Fatalf("scan: %v", err) + } + got = append(got, n) + } + if len(got) != 2 || got[0] != "Open" || got[1] != "Closed" { + t.Errorf("got %v, want [Open Closed] in ordinal order", got) + } +} + +func TestMicroflowParametersTableIsQueryable(t *testing.T) { + cat, err := New() + if err != nil { + t.Fatalf("New: %v", err) + } + defer cat.Close() + + if _, err := cat.CatalogDB().Exec( + `INSERT INTO microflow_parameters_data (Id, MicroflowQualifiedName, Name, ParameterType, Ordinal) + VALUES ('p1', 'Shop.ACT_Place', 'Order', 'Object:Shop.Order', 0)`, + ); err != nil { + t.Fatalf("insert: %v", err) + } + + var name, ptype string + if err := cat.CatalogDB().QueryRow( + `SELECT Name, ParameterType FROM microflow_parameters WHERE MicroflowQualifiedName = 'Shop.ACT_Place'`, + ).Scan(&name, &ptype); err != nil { + t.Fatalf("select: %v", err) + } + if name != "Order" || ptype != "Object:Shop.Order" { + t.Errorf("got (%q, %q), want the parameter name and its encoded type", name, ptype) + } +} + +// TestNewTablesAreListed pins that the additions show up in SHOW CATALOG TABLES. +// A table nothing lists is a table nobody discovers. +func TestNewTablesAreListed(t *testing.T) { + cat, err := New() + if err != nil { + t.Fatalf("New: %v", err) + } + defer cat.Close() + + listed := map[string]bool{} + for _, name := range cat.Tables() { + listed[name] = true + } + for _, want := range []string{"CATALOG.ENUMERATION_VALUES", "CATALOG.MICROFLOW_PARAMETERS"} { + if !listed[want] { + t.Errorf("%s is not in Tables()", want) + } + } +} diff --git a/mdl/executor/calculated_attributes.go b/mdl/executor/calculated_attributes.go new file mode 100644 index 000000000..6437ec9fa --- /dev/null +++ b/mdl/executor/calculated_attributes.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// resolveCalculatedValue builds the DomainModels$CalculatedValue for an +// attribute declared `CALCULATED BY Module.Microflow`, and refuses the bindings +// Mendix rejects. +// +// Two things are decided here (#917): +// +// - PassEntity, which says whether the microflow receives the owning entity. +// It is derived from the signature rather than assumed: a microflow that +// takes the entity gets true, a parameterless one false. Guessing either way +// produces a binding mxbuild rejects. +// - Whether the binding is valid at all. Mendix checks the signature at build +// time — a parameter of the wrong entity is CE7247 "Microflow parameter 'X' +// should be of type Module.Entity" — so a mismatch is refused here rather +// than written and discovered at build. Same placement as the other +// write-blocking rules (the #833 lesson: `check` alone is not enough, +// because a script can skip it). +// +// A bare `CALCULATED` with no microflow is left unbound: that is the "calculated +// but not yet wired" state Studio Pro also allows. +func resolveCalculatedValue(ctx *ExecContext, mfName *ast.QualifiedName, entityQN, attrName string, attrType ast.DataType) (*domainmodel.AttributeValue, error) { + value := &domainmodel.AttributeValue{Type: "CalculatedValue"} + if mfName == nil { + return value, nil + } + qn := mfName.String() + + mfID, err := resolveMicroflowByName(ctx, qn) + if err != nil { + return nil, mdlerrors.NewBackend(fmt.Sprintf("attribute '%s'", attrName), err) + } + value.MicroflowID = mfID + value.MicroflowName = qn + + // A microflow created earlier in this same script is not readable yet, so + // its signature cannot be checked. Bind it and let the build have the last + // word rather than refusing something that may well be correct. + mf := findMicroflowByQualifiedName(ctx, qn) + if mf == nil { + value.PassEntity = true + return value, nil + } + + switch len(mf.Parameters) { + case 0: + // Parameterless calculation: Mendix stores PassEntity=false. + value.PassEntity = false + case 1: + obj, ok := mf.Parameters[0].Type.(*microflows.ObjectType) + if !ok { + return nil, mdlerrors.NewValidationf( + "attribute '%s': calculation microflow '%s' takes a %s parameter; it must take the owning entity '%s' (or no parameter at all)", + attrName, qn, mf.Parameters[0].Type.GetTypeName(), entityQN) + } + if obj.EntityQualifiedName != entityQN { + return nil, mdlerrors.NewValidationf( + "attribute '%s': calculation microflow '%s' takes a parameter of type '%s'; it must take the owning entity '%s' (mxbuild reports this as CE7247)", + attrName, qn, obj.EntityQualifiedName, entityQN) + } + value.PassEntity = true + default: + return nil, mdlerrors.NewValidationf( + "attribute '%s': calculation microflow '%s' takes %d parameters; a calculated attribute passes at most the owning entity '%s'", + attrName, qn, len(mf.Parameters), entityQN) + } + + if want := calculatedReturnTypeName(attrType); want != "" { + got := "Void" + if mf.ReturnType != nil { + got = mf.ReturnType.GetTypeName() + } + if !returnTypeSatisfies(got, want) { + return nil, mdlerrors.NewValidationf( + "attribute '%s': calculation microflow '%s' returns %s; the attribute is %s, and Mendix requires the return type to match", + attrName, qn, got, want) + } + } + return value, nil +} + +// returnTypeSatisfies reports whether a microflow returning got can calculate an +// attribute wanting want. Mendix treats Integer and Long as one family here — +// its own message is CE7247 "Microflow return type should be Integer/Long." — +// so an Integer attribute calculated by a Long-returning microflow builds +// clean and must not be refused. Every other pairing is exact; that is measured +// for Integer/Long/String and assumed (strictly) elsewhere, which errs toward +// refusing rather than writing something the build rejects. +func returnTypeSatisfies(got, want string) bool { + if got == want { + return true + } + intFamily := func(s string) bool { return s == "Integer" || s == "Long" } + return intFamily(got) && intFamily(want) +} + +// calculatedReturnTypeName is the microflow return type an attribute of this +// kind requires, or "" when the kind is not one a calculation can produce (the +// auto-* pseudo-types) and the check should be skipped. +func calculatedReturnTypeName(t ast.DataType) string { + switch t.Kind { + case ast.TypeString, ast.TypeInteger, ast.TypeLong, ast.TypeDecimal, + ast.TypeBoolean, ast.TypeDateTime, ast.TypeDate, ast.TypeBinary, + ast.TypeEnumeration: + return t.Kind.String() + default: + return "" + } +} + +// findMicroflowByQualifiedName returns the live microflow with this qualified +// name, or nil. Excluded twins are skipped (#914). +func findMicroflowByQualifiedName(ctx *ExecContext, qualifiedName string) *microflows.Microflow { + all, err := ctx.Backend.ListMicroflows() + if err != nil { + return nil + } + h, err := getHierarchy(ctx) + if err != nil { + return nil + } + mf, ok := pickLive(all, + func(m *microflows.Microflow) bool { + return h.GetQualifiedName(m.ContainerID, m.Name) == qualifiedName + }, + func(m *microflows.Microflow) bool { return m.Excluded }, + ) + if !ok { + return nil + } + return mf +} diff --git a/mdl/executor/calculated_attributes_test.go b/mdl/executor/calculated_attributes_test.go new file mode 100644 index 000000000..dbcebaaaa --- /dev/null +++ b/mdl/executor/calculated_attributes_test.go @@ -0,0 +1,141 @@ +// 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/sdk/microflows" +) + +// calcCtx wires a backend holding one module and the given microflows. +func calcCtx(t *testing.T, mfs []*microflows.Microflow) *ExecContext { + t.Helper() + const moduleID = model.ID("module-1") + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{{ + BaseElement: model.BaseElement{ID: moduleID}, + Name: "MyFirstModule", + }}, nil + }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { return mfs, nil }, + } + ctx, _ := newMockCtx(t, withBackend(mb)) + return ctx +} + +func calcMicroflow(name string, ret microflows.DataType, params ...microflows.DataType) *microflows.Microflow { + mf := µflows.Microflow{ + BaseElement: model.BaseElement{ID: model.ID("mf-" + name)}, + ContainerID: model.ID("module-1"), + Name: name, + ReturnType: ret, + } + for i, p := range params { + mf.Parameters = append(mf.Parameters, µflows.MicroflowParameter{ + Name: string(rune('A' + i)), + Type: p, + }) + } + return mf +} + +func intAttr() ast.DataType { return ast.DataType{Kind: ast.TypeInteger} } + +// TestResolveCalculatedValue_PassEntityFollowsSignature pins the two shapes +// mxbuild accepts (both measured at 0 errors on 11.13.0): a microflow taking +// the owning entity stores PassEntity=true, a parameterless one stores false. +// Hardcoding either produces a binding the build rejects. +func TestResolveCalculatedValue_PassEntityFollowsSignature(t *testing.T) { + order := µflows.ObjectType{EntityQualifiedName: "MyFirstModule.Order"} + ctx := calcCtx(t, []*microflows.Microflow{ + calcMicroflow("CalcWithEntity", µflows.IntegerType{}, order), + calcMicroflow("CalcNoParam", µflows.IntegerType{}), + }) + + qn := ast.QualifiedName{Module: "MyFirstModule", Name: "CalcWithEntity"} + v, err := resolveCalculatedValue(ctx, &qn, "MyFirstModule.Order", "Total", intAttr()) + if err != nil { + t.Fatalf("entity-parameter microflow rejected: %v", err) + } + if !v.PassEntity { + t.Error("PassEntity = false for a microflow taking the owning entity") + } + + qn = ast.QualifiedName{Module: "MyFirstModule", Name: "CalcNoParam"} + v, err = resolveCalculatedValue(ctx, &qn, "MyFirstModule.Order", "Total", intAttr()) + if err != nil { + t.Fatalf("parameterless microflow rejected, but mxbuild accepts it: %v", err) + } + if v.PassEntity { + t.Error("PassEntity = true for a parameterless microflow") + } +} + +// TestResolveCalculatedValue_RefusesWrongEntityParameter — mxbuild reports this +// as CE7247 "Microflow parameter 'X' should be of type Module.Entity", so it is +// refused before the write rather than discovered at build time. +func TestResolveCalculatedValue_RefusesWrongEntityParameter(t *testing.T) { + other := µflows.ObjectType{EntityQualifiedName: "MyFirstModule.Other"} + ctx := calcCtx(t, []*microflows.Microflow{ + calcMicroflow("CalcWrongEntity", µflows.IntegerType{}, other), + }) + + qn := ast.QualifiedName{Module: "MyFirstModule", Name: "CalcWrongEntity"} + _, err := resolveCalculatedValue(ctx, &qn, "MyFirstModule.Order", "Total", intAttr()) + if err == nil { + t.Fatal("a microflow taking the wrong entity was accepted (mxbuild: CE7247)") + } + if !strings.Contains(err.Error(), "MyFirstModule.Order") { + t.Errorf("error should name the entity the microflow must take, got: %v", err) + } +} + +// TestResolveCalculatedValue_RefusesWrongReturnType — mxbuild: CE7247 +// "Microflow return type should be Integer/Long." +func TestResolveCalculatedValue_RefusesWrongReturnType(t *testing.T) { + order := µflows.ObjectType{EntityQualifiedName: "MyFirstModule.Order"} + ctx := calcCtx(t, []*microflows.Microflow{ + calcMicroflow("CalcString", µflows.StringType{}, order), + }) + + qn := ast.QualifiedName{Module: "MyFirstModule", Name: "CalcString"} + if _, err := resolveCalculatedValue(ctx, &qn, "MyFirstModule.Order", "Total", intAttr()); err == nil { + t.Fatal("a String-returning microflow was bound to an Integer attribute (mxbuild: CE7247)") + } +} + +// TestResolveCalculatedValue_IntegerLongAreOneFamily is the case that made the +// first version of this rule wrong: Mendix's own message is "should be +// Integer/Long", and a Long-returning microflow on an Integer attribute builds +// at 0 errors — so a strict equality check refuses valid MDL. +func TestResolveCalculatedValue_IntegerLongAreOneFamily(t *testing.T) { + order := µflows.ObjectType{EntityQualifiedName: "MyFirstModule.Order"} + ctx := calcCtx(t, []*microflows.Microflow{ + calcMicroflow("CalcLong", µflows.LongType{}, order), + }) + + qn := ast.QualifiedName{Module: "MyFirstModule", Name: "CalcLong"} + if _, err := resolveCalculatedValue(ctx, &qn, "MyFirstModule.Order", "Total", intAttr()); err != nil { + t.Fatalf("Long return on an Integer attribute refused, but mxbuild accepts it: %v", err) + } +} + +// TestResolveCalculatedValue_BareCalculatedIsUnbound — `CALCULATED` with no +// microflow is the "not yet wired" state Studio Pro also allows. +func TestResolveCalculatedValue_BareCalculatedIsUnbound(t *testing.T) { + ctx := calcCtx(t, nil) + v, err := resolveCalculatedValue(ctx, nil, "MyFirstModule.Order", "Total", intAttr()) + if err != nil { + t.Fatalf("bare CALCULATED rejected: %v", err) + } + if v.Type != "CalculatedValue" || v.MicroflowName != "" { + t.Errorf("bare CALCULATED should be an unbound CalculatedValue, got %+v", v) + } +} diff --git a/mdl/executor/cmd_entities.go b/mdl/executor/cmd_entities.go index b1f0e13a5..63f5e7d5d 100644 --- a/mdl/executor/cmd_entities.go +++ b/mdl/executor/cmd_entities.go @@ -227,16 +227,9 @@ func execCreateEntity(ctx *ExecContext, s *ast.CreateEntityStmt) error { // Value type: CALCULATED or DEFAULT if a.Calculated { - attrValue := &domainmodel.AttributeValue{ - Type: "CalculatedValue", - } - if a.CalculatedMicroflow != nil { - mfID, err := resolveMicroflowByName(ctx, a.CalculatedMicroflow.String()) - if err != nil { - return mdlerrors.NewBackend(fmt.Sprintf("attribute '%s'", a.Name), err) - } - attrValue.MicroflowID = mfID - attrValue.MicroflowName = a.CalculatedMicroflow.String() + attrValue, err := resolveCalculatedValue(ctx, a.CalculatedMicroflow, s.Name.String(), a.Name, a.Type) + if err != nil { + return err } attr.Value = attrValue } else if a.HasDefault { @@ -745,16 +738,9 @@ func execAlterEntity(ctx *ExecContext, s *ast.AlterEntityStmt) error { } attr.ID = attrID if a.Calculated { - attrValue := &domainmodel.AttributeValue{ - Type: "CalculatedValue", - } - if a.CalculatedMicroflow != nil { - mfID, err := resolveMicroflowByName(ctx, a.CalculatedMicroflow.String()) - if err != nil { - return mdlerrors.NewBackend(fmt.Sprintf("attribute '%s'", a.Name), err) - } - attrValue.MicroflowID = mfID - attrValue.MicroflowName = a.CalculatedMicroflow.String() + attrValue, err := resolveCalculatedValue(ctx, a.CalculatedMicroflow, s.Name.String(), a.Name, a.Type) + if err != nil { + return err } attr.Value = attrValue } else if a.HasDefault { @@ -928,16 +914,9 @@ func execAlterEntity(ctx *ExecContext, s *ast.AlterEntityStmt) error { if attr.Name == s.AttributeName { attr.Type = convertDataType(s.DataType) if s.Calculated { - attrValue := &domainmodel.AttributeValue{ - Type: "CalculatedValue", - } - if s.CalculatedMicroflow != nil { - mfID, err := resolveMicroflowByName(ctx, s.CalculatedMicroflow.String()) - if err != nil { - return mdlerrors.NewBackend(fmt.Sprintf("attribute '%s'", s.AttributeName), err) - } - attrValue.MicroflowID = mfID - attrValue.MicroflowName = s.CalculatedMicroflow.String() + attrValue, err := resolveCalculatedValue(ctx, s.CalculatedMicroflow, s.Name.String(), s.AttributeName, s.DataType) + if err != nil { + return err } attr.Value = attrValue } diff --git a/mdl/executor/cmd_export_mappings.go b/mdl/executor/cmd_export_mappings.go index ae778c545..645af902c 100644 --- a/mdl/executor/cmd_export_mappings.go +++ b/mdl/executor/cmd_export_mappings.go @@ -103,7 +103,7 @@ func describeExportMapping(ctx *ExecContext, name ast.QualifiedName) error { modID := h.FindModuleID(em.ContainerID) moduleName := h.GetModuleName(modID) - fmt.Fprintf(ctx.Output, "create export mapping %s.%s\n", moduleName, em.Name) + fmt.Fprintf(ctx.Output, "create or modify export mapping %s.%s\n", moduleName, em.Name) if em.JsonStructure != "" { fmt.Fprintf(ctx.Output, " with json structure %s\n", em.JsonStructure) @@ -144,11 +144,11 @@ func printExportMappingElement(w io.Writer, elem *model.ExportMappingElement, de assoc := elem.Association entity := elem.Entity if assoc == "" && entity == "" { - fmt.Fprintf(w, "%s. as %s", indent, elem.ExposedName) + fmt.Fprintf(w, "%s. as %s", indent, mappingMemberName(elem.JsonPath, elem.ExposedName)) } else if assoc == "" { - fmt.Fprintf(w, "%s./%s as %s", indent, entity, elem.ExposedName) + fmt.Fprintf(w, "%s./%s as %s", indent, entity, mappingMemberName(elem.JsonPath, elem.ExposedName)) } else { - fmt.Fprintf(w, "%s%s/%s as %s", indent, assoc, entity, elem.ExposedName) + fmt.Fprintf(w, "%s%s/%s as %s", indent, assoc, entity, mappingMemberName(elem.JsonPath, elem.ExposedName)) } if len(elem.Children) > 0 { fmt.Fprintln(w, " {") @@ -172,7 +172,7 @@ func printExportMappingElement(w io.Writer, elem *model.ExportMappingElement, de if parts := strings.Split(attrName, "."); len(parts) == 3 { attrName = parts[2] } - fmt.Fprintf(w, "%s%s = %s", indent, elem.ExposedName, attrName) + fmt.Fprintf(w, "%s%s = %s", indent, mappingMemberName(elem.JsonPath, elem.ExposedName), attrName) } } diff --git a/mdl/executor/cmd_export_mappings_mock_test.go b/mdl/executor/cmd_export_mappings_mock_test.go index eaf30d45c..1124cc2a4 100644 --- a/mdl/executor/cmd_export_mappings_mock_test.go +++ b/mdl/executor/cmd_export_mappings_mock_test.go @@ -86,7 +86,8 @@ func TestDescribeExportMapping_Mock(t *testing.T) { ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) assertNoError(t, describeExportMapping(ctx, ast.QualifiedName{Module: "Integration", Name: "ExportOrders"})) - assertContainsStr(t, buf.String(), "create export mapping") + // Re-runnable header, as for import mappings (#915). + assertContainsStr(t, buf.String(), "create or modify export mapping") } func TestDescribeExportMapping_NotFound(t *testing.T) { diff --git a/mdl/executor/cmd_import_mappings.go b/mdl/executor/cmd_import_mappings.go index fd96629f5..ddf213d05 100644 --- a/mdl/executor/cmd_import_mappings.go +++ b/mdl/executor/cmd_import_mappings.go @@ -103,7 +103,7 @@ func describeImportMapping(ctx *ExecContext, name ast.QualifiedName) error { modID := h.FindModuleID(im.ContainerID) moduleName := h.GetModuleName(modID) - fmt.Fprintf(ctx.Output, "create import mapping %s.%s\n", moduleName, im.Name) + fmt.Fprintf(ctx.Output, "create or modify import mapping %s.%s\n", moduleName, im.Name) if im.JsonStructure != "" { fmt.Fprintf(ctx.Output, " with json structure %s\n", im.JsonStructure) @@ -153,11 +153,11 @@ func printImportMappingElement(w io.Writer, elem *model.ImportMappingElement, de assoc := elem.Association entity := elem.Entity if assoc == "" && entity == "" { - fmt.Fprintf(w, "%s%s . = %s", indent, handling, elem.ExposedName) + fmt.Fprintf(w, "%s%s . = %s", indent, handling, mappingMemberName(elem.JsonPath, elem.ExposedName)) } else if assoc == "" { - fmt.Fprintf(w, "%s%s ./%s = %s", indent, handling, entity, elem.ExposedName) + fmt.Fprintf(w, "%s%s ./%s = %s", indent, handling, entity, mappingMemberName(elem.JsonPath, elem.ExposedName)) } else { - fmt.Fprintf(w, "%s%s %s/%s = %s", indent, handling, assoc, entity, elem.ExposedName) + fmt.Fprintf(w, "%s%s %s/%s = %s", indent, handling, assoc, entity, mappingMemberName(elem.JsonPath, elem.ExposedName)) } if len(elem.Children) > 0 { fmt.Fprintln(w, " {") @@ -185,10 +185,43 @@ func printImportMappingElement(w io.Writer, elem *model.ImportMappingElement, de if elem.IsKey { keyStr = " key" } - fmt.Fprintf(w, "%s%s = %s%s", indent, attrName, elem.ExposedName, keyStr) + fmt.Fprintf(w, "%s%s = %s%s", indent, attrName, mappingMemberName(elem.JsonPath, elem.ExposedName), keyStr) } } +// mappingMemberName is the JSON member name DESCRIBE should print for a mapping +// element: the raw key taken from its JsonPath, not the derived ExposedName. +// +// The two differ for any lowercase-initial key, because Mendix derives +// ExposedName by capitalising the initial (and suffixing "Item" for an array's +// item object) — Studio Pro does the same, so the stored ExposedName is correct +// and is deliberately left alone. But printing it made DESCRIBE output that no +// longer matched the script that produced it: `Total = total` came back as +// `Total = Total`, `= item` as `= ItemItem`, `LineId = id` as `LineId = _id`. +// The mapping those re-execute to is identical (jsonSchemaIndex.resolve accepts +// either spelling since #882), but a diff of script vs DESCRIBE was pure noise. +// +// The raw key is also the index's FIRST lookup, so printing it cannot regress +// that resolution. Elements with no JsonPath — an XML-schema or +// message-definition mapping — keep the exposed name, which is all they have. +// (issue #915) +func mappingMemberName(jsonPath, exposedName string) string { + if jsonPath == "" { + return exposedName + } + // An array's item object is addressed by the ARRAY's key: the mapping element + // sits at "(Object)|item|(Object)" and the script wrote "item". + trimmed := strings.TrimSuffix(jsonPath, "|(Object)") + i := strings.LastIndex(trimmed, "|") + if i < 0 { + return exposedName + } + if name := trimmed[i+1:]; name != "" && name != "(Object)" { + return name + } + return exposedName +} + // execCreateImportMapping creates a new import mapping. func execCreateImportMapping(ctx *ExecContext, s *ast.CreateImportMappingStmt) error { if !ctx.ConnectedForWrite() { diff --git a/mdl/executor/cmd_import_mappings_mock_test.go b/mdl/executor/cmd_import_mappings_mock_test.go index 976fbbea9..2a997e73f 100644 --- a/mdl/executor/cmd_import_mappings_mock_test.go +++ b/mdl/executor/cmd_import_mappings_mock_test.go @@ -86,7 +86,9 @@ func TestDescribeImportMapping_Mock(t *testing.T) { ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) assertNoError(t, describeImportMapping(ctx, ast.QualifiedName{Module: "Integration", Name: "ImportOrders"})) - assertContainsStr(t, buf.String(), "create import mapping") + // DESCRIBE emits a re-runnable header: the bare `create` form failed with + // "import mapping already exists" against the project it was read from (#915). + assertContainsStr(t, buf.String(), "create or modify import mapping") } func TestDescribeImportMapping_NotFound(t *testing.T) { diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index c35a486ff..f776a6875 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -57,7 +57,7 @@ type flowBuilder struct { // without this a describe→exec round-trip silently moved it (a Studio Pro // flow's 145;200 became 100;200). Nil on a fresh CREATE, where the position // is derived from the first annotated activity as before. - startPosition *model.Point + startPosition *model.Point backend backend.FullBackend // For looking up page/microflow references hierarchy *ContainerHierarchy // For resolving container IDs to module names pendingAnnotations *ast.ActivityAnnotations // Pending annotations to attach to next activity diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 7bdd545ec..8e8c3362d 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -1101,7 +1101,11 @@ func (fb *flowBuilder) addListOperationAction(s *ast.ListOperationStmt) model.ID // a declared String variable, Mendix requires a Change Variable action // carrying the string expression — a List operation activity on strings fails // the build (CE0023/CE0097/CE0111). Ledger findings #53 (contains) and #63 (find). - if fb.declaredVars != nil && fb.declaredVars[s.InputVariable] == "String" { + // + // The operation test is shared with MDL063, which must not report the + // CE0111 this rewrite exists to avoid — see stringOverloadedListOp. + if fb.declaredVars != nil && fb.declaredVars[s.InputVariable] == "String" && + stringOverloadedListOp(s.Operation) { switch s.Operation { case ast.ListOpContains: return fb.addChangeVariableAction(&ast.MfSetStmt{ diff --git a/mdl/executor/cmd_microflows_create.go b/mdl/executor/cmd_microflows_create.go index 6f1e7e34e..e7220e24e 100644 --- a/mdl/executor/cmd_microflows_create.go +++ b/mdl/executor/cmd_microflows_create.go @@ -275,15 +275,15 @@ func execCreateMicroflow(ctx *ExecContext, s *ast.CreateMicroflowStmt) error { // survive. Preserved the way the folder and allowed roles already are. startPosition: storedStartPosition(ctx, existingID), posX: 200, - posY: 200, - baseY: 200, // Base Y for happy path - spacing: HorizontalSpacing, - varTypes: varTypes, - declaredVars: declaredVars, - measurer: &layoutMeasurer{varTypes: varTypes}, - backend: ctx.Backend, - hierarchy: hierarchy, - restServices: restServices, + posY: 200, + baseY: 200, // Base Y for happy path + spacing: HorizontalSpacing, + varTypes: varTypes, + declaredVars: declaredVars, + measurer: &layoutMeasurer{varTypes: varTypes}, + backend: ctx.Backend, + hierarchy: hierarchy, + restServices: restServices, } mf.ObjectCollection = builder.buildFlowGraph(s.Body, s.ReturnType) diff --git a/mdl/executor/mapping_describe_roundtrip_test.go b/mdl/executor/mapping_describe_roundtrip_test.go new file mode 100644 index 000000000..d3e6506ac --- /dev/null +++ b/mdl/executor/mapping_describe_roundtrip_test.go @@ -0,0 +1,158 @@ +// 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" +) + +// TestMappingMemberName covers the name DESCRIBE prints for a mapping element. +// The array case is the one that produced "ItemItem": the mapping element sits +// at the item object, but the script addressed the ARRAY. +func TestMappingMemberName(t *testing.T) { + cases := []struct { + name string + jsonPath string + exposed string + want string + }{ + {"value under root", "(Object)|total", "Total", "total"}, + {"camelCase preserved", "(Object)|camelCase", "CamelCase", "camelCase"}, + {"array item object uses the array's key", "(Object)|item|(Object)", "ItemItem", "item"}, + {"value inside an array item", "(Object)|item|(Object)|id", "_id", "id"}, + {"no JsonPath falls back (XML / message mapping)", "", "Total", "Total"}, + {"root has no member name", "(Object)", "JsonObject", "JsonObject"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := mappingMemberName(c.jsonPath, c.exposed); got != c.want { + t.Errorf("mappingMemberName(%q, %q) = %q, want %q", c.jsonPath, c.exposed, got, c.want) + } + }) + } +} + +// TestDescribeImportMapping_RoundTripsMemberNames is #915: DESCRIBE has to +// reproduce the script that produced the mapping. Printing ExposedName turned +// `Total = total` into `Total = Total` and `= item` into `= ItemItem`, so a diff +// of script against DESCRIBE was pure noise — and the bare `create` header meant +// the output could not be re-run at all ("import mapping already exists"). +func TestDescribeImportMapping_RoundTripsMemberNames(t *testing.T) { + mod := mkModule("Integration") + im := &model.ImportMapping{ + BaseElement: model.BaseElement{ID: nextID("im")}, + ContainerID: mod.ID, + Name: "IMM_Payload", + JsonStructure: "Integration.JSON_Payload", + Elements: []*model.ImportMappingElement{{ + Kind: "Object", + Entity: "Integration.Payload", + ObjectHandling: "Create", + ExposedName: "JsonObject", + JsonPath: "(Object)", + Children: []*model.ImportMappingElement{ + { + Kind: "Value", + Attribute: "Integration.Payload.Total", + ExposedName: "Total", + JsonPath: "(Object)|total", + }, + { + Kind: "Object", + Entity: "Integration.Line", + Association: "Integration.Line_Payload", + ObjectHandling: "Create", + ExposedName: "ItemItem", + JsonPath: "(Object)|item|(Object)", + Children: []*model.ImportMappingElement{{ + Kind: "Value", + Attribute: "Integration.Line.LineId", + ExposedName: "_id", + JsonPath: "(Object)|item|(Object)|id", + }}, + }, + }, + }}, + } + + h := mkHierarchy(mod) + withContainer(h, im.ContainerID, mod.ID) + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetImportMappingByQualifiedNameFunc: func(moduleName, name string) (*model.ImportMapping, error) { + return im, nil + }, + } + + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) + assertNoError(t, describeImportMapping(ctx, ast.QualifiedName{Module: "Integration", Name: "IMM_Payload"})) + out := buf.String() + + // Re-runnable against the project it was read from. + if !strings.Contains(out, "create or modify import mapping") { + t.Errorf("DESCRIBE must emit a re-runnable header, got:\n%s", out) + } + // The raw JSON keys the script wrote, not Mendix's derived display names. + for _, want := range []string{"= total", "= item", "= id"} { + if !strings.Contains(out, want) { + t.Errorf("missing %q in DESCRIBE output:\n%s", want, out) + } + } + for _, unwanted := range []string{"= Total", "ItemItem", "= _id"} { + if strings.Contains(out, unwanted) { + t.Errorf("DESCRIBE printed the derived name %q instead of the JSON key:\n%s", unwanted, out) + } + } +} + +// TestDescribeExportMapping_RoundTripsMemberNames — the export side had the +// identical defect, which the issue did not mention. +func TestDescribeExportMapping_RoundTripsMemberNames(t *testing.T) { + mod := mkModule("Integration") + em := &model.ExportMapping{ + BaseElement: model.BaseElement{ID: nextID("em")}, + ContainerID: mod.ID, + Name: "EMM_Payload", + JsonStructure: "Integration.JSON_Payload", + Elements: []*model.ExportMappingElement{{ + Kind: "Object", + Entity: "Integration.Payload", + ExposedName: "JsonObject", + JsonPath: "(Object)", + Children: []*model.ExportMappingElement{{ + Kind: "Value", + Attribute: "Integration.Payload.Total", + ExposedName: "Total", + JsonPath: "(Object)|total", + }}, + }}, + } + + h := mkHierarchy(mod) + withContainer(h, em.ContainerID, mod.ID) + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + GetExportMappingByQualifiedNameFunc: func(moduleName, name string) (*model.ExportMapping, error) { + return em, nil + }, + } + + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) + assertNoError(t, describeExportMapping(ctx, ast.QualifiedName{Module: "Integration", Name: "EMM_Payload"})) + out := buf.String() + + if !strings.Contains(out, "create or modify export mapping") { + t.Errorf("DESCRIBE must emit a re-runnable header, got:\n%s", out) + } + if !strings.Contains(out, "total =") { + t.Errorf("expected the raw JSON key %q in:\n%s", "total =", out) + } + if strings.Contains(out, "Total =") { + t.Errorf("DESCRIBE printed the derived ExposedName instead of the JSON key:\n%s", out) + } +} diff --git a/mdl/executor/typecheck.go b/mdl/executor/typecheck.go new file mode 100644 index 000000000..0e0053730 --- /dev/null +++ b/mdl/executor/typecheck.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "context" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/exprcatalog" + "github.com/mendixlabs/mxcli/mdl/exprcheck/adapters" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// TypeCheckProgram type-checks the expressions in a script's microflows and +// nanoflows against the connected project, returning one violation per hint. +// +// This is the catalog-backed tier of PROPOSAL_expression_type_checking: the +// rules that need to know an attribute's type, an enumeration's cases, or a +// microflow's return type. The scope-local tier already runs unconditionally in +// ValidateProgram, so this only adds what a project can answer. +// +// Hints keep exprcheck's own E0xx codes rather than being remapped. They are a +// coherent, documented set with a hints registry behind them, and renaming them +// at the boundary would mean two vocabularies for one diagnostic — the code in +// the message would no longer match the code you can look up. +// +// A project that cannot be read, or a catalog that cannot be built, returns no +// violations rather than an error. Type checking is advisory: a caller that +// could not consult the project should report what it could check, not fail. +func (e *Executor) TypeCheckProgram(prog *ast.Program) []linter.Violation { + if prog == nil || e == nil { + return nil + } + ctx := e.newExecContext(context.Background()) + if !ctx.Connected() { + return nil + } + + // Fast mode is enough: attributes, enumeration values, microflows and their + // parameters are all built in it. Only permissions, references, strings and + // XPath need a full build, and none of them feed a type lookup — so a check + // never pays for a full catalog. + if err := ensureCatalog(ctx, false); err != nil { + return nil + } + cat := ctx.Catalog + if cat == nil { + return nil + } + + reader, err := exprcatalog.Load(cat.CatalogDB()) + if err != nil { + return nil + } + + // microflowExprSource falls back to rendering the AST when the visitor did + // not attach source text, which it does for some slots and not others. The + // adapter's own default reads SourceExpr only, and would silently skip + // whichever half of a flow happened not to carry one. + adapter := adapters.NewCheckAdapter(reader, adapters.WithSourceFunc(microflowExprSource)) + var out []linter.Violation + for _, stmt := range prog.Statements { + switch s := stmt.(type) { + case *ast.CreateMicroflowStmt: + out = append(out, adapter.CheckMicroflow(s).AsViolations()...) + case *ast.CreateNanoflowStmt: + out = append(out, adapter.CheckNanoflow(s).AsViolations()...) + } + } + return out +} diff --git a/mdl/executor/typecheck_test.go b/mdl/executor/typecheck_test.go new file mode 100644 index 000000000..e9edffc9d --- /dev/null +++ b/mdl/executor/typecheck_test.go @@ -0,0 +1,289 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend" + mprbackend "github.com/mendixlabs/mxcli/mdl/backend/mpr" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// typeCheckFixture copies the shared fixture project into a temp dir, connects an +// executor to it for writing, and seeds an enumeration plus an entity that uses +// it. +// +// A real project rather than a mock: the whole point of this path is that the +// catalog answers questions about a model on disk, and every one of the three +// defects found while building it (a missing column, an absent table, an +// expression whose source text the walk never saw) would have passed a mocked +// test. +func typeCheckFixture(t *testing.T) *Executor { + t.Helper() + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../testdata/expr-checker")); err != nil { + t.Fatalf("copy fixture: %v", err) + } + proj := filepath.Join(dst, "minimal.mpr") + + exec := New(&bytes.Buffer{}) + exec.SetQuiet(true) + exec.SetBackendFactory(func() backend.FullBackend { return mprbackend.New() }) + t.Cleanup(func() { exec.Close() }) + + run(t, exec, "CONNECT LOCAL '"+visitor.QuoteString(proj)+"'") + run(t, exec, `CREATE ENUMERATION MyFirstModule.OrderStatus (Open 'Open', Closed 'Closed');`) + run(t, exec, `CREATE PERSISTENT ENTITY MyFirstModule.Ticket ( + Title: String(100), + Status: Enumeration(MyFirstModule.OrderStatus) + );`) + run(t, exec, `CREATE PERSISTENT ENTITY MyFirstModule.Reporter (Email: String(200));`) + run(t, exec, `CREATE ASSOCIATION MyFirstModule.Ticket_Reporter + FROM MyFirstModule.Ticket TO MyFirstModule.Reporter;`) + return exec +} + +func run(t *testing.T, exec *Executor, mdl string) { + t.Helper() + prog, errs := visitor.Build(mdl) + if len(errs) > 0 { + t.Fatalf("parsing %q: %v", mdl, errs) + } + for _, stmt := range prog.Statements { + if err := exec.Execute(stmt); err != nil { + t.Fatalf("executing %q: %v", mdl, err) + } + } +} + +func typeCheck(t *testing.T, exec *Executor, mdl string) []linter.Violation { + t.Helper() + prog, errs := visitor.Build(mdl) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + return exec.TypeCheckProgram(prog) +} + +// TestTypeCheckProgramCatchesEnumStringLiteral is the end-to-end proof that the +// checker now checks something. Before the CatalogReader seam had an +// implementation, this ran with a nil Catalog and every semantic rule was +// skipped — a green result that meant nothing. +func TestTypeCheckProgramCatchesEnumStringLiteral(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Bug () +BEGIN + $T = CREATE MyFirstModule.Ticket (Title = 'x', Status = 'Open'); +END; +`) + + if len(got) != 1 { + t.Fatalf("got %d violations, want 1: %+v", len(got), got) + } + if got[0].RuleID != "E001" { + t.Errorf("rule is %q, want exprcheck's own E001 — the codes are kept, not remapped", got[0].RuleID) + } + if got[0].Severity != linter.SeverityError { + t.Errorf("severity is %v, want error", got[0].Severity) + } + // The fix must name the enum the catalog resolved, which is the part that + // only works because AttributeEnumQN and EnumCases have data behind them. + if !strings.Contains(got[0].Suggestion, "MyFirstModule.OrderStatus.Open") { + t.Errorf("suggestion is %q, want the qualified enum value", got[0].Suggestion) + } +} + +// TestTypeCheckProgramAcceptsTheCorrectedForm is the control. Without it the +// test above would pass against a checker that flagged everything. +func TestTypeCheckProgramAcceptsTheCorrectedForm(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Fixed () +BEGIN + $T = CREATE MyFirstModule.Ticket (Title = 'x', Status = MyFirstModule.OrderStatus.Open); +END; +`) + + if len(got) != 0 { + t.Errorf("the corrected form was flagged: %+v", got) + } +} + +// TestTypeCheckProgramSeesChangeAsWellAsCreate pins the fix for the second +// wiring defect. The adapter's default source function reads only +// ast.SourceExpr, and the visitor attaches one to a CREATE's value but not to a +// CHANGE's — so half the enum mistakes in one microflow were invisible until the +// executor supplied a source function that can render either. +func TestTypeCheckProgramSeesChangeAsWellAsCreate(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Both () +BEGIN + $T = CREATE MyFirstModule.Ticket (Status = 'Open'); + CHANGE $T (Status = 'Closed'); +END; +`) + + if len(got) != 2 { + t.Fatalf("got %d violations, want one for the CREATE and one for the CHANGE: %+v", len(got), got) + } + var sawOpen, sawClosed bool + for _, v := range got { + sawOpen = sawOpen || strings.Contains(v.Suggestion, "OrderStatus.Open") + sawClosed = sawClosed || strings.Contains(v.Suggestion, "OrderStatus.Closed") + } + if !sawOpen || !sawClosed { + t.Errorf("expected both values reported, got %+v", got) + } +} + +// TestTypeCheckProgramLeavesNonEnumAttributesAlone pins that the rule keys off +// the attribute's actual type, not off any string literal in a member slot. +func TestTypeCheckProgramLeavesNonEnumAttributesAlone(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Strings () +BEGIN + $T = CREATE MyFirstModule.Ticket (Title = 'Open'); +END; +`) + + if len(got) != 0 { + t.Errorf("a String attribute assigned a string literal was flagged: %+v", got) + } +} + +// TestTypeCheckProgramWithoutAConnectionIsSilent pins the advisory contract: a +// caller that cannot consult a project gets no violations rather than an error. +func TestTypeCheckProgramWithoutAConnectionIsSilent(t *testing.T) { + exec := New(&bytes.Buffer{}) + defer exec.Close() + + prog, errs := visitor.Build(`CREATE MICROFLOW M.A () BEGIN LOG 'x'; END;`) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs) + } + if got := exec.TypeCheckProgram(prog); got != nil { + t.Errorf("an unconnected executor reported %+v", got) + } + if got := exec.TypeCheckProgram(nil); got != nil { + t.Errorf("a nil program reported %+v", got) + } +} + +// TestTypeCheckProgramResolvesAttributePaths is the Tier-2 case: `$T/Status` +// has no slot naming its attribute, so the only way to know it is an +// enumeration is to type the variable and look the attribute up. inferKind +// returned KindUnknown for every attribute path until this landed, which is why +// the shape the proposal opens with went uncaught. +func TestTypeCheckProgramResolvesAttributePaths(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Path ($T: MyFirstModule.Ticket) +BEGIN + IF $T/Status = 'Open' THEN + LOG 'open'; + END IF; +END; +`) + + if len(got) != 1 { + t.Fatalf("got %d violations, want 1: %+v", len(got), got) + } + if got[0].RuleID != "E001" { + t.Errorf("rule is %q, want E001", got[0].RuleID) + } + if !strings.Contains(got[0].Suggestion, "MyFirstModule.OrderStatus.Open") { + t.Errorf("suggestion is %q, want the qualified enum value", got[0].Suggestion) + } +} + +// TestTypeCheckProgramTypesAParameter pins the half of the scope that was +// missing entirely: buildVarEntityScope walks only the body, so a variable the +// microflow takes as a parameter — the ordinary case — was never typed. +func TestTypeCheckProgramTypesAParameter(t *testing.T) { + exec := typeCheckFixture(t) + + // The variable is introduced by RETRIEVE rather than by a parameter, which + // buildVarEntityScope already covered; this is the control for the pair. + fromBody := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Body () +BEGIN + RETRIEVE $T FROM MyFirstModule.Ticket; + IF $T/Status = 'Open' THEN + LOG 'x'; + END IF; +END; +`) + if len(fromBody) != 1 { + t.Errorf("a RETRIEVE-introduced variable was not typed: %+v", fromBody) + } + + fromParam := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Param ($T: MyFirstModule.Ticket) +BEGIN + IF $T/Status = 'Open' THEN + LOG 'x'; + END IF; +END; +`) + if len(fromParam) != 1 { + t.Errorf("a parameter-introduced variable was not typed: %+v", fromParam) + } +} + +// TestTypeCheckProgramFollowsAnAssociation pins the multi-hop path. A Mendix +// expression does not name the intermediate entity the way XPath does, so each +// hop has to be resolved rather than read off the path. +func TestTypeCheckProgramFollowsAnAssociation(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Hop ($T: MyFirstModule.Ticket) +BEGIN + DECLARE $Label String = 'to: ' + $T/MyFirstModule.Ticket_Reporter/Email; + DECLARE $Bad String = 'status: ' + $T/Status; +END; +`) + + // Email is a String, so concatenating it is fine and must stay quiet; Status + // is an Enumeration, which Mendix will not concatenate. + if len(got) != 1 { + t.Fatalf("got %d violations, want only the Enumeration concat: %+v", len(got), got) + } + if got[0].RuleID != "E004" { + t.Errorf("rule is %q, want E004", got[0].RuleID) + } +} + +// TestTypeCheckProgramLeavesUnresolvableVariablesAlone pins the failure +// direction end to end. $currentUser is a platform variable the scope does not +// know, and a real project is full of them. +func TestTypeCheckProgramLeavesUnresolvableVariablesAlone(t *testing.T) { + exec := typeCheckFixture(t) + + got := typeCheck(t, exec, ` +CREATE OR REPLACE MICROFLOW MyFirstModule.ACT_Unknown () +BEGIN + IF $currentUser/Name = 'Ada' THEN + LOG 'x'; + END IF; +END; +`) + + if len(got) != 0 { + t.Errorf("an untypeable variable produced %+v", got) + } +} diff --git a/mdl/executor/validate_builtin_misuse_test.go b/mdl/executor/validate_builtin_misuse_test.go new file mode 100644 index 000000000..2344d677f --- /dev/null +++ b/mdl/executor/validate_builtin_misuse_test.go @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import "testing" + +// TestMisusedBuiltinPropertyNamesTheWorkingSpelling is the regression test for +// mxcli-owid #38: `combobox cb (Association: …, Caption: Name)` passed check, +// executed, lost the caption, and failed the build with +// CE0642 "Property 'Caption' is required." The working spelling is +// CaptionAttribute, so the author was one property name away — and concluded +// the feature was unusable and redesigned around it. +func TestMisusedBuiltinPropertyNamesTheWorkingSpelling(t *testing.T) { + const comboBox = "com.mendix.widget.web.combobox.Combobox" + + right, wrong := misusedBuiltinProperty(comboBox, "Caption") + if !wrong { + t.Fatal("Caption on a combobox was not flagged") + } + if right != "CaptionAttribute" { + t.Errorf("suggested %q, want CaptionAttribute", right) + } +} + +// TestMisusedBuiltinPropertyIsCaseInsensitive — property lookup elsewhere folds +// case, so a rule that did not would miss `caption:`. +func TestMisusedBuiltinPropertyIsCaseInsensitive(t *testing.T) { + const comboBox = "com.mendix.widget.web.combobox.Combobox" + for _, spelling := range []string{"Caption", "caption", "CAPTION"} { + if _, wrong := misusedBuiltinProperty(comboBox, spelling); !wrong { + t.Errorf("%q was not flagged", spelling) + } + } +} + +// TestMisusedBuiltinPropertyLeavesEverythingElseAlone. The rule is a short list +// of measured cases; it must not touch the working spelling, other properties on +// the same widget, or the same property on a widget that does route it. +func TestMisusedBuiltinPropertyLeavesEverythingElseAlone(t *testing.T) { + const comboBox = "com.mendix.widget.web.combobox.Combobox" + cases := []struct{ widgetID, key string }{ + {comboBox, "CaptionAttribute"}, // the fix must not be flagged + {comboBox, "Label"}, + {comboBox, "Association"}, + {comboBox, "Class"}, + // An action button's Caption is routed and entirely correct. + {"com.mendix.widget.web.actionbutton.ActionButton", "Caption"}, + {"com.mendix.widget.custom.unknown.Unknown", "Caption"}, + } + for _, c := range cases { + if right, wrong := misusedBuiltinProperty(c.widgetID, c.key); wrong { + t.Errorf("%s on %s was flagged (suggested %q)", c.key, c.widgetID, right) + } + } +} diff --git a/mdl/executor/validate_column_name_test.go b/mdl/executor/validate_column_name_test.go new file mode 100644 index 000000000..318c3a705 --- /dev/null +++ b/mdl/executor/validate_column_name_test.go @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +func columnWidget(name, attr, caption string) *ast.WidgetV3 { + w := &ast.WidgetV3{Type: "COLUMN", Name: name, Properties: map[string]any{}} + if attr != "" { + w.Properties["Attribute"] = attr + } + if caption != "" { + w.Properties["Caption"] = caption + } + return w +} + +// gridWith wraps columns in the DataGrid the rule is reported against. +func gridWith(cols ...*ast.WidgetV3) *ast.WidgetV3 { + return &ast.WidgetV3{Type: "DATAGRID", Name: "dg1", Children: cols} +} + +// TestColumnNameWarningNamesTheAddressableName. DataGrid 2 stores no column +// name, so the one written in MDL is dropped and the column is addressed by a +// derived name. An author who wrote `colLabel` otherwise finds out only when +// `ALTER PAGE … ON dg1.colLabel` fails on a column they just named. +func TestColumnNameWarningNamesTheAddressableName(t *testing.T) { + v := validateDataGrid2ColumnNames(gridWith(columnWidget("colLabel", "Label", "The Label")), "page X") + if len(v) != 1 { + t.Fatalf("violations = %d, want 1", len(v)) + } + if v[0].RuleID != "MDL-WIDGET16" { + t.Errorf("RuleID = %q", v[0].RuleID) + } + if !strings.Contains(v[0].Message, "colLabel → Label") { + t.Errorf("message does not name the mapping:\n%s", v[0].Message) + } +} + +// TestColumnNameWarningIsQuietWhenTheNamesAgree — writing the name the column +// will actually answer to is not a mistake and must not be nagged about. +func TestColumnNameWarningIsQuietWhenTheNamesAgree(t *testing.T) { + for _, name := range []string{"Label", "label"} { + if v := validateDataGrid2ColumnNames(gridWith(columnWidget(name, "Label", "")), "page X"); len(v) != 0 { + t.Errorf("%s warned unnecessarily: %s", name, v[0].Message) + } + } +} + +// TestColumnNameWarningUsesTheCaptionWhenThereIsNoAttribute — a custom-content +// column keys on its caption, sanitized the same way the writer does. +func TestColumnNameWarningUsesTheCaptionWhenThereIsNoAttribute(t *testing.T) { + v := validateDataGrid2ColumnNames(gridWith(columnWidget("colActions", "", "Row actions")), "page X") + if len(v) != 1 { + t.Fatalf("violations = %d, want 1", len(v)) + } + if !strings.Contains(v[0].Message, "colActions → Row_actions") { + t.Errorf("message should name the sanitized caption:\n%s", v[0].Message) + } +} + +// TestColumnNameWarningStaysSilentWhenItCannotTell. With neither attribute nor +// caption the addressable name is colN, which depends on the column's position — +// naming a wrong one would be worse than saying nothing. +func TestColumnNameWarningStaysSilentWhenItCannotTell(t *testing.T) { + if v := validateDataGrid2ColumnNames(gridWith(columnWidget("colMystery", "", "")), "page X"); len(v) != 0 { + t.Errorf("guessed a name it cannot know: %s", v[0].Message) + } +} + +// TestColumnNameWarningIgnoresNonColumns — the rule is scoped to DataGrid 2 +// columns, not to every object-list item. +func TestColumnNameWarningIgnoresNonColumns(t *testing.T) { + w := columnWidget("someItem", "Label", "") + w.Type = "TEXTBOX" + if v := validateDataGrid2ColumnNames(gridWith(w), "page X"); len(v) != 0 { + t.Errorf("warned on a non-column: %s", v[0].Message) + } +} + +// TestColumnNameWarningIsOnePerGrid pins the aggregation. Emitting one info per +// column produced 44 of them on a real project, all saying the same thing about +// the same grid — the fact belongs to the grid, not to each column. +func TestColumnNameWarningIsOnePerGrid(t *testing.T) { + grid := gridWith( + columnWidget("colA", "Alpha", ""), + columnWidget("colB", "Beta", ""), + columnWidget("colC", "Gamma", ""), + ) + v := validateDataGrid2ColumnNames(grid, "page X") + if len(v) != 1 { + t.Fatalf("violations = %d, want 1 for a three-column grid", len(v)) + } + for _, want := range []string{"colA → Alpha", "colB → Beta", "colC → Gamma", "dg1"} { + if !strings.Contains(v[0].Message, want) { + t.Errorf("message does not mention %s:\n%s", want, v[0].Message) + } + } +} diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 9e96ff01d..c757de93a 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -37,6 +37,8 @@ func ValidateMicroflow(stmt *ast.CreateMicroflowStmt) []linter.Violation { p.Type.EntityRef.Name, p.Type.EntityRef.Name)) } } + v.params = stmt.Parameters + v.excluded = stmt.Excluded v.validate(stmt.Body) return v.violations } @@ -51,6 +53,12 @@ type microflowValidator struct { // varKinds maps in-scope variable names (params + declared) to their kind, // used to detect assigning a Decimal expression to an Integer/Long target. varKinds map[string]exprcheck.TypeKind + // params is the microflow's parameter list. A parameter occupies the same + // flat variable namespace as every activity output (MDL063). + params []ast.MicroflowParam + // excluded marks an @excluded document. mxbuild does not check one, so the + // #893 rules stand down for it — see skipCEGapRules. + excluded bool } func (v *microflowValidator) addViolation(ruleID string, severity linter.Severity, message, suggestion string) { @@ -101,6 +109,11 @@ func (v *microflowValidator) validate(body []ast.MicroflowStatement) { // variables are only VISIBLE inside its body, so using one after the loop // is CE0108. v.checkLoopScoping(body) + + // #893: three constructs that passed check and exec and were then rejected + // by the build. See validate_microflow_ce_gaps.go for the measurements. + v.checkReturnInLoop(body) + v.checkDuplicateVariableNames(v.params, body) } // checkDuplicateLoopVariables flags a loop iterator name used by more than one @@ -253,6 +266,8 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkNumericAssignment("$"+stmt.Variable, k, stmt.InitialValue) } } + // #893 item 1: a Create Variable activity requires a value (CE0038). + v.checkDeclareHasValue(stmt) v.checkExprFunctions(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) v.checkDivisionSlash(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) v.checkDateTimeLiterals(fmt.Sprintf("declare '$%s'", stmt.Variable), stmt.InitialValue) diff --git a/mdl/executor/validate_microflow_ce_gaps.go b/mdl/executor/validate_microflow_ce_gaps.go new file mode 100644 index 000000000..7b9062834 --- /dev/null +++ b/mdl/executor/validate_microflow_ce_gaps.go @@ -0,0 +1,375 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "reflect" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/exprcheck" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// Three constructs from upstream #893 that parsed, passed `mxcli check`, were +// written by `exec`, and were then rejected by the build. Each was reproduced on +// mxbuild 11.6.6 against a blank app with a 1-error baseline, and each fix +// suggested below was measured to take that app back to the baseline: +// +// MDL061 declare with no value CE0038 fix: `= ''` +// MDL062 return inside a loop CE0068 fix: `break` +// MDL063 duplicate variable name CE0111 fix: drop the declare +// +// Error severity is enough to close the gap the issue was filed about: exec runs +// a pre-flight check over the whole script and refuses with nothing written, so +// "green check, green exec, red Errors pane" no longer happens. Verified end to +// end on the issue's own reproduction — 3 errors reported, 0 microflows written. +// +// They are deliberately NOT added to execEnforcedMicroflowRules, which would +// make `--no-check` refuse them too. That list's bar is a claim verified against +// a real mxbuild, which these meet; the reason to stay off it is different. +// Unlike the XPath rules there, these three predict what the BUILDER emits, and +// building this change turned up four shapes where the AST says "broken" and the +// build says otherwise — two of them in mxcli's own shipped examples. Each is +// exempted and tested below, but a rule with that failure mode should leave the +// author an escape hatch rather than become an unconditional write barrier. + +// skipCEGapRules reports whether the three rules stand down for this microflow. +// +// An @excluded document is not part of the build, and mxbuild does not check it: +// measured on 11.6.6, the very microflow that is CE0111 when included produces +// no error at all when excluded. #312 made `check --references` skip excluded +// microflows for that reason — agentic workflows deliberately stash broken +// intermediate state in excluded scaffolding — and error-severity rules block +// `exec`, so flagging them here would undo that fix by another route. +func (v *microflowValidator) skipCEGapRules() bool { return v.excluded } + +// checkDeclareHasValue flags a `declare` with no initial value — MDL061. +// +// A declare maps to a Create Variable activity, whose Value property Mendix +// requires: CE0038 "The 'Value' property is required." There is no +// uninitialized variable in a microflow, so this is not a defaultable omission; +// mxcli cannot pick the value without inventing semantics (0? empty? for a +// DateTime, what?), and the author writing the value is a one-token fix. +// +// A list or object declare is left to MDL040/MDL043, which reject the TYPE. On +// those, "supply a value" points at the wrong fix — the activity cannot hold +// the type at all, so adding `= empty` does not help. +func (v *microflowValidator) checkDeclareHasValue(stmt *ast.DeclareStmt) { + if v.skipCEGapRules() || stmt.InitialValue != nil { + return + } + if stmt.Type.Kind == ast.TypeListOf { + return // MDL040 + } + if stmt.Type.Kind == ast.TypeEntity || + (stmt.Type.Kind == ast.TypeEnumeration && stmt.Type.EnumRef != nil && !stmt.Type.ExplicitEnum) { + return // MDL043 + } + + v.addViolation("MDL061", linter.SeverityError, + fmt.Sprintf("declare '$%s' has no initial value, but the Create Variable activity it builds "+ + "requires one — mxbuild rejects it with CE0038 \"The 'Value' property is required.\". "+ + "Mendix has no uninitialized variable: every variable is created with a value.", + stmt.Variable), + fmt.Sprintf("Give it a starting value, e.g. declare $%s %s = %s", + stmt.Variable, declaredTypeLabel(stmt.Type), zeroValueHint(stmt.Type))) +} + +// declaredTypeLabel renders a declare's type the way the statement spells it, +// for use in the suggested replacement line. +func declaredTypeLabel(t ast.DataType) string { + if t.Kind == ast.TypeEnumeration && t.EnumRef != nil { + return "Enumeration(" + t.EnumRef.String() + ")" + } + return t.Kind.String() +} + +// zeroValueHint is the value the suggestion offers as a starting point. It is +// advice printed in a message, never something mxcli writes on the author's +// behalf — picking the value is the author's call, which is the whole reason +// MDL061 is a refusal rather than a silent default. +func zeroValueHint(t ast.DataType) string { + switch t.Kind { + case ast.TypeString: + return "''" + case ast.TypeInteger, ast.TypeLong: + return "0" + case ast.TypeDecimal: + return "0.0" + case ast.TypeBoolean: + return "false" + default: + return "empty" + } +} + +// checkReturnInLoop flags a `return` anywhere inside a loop — MDL062. +// +// A return builds an End event, and Mendix forbids one inside a LoopedActivity: +// CE0068 "End events cannot be placed inside a loop." Nesting does not help — a +// return inside a branch inside the loop is still inside the loop — so the walk +// tracks loop depth rather than looking only at the loop's immediate body. +// +// `break` is the construct that replaces it: it leaves the loop, and execution +// continues after it. Returning a value from inside a loop needs the value +// stashed in a variable and a single return after the loop. +// +// The rule predicts what the BUILDER emits, not what the MDL looks like, and +// two forms measured clean on mxbuild 11.6.6 are therefore exempt. Both were +// found by running the rule over the shipped examples before wiring it up: +// +// - `while true` is built as an ExclusiveMerge back-edge, not a +// LoopedActivity (#350). With no loop object there is no "inside a loop", +// and the return is an ordinary End event. A plain `while ` IS a +// LoopedActivity and is not exempt — measured separately. +// - `returns T as $Var` makes buildFlowGraph synthesize the End event from +// the variable, and no End event lands inside the loop. Firing here would +// name the wrong defect: that shape builds CE0109 ("Undefined variable") +// instead, which is a separate gap and not what this rule is about. +func (v *microflowValidator) checkReturnInLoop(body []ast.MicroflowStatement) { + if v.skipCEGapRules() { + return + } + // See the AS-clause note above: the builder routes the return elsewhere. + if v.returnType != nil && v.returnType.Variable != "" { + return + } + + var walk func(stmts []ast.MicroflowStatement, depth int) + walk = func(stmts []ast.MicroflowStatement, depth int) { + for _, s := range stmts { + switch st := s.(type) { + case *ast.ReturnStmt: + if depth > 0 { + v.addViolation("MDL062", linter.SeverityError, + "return inside a loop builds an End event inside the loop, which Mendix does "+ + "not allow — mxbuild rejects it with CE0068 \"End events cannot be placed "+ + "inside a loop.\"", + "Use break to leave the loop; to return a value, assign it to a variable "+ + "inside the loop and put a single return after the loop") + } + case *ast.LoopStmt: + walk(st.Body, depth+1) + case *ast.WhileStmt: + inner := depth + 1 + if isUnconditionalTrueWhile(st) { + inner = depth // an ExclusiveMerge back-edge, not a loop object + } + walk(st.Body, inner) + case *ast.IfStmt: + walk(st.ThenBody, depth) + walk(st.ElseBody, depth) + case *ast.EnumSplitStmt: + for _, c := range st.Cases { + walk(c.Body, depth) + } + walk(st.ElseBody, depth) + case *ast.InheritanceSplitStmt: + for _, c := range st.Cases { + walk(c.Body, depth) + } + walk(st.ElseBody, depth) + } + } + } + walk(body, 0) +} + +// buildsAsAProducer reports whether a statement that LOOKS like a producer in +// the AST actually builds as one. +// +// `set $Match = contains($Hay, $Needle)` on two Strings parses as a +// ListOperationStmt — the visitor cannot tell a string function from a list +// operation at parse time — but addListOperationAction rewrites it into a +// Change Variable when the input is a declared String, precisely because a list +// operation would create its output variable and collide with the declare +// (ledger findings #53/#63, whose examples exist to pin that rewrite). +// +// MDL063 must not report the CE0111 that rewrite exists to prevent: both +// examples build at 0 errors above baseline on mxbuild 11.6.6, and an earlier +// draft of this rule flagged them. The operation test is shared with the +// builder (stringOverloadedListOp) so the two cannot drift apart silently. +func (v *microflowValidator) buildsAsAProducer(s ast.MicroflowStatement) bool { + lo, ok := s.(*ast.ListOperationStmt) + if !ok { + return true + } + return !(stringOverloadedListOp(lo.Operation) && v.varKinds[lo.InputVariable] == exprcheck.KindString) +} + +// stringOverloadedListOp reports whether a list operation shares its name with a +// String function, so the same MDL spells both. Read by the builder, which +// rewrites these into a Change Variable, and by MDL063, which must stay silent +// on exactly the statements that rewrite covers. +func stringOverloadedListOp(op ast.ListOperationType) bool { + return op == ast.ListOpContains || op == ast.ListOpFind +} + +// producedVar is one definition of a variable name within a microflow. +type producedVar struct { + name string + label string // how the definition is described to the author + loop bool // the definition is a loop iterator (MDL052's territory) +} + +// checkDuplicateVariableNames flags a name defined more than once — MDL063. +// +// A microflow's variable namespace is FLAT. Measured on mxbuild 11.6.6, each of +// these is CE0111 "Duplicate variable name", isolated one microflow at a time: +// two declares in one body; a declare outside a loop and another inside it; a +// declare in each of two SIBLING if/else branches; two retrieves; a parameter +// and a declare; a loop iterator and a declare. Neither branches nor loop bodies +// open a scope, so the walk deliberately does not track one. +// +// The distinction that matters is create versus assign: `$X = 'b'` after +// `declare $X` is a Change Variable, which defines nothing and measured clean. +// A rule that keyed on "the name appears on the left of `=`" would flag the +// normal way to update a variable. +// +// Iterator-versus-iterator is left to MDL052, whose message explains loop +// scoping; two rules on one line is noise. +func (v *microflowValidator) checkDuplicateVariableNames(params []ast.MicroflowParam, body []ast.MicroflowStatement) { + if v.skipCEGapRules() { + return + } + first := map[string]producedVar{} + + report := func(p producedVar) { + prev, seen := first[p.name] + if !seen { + first[p.name] = p + return + } + if prev.loop && p.loop { + return // MDL052 + } + v.addViolation("MDL063", linter.SeverityError, + fmt.Sprintf("'$%s' is created twice in this microflow — first by %s, then by %s. "+ + "A microflow's variable names are unique flow-wide (branches and loop bodies do "+ + "not open a scope), so mxbuild rejects this with CE0111 \"Duplicate variable name\".", + p.name, prev.label, p.label), + fmt.Sprintf("Rename one of them, or — if you meant to reuse the first — drop the "+ + "redundant definition: %s already creates '$%s', and an activity always creates "+ + "its own output variable rather than writing into an existing one", + prev.label, p.name)) + } + + for _, p := range params { + if p.Name != "" { + report(producedVar{name: p.Name, label: "the microflow parameter"}) + } + } + + var walk func(stmts []ast.MicroflowStatement) + walk = func(stmts []ast.MicroflowStatement) { + for _, s := range stmts { + if v.buildsAsAProducer(s) { + for _, p := range statementProducedVars(s) { + report(p) + } + } + switch st := s.(type) { + case *ast.LoopStmt: + walk(st.Body) + case *ast.WhileStmt: + walk(st.Body) + case *ast.IfStmt: + walk(st.ThenBody) + walk(st.ElseBody) + case *ast.EnumSplitStmt: + for _, c := range st.Cases { + walk(c.Body) + } + walk(st.ElseBody) + case *ast.InheritanceSplitStmt: + for _, c := range st.Cases { + walk(c.Body) + } + walk(st.ElseBody) + } + } + } + walk(body) +} + +// statementProducedVars returns the variables a statement CREATES. +// +// The `OutputVariable` field is read by reflection rather than from a switch: +// fifteen statement types carry it, the name is unambiguous — a field called +// OutputVariable is always a producer — and a statement type added later is +// covered without anyone remembering to extend a list. The alternative, a +// hand-maintained type switch, is exactly the shape of the blind spot that let +// #892's DROP FOLDER guard miss five document kinds. +// +// Reflection is NOT used for the `Variable` field, which is ambiguous: it names +// the produced variable on declare/create/retrieve/create list, and a CONSUMED +// one on change/commit/delete/rollback and on the split statements. Those five +// producers are therefore listed explicitly. +func statementProducedVars(s ast.MicroflowStatement) []producedVar { + var out []producedVar + + switch st := s.(type) { + case *ast.DeclareStmt: + out = append(out, producedVar{name: st.Variable, label: "declare"}) + case *ast.CreateObjectStmt: + out = append(out, producedVar{name: st.Variable, label: "create"}) + case *ast.CreateListStmt: + out = append(out, producedVar{name: st.Variable, label: "create list"}) + case *ast.RetrieveStmt: + out = append(out, producedVar{name: st.Variable, label: "retrieve"}) + case *ast.LoopStmt: + out = append(out, producedVar{name: st.LoopVariable, label: "the loop iterator", loop: true}) + } + + if name := outputVariableField(s); name != "" { + out = append(out, producedVar{name: name, label: statementProducerLabel(s)}) + } + + // A producer with an empty name is an optional output the author left off. + kept := out[:0] + for _, p := range out { + if p.name != "" { + kept = append(kept, p) + } + } + return kept +} + +// outputVariableField reads a statement's OutputVariable field, if it has one. +func outputVariableField(s ast.MicroflowStatement) string { + rv := reflect.ValueOf(s) + if rv.Kind() == reflect.Ptr { + if rv.IsNil() { + return "" + } + rv = rv.Elem() + } + if rv.Kind() != reflect.Struct { + return "" + } + f := rv.FieldByName("OutputVariable") + if !f.IsValid() || f.Kind() != reflect.String { + return "" + } + return f.String() +} + +// statementProducerLabel describes an activity in the author's vocabulary. +// stmtActivityName covers the common ones; anything it does not know falls back +// to the AST type name rather than the generic "Activity", so a statement type +// added later still names itself in the message. +func statementProducerLabel(s ast.MicroflowStatement) string { + if name := stmtActivityName(s); name != "Activity" { + return name + } + t := reflect.TypeOf(s) + if t != nil && t.Kind() == reflect.Ptr { + t = t.Elem() + } + if t == nil { + return "an activity" + } + return "the " + strings.TrimSuffix(t.Name(), "Stmt") + " activity" +} diff --git a/mdl/executor/validate_microflow_ce_gaps_test.go b/mdl/executor/validate_microflow_ce_gaps_test.go new file mode 100644 index 000000000..3fe9e52ac --- /dev/null +++ b/mdl/executor/validate_microflow_ce_gaps_test.go @@ -0,0 +1,457 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// violationsByRule returns the set of rule IDs present in a violation slice. +func violationsByRule(vs []linter.Violation) map[string]linter.Violation { + out := map[string]linter.Violation{} + for _, v := range vs { + if _, seen := out[v.RuleID]; !seen { + out[v.RuleID] = v + } + } + return out +} + +// checkMicroflowSource parses one CREATE MICROFLOW statement and validates it. +func checkMicroflowSource(t *testing.T, src string) []linter.Violation { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + stmt, ok := prog.Statements[0].(*ast.CreateMicroflowStmt) + if !ok { + t.Fatalf("statement 0 is %T, want *ast.CreateMicroflowStmt", prog.Statements[0]) + } + return ValidateMicroflow(stmt) +} + +// upstream #893 item 1. `declare $X String;` with no initial value parsed, passed +// `mxcli check` and was written by `exec`; mxbuild then rejected the Create +// Variable activity with CE0038 "The 'Value' property is required." +// +// Measured on mxbuild 11.6.6 against a blank app whose baseline is 1 error: +// the probe microflow took it to 2, and supplying `= ”` took it back to 1. +func TestMDL061_DeclareWithoutValue(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_BareDeclare () +begin + declare $X String; + log info node 'x' 'y'; +end;`) + + got, ok := violationsByRule(vs)["MDL061"] + if !ok { + t.Fatalf("expected MDL061 for a value-less declare, got %#v", vs) + } + if got.Severity != linter.SeverityError { + t.Errorf("MDL061 severity = %v, want Error (mxbuild rejects it with CE0038)", got.Severity) + } + if !strings.Contains(got.Message, "X") { + t.Errorf("MDL061 message should name the variable, got %q", got.Message) + } + // The suggestion must be actionable: the fix is a value, and `= ''` was + // verified to build clean. + if !strings.Contains(got.Suggestion, "=") { + t.Errorf("MDL061 suggestion should show the initializer form, got %q", got.Suggestion) + } +} + +// The control for MDL061: an initialized primitive declare is exactly what the +// fix advice tells the user to write, so it must not be flagged — otherwise the +// rule has no escape and the syntax becomes unusable rather than fixable. +func TestMDL061_InitializedDeclareIsClean(t *testing.T) { + for _, src := range []string{ + `create microflow Synthetic.MF_S () begin declare $X String = ''; end;`, + `create microflow Synthetic.MF_I () begin declare $N Integer = 0; end;`, + `create microflow Synthetic.MF_B () begin declare $B Boolean = false; end;`, + } { + vs := checkMicroflowSource(t, src) + if _, bad := violationsByRule(vs)["MDL061"]; bad { + t.Errorf("MDL061 fired on an initialized declare: %s", src) + } + } +} + +// A list or object declare is already rejected by MDL040/MDL043, which say the +// activity cannot hold that type at all. Adding "and it needs a value" on top is +// noise pointing at the wrong fix — the user must not add `= empty`, they must +// stop declaring it. +func TestMDL061_DefersToTheTypeRules(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_List () +begin + declare $Items list of Synthetic.Item; +end;`) + ids := violationsByRule(vs) + if _, ok := ids["MDL040"]; !ok { + t.Fatalf("expected MDL040 for a list declare, got %#v", vs) + } + if _, bad := ids["MDL061"]; bad { + t.Error("MDL061 must not pile onto a list declare — MDL040 already says the activity cannot hold a list") + } +} + +// upstream #893 item 2. A `return` inside a loop builds an End event inside the +// LoopedActivity, which Mendix forbids: CE0068 "End events cannot be placed +// inside a loop." Measured on mxbuild 11.6.6; replacing it with `break` took the +// same app back to its 1-error baseline. +func TestMDL062_ReturnInsideLoop(t *testing.T) { + cases := []struct { + name string + src string + }{ + { + name: "directly in a for-loop body (the reported case)", + src: `create microflow Synthetic.MF_LoopReturn () +begin + retrieve $PartList from Synthetic.Part; + loop $Part in $PartList + begin + return; + end loop; +end;`, + }, + { + name: "in a while body", + src: `create microflow Synthetic.MF_WhileReturn () +begin + declare $Go Boolean = true; + while $Go + begin + return; + end while; +end;`, + }, + { + name: "nested inside a branch inside the loop — still inside the loop", + src: `create microflow Synthetic.MF_LoopIfReturn () +begin + retrieve $PartList from Synthetic.Part; + loop $Part in $PartList + begin + if 1 = 1 then + return; + end if; + end loop; +end;`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, ok := violationsByRule(checkMicroflowSource(t, tc.src))["MDL062"] + if !ok { + t.Fatal("expected MDL062 for a return inside a loop") + } + if got.Severity != linter.SeverityError { + t.Errorf("MDL062 severity = %v, want Error (mxbuild rejects it with CE0068)", got.Severity) + } + if !strings.Contains(got.Suggestion, "break") { + t.Errorf("MDL062 should point at `break`, the construct that replaces it: %q", got.Suggestion) + } + }) + } +} + +// The control for MDL062: a return AFTER the loop is the normal shape and must +// stay clean, or the rule makes every loop-bearing microflow unwritable. +func TestMDL062_ReturnAfterLoopIsClean(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_ReturnAfter () returns Boolean +begin + retrieve $PartList from Synthetic.Part; + loop $Part in $PartList + begin + break; + end loop; + return true; +end;`) + if _, bad := violationsByRule(vs)["MDL062"]; bad { + t.Errorf("MDL062 fired on a return placed after the loop: %#v", vs) + } +} + +// upstream #893 item 6, generalised to the rule mxbuild actually enforces. +// +// A microflow's variable namespace is FLAT: branches and loops do not scope it. +// Measured on mxbuild 11.6.6, each of these is CE0111 "Duplicate variable name" +// and each was isolated in its own microflow to attribute the error: +// +// declare $X + `$X = call microflow …` (the reported case) +// declare $X + declare $X same body +// declare $X + declare $X one outside a loop, one inside it +// declare $X + declare $X in two SIBLING if/else branches +// retrieve $L + retrieve $L +// parameter Name + declare $Name +// loop iterator $I + declare $I +func TestMDL063_DuplicateVariableNames(t *testing.T) { + cases := []struct { + name string + src string + }{ + { + name: "declare then an activity output of the same name (the reported case)", + src: `create microflow Synthetic.MF_Shadow () +begin + declare $Session String = ''; + $Session = call microflow Synthetic.Helper(); +end;`, + }, + { + name: "two declares in the same body", + src: `create microflow Synthetic.MF_TwoDeclares () +begin + declare $X String = 'a'; + declare $X String = 'b'; +end;`, + }, + { + name: "a loop body does not open a new scope", + src: `create microflow Synthetic.MF_LoopScope () +begin + declare $X String = 'a'; + retrieve $L from Synthetic.Item; + loop $I in $L + begin + declare $X String = 'b'; + end loop; +end;`, + }, + { + name: "sibling branches do not isolate either", + src: `create microflow Synthetic.MF_Branches () +begin + if 1 = 1 then + declare $X String = 'a'; + else + declare $X String = 'b'; + end if; +end;`, + }, + { + name: "two retrieves into the same name", + src: `create microflow Synthetic.MF_TwoRetrieves () +begin + retrieve $L from Synthetic.Item; + retrieve $L from Synthetic.Item; +end;`, + }, + { + name: "a parameter is a variable too", + src: `create microflow Synthetic.MF_ParamClash (Name: String) +begin + declare $Name String = 'x'; +end;`, + }, + { + name: "so is a loop iterator", + src: `create microflow Synthetic.MF_IterClash () +begin + retrieve $L from Synthetic.Item; + loop $I in $L + begin + declare $I String = 'x'; + end loop; +end;`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, ok := violationsByRule(checkMicroflowSource(t, tc.src))["MDL063"] + if !ok { + t.Fatal("expected MDL063 for a duplicated variable name") + } + if got.Severity != linter.SeverityError { + t.Errorf("MDL063 severity = %v, want Error (mxbuild rejects it with CE0111)", got.Severity) + } + }) + } +} + +// The control for MDL063, and the distinction the rule turns on: assigning to an +// existing variable is a Change Variable activity, which creates nothing and is +// exactly how a declared variable is meant to be updated. Measured clean on +// mxbuild 11.6.6. Getting this wrong would flag the normal idiom. +func TestMDL063_AssignmentToADeclaredVariableIsClean(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_Assign () +begin + declare $X String = 'a'; + $X = 'b'; + $X = 'c'; +end;`) + if _, bad := violationsByRule(vs)["MDL063"]; bad { + t.Errorf("MDL063 fired on a plain reassignment, which is a Change Variable and always valid: %#v", vs) + } +} + +// Distinct names in every producing position must stay clean — the rule keys on +// the NAME, so a bug that keyed on the statement kind instead would fire here. +func TestMDL063_DistinctNamesAreClean(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_Distinct (In: String) +begin + declare $A String = 'a'; + retrieve $L from Synthetic.Item; + loop $I in $L + begin + declare $B String = 'b'; + end loop; + $C = call microflow Synthetic.Helper(); +end;`) + if _, bad := violationsByRule(vs)["MDL063"]; bad { + t.Errorf("MDL063 fired although every producer has a distinct name: %#v", vs) + } +} + +// MDL052 already owns iterator-vs-iterator, with a message about loop scoping +// that MDL063's generic wording would not improve on. Two rules firing on one +// line is noise, so MDL063 stands aside for that pair only. +func TestMDL063_LeavesIteratorPairsToMDL052(t *testing.T) { + ids := violationsByRule(checkMicroflowSource(t, `create microflow Synthetic.MF_TwoLoops () +begin + retrieve $L from Synthetic.Item; + loop $R in $L + begin + log info node 'x' 'y'; + end loop; + loop $R in $L + begin + log info node 'x' 'y'; + end loop; +end;`)) + if _, ok := ids["MDL052"]; !ok { + t.Fatal("expected MDL052 for two loops sharing an iterator name") + } + if _, bad := ids["MDL063"]; bad { + t.Error("MDL063 must not double-report what MDL052 already covers") + } +} + +// The exemptions below are not judgement calls — each is a shape that mxbuild +// 11.6.6 accepts, found by running the rules over the shipped examples before +// wiring them in. Without them these rules would refuse MDL that builds clean, +// which is the failure mode execEnforcedMicroflowRules warns about with MDL009. + +// #350's manual retry loop. `while true` is built as an ExclusiveMerge back-edge +// rather than a LoopedActivity, so there is no loop object for the End event to +// be inside. Measured: 0 errors above baseline, no CE0068. +func TestMDL062_ExemptsWhileTrue(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_ManualRetry (Flag: Boolean) +begin + while true + begin + if $Flag then + return; + end if; + continue; + end while; +end;`) + if _, bad := violationsByRule(vs)["MDL062"]; bad { + t.Errorf("MDL062 fired on a `while true` manual loop, which builds no LoopedActivity: %#v", vs) + } +} + +// ... but a `while` with a real condition IS a LoopedActivity. Measured CE0068, +// so the exemption must be keyed on the literal `true`, not on `while`. +func TestMDL062_WhileWithConditionIsNotExempt(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_CondWhile (Flag: Boolean) +begin + while $Flag + begin + return; + end while; +end;`) + if _, ok := violationsByRule(vs)["MDL062"]; !ok { + t.Errorf("MDL062 must still fire for `while ` — measured CE0068 on mxbuild: %#v", vs) + } +} + +// With `returns T as $Var` the builder takes the End event's value from the +// variable and none lands inside the loop. Measured: no CE0068 (that shape +// builds CE0109 instead, a different defect this rule must not mislabel). +func TestMDL062_ExemptsReturnsAsClause(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_AsClause () returns Boolean as $Done +begin + retrieve $L from Synthetic.Item; + loop $I in $L + begin + return true; + end loop; +end;`) + if _, bad := violationsByRule(vs)["MDL062"]; bad { + t.Errorf("MDL062 fired on a `returns … as $Var` microflow, where no End event lands in the loop: %#v", vs) + } +} + +// `set $Match = contains($Hay, $Needle)` on two Strings parses as a +// ListOperationStmt, but addListOperationAction rewrites it to a Change +// Variable exactly to avoid this CE0111 (ledger #53/#63). Both examples build +// at baseline; an earlier draft of MDL063 flagged them. +func TestMDL063_ExemptsStringOverloadedListOps(t *testing.T) { + for _, src := range []string{ + `create microflow Synthetic.MF_C (Hay: String, Needle: String) returns Boolean +begin + declare $Match Boolean = false; + set $Match = contains($Hay, $Needle); + return $Match; +end;`, + `create microflow Synthetic.MF_F (Raw: String, Needle: String) returns Integer +begin + declare $At Integer = 0; + set $At = find($Raw, $Needle); + return $At; +end;`, + } { + vs := checkMicroflowSource(t, src) + if _, bad := violationsByRule(vs)["MDL063"]; bad { + t.Errorf("MDL063 fired on a string function the builder rewrites to a Change Variable: %s\n%#v", src, vs) + } + } +} + +// A genuine list operation still creates its output variable, so a declare in +// front of one is still CE0111 — the exemption is keyed on the input being a +// declared String, not on the operation name alone. +func TestMDL063_GenuineListOperationIsNotExempt(t *testing.T) { + vs := checkMicroflowSource(t, `create microflow Synthetic.MF_ListC (Items: list of Synthetic.Item, One: Synthetic.Item) returns Boolean +begin + declare $Found Boolean = false; + set $Found = contains($Items, $One); + return $Found; +end;`) + if _, ok := violationsByRule(vs)["MDL063"]; !ok { + t.Errorf("MDL063 must still fire for a real list operation feeding a pre-declared variable: %#v", vs) + } +} + +// mxbuild does not check an @excluded document: measured, the microflow that is +// CE0111 when included produces no error at all when excluded. Because these +// rules are error-severity they block `exec`, so flagging excluded scaffolding +// would undo #312 by another route. +func TestCEGapRules_SkipExcludedMicroflows(t *testing.T) { + vs := checkMicroflowSource(t, `@excluded +create microflow Synthetic.MF_Excluded () returns String as $msg +begin + declare $X String; + declare $msg String = ''; + $msg = call microflow Synthetic.NoSuchTarget(); + retrieve $L from Synthetic.Item; + loop $I in $L + begin + return $msg; + end loop; +end;`) + for _, id := range []string{"MDL061", "MDL062", "MDL063"} { + if _, bad := violationsByRule(vs)[id]; bad { + t.Errorf("%s fired on an @excluded microflow, which mxbuild never checks: %#v", id, vs) + } + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index d0f74a52e..588c6b540 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -25,6 +25,7 @@ import ( func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // Statement-level checks that need no project connection. var violations []linter.Violation + securityEnabled := programEnablesSecurity(prog) for _, stmt := range prog.Statements { // Check enumeration values for reserved words if enumStmt, ok := stmt.(*ast.CreateEnumerationStmt); ok { @@ -38,6 +39,15 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { if alterStmt, ok := stmt.(*ast.AlterEntityStmt); ok { violations = append(violations, ValidateAlterEntity(alterStmt)...) } + // A user role with no System module role cannot sign in (CE0156) — but + // only once security is on, which the script may say itself. + if roleStmt, ok := stmt.(*ast.CreateUserRoleStmt); ok { + violations = append(violations, ValidateUserRoleSystemModuleRole(roleStmt, securityEnabled)...) + } + // A page with parameters and a Url must name each parameter in it (CE5601). + if pageStmt, ok := stmt.(*ast.CreatePageStmtV3); ok { + violations = append(violations, ValidatePageURLParameters(pageStmt)...) + } // Check microflow body for common issues if mfStmt, ok := stmt.(*ast.CreateMicroflowStmt); ok { violations = append(violations, ValidateMicroflow(mfStmt)...) diff --git a/mdl/executor/validate_role_and_url.go b/mdl/executor/validate_role_and_url.go new file mode 100644 index 000000000..fa39a3a2e --- /dev/null +++ b/mdl/executor/validate_role_and_url.go @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Two statically decidable rules that MxBuild reports and `mxcli check` did not. +// +// Both were found by a real project (mxcli-dbreplication, finding F10): four MDL +// scripts passed `mxcli check` with 0 errors and executed cleanly, and the first +// real validation then failed with three errors, two of them these. Neither is +// exotic, and neither needs the project — the MDL alone says everything. +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// systemModuleName is the module whose roles make a user role able to sign in. +const systemModuleName = "System" + +// ValidateUserRoleSystemModuleRole reports MDL-SEC20 (MxBuild CE0156) for a user +// role built only from application module roles. +// +// A user role with no System module role cannot sign in or touch System +// entities, so the app is unusable for anyone holding only that role. +// +// **It is an error only when security is on.** Measured on Mendix 11.13: the +// same role is CE0156 at security level Prototype and *no error at all* at +// level Off, where roles are stored but not validated. A blank project ships +// Off, so reporting this as an error unconditionally fails scripts that are +// perfectly valid for the project they target — it broke two of mxcli's own +// examples, which is how the over-reach was caught. +// +// So: an error when the script itself turns security on, because then the author +// has said which world they are in; a warning otherwise, since the script may +// well be applied to a project that has it on and the advice still holds. +func ValidateUserRoleSystemModuleRole(stmt *ast.CreateUserRoleStmt, securityEnabled bool) []linter.Violation { + if stmt == nil || len(stmt.ModuleRoles) == 0 { + // A role with no module roles at all is a different (and legitimate) + // thing — a placeholder to be extended later by ALTER USER ROLE. + return nil + } + for _, r := range stmt.ModuleRoles { + if strings.EqualFold(r.Module, systemModuleName) { + return nil + } + } + severity := linter.SeverityWarning + when := "if this project has security enabled, " + if securityEnabled { + severity = linter.SeverityError + when = "this script enables security, so " + } + return []linter.Violation{{ + RuleID: "MDL-SEC20", + Severity: severity, + Message: fmt.Sprintf( + "user role %q has no System module role — %snobody holding it can sign in or read "+ + "System entities (MxBuild reports this as CE0156 once security is on; at security "+ + "level Off it is not flagged). Add System.User: CREATE USER ROLE %s (%s, System.User)", + stmt.Name, when, stmt.Name, joinQualified(stmt.ModuleRoles)), + }} +} + +// programEnablesSecurity reports whether the script sets a project security +// level other than Off — the condition that makes CE0156 real. +func programEnablesSecurity(prog *ast.Program) bool { + for _, stmt := range prog.Statements { + s, ok := stmt.(*ast.AlterProjectSecurityStmt) + if !ok { + continue + } + if s.SecurityLevel != "" && !strings.EqualFold(s.SecurityLevel, "Off") { + return true + } + } + return false +} + +func joinQualified(names []ast.QualifiedName) string { + parts := make([]string, len(names)) + for i, n := range names { + parts[i] = n.String() + } + return strings.Join(parts, ", ") +} + +// ValidatePageURLParameters reports MDL-PAGE20 (MxBuild CE5601) when a page has +// both parameters and a URL, and the URL does not name every parameter. +// +// Mendix builds a deep link from the URL, so each parameter needs a `{Name}` +// segment to be bound from it. Without one the page cannot be opened by URL and +// the build fails. The check runs only when a URL is set: a page with parameters +// and no URL is perfectly normal. +func ValidatePageURLParameters(stmt *ast.CreatePageStmtV3) []linter.Violation { + if stmt == nil || stmt.URL == "" || len(stmt.Parameters) == 0 { + return nil + } + var missing []string + for _, p := range stmt.Parameters { + if p.Name == "" { + continue + } + if !urlBindsParameter(stmt.URL, p.Name) { + missing = append(missing, p.Name) + } + } + if len(missing) == 0 { + return nil + } + suggested := stmt.URL + for _, name := range missing { + suggested = strings.TrimRight(suggested, "/") + "/{" + name + "}" + } + return []linter.Violation{{ + RuleID: "MDL-PAGE20", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "page %s has a Url but no segment for parameter %s — Mendix binds each page "+ + "parameter from the URL, so the page cannot be opened by link and the build "+ + "fails (MxBuild reports this as CE5601). Try Url: '%s'", + stmt.Name.String(), quoteList(missing), suggested), + }} +} + +// urlBindsParameter reports whether the URL has a segment binding this +// parameter. Mendix allows an attribute path inside the segment — the common +// form is `{Customer/Name}`, which binds the Customer parameter by one of its +// attributes — so matching `{Name}` exactly would flag correct URLs. The match +// is on the segment's leading identifier. +func urlBindsParameter(url, param string) bool { + rest := url + for { + open := strings.Index(rest, "{") + if open < 0 { + return false + } + rest = rest[open+1:] + close := strings.Index(rest, "}") + if close < 0 { + return false + } + seg := rest[:close] + rest = rest[close+1:] + if head, _, _ := strings.Cut(seg, "/"); strings.EqualFold(strings.TrimPrefix(head, "$"), param) { + return true + } + } +} + +func quoteList(names []string) string { + parts := make([]string, len(names)) + for i, n := range names { + parts[i] = fmt.Sprintf("%q", n) + } + if len(parts) == 1 { + return parts[0] + } + return strings.Join(parts[:len(parts)-1], ", ") + " and " + parts[len(parts)-1] +} diff --git a/mdl/executor/validate_role_and_url_test.go b/mdl/executor/validate_role_and_url_test.go new file mode 100644 index 000000000..767964c1d --- /dev/null +++ b/mdl/executor/validate_role_and_url_test.go @@ -0,0 +1,218 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +func qn(module, name string) ast.QualifiedName { + return ast.QualifiedName{Module: module, Name: name} +} + +// TestUserRoleWithoutSystemRoleIsAnError is the regression test for CE0156. A +// user role built only from application module roles cannot sign in or read +// System entities, so the app is unusable for anyone holding it — and the whole +// thing is decidable from the MDL. +func TestUserRoleWithoutSystemRoleIsAnError(t *testing.T) { + stmt := &ast.CreateUserRoleStmt{ + Name: "Evaluator", + ModuleRoles: []ast.QualifiedName{qn("ReplicationLab", "Evaluator")}, + } + v := ValidateUserRoleSystemModuleRole(stmt, true) + if len(v) != 1 { + t.Fatalf("violations = %d, want 1", len(v)) + } + if v[0].RuleID != "MDL-SEC20" { + t.Errorf("RuleID = %q", v[0].RuleID) + } + if !strings.Contains(v[0].Message, "System.User") { + t.Errorf("message does not name the fix:\n%s", v[0].Message) + } + if !strings.Contains(v[0].Message, "CE0156") { + t.Errorf("message does not cite the MxBuild code:\n%s", v[0].Message) + } +} + +func TestUserRoleWithSystemRoleIsAccepted(t *testing.T) { + for _, sys := range []string{"System", "system", "SYSTEM"} { + stmt := &ast.CreateUserRoleStmt{ + Name: "Evaluator", + ModuleRoles: []ast.QualifiedName{ + qn("ReplicationLab", "Evaluator"), qn(sys, "User"), + }, + } + if v := ValidateUserRoleSystemModuleRole(stmt, true); len(v) != 0 { + t.Errorf("%s.User rejected: %s", sys, v[0].Message) + } + } +} + +// TestUserRoleWithNoModuleRolesIsNotFlagged — a role declared empty and extended +// later by ALTER USER ROLE is a legitimate shape, not a missing System role. +func TestUserRoleWithNoModuleRolesIsNotFlagged(t *testing.T) { + if v := ValidateUserRoleSystemModuleRole(&ast.CreateUserRoleStmt{Name: "Placeholder"}, true); len(v) != 0 { + t.Errorf("an empty role was flagged: %s", v[0].Message) + } +} + +// TestPageURLMissingParameterSegmentIsAnError is the regression test for CE5601. +// Mendix binds each page parameter from the URL, so one without a {Name} segment +// cannot be opened by link and fails the build. +func TestPageURLMissingParameterSegmentIsAnError(t *testing.T) { + stmt := &ast.CreatePageStmtV3{ + Name: ast.QualifiedName{Module: "Lab", Name: "ScenarioDetail"}, + URL: "scenario", + Parameters: []ast.PageParameter{{Name: "Scenario"}}, + } + v := ValidatePageURLParameters(stmt) + if len(v) != 1 { + t.Fatalf("violations = %d, want 1", len(v)) + } + if v[0].RuleID != "MDL-PAGE20" { + t.Errorf("RuleID = %q", v[0].RuleID) + } + for _, want := range []string{"CE5601", `"Scenario"`, "scenario/{Scenario}"} { + if !strings.Contains(v[0].Message, want) { + t.Errorf("message does not contain %s:\n%s", want, v[0].Message) + } + } +} + +func TestPageURLWithEveryParameterIsAccepted(t *testing.T) { + stmt := &ast.CreatePageStmtV3{ + Name: ast.QualifiedName{Module: "Lab", Name: "P"}, + URL: "run/{Scenario}/{Step}", + Parameters: []ast.PageParameter{{Name: "Scenario"}, {Name: "Step"}}, + } + if v := ValidatePageURLParameters(stmt); len(v) != 0 { + t.Errorf("a complete URL was rejected: %s", v[0].Message) + } +} + +// TestPageWithParametersAndNoURLIsNotFlagged — parameters without a URL are +// entirely normal; the rule only applies once a deep link exists. +func TestPageWithParametersAndNoURLIsNotFlagged(t *testing.T) { + stmt := &ast.CreatePageStmtV3{ + Name: ast.QualifiedName{Module: "Lab", Name: "P"}, + Parameters: []ast.PageParameter{{Name: "Scenario"}}, + } + if v := ValidatePageURLParameters(stmt); len(v) != 0 { + t.Errorf("a URL-less page was flagged: %s", v[0].Message) + } +} + +// TestPageURLNamesEveryMissingParameter — reporting only the first would send +// the author round the loop once per parameter. +func TestPageURLNamesEveryMissingParameter(t *testing.T) { + stmt := &ast.CreatePageStmtV3{ + Name: ast.QualifiedName{Module: "Lab", Name: "P"}, + URL: "run", + Parameters: []ast.PageParameter{{Name: "Scenario"}, {Name: "Step"}}, + } + v := ValidatePageURLParameters(stmt) + if len(v) != 1 { + t.Fatalf("violations = %d, want 1", len(v)) + } + if !strings.Contains(v[0].Message, `"Scenario" and "Step"`) { + t.Errorf("message does not name both:\n%s", v[0].Message) + } + if !strings.Contains(v[0].Message, "run/{Scenario}/{Step}") { + t.Errorf("suggestion does not add both segments:\n%s", v[0].Message) + } +} + +// TestPageURLAcceptsAnAttributePathSegment. Mendix binds a parameter by one of +// its attributes — `url: 'p006_dataform/{Customer/Name}'` is the shape mxcli's +// own page examples use — so matching `{Customer}` exactly would flag correct +// URLs as errors. That false positive is worse than the missing rule was. +func TestPageURLAcceptsAnAttributePathSegment(t *testing.T) { + stmt := &ast.CreatePageStmtV3{ + Name: ast.QualifiedName{Module: "PgTest", Name: "P006_DataForm"}, + URL: "p006_dataform/{Customer/Name}", + Parameters: []ast.PageParameter{{Name: "Customer"}}, + } + if v := ValidatePageURLParameters(stmt); len(v) != 0 { + t.Errorf("an attribute-path URL was flagged: %s", v[0].Message) + } +} + +func TestURLBindsParameterFormats(t *testing.T) { + cases := []struct { + url, param string + want bool + }{ + {"scenario/{Scenario}", "Scenario", true}, + {"p006/{Customer/Name}", "Customer", true}, + {"p006/{$Customer}", "Customer", true}, + {"p006/{customer}", "Customer", true}, + {"a/{Other}/b/{Scenario}", "Scenario", true}, + {"scenario", "Scenario", false}, + {"scenario/{Other}", "Scenario", false}, + {"scenario/{ScenarioExtra}", "Scenario", false}, + {"scenario/{unclosed", "Scenario", false}, + } + for _, c := range cases { + if got := urlBindsParameter(c.url, c.param); got != c.want { + t.Errorf("urlBindsParameter(%q, %q) = %v, want %v", c.url, c.param, got, c.want) + } + } +} + +// TestUserRoleSeverityFollowsSecurityLevel pins the measurement behind MDL-SEC20. +// +// On Mendix 11.13 the same role is CE0156 at security level Prototype and *no +// error at all* at level Off, where roles are stored but not validated. A blank +// project ships Off, so reporting it as an error unconditionally fails scripts +// that are correct for the project they target — it broke two of mxcli's own +// examples in CI, which is how the over-reach was caught. +func TestUserRoleSeverityFollowsSecurityLevel(t *testing.T) { + stmt := &ast.CreateUserRoleStmt{ + Name: "Evaluator", + ModuleRoles: []ast.QualifiedName{qn("Lab", "Evaluator")}, + } + + warn := ValidateUserRoleSystemModuleRole(stmt, false) + if len(warn) != 1 || warn[0].Severity != linter.SeverityWarning { + t.Fatalf("without security enabled: got %d violations, severity %v; want 1 warning", + len(warn), warn[0].Severity) + } + if !strings.Contains(warn[0].Message, "security level Off it is not flagged") { + t.Errorf("warning does not explain when it applies:\n%s", warn[0].Message) + } + + err := ValidateUserRoleSystemModuleRole(stmt, true) + if len(err) != 1 || err[0].Severity != linter.SeverityError { + t.Fatalf("with security enabled: got %d violations, severity %v; want 1 error", + len(err), err[0].Severity) + } +} + +// TestProgramEnablesSecurity — the escalation condition is the script saying so. +func TestProgramEnablesSecurity(t *testing.T) { + cases := []struct { + level string + want bool + }{ + {"PROTOTYPE", true}, + {"Production", true}, + {"OFF", false}, + {"off", false}, + {"", false}, + } + for _, c := range cases { + prog := &ast.Program{Statements: []ast.Statement{ + &ast.AlterProjectSecurityStmt{SecurityLevel: c.level}, + }} + if got := programEnablesSecurity(prog); got != c.want { + t.Errorf("level %q: programEnablesSecurity = %v, want %v", c.level, got, c.want) + } + } + if programEnablesSecurity(&ast.Program{}) { + t.Error("an empty program was read as enabling security") + } +} diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 5a07ab0ba..75133c677 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -132,6 +132,8 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc if mapping != nil { out = append(out, validateObjectListItemEnums(w, mapping, locationPrefix)...) } + // Reported once per grid, not once per column — see the rule's comment. + out = append(out, validateDataGrid2ColumnNames(w, locationPrefix)...) if len(w.Children) > 0 { out = append(out, validateWidgetTreeIn(w.Children, registry, locationPrefix, objectListMappingSet(def))...) } @@ -969,6 +971,21 @@ func validatePluggableWidgetProperties(w *ast.WidgetV3, registry *WidgetRegistry // dedicated path rather than via propertyMappings. Accept them // universally so the validator doesn't false-positive on legitimate // MDL idioms like `Label: 'X'` on widgets whose def.json omits it. + // A builtin name the engine has no route for on *this* widget is worse + // than an unknown one: it is accepted here, dropped on write, and shows + // up as a required-property error from MxBuild with nothing pointing at + // the cause. + if right, wrong := misusedBuiltinProperty(def.WidgetID, key); wrong { + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET17", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` (%s) has no `%s` property — the value is dropped on write and "+ + "MxBuild then reports the property as missing. Use `%s:` instead", + locationPrefix, w.Name, def.MDLName, key, right), + }) + continue + } if isBuiltinPropName(key) { continue } @@ -1267,3 +1284,126 @@ func min3(a, b, c int) int { } return c } + +// validateDataGrid2ColumnNames warns (MDL-WIDGET16) that the names written on a +// pluggable DataGrid 2's columns are discarded, and says what each column will +// actually be addressable as. +// +// Mendix stores no name on a DataGrid 2 column. Its schema has no name or +// identifier key at column level — the only human-facing label is `header`, the +// caption — so the name in `column colLabel (attribute: Label, …)` reaches +// DataGridColumnSpec, which has no field for it, and is dropped. Everything +// downstream then addresses the column by a *derived* name: the bound attribute +// for an attribute column, the sanitized caption otherwise, `colN` as a last +// resort. +// +// The consequence is not obvious from the MDL. An author who wrote `colLabel` +// reaches for `ALTER PAGE … ON dg1.colLabel` and gets "column not found" for a +// column they just named, while `describe page` shows a name they never wrote. +// +// **One violation per grid, listing its columns.** The first version emitted one +// per column, which a real project (mxcli-dbreplication, finding F6) reported as +// 44 infos saying the same thing. It is one fact about the grid; repeating it +// per column buries the rest of the report without adding information. +// +// It warns rather than rejects: the name is harmless, it reads as documentation +// in the source, and rejecting it would break every existing script — mxcli's +// own doctype tests name every column. What the author needs is to know which +// name addresses it. +func validateDataGrid2ColumnNames(grid *ast.WidgetV3, locationPrefix string) []linter.Violation { + if grid == nil || !strings.EqualFold(grid.Type, "DATAGRID") { + return nil + } + var renamed []string + for _, child := range grid.Children { + if child == nil || !strings.EqualFold(child.Type, "COLUMN") || child.Name == "" { + continue + } + addressable := derivedDataGrid2ColumnName(child) + if addressable == "" || strings.EqualFold(addressable, child.Name) { + continue + } + renamed = append(renamed, fmt.Sprintf("%s → %s", child.Name, addressable)) + } + if len(renamed) == 0 { + return nil + } + return []linter.Violation{{ + RuleID: "MDL-WIDGET16", + Severity: linter.SeverityInfo, + Message: fmt.Sprintf( + "%s: DataGrid 2 stores no column names, so the names on %s are dropped on write. "+ + "Address these columns by their derived name in ALTER PAGE (attribute columns "+ + "key on the bound attribute, others on the caption), and expect DESCRIBE to "+ + "show it: %s.", + locationPrefix, grid.Name, strings.Join(renamed, ", ")), + }} +} + +// derivedDataGrid2ColumnName mirrors the name derivation the writer and the page +// mutator apply, so the warning names the same string ALTER will accept. +// Deliberately conservative: when it cannot tell (no attribute, no caption — the +// colN case, which depends on position) it returns "" and nothing is reported, +// because a wrong name in the message would be worse than none. +func derivedDataGrid2ColumnName(w *ast.WidgetV3) string { + if attr := w.GetAttribute(); attr != "" { + parts := strings.Split(attr, ".") + return parts[len(parts)-1] + } + if caption := w.GetCaption(); caption != "" { + sanitized := strings.Map(func(r rune) rune { + if r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || r == '_' { + return r + } + return '_' + }, caption) + return strings.Trim(sanitized, "_") + } + return "" +} + +// builtinPropertyMisuse names builtin MDL properties that are wrong on a +// specific widget, and the one that is right. +// +// isBuiltinPropName accepts Label/Caption/Class/… on every widget, deliberately: +// the engine routes them through dedicated paths rather than through a def's +// propertyMappings, so validating them against the def would false-positive on +// ordinary MDL. The cost is that a builtin the engine does *not* route for a +// given widget is accepted and silently dropped. +// +// That bit a real project: `combobox cb (Association: …, Caption: Name)` passes +// `check`, executes, loses the caption, and fails the build with +// +// [error] [CE0642] "Property 'Caption' is required." at Combo box 'cb' +// +// The working spelling is `CaptionAttribute:`, which round-trips — so the author +// was one property name away and concluded the feature was unusable +// (mxcli-owid, finding #38). +// +// This is an explicit list rather than something inferred. Whether a builtin is +// routed for a widget lives in the engine's own dispatch, not in the .def.json — +// the combobox def declares optionsSourceAssociationCaption{Type,Expression} and +// nothing called `Caption` — so inferring it would mean reimplementing that +// dispatch here and getting it wrong in the other direction. Add a row when a +// case is measured, and cite the build error in the commit. +var builtinPropertyMisuse = map[string]map[string]string{ + "com.mendix.widget.web.combobox.Combobox": { + "Caption": "CaptionAttribute", + }, +} + +// misusedBuiltinProperty reports the right property name when key is a builtin +// that this widget does not route, matching case-insensitively the way the rest +// of the property lookup does. +func misusedBuiltinProperty(widgetID, key string) (string, bool) { + byName, ok := builtinPropertyMisuse[widgetID] + if !ok { + return "", false + } + for wrong, right := range byName { + if strings.EqualFold(wrong, key) { + return right, true + } + } + return "", false +} diff --git a/mdl/executor/widget_defs.go b/mdl/executor/widget_defs.go index 68711f21c..709476e91 100644 --- a/mdl/executor/widget_defs.go +++ b/mdl/executor/widget_defs.go @@ -496,46 +496,13 @@ var itemSlotAcceptedChildTypes = map[string]map[string]map[string][]string{ }, } -// itemPropertyAliases lists alternative MDL property names that should -// resolve to a given schema property on an object-list item. Keyed by -// (widgetID, objectListPropertyKey, itemPropertyKey). +// itemPropertyAliases is the shared alias table in mdl/types. // -// Example: a DataGrid column's `Caption: '...'` in MDL fills the schema's -// `header` property. Without the alias, the engine looks up `header` in -// the AST property bag and finds nothing — the caption is silently dropped. -// -// Aliases here capture conventions from the historical keyword path; when -// the keyword path is retired (v0.12.0 Phase 4) this stays as the single -// source of truth. -var itemPropertyAliases = map[string]map[string]map[string][]string{ - "com.mendix.widget.web.datagrid.Datagrid": { - "columns": { - "header": {"Caption"}, - "dynamicText": {"Content"}, - // MDL `ColumnWidth: manual` fills the schema's `width` enum. The - // keyword path mapped this (`colPropString(..., "ColumnWidth")`); - // without the alias the engine leaves width at its `autoFill` - // default, so a `Size:` value becomes invalid (size only applies - // when width=manual) and Studio Pro flags CE0463. - "width": {"ColumnWidth"}, - // MDL `DynamicCellClass: ''` fills the schema's `columnClass` - // expression (a per-cell dynamic CSS class). Without the alias the - // engine looks up `columnClass` in the AST property bag, finds - // nothing, and writes an empty expression — the class is silently - // dropped. Bug 10a. - "columnClass": {"DynamicCellClass"}, - }, - }, - "com.mendix.widget.web.heatmap.HeatMap": { - "scaleColors": { - // MDL `ColorValue: '#rrggbb'` fills the schema's `colour` primitive - // (British spelling). Without the alias the engine looks up `colour`, - // doesn't find `ColorValue`, and the scale colour is silently dropped - // on write. Same class as `columnClass` (Bug 10a). - "colour": {"ColorValue"}, - }, - }, -} +// It used to be declared here as a literal, with the page mutator keeping a +// second hand-written copy for the ALTER path. The two drifted — see +// types.ItemPropertyAliases — so the table moved to a package both can import +// and this is now a reference, not a duplicate. +var itemPropertyAliases = types.ItemPropertyAliases // propertyAliases lists alternative MDL names for a widget's TOP-LEVEL properties // (not object-list items — those use itemPropertyAliases). Needed where a widget diff --git a/mdl/exprcatalog/exprcatalog.go b/mdl/exprcatalog/exprcatalog.go new file mode 100644 index 000000000..0c7cbd793 --- /dev/null +++ b/mdl/exprcatalog/exprcatalog.go @@ -0,0 +1,325 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Package exprcatalog implements exprcheck.CatalogReader over mxcli's catalog. +// +// exprcheck ships a complete expression type checker whose semantic rules — +// enum-value comparisons, attribute/operand type mismatches, function argument +// types — all run through the CatalogReader seam. Until this package existed +// nothing implemented that seam, so every real invocation ran with a nil Catalog +// and every semantic rule was silently skipped. The checker was in the tree and +// checked nothing. +// +// The whole index is loaded once, in four queries, rather than answering each +// lookup with SQL. A project has thousands of expressions and each one asks +// several questions; per-question round trips would dominate the run even +// against SQLite. This is the memoized reader PROPOSAL_expression_type_checking +// § 4 asks for. +// +// # Failure mode +// +// Every method returns (zero, false) for anything the catalog cannot answer, and +// exprcheck reads that as KindUnknown, which suppresses the downstream rule. A +// stale or partial catalog therefore makes the checker *catch less*, never raise +// a false positive on valid code — the correct direction for an advisory gate. +// A mutating consumer must not inherit that reading; see the same proposal's +// § Fourth consumer. +// +// One known blind spot lands in that same bucket, and is worth knowing before +// wondering why a check did not fire: **the System module contributes no +// enumerations**. mxcli reads zero of them from a project (`show enumerations in +// System` is empty on a stock 11.13 app) because they are platform metadata +// rather than stored units, so an attribute typed by e.g. +// System.WorkflowEventType resolves its enum name but not its cases, and the +// enum-value rule is skipped for it. Attributes typed by a project's or a +// marketplace module's own enumerations resolve fully. +package exprcatalog + +import ( + "database/sql" + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/exprcheck" +) + +// Querier is the slice of the catalog this package needs: *sql.DB satisfies it, +// which is what catalog.Catalog.CatalogDB() returns. +type Querier interface { + Query(query string, args ...any) (*sql.Rows, error) +} + +// Reader answers exprcheck's type lookups from a loaded index. +type Reader struct { + // attrKind is keyed "Module.Entity.Attr" — the same shape callers already + // hold, so a lookup is one map hit rather than a nested one. + attrKind map[string]exprcheck.TypeKind + attrEnum map[string]string + enumCase map[string][]string + mfReturn map[string]exprcheck.TypeKind + mfParam map[string]exprcheck.TypeKind + // assoc maps an association's qualified name to its two ends, FROM first. + assoc map[string][2]string +} + +var _ exprcheck.CatalogReader = (*Reader)(nil) + +// Load builds the index from a catalog database. +// +// A table that does not exist yet — an old cache file written before schema 10 — +// leaves its part of the index empty rather than failing the load: the caller +// gets a checker that catches less, which is the same degradation as a stale +// row and better than no checking at all. +func Load(db Querier) (*Reader, error) { + if db == nil { + return nil, fmt.Errorf("exprcatalog: nil catalog") + } + r := &Reader{ + attrKind: map[string]exprcheck.TypeKind{}, + attrEnum: map[string]string{}, + enumCase: map[string][]string{}, + mfReturn: map[string]exprcheck.TypeKind{}, + mfParam: map[string]exprcheck.TypeKind{}, + assoc: map[string][2]string{}, + } + for _, load := range []func(Querier) error{ + r.loadAttributes, r.loadEnumValues, r.loadMicroflows, r.loadParameters, + r.loadAssociations, + } { + if err := load(db); err != nil { + return nil, err + } + } + return r, nil +} + +func (r *Reader) loadAttributes(db Querier) error { + rows, err := db.Query(`SELECT EntityQualifiedName, Name, DataType, EnumerationQualifiedName FROM attributes`) + if err != nil { + return missingTableOK(err) + } + defer rows.Close() + for rows.Next() { + var entity, name, dataType, enumQN sql.NullString + if err := rows.Scan(&entity, &name, &dataType, &enumQN); err != nil { + return err + } + if entity.String == "" || name.String == "" { + continue + } + key := entity.String + "." + name.String + if k, ok := attributeKind(dataType.String); ok { + r.attrKind[key] = k + } + if enumQN.String != "" { + r.attrEnum[key] = enumQN.String + } + } + return rows.Err() +} + +func (r *Reader) loadEnumValues(db Querier) error { + rows, err := db.Query( + `SELECT EnumerationQualifiedName, Name FROM enumeration_values ORDER BY EnumerationQualifiedName, Ordinal`) + if err != nil { + return missingTableOK(err) + } + defer rows.Close() + for rows.Next() { + var enumQN, name sql.NullString + if err := rows.Scan(&enumQN, &name); err != nil { + return err + } + if enumQN.String == "" || name.String == "" { + continue + } + r.enumCase[enumQN.String] = append(r.enumCase[enumQN.String], name.String) + } + return rows.Err() +} + +func (r *Reader) loadMicroflows(db Querier) error { + // The microflows view already covers nanoflows; MicroflowType only filters + // them apart, and a caller naming a flow does not care which it is. + rows, err := db.Query(`SELECT QualifiedName, ReturnType FROM microflows`) + if err != nil { + return missingTableOK(err) + } + defer rows.Close() + for rows.Next() { + var qn, returnType sql.NullString + if err := rows.Scan(&qn, &returnType); err != nil { + return err + } + if qn.String == "" { + continue + } + if k, ok := flowKind(returnType.String); ok { + r.mfReturn[qn.String] = k + } + } + return rows.Err() +} + +func (r *Reader) loadParameters(db Querier) error { + rows, err := db.Query(`SELECT MicroflowQualifiedName, Name, ParameterType FROM microflow_parameters`) + if err != nil { + return missingTableOK(err) + } + defer rows.Close() + for rows.Next() { + var qn, name, paramType sql.NullString + if err := rows.Scan(&qn, &name, ¶mType); err != nil { + return err + } + if qn.String == "" || name.String == "" { + continue + } + if k, ok := flowKind(paramType.String); ok { + r.mfParam[qn.String+"("+name.String] = k + } + } + return rows.Err() +} + +func (r *Reader) loadAssociations(db Querier) error { + rows, err := db.Query(`SELECT QualifiedName, FromEntity, ToEntity FROM associations`) + if err != nil { + return missingTableOK(err) + } + defer rows.Close() + for rows.Next() { + var qn, from, to sql.NullString + if err := rows.Scan(&qn, &from, &to); err != nil { + return err + } + // An end that is not recorded leaves the association out entirely, so a + // path through it reads as unresolved rather than half-resolved. + if qn.String == "" || from.String == "" || to.String == "" { + continue + } + r.assoc[qn.String] = [2]string{from.String, to.String} + } + return rows.Err() +} + +// AssociationTarget returns the entity at the other end of an association. +// +// Both directions resolve: a Mendix expression follows an association from its +// FROM end and from its TO end alike, and the path looks the same either way. +// The signature matches xpathrefs.Model so the two resolvers can converge. +func (r *Reader) AssociationTarget(assocQN, fromEntityQN string) (string, bool) { + ends, ok := r.assoc[assocQN] + if !ok || fromEntityQN == "" { + return "", false + } + switch fromEntityQN { + case ends[0]: + return ends[1], true + case ends[1]: + return ends[0], true + } + // A self-association resolves above; anything else means the path does not + // start where it claims to. + return "", false +} + +// AttributeKind returns the kind of Module.Entity.Attr. +func (r *Reader) AttributeKind(entityQN, attrName string) (exprcheck.TypeKind, bool) { + k, ok := r.attrKind[entityQN+"."+attrName] + return k, ok +} + +// AttributeEnumQN returns which enumeration an enumeration-typed attribute uses. +func (r *Reader) AttributeEnumQN(entityQN, attrName string) (string, bool) { + qn, ok := r.attrEnum[entityQN+"."+attrName] + return qn, ok +} + +// EnumCases returns an enumeration's value names in model order. +func (r *Reader) EnumCases(enumQN string) ([]string, bool) { + cases, ok := r.enumCase[enumQN] + if !ok { + return nil, false + } + // Copy: the index outlives any one check, and a caller that sorted or + // truncated the slice in place would corrupt every later lookup. + out := make([]string, len(cases)) + copy(out, cases) + return out, true +} + +// MicroflowReturn returns a microflow's or nanoflow's return kind. +func (r *Reader) MicroflowReturn(qn string) (exprcheck.TypeKind, bool) { + k, ok := r.mfReturn[qn] + return k, ok +} + +// MicroflowParam returns the kind of one named parameter. +func (r *Reader) MicroflowParam(qn, paramName string) (exprcheck.TypeKind, bool) { + k, ok := r.mfParam[qn+"("+strings.TrimPrefix(paramName, "$")] + return k, ok +} + +// attributeKind maps a domain-model attribute's stored type name. +// +// The names come from domainmodel's GetTypeName, which is the bare kind — an +// enumeration attribute reports "Enumeration" and says nothing about which one, +// hence the separate AttributeEnumQN lookup. +func attributeKind(name string) (exprcheck.TypeKind, bool) { + switch name { + case "String", "HashedString": + // A hashed string is still a String everywhere an expression can touch + // it; only the storage differs. + return exprcheck.KindString, true + case "Integer": + return exprcheck.KindInteger, true + case "Long", "AutoNumber": + // AutoNumber is a Long that the runtime assigns. + return exprcheck.KindLong, true + case "Decimal": + return exprcheck.KindDecimal, true + case "Boolean": + return exprcheck.KindBoolean, true + case "DateTime", "Date": + return exprcheck.KindDateTime, true + case "Binary": + return exprcheck.KindBinary, true + case "Enumeration": + return exprcheck.KindEnumeration, true + } + return exprcheck.KindUnknown, false +} + +// flowKind maps a microflow parameter or return type as the catalog encodes it, +// which carries the referenced element after a colon ("Object:Mod.Entity"). +func flowKind(name string) (exprcheck.TypeKind, bool) { + base, _, _ := strings.Cut(name, ":") + switch base { + case "Object": + return exprcheck.KindObject, true + case "List": + return exprcheck.KindList, true + case "Enumeration": + return exprcheck.KindEnumeration, true + case "Void": + // A void microflow has no value, and exprcheck has no kind that says so. + // Reporting "not found" makes it unknown, which suppresses rules rather + // than inventing a type — calling a void flow in a value position is a + // real error, but it is not this seam's to diagnose. + return exprcheck.KindUnknown, false + } + return attributeKind(base) +} + +// missingTableOK turns "no such table" into a soft miss. +// +// NewFromFile applies the current schema to an old cache with CREATE TABLE IF +// NOT EXISTS, so this should not fire in practice — but a reader that hard-fails +// on one absent table would take the whole check down over a lookup it is +// designed to survive losing. +func missingTableOK(err error) error { + if err != nil && strings.Contains(err.Error(), "no such table") { + return nil + } + return err +} diff --git a/mdl/exprcatalog/exprcatalog_test.go b/mdl/exprcatalog/exprcatalog_test.go new file mode 100644 index 000000000..5d1f00c05 --- /dev/null +++ b/mdl/exprcatalog/exprcatalog_test.go @@ -0,0 +1,240 @@ +// SPDX-License-Identifier: Apache-2.0 + +package exprcatalog + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/catalog" + "github.com/mendixlabs/mxcli/mdl/exprcheck" +) + +// seeded returns a Reader over a real catalog database holding a small Shop +// model. A real database rather than a stub: the point of this package is that +// the SQL matches the schema, which a stub cannot check. +func seeded(t *testing.T) *Reader { + t.Helper() + cat, err := catalog.New() + if err != nil { + t.Fatalf("catalog.New: %v", err) + } + t.Cleanup(func() { cat.Close() }) + + db := cat.CatalogDB() + exec := func(q string, args ...any) { + t.Helper() + if _, err := db.Exec(q, args...); err != nil { + t.Fatalf("seed %q: %v", q, err) + } + } + + exec(`INSERT INTO attributes_data (Id, Name, EntityQualifiedName, DataType, EnumerationQualifiedName) + VALUES ('a1', 'Name', 'Shop.Order', 'String', ''), + ('a2', 'Quantity', 'Shop.Order', 'Integer', ''), + ('a3', 'Status', 'Shop.Order', 'Enumeration', 'Shop.OrderStatus'), + ('a4', 'Code', 'Shop.Order', 'AutoNumber', ''), + ('a5', 'Secret', 'Shop.Order', 'HashedString', ''), + ('a6', 'Name', 'Shop.Customer', 'String', '')`) + + exec(`INSERT INTO enumeration_values_data (Id, EnumerationQualifiedName, Name, Ordinal) + VALUES ('v1', 'Shop.OrderStatus', 'Open', 0), + ('v2', 'Shop.OrderStatus', 'Shipped', 1), + ('v3', 'Shop.OrderStatus', 'Closed', 2)`) + + exec(`INSERT INTO microflows_data (Id, QualifiedName, MicroflowType, ReturnType) + VALUES ('m1', 'Shop.ACT_Total', 'MICROFLOW', 'Decimal'), + ('m2', 'Shop.ACT_Find', 'MICROFLOW', 'Object:Shop.Order'), + ('m3', 'Shop.ACT_Log', 'MICROFLOW', 'Void'), + ('m4', 'Shop.NF_Refresh', 'NANOFLOW', 'Boolean')`) + + exec(`INSERT INTO microflow_parameters_data (Id, MicroflowQualifiedName, Name, ParameterType, Ordinal) + VALUES ('p1', 'Shop.ACT_Total', 'Order', 'Object:Shop.Order', 0), + ('p2', 'Shop.ACT_Total', 'Discount', 'Decimal', 1)`) + + exec(`INSERT INTO associations_data (Id, QualifiedName, FromEntity, ToEntity) + VALUES ('as1', 'Shop.Order_Customer', 'Shop.Order', 'Shop.Customer'), + ('as2', 'Shop.Broken', 'Shop.Order', '')`) + + r, err := Load(db) + if err != nil { + t.Fatalf("Load: %v", err) + } + return r +} + +// TestReaderSatisfiesTheSeam is the point of the package: before it, nothing +// implemented CatalogReader, so exprcheck ran with a nil Catalog and every +// semantic rule was skipped. +func TestReaderSatisfiesTheSeam(t *testing.T) { + var _ exprcheck.CatalogReader = seeded(t) +} + +func TestAttributeKind(t *testing.T) { + r := seeded(t) + tests := []struct { + entity, attr string + want exprcheck.TypeKind + found bool + }{ + {"Shop.Order", "Name", exprcheck.KindString, true}, + {"Shop.Order", "Quantity", exprcheck.KindInteger, true}, + {"Shop.Order", "Status", exprcheck.KindEnumeration, true}, + // AutoNumber is a runtime-assigned Long, HashedString a String with + // different storage — both are ordinary types to an expression. + {"Shop.Order", "Code", exprcheck.KindLong, true}, + {"Shop.Order", "Secret", exprcheck.KindString, true}, + // Same attribute name on another entity must not answer for this one. + {"Shop.Customer", "Quantity", exprcheck.KindUnknown, false}, + {"Shop.Missing", "Name", exprcheck.KindUnknown, false}, + } + for _, tc := range tests { + got, ok := r.AttributeKind(tc.entity, tc.attr) + if got != tc.want || ok != tc.found { + t.Errorf("AttributeKind(%q, %q) = (%v, %v), want (%v, %v)", + tc.entity, tc.attr, got, ok, tc.want, tc.found) + } + } +} + +func TestAttributeEnumQN(t *testing.T) { + r := seeded(t) + if qn, ok := r.AttributeEnumQN("Shop.Order", "Status"); !ok || qn != "Shop.OrderStatus" { + t.Errorf("got (%q, %v), want Shop.OrderStatus", qn, ok) + } + // A non-enumeration attribute has no enum, and must report that rather than + // an empty string that reads as one. + if qn, ok := r.AttributeEnumQN("Shop.Order", "Name"); ok { + t.Errorf("a String attribute reported enum %q", qn) + } +} + +func TestEnumCases(t *testing.T) { + r := seeded(t) + got, ok := r.EnumCases("Shop.OrderStatus") + if !ok { + t.Fatal("the enumeration's cases were not found") + } + want := []string{"Open", "Shipped", "Closed"} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("got %v, want %v — cases must come back in model order", got, want) + } + } + if _, ok := r.EnumCases("Shop.Nope"); ok { + t.Error("an unknown enumeration reported cases") + } +} + +// TestEnumCasesReturnsACopy pins that a caller cannot corrupt the index. The +// reader is loaded once and serves every expression in the run, so a slice +// handed out by reference and then sorted in place would change the answer for +// everything after it. +func TestEnumCasesReturnsACopy(t *testing.T) { + r := seeded(t) + first, _ := r.EnumCases("Shop.OrderStatus") + first[0] = "MUTATED" + second, _ := r.EnumCases("Shop.OrderStatus") + if second[0] != "Open" { + t.Errorf("the index was mutated through a returned slice: got %q", second[0]) + } +} + +func TestMicroflowReturn(t *testing.T) { + r := seeded(t) + tests := []struct { + qn string + want exprcheck.TypeKind + found bool + }{ + {"Shop.ACT_Total", exprcheck.KindDecimal, true}, + {"Shop.ACT_Find", exprcheck.KindObject, true}, + // Nanoflows share the microflows view and must resolve the same way. + {"Shop.NF_Refresh", exprcheck.KindBoolean, true}, + // Void has no kind that says "no value", so it reports not-found rather + // than inventing one. + {"Shop.ACT_Log", exprcheck.KindUnknown, false}, + {"Shop.Absent", exprcheck.KindUnknown, false}, + } + for _, tc := range tests { + got, ok := r.MicroflowReturn(tc.qn) + if got != tc.want || ok != tc.found { + t.Errorf("MicroflowReturn(%q) = (%v, %v), want (%v, %v)", tc.qn, got, ok, tc.want, tc.found) + } + } +} + +func TestMicroflowParam(t *testing.T) { + r := seeded(t) + if got, ok := r.MicroflowParam("Shop.ACT_Total", "Order"); !ok || got != exprcheck.KindObject { + t.Errorf("got (%v, %v), want an Object parameter", got, ok) + } + // Callers hold variable names with the sigil; accept either spelling. + if got, ok := r.MicroflowParam("Shop.ACT_Total", "$Discount"); !ok || got != exprcheck.KindDecimal { + t.Errorf("got (%v, %v), want a Decimal parameter for the $-prefixed name", got, ok) + } + if _, ok := r.MicroflowParam("Shop.ACT_Total", "Nope"); ok { + t.Error("an unknown parameter resolved") + } + // A parameter of one flow must not answer for another. + if _, ok := r.MicroflowParam("Shop.ACT_Find", "Order"); ok { + t.Error("a parameter leaked across microflows") + } +} + +// TestLoadOnEmptyCatalogIsNotAnError pins the degradation the package promises: +// an empty or partial catalog yields a reader that answers "unknown", which +// exprcheck reads as catch-less. It must not fail the caller. +func TestLoadOnEmptyCatalogIsNotAnError(t *testing.T) { + cat, err := catalog.New() + if err != nil { + t.Fatalf("catalog.New: %v", err) + } + defer cat.Close() + + r, err := Load(cat.CatalogDB()) + if err != nil { + t.Fatalf("Load on an empty catalog: %v", err) + } + if _, ok := r.AttributeKind("Shop.Order", "Name"); ok { + t.Error("an empty catalog answered a lookup") + } +} + +func TestLoadRejectsANilCatalog(t *testing.T) { + if _, err := Load(nil); err == nil { + t.Error("Load(nil) succeeded; a nil catalog would silently disable every check") + } +} + +// TestAssociationTarget pins the hop resolution an expression needs. Unlike +// XPath, `$Order/Shop.Order_Customer/Name` does not name the intermediate +// entity, so the association is the only thing that says where the path lands. +func TestAssociationTarget(t *testing.T) { + r := seeded(t) + tests := []struct { + assoc, from, want string + ok bool + }{ + {"Shop.Order_Customer", "Shop.Order", "Shop.Customer", true}, + // Either end: a Mendix expression follows an association both ways and + // the path looks identical. + {"Shop.Order_Customer", "Shop.Customer", "Shop.Order", true}, + // Starting somewhere the association does not touch is not a hop. + {"Shop.Order_Customer", "Shop.Elsewhere", "", false}, + {"Shop.Order_Customer", "", "", false}, + {"Shop.Nope", "Shop.Order", "", false}, + // An association with an unrecorded end is left out entirely rather + // than resolving to half an answer. + {"Shop.Broken", "Shop.Order", "", false}, + } + for _, tc := range tests { + got, ok := r.AssociationTarget(tc.assoc, tc.from) + if got != tc.want || ok != tc.ok { + t.Errorf("AssociationTarget(%q, %q) = (%q, %v), want (%q, %v)", + tc.assoc, tc.from, got, ok, tc.want, tc.ok) + } + } +} diff --git a/mdl/exprcheck/adapters/adapter_scope.go b/mdl/exprcheck/adapters/adapter_scope.go index 2b913285e..53b3c68c2 100644 --- a/mdl/exprcheck/adapters/adapter_scope.go +++ b/mdl/exprcheck/adapters/adapter_scope.go @@ -2,7 +2,12 @@ package adapters -import "github.com/mendixlabs/mxcli/mdl/ast" +import ( + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "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. @@ -40,3 +45,56 @@ func buildVarEntityScope(body []ast.MicroflowStatement) map[string]string { walk(body) return scope } + +// entityScope adapts a variable→entity map plus an association resolver to +// exprcheck.EntityScope, which is what lets `$Order/Customer/Name` be typed. +type entityScope struct { + vars map[string]string + assoc associationResolver +} + +// associationResolver is the association half of the model. exprcatalog.Reader +// satisfies it, and so does mdl/xpathrefs' Model — the two resolvers answer the +// same question and the signature is deliberately shared. +type associationResolver interface { + AssociationTarget(assocQN, fromEntityQN string) (string, bool) +} + +var _ exprcheck.EntityScope = entityScope{} + +func (e entityScope) VariableEntity(name string) (string, bool) { + qn, ok := e.vars[strings.TrimPrefix(name, "$")] + return qn, ok && qn != "" +} + +func (e entityScope) AssociationTarget(assocQN, fromEntityQN string) (string, bool) { + if e.assoc == nil { + return "", false + } + return e.assoc.AssociationTarget(assocQN, fromEntityQN) +} + +// addParamEntities records the entity a parameter holds. +// +// buildVarEntityScope 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. +// +// The visitor cannot tell `$P: Mod.Person` from an enumeration-typed parameter: +// a bare qualified name parses as TypeEnumeration with EnumRef set (see +// 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) { + for _, p := range params { + if p.Name == "" { + continue + } + switch { + case p.Type.EntityRef != nil: + scope[p.Name] = p.Type.EntityRef.String() + case p.Type.Kind == ast.TypeEnumeration && p.Type.EnumRef != nil: + scope[p.Name] = p.Type.EnumRef.String() + } + } +} diff --git a/mdl/exprcheck/adapters/check.go b/mdl/exprcheck/adapters/check.go index 28f8660dd..7623bdd82 100644 --- a/mdl/exprcheck/adapters/check.go +++ b/mdl/exprcheck/adapters/check.go @@ -3,6 +3,8 @@ package adapters import ( + "strings" + "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/exprcheck" exprhints "github.com/mendixlabs/mxcli/mdl/exprcheck/hints" @@ -13,14 +15,50 @@ type CheckAdapter struct { parser exprcheck.Parser slots exprcheck.SlotResolver catalog exprcheck.CatalogReader + source func(ast.Expression) string + // 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 exprcheck.EntityScope +} + +// Option configures a CheckAdapter. +type Option func(*CheckAdapter) + +// WithSourceFunc supplies the function that recovers an expression's source +// text. +// +// The default reads only ast.SourceExpr, which the visitor produces for some +// slots and not others: measured on a create-and-change microflow, the CREATE's +// value arrived as a SourceExpr and the CHANGE's as a bare LiteralExpr, so half +// the enum-literal mistakes in one flow were invisible. Callers that can render +// an expression back to text — mdl/executor has expressionToString — should pass +// it in so coverage does not depend on which slot the visitor happened to wrap. +func WithSourceFunc(f func(ast.Expression) string) Option { + return func(c *CheckAdapter) { + if f != nil { + c.source = f + } + } } -func NewCheckAdapter(cat exprcheck.CatalogReader) *CheckAdapter { - return &CheckAdapter{ +func NewCheckAdapter(cat exprcheck.CatalogReader, opts ...Option) *CheckAdapter { + c := &CheckAdapter{ parser: exprcheck.NewParser(), slots: exprcheck.DefaultSlotResolver(), catalog: cat, + source: exprSource, + } + // Association traversal is optional: CatalogReader does not require it, so a + // reader that cannot answer it simply leaves multi-hop paths unresolved. + if ar, ok := cat.(associationResolver); ok { + c.assoc = ar + } + for _, opt := range opts { + opt(c) } + return c } type Result struct { @@ -32,12 +70,37 @@ func (c *CheckAdapter) CheckMicroflow(stmt *ast.CreateMicroflowStmt) *Result { if stmt == nil { return r } - c.walkBody(stmt.Body, stmt.Name.String(), r) + c.walkFlow(stmt.Body, stmt.Parameters, stmt.Name.String(), r) + return r +} + +// CheckNanoflow checks a nanoflow's expressions. +// +// A nanoflow's body is the same []ast.MicroflowStatement, and every rule here +// is about expressions rather than about which activities are legal, so the two +// share one walk. Giving nanoflows their own entry point rather than leaving +// callers to reach for CheckMicroflow keeps the asymmetry from looking +// deliberate. +func (c *CheckAdapter) CheckNanoflow(stmt *ast.CreateNanoflowStmt) *Result { + r := &Result{} + if stmt == nil { + return r + } + c.walkFlow(stmt.Body, stmt.Parameters, stmt.Name.String(), r) return r } -func (c *CheckAdapter) walkBody(body []ast.MicroflowStatement, mf string, r *Result) { +// walkFlow checks one flow's expressions with its variables typed. +// +// 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. +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) } @@ -89,7 +152,10 @@ func (c *CheckAdapter) walkBodyWithScope(body []ast.MicroflowStatement, mf strin } func (c *CheckAdapter) checkExpr(expr ast.Expression, slot, mf string, r *Result) { - src := exprSource(expr) + // The captured source can carry the trailing layout of the statement it was + // lifted from ("'Open'\n "), which the lexer would then have to recover + // from. Trim before parsing rather than teaching every rule about it. + src := strings.TrimSpace(c.source(expr)) if src == "" { return } @@ -98,6 +164,7 @@ func (c *CheckAdapter) checkExpr(expr ast.Expression, slot, mf string, r *Result Microflow: mf, Slots: c.slots, Catalog: c.catalog, + Entities: c.entities, }) r.Hints = append(r.Hints, hints...) } diff --git a/mdl/exprcheck/adapters/check_test.go b/mdl/exprcheck/adapters/check_test.go index ade5a8499..76193006f 100644 --- a/mdl/exprcheck/adapters/check_test.go +++ b/mdl/exprcheck/adapters/check_test.go @@ -73,3 +73,63 @@ func TestCheckAdapter_CreateItemEmbedsEntityAttrInSlotPath(t *testing.T) { t.Errorf("expected catalog query %q, got %+v", want, stub.calls) } } + +// TestWithSourceFuncIsUsed pins that a caller can supply the source recovery. +// +// The default reads only ast.SourceExpr, and the visitor attaches one to some +// slots and not others — measured on a fixture project, neither a CREATE's nor a +// CHANGE's enum value carried one, so the adapter saw nothing to check. A caller +// that can render the AST back to text (mdl/executor's expressionToString) must +// be able to say so. +func TestWithSourceFuncIsUsed(t *testing.T) { + called := 0 + c := NewCheckAdapter(nil, WithSourceFunc(func(ast.Expression) string { + called++ + return "'x'" + })) + r := &Result{} + c.checkExpr(&ast.LiteralExpr{}, "DeclareStmt.InitialValue", "M.A", r) + if called != 1 { + t.Errorf("the supplied source function was called %d times, want 1", called) + } +} + +// TestSourceIsTrimmedBeforeParsing pins that captured layout does not reach the +// lexer. SourceExpr.Source arrives carrying the statement's trailing newline and +// indentation ("'Open'\n "), which is the parser's problem only if we hand it +// over. +func TestSourceIsTrimmedBeforeParsing(t *testing.T) { + var seen string + c := NewCheckAdapter(nil, WithSourceFunc(func(ast.Expression) string { + return " 'Open'\n " + })) + c.parser = parserFunc(func(src string, ctx exprcheck.Context) (exprcheck.RobustExpr, []exprcheck.Hint) { + seen = src + return nil, nil + }) + c.checkExpr(&ast.LiteralExpr{}, "CreateItem.Value", "M.A", &Result{}) + if seen != "'Open'" { + t.Errorf("the parser received %q, want the trimmed source", seen) + } +} + +// TestEmptySourceIsSkipped pins that an expression with no recoverable text +// costs nothing — the parser is never called for it. +func TestEmptySourceIsSkipped(t *testing.T) { + calls := 0 + c := NewCheckAdapter(nil, WithSourceFunc(func(ast.Expression) string { return " " })) + c.parser = parserFunc(func(src string, ctx exprcheck.Context) (exprcheck.RobustExpr, []exprcheck.Hint) { + calls++ + return nil, nil + }) + c.checkExpr(&ast.LiteralExpr{}, "CreateItem.Value", "M.A", &Result{}) + if calls != 0 { + t.Errorf("the parser was called %d times for blank source, want 0", calls) + } +} + +type parserFunc func(string, exprcheck.Context) (exprcheck.RobustExpr, []exprcheck.Hint) + +func (f parserFunc) Parse(src string, ctx exprcheck.Context) (exprcheck.RobustExpr, []exprcheck.Hint) { + return f(src, ctx) +} diff --git a/mdl/exprcheck/attribute_path_test.go b/mdl/exprcheck/attribute_path_test.go new file mode 100644 index 000000000..eabdeb4f3 --- /dev/null +++ b/mdl/exprcheck/attribute_path_test.go @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: Apache-2.0 + +package exprcheck + +import "testing" + +// pathCatalog is a small Shop model: Order has Total (Decimal), Status +// (Enumeration Shop.OrderStatus) and Shipped (Boolean); Customer has Name +// (String). Shop.Order_Customer joins them. +type pathCatalog struct{} + +func (pathCatalog) AttributeKind(entityQN, attr string) (TypeKind, bool) { + switch entityQN + "." + attr { + case "Shop.Order.Total": + return KindDecimal, true + case "Shop.Order.Status": + return KindEnumeration, true + case "Shop.Order.Shipped": + return KindBoolean, true + case "Shop.Customer.Name": + return KindString, true + } + return KindUnknown, false +} + +func (pathCatalog) AttributeEnumQN(entityQN, attr string) (string, bool) { + if entityQN == "Shop.Order" && attr == "Status" { + return "Shop.OrderStatus", true + } + return "", false +} + +func (pathCatalog) EnumCases(enumQN string) ([]string, bool) { + if enumQN == "Shop.OrderStatus" { + return []string{"Open", "Shipped", "Closed"}, true + } + return nil, false +} + +func (pathCatalog) MicroflowReturn(string) (TypeKind, bool) { return KindUnknown, false } +func (pathCatalog) MicroflowParam(string, string) (TypeKind, bool) { return KindUnknown, false } + +// pathScope types $O as an Order and $C as a Customer, and knows one +// association. +type pathScope struct{} + +func (pathScope) VariableEntity(name string) (string, bool) { + switch name { + case "O": + return "Shop.Order", true + case "C": + return "Shop.Customer", true + } + return "", false +} + +func (pathScope) AssociationTarget(assocQN, from string) (string, bool) { + if assocQN != "Shop.Order_Customer" { + return "", false + } + switch from { + case "Shop.Order": + return "Shop.Customer", true + case "Shop.Customer": + return "Shop.Order", true + } + return "", false +} + +func pathCtx() Context { + return Context{Catalog: pathCatalog{}, Entities: pathScope{}, Slots: DefaultSlotResolver()} +} + +func kindOf(t *testing.T, src string, ctx Context) TypeKind { + t.Helper() + e, _ := NewParser().Parse(src, ctx) + return inferKind(e, ctx) +} + +// TestAttributePathResolvesToItsKind is the gap this file closes. inferKind +// returned KindUnknown for every AttributePathExpr, so `$obj/Attr` typed to +// nothing and every rule downstream of it stayed quiet. +func TestAttributePathResolvesToItsKind(t *testing.T) { + ctx := pathCtx() + tests := []struct { + src string + want TypeKind + }{ + {"$O/Total", KindDecimal}, + {"$O/Status", KindEnumeration}, + {"$O/Shipped", KindBoolean}, + // Through an association hop, which an expression does not spell the + // intermediate entity for — unlike XPath. + {"$O/Shop.Order_Customer/Name", KindString}, + // Reverse direction resolves too. + {"$C/Shop.Order_Customer/Total", KindDecimal}, + } + for _, tc := range tests { + if got := kindOf(t, tc.src, ctx); got != tc.want { + t.Errorf("%s inferred %v, want %v", tc.src, got, tc.want) + } + } +} + +// TestAttributePathUnknownsStayUnknown pins the failure direction: anything the +// seams cannot answer types to nothing, which suppresses the rule that asked +// rather than guessing at it. +func TestAttributePathUnknownsStayUnknown(t *testing.T) { + ctx := pathCtx() + for _, src := range []string{ + "$Unknown/Total", // variable not in scope + "$O/NoSuchAttribute", // attribute not on the entity + "$O/Shop.Mystery/Name", // unresolvable association + "$C/Total", // right name, wrong entity + "$O/Shop.Order_Customer/NoSuchAttr", // hop resolves, attribute does not + } { + if got := kindOf(t, src, ctx); got != KindUnknown { + t.Errorf("%s inferred %v, want KindUnknown", src, got) + } + } +} + +// TestAttributePathNeedsBothSeams pins that the resolution is off unless the +// caller supplied both halves — a Context with no EntityScope must behave +// exactly as it did before. +func TestAttributePathNeedsBothSeams(t *testing.T) { + if got := kindOf(t, "$O/Total", Context{Catalog: pathCatalog{}}); got != KindUnknown { + t.Errorf("with no EntityScope, inferred %v, want KindUnknown", got) + } + if got := kindOf(t, "$O/Total", Context{Entities: pathScope{}}); got != KindUnknown { + t.Errorf("with no Catalog, inferred %v, want KindUnknown", got) + } +} + +// TestEnumComparedToStringLiteral is the case the proposal opens with, and the +// one a person actually writes. It has no slot to key off — the only thing that +// says "this is an enumeration" is the attribute path on the other side. +func TestEnumComparedToStringLiteral(t *testing.T) { + ctx := pathCtx() + for _, src := range []string{ + "$O/Status = 'Open'", + "$O/Status != 'Open'", + "'Open' = $O/Status", // operand order must not matter + } { + _, hs := NewParser().Parse(src, ctx) + if len(hs) != 1 { + t.Fatalf("%s produced %d hints, want 1: %+v", src, len(hs), hs) + } + if hs[0].Code != "E001" { + t.Errorf("%s reported %s, want E001 — the same code as the slot form", src, hs[0].Code) + } + if hs[0].Fix != "Shop.OrderStatus.Open" { + t.Errorf("%s suggested %q, want the qualified enum value", src, hs[0].Fix) + } + if hs[0].Reference == nil || len(hs[0].Reference.EnumValues) != 3 { + t.Errorf("%s did not carry the enum's legal values: %+v", src, hs[0].Reference) + } + } +} + +// TestEnumComparisonLeavesValidFormsAlone is the control. Without it the test +// above would pass against a rule that flagged every comparison. +func TestEnumComparisonLeavesValidFormsAlone(t *testing.T) { + ctx := pathCtx() + for _, src := range []string{ + "$O/Status = Shop.OrderStatus.Open", // the correct spelling + "$C/Name = 'Ada'", // a String attribute really is compared to a string + "$O/Total = 10", // no string literal at all + "$Unknown/Status = 'Open'", // unresolvable variable — no guessing + "'Open' = 'Open'", // two literals, no attribute + } { + if _, hs := NewParser().Parse(src, ctx); len(hs) != 0 { + t.Errorf("%s was flagged: %+v", src, hs) + } + } +} + +// TestEnumComparisonNeedsTheCatalog pins that the rule cannot fire without the +// seams, so a project-less check is unaffected. +func TestEnumComparisonNeedsTheCatalog(t *testing.T) { + if _, hs := NewParser().Parse("$O/Status = 'Open'", Context{}); len(hs) != 0 { + t.Errorf("a syntax-only context reported %+v", hs) + } +} + +// TestConcatWithResolvedPath pins that the existing E004 rule benefits from the +// resolution too — a Boolean or Enumeration operand cannot be concatenated, +// while Mendix does auto-convert a numeric one (which is why Total is silent). +func TestConcatWithResolvedPath(t *testing.T) { + ctx := pathCtx() + if _, hs := NewParser().Parse("'status: ' + $O/Status", ctx); len(hs) != 1 || hs[0].Code != "E004" { + t.Errorf("concatenating an Enumeration was not flagged: %+v", hs) + } + if _, hs := NewParser().Parse("'total: ' + $O/Total", ctx); len(hs) != 0 { + t.Errorf("concatenating a Decimal was flagged, but Mendix auto-converts it: %+v", hs) + } +} diff --git a/mdl/exprcheck/interfaces.go b/mdl/exprcheck/interfaces.go index a64b43506..524db1f90 100644 --- a/mdl/exprcheck/interfaces.go +++ b/mdl/exprcheck/interfaces.go @@ -18,9 +18,10 @@ type Context struct { Line int Column int - Scope Scope - Catalog CatalogReader // nil → semantic checks disabled - Slots SlotResolver // nil → slot-kind checks disabled + Scope Scope + Catalog CatalogReader // nil → semantic checks disabled + Slots SlotResolver // nil → slot-kind checks disabled + Entities EntityScope // nil → attribute paths stay KindUnknown } // IsSemanticEnabled reports whether Catalog and Slots are both wired so that @@ -67,6 +68,28 @@ type Scope interface { Lookup(name string) (TypeKind, bool) } +// EntityScope resolves the object side of an expression: which entity a +// variable holds, and where an association leads from there. +// +// It is separate from Scope because Scope speaks TypeKind, which can say "$P is +// an Object" but not *which* entity — and without that, `$P/Status` cannot be +// resolved to an attribute at all. Keeping it out of CatalogReader too: the +// variable half is per-microflow rather than per-project, and leaving +// CatalogReader's shape untouched keeps it re-syncable from the upstream fork. +// +// A nil Context.Entities is the pre-existing behaviour: attribute paths infer +// KindUnknown and every rule that depends on them stays quiet. +type EntityScope interface { + // VariableEntity returns the qualified entity name a variable holds. The + // name is passed without the leading '$'. + VariableEntity(name string) (string, bool) + // AssociationTarget returns the entity at the other end of association + // assocQN traversed from fromEntityQN, and whether assocQN is an + // association at all. Both directions resolve — a Mendix expression can + // follow an association from either end. + AssociationTarget(assocQN, fromEntityQN string) (string, bool) +} + type SlotConstraint struct { Kind TypeKind ResolveBy string diff --git a/mdl/exprcheck/parser.go b/mdl/exprcheck/parser.go index 954e6ee9b..aa2ffa859 100644 --- a/mdl/exprcheck/parser.go +++ b/mdl/exprcheck/parser.go @@ -120,9 +120,69 @@ func parseCmp(s *Stream, ctx Context) (RobustExpr, []Hint) { if op == "" { return left, hints } - s.Consume() + opTok := s.Consume() right, h := parseAdd(s, ctx) - return &BinExpr{Op: op, L: left, R: right}, append(hints, h...) + hints = append(hints, h...) + if op == "=" || op == "!=" { + hints = append(hints, checkEnumComparedToString(left, right, ctx, opTok)...) + } + return &BinExpr{Op: op, L: left, R: right}, hints +} + +// checkEnumComparedToString emits E001 for `$obj/EnumAttr = 'Value'`. +// +// This is the same defect checkStringLitVsSlot reports, found a different way. +// That one keys off the *slot* — a create or change member names its attribute, +// so the enum is known before the value is read — which covers assignment and +// nothing else. A comparison has no slot: the only thing that says "this is an +// enumeration" is the other operand, which means resolving an attribute path. +// It is the shape the proposal opens with (`if $Order/Status = 'Open'`) and the +// one a person actually writes. +// +// Same code and message as the slot form on purpose: one defect should not have +// two names depending on where it was spotted. +func checkEnumComparedToString(left, right RobustExpr, ctx Context, opTok Token) []Hint { + lit, path := pairEnumPathWithStringLit(left, right) + if lit == nil || path == nil { + return nil + } + enumQN, ok := attributePathEnumQN(path, ctx) + if !ok { + return nil + } + vals, _ := ctx.Catalog.EnumCases(enumQN) + return []Hint{{ + Code: "E001", + Slug: "enum-string-mismatch", + Severity: hints.SeverityError, + Where: hintsLocation(ctx, opTok.Pos), + YouWrote: "'" + lit.Value + "'", + Problem: "Comparing or assigning an Enumeration attribute against " + + "a string literal. In Mendix expressions, enumeration values " + + "must be written as Module.Enum.Value, never as a quoted string.", + Fix: enumQN + "." + lit.Value, + Reference: &hints.Reference{ + Enum: enumQN, + EnumValues: vals, + AttributeName: path.Path[len(path.Path)-1], + }, + }} +} + +// pairEnumPathWithStringLit returns the operands when one side is a string +// literal and the other an attribute path, in either order. +func pairEnumPathWithStringLit(left, right RobustExpr) (*StringLit, *AttributePathExpr) { + if lit, ok := left.(*StringLit); ok { + if path, ok := right.(*AttributePathExpr); ok { + return lit, path + } + } + if lit, ok := right.(*StringLit); ok { + if path, ok := left.(*AttributePathExpr); ok { + return lit, path + } + } + return nil, nil } func parseAdd(s *Stream, ctx Context) (RobustExpr, []Hint) { @@ -521,12 +581,70 @@ func inferKind(e RobustExpr, ctx Context) TypeKind { return KindUnknown case *TokenExpr: return KindString - case *AttributePathExpr, *QNameExpr, *ConstantRef, *RecoveredExpr: + case *AttributePathExpr: + return attributePathKind(n, ctx) + case *QNameExpr, *ConstantRef, *RecoveredExpr: return KindUnknown } return KindUnknown } +// attributePathKind resolves `$Var/Attr` and `$Var/Mod.Assoc/Attr` to the kind +// of the attribute they land on. +// +// Both seams must be present: Entities to type the variable and follow the +// association hops, Catalog to answer what the terminal attribute is. Anything +// unresolvable returns KindUnknown, which suppresses the rule that asked — +// catching less rather than guessing. +func attributePathKind(n *AttributePathExpr, ctx Context) TypeKind { + entity, ok := pathTargetEntity(n, ctx) + if !ok { + return KindUnknown + } + kind, ok := ctx.Catalog.AttributeKind(entity, n.Path[len(n.Path)-1]) + if !ok { + return KindUnknown + } + return kind +} + +// pathTargetEntity walks everything before the final segment and returns the +// entity that segment is a member of. +func pathTargetEntity(n *AttributePathExpr, ctx Context) (string, bool) { + if ctx.Entities == nil || ctx.Catalog == nil || n == nil || len(n.Path) == 0 { + return "", false + } + cur, ok := ctx.Entities.VariableEntity(n.Variable) + if !ok || cur == "" { + return "", false + } + // Every segment but the last is an association hop. Unlike XPath, a Mendix + // expression does not name the intermediate entity, so each one has to be + // resolved rather than read off the path. + for _, seg := range n.Path[:len(n.Path)-1] { + next, ok := ctx.Entities.AssociationTarget(seg, cur) + if !ok { + return "", false + } + cur = next + } + return cur, true +} + +// attributePathEnumQN returns the enumeration a path lands on, when it lands on +// an enumeration attribute. +func attributePathEnumQN(n *AttributePathExpr, ctx Context) (string, bool) { + entity, ok := pathTargetEntity(n, ctx) + if !ok { + return "", false + } + attr := n.Path[len(n.Path)-1] + if kind, ok := ctx.Catalog.AttributeKind(entity, attr); !ok || kind != KindEnumeration { + return "", false + } + return ctx.Catalog.AttributeEnumQN(entity, attr) +} + // checkBoolOperand emits E009 when expr's inferred kind is known and non-Boolean. // op is the operator keyword ("not", "and", "or") used in the hint message. func checkBoolOperand(expr RobustExpr, ctx Context, op string) []Hint { diff --git a/mdl/types/widget_item_aliases.go b/mdl/types/widget_item_aliases.go new file mode 100644 index 000000000..06ecfd3b0 --- /dev/null +++ b/mdl/types/widget_item_aliases.go @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 + +package types + +// ItemPropertyAliases lists alternative MDL property names that resolve to a +// schema property on an object-list item, keyed by +// (widgetID, objectListPropertyKey, itemPropertyKey). +// +// It lives here rather than beside its first consumer because **two** code paths +// need it and they must not disagree: the create path (the pluggable widget +// engine, which writes a column from MDL) and the ALTER path (the page mutator, +// which edits a column already in the document). Those kept separate hand-written +// tables and drifted — `DynamicCellClass` was aliased on create and absent on +// ALTER, so `ALTER PAGE SET DynamicCellClass ON grid.Column` failed with +// "column property not found" for a property the very same tool had just written +// (mendixlabs/mxcli#919). One table cannot drift from itself. +// +// Only genuine renames belong here. A property whose MDL name differs from the +// schema key by case alone (`Sortable` → `sortable`) needs no entry: both +// consumers match case-insensitively against the schema keys the document +// actually declares, so listing those would be a second thing to keep in sync +// for no gain. +var ItemPropertyAliases = map[string]map[string]map[string][]string{ + DataGridWidgetID: { + DataGridColumnsKey: { + "header": {"Caption"}, + "dynamicText": {"Content"}, + // MDL `ColumnWidth: manual` fills the schema's `width` enum. The + // keyword path mapped this (`colPropString(..., "ColumnWidth")`); + // without the alias the engine leaves width at its `autoFill` + // default, so a `Size:` value becomes invalid (size only applies + // when width=manual) and Studio Pro flags CE0463. + "width": {"ColumnWidth"}, + // MDL `DynamicCellClass: ''` fills the schema's `columnClass` + // expression (a per-cell dynamic CSS class). Without the alias the + // engine looks up `columnClass` in the AST property bag, finds + // nothing, and writes an empty expression — the class is silently + // dropped. Bug 10a. + "columnClass": {"DynamicCellClass"}, + }, + }, + "com.mendix.widget.web.heatmap.HeatMap": { + "scaleColors": { + // MDL `ColorValue: '#rrggbb'` fills the schema's `colour` primitive + // (British spelling). Without the alias the engine looks up `colour`, + // doesn't find `ColorValue`, and the scale colour is silently dropped + // on write. Same class as `columnClass` (Bug 10a). + "colour": {"ColorValue"}, + }, + }, +} + +const ( + // DataGridWidgetID is the pluggable DataGrid 2 widget. + DataGridWidgetID = "com.mendix.widget.web.datagrid.Datagrid" + // DataGridColumnsKey is its object-list property holding the columns. + DataGridColumnsKey = "columns" +) + +// ItemPropertyAliasesFor returns the schemaKey → MDL-alias table for one +// object-list slot, or nil when the widget declares none. +func ItemPropertyAliasesFor(widgetID, objectListKey string) map[string][]string { + byList, ok := ItemPropertyAliases[widgetID] + if !ok { + return nil + } + return byList[objectListKey] +} diff --git a/sdk/domainmodel/domainmodel.go b/sdk/domainmodel/domainmodel.go index 59ed78e9c..6b5cf0f89 100644 --- a/sdk/domainmodel/domainmodel.go +++ b/sdk/domainmodel/domainmodel.go @@ -322,7 +322,10 @@ type AttributeValue struct { DefaultValue string `json:"defaultValue,omitempty"` MicroflowID model.ID `json:"microflowId,omitempty"` MicroflowName string `json:"microflowName,omitempty"` // Qualified name (e.g. "Module.Microflow") — BSON stores ByNameReference as string - ViewReference string `json:"viewReference,omitempty"` // OQL column reference for view entity attributes + // PassEntity is DomainModels$CalculatedValue.PassEntity: whether the + // calculation microflow receives the owning entity as its parameter (#917). + PassEntity bool `json:"passEntity,omitempty"` + ViewReference string `json:"viewReference,omitempty"` // OQL column reference for view entity attributes } // Association represents an association between entities.