diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 64b254391..ec5445c7b 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -627,3 +627,4 @@ {"area":"mdl/executor","date":"2026-09-15","symptom":"Two check-time validators (validate_import_mapping_find.go, validate_offline_paths.go) imported sdk/mpr directly — which CLAUDE.md's backend-abstraction checklist forbids for the executor. After porting them to the backend, coverage of every ported function was 0.0%: offlineProfilesIn, projectEntityFacts and the new openProjectForValidation were never executed by the suite.","cause":"Both validators FAIL OPEN by design: an unreadable project returns nil and silences the rule rather than failing the check on something it could not inspect. The existing tests only exercised the pure helpers (offlinePathViolations) or passed an empty projectPath, so the reader half never ran.","file":"mdl/executor/validate_project_reader.go","fix":"Added openProjectForValidation as the package's one read-only connect, and tests that exercise all three against a real fixture. Coverage 0.0% -> 72%/78%/100%.","insight":"A FAIL-OPEN validator is the worst shape for an unverified port: break its reader and the rule stops firing, which looks exactly like a project the rule does not apply to. Nothing goes red. So for any fail-open code path, the test must assert the rule FIRES on a project that should trigger it — asserting it stays quiet proves nothing, since quiet is also the failure mode. Two setup details that decide whether such a test is real. (1) The fixture ships only an ONLINE navigation profile, so offlineProfilesIn returns empty on it either way; the test has to SEED an offline profile, and Mendix fixes the legal names (Responsive/Phone/Tablet + the *Offline variants) — an invented name is refused by the executor, which is how the first attempt failed. (2) Assert the control first: the stock fixture reports zero offline profiles, so a reader that invented one is caught before the positive assertion runs. Also worth noting the signature constraint that shaped the fix: ValidateProgram takes a project PATH, not a backend, because `check --references` validates a script against a project it never connects an executor to — so these open their own short-lived read-only connection rather than threading ctx.Backend through a public signature and every caller."} {"area":"mdl/executor","date":"2026-09-15","symptom":"TestRoundtripPage_MicroflowButtonWithCurrentObject failed on main and on every branch cut from it: 'Expected Target: $currentObject parameter mapping in describe output', while the printed output plainly contained the mapping as \"Target\": $currentObject. Unit tests were green; only the integration suite (-tags integration) caught it.","cause":"Not a describe regression at all. mdl/executor/identifier_quoting.go's mdlIdent quotes any identifier that does not LEX as a bare identifier, running the real ANTLR lexer. #476 (notify workflow ... TARGET) added `TARGET: T A R G E T;` to MDLLexer.g4, so the parameter named Target began lexing as a keyword token and DESCRIBE started quoting it. The output became MORE correct; the test's exact-substring assertion went stale.","file":"mdl/executor/roundtrip_page_test.go","fix":"Made the assertion quoting-agnostic (accepts Target: or \\\"Target\\\":). Controlled by renaming the expected parameter to a name that is absent, which still fails — so the assertion continues to detect a genuinely dropped mapping rather than passing on anything.","insight":"Adding a keyword to MDLLexer.g4 silently reformats DESCRIBE output for every existing element whose NAME matches that keyword, anywhere mdlIdent is used — the grammar change and the broken test are in different packages with no compile-time link, so nothing points from one to the other. When adding a token, grep the test tree for exact-substring assertions containing that word: here `grep -rn '\"Target: '` found the single collision in seconds, where reading the #476 diff never would have. The deeper rule is that an exact-substring assertion on DESCRIBE output encodes a quoting decision the test does not care about; assert the mapping quoting-agnostically, or re-parse the output, since what a roundtrip test means to check is that the mapping survived. Note the input side did NOT break: TARGET was added to the non-reserved-keyword rule, so scripts writing `Target:` unquoted still parse — which is why check-mdl's 544 scripts stayed green and only this one output assertion moved."} {"area": "mdl/executor", "date": "2026-09-15", "symptom": "check --references rejected a page that mxbuild builds at 0 errors: 'the constraint on Administration.Account names \"System.UserRoles\", which is neither an attribute nor an association of it'. Administration.Account extends System.User and System.UserRoles is declared from System.User, so it IS an association of it, by inheritance. Every inherited association was a false positive, and separately so was every cross-module one. Inherited ATTRIBUTES were fine throughout, which is what made it look like a narrow bug rather than a whole axis.", "cause": "associationTargetFrom matched the start entity against the association's two ends by exact equality and scanned only dm.Associations. Its doc comment said a specialisation deliberately returns false, 'the cost of being wrong is a false error on a working script' - sound while its only caller (resolveMemberOnEntity, typing an association retrieve) treated false as silence. The new XPath constraint checker's noteQualified then called the same helper and treated false as EVIDENCE, so the exact case the comment declined to chase became the finding. noteQualified did carry a three-valued guard, but it tested whether the BASE ENTITY was known - the wrong axis; Administration.Account is known.", "file": "mdl/executor/validate_member_refs.go", "fix": "Replaced the boolean with assocResolution (resolved / missing / notAnEnd / unknown). The start entity is matched through its generalization chain (generalizationChain, built on findEntityByQN), and a chain that could not be walked to its root yields assocUnknown = silence. associationEndsByName also reads dm.CrossAssociations, whose far end is held by NAME not by element ID. noteQualified reports only assocMissing and assocNotAnEnd.", "insight": "A helper whose contract is 'false when it cannot establish the answer' is safe only while every caller treats false as silence; the moment one caller treats it as evidence, the comment promising restraint becomes the specification of a false positive. A boolean cannot carry 'no' and 'don't know' to two callers that need to tell them apart - if the codebase already has a three-valued resolver next door (memberResolution, ten lines up), reusing the two-valued sibling is the smell. Two cheap guards would have caught it: a fixture entity with a GENERALIZATION carrying an association, and one CROSS-MODULE association - a fixture of flat single-module entities cannot distinguish a correct resolver from one comparing two names. Establish the verdict with the build, not by reading: mx check on the rejected page said 0 errors, which settles it in one run."} +{"area":"mdl/executor","date":"2026-09-16","symptom":"A pluggable widget with two or more independent named datasource properties could not be authored or round-tripped: MDL-WIDGET05 rejected real datasource expressions under those keys, generic DataSource fanned one source across all mappings, datasource-dependent modes ignored the named value, and DESCRIBE PAGE retained only one source.","cause":"resolveMapping and the hasDataSource mode condition only read w.GetDataSource and never inspected datasource mappings by property key or alias. The later explicit-property pass did not consider property keys and aliases consumed, so it could overwrite a correctly mapped attribute under the final datasource context. DESCRIBE used a first-source fallback with no place to retain property keys.","file":"mdl/executor/widget_engine.go, mdl/executor/validate_widgets.go, mdl/executor/cmd_pages_describe_pluggable.go","fix":"Resolve a structured DataSourceV3 by datasource mapping key or alias before the generic fallback; let hasDataSource inspect only the active mode's datasource mappings (not datasource-shaped named actions); exclude every mapped spelling from explicit fallback; allow only structured values in named datasource slots; collect and emit all named datasource keys when more than one is populated.","insight":"Datasource is not a widget-wide singleton once a schema exposes several datasource properties. Treat the mapping as the unit of context: select the mode from its datasource mappings, resolve its named source, update entityContext, then resolve dependent attributes. Inspecting arbitrary DataSourceV3 values is unsafe because named microflow and nanoflow actions share that parser shape. A generic convenience keyword may remain for a single-source widget, but DESCRIBE must switch to schema keys when collapsing would lose identity. The earlier #643 guard correctly rejected scalar lookalikes but encoded the deferred multi-source limitation as a permanent rule; validator rules should distinguish an unsupported value shape from an unsupported capability.","refs":["mendixlabs/mxcli#643","mendixlabs/mxcli#1109"]} diff --git a/.claude/skills/mendix/custom-widgets/SKILL.md b/.claude/skills/mendix/custom-widgets/SKILL.md index 9d0a446c2..f83fc5fa8 100644 --- a/.claude/skills/mendix/custom-widgets/SKILL.md +++ b/.claude/skills/mendix/custom-widgets/SKILL.md @@ -338,6 +338,32 @@ Object-list *item* action slots (chart series `staticOnClickAction`, popupmenu item `action`) have mappings generated but the engine still skips them at apply time. See upstream #956. +**Widgets with multiple datasources use each datasource property's own key.** +The friendly `DataSource:` spelling is for a widget with one logical source. +When a widget exposes independent lists, name them exactly as `describe widget` +does: + +```sql +multisource dashboard ( + primarySource: microflow Dashboard.DS_PrimaryRows, + primaryLabelAttribute: Label, + + secondarySource: microflow Dashboard.DS_SecondaryRows, + secondaryDateAttribute: OccurredAt, + + summarySource: database from Dashboard.SummaryRow, + summaryValueAttribute: Total +) +``` + +Each named property accepts the normal datasource expressions (`database`, +`microflow`, `nanoflow`, page parameter, selection, or association). Attribute +mappings resolve against the matching source's entity context; in `.def.json`, +put each datasource mapping before the attribute mappings that depend on it. +Do not replace the named properties with one generic `DataSource:`: that loses +which entity owns each attribute. `DESCRIBE PAGE` preserves the named form +whenever a widget has more than one populated datasource. + ### Step 2 -- Extract BSON template from Studio Pro The .def.json only describes mapping rules. The engine also needs a **template JSON** with the complete Type + Object BSON structure. @@ -512,7 +538,7 @@ Modes are evaluated in definition order -- first match wins. A mode with no `con | Source | Resolution logic | |--------|-----------------| | `attribute` | `w.GetAttribute()` -> `pageBuilder.resolveAttributePath()` | -| `datasource` | `w.GetDataSource()` -> `pageBuilder.buildDataSourceV3()` -> also updates `entityContext` | +| `datasource` | Named datasource property matching the mapping key/alias, otherwise `w.GetDataSource()` -> `pageBuilder.buildDataSourceV3()` -> also updates `entityContext` | | `association` | `w.GetAttribute()` -> `pageBuilder.resolveAssociationPath()` + uses current `entityContext` | | `selection` | `w.GetSelection()` or `mapping.Default` fallback | | `CaptionAttribute` | `w.GetStringProp("CaptionAttribute")` -> auto-prefixed with `entityContext` if relative | diff --git a/CHANGELOG.md b/CHANGELOG.md index c6110181a..81612f414 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Multi-source pluggable widgets can bind each datasource by its own property name.** Independent sources on one widget may return different entities, but the generic `DataSource:` spelling fanned one source across every datasource mapping, while the named form was rejected by MDL-WIDGET05 or overwritten during the explicit-property pass. Named datasource expressions now select datasource-dependent widget modes, resolve by mapping key or alias, retain their own entity context, and round-trip through `DESCRIBE PAGE`; scalar lookalikes remain an error. Single-source widgets keep the generic spelling. + - **`check --references` rejected every XPath constraint that hops an INHERITED or a CROSS-MODULE association** (ako/mxcli-sudoku FINDINGS #57). The constraint-member check added in `3aa2ee0e` reported the stock `Administration.Account_Overview` page — `Administration.Account extends System.User`, so `System.UserRoles` (declared from `System.User`) is an association of it by inheritance, and `mx check` on the rejected page says 0 errors. Because the false positive lands on a Marketplace module almost every app has, `check` stopped being usable as a gate for anyone whose entities inherit, which is the normal case for anything extending `System.User`, `System.Image` or `System.FileDocument`. The lookup matched the start entity against the association's two ends by exact equality and read only `dm.Associations`. Its comment said the specialisation case was deliberately not chased — "the cost of being wrong is a false error on a working script" — and that was sound while its only caller treated `false` as *silence*; the new check treated the same `false` as *evidence*, so the precise case the comment declined to chase became the finding. It is three-valued now (resolved / missing / not-an-end / unknown): the start entity is matched through its generalization chain, a cross-module association is found where it is actually stored (`CrossAssociations`, far end held by name), and a chain that could not be walked to its root is silence rather than a report. The check still fires on an association the entity genuinely lacks, including a specialisation's association named on its generalization. diff --git a/mdl-examples/bug-tests/mapping-1-named-widget-datasources.mdl b/mdl-examples/bug-tests/mapping-1-named-widget-datasources.mdl new file mode 100644 index 000000000..7678b9ce3 --- /dev/null +++ b/mdl-examples/bug-tests/mapping-1-named-widget-datasources.mdl @@ -0,0 +1,43 @@ +-- A datasource mapping may be addressed by its schema key. +-- +-- ComboBox is the public, embedded regression fixture. Its association mode is +-- selected by hasDataSource and maps optionsSourceAssociationDataSource to the +-- widget's datasource slot. Before the fix, the structured named expression +-- below was rejected by MDL-WIDGET05; allowing it without updating mode +-- selection would instead choose the enumeration mode and still drop it. +-- +-- The executor unit tests extend this same mapping behavior to a synthetic +-- widget with three simultaneously populated datasource mappings and prove +-- that DESCRIBE PAGE preserves all three schema keys. + +create module NamedDS; +create module role NamedDS.User; + +create persistent entity NamedDS.Customer ( + Name: string(200) +); + +create persistent entity NamedDS.Order ( + Number: string(80) +); + +create association NamedDS.Order_Customer + from NamedDS.Order to NamedDS.Customer; + +create or replace page NamedDS.Order_Edit ( + Title: 'Named datasource mapping', + Layout: Atlas_Core.Atlas_Default, + Params: { $Order: NamedDS.Order } +) { + dataview orderView (DataSource: $Order) { + pluggablewidget 'com.mendix.widget.web.combobox.Combobox' customer ( + optionsSourceAssociationDataSource: database from NamedDS.Customer, + Association: Order_Customer, + CaptionAttribute: Name + ) + } +} + +grant view on page NamedDS.Order_Edit to NamedDS.User; + +describe page NamedDS.Order_Edit; diff --git a/mdl/executor/cmd_pages_describe.go b/mdl/executor/cmd_pages_describe.go index e1c0f6c06..fe42240ad 100644 --- a/mdl/executor/cmd_pages_describe.go +++ b/mdl/executor/cmd_pages_describe.go @@ -609,20 +609,25 @@ type rawWidget struct { // named variants are spelled identically, so a reader that keeps the name // without the kind cannot avoid converting one into the other // (mendixlabs/mxcli#1059). Same split as types.NavMenuItem's. - Icon string // e.g. Atlas_Core.Atlas_Filled.pencil - IconType string // storage $Type, "" when the widget carries no icon - IconCode int // Forms$GlyphIcon's Code — the only identity a glyph icon has - Selection string // For Gallery selection mode (Single, Multi, None) - Class string // CSS class from Appearance - Style string // Inline CSS style from Appearance - DynamicClasses string // Dynamic-classes expression from Appearance - Parameters []string - Children []rawWidget - FilterWidgets []rawWidget // For Gallery filter widgets - ControlBar []rawWidget // For DataGrid2 CONTROLBAR widgets - Rows []rawWidgetRow - DataSource *rawDataSource - DataGridColumns []rawDataGridColumn // For DataGrid2 widgets + Icon string // e.g. Atlas_Core.Atlas_Filled.pencil + IconType string // storage $Type, "" when the widget carries no icon + IconCode int // Forms$GlyphIcon's Code — the only identity a glyph icon has + Selection string // For Gallery selection mode (Single, Multi, None) + Class string // CSS class from Appearance + Style string // Inline CSS style from Appearance + DynamicClasses string // Dynamic-classes expression from Appearance + Parameters []string + Children []rawWidget + FilterWidgets []rawWidget // For Gallery filter widgets + ControlBar []rawWidget // For DataGrid2 CONTROLBAR widgets + Rows []rawWidgetRow + DataSource *rawDataSource + // NamedDataSources preserves widgets that expose more than one datasource. + // A single datasource continues to use DataSource and its friendly MDL + // keyword; multiple sources must retain their schema property keys or a + // describe -> exec round trip would fan one source out over every mapping. + NamedDataSources []rawNamedDataSource + DataGridColumns []rawDataGridColumn // For DataGrid2 widgets // Input widget properties Editable string // "Always", "Never", "Conditional" ReadOnlyStyle string // "Inherit", "Control", "Text" @@ -724,6 +729,11 @@ type rawNamedAction struct { MDL string } +type rawNamedDataSource struct { + Key string + DataSource *rawDataSource +} + type rawExplicitProp struct { Key string Value string // attribute short name or primitive value diff --git a/mdl/executor/cmd_pages_describe_datasource.go b/mdl/executor/cmd_pages_describe_datasource.go index 0d776f8c0..758b5d717 100644 --- a/mdl/executor/cmd_pages_describe_datasource.go +++ b/mdl/executor/cmd_pages_describe_datasource.go @@ -309,6 +309,19 @@ func appendDataSourceProp(props []string, ds *rawDataSource) []string { return props } +func appendNamedDataSourceProps(props []string, sources []rawNamedDataSource) []string { + for _, source := range sources { + if expr := dataSourceExpr(source.DataSource); expr != "" { + props = append(props, fmt.Sprintf("%s: %s", source.Key, expr)) + continue + } + if comment := dataSourceComment(source.DataSource); comment != "" { + props = append(props, fmt.Sprintf("-- %s: %s", source.Key, strings.TrimPrefix(comment, "-- "))) + } + } + return props +} + // xpathConstraintClause renders a stored XPath constraint as the MDL the page // grammar accepts after WHERE, or "" when there is no constraint. // diff --git a/mdl/executor/cmd_pages_describe_named_datasource_test.go b/mdl/executor/cmd_pages_describe_named_datasource_test.go new file mode 100644 index 000000000..4a413964a --- /dev/null +++ b/mdl/executor/cmd_pages_describe_named_datasource_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func buildMultiSourceWidget() map[string]any { + propertyType := func(id, key string) map[string]any { + return map[string]any{"$ID": id, "PropertyKey": key} + } + property := func(id, flow string) map[string]any { + return map[string]any{ + "TypePointer": id, + "Value": map[string]any{"DataSource": map[string]any{ + "$Type": "Forms$MicroflowSource", + "Microflow": flow, + }}, + } + } + return map[string]any{ + "Type": map[string]any{ + "WidgetId": "com.example.multisource.MultiSource", + "ObjectType": map[string]any{"PropertyTypes": []any{ + propertyType("primary-id", "primarySource"), + propertyType("secondary-id", "secondarySource"), + propertyType("summary-id", "summarySource"), + }}, + }, + "Object": map[string]any{"Properties": []any{ + property("primary-id", "Demo.DS_Primary"), + property("secondary-id", "Demo.DS_Secondary"), + property("summary-id", "Demo.DS_Summary"), + }}, + } +} + +func TestNamedCustomWidgetDataSourcesPreservePropertyKeys(t *testing.T) { + sources := namedCustomWidgetDataSources(buildMultiSourceWidget()) + if len(sources) != 3 { + t.Fatalf("got %d sources, want 3: %#v", len(sources), sources) + } + for i, want := range []string{"primarySource", "secondarySource", "summarySource"} { + if sources[i].Key != want { + t.Errorf("source %d key = %q, want %q", i, sources[i].Key, want) + } + } +} + +func TestDescribeEmitsEveryNamedCustomWidgetDataSource(t *testing.T) { + var buf bytes.Buffer + ctx := &ExecContext{Output: &buf} + + outputWidgetMDLV3(ctx, rawWidget{ + Type: "CustomWidgets$CustomWidget", + RenderMode: "multisource", + Name: "dashboard", + WidgetID: "com.example.multisource.MultiSource", + NamedDataSources: namedCustomWidgetDataSources(buildMultiSourceWidget()), + }, 0) + + out := buf.String() + for _, want := range []string{ + "primarySource: microflow Demo.DS_Primary", + "secondarySource: microflow Demo.DS_Secondary", + "summarySource: microflow Demo.DS_Summary", + } { + if !strings.Contains(out, want) { + t.Errorf("description missing %q:\n%s", want, out) + } + } + if strings.Contains(out, "DataSource:") { + t.Errorf("multi-source widget was collapsed to generic DataSource:\n%s", out) + } + + page := "create page Demo.Showcase (Title: 'Demo') {\n" + out + "}\n" + if _, errs := visitor.Build(page); len(errs) > 0 { + t.Fatalf("named datasource DESCRIBE output is not valid MDL: %v\n%s", errs[0], page) + } +} diff --git a/mdl/executor/cmd_pages_describe_output.go b/mdl/executor/cmd_pages_describe_output.go index 5dc16b38d..0a0bb890d 100644 --- a/mdl/executor/cmd_pages_describe_output.go +++ b/mdl/executor/cmd_pages_describe_output.go @@ -661,7 +661,7 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { props = appendConditionalProps(props, w) props = appendAppearanceProps(props, w) formatWidgetProps(ctx.Output, prefix, header, props, "\n") - } else if (len(w.ExplicitProperties) > 0 || len(w.ObjectLists) > 0 || w.OnClick != "" || + } else if (len(w.ExplicitProperties) > 0 || len(w.NamedDataSources) > 0 || len(w.ObjectLists) > 0 || w.OnClick != "" || w.OnChange != "" || len(w.NamedActions) > 0) && w.WidgetID != "" { // Generic pluggable widget with explicit properties, object-list child // blocks (chart series/lines/scaleColors), and/or an onClick action. @@ -681,6 +681,7 @@ func outputWidgetMDLV3(ctx *ExecContext, w rawWidget, indent int) { // the datasource is gone. The DESCRIBE text was byte-identical before // and after, so only mx check separated them (#956). props = appendDataSourceProp(props, w.DataSource) + props = appendNamedDataSourceProps(props, w.NamedDataSources) for _, ep := range w.ExplicitProperties { props = append(props, fmt.Sprintf("%s: %s", ep.Key, explicitPropValue(ep))) } diff --git a/mdl/executor/cmd_pages_describe_parse.go b/mdl/executor/cmd_pages_describe_parse.go index 6d3cfbca7..b8551f55e 100644 --- a/mdl/executor/cmd_pages_describe_parse.go +++ b/mdl/executor/cmd_pages_describe_parse.go @@ -483,14 +483,18 @@ func parseRawWidget(ctx *ExecContext, w map[string]any, parentEntityContext ...s // (the action's parameter has no default once its datasource is gone), // while the DESCRIBE text was byte-identical before and after (#956). // - // anyCustomWidgetDataSource, not firstObjectPropertyDataSource: the latter - // stops at the first property whose DataSource parses at all, and a File - // Uploader has one carrying no reference ahead of its real one. + // Read every named source, skipping empty datasource-shaped properties. + // firstObjectPropertyDataSource cannot do that: it stops at the first + // property whose DataSource parses at all, and a File Uploader has one + // carrying no reference ahead of its real one. if widget.DataSource == nil { - if ds := anyCustomWidgetDataSource(w); ds != nil { - widget.DataSource = ds + named := namedCustomWidgetDataSources(w) + if len(named) > 1 { + widget.NamedDataSources = named + } else if len(named) == 1 { + widget.DataSource = named[0].DataSource if widget.EntityContext == "" { - widget.EntityContext = dataSourceEntityContext(ctx, ds) + widget.EntityContext = dataSourceEntityContext(ctx, named[0].DataSource) } } } diff --git a/mdl/executor/cmd_pages_describe_pluggable.go b/mdl/executor/cmd_pages_describe_pluggable.go index 28add25ef..3490d5353 100644 --- a/mdl/executor/cmd_pages_describe_pluggable.go +++ b/mdl/executor/cmd_pages_describe_pluggable.go @@ -1353,26 +1353,26 @@ func (e *Executor) extractCustomWidgetPropertyAssociation(w map[string]any, prop return extractCustomWidgetPropertyAssociation(e.newExecContext(context.Background()), w, propertyKey) } -// anyCustomWidgetDataSource returns the first datasource a pluggable widget's -// properties hold that names something — skipping any that parse to an empty -// reference. -// -// firstObjectPropertyDataSource is NOT a substitute, and the difference is the -// whole point: it returns as soon as a property's DataSource parses to a -// non-nil value, even one carrying no reference. A File Uploader has such a -// property ahead of its real one, so the caller received an empty datasource, -// discarded it, and described the widget as having none — which is exactly the -// silent drop this exists to prevent (#956). -func anyCustomWidgetDataSource(w map[string]any) *rawDataSource { +// namedCustomWidgetDataSources returns every usable datasource on a pluggable +// widget, retaining the widget schema's property key. The key is essential for +// multi-source widgets: `primarySource` and `secondarySource` may both be +// microflows, but their attribute mappings resolve against different entities. +func namedCustomWidgetDataSources(w map[string]any) []rawNamedDataSource { obj, ok := w["Object"].(map[string]any) if !ok { return nil } + propTypeKeyMap := buildPropertyTypeKeyMap(w, false) + var result []rawNamedDataSource for _, prop := range getBsonArrayElements(obj["Properties"]) { propMap, ok := prop.(map[string]any) if !ok { continue } + key := propTypeKeyMap[extractBinaryID(propMap["TypePointer"])] + if key == "" { + continue + } value, ok := propMap["Value"].(map[string]any) if !ok { continue @@ -1381,9 +1381,9 @@ func anyCustomWidgetDataSource(w map[string]any) *rawDataSource { if !ok || ds == nil { continue } - if result := parseDataSource(ds); result != nil && result.Reference != "" { - return result + if parsed := parseDataSource(ds); parsed != nil && (parsed.Reference != "" || parsed.Unsupported != "") { + result = append(result, rawNamedDataSource{Key: key, DataSource: parsed}) } } - return nil + return result } diff --git a/mdl/executor/validate_widgets.go b/mdl/executor/validate_widgets.go index 240b643d4..05611de6e 100644 --- a/mdl/executor/validate_widgets.go +++ b/mdl/executor/validate_widgets.go @@ -1106,17 +1106,21 @@ func validatePluggableWidgetProperties(w *ast.WidgetV3, registry *WidgetRegistry } lower := strings.ToLower(key) - // A datasource-typed property must be supplied via the widget's - // `datasource:` clause (which the engine reads), NOT as a named value - // like `optionsSourceAssociationDataSource: Module.Entity` — that lands - // in a different slot and is silently dropped, so the widget builds - // without an entity (CE0642). Flag it instead of passing it (issue #643). + // Named datasource properties are supported when the value parsed as a + // real datasource expression (`people: microflow M.DS_People`). A scalar + // that merely names an entity is still invalid: it cannot be persisted as + // a datasource and caused the silent drop/CE0642 from issue #643. if dsKeys[lower] { + if raw, ok := lookupProperty(w.Properties, key); ok { + if _, ok := raw.(*ast.DataSourceV3); ok { + continue + } + } out = append(out, linter.Violation{ RuleID: "MDL-WIDGET05", Severity: linter.SeverityError, Message: fmt.Sprintf( - "%s: widget `%s` (%s) property `%s` is datasource-typed — provide it via the widget `datasource:` clause (e.g. `datasource: database Module.Entity`); a value written as `%s: …` is not persisted", + "%s: widget `%s` (%s) property `%s` is datasource-typed — use a datasource expression such as `%s: database Module.Entity` or the generic `datasource:` clause; the supplied scalar value cannot be persisted", locationPrefix, w.Name, def.MDLName, key, key, ), }) @@ -1248,14 +1252,28 @@ func actionStorageKeys(def *WidgetDefinition) map[string]string { return out } -// These must be authored via the widget `datasource:` clause, not by name. +// Datasource properties may be authored by name when their value is a real +// *ast.DataSourceV3. The key set lets validation reject scalar lookalikes. +// +// A datasource mapping's MdlAliases are authorable spellings too (resolved by +// namedDataSourceValue in widget_engine.go exactly like the PropertyKey +// itself), so they belong in this set with the same lowercased normalization +// as the PropertyKey — otherwise a scalar written under an alias (e.g. +// `ItemsSource: 'x'`) skips the MDL-WIDGET05 datasource-typed check entirely +// and falls through to the generic property handling below. func datasourceTypedKeys(def *WidgetDefinition) map[string]bool { out := make(map[string]bool) collect := func(ms []PropertyMapping) { for _, m := range ms { - if m.Operation == "datasource" && m.PropertyKey != "" { + if m.Operation != "datasource" { + continue + } + if m.PropertyKey != "" { out[strings.ToLower(m.PropertyKey)] = true } + for _, alias := range m.MdlAliases { + out[strings.ToLower(alias)] = true + } } } collect(def.PropertyMappings) diff --git a/mdl/executor/validate_widgets_643_test.go b/mdl/executor/validate_widgets_643_test.go index ff216d619..99a4128cf 100644 --- a/mdl/executor/validate_widgets_643_test.go +++ b/mdl/executor/validate_widgets_643_test.go @@ -75,6 +75,52 @@ func TestIssue643_DatasourceClause_NotFlagged(t *testing.T) { } } +func TestNamedDatasourceExpression_NotFlagged(t *testing.T) { + reg := LoadWidgetRegistry("") + if reg == nil { + t.Fatal("built-in widget registry not available") + } + w := combo(map[string]any{ + "optionsSourceType": "association", + "optionsSourceAssociationDataSource": &ast.DataSourceV3{ + Type: "database", Reference: "Administration.Account", + }, + }) + for _, v := range validatePluggableWidgetProperties(w, reg, "page P") { + if v.RuleID == "MDL-WIDGET05" { + t.Errorf("named datasource expression must not trigger MDL-WIDGET05: %s", v.Message) + } + } +} + +// A datasource mapping's MdlAliases (e.g. ItemsSource) must be classified the +// same as its PropertyKey. Before this, datasourceTypedKeys only carried the +// PropertyKey, so a scalar value written under the alias skipped the +// datasource-typed check entirely and fell through to generic property +// handling instead of being flagged. +func TestDatasourceAlias_ScalarRejected(t *testing.T) { + def := &WidgetDefinition{ + WidgetID: "com.example.aliaswidget.AliasWidget", + MDLName: "aliaswidget", + PropertyMappings: []PropertyMapping{ + {PropertyKey: "primarySource", Source: "DataSource", Operation: "datasource", MdlAliases: []string{"ItemsSource"}}, + }, + } + reg := &WidgetRegistry{byWidgetID: map[string]*WidgetDefinition{def.WidgetID: def}} + + w := &ast.WidgetV3{ + Name: "aw", Type: "pluggablewidget", + Properties: map[string]any{ + "WidgetType": def.WidgetID, + "ItemsSource": "Module.Entity", + }, + } + got := ruleIDs(validatePluggableWidgetProperties(w, reg, "page P")) + if _, ok := got["MDL-WIDGET05"]; !ok { + t.Errorf("expected MDL-WIDGET05 for scalar value under alias ItemsSource, got rules: %v", keysOf(got)) + } +} + func keysOf(m map[string]string) []string { var ks []string for k := range m { diff --git a/mdl/executor/widget_engine.go b/mdl/executor/widget_engine.go index 546de4995..5a4f9476f 100644 --- a/mdl/executor/widget_engine.go +++ b/mdl/executor/widget_engine.go @@ -257,6 +257,14 @@ type PluggableWidgetEngine struct { // consults it to tell the widget's PRIMARY attribute property from its others // (#238); nil outside a build. currentDef *WidgetDefinition + + // currentModeDataSourceCount is how many PropertyMappings in the selected + // mode's mappings have Source == "DataSource". resolveMapping's "DataSource" + // case consults it to decide whether an unnamed generic `DataSource:` is a + // safe fallback for a mapping the script left unnamed: safe for a + // single-datasource widget, wrong for a multi-source one, where falling back + // would silently duplicate one datasource's binding into every unnamed slot. + currentModeDataSourceCount int } // NewPluggableWidgetEngine creates a new engine with the given backend and page builder. @@ -309,6 +317,15 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* return nil, err } + oldModeDataSourceCount := e.currentModeDataSourceCount + e.currentModeDataSourceCount = 0 + for _, m := range mappings { + if m.Source == "DataSource" { + e.currentModeDataSourceCount++ + } + } + defer func() { e.currentModeDataSourceCount = oldModeDataSourceCount }() + // 3. Apply property mappings. // // A property the widget's editorConfig hides under the current configuration @@ -503,17 +520,9 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* } // 4.6 Apply explicit properties (not covered by .def.json mappings) - mappedKeys := make(map[string]bool) - for _, m := range mappings { - if m.Source != "" { - mappedKeys[m.Source] = true - } - } - for _, s := range slots { - mappedKeys[s.MDLContainer] = true - } + mappedKeys := mappedWidgetPropertyNames(mappings, slots) for propName, propVal := range w.Properties { - if mappedKeys[propName] || isBuiltinPropName(propName) { + if mappedKeys[strings.ToLower(propName)] || isBuiltinPropName(propName) { continue } entry, ok := propertyTypeIDs[propName] @@ -609,6 +618,28 @@ func (e *PluggableWidgetEngine) Build(def *WidgetDefinition, w *ast.WidgetV3) (* return cw, nil } +// mappedWidgetPropertyNames returns every AST spelling consumed by the mapping +// pass. The explicit-property fallback must skip all of them or it will apply a +// named property twice, potentially under a different datasource context. +func mappedWidgetPropertyNames(mappings []PropertyMapping, slots []ChildSlotMapping) map[string]bool { + mapped := make(map[string]bool) + for _, mapping := range mappings { + if mapping.Source != "" { + mapped[strings.ToLower(mapping.Source)] = true + } + if mapping.PropertyKey != "" { + mapped[strings.ToLower(mapping.PropertyKey)] = true + } + for _, alias := range mapping.MdlAliases { + mapped[strings.ToLower(alias)] = true + } + } + for _, slot := range slots { + mapped[strings.ToLower(slot.MDLContainer)] = true + } + return mapped +} + // isPrimaryAttributeMapping reports whether mapping is the one a bare // `Attribute:` should fill: the FIRST attribute-typed mapping in the definition, // which is the widget's primary attribute (GenerateDefJSON preserves the widget @@ -877,7 +908,7 @@ func (e *PluggableWidgetEngine) selectMappings(def *WidgetDefinition, w *ast.Wid } continue } - if e.evaluateCondition(mode.Condition, w) { + if e.evaluateCondition(mode.Condition, w, mode.PropertyMappings) { return mode.PropertyMappings, mode.ChildSlots, nil } } @@ -893,10 +924,22 @@ func (e *PluggableWidgetEngine) selectMappings(def *WidgetDefinition, w *ast.Wid } // evaluateCondition checks a built-in condition string against the AST widget. -func (e *PluggableWidgetEngine) evaluateCondition(condition string, w *ast.WidgetV3) bool { +func (e *PluggableWidgetEngine) evaluateCondition(condition string, w *ast.WidgetV3, mappings []PropertyMapping) bool { switch { case condition == "hasDataSource": - return w.GetDataSource() != nil + if w.GetDataSource() != nil { + return true + } + // A mode that owns a named datasource mapping must activate when that + // property's structured datasource is present. Looking at every AST + // DataSourceV3 would misclassify named action slots: microflow/nanoflow + // actions share that parser shape and are converted only by their mapping. + for _, mapping := range mappings { + if strings.EqualFold(mapping.Operation, "datasource") && namedDataSourceValue(mapping, w) != nil { + return true + } + } + return false case condition == "hasAttribute": return w.GetAttribute() != "" case strings.HasPrefix(condition, "hasProp:"): @@ -924,6 +967,31 @@ func namedPropValue(mapping PropertyMapping, w *ast.WidgetV3) string { return "" } +// namedDataSourceValue returns a datasource authored using the widget schema's +// own property key (or one of its aliases), for example: +// +// primarySource: microflow Demo.DS_Primary +// secondarySource: microflow Demo.DS_Secondary +// +// Keeping this separate from namedPropValue is important: stringifyAny would +// turn the structured datasource AST into text and lose its source type, +// arguments and constraints. +func namedDataSourceValue(mapping PropertyMapping, w *ast.WidgetV3) *ast.DataSourceV3 { + if v, ok := lookupProperty(w.Properties, mapping.PropertyKey); ok { + if ds, ok := v.(*ast.DataSourceV3); ok { + return ds + } + } + for _, alias := range mapping.MdlAliases { + if v, ok := lookupProperty(w.Properties, alias); ok { + if ds, ok := v.(*ast.DataSourceV3); ok { + return ds + } + } + } + return nil +} + // resolveMapping resolves a PropertyMapping's source into a BuildContext. func (e *PluggableWidgetEngine) resolveMapping(mapping PropertyMapping, w *ast.WidgetV3) (*BuildContext, error) { ctx := &BuildContext{pageBuilder: e.pageBuilder} @@ -1019,7 +1087,18 @@ func (e *PluggableWidgetEngine) resolveMapping(mapping PropertyMapping, w *ast.W } case "DataSource": - if ds := w.GetDataSource(); ds != nil { + // A widget may expose several independently named datasources. Prefer the + // value authored for this mapping; keep the generic `datasource:` clause + // as the backward-compatible fallback, but only for single-datasource + // widgets. A mode with several DataSource mappings has no single + // datasource the generic clause could mean, so falling back there would + // silently copy one binding into every unnamed slot; each must be + // addressed by its own schema key instead. + ds := namedDataSourceValue(mapping, w) + if ds == nil && e.currentModeDataSourceCount <= 1 { + ds = w.GetDataSource() + } + if ds != nil { dataSource, entityName, err := e.pageBuilder.buildDataSourceV3(ds) if err != nil { return nil, mdlerrors.NewBackend("build datasource", err) diff --git a/mdl/executor/widget_engine_test.go b/mdl/executor/widget_engine_test.go index 7ac227b39..767a8cf94 100644 --- a/mdl/executor/widget_engine_test.go +++ b/mdl/executor/widget_engine_test.go @@ -158,6 +158,7 @@ func TestEvaluateCondition(t *testing.T) { name string condition string widget *ast.WidgetV3 + mappings []PropertyMapping expected bool }{ { @@ -170,6 +171,24 @@ func TestEvaluateCondition(t *testing.T) { }, expected: true, }, + { + name: "hasDataSource with named mapped datasource present", + condition: "hasDataSource", + widget: &ast.WidgetV3{Properties: map[string]any{ + "optionsSource": &ast.DataSourceV3{Type: "database", Reference: "Module.Entity"}, + }}, + mappings: []PropertyMapping{{PropertyKey: "optionsSource", Source: "DataSource", Operation: "datasource"}}, + expected: true, + }, + { + name: "hasDataSource ignores datasource-shaped named action", + condition: "hasDataSource", + widget: &ast.WidgetV3{Properties: map[string]any{ + "onComplete": &ast.DataSourceV3{Type: "microflow", Reference: "Module.ACT_Complete"}, + }}, + mappings: []PropertyMapping{{PropertyKey: "onComplete", Operation: "action"}}, + expected: false, + }, { name: "hasDataSource without datasource", condition: "hasDataSource", @@ -210,7 +229,7 @@ func TestEvaluateCondition(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - result := engine.evaluateCondition(tc.condition, tc.widget) + result := engine.evaluateCondition(tc.condition, tc.widget, tc.mappings) if result != tc.expected { t.Errorf("evaluateCondition(%q) = %v, want %v", tc.condition, result, tc.expected) } @@ -249,9 +268,12 @@ func TestSelectMappings_WithModes(t *testing.T) { def := &WidgetDefinition{ Modes: []WidgetMode{ { - Name: "association", - Condition: "hasDataSource", - PropertyMappings: []PropertyMapping{{PropertyKey: "assoc", Operation: "association"}}, + Name: "association", + Condition: "hasDataSource", + PropertyMappings: []PropertyMapping{ + {PropertyKey: "optionsSource", Source: "DataSource", Operation: "datasource"}, + {PropertyKey: "assoc", Operation: "association"}, + }, }, { Name: "default", @@ -270,11 +292,24 @@ func TestSelectMappings_WithModes(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if len(mappings) != 1 || mappings[0].PropertyKey != "assoc" { + if len(mappings) != 2 || mappings[0].PropertyKey != "optionsSource" { t.Errorf("expected association mode, got %v", mappings) } }) + t.Run("named datasource matches association mode", func(t *testing.T) { + w := &ast.WidgetV3{Properties: map[string]any{ + "optionsSource": &ast.DataSourceV3{Type: "database", Reference: "Module.Entity"}, + }} + mappings, _, err := engine.selectMappings(def, w) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(mappings) != 2 || mappings[0].PropertyKey != "optionsSource" { + t.Errorf("expected association mode for named datasource, got %v", mappings) + } + }) + t.Run("falls back to default mode", func(t *testing.T) { w := &ast.WidgetV3{Properties: map[string]any{}} mappings, _, err := engine.selectMappings(def, w) diff --git a/mdl/executor/widget_named_property_test.go b/mdl/executor/widget_named_property_test.go index ae57a810a..1acf06ebc 100644 --- a/mdl/executor/widget_named_property_test.go +++ b/mdl/executor/widget_named_property_test.go @@ -10,6 +10,48 @@ import ( "github.com/mendixlabs/mxcli/sdk/widgets/mpk" ) +func TestNamedDataSourceValue(t *testing.T) { + named := &ast.DataSourceV3{Type: "microflow", Reference: "Demo.DS_Primary"} + generic := &ast.DataSourceV3{Type: "microflow", Reference: "Demo.DS_Other"} + mapping := PropertyMapping{PropertyKey: "primarySource", Source: "DataSource", Operation: "datasource"} + w := &ast.WidgetV3{Properties: map[string]any{ + "PrimarySource": named, + "DataSource": generic, + }} + + if got := namedDataSourceValue(mapping, w); got != named { + t.Fatalf("namedDataSourceValue() = %#v, want the named datasource %#v", got, named) + } +} + +func TestNamedDataSourceValue_Alias(t *testing.T) { + named := &ast.DataSourceV3{Type: "database", Reference: "Demo.PrimaryRow"} + mapping := PropertyMapping{ + PropertyKey: "primarySource", Source: "DataSource", Operation: "datasource", MdlAliases: []string{"ItemsSource"}, + } + w := &ast.WidgetV3{Properties: map[string]any{"itemssource": named}} + + if got := namedDataSourceValue(mapping, w); got != named { + t.Fatalf("namedDataSourceValue() = %#v, want datasource supplied through alias", got) + } +} + +func TestMappedPropertyNamesAreCaseInsensitive(t *testing.T) { + mappings := []PropertyMapping{{ + PropertyKey: "primaryLabelAttribute", + Source: "Attribute", + Operation: "attribute", + MdlAliases: []string{"ItemLabel"}, + }} + + mapped := mappedWidgetPropertyNames(mappings, nil) + for _, name := range []string{"attribute", "primarylabelattribute", "itemlabel"} { + if !mapped[name] { + t.Errorf("mapped property set does not contain %q", name) + } + } +} + // namedPropValue routes a widget-level property to the right MDL keyword via its // PropertyKey or a registered alias (item 1b — PieChart/HeatMap bind several // attribute/texttemplate properties that the single generic `Attribute:` keyword @@ -55,6 +97,51 @@ func TestResolveMapping_NamedAttribute(t *testing.T) { } } +// A mode with several DataSource mappings has no single datasource the +// generic `DataSource:` clause could mean, so an unnamed slot must not fall +// back to it — that would silently copy one binding into every unnamed slot +// instead of requiring each to be addressed by its own schema key. +func TestResolveMapping_DataSourceFallback_SuppressedInMultiSourceMode(t *testing.T) { + generic := &ast.DataSourceV3{Type: "parameter", Reference: "P"} + pb := &pageBuilder{ + paramScope: map[string]model.ID{"P": model.ID("entity-id")}, + paramEntityNames: map[string]string{"P": "Demo.Entity"}, + } + engine := &PluggableWidgetEngine{pageBuilder: pb, currentModeDataSourceCount: 2} + mapping := PropertyMapping{PropertyKey: "secondarySource", Source: "DataSource", Operation: "datasource"} + w := &ast.WidgetV3{Properties: map[string]any{"DataSource": generic}} + + ctx, err := engine.resolveMapping(mapping, w) + if err != nil { + t.Fatalf("resolveMapping: %v", err) + } + if ctx.DataSource != nil { + t.Errorf("DataSource = %#v, want nil — an unnamed slot in a multi-source mode must not fall back to the generic clause", ctx.DataSource) + } +} + +// The single-datasource widget the fallback exists for still gets it: with at +// most one DataSource mapping in the mode, the generic `DataSource:` clause is +// unambiguous. +func TestResolveMapping_DataSourceFallback_AllowedInSingleSourceMode(t *testing.T) { + generic := &ast.DataSourceV3{Type: "parameter", Reference: "P"} + pb := &pageBuilder{ + paramScope: map[string]model.ID{"P": model.ID("entity-id")}, + paramEntityNames: map[string]string{"P": "Demo.Entity"}, + } + engine := &PluggableWidgetEngine{pageBuilder: pb, currentModeDataSourceCount: 1} + mapping := PropertyMapping{PropertyKey: "dataSource", Source: "DataSource", Operation: "datasource"} + w := &ast.WidgetV3{Properties: map[string]any{"DataSource": generic}} + + ctx, err := engine.resolveMapping(mapping, w) + if err != nil { + t.Fatalf("resolveMapping: %v", err) + } + if ctx.DataSource == nil { + t.Errorf("DataSource = nil, want the generic clause's datasource for a single-source widget") + } +} + // A widget-level texttemplate (PieChart seriesName) reads its named MDL value. func TestResolveMapping_NamedTextTemplate(t *testing.T) { engine := &PluggableWidgetEngine{pageBuilder: &pageBuilder{}}