Skip to content

Commit 2654008

Browse files
engalarclaude
engalar
and
claude
committed
fix: address code review issues in ALTER WORKFLOW
- Fix append bug in applySetWorkflowProperty: change signature to *bson.D so PARAMETER insertion is visible to caller - Extract buildSubFlowBson helper to eliminate 4 duplicated sub-flow construction patterns (~60 lines removed) - Add SetName to WorkflowActivity interface, replacing 9-branch type switch in deduplicateNewActivityName with single method call - Fix single-line for loop formatting in applyReplaceActivity Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 14c7b45 commit 2654008

2 files changed

Lines changed: 56 additions & 119 deletions

File tree

‎mdl/executor/cmd_alter_workflow.go‎

Lines changed: 50 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (e *Executor) execAlterWorkflow(s *ast.AlterWorkflowStmt) error {
6161
for _, op := range s.Operations {
6262
switch o := op.(type) {
6363
case *ast.SetWorkflowPropertyOp:
64-
if err := applySetWorkflowProperty(rawData, o); err != nil {
64+
if err := applySetWorkflowProperty(&rawData, o); err != nil {
6565
return fmt.Errorf("SET %s failed: %w", o.Property, err)
6666
}
6767
case *ast.SetActivityPropertyOp:
@@ -134,64 +134,63 @@ func (e *Executor) execAlterWorkflow(s *ast.AlterWorkflowStmt) error {
134134
}
135135

136136
// applySetWorkflowProperty sets a workflow-level property in raw BSON.
137-
func applySetWorkflowProperty(doc bson.D, op *ast.SetWorkflowPropertyOp) error {
137+
func applySetWorkflowProperty(doc *bson.D, op *ast.SetWorkflowPropertyOp) error {
138138
switch op.Property {
139139
case "DISPLAY":
140140
// WorkflowName is a StringTemplate with Text field
141-
wfName := dGetDoc(doc, "WorkflowName")
141+
wfName := dGetDoc(*doc, "WorkflowName")
142142
if wfName != nil {
143143
dSet(wfName, "Text", op.Value)
144144
}
145145
// Also update Title (top-level string)
146-
dSet(doc, "Title", op.Value)
146+
dSet(*doc, "Title", op.Value)
147147
return nil
148148

149149
case "DESCRIPTION":
150150
// WorkflowDescription is a StringTemplate with Text field
151-
wfDesc := dGetDoc(doc, "WorkflowDescription")
151+
wfDesc := dGetDoc(*doc, "WorkflowDescription")
152152
if wfDesc != nil {
153153
dSet(wfDesc, "Text", op.Value)
154154
}
155155
return nil
156156

157157
case "EXPORT_LEVEL":
158-
dSet(doc, "ExportLevel", op.Value)
158+
dSet(*doc, "ExportLevel", op.Value)
159159
return nil
160160

161161
case "DUE_DATE":
162-
dSet(doc, "DueDate", op.Value)
162+
dSet(*doc, "DueDate", op.Value)
163163
return nil
164164

165165
case "OVERVIEW_PAGE":
166166
qn := op.Entity.Module + "." + op.Entity.Name
167167
if qn == "." {
168168
// Clear overview page
169-
dSet(doc, "AdminPage", nil)
169+
dSet(*doc, "AdminPage", nil)
170170
} else {
171171
pageRef := bson.D{
172172
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
173173
{Key: "$Type", Value: "Workflows$PageReference"},
174174
{Key: "Page", Value: qn},
175175
}
176-
dSet(doc, "AdminPage", pageRef)
176+
dSet(*doc, "AdminPage", pageRef)
177177
}
178178
return nil
179179

180180
case "PARAMETER":
181181
qn := op.Entity.Module + "." + op.Entity.Name
182182
if qn == "." {
183183
// Clear parameter — remove it
184-
for i, elem := range doc {
184+
for i, elem := range *doc {
185185
if elem.Key == "Parameter" {
186-
// Set to nil (remove the parameter)
187-
doc[i].Value = nil
186+
(*doc)[i].Value = nil
188187
return nil
189188
}
190189
}
191190
return nil
192191
}
193192
// Check if Parameter already exists
194-
param := dGetDoc(doc, "Parameter")
193+
param := dGetDoc(*doc, "Parameter")
195194
if param != nil {
196195
dSet(param, "Entity", qn)
197196
} else {
@@ -202,22 +201,15 @@ func applySetWorkflowProperty(doc bson.D, op *ast.SetWorkflowPropertyOp) error {
202201
{Key: "Entity", Value: qn},
203202
{Key: "Name", Value: "WorkflowContext"},
204203
}
205-
// Append to doc (Parameter field)
206-
for i, elem := range doc {
204+
// Check if field exists with nil value
205+
for i, elem := range *doc {
207206
if elem.Key == "Parameter" {
208-
doc[i].Value = newParam
209-
return nil
210-
}
211-
}
212-
// Field doesn't exist, need to add it before PersistentId
213-
for i, elem := range doc {
214-
if elem.Key == "PersistentId" {
215-
// Insert before PersistentId
216-
doc = append(doc[:i+1], doc[i:]...)
217-
doc[i] = bson.E{Key: "Parameter", Value: newParam}
207+
(*doc)[i].Value = newParam
218208
return nil
219209
}
220210
}
211+
// Field doesn't exist — append it
212+
*doc = append(*doc, bson.E{Key: "Parameter", Value: newParam})
221213
}
222214
return nil
223215

@@ -454,33 +446,37 @@ func deduplicateNewActivityName(act workflows.WorkflowActivity, existingNames ma
454446
for i := 2; i < 1000; i++ {
455447
candidate := fmt.Sprintf("%s_%d", name, i)
456448
if !existingNames[candidate] {
457-
// Update the activity's Name field
458-
switch a := act.(type) {
459-
case *workflows.CallMicroflowTask:
460-
a.Name = candidate
461-
case *workflows.UserTask:
462-
a.Name = candidate
463-
case *workflows.ExclusiveSplitActivity:
464-
a.Name = candidate
465-
case *workflows.ParallelSplitActivity:
466-
a.Name = candidate
467-
case *workflows.JumpToActivity:
468-
a.Name = candidate
469-
case *workflows.WaitForTimerActivity:
470-
a.Name = candidate
471-
case *workflows.WaitForNotificationActivity:
472-
a.Name = candidate
473-
case *workflows.CallWorkflowActivity:
474-
a.Name = candidate
475-
case *workflows.WorkflowAnnotationActivity:
476-
a.Name = candidate
477-
}
449+
act.SetName(candidate)
478450
existingNames[candidate] = true
479451
return
480452
}
481453
}
482454
}
483455

456+
// buildSubFlowBson builds a Workflows$Flow BSON document from AST activity nodes,
457+
// with auto-binding and name deduplication against existing workflow activities.
458+
func (e *Executor) buildSubFlowBson(doc bson.D, activities []ast.WorkflowActivityNode) bson.D {
459+
subActs := buildWorkflowActivities(activities)
460+
e.autoBindActivitiesInFlow(subActs)
461+
existingNames := collectAllActivityNames(doc)
462+
for _, act := range subActs {
463+
deduplicateNewActivityName(act, existingNames)
464+
}
465+
var subActsBson bson.A
466+
subActsBson = append(subActsBson, int32(3))
467+
for _, act := range subActs {
468+
bsonDoc := mpr.SerializeWorkflowActivity(act)
469+
if bsonDoc != nil {
470+
subActsBson = append(subActsBson, bsonDoc)
471+
}
472+
}
473+
return bson.D{
474+
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
475+
{Key: "$Type", Value: "Workflows$Flow"},
476+
{Key: "Activities", Value: subActsBson},
477+
}
478+
}
479+
484480
// applyInsertAfterActivity inserts a new activity after a named activity.
485481
func (e *Executor) applyInsertAfterActivity(doc bson.D, op *ast.InsertAfterOp) error {
486482
idx, activities, containingFlow, err := findActivityIndex(doc, op.ActivityRef, op.AtPosition)
@@ -546,7 +542,10 @@ func (e *Executor) applyReplaceActivity(doc bson.D, op *ast.ReplaceActivityOp) e
546542
}
547543

548544
e.autoBindActivitiesInFlow(newActs)
549-
for _, act := range newActs { deduplicateNewActivityName(act, collectAllActivityNames(doc)) }
545+
existingNames := collectAllActivityNames(doc)
546+
for _, act := range newActs {
547+
deduplicateNewActivityName(act, existingNames)
548+
}
550549

551550
newBsonActs := make([]any, 0, len(newActs))
552551
for _, act := range newActs {
@@ -580,24 +579,7 @@ func (e *Executor) applyInsertOutcome(doc bson.D, op *ast.InsertOutcomeOp) error
580579

581580
// Build sub-flow if activities provided
582581
if len(op.Activities) > 0 {
583-
subActs := buildWorkflowActivities(op.Activities)
584-
e.autoBindActivitiesInFlow(subActs)
585-
existingNames := collectAllActivityNames(doc)
586-
for _, act := range subActs { deduplicateNewActivityName(act, existingNames) }
587-
var subActsBson bson.A
588-
subActsBson = append(subActsBson, int32(3))
589-
for _, act := range subActs {
590-
bsonDoc := mpr.SerializeWorkflowActivity(act)
591-
if bsonDoc != nil {
592-
subActsBson = append(subActsBson, bsonDoc)
593-
}
594-
}
595-
flowDoc := bson.D{
596-
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
597-
{Key: "$Type", Value: "Workflows$Flow"},
598-
{Key: "Activities", Value: subActsBson},
599-
}
600-
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: flowDoc})
582+
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: e.buildSubFlowBson(doc, op.Activities)})
601583
}
602584

603585
outcomeDoc = append(outcomeDoc,
@@ -661,24 +643,7 @@ func (e *Executor) applyInsertPath(doc bson.D, op *ast.InsertPathOp) error {
661643
}
662644

663645
if len(op.Activities) > 0 {
664-
subActs := buildWorkflowActivities(op.Activities)
665-
e.autoBindActivitiesInFlow(subActs)
666-
existingNames := collectAllActivityNames(doc)
667-
for _, act := range subActs { deduplicateNewActivityName(act, existingNames) }
668-
var subActsBson bson.A
669-
subActsBson = append(subActsBson, int32(3))
670-
for _, act := range subActs {
671-
bsonDoc := mpr.SerializeWorkflowActivity(act)
672-
if bsonDoc != nil {
673-
subActsBson = append(subActsBson, bsonDoc)
674-
}
675-
}
676-
flowDoc := bson.D{
677-
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
678-
{Key: "$Type", Value: "Workflows$Flow"},
679-
{Key: "Activities", Value: subActsBson},
680-
}
681-
pathDoc = append(pathDoc, bson.E{Key: "Flow", Value: flowDoc})
646+
pathDoc = append(pathDoc, bson.E{Key: "Flow", Value: e.buildSubFlowBson(doc, op.Activities)})
682647
}
683648

684649
pathDoc = append(pathDoc, bson.E{Key: "PersistentId", Value: mpr.IDToBsonBinary(mpr.GenerateID())})
@@ -760,24 +725,7 @@ func (e *Executor) applyInsertBranch(doc bson.D, op *ast.InsertBranchOp) error {
760725
}
761726

762727
if len(op.Activities) > 0 {
763-
subActs := buildWorkflowActivities(op.Activities)
764-
e.autoBindActivitiesInFlow(subActs)
765-
existingNames := collectAllActivityNames(doc)
766-
for _, act := range subActs { deduplicateNewActivityName(act, existingNames) }
767-
var subActsBson bson.A
768-
subActsBson = append(subActsBson, int32(3))
769-
for _, act := range subActs {
770-
bsonDoc := mpr.SerializeWorkflowActivity(act)
771-
if bsonDoc != nil {
772-
subActsBson = append(subActsBson, bsonDoc)
773-
}
774-
}
775-
flowDoc := bson.D{
776-
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
777-
{Key: "$Type", Value: "Workflows$Flow"},
778-
{Key: "Activities", Value: subActsBson},
779-
}
780-
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: flowDoc})
728+
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: e.buildSubFlowBson(doc, op.Activities)})
781729
}
782730

783731
outcomes := dGetArrayElements(dGet(actDoc, "Outcomes"))
@@ -868,24 +816,7 @@ func (e *Executor) applyInsertBoundaryEvent(doc bson.D, op *ast.InsertBoundaryEv
868816
}
869817

870818
if len(op.Activities) > 0 {
871-
subActs := buildWorkflowActivities(op.Activities)
872-
e.autoBindActivitiesInFlow(subActs)
873-
existingNames := collectAllActivityNames(doc)
874-
for _, act := range subActs { deduplicateNewActivityName(act, existingNames) }
875-
var subActsBson bson.A
876-
subActsBson = append(subActsBson, int32(3))
877-
for _, act := range subActs {
878-
bsonDoc := mpr.SerializeWorkflowActivity(act)
879-
if bsonDoc != nil {
880-
subActsBson = append(subActsBson, bsonDoc)
881-
}
882-
}
883-
flowDoc := bson.D{
884-
{Key: "$ID", Value: mpr.IDToBsonBinary(mpr.GenerateID())},
885-
{Key: "$Type", Value: "Workflows$Flow"},
886-
{Key: "Activities", Value: subActsBson},
887-
}
888-
eventDoc = append(eventDoc, bson.E{Key: "Flow", Value: flowDoc})
819+
eventDoc = append(eventDoc, bson.E{Key: "Flow", Value: e.buildSubFlowBson(doc, op.Activities)})
889820
}
890821

891822
eventDoc = append(eventDoc, bson.E{Key: "PersistentId", Value: mpr.IDToBsonBinary(mpr.GenerateID())})

‎sdk/workflows/workflow.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ type Flow struct {
6262
type WorkflowActivity interface {
6363
GetID() model.ID
6464
GetName() string
65+
SetName(string)
6566
GetCaption() string
6667
ActivityType() string
6768
}
@@ -84,6 +85,11 @@ func (a *BaseWorkflowActivity) GetName() string {
8485
return a.Name
8586
}
8687

88+
// SetName sets the activity's name.
89+
func (a *BaseWorkflowActivity) SetName(name string) {
90+
a.Name = name
91+
}
92+
8793
// GetCaption returns the activity's caption.
8894
func (a *BaseWorkflowActivity) GetCaption() string {
8995
return a.Caption

0 commit comments

Comments
 (0)