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/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.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/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 cd6dce27b..519c86cb8 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/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/executor/cmd_alter_page.go b/mdl/executor/cmd_alter_page.go index 9d213ef8f..928b9d385 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 ea98f08f6..05c80c5da 100644 --- a/mdl/executor/cmd_javaactions.go +++ b/mdl/executor/cmd_javaactions.go @@ -458,6 +458,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_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/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/validate.go b/mdl/executor/validate.go index 9163fac70..17c4c739c 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -567,6 +567,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_program.go b/mdl/executor/validate_program.go index 5fbf7f6dc..4fcda8bde 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)...) @@ -185,6 +190,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. 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/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