diff --git a/mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl b/mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl new file mode 100644 index 000000000..15dbe1e32 --- /dev/null +++ b/mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl @@ -0,0 +1,61 @@ +-- Bug test: `create or modify external entity` must preserve fields the +-- statement omits rather than overwriting them with zero values. +-- +-- Issue #594: the executor (mdl/executor/cmd_odata.go) unconditionally wrote +-- `existingEntity.X = s.X` for every property. When the MDL statement omitted +-- `RemoteName`, `s.RemoteName` was the empty string and the existing value +-- was wiped. The resulting `Rest$ODataRemoteEntitySource` BSON had +-- `RemoteName: ""`, which Studio Pro's Integration Pane visualiser then +-- dereferences in `ODataRemoteEntitySource.get_RemoteId()` and NREs with +-- ArgumentNullException("EntityTypeName") — Studio Pro aliases the BSON +-- `RemoteName` field as the C# property `EntityTypeName`. +-- +-- The Go unit test in mdl/executor/cmd_odata_mock_test.go +-- (TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594) covers +-- the executor logic with a mock backend. This MDL script is the end-to-end +-- regression test: applying it to a real project must leave a +-- bug594.RemoteAccount with RemoteName = "Account" after the modify, and +-- the project must open in Studio Pro without the Integration Pane NRE. +-- +-- Run with: +-- mxcli exec mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl -p .mpr +-- mxcli describe entity bug594.RemoteAccount -p .mpr # RemoteName must still be 'Account' +-- then open .mpr in Studio Pro and confirm the Integration Pane renders. +create module bug594; + +create odata client bug594.SalesforceAPI ( + ODataVersion: OData4, + MetadataUrl: 'https://api.salesforce.com/odata/v4/$metadata', + timeout: 300 +); + +create external entity bug594.RemoteAccount +from odata client bug594.SalesforceAPI +( + EntitySet: 'Accounts', + RemoteName: 'Account', + Countable: Yes, + Creatable: Yes, + Deletable: No, + Updatable: No +) +( + AccountId: string(200), + AccountName: string(255) +); + +-- Modify without restating RemoteName / Countable / Creatable. Before the +-- fix, every omitted property was overwritten with its zero value and the +-- entity ended up unusable. After the fix, all of them are preserved. +create or modify external entity bug594.RemoteAccount +from odata client bug594.SalesforceAPI +( + EntitySet: 'Accounts', + AllowCreateChangeLocally: Yes +) +( + AccountId: string(200), + AccountName: string(255) +); + +describe entity bug594.RemoteAccount; diff --git a/mdl/ast/ast_odata.go b/mdl/ast/ast_odata.go index 83d5a0516..8ed8a2c24 100644 --- a/mdl/ast/ast_odata.go +++ b/mdl/ast/ast_odata.go @@ -121,16 +121,21 @@ type DropODataServiceStmt struct { func (s *DropODataServiceStmt) isStatement() {} // CreateExternalEntityStmt represents: CREATE [OR MODIFY] EXTERNAL ENTITY Module.Name FROM ODATA CLIENT Module.Service (...) (attrs); +// +// Scalar property fields are pointers so the executor can distinguish +// "omitted by user" (nil → preserve existing value on CREATE OR MODIFY) +// from "explicitly set to zero" (e.g. Countable: false). Treating omitted +// fields as zero on modify silently corrupted entities — see issue #594. type CreateExternalEntityStmt struct { Name QualifiedName ServiceRef QualifiedName // FROM ODATA CLIENT ... - EntitySet string - RemoteName string - Countable bool - Creatable bool - Deletable bool - Updatable bool - AllowCreateChangeLocally bool // "Allow creating and changing locally" flag + EntitySet *string + RemoteName *string + Countable *bool + Creatable *bool + Deletable *bool + Updatable *bool + AllowCreateChangeLocally *bool // "Allow creating and changing locally" flag Attributes []Attribute // reuse from ast_entity.go Documentation string CreateOrModify bool diff --git a/mdl/executor/cmd_odata.go b/mdl/executor/cmd_odata.go index c2cf73f25..4ffb24e50 100644 --- a/mdl/executor/cmd_odata.go +++ b/mdl/executor/cmd_odata.go @@ -806,16 +806,34 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt) serviceRef := s.ServiceRef.String() if existingEntity != nil { - // Update existing entity + // Update existing entity. Issue #594: only assign fields that the + // MDL statement explicitly set. Omitted (nil) fields preserve the + // prior value — previously the executor unconditionally wrote zero + // values, wiping fields like RemoteName and triggering Studio Pro + // NREs (e.g. ODataRemoteEntitySource.RemoteId on empty RemoteName). existingEntity.Source = "Rest$ODataRemoteEntitySource" existingEntity.RemoteServiceName = serviceRef - existingEntity.RemoteEntitySet = s.EntitySet - existingEntity.RemoteEntityName = s.RemoteName - existingEntity.Countable = s.Countable - existingEntity.Creatable = s.Creatable - existingEntity.Deletable = s.Deletable - existingEntity.Updatable = s.Updatable - existingEntity.CreateChangeLocally = s.AllowCreateChangeLocally + if s.EntitySet != nil { + existingEntity.RemoteEntitySet = *s.EntitySet + } + if s.RemoteName != nil { + existingEntity.RemoteEntityName = *s.RemoteName + } + if s.Countable != nil { + existingEntity.Countable = *s.Countable + } + if s.Creatable != nil { + existingEntity.Creatable = *s.Creatable + } + if s.Deletable != nil { + existingEntity.Deletable = *s.Deletable + } + if s.Updatable != nil { + existingEntity.Updatable = *s.Updatable + } + if s.AllowCreateChangeLocally != nil { + existingEntity.CreateChangeLocally = *s.AllowCreateChangeLocally + } if len(attrs) > 0 { existingEntity.Attributes = attrs } @@ -829,6 +847,11 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt) return nil } + // Initial create: EntitySet is required (no prior value to preserve). + if s.EntitySet == nil || *s.EntitySet == "" { + return mdlerrors.NewValidation("EntitySet property is required when creating an external entity") + } + // Auto-position based on existing entities location := model.Point{X: 100 + len(dm.Entities)*150, Y: 100} @@ -840,13 +863,13 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt) Attributes: attrs, Source: "Rest$ODataRemoteEntitySource", RemoteServiceName: serviceRef, - RemoteEntitySet: s.EntitySet, - RemoteEntityName: s.RemoteName, - Countable: s.Countable, - Creatable: s.Creatable, - Deletable: s.Deletable, - Updatable: s.Updatable, - CreateChangeLocally: s.AllowCreateChangeLocally, + RemoteEntitySet: *s.EntitySet, + RemoteEntityName: derefString(s.RemoteName), + Countable: derefBool(s.Countable), + Creatable: derefBool(s.Creatable), + Deletable: derefBool(s.Deletable), + Updatable: derefBool(s.Updatable), + CreateChangeLocally: derefBool(s.AllowCreateChangeLocally), } newEntity.ID = model.ID(types.GenerateID()) @@ -857,6 +880,20 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt) return nil } +func derefString(p *string) string { + if p == nil { + return "" + } + return *p +} + +func derefBool(p *bool) bool { + if p == nil { + return false + } + return *p +} + // ============================================================================ // OData Write Handlers (CREATE / ALTER / DROP) // ============================================================================ diff --git a/mdl/executor/cmd_odata_mock_test.go b/mdl/executor/cmd_odata_mock_test.go index 74462f0e6..fae790dba 100644 --- a/mdl/executor/cmd_odata_mock_test.go +++ b/mdl/executor/cmd_odata_mock_test.go @@ -12,6 +12,9 @@ import ( "github.com/mendixlabs/mxcli/sdk/domainmodel" ) +func strPtr(s string) *string { return &s } +func boolPtr(b bool) *bool { return &b } + func TestShowODataClients_Mock(t *testing.T) { mod := mkModule("MyModule") svc := &model.ConsumedODataService{ @@ -227,7 +230,7 @@ func TestCreateExternalEntity_RejectsNonExistentClient(t *testing.T) { stmt := &ast.CreateExternalEntityStmt{ Name: ast.QualifiedName{Module: "MyModule", Name: "FakeEntity"}, ServiceRef: ast.QualifiedName{Module: "MyModule", Name: "NonExistentClient"}, - EntitySet: "Products", + EntitySet: strPtr("Products"), } err := execCreateExternalEntity(ctx, stmt) assertError(t, err) @@ -267,7 +270,7 @@ func TestCreateExternalEntity_AcceptsExistingClient(t *testing.T) { stmt := &ast.CreateExternalEntityStmt{ Name: ast.QualifiedName{Module: "MyModule", Name: "Product"}, ServiceRef: ast.QualifiedName{Module: "MyModule", Name: "ProductsClient"}, - EntitySet: "Products", + EntitySet: strPtr("Products"), } assertNoError(t, execCreateExternalEntity(ctx, stmt)) if created == nil { @@ -310,8 +313,8 @@ func TestCreateExternalEntity_AllowCreateChangeLocally_Issue534(t *testing.T) { stmt := &ast.CreateExternalEntityStmt{ Name: ast.QualifiedName{Module: "TripPin", Name: "People"}, ServiceRef: ast.QualifiedName{Module: "TripPin", Name: "TripPinClient"}, - EntitySet: "People", - AllowCreateChangeLocally: true, + EntitySet: strPtr("People"), + AllowCreateChangeLocally: boolPtr(true), } assertNoError(t, execCreateExternalEntity(ctx, stmt)) if created == nil { @@ -322,6 +325,98 @@ func TestCreateExternalEntity_AllowCreateChangeLocally_Issue534(t *testing.T) { } } +// TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594 verifies that +// CREATE OR MODIFY EXTERNAL ENTITY preserves the existing value of any field +// that is omitted from the MDL statement, rather than overwriting it with the +// zero value. Previously the executor unconditionally wrote `existingEntity.X = s.X` +// for every field, so omitting `RemoteName` on `or modify` wiped the BSON +// `RemoteName` to "" and triggered Studio Pro's +// ODataRemoteEntitySource.get_RemoteId() NRE in the Integration pane. +func TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594(t *testing.T) { + mod := mkModule("OdTest") + h := mkHierarchy(mod) + svc := &model.ConsumedODataService{ + BaseElement: model.BaseElement{ID: nextID("cos")}, + ContainerID: mod.ID, + Name: "SalesforceAPI", + } + withContainer(h, svc.ContainerID, mod.ID) + + // Pre-existing external entity with non-default values for every field + // that the bug could clobber. + existing := &domainmodel.Entity{ + BaseElement: model.BaseElement{ID: nextID("ent")}, + Name: "RemoteAccount", + Source: "Rest$ODataRemoteEntitySource", + RemoteServiceName: "OdTest.SalesforceAPI", + RemoteEntitySet: "Accounts", + RemoteEntityName: "Account", // <-- the field that #594 wiped + Countable: true, + Creatable: true, + Deletable: true, + Updatable: true, + CreateChangeLocally: false, + } + dm := &domainmodel.DomainModel{ + BaseElement: model.BaseElement{ID: nextID("dm")}, + ContainerID: mod.ID, + Entities: []*domainmodel.Entity{existing}, + } + + var updated *domainmodel.Entity + mb := &mock.MockBackend{ + IsConnectedFunc: func() bool { return true }, + ListModulesFunc: func() ([]*model.Module, error) { + return []*model.Module{mod}, nil + }, + GetDomainModelFunc: func(id model.ID) (*domainmodel.DomainModel, error) { + return dm, nil + }, + ListConsumedODataServicesFunc: func() ([]*model.ConsumedODataService, error) { + return []*model.ConsumedODataService{svc}, nil + }, + UpdateEntityFunc: func(dmID model.ID, entity *domainmodel.Entity) error { + updated = entity + return nil + }, + } + + ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h)) + + // Only mention EntitySet + AllowCreateChangeLocally — every other + // scalar should be preserved from the existing entity, not zeroed. + stmt := &ast.CreateExternalEntityStmt{ + Name: ast.QualifiedName{Module: "OdTest", Name: "RemoteAccount"}, + ServiceRef: ast.QualifiedName{Module: "OdTest", Name: "SalesforceAPI"}, + EntitySet: strPtr("Accounts"), + AllowCreateChangeLocally: boolPtr(true), + CreateOrModify: true, + } + assertNoError(t, execCreateExternalEntity(ctx, stmt)) + + if updated == nil { + t.Fatal("expected UpdateEntity to be called") + } + if updated.RemoteEntityName != "Account" { + t.Errorf("RemoteEntityName = %q, want \"Account\" (omitted RemoteName must preserve existing value)", updated.RemoteEntityName) + } + if !updated.Countable { + t.Errorf("Countable = false, want true (omitted Countable must preserve existing value)") + } + if !updated.Creatable { + t.Errorf("Creatable = false, want true (omitted Creatable must preserve existing value)") + } + if !updated.Deletable { + t.Errorf("Deletable = false, want true (omitted Deletable must preserve existing value)") + } + if !updated.Updatable { + t.Errorf("Updatable = false, want true (omitted Updatable must preserve existing value)") + } + if !updated.CreateChangeLocally { + t.Errorf("CreateChangeLocally = false, want true (explicitly set value)") + } +} + // TestDescribeODataService_ExposeRoundtrip verifies that DESCRIBE ODATA SERVICE // output for entities with key/filterable/sortable members is valid MDL that // the parser can re-parse (issue #400). diff --git a/mdl/visitor/visitor_odata.go b/mdl/visitor/visitor_odata.go index 6abef4cf3..ed2dc2ee5 100644 --- a/mdl/visitor/visitor_odata.go +++ b/mdl/visitor/visitor_odata.go @@ -165,21 +165,22 @@ func (b *Builder) ExitCreateExternalEntityStatement(ctx *parser.CreateExternalEn value := odataAssignmentValueText(prop) boolVal := strings.EqualFold(value, "true") || strings.EqualFold(value, "yes") + strVal := value switch strings.ToLower(name) { case "entityset": - stmt.EntitySet = value + stmt.EntitySet = &strVal case "remotename": - stmt.RemoteName = value + stmt.RemoteName = &strVal case "countable": - stmt.Countable = boolVal + stmt.Countable = &boolVal case "creatable": - stmt.Creatable = boolVal + stmt.Creatable = &boolVal case "deletable": - stmt.Deletable = boolVal + stmt.Deletable = &boolVal case "updatable": - stmt.Updatable = boolVal + stmt.Updatable = &boolVal case "allowcreatechangelocally", "allowcreatingandchanginglocally", "createchangelocally": - stmt.AllowCreateChangeLocally = boolVal + stmt.AllowCreateChangeLocally = &boolVal } } diff --git a/mdl/visitor/visitor_odata_test.go b/mdl/visitor/visitor_odata_test.go index 8b92e28d0..67e8fc240 100644 --- a/mdl/visitor/visitor_odata_test.go +++ b/mdl/visitor/visitor_odata_test.go @@ -96,11 +96,14 @@ func TestCreateExternalEntity(t *testing.T) { if stmt.ServiceRef.Name != "PetStore" { t.Errorf("Got ServiceRef %s", stmt.ServiceRef.Name) } - if stmt.EntitySet != "Products" { - t.Errorf("Got EntitySet %q", stmt.EntitySet) + if stmt.EntitySet == nil || *stmt.EntitySet != "Products" { + t.Errorf("Got EntitySet %v", stmt.EntitySet) } - if !stmt.Countable { - t.Error("Expected Countable true") + if stmt.RemoteName == nil || *stmt.RemoteName != "Product" { + t.Errorf("Got RemoteName %v", stmt.RemoteName) + } + if stmt.Countable == nil || !*stmt.Countable { + t.Errorf("Expected Countable true, got %v", stmt.Countable) } if len(stmt.Attributes) != 2 { t.Errorf("Expected 2 attributes, got %d", len(stmt.Attributes))