From ad256e5728d32751e9afb1566ba1da4cee08e0d2 Mon Sep 17 00:00:00 2001 From: Ako Date: Fri, 18 Sep 2026 10:48:17 +0000 Subject: [PATCH] =?UTF-8?q?feat(entities):=20ALTER=20ENTITIES=20=E2=80=94?= =?UTF-8?q?=20the=20bulk=20ADD=20ATTRIBUTE=20form?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Giving every entity in a module an audit trail cost one statement per entity. Measured on a real module: 10 statements, ~242 tokens, against one statement at ~25 -- and at 200 entities the single-entity form is ~9,700 tokens of near-identical text for an agent to emit and a human to review. alter entities in Sales add attribute if not exists CreatedDate: AutoCreatedDate, add attribute if not exists ChangedDate: AutoChangedDate where persistent; Scope is deliberately narrow, following ALTER PAGES, which is bulk for exactly one operation. ADD ATTRIBUTE only: DROP and RENAME aimed at a set are destructive by a typo, and SET POSITION on every entity is meaningless. WHERE reuses the persistence words CREATE ENTITY already uses rather than inventing a predicate language, so no new lexer token was needed. The executor resolves the target set and then runs each action through execAlterEntity UNCHANGED. That delegation is the design: the single-entity path already carries the reserved-word refusals, access-rule reconciliation, the IF NOT EXISTS skip and write elision, and a second implementation would have to be kept in step with all of it. Three exclusions, and mxbuild taught me two of them -- the first version of this passed every unit test and produced a project with 12 errors: - A VIEW entity is never a target, with or without a filter. Its columns come from its OQL select list, so an added attribute is CE6770 "View Entity is out of sync with the OQL Query" (10 of the 12). - A SPECIALIZATION whose ancestor is also a target is skipped: the same name on a generalization and its child is CE0069 "Duplicate member name" (the other 2, on DmTest.Vehicle/Truck/PassengerCar). The parent is kept and the child inherits the member, which is what the author wanted. - An UNSCOPED sweep skips System and every Marketplace module, and reports which. An upgrade replaces those modules and takes the attribute with it, so the write would be silently undone later rather than refused now. Naming a module with IN is taken as meaning it, the same division `mxcli layout` makes. Verified on a real 11.14 project: one statement gives all 5 persistent entities in a module both audit members, a re-run reports each as already present, and an unscoped sweep skips 8 System/Marketplace modules and touches only the 2 user ones. mxbuild is clean -- the doctype script now exercises all three filter forms and `TestMxCheck_DoctypeScripts` passes at 0 errors, having failed at 12 before the exclusions. Full stack per the checklist: grammar, AST, visitor, executor, syntax topic, quick reference, doctype example, and tests at the parser and executor layers. The registry handler-count snapshot is updated, which is the guard working as intended. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BNDe35kDNsMX5cz4Ahn4rk --- cmd/mxcli/syntax/features_domain_model.go | 7 +- docs/01-project/MDL_QUICK_REFERENCE.md | 1 + .../01-domain-model-examples.mdl | 28 +++ mdl/ast/ast_entity.go | 29 +++ mdl/executor/cmd_entities_bulk.go | 207 ++++++++++++++++++ mdl/executor/cmd_entities_bulk_test.go | 195 +++++++++++++++++ mdl/executor/register_stubs.go | 3 + mdl/executor/registry_test.go | 1 + mdl/grammar/MDLParser.g4 | 1 + mdl/grammar/domains/MDLDomainModel.g4 | 28 +++ mdl/visitor/visitor_alter_entities_test.go | 87 ++++++++ mdl/visitor/visitor_entity.go | 50 +++++ 12 files changed, 634 insertions(+), 3 deletions(-) create mode 100644 mdl/executor/cmd_entities_bulk.go create mode 100644 mdl/executor/cmd_entities_bulk_test.go create mode 100644 mdl/visitor/visitor_alter_entities_test.go diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index de35eeb79b..b3175a7364 100644 --- a/cmd/mxcli/syntax/features_domain_model.go +++ b/cmd/mxcli/syntax/features_domain_model.go @@ -88,15 +88,16 @@ func init() { Register(SyntaxFeature{ Path: "domain-model.entity.alter", - Summary: "ALTER ENTITY: add/rename/modify/drop attributes, indexes, documentation, event handlers", + Summary: "ALTER ENTITY: add/rename/modify/drop attributes, indexes, documentation, event handlers; ALTER ENTITIES for the bulk add", Keywords: []string{ "alter entity", "modify entity", "add attribute", "drop attribute", "rename attribute", "add index", "event handler", "documentation", "if not exists", "if exists", "idempotent", + "alter entities", "bulk", "every entity", "all entities", "where persistent", }, - Syntax: "ALTER ENTITY Module.Name ADD ATTRIBUTE [IF NOT EXISTS] AttrName: Type [constraints];\nALTER ENTITY Module.Name DROP ATTRIBUTE [IF EXISTS] AttrName;\nALTER ENTITY Module.Name RENAME ATTRIBUTE OldName TO NewName;\nALTER ENTITY Module.Name MODIFY ATTRIBUTE AttrName Type [DEFAULT val];\nALTER ENTITY Module.Name DROP DEFAULT ON ATTRIBUTE AttrName;\nALTER ENTITY Module.Name ADD INDEX [name] [ON] (attr1, attr2);\nALTER ENTITY Module.Name SET DOCUMENTATION 'text';\nALTER ENTITY Module.Name SET POSITION (x, y);\nALTER ENTITY Module.Name ADD EVENT HANDLER ON BEFORE COMMIT CALL Module.MF RAISE ERROR;\n\nSET POSITION places the entity in the domain-model editor, and CREATE ENTITY\ntakes the same thing as an @Position(x, y) annotation. Both are the box's\nCENTRE, not its top-left corner. An entity created without one takes the next\nslot in a wrapping grid, which is a default rather than a layout: to arrange a\nwhole module from its association graph, run 'mxcli layout -p app.mpr'\n(--dry-run first; it replaces positions you set by hand).\n\nMODIFY ATTRIBUTE always takes a type — restate it even when you are only\nchanging the default. There is no 'MODIFY ATTRIBUTE X SET DEFAULT v' form:\nSET would be read as the type name. Use DROP DEFAULT to clear one.\n\nIF NOT EXISTS / IF EXISTS make the add/drop a no-op (skipped, not an error)\nwhen the attribute is already present / already gone — so a domain script\nre-runs cleanly. For a whole script, 'mxcli exec --continue-on-error' reports\neach failed statement and keeps going instead of halting at the first.\n\nRENAME ATTRIBUTE also rewrites every reference to the attribute: the stored\nqualified names (microflow create/change members, page widgets, the entity's own\nvalidation and access rules) AND the bare steps inside XPath constraints, which\nare resolved to their owning entity first so another entity's identically-named\nattribute is left alone. A constraint that cannot be resolved is reported and\nleft unchanged, never guessed at. Uses inside microflow expressions ($obj/Attr)\nare free text and are NOT rewritten; mxbuild reports those as CE0117.", - Example: "ALTER ENTITY Shop.Customer ADD ATTRIBUTE Phone: String(20);\nALTER ENTITY Shop.Customer ADD ATTRIBUTE IF NOT EXISTS Phone: String(20); -- re-runnable\nALTER ENTITY Shop.Customer DROP ATTRIBUTE IF EXISTS OldField; -- re-runnable\nALTER ENTITY Shop.Customer RENAME ATTRIBUTE Email TO EmailAddress;\nALTER ENTITY Shop.Customer MODIFY ATTRIBUTE Phone String(30) DEFAULT ''; -- type restated\nALTER ENTITY Shop.Customer DROP DEFAULT ON ATTRIBUTE Phone; -- clear a default\nALTER ENTITY Shop.Customer ADD INDEX ON (EmailAddress);\nALTER ENTITY Shop.Customer\n ADD EVENT HANDLER ON BEFORE COMMIT CALL Shop.Validate($currentObject) RAISE ERROR;", + Syntax: "ALTER ENTITY Module.Name ADD ATTRIBUTE [IF NOT EXISTS] AttrName: Type [constraints];\nALTER ENTITY Module.Name DROP ATTRIBUTE [IF EXISTS] AttrName;\nALTER ENTITY Module.Name RENAME ATTRIBUTE OldName TO NewName;\nALTER ENTITY Module.Name MODIFY ATTRIBUTE AttrName Type [DEFAULT val];\nALTER ENTITY Module.Name DROP DEFAULT ON ATTRIBUTE AttrName;\nALTER ENTITY Module.Name ADD INDEX [name] [ON] (attr1, attr2);\nALTER ENTITY Module.Name SET DOCUMENTATION 'text';\nALTER ENTITY Module.Name SET POSITION (x, y);\nALTER ENTITY Module.Name ADD EVENT HANDLER ON BEFORE COMMIT CALL Module.MF RAISE ERROR;\nALTER ENTITIES [IN Module] ADD ATTRIBUTE [IF NOT EXISTS] AttrName: Type [, ...]\n [WHERE PERSISTENT | WHERE NON-PERSISTENT];\n\nALTER ENTITIES is the bulk form: one statement applied to every entity in a\nmodule instead of one statement per entity. Only ADD ATTRIBUTE is offered --\nDROP and RENAME aimed at a set are destructive by a typo, and SET POSITION on\nevery entity is meaningless. Pair it with IF NOT EXISTS so the script re-runs.\n\nWHERE filters by persistence, using the same words CREATE ENTITY uses. A VIEW\nentity matches NEITHER: its rows come from an OQL query, so it is not the\npersistent/non-persistent distinction this filter means.\n\nWITHOUT IN, the sweep covers the whole project but SKIPS System and every\nMarketplace module, reporting which -- an upgrade replaces those modules and\nwould take the attribute with it. Naming a module with IN is taken as meaning\nit, so a deliberate edit there is still possible.\n\nSET POSITION places the entity in the domain-model editor, and CREATE ENTITY\ntakes the same thing as an @Position(x, y) annotation. Both are the box's\nCENTRE, not its top-left corner. An entity created without one takes the next\nslot in a wrapping grid, which is a default rather than a layout: to arrange a\nwhole module from its association graph, run 'mxcli layout -p app.mpr'\n(--dry-run first; it replaces positions you set by hand).\n\nMODIFY ATTRIBUTE always takes a type — restate it even when you are only\nchanging the default. There is no 'MODIFY ATTRIBUTE X SET DEFAULT v' form:\nSET would be read as the type name. Use DROP DEFAULT to clear one.\n\nIF NOT EXISTS / IF EXISTS make the add/drop a no-op (skipped, not an error)\nwhen the attribute is already present / already gone — so a domain script\nre-runs cleanly. For a whole script, 'mxcli exec --continue-on-error' reports\neach failed statement and keeps going instead of halting at the first.\n\nRENAME ATTRIBUTE also rewrites every reference to the attribute: the stored\nqualified names (microflow create/change members, page widgets, the entity's own\nvalidation and access rules) AND the bare steps inside XPath constraints, which\nare resolved to their owning entity first so another entity's identically-named\nattribute is left alone. A constraint that cannot be resolved is reported and\nleft unchanged, never guessed at. Uses inside microflow expressions ($obj/Attr)\nare free text and are NOT rewritten; mxbuild reports those as CE0117.", + Example: "ALTER ENTITY Shop.Customer ADD ATTRIBUTE Phone: String(20);\nALTER ENTITY Shop.Customer ADD ATTRIBUTE IF NOT EXISTS Phone: String(20); -- re-runnable\nALTER ENTITY Shop.Customer DROP ATTRIBUTE IF EXISTS OldField; -- re-runnable\nALTER ENTITY Shop.Customer RENAME ATTRIBUTE Email TO EmailAddress;\nALTER ENTITY Shop.Customer MODIFY ATTRIBUTE Phone String(30) DEFAULT ''; -- type restated\nALTER ENTITY Shop.Customer DROP DEFAULT ON ATTRIBUTE Phone; -- clear a default\nALTER ENTITY Shop.Customer ADD INDEX ON (EmailAddress);\nALTER ENTITY Shop.Customer\n ADD EVENT HANDLER ON BEFORE COMMIT CALL Shop.Validate($currentObject) RAISE ERROR;\n\n-- give every persistent entity in a module an audit trail, in one statement\nALTER ENTITIES IN Shop\n ADD ATTRIBUTE IF NOT EXISTS CreatedDate: AutoCreatedDate,\n ADD ATTRIBUTE IF NOT EXISTS ChangedDate: AutoChangedDate\n WHERE PERSISTENT;", SeeAlso: []string{"domain-model.entity.create", "domain-model.entity.attributes"}, }) diff --git a/docs/01-project/MDL_QUICK_REFERENCE.md b/docs/01-project/MDL_QUICK_REFERENCE.md index 25eb48a807..cecc992f28 100644 --- a/docs/01-project/MDL_QUICK_REFERENCE.md +++ b/docs/01-project/MDL_QUICK_REFERENCE.md @@ -102,6 +102,7 @@ Modifies an existing entity without full replacement. | Set position | `alter entity Module.Name set position (100, 200);` | Canvas position | | Add system attribute | `alter entity Module.Name add attribute owner: autoowner;` | Same syntax as regular attributes | | Drop system attribute | `alter entity Module.Name drop attribute owner;` | Drop by system attribute name | +| Add attribute to every entity | `alter entities [in Module] add attribute [if not exists] attr: type [, ...] [where persistent\|non-persistent];` | The bulk form — one statement instead of one per entity. **ADD ATTRIBUTE only**: drop/rename aimed at a set are destructive by a typo. A **view** entity matches neither persistence filter. **Without `in`**, the sweep skips System and every Marketplace module (and says which) — an upgrade replaces those and would take the attribute with it | > **Re-running domain scripts.** `IF NOT EXISTS` / `IF EXISTS` make an individual > create/add/drop a no-op when already applied — accepted on `create entity`, diff --git a/mdl-examples/doctype-tests/01-domain-model-examples.mdl b/mdl-examples/doctype-tests/01-domain-model-examples.mdl index 9fa73c6513..dc10791952 100644 --- a/mdl-examples/doctype-tests/01-domain-model-examples.mdl +++ b/mdl-examples/doctype-tests/01-domain-model-examples.mdl @@ -1881,6 +1881,34 @@ type ReferenceSet owner both; / +-- ############################################################################ +-- LEVEL 13: BULK ALTER — one statement for a whole module +-- ############################################################################ +-- +-- ALTER ENTITIES replaces one ALTER ENTITY per entity. Only ADD ATTRIBUTE is +-- offered: drop and rename aimed at a set are destructive by a typo. +-- +-- IF NOT EXISTS is what makes it re-runnable, and this file is re-run on every +-- doctype pass — the second run must skip rather than fail. + +-- Give every persistent entity in the module an audit trail. +alter entities in DmTest + add attribute if not exists AuditCreated: AutoCreatedDate, + add attribute if not exists AuditChanged: AutoChangedDate + where persistent; +/ + +-- The non-persistent half of the module, addressed on its own. +alter entities in DmTest + add attribute if not exists ScratchNote: String(40) + where non-persistent; +/ + +-- No filter: every entity in the module, view entities included. +alter entities in DmTest + add attribute if not exists BulkTouched: Boolean; +/ + -- Verify generalized entities appear in listings show entities in DmTest; describe entity DmTest.Attachment; diff --git a/mdl/ast/ast_entity.go b/mdl/ast/ast_entity.go index 321125eed5..2e2ef405e1 100644 --- a/mdl/ast/ast_entity.go +++ b/mdl/ast/ast_entity.go @@ -128,6 +128,35 @@ type AlterEntityStmt struct { func (s *AlterEntityStmt) isStatement() {} +// EntityPersistenceFilter narrows ALTER ENTITIES to one persistence kind. +type EntityPersistenceFilter int + +const ( + // EntityFilterAll is the absence of a WHERE clause. + EntityFilterAll EntityPersistenceFilter = iota + EntityFilterPersistent + EntityFilterNonPersistent +) + +// AlterEntitiesStmt is the bulk form: one statement applied to every entity in +// a module, rather than one statement per entity. +// +// It carries a LIST of AlterEntityStmt rather than its own operation fields. +// The executor resolves the target set and then runs each action through the +// single-entity path unchanged, so every domain rule, refusal and idempotence +// guard that applies to ALTER ENTITY applies here for free and cannot drift. +type AlterEntitiesStmt struct { + // Module is the module to sweep. Empty means every module in the project. + Module string + // Filter narrows the set by persistence; EntityFilterAll means no WHERE. + Filter EntityPersistenceFilter + // Actions are applied to each matched entity, in order. Each carries a + // zero Name; the executor fills it in per entity. + Actions []*AlterEntityStmt +} + +func (s *AlterEntitiesStmt) isStatement() {} + // ============================================================================ // View Entity Statements // ============================================================================ diff --git a/mdl/executor/cmd_entities_bulk.go b/mdl/executor/cmd_entities_bulk.go new file mode 100644 index 0000000000..8605a69461 --- /dev/null +++ b/mdl/executor/cmd_entities_bulk.go @@ -0,0 +1,207 @@ +// 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/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// execAlterEntities handles the bulk form: +// +// ALTER ENTITIES [IN ] ADD ATTRIBUTE ... [WHERE PERSISTENT|NON-PERSISTENT] +// +// It resolves the target set and then runs each action through execAlterEntity +// unchanged. That delegation is the whole design: the single-entity path +// already carries the reserved-word refusals, the access-rule reconciliation, +// the IF NOT EXISTS skip and the write elision, and a second implementation +// would have to be kept in step with all of it. One statement per entity is +// what this replaces, not the code that executes one. +func execAlterEntities(ctx *ExecContext, s *ast.AlterEntitiesStmt) error { + if !ctx.Connected() { + return mdlerrors.NewNotConnected() + } + + targets, err := resolveBulkEntityTargets(ctx, s) + if err != nil { + return err + } + if len(targets) == 0 { + // Not an error: a module whose entities all match already, or a filter + // that excludes everything, is a legitimate no-op in a re-run script. + fmt.Fprintf(ctx.Output, "No entities matched%s\n", bulkScopeSuffix(s)) + return nil + } + + for _, qn := range targets { + for _, action := range s.Actions { + // Copy per entity: execAlterEntity reads Name, and the parsed + // action is shared across every target. + perEntity := *action + perEntity.Name = qn + if err := execAlterEntity(ctx, &perEntity); err != nil { + return fmt.Errorf("%s: %w", qn, err) + } + } + } + + fmt.Fprintf(ctx.Output, "Altered %d entit%s%s\n", + len(targets), plural(len(targets), "y", "ies"), bulkScopeSuffix(s)) + return nil +} + +// resolveBulkEntityTargets returns the qualified names the statement applies +// to, sorted, so a run is deterministic and its output diffable. +func resolveBulkEntityTargets(ctx *ExecContext, s *ast.AlterEntitiesStmt) ([]ast.QualifiedName, error) { + modules, err := ctx.Backend.ListModules() + if err != nil { + return nil, mdlerrors.NewBackend("list modules", err) + } + + var out []ast.QualifiedName + var skipped []string + // qualified name -> its generalization's qualified name ("" at the root) + parentOf := map[string]string{} + matchedModule := false + for _, m := range modules { + if s.Module != "" && !strings.EqualFold(m.Name, s.Module) { + continue + } + matchedModule = true + + // Sweeping the whole project must not edit System or a Marketplace + // module: an upgrade replaces the module and the attribute goes with + // it, so the write is silently undone later rather than refused now. + // Naming the module explicitly is taken as meaning it -- the same + // division `mxcli layout` makes. + if s.Module == "" && isUneditableModule(m) { + skipped = append(skipped, m.Name) + continue + } + + dm, err := ctx.Backend.GetDomainModel(m.ID) + if err != nil { + return nil, mdlerrors.NewBackend("get domain model", err) + } + if dm == nil { + continue + } + for _, e := range dm.Entities { + if !entityMatchesFilter(e, s.Filter) { + continue + } + out = append(out, ast.QualifiedName{Module: m.Name, Name: e.Name}) + parentOf[m.Name+"."+e.Name] = e.GeneralizationRef + } + } + + if s.Module != "" && !matchedModule { + return nil, mdlerrors.NewNotFoundMsg("module", s.Module, + fmt.Sprintf("module not found: %s", s.Module)) + } + + if len(skipped) > 0 { + sort.Strings(skipped) + fmt.Fprintf(ctx.Output, "Skipped %d System/Marketplace module(s): %s\n", + len(skipped), strings.Join(skipped, ", ")) + } + + out = dropInheritedTargets(out, parentOf) + + sort.Slice(out, func(i, j int) bool { + if out[i].Module != out[j].Module { + return out[i].Module < out[j].Module + } + return out[i].Name < out[j].Name + }) + return out, nil +} + +// entityMatchesFilter applies the WHERE clause. A view entity is neither +// persistent nor non-persistent in the sense this filter means -- its rows come +// from an OQL query -- so it is excluded from both, rather than silently +// treated as one of them. +func entityMatchesFilter(e *domainmodel.Entity, f ast.EntityPersistenceFilter) bool { + if e == nil { + return false + } + // A view entity is never a target, with or without a filter: its columns + // are defined by its OQL select list, and adding an attribute gives + // CE6770 "View Entity is out of sync with the OQL Query". + if e.Source != "" || e.OqlQuery != "" { + return false + } + switch f { + case ast.EntityFilterPersistent: + return e.Persistable + case ast.EntityFilterNonPersistent: + return !e.Persistable + default: + return true + } +} + +func bulkScopeSuffix(s *ast.AlterEntitiesStmt) string { + scope := "" + if s.Module != "" { + scope = " in " + s.Module + } + switch s.Filter { + case ast.EntityFilterPersistent: + scope += " (persistent only)" + case ast.EntityFilterNonPersistent: + scope += " (non-persistent only)" + } + return scope +} + +// isUneditableModule reports a module whose contents an upgrade replaces. +func isUneditableModule(m *model.Module) bool { + if m == nil { + return false + } + if m.Name == "System" { + return true + } + return m.FromAppStore || strings.TrimSpace(m.AppStoreGuid) != "" +} + +// dropInheritedTargets removes an entity whose ANCESTOR is also a target. +// +// Mendix refuses an attribute name that appears on both a generalization and +// its specialization -- CE0069 "Duplicate member name" -- and adding it to the +// parent is what the author wants anyway, since the child inherits it. Without +// this, ALTER ENTITIES over a module with any inheritance produced a project +// that would not build, which is how DmTest.Vehicle/Truck/PassengerCar caught +// it. The child is dropped rather than the parent: dropping the parent would +// leave the attribute off entities that have no specialization. +func dropInheritedTargets(targets []ast.QualifiedName, parentOf map[string]string) []ast.QualifiedName { + inSet := make(map[string]bool, len(targets)) + for _, t := range targets { + inSet[t.Module+"."+t.Name] = true + } + + kept := targets[:0] + for _, t := range targets { + covered := false + // Walk up the chain; a cycle is impossible in a loadable model, but + // bound the walk anyway rather than trust that. + for p, hops := parentOf[t.Module+"."+t.Name], 0; p != "" && hops < 64; hops++ { + if inSet[p] { + covered = true + break + } + p = parentOf[p] + } + if !covered { + kept = append(kept, t) + } + } + return kept +} diff --git a/mdl/executor/cmd_entities_bulk_test.go b/mdl/executor/cmd_entities_bulk_test.go new file mode 100644 index 0000000000..1c1c6aa61b --- /dev/null +++ b/mdl/executor/cmd_entities_bulk_test.go @@ -0,0 +1,195 @@ +// 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/domainmodel" +) + +// bulkFixture: one user module with a persistent, a non-persistent and a view +// entity, plus a Marketplace module and System. +func bulkFixture() (mods []*model.Module, dms map[model.ID]*domainmodel.DomainModel) { + sales := mkModule("Sales") + admin := mkModule("Administration") + admin.FromAppStore = true + system := mkModule("System") + + order := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Order", Persistable: true} + draft := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Draft", Persistable: false} + report := &domainmodel.Entity{ + BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Report", + Persistable: true, Source: "OqlViewEntitySource", OqlQuery: "select 1", + } + vehicle := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Vehicle", Persistable: true} + truck := &domainmodel.Entity{ + BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Truck", + Persistable: true, GeneralizationRef: "Sales.Vehicle", + } + account := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "Account", Persistable: true} + user := &domainmodel.Entity{BaseElement: model.BaseElement{ID: nextID("ent")}, Name: "User", Persistable: true} + + return []*model.Module{sales, admin, system}, map[model.ID]*domainmodel.DomainModel{ + sales.ID: mkDomainModel(sales.ID, order, draft, report, vehicle, truck), + admin.ID: mkDomainModel(admin.ID, account), + system.ID: mkDomainModel(system.ID, user), + } +} + +func runBulk(t *testing.T, stmt *ast.AlterEntitiesStmt) (touched []string, out string) { + t.Helper() + mods, dms := bulkFixture() + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return mods, nil }, + GetDomainModelFunc: func(id model.ID) (*domainmodel.DomainModel, error) { return dms[id], nil }, + UpdateEntityFunc: func(dmID model.ID, e *domainmodel.Entity) error { + touched = append(touched, e.Name) + return nil + }, + } + ctx, buf := newMockCtx(t, withBackend(mb)) + if err := execAlterEntities(ctx, stmt); err != nil { + t.Fatalf("execAlterEntities: %v", err) + } + return touched, buf.String() +} + +func addAudit() []*ast.AlterEntityStmt { + return []*ast.AlterEntityStmt{{ + Operation: ast.AlterEntityAddAttribute, + Attribute: &ast.Attribute{Name: "Note", Type: ast.DataType{Kind: ast.TypeString, Length: 10}}, + IfNotExists: true, + }} +} + +// WHERE PERSISTENT must exclude the non-persistent entity AND the view entity: +// a view's rows come from an OQL query, so it is neither, and treating it as +// persistent would aim a write at a document that cannot take one. +func TestAlterEntities_PersistentFilterExcludesViewAndNonPersistent(t *testing.T) { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{ + Module: "Sales", Filter: ast.EntityFilterPersistent, Actions: addAudit(), + }) + // Vehicle is in; Truck is not, because it inherits from Vehicle. + want := map[string]bool{"Order": true, "Vehicle": true} + if len(touched) != len(want) { + t.Fatalf("touched %v, want %v — Draft is non-persistent, Report is a view, Truck inherits", touched, want) + } + for _, n := range touched { + if !want[n] { + t.Errorf("touched %q unexpectedly", n) + } + } +} + +func TestAlterEntities_NonPersistentFilter(t *testing.T) { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{ + Module: "Sales", Filter: ast.EntityFilterNonPersistent, Actions: addAudit(), + }) + if len(touched) != 1 || touched[0] != "Draft" { + t.Fatalf("touched %v, want only [Draft]", touched) + } +} + +// No WHERE: every entity in the named module, the view included — the filter is +// opt-in, and a statement that names no filter must not quietly apply one. +func TestAlterEntities_NoFilterTakesEveryEntityInTheModule(t *testing.T) { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{Module: "Sales", Actions: addAudit()}) + // Order, Draft, Vehicle. Report is a view (never a target) and Truck + // inherits from Vehicle. + if len(touched) != 3 { + t.Fatalf("touched %v, want 3 (Order, Draft, Vehicle)", touched) + } + for _, n := range touched { + if n == "Report" { + t.Errorf("a view entity must never be a target — CE6770") + } + if n == "Truck" { + t.Errorf("a specialization of a target must be skipped — CE0069") + } + } +} + +// An unscoped sweep must not edit System or a Marketplace module: an upgrade +// replaces the module and takes the attribute with it, so the write is undone +// later rather than refused now. +func TestAlterEntities_UnscopedSweepSkipsSystemAndMarketplace(t *testing.T) { + touched, out := runBulk(t, &ast.AlterEntitiesStmt{Filter: ast.EntityFilterPersistent, Actions: addAudit()}) + for _, name := range touched { + if name == "Account" || name == "User" { + t.Errorf("touched %q — a System/Marketplace entity must be skipped on an unscoped sweep", name) + } + } + if len(touched) != 2 { + t.Fatalf("touched %v, want 2 (Order, Vehicle)", touched) + } + if !strings.Contains(out, "Skipped 2 System/Marketplace module(s)") { + t.Errorf("the skip must be reported, not silent; output was:\n%s", out) + } +} + +// Naming the module explicitly is taken as meaning it — otherwise a deliberate +// edit to a Marketplace module would be impossible rather than merely guarded. +func TestAlterEntities_NamedMarketplaceModuleIsAllowed(t *testing.T) { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{Module: "Administration", Actions: addAudit()}) + if len(touched) != 1 || touched[0] != "Account" { + t.Fatalf("touched %v, want [Account] — an explicitly named module is not skipped", touched) + } +} + +func TestAlterEntities_UnknownModuleIsAnError(t *testing.T) { + mods, dms := bulkFixture() + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return mods, nil }, + GetDomainModelFunc: func(id model.ID) (*domainmodel.DomainModel, error) { return dms[id], nil }, + } + ctx, _ := newMockCtx(t, withBackend(mb)) + err := execAlterEntities(ctx, &ast.AlterEntitiesStmt{Module: "NoSuchModule", Actions: addAudit()}) + if err == nil { + t.Fatal("a misspelled module must be an error, not a silent no-op that reports 0 entities") + } + if !strings.Contains(err.Error(), "NoSuchModule") { + t.Errorf("error should name the module, got: %v", err) + } +} + +// A view entity is out of reach even when no filter is given: its columns come +// from its OQL select list, and an added attribute is CE6770. +func TestAlterEntities_ViewEntityIsNeverATarget(t *testing.T) { + for _, f := range []ast.EntityPersistenceFilter{ + ast.EntityFilterAll, ast.EntityFilterPersistent, ast.EntityFilterNonPersistent, + } { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{Module: "Sales", Filter: f, Actions: addAudit()}) + for _, n := range touched { + if n == "Report" { + t.Errorf("filter %v: touched the view entity Report", f) + } + } + } +} + +// Adding the same attribute to a generalization AND its specialization is +// CE0069 "Duplicate member name". The parent is kept, the child dropped -- the +// child inherits the member, so the author's intent is still satisfied. +func TestAlterEntities_SpecializationOfATargetIsSkipped(t *testing.T) { + touched, _ := runBulk(t, &ast.AlterEntitiesStmt{ + Module: "Sales", Filter: ast.EntityFilterPersistent, Actions: addAudit(), + }) + var sawVehicle, sawTruck bool + for _, n := range touched { + sawVehicle = sawVehicle || n == "Vehicle" + sawTruck = sawTruck || n == "Truck" + } + if !sawVehicle { + t.Error("the generalization must be touched") + } + if sawTruck { + t.Error("the specialization must be skipped — it inherits the member (CE0069)") + } +} diff --git a/mdl/executor/register_stubs.go b/mdl/executor/register_stubs.go index 8180c48475..3764c315ea 100644 --- a/mdl/executor/register_stubs.go +++ b/mdl/executor/register_stubs.go @@ -70,6 +70,9 @@ func registerEntityHandlers(r *Registry) { r.Register(&ast.AlterEntityStmt{}, func(ctx *ExecContext, stmt ast.Statement) error { return execAlterEntity(ctx, stmt.(*ast.AlterEntityStmt)) }) + r.Register(&ast.AlterEntitiesStmt{}, func(ctx *ExecContext, stmt ast.Statement) error { + return execAlterEntities(ctx, stmt.(*ast.AlterEntitiesStmt)) + }) r.Register(&ast.DropEntityStmt{}, func(ctx *ExecContext, stmt ast.Statement) error { return execDropEntity(ctx, stmt.(*ast.DropEntityStmt)) }) diff --git a/mdl/executor/registry_test.go b/mdl/executor/registry_test.go index f420785df0..2cea55c756 100644 --- a/mdl/executor/registry_test.go +++ b/mdl/executor/registry_test.go @@ -162,6 +162,7 @@ func allKnownStatements() []ast.Statement { &ast.AlterAssociationStmt{}, &ast.AlterConsumedMCPServiceStmt{}, &ast.AlterEntityStmt{}, + &ast.AlterEntitiesStmt{}, &ast.AlterMessageDefinitionCollectionStmt{}, &ast.AlterMessageDefinitionStmt{}, &ast.AlterEnumerationStmt{}, diff --git a/mdl/grammar/MDLParser.g4 b/mdl/grammar/MDLParser.g4 index 7d4caa818d..86a0e26d8e 100644 --- a/mdl/grammar/MDLParser.g4 +++ b/mdl/grammar/MDLParser.g4 @@ -139,6 +139,7 @@ createStatement alterStatement : ALTER ENTITY qualifiedName alterEntityAction (COMMA? alterEntityAction)* + | alterEntitiesStatement | ALTER ASSOCIATION qualifiedName alterAssociationAction+ | ALTER ENUMERATION qualifiedName alterEnumerationAction+ | ALTER ODATA CLIENT qualifiedName SET odataAlterAssignment (COMMA odataAlterAssignment)* diff --git a/mdl/grammar/domains/MDLDomainModel.g4 b/mdl/grammar/domains/MDLDomainModel.g4 index 4736682301..0253288d31 100644 --- a/mdl/grammar/domains/MDLDomainModel.g4 +++ b/mdl/grammar/domains/MDLDomainModel.g4 @@ -228,6 +228,34 @@ deleteBehavior // ALTER ENTITY / ASSOCIATION / ENUMERATION ACTIONS // ============================================================================= +// ALTER ENTITIES [IN ] ADD ATTRIBUTE ... [WHERE PERSISTENT|NON-PERSISTENT] +// +// The bulk form, and deliberately narrower than alterEntityAction: an audit +// trail is added to every entity in a module at once, which is the case that +// made the single-entity form cost one statement per entity. Only ADD +// ATTRIBUTE is offered, because it is the only action here that is safe to +// aim at a set -- DROP and RENAME in bulk are destructive by a typo, and SET +// POSITION on every entity is meaningless. Same reasoning as ALTER PAGES, +// which is bulk for exactly one operation. +// +// WHERE reuses the persistence words CREATE ENTITY already uses rather than +// inventing a predicate language: audit members belong on stored entities, +// and a non-persistent helper is the thing you want to skip. +alterEntitiesStatement + : ALTER ENTITIES (IN identifierOrKeyword)? + alterEntitiesAction (COMMA? alterEntitiesAction)* + (WHERE entityPersistenceFilter)? + ; + +alterEntitiesAction + : docComment? ADD ATTRIBUTE ifNotExists? attributeDefinition + ; + +entityPersistenceFilter + : PERSISTENT + | NON_PERSISTENT + ; + alterEntityAction : docComment? ADD ATTRIBUTE ifNotExists? attributeDefinition | docComment? ADD COLUMN ifNotExists? attributeDefinition diff --git a/mdl/visitor/visitor_alter_entities_test.go b/mdl/visitor/visitor_alter_entities_test.go new file mode 100644 index 0000000000..c2b734099b --- /dev/null +++ b/mdl/visitor/visitor_alter_entities_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +func TestBuildAlterEntities(t *testing.T) { + tests := []struct { + name string + src string + module string + filter ast.EntityPersistenceFilter + actions int + firstAttr string + ifNotExist bool + }{ + { + name: "module scoped, two actions, persistent filter", + src: "alter entities in Sales add attribute if not exists CreatedDate: AutoCreatedDate, add attribute if not exists ChangedDate: AutoChangedDate where persistent;", + module: "Sales", filter: ast.EntityFilterPersistent, actions: 2, + firstAttr: "CreatedDate", ifNotExist: true, + }, + { + name: "no module, no filter", + src: "alter entities add attribute Note: string(10);", + module: "", filter: ast.EntityFilterAll, actions: 1, firstAttr: "Note", + }, + { + name: "non-persistent filter", + src: "alter entities in Sales add attribute Scratch: string(5) where non-persistent;", + module: "Sales", filter: ast.EntityFilterNonPersistent, actions: 1, firstAttr: "Scratch", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prog, errs := Build(tt.src) + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + if len(prog.Statements) != 1 { + t.Fatalf("got %d statements, want 1", len(prog.Statements)) + } + s, ok := prog.Statements[0].(*ast.AlterEntitiesStmt) + if !ok { + t.Fatalf("got %T, want *ast.AlterEntitiesStmt", prog.Statements[0]) + } + if s.Module != tt.module { + t.Errorf("Module = %q, want %q", s.Module, tt.module) + } + if s.Filter != tt.filter { + t.Errorf("Filter = %v, want %v", s.Filter, tt.filter) + } + if len(s.Actions) != tt.actions { + t.Fatalf("got %d actions, want %d", len(s.Actions), tt.actions) + } + a := s.Actions[0] + if a.Attribute == nil || a.Attribute.Name != tt.firstAttr { + t.Errorf("first attribute = %v, want %q", a.Attribute, tt.firstAttr) + } + if a.IfNotExists != tt.ifNotExist { + t.Errorf("IfNotExists = %v, want %v", a.IfNotExists, tt.ifNotExist) + } + // The per-entity name is filled in by the executor, never by the + // visitor: a bulk action that carried a name would silently apply + // to that one entity instead of the set. + if a.Name.Name != "" || a.Name.Module != "" { + t.Errorf("action carries a name %v; the executor must fill it per entity", a.Name) + } + }) + } +} + +// The single-entity form must be unaffected by the new alternative. +func TestBuildAlterEntitySingularStillParses(t *testing.T) { + prog, errs := Build("alter entity Sales.Order add attribute if not exists Note: string(10);") + if len(errs) > 0 { + t.Fatalf("parse errors: %v", errs) + } + if _, ok := prog.Statements[0].(*ast.AlterEntityStmt); !ok { + t.Fatalf("got %T, want *ast.AlterEntityStmt", prog.Statements[0]) + } +} diff --git a/mdl/visitor/visitor_entity.go b/mdl/visitor/visitor_entity.go index 1b1858e257..6c152120e1 100644 --- a/mdl/visitor/visitor_entity.go +++ b/mdl/visitor/visitor_entity.go @@ -1139,3 +1139,53 @@ func (b *Builder) ExitCreateIndexStatement(ctx *parser.CreateIndexStatementConte Index: &ast.Index{Columns: buildIndexColumns(ctx.IndexAttributeList())}, }) } + +// ExitAlterEntitiesStatement handles the bulk form, +// ALTER ENTITIES [IN ] ADD ATTRIBUTE ... [WHERE PERSISTENT|NON-PERSISTENT]. +// +// Each action is built into an ordinary AlterEntityStmt with an EMPTY Name: the +// executor resolves the target set and fills the name in per entity, so a bulk +// add and a single add run the identical code path and cannot diverge. +func (b *Builder) ExitAlterEntitiesStatement(ctx *parser.AlterEntitiesStatementContext) { + stmt := &ast.AlterEntitiesStmt{} + + if mod := ctx.IdentifierOrKeyword(); mod != nil { + stmt.Module = unquoteIdentifier(mod.GetText()) + } + + if f := ctx.EntityPersistenceFilter(); f != nil { + fc := f.(*parser.EntityPersistenceFilterContext) + switch { + case fc.NON_PERSISTENT() != nil: + stmt.Filter = ast.EntityFilterNonPersistent + case fc.PERSISTENT() != nil: + stmt.Filter = ast.EntityFilterPersistent + } + } + + for _, act := range ctx.AllAlterEntitiesAction() { + ac := act.(*parser.AlterEntitiesActionContext) + attrDef := ac.AttributeDefinition() + if attrDef == nil { + continue + } + attr := buildSingleAttribute(attrDef.(*parser.AttributeDefinitionContext)) + if attr == nil { + continue + } + if attr.Documentation == "" { + if docCtx := ac.DocComment(); docCtx != nil { + attr.Documentation = extractDocComment(docCtx.GetText()) + } + } + stmt.Actions = append(stmt.Actions, &ast.AlterEntityStmt{ + Operation: ast.AlterEntityAddAttribute, + Attribute: attr, + IfNotExists: ac.IfNotExists() != nil, + }) + } + + if len(stmt.Actions) > 0 { + b.statements = append(b.statements, stmt) + } +}