diff --git a/.claude/skills/fix-issue.md b/.claude/skills/fix-issue.md index e3ab54dae..61f72112b 100644 --- a/.claude/skills/fix-issue.md +++ b/.claude/skills/fix-issue.md @@ -364,6 +364,9 @@ cases for these three BSON types — they fell to `default: return nil`. | The SessionStart hook `mxcli init` writes cannot survive an idle reap: it is guarded on `test -x ./mxcli`, and `.gitignore` excludes that binary (~85 MB) on purpose. The container is reclaimed, the repo re-cloned without it, the guard fails and the hook no-ops through `\|\| true` — the next session has no mxcli, no MxBuild cache, no database, and no message saying so | The hook inlined the whole bring-up in one shell line, so "binary missing" could only be expressed as "skip". A hook line cannot reasonably do OS/arch detection and a download; something committed has to | `cmd/mxcli/init_hook.go` (`bootstrapScriptTemplate`, `writeBootstrapScript`, `sessionStartHookCommand`, `sessionStartHookMarkers`) | Emit a committed `.claude/bootstrap-mxcli.sh` that resolves OS/arch, fetches the binary when absent (`MXCLI_TAG` to pin), then runs the setup; the hook becomes `sh .claude/bootstrap-mxcli.sh \|\| true`. **Changing the hook command breaks dedupe**, which matched on the old command string — so `addSessionStartHook` now recognises *any* known marker and **rewrites the entry in place**, migrating an old project instead of leaving it with two hooks that both run. **Generalisable**: a guard whose condition is something you deliberately do not commit is a silent no-op waiting for a fresh clone — make the guard able to satisfy itself. Verified by reproducing the reap: moved `./mxcli` out of the project, ran the hook command verbatim, watched it re-download (88 MB, new mtime) and finish with "Setup complete … database ready". Tests `TestAddSessionStartHook_MigratesLegacyCommand`, `TestEnsureSessionStartHook_WritesFile`. mxcli-todo #2 | | `mxcli exec -p app.mpr - <<'EOF' … EOF` fails with `Error reading file: open -: no such file or directory` — `-` is taken literally as a filename, so MDL cannot be piped or written as a heredoc and every ad-hoc script needs a temp file first | `exec` (and `check`) called `os.ReadFile(path)` directly, with no case for the conventional stdin spelling | `cmd/mxcli/mdlsource.go` (new `readMDLSource`, `mdlSourceLabel`), `cmd/mxcli/cmd_exec.go`, `cmd/mxcli/cmd_check.go` | One helper both commands share, so `check` gained the same spelling rather than only the reported one; `check` reports the source as `` instead of a bare `-`. Verified live: a heredoc through `exec` and a pipe through `check` both run. Tests `cmd/mxcli/mdlsource_test.go`. mxcli-todo #5 | | `mxcli syntax` documents spellings the parser rejects, so an agent following the reference writes MDL that fails — `TEXTBOX … (Binds: Attr)` ("'Binds:' is no longer supported, use 'Attribute:' instead") and `DataSource: MICROFLOW Module.MF()` (a zero-arg microflow datasource takes NO parens, unlike RETRIEVE/CALL) | Nothing checks the `syntax` corpus against the parser. `make check-skill-mdl` validates MDL blocks in the skills and the docs site, but the `Syntax`/`Example` strings in `cmd/mxcli/syntax/*.go` are not covered, so a retired spelling can sit there indefinitely | `cmd/mxcli/syntax/features_page.go` (10 × `Binds:` → `Attribute:`, the datasource parens), `cmd/mxcli/syntax/retired_spellings_test.go` (new guard) | Fix the text **and** pin it: a table-driven test fails if a retired spelling reappears in any topic's Syntax or Example. It is a spelling guard rather than a parse — the snippets are fragments (a DATAVIEW body, a property line) that do not stand alone as statements, so they cannot just be fed to the parser. Proven by reintroducing `Binds:` and watching the test name the topic and field. **A third claim in the same report did not reproduce**: `CONTAINER (OnClick: SHOW_PAGE M.P(Param: $currentObject))` parses fine on current main, so only the two verified ones were changed. mxcli-todo #8 | +| `alter page … set Editable = [expr]` (or `set Visible`) writes a project Studio Pro refuses to open: `StorageLoadException: Conditional editability settings has an invalid value '' for property Attribute`. `mxcli check` ✓ and `mx check` ✓ — neither inspects the stored value. The identical settings written by `create page` load fine | The ALTER path builds the `Forms$Conditional{Visibility,Editability}Settings` node by hand and wrote `Attribute: null`. `Attribute` is a **BY_NAME** `AttributeIdentifier`, so its unset value is the empty string, not null — exactly what the CREATE path already encodes via `codec.RegisterTypeDefaults(..., EmptyStringFields: []string{"Attribute"})`, whose comment records this same StorageLoadException from #627. Only the hand-built ALTER node missed it | `mdl/backend/pagemutator/mutator.go` (`setWidgetConditionalSettingMut`) | Write `{Key: "Attribute", Value: ""}`, not `nil`. **General rule: when one path hand-builds BSON that another path builds through the codec, diff the two encodings rather than eyeballing the hand-built one** — `mxcli bson dump --type page --object M.P` on a CREATE-authored and an ALTER-authored widget makes the divergence a one-line diff (key sets and values were otherwise identical). `SourceVariable` stays `nil`: it is BY_ID, where null *is* the absent value, so "null is wrong" is per-field, not a blanket rule. Test `TestSetWidgetConditionalSetting_AttributeIsEmptyString`; repro `mdl-examples/bug-tests/851-alter-page-conditional-attribute.mdl`. Issue #851 | +| A widget conditional using a function whose name is also an MDL lexer keyword — `visible: [trim($currentObject/Slug) != '']`, `[length(…) > 0]`, `empty`/`count`/`find` — **silently drops the whole property**; `mxcli check` ✓, `mx check` ✓, and the widget renders unconditionally visible. `toUpperCase`/`isMatch`/`contains` in the same position work | `xpathFunctionName` (MDLPage.g4) enumerated only `IDENTIFIER \| HYPHENATED_ID \| NOT \| TRUE \| FALSE \| CONTAINS`, so `trim(` never matched `xpathFunctionCall`. The enclosing `[...]` then failed to parse as an `xpathConstraint` and matched the generic `propertyValueV3` alternative instead, so the visitor set `Visible` (an array) rather than `VisibleIf`, and the builder's `else if pages.StaticVisibleExpression(...)` — which reads only bool/string — never fired | `mdl/grammar/domains/MDLPage.g4` (`xpathFunctionName`) + `mdl/executor/validate_widgets.go` (`validateConsumableConditional`) | Define `xpathFunctionName : xpathWord \| NOT` — `xpathWord` is a negated token set, so it self-maintains as the lexer gains keywords; an enumerated list reacquires this bug with the next promoted function name. Safe because `xpathFunctionCall` requires a following LPAREN and no `xpathStepValue` may be followed by one, so bare `empty` still parses as a path word. `NOT` is spelled out (xpathWord excludes it). **Also add the general guard**: MDL-WIDGET19 errors when `Visible`/`Editable` holds a value that is neither routed to `VisibleIf`/`EditableIf` nor a bool/string — that is the residue signature of any conditional the visitor could not build, so the next one fails loudly instead of vanishing. `make grammar` regenerates the parser (not committed). **Verify in a browser, not at `mx check`** — a dropped property is still a valid model, so `mx check` reports 0 errors before AND after; the symptom only exists at render time (see `verify-in-runtime.md`). The repro script carries a `Bug852.Verify` page for this: `Slug` is three spaces, so `trim()` changes the outcome and a dropped `Visible` renders (Mendix defaults to visible). Pre-fix all 5 markers render; post-fix only the 3 that should. **One rule, two contexts**: `xpathConstraint` serves both `Visible:`/`Editable:` (a Mendix *client expression* — trim/length/toUpperCase/find) and a datasource `where` (real *XPath* — contains/starts-with/ends-with/string-length/not, `length()` = list length, aggregates Java-only, and `empty`/`NULL` are KEYWORDS not calls). The sets differ, so the grammar must not enumerate either; mxbuild adjudicates. Regression-test the XPath side when touching this rule — `[Name = empty]`, `[Name = NULL]`, `not()`, `contains()`, `starts-with()`, `string-length()` all still parse and `mx check` clean. Tests `TestConditionalVisibility_KeywordFunctionNames`, `TestValidateStaticWidget_UnconsumableConditional`; repro `mdl-examples/bug-tests/852-conditional-keyword-functions.mdl`. Issue #852 | +| `download file $Doc;` is accepted by `mxcli check` and `mxcli exec` ("Created microflow") but the activity lands with **no action at all** — `describe` renders `-- Empty action` and `mx check` fails `[CE0008] "No action defined."`. Same for `download file $Doc show in browser;` | `microflowActionToGen` (the modelsdk write path) had no `*microflows.DownloadFileAction` case, so it hit `default: return nil` and the enclosing ActionActivity was serialized with a nil Action. Grammar, visitor, flow builder, read path and DESCRIBE formatter were all already in place, so the statement passed every stage that reports anything and vanished at the one that does not | `mdl/backend/modelsdk/microflow_write.go` (`microflowActionToGen`) | Add the case, setting `FileDocumentVariableName`, `ShowFileInBrowser` and `ErrorHandlingType` (Rollback default). **The storage key is `ShowFileInBrowser`, not `ShowInBrowser`** — the gen setter binds the right one; legacy's `parseDownloadFileAction` reads the wrong key. **Test at the round trip, not the reader**: a reader-only test starts from BSON the writer never had to produce, so `TestActionFromGen_DownloadFile` was green throughout. `roundTripMicroflow` (model→gen→codec→model) is the harness; assert the ActionActivity's `Action` is non-nil, which is the CE0008 shape itself. This is the same silent-drop mechanism as the `microflowObjectToGen` default branch (#791) — when auditing, diff the write switch's cases against `sdk/mpr/writer_microflow_actions.go`. Test `TestMicroflowRoundTrip_DownloadFile`; repro `mdl-examples/bug-tests/850-download-file-action.mdl`. Issue #850 | | A decision written with **uppercase** keywords — `IF $T/Status != M.Status.Done AND $T/CompletedOn != empty` — passes `mxcli check` and then fails the build with `[error] [CE0117] "Error(s) in expression."`, quoting the expression back with `AND` still uppercase. The same condition written with `=` builds fine, which makes it look like `!=` cannot be an operand of `AND` | Mendix requires its word operators lowercase. A rebuilt `BinaryExpr` gets `strings.ToLower(e.Operator)`; a condition kept as an `ast.SourceExpr` (original text **plus** the parsed tree) returned `e.Source` verbatim and skipped it. The `=` form parses to a BinaryExpr and the `!=` form to a SourceExpr — hence the operator-shaped illusion | `mdl/executor/cmd_microflows_helpers.go` (`normalizeMendixOperatorCase`, applied in the `SourceExpr` branch) | Lowercase `and/or/not/div/mod` in preserved source, leaving everything else byte-identical: a scanner that tracks single-quoted literals (with `''` escapes) and skips any word preceded by `.`, `/` or `$`, so `'AND'`, `M.Enum.And`, `$Task/Mod` and `$Android` are untouched. **The reporter's own probe table is the cautionary bit** — nine builds established a rule ("any `!=` inside `AND` fails") that was real in every observation and wrong about the cause, because every failing probe was uppercase and the control was not. When a table's rule tracks a token, check what ELSE differs between the rows. Reproduced and fixed against mxbuild 11.12.1: stored `AND` → CE0117, stored `and` → 0 errors. Repro `mdl-examples/bug-tests/todo-14b-uppercase-and-operator.mdl`; tests `TestNormalizeMendixOperatorCase`. mxcli-todo #14b | | `ALTER ENTITY X ADD EVENT HANDLER …` errors when the handler exists and `DROP EVENT HANDLER …` errors when it does not, so a script containing either cannot be re-run — and a defensive drop-then-add fails on whichever half does not match. `ADD ATTRIBUTE` has `IF NOT EXISTS`; event handlers had no equivalent | The idempotency guards added for attributes (findings #10) were never extended to the event-handler clauses, which are the one member with no other re-run route | `mdl/grammar/domains/MDLDomainModel.g4` (`ifNotExists?` / `ifExists?` on the event-handler clauses), `mdl/visitor/visitor_entity.go`, `mdl/executor/cmd_entities.go` | Reuse the existing `ifNotExists`/`ifExists` grammar rules rather than inventing a second spelling, so the guard reads the same everywhere. The error messages now name the flag, so the fix is discoverable from the failure. Verified by running the same script twice: first run drops (skipping, absent) then adds; second run skips both, exit 0, and the project still builds 0 errors. mxcli-todo #18 | | `CREATE DEMO USER` reports success and `SHOW PROJECT SECURITY` reports `Demo Users Enabled: true`, yet the running app has **zero** accounts — `SELECT Name FROM Administration.Account` returns 0 rows, there is no login page, and none of the row-level XPath rules are enforced | A blank mxcli template ships with **Security Level: Off**, and with it off the runtime creates no accounts at all. The demo users are written to the model correctly; nothing connected the two facts, so the model said yes and the app said nothing | `mdl/executor/cmd_security_write.go` (`warnDemoUsersInert`, called after a successful create) | Say it at the moment the user would otherwise believe it worked, and name the one statement that fixes it (`alter project security level prototype`). A *warning*, not a refusal: authoring demo users before raising the level is legitimate ordering. **Generalisable**: when a write succeeds but a project-level setting makes it inert, the write path is the only place with both facts in hand. Tests `TestWarnDemoUsersInert`. mxcli-todo #15 | diff --git a/.claude/skills/mendix/create-page.md b/.claude/skills/mendix/create-page.md index d366f6b4c..fbde3e364 100644 --- a/.claude/skills/mendix/create-page.md +++ b/.claude/skills/mendix/create-page.md @@ -1082,8 +1082,38 @@ textbox txtHidden (label: 'Hidden', attribute: Name, visible: false) -- A quoted-string expression is also accepted (CREATE and ALTER). Unlike the -- bracket form, it is NOT auto-rooted — write $currentObject/ yourself. dynamictext ovChip (content: 'chip', visible: '$currentObject/Name != empty') + +-- Function calls work in the bracket form, including functions whose name is +-- also an MDL keyword (trim, length, find). Arguments are rooted like any other +-- reference. +dynamictext tTrim (content: 'x', visible: [trim($currentObject/Slug) != '']) +textbox txtSlug (label: 'Slug', attribute: Slug, editable: [length(Slug) > 0]) ``` +> **`visible:`/`editable:` is a Mendix *expression*, not XPath** — a different +> function set from a datasource `where` clause, even though both use `[ ... ]`: +> +> | | `visible:` / `editable:` (client expression) | `where [ … ]` (XPath) | +> |---|---|---| +> | String tests | `trim()`, `length()`, `toUpperCase()`, `find()`, `contains()` | `contains()`, `starts-with()`, `ends-with()`, `string-length()` | +> | `length()` | character count | number of elements in a list | +> | Emptiness | `$currentObject/X != ''` / `!= empty` | `[X = empty]` or `[X = NULL]` — a **keyword**, never `empty(…)` | +> | Aggregates | not available | `count()`/`avg()`/`min()`/`max()`/`sum()` are Java-API-only | +> +> mxcli's grammar accepts any function name in both and lets MxBuild adjudicate, +> so a wrong-context call surfaces as **CE0117** "Error(s) in expression" at +> build rather than as a parse error. See the Mendix reference guide: +> [XPath constraint functions](https://docs.mendix.com/refguide/xpath-constraint-functions/), +> [XPath keywords](https://docs.mendix.com/refguide/xpath-keywords-and-system-variables/). + +> **An unparseable conditional is an error, not a silent drop.** If the +> expression inside `visible: [ ... ]` / `editable: [ ... ]` can't be parsed, the +> property has nowhere to go and would vanish on write — leaving the widget +> unconditionally visible/editable, which looks identical to a specificity bug in +> the running app. `mxcli check` reports this as **MDL-WIDGET19** and fails the +> command instead. Until v0.16.x, `trim(…)` and `length(…)` hit exactly this path +> and disappeared without a word (issue #852). + > **Attribute rooting is automatic** — a bare attribute in a widget > visibility/editability expression (`[Name != '']`, `[IsActive]`) is rooted in the > widget data context as `$currentObject/Name != ''` for you, so it no longer diff --git a/mdl-examples/bug-tests/850-download-file-action.mdl b/mdl-examples/bug-tests/850-download-file-action.mdl new file mode 100644 index 000000000..73931c3f3 --- /dev/null +++ b/mdl-examples/bug-tests/850-download-file-action.mdl @@ -0,0 +1,49 @@ +-- ============================================================================ +-- Issue #850 — `download file` parsed but never written to the project +-- ============================================================================ +-- +-- `microflowActionToGen` (mdl/backend/modelsdk/microflow_write.go) switches on +-- the semantic action type and had no `*microflows.DownloadFileAction` case, so +-- the action fell through to `default: return nil` and the enclosing +-- ActionActivity was written with no Action at all. +-- +-- Everything either side of the writer was already in place — grammar, visitor, +-- flow builder, read path and DESCRIBE formatter — so the statement was accepted +-- at every stage that reports anything and vanished at the one stage that does +-- not. `mxcli check` passed, `mxcli exec` printed "Created microflow", and only +-- `mx check` noticed: +-- +-- [error] [CE0008] "No action defined." at Action activity 'Activity' +-- +-- To verify: +-- 1. Run this script. +-- 2. `mxcli describe microflow Bug850.ACT_Download` — the body must render +-- `download file $Doc;`, NOT `-- Empty action`. Likewise +-- `Bug850.ACT_DownloadInBrowser` must render `… show in browser;`. +-- 3. mx check should report 0 errors (CE0008 before the fix). +-- +-- Note the storage key is ShowFileInBrowser, not ShowInBrowser. +-- ============================================================================ + +CREATE MODULE Bug850; + +CREATE OR MODIFY PERSISTENT ENTITY Bug850.Doc EXTENDS System.FileDocument ( +); + +CREATE OR MODIFY MICROFLOW Bug850.ACT_Download ( + $Doc: Bug850.Doc +) +RETURNS Boolean +BEGIN + download file $Doc; + return true; +END; + +CREATE OR MODIFY MICROFLOW Bug850.ACT_DownloadInBrowser ( + $Doc: Bug850.Doc +) +RETURNS Boolean +BEGIN + download file $Doc show in browser; + return true; +END; diff --git a/mdl-examples/bug-tests/851-alter-page-conditional-attribute.mdl b/mdl-examples/bug-tests/851-alter-page-conditional-attribute.mdl new file mode 100644 index 000000000..b746ce019 --- /dev/null +++ b/mdl-examples/bug-tests/851-alter-page-conditional-attribute.mdl @@ -0,0 +1,63 @@ +-- ============================================================================ +-- Issue #851 — `alter page … set Editable = [expr]` produced an unopenable project +-- ============================================================================ +-- +-- The ALTER path built the Forms$Conditional{Visibility,Editability}Settings node +-- by hand and wrote `Attribute: null`. Attribute is a BY_NAME AttributeIdentifier, +-- so its unset value is the empty string; a null fails the reader with +-- +-- StorageLoadException: Conditional editability settings has an invalid value '' +-- for property Attribute +-- +-- The CREATE path already encoded it as "" (via the TypeDefaults in +-- mdl/backend/modelsdk/widget_write.go), which is why a widget authored with +-- CREATE loaded and the same widget authored with ALTER did not. Neither +-- `mxcli check` nor `mx check` inspects the stored value, so both reported +-- success on the broken project. +-- +-- To verify in Studio Pro: +-- 1. Run this script. +-- 2. Open Bug851.PageAltered — it must open without a StorageLoadException, +-- and txtAltered must show "Editable: conditional" with the expression. +-- 3. Compare against Bug851.PageCreated (same settings via CREATE): the two +-- widgets' conditional settings must be structurally identical. +-- 4. mx check should report 0 errors. +-- ============================================================================ + +CREATE MODULE Bug851; + +CREATE OR MODIFY PERSISTENT ENTITY Bug851.Thing ( + Slug: String(200) +); + +-- Reference: the CREATE path, which was already correct. +CREATE OR REPLACE PAGE Bug851.PageCreated ( + Params: { $currentObject: Bug851.Thing }, + Title: 'Conditional settings via CREATE', + Layout: Atlas_Core.Atlas_Default +) { + dataview dv (datasource: $currentObject) { + textbox txtCreated ( + label: 'Slug', + attribute: Slug, + editable: [$currentObject/Slug != ''], + visible: [$currentObject/Slug != ''] + ) + } +} + +-- The regression: the same two settings applied through ALTER. +CREATE OR REPLACE PAGE Bug851.PageAltered ( + Params: { $currentObject: Bug851.Thing }, + Title: 'Conditional settings via ALTER', + Layout: Atlas_Core.Atlas_Default +) { + dataview dv (datasource: $currentObject) { + textbox txtAltered (label: 'Slug', attribute: Slug) + } +} + +ALTER PAGE Bug851.PageAltered { + set Editable = [$currentObject/Slug != ''] on txtAltered; + set Visible = [$currentObject/Slug != ''] on txtAltered; +}; diff --git a/mdl-examples/bug-tests/852-conditional-keyword-functions.mdl b/mdl-examples/bug-tests/852-conditional-keyword-functions.mdl new file mode 100644 index 000000000..6e5e9791c --- /dev/null +++ b/mdl-examples/bug-tests/852-conditional-keyword-functions.mdl @@ -0,0 +1,103 @@ +-- ============================================================================ +-- Issue #852 — `trim()` / `length()` in a widget conditional silently dropped it +-- ============================================================================ +-- +-- `xpathFunctionName` admitted only IDENTIFIER, HYPHENATED_ID, NOT, TRUE, FALSE +-- and CONTAINS, so a call to a function whose name is also an MDL lexer keyword +-- never matched `xpathFunctionCall`. The enclosing `Visible: [...]` then failed +-- to parse as an xpathConstraint, fell through to the generic property-value +-- alternative, and the widget's whole conditional property was dropped — with no +-- diagnostic from `mxcli check` or `mx check`. +-- +-- A dropped Visible defaults to "always visible", so the only symptom was a +-- widget that should have been hidden showing up in the running app. +-- +-- toUpperCase / contains were never affected (plain IDENTIFIERs, and CONTAINS was +-- listed) — they are included below so the two classes can be compared directly. +-- +-- To verify: +-- 1. Run this script. +-- 2. `mxcli describe page Bug852.PageConditionals` — EVERY dynamictext must +-- carry its Visible expression. Before the fix, tKeyword* lost theirs while +-- tIdentifier* kept theirs. +-- 3. mx check should report 0 errors. +-- +-- Only Mendix-valid client-expression functions appear below. `count(…)` and +-- `empty(…)` are ALSO keyword tokens and now parse, but they are not client +-- expression functions — `empty` is a literal (`$x != empty`), not a call — so +-- mxbuild rejects them with CE0117. Before this fix they were silently dropped; +-- now they fail loudly at `mx check`, which is the intended direction but is a +-- visible behaviour change for anyone who had written one. +-- ============================================================================ + +CREATE MODULE Bug852; + +CREATE OR MODIFY PERSISTENT ENTITY Bug852.Thing ( + Slug: String(200) +); + +CREATE OR REPLACE PAGE Bug852.PageConditionals ( + Params: { $currentObject: Bug852.Thing }, + Title: 'Conditionals using keyword-named functions', + Layout: Atlas_Core.Atlas_Default +) { + dataview dv (datasource: $currentObject) { + -- Keyword-named functions: these were the dropped ones. + dynamictext tKeywordTrim (content: 'trim', visible: [trim($currentObject/Slug) != '']) + dynamictext tKeywordLength (content: 'length', visible: [length($currentObject/Slug) > 0]) + dynamictext tKeywordFind (content: 'find', visible: [find($currentObject/Slug, 'x') >= 0]) + + -- Identifier-named functions: these always worked. Guard against regressing them. + dynamictext tIdentifierUpper (content: 'upper', visible: [toUpperCase($currentObject/Slug) != '']) + dynamictext tIdentifierContains (content: 'contains', visible: [contains($currentObject/Slug, 'x')]) + + -- A bare attribute inside a keyword-named call is still rooted in the data + -- context, exactly as it is outside one. + textbox txtEditable (label: 'Slug', attribute: Slug, editable: [trim(Slug) != '']) + } +} + +-- ============================================================================ +-- Runtime verification page (issue #852) +-- ============================================================================ +-- +-- Slug is three spaces: non-empty as a raw string, empty once trimmed. That is +-- what makes trim() observable rather than incidental — a widget whose Visible +-- expression was dropped renders, because the Mendix default is "visible". +-- +-- Boot with `mxcli run --local -p .mpr` and open /p/verify852: +-- TRIM_HIDDEN_MARKER must NOT render (trim(' ') = '' -> false) +-- TRIM_VISIBLE_MARKER must render (negation of the above) +-- LEN_HIDDEN_MARKER must NOT render (length(trim(' ')) = 0) +-- NOTRIM_VISIBLE_MARKER must render (control: ' ' != '' is true, so +-- trim() genuinely changes the outcome) +-- PAGE_RENDERED_BEACON must render (proves the page rendered at all) +-- +-- Before the fix all five rendered. `mx check` reported 0 errors either way — +-- a dropped property is still a valid model, which is why this needs a browser. +-- ============================================================================ + +CREATE OR MODIFY NON-PERSISTENT ENTITY Bug852.Probe ( + Slug: String(200) +); + +CREATE OR MODIFY MICROFLOW Bug852.DS_Probe () +RETURNS Bug852.Probe +BEGIN + $p = create Bug852.Probe (Slug = ' '); + return $p; +END; + +CREATE OR REPLACE PAGE Bug852.Verify ( + Title: 'Verify 852', + Layout: Atlas_Core.Atlas_Default, + Url: 'verify852' +) { + dataview dv (datasource: microflow Bug852.DS_Probe) { + dynamictext tTrimHidden (content: 'TRIM_HIDDEN_MARKER', visible: [trim($currentObject/Slug) != '']) + dynamictext tTrimVisible (content: 'TRIM_VISIBLE_MARKER', visible: [trim($currentObject/Slug) = '']) + dynamictext tLenHidden (content: 'LEN_HIDDEN_MARKER', visible: [length(trim($currentObject/Slug)) > 0]) + dynamictext tNoTrimVisible (content: 'NOTRIM_VISIBLE_MARKER', visible: [$currentObject/Slug != '']) + dynamictext tBeacon (content: 'PAGE_RENDERED_BEACON') + } +} diff --git a/mdl/backend/modelsdk/microflow_downloadfile_test.go b/mdl/backend/modelsdk/microflow_downloadfile_test.go index a5765560c..2c194614e 100644 --- a/mdl/backend/modelsdk/microflow_downloadfile_test.go +++ b/mdl/backend/modelsdk/microflow_downloadfile_test.go @@ -5,6 +5,7 @@ package modelsdkbackend import ( "testing" + "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/microflows" "go.mongodb.org/mongo-driver/v2/bson" ) @@ -35,3 +36,71 @@ func TestActionFromGen_DownloadFile(t *testing.T) { t.Errorf("ErrorHandlingType = %q, want Rollback default", df.ErrorHandlingType) } } + +// TestMicroflowRoundTrip_DownloadFile is the write-side counterpart of +// TestActionFromGen_DownloadFile, and the test that would have caught issue #850. +// +// microflowActionToGen had no *microflows.DownloadFileAction case, so the action +// fell through to `default: return nil` and the enclosing ActionActivity was +// written with no Action at all. `download file $Doc;` was accepted by the +// grammar, the visitor, the flow builder and the DESCRIBE formatter, and only +// vanished in the one stage that reports nothing — `mxcli exec` printed "Created +// microflow" and `mx check` then failed with CE0008 "No action defined." +// +// A reader-only test cannot catch this class: it starts from BSON the writer +// never had to produce. The round trip closes that gap. +func TestMicroflowRoundTrip_DownloadFile(t *testing.T) { + for _, showInBrowser := range []bool{true, false} { + name := "ShowInBrowser=false" + if showInBrowser { + name = "ShowInBrowser=true" + } + t.Run(name, func(t *testing.T) { + act := µflows.DownloadFileAction{ + FileDocument: "Doc", + ShowInBrowser: showInBrowser, + ErrorHandlingType: microflows.ErrorHandlingTypeRollback, + } + act.ID = model.ID("df-1") + activity := µflows.ActionActivity{Action: act} + activity.ID = model.ID("act-1") + + mf := µflows.Microflow{ + Name: "ACT_Download", + ObjectCollection: µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{activity}, + }, + } + mf.ID = model.ID("mf-1") + + got := roundTripMicroflow(t, mf) + + var found *microflows.DownloadFileAction + for _, obj := range got.ObjectCollection.Objects { + aa, ok := obj.(*microflows.ActionActivity) + if !ok { + continue + } + if aa.Action == nil { + t.Fatal("ActionActivity round-tripped with a nil Action — " + + "this is the CE0008 \"No action defined.\" shape from #850") + } + if df, ok := aa.Action.(*microflows.DownloadFileAction); ok { + found = df + } + } + if found == nil { + t.Fatal("no DownloadFileAction survived the round trip") + } + if found.FileDocument != "Doc" { + t.Errorf("FileDocument = %q, want Doc", found.FileDocument) + } + if found.ShowInBrowser != showInBrowser { + t.Errorf("ShowInBrowser = %v, want %v", found.ShowInBrowser, showInBrowser) + } + if found.ErrorHandlingType != microflows.ErrorHandlingTypeRollback { + t.Errorf("ErrorHandlingType = %q, want Rollback", found.ErrorHandlingType) + } + }) + } +} diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index 51878c559..05db0d046 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -543,6 +543,21 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { } addPartList(g, "ParameterMappings", mappings) return g + case *microflows.DownloadFileAction: + // DOWNLOAD FILE. Without this case the action fell through to + // `default: return nil` and the enclosing ActionActivity was written with + // no Action at all — `mxcli exec` reported "Created microflow" and only + // `mx check` noticed, as CE0008 "No action defined." (issue #850). + // + // The storage key is ShowFileInBrowser, not ShowInBrowser; the gen setter + // binds the right one (legacy's parseDownloadFileAction reads the wrong + // key — see TestActionFromGen_DownloadFile). + g := genMf.NewDownloadFileAction() + g.SetID(element.ID(a.ID)) + g.SetErrorHandlingType(orDefault(string(a.ErrorHandlingType), "Rollback")) + g.SetFileDocumentVariableName(a.FileDocument) + g.SetShowFileInBrowser(a.ShowInBrowser) + return g case *microflows.LogMessageAction: g := genMf.NewLogMessageAction() g.SetID(element.ID(a.ID)) diff --git a/mdl/backend/pagemutator/mutator.go b/mdl/backend/pagemutator/mutator.go index 8613c2456..a320725f6 100644 --- a/mdl/backend/pagemutator/mutator.go +++ b/mdl/backend/pagemutator/mutator.go @@ -2139,7 +2139,14 @@ func setWidgetConditionalSettingMut(widget bson.D, field, typeName, expression s doc := bson.D{ {Key: "$ID", Value: bsonutil.NewIDBsonBinary()}, {Key: "$Type", Value: typeName}, - {Key: "Attribute", Value: nil}, + // Attribute is a BY_NAME AttributeIdentifier: unset is the empty string, + // NOT null. A null fails the reader with StorageLoadException "…has an + // invalid value '' for property Attribute" and the project will not open, + // while `mx check` still passes. The CREATE path encodes it the same way + // via the Forms$Conditional{Visibility,Editability}Settings TypeDefaults + // (mdl/backend/modelsdk/widget_write.go, EmptyStringFields); this ALTER + // path builds the node by hand and has to match. Issue #851. + {Key: "Attribute", Value: ""}, {Key: "Conditions", Value: bson.A{int32(3)}}, {Key: "Expression", Value: expression}, } diff --git a/mdl/backend/pagemutator/mutator_test.go b/mdl/backend/pagemutator/mutator_test.go index 45da2b295..1e667b9c4 100644 --- a/mdl/backend/pagemutator/mutator_test.go +++ b/mdl/backend/pagemutator/mutator_test.go @@ -346,6 +346,80 @@ func TestSetWidgetProperty_VisibleExpression(t *testing.T) { }) } +// lookupBsonKey reports a key's value AND whether the key is present, which +// bsonnav.DGet cannot distinguish (an absent key and a stored null both read +// back as nil). Presence is the thing under test here: Mendix fills an omitted +// optional property on load, so "absent" and "null" fail differently. +func lookupBsonKey(doc bson.D, key string) (any, bool) { + for _, e := range doc { + if e.Key == key { + return e.Value, true + } + } + return nil, false +} + +// makeEditabilityWidget builds an input widget with null conditional +// visibility/editability slots (as Studio Pro writes them when unset). +func makeEditabilityWidget(name string) bson.D { + return bson.D{ + {Key: "$Type", Value: "Forms$TextBox"}, + {Key: "Name", Value: name}, + {Key: "ConditionalVisibilitySettings", Value: nil}, + {Key: "ConditionalEditabilitySettings", Value: nil}, + } +} + +// TestSetWidgetConditionalSetting_AttributeIsEmptyString locks in the fix for +// issue #851: `alter page … set Editable = [expr]` produced a project Studio Pro +// refused to load with StorageLoadException "Conditional editability settings has +// an invalid value ” for property Attribute". +// +// Attribute is a BY_NAME AttributeIdentifier, so the absent value is the empty +// string, not null — the CREATE path already encodes it that way via the +// Forms$Conditional{Visibility,Editability}Settings TypeDefaults +// (EmptyStringFields: Attribute, see mdl/backend/modelsdk/widget_write.go). The +// ALTER path built the node by hand and wrote nil, so the same widget authored +// through ALTER instead of CREATE was unloadable. +func TestSetWidgetConditionalSetting_AttributeIsEmptyString(t *testing.T) { + cases := []struct { + prop string // MDL property as the visitor delivers the [expr] form + field string // BSON slot it lands in + }{ + {"EditableIf", "ConditionalEditabilitySettings"}, + {"VisibleIf", "ConditionalVisibilitySettings"}, + } + for _, tc := range cases { + t.Run(tc.prop, func(t *testing.T) { + rawData := makeRawPage(makeEditabilityWidget("b1")) + m := &Mutator{rawData: rawData, widgetFinder: findBsonWidget} + expr := "$currentObject/Slug != ''" + if err := m.SetWidgetProperty("b1", tc.prop, expr); err != nil { + t.Fatalf("SetWidgetProperty(%s) failed: %v", tc.prop, err) + } + node := bsonnav.DGetDoc(findBsonWidget(rawData, "b1").widget, tc.field) + if node == nil { + t.Fatalf("expected a %s node", tc.field) + } + if got := bsonnav.DGetString(node, "Expression"); got != expr { + t.Errorf("Expression = %q, want %q", got, expr) + } + attr, ok := lookupBsonKey(node, "Attribute") + if !ok { + t.Fatal("Attribute key missing — Studio Pro requires the slot to be present") + } + if attr != any("") { + t.Errorf("Attribute = %#v, want %#v — a null here is not a valid "+ + "AttributeIdentifier and Studio Pro refuses to load the page", attr, "") + } + // SourceVariable is a BY_ID reference and stays null when unset. + if sv, ok := lookupBsonKey(node, "SourceVariable"); !ok || sv != nil { + t.Errorf("SourceVariable = %#v (present=%v), want nil", sv, ok) + } + }) + } +} + func TestSetWidgetProperty_ButtonStyle(t *testing.T) { w1 := bson.D{ {Key: "$Type", Value: "Pages$ActionButton"}, diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 09dcf6e81..088236a23 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -702,6 +702,54 @@ func violation18(locationPrefix string, w *ast.WidgetV3, msg string) linter.Viol // validateStaticWidget checks value-level constraints on built-in (non-pluggable) // widgets that the grammar can't express and that otherwise fail silently or at // build time rather than at `mxcli check` time. +// validateConsumableConditional (MDL-WIDGET19) rejects a `Visible:` / `Editable:` +// value that no builder can consume, so an expression the visitor failed to turn +// into VisibleIf/EditableIf fails the command instead of vanishing from the page. +// +// The bracket form is routed to VisibleIf/EditableIf by the visitor; the plain +// slot then holds only a static form, which pages.StaticVisibleExpression reads +// from a bool or a string. Anything else is parse residue — the `[...]` matched +// the generic property-value alternative rather than an xpathConstraint — and the +// builder's `else if` simply doesn't fire. That is the silent-drop mechanism +// behind issue #852, where `trim(…)`/`length(…)` were unparseable as conditional +// expressions and the whole property disappeared; a missing Visible defaults to +// "always visible", so nothing downstream could notice. +// +// The grammar fix removes the known trigger. This rule is the general guard: the +// next function name promoted to a lexer token fails loudly here instead. +func validateConsumableConditional(w *ast.WidgetV3, locationPrefix string) []linter.Violation { + var out []linter.Violation + for _, p := range []struct{ plain, routed string }{ + {"Visible", "VisibleIf"}, + {"Editable", "EditableIf"}, + } { + if _, routed := w.Properties[p.routed]; routed { + continue + } + v, present := w.Properties[p.plain] + if !present || v == nil { + continue + } + switch v.(type) { + case bool, string: + continue // a static form StaticVisibleExpression consumes + } + out = append(out, linter.Violation{ + RuleID: "MDL-WIDGET19", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: widget `%s` (%s) has a `%s` value that could not be parsed as a conditional expression "+ + "and would be dropped on write (leaving the widget unconditionally %s) — "+ + "check the expression inside `%s: [ ... ]`", + locationPrefix, w.Name, w.Type, strings.ToLower(p.plain), + map[string]string{"Visible": "visible", "Editable": "editable"}[p.plain], + strings.ToLower(p.plain), + ), + }) + } + return out +} + func validateStaticWidget(w *ast.WidgetV3, locationPrefix string) []linter.Violation { var out []linter.Violation @@ -742,6 +790,8 @@ func validateStaticWidget(w *ast.WidgetV3, locationPrefix string) []linter.Viola out = append(out, *v) } + out = append(out, validateConsumableConditional(w, locationPrefix)...) + // A DataView cannot use a database data source — a data view shows one object, // so Mendix offers only Context / Microflow / Nanoflow / Listen sources. // mxcli used to accept it: the modelsdk engine then errors "not yet supported — diff --git a/mdl/executor/validate_widgets_test.go b/mdl/executor/validate_widgets_test.go index de1e8c594..ce3f1e15d 100644 --- a/mdl/executor/validate_widgets_test.go +++ b/mdl/executor/validate_widgets_test.go @@ -154,6 +154,76 @@ func TestValidateStaticWidget_DataViewDatabaseSource(t *testing.T) { } } +// TestValidateStaticWidget_UnconsumableConditional — MDL-WIDGET19 is the safety +// net asked for in issue #852: the grammar fix stops `Visible: [trim(…)]` from +// falling through, but any FUTURE conditional expression the visitor cannot turn +// into VisibleIf/EditableIf would land in the plain Visible/Editable slot as a +// non-string, non-bool value, which the builder drops without a word. A dropped +// Visible reads as "always visible", so the failure is invisible until someone +// looks at the running app. +// +// Recognized static forms (bool, expression string) must NOT be flagged — they +// are consumed by pages.StaticVisibleExpression. +func TestValidateStaticWidget_UnconsumableConditional(t *testing.T) { + cases := []struct { + name string + widget *ast.WidgetV3 + want bool // expect an MDL-WIDGET19 violation + }{ + { + "unparsed bracket residue → flagged", + &ast.WidgetV3{Type: "dynamictext", Name: "a1", Properties: map[string]any{ + "Visible": []any{"trim($currentObject/Slug)", "!=", "''"}, + }}, + true, + }, + { + "unparsed Editable residue → flagged", + &ast.WidgetV3{Type: "textbox", Name: "b1", Properties: map[string]any{ + "Editable": []any{"length($currentObject/Slug)", ">", "0"}, + }}, + true, + }, + { + "routed to VisibleIf → not flagged", + &ast.WidgetV3{Type: "dynamictext", Name: "a2", Properties: map[string]any{ + "VisibleIf": "trim($currentObject/Slug) != ''", + }}, + false, + }, + { + "static bool → not flagged", + &ast.WidgetV3{Type: "dynamictext", Name: "a3", Properties: map[string]any{"Visible": false}}, + false, + }, + { + "expression string → not flagged", + &ast.WidgetV3{Type: "dynamictext", Name: "a4", Properties: map[string]any{ + "Visible": "$currentObject/Slug != ''", + }}, + false, + }, + { + "no visibility property → not flagged", + &ast.WidgetV3{Type: "dynamictext", Name: "a5", Properties: map[string]any{"Content": "x"}}, + false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := false + for _, v := range validateStaticWidget(c.widget, "page X") { + if v.RuleID == "MDL-WIDGET19" { + got = true + } + } + if got != c.want { + t.Errorf("MDL-WIDGET19 present = %v, want %v", got, c.want) + } + }) + } +} + // TestValidateWidgetExpressionAssociations — MDL-WIDGET13 flags an association // step inside an expression-typed widget property (DynamicClasses/VisibleIf/ // EditableIf). Such expressions fail the build with CE0117; a data binding on the diff --git a/mdl/grammar/domains/MDLPage.g4 b/mdl/grammar/domains/MDLPage.g4 index cc931826f..a8522f2b5 100644 --- a/mdl/grammar/domains/MDLPage.g4 +++ b/mdl/grammar/domains/MDLPage.g4 @@ -152,13 +152,51 @@ xpathFunctionCall : xpathFunctionName LPAREN (xpathExpr (COMMA xpathExpr)*)? RPAREN ; +/** Function name inside a bracketed [ … ] constraint. + * + * Any single word may name a function, exactly as any single word may be a name + * part (see xpathQualifiedName). The enumerated form this replaces listed only + * IDENTIFIER, HYPHENATED_ID, NOT, TRUE, FALSE and CONTAINS, so a call to a + * function whose name is also a lexer keyword — `trim(…)`, `length(…)` — never + * matched xpathFunctionCall. The enclosing `Visible: [...]` / `Editable: [...]` + * then failed to parse as an xpathConstraint and fell through to the generic + * property-value alternative, so the widget's whole conditional property was + * dropped without a diagnostic (a dropped Visible reads as "always visible" at + * runtime). Issue #852. + * + * The grammar deliberately does NOT enumerate a valid function set, because + * xpathConstraint serves TWO contexts with DIFFERENT ones: + * + * - `Visible:` / `Editable:` — a Mendix *client expression*, where the string + * functions apply: trim(), length(), toUpperCase(), find(), contains(). + * - a datasource `where` clause — real *XPath*, where the function set is + * contains/starts-with/ends-with/string-length/not/true/false and the + * *-from-dateTime family, `length()` means list length rather than character + * count, and the aggregates (count/avg/min/max/sum) are Java-API-only. + * `empty` and `NULL` are keywords here (`[Name = empty]`), never calls. + * See docs.mendix.com/refguide/xpath-constraint-functions/ and + * .../xpath-keywords-and-system-variables/. + * + * One rule cannot encode both sets, and guessing wrong rejects valid MDL. So the + * grammar accepts any name and lets mxbuild adjudicate semantics — it reports an + * unknown or wrong-context function as CE0117 against the real version's rules, + * which no table here could track. Verified on 11.6.6: in a widget conditional + * trim/length/find pass while count/empty give CE0117; in a `where` clause + * `[Name = empty]`, `[Name = NULL]`, not(), contains(), starts-with() and + * string-length() all pass. + * + * xpathWord is a negated token set, so it self-maintains as the lexer grows new + * keywords — an enumerated list would silently reacquire this bug with the next + * function name that gets promoted to a token. NOT is spelled out because + * xpathWord excludes it (it is an operator elsewhere in the expression grammar) + * while `not(…)` is a legitimate call. + * + * This cannot swallow a path: xpathFunctionCall requires an LPAREN after the + * name, and no xpathStepValue may be followed by one, so `empty` alone still + * parses as a word via xpathPath — which is what keeps `[Name = empty]` working. */ xpathFunctionName - : IDENTIFIER - | HYPHENATED_ID + : xpathWord | NOT - | TRUE - | FALSE - | CONTAINS ; // ============================================================================= diff --git a/mdl/visitor/visitor_conditional_visibility_test.go b/mdl/visitor/visitor_conditional_visibility_test.go index 1549292c5..8d35c3762 100644 --- a/mdl/visitor/visitor_conditional_visibility_test.go +++ b/mdl/visitor/visitor_conditional_visibility_test.go @@ -88,3 +88,63 @@ func TestConditionalVisibility_EnumLiteralPreserved(t *testing.T) { t.Errorf("VisibleIf = %q, want %q", got, want) } } + +// Issue #852 — a widget conditional expression that calls a function whose name +// is also an MDL lexer keyword (trim, length, …) silently dropped the whole +// property: `xpathFunctionName` only admitted IDENTIFIER/HYPHENATED_ID plus a +// handful of keywords, so `trim(…)` never matched xpathFunctionCall and the +// expression built to nothing. The property then vanished from the page, and a +// dropped Visible defaults to "always visible" — a wrong-behaviour failure that +// passed both `mxcli check` and `mx check`. +// +// Non-keyword function names (toUpperCase, isMatch) were never affected; they +// are plain IDENTIFIERs. They are covered here so a future narrowing of the rule +// cannot regress them silently. +// +// Scope: this asserts the PARSER builds the call and the property survives, not +// that Mendix accepts the function. The two are deliberately separate — MDL does +// not adjudicate Mendix expression semantics at the grammar layer. `count` and +// `empty` are included because they are keyword tokens (the thing under test) +// even though mxbuild rejects them in a client expression with CE0117: `empty` +// is a literal (`$x != empty`), not a call. Verified against mxbuild 11.6.6, +// where trim/length/find pass and count/empty do not. The shipped example +// mdl-examples/bug-tests/852-conditional-keyword-functions.mdl uses only the +// valid set so it stays `mx check`-clean. +func TestConditionalVisibility_KeywordFunctionNames(t *testing.T) { + cases := []struct { + name string + expr string // what goes inside Visible: [ ... ] + want string + }{ + {"trim", "trim($currentObject/Slug) != ''", "trim($currentObject/Slug) != ''"}, + {"length", "length($currentObject/Slug) > 0", "length($currentObject/Slug) > 0"}, + {"empty", "empty($currentObject/Slug)", "empty($currentObject/Slug)"}, + {"count", "count($currentObject/Items) > 0", "count($currentObject/Items) > 0"}, + {"find", "find($currentObject/Slug, 'x') >= 0", "find($currentObject/Slug, 'x') >= 0"}, + // Already worked — guard against regressing them. + {"contains", "contains($currentObject/Slug, 'x')", "contains($currentObject/Slug, 'x')"}, + {"toUpperCase", "toUpperCase($currentObject/Slug) != ''", "toUpperCase($currentObject/Slug) != ''"}, + // A bare attribute inside a keyword-named call still gets rooted. + {"trim roots bare attr", "trim(Slug) != ''", "trim($currentObject/Slug) != ''"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + input := "CREATE PAGE M.P (Title: 'P') { CONTAINER ctn (Visible: [" + c.expr + "]) { DYNAMICTEXT t (Content: 'x') } };" + prog, errs := Build(input) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + ctn := findWidgetV3(prog.Statements[0].(*ast.CreatePageStmtV3).Widgets, "ctn") + if ctn == nil { + t.Fatal("container ctn not found") + } + got, ok := ctn.Properties["VisibleIf"].(string) + if !ok { + t.Fatalf("VisibleIf missing entirely — the property was dropped (Properties: %v)", ctn.Properties) + } + if got != c.want { + t.Errorf("VisibleIf = %q, want %q", got, c.want) + } + }) + } +}