Merge pull request #243 from retran/fix/dset-silent-failure

fix: handle missing TaskPage key in workflow activity mutation

Author: ako

mdl/backend/mpr/backend.go

@@ -85,9 +85,11 @@ func (b *MprBackend) Path() string      { return b.path }
 // for new code.
 func (b *MprBackend) MprReader() *mpr.Reader { return b.reader }
 
-func (b *MprBackend) Version() types.MPRVersion                 { return convertMPRVersion(b.reader.Version()) }
-func (b *MprBackend) ProjectVersion() *types.ProjectVersion     { return convertProjectVersion(b.reader.ProjectVersion()) }
-func (b *MprBackend) GetMendixVersion() (string, error)         { return b.reader.GetMendixVersion() }
+func (b *MprBackend) Version() types.MPRVersion { return convertMPRVersion(b.reader.Version()) }
+func (b *MprBackend) ProjectVersion() *types.ProjectVersion {
+	return convertProjectVersion(b.reader.ProjectVersion())
+}
+func (b *MprBackend) GetMendixVersion() (string, error) { return b.reader.GetMendixVersion() }
 
 // Commit is a no-op — the MPR writer auto-commits on each write operation.
 func (b *MprBackend) Commit() error { return nil }
@@ -112,7 +114,9 @@ func (b *MprBackend) DeleteModuleWithCleanup(id model.ID, moduleName string) err
 // FolderBackend
 // ---------------------------------------------------------------------------
 
-func (b *MprBackend) ListFolders() ([]*types.FolderInfo, error) { return convertFolderInfoSlice(b.reader.ListFolders()) }
+func (b *MprBackend) ListFolders() ([]*types.FolderInfo, error) {
+	return convertFolderInfoSlice(b.reader.ListFolders())
+}
 func (b *MprBackend) CreateFolder(folder *model.Folder) error { return b.writer.CreateFolder(folder) }
 func (b *MprBackend) DeleteFolder(id model.ID) error          { return b.writer.DeleteFolder(id) }
 func (b *MprBackend) MoveFolder(id model.ID, newContainerID model.ID) error {
@@ -678,8 +682,10 @@ func (b *MprBackend) UpdateRawUnit(unitID string, contents []byte) error {
 // MetadataBackend
 // ---------------------------------------------------------------------------
 
-func (b *MprBackend) ListAllUnitIDs() ([]string, error)     { return b.reader.ListAllUnitIDs() }
-func (b *MprBackend) ListUnits() ([]*types.UnitInfo, error)   { return convertUnitInfoSlice(b.reader.ListUnits()) }
+func (b *MprBackend) ListAllUnitIDs() ([]string, error) { return b.reader.ListAllUnitIDs() }
+func (b *MprBackend) ListUnits() ([]*types.UnitInfo, error) {
+	return convertUnitInfoSlice(b.reader.ListUnits())
+}
 func (b *MprBackend) GetUnitTypes() (map[string]int, error) { return b.reader.GetUnitTypes() }
 func (b *MprBackend) GetProjectRootID() (string, error)     { return b.reader.GetProjectRootID() }
 func (b *MprBackend) ContentsDir() string                   { return b.reader.ContentsDir() }

mdl/backend/mpr/convert_roundtrip_test.go

@@ -648,4 +648,3 @@ func TestFieldCountDrift(t *testing.T) {
 	assertFieldCount(t, "mpr.EntityAccessRevocation", mpr.EntityAccessRevocation{}, 6)
 	assertFieldCount(t, "types.EntityAccessRevocation", types.EntityAccessRevocation{}, 6)
 }
-

mdl/backend/mpr/workflow_mutator.go

@@ -162,14 +162,19 @@ func (m *mprWorkflowMutator) SetActivityProperty(activityRef string, atPos int,
 	case "page":
 		taskPage := dGetDoc(actDoc, "TaskPage")
 		if taskPage != nil {
+			// TaskPage exists and has a value — update the Page field in place.
 			dSet(taskPage, "Page", value)
-		} else {
-			pageRef := bson.D{
-				{Key: "$ID", Value: bsonutil.NewIDBsonBinary()},
-				{Key: "$Type", Value: "Workflows$PageReference"},
-				{Key: "Page", Value: value},
-			}
-			dSet(actDoc, "TaskPage", pageRef)
+			return nil
+		}
+		pageRef := bson.D{
+			{Key: "$ID", Value: bsonutil.NewIDBsonBinary()},
+			{Key: "$Type", Value: "Workflows$PageReference"},
+			{Key: "Page", Value: value},
+		}
+		if !dSet(actDoc, "TaskPage", pageRef) {
+			// TaskPage key absent — append to activity and replace in BSON tree.
+			actDoc = append(actDoc, bson.E{Key: "TaskPage", Value: pageRef})
+			m.replaceActivity(actDoc)
 		}
 		return nil
 
@@ -555,6 +560,42 @@ func (m *mprWorkflowMutator) Save() error {
 // Internal helpers — activity search
 // ---------------------------------------------------------------------------
 
+// replaceActivity replaces an activity document in the workflow's BSON tree
+// by matching on $ID. This is needed when appending new keys to an activity
+// document, because the slice header returned by findActivityByCaption cannot
+// propagate appends back to the parent bson.A.
+func (m *mprWorkflowMutator) replaceActivity(updated bson.D) {
+	actID := extractBinaryIDFromDoc(dGet(updated, "$ID"))
+	if actID == "" {
+		return
+	}
+	flow := dGetDoc(m.rawData, "Flow")
+	if flow == nil {
+		return
+	}
+	replaceActivityRecursive(flow, actID, updated)
+}
+
+func replaceActivityRecursive(flow bson.D, actID string, updated bson.D) bool {
+	elements := dGetArrayElements(dGet(flow, "Activities"))
+	for i, elem := range elements {
+		actDoc, ok := elem.(bson.D)
+		if !ok {
+			continue
+		}
+		if extractBinaryIDFromDoc(dGet(actDoc, "$ID")) == actID {
+			elements[i] = updated
+			return true
+		}
+		for _, nestedFlow := range getNestedFlows(actDoc) {
+			if replaceActivityRecursive(nestedFlow, actID, updated) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // findActivityByCaption searches the workflow for an activity matching caption.
 func (m *mprWorkflowMutator) findActivityByCaption(caption string, atPosition int) (bson.D, error) {
 	flow := dGetDoc(m.rawData, "Flow")

mdl/backend/mpr/workflow_mutator_test.go

@@ -1136,8 +1136,7 @@ func TestWorkflowMutator_InsertBoundaryEvent_NoDelay(t *testing.T) {
 // ---------------------------------------------------------------------------
 
 func TestWorkflowMutator_SetActivityProperty_Page_New(t *testing.T) {
-	// Note: When TaskPage key doesn't pre-exist in BSON, dSet silently fails.
-	// The key must be present (even as nil) for PAGE to work on a new activity.
+	// TaskPage key present with nil value — should be replaced with a new PageReference.
 	act := makeWfActivity("Workflows$UserTask", "Review", "task1")
 	act = append(act, bson.E{Key: "TaskPage", Value: nil})
 	m := newMutator(makeWorkflowDoc(act))
@@ -1157,7 +1156,8 @@ func TestWorkflowMutator_SetActivityProperty_Page_New(t *testing.T) {
 }
 
 func TestWorkflowMutator_SetActivityProperty_Page_MissingKey(t *testing.T) {
-	// BUG: dSet silently fails when TaskPage key is absent — pageRef is lost.
+	// Regression test: dSet silently failed when TaskPage key was absent.
+	// Fixed by appending the key to the activity and replacing it in the BSON tree.
 	act := makeWfActivity("Workflows$UserTask", "Review", "task1")
 	// No TaskPage field at all
 	m := newMutator(makeWorkflowDoc(act))
@@ -1169,9 +1169,67 @@ func TestWorkflowMutator_SetActivityProperty_Page_MissingKey(t *testing.T) {
 
 	actDoc, _ := m.findActivityByCaption("Review", 0)
 	taskPage := dGetDoc(actDoc, "TaskPage")
-	// This documents the bug: TaskPage is nil because dSet can't create new keys
-	if taskPage != nil {
-		t.Log("BUG FIXED: TaskPage is now set even when key was absent")
+	if taskPage == nil {
+		t.Fatal("TaskPage should be set even when key was absent")
+	}
+	if got := dGetString(taskPage, "Page"); got != "MyModule.TaskPage" {
+		t.Errorf("Page = %q, want MyModule.TaskPage", got)
+	}
+}
+
+func TestWorkflowMutator_SetActivityProperty_Page_MissingKey_NestedSubFlow(t *testing.T) {
+	// Exercises the recursive replaceActivity path: the target activity lives
+	// inside an outcome's sub-flow, not at the top level.
+	// Use distinct $IDs so replaceActivity cannot accidentally match the parent.
+	parentID := primitive.Binary{Subtype: 0x04, Data: []byte{1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}
+	nestedID := primitive.Binary{Subtype: 0x04, Data: []byte{2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}
+
+	nestedAct := bson.D{
+		{Key: "$ID", Value: nestedID},
+		{Key: "$Type", Value: "Workflows$UserTask"},
+		{Key: "Caption", Value: "NestedReview"},
+		{Key: "Name", Value: "nested1"},
+	}
+	// No TaskPage field at all on the nested activity.
+
+	outcome := bson.D{
+		{Key: "$ID", Value: primitive.Binary{Subtype: 0x04, Data: make([]byte, 16)}},
+		{Key: "$Type", Value: "Workflows$BooleanOutcome"},
+		{Key: "Flow", Value: bson.D{
+			{Key: "$ID", Value: primitive.Binary{Subtype: 0x04, Data: make([]byte, 16)}},
+			{Key: "$Type", Value: "Workflows$Flow"},
+			{Key: "Activities", Value: bson.A{int32(3), nestedAct}},
+		}},
+	}
+	parentAct := bson.D{
+		{Key: "$ID", Value: parentID},
+		{Key: "$Type", Value: "Workflows$Decision"},
+		{Key: "Caption", Value: "Check"},
+		{Key: "Name", Value: "decision1"},
+		{Key: "Outcomes", Value: bson.A{int32(3), outcome}},
+	}
+	m := newMutator(makeWorkflowDoc(parentAct))
+
+	if err := m.SetActivityProperty("NestedReview", 0, "PAGE", "MyModule.NestedPage"); err != nil {
+		t.Fatalf("SetActivityProperty PAGE on nested activity failed: %v", err)
+	}
+
+	actDoc, _ := m.findActivityByCaption("NestedReview", 0)
+	taskPage := dGetDoc(actDoc, "TaskPage")
+	if taskPage == nil {
+		t.Fatal("TaskPage should be set on nested activity even when key was absent")
+	}
+	if got := dGetString(taskPage, "Page"); got != "MyModule.NestedPage" {
+		t.Errorf("Page = %q, want MyModule.NestedPage", got)
+	}
+
+	// Verify parent decision still has its Outcomes intact.
+	parentDoc, _ := m.findActivityByCaption("Check", 0)
+	if parentDoc == nil {
+		t.Fatal("parent decision activity should still exist")
+	}
+	if outcomes := dGet(parentDoc, "Outcomes"); outcomes == nil {
+		t.Fatal("parent decision Outcomes should still be present")
 	}
 }
 

mdl/executor/widget_registry.go

@@ -16,8 +16,8 @@ import (
 
 // WidgetRegistry holds loaded widget definitions keyed by uppercase MDL name.
 type WidgetRegistry struct {
-	byMDLName      map[string]*WidgetDefinition // keyed by uppercase MDLName
-	byWidgetID     map[string]*WidgetDefinition // keyed by widgetId
+	byMDLName       map[string]*WidgetDefinition // keyed by uppercase MDLName
+	byWidgetID      map[string]*WidgetDefinition // keyed by widgetId
 	knownOperations map[string]bool              // operations accepted during validation
 }
 

mdl/types/edmx_test.go

@@ -217,9 +217,9 @@ func TestFindEntityType(t *testing.T) {
 
 func TestResolveNavType(t *testing.T) {
 	tests := []struct {
-		input      string
-		typeName   string
-		isMany     bool
+		input    string
+		typeName string
+		isMany   bool
 	}{
 		{"Collection(NS.Order)", "Order", true},
 		{"NS.Customer", "Customer", false},

mdl/types/id_test.go

@@ -156,7 +156,7 @@ func TestValidateID(t *testing.T) {
 		{"AABBCCDD-EEFF-1122-3344-556677889900", true},
 		{"", false},
 		{"too-short", false},
-		{"a1b2c3d4-e5f6-7890-abcd-ef123456789", false},  // 35 chars
+		{"a1b2c3d4-e5f6-7890-abcd-ef123456789", false},   // 35 chars
 		{"a1b2c3d4-e5f6-7890-abcd-ef12345678901", false}, // 37 chars
 		{"a1b2c3d4xe5f6-7890-abcd-ef1234567890", false},  // wrong separator
 		{"g1b2c3d4-e5f6-7890-abcd-ef1234567890", false},  // invalid hex