From a904e745fc3233b45fdb920ae76ac5676282b913 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 08:16:14 +0000 Subject: [PATCH] feat(lint): CONV020 reports a created object never committed or passed on (mendixlabs/mxcli#1518) An object created with commit = No exists only in memory. If the microflow never commits it, returns it or hands it to anything that could, it is discarded when the microflow ends - and check, mx check and the build are all silent. In the field an after-startup seed microflow logged "Seeded 20" and the table had 0 rows. CONV020 (warning, category quality) walks each microflow's stored object graph via FullMicroflow, like CONV011. It reports a CreateObjectAction with an explicit Commit = No whose entity the catalog lists as persistent, when nothing else in the flow uses the output variable except to read a member ($o/Attr) or change it without commit. Every other mention counts as passing the object on: the walker scans every string of every object in the flow (loops and error-handler paths included) by reflection, so a field holding the variable name (commit, delete, rollback, page passed object, workflow context, export mapping source, ...) or a bare $o in any expression (return value, call arguments, list change, association value) keeps the rule quiet, including action types the rule does not know. It is flow-insensitive on purpose: a commit on one branch only is an accepted false negative. Nanoflows, rules, non-persistent entities, an unstored commit value and System/Marketplace modules are out of scope. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01W3x8Fyvir8kLyPgJs1BDmF --- .claude/commands/mendix/lint.md | 1 + .claude/skills/mendix/assess-quality/SKILL.md | 19 +- CHANGELOG.md | 1 + cmd/mxcli/cmd_lint.go | 6 +- docs-site/src/tools/builtin-rules.md | 27 ++ docs-site/src/tools/linting.md | 2 +- .../1518-created-object-never-persisted.mdl | 47 ++++ mdl/linter/report.go | 1 + .../rules/conv_created_never_persisted.go | 236 ++++++++++++++++++ .../conv_created_never_persisted_test.go | 186 ++++++++++++++ 10 files changed, 514 insertions(+), 12 deletions(-) create mode 100644 mdl-examples/bug-tests/1518-created-object-never-persisted.mdl create mode 100644 mdl/linter/rules/conv_created_never_persisted.go create mode 100644 mdl/linter/rules/conv_created_never_persisted_test.go diff --git a/.claude/commands/mendix/lint.md b/.claude/commands/mendix/lint.md index 6263a6896c..1953a781b6 100644 --- a/.claude/commands/mendix/lint.md +++ b/.claude/commands/mendix/lint.md @@ -49,6 +49,7 @@ mxcli lint -p app.mpr --exclude System --exclude Administration | CONV012 | quality | ExclusiveSplitCaption - Exclusive splits need meaningful captions | | CONV013 | quality | ErrorHandlingOnCalls - External calls (REST/WS/Java) need custom error handling | | CONV014 | quality | NoContinueErrorHandling - Don't silently swallow errors with Continue | +| CONV020 | quality | CreatedObjectNeverPersisted - A microflow creates a persistent object without commit and never commits, returns or passes it on, so it is discarded | ## Bundled Starlark Rules diff --git a/.claude/skills/mendix/assess-quality/SKILL.md b/.claude/skills/mendix/assess-quality/SKILL.md index e9ca03e1a0..c232305bed 100644 --- a/.claude/skills/mendix/assess-quality/SKILL.md +++ b/.claude/skills/mendix/assess-quality/SKILL.md @@ -12,14 +12,14 @@ description: "Audit a Mendix project against best practices for naming, security |---|---| | 29-36 | When to Use This Skill | | 37-96 | Assessment Workflow | -| 97-352 | Quality Guidelines Reference | -| 353-363 | Assessment Report Template | -| 364-376 | Executive Summary | -| 377-387 | project Metrics | -| 388-400 | Automated Findings | -| 401-420 | Manual Review Findings | -| 421-436 | Recommendations | -| 437-447 | Sources | +| 97-353 | Quality Guidelines Reference | +| 354-364 | Assessment Report Template | +| 365-377 | Executive Summary | +| 378-388 | project Metrics | +| 389-401 | Automated Findings | +| 402-421 | Manual Review Findings | +| 422-437 | Recommendations | +| 438-448 | Sources | # Assess Mendix Project Quality @@ -200,7 +200,7 @@ The Dutch Institute for Vulnerability Disclosure (DIVD) found widespread authori ### D. Maintainability -**Enforced by rules:** QUAL001-QUAL004, CONV009, CONV012, CONV014, MPR002, MPR004, MPR006 +**Enforced by rules:** QUAL001-QUAL004, CONV009, CONV012, CONV014, CONV020, MPR002, MPR004, MPR006 | Guideline | Rule/Check | Threshold | |-----------|-----------|-----------| @@ -211,6 +211,7 @@ The Dutch Institute for Vulnerability Disclosure (DIVD) found widespread authori | No empty microflows | MPR002 | Every microflow should have at least one activity | | Caption on exclusive splits | CONV012 | All decision points must have captions | | No "Continue" error handling | CONV014 | Never swallow errors silently | +| No created object silently discarded | CONV020 | A persistent object created without commit is committed, returned or passed on | | ACT_ microflows: thin controllers | CONV010 | Only page activities + submicroflow calls | **Manual checks:** diff --git a/CHANGELOG.md b/CHANGELOG.md index 86f6971661..591cd5ce44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Lint rule CONV020: an object created in a microflow and never committed, returned or passed on** (mendixlabs/mxcli#1518) — `$o = create Shop.Item (…)` without `commit` makes an object in memory only; if the microflow then never commits it or hands it to anything that could, the object is discarded when the microflow ends, and `check`, `mx check` and the build all pass. In the field an after-startup seed microflow logged "Seeded 20" and the table had 0 rows. CONV020 (warning) reports a create with commit No of a persistent entity in a microflow when nothing in the flow uses the variable except to read a member (`$o/Cnt`) or change it without commit. Any other mention — commit, change with commit, return, an argument to a microflow, Java/JavaScript action, workflow, mapping or page, a list, an association value, delete, rollback, or the bare variable in any expression — counts as passing it on. It is conservative by design: flow-insensitive (a commit on one branch or in an error handler counts), and nanoflows, non-persistent entities and System/Marketplace modules are not reported; anything it cannot classify keeps it quiet. Docs: "Built-in Rules" › CONV020 details. - **`layoutheader` and native layout parts — every Atlas layout copies whole** — `describe layout` → `create layout` of Atlas's Phone_ and Tablet_ layouts dropped their header bar (`Forms$Header`, printed as NOT re-executable), so pages bound to its `HeaderLeft` / `HeaderRight` placeholders failed CE1613 and a copy of Phone_Sidebar or Tablet_Sidebar failed CE0611 "A sidebar toggle button is required if a region can toggle". `layoutheader header1 { region left { … } region right { … } }` writes it. A native layout's copy silently lost its header placeholder, bottom bar and sidebar — pages bound to `Header` or a sidebar placeholder then failed CE1613; they are `region header { placeholder Header }`, `showbottombar: true` and `region sidebar { … }`, and the native content is written with the eight keys Studio Pro stores. Measured on a new 11.13.0 app: all 22 Atlas_Core layouts copied, with pages bound to the copied header and sidebar placeholders, build with 0 errors. - **`sidebartoggle` — Atlas's responsive sidebar toggle (`Forms$SidebarToggleButton`) in MDL** (mendixlabs/mxcli#1220) — copying `Atlas_Core.Atlas_Default` with `describe layout` → `create layout` dropped the button that opens the sidebar on phones and tablets: `describe` printed it as a NOT re-executable comment, and `exec` gave no warning. `sidebartoggle sidebarToggle3 (Tooltip: 'Toggle Menu', ButtonStyle: Primary, Icon: 'Atlas_Core.Atlas_Filled.navigation-menu', Class: 'toggle-btn')` takes an action button's caption, tooltip, style and icon, and `RenderMode: Link`; it has no action. The written widget has the keys of Atlas's own, including its caption under `CaptionTemplate` rather than the `Caption` key the vendored metamodel bound. A scroll container region's `ToggleMode` (`ToggleMode: 'ShrinkContentInitiallyClosed'`), which the toggle opens, is described and written too — without it the copy failed CE0628. - **`mxcli marketplace package` — export a module as a `.mpk`** (mendixlabs/mxcli#1382) — `mxcli marketplace package MyModule -p app.mpr [-o dir] [--all-libs] [--exclude REGEX] [--exclude-managed-dependencies] [--json]` is the reverse of `marketplace install --file`: it wraps Mendix's own `mx create-module-package` (resolved for the project's Mendix version), so the package is what Studio Pro's *Export module package* makes, and it installs back with `install --file`. By default only the module's own required `userlib/` JARs go in (`mx -l`; measured 12.5 KB against 24 MB with Business Events installed); `--all-libs` bundles every JAR. `-l` filters only JARs — mx still exports the app's whole `resources/` folder (measured: another module's `resources/EventLog/` came along) — so after the export mxcli opens the package, warns on stderr about each `resources//`, `javasource//` or `themesource//` file of another module, and prints the `--exclude` pattern that drops them in the one form mx accepts (`^resources.EventLog.`; a character class or backslash is rejected by mx with exit 1 and a usage message, and an exit 0 that wrote no package is reported as an error too). A module name that is not in the project lists the project's modules instead of failing with mx's exit 3, and mx's exit codes 1–4 are reported with what they mean and what mx printed. `mx create-module-package` writes into the project it is given — measured on a blank 11.12.5 app: `theme-cache/web/theme.compiled.css` and its map rewritten, `deployment/sass/main.scss` added, the module's `themesource/` folder created, model files untouched — so it runs on a temporary copy, as `docker check` does; a before/after listing of the project (sizes and modification times) was identical. The `mx` that runs must be the project's Mendix version: `ResolveMxForVersion` prefers an `mx` on PATH and falls back to any cached one, so mxcli reads the version of the `mx` it found (its `mx.deps.json`, else its version-named folder) and refuses a different major.minor.patch, or one it cannot identify, naming both versions and the fix (`mxcli setup mxbuild --version X`); `--mxbuild-path` is the override, with a warning on stderr. mx's own output on a successful export goes to stderr, cancelling the command stops mx, and an output directory inside the project is not copied into the temporary copy. Round trip on 11.12.5: export, `install --file` into a second fresh app, `mx check` — 0 errors. diff --git a/cmd/mxcli/cmd_lint.go b/cmd/mxcli/cmd_lint.go index fb145b0bcb..cbe03c8e6b 100644 --- a/cmd/mxcli/cmd_lint.go +++ b/cmd/mxcli/cmd_lint.go @@ -76,6 +76,7 @@ Bundled Starlark rules (in .claude/lint-rules/): - No validation rules (CONV015) - use microflows instead of entity validation rules - No event handlers (CONV016) - avoid entity event handlers - No calculated attributes (CONV017) - avoid calculated attributes + - Created object never persisted (CONV020) - a created object that is never committed, returned or passed on Custom Starlark rules in .claude/lint-rules/*.star are loaded automatically. @@ -432,7 +433,8 @@ func builtinLintRules() []linter.Rule { rules.NewExclusiveSplitCaptionRule(), rules.NewErrorHandlingOnCallsRule(), rules.NewNoContinueErrorHandlingRule(), - rules.NewIrreducibleFlowGraphRule(), // MDL-FLOW01 - graph structure vs MDL nesting - rules.NewImportRangeObjectMappingRule(), // MDL-MAP04 - First over an object-rooted mapping in the model + rules.NewCreatedObjectNeverPersistedRule(), // CONV020 - create without commit, never persisted or passed on + rules.NewIrreducibleFlowGraphRule(), // MDL-FLOW01 - graph structure vs MDL nesting + rules.NewImportRangeObjectMappingRule(), // MDL-MAP04 - First over an object-rooted mapping in the model } } diff --git a/docs-site/src/tools/builtin-rules.md b/docs-site/src/tools/builtin-rules.md index 55beb7343b..8a7f499bde 100644 --- a/docs-site/src/tools/builtin-rules.md +++ b/docs-site/src/tools/builtin-rules.md @@ -39,9 +39,36 @@ The **lint rules** below run with `mxcli lint`. There is also a separate group o | **CONV012** | Exclusive split captions -- Checks that decision branches have meaningful captions | | **CONV013** | Error handling on external calls -- Ensures external service calls have error handling | | **CONV014** | No continue error handling -- Warns against using CONTINUE error handling without logging | +| **CONV020** | Created object never persisted -- A microflow creates an object of a persistent entity without commit, and never commits, returns or passes it on, so the object is discarded when the microflow ends ([details](#conv020-details)) | | **MDL-FLOW01** | Un-describable branch structure -- Decision branches that re-enter each other's paths, so `DESCRIBE MICROFLOW` cannot render them as nested `IF`s without changing what they mean | | **MDL-MAP04** | First over an object-rooted import mapping -- An `import from mapping` or `rest call … returns mapping` activity already in the model stores Studio Pro's *First* (`ForceSingleOccurrence` or `Range.SingleObject`) over a mapping that returns one object. `mx check` reports 0 errors; the activity throws `key not found: Path(QName(None,),None,)` when it runs. `mxcli check` refuses the same statement in a script under the same ID; this finds activities written before that check existed, or with `exec --no-check`. A list-rooted mapping's *First* is legitimate and is not reported | +### CONV020 details {#conv020-details} + +`$Item = create Shop.Item (...)` without `commit` makes an object in memory only. +If the microflow then neither commits it nor hands it to anything that could, +the object is gone when the microflow ends. Nothing else reports this: `check`, +`mx check` and the build all pass, and the microflow runs without error. In the +report that prompted the rule, an after-startup seed microflow logged +"Seeded 20" and the table stayed empty. + +The rule reports a create only when **all** of these hold: + +- it is in a **microflow** (not a nanoflow or a rule) and its commit setting is + **No**; +- the entity is **persistent** (a non-persistent entity is meant to be + temporary), and is not a System or Marketplace entity; +- nothing in the microflow, on any path, uses the variable except to read one + of its members (`$Item/Name`) or to change it without commit. + +Any other mention counts as passing the object on and keeps the rule quiet: +`commit`, `change … commit`, returning it, an argument to a microflow, Java or +JavaScript action, workflow, mapping or page, adding it to a list, using it as +an association value on another object, `delete`, `rollback`, or the bare +variable anywhere in an expression. The check is flow-insensitive: a commit on +one branch only, or only in an error handler, counts, so that case is not +reported. When the rule cannot tell, it stays quiet. + ## Running Built-in Rules Built-in rules run automatically with `mxcli lint`: diff --git a/docs-site/src/tools/linting.md b/docs-site/src/tools/linting.md index 8f7dc0eb95..5b10e6e3c7 100644 --- a/docs-site/src/tools/linting.md +++ b/docs-site/src/tools/linting.md @@ -18,7 +18,7 @@ The linting system provides: |----------|--------|-------| | MDL | MDL001-MDL007 | Naming conventions, empty microflows, domain model size | | Security | SEC001-SEC010 | Access rules, password policy, demo users, PII exposure | -| Convention | CONV001-CONV019 | Best practice conventions, error handling | +| Convention | CONV001-CONV020 | Best practice conventions, error handling | | Quality | QUAL001-QUAL004 | Complexity, documentation, long microflows | | Architecture | ARCH001-ARCH003 | Cross-module data, entity business keys | | Design | DESIGN001 | Entity attribute count | diff --git a/mdl-examples/bug-tests/1518-created-object-never-persisted.mdl b/mdl-examples/bug-tests/1518-created-object-never-persisted.mdl new file mode 100644 index 0000000000..e1e910f19c --- /dev/null +++ b/mdl-examples/bug-tests/1518-created-object-never-persisted.mdl @@ -0,0 +1,47 @@ +mdl 1; +-- mendixlabs/mxcli#1518: an object created without commit and never committed, +-- returned or passed on is discarded when the microflow ends. check, mx check +-- and the build are silent; lint rule CONV020 reports it. +-- +-- mxcli exec 1518-created-object-never-persisted.mdl -p app.mpr +-- mxcli lint -p app.mpr -r CONV020 +-- +-- Expected: exactly one CONV020 warning, on Tmp1518.Seed. The other three are +-- controls: committed, a non-persistent entity, and a nanoflow. + +create module Tmp1518; + +create persistent entity Tmp1518.Item (Cnt: Integer); +create non-persistent entity Tmp1518.Helper (Cnt: Integer); + +-- Reported: $o is never committed, returned or passed on. +create microflow Tmp1518.Seed () returns Boolean as $b +begin + declare $b Boolean = true; + $o = create Tmp1518.Item (Cnt = 1); + log info node 'Seed' 'Seeded {1}' with ({1} = toString($o/Cnt)); + return $b; +end; + +-- Not reported: committed. +create microflow Tmp1518.SeedCommitted () returns Boolean as $b +begin + declare $b Boolean = true; + $o = create Tmp1518.Item (Cnt = 1); + commit $o; + return $b; +end; + +-- Not reported: a non-persistent entity is meant to be temporary. +create microflow Tmp1518.TempHelper () returns Boolean as $b +begin + declare $b Boolean = true; + $h = create Tmp1518.Helper (Cnt = 1); + return $b; +end; + +-- Not reported: nanoflows are out of scope. +create nanoflow Tmp1518.SeedInClient () +begin + $o = create Tmp1518.Item (Cnt = 1); +end; diff --git a/mdl/linter/report.go b/mdl/linter/report.go index a51eb28621..726f4b32d7 100644 --- a/mdl/linter/report.go +++ b/mdl/linter/report.go @@ -78,6 +78,7 @@ var categoryMapping = map[string]string{ "CONV015": "Quality", "CONV018": "Quality", "CONV019": "Quality", + "CONV020": "Quality", // Architecture "ARCH001": "Architecture", diff --git a/mdl/linter/rules/conv_created_never_persisted.go b/mdl/linter/rules/conv_created_never_persisted.go new file mode 100644 index 0000000000..af0a83eda3 --- /dev/null +++ b/mdl/linter/rules/conv_created_never_persisted.go @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "fmt" + "reflect" + "regexp" + "strings" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// CreatedObjectNeverPersistedRule flags a microflow that creates an object of a +// persistent entity without committing it and then does nothing that could +// persist it or hand it on: no commit, no return, no argument to any call, no +// association to another object, no list, no delete or rollback +// (mendixlabs/mxcli#1518). +// +// The object is discarded when the microflow ends. An after-startup seed +// microflow that logged "Seeded 20" while the table stayed empty is the case +// the rule exists for — check, mx check and the build are all silent on it. +// +// The rule is deliberately conservative: it reports only what it can prove. +// - Only an explicit Commit = No create of an entity the catalog knows to be +// persistent, in a microflow (a nanoflow's object lives in the client, and +// is commonly handed to a page through means the flow does not show). +// - It is flow-insensitive: a commit on any path, including an error +// handler, counts, so an object committed on one branch only is not +// reported (an accepted false negative). +// - Every string in every object of the flow is scanned. The only uses that +// do NOT count as passing the object on are reading a member of it +// ($Obj/Attr) and changing it without commit. Anything else that names +// the variable — a bare $Obj in any expression, or a field holding the +// variable name — counts as a use, including uses the rule does not know +// about. When in doubt it stays quiet. +type CreatedObjectNeverPersistedRule struct{} + +func NewCreatedObjectNeverPersistedRule() *CreatedObjectNeverPersistedRule { + return &CreatedObjectNeverPersistedRule{} +} + +func (r *CreatedObjectNeverPersistedRule) ID() string { return "CONV020" } +func (r *CreatedObjectNeverPersistedRule) Name() string { return "CreatedObjectNeverPersisted" } +func (r *CreatedObjectNeverPersistedRule) Category() string { return "quality" } +func (r *CreatedObjectNeverPersistedRule) DefaultSeverity() linter.Severity { + return linter.SeverityWarning +} + +func (r *CreatedObjectNeverPersistedRule) Description() string { + return "A microflow creates an object of a persistent entity without committing it, and never commits, returns, passes on, stores or deletes it" +} + +func (r *CreatedObjectNeverPersistedRule) Check(ctx *linter.LintContext) []linter.Violation { + if ctx.Reader() == nil { + return nil + } + + persistent := map[string]bool{} + for e := range ctx.Entities() { + if e.EntityType == "Persistent" { + persistent[strings.ToLower(e.QualifiedName)] = true + } + } + if len(persistent) == 0 { + return nil + } + isPersistent := func(qn string) bool { return persistent[strings.ToLower(qn)] } + + var violations []linter.Violation + for mf := range ctx.Microflows() { + if mf.MicroflowType != "MICROFLOW" || ctx.IsExcluded(mf.ModuleName) { + continue + } + full, err := ctx.FullMicroflow(model.ID(mf.ID)) + if err != nil || full == nil || full.ObjectCollection == nil { + continue + } + violations = append(violations, findUnpersistedCreates(full.ObjectCollection, mf, isPersistent, r)...) + } + return violations +} + +// findUnpersistedCreates reports each uncommitted create of a persistent +// entity in oc (loops included) whose output variable is never used in a way +// that could persist it or hand it on. +func findUnpersistedCreates(oc *microflows.MicroflowObjectCollection, mf linter.Microflow, + isPersistent func(string) bool, r *CreatedObjectNeverPersistedRule) []linter.Violation { + + var violations []linter.Violation + for _, create := range uncommittedCreates(oc.Objects) { + if create.OutputVariable == "" || !isPersistent(create.EntityQualifiedName) { + continue + } + if variableEscapes(oc, create) { + continue + } + violations = append(violations, linter.Violation{ + RuleID: r.ID(), + Severity: r.DefaultSeverity(), + Message: fmt.Sprintf("%s '%s.%s' creates $%s (%s) without committing it, and never commits, "+ + "returns or passes it on: the object is discarded when the microflow ends.", + mf.DocumentNounTitle(), mf.ModuleName, mf.Name, create.OutputVariable, create.EntityQualifiedName), + Location: linter.Location{ + Module: mf.ModuleName, + DocumentType: mf.DocumentNoun(), + DocumentName: mf.Name, + DocumentID: mf.ID, + }, + Suggestion: fmt.Sprintf("Commit $%s (or create it with commit), return it, or pass it on; "+ + "if it is only a temporary value, use a non-persistent entity.", create.OutputVariable), + }) + } + return violations +} + +// uncommittedCreates returns the create actions with an explicit Commit = No, +// at any loop depth. An empty commit value is unknown, not No, and is skipped. +func uncommittedCreates(objects []microflows.MicroflowObject) []*microflows.CreateObjectAction { + var out []*microflows.CreateObjectAction + for _, obj := range objects { + switch o := obj.(type) { + case *microflows.ActionActivity: + if c, ok := o.Action.(*microflows.CreateObjectAction); ok && c.Commit == microflows.CommitTypeNo { + out = append(out, c) + } + case *microflows.LoopedActivity: + if o.ObjectCollection != nil { + out = append(out, uncommittedCreates(o.ObjectCollection.Objects)...) + } + } + } + return out +} + +// variableEscapes reports whether anything in the flow, other than create +// itself, names create's output variable in a way that is not merely reading +// a member or changing it without commit. +func variableEscapes(oc *microflows.MicroflowObjectCollection, create *microflows.CreateObjectAction) bool { + name := create.OutputVariable + // $Name as a whole word, case-insensitively, and what follows it. + ref := regexp.MustCompile(`(?i)\$` + regexp.QuoteMeta(name) + `\b(\s*/)?`) + w := &escapeWalker{ + name: name, + ref: ref, + skip: create, + visited: map[uintptr]bool{}, + } + w.walk(reflect.ValueOf(oc)) + return w.escapes +} + +type escapeWalker struct { + name string + ref *regexp.Regexp + skip *microflows.CreateObjectAction + visited map[uintptr]bool + escapes bool +} + +func (w *escapeWalker) walk(v reflect.Value) { + if w.escapes || !v.IsValid() { + return + } + switch v.Kind() { + case reflect.Pointer: + if v.IsNil() { + return + } + p := v.Pointer() + if w.visited[p] { + return + } + w.visited[p] = true + if v.Type() == reflect.TypeOf(w.skip) && v.Interface().(*microflows.CreateObjectAction) == w.skip { + return + } + if ch, ok := v.Interface().(*microflows.ChangeObjectAction); ok && w.isUncommittedChangeOfVar(ch) { + // Changing the object without commit does not persist it; the + // new member values are still scanned (setting an association + // of the object to itself would be a use). + for _, c := range ch.Changes { + w.walk(reflect.ValueOf(c)) + } + return + } + w.walk(v.Elem()) + case reflect.Interface: + if !v.IsNil() { + w.walk(v.Elem()) + } + case reflect.Struct: + for i := 0; i < v.NumField(); i++ { + if v.Type().Field(i).IsExported() { + w.walk(v.Field(i)) + } + } + case reflect.Slice, reflect.Array: + if v.Type().Elem().Kind() == reflect.Uint8 { + return // raw bytes hold no expression + } + for i := 0; i < v.Len(); i++ { + w.walk(v.Index(i)) + } + case reflect.Map: + iter := v.MapRange() + for iter.Next() { + w.walk(iter.Key()) + w.walk(iter.Value()) + } + case reflect.String: + w.escapes = w.stringUses(v.String()) + } +} + +func (w *escapeWalker) isUncommittedChangeOfVar(ch *microflows.ChangeObjectAction) bool { + return strings.EqualFold(ch.ChangeVariable, w.name) && ch.Commit == microflows.CommitTypeNo +} + +// stringUses reports whether s uses the variable as anything other than the +// owner of a member being read. A field holding exactly the variable name is +// a variable-name field (commit, delete, rollback, loop source, retrieve over +// an association, a page's passed object...) and is a use. +func (w *escapeWalker) stringUses(s string) bool { + if strings.EqualFold(strings.TrimPrefix(strings.TrimSpace(s), "$"), w.name) { + return true + } + for _, m := range w.ref.FindAllStringSubmatch(s, -1) { + if m[1] == "" { // not followed by '/': the object itself + return true + } + } + return false +} diff --git a/mdl/linter/rules/conv_created_never_persisted_test.go b/mdl/linter/rules/conv_created_never_persisted_test.go new file mode 100644 index 0000000000..c6bac0e040 --- /dev/null +++ b/mdl/linter/rules/conv_created_never_persisted_test.go @@ -0,0 +1,186 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rules + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// mendixlabs/mxcli#1518: an after-startup seed microflow created its objects +// with commit = No and never committed, returned or passed them on. It logged +// "Seeded 20"; the table had 0 rows. check, mx check and the build said nothing. + +func act(a microflows.MicroflowAction) *microflows.ActionActivity { + return µflows.ActionActivity{Action: a} +} + +// createItem is the issue's `$o = create Tmp.Item (Cnt = 1)`. +func createItem(commit microflows.CommitType) *microflows.ActionActivity { + return act(µflows.CreateObjectAction{ + EntityQualifiedName: "Tmp.Item", + OutputVariable: "o", + Commit: commit, + InitialMembers: []*microflows.MemberChange{ + {AttributeQualifiedName: "Tmp.Item.Cnt", Type: microflows.MemberChangeTypeSet, Value: "1"}, + }, + }) +} + +func persistentTmp(qn string) bool { + return qn == "Tmp.Item" || qn == "Tmp.Other" +} + +func lintCreated(objects ...microflows.MicroflowObject) []linter.Violation { + oc := µflows.MicroflowObjectCollection{Objects: objects} + return findUnpersistedCreates(oc, testMicroflow(), persistentTmp, NewCreatedObjectNeverPersistedRule()) +} + +// The issue's repro: create, nothing else, return an unrelated Boolean. +func TestCreatedNeverPersisted_IssueRepro(t *testing.T) { + got := lintCreated( + µflows.StartEvent{}, + act(µflows.CreateVariableAction{VariableName: "b", DataType: µflows.BooleanType{}, InitialValue: "true"}), + createItem(microflows.CommitTypeNo), + µflows.EndEvent{ReturnValue: "$b"}, + ) + if len(got) != 1 { + t.Fatalf("want 1 violation, got %d: %+v", len(got), got) + } + v := got[0] + if v.RuleID != "CONV020" || v.Severity != linter.SeverityWarning { + t.Errorf("got %s/%s, want CONV020/warning", v.RuleID, v.Severity) + } + if !strings.Contains(v.Message, "$o") || !strings.Contains(v.Message, "Tmp.Item") { + t.Errorf("message %q should name the variable and the entity", v.Message) + } +} + +// Uses that do not persist or hand the object on: still reported. +func TestCreatedNeverPersisted_NonEscapingUsesStillFlagged(t *testing.T) { + cases := map[string][]microflows.MicroflowObject{ + "member read in a log message": { + act(µflows.LogMessageAction{LogNodeName: "'Seed'", TemplateParameters: []string{"toString($o/Cnt)"}}), + }, + "change without commit": { + act(µflows.ChangeObjectAction{ChangeVariable: "o", Commit: microflows.CommitTypeNo, + Changes: []*microflows.MemberChange{{AttributeQualifiedName: "Tmp.Item.Cnt", Value: "$o/Cnt + 1"}}}), + }, + "member read in a split": { + µflows.ExclusiveSplit{SplitCondition: µflows.ExpressionSplitCondition{Expression: "$o/Cnt > 0"}}, + }, + "member read in the return value": { + µflows.EndEvent{ReturnValue: "$o / Cnt > 0"}, + }, + "inside a loop": { + µflows.LoopedActivity{ + LoopSource: µflows.IterableList{ListVariableName: "Rows", VariableName: "Row"}, + ObjectCollection: µflows.MicroflowObjectCollection{Objects: []microflows.MicroflowObject{createItem(microflows.CommitTypeNo)}}, + }, + }, + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + objects := extra + if name != "inside a loop" { + objects = append([]microflows.MicroflowObject{createItem(microflows.CommitTypeNo)}, extra...) + } + if got := lintCreated(objects...); len(got) != 1 { + t.Errorf("want 1 violation, got %d: %+v", len(got), got) + } + }) + } +} + +// Every way the object can be persisted or handed on: not reported. +func TestCreatedNeverPersisted_EscapesAreNotFlagged(t *testing.T) { + cases := map[string]microflows.MicroflowObject{ + "commit": act(µflows.CommitObjectsAction{CommitVariable: "o", WithEvents: true}), + "change with commit": act(µflows.ChangeObjectAction{ChangeVariable: "o", Commit: microflows.CommitTypeYes}), + "change with commit, no evt": act(µflows.ChangeObjectAction{ChangeVariable: "o", Commit: microflows.CommitTypeYesWithoutEvents}), + "returned": µflows.EndEvent{ReturnValue: "$o"}, + "microflow argument": act(µflows.MicroflowCallAction{MicroflowCall: µflows.MicroflowCall{ + Microflow: "Tmp.SUB_Save", + ParameterMappings: []*microflows.MicroflowCallParameterMapping{{Parameter: "Tmp.SUB_Save.Item", Argument: "$o"}}}}), + "java action argument": act(µflows.JavaActionCallAction{JavaAction: "Tmp.JA_Save", + ParameterMappings: []*microflows.JavaActionParameterMapping{{Parameter: "Tmp.JA_Save.Item", + Value: µflows.BasicCodeActionParameterValue{Argument: "$o"}}}}), + "javascript action argument": act(µflows.JavaScriptActionCallAction{JavaScriptAction: "Tmp.JSA_Save", + ParameterMappings: []*microflows.JavaScriptActionParameterMapping{{Parameter: "Tmp.JSA_Save.Item", + Value: µflows.BasicCodeActionParameterValue{Argument: "$o"}}}}), + "workflow context": act(µflows.WorkflowCallAction{Workflow: "Tmp.WF", WorkflowContextVariable: "o"}), + "export mapping": act(µflows.ExportMappingCallAction{MappingID: "m1", SourceVariable: "o", OutputVariable: "json"}), + "show page argument": act(µflows.ShowPageAction{PageName: "Tmp.Item_Edit", PageParameterMappings: []*microflows.PageParameterMapping{{Parameter: "Tmp.Item_Edit.Item", Argument: "$o"}}}), + "show page passed obj": act(µflows.ShowPageAction{PageName: "Tmp.Item_Edit", PassedObject: "o"}), + "added to a list": act(µflows.ChangeListAction{ChangeVariable: "Items", Type: microflows.ChangeListTypeAdd, Value: "$o"}), + "association on another object": act(µflows.ChangeObjectAction{ChangeVariable: "Order", Commit: microflows.CommitTypeNo, + Changes: []*microflows.MemberChange{{AssociationQualifiedName: "Tmp.Order_Item", Type: microflows.MemberChangeTypeSet, Value: "$o"}}}), + "association value of a new object": act(µflows.CreateObjectAction{EntityQualifiedName: "Tmp.Other", OutputVariable: "p", + Commit: microflows.CommitTypeYes, + InitialMembers: []*microflows.MemberChange{{AssociationQualifiedName: "Tmp.Other_Item", Value: "$o"}}}), + "deleted": act(µflows.DeleteObjectAction{DeleteVariable: "o"}), + "rolled back": act(µflows.RollbackObjectAction{RollbackVariable: "o"}), + "bare in a split": µflows.ExclusiveSplit{SplitCondition: µflows.ExpressionSplitCondition{Expression: "$o != empty"}}, + "case differs": act(µflows.CommitObjectsAction{CommitVariable: "O"}), + } + for name, use := range cases { + t.Run(name, func(t *testing.T) { + if got := lintCreated(createItem(microflows.CommitTypeNo), use); len(got) != 0 { + t.Errorf("want no violation, got %+v", got) + } + }) + } +} + +// A commit reached only through an error handler, or on one branch only, is +// still a commit: the rule is flow-insensitive on purpose. +func TestCreatedNeverPersisted_ErrorHandlerCommitCounts(t *testing.T) { + create := createItem(microflows.CommitTypeNo) + failing := act(µflows.MicroflowCallAction{ErrorHandlingType: microflows.ErrorHandlingTypeCustom, + MicroflowCall: µflows.MicroflowCall{Microflow: "Tmp.SUB_Fails"}}) + handlerCommit := act(µflows.CommitObjectsAction{CommitVariable: "o"}) + if got := lintCreated(create, failing, handlerCommit, µflows.EndEvent{}); len(got) != 0 { + t.Errorf("want no violation, got %+v", got) + } +} + +// What the rule leaves alone by design. +func TestCreatedNeverPersisted_OutOfScope(t *testing.T) { + nonPersistent := act(µflows.CreateObjectAction{EntityQualifiedName: "Tmp.Helper", OutputVariable: "h", Commit: microflows.CommitTypeNo}) + unknownCommit := createItem("") + committed := createItem(microflows.CommitTypeYes) + noOutput := act(µflows.CreateObjectAction{EntityQualifiedName: "Tmp.Item", Commit: microflows.CommitTypeNo}) + for name, obj := range map[string]microflows.MicroflowObject{ + "non-persistent entity": nonPersistent, + "commit value not stored": unknownCommit, + "created with commit": committed, + "no output variable": noOutput, + } { + t.Run(name, func(t *testing.T) { + if got := lintCreated(obj); len(got) != 0 { + t.Errorf("want no violation, got %+v", got) + } + }) + } +} + +// Nanoflows and rules are skipped by Check, and without a reader it does nothing. +func TestCreatedNeverPersistedRule_NilReader(t *testing.T) { + if got := NewCreatedObjectNeverPersistedRule().Check(linter.NewLintContextFromDB(nil)); got != nil { + t.Errorf("want nil without a reader, got %v", got) + } +} + +// $o must not match $other or $o_2. +func TestCreatedNeverPersisted_WholeVariableNameOnly(t *testing.T) { + got := lintCreated(createItem(microflows.CommitTypeNo), + act(µflows.CommitObjectsAction{CommitVariable: "other"}), + µflows.EndEvent{ReturnValue: "$o_2"}, + ) + if len(got) != 1 { + t.Errorf("want 1 violation, got %d: %+v", len(got), got) + } +}