Claude/gracious planck 3h1lwi - #1117
Merged
Merged
Conversation
sdk/pages declared its own PropertyTypeIDEntry and PropertyTranslation with the same shape as the ones in mdl/types, so every hop between the widget template loader and the widget object builder went through a hand-written field copy: convertPropTypeIDs in mdl/backend/modelsdk, plus a second inline copy in BuildFilterWidget. A hand-written copy can only lose fields. Both copies omitted DataSourceProperty -- the widget template's own statement of which datasource a dependent property binds against (widget.xml's `dataSource="..."`). The loader reads it, neither copy forwarded it, and nothing failed: the widget engine simply never learned the link, and reconstructs it from .def.json mapping order instead. Re-export the mdl/types types as aliases and delete both copies. mdl/types has no internal dependencies, so this adds no import cycle; it is the same re-export pattern sdk/mpr already uses (CLAUDE.md: shared types live in mdl/types, other packages alias them rather than re-declare). No behaviour change on its own -- nothing reads DataSourceProperty yet. It is the prerequisite for binding a multi-datasource widget's dependent properties to the right entity (mendixlabs#1109). Tests: TestLoadWidgetTemplatePreservesDataSourceProperty asserts the link survives GetTemplateFullBSON -> LoadWidgetTemplate -> PropertyTypeIDs() for DatagridDropdownFilter (linkedDs + refOptions, populated simultaneously) and Combobox (association + database, exclusive by mode). Control: restoring convertPropTypeIDs while keeping the alias fails all eight links with DataSourceProperty = "", so the copy is demonstrably what dropped it. TestPropertyTypeIDEntryIsCanonicalType compares reflect.TypeOf rather than asserting assignability, because `var _ pages.X = types.X{}` still compiles for two identical-but-distinct structs and would not catch a re-declaration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
A pluggable widget may expose several datasources at once. DatagridDropdownFilter is the shipped case: `linkedDs` holds the grid's rows and `refOptions` the association target's option list, both populated, with `attr` and `refEntity` binding against the first and `refCaption`, `refCaptionExp` and `refSearchAttr` against the second. The engine had one entity to offer them: pageBuilder.entityContext, overwritten by each datasource mapping in turn. Every dependent therefore bound against whichever datasource mapping ran last, which is right only while a .def.json happens to interleave each datasource with its own dependents -- a convention nothing enforces and a regenerated definition can silently break. The symptom is an AttributeRef into the wrong entity: valid-looking BSON, clean mxcli check, CE1613 at build time. The widget already answers the question. widget.xml's `dataSource="..."` names, per property, which datasource it belongs to; it reaches us as PropertyTypeIDEntry.DataSourceProperty and became readable in the commit before this one. Record each datasource's resolved entity under its own property key and resolve a dependent's entity through entityContextFor(), which consults that link and otherwise returns the shared context. So the change is inert for every widget shipped today: a property with no DataSourceProperty -- which is all of them, on every single-datasource widget -- takes the fallback branch and behaves exactly as before. It does not yet make a multi-datasource widget authorable; naming a datasource by its schema key is still refused by MDL-WIDGET05 (mendixlabs#1109). Tests: TestResolveMapping_AttributeBindsToItsOwnDataSource drives `attr`, `refCaption` and `refSearchAttr` through resolveMapping with the REAL DatagridDropdownFilter template metadata -- hand-written links would only make the test agree with itself -- and asserts two different entities. Two controls, because neither alone is enough. In-test: TestResolveMapping_WithoutTemplateLinksBindsToSharedContext pins the old behaviour (refCaption -> the GRID's entity) and says so, so the first test cannot quietly stop proving anything. External: stubbing entityContextFor to return the shared context fails refCaption and refSearchAttr with exactly the reported symptom. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
A datasource-typed property can now be given under its own schema key, so a
widget exposing several datasources can be given each of them:
combobox cmbCustomer (
Association: Order_Customer,
optionsSourceAssociationDataSource: database from Module.Customer,
CaptionAttribute: Name
)
mendixlabs#643 closed a silent-drop defect by refusing the named form
outright, leaving the generic `datasource:` clause as the only spelling. That
clause says "the datasource", which a multi-source widget does not have -- this
restores the capability mendixlabs#643 deferred, without giving the defect back.
Four places assumed one datasource per widget, and all four had to move
together; three of them would each have been enough to leave the value dropped:
- datasourceTypedKeys rejected every named datasource-typed key regardless of
the VALUE. It now judges the value's shape: a real datasource is accepted, a
scalar is still MDL-WIDGET05. Aliases join the key set, or the same scalar
reaches the same drop under the other spelling.
- resolveMapping's DataSource case read only w.GetDataSource(). It now prefers
the value authored under this mapping's own key or alias.
- hasDataSource was widget-wide, so a named datasource selected the enumeration
mode and the value was dropped anyway. It now also consults the candidate
mode's own DATASOURCE mappings -- only those, because a microflow action and a
microflow datasource parse to the same AST shape. The new
`hasDataSource:<propertyKey>` says WHICH datasource selects a mode, for a
widget whose modes are exclusive by that (a ComboBox's association vs
database), where bare hasDataSource cannot tell them apart and mode order
silently decides.
- MDL-WIDGET16 read only the generic clause, so a ComboBox whose option list was
named got "this datasource is fine" from WIDGET05 and "there is no datasource"
from WIDGET16 at once -- and since exec refuses a script with errors, the page
could not be written at all. Found by an example fixture, not by the unit
tests.
The generic clause on a widget exposing several is now REFUSED, naming the keys.
It previously filled whichever datasource-typed property Go's map iteration
yielded first, so the same script produced different documents run to run with no
error either way. Both guesses are indefensible: fanning it out duplicates one
binding across unrelated slots, taking the first leaves the others unset (CE0642
against a property the author never mentioned).
No shipped definition declares two datasource mappings in one mode yet -- mxcli
models each widget as single-source-per-mode -- so this changes nothing for them.
It is the capability; the definitions are data that can follow, per widget, with
Studio Pro verification.
Verified end to end on a real project with a definition mapping both of a
DatagridDropdownFilter's datasources: both persisted, and the dependents stored
as NamedDS.Order.Number (attr -> linkedDs) and NamedDS.Customer.Name
(refCaption -> refOptions) -- two entities, from the widget package's own links.
Control: ignoring the named value, as before this change, fails
TestBuild_TwoNamedDataSources on all four assertions. mendixlabs#643's negative fixture
still passes, and TestMDLWIDGET05_JudgesTheValueNotTheKey pins both directions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
`describe page` rendered a pluggable widget with two configured datasources as if it had one, so describe -> exec dropped the second binding -- the round-trip half of mendixlabs#1109, and the last place the single-datasource assumption lived. Two separate causes, and fixing either alone leaves it broken: - The describer had one slot (rawWidget.DataSource) and seven emitter branches read it directly. They now go through appendWidgetDataSources, which prefers the named set, for the same reason they all went through appendDataSourceProp: a datasource must not render one way in a DataView and another in a Gallery (the drift mendixlabs#941 was). A branch left reading w.DataSource would quietly keep describing multi-source widgets as single-source. - The per-widget extractors each know about ONE of their widget's datasources -- the drop-down filter's reads `refOptions` and never `linkedDs` -- and they run before the generic fallback, whose `if widget.DataSource == nil` guard then skipped it entirely. Whether a widget is multi-source is a property of the stored document, not of which extractor happened to run, so the detection now runs unconditionally after them. Nothing shipped today describes differently. A widget with ONE configured source keeps the generic `DataSource:` clause, and the empty-reference filter does double duty: an unset second datasource parses to no reference and drops out of the count, so a genuinely single-source widget stays single-source. Verified: a ComboBox in association mode describes byte-identically. A source whose schema key cannot be resolved from the widget's PropertyTypes falls back to the unnamed spelling rather than being skipped. Skipping is mendixlabs#956's silent drop reached by a new route, and the key is exactly the part that cannot be relied on. Verified end to end: a DatagridDropdownFilter carrying both datasources now describes as `linkedDs: database from NamedDS.Order, refOptions: database from NamedDS.Customer, CaptionAttribute: Name`, and re-executing that output stores both entities plus the caption bound to the right one. What the round trip does not carry is `attr`, which mxcli already reports as MDL-WIDGET10 ("hidden when `baseType` is not \"attr\" -- the value will be ignored"): a value the widget ignores, so not describing it is correct. Control: reverting appendWidgetDataSources to emit w.DataSource fails both named-emission tests, the first showing exactly one entity where the widget has two. One trap worth the comment it now carries: adding `len(w.NamedDataSources) > 0` to the generic pluggable branch made it STEAL widgets from the filter branch further down, silently dropping CaptionAttribute. Branch order in this emitter is load-bearing -- widen the existing gate instead of adding a new one earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
Investigating whether the shipped DROPDOWNFILTER definition should map `linkedDs` alongside `refOptions` turned up the opposite answer: it must not, and mxcli had no way to know that. `linkedDs` is declared `isLinked="true"` in the widget's own widget.xml. A linked datasource is filled from the CONTAINING widget -- a Data Grid 2 supplies its column filter's list -- so it is not the author's to set. Three measurements on Mendix 11.6.6, each of which alone would have settled it: - Five Studio Pro-authored drop-down filters in testdata/expr-checker store `linkedDs` EMPTY (all in attribute mode, all inside a grid). - The shipped shape -- an association-mode filter in a Data Grid 2 column, no linkedDs -- passes `mx check` at 0 errors. - A filter written WITH linkedDs still fails CE0642 "Property 'Datasource to Filter' is required". mxbuild resolves the property from the parent rather than reading what is stored, so writing it is not merely useless. That third one is an inverted signal worth remembering: the build complains the property is MISSING while the value sits in the document, which reads as "write it harder" rather than "do not write it". So the definition is unchanged, and the reason is recorded in the mode's own description so the next person does not re-derive it. What is added is the guard: IsLinked now rides along in PropertyTypeIDEntry (it was in the template ValueType and dropped at the same boundary DataSourceProperty was), and a definition mapping a linked datasource is refused at build time. Refused, not skipped -- a silently ignored mapping is how an author concludes mxcli wrote it. Measured across every widget package in testdata/expr-checker: `linkedDs` is the ONLY linked datasource among the eight multi-datasource widgets. DROPDOWNFILTER is therefore single-source from MDL's side, while ComboBox (association vs database) and the six charts (static vs dynamic) are genuinely multi-source. That also retargets the mendixlabs#1109 multi-source tests, which used a DropdownFilter definition mapping both -- exactly the mistake this guard now catches. They use ComboBox's two authorable datasources instead, which is the honest exemplar. Control: dropping IsLinked from the entry (as it was before) makes both guard tests fail -- the refusal never fires and the template reads as unlinked. TestBuild_DropdownFilterWithoutLinkedMappingBuilds is the other side, so the refusal is pinned to that one property rather than to the widget or to having two datasource mappings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
The charts' `staticDataSource` and `dynamicDataSource` are already mapped, and have been: they are object-list ITEM properties inside the `lines`/`series` list, not widget-level ones, and GenerateDefJSON emits both from the .mpk together with every dependent's `dataSource` link. Nothing needed mapping. What was missing is that the links were never read. buildObjectListItem pre-resolves every datasource an item configures and dropped each resolved entity into the one shared pageBuilder.entityContext, so the LAST one won -- the item twin of the widget-level defect fixed for mendixlabs#1109. Measured on Mendix 11.6.6: a line chart series given a static source over SalesByRegion and a dynamic one over Forecast wrote its static x/y attributes as `CH.Forecast.Region` and `CH.Forecast.Total`, attributes that entity does not have, and mxbuild answered CE1613 "The selected attribute … no longer exists." twice. After the fix the same script writes Sales-side attributes against the static source and Forecast-side against the dynamic one, and the project checks at 0 errors. itemEntityContextFor reads ItemPropertyMapping.DataSource and falls back to the shared context, so a series configuring ONE datasource -- every series in every real chart, since `dataSet` selects static or dynamic -- resolves exactly as before. A two-series chart, one static and one dynamic, already worked and still does (verified end to end at 0 errors both before and after). Two things made this hard to see, both now in the finding. `mxcli check` only WARNS here (MDL-WIDGET10: the inactive set is hidden, "the value will be ignored"), and DESCRIBE renders the series correctly, so the wrong reference is visible only in the stored BSON or from mxbuild. And a recursive widget.xml scan makes static/dynamic look like widget-level properties, which is how they got proposed for widget-level mapping in the first place. Control: stubbing itemEntityContextFor to return the shared context fails the test with exactly the reported binding (`Sales.Customer.Number` for a static attribute). TestBuildObjectListItem_WithoutTheLinkTheLastDataSourceWins pins that old behaviour in-test, so the first test cannot quietly stop proving anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UTxHZd5s6mtZETDqgtpBK7
AI Code ReviewWhat Looks GoodThis PR effectively implements a fix for issue #1109 regarding named widget datasources for pluggable widgets. The changes are well-structured and address the core problem comprehensively:
RecommendationApprove this PR. It successfully implements a fix for issue #1109 with:
The changes are focused, well-documented, and follow the established patterns in the codebase. The fix enables the requested functionality (using named datasource properties for pluggable widgets with multiple datasources) while maintaining all existing behavior for single-source widgets. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.