diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index 4a184b002..858e4f762 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -709,4 +709,5 @@ {"area": "mdl/executor", "symptom": "`alter styling … set 'Spacing bottom' = 'Outer medium'` (an Atlas Core 4.1.3 old name) warned MDL-WIDGET11 \"which no widget type in this project's theme declares — mxbuild reports this as CE6083\"; mxbuild actually reports CE6087 \"Design properties have been renamed in your theme\"", "cause": "validateAlterStylingDesignProps checked only current property names across all widget groups; #679 taught the authoring paths the theme's oldNames but not ALTER STYLING, which then offered a spelling near-miss instead of the current property", "file": "`mdl/executor/validate_alter_styling.go` (`renamedAnywhere`, `renamedStylingSuggestion`)", "insight": "**Pick the real-run example by what the resolver can see**: ALTER STYLING knows only a widget NAME, so it asks the whole theme — and Atlas 4.1.3 declares a CURRENT 'Align content' on the Image widget while 'Align content' is an old name on DivContainer, so that example is (correctly) silent under the under-report policy; 'Spacing bottom' is declared nowhere under a current name and exercises the rename path. A renamed key whose replacement is a compound (Spacing side, multi-select option) cannot be written by ALTER STYLING's one flat value, so the suggestion must point at the inline DesignProperties form rather than a `set` it cannot execute. Real run: old name via exec → CE6087; the suggested inline form → 0 errors", "refs": ["ako/mxcli#679"], "rules": ["MDL-WIDGET11"], "date": "2026-09-25"} {"area": "mdl/executor", "date": "2026-09-25", "symptom": "A `label` widget's DesignProperties were never checked: describe → check --references of FeedbackModule.ShareFeedback_Logo (Feedback v4.0.2, Atlas Core 4.1.3, Mendix 11.13.0) warned MDL-WIDGET11 'renamed' on the containers but not on label1's 'Spacing bottom': 'Outer none' (CE6087 in mxbuild). Same gap on the write side: `label l (DesignProperties: ['Style': '#ff0000'])` checked clean, exec'd, and failed mx check with [CE6085] \"Unknown option #ff0000 for design property Style.\" at Label", "cause": "The `label` keyword (PR #670, writes Forms$Label) was never added to mdlKeywordToDesignPropsKey, so resolveDesignPropsKey returned 'label' — no design-properties.json key. validateWidgetDesignProps skips a widget whose key is absent (meant for unknown pluggables), and the builder's GetPropertiesForWidget returned only the 'Widget' base group, so the Label's own ColorPicker 'Style' was unknown and a free colour fell to the Option default. bsonTypeToDesignPropsKey already had Forms$Label → Label; only the keyword half was missing", "file": "mdl/executor/theme_reader.go", "insight": "Adding a native widget keyword has a third registration nobody asks for: mdlKeywordToDesignPropsKey. Missing it is silent in both directions — the validator's 'no theme key → skip' rule turns an unmapped keyword into approval, and the builder still gets the Widget base group, so Spacing/Hide on write correctly and only the type-specific properties (Label's ColorPicker Style) mis-type. Test a type-specific property with an off-list value; a Widget-base property passes either way. Quick audit: a keyword should resolve to the same key as its stored $Type — groupbox (GroupBox, which has a ColorPicker Style), tabcontainer, navigationtree, menubar, simplemenubar, row/column (LayoutGridRow/Column) are still unmapped. Also seen: MDL-WIDGET12 warns on a ColorPicker free colour that the builder writes as Custom and mxbuild accepts (0 errors) — a pre-existing false positive, not changed here", "refs": ["#670", "#679"], "rules": ["MDL-WIDGET11", "MDL-WIDGET12"], "ce": ["CE6085", "CE6087"]} {"area": "mdl/executor", "date": "2026-09-25", "symptom": "`mxcli check` passed a microflow with a commit inside a loop \u2014 one database round trip per iteration \u2014 that `mxcli lint` already flagged as CONV011. The defect surfaced only at project-wide lint time, long after the write.", "cause": "CONV011 reads the STORED model, so it cannot speak until `exec` has written the microflow; `check` reads the MDL and had no equivalent rule. The gap is temporal, not a missing capability on either side.", "file": "`mdl/executor/validate_commit_in_loop.go` (MDL-PERF01, hooked in `validate_microflow.go`), test `validate_commit_in_loop_test.go`, example `mdl-examples/bug-tests/1186-commit-in-loop.mdl`", "insight": "**When adding a check-time rule that anticipates an existing lint rule, pin the BOUNDARY to the lint rule's, not to the better one, and say why in the code.** A `while true` is built as an ExclusiveMerge back-edge rather than a LoopedActivity, so CONV011 (which walks LoopedActivity) does not flag a commit inside one. A commit there is arguably still N+1, and the tempting move is to be more correct \u2014 but two rules for one concept that disagree on what counts is precisely how a pair drifts, and this repo already has CONV010's three successive short allowlists as the worked example. If the case is worth reporting it is worth reporting in BOTH, and the stored-model rule is the one that sees the built flow. The test that pins this carries a control on the control: `while true` is exempt, `while ` is not, so the exemption cannot silently become 'never flag a while'. Name the sibling rule in the message (`lint reports this as CONV011`) so a reader hitting one recognises the other rather than filing it twice. Also worth reusing: `checkReturnInLoop` already had the depth-tracking walk over Loop/While/If/EnumSplit/InheritanceSplit \u2014 copying its shape got the nesting cases right for free.", "refs": ["ako/mxcli#681", "mendixlabs/mxcli#1186"], "rules": ["MDL-PERF01"]} +{"area":"mdl/executor","date":"2026-09-25","symptom":"`alter page FeedbackModule.ShareFeedback_Logo { insert after textBox1 { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } }` passed `check --references`; exec wrote a bare AttributeRef and `mx check` could not LOAD the project (ArgumentNullException setting 'Attribute'). Same at page top level outside any data container; a text box's `Attribute:` there is silently dropped (CE7005).","cause":"ALTER's entity context comes from the STORED document (nearest enclosing data source, or a flow source's return type via resolveDataSourceFlowEntity). With the flow missing (Feedback v4.0.2 ships no DS_FeedbackForm) or no container at all, entityContext is \"\" and resolveAttributePath returns the bare name. CREATE PAGE refused this at check time (relaxExcludedWidgetRefs/unscopedBindings, #678); ALTER's check never opened the document, so nothing could know the scope.","file":"mdl/executor/validate_alter_unscoped.go, mdl/executor/cmd_alter_page.go (alterEntityContext), mdl/executor/validate.go (bindingsWithoutScope)","insight":"For ALTER, scope is a property of the stored document, not the statement: a check-time question about it must open the document (OpenPageForMutation, never Save; validate_alter_set.go already does this) and ask through the SAME function exec uses, so the INSERT/REPLACE entity resolution was lifted into alterEntityContext rather than restated, and the binding walk lifted out of unscopedBindings (bindingsWithoutScope) rather than copied. Controls that keep it from blocking working scripts: skip a target the stored doc lacks (added earlier in the script), a flow the script declares with an entity return (sc.flowParams), DataGrid2 column and list-view-template paths, and documents the script creates. Reproduce with a Studio Pro-authored page whose flow is genuinely absent.","refs":["#678","#685"]} {"area": "mdl/executor", "date": "2026-09-25", "symptom": "`describe page` on a File Uploader (files mode) emits `DataSource: association …`, and exec of that output fails: \"widget `upFiles` (fileuploader) exposes 2 datasources, so a generic `datasource:` clause is ambiguous — name the one you mean: associatedFiles, associatedImages\"", "cause": "DESCRIBE chose generic vs named by counting CONFIGURED datasources (namedCustomWidgetDataSources drops unset ones, so files mode = 1), while the builder's refuseAmbiguousGenericDataSource counts DECLARED datasource mappings (a generated def maps every top-level datasource = 2). Two sides of one round trip deciding the same question from different evidence.", "file": "mdl/executor/cmd_pages_describe_parse.go", "insight": "When describe and build each decide 'is this ambiguous?', they must count the same set. Fix read the DECLARED count from the stored schema (PropertyTypes with ValueType.Type=DataSource, excluding IsLinked) and excluded widgets with an embedded .def.json — those are hand-written and pick one datasource mapping per mode, so a database-mode ComboBox (two declared) must keep the generic clause. The #956 bug-test script itself authored the refused generic clause on a File Uploader: a bug-test that only runs `mxcli check` without a project can't see an exec-time refusal, so grep bug-tests for the old spelling whenever a builder starts refusing one.", "refs": ["mendixlabs/mxcli#1199", "mendixlabs/mxcli#956", "mendixlabs/mxcli#1109"], "rules": []} diff --git a/mdl-examples/bug-tests/alter-page-unscoped-insert-bindings.mdl b/mdl-examples/bug-tests/alter-page-unscoped-insert-bindings.mdl new file mode 100644 index 000000000..24c0891e8 --- /dev/null +++ b/mdl-examples/bug-tests/alter-page-unscoped-insert-bindings.mdl @@ -0,0 +1,76 @@ +-- ============================================================================ +-- ALTER PAGE INSERT/REPLACE where no entity is in scope: bindings qualified +-- ============================================================================ +-- +-- Symptom: on FeedbackModule.ShareFeedback_Logo (Feedback v4.0.2, Mendix +-- 11.13.0), whose data view is sourced by a nanoflow the module does not ship, +-- alter page FeedbackModule.ShareFeedback_Logo { insert after textBox1 { +-- image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } } +-- passed `mxcli check --references`, and exec wrote the parameter as a BARE +-- attribute reference: `mx check` could no longer LOAD the project +-- (ArgumentNullException setting 'Attribute'). The same happens to a widget +-- inserted outside every data container; a text box's `Attribute:` there is +-- dropped instead (CE7005 "No value selection has been made"). +-- +-- Cause: ALTER takes its entity from the STORED document (the enclosing data +-- source, or a flow source's return type). With none, the builder has nothing +-- to qualify a bare name against. CREATE PAGE already refused this at check +-- time; ALTER's check never opened the document. +-- +-- Fix: `check --references` opens the stored page, resolves the insertion +-- point's entity through the same function exec uses (alterEntityContext), and +-- refuses bare bindings when there is none, naming widget and binding. +-- +-- Verify: exec with -p → both ALTERs apply, `mxcli docker check` → 0 errors. +-- Change the qualified binding in the first ALTER to `{1} = Subject`, or +-- insert `textbox t (Attribute: Subject)` after ctTop (no data container) → +-- `check --references` reports it. The bare name in the second ALTER (inside +-- dvEntity, entity in scope) stays unflagged. +-- ============================================================================ + +create module BugTestAlterScope; +/ + +create persistent entity BugTestAlterScope.Draft ( + Subject: String(200) +); +/ + +@excluded +create or modify page BugTestAlterScope.Draft_Example +( Title: 'Draft (example)', Layout: Atlas_Core.Atlas_Default, + Params: { $Draft: BugTestAlterScope.Draft } ) +{ + container ctTop { + dynamictext txtIntro (Content: 'Example') + } + layoutgrid lg { + row r { + column c (DesktopWidth: AutoFill) { + dataview dvFlow (DataSource: nanoflow BugTestAlterScope.DS_MissingForm) { + textbox txtSubject (Label: 'Subject', Attribute: BugTestAlterScope.Draft.Subject) + } + dataview dvEntity (DataSource: $Draft) { + textbox txtSubject2 (Label: 'Subject', Attribute: Subject) + } + } + } + } +} +/ + +-- Inside the data view whose flow is missing: the binding must be qualified. +alter page BugTestAlterScope.Draft_Example { + insert after txtSubject { + dynamictext txtEcho (Content: 'About: {1}', ContentParams: [{1} = BugTestAlterScope.Draft.Subject]) + } +}; +/ + +-- Inside a data view whose entity is known: a bare name is fine. +alter page BugTestAlterScope.Draft_Example { + insert after txtSubject2 { + dynamictext txtEcho2 (Content: 'About: {1}', ContentParams: [{1} = Subject]) + } +}; +/ diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index 2af79fd09..6266ec845 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -349,19 +349,7 @@ func applyInsertWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op // target IS the container, so the children take the target's own context (e.g. // a dataview's entity). into := strings.EqualFold(op.Position, "INTO") - entityCtx := mutator.EnclosingEntity(op.Target.Widget) - if into { - entityCtx = mutator.EnclosingEntityForChildren(op.Target.Widget) - } - // A microflow/nanoflow datasource contributes no entity to the BSON walk (its - // entity is the flow's RETURN type), so resolve it via the model — otherwise a - // widget inserted into a flow-sourced list binds nothing (CE0402/CE1613). (#55) - if entityCtx == "" { - mfQN, nfQN := mutator.EnclosingDataSourceFlow(op.Target.Widget, into) - if e := resolveDataSourceFlowEntity(ctx, moduleName, moduleID, mfQN, nfQN); e != "" { - entityCtx = e - } - } + entityCtx, _ := alterEntityContext(ctx, mutator, op.Target.Widget, into, moduleName, moduleID) // Build new widgets from AST widgets, err := buildWidgetsFromAST(ctx, op.Widgets, moduleName, moduleID, entityCtx, mutator) @@ -435,14 +423,7 @@ func applyReplaceWidgetMutator(ctx *ExecContext, mutator backend.PageMutator, op } // Find entity context from enclosing DataView/DataGrid/ListView for regular widget replace. - entityCtx := mutator.EnclosingEntity(op.Target.Widget) - // Resolve a microflow/nanoflow datasource's return entity (see the INSERT path). - if entityCtx == "" { - mfQN, nfQN := mutator.EnclosingDataSourceFlow(op.Target.Widget, false) - if e := resolveDataSourceFlowEntity(ctx, moduleName, moduleID, mfQN, nfQN); e != "" { - entityCtx = e - } - } + entityCtx, _ := alterEntityContext(ctx, mutator, op.Target.Widget, false, moduleName, moduleID) // Build new widgets from AST, excluding the target widget/column from the // duplicate-name scope so a same-name replacement is allowed. @@ -578,6 +559,35 @@ func buildColumnSpecsFromAST(ctx *ExecContext, widgets []*ast.WidgetV3, moduleNa // Widget building from AST (domain logic stays in executor) // ============================================================================ +// alterEntityContext is the entity an INSERT or REPLACE builds its widgets +// against, plus the flow that was meant to supply it when a flow data source is +// where the scope comes from. forChildren is INSERT INTO: the target IS the +// container, so its own data source decides; otherwise (INSERT BEFORE/AFTER, +// REPLACE) the target is a sibling and the nearest ENCLOSING source does. +// +// A microflow/nanoflow datasource contributes no entity to the BSON walk (its +// entity is the flow's RETURN type), so it is resolved via the model — otherwise +// a widget inserted into a flow-sourced list binds nothing (CE0402/CE1613, #55). +// +// Shared with the check-time pass (validate_alter_unscoped.go), so check and exec +// cannot disagree about which entity is in scope. +func alterEntityContext(ctx *ExecContext, mutator backend.PageMutator, widgetRef string, forChildren bool, moduleName string, moduleID model.ID) (entity, flow string) { + if forChildren { + entity = mutator.EnclosingEntityForChildren(widgetRef) + } else { + entity = mutator.EnclosingEntity(widgetRef) + } + if entity != "" { + return entity, "" + } + mfQN, nfQN := mutator.EnclosingDataSourceFlow(widgetRef, forChildren) + flow = mfQN + if flow == "" { + flow = nfQN + } + return resolveDataSourceFlowEntity(ctx, moduleName, moduleID, mfQN, nfQN), flow +} + // resolveDataSourceFlowEntity resolves the entity context contributed by a // microflow/nanoflow datasource — its RETURN entity — for ALTER PAGE widget // builds. A flow datasource stores no entity in its own BSON (the entity lives diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index f1ebeee7d..01f9c5f1a 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -367,6 +367,11 @@ func validateProgramWithWarnings(ctx *ExecContext, prog *ast.Program) ([]error, // widget that is already stored, so its property can only be resolved // against the document — which is why it passed check and failed exec. errors = append(errors, validateAlterSetProperties(ctx, prog, sc)...) + // The entity an ALTER's INSERT / REPLACE binds against is in the stored + // document, not the statement. Where it has none (a missing flow source, or + // no data container at all) a bare binding is written bare and the project + // no longer loads — CREATE PAGE already refused this at check time. + errors = append(errors, validateAlterUnscopedBindings(ctx, prog, sc)...) return errors, sc.warnings } @@ -940,21 +945,6 @@ func (sc *scriptContext) relaxExcludedWidgetRefs(kind, name string, widgets []*a // the page writer's refusal of a bare attribute reference. func unscopedBindings(widgets []*ast.WidgetV3, ref string) []string { var out []string - var inScope func(ws []*ast.WidgetV3) - inScope = func(ws []*ast.WidgetV3) { - for _, w := range ws { - if w == nil { - continue - } - if _, own := w.Properties["DataSource"].(*ast.DataSourceV3); own { - continue // its own data source decides its children's scope - } - for _, b := range bareBindingsOf(w) { - out = append(out, fmt.Sprintf("%s `%s` (%s)", strings.ToLower(w.Type), w.Name, b)) - } - inScope(w.Children) - } - } var find func(ws []*ast.WidgetV3) find = func(ws []*ast.WidgetV3) { for _, w := range ws { @@ -966,7 +956,7 @@ func unscopedBindings(widgets []*ast.WidgetV3, ref string) []string { for _, b := range bareBindingsOf(w) { // the container's own bindings, e.g. its visibility out = append(out, fmt.Sprintf("%s `%s` (%s)", strings.ToLower(w.Type), w.Name, b)) } - inScope(w.Children) + out = append(out, bindingsWithoutScope(w.Children)...) continue } find(w.Children) @@ -976,6 +966,28 @@ func unscopedBindings(widgets []*ast.WidgetV3, ref string) []string { return out } +// bindingsWithoutScope names the bare bindings of widgets placed where no +// entity is in scope — the children of a container whose data source flow is +// missing, or widgets an ALTER inserts outside any container that resolves to +// an entity. Descent stops at a widget with a data source of its own, which +// scopes its children. +func bindingsWithoutScope(ws []*ast.WidgetV3) []string { + var out []string + for _, w := range ws { + if w == nil { + continue + } + if _, own := w.Properties["DataSource"].(*ast.DataSourceV3); own { + continue // its own data source decides its children's scope + } + for _, b := range bareBindingsOf(w) { + out = append(out, fmt.Sprintf("%s `%s` (%s)", strings.ToLower(w.Type), w.Name, b)) + } + out = append(out, bindingsWithoutScope(w.Children)...) + } + return out +} + // bareBindingsOf lists a widget's attribute bindings that need an entity in // scope to resolve. func bareBindingsOf(w *ast.WidgetV3) []string { diff --git a/mdl/executor/validate_alter_unscoped.go b/mdl/executor/validate_alter_unscoped.go new file mode 100644 index 000000000..68df997eb --- /dev/null +++ b/mdl/executor/validate_alter_unscoped.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/model" +) + +// A bare binding in a widget that ALTER PAGE / ALTER SNIPPET inserts where no +// entity is in scope passed `check --references` and was only caught — or not +// caught — at exec. +// +// The builder qualifies a bare `Attribute: X` or `{1} = X` with the entity of +// the insertion point, which exec reads from the STORED document: the nearest +// enclosing data source, or for a flow source the flow's return type. When there +// is none — the flow is missing, as FeedbackModule.DS_FeedbackForm is from +// Feedback v4.0.2's ShareFeedback_Logo, or the widget goes outside every data +// container — the binding is written bare. Measured on Mendix 11.13.0: an image +// URL parameter written that way left a project `mx check` could not LOAD +// (ArgumentNullException setting 'Attribute'); a text box's `Attribute:` was +// silently dropped and the build failed with CE7005 "No value selection has +// been made". +// +// CREATE PAGE catches the same shape at check time (relaxExcludedWidgetRefs, +// via unscopedBindings). ALTER could not, because its scope is not in the +// statement: it is in the document. So this pass opens the stored document and +// asks it the question exec asks, through the same function (alterEntityContext) +// — check and exec cannot disagree about which entity is in scope. + +// validateAlterUnscopedBindings reports the bare bindings of widgets an +// INSERT or REPLACE places where the stored document puts no entity in scope. +func validateAlterUnscopedBindings(ctx *ExecContext, prog *ast.Program, sc *scriptContext) []error { + if prog == nil || !ctx.Connected() { + return nil + } + h, err := getHierarchy(ctx) + if err != nil || h == nil { + return nil + } + opened := map[model.ID]backend.PageMutator{} + + var errs []error + for _, stmt := range prog.Statements { + s, ok := stmt.(*ast.AlterPageStmt) + if !ok || !hasWidgetBuildingOp(s) || alterTargetComesFromScript(sc, s) { + continue + } + unitID, containerID, containerType, err := resolveAlterPageUnit(ctx, s, h) + if err != nil { + continue // validateAlterTarget's finding + } + mutator, seen := opened[unitID] + if !seen { + // Read-only: the mutator is never saved. A document that will not + // open is silence, never a finding. + mutator, _ = ctx.Backend.OpenPageForMutation(unitID) + opened[unitID] = mutator + } + if mutator == nil { + continue + } + modName := h.GetModuleName(containerID) + label := fmt.Sprintf("alter %s %s", containerType, s.PageName.String()) + for _, op := range s.Operations { + var target ast.WidgetRef + var widgets []*ast.WidgetV3 + var into bool + switch o := op.(type) { + case *ast.InsertWidgetOp: + target, widgets, into = o.Target, o.Widgets, strings.EqualFold(o.Position, "INTO") + if allListViewTemplates(widgets) { + continue // built against the list view's own entity, on its own path + } + case *ast.ReplaceWidgetOp: + target, widgets = o.Target, o.NewWidgets + default: + continue + } + // DataGrid 2 columns take their own path, scoped by the grid. + if target.IsColumn() && allColumns(widgets) { + continue + } + // A target the stored document lacks is either added earlier in the + // script or refused by exec as not found; neither is this finding. + if !mutator.FindWidget(target.Widget) { + continue + } + if msg := unscopedInsertion(ctx, sc, mutator, target.Widget, into, modName, containerID, widgets); msg != "" { + errs = append(errs, mdlerrors.NewValidation(fmt.Sprintf("%s: %s", label, msg))) + } + } + } + return errs +} + +// unscopedInsertion returns the finding for one INSERT/REPLACE, or "". +func unscopedInsertion(ctx *ExecContext, sc *scriptContext, mutator backend.PageMutator, target string, into bool, + modName string, modID model.ID, widgets []*ast.WidgetV3) string { + entity, flow := alterEntityContext(ctx, mutator, target, into, modName, modID) + if entity != "" { + return "" + } + if flow != "" && sc != nil { + // A flow the script creates is not in the project yet; its declared + // return type is what exec will resolve against. + if sig, ok := sc.flowParams[strings.ToLower(flow)]; ok { + if sig.Returns != "" { + return "" + } + } + } + bare := bindingsWithoutScope(widgets) + if len(bare) == 0 { + return "" + } + where := fmt.Sprintf("the insertion point (`%s`) is in no data container", target) + if flow != "" { + where = fmt.Sprintf("the data container around `%s` is sourced by %s, which does not exist or returns no entity", target, flow) + } + return fmt.Sprintf("%s, so no entity is in scope and these bindings cannot be qualified: %s. "+ + "Written without an entity, a template parameter is stored bare (Mendix can no longer load the "+ + "project) and an attribute binding is dropped (CE7005). Qualify them (Module.Entity.Attribute), "+ + "or insert into a data container whose entity is known.", + where, strings.Join(bare, ", ")) +} + +// hasWidgetBuildingOp reports whether a statement carries an INSERT or REPLACE, +// so a script of pure SETs and DROPs never opens a document for this pass. +func hasWidgetBuildingOp(s *ast.AlterPageStmt) bool { + for _, op := range s.Operations { + switch op.(type) { + case *ast.InsertWidgetOp, *ast.ReplaceWidgetOp: + return true + } + } + return false +} diff --git a/mdl/executor/validate_alter_unscoped_test.go b/mdl/executor/validate_alter_unscoped_test.go new file mode 100644 index 000000000..de9c9093c --- /dev/null +++ b/mdl/executor/validate_alter_unscoped_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "go.mongodb.org/mongo-driver/bson" + + "github.com/mendixlabs/mxcli/mdl/backend" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/mdl/backend/pagemutator" + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/pages" +) + +// storedScopesPage is a page with the three insertion contexts that matter: +// +// - dvFlow: a data view sourced by a nanoflow the project does not contain — +// the shape of Feedback v4.0.2's ShareFeedback_Logo, whose +// FeedbackModule.DS_FeedbackForm is missing. Nothing puts an entity in scope. +// - dvEntity: a data view with a database source, so MyModule.Customer is in +// scope for its children. +// - tbTop: a widget at the top level, outside any data container. +func storedScopesPage() bson.D { + textbox := func(name string) bson.D { + return bson.D{{Key: "$Type", Value: "Forms$TextBox"}, {Key: "Name", Value: name}} + } + dvFlow := bson.D{ + {Key: "$Type", Value: "Forms$DataView"}, + {Key: "Name", Value: "dvFlow"}, + {Key: "DataSource", Value: bson.D{ + {Key: "$Type", Value: "Forms$NanoflowSource"}, + {Key: "Nanoflow", Value: "MyModule.DS_Missing"}, + }}, + {Key: "Widgets", Value: bson.A{int32(2), textbox("tbFlow")}}, + } + dvEntity := bson.D{ + {Key: "$Type", Value: "Forms$DataView"}, + {Key: "Name", Value: "dvEntity"}, + {Key: "DataSource", Value: bson.D{ + {Key: "$Type", Value: "Forms$DataViewSource"}, + {Key: "EntityRef", Value: bson.D{ + {Key: "$Type", Value: "DomainModels$DirectEntityRef"}, + {Key: "Entity", Value: "MyModule.Customer"}, + }}, + }}, + {Key: "Widgets", Value: bson.A{int32(2), textbox("tbEntity")}}, + } + return bson.D{ + {Key: "$Type", Value: "Forms$Page"}, + {Key: "FormCall", Value: bson.D{ + {Key: "Arguments", Value: bson.A{ + int32(2), + bson.D{{Key: "Widgets", Value: bson.A{int32(2), textbox("tbTop"), dvFlow, dvEntity}}}, + }}, + }}, + } +} + +func scopesPageCtx(t *testing.T) (*ExecContext, *countingDeps) { + t.Helper() + mod := mkModule("MyModule") + pg := mkPage(mod.ID, "P_Scopes") + deps := &countingDeps{} + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListFoldersFunc: func() ([]*types.FolderInfo, error) { return nil, nil }, + ListPagesFunc: func() ([]*pages.Page, error) { return []*pages.Page{pg}, nil }, + OpenPageForMutationFunc: func(unitID model.ID) (backend.PageMutator, error) { + return pagemutator.New(storedScopesPage(), unitID, deps), nil + }, + } + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(mkHierarchy(mod))) + return ctx, deps +} + +func checkAlterUnscoped(t *testing.T, ctx *ExecContext, src string) []error { + t.Helper() + prog := parseMDL(t, src) + sc := newScriptContext() + sc.collectDefinitions(prog) + return validateAlterUnscopedBindings(ctx, prog, sc) +} + +// TestAlterUnscoped_MissingFlowSource is the reported gap: an image inserted +// next to a widget inside a data view whose nanoflow is missing, with its URL +// parameter bound bare. check --references passed; exec wrote a bare +// AttributeRef and `mx check` could not LOAD the project. +func TestAlterUnscoped_MissingFlowSource(t *testing.T) { + ctx, deps := scopesPageCtx(t) + errs := checkAlterUnscoped(t, ctx, `alter page MyModule.P_Scopes { + insert after tbFlow { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } + }`) + if len(errs) != 1 { + t.Fatalf("got %d errors, want 1: %v", len(errs), errs) + } + msg := errs[0].Error() + for _, want := range []string{"MyModule.P_Scopes", "zzImg", "ImageB64", "MyModule.DS_Missing", "Module.Entity.Attribute"} { + if !strings.Contains(msg, want) { + t.Errorf("error %q does not name %q", msg, want) + } + } + if deps.saves != 0 { + t.Errorf("validation wrote to storage %d times, want 0", deps.saves) + } +} + +// TestAlterUnscoped_ReplaceAndInto — REPLACE resolves the sibling context and +// INSERT INTO the target's own; both land in the flow-sourced data view. +func TestAlterUnscoped_ReplaceAndInto(t *testing.T) { + ctx, _ := scopesPageCtx(t) + for _, src := range []string{ + `alter page MyModule.P_Scopes { replace tbFlow with { textbox tbNew (Attribute: Subject) } }`, + `alter page MyModule.P_Scopes { insert into dvFlow { textbox tbNew (Attribute: Subject) } }`, + } { + errs := checkAlterUnscoped(t, ctx, src) + if len(errs) != 1 || !strings.Contains(errs[0].Error(), "Attribute: Subject") { + t.Errorf("%s:\n got %v, want one error naming Attribute: Subject", src, errs) + } + } +} + +// TestAlterUnscoped_TopLevel — a widget inserted outside every data container +// has no entity either. Measured on 11.13.0: the image parameter was written +// bare (project unloadable) and a text box's attribute was silently dropped. +func TestAlterUnscoped_TopLevel(t *testing.T) { + ctx, _ := scopesPageCtx(t) + errs := checkAlterUnscoped(t, ctx, `alter page MyModule.P_Scopes { + insert before tbTop { image zzTop (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = FullName]) } + }`) + if len(errs) != 1 || !strings.Contains(errs[0].Error(), "FullName") || + !strings.Contains(errs[0].Error(), "no data container") { + t.Fatalf("got %v, want one error naming FullName and the missing container", errs) + } +} + +// --------------------------------------------------------------------------- +// Controls — a bare binding with an entity in scope is the ordinary case +// --------------------------------------------------------------------------- + +func TestAlterUnscoped_Controls(t *testing.T) { + cases := map[string]string{ + "entity in scope (sibling)": `alter page MyModule.P_Scopes { + insert after tbEntity { textbox t (Attribute: Name) } }`, + "entity in scope (into)": `alter page MyModule.P_Scopes { + insert into dvEntity { textbox t (Attribute: Name) } }`, + "qualified binding under the missing flow": `alter page MyModule.P_Scopes { + insert after tbFlow { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = MyModule.Feedback.ImageB64]) } }`, + "nested container scopes its own children": `alter page MyModule.P_Scopes { + insert before tbTop { dataview dvNew (DataSource: database MyModule.Customer) { textbox t (Attribute: Name) } } }`, + "page parameter path": `alter page MyModule.P_Scopes { + insert after tbFlow { dynamictext d (Content: '{1}', ContentParams: [{1} = $Customer/Name]) } }`, + "unknown target is someone else's finding": `alter page MyModule.P_Scopes { + insert after noSuchWidget { textbox t (Attribute: Name) } }`, + // The flow is created by the script: its declared return type is what + // exec will resolve against, so the bindings are in scope. + "flow defined in the script": `create nanoflow MyModule.DS_Missing () returns MyModule.Customer begin + return empty; end; + alter page MyModule.P_Scopes { insert after tbFlow { textbox t (Attribute: Name) } }`, + } + for name, src := range cases { + t.Run(name, func(t *testing.T) { + ctx, _ := scopesPageCtx(t) + if errs := checkAlterUnscoped(t, ctx, src); len(errs) != 0 { + t.Errorf("got %v, want none", errs) + } + }) + } +}