From c7f89f5e0608e7e54c5b4b867faf205e64a607f7 Mon Sep 17 00:00:00 2001 From: Ako Date: Thu, 1 Oct 2026 19:27:24 +0000 Subject: [PATCH 1/3] 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 b42032884a..bb7f53251c 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 ccd3abf86d..bcc65de6e2 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 b07c04bb98..ca0d91a38c 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 0000000000..f73ef0c0b8 --- /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/3] 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 b42032884a..62dc4c89c1 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 898dec6bbd..fd072be987 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 0000000000..c25d1eacb9 --- /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 0000000000..eec264a5cb --- /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 4f1e883845..2e84dbd91d 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 0000000000..180a4dc9ad --- /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 79fac38e33..7778fbdeb0 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 e02fd24d5d..ee8292e9a1 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/3] 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 e55dc06ee8..3cdc21513f 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 235a9f6acd..2a45fb7e01 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 d50f5a2d85..2300d1ce8b 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 063d7038fa..790bf5f53e 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 0e841f9ca8..5248711305 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 0000000000..7e4f12ebc2 --- /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 0000000000..cd06565618 --- /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 983cb2a307..3c06e84cf9 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 {