From c7f89f5e0608e7e54c5b4b867faf205e64a607f7 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 19:27:24 +0000 Subject: [PATCH 1/5] fix(domainmodel): UpdateAttribute carries the stored attribute GUID (#627) The fluent API's AttributeModifier.Apply() rebuilt the attribute with raw == nil and kept only its $ID, so the codec minted GUID = $ID - the #1119 data-loss class. The write guard refused it, leaving the API unusable on any Studio Pro-authored attribute. Carry the stored raw bytes and export level via carryStoredAttribute, now shared with the entity rewrite's carryAttributeIdentity. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-backend.jsonl | 1 + mdl/backend/modelsdk/domainmodel_alter.go | 8 + .../modelsdk/domainmodel_child_identity.go | 29 ++-- .../issue627_update_attribute_guid_test.go | 138 ++++++++++++++++++ 4 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 mdl/backend/modelsdk/issue627_update_attribute_guid_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index b42032884..bb7f53251 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -149,3 +149,4 @@ {"area": "mdl/backend", "date": "2026-09-29", "symptom": "describe -> exec, CREATE OR MODIFY or an ALTER that rebuilds the document turns an API-exported document Hidden: enumerations, pages, layouts, rules, view-entity OQL source documents, import/export mappings, JSON structures, published and consumed REST services, scheduled events, workflows, database connections, business event services, data transformers, queues, regular expressions and agent-editor documents. The run reports success, mx check is clean; the module's public surface silently shrinks. A workflow's own `export level API` clause was a no-op on create and on rewrite.", "cause": "Each rewrite converter builds a fresh document and writes ExportLevel as a constant (\"Hidden\"), or passes the semantic model's value where the executor itself filled in \"Hidden\" (mappings, database connection, business events), and the unit is replaced wholesale. MDL has no export-level spelling for most of these kinds, so describe cannot print it and the executed script cannot restore it. workflowToGen ignored wf.ExportLevel entirely. The round-trip harness could not see it: every document in TestApp and PedApp is Hidden, the constant itself.", "file": "mdl/backend/modelsdk/export_level_carry.go", "fix": "One byte-level carry, keepStoredExportLevel(unitID, contents): replaces only the top-level ExportLevel element of the freshly encoded rewrite with the stored value, copying every other element verbatim, and never adds the key. Wired into every Update path that writes ExportLevel (UpdateEnumeration/Rule/Layout/ImportMapping/ExportMapping/JsonStructure/PublishedRestService/ConsumedRestService/DataTransformer/DatabaseConnection/BusinessEventService, writeCustomBlob update, WriteViewEntitySourceDocument update; page via carryStoredPageHeader). Kinds with an MDL spelling (workflow, scheduled event, queue, regular expression) use keepStoredExportLevelUnlessSet: an authored level wins. workflowToGen now writes orDefault(wf.ExportLevel, \"Hidden\").", "insight": "A fixture-driven round trip is blind to any constant that happens to equal every fixture value: 775 TestApp documents round-tripped while 10 kinds hid API documents. Set the subject to the non-default value first (here: patch ExportLevel to API on the working copy) and run both the plain describe output and an edited one, because an elided unchanged write passes a converter that still writes the constant. Carrying at the encoded-bytes level covers gen-typed, newElem-built and hand-serialized writers with one helper, where a gen setter per converter would have needed three mechanisms.", "refs": ["ako/mxcli#816", "ako/mxcli#801", "ako/mxcli#812"], "test": "mdl/backend/modelsdk/issue816_export_level_test.go (TestUpdatePaths_KeepStoredExportLevel, 18 kinds); mdl/roundtrip/export_level_test.go (TestTestAppExportLevelSurvivesRoundTrip, -tags integration)"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#859 review of PR #864: after the built comparison landed, changing or adding `show page M.P with title = 'X'` in a `create or modify microflow` reported \"Unchanged microflow\" and wrote nothing, under mdl 0 and mdl 1 (main spliced it). Nothing warned.", "cause": "builtAsStored compares the declared flow and the stored flow both READ BACK through the codec, so any property the reader drops compares equal whatever either side holds. The ShowFormAction reader never read FormSettings.TitleOverride. Probing encode(built) against encode(readback(built)) over mdl-examples found the reader also dropped ExclusiveSplit/LoopedActivity ErrorHandlingType and a REST call's bound output variable (ResultHandling.ResultVariableName -> RestCallAction.OutputVariable), plus CallWebServiceAction (#861). Before the built comparison such a loss was a visible phantom re-splice; after it, a silently dropped edit.", "fix": "ReadBackMicroflow/ReadBackNanoflow re-encode what they read back and refuse (error -> statement diff, the pre-#859 path) when it is not the document first written, $IDs aside (sameWritten). The reader now reads TitleOverride, the split's and loop's ErrorHandlingType, and a bound REST call's OutputVariable, so those flows keep matching.", "insight": "A comparison made on both sides through the same lossy reader cannot see what the reader loses; the lost property becomes a change that is never written. When equality is decided after a decode, prove the decode lossless for the value at hand (write it again and compare bytes) and fall back when it is not. The probe that found the fields: diff encode(x) with encode(decode(encode(x))) over every mdl-examples flow.", "issue": "ako/mxcli#859", "file": "mdl/backend/modelsdk/microflow_readback.go, mdl/backend/modelsdk/microflow_read_actions.go, mdl/backend/modelsdk/microflow.go, mdl/roundtrip/flow_idempotent_shapes_test.go"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#843 (rehearsal M2): under mdl 1, `create or modify nanoflow … returns Boolean as $Done` over a nanoflow stored without a return variable refuses \"the stored document has no ReturnVariableName property … set it in Studio Pro\"; the same statement on a microflow reports \"set: ReturnVariableName\".", "cause": "mfmutator.SetHeader refuses any stated header key the stored document lacks (a key the project version does not declare makes the document unopenable). mxcli's nanoflow writer omits ReturnVariableName when the statement has no `as $Var`, while the microflow writer always writes it on 10+, so only nanoflows hit the refusal.", "fix": "Optional mfmutator.PropertyDeclarer on Deps; the codec deps answer from the metamodel version data (type, then Microflows$MicroflowBase; ReturnVariableName is 10.12+) against the project version, and SetHeader inserts the key after its predecessor in the encoder's order. No answer (MCP, unknown version) keeps the refusal.", "insight": "A refusal keyed on 'the stored document lacks the key' conflates 'this version has no such property' with 'the writer left it out'; the metamodel version data separates the two. Studio Pro 11 stores ReturnVariableName on every nanoflow (PedApp: 13 of 13), so adding it matches what Studio Pro writes.", "issue": "ako/mxcli#843", "file": "mdl/backend/mfmutator/header.go"} +{"date": "2026-10-01", "area": "mdl/backend", "symptom": "ako/mxcli#627: the fluent API's AttributeModifier.Apply() (Backend.UpdateAttribute) on a Studio Pro-authored attribute is refused with \"refusing to write unit …: 1 element(s) kept their $ID but would be written with a different GUID (DomainModels$Attribute)\"; without the #1119 guard it would re-mint the GUID and drop the column on the next deploy.", "cause": "UpdateAttribute rebuilds the attribute with attributeToGen (raw == nil) and carried only the $ID, so the codec's EmitGUID default wrote GUID = $ID. No MDL statement reaches it — every ALTER ENTITY form goes through UpdateEntity, which has the carry — so it was found by enumerating the converter's call sites, not by a repro.", "fix": "UpdateAttribute carries the stored attribute's raw bytes and export level onto the rebuild via carryStoredAttribute, the helper now shared with carryAttributeIdentity. Attribute -> Attribute keeps $Type, so SetRaw suffices.", "insight": "Test on PedApp (GUID != $ID); an mxcli-created attribute re-mints the same value and cannot fail. The unfixed code fails the test via the write guard's refusal, which is the observable symptom on main.", "issue": "ako/mxcli#627", "file": "mdl/backend/modelsdk/domainmodel_alter.go"} diff --git a/mdl/backend/modelsdk/domainmodel_alter.go b/mdl/backend/modelsdk/domainmodel_alter.go index ccd3abf86..bcc65de6e 100644 --- a/mdl/backend/modelsdk/domainmodel_alter.go +++ b/mdl/backend/modelsdk/domainmodel_alter.go @@ -774,6 +774,14 @@ func (b *Backend) UpdateAttribute(domainModelID, entityID model.ID, attr *domain next := attributeToGen(attr, entityIsExternal(ent)) next.SetID(items[idx].ID()) + // attributeToGen returns raw == nil, which the codec reads as a NEW element + // and mints GUID = $ID for — the runtime keys mendixsystem$attribute.id on + // that GUID and would drop the column (ako/mxcli#627, the #1119 class). + // Attribute -> Attribute keeps the $Type, so carrying the stored raw bytes + // is enough; no raw transform is needed. + if stored, ok := items[idx].(*genDm.Attribute); ok { + carryStoredAttribute(next, stored) + } // The generated list offers only Append and Remove, so an in-place replace // means rebuilding it. Order is worth the rebuild: it is the order Studio diff --git a/mdl/backend/modelsdk/domainmodel_child_identity.go b/mdl/backend/modelsdk/domainmodel_child_identity.go index b07c04bb9..ca0d91a38 100644 --- a/mdl/backend/modelsdk/domainmodel_child_identity.go +++ b/mdl/backend/modelsdk/domainmodel_child_identity.go @@ -82,15 +82,7 @@ func carryAttributeIdentity(ge, orig *genDm.Entity, entity *domainmodel.Entity) claimed := make(map[string]bool, len(storedByID)) carry := func(ga, sa *genDm.Attribute) { - ga.SetID(sa.ID()) - ga.SetRaw(sa.Raw()) - // attributeToGen sets ExportLevel "Hidden", and a property the rebuild - // sets wins over the carried raw bytes — so without this an API attribute - // became Hidden on every rewrite of its entity (ako/mxcli#801). The - // semantic attribute has no export level to take it from. - if lvl := sa.ExportLevel(); lvl != "" { - ga.SetExportLevel(lvl) - } + carryStoredAttribute(ga, sa) claimed[string(sa.ID())] = true } @@ -170,3 +162,22 @@ func carryIndexIdentity(ge, orig *genDm.Entity, entity *domainmodel.Entity) { claimed[string(si.ID())] = true } } + +// carryStoredAttribute makes the rebuilt attribute ga the stored attribute sa as +// far as identity goes: the stored $ID and raw bytes (so the GUID passes through +// instead of being re-minted as $ID — #1119, ako/mxcli#627) and the stored export +// level. Both rebuild sites use it: the entity rewrite (carryAttributeIdentity) +// and the single-attribute rewrite (Backend.UpdateAttribute). +func carryStoredAttribute(ga, sa *genDm.Attribute) { + ga.SetID(sa.ID()) + if raw := sa.Raw(); raw != nil { + ga.SetRaw(raw) + } + // attributeToGen sets ExportLevel "Hidden", and a property the rebuild + // sets wins over the carried raw bytes — so without this an API attribute + // became Hidden on every rewrite of its entity (ako/mxcli#801). The + // semantic attribute has no export level to take it from. + if lvl := sa.ExportLevel(); lvl != "" { + ga.SetExportLevel(lvl) + } +} diff --git a/mdl/backend/modelsdk/issue627_update_attribute_guid_test.go b/mdl/backend/modelsdk/issue627_update_attribute_guid_test.go new file mode 100644 index 000000000..f73ef0c0b --- /dev/null +++ b/mdl/backend/modelsdk/issue627_update_attribute_guid_test.go @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "os" + "path/filepath" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// copyPedApp copies the Studio Pro-authored PedApp fixture into a temp dir. Its +// elements have GUID != $ID, which is the only subject on which a re-minted +// storage GUID is observable (CLAUDE.md, "A GUID Is the Database's Identity"). +func copyPedApp(t *testing.T) string { + t.Helper() + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../../testdata/pedapp")); err != nil { + t.Fatalf("copy PedApp fixture: %v", err) + } + return filepath.Join(dst, "PedApp.mpr") +} + +// TestIssue627_UpdateAttributePreservesGUID guards ako/mxcli#627: the +// AttributeModifier path of the fluent API (Backend.UpdateAttribute) rebuilt the +// attribute from the semantic model with raw == nil, so the codec minted +// GUID = $ID — the #1119 data-loss class, the column dropped on the next deploy. +// The #1119 write guard refused it, which left the API unusable on any Studio +// Pro-authored attribute. +// +// The subject is a PedApp string attribute whose stored GUID differs from its +// $ID; the comparison is against the GUID read before the write, never against a +// previous run (a re-mint is stable, so a second write is elided). +func TestIssue627_UpdateAttributePreservesGUID(t *testing.T) { + proj := copyPedApp(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + + dmID, ent, attr := studioProStringAttribute(t, b) + before := attributeGUIDs(t, b, dmID, ent.ID) + ids := attributeIDs(t, b, dmID, ent.ID) + want := before[attr.Name] + if want == "" || want == ids[attr.Name] { + t.Fatalf("subject %s.%s: GUID %q, $ID %q — need a stored GUID that differs from $ID", + ent.Name, attr.Name, want, ids[attr.Name]) + } + + st := attr.Type.(*domainmodel.StringAttributeType) + st.Length += 7 + if err := b.UpdateAttribute(dmID, ent.ID, attr); err != nil { + t.Fatalf("UpdateAttribute: %v", err) + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + + b2 := New() + if err := b2.Connect(proj); err != nil { + t.Fatalf("reconnect: %v", err) + } + t.Cleanup(func() { _ = b2.Disconnect() }) + + after := attributeGUIDs(t, b2, dmID, ent.ID) + if got := after[attr.Name]; got != want { + t.Errorf("attribute %s.%s: storage GUID changed %s -> %s", ent.Name, attr.Name, want, got) + } + // The siblings pass through as stored bytes; assert it so a future rebuild + // of the whole list is caught here too. + for name, g := range before { + if after[name] != g { + t.Errorf("sibling attribute %s: storage GUID changed %s -> %s", name, g, after[name]) + } + } + // And the edit itself landed — otherwise "GUID unchanged" proves nothing. + dm, err := b2.GetDomainModel(moduleOfDM(t, b2, dmID)) + if err != nil { + t.Fatalf("GetDomainModel: %v", err) + } + var gotLen int + for _, e := range dm.Entities { + if e.ID != ent.ID { + continue + } + for _, a := range e.Attributes { + if a.Name == attr.Name { + if s, ok := a.Type.(*domainmodel.StringAttributeType); ok { + gotLen = s.Length + } + } + } + } + if gotLen != st.Length { + t.Errorf("attribute %s.%s: length = %d after UpdateAttribute, want %d", ent.Name, attr.Name, gotLen, st.Length) + } +} + +// studioProStringAttribute picks the first string attribute with a non-zero +// length in a loadable, non-System domain model. +func studioProStringAttribute(t *testing.T, b *Backend) (model.ID, *domainmodel.Entity, *domainmodel.Attribute) { + t.Helper() + dms, err := b.ListDomainModels() + if err != nil { + t.Fatalf("ListDomainModels: %v", err) + } + for _, d := range dms { + if _, err := b.loadDomainModelGen(d.ID); err != nil { + continue + } + for _, e := range d.Entities { + for _, a := range e.Attributes { + if s, ok := a.Type.(*domainmodel.StringAttributeType); ok && s.Length > 0 { + return d.ID, e, a + } + } + } + } + t.Fatal("no string attribute in a loadable PedApp domain model") + return "", nil, nil +} + +func moduleOfDM(t *testing.T, b *Backend, dmID model.ID) model.ID { + t.Helper() + dms, err := b.ListDomainModels() + if err != nil { + t.Fatalf("ListDomainModels: %v", err) + } + for _, d := range dms { + if d.ID == dmID { + return d.ContainerID + } + } + t.Fatalf("domain model %s not found", dmID) + return "" +} From 4bca1f05702a49eb180280d8519bb70ab77ae4ac Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 20:23:21 +0000 Subject: [PATCH 2/5] fix(move): MOVE ENTITY handles existing cross-associations (#628) MoveEntity scanned only the plain associations of the source unit, so a cross-association created by an earlier move was invisible: moving its second endpoint left a ParentPointer naming an element absent from its unit and Studio Pro could not open the project. Handle the three shapes: convert back to a plain association when both endpoints share a module (raw transform, GUID carried), let an own cross-association travel with its FROM entity, and re-point a cross-association in another module. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-backend.jsonl | 1 + ...ove-entity-503-preserves-storage-guids.mdl | 6 +- ...entity-628-existing-cross-associations.mdl | 58 ++++ .../modelsdk/association_move_cross.go | 242 ++++++++++++++ .../modelsdk/association_move_write.go | 20 +- .../issue628_move_cross_assoc_test.go | 305 ++++++++++++++++++ mdl/executor/cmd_move.go | 22 +- mdl/types/entity_move.go | 4 + 8 files changed, 649 insertions(+), 9 deletions(-) create mode 100644 mdl-examples/bug-tests/move-entity-628-existing-cross-associations.mdl create mode 100644 mdl/backend/modelsdk/association_move_cross.go create mode 100644 mdl/backend/modelsdk/issue628_move_cross_assoc_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index b42032884..62dc4c89c 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -149,3 +149,4 @@ {"area": "mdl/backend", "date": "2026-09-29", "symptom": "describe -> exec, CREATE OR MODIFY or an ALTER that rebuilds the document turns an API-exported document Hidden: enumerations, pages, layouts, rules, view-entity OQL source documents, import/export mappings, JSON structures, published and consumed REST services, scheduled events, workflows, database connections, business event services, data transformers, queues, regular expressions and agent-editor documents. The run reports success, mx check is clean; the module's public surface silently shrinks. A workflow's own `export level API` clause was a no-op on create and on rewrite.", "cause": "Each rewrite converter builds a fresh document and writes ExportLevel as a constant (\"Hidden\"), or passes the semantic model's value where the executor itself filled in \"Hidden\" (mappings, database connection, business events), and the unit is replaced wholesale. MDL has no export-level spelling for most of these kinds, so describe cannot print it and the executed script cannot restore it. workflowToGen ignored wf.ExportLevel entirely. The round-trip harness could not see it: every document in TestApp and PedApp is Hidden, the constant itself.", "file": "mdl/backend/modelsdk/export_level_carry.go", "fix": "One byte-level carry, keepStoredExportLevel(unitID, contents): replaces only the top-level ExportLevel element of the freshly encoded rewrite with the stored value, copying every other element verbatim, and never adds the key. Wired into every Update path that writes ExportLevel (UpdateEnumeration/Rule/Layout/ImportMapping/ExportMapping/JsonStructure/PublishedRestService/ConsumedRestService/DataTransformer/DatabaseConnection/BusinessEventService, writeCustomBlob update, WriteViewEntitySourceDocument update; page via carryStoredPageHeader). Kinds with an MDL spelling (workflow, scheduled event, queue, regular expression) use keepStoredExportLevelUnlessSet: an authored level wins. workflowToGen now writes orDefault(wf.ExportLevel, \"Hidden\").", "insight": "A fixture-driven round trip is blind to any constant that happens to equal every fixture value: 775 TestApp documents round-tripped while 10 kinds hid API documents. Set the subject to the non-default value first (here: patch ExportLevel to API on the working copy) and run both the plain describe output and an edited one, because an elided unchanged write passes a converter that still writes the constant. Carrying at the encoded-bytes level covers gen-typed, newElem-built and hand-serialized writers with one helper, where a gen setter per converter would have needed three mechanisms.", "refs": ["ako/mxcli#816", "ako/mxcli#801", "ako/mxcli#812"], "test": "mdl/backend/modelsdk/issue816_export_level_test.go (TestUpdatePaths_KeepStoredExportLevel, 18 kinds); mdl/roundtrip/export_level_test.go (TestTestAppExportLevelSurvivesRoundTrip, -tags integration)"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#859 review of PR #864: after the built comparison landed, changing or adding `show page M.P with title = 'X'` in a `create or modify microflow` reported \"Unchanged microflow\" and wrote nothing, under mdl 0 and mdl 1 (main spliced it). Nothing warned.", "cause": "builtAsStored compares the declared flow and the stored flow both READ BACK through the codec, so any property the reader drops compares equal whatever either side holds. The ShowFormAction reader never read FormSettings.TitleOverride. Probing encode(built) against encode(readback(built)) over mdl-examples found the reader also dropped ExclusiveSplit/LoopedActivity ErrorHandlingType and a REST call's bound output variable (ResultHandling.ResultVariableName -> RestCallAction.OutputVariable), plus CallWebServiceAction (#861). Before the built comparison such a loss was a visible phantom re-splice; after it, a silently dropped edit.", "fix": "ReadBackMicroflow/ReadBackNanoflow re-encode what they read back and refuse (error -> statement diff, the pre-#859 path) when it is not the document first written, $IDs aside (sameWritten). The reader now reads TitleOverride, the split's and loop's ErrorHandlingType, and a bound REST call's OutputVariable, so those flows keep matching.", "insight": "A comparison made on both sides through the same lossy reader cannot see what the reader loses; the lost property becomes a change that is never written. When equality is decided after a decode, prove the decode lossless for the value at hand (write it again and compare bytes) and fall back when it is not. The probe that found the fields: diff encode(x) with encode(decode(encode(x))) over every mdl-examples flow.", "issue": "ako/mxcli#859", "file": "mdl/backend/modelsdk/microflow_readback.go, mdl/backend/modelsdk/microflow_read_actions.go, mdl/backend/modelsdk/microflow.go, mdl/roundtrip/flow_idempotent_shapes_test.go"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#843 (rehearsal M2): under mdl 1, `create or modify nanoflow … returns Boolean as $Done` over a nanoflow stored without a return variable refuses \"the stored document has no ReturnVariableName property … set it in Studio Pro\"; the same statement on a microflow reports \"set: ReturnVariableName\".", "cause": "mfmutator.SetHeader refuses any stated header key the stored document lacks (a key the project version does not declare makes the document unopenable). mxcli's nanoflow writer omits ReturnVariableName when the statement has no `as $Var`, while the microflow writer always writes it on 10+, so only nanoflows hit the refusal.", "fix": "Optional mfmutator.PropertyDeclarer on Deps; the codec deps answer from the metamodel version data (type, then Microflows$MicroflowBase; ReturnVariableName is 10.12+) against the project version, and SetHeader inserts the key after its predecessor in the encoder's order. No answer (MCP, unknown version) keeps the refusal.", "insight": "A refusal keyed on 'the stored document lacks the key' conflates 'this version has no such property' with 'the writer left it out'; the metamodel version data separates the two. Studio Pro 11 stores ReturnVariableName on every nanoflow (PedApp: 13 of 13), so adding it matches what Studio Pro writes.", "issue": "ako/mxcli#843", "file": "mdl/backend/mfmutator/header.go"} +{"date": "2026-10-01", "area": "mdl/backend", "symptom": "ako/mxcli#628: `move entity A.Parent to B; move entity A.Child to B;` both report success, then Studio Pro cannot open the project: System.AggregateException: The given key '' was not present in the dictionary.", "cause": "MoveEntity scanned only sourceDM.AssociationsItems(); an existing cross-association (created by the first move) was invisible, so moving its FROM entity left its ParentPointer naming an element absent from the unit, and moving its TO entity left Child naming the old module. Cross-unit damage, so the #1119 write guard cannot see it.", "fix": "association_move_cross.go: own cross-associations (FROM = moved entity) travel to the target, or become a plain DomainModels$Association there when the TO entity is already in it (raw transform: $Type, Child -> ChildPointer binary id, connection points added, GUID passed through); a cross-association in the target naming the entity becomes plain; any other module's cross-association naming it is re-pointed. MovedAssociation.SameModule lets the executor report the plain conversions apart.", "insight": "Assert on the stored documents (every ParentPointer/ChildPointer resolves in its own unit, every cross Child resolves to an entity in the named module); mx check is a weak signal for this class. The four sequences (to-then-from, from-then-to, travel, re-point) are distinct shapes, not one.", "issue": "ako/mxcli#628", "file": "mdl/backend/modelsdk/association_move_cross.go"} diff --git a/mdl-examples/bug-tests/move-entity-503-preserves-storage-guids.mdl b/mdl-examples/bug-tests/move-entity-503-preserves-storage-guids.mdl index 898dec6bb..fd072be98 100644 --- a/mdl-examples/bug-tests/move-entity-503-preserves-storage-guids.mdl +++ b/mdl-examples/bug-tests/move-entity-503-preserves-storage-guids.mdl @@ -69,9 +69,9 @@ CREATE MODULE BugMoveGuid503B; -- -- Measured with a binary predating both fixes here, so it is a third defect on this -- command and not something these fixes introduced — but it is why this file keeps --- one endpoint of each pair put. Filed as ako/mxcli#628: MoveEntity scans --- AssociationsItems() and never CrossAssociationsItems(), so a cross-association --- that already exists is invisible to the move. +-- one endpoint of each pair put. Filed and fixed as ako/mxcli#628 (MoveEntity +-- never looked at CrossAssociationsItems()); the both-endpoints sequences are +-- exercised in move-entity-628-existing-cross-associations.mdl. -- Pair 1 — the TO side moves, so the cross-association STAYS in the source module -- and its qualified name does not change. diff --git a/mdl-examples/bug-tests/move-entity-628-existing-cross-associations.mdl b/mdl-examples/bug-tests/move-entity-628-existing-cross-associations.mdl new file mode 100644 index 000000000..c25d1eacb --- /dev/null +++ b/mdl-examples/bug-tests/move-entity-628-existing-cross-associations.mdl @@ -0,0 +1,58 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#628 — MOVE ENTITY must see the cross-associations that already exist +-- ============================================================================ +-- +-- MoveEntity converted the plain associations of the moved entity and never +-- looked at existing cross-associations. Moving the SECOND endpoint of an +-- association to another module — the normal way to reorganise a module one +-- entity at a time — left a cross-association whose ParentPointer named an +-- element absent from its unit, and Studio Pro could no longer open the project: +-- +-- System.AggregateException: The given key '' was not present in the dictionary. +-- +-- Three shapes, each below: +-- 1. both endpoints end up in one module -> a plain association again; +-- 2. the FROM end moves on -> the cross-association travels with it; +-- 3. the TO end moves on -> the cross-association elsewhere is re-pointed. +-- +-- mxcli exec move-entity-628-existing-cross-associations.mdl -p app.mpr +-- mxcli docker check -p app.mpr +-- +-- Expected: every statement succeeds and the project checks clean. Afterwards +-- describe association Bug628B.Child_Parent; -> from Bug628B.Child to Bug628B.Parent +-- describe association Bug628E.C2_P2; -> from Bug628E.C2 to Bug628D.P2 +-- describe association Bug628C.C3_P3; -> from Bug628C.C3 to Bug628E.P3 +-- +-- The storage GUID carry through each conversion is asserted on Studio +-- Pro-authored content in mdl/backend/modelsdk/issue628_move_cross_assoc_test.go; +-- in a script like this one every element is mxcli-created (GUID == $ID), so a +-- lost GUID is not observable here. +-- ============================================================================ + +create module Bug628A; +create module Bug628B; +create module Bug628C; +create module Bug628D; +create module Bug628E; + +-- Shape 1 (the issue's reproduction). +create or modify persistent entity Bug628A.Parent ( Code: String(50) ); +create or modify persistent entity Bug628A.Child ( Descr: String(100) ); +create or modify association Bug628A.Child_Parent from Bug628A.Child to Bug628A.Parent type Reference; +move entity Bug628A.Parent to Bug628B; +move entity Bug628A.Child to Bug628B; + +-- Shape 2. +create or modify persistent entity Bug628C.P2 ( Code: String(50) ); +create or modify persistent entity Bug628C.C2 ( Descr: String(100) ); +create or modify association Bug628C.C2_P2 from Bug628C.C2 to Bug628C.P2 type Reference; +move entity Bug628C.P2 to Bug628D; +move entity Bug628C.C2 to Bug628E; + +-- Shape 3. +create or modify persistent entity Bug628C.P3 ( Code: String(50) ); +create or modify persistent entity Bug628C.C3 ( Descr: String(100) ); +create or modify association Bug628C.C3_P3 from Bug628C.C3 to Bug628C.P3 type Reference; +move entity Bug628C.P3 to Bug628D; +move entity Bug628D.P3 to Bug628E; diff --git a/mdl/backend/modelsdk/association_move_cross.go b/mdl/backend/modelsdk/association_move_cross.go new file mode 100644 index 000000000..eec264a5c --- /dev/null +++ b/mdl/backend/modelsdk/association_move_cross.go @@ -0,0 +1,242 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "fmt" + "strings" + + "go.mongodb.org/mongo-driver/v2/bson" + "go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore" + + "github.com/mendixlabs/mxcli/mdl/types" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/modelsdk/element" + genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" + "github.com/mendixlabs/mxcli/modelsdk/meta" + mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" + "github.com/mendixlabs/mxcli/sdk/domainmodel" +) + +// A MOVE ENTITY has to look at the cross-associations that already exist, not +// only at the plain associations it converts (ako/mxcli#628). A cross-association +// lives in the unit of its FROM entity, holds that entity by id (ParentPointer) +// and the TO entity by qualified name (Child). Moving entity E from S to T leaves +// three shapes that are not the same case: +// +// 1. Both endpoints end up in one module. The cross-association becomes a plain +// DomainModels$Association again in that module — the inverse of +// crossAssocRawFromAssoc. This is what moving the second endpoint of an +// association across, one entity at a time, produces. +// 2. E is the FROM entity and the TO entity is elsewhere. The cross-association +// travels with E to T (its ParentPointer would not resolve in S), so its +// qualified name changes S.X -> T.X. +// 3. E is the TO entity of a cross-association in ANOTHER module. That module's +// unit names `S.E`; it is re-pointed to `T.E`. +// +// Left alone, shape 1 and 2 leave a ParentPointer naming an element absent from +// its unit — Studio Pro fails to load the project ("The given key was not present +// in the dictionary") — and shape 3 leaves a dangling qualified name. Every +// conversion carries the stored document, GUID included: the runtime keys the +// association's data on that GUID (CLAUDE.md, "A GUID Is the Database's Identity"). + +// moveOwnCrossAssociations handles shapes 1 and 2: the cross-associations in the +// source unit whose FROM entity is the moved one. It removes them from sourceDM +// and adds them to targetDM, as plain associations when their TO entity lives in +// the target module. +func moveOwnCrossAssociations(sourceDM, targetDM *genDm.DomainModel, entityID model.ID, sourceModuleName, targetModuleName string) []types.MovedAssociation { + targetEntities := entityIDsByName(targetDM) + var out []types.MovedAssociation + var removeIdx []int + for i, el := range sourceDM.CrossAssociationsItems() { + ca, ok := el.(*genDm.CrossAssociation) + if !ok || string(ca.ParentRefID()) != string(entityID) { + continue + } + moved := types.MovedAssociation{ + Name: ca.Name(), + OldQualifiedName: sourceModuleName + "." + ca.Name(), + NewQualifiedName: targetModuleName + "." + ca.Name(), + } + mod, ent, _ := strings.Cut(ca.ChildQualifiedName(), ".") + if toID, ok := targetEntities[ent]; ok && mod == targetModuleName { + targetDM.AddAssociations(assocFromGenCrossAssoc(ca, toID)) + moved.SameModule = true + } else { + targetDM.AddCrossAssociations(cloneCrossAssoc(ca)) + } + removeIdx = append(removeIdx, i) + out = append(out, moved) + } + for i := len(removeIdx) - 1; i >= 0; i-- { + sourceDM.RemoveCrossAssociations(removeIdx[i]) + } + return out +} + +// convertIncomingCrossAssociations handles shape 1 seen from the other side: a +// cross-association in the TARGET unit whose TO entity is the moved one. Both +// endpoints are now in the target module, so it becomes a plain association +// there. Its qualified name does not change. +func convertIncomingCrossAssociations(targetDM *genDm.DomainModel, entityID model.ID, oldEntityQN, targetModuleName string) []types.MovedAssociation { + var out []types.MovedAssociation + var removeIdx []int + for i, el := range targetDM.CrossAssociationsItems() { + ca, ok := el.(*genDm.CrossAssociation) + if !ok || ca.ChildQualifiedName() != oldEntityQN { + continue + } + targetDM.AddAssociations(assocFromGenCrossAssoc(ca, string(entityID))) + removeIdx = append(removeIdx, i) + qn := targetModuleName + "." + ca.Name() + out = append(out, types.MovedAssociation{Name: ca.Name(), OldQualifiedName: qn, NewQualifiedName: qn, SameModule: true}) + } + for i := len(removeIdx) - 1; i >= 0; i-- { + targetDM.RemoveCrossAssociations(removeIdx[i]) + } + return out +} + +// repointCrossAssociationsElsewhere handles shape 3: every cross-association in a +// domain model other than the source and target units that names the moved entity +// as its TO entity is re-pointed to the entity's new qualified name. +func (b *Backend) repointCrossAssociationsElsewhere(sourceDMID, targetDMID model.ID, oldEntityQN, newEntityQN string) error { + dms, err := b.ListDomainModels() + if err != nil { + return fmt.Errorf("list domain models: %w", err) + } + for _, info := range dms { + if info.ID == sourceDMID || info.ID == targetDMID || string(info.ID) == meta.SystemDomainModelID { + continue + } + gdm, err := b.loadDomainModelGen(info.ID) + if err != nil { + return err + } + changed := false + for _, el := range gdm.CrossAssociationsItems() { + if ca, ok := el.(*genDm.CrossAssociation); ok && ca.ChildQualifiedName() == oldEntityQN { + ca.SetChildQualifiedName(newEntityQN) + changed = true + } + } + if changed { + if err := b.persistDM(info.ID, gdm); err != nil { + return err + } + } + } + return nil +} + +func entityIDsByName(dm *genDm.DomainModel) map[string]string { + out := map[string]string{} + for _, el := range dm.EntitiesItems() { + if e, ok := el.(*genDm.Entity); ok { + out[e.Name()] = string(e.ID()) + } + } + return out +} + +// cloneCrossAssoc re-homes a stored cross-association in another unit unchanged: +// a clean element over the stored bytes, so the encoder passes the whole document +// — GUID included — through verbatim. +func cloneCrossAssoc(ca *genDm.CrossAssociation) *genDm.CrossAssociation { + if raw := ca.Raw(); raw != nil { + out := genDm.NewCrossAssociation() + out.SetRaw(raw) + out.InitFromRaw(raw) + out.SetID(ca.ID()) + return out + } + return ca +} + +// assocFromGenCrossAssoc converts a cross-association back into the plain +// association both of whose endpoints now share its unit; childID is the TO +// entity's element id. Like the forward conversion it prefers a raw transform of +// the stored document, so the GUID and every untouched property survive; the +// property build is the fallback for a cross-association never persisted. +func assocFromGenCrossAssoc(ca *genDm.CrossAssociation, childID string) *genDm.Association { + if raw, ok := assocRawFromCrossAssoc(ca, childID); ok { + out := genDm.NewAssociation() + out.SetRaw(raw) + out.InitFromRaw(raw) + out.SetID(ca.ID()) + return out + } + + out := genDm.NewAssociation() + out.SetID(ca.ID()) + out.SetName(ca.Name()) + out.SetDocumentation(ca.Documentation()) + out.SetExportLevel(orDefault(ca.ExportLevel(), "Hidden")) + out.SetParentID(ca.ParentRefID()) + out.SetChildID(element.ID(childID)) + out.SetType(ca.Type()) + out.SetOwner(ca.Owner()) + out.SetStorageFormat(orDefault(ca.StorageFormat(), "Column")) + out.SetParentConnection(domainmodel.DefaultParentConnection) + out.SetChildConnection(domainmodel.DefaultChildConnection) + pdb, cdb := "DeleteMeButKeepReferences", "DeleteMeButKeepReferences" + if odb, ok := ca.DeleteBehavior().(*genDm.AssociationDeleteBehavior); ok { + pdb, cdb = orDefault(odb.ParentDeleteBehavior(), pdb), orDefault(odb.ChildDeleteBehavior(), cdb) + } + out.SetDeleteBehavior(deleteBehaviorToGen(pdb, cdb)) + if src, ok := ca.Source().(*genDm.OqlViewAssociationSource); ok && src != nil { + out.SetSource(oqlViewAssociationSourceToGen(src.Reference())) + } + assignID(out.DeleteBehavior()) + return out +} + +// assocRawFromCrossAssoc is the inverse of crossAssocRawFromAssoc: $Type becomes +// DomainModels$Association, Child (a qualified name) becomes ChildPointer (the TO +// entity's 16-byte id, resolvable now that it shares the unit), and the two +// connection points a plain association declares — the line's on-canvas anchors — +// are added with the defaults mxcli and Studio Pro write for a new association. +// Everything else, the GUID above all, passes through. The key set is the one +// `generated/metamodel` declares for DomainModelsAssociation. +func assocRawFromCrossAssoc(ca *genDm.CrossAssociation, childID string) (bson.Raw, bool) { + raw := ca.Raw() + if raw == nil { + return nil, false + } + elems, err := bsoncore.Document(raw).Elements() + if err != nil { + return nil, false + } + out := make(bson.D, 0, len(elems)+2) + child, parent := false, false + for _, e := range elems { + switch e.Key() { + case "$Type": + out = append(out, bson.E{Key: "$Type", Value: "DomainModels$Association"}) + case "Child": + out = append(out, + bson.E{Key: "ChildConnection", Value: domainmodel.DefaultChildConnection}, + bson.E{Key: "ChildPointer", Value: mmpr.IDToBsonBinary(childID)}) + child = true + case "ParentPointer": + v := e.Value() + out = append(out, + bson.E{Key: "ParentConnection", Value: domainmodel.DefaultParentConnection}, + bson.E{Key: "ParentPointer", Value: bson.RawValue{Type: bson.Type(v.Type), Value: v.Data}}) + parent = true + case "ChildConnection", "ParentConnection": + // Not declared on a cross-association; never carry a stray one twice. + default: + v := e.Value() + out = append(out, bson.E{Key: e.Key(), Value: bson.RawValue{Type: bson.Type(v.Type), Value: v.Data}}) + } + } + if !child || !parent { + return nil, false + } + b, err := bson.Marshal(out) + if err != nil { + return nil, false + } + return bson.Raw(b), true +} diff --git a/mdl/backend/modelsdk/association_move_write.go b/mdl/backend/modelsdk/association_move_write.go index 4f1e88384..2e84dbd91 100644 --- a/mdl/backend/modelsdk/association_move_write.go +++ b/mdl/backend/modelsdk/association_move_write.go @@ -368,6 +368,15 @@ func (b *Backend) MoveEntity(entity *domainmodel.Entity, sourceDMID, targetDMID sourceDM.RemoveAssociations(removeIdx[i]) } + // The cross-associations that already exist (ako/mxcli#628): the ones the moved + // entity is the FROM end of travel with it — or become plain associations again + // when their TO entity is already in the target — and the ones in the target + // that point at it become plain associations there. See association_move_cross.go. + oldEntityQN := sourceModuleName + "." + entity.Name + newEntityQN := targetModuleName + "." + entity.Name + converted = append(converted, moveOwnCrossAssociations(sourceDM, targetDM, entity.ID, sourceModuleName, targetModuleName)...) + converted = append(converted, convertIncomingCrossAssociations(targetDM, entity.ID, oldEntityQN, targetModuleName)...) + // Rewrite the moved entity's module-qualified refs (view source + validations). oldPrefix, newPrefix := sourceModuleName+".", targetModuleName+"." if entity.Source == "DomainModels$OqlViewEntitySource" && strings.HasPrefix(entity.SourceDocumentRef, oldPrefix) { @@ -390,9 +399,9 @@ func (b *Backend) MoveEntity(entity *domainmodel.Entity, sourceDMID, targetDMID // // An ATTRIBUTE reference always follows the entity, so its module prefix is // rewritten unconditionally. An ASSOCIATION reference is rewritten only for the - // associations this move actually sent to the target module: a pre-existing - // cross-association whose parent is the moved entity is not in the conversion - // list and does not travel, so a blanket prefix swap would break it. + // associations this move actually sent to the target module — a converted one, + // or a pre-existing cross-association the moved entity is the FROM end of + // (#628) — never by a blanket prefix swap. assocRenames := make(map[string]string, len(converted)) for _, m := range converted { if m.Moved() { @@ -438,5 +447,10 @@ func (b *Backend) MoveEntity(entity *domainmodel.Entity, sourceDMID, targetDMID if err := b.persistDM(targetDMID, targetDM); err != nil { return nil, fmt.Errorf("MoveEntity: persist target: %w", err) } + // A cross-association in any OTHER module that names the moved entity is + // re-pointed; nothing else in the move rewrites that module's unit (#628). + if err := b.repointCrossAssociationsElsewhere(sourceDMID, targetDMID, oldEntityQN, newEntityQN); err != nil { + return nil, fmt.Errorf("MoveEntity: re-point cross-associations: %w", err) + } return converted, nil } diff --git a/mdl/backend/modelsdk/issue628_move_cross_assoc_test.go b/mdl/backend/modelsdk/issue628_move_cross_assoc_test.go new file mode 100644 index 000000000..180a4dc9a --- /dev/null +++ b/mdl/backend/modelsdk/issue628_move_cross_assoc_test.go @@ -0,0 +1,305 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mendixlabs/mxcli/model" + genDm "github.com/mendixlabs/mxcli/modelsdk/gen/domainmodels" +) + +// TestIssue628_MoveEntitySeesExistingCrossAssociations guards ako/mxcli#628: +// MoveEntity converted the plain associations of the moved entity and never +// looked at the cross-associations that already existed. Moving the second +// endpoint of an association across — the normal way to reorganise a module one +// entity at a time — left a cross-association whose ParentPointer named an +// element absent from its unit, and Studio Pro could no longer load the project +// ("The given key was not present in the dictionary"). +// +// The issue's honest assertion is on the stored documents: after every sequence, +// no association pointer may name an element absent from the unit it lives in, +// and no cross-association may name an entity that is not where it says. The +// association's storage GUID must survive each conversion, which only a Studio +// Pro-authored subject (PedApp, GUID != $ID) can detect. +func TestIssue628_MoveEntitySeesExistingCrossAssociations(t *testing.T) { + type step struct { + moveFrom bool // move the FROM (parent) entity; otherwise the TO (child) entity + to int // index into the three other modules + } + for _, tc := range []struct { + name string + steps []step + // wantPlainIn is the module index (into others) whose unit must hold the + // association as a plain association afterwards; -1 means it stays a + // cross-association. + wantPlainIn int + // wantCrossIn / wantChild, when the association stays cross-module: the + // unit holding it (-1 = the original module) and the module of its TO end. + wantCrossIn, wantChildIn int + }{ + // The issue's reproduction: shape 1 seen from the FROM end. + {name: "ToThenFrom_BackToPlain", steps: []step{{false, 0}, {true, 0}}, wantPlainIn: 0}, + // Shape 1 seen from the TO end: the cross-association is in the target. + {name: "FromThenTo_BackToPlain", steps: []step{{true, 0}, {false, 0}}, wantPlainIn: 0}, + // Shape 2: the FROM end moves on, the cross-association travels with it. + {name: "ToThenFromElsewhere_Travels", steps: []step{{false, 0}, {true, 1}}, wantPlainIn: -1, wantCrossIn: 1, wantChildIn: 0}, + // Shape 3: the TO end moves again; the cross-association stays in a module + // that is neither source nor target of the second move and is re-pointed. + {name: "ToThenToElsewhere_Repointed", steps: []step{{false, 0}, {false, 1}}, wantPlainIn: -1, wantCrossIn: -1, wantChildIn: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + proj := copyPedAppFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + srcID, srcMod, assocName, guid, childID, parentID := associationSubjectDistinct(t, b) + if guid == "" || guid == assocRawID(t, b, srcID, assocName) { + t.Fatalf("subject %s.%s: GUID %q must exist and differ from $ID", srcMod, assocName, guid) + } + others := otherDomainModels(t, b, srcID, 2) + + where := map[model.ID]int{childID: -1, parentID: -1} // -1 = srcMod + dmOf := func(i int) (model.ID, string) { + if i < 0 { + return srcID, srcMod + } + return others[i].id, others[i].name + } + for _, s := range tc.steps { + entID := childID + if s.moveFrom { + entID = parentID + } + fromDM, fromMod := dmOf(where[entID]) + toDM, toMod := dmOf(s.to) + ent := entityByID(t, b, fromDM, entID) + if _, err := b.MoveEntity(ent, fromDM, toDM, fromMod, toMod); err != nil { + t.Fatalf("MoveEntity %s.%s -> %s: %v", fromMod, ent.Name, toMod, err) + } + where[entID] = s.to + } + if err := b.Disconnect(); err != nil { + t.Fatalf("disconnect: %v", err) + } + + b2 := New() + if err := b2.Connect(proj); err != nil { + t.Fatalf("reconnect: %v", err) + } + t.Cleanup(func() { _ = b2.Disconnect() }) + + assertNoDanglingAssociations(t, b2) + + if tc.wantPlainIn >= 0 { + dmID, mod := dmOf(tc.wantPlainIn) + if got := crossAssocGUID(t, b2, dmID, assocName); got != "" { + t.Errorf("%s is still a cross-association in %s; both endpoints are there", assocName, mod) + } + got, ok := plainAssocGUID(t, b2, dmID, assocName) + if !ok { + t.Fatalf("%s is not a plain association in %s after both endpoints moved there", assocName, mod) + } + if got != guid { + t.Errorf("%s: storage GUID changed %s -> %s", assocName, guid, got) + } + return + } + dmID, mod := dmOf(tc.wantCrossIn) + got := crossAssocGUID(t, b2, dmID, assocName) + if got != guid { + t.Errorf("%s in %s: storage GUID %q, want %s", assocName, mod, got, guid) + } + _, childMod := dmOf(tc.wantChildIn) + if ref := crossAssocChild(t, b2, dmID, assocName); !strings.HasPrefix(ref, childMod+".") { + t.Errorf("%s in %s names TO entity %q, want one in %s", assocName, mod, ref, childMod) + } + }) + } +} + +type dmRef struct { + id model.ID + name string +} + +// associationSubjectDistinct is associationSubject restricted to an association +// between two different entities, with no other association between the pair. +func associationSubjectDistinct(t *testing.T, b *Backend) (dmID model.ID, moduleName, assocName, assocGUID string, childID, parentID model.ID) { + t.Helper() + dms, err := b.ListDomainModels() + if err != nil { + t.Fatalf("ListDomainModels: %v", err) + } + for _, d := range dms { + gdm, err := b.loadDomainModelGen(d.ID) + if err != nil { + continue + } + mod, err := b.GetModule(d.ContainerID) + if err != nil || mod == nil { + continue + } + for _, el := range gdm.AssociationsItems() { + a, ok := el.(*genDm.Association) + if !ok || a.Raw() == nil || a.ChildRefID() == a.ParentRefID() { + continue + } + return d.ID, mod.Name, a.Name(), rawKeyHex(t, a.Raw(), "GUID"), + model.ID(a.ChildRefID()), model.ID(a.ParentRefID()) + } + } + t.Fatal("no loadable domain model with an association between two entities") + return +} + +func otherDomainModels(t *testing.T, b *Backend, exclude model.ID, n int) []dmRef { + t.Helper() + dms, err := b.ListDomainModels() + if err != nil { + t.Fatalf("ListDomainModels: %v", err) + } + var out []dmRef + for _, d := range dms { + if d.ID == exclude { + continue + } + if _, err := b.loadDomainModelGen(d.ID); err != nil { + continue + } + mod, err := b.GetModule(d.ContainerID) + if err != nil || mod == nil { + continue + } + out = append(out, dmRef{d.ID, mod.Name}) + if len(out) == n { + return out + } + } + t.Fatalf("fixture has fewer than %d other loadable domain models", n) + return nil +} + +func assocRawID(t *testing.T, b *Backend, dmID model.ID, name string) string { + t.Helper() + gdm, err := b.loadDomainModelGen(dmID) + if err != nil { + t.Fatalf("loadDomainModelGen: %v", err) + } + for _, el := range gdm.AssociationsItems() { + if a, ok := el.(*genDm.Association); ok && a.Name() == name { + return rawKeyHex(t, a.Raw(), "$ID") + } + } + return "" +} + +func plainAssocGUID(t *testing.T, b *Backend, dmID model.ID, name string) (string, bool) { + t.Helper() + gdm, err := b.loadDomainModelGen(dmID) + if err != nil { + t.Fatalf("loadDomainModelGen: %v", err) + } + for _, el := range gdm.AssociationsItems() { + if a, ok := el.(*genDm.Association); ok && a.Name() == name { + return rawKeyHex(t, a.Raw(), "GUID"), true + } + } + return "", false +} + +func crossAssocChild(t *testing.T, b *Backend, dmID model.ID, name string) string { + t.Helper() + gdm, err := b.loadDomainModelGen(dmID) + if err != nil { + t.Fatalf("loadDomainModelGen: %v", err) + } + for _, el := range gdm.CrossAssociationsItems() { + if ca, ok := el.(*genDm.CrossAssociation); ok && ca.Name() == name { + return ca.ChildQualifiedName() + } + } + return "" +} + +// assertNoDanglingAssociations is the stored-document form of "the project +// opens": every association pointer resolves inside its own unit, and every +// cross-association's TO name resolves to an entity in the module it names. +func assertNoDanglingAssociations(t *testing.T, b *Backend) { + t.Helper() + dms, err := b.ListDomainModels() + if err != nil { + t.Fatalf("ListDomainModels: %v", err) + } + type unit struct { + mod string + dm *genDm.DomainModel + } + var units []unit + entitiesByModule := map[string]map[string]bool{} + for _, d := range dms { + gdm, err := b.loadDomainModelGen(d.ID) + if err != nil { + continue + } + mod, err := b.GetModule(d.ContainerID) + if err != nil || mod == nil { + continue + } + units = append(units, unit{mod.Name, gdm}) + names := map[string]bool{} + for _, el := range gdm.EntitiesItems() { + if e, ok := el.(*genDm.Entity); ok { + names[e.Name()] = true + } + } + entitiesByModule[mod.Name] = names + } + for _, u := range units { + ids := map[string]bool{} + for _, el := range u.dm.EntitiesItems() { + ids[string(el.ID())] = true + } + for _, el := range u.dm.AssociationsItems() { + if a, ok := el.(*genDm.Association); ok { + if !ids[string(a.ParentRefID())] || !ids[string(a.ChildRefID())] { + t.Errorf("association %s.%s points at an entity outside its unit", u.mod, a.Name()) + } + } + } + for _, el := range u.dm.CrossAssociationsItems() { + ca, ok := el.(*genDm.CrossAssociation) + if !ok { + continue + } + if !ids[string(ca.ParentRefID())] { + t.Errorf("cross-association %s.%s: ParentPointer names an entity absent from its unit", u.mod, ca.Name()) + } + mod, ent, _ := strings.Cut(ca.ChildQualifiedName(), ".") + if mod == "System" { + continue + } + if !entitiesByModule[mod][ent] { + t.Errorf("cross-association %s.%s: Child %q does not resolve", u.mod, ca.Name(), ca.ChildQualifiedName()) + } + if mod == u.mod { + t.Errorf("cross-association %s.%s names an entity in its own module; it should be a plain association", u.mod, ca.Name()) + } + } + } +} + +// copyPedAppFixture copies the Studio Pro-authored PedApp fixture into a temp +// dir; its elements have GUID != $ID, so a lost storage GUID is observable. +func copyPedAppFixture(t *testing.T) string { + t.Helper() + dst := t.TempDir() + if err := os.CopyFS(dst, os.DirFS("../../../testdata/pedapp")); err != nil { + t.Fatalf("copy PedApp fixture: %v", err) + } + return filepath.Join(dst, "PedApp.mpr") +} diff --git a/mdl/executor/cmd_move.go b/mdl/executor/cmd_move.go index 79fac38e3..7778fbdeb 100644 --- a/mdl/executor/cmd_move.go +++ b/mdl/executor/cmd_move.go @@ -410,9 +410,25 @@ func moveEntity(ctx *ExecContext, name ast.QualifiedName, sourceModule, targetMo } fmt.Fprintf(ctx.Output, "Moved entity %s to %s\n", name.String(), targetModule.Name) - if len(convertedAssocs) > 0 { - fmt.Fprintf(ctx.Output, "Converted %d association(s) to cross-module associations:\n", len(convertedAssocs)) - for _, assocName := range types.MovedAssociationNames(convertedAssocs) { + // A move that brings both endpoints of a cross-association into one module turns + // it back into a plain association (ako/mxcli#628); report the two apart. + var toCross, toPlain []types.MovedAssociation + for _, a := range convertedAssocs { + if a.SameModule { + toPlain = append(toPlain, a) + } else { + toCross = append(toCross, a) + } + } + if len(toCross) > 0 { + fmt.Fprintf(ctx.Output, "Converted %d association(s) to cross-module associations:\n", len(toCross)) + for _, assocName := range types.MovedAssociationNames(toCross) { + fmt.Fprintf(ctx.Output, " - %s\n", assocName) + } + } + if len(toPlain) > 0 { + fmt.Fprintf(ctx.Output, "Converted %d cross-module association(s) to associations within %s:\n", len(toPlain), targetModule.Name) + for _, assocName := range types.MovedAssociationNames(toPlain) { fmt.Fprintf(ctx.Output, " - %s\n", assocName) } } diff --git a/mdl/types/entity_move.go b/mdl/types/entity_move.go index e02fd24d5..ee8292e9a 100644 --- a/mdl/types/entity_move.go +++ b/mdl/types/entity_move.go @@ -27,6 +27,10 @@ type MovedAssociation struct { // NewQualifiedName is what it is called after it. Equal to OldQualifiedName // when the association did not change module. NewQualifiedName string + // SameModule reports that the move brought both endpoints into one module, so + // the association is now a plain (same-module) association rather than a + // cross-module one (ako/mxcli#628). + SameModule bool } // Moved reports whether the association's qualified name changed, i.e. whether From d20c47a8afd4922abbeccce401eea554bb478119 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 20:31:22 +0000 Subject: [PATCH 3/5] fix(mpr): refuse file writes while Studio Pro has the project open (mendixlabs/mxcli#849) Studio Pro does not reload the model from disk, so a write mxcli makes while the project is open is silently discarded by Studio Pro's next save. The writer now refuses any write that would reach storage while Studio Pro's .mpr.lock is beside the .mpr (matched without regard to case, as Studio Pro lower-cases it). Reads and writes elided as no-ops are never refused; exec --force or MXCLI_ALLOW_STUDIO_PRO_OPEN=1 override. Co-Authored-By: Claude Opus 5.5 --- .../skills/fix-issue/findings/modelsdk.jsonl | 1 + .claude/skills/mendix/check-syntax/SKILL.md | 4 +- cmd/mxcli/cmd_exec.go | 17 +++ docs-site/src/reference/capabilities.md | 2 +- docs-site/src/tutorial/quickstart.md | 2 +- modelsdk/mpr/studiopro_lock.go | 99 ++++++++++++++ modelsdk/mpr/studiopro_lock_test.go | 124 ++++++++++++++++++ modelsdk/mpr/writer_core.go | 18 +++ 8 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 modelsdk/mpr/studiopro_lock.go create mode 100644 modelsdk/mpr/studiopro_lock_test.go diff --git a/.claude/skills/fix-issue/findings/modelsdk.jsonl b/.claude/skills/fix-issue/findings/modelsdk.jsonl index e55dc06ee..3cdc21513 100644 --- a/.claude/skills/fix-issue/findings/modelsdk.jsonl +++ b/.claude/skills/fix-issue/findings/modelsdk.jsonl @@ -24,3 +24,4 @@ {"area":"modelsdk/mpr","date":"2026-09-25","symptom":"`alter page FeedbackModule.ShareFeedback_Logo { insert after textBox1 { image zzImg (ImageType: imageUrl, ImageUrl: '{1}', ImageUrlParams: [{1} = ImageB64]) } }` (data view over a nanoflow the project lacks, 11.13.0) reported \"Altered page\"; `mxcli docker check` then could not LOAD the project: ArgumentNullException setting 'Attribute' of an Attribute in a Page","cause":"The bare-AttributeRef refusal (#678) lived in encodePage/encodeSnippet only. ALTER PAGE patches the stored BSON in pagemutator and saves via UpdateRawUnit, never passing the encoder; with no entity in scope the pluggable-widget template-parameter builder (widgetobj) writes the name as given, so DomainModels$AttributeRef{Attribute:\"ImageB64\"} reached disk","file":"modelsdk/canon/attributeref.go; modelsdk/mpr/writer_core.go (updateUnit, insertUnit)","insight":"A guard placed in one encoder covers one write path; the page family has at least four (encodePage/Snippet, pagemutator Save, widget sync apply, layout/template raw writes). Put an unloadable-shape refusal at the writer beside DuplicateElementIDError, as that one already argued. Measured before refusing stored refs too: 73 of 73 AttributeRefs across all 374 units of a stock 11.13 project are qualified (71 page, 1 snippet, 1 page template, none elsewhere) — a stored bare one cannot have come from Studio Pro, so refusing ALL bare refs (not only new ones) blocks nothing legitimate. The textbox path does NOT reproduce it: attributeRefToGen nulls a bare name (a silent binding drop instead); the pluggable/column template builders are the ones that write it verbatim. The test goes through the real mutator + writer on the expr-checker fixture (InsertColumns with a bare CaptionParams ref).","refs":["#678"]} {"area": "modelsdk/widgets", "date": "2026-09-26", "symptom": "CE0463 \"The definition of this widget has changed\" on every page carrying a pluggable widget built from its .mpk whose action properties declare `` (Signature 2.1.0, Calendar 2.6.0 on 11.12.2) — even with no action configured. `mx update-widgets` clears it", "cause": "The .mpk parser had no field for `` (modelsdk/widgets/mpk/mpk.go xmlProperty/PropertyDef), and createDefaultValueType hardcoded `ActionVariables: [2]`; reconcileValueTypesFromMPK never touched the list. The typed gen class (CustomWidgets$WidgetActionVariable) existed but the map-based template pipeline never fed it", "file": "modelsdk/widgets/mpk/mpk.go (ActionVariable, toActionVariables), modelsdk/widgets/augment.go (buildActionVariablesArray, actionVariablesMatch, createDefaultValueType, reconcileValueTypesFromMPK); tests actionvariables_test.go in both packages; example mdl-examples/bug-tests/1200-mpk-action-variables.mdl", "insight": "Third instance of the same shape after #716 (onChange) and #956 (defaultType): a widget.xml attribute/element that is part of the DEFINITION, never parsed, written as its empty default. Cheapest audit: diff every key Studio Pro stores on a WidgetValueType in the embedded templates against what createDefaultValueType derives from the .mpk — the embedded combobox.json already held the correct ActionVariables entry, i.e. the oracle was in the repo. `sdk/widgets/augment.go` has no importers; the live BSON path is modelsdk/widgets. When reconciling a list from the .mpk, rewrite only on disagreement, or an agreeing template's entry $IDs churn — the Combobox augment test is the no-change control (it fails if the rewrite is unconditional).", "refs": ["mendixlabs/mxcli#1200", "mendixlabs/mxcli#956", "#716"], "ce": ["CE0463"]} {"area": "modelsdk/canon", "date": "2026-09-26", "symptom": "A page rewrite still loses translations despite CarryTranslations: an empty caption's en_US '' vanishes (Texts$Text with no items), and a label's nl_NL 'Gebruikers' is dropped because its English 'Account Overview' is also the page title's", "cause": "Positional pairing needs the whole document's text paths unchanged — one DataGrid2 rebuilt from its template breaks that. Source pairing keys on (language, text): (en_US, '') is ambiguous on any real page, a shared English source is ambiguous, and a rebuilt EMPTY text has no translation to look up by at all", "file": "modelsdk/canon/translations.go", "insight": "Address a text by the named element that owns it — ($Type, Name, path from the element) — because a widget Name is unique per document. Exact only where the path from the element crosses no list index (a rebuilt pluggable widget reorders Properties; pairing there moves a translation onto the wrong property) and where the address occurs once in each document. Order: positional when the shape is unchanged, then owning element, then source", "refs": ["ako/mxcli#705"]} +{"date": "2026-10-01", "area": "modelsdk", "symptom": "mendixlabs/mxcli#849: `mxcli exec` writes the .mpr while Studio Pro has the project open; the write succeeds, mx check passes, and Studio Pro's next save silently discards it. No warning, no error.", "cause": "No write path looked for Studio Pro at all; the rule 'close Studio Pro first' existed only in prose (README, docs-site, skills).", "fix": "modelsdk/mpr/studiopro_lock.go: Writer.guardWrite refuses every write that would reach storage (updateUnit and WriteTransaction.WriteUnit after no-op elision, insertUnit, deleteUnit, MoveUnit, UpdateUnitContainer) with StudioProOpenError while `.mpr.lock` (matched case-insensitively) is beside the .mpr; `exec --force` / MXCLI_ALLOW_STUDIO_PRO_OPEN=1 override. Reads and elided no-op writes are never refused.", "insight": "Studio Pro's own generated .gitignore is the evidence for the signal and its spelling: it lists `testapp.mpr.lock` (lower-cased) beside `TestApp.mpr`, plus `mprcontents/mprjournal*`. Guarding at the storage layer after elision, not at command level, keeps twice-exec a no-op instead of an error and covers every command that writes.", "issue": "mendixlabs/mxcli#849", "file": "modelsdk/mpr/studiopro_lock.go"} diff --git a/.claude/skills/mendix/check-syntax/SKILL.md b/.claude/skills/mendix/check-syntax/SKILL.md index 235a9f6ac..2a45fb7e0 100644 --- a/.claude/skills/mendix/check-syntax/SKILL.md +++ b/.claude/skills/mendix/check-syntax/SKILL.md @@ -625,7 +625,9 @@ does not hot-reload when an external process changes the file. So after `mxcli e - `ped_read_document` / `ped_check_errors` will show the **stale** pre-exec model until Studio Pro re-scans — call `refresh_project` first (or reload the project in the UI). - **Hazard:** if Studio Pro later saves on its own, it overwrites mxcli's disk write with - its in-memory copy, silently discarding your MDL changes. + its in-memory copy, silently discarding your MDL changes. So a file-based write is + **refused** while Studio Pro's `.mpr.lock` is beside the `.mpr`; `exec --force` + (or `MXCLI_ALLOW_STUDIO_PRO_OPEN=1`) overrides it, e.g. for a lock left by a crash. **Safest practice:** don't keep the same project open-and-saving in Studio Pro while mxcli writes it. Either close (or don't save in) Studio Pro during MDL authoring, or diff --git a/cmd/mxcli/cmd_exec.go b/cmd/mxcli/cmd_exec.go index d50f5a2d8..2300d1ce8 100644 --- a/cmd/mxcli/cmd_exec.go +++ b/cmd/mxcli/cmd_exec.go @@ -11,6 +11,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/executor" "github.com/mendixlabs/mxcli/mdl/linter" "github.com/mendixlabs/mxcli/mdl/visitor" + mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" "github.com/spf13/cobra" ) @@ -35,6 +36,13 @@ makes a partially-applied domain script re-runnable — the already-applied statements (e.g. "attribute already exists") error individually while the not- yet-applied ones still run — without a failure masking later work. +A write is refused while Studio Pro has the project open (its .mpr.lock +is beside the .mpr): Studio Pro does not reload the model from disk, and its next +save would silently discard the change. Close the project in Studio Pro, or route +writes through it with --mcp. --force writes anyway (for a lock left behind by a +crash); MXCLI_ALLOW_STUDIO_PRO_OPEN=1 does the same for every command. Reads, and +re-running a script whose statements change nothing, are never refused. + Pass "-" as the file to read the script from standard input, so MDL can be piped or written inline as a heredoc without a temporary file. @@ -53,6 +61,13 @@ Example: projectPath, _ := cmd.Flags().GetString("project") continueOnError, _ := cmd.Flags().GetBool("continue-on-error") skipCheck, _ := cmd.Flags().GetBool("no-check") + if force, _ := cmd.Flags().GetBool("force"); force { + mmpr.AllowWritesWhileStudioProOpen = true + if lock, _ := mmpr.StudioProLockFile(projectPath); lock != "" { + fmt.Fprintf(os.Stderr, "Warning: Studio Pro appears to have this project open (%s); writing anyway (--force). "+ + "Studio Pro's next save will discard these changes unless the project is closed or reloaded first.\n", lock) + } + } depPolicy := deprecationPolicy(cmd) // Read the script (a path, or "-" for stdin) @@ -222,6 +237,8 @@ Example: func init() { execCmd.Flags().Bool("no-check", false, "Skip the pre-flight semantic checks and apply the script even if mxcli check would report errors") + execCmd.Flags().Bool("force", false, + "Write even though Studio Pro appears to have the project open (its .mpr.lock is present) — e.g. a lock left behind by a crash") execCmd.Flags().Bool("continue-on-error", false, "Run every statement, reporting each failure instead of halting at the first (exits non-zero if any failed) — makes a partially-applied script re-runnable") } diff --git a/docs-site/src/reference/capabilities.md b/docs-site/src/reference/capabilities.md index 063d7038f..790bf5f53 100644 --- a/docs-site/src/reference/capabilities.md +++ b/docs-site/src/reference/capabilities.md @@ -142,7 +142,7 @@ Everything mxcli can do, organized by use case. |---|---|---| | Design properties (Atlas v3) | Requires Mendix 11.0+ | Use CSS classes on 10.x | | REST query parameters | Requires Mendix 11.0+ | Build query string manually on 10.x | -| Concurrent editing | Not supported | Close Studio Pro before mxcli writes | +| Concurrent editing | Not supported — a file-based write is refused while Studio Pro has the project open (`.mpr.lock` present) | Close Studio Pro before mxcli writes, or write through it with `--mcp`; `exec --force` overrides | | Widget template drift | CE0463 on version mismatch | MPK augmentation handles most cases | | Marketplace module update | Existing modules are reported, not updated in place | Update via Studio Pro (preserves local edits and entity IDs) | | 47 of 52 metamodel domains | Not yet implemented | REST, OData write, etc. pending | diff --git a/docs-site/src/tutorial/quickstart.md b/docs-site/src/tutorial/quickstart.md index 0e841f9ca..524871130 100644 --- a/docs-site/src/tutorial/quickstart.md +++ b/docs-site/src/tutorial/quickstart.md @@ -136,4 +136,4 @@ mxcli setup mxbuild -p your-app.mpr **"CGO not available"** -- mxcli uses pure Go SQLite. No C compiler needed. If you see CGO errors, ensure you're using the official pre-built binary or a `make build` from source. -**Project won't open in Studio Pro after changes** -- Close Studio Pro before running mxcli write commands, then reopen. See [F4 sync support](../appendixes/version-compatibility.md) for details. +**"refusing to write …: Studio Pro has this project open"** -- Close the project in Studio Pro before running mxcli write commands, then reopen it. Studio Pro does not reload the model from disk, so its next save would silently discard mxcli's changes. If Studio Pro is not running, the `.mpr.lock` was left behind by a crash: delete it, or pass `--force` to `exec`. diff --git a/modelsdk/mpr/studiopro_lock.go b/modelsdk/mpr/studiopro_lock.go new file mode 100644 index 000000000..7e4f12ebc --- /dev/null +++ b/modelsdk/mpr/studiopro_lock.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +// Studio Pro open-project guard (mendixlabs/mxcli#849). +// +// Studio Pro keeps the model in memory and does not reload it when another +// process changes the files. A write mxcli makes while the project is open is +// silently discarded the next time Studio Pro saves: the write succeeds, `mx +// check` passes, and the change is gone. So every write that would reach storage +// is refused while Studio Pro has the project open, unless the caller opted out +// (exec --force, or MXCLI_ALLOW_STUDIO_PRO_OPEN=1). Reads are never refused, and +// a write that reconciliation elides as a no-op never gets here, so re-running a +// script that is already applied keeps working. +// +// The signal is the lock file Studio Pro creates beside the .mpr while the +// project is open and removes on close: `.mpr.lock`. Studio Pro's own +// generated .gitignore lists it (with the project name LOWER-cased — TestApp's +// says `testapp.mpr.lock` beside `TestApp.mpr`), so the name is matched without +// regard to case. A Studio Pro that crashed leaves the file behind; the message +// says so and how to proceed, because failing toward noise is the point — the +// failure this guards against is silent. + +// AllowWritesWhileStudioProOpen disables the guard for this process. It is the +// `--force` of the CLI; nothing sets it implicitly. +var AllowWritesWhileStudioProOpen bool + +// AllowStudioProOpenEnv is the environment variable that disables the guard, +// for commands that have no --force of their own (REPL, -c, other writers). +const AllowStudioProOpenEnv = "MXCLI_ALLOW_STUDIO_PRO_OPEN" + +// StudioProOpenError reports a refused write. +type StudioProOpenError struct { + MprPath string + LockPath string + LockTime time.Time +} + +func (e *StudioProOpenError) Error() string { + return fmt.Sprintf("refusing to write %s: Studio Pro has this project open (lock file %s, last modified %s). "+ + "Studio Pro does not reload the model from disk, so its next save would silently discard this change. "+ + "Close the project in Studio Pro and retry, or write through Studio Pro with --mcp. "+ + "If Studio Pro is not running (a lock left behind by a crash), delete the lock file, "+ + "or re-run with --force (exec) or %s=1 to write anyway", + filepath.Base(e.MprPath), e.LockPath, e.LockTime.Format(time.RFC3339), AllowStudioProOpenEnv) +} + +// StudioProLockFile returns the path of the Studio Pro lock file beside mprPath, +// or "" when there is none. +func StudioProLockFile(mprPath string) (string, time.Time) { + if mprPath == "" { + return "", time.Time{} + } + dir, base := filepath.Split(mprPath) + if dir == "" { + dir = "." + } + want := base + ".lock" + entries, err := os.ReadDir(dir) + if err != nil { + return "", time.Time{} + } + for _, e := range entries { + if e.IsDir() || !strings.EqualFold(e.Name(), want) { + continue + } + var mod time.Time + if info, err := e.Info(); err == nil { + mod = info.ModTime() + } + return filepath.Join(dir, e.Name()), mod + } + return "", time.Time{} +} + +// studioProOpenGuard is called immediately before a write reaches storage. +func studioProOpenGuard(mprPath string) error { + if AllowWritesWhileStudioProOpen || os.Getenv(AllowStudioProOpenEnv) == "1" { + return nil + } + lock, mod := StudioProLockFile(mprPath) + if lock == "" { + return nil + } + return &StudioProOpenError{MprPath: mprPath, LockPath: lock, LockTime: mod} +} + +// guardWrite applies the guard to this writer's project. +func (w *Writer) guardWrite() error { + return studioProOpenGuard(w.reader.path) +} diff --git a/modelsdk/mpr/studiopro_lock_test.go b/modelsdk/mpr/studiopro_lock_test.go new file mode 100644 index 000000000..cd0656561 --- /dev/null +++ b/modelsdk/mpr/studiopro_lock_test.go @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mpr + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// mendixlabs/mxcli#849: a file-based write while Studio Pro has the project open +// is silently discarded by Studio Pro's next save. The writer refuses it when +// Studio Pro's lock file is beside the .mpr. + +const lockUnitID = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + +// studioProLock creates the lock file the way Studio Pro names it: the project +// name lower-cased (Studio Pro's own .gitignore lists `testapp.mpr.lock` beside +// `TestApp.mpr`), here against the fixture's `app.mpr`, so it is spelled in +// upper case to prove the match ignores case. +func studioProLock(t *testing.T, w *Writer) string { + t.Helper() + p := filepath.Join(filepath.Dir(w.reader.path), "APP.MPR.LOCK") + if err := os.WriteFile(p, nil, 0644); err != nil { + t.Fatalf("create lock: %v", err) + } + return p +} + +func TestStudioProOpen_WriteRefused(t *testing.T) { + t.Setenv(AllowStudioProOpenEnv, "") + stored := unitDoc(t, "Before") + w, unitPath := newV2WriterForCommitTest(t, lockUnitID, stored) + studioProLock(t, w) + + err := w.UpdateRawUnit(lockUnitID, unitDoc(t, "After")) + var spErr *StudioProOpenError + if !errors.As(err, &spErr) { + t.Fatalf("UpdateRawUnit with Studio Pro open: err = %v, want *StudioProOpenError", err) + } + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(stored) { + t.Error("a refused write reached disk") + } + if err := w.DeleteUnit(lockUnitID); !errors.As(err, &spErr) { + t.Errorf("DeleteUnit with Studio Pro open: err = %v, want *StudioProOpenError", err) + } + if err := w.InsertUnit("bbbbbbbb-bbbb-cccc-dddd-eeeeeeeeeeee", lockUnitID, "Documents", "Projects$Folder", unitDoc(t, "New")); !errors.As(err, &spErr) { + t.Errorf("InsertUnit with Studio Pro open: err = %v, want *StudioProOpenError", err) + } + + // Reads are never refused. + if got, err := w.reader.GetRawUnitBytes(lockUnitID); err != nil || string(got) != string(stored) { + t.Errorf("read with Studio Pro open: err = %v", err) + } +} + +// A write that reconciliation elides never reaches storage, so it is not +// refused: re-running an applied script with Studio Pro open stays a no-op +// rather than an error. +func TestStudioProOpen_NoOpWriteNotRefused(t *testing.T) { + t.Setenv(AllowStudioProOpenEnv, "") + stored := unitDoc(t, "Same") + w, _ := newV2WriterForCommitTest(t, lockUnitID, stored) + studioProLock(t, w) + if err := w.UpdateRawUnit(lockUnitID, unitDoc(t, "Same")); err != nil { + t.Fatalf("no-op write with Studio Pro open refused: %v", err) + } +} + +// Controls: the same write lands without a lock file, and with the override. +func TestStudioProOpen_Controls(t *testing.T) { + t.Setenv(AllowStudioProOpenEnv, "") + t.Run("NoLock", func(t *testing.T) { + w, unitPath := newV2WriterForCommitTest(t, lockUnitID, unitDoc(t, "Before")) + after := unitDoc(t, "After") + if err := w.UpdateRawUnit(lockUnitID, after); err != nil { + t.Fatalf("UpdateRawUnit without a lock: %v", err) + } + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(after) { + t.Error("write did not land") + } + }) + t.Run("Env", func(t *testing.T) { + t.Setenv(AllowStudioProOpenEnv, "1") + w, unitPath := newV2WriterForCommitTest(t, lockUnitID, unitDoc(t, "Before")) + studioProLock(t, w) + after := unitDoc(t, "After") + if err := w.UpdateRawUnit(lockUnitID, after); err != nil { + t.Fatalf("UpdateRawUnit with %s=1: %v", AllowStudioProOpenEnv, err) + } + if onDisk, _ := os.ReadFile(unitPath); string(onDisk) != string(after) { + t.Error("write did not land") + } + }) + t.Run("Force", func(t *testing.T) { + AllowWritesWhileStudioProOpen = true + t.Cleanup(func() { AllowWritesWhileStudioProOpen = false }) + w, _ := newV2WriterForCommitTest(t, lockUnitID, unitDoc(t, "Before")) + studioProLock(t, w) + if err := w.UpdateRawUnit(lockUnitID, unitDoc(t, "After")); err != nil { + t.Fatalf("UpdateRawUnit with --force: %v", err) + } + }) +} + +func TestStudioProLockFile_MatchesOnlyTheProjectsLock(t *testing.T) { + dir := t.TempDir() + mpr := filepath.Join(dir, "TestApp.mpr") + for _, n := range []string{"Other.mpr.lock", "TestApp.mpr.bak", "testapp.mpr.lock.old"} { + if err := os.WriteFile(filepath.Join(dir, n), nil, 0644); err != nil { + t.Fatal(err) + } + } + if got, _ := StudioProLockFile(mpr); got != "" { + t.Fatalf("StudioProLockFile = %q with no lock for this project", got) + } + if err := os.WriteFile(filepath.Join(dir, "testapp.mpr.lock"), nil, 0644); err != nil { + t.Fatal(err) + } + if got, _ := StudioProLockFile(mpr); filepath.Base(got) != "testapp.mpr.lock" { + t.Fatalf("StudioProLockFile = %q, want the lower-cased lock Studio Pro writes", got) + } +} diff --git a/modelsdk/mpr/writer_core.go b/modelsdk/mpr/writer_core.go index 983cb2a30..3c06e84cf 100644 --- a/modelsdk/mpr/writer_core.go +++ b/modelsdk/mpr/writer_core.go @@ -230,6 +230,9 @@ func (wt *WriteTransaction) WriteUnit(unitID string, contents []byte) error { if unchanged { return nil } + if err := wt.writer.guardWrite(); err != nil { + return err + } unitIDBlob := uuidToBlob(unitID) @@ -538,6 +541,9 @@ func (w *Writer) insertUnit(unitID, containerID, containmentName, unitType strin if err := canon.BareAttributeRefError(unitID, contents); err != nil { return err } + if err := w.guardWrite(); err != nil { + return err + } // Convert UUID strings to 16-byte blobs for database unitIDBlob := uuidToBlob(unitID) @@ -647,6 +653,9 @@ func (w *Writer) updateUnit(unitID string, contents []byte, opts ...canon.Option if unchanged { return nil } + if err := w.guardWrite(); err != nil { + return err + } // Convert UUID string to 16-byte blob unitIDBlob := uuidToBlob(unitID) @@ -894,6 +903,9 @@ func (w *Writer) MoveUnit(unitID, newContainerID string) error { if stored, err := w.containerOfUnit(unitID); err == nil && bytes.Equal(stored, target) { return nil } + if err := w.guardWrite(); err != nil { + return err + } _, err := w.reader.db.Exec(`UPDATE Unit SET ContainerID = ? WHERE UnitID = ?`, target, uuidToBlob(unitID)) if err == nil { @@ -919,6 +931,9 @@ func (w *Writer) deleteUnit(unitID string) error { if unitIDBlob == nil { return fmt.Errorf("invalid unit ID: %s", unitID) } + if err := w.guardWrite(); err != nil { + return err + } w.dropDeferred(unitID) w.rememberRemovedUnit(unitID) @@ -968,6 +983,9 @@ func (w *Writer) UpdateUnitContainer(unitID, newContainerID string) error { if containerIDBlob == nil { return fmt.Errorf("invalid container ID: %s", newContainerID) } + if err := w.guardWrite(); err != nil { + return err + } result, err := w.reader.db.Exec(`UPDATE Unit SET ContainerID = ? WHERE UnitID = ?`, containerIDBlob, unitIDBlob) if err != nil { From 2ed9b8649c970c66f1d6db0334ae0ebf26167034 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 20:34:47 +0000 Subject: [PATCH 4/5] fix(published-rest): write an operation's query and body parameters, its mappings and its commit option (#571) create published rest service wrote only the path's {name} placeholders as operation parameters, each a String: a query or body microflow parameter failed mx check with CE0350, an Integer {id} with CE6539. import mapping, export mapping and commit parsed and were thrown away. - Parameters are derived from the microflow as Studio Pro derives them (path name -> Path, object/list -> Body, HttpRequest/HttpResponse -> none, else Query), each with the microflow parameter's type, merged over the stored parameters so a header / renamed / described one survives. - Mappings and commit go AST -> model -> BSON and back; describe prints them and notes parameters MDL cannot state. An unknown commit option is refused by exec and check (MDL-REST03). - create or modify carries the restated operation's summary, documentation and object handling; the service rewrite carries the stored keys MDL cannot state (authentication, CORS, documentation). List markers as Studio Pro writes them. TestApp's Services.OrdersRestApi leaves the round-trip allowlist. Fixes mendixlabs/mxcli#1206 Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-executor.jsonl | 1 + CHANGELOG.md | 1 + cmd/mxcli/syntax/features_integration.go | 11 +- docs-site/src/examples/rest-integration.md | 32 +- ...published-rest-parameters-and-mappings.mdl | 94 ++++++ mdl/backend/modelsdk/export_level_carry.go | 68 ++++ mdl/backend/modelsdk/integration_read.go | 43 ++- mdl/backend/modelsdk/published_rest_write.go | 147 +++++--- .../modelsdk/published_rest_write_test.go | 125 +++++++ mdl/executor/cmd_published_rest.go | 290 ++++++++++++++-- .../cmd_published_rest_params_test.go | 314 ++++++++++++++++++ mdl/executor/validate_program.go | 4 + mdl/executor/validate_rest_mapping.go | 37 +++ mdl/roundtrip/testapp_allowlist_test.go | 1 - mdl/visitor/visitor_rest.go | 4 +- model/types.go | 57 ++++ 16 files changed, 1153 insertions(+), 76 deletions(-) create mode 100644 mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl create mode 100644 mdl/executor/cmd_published_rest_params_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index db4ac1363..bf6255a66 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -791,3 +791,4 @@ {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#890 (rehearsal 2, R-rep): re-running a settled script wrote nothing (units_written=0) but still printed \"Granted access on ...\", \"Set project security level to ...\", \"Added module roles ... to user role ...\", \"Updated ... settings\" / \"Updated configuration ...\", and `move ... to folder` printed \"Moved ... to new location\"; the output could not serve as the #859 'second run reports Unchanged' gate. Also: a second `move` of the same document in one session failed \"microflow not found\".", "cause": "Those handlers printed their sentence with fmt.Fprintf after the backend call instead of going through ReportMutation's write-elision evidence (WriteStats offered vs written). Grants/revokes inside a program run are deferred (#872 accessRuleRun), so even ReportMutation could not see their write at statement time. The typed movers changed a container without invalidating the cached hierarchy.", "file": "mdl/executor/report_mutation.go, mdl/executor/access_rule_run.go, mdl/executor/cmd_security_write.go, mdl/executor/cmd_settings.go, mdl/executor/cmd_move.go", "fix": "ExecContext.reportWrite(unchanged, sentence...) prints the sentence or `Unchanged ` (through the run tally) on the ReportMutation evidence rule; used for project security level/demo users/strict mode/guest access, alter user role module roles, settings section/configuration/constant updates. Access-rule reports go through reportAccessRule: held on the open accessRuleRun and printed at its flush, Unchanged when the flush offered and elided (notices like 'No access rules found' print regardless). execMove short-circuits a document already in the target container (alreadyPlaced -> Unchanged) and invalidates the hierarchy after every move.", "test": "mdl/executor/noop_reporting_pedapp_test.go TestNoopRerun_ReportsUnchanged (PedApp, per statement: run 1 reports its write = control; run 2 writes no file and reports Unchanged, for grant, security level, demo users, strict mode, user role module roles, settings runtime, configuration (alter and create or modify), move) and TestNoopRerun_ProgramReportsUnchanged (program run incl. a revoke+grant reset; run 2 writes nothing and reports no write verb; run 1's net-nothing reset reports no write). Revert check: every case fails with the write sentence on run 2; the move case with 'microflow not found'.", "insight": "A report printed after a backend call is a claim about storage the handler cannot make on its own; route every write report through the write-stats evidence, and where writes are deferred, defer the report with them. The output only becomes an idempotency gate when no statement prints a write verb by construction."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#840 (mxcli-ledger, finding 162): `mxcli describe` (no header, no option) wrote mdl 0 spellings that mdl 1 refuses, and its microflow output held `$N = count($Hits)` / `$x = find($L, …)` — call forms registered as deprecated (MDL-DEPR003/004), so subcommand output was neither mdl 1-runnable nor mdl 0-clean. At the freeze a second instance surfaced on TestApp: describe of a chart series' text template wrote `staticTooltipHoverTextParams: [{1} = X]`, the bracketed form MDL-DEPR124 deprecates, under both versions.", "cause": "formatListOperation / the AggregateListAction case gated the statement form on describeLanguage >= V1 and fell back to the call form for mdl 0, although the statement form parses with the same meaning and no warning under mdl 0. The object-list describer (cmd_pages_describe_objectlist.go) wrote a TextTemplate's Params as \"[\" + … + \"]\". The roundtrip test that should have caught both (describeUsesCanonicalSpellings) filtered deprecations to an R8 allowlist ('describe keeps them under mdl 0'), so any code outside the list was invisible.", "file": "mdl/executor/cmd_microflows_format_action.go, mdl/executor/cmd_pages_describe_objectlist.go, mdl/roundtrip/describe_canonical_spelling_test.go", "fix": "Describe writes the List operation / Aggregate list statement in every language; only an activity the statement cannot express falls back to the call. Object-list template parameters are written in ( ). The canonical-spelling roundtrip test now checks EVERY registered deprecation under both describe languages (describeAs V1 and V0) on PedApp and TestApp, with no code filter. Freeze: langver.Frozen = V1, every describe output starts with `mdl 1;`, `--mdl 0|1` on describe/context/diff-local.", "test": "mdl/executor/cmd_microflows_format_list_activity_test.go TestDescribeListActivityUnderMdl1 (both versions, plus the call-form control warning under mdl 0); mdl/roundtrip/describe_canonical_spelling_test.go TestTestAppDescribeUsesCanonicalSpellings (failed on Snip_TaskDashboard_Numbers & 2 more with MDL-DEPR124 before the objectlist fix).", "insight": "A test that filters warnings to a list of 'codes this test owns' hides every code added later; a 'never emits X' property must check the whole registry, and in every output language the command can be asked for."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#905 part 1 (#897 item 4, rehearsal 3 G2): under `mdl 1`, `create or modify microflow` that grows a stored flow by an activity whose custom error handler ends in its own `return` (`$Ok = call microflow … on error begin log …; return; end error;`) was refused on every run, as an insert or a replace: \"an error handler in the fragment ends at an end event of its own … a return inside an error handler is not spliced yet\". Created fresh the statement worked; mdl 0 rebuilt (MDL-V1-REBUILD). On CapTrack the stub-then-real pair (13-actions stub, 30-export real ACT_Export_Excel) left the placeholder stored on every run with mx check clean. Once spliced, the second run of the real CapTrack script was refused: \"replace $Written: cannot replace the ActionActivity …: it has an error handler\".", "cause": "addErrorHandlerFlow (cmd_microflows_builder_flows.go) builds a handler body with a child flowBuilder and merged its objects and flows into the parent, but not its returnEndIDs, so cutFragment saw the handler's return end event as one the builder added and refused it. Second defect, exposed by the grow: a log/show message/validation feedback message written as an expression (`'failed for ' + $User/Name`) is stored as template '{1}' with the expression as parameter, and describe prints that form; declaredMatches compared the two spellings as different statements. Where builtAsStored is false (a spliced or Studio Pro-drawn flow), the statement diff then replaced the activity: absorbed by write elision on the main path, but refused when the message sits in a stored activity's error handler.", "file": "mdl/executor/cmd_microflows_builder_flows.go, mdl/executor/flow_declared_match.go", "fix": "addErrorHandlerFlow copies errBuilder.returnEndIDs into the parent's (lastReturnEndID untouched), so a handler's return is a new end event of the flow like a guard's (#888); placement/room checks (checkRoom/checkBranches) apply unchanged and refuse where the handler's return branch would cross a stored flow. matchValue normalises LogStmt/ShowMessageStmt/ValidationFeedbackStmt (messageAsTemplate) to the builder's stored form: a non-literal message becomes '{1}' with the expression as first parameter (a log stating its own `with (...)` keeps its message, as the builder does).", "test": "mdl/executor/cmd_alter_flow_handler_return_test.go TestCutFragment_HandlerReturnIsANewEndEvent; mdl/executor/flow_message_respelling_test.go (with controls); mdl/roundtrip/flow_splice_handler_return_test.go TestSpliceRerun_GrowByHandlerReturn (replace/insert under mdl 1, insert under mdl 0, verdict agreement check/diff/exec, twice-exec, a changed-flow control), TestSpliceRerun_GrowStudioProFlowByHandlerReturn (PedApp ShowPasswordForm, all stored IDs kept, description fixed point), TestSpliceRerun_HandlerReturnWithNoRoomIsRefused. Revert checks: returnEndIDs not copied -> every grow refused 'not spliced yet' (check predicts it); messageAsTemplate off -> second run refused 'replace $Ok … it has an error handler'. CapTrack copy: fmt --upgrade -p 13+30, exec 13, 30, 30 -> real flow stored, third run 0 units; mx check identical to baseline (0 errors). PedApp repros mx check identical to baseline.", "insight": "Child builders (error handler, loop) each keep builder state the parent's consumers rely on; when a new piece of builder state is added for the splice (returnEndIDs, #888), enumerate the child builders and decide per child whether it propagates. A grow test that only re-runs on an mxcli-authored flow can pass because builtAsStored short-circuits the statement diff; the respelling only surfaced on the real project and the Studio Pro-drawn flow, where the statement diff decides."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#571 / mendixlabs/mxcli#1206: `create published rest service` wrote only the path's {name} placeholders as operation parameters, each a String: every query and body microflow parameter failed mx check with CE0350, an Integer {id} with CE6539. `import mapping` / `export mapping` / `commit` on an operation parsed and were thrown away (CE0350 on the body, CE0354 on an object-returning microflow). Executing describe of a Studio Pro service (TestApp Services.OrdersRestApi) reported 'Modified' and broke a 0-error app with 5 errors: mappings cleared, Integer path params retyped String, body param dropped, Commit No->Yes, Basic+Session authentication turned off", "cause": "publishedRestOperationToGen built parameters from the path alone and wrote ExportMapping/ImportMapping \"\" and Commit \"Yes\" as constants; the reader never read parameters, mappings or commit, so describe could not print them and ALTER (which rewrites every operation) lost them too; the service writer also emitted constants for AuthenticationTypes / AuthenticationMicroflow / CorsConfiguration / Documentation / PublicDocumentation with no carry; Resources and operation Parameters were registered with list marker 2 where Studio Pro writes 3", "file": "mdl/executor/cmd_published_rest.go, mdl/backend/modelsdk/published_rest_write.go, mdl/backend/modelsdk/integration_read.go, mdl/backend/modelsdk/export_level_carry.go, mdl/visitor/visitor_rest.go, model/types.go", "fix": "the executor derives operation parameters from the microflow as Studio Pro does (path name -> Path, object/list -> Body, System.HttpRequest/HttpResponse -> none, else Query; the microflow parameter's type), merged over the stored parameters per bound microflow parameter so a header/renamed/described parameter survives; mappings and commit flow AST -> model -> BSON and back, describe prints them (commit when not Yes) and notes parameters MDL cannot state; an unknown commit value is refused at exec and by check (MDL-REST03); create or modify carries summary/documentation/object handling of the restated operation; UpdatePublishedRestService carries the stored service-level keys MDL cannot state (keepStoredTopLevel); list markers measured from TestApp", "test": "mdl/executor/cmd_published_rest_params_test.go; mdl/backend/modelsdk/published_rest_write_test.go TestCreatePublishedRestService_WritesParametersAndBindings, TestWithStoredTopLevel; mdl/roundtrip TestTestAppRoundTrip/published_rest_service_Services.OrdersRestApi (allowlist entry struck); mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl (TestApp copy: old binary 16 mx check errors, fixed 0, describe->exec Unchanged twice)", "insight": "A clause that parses and is then ignored is worse than a parse error: the grammar advertised import/export mapping for months while the writer hard-coded them empty. The fastest witness was the round-trip harness's own allowlist entry for the one Studio Pro published REST service in TestApp: removing it printed the whole loss set (bindings, parameter types, markers, authentication) in one diff. Studio Pro's metamodel (ped_get_schema over the MCP tunnel) gave the enum values and defaults: Commit defaults to No there, while mxcli keeps writing Yes when the clause is absent so existing scripts do not churn, and describe prints commit whenever it is not Yes."} diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a9513a23..66b426022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **A published REST operation gets its query and body parameters, its mappings and its commit option** (ako/mxcli#571, mendixlabs/mxcli#1206) — `create published rest service` wrote only the path's `{name}` placeholders, each as a String, so a query or body microflow parameter failed `mx check` with CE0350 and an Integer `{id}` with CE6539; `import mapping`, `export mapping` and `commit` on an operation parsed and were thrown away (CE0350 on the body, CE0354 on a microflow returning an object). The parameters are now derived from the microflow as Studio Pro derives them — named in the path: path parameter; an object or a list: the body; `System.HttpRequest` / `HttpResponse`: none; anything else: query — each with the microflow parameter's type, and the bindings are written. `describe` prints the bindings (`commit` when it is not `Yes`, the value written without the clause) and names in a comment a parameter MDL cannot state (a header, a renamed or described parameter), which `create or modify` and `alter` keep. An unknown commit option is refused by `exec` and `check` (**MDL-REST03**). Executing the `describe` output of a Studio Pro service used to clear its mappings, retype its Integer path parameters, drop its body parameter, set Commit to Yes and turn its Basic and Session authentication off — measured on TestApp's `Services.OrdersRestApi`, 0 `mx check` errors before, 5 after; it now writes nothing. - **`create or modify` of a flow can change an activity's notes, and matches an explicit `on error rollback`** (ako/mxcli#859, part) — a statement that added, reworded or took off an `@annotation` on an activity (or dropped an annotated activity) was refused under the `mdl 1;` header ("the annotations on the replaced … change"; "… carries an annotation, which would be left behind") on every run, and rebuilt the whole flow without it. The stored notes of that activity are now replaced by the ones the statement states; a note shared with another activity (one `describe` gives an `id:`) is still refused. This is what made the Studio Pro-authored PedApp nanoflows `ACT_Feedback_TriggerScreenshotMode` and `ACT_Feedback_UploadImage` unrunnable when their plain description was put under `mdl 1;`: a note's `\r\n` is two characters under mdl 1, so it is a real change, now written once. Separately, `commit $E on error rollback` (any activity with a bare `on error rollback`, which `describe` never prints because it is the stored default) did not match its own stored activity, so it was dropped and written again whenever anything else in the flow changed. - **`describe microflow` prints a Show Page action's title override** (ako/mxcli#869) — `show page M.P with title = '…'` used to describe without its override, so describe → exec dropped it, and taking the override out of a `create or modify microflow` reported "Unchanged" and wrote nothing, under mdl 0 and mdl 1. - **Re-running a `call web service` statement unchanged writes nothing** (ako/mxcli#861) — the call's activity was re-spliced on every run, because its stored raw document holds the element IDs each build mints and was compared byte for byte; it is now compared with those IDs set aside. A stored call that lacks a key mxcli's structured form writes (such as `ErrorHandlingType`), or that refers to its service by ID, now describes as `call web service raw '…'` instead of a structured form that would write a different document. diff --git a/cmd/mxcli/syntax/features_integration.go b/cmd/mxcli/syntax/features_integration.go index 13be374c1..f09c2aaf7 100644 --- a/cmd/mxcli/syntax/features_integration.go +++ b/cmd/mxcli/syntax/features_integration.go @@ -318,9 +318,18 @@ func init() { Keywords: []string{ "create published rest", "publish rest", "rest resource", "rest operation", "microflow", "path parameter", + "query parameter", "body parameter", "import mapping", "export mapping", "commit", "grant access", "revoke access", }, - Syntax: "CREATE [OR MODIFY] PUBLISHED REST SERVICE Module.Name (\n Path: 'rest/api/v1',\n Version: '1.0.0',\n ServiceName: 'My API'\n)\n{\n RESOURCE 'name' {\n GET '' MICROFLOW Module.GetAll;\n GET '{id}' MICROFLOW Module.GetById;\n POST '' MICROFLOW Module.Create;\n }\n};\n\nALTER PUBLISHED REST SERVICE Module.Name SET Version = '2.0.0';\nALTER PUBLISHED REST SERVICE Module.Name ADD RESOURCE 'items' { ... };\nALTER PUBLISHED REST SERVICE Module.Name DROP RESOURCE 'legacy';\nDROP PUBLISHED REST SERVICE Module.Name;", + Syntax: "CREATE [OR MODIFY] PUBLISHED REST SERVICE Module.Name (\n Path: 'rest/api/v1',\n Version: '1.0.0',\n ServiceName: 'My API'\n)\n{\n RESOURCE 'name' {\n GET '' MICROFLOW Module.GetAll;\n GET '{id}' MICROFLOW Module.GetById;\n POST '' MICROFLOW Module.Create\n [IMPORT MAPPING Module.IMM] [EXPORT MAPPING Module.EMM]\n [COMMIT Yes | YesWithoutEvents | No]; -- no COMMIT clause: Yes\n }\n};\n\n" + + "-- Operation parameters come from the microflow, as Studio Pro derives them:\n" + + "-- a parameter named in the path ('{id}') -> path parameter\n" + + "-- an object or a list -> the body\n" + + "-- System.HttpRequest / HttpResponse -> none (the request and response)\n" + + "-- anything else -> query parameter\n" + + "-- each with the microflow parameter's type. Create the microflow first.\n" + + "-- A header parameter, a renamed one or a description set in Studio Pro has\n" + + "-- no MDL spelling: describe notes it, CREATE OR MODIFY / ALTER keep it.\n\nALTER PUBLISHED REST SERVICE Module.Name SET Version = '2.0.0';\nALTER PUBLISHED REST SERVICE Module.Name ADD RESOURCE 'items' { ... };\nALTER PUBLISHED REST SERVICE Module.Name DROP RESOURCE 'legacy';\nDROP PUBLISHED REST SERVICE Module.Name;", Example: "mdl 1;\nCREATE PUBLISHED REST SERVICE Module.OrderAPI (\n Path: 'rest/orders/v1',\n Version: '1.0.0',\n ServiceName: 'Order API'\n)\n{\n RESOURCE 'orders' {\n GET '' MICROFLOW Module.GetAllOrders;\n GET '{id}' MICROFLOW Module.GetOrderById;\n POST '' MICROFLOW Module.CreateOrder;\n DELETE '{id}' MICROFLOW Module.DeleteOrder;\n }\n};\n\nGRANT ACCESS ON PUBLISHED REST SERVICE Module.OrderAPI\n TO Module.User, Module.Admin;", SeeAlso: []string{"rest", "rest.consumed"}, }) diff --git a/docs-site/src/examples/rest-integration.md b/docs-site/src/examples/rest-integration.md index 28dbdcdee..0f8c8309f 100644 --- a/docs-site/src/examples/rest-integration.md +++ b/docs-site/src/examples/rest-integration.md @@ -325,7 +325,37 @@ CREATE PUBLISHED REST SERVICE Module.OrderAPI ( }; ``` -**Operation paths:** Use empty string `''` for the root, `'{paramName}'` for path parameters. Do NOT start or end with `/`. Path parameters must match a microflow parameter name exactly (case-sensitive) — e.g., `'{id}'` requires the microflow to declare `$id: String`. +**Operation paths:** Use empty string `''` for the root, `'{paramName}'` for path parameters. Do NOT start or end with `/`. Path parameters must match a microflow parameter name exactly (case-sensitive) — `'{id}'` binds the microflow's `$id`, whatever its type. + +**Operation parameters** come from the microflow, the way Studio Pro derives them. Create the microflow before the service: + +| Microflow parameter | Operation parameter | +|---|---| +| named in the path (`'{id}'`) | a path parameter | +| an object or a list | the body | +| `System.HttpRequest`, `System.HttpResponse` | none — they are the request and the response | +| anything else | a query parameter | + +Each gets the microflow parameter's type. A header parameter, a renamed parameter or a description set in Studio Pro has no MDL spelling: `describe` notes it in a comment, and `CREATE OR MODIFY` / `ALTER` on that project keep it. + +**Mappings and commit:** a body that is not a file document needs an import mapping, and a microflow returning an object or a list needs an export mapping: + +```sql +mdl 1; +CREATE OR MODIFY PUBLISHED REST SERVICE Module.OrderAPI ( + Path: 'rest/orders/v1', + Version: '1.0.0', + ServiceName: 'Order API' +) +{ + RESOURCE 'orders' { + -- COMMIT: Yes (the default without the clause) | YesWithoutEvents | No + POST '' MICROFLOW Module.PRS_CreateOrder + IMPORT MAPPING Module.IMM_Order EXPORT MAPPING Module.EMM_Order + COMMIT YesWithoutEvents; + } +}; +``` ### Multiple Resources diff --git a/mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl b/mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl new file mode 100644 index 000000000..600bff301 --- /dev/null +++ b/mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl @@ -0,0 +1,94 @@ +mdl 1; +-- ============================================================================ +-- ako/mxcli#571, mendixlabs/mxcli#1206: published REST operation parameters +-- and mapping bindings +-- ============================================================================ +-- +-- Before the fix only the path's {name} placeholders were written, each as a +-- String: the query and body parameters failed mx check with CE0350, an +-- Integer {id} with CE6539. The import mapping, export mapping and commit +-- clauses parsed and were thrown away, so a body that is not a file document +-- failed with CE0350 and a microflow returning an object with CE0354. +-- +-- Usage: +-- mxcli exec mdl-examples/bug-tests/571-published-rest-parameters-and-mappings.mdl -p app.mpr +-- mxcli docker check -p app.mpr -> 0 errors +-- mxcli exec (the same script again) -> writes nothing +-- +-- The operation parameters are derived from each microflow, as Studio Pro +-- derives them: a parameter named in the path is a path parameter, an object +-- or a list is the body, System.HttpRequest / HttpResponse are left out, and +-- anything else is a query parameter, each with the microflow parameter's type. + +create module RestQ; + +create persistent entity RestQ.Upload extends System.FileDocument ( + Label: String(100) +); + +create persistent entity RestQ.Item ( + Code: String(20), + Quantity: Integer +); + +create json structure RestQ.JSON_Item + sample '{"code": "A1", "quantity": 3}'; + +create import mapping RestQ.IMM_Item + with json structure RestQ.JSON_Item +{ + create RestQ.Item { + Code = code, + Quantity = quantity + } +}; + +create export mapping RestQ.EMM_Item + with json structure RestQ.JSON_Item +{ + RestQ.Item { + code = Code, + quantity = Quantity + } +}; + +create microflow RestQ.GetStatus ($orderNumber: String, $count: Integer, $httpRequest: System.HttpRequest) +returns String as $Result +begin + declare $Result String = $orderNumber + ':' + toString($count); + return $Result; +end; + +create microflow RestQ.GetById ($id: Integer, $verbose: Boolean) +returns String as $Result +begin + declare $Result String = toString($id); + return $Result; +end; + +create microflow RestQ.PutFile ($file: RestQ.Upload) +returns String as $Result +begin + declare $Result String = 'ok'; + return $Result; +end; + +create microflow RestQ.PostItem ($item: RestQ.Item) +returns RestQ.Item as $item +begin + return $item; +end; + +create published rest service RestQ.Orders ( + Path: 'rest/orders/v1', + Version: '1.0.0', + ServiceName: 'Orders' +) +{ + resource 'orders' { + get 'status' microflow RestQ.GetStatus; + get 'items/{id}' microflow RestQ.GetById; + post 'upload' microflow RestQ.PutFile; + post 'item' microflow RestQ.PostItem import mapping RestQ.IMM_Item export mapping RestQ.EMM_Item commit YesWithoutEvents; + } +}; diff --git a/mdl/backend/modelsdk/export_level_carry.go b/mdl/backend/modelsdk/export_level_carry.go index 7f1d2c9d6..5bea6df5c 100644 --- a/mdl/backend/modelsdk/export_level_carry.go +++ b/mdl/backend/modelsdk/export_level_carry.go @@ -4,6 +4,8 @@ package modelsdkbackend import ( "fmt" + "sort" + "strings" "go.mongodb.org/mongo-driver/v2/x/bsonx/bsoncore" ) @@ -97,3 +99,69 @@ func withExportLevel(doc []byte, lvl string) ([]byte, error) { } return out, nil } + +// keepStoredTopLevel returns contents with each top-level key in keys set to +// the value the stored unit holds, for a document kind whose statement cannot +// state those properties and whose writer therefore emits constants for them. +// A key the stored unit lacks is left as written; a key only the stored unit +// has is inserted before the first written key that sorts after it, the order +// Studio Pro writes. +func (b *Backend) keepStoredTopLevel(unitID string, contents []byte, keys []string) ([]byte, error) { + if b.reader == nil || unitID == "" || len(contents) == 0 { + return contents, nil + } + stored, err := b.reader.GetRawUnitBytes(unitID) + if err != nil || len(stored) == 0 { + return contents, nil + } + return withStoredTopLevel(contents, stored, keys) +} + +// withStoredTopLevel is keepStoredTopLevel on bytes. +func withStoredTopLevel(doc, stored []byte, keys []string) ([]byte, error) { + carry := map[string]bsoncore.Value{} + for _, k := range keys { + if v, err := bsoncore.Document(stored).LookupErr(k); err == nil { + carry[k] = v + } + } + if len(carry) == 0 { + return doc, nil + } + elems, err := bsoncore.Document(doc).Elements() + if err != nil { + return nil, fmt.Errorf("carry stored properties: %w", err) + } + present := map[string]bool{} + for _, el := range elems { + present[el.Key()] = true + } + var missing []string + for k := range carry { + if !present[k] { + missing = append(missing, k) + } + } + sort.Strings(missing) + idx, out := bsoncore.AppendDocumentStart(nil) + for _, el := range elems { + key := el.Key() + for len(missing) > 0 && missing[0] < key && !strings.HasPrefix(key, "$") { + out = bsoncore.AppendValueElement(out, missing[0], carry[missing[0]]) + missing = missing[1:] + } + if v, ok := carry[key]; ok { + out = bsoncore.AppendValueElement(out, key, v) + continue + } + out = append(out, el...) + } + for _, k := range missing { + out = bsoncore.AppendValueElement(out, k, carry[k]) + } + out, err = bsoncore.AppendDocumentEnd(out, idx) + if err != nil { + return nil, fmt.Errorf("carry stored properties: %w", err) + } + return out, nil +} diff --git a/mdl/backend/modelsdk/integration_read.go b/mdl/backend/modelsdk/integration_read.go index 0ce146a12..49d759174 100644 --- a/mdl/backend/modelsdk/integration_read.go +++ b/mdl/backend/modelsdk/integration_read.go @@ -10,6 +10,7 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/element" genBe "github.com/mendixlabs/mxcli/modelsdk/gen/businessevents" genDb "github.com/mendixlabs/mxcli/modelsdk/gen/databaseconnector" + genDT "github.com/mendixlabs/mxcli/modelsdk/gen/datatypes" genExportMappings "github.com/mendixlabs/mxcli/modelsdk/gen/exportmappings" genImportMappings "github.com/mendixlabs/mxcli/modelsdk/gen/importmappings" genOp "github.com/mendixlabs/mxcli/modelsdk/gen/odatapublish" @@ -427,11 +428,21 @@ func (b *Backend) ListPublishedRestServices() ([]*model.PublishedRestService, er continue } operation := &model.PublishedRestOperation{ - Path: op.Path(), - HTTPMethod: op.HttpMethod(), - Summary: op.Summary(), - Microflow: op.MicroflowQualifiedName(), - Deprecated: op.Deprecated(), + Path: op.Path(), + HTTPMethod: op.HttpMethod(), + Summary: op.Summary(), + Microflow: op.MicroflowQualifiedName(), + Deprecated: op.Deprecated(), + Documentation: op.Documentation(), + ImportMapping: op.ImportMappingQualifiedName(), + ExportMapping: op.ExportMappingQualifiedName(), + Commit: op.Commit(), + ObjectHandlingBackup: op.ObjectHandlingBackup(), + } + for _, pEl := range op.ParametersItems() { + if p, ok := pEl.(*genRest.RestOperationParameter); ok { + operation.OperationParameters = append(operation.OperationParameters, publishedRestParameterFromGen(p)) + } } operation.ID = model.ID(op.ID()) operation.TypeName = "Rest$PublishedRestServiceOperation" @@ -712,3 +723,25 @@ func rawQueryTypeName(q *genDb.DatabaseQuery) string { v, _ := q.Raw().Lookup(dbconnector.TypeKey).StringValueOK() return v } + +// publishedRestParameterFromGen reads one Rest$RestOperationParameter. +func publishedRestParameterFromGen(p *genRest.RestOperationParameter) *model.PublishedRestOperationParameter { + param := &model.PublishedRestOperationParameter{ + Name: p.Name(), + ParameterType: p.ParameterType(), + MicroflowParameter: p.MicroflowParameterQualifiedName(), + Description: p.Description(), + } + if t := p.Type(); t != nil { + param.DataType = strings.TrimSuffix(strings.TrimPrefix(t.TypeName(), "DataTypes$"), "Type") + switch g := t.(type) { + case *genDT.ObjectType: + param.QualifiedName = g.EntityQualifiedName() + case *genDT.ListType: + param.QualifiedName = g.EntityQualifiedName() + case *genDT.EnumerationType: + param.QualifiedName = g.EnumerationQualifiedName() + } + } + return param +} diff --git a/mdl/backend/modelsdk/published_rest_write.go b/mdl/backend/modelsdk/published_rest_write.go index 8370e4e4d..919db33b7 100644 --- a/mdl/backend/modelsdk/published_rest_write.go +++ b/mdl/backend/modelsdk/published_rest_write.go @@ -13,25 +13,27 @@ import ( "github.com/mendixlabs/mxcli/modelsdk/element" mmpr "github.com/mendixlabs/mxcli/modelsdk/mpr" "github.com/mendixlabs/mxcli/modelsdk/property" + "github.com/mendixlabs/mxcli/sdk/microflows" ) func init() { - // Resources / Operations / operation Parameters serialize with the typed-array - // marker 2 (populated keyed by child $Type; empty via MandatoryListMarkers). The - // service's AllowedRoles is a marker-1 reference-string list, AuthenticationTypes - // and Parameters are empty marker-2 lists, and CorsConfiguration is BSON null. - codec.RegisterListMarker("Rest$PublishedRestServiceResource", 2) + // List markers as Studio Pro 11 writes them (measured on ako/TestApp's + // Services.OrdersRestApi): Resources and operation Parameters use 3, + // Operations uses 2, and the service's empty Parameters list is [3]. The + // service's AllowedRoles and AuthenticationTypes are marker-1 string lists, + // and CorsConfiguration is BSON null. + codec.RegisterListMarker("Rest$PublishedRestServiceResource", 3) codec.RegisterListMarker("Rest$PublishedRestServiceOperation", 2) - codec.RegisterListMarker("Rest$RestOperationParameter", 2) + codec.RegisterListMarker("Rest$RestOperationParameter", 3) codec.RegisterTypeDefaults("Rest$PublishedRestService", codec.TypeDefaults{ - MandatoryListMarkers: map[string]int32{"AllowedRoles": 1, "AuthenticationTypes": 2, "Parameters": 2}, + MandatoryListMarkers: map[string]int32{"AllowedRoles": 1, "AuthenticationTypes": 1, "Parameters": 3}, NullFields: []string{"CorsConfiguration"}, }) codec.RegisterTypeDefaults("Rest$PublishedRestServiceResource", codec.TypeDefaults{ MandatoryListMarkers: map[string]int32{"Operations": 2}, }) codec.RegisterTypeDefaults("Rest$PublishedRestServiceOperation", codec.TypeDefaults{ - MandatoryListMarkers: map[string]int32{"Parameters": 2}, + MandatoryListMarkers: map[string]int32{"Parameters": 3}, }) } @@ -73,6 +75,12 @@ func (b *Backend) UpdatePublishedRestService(svc *model.PublishedRestService) er if err != nil { return fmt.Errorf("UpdatePublishedRestService: %w", err) } + // ... and the service-level properties MDL has no spelling for as + // constants too: carry the stored ones (ako/mxcli#571). + contents, err = b.keepStoredTopLevel(string(svc.ID), contents, publishedRestServiceUnauthored) + if err != nil { + return fmt.Errorf("UpdatePublishedRestService: %w", err) + } return b.writer.UpdateRawUnit(string(svc.ID), contents) } @@ -121,6 +129,20 @@ func (b *Backend) UpdatePublishedRestServiceRoles(unitID model.ID, roles []strin return b.writer.UpdateRawUnit(string(unitID), out) } +// publishedRestServiceUnauthored are the Rest$PublishedRestService keys a +// create or modify / alter cannot state: the writer emits a constant for each, +// so a rewrite carries the stored value instead. Without the carry, executing +// the describe output of a Studio Pro service turned its Basic and Session +// authentication off (ako/mxcli#571). +var publishedRestServiceUnauthored = []string{ + "AuthenticationMicroflow", + "AuthenticationTypes", + "CorsConfiguration", + "Documentation", + "Parameters", + "PublicDocumentation", +} + func publishedRestServiceToGen(svc *model.PublishedRestService) element.Element { g := newElem("Rest$PublishedRestService", string(svc.ID)) addStr(g, "Name", svc.Name) @@ -164,26 +186,36 @@ func publishedRestOperationToGen(op *model.PublishedRestOperation) element.Eleme addStr(g, "Microflow", op.Microflow) addStr(g, "Summary", op.Summary) addBool(g, "Deprecated", op.Deprecated) - addStr(g, "Commit", "Yes") - addStr(g, "Documentation", "") - addStr(g, "ExportMapping", "") - addStr(g, "ImportMapping", "") - addStr(g, "ObjectHandlingBackup", "Create") - // Path parameters are auto-extracted from {name} placeholders and wired to the - // matching microflow parameter (Module.Microflow.name) — without that wiring - // mx check raises CE6538 / CE0350. - params := make([]element.Element, 0) - for _, name := range extractPathParams(op.Path) { - p := newElem("Rest$RestOperationParameter", "") - addStr(p, "Name", name) - addPart(p, "Type", newElem("DataTypes$StringType", "")) - addStr(p, "ParameterType", "Path") - mfParam := "" - if op.Microflow != "" { - mfParam = op.Microflow + "." + name + addStr(g, "Commit", orDefault(op.Commit, "Yes")) + addStr(g, "Documentation", op.Documentation) + addStr(g, "ExportMapping", op.ExportMapping) + addStr(g, "ImportMapping", op.ImportMapping) + addStr(g, "ObjectHandlingBackup", orDefault(op.ObjectHandlingBackup, "Create")) + // The executor derives the parameters from the microflow (path, query, body), + // as Studio Pro does. When it could not read the microflow only the path's + // {name} placeholders are known: those are written as String path + // parameters wired to the microflow parameter of that name, since without + // that wiring mx check raises CE6538 / CE0350. + opParams := op.OperationParameters + if len(opParams) == 0 { + for _, name := range op.PathParameterNames() { + mfParam := "" + if op.Microflow != "" { + mfParam = op.Microflow + "." + name + } + opParams = append(opParams, &model.PublishedRestOperationParameter{ + Name: name, ParameterType: "Path", MicroflowParameter: mfParam, DataType: "String", + }) } - addStr(p, "MicroflowParameter", mfParam) - addStr(p, "Description", "") + } + params := make([]element.Element, 0, len(opParams)) + for _, param := range opParams { + p := newElem("Rest$RestOperationParameter", "") + addStr(p, "Name", param.Name) + addPart(p, "Type", publishedRestParameterTypeToGen(param)) + addStr(p, "ParameterType", param.ParameterType) + addStr(p, "MicroflowParameter", param.MicroflowParameter) + addStr(p, "Description", param.Description) params = append(params, p) } if len(params) > 0 { @@ -192,6 +224,49 @@ func publishedRestOperationToGen(op *model.PublishedRestOperation) element.Eleme return g } +// publishedRestParameterTypeToGen builds the DataTypes$* element of an +// operation parameter. Long is not among an operation parameter's types +// (Studio Pro 11.14's schema for Rest$RestOperationParameter.type): it is +// written as Integer, as microflowDataTypeToGen writes it. +func publishedRestParameterTypeToGen(p *model.PublishedRestOperationParameter) element.Element { + switch p.DataType { + case "Float": + return newElem("DataTypes$FloatType", "") + case "Empty": + return newElem("DataTypes$EmptyType", "") + case "Unknown": + return newElem("DataTypes$UnknownType", "") + } + return microflowDataTypeToGen(publishedRestParameterDataType(p)) +} + +// publishedRestParameterDataType is the microflow data type an operation +// parameter carries. +func publishedRestParameterDataType(p *model.PublishedRestOperationParameter) microflows.DataType { + switch p.DataType { + case "Boolean": + return µflows.BooleanType{} + case "Integer", "Long": + return µflows.IntegerType{} + case "Decimal": + return µflows.DecimalType{} + case "DateTime", "Date": + return µflows.DateTimeType{} + case "Binary": + return µflows.BinaryType{} + case "Enumeration": + return µflows.EnumerationType{EnumerationQualifiedName: p.QualifiedName} + case "Object": + return µflows.ObjectType{EntityQualifiedName: p.QualifiedName} + case "List": + return µflows.ListType{EntityQualifiedName: p.QualifiedName} + case "Void": + return nil + default: + return µflows.StringType{} + } +} + // addByNameRefList adds a marker-1 reference-string list property (qualified // names), the form Mendix uses for AllowedRoles / AllowedModuleRoles. func addByNameRefList(b *element.Base, name, targetType string, qnames []string) { @@ -202,24 +277,6 @@ func addByNameRefList(b *element.Base, name, targetType string, qnames []string) } } -// extractPathParams returns parameter names from {param} placeholders in a path. -func extractPathParams(path string) []string { - var names []string - for { - start := strings.Index(path, "{") - if start < 0 { - break - } - end := strings.Index(path[start:], "}") - if end < 0 { - break - } - names = append(names, path[start+1:start+end]) - path = path[start+end+1:] - } - return names -} - // httpMethodToMendix converts an HTTP method name to Mendix casing. func httpMethodToMendix(method string) string { switch strings.ToUpper(method) { diff --git a/mdl/backend/modelsdk/published_rest_write_test.go b/mdl/backend/modelsdk/published_rest_write_test.go index 3a0781d1d..6c4766bc6 100644 --- a/mdl/backend/modelsdk/published_rest_write_test.go +++ b/mdl/backend/modelsdk/published_rest_write_test.go @@ -3,8 +3,11 @@ package modelsdkbackend import ( + "strings" "testing" + "go.mongodb.org/mongo-driver/bson" + "github.com/mendixlabs/mxcli/model" ) @@ -72,3 +75,125 @@ func TestCreatePublishedRestService_RoundTrip(t *testing.T) { t.Errorf("operations = %d, want 2", len(got.Resources[0].Operations)) } } + +// TestCreatePublishedRestService_WritesParametersAndBindings is ako/mxcli#571 / +// mendixlabs/mxcli#1206 at the storage layer: the operation's query and body +// parameters, each with its own type, its mapping bindings and its commit +// option are written and read back. Before, only String path parameters were +// written, and ExportMapping / ImportMapping / Commit were constants. +func TestCreatePublishedRestService_WritesParametersAndBindings(t *testing.T) { + proj := copyFixture(t) + b := New() + if err := b.Connect(proj); err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = b.Disconnect() }) + mod, err := b.GetModuleByName("MyFirstModule") + if err != nil || mod == nil { + t.Fatalf("GetModuleByName: %v", err) + } + params := []*model.PublishedRestOperationParameter{ + {Name: "id", ParameterType: "Path", MicroflowParameter: "MyFirstModule.ACT_Put.id", DataType: "Integer"}, + {Name: "verbose", ParameterType: "Query", MicroflowParameter: "MyFirstModule.ACT_Put.verbose", DataType: "Boolean"}, + {Name: "X-Mode", ParameterType: "Header", MicroflowParameter: "MyFirstModule.ACT_Put.mode", DataType: "Enumeration", QualifiedName: "MyFirstModule.Mode", Description: "mode"}, + {Name: "body", ParameterType: "Body", MicroflowParameter: "MyFirstModule.ACT_Put.body", DataType: "Object", QualifiedName: "MyFirstModule.Thing"}, + } + svc := &model.PublishedRestService{ + ContainerID: mod.ID, Name: "ZzParams", Path: "rest/zzp/v1", + Resources: []*model.PublishedRestResource{{ + Name: "things", + Operations: []*model.PublishedRestOperation{ + { + Path: "{id}", HTTPMethod: "PUT", Microflow: "MyFirstModule.ACT_Put", + ImportMapping: "MyFirstModule.IMM_Thing", ExportMapping: "MyFirstModule.EMM_Thing", + Commit: "YesWithoutEvents", ObjectHandlingBackup: "Error", + OperationParameters: params, + }, + // No derived parameters: the path's placeholder as a String (the + // writer's fallback when the microflow could not be read). + {Path: "{key}", HTTPMethod: "GET", Microflow: "MyFirstModule.ACT_Get"}, + }, + }}, + } + if err := b.CreatePublishedRestService(svc); err != nil { + t.Fatalf("CreatePublishedRestService: %v", err) + } + + b2 := New() + if err := b2.Connect(proj); err != nil { + t.Fatalf("reconnect: %v", err) + } + t.Cleanup(func() { _ = b2.Disconnect() }) + all, err := b2.ListPublishedRestServices() + if err != nil { + t.Fatalf("ListPublishedRestServices: %v", err) + } + var got *model.PublishedRestService + for _, s := range all { + if s.Name == "ZzParams" { + got = s + } + } + if got == nil { + t.Fatal("ZzParams not found") + } + op := got.Resources[0].Operations[0] + if op.ImportMapping != "MyFirstModule.IMM_Thing" || op.ExportMapping != "MyFirstModule.EMM_Thing" || + op.Commit != "YesWithoutEvents" || op.ObjectHandlingBackup != "Error" { + t.Errorf("bindings read back as import %q export %q commit %q handling %q", + op.ImportMapping, op.ExportMapping, op.Commit, op.ObjectHandlingBackup) + } + if len(op.OperationParameters) != len(params) { + t.Fatalf("parameters = %d, want %d: %+v", len(op.OperationParameters), len(params), op.OperationParameters) + } + for i, want := range params { + if g := *op.OperationParameters[i]; g != *want { + t.Errorf("parameter %d = %+v, want %+v", i, g, *want) + } + } + fallback := got.Resources[0].Operations[1] + if len(fallback.OperationParameters) != 1 || *fallback.OperationParameters[0] != (model.PublishedRestOperationParameter{ + Name: "key", ParameterType: "Path", MicroflowParameter: "MyFirstModule.ACT_Get.key", DataType: "String", + }) { + t.Errorf("fallback parameters = %+v", fallback.OperationParameters) + } + if fallback.Commit != "Yes" || fallback.ObjectHandlingBackup != "Create" { + t.Errorf("defaults = commit %q handling %q, want Yes / Create", fallback.Commit, fallback.ObjectHandlingBackup) + } +} + +// TestWithStoredTopLevel carries the stored values of the listed keys, +// inserts a stored-only key in sorted position, and leaves every other key as +// written. The control: an unlisted key keeps the written value. +func TestWithStoredTopLevel(t *testing.T) { + written, _ := bson.Marshal(bson.D{ + {Key: "$ID", Value: "x"}, {Key: "AuthenticationTypes", Value: bson.A{int32(1)}}, + {Key: "Name", Value: "new"}, {Key: "Version", Value: "2"}, + }) + stored, _ := bson.Marshal(bson.D{ + {Key: "$ID", Value: "x"}, {Key: "AuthenticationTypes", Value: bson.A{int32(1), "Basic"}}, + {Key: "Name", Value: "old"}, {Key: "PublicDocumentation", Value: ""}, {Key: "Version", Value: "1"}, + }) + out, err := withStoredTopLevel(written, stored, []string{"AuthenticationTypes", "PublicDocumentation", "Missing"}) + if err != nil { + t.Fatal(err) + } + var d bson.D + if err := bson.Unmarshal(out, &d); err != nil { + t.Fatal(err) + } + var keys []string + for _, e := range d { + keys = append(keys, e.Key) + } + if got := strings.Join(keys, ","); got != "$ID,AuthenticationTypes,Name,PublicDocumentation,Version" { + t.Errorf("keys = %s", got) + } + m := d.Map() + if a, ok := m["AuthenticationTypes"].(bson.A); !ok || len(a) != 2 || a[1] != "Basic" { + t.Errorf("AuthenticationTypes = %v, want the stored [1 Basic]", m["AuthenticationTypes"]) + } + if m["Name"] != "new" || m["Version"] != "2" { + t.Errorf("unlisted keys changed: Name %v Version %v", m["Name"], m["Version"]) + } +} diff --git a/mdl/executor/cmd_published_rest.go b/mdl/executor/cmd_published_rest.go index 40533f572..6533f0288 100644 --- a/mdl/executor/cmd_published_rest.go +++ b/mdl/executor/cmd_published_rest.go @@ -11,6 +11,7 @@ import ( "github.com/mendixlabs/mxcli/mdl/ast" mdlerrors "github.com/mendixlabs/mxcli/mdl/errors" "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" ) // listPublishedRestServices handles SHOW PUBLISHED REST SERVICES [IN module] command. @@ -135,8 +136,11 @@ func describePublishedRestService(ctx *ExecContext, name ast.QualifiedName) erro if op.Path != "" { opPath = " " + mdlQuoted(op.Path) } - fmt.Fprintf(ctx.Output, " %s%s%s%s;%s\n", - strings.ToLower(op.HTTPMethod), opPath, mf, deprecated, summary) + fmt.Fprintf(ctx.Output, " %s%s%s%s%s;%s\n", + strings.ToLower(op.HTTPMethod), opPath, mf, deprecated, publishedRestBindingClauses(op), summary) + for _, note := range publishedRestParameterNotes(op) { + fmt.Fprintf(ctx.Output, " -- %s\n", note) + } } fmt.Fprintln(ctx.Output, " }") } @@ -159,6 +163,55 @@ func describePublishedRestService(ctx *ExecContext, name ast.QualifiedName) erro return mdlerrors.NewNotFound("published rest service", name.String()) } +// publishedRestBindingClauses prints an operation's mapping bindings and its +// commit option, in the grammar's order. Commit is printed when it is not +// "Yes", the value exec writes when the statement has no commit clause. +func publishedRestBindingClauses(op *model.PublishedRestOperation) string { + var b strings.Builder + if op.ImportMapping != "" { + b.WriteString(" import mapping " + op.ImportMapping) + } + if op.ExportMapping != "" { + b.WriteString(" export mapping " + op.ExportMapping) + } + if op.Commit != "" && op.Commit != "Yes" { + b.WriteString(" commit " + op.Commit) + } + return b.String() +} + +// publishedRestParameterNotes names the operation parameters MDL cannot state: +// it derives every parameter from the microflow, so a header or form +// parameter, a parameter renamed away from its microflow parameter, or one +// with a description has no spelling. create or modify on the same project +// keeps them; a fresh create derives the parameter again. +func publishedRestParameterNotes(op *model.PublishedRestOperation) []string { + var notes []string + for _, p := range op.OperationParameters { + bound := p.MicroflowParameter + if i := strings.LastIndex(bound, "."); i >= 0 { + bound = bound[i+1:] + } + var why []string + switch p.ParameterType { + case "Path", "Query", "Body": + default: + why = append(why, strings.ToLower(p.ParameterType)+" parameter") + } + if bound != p.Name { + why = append(why, fmt.Sprintf("bound to $%s", bound)) + } + if p.Description != "" { + why = append(why, "description "+mdlQuoted(strings.ReplaceAll(p.Description, "\n", " "))) + } + if len(why) > 0 { + notes = append(notes, fmt.Sprintf("parameter %s: %s (not expressible in MDL; kept by create or modify on this project)", + p.Name, strings.Join(why, ", "))) + } + } + return notes +} + // findPublishedRestService looks up a published REST service by module and name. func findPublishedRestService(ctx *ExecContext, moduleName, name string) (*model.PublishedRestService, error) { @@ -234,21 +287,16 @@ func execCreatePublishedRestService(ctx *ExecContext, s *ast.CreatePublishedRest } for _, resDef := range s.Resources { - resource := &model.PublishedRestResource{ - Name: resDef.Name, - } - for _, opDef := range resDef.Operations { - op := &model.PublishedRestOperation{ - HTTPMethod: opDef.HTTPMethod, - Path: opDef.Path, - Microflow: opDef.Microflow.String(), - Summary: "", - Deprecated: opDef.Deprecated, - } - resource.Operations = append(resource.Operations, op) + resource, err := astResourceDefToModel(resDef) + if err != nil { + return err } svc.Resources = append(svc.Resources, resource) } + if existing != nil { + carryStoredOperations(svc, existing) + } + deriveOperationParameters(ctx, svc) if existing != nil { if s.Folder == "" { @@ -309,17 +357,212 @@ func execDropPublishedRestService(ctx *ExecContext, s *ast.DropPublishedRestServ // astResourceDefToModel converts an AST PublishedRestResourceDef to the // runtime model type used by the writer. -func astResourceDefToModel(def *ast.PublishedRestResourceDef) *model.PublishedRestResource { +func astResourceDefToModel(def *ast.PublishedRestResourceDef) (*model.PublishedRestResource, error) { resource := &model.PublishedRestResource{Name: def.Name} for _, opDef := range def.Operations { + commit, err := publishedRestCommit(opDef.Commit) + if err != nil { + return nil, mdlerrors.NewValidation(fmt.Sprintf("resource '%s', operation %s %s: %v", + def.Name, opDef.HTTPMethod, opDef.Path, err)) + } resource.Operations = append(resource.Operations, &model.PublishedRestOperation{ - HTTPMethod: opDef.HTTPMethod, - Path: opDef.Path, - Microflow: opDef.Microflow.String(), - Deprecated: opDef.Deprecated, + HTTPMethod: opDef.HTTPMethod, + Path: opDef.Path, + Microflow: opDef.Microflow.String(), + Deprecated: opDef.Deprecated, + ImportMapping: opDef.ImportMapping, + ExportMapping: opDef.ExportMapping, + Commit: commit, }) } - return resource + return resource, nil +} + +// publishedRestCommitValues are the values of Rest$PublishedRestServiceOperation.Commit. +var publishedRestCommitValues = []string{"Yes", "YesWithoutEvents", "No"} + +// publishedRestCommit returns the stored spelling of an operation's commit +// clause ("" when the statement has none). Any other value used to parse and +// be thrown away; it is refused, since there is nothing correct to write. +func publishedRestCommit(v string) (string, error) { + if v == "" { + return "", nil + } + for _, c := range publishedRestCommitValues { + if strings.EqualFold(v, c) { + return c, nil + } + } + return "", fmt.Errorf("commit %s is not a commit option (allowed: %s)", v, strings.Join(publishedRestCommitValues, ", ")) +} + +// carryStoredOperations copies onto each operation a statement declares what +// MDL cannot state about it — summary, documentation, the import mapping's +// object handling, and the stored parameters (their names, kinds and +// descriptions) — from the stored operation it restates. An operation is +// matched by its resource's name, its method and its path, in order; one +// without a match is new and gets the defaults. +func carryStoredOperations(svc, stored *model.PublishedRestService) { + pool := map[string][]*model.PublishedRestOperation{} + key := func(res string, op *model.PublishedRestOperation) string { + return res + "\x00" + strings.ToUpper(op.HTTPMethod) + "\x00" + op.Path + } + for _, res := range stored.Resources { + for _, op := range res.Operations { + k := key(res.Name, op) + pool[k] = append(pool[k], op) + } + } + for _, res := range svc.Resources { + for _, op := range res.Operations { + k := key(res.Name, op) + if len(pool[k]) == 0 { + continue + } + old := pool[k][0] + pool[k] = pool[k][1:] + op.Summary = old.Summary + op.Documentation = old.Documentation + op.ObjectHandlingBackup = old.ObjectHandlingBackup + if old.Microflow == op.Microflow { + op.OperationParameters = old.OperationParameters + } + } + } +} + +// deriveOperationParameters gives every operation the parameters Studio Pro +// derives from its microflow (ako/mxcli#571, mendixlabs/mxcli#1206): a +// parameter named in the path is a path parameter, an object or a list is the +// body, System.HttpRequest and System.HttpResponse are the request and the +// response themselves, and anything else is a query parameter — each with the +// microflow parameter's type. Without the query and body parameters mx check +// reports CE0350, and a String path parameter bound to an Integer is CE6539. +// +// The operation's stored parameters (op.OperationParameters on entry) win for +// the microflow parameter they bind: Studio Pro lets a parameter be renamed, +// described or turned into a header, and MDL has no spelling for that, so a +// rewrite keeps it. Only the type follows the microflow, and a path parameter +// follows the path. +func deriveOperationParameters(ctx *ExecContext, svc *model.PublishedRestService) { + var microflowsByName map[string]*microflows.Microflow + for _, resource := range svc.Resources { + for _, op := range resource.Operations { + if op.Microflow == "" { + continue + } + if microflowsByName == nil { + microflowsByName = liveMicroflowsByQualifiedName(ctx) + } + mf := microflowsByName[op.Microflow] + if mf == nil { + if len(op.OperationParameters) == 0 && !ctx.Quiet { + fmt.Fprintf(ctx.Output, "Warning: microflow %s not found, so operation %s %s gets only its path parameters, as String -- "+ + "create the microflow before the service, or its other parameters fail mx check with CE0350\n", + op.Microflow, strings.ToUpper(op.HTTPMethod), op.Path) + } + continue + } + op.OperationParameters = mergeOperationParameters(op.OperationParameters, operationParametersOf(op.Microflow, mf, op.PathParameterNames())) + } + } +} + +// liveMicroflowsByQualifiedName indexes the project's live microflows (an +// excluded twin never shadows the live one, #914). +func liveMicroflowsByQualifiedName(ctx *ExecContext) map[string]*microflows.Microflow { + out := map[string]*microflows.Microflow{} + all, err := ctx.Backend.ListMicroflows() + if err != nil { + return out + } + h, err := getHierarchy(ctx) + if err != nil { + return out + } + for _, mf := range all { + qn := h.GetQualifiedName(mf.ContainerID, mf.Name) + if prev, ok := out[qn]; ok && !prev.Excluded { + continue + } + out[qn] = mf + } + return out +} + +// operationParametersOf maps a microflow's parameters to operation parameters +// by the rule Studio Pro applies. +func operationParametersOf(mfName string, mf *microflows.Microflow, pathNames []string) []*model.PublishedRestOperationParameter { + inPath := make(map[string]bool, len(pathNames)) + for _, name := range pathNames { + inPath[name] = true + } + var params []*model.PublishedRestOperationParameter + for _, p := range mf.Parameters { + param := &model.PublishedRestOperationParameter{ + Name: p.Name, + ParameterType: "Query", + MicroflowParameter: mfName + "." + p.Name, + } + if p.Type != nil { + param.DataType = p.Type.GetTypeName() + } + switch t := p.Type.(type) { + case *microflows.ObjectType: + if t.EntityQualifiedName == "System.HttpRequest" || t.EntityQualifiedName == "System.HttpResponse" { + continue + } + param.ParameterType, param.DataType, param.QualifiedName = "Body", "Object", t.EntityQualifiedName + case *microflows.ListType: + param.ParameterType, param.DataType, param.QualifiedName = "Body", "List", t.EntityQualifiedName + case *microflows.EnumerationType: + param.DataType, param.QualifiedName = "Enumeration", t.EnumerationQualifiedName + } + if inPath[p.Name] { + param.ParameterType = "Path" + } + params = append(params, param) + } + return params +} + +// mergeOperationParameters keeps each stored parameter whose microflow +// parameter the microflow still has — in stored order, with the derived type, +// and the derived kind where either side is a path parameter — drops the ones +// it no longer has, keeps an unbound one as stored, and appends the derived +// parameters no stored one binds. +func mergeOperationParameters(stored, derived []*model.PublishedRestOperationParameter) []*model.PublishedRestOperationParameter { + byBinding := make(map[string]*model.PublishedRestOperationParameter, len(derived)) + for _, d := range derived { + byBinding[d.MicroflowParameter] = d + } + used := map[string]bool{} + var out []*model.PublishedRestOperationParameter + for _, sp := range stored { + if sp.MicroflowParameter == "" { + out = append(out, sp) + continue + } + d, ok := byBinding[sp.MicroflowParameter] + if !ok || used[sp.MicroflowParameter] { + continue + } + used[sp.MicroflowParameter] = true + kept := *sp + if d.DataType != "" { // "" is a type the reader could not read: keep the stored one + kept.DataType, kept.QualifiedName = d.DataType, d.QualifiedName + } + if d.ParameterType == "Path" || sp.ParameterType == "Path" { + kept.Name, kept.ParameterType = d.Name, d.ParameterType + } + out = append(out, &kept) + } + for _, d := range derived { + if !used[d.MicroflowParameter] { + out = append(out, d) + } + } + return out } // execAlterPublishedRestService applies SET / ADD RESOURCE / DROP RESOURCE @@ -363,7 +606,11 @@ func execAlterPublishedRestService(ctx *ExecContext, s *ast.AlterPublishedRestSe return mdlerrors.NewAlreadyExistsMsg("resource", a.Resource.Name, fmt.Sprintf("resource '%s' already exists on %s.%s", a.Resource.Name, s.Name.Module, s.Name.Name)) } } - svc.Resources = append(svc.Resources, astResourceDefToModel(a.Resource)) + resource, err := astResourceDefToModel(a.Resource) + if err != nil { + return err + } + svc.Resources = append(svc.Resources, resource) case *ast.PublishedRestDropResourceAction: idx := -1 @@ -383,6 +630,7 @@ func execAlterPublishedRestService(ctx *ExecContext, s *ast.AlterPublishedRestSe } } + deriveOperationParameters(ctx, svc) if err := ctx.Backend.UpdatePublishedRestService(svc); err != nil { return mdlerrors.NewBackend("alter published rest service", err) } diff --git a/mdl/executor/cmd_published_rest_params_test.go b/mdl/executor/cmd_published_rest_params_test.go new file mode 100644 index 000000000..011a5c0fd --- /dev/null +++ b/mdl/executor/cmd_published_rest_params_test.go @@ -0,0 +1,314 @@ +// 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/microflows" +) + +// publishedRestFixture is a module RestQ holding the microflows of +// mendixlabs/mxcli#1206's repro, with the given stored services. +type publishedRestFixture struct { + mod *model.Module + stored []*model.PublishedRestService + created *model.PublishedRestService + updated *model.PublishedRestService +} + +func newPublishedRestFixture(t *testing.T, stored ...*model.PublishedRestService) (*publishedRestFixture, *ExecContext, *strings.Builder) { + t.Helper() + f := &publishedRestFixture{mod: mkModule("RestQ"), stored: stored} + for _, s := range stored { + s.ContainerID = f.mod.ID + } + param := func(name string, dt microflows.DataType) *microflows.MicroflowParameter { + return µflows.MicroflowParameter{Name: name, Type: dt} + } + mf := func(name string, params ...*microflows.MicroflowParameter) *microflows.Microflow { + return µflows.Microflow{ + BaseElement: model.BaseElement{ID: nextID("mf")}, + ContainerID: f.mod.ID, Name: name, Parameters: params, + } + } + mfs := []*microflows.Microflow{ + mf("GetStatus", + param("orderNumber", µflows.StringType{}), + param("count", µflows.IntegerType{}), + param("httpRequest", µflows.ObjectType{EntityQualifiedName: "System.HttpRequest"})), + mf("GetById", + param("id", µflows.IntegerType{}), + param("verbose", µflows.BooleanType{})), + mf("PutFile", param("file", µflows.ObjectType{EntityQualifiedName: "RestQ.Upload"})), + mf("PutMany", param("items", µflows.ListType{EntityQualifiedName: "RestQ.Item"})), + } + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{f.mod}, nil }, + ListMicroflowsFunc: func() ([]*microflows.Microflow, error) { + return mfs, nil + }, + ListPublishedRestServicesFunc: func() ([]*model.PublishedRestService, error) { return f.stored, nil }, + CreatePublishedRestServiceFunc: func(svc *model.PublishedRestService) error { + f.created = svc + return nil + }, + UpdatePublishedRestServiceFunc: func(svc *model.PublishedRestService) error { + f.updated = svc + return nil + }, + } + h := mkHierarchy(f.mod) + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + out := &strings.Builder{} + ctx.Output = out + return f, ctx, out +} + +func execPublishedRest(t *testing.T, ctx *ExecContext, src string) error { + t.Helper() + prog := parseMDL(t, src) + for _, stmt := range prog.Statements { + var err error + switch s := stmt.(type) { + case *ast.CreatePublishedRestServiceStmt: + err = execCreatePublishedRestService(ctx, s) + case *ast.AlterPublishedRestServiceStmt: + err = execAlterPublishedRestService(ctx, s) + default: + t.Fatalf("unexpected statement %T", stmt) + } + if err != nil { + return err + } + } + return nil +} + +func paramsString(op *model.PublishedRestOperation) string { + var parts []string + for _, p := range op.OperationParameters { + s := p.ParameterType + " " + p.Name + ":" + p.DataType + if p.QualifiedName != "" { + s += "(" + p.QualifiedName + ")" + } + s += "->" + p.MicroflowParameter + parts = append(parts, s) + } + return strings.Join(parts, "; ") +} + +// TestCreatePublishedRestService_DerivesParameters is mendixlabs/mxcli#1206: +// only the path's placeholders were written, each a String, so every query and +// body parameter failed mx check with CE0350 and an Integer {id} with CE6539. +func TestCreatePublishedRestService_DerivesParameters(t *testing.T) { + f, ctx, _ := newPublishedRestFixture(t) + assertNoError(t, execPublishedRest(t, ctx, `create published rest service RestQ.Orders (Path: 'rest/orders/v1') { + resource 'orders' { + get 'status' microflow RestQ.GetStatus; + get 'items/{id}' microflow RestQ.GetById; + post 'upload' microflow RestQ.PutFile; + put 'many' microflow RestQ.PutMany; + } +};`)) + if f.created == nil { + t.Fatal("nothing created") + } + ops := f.created.Resources[0].Operations + want := []string{ + "Query orderNumber:String->RestQ.GetStatus.orderNumber; Query count:Integer->RestQ.GetStatus.count", + "Path id:Integer->RestQ.GetById.id; Query verbose:Boolean->RestQ.GetById.verbose", + "Body file:Object(RestQ.Upload)->RestQ.PutFile.file", + "Body items:List(RestQ.Item)->RestQ.PutMany.items", + } + for i, w := range want { + if got := paramsString(ops[i]); got != w { + t.Errorf("operation %d parameters:\n got %s\nwant %s", i, got, w) + } + } +} + +// TestCreatePublishedRestService_WritesMappingBindings is ako/mxcli#571: the +// import mapping, export mapping and commit clauses parsed and were thrown away. +func TestCreatePublishedRestService_WritesMappingBindings(t *testing.T) { + f, ctx, _ := newPublishedRestFixture(t) + assertNoError(t, execPublishedRest(t, ctx, `create published rest service RestQ.Orders (Path: 'rest/orders/v1') { + resource 'orders' { + post 'upload' microflow RestQ.PutFile import mapping RestQ.IMM_Upload export mapping "RestQ"."EMM_Result" commit yeswithoutevents; + get 'status' microflow RestQ.GetStatus; + } +};`)) + op := f.created.Resources[0].Operations[0] + if op.ImportMapping != "RestQ.IMM_Upload" || op.ExportMapping != "RestQ.EMM_Result" || op.Commit != "YesWithoutEvents" { + t.Errorf("bindings = import %q export %q commit %q, want RestQ.IMM_Upload, RestQ.EMM_Result, YesWithoutEvents", + op.ImportMapping, op.ExportMapping, op.Commit) + } + if other := f.created.Resources[0].Operations[1]; other.ImportMapping != "" || other.ExportMapping != "" || other.Commit != "" { + t.Errorf("an operation without clauses got bindings: %+v", other) + } +} + +// TestCreatePublishedRestService_RefusesUnknownCommit: `commit Maybe` parsed +// and was thrown away; there is nothing correct to write, so it is refused by +// exec and by check (MDL-REST03). +func TestCreatePublishedRestService_RefusesUnknownCommit(t *testing.T) { + src := `create published rest service RestQ.Orders (Path: 'rest/orders/v1') { + resource 'orders' { post 'upload' microflow RestQ.PutFile import mapping RestQ.IMM commit Maybe; } +};` + f, ctx, _ := newPublishedRestFixture(t) + err := execPublishedRest(t, ctx, src) + if err == nil || !strings.Contains(err.Error(), "commit Maybe") { + t.Fatalf("exec error = %v, want a refusal naming commit Maybe", err) + } + if f.created != nil { + t.Error("the service was written despite the refusal") + } + v := ValidatePublishedRestCommit(parseMDL(t, src)) + if len(v) != 1 || v[0].RuleID != "MDL-REST03" { + t.Errorf("check = %+v, want one MDL-REST03", v) + } + if v := ValidatePublishedRestCommit(parseMDL(t, strings.Replace(src, "Maybe", "No", 1))); len(v) != 0 { + t.Errorf("control: commit No flagged: %+v", v) + } +} + +// TestCreateOrModifyPublishedRestService_KeepsStoredParameters: a parameter +// Studio Pro lets the user rename, describe or turn into a header has no MDL +// spelling; create or modify of the same operation keeps it, as it keeps the +// summary, documentation and object handling, while the type follows the +// microflow and a new microflow parameter is derived. +func TestCreateOrModifyPublishedRestService_KeepsStoredParameters(t *testing.T) { + stored := &model.PublishedRestService{ + BaseElement: model.BaseElement{ID: nextID("prs")}, + Name: "Orders", + Path: "rest/orders/v1", + Resources: []*model.PublishedRestResource{{ + Name: "orders", + Operations: []*model.PublishedRestOperation{{ + HTTPMethod: "Get", Path: "status", Microflow: "RestQ.GetStatus", + Summary: "Order status", Documentation: "docs", ObjectHandlingBackup: "Error", Commit: "No", + OperationParameters: []*model.PublishedRestOperationParameter{ + {Name: "X-Count", ParameterType: "Header", MicroflowParameter: "RestQ.GetStatus.count", DataType: "String", Description: "how many"}, + }, + }}, + }}, + } + f, ctx, out := newPublishedRestFixture(t, stored) + assertNoError(t, execPublishedRest(t, ctx, `create or modify published rest service RestQ.Orders (Path: 'rest/orders/v1') { + resource 'orders' { get 'status' microflow RestQ.GetStatus commit No; } +};`)) + if f.updated == nil { + t.Fatalf("nothing updated; output:\n%s", out) + } + op := f.updated.Resources[0].Operations[0] + if got, want := paramsString(op), "Header X-Count:Integer->RestQ.GetStatus.count; Query orderNumber:String->RestQ.GetStatus.orderNumber"; got != want { + t.Errorf("parameters:\n got %s\nwant %s", got, want) + } + if op.OperationParameters[0].Description != "how many" { + t.Errorf("description lost: %+v", op.OperationParameters[0]) + } + if op.Summary != "Order status" || op.Documentation != "docs" || op.ObjectHandlingBackup != "Error" { + t.Errorf("unstated operation properties not carried: %+v", op) + } +} + +// TestAlterPublishedRestService_DerivesAddedOperations: ALTER writes every +// operation again, so an added resource derives its parameters and keeps its +// bindings, and the untouched operations keep theirs. +func TestAlterPublishedRestService_DerivesAddedOperations(t *testing.T) { + stored := &model.PublishedRestService{ + BaseElement: model.BaseElement{ID: nextID("prs")}, + Name: "Orders", + Path: "rest/orders/v1", + Resources: []*model.PublishedRestResource{{ + Name: "orders", + Operations: []*model.PublishedRestOperation{{ + HTTPMethod: "Post", Path: "upload", Microflow: "RestQ.PutFile", + ImportMapping: "RestQ.IMM_Upload", Commit: "No", + OperationParameters: []*model.PublishedRestOperationParameter{ + {Name: "file", ParameterType: "Body", MicroflowParameter: "RestQ.PutFile.file", DataType: "Object", QualifiedName: "RestQ.Upload"}, + }, + }}, + }}, + } + f, ctx, _ := newPublishedRestFixture(t, stored) + assertNoError(t, execPublishedRest(t, ctx, `alter published rest service RestQ.Orders + add resource 'items' { get '{id}' microflow RestQ.GetById export mapping RestQ.EMM_Item; };`)) + if f.updated == nil { + t.Fatal("nothing updated") + } + kept := f.updated.Resources[0].Operations[0] + if kept.ImportMapping != "RestQ.IMM_Upload" || kept.Commit != "No" || paramsString(kept) != "Body file:Object(RestQ.Upload)->RestQ.PutFile.file" { + t.Errorf("untouched operation changed: %+v %s", kept, paramsString(kept)) + } + added := f.updated.Resources[1].Operations[0] + if added.ExportMapping != "RestQ.EMM_Item" { + t.Errorf("added operation export mapping = %q", added.ExportMapping) + } + if got, want := paramsString(added), "Path id:Integer->RestQ.GetById.id; Query verbose:Boolean->RestQ.GetById.verbose"; got != want { + t.Errorf("added operation parameters:\n got %s\nwant %s", got, want) + } +} + +// TestCreatePublishedRestService_MissingMicroflowWarns: without the microflow +// only the path is known; the operation keeps today's path-only parameters and +// exec says what mx check will report. +func TestCreatePublishedRestService_MissingMicroflowWarns(t *testing.T) { + f, ctx, out := newPublishedRestFixture(t) + assertNoError(t, execPublishedRest(t, ctx, `create published rest service RestQ.Orders (Path: 'rest/orders/v1') { + resource 'orders' { get '{id}' microflow RestQ.NotYet; } +};`)) + if len(f.created.Resources[0].Operations[0].OperationParameters) != 0 { + t.Errorf("parameters derived for a missing microflow: %s", paramsString(f.created.Resources[0].Operations[0])) + } + if !strings.Contains(out.String(), "microflow RestQ.NotYet not found") { + t.Errorf("no warning; output:\n%s", out) + } +} + +// TestDescribePublishedRestService_PrintsBindings: describe prints the +// bindings exec now writes, so executing its output restates them, and names +// the parameters MDL cannot state. +func TestDescribePublishedRestService_PrintsBindings(t *testing.T) { + stored := &model.PublishedRestService{ + BaseElement: model.BaseElement{ID: nextID("prs")}, + Name: "Orders", + Path: "rest/orders/v1", + Resources: []*model.PublishedRestResource{{ + Name: "orders", + Operations: []*model.PublishedRestOperation{ + { + HTTPMethod: "Post", Path: "upload", Microflow: "RestQ.PutFile", + ImportMapping: "RestQ.IMM_Upload", ExportMapping: "RestQ.EMM_Result", Commit: "YesWithoutEvents", + }, + { + HTTPMethod: "Get", Path: "status", Microflow: "RestQ.GetStatus", Commit: "Yes", + OperationParameters: []*model.PublishedRestOperationParameter{ + {Name: "X-Count", ParameterType: "Header", MicroflowParameter: "RestQ.GetStatus.count", DataType: "Integer"}, + {Name: "orderNumber", ParameterType: "Query", MicroflowParameter: "RestQ.GetStatus.orderNumber", DataType: "String"}, + }, + }, + }, + }}, + } + _, ctx, out := newPublishedRestFixture(t, stored) + assertNoError(t, describePublishedRestService(ctx, ast.QualifiedName{Module: "RestQ", Name: "Orders"})) + got := out.String() + assertContainsStr(t, got, "post 'upload' microflow RestQ.PutFile import mapping RestQ.IMM_Upload export mapping RestQ.EMM_Result commit YesWithoutEvents;") + assertContainsStr(t, got, "get 'status' microflow RestQ.GetStatus;") + assertContainsStr(t, got, "-- parameter X-Count: header parameter, bound to $count") + if strings.Contains(got, "parameter orderNumber") { + t.Errorf("a derivable parameter was flagged:\n%s", got) + } + // The output re-parses, and its bindings parse back to what is stored. + prog := parseMDL(t, got) + op := prog.Statements[0].(*ast.CreatePublishedRestServiceStmt).Resources[0].Operations[0] + if op.ImportMapping != "RestQ.IMM_Upload" || op.ExportMapping != "RestQ.EMM_Result" || op.Commit != "YesWithoutEvents" { + t.Errorf("describe output parses back to %+v", op) + } +} diff --git a/mdl/executor/validate_program.go b/mdl/executor/validate_program.go index 9eeb51bab..484912c80 100644 --- a/mdl/executor/validate_program.go +++ b/mdl/executor/validate_program.go @@ -289,6 +289,10 @@ func ValidateProgram(prog *ast.Program, projectPath string) []linter.Violation { // operation, so the mapping would be dropped in silence (#843). violations = append(violations, ValidateRestClientMappings(prog)...) + // Flag a published REST operation whose commit option is not one Mendix + // has; it used to parse and be thrown away (ako/mxcli#571). + violations = append(violations, ValidatePublishedRestCommit(prog)...) + // Flag a scheduled event whose Repeat and fields disagree (a Multiplier on // a Daily repeat, an HourOfDay of 99). Decidable from the statement, so it // runs here rather than at exec, where the script would already have diff --git a/mdl/executor/validate_rest_mapping.go b/mdl/executor/validate_rest_mapping.go index 186b777be..d4f22a697 100644 --- a/mdl/executor/validate_rest_mapping.go +++ b/mdl/executor/validate_rest_mapping.go @@ -54,3 +54,40 @@ func ValidateRestClientMappings(prog *ast.Program) []linter.Violation { } return out } + +// ValidatePublishedRestCommit reports (MDL-REST03) a published REST operation +// whose commit clause names no commit option. exec refuses the same statement +// (publishedRestCommit), so check predicts it. +func ValidatePublishedRestCommit(prog *ast.Program) []linter.Violation { + var out []linter.Violation + visit := func(res *ast.PublishedRestResourceDef) { + if res == nil { + return + } + for _, op := range res.Operations { + if _, err := publishedRestCommit(op.Commit); err != nil { + out = append(out, linter.Violation{ + RuleID: "MDL-REST03", + Severity: linter.SeverityError, + Message: "resource '" + res.Name + "', operation " + op.HTTPMethod + " " + op.Path + ": " + err.Error(), + Suggestion: "Write commit Yes, commit YesWithoutEvents or commit No, or leave the clause out (Yes).", + }) + } + } + } + for _, stmt := range prog.Statements { + switch s := stmt.(type) { + case *ast.CreatePublishedRestServiceStmt: + for _, res := range s.Resources { + visit(res) + } + case *ast.AlterPublishedRestServiceStmt: + for _, a := range s.Actions { + if add, ok := a.(*ast.PublishedRestAddResourceAction); ok { + visit(add.Resource) + } + } + } + } + return out +} diff --git a/mdl/roundtrip/testapp_allowlist_test.go b/mdl/roundtrip/testapp_allowlist_test.go index f9330a296..15ea36da4 100644 --- a/mdl/roundtrip/testapp_allowlist_test.go +++ b/mdl/roundtrip/testapp_allowlist_test.go @@ -184,7 +184,6 @@ var testAppKnownFailures = map[string]knownFailure{ "page WorkflowCommons.WorkflowUserTaskView_View": {laws: []law{lawGetPut}, issue: "#721 #826", why: "executes since describe prints snippet call Params (#826); the page Appearance keeps 1 of 3 DesignProperties (#721 C)"}, "page WorkflowCommons.Workflow_Dashboard": {laws: []law{lawGetPut}, issue: "#721", why: "page: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "page WorkflowCommons.Workflow_JumpTo_Options": {laws: []law{lawGetPut}, issue: "#721", why: "page: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, - "published rest service Services.OrdersRestApi": {laws: []law{lawGetPut}, issue: "#721", why: "published rest service: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "rest client Clients.OrdersRestClient": {laws: []law{lawParse}, issue: "#721", why: "rest client: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "rest client Mappings.RestClient": {laws: []law{lawParse}, issue: "#721", why: "rest client: breaks parse on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, "rule Rules.Rule1": {laws: []law{lawGetPut}, issue: "#721", why: "rule: breaks getput on TestApp, measured when it joined the harness (#743); not yet triaged into #721's classes"}, diff --git a/mdl/visitor/visitor_rest.go b/mdl/visitor/visitor_rest.go index c092688a2..ebf12ad97 100644 --- a/mdl/visitor/visitor_rest.go +++ b/mdl/visitor/visitor_rest.go @@ -450,7 +450,7 @@ func buildPublishedRestResourceDef(rc *parser.PublishedRestResourceContext) *ast // Import/Export mapping (qualifiedName after IMPORT/EXPORT MAPPING) if oc.IMPORT() != nil && len(allQN) >= 2 { - opDef.ImportMapping = allQN[1].GetText() + opDef.ImportMapping = buildQualifiedName(allQN[1]).String() } if oc.EXPORT() != nil { idx := 1 @@ -458,7 +458,7 @@ func buildPublishedRestResourceDef(rc *parser.PublishedRestResourceContext) *ast idx = 2 } if len(allQN) > idx { - opDef.ExportMapping = allQN[idx].GetText() + opDef.ExportMapping = buildQualifiedName(allQN[idx]).String() } } diff --git a/model/types.go b/model/types.go index 351913cca..fa8789a61 100644 --- a/model/types.go +++ b/model/types.go @@ -5,6 +5,7 @@ package model import ( "encoding/json" + "strings" "time" "go.mongodb.org/mongo-driver/bson" @@ -816,6 +817,62 @@ type PublishedRestOperation struct { Microflow string `json:"microflow,omitempty"` Deprecated bool `json:"deprecated,omitempty"` Parameters []string `json:"parameters,omitempty"` // path parameter names extracted from {param} in Path + + // Documentation is the operation's documentation; MDL has no spelling for + // it, so a rewrite carries the stored value. + Documentation string `json:"documentation,omitempty"` + // ImportMapping / ExportMapping are the qualified names of the mappings bound + // to the request body and the response ("" when none). + ImportMapping string `json:"importMapping,omitempty"` + ExportMapping string `json:"exportMapping,omitempty"` + // Commit is "Yes", "YesWithoutEvents" or "No" ("" is written as "Yes"). + Commit string `json:"commit,omitempty"` + // ObjectHandlingBackup is the import mapping's fallback object handling: + // "Create", "Ignore" or "Error" ("" is written as "Create"). MDL has no + // spelling for it, so a rewrite carries the stored value. + ObjectHandlingBackup string `json:"objectHandlingBackup,omitempty"` + // OperationParameters are the Rest$RestOperationParameter elements, in + // stored order. The executor derives them from the operation's microflow, + // as Studio Pro does. Empty means only the path is known: the writer then + // writes each {name} placeholder as a String path parameter. + OperationParameters []*PublishedRestOperationParameter `json:"operationParameters,omitempty"` +} + +// PathParameterNames returns the names of the {name} placeholders in the +// operation's path, in order. +func (op *PublishedRestOperation) PathParameterNames() []string { + var names []string + path := op.Path + for { + start := strings.Index(path, "{") + if start < 0 { + break + } + end := strings.Index(path[start:], "}") + if end < 0 { + break + } + names = append(names, path[start+1:start+end]) + path = path[start+end+1:] + } + return names +} + +// PublishedRestOperationParameter is one Rest$RestOperationParameter of a +// published REST operation. +type PublishedRestOperationParameter struct { + Name string `json:"name"` + // ParameterType is "Path", "Query", "Body", "Header" or "Form". + ParameterType string `json:"parameterType"` + // MicroflowParameter is the bound microflow parameter, qualified as + // Module.Microflow.Parameter ("" when unbound). + MicroflowParameter string `json:"microflowParameter,omitempty"` + Description string `json:"description,omitempty"` + // DataType is the parameter's type: "String", "Integer", "Long", + // "Decimal", "Boolean", "DateTime", "Binary", "Enumeration", "Object" or + // "List". QualifiedName is the enumeration or the entity. + DataType string `json:"dataType"` + QualifiedName string `json:"qualifiedName,omitempty"` } // ============================================================================ From 5cbc1eb36e8197506c623f3b4a6a63f010f4c521 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 21:11:32 +0000 Subject: [PATCH 5/5] fix(flows): refuse or write correctly six silent wrong writes in flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - mendixlabs/mxcli#591: list activities and cast carry the flow flavour's error handling (Abort in a nanoflow, where "Rollback" was CE6035); create, commit, call nanoflow and call microflow in a nanoflow accept only a handler without rollback (measured), refused otherwise; check now reports the nanoflow rules exec's build enforces (MDL091) and the annotation rules. - mendixlabs/mxcli#698: the MCP mapper writes errorHandlingType and refuses a custom handler / error-handler flow PED cannot express. - mendixlabs/mxcli#991: @anchor and @curve inside an error handler are applied; describe emits the handler body's layout annotations. - mendixlabs/mxcli#992: `@anchor(true: (to: top))` parsed as a division; the paren value now wins, unusable @anchor parameters are refused (MDL092), and `@curve(true: …)` is refused on nanoflows too. - mendixlabs/mxcli#870: lock/unlock name their workflow; `pause all` / `unpause all` is Studio Pro's "(Un)pause instances"; bare `all` is refused (MDL-WF17); describe no longer turns a Studio Pro lock into `all`. - mendixlabs/mxcli#175: `call workflow … on error continue` refused (MDL076), MDL076 exec-enforced. Co-Authored-By: Claude Opus 5.5 --- .../fix-issue/findings/mdl-backend.jsonl | 1 + .../fix-issue/findings/mdl-executor.jsonl | 4 + .../fix-issue/findings/mdl-grammar.jsonl | 1 + .../skills/mendix/write-nanoflows/SKILL.md | 19 +- .../skills/mendix/write-workflows/SKILL.md | 5 +- cmd/mxcli/syntax/features_microflow.go | 25 ++- docs-site/src/language/nanoflows.md | 2 +- ...low-workflow-action-position-roundtrip.mdl | 2 +- .../bug-tests/workflow-actions-describe.mdl | 15 +- mdl/ast/ast_microflow.go | 6 + mdl/ast/ast_microflow_workflow.go | 12 +- mdl/backend/mcp/microflow.go | 53 +++++ .../mcp/microflow_error_handling_test.go | 72 +++++++ .../microflow_list_error_handling_test.go | 134 ++++++++++++ .../modelsdk/microflow_read_actions.go | 19 +- .../modelsdk/microflow_workflow_write.go | 8 +- mdl/backend/modelsdk/microflow_write.go | 13 +- .../cmd_microflows_builder_actions.go | 42 ++-- mdl/executor/cmd_microflows_builder_flows.go | 46 ++++- .../cmd_microflows_builder_workflow.go | 2 + mdl/executor/cmd_microflows_format_action.go | 38 ++-- mdl/executor/cmd_microflows_show_helpers.go | 27 ++- mdl/executor/cmd_microflows_traverse_test.go | 17 +- mdl/executor/flow_handler_annotations_test.go | 193 ++++++++++++++++++ mdl/executor/lock_workflow_selection_test.go | 116 +++++++++++ .../microflow_error_handler_authoring_test.go | 41 ++++ mdl/executor/microflow_error_handling_test.go | 40 ++++ .../nanoflow_list_error_handling_test.go | 105 ++++++++++ mdl/executor/nanoflow_validation.go | 112 +++++----- mdl/executor/roundtrip_microflow_test.go | 6 +- mdl/executor/roundtrip_nanoflow_test.go | 8 +- mdl/executor/validate.go | 11 + mdl/executor/validate_microflow.go | 44 ++++ .../validate_microflow_error_handling.go | 6 + mdl/executor/validate_nanoflow.go | 51 +++++ mdl/executor/validate_nanoflow_check_test.go | 108 ++++++++++ mdl/exprcheck/adapters/adapter_scope.go | 23 ++- mdl/grammar/domains/MDLMicroflow.g4 | 13 +- mdl/grammar/domains/MDLSettings.g4 | 6 +- mdl/roundtrip/flow_modify_notes_test.go | 2 +- mdl/visitor/visitor_anchor_test.go | 42 ++++ mdl/visitor/visitor_microflow_statements.go | 83 +++++--- mdl/visitor/visitor_microflow_workflow.go | 19 +- sdk/microflows/microflows_actions.go | 25 +++ 44 files changed, 1439 insertions(+), 178 deletions(-) create mode 100644 mdl/backend/mcp/microflow_error_handling_test.go create mode 100644 mdl/backend/modelsdk/microflow_list_error_handling_test.go create mode 100644 mdl/executor/flow_handler_annotations_test.go create mode 100644 mdl/executor/lock_workflow_selection_test.go create mode 100644 mdl/executor/nanoflow_list_error_handling_test.go create mode 100644 mdl/executor/validate_nanoflow_check_test.go diff --git a/.claude/skills/fix-issue/findings/mdl-backend.jsonl b/.claude/skills/fix-issue/findings/mdl-backend.jsonl index b42032884..d9208841d 100644 --- a/.claude/skills/fix-issue/findings/mdl-backend.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-backend.jsonl @@ -149,3 +149,4 @@ {"area": "mdl/backend", "date": "2026-09-29", "symptom": "describe -> exec, CREATE OR MODIFY or an ALTER that rebuilds the document turns an API-exported document Hidden: enumerations, pages, layouts, rules, view-entity OQL source documents, import/export mappings, JSON structures, published and consumed REST services, scheduled events, workflows, database connections, business event services, data transformers, queues, regular expressions and agent-editor documents. The run reports success, mx check is clean; the module's public surface silently shrinks. A workflow's own `export level API` clause was a no-op on create and on rewrite.", "cause": "Each rewrite converter builds a fresh document and writes ExportLevel as a constant (\"Hidden\"), or passes the semantic model's value where the executor itself filled in \"Hidden\" (mappings, database connection, business events), and the unit is replaced wholesale. MDL has no export-level spelling for most of these kinds, so describe cannot print it and the executed script cannot restore it. workflowToGen ignored wf.ExportLevel entirely. The round-trip harness could not see it: every document in TestApp and PedApp is Hidden, the constant itself.", "file": "mdl/backend/modelsdk/export_level_carry.go", "fix": "One byte-level carry, keepStoredExportLevel(unitID, contents): replaces only the top-level ExportLevel element of the freshly encoded rewrite with the stored value, copying every other element verbatim, and never adds the key. Wired into every Update path that writes ExportLevel (UpdateEnumeration/Rule/Layout/ImportMapping/ExportMapping/JsonStructure/PublishedRestService/ConsumedRestService/DataTransformer/DatabaseConnection/BusinessEventService, writeCustomBlob update, WriteViewEntitySourceDocument update; page via carryStoredPageHeader). Kinds with an MDL spelling (workflow, scheduled event, queue, regular expression) use keepStoredExportLevelUnlessSet: an authored level wins. workflowToGen now writes orDefault(wf.ExportLevel, \"Hidden\").", "insight": "A fixture-driven round trip is blind to any constant that happens to equal every fixture value: 775 TestApp documents round-tripped while 10 kinds hid API documents. Set the subject to the non-default value first (here: patch ExportLevel to API on the working copy) and run both the plain describe output and an edited one, because an elided unchanged write passes a converter that still writes the constant. Carrying at the encoded-bytes level covers gen-typed, newElem-built and hand-serialized writers with one helper, where a gen setter per converter would have needed three mechanisms.", "refs": ["ako/mxcli#816", "ako/mxcli#801", "ako/mxcli#812"], "test": "mdl/backend/modelsdk/issue816_export_level_test.go (TestUpdatePaths_KeepStoredExportLevel, 18 kinds); mdl/roundtrip/export_level_test.go (TestTestAppExportLevelSurvivesRoundTrip, -tags integration)"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#859 review of PR #864: after the built comparison landed, changing or adding `show page M.P with title = 'X'` in a `create or modify microflow` reported \"Unchanged microflow\" and wrote nothing, under mdl 0 and mdl 1 (main spliced it). Nothing warned.", "cause": "builtAsStored compares the declared flow and the stored flow both READ BACK through the codec, so any property the reader drops compares equal whatever either side holds. The ShowFormAction reader never read FormSettings.TitleOverride. Probing encode(built) against encode(readback(built)) over mdl-examples found the reader also dropped ExclusiveSplit/LoopedActivity ErrorHandlingType and a REST call's bound output variable (ResultHandling.ResultVariableName -> RestCallAction.OutputVariable), plus CallWebServiceAction (#861). Before the built comparison such a loss was a visible phantom re-splice; after it, a silently dropped edit.", "fix": "ReadBackMicroflow/ReadBackNanoflow re-encode what they read back and refuse (error -> statement diff, the pre-#859 path) when it is not the document first written, $IDs aside (sameWritten). The reader now reads TitleOverride, the split's and loop's ErrorHandlingType, and a bound REST call's OutputVariable, so those flows keep matching.", "insight": "A comparison made on both sides through the same lossy reader cannot see what the reader loses; the lost property becomes a change that is never written. When equality is decided after a decode, prove the decode lossless for the value at hand (write it again and compare bytes) and fall back when it is not. The probe that found the fields: diff encode(x) with encode(decode(encode(x))) over every mdl-examples flow.", "issue": "ako/mxcli#859", "file": "mdl/backend/modelsdk/microflow_readback.go, mdl/backend/modelsdk/microflow_read_actions.go, mdl/backend/modelsdk/microflow.go, mdl/roundtrip/flow_idempotent_shapes_test.go"} {"date": "2026-09-30", "area": "mdl/backend", "symptom": "ako/mxcli#843 (rehearsal M2): under mdl 1, `create or modify nanoflow … returns Boolean as $Done` over a nanoflow stored without a return variable refuses \"the stored document has no ReturnVariableName property … set it in Studio Pro\"; the same statement on a microflow reports \"set: ReturnVariableName\".", "cause": "mfmutator.SetHeader refuses any stated header key the stored document lacks (a key the project version does not declare makes the document unopenable). mxcli's nanoflow writer omits ReturnVariableName when the statement has no `as $Var`, while the microflow writer always writes it on 10+, so only nanoflows hit the refusal.", "fix": "Optional mfmutator.PropertyDeclarer on Deps; the codec deps answer from the metamodel version data (type, then Microflows$MicroflowBase; ReturnVariableName is 10.12+) against the project version, and SetHeader inserts the key after its predecessor in the encoder's order. No answer (MCP, unknown version) keeps the refusal.", "insight": "A refusal keyed on 'the stored document lacks the key' conflates 'this version has no such property' with 'the writer left it out'; the metamodel version data separates the two. Studio Pro 11 stores ReturnVariableName on every nanoflow (PedApp: 13 of 13), so adding it matches what Studio Pro writes.", "issue": "ako/mxcli#843", "file": "mdl/backend/mfmutator/header.go"} +{"date": "2026-10-01", "area": "mdl/backend", "symptom": "mendixlabs/mxcli#698: over --mcp, `call microflow … on error rollback|continue` was created with PED's default error handling; a custom handler's error edge was written as an ordinary sequence flow. No error either way.", "cause": "mapMicroflowAction built every action map without errorHandlingType (only notify carried it), and buildFlowDocContent wrote every flow alike — PED's SequenceFlow constructor has no property for an error-handler flow (ped_get_schema, Studio Pro 11.14).", "file": "mdl/backend/mcp/microflow.go (mapObjectTree, buildFlowDocContent)", "fix": "carryErrorHandlingType writes the action's ErrorHandlingType (read by reflection); Custom/CustomWithoutRollBack and IsErrorHandler flows are refused with a message to run without --mcp.", "test": "TestMapObjectTree_CarriesErrorHandlingType, TestMapObjectTree_RefusesACustomErrorHandler", "insight": "ped_get_schema answers 'can PED express this' in one call; check it before mapping a property, and refuse what it cannot hold."} diff --git a/.claude/skills/fix-issue/findings/mdl-executor.jsonl b/.claude/skills/fix-issue/findings/mdl-executor.jsonl index db4ac1363..58486da0e 100644 --- a/.claude/skills/fix-issue/findings/mdl-executor.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-executor.jsonl @@ -791,3 +791,7 @@ {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#890 (rehearsal 2, R-rep): re-running a settled script wrote nothing (units_written=0) but still printed \"Granted access on ...\", \"Set project security level to ...\", \"Added module roles ... to user role ...\", \"Updated ... settings\" / \"Updated configuration ...\", and `move ... to folder` printed \"Moved ... to new location\"; the output could not serve as the #859 'second run reports Unchanged' gate. Also: a second `move` of the same document in one session failed \"microflow not found\".", "cause": "Those handlers printed their sentence with fmt.Fprintf after the backend call instead of going through ReportMutation's write-elision evidence (WriteStats offered vs written). Grants/revokes inside a program run are deferred (#872 accessRuleRun), so even ReportMutation could not see their write at statement time. The typed movers changed a container without invalidating the cached hierarchy.", "file": "mdl/executor/report_mutation.go, mdl/executor/access_rule_run.go, mdl/executor/cmd_security_write.go, mdl/executor/cmd_settings.go, mdl/executor/cmd_move.go", "fix": "ExecContext.reportWrite(unchanged, sentence...) prints the sentence or `Unchanged ` (through the run tally) on the ReportMutation evidence rule; used for project security level/demo users/strict mode/guest access, alter user role module roles, settings section/configuration/constant updates. Access-rule reports go through reportAccessRule: held on the open accessRuleRun and printed at its flush, Unchanged when the flush offered and elided (notices like 'No access rules found' print regardless). execMove short-circuits a document already in the target container (alreadyPlaced -> Unchanged) and invalidates the hierarchy after every move.", "test": "mdl/executor/noop_reporting_pedapp_test.go TestNoopRerun_ReportsUnchanged (PedApp, per statement: run 1 reports its write = control; run 2 writes no file and reports Unchanged, for grant, security level, demo users, strict mode, user role module roles, settings runtime, configuration (alter and create or modify), move) and TestNoopRerun_ProgramReportsUnchanged (program run incl. a revoke+grant reset; run 2 writes nothing and reports no write verb; run 1's net-nothing reset reports no write). Revert check: every case fails with the write sentence on run 2; the move case with 'microflow not found'.", "insight": "A report printed after a backend call is a claim about storage the handler cannot make on its own; route every write report through the write-stats evidence, and where writes are deferred, defer the report with them. The output only becomes an idempotency gate when no statement prints a write verb by construction."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#840 (mxcli-ledger, finding 162): `mxcli describe` (no header, no option) wrote mdl 0 spellings that mdl 1 refuses, and its microflow output held `$N = count($Hits)` / `$x = find($L, …)` — call forms registered as deprecated (MDL-DEPR003/004), so subcommand output was neither mdl 1-runnable nor mdl 0-clean. At the freeze a second instance surfaced on TestApp: describe of a chart series' text template wrote `staticTooltipHoverTextParams: [{1} = X]`, the bracketed form MDL-DEPR124 deprecates, under both versions.", "cause": "formatListOperation / the AggregateListAction case gated the statement form on describeLanguage >= V1 and fell back to the call form for mdl 0, although the statement form parses with the same meaning and no warning under mdl 0. The object-list describer (cmd_pages_describe_objectlist.go) wrote a TextTemplate's Params as \"[\" + … + \"]\". The roundtrip test that should have caught both (describeUsesCanonicalSpellings) filtered deprecations to an R8 allowlist ('describe keeps them under mdl 0'), so any code outside the list was invisible.", "file": "mdl/executor/cmd_microflows_format_action.go, mdl/executor/cmd_pages_describe_objectlist.go, mdl/roundtrip/describe_canonical_spelling_test.go", "fix": "Describe writes the List operation / Aggregate list statement in every language; only an activity the statement cannot express falls back to the call. Object-list template parameters are written in ( ). The canonical-spelling roundtrip test now checks EVERY registered deprecation under both describe languages (describeAs V1 and V0) on PedApp and TestApp, with no code filter. Freeze: langver.Frozen = V1, every describe output starts with `mdl 1;`, `--mdl 0|1` on describe/context/diff-local.", "test": "mdl/executor/cmd_microflows_format_list_activity_test.go TestDescribeListActivityUnderMdl1 (both versions, plus the call-form control warning under mdl 0); mdl/roundtrip/describe_canonical_spelling_test.go TestTestAppDescribeUsesCanonicalSpellings (failed on Snip_TaskDashboard_Numbers & 2 more with MDL-DEPR124 before the objectlist fix).", "insight": "A test that filters warnings to a list of 'codes this test owns' hides every code added later; a 'never emits X' property must check the whole registry, and in every output language the command can be asked for."} {"date": "2026-10-01", "area": "mdl/executor", "symptom": "ako/mxcli#905 part 1 (#897 item 4, rehearsal 3 G2): under `mdl 1`, `create or modify microflow` that grows a stored flow by an activity whose custom error handler ends in its own `return` (`$Ok = call microflow … on error begin log …; return; end error;`) was refused on every run, as an insert or a replace: \"an error handler in the fragment ends at an end event of its own … a return inside an error handler is not spliced yet\". Created fresh the statement worked; mdl 0 rebuilt (MDL-V1-REBUILD). On CapTrack the stub-then-real pair (13-actions stub, 30-export real ACT_Export_Excel) left the placeholder stored on every run with mx check clean. Once spliced, the second run of the real CapTrack script was refused: \"replace $Written: cannot replace the ActionActivity …: it has an error handler\".", "cause": "addErrorHandlerFlow (cmd_microflows_builder_flows.go) builds a handler body with a child flowBuilder and merged its objects and flows into the parent, but not its returnEndIDs, so cutFragment saw the handler's return end event as one the builder added and refused it. Second defect, exposed by the grow: a log/show message/validation feedback message written as an expression (`'failed for ' + $User/Name`) is stored as template '{1}' with the expression as parameter, and describe prints that form; declaredMatches compared the two spellings as different statements. Where builtAsStored is false (a spliced or Studio Pro-drawn flow), the statement diff then replaced the activity: absorbed by write elision on the main path, but refused when the message sits in a stored activity's error handler.", "file": "mdl/executor/cmd_microflows_builder_flows.go, mdl/executor/flow_declared_match.go", "fix": "addErrorHandlerFlow copies errBuilder.returnEndIDs into the parent's (lastReturnEndID untouched), so a handler's return is a new end event of the flow like a guard's (#888); placement/room checks (checkRoom/checkBranches) apply unchanged and refuse where the handler's return branch would cross a stored flow. matchValue normalises LogStmt/ShowMessageStmt/ValidationFeedbackStmt (messageAsTemplate) to the builder's stored form: a non-literal message becomes '{1}' with the expression as first parameter (a log stating its own `with (...)` keeps its message, as the builder does).", "test": "mdl/executor/cmd_alter_flow_handler_return_test.go TestCutFragment_HandlerReturnIsANewEndEvent; mdl/executor/flow_message_respelling_test.go (with controls); mdl/roundtrip/flow_splice_handler_return_test.go TestSpliceRerun_GrowByHandlerReturn (replace/insert under mdl 1, insert under mdl 0, verdict agreement check/diff/exec, twice-exec, a changed-flow control), TestSpliceRerun_GrowStudioProFlowByHandlerReturn (PedApp ShowPasswordForm, all stored IDs kept, description fixed point), TestSpliceRerun_HandlerReturnWithNoRoomIsRefused. Revert checks: returnEndIDs not copied -> every grow refused 'not spliced yet' (check predicts it); messageAsTemplate off -> second run refused 'replace $Ok … it has an error handler'. CapTrack copy: fmt --upgrade -p 13+30, exec 13, 30, 30 -> real flow stored, third run 0 units; mx check identical to baseline (0 errors). PedApp repros mx check identical to baseline.", "insight": "Child builders (error handler, loop) each keep builder state the parent's consumers rely on; when a new piece of builder state is added for the splice (returnEndIDs, #888), enumerate the child builders and decide per child whether it propagates. A grow test that only re-runs on an mxcli-authored flow can pass because builtAsStored short-circuits the statement diff; the respelling only surfaced on the real project and the Studio Pro-drawn flow, where the statement diff decides."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#591: a nanoflow with `$L = create list of …`, `add $x to $L`, `remove …`, `$n = count $l`, `$h = head $l`, filter/sort or a cast passed check and exec, then mx check (11.14.0) reported CE6035 \"Error handling type is not supported\" at Create list / Change list / Aggregate list / List operation activity — one per activity. `call nanoflow … on error continue` in a nanoflow, and create/commit/call microflow with continue, rollback or a custom handler WITH rollback, failed the same way. `mxcli check` reported none of it: the nanoflow rules ran only inside exec's build.", "cause": "The list activities and cast had no ErrorHandlingType on their sdk structs, so the builder could not supply the flow flavour's default and both writers stamped a literal \"Rollback\" — CE6035 in a nanoflow, where Studio Pro stores \"Abort\" on every action (130 actions over ako/TestApp's 11 nanoflows) and \"Rollback\" on the same actions in a microflow. The per-clause rule for nanoflows covered six statements; create/commit/call nanoflow/call microflow accept ONLY a handler without rollback there (measured, one nanoflow per cell). ValidateNanoflow ran only MDL044, so check passed every nanoflow-body refusal exec's build makes.", "file": "sdk/microflows/microflows_actions.go; mdl/executor/cmd_microflows_builder_actions.go; mdl/backend/modelsdk/microflow_write.go + microflow_read_actions.go; mdl/executor/nanoflow_validation.go; mdl/executor/validate_nanoflow.go", "fix": "ErrorHandlingType on CreateList/ChangeList/ListOperation/Aggregate/Cast, set to fb.ehType(nil) (Abort in a nanoflow, Rollback in a microflow), written with orDefault(…,\"Rollback\") and read back so built-vs-stored compares it. nanoflowWithoutRollbackOnly refuses every other clause on create/commit/call nanoflow/call microflow in a nanoflow. ValidateNanoflow reports validateNanoflowBody's messages as MDL091 and runs the annotation rules.", "test": "TestListActions_ErrorHandlingFollowsTheFlowFlavour, TestCastAction_ErrorHandlingFollowsTheFlowFlavour, TestListActions_ErrorHandlingTypeRoundTrips, TestNanoflow_RefusesAllButWithoutRollbackOnCreateCommitAndCalls, TestValidateNanoflow_ReportsWhatExecWouldRefuse", "insight": "A hardcoded writer literal is a default for ONE flow flavour. The measurement that settled it was a one-nanoflow-per-activity script built with mx check, and the Studio Pro survey (Abort everywhere in nanoflows) told which value is right. Survey stored values per flavour BEFORE trusting a literal; and a rule that exec's build enforces must have a check-side twin, or check passes what exec refuses."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#991: inside `on error … begin … end error`, @anchor and @curve parsed, passed check and exec, and changed nothing (the edges kept the default sides and 0;0 control vectors); DESCRIBE printed no @position/@anchor/@curve/@caption/@color/@excluded for a handler-body statement, so describe → exec put a laid-out handler back on auto-placement and reported success.", "cause": "addErrorHandlerFlow created the handler's edges with newErrorHandlerFlow/newHorizontalFlow and never applied the statements' anchors; a @curve was recorded on the child builder's curveByOrigin, which nothing applied. The describer's handler traversal (collectErrorHandlerStatementSpans) emitted only the notes. And the anchor emitter judged `to:` against the main path's default (left), while the error edge enters the TOP by default, so `to: left` on a handler's first statement read as default and was omitted.", "file": "mdl/executor/cmd_microflows_builder_flows.go (addErrorHandlerFlow); mdl/executor/cmd_microflows_show_helpers.go (collectErrorHandlerStatementSpans, emitAnchorAnnotationWithActivityMap)", "fix": "applyUserAnchors on the error edge (destination = first statement's to:) and between handler statements; the tail edge takes only the last statement's from: (originOnly); the child builder's curves merge into the parent's. The handler traversal calls emitObjectAnnotations; an activity whose only incoming flow is an error edge is judged against top.", "test": "TestErrorHandlerBody_HonoursAnchorAndCurve, TestErrorHandlerBody_TailEdgeTakesTheLastStatementsFromAndCurve, TestErrorHandlerBody_DescribeKeepsItsLayout, TestValidateMicroflow_ChecksAnnotationsInsideAHandler", "insight": "The error-handler body is a second, smaller builder AND a second, smaller describer; every annotation feature added to the main path has to be checked against both. A describe fixed-point test on a hand-placed handler is what exposes the default-side mismatch."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#870: `lock workflow all;` / `unlock workflow all;` passed check and exec and failed mx check with CE1825 \"The 'Workflow' property is required\". Worse, DESCRIBE printed the Studio Pro activity WorkflowCommons.ACT_WorkflowDefinition_Lock (PauseAllWorkflows true WITH a $WorkflowDefinition selection) as `lock workflow all;` — describe → exec dropped the workflow and wrote the unbuildable form. `lock workflow Mod.Wf;`, which describe printed for a name selection, did not parse.", "cause": "MDL read PauseAllWorkflows as \"all workflows\"; it is Studio Pro's \"Pause instances\" checkbox ON the selected definition (Unlock: \"Unpause instances\", ResumeAllPausedWorkflows), on by default — the metamodel has no all-definitions selection. The writer omitted the selection whenever the flag was set.", "file": "mdl/grammar/domains/MDLMicroflow.g4 (lock/unlockWorkflowStatement); mdl/visitor/visitor_microflow_workflow.go; mdl/executor/cmd_microflows_format_action.go; mdl/backend/modelsdk/microflow_workflow_write.go; mdl/executor/validate_microflow.go", "fix": "`lock workflow $WfDef|Mod.Wf [pause all]` / `unlock workflow … [unpause all]`; the writer always writes a named selection; describe prints the selection plus the flag; a bare `all` is refused at check and exec (MDL-WF17).", "test": "TestLockWorkflow_PauseInstancesIsAFlagOnASelection, TestLockWorkflow_DescribeKeepsTheSelection, TestLockWorkflow_AllWithoutAWorkflowIsRefused, TestLockWorkflow_FlagKeepsTheSelection", "insight": "The Studio Pro data point the issue asked for was already in ako/TestApp (WorkflowCommons stores both branches of the checkbox). Look for a Studio Pro-authored instance in the fixtures before designing syntax from the metamodel's property name."} +{"date": "2026-10-01", "area": "mdl/executor", "symptom": "mendixlabs/mxcli#175: `call workflow … on error continue` passed check and exec, then mx check: CE6035 at Call workflow activity. On 11.14.0 only continue fails: no clause, rollback and both custom handlers build.", "cause": "continueUnsupportedOn did not list call workflow, and adapters.StatementErrorHandling — a hand-kept list of statements carrying a clause — did not list CallWorkflowStmt (nor the REST/other workflow statements), so MDL076 could not even see the clause. MDL076 was also check-only: exec without the pre-check (-c, REPL, --no-check) wrote it.", "file": "mdl/executor/validate_microflow_error_handling.go; mdl/exprcheck/adapters/adapter_scope.go; mdl/executor/validate.go", "fix": "call workflow in continueUnsupportedOn; StatementErrorHandling falls back to reading any statement's ErrorHandling field by reflection (getErrorHandling delegates to it); MDL076 is exec-enforced.", "test": "TestMDL076_ReportsContinueOnCallWorkflow, TestMDL076_CallWorkflowAcceptsTheOtherClauses, TestMDL076_IsExecEnforced", "insight": "Three lists of 'statements with an ON ERROR clause' existed; each had drifted. A rule keyed on such a list silently does nothing for a statement the list forgot."} diff --git a/.claude/skills/fix-issue/findings/mdl-grammar.jsonl b/.claude/skills/fix-issue/findings/mdl-grammar.jsonl index 883563cce..d8f8d9fcf 100644 --- a/.claude/skills/fix-issue/findings/mdl-grammar.jsonl +++ b/.claude/skills/fix-issue/findings/mdl-grammar.jsonl @@ -67,3 +67,4 @@ {"area": "mdl/grammar", "date": "2026-09-28", "symptom": "After `ai` and `sample` became lexer keywords (R10), `alter enumeration M.E drop value Sample;` (and add/rename/modify value) failed with `mismatched input 'Sample' expecting IDENTIFIER`, though `create enumeration M.E ( Sample 'Sample' )` still parsed.", "cause": "alterEnumerationAction named values with a bare IDENTIFIER while create used enumValueName (IDENTIFIER | QUOTED_IDENTIFIER | keyword); every new lexer keyword silently shrinks the set of value names alter can reach. Model and older keywords were already unreachable.", "file": "mdl/grammar/domains/MDLDomainModel.g4, mdl/visitor/visitor_enumeration.go", "insight": "alterEnumerationAction now takes enumValueName and the visitor unquotes it. When a PR adds a lexer token, grep the grammar for bare IDENTIFIER in name positions (not identifierOrKeyword / a *Name rule) and try the new word there; the round-trip apps only catch it if they happen to use the word. Test TestAlterEnumerationValueNamedLikeAKeyword (fails on the IDENTIFIER-only rule). ako/mxcli#755 review.", "refs": ["#755", "#785"]} {"area": "mdl/grammar", "date": "2026-09-30", "symptom": "`create or modify constant M.DbPassword type string default '' PRIVATE;` -> `line 22:80 no viable alternative at input 'PRIVATE'` after upgrading past v0.24.0; `fmt --upgrade` failed with the same error because it must parse first. 9 credential constants in mxcli-formula1; the database-connections skill had taught the word.", "cause": "No grammar rule ever had PRIVATE. In v0.24.0 (and nightly 8c227f46) `helpStatement: IDENTIFIER (helpTopicWord …)*` was a catch-all and `;` was optional, so `… default '' private;` parsed as TWO statements: the constant, then a help statement `private` that the visitor built nothing for (`check` said `Syntax OK (2 statements)`). R7's IsHelpWord predicate (#755, bf0d1d38; the gated b518ae72 was reverted by dd977eea) closed the catch-all, turning the silent no-op into a parse error with no registry entry.", "file": "`mdl/grammar/domains/MDLDomainModel.g4` (constantOption `{IsPrivateWord(...)}? IDENTIFIER /* @alias MDL-DEPR138 */`), `mdl/grammar/MDLParser.g4` (IsPrivateWord), `mdl/deprecation/deprecation.go` (ConstantPrivate, RemovedIn 1), `mdl/visitor/visitor_enumeration.go` (record + delete fix), `mdl/visitor/visitor_deprecations.go` (recordDeprecation refuses at RemovedIn), tests `mdl/visitor/visitor_constant_private_test.go`, `mdl/upgrade/constant_private_test.go`, example `mdl-examples/deprecated-aliases/bug-tests--865-constant-private.mdl`", "insight": "To find what an old catch-all silently accepted, do not diff grammars — the word was in neither. Dump the OLD parse tree (`ToStringTree`) for the failing line; it showed `(helpStatement private)`. Then audit the class by walking every error-free old parse of a corpus for helpStatement nodes whose word is not help/exit/quit: over the repo's skills/docs/examples at 8c227f46 and all of mxcli-formula1, the only such word in a clean parse was PRIVATE (after a constant), so a targeted alias is the whole fix, not re-opening the catch-all. Match the word by a semantic predicate on IDENTIFIER, not a new keyword, so `private` stays usable as a name. Divergence to know: the old catch-all also swallowed words AFTER private (`private exposed to client` dropped the exposure); the alias does not.", "refs": ["ako/mxcli#865", "ako/mxcli#755", "ako/mxcli#714"]} {"area": "mdl/grammar", "date": "2026-10-01", "symptom": "`fmt --upgrade --header` refused `$Ordered = sort($Rows, Position);` with `MDL-V1-LIST: the operand is not a variable (a nested call or an expression)` and left the whole file at mdl 0 (2 mxcli-formula1 files); any keyword attribute (Position, Status, Type, Date, Value, Title, Caption, Content, Index) did it, and `sort($L, Status desc)` did not parse at all.", "cause": "The call form's `sortSpec` took only `IDENTIFIER | QUOTED_IDENTIFIER`, while the statement form's `listSortItem` took `identifierOrKeyword`. With a keyword attribute the listOperationStatement alternative failed and the line fell through to setStatement as a generic function call; mdl 0 still built the sort from it (buildListOrAggregateStatement), but the upgrade fix (setCallFix -> singleListCall) found no ListOperationContext and reported a nested call.", "file": "`mdl/grammar/domains/MDLMicroflow.g4` (sortSpec: identifierOrKeyword (ASC|DESC)?), `mdl/visitor/visitor_microflow_actions.go` (buildSortSpecList), `mdl/visitor/visitor_microflow_expression.go` (sort spec args); tests `mdl/upgrade/gated_test.go` TestUpgrade_KeywordAttributeNames, `mdl/visitor/visitor_microflow_sort_quoted_test.go` TestUnquotedKeywordSortAttribute", "insight": "A misleading upgrade reason ('not a variable') was a grammar asymmetry between a call form and its statement form: when one rule falls through to the generic expression path, the upgrader sees a function call and loses the structure. Compare the two forms' operand rules side by side. Proven with a control binary: `check` over 741 example/doc/skill scripts identical, and `fmt --upgrade --header` over 172 rehearsal scripts differs in exactly the two formula1 files. find/filter by member and sum/min/max over `$L.Keyword` were already fine.", "refs": ["ako/mxcli#889", "ako/mxcli#714"]} +{"date": "2026-10-01", "area": "mdl/grammar", "symptom": "mendixlabs/mxcli#992: `@anchor(true: (to: top))` — the per-case form DESCRIBE emits, with one side — parsed, passed check and exec, and left the true edge on the default sides. `@anchor(true: (from: right, to: top))` worked. `@curve(true: …)` on a NANOFLOW passed check and exec and was dropped (on a microflow MDL060 refused it).", "cause": "annotationParam listed `annotationValue | annotationParenValue`; `(to: top)` also matches annotationValue's expression alternative, as `to : top` — `:` is Mendix's division operator — and ANTLR took the first alternative, so the nested-anchor reader found no paren value. The visitor then skipped anything it could not use. Nanoflows never ran the annotation rules (ValidateNanoflow ran only MDL044).", "file": "mdl/grammar/domains/MDLSettings.g4 (annotationParam); mdl/visitor/visitor_microflow_statements.go (parseAnchorAnnotation)", "fix": "annotationParenValue before annotationValue; parseAnchorAnnotation records every parameter it cannot use in InvalidAnchors, refused as MDL092 at check and exec; ValidateNanoflow runs checkUnknownAnnotations over the whole body.", "test": "TestAnchorAnnotation_SplitBranchWithOneSide, TestAnchorAnnotation_RecordsWhatItCannotUse, TestSplitBranch_OneSidedAnchorReachesTheEdge, TestAnchorParameterItCannotUseIsRefused", "insight": "A two-key test case hid the one-key failure: `(from: x, to: y)` cannot be an expression (comma), `(to: y)` can. Dump the parse tree (ToStringTree) for the exact failing text before reading the visitor."} diff --git a/.claude/skills/mendix/write-nanoflows/SKILL.md b/.claude/skills/mendix/write-nanoflows/SKILL.md index 38acc40c5..4f8ceb08c 100644 --- a/.claude/skills/mendix/write-nanoflows/SKILL.md +++ b/.claude/skills/mendix/write-nanoflows/SKILL.md @@ -573,19 +573,24 @@ IF $Location = empty THEN END IF; ``` -For per-action error handling without CONTINUE: +For per-action error handling on a call, use a handler WITHOUT rollback — the +only form a nanoflow call takes: ```mdl -$Result = CALL NANOFLOW Sales.NAV_Risky () ON ERROR ROLLBACK; +$Result = CALL NANOFLOW Sales.NAV_Risky () ON ERROR WITHOUT ROLLBACK BEGIN + LOG WARNING NODE 'Sales' 'Could not load the data.'; +END ERROR; ``` -### Most activities take NO error handling in a nanoflow +### Which activities take which clause in a nanoflow -An `ON ERROR` clause of **any** form is rejected on these six, with -**CE6035** "Error handling type is not supported" — measured on Mendix 11.14.0: +Measured on Mendix 11.14.0; every other form is **CE6035** "Error handling type +is not supported", and mxcli refuses it (MDL091): -| Refused in a nanoflow | Accepted | +| Activity in a nanoflow | Accepted clauses | |---|---| -| `CHANGE`, `LOG`, `SHOW PAGE`, `CLOSE PAGE`, `SHOW MESSAGE`, `VALIDATION FEEDBACK` | `DECLARE`, `SET` (the two *variable* activities) | +| `DECLARE`, `SET`, `RETRIEVE`, `DELETE` | every form | +| `CREATE`, `COMMIT`, `CALL NANOFLOW`, `CALL MICROFLOW` | only `ON ERROR WITHOUT ROLLBACK BEGIN … END ERROR` | +| `CHANGE`, `LOG`, `SHOW PAGE`, `CLOSE PAGE`, `SHOW MESSAGE`, `VALIDATION FEEDBACK` | none | mxcli refuses the clause rather than writing a nanoflow mxbuild rejects. The split is by activity, not by "client-side vs server-side" — `SHOW MESSAGE` is as diff --git a/.claude/skills/mendix/write-workflows/SKILL.md b/.claude/skills/mendix/write-workflows/SKILL.md index e2c149c85..e8689514f 100644 --- a/.claude/skills/mendix/write-workflows/SKILL.md +++ b/.claude/skills/mendix/write-workflows/SKILL.md @@ -408,8 +408,11 @@ workflow / its tasks. They are easy to miss — there is no `complete task`: required**: a notify without one fails the build (CE0166, MDL-WF16). Name the element as `Module.Workflow.ElementName`; mxcli works out which kind it is and refuses one a notification cannot reach (a timer start, a user task). -- `open user task $Task`, `lock workflow $Wf`, and +- `open user task $Task`, `lock workflow $WfDef`, and `workflow operation abort|pause|restart|retry|continue $Wf` are also statements. + A lock or unlock names its workflow definition (`$WfDef` or `Module.Workflow`); + `pause all` / `unpause all` after it is Studio Pro's "Pause / Unpause instances". + A bare `lock workflow all` is refused (MDL-WF17) — it built as CE1825. A common shape: the task page's buttons call a microflow that does the change and then `set task outcome $Task ''`, leaving the workflow's outcome branch diff --git a/cmd/mxcli/syntax/features_microflow.go b/cmd/mxcli/syntax/features_microflow.go index 26111d400..78aaecefb 100644 --- a/cmd/mxcli/syntax/features_microflow.go +++ b/cmd/mxcli/syntax/features_microflow.go @@ -180,17 +180,20 @@ func init() { "-- Two limits, both enforced rather than silently ignored:\n" + "--\n" + "-- ON ERROR CONTINUE is rejected by Mendix (CE6035) on CREATE, CHANGE,\n" + - "-- COMMIT, LOG, SHOW PAGE, CLOSE PAGE, SHOW MESSAGE and VALIDATION\n" + - "-- FEEDBACK -> MDL076. A custom handler IS accepted on all of them, and\n" + - "-- CONTINUE is fine on DECLARE, SET, RETRIEVE, DELETE and CALL MICROFLOW.\n" + + "-- COMMIT, LOG, SHOW PAGE, CLOSE PAGE, SHOW MESSAGE, VALIDATION\n" + + "-- FEEDBACK and CALL WORKFLOW -> MDL076. A custom handler IS accepted on\n" + + "-- all of them, and CONTINUE is fine on DECLARE, SET, RETRIEVE, DELETE\n" + + "-- and CALL MICROFLOW.\n" + "--\n" + "-- List operations and aggregates ($x = head $l, $n = count $l) have\n" + "-- no error handling in Mendix at all -> MDL077.\n" + "--\n" + - "-- IN A NANOFLOW only DECLARE and SET take a clause at all. CHANGE, LOG,\n" + - "-- SHOW PAGE, CLOSE PAGE, SHOW MESSAGE and VALIDATION FEEDBACK are CE6035\n" + - "-- there in EVERY form, and are refused: a nanoflow activity aborts the\n" + - "-- flow on error by default and has no transaction to roll back.\n" + + "-- IN A NANOFLOW a statement with no clause aborts the flow on error, and\n" + + "-- there is no transaction to roll back. DECLARE, SET, RETRIEVE and DELETE\n" + + "-- take every clause. CREATE, COMMIT, CALL NANOFLOW and CALL MICROFLOW take\n" + + "-- only ON ERROR WITHOUT ROLLBACK BEGIN ... END ERROR. CHANGE, LOG, SHOW\n" + + "-- PAGE, CLOSE PAGE, SHOW MESSAGE and VALIDATION FEEDBACK take none. Every\n" + + "-- other form is CE6035 and is refused (MDL091).\n" + "--\n" + "-- A handler that does NOT end in RETURN/RAISE ERROR merges back into the main\n" + "-- flow, so a variable created after the merge is out of scope on the error\n" + @@ -633,10 +636,18 @@ func init() { "@merge(x, y) -- the implicit merge that closes a split\n" + "@anchor(from: bottom, to: top, true: (…), false: (…)) -- on an IF: to = its incoming flow,\n" + " -- from = the flow leaving its closing merge\n" + + "@anchor(true: (to: top)) -- one branch's edge; either side may be left out\n" + "@caption 'text'\n@color Green\n@annotation 'a note'\n@excluded\n" + "@applyentityaccess | @applyentityaccess(false) -- DOCUMENT-level, before CREATE MICROFLOW/RULE\n" + "@annotation(id: n1, text: 'a note', position: (x, y), size: (w, h))\n" + "@annotation(id: n1) -- attaches THAT note to another activity\n\n" + + "Inside ON ERROR ... BEGIN ... END ERROR the annotations mean what they mean\n" + + "outside it: @anchor(to:) on the handler's first statement is the side the\n" + + "error edge enters, @anchor(from:) and @curve shape the edge leaving a\n" + + "statement — for the last one, the edge that rejoins the main flow.\n\n" + + "@curve has no per-branch form: on a split it curves every outgoing edge, and\n" + + "@curve(true: …) is refused (MDL060). An @anchor parameter that is not a\n" + + "side mxcli knows is refused too (MDL092) rather than left on the default.\n\n" + "An unrecognised @name is an error (MDL059): it would parse and do nothing,\n" + "so a typo of @position would silently discard the layout. That covers\n" + "DOCUMENT annotations too — a typo, or one on a document kind that does not\n" + diff --git a/docs-site/src/language/nanoflows.md b/docs-site/src/language/nanoflows.md index b06c8d549..64a0734c7 100644 --- a/docs-site/src/language/nanoflows.md +++ b/docs-site/src/language/nanoflows.md @@ -124,7 +124,7 @@ The following activities are server-only and cannot be used in nanoflows: - `CALL EXTERNAL ACTION` — external actions are server-side - All **workflow actions** (call/open workflow, set task outcome, user task, etc.) -> **Note:** Per-action error handling (`on error continue`) IS supported in nanoflows. Only `ErrorEvent` (raise error as a standalone flow action) is forbidden. Note that `on error rollback` is syntactically valid but only rolls back in-memory changes — nanoflows have no database transactions. +> **Note:** Per-action error handling is supported in nanoflows, per activity. Only `ErrorEvent` (raise error as a standalone flow action) is forbidden. A nanoflow has no database transaction, so an activity without a clause aborts the flow on error. `declare`, `set`, `retrieve` and `delete` take every clause; `create`, `commit`, `call nanoflow` and `call microflow` take only `on error without rollback begin … end error`; `change`, `log`, `show page`, `close page`, `show message` and `validation feedback` take none. Every other form is CE6035 at build time, and mxcli refuses it (MDL091) — measured on Mendix 11.14.0. ## SHOW and DESCRIBE diff --git a/mdl-examples/bug-tests/microflow-workflow-action-position-roundtrip.mdl b/mdl-examples/bug-tests/microflow-workflow-action-position-roundtrip.mdl index cbbdb544a..b8f2bfc6d 100644 --- a/mdl-examples/bug-tests/microflow-workflow-action-position-roundtrip.mdl +++ b/mdl-examples/bug-tests/microflow-workflow-action-position-roundtrip.mdl @@ -39,7 +39,7 @@ end; create microflow BugWfPos.Mixed ( $Workflow: System.Workflow ) returns boolean as $Ok begin lock workflow $Workflow; - unlock workflow all; + unlock workflow $Workflow unpause all; workflow operation pause $Workflow; return true; end; diff --git a/mdl-examples/bug-tests/workflow-actions-describe.mdl b/mdl-examples/bug-tests/workflow-actions-describe.mdl index fbbd5c7be..8908b1713 100644 --- a/mdl-examples/bug-tests/workflow-actions-describe.mdl +++ b/mdl-examples/bug-tests/workflow-actions-describe.mdl @@ -23,12 +23,11 @@ mdl 1; -- and DESCRIBE re-quoted what it read: 'cancelled' became '''cancelled''', -- then seven quotes a side, doubling on every round trip. -- --- NOT covered here: `lock workflow all` / `unlock workflow all`. Those write an --- activity mxbuild rejects with CE1825 "The 'Workflow' property is required" — --- a lock always needs a specific workflow definition. Writing the selection --- anyway does not help, because with `all` there is no workflow to name. That --- is a separate, still-open defect; the variable form below is the one that --- builds. +-- `lock workflow all` / `unlock workflow all` are refused (MDL-WF17): they wrote +-- an activity mxbuild rejects with CE1825 "The 'Workflow' property is +-- required" — a lock always names its workflow definition. Studio Pro's "Pause +-- instances" / "Unpause instances" options are `pause all` / `unpause all` after +-- the workflow (mendixlabs/mxcli#870). -- -- Verified on Mendix 11.13.0: 0 errors, and describe→exec→describe is stable. -- ============================================================================ @@ -67,7 +66,11 @@ begin $Records = get workflow activity records $Wf; open workflow $Wf; lock workflow $WfDef; + lock workflow $WfDef pause all; unlock workflow $WfDef; + unlock workflow $WfDef unpause all; + lock workflow W.ApproveOrder pause all; + unlock workflow W.ApproveOrder; workflow operation pause $Wf; workflow operation abort $Wf reason 'cancelled by user'; end; diff --git a/mdl/ast/ast_microflow.go b/mdl/ast/ast_microflow.go index b878a6e25..dd593b382 100644 --- a/mdl/ast/ast_microflow.go +++ b/mdl/ast/ast_microflow.go @@ -407,6 +407,12 @@ type ActivityAnnotations struct { // than silently straightening the edge. InvalidCurves []string + // InvalidAnchors holds the raw text of any @anchor parameter the visitor + // could not use — an unknown key, or a side that is not top/right/bottom/ + // left — so validation can refuse it rather than leave the edge on the + // builder's default sides in silence (mendixlabs/mxcli#992). + InvalidAnchors []string + // InvalidNotes holds the raw text of any `@annotation(...)` parameter // the visitor could not use — an unknown key, or a malformed `position:`/`size:` // pair — so validation can refuse it. Dropping it would lose the note diff --git a/mdl/ast/ast_microflow_workflow.go b/mdl/ast/ast_microflow_workflow.go index 425f94bdb..ac9fdd9a1 100644 --- a/mdl/ast/ast_microflow_workflow.go +++ b/mdl/ast/ast_microflow_workflow.go @@ -100,7 +100,13 @@ func (*OpenWorkflowStmt) isMicroflowStatement() {} // LockWorkflowStmt represents: LOCK WORKFLOW ($WorkflowVar | ALL) type LockWorkflowStmt struct { - WorkflowVariable string + WorkflowVariable string + // Workflow is the qualified name of the workflow definition, the other + // selection Studio Pro offers ("Input type: workflow document"). + Workflow string + // PauseAllWorkflows is `pause all`, Studio Pro's "Pause instances". The bare + // `lock workflow all` also sets it, with no selection — the form check and + // exec refuse (mendixlabs/mxcli#870). PauseAllWorkflows bool ErrorHandling *ErrorHandlingClause Annotations *ActivityAnnotations @@ -110,7 +116,9 @@ func (*LockWorkflowStmt) isMicroflowStatement() {} // UnlockWorkflowStmt represents: UNLOCK WORKFLOW ($WorkflowVar | ALL) type UnlockWorkflowStmt struct { - WorkflowVariable string + WorkflowVariable string + Workflow string // qualified name; see LockWorkflowStmt + // ResumeAllPausedWorkflows is `unpause all`, "Unpause instances". ResumeAllPausedWorkflows bool ErrorHandling *ErrorHandlingClause Annotations *ActivityAnnotations diff --git a/mdl/backend/mcp/microflow.go b/mdl/backend/mcp/microflow.go index 56e7a1786..b53c54305 100644 --- a/mdl/backend/mcp/microflow.go +++ b/mdl/backend/mcp/microflow.go @@ -4,6 +4,7 @@ package mcp import ( "fmt" + "reflect" "strconv" "strings" @@ -293,6 +294,12 @@ func (b *Backend) buildFlowDocContent(kind, name string, params []*microflows.Mi if !ok1 || !ok2 { return nil, fmt.Errorf("%s %q: a sequence flow references an object that is not supported yet", kind, name) } + if f.IsErrorHandler { + // PED's SequenceFlow has no property marking an error-handler + // flow, so this edge would be written as an ordinary one — a + // second normal exit from the activity (mendixlabs/mxcli#698). + return nil, fmt.Errorf("%s %q: an error handler flow cannot be authored over MCP (Studio Pro's PED API has no error-handler flow) — run without --mcp", kind, name) + } pf := map[string]any{ "originId": fmt.Sprintf("$id(%s)", op), "destinationId": fmt.Sprintf("$id(%s)", dp), @@ -437,6 +444,9 @@ func (b *Backend) mapObjectTree(o microflows.MicroflowObject, path string, idPat if err != nil { return nil, err } + if err := carryErrorHandlingType(obj.Action, action); err != nil { + return nil, err + } return map[string]any{ "$Type": "Microflows$ActionActivity", "relativeMiddlePoint": pos, @@ -1115,3 +1125,46 @@ func mfEnumName(dt microflows.DataType) string { } return "" } + +// carryErrorHandlingType writes an action's error-handling type onto its PED +// map. mapMicroflowAction built each action without it, so `on error rollback` +// or `on error continue` authored over --mcp landed with PED's default — the +// clause silently gone (mendixlabs/mxcli#698). PED declares errorHandlingType on +// every action element ('Rollback' | 'Custom' | 'CustomWithoutRollBack' | +// 'Continue' | 'Abort', ped_get_schema on Studio Pro 11.14). +// +// The two custom forms are refused: they need an error-handler flow, which +// PED's SequenceFlow cannot express. +func carryErrorHandlingType(a microflows.MicroflowAction, m map[string]any) error { + eh := actionErrorHandlingType(a) + switch eh { + case "": + return nil + case microflows.ErrorHandlingTypeCustom, microflows.ErrorHandlingTypeCustomWithoutRollback: + return fmt.Errorf("a custom error handler (`on error … begin … end error`) cannot be authored over MCP: Studio Pro's PED API has no error-handler flow — run without --mcp") + } + m["errorHandlingType"] = string(eh) + return nil +} + +var errorHandlingTypeType = reflect.TypeOf(microflows.ErrorHandlingType("")) + +// actionErrorHandlingType reads the ErrorHandlingType field every action that +// has one carries; "" for one that has none. +func actionErrorHandlingType(a microflows.MicroflowAction) microflows.ErrorHandlingType { + v := reflect.ValueOf(a) + if v.Kind() == reflect.Ptr { + if v.IsNil() { + return "" + } + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return "" + } + f := v.FieldByName("ErrorHandlingType") + if !f.IsValid() || f.Type() != errorHandlingTypeType { + return "" + } + return microflows.ErrorHandlingType(f.String()) +} diff --git a/mdl/backend/mcp/microflow_error_handling_test.go b/mdl/backend/mcp/microflow_error_handling_test.go new file mode 100644 index 000000000..db348133c --- /dev/null +++ b/mdl/backend/mcp/microflow_error_handling_test.go @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func callActivity(eh microflows.ErrorHandlingType) *microflows.ActionActivity { + a := µflows.ActionActivity{Action: µflows.MicroflowCallAction{ + ErrorHandlingType: eh, + MicroflowCall: µflows.MicroflowCall{Microflow: "M.SUB"}, + }} + a.ID = "act-1" + return a +} + +// mendixlabs/mxcli#698. The MCP mapper built the action without its +// errorHandlingType, so `call microflow … on error rollback|continue` authored +// over --mcp landed with PED's default — the clause silently lost. The PED +// schema (ped_get_schema, Studio Pro 11.14) declares errorHandlingType on every +// action element: 'Rollback' | 'Custom' | 'CustomWithoutRollBack' | 'Continue' +// | 'Abort'. +func TestMapObjectTree_CarriesErrorHandlingType(t *testing.T) { + b := &Backend{} + for _, eh := range []microflows.ErrorHandlingType{microflows.ErrorHandlingTypeRollback, microflows.ErrorHandlingTypeContinue} { + m, err := b.mapObjectTree(callActivity(eh), "/objects/0", map[model.ID]string{}) + if err != nil { + t.Fatalf("%s: %v", eh, err) + } + action := m["action"].(map[string]any) + if action["errorHandlingType"] != string(eh) { + t.Errorf("%s: action errorHandlingType = %v", eh, action["errorHandlingType"]) + } + } + // No clause: nothing written, PED's default applies — as before. + m, err := b.mapObjectTree(callActivity(""), "/objects/0", map[model.ID]string{}) + if err != nil { + t.Fatal(err) + } + if _, ok := m["action"].(map[string]any)["errorHandlingType"]; ok { + t.Error("an action with no error handling wrote one") + } +} + +// A custom handler needs an error-handler flow, and PED's SequenceFlow has no +// property for one (ped_get_schema: originId, destinationId, sides, caseValue). +// Written anyway, the handler's error edge became an ordinary flow. Refused +// rather than dropped. +func TestMapObjectTree_RefusesACustomErrorHandler(t *testing.T) { + b := &Backend{} + for _, eh := range []microflows.ErrorHandlingType{microflows.ErrorHandlingTypeCustom, microflows.ErrorHandlingTypeCustomWithoutRollback} { + if _, err := b.mapObjectTree(callActivity(eh), "/objects/0", map[model.ID]string{}); err == nil || !strings.Contains(err.Error(), "error handler") { + t.Errorf("%s: accepted (%v)", eh, err) + } + } + oc := µflows.MicroflowObjectCollection{ + Objects: []microflows.MicroflowObject{callActivity(microflows.ErrorHandlingTypeRollback), func() microflows.MicroflowObject { + e := µflows.EndEvent{} + e.ID = "end-1" + return e + }()}, + Flows: []*microflows.SequenceFlow{{OriginID: "act-1", DestinationID: "end-1", IsErrorHandler: true}}, + } + if _, err := b.buildFlowDocContent("microflow", "MF", nil, oc, µflows.VoidType{}); err == nil || !strings.Contains(err.Error(), "error handler") { + t.Errorf("an error-handler flow was written as an ordinary one (%v)", err) + } +} diff --git a/mdl/backend/modelsdk/microflow_list_error_handling_test.go b/mdl/backend/modelsdk/microflow_list_error_handling_test.go new file mode 100644 index 000000000..b16c47514 --- /dev/null +++ b/mdl/backend/modelsdk/microflow_list_error_handling_test.go @@ -0,0 +1,134 @@ +// SPDX-License-Identifier: Apache-2.0 + +package modelsdkbackend + +import ( + "fmt" + "testing" + + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// mendixlabs/mxcli#591. The writer stamped a literal "Rollback" on the list +// activities and cast, which is CE6035 in a nanoflow, whose activities store +// "Abort". The builder now supplies the flow flavour's default; this pins that +// the writer emits it and the reader returns it, and that an action with none +// keeps the historical "Rollback". +func TestListActions_ErrorHandlingTypeRoundTrips(t *testing.T) { + actions := func(eh microflows.ErrorHandlingType) []microflows.MicroflowAction { + return []microflows.MicroflowAction{ + µflows.CreateListAction{OutputVariable: "L", EntityQualifiedName: "M.E", ErrorHandlingType: eh}, + µflows.ChangeListAction{ChangeVariable: "L", Type: microflows.ChangeListTypeAdd, Value: "$P", ErrorHandlingType: eh}, + µflows.ListOperationAction{OutputVariable: "H", Operation: µflows.HeadOperation{ListVariable: "L"}, ErrorHandlingType: eh}, + µflows.AggregateListAction{InputVariable: "L", OutputVariable: "N", Function: microflows.AggregateFunctionCount, ErrorHandlingType: eh}, + µflows.CastAction{OutputVariable: "S", ErrorHandlingType: eh}, + } + } + for _, tc := range []struct { + set, want microflows.ErrorHandlingType + }{ + {microflows.ErrorHandlingTypeAbort, microflows.ErrorHandlingTypeAbort}, + {"", microflows.ErrorHandlingTypeRollback}, + } { + oc := µflows.MicroflowObjectCollection{} + for i, a := range actions(tc.set) { + setActionID(a, model.ID(fmt.Sprintf("a-%d", i))) + act := µflows.ActionActivity{Action: a} + act.ID = model.ID(fmt.Sprintf("act-%d", i)) + act.Position = model.Point{X: 100 * i, Y: 100} + oc.Objects = append(oc.Objects, act) + } + mf := µflows.Microflow{Name: "MF", ObjectCollection: oc} + mf.ID = "mf-1" + got := roundTripMicroflow(t, mf) + n := 0 + for _, obj := range got.ObjectCollection.Objects { + aa, ok := obj.(*microflows.ActionActivity) + if !ok || aa.Action == nil { + continue + } + var eh microflows.ErrorHandlingType + switch a := aa.Action.(type) { + case *microflows.CreateListAction: + eh = a.ErrorHandlingType + case *microflows.ChangeListAction: + eh = a.ErrorHandlingType + case *microflows.ListOperationAction: + eh = a.ErrorHandlingType + case *microflows.AggregateListAction: + eh = a.ErrorHandlingType + case *microflows.CastAction: + eh = a.ErrorHandlingType + default: + continue + } + n++ + if eh != tc.want { + t.Errorf("set %q: %T read back %q, want %q", tc.set, aa.Action, eh, tc.want) + } + } + if n != 5 { + t.Fatalf("set %q: %d actions survived the round trip, want 5", tc.set, n) + } + } +} + +func setActionID(a microflows.MicroflowAction, id model.ID) { + switch x := a.(type) { + case *microflows.CreateListAction: + x.ID = id + case *microflows.ChangeListAction: + x.ID = id + case *microflows.ListOperationAction: + x.ID = id + case *microflows.AggregateListAction: + x.ID = id + case *microflows.CastAction: + x.ID = id + } +} + +// mendixlabs/mxcli#870: the writer omitted the workflow selection whenever the +// "Pause instances" / "Unpause instances" flag was set, reading the flag as +// "all workflows". Studio Pro stores the flag WITH the selection +// (WorkflowCommons.ACT_WorkflowDefinition_Lock in ako/TestApp), and without +// one the activity is CE1825. +func TestLockWorkflow_FlagKeepsTheSelection(t *testing.T) { + lock := µflows.LockWorkflowAction{PauseAllWorkflows: true, WorkflowVariable: "WorkflowDefinition"} + lock.ID = "l-1" + unlock := µflows.UnlockWorkflowAction{ResumeAllPausedWorkflows: true, Workflow: "M.Approve"} + unlock.ID = "u-1" + oc := µflows.MicroflowObjectCollection{} + for i, a := range []microflows.MicroflowAction{lock, unlock} { + act := µflows.ActionActivity{Action: a} + act.ID = model.ID(fmt.Sprintf("act-%d", i)) + act.Position = model.Point{X: 100 * i, Y: 100} + oc.Objects = append(oc.Objects, act) + } + mf := µflows.Microflow{Name: "MF", ObjectCollection: oc} + mf.ID = "mf-1" + got := roundTripMicroflow(t, mf) + n := 0 + for _, obj := range got.ObjectCollection.Objects { + aa, _ := obj.(*microflows.ActionActivity) + if aa == nil { + continue + } + switch a := aa.Action.(type) { + case *microflows.LockWorkflowAction: + n++ + if !a.PauseAllWorkflows || a.WorkflowVariable != "WorkflowDefinition" { + t.Errorf("lock read back flag=%v var=%q, want true/WorkflowDefinition", a.PauseAllWorkflows, a.WorkflowVariable) + } + case *microflows.UnlockWorkflowAction: + n++ + if !a.ResumeAllPausedWorkflows || a.Workflow != "M.Approve" { + t.Errorf("unlock read back flag=%v workflow=%q, want true/M.Approve", a.ResumeAllPausedWorkflows, a.Workflow) + } + } + } + if n != 2 { + t.Fatalf("%d actions survived, want 2", n) + } +} diff --git a/mdl/backend/modelsdk/microflow_read_actions.go b/mdl/backend/modelsdk/microflow_read_actions.go index 8e49cd756..2f1bf36f2 100644 --- a/mdl/backend/modelsdk/microflow_read_actions.go +++ b/mdl/backend/modelsdk/microflow_read_actions.go @@ -197,15 +197,17 @@ func actionFromGen(el element.Element) microflows.MicroflowAction { out := µflows.CreateListAction{ EntityQualifiedName: a.EntityQualifiedName(), OutputVariable: a.OutputVariableName(), + ErrorHandlingType: microflows.ErrorHandlingType(a.ErrorHandlingType()), } out.ID = model.ID(a.ID()) return out case *genMf.ChangeListAction: out := µflows.ChangeListAction{ - ChangeVariable: a.ChangeVariableName(), - Type: microflows.ChangeListType(a.Type()), - Value: a.Value(), + ChangeVariable: a.ChangeVariableName(), + Type: microflows.ChangeListType(a.Type()), + Value: a.Value(), + ErrorHandlingType: microflows.ErrorHandlingType(a.ErrorHandlingType()), } out.ID = model.ID(a.ID()) return out @@ -223,6 +225,7 @@ func actionFromGen(el element.Element) microflows.MicroflowAction { // them back silently deleted the fold (#1004). ReduceInitialValue: a.ReduceInitialValueExpression(), ReduceReturnType: dataTypeFromGen(a.ReduceReturnDataType()), + ErrorHandlingType: microflows.ErrorHandlingType(a.ErrorHandlingType()), } out.ID = model.ID(a.ID()) return out @@ -230,7 +233,10 @@ func actionFromGen(el element.Element) microflows.MicroflowAction { case *genMf.CastAction: // ObjectVariable (the cast input) is not stored via a gen setter, so it is // not reconstructable here; OutputVariable is. - out := µflows.CastAction{OutputVariable: a.OutputVariableName()} + out := µflows.CastAction{ + OutputVariable: a.OutputVariableName(), + ErrorHandlingType: microflows.ErrorHandlingType(a.ErrorHandlingType()), + } out.ID = model.ID(a.ID()) return out @@ -369,7 +375,10 @@ func actionFromGen(el element.Element) microflows.MicroflowAction { // keys), so read both from the raw BSON — the inverse of the write's // listOperationToGen. raw := a.Raw() - out := µflows.ListOperationAction{OutputVariable: rawStr(raw, "ResultVariableName")} + out := µflows.ListOperationAction{ + OutputVariable: rawStr(raw, "ResultVariableName"), + ErrorHandlingType: microflows.ErrorHandlingType(rawStr(raw, "ErrorHandlingType")), + } out.ID = model.ID(a.ID()) if opDoc, ok := raw.Lookup("NewOperation").DocumentOK(); ok { out.Operation = listOperationFromRaw(opDoc) diff --git a/mdl/backend/modelsdk/microflow_workflow_write.go b/mdl/backend/modelsdk/microflow_workflow_write.go index 74b32f37c..a23bc9d54 100644 --- a/mdl/backend/modelsdk/microflow_workflow_write.go +++ b/mdl/backend/modelsdk/microflow_workflow_write.go @@ -86,7 +86,11 @@ func workflowMicroflowActionToGen(act microflows.MicroflowAction) element.Elemen g := newElem("Microflows$LockWorkflowAction", string(a.ID)) addStr(g, "ErrorHandlingType", orDefault(string(a.ErrorHandlingType), "Rollback")) addBool(g, "PauseAllWorkflows", a.PauseAllWorkflows) - if !a.PauseAllWorkflows { + // The flag is "Pause instances" on the selected workflow, not "all + // workflows": Studio Pro stores it WITH the selection, and leaving the + // selection out for it was CE1825 (mendixlabs/mxcli#870). Only a lock + // naming nothing — refused by check and exec — has none to write. + if a.Workflow != "" || a.WorkflowVariable != "" { addPart(g, "WorkflowSelection", workflowSelectionToGen(a.Workflow, a.WorkflowVariable)) } return g @@ -94,7 +98,7 @@ func workflowMicroflowActionToGen(act microflows.MicroflowAction) element.Elemen g := newElem("Microflows$UnlockWorkflowAction", string(a.ID)) addStr(g, "ErrorHandlingType", orDefault(string(a.ErrorHandlingType), "Rollback")) addBool(g, "ResumeAllPausedWorkflows", a.ResumeAllPausedWorkflows) - if !a.ResumeAllPausedWorkflows { + if a.Workflow != "" || a.WorkflowVariable != "" { addPart(g, "WorkflowSelection", workflowSelectionToGen(a.Workflow, a.WorkflowVariable)) } return g diff --git a/mdl/backend/modelsdk/microflow_write.go b/mdl/backend/modelsdk/microflow_write.go index a81a4df8c..a7983791e 100644 --- a/mdl/backend/modelsdk/microflow_write.go +++ b/mdl/backend/modelsdk/microflow_write.go @@ -702,7 +702,7 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { // Storage $Type Microflows$CastAction; output bound to "VariableName". g := genMf.NewCastAction() g.SetID(element.ID(a.ID)) - g.SetErrorHandlingType("Rollback") + g.SetErrorHandlingType(orDefault(string(a.ErrorHandlingType), "Rollback")) g.SetOutputVariableName(a.OutputVariable) return g case *microflows.AggregateListAction: @@ -711,7 +711,7 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { // by-name ref. Expression mode is mutually exclusive with Attribute. g := genMf.NewAggregateListAction() g.SetID(element.ID(a.ID)) - g.SetErrorHandlingType("Rollback") + g.SetErrorHandlingType(orDefault(string(a.ErrorHandlingType), "Rollback")) g.SetAggregateFunction(string(a.Function)) g.SetInputListVariableName(a.InputVariable) if a.UseExpression { @@ -735,7 +735,10 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { // Storage $Type Microflows$CreateListAction; output bound to "VariableName". g := genMf.NewCreateListAction() g.SetID(element.ID(a.ID)) - g.SetErrorHandlingType("Rollback") + // The flow flavour's default, which the builder supplies: Rollback in a + // microflow, Abort in a nanoflow — "Rollback" there is CE6035 + // (mendixlabs/mxcli#591). Same for the other list activities and cast. + g.SetErrorHandlingType(orDefault(string(a.ErrorHandlingType), "Rollback")) if a.EntityQualifiedName != "" { g.SetEntityQualifiedName(a.EntityQualifiedName) } @@ -747,7 +750,7 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { // when called, so guard it the same way. g := genMf.NewChangeListAction() g.SetID(element.ID(a.ID)) - g.SetErrorHandlingType("Rollback") + g.SetErrorHandlingType(orDefault(string(a.ErrorHandlingType), "Rollback")) g.SetChangeVariableName(a.ChangeVariable) g.SetType(string(a.Type)) if a.Value != "" { @@ -780,7 +783,7 @@ func microflowActionToGen(action microflows.MicroflowAction) element.Element { // "VariableName"), so this action and its operation sub-elements are // built directly with the verified legacy BSON keys. e := newElem("Microflows$ListOperationsAction", string(a.ID)) - addStr(e, "ErrorHandlingType", "Rollback") + addStr(e, "ErrorHandlingType", orDefault(string(a.ErrorHandlingType), "Rollback")) if a.Operation != nil { addPart(e, "NewOperation", listOperationToGen(a.Operation)) } diff --git a/mdl/executor/cmd_microflows_builder_actions.go b/mdl/executor/cmd_microflows_builder_actions.go index 24dd44f0d..f0ca35929 100644 --- a/mdl/executor/cmd_microflows_builder_actions.go +++ b/mdl/executor/cmd_microflows_builder_actions.go @@ -1060,9 +1060,10 @@ func qualifiedNameString(qn ast.QualifiedName) string { func (fb *flowBuilder) addCastAction(s *ast.CastObjectStmt) model.ID { action := µflows.CastAction{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - ObjectVariable: s.ObjectVariable, - OutputVariable: s.OutputVariable, + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + ObjectVariable: s.ObjectVariable, + OutputVariable: s.OutputVariable, + ErrorHandlingType: fb.ehType(nil), } activity := µflows.ActionActivity{ @@ -1774,9 +1775,10 @@ func (fb *flowBuilder) addListOperationAction(s *ast.ListOperationStmt) model.ID } action := µflows.ListOperationAction{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - Operation: operation, - OutputVariable: s.OutputVariable, + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Operation: operation, + OutputVariable: s.OutputVariable, + ErrorHandlingType: fb.ehType(nil), } // Track output variable type for operations that preserve/produce list types @@ -1902,10 +1904,11 @@ func (fb *flowBuilder) addAggregateListAction(s *ast.AggregateListStmt) model.ID } action := µflows.AggregateListAction{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - InputVariable: s.InputVariable, - OutputVariable: s.OutputVariable, - Function: function, + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + InputVariable: s.InputVariable, + OutputVariable: s.OutputVariable, + Function: function, + ErrorHandlingType: fb.ehType(nil), } // The fold Mendix stores beside the expression. REDUCE names both in MDL; @@ -1968,6 +1971,7 @@ func (fb *flowBuilder) addCreateListAction(s *ast.CreateListStmt) model.ID { BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, OutputVariable: s.Variable, EntityQualifiedName: entityQN, + ErrorHandlingType: fb.ehType(nil), } // Register variable type as list @@ -1999,10 +2003,11 @@ func (fb *flowBuilder) addAddToListAction(s *ast.AddToListStmt) model.ID { value = "$" + s.Item } action := µflows.ChangeListAction{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - Type: microflows.ChangeListTypeAdd, - ChangeVariable: s.List, - Value: value, + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Type: microflows.ChangeListTypeAdd, + ChangeVariable: s.List, + Value: value, + ErrorHandlingType: fb.ehType(nil), } activity := µflows.ActionActivity{ @@ -2025,10 +2030,11 @@ func (fb *flowBuilder) addAddToListAction(s *ast.AddToListStmt) model.ID { // addRemoveFromListAction creates a REMOVE FROM list statement. func (fb *flowBuilder) addRemoveFromListAction(s *ast.RemoveFromListStmt) model.ID { action := µflows.ChangeListAction{ - BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, - Type: microflows.ChangeListTypeRemove, - ChangeVariable: s.List, - Value: "$" + s.Item, + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Type: microflows.ChangeListTypeRemove, + ChangeVariable: s.List, + Value: "$" + s.Item, + ErrorHandlingType: fb.ehType(nil), } activity := µflows.ActionActivity{ diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index 84e8e0458..7799b7cbd 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -759,7 +759,14 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in var lastErrID model.ID var lastErrCase string var lastErrAnchor *ast.FlowAnchors + // prevOwnAnchor is the @anchor of the handler statement before this one, + // whose `from:` is the side the edge between them leaves. The loop below + // built its edges with the default sides and never applied either end, so + // an @anchor inside a handler parsed, passed check and exec, and did + // nothing (mendixlabs/mxcli#991). + var prevOwnAnchor *ast.FlowAnchors for _, stmt := range errorBody { + thisAnchor := stmtOwnAnchor(stmt) actID := errBuilder.addStatement(stmt) if errBuilder.pendingJoin != nil { // A handler whose FIRST statement is a join has no activity of its @@ -782,11 +789,18 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in if actID != "" { errBuilder.applyPendingAnnotations(actID) if lastErrID == "" { - // Connect source activity to first error handler activity - fb.flows = append(fb.flows, newErrorHandlerFlow(sourceActivityID, actID)) + // Connect source activity to first error handler activity. The + // statement's `to:` is the side its incoming edge — this one — + // enters; the side the edge leaves the source has no spelling. + flow := newErrorHandlerFlow(sourceActivityID, actID) + applyUserAnchors(flow, nil, thisAnchor) + fb.flows = append(fb.flows, flow) } else { - errBuilder.flows = append(errBuilder.flows, newHorizontalFlow(lastErrID, actID)) + flow := newHorizontalFlow(lastErrID, actID) + applyUserAnchors(flow, prevOwnAnchor, thisAnchor) + errBuilder.flows = append(errBuilder.flows, flow) } + prevOwnAnchor = thisAnchor if errBuilder.nextConnectionPoint != "" { lastErrID = errBuilder.nextConnectionPoint lastErrCase = errBuilder.nextFlowCase @@ -794,10 +808,15 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in errBuilder.nextConnectionPoint = "" errBuilder.nextFlowCase = "" errBuilder.nextFlowAnchor = nil + // A compound statement steers its own exits. + prevOwnAnchor = nil } else { lastErrID = actID lastErrCase = "" - lastErrAnchor = nil + // The edge that rejoins the main flow leaves this statement, so + // it takes the statement's `from:` — and only that: its `to:` + // belongs to the edge coming in. + lastErrAnchor = originOnly(thisAnchor) } } } @@ -812,6 +831,16 @@ func (fb *flowBuilder) addErrorHandlerFlow(sourceActivityID model.ID, sourceX in fb.flows = append(fb.flows, errBuilder.flows...) fb.annotationFlows = append(fb.annotationFlows, errBuilder.annotationFlows...) fb.errors = append(fb.errors, errBuilder.errors...) + // A @curve inside the handler is recorded on errBuilder, and applied by the + // builder that owns the flows once its graph is complete — this one, since + // the handler's flows (and the edge that rejoins the main flow) land here. + // Left on errBuilder it was never applied (mendixlabs/mxcli#991). + for id, c := range errBuilder.curveByOrigin { + if fb.curveByOrigin == nil { + fb.curveByOrigin = map[model.ID]*ast.FlowCurve{} + } + fb.curveByOrigin[id] = c + } if fb.annotationsByLabel == nil { fb.annotationsByLabel = errBuilder.annotationsByLabel } @@ -971,6 +1000,15 @@ func applyUserAnchors(flow *microflows.SequenceFlow, origin *ast.FlowAnchors, de } } +// originOnly is an anchor's `from:` alone, for an edge whose destination the +// statement does not own. +func originOnly(a *ast.FlowAnchors) *ast.FlowAnchors { + if a == nil || a.From == ast.AnchorSideUnset { + return nil + } + return &ast.FlowAnchors{From: a.From, To: ast.AnchorSideUnset} +} + func branchDestinationAnchor(branchAnchor, stmtAnchor *ast.FlowAnchors) *ast.FlowAnchors { // The split branch annotation owns the incoming edge to the first branch // activity. If it specifies `to`, it must win over the first statement's diff --git a/mdl/executor/cmd_microflows_builder_workflow.go b/mdl/executor/cmd_microflows_builder_workflow.go index b5b32bba4..76294cded 100644 --- a/mdl/executor/cmd_microflows_builder_workflow.go +++ b/mdl/executor/cmd_microflows_builder_workflow.go @@ -210,6 +210,7 @@ func (fb *flowBuilder) addLockWorkflowAction(s *ast.LockWorkflowStmt) model.ID { ErrorHandlingType: convertErrorHandlingType(s.ErrorHandling), PauseAllWorkflows: s.PauseAllWorkflows, WorkflowVariable: s.WorkflowVariable, + Workflow: s.Workflow, } return fb.wrapAction(action, s.ErrorHandling) } @@ -220,6 +221,7 @@ func (fb *flowBuilder) addUnlockWorkflowAction(s *ast.UnlockWorkflowStmt) model. ErrorHandlingType: convertErrorHandlingType(s.ErrorHandling), ResumeAllPausedWorkflows: s.ResumeAllPausedWorkflows, WorkflowVariable: s.WorkflowVariable, + Workflow: s.Workflow, } return fb.wrapAction(action, s.ErrorHandling) } diff --git a/mdl/executor/cmd_microflows_format_action.go b/mdl/executor/cmd_microflows_format_action.go index 559b10e39..d09161ca2 100644 --- a/mdl/executor/cmd_microflows_format_action.go +++ b/mdl/executor/cmd_microflows_format_action.go @@ -1003,22 +1003,10 @@ func formatAction( return fmt.Sprintf("open workflow $%s;", a.WorkflowVariable) case *microflows.LockWorkflowAction: - if a.PauseAllWorkflows { - return "lock workflow all;" - } - if a.Workflow != "" { - return fmt.Sprintf("lock workflow %s;", a.Workflow) - } - return fmt.Sprintf("lock workflow $%s;", a.WorkflowVariable) + return "lock workflow " + workflowSelectionMDL(a.Workflow, a.WorkflowVariable, a.PauseAllWorkflows, "pause all") + ";" case *microflows.UnlockWorkflowAction: - if a.ResumeAllPausedWorkflows { - return "unlock workflow all;" - } - if a.Workflow != "" { - return fmt.Sprintf("unlock workflow %s;", a.Workflow) - } - return fmt.Sprintf("unlock workflow $%s;", a.WorkflowVariable) + return "unlock workflow " + workflowSelectionMDL(a.Workflow, a.WorkflowVariable, a.ResumeAllPausedWorkflows, "unpause all") + ";" case *microflows.JavaScriptActionCallAction: jsActionName := a.JavaScriptAction @@ -2206,3 +2194,25 @@ func templateArgsClause(ctx *ExecContext, args []string) string { } return " with (" + strings.Join(parts, ", ") + ")" } + +// workflowSelectionMDL spells a lock/unlock's workflow and its instances flag. +// The flag is Studio Pro's "Pause instances" / "Unpause instances" on the +// selected workflow, and was printed as a bare `all` in place of the workflow +// — a statement that dropped the selection and rebuilt as CE1825 +// (mendixlabs/mxcli#870). A stored activity with the flag and no selection +// still prints as `all`, which check and exec refuse: it has no buildable form. +func workflowSelectionMDL(workflow, variable string, flag bool, flagWords string) string { + var sel string + switch { + case workflow != "": + sel = workflow + case variable != "": + sel = "$" + variable + default: + return "all" + } + if flag { + sel += " " + flagWords + } + return sel +} diff --git a/mdl/executor/cmd_microflows_show_helpers.go b/mdl/executor/cmd_microflows_show_helpers.go index 839dfe777..e9bc1db39 100644 --- a/mdl/executor/cmd_microflows_show_helpers.go +++ b/mdl/executor/cmd_microflows_show_helpers.go @@ -337,15 +337,22 @@ func emitAnchorAnnotationWithActivityMap( break } } + defaultTo := anchorSideKeyword(AnchorLeft) if incoming := flowsByDest[id]; len(incoming) > 0 { to = anchorSideKeyword(incoming[0].DestinationConnectionIndex) + // An error handler's first activity is entered by the error edge, which + // the builder draws into its TOP (newErrorHandlerFlow). Judged against + // the left side, `to: left` there read as the default and was left out, + // and the rebuild entered the top (mendixlabs/mxcli#991). + if len(incoming) == 1 && incoming[0].IsErrorHandler { + defaultTo = anchorSideKeyword(AnchorTop) + } } if from == "" && to == "" { return } defaultFrom := anchorSideKeyword(AnchorRight) - defaultTo := anchorSideKeyword(AnchorLeft) var parts []string if from != "" && from != defaultFrom { parts = append(parts, "from: "+from) @@ -2491,13 +2498,21 @@ func collectErrorHandlerStatementSpans( visited := make(map[model.ID]bool) stopID := firstReachableErrorHandlerMerge(startID, activityMap, flowsByOrigin) - // A note on a handler-body activity is emitted here or nowhere: this + // A handler-body activity's annotations are emitted here or nowhere: this // traversal is a second, smaller describer and the main one never reaches - // inside an `on error begin … end error` block. Without it the write path attaches the - // note and the read path drops it, which is the same round-trip loss #1077 - // is about, one nesting level down. + // inside an `on error begin … end error` block. It used to emit the notes + // alone (#1077) — so @position, @anchor, @curve, @caption, @color and + // @excluded, all of which the builder honours in a handler, came back + // missing and describe → exec put a laid-out handler back on + // auto-placement (mendixlabs/mxcli#991). Same emitter as the main path. + flowsByDest := map[model.ID][]*microflows.SequenceFlow{} + for _, flows := range flowsByOrigin { + for _, f := range flows { + flowsByDest[f.DestinationID] = append(flowsByDest[f.DestinationID], f) + } + } notes := func(obj microflows.MicroflowObject, indentStr string) { - statements = append(statements, annotationsByTarget.lines(obj.GetID(), obj.GetPosition(), objectHeight(obj), indentStr)...) + emitObjectAnnotations(obj, &statements, indentStr, annotationsByTarget, flowsByOrigin, flowsByDest, activityMap) } splitMergeMap := findErrorHandlerSplitMergePoints(ctx, activityMap, flowsByOrigin) diff --git a/mdl/executor/cmd_microflows_traverse_test.go b/mdl/executor/cmd_microflows_traverse_test.go index 70c303dad..b0d326542 100644 --- a/mdl/executor/cmd_microflows_traverse_test.go +++ b/mdl/executor/cmd_microflows_traverse_test.go @@ -1056,7 +1056,9 @@ func TestCollectErrorHandlerStatements_Simple(t *testing.T) { mkID("err_log"): {mkFlow("err_log", "err_end")}, } - stmts := e.collectErrorHandlerStatements(mkID("err_log"), activityMap, flowsByOrigin, nil, nil, nil) + // Annotation lines are left out: a handler-body statement now carries its + // @position/@anchor like any other (mendixlabs/mxcli#991). + stmts := statementLines(e.collectErrorHandlerStatements(mkID("err_log"), activityMap, flowsByOrigin, nil, nil, nil)) if len(stmts) != 2 { t.Fatalf("expected 2 statements, got %d: %v", len(stmts), stmts) } @@ -1085,7 +1087,7 @@ func TestCollectErrorHandlerStatements_StopsAtMerge(t *testing.T) { mkID("merge"): {mkFlow("merge", "after")}, } - stmts := e.collectErrorHandlerStatements(mkID("err_log"), activityMap, flowsByOrigin, nil, nil, nil) + stmts := statementLines(e.collectErrorHandlerStatements(mkID("err_log"), activityMap, flowsByOrigin, nil, nil, nil)) // Should stop at merge, not include "after" if len(stmts) != 1 { t.Fatalf("expected 1 statement (stop at merge), got %d: %v", len(stmts), stmts) @@ -1587,3 +1589,14 @@ func TestTraverseFlow_InheritanceSplitOmitsEmptyElse(t *testing.T) { t.Errorf("an (empty) branch WITH a body must still render:\n%s", out) } } + +// statementLines drops the @annotation lines from described statements. +func statementLines(lines []string) []string { + var out []string + for _, l := range lines { + if !strings.HasPrefix(strings.TrimSpace(l), "@") { + out = append(out, l) + } + } + return out +} diff --git a/mdl/executor/flow_handler_annotations_test.go b/mdl/executor/flow_handler_annotations_test.go new file mode 100644 index 000000000..efcbb2c1a --- /dev/null +++ b/mdl/executor/flow_handler_annotations_test.go @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +func buildCollectionFromMDL(t *testing.T, nanoflow bool, src string) *microflows.MicroflowObjectCollection { + t.Helper() + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatal(errs) + } + var body []ast.MicroflowStatement + switch s := prog.Statements[len(prog.Statements)-1].(type) { + case *ast.CreateMicroflowStmt: + body = s.Body + case *ast.CreateNanoflowStmt: + body = s.Body + } + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing, isNanoflow: nanoflow} + return fb.buildFlowGraph(body, nil) +} + +func objectAtPos(oc *microflows.MicroflowObjectCollection, x, y int) model.ID { + for _, o := range oc.Objects { + if p := o.GetPosition(); p.X == x && p.Y == y { + return o.GetID() + } + } + return "" +} + +// mendixlabs/mxcli#991. Inside `on error … begin … end error` the statements' +// @position was honoured but @anchor and @curve were not: the handler loop +// created its flows with the builder's default sides and never recorded a +// curve, so check and exec passed and the edges came out straight, on default +// sides. They now mean what they mean outside a handler — @anchor's `to:` is +// the side the statement's incoming edge enters (here, the error edge), `from:` +// and @curve shape the edge leaving it. +func TestErrorHandlerBody_HonoursAnchorAndCurve(t *testing.T) { + src := `mdl 1; +create nanoflow M.NF () +begin + @position(240, 200) + call microflow M.SUB() on error without rollback begin + @position(240, 380) + @anchor(from: right, to: left) + @curve(from: (0, 30), to: (0, -30)) + log error node 'R' 'one'; + @position(440, 380) + @anchor(to: top) + log error node 'R' 'two'; + end error; + @position(560, 200) + return; +end;` + oc := buildCollectionFromMDL(t, true, src) + one, two := objectAtPos(oc, 240, 380), objectAtPos(oc, 440, 380) + if one == "" || two == "" { + t.Fatal("handler activities not at their @position") + } + var errEdge, between *microflows.SequenceFlow + for _, f := range oc.Flows { + switch { + case f.IsErrorHandler && f.DestinationID == one: + errEdge = f + case f.OriginID == one && f.DestinationID == two: + between = f + } + } + if errEdge == nil || between == nil { + t.Fatalf("missing flows: error edge %v, between %v", errEdge, between) + } + if errEdge.DestinationConnectionIndex != AnchorLeft { + t.Errorf("error edge enters side %d, want left (%d) from @anchor(to: left)", errEdge.DestinationConnectionIndex, AnchorLeft) + } + if between.OriginConnectionIndex != AnchorRight || between.DestinationConnectionIndex != AnchorTop { + t.Errorf("handler edge is (%d,%d), want (right,top) = (%d,%d)", between.OriginConnectionIndex, between.DestinationConnectionIndex, AnchorRight, AnchorTop) + } + if between.OriginControlVector != "0;30" || between.DestinationControlVector != "0;-30" { + t.Errorf("handler edge curve is %q/%q, want 0;30/0;-30", between.OriginControlVector, between.DestinationControlVector) + } +} + +// The last handler statement's @anchor(from:) and @curve shape the edge that +// rejoins the main flow; its `to:` stays on its own incoming edge. +func TestErrorHandlerBody_TailEdgeTakesTheLastStatementsFromAndCurve(t *testing.T) { + src := `mdl 1; +create nanoflow M.NF () +begin + @position(240, 200) + call microflow M.SUB() on error without rollback begin + @position(240, 380) + @anchor(from: top, to: left) + @curve(from: (0, -30), to: (-30, 0)) + log error node 'R' 'one'; + end error; + @position(560, 200) + return; +end;` + oc := buildCollectionFromMDL(t, true, src) + one := objectAtPos(oc, 240, 380) + var tail *microflows.SequenceFlow + for _, f := range oc.Flows { + if f.OriginID == one && !f.IsErrorHandler { + tail = f + } + } + if tail == nil { + t.Fatal("no edge leaves the handler") + } + if tail.OriginConnectionIndex != AnchorTop { + t.Errorf("rejoin edge leaves side %d, want top (%d)", tail.OriginConnectionIndex, AnchorTop) + } + if tail.DestinationConnectionIndex == AnchorLeft { + t.Errorf("the statement's `to:` leaked onto its outgoing edge") + } + if tail.OriginControlVector != "0;-30" || tail.DestinationControlVector != "-30;0" { + t.Errorf("rejoin edge curve is %q/%q, want 0;-30/-30;0", tail.OriginControlVector, tail.DestinationControlVector) + } +} + +// mendixlabs/mxcli#992, end to end: the one-sided per-case form describe emits +// now reaches the true edge. +func TestSplitBranch_OneSidedAnchorReachesTheEdge(t *testing.T) { + src := `mdl 1; +create nanoflow M.NF ($Q: Integer) +begin + @position(360, 200) + @anchor(true: (to: top)) + if $Q = 0 then + @position(560, 80) + log info node 'a' 'b'; + else + @position(560, 320) + log info node 'a' 'c'; + end if; +end;` + oc := buildCollectionFromMDL(t, true, src) + dest := objectAtPos(oc, 560, 80) + for _, f := range oc.Flows { + if f.DestinationID == dest { + if f.DestinationConnectionIndex != AnchorTop { + t.Errorf("true edge enters side %d, want top (%d)", f.DestinationConnectionIndex, AnchorTop) + } + return + } + } + t.Fatal("no true edge") +} + +// mendixlabs/mxcli#991, the read half. DESCRIBE printed no @position — nor +// @anchor, @curve, @caption, @color or @excluded — for a statement inside an +// error handler, though the builder honours each there. So describe → exec of a +// flow whose handler someone laid out put the handler back on auto-placement, +// reporting success. The handler body now carries the same annotations as the +// main path, and a second describe is a fixed point. +func TestErrorHandlerBody_DescribeKeepsItsLayout(t *testing.T) { + src := `mdl 1; +create microflow M.F () +begin + @position(240, 200) + call microflow M.SUB() on error without rollback begin + @position(240, 380) + @anchor(from: right, to: left) + @curve(from: (0, 30), to: (0, -30)) + @caption 'Handled' + @color Red + log error node 'R' 'one'; + end error; + @position(560, 200) + return; +end;` + oc := buildCollectionFromMDL(t, false, src) + body := describeBody(t, µflows.Microflow{ObjectCollection: oc}) + for _, want := range []string{"@position(240, 380)", "to: left", "@curve(from: (0, 30), to: (0, -30))", "@caption 'Handled'", "@color Red"} { + if !strings.Contains(body, want) { + t.Errorf("describe dropped %q inside the handler:\n%s", want, body) + } + } + again := buildCollectionFromMDL(t, false, "mdl 1;\ncreate microflow M.F ()\nbegin\n"+body+"\nend;") + if second := describeBody(t, µflows.Microflow{ObjectCollection: again}); second != body { + t.Errorf("describe is not a fixed point:\n--- first\n%s\n--- second\n%s", body, second) + } +} diff --git a/mdl/executor/lock_workflow_selection_test.go b/mdl/executor/lock_workflow_selection_test.go new file mode 100644 index 000000000..faa6ce4de --- /dev/null +++ b/mdl/executor/lock_workflow_selection_test.go @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// mendixlabs/mxcli#870. A Lock workflow activity always names a workflow +// definition; PauseAllWorkflows is Studio Pro's "Pause instances" checkbox +// (Unlock's is "Unpause instances", ResumeAllPausedWorkflows), on by default. +// WorkflowCommons.ACT_WorkflowDefinition_Lock in ako/TestApp stores both +// branches of that choice: PauseAllWorkflows true WITH a WorkflowDefinition +// variable selection, and false with the same selection. +// +// MDL read the flag as "all workflows": `lock workflow all;` wrote the flag and +// no selection, which mxbuild rejects with CE1825 "The 'Workflow' property is +// required", and DESCRIBE printed the Studio Pro activity as that same +// `lock workflow all;` — dropping the workflow, so describe → exec turned a +// working microflow into one that does not build. + +func lockStmt(t *testing.T, src string) ast.MicroflowStatement { + t.Helper() + prog, errs := visitor.Build("mdl 1;\ncreate microflow M.MF ($Wf: System.WorkflowDefinition)\nbegin\n" + src + "\nend;") + if len(errs) > 0 { + t.Fatalf("%s: %v", src, errs) + } + return prog.Statements[len(prog.Statements)-1].(*ast.CreateMicroflowStmt).Body[0] +} + +func TestLockWorkflow_PauseInstancesIsAFlagOnASelection(t *testing.T) { + for _, tc := range []struct { + src string + variable, name string + flag bool + }{ + {"lock workflow $Wf;", "Wf", "", false}, + {"lock workflow $Wf pause all;", "Wf", "", true}, + {"lock workflow M.Approve pause all;", "", "M.Approve", true}, + {"unlock workflow $Wf;", "Wf", "", false}, + {"unlock workflow $Wf unpause all;", "Wf", "", true}, + {"unlock workflow M.Approve;", "", "M.Approve", false}, + } { + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing} + oc := fb.buildFlowGraph([]ast.MicroflowStatement{lockStmt(t, tc.src)}, nil) + var variable, name string + var flag, found bool + for _, o := range oc.Objects { + aa, ok := o.(*microflows.ActionActivity) + if !ok { + continue + } + switch a := aa.Action.(type) { + case *microflows.LockWorkflowAction: + variable, name, flag, found = a.WorkflowVariable, a.Workflow, a.PauseAllWorkflows, true + case *microflows.UnlockWorkflowAction: + variable, name, flag, found = a.WorkflowVariable, a.Workflow, a.ResumeAllPausedWorkflows, true + } + } + if !found { + t.Errorf("%s: no action built", tc.src) + continue + } + if variable != tc.variable || name != tc.name || flag != tc.flag { + t.Errorf("%s: built var=%q name=%q flag=%v, want var=%q name=%q flag=%v", + tc.src, variable, name, flag, tc.variable, tc.name, tc.flag) + } + } +} + +// The Studio Pro activity describes as a statement that rebuilds it. +func TestLockWorkflow_DescribeKeepsTheSelection(t *testing.T) { + for _, tc := range []struct { + action microflows.MicroflowAction + want string + }{ + {µflows.LockWorkflowAction{PauseAllWorkflows: true, WorkflowVariable: "WorkflowDefinition"}, "lock workflow $WorkflowDefinition pause all;"}, + {µflows.LockWorkflowAction{WorkflowVariable: "WorkflowDefinition"}, "lock workflow $WorkflowDefinition;"}, + {µflows.LockWorkflowAction{PauseAllWorkflows: true, Workflow: "M.Approve"}, "lock workflow M.Approve pause all;"}, + {µflows.UnlockWorkflowAction{ResumeAllPausedWorkflows: true, WorkflowVariable: "WorkflowDefinition"}, "unlock workflow $WorkflowDefinition unpause all;"}, + {µflows.UnlockWorkflowAction{Workflow: "M.Approve"}, "unlock workflow M.Approve;"}, + } { + if got := formatAction(&ExecContext{}, tc.action, nil, nil); got != tc.want { + t.Errorf("describe: got %q, want %q", got, tc.want) + } + // ... and the description parses back to the same activity. + lockStmt(t, tc.want) + } +} + +// `lock workflow all` names no workflow, and there is no model for it: refused +// at check and exec rather than written into a microflow that cannot build. +func TestLockWorkflow_AllWithoutAWorkflowIsRefused(t *testing.T) { + for _, src := range []string{"lock workflow all;", "unlock workflow all;"} { + prog, errs := visitor.Build("create microflow M.MF ()\nbegin\n" + src + "\nend;") + if len(errs) > 0 { + t.Fatal(errs) + } + mf := prog.Statements[len(prog.Statements)-1].(*ast.CreateMicroflowStmt) + if vs := ValidateMicroflow(mf); !hasErrorRule(vs, lockWorkflowAllRule) { + t.Errorf("%s: check passed it: %+v", src, vs) + } + if err := validateMicroflowRules(mf); err == nil { + t.Errorf("%s: exec accepted it", src) + } + } + // CONTROL: a lock that names its workflow is clean. + prog, _ := visitor.Build("create microflow M.MF ($Wf: System.WorkflowDefinition)\nbegin\nlock workflow $Wf pause all;\nunlock workflow $Wf;\nend;") + if vs := ValidateMicroflow(prog.Statements[len(prog.Statements)-1].(*ast.CreateMicroflowStmt)); hasErrorRule(vs, lockWorkflowAllRule) { + t.Errorf("a lock naming its workflow was refused: %+v", vs) + } +} diff --git a/mdl/executor/microflow_error_handler_authoring_test.go b/mdl/executor/microflow_error_handler_authoring_test.go index 2a9442a38..0a4ad09c6 100644 --- a/mdl/executor/microflow_error_handler_authoring_test.go +++ b/mdl/executor/microflow_error_handler_authoring_test.go @@ -368,3 +368,44 @@ func containsSubstringAny(errs []string, want string) bool { } return false } + +// mendixlabs/mxcli#591: `call nanoflow … on error continue` in a nanoflow passed +// check and exec, then failed the build with CE6035. MEASURED on 11.14.0 (an +// ako/TestApp copy, one nanoflow per cell): create, commit, call nanoflow and +// call microflow each accept only a handler WITHOUT rollback in a nanoflow — +// `on error continue`, `on error rollback` and a custom handler with rollback +// are all CE6035. Retrieve and delete accepted all four, and are the control. +func TestNanoflow_RefusesAllButWithoutRollbackOnCreateCommitAndCalls(t *testing.T) { + stmts := map[string]string{ + "create": "$O = create M.Car (Brand = 'x')", + "commit": "commit $Car", + "call nanoflow": "call nanoflow M.NF_Sub()", + "call microflow": "call microflow M.MF_Sub()", + } + refused := map[string]string{ + "continue": " on error continue;", + "rollback": " on error rollback;", + "custom": " on error begin log info node 'B' 'e'; end error;", + } + for name, stmt := range stmts { + for form, clause := range refused { + if errs := nanoflowErrorsFor(t, stmt+clause); !containsSubstringAny(errs, "on error") { + t.Errorf("%s with %s was accepted in a nanoflow, but mxbuild reports CE6035: %v", name, form, errs) + } + } + // The one handler the four accept, and no clause at all. + for _, ok := range []string{" on error without rollback begin log info node 'B' 'e'; end error;", ";"} { + if errs := nanoflowErrorsFor(t, stmt+ok); containsSubstringAny(errs, "on error") { + t.Errorf("%s%s was refused, but it builds: %v", name, ok, errs) + } + } + } + // CONTROL: retrieve and delete accept every form in a nanoflow. + for _, stmt := range []string{"retrieve $L from M.Car", "delete $Car"} { + for _, clause := range refused { + if errs := nanoflowErrorsFor(t, stmt+clause); containsSubstringAny(errs, "on error") { + t.Errorf("%s%s was refused, but it builds: %v", stmt, clause, errs) + } + } + } +} diff --git a/mdl/executor/microflow_error_handling_test.go b/mdl/executor/microflow_error_handling_test.go index dcab0d12f..3df7afec9 100644 --- a/mdl/executor/microflow_error_handling_test.go +++ b/mdl/executor/microflow_error_handling_test.go @@ -134,3 +134,43 @@ func TestMDL076_OnlyContinueIsRefused(t *testing.T) { } } } + +// mendixlabs/mxcli#175. `call workflow … on error continue` passed check and +// exec, then failed the build: CE6035 "Error handling type is not supported" at +// Call workflow activity. MEASURED on 11.14.0 (ako/TestApp, one microflow per +// clause): continue is CE6035; no clause, rollback, a custom handler and a +// custom handler without rollback all build at 0 errors. +func TestMDL076_ReportsContinueOnCallWorkflow(t *testing.T) { + v := µflowValidator{mfName: "M.ACT_X"} + v.checkErrorHandlingContinueSupported(&ast.CallWorkflowStmt{ + ErrorHandling: &ast.ErrorHandlingClause{Type: ast.ErrorHandlingContinue}, + }) + if len(v.violations) != 1 || v.violations[0].RuleID != continueUnsupportedRule { + t.Fatalf("`call workflow … on error continue` was accepted: %+v", v.violations) + } +} + +// CONTROL for the above: every other form was measured to build, so refusing +// one would reject a working script. +func TestMDL076_CallWorkflowAcceptsTheOtherClauses(t *testing.T) { + for _, eh := range []*ast.ErrorHandlingClause{ + nil, + {Type: ast.ErrorHandlingRollback}, + {Type: ast.ErrorHandlingCustom}, + {Type: ast.ErrorHandlingCustomWithoutRollback}, + } { + v := µflowValidator{mfName: "M.ACT_X"} + v.checkErrorHandlingContinueSupported(&ast.CallWorkflowStmt{ErrorHandling: eh}) + if len(v.violations) != 0 { + t.Errorf("clause %+v was reported on a call workflow: %+v", eh, v.violations) + } + } +} + +// MDL076 is a rule exec enforces, not only check: a script run with --no-check, +// through -c or the REPL wrote the CE6035 activity before. +func TestMDL076_IsExecEnforced(t *testing.T) { + if !execEnforcedMicroflowRules[continueUnsupportedRule] { + t.Fatalf("%s is reported by check but not refused by exec", continueUnsupportedRule) + } +} diff --git a/mdl/executor/nanoflow_list_error_handling_test.go b/mdl/executor/nanoflow_list_error_handling_test.go new file mode 100644 index 000000000..cf8022e07 --- /dev/null +++ b/mdl/executor/nanoflow_list_error_handling_test.go @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// mendixlabs/mxcli#591. The list activities — create list, add to / remove from +// list, list operations, aggregates and cast — carried no error-handling type +// at all, and both writers stamped a literal "Rollback" on them. In a NANOFLOW +// that is CE6035 "Error handling type is not supported" on every one of them, +// measured on 11.14.0 (ako/TestApp copy, one nanoflow per activity): Create list, +// Change list (add and remove), Aggregate list and List operation (head, filter, +// sort) all failed. Studio Pro stores "Abort" on every action of every nanoflow +// in ako/TestApp (130 actions over 11 nanoflows), and "Rollback" on the same +// actions in a microflow — so the value is the flow flavour's default, which is +// what the builder now supplies. +func TestListActions_ErrorHandlingFollowsTheFlowFlavour(t *testing.T) { + src := `mdl 1; +create microflow M.F ($Products: List of M.Product, $P: M.Product) +begin + $L = create list of M.Product; + add $P to $L; + remove $P from $L; + $H = head $Products; + $N = count $Products; +end;` + prog, errs := visitor.Build(src) + if len(errs) > 0 { + t.Fatal(errs) + } + body := prog.Statements[len(prog.Statements)-1].(*ast.CreateMicroflowStmt).Body + + for _, tc := range []struct { + nanoflow bool + want microflows.ErrorHandlingType + }{ + {true, microflows.ErrorHandlingTypeAbort}, + {false, microflows.ErrorHandlingTypeRollback}, + } { + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing, isNanoflow: tc.nanoflow} + oc := fb.buildFlowGraph(body, nil) + seen := 0 + for _, obj := range oc.Objects { + aa, ok := obj.(*microflows.ActionActivity) + if !ok { + continue + } + var got microflows.ErrorHandlingType + switch a := aa.Action.(type) { + case *microflows.CreateListAction: + got = a.ErrorHandlingType + case *microflows.ChangeListAction: + got = a.ErrorHandlingType + case *microflows.ListOperationAction: + got = a.ErrorHandlingType + case *microflows.AggregateListAction: + got = a.ErrorHandlingType + default: + continue + } + seen++ + if got != tc.want { + t.Errorf("nanoflow=%v: %T has error handling %q, want %q", tc.nanoflow, aa.Action, got, tc.want) + } + } + if seen != 5 { + t.Fatalf("nanoflow=%v: found %d list activities, want 5", tc.nanoflow, seen) + } + } +} + +// The cast activity is built by a different path from the list ones (an +// inheritance split's case body), so it is pinned on its own. +func TestCastAction_ErrorHandlingFollowsTheFlowFlavour(t *testing.T) { + for _, tc := range []struct { + nanoflow bool + want microflows.ErrorHandlingType + }{ + {true, microflows.ErrorHandlingTypeAbort}, + {false, microflows.ErrorHandlingTypeRollback}, + } { + fb := &flowBuilder{posX: 100, posY: 100, spacing: HorizontalSpacing, isNanoflow: tc.nanoflow} + oc := fb.buildFlowGraph([]ast.MicroflowStatement{&ast.CastObjectStmt{ObjectVariable: "Obj", OutputVariable: "Sub"}}, nil) + found := false + for _, obj := range oc.Objects { + if aa, ok := obj.(*microflows.ActionActivity); ok { + if c, ok := aa.Action.(*microflows.CastAction); ok { + found = true + if c.ErrorHandlingType != tc.want { + t.Errorf("nanoflow=%v: cast has error handling %q, want %q", tc.nanoflow, c.ErrorHandlingType, tc.want) + } + } + } + } + if !found { + t.Fatal("no cast action built") + } + } +} diff --git a/mdl/executor/nanoflow_validation.go b/mdl/executor/nanoflow_validation.go index bfee0f959..2863a306e 100644 --- a/mdl/executor/nanoflow_validation.go +++ b/mdl/executor/nanoflow_validation.go @@ -94,50 +94,12 @@ func checkDisallowedNanoflowAction(stmt ast.MicroflowStatement) string { return "" } -// getErrorHandling extracts the ErrorHandlingClause from statements that have one. -// -// Only statements reachable in nanoflows (i.e., NOT in the denylist) need coverage -// here. Disallowed actions are rejected by checkDisallowedNanoflowAction before -// this function is called. Statements like ListOperationStmt that have no -// ErrorHandling field are also omitted (they return nil implicitly via default). +// getErrorHandling extracts the ErrorHandlingClause from a statement that has +// one. It is stmtErrorHandling: a second hand-kept list of the statements that +// take a clause drifted from the first, which is how a handler body went +// unwalked here. func getErrorHandling(stmt ast.MicroflowStatement) *ast.ErrorHandlingClause { - switch s := stmt.(type) { - case *ast.CreateObjectStmt: - return s.ErrorHandling - case *ast.MfCommitStmt: - return s.ErrorHandling - case *ast.DeleteObjectStmt: - return s.ErrorHandling - case *ast.RetrieveStmt: - return s.ErrorHandling - case *ast.CallMicroflowStmt: - return s.ErrorHandling - case *ast.CallNanoflowStmt: - return s.ErrorHandling - case *ast.CallJavaScriptActionStmt: - return s.ErrorHandling - // The eight statements mendixlabs/mxcli#1078 gave an onErrorClause. None is - // on the denylist above, so all eight are reachable in a nanoflow — and - // without them here their handler BODIES are never walked, so a Java action - // or REST call nested inside `declare … on error begin … end error` would go unreported. - case *ast.DeclareStmt: - return s.ErrorHandling - case *ast.MfSetStmt: - return s.ErrorHandling - case *ast.ChangeObjectStmt: - return s.ErrorHandling - case *ast.LogStmt: - return s.ErrorHandling - case *ast.ShowPageStmt: - return s.ErrorHandling - case *ast.ClosePageStmt: - return s.ErrorHandling - case *ast.ShowMessageStmt: - return s.ErrorHandling - case *ast.ValidationFeedbackStmt: - return s.ErrorHandling - } - return nil + return stmtErrorHandling(stmt) } // validateNanoflowReturnType checks that the return type is allowed for nanoflows. @@ -241,11 +203,14 @@ func checkNanoflowErrorHandling(stmt ast.MicroflowStatement) string { keyword = "show message" case *ast.ValidationFeedbackStmt: keyword = "validation feedback" + case *ast.CreateObjectStmt, *ast.MfCommitStmt, *ast.CallNanoflowStmt, *ast.CallMicroflowStmt: + return checkNanoflowWithoutRollbackOnly(stmt) default: // declare and set are deliberately absent: both variable activities accept - // every form in a nanoflow. So do the statements that could already carry - // the clause (commit, create, retrieve, the calls) — unmeasured here, and - // refusing them would reject nanoflows that build today. + // every form in a nanoflow, and so do retrieve and delete (measured, + // mendixlabs/mxcli#591). The remaining statements that carry the clause + // are unmeasured here, and refusing them would reject nanoflows that + // build today. return "" } return "`" + keyword + " ... on error` is not supported in a nanoflow — Mendix rejects " + @@ -253,3 +218,58 @@ func checkNanoflowErrorHandling(stmt ast.MicroflowStatement) string { " there with CE6035 \"Error handling type is not supported\". Drop the clause " + "(a nanoflow activity aborts the flow on error by default)" } + +// nanoflowWithoutRollbackOnly names the nanoflow activities that accept exactly +// one error-handling clause: a custom handler WITHOUT rollback. +// +// MEASURED on Mendix 11.14.0 (an ako/TestApp copy, one nanoflow per cell), +// mendixlabs/mxcli#591: +// +// activity (in a NANOFLOW) continue rollback custom without rollback +// Create object CE6035 CE6035 CE6035 ok +// Commit CE6035 CE6035 CE6035 ok +// Call nanoflow CE6035 CE6035 CE6035 ok +// Call microflow CE6035 CE6035 CE6035 ok +// Retrieve ok ok ok ok +// Delete ok ok ok ok +// +// With no clause the activity stores the nanoflow default Abort, which builds; +// a nanoflow has no transaction for "rollback" to undo, and Studio Pro's own +// nanoflows in ako/TestApp store only Abort and CustomWithoutRollBack. +var nanoflowWithoutRollbackOnly = map[string]string{ + "create": "Create object activity", + "commit": "Commit object(s) activity", + "call nanoflow": "Nanoflow call action activity", + "call microflow": "Call microflow activity", +} + +func checkNanoflowWithoutRollbackOnly(stmt ast.MicroflowStatement) string { + eh := getErrorHandling(stmt) + if eh == nil || eh.Type == ast.ErrorHandlingCustomWithoutRollback { + return "" + } + var keyword string + switch stmt.(type) { + case *ast.CreateObjectStmt: + keyword = "create" + case *ast.MfCommitStmt: + keyword = "commit" + case *ast.CallNanoflowStmt: + keyword = "call nanoflow" + case *ast.CallMicroflowStmt: + keyword = "call microflow" + default: + return "" + } + form := "on error rollback" + switch eh.Type { + case ast.ErrorHandlingContinue: + form = "on error continue" + case ast.ErrorHandlingCustom: + form = "on error begin … end error" + } + return "`" + keyword + " ... " + form + "` is not supported in a nanoflow — Mendix rejects it on a " + + nanoflowWithoutRollbackOnly[keyword] + " there with CE6035 \"Error handling type is not supported\". " + + "A nanoflow has no transaction to roll back: drop the clause (the activity aborts the flow on error), " + + "or handle the error with `on error without rollback begin … end error`" +} diff --git a/mdl/executor/roundtrip_microflow_test.go b/mdl/executor/roundtrip_microflow_test.go index a6359ddaa..07ed281e8 100644 --- a/mdl/executor/roundtrip_microflow_test.go +++ b/mdl/executor/roundtrip_microflow_test.go @@ -681,12 +681,14 @@ func TestRoundtripMicroflow_ErrorHandlingContinue(t *testing.T) { createMDL := `create microflow ` + mfName + ` () returns Boolean begin $Obj = create RoundtripTest.MfCommitItem; - commit $Obj on error continue; + delete $Obj on error continue; return true; end;` + // delete, not commit: `commit … on error continue` is CE6035 (MDL076), + // which exec now refuses as check always did (mendixlabs/mxcli#175). assertMicroflowContains(t, env, mfName, createMDL, - []string{"commit", "on error continue", "return"}, + []string{"delete", "on error continue", "return"}, nil, ) } diff --git a/mdl/executor/roundtrip_nanoflow_test.go b/mdl/executor/roundtrip_nanoflow_test.go index 2f750d5c5..47d455596 100644 --- a/mdl/executor/roundtrip_nanoflow_test.go +++ b/mdl/executor/roundtrip_nanoflow_test.go @@ -239,12 +239,16 @@ end;` nfName := testModule + ".RT_NF_ErrorHandling" createMDL := `create nanoflow ` + nfName + ` () returns Boolean begin - $Result = call microflow ` + mfName + ` () on error continue; + $Result = call microflow ` + mfName + ` () on error without rollback begin + log error node 'RT' 'call failed'; + end error; return $Result; end;` + // A call in a nanoflow takes only a handler without rollback; `on error + // continue` there is CE6035 and refused (mendixlabs/mxcli#591). assertNanoflowContains(t, env, nfName, createMDL, - []string{"nanoflow", "call microflow", "on error continue", "return"}, + []string{"nanoflow", "call microflow", "on error without rollback", "return"}, nil, ) } diff --git a/mdl/executor/validate.go b/mdl/executor/validate.go index 1926355eb..d10dc828d 100644 --- a/mdl/executor/validate.go +++ b/mdl/executor/validate.go @@ -1577,9 +1577,20 @@ var execEnforcedMicroflowRules = map[string]bool{ // otherwise `check` catches the typo and the write that follows does not. "MDL059": true, "MDL060": true, + // MDL092: an @anchor parameter the visitor cannot use is dropped, the edge + // keeping its default sides — the same class as MDL060 (mendixlabs/mxcli#992). + "MDL092": true, + // MDL076: `on error continue` on an activity that rejects it is CE6035 at + // build time — every row of continueUnsupportedOn was measured on 11.14.0. + // check reported it; exec without the pre-check (-c, the REPL, --no-check) + // wrote it (mendixlabs/mxcli#175). + "MDL076": true, // MDL-WF16: a notify workflow with no target is CE0166 at build time, // measured on the 11.6, 11.10 and 11.13 mxbuilds. "MDL-WF16": true, + // MDL-WF17: `lock workflow all` / `unlock workflow all` is CE1825, measured + // on 11.13.0 and 11.14.0 (mendixlabs/mxcli#870). + "MDL-WF17": true, } // validateMicroflowRules runs the MDL0xx microflow rule set (ValidateMicroflow) diff --git a/mdl/executor/validate_microflow.go b/mdl/executor/validate_microflow.go index 5902bbfec..e9e8723b8 100644 --- a/mdl/executor/validate_microflow.go +++ b/mdl/executor/validate_microflow.go @@ -215,7 +215,21 @@ func (v *microflowValidator) walkBody(body []ast.MicroflowStatement) { v.checkErrorHandlingContinueSupported(s) v.checkErrorHandlingSupported(s) v.checkStmtExprFunctions(s) + // An annotation inside `on error begin … end error` is honoured like one + // outside it, so a malformed one there must be refused too — this walk + // never entered a handler body (mendixlabs/mxcli#991). + if eh := stmtErrorHandling(s); eh != nil { + v.walkAnnotations(eh.Body) + } switch stmt := s.(type) { + case *ast.LockWorkflowStmt: + if stmt.WorkflowVariable == "" && stmt.Workflow == "" { + v.refuseWorkflowAll("lock", "pause", "a Lock") + } + case *ast.UnlockWorkflowStmt: + if stmt.WorkflowVariable == "" && stmt.Workflow == "" { + v.refuseWorkflowAll("unlock", "unpause", "an Unlock") + } case *ast.NotifyWorkflowStmt: // MDL-WF16. A notify reaches one named element of the workflow, and the // build refuses one that names none: CE0166 "The 'Target' property is @@ -1486,6 +1500,9 @@ var knownActivityAnnotations = map[string]bool{ "start": true, } +// invalidAnchorRule refuses an @anchor parameter the visitor could not use. +const invalidAnchorRule = "MDL092" + // checkUnknownAnnotations rejects an @annotation name the visitor does not // implement. // @@ -1509,6 +1526,15 @@ func (v *microflowValidator) checkUnknownAnnotations(s ast.MicroflowStatement) { "A sequence flow's shape is two bezier control vectors, each a pixel offset from its end "+ "of the line: `@curve(from: (40, -90), to: (-40, 90))`. Only `from:` and `to:` are accepted.") } + for _, bad := range ann.InvalidAnchors { + v.addViolation(invalidAnchorRule, linter.SeverityError, + fmt.Sprintf("`@anchor` parameter `%s` is not one mxcli understands, so the edge it "+ + "names keeps its default sides", bad), + "A side is top, right, bottom or left. The flow leaving a statement is "+ + "`@anchor(from: right, to: left)`; an IF's branches are `@anchor(true: (from: …, to: …), "+ + "false: (…))`, a loop's `@anchor(iterator: (…), tail: (…))` — either side of a pair may be "+ + "left out (mendixlabs/mxcli#992).") + } for _, name := range ann.UnknownNames { v.addViolation("MDL059", linter.SeverityError, fmt.Sprintf("unknown annotation `@%s` — it parses but does nothing, so whatever it was "+ @@ -1583,3 +1609,21 @@ func (v *microflowValidator) checkCaptionOnLoop(ann *ast.ActivityAnnotations, wh "Use @annotation to attach a note to the loop instead.", "Replace @caption with @annotation to label the loop") } + +// lockWorkflowAllRule refuses `lock workflow all` / `unlock workflow all`. +const lockWorkflowAllRule = "MDL-WF17" + +// refuseWorkflowAll reports a lock or unlock that names no workflow. A Lock +// workflow activity always targets one definition; PauseAllWorkflows is the +// "Pause instances" option ON that definition, not "every workflow", and the +// metamodel has no all-definitions selection. The bare form built as CE1825 +// "The 'Workflow' property is required" (mendixlabs/mxcli#870). +func (v *microflowValidator) refuseWorkflowAll(verb, flag, activity string) { + label := strings.ToUpper(flag[:1]) + flag[1:] + v.addViolation(lockWorkflowAllRule, linter.SeverityError, + fmt.Sprintf("`%s workflow all` names no workflow — %s workflow activity always targets one "+ + "workflow definition, and the build fails CE1825 \"The 'Workflow' property is required\"", verb, activity), + fmt.Sprintf("Name the workflow: `%s workflow $WorkflowDefinition;` or `%s workflow Module.Workflow;`. "+ + "To %s the running instances of that workflow as well (Studio Pro's \"%s instances\"), add `%s all`: "+ + "`%s workflow $WorkflowDefinition %s all;`.", verb, verb, flag, label, flag, verb, flag)) +} diff --git a/mdl/executor/validate_microflow_error_handling.go b/mdl/executor/validate_microflow_error_handling.go index f63c0b10c..4645aeac8 100644 --- a/mdl/executor/validate_microflow_error_handling.go +++ b/mdl/executor/validate_microflow_error_handling.go @@ -58,6 +58,10 @@ var continueUnsupportedOn = map[string]string{ "close page": "Close page activity", "show message": "Show message activity", "validation feedback": "Validation feedback activity", + // mendixlabs/mxcli#175, measured on 11.14.0 in ako/TestApp: continue is + // CE6035 on a Call workflow activity; no clause, rollback and both custom + // handlers build. + "call workflow": "Call workflow activity", } // checkErrorHandlingContinueSupported reports `ON ERROR CONTINUE` on a statement @@ -114,6 +118,8 @@ func continueUnsupportedStatement(stmt ast.MicroflowStatement) (keyword, activit keyword = "show message" case *ast.ValidationFeedbackStmt: keyword = "validation feedback" + case *ast.CallWorkflowStmt: + keyword = "call workflow" default: // DeclareStmt and MfSetStmt are deliberately absent: create-variable and // change-variable accept Continue on 11.14.0. diff --git a/mdl/executor/validate_nanoflow.go b/mdl/executor/validate_nanoflow.go index f2d65739f..a3599b31d 100644 --- a/mdl/executor/validate_nanoflow.go +++ b/mdl/executor/validate_nanoflow.go @@ -28,9 +28,60 @@ func ValidateNanoflow(stmt *ast.CreateNanoflowStmt) []linter.Violation { varKinds: map[string]exprcheck.TypeKind{}, } v.walkExprFunctions(stmt.Body) + v.walkAnnotations(stmt.Body) + // The nanoflow restrictions exec's build refuses (validateNanoflow): an + // action a nanoflow cannot hold, an error-handling clause its activity + // rejects (CE6035), a Binary return. They ran only inside exec, so `check` + // passed what exec then refused (mendixlabs/mxcli#591). + for _, msg := range validateNanoflowBody(stmt.Body) { + v.addViolation(nanoflowActivityRule, linter.SeverityError, msg, "") + } + if msg := validateNanoflowReturnType(stmt.ReturnType); msg != "" { + v.addViolation(nanoflowActivityRule, linter.SeverityError, msg, "") + } return v.violations } +// nanoflowActivityRule reports what a nanoflow cannot hold — the messages of +// validateNanoflow, which exec refuses in its build. A check-side ID for them. +const nanoflowActivityRule = "MDL091" + +// walkAnnotations applies checkUnknownAnnotations (MDL059, MDL060, +// MDL079) to every statement of a body, nested ones and error-handler bodies +// included. A nanoflow runs it over its whole body; a microflow's walkBody +// covers its own statements and hands it the handler bodies, which walkBody +// does not enter. A nanoflow is where layout annotations matter most — Studio Pro's +// PED API does not write nanoflows, so MDL is their only scripted writer — and +// an annotation that parsed but was dropped there passed check AND exec +// (mendixlabs/mxcli#992). +func (v *microflowValidator) walkAnnotations(body []ast.MicroflowStatement) { + for _, s := range body { + v.checkUnknownAnnotations(s) + switch stmt := s.(type) { + case *ast.IfStmt: + v.walkAnnotations(stmt.ThenBody) + v.walkAnnotations(stmt.ElseBody) + case *ast.EnumSplitStmt: + for _, c := range stmt.Cases { + v.walkAnnotations(c.Body) + } + v.walkAnnotations(stmt.ElseBody) + case *ast.InheritanceSplitStmt: + for _, c := range stmt.Cases { + v.walkAnnotations(c.Body) + } + v.walkAnnotations(stmt.ElseBody) + case *ast.LoopStmt: + v.walkAnnotations(stmt.Body) + case *ast.WhileStmt: + v.walkAnnotations(stmt.Body) + } + if eh := stmtErrorHandling(s); eh != nil { + v.walkAnnotations(eh.Body) + } + } +} + // walkExprFunctions applies checkStmtExprFunctions to every statement in the // body, including branches, loops and error-handler bodies. func (v *microflowValidator) walkExprFunctions(body []ast.MicroflowStatement) { diff --git a/mdl/executor/validate_nanoflow_check_test.go b/mdl/executor/validate_nanoflow_check_test.go new file mode 100644 index 000000000..79fbf77ca --- /dev/null +++ b/mdl/executor/validate_nanoflow_check_test.go @@ -0,0 +1,108 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/linter" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +func nanoflowViolations(t *testing.T, body string) []linter.Violation { + t.Helper() + prog, errs := visitor.Build("mdl 1;\ncreate nanoflow M.NF_T()\nbegin\n" + body + "\nend;") + if len(errs) > 0 { + t.Fatalf("parsing:\n%s\nerrors: %v", body, errs) + } + return ValidateNanoflow(prog.Statements[len(prog.Statements)-1].(*ast.CreateNanoflowStmt)) +} + +func hasErrorRule(vs []linter.Violation, rule string) bool { + for _, v := range vs { + if v.RuleID == rule && v.Severity == linter.SeverityError { + return true + } + } + return false +} + +// `mxcli check` reported nothing a nanoflow could not hold: the nanoflow rules +// ran only inside exec's build (validateNanoflow), and the annotation rules +// (MDL059/MDL060) ran only for CREATE MICROFLOW. So `call nanoflow … on error +// continue` (mendixlabs/mxcli#591), `show home page` in a nanoflow, and the +// per-case `@curve(true: …)` mxcli does not implement (mendixlabs/mxcli#992) +// all passed check — the curve one passed exec too, and was dropped. +func TestValidateNanoflow_ReportsWhatExecWouldRefuse(t *testing.T) { + for _, tc := range []struct{ name, body, rule string }{ + {"error handling", "call nanoflow M.NF_Sub() on error continue;", nanoflowActivityRule}, + {"disallowed action", "show home page;", nanoflowActivityRule}, + {"per-case curve", "@curve(true: (from: (15, 0), to: (0, -30)))\nif true then\n log info node 'a' 'b';\nend if;", "MDL060"}, + {"unknown annotation", "@postion(1, 2)\nlog info node 'a' 'b';", "MDL059"}, + {"inside a handler", "call microflow M.MF() on error without rollback begin\n @curve(true: (from: (1, 0)))\n log info node 'a' 'b';\nend error;", "MDL060"}, + } { + t.Run(tc.name, func(t *testing.T) { + if vs := nanoflowViolations(t, tc.body); !hasErrorRule(vs, tc.rule) { + t.Errorf("check passed a nanoflow exec refuses or drops; want %s, got %+v", tc.rule, vs) + } + }) + } + // CONTROL: a nanoflow with none of these is clean. + if vs := nanoflowViolations(t, "@position(100, 100)\n@curve(from: (0, 30), to: (0, -30))\nlog info node 'a' 'b';"); hasErrorRule(vs, nanoflowActivityRule) || hasErrorRule(vs, "MDL060") || hasErrorRule(vs, "MDL059") { + t.Errorf("a valid nanoflow was reported: %+v", vs) + } +} + +// exec refuses MDL060 and MDL059 on a nanoflow as it does on a microflow. +func TestValidateNanoflowRules_RefusesAnIgnoredCurve(t *testing.T) { + prog, errs := visitor.Build("mdl 1;\ncreate nanoflow M.NF_T()\nbegin\n@curve(true: (from: (15, 0)))\nif true then\n log info node 'a' 'b';\nend if;\nend;") + if len(errs) > 0 { + t.Fatal(errs) + } + if err := validateNanoflowRules(prog.Statements[len(prog.Statements)-1].(*ast.CreateNanoflowStmt)); err == nil { + t.Fatal("exec accepted a @curve it would drop") + } +} + +// A microflow's annotation rules did not enter an error-handler body either. +func TestValidateMicroflow_ChecksAnnotationsInsideAHandler(t *testing.T) { + prog, errs := visitor.Build("mdl 1;\ncreate microflow M.MF_T()\nbegin\ncall microflow M.MF() on error without rollback begin\n @postion(1, 2)\n log info node 'a' 'b';\nend error;\nend;") + if len(errs) > 0 { + t.Fatal(errs) + } + if vs := ValidateMicroflow(prog.Statements[len(prog.Statements)-1].(*ast.CreateMicroflowStmt)); !hasErrorRule(vs, "MDL059") { + t.Fatalf("an unknown annotation inside a handler passed: %+v", vs) + } +} + +// mendixlabs/mxcli#992: an @anchor parameter the visitor cannot use was +// skipped, and the edge kept the builder's default sides. Refused at check and +// exec, on a microflow and a nanoflow alike. +func TestAnchorParameterItCannotUseIsRefused(t *testing.T) { + const body = "@anchor(true: (to: middle))\nif true then\n log info node 'a' 'b';\nend if;" + prog, errs := visitor.Build("mdl 1;\ncreate microflow M.MF_T()\nbegin\n" + body + "\nend;\ncreate nanoflow M.NF_T()\nbegin\n" + body + "\nend;") + if len(errs) > 0 { + t.Fatal(errs) + } + n := len(prog.Statements) + mf := prog.Statements[n-2].(*ast.CreateMicroflowStmt) + nf := prog.Statements[n-1].(*ast.CreateNanoflowStmt) + if vs := ValidateMicroflow(mf); !hasErrorRule(vs, invalidAnchorRule) { + t.Errorf("microflow: check passed an @anchor it drops: %+v", vs) + } + if vs := ValidateNanoflow(nf); !hasErrorRule(vs, invalidAnchorRule) { + t.Errorf("nanoflow: check passed an @anchor it drops: %+v", vs) + } + if err := validateMicroflowRules(mf); err == nil { + t.Error("microflow: exec accepted an @anchor it drops") + } + if err := validateNanoflowRules(nf); err == nil { + t.Error("nanoflow: exec accepted an @anchor it drops") + } + // CONTROL: the one-sided per-case form is valid. + prog, _ = visitor.Build("mdl 1;\ncreate nanoflow M.NF_T()\nbegin\n@anchor(true: (to: top))\nif true then\n log info node 'a' 'b';\nend if;\nend;") + if vs := ValidateNanoflow(prog.Statements[len(prog.Statements)-1].(*ast.CreateNanoflowStmt)); hasErrorRule(vs, invalidAnchorRule) { + t.Errorf("a valid per-case @anchor was refused: %+v", vs) + } +} diff --git a/mdl/exprcheck/adapters/adapter_scope.go b/mdl/exprcheck/adapters/adapter_scope.go index 87e32511d..facdc59ba 100644 --- a/mdl/exprcheck/adapters/adapter_scope.go +++ b/mdl/exprcheck/adapters/adapter_scope.go @@ -3,6 +3,7 @@ package adapters import ( + "reflect" "strings" "github.com/mendixlabs/mxcli/mdl/ast" @@ -239,7 +240,27 @@ func StatementErrorHandling(stmt ast.MicroflowStatement) *ast.ErrorHandlingClaus case *ast.AggregateListStmt: return s.ErrorHandling } - return nil + return reflectedErrorHandling(stmt) +} + +var errorHandlingClauseType = reflect.TypeOf(&ast.ErrorHandlingClause{}) + +// reflectedErrorHandling reads the clause off any other statement that carries +// one in an `ErrorHandling` field. The switch above is a list, and a list +// drifts: `call workflow`, the REST statements and the other workflow +// statements all take an onErrorClause and were missing from it, so the rules +// that ask for the clause — MDL076 for one (mendixlabs/mxcli#175) — could not +// see it, and their handler bodies were never walked. +func reflectedErrorHandling(stmt ast.MicroflowStatement) *ast.ErrorHandlingClause { + v := reflect.ValueOf(stmt) + if v.Kind() != reflect.Ptr || v.IsNil() || v.Elem().Kind() != reflect.Struct { + return nil + } + f := v.Elem().FieldByName("ErrorHandling") + if !f.IsValid() || f.Type() != errorHandlingClauseType || f.IsNil() { + return nil + } + return f.Interface().(*ast.ErrorHandlingClause) } // errorHandlerBody returns a statement's custom ON ERROR body, or nil when it diff --git a/mdl/grammar/domains/MDLMicroflow.g4 b/mdl/grammar/domains/MDLMicroflow.g4 index a754ab625..c43ed60cf 100644 --- a/mdl/grammar/domains/MDLMicroflow.g4 +++ b/mdl/grammar/domains/MDLMicroflow.g4 @@ -684,14 +684,19 @@ openWorkflowStatement : OPEN WORKFLOW VARIABLE onErrorClause? ; -// LOCK WORKFLOW $Wf; or LOCK WORKFLOW ALL; +// LOCK WORKFLOW $WfDef [PAUSE ALL]; or LOCK WORKFLOW Module.Workflow [PAUSE ALL]; +// PAUSE ALL is Studio Pro's "Pause instances" (PauseAllWorkflows). A lock always +// names its workflow: the bare `LOCK WORKFLOW ALL` still parses, and check and +// exec refuse it — there is no model for it, it built as CE1825 +// (mendixlabs/mxcli#870). ALL is before qualifiedName so `all` stays that form. lockWorkflowStatement - : LOCK WORKFLOW (VARIABLE | ALL) onErrorClause? + : LOCK WORKFLOW (VARIABLE | ALL | qualifiedName) (PAUSE ALL)? onErrorClause? ; -// UNLOCK WORKFLOW $Wf; or UNLOCK WORKFLOW ALL; +// UNLOCK WORKFLOW $WfDef [UNPAUSE ALL]; — UNPAUSE ALL is "Unpause instances" +// (ResumeAllPausedWorkflows). Same rule for the bare ALL. unlockWorkflowStatement - : UNLOCK WORKFLOW (VARIABLE | ALL) onErrorClause? + : UNLOCK WORKFLOW (VARIABLE | ALL | qualifiedName) (UNPAUSE ALL)? onErrorClause? ; callArgumentList diff --git a/mdl/grammar/domains/MDLSettings.g4 b/mdl/grammar/domains/MDLSettings.g4 index b3e5ff3dc..57bbd77f0 100644 --- a/mdl/grammar/domains/MDLSettings.g4 +++ b/mdl/grammar/domains/MDLSettings.g4 @@ -569,7 +569,11 @@ annotationParams ; annotationParam - : annotationParamName COLON (annotationValue | annotationParenValue) // Named parameter + // annotationParenValue FIRST: `(to: top)` is also an expression — `:` is + // Mendix's division operator — and ANTLR takes the first alternative that + // matches, so with annotationValue first `@anchor(true: (to: top))` parsed + // as `true: to ÷ top` and set no anchor (mendixlabs/mxcli#992). + : annotationParamName COLON (annotationParenValue | annotationValue) // Named parameter | annotationValue // Positional parameter ; diff --git a/mdl/roundtrip/flow_modify_notes_test.go b/mdl/roundtrip/flow_modify_notes_test.go index 9aa090b60..bd44fdd16 100644 --- a/mdl/roundtrip/flow_modify_notes_test.go +++ b/mdl/roundtrip/flow_modify_notes_test.go @@ -185,7 +185,7 @@ end; create or modify nanoflow MyFirstModule.Rerun_RollbackNf ($E: MyFirstModule.RerunRbThing) begin change $E (Name = 'one'); - commit $E on error rollback; + delete $E on error rollback; end; ` if err := h.exec(flows); err != nil { diff --git a/mdl/visitor/visitor_anchor_test.go b/mdl/visitor/visitor_anchor_test.go index d3d578b91..fc5534b2a 100644 --- a/mdl/visitor/visitor_anchor_test.go +++ b/mdl/visitor/visitor_anchor_test.go @@ -240,3 +240,45 @@ end loop;` loop.Annotations.BodyTailAnchor) } } + +// mendixlabs/mxcli#992. `@anchor(true: (to: top))` — ONE side in the nested +// pair, the form describe emits — parsed without error and set nothing: `(to: +// top)` matched annotationValue's expression alternative first, as `to : top` +// (`:` is Mendix's division operator), so the nested-anchor reader never saw +// it and the true edge kept the builder's default sides. +func TestAnchorAnnotation_SplitBranchWithOneSide(t *testing.T) { + for _, tc := range []struct { + src string + from, to ast.AnchorSide + }{ + {"@anchor(true: (to: top))", ast.AnchorSideUnset, ast.AnchorSideTop}, + {"@anchor(true: (from: bottom))", ast.AnchorSideBottom, ast.AnchorSideUnset}, + } { + stmt := firstStatement(t, tc.src+"\nif true then\n log info node 'App' 'yes';\nend if;") + ifStmt := stmt.(*ast.IfStmt) + a := ifStmt.Annotations.TrueBranchAnchor + if a == nil { + t.Errorf("%s: TrueBranchAnchor not set", tc.src) + continue + } + if a.From != tc.from || a.To != tc.to { + t.Errorf("%s: got from=%v to=%v, want from=%v to=%v", tc.src, a.From, a.To, tc.from, tc.to) + } + } +} + +// An @anchor parameter the reader cannot use is recorded, so validation can +// refuse it rather than leave the edge on its default sides in silence. +func TestAnchorAnnotation_RecordsWhatItCannotUse(t *testing.T) { + for _, src := range []string{ + "@anchor(true: (to: middle))", + "@anchor(sideways: (to: top))", + "@anchor(from: up)", + } { + stmt := firstStatement(t, src+"\nif true then\n log info node 'App' 'yes';\nend if;") + ann := stmt.(*ast.IfStmt).Annotations + if ann == nil || len(ann.InvalidAnchors) == 0 { + t.Errorf("%s: nothing recorded", src) + } + } +} diff --git a/mdl/visitor/visitor_microflow_statements.go b/mdl/visitor/visitor_microflow_statements.go index 63d9f1df3..eff42316f 100644 --- a/mdl/visitor/visitor_microflow_statements.go +++ b/mdl/visitor/visitor_microflow_statements.go @@ -472,45 +472,61 @@ func hasLaterActivityAnnotation(annotations []parser.IAnnotationContext, start i // parseAnchorAnnotation populates Anchor / TrueBranchAnchor / FalseBranchAnchor / // IteratorAnchor / BodyTailAnchor fields on result from the @anchor(...) params. +// A parameter it cannot use is recorded in InvalidAnchors, never skipped: a +// skipped one left the edge on its default sides with nothing said +// (mendixlabs/mxcli#992). func parseAnchorAnnotation(params *parser.AnnotationParamsContext, result *ast.ActivityAnnotations) { flat := &ast.FlowAnchors{From: ast.AnchorSideUnset, To: ast.AnchorSideUnset} flatSet := false + invalid := func(pCtx *parser.AnnotationParamContext) { + result.InvalidAnchors = append(result.InvalidAnchors, strings.TrimSpace(pCtx.GetText())) + } for _, p := range params.AllAnnotationParam() { pCtx := p.(*parser.AnnotationParamContext) nameCtx := pCtx.AnnotationParamName() if nameCtx == nil { - continue // positional form not supported for @anchor + invalid(pCtx) // positional form not supported for @anchor + continue } key := strings.ToLower(nameCtx.GetText()) switch key { - case "from": - if side, ok := parseAnchorSideFromValue(pCtx.AnnotationValue()); ok { - flat.From = side - flatSet = true + case "from", "to": + side, ok := parseAnchorSideFromValue(pCtx.AnnotationValue()) + if !ok { + invalid(pCtx) + continue } - case "to": - if side, ok := parseAnchorSideFromValue(pCtx.AnnotationValue()); ok { + if key == "from" { + flat.From = side + } else { flat.To = side - flatSet = true - } - case "true": - if nested := pCtx.AnnotationParenValue(); nested != nil { - result.TrueBranchAnchor = parseNestedFlowAnchors(nested.(*parser.AnnotationParenValueContext)) } - case "false": - if nested := pCtx.AnnotationParenValue(); nested != nil { - result.FalseBranchAnchor = parseNestedFlowAnchors(nested.(*parser.AnnotationParenValueContext)) + flatSet = true + case "true", "false", "iterator", "tail": + nested := pCtx.AnnotationParenValue() + if nested == nil { + invalid(pCtx) + continue } - case "iterator": - if nested := pCtx.AnnotationParenValue(); nested != nil { - result.IteratorAnchor = parseNestedFlowAnchors(nested.(*parser.AnnotationParenValueContext)) + fa, ok := parseNestedFlowAnchors(nested.(*parser.AnnotationParenValueContext)) + if !ok { + invalid(pCtx) + continue } - case "tail": - if nested := pCtx.AnnotationParenValue(); nested != nil { - result.BodyTailAnchor = parseNestedFlowAnchors(nested.(*parser.AnnotationParenValueContext)) + switch key { + case "true": + result.TrueBranchAnchor = fa + case "false": + result.FalseBranchAnchor = fa + case "iterator": + result.IteratorAnchor = fa + case "tail": + result.BodyTailAnchor = fa } + default: + invalid(pCtx) } } @@ -519,11 +535,13 @@ func parseAnchorAnnotation(params *parser.AnnotationParamsContext, result *ast.A } } -// parseNestedFlowAnchors parses a `(from: X, to: Y)` sub-expression into FlowAnchors. -func parseNestedFlowAnchors(p *parser.AnnotationParenValueContext) *ast.FlowAnchors { +// parseNestedFlowAnchors parses a `(from: X, to: Y)` sub-expression into +// FlowAnchors. Either side may be omitted; ok is false when the pair holds +// anything else, or nothing. +func parseNestedFlowAnchors(p *parser.AnnotationParenValueContext) (*ast.FlowAnchors, bool) { inner := p.AnnotationParams() if inner == nil { - return nil + return nil, false } fa := &ast.FlowAnchors{From: ast.AnchorSideUnset, To: ast.AnchorSideUnset} set := false @@ -531,26 +549,23 @@ func parseNestedFlowAnchors(p *parser.AnnotationParenValueContext) *ast.FlowAnch ppCtx := pp.(*parser.AnnotationParamContext) nameCtx := ppCtx.AnnotationParamName() if nameCtx == nil { - continue + return nil, false } - key := strings.ToLower(nameCtx.GetText()) side, ok := parseAnchorSideFromValue(ppCtx.AnnotationValue()) if !ok { - continue + return nil, false } - switch key { + switch strings.ToLower(nameCtx.GetText()) { case "from": fa.From = side - set = true case "to": fa.To = side - set = true + default: + return nil, false } + set = true } - if !set { - return nil - } - return fa + return fa, set } // parseAnchorSideFromValue extracts a side keyword from an annotationValue. diff --git a/mdl/visitor/visitor_microflow_workflow.go b/mdl/visitor/visitor_microflow_workflow.go index 49d561f8a..5bb2b6df1 100644 --- a/mdl/visitor/visitor_microflow_workflow.go +++ b/mdl/visitor/visitor_microflow_workflow.go @@ -229,10 +229,14 @@ func buildLockWorkflowStatement(ctx parser.ILockWorkflowStatementContext) *ast.L c := ctx.(*parser.LockWorkflowStatementContext) stmt := &ast.LockWorkflowStmt{} - if c.ALL() != nil { - stmt.PauseAllWorkflows = true - } else if v := c.VARIABLE(); v != nil { + if v := c.VARIABLE(); v != nil { stmt.WorkflowVariable = strings.TrimPrefix(v.GetText(), "$") + } else if qn := c.QualifiedName(); qn != nil { + stmt.Workflow = getQualifiedNameText(qn) + } + // `pause all` after a workflow, or the bare `lock workflow all`. + if c.PAUSE() != nil || len(c.AllALL()) > 0 { + stmt.PauseAllWorkflows = true } if errClause := c.OnErrorClause(); errClause != nil { stmt.ErrorHandling = buildOnErrorClause(errClause) @@ -247,10 +251,13 @@ func buildUnlockWorkflowStatement(ctx parser.IUnlockWorkflowStatementContext) *a c := ctx.(*parser.UnlockWorkflowStatementContext) stmt := &ast.UnlockWorkflowStmt{} - if c.ALL() != nil { - stmt.ResumeAllPausedWorkflows = true - } else if v := c.VARIABLE(); v != nil { + if v := c.VARIABLE(); v != nil { stmt.WorkflowVariable = strings.TrimPrefix(v.GetText(), "$") + } else if qn := c.QualifiedName(); qn != nil { + stmt.Workflow = getQualifiedNameText(qn) + } + if c.UNPAUSE() != nil || len(c.AllALL()) > 0 { + stmt.ResumeAllPausedWorkflows = true } if errClause := c.OnErrorClause(); errClause != nil { stmt.ErrorHandling = buildOnErrorClause(errClause) diff --git a/sdk/microflows/microflows_actions.go b/sdk/microflows/microflows_actions.go index c338a2dcb..9282648c3 100644 --- a/sdk/microflows/microflows_actions.go +++ b/sdk/microflows/microflows_actions.go @@ -206,6 +206,11 @@ type AggregateListAction struct { AttributeQualifiedName string `json:"attributeQualifiedName,omitempty"` // BY_NAME_REFERENCE: Module.Entity.Attribute UseExpression bool `json:"useExpression,omitempty"` // true when Expression is used instead of Attribute Expression string `json:"expression,omitempty"` // Mendix expression string (when UseExpression=true) + // ErrorHandlingType is the flow flavour's default: Rollback in a microflow, + // Abort in a nanoflow. MDL has no clause for it, and "Rollback" in a + // nanoflow is CE6035 (mendixlabs/mxcli#591). Empty means the writer's + // historical "Rollback". + ErrorHandlingType ErrorHandlingType `json:"errorHandlingType,omitempty"` // ReduceInitialValue and ReduceReturnType are what REDUCE folds from, and // the type it folds to. Studio Pro writes both on *every* AggregateAction it @@ -272,6 +277,11 @@ type ListOperationAction struct { model.BaseElement Operation ListOperation `json:"operation,omitempty"` OutputVariable string `json:"outputVariable,omitempty"` + // ErrorHandlingType is the flow flavour's default: Rollback in a microflow, + // Abort in a nanoflow. MDL has no clause for it, and "Rollback" in a + // nanoflow is CE6035 (mendixlabs/mxcli#591). Empty means the writer's + // historical "Rollback". + ErrorHandlingType ErrorHandlingType `json:"errorHandlingType,omitempty"` } func (ListOperationAction) isMicroflowAction() {} @@ -409,6 +419,11 @@ type CreateListAction struct { EntityID model.ID `json:"entityId,omitempty"` EntityQualifiedName string `json:"entityQualifiedName,omitempty"` OutputVariable string `json:"outputVariable"` + // ErrorHandlingType is the flow flavour's default: Rollback in a microflow, + // Abort in a nanoflow. MDL has no clause for it, and "Rollback" in a + // nanoflow is CE6035 (mendixlabs/mxcli#591). Empty means the writer's + // historical "Rollback". + ErrorHandlingType ErrorHandlingType `json:"errorHandlingType,omitempty"` } func (CreateListAction) isMicroflowAction() {} @@ -419,6 +434,11 @@ type ChangeListAction struct { ChangeVariable string `json:"changeVariable"` Type ChangeListType `json:"type"` Value string `json:"value,omitempty"` + // ErrorHandlingType is the flow flavour's default: Rollback in a microflow, + // Abort in a nanoflow. MDL has no clause for it, and "Rollback" in a + // nanoflow is CE6035 (mendixlabs/mxcli#591). Empty means the writer's + // historical "Rollback". + ErrorHandlingType ErrorHandlingType `json:"errorHandlingType,omitempty"` } func (ChangeListAction) isMicroflowAction() {} @@ -461,6 +481,11 @@ type CastAction struct { model.BaseElement ObjectVariable string `json:"objectVariable"` OutputVariable string `json:"outputVariable"` + // ErrorHandlingType is the flow flavour's default: Rollback in a microflow, + // Abort in a nanoflow. MDL has no clause for it, and "Rollback" in a + // nanoflow is CE6035 (mendixlabs/mxcli#591). Empty means the writer's + // historical "Rollback". + ErrorHandlingType ErrorHandlingType `json:"errorHandlingType,omitempty"` } func (CastAction) isMicroflowAction() {}