diff --git a/.claude/commands/mendix/lint.md b/.claude/commands/mendix/lint.md index 636d2b4301..b17a4b3098 100644 --- a/.claude/commands/mendix/lint.md +++ b/.claude/commands/mendix/lint.md @@ -51,6 +51,7 @@ mxcli lint -p app.mpr --exclude System --exclude Administration | CONV013 | quality | ErrorHandlingOnCalls - External calls (REST/WS/Java) need custom error handling | | CONV014 | quality | NoContinueErrorHandling - Don't silently swallow errors with Continue | | CONV020 | quality | CreatedObjectNeverPersisted - A microflow creates a persistent object without commit and never commits, returns or passes it on, so it is discarded | +| CONV021 | quality | CreateChangeThenCommit - A create/change without commit directly followed by a Commit of the same object; set Commit on the create/change instead | ## Bundled Starlark Rules diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-31-lint-rules-only-for-claude-and-opencode.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-31-lint-rules-only-for-claude-and-opencode.json new file mode 100644 index 0000000000..71534b4da3 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-31-lint-rules-only-for-claude-and-opencode.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-31", "symptom": "`mxcli init --tool cursor` (and aider, continue, copilot, vibe, windsurf) installed no Starlark lint rules, so `mxcli lint` ran the built-in Go rules alone; a project copy without .claude/ did the same. No notice, exit 0: 210 findings from 17 rules read as cleaner than 1,984 from 49.", "cause": "init wrote .claude/lint-rules/ only inside the claude and opencode branches, although the lint config next to it was already written outside the per-tool branches for exactly this reason; projectLintRules treated a missing rules directory as an empty one without saying so.", "file": "cmd/mxcli/init.go", "insight": "Anything `mxcli lint` reads is tool-independent and belongs outside the per-tool branches of init; test it by iterating SupportedTools, not by naming the tools that happen to work. A smaller rule set is indistinguishable from a cleaner project in every output, so any path that runs fewer rules than it could must say so on stderr.", "issue": "ako/mxcli#1292"} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-nightly-10-24-only-restclient-query-params-wants-write-refusal.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-nightly-10-24-only-restclient-query-params-wants-write-refusal.json new file mode 100644 index 0000000000..d69aa129d7 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-nightly-10-24-only-restclient-query-params-wants-write-refusal.json @@ -0,0 +1 @@ +{"area":"mdl/executor","date":"2026-10-31","symptom":"The nightly goes red on the Mendix 10.24.24 leg only, in `TestRoundtripRestClient_WithQueryParams`: `want the QueryParameters refusal, got: query: ( … ) on a REST operation requires Mendix 11.0.0+ (project is 10.24.24.119349)`. Every 11.x leg, push CI and the unit tests are green","cause":"Not a product defect: the statement is still refused below 11.0, by a different layer. 9aa1df3e (ako/mxcli#1811 sweep) added a check-time gate, `rest_client_query_params` in documentTypeGates, which refuses the statement before the write is reached, while the test pinned the write path's refusal text (`Rest$RestOperation.QueryParameters (Mendix 11.0.0+)`). Only a pre-11.0 project takes that branch (`restClientHasMendix11Shapes`), and per-PR CI runs one newer version, so the PR that added the gate never ran it","file":"`mdl/executor/roundtrip_restclient_test.go` (`TestRoundtripRestClient_WithQueryParams`); the gate is `mdl/executor/version_gates_documents.go`","insight":"A test that pins a write-time refusal is a bet that nothing refuses earlier, and moving a refusal to check time is exactly the direction CLAUDE.md asks for, so every check-time gate added should grep the integration tests for the write path's message (`cannot store .` / `. (Mendix X+)`) and move those assertions to the gate's ` requires Mendix X+`. The 9.24 leg cannot show this one: there `create rest client` itself is gated (10.1), which fires first and was already among that leg's 69 known failures, the same count before and after the gate landed. Reproduce in about 10s: `mxcli setup mxbuild --version 10.24.24.119349`, then `MX_BINARY=~/.mxcli/mxbuild/10.24.24.119349/modeler/mx go test -tags integration -run 'TestRoundtripRestClient_WithQueryParams$' ./mdl/executor/`. Controls: the old assertion fails at line 151 with the nightly's exact message; the new one passes, and fails with the write path's `cannot store Rest$RestOperation.QueryParameters (Mendix 11.0.0+)` when the gate's applies is stubbed to false","refs":["mendixlabs/mxcli nightly run 41183093941","ako/mxcli nightly run 41182765181","ako/mxcli#1811"],"test":"mdl/executor/roundtrip_restclient_test.go"} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-optional-page-parameter-invisible.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-optional-page-parameter-invisible.json new file mode 100644 index 0000000000..de0da62e23 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-optional-page-parameter-invisible.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-31", "symptom": "A page parameter Studio Pro marks not Required (Forms$PageParameter.IsRequired = false, Mendix 11.5+), opened by a menu item with no argument (mx check 0 errors): describe page printed `Params: ( $Sale: MyFirstModule.Sale )`, describe navigation printed `show page MyFirstModule.Sale_View(Sale = $currentObject)`, and executing that output was refused with MDL-PAGEARG01 — the navigation profile could not be changed from MDL (Mendix 11.12.5)", "cause": "MDL had no spelling for the flag (the builder hard-coded IsRequired true), describe page did not print it, and pageActionParameters synthesised `= $currentObject` for every target parameter when no mapping is stored — right for a required one inside a data widget, wrong for an optional one on a context-less menu item. The reader could not be trusted either: gen's IsRequired() reads an absent key as false, so every pre-11.5 parameter would have looked optional", "file": "mdl/grammar/domains/MDLPage.g4 (pageParameter OPTIONAL?), MDLLexer.g4 + keyword rule; mdl/visitor/visitor_page.go; mdl/executor/cmd_pages_builder_v3.go (IsRequired: !Optional; snippet refusal); cmd_alter_page.go + mdl/backend/modelsdk/page_mutator_write.go (alter add parameters); version_gates_documents.go + sdk/versions/mendix-11.yaml (page_parameter_optional 11.5.0); validate_snippet_parameters.go (MDL087); mdl/backend/modelsdk/page.go (pageParameterIsRequired reads the raw key); cmd_pages_describe.go / cmd_pages_describe_output.go", "fix": "`Params: ( $P: Type optional )`, written as IsRequired false through CREATE and ALTER … ADD PARAMETERS, gated to 11.5+ (MDL-VERSION01) and refused on a snippet parameter (MDL087); describe page prints `optional`; the implicit `= $currentObject` mapping is only for required parameters; the reader treats an absent IsRequired as required", "insight": "Before trusting a gen bool accessor for a property newer than the oldest supported version, check what it returns for an absent key: Primitive[bool] gives false, which here would have flipped every pre-11.5 parameter to optional — read element.Raw() and treat absent as the platform's old behaviour; the control (revert to gp.IsRequired()) fails the no-stored-flag test. The real check settles both directions on the 11.6.6 fixture: menu item with no argument → 0 errors with `optional`, CE1571 without. No fixture page (58 parameters in TestApp + PedApp) is optional, so the Studio Pro shape was seeded. Left out: a check for the required case (`show page P` with no argument on a context-less widget is CE1571 only at mx check), and parameter DefaultValue (11.5), which has no spelling and no fixture evidence", "refs": ["mendixlabs/mxcli#1640", "ako/mxcli#1649"], "ce": ["CE1571"], "rules": ["MDL-PAGEARG01", "MDL-VERSION01", "MDL087"], "test": "TestIssue1649_OptionalPageParameterIsWrittenAndDescribed, TestIssue1649_DescribeNavigationOpensAnOptionalParameterPageWithNoArgument, TestIssue1649_AParameterWithNoStoredFlagIsRequired, TestIssue1649_OptionalIsRefusedWhereItCannotBeStored, TestIssue1649_AlterAddsAnOptionalParameter (mdl/executor/issue1649_optional_page_parameter_test.go); mdl-examples/bug-tests/1640-optional-page-parameter.mdl"} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-pluggable-expression-default-written-as-primitive.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-pluggable-expression-default-written-as-primitive.json new file mode 100644 index 0000000000..c8662e778a --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-pluggable-expression-default-written-as-primitive.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-31", "symptom": "Every combo box mxcli wrote stored customEditabilityExpression with PrimitiveValue \"false\" as well as Expression \"false\"; Studio Pro stores PrimitiveValue \"\" (ako/TestApp Rules.RuleAction_NewEdit). mx check accepted both, so mxcli's widgets differed from Studio Pro's and a page saved by one never re-ran Unchanged against the other", "cause": "A property the widget's editorConfig hides is reset to its declared default (hiddenUnnamedProperties / unmappedHiddenResets in PluggableWidgetEngine.Build), and both reset sites called builder.SetPrimitive whatever the property's value type. The template and the .mpk augment were already right (PrimitiveValue \"\"); the explicit `key: value` path routes by ValueType and was right too", "file": "mdl/executor/widget_engine.go (resetToDeclaredDefault, used by the two hidden-property reset sites)", "fix": "Write the declared default into the slot its value type keeps it in: SetExpression for an Expression property (PrimitiveValue left as the template's \"\"), SetPrimitive otherwise", "insight": "Measure the slots by the property's declared ValueType, read from the widget's own Type, not by a $Type pattern: over 221 TestApp + PedApp pages, snippets and layouts, 939 Expression-typed WidgetValues fill PrimitiveValue 0 times, and no other type fills Expression — strictly exclusive, which is what made the fix safe against CE0463. Bisect the pipeline before reading it: GetTemplateFullBSON already produced PrimitiveValue \"\", which put the defect after template load and pointed straight at the two SetPrimitive reset calls. mx check passes with and without the fix (control), so the test has to assert the stored slot, not mx", "refs": ["ako/mxcli#548"], "test": "TestIssue548_ExpressionPropertyIsNotWrittenAsAPrimitive (mdl/executor/issue548_expression_slot_test.go); mdl-examples/bug-tests/548-pluggable-expression-slot.mdl"} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-source-catalog-widget-header-race.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-source-catalog-widget-header-race.json new file mode 100644 index 0000000000..67621dce08 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-31-source-catalog-widget-header-race.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-31", "symptom": "`refresh catalog full source force` on an unchanged project described a project pluggable widget as `pluggablewidget '' name` on some runs and by its keyword on others; once cached, the incremental build kept the wrong row.", "cause": "captureDescribeParallel built a fresh ExecContext per describe, so each of up to eight workers loaded its own widget registry; one unstamped .def.json (hand-made, or its .mpk gone) made every load call RefreshWidgetDefinitions(force), which truncated and rewrote every definition (os.WriteFile) while the other workers read them. A torn read aborted LoadUserDefinitions, its error discarded, so that describe's registry lacked the widget and pluggableWidgetHeader fell back as designed.", "file": "mdl/executor/cmd_catalog.go", "insight": "The tell was the SHAPE: two of twenty widgets, all on one page — a per-describe input going bad, not a broken lookup (that hits all twenty). It did not reproduce on a clean project; it needs a definition that stays stale forever. `strace -e trace=openat` settled it in one run: 16 writes and 30 reads of one .def.json during a single build. A per-goroutine clone of a context silently re-runs every lazy loader it holds — copy the loaded state in, and load it before the fan-out.", "issue": "ako/mxcli#1799"} diff --git a/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-message-definition-references.json b/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-message-definition-references.json new file mode 100644 index 0000000000..24359e1207 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-other/2026-10-31-message-definition-references.json @@ -0,0 +1 @@ +{"area": "mdl/catalog", "date": "2026-10-31", "symptom": "`show references to` an entity, attribute or association never listed a message definition that exposes it (EvoraICX: EvoraCore.MD_ResolutionStep exposes InvestigationTask and its 15 attributes, 0 edges), so renaming or deleting one looked safe.", "cause": "memberRefSourceTypes, the closed list of unit types the raw member-ref walk visits, had the two mapping types but neither message-definition type, although a message definition stores its Entity/Attribute/Association by qualified name exactly as a mapping does.", "file": "mdl/catalog/builder_member_refs.go", "insight": "When a document type stores members by qualified name, the raw walk needs only a row in memberRefSourceTypes (plus the entity case alongside the mappings); no per-type extractor. List the document types the gen package registers RefByName props for (codec.DefaultRefRegistry) and diff against memberRefSourceTypes to find the next gap. TestSkillDocumentsRealRefObjectTypes checks only that documented values exist, not that every source type is documented, so the lint-rule doc must be updated by hand.", "issue": "ako/mxcli#1781"} diff --git a/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-cold-unit-index-notice.json b/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-cold-unit-index-notice.json new file mode 100644 index 0000000000..f878d6a16b --- /dev/null +++ b/.claude/skills/fix-issue/findings/modelsdk/2026-10-31-cold-unit-index-notice.json @@ -0,0 +1 @@ +{"area": "modelsdk/mpr", "date": "2026-10-31", "symptom": "The first command after upgrading to the unit-index-2 format (or on a fresh clone) opened every unit file to build the index — 641 opens where the previous build opened 383 — with no explanation, a visible stall on Windows where an open costs about a millisecond.", "cause": "By design the index is a pure cache and a cold build reads everything; nothing said it was happening. The new format stores Name and Excluded, which the old unit-types.idx never had, so it cannot be seeded from the old file.", "file": "modelsdk/mpr/reader_units_v2.go", "insight": "A one-time cost a user cannot see reads as a regression; one line on stderr turns it into an expected step. Keep the notice in the library behind a nil-by-default writer (mpr.ColdUnitIndexNotice) that the CLI sets with the banner, so quiet mode and library callers stay silent, and count units before buildUnitCache opens its row query rather than during it.", "issue": "ako/mxcli#1806"} diff --git a/.claude/skills/mendix/assess-quality/SKILL.md b/.claude/skills/mendix/assess-quality/SKILL.md index c232305bed..6a879c8caa 100644 --- a/.claude/skills/mendix/assess-quality/SKILL.md +++ b/.claude/skills/mendix/assess-quality/SKILL.md @@ -12,14 +12,14 @@ description: "Audit a Mendix project against best practices for naming, security |---|---| | 29-36 | When to Use This Skill | | 37-96 | Assessment Workflow | -| 97-353 | Quality Guidelines Reference | -| 354-364 | Assessment Report Template | -| 365-377 | Executive Summary | -| 378-388 | project Metrics | -| 389-401 | Automated Findings | -| 402-421 | Manual Review Findings | -| 422-437 | Recommendations | -| 438-448 | Sources | +| 97-354 | Quality Guidelines Reference | +| 355-365 | Assessment Report Template | +| 366-378 | Executive Summary | +| 379-389 | project Metrics | +| 390-402 | Automated Findings | +| 403-422 | Manual Review Findings | +| 423-438 | Recommendations | +| 439-449 | Sources | # Assess Mendix Project Quality @@ -200,7 +200,7 @@ The Dutch Institute for Vulnerability Disclosure (DIVD) found widespread authori ### D. Maintainability -**Enforced by rules:** QUAL001-QUAL004, CONV009, CONV012, CONV014, CONV020, MPR002, MPR004, MPR006 +**Enforced by rules:** QUAL001-QUAL004, CONV009, CONV012, CONV014, CONV020, CONV021, MPR002, MPR004, MPR006 | Guideline | Rule/Check | Threshold | |-----------|-----------|-----------| @@ -212,6 +212,7 @@ The Dutch Institute for Vulnerability Disclosure (DIVD) found widespread authori | Caption on exclusive splits | CONV012 | All decision points must have captions | | No "Continue" error handling | CONV014 | Never swallow errors silently | | No created object silently discarded | CONV020 | A persistent object created without commit is committed, returned or passed on | +| Commit on the create/change, not after it | CONV021 | No Create/Change object activity directly followed by a Commit of the same object | | ACT_ microflows: thin controllers | CONV010 | Only page activities + submicroflow calls | **Manual checks:** diff --git a/.claude/skills/mendix/write-lint-rules/SKILL.md b/.claude/skills/mendix/write-lint-rules/SKILL.md index 0841c61aef..67adc624be 100644 --- a/.claude/skills/mendix/write-lint-rules/SKILL.md +++ b/.claude/skills/mendix/write-lint-rules/SKILL.md @@ -561,13 +561,13 @@ Returned by `permissions()` (all types) or `permissions_for()` (entity-specific) ### reference | Property | Type | Example | |----------|------|---------| -| `source_type` | string | The document the edge comes FROM, upper-case: `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"SNIPPET"`, `"ENTITY"`, `"ASSOCIATION"`, `"WORKFLOW"`, `"NAVIGATION"`, `"SCHEDULED_EVENT"`, `"PUBLISHED_REST_OPERATION"`, `"PUBLISHED_REST_SERVICE"`, `"PUBLISHED_ODATA_SERVICE"`, `"CONSUMED_ODATA_SERVICE"`, `"BUSINESS_EVENT_SERVICE"`, `"PROJECT_SETTINGS"`, `"IMPORT_MAPPING"`, `"EXPORT_MAPPING"` | +| `source_type` | string | The document the edge comes FROM, upper-case: `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"SNIPPET"`, `"ENTITY"`, `"ASSOCIATION"`, `"WORKFLOW"`, `"NAVIGATION"`, `"SCHEDULED_EVENT"`, `"PUBLISHED_REST_OPERATION"`, `"PUBLISHED_REST_SERVICE"`, `"PUBLISHED_ODATA_SERVICE"`, `"CONSUMED_ODATA_SERVICE"`, `"BUSINESS_EVENT_SERVICE"`, `"PROJECT_SETTINGS"`, `"IMPORT_MAPPING"`, `"EXPORT_MAPPING"`, `"MESSAGE_DEFINITION_COLLECTION"`, `"MESSAGE_DEFINITION"` | | `source_id` | string | Source UUID | | `source_name` | string | `"Sales.ACT_Customer_Create"` | | `target_type` | string | What it points AT, upper-case: `"ENTITY"`, `"ASSOCIATION"`, `"MICROFLOW"`, `"NANOFLOW"`, `"RULE"`, `"PAGE"`, `"LAYOUT"`, `"WORKFLOW"`, `"WIDGET"`, `"JAVA_ACTION"`, `"JAVASCRIPT_ACTION"`, `"REST_OPERATION"`, `"REGULAR_EXPRESSION"`, `"ATTRIBUTE"`, `"ENUMERATION"`, `"ENUMERATION_VALUE"`. `LAYOUT`, `WIDGET`, `ATTRIBUTE`, `ENUMERATION` and `ENUMERATION_VALUE` are only ever targets; `SCHEDULED_EVENT`, `PROJECT_SETTINGS` and the service types only ever sources | | `target_id` | string | Target UUID | | `target_name` | string | `"Sales.Customer"`; three-part for an attribute or an enumeration value: `"Sales.Order.Total"`, `"Sales.OrderStatus.Open"` | -| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"commit"` (a commit action, or a create/change that commits — beside its `"create"`/`"change"` edge; a commit of a variable whose entity the flow cannot tell has no edge), `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"` (a published REST operation, OData resource or action, or a service's authentication microflow), `"event"` (an entity event or a business event handler), `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity), `"split"` (a decision branches on an object being the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | +| `ref_kind` | string | How it references: `"call"`, `"create"`, `"retrieve"`, `"change"`, `"delete"`, `"commit"` (a commit action, or a create/change that commits — beside its `"create"`/`"change"` edge; a commit of a variable whose entity the flow cannot tell has no edge), `"show_page"`, `"datasource"`, `"action"`, `"layout"`, `"parameter"`, `"return"`, `"generalize"`, `"associate"`, `"home_page"`, `"login_page"`, `"menu_item"`, `"calculate"`, `"schedule"`, `"validate"`, `"settings"`, `"widget"`, `"sync"`, `"publish"` (a published REST operation, OData resource or action, or a service's authentication microflow), `"event"` (an entity event or a business event handler), `"member"` (binds/reads/writes an attribute or navigates an association), `"xpath"` (an XPath constraint names it), `"type"` (typed as an enumeration), `"value"` (an expression names an enumeration value), `"mapping"` (an import/export mapping maps the entity, or a message definition exposes it), `"split"` (a decision branches on an object being the entity) — lower-case, unlike the types above. Attribute names used only through a variable in a free-text expression (`$Order/Total`) have no edge | | `module_name` | string | Source module | ### project_security diff --git a/CHANGELOG.md b/CHANGELOG.md index 3def47fc56..90708168cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,12 +6,28 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### Added + +- **Optional page parameters** (mendixlabs/mxcli#1640, ako/mxcli#1649) — `Params: ( $Sale: Shop.Sale optional )` is Studio Pro's unticked *Required* (`Forms$PageParameter.IsRequired = false`, Mendix 11.5+): the page can be opened with no argument, from a menu item for example, where a required parameter is CE1571. `create page`, `alter page … add parameters` and `describe page` all spell it. `describe navigation` no longer invents `(Sale = $currentObject)` for a menu item opening such a page, which made its output one `exec` refused (MDL-PAGEARG01) and left the navigation profile unchangeable from MDL. Below 11.5 `optional` is refused (MDL-VERSION01), and a snippet parameter cannot be optional (MDL087). + +### Changed + +- **The first command on a project says it is building the unit index** (ako/mxcli#1806) — after an upgrade to the `unit-index-2.idx` format, or on a fresh clone, one command opens every unit file to build the index, which on Windows is a visible stall. It now prints one line on stderr (`Building the unit index for this project's N units (once; …)`); `-q` / `MXCLI_QUIET` keep it silent. + +### Fixed + +- **Every `mxcli init --tool ` installs the bundled lint rules, and `lint` says when it runs without them** (ako/mxcli#1292, mendixlabs/mxcli#1242) — only `claude` and `opencode` wrote `.claude/lint-rules/`, so `init --tool cursor` (or aider, continue, copilot, vibe, windsurf) left `mxcli lint` running the built-in Go rules alone, silently. `lint` and `report` now print a warning when no `.claude/lint-rules/` is found, naming how many rules run instead. +- **`show references to` an entity, attribute or association lists the message definitions that expose it** (#1781) — a message definition (the pre-11.15 collection, `MESSAGE_DEFINITION_COLLECTION`, and the 11.15+ document, `MESSAGE_DEFINITION`) was not walked for references, so impact analysis called an exposed entity or attribute unused. The entity edges are `mapping`, the attribute and association edges `member`, as for an import or export mapping. +- **A pluggable widget's hidden expression property is stored as Studio Pro stores it** (ako/mxcli#548) — a property the widget's editorConfig hides is written at its declared default, and for an Expression-typed one that default went into `PrimitiveValue` as well as `Expression` (every combo box's `customEditabilityExpression`, `PrimitiveValue: "false"`). Studio Pro leaves `PrimitiveValue` empty on all 939 Expression-typed values of ako/TestApp and PedApp; mx check accepted both, so the cost was a widget that differed from Studio Pro's. +- **A source catalog build describes a project widget the same way every run** (#1799) — `refresh catalog full source` sometimes wrote a pluggable widget in the generic `pluggablewidget ''` form instead of its own keyword (`vegachart …`), and since #1802 an incremental build kept that row until a `force`. Each parallel describe loaded its own widget registry, and with one definition that never counts as current (hand-made, or its `.mpk` removed), each load rewrote every `.def.json` while the other describes read them. The describes now share the build's registry, and a definition is replaced by rename, so no reader sees a half-written file. + ## [0.29.0] - 2026-10-30 Headline: **Write what the project's own Mendix version stores, and commit it the way Studio Pro does.** Every write is now checked against the element types and property shapes of the project's Mendix version, so a statement 9.x or 10.x cannot store is refused by `check -p` instead of leaving a project Studio Pro cannot open, and `mxcli git commit` / `git doctor` keep a plain-git history in the shape Studio Pro expects. Alongside it: workflows, pages and Java actions gain the settings a rewrite used to reset, `alter microflow` edits inside loop bodies, the debugger works end to end, and commands on large MPR v2 projects open only the files they need. ### Added +- **Lint rule CONV021 (CreateChangeThenCommit)** — warns when a microflow's Create or Change object activity without commit is directly followed by a Commit of the same object, and suggests the one activity that does both: Commit set on the create/change (Yes, or Yes without events for a commit without events; Refresh in client kept). It stays quiet when merging could change behaviour: another flow into the commit, an error-handler flow between them, or custom error handling on either. - **`mxcli git commit -m … -p app.mpr` — commit the way Studio Pro does** (mendixlabs/mxcli#1669) — Studio Pro's *Commit* is more than `git commit`: it keeps the ContentsHash index in step, stages exactly the files a Mendix repository ships, and attaches the mx_metadata note. The new command is that sequence in one line: mismatched hashes are repaired from the files on disk first (`MISSING` and `ORPHAN` entries have no automatic repair and refuse the commit), everything is staged and the files Studio Pro never commits (`project-settings.user.json`, `*.mpr.lock`, `*.mpr.bak`, and the project's own `.mendix-cache/`, `deployment/`, `theme-cache/`, `.mxcli/` beside the `.mpr` — a folder of the same name further down, such as `resources/deployment/`, is committed) are unstaged again, `git commit` runs with the caller's message and identity, and the mx_metadata note is computed from the new commit and attached, so Studio Pro can version-check the revision and show what changed. `--push` pushes the branch and the notes ref together (`git push` alone loses the note the next time Studio Pro fetches); `--dry-run` reports what it would commit without committing — and without repairing anything, hash drift included, because a dry run writes nothing: it stages into a copy of the index, so what the user had staged is left as it was. The commit is refused while Studio Pro has the project open (`.mpr.lock`) — honouring the same `MXCLI_ALLOW_STUDIO_PRO_OPEN=1` override as every write — and during an unresolved merge, which `git add -A` would otherwise commit with its conflict markers. Docs: "Working Outside Studio Pro" › *One commit, all the steps*. - **`mxcli git doctor [-p app.mpr] [--fix]` — audit a repository for everything a plain-git history misses, and repair it** (mendixlabs/mxcli#1669) — a repository committed only with git drifts from what Studio Pro expects in four ways, each invisible on its own: the local git config Studio Pro writes is absent (`core.autocrlf=False` keeps a checkout from rewriting line endings inside `.mxunit` files on Windows; `mendix.commits-since-gc` and `mendix.lineEndingResetDone` are its own bookkeeping), the remote is SSH which Studio Pro cannot use (HTTPS only), commits have no note under `refs/notes/mx_metadata` (history shows `(unknown)`, the version check fails), and a malformed note Studio Pro cannot read at all. `doctor` reports all four in one read-only pass and exits 1 while an issue remains, so it can gate CI. `--fix` sets the config keys, rewrites SSH remote URLs (`git@host:org/app.git`, `ssh://…`) to HTTPS — judged as configured, so a `url..insteadOf` rule in the user's git config (https → `git@` is a common one) is reported rather than "repaired" on every run — and backfills notes: computed from the model with `-p` (the same engine `git note` uses), or a template note with a `--version` ModelerVersion where the model at a commit cannot be read (MPR v1 history, a revision without the `.mpr`); with neither, the newest valid note in history supplies the version. Notes Studio Pro wrote are never touched, with or without `--fix`; `--fix` does not push. A clone whose notes ref lacks the remote's notes — `git clone` never fetches `refs/notes/*`, so every fresh clone and CI checkout — is reported as exactly that, with the fetch command, rather than as a history of unannotated commits; `--fix` fetches the notes first, as a fast-forward only. Docs: "Working Outside Studio Pro" › *Auditing a repository*. - **Workflows say what Studio Pro lets them say** — a user task takes a `task name '…'` clause (what the task inbox shows, until now always the caption), and the workflow's display and description and a task's name and description take template parameters, `display 'Approval for {1}' with ({1} = $WorkflowContext/Title)`, in CREATE and ALTER. A timer boundary event takes `caption '…'`, and a non-interrupting timer can recur: `repeat every 1 day at most 3 times`. `alter workflow … set (Annotation: '…')` sets the workflow's note, and `… on ` an activity's. A `describe` → `create or modify` that would still reset a setting MDL cannot state is refused, naming the setting, instead of reporting "Modified workflow". diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index e42ce6d8e0..d0ff36e519 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -77,6 +77,7 @@ Bundled Starlark rules (in .claude/lint-rules/): - No event handlers (CONV016) - avoid entity event handlers - No calculated attributes (CONV017) - avoid calculated attributes - Created object never persisted (CONV020) - a created object that is never committed, returned or passed on + - Create/change then commit (CONV021) - a create/change directly followed by a commit of the same object; set Commit on it instead Custom Starlark rules in .claude/lint-rules/*.star are loaded automatically. @@ -435,6 +436,7 @@ func builtinLintRules() []linter.Rule { rules.NewErrorHandlingOnCallsRule(), rules.NewNoContinueErrorHandlingRule(), rules.NewCreatedObjectNeverPersistedRule(), // CONV020 - create without commit, never persisted or passed on + rules.NewCreateChangeThenCommitRule(), // CONV021 - create/change then a commit of the same object rules.NewIrreducibleFlowGraphRule(), // MDL-FLOW01 - graph structure vs MDL nesting rules.NewImportRangeObjectMappingRule(), // MDL-MAP04 - First over an object-rooted mapping in the model } diff --git a/cmd/mxcli/init.go b/cmd/mxcli/init.go index f5426e308f..e0d64e3ea6 100644 --- a/cmd/mxcli/init.go +++ b/cmd/mxcli/init.go @@ -231,20 +231,15 @@ Container Runtime: } // Create .claude directory for Claude-specific content (if Claude is selected) - var claudeDir, commandsDir, lintRulesDir string + var claudeDir, commandsDir string if slices.Contains(tools, "claude") { claudeDir = filepath.Join(absDir, ".claude") commandsDir = filepath.Join(claudeDir, "commands") - lintRulesDir = filepath.Join(claudeDir, "lint-rules") if err := os.MkdirAll(commandsDir, 0755); err != nil { fmt.Fprintf(os.Stderr, "Error creating .claude/commands directory: %v\n", err) os.Exit(1) } - if err := os.MkdirAll(lintRulesDir, 0755); err != nil { - fmt.Fprintf(os.Stderr, "Error creating .claude/lint-rules directory: %v\n", err) - os.Exit(1) - } } // Create .opencode directory for OpenCode-specific content (if OpenCode is selected) @@ -262,18 +257,6 @@ Container Runtime: fmt.Fprintf(os.Stderr, "Error creating .opencode/skills directory: %v\n", err) os.Exit(1) } - - // Lint rules stay in .claude/lint-rules/ (read by mxcli lint). - // Ensure that directory exists even when claude tool is not selected. - if !slices.Contains(tools, "claude") { - if lintRulesDir == "" { - lintRulesDir = filepath.Join(absDir, ".claude", "lint-rules") - } - if err := os.MkdirAll(lintRulesDir, 0755); err != nil { - fmt.Fprintf(os.Stderr, "Error creating .claude/lint-rules directory: %v\n", err) - os.Exit(1) - } - } } // Seed the lint config. `mxcli lint` reads this regardless of which AI @@ -282,6 +265,17 @@ Container Runtime: fmt.Printf(" Created %s (System module excluded from lint)\n", filepath.Base(path)) } + // The bundled Starlark rules, for the same reason. Only claude and + // opencode wrote them, so `init --tool cursor` left lint running the Go + // rules alone — 17 rules instead of 49 on one project, silently + // (ako/mxcli#1292). + lintRulesDir := filepath.Join(absDir, ".claude", "lint-rules") + if err := os.MkdirAll(lintRulesDir, 0755); err != nil { + fmt.Fprintf(os.Stderr, "Error creating .claude/lint-rules directory: %v\n", err) + os.Exit(1) + } + initBundledLintRules(lintRulesDir) + // Write the skills. One code path with the SessionStart refresh, so a // fresh init and an upgraded project end up with byte-identical trees — // including the `.claude/skills/` copy Claude Code actually scans. @@ -344,7 +338,7 @@ Container Runtime: fmt.Printf(" Created %s\n", file.Path) } - // Claude-specific: write commands and lint rules + // Claude-specific: write commands if toolName == "claude" && commandsDir != "" { cmdCount := 0 err = fs.WalkDir(commandsFS, "commands", func(path string, d fs.DirEntry, err error) error { @@ -370,11 +364,9 @@ Container Runtime: } else { fmt.Printf(" Created %d command files in .claude/commands/\n", cmdCount) } - - initBundledLintRules(lintRulesDir) } - // OpenCode-specific: write commands, lint rules, and skills + // OpenCode-specific: write commands and skills if toolName == "opencode" && opencodeCommandsDir != "" { cmdCount := 0 err = fs.WalkDir(commandsFS, "commands", func(path string, d fs.DirEntry, err error) error { @@ -401,13 +393,6 @@ Container Runtime: fmt.Printf(" Created %d command files in .opencode/commands/\n", cmdCount) } - // Only write lint rules from the OpenCode path when Claude is not also - // being initialised — the Claude path already writes the same files to - // .claude/lint-rules/ and we don't want duplicate log output or writes. - if !slices.Contains(tools, "claude") { - initBundledLintRules(lintRulesDir) - } - skillCount2, err := writeWrappedSkills(opencodeSkillsDir, wrapSkillContent) if err != nil { fmt.Fprintf(os.Stderr, " Error writing OpenCode skills: %v\n", err) diff --git a/cmd/mxcli/init_lint_rules_every_tool_test.go b/cmd/mxcli/init_lint_rules_every_tool_test.go new file mode 100644 index 0000000000..0e97392386 --- /dev/null +++ b/cmd/mxcli/init_lint_rules_every_tool_test.go @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "io/fs" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// `mxcli lint` reads Starlark rules whichever AI tool a project uses, so every +// `init --tool ` must give it the same rule set. Only claude and opencode +// wrote them: `init --tool cursor` installed none, and lint then ran the Go +// rules alone, silently, with exit 0 (mendixlabs/mxcli#1242, ako/mxcli#1292). +func TestInitInstallsTheBundledLintRulesForEveryTool(t *testing.T) { + bundled := 0 + _ = fs.WalkDir(lintRulesFS, "lint-rules", func(p string, d fs.DirEntry, err error) error { + if err == nil && !d.IsDir() && strings.HasSuffix(p, ".star") { + bundled++ + } + return nil + }) + if bundled == 0 { + t.Fatal("no bundled .star rules embedded — the test would prove nothing") + } + + tools := make([]string, 0, len(SupportedTools)) + for name := range SupportedTools { + tools = append(tools, name) + } + sort.Strings(tools) + for _, tool := range tools { + t.Run(tool, func(t *testing.T) { + dir := t.TempDir() + runInit(t, []string{tool}, dir) + + found := linter.FindLintRulesDir(dir) + if found == "" { + t.Fatalf("init --tool %s: lint finds no rules directory, so it would run the Go rules only", tool) + } + rules, failures, err := linter.LoadStarlarkRulesFromDir(found) + if err != nil || len(failures) > 0 { + t.Fatalf("loading %s: %v %v", found, err, failures) + } + if len(rules) != bundled { + t.Errorf("init --tool %s: lint loads %d Starlark rules from %s, want the %d bundled ones", + tool, len(rules), filepath.Base(filepath.Dir(found))+"/"+filepath.Base(found), bundled) + } + }) + } +} + +// With no rules directory, lint and report run the built-in Go rules only. That +// is a legitimate state (a scratch copy of the model, a CI checkout without +// .claude/), but it was silent and exited 0: 210 findings from 17 rules read as +// a clean result next to 1,984 from 49 (ako/mxcli#1292). Say so. +func TestProjectLintRulesSaysWhenNoStarlarkRulesAreFound(t *testing.T) { + bare := t.TempDir() + var warn strings.Builder + rules := projectLintRules(bare, &warn) + if len(rules) != len(builtinLintRules()) { + t.Fatalf("bare project: %d rules, want only the %d built-in ones", len(rules), len(builtinLintRules())) + } + if !strings.Contains(warn.String(), "no .claude/lint-rules/") { + t.Errorf("bare project: no notice that the Starlark rules are absent; stderr was %q", warn.String()) + } + + // Control: a project with the rules says nothing about them missing. + withRules := t.TempDir() + runInit(t, []string{"cursor"}, withRules) + warn.Reset() + if rules := projectLintRules(withRules, &warn); len(rules) <= len(builtinLintRules()) { + t.Fatalf("initialised project: %d rules, want more than the %d built-in ones", len(rules), len(builtinLintRules())) + } + if strings.Contains(warn.String(), "no .claude/lint-rules/") { + t.Errorf("initialised project warned that no rules were found: %q", warn.String()) + } +} diff --git a/cmd/mxcli/lint_setup.go b/cmd/mxcli/lint_setup.go index db1d8bf631..9da455bd64 100644 --- a/cmd/mxcli/lint_setup.go +++ b/cmd/mxcli/lint_setup.go @@ -5,6 +5,7 @@ package main import ( "fmt" "io" + "path/filepath" "github.com/mendixlabs/mxcli/mdl/linter" ) @@ -26,6 +27,19 @@ func projectLintRules(projectDir string, warn io.Writer) []linter.Rule { // Searched upward from the project, so one directory at the repo root // serves an app in a subfolder (#904). lintRulesDir := linter.FindLintRulesDir(projectDir) + if lintRulesDir == "" { + from := projectDir + if abs, err := filepath.Abs(projectDir); err == nil { + from = abs + } + // Legitimate (a scratch copy of the model, a CI checkout without + // .claude/), but it used to be silent: the smaller result read as a + // cleaner project — 210 findings from 17 rules instead of 1,984 from 49 + // (ako/mxcli#1292). + fmt.Fprintf(warn, "Warning: no .claude/lint-rules/ directory was found from %s up to the repository root, "+ + "so only the %d built-in rules run. Run 'mxcli init' to install the bundled Starlark rules.\n", + from, len(lintRules)) + } starlarkRules, loadFailures, err := linter.LoadStarlarkRulesFromDir(lintRulesDir) if err != nil { // Previously discarded in `lint`, which made an unreadable directory diff --git a/cmd/mxcli/lsp_completions_gen.go b/cmd/mxcli/lsp_completions_gen.go index c71fdfd42e..1e95c59e03 100644 --- a/cmd/mxcli/lsp_completions_gen.go +++ b/cmd/mxcli/lsp_completions_gen.go @@ -396,6 +396,7 @@ var mdlGeneratedKeywords = []protocol.CompletionItem{ {Label: "RULE", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, {Label: "REQUIRED", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, {Label: "NULLABLE", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, + {Label: "OPTIONAL", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, {Label: "LOCALIZED", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, {Label: "ERROR", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, {Label: "IGNORE", Kind: protocol.CompletionItemKindKeyword, Detail: "Validation keyword"}, diff --git a/cmd/mxcli/main.go b/cmd/mxcli/main.go index 7221997c9d..ce1dba1fc6 100644 --- a/cmd/mxcli/main.go +++ b/cmd/mxcli/main.go @@ -16,6 +16,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/executor" "github.com/mendixlabs/mxcli/mdl/repl" "github.com/mendixlabs/mxcli/mdl/visitor" + mpr "github.com/mendixlabs/mxcli/modelsdk/mpr" "github.com/spf13/cobra" ) @@ -31,6 +32,10 @@ func main() { // Show warning banner unless --quiet, -q, --help, -h, or --version is passed if !shouldSuppressWarning() { fmt.Fprint(os.Stderr, warningBanner) + // The first command on a project (after an upgrade, or on a fresh + // clone) opens every unit file to build the unit index; say so + // rather than stall unexplained (ako/mxcli#1806). + mpr.ColdUnitIndexNotice = os.Stderr } // Open the process session before cobra sees argv. Cobra validates a diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index e1efd5d29f..10cd58bde3 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -12,7 +12,7 @@ func init() { "page", "pages", "form", "UI", "user interface", "widget", "layout", "screen", }, - Syntax: "CREATE PAGE Module.Name [FOLDER 'FolderPath']\n (\n Title: 'Page Title',\n Layout: Module.LayoutName\n [, Params: ( $Param: Module.Entity )]\n [, Url: 'page-url']\n [, Variables: ( $var: Boolean = true )] -- the default is an expression, written bare\n [, PopupWidth: 800, PopupHeight: 480, PopupResizable: true]\n [, PopupCloseAction: cancelButton1]\n [, Class: 'css-class', Style: 'css: rule']\n [, DesignProperties: ('Spacing top': 'Inner large')] -- the page's own; Atlas\n -- declares them under Document and Page\n )\n {\n -- widgets\n }", + Syntax: "CREATE PAGE Module.Name [FOLDER 'FolderPath']\n (\n Title: 'Page Title',\n Layout: Module.LayoutName\n [, Params: ( $Param: Module.Entity [optional] )] -- optional: 11.5+, may be opened with no argument\n [, Url: 'page-url']\n [, Variables: ( $var: Boolean = true )] -- the default is an expression, written bare\n [, PopupWidth: 800, PopupHeight: 480, PopupResizable: true]\n [, PopupCloseAction: cancelButton1]\n [, Class: 'css-class', Style: 'css: rule']\n [, DesignProperties: ('Spacing top': 'Inner large')] -- the page's own; Atlas\n -- declares them under Document and Page\n )\n {\n -- widgets\n }", Example: "CREATE PAGE MyModule.EditCustomer\n (\n Params: ( $Customer: MyModule.Customer ),\n Title: 'Edit Customer',\n Layout: Atlas_Core.PopupLayout,\n Class: 'container-fluid'\n )\n {\n DATAVIEW dvCustomer (DataSource: $Customer) {\n TEXTBOX txtName (Label: 'Name', Attribute: Name)\n FOOTER {\n ACTIONBUTTON btnSave (Caption: 'Save', Action: SAVE CHANGES, ButtonStyle: Primary)\n ACTIONBUTTON btnCancel (Caption: 'Cancel', Action: CANCEL CHANGES)\n }\n }\n };", SeeAlso: []string{"page.create", "page.widgets", "page.alter", "snippet", "rename"}, }) diff --git a/docs-site/src/reference/page/create-page.md b/docs-site/src/reference/page/create-page.md index 326c3bc81b..e62da012fc 100644 --- a/docs-site/src/reference/page/create-page.md +++ b/docs-site/src/reference/page/create-page.md @@ -5,7 +5,7 @@ ```sql CREATE [ OR REPLACE ] PAGE module.Name [ FOLDER 'path' ] ( - [ Params: ( $param : Module.Entity | Type [, ...] ), ] + [ Params: ( $param : Module.Entity | Type [optional] [, ...] ), ] Title: 'title', Layout: Module.LayoutName [, Variables: ( $name : type = [, ...] ) ] @@ -27,7 +27,7 @@ A replace (`OR REPLACE` or `OR MODIFY`) is refused when the stored page holds a The page declaration block `( ... )` contains comma-separated key-value properties: -- **Params** -- Page parameters. Each parameter has a `$`-prefixed name and a type — either an entity (`Module.Entity`) or a primitive (`String`, `Integer`, `Decimal`, `Boolean`, `DateTime`). Parameters are passed when the page is opened (e.g., from a microflow `SHOW PAGE` or from a data view). Primitive parameters require Mendix 11.6+. A parameter cannot be named with a platform-reserved word — `Context`, `Type`, `Id`, `Owner`, `Object`, `CurrentUser` or a Java keyword, in any case and even quoted: Mendix reports CE7247 "The name 'Context' is a reserved word.", and `mxcli check` refuses it (MDL-PARAM01). The same holds for snippet, Java action and JavaScript action parameters. +- **Params** -- Page parameters. Each parameter has a `$`-prefixed name and a type — either an entity (`Module.Entity`) or a primitive (`String`, `Integer`, `Decimal`, `Boolean`, `DateTime`). Parameters are passed when the page is opened (e.g., from a microflow `SHOW PAGE` or from a data view). Primitive parameters require Mendix 11.6+. A parameter is required unless it says `optional` (`$Sale: Shop.Sale optional`, Studio Pro's unticked *Required*, Mendix 11.5+): an optional parameter lets the page be opened with no argument, from a menu item for example, where a required one is CE1571. `describe page` prints it, and a snippet parameter cannot be optional. A parameter cannot be named with a platform-reserved word — `Context`, `Type`, `Id`, `Owner`, `Object`, `CurrentUser` or a Java keyword, in any case and even quoted: Mendix reports CE7247 "The name 'Context' is a reserved word.", and `mxcli check` refuses it (MDL-PARAM01). The same holds for snippet, Java action and JavaScript action parameters. - **Title** -- The page title displayed in the browser tab or page header. - **Layout** -- The layout the page is based on. Must be a qualified name referring to an existing layout (e.g., `Atlas_Core.Atlas_Default`, `Atlas_Core.PopupLayout`). - **Folder** -- Optional folder path within the module. Nested folders use `/` separator. diff --git a/docs-site/src/tools/builtin-rules.md b/docs-site/src/tools/builtin-rules.md index 5cfc144148..ba8990a4e8 100644 --- a/docs-site/src/tools/builtin-rules.md +++ b/docs-site/src/tools/builtin-rules.md @@ -40,6 +40,7 @@ The **lint rules** below run with `mxcli lint`. There is also a separate group o | **CONV013** | Error handling on external calls -- Ensures external service calls have error handling | | **CONV014** | No continue error handling -- Warns against using CONTINUE error handling without logging | | **CONV020** | Created object never persisted -- A microflow creates an object of a persistent entity without commit, and never commits, returns or passes it on, so the object is discarded when the microflow ends ([details](#conv020-details)) | +| **CONV021** | Create/change then commit -- A Create or Change object activity without commit, directly followed by a Commit of the same object; one create/change with Commit set does both ([details](#conv021-details)) | | **MDL-FLOW01** | Un-describable branch structure -- Decision branches that re-enter each other's paths, so `DESCRIBE MICROFLOW` cannot render them as nested `IF`s without changing what they mean | | **MDL-MAP04** | First over an object-rooted import mapping -- An `import from mapping` or `rest call … returns mapping` activity already in the model stores Studio Pro's *First* (`ForceSingleOccurrence` or `Range.SingleObject`) over a mapping that returns one object. `mx check` reports 0 errors; the activity throws `key not found: Path(QName(None,),None,)` when it runs. `mxcli check` refuses the same statement in a script under the same ID; this finds activities written before that check existed, or with `exec --no-check`. A list-rooted mapping's *First* is legitimate and is not reported | @@ -69,6 +70,36 @@ variable anywhere in an expression. The check is flow-insensitive: a commit on one branch only, or only in an error handler, counts, so that case is not reported. When the rule cannot tell, it stays quiet. +### CONV021 details {#conv021-details} + +```sql +$Order = create Shop.Order (Code = 'A1'); +commit $Order; +``` + +is two activities doing what one does. Studio Pro's Create object and Change +object activities have a **Commit** option (Yes, Yes without events, No), and the +expected way to model "create and save" or "change and save" is that option, not +a separate Commit activity: + +```sql +$Order = create Shop.Order (Code = 'A1') commit; +``` + +The suggestion carries the commit's settings over: a Commit with events becomes +Commit **Yes**, one without events **Yes without events** (`commit without +events`), and **Refresh in client** is kept if either activity had it (`refresh`). + +The rule reports a pair only when merging it cannot change what the microflow +does: + +- it is in a **microflow**, the create/change has Commit **No**, and the commit + names the object it creates or changes; +- the commit **directly** follows it on the normal flow (not an error handler + flow), and no other flow leads into the commit; +- neither activity has **custom error handling**, which merging would move or + drop. + ## Running Built-in Rules Built-in rules run automatically with `mxcli lint`: diff --git a/docs-site/src/tools/linting.md b/docs-site/src/tools/linting.md index 5b10e6e3c7..4183f83e41 100644 --- a/docs-site/src/tools/linting.md +++ b/docs-site/src/tools/linting.md @@ -18,7 +18,7 @@ The linting system provides: |----------|--------|-------| | MDL | MDL001-MDL007 | Naming conventions, empty microflows, domain model size | | Security | SEC001-SEC010 | Access rules, password policy, demo users, PII exposure | -| Convention | CONV001-CONV020 | Best practice conventions, error handling | +| Convention | CONV001-CONV021 | Best practice conventions, error handling | | Quality | QUAL001-QUAL004 | Complexity, documentation, long microflows | | Architecture | ARCH001-ARCH003 | Cross-module data, entity business keys | | Design | DESIGN001 | Entity attribute count | diff --git a/mdl-examples/bug-tests/1640-optional-page-parameter.mdl b/mdl-examples/bug-tests/1640-optional-page-parameter.mdl new file mode 100644 index 0000000000..9070994e7e --- /dev/null +++ b/mdl-examples/bug-tests/1640-optional-page-parameter.mdl @@ -0,0 +1,40 @@ +-- mendixlabs/mxcli#1640 (ako/mxcli#1649): an optional page parameter +-- (Forms$PageParameter.IsRequired = false, Mendix 11.5+) was invisible to +-- mxcli. +-- +-- Symptom: on a page whose parameter Studio Pro marks not Required, opened by a +-- menu item with no argument (mx check 0 errors), `describe page` printed +-- Params: ( $Sale: MyFirstModule.Sale ) +-- `describe navigation` printed +-- menu item 'Sale' ( OnClick: show page MyFirstModule.Sale_View(Sale = $currentObject) … ) +-- and executing that output was refused with MDL-PAGEARG01, so the navigation +-- profile could no longer be changed from MDL. +-- +-- Fix: `$Sale: Shop.Sale optional` writes IsRequired = false (refused below +-- 11.5, MDL-VERSION01, and on a snippet parameter, MDL087); describe page +-- prints it; and describe navigation passes no argument for an optional +-- parameter with no stored mapping, so its output re-runs "Unchanged". A +-- parameter stored without IsRequired (below 11.5) reads as required. +-- +-- To verify: run this script on an 11.5+ project, then `mxcli docker check`: +-- 0 errors (remove `optional` and the menu item is CE1571). In Studio Pro the +-- parameter's Required box is unticked. `describe navigation Responsive` prints +-- the Sale item without an argument. + +mdl 1; + +create module Shop; + +create persistent entity Shop.Sale ( + Name: string(100) +); + +create page Shop.Sale_View ( + Title: 'Sale', + Layout: Atlas_Core.Atlas_Default, + Params: ( $Sale: Shop.Sale optional ) +) { + dataview dvSale (DataSource: $Sale) { + textbox tbName (Attribute: Name) + } +}; diff --git a/mdl-examples/bug-tests/1781-message-definition-references.mdl b/mdl-examples/bug-tests/1781-message-definition-references.mdl new file mode 100644 index 0000000000..8777440bff --- /dev/null +++ b/mdl-examples/bug-tests/1781-message-definition-references.mdl @@ -0,0 +1,40 @@ +mdl 1; +-- ako/mxcli#1781: a message definition that exposes an entity, its attributes +-- and an association never appeared in `show references to` any of them, so +-- renaming or deleting one looked safe while the definition still used it. +-- +-- mxcli exec this-file.mdl -p App.mpr -- Mendix 11.14 or older +-- mxcli -p App.mpr -c "refresh catalog full; show references to Bug1781.Order" +-- +-- Before the fix: the entity's references listed nothing from MD_Order. After +-- it, MD_Order is listed as `mapping` for Bug1781.Order and Bug1781.Customer, +-- and as `member` for each exposed attribute and the association. +-- On 11.15+ the same holds for `create message definition` documents, whose +-- source type is MESSAGE_DEFINITION instead of MESSAGE_DEFINITION_COLLECTION. + +create module Bug1781; + +create persistent entity Bug1781.Customer ( + Name: String(100) +); + +create persistent entity Bug1781.Order ( + OrderId: Long, + Total: Decimal, + -- Not exposed: the control, which must get no edge from MD_Order. + Notes: String(200) +); + +create association Bug1781.Order_Customer from Bug1781.Order to Bug1781.Customer; + +-- @version: ..11.14 +create message definition collection Bug1781.MD_Order +{ + definition OrderMessage for Bug1781.Order as 'Order' { + OrderId, + Total, + Bug1781.Order_Customer/Bug1781.Customer { + Name + } + } +}; diff --git a/mdl-examples/bug-tests/1799-source-catalog-widget-header-race.mdl b/mdl-examples/bug-tests/1799-source-catalog-widget-header-race.mdl new file mode 100644 index 0000000000..904c1e604f --- /dev/null +++ b/mdl-examples/bug-tests/1799-source-catalog-widget-header-race.mdl @@ -0,0 +1,33 @@ +mdl 1; +-- ako/mxcli#1799: a source catalog build described a project pluggable widget +-- by its generic form on some runs and by its keyword on others, for the same +-- unchanged project: +-- +-- vegachart chartReplayWin ( spec: … +-- pluggablewidget 'f1.widget.web.vegachart.VegaChart' chartReplayWin ( spec: … +-- +-- Repro, in a project that has the widget's .mpk in widgets/: +-- +-- mxcli exec this-file.mdl -p App.mpr +-- # one definition without a current generatorVersion stamp — a hand-made +-- # one, or one whose .mpk was removed — keeps every load "stale" +-- echo '{"widgetId":"acme.widget.Orphan","mdlName":"orphanwidget"}' \ +-- > .mxcli/widgets/aaorphan.def.json +-- for i in 1 2 3 4 5 6; do +-- mxcli -p App.mpr -c "refresh catalog full source force" +-- sqlite3 .mxcli/catalog.db "select count(*) from source +-- where instr(SourceText, 'pluggablewidget ''com.mendix.widget.web.accordion')>0" +-- done +-- +-- Before the fix the count was non-zero on some runs: every parallel describe +-- loaded its own registry, each load rewrote the definitions (truncating +-- them) because of the stale one, and a describe that read a truncated file +-- lost the widget. After it, 0 on every run: the describes share the build's +-- registry, and a definition is replaced by rename, never truncated. + +create or modify module WidgetProbe; + +create or modify page WidgetProbe.P1799 ( title: 'P1799', layout: Atlas_Core.Atlas_Default ) +{ + accordion acc1 +}; diff --git a/mdl-examples/bug-tests/548-pluggable-expression-slot.mdl b/mdl-examples/bug-tests/548-pluggable-expression-slot.mdl new file mode 100644 index 0000000000..df9faee1c2 --- /dev/null +++ b/mdl-examples/bug-tests/548-pluggable-expression-slot.mdl @@ -0,0 +1,35 @@ +-- ako/mxcli#548: a pluggable widget's Expression-typed property was written +-- with its value in PrimitiveValue as well as in Expression. +-- +-- Symptom: the combo box below, written by mxcli, stored +-- customEditabilityExpression PrimitiveValue: "false" Expression: "false" +-- (`mxcli bson dump -p app.mpr --type page --object Shop.Thing_Edit`), where +-- Studio Pro stores PrimitiveValue "" — on all 939 Expression-typed values of +-- ako/TestApp and PedApp. mx check accepted both, so the only effect was that +-- mxcli's widgets differed from Studio Pro's and a page saved by one never +-- re-ran "Unchanged" against the other. +-- +-- Cause and fix: a property the widget's editorConfig hides is reset to its +-- declared default, and the reset used the primitive setter for every value +-- type. It now writes an Expression property's default into Expression only. +-- +-- To verify: run this script and dump the page; customEditabilityExpression +-- has Expression "false" and PrimitiveValue "". Then save the page in Studio +-- Pro: the value is unchanged. + +mdl 1; + +create module Shop; + +create enumeration Shop.Colour (Red 'Red', Blue 'Blue'); + +create persistent entity Shop.Thing ( + Colour: enumeration(Shop.Colour), + Name: string(100) +); + +create page Shop.Thing_Edit (Title: 'Thing', Layout: Atlas_Core.Atlas_Default, Params: ($Thing: Shop.Thing)) { + dataview dvThing (DataSource: $Thing) { + combobox cbColour (Label: 'Colour', Attribute: Colour) + } +}; diff --git a/mdl/ast/ast_page.go b/mdl/ast/ast_page.go index f8ad54c09f..ffb6baadb2 100644 --- a/mdl/ast/ast_page.go +++ b/mdl/ast/ast_page.go @@ -12,6 +12,10 @@ type PageParameter struct { Name string // Parameter name (without $ prefix) EntityType QualifiedName // Entity type (for backward compatibility; empty for primitives) Type DataType // Full data type (primitives + entities) + // Optional is `$Name: Type optional`: Forms$PageParameter.IsRequired + // false, a Mendix 11.5+ page parameter the page can be opened without + // (mendixlabs/mxcli#1640). Snippet parameters have no such flag. + Optional bool } // PageVariable represents a page variable: $Name: DataType = 'defaultExpression' diff --git a/mdl/backend/modelsdk/page.go b/mdl/backend/modelsdk/page.go index 38dd938e74..9dc9a98ec7 100644 --- a/mdl/backend/modelsdk/page.go +++ b/mdl/backend/modelsdk/page.go @@ -192,7 +192,7 @@ func pageFromGen(p *genPg.Page, containerID model.ID) *pages.Page { ContainerID: out.ID, Name: gp.Name(), DefaultValue: gp.DefaultValue(), - IsRequired: gp.IsRequired(), + IsRequired: pageParameterIsRequired(gp), } pp.ID = model.ID(gp.ID()) // Object parameters carry an entity (rendered as the qualified entity @@ -345,3 +345,21 @@ func (b *Backend) PageLayoutName(id model.ID) (string, error) { } return backend.PageLayoutNameFromRaw(raw), nil } + +// pageParameterIsRequired is a stored page parameter's IsRequired. The property +// is Mendix 11.5+ (mendixlabs/mxcli#1640): a parameter stored without it — every +// one below 11.5 — is required, and only a stored false is optional. The gen +// accessor alone reads an absent key as false, which would make every older +// parameter look optional. +func pageParameterIsRequired(gp *genPg.PageParameter) bool { + raw := gp.Raw() + if raw == nil { + return gp.IsRequired() + } + v, err := raw.LookupErr("IsRequired") + if err != nil { + return true + } + b, ok := v.BooleanOK() + return !ok || b +} diff --git a/mdl/backend/modelsdk/page_mutator_write.go b/mdl/backend/modelsdk/page_mutator_write.go index 17856759ed..71bace65ae 100644 --- a/mdl/backend/modelsdk/page_mutator_write.go +++ b/mdl/backend/modelsdk/page_mutator_write.go @@ -54,7 +54,8 @@ func (d codecPageDeps) BuildDataGrid2Column(col *backend.DataGridColumnSpec, col // SerializeParameter builds a new page or snippet parameter through the same // converters CREATE writes with, so ALTER gets the same version gate on // IsRequired/DefaultValue (11.5+) and the same ParameterType shapes. A page -// parameter MDL declares is always required with no default, as on CREATE. +// parameter MDL declares is required unless it says `optional`, and has no +// default, as on CREATE. func (d codecPageDeps) SerializeParameter(container backend.ContainerKind, p backend.PageParameterSpec) (bson.D, error) { var el element.Element switch container { @@ -63,7 +64,7 @@ func (d codecPageDeps) SerializeParameter(container backend.ContainerKind, p bac Name: p.Name, TypeName: p.PrimitiveType, EntityName: p.EntityName, - IsRequired: true, + IsRequired: !p.Optional, }, d.b.ProjectVersion()) case backend.ContainerSnippet: el = snippetParameterToGen(&pages.SnippetParameter{ diff --git a/mdl/backend/mutation.go b/mdl/backend/mutation.go index 7ff8a51d6e..a4fc7bcb9e 100644 --- a/mdl/backend/mutation.go +++ b/mdl/backend/mutation.go @@ -20,6 +20,9 @@ type PageParameterSpec struct { Name string PrimitiveType string EntityName string + // Optional writes a page parameter with IsRequired false (11.5+, + // mendixlabs/mxcli#1640). A snippet parameter has no such flag. + Optional bool } const ( diff --git a/mdl/catalog/builder_member_refs.go b/mdl/catalog/builder_member_refs.go index fa252ad7b1..0600ffc6a5 100644 --- a/mdl/catalog/builder_member_refs.go +++ b/mdl/catalog/builder_member_refs.go @@ -52,6 +52,11 @@ var memberRefSourceTypes = map[string]string{ "Workflows$Workflow": RefObjectWorkflow, "ImportMappings$ImportMapping": RefObjectImportMapping, "ExportMappings$ExportMapping": RefObjectExportMapping, + // A message definition exposes an entity, its attributes and associations + // by qualified name (#1781). Before 11.15 the definitions sit inside a + // collection document; from 11.15 each is a document of its own. + "MessageDefinitions$MessageDefinitionCollection": RefObjectMessageDefinitionCollection, + "MessageDefinitions$MessageDefinition2": RefObjectMessageDefinition, } // memberRefSkipKeys are string properties that hold prose or a document's own @@ -180,14 +185,17 @@ func (s *edgeSet) sorted() []refEdge { return s.edges } -// memberRefsInUnit returns the member, enumeration and (for mappings) entity -// edges one stored document makes. +// memberRefsInUnit returns the member, enumeration and (for mappings and +// message definitions) entity edges one stored document makes. func memberRefsInUnit(contents []byte, sourceType string, idx *memberRefIndex) []refEdge { var doc bson.D if err := bson.Unmarshal(contents, &doc); err != nil { return nil } - isMapping := sourceType == RefObjectImportMapping || sourceType == RefObjectExportMapping + // Only these name an entity by a bare string that is a use of it; anywhere + // else an entity name is matched by the typed reference extractors. + isMapping := sourceType == RefObjectImportMapping || sourceType == RefObjectExportMapping || + sourceType == RefObjectMessageDefinitionCollection || sourceType == RefObjectMessageDefinition var set edgeSet walkBSONStrings(doc, "", func(key, v string) { if memberRefSkipKeys[key] || v == "" { diff --git a/mdl/catalog/builder_member_refs_test.go b/mdl/catalog/builder_member_refs_test.go index 3f9cb5321a..9ec6205dc3 100644 --- a/mdl/catalog/builder_member_refs_test.go +++ b/mdl/catalog/builder_member_refs_test.go @@ -196,6 +196,42 @@ func memberRefsFixture(t *testing.T) *Catalog { }}, } + // A message definition exposes an entity, its attributes and an + // association, each by qualified name (#1781). Two storage shapes: before + // 11.15 the definitions sit inside a MessageDefinitionCollection; from 11.15 + // each is a MessageDefinition2 document of its own. + exposedOrder := bson.D{ + {Key: "$Type", Value: "MessageDefinitions$ExposedEntity"}, + {Key: "Entity", Value: "Shop.Order"}, + {Key: "Children", Value: bson.A{int32(2), + bson.D{ + {Key: "$Type", Value: "MessageDefinitions$ExposedAttribute"}, + {Key: "Attribute", Value: "Shop.Order.Total"}, + }, + bson.D{ + {Key: "$Type", Value: "MessageDefinitions$ExposedAssociation"}, + {Key: "Association", Value: "Shop.Order_Customer"}, + {Key: "Entity", Value: "Shop.Customer"}, + }, + }}, + } + mdCollectionRaw := bson.D{ + {Key: "$Type", Value: "MessageDefinitions$MessageDefinitionCollection"}, + {Key: "Name", Value: "MD_Order"}, + {Key: "MessageDefinitions", Value: bson.A{int32(2), + bson.D{ + {Key: "$Type", Value: "MessageDefinitions$EntityMessageDefinition"}, + {Key: "Name", Value: "Order"}, + {Key: "ExposedEntity", Value: exposedOrder}, + }, + }}, + } + mdDocumentRaw := bson.D{ + {Key: "$Type", Value: "MessageDefinitions$MessageDefinition2"}, + {Key: "Name", Value: "MD2_Order"}, + {Key: "ExposedEntity", Value: exposedOrder}, + } + rawUnit := func(id, typ string, doc bson.D) *types.RawUnit { b, err := bson.Marshal(doc) if err != nil { @@ -207,6 +243,8 @@ func memberRefsFixture(t *testing.T) *Catalog { rawUnit("mf-1", "Microflows$Microflow", mfRaw), rawUnit("pg-1", "Forms$Page", pageRaw), rawUnit("im-1", "ImportMappings$ImportMapping", mappingRaw), + rawUnit("mdc-1", "MessageDefinitions$MessageDefinitionCollection", mdCollectionRaw), + rawUnit("md2-1", "MessageDefinitions$MessageDefinition2", mdDocumentRaw), } b := &Builder{ @@ -320,6 +358,39 @@ func TestReferencesReachMembersEnumerationsWorkflowsAndMappings(t *testing.T) { } } +// A message definition that exposes an entity, an attribute and an +// association is a use of all three (#1781): without the edges, renaming or +// deleting any of them looked safe while the definition still depended on it. +// Both storage shapes, the collection and the 11.15+ document. +func TestReferencesReachMessageDefinitions(t *testing.T) { + cat := memberRefsFixture(t) + + for _, src := range []refRow{ + {sourceType: "MESSAGE_DEFINITION_COLLECTION", sourceName: "Shop.MD_Order"}, + {sourceType: "MESSAGE_DEFINITION", sourceName: "Shop.MD2_Order"}, + } { + for _, w := range []refRow{ + {src.sourceType, src.sourceName, "ENTITY", "Shop.Order", "mapping"}, + {src.sourceType, src.sourceName, "ENTITY", "Shop.Customer", "mapping"}, + {src.sourceType, src.sourceName, "ATTRIBUTE", "Shop.Order.Total", "member"}, + {src.sourceType, src.sourceName, "ASSOCIATION", "Shop.Order_Customer", "member"}, + } { + if got := refsTo(t, cat, w.targetName); got[w] != 1 { + t.Errorf("edge %s %s -> %s %s (%s): %d, want 1; refs to %s: %v", + w.sourceType, w.sourceName, w.targetType, w.targetName, w.kind, got[w], w.targetName, got) + } + } + } + + // Control: what no definition exposes has no message-definition edge, so + // the edges above come from the exposure, not from every name the walk knows. + for r := range refsTo(t, cat, "Shop.Order.Code") { + if strings.HasPrefix(r.sourceType, "MESSAGE_DEFINITION") { + t.Errorf("unexposed attribute has a message-definition edge: %v", r) + } + } +} + // scanPaths is the resolver both XPath constraints and expressions go through. // Each case pins one way a bare word could be mistaken for a member, or a real // member missed. diff --git a/mdl/catalog/builder_references.go b/mdl/catalog/builder_references.go index bd144f2e83..9f88624eaa 100644 --- a/mdl/catalog/builder_references.go +++ b/mdl/catalog/builder_references.go @@ -42,7 +42,7 @@ const ( RefKindXPath = "xpath" // An XPath constraint names an attribute, association or enumeration value RefKindType = "type" // An attribute, parameter or variable is typed as an enumeration RefKindValue = "value" // An expression names an enumeration value - RefKindMapping = "mapping" // An import or export mapping maps an entity + RefKindMapping = "mapping" // An import or export mapping maps an entity, or a message definition exposes one RefKindSplit = "split" // A decision branches on an object being a specialization ) @@ -100,6 +100,9 @@ const ( RefObjectEnumerationValue = "ENUMERATION_VALUE" RefObjectImportMapping = "IMPORT_MAPPING" RefObjectExportMapping = "EXPORT_MAPPING" + // Message definitions: the pre-11.15 collection and the 11.15+ document. + RefObjectMessageDefinitionCollection = "MESSAGE_DEFINITION_COLLECTION" + RefObjectMessageDefinition = "MESSAGE_DEFINITION" ) // RefSourceObjectTypes is every value that reaches refs.SourceType, and @@ -127,6 +130,8 @@ var ( RefObjectProjectSettings, RefObjectImportMapping, RefObjectExportMapping, + RefObjectMessageDefinitionCollection, + RefObjectMessageDefinition, } RefTargetObjectTypes = []string{ diff --git a/mdl/executor/catalog_source_widget_registry_1799_test.go b/mdl/executor/catalog_source_widget_registry_1799_test.go new file mode 100644 index 0000000000..3a0c9d0cd5 --- /dev/null +++ b/mdl/executor/catalog_source_widget_registry_1799_test.go @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// ako/mxcli#1799: a source catalog build described a pluggable widget by its +// generic form at random — +// +// vegachart chartReplayWin ( spec: … +// +// on one run and +// +// pluggablewidget 'f1.widget.web.vegachart.VegaChart' chartReplayWin ( spec: … +// +// on the next, same binary, same unchanged project. +// +// Every parallel describe built its own context and so loaded its own widget +// registry from disk, while the same loads rewrote `.mxcli/widgets/*.def.json` +// whenever one definition was unstamped. A describe that read a file another +// worker had just truncated got a registry missing the widget, and +// pluggableWidgetHeader fell back to the id form — correctly, for the registry +// it was given. + +// A parallel describe must use the build's registry, not reload one: the test +// removes the definitions from disk after the build loaded them, so a reload +// finds none, which is the state a torn read left a worker in. +func TestParallelDescribeUsesTheBuildsWidgetRegistry(t *testing.T) { + dir := t.TempDir() + mprPath := filepath.Join(dir, "App.mpr") + defsDir := filepath.Join(dir, ".mxcli", "widgets") + if err := os.MkdirAll(defsDir, 0o755); err != nil { + t.Fatal(err) + } + def := `{"widgetId": "probe.widget.web.Probe", "mdlName": "probewidget", "generatorVersion": ` + + jsonInt(WidgetDefGeneratorVersion) + `}` + if err := os.WriteFile(filepath.Join(defsDir, "probewidget.def.json"), []byte(def), 0o644); err != nil { + t.Fatal(err) + } + + ctx := &ExecContext{MprPath: mprPath} + built := ctx.GetWidgetRegistry() + if got := pluggableWidgetHeader(built, "probe.widget.web.Probe", "w1"); got != "probewidget w1" { + t.Fatalf("build registry: header = %q, want %q (fixture does not resolve)", got, "probewidget w1") + } + + if err := os.RemoveAll(defsDir); err != nil { + t.Fatal(err) + } + + var buf bytes.Buffer + local := parallelDescribeContext(ctx, &buf, "") + if got := pluggableWidgetHeader(local.GetWidgetRegistry(), "probe.widget.web.Probe", "w1"); got != "probewidget w1" { + t.Errorf("parallel describe: header = %q, want %q — the describe reloaded the registry instead of using the build's", got, "probewidget w1") + } + if local.GetWidgetRegistry() != built { + t.Errorf("parallel describe has its own registry; want the build's") + } +} + +// Refreshing the definitions must never let a reader see a torn file: another +// mxcli process, or the language server, reads them while a refresh writes. +func TestWidgetDefinitionRefreshNeverExposesATornFile(t *testing.T) { + dir := t.TempDir() + mprPath := filepath.Join(dir, "App.mpr") + if err := os.MkdirAll(filepath.Join(dir, "widgets"), 0o755); err != nil { + t.Fatal(err) + } + const fixture = "../../testdata/expr-checker/widgets/Charts.mpk" + src, err := os.ReadFile(fixture) + if err != nil { + t.Skipf("Charts.mpk fixture not available: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "widgets", "Charts.mpk"), src, 0o644); err != nil { + t.Fatal(err) + } + if _, err := RefreshWidgetDefinitions(mprPath, false, nil); err != nil { + t.Fatal(err) + } + defsDir := filepath.Join(dir, ".mxcli", "widgets") + + stop := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for range 30 { + if _, err := RefreshWidgetDefinitions(mprPath, true, nil); err != nil { + t.Errorf("refresh: %v", err) + break + } + } + close(stop) + }() + + var torn []string + for reading := true; reading; { + select { + case <-stop: + reading = false + default: + } + entries, _ := os.ReadDir(defsDir) + for _, e := range entries { + if !strings.HasSuffix(e.Name(), ".def.json") { + continue + } + data, err := os.ReadFile(filepath.Join(defsDir, e.Name())) + if err != nil { + continue + } + var v map[string]any + if json.Unmarshal(data, &v) != nil && len(torn) < 5 { + torn = append(torn, e.Name()+" ("+jsonInt(len(data))+" bytes)") + } + } + } + wg.Wait() + if len(torn) > 0 { + t.Errorf("a reader saw a partly written definition during a refresh: %v", torn) + } +} + +func jsonInt(n int) string { + b, _ := json.Marshal(n) + return string(b) +} diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index ebf4698ce6..87375b73e5 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -221,9 +221,12 @@ func checkAddedParameterURLSegments(ctx *ExecContext, s *ast.AlterPageStmt, h *C // DataTypes $Type, anything else must name an entity that exists, and a snippet // takes entities only (MDL087 — mxbuild's CE0046 otherwise). func alterParameterSpec(ctx *ExecContext, s *ast.AlterPageStmt, containerType string, p ast.PageParameter) (backend.PageParameterSpec, error) { - spec := backend.PageParameterSpec{Name: p.Name} + spec := backend.PageParameterSpec{Name: p.Name, Optional: p.Optional} bsonType := pageParamBSONType(p.Type) if strings.EqualFold(containerType, "snippet") { + if p.Optional { + return spec, mdlerrors.NewValidation(optionalSnippetParameterMessage(s.PageName, p.Name)) + } if caption := types.SnippetParameterTypeRule(bsonType); caption != "" { return spec, mdlerrors.NewValidationf( "snippet '%s' cannot take parameter $%s with the primitive type %s — a snippet "+ diff --git a/mdl/executor/cmd_catalog.go b/mdl/executor/cmd_catalog.go index 80fd7981df..f6235e4ae7 100644 --- a/mdl/executor/cmd_catalog.go +++ b/mdl/executor/cmd_catalog.go @@ -542,6 +542,9 @@ func buildCatalog(ctx *ExecContext, full, isSource, communities bool, resolution if isSource { builder.SetSourceMode(true) preWarmCache(ctx) + // Loaded here, on one goroutine, for the describe workers to share + // (parallelDescribeContext). + ctx.GetWidgetRegistry() builder.SetDescribeFunc(func(objectType string, qualifiedName string, id string) (string, error) { if hook := sourceDescribeHook; hook != nil { hook(objectType, qualifiedName) @@ -1037,25 +1040,8 @@ func captureDescribeParallel(ctx *ExecContext, objectType string, qualifiedName } qn := ast.QualifiedName{Module: parts[0], Name: parts[1]} - // Create a goroutine-local context: shared backend + cache, own output buffer. var buf bytes.Buffer - localCtx := &ExecContext{ - Context: ctx.Context, - Output: &buf, - Format: ctx.Format, - Quiet: ctx.Quiet, - Logger: ctx.Logger, - Backend: ctx.Backend, - Cache: ctx.Cache, - MprPath: ctx.MprPath, - // Pins the describe to this document rather than to whichever - // document of this name the lookup prefers (#1185). - describeID: id, - // The text feeds the search index, where layout annotations do not - // matter, and telling derived layout from authored rebuilds each flow - // several times — it made a full source refresh 2.3x slower (#766). - describeFullLayout: true, - } + localCtx := parallelDescribeContext(ctx, &buf, id) describe := describeDispatch(objectType) if describe == nil { @@ -1068,6 +1054,37 @@ func captureDescribeParallel(ctx *ExecContext, objectType string, qualifiedName return buf.String(), nil } +// parallelDescribeContext is the goroutine-local context one parallel describe +// runs in: shared backend + cache + widget registry, own output buffer. +// +// The registry is the build's, loaded once before the fan-out. Each describe +// used to load its own, and every load could rewrite the project's +// .def.json files, so a worker that read a file another had just truncated +// got a registry without that widget and described it in the generic +// `pluggablewidget ''` form — on some runs and not others (#1799). The +// registry is read-only once loaded, so sharing it is safe. +func parallelDescribeContext(ctx *ExecContext, out io.Writer, id model.ID) *ExecContext { + return &ExecContext{ + widgetRegistry: ctx.widgetRegistry, + widgetRegistryLoaded: ctx.widgetRegistryLoaded, + Context: ctx.Context, + Output: out, + Format: ctx.Format, + Quiet: ctx.Quiet, + Logger: ctx.Logger, + Backend: ctx.Backend, + Cache: ctx.Cache, + MprPath: ctx.MprPath, + // Pins the describe to this document rather than to whichever + // document of this name the lookup prefers (#1185). + describeID: id, + // The text feeds the search index, where layout annotations do not + // matter, and telling derived layout from authored rebuilds each flow + // several times — it made a full source refresh 2.3x slower (#766). + describeFullLayout: true, + } +} + // describeDocFunc renders one document as MDL into ctx.Output. type describeDocFunc func(ctx *ExecContext, name ast.QualifiedName) error diff --git a/mdl/executor/cmd_pages_builder_v3.go b/mdl/executor/cmd_pages_builder_v3.go index bafbcfc381..cde793157b 100644 --- a/mdl/executor/cmd_pages_builder_v3.go +++ b/mdl/executor/cmd_pages_builder_v3.go @@ -131,7 +131,9 @@ func (pb *pageBuilder) buildPageV3(s *ast.CreatePageStmtV3) (*pages.Page, error) }, ContainerID: page.ID, Name: param.Name, - IsRequired: true, // Page parameters are required by default + // Required unless the statement says `optional` (11.5+, gated in + // documentTypeGates; mendixlabs/mxcli#1640). + IsRequired: !param.Optional, } // Check if this is a primitive type or entity type @@ -302,6 +304,9 @@ func (pb *pageBuilder) buildSnippetV3(s *ast.CreateSnippetStmtV3) (*pages.Snippe // than written, which storage would allow and mxbuild would not // (CE0046). Same rule check applies, so a script cannot pass one and // fail the other (mendixlabs/mxcli#1028). + if param.Optional { + return nil, mdlerrors.NewValidation(optionalSnippetParameterMessage(s.Name, param.Name)) + } if caption := types.SnippetParameterTypeRule(pageParamBSONType(param.Type)); caption != "" { return nil, mdlerrors.NewValidationf( "snippet '%s' declares parameter $%s with the primitive type %s — a snippet "+ diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index 0f7b55f7cc..947c1f0fad 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -138,7 +138,12 @@ func describePage(ctx *ExecContext, name ast.QualifiedName) error { params := []string{} for _, p := range foundPage.Parameters { typeName := pageParamTypeMDL(p) - params = append(params, fmt.Sprintf("$%s: %s", p.Name, typeName)) + decl := fmt.Sprintf("$%s: %s", p.Name, typeName) + // An 11.5+ parameter the page can be opened without (#1640). + if !p.IsRequired { + decl += " optional" + } + params = append(params, decl) } props = append(props, fmt.Sprintf("Params: ( %s )", strings.Join(params, ", "))) } diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 9a625783bd..75c45d2f0f 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -1846,8 +1846,12 @@ func pageActionParameters(ctx *ExecContext, settings map[string]any, pageName st if explicit := extractPageParameters(ctx, settings); explicit != "" { return explicit } - // No stored mapping: the target page's own parameters are the mapping, each - // bound to the row object the enclosing widget supplies. + // No stored mapping: the target page's own required parameters are the + // mapping, each bound to the row object the enclosing widget supplies. An + // optional one (11.5+) with no mapping is simply not passed — a menu item + // opens such a page with no argument, and inventing `= $currentObject` + // there made the description one exec refuses (MDL-PAGEARG01, + // mendixlabs/mxcli#1640). var params []string for _, name := range targetPageParameterNames(ctx, pageName) { params = append(params, visitor.ParameterNameSpelling(name)+" = $currentObject") @@ -1855,9 +1859,9 @@ func pageActionParameters(ctx *ExecContext, settings map[string]any, pageName st return strings.Join(params, ", ") } -// targetPageParameterNames returns the parameter names declared by a page, by -// qualified name. Returns nil when the page cannot be resolved — a description -// that omits an argument is better than one that invents a name. +// targetPageParameterNames returns the required parameter names declared by a +// page, by qualified name. Returns nil when the page cannot be resolved — a +// description that omits an argument is better than one that invents a name. func targetPageParameterNames(ctx *ExecContext, qualifiedName string) []string { module, name, ok := strings.Cut(qualifiedName, ".") if !ok || module == "" || name == "" { @@ -1880,7 +1884,7 @@ func targetPageParameterNames(ctx *ExecContext, qualifiedName string) []string { } var names []string for _, param := range p.Parameters { - if param != nil && param.Name != "" { + if param != nil && param.Name != "" && param.IsRequired { names = append(names, param.Name) } } diff --git a/mdl/executor/cmd_pages_describe_pageparams_test.go b/mdl/executor/cmd_pages_describe_pageparams_test.go index d47d34825e..607c29eb22 100644 --- a/mdl/executor/cmd_pages_describe_pageparams_test.go +++ b/mdl/executor/cmd_pages_describe_pageparams_test.go @@ -13,6 +13,13 @@ import ( // pageParamFixture builds a module with a target page that declares one // parameter — the drill-down page a grid's link button opens. func pageParamFixture(t *testing.T, params ...string) *ExecContext { + t.Helper() + return pageParamFixtureWith(t, nil, params...) +} + +// pageParamFixtureWith is pageParamFixture with the named parameters optional +// (IsRequired false, Mendix 11.5+); every other one is required. +func pageParamFixtureWith(t *testing.T, optional map[string]bool, params ...string) *ExecContext { t.Helper() mod := mkModule("Formula1Frontend") @@ -21,12 +28,12 @@ func pageParamFixture(t *testing.T, params ...string) *ExecContext { ContainerID: mod.ID, Name: "Race_Weekend", } - for i, p := range params { + for _, p := range params { target.Parameters = append(target.Parameters, &pages.PageParameter{ BaseElement: model.BaseElement{ID: model.ID("pp-" + p)}, ContainerID: target.ID, Name: p, - IsRequired: i == 0, + IsRequired: !optional[p], }) } @@ -83,6 +90,23 @@ func TestRenderShowPageAction_RecoversEveryParameter(t *testing.T) { } } +// An optional parameter (Mendix 11.5+) with no stored mapping is not passed, +// so describe recovers no argument for it — inventing one made a menu item's +// description refused by exec (MDL-PAGEARG01, mendixlabs/mxcli#1640). +func TestRenderShowPageAction_SkipsAnOptionalParameter(t *testing.T) { + ctx := pageParamFixtureWith(t, map[string]bool{"Season": true}, "Race", "Season") + + got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", nil)) + want := "show page Formula1Frontend.Race_Weekend(Race = $currentObject)" + if got != want { + t.Errorf("got: %s\nwant: %s", got, want) + } + ctx = pageParamFixtureWith(t, map[string]bool{"Race": true}, "Race") + if got := renderClientActionMDL(ctx, showPageAction("Formula1Frontend.Race_Weekend", nil)); got != "show page Formula1Frontend.Race_Weekend" { + t.Errorf("a page whose only parameter is optional grew an argument: %s", got) + } +} + // A page that takes no parameters must not grow an argument list. func TestRenderShowPageAction_NoParametersStaysBare(t *testing.T) { ctx := pageParamFixture(t) diff --git a/mdl/executor/issue1649_optional_page_parameter_test.go b/mdl/executor/issue1649_optional_page_parameter_test.go new file mode 100644 index 0000000000..df72bcc864 --- /dev/null +++ b/mdl/executor/issue1649_optional_page_parameter_test.go @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// mendixlabs/mxcli#1640 (ako/mxcli#1649): since Mendix 11.5 a page parameter +// can be optional (Studio Pro: untick Required; Forms$PageParameter.IsRequired +// = false), and a menu item may then open the page with no argument — mx check +// 0 errors. mxcli could neither say nor see it: describe page dropped the flag, +// describe navigation invented `(Sale = $currentObject)` for that menu item, and +// executing describe's output was refused with MDL-PAGEARG01, so the navigation +// profile could no longer be changed from MDL at all. + +const issue1649Setup = `create persistent entity MyFirstModule.Sale (Name: String(100));` + +const issue1649Page = `create page MyFirstModule.Sale_View ( + Title: 'Sale', + Layout: Atlas_Core.Atlas_Default, + Params: ( $Sale: MyFirstModule.Sale optional ) +) { + dataview dvSale (DataSource: $Sale) { textbox tbName (Attribute: Name) } +};` + +// storedParameterRequired reads the stored IsRequired of the page's parameter, +// and whether the key is stored at all. +func storedParameterRequired(t *testing.T, exec *Executor, id string) (required, present bool) { + t.Helper() + params := bsonDocs(bsonValue(storedUnitDoc(t, exec, id), "Parameters")) + if len(params) != 1 { + t.Fatalf("page stores %d parameters", len(params)) + } + v, ok := bsonnav.DGet(params[0], "IsRequired").(bool) + return v, ok +} + +func TestIssue1649_OptionalPageParameterIsWrittenAndDescribed(t *testing.T) { + exec, out := pageFixtureExecutor(t) + run(t, exec, issue1649Setup) + run(t, exec, issue1649Page) + id := unitIDByName(t, exec, "page", "Sale_View") + if required, present := storedParameterRequired(t, exec, id); required || !present { + t.Errorf("stored IsRequired = %v (present %v), want false", required, present) + } + + out.Reset() + run(t, exec, "describe page MyFirstModule.Sale_View") + described := out.String() + if !strings.Contains(described, "Params: ( $Sale: MyFirstModule.Sale optional )") { + t.Fatalf("describe page does not say the parameter is optional:\n%s", described) + } + out.Reset() + run(t, exec, described) + if got := out.String(); !strings.Contains(got, "Unchanged page") { + t.Errorf("re-running describe wrote the page:\n%s", got) + } + + // Without the word the parameter is required, as before. + run(t, exec, strings.Replace(described, " optional", "", 1)) + if required, _ := storedParameterRequired(t, exec, id); !required { + t.Error("dropping `optional` left the parameter optional") + } +} + +// The report's scenario: a Studio Pro page whose parameter is optional, opened +// by a menu item with no argument. +func TestIssue1649_DescribeNavigationOpensAnOptionalParameterPageWithNoArgument(t *testing.T) { + exec, out := pageFixtureExecutor(t) + run(t, exec, issue1649Setup) + run(t, exec, strings.Replace(issue1649Page, " optional", "", 1)) + id := unitIDByName(t, exec, "page", "Sale_View") + + // Untick Required, as Studio Pro stores it. + doc := storedUnitDoc(t, exec, id) + param := bsonDocs(bsonValue(doc, "Parameters"))[0] + if !bsonnav.DSet(param, "IsRequired", false) { + t.Fatal("the stored parameter has no IsRequired to clear") + } + writeUnitDoc(t, exec, id, doc) + + out.Reset() + run(t, exec, "describe page MyFirstModule.Sale_View") + if !strings.Contains(out.String(), "$Sale: MyFirstModule.Sale optional") { + t.Errorf("describe page hides the Studio Pro optional flag:\n%s", out.String()) + } + + run(t, exec, `create or modify navigation Responsive + home page MyFirstModule.Home_Web +{ + menu item 'Home' ( OnClick: show page MyFirstModule.Home_Web ) + menu item 'Sale' ( OnClick: show page MyFirstModule.Sale_View ) +};`) + out.Reset() + run(t, exec, "describe navigation Responsive") + described := out.String() + if strings.Contains(described, "Sale = $currentObject") { + t.Fatalf("describe navigation invents an argument for the optional parameter:\n%s", described) + } + if !strings.Contains(described, "show page MyFirstModule.Sale_View") { + t.Fatalf("describe navigation lost the menu item:\n%s", described) + } + out.Reset() + if err := runErr(t, exec, described); err != nil { + t.Fatalf("re-running describe navigation is refused: %v", err) + } + if got := out.String(); !strings.Contains(got, "Unchanged") { + t.Errorf("re-running describe navigation wrote it:\n%s", got) + } +} + +// A required parameter is still mapped implicitly, and one stored before 11.5 +// — no IsRequired key at all — reads as required, not as optional. +func TestIssue1649_AParameterWithNoStoredFlagIsRequired(t *testing.T) { + exec, out := pageFixtureExecutor(t) + run(t, exec, issue1649Setup) + run(t, exec, strings.Replace(issue1649Page, " optional", "", 1)) + id := unitIDByName(t, exec, "page", "Sale_View") + + doc := storedUnitDoc(t, exec, id) + params, _ := bsonValue(doc, "Parameters").(bson.A) + p := params[1].(bson.D) + var kept bson.D + for _, e := range p { + if e.Key != "IsRequired" && e.Key != "DefaultValue" { + kept = append(kept, e) + } + } + params[1] = kept + writeUnitDoc(t, exec, id, doc) + if _, present := storedParameterRequired(t, exec, id); present { + t.Fatal("seed did not take") + } + + out.Reset() + run(t, exec, "describe page MyFirstModule.Sale_View") + if strings.Contains(out.String(), "optional") { + t.Errorf("a parameter with no stored IsRequired is described as optional:\n%s", out.String()) + } +} + +func TestIssue1649_OptionalIsRefusedWhereItCannotBeStored(t *testing.T) { + // A snippet parameter has no IsRequired at all. + prog, errs := visitor.Build(`create snippet MyFirstModule.SaleCard (Params: ( $Sale: MyFirstModule.Sale optional )) { + dynamictext txt (Content: 'x') +};`) + if len(errs) > 0 { + t.Fatal(errs) + } + var got []string + for _, v := range validateSnippetParameters(prog.Statements[0]) { + got = append(got, v.RuleID+" "+v.Message) + } + if !strings.Contains(strings.Join(got, "\n"), "optional") { + t.Errorf("an optional snippet parameter is not refused: %v", got) + } + + // A page parameter's IsRequired is 11.5+. + page := &ast.CreatePageStmtV3{Name: ast.QualifiedName{Module: "M", Name: "P"}, + Parameters: []ast.PageParameter{{Name: "Sale", EntityType: ast.QualifiedName{Module: "M", Name: "Sale"}, Optional: true}}} + for _, c := range []struct { + v [3]int + want bool + }{{[3]int{11, 4, 0}, true}, {[3]int{11, 5, 0}, false}} { + prog := &ast.Program{Statements: []ast.Statement{page}} + var hit bool + for _, vi := range versionGateViolations(versionCtx(t, c.v[0], c.v[1], c.v[2]), prog) { + if vi.RuleID == "MDL-VERSION01" && strings.Contains(vi.Message, "optional page parameter") { + hit = true + } + } + if hit != c.want { + t.Errorf("Mendix %v: optional page parameter refused = %v, want %v", c.v, hit, c.want) + } + } +} + +// `alter page … { add parameters $P: Type optional }` writes the flag too, and +// a snippet refuses it. +func TestIssue1649_AlterAddsAnOptionalParameter(t *testing.T) { + exec, _ := pageFixtureExecutor(t) + run(t, exec, issue1649Setup) + run(t, exec, `create page MyFirstModule.Sale_List (Title: 'Sales', Layout: Atlas_Core.Atlas_Default) { + dynamictext txt (Content: 'Sales') +};`) + run(t, exec, `alter page MyFirstModule.Sale_List { add parameters $Sale: MyFirstModule.Sale optional; };`) + id := unitIDByName(t, exec, "page", "Sale_List") + if required, present := storedParameterRequired(t, exec, id); required || !present { + t.Errorf("alter wrote IsRequired = %v (present %v), want false", required, present) + } + + run(t, exec, `create snippet MyFirstModule.SaleCard { dynamictext txt (Content: 'x') };`) + err := runErr(t, exec, `alter snippet MyFirstModule.SaleCard { add parameters $Sale: MyFirstModule.Sale optional; };`) + if err == nil || !strings.Contains(err.Error(), "only a page parameter can be optional") { + t.Errorf("an optional snippet parameter was added: %v", err) + } +} diff --git a/mdl/executor/issue548_expression_slot_test.go b/mdl/executor/issue548_expression_slot_test.go new file mode 100644 index 0000000000..6ea7c720e4 --- /dev/null +++ b/mdl/executor/issue548_expression_slot_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" +) + +// ako/mxcli#548: a pluggable widget's expression-typed property was written +// with its value in PrimitiveValue as well as in Expression. Studio Pro never +// does that: across ako/TestApp and PedApp (221 pages, snippets and layouts), +// every one of 939 Expression-typed WidgetValues stores PrimitiveValue "", and +// no other value type stores an Expression. So the first describe → exec of a +// page mxcli wrote was never "Unchanged" against one Studio Pro saved, and +// mxcli's own output differed from Studio Pro's on every combo box +// (customEditabilityExpression) — the property was hidden, and reset to its +// declared default with the primitive setter. + +const issue548Page = `create or modify page MyFirstModule.Things548 (Title: 'Things', Layout: Atlas_Core.Atlas_Default, Params: ($Thing: MyFirstModule.Thing)) { + dataview dvThing (DataSource: $Thing) { + combobox cbColour (Label: 'Colour', Attribute: Colour) + } + datagrid dgThings (DataSource: database from MyFirstModule.Thing) { + column (Attribute: Name, Caption: 'Name') + } + gallery galThings (DataSource: database from MyFirstModule.Thing) { + dynamictext txtName (Content: '{1}', ContentParams: ({1} = Name)) + } + image imgLogo (ImageType: 'imageUrl', ImageUrl: 'https://example.com/logo.png') +};` + +// expressionSlots lists, for every pluggable widget in doc, the Expression- +// typed properties whose stored value fills PrimitiveValue. +func expressionSlots(doc bson.D) (expressionProps int, both []string) { + var walk func(any) + walk = func(v any) { + switch n := v.(type) { + case bson.D: + if bsonString(n, "$Type") == "CustomWidgets$CustomWidget" { + types := map[string][2]string{} // property type id -> key, value type + var collect func(ot bson.D) + collect = func(ot bson.D) { + for _, pt := range bsonDocs(bsonValue(ot, "PropertyTypes")) { + vt := bsonDocField(pt, "ValueType") + types[bsonIDKey(bsonValue(pt, "$ID"))] = [2]string{bsonString(pt, "PropertyKey"), bsonString(vt, "Type")} + collect(bsonDocField(vt, "ObjectType")) + } + } + collect(bsonDocField(bsonDocField(n, "Type"), "ObjectType")) + var obj func(o bson.D) + obj = func(o bson.D) { + for _, p := range bsonDocs(bsonValue(o, "Properties")) { + kt := types[bsonIDKey(bsonValue(p, "TypePointer"))] + val := bsonDocField(p, "Value") + if kt[1] == "Expression" { + expressionProps++ + if pv := bsonString(val, "PrimitiveValue"); pv != "" { + both = append(both, fmt.Sprintf("%s.%s PrimitiveValue=%q Expression=%q", + bsonString(n, "Name"), kt[0], pv, bsonString(val, "Expression"))) + } + } + for _, item := range bsonDocs(bsonValue(val, "Objects")) { + obj(item) + } + } + } + obj(bsonDocField(n, "Object")) + } + for _, e := range n { + walk(e.Value) + } + case bson.A: + for _, e := range n { + walk(e) + } + } + } + walk(doc) + return expressionProps, both +} + +func TestIssue548_ExpressionPropertyIsNotWrittenAsAPrimitive(t *testing.T) { + exec, _ := pageFixtureExecutor(t) + run(t, exec, `create enumeration MyFirstModule.Colour (Red 'Red', Blue 'Blue'); +create persistent entity MyFirstModule.Thing (Colour: Enumeration(MyFirstModule.Colour), Name: String(100));`) + run(t, exec, issue548Page) + doc := storedUnitDoc(t, exec, unitIDByName(t, exec, "page", "Things548")) + + n, both := expressionSlots(doc) + if n < 4 { + t.Fatalf("found only %d Expression-typed properties, so the page shows nothing", n) + } + if len(both) > 0 { + t.Errorf("%d of %d Expression-typed properties store their value in PrimitiveValue too:\n %s", + len(both), n, strings.Join(both, "\n ")) + } + // The hidden combo-box property the report names keeps its declared + // default, in the Expression slot. + cb := namedDoc(t, doc, "cbColour") + v := widgetProp(t, cb, "customEditabilityExpression") + if got := bsonString(v, "Expression"); got != "false" { + t.Errorf("customEditabilityExpression Expression = %q, want its declared default false", got) + } +} diff --git a/mdl/executor/r2_rest_describe_test.go b/mdl/executor/r2_rest_describe_test.go index 806f83be7c..11eb62599c 100644 --- a/mdl/executor/r2_rest_describe_test.go +++ b/mdl/executor/r2_rest_describe_test.go @@ -154,7 +154,7 @@ func TestDescribePageHeaderMaps_InParens(t *testing.T) { BaseElement: model.BaseElement{ID: nextID("pg")}, ContainerID: mod.ID, Name: "Edit", - Parameters: []*pages.PageParameter{{Name: "Order", EntityName: "M.Order"}}, + Parameters: []*pages.PageParameter{{Name: "Order", EntityName: "M.Order", IsRequired: true}}, } mb := &mock.MockBackend{ IsConnectedFunc: func() bool { return true }, diff --git a/mdl/executor/roundtrip_restclient_test.go b/mdl/executor/roundtrip_restclient_test.go index a2a512ccc4..0b503fe756 100644 --- a/mdl/executor/roundtrip_restclient_test.go +++ b/mdl/executor/roundtrip_restclient_test.go @@ -142,13 +142,15 @@ func TestRoundtripRestClient_WithQueryParams(t *testing.T) { } };` + // Below 11.0 the statement's version gate (version_gates_documents.go) + // refuses it before the write is reached. if !restClientHasMendix11Shapes(env) { err := env.executeMDL(createMDL) if err == nil { - t.Fatal("a Query: operation was accepted on a Mendix 10 project; want the QueryParameters refusal") + t.Fatal("a Query: operation was accepted on a Mendix 10 project; want the query-parameter version gate") } - if !strings.Contains(err.Error(), "Rest$RestOperation.QueryParameters (Mendix 11.0.0+)") { - t.Fatalf("want the QueryParameters refusal, got: %v", err) + if !strings.Contains(err.Error(), "query: ( … ) on a REST operation requires Mendix 11.0.0+") { + t.Fatalf("want the query-parameter version gate, got: %v", err) } return } diff --git a/mdl/executor/validate_snippet_parameters.go b/mdl/executor/validate_snippet_parameters.go index 9af54aef72..c7514196ff 100644 --- a/mdl/executor/validate_snippet_parameters.go +++ b/mdl/executor/validate_snippet_parameters.go @@ -59,6 +59,14 @@ func validateSnippetParameters(stmt ast.Statement) []linter.Violation { var out []linter.Violation for _, p := range params { + if p.Optional { + out = append(out, linter.Violation{ + RuleID: "MDL087", + Severity: linter.SeverityError, + Location: linter.Location{Module: name.Module, DocumentType: "snippet", DocumentName: name.Name}, + Message: optionalSnippetParameterMessage(name, p.Name), + }) + } caption := types.SnippetParameterTypeRule(pageParamBSONType(p.Type)) if caption == "" { continue @@ -86,6 +94,15 @@ func validateSnippetParameters(stmt ast.Statement) []linter.Violation { return out } +// optionalSnippetParameterMessage refuses `optional` on a snippet parameter: +// Forms$SnippetParameter stores no IsRequired in any version, so the word +// would be dropped (mendixlabs/mxcli#1640). +func optionalSnippetParameterMessage(snippet ast.QualifiedName, param string) string { + return fmt.Sprintf("snippet '%s' declares parameter $%s optional — only a page parameter can be "+ + "optional (Forms$PageParameter.IsRequired, Mendix 11.5+); a snippet parameter is always "+ + "supplied by the snippet call. Remove `optional`", snippet.String(), param) +} + // paramTypeSourceName names a primitive parameter type the way MDL spells it, // for the message. It is deliberately not the CE0046 caption: the author is // looking for the word in their own script. diff --git a/mdl/executor/version_gates_documents.go b/mdl/executor/version_gates_documents.go index f75c2245d9..bda91c74c9 100644 --- a/mdl/executor/version_gates_documents.go +++ b/mdl/executor/version_gates_documents.go @@ -32,6 +32,38 @@ var documentTypeGates = []statementTypeGate{ return ok && !strings.EqualFold(s.ContainerType, "SNIPPET") && alterPageAddsParameter(s) }, }, + { + // `optional` writes Forms$PageParameter.IsRequired = false, a property + // below whose introduction every page parameter is required — the + // writer leaves it out there, so the word would be dropped + // (mendixlabs/mxcli#1640). + typeGate: typeGate{ + versionGate: versionGate{"pages", "page_parameter_optional", "an optional page parameter", + "below 11.5 every page parameter is required: remove `optional` and pass an argument wherever the page is opened, or upgrade the project"}, + types: []string{"Forms$PageParameter"}, + properties: []string{"Forms$PageParameter.IsRequired"}, + }, + applies: func(stmt ast.Statement) bool { + switch s := stmt.(type) { + case *ast.CreatePageStmtV3: + for _, p := range s.Parameters { + if p.Optional { + return true + } + } + case *ast.AlterPageStmt: + if strings.EqualFold(s.ContainerType, "SNIPPET") { + return false // refused for a snippet whatever the version + } + for _, op := range s.Operations { + if add, ok := op.(*ast.AddParameterOp); ok && add.Parameter.Optional { + return true + } + } + } + return false + }, + }, { typeGate: typeGate{ versionGate: versionGate{"pages", "snippet_parameters", "a snippet parameter", diff --git a/mdl/executor/widget_defs.go b/mdl/executor/widget_defs.go index f2841c8fa0..54db7ce262 100644 --- a/mdl/executor/widget_defs.go +++ b/mdl/executor/widget_defs.go @@ -124,7 +124,7 @@ func RefreshWidgetDefinitions(projectPath string, force bool, output io.Writer) stats.Refreshed++ } - if err := os.WriteFile(outPath, freshData, 0644); err != nil { + if err := writeDefinitionFile(outPath, freshData); err != nil { return stats, fmt.Errorf("failed to write %s: %w", outPath, err) } if output != nil { @@ -144,6 +144,35 @@ func RefreshWidgetDefinitions(projectPath string, force bool, output io.Writer) return stats, nil } +// writeDefinitionFile replaces a .def.json so that a reader sees the old +// content or the new, never a part of either. Definitions are read while they +// are refreshed — by another mxcli process, the language server, or (until +// #1799) the describe workers of the same catalog build — and os.WriteFile +// truncates first: a reader that lands in between gets an empty or half file, +// fails to parse it, and loads a registry without that widget. +// +// The temporary file does not end in .def.json, so a directory scan never +// reads it. Where the rename cannot replace the file (Windows, while a reader +// holds it open), it falls back to the plain write. +func writeDefinitionFile(path string, data []byte) error { + tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".tmp-*") + if err != nil { + return os.WriteFile(path, data, 0644) + } + tmpPath := tmp.Name() + _, werr := tmp.Write(data) + cerr := tmp.Close() + if werr == nil && cerr == nil { + if err := os.Chmod(tmpPath, 0644); err == nil { + if err := os.Rename(tmpPath, path); err == nil { + return nil + } + } + } + _ = os.Remove(tmpPath) + return os.WriteFile(path, data, 0644) +} + // RefreshStaleWidgetDefinitions makes a project's `.mxcli/widgets/*.def.json` // current before the engine reads them, in two cases: // diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index ae79fc5571..cfb0770f17 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -455,7 +455,7 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* for _, mapping := range mappings { if reset, hidden := hiddenSkip[strings.ToLower(mapping.PropertyKey)]; hidden { if reset != "" && mapping.Operation == "primitive" { - builder.SetPrimitive(mapping.PropertyKey, reset) + resetToDeclaredDefault(builder, propertyTypeIDs, mapping.PropertyKey, reset) } continue } @@ -488,7 +488,7 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* // The set of properties that must be default-valued is the widget's to decide, // not mxcli's; making it a subset of what MDL has words for was the mistake. for propKey, reset := range unmappedHiddenResets(mappings, e.visibilityRules(def), hiddenSkip) { - builder.SetPrimitive(propKey, reset) + resetToDeclaredDefault(builder, propertyTypeIDs, propKey, reset) } // 4. Auto datasource: map AST DataSource to first DataSource-type property. @@ -1030,6 +1030,21 @@ func unmappedHiddenResets(mappings []PropertyMapping, rules []types.WidgetVisibi return out } +// resetToDeclaredDefault writes a hidden property's declared default into the +// slot its value type keeps it in. An Expression property keeps it in +// Expression, with PrimitiveValue left empty: that is how Studio Pro stores +// every one of the 939 Expression-typed values across ako/TestApp and PedApp, +// and the primitive setter this used to call put a combo box's hidden +// customEditabilityExpression default in both (ako/mxcli#548). Every other type +// a reset reaches is a primitive. +func resetToDeclaredDefault(builder backend.WidgetObjectBuilder, ids map[string]pages.PropertyTypeIDEntry, key, value string) { + if ids[key].ValueType == "Expression" { + builder.SetExpression(key, value) + return + } + builder.SetPrimitive(key, value) +} + // templateAttrPlaceholderRe matches `{AttrName}` placeholders in a text // template. Numeric parameter references (`{1}`) don't match and pass through // verbatim. diff --git a/mdl/grammar/MDLLexer.g4 b/mdl/grammar/MDLLexer.g4 index b8f96bb8aa..50deaf6dbe 100644 --- a/mdl/grammar/MDLLexer.g4 +++ b/mdl/grammar/MDLLexer.g4 @@ -626,6 +626,7 @@ FEEDBACK: F E E D B A C K; RULE: R U L E; REQUIRED: R E Q U I R E D; NULLABLE: N U L L A B L E; // MODIFY ATTRIBUTE … NULLABLE — clears NOT NULL (Bug 12a) +OPTIONAL: O P T I O N A L; // Params: ( $P: Type OPTIONAL ) — an 11.5+ page parameter that is not required (mendixlabs/mxcli#1640) LOCALIZED: L O C A L I Z E D; // DateTime [NOT] LOCALIZED — DateTimeAttributeType.LocalizeDate (#1373) ERROR: E R R O R; IGNORE: I G N O R E; diff --git a/mdl/grammar/domains/MDLPage.g4 b/mdl/grammar/domains/MDLPage.g4 index 96c40a7bc6..a8c28e4ceb 100644 --- a/mdl/grammar/domains/MDLPage.g4 +++ b/mdl/grammar/domains/MDLPage.g4 @@ -60,8 +60,11 @@ pageParameterList : pageParameter (COMMA pageParameter)* ; +// `OPTIONAL` is Forms$PageParameter.IsRequired = false (Mendix 11.5+): the page +// may be opened without an argument for it (mendixlabs/mxcli#1640). A snippet +// parameter shares the rule and has no such flag; the executor refuses it there. pageParameter - : (IDENTIFIER | VARIABLE | QUOTED_IDENTIFIER) COLON dataType + : (IDENTIFIER | VARIABLE | QUOTED_IDENTIFIER) COLON dataType OPTIONAL? ; // A snippet parameter is a page parameter. There used to be a byte-identical diff --git a/mdl/grammar/domains/MDLSettings.g4 b/mdl/grammar/domains/MDLSettings.g4 index 749aab01d9..55d147c6ce 100644 --- a/mdl/grammar/domains/MDLSettings.g4 +++ b/mdl/grammar/domains/MDLSettings.g4 @@ -735,7 +735,7 @@ keyword | DATASOURCE | DEFAULT | DESIGNPROPERTIES | DESKTOPWIDTH | DISPLAY | DOCUMENTATION | EDITABLE | FILTER | FILTERTYPE | HEADER | FOOTER | ICON | GLYPH | GLYPHS | DARK | LABEL | ONCLICK | ONCHANGE | PARAMS | PASSING - | PHONEWIDTH | TABLETWIDTH | READONLY | RENDERMODE | REQUIRED | NULLABLE | LOCALIZED + | PHONEWIDTH | TABLETWIDTH | READONLY | RENDERMODE | REQUIRED | NULLABLE | LOCALIZED | OPTIONAL | SELECTION | STYLE | STYLING | TABINDEX | TITLE | TOOLTIP | URL | POSITION | VISIBLE | WIDTH | HEIGHT | WIDGETTYPE // Microflow document properties — keywords only inside a microflow header, diff --git a/mdl/linter/report.go b/mdl/linter/report.go index 726f4b32d7..104c12f2c3 100644 --- a/mdl/linter/report.go +++ b/mdl/linter/report.go @@ -79,6 +79,7 @@ var categoryMapping = map[string]string{ "CONV018": "Quality", "CONV019": "Quality", "CONV020": "Quality", + "CONV021": "Quality", // Architecture "ARCH001": "Architecture", diff --git a/mdl/linter/rules/conv_create_change_then_commit.go b/mdl/linter/rules/conv_create_change_then_commit.go new file mode 100644 index 0000000000..60d64e62b4 --- /dev/null +++ b/mdl/linter/rules/conv_create_change_then_commit.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// CreateChangeThenCommitRule flags a Create object or Change object activity +// that does not commit, directly followed by a Commit activity of that same +// object. The two activities do what one does: the create or change with its +// Commit option set (Yes, or Yes without events), which is how Studio Pro +// models it and what reviewers expect to see. +// +// It reports only a pair the merge would not change the meaning of: +// - the create/change has Commit = No, and the commit names its object; +// - the flow between them is the normal flow, not an error handler; +// - the commit has no other incoming flow, so no other path relies on it; +// - neither has custom error handling, which the merge would move or drop. +// +// Commit's "with events" maps onto Commit = Yes, "without events" onto Yes +// without events, and Refresh in client is kept if either had it. Microflows +// only, as asked for; a nanoflow's create/change offers the same option, but +// nothing measured that the pair behaves identically there. +type CreateChangeThenCommitRule struct{} + +func NewCreateChangeThenCommitRule() *CreateChangeThenCommitRule { + return &CreateChangeThenCommitRule{} +} + +func (r *CreateChangeThenCommitRule) ID() string { return "CONV021" } +func (r *CreateChangeThenCommitRule) Name() string { return "CreateChangeThenCommit" } +func (r *CreateChangeThenCommitRule) Category() string { return "quality" } +func (r *CreateChangeThenCommitRule) DefaultSeverity() linter.Severity { + return linter.SeverityWarning +} + +func (r *CreateChangeThenCommitRule) Description() string { + return "A Create or Change object activity directly followed by a Commit of the same object; set Commit on the create/change instead" +} + +func (r *CreateChangeThenCommitRule) Check(ctx *linter.LintContext) []linter.Violation { + if ctx.Reader() == nil { + return nil + } + var violations []linter.Violation + for mf := range ctx.Microflows() { + if mf.MicroflowType != "MICROFLOW" || ctx.IsExcluded(mf.ModuleName) { + continue + } + full, err := ctx.FullMicroflow(model.ID(mf.ID)) + if err != nil || full == nil || full.ObjectCollection == nil { + continue + } + violations = append(violations, findCreateChangeThenCommit(full.ObjectCollection, mf, r)...) + } + return violations +} + +// findCreateChangeThenCommit reports each create/change → commit pair in oc +// and in the bodies of its loops (a loop body has its own flows). +func findCreateChangeThenCommit(oc *microflows.MicroflowObjectCollection, mf linter.Microflow, + r *CreateChangeThenCommitRule) []linter.Violation { + + byID := map[model.ID]microflows.MicroflowObject{} + incoming := map[model.ID]int{} + for _, obj := range oc.Objects { + byID[obj.GetID()] = obj + } + for _, f := range oc.Flows { + incoming[f.DestinationID]++ + } + + var violations []linter.Violation + for _, f := range oc.Flows { + if f.IsErrorHandler || incoming[f.DestinationID] != 1 { + continue + } + from, ok := byID[f.OriginID].(*microflows.ActionActivity) + if !ok { + continue + } + to, ok := byID[f.DestinationID].(*microflows.ActionActivity) + if !ok { + continue + } + commit, ok := to.Action.(*microflows.CommitObjectsAction) + if !ok || !defaultErrorHandling(to) || !defaultErrorHandling(from) { + continue + } + kind, variable, refresh, ok := uncommittedCreateOrChange(from.Action) + if !ok || variable == "" || !strings.EqualFold(variable, commit.CommitVariable) { + continue + } + violations = append(violations, createChangeThenCommitViolation(mf, r, kind, variable, commit, refresh)) + } + + for _, obj := range oc.Objects { + if loop, ok := obj.(*microflows.LoopedActivity); ok && loop.ObjectCollection != nil { + violations = append(violations, findCreateChangeThenCommit(loop.ObjectCollection, mf, r)...) + } + } + return violations +} + +// uncommittedCreateOrChange returns the activity kind ("Create" or "Change"), +// the object variable it creates or changes, and its Refresh in client, when +// action is a create/change whose Commit is No. An empty Commit is unknown, not +// No, and is skipped. +func uncommittedCreateOrChange(action microflows.MicroflowAction) (kind, variable string, refresh, ok bool) { + switch a := action.(type) { + case *microflows.CreateObjectAction: + if a.Commit == microflows.CommitTypeNo { + return "Create", a.OutputVariable, a.RefreshInClient, true + } + case *microflows.ChangeObjectAction: + if a.Commit == microflows.CommitTypeNo { + return "Change", a.ChangeVariable, a.RefreshInClient, true + } + } + return "", "", false, false +} + +// defaultErrorHandling reports whether the activity's error handling is the +// default (Rollback), stored on the action or the activity, or left unset. +func defaultErrorHandling(a *microflows.ActionActivity) bool { + t := microflows.ObjectErrorHandlingType(a) + return t == "" || t == microflows.ErrorHandlingTypeRollback +} + +func createChangeThenCommitViolation(mf linter.Microflow, r *CreateChangeThenCommitRule, kind, variable string, + commit *microflows.CommitObjectsAction, refresh bool) linter.Violation { + + option, mdl := "Yes", "commit" + if !commit.WithEvents { + option, mdl = "Yes without events", "commit without events" + } + if refresh || commit.RefreshInClient { + option += ", with Refresh in client" + mdl += " refresh" + } + verb, statement := "changes", fmt.Sprintf("change $%s (…) %s;", variable, mdl) + if kind == "Create" { + verb, statement = "creates", fmt.Sprintf("$%s = create …(…) %s;", variable, mdl) + } + return linter.Violation{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("%s '%s.%s' %s $%s and commits it in the very next activity: "+ + "one %s object activity with Commit set does both.", + mf.DocumentNounTitle(), mf.ModuleName, mf.Name, verb, variable, kind), + Location: linter.Location{ + Module: mf.ModuleName, + DocumentType: mf.DocumentNoun(), + DocumentName: mf.Name, + DocumentID: mf.ID, + }, + Suggestion: fmt.Sprintf("Remove the Commit activity and set Commit to %s on the %s object activity (in MDL: %s)", + option, kind, statement), + } +} diff --git a/mdl/linter/rules/conv_create_change_then_commit_test.go b/mdl/linter/rules/conv_create_change_then_commit_test.go new file mode 100644 index 0000000000..dc5d4f69e7 --- /dev/null +++ b/mdl/linter/rules/conv_create_change_then_commit_test.go @@ -0,0 +1,161 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// CONV021: a Create or Change object activity without commit, directly followed +// by a Commit of the same object, is one create/change with Commit set. + +func activityWithID(id string, a microflows.MicroflowAction) *microflows.ActionActivity { + aa := µflows.ActionActivity{Action: a} + aa.ID = model.ID(id) + return aa +} + +func flow(from, to string) *microflows.SequenceFlow { + return µflows.SequenceFlow{OriginID: model.ID(from), DestinationID: model.ID(to)} +} + +func createOrder(commit microflows.CommitType) *microflows.ActionActivity { + return activityWithID("create", µflows.CreateObjectAction{ + EntityQualifiedName: "Shop.Order", + OutputVariable: "Order", + Commit: commit, + }) +} + +func changeOrder(commit microflows.CommitType) *microflows.ActionActivity { + return activityWithID("change", µflows.ChangeObjectAction{ChangeVariable: "Order", Commit: commit}) +} + +func commitVar(id, variable string, withEvents bool) *microflows.ActionActivity { + return activityWithID(id, µflows.CommitObjectsAction{CommitVariable: variable, WithEvents: withEvents}) +} + +func lintPairs(objects []microflows.MicroflowObject, flows ...*microflows.SequenceFlow) []linter.Violation { + oc := µflows.MicroflowObjectCollection{Objects: objects, Flows: flows} + return findCreateChangeThenCommit(oc, testMicroflow(), NewCreateChangeThenCommitRule()) +} + +func TestCreateThenCommit_IsReported(t *testing.T) { + got := lintPairs( + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), commitVar("commit", "Order", true)}, + flow("create", "commit"), + ) + if len(got) != 1 { + t.Fatalf("want 1 violation, got %d: %+v", len(got), got) + } + v := got[0] + if v.RuleID != "CONV021" || v.Severity != linter.SeverityWarning { + t.Errorf("got %s/%s, want CONV021/warning", v.RuleID, v.Severity) + } + if !strings.Contains(v.Message, "creates $Order") || !strings.Contains(v.Message, "Create object activity") { + t.Errorf("message %q should name the create and the variable", v.Message) + } + if !strings.Contains(v.Suggestion, "Commit to Yes on the Create object activity") || + !strings.Contains(v.Suggestion, "$Order = create …(…) commit;") { + t.Errorf("suggestion %q should name Commit = Yes and the MDL form", v.Suggestion) + } +} + +// A commit without events maps onto Yes without events, and Refresh in client +// set on either activity is kept. +func TestChangeThenCommitWithoutEvents_KeepsTheSettings(t *testing.T) { + commit := commitVar("commit", "Order", false) + commit.Action.(*microflows.CommitObjectsAction).RefreshInClient = true + got := lintPairs( + []microflows.MicroflowObject{changeOrder(microflows.CommitTypeNo), commit}, + flow("change", "commit"), + ) + if len(got) != 1 { + t.Fatalf("want 1 violation, got %d: %+v", len(got), got) + } + s := got[0].Suggestion + if !strings.Contains(s, "Yes without events, with Refresh in client on the Change object activity") || + !strings.Contains(s, "change $Order (…) commit without events refresh;") { + t.Errorf("suggestion %q should carry without events and refresh", s) + } + if !strings.Contains(got[0].Message, "changes $Order") { + t.Errorf("message %q should name the change", got[0].Message) + } +} + +// A pair inside a loop body is found there; the loop body has its own flows. +func TestCreateThenCommit_InsideALoop(t *testing.T) { + loop := µflows.LoopedActivity{ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{changeOrder(microflows.CommitTypeNo), commitVar("commit", "Order", true)}, + Flows: []*microflows.SequenceFlow{flow("change", "commit")}, + }} + if got := lintPairs([]microflows.MicroflowObject{loop}); len(got) != 1 { + t.Fatalf("want 1 violation from the loop body, got %d", len(got)) + } +} + +// Variable names are matched as Mendix does, without regard to case. +func TestCreateThenCommit_VariableCaseInsensitive(t *testing.T) { + got := lintPairs( + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), commitVar("commit", "order", true)}, + flow("create", "commit"), + ) + if len(got) != 1 { + t.Fatalf("want 1 violation, got %d", len(got)) + } +} + +// Each of these is not the pattern, or merging would change what the flow +// does, so none is reported. +func TestCreateThenCommit_NotReported(t *testing.T) { + customCommit := commitVar("commit", "Order", true) + customCommit.Action.(*microflows.CommitObjectsAction).ErrorHandlingType = microflows.ErrorHandlingTypeCustom + customCreate := createOrder(microflows.CommitTypeNo) + customCreate.Action.(*microflows.CreateObjectAction).ErrorHandlingType = microflows.ErrorHandlingTypeCustomWithoutRollback + errorFlow := flow("create", "commit") + errorFlow.IsErrorHandler = true + log := activityWithID("log", µflows.LogMessageAction{}) + + cases := []struct { + name string + objects []microflows.MicroflowObject + flows []*microflows.SequenceFlow + }{ + {"commit of another object", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), commitVar("commit", "Line", true)}, + []*microflows.SequenceFlow{flow("create", "commit")}}, + {"create already commits", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeYes), commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{flow("create", "commit")}}, + {"commit setting unknown", + []microflows.MicroflowObject{createOrder(""), commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{flow("create", "commit")}}, + {"an activity in between", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), log, commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{flow("create", "log"), flow("log", "commit")}}, + {"commit reached by another path too", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), log, commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{flow("create", "commit"), flow("log", "commit")}}, + {"reached through the error handler flow", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{errorFlow}}, + {"commit has custom error handling", + []microflows.MicroflowObject{createOrder(microflows.CommitTypeNo), customCommit}, + []*microflows.SequenceFlow{flow("create", "commit")}}, + {"create has custom error handling", + []microflows.MicroflowObject{customCreate, commitVar("commit", "Order", true)}, + []*microflows.SequenceFlow{flow("create", "commit")}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := lintPairs(c.objects, c.flows...); len(got) != 0 { + t.Errorf("want no violation, got %+v", got) + } + }) + } +} diff --git a/mdl/visitor/visitor_page.go b/mdl/visitor/visitor_page.go index 589d97af74..96a37ecb3a 100644 --- a/mdl/visitor/visitor_page.go +++ b/mdl/visitor/visitor_page.go @@ -66,6 +66,7 @@ func buildPageParameter(paramCtx *parser.PageParameterContext) ast.PageParameter Name: name, EntityType: entityType, Type: dataType, + Optional: paramCtx.OPTIONAL() != nil, } } diff --git a/modelsdk/mpr/reader_units.go b/modelsdk/mpr/reader_units.go index 058d45451e..d98da40473 100644 --- a/modelsdk/mpr/reader_units.go +++ b/modelsdk/mpr/reader_units.go @@ -322,6 +322,7 @@ func (r *Reader) listUnitsByTypeV2Where(typeName, containerID string, keep func( // comes from the type index when its ContentsHash is known there, so only new // and changed units are opened (reader_units_v2.go, mendixlabs/mxcli#1603). func (r *Reader) buildUnitCache() error { + r.announceColdUnitIndex() rows, err := r.db.Query(` SELECT UnitID, ContainerID, ContainmentName, COALESCE(ContentsHash, '') FROM Unit diff --git a/modelsdk/mpr/reader_units_v2.go b/modelsdk/mpr/reader_units_v2.go index 6178b76962..51ddadb639 100644 --- a/modelsdk/mpr/reader_units_v2.go +++ b/modelsdk/mpr/reader_units_v2.go @@ -5,6 +5,8 @@ package mpr import ( "bufio" "bytes" + "fmt" + "io" "os" "path/filepath" "strconv" @@ -59,6 +61,14 @@ const unitTypeIndexName = "unit-index-2.idx" // without it is ignored, so a format change only costs one cold start. const unitTypeIndexHeader = "mxcli-unit-index 2" +// ColdUnitIndexNotice, when set, is told in one line that a reader is about to +// build the unit index from nothing: no persisted index, or one in an older +// format. That build opens every unit file once — on the first command after an +// upgrade or on a fresh clone — and on Windows, where an open costs about a +// millisecond, it is a visible stall with no explanation (ako/mxcli#1806). nil, +// the default, says nothing: a library caller did not ask for stderr output. +var ColdUnitIndexNotice io.Writer + // unitIndexEntry is what the index knows of the unit whose bytes hash to its // key: everything derived from the top level of the document. type unitIndexEntry struct { @@ -108,6 +118,31 @@ func (r *Reader) knownUnit(hash string) (unitIndexEntry, bool) { return e, ok } +// announceColdUnitIndex tells ColdUnitIndexNotice that this build starts from +// an empty index and so opens every unit file. Only where the index is +// persisted: without a path every build is cold, and a notice on every command +// would promise a "once" that never comes. Called before buildUnitCache opens +// its row query, so the count runs on a free connection. +func (r *Reader) announceColdUnitIndex() { + w := ColdUnitIndexNotice + if w == nil || r.unitTypeIndexPath() == "" { + return + } + r.v2TypesMu.Lock() + r.knownUnit("") // loads the persisted index on first use + cold := len(r.v2Types) == 0 + r.v2TypesMu.Unlock() + if !cold { + return + } + var units int + if err := r.db.QueryRow(`SELECT COUNT(*) FROM Unit`).Scan(&units); err != nil || units == 0 { + return + } + fmt.Fprintf(w, "Building the unit index for this project's %d units (once; later commands reuse .mxcli/%s)…\n", + units, unitTypeIndexName) +} + // learnUnit records a hash's entry. Caller holds v2TypesMu. func (r *Reader) learnUnit(hash string, e unitIndexEntry) { if hash == "" { diff --git a/modelsdk/mpr/reader_units_v2_notice_test.go b/modelsdk/mpr/reader_units_v2_notice_test.go new file mode 100644 index 0000000000..1f42248752 --- /dev/null +++ b/modelsdk/mpr/reader_units_v2_notice_test.go @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "bytes" + "strings" + "testing" +) + +// ako/mxcli#1806: the first command after an upgrade (or on a fresh clone) +// builds the unit index by opening every unit file — 641 opens where the +// previous build opened 383, before the steady-state win. On Windows, with an +// open costing about a millisecond, that is an unexplained stall at the moment +// a user is checking whether the new version works. Say it is happening. +func TestBuildingTheUnitIndexFromNothingIsAnnounced(t *testing.T) { + const n = 40 + p, _ := seedProject(t, n) + + var notice bytes.Buffer + prev := ColdUnitIndexNotice + ColdUnitIndexNotice = ¬ice + t.Cleanup(func() { ColdUnitIndexNotice = prev }) + + listModules(t, p.reader()) + got := notice.String() + if !strings.Contains(got, "unit index") || !strings.Contains(got, "41 units") { + t.Errorf("cold build: notice = %q, want one line naming the unit index and the 41 units", got) + } + if strings.Count(got, "\n") != 1 { + t.Errorf("cold build: notice printed %d lines, want 1: %q", strings.Count(got, "\n"), got) + } + + // Control: the next process finds the index and says nothing. + notice.Reset() + listModules(t, p.reader()) + if notice.Len() != 0 { + t.Errorf("warm build announced an index build: %q", notice.String()) + } +} + +// A library caller that did not ask for the notice gets none. +func TestUnitIndexNoticeIsSilentByDefault(t *testing.T) { + if ColdUnitIndexNotice != nil { + t.Fatalf("ColdUnitIndexNotice defaults to %v, want nil", ColdUnitIndexNotice) + } +} diff --git a/sdk/versions/mendix-11.yaml b/sdk/versions/mendix-11.yaml index bd37439884..2d7727fb88 100644 --- a/sdk/versions/mendix-11.yaml +++ b/sdk/versions/mendix-11.yaml @@ -145,6 +145,13 @@ features: page_parameters: min_version: "11.0.0" mdl: "CREATE PAGE Module.Name (Param: Type, ...) { ... }" + page_parameter_optional: + # Studio Pro's "Required" checkbox on a page parameter, unticked: + # Forms$PageParameter.IsRequired = false. The property is introduced in + # 11.5.0 (modelsdk/gen/pages/version.go, from the reflection data); below + # it every page parameter is required (mendixlabs/mxcli#1640). + min_version: "11.5.0" + mdl: "CREATE PAGE Module.Name (Params: ( $P: Module.Entity optional )) { ... }" page_variables: min_version: "11.0.0" flow_call_output_mappings: