From 968dc4fec02a67634397c813253c012a6193b425 Mon Sep 17 00:00:00 2001 From: Ako Date: Fri, 25 Sep 2026 14:03:40 +0000 Subject: [PATCH] fix(check): refuse bare bindings ALTER PAGE inserts where no entity is in scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `alter page … { insert after textBox1 { image … ImageUrlParams: [{1} = ImageB64] } }` on Feedback's ShareFeedback_Logo, whose data view is sourced by a nanoflow the project lacks, passed `check --references`; exec then wrote a bare AttributeRef and `mx check` could not load the project. A widget inserted outside every data container fails the same way (a text box's Attribute is dropped instead: CE7005). ALTER's entity context lives in the stored document, so `check --references` now opens it (read-only, as the ALTER … SET dry run already does) and resolves the insertion point's entity through alterEntityContext — lifted out of the INSERT/REPLACE exec paths so check and exec share it. With no entity, the bare bindings of the inserted widgets are reported via bindingsWithoutScope, the walk lifted out of CREATE PAGE's unscopedBindings (#678). Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + .../alter-page-unscoped-insert-bindings.mdl | 76 ++++++++ mdl/executor/cmd_alter_page.go | 52 +++--- mdl/executor/validate.go | 44 +++-- mdl/executor/validate_alter_unscoped.go | 143 +++++++++++++++ mdl/executor/validate_alter_unscoped_test.go | 173 ++++++++++++++++++ 6 files changed, 452 insertions(+), 37 deletions(-) create mode 100644 mdl-examples/bug-tests/alter-page-unscoped-insert-bindings.mdl create mode 100644 mdl/executor/validate_alter_unscoped.go create mode 100644 mdl/executor/validate_alter_unscoped_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index ed70b75f4..7cd8b9254 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -706,3 +706,4 @@ {"area": "mdl/executor", "symptom": "describe → exec of Feedback v4.0.2's EXCLUDED FeedbackModule.ShareFeedback_Logo refused `nanoflow not found: FeedbackModule.DS_FeedbackForm (data source)`; forcing it through left a project `mx check` could not LOAD (ArgumentNullException setting 'Attribute')", "cause": "The data view's flow is not in the project, so DESCRIBE had no context entity and printed every binding inside it bare (`Attribute: Subject`, `{1} = ImageB64`, `Visible: _showEmail in (…)`); exec had nothing to qualify them against, and a bare DomainModels$AttributeRef makes Mendix's loader throw. The stored model always had the full names", "file": "`mdl/executor/cmd_pages_describe_flowcontext.go` (`withQualifiedAttrs`, `describeAttr`), `mdl/executor/validate.go` (`unscopedBindings`), `mdl/executor/cmd_pages_builder_v3.go` (dangling DS flow kept by name), `mdl/backend/modelsdk/page_bare_attributeref.go` (`refuseBareAttributeRefs`)", "insight": "**The information was never lost — DESCRIBE threw it away**: every AttributeRef inside the unresolvable container still stored Module.Entity.Attr; shortening to the bare name is only safe where the reader can re-derive the entity, so key the shortening on whether the context resolved, not on habit. Measure the loader's tolerance before adding a write guard: 72 of 72 Studio Pro AttributeRefs in the project are qualified, so refusing a bare one refuses only writes that were already fatal — and turns a load-time stack trace into a statement-level error naming the attribute. Pair a blanket AST-level refusal with the exact failing slots (Attribute, CaptionAttribute, Visible-in, *Params) so the refusal names the widget, and keep the writer guard as the net for slots the walk does not know. Forced-fault run: hand-edit one qualified binding back to bare and confirm the refusal names it", "refs": ["ako/mxcli#675", "FeedbackModule.ShareFeedback_Logo"], "date": "2026-09-25"} {"area": "mdl/executor", "symptom": "describe → exec of Administration.Account_New fails `mx check` with [CE0642] \"Property 'Caption' is required.\" at Combo box 'comboBox2' (Account_Edit comboBox4 too); check and exec report success", "cause": "The ComboBox caption is an EXPRESSION (optionsSourceAssociationCaptionType=expression, optionsSourceAssociationCaptionExpression='$currentObject/Description'); describe read only the attribute caption, so the widget was rewritten with none. The write side already worked via the explicit-property pass, but MDL-WIDGET06 claimed both keys 'will be dropped'", "file": "`mdl/executor/cmd_pages_describe_parse.go` (combobox branch), `cmd_pages_describe_output.go`, `validate_widget_explicit_writable.go` (`persistedByExplicitPass`), `validate_widgets.go` (MDL-WIDGET06)", "insight": "**Resolve pluggable-widget properties by key before theorising**: a 20-line script mapping each Property's TypePointer to its PropertyKey through the widget's own Type.ObjectType settled the cause in one run, where the ndsl dump shows only anonymous values. Then TEST THE WRITE SIDE before building one: adding the two storage keys to the describe output by hand persisted both and built clean, which shrank the fix to describe + a false warning — no new syntax. A validator rule that asserts 'not persisted' must be tied to what the write path handles (here: the explicit pass writes Expression/TextTemplate/Attribute and scalar types), or it goes stale when the writer grows; the #643 test pinned the stale claim. A dedicated extractor for a 'known' pluggable widget silently drops every property it does not map — generic widgets emit them, known ones do not", "refs": ["ako/mxcli#664", "ako/mxcli#643"], "ce": ["CE0642"], "rules": ["MDL-WIDGET06"], "date": "2026-09-25"} {"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"]} 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) + } + }) + } +}