diff --git a/cmd/mxcli/syntax/features_domain_model.go b/cmd/mxcli/syntax/features_domain_model.go index de35eeb79..b3175a736 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 25eb48a80..cecc992f2 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 9fa73c651..dc1079195 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 321125eed..2e2ef405e 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 000000000..8605a6946 --- /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 000000000..1c1c6aa61 --- /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 8180c4847..3764c315e 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 f420785df..2cea55c75 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 7d4caa818..86a0e26d8 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 473668230..0253288d3 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 000000000..c2b734099 --- /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 1b1858e25..6c152120e 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) + } +}