diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 692076fb2..b6e80b163 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -881,3 +881,4 @@ {"area": "mdl-executor", "date": "2026-10-07", "refs": ["mendixlabs/mxcli#1324"], "symptom": "A widget directly inside data view `dvP` that reads `$dvP` OUTSIDE an action — a nested data view's `DataSource: microflow M.F(Gate = $dvP)`, `Visible: $dvP/Name != ''`, `Editable: …`, `DynamicClasses: …`, or a nested list's `database from M.E where [Name = $dvP/Name]` — passes `check` and `exec`, then `mx check` reports `[CE0117] \"Error(s) in expression.\"` at the widget (CE0161 \"Error(s) in XPath constraint.\" for the `where`)", "cause": "MDL-BUTTON02 (the #1324 fix) only looked at action arguments, though every slot evaluated in the widget's enclosing context has the same scope: a container's widget-name variable exists only one data container below it", "file": "`mdl/executor/validate_page_button_context.go` (`checkOwnContainerName` now takes the widget and walks action args, `GetDataSource().Args` / `.Where`, and `ownNameExprProps`)", "insight": "**A scope rule belongs to the context, not to the slot the report happened to use.** The follow-up question that settled it was one probe per slot with a control one data view deeper: five of five slots failed in the own context and all five controls built clean, so the rule is 'anything evaluated in this widget's enclosing context' — enumerate those slots rather than wait for one report each. Two things that would have wasted a build: (1) `Visible:`/`Editable:` arrive in the AST as `VisibleIf`/`EditableIf` strings (dump `w.Properties` before keying on what the author wrote), and (2) text-template parameters (`ContentParams ({1} = $dvP/Name)`) are already refused by MDL-WIDGET24 for an unrelated reason (a template parameter is an attribute name, not a variable path), so they are not a scope case at all. The XPath slot reports a different code (CE0161), which is why the message carries the code per slot. Control: stubbing the data-source and property slots fails the new test with `flagged \"\"`; the `$currentObject` rewrite of all five builds at 0 errors. Repro `mdl-examples/bug-tests/1324-own-data-container-name-outside-actions.fail.mdl`"} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`describe structure depth 2|3` / `mxcli structure -d 3` never annotates a page with its data widgets (`Page M.P [DataView, …]`), on any project and with a full catalog — every page prints bare. Evora Factory Management: 0 of 54 pages annotated, although CATALOG.widgets holds their data views and grids.", "cause": "structurePages queried `widgets … where ParentWidget = ''`; widgets_data has never had a ParentWidget column (the tree position, added later, is ParentWidgetId + Depth). The query failed and the error was discarded (`if err == nil { … }`), so the annotation was dead code from the initial commit. Every other catalog query in cmd_structure.go swallowed errors the same way (`if err != nil || len(rows) == 0 { return }`).", "file": "`mdl/executor/cmd_structure.go` (`structureQuery`, `structurePages`, `queryCountByModule`, `shortWidgetType`), `mdl/executor/cmd_structure_page_widgets_test.go`", "insight": "A fixed SQL query against a table the builder owns can only fail through drift inside mxcli, never because of the user's project, so swallowing its error converts a schema mismatch into silent absence — the same shape as the depth-1 flow-count casing bug (#717), which a swallowed error also hid. Route such queries through one helper that returns the error; then a column rename fails the first test that runs the command against a real built catalog. That test must build the catalog with catalog.NewBuilder in FULL mode (SetFullMode(true)) over raw page BSON from GetRawUnitFunc — widgets_data is empty in fast mode, and a fast-mode test passes against the broken query because 'no widgets' and 'query failed' both print a bare page. Mock gotcha: the builder dereferences GetNavigation, whose mock default is (nil, nil), so stub it with an empty NavigationDocument. Semantics choice: 'top-level' cannot mean Depth = 0 — real pages wrap content in a layout grid, so a root filter lists almost nothing; list data widgets with no data-widget ancestor instead (walk ParentWidgetId). Measured on Evora after the fix: 45 of 54 pages annotated (181 of 212 with `all`), and the 9 bare pages have no data widget in CATALOG.widgets.", "refs": ["#717"]} {"area": "mdl/executor", "date": "2026-10-07", "symptom": "`mxcli check script.mdl -p app.mpr --references` printed \"Check passed!\" for `grant read * on entity System.Nope to M.R`, for a grant to `M.NopeRole`, and for every other GRANT/REVOKE form naming a missing entity, document, member or role; also for `create user role R ( ModuleRoles: (M.NopeRole) )`, `alter user role … add module roles`, and a demo user with an unknown user role or entity (measured on Evora Factory Management, 10.24.15). exec refused the grants after the earlier statements were written; the user-role and demo-user shapes it wrote unresolved (CE1613 at build).", "cause": "No validate path resolved a security statement's names. validateWithContext's switch had no case for any of them (only the role-free validateCrossModuleGrant ran), and the user-role/demo-user executors store module and user role names verbatim without resolving them, so neither check nor exec stood between a typo and the model.", "file": "`mdl/executor/validate_grant_refs.go` (`validateGrantReferences`), wired into `validateProgramWithWarnings` in `validate.go`; test `check_grant_references_pedapp_test.go`", "insight": "Two things made the obvious version wrong. (1) What check refuses has to be what exec refuses, form by form: exec refuses an unknown role on a GRANT and on an entity REVOKE, but a document REVOKE (and `alter user role … drop module roles`) of an unknown role is a reported no-op, which keeps cleanup scripts re-runnable after the role is dropped — refusing it would make check louder than exec. Likewise every System entity is refused by exec (refuseSystemEntityGrant), so `grant … on entity System.User` must be refused by check too, while System MODULE roles in a user role must pass. (2) A script's own effects are not only its CREATE statements: creating a microflow/nanoflow/page in a module with no module roles auto-creates `.User` (defaultDocumentAccessRoles), and doctype-tests/02b grants to exactly that role — the first version flagged 10 statements there. Walking the program in statement order (not the whole-program scriptContext) gives the forward-reference hint for free. Performance trap: reading every module's security costs ~5s on Evora; read only the modules a statement names (~0.1s).", "refs": ["mdl-examples/bug-tests/check-grant-unknown-reference-refused.mdl", "mdl-examples/bug-tests/check-grant-unknown-reference.mdl", "ako/mxcli#1020"]} +{"area": "mdl/executor", "date": "2026-10-07", "symptom": "A DataGrid 2 column's `Visible:` expression is stored as `true` (always visible) for every spelling but the old quoted one: `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)`, `Visible: [cond]`. check clean, exec reports \"Created page\", describe shows no Visible. `alter page … set (Visible: ) on grid column(…)` is refused as \"column property VisibleIf not found\"; `insert` of such a column drops it, and drops `Visible: false` too.", "cause": "The visitor lowers every expression spelling of Visible to the key VisibleIf (the page-widget conditional-visibility key) and keeps only plain values under Visible. The column writers read only visible/Visible: the widget engine by schema key + types.ItemPropertyAliases (no VisibleIf alias), the ALTER mutator's resolveColumnPropertyKey the same table, and widgetobj.BuildDataGrid2Column (ALTER insert/replace via buildColumnSpecFromAST) the exact key \"Visible\" as a string only, so a bool false fell to the default too. The mirror image of widget-visible-expression.mdl, where page widgets read VisibleIf and dropped Visible.", "file": "`mdl/types/widget_item_aliases.go` (`visible` ← `VisibleIf`), `mdl/executor/widget_engine.go` (WidgetDefGeneratorVersion 18), `mdl/executor/cmd_pages_builder_v3_widgets.go` (`columnSpecProperties`), `mdl/executor/cmd_pages_describe_output.go` (column Visible via widgetConditionMDL), `mdl/executor/validate_column_visible_scope.go` (MDL-WIDGET43)", "insight": "When the visitor lowers one MDL property to two AST keys by value shape, every consumer must read both — grep the consumers of the key the visitor writes, not of the property name. Making the value persist exposed what the drop had hidden: a column's visible expression has no row object (no dataSource in the widget schema, unlike columnClass), so every $currentObject example became CE0117 at build — measured on 11.14.0 — and needed a check rule (MDL-WIDGET43) in the same change. The widget def's per-item `dataSource` field is the signal for whether $currentObject exists.", "refs": ["mdl-examples/bug-tests/datagrid-column-visible-expression.mdl", "mdl-examples/bug-tests/widget-visible-expression.mdl"]} diff --git a/CHANGELOG.md b/CHANGELOG.md index cd3b512da..807a576d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed - **The catalog names a call's and a delete's target** (mendixlabs/mxcli#1305) — `activities_for()` and `CATALOG.ACTIVITIES` returned every microflow, nanoflow, Java action and JavaScript action call with `action_ref=""`, and every delete with `entity_ref=""`, although `refs_from()` had both targets; a loop-scoped lint rule could not follow a call out of the loop. `action_ref` / `ActionRef` is now the called document, `entity_ref` / `EntityRef` a delete's entity (when the flow types the variable: a parameter, a create or retrieve output, a loop iterator), and the new `queue_ref` / `QueueRef` the task queue a microflow or Java action call runs in, so a rule can skip a call that runs asynchronously. A nanoflow's JavaScript action call now has a `refs_from()` `call` row (`target_type` `"JAVASCRIPT_ACTION"`), so `show callers` sees it. The catalog schema is bumped to 23; a cached catalog rebuilds. +- **A DataGrid 2 column's `Visible:` expression is written** — `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)` and `Visible: [cond]` on a column passed `check` and `exec` and were stored as `true`, so the column was always visible; only the old quoted `Visible: ''` was kept. `alter page … set (Visible: ) on grid column(…)` was refused as "column property VisibleIf not found", and an inserted column dropped `Visible: false` too. `describe` now prints the bare expression. A column's visibility is evaluated once for the grid, with no row object, so `$currentObject` there is CE0117 at build; `check` refuses it as **MDL-WIDGET43** (measured on 11.14.0). Use a page variable or parameter. Projects regenerate their widget definitions (generator version 18). - **`set $Param = …` on a parameter is refused** — a Change variable cannot target a parameter, and mxbuild rejects it with CE7247 "Parameter 'N' cannot be changed." (measured on 11.14.0 for Integer and String parameters in a microflow, a nanoflow and a rule). `check` and `exec` passed it. MDL-SET01 now refuses it for every parameter but a list (`set` on a list parameter is a Change list Replace, which builds), and `exec` enforces the rule. Copy the parameter into a variable first: `declare $Value Integer = $N;`. - **`check` refuses a button that passes its own data container by widget name** (mendixlabs/mxcli#1324) — `actionbutton btnOwn (Action: call microflow M.F(Gate = $dvGate))` directly inside data view `dvGate` passed `check --references` and `exec`, then `mx check` reported `[CE0117] "Error(s) in expression." at Action button 'btnOwn'`. A data container's name is a variable only for the containers nested below it; in its own context the object is `$currentObject`. Reported as **MDL-BUTTON02** (error, so `exec` refuses it), for data views, list views, galleries and data grids alike, including attribute paths (`$dvGate/Name`) and a control bar inside the container. The rule covers every slot evaluated in that context, not only action arguments: a nested widget's microflow data-source arguments, `Visible:`, `Editable:` and `DynamicClasses:` (CE0117), and a nested list's XPath `where` (CE0161). A grid's own name from its control bar (the selection) and an enclosing container's name from a nested one are not flagged. The flagged set matches mxbuild 11.14.0's CE0117s widget for widget on a ten-button probe page. - **`set $Obj = …` on an object variable is refused** (mendixlabs/mxcli#1323) — with both variables single objects (e.g. a Reference retrieved from its FROM entity), `set $Cursor = $Next;` passed `check --references` and `exec` and was written as a Change variable action, which mxbuild refuses with CE7247 "Variable 'Cursor' does not have a primitive type". Mendix has no action that reassigns an object variable, so `check` (MDL-SET01, for the objects it can see without a project), `check --references` and `exec` now refuse it and name the alternatives (a sub-microflow that returns the next object, `change $Obj (…)`). `check --references` now types a Reference retrieve from its FROM entity as the object it is. `set` on a list variable stays a Change list Replace (ako/mxcli#949). diff --git a/cmd/mxcli/syntax/features_page.go b/cmd/mxcli/syntax/features_page.go index 0e4a53514..b1d60a6be 100644 --- a/cmd/mxcli/syntax/features_page.go +++ b/cmd/mxcli/syntax/features_page.go @@ -402,7 +402,7 @@ LIST IMPACT OF htmlelement; "column width", "alignment", "wrap text", "visible", "dynamic cell class", "tooltip", "associated attribute", "association column", }, - Syntax: "COLUMN name (\n Attribute: AttrName, -- own attribute\n -- or an attribute over an association (bare association name):\n -- Attribute: Assoc/Attr e.g. Order_Customer/Name\n Caption: 'Header'\n [, Sortable: true|false]\n [, Resizable: true|false]\n [, Draggable: true|false]\n [, Hidable: yes|hidden|no]\n [, ColumnWidth: autoFill|autoFit|manual]\n [, Size: integer]\n [, Alignment: left|center|right]\n [, WrapText: true|false]\n [, Visible: 'expression']\n [, DynamicCellClass: 'expression']\n [, Tooltip: 'text']\n)", + Syntax: "COLUMN name (\n Attribute: AttrName, -- own attribute\n -- or an attribute over an association (bare association name):\n -- Attribute: Assoc/Attr e.g. Order_Customer/Name\n Caption: 'Header'\n [, Sortable: true|false]\n [, Resizable: true|false]\n [, Draggable: true|false]\n [, Hidable: yes|hidden|no]\n [, ColumnWidth: autoFill|autoFit|manual]\n [, Size: integer]\n [, Alignment: left|center|right]\n [, WrapText: true|false]\n [, Visible: ] -- once for the grid: a page variable or parameter, never $currentObject (MDL-WIDGET43)\n [, DynamicCellClass: ] -- per row: $currentObject is the row\n [, Tooltip: 'text']\n)", Example: "COLUMN colPrice (\n Attribute: Price, Caption: 'Price',\n Alignment: right, Sortable: false,\n ColumnWidth: manual, Size: 150,\n Tooltip: 'Price in USD'\n)\n\n-- Associated attribute (attribute over a reference association):\nCOLUMN colCustomer (Attribute: Order_Customer/Name, Caption: 'Customer')", SeeAlso: []string{"page.widgets"}, }) diff --git a/docs-site/src/appendixes/quick-reference.md b/docs-site/src/appendixes/quick-reference.md index 8a5258f64..443a7d325 100644 --- a/docs-site/src/appendixes/quick-reference.md +++ b/docs-site/src/appendixes/quick-reference.md @@ -385,8 +385,8 @@ MDL uses explicit property declarations for pages: | `Hidable` | `yes`, `hidden`, `no` | `yes` | `Hidable: no` | | `ColumnWidth` | `autoFill`, `autoFit`, `manual` | `autoFill` | `ColumnWidth: manual` | | `Size` | integer (px) | `1` | `Size: 200` | -| `Visible` | expression string | `true` | `Visible: '$showColumn'` (page variable, not $currentObject) | -| `DynamicCellClass` | expression string | (empty) | `DynamicCellClass: if(...) then ... else ...` | +| `Visible` | expression | `true` | `Visible: $showColumn` — evaluated once for the grid: a page variable or parameter, never `$currentObject` (MDL-WIDGET43, CE0117) | +| `DynamicCellClass` | expression | (empty) | `DynamicCellClass: if $currentObject/Stock < 10 then 'text-danger' else ''` | | `Tooltip` | text string | (empty) | `Tooltip: 'Price in USD'` | **Page Example:** diff --git a/docs-wiki/bug-patterns/silent-property-drop.md b/docs-wiki/bug-patterns/silent-property-drop.md index 7ce6c6300..5c0caa167 100644 --- a/docs-wiki/bug-patterns/silent-property-drop.md +++ b/docs-wiki/bug-patterns/silent-property-drop.md @@ -125,6 +125,16 @@ answer "this shape does not fit" (MDL-WIDGET42, and an error at build) instead o falling to `default: continue`. When a grammar alternative is chosen by the *first token*, audit which other meanings that token starts. +**One property, two keys: a consumer that reads one drops the other.** The +visitor lowers `Visible:` by value shape — an expression to `VisibleIf`, a plain +value to `Visible` — and the two writers each read only one: page widgets read +`VisibleIf` (and once dropped `Visible: false`), DataGrid 2 columns read +`Visible` (and dropped every expression). Grep the consumers of the *key the +visitor writes*, not of the property name. And expect persisting a value to +surface a rule its absence hid: a column's visibility has no row object, so the +`$currentObject` examples that had always "worked" became CE0117 the moment they +were written, and the fix needed a check rule to go with it. + **Children drop the same way properties do.** A widget's body is distributed by several passes that each skip what they do not recognise, so a child matching no container, no slot and no catch-all is built and discarded exactly as an diff --git a/mdl-examples/bug-tests/datagrid-column-visible-expression.mdl b/mdl-examples/bug-tests/datagrid-column-visible-expression.mdl new file mode 100644 index 000000000..dabf9414d --- /dev/null +++ b/mdl-examples/bug-tests/datagrid-column-visible-expression.mdl @@ -0,0 +1,54 @@ +mdl 1; +-- ============================================================================ +-- A DataGrid 2 column's Visible expression was silently dropped +-- ============================================================================ +-- +-- Symptom (before fix): +-- `Visible: $showPrices`, `Visible: if … then … else …`, `visible: not(…)` and +-- the bracketed `Visible: [cond]` on a column passed check, exec reported +-- "Created page", and the column was stored always visible (Expression +-- "true"). Only the old quoted spelling `Visible: ''` was kept. +-- `alter page … set (Visible: ) on grid column(…)` was refused as +-- "column property VisibleIf not found". +-- +-- Root cause: +-- The mirror image of widget-visible-expression.mdl. The visitor lowers +-- every expression spelling of Visible to the key VisibleIf, the key a page +-- widget's conditional visibility reads. The column writers read only +-- `visible` / `Visible`: the widget engine by schema key plus +-- types.ItemPropertyAliases, ALTER's insert path by the exact key. +-- +-- After fix: +-- `visible` <- `VisibleIf` alias (create and alter set; widget defs +-- regenerate at generator version 18), the insert/replace path normalises +-- the column's properties, and describe prints the bare expression. +-- A column's visibility is evaluated once for the grid: it has no row +-- object, so `$currentObject` there is CE0117 — refused at check time as +-- MDL-WIDGET43. Use a page variable or parameter. +-- Verified on Mendix 11.14.0: mx check = 0 errors; describe round-trips. +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/datagrid-column-visible-expression.mdl -p app.mpr +-- ============================================================================ + +create entity MyFirstModule.ColVisItem ( Name: String(100), Price: Decimal ); + +create or modify page MyFirstModule.P_ColumnVisible +( Title: 'Column visible', Layout: Atlas_Core.Atlas_Default, + Variables: ( $showPrices: boolean = 'true' ) ) +{ + datagrid dg (datasource: database MyFirstModule.ColVisItem) { + column (attribute: Name, caption: 'Var', Visible: $showPrices) + column (attribute: Price, caption: 'If', Visible: if $showPrices then true else false) + column (attribute: Price, caption: 'Not', visible: not($showPrices)) + column (attribute: Name, caption: 'Hidden', Visible: false) + column (attribute: Name, caption: 'Later') + } +}; + +-- Previously refused: "column property VisibleIf not found". +alter page MyFirstModule.P_ColumnVisible { + set (Visible: $showPrices) on dg column('Later') +}; + +describe page MyFirstModule.P_ColumnVisible; diff --git a/mdl-examples/doctype-tests/03-page-examples.mdl b/mdl-examples/doctype-tests/03-page-examples.mdl index e4d69081f..c67244f19 100644 --- a/mdl-examples/doctype-tests/03-page-examples.mdl +++ b/mdl-examples/doctype-tests/03-page-examples.mdl @@ -2249,7 +2249,7 @@ create page PgTest.P033b_DataGrid_ColumnProperties folder 'DataGrid' column ( attribute: Stock, caption: 'In Stock', Alignment: center, - visible: '$showStockColumn', + visible: $showStockColumn, DynamicCellClass: if($currentObject/Stock < 10) then 'text-danger' else '' ) diff --git a/mdl/backend/pagemutator/column_property_test.go b/mdl/backend/pagemutator/column_property_test.go index be03ba923..f8925eee5 100644 --- a/mdl/backend/pagemutator/column_property_test.go +++ b/mdl/backend/pagemutator/column_property_test.go @@ -158,3 +158,16 @@ func TestSetColumnPropertyErrorListsWhatIsSettable(t *testing.T) { } } } + +// `alter page … set (Visible: ) on grid column(…)` arrives as +// VisibleIf — the key the visitor lowers every expression spelling of Visible +// to — and was refused as "column property VisibleIf not found" while the plain +// `Visible: false` was accepted. The shared alias table resolves it, as on create. +func TestSetColumnPropertyResolvesVisibleIf(t *testing.T) { + keys := map[string]string{"44444444-4444-4444-4444-444444444444": "visible"} + for _, name := range []string{"VisibleIf", "Visible"} { + if got := resolveColumnPropertyKey(name, keys); got != "visible" { + t.Errorf("resolveColumnPropertyKey(%q) = %q, want visible", name, got) + } + } +} diff --git a/mdl/executor/cmd_pages_builder_v3_widgets.go b/mdl/executor/cmd_pages_builder_v3_widgets.go index 71091bfd9..45ff64a67 100644 --- a/mdl/executor/cmd_pages_builder_v3_widgets.go +++ b/mdl/executor/cmd_pages_builder_v3_widgets.go @@ -220,7 +220,7 @@ func (pb *pageBuilder) buildColumnSpecFromAST(child *ast.WidgetV3) (*backend.Dat ShowContentAs: child.GetStringProp("ShowContentAs"), Content: child.GetContent(), ContentParams: pb.buildClientTemplateParams(child.GetContentParams()), - Properties: child.Properties, + Properties: columnSpecProperties(child), } for _, grandchild := range child.Children { if filterWidgetID := dataGridFilterWidgetID(grandchild.Type); filterWidgetID != "" { @@ -246,6 +246,38 @@ func (pb *pageBuilder) buildColumnSpecFromAST(child *ast.WidgetV3) (*backend.Dat return &col, nil } +// columnSpecProperties puts a column's properties in the terms the column +// builder (widgetobj.BuildDataGrid2Column) reads: each expression property as a +// string under its canonical key. The visitor routes every expression spelling +// of Visible — `[cond]`, `if … then … else …`, `$currentObject/Flag` — to +// "VisibleIf", `Visible: false` arrives as a bool, and a named expression +// property keeps the key as written; the builder reads only a string under +// "Visible" / "DynamicCellClass", so each of these was written as the default +// (always visible, no class) with check clean and exec reporting success. +// The AST map is copied, never changed. +func columnSpecProperties(child *ast.WidgetV3) map[string]any { + if len(child.Properties) == 0 { + return child.Properties + } + props := make(map[string]any, len(child.Properties)) + for k, v := range child.Properties { + switch { + case strings.EqualFold(k, "VisibleIf"), strings.EqualFold(k, "Visible"): + // resolved below + case strings.EqualFold(k, "DynamicCellClass"): + props["DynamicCellClass"] = v + default: + props[k] = v + } + } + if expr := child.GetStringProp("VisibleIf"); expr != "" { + props["Visible"] = expr + } else if expr, ok := pages.StaticVisibleExpression(child.Properties["Visible"]); ok { + props["Visible"] = expr + } + return props +} + func (pb *pageBuilder) buildDataGridColumnV3(w *ast.WidgetV3) (*pages.DataGridColumn, error) { col := &pages.DataGridColumn{ BaseElement: model.BaseElement{ diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index ce94b0c37..908b2f030 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -1183,7 +1183,15 @@ func outputDataGrid2ColumnV3(ctx *ExecContext, prefix string, col rawDataGridCol props = append(props, fmt.Sprintf("Size: %s", col.Size)) } if col.Visible != "" && col.Visible != "true" { - props = append(props, fmt.Sprintf("Visible: %s", mdlQuote(ctx, col.Visible))) + // The expression as written, as for a page widget's conditional + // visibility; `false` is the plain value, which a column reads as the + // expression false. The quoted text this printed before was the + // one spelling a column's Visible did not drop. + if strings.EqualFold(strings.TrimSpace(col.Visible), "false") { + props = append(props, "Visible: false") + } else { + props = append(props, widgetConditionMDL("Visible", col.Visible)) + } } if col.DynamicCellClass != "" { props = append(props, fmt.Sprintf("DynamicCellClass: %s", col.DynamicCellClass)) // an expression, printed as-is diff --git a/mdl/executor/datagrid_column_visible_test.go b/mdl/executor/datagrid_column_visible_test.go new file mode 100644 index 000000000..0c6f85c00 --- /dev/null +++ b/mdl/executor/datagrid_column_visible_test.go @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "strings" + "testing" +) + +// A DataGrid 2 column's Visible is an expression property, read by the column +// builder (widgetobj.BuildDataGrid2Column) from the key "Visible" as a string. +// The visitor routes every expression spelling — `Visible: [cond]`, `Visible: +// if … then … else …`, `Visible: $currentObject/Flag` — to "VisibleIf", and +// `Visible: false` arrives as a bool, so each one reached the builder as "no +// value" and the column was written always visible: check clean, exec +// reporting "Created page". columnSpecProperties is where the column spec's +// properties are put in the builder's terms. +func TestDataGridColumnVisible_EverySpellingReachesTheBuilder(t *testing.T) { + cases := []struct { + name, prop string + want any // nil: the builder's default (always visible) + }{ + {"bracket condition", `Visible: [Price > 100]`, "$currentObject/Price > 100"}, + {"if expression", `Visible: if $currentObject/Price > 100 then true else false`, "if $currentObject/Price > 100 then true else false"}, + {"attribute path", `Visible: $currentObject/Featured`, "$currentObject/Featured"}, + {"lowercase key", `visible: $currentObject/Featured`, "$currentObject/Featured"}, + {"static false", `Visible: false`, "false"}, + {"control: quoted expression (mdl 0)", `Visible: 'if $currentObject/Price > 100 then true else false'`, "if $currentObject/Price > 100 then true else false"}, + {"control: static true", `Visible: true`, nil}, + {"control: absent", `Sortable: true`, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ws := pageWidgets(t, `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + datagrid dg (datasource: database M.Thing) { + column (attribute: Name, caption: 'N', `+tc.prop+`) + } +}`) + props := columnSpecProperties(ws["dg"].Children[0]) + got, present := props["Visible"] + if tc.want == nil { + if present { + t.Fatalf("Visible = %#v, want it absent (always visible)", got) + } + return + } + if got != tc.want { + t.Fatalf("Visible = %#v, want %q — the column would be written always visible", got, tc.want) + } + }) + } +} + +// The builder reads DynamicCellClass by its canonical key; the visitor keeps a +// named expression property under the key as written. +func TestDataGridColumnDynamicCellClass_AnyCase(t *testing.T) { + ws := pageWidgets(t, `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + datagrid dg (datasource: database M.Thing) { + column (attribute: Name, caption: 'N', dynamiccellclass: if $currentObject/Price > 100 then 'hi' else '') + } +}`) + props := columnSpecProperties(ws["dg"].Children[0]) + if got := props["DynamicCellClass"]; got != "if $currentObject/Price > 100 then 'hi' else ''" { + t.Fatalf("DynamicCellClass = %#v, want the expression", got) + } +} + +// describe prints a column's Visible in the form a page widget's is printed, +// and re-executing that output stores the same expression. +func TestDescribeDataGridColumnVisible_RoundTrip(t *testing.T) { + for _, expr := range []string{ + "$currentObject/Price > 100", + "if $currentObject/Price > 100 then true else false", + "$currentObject/Featured", + "false", + } { + var out bytes.Buffer + outputDataGrid2ColumnV3(&ExecContext{Output: &out}, "", rawDataGridColumn{Attribute: "Name", Caption: "N", Visible: expr}) + line := strings.TrimSpace(out.String()) + if strings.Contains(line, "Visible: '") { + t.Errorf("describe quoted the expression: %s", line) + } + ws := pageWidgets(t, `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + datagrid dg (datasource: database M.Thing) { + `+line+` + } +}`) + if got := columnSpecProperties(ws["dg"].Children[0])["Visible"]; got != expr { + t.Errorf("re-exec of %q stores Visible %#v, want %q", line, got, expr) + } + } +} + +// MDL-WIDGET43: measured on Mendix 11.14.0, every column whose Visible names +// $currentObject is CE0117 in mx check, and `Visible: false` is clean. +func TestMDLWIDGET43_ColumnVisibleHasNoRowObject(t *testing.T) { + cases := []struct { + prop string + want int + }{ + {`Visible: [Price > 100]`, 1}, + {`Visible: $currentObject/Featured`, 1}, + {`Visible: if $currentObject/Price > 100 then true else false`, 1}, + {`Visible: 'if $currentObject/Price > 100 then true else false'`, 1}, + {`Visible: false`, 0}, + {`Visible: $ShowPrices`, 0}, + {`DynamicCellClass: if $currentObject/Featured then 'hi' else ''`, 0}, + } + for _, tc := range cases { + src := `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + datagrid dg (datasource: database M.Thing) { + column (attribute: Name, caption: 'N', ` + tc.prop + `) + } +};` + if got := widgetViolations(t, src, "MDL-WIDGET43"); len(got) != tc.want { + t.Errorf("%s: MDL-WIDGET43 = %d, want %d: %v", tc.prop, len(got), tc.want, got) + } + } + // A page widget's conditional visibility has the widget's row object. + if got := widgetViolations(t, `create page M.P (title: 'P', layout: Atlas_Core.Atlas_Default) { + dataview dv (datasource: $Thing) { textbox t (attribute: Name, Visible: $currentObject/Featured) } +};`, "MDL-WIDGET43"); len(got) != 0 { + t.Errorf("control: a textbox's Visible was refused: %v", got) + } +} + +func TestMDLWIDGET43_AlterSetAndInsert(t *testing.T) { + for _, tc := range []struct { + src string + want int + }{ + {`alter page M.P { set (Visible: $currentObject/Featured) on dg column('N') };`, 1}, + {`alter page M.P { set (Visible: [Price > 1]) on dg column(Name) };`, 1}, + {`alter page M.P { insert after dg column('N') { column (attribute: Name, caption: 'X', Visible: $currentObject/F) } };`, 1}, + {`alter page M.P { set (Visible: false) on dg column('N') };`, 0}, + {`alter page M.P { set (Visible: $currentObject/Featured) on txt1 };`, 0}, + } { + var got int + for _, v := range ValidateWidgetProperties(parseMDL(t, tc.src), "") { + if v.RuleID == "MDL-WIDGET43" { + got++ + } + } + if got != tc.want { + t.Errorf("%s: MDL-WIDGET43 = %d, want %d", tc.src, got, tc.want) + } + } +} diff --git a/mdl/executor/validate_column_visible_scope.go b/mdl/executor/validate_column_visible_scope.go new file mode 100644 index 000000000..80831d94b --- /dev/null +++ b/mdl/executor/validate_column_visible_scope.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "regexp" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +var currentObjectRe = regexp.MustCompile(`(?i)\$currentObject\b`) + +// validateColumnVisibleScope (MDL-WIDGET43) refuses `$currentObject` in a +// DataGrid 2 column's Visible: +// +// column (attribute: Name, Visible: $currentObject/Featured) -- CE0117 +// +// A column's `visible` expression is evaluated once for the grid, not per row: +// the widget declares it with no dataSource, unlike `columnClass`, so there is +// no row object and mxbuild reports CE0117 "Error(s) in expression". The +// bracketed form `Visible: [Attr > 1]` roots a bare attribute in $currentObject +// and lands here too. Until the column builder read the expression it was +// dropped, so these always checked and executed clean; an error now, so exec +// refuses rather than writing a page that fails the build. +func validateColumnVisibleScope(w *ast.WidgetV3, locationPrefix string) []linter.Violation { + if w == nil || !strings.EqualFold(w.Type, "column") { + return nil + } + return columnVisibleScopeViolation(fmt.Sprintf("%s: datagrid column `%s`", locationPrefix, columnLabel(w)), w.Properties) +} + +// validateAlterSetColumnVisibleScope is MDL-WIDGET43 for +// `alter page … set (Visible: …) on grid column(…)`. +func validateAlterSetColumnVisibleScope(op *ast.SetPropertyOp, locationPrefix string) []linter.Violation { + if !op.Target.IsColumnAddress() && op.Target.Column == "" { + return nil + } + return columnVisibleScopeViolation(fmt.Sprintf("%s: set on `%s`", locationPrefix, op.Target.Name()), op.Properties) +} + +func columnVisibleScopeViolation(where string, props map[string]any) []linter.Violation { + var expr string + for k, v := range props { + if s, ok := v.(string); ok && (strings.EqualFold(k, "VisibleIf") || strings.EqualFold(k, "Visible")) && s != "" { + expr = s + } + } + if !currentObjectRe.MatchString(expr) { + return nil + } + return []linter.Violation{{ + RuleID: "MDL-WIDGET43", + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s Visible uses $currentObject — a column's visibility is evaluated once for the grid, "+ + "with no row object, so Mendix rejects it (CE0117): %s", + where, expr), + Suggestion: "use a page parameter or variable (`Visible: $ShowPrices`), or hide per row with " + + "DynamicCellClass or the cell content instead", + }} +} + +func columnLabel(w *ast.WidgetV3) string { + if c := w.GetCaption(); c != "" { + return c + } + if a := w.GetAttribute(); a != "" { + return a + } + return w.Name +} diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 7a70d4dd7..b1042ba1b 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -159,6 +159,7 @@ func validateWidgetPropertiesOf(stmt ast.Statement, registry *WidgetRegistry) [] out = append(out, validateWidgetSubtree(o.NewWidgets, registry, "alter "+s.PageName.String())...) case *ast.SetPropertyOp: out = append(out, validateAlterSetLegacyExpressionText(o, "alter "+s.PageName.String())...) + out = append(out, validateAlterSetColumnVisibleScope(o, "alter "+s.PageName.String())...) } } return out @@ -232,6 +233,8 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc // …and the OLD spelling, a quoted string holding the expression's text, // which now stores that text as a class name (MDL-WIDGET33). out = append(out, validateLegacyExpressionText(w, locationPrefix)...) + // A column's Visible has no row object (MDL-WIDGET43, CE0117). + out = append(out, validateColumnVisibleScope(w, locationPrefix)...) // #1062: an action slot holding something that is not an action, which // used to check clean, exec clean, build clean and render dead. Runs for // every widget kind and needs no definition, for the same reason as the diff --git a/mdl/executor/widget_defs_test.go b/mdl/executor/widget_defs_test.go index ec28df60a..d055b918b 100644 --- a/mdl/executor/widget_defs_test.go +++ b/mdl/executor/widget_defs_test.go @@ -580,6 +580,7 @@ func TestObjectListItemAliases(t *testing.T) { {Key: "attribute", Type: "attribute"}, {Key: "width", Type: "enumeration"}, {Key: "columnClass", Type: "expression"}, + {Key: "visible", Type: "expression"}, }, }, }, @@ -614,6 +615,12 @@ func TestObjectListItemAliases(t *testing.T) { if got := aliases["columnClass"]; len(got) != 1 || got[0] != "DynamicCellClass" { t.Errorf("columnClass MdlAliases = %v, want [DynamicCellClass]", got) } + // visible is filled by the visitor's VisibleIf — every expression spelling + // of `Visible:` (`if … then … else …`, `$currentObject/Flag`, `[cond]`) is + // lowered there; without the alias a column's visibility was dropped. + if got := aliases["visible"]; len(got) != 1 || got[0] != "VisibleIf" { + t.Errorf("visible MdlAliases = %v, want [VisibleIf]", got) + } // tooltip and attribute have no aliases — schema name is the MDL keyword. if got := aliases["tooltip"]; len(got) != 0 { t.Errorf("tooltip MdlAliases = %v, want []", got) diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index 088cb188c..f1e152813 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -92,7 +92,11 @@ const defaultSlotContainer = "template" // to a value the widget hides, which mxbuild rejects with CE0463 and which // makes `mx create-module-package` refuse the module (upstream #931). Bump // forces existing projects to regenerate their defs with both. -const WidgetDefGeneratorVersion = 17 +// 18 — DataGrid column `visible` ← `VisibleIf` alias. Every expression +// spelling of a column's `Visible:` is lowered to VisibleIf, so without it +// the column was written always visible. Bump forces existing projects to +// regenerate so the alias reaches their datagrid def. +const WidgetDefGeneratorVersion = 18 // WidgetDefinition describes how to construct a pluggable widget from MDL syntax. // Loaded from embedded JSON definition files (*.def.json). diff --git a/mdl/types/widget_item_aliases.go b/mdl/types/widget_item_aliases.go index 06ecfd3b0..998458702 100644 --- a/mdl/types/widget_item_aliases.go +++ b/mdl/types/widget_item_aliases.go @@ -37,6 +37,16 @@ var ItemPropertyAliases = map[string]map[string]map[string][]string{ // nothing, and writes an empty expression — the class is silently // dropped. Bug 10a. "columnClass": {"DynamicCellClass"}, + // The visitor lowers every expression spelling of `Visible:` — + // `Visible: if … then … else …`, `Visible: $currentObject/Flag`, the + // deprecated `Visible: [cond]` — to the key `VisibleIf`, the form a + // page widget's conditional visibility reads. Only the plain + // `Visible: false` / `Visible: ''` stays under `Visible`. Without + // the alias the engine found no `visible` and wrote the default "true": + // the column always visible, check clean, exec reporting success. On + // ALTER, `set (Visible: )` was refused as "column property + // VisibleIf not found". + "visible": {"VisibleIf"}, }, }, "com.mendix.widget.web.heatmap.HeatMap": {