Skip to content

Commit 29698c1

Browse files
retranretran
committed
fix: address review feedback — error logging, auto-bind migration, output threading, disconnect ctx cleanup, cache invalidation, theme registry persistence, parallel ctx fields, settings persistence, structured logging, nil-safety guards, project-scoped cache reset on connect
1 parent 437c352 commit 29698c1

File tree

12 files changed

+226
-65
lines changed

12 files changed

+226
-65
lines changed

‎mdl/catalog/builder.go‎

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,72 @@ import (
1010

1111
"github.com/mendixlabs/mxcli/model"
1212
"github.com/mendixlabs/mxcli/sdk/domainmodel"
13+
"github.com/mendixlabs/mxcli/sdk/javaactions"
1314
"github.com/mendixlabs/mxcli/sdk/microflows"
1415
"github.com/mendixlabs/mxcli/sdk/mpr"
1516
"github.com/mendixlabs/mxcli/sdk/pages"
17+
"github.com/mendixlabs/mxcli/sdk/security"
1618
"github.com/mendixlabs/mxcli/sdk/workflows"
1719
)
1820

21+
// CatalogReader defines the read-only backend surface used by the catalog builder.
22+
// Both *mpr.Reader and backend.FullBackend satisfy this interface.
23+
type CatalogReader interface {
24+
// Infrastructure
25+
GetRawUnit(id model.ID) (map[string]any, error)
26+
ListRawUnitsByType(typePrefix string) ([]*mpr.RawUnit, error)
27+
ListUnits() ([]*mpr.UnitInfo, error)
28+
ListFolders() ([]*mpr.FolderInfo, error)
29+
30+
// Modules
31+
ListModules() ([]*model.Module, error)
32+
33+
// Settings & security
34+
GetProjectSettings() (*model.ProjectSettings, error)
35+
GetProjectSecurity() (*security.ProjectSecurity, error)
36+
GetNavigation() (*mpr.NavigationDocument, error)
37+
38+
// Domain models & enumerations
39+
ListDomainModels() ([]*domainmodel.DomainModel, error)
40+
ListEnumerations() ([]*model.Enumeration, error)
41+
ListConstants() ([]*model.Constant, error)
42+
43+
// Microflows & nanoflows
44+
ListMicroflows() ([]*microflows.Microflow, error)
45+
ListNanoflows() ([]*microflows.Nanoflow, error)
46+
47+
// Pages, layouts & snippets
48+
ListPages() ([]*pages.Page, error)
49+
ListLayouts() ([]*pages.Layout, error)
50+
ListSnippets() ([]*pages.Snippet, error)
51+
52+
// Workflows
53+
ListWorkflows() ([]*workflows.Workflow, error)
54+
55+
// Java actions
56+
ListJavaActionsFull() ([]*javaactions.JavaAction, error)
57+
58+
// Services
59+
ListConsumedODataServices() ([]*model.ConsumedODataService, error)
60+
ListPublishedODataServices() ([]*model.PublishedODataService, error)
61+
ListConsumedRestServices() ([]*model.ConsumedRestService, error)
62+
ListPublishedRestServices() ([]*model.PublishedRestService, error)
63+
ListBusinessEventServices() ([]*model.BusinessEventService, error)
64+
ListDatabaseConnections() ([]*model.DatabaseConnection, error)
65+
66+
// Mappings & JSON structures
67+
ListImportMappings() ([]*model.ImportMapping, error)
68+
ListExportMappings() ([]*model.ExportMapping, error)
69+
ListJsonStructures() ([]*mpr.JsonStructure, error)
70+
}
71+
1972
// DescribeFunc generates MDL source for a given object type and qualified name.
2073
type DescribeFunc func(objectType string, qualifiedName string) (string, error)
2174

2275
// Builder populates catalog tables from MPR data.
2376
type Builder struct {
2477
catalog *Catalog
25-
reader *mpr.Reader
78+
reader CatalogReader
2679
snapshot *Snapshot
2780
progress ProgressFunc
2881
hierarchy *hierarchy
@@ -148,7 +201,7 @@ func (h *hierarchy) buildFolderPath(containerID model.ID) string {
148201
}
149202

150203
// NewBuilder creates a new catalog builder.
151-
func NewBuilder(catalog *Catalog, reader *mpr.Reader) *Builder {
204+
func NewBuilder(catalog *Catalog, reader CatalogReader) *Builder {
152205
return &Builder{
153206
catalog: catalog,
154207
reader: reader,

‎mdl/executor/cmd_alter_workflow.go‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package executor
44

55
import (
66
"fmt"
7+
"io"
78
"strings"
89

910
"go.mongodb.org/mongo-driver/bson"
@@ -21,7 +22,6 @@ const bsonArrayMarker = int32(3)
2122

2223
// execAlterWorkflow handles ALTER WORKFLOW Module.Name { operations }.
2324
func execAlterWorkflow(ctx *ExecContext, s *ast.AlterWorkflowStmt) error {
24-
e := ctx.executor
2525
if !ctx.Connected() {
2626
return mdlerrors.NewNotConnected()
2727
}
@@ -82,47 +82,47 @@ func execAlterWorkflow(ctx *ExecContext, s *ast.AlterWorkflowStmt) error {
8282
return mdlerrors.NewBackend("SET ACTIVITY", err)
8383
}
8484
case *ast.InsertAfterOp:
85-
if err := applyInsertAfterActivity(e, rawData, o); err != nil {
85+
if err := applyInsertAfterActivity(ctx, rawData, o); err != nil {
8686
return mdlerrors.NewBackend("INSERT AFTER", err)
8787
}
8888
case *ast.DropActivityOp:
8989
if err := applyDropActivity(rawData, o); err != nil {
9090
return mdlerrors.NewBackend("DROP ACTIVITY", err)
9191
}
9292
case *ast.ReplaceActivityOp:
93-
if err := applyReplaceActivity(e, rawData, o); err != nil {
93+
if err := applyReplaceActivity(ctx, rawData, o); err != nil {
9494
return mdlerrors.NewBackend("REPLACE ACTIVITY", err)
9595
}
9696
case *ast.InsertOutcomeOp:
97-
if err := applyInsertOutcome(e, rawData, o); err != nil {
97+
if err := applyInsertOutcome(ctx, rawData, o); err != nil {
9898
return mdlerrors.NewBackend("INSERT OUTCOME", err)
9999
}
100100
case *ast.DropOutcomeOp:
101101
if err := applyDropOutcome(rawData, o); err != nil {
102102
return mdlerrors.NewBackend("DROP OUTCOME", err)
103103
}
104104
case *ast.InsertPathOp:
105-
if err := applyInsertPath(e, rawData, o); err != nil {
105+
if err := applyInsertPath(ctx, rawData, o); err != nil {
106106
return mdlerrors.NewBackend("INSERT PATH", err)
107107
}
108108
case *ast.DropPathOp:
109109
if err := applyDropPath(rawData, o); err != nil {
110110
return mdlerrors.NewBackend("DROP PATH", err)
111111
}
112112
case *ast.InsertBranchOp:
113-
if err := applyInsertBranch(e, rawData, o); err != nil {
113+
if err := applyInsertBranch(ctx, rawData, o); err != nil {
114114
return mdlerrors.NewBackend("INSERT BRANCH", err)
115115
}
116116
case *ast.DropBranchOp:
117117
if err := applyDropBranch(rawData, o); err != nil {
118118
return mdlerrors.NewBackend("DROP BRANCH", err)
119119
}
120120
case *ast.InsertBoundaryEventOp:
121-
if err := applyInsertBoundaryEvent(e, rawData, o); err != nil {
121+
if err := applyInsertBoundaryEvent(ctx, rawData, o); err != nil {
122122
return mdlerrors.NewBackend("INSERT BOUNDARY EVENT", err)
123123
}
124124
case *ast.DropBoundaryEventOp:
125-
if err := applyDropBoundaryEvent(rawData, o); err != nil {
125+
if err := applyDropBoundaryEvent(ctx.Output, rawData, o); err != nil {
126126
return mdlerrors.NewBackend("DROP BOUNDARY EVENT", err)
127127
}
128128
default:
@@ -484,9 +484,9 @@ func deduplicateNewActivityName(act workflows.WorkflowActivity, existingNames ma
484484

485485
// buildSubFlowBson builds a Workflows$Flow BSON document from AST activity nodes,
486486
// with auto-binding and name deduplication against existing workflow activities.
487-
func buildSubFlowBson(e *Executor, doc bson.D, activities []ast.WorkflowActivityNode) bson.D {
487+
func buildSubFlowBson(ctx *ExecContext, doc bson.D, activities []ast.WorkflowActivityNode) bson.D {
488488
subActs := buildWorkflowActivities(activities)
489-
autoBindActivitiesInFlow(e, subActs)
489+
autoBindActivitiesInFlow(ctx, subActs)
490490
existingNames := collectAllActivityNames(doc)
491491
for _, act := range subActs {
492492
deduplicateNewActivityName(act, existingNames)
@@ -507,7 +507,7 @@ func buildSubFlowBson(e *Executor, doc bson.D, activities []ast.WorkflowActivity
507507
}
508508

509509
// applyInsertAfterActivity inserts a new activity after a named activity.
510-
func applyInsertAfterActivity(e *Executor, doc bson.D, op *ast.InsertAfterOp) error {
510+
func applyInsertAfterActivity(ctx *ExecContext, doc bson.D, op *ast.InsertAfterOp) error {
511511
idx, activities, containingFlow, err := findActivityIndex(doc, op.ActivityRef, op.AtPosition)
512512
if err != nil {
513513
return err
@@ -519,7 +519,7 @@ func applyInsertAfterActivity(e *Executor, doc bson.D, op *ast.InsertAfterOp) er
519519
}
520520

521521
// Auto-bind parameters and deduplicate against existing workflow names
522-
autoBindActivitiesInFlow(e, newActs)
522+
autoBindActivitiesInFlow(ctx, newActs)
523523
existingNames := collectAllActivityNames(doc)
524524
for _, act := range newActs {
525525
deduplicateNewActivityName(act, existingNames)
@@ -559,7 +559,7 @@ func applyDropActivity(doc bson.D, op *ast.DropActivityOp) error {
559559
}
560560

561561
// applyReplaceActivity replaces an activity in place.
562-
func applyReplaceActivity(e *Executor, doc bson.D, op *ast.ReplaceActivityOp) error {
562+
func applyReplaceActivity(ctx *ExecContext, doc bson.D, op *ast.ReplaceActivityOp) error {
563563
idx, activities, containingFlow, err := findActivityIndex(doc, op.ActivityRef, op.AtPosition)
564564
if err != nil {
565565
return err
@@ -570,7 +570,7 @@ func applyReplaceActivity(e *Executor, doc bson.D, op *ast.ReplaceActivityOp) er
570570
return mdlerrors.NewValidation("failed to build replacement activity")
571571
}
572572

573-
autoBindActivitiesInFlow(e, newActs)
573+
autoBindActivitiesInFlow(ctx, newActs)
574574
existingNames := collectAllActivityNames(doc)
575575
for _, act := range newActs {
576576
deduplicateNewActivityName(act, existingNames)
@@ -594,7 +594,7 @@ func applyReplaceActivity(e *Executor, doc bson.D, op *ast.ReplaceActivityOp) er
594594
}
595595

596596
// applyInsertOutcome adds a new outcome to a user task.
597-
func applyInsertOutcome(e *Executor, doc bson.D, op *ast.InsertOutcomeOp) error {
597+
func applyInsertOutcome(ctx *ExecContext, doc bson.D, op *ast.InsertOutcomeOp) error {
598598
actDoc, err := findActivityByCaption(doc, op.ActivityRef, op.AtPosition)
599599
if err != nil {
600600
return err
@@ -608,7 +608,7 @@ func applyInsertOutcome(e *Executor, doc bson.D, op *ast.InsertOutcomeOp) error
608608

609609
// Build sub-flow if activities provided
610610
if len(op.Activities) > 0 {
611-
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(e, doc, op.Activities)})
611+
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(ctx, doc, op.Activities)})
612612
}
613613

614614
outcomeDoc = append(outcomeDoc,
@@ -660,7 +660,7 @@ func applyDropOutcome(doc bson.D, op *ast.DropOutcomeOp) error {
660660
}
661661

662662
// applyInsertPath adds a new path to a parallel split.
663-
func applyInsertPath(e *Executor, doc bson.D, op *ast.InsertPathOp) error {
663+
func applyInsertPath(ctx *ExecContext, doc bson.D, op *ast.InsertPathOp) error {
664664
actDoc, err := findActivityByCaption(doc, op.ActivityRef, op.AtPosition)
665665
if err != nil {
666666
return err
@@ -672,7 +672,7 @@ func applyInsertPath(e *Executor, doc bson.D, op *ast.InsertPathOp) error {
672672
}
673673

674674
if len(op.Activities) > 0 {
675-
pathDoc = append(pathDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(e, doc, op.Activities)})
675+
pathDoc = append(pathDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(ctx, doc, op.Activities)})
676676
}
677677

678678
pathDoc = append(pathDoc, bson.E{Key: "PersistentId", Value: mpr.IDToBsonBinary(mpr.GenerateID())})
@@ -719,7 +719,7 @@ func applyDropPath(doc bson.D, op *ast.DropPathOp) error {
719719
}
720720

721721
// applyInsertBranch adds a new branch to a decision.
722-
func applyInsertBranch(e *Executor, doc bson.D, op *ast.InsertBranchOp) error {
722+
func applyInsertBranch(ctx *ExecContext, doc bson.D, op *ast.InsertBranchOp) error {
723723
actDoc, err := findActivityByCaption(doc, op.ActivityRef, op.AtPosition)
724724
if err != nil {
725725
return err
@@ -754,7 +754,7 @@ func applyInsertBranch(e *Executor, doc bson.D, op *ast.InsertBranchOp) error {
754754
}
755755

756756
if len(op.Activities) > 0 {
757-
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(e, doc, op.Activities)})
757+
outcomeDoc = append(outcomeDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(ctx, doc, op.Activities)})
758758
}
759759

760760
outcomes := dGetArrayElements(dGet(actDoc, "Outcomes"))
@@ -820,7 +820,7 @@ func applyDropBranch(doc bson.D, op *ast.DropBranchOp) error {
820820
}
821821

822822
// applyInsertBoundaryEvent adds a boundary event to an activity.
823-
func applyInsertBoundaryEvent(e *Executor, doc bson.D, op *ast.InsertBoundaryEventOp) error {
823+
func applyInsertBoundaryEvent(ctx *ExecContext, doc bson.D, op *ast.InsertBoundaryEventOp) error {
824824
actDoc, err := findActivityByCaption(doc, op.ActivityRef, op.AtPosition)
825825
if err != nil {
826826
return err
@@ -845,7 +845,7 @@ func applyInsertBoundaryEvent(e *Executor, doc bson.D, op *ast.InsertBoundaryEve
845845
}
846846

847847
if len(op.Activities) > 0 {
848-
eventDoc = append(eventDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(e, doc, op.Activities)})
848+
eventDoc = append(eventDoc, bson.E{Key: "Flow", Value: buildSubFlowBson(ctx, doc, op.Activities)})
849849
}
850850

851851
eventDoc = append(eventDoc, bson.E{Key: "PersistentId", Value: mpr.IDToBsonBinary(mpr.GenerateID())})
@@ -865,7 +865,7 @@ func applyInsertBoundaryEvent(e *Executor, doc bson.D, op *ast.InsertBoundaryEve
865865
//
866866
// Limitation: this always removes events[0]. There is currently no syntax to
867867
// target a specific boundary event by name or type when multiple exist.
868-
func applyDropBoundaryEvent(doc bson.D, op *ast.DropBoundaryEventOp) error {
868+
func applyDropBoundaryEvent(w io.Writer, doc bson.D, op *ast.DropBoundaryEventOp) error {
869869
actDoc, err := findActivityByCaption(doc, op.ActivityRef, op.AtPosition)
870870
if err != nil {
871871
return err
@@ -877,7 +877,7 @@ func applyDropBoundaryEvent(doc bson.D, op *ast.DropBoundaryEventOp) error {
877877
}
878878

879879
if len(events) > 1 {
880-
fmt.Printf("warning: activity %q has %d boundary events; dropping the first one\n", op.ActivityRef, len(events))
880+
fmt.Fprintf(w, "warning: activity %q has %d boundary events; dropping the first one\n", op.ActivityRef, len(events))
881881
}
882882

883883
// Drop the first boundary event

‎mdl/executor/cmd_alter_workflow_test.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package executor
44

55
import (
6+
"io"
67
"strings"
78
"testing"
89

@@ -319,7 +320,7 @@ func TestDropBoundaryEvent_Single(t *testing.T) {
319320
doc := makeWorkflowDoc(act)
320321

321322
op := &ast.DropBoundaryEventOp{ActivityRef: "Review"}
322-
if err := applyDropBoundaryEvent(doc, op); err != nil {
323+
if err := applyDropBoundaryEvent(io.Discard, doc, op); err != nil {
323324
t.Fatalf("DROP BOUNDARY EVENT failed: %v", err)
324325
}
325326

@@ -337,7 +338,7 @@ func TestDropBoundaryEvent_Multiple_DropsFirst(t *testing.T) {
337338
doc := makeWorkflowDoc(act)
338339

339340
op := &ast.DropBoundaryEventOp{ActivityRef: "Review"}
340-
if err := applyDropBoundaryEvent(doc, op); err != nil {
341+
if err := applyDropBoundaryEvent(io.Discard, doc, op); err != nil {
341342
t.Fatalf("DROP BOUNDARY EVENT failed: %v", err)
342343
}
343344

@@ -358,7 +359,7 @@ func TestDropBoundaryEvent_NoEvents(t *testing.T) {
358359
doc := makeWorkflowDoc(act)
359360

360361
op := &ast.DropBoundaryEventOp{ActivityRef: "Review"}
361-
err := applyDropBoundaryEvent(doc, op)
362+
err := applyDropBoundaryEvent(io.Discard, doc, op)
362363
if err == nil {
363364
t.Fatal("Expected error when dropping from activity with no boundary events")
364365
}

‎mdl/executor/cmd_catalog.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,11 @@ func ensureCatalog(ctx *ExecContext, full bool) error {
211211
}
212212
}
213213

214+
// Guard against building without a connection.
215+
if !ctx.Connected() {
216+
return mdlerrors.NewNotConnected()
217+
}
218+
214219
// Build fresh catalog
215220
return buildCatalog(ctx, full)
216221
}
@@ -347,7 +352,7 @@ func buildCatalog(ctx *ExecContext, full bool, source ...bool) error {
347352
cat.SetProject("default", "Current Project", version)
348353

349354
// Build catalog
350-
builder := catalog.NewBuilder(cat, e.reader)
355+
builder := catalog.NewBuilder(cat, ctx.Backend)
351356
builder.SetFullMode(full)
352357
if isSource {
353358
builder.SetSourceMode(true)
@@ -684,17 +689,22 @@ func captureDescribeParallel(ctx *ExecContext, objectType string, qualifiedName
684689
}
685690
qn := ast.QualifiedName{Module: parts[0], Name: parts[1]}
686691

687-
// Create a goroutine-local executor: shared reader + cache, own output buffer.
688-
// TODO: Replace with ExecContext.Fork() when MR 3 makes ExecContext self-contained.
692+
// Create a goroutine-local context: shared backend + cache, own output buffer.
689693
var buf bytes.Buffer
690694
local := &Executor{
691695
reader: ctx.executor.reader,
692696
output: &buf,
693697
cache: ctx.Cache,
694698
}
695699
localCtx := &ExecContext{
700+
Context: ctx.Context,
696701
Output: &buf,
702+
Format: ctx.Format,
703+
Quiet: ctx.Quiet,
704+
Logger: ctx.Logger,
705+
Backend: ctx.Backend,
697706
Cache: ctx.Cache,
707+
MprPath: ctx.MprPath,
698708
executor: local,
699709
}
700710

‎mdl/executor/cmd_javaactions.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ func execCreateJavaAction(ctx *ExecContext, s *ast.CreateJavaActionStmt) error {
422422
}
423423

424424
// Clear cache
425-
ctx.Cache = nil
425+
ctx.InvalidateCache()
426426

427427
fmt.Fprintf(ctx.Output, "Created java action: %s.%s\n", s.Name.Module, s.Name.Name)
428428
return nil

‎mdl/executor/cmd_misc.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ func execRefresh(ctx *ExecContext) error {
3838

3939
// execSet handles SET statements.
4040
func execSet(ctx *ExecContext, s *ast.SetStmt) error {
41+
if ctx.Settings == nil {
42+
ctx.Settings = make(map[string]any)
43+
// Persist back to Executor so subsequent statements see the map.
44+
if ctx.executor != nil {
45+
ctx.executor.settings = ctx.Settings
46+
}
47+
}
4148
ctx.Settings[s.Key] = s.Value
4249
fmt.Fprintf(ctx.Output, "Set %s = %v\n", s.Key, s.Value)
4350
return nil

0 commit comments

Comments
 (0)