diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-bare-retrieve-association-empty-module.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-bare-retrieve-association-empty-module.json new file mode 100644 index 000000000..579cc03cc --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-bare-retrieve-association-empty-module.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-26", "symptom": "`retrieve $C from $O/Ord_Cust;` (association written by its short name) passed `check -p --references` and exec and was stored as the association `.Ord_Cust` (empty module): mx check CE1613 \"The selected association '.Ord_Cust' no longer exists.\", and describe printed `retrieve $C from $O/.Ord_Cust;`, which does not parse", "cause": "addRetrieveAction built the association name as `s.Source.Module + \".\" + s.Source.Name`; the visitor leaves Module empty for a bare name, and nothing on the check side looked at a bare retrieve association at all. MDL-RETRIEVE03's qualifyAssoc guessed the start entity's module instead — a second answer to the same question", "file": "`mdl/executor/bare_association.go` (new: resolveBareAssociation, associationCatalog, generalizationChainFor, the shared refusal text), `mdl/executor/cmd_microflows_builder_actions.go` (addRetrieveAction, qualifyBareRetrieveAssociation), `mdl/executor/validate_flow_member_refs.go` (new, check side), `mdl/executor/flow_member_spelling.go` (speller qualifies it), `mdl/executor/validate_retrieve_association_list.go` (qualifyAssoc)", "insight": "Resolve rather than refuse: exactly one association of the name with an end on the start entity's generalization chain (either end — a retrieve from the TO side is legal; a specialization inherits) is unambiguous, and every resolved shape built at 0 errors on 11.13.0, including `$Account/UserRoles` -> System.UserRoles. Refuse the rest, and never write the bare name: an empty module is CE1613 every time, so even 'cannot tell' (start entity unknown to the builder) is a refusal at exec, while check stays silent there. The resolver is a function of plain data (name, chain, catalog) so exec feeds it the project as earlier statements left it and check feeds project + associations declared BEFORE the flow — a later-declared one resolves at check time and not in exec's statement order. Resolving introduced a second spelling of the same member, so the describe speller (describedMemberSpellings) must qualify it too, or `create or modify` rewrote the flow on every re-run (asserted: 'Unchanged microflow')", "fix": "Bare retrieve associations resolve through resolveBareAssociation in the builder, the speller, MDL-RETRIEVE03 and check --references; missing/ambiguous are refused by check and exec with the qualified spelling; exec also refuses an unresolvable bare name rather than writing `.Name`", "refs": ["ako/mxcli#1690", "ako/mxcli#1399"], "ce": ["CE1613"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-expression-member-entity-lacks.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-expression-member-entity-lacks.json new file mode 100644 index 000000000..c3ded7f04 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-expression-member-entity-lacks.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-26", "symptom": "`toString($User/id)`, `$A/id = $B/id`, `$User/Bogus` in a microflow expression passed `check -p --references` (and the type pass's 'Expression types OK') and exec; mx check CE0117 \"Error(s) in expression.\" (mendixlabs/mxcli#1650)", "cause": "Nothing resolved the member steps of an expression's `$Var/...` path against the variable's entity: the expression type checker infers KindUnknown for an attribute it cannot find and stays silent, and the reference pass only judged CREATE/CHANGE members", "file": "`mdl/executor/validate_flow_member_refs.go` (expressionPath, associationHop, statementAttributePaths)", "insight": "Put it in the reference pass, not the type pass: exec's preflight runs ValidateProgramWithWarnings, not TypeCheckProgram, so only the former gives check and exec one verdict. Measured on 11.13.0 in an app with no other error (one CE1613 elsewhere suppressed all twelve CE0117s): CE0117 for `id` in any case, a wrong-case attribute, a capitalised `CreatedDate`, `changedDate` the root does not store, bare `owner`, a BARE association (`$O/Ord_Cust/Name`), a missing association, and a missing attribute after a hop; clean for inherited members incl. System.User's and `createdDate` on a specialization whose root stores it. Collect paths by reflecting over each statement's fields (skipping nested bodies, which the typed walk visits with their own scope, a retrieve's XPath and a feedback target) rather than a hand-kept switch of statement kinds. System.User is readable from the project here (modelsdk backend), so `$User/Bogus` is judged", "fix": "check --references (and so exec's preflight) reports a `$Var/` step that is not an attribute of the entity or its generalizations, a bare association, a missing association or one the entity is no end of; `id` gets the CommunityCommons.getGUID / compare-the-objects hint", "refs": ["ako/mxcli#1697", "mendixlabs/mxcli#1650"], "ce": ["CE0117"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-list-operation-member-entity-lacks.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-list-operation-member-entity-lacks.json new file mode 100644 index 000000000..64a1d745b --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-list-operation-member-entity-lacks.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-26", "symptom": "find / filter / sort / an aggregate by a member the list's entity does not have (`find $l by Numbr = '42'`, `sum $l by Totl`) passed `check -p` and exec; mx check CE1613 \"The selected attribute 'Shop.Order.Numbr' no longer exists.\" once per activity. Also: `sum $sl by Total` with Total INHERITED was written as `Shop.SubOrd.Total` (CE1613) though the member exists", "cause": "resolveMemberChange (reached through resolveListOperationMember), the sort branch and the aggregate all fell back to writing `.` when the name resolved to nothing; the aggregate never walked the generalization chain at all. check --references resolved CREATE/CHANGE members (#1048) and retrieve constraints (#1213) but no list-operation member", "file": "`mdl/executor/member_verdict.go` (new: memberVerdictOver, the one chain walk, with backendMemberModel; flowMemberVerdict; listMemberRefusal), `mdl/executor/cmd_microflows_builder_actions.go` (refuseMissingListMember in find/filter/sort/aggregate; aggregate qualifies with the declaring entity), `mdl/executor/validate_flow_member_refs.go` (scriptMemberModel, listMember), `mdl/executor/validate_member_refs.go` (resolveMemberOnEntity/nearMembers delegate)", "insight": "Make the builder refuse with the check's verdict and the check's words, instead of adding a check that predicts a builder which fabricates: one walk (memberVerdictOver) over a per-link interface, read from the backend by exec and from script-over-backend by check — the script overlay only changes how a LINK is read, never the walk, which is the duplicate-resolver-drift remedy taken literally. System dates are the trap: a sort reads `createdDate` where the root stores it (clean), but an aggregate cannot — `maximum $l by createdDate` is CE0226 'Entity ... does not contain attribute ...' even where stored. That was first measured as clean because the same app had a CE1613: **mx check reports neither CE0117 nor CE0226 while any CE1613 exists** — re-measure every control in an app with no other error", "fix": "find/filter by member, sort and aggregate refuse a bare member established as missing (exec without the preflight says exactly what check says, asserted); aggregates qualify an inherited attribute with its declaring entity; an aggregate over a system date is refused naming CE0226", "refs": ["ako/mxcli#1646", "ako/mxcli#1637", "ako/mxcli#1629"], "ce": ["CE1613", "CE0226"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-member-reusing-inherited-name-passes-check-ce0069.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-member-reusing-inherited-name-passes-check-ce0069.json new file mode 100644 index 000000000..36f9b96c9 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-member-reusing-inherited-name-passes-check-ce0069.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-26", "symptom": "A member that reuses a name already on the entity's generalization chain passed `check -p` and exec, and mxbuild rejected it as CE0069 \"Duplicate member name 'X'.\" — an attribute on a specialization named like an inherited attribute (`HR.Emp extends Other.Person (Name: …)`), a cross-module association `HR.Person_Dept from HR.Emp` beside the inherited `Other.Person_Dept`, `extends System.User (Name: …)` / `(UserRoles: …)`, `alter entity add attribute` of a name a specialization has, a rename into such a name, and an entity's own attribute and association of one name", "cause": "Nothing decided 'this entity already has a member of that name' along the chain. The only CE0069 guard was dropInheritedTargets inside bulk ALTER ENTITIES, which drops a child when its parent is also a target and does not look outside the target set; create entity, alter entity add/rename attribute, create association, rename association and `set owner both` checked only the entity's own attribute list or the module's association names", "file": "`mdl/executor/member_name_clash.go` (memberWorld, memberNameClash — the one verdict; refuseMemberNameClash in execCreateEntity, execAlterEntity add/rename, execAlterEntities before any write, execCreateAssociation, execAlterAssociation set owner both, execRenameAssociation; validateMemberNameClashes in the check -p reference pass; scriptMemberNameClashes as MDL-MEMBER01 in project-free ValidateProgram)", "insight": "Measure the rule's edges before writing it, in one app with no other error — 17 shapes in a single mx check run on 11.13.0 settled it: CE0069 covers attribute/attribute, association/association, attribute/association BOTH ways, two levels up, System ancestors' attributes AND associations (System.User.Name, System.UserRoles, System.FileDocument.Size), the DOWNWARD direction (generalization gains a specialization's member), renames, and an entity's own attribute vs own association. An association is a member of its FROM entity only — of its TO entity too under owner Both (reference and reference set); an attribute on the TO end of an owner-Default association does not collide. Names differing only in case do NOT collide, so the comparison is exact, unlike validateBusinessEventInheritedMembers. Check must replay the script in order over the project's members (create, alter add/drop/rename, create/drop/rename association, rename/move entity): judging against the stored model alone refuses a drop-then-re-add and misses the ordinary one-script case, and a child created before its lazily-resolved parent is only caught by also walking DOWN from the parent when it is created. A refusal reports only clashes the statement introduces (a re-run `create or modify` of an unchanged entity is not judged), and the bulk path is judged for every target before the first write so a refusal leaves nothing behind", "refs": ["ako/mxcli#1645"], "ce": ["CE0069"], "rules": ["MDL-MEMBER01"]} diff --git a/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-page-parameter-named-context-passes-check-ce7247.json b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-page-parameter-named-context-passes-check-ce7247.json new file mode 100644 index 000000000..1a4307a48 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-executor/2026-10-26-page-parameter-named-context-passes-check-ce7247.json @@ -0,0 +1 @@ +{"area": "mdl/executor", "date": "2026-10-26", "symptom": "A page parameter named `Context` (`Params: ( $Context: MyFirstModule.Sale )`) passed `check` and exec, and mx check reported CE7247 \"The name 'Context' is a reserved word.\" at Page parameter 'Context' — after the page and its callers were written. The same for a snippet, Java action or JavaScript action parameter named with any platform-reserved word (`Type`, `Id`, `Owner`, `CurrentUser`, a Java keyword such as `Class`), and for anything named `Object`", "cause": "The platform reserved-word list (types.PlatformReservedWords) was applied to attribute names (MDL021) and enumeration values (MDL010) only; no validator looked at parameter names. The list also lacked `object`, which is CE7247 everywhere the list applies", "file": "`mdl/executor/validate_reserved_parameter_names.go` (validateReservedParameterNames, MDL-PARAM01, wired into ValidateProgram; refuseReservedParameterNames in execCreatePageV3, execCreateSnippetV3, execAlterPage, execCreateJavaAction, execCreateJavaScriptAction), `mdl/types/reserved_words.go` (+`object`)", "insight": "Measure which list applies WHERE, before reusing one: ~120 names × 6 contexts in three mx check runs on 11.13.0. Pages, snippets, Java actions and JavaScript actions share the attribute list exactly (case-insensitive, quoted refused) — every listed name tried was CE7247, every unlisted one clean, except `Object`, which nobody had listed because it is not a Java keyword (also CE7247 on an attribute, an enumeration value and an ENTITY). Microflow and nanoflow parameters and variables do NOT use it — `$Context`, `$Type`, `$Class` build clean there — and are refused for a different set: the system variables (CE0110 \"Variable name 'latestError' is reserved.\" for currentObject/latestError/latestHttpResponse/latestSoapFault/currentSession/currentDeviceType, parameters and variables) and CE0039 for a variable named CurrentUser — not covered by this fix. Page VARIABLES named Context/Type/Object build clean too. Extending the shared list rather than adding a parallel one made MDL021/MDL010 pick up `Object` for free", "refs": ["ako/mxcli#1651", "mendixlabs/mxcli#1642"], "ce": ["CE7247"], "rules": ["MDL-PARAM01", "MDL021", "MDL010"]} diff --git a/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-26-xpath-constant-parse-error-and-silent-loss.json b/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-26-xpath-constant-parse-error-and-silent-loss.json new file mode 100644 index 000000000..63a3b34f3 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-grammar/2026-10-26-xpath-constant-parse-error-and-silent-loss.json @@ -0,0 +1 @@ +{"area": "mdl/grammar", "date": "2026-10-26", "symptom": "ako/mxcli#1685: `retrieve $Items from Shop.Item where [Total > @Shop.C_Limit];` was a parse error (`mismatched input 'Shop' expecting ']'`), read as 'MDL cannot write a constant in an XPath'. Worse, silent: a page data source written `where Total > @Shop.C_Limit` stored `[Total > ]`; describe of any stored constraint holding a constant on an entity with an enum attribute printed `where Total > @;`; and a long one written in the quoted form was laid out and STORED as `[Total > @]`", "cause": "No XPath rule for a constant: `@` (AT) was admitted by the negated xpathWord set as a one-word name. ParseXPathConstraint (error listeners off) recovered past the module name and returned ok with IdentifierExpr \"@\", so every re-render of the parse (enum enrichment, FormatXPathConstraint, check's member walk, which reported an attribute named \"@\") lost the constant; separately the page-side xpathExprToString had no ConstantRefExpr case and rendered \"\" for the expression-grammar form", "file": "mdl/grammar/domains/MDLPage.g4 (xpathConstantRef, AT out of xpathWord), mdl/visitor/visitor_xpath.go (buildXPathConstantRef), mdl/visitor/visitor_page_v3.go (xpathExprToString), mdl/executor/validate_xpath_constants.go (MDL-XPATH03), validate_user_role_tokens.go (walkXPathSlots)", "fix": "`@Module.Name` is an xpathValueExpr (a value, not a step) and AT is no longer a name word; the visitor builds ast.ConstantRefExpr and the page renderer prints it. check refuses a constant in any XPath slot (MDL-XPATH03, ValidateProgram, no project needed) with the variable workaround; excluded flows warn", "test": "mdl/visitor/visitor_xpath_constant_test.go, mdl/executor/validate_xpath_constants_test.go, mdl/executor/xpath_constant_1685_test.go (PedApp: stored verbatim in retrieve, list view and access rule, describe -> exec writes nothing, rename constant rewrites all three). Reverting the grammar fails every test with the reported parse error and `[Total > @]`; reverting only the renderer case gives `[Total > ]`", "insight": "The issue's premise was wrong and only a measurement showed it: mxbuild 11.13.0 rejects `@Mod.C` in a retrieve (Integer, String, Boolean constant), a list view data source, an access rule and a user task's targeting with CE0161, and an UNKNOWN constant is the same CE0161 rather than CE1613 (11.15.0 agrees for the retrieve) - XPath never resolves the name; the variable form is clean. So the right fix for this parse error was to parse it and REFUSE it with the workaround, not to store it. The Mendix runtime's MxPath grammar does have a CONSTANT token (com.mendix.languages.jar), which is why the syntax looks plausible. Access-rule XPaths are not validated by mx check at all while project security is Off - a garbage `[Total >]` passes - so measure access rules under Production security, with a bad-constraint control. A lenient parse that returns ok is not evidence it read everything: grep a renderer for `default: return \"\"` before trusting a new AST node to round-trip", "refs": ["ako/mxcli#1685", "ako/mxcli#1671"], "ce": ["CE0161"], "rules": ["MDL-XPATH03"]} diff --git a/.claude/skills/fix-issue/findings/mdl-visitor/2026-10-27-alter-workflow-annotation.json b/.claude/skills/fix-issue/findings/mdl-visitor/2026-10-27-alter-workflow-annotation.json new file mode 100644 index 000000000..784b57279 --- /dev/null +++ b/.claude/skills/fix-issue/findings/mdl-visitor/2026-10-27-alter-workflow-annotation.json @@ -0,0 +1 @@ +{"area": "mdl/visitor", "date": "2026-10-27", "symptom": "`alter workflow M.W { set (Annotation: '…'); }` was refused (\"a workflow has no property Annotation\"), and so was `set (Annotation: '…') on X`, though CREATE writes both notes (header `annotation '…'`, `@annotation '…'`); the refusal did not name the CREATE spellings, so an eval agent concluded workflow notes were unsupported. Written without the braces it was diagnosed as \"`set (annotation` is a session command\". Underneath: the header clause wrote the workflow's own Annotation, which Studio Pro shows nowhere, and describe never read the start activity's, so a Studio Pro note on the start was dropped by describe -> replay", "cause": "The ALTER key tables and the wfmutator setters were never given the notes CREATE had since ako/mxcli#707, and #707 put the start's note in Workflows$Workflow.Annotation: both it and WorkflowActivity.annotation are in the metamodel from 9.15.0, so the metamodel could not say which one Studio Pro shows. The braceless form: the old per-action grammar takes `set display '…'` unbraced, so the parser tried it, failed at `(`, and recovered into the session command `set = `", "file": "`mdl/visitor/visitor_alter_workflow.go` (keys), `mdl/backend/wfmutator/mutator.go` (`setStartNote`, `setNote`), `mdl/backend/modelsdk/workflow_read.go` (start activity's note), `sdk/workflows/workflow.go` (`StartNote`), `mdl/executor/cmd_workflows.go` + `cmd_workflows_write.go` (describe, CREATE), `mdl/visitor/visitor.go` (braces hint), `mdl/visitor/visitor_session_commands.go` (stand down on a failed line)", "fix": "The start's note is StartWorkflowActivity.Annotation: CREATE writes the header clause there, the reader reads it (and the workflow's own, for workflows written before), describe prints StartNote(). `Annotation` is an ALTER workflow key (the start's note; it also clears the workflow's own) and an activity key; `''` stores null; `on Start` is the workflow key; an end is refused. setNote writes {$ID, $Type: Workflows$Annotation, Description}, keeps an edited note's $ID, and refuses a document with no Annotation slot. The braceless line gets a hint showing the braced statement; the session-command refusal stands down on a line that already has a syntax error", "insight": "**When the metamodel has two slots for one concept, ask the editor which one it draws.** Workflow.annotation and the start activity's are both in the 9.15.0 metamodel; the answer was in mxbuild's own `ide-client/workflow-editor/editor.js` — four views render the note badge (ActivityView, BoundaryEventView, EventSubProcessView, StartActivityView), each from its own element, and the Workflow view renders none. Ten minutes of grep settled what #707 had to guess, and turned an ALTER feature into a describe data-loss fix: a Studio Pro start note was silently dropped by every describe -> replay. **A recovery-made statement's error outranks the real one in the reader's eyes** because it reads as a diagnosis: guard visitor errors on recovered trees with `syntaxErrorLines`. **A missing slot is information** — every type with a note stores `Annotation: null` when empty (11.15.0), so refuse rather than add one", "refs": ["ako/mxcli#1717", "ako/mxcli#707"]} diff --git a/.claude/skills/mendix/create-page/SKILL.md b/.claude/skills/mendix/create-page/SKILL.md index 767ba259d..3d5fdb32f 100644 --- a/.claude/skills/mendix/create-page/SKILL.md +++ b/.claude/skills/mendix/create-page/SKILL.md @@ -12,15 +12,15 @@ description: "CREATE PAGE syntax reference — parameters, variables, layouts, a |---|---| | 28-43 | Reference files | | 44-46 | Overview | -| 47-198 | Syntax | -| 199-269 | Basic Examples | -| 270-291 | Modifying Existing Pages | -| 292-395 | Conditional Visibility and Editability | -| 396-560 | Known Limitations | -| 561-569 | Tips | -| 570-596 | Related Commands | -| 597-621 | PLUGGABLEWIDGET Escape Hatch | -| 622-625 | See Also | +| 47-206 | Syntax | +| 207-277 | Basic Examples | +| 278-299 | Modifying Existing Pages | +| 300-403 | Conditional Visibility and Editability | +| 404-568 | Known Limitations | +| 569-577 | Tips | +| 578-604 | Related Commands | +| 605-629 | PLUGGABLEWIDGET Escape Hatch | +| 630-633 | See Also | # CREATE PAGE - MDL Syntax Guide @@ -63,6 +63,14 @@ create [or replace] page Module.PageName } ``` +**Parameter names are held to the platform's reserved words.** A page or snippet +parameter named `Context`, `Type`, `Id`, `Owner`, `Object`, `CurrentUser` or a Java +keyword (`Class`, `New`, …), in any case, is CE7247 "The name 'Context' is a reserved +word." at build time — and quoting does not help. `mxcli check` refuses it (MDL-PARAM01). +Name the parameter after its entity (`$Sale`) or qualify it (`$LaneContext`). The +system variable names (`$currentObject`, `$latestError`, …) are *not* on this list for +pages. + **Pop-up dimensions** (`PopupWidth` / `PopupHeight` / `PopupResizable`) apply when the page is opened in a pop-up. They are optional — omitting them uses the Mendix defaults (600 × 600, not resizable). Unlike the other header keywords, these property names are diff --git a/.claude/skills/mendix/generate-domain-model/reference/syntax.md b/.claude/skills/mendix/generate-domain-model/reference/syntax.md index 4568638a6..a6e89ef6c 100644 --- a/.claude/skills/mendix/generate-domain-model/reference/syntax.md +++ b/.claude/skills/mendix/generate-domain-model/reference/syntax.md @@ -316,6 +316,27 @@ access is out of date". The one exception is entities extending `System.User`, w inherited platform members Mendix manages and which must not be granted. See `manage-security`. +**Member names are unique along the chain.** An inherited member *is* a member of +the child, so the child cannot declare one of the same name — not an attribute, and +not an association it owns (the association's name is a member of its FROM entity, +and under `owner both` of its TO entity too). That holds up the chain to the System +entities (`System.User` has `Name` and the association `UserRoles`; +`System.FileDocument` has `Name` and `Size`) and down it: a generalization cannot +gain a member a specialization already declares. An entity's own attribute and +association cannot share a name either. Names that differ only in case do not +collide. `mxcli check` refuses each (MDL-MEMBER01); Mendix reports CE0069 +"Duplicate member name". + +```sql +mdl 1; +-- ❌ WRONG: Employee inherits Name from Person — CE0069 "Duplicate member name 'Name'." +create persistent entity Module.Person (Name: string(100)); +create persistent entity Module.Employee extends Module.Person (Name: string(100)); +``` + +Drop the child's copy and use the inherited one, or give it another name +(`DisplayName`). + #### System Attributes (Auditing) Mendix supports four built-in auditing properties on persistent entities. Declare them as regular attributes using pseudo-types (like `autonumber`): @@ -688,8 +709,9 @@ create persistent entity Module."VATRate" ( > quoted** (the check strips the quotes and still flags them): `Type` (CE7247, MDL021), > the audit attributes `CreatedDate` / `ChangedDate` / `Owner` / `ChangedBy` (MDL020 — use > the `AutoCreatedDate` / `AutoChangedDate` / `AutoOwner` / `AutoChangedBy` pseudo-types -> instead), plus `ID`, `GUID`, `CurrentUser` and the Java-keyword list. `"Type": String` -> fails MDL021 — rename to `ResourceType` / `TypeValue`. +> instead), plus `ID`, `GUID`, `CurrentUser`, `Object` and the Java-keyword list. `"Type": String` +> fails MDL021 — rename to `ResourceType` / `TypeValue`. The same list applies to page, +> snippet, Java action and JavaScript action **parameters** (MDL-PARAM01). > **Caveat 2 — a third grammar: OQL, where quoting DOES work.** `Year`, `Month`, > `Quarter`, `Week`, `Day`, `Hour`, `Minute`, `Second`, `Weekday`, `DayOfYear`, diff --git a/.claude/skills/mendix/write-microflows/reference/data-operations.md b/.claude/skills/mendix/write-microflows/reference/data-operations.md index dc2019090..d05991446 100644 --- a/.claude/skills/mendix/write-microflows/reference/data-operations.md +++ b/.claude/skills/mendix/write-microflows/reference/data-operations.md @@ -251,6 +251,14 @@ through to the build: idiom is written: inside `loop $item in $L`, `find $Others by Key = $item/Key` navigates the **loop's** variable on the right-hand side. +A `by` member, a `sort` key and an aggregate's attribute must be a member of the +list's entity or of an entity it extends. One that is not — a typo, a wrong case, +`owner` for `System.owner` — is refused by `check -p --references` and by exec, +naming the entity and what it has; it used to be written as the attribute +`Module.Entity.Typo` and fail the build with CE1613 (ako/mxcli#1646). A `sort` +may use the system dates `createdDate` / `changedDate` where the entity's root +stores them; an aggregate never can (`maximum $L by createdDate` is CE0226). + **`sort` is not an expression.** It takes attribute names directly — `sort $Orders by CreateDate desc` — and any word works as a name there, so an attribute called `Count` or `Date` needs no quotes. Writing `$currentObject/` @@ -404,9 +412,17 @@ end loop; it. Looping a single object passes `mxcli check` but produces an invalid project (mxbuild `StorageLoadException` — the loop's `change` writes an unqualified attribute). Loop only over the list-valued (reverse / ReferenceSet) form. -- The association is always fully qualified (`Module.Association`), and the start - variable is an object you already have (a parameter, a prior retrieve/create, or - a loop iterator). Both forms above are mxbuild-verified (0 errors). +- Write the association qualified (`Module.Association`); the start variable is + an object you already have (a parameter, a prior retrieve/create, or a loop + iterator). Both forms above are mxbuild-verified (0 errors). +- A **bare** association name (`retrieve $C from $O/Order_Customer;`) is resolved + against the start variable's entity — through its generalizations, from either + end — when exactly one association of that name fits, and is stored and + described qualified. One that fits no association, or two (two modules may + each have one of the name), is refused by `check -p --references` and by exec + with the qualified spelling to write; so is a bare name on a variable whose + entity exec cannot tell. It used to be stored as `.Order_Customer`, with an + empty module — CE1613, and a describe that does not parse (ako/mxcli#1690). **Enumeration attributes in WHERE**: XPath is a database query, so enum values are stored as plain strings. Both forms are valid — mxcli converts the qualified name to a string literal in BSON: @@ -453,6 +469,16 @@ set $Order/Shop.Order_Product = $Product; **Critical**: Always use fully qualified association names (`Module.AssociationName`). +`check -p --references` (and exec, which runs it first) resolves every +`$Var/` path against the entity the variable holds, through its +generalizations and across association hops, and reports what mxbuild would +reject with CE0117 "Error(s) in expression.": an attribute the entity does not +have (names are case-sensitive), a bare association (`$Order/Order_Customer/Name`), +an association that does not exist or does not start at the entity, and +`$Obj/id` — an object's id is not readable in an expression. Compare the objects +themselves (`$A = $B`), or read a GUID with `CommunityCommons.getGUID(item = $Obj)` +(ako/mxcli#1697). A variable whose entity check cannot tell is not judged. + ### XPath in Expressions ```mdl diff --git a/.claude/skills/mendix/write-workflows/SKILL.md b/.claude/skills/mendix/write-workflows/SKILL.md index 4406f7db8..7d54dbcc5 100644 --- a/.claude/skills/mendix/write-workflows/SKILL.md +++ b/.claude/skills/mendix/write-workflows/SKILL.md @@ -14,16 +14,16 @@ description: "Author Mendix workflows in MDL — user tasks, decisions, parallel | 48-118 | Syntax — CREATE WORKFLOW | | 119-300 | Activities | | 301-307 | DROP WORKFLOW | -| 308-373 | ALTER WORKFLOW | -| 374-394 | DESCRIBE round-trip | -| 395-415 | Activity names, and why `jump to` depends on them | -| 416-436 | Rewriting an existing workflow | -| 437-446 | Task pages | -| 447-534 | Microflow statements for workflow tasks | -| 535-555 | System-module enumerations are synthesized, not stored | -| 556-680 | Platform rules | -| 681-711 | Observing a running workflow | -| 712-721 | Validate before presenting | +| 308-375 | ALTER WORKFLOW | +| 376-396 | DESCRIBE round-trip | +| 397-417 | Activity names, and why `jump to` depends on them | +| 418-438 | Rewriting an existing workflow | +| 439-448 | Task pages | +| 449-536 | Microflow statements for workflow tasks | +| 537-557 | System-module enumerations are synthesized, not stored | +| 558-682 | Platform rules | +| 683-713 | Observing a running workflow | +| 714-723 | Validate before presenting | # Mendix Workflows Skill @@ -204,7 +204,7 @@ user's members with no entity in front — `targeting users xpath which hides every CE0161 from mx check. Measured: [reference/targeting.md](reference/targeting.md). **Notes** attach to an activity with `@annotation '…'` on the line before it, -as in a microflow; the workflow's own note is the header clause +as in a microflow; the note on the workflow's start is the header clause `annotation '…'`, and an event sub-process takes `@annotation` before `event subprocess`. One note per activity; no other `@` annotation is accepted. @@ -336,9 +336,11 @@ caption `'Start'`) is addressable too, but nothing goes before it — `insert before start1` is refused (it would be `CE9526`); use `insert after start1`. Workflow keys: `Display`, `Description`, `ExportLevel`, `DueDate`, -`OverviewPage`, `Parameter: $WorkflowContext: Module.Entity`. Activity keys -(with `on `): `Page`, `Description`, `Targeting: microflow M.F` / -`Targeting: xpath [ … ]`, `DueDate`. +`OverviewPage`, `Parameter: $WorkflowContext: Module.Entity`, `Annotation` (the +header's `annotation '…'`, the start's note). Activity keys (with `on +`): `Page`, `Description`, `TaskName`, `Targeting: microflow M.F` / +`Targeting: xpath [ … ]`, `DueDate`, `Annotation` (its `@annotation '…'`; `''` +removes a note, and an end takes none). **Adding to an activity: `insert into`.** What goes in the braces is the activity's own clause, as `create workflow` writes it — and it has to match the diff --git a/.claude/skills/mendix/xpath-constraints/SKILL.md b/.claude/skills/mendix/xpath-constraints/SKILL.md index 2817bbbd6..a47dfdb39 100644 --- a/.claude/skills/mendix/xpath-constraints/SKILL.md +++ b/.claude/skills/mendix/xpath-constraints/SKILL.md @@ -11,14 +11,14 @@ description: "XPath constraint syntax for MDL — retrieve WHERE clauses, page d | lines | section | |---|---| | 28-34 | When to Use This Skill | -| 35-75 | XPath vs Mendix Expressions | -| 76-119 | Syntax Reference | -| 120-297 | How a Constraint Is Laid Out on Disk | -| 298-348 | Usage Contexts | -| 349-388 | Enumeration Attributes | -| 389-430 | Common Patterns | -| 431-442 | Validation | -| 443-452 | Troubleshooting | +| 35-87 | XPath vs Mendix Expressions | +| 88-131 | Syntax Reference | +| 132-309 | How a Constraint Is Laid Out on Disk | +| 310-360 | Usage Contexts | +| 361-400 | Enumeration Attributes | +| 401-442 | Common Patterns | +| 443-454 | Validation | +| 455-464 | Troubleshooting | # XPath Constraints in MDL @@ -45,6 +45,7 @@ This skill provides reference for writing XPath constraint expressions in MDL RE | Token quoting | `'[%CurrentUser%]'` (quoted) | `[%CurrentUser%]` (unquoted) | | Nested filter | `Assoc/entity[pred]` | Not applicable | | Arithmetic on value | **not supported** — pre-compute into a variable | `+`, `-`, `*`, `div`, `mod` | +| Constant | **not supported** — assign it to a variable | `@Module.Constant` | > **XPath constraints cannot compute values.** `where [Seq = $Game/MoveSeq + 1]` > is a parse error (Mendix XPath has no arithmetic on the value side). Compute the @@ -73,6 +74,17 @@ This skill provides reference for writing XPath constraint expressions in MDL RE > retrieve $L from Mod.T where [DueDate > $Cutoff]; > ``` +> **Constants are not available in XPath either.** `[Total > @Mod.C_Limit]` +> parses, but mxbuild rejects a constant in a retrieve, a data source, an access +> rule and a user task's targeting alike (`CE0161`, measured on 11.13.0) — an +> unknown one too, so it is never a missing-constant error. `mxcli check` +> refuses it (**MDL-XPATH03**). In a flow, assign it to a variable first: +> ```mdl +> -- ❌ WRONG: retrieve $L from Mod.T where [Total > @Mod.C_Limit]; +> declare $Limit integer = @Mod.C_Limit; +> retrieve $L from Mod.T where [Total > $Limit]; +> ``` + ## Syntax Reference ### Simple Comparisons diff --git a/CLAUDE.md b/CLAUDE.md index 1439eaa64..5ebc299db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -127,10 +127,10 @@ When generating Mendix expression strings (e.g., in `expressionToString()`), sin The skills advise **quoting all identifiers** to avoid keyword collisions, but this only escapes **MDL parser** keywords (so `"create"`, `"status"`, `"end"` become valid attribute names). It does **not** exempt names the Mendix **platform** reserves for entity members — those are rejected by Studio Pro (and by `mxcli check --references`) **even when quoted**, because the check strips the quotes and validates the bare name: -- `Type` → CE7247 / `MDL021` ("reserved word"). Rename (e.g. `ResourceType`, `TypeValue`). Also `ID`, `GUID`, `CurrentUser`, and the Java-keyword list. +- `Type` → CE7247 / `MDL021` ("reserved word"). Rename (e.g. `ResourceType`, `TypeValue`). Also `ID`, `GUID`, `CurrentUser`, `Object`, and the Java-keyword list; page/snippet/Java/JavaScript action parameters too (`MDL-PARAM01`). - `CreatedDate` / `ChangedDate` / `Owner` / `ChangedBy` → `MDL020` on persistent entities. Use the `AutoCreatedDate` / `AutoChangedDate` / `AutoOwner` / `AutoChangedBy` pseudo-types for the audit fields, or a different name for an unrelated value. -The reserved-word lists live in `mdl/executor/cmd_enumerations.go` (`mendixReservedWords`, `mendixSystemAttributeNames`). "Always safe to quote" in the skills means *parser*-safe, not *platform*-safe. +The reserved-word lists live in `mdl/types/reserved_words.go`. "Always safe to quote" in the skills means *parser*-safe, not *platform*-safe. ### A `GUID` Is the Database's Identity — Never Mint One for an Existing Element diff --git a/cmd/mxcli/syntax/features_integration.go b/cmd/mxcli/syntax/features_integration.go index 2f0502c55..054c88d21 100644 --- a/cmd/mxcli/syntax/features_integration.go +++ b/cmd/mxcli/syntax/features_integration.go @@ -617,9 +617,10 @@ DESCRIBE DATABASE CONNECTION Ops.Erp;`, "xpath", "constraint", "where", "predicate", "filter", "retrieve", "association path", "enumeration", "formatting", "readable", "line length", "wrap", "multi-line", + "constant", "@", }, - Syntax: "WHERE [condition]\nWHERE [cond1][cond2] -- implicit AND\nWHERE [cond1] AND [cond2]\nWHERE [cond1] OR [cond2]\n\nHow it is STORED is mxcli's choice, not your whitespace: a constraint is\nrebuilt from its parse tree on every write, so the layout is derived from the\nexpression. 80 columns or fewer is stored exactly as written. Longer is broken\nat its top-level and/or joints, one clause per line, so it can be read in\nStudio Pro's XPath editor without scrolling sideways:\n\n [\n Archived = false\n and Status = 'Open'\n and ReportedOn > '[%BeginOfCurrentDay%]'\n ]\n\nWhere and/or meet, the and-runs get explicit parentheses (Mendix binds `and`\ntighter). A clause with no boolean joint — one long comparison or association\npath — is left whole and over width. DESCRIBE puts it back on one line, and\nre-executing that re-derives the same stored text.", - Example: "RETRIEVE $Orders FROM Module.Order\n WHERE [State = 'Completed'][IsPaid = true]\n SORT BY OrderDate DESC;\n\n-- Enumeration attribute: qualified name preferred (mxcli converts to 'Open' in BSON)\nRETRIEVE $Open FROM Module.Order\n WHERE [Status = Module.OrderStatus.Open];\n\n-- OR: string literal form also accepted\nRETRIEVE $Open FROM Module.Order\n WHERE [Status = 'Open'];\n\n-- Association path traversal\nWHERE [Module.Order_Customer/Module.Customer/Name = $Name]\n\n-- Mendix tokens\nWHERE [System.owner = '[%CurrentUser%]']", + Syntax: "WHERE [condition]\nWHERE [cond1][cond2] -- implicit AND\nWHERE [cond1] AND [cond2]\nWHERE [cond1] OR [cond2]\n\nA constraint cannot reference a constant: [Total > @Module.C_Limit] is\nrefused (MDL-XPATH03) — mxbuild rejects it with CE0161 in a retrieve, a data\nsource, an access rule and a user task's targeting. Assign the constant to a\nvariable and compare with the variable.\n\nHow it is STORED is mxcli's choice, not your whitespace: a constraint is\nrebuilt from its parse tree on every write, so the layout is derived from the\nexpression. 80 columns or fewer is stored exactly as written. Longer is broken\nat its top-level and/or joints, one clause per line, so it can be read in\nStudio Pro's XPath editor without scrolling sideways:\n\n [\n Archived = false\n and Status = 'Open'\n and ReportedOn > '[%BeginOfCurrentDay%]'\n ]\n\nWhere and/or meet, the and-runs get explicit parentheses (Mendix binds `and`\ntighter). A clause with no boolean joint — one long comparison or association\npath — is left whole and over width. DESCRIBE puts it back on one line, and\nre-executing that re-derives the same stored text.", + Example: "RETRIEVE $Orders FROM Module.Order\n WHERE [State = 'Completed'][IsPaid = true]\n SORT BY OrderDate DESC;\n\n-- Enumeration attribute: qualified name preferred (mxcli converts to 'Open' in BSON)\nRETRIEVE $Open FROM Module.Order\n WHERE [Status = Module.OrderStatus.Open];\n\n-- OR: string literal form also accepted\nRETRIEVE $Open FROM Module.Order\n WHERE [Status = 'Open'];\n\n-- Association path traversal\nWHERE [Module.Order_Customer/Module.Customer/Name = $Name]\n\n-- Mendix tokens\nWHERE [System.owner = '[%CurrentUser%]']\n\n-- A constant, through a variable\nDECLARE $Limit Integer = @Module.C_Limit;\nRETRIEVE $Big FROM Module.Order\n WHERE [Total > $Limit];", SeeAlso: []string{"xpath.functions"}, }) diff --git a/cmd/mxcli/syntax/features_workflow.go b/cmd/mxcli/syntax/features_workflow.go index d4546c436..c8013b1ca 100644 --- a/cmd/mxcli/syntax/features_workflow.go +++ b/cmd/mxcli/syntax/features_workflow.go @@ -580,9 +580,10 @@ func init() { // Mendix cannot LOAD, ako/mxcli#415). Syntax: "ALTER WORKFLOW Module.Name {\n" + " SET ( Display: '', Description: '', ExportLevel: API | Hidden,\n" + - " DueDate: , OverviewPage: Module.Page, Parameter: $WorkflowContext: Module.Entity );\n" + - " SET ( Page: Module.Page, Description: '', DueDate: ,\n" + - " Targeting: microflow Module.Mf | Targeting: xpath [] ) ON ;\n" + + " DueDate: , OverviewPage: Module.Page, Parameter: $WorkflowContext: Module.Entity,\n" + + " Annotation: '' );\n" + + " SET ( Page: Module.Page, Description: '', TaskName: '', DueDate: ,\n" + + " Targeting: microflow Module.Mf | Targeting: xpath [], Annotation: '' ) ON ;\n" + " INSERT BEFORE | AFTER { }\n" + " INSERT INTO { OUTCOMES '' { } }\n" + " INSERT INTO { OUTCOMES true | false | default | '' -> { } }\n" + @@ -593,7 +594,9 @@ func init() { " PATH , BOUNDARY EVENT;\n" + "};\n\n" + "-- is a name (ReviewTask) or a caption ('Review the request'),\n" + - "-- optionally @n to choose one of several matches: describe workflow prints the names.", + "-- optionally @n to choose one of several matches: describe workflow prints the names.\n" + + "-- Annotation is the note CREATE writes: the header's ANNOTATION '…' without ON, the\n" + + "-- @annotation '…' before the activity with ON; '' removes it.", Example: "alter workflow HR.LeaveApproval {\n" + " set (DueDate: addDays([%CurrentDateTime%], 7));\n" + " set (Page: HR.ReviewPage) on ReviewTask;\n" + diff --git a/docs-site/src/language/activity-types.md b/docs-site/src/language/activity-types.md index dd904eec9..9eb8edcec 100644 --- a/docs-site/src/language/activity-types.md +++ b/docs-site/src/language/activity-types.md @@ -96,6 +96,14 @@ RETRIEVE $RecentOrders FROM Sales.Order With `FIRST` the result is a single entity object (Mendix's "First object" range). Otherwise the result is a list, including with `LIMIT`/`OFFSET`. A bare `LIMIT 1` is a list of one (a script file without the header reads it differently; see [Language Versions and Migration](versions.md)). `describe` prints the object range as `FIRST`. +A WHERE clause cannot reference a constant. `WHERE Total > @Sales.MinimumTotal` parses, and `mxcli check` refuses it (MDL-XPATH03): Mendix rejects a constant in any XPath constraint with CE0161, in a data source and an access rule as much as in a retrieve. Assign the constant to a variable and compare with the variable: + +```sql +DECLARE $Minimum Decimal = @Sales.MinimumTotal; +RETRIEVE $LargeOrders FROM Sales.Order + WHERE Total > $Minimum; +``` + ### RETRIEVE by Association Retrieves objects by following an association from an existing object: @@ -108,7 +116,11 @@ RETRIEVE $Lines FROM $Order/Sales.OrderLine_Order; RETRIEVE $Customer FROM $Order/Sales.Order_Customer; ``` -The association path uses the format `$Variable/Module.AssociationName`. +The association path uses the format `$Variable/Module.AssociationName`. A bare +name (`$Order/Order_Customer`) is resolved against the variable's entity — through +its generalizations, from either end of the association — when exactly one +association of that name fits, and stored qualified; a name that fits none, or +more than one, is refused by `mxcli check -p --references` and by `exec`. ### RETRIEVE from Variable List diff --git a/docs-site/src/reference/domain-model/create-entity.md b/docs-site/src/reference/domain-model/create-entity.md index bcca13820..525a8036b 100644 --- a/docs-site/src/reference/domain-model/create-entity.md +++ b/docs-site/src/reference/domain-model/create-entity.md @@ -256,6 +256,7 @@ INDEX (OrderDate DESC); - `String` requires an explicit length: `String(200)`. Use `String(unlimited)` for unbounded text. - `EXTENDS` must appear **before** the opening parenthesis, not after the closing one. +- A member name must be unique along the generalization chain: a specialization cannot declare an attribute or own an association named like a member it inherits (from `System.User` and the other System entities too), and a generalization cannot gain one a specialization already has. An entity's attributes and associations share that name space; names differing only in case do not collide. `mxcli check` refuses it (MDL-MEMBER01); Mendix reports it as CE0069 "Duplicate member name". - Boolean attributes without an explicit `DEFAULT` automatically default to `FALSE`. - `AutoNumber` attributes are only valid on persistent entities. - The `@Position` annotation is optional and only affects the visual layout in Mendix Studio Pro. diff --git a/docs-site/src/reference/page/create-page.md b/docs-site/src/reference/page/create-page.md index 94c5600cd..906c7467a 100644 --- a/docs-site/src/reference/page/create-page.md +++ b/docs-site/src/reference/page/create-page.md @@ -27,7 +27,7 @@ A replace (`OR REPLACE` or `OR MODIFY`) is refused when the stored page holds a The page declaration block `( ... )` contains comma-separated key-value properties: -- **Params** -- Page parameters. Each parameter has a `$`-prefixed name and a type — either an entity (`Module.Entity`) or a primitive (`String`, `Integer`, `Decimal`, `Boolean`, `DateTime`). Parameters are passed when the page is opened (e.g., from a microflow `SHOW PAGE` or from a data view). Primitive parameters require Mendix 11.6+. +- **Params** -- Page parameters. Each parameter has a `$`-prefixed name and a type — either an entity (`Module.Entity`) or a primitive (`String`, `Integer`, `Decimal`, `Boolean`, `DateTime`). Parameters are passed when the page is opened (e.g., from a microflow `SHOW PAGE` or from a data view). Primitive parameters require Mendix 11.6+. A parameter cannot be named with a platform-reserved word — `Context`, `Type`, `Id`, `Owner`, `Object`, `CurrentUser` or a Java keyword, in any case and even quoted: Mendix reports CE7247 "The name 'Context' is a reserved word.", and `mxcli check` refuses it (MDL-PARAM01). The same holds for snippet, Java action and JavaScript action parameters. - **Title** -- The page title displayed in the browser tab or page header. - **Layout** -- The layout the page is based on. Must be a qualified name referring to an existing layout (e.g., `Atlas_Core.Atlas_Default`, `Atlas_Core.PopupLayout`). - **Folder** -- Optional folder path within the module. Nested folders use `/` separator. diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index a04201871..387e049f6 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -559,6 +559,7 @@ it is for pages. | Rollback | `rollback $entity [refresh];` | Reverts uncommitted changes | | Retrieve (DB) | `retrieve $Var from Module.Entity [where condition] [sort by Attr asc\|desc, ...] [first \| [limit n] [offset n]];` | Database XPath retrieve. `first` binds a single **object** (Mendix's "First object" range); `limit`/`offset` bind a list. `limit 1` without `offset` is a list of one under `mdl 1;`, and without the header keeps its old meaning, the object, with warning MDL-V1-LIMIT1 | | Retrieve (DB), sorted | `sort by Attr asc` / `sort by Module.Other.Attr asc` / `sort by Module.Assoc/Module.Other.Attr asc` | A bare name is qualified with the entity **declaring** it, which may be an ancestor. `describe` prints the bare name whenever it resolves back to the stored attribute, and the qualified name otherwise. A sort may also navigate associations — one `/` per hop, the last segment is the attribute — and mxcli stores the hops as the `EntityRef` Mendix needs; without them the build is **CE7247**. **Name the hop when more than one association reaches the same entity**: a bare `Module.Other.Attr` is resolved by inference, which walks the generalization chain across modules (`Administration.Account` reaches `System.Language.Code` through `System.User_Language`) but cannot tell `Order_ShipTo` from `Order_BillTo` — measured, a sort on the billing address round-tripped into one on the shipping address at 0 errors both sides (mendixlabs/mxcli#1152). The same spelling works in a page datasource's `sort by` | +| Retrieve (DB), constant | `declare $Limit Integer = @Module.C_Limit;` then `retrieve $L from Module.Entity where [Total > $Limit];` | An XPath constraint cannot reference a constant: `[Total > @Module.C_Limit]` parses and is refused by check (**MDL-XPATH03**), because mxbuild rejects it with CE0161 in a retrieve, a data source, an access rule and a user task's targeting alike (ako/mxcli#1685). Assign the constant to a variable and compare with that | | Retrieve (Assoc) | `retrieve $list from $Parent/Module.AssocName;` | Retrieve by association | | Add to list | `add expression to $list;` | Also accepts existing `add $item to $list;` form | | Remove from list | `remove $item from $list;` | Change list: Remove | @@ -854,8 +855,9 @@ Modify an existing workflow's properties, activities, outcomes, paths, condition | Operation | Syntax | Notes | |-----------|--------|-------| -| Set workflow properties | `set (Display: 'name', Description: 'text', ExportLevel: API, DueDate: , OverviewPage: Module.Page, Parameter: $WorkflowContext: Module.Entity);` | Any subset of the keys | +| Set workflow properties | `set (Display: 'name', Description: 'text', ExportLevel: API, DueDate: , OverviewPage: Module.Page, Parameter: $WorkflowContext: Module.Entity, Annotation: 'note');` | Any subset of the keys. `Annotation` is the header's `annotation '…'`, the note on the start; `''` removes it | | Set activity properties | `set (Page: Module.Page, Description: 'text', DueDate: ) on activity;` | User task page, description, due date | +| Set an activity's note | `set (Annotation: 'note') on activity;` | The `@annotation '…'` before it; `''` removes it. On the start it is the header's note; an end takes none (Studio Pro shows no note there) | | Set activity targeting | `set (Targeting: microflow Module.MF) on activity;` / `set (Targeting: xpath []) on activity;` | XPath in brackets; the quoted `xpath '[…]'` warns MDL-DEPR031 | | Insert activities | `insert after activity { … }` / `insert before activity { … }` | One or more activities, as in `create workflow` | | Replace activity | `replace activity with { … }` | Replace in place | diff --git a/mdl-examples/bug-tests/1717-alter-workflow-annotation.mdl b/mdl-examples/bug-tests/1717-alter-workflow-annotation.mdl new file mode 100644 index 000000000..00f800438 --- /dev/null +++ b/mdl-examples/bug-tests/1717-alter-workflow-annotation.mdl @@ -0,0 +1,80 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1717 — "`alter workflow … set (Annotation: '…')` is refused for the +-- workflow and for an activity" +-- ============================================================================ +-- +-- CREATE writes two notes: the header's `annotation '…'` (the note on the +-- workflow's start) and `@annotation '…'` before an activity. ALTER now sets +-- both as `Annotation`: without `on` (or `on Start`) the start's, with `on X` +-- activity X's. `Annotation: ''` removes the note. Before, both were refused — +-- "a workflow has no property Annotation" — and the error did not name the +-- CREATE spellings, so an agent concluded workflow notes were unsupported. +-- +-- The start's note is the START ACTIVITY's: Studio Pro 11.15's workflow editor +-- (ide-client/workflow-editor, shipped with mxbuild) draws note badges on +-- activities, boundary events, event sub-processes and the start — from +-- StartWorkflowActivity.Annotation — and the workflow's own Annotation nowhere. +-- The header clause wrote the workflow's own (since ako/mxcli#707), so it now +-- writes the start's, describe reads the start's (the workflow's own still +-- reads for workflows written before), and a Studio Pro note on the start is no +-- longer dropped by describe -> replay. +-- +-- Written without the braces, `alter workflow M.W set (Annotation: '…');` was +-- diagnosed as "`set (annotation` is a session command"; it is now a syntax +-- error that shows the braced statement. +-- +-- Measured on 11.15.0: CREATE stores the header note on the start activity and +-- null on the workflow; after the ALTER below the project checks clean and +-- describe -> replay reports "Unchanged workflow". A workflow written by the +-- previous build (note on the workflow) describes the same, and its replay +-- moves the note to the start once ("Modified", then "Unchanged"). `on End` is +-- refused: Studio Pro draws no note on an end. +-- +-- EXPECTED: `mxcli check` passes this file. +-- Guard: mdl/visitor/visitor_alter_workflow_annotation_test.go, +-- mdl/visitor/alter_workflow_braces_hint_test.go, +-- mdl/backend/wfmutator/annotation_test.go, +-- mdl/backend/modelsdk/workflow_start_note_test.go, +-- mdl/executor/workflow_start_note_test.go. +-- ============================================================================ + +create module BugAnnotation1717; + +create persistent entity BugAnnotation1717.Request ( + Title: String(200) +); + +create page BugAnnotation1717.WF_Review +( + params: ( $WorkflowUserTask: System.WorkflowUserTask ), + title: 'Review', + layout: Atlas_Core.Atlas_Default +) +{ + dataview dvTask (DataSource: $WorkflowUserTask) { + actionbutton btnDone (Caption: 'Done', Action: complete task 'Done') + } +}; + +create microflow BugAnnotation1717.ACT_Log ($WorkflowContext: BugAnnotation1717.Request) +begin + log info node 'WF' 'logged'; +end; + +create workflow BugAnnotation1717.DocumentApproval + parameter $WorkflowContext: BugAnnotation1717.Request +begin + user task Review 'Review' + page BugAnnotation1717.WF_Review + targeting users xpath [System.UserRoles = '[%UserRole_User%]'] + outcomes 'Done' { }; + @annotation 'Logs it' + call microflow BugAnnotation1717.ACT_Log; +end workflow; + +alter workflow BugAnnotation1717.DocumentApproval { + set (Annotation: 'Started from the request form'); + set (Annotation: 'Check the attachment first') on Review; + set (Annotation: '') on ACT_Log; +}; diff --git a/mdl-examples/bug-tests/member-name-1645-inherited-association.fail.mdl b/mdl-examples/bug-tests/member-name-1645-inherited-association.fail.mdl new file mode 100644 index 000000000..09448ca95 --- /dev/null +++ b/mdl-examples/bug-tests/member-name-1645-inherited-association.fail.mdl @@ -0,0 +1,17 @@ +-- NEGATIVE TEST (.fail.mdl) — EXPECTED to fail `mxcli check` with MDL-MEMBER01 +-- (ako/mxcli#1645). +-- +-- An association is a member of its FROM entity (and, under owner Both, of its +-- TO entity too), so its name shares the entity's member name space. HR.Emp +-- inherits the association Person_Dept from Other.Person; a second association +-- of that name from HR.Emp passed check and exec, and mxbuild rejected it with +-- [CE0069] "Duplicate member name 'Person_Dept'." at Cross-module association 'HR.Person_Dept' +-- Measured on mx 11.13.0 (and reported on 11.12.5). +mdl 1; +create module Other; +create module HR; +create persistent entity Other.Person (Name: String(50)); +create persistent entity Other.Dept (Name: String(50)); +create association Other.Person_Dept from Other.Person to Other.Dept; +create persistent entity HR.Emp extends Other.Person (Code: String(10)); +create association HR.Person_Dept from HR.Emp to Other.Dept; diff --git a/mdl-examples/bug-tests/member-name-1645-inherited-attribute.fail.mdl b/mdl-examples/bug-tests/member-name-1645-inherited-attribute.fail.mdl new file mode 100644 index 000000000..1b404c1e0 --- /dev/null +++ b/mdl-examples/bug-tests/member-name-1645-inherited-attribute.fail.mdl @@ -0,0 +1,19 @@ +-- NEGATIVE TEST (.fail.mdl) — EXPECTED to fail `mxcli check` with MDL-MEMBER01 +-- (ako/mxcli#1645). +-- +-- A member name must be unique along an entity's generalization chain. HR.Emp +-- inherits Name from Other.Person, so declaring its own Name is a second member +-- of that name: check and exec passed it, and mxbuild rejected the model with +-- [CE0069] "Duplicate member name 'Name'." at Attribute 'HR.Emp.Name' +-- Measured on mx 11.13.0 (and reported on 11.12.5). +-- +-- The same holds for an association (see member-name-1645-inherited-association), +-- for a System ancestor's members (`extends System.User` with `Name` or +-- `UserRoles`), for a generalization gaining a member a specialization already +-- has, and for a rename into such a name. Names that differ only in case do not +-- collide. +mdl 1; +create module Other; +create module HR; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Emp extends Other.Person (Name: String(50), Code: String(10)); diff --git a/mdl-examples/bug-tests/page-params-1651-reserved-context.fail.mdl b/mdl-examples/bug-tests/page-params-1651-reserved-context.fail.mdl new file mode 100644 index 000000000..0896dacb9 --- /dev/null +++ b/mdl-examples/bug-tests/page-params-1651-reserved-context.fail.mdl @@ -0,0 +1,24 @@ +-- NEGATIVE TEST (.fail.mdl) — EXPECTED to fail `mxcli check` with MDL-PARAM01 +-- (ako/mxcli#1651, a mirror of mendixlabs/mxcli#1642). +-- +-- `Context` is on the platform's reserved-word list — the one attribute names +-- (MDL021) and enumeration values (MDL010) are checked against — and that list +-- binds page parameters too. check and exec passed this page, and mx check +-- then reported +-- [CE7247] "The name 'Context' is a reserved word." at Page parameter 'Context' +-- Measured on mx 11.13.0 (and reported on 11.12.5): the same for a snippet, Java +-- action and JavaScript action parameter, in any case, and quoted. Microflow and +-- nanoflow parameters are judged against a different list and build clean. +-- +-- The reporter's workaround, `$LaneContext`, builds clean; so does `$Sale`. +mdl 1; +create persistent entity MyFirstModule.Sale (Name: String(100)); +create page MyFirstModule.Sale_View ( + Title: 'Sale', + Layout: Atlas_Core.Atlas_Default, + Params: ( $Context: MyFirstModule.Sale ) +) { + layoutgrid lg { row { column (DesktopWidth: AutoFill) { + dataview dvSale (DataSource: $Context) { textbox tbName (Attribute: Name) } + } } } +}; diff --git a/mdl-examples/bug-tests/references-1646-list-operation-missing-member.mdl b/mdl-examples/bug-tests/references-1646-list-operation-missing-member.mdl new file mode 100644 index 000000000..37b2997d9 --- /dev/null +++ b/mdl-examples/bug-tests/references-1646-list-operation-missing-member.mdl @@ -0,0 +1,57 @@ +-- ako/mxcli#1646: find, filter, sort or an aggregate by a member the list's +-- entity does not have passed `check -p` and exec; mxbuild then failed once per +-- activity (measured on 11.12.5 and 11.13.0): +-- +-- [CE1613] "The selected attribute 'RefB1646.Ord.Numbr' no longer exists." at list operation activity 'Find' +-- [CE1613] "The selected attribute 'RefB1646.Ord.Totl' no longer exists." at ... 'Filter', 'Sort', aggregate list +-- +-- The flow builder fell back to writing `.` when the name +-- resolved to nothing. Now `check -p --references`, exec's preflight and the +-- builder (exec --no-check, exec -c, the REPL) ask one verdict and refuse the +-- four statements in RefB1646.Missing with the same message: +-- +-- find $l by Numbr: RefB1646.Ord has no member "Numbr" — it has Number, Total — mxbuild reports this as CE1613 ... +-- +-- RefB1646.Controls builds at 0 errors and stays accepted: own and inherited +-- attributes, a bare and an inherited cross-module association, System.owner, +-- a sort on a system date the root stores. Its `sum $sl by Total` — an +-- inherited attribute — used to be written as RefB1646.SubOrd.Total (CE1613); +-- it is now qualified with the entity that declares it, RefB1646.Ord.Total. +-- +-- `check` without a project cannot see any of this (the members live in the +-- model), so this file is a plain .mdl; the refusal is covered by +-- mdl/executor/list_member_refs_1646_test.go. +mdl 1; + +create module RefB1646; +create module RefO1646; +create persistent entity RefB1646.Cust (Name: String(50)); +create persistent entity RefB1646.Ord (Number: String(20), Total: Decimal, owner: AutoOwner, createdDate: AutoCreatedDate); +create persistent entity RefB1646.SubOrd extends RefB1646.Ord (Extra: String(20)); +create association RefB1646.Ord_Cust from RefB1646.Ord to RefB1646.Cust; +create persistent entity RefO1646.Base (BaseName: String(20)); +create persistent entity RefO1646.Thing (ThingName: String(20)); +create association RefO1646.Base_Thing from RefO1646.Base to RefO1646.Thing; +create persistent entity RefB1646.Spec extends RefO1646.Base (SpecName: String(20)); + +create microflow RefB1646.Controls ($l: List of RefB1646.Ord, $sl: List of RefB1646.SubOrd, $spl: List of RefB1646.Spec, $c: RefB1646.Cust, $th: RefO1646.Thing, $u: System.User) +begin + $a1 = find $sl by Number = '1'; + $a2 = sort $sl by Number asc; + $a3 = sum $sl by Total; + $a4 = find $l by Ord_Cust = $c; + $a5 = find $spl by Base_Thing = $th; + $a6 = sort $l by createdDate asc; + $a7 = find $l by System.owner = $u; + $a8 = sort $spl by BaseName desc; +end; + +-- Refused by check -p --references and by exec (the reported repro). +create microflow RefB1646.Missing ($l: List of RefB1646.Ord) returns Boolean +begin + $m = find $l by Numbr = '42'; + $f = filter $l by Totl = 5; + $s = sort $l by Totl asc; + $t = sum $l by Totl; + return true; +end; diff --git a/mdl-examples/bug-tests/references-1690-bare-retrieve-association.mdl b/mdl-examples/bug-tests/references-1690-bare-retrieve-association.mdl new file mode 100644 index 000000000..8b6b16fb7 --- /dev/null +++ b/mdl-examples/bug-tests/references-1690-bare-retrieve-association.mdl @@ -0,0 +1,41 @@ +-- ako/mxcli#1690: `retrieve $C from $O/Ord_Cust;` — an association path written +-- with the association's SHORT name — passed `check -p --references` and exec, +-- and was stored as the association ".Ord_Cust" (empty module). Mendix 11.13.0: +-- +-- [CE1613] "The selected association '.Ord_Cust' no longer exists." +-- at Retrieve object(s) activity 'Retrieve from association' +-- +-- and describe printed `retrieve $C from $O/.Ord_Cust;`, which does not parse. +-- +-- Now a bare name resolves against the start variable's entity — through its +-- generalization chain, from either end of the association — when exactly one +-- association of that name fits, and is stored qualified: every retrieve below +-- is stored as the association in the comment, describe prints that, and +-- `mx check` reports 0 errors (measured on 11.13.0). +-- +-- A bare name that fits no association, or more than one, is refused by +-- `check -p --references` and by exec, with the qualified spelling to write: +-- +-- retrieve $C from $O/Ord_Kust; -- no association called "Ord_Kust" has RefB1690.Ord ... +-- +-- It is never written with an empty module. +mdl 1; + +create module RefB1690; +create module RefO1690; +create persistent entity RefB1690.Cust (Name: String(50)); +create persistent entity RefB1690.Ord (Number: String(20)); +create persistent entity RefB1690.SubOrd extends RefB1690.Ord (Extra: String(20)); +create association RefB1690.Ord_Cust from RefB1690.Ord to RefB1690.Cust; +create persistent entity RefO1690.Base (BaseName: String(20)); +create persistent entity RefO1690.Thing (ThingName: String(20)); +create association RefO1690.Base_Thing from RefO1690.Base to RefO1690.Thing; +create persistent entity RefB1690.Spec extends RefO1690.Base (SpecName: String(20)); + +create microflow RefB1690.BareAssociations ($O: RefB1690.Ord, $C: RefB1690.Cust, $S: RefB1690.SubOrd, $Sp: RefB1690.Spec) +begin + retrieve $Cust from $O/Ord_Cust; -- RefB1690.Ord_Cust (the owner) + retrieve $Orders from $C/Ord_Cust; -- RefB1690.Ord_Cust (the TO end) + retrieve $SubCust from $S/Ord_Cust; -- RefB1690.Ord_Cust (a specialization) + retrieve $Thing from $Sp/Base_Thing; -- RefO1690.Base_Thing (inherited from another module) +end; diff --git a/mdl-examples/bug-tests/references-1697-expression-missing-member.mdl b/mdl-examples/bug-tests/references-1697-expression-missing-member.mdl new file mode 100644 index 000000000..8154b5498 --- /dev/null +++ b/mdl-examples/bug-tests/references-1697-expression-missing-member.mdl @@ -0,0 +1,50 @@ +-- ako/mxcli#1697 (mirror of mendixlabs/mxcli#1650): `$Var/` in an +-- expression, for a member the variable's entity does not have — the system +-- `id` above all — passed `check -p --references` (the type pass even printed +-- "Expression types OK") and exec wrote it; mxbuild then reported +-- +-- [CE0117] "Error(s) in expression." at Create variable activity ... +-- +-- Measured on 11.13.0, one microflow per shape, in an app with no other error +-- (a CE1613 anywhere suppresses every CE0117 — mx check then reports none): +-- +-- CE0117 $U/id, $O/id = $S/id, $U/Bogus, $O/number (for Number), $O/CreatedDate, +-- $O/changedDate (not stored), $O/owner, $O/Ord_Cust/Name (bare association), +-- $O/Mod.Nope/Name, $O/Mod.Ord_Cust/Mod.Cust/Nam +-- clean own and inherited attributes (also from System.User), $O/Mod.Ord_Cust/Name, +-- $O/createdDate and $S/createdDate where the root stores it +-- +-- `check -p --references` — and so exec's preflight — now reports every +-- statement in RefB1697.Missing, e.g. +-- +-- $User/id: System.User has no attribute "id" — mxbuild rejects the expression with CE0117 ... +-- An object's id is not readable in an expression: compare the objects themselves +-- (`$A = $B`), or read a GUID with CommunityCommons.getGUID(item = $X) +-- +-- and accepts RefB1697.Controls, which builds at 0 errors. Plain `check` has no +-- model to resolve members against, so this file is a plain .mdl; the check is +-- covered by mdl/executor/expression_member_refs_1697_test.go. +mdl 1; + +create module RefB1697; +create persistent entity RefB1697.Cust (Name: String(50)); +create persistent entity RefB1697.Ord (Number: String(20), createdDate: AutoCreatedDate); +create persistent entity RefB1697.SubOrd extends RefB1697.Ord (Extra: String(20)); +create association RefB1697.Ord_Cust from RefB1697.Ord to RefB1697.Cust; + +create microflow RefB1697.Controls ($O: RefB1697.Ord, $S: RefB1697.SubOrd, $User: System.User) returns String as $r +begin + declare $r String = $O/Number + $S/Number + $User/Name + $O/RefB1697.Ord_Cust/Name; + declare $d DateTime = $S/createdDate; + return $r; +end; + +-- Reported by check -p --references (the reported repro and its siblings). +create microflow RefB1697.Missing ($O: RefB1697.Ord, $User: System.User) returns String as $r +begin + declare $r String = toString($User/id); + declare $b String = $User/Bogus; + declare $n String = $O/number; + declare $c String = $O/Ord_Cust/Name; + return $r; +end; diff --git a/mdl-examples/bug-tests/xpath-1685-constant-in-constraint.fail.mdl b/mdl-examples/bug-tests/xpath-1685-constant-in-constraint.fail.mdl new file mode 100644 index 000000000..7e94f58a6 --- /dev/null +++ b/mdl-examples/bug-tests/xpath-1685-constant-in-constraint.fail.mdl @@ -0,0 +1,48 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1685 — a constant inside an XPath constraint +-- ============================================================================ +-- +-- NEGATIVE TEST (.fail.mdl): `mxcli check` MUST reject this file +-- (MDL-XPATH03). An unexpected pass means the refusal has regressed. +-- +-- Symptom: `where [Total > @Shop.C_Limit]` was a parse error, +-- line 4:49 mismatched input 'Shop' expecting ']' +-- which reads as "MDL cannot spell this". `@` was an XPath name word of its +-- own, so the lenient re-parse behind describe and the long-constraint layout +-- turned a stored `[Total > @Shop.C_Limit]` into `Total > @` (and wrote +-- `[Total > @]` back for a long one), and a data source written +-- `where Total > @Shop.C_Limit` stored `[Total > ]`. +-- +-- The constraint now parses, and check gives mxbuild's answer: an XPath +-- constraint cannot reference a constant. Measured on 11.13.0, each slot below +-- alone in a fresh app (production security, or access rules are not checked): +-- +-- [error] [CE0161] "Error(s) in XPath constraint." at Retrieve object(s) activity … +-- [error] [CE0161] "Error(s) in XPath constraint." at List view 'lvItems' +-- [error] [CE0161] "Error(s) in XPath constraint." at Access rule of entity 'Bug1685.Item' +-- +-- An unknown constant is the same CE0161, not CE1613: XPath never resolves +-- the name. The variable form that builds is +-- xpath-1685-constant-through-variable.mdl. +-- +-- mxcli check mdl-examples/bug-tests/xpath-1685-constant-in-constraint.fail.mdl +-- ============================================================================ + +create module Bug1685; +create constant Bug1685.C_Limit (Type: Integer, DefaultValue: 10); +create persistent entity Bug1685.Item (Total: Integer, Name: String(100)); +create module role Bug1685.User; + +create microflow Bug1685.FetchAboveLimit () +begin + retrieve $Items from Bug1685.Item where [Total > @Bug1685.C_Limit]; +end; + +create page Bug1685.Items (Title: 'Items', Layout: Atlas_Core.Atlas_Default) { + listview lvItems (DataSource: database from Bug1685.Item where [Total > @Bug1685.C_Limit]) { + dynamictext txtName (Content: 'x') + } +}; + +grant read * on entity Bug1685.Item to Bug1685.User where [Total > @Bug1685.C_Limit]; diff --git a/mdl-examples/bug-tests/xpath-1685-constant-through-variable.mdl b/mdl-examples/bug-tests/xpath-1685-constant-through-variable.mdl new file mode 100644 index 000000000..866ae9431 --- /dev/null +++ b/mdl-examples/bug-tests/xpath-1685-constant-through-variable.mdl @@ -0,0 +1,24 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#1685 — a constant in a retrieve's constraint, the way that builds +-- ============================================================================ +-- +-- An XPath constraint cannot reference a constant: `[Total > @Bug1685V.C_Limit]` +-- is refused by `mxcli check` (MDL-XPATH03) because mxbuild rejects it with +-- CE0161 — see xpath-1685-constant-in-constraint.fail.mdl. Assign the constant +-- to a variable and compare with the variable. +-- +-- mxcli exec mdl-examples/bug-tests/xpath-1685-constant-through-variable.mdl -p app.mpr +-- mx check app.mpr # 0 errors (measured on 11.13.0 and 11.15.0) +-- mxcli -p app.mpr -c "describe microflow Bug1685V.FetchAboveLimit" +-- ============================================================================ + +create module Bug1685V; +create constant Bug1685V.C_Limit (Type: Integer, DefaultValue: 10); +create persistent entity Bug1685V.Item (Total: Integer, Name: String(100)); + +create microflow Bug1685V.FetchAboveLimit () +begin + declare $Limit Integer = @Bug1685V.C_Limit; + retrieve $Items from Bug1685V.Item where [Total > $Limit]; +end; diff --git a/mdl-examples/doctype-tests/16-xpath-examples.mdl b/mdl-examples/doctype-tests/16-xpath-examples.mdl index ef65ae29f..4824f2f21 100644 --- a/mdl-examples/doctype-tests/16-xpath-examples.mdl +++ b/mdl-examples/doctype-tests/16-xpath-examples.mdl @@ -290,6 +290,35 @@ begin return true; end; +-- ============================================================================ +-- MARK: Constants +-- ============================================================================ + +-- An XPath constraint cannot reference a constant. `[TotalAmount > +-- @XpathTest.LargeOrderAmount]` parses, and `mxcli check` refuses it +-- (MDL-XPATH03): mxbuild rejects a constant in a retrieve, a data source, an +-- access rule and a user task's targeting with CE0161 (ako/mxcli#1685). +-- +-- retrieve $Large from XpathTest.Order +-- where [TotalAmount > @XpathTest.LargeOrderAmount]; -- refused +-- +-- In a flow, assign the constant to a variable and compare with the variable. + +create constant XpathTest.LargeOrderAmount +( Type: decimal, DefaultValue: 1000 ); + +/** + * Constant in a constraint, through a variable + */ +create microflow XpathTest.Retrieve_ConstantThroughVariable () +returns boolean +begin + declare $Threshold decimal = @XpathTest.LargeOrderAmount; + retrieve $Large from XpathTest.Order + where [TotalAmount > $Threshold]; + return true; +end; + -- ============================================================================ -- MARK: Nested Predicates -- ============================================================================ diff --git a/mdl/backend/modelsdk/workflow_read.go b/mdl/backend/modelsdk/workflow_read.go index f4b7da181..5da51c1cd 100644 --- a/mdl/backend/modelsdk/workflow_read.go +++ b/mdl/backend/modelsdk/workflow_read.go @@ -314,6 +314,10 @@ func workflowSimpleActivityFromGen(el element.Element) workflows.WorkflowActivit case "Workflows$StartWorkflowActivity": a := &workflows.StartWorkflowActivity{} setBase(&a.BaseWorkflowActivity) + // The note Studio Pro shows on the start (ako/mxcli#1717). + if s, ok := el.(*genWf.StartWorkflowActivity); ok { + a.Annotation = annotationText(s.Annotation()) + } return a case "Workflows$EndWorkflowActivity": a := &workflows.EndWorkflowActivity{} diff --git a/mdl/backend/modelsdk/workflow_start_note_test.go b/mdl/backend/modelsdk/workflow_start_note_test.go new file mode 100644 index 000000000..f887e7e74 --- /dev/null +++ b/mdl/backend/modelsdk/workflow_start_note_test.go @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "testing" + + "github.com/mendixlabs/mxcli/modelsdk/codec" + genWf "github.com/mendixlabs/mxcli/modelsdk/gen/workflows" + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// roundTripWorkflow writes wf as the unit stores it and reads it back. +func roundTripWorkflow(t *testing.T, wf *workflows.Workflow) (*genWf.Workflow, *workflows.Workflow) { + t.Helper() + raw, err := (&codec.Encoder{}).Encode(workflowToGen(wf)) + if err != nil { + t.Fatalf("encode: %v", err) + } + el, err := codec.NewDecoder(codec.DefaultRegistry).Decode(raw) + if err != nil { + t.Fatalf("decode: %v", err) + } + g := el.(*genWf.Workflow) + return g, workflowFromGen(g, "") +} + +// ako/mxcli#1717: the note on a workflow's start is the start activity's — +// Studio Pro 11.15's workflow editor draws the start's note from +// StartWorkflowActivity.Annotation and the workflow's own Annotation nowhere — +// so it is written there and read back from there. +func TestWorkflowStartNote_IsTheStartActivitys(t *testing.T) { + start := &workflows.StartWorkflowActivity{} + start.Name, start.Caption, start.Annotation = "Start", "Start", "Started from the request form" + wf := &workflows.Workflow{Name: "WF", Parameter: &workflows.WorkflowParameter{EntityRef: "M.Ctx"}, + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{start, &workflows.EndWorkflowActivity{}}}} + + g, back := roundTripWorkflow(t, wf) + if g.Annotation() != nil { + t.Errorf("the workflow's own Annotation was written: %v", g.Annotation()) + } + if got := back.StartNote(); got != "Started from the request form" { + t.Errorf("StartNote() = %q, want the start activity's note", got) + } + if back.Annotation != "" { + t.Errorf("read the start's note into the workflow's own slot: %q", back.Annotation) + } +} + +// A workflow mxcli wrote before #1717 keeps its note on the workflow; it still +// reads as the start's note until a rewrite moves it. +func TestWorkflowStartNote_FallsBackToTheWorkflows(t *testing.T) { + start := &workflows.StartWorkflowActivity{} + start.Name, start.Caption = "Start", "Start" + wf := &workflows.Workflow{Name: "WF", Annotation: "Written before #1717", + Parameter: &workflows.WorkflowParameter{EntityRef: "M.Ctx"}, + Flow: &workflows.Flow{Activities: []workflows.WorkflowActivity{start, &workflows.EndWorkflowActivity{}}}} + + _, back := roundTripWorkflow(t, wf) + if got := back.StartNote(); got != "Written before #1717" { + t.Errorf("StartNote() = %q, want the workflow's own note", got) + } +} diff --git a/mdl/backend/wfmutator/annotation_test.go b/mdl/backend/wfmutator/annotation_test.go new file mode 100644 index 000000000..66e809c31 --- /dev/null +++ b/mdl/backend/wfmutator/annotation_test.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 + +package wfmutator + +import ( + "bytes" + "strings" + "testing" + + "go.mongodb.org/mongo-driver/v2/bson" + + "github.com/mendixlabs/mxcli/mdl/backend/bsonnav" +) + +// withNullAnnotation adds the `Annotation: null` every workflow and activity +// type carrying a note stores when it has none (11.15.0). +func withNullAnnotation(d bson.D) bson.D { + return append(d, bson.E{Key: "Annotation", Value: nil}) +} + +// assertNote checks d's Annotation is a Workflows$Annotation holding text, in +// Studio Pro's exact property set ($ID, $Type, Description). +func assertNote(t *testing.T, d bson.D, text string) bson.D { + t.Helper() + note := bsonnav.DGetDoc(d, "Annotation") + if note == nil { + t.Fatalf("Annotation = %v, want a note", bsonnav.DGet(d, "Annotation")) + } + var keys []string + for _, e := range note { + keys = append(keys, e.Key) + } + if strings.Join(keys, ",") != "$ID,$Type,Description" { + t.Errorf("note keys = %v, want $ID,$Type,Description", keys) + } + if bsonnav.DGetString(note, "$Type") != "Workflows$Annotation" || bsonnav.DGetString(note, "Description") != text { + t.Errorf("note = %v, want Workflows$Annotation %q", note, text) + } + return note +} + +// startDoc is a workflow whose flow starts with a start activity carrying the +// Annotation slot, and whose own Annotation holds legacy, as mxcli wrote the +// header's note before ako/mxcli#1717. +func startDoc(legacy any) bson.D { + start := withNullAnnotation(makeWfActivity("Workflows$StartWorkflowActivity", "Start", "Start")) + return append(makeWorkflowDoc(start), bson.E{Key: "Annotation", Value: legacy}) +} + +func startActivity(t *testing.T, m *Mutator) bson.D { + t.Helper() + d, err := m.findActivityByCaption("Start", 0) + if err != nil { + t.Fatal(err) + } + return d +} + +// ako/mxcli#1717: `set (Annotation: '…')` writes the note on the workflow's +// start — the start activity's, where Studio Pro shows it, and what CREATE's +// header `annotation '…'` writes — clears the workflow's own, and the empty +// string removes it. +func TestWorkflowMutator_SetProperty_Annotation(t *testing.T) { + legacy := bson.D{{Key: "$ID", Value: bson.Binary{Data: make([]byte, 16)}}, + {Key: "$Type", Value: "Workflows$Annotation"}, {Key: "Description", Value: "Old"}} + m := newMutator(startDoc(legacy)) + if err := m.SetProperty("annotation", "Started from the request form"); err != nil { + t.Fatal(err) + } + first := assertNote(t, startActivity(t, m), "Started from the request form") + if v, present := lookup(m.rawData, "Annotation"); !present || v != nil { + t.Errorf("the workflow's own Annotation = %v (present %v), want null", v, present) + } + + // Editing a note keeps its $ID: the unit is rewritten, the element is not. + if err := m.SetProperty("annotation", "Reworded"); err != nil { + t.Fatal(err) + } + second := assertNote(t, startActivity(t, m), "Reworded") + id1, _ := bsonnav.DGet(first, "$ID").(bson.Binary) + id2, _ := bsonnav.DGet(second, "$ID").(bson.Binary) + if !bytes.Equal(id1.Data, id2.Data) { + t.Errorf("editing the note changed its $ID") + } + + if err := m.SetProperty("annotation", ""); err != nil { + t.Fatal(err) + } + if v, present := lookup(startActivity(t, m), "Annotation"); !present || v != nil { + t.Errorf("after removing: Annotation = %v (present %v), want null", v, present) + } +} + +// `on Start` is the same note, set the same way. +func TestWorkflowMutator_SetActivityProperty_AnnotationOnStart(t *testing.T) { + m := newMutator(startDoc("ignored")) + if err := m.SetActivityProperty("Start", 0, "annotation", "From the form"); err != nil { + t.Fatal(err) + } + assertNote(t, startActivity(t, m), "From the form") + if v, _ := lookup(m.rawData, "Annotation"); v != nil { + t.Errorf("the workflow's own Annotation = %v, want null", v) + } +} + +// A start activity stored without an Annotation slot (a version without +// workflow notes) is refused, not given a property its version does not +// declare. +func TestWorkflowMutator_SetProperty_AnnotationNeedsTheSlot(t *testing.T) { + m := newMutator(makeWorkflowDoc(makeWfActivity("Workflows$StartWorkflowActivity", "Start", "Start"))) + if err := m.SetProperty("annotation", "x"); err == nil { + t.Fatal("a start activity without an Annotation slot must be refused") + } + if _, present := lookup(startActivity(t, m), "Annotation"); present { + t.Error("the refused set wrote an Annotation") + } + if err := newMutator(makeWorkflowDoc()).SetProperty("annotation", "x"); err == nil { + t.Error("a workflow without a start activity must be refused") + } +} + +// `set (Annotation: '…') on X` writes the activity's note, the one +// `@annotation '…'` before it writes. +func TestWorkflowMutator_SetActivityProperty_Annotation(t *testing.T) { + act := withNullAnnotation(makeWfActivity("Workflows$CallMicroflowActivity", "Log it", "ACT_Log")) + m := newMutator(makeWorkflowDoc(act)) + if err := m.SetActivityProperty("ACT_Log", 0, "annotation", "Logs it"); err != nil { + t.Fatal(err) + } + actDoc, _ := m.findActivityByCaption("ACT_Log", 0) + assertNote(t, actDoc, "Logs it") + + if err := m.SetActivityProperty("ACT_Log", 0, "annotation", ""); err != nil { + t.Fatal(err) + } + actDoc, _ = m.findActivityByCaption("ACT_Log", 0) + if v, present := lookup(actDoc, "Annotation"); !present || v != nil { + t.Errorf("after removing: Annotation = %v (present %v), want null", v, present) + } +} + +// Studio Pro draws no note on an end, and describe prints none, so a note on +// one is refused; so is an activity stored without the Annotation slot. +func TestWorkflowMutator_SetActivityProperty_AnnotationRefusals(t *testing.T) { + for _, tc := range []struct{ typeName, name, want string }{ + {"Workflows$EndWorkflowActivity", "End", "end"}, + {"Workflows$EndOfParallelSplitPathActivity", "PathEnd", "end"}, + {"Workflows$CallMicroflowActivity", "NoSlot", "no annotation"}, + } { + act := makeWfActivity(tc.typeName, "", tc.name) + if tc.name != "NoSlot" { + act = withNullAnnotation(act) + } + m := newMutator(makeWorkflowDoc(act)) + err := m.SetActivityProperty(tc.name, 0, "annotation", "x") + if err == nil { + t.Errorf("%s: accepted", tc.typeName) + continue + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("%s: error %q does not mention %q", tc.typeName, err, tc.want) + } + actDoc, _ := m.findActivityByCaption(tc.name, 0) + if v, _ := lookup(actDoc, "Annotation"); v != nil { + t.Errorf("%s: the refused set wrote %v", tc.typeName, v) + } + } +} + +// lookup reports a key's value and whether the key is present at all, which +// bsonnav.DGet cannot tell apart for a null. +func lookup(d bson.D, key string) (any, bool) { + for _, e := range d { + if e.Key == key { + return e.Value, true + } + } + return nil, false +} diff --git a/mdl/backend/wfmutator/mutator.go b/mdl/backend/wfmutator/mutator.go index b0f1ae388..5bdd23f75 100644 --- a/mdl/backend/wfmutator/mutator.go +++ b/mdl/backend/wfmutator/mutator.go @@ -96,11 +96,66 @@ func (m *Mutator) SetProperty(prop string, value string) error { bsonnav.DSet(m.rawData, "DueDate", value) return nil + case "annotation": + return m.setStartNote(value) + default: return fmt.Errorf("unsupported workflow property: %s", prop) } } +// setStartNote sets the note on the workflow's start, the header's +// `annotation '…'` (ako/mxcli#1717): the start activity's, where Studio Pro +// stores and shows it. The workflow's own Annotation — shown nowhere, and where +// mxcli wrote the note before — is cleared, or describe would still print the +// note the statement replaced or removed. +func (m *Mutator) setStartNote(text string) error { + var start bson.D + for _, el := range bsonnav.DGetArrayElements(bsonnav.DGet(bsonnav.DGetDoc(m.rawData, "Flow"), "Activities")) { + if d, ok := el.(bson.D); ok && bsonnav.DGetString(d, "$Type") == "Workflows$StartWorkflowActivity" { + start = d + break + } + } + if start == nil { + return fmt.Errorf("the workflow has no start activity to hold its note") + } + if err := setNote(start, "the workflow's start activity", text); err != nil { + return err + } + bsonnav.DSet(m.rawData, "Annotation", nil) + return nil +} + +// setNote writes the note Studio Pro attaches to an activity (ako/mxcli#1717): +// a Workflows$Annotation holding Description — the shape CREATE writes for +// `@annotation '…'` — or null for "". +// An existing note keeps its $ID. A document without the Annotation slot is +// refused: every type with a note stores the slot, null when empty (11.15.0), +// so a missing one is a type or version that has no note to write. +func setNote(owner bson.D, what, text string) error { + for i := range owner { + if owner[i].Key != "Annotation" { + continue + } + note, _ := owner[i].Value.(bson.D) + switch { + case text == "": + owner[i].Value = nil + case note != nil && bsonnav.DSet(note, "Description", text): + // Edited in place, keeping the note's $ID. + default: + owner[i].Value = bson.D{ + {Key: "$ID", Value: bsonutil.NewIDBsonBinary()}, + {Key: "$Type", Value: "Workflows$Annotation"}, + {Key: "Description", Value: text}, + } + } + return nil + } + return fmt.Errorf("%s has no annotation to set", what) +} + func (m *Mutator) SetPropertyWithEntity(prop string, value string, entity string) error { switch prop { case "overview_page": @@ -272,6 +327,18 @@ func (m *Mutator) SetActivityProperty(activityRef string, atPos int, prop string bsonnav.DSet(actDoc, "DueDate", value) return nil + case "annotation": + switch bsonnav.DGetString(actDoc, "$Type") { + case "Workflows$StartWorkflowActivity": + // The workflow's note, which describe prints in the header. + return m.setStartNote(value) + case "Workflows$EndWorkflowActivity", "Workflows$EndOfParallelSplitPathActivity", "Workflows$EndOfBoundaryEventPathActivity": + // Studio Pro's workflow editor draws no note on an end (11.15.0), + // and describe prints none: a note there is one nothing shows. + return fmt.Errorf("activity %q is an end of the flow, which shows no note", activityRef) + } + return setNote(actDoc, fmt.Sprintf("activity %q", activityRef), value) + default: return fmt.Errorf("unsupported activity property: %s", prop) } diff --git a/mdl/executor/alter_workflow_annotation_test.go b/mdl/executor/alter_workflow_annotation_test.go new file mode 100644 index 000000000..ea6a9572e --- /dev/null +++ b/mdl/executor/alter_workflow_annotation_test.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/backend/mock" +) + +// ako/mxcli#1717: `Annotation` reaches the mutator as the workflow's note +// without `on` and the activity's with it, and the empty string reaches it as +// the text that removes the note. A note is no template, so no parameters are +// set. +func TestAlterWorkflow_AnnotationReachesTheMutator(t *testing.T) { + var calls []string + rec := func(format string, a ...any) { calls = append(calls, fmt.Sprintf(format, a...)) } + mut := &mock.MockWorkflowMutator{ + SetPropertyFunc: func(prop, v string) error { rec("set %s=%q", prop, v); return nil }, + SetActivityPropertyFunc: func(ref string, pos int, prop, v string) error { + rec("set %s=%q on %s@%d", prop, v, ref, pos) + return nil + }, + SetTemplateParametersFunc: func(ref string, _ int, prop string, _ []string) error { + rec("params %s on %q", prop, ref) + return nil + }, + } + ctx := genericWfCtx(t, mut) + err := execAlterWf(t, ctx, `alter workflow Sales.WF { + set (Annotation: 'Started from the request form'); + set (Annotation: 'Check it') on task1; + set (Annotation: '') on 'Review'@2; +};`) + if err != nil { + t.Fatal(err) + } + want := []string{ + `set annotation="Started from the request form"`, + `set annotation="Check it" on task1@0`, + `set annotation="" on Review@2`, + } + if strings.Join(calls, "|") != strings.Join(want, "|") { + t.Errorf("calls = %v\nwant %v", calls, want) + } +} diff --git a/mdl/executor/bare_association.go b/mdl/executor/bare_association.go new file mode 100644 index 000000000..0d669b2a0 --- /dev/null +++ b/mdl/executor/bare_association.go @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// `retrieve $C from $O/Ord_Cust` — an association written by its bare name — +// passed check -p --references and exec, and was stored as the association +// ".Ord_Cust", with an empty module: CE1613 "The selected association +// '.Ord_Cust' no longer exists.", and a describe that does not parse +// (ako/mxcli#1690, measured on 11.13.0). The builder joined the AST's empty +// Module to the name. +// +// Which association a bare name means from an object is answered here once, +// as a function of the data its caller hands it (resolveBareAssociation): the +// flow builder feeds it the project as the statements before have left it, +// check --references the project overlaid with the associations the script +// declares before the flow — so the two cannot disagree about the rule +// (docs-wiki/bug-patterns/duplicate-resolver-drift.md). + +// generalizationChainFor is generalizationChain against a backend: entityQN +// followed by its generalizations, nearest first, and whether the walk reached +// the root. An incomplete chain is enough to answer "yes", never "no". +func generalizationChainFor(b entityLookupBackend, entityQN string) ([]string, bool) { + if b == nil || entityQN == "" { + return nil, false + } + var chain []string + seen := map[string]bool{} + for currentQN := entityQN; currentQN != ""; { + if seen[currentQN] { + return chain, false + } + seen[currentQN] = true + chain = append(chain, currentQN) + entity, ok := findEntityByQN(b, currentQN) + if !ok { + return chain, false + } + currentQN = entity.GeneralizationRef + } + return chain, true +} + +// associationCatalog is every association of a project with its two ends, +// keyed by lower-cased qualified name. unnamed holds the bare names of +// associations whose ends could not be named (an external entity, a dangling +// pointer): a bare name that matches one of them cannot be ruled out. +type associationCatalog struct { + shapes map[string]assocShape + unnamed map[string]bool + complete bool // the module list and every domain model were read +} + +// projectAssociationCatalog indexes the associations of the project behind b. +// A domain model is kept as two lists — an association to another module (or +// to System) is a CrossAssociation with the far end held by name — and both +// are read, or every association that leaves its module is missed. +func projectAssociationCatalog(modules []*model.Module, dms []*domainmodel.DomainModel) associationCatalog { + c := associationCatalog{shapes: map[string]assocShape{}, unnamed: map[string]bool{}, complete: true} + moduleNames := make(map[model.ID]string, len(modules)) + for _, m := range modules { + if m != nil { + moduleNames[m.ID] = m.Name + } + } + byID := map[model.ID]string{} + for _, dm := range dms { + if dm == nil { + continue + } + if mod := moduleNames[dm.ContainerID]; mod != "" { + for _, ent := range dm.Entities { + if ent != nil { + byID[ent.ID] = mod + "." + ent.Name + } + } + } + } + for _, dm := range dms { + if dm == nil { + continue + } + mod := moduleNames[dm.ContainerID] + if mod == "" { + c.complete = false + continue + } + for _, a := range dm.Associations { + if a == nil { + continue + } + from, to := byID[a.ParentID], byID[a.ChildID] + if from == "" || to == "" { + c.unnamed[a.Name] = true + continue + } + qn := mod + "." + a.Name + c.shapes[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: to, + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} + } + for _, a := range dm.CrossAssociations { + if a == nil { + continue + } + from := byID[a.ParentID] + if from == "" || a.ChildRef == "" { + c.unnamed[a.Name] = true + continue + } + qn := mod + "." + a.Name + c.shapes[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: a.ChildRef, + refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), + view: a.Source == domainmodel.OqlViewAssociationSource} + } + } + return c +} + +// associationCatalogFor reads the catalog through a backend. +func associationCatalogFor(b interface { + ListModules() ([]*model.Module, error) + ListDomainModels() ([]*domainmodel.DomainModel, error) +}) associationCatalog { + if b == nil { + return associationCatalog{} + } + modules, err := b.ListModules() + if err != nil { + return associationCatalog{} + } + dms, err := b.ListDomainModels() + if err != nil { + return associationCatalog{} + } + return projectAssociationCatalog(modules, dms) +} + +// bareAssocVerdict is the answer to "which association does this bare name +// mean from this entity". +type bareAssocVerdict int + +const ( + // bareAssocResolved: exactly one association of the name has the entity, + // or one of its generalizations, as an end. + bareAssocResolved bareAssocVerdict = iota + // bareAssocMissing: none does, the whole chain and every association + // were read. Refused. + bareAssocMissing + // bareAssocAmbiguous: more than one does — two modules may each have an + // association of the name. Refused: picking one is a guess. + bareAssocAmbiguous + // bareAssocUnknown: something could not be read, so no association was + // established. exec still refuses — the bare name cannot be written, it + // is stored with an empty module — and check stays silent. + bareAssocUnknown +) + +// resolveBareAssociation picks the association a bare name means when a flow +// follows it from an object: the one of that name with an end anywhere on the +// object's generalization chain (an association is traversable from either +// end, and a specialization inherits its generalizations' associations). +// +// chain is the entity followed by its generalizations; chainComplete whether +// it reached the root. matches lists every association that fits, sorted. +func resolveBareAssociation(name string, chain []string, chainComplete bool, cat associationCatalog) (qn string, matches []string, verdict bareAssocVerdict) { + onChain := make(map[string]bool, len(chain)) + for _, e := range chain { + onChain[e] = true + } + for _, sh := range cat.shapes { + if _, short, ok := strings.Cut(sh.qn, "."); !ok || short != name { + continue + } + if onChain[sh.from] || onChain[sh.to] { + matches = append(matches, sh.qn) + } + } + sort.Strings(matches) + switch { + case len(matches) == 1: + return matches[0], matches, bareAssocResolved + case len(matches) > 1: + return "", matches, bareAssocAmbiguous + case len(chain) == 0 || !chainComplete || !cat.complete || cat.unnamed[name]: + return "", nil, bareAssocUnknown + } + return "", nil, bareAssocMissing +} + +// bareRetrieveAssociationRefusal is the message exec refuses a bare-association +// retrieve with, and check reports it with: what the name could not be +// resolved to, and the qualified spelling that would work. entityQN is "" when +// the start variable's entity is not known. +func bareRetrieveAssociationRefusal(variable, start, name, entityQN string, matches []string, v bareAssocVerdict) string { + stmt := fmt.Sprintf("retrieve $%s from $%s/%s", variable, start, name) + const stored = "written as it stands it is stored with an empty module, and mxbuild reports CE1613 " + + "\"The selected association '.%s' no longer exists\"" + switch v { + case bareAssocAmbiguous: + return fmt.Sprintf("%s: %q could be any of %s from %s — qualify it with the one you mean (`$%s/%s`); "+stored, + stmt, name, strings.Join(matches, ", "), entityQN, start, matches[0], name) + case bareAssocMissing: + return fmt.Sprintf("%s: no association called %q has %s, or an entity it extends, at either end — "+ + "fix the name, or qualify it as `$%s/Module.%s`; "+stored, + stmt, name, entityQN, start, name, name) + } + why := "the entity $" + start + " holds is not known here" + if entityQN != "" { + why = "the model of " + entityQN + " could not be read in full" + } + return fmt.Sprintf("%s: the association is not qualified, and %s, so which association it names cannot be "+ + "worked out — qualify it as `$%s/Module.%s`; "+stored, stmt, why, start, name, name) +} diff --git a/mdl/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index 8bf889b0b..5451ac38e 100644 --- a/mdl/executor/cmd_alter_page.go +++ b/mdl/executor/cmd_alter_page.go @@ -23,6 +23,11 @@ func execAlterPage(ctx *ExecContext, s *ast.AlterPageStmt) error { if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // A parameter named with a platform-reserved word is CE7247 (MDL-PARAM01, + // ako/mxcli#1651) — refused here too, so --no-check cannot write it. + if err := refuseReservedParameterNames(s); err != nil { + return err + } h, err := getHierarchy(ctx) if err != nil { diff --git a/mdl/executor/cmd_associations.go b/mdl/executor/cmd_associations.go index 4ce82a91c..6ca48be9f 100644 --- a/mdl/executor/cmd_associations.go +++ b/mdl/executor/cmd_associations.go @@ -30,6 +30,14 @@ func execCreateAssociation(ctx *ExecContext, s *ast.CreateAssociationStmt) error return mdlerrors.NewValidationf("%s\n %s", vs[0].Message, vs[0].Suggestion) } + // The association's name becomes a member of its FROM entity (and of its + // TO entity under owner Both), which must be unique along that entity's + // generalization chain — CE0069 (ako/mxcli#1645). Before the module is + // auto-created, so a refusal leaves nothing behind. + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } + // Find or auto-create module module, err := findOrCreateModule(ctx, s.Name.Module) if err != nil { @@ -321,6 +329,11 @@ func execAlterAssociation(ctx *ExecContext, s *ast.AlterAssociationStmt) error { return mdlerrors.NewValidationf("%s\n %s", v.Message, v.Suggestion) } } + // Owner Both makes it a member of the TO entity too (CE0069, + // ako/mxcli#1645). + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } } // Try intra-module associations first diff --git a/mdl/executor/cmd_entities.go b/mdl/executor/cmd_entities.go index 78b198097..a6eb310cb 100644 --- a/mdl/executor/cmd_entities.go +++ b/mdl/executor/cmd_entities.go @@ -146,6 +146,14 @@ func execCreateEntity(ctx *ExecContext, s *ast.CreateEntityStmt) error { } } + // A member name must be unique along the generalization chain — up to the + // root, System entities included, and down through the specializations + // (CE0069, ako/mxcli#1645). The verdict check gives; before the module is + // auto-created, so a refusal leaves nothing behind. + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } + // Find or auto-create module module, err := findOrCreateModule(ctx, s.Name.Module) if err != nil { @@ -1305,6 +1313,10 @@ func execAlterEntity(ctx *ExecContext, s *ast.AlterEntityStmt) error { if isViewEntity(entity) { return viewEntityAttributeSetRefusal(s.Name.String(), "add", a.Name) } + // Not a name another member on the chain has (CE0069, ako/mxcli#1645). + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } // A specialization cannot store a system member (see // checkSpecializationSystemMembers): setting the flag here would be // dropped by the writer, which stores none on a Generalization. @@ -1437,6 +1449,10 @@ func execAlterEntity(ctx *ExecContext, s *ast.AlterEntityStmt) error { if target == nil { return mdlerrors.NewNotFoundMsg("attribute", s.AttributeName, fmt.Sprintf("attribute '%s' not found on entity %s", s.AttributeName, s.Name)) } + // Nor a rename into one (CE0069, ako/mxcli#1645). + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } target.Name = s.NewName // The entity's own access and validation rules hold the attribute's // qualified name as a string, so they have to move with it *in the model* diff --git a/mdl/executor/cmd_entities_bulk.go b/mdl/executor/cmd_entities_bulk.go index 8605a6946..956dd7956 100644 --- a/mdl/executor/cmd_entities_bulk.go +++ b/mdl/executor/cmd_entities_bulk.go @@ -39,6 +39,17 @@ func execAlterEntities(ctx *ExecContext, s *ast.AlterEntitiesStmt) error { return nil } + // Judge every target before writing to any: the per-entity path refuses a + // name on the chain too, but only after the targets before it were written + // (CE0069, ako/mxcli#1645). + names := make([]string, len(targets)) + for i, qn := range targets { + names[i] = qn.String() + } + if err := projectMemberWorld(ctx).bulkClash(names, s.Actions); err != nil { + return err + } + for _, qn := range targets { for _, action := range s.Actions { // Copy per entity: execAlterEntity reads Name, and the parsed diff --git a/mdl/executor/cmd_javaactions.go b/mdl/executor/cmd_javaactions.go index a5d2a77bf..cc7b33fec 100644 --- a/mdl/executor/cmd_javaactions.go +++ b/mdl/executor/cmd_javaactions.go @@ -480,6 +480,11 @@ func execCreateJavaAction(ctx *ExecContext, s *ast.CreateJavaActionStmt) error { if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // A parameter named with a platform-reserved word is CE7247 (MDL-PARAM01, + // ako/mxcli#1651) — refused here too, so --no-check cannot write it. + if err := refuseReservedParameterNames(s); err != nil { + return err + } // Get hierarchy for module/folder resolution h, err := getHierarchy(ctx) diff --git a/mdl/executor/cmd_javascript_actions_write.go b/mdl/executor/cmd_javascript_actions_write.go index d57c7c034..b5f7c057b 100644 --- a/mdl/executor/cmd_javascript_actions_write.go +++ b/mdl/executor/cmd_javascript_actions_write.go @@ -20,6 +20,11 @@ func execCreateJavaScriptAction(ctx *ExecContext, s *ast.CreateJavaScriptActionS if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // A parameter named with a platform-reserved word is CE7247 (MDL-PARAM01, + // ako/mxcli#1651) — refused here too, so --no-check cannot write it. + if err := refuseReservedParameterNames(s); err != nil { + return err + } // A text template is a Java action type only (MDL-JSA01). if bad := unsupportedJavaScriptActionTypes(s); len(bad) > 0 { return mdlerrors.NewValidationf("%s", strings.Join(bad, "\n")) diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 13496f30c..4862606c1 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -123,6 +123,10 @@ type flowBuilder struct { // is derived from the first annotated activity as before. startPosition *model.Point backend backend.FullBackend // For looking up page/microflow references + // assocCatalog is the project's associations with their ends, read once + // per flow for the bare association names a retrieve resolves + // (ako/mxcli#1690). nil until first needed. + assocCatalog *associationCatalog // quiet silences the builder's own stderr warnings. Set for describe's // layout check (derivedFlowLayout), which rebuilds a flow it only reads and // must not repeat, once per round, a warning nobody asked for. diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index fb4bb0997..cbdf6f816 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -1225,14 +1225,25 @@ func (fb *flowBuilder) addRetrieveAction(s *ast.RetrieveStmt) model.ID { // Association retrieve: RETRIEVE $List FROM $Parent/Module.AssocName // Always use AssociationRetrieveSource to preserve the original syntax. // The runtime resolves traversal direction from association metadata. - assocQN := s.Source.Module + "." + s.Source.Name + // + // A bare name (`$O/Ord_Cust`) resolves against the start variable's + // entity, or is refused: joined as it stands it was stored as ".Ord_Cust", + // with an empty module — CE1613, and a describe that does not parse + // (ako/mxcli#1690). + assocModule, assocName := s.Source.Module, s.Source.Name + if assocModule == "" { + if qn, ok := fb.qualifyBareRetrieveAssociation(s); ok { + assocModule, assocName, _ = strings.Cut(qn, ".") + } + } + assocQN := assocModule + "." + assocName // Look up association to determine type and direction. // For Reference associations, AssociationRetrieveSource always returns a single // object (the entity on the other end). When the user navigates from the child // (non-owner) side, the intent is to get a list of parent entities — we must use // a DatabaseRetrieveSource with XPath constraint instead. - assocInfo := fb.lookupAssociation(s.Source.Module, s.Source.Name) + assocInfo := fb.lookupAssociation(assocModule, assocName) startVarType := "" if fb.varTypes != nil { startVarType = fb.varTypes[s.StartVariable] @@ -1858,6 +1869,7 @@ func (fb *flowBuilder) addListOperationAction(s *ast.ListOperationStmt) model.ID if declared, ok := fb.resolveAttributeInEntityHierarchy(entityType, spec.Attribute); ok { attrQN = declared } else { + fb.refuseMissingListMember("sort", s.InputVariable, entityType, spec.Attribute) attrQN = entityType + "." + spec.Attribute } } @@ -1981,6 +1993,13 @@ func (fb *flowBuilder) listAttributeOperation(s *ast.ListOperationStmt, filter b return nil } + verb := "find" + if filter { + verb = "filter" + } + if fb.refuseMissingListMember(verb, s.InputVariable, fb.listElementEntity(s.InputVariable), fieldName) { + return nil + } attributeName, associationName := fb.resolveListOperationMember(s.InputVariable, fieldName) if associationName == "" && !strings.Contains(attributeName, ".") { return nil @@ -2080,11 +2099,15 @@ func (fb *flowBuilder) addAggregateListAction(s *ast.AggregateListStmt) model.ID action.UseExpression = true action.Expression = expressionToString(s.Expression) } else if s.Attribute != "" { - // For SUM/AVG/MIN/MAX, build qualified attribute name from variable type - if fb.varTypes != nil { - listType := fb.varTypes[s.InputVariable] - if after, ok := strings.CutPrefix(listType, "List of "); ok { - entityType := after + // For SUM/AVG/MIN/MAX, qualify the attribute with the entity that + // DECLARES it, as a sort does: an inherited one qualified with the + // list's own entity is CE1613 "The selected attribute 'M.Sub.Total' no + // longer exists" (measured on 11.13.0, ako/mxcli#1646). + if entityType := fb.listElementEntity(s.InputVariable); entityType != "" { + if declared, ok := fb.resolveAttributeInEntityHierarchy(entityType, s.Attribute); ok && !strings.Contains(s.Attribute, ".") { + action.AttributeQualifiedName = declared + } else { + fb.refuseMissingListMember(strings.ToLower(s.Operation.String()), s.InputVariable, entityType, s.Attribute) action.AttributeQualifiedName = entityType + "." + s.Attribute } } @@ -2663,6 +2686,56 @@ func (fb *flowBuilder) entityIsSubtypeOf(candidateQN, ancestorQN string) bool { return false } +// refuseMissingListMember records the refusal for a list operation's or an +// aggregate's member that the list's entity provably does not have, and +// reports whether it did. Without it the builder wrote `.` +// anyway, which mxbuild rejects with CE1613 (ako/mxcli#1646). The verdict and +// the message are check --references' own (flowMemberVerdict, +// listMemberRefusal), so exec without the preflight says what check says. A +// member that cannot be judged — the entity unknown, a module unreadable, a +// qualified name — is written as before. +func (fb *flowBuilder) refuseMissingListMember(verb, list, entityQN, member string) bool { + if fb == nil || fb.backend == nil || entityQN == "" || member == "" || strings.Contains(member, ".") { + return false + } + if res, _ := flowMemberVerdict(fb.backend, entityQN, member, !isAggregateVerb(verb)); res != memberMissing { + return false + } + fb.addError("%s", listMemberRefusal(listMemberWhere(verb, list, member), entityQN, member, nearMembersFor(fb.backend, entityQN))) + return true +} + +// qualifyBareRetrieveAssociation resolves the association a retrieve names by +// its bare name — `retrieve $C from $O/Ord_Cust` — against the entity the start +// variable holds, and records the refusal when it cannot. The answer is +// resolveBareAssociation's, which check --references asks too. +func (fb *flowBuilder) qualifyBareRetrieveAssociation(s *ast.RetrieveStmt) (string, bool) { + entity := "" + if fb.varTypes != nil { + entity = strings.TrimPrefix(fb.varTypes[s.StartVariable], "List of ") + } + qn, matches, verdict := fb.resolveBareAssociationFrom(entity, s.Source.Name) + if verdict == bareAssocResolved { + return qn, true + } + fb.addError("%s", bareRetrieveAssociationRefusal(s.Variable, s.StartVariable, s.Source.Name, entity, matches, verdict)) + return "", false +} + +// resolveBareAssociationFrom is resolveBareAssociation over the project as the +// builder sees it: the statements before this one already written. +func (fb *flowBuilder) resolveBareAssociationFrom(entityQN, name string) (string, []string, bareAssocVerdict) { + if fb == nil || fb.backend == nil || entityQN == "" || name == "" { + return "", nil, bareAssocUnknown + } + if fb.assocCatalog == nil { + cat := associationCatalogFor(fb.backend) + fb.assocCatalog = &cat + } + chain, complete := generalizationChainFor(fb.backend, entityQN) + return resolveBareAssociation(name, chain, complete, *fb.assocCatalog) +} + // resolveMemberChangeFallback preserves the authored member name shape when the // entity metadata is unavailable. // diff --git a/mdl/executor/cmd_pages_create_v3.go b/mdl/executor/cmd_pages_create_v3.go index c0d2fa157..6e07192b0 100644 --- a/mdl/executor/cmd_pages_create_v3.go +++ b/mdl/executor/cmd_pages_create_v3.go @@ -20,6 +20,11 @@ func execCreatePageV3(ctx *ExecContext, s *ast.CreatePageStmtV3) error { if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // A parameter named with a platform-reserved word is CE7247 (MDL-PARAM01, + // ako/mxcli#1651) — refused here too, so --no-check cannot write it. + if err := refuseReservedParameterNames(s); err != nil { + return err + } // Version pre-check: page parameters exist from Mendix 9.4, not 11.0. The // registry used to say 11.0 — a figure that came from an illustrative sample @@ -203,6 +208,11 @@ func execCreateSnippetV3(ctx *ExecContext, s *ast.CreateSnippetStmtV3) error { if !ctx.ConnectedForWrite() { return mdlerrors.NewNotConnectedWrite() } + // A parameter named with a platform-reserved word is CE7247 (MDL-PARAM01, + // ako/mxcli#1651) — refused here too, so --no-check cannot write it. + if err := refuseReservedParameterNames(s); err != nil { + return err + } // Find or auto-create module module, err := findOrCreateModule(ctx, s.Name.Module) diff --git a/mdl/executor/cmd_rename.go b/mdl/executor/cmd_rename.go index 4027ab660..b8cfcd04d 100644 --- a/mdl/executor/cmd_rename.go +++ b/mdl/executor/cmd_rename.go @@ -472,6 +472,11 @@ func execRenameAssociation(ctx *ExecContext, s *ast.RenameStmt) error { if collision { return mdlerrors.NewValidationf("association %s already exists in module %s", s.NewName, s.Name.Module) } + // The new name must also be free on its owners' generalization chains + // (CE0069, ako/mxcli#1645). + if err := refuseMemberNameClash(ctx, s); err != nil { + return err + } hits, err := ctx.Backend.RenameReferences(oldQualifiedName, newQualifiedName, s.DryRun) if err != nil { diff --git a/mdl/executor/cmd_workflows.go b/mdl/executor/cmd_workflows.go index d3452adb3..84fc531d7 100644 --- a/mdl/executor/cmd_workflows.go +++ b/mdl/executor/cmd_workflows.go @@ -169,9 +169,10 @@ func describeWorkflowToString(ctx *ExecContext, name ast.QualifiedName) (string, } // The note attached to the workflow's start. It was a `--` comment, which - // a replay dropped (ako/mxcli#707). - if targetWf.Annotation != "" { - lines = append(lines, fmt.Sprintf(" annotation %s", mdlQuoted(targetWf.Annotation))) + // a replay dropped (ako/mxcli#707); it is the start activity's, as Studio + // Pro shows it (ako/mxcli#1717). + if note := targetWf.StartNote(); note != "" { + lines = append(lines, fmt.Sprintf(" annotation %s", mdlQuoted(note))) } lines = append(lines, formatWorkflowEventHandlers(ctx, targetWf.EventHandlers)...) diff --git a/mdl/executor/cmd_workflows_write.go b/mdl/executor/cmd_workflows_write.go index 4022eef9f..87806fc26 100644 --- a/mdl/executor/cmd_workflows_write.go +++ b/mdl/executor/cmd_workflows_write.go @@ -194,7 +194,6 @@ func execCreateWorkflow(ctx *ExecContext, s *ast.CreateWorkflowStmt) error { headerNorm := newContextExprNormalizer(s.ParameterVar) wf.WorkflowNameParams = headerNorm.rewriteAll(workflowTemplateExprs(s.DisplayParams)) wf.WorkflowDescriptionParams = headerNorm.rewriteAll(workflowTemplateExprs(s.DescriptionParams)) - wf.Annotation = s.Annotation if s.ExportLevel != "" { wf.ExportLevel = s.ExportLevel } @@ -218,6 +217,10 @@ func execCreateWorkflow(ctx *ExecContext, s *ast.CreateWorkflowStmt) error { startAct.ID = model.ID(generateWorkflowUUID()) startAct.Caption = "Start" startAct.Name = "Start" + // The header's `annotation '…'` is the note on the start, which Studio Pro + // stores on the start activity and shows from there; the workflow's own + // Annotation, where it was written before, it shows nowhere (ako/mxcli#1717). + startAct.Annotation = s.Annotation // Add implicit end activity endAct := &workflows.EndWorkflowActivity{} diff --git a/mdl/executor/expression_member_refs_1697_test.go b/mdl/executor/expression_member_refs_1697_test.go new file mode 100644 index 000000000..fdcc98276 --- /dev/null +++ b/mdl/executor/expression_member_refs_1697_test.go @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + "testing" +) + +// ako/mxcli#1697 (mendixlabs/mxcli#1650): `$Var/` in an expression, +// for a member the variable's entity does not have, passed check -p +// --references — the type pass even said "Expression types OK" — and exec +// wrote it; mxbuild then reported CE0117 "Error(s) in expression.". The +// reported shapes are the system `id` and a misspelling, on System.User and on +// an entity the same script creates. +// +// Measured on 11.13.0, one microflow per shape, in an app with no other error +// (a CE1613 elsewhere suppresses every CE0117 — mx check then reports none): +// +// CE0117 $O/id, $O/id = $S/id, $U/Bogus, $O/Bogus, $O/number (for Number), +// $O/CreatedDate, $O/changedDate on an entity that does not store it, +// $O/owner, $O/Ord_Cust/Name (a bare association), +// $O/Mod.Nope/Name, $O/Mod.Ord_Cust/Mod.Cust/Nam +// clean $O/Number, an inherited attribute (also from another module and +// from System.User), $O/Mod.Ord_Cust/Name, $O/createdDate where stored +const exprMemberHead = `mdl 1; +create module B1697; +create module O1697; +create persistent entity B1697.Cust (Name: String(50)); +create persistent entity B1697.Ord (Number: String(20), createdDate: AutoCreatedDate); +create persistent entity B1697.SubOrd extends B1697.Ord (Extra: String(20)); +create association B1697.Ord_Cust from B1697.Ord to B1697.Cust; +create persistent entity O1697.Base (BaseName: String(20)); +create persistent entity B1697.Spec extends O1697.Base (SpecName: String(20)); +` + +const exprMemberFlow = `create or modify microflow B1697.F ($O: B1697.Ord, $S: B1697.SubOrd, $U: System.User, $A: Administration.Account, $Sp: B1697.Spec) returns %s as $r +begin + declare $r %s = %s; + return $r; +end; +` + +func exprMemberScript(typ, expr string) string { + return exprMemberHead + fmt.Sprintf(exprMemberFlow, typ, typ, expr) +} + +// The reported shapes and their siblings: check -p reports each, naming the +// member and the entity, and the id with the way to get what it was for. +func TestExpressionMissingMember_Reported(t *testing.T) { + cases := []struct{ typ, expr, want string }{ + {"String", "toString($U/id)", `$U/id: System.User has no attribute "id"`}, + {"String", "toString($O/id)", "object's id is not readable in an expression"}, + {"Boolean", "$O/id = $S/id", `$S/id: B1697.SubOrd has no attribute "id"`}, + {"String", "$U/Bogus", `$U/Bogus: System.User has no attribute "Bogus"`}, + {"String", "$O/Bogus", `$O/Bogus: B1697.Ord has no attribute "Bogus"`}, + {"String", "$O/number", `$O/number: B1697.Ord has no attribute "number"`}, + {"DateTime", "$O/CreatedDate", "the system member is spelled `createdDate`"}, + {"DateTime", "$O/changedDate", `$O/changedDate: B1697.Ord has no attribute "changedDate"`}, + {"String", "toString($O/owner)", "the system member is spelled `System.owner`"}, + {"String", "$O/Ord_Cust/Name", "Ord_Cust is an association: an expression names it qualified, `$O/B1697.Ord_Cust/…`"}, + {"String", "$O/B1697.Nope/Name", `$O/B1697.Nope/Name: there is no association B1697.Nope`}, + {"String", "$O/B1697.Ord_Cust/B1697.Cust/Nam", `$O/B1697.Ord_Cust/B1697.Cust/Nam: B1697.Cust has no attribute "Nam"`}, + } + for _, c := range cases { + t.Run(c.expr, func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + got := strings.Join(agreeCheck(t, exec, dir, exprMemberScript(c.typ, c.expr)), "\n") + if !strings.Contains(got, c.want) { + t.Errorf("check -p must report %q, reported:\n%s", c.want, got) + } + if !strings.Contains(got, "CE0117") { + t.Errorf("the report must name what mxbuild says (CE0117), reported:\n%s", got) + } + }) + } +} + +// What mxbuild builds clean is not reported. +func TestExpressionMember_ControlsAccepted(t *testing.T) { + cases := []struct{ typ, expr string }{ + {"String", "$O/Number"}, + {"String", "$S/Number"}, + {"String", "$Sp/BaseName"}, + {"String", "$U/Name"}, + {"String", "$A/Name"}, + {"String", "$A/FullName"}, + {"String", "$O/B1697.Ord_Cust/Name"}, + {"String", "$O/B1697.Ord_Cust/B1697.Cust/Name"}, + // The target entity by its bare name: the builder qualifies it (#829). + {"String", "$O/B1697.Ord_Cust/Cust/Name"}, + {"DateTime", "$O/createdDate"}, + {"DateTime", "$S/createdDate"}, + } + for _, c := range cases { + t.Run(c.expr, func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, exprMemberScript(c.typ, c.expr), "") + }) + } +} + +// The reported repro, against the project: System.User is read from it. +func TestExpressionMissingMember_SystemUserRepro(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + const src = `create module ModI; +create microflow ModI.MF_UserIdText ($User: System.User) returns String as $r +begin + declare $r String = toString($User/id); + return $r; +end; +` + got := strings.Join(agreeCheck(t, exec, dir, src), "\n") + for _, want := range []string{`$User/id: System.User has no attribute "id"`, "CommunityCommons.getGUID"} { + if !strings.Contains(got, want) { + t.Errorf("check -p must report %q, reported:\n%s", want, got) + } + } +} + +// An expression in a nested block and in a loop over a typed list is judged +// against the entity the variable holds there. +func TestExpressionMissingMember_NestedAndLoop(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := exprMemberHead + `create microflow B1697.G ($l: List of B1697.Ord) returns Boolean +begin + loop $it in $l + begin + if $it/Numbr = 'x' then + log info node 'n' 'found'; + end if; + end loop; + return true; +end; +` + got := strings.Join(agreeCheck(t, exec, dir, src), "\n") + if want := `$it/Numbr: B1697.Ord has no attribute "Numbr"`; !strings.Contains(got, want) { + t.Errorf("check -p must report %q, reported:\n%s", want, got) + } +} diff --git a/mdl/executor/flow_member_spelling.go b/mdl/executor/flow_member_spelling.go index 449cd1aef..50c1deb56 100644 --- a/mdl/executor/flow_member_spelling.go +++ b/mdl/executor/flow_member_spelling.go @@ -93,6 +93,14 @@ func (s *memberSpeller) respell(st ast.MicroflowStatement) { switch x := st.(type) { case *ast.RetrieveStmt: if x.StartVariable != "" { + // describe prints the stored association, which is qualified; a + // bare one is the same retrieve when the builder resolves it to + // that association (ako/mxcli#1690). + if x.Source.Module == "" { + if qn, _, v := s.fb.resolveBareAssociationFrom(s.entities[x.StartVariable], x.Source.Name); v == bareAssocResolved { + x.Source.Module, x.Source.Name, _ = strings.Cut(qn, ".") + } + } return } for i := range x.SortColumns { diff --git a/mdl/executor/list_member_refs_1646_test.go b/mdl/executor/list_member_refs_1646_test.go new file mode 100644 index 000000000..6c8e83969 --- /dev/null +++ b/mdl/executor/list_member_refs_1646_test.go @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "fmt" + "strings" + "testing" +) + +// ako/mxcli#1646: find, filter, sort or an aggregate by a member the list's +// entity does not have passed check -p and exec, and mxbuild then failed once +// per activity (measured on 11.12.5 and again on 11.13.0): +// +// [CE1613] "The selected attribute 'Shop.Order.Numbr' no longer exists." at list operation activity 'Find' +// [CE1613] "The selected attribute 'Shop.Order.Totl' no longer exists." at aggregate list activity … +// +// The builder resolved the bare name and, when it named nothing, fell back to +// writing `.` anyway — the fabricated attribute mxbuild rejects — +// while the same misspelling in a retrieve constraint was refused (#1213). Now +// the builder and check ask one question, flowMemberVerdict, and refuse a name +// that is neither an attribute, an association nor a stored system member of +// the entity or anything it extends. +const listMemberHead = `mdl 1; +create module B1646; +create module O1646; +create persistent entity B1646.Cust (Name: String(50)); +create persistent entity B1646.Ord (Number: String(20), Total: Decimal, owner: AutoOwner, createdDate: AutoCreatedDate); +create persistent entity B1646.SubOrd extends B1646.Ord (Extra: String(20)); +create association B1646.Ord_Cust from B1646.Ord to B1646.Cust; +create persistent entity O1646.Base (BaseName: String(20)); +create persistent entity O1646.Thing (ThingName: String(20)); +create association O1646.Base_Thing from O1646.Base to O1646.Thing; +create persistent entity B1646.Spec extends O1646.Base (SpecName: String(20)); +` + +const listMemberFlow = `create or modify microflow B1646.F ($l: List of B1646.Ord, $sl: List of B1646.SubOrd, $spl: List of B1646.Spec, $c: B1646.Cust, $th: O1646.Thing, $u: System.User) returns Boolean +begin + %s + return true; +end; +` + +// The reported misspellings: check -p and exec both refuse each, naming the +// entity and the member, rather than writing the fabricated attribute. +func TestListOperationMissingMember_Refused(t *testing.T) { + cases := []struct{ stmt, want string }{ + {"$m = find $l by Numbr = '42';", `find $l by Numbr: B1646.Ord has no member "Numbr"`}, + {"$m = filter $l by Totl = 5;", `filter $l by Totl: B1646.Ord has no member "Totl"`}, + {"$m = sort $l by Totl asc;", `sort $l by Totl: B1646.Ord has no member "Totl"`}, + {"$m = sum $l by Totl;", `sum $l by Totl: B1646.Ord has no member "Totl"`}, + {"$m = maximum $sl by Extr;", `maximum $sl by Extr: B1646.SubOrd has no member "Extr"`}, + // `owner` is the association System.owner, which a bare name does not + // reach: written as the attribute B1646.Ord.owner, CE1613 (11.13.0). + {"$m = find $l by owner = $u;", "the system member is spelled `System.owner`"}, + // A sort reads a stored system date; an aggregate does not: CE0226, + // on the entity that stores it and on a specialization (11.13.0). + {"$m = maximum $l by createdDate;", "maximum $l by createdDate: an aggregate cannot read the system member createdDate"}, + {"$m = maximum $sl by createdDate;", "CE0226"}, + } + for _, c := range cases { + t.Run(c.stmt, func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, listMemberHead+fmt.Sprintf(listMemberFlow, c.stmt), c.want) + }) + } +} + +// What mxbuild builds clean stays accepted (each measured with mx check on +// 11.13.0): an own and an inherited attribute, an own association by its bare +// name, one inherited from a generalization in another module (#1637), the +// System.owner association (#1629), and the system dates the root stores. A +// list whose entity is not known (a call's result) is not judged at all. +func TestListOperationMember_ControlsAccepted(t *testing.T) { + stmts := []string{ + "$m = find $sl by Number = '1';", + "$m = sort $sl by Number asc;", + "$m = sum $sl by Total;", + "$m = find $l by Ord_Cust = $c;", + "$m = find $spl by Base_Thing = $th;", + "$m = find $l by System.owner = $u;", + "$m = sort $l by createdDate asc;", + "$m = sort $sl by createdDate desc;", + "$m = sort $spl by BaseName desc;", + } + for _, stmt := range stmts { + t.Run(stmt, func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, listMemberHead+fmt.Sprintf(listMemberFlow, stmt), "") + }) + } + t.Run("untyped list", func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := listMemberHead + `create microflow B1646.Src () returns List of B1646.Ord +begin + retrieve $all from B1646.Ord; + return $all; +end; +create microflow B1646.G ($x: Integer) returns Boolean +begin + $l = call microflow B1646.Src(); + $m = sort $l by Number asc; + return true; +end; +` + assertAgree(t, exec, dir, src, "") + }) +} + +// An aggregate by an INHERITED attribute was written against the list's own +// entity — `B1646.SubOrd.Total` — which mxbuild reports as CE1613 "The selected +// attribute 'B1646.SubOrd.Total' no longer exists" (measured on 11.13.0). It is +// qualified against the entity that declares it, as a sort already was +// (mendixlabs/mxcli#1430). +func TestAggregateInheritedAttribute_StoresTheDeclaringEntity(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, listMemberHead+fmt.Sprintf(listMemberFlow, "$m = sum $sl by Total;"), "") + unit := microflowUnit(t, exec, "B1646", "F") + if bytes.Contains(unit, []byte("B1646.SubOrd.Total")) { + t.Fatal("stored the inherited attribute as B1646.SubOrd.Total — CE1613") + } + if !bytes.Contains(unit, []byte("B1646.Ord.Total")) { + t.Fatal("the aggregate does not name B1646.Ord.Total") + } +} + +// The builder's refusal and check's report are one message: an exec that +// skipped the preflight (exec -c, the REPL, --no-check) says what check says. +func TestListOperationMissingMember_ExecSaysWhatCheckSays(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := listMemberHead + fmt.Sprintf(listMemberFlow, "$m = find $l by Numbr = '42';") + checkSays := strings.Join(agreeCheck(t, exec, dir, src), "\n") + err := agreeExec(t, exec, src) + if err == nil { + t.Fatal("exec accepted find by a missing member") + } + const msg = `find $l by Numbr: B1646.Ord has no member "Numbr" — it has Number, Total` + if !strings.Contains(checkSays, msg) || !strings.Contains(err.Error(), msg) { + t.Errorf("check and exec must both say %q\ncheck: %s\nexec: %v", msg, checkSays, err) + } +} + +// A member the same script adds before the flow is not refused: check leaves +// an altered entity unjudged, and exec's builder runs after the alter. +// `create or modify` of a STORED flow is the shape where check's flow-verdict +// simulation builds the declared flow against the project as stored. +func TestListOperationMember_AddedByTheScriptIsAccepted(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + if err := agreeExec(t, exec, listMemberHead+fmt.Sprintf(listMemberFlow, "$m = find $l by Number = '1';")); err != nil { + t.Fatalf("setup: %v", err) + } + src := "mdl 1;\nalter entity B1646.Ord add attribute Code: String(10);\n" + + fmt.Sprintf(listMemberFlow, "$m = find $l by Code = 'x';\n $s = sort $l by Code asc;\n $t = sum $l by Total;") + assertAgree(t, exec, dir, src, "") +} diff --git a/mdl/executor/member_name_clash.go b/mdl/executor/member_name_clash.go new file mode 100644 index 000000000..f5199eafa --- /dev/null +++ b/mdl/executor/member_name_clash.go @@ -0,0 +1,942 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/meta" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// A member name must be unique along an entity's generalization chain +// (ako/mxcli#1645). mxbuild reports a second one as +// +// CE0069 "Duplicate member name 'X'." +// +// and that is the only place it was reported: check passed it and exec wrote +// it. What counts, measured on mx 11.13.0 in a blank app (every case below is +// one CE0069, every "no" builds clean): +// +// - an attribute or association whose name an ANCESTOR's attribute or +// association already has — attribute/attribute, association/association +// (cross-module), attribute/association both ways, two generalization +// levels up, and System entities' members too: `extends System.User` +// with `Name` collides with System.User.Name, with `UserRoles` with the +// association System.UserRoles; `extends System.FileDocument` with `Size`. +// - the other direction: adding to a generalization a member one of its +// SPECIALIZATIONS already declares. +// - an attribute and an association of the SAME entity. +// - a rename into such a name — of an attribute or of an association. +// - an association is a member of its FROM entity; under owner Both it is a +// member of its TO entity as well (reference and reference set alike). +// Under owner Default, an attribute on the TO entity named like it: no. +// - names that differ only in case: no. The comparison is exact. +// - two siblings declaring the same name, or an unrelated entity: no. +// +// memberWorld is the slice of the domain model that decides it, and +// memberNameClash the one verdict. exec judges a statement against the project +// as it stands (projectMemberWorld); check judges each statement against the +// project with the script's earlier statements applied (validateMemberNameClashes), +// because the ordinary shape is one script that creates the generalization and +// the specialization both. + +// memberWorld holds each entity's generalization and own attributes, and each +// association's ends. The members of an entity are derived from both. +type memberWorld struct { + entities map[string]*worldEntity // keyed by exact qualified name + assocs map[string]*worldAssoc // keyed by exact qualified name + // uneditable are modules a bulk ALTER ENTITIES without IN skips (System, + // Marketplace) — see resolveBulkEntityTargets. + uneditable map[string]bool +} + +type worldEntity struct { + gen string + attrs []string + persistable bool + // notBulk marks a view or external entity, which ALTER ENTITIES never + // targets (entityMatchesFilter). + notBulk bool +} + +type worldAssoc struct { + name string + from string // entity qualified name + to string // entity qualified name + both bool // owner Both: a member of the TO entity too +} + +// worldMember is one member of an entity: what it is called and what it is. +type worldMember struct { + name string + owner string // the entity that has it as its own member + ref string // Module.Entity.Attribute, or Module.Association + assoc bool +} + +func (m worldMember) kind() string { + if m.assoc { + return "association" + } + return "attribute" +} + +func newMemberWorld() *memberWorld { + return &memberWorld{ + entities: map[string]*worldEntity{}, + assocs: map[string]*worldAssoc{}, + uneditable: map[string]bool{}, + } +} + +// projectMemberWorld reads the member world from the connected project. It +// includes the System module, whose entities' members a specialization of one +// inherits like any other's. Returns nil when the domain model cannot be read, +// which every caller takes as "nothing to judge". +func projectMemberWorld(ctx *ExecContext) *memberWorld { + if ctx == nil || !ctx.Connected() { + return nil + } + modules, err := getModulesFromCache(ctx) + if err != nil { + return nil + } + dms, err := ctx.Backend.ListDomainModels() + if err != nil { + return nil + } + moduleNames := make(map[model.ID]string, len(modules)) + w := newMemberWorld() + for _, m := range modules { + moduleNames[m.ID] = m.Name + if isUneditableModule(m) { + w.uneditable[m.Name] = true + } + } + entityQN := map[model.ID]string{} + for _, dm := range dms { + mod := moduleNames[dm.ContainerID] + if mod == "" { + continue + } + for _, e := range dm.Entities { + if e == nil { + continue + } + qn := mod + "." + e.Name + entityQN[e.ID] = qn + we := &worldEntity{gen: e.GeneralizationRef, persistable: e.Persistable, + notBulk: e.Source != "" || e.OqlQuery != ""} + for _, a := range e.Attributes { + if a != nil { + we.attrs = append(we.attrs, a.Name) + } + } + w.entities[qn] = we + } + } + for _, dm := range dms { + mod := moduleNames[dm.ContainerID] + if mod == "" { + continue + } + for _, a := range dm.Associations { + if a == nil { + continue + } + w.assocs[mod+"."+a.Name] = &worldAssoc{ + name: a.Name, from: entityQN[a.ParentID], to: entityQN[a.ChildID], + both: a.Owner == domainmodel.AssociationOwnerBoth, + } + } + for _, ca := range dm.CrossAssociations { + if ca == nil { + continue + } + w.assocs[mod+"."+ca.Name] = &worldAssoc{ + name: ca.Name, from: entityQN[ca.ParentID], to: ca.ChildRef, + both: ca.Owner == domainmodel.AssociationOwnerBoth, + } + } + } + // A backend that lists no System domain model still has the System + // entities: take them from modelsdk/meta, as a project-free check does. + if _, ok := w.entities["System.User"]; !ok { + sys := systemMemberWorld() + for qn, e := range sys.entities { + if _, have := w.entities[qn]; !have { + w.entities[qn] = e + } + } + for qn, a := range sys.assocs { + if _, have := w.assocs[qn]; !have { + w.assocs[qn] = a + } + } + w.uneditable["System"] = true + } + return w +} + +// members lists an entity's own members: its attributes, then the associations +// it is an end of as a member (FROM always, TO under owner Both), by name. +func (w *memberWorld) members(qn string) []worldMember { + var out []worldMember + if e := w.entities[qn]; e != nil { + for _, a := range e.attrs { + out = append(out, worldMember{name: a, owner: qn, ref: qn + "." + a}) + } + } + var assocs []worldMember + for aqn, a := range w.assocs { + if a.from == qn || (a.both && a.to == qn) { + assocs = append(assocs, worldMember{name: a.name, owner: qn, ref: aqn, assoc: true}) + } + } + sort.Slice(assocs, func(i, j int) bool { return assocs[i].ref < assocs[j].ref }) + return append(out, assocs...) +} + +// ancestors walks the generalization chain above qn, nearest first, as far as +// it resolves in the world. +func (w *memberWorld) ancestors(qn string) []string { + var out []string + seen := map[string]bool{qn: true} + for e := w.entities[qn]; e != nil && e.gen != "" && !seen[e.gen]; e = w.entities[e.gen] { + seen[e.gen] = true + if w.entities[e.gen] == nil { + break + } + out = append(out, e.gen) + } + return out +} + +// descendants lists every entity whose chain passes through qn, nearest first. +func (w *memberWorld) descendants(qn string) []string { + children := map[string][]string{} + for cqn, e := range w.entities { + if e.gen != "" { + children[e.gen] = append(children[e.gen], cqn) + } + } + var out []string + seen := map[string]bool{qn: true} + queue := []string{qn} + for len(queue) > 0 { + cur := queue[0] + queue = queue[1:] + kids := children[cur] + sort.Strings(kids) + for _, k := range kids { + if !seen[k] { + seen[k] = true + out = append(out, k) + queue = append(queue, k) + } + } + } + return out +} + +// memberClash is a member already holding the name a statement gives another. +type memberClash struct { + subject string // the entity gaining the new member + existing worldMember // the member that already has the name + relation string // "own", "ancestor" or "descendant" + // via names the entity whose new generalization brings the inherited + // member, when that — not the member itself — is what the statement changes. + via string +} + +// findClash looks for a member called name anywhere on subject's chain — its +// own members, its ancestors', its specializations' — other than one skip +// accepts. The comparison is exact: mxbuild does not report two names that +// differ only in case. +func (w *memberWorld) findClash(subject, name string, skip func(worldMember) bool) *memberClash { + look := func(qn, relation string) *memberClash { + for _, m := range w.members(qn) { + if m.name == name && (skip == nil || !skip(m)) { + return &memberClash{subject: subject, existing: m, relation: relation} + } + } + return nil + } + if c := look(subject, "own"); c != nil { + return c + } + for _, a := range w.ancestors(subject) { + if c := look(a, "ancestor"); c != nil { + return c + } + } + for _, d := range w.descendants(subject) { + if c := look(d, "descendant"); c != nil { + return c + } + } + return nil +} + +// clashError words a clash for the member about to be added: what it is, the +// member that already has its name and where that member comes from, and the +// way out. +func clashError(newKind, newRef, name string, c *memberClash) error { + e := c.existing + ce := fmt.Sprintf("mxbuild rejects a second one with CE0069 \"Duplicate member name '%s'.\"", name) + var why, fix string + switch c.relation { + case "own": + why = fmt.Sprintf("%s already has %s %s — an entity's attributes and associations share one name space, so %s", + c.subject, e.kind(), e.ref, ce) + fix = fmt.Sprintf("give the %s another name", newKind) + case "ancestor": + inherits := "inherits" + if c.via != "" { + inherits = "would inherit" + } + why = fmt.Sprintf("%s %s '%s' from %s (%s %s); a member name must be unique along the generalization chain, so %s", + c.subject, inherits, name, e.owner, e.kind(), e.ref, ce) + if c.via != "" { + why = fmt.Sprintf("through the new generalization of %s, %s", c.via, why) + } + fix = fmt.Sprintf("give the %s another name", newKind) + if !e.assoc && newKind == "attribute" { + fix += ", or drop it and use the inherited one" + } + default: + why = fmt.Sprintf("%s's specialization %s already has '%s' (%s %s) and would inherit this one too; a member name must be unique along the generalization chain, so %s", + c.subject, e.owner, name, e.kind(), e.ref, ce) + fix = fmt.Sprintf("give the %s another name, or rename or drop %s first", newKind, e.ref) + } + return mdlerrors.NewValidationf("%s %s: %s — %s", newKind, newRef, why, fix) +} + +// memberNameClash is the verdict check and exec share: the error for a +// statement that would give some entity's generalization chain two members of +// one name, judged against w; nil when it adds no member, or none clashes, or +// w is nil. A statement exec refuses for another reason — the entity is +// missing, the attribute or association already exists — is left to that +// refusal rather than reported twice. +func memberNameClash(w *memberWorld, stmt ast.Statement) error { + if w == nil { + return nil + } + switch s := stmt.(type) { + case *ast.CreateEntityStmt: + return w.entityDeclClash(s) + case *ast.AlterEntityStmt: + return w.alterEntityClash(s) + case *ast.CreateAssociationStmt: + return w.createAssociationClash(s) + case *ast.AlterAssociationStmt: + return w.alterAssociationClash(s) + case *ast.RenameStmt: + if s.ObjectType == "association" { + return w.renameAssociationClash(s) + } + } + return nil +} + +// declaredAttrNames are the attributes a declaration stores: its AutoX +// pseudo-types become entity flags, not attributes. +func declaredAttrNames(attrs []ast.Attribute) []string { + var out []string + for _, a := range attrs { + if systemMemberOfPseudoType(a.Type.Kind) != "" { + continue + } + out = append(out, a.Name) + } + return out +} + +func (w *memberWorld) entityDeclClash(s *ast.CreateEntityStmt) error { + qn := s.Name.String() + old, exists := w.entities[qn] + if exists && (s.IfNotExists || !s.CreateOrModify) { + return nil // skipped, or refused as already existing + } + gen := "" + if s.Generalization != nil { + gen = s.Generalization.String() + } + declared := declaredAttrNames(s.Attributes) + had := map[string]bool{} + if exists { + for _, a := range old.attrs { + had[a] = true + } + } + hyp := w.withEntity(qn, &worldEntity{gen: gen, attrs: declared, persistable: s.Kind != ast.EntityNonPersistent}) + + // A declared attribute the entity did not have meets everything on its + // chain. Another declared attribute of the same name is not this rule's — + // the attribute list itself is. + for _, name := range declared { + if had[name] { + continue + } + if c := hyp.findClash(qn, name, func(m worldMember) bool { return m.owner == qn && !m.assoc }); c != nil { + return clashError("attribute", qn+"."+name, name, c) + } + } + if exists && old.gen == gen { + return nil // the rest of the chain is as it was + } + // A new generalization brings new inherited members, which every member + // already below it meets: the entity's own, and its specializations'. + above := hyp.ancestors(qn) + meet := func(subject string, m worldMember) error { + for _, a := range above { + for _, o := range hyp.members(a) { + if o.name == m.name { + c := &memberClash{subject: subject, existing: o, relation: "ancestor"} + if subject != qn { + c.via = qn + } + return clashError(m.kind(), m.ref, m.name, c) + } + } + } + return nil + } + for _, d := range append([]string{qn}, hyp.descendants(qn)...) { + for _, m := range hyp.members(d) { + if err := meet(d, m); err != nil { + return err + } + } + } + return nil +} + +func (w *memberWorld) alterEntityClash(s *ast.AlterEntityStmt) error { + qn := s.Name.String() + e := w.entities[qn] + if e == nil { + return nil // refused as not found + } + switch s.Operation { + case ast.AlterEntityAddAttribute: + if s.Attribute == nil || systemMemberOfPseudoType(s.Attribute.Type.Kind) != "" { + return nil + } + name := s.Attribute.Name + if containsString(e.attrs, name) { + return nil // skipped (IF NOT EXISTS) or refused as existing + } + if c := w.findClash(qn, name, nil); c != nil { + return clashError("attribute", qn+"."+name, name, c) + } + case ast.AlterEntityRenameAttribute: + if s.NewName == s.AttributeName || !containsString(e.attrs, s.AttributeName) || containsString(e.attrs, s.NewName) { + return nil + } + self := qn + "." + s.AttributeName + if c := w.findClash(qn, s.NewName, func(m worldMember) bool { return m.ref == self }); c != nil { + return clashError("attribute", qn+"."+s.NewName, s.NewName, c) + } + } + return nil +} + +// createAssociationEnds resolves a CREATE ASSOCIATION's FROM and TO entities the way +// exec does: an unqualified end is in the association's module. +func createAssociationEnds(s *ast.CreateAssociationStmt) (from, to string) { + pm, cm := s.Parent.Module, s.Child.Module + if pm == "" { + pm = s.Name.Module + } + if cm == "" { + cm = s.Name.Module + } + return pm + "." + s.Parent.Name, cm + "." + s.Child.Name +} + +// crossModuleEnds reports whether a statement's two ends are in different +// modules — exec's isCrossModule, which decides whether OR MODIFY moves the TO +// end. +func crossModuleEnds(from, to string) bool { + fm, _, _ := strings.Cut(from, ".") + tm, _, _ := strings.Cut(to, ".") + return fm != tm +} + +// owners are the entities an association is a member of. +func (a *worldAssoc) owners() []string { + if a.both && a.to != "" && a.to != a.from { + return []string{a.from, a.to} + } + return []string{a.from} +} + +func (w *memberWorld) createAssociationClash(s *ast.CreateAssociationStmt) error { + qn := s.Name.String() + from, to := createAssociationEnds(s) + both := s.Owner == ast.OwnerBoth + before := map[string]bool{} + after := &worldAssoc{name: s.Name.Name, from: from, to: to, both: both} + if old := w.assocs[qn]; old != nil { + if s.IfNotExists || !s.CreateOrModify { + return nil // skipped, or refused as already existing + } + for _, o := range old.owners() { + before[o] = true + } + // OR MODIFY keeps the FROM end; it moves the TO end of a cross-module + // association only (execCreateAssociation). + after.from = old.from + if !crossModuleEnds(from, to) { + after.to = old.to + } + } + for _, o := range after.owners() { + if o == "" || before[o] || w.entities[o] == nil { + continue + } + if c := w.findClash(o, s.Name.Name, func(m worldMember) bool { return m.ref == qn }); c != nil { + return clashError("association", qn, s.Name.Name, c) + } + } + return nil +} + +func (w *memberWorld) alterAssociationClash(s *ast.AlterAssociationStmt) error { + if s.Operation != ast.AlterAssociationSetOwner || s.Owner != ast.OwnerBoth { + return nil + } + qn := s.Name.String() + a := w.assocs[qn] + if a == nil || a.both || a.to == "" || a.to == a.from || w.entities[a.to] == nil { + return nil + } + if c := w.findClash(a.to, a.name, func(m worldMember) bool { return m.ref == qn }); c != nil { + return clashError("association", qn, a.name, c) + } + return nil +} + +func (w *memberWorld) renameAssociationClash(s *ast.RenameStmt) error { + oldQN := s.Name.String() + a := w.assocs[oldQN] + if a == nil || s.NewName == a.name || w.assocs[s.Name.Module+"."+s.NewName] != nil { + return nil // refused as not found, or as a name the module already has + } + for _, o := range a.owners() { + if w.entities[o] == nil { + continue + } + if c := w.findClash(o, s.NewName, func(m worldMember) bool { return m.ref == oldQN }); c != nil { + return clashError("association", s.Name.Module+"."+s.NewName, s.NewName, c) + } + } + return nil +} + +// withEntity is w with one entity replaced, sharing everything else. +func (w *memberWorld) withEntity(qn string, e *worldEntity) *memberWorld { + entities := make(map[string]*worldEntity, len(w.entities)+1) + for k, v := range w.entities { + entities[k] = v + } + entities[qn] = e + return &memberWorld{entities: entities, assocs: w.assocs, uneditable: w.uneditable} +} + +// apply replays a statement's effect on the members, for check's ordered pass. +// It follows what exec does closely enough that a later statement is judged +// against the model it will meet; a statement it does not model leaves the +// world as it was, which can only make check quieter, never refuse more. +func (w *memberWorld) apply(stmt ast.Statement) { + switch s := stmt.(type) { + case *ast.CreateEntityStmt: + qn := s.Name.String() + if _, exists := w.entities[qn]; exists && (s.IfNotExists || !s.CreateOrModify) { + return + } + gen := "" + if s.Generalization != nil { + gen = s.Generalization.String() + } + w.entities[qn] = &worldEntity{gen: gen, attrs: declaredAttrNames(s.Attributes), persistable: s.Kind != ast.EntityNonPersistent} + case *ast.CreateViewEntityStmt: + qn := s.Name.String() + if _, exists := w.entities[qn]; exists && (s.IfNotExists || !(s.CreateOrModify || s.CreateOrReplace)) { + return + } + e := &worldEntity{notBulk: true} + for _, a := range s.Attributes { + e.attrs = append(e.attrs, a.Name) + } + w.entities[qn] = e + case *ast.CreateExternalEntityStmt: + qn := s.Name.String() + if _, exists := w.entities[qn]; exists && (s.IfNotExists || !s.CreateOrModify) { + return + } + w.entities[qn] = &worldEntity{attrs: declaredAttrNames(s.Attributes), persistable: true, notBulk: true} + case *ast.DropEntityStmt: + // The backend's DeleteEntity removes every association with the + // entity at either end, in every module, along with it. + qn := s.Name.String() + delete(w.entities, qn) + for aqn, a := range w.assocs { + if a.from == qn || a.to == qn { + delete(w.assocs, aqn) + } + } + case *ast.AlterEntityStmt: + e := w.entities[s.Name.String()] + if e == nil { + return + } + switch s.Operation { + case ast.AlterEntityAddAttribute: + if s.Attribute != nil && systemMemberOfPseudoType(s.Attribute.Type.Kind) == "" && !containsString(e.attrs, s.Attribute.Name) { + e.attrs = append(append([]string(nil), e.attrs...), s.Attribute.Name) + } + case ast.AlterEntityDropAttribute: + var kept []string + for _, a := range e.attrs { + if a != s.AttributeName { + kept = append(kept, a) + } + } + e.attrs = kept + case ast.AlterEntityRenameAttribute: + renamed := append([]string(nil), e.attrs...) + for i, a := range renamed { + if a == s.AttributeName { + renamed[i] = s.NewName + } + } + e.attrs = renamed + } + case *ast.AlterEntitiesStmt: + for _, qn := range w.bulkTargets(s) { + mod, name, _ := strings.Cut(qn, ".") + for _, action := range s.Actions { + per := *action + per.Name = ast.QualifiedName{Module: mod, Name: name} + w.apply(&per) + } + } + case *ast.CreateAssociationStmt: + qn := s.Name.String() + from, to := createAssociationEnds(s) + if old := w.assocs[qn]; old != nil { + if s.IfNotExists || !s.CreateOrModify { + return + } + from = old.from + if !crossModuleEnds(from, to) { + to = old.to + } + } + w.assocs[qn] = &worldAssoc{name: s.Name.Name, from: from, to: to, both: s.Owner == ast.OwnerBoth} + case *ast.AlterAssociationStmt: + if a := w.assocs[s.Name.String()]; a != nil && s.Operation == ast.AlterAssociationSetOwner { + w.assocs[s.Name.String()] = &worldAssoc{name: a.name, from: a.from, to: a.to, both: s.Owner == ast.OwnerBoth} + } + case *ast.DropAssociationStmt: + delete(w.assocs, s.Name.String()) + case *ast.RenameStmt: + if s.DryRun { + return + } + switch s.ObjectType { + case "association": + if a := w.assocs[s.Name.String()]; a != nil { + delete(w.assocs, s.Name.String()) + w.assocs[s.Name.Module+"."+s.NewName] = &worldAssoc{name: s.NewName, from: a.from, to: a.to, both: a.both} + } + case "entity": + w.renameEntity(s.Name.String(), s.Name.Module+"."+s.NewName) + case "module": + // RENAME MODULE names the module in Name.Name or Name.Module. + oldMod := s.Name.Name + if s.Name.Module != "" { + oldMod = s.Name.Module + } + var entities, assocs []string + for qn := range w.entities { + if strings.HasPrefix(qn, oldMod+".") { + entities = append(entities, qn) + } + } + for qn := range w.assocs { + if strings.HasPrefix(qn, oldMod+".") { + assocs = append(assocs, qn) + } + } + for _, qn := range entities { + w.renameEntity(qn, s.NewName+qn[len(oldMod):]) + } + for _, qn := range assocs { + a := w.assocs[qn] + delete(w.assocs, qn) + w.assocs[s.NewName+qn[len(oldMod):]] = a + } + } + case *ast.MoveStmt: + if s.DocumentType == ast.DocumentTypeEntity && s.TargetModule != "" && s.TargetModule != s.Name.Module { + w.renameEntity(s.Name.String(), s.TargetModule+"."+s.Name.Name) + } + } +} + +// renameEntity moves an entity to a new qualified name and repoints what +// refers to it: its specializations and its associations' ends. +func (w *memberWorld) renameEntity(oldQN, newQN string) { + e := w.entities[oldQN] + if e == nil || oldQN == newQN { + return + } + delete(w.entities, oldQN) + w.entities[newQN] = e + for _, x := range w.entities { + if x.gen == oldQN { + x.gen = newQN + } + } + for _, a := range w.assocs { + if a.from == oldQN { + a.from = newQN + } + if a.to == oldQN { + a.to = newQN + } + } +} + +// bulkTargets is resolveBulkEntityTargets over the world: the entities an +// ALTER ENTITIES statement adds to, after dropping those whose ancestor is +// also a target. +func (w *memberWorld) bulkTargets(s *ast.AlterEntitiesStmt) []string { + var targets []ast.QualifiedName + parentOf := map[string]string{} + for qn, e := range w.entities { + mod, name, ok := strings.Cut(qn, ".") + if !ok { + continue + } + if s.Module != "" && !strings.EqualFold(mod, s.Module) { + continue + } + if s.Module == "" && (w.uneditable[mod] || mod == "System") { + continue + } + if e.notBulk { + continue + } + switch s.Filter { + case ast.EntityFilterPersistent: + if !e.persistable { + continue + } + case ast.EntityFilterNonPersistent: + if e.persistable { + continue + } + } + targets = append(targets, ast.QualifiedName{Module: mod, Name: name}) + parentOf[qn] = e.gen + } + targets = dropInheritedTargets(targets, parentOf) + out := make([]string, 0, len(targets)) + for _, t := range targets { + out = append(out, t.String()) + } + sort.Strings(out) + return out +} + +// validateMemberNameClashes is check's side of the rule against a project: the +// script replayed over the project's members in statement order. +func validateMemberNameClashes(ctx *ExecContext, prog *ast.Program) []error { + if prog == nil || !ctx.Connected() || !programTouchesMembers(prog) { + return nil + } + return replayMemberNameClashes(projectMemberWorld(ctx), prog) +} + +// scriptMemberNameClashes is the same rule for `check` without a project +// (MDL-MEMBER01): the script replayed over the System module alone, whose +// members a specialization of a System entity inherits. A clash it reports is +// between members the script itself declares, or one of them and a System +// member, so it holds whatever the project turns out to contain; the clashes +// that need the project are left to `check -p` and exec. +func scriptMemberNameClashes(prog *ast.Program) []linter.Violation { + if prog == nil || !programTouchesMembers(prog) { + return nil + } + var out []linter.Violation + w := systemMemberWorld() + for i, stmt := range prog.Statements { + err := w.judgeAndApply(stmt) + if err == nil { + continue + } + // The verdict's text ends with its way out; the report shows that as + // the suggestion. + msg, fix := err.Error(), "" + if k := strings.LastIndex(msg, " — "); k >= 0 { + msg, fix = msg[:k], msg[k+len(" — "):] + } + docType, name := memberStatementSubject(stmt) + out = append(out, linter.Violation{ + RuleID: "MDL-MEMBER01", + Severity: linter.SeverityError, + Message: fmt.Sprintf("statement %d: %s", i+1, msg), + Suggestion: fix, + Location: linter.Location{Module: name.Module, DocumentType: docType, DocumentName: name.Name}, + }) + } + return out +} + +// replayMemberNameClashes judges each statement by the verdict exec gives, then +// applies it, in statement order. +func replayMemberNameClashes(w *memberWorld, prog *ast.Program) []error { + if w == nil { + return nil + } + var errs []error + for i, stmt := range prog.Statements { + if err := w.judgeAndApply(stmt); err != nil { + errs = append(errs, fmt.Errorf("statement %d: %w", i+1, err)) + } + } + return errs +} + +// judgeAndApply judges one statement and, when it passes, applies it — a +// refused statement is not, since exec stops there. ALTER ENTITIES is judged +// per target, as exec runs it. +func (w *memberWorld) judgeAndApply(stmt ast.Statement) error { + var err error + if bulk, ok := stmt.(*ast.AlterEntitiesStmt); ok { + err = w.bulkClash(w.bulkTargets(bulk), bulk.Actions) + } else { + err = memberNameClash(w, stmt) + } + if err == nil { + w.apply(stmt) + } + return err +} + +// memberStatementSubject names what a member-adding statement is about, for a +// report's location. +func memberStatementSubject(stmt ast.Statement) (string, ast.QualifiedName) { + switch s := stmt.(type) { + case *ast.CreateEntityStmt: + return "entity", s.Name + case *ast.AlterEntityStmt: + return "entity", s.Name + case *ast.AlterEntitiesStmt: + return "module", ast.QualifiedName{Module: s.Module} + case *ast.CreateAssociationStmt: + return "association", s.Name + case *ast.AlterAssociationStmt: + return "association", s.Name + case *ast.RenameStmt: + return s.ObjectType, s.Name + } + return "", ast.QualifiedName{} +} + +// systemMemberWorld is the System module's members as Studio Pro shows them, +// from modelsdk/meta — what a project-free check knows about the ancestors a +// script names. +func systemMemberWorld() *memberWorld { + w := newMemberWorld() + w.uneditable["System"] = true + for _, e := range meta.ModelerSystemEntities() { + we := &worldEntity{gen: e.Generalization, persistable: e.Persistable, notBulk: true} + for _, a := range e.Attributes { + we.attrs = append(we.attrs, a.Name) + } + w.entities["System."+e.Name] = we + } + for _, a := range meta.ModelerSystemAssociations() { + w.assocs["System."+a.Name] = &worldAssoc{ + name: a.Name, from: "System." + a.Parent, to: "System." + a.Child, both: a.Owner == "Both", + } + } + return w +} + +// bulkClash judges ALTER ENTITIES target by target, applying each add before +// the next is judged — the order exec writes them in. It runs on a copy, so a +// refusal leaves w untouched. targets are exec's (resolveBulkEntityTargets) at +// exec time and the world's (bulkTargets) at check time. +func (w *memberWorld) bulkClash(targets []string, actions []*ast.AlterEntityStmt) error { + if w == nil { + return nil + } + sim := w.clone() + for _, qn := range targets { + mod, name, _ := strings.Cut(qn, ".") + for _, action := range actions { + per := *action + per.Name = ast.QualifiedName{Module: mod, Name: name} + if err := memberNameClash(sim, &per); err != nil { + return fmt.Errorf("%s: %w", qn, err) + } + sim.apply(&per) + } + } + return nil +} + +// clone copies the world deeply enough that apply on the copy leaves w as it +// was. +func (w *memberWorld) clone() *memberWorld { + c := newMemberWorld() + for k, v := range w.entities { + e := *v + e.attrs = append([]string(nil), v.attrs...) + c.entities[k] = &e + } + for k, v := range w.assocs { + a := *v + c.assocs[k] = &a + } + for k, v := range w.uneditable { + c.uneditable[k] = v + } + return c +} + +// programTouchesMembers reports whether a script has a statement that can add +// a member — so a script that does not pays nothing for reading the model. +func programTouchesMembers(prog *ast.Program) bool { + for _, stmt := range prog.Statements { + switch s := stmt.(type) { + case *ast.CreateEntityStmt, *ast.AlterEntityStmt, *ast.AlterEntitiesStmt, + *ast.CreateAssociationStmt, *ast.AlterAssociationStmt: + return true + case *ast.RenameStmt: + if s.ObjectType == "association" { + return true + } + } + } + return false +} + +// refuseMemberNameClash is exec's side: the same verdict, against the project +// as this statement finds it. +func refuseMemberNameClash(ctx *ExecContext, stmt ast.Statement) error { + return memberNameClash(projectMemberWorld(ctx), stmt) +} diff --git a/mdl/executor/member_name_clash_pedapp_test.go b/mdl/executor/member_name_clash_pedapp_test.go new file mode 100644 index 000000000..e65711331 --- /dev/null +++ b/mdl/executor/member_name_clash_pedapp_test.go @@ -0,0 +1,228 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import "testing" + +// ako/mxcli#1645: a member that reuses a name already on the entity's +// generalization chain passed check and exec, and mxbuild rejected it with +// CE0069 "Duplicate member name". Every refusal below is a model measured as +// CE0069 on mx 11.13.0; every control is one measured clean. check -p and exec +// must agree on each (assertAgree), and exec's verdict is its own — agreeExec +// runs the statements as `exec --no-check` does. + +const ce0069 = "CE0069" + +// The reporter's two scripts, verbatim but for the module names. +func TestMemberClash_IssueRepro(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module Other; +create module HR; +create persistent entity Other.Person (Name: String(50)); +create persistent entity Other.Dept (Name: String(50)); +create association Other.Person_Dept from Other.Person to Other.Dept; +create persistent entity HR.Emp extends Other.Person (Code: String(10)); +create association HR.Person_Dept from HR.Emp to Other.Dept;`, + "HR.Emp inherits 'Person_Dept' from Other.Person (association Other.Person_Dept)") + + exec2, _, dir2 := openPedAppCopy(t) + assertAgree(t, exec2, dir2, `mdl 1; +create module Other; +create module HR; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Emp extends Other.Person (Name: String(50), Code: String(10));`, + "HR.Emp inherits 'Name' from Other.Person (attribute Other.Person.Name)") +} + +// The controls: a non-colliding name, the same name on an unrelated entity, on +// two siblings, and in another case — none of which mxbuild reports. +func TestMemberClash_Controls(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module CtlA; +create module CtlB; +create persistent entity CtlA.Person (Name: String(50)); +create persistent entity CtlA.Dept (Name: String(50)); +create association CtlA.Person_Dept from CtlA.Person to CtlA.Dept; +create persistent entity CtlB.Emp extends CtlA.Person (Code: String(10)); +create persistent entity CtlB.Contractor extends CtlA.Person (Code: String(10)); +create persistent entity CtlB.Unrelated (Name: String(50), Person_Dept: String(10)); +create persistent entity CtlB.Cased extends CtlA.Person (name: String(50)); +create association CtlB.Emp_Dept from CtlB.Emp to CtlA.Dept; +alter entity CtlA.Person add attribute Title: String(50);`, "") + // An association is a member of its TO entity only under owner Both. + assertAgree(t, exec, dir, `mdl 1; +create persistent entity CtlA.Target (Link: String(10)); +create association CtlA.Link from CtlA.Dept to CtlA.Target;`, "") + // A re-run of what is stored changes nothing on the chain. + assertAgree(t, exec, dir, `mdl 1; +create or modify persistent entity CtlB.Emp extends CtlA.Person (Code: String(10));`, "") +} + +// Both members kinds collide with both: an association named like an inherited +// attribute, an attribute named like an inherited association. +func TestMemberClash_AttributeAndAssociationShareOneNameSpace(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module G3; +create module S3; +create persistent entity G3.Base (Code: String(10)); +create persistent entity S3.Spec extends G3.Base (X: String(10)); +create persistent entity S3.T (Y: String(10)); +create association S3.Code from S3.Spec to S3.T;`, + "S3.Spec inherits 'Code' from G3.Base (attribute G3.Base.Code)") + + assertAgree(t, exec, dir, `mdl 1; +create module G4; +create module S4; +create persistent entity G4.Base (A: String(10)); +create persistent entity G4.T (B: String(10)); +create association G4.Link from G4.Base to G4.T; +create persistent entity S4.Spec extends G4.Base (Link: String(10));`, + "S4.Spec inherits 'Link' from G4.Base (association G4.Link)") + + // The same entity's own attribute and association. + assertAgree(t, exec, dir, `mdl 1; +create module M10; +create persistent entity M10.A (Link: String(10)); +create persistent entity M10.B (Z: String(10)); +create association M10.Link from M10.A to M10.B;`, + "M10.A already has attribute M10.A.Link") + + // Under owner Both the association is a member of the TO entity too. + assertAgree(t, exec, dir, `mdl 1; +create module M11; +create persistent entity M11.T (Link: String(10)); +create persistent entity M11.F (Z: String(10)); +create association M11.Link from M11.F to M11.T type reference owner both;`, + "M11.T already has attribute M11.T.Link") +} + +// The generalization chain is walked through System entities: their +// attributes and their associations both count. +func TestMemberClash_SystemAncestors(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module S7; +create persistent entity S7.Acct extends System.User (Name: String(50));`, + "S7.Acct inherits 'Name' from System.User (attribute System.User.Name)") + assertAgree(t, exec, dir, `mdl 1; +create module S7b; +create persistent entity S7b.Acct extends System.User (UserRoles: String(10));`, + "S7b.Acct inherits 'UserRoles' from System.User (association System.UserRoles)") + assertAgree(t, exec, dir, `mdl 1; +create module S8; +create persistent entity S8.Doc extends System.FileDocument (Size: Long);`, + "S8.Doc inherits 'Size' from System.FileDocument") +} + +// The other direction: a generalization gaining a member a specialization +// already has — in one script, and against the stored project. Two levels. +func TestMemberClash_GeneralizationGainsSpecializationsMember(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module G5; +create module S5; +create persistent entity G5.Base (Name: String(50)); +create persistent entity G5.Mid extends G5.Base (M: String(10)); +create persistent entity S5.Spec extends G5.Mid (Code: String(10)); +alter entity G5.Base add attribute Code: String(10);`, + "G5.Base's specialization S5.Spec already has 'Code' (attribute S5.Spec.Code)") + + exec2, out2, dir2 := openPedAppCopy(t) + if err := agreeExec(t, exec2, `mdl 1; +create module G5; +create module S5; +create persistent entity G5.Base (Name: String(50)); +create persistent entity S5.Spec extends G5.Base (Code: String(10));`); err != nil { + t.Fatalf("setup: %v\n%s", err, out2.String()) + } + assertAgree(t, exec2, dir2, "mdl 1;\nalter entity G5.Base add attribute Code: String(10);", + "G5.Base's specialization S5.Spec already has 'Code'") + assertAgree(t, exec2, dir2, "mdl 1;\ncreate or modify persistent entity G5.Base (Name: String(50), Code: String(10));", + "G5.Base's specialization S5.Spec already has 'Code'") + // Dropping the specialization's member first makes room: the script is + // replayed in order, not judged against the stored model alone. + assertAgree(t, exec2, dir2, `mdl 1; +alter entity S5.Spec drop attribute Code; +alter entity G5.Base add attribute Code: String(10);`, "") +} + +// A specialization created before its generalization (the generalization is +// resolved lazily): the parent's member meets the child's when the parent is +// created. +func TestMemberClash_SpecializationCreatedFirst(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module G6; +create persistent entity G6.Spec extends G6.Base (Name: String(50)); +create persistent entity G6.Base (Name: String(50));`, + "G6.Base's specialization G6.Spec already has 'Name'") +} + +// A rename into an inherited name — of an attribute and of an association. +func TestMemberClash_Renames(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + assertAgree(t, exec, dir, `mdl 1; +create module G6; +create module S6; +create persistent entity G6.Base (Name: String(50)); +create persistent entity S6.Spec extends G6.Base (Other: String(50)); +alter entity S6.Spec rename attribute Other to Name;`, + "S6.Spec inherits 'Name' from G6.Base") + + assertAgree(t, exec, dir, `mdl 1; +create module G13; +create module S13; +create persistent entity G13.Base (A: String(10)); +create persistent entity G13.T (B: String(10)); +create association G13.Lnk from G13.Base to G13.T; +create persistent entity S13.Spec extends G13.Base (C: String(10)); +create association S13.Foo from S13.Spec to G13.T; +rename association S13.Foo to Lnk;`, + "S13.Spec inherits 'Lnk' from G13.Base (association G13.Lnk)") +} + +// Owner Both makes the association a member of the TO entity: setting it on +// one whose TO entity already has the name, and a new generalization that +// brings a name the entity's specialization has. +func TestMemberClash_OwnerBothAndNewGeneralization(t *testing.T) { + exec, out, dir := openPedAppCopy(t) + if err := agreeExec(t, exec, `mdl 1; +create module M12; +create persistent entity M12.T (Link: String(10)); +create persistent entity M12.F (Z: String(10)); +create association M12.Link from M12.F to M12.T; +create persistent entity M12.Base (Code: String(10)); +create persistent entity M12.Mid (A: String(10)); +create persistent entity M12.Leaf extends M12.Mid (Code: String(10));`); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + assertAgree(t, exec, dir, "mdl 1;\nalter association M12.Link set owner both;", + "M12.T already has attribute M12.T.Link") + assertAgree(t, exec, dir, "mdl 1;\ncreate or modify persistent entity M12.Mid extends M12.Base (A: String(10));", + "through the new generalization of M12.Mid, M12.Leaf would inherit 'Code' from M12.Base") +} + +// ALTER ENTITIES adds to every entity of a module: one that inherits the name +// from outside the target set is refused before anything is written. +func TestMemberClash_BulkAlterEntities(t *testing.T) { + exec, out, dir := openPedAppCopy(t) + if err := agreeExec(t, exec, `mdl 1; +create module Other; +create module HR; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Alpha (A: String(10)); +create persistent entity HR.Emp extends Other.Person (Code: String(10));`); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + assertAgree(t, exec, dir, "mdl 1;\nalter entities in HR add attribute Name: String(50);", + "HR.Emp inherits 'Name' from Other.Person") + // Nothing was written: HR.Alpha, which sorts first, did not get it — adding + // it now is not refused as already existing. + if err := agreeExec(t, exec, "mdl 1;\nalter entity HR.Alpha add attribute Name: String(50);"); err != nil { + t.Errorf("the refused bulk statement wrote HR.Alpha.Name: %v", err) + } + assertAgree(t, exec, dir, "mdl 1;\nalter entities in HR add attribute Note: String(50);", "") +} diff --git a/mdl/executor/member_name_clash_test.go b/mdl/executor/member_name_clash_test.go new file mode 100644 index 000000000..c64385145 --- /dev/null +++ b/mdl/executor/member_name_clash_test.go @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// ako/mxcli#1645 without a project: `mxcli check script.mdl` judges the clashes +// between members the script declares, and between one of them and a System +// member, against the same verdict exec gives (MDL-MEMBER01). What needs the +// project is silent here and left to `check -p`. +func TestMemberClash_NoProjectCheck(t *testing.T) { + cases := []struct { + name, src, want string + }{ + {"inherited attribute", `mdl 1; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Emp extends Other.Person (Name: String(50), Code: String(10));`, + "HR.Emp inherits 'Name' from Other.Person (attribute Other.Person.Name)"}, + {"inherited association", `mdl 1; +create persistent entity Other.Person (Name: String(50)); +create persistent entity Other.Dept (Name: String(50)); +create association Other.Person_Dept from Other.Person to Other.Dept; +create persistent entity HR.Emp extends Other.Person (Code: String(10)); +create association HR.Person_Dept from HR.Emp to Other.Dept;`, + "HR.Emp inherits 'Person_Dept' from Other.Person (association Other.Person_Dept)"}, + {"System ancestor", `mdl 1; +create persistent entity HR.Acct extends System.User (Name: String(50));`, + "HR.Acct inherits 'Name' from System.User (attribute System.User.Name)"}, + {"System ancestor's association", `mdl 1; +create persistent entity HR.Acct extends System.User (UserRoles: String(50));`, + "HR.Acct inherits 'UserRoles' from System.User (association System.UserRoles)"}, + {"two levels below a System entity", `mdl 1; +create persistent entity HR.Pic extends System.Image (Caption: String(50)); +create persistent entity HR.Thumb extends HR.Pic (Size: Long);`, + "HR.Thumb inherits 'Size' from System.FileDocument"}, + // Controls. + {"clean", `mdl 1; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Emp extends Other.Person (Code: String(10)); +create persistent entity HR.Contractor extends Other.Person (Code: String(10)); +create persistent entity HR.Cased extends Other.Person (name: String(10));`, ""}, + {"an entity the script does not declare", `mdl 1; +alter entity HR.Emp add attribute Name: String(50);`, ""}, + // Dropping an entity drops the associations at either end with it. + {"an association dropped with its entity", `mdl 1; +create persistent entity M.A (X: String(10)); +create persistent entity M.B (Y: String(10)); +create association M.Link from M.A to M.B; +drop entity M.A; +create persistent entity M.A (Link: String(10));`, ""}, + {"a renamed module", `mdl 1; +create persistent entity Other.Person (Name: String(50)); +rename module Other to Base; +create persistent entity HR.Emp extends Base.Person (Name: String(50));`, + "HR.Emp inherits 'Name' from Base.Person"}, + {"dropped before it is re-added", `mdl 1; +create persistent entity Other.Person (Name: String(50)); +create persistent entity HR.Emp extends Other.Person (Code: String(10)); +alter entity HR.Emp drop attribute Code; +alter entity Other.Person add attribute Code: String(10);`, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + prog, errs := visitor.Build(c.src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + var got []string + for _, v := range ValidateProgram(prog, "") { + if v.RuleID == "MDL-MEMBER01" { + got = append(got, v.Message) + } + } + if c.want == "" { + if len(got) != 0 { + t.Errorf("control must pass, got %v", got) + } + return + } + if len(got) != 1 || !strings.Contains(got[0], c.want) || !strings.Contains(got[0], "CE0069") { + t.Errorf("want one MDL-MEMBER01 naming %q and CE0069, got %v", c.want, got) + } + }) + } +} diff --git a/mdl/executor/member_verdict.go b/mdl/executor/member_verdict.go new file mode 100644 index 000000000..337aa833e --- /dev/null +++ b/mdl/executor/member_verdict.go @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "sort" + "strings" +) + +// Whether an entity has a member of a given name, for a member a flow names +// bare — a list operation's, an aggregate's — answered once for the flow +// builder and check --references alike (ako/mxcli#1646). The builder fell back +// to writing `.` when its own resolution found nothing, which is +// the attribute mxbuild rejects with CE1613; it now asks this verdict first and +// refuses a name established as missing, with the message check reports. + +// memberKind says what a resolved member name is. +type memberKind int + +const ( + memberIsAttribute memberKind = iota + 1 + memberIsAssociation + // memberIsSystemDate is createdDate / changedDate: not an attribute in the + // model but a flag on the root of the generalization chain, read and sorted + // on by that lower-camel name (measured clean on 11.13.0 for a sort, a + // maximum and an expression, on an entity whose root stores it). + memberIsSystemDate +) + +// memberLinkFacts is what the member verdict needs to know about one link of +// an entity's generalization chain. +type memberLinkFacts struct { + attribute bool // the entity declares an attribute of the name + association bool // its module has an association of the name + generalization string // the next link; "" at the root + storesDate bool // at the root: it stores the system date the name spells +} + +// memberModel reads one link. ok is false when the link cannot be read — a +// module or domain model the backend cannot load, an entity it does not have — +// which makes the verdict memberUnknown: silence, never a refusal. +type memberModel interface { + memberLink(entityQN, name string) (facts memberLinkFacts, ok bool) +} + +// memberVerdictOver is THE walk: entityQN and its generalizations, nearest +// first, for a member called name — an attribute of a link, or an association +// of that name in a link's module (an association is named on the module that +// owns it, not on either end). With systemDates, createdDate / changedDate are +// members too when the chain's root stores them: they are flags on the root, +// not attributes, and a flow reads and sorts on them by that lower-camel name +// (measured clean on 11.13.0 for a sort, a maximum and an expression; reading +// one the root does not store is CE0117). +// +// Matching is case-sensitive, as Mendix's own is: mxbuild rejects `orderno` +// for `OrderNo` with CE1613 in a change and CE0117 in an expression. +func memberVerdictOver(m memberModel, entityQN, name string, systemDates bool) (memberResolution, memberKind) { + if m == nil || entityQN == "" || name == "" { + return memberUnknown, 0 + } + date := systemDates && (name == "createdDate" || name == "changedDate") + seen := map[string]bool{} + for qn := entityQN; qn != ""; { + if seen[qn] { + // A generalization cycle is not a model mxcli should reason about. + return memberUnknown, 0 + } + seen[qn] = true + link, ok := m.memberLink(qn, name) + if !ok { + return memberUnknown, 0 + } + switch { + case link.attribute: + return memberFound, memberIsAttribute + case link.association: + return memberFound, memberIsAssociation + case link.generalization == "": + if date && link.storesDate { + return memberFound, memberIsSystemDate + } + return memberMissing, 0 + } + qn = link.generalization + } + return memberUnknown, 0 +} + +// backendMemberModel reads links from a backend: the project as the statements +// before have left it, which is what the flow builder sees. +type backendMemberModel struct{ b entityLookupBackend } + +func (m backendMemberModel) memberLink(entityQN, name string) (memberLinkFacts, bool) { + if m.b == nil { + return memberLinkFacts{}, false + } + module, entityName, ok := strings.Cut(entityQN, ".") + if !ok { + return memberLinkFacts{}, false + } + mod, err := m.b.GetModuleByName(module) + if err != nil || mod == nil { + return memberLinkFacts{}, false + } + dm, err := m.b.GetDomainModel(mod.ID) + if err != nil || dm == nil { + return memberLinkFacts{}, false + } + entity := dm.FindEntityByName(entityName) + if entity == nil { + return memberLinkFacts{}, false + } + facts := memberLinkFacts{generalization: entity.GeneralizationRef, association: associationNamedInModule(dm, name)} + for _, attr := range entity.Attributes { + if attr != nil && attr.Name == name { + facts.attribute = true + } + } + if entity.GeneralizationRef == "" && (name == "createdDate" || name == "changedDate") { + facts.storesDate = entityStoresSystemMember(entity, name) + } + return facts, true +} + +// entityMemberKind is the verdict on an attribute or association name against +// a backend — resolveMemberOnEntity's question, for a CREATE / CHANGE member. +func entityMemberKind(b entityLookupBackend, entityQN, name string) (memberResolution, memberKind) { + return memberVerdictOver(backendMemberModel{b}, entityQN, name, false) +} + +// flowMemberVerdict is the verdict on a member a flow names bare — a list +// operation's, an aggregate's — against a backend; systemDates says whether +// the statement can read createdDate / changedDate (a sort can, an aggregate +// cannot). exec's builder asks this; check asks the same walk through the +// script (flowMemberRefs.member). +func flowMemberVerdict(b entityLookupBackend, entityQN, name string, systemDates bool) (memberResolution, memberKind) { + return memberVerdictOver(backendMemberModel{b}, entityQN, name, systemDates) +} + +// listMemberRefusal is the message for a list operation or aggregate naming a +// member its list's entity does not have, shared by the flow builder and +// check --references so both say the same thing (ako/mxcli#1646). where is the +// statement as written ("find $l by Numbr"); near is nearMembersFor's list. +// +// An aggregate over a system date gets its own: a sort and an expression read +// createdDate / changedDate where the root stores them, an aggregate never does +// — measured on 11.13.0, `maximum $l by createdDate` is CE0226 "Entity 'M.E' +// does not contain attribute 'M.E.createdDate'." even where the root stores it. +func listMemberRefusal(where, entityQN, name, near string) string { + if name == "createdDate" || name == "changedDate" { + return fmt.Sprintf( + "%s: an aggregate cannot read the system member %s — it is not an attribute of %s, and mxbuild reports "+ + "CE0226 \"Entity '%s' does not contain attribute '%s.%s'.\"", + where, name, entityQN, entityQN, entityQN, name) + } + return fmt.Sprintf( + "%s: %s has no member %q%s%s — mxbuild reports this as CE1613 "+ + "\"The selected attribute '%s.%s' no longer exists\"", + where, entityQN, name, near, flowSystemMemberHint(name), entityQN, name) +} + +// listMemberWhere spells the list statement a member belongs to, as written: +// "find $l by Numbr", "sum $l by Totl". +func listMemberWhere(verb, list, member string) string { + return fmt.Sprintf("%s $%s by %s", verb, list, member) +} + +// flowSystemMemberHint names the spelling of a system member written another +// way in a flow: `owner` / `changedBy` are associations to System.User written +// qualified, and the dates are lower camel case. +func flowSystemMemberHint(name string) string { + for _, spelling := range xpathSystemMemberSpellings { + bare := strings.TrimPrefix(spelling, "System.") + if spelling != name && strings.EqualFold(bare, strings.TrimPrefix(name, "System.")) { + return fmt.Sprintf(" (the system member is spelled `%s`)", spelling) + } + } + return "" +} + +// nearMembersFor lists what the entity does have, capped so a wide entity does +// not bury the error. A typo is cheap to fix only when you can see what is +// there. +func nearMembersFor(b entityLookupBackend, entityQN string) string { + entity, ok := findEntityByQN(b, entityQN) + if !ok { + return "" + } + var names []string + for _, a := range entity.Attributes { + if a != nil { + names = append(names, a.Name) + } + } + return nearMemberList(names) +} + +// nearMemberList is the " — it has A, B" suffix for a list of attribute names. +func nearMemberList(names []string) string { + const max = 10 + if len(names) == 0 { + return "" + } + names = append([]string(nil), names...) + sort.Strings(names) + if len(names) > max { + return fmt.Sprintf(" — it has %s and %d more", + strings.Join(names[:max], ", "), len(names)-max) + } + return fmt.Sprintf(" — it has %s", strings.Join(names, ", ")) +} diff --git a/mdl/executor/retrieve_bare_association_1690_test.go b/mdl/executor/retrieve_bare_association_1690_test.go new file mode 100644 index 000000000..207739817 --- /dev/null +++ b/mdl/executor/retrieve_bare_association_1690_test.go @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// ako/mxcli#1690: `retrieve $C from $O/Ord_Cust;` — an association path with +// the association's SHORT name — passed check -p --references and exec, and +// was stored as the association ".Ord_Cust", with an empty module. Measured on +// 11.13.0: +// +// [CE1613] "The selected association '.Ord_Cust' no longer exists." +// at Retrieve object(s) activity 'Retrieve from association' +// +// and describe printed `retrieve $C from $O/.Ord_Cust;`, which does not parse. +// The builder joined the AST's empty Module to the name. A bare name now +// resolves against the start variable's entity — through its generalization +// chain, from either end — when exactly one association fits, and is refused +// otherwise; it is never written with an empty module. +const bareAssocHead = `mdl 1; +create module B1690; +create module O1690; +create persistent entity B1690.Cust (Name: String(50)); +create persistent entity B1690.Ord (Number: String(20)); +create persistent entity B1690.SubOrd extends B1690.Ord (Extra: String(20)); +create association B1690.Ord_Cust from B1690.Ord to B1690.Cust; +create persistent entity O1690.Base (BaseName: String(20)); +create persistent entity O1690.Thing (ThingName: String(20)); +create association O1690.Base_Thing from O1690.Base to O1690.Thing; +create persistent entity B1690.Spec extends O1690.Base (SpecName: String(20)); +` + +// The reported shape, and the controls a resolution must keep accepting: the +// TO end, a specialization, an association inherited from another module, and +// a System association reached from a System.User specialization. Each is +// stored qualified, and describe prints that. +func TestBareRetrieveAssociation_ResolvesToTheQualifiedName(t *testing.T) { + cases := []struct { + name, params, retrieve, stored string + }{ + {"from the owner", "$O: B1690.Ord", "retrieve $C from $O/Ord_Cust;", "B1690.Ord_Cust"}, + {"from the TO end", "$C: B1690.Cust", "retrieve $Os from $C/Ord_Cust;", "B1690.Ord_Cust"}, + {"from a specialization", "$S: B1690.SubOrd", "retrieve $C from $S/Ord_Cust;", "B1690.Ord_Cust"}, + {"inherited from another module", "$Sp: B1690.Spec", "retrieve $T from $Sp/Base_Thing;", "O1690.Base_Thing"}, + {"System association of a user entity", "$A: Administration.Account", "retrieve $R from $A/UserRoles;", "System.UserRoles"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + exec, out, dir := openPedAppCopy(t) + flow := "create or modify microflow B1690.F (" + c.params + ")\nbegin\n " + c.retrieve + "\nend;\n" + assertAgree(t, exec, dir, bareAssocHead+flow, "") + + unit := microflowUnit(t, exec, "B1690", "F") + short := c.stored[strings.Index(c.stored, ".")+1:] + if bytes.Contains(unit, []byte("\x00."+short+"\x00")) { + t.Fatalf("stored the association as %q — an empty module, CE1613", "."+short) + } + if !bytes.Contains(unit, []byte(c.stored)) { + t.Fatalf("the stored retrieve does not name %s", c.stored) + } + + out.Reset() + if err := agreeExec(t, exec, "describe microflow B1690.F;"); err != nil { + t.Fatalf("describe: %v", err) + } + want := strings.Replace(c.retrieve, "/"+short+";", "/"+c.stored+";", 1) + if !strings.Contains(out.String(), want) { + t.Fatalf("describe must print %q, got:\n%s", want, out.String()) + } + + // Run the script again: the declared bare name and the stored + // qualified one are the same member, so nothing changes. + out.Reset() + if err := agreeExec(t, exec, "mdl 1;\n"+flow); err != nil { + t.Fatalf("re-run: %v", err) + } + if !strings.Contains(out.String(), "Unchanged microflow") { + t.Errorf("re-running the same retrieve must change nothing, got:\n%s", out.String()) + } + }) + } +} + +// The entity and the association can come from the script itself, which is +// the reported shape: check resolves them from the declarations. +func TestBareRetrieveAssociation_ScriptDeclared(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + const src = `mdl 1; +create module S1690; +create persistent entity S1690.Cust (Name: String(50)); +create persistent entity S1690.Ord (Number: String(20)); +create association S1690.Ord_Cust from S1690.Ord to S1690.Cust; +create microflow S1690.GetCust ($O: S1690.Ord) +begin + retrieve $C from $O/Ord_Cust; +end; +` + assertAgree(t, exec, dir, src, "") + if unit := microflowUnit(t, exec, "S1690", "GetCust"); !bytes.Contains(unit, []byte("S1690.Ord_Cust")) { + t.Fatal("the stored retrieve does not name S1690.Ord_Cust") + } +} + +// What cannot be resolved is refused by check -p and by exec alike, with the +// qualified spelling to write instead. +func TestBareRetrieveAssociation_Refused(t *testing.T) { + t.Run("no such association", func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := bareAssocHead + `create microflow B1690.F ($O: B1690.Ord) +begin + retrieve $C from $O/Ord_Kust; +end; +` + assertAgree(t, exec, dir, src, `no association called "Ord_Kust" has B1690.Ord`) + }) + t.Run("an association the entity is no end of", func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := bareAssocHead + `create microflow B1690.F ($Sp: B1690.Spec) +begin + retrieve $C from $Sp/Ord_Cust; +end; +` + assertAgree(t, exec, dir, src, `no association called "Ord_Cust" has B1690.Spec`) + }) + t.Run("two modules each have one", func(t *testing.T) { + exec, _, dir := openPedAppCopy(t) + src := bareAssocHead + `create association O1690.Ord_Cust from O1690.Thing to B1690.Ord; +create microflow B1690.F ($O: B1690.Ord) +begin + retrieve $C from $O/Ord_Cust; +end; +` + assertAgree(t, exec, dir, src, `"Ord_Cust" could be any of B1690.Ord_Cust, O1690.Ord_Cust`) + }) +} + +// A start variable whose entity the builder does not know cannot be resolved, +// and the bare name cannot be written — refused rather than stored as ".Name". +func TestBareRetrieveAssociation_UnknownStartEntityIsRefusedByExec(t *testing.T) { + fb := &flowBuilder{varTypes: map[string]string{}, declaredVars: map[string]string{}} + fb.addRetrieveAction(&ast.RetrieveStmt{Variable: "C", StartVariable: "X", + Source: ast.QualifiedName{Name: "Ord_Cust"}}) + got := strings.Join(fb.GetErrors(), "\n") + if !strings.Contains(got, "the association is not qualified") { + t.Fatalf("a bare association from an unknown entity must be refused, got: %q", got) + } +} + +// diff-then-patch compares a declared flow with describe's spelling, which is +// the stored, qualified one. The speller qualifies a bare retrieve association +// the way the builder resolves it, or every run rewrote the flow. +func TestBareRetrieveAssociation_SpellerQualifies(t *testing.T) { + exec, _, _ := openPedAppCopy(t) + if err := agreeExec(t, exec, bareAssocHead); err != nil { + t.Fatalf("setup: %v", err) + } + prog, errs := visitor.Build(`create microflow B1690.F ($S: B1690.SubOrd) +begin + retrieve $C from $S/Ord_Cust; +end; +`) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + f := prog.Statements[0].(*ast.CreateMicroflowStmt) + got := describedMemberSpellings(exec.newExecContext(context.Background()), f.Parameters, f.Body, nil) + if src := got[0].(*ast.RetrieveStmt).Source.String(); src != "B1690.Ord_Cust" { + t.Errorf("declared retrieve spelled %q; describe prints B1690.Ord_Cust", src) + } +} diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 87cfc4d7f..855963945 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -572,6 +572,11 @@ func validateProgramWithWarnings(ctx *ExecContext, prog *ast.Program) ([]error, // inherits (BusinessEvents.PublishedBusinessEvent's, read from the project): // mxbuild reports the duplicate after exec wrote it (mendixlabs/mxcli#1525). errors = append(errors, validateBusinessEventInheritedMembers(ctx, prog, sc)...) + // A member named like one already on its entity's generalization chain — + // inherited, a specialization's, or the entity's own other kind: mxbuild's + // CE0069, after exec wrote it (ako/mxcli#1645). Replayed in statement + // order, judged by the verdict exec gives. + errors = append(errors, validateMemberNameClashes(ctx, prog)...) return errors, sc.warnings } diff --git a/mdl/executor/validate_assoc_list_source.go b/mdl/executor/validate_assoc_list_source.go index 456a759ca..2619f2240 100644 --- a/mdl/executor/validate_assoc_list_source.go +++ b/mdl/executor/validate_assoc_list_source.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/mendixlabs/mxcli/mdl/ast" - "github.com/mendixlabs/mxcli/model" "github.com/mendixlabs/mxcli/sdk/domainmodel" ) @@ -70,53 +69,26 @@ var listWidgetTypes = map[string]bool{ // checkAssociationShapes indexes the project's associations, overlaid with the // script's, by lower-cased qualified name. func checkAssociationShapes(ctx *ExecContext, sc *scriptContext) map[string]assocShape { - out := map[string]assocShape{} + return checkAssociationCatalog(ctx, sc).shapes +} + +// checkAssociationCatalog is checkAssociationShapes with what could not be +// read, for a check that must tell "no such association" from "not looked". +func checkAssociationCatalog(ctx *ExecContext, sc *scriptContext) associationCatalog { + cat := associationCatalog{shapes: map[string]assocShape{}, unnamed: map[string]bool{}} if ctx != nil && ctx.Backend != nil { if modules, err := getModulesFromCache(ctx); err == nil { - moduleNames := make(map[model.ID]string, len(modules)) - for _, m := range modules { - moduleNames[m.ID] = m.Name - } if dms, err := ctx.Backend.ListDomainModels(); err == nil { - byID := map[model.ID]string{} - for _, dm := range dms { - if mod := moduleNames[dm.ContainerID]; mod != "" { - for _, ent := range dm.Entities { - byID[ent.ID] = mod + "." + ent.Name - } - } - } - for _, dm := range dms { - mod := moduleNames[dm.ContainerID] - if mod == "" { - continue - } - for _, a := range dm.Associations { - if from, to := byID[a.ParentID], byID[a.ChildID]; from != "" && to != "" { - qn := mod + "." + a.Name - out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: to, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), - view: a.Source == domainmodel.OqlViewAssociationSource} - } - } - for _, a := range dm.CrossAssociations { - if from := byID[a.ParentID]; from != "" && a.ChildRef != "" { - qn := mod + "." + a.Name - out[strings.ToLower(qn)] = assocShape{qn: qn, from: from, to: a.ChildRef, - refSet: a.Type == domainmodel.AssociationTypeReferenceSet, owner: sdkOwnerName(a.Owner), - view: a.Source == domainmodel.OqlViewAssociationSource} - } - } - } + cat = projectAssociationCatalog(modules, dms) } } } if sc != nil { for k, v := range sc.associationShapes { - out[k] = v + cat.shapes[k] = v } } - return out + return cat } // checkAssocListSource reports MDL-ASSOCDS01 for one widget, given the data diff --git a/mdl/executor/validate_flow_member_refs.go b/mdl/executor/validate_flow_member_refs.go new file mode 100644 index 000000000..9150917df --- /dev/null +++ b/mdl/executor/validate_flow_member_refs.go @@ -0,0 +1,466 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "reflect" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// flowMemberRefs is what check --references knows while it walks a script's +// flows in order, for the member questions member_verdict.go answers: the +// project, and what the script declares. +// +// Entities are taken from the whole script — a flow's parameters name them, so +// exec cannot reach the flow before they exist. Associations only from the +// statements BEFORE the flow, because that is all exec's builder sees when it +// resolves a bare association name: one declared further down resolves at +// check time and is refused at exec time otherwise. +type flowMemberRefs struct { + ctx *ExecContext + sc *scriptContext + declared map[string]assocShape // associations of the statements so far + project *associationCatalog // read on first use + merged *associationCatalog // project + declared, rebuilt when declared grows +} + +func newFlowMemberRefs(ctx *ExecContext, sc *scriptContext) *flowMemberRefs { + return &flowMemberRefs{ctx: ctx, sc: sc, declared: map[string]assocShape{}} +} + +// declare records the associations stmt creates, for the flows after it. +func (r *flowMemberRefs) declare(stmt ast.Statement) { + switch stmt.(type) { + case *ast.CreateAssociationStmt, *ast.CreateViewEntityStmt: + default: + return + } + one := newScriptContext() + one.collectSingle(stmt) + for k, v := range one.associationShapes { + r.declared[k] = v + r.merged = nil + } +} + +// catalog is the project's associations overlaid with the declared ones. +func (r *flowMemberRefs) catalog() associationCatalog { + if r.merged != nil { + return *r.merged + } + if r.project == nil { + cat := checkAssociationCatalog(r.ctx, nil) + r.project = &cat + } + merged := associationCatalog{shapes: make(map[string]assocShape, len(r.project.shapes)+len(r.declared)), + unnamed: r.project.unnamed, complete: r.project.complete} + for k, v := range r.project.shapes { + merged.shapes[k] = v + } + for k, v := range r.declared { + merged.shapes[k] = v + } + r.merged = &merged + return merged +} + +// chain is entityQN's generalization chain through the script's declarations +// and then the project. +func (r *flowMemberRefs) chain(entityQN string) ([]string, bool) { + var chain []string + seen := map[string]bool{} + for qn := entityQN; qn != ""; { + if seen[qn] { + return chain, false + } + seen[qn] = true + if r.sc != nil && r.sc.entities[qn] { + chain = append(chain, qn) + qn = r.sc.entityGeneralizations[strings.ToLower(qn)] + continue + } + rest, complete := generalizationChain(r.ctx, qn) + return append(chain, rest...), complete + } + return chain, true +} + +// bareRetrieveAssociation reports `retrieve $X from $V/Name` when Name, a bare +// association name, does not resolve to exactly one association from the +// entity $V holds (ako/mxcli#1690). It is silent where the entity is not known +// or the model could not be read: exec decides those, and refuses them too, +// since a bare name cannot be written. +func (r *flowMemberRefs) bareRetrieveAssociation(s *ast.RetrieveStmt, vars map[string]string) string { + if s.StartVariable == "" || s.Source.Module != "" || s.Source.Name == "" { + return "" + } + entity := vars[strings.TrimPrefix(s.StartVariable, "$")] + if entity == "" { + return "" + } + chain, complete := r.chain(entity) + _, matches, verdict := resolveBareAssociation(s.Source.Name, chain, complete, r.catalog()) + if verdict != bareAssocMissing && verdict != bareAssocAmbiguous { + return "" + } + return bareRetrieveAssociationRefusal(s.Variable, s.StartVariable, s.Source.Name, entity, matches, verdict) +} + +// statementMembers is what one flow statement names that its entity does not +// have, each report prefixed with the flow's name. +func (r *flowMemberRefs) statementMembers(flowQN string, st ast.MicroflowStatement, vars map[string]string) []string { + var out []string + report := func(msg string) { + if msg != "" { + out = append(out, flowQN+": "+msg) + } + } + seen := map[string]bool{} + for _, p := range statementAttributePaths(st) { + if msg := r.expressionPath(p, vars); !seen[msg] { + seen[msg] = true + report(msg) + } + } + switch s := st.(type) { + case *ast.RetrieveStmt: + report(r.bareRetrieveAssociation(s, vars)) + case *ast.ListOperationStmt: + for _, m := range listOperationMembers(s) { + report(r.listMember(m.verb, s.InputVariable, vars[s.InputVariable], m.name)) + } + case *ast.AggregateListStmt: + if !s.IsExpression && s.Attribute != "" { + report(r.listMember(strings.ToLower(s.Operation.String()), s.InputVariable, vars[s.InputVariable], s.Attribute)) + } + } + return out +} + +type listOpMember struct{ verb, name string } + +// listOperationMembers are the members a find, filter or sort names: the left +// of a by-member find / filter (the by-expression form is an expression), and +// each sort key. +func listOperationMembers(s *ast.ListOperationStmt) []listOpMember { + switch s.Operation { + case ast.ListOpFind, ast.ListOpFilter: + if s.ByExpression || !ast.IsMemberEquality(s.Condition) { + return nil + } + name, ok := listOperationFieldName(s.Condition.(*ast.BinaryExpr).Left) + if !ok { + return nil + } + return []listOpMember{{strings.ToLower(s.Operation.String()), name}} + case ast.ListOpSort: + var out []listOpMember + for _, spec := range s.SortSpecs { + out = append(out, listOpMember{"sort", spec.Attribute}) + } + return out + } + return nil +} + +// listMember reports a list operation's or aggregate's bare member that the +// list's entity does not have (ako/mxcli#1646), in the words exec's builder +// refuses it with (refuseMissingListMember). A qualified member is a different +// question — validateListOperationSystemMembers and the builder's association +// lookup answer it — and an untyped list is not judged. +func (r *flowMemberRefs) listMember(verb, list, entityQN, name string) string { + if entityQN == "" || name == "" || strings.Contains(name, ".") { + return "" + } + if res, _ := r.member(entityQN, name, !isAggregateVerb(verb)); res != memberMissing { + return "" + } + return listMemberRefusal(listMemberWhere(verb, list, name), entityQN, name, r.nearMembers(entityQN)) +} + +// isAggregateVerb reports an aggregate list function: one that cannot read a +// system date (listMemberRefusal). +func isAggregateVerb(verb string) bool { + switch verb { + case "sum", "average", "minimum", "maximum": + return true + } + return false +} + +// member is flowMemberVerdict through the script: the same walk +// (memberVerdictOver), reading each link from the script where the script +// declares it — exec's builder will find exactly that in the project — and +// from the project otherwise. A link the script alters is not judged: the +// alter may add the member. An association the statements before the flow +// create counts as a member of its module, as a stored one does. +func (r *flowMemberRefs) member(entityQN, name string, systemDates bool) (memberResolution, memberKind) { + return memberVerdictOver(scriptMemberModel{r}, entityQN, name, systemDates) +} + +type scriptMemberModel struct{ r *flowMemberRefs } + +func (m scriptMemberModel) memberLink(entityQN, name string) (memberLinkFacts, bool) { + r := m.r + if r.sc != nil && r.sc.alteredEntities[entityQN] { + return memberLinkFacts{}, false + } + var decl *ast.CreateEntityStmt + if r.sc != nil { + decl = r.sc.entityDecls[entityQN] + } + if decl == nil { + facts, ok := backendMemberModel{r.backend()}.memberLink(entityQN, name) + if ok && !facts.association { + facts.association = r.declaredAssociationInModule(entityQN, name) + } + return facts, ok + } + var facts memberLinkFacts + if decl.Generalization != nil { + facts.generalization = decl.Generalization.String() + } + for _, a := range decl.Attributes { + switch { + case isAutoSystemMemberType(a.Type.Kind): + if systemMemberOfPseudoType(a.Type.Kind) == name { + facts.storesDate = true + } + case a.Name == name: + facts.attribute = true + } + } + facts.association = r.declaredAssociationInModule(entityQN, name) || r.projectAssociationInModule(entityQN, name) + return facts, true +} + +func (r *flowMemberRefs) backend() entityLookupBackend { + if r.ctx == nil { + return nil + } + b, _ := r.ctx.Backend.(entityLookupBackend) + return b +} + +// declaredAssociationInModule reports an association of this name that a +// statement before the flow creates in entityQN's module. +func (r *flowMemberRefs) declaredAssociationInModule(entityQN, name string) bool { + module, _, _ := strings.Cut(entityQN, ".") + _, ok := r.declared[strings.ToLower(module+"."+name)] + return ok +} + +// projectAssociationInModule reports a stored association of this name in +// entityQN's module, for an entity the script (re)declares. +func (r *flowMemberRefs) projectAssociationInModule(entityQN, name string) bool { + module, _, _ := strings.Cut(entityQN, ".") + _, ok := r.catalog().shapes[strings.ToLower(module+"."+name)] + return ok +} + +// nearMembers is nearMembersFor through the script's declarations. +func (r *flowMemberRefs) nearMembers(entityQN string) string { + if r.sc != nil { + if decl := r.sc.entityDecls[entityQN]; decl != nil { + var names []string + for _, a := range decl.Attributes { + if !isAutoSystemMemberType(a.Type.Kind) { + names = append(names, a.Name) + } + } + return nearMemberList(names) + } + } + return nearMembersFor(r.backend(), entityQN) +} + +// expressionPath reports a `$Var/` path in an expression that names a +// member the variable's entity does not have (ako/mxcli#1697): an attribute +// that is not there (the system `id` above all, which no expression can read), +// an association written by its bare name, or one that does not exist or does +// not start at the entity. mxbuild rejects each with CE0117 "Error(s) in +// expression." (measured on 11.13.0). Each step is resolved through +// flowMemberRefs.member — the verdict exec's builder asks for a list member — +// and the path is followed across association hops as far as it resolves; +// anything not established is silence. +func (r *flowMemberRefs) expressionPath(p *ast.AttributePathExpr, vars map[string]string) string { + cur := vars[strings.TrimPrefix(p.Variable, "$")] + steps := expressionPathSteps(p) + if cur == "" || len(steps) == 0 { + return "" + } + text := "$" + strings.TrimPrefix(p.Variable, "$") + "/" + strings.Join(steps, "/") + const rejected = " — mxbuild rejects the expression with CE0117 \"Error(s) in expression.\"" + for i := 0; i < len(steps); i++ { + step := steps[i] + switch strings.Count(step, ".") { + case 0: + res, kind := r.member(cur, step, true) + switch { + case res == memberMissing: + msg := fmt.Sprintf("%s: %s has no attribute %q%s%s", text, cur, step, flowSystemMemberHint(step), rejected) + if strings.EqualFold(step, "id") { + return msg + " An object's id is not readable in an expression: compare the objects " + + "themselves (`$A = $B`), or read a GUID with CommunityCommons.getGUID(item = $X)" + } + return msg + r.nearMembers(cur) + case res == memberFound && kind == memberIsAssociation: + qualified := "Module." + step + chain, complete := r.chain(cur) + if qn, _, v := resolveBareAssociation(step, chain, complete, r.catalog()); v == bareAssocResolved { + qualified = qn + } + return fmt.Sprintf("%s: %s is an association: an expression names it qualified, `$%s/%s/…`%s", + text, step, strings.TrimPrefix(p.Variable, "$"), qualified, rejected) + } + // An attribute: whatever follows it is not a member question. + return "" + case 1: + if _, ok := builtinSystemMemberAssociation(step); ok { + return "" // System.owner / System.changedBy: no domain model lists them + } + if r.isEntity(step) { + return "" // an entity step that is not the hop's target: not modelled + } + to, res := r.associationHop(step, cur) + switch res { + case assocMissing: + return fmt.Sprintf("%s: there is no association %s%s", text, step, rejected) + case assocNotAnEnd: + return fmt.Sprintf("%s: %s is not an end of %s, nor of anything it extends%s", text, cur, step, rejected) + case assocResolved: + default: + return "" + } + cur = to + // `$O/Mod.Assoc/Mod.Target/Attr` names the target after the hop, + // and the builder qualifies a bare `Target` in place (#829). + if i+1 < len(steps) && (steps[i+1] == to || strings.HasSuffix(to, "."+steps[i+1])) { + i++ + } + default: + return "" + } + } + return "" +} + +// associationHop follows a qualified association from an object of entityQN: +// an association the script creates by its declared ends, one the project has +// by resolveAssociationFrom — which also answers "missing" for an association +// in a module the script creates and does not declare it in. A module whose +// entities come from a service contract is not judged. +func (r *flowMemberRefs) associationHop(assocQN, entityQN string) (string, assocResolution) { + if r.sc == nil { + return resolveAssociationFrom(r.ctx, assocQN, entityQN) + } + chain, complete := r.chain(entityQN) + if ends, ok := r.sc.associationEnds[strings.ToLower(assocQN)]; ok { + for _, e := range chain { + switch e { + case ends[0]: + return ends[1], assocResolved + case ends[1]: + return ends[0], assocResolved + } + } + if complete { + return "", assocNotAnEnd + } + return "", assocUnknown + } + if module, _, _ := strings.Cut(assocQN, "."); r.sc.externalEntityModules[module] { + return "", assocUnknown + } + // A link the script declares is not in the project, so the project cannot + // say whether it is an end; the first stored link answers for itself and + // everything it extends. + for _, e := range chain { + to, res := resolveAssociationFrom(r.ctx, assocQN, e) + if res == assocResolved || res == assocMissing || !r.sc.entities[e] { + return to, res + } + } + if complete { + return "", assocNotAnEnd + } + return "", assocUnknown +} + +// isEntity reports a qualified name the script or the project has as an entity. +func (r *flowMemberRefs) isEntity(qn string) bool { + if r.sc != nil && r.sc.entities[qn] { + return true + } + _, ok := findEntityByQN(r.backend(), qn) + return ok +} + +// expressionPathSteps are the member steps of `$v/A/B`. +func expressionPathSteps(p *ast.AttributePathExpr) []string { + if len(p.Path) > 0 { + return p.Path + } + steps := make([]string, 0, len(p.Segments)) + for _, sg := range p.Segments { + steps = append(steps, sg.Name) + } + return steps +} + +var ( + flowBodyType = reflect.TypeOf([]ast.MicroflowStatement(nil)) + errorHandlerPtr = reflect.TypeOf((*ast.ErrorHandlingClause)(nil)) +) + +// statementAttributePaths collects the `$Var/…` paths in one statement's own +// expressions — every field, so a statement kind added later is covered +// without a case here. Nested bodies and error handlers are left out: the +// typed walk visits their statements itself, with the variables in scope +// there. A retrieve's constraint is XPath, judged by validateRetrieveMembers, +// and a validation feedback's target is a member reference, not an expression. +func statementAttributePaths(st ast.MicroflowStatement) []*ast.AttributePathExpr { + if _, ok := st.(*ast.RetrieveStmt); ok { + return nil + } + var out []*ast.AttributePathExpr + var walk func(rv reflect.Value, top bool) + walk = func(rv reflect.Value, top bool) { + if rv.Type() == flowBodyType || rv.Type() == errorHandlerPtr { + return + } + switch rv.Kind() { + case reflect.Interface: + if !rv.IsNil() { + walk(rv.Elem(), false) + } + case reflect.Pointer: + if rv.IsNil() { + return + } + if p, ok := rv.Interface().(*ast.AttributePathExpr); ok { + out = append(out, p) + return + } + walk(rv.Elem(), top) + case reflect.Struct: + _, feedback := st.(*ast.ValidationFeedbackStmt) + for i := 0; i < rv.NumField(); i++ { + f := rv.Type().Field(i) + if !f.IsExported() || (top && feedback && f.Name == "AttributePath") { + continue + } + walk(rv.Field(i), false) + } + case reflect.Slice, reflect.Array: + for i := 0; i < rv.Len(); i++ { + walk(rv.Index(i), false) + } + } + } + walk(reflect.ValueOf(st), true) + return out +} diff --git a/mdl/executor/validate_member_refs.go b/mdl/executor/validate_member_refs.go index eb4ac188f..58f60edfa 100644 --- a/mdl/executor/validate_member_refs.go +++ b/mdl/executor/validate_member_refs.go @@ -4,7 +4,6 @@ package executor import ( "fmt" - "sort" "strings" "github.com/mendixlabs/mxcli/mdl/ast" @@ -76,49 +75,11 @@ const ( // error this exists to catch. func resolveMemberOnEntity(ctx *ExecContext, entityQN, memberName string) memberResolution { b, ok := ctx.Backend.(entityLookupBackend) - if !ok || entityQN == "" || memberName == "" { + if !ok { return memberUnknown } - seen := map[string]bool{} - for currentQN := entityQN; currentQN != ""; { - if seen[currentQN] { - // A generalization cycle is not a model mxcli should reason about. - return memberUnknown - } - seen[currentQN] = true - - parts := strings.SplitN(currentQN, ".", 2) - if len(parts) != 2 { - return memberUnknown - } - mod, err := b.GetModuleByName(parts[0]) - if err != nil || mod == nil { - return memberUnknown - } - dm, err := b.GetDomainModel(mod.ID) - if err != nil || dm == nil { - // The System module lands here. Silence, not a report. - return memberUnknown - } - entity := dm.FindEntityByName(parts[1]) - if entity == nil { - return memberUnknown - } - for _, attr := range entity.Attributes { - if attr != nil && attr.Name == memberName { - return memberFound - } - } - // An association is a legal member of a CREATE / CHANGE too, and it is - // named on the entity that OWNS it rather than on either end, so it is - // checked against the module's whole association list rather than the - // entity's attributes. - if associationNamedInModule(dm, memberName) { - return memberFound - } - currentQN = entity.GeneralizationRef - } - return memberMissing + res, _ := entityMemberKind(b, entityQN, memberName) + return res } // backendRefusesModule reports whether the project's module list answers, @@ -294,26 +255,10 @@ func associationEndsByName(ctx *ExecContext, dm *domainmodel.DomainModel, name s // chain for it would turn that into memberUnknown and cost the check its reach. func generalizationChain(ctx *ExecContext, entityQN string) ([]string, bool) { b, ok := ctx.Backend.(entityLookupBackend) - if !ok || entityQN == "" { + if !ok { return nil, false } - var chain []string - seen := map[string]bool{} - for currentQN := entityQN; currentQN != ""; { - if seen[currentQN] { - // A generalization cycle is not a model mxcli should reason about. - return chain, false - } - seen[currentQN] = true - chain = append(chain, currentQN) - - entity, ok := findEntityByQN(b, currentQN) - if !ok { - return chain, false - } - currentQN = entity.GeneralizationRef - } - return chain, true + return generalizationChainFor(b, entityQN) } // validateMemberReferences reports CREATE / CHANGE member names that do not @@ -326,27 +271,30 @@ func validateMemberReferences(ctx *ExecContext, prog *ast.Program, sc *scriptCon return nil } authored := authoredMembers(prog) + refs := newFlowMemberRefs(ctx, sc) var out []string for _, stmt := range prog.Statements { switch s := stmt.(type) { case *ast.CreateMicroflowStmt: - out = append(out, checkFlowMembers(ctx, sc, authored, + out = append(out, checkFlowMembers(ctx, sc, authored, refs, s.Name.String(), s.Parameters, s.Body)...) case *ast.CreateNanoflowStmt: - out = append(out, checkFlowMembers(ctx, sc, authored, + out = append(out, checkFlowMembers(ctx, sc, authored, refs, s.Name.String(), s.Parameters, s.Body)...) } + refs.declare(stmt) } return out } // checkFlowMembers walks one flow body, tracking which entity each variable // holds so a CHANGE can be resolved at all. -func checkFlowMembers(ctx *ExecContext, sc *scriptContext, authored map[string]bool, +func checkFlowMembers(ctx *ExecContext, sc *scriptContext, authored map[string]bool, refs *flowMemberRefs, flowQN string, params []ast.MicroflowParam, body []ast.MicroflowStatement) []string { var out []string walkTypedFlow(ctx, sc, params, body, func(st ast.MicroflowStatement, vars map[string]string) { + out = append(out, refs.statementMembers(flowQN, st, vars)...) switch s := st.(type) { case *ast.CreateObjectStmt: out = append(out, checkMembers(ctx, sc, authored, flowQN, @@ -671,40 +619,9 @@ func authoredMembers(prog *ast.Program) map[string]bool { // nearMembers lists what the entity does have, capped so a wide entity does not // bury the error. A typo is cheap to fix only when you can see what is there. func nearMembers(ctx *ExecContext, entityQN string) string { - const max = 10 b, ok := ctx.Backend.(entityLookupBackend) if !ok { return "" } - parts := strings.SplitN(entityQN, ".", 2) - if len(parts) != 2 { - return "" - } - mod, err := b.GetModuleByName(parts[0]) - if err != nil || mod == nil { - return "" - } - dm, err := b.GetDomainModel(mod.ID) - if err != nil || dm == nil { - return "" - } - entity := dm.FindEntityByName(parts[1]) - if entity == nil { - return "" - } - var names []string - for _, a := range entity.Attributes { - if a != nil { - names = append(names, a.Name) - } - } - if len(names) == 0 { - return "" - } - sort.Strings(names) - if len(names) > max { - return fmt.Sprintf(" — it has %s and %d more", - strings.Join(names[:max], ", "), len(names)-max) - } - return fmt.Sprintf(" — it has %s", strings.Join(names, ", ")) + return nearMembersFor(b, entityQN) } diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 537e71b22..825312c05 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -96,6 +96,11 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // so this was reachable straight from `mxcli syntax snippet.create` // (mendixlabs/mxcli#1028). violations = append(violations, validateSnippetParameters(stmt)...) + // A page, snippet, Java action or JavaScript action parameter named with + // a platform-reserved word (`Context`, `Type`, `Id`, …): CE7247 at build + // time, after exec wrote the document and its callers (MDL-PARAM01, + // ako/mxcli#1651). The list attributes and enumeration values use. + violations = append(violations, validateReservedParameterNames(stmt)...) // A microflow's URL / export level / concurrency clauses, against the // same rules the writer applies (MDL-MF01..MF04). violations = append(violations, validateMicroflowDocumentProperties(stmt)...) @@ -188,6 +193,15 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // Check for intra-script duplicate definitions (CREATE X … CREATE X without DROP) violations = append(violations, CheckScriptDuplicates(prog)...) + // A member named like one already on its entity's generalization chain + // (CE0069, ako/mxcli#1645), among what the script declares and the System + // module. With a project, the reference pass judges it against the whole + // model instead (validateMemberNameClashes) — running both would report it + // twice. + if projectPath == "" { + violations = append(violations, scriptMemberNameClashes(prog)...) + } + // Validate design properties against the project's theme registry // (themesource design-properties.json) — flags unknown keys and invalid // option values, listing the allowed values. Only runs with --project. @@ -221,6 +235,11 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // answer first; the project, when there is one, for the rest. violations = append(violations, ValidateDatasourceXPathComparands(prog, project)...) + // A constant in any XPath constraint (MDL-XPATH03): mxbuild rejects + // `[Total > @Mod.C]` with CE0161 whether or not the constant exists, so this + // needs no project (ako/mxcli#1685). + violations = append(violations, validateXPathConstants(prog)...) + // An association between a persistable and a non-persistable entity must // start in the non-persistable one and have owner Default — CE0001 // (MDL-ASSOC01, ako/mxcli#819). The script's declarations answer first. diff --git a/mdl/executor/validate_reserved_parameter_names.go b/mdl/executor/validate_reserved_parameter_names.go new file mode 100644 index 000000000..0066f5111 --- /dev/null +++ b/mdl/executor/validate_reserved_parameter_names.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/types" +) + +// validateReservedParameterNames (MDL-PARAM01) refuses a page, snippet, Java +// action or JavaScript action parameter named with a word the Mendix platform +// reserves (ako/mxcli#1651, mirroring mendixlabs/mxcli#1642). mxbuild reports it +// as +// +// CE7247 "The name 'Context' is a reserved word." at Page parameter 'Context' +// +// after exec has written the page and every caller that passes the parameter by +// name — so renaming it then means changing all of them. +// +// The list is types.PlatformReservedWords, the one attribute names (MDL021) and +// enumeration values (MDL010) are held to; nothing here keeps a second one. +// Measured on mx 11.13.0 in a blank app, one element per name and context: every +// listed name tried (Java keywords such as `Class`, `New`, `this`, `int`, +// `null`, `true`, `Long`, `Package`; Mendix's own `Context`, `CurrentUser`, +// `GUID`, `Id`, `MendixObject`, `Owner`, `Type`, `con`, `submetaobjectname`, +// `ChangedDate`, `changedby`, `createddate`) is CE7247 as a page parameter, a +// snippet parameter and a Java action parameter, and `Context`/`Type` as a +// JavaScript action parameter — in any case (`Class`, `New`, `Id` as well as +// `int`, `this`), and a quoted name is stored, and refused, as its bare self. `Object` was the one reserved name the list +// lacked (CE7247 in all of those places, and for an attribute, an enumeration +// value and an entity), and was added to it. Names not on it built clean: the +// system variables `currentObject`, `latestError`, `latestHttpResponse`, +// `latestSoapFault`, `currentSession`, `currentDeviceType`, and `String`, +// `System`, `User`, `List`, `Var`, `record`, `currentPage`, `Session`. +// +// A microflow or nanoflow parameter is NOT judged against this list: measured, +// `$Context`, `$Type`, `$Class` and the rest build clean there. A flow parameter +// or variable is refused for a different set of names (the system variables: +// CE0110 "Variable name 'latestError' is reserved.", CE0039 for a variable named +// `CurrentUser`), which this rule does not cover. +func validateReservedParameterNames(stmt ast.Statement) []linter.Violation { + type param struct { + name string + entity string // the parameter's entity, for the suggested name + } + var ( + kind string + owner ast.QualifiedName + dollar = "$" + params []param + ) + pageParams := func(ps []ast.PageParameter) { + for _, p := range ps { + params = append(params, param{name: p.Name, entity: parameterEntityName(p.EntityType, p.Type)}) + } + } + codeParams := func(ps []ast.JavaActionParam) { + dollar = "" + for _, p := range ps { + params = append(params, param{name: p.Name, entity: parameterEntityName(ast.QualifiedName{}, p.Type)}) + } + } + switch s := stmt.(type) { + case *ast.CreatePageStmtV3: + kind, owner = "page", s.Name + pageParams(s.Parameters) + case *ast.CreateSnippetStmtV3: + kind, owner = "snippet", s.Name + pageParams(s.Parameters) + case *ast.AlterPageStmt: + kind, owner = strings.ToLower(s.ContainerType), s.PageName + if kind != "page" && kind != "snippet" { + return nil + } + for _, op := range s.Operations { + if add, ok := op.(*ast.AddParameterOp); ok { + pageParams([]ast.PageParameter{add.Parameter}) + } + } + case *ast.CreateJavaActionStmt: + kind, owner = "java action", s.Name + codeParams(s.Parameters) + case *ast.CreateJavaScriptActionStmt: + kind, owner = "javascript action", s.Name + codeParams(s.Parameters) + default: + return nil + } + + var out []linter.Violation + for _, p := range params { + name := strings.Trim(strings.TrimSpace(p.name), `"`) + if !types.IsPlatformReserved(name) { + continue + } + alt := p.entity + if alt == "" || types.IsPlatformReserved(alt) || strings.EqualFold(alt, name) { + alt = name + "Value" + } + out = append(out, linter.Violation{ + RuleID: "MDL-PARAM01", + Severity: linter.SeverityError, + Location: linter.Location{Module: owner.Module, DocumentType: kind, DocumentName: owner.Name}, + Message: fmt.Sprintf("%s parameter %s%s of %s is a reserved word — mxbuild rejects it with "+ + "CE7247 \"The name '%s' is a reserved word.\"", kind, dollar, name, owner.String(), name), + Suggestion: fmt.Sprintf("rename it, e.g. %s%s, and pass it under that name wherever the %s is "+ + "called. Quoting does not help: the platform checks the bare name.", dollar, alt, kind), + }) + } + return out +} + +// parameterEntityName is the entity a parameter carries, "" for a primitive. +// A bare `Module.Name` parses as an enumeration reference (see +// TypeEnumeration vs TypeEntity in CLAUDE.md), so that is read too. +func parameterEntityName(entity ast.QualifiedName, t ast.DataType) string { + switch { + case entity.Name != "": + return entity.Name + case t.EntityRef != nil: + return t.EntityRef.Name + case t.EnumRef != nil && !t.ExplicitEnum: + return t.EnumRef.Name + } + return "" +} + +// refuseReservedParameterNames is the same verdict in the exec handlers, so a +// script run with --no-check cannot write what check refuses. +func refuseReservedParameterNames(stmt ast.Statement) error { + if v := firstBlockingViolation(validateReservedParameterNames(stmt)); v != nil { + return mdlerrors.NewValidationf("%s — %s", v.Message, v.Suggestion) + } + return nil +} diff --git a/mdl/executor/validate_reserved_parameter_names_test.go b/mdl/executor/validate_reserved_parameter_names_test.go new file mode 100644 index 000000000..7cfbb1c68 --- /dev/null +++ b/mdl/executor/validate_reserved_parameter_names_test.go @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// ako/mxcli#1651 (mendixlabs/mxcli#1642): a page parameter named `Context` +// passed check and exec, and mx check reported CE7247 "The name 'Context' is a +// reserved word." The platform's reserved-word list — the one attributes and +// enumeration values are already checked against — applies to the parameters +// of pages, snippets, Java actions and JavaScript actions too, case-insensitively, +// and quoting does not escape it. It does NOT apply to microflow or nanoflow +// parameters, which mxbuild judges against a different list. +func TestReservedParameterNames(t *testing.T) { + cases := []struct { + name, src, want string + }{ + {"page parameter", `create page M.Sale_View (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $Context: M.Sale )) { };`, + "page parameter $Context of M.Sale_View is a reserved word"}, + {"any case", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $CLASS: M.Sale )) { };`, + "page parameter $CLASS of M.P is a reserved word"}, + {"quoted", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( "Context": M.Sale )) { };`, + "page parameter $Context of M.P is a reserved word"}, + {"primitive page parameter", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $Type: String )) { };`, + "page parameter $Type of M.P is a reserved word"}, + {"Object, measured CE7247 though not a Java keyword", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $Object: M.Sale )) { };`, + "page parameter $Object of M.P is a reserved word"}, + {"snippet parameter", `create snippet M.S ( params: ( $Owner: M.Sale ) ) { };`, + "snippet parameter $Owner of M.S is a reserved word"}, + {"added to a page", `alter page M.P { add parameters $Id: M.Sale; };`, + "page parameter $Id of M.P is a reserved word"}, + {"added to a snippet", `alter snippet M.S { add parameters $Context: M.Sale; };`, + "snippet parameter $Context of M.S is a reserved word"}, + {"java action parameter", "create java action M.JA(Context: string not null) returns boolean as $$\nreturn true;\n$$;", + "java action parameter Context of M.JA is a reserved word"}, + {"quoted java action parameter", "create java action M.JA(\"Context\": string not null) returns boolean as $$\nreturn true;\n$$;", + "java action parameter Context of M.JA is a reserved word"}, + {"javascript action parameter", "create javascript action M.JS(Type: String not null) returns Boolean as $$\nreturn Promise.resolve(true);\n$$;", + "javascript action parameter Type of M.JS is a reserved word"}, + // Controls. + {"another name", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $LaneContext: M.Sale, $Sale: M.Sale )) { };`, ""}, + {"a system variable name is not on this list", `create page M.P (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $currentObject: M.Sale, $latestError: M.Sale )) { };`, ""}, + {"microflow parameters are judged by another list", "create microflow M.MF ($Context: String, $Type: String) begin end;", ""}, + {"nanoflow parameters too", "create nanoflow M.NF ($Context: String) begin end;", ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + prog, errs := visitor.Build("mdl 1;\n" + c.src) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + var got []linter.Violation + for _, v := range ValidateProgram(prog, "") { + if v.RuleID == "MDL-PARAM01" { + got = append(got, v) + } + } + if c.want == "" { + if len(got) != 0 { + t.Errorf("control must pass, got %v", got[0].Message) + } + return + } + if len(got) != 1 { + t.Fatalf("want one MDL-PARAM01, got %d: %v", len(got), got) + } + v := got[0] + if v.Severity != linter.SeverityError || !strings.Contains(v.Message, c.want) || !strings.Contains(v.Message, "CE7247") { + t.Errorf("want an error naming %q and CE7247, got %s: %s", c.want, v.Severity, v.Message) + } + if !strings.Contains(v.Suggestion, "rename") { + t.Errorf("want a rename suggestion, got %q", v.Suggestion) + } + }) + } +} + +// The suggestion names a usable alternative: the entity a parameter carries, +// which is how Studio Pro names a fresh one. +func TestReservedParameterNames_SuggestsEntityName(t *testing.T) { + prog, errs := visitor.Build("mdl 1;\ncreate page M.Sale_View (title: 'S', layout: Atlas_Core.Atlas_Default, params: ( $Context: M.Sale )) { };") + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + vs := validateReservedParameterNames(prog.Statements[0]) + if len(vs) != 1 || !strings.Contains(vs[0].Suggestion, "$Sale") { + t.Errorf("want a suggestion of $Sale, got %v", vs) + } +} + +// The list attributes and enumeration values are checked against gains +// `Object`: measured CE7247 on 11.13.0 for an attribute, an enumeration value, a +// page, snippet, Java action and JavaScript action parameter, and an entity. +func TestReservedWords_Object(t *testing.T) { + prog, errs := visitor.Build(`mdl 1; +create non-persistent entity M.E ("Object": String(10)); +create enumeration M.Kind (Object 'Object', Other 'Other');`) + if len(errs) > 0 { + t.Fatalf("parse: %v", errs[0]) + } + seen := map[string]bool{} + for _, v := range ValidateProgram(prog, "") { + seen[v.RuleID] = true + } + if !seen["MDL021"] || !seen["MDL010"] { + t.Errorf("want MDL021 (attribute) and MDL010 (enumeration value) for Object, got %v", seen) + } +} + +// check -p and exec agree on the reporter's page (exec --no-check included, as +// agreeExec runs it), with the reporter's workaround as the control. +func TestCheckExecAgree_ReservedPageParameter(t *testing.T) { + exec, out, dir := openPedAppCopy(t) + if err := agreeExec(t, exec, "mdl 1;\ncreate persistent entity MyFirstModule.Sale (Name: String(100));"); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + page := func(param string) string { + return `mdl 1; +create page MyFirstModule.Sale_View ( + Title: 'Sale', + Layout: Atlas_Core.Atlas_Default, + Params: ( $` + param + `: MyFirstModule.Sale ) +) { + layoutgrid lg { row { column (DesktopWidth: AutoFill) { + dataview dvSale (DataSource: $` + param + `) { textbox tbName (Attribute: Name) } + } } } +};` + } + assertAgree(t, exec, dir, page("Context"), "page parameter $Context of MyFirstModule.Sale_View is a reserved word") + assertAgree(t, exec, dir, page("LaneContext"), "") + assertAgree(t, exec, dir, "mdl 1;\ncreate java action MyFirstModule.JA_Reserved(Context: string not null) returns boolean as $$\nreturn true;\n$$;", + "java action parameter Context of MyFirstModule.JA_Reserved is a reserved word") +} diff --git a/mdl/executor/validate_retrieve_association_list.go b/mdl/executor/validate_retrieve_association_list.go index 78718b04a..47806d239 100644 --- a/mdl/executor/validate_retrieve_association_list.go +++ b/mdl/executor/validate_retrieve_association_list.go @@ -236,7 +236,7 @@ func associationListObjectUses(ctx *ExecContext, shapes map[string]assocShape, if isList { lists[st.Variable] = other bound[st.Variable] = assocListBinding{ - assoc: shapes[strings.ToLower(qualifyAssoc(st.Source, startEntity))].qn, + assoc: shapes[strings.ToLower(qualifyAssoc(ctx, shapes, st.Source, startEntity))].qn, start: start, startEntity: startEntity, elem: other, } } else { @@ -247,12 +247,23 @@ func associationListObjectUses(ctx *ExecContext, shapes map[string]assocShape, return out } -func qualifyAssoc(assoc ast.QualifiedName, startEntity string) string { +// qualifyAssoc is the association a retrieve names. A bare name resolves the +// way the flow builder resolves it (resolveBareAssociation, ako/mxcli#1690), +// not by assuming the start entity's module; one that does not resolve is "", +// which no shape matches, so the rule stays silent on it. +func qualifyAssoc(ctx *ExecContext, shapes map[string]assocShape, assoc ast.QualifiedName, startEntity string) string { if assoc.Module != "" { return assoc.String() } - mod, _, _ := strings.Cut(startEntity, ".") - return mod + "." + assoc.Name + chain, complete := generalizationChain(ctx, startEntity) + if len(chain) == 0 { + chain = []string{startEntity} + } + qn, _, verdict := resolveBareAssociation(assoc.Name, chain, complete, associationCatalog{shapes: shapes, complete: true}) + if verdict != bareAssocResolved { + return "" + } + return qn } // associationRetrieveShape decides what a retrieve over assoc from an object of @@ -261,7 +272,7 @@ func qualifyAssoc(assoc ast.QualifiedName, startEntity string) string { func associationRetrieveShape(ctx *ExecContext, shapes map[string]assocShape, assoc ast.QualifiedName, startEntity string) (other string, isList, known bool) { - shape, ok := shapes[strings.ToLower(qualifyAssoc(assoc, startEntity))] + shape, ok := shapes[strings.ToLower(qualifyAssoc(ctx, shapes, assoc, startEntity))] if !ok || shape.owner == "" || strings.EqualFold(shape.from, shape.to) { return "", false, false } diff --git a/mdl/executor/validate_user_role_tokens.go b/mdl/executor/validate_user_role_tokens.go index cba8ddf0a..6ee61cfaa 100644 --- a/mdl/executor/validate_user_role_tokens.go +++ b/mdl/executor/validate_user_role_tokens.go @@ -120,6 +120,13 @@ func userRoleNames(ctx *ExecContext, prog *ast.Program) (map[string]bool, []stri // depth: a database retrieve's, a data source's, an access rule's, a user // task's targeting (as created and as altered) and an offline sync's. func walkXPathConstraints(v any, fn func(xpath string)) { + walkXPathSlots(v, func(_, xpath string) { fn(xpath) }) +} + +// walkXPathSlots is walkXPathConstraints that also names the slot each +// constraint sits in ("retrieve", "data source", "access rule", "user task +// targeting", "offline sync"), for a message that says where to look. +func walkXPathSlots(v any, fn func(slot, xpath string)) { var walk func(rv reflect.Value) walk = func(rv reflect.Value) { switch rv.Kind() { @@ -134,25 +141,25 @@ func walkXPathConstraints(v any, fn func(xpath string)) { switch n := rv.Interface().(type) { case *ast.RetrieveStmt: if n.StartVariable == "" && n.Where != nil { - fn(expressionToXPath(n.Where)) + fn("retrieve", expressionToXPath(n.Where)) } return case *ast.DataSourceV3: - fn(n.Where) + fn("data source", n.Where) return case *ast.GrantEntityAccessStmt: - fn(n.XPathConstraint) + fn("access rule", n.XPathConstraint) return case *ast.WorkflowTargetingNode: - fn(n.XPath) + fn("user task targeting", n.XPath) return case *ast.SetActivityPropertyOp: if n.Property == "targeting_xpath" { - fn(n.Value) + fn("user task targeting", n.Value) } return case *ast.NavSyncDef: - fn(n.Constraint) + fn("offline sync", n.Constraint) return } walk(rv.Elem()) diff --git a/mdl/executor/validate_xpath_constants.go b/mdl/executor/validate_xpath_constants.go new file mode 100644 index 000000000..a5a23341c --- /dev/null +++ b/mdl/executor/validate_xpath_constants.go @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "fmt" + "sort" + "strings" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// An XPath constraint cannot reference a constant. Mendix expressions write one +// as `@Module.Constant`, and the same spelling in a constraint reads like it +// should work — but mxbuild rejects it (ako/mxcli#1685). Measured on 11.13.0 +// (the retrieve also on 11.15.0), each case alone in a fresh app: +// +// retrieve … where [Total > @Shop.C_Limit] CE0161 Integer constant +// retrieve … where [Name = @Shop.C_Name] CE0161 String constant +// retrieve … where [Active = @Shop.C_Flag] CE0161 Boolean constant +// retrieve … where [Total > @Shop.Nope] CE0161 no such constant — not CE1613: +// XPath does not resolve the name +// list view database from … [Total > @Shop.C_Limit] CE0161 +// access rule … [Total > @Shop.C_Limit] CE0161 (production security; with +// security off mxbuild checks no +// access rule at all) +// user task targeting xpath [Name = @Wf.C_Name] CE0161 ([Name = 'Reviewer'] clean) +// declare $Limit Integer = @Shop.C_Limit; +// retrieve … where [Total > $Limit] clean the control +// +// Until the XPath grammar had a rule for it, `@` parsed as a one-word name and +// the statement failed at the module name ("mismatched input 'Shop' expecting +// ']'"), which reads as a gap in MDL rather than a rule of the platform. Now it +// parses, and this says what is actually wrong and what to write instead. +// +// It needs no project: the refusal does not depend on whether the constant +// exists. An offline sync constraint is refused alike; it was not measured +// separately. + +const xpathConstantRule = "MDL-XPATH03" + +// validateXPathConstants reports every constant referenced inside an XPath +// constraint the script writes. +func validateXPathConstants(prog *ast.Program) []linter.Violation { + if prog == nil { + return nil + } + var out []linter.Violation + for _, stmt := range prog.Statements { + loc, where := xpathConstantLocation(stmt) + severity := linter.SeverityError + if _, _, excluded := excludedFlow(stmt); excluded { + // Mendix does not validate an excluded document. + severity = linter.SeverityWarning + } + seen := map[string]bool{} + walkXPathSlots(stmt, func(slot, xpath string) { + for _, name := range xpathConstantRefs(xpath) { + if seen[slot+"\x00"+name] { + continue + } + seen[slot+"\x00"+name] = true + out = append(out, linter.Violation{ + RuleID: xpathConstantRule, + Severity: severity, + Location: loc, + Message: fmt.Sprintf( + "%s%s: the XPath constraint uses the constant @%s — an XPath constraint cannot reference a constant, "+ + "and mxbuild rejects it with CE0161 \"Error(s) in XPath constraint.\"", + slot, where, name), + Suggestion: xpathConstantSuggestion(slot, name), + }) + } + }) + } + return out +} + +// xpathConstantSuggestion is what to write instead. Only a flow has variables; +// the other slots have nowhere to put the constant's value. +func xpathConstantSuggestion(slot, name string) string { + short := name + if i := strings.LastIndex(name, "."); i >= 0 { + short = name[i+1:] + } + if slot == "retrieve" { + return fmt.Sprintf("Assign the constant to a variable first and compare with the variable: "+ + "`declare $%s = @%s;`, then `$%s` in the constraint.", short, name, short) + } + return fmt.Sprintf("A %s has no variable to hold @%s. Write the value into the constraint, or retrieve "+ + "in a microflow, where the constant can be assigned to a variable first (a microflow data source).", slot, name) +} + +// xpathConstantLocation names the document a statement writes, for the message. +func xpathConstantLocation(stmt ast.Statement) (linter.Location, string) { + if g, ok := stmt.(*ast.GrantEntityAccessStmt); ok { + return linter.Location{Module: g.Entity.Module, DocumentType: "entity", DocumentName: g.Entity.Name}, + " on " + g.Entity.String() + } + kind, name, _ := stmtCreateKind(stmt) + if name == "" { + return linter.Location{}, "" + } + loc := linter.Location{DocumentType: kind, DocumentName: name} + if i := strings.Index(name, "."); i >= 0 { + loc.Module, loc.DocumentName = name[:i], name[i+1:] + } + return loc, " in " + kind + " " + name +} + +// xpathConstantRefs returns the qualified names of the constants a stored XPath +// constraint references — each `@Name.Name` outside a string literal — sorted +// and without duplicates. A literal is data (`'info@shop.example'`), and a +// doubled quote inside one is an escaped quote, not its end. +func xpathConstantRefs(xpath string) []string { + set := map[string]bool{} + inString := false + for i := 0; i < len(xpath); i++ { + c := xpath[i] + if c == '\'' { + if inString && i+1 < len(xpath) && xpath[i+1] == '\'' { + i++ + continue + } + inString = !inString + continue + } + if inString || c != '@' { + continue + } + j := i + 1 + for j < len(xpath) && (isWordByte(xpath[j]) || xpath[j] == '.' || xpath[j] == '"') { + j++ + } + if name := strings.Trim(strings.ReplaceAll(xpath[i+1:j], `"`, ""), "."); name != "" { + set[name] = true + } + i = j - 1 + } + names := make([]string, 0, len(set)) + for n := range set { + names = append(names, n) + } + sort.Strings(names) + return names +} diff --git a/mdl/executor/validate_xpath_constants_test.go b/mdl/executor/validate_xpath_constants_test.go new file mode 100644 index 000000000..223f24ee1 --- /dev/null +++ b/mdl/executor/validate_xpath_constants_test.go @@ -0,0 +1,159 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/linter" +) + +// ako/mxcli#1685: `[Total > @Shop.C_Limit]` was a parse error, which reads as +// "MDL cannot spell this". It parses now, and the answer is mxbuild's: an XPath +// constraint cannot use a constant. Measured on 11.13.0 (the retrieve also on +// 11.15.0), each slot alone in a fresh app, CE0161 "Error(s) in XPath +// constraint." for an Integer, a String and a Boolean constant alike — in a +// retrieve, a list view's database source and an access rule (production +// security; with security off mxbuild does not check access rules at all). An +// unknown constant is the same CE0161, never CE1613: XPath does not resolve the +// name. The control, the constant assigned to a variable and the variable +// compared, builds clean. + +// xpathConstantSlots puts `@Shop.C_Limit` in each XPath slot mxcli writes. +func xpathConstantSlots() map[string]string { + return map[string]string{ + "retrieve": `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where [Total > @Shop.C_Limit]; + end;`, + "retrieve, unbracketed": `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where Total > @Shop.C_Limit; + end;`, + "retrieve, second group": `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where [Name != empty][Total > @Shop.C_Limit]; + end;`, + "retrieve, nested predicate": `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where [Shop.Item_Order/Shop.Order[Total > @Shop.C_Limit]]; + end;`, + "nanoflow retrieve": `create nanoflow Shop.N () + begin + retrieve $Items from Shop.Item where [Total > @Shop.C_Limit]; + end;`, + "list view database source": `create page Shop.P (title: 'Items', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database from Shop.Item where [Total > @Shop.C_Limit]) { + dynamictext t (Content: 'x') + } + };`, + "data source, unbracketed": `create page Shop.P (title: 'Items', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database from Shop.Item where Total > @Shop.C_Limit) { + dynamictext t (Content: 'x') + } + };`, + "access rule": `grant read * on entity Shop.Item to Shop.User where [Total > @Shop.C_Limit];`, + "user task targeting": `create workflow Shop.W + parameter $Context: Shop.Item + begin + user task Review 'Review' page Shop.TaskPage + targeting users xpath [Name = @Shop.C_Limit] outcomes 'Done' { }; + end workflow;`, + } +} + +func xpathConstantErrors(vs []linter.Violation) (errs, warns []linter.Violation) { + for _, v := range vs { + if v.RuleID != xpathConstantRule { + continue + } + if v.Severity == linter.SeverityError { + errs = append(errs, v) + } else { + warns = append(warns, v) + } + } + return errs, warns +} + +func TestXPathConstants_RefusedInEverySlot(t *testing.T) { + for slot, src := range xpathConstantSlots() { + t.Run(slot, func(t *testing.T) { + errs, _ := xpathConstantErrors(validateXPathConstants(parseMDL(t, src))) + if len(errs) != 1 { + t.Fatalf("got %d %s errors, want 1: %v", len(errs), xpathConstantRule, errs) + } + msg := errs[0].Message + " " + errs[0].Suggestion + for _, w := range []string{"@Shop.C_Limit", "CE0161", "variable"} { + if !strings.Contains(msg, w) { + t.Errorf("message should mention %q: %s", w, msg) + } + } + }) + } +} + +// CONTROLS: an `@` that is not a constant in an XPath constraint is not one. +func TestXPathConstants_SilentOutsideAConstraint(t *testing.T) { + for name, src := range map[string]string{ + // The workaround the message suggests. + "constant via a variable": `create microflow Shop.F () + begin + declare $Limit Integer = @Shop.C_Limit; + retrieve $Items from Shop.Item where [Total > $Limit]; + end;`, + // Data, not a reference. + "inside a string literal": `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where [Name = '@Shop.C_Limit' or Name = 'it''s @Shop.C_Limit']; + end;`, + "an e-mail address in a literal": `grant read * on entity Shop.Item to Shop.User where [Name = 'info@shop.example'];`, + // A widget condition is a client expression, where a constant is valid. + "visible condition": `create page Shop.P (title: 'Items', layout: Atlas_Core.Atlas_Default) { + dataview dv (datasource: $Item) { + textbox tb (Attribute: Name, Visible: [Total > @Shop.C_Limit]) + } + };`, + "a microflow expression": `create microflow Shop.F () returns Integer + begin + return @Shop.C_Limit * 2; + end;`, + } { + t.Run(name, func(t *testing.T) { + if vs := validateXPathConstants(parseMDL(t, src)); len(vs) != 0 { + t.Errorf("reported: %v", vs) + } + }) + } +} + +// Mendix does not validate an excluded document: a warning, not an error. +func TestXPathConstants_ExcludedFlowWarns(t *testing.T) { + errs, warns := xpathConstantErrors(validateXPathConstants(parseMDL(t, "@excluded\n"+xpathConstantSlots()["retrieve"]))) + if len(errs) != 0 || len(warns) != 1 { + t.Errorf("excluded: got errors %v, warnings %v; want one warning", errs, warns) + } +} + +// Two constants in one constraint are two findings; the same one twice is one. +func TestXPathConstants_EachConstantOnce(t *testing.T) { + src := `create microflow Shop.F () + begin + retrieve $Items from Shop.Item where [Total > @Shop.C_Min and Total < @Shop.C_Max and Total != @Shop.C_Min]; + end;` + errs, _ := xpathConstantErrors(validateXPathConstants(parseMDL(t, src))) + if len(errs) != 2 { + t.Fatalf("got %d errors, want 2 (C_Min, C_Max): %v", len(errs), errs) + } +} + +// The check is part of ValidateProgram, the list `mxcli check` reports and +// `mxcli exec` refuses on — no project needed, the refusal does not depend on +// one. +func TestXPathConstants_RunsInValidateProgram(t *testing.T) { + errs, _ := xpathConstantErrors(ValidateProgram(parseMDL(t, xpathConstantSlots()["access rule"]), "")) + if len(errs) != 1 { + t.Fatalf("ValidateProgram did not report the constant: %v", errs) + } +} diff --git a/mdl/executor/workflow_start_note_test.go b/mdl/executor/workflow_start_note_test.go new file mode 100644 index 000000000..15e5a15b6 --- /dev/null +++ b/mdl/executor/workflow_start_note_test.go @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/backend/mock" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/workflows" +) + +// ako/mxcli#1717: a note a Studio Pro author puts on the workflow's start is +// stored on the start activity. describe prints it as the header's `annotation +// '…'` — it printed only the workflow's own Annotation, which Studio Pro shows +// nowhere, so a replay dropped the start's note — and CREATE writes the header +// clause back onto the start activity, not onto the workflow. +func TestWorkflowStartNote_SurvivesDescribeAndCreate(t *testing.T) { + mod := mkModule("Sales") + wf := mkWorkflow(mod.ID, "Approve") + start := &workflows.StartWorkflowActivity{} + start.Name, start.Caption, start.Annotation = "Start", "Start", "Started from the request form" + end := &workflows.EndWorkflowActivity{} + end.Name, end.Caption = "End", "End" + wf.Flow = &workflows.Flow{Activities: []workflows.WorkflowActivity{start, end}} + + h := mkHierarchy(mod) + withContainer(h, wf.ContainerID, mod.ID) + var written *workflows.Workflow + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil }, + ListWorkflowsFunc: func() ([]*workflows.Workflow, error) { return []*workflows.Workflow{wf}, nil }, + CreateWorkflowFunc: func(w *workflows.Workflow) error { written = w; return nil }, + } + ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h)) + assertNoError(t, describeWorkflow(ctx, ast.QualifiedName{Module: "Sales", Name: "Approve"})) + out := buf.String() + if !strings.Contains(out, "annotation 'Started from the request form'") { + t.Fatalf("describe does not print the start's note:\n%s", out) + } + + stmt := findStmt[*ast.CreateWorkflowStmt](t, reparse(t, out), out) + stmt.Name.Name = "ApproveCopy" + stmt.CreateOrModify = false + assertNoError(t, execCreateWorkflow(ctx, stmt)) + if written == nil || written.Flow == nil || len(written.Flow.Activities) == 0 { + t.Fatalf("nothing written: %+v", written) + } + s, ok := written.Flow.Activities[0].(*workflows.StartWorkflowActivity) + if !ok || s.Annotation != "Started from the request form" { + t.Errorf("start activity = %+v, want the note on it", written.Flow.Activities[0]) + } + if written.Annotation != "" { + t.Errorf("the note was also written to the workflow's own Annotation: %q", written.Annotation) + } +} diff --git a/mdl/executor/xpath_constant_1685_test.go b/mdl/executor/xpath_constant_1685_test.go new file mode 100644 index 000000000..3fa4fae27 --- /dev/null +++ b/mdl/executor/xpath_constant_1685_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#1685, the read side. A stored constraint holding `@Module.Constant` +// was re-parsed with `@` as an attribute name: describe's enum enrichment then +// printed `Total > @`, and the long-constraint layout wrote `[Total > @]` back. +// mxbuild rejects such a constraint (CE0161, see validateXPathConstants), but a +// stored one is still described as stored, not as something shorter. + +func TestXPathConstant_EnrichmentKeepsTheConstant(t *testing.T) { + got := enrichXPathGroups("[Status = 'Open' and Total > @Shop.C_Limit]", map[string]string{"Status": "Shop.S"}) + if want := "[Status = Shop.S.Open and Total > @Shop.C_Limit]"; got != want { + t.Errorf("enrichXPathGroups = %q, want %q", got, want) + } +} + +func TestXPathConstant_StoredLayoutKeepsTheConstant(t *testing.T) { + where := "Total > @Shop.C_Limit and Name != empty and Description != empty and Category = 'Hardware'" + got := retrieveXPathConstraint(&ast.SourceExpr{Source: where}, "Shop.Item") + if !strings.Contains(got, "Total > @Shop.C_Limit") { + t.Errorf("stored constraint lost the constant:\n%s", got) + } +} + +// End to end on a real project: the constraint is written as given, described +// back as written (with enum enrichment in play, which is where `@` went +// missing), re-executing the description writes nothing, and RENAME CONSTANT +// rewrites it — the path ako/mxcli#1671 could only unit-test, because MDL could +// not write a constant into a constraint. This runs the executor directly, past +// the MDL-XPATH03 refusal a script would meet first. +func TestXPathConstant_DescribeRoundTripAndRename(t *testing.T) { + exec, out, dir := openPedAppCopy(t) + setup := `create module Shop1685; +create constant Shop1685.C_Limit (Type: Integer, DefaultValue: '10'); +create enumeration Shop1685.Status (Open 'Open', Closed 'Closed'); +create persistent entity Shop1685.Item (Total: Integer, Name: String(100), Status: Shop1685.Status); +create module role Shop1685.User; +create microflow Shop1685.Fetch () +begin + retrieve $Items from Shop1685.Item where [Status = Shop1685.Status.Open and Total > @Shop1685.C_Limit]; +end; +create page Shop1685.Items (title: 'Items', layout: Atlas_Core.Atlas_Default) { + listview lv (datasource: database from Shop1685.Item where [Total > @Shop1685.C_Limit]) { + dynamictext t (Content: 'x') + } +}; +grant read * on entity Shop1685.Item to Shop1685.User where [Total > @Shop1685.C_Limit];` + if err := afRun(t, exec, setup); err != nil { + t.Fatalf("setup: %v\n%s", err, out.String()) + } + + describe := func(stmt string) string { + t.Helper() + out.Reset() + if err := afRun(t, exec, stmt); err != nil { + t.Fatalf("%s: %v\n%s", stmt, err, out.String()) + } + return out.String() + } + describeAll := func() map[string]string { + return map[string]string{ + "microflow": describe("describe microflow Shop1685.Fetch;"), + "page": describe("describe page Shop1685.Items;"), + "entity": describe("describe entity Shop1685.Item;"), + } + } + + descs := describeAll() + for doc, want := range map[string]string{ + "microflow": "Total > @Shop1685.C_Limit", + "page": "[Total > @Shop1685.C_Limit]", + "entity": "where [Total > @Shop1685.C_Limit]", + } { + if !strings.Contains(descs[doc], want) { + t.Errorf("describe %s lacks %q:\n%s", doc, want, descs[doc]) + } + } + + // Describe → exec writes nothing. + before := projectFiles(t, dir) + for doc, d := range descs { + if err := afRun(t, exec, d); err != nil { + t.Fatalf("re-executing the %s description: %v\n%s", doc, err, d) + } + } + if changed := diffProjectFiles(before, projectFiles(t, dir)); len(changed) != 0 { + t.Errorf("re-executing the descriptions rewrote %v", changed) + } + + out.Reset() + if err := afRun(t, exec, "rename constant Shop1685.C_Limit to C_Max;"); err != nil { + t.Fatalf("rename: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "Updated 3 reference(s)") { + t.Errorf("rename should report the three constraints it rewrote:\n%s", out.String()) + } + for doc, d := range describeAll() { + if strings.Contains(d, "C_Limit") || !strings.Contains(d, "@Shop1685.C_Max") { + t.Errorf("after the rename, describe %s should name @Shop1685.C_Max only:\n%s", doc, d) + } + } +} diff --git a/mdl/grammar/domains/MDLPage.g4 b/mdl/grammar/domains/MDLPage.g4 index 6db6b3a82..bbf73585f 100644 --- a/mdl/grammar/domains/MDLPage.g4 +++ b/mdl/grammar/domains/MDLPage.g4 @@ -143,10 +143,24 @@ xpathComparisonExpr xpathValueExpr : xpathFunctionCall | MINUS xpathValueExpr + | xpathConstantRef | xpathPath | LPAREN xpathExpr RPAREN ; +/** A constant, `@Module.Constant`, spelled as an expression spells one: a value, + * never a path step, so it takes no predicate and no `/`. + * + * Before this rule `@` was an xpathWord, a one-word name of its own, so + * `[Total > @Shop.C_Limit]` failed at `Shop` — and the lenient re-parse that + * describe and the long-constraint layout use recovered to an attribute called + * "@" and wrote the constraint back without the constant (ako/mxcli#1685). + * Parsing it is what lets `mxcli check` say what is actually wrong: mxbuild + * rejects a constant in an XPath constraint (CE0161), see validateXPathConstants. */ +xpathConstantRef + : AT xpathQualifiedName + ; + xpathPath : xpathStep (SLASH xpathStep)* ; @@ -176,13 +190,17 @@ xpathQualifiedName * HYPHENATED_ID (`starts-with`), so a standalone `-` is never part of a name. * While it was admissible here, `[Amount > -7]` parsed the sign as a name word * and left the digits stranded — reported as "negative literals truncate to -" - * (issuetracker finding #18). It is a unary operator; see xpathValueExpr. */ + * (issuetracker finding #18). It is a unary operator; see xpathValueExpr. + * + * AT is excluded for the same reason: `@` is never part of a name, it opens a + * constant (xpathConstantRef). Admitted here, `[Name = @]` compared with an + * attribute called "@" (ako/mxcli#1685). */ xpathWord : ~( DOT | SLASH | LBRACKET | RBRACKET | LPAREN | RPAREN | COMMA | EQUALS | NOT_EQUALS | LESS_THAN | LESS_THAN_OR_EQUAL | GREATER_THAN | GREATER_THAN_OR_EQUAL | AND | OR | NOT - | SEMICOLON | MINUS + | SEMICOLON | MINUS | AT | STRING_LITERAL | NUMBER_LITERAL | VARIABLE | MENDIX_TOKEN | DOLLAR_STRING ) ; diff --git a/mdl/grammar/domains/MDLWorkflow.g4 b/mdl/grammar/domains/MDLWorkflow.g4 index 51f211482..d16dffc85 100644 --- a/mdl/grammar/domains/MDLWorkflow.g4 +++ b/mdl/grammar/domains/MDLWorkflow.g4 @@ -443,8 +443,9 @@ alterWorkflowTargetMember ; // `Key: value`, the key naming a workflow property (Display, Description, -// ExportLevel, DueDate, OverviewPage, Parameter) or, with `on `, an -// activity property (Page, Description, Targeting, DueDate). Which keys exist +// ExportLevel, DueDate, OverviewPage, Parameter, Annotation) or, with `on +// `, an activity property (Page, Description, TaskName, Targeting, +// DueDate, Annotation). Which keys exist // is the visitor's call, so an unknown key is refused with the list. alterWorkflowAssignment : identifierOrKeyword COLON alterWorkflowValue diff --git a/mdl/types/reserved_words.go b/mdl/types/reserved_words.go index 49e6ed23e..aac105e0b 100644 --- a/mdl/types/reserved_words.go +++ b/mdl/types/reserved_words.go @@ -40,6 +40,11 @@ var PlatformReservedWords = map[string]bool{ "changedby": true, "changeddate": true, "con": true, "context": true, "createddate": true, "currentuser": true, "guid": true, "id": true, "mendixobject": true, "owner": true, "submetaobjectname": true, + // Not a Java keyword, and missing here until measured: CE7247 on mx + // 11.13.0 for an attribute, an enumeration value, an entity and a page, + // snippet, Java action and JavaScript action parameter named `Object` + // (ako/mxcli#1651). `String`, `System`, `User`, `List` are not reserved. + "object": true, } // SystemAttributeNames are attribute names the Mendix runtime manages itself on diff --git a/mdl/visitor/alter_workflow_braces_hint_test.go b/mdl/visitor/alter_workflow_braces_hint_test.go new file mode 100644 index 000000000..f21ef3eae --- /dev/null +++ b/mdl/visitor/alter_workflow_braces_hint_test.go @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "strings" + "testing" +) + +// ako/mxcli#1717: `alter workflow M.W set (Key: …);` without the braces failed +// to parse, and error recovery read `set (key: …)` as the session command `set +// = ` — so the last error said a session command does not belong +// in a script, which sent the author nowhere near the fix. +func TestAlterWorkflowWithoutBracesIsHinted(t *testing.T) { + _, errs := Build("mdl 1;\nalter workflow M.W set (Annotation: 'Started from the request form');\n") + if len(errs) == 0 { + t.Fatal("accepted") + } + var all []string + for _, e := range errs { + all = append(all, e.Error()) + } + joined := strings.Join(all, "\n") + if !strings.Contains(joined, "alter workflow M.W { set (Annotation:") { + t.Errorf("no braces hint naming the statement:\n%s", joined) + } + if strings.Contains(joined, "session command") { + t.Errorf("still diagnosed as a session command:\n%s", joined) + } +} + +// The controls: the braced form and the old per-action form parse, and a +// session command on a line of its own is still refused in a script. +func TestAlterWorkflowBracesHintControls(t *testing.T) { + for _, src := range []string{ + "mdl 1;\nalter workflow M.W { set (Annotation: 'x'); };\n", + "alter workflow M.W set display 'x';\n", + } { + if _, errs := Build(src); len(errs) != 0 { + t.Errorf("%q: %v", src, errs) + } + } + _, errs := Build("mdl 1;\nset autocommit = true;\n") + if len(errs) == 0 || !strings.Contains(errs[0].Error(), "session command") { + t.Errorf("a session command in a script: %v", errs) + } +} diff --git a/mdl/visitor/visitor.go b/mdl/visitor/visitor.go index c22a4fd98..0f4e49afe 100644 --- a/mdl/visitor/visitor.go +++ b/mdl/visitor/visitor.go @@ -188,6 +188,14 @@ func enhanceErrorMessageAt(msg, offendingLine string, site errorSite) string { " drop microflow if exists M.F; (correct)\n"+ " drop microflow M.F if exists; (SQL order, not MDL)", msg) } + // `alter workflow M.W set (…);` without the braces (ako/mxcli#1717). The + // old per-action form takes `set display '…'` unbraced, so the parser + // tries that, fails at the parenthesis, and recovers into the session + // command `set = `: no error so far names the braces. + if m := alterWorkflowNoBracesRe.FindStringSubmatch(offendingLine); m != nil { + return fmt.Sprintf("%s\n\n The generic `alter workflow` takes its operations in braces:\n"+ + " alter workflow %s { %s; }; (correct)", msg, m[1], strings.TrimRight(m[2], "; \t")) + } // An icon name with a hyphen (`add-circle`): it is not an identifier, so // the last segment is quoted — and only that segment, since an icon is a // qualified name, not a string. Unquoted the error points at a dot; quoted @@ -464,6 +472,11 @@ func enhanceErrorMessageAt(msg, offendingLine string, site errorSite) string { // the clause after the name rather than before it. var dropIfExistsAfterNameRe = regexp.MustCompile(`(?i)^\s*drop\s+[a-z][a-z ]*?\s+[\w."]+\.[\w."]+\s+if\s+exists\b`) +// alterWorkflowNoBracesRe matches the generic `set ( … )` operation written +// straight after `alter workflow `, without the braces: the name, and +// the operation. +var alterWorkflowNoBracesRe = regexp.MustCompile(`(?i)^\s*alter\s+workflow\s+([\w."]+)\s+(set\s*\(.*)$`) + // hyphenatedIconRe matches an icon reference whose last segment has a hyphen, // unquoted or quoted as one string: `Icon: Mod.Coll.add-circle` or // `Icon: 'Mod.Coll.add-circle'`. Group 1 is the collection, group 2 the name. diff --git a/mdl/visitor/visitor_alter_workflow.go b/mdl/visitor/visitor_alter_workflow.go index fd8b605f5..9d9205e1d 100644 --- a/mdl/visitor/visitor_alter_workflow.go +++ b/mdl/visitor/visitor_alter_workflow.go @@ -39,6 +39,7 @@ var workflowPropertyKeys = map[string]string{ "duedate": "due_date", "overviewpage": "overview_page", "parameter": "parameter", + "annotation": "annotation", } var activityPropertyKeys = map[string]string{ @@ -47,11 +48,12 @@ var activityPropertyKeys = map[string]string{ "taskname": "task_name", "targeting": "targeting", "duedate": "due_date", + "annotation": "annotation", } const ( - workflowKeysText = "Display, Description, ExportLevel, DueDate, OverviewPage, Parameter" - activityKeysText = "Page, Description, TaskName, Targeting, DueDate" + workflowKeysText = "Display, Description, ExportLevel, DueDate, OverviewPage, Parameter, Annotation" + activityKeysText = "Page, Description, TaskName, Targeting, DueDate, Annotation" ) // buildAlterWorkflowOperation builds one generic operation. A `set` list and a @@ -293,6 +295,16 @@ func alterWorkflowTemplateValue(v *parser.AlterWorkflowValueContext) (string, [] return s, buildWorkflowTemplateParams(v.TemplateParams()), true } +// alterWorkflowNoteValue reads a note's text (ako/mxcli#1717): a plain +// string, since a Workflows$Annotation stores a Description and no template. +// The empty string removes the note. +func alterWorkflowNoteValue(v *parser.AlterWorkflowValueContext) (string, bool) { + if v.TemplateParams() != nil { + return "", false + } + return alterWorkflowStringValue(v) +} + // alterWorkflowExpressionValue reads an expression-valued property (a due // date): the expression written bare, or — the deprecated form, MDL-DEPR080 — // its string's content, as workflowExpression reads it everywhere else. @@ -321,6 +333,13 @@ func (b *Builder) buildWorkflowSet(key, prop string, v *parser.AlterWorkflowValu return nil } op.Value, op.Params = s, params + case "annotation": + s, ok := alterWorkflowNoteValue(v) + if !ok { + b.alterWorkflowValueError(key, "a string: `"+key+": '…'`, or `"+key+": ''` to remove the note") + return nil + } + op.Value = s case "export_level": qn := v.QualifiedName() if qn == nil || v.VARIABLE() != nil || v.MICROFLOW() != nil || strings.Contains(qn.GetText(), ".") { @@ -373,6 +392,13 @@ func (b *Builder) buildActivitySet(key, prop string, v *parser.AlterWorkflowValu return nil } op.Value, op.Params = s, params + case "annotation": + s, ok := alterWorkflowNoteValue(v) + if !ok { + b.alterWorkflowValueError(key, "a string: `"+key+": '…'`, or `"+key+": ''` to remove the note") + return nil + } + op.Value = s case "targeting": switch { case v.MICROFLOW() != nil: diff --git a/mdl/visitor/visitor_alter_workflow_annotation_test.go b/mdl/visitor/visitor_alter_workflow_annotation_test.go new file mode 100644 index 000000000..a38722708 --- /dev/null +++ b/mdl/visitor/visitor_alter_workflow_annotation_test.go @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#1717: the notes CREATE writes — the header's `annotation '…'` and +// an activity's `@annotation '…'` — are set in ALTER as `Annotation`, on the +// workflow and with `on `; the empty string removes one. +func TestAlterWorkflow_Annotation(t *testing.T) { + prog := mustBuild(t, `alter workflow M.W { + set (Annotation: 'Started from the request form'); + set (Annotation: 'Logs it') on ACT_Log; + set (Annotation: '') on 'Review'@2; +};`) + ops := prog.Statements[0].(*ast.AlterWorkflowStmt).Operations + if len(ops) != 3 { + t.Fatalf("ops = %d, want 3", len(ops)) + } + wf, ok := ops[0].(*ast.SetWorkflowPropertyOp) + if !ok || wf.Property != "annotation" || wf.Value != "Started from the request form" { + t.Errorf("workflow op = %#v", ops[0]) + } + act, ok := ops[1].(*ast.SetActivityPropertyOp) + if !ok || act.Property != "annotation" || act.Value != "Logs it" || act.ActivityRef != "ACT_Log" { + t.Errorf("activity op = %#v", ops[1]) + } + clear, ok := ops[2].(*ast.SetActivityPropertyOp) + if !ok || clear.Property != "annotation" || clear.Value != "" || clear.ActivityRef != "Review" || clear.AtPosition != 2 { + t.Errorf("removing op = %#v", ops[2]) + } +} + +// A note is a plain string: Workflows$Annotation stores a Description and no +// template, so parameters, a page or an expression are refused with the form +// that works. +func TestAlterWorkflow_AnnotationTakesAString(t *testing.T) { + for _, src := range []string{ + `alter workflow M.W { set (Annotation: 'For {1}' with ({1} = $WorkflowContext/Name)); };`, + `alter workflow M.W { set (Annotation: M.Page) on Review; };`, + `alter workflow M.W { set (Annotation: addDays([%CurrentDateTime%], 1)); };`, + } { + _, errs := Build(src) + if len(errs) == 0 { + t.Errorf("%s: accepted", src) + continue + } + if !strings.Contains(errs[0].Error(), "Annotation takes a string") { + t.Errorf("%s: error %q", src, errs[0]) + } + } +} diff --git a/mdl/visitor/visitor_page_v3.go b/mdl/visitor/visitor_page_v3.go index c7b1e0a25..62be62d87 100644 --- a/mdl/visitor/visitor_page_v3.go +++ b/mdl/visitor/visitor_page_v3.go @@ -2034,6 +2034,10 @@ func xpathExprToString(expr ast.Expression) string { return e.Name + "(" + strings.Join(args, ", ") + ")" case *ast.TokenExpr: return "[%" + e.Token + "%]" + case *ast.ConstantRefExpr: + // Without a case the constant rendered as "": a data source written + // `where Total > @Shop.C_Limit` stored `[Total > ]` (ako/mxcli#1685). + return "@" + e.QualifiedName.String() case *ast.ParenExpr: return "(" + xpathExprToString(e.Inner) + ")" case *ast.IdentifierExpr: diff --git a/mdl/visitor/visitor_session_commands.go b/mdl/visitor/visitor_session_commands.go index 0832bc443..27ac92fa3 100644 --- a/mdl/visitor/visitor_session_commands.go +++ b/mdl/visitor/visitor_session_commands.go @@ -42,6 +42,12 @@ func (b *Builder) ExitUtilityStatement(ctx *parser.UtilityStatementContext) { // where a session command belongs, under every language version. return } + if b.syntaxErrorLines[ctx.GetStart().GetLine()] { + // Error recovery made it of a line that already failed to parse — + // `alter workflow M.W set (Key: …)` without its braces reads as `set + // (key` (ako/mxcli#1717) — and the syntax error is the one to act on. + return + } if b.gate(sessionCommandInScript, ctx) { b.addError(fmt.Errorf("line %d: `%s` is a session command, not a model statement: under %s a script "+ "holds model statements only. Type it at the REPL, or use the command-line flag "+ diff --git a/mdl/visitor/visitor_xpath.go b/mdl/visitor/visitor_xpath.go index e1837d72e..101f7a9e0 100644 --- a/mdl/visitor/visitor_xpath.go +++ b/mdl/visitor/visitor_xpath.go @@ -133,6 +133,12 @@ func buildXPathValueExpr(ctx parser.IXpathValueExprContext) ast.Expression { } } + // Constant: @Module.Constant (ako/mxcli#1685). Before it had a rule, `@` + // parsed as an attribute name and the constant was dropped on re-render. + if c := valCtx.XpathConstantRef(); c != nil { + return buildXPathConstantRef(c) + } + // Path: step/step/step if path := valCtx.XpathPath(); path != nil { return buildXPathPath(path) @@ -272,6 +278,29 @@ func buildXPathQualifiedName(ctx parser.IXpathQualifiedNameContext) ast.Expressi } } +// buildXPathConstantRef builds a ConstantRefExpr from xpathConstantRef, split +// the way an expression's `@Module.Name` is: the first word is the module. +// A recovered parse may hand over the `@` alone; that yields an empty name +// rather than a nil dereference (see the-visitor-walks-a-failed-parse). +func buildXPathConstantRef(ctx parser.IXpathConstantRefContext) ast.Expression { + ref := &ast.ConstantRefExpr{} + c, ok := ctx.(*parser.XpathConstantRefContext) + if !ok || c.XpathQualifiedName() == nil { + return ref + } + words := c.XpathQualifiedName().(*parser.XpathQualifiedNameContext).AllXpathWord() + parts := make([]string, len(words)) + for i, w := range words { + parts[i] = unquoteIdentifier(w.GetText()) + } + if len(parts) == 1 { + ref.QualifiedName.Name = parts[0] + } else if len(parts) > 1 { + ref.QualifiedName = ast.QualifiedName{Module: parts[0], Name: strings.Join(parts[1:], ".")} + } + return ref +} + // buildXPathFunctionCall builds a FunctionCallExpr from xpathFunctionCall. func buildXPathFunctionCall(ctx parser.IXpathFunctionCallContext) ast.Expression { if ctx == nil { diff --git a/mdl/visitor/visitor_xpath_constant_test.go b/mdl/visitor/visitor_xpath_constant_test.go new file mode 100644 index 000000000..586b11158 --- /dev/null +++ b/mdl/visitor/visitor_xpath_constant_test.go @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +// ako/mxcli#1685: an XPath constraint could not name a constant. Mendix XPath +// writes one as `@Module.Constant`, exactly as an expression does, but the +// constraint grammar had no value of that shape: `@` was admitted as a ONE-WORD +// name on its own, so `[Total > @Shop.C_Limit]` failed at `Shop` ("mismatched +// input 'Shop' expecting ']'") and a Studio Pro-authored retrieve or data source +// using a constant could not be described and executed back. +// +// The lenient parse describe and the formatter use was worse than the error: it +// recovered past the module name and handed back the bare `@` as an attribute, +// so anything that re-rendered the constraint dropped the constant. + +func TestXPath_ConstantReference_IsAConstantNotAName(t *testing.T) { + expr, ok := ParseXPathConstraint("[Total > @Shop.C_Limit]") + if !ok { + t.Fatal("ParseXPathConstraint did not parse a constant comparand") + } + bin, ok := expr.(*ast.BinaryExpr) + if !ok { + t.Fatalf("built %T, want *ast.BinaryExpr", expr) + } + c, ok := bin.Right.(*ast.ConstantRefExpr) + if !ok { + t.Fatalf("right operand = %T (%#v), want *ast.ConstantRefExpr", bin.Right, bin.Right) + } + if got := c.QualifiedName.String(); got != "Shop.C_Limit" { + t.Errorf("constant = %q, want Shop.C_Limit", got) + } +} + +// Every position a value takes in a constraint: either side of a comparison, a +// function argument, inside a nested predicate, under not(), and next to the +// other operand kinds. The re-render is what describe enrichment and the +// long-constraint formatter write back, so it must give back the input. +func TestXPath_ConstantReference_RoundTrips(t *testing.T) { + for _, in := range []string{ + "[Total > @Shop.C_Limit]", + "[@Shop.C_Limit <= Total]", + "[contains(Name, @Shop.C_Prefix)]", + "[Shop.Item_Order/Shop.Order[Total >= @Shop.C_Limit]/Name = @Shop.C_Name]", + "[Total > @Shop.C_Limit and Name != empty]", + "[not(Total > @Shop.C_Limit)]", + "[(Total > @Shop.C_Min or Total < @Shop.C_Max) and Name = $Name]", + "[Total > -@Shop.C_Limit]", + // A keyword is a name part in XPath; so it is in a constant's name. + "[Total > @Shop.Limit]", + "[Total > @Shop.Status]", + } { + expr, ok := ParseXPathConstraint(in) + if !ok { + t.Errorf("ParseXPathConstraint(%q) did not parse", in) + continue + } + if got := "[" + xpathExprToString(expr) + "]"; got != in { + t.Errorf("round trip of %q = %q", in, got) + } + } +} + +// `@` alone is not a name. While xpathWord admitted it, `[Name = @]` parsed as +// a comparison with an attribute called "@". +func TestXPath_ConstantReference_BareAtIsNotAName(t *testing.T) { + src := "create microflow Shop.F ()\nbegin\n retrieve $Items from Shop.Item where [Name = @];\nend;\n" + if _, errs := Build(src); len(errs) == 0 { + t.Error("`[Name = @]` parsed; want a syntax error") + } + // The lenient re-parse hands the visitor a recovered tree with the name + // missing; it must not dereference what is not there. + for _, in := range []string{"[Name = @]", "[Name = @ and Total > 1]", "[@]", "[Name = @.]"} { + _, _ = ParseXPathConstraint(in) + } +} + +// The three writers keep the constant as written: the retrieve and the access +// rule store their source text, the data source re-renders its parse. +func TestXPath_ConstantReference_StoredVerbatimInEverySlot(t *testing.T) { + t.Run("retrieve", func(t *testing.T) { + for _, tc := range []struct{ where, want string }{ + {"[Total > @Shop.C_Limit]", "[Total > @Shop.C_Limit]"}, + {"[Total > @Shop.C_Limit][Name != empty]", "[Total > @Shop.C_Limit][Name != empty]"}, + } { + prog := mustBuild(t, "create microflow Shop.F ()\nbegin\n retrieve $Items from Shop.Item where "+tc.where+";\nend;\n") + mf := prog.Statements[0].(*ast.CreateMicroflowStmt) + r, ok := mf.Body[0].(*ast.RetrieveStmt) + if !ok { + t.Fatalf("body[0] = %T, want *ast.RetrieveStmt", mf.Body[0]) + } + src, ok := r.Where.(*ast.SourceExpr) + if !ok { + t.Fatalf("where %s built %T, want *ast.SourceExpr", tc.where, r.Where) + } + if got := strings.TrimSpace(src.Source); got != tc.want && "["+got+"]" != tc.want { + t.Errorf("where %s stored as %q, want %q", tc.where, got, tc.want) + } + } + }) + + t.Run("data source", func(t *testing.T) { + // The unbracketed form goes through the expression grammar, which always + // had constants — and then through the same renderer, which had no case + // for one and stored `[Total > ]`. + for _, where := range []string{ + "[Total > @Shop.C_Limit and Name != empty]", + "Total > @Shop.C_Limit and Name != empty", + } { + prog := mustBuild(t, `create page Shop.P (title: 'T', layout: Atlas_Core.Atlas_Default) { + gallery g (datasource: database from Shop.Item where `+where+`) { + dynamictext t (content: 'x') + } +}`) + page := prog.Statements[0].(*ast.CreatePageStmtV3) + ds := findWidget(page.Widgets, "g").Properties["DataSource"].(*ast.DataSourceV3) + if want := "[Total > @Shop.C_Limit and Name != empty]"; ds.Where != want { + t.Errorf("where %s: datasource Where = %q, want %q", where, ds.Where, want) + } + } + }) + + t.Run("access rule", func(t *testing.T) { + g, _ := grantStmt(t, `grant read * on entity Shop.Item to Shop.User where [Total > @Shop.C_Limit];`) + if want := "[Total > @Shop.C_Limit]"; g.XPathConstraint != want { + t.Errorf("XPathConstraint = %q, want %q", g.XPathConstraint, want) + } + }) +} + +// A constraint too long for one line is laid out from its parse. The layout +// must keep the constant; a recovered parse that read `@` as a name wrote the +// constraint back without it. +func TestXPath_ConstantReference_SurvivesTheLongConstraintLayout(t *testing.T) { + in := "[Total > @Shop.C_Limit and Name != empty and Description != empty and Category = 'Hardware']" + got := FormatXPathConstraint(in) + if got == in { + t.Fatalf("constraint of %d characters was not laid out; the test needs one that is", len(in)) + } + if !strings.Contains(got, "Total > @Shop.C_Limit") { + t.Errorf("layout dropped the constant:\n%s", got) + } + if flat := FlattenXPathConstraint(got); flat != in { + t.Errorf("layout changed the constraint:\n got %s\nwant %s", flat, in) + } +} diff --git a/sdk/workflows/workflow.go b/sdk/workflows/workflow.go index 46d57088b..dc57904ce 100644 --- a/sdk/workflows/workflow.go +++ b/sdk/workflows/workflow.go @@ -40,7 +40,9 @@ type Workflow struct { DueDate string `json:"dueDate,omitempty"` // Due date expression - // Annotation + // Annotation is the workflow's own note. Studio Pro 11.15 shows it nowhere: + // the note on the start is the start activity's (StartNote). mxcli wrote + // the header `annotation '…'` here before ako/mxcli#1717. Annotation string `json:"annotation,omitempty"` // Annotation description text // Context parameter @@ -121,6 +123,23 @@ func (a *BaseWorkflowActivity) SetName(name string) { } // GetCaption returns the activity's caption. +// StartNote is the note on the workflow's start, which describe prints as the +// header's `annotation '…'`: the start activity's, as Studio Pro stores and +// shows it — the workflow editor draws the start's note from +// StartWorkflowActivity.Annotation and the workflow's own Annotation nowhere +// (11.15.0, ako/mxcli#1717) — or else the workflow's own, where mxcli wrote it +// before. +func (w *Workflow) StartNote() string { + if w.Flow != nil { + for _, a := range w.Flow.Activities { + if s, ok := a.(*StartWorkflowActivity); ok && s.Annotation != "" { + return s.Annotation + } + } + } + return w.Annotation +} + // SetAnnotation sets the note attached to the activity. func (a *BaseWorkflowActivity) SetAnnotation(s string) { a.Annotation = s