Skip to content

Commit dd67806

Browse files
akoclaude
andcommitted
fix #594: preserve omitted scalar fields on create or modify external entity
`execCreateExternalEntity` unconditionally wrote `existingEntity.X = s.X` for every property in the modify branch. When the MDL statement omitted `RemoteName`, `s.RemoteName` was the empty string and the pre-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`). Convert the scalar fields on `CreateExternalEntityStmt` to pointers so the executor can distinguish "omitted by the user" (nil → preserve existing value on `or modify`) from "explicitly set to zero" (`Countable: false`). This matches the existing tri-state convention already used for `AlterProjectSecurityStmt.DemoUsersEnabled *bool`. Wire the visitor to allocate pointers only when a property is present in the statement, and update the executor to gate each assignment on non-nil. Initial create still requires `EntitySet` (no prior value to preserve) and rejects the statement with a validation error if it's omitted. Add a focused mock-backend test that constructs a pre-existing entity with non-default values for every clobberable field, runs `or modify` with only `EntitySet` and `AllowCreateChangeLocally` set, and asserts all other fields retain their pre-existing values. The test fails on the old executor logic (verified by temporarily reverting the RemoteName branch — it reports `RemoteEntityName = "", want "Account"`). Add `mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl` as the end-to-end regression fixture for Studio Pro validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 4f0293f commit dd67806

6 files changed

Lines changed: 239 additions & 37 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
-- Bug test: `create or modify external entity` must preserve fields the
2+
-- statement omits rather than overwriting them with zero values.
3+
--
4+
-- Issue #594: the executor (mdl/executor/cmd_odata.go) unconditionally wrote
5+
-- `existingEntity.X = s.X` for every property. When the MDL statement omitted
6+
-- `RemoteName`, `s.RemoteName` was the empty string and the existing value
7+
-- was wiped. The resulting `Rest$ODataRemoteEntitySource` BSON had
8+
-- `RemoteName: ""`, which Studio Pro's Integration Pane visualiser then
9+
-- dereferences in `ODataRemoteEntitySource.get_RemoteId()` and NREs with
10+
-- ArgumentNullException("EntityTypeName") — Studio Pro aliases the BSON
11+
-- `RemoteName` field as the C# property `EntityTypeName`.
12+
--
13+
-- The Go unit test in mdl/executor/cmd_odata_mock_test.go
14+
-- (TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594) covers
15+
-- the executor logic with a mock backend. This MDL script is the end-to-end
16+
-- regression test: applying it to a real project must leave a
17+
-- bug594.RemoteAccount with RemoteName = "Account" after the modify, and
18+
-- the project must open in Studio Pro without the Integration Pane NRE.
19+
--
20+
-- Run with:
21+
-- mxcli exec mdl-examples/bug-tests/594-or-modify-external-entity-preserve-omitted.mdl -p <project>.mpr
22+
-- mxcli describe entity bug594.RemoteAccount -p <project>.mpr # RemoteName must still be 'Account'
23+
-- then open <project>.mpr in Studio Pro and confirm the Integration Pane renders.
24+
create module bug594;
25+
26+
create odata client bug594.SalesforceAPI (
27+
ODataVersion: OData4,
28+
MetadataUrl: 'https://api.salesforce.com/odata/v4/$metadata',
29+
timeout: 300
30+
);
31+
32+
create external entity bug594.RemoteAccount
33+
from odata client bug594.SalesforceAPI
34+
(
35+
EntitySet: 'Accounts',
36+
RemoteName: 'Account',
37+
Countable: Yes,
38+
Creatable: Yes,
39+
Deletable: No,
40+
Updatable: No
41+
)
42+
(
43+
AccountId: string(200),
44+
AccountName: string(255)
45+
);
46+
47+
-- Modify without restating RemoteName / Countable / Creatable. Before the
48+
-- fix, every omitted property was overwritten with its zero value and the
49+
-- entity ended up unusable. After the fix, all of them are preserved.
50+
create or modify external entity bug594.RemoteAccount
51+
from odata client bug594.SalesforceAPI
52+
(
53+
EntitySet: 'Accounts',
54+
AllowCreateChangeLocally: Yes
55+
)
56+
(
57+
AccountId: string(200),
58+
AccountName: string(255)
59+
);
60+
61+
describe entity bug594.RemoteAccount;

mdl/ast/ast_odata.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -121,16 +121,21 @@ type DropODataServiceStmt struct {
121121
func (s *DropODataServiceStmt) isStatement() {}
122122

123123
// CreateExternalEntityStmt represents: CREATE [OR MODIFY] EXTERNAL ENTITY Module.Name FROM ODATA CLIENT Module.Service (...) (attrs);
124+
//
125+
// Scalar property fields are pointers so the executor can distinguish
126+
// "omitted by user" (nil → preserve existing value on CREATE OR MODIFY)
127+
// from "explicitly set to zero" (e.g. Countable: false). Treating omitted
128+
// fields as zero on modify silently corrupted entities — see issue #594.
124129
type CreateExternalEntityStmt struct {
125130
Name QualifiedName
126131
ServiceRef QualifiedName // FROM ODATA CLIENT ...
127-
EntitySet string
128-
RemoteName string
129-
Countable bool
130-
Creatable bool
131-
Deletable bool
132-
Updatable bool
133-
AllowCreateChangeLocally bool // "Allow creating and changing locally" flag
132+
EntitySet *string
133+
RemoteName *string
134+
Countable *bool
135+
Creatable *bool
136+
Deletable *bool
137+
Updatable *bool
138+
AllowCreateChangeLocally *bool // "Allow creating and changing locally" flag
134139
Attributes []Attribute // reuse from ast_entity.go
135140
Documentation string
136141
CreateOrModify bool

mdl/executor/cmd_odata.go

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -806,16 +806,34 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt)
806806
serviceRef := s.ServiceRef.String()
807807

808808
if existingEntity != nil {
809-
// Update existing entity
809+
// Update existing entity. Issue #594: only assign fields that the
810+
// MDL statement explicitly set. Omitted (nil) fields preserve the
811+
// prior value — previously the executor unconditionally wrote zero
812+
// values, wiping fields like RemoteName and triggering Studio Pro
813+
// NREs (e.g. ODataRemoteEntitySource.RemoteId on empty RemoteName).
810814
existingEntity.Source = "Rest$ODataRemoteEntitySource"
811815
existingEntity.RemoteServiceName = serviceRef
812-
existingEntity.RemoteEntitySet = s.EntitySet
813-
existingEntity.RemoteEntityName = s.RemoteName
814-
existingEntity.Countable = s.Countable
815-
existingEntity.Creatable = s.Creatable
816-
existingEntity.Deletable = s.Deletable
817-
existingEntity.Updatable = s.Updatable
818-
existingEntity.CreateChangeLocally = s.AllowCreateChangeLocally
816+
if s.EntitySet != nil {
817+
existingEntity.RemoteEntitySet = *s.EntitySet
818+
}
819+
if s.RemoteName != nil {
820+
existingEntity.RemoteEntityName = *s.RemoteName
821+
}
822+
if s.Countable != nil {
823+
existingEntity.Countable = *s.Countable
824+
}
825+
if s.Creatable != nil {
826+
existingEntity.Creatable = *s.Creatable
827+
}
828+
if s.Deletable != nil {
829+
existingEntity.Deletable = *s.Deletable
830+
}
831+
if s.Updatable != nil {
832+
existingEntity.Updatable = *s.Updatable
833+
}
834+
if s.AllowCreateChangeLocally != nil {
835+
existingEntity.CreateChangeLocally = *s.AllowCreateChangeLocally
836+
}
819837
if len(attrs) > 0 {
820838
existingEntity.Attributes = attrs
821839
}
@@ -829,6 +847,11 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt)
829847
return nil
830848
}
831849

850+
// Initial create: EntitySet is required (no prior value to preserve).
851+
if s.EntitySet == nil || *s.EntitySet == "" {
852+
return mdlerrors.NewValidation("EntitySet property is required when creating an external entity")
853+
}
854+
832855
// Auto-position based on existing entities
833856
location := model.Point{X: 100 + len(dm.Entities)*150, Y: 100}
834857

@@ -840,13 +863,13 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt)
840863
Attributes: attrs,
841864
Source: "Rest$ODataRemoteEntitySource",
842865
RemoteServiceName: serviceRef,
843-
RemoteEntitySet: s.EntitySet,
844-
RemoteEntityName: s.RemoteName,
845-
Countable: s.Countable,
846-
Creatable: s.Creatable,
847-
Deletable: s.Deletable,
848-
Updatable: s.Updatable,
849-
CreateChangeLocally: s.AllowCreateChangeLocally,
866+
RemoteEntitySet: *s.EntitySet,
867+
RemoteEntityName: derefString(s.RemoteName),
868+
Countable: derefBool(s.Countable),
869+
Creatable: derefBool(s.Creatable),
870+
Deletable: derefBool(s.Deletable),
871+
Updatable: derefBool(s.Updatable),
872+
CreateChangeLocally: derefBool(s.AllowCreateChangeLocally),
850873
}
851874
newEntity.ID = model.ID(types.GenerateID())
852875

@@ -857,6 +880,20 @@ func execCreateExternalEntity(ctx *ExecContext, s *ast.CreateExternalEntityStmt)
857880
return nil
858881
}
859882

883+
func derefString(p *string) string {
884+
if p == nil {
885+
return ""
886+
}
887+
return *p
888+
}
889+
890+
func derefBool(p *bool) bool {
891+
if p == nil {
892+
return false
893+
}
894+
return *p
895+
}
896+
860897
// ============================================================================
861898
// OData Write Handlers (CREATE / ALTER / DROP)
862899
// ============================================================================

mdl/executor/cmd_odata_mock_test.go

Lines changed: 99 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ import (
1212
"github.com/mendixlabs/mxcli/sdk/domainmodel"
1313
)
1414

15+
func strPtr(s string) *string { return &s }
16+
func boolPtr(b bool) *bool { return &b }
17+
1518
func TestShowODataClients_Mock(t *testing.T) {
1619
mod := mkModule("MyModule")
1720
svc := &model.ConsumedODataService{
@@ -227,7 +230,7 @@ func TestCreateExternalEntity_RejectsNonExistentClient(t *testing.T) {
227230
stmt := &ast.CreateExternalEntityStmt{
228231
Name: ast.QualifiedName{Module: "MyModule", Name: "FakeEntity"},
229232
ServiceRef: ast.QualifiedName{Module: "MyModule", Name: "NonExistentClient"},
230-
EntitySet: "Products",
233+
EntitySet: strPtr("Products"),
231234
}
232235
err := execCreateExternalEntity(ctx, stmt)
233236
assertError(t, err)
@@ -267,7 +270,7 @@ func TestCreateExternalEntity_AcceptsExistingClient(t *testing.T) {
267270
stmt := &ast.CreateExternalEntityStmt{
268271
Name: ast.QualifiedName{Module: "MyModule", Name: "Product"},
269272
ServiceRef: ast.QualifiedName{Module: "MyModule", Name: "ProductsClient"},
270-
EntitySet: "Products",
273+
EntitySet: strPtr("Products"),
271274
}
272275
assertNoError(t, execCreateExternalEntity(ctx, stmt))
273276
if created == nil {
@@ -310,8 +313,8 @@ func TestCreateExternalEntity_AllowCreateChangeLocally_Issue534(t *testing.T) {
310313
stmt := &ast.CreateExternalEntityStmt{
311314
Name: ast.QualifiedName{Module: "TripPin", Name: "People"},
312315
ServiceRef: ast.QualifiedName{Module: "TripPin", Name: "TripPinClient"},
313-
EntitySet: "People",
314-
AllowCreateChangeLocally: true,
316+
EntitySet: strPtr("People"),
317+
AllowCreateChangeLocally: boolPtr(true),
315318
}
316319
assertNoError(t, execCreateExternalEntity(ctx, stmt))
317320
if created == nil {
@@ -322,6 +325,98 @@ func TestCreateExternalEntity_AllowCreateChangeLocally_Issue534(t *testing.T) {
322325
}
323326
}
324327

328+
// TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594 verifies that
329+
// CREATE OR MODIFY EXTERNAL ENTITY preserves the existing value of any field
330+
// that is omitted from the MDL statement, rather than overwriting it with the
331+
// zero value. Previously the executor unconditionally wrote `existingEntity.X = s.X`
332+
// for every field, so omitting `RemoteName` on `or modify` wiped the BSON
333+
// `RemoteName` to "" and triggered Studio Pro's
334+
// ODataRemoteEntitySource.get_RemoteId() NRE in the Integration pane.
335+
func TestCreateOrModifyExternalEntity_PreservesOmittedFields_Issue594(t *testing.T) {
336+
mod := mkModule("OdTest")
337+
h := mkHierarchy(mod)
338+
svc := &model.ConsumedODataService{
339+
BaseElement: model.BaseElement{ID: nextID("cos")},
340+
ContainerID: mod.ID,
341+
Name: "SalesforceAPI",
342+
}
343+
withContainer(h, svc.ContainerID, mod.ID)
344+
345+
// Pre-existing external entity with non-default values for every field
346+
// that the bug could clobber.
347+
existing := &domainmodel.Entity{
348+
BaseElement: model.BaseElement{ID: nextID("ent")},
349+
Name: "RemoteAccount",
350+
Source: "Rest$ODataRemoteEntitySource",
351+
RemoteServiceName: "OdTest.SalesforceAPI",
352+
RemoteEntitySet: "Accounts",
353+
RemoteEntityName: "Account", // <-- the field that #594 wiped
354+
Countable: true,
355+
Creatable: true,
356+
Deletable: true,
357+
Updatable: true,
358+
CreateChangeLocally: false,
359+
}
360+
dm := &domainmodel.DomainModel{
361+
BaseElement: model.BaseElement{ID: nextID("dm")},
362+
ContainerID: mod.ID,
363+
Entities: []*domainmodel.Entity{existing},
364+
}
365+
366+
var updated *domainmodel.Entity
367+
mb := &mock.MockBackend{
368+
IsConnectedFunc: func() bool { return true },
369+
ListModulesFunc: func() ([]*model.Module, error) {
370+
return []*model.Module{mod}, nil
371+
},
372+
GetDomainModelFunc: func(id model.ID) (*domainmodel.DomainModel, error) {
373+
return dm, nil
374+
},
375+
ListConsumedODataServicesFunc: func() ([]*model.ConsumedODataService, error) {
376+
return []*model.ConsumedODataService{svc}, nil
377+
},
378+
UpdateEntityFunc: func(dmID model.ID, entity *domainmodel.Entity) error {
379+
updated = entity
380+
return nil
381+
},
382+
}
383+
384+
ctx, _ := newMockCtx(t, withBackend(mb), withHierarchy(h))
385+
386+
// Only mention EntitySet + AllowCreateChangeLocally — every other
387+
// scalar should be preserved from the existing entity, not zeroed.
388+
stmt := &ast.CreateExternalEntityStmt{
389+
Name: ast.QualifiedName{Module: "OdTest", Name: "RemoteAccount"},
390+
ServiceRef: ast.QualifiedName{Module: "OdTest", Name: "SalesforceAPI"},
391+
EntitySet: strPtr("Accounts"),
392+
AllowCreateChangeLocally: boolPtr(true),
393+
CreateOrModify: true,
394+
}
395+
assertNoError(t, execCreateExternalEntity(ctx, stmt))
396+
397+
if updated == nil {
398+
t.Fatal("expected UpdateEntity to be called")
399+
}
400+
if updated.RemoteEntityName != "Account" {
401+
t.Errorf("RemoteEntityName = %q, want \"Account\" (omitted RemoteName must preserve existing value)", updated.RemoteEntityName)
402+
}
403+
if !updated.Countable {
404+
t.Errorf("Countable = false, want true (omitted Countable must preserve existing value)")
405+
}
406+
if !updated.Creatable {
407+
t.Errorf("Creatable = false, want true (omitted Creatable must preserve existing value)")
408+
}
409+
if !updated.Deletable {
410+
t.Errorf("Deletable = false, want true (omitted Deletable must preserve existing value)")
411+
}
412+
if !updated.Updatable {
413+
t.Errorf("Updatable = false, want true (omitted Updatable must preserve existing value)")
414+
}
415+
if !updated.CreateChangeLocally {
416+
t.Errorf("CreateChangeLocally = false, want true (explicitly set value)")
417+
}
418+
}
419+
325420
// TestDescribeODataService_ExposeRoundtrip verifies that DESCRIBE ODATA SERVICE
326421
// output for entities with key/filterable/sortable members is valid MDL that
327422
// the parser can re-parse (issue #400).

mdl/visitor/visitor_odata.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,21 +165,22 @@ func (b *Builder) ExitCreateExternalEntityStatement(ctx *parser.CreateExternalEn
165165
value := odataAssignmentValueText(prop)
166166

167167
boolVal := strings.EqualFold(value, "true") || strings.EqualFold(value, "yes")
168+
strVal := value
168169
switch strings.ToLower(name) {
169170
case "entityset":
170-
stmt.EntitySet = value
171+
stmt.EntitySet = &strVal
171172
case "remotename":
172-
stmt.RemoteName = value
173+
stmt.RemoteName = &strVal
173174
case "countable":
174-
stmt.Countable = boolVal
175+
stmt.Countable = &boolVal
175176
case "creatable":
176-
stmt.Creatable = boolVal
177+
stmt.Creatable = &boolVal
177178
case "deletable":
178-
stmt.Deletable = boolVal
179+
stmt.Deletable = &boolVal
179180
case "updatable":
180-
stmt.Updatable = boolVal
181+
stmt.Updatable = &boolVal
181182
case "allowcreatechangelocally", "allowcreatingandchanginglocally", "createchangelocally":
182-
stmt.AllowCreateChangeLocally = boolVal
183+
stmt.AllowCreateChangeLocally = &boolVal
183184
}
184185
}
185186

mdl/visitor/visitor_odata_test.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,14 @@ func TestCreateExternalEntity(t *testing.T) {
9696
if stmt.ServiceRef.Name != "PetStore" {
9797
t.Errorf("Got ServiceRef %s", stmt.ServiceRef.Name)
9898
}
99-
if stmt.EntitySet != "Products" {
100-
t.Errorf("Got EntitySet %q", stmt.EntitySet)
99+
if stmt.EntitySet == nil || *stmt.EntitySet != "Products" {
100+
t.Errorf("Got EntitySet %v", stmt.EntitySet)
101101
}
102-
if !stmt.Countable {
103-
t.Error("Expected Countable true")
102+
if stmt.RemoteName == nil || *stmt.RemoteName != "Product" {
103+
t.Errorf("Got RemoteName %v", stmt.RemoteName)
104+
}
105+
if stmt.Countable == nil || !*stmt.Countable {
106+
t.Errorf("Expected Countable true, got %v", stmt.Countable)
104107
}
105108
if len(stmt.Attributes) != 2 {
106109
t.Errorf("Expected 2 attributes, got %d", len(stmt.Attributes))

0 commit comments

Comments
 (0)