diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-18-selection-property-by-widget-key-stored-as-primitive.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-18-selection-property-by-widget-key-stored-as-primitive.json new file mode 100644 index 0000000000..fd2319cd90 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-18-selection-property-by-widget-key-stored-as-primitive.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-18", "symptom": "`datagrid dg (DataSource: database Shop.Ord, itemSelection: Single)` passed `check -p`, `exec` reported \"Created page\" and `mx check` found 0 errors, but the grid had no selection: the stored itemSelection read PrimitiveValue 'Single' and Selection 'None'. `Selection: Single` stored it correctly. On a Gallery `itemSelection: None` left the Selection at the mapping default Single, and `itemSelection: Multi` stored Selection Single. Separately `gallery g (…, advanced: true)` stored nothing and said nothing (mendixlabs/mxcli#1618).", "cause": "Two passes disagreed about one property. resolveMapping read a selection mapping only from its SOURCE keyword (`Selection:`), and the explicit-key pass skipped names the mapping pass had handled by checking them against mappedKeys, which holds the mappings' SOURCE keywords — so the property's KEY (`itemSelection`) matched neither and fell to the scalar branch, where every non-attribute value type is written with SetPrimitive (PrimitiveValue, a field a selection property never reads). Separately, refuseUndeclaredMappedKey exempted every mapping with a definition constant (`value`), including one the script had overridden by its key, so `advanced: true` on a Gallery 3.x (which declares no `advanced`) was dropped by the builder's skip-unknown-key.", "file": "mdl/executor/widget_selection_value.go (one reader for both spellings: validates against the template's SelectionTypes, refuses a disagreement — MDL-WIDGET50, check and exec); widget_engine.go (resolveMapping selection branch, the explicit pass's selectionKeys and its Selection case, refuseUndeclaredMappedKey with BuildContext.ScriptAuthored); validate_widget_gallery_click.go reads the key too; modelsdk/widgets/loader.go fills PropertyTypeIDEntry.SelectionTypes", "insight": "A set of \"already handled\" names must be keyed the same way as the lookup that consults it. mappedKeys was built from mapping.Source and consulted with the name the author wrote, which for a widget key is the PropertyKey; the two coincide only for keywords, so the by-key path silently re-wrote the property by whatever its value type fell to. When a by-key write 'works' but the widget behaves as if unset, compare the stored WidgetValue field (Selection vs PrimitiveValue) against a Studio Pro-authored or keyword-authored control, ids stripped — mx check cannot tell, because both fields are valid. Other value types still reach the same scalar default in the explicit pass (Association, Action, DataSource, Widgets, Icon, Image, Object, DateTime given a plain string); this fix routes Selection only.", "refs": ["mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.mdl", "mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.fail.mdl", "mdl/executor/widget_itemselection_by_key_test.go"], "rules": ["MDL-WIDGET50", "MDL-WIDGET49"]} diff --git a/.claude/skills/mendix/create-page/reference/widgets.md b/.claude/skills/mendix/create-page/reference/widgets.md index a8fddacc31..83f3b5ec8c 100644 --- a/.claude/skills/mendix/create-page/reference/widgets.md +++ b/.claude/skills/mendix/create-page/reference/widgets.md @@ -652,6 +652,11 @@ gallery cards (datasource: database Module.Product, selection: Single, onClickTr ``` `onSelectionChange:` and `onConfigurationChange:` take an action the same way. +**Selection by key.** `selection:` is the keyword for the widget's `itemSelection` +property, on a gallery and on a data grid 2 alike; `itemSelection: Multi` names the +same property and stores the same thing. Write one of the two: a value that is not +`None`, `Single` or `Multi`, or two spellings that disagree, is refused (MDL-WIDGET50). + ### Filter Widgets A filter widget lives in one of two places: **inside a DATA GRID column's own braces**, diff --git a/CHANGELOG.md b/CHANGELOG.md index ea8ca9c59d..bd09694ea8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A selection property written by its widget key — `itemSelection: Single` on a Data grid 2 or a Gallery — is stored as a selection** (mendixlabs/mxcli#1618) — `datagrid dg (DataSource: database Shop.Ord, itemSelection: Single)` passed `check -p`, `exec` reported "Created page" and `mx check` found nothing, but the grid had no selection: the value was written as a plain primitive (`PrimitiveValue 'Single'`, `Selection 'None'`), and a selection property keeps it in `Selection`. The keyword spelling, `Selection: Single`, was always right; on a Gallery the key also lost to the default Single (`itemSelection: None` stored Single). Both spellings now store the same document, ids aside, for `None`, `Single` and `Multi`. +- **A mapped property written by its key that the installed widget does not declare is refused instead of dropped** (mendixlabs/mxcli#1618) — `gallery g (…, advanced: true)`: the Gallery definition maps `advanced` to a constant, Gallery 3.x declares no such property, and the value vanished with check, exec and `mx check` all clean. `check` says so too (MDL-WIDGET49) rather than passing what `exec` then refuses. An authored value equal to the constant (`advanced: false`) still passes, as it drops nothing. - **A specialization of a non-persistent entity reads as non-persistent** (mendixlabs/mxcli#1600) — `describe entity Probe.Child` printed `create or modify persistent entity Probe.Child extends Probe.Base (` when `Probe.Base` is non-persistent, so a reader took the child for a table and the output stated a combination Mendix does not have. A specialization stores no persistable flag of its own; it takes its root generalization's, and the reader defaulted it to persistent. The flag is now resolved along the generalization chain, across modules and into the System module (whose `HttpRequest`, `HttpResponse` and `SoapFault` read persistent for the same reason), and the fix reaches everything that reads it: `describe`, the catalog's `EntityType`, the domain-model diagrams, and `alter entities … where persistent`, which used to add its attribute to such a child as well. Re-running the describe output leaves the entity unchanged. - **`go mod tidy` runs again, and the expression round-trip test runs for the first time** — `mdl/exprcheck/roundtrip_test.go` imported `mdl/backend/mpr` (the legacy engine, deleted) plus `executor.GetHierarchyForMining` and `executor.DescribeMicroflowToString` (both removed). It never broke a build because it sat behind `//go:build roundtrip`, a tag no Makefile target and no CI job has ever passed — the only file in the tree using it. `go mod tidy` considers every build tag, so it was the one command that still saw the file, and it failed with "no matching versions for query latest" on the deleted package, which reads as a dependency problem rather than a dead test. The test is ported to `mdl/executor/exprcheck_roundtrip_test.go` under the `integration` tag, where `setupTestEnv`/`describeMDL` already provide the real-project harness and `make test-integration-executor` actually runs it; the removed hierarchy helper is replaced by building the container-to-module map from `backend.ListModules()`. Its assertion is worth keeping: describing every microflow and re-parsing each expression with the robust parser catches grammar gaps — a missing `parsePrimary` branch, a missing `funcTable` entry, a missing slot constraint — that the unit tests cannot, since those only feed the parser expressions someone thought to write down. It now also fails rather than passing silently when the fixture project yields no microflows or no expression slots. diff --git a/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.fail.mdl b/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.fail.mdl new file mode 100644 index 0000000000..5a1a0ce591 --- /dev/null +++ b/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.fail.mdl @@ -0,0 +1,28 @@ +mdl 1; +-- ============================================================================ +-- (mendixlabs/mxcli#1618) — a selection property given a value it cannot hold +-- ============================================================================ +-- +-- NEGATIVE TEST: this script must FAIL `mxcli check` (MDL-WIDGET50). +-- +-- Both gallery statements below used to pass check and exec. `itemSelection: +-- Bogus` was stored as a primitive nothing reads; `Selection: Single` together +-- with `itemSelection: Multi` stored Single in one field and Multi in another. +-- The widget declares three selections (None, Single, Multi), and the keyword and +-- the key name ONE property, so a value outside the three, or two values for it, +-- is refused instead of guessed at. + +create module BugIselFail; +create persistent entity BugIselFail.Ord ( Code: String(20) ); + +create page BugIselFail.Invalid ( Title: 'Invalid', Layout: Atlas_Core.Atlas_Default ) { + gallery galBogus ( DataSource: database BugIselFail.Ord, itemSelection: Bogus ) { + template { dynamictext txtBogus (Content: 'x') } + } +}; + +create page BugIselFail.Conflict ( Title: 'Conflict', Layout: Atlas_Core.Atlas_Default ) { + gallery galBoth ( DataSource: database BugIselFail.Ord, Selection: Single, itemSelection: Multi ) { + template { dynamictext txtBoth (Content: 'x') } + } +}; diff --git a/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.mdl b/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.mdl new file mode 100644 index 0000000000..c43d4feaa2 --- /dev/null +++ b/mdl-examples/bug-tests/1618-datagrid-itemselection-by-key.mdl @@ -0,0 +1,62 @@ +mdl 1; +-- ============================================================================ +-- (mendixlabs/mxcli#1618) — a selection property written by its widget key +-- ============================================================================ +-- +-- `itemSelection: Single` on a Data grid 2 passed check, exec reported +-- "Created page" and mx check found nothing, but the grid had no selection: +-- the stored property read PrimitiveValue 'Single' and Selection 'None'. A +-- selection-typed property keeps its value in `Selection`; only the mapped +-- keyword (`Selection: Single`) wrote it there. The key fell through to the +-- scalar route and was written as a primitive the widget never reads. +-- +-- Expected now: each pair below stores the SAME bytes (ids aside) — by key and +-- by keyword — with Selection Single / Multi and an empty PrimitiveValue. A +-- gallery's `itemSelection: None` turns off the Single it selects by default. +-- +-- Verify against a project (-p), then read the stored value: +-- mxcli exec 1618-datagrid-itemselection-by-key.mdl -p app.mpr +-- mxcli bson dump -p app.mpr --type page --object "BugIsel.ByKey" --format json +-- -> the itemSelection property: Selection 'Single' / 'Multi', PrimitiveValue '' +-- mx check app.mpr -> 0 errors +-- +-- Refused, by check and exec (MDL-WIDGET50) — see +-- 1618-datagrid-itemselection-by-key.fail.mdl: +-- itemSelection: Bogus (not one of None, Single, Multi) +-- Selection: Single, itemSelection: Multi (two spellings that disagree) + +create module BugIsel; +create persistent entity BugIsel.Ord ( Code: String(20) ); + +create page BugIsel.ByKey ( Title: 'By key', Layout: Atlas_Core.Atlas_Default ) { + datagrid dgSingle ( DataSource: database BugIsel.Ord, itemSelection: Single ) { + column (Attribute: Code) + } + datagrid dgMulti ( DataSource: database BugIsel.Ord, itemSelection: Multi ) { + column (Attribute: Code) + } + gallery galNone ( DataSource: database BugIsel.Ord, itemSelection: None ) { + template { dynamictext txtNone (Content: 'none') } + } + gallery galMulti ( DataSource: database BugIsel.Ord, itemSelection: Multi ) { + template { dynamictext txtMulti (Content: 'multi') } + } +}; + +-- The mapped keyword: the control the key must equal. +create page BugIsel.ByKeyword ( Title: 'By keyword', Layout: Atlas_Core.Atlas_Default ) { + datagrid dgSingle ( DataSource: database BugIsel.Ord, Selection: Single ) { + column (Attribute: Code) + } + datagrid dgMulti ( DataSource: database BugIsel.Ord, Selection: Multiple ) { + column (Attribute: Code) + } + gallery galNone ( DataSource: database BugIsel.Ord, Selection: None ) { + template { dynamictext txtNone (Content: 'none') } + } + gallery galMulti ( DataSource: database BugIsel.Ord, Selection: Multiple ) { + template { dynamictext txtMulti (Content: 'multi') } + } +}; + +describe page BugIsel.ByKey; diff --git a/mdl/executor/validate_widget_authored_constant.go b/mdl/executor/validate_widget_authored_constant.go new file mode 100644 index 0000000000..13aea644e8 --- /dev/null +++ b/mdl/executor/validate_widget_authored_constant.go @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "path/filepath" + "strings" + "sync" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/modelsdk/widgets/mpk" +) + +// authoredConstantUndeclared is the rule ID for a value the script writes, by the +// property's key, over a definition constant the installed widget has no property +// for. +const authoredConstantUndeclared = "MDL-WIDGET49" + +// validateAuthoredConstantUndeclared is `check`'s twin of the build-time refusal +// in refuseUndeclaredMappedKey: a mapping that carries a constant (`value` in the +// definition) is overridden when the script names the property by its key, and +// when the installed widget declares no such property the builder skips it — +// `gallery g (…, advanced: true)` on a Gallery 3.x, which has no `advanced`, +// checked clean and then failed exec (or, before the refusal, stored nothing). +// +// An authored value equal to the constant is left alone, as the builder leaves +// it: it drops nothing the definition would not have written anyway. +// +// The widget's declared keys come from the project's installed package when +// there is one, and otherwise from the embedded template the builder starts +// from; with neither, nothing is known and nothing is reported. +func validateAuthoredConstantUndeclared(w *ast.WidgetV3, def *WidgetDefinition, projectPath, locationPrefix string) []linter.Violation { + if w == nil || def == nil { + return nil + } + var declared map[string]bool + var out []linter.Violation + seen := map[string]bool{} + check := func(ms []PropertyMapping) { + for _, m := range ms { + if m.Value == "" || m.Operation == "selection" || seen[m.PropertyKey] { + continue + } + raw, ok := lookupProperty(w.Properties, m.PropertyKey) + if !ok { + continue + } + authored := stringifyAny(raw) + if authored == "" || authored == m.Value { + continue + } + if declared == nil { + declared = declaredWidgetKeys(projectPath, def.WidgetID) + } + if len(declared) == 0 || declared[strings.ToLower(m.PropertyKey)] { + continue + } + seen[m.PropertyKey] = true + out = append(out, linter.Violation{ + RuleID: authoredConstantUndeclared, + Severity: linter.SeverityError, + Message: fmt.Sprintf( + "%s: %s has no property `%s` in the installed widget — its definition maps it to the constant %q, "+ + "and the value `%s` written here would be dropped", + locationPrefix, widgetLabel(w.Name, def.MDLName), m.PropertyKey, m.Value, authored), + Suggestion: "Remove the property, or update the widget package if a newer version declares it.", + }) + } + } + check(def.PropertyMappings) + for _, mode := range def.Modes { + check(mode.PropertyMappings) + } + return out +} + +var ( + declaredWidgetKeysMu sync.Mutex + declaredWidgetKeysCache = map[string]map[string]bool{} +) + +// declaredWidgetKeys returns the lower-cased top-level property keys a widget +// declares: the project's installed package first, then the embedded template. +// Empty when neither is known. +func declaredWidgetKeys(projectPath, widgetID string) map[string]bool { + cacheKey := projectPath + "\x00" + widgetID + declaredWidgetKeysMu.Lock() + if d, ok := declaredWidgetKeysCache[cacheKey]; ok { + declaredWidgetKeysMu.Unlock() + return d + } + declaredWidgetKeysMu.Unlock() + + out := map[string]bool{} + if projectPath != "" { + projectDir := projectPath + if strings.EqualFold(filepath.Ext(projectDir), ".mpr") { + projectDir = filepath.Dir(projectDir) + } + if mpkPath, err := mpk.FindMPK(projectDir, widgetID); err == nil && mpkPath != "" { + if wd, err := mpk.ParseMPKForWidget(mpkPath, widgetID); err == nil && wd != nil { + for _, p := range wd.Properties { + out[strings.ToLower(p.Key)] = true + } + } + } + } + if len(out) == 0 { + for k := range widgetPropertyTypes(widgetID) { + out[k] = true + } + } + + declaredWidgetKeysMu.Lock() + declaredWidgetKeysCache[cacheKey] = out + declaredWidgetKeysMu.Unlock() + return out +} diff --git a/mdl/executor/validate_widget_explicit_writable.go b/mdl/executor/validate_widget_explicit_writable.go index 054297f780..eae32c3ee9 100644 --- a/mdl/executor/validate_widget_explicit_writable.go +++ b/mdl/executor/validate_widget_explicit_writable.go @@ -11,12 +11,12 @@ import ( // explicitPassWritableTypes are the property value types the widget engine's // explicit-property pass (PluggableWidgetEngine.Build, step 4.6) writes by -// storage key: Expression, TextTemplate and Attribute through their own -// setters, and the scalar types as a primitive. A value of any other type +// storage key: Expression, TextTemplate, Attribute and Selection through their +// own setters, and the scalar types as a primitive. A value of any other type // (objects, widgets, icons, images, actions, datasources…) has no correct // route there, so MDL-WIDGET06 stays true for it. var explicitPassWritableTypes = map[string]bool{ - "expression": true, "texttemplate": true, "attribute": true, + "expression": true, "texttemplate": true, "attribute": true, "selection": true, "string": true, "boolean": true, "integer": true, "decimal": true, "enumeration": true, } diff --git a/mdl/executor/validate_widget_gallery_click.go b/mdl/executor/validate_widget_gallery_click.go index b63c5f856a..713161cfe5 100644 --- a/mdl/executor/validate_widget_gallery_click.go +++ b/mdl/executor/validate_widget_gallery_click.go @@ -33,6 +33,10 @@ func validateGalleryClickAmbiguity(w *ast.WidgetV3, locationPrefix string) []lin selection := "Single" if v, ok := lookupProperty(w.Properties, "Selection"); ok { selection = stringifyAny(v) + } else if v, ok := lookupProperty(w.Properties, "itemSelection"); ok { + // The property's own key stores the same selection as the keyword; reading + // only the keyword called a `itemSelection: None` gallery a selecting one. + selection = stringifyAny(v) } trigger := "single" if v, ok := lookupProperty(w.Properties, "onClickTrigger"); ok { diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 22d9638236..31e5b1ee3d 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -264,6 +264,12 @@ func validateWidgetTreeIn(widgets []*ast.WidgetV3, registry *WidgetRegistry, loc out = append(out, validateWidgetActionSlot(w, locationPrefix)...) // #842: a Gallery row click the widget itself calls ambiguous. out = append(out, validateGalleryClickAmbiguity(w, locationPrefix)...) + // A selection property given a value its widget does not offer, or two + // spellings that disagree (`Selection:` and `itemSelection:`). + out = append(out, validateSelectionProperties(w, lookupWidgetDef(w, registry), locationPrefix)...) + // A definition constant the script overrides by key, on a widget with no such + // property: the builder drops it, and exec refuses (refuseUndeclaredMappedKey). + out = append(out, validateAuthoredConstantUndeclared(w, lookupWidgetDef(w, registry), registry.projectPath, locationPrefix)...) // #928: contentparams with no `{N}` placeholder to consume them. if lookupWidgetDef(w, registry) != nil { out = append(out, validatePluggableContentParams(w, locationPrefix)...) diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index 495a5344fb..0a73a01182 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -248,7 +248,11 @@ type BuildContext struct { // ImageEntity is the entity an image-typed property binds to when it was // written `$currentObject` rather than as a collection entry (#1476). ImageEntity string - pageBuilder *pageBuilder + // ScriptAuthored says PrimitiveVal came from the script rather than from the + // definition's own constant: a mapping with a `value` writes that constant + // unless the script names the property by its key. + ScriptAuthored bool + pageBuilder *pageBuilder } // ============================================================================= @@ -602,8 +606,29 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* for _, s := range slots { mappedKeys[s.MDLContainer] = true } + // mappedKeys holds the mappings' SOURCE keywords, but the loop below looks a + // name up as the author wrote it — and an author who writes a widget's own + // property key (`itemSelection:`) matches no keyword. A set of "already + // handled" names has to be keyed the way the lookup that consults it is, or a + // name the mapping pass DID consume is written a second time here, by whatever + // the property's value type falls to. + // + // A selection mapping reads the key (resolveMapping → resolveSelection), so the + // key it consumed is recorded under that name, lower-cased as lookupProperty + // compares. Other operations keep their old route through this pass: they do + // not all read the key, and one that does not would lose the value. + selectionKeys := make(map[string]bool) + for _, m := range mappings { + if m.Operation != "selection" { + continue + } + selectionKeys[strings.ToLower(m.PropertyKey)] = true + for _, a := range m.MdlAliases { + selectionKeys[strings.ToLower(a)] = true + } + } for propName, propVal := range w.Properties { - if mappedKeys[propName] || isBuiltinPropName(propName) { + if mappedKeys[propName] || selectionKeys[strings.ToLower(propName)] || isBuiltinPropName(propName) { continue } entry, ok := propertyTypeIDs[propName] @@ -658,6 +683,18 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* break } builder.SetTextTemplateWithParams(propName, strVal, e.entityContextFor(propName)) + case "Selection": + // A selection-typed property no mapping reaches (a hand-edited or + // older definition). Its value lives in the WidgetValue's Selection + // field; the scalar route below writes PrimitiveValue, which the widget + // never reads. Mapped ones are consumed by the mapping pass and skipped + // above. + val, problem := resolveSelectionValue(w, widgetLabelForError(w), + PropertyMapping{PropertyKey: propName, Operation: "selection"}, entry.SelectionTypes, false) + if problem != nil { + return nil, problem.execError() + } + builder.SetSelection(propName, val) case "Attribute": // The property's own scope, as the mapping pass uses — this pass // re-applies a property authored by its schema key AFTER every @@ -1023,8 +1060,15 @@ func singleAttributeOverflowMessage(w *ast.WidgetV3, m PropertyMapping, attrs [] // (`value` in the .def.json) is not checked: it is the definition's default, not // the user's data, and an older widget version lacking the key is not the // script's fault. +// +// The exemption ends where the script takes the constant over. `advanced: true` +// on a Gallery names the mapped property by its key, so the mapping writes the +// author's value in place of the constant — and a Gallery 3.x declares no +// `advanced`, so the value vanished with nothing said. An authored value equal +// to the constant is still exempt: it drops nothing the definition would not +// have written anyway. func refuseUndeclaredMappedKey(builder backend.WidgetObjectBuilder, def *WidgetDefinition, mapping PropertyMapping, ctx *BuildContext, w *ast.WidgetV3) error { - if mapping.Value != "" || !ctx.carriesValue() { + if constantOnly := mapping.Value != "" && (!ctx.ScriptAuthored || ctx.PrimitiveVal == mapping.Value); constantOnly || !ctx.carriesValue() { return nil } declarer, ok := builder.(backend.WidgetPropertyDeclarer) @@ -1441,9 +1485,41 @@ func (e *PluggableWidgetEngine) recordDataSourceEntity(propertyKey, entityName s } // resolveMapping resolves a PropertyMapping's source into a BuildContext. +// resolveSelection reads a selection-typed property from the mapped keyword and +// from its own key, validated against the selection kinds the loaded template +// declares for it. "" means the script wrote neither. See +// widget_selection_value.go. +func (e *PluggableWidgetEngine) resolveSelection(mapping PropertyMapping, w *ast.WidgetV3) (string, error) { + val, problem := resolveSelectionValue(w, widgetLabelForError(w), mapping, + e.currentPropertyTypeIDs[mapping.PropertyKey].SelectionTypes, + selectionKeywordShared(e.currentDef, mapping.Source)) + if problem != nil { + return "", problem.execError() + } + return val, nil +} + func (e *PluggableWidgetEngine) resolveMapping(mapping PropertyMapping, w *ast.WidgetV3) (*BuildContext, error) { ctx := &BuildContext{pageBuilder: e.pageBuilder} + // A selection-typed property reads from the mapped keyword AND from the + // property's own key, and validates both (widget_selection_value.go). It comes + // first because its constant, if any, is only the fallback. + if mapping.Operation == "selection" { + val, err := e.resolveSelection(mapping, w) + if err != nil { + return nil, err + } + if val == "" { + val = mapping.Value + } + if val == "" { + val = mapping.Default + } + ctx.PrimitiveVal = val + return ctx, nil + } + if mapping.Value != "" { ctx.PrimitiveVal = mapping.Value // AST overrides the def.json default: when the user explicitly sets the @@ -1456,6 +1532,7 @@ func (e *PluggableWidgetEngine) resolveMapping(mapping PropertyMapping, w *ast.W return nil, err } ctx.PrimitiveVal = val + ctx.ScriptAuthored = true } return ctx, nil } @@ -1622,13 +1699,6 @@ func (e *PluggableWidgetEngine) resolveMapping(mapping PropertyMapping, w *ast.W } } - case "Selection": - val := w.GetSelection() - if val == "" && mapping.Default != "" { - val = mapping.Default - } - ctx.PrimitiveVal = val - case "CaptionAttribute": if captionAttr := w.GetStringProp("CaptionAttribute"); captionAttr != "" { // Qualified with the entity that DECLARES the attribute, as every diff --git a/mdl/executor/widget_itemselection_by_key_test.go b/mdl/executor/widget_itemselection_by_key_test.go new file mode 100644 index 0000000000..08ecf9751c --- /dev/null +++ b/mdl/executor/widget_itemselection_by_key_test.go @@ -0,0 +1,580 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "path/filepath" + "reflect" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + modelsdkbackend "github.com/mendixlabs/mxcli/mdl/backend/modelsdk" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" + "github.com/mendixlabs/mxcli/sdk/pages" + "github.com/mendixlabs/mxcli/sdk/widgets/mpk" +) + +// A selection-typed property written by its widget KEY. +// +// datagrid dg (DataSource: database Shop.Ord, itemSelection: Single) { column (Attribute: Code) } +// +// passed check, exec reported "Created page" and mx check found nothing, but the +// grid had NO selection: the stored itemSelection read `PrimitiveValue 'Single'` +// and `Selection 'None'`. A selection-typed property keeps its value in the +// WidgetValue's `Selection` field, not in `PrimitiveValue`, so `itemSelection:` +// was written into a field the widget never reads. The mapped spelling, +// `Selection: Single`, stored it correctly. +// +// Two code paths disagreed about the same property. The mapping pass reads a +// selection mapping from its SOURCE keyword (`Selection:`) alone; the +// explicit-key pass skips what the mapping pass handled by comparing the +// authored name against the mappings' SOURCE names — so a name that is the +// property's own KEY (`itemSelection`) matched neither, and fell to the +// scalar branch, which writes every non-attribute type as a primitive. + +// selectionEngine builds over Shop.Ord (Code), page-level: the widget's own +// DataSource decides the entity. +func selectionEngine(t *testing.T) *PluggableWidgetEngine { + t.Helper() + real := &modelsdkbackend.Backend{} + mod := &model.Module{BaseElement: model.BaseElement{ID: model.ID("mod-shop")}, Name: "Shop"} + b := &mock.MockBackend{ + LoadWidgetTemplateFunc: real.LoadWidgetTemplate, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { + return []*domainmodel.DomainModel{{ + BaseElement: model.BaseElement{ID: model.ID("dm-shop")}, + ContainerID: mod.ID, + Entities: []*domainmodel.Entity{ + {BaseElement: model.BaseElement{ID: model.ID("e-ord")}, Name: "Ord", + Attributes: []*domainmodel.Attribute{{Name: "Code"}}}, + }, + }}, nil + }, + } + pb := &pageBuilder{ + backend: b, + paramEntityNames: map[string]string{}, + widgetScope: map[string]model.ID{}, + } + e := NewPluggableWidgetEngine(b, pb) + pb.pluggableEngine = e + return e +} + +// datagridDef is the definition a project gets for Data grid 2: generated from +// the installed package, not embedded. The test reads the package from testdata, +// so the mapping is the generator's real output. +func datagridDef(t *testing.T) *WidgetDefinition { + t.Helper() + path := filepath.Join("..", "..", "testdata", "expr-checker", "widgets", "com.mendix.widget.web.Datagrid.mpk") + defs, err := mpk.ParseMPKAll(path) + if err != nil { + t.Fatalf("parse %s: %v", path, err) + } + if len(defs) == 0 { + t.Fatalf("%s holds no widget", path) + } + return GenerateDefJSON(defs[0], "DATAGRID") +} + +func galleryDef(t *testing.T) *WidgetDefinition { + t.Helper() + reg, err := NewWidgetRegistry() + if err != nil { + t.Fatal(err) + } + def, ok := reg.Get("gallery") + if !ok { + t.Fatal("gallery is not registered") + } + return def +} + +// selectionWidget is ` w (DataSource: database Shop.Ord, )`. +func selectionWidget(kind string, props map[string]any) *ast.WidgetV3 { + all := map[string]any{"DataSource": &ast.DataSourceV3{Type: "database", Reference: "Shop.Ord"}} + for k, v := range props { + all[k] = v + } + return &ast.WidgetV3{Name: "w", Type: kind, Properties: all} +} + +func buildSelection(t *testing.T, def *WidgetDefinition, kind string, props map[string]any) (*pages.CustomWidget, error) { + t.Helper() + e := selectionEngine(t) + return e.Build(def, selectionWidget(kind, props)) +} + +func mustBuildSelection(t *testing.T, def *WidgetDefinition, kind string, props map[string]any) *pages.CustomWidget { + t.Helper() + cw, err := buildSelection(t, def, kind, props) + if err != nil { + t.Fatalf("Build: %v", err) + } + return cw +} + +func storedSelection(t *testing.T, cw *pages.CustomWidget, key string) string { + t.Helper() + s, _ := bsonField(customWidgetValue(t, cw, key), "Selection").(string) + return s +} + +// storedWithoutIDs drops what differs between two builds of the same widget by +// construction — freshly minted element ids and the pointers to them — so what +// is left is the stored configuration. +func storedWithoutIDs(v any) any { + switch x := v.(type) { + case bson.D: + out := make(bson.D, 0, len(x)) + for _, e := range x { + if e.Key == "$ID" || e.Key == "TypePointer" { + continue + } + out = append(out, bson.E{Key: e.Key, Value: storedWithoutIDs(e.Value)}) + } + return out + case bson.A: + out := make(bson.A, len(x)) + for i, e := range x { + out[i] = storedWithoutIDs(e) + } + return out + } + return v +} + +// sameStoredObject fails unless the two widgets store the same configuration, +// every property, ids aside. The visibility rules that hang off itemSelection +// (itemSelectionMethod, onSelectionChange, singleSelectionColumnLabel…) would +// show a divergence there even where itemSelection itself matched. +func sameStoredObject(t *testing.T, a, b *pages.CustomWidget) { + t.Helper() + if av, bv := storedWithoutIDs(a.RawObject), storedWithoutIDs(b.RawObject); !reflect.DeepEqual(av, bv) { + t.Errorf("the widgets are stored differently:\n by key: %v\n mapped: %v", av, bv) + } +} + +// sameStoredProperty fails unless the property is stored identically (ids +// aside) in both widgets. +func sameStoredProperty(t *testing.T, a, b *pages.CustomWidget, key string) { + t.Helper() + av, bv := storedWithoutIDs(customWidgetValue(t, a, key)), storedWithoutIDs(customWidgetValue(t, b, key)) + if !reflect.DeepEqual(av, bv) { + t.Errorf("%s is stored differently:\n by key: %v\n mapped: %v", key, av, bv) + } +} + +// The reported case, per widget and per value. The mapped spelling is the +// control: it already worked, and the by-key one must store exactly what it does. +func TestItemSelectionByKey_StoresTheSelectionField(t *testing.T) { + // `Multiple` is the mapped keyword's spelling for what the widget calls Multi. + cases := []struct{ byKey, mapped, want string }{ + {"Single", "Single", "Single"}, + {"Multi", "Multiple", "Multi"}, + {"None", "None", "None"}, + } + for _, w := range []struct { + kind string + def func(*testing.T) *WidgetDefinition + }{{"datagrid", datagridDef}, {"gallery", galleryDef}} { + for _, c := range cases { + t.Run(w.kind+" "+c.byKey, func(t *testing.T) { + def := w.def(t) + byKey := mustBuildSelection(t, def, w.kind, map[string]any{"itemSelection": c.byKey}) + mapped := mustBuildSelection(t, def, w.kind, map[string]any{"Selection": c.mapped}) + + if got := storedSelection(t, byKey, "itemSelection"); got != c.want { + t.Errorf("by key: stored Selection = %q, want %q", got, c.want) + } + if got := storedPrimitive(t, byKey, "itemSelection"); got != storedPrimitive(t, mapped, "itemSelection") { + t.Errorf("by key: PrimitiveValue = %q — a selection property keeps its value in Selection, "+ + "and the primitive field must stay what the mapped spelling leaves (%q)", + got, storedPrimitive(t, mapped, "itemSelection")) + } + sameStoredProperty(t, byKey, mapped, "itemSelection") + sameStoredObject(t, byKey, mapped) + }) + } + } +} + +// The default the mapping supplies must not survive an explicit by-key value — +// a gallery selects Single when told nothing, and `itemSelection: None` is the +// way to turn that off. +func TestItemSelectionByKey_OverridesTheMappingDefault(t *testing.T) { + def := galleryDef(t) + if got := storedSelection(t, mustBuildSelection(t, def, "gallery", nil), "itemSelection"); got != "Single" { + t.Fatalf("control: an unconfigured gallery stores Selection %q, want Single", got) + } + cw := mustBuildSelection(t, def, "gallery", map[string]any{"itemSelection": "None"}) + if got := storedSelection(t, cw, "itemSelection"); got != "None" { + t.Errorf("gallery `itemSelection: None` stored Selection %q, want None", got) + } +} + +// A value the widget does not declare is refused, not stored. Case is the +// author's: the mapped keyword accepts any case and so does the key. +func TestItemSelectionByKey_InvalidValueRefusedByExec(t *testing.T) { + for _, kind := range []string{"datagrid", "gallery"} { + t.Run(kind, func(t *testing.T) { + def := datagridDef(t) + if kind == "gallery" { + def = galleryDef(t) + } + _, err := buildSelection(t, def, kind, map[string]any{"itemSelection": "Bogus"}) + if err == nil { + t.Fatal("`itemSelection: Bogus` was written") + } + for _, want := range []string{"itemSelection", "Bogus", "None", "Single", "Multi"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal should mention %q, got: %v", want, err) + } + } + }) + } + // The control: a valid value in another case is fine. + if _, err := buildSelection(t, datagridDef(t), "datagrid", map[string]any{"itemSelection": "multi"}); err != nil { + t.Errorf("`itemSelection: multi` should be accepted: %v", err) + } +} + +// Both spellings given: the mapped keyword and the key name the same property, +// so they must agree. Disagreement is refused — either would be a guess, and +// "the last one wins" depends on the order of two passes. +func TestItemSelectionByKey_ConflictingSpellingsRefusedByExec(t *testing.T) { + for _, kind := range []string{"datagrid", "gallery"} { + t.Run(kind, func(t *testing.T) { + def := datagridDef(t) + if kind == "gallery" { + def = galleryDef(t) + } + _, err := buildSelection(t, def, kind, map[string]any{"Selection": "Single", "itemSelection": "Multi"}) + if err == nil { + t.Fatal("`Selection: Single` with `itemSelection: Multi` was written") + } + for _, want := range []string{"Selection", "itemSelection", "Single", "Multi"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal should mention %q, got: %v", want, err) + } + } + }) + } + // The controls: the same value twice, however spelled, is one statement. + for _, c := range []struct{ mapped, byKey string }{{"Single", "single"}, {"Multiple", "Multi"}} { + cw, err := buildSelection(t, datagridDef(t), "datagrid", map[string]any{"Selection": c.mapped, "itemSelection": c.byKey}) + if err != nil { + t.Errorf("Selection: %s with itemSelection: %s agree and must be accepted: %v", c.mapped, c.byKey, err) + continue + } + if got := storedSelection(t, cw, "itemSelection"); got != canonicalSelection(c.byKey) { + t.Errorf("Selection: %s with itemSelection: %s stored %q", c.mapped, c.byKey, got) + } + } +} + +// A selection-typed property no mapping covers still has a value type, and the +// explicit-key pass has to honour it — a definition edited by hand, or one +// generated before the property existed, must not turn the same key back into a +// primitive write. +func TestItemSelectionByKey_UnmappedSelectionPropertyIsWrittenAsSelection(t *testing.T) { + def := datagridDef(t) + var kept []PropertyMapping + for _, m := range def.PropertyMappings { + if m.PropertyKey != "itemSelection" { + kept = append(kept, m) + } + } + if len(kept) == len(def.PropertyMappings) { + t.Fatal("test setup: the generated definition has no itemSelection mapping to remove") + } + def.PropertyMappings = kept + + cw := mustBuildSelection(t, def, "datagrid", map[string]any{"itemSelection": "Multi"}) + if got := storedSelection(t, cw, "itemSelection"); got != "Multi" { + t.Errorf("unmapped itemSelection stored Selection %q, want Multi", got) + } + if got := storedPrimitive(t, cw, "itemSelection"); got != "" { + t.Errorf("unmapped itemSelection wrote PrimitiveValue %q", got) + } + if _, err := buildSelection(t, def, "datagrid", map[string]any{"itemSelection": "Bogus"}); err == nil { + t.Error("an unmapped itemSelection: Bogus was written") + } +} + +// Properties that ARE scalars keep their route. These are the controls for the +// change: a fix that rerouted by-key writes wholesale would show here first. +func TestItemSelectionByKey_ScalarKeysUnchanged(t *testing.T) { + cw := mustBuildSelection(t, datagridDef(t), "datagrid", map[string]any{"refreshInterval": 30, "pageSize": 7}) + if got := storedPrimitive(t, cw, "refreshInterval"); got != "30" { + t.Errorf("datagrid refreshInterval = %q, want 30", got) + } + if got := storedPrimitive(t, cw, "pageSize"); got != "7" { + t.Errorf("datagrid pageSize = %q, want 7", got) + } + g := mustBuildSelection(t, galleryDef(t), "gallery", map[string]any{"pageSize": 30, "pagination": "virtualScrolling"}) + if got := storedPrimitive(t, g, "pageSize"); got != "30" { + t.Errorf("gallery pageSize = %q, want 30", got) + } + if got := storedPrimitive(t, g, "pagination"); got != "virtualScrolling" { + t.Errorf("gallery pagination = %q, want virtualScrolling", got) + } +} + +// A mapped constant written by its key. +// +// The gallery definition maps `advanced` to the constant "false", but the +// Gallery widget (3.x) declares no such property — the definition names a key an +// older version had. The mapping pass wrote the author's `advanced: true` over +// the constant, the builder skipped the unknown key without a word, and the +// refusal that exists for exactly this (refuseUndeclaredMappedKey) exempted every +// mapping with a constant, authored or not. Nothing was stored and nothing was said. +func TestMappedConstantByKey_UndeclaredKeyIsRefusedWhenAuthored(t *testing.T) { + def := galleryDef(t) + _, err := buildSelection(t, def, "gallery", map[string]any{"advanced": true}) + if err == nil { + t.Fatal("gallery `advanced: true` was accepted and dropped: the widget has no such property") + } + for _, want := range []string{"advanced", "does not declare"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("refusal should mention %q, got: %v", want, err) + } + } + // The controls. The definition's own constant is not the author's data, and + // an authored value equal to it drops nothing the definition would not have. + for name, props := range map[string]map[string]any{"unset": nil, "equal to the constant": {"advanced": false}} { + if _, err := buildSelection(t, def, "gallery", props); err != nil { + t.Errorf("gallery with advanced %s was refused: %v", name, err) + } + } +} + +// …and on a widget that does declare the property, the author's value replaces +// the constant, as it always did. +func TestMappedConstantByKey_DeclaredKeyIsStored(t *testing.T) { + on := mustBuildSelection(t, datagridDef(t), "datagrid", map[string]any{"advanced": true}) + off := mustBuildSelection(t, datagridDef(t), "datagrid", nil) + if got := storedPrimitive(t, on, "advanced"); got != "true" { + t.Errorf("datagrid `advanced: true` stored %q, want true", got) + } + if got := storedPrimitive(t, off, "advanced"); got != "false" { + t.Errorf("control: an unconfigured datagrid stores advanced = %q, want false", got) + } +} + +// --- check ------------------------------------------------------------------- + +func selectionRegistry(t *testing.T) *WidgetRegistry { + t.Helper() + reg, err := NewWidgetRegistry() + if err != nil { + t.Fatal(err) + } + dg := datagridDef(t) + dg.MDLName = "datagrid" + reg.byMDLName["DATAGRID"] = dg + reg.byWidgetID[dg.WidgetID] = dg + return reg +} + +func selectionPage(widget string) string { + return `create page Shop.P ( Title: 'P' ) +{ + ` + widget + ` +}` +} + +func checkSelection(t *testing.T, widget string) []linter.Violation { + t.Helper() + prog, errs := visitor.Build(selectionPage(widget)) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + var out []linter.Violation + reg := selectionRegistry(t) + for _, stmt := range prog.Statements { + out = append(out, ValidateWidgetPropertiesForStatement(stmt, reg)...) + } + return GateWidgetViolations(out, prog.LanguageVersion) +} + +func violationsOf(vs []linter.Violation, rule string) []linter.Violation { + var out []linter.Violation + for _, v := range vs { + if v.RuleID == rule { + out = append(out, v) + } + } + return out +} + +func TestItemSelectionByKey_InvalidValueRefusedByCheck(t *testing.T) { + for _, widget := range []string{ + `datagrid dg (DataSource: database Shop.Ord, itemSelection: Bogus) { column (Attribute: Code) }`, + `gallery g (DataSource: database Shop.Ord, itemSelection: Bogus) { template t { dynamictext x (Content: 'x') } }`, + } { + t.Run(strings.Fields(widget)[0], func(t *testing.T) { + got := violationsOf(checkSelection(t, widget), selectionValueRule) + if len(got) != 1 { + t.Fatalf("got %d %s violations, want 1", len(got), selectionValueRule) + } + if got[0].Severity != linter.SeverityError { + t.Errorf("severity = %v, want Error", got[0].Severity) + } + for _, want := range []string{"itemSelection", "Bogus", "None", "Single", "Multi"} { + if !strings.Contains(got[0].Message, want) { + t.Errorf("message should mention %q: %s", want, got[0].Message) + } + } + }) + } +} + +func TestItemSelectionByKey_ConflictRefusedByCheck(t *testing.T) { + widget := `datagrid dg (DataSource: database Shop.Ord, Selection: Single, itemSelection: Multi) { column (Attribute: Code) }` + got := violationsOf(checkSelection(t, widget), selectionValueRule) + if len(got) != 1 { + t.Fatalf("got %d %s violations, want 1", len(got), selectionValueRule) + } + for _, want := range []string{"Selection", "itemSelection", "Single", "Multi"} { + if !strings.Contains(got[0].Message, want) { + t.Errorf("message should mention %q: %s", want, got[0].Message) + } + } +} + +// The controls: every spelling that is right stays silent, so the rule cannot +// have been satisfied by firing on everything. +func TestItemSelectionByKey_ValidSpellingsPassCheck(t *testing.T) { + for _, widget := range []string{ + `datagrid dg (DataSource: database Shop.Ord, itemSelection: Single) { column (Attribute: Code) }`, + `datagrid dg (DataSource: database Shop.Ord, itemSelection: Multi) { column (Attribute: Code) }`, + `datagrid dg (DataSource: database Shop.Ord, itemSelection: none) { column (Attribute: Code) }`, + `datagrid dg (DataSource: database Shop.Ord, Selection: Multiple) { column (Attribute: Code) }`, + `datagrid dg (DataSource: database Shop.Ord, Selection: Single, itemSelection: Single) { column (Attribute: Code) }`, + `datagrid dg (DataSource: database Shop.Ord) { column (Attribute: Code) }`, + } { + t.Run(widget, func(t *testing.T) { + if got := violationsOf(checkSelection(t, widget), selectionValueRule); len(got) != 0 { + t.Errorf("a right spelling was refused: %v", got) + } + }) + } +} + +// The gallery's own ambiguity check reads the selection the widget will get. +// It read `Selection:` alone, so a by-key `itemSelection: None` — which stores +// None — was still reported as "a selection and an onClick". +func TestItemSelectionByKey_GalleryClickRuleSeesTheKey(t *testing.T) { + const click = `onClick: show_page Shop.Detail` + for _, c := range []struct { + props string + refused bool + }{ + {`itemSelection: None`, false}, + {`Selection: None`, false}, + {`itemSelection: Single`, true}, + {``, true}, + } { + t.Run(c.props, func(t *testing.T) { + w := `gallery g (DataSource: database Shop.Ord, ` + click + `, ` + c.props + `) { template t { dynamictext x (Content: 'x') } }` + if c.props == "" { + w = `gallery g (DataSource: database Shop.Ord, ` + click + `) { template t { dynamictext x (Content: 'x') } }` + } + got := violationsOf(checkSelection(t, w), galleryClickRefused.Code) + if (len(got) > 0) != c.refused { + t.Errorf("the gallery click rule fired = %v, want %v", len(got) > 0, c.refused) + } + }) + } +} + +// The check-time twin of the build refusal above. `gallery … advanced: true` +// checked clean and then failed exec; check now says so first. The controls are +// the value that equals the constant, and a widget that does declare the key. +func TestMappedConstantByKey_UndeclaredKeyIsRefusedByCheck(t *testing.T) { + const tmpl = `template { dynamictext t (Content: 'x') }` + got := violationsOf(checkSelection(t, + `gallery g (DataSource: database Shop.Ord, advanced: true) { `+tmpl+` }`), authoredConstantUndeclared) + if len(got) != 1 { + t.Fatalf("gallery `advanced: true`: got %d %s violations, want 1", len(got), authoredConstantUndeclared) + } + if got[0].Severity != linter.SeverityError || !strings.Contains(got[0].Message, "advanced") { + t.Errorf("unexpected violation: %+v", got[0]) + } + for name, widget := range map[string]string{ + "gallery advanced: false": `gallery g (DataSource: database Shop.Ord, advanced: false) { ` + tmpl + ` }`, + "gallery without advanced": `gallery g (DataSource: database Shop.Ord) { ` + tmpl + ` }`, + "datagrid advanced: true": `datagrid dg (DataSource: database Shop.Ord, advanced: true) { column (Attribute: Code) }`, + } { + if got := violationsOf(checkSelection(t, widget), authoredConstantUndeclared); len(got) != 0 { + t.Errorf("%s was refused: %v", name, got) + } + } +} + +// A selection-typed property that has no keyword at all — the combo box's +// `optionsSourceDatabaseItemSelection` — can only be written by its key. It goes +// through the explicit-key pass, which must store the Selection field (not a +// primitive), and `check` must not call it unpersisted (MDL-WIDGET06). +func TestItemSelectionByKey_ComboBoxSelectionWithNoKeyword(t *testing.T) { + reg, err := NewWidgetRegistry() + if err != nil { + t.Fatal(err) + } + def, ok := reg.Get("combobox") + if !ok { + t.Fatal("combobox is not registered") + } + const key = "optionsSourceDatabaseItemSelection" + for _, m := range selectionMappingsOf(def) { + if m.PropertyKey == key { + t.Fatalf("test setup: %s is mapped; this test is for a selection with no keyword", key) + } + } + + e := selectionEngine(t) + w := &ast.WidgetV3{Name: "cb", Type: "combobox", Properties: map[string]any{ + "DataSource": &ast.DataSourceV3{Type: "database", Reference: "Shop.Ord"}, + key: "Multi", + }} + cw, err := e.Build(def, w) + if err != nil { + t.Fatalf("Build: %v", err) + } + if got := storedSelection(t, cw, key); got != "Multi" { + t.Errorf("%s stored Selection %q, want Multi", key, got) + } + if got := storedPrimitive(t, cw, key); got != "" { + t.Errorf("%s wrote PrimitiveValue %q", key, got) + } + + // check: the key is recognised (the project's package declares it) and, being + // written by the explicit pass, must not be reported as dropped. + d := *def + d.KnownProperties = append(append([]string(nil), def.KnownProperties...), key) + reg2 := selectionRegistry(t) + reg2.byMDLName["COMBOBOX"] = &d + reg2.byWidgetID[d.WidgetID] = &d + prog, errs := visitor.Build(selectionPage(`combobox cb (DataSource: database Shop.Ord, ` + key + `: Multi)`)) + if len(errs) > 0 { + t.Fatalf("parse error: %v", errs[0]) + } + var vs []linter.Violation + for _, stmt := range prog.Statements { + vs = append(vs, ValidateWidgetPropertiesForStatement(stmt, reg2)...) + } + for _, v := range vs { + if v.RuleID == "MDL-WIDGET06" || v.RuleID == "MDL-WIDGET01" || v.RuleID == selectionValueRule { + t.Errorf("unexpected %s: %s", v.RuleID, v.Message) + } + } +} diff --git a/mdl/executor/widget_selection_value.go b/mdl/executor/widget_selection_value.go new file mode 100644 index 0000000000..f98df47335 --- /dev/null +++ b/mdl/executor/widget_selection_value.go @@ -0,0 +1,235 @@ +// SPDX-License-Identifier: Apache-2.0 + +// A selection-typed widget property — a Data grid 2's or Gallery's +// `itemSelection` — and the two ways MDL lets a script name it. +// +// The mapped keyword, `Selection: Single`, is the older spelling; the property's +// own key, `itemSelection: Single`, is how every other widget property is +// addressed and what DESCRIBE emits for a widget whose keyword is not mapped. +// Both name ONE property, and that property keeps its value in the WidgetValue's +// `Selection` field — not in `PrimitiveValue`, where a plain scalar lives. +// +// Before, only the keyword was read as a selection. The key fell through to the +// explicit-property pass, which wrote it as a scalar: `PrimitiveValue 'Single'`, +// `Selection 'None'`, a grid with no selection, and nothing reported — check, +// exec and mx check were all clean. The pass that decides which names the mapping +// pass already handled compared them against the mappings' SOURCE keywords, so the +// property's KEY matched nothing. +// +// This file is the one place that reads either spelling, validates the value +// against what the widget declares, and decides what happens when both are given. +// The engine (resolveMapping, the explicit pass) and `check` +// (validateSelectionProperties) both call it, so they cannot disagree. + +package executor + +import ( + "fmt" + "strings" + "sync" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/linter" + mwidgets "github.com/mendixlabs/mxcli/modelsdk/widgets" +) + +// selectionValueRule is the rule ID for a selection property given a value the +// widget does not offer, or two spellings that disagree. Not gated on the +// language version: neither input ever produced what the script said, so there +// is no earlier meaning to preserve (compare refuseUndeclaredMappedKey, which is +// ungated for the same reason). +const selectionValueRule = "MDL-WIDGET50" + +// allSelectionTypes is the set a selection property falls back to when the +// widget's template names none. It is the whole of the Mendix enumeration, so +// the fallback never refuses a value the widget could take. +var allSelectionTypes = []string{"None", "Single", "Multi"} + +// selectionProblem is a selection value `check` and `exec` both refuse. +type selectionProblem struct{ msg string } + +func (p *selectionProblem) Error() string { return p.msg } + +// execError is the refusal exec makes. check reports the rule ID itself, so the +// message carries none; exec has no such wrapper and names it here. +func (p *selectionProblem) execError() error { + return mdlerrors.NewValidation(p.msg + " [" + selectionValueRule + "]") +} + +// selectionAuthored reads what the script wrote for a selection property: under +// the mapped keyword (the mapping's Source), and under the property's own key or +// one of its aliases. Either is "" when not written. +func selectionAuthored(w *ast.WidgetV3, m PropertyMapping) (mapped, byKey string) { + if m.Source != "" { + if v, ok := lookupProperty(w.Properties, m.Source); ok { + mapped = stringifyAny(v) + } + } + for _, name := range append([]string{m.PropertyKey}, m.MdlAliases...) { + if name == "" || strings.EqualFold(name, m.Source) { + continue + } + if v, ok := lookupProperty(w.Properties, name); ok { + if s := stringifyAny(v); s != "" { + return mapped, s + } + } + } + return mapped, "" +} + +// resolveSelectionValue returns the selection a script gives a property, in the +// canonical spelling (None, Single or Multi), or "" when the script gives none. +// +// allowed is what the widget declares for the property (empty: all three). +// keywordShared says the mapped keyword feeds more than one selection property, +// so it names none of them in particular and cannot contradict a key. +// +// Precedence, when both spellings are given: they must agree. A disagreement is +// refused rather than resolved, because either choice is a guess and "the last +// pass wins" would make the stored value depend on the order of two passes. +func resolveSelectionValue(w *ast.WidgetV3, label string, m PropertyMapping, allowed []string, keywordShared bool) (string, *selectionProblem) { + if len(allowed) == 0 { + allowed = allSelectionTypes + } + mapped, byKey := selectionAuthored(w, m) + canon := func(spelling, raw string) (string, *selectionProblem) { + if raw == "" { + return "", nil + } + c := canonicalSelection(raw) + for _, a := range allowed { + if strings.EqualFold(c, a) { + return a, nil + } + } + return "", &selectionProblem{fmt.Sprintf( + "%s: `%s: %s` is not a selection this widget offers — valid values are %s", + label, spelling, raw, strings.Join(allowed, ", "))} + } + mappedVal, p := canon(m.Source, mapped) + if p != nil { + return "", p + } + keyVal, p := canon(m.PropertyKey, byKey) + if p != nil { + return "", p + } + if mappedVal != "" && keyVal != "" && mappedVal != keyVal && !keywordShared { + return "", &selectionProblem{fmt.Sprintf( + "%s: `%s: %s` and `%s: %s` both set the selection and disagree — keep one of them", + label, m.Source, mapped, m.PropertyKey, byKey)} + } + if keyVal != "" { + return keyVal, nil + } + return mappedVal, nil +} + +// selectionMappingsOf lists the selection mappings of a definition — top level +// and in every mode, one per property key. +func selectionMappingsOf(def *WidgetDefinition) []PropertyMapping { + if def == nil { + return nil + } + var out []PropertyMapping + seen := map[string]bool{} + add := func(ms []PropertyMapping) { + for _, m := range ms { + if m.Operation == "selection" && !seen[m.PropertyKey] { + seen[m.PropertyKey] = true + out = append(out, m) + } + } + } + add(def.PropertyMappings) + for _, mode := range def.Modes { + add(mode.PropertyMappings) + } + return out +} + +// selectionKeywordShared reports whether the mapped keyword feeds several of the +// definition's selection properties. +func selectionKeywordShared(def *WidgetDefinition, source string) bool { + if source == "" { + return false + } + n := 0 + for _, m := range selectionMappingsOf(def) { + if m.Source == source { + n++ + } + } + return n > 1 +} + +var templateSelectionTypesCache sync.Map // widgetID → map[propertyKey][]string + +// templateSelectionTypes returns, per property key, the selection kinds the +// widget's embedded template declares for its selection-typed properties. It is +// the check-time twin of PropertyTypeIDEntry.SelectionTypes, which the builder +// reads from the same template; empty when the widget has no embedded template. +func templateSelectionTypes(widgetID string) map[string][]string { + if v, ok := templateSelectionTypesCache.Load(widgetID); ok { + return v.(map[string][]string) + } + out := map[string][]string{} + if tmpl, err := mwidgets.GetTemplate(widgetID); err == nil && tmpl != nil { + objType, _ := tmpl.Type["ObjectType"].(map[string]any) + pts, _ := objType["PropertyTypes"].([]any) + for _, pt := range pts { + m, ok := pt.(map[string]any) + if !ok { + continue // leading array marker + } + vt, _ := m["ValueType"].(map[string]any) + if vt == nil || asString(vt["Type"]) != "Selection" { + continue + } + out[asString(m["PropertyKey"])] = mwidgets.SelectionTypesOf(vt) + } + } + templateSelectionTypesCache.Store(widgetID, out) + return out +} + +// validateSelectionProperties refuses, at check time, a selection property given +// a value its widget does not offer, or given under both spellings with two +// different values — the same refusals the engine makes on exec. +// +// It covers the definition's selection mappings and any selection-typed property +// of the template no mapping reaches (those are written by the explicit-property +// pass, which applies the same rule). +func validateSelectionProperties(w *ast.WidgetV3, def *WidgetDefinition, locationPrefix string) []linter.Violation { + if w == nil || def == nil { + return nil + } + types := templateSelectionTypes(def.WidgetID) + mappings := selectionMappingsOf(def) + covered := map[string]bool{} + for _, m := range mappings { + covered[m.PropertyKey] = true + } + for key := range types { + if !covered[key] { + mappings = append(mappings, PropertyMapping{PropertyKey: key, Operation: "selection"}) + } + } + var out []linter.Violation + for _, m := range mappings { + _, p := resolveSelectionValue(w, widgetLabel(w.Name, def.MDLName), m, types[m.PropertyKey], + selectionKeywordShared(def, m.Source)) + if p == nil { + continue + } + out = append(out, linter.Violation{ + RuleID: selectionValueRule, + Severity: linter.SeverityError, + Message: locationPrefix + ": " + p.msg, + Suggestion: "Write the selection once — `itemSelection: Single` or `Selection: Single` — with None, Single or Multi.", + }) + } + return out +} diff --git a/mdl/types/widget_property_type.go b/mdl/types/widget_property_type.go index 72ded9353b..a55c9cef24 100644 --- a/mdl/types/widget_property_type.go +++ b/mdl/types/widget_property_type.go @@ -17,6 +17,12 @@ type PropertyTypeIDEntry struct { DefaultValue string // Default value from the template's ValueType ValueType string // Type of value (Boolean, Integer, String, DataSource, etc.) Required bool // Whether this property is required + // SelectionTypes lists the selection kinds a Selection-typed property offers, + // as the widget declares them ( in widget.xml, the template's + // ValueType.SelectionTypes): None, Single and/or Multi. Empty for every other + // type, and where the template carries none — a caller then falls back to the + // full set rather than guessing a narrower one. + SelectionTypes []string // DefaultTranslations are the widget-shipped for this property. // A REQUIRED TextTemplate the author leaves unset must be serialized WITH this // text: a null there is CE0463 "the definition of this widget has changed", diff --git a/modelsdk/widgets/loader.go b/modelsdk/widgets/loader.go index faf53f8ea3..cab4187100 100644 --- a/modelsdk/widgets/loader.go +++ b/modelsdk/widgets/loader.go @@ -415,6 +415,7 @@ func jsonToBSONWithMappingAndObjectType(data map[string]any, idMapping map[strin Required: required, IsLinked: isLinked, DataSourceProperty: dataSourceProp, + SelectionTypes: SelectionTypesOf(data["ValueType"]), } if nestedObjectTypeID != "" { entry.ObjectTypeID = nestedObjectTypeID @@ -1087,6 +1088,25 @@ func ListAvailableTemplates() []string { return result } +// SelectionTypesOf reads a template ValueType's SelectionTypes — the selection +// kinds (None, Single, Multi) a Selection-typed property offers. The array is +// marker-prefixed like every Mendix list in a template ([1, "None", "Single"]), +// so only its strings count. Nil for a ValueType that has none. +func SelectionTypesOf(valueType any) []string { + vt, ok := valueType.(map[string]any) + if !ok { + return nil + } + items, _ := vt["SelectionTypes"].([]any) + var out []string + for _, it := range items { + if s, ok := it.(string); ok && s != "" { + out = append(out, s) + } + } + return out +} + // readPropertyTranslations reads a template ValueType's widget-shipped // Translations array (marker-prefixed CustomWidgets$WidgetTranslation entries) // into the property entry. Used to serialize a required TextTemplate the author