Skip to content

Commit ed97d97

Browse files
committed
fixup: address PR #267 review (backend abstraction + version helper dedup)
Addresses two structural issues from ako's review: 1. **sdk/mpr import in cmd_microflows_builder.go** — lookupMicroflowReturnType called mpr.ParseMicroflowBSON directly, violating the executor's backend abstraction contract. Added ParseMicroflowBSON(contents, unitID, containerID) to the MicroflowBackend interface, implemented on MprBackend (delegates to mpr.ParseMicroflowBSON) and Mock (Func-field with descriptive 'MockBackend.ParseMicroflowBSON not configured' default). lookupMicroflow ReturnType now goes through fb.backend.ParseMicroflowBSON. sdk/mpr import removed from the builder. 2. **newestVersionedPath duplication** — the four version-comparison helpers (newestVersionedPath, versionFromPath, parseVersionParts, compareVersion Parts) lived identically in cmd/mxcli/docker/download.go and mdl/executor/roundtrip_helpers_test.go. Exported the production helper as docker.NewestVersionedPath (with a full doc comment explaining the ranking); internal docker callers updated; the integration-test harness now imports docker and calls docker.NewestVersionedPath. All four copies removed from the test file.
1 parent 14141fc commit ed97d97

10 files changed

Lines changed: 41 additions & 86 deletions

File tree

cmd/mxcli/docker/check.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func ResolveMxForVersion(mxbuildPath, preferredVersion string) (string, error) {
162162
if exact := exactVersionedPath(matches, preferredVersion); exact != "" {
163163
return exact, nil
164164
}
165-
if newest := newestVersionedPath(matches); newest != "" {
165+
if newest := NewestVersionedPath(matches); newest != "" {
166166
return newest, nil
167167
}
168168
}

cmd/mxcli/docker/detect.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ func resolveMxBuild(explicitPath string, preferredVersion ...string) (string, er
116116
if exact := exactVersionedPath(matches, targetVersion); exact != "" {
117117
return exact, nil
118118
}
119-
if newest := newestVersionedPath(matches); newest != "" {
119+
if newest := NewestVersionedPath(matches); newest != "" {
120120
return newest, nil
121121
}
122122
}

cmd/mxcli/docker/download.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func anyCachedBinaryPath(names []string) string {
7676
found, _ := filepath.Glob(pattern)
7777
matches = append(matches, found...)
7878
}
79-
return newestVersionedPath(matches)
79+
return NewestVersionedPath(matches)
8080
}
8181

8282
func globVersionedMatches(patterns []string) []string {
@@ -98,10 +98,16 @@ func exactVersionedPath(paths []string, version string) string {
9898
exact = append(exact, path)
9999
}
100100
}
101-
return newestVersionedPath(exact)
101+
return NewestVersionedPath(exact)
102102
}
103103

104-
func newestVersionedPath(paths []string) string {
104+
// NewestVersionedPath selects the lexicographically-highest "versioned"
105+
// directory from paths, where "versioned" means the grandparent directory
106+
// name parses as a dotted numeric version (`11.9.0`). Paths whose version
107+
// cannot be parsed compare as a pure lexicographic fallback, but always rank
108+
// below any parseable version. Used by both the mx-binary resolver and the
109+
// integration test harness.
110+
func NewestVersionedPath(paths []string) string {
105111
var best string
106112
var bestVersion []int
107113
bestValid := false

mdl/backend/microflow.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ type MicroflowBackend interface {
2020
// map. Used by diff-local and other callers that have raw map data.
2121
ParseMicroflowFromRaw(raw map[string]any, unitID, containerID model.ID) *microflows.Microflow
2222

23+
// ParseMicroflowBSON parses raw microflow BSON bytes into a Microflow.
24+
// Used by the executor to inspect microflows it has not necessarily
25+
// loaded via ListMicroflows (e.g. to resolve a CALL MICROFLOW's return
26+
// type from its raw unit).
27+
ParseMicroflowBSON(contents []byte, unitID, containerID model.ID) (*microflows.Microflow, error)
28+
2329
ListNanoflows() ([]*microflows.Nanoflow, error)
2430
GetNanoflow(id model.ID) (*microflows.Nanoflow, error)
2531
CreateNanoflow(nf *microflows.Nanoflow) error

mdl/backend/mock/backend.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ type MockBackend struct {
8282
DeleteMicroflowFunc func(id model.ID) error
8383
MoveMicroflowFunc func(mf *microflows.Microflow) error
8484
ParseMicroflowFromRawFunc func(raw map[string]any, unitID, containerID model.ID) *microflows.Microflow
85+
ParseMicroflowBSONFunc func(contents []byte, unitID, containerID model.ID) (*microflows.Microflow, error)
8586
ListNanoflowsFunc func() ([]*microflows.Nanoflow, error)
8687
GetNanoflowFunc func(id model.ID) (*microflows.Nanoflow, error)
8788
CreateNanoflowFunc func(nf *microflows.Nanoflow) error

mdl/backend/mock/mock_microflow.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
package mock
44

55
import (
6+
"fmt"
7+
68
"github.com/mendixlabs/mxcli/model"
79
"github.com/mendixlabs/mxcli/sdk/microflows"
810
)
@@ -56,6 +58,13 @@ func (m *MockBackend) ParseMicroflowFromRaw(raw map[string]any, unitID, containe
5658
panic("mock ParseMicroflowFromRaw called but ParseMicroflowFromRawFunc is not set")
5759
}
5860

61+
func (m *MockBackend) ParseMicroflowBSON(contents []byte, unitID, containerID model.ID) (*microflows.Microflow, error) {
62+
if m.ParseMicroflowBSONFunc != nil {
63+
return m.ParseMicroflowBSONFunc(contents, unitID, containerID)
64+
}
65+
return nil, fmt.Errorf("MockBackend.ParseMicroflowBSON not configured")
66+
}
67+
5968
func (m *MockBackend) ListNanoflows() ([]*microflows.Nanoflow, error) {
6069
if m.ListNanoflowsFunc != nil {
6170
return m.ListNanoflowsFunc()

mdl/backend/mpr/backend.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,9 @@ func (b *MprBackend) ListNanoflows() ([]*microflows.Nanoflow, error) {
231231
func (b *MprBackend) ParseMicroflowFromRaw(raw map[string]any, unitID, containerID model.ID) *microflows.Microflow {
232232
return mpr.ParseMicroflowFromRaw(raw, unitID, containerID)
233233
}
234+
func (b *MprBackend) ParseMicroflowBSON(contents []byte, unitID, containerID model.ID) (*microflows.Microflow, error) {
235+
return mpr.ParseMicroflowBSON(contents, unitID, containerID)
236+
}
234237
func (b *MprBackend) GetNanoflow(id model.ID) (*microflows.Nanoflow, error) {
235238
return b.reader.GetNanoflow(id)
236239
}

mdl/executor/cmd_microflows_builder.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"github.com/mendixlabs/mxcli/mdl/types"
1313
"github.com/mendixlabs/mxcli/model"
1414
"github.com/mendixlabs/mxcli/sdk/microflows"
15-
"github.com/mendixlabs/mxcli/sdk/mpr"
1615
)
1716

1817
// flowBuilder helps construct the flow graph from AST statements.
@@ -131,7 +130,7 @@ func (fb *flowBuilder) lookupMicroflowReturnType(qualifiedName string) microflow
131130
}
132131

133132
if rawUnit, err := fb.backend.GetRawUnitByName("microflow", qualifiedName); err == nil && rawUnit != nil && len(rawUnit.Contents) > 0 {
134-
if mf, err := mpr.ParseMicroflowBSON(rawUnit.Contents, model.ID(rawUnit.ID), ""); err == nil && mf != nil {
133+
if mf, err := fb.backend.ParseMicroflowBSON(rawUnit.Contents, model.ID(rawUnit.ID), ""); err == nil && mf != nil {
135134
return mf.ReturnType
136135
}
137136
}

mdl/executor/roundtrip_helpers_mx_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"os"
99
"path/filepath"
1010
"testing"
11+
12+
"github.com/mendixlabs/mxcli/cmd/mxcli/docker"
1113
)
1214

1315
func TestNewestVersionedPath_PicksNewestNumericVersion(t *testing.T) {
@@ -19,10 +21,10 @@ func TestNewestVersionedPath_PicksNewestNumericVersion(t *testing.T) {
1921
"/tmp/.mxcli/mxbuild/9.24.40.80973/modeler/mx",
2022
}
2123

22-
got := newestVersionedPath(paths)
24+
got := docker.NewestVersionedPath(paths)
2325
want := "/tmp/.mxcli/mxbuild/11.9.0/modeler/mx"
2426
if got != want {
25-
t.Fatalf("newestVersionedPath() = %q, want %q", got, want)
27+
t.Fatalf("docker.NewestVersionedPath() = %q, want %q", got, want)
2628
}
2729
}
2830

mdl/executor/roundtrip_helpers_test.go

Lines changed: 6 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,10 @@ import (
1818
"os"
1919
"os/exec"
2020
"path/filepath"
21-
"strconv"
2221
"strings"
2322
"testing"
2423

24+
"github.com/mendixlabs/mxcli/cmd/mxcli/docker"
2525
"github.com/mendixlabs/mxcli/mdl/ast"
2626
"github.com/mendixlabs/mxcli/mdl/backend"
2727
mprbackend "github.com/mendixlabs/mxcli/mdl/backend/mpr"
@@ -160,88 +160,17 @@ func findMxBinary() string {
160160
if home, err := os.UserHomeDir(); err == nil {
161161
pattern := filepath.Join(home, ".mxcli", "mxbuild", "*", "modeler", "mx")
162162
if matches, _ := filepath.Glob(pattern); len(matches) > 0 {
163-
return newestVersionedPath(matches)
163+
return docker.NewestVersionedPath(matches)
164164
}
165165
}
166166

167167
return ""
168168
}
169169

170-
func newestVersionedPath(paths []string) string {
171-
var best string
172-
var bestVersion []int
173-
bestValid := false
174-
175-
for _, path := range paths {
176-
versionParts, ok := parseVersionParts(versionFromPath(path))
177-
switch {
178-
case best == "":
179-
best = path
180-
bestVersion = versionParts
181-
bestValid = ok
182-
case ok && !bestValid:
183-
best = path
184-
bestVersion = versionParts
185-
bestValid = true
186-
case ok && bestValid:
187-
if cmp := compareVersionParts(versionParts, bestVersion); cmp > 0 || (cmp == 0 && path > best) {
188-
best = path
189-
bestVersion = versionParts
190-
}
191-
case !ok && !bestValid && path > best:
192-
best = path
193-
}
194-
}
195-
196-
return best
197-
}
198-
199-
func versionFromPath(path string) string {
200-
versionDir := filepath.Dir(filepath.Dir(path))
201-
return filepath.Base(versionDir)
202-
}
203-
204-
func parseVersionParts(version string) ([]int, bool) {
205-
if version == "" {
206-
return nil, false
207-
}
208-
parts := strings.Split(version, ".")
209-
values := make([]int, 0, len(parts))
210-
for _, part := range parts {
211-
if part == "" {
212-
return nil, false
213-
}
214-
value, err := strconv.Atoi(part)
215-
if err != nil {
216-
return nil, false
217-
}
218-
values = append(values, value)
219-
}
220-
return values, true
221-
}
222-
223-
func compareVersionParts(left, right []int) int {
224-
maxLen := len(left)
225-
if len(right) > maxLen {
226-
maxLen = len(right)
227-
}
228-
for i := 0; i < maxLen; i++ {
229-
var l, r int
230-
if i < len(left) {
231-
l = left[i]
232-
}
233-
if i < len(right) {
234-
r = right[i]
235-
}
236-
switch {
237-
case l < r:
238-
return -1
239-
case l > r:
240-
return 1
241-
}
242-
}
243-
return 0
244-
}
170+
// newestVersionedPath / versionFromPath / parseVersionParts / compareVersionParts
171+
// used to be duplicated here. They now live as exported helpers in
172+
// cmd/mxcli/docker (docker.NewestVersionedPath). The integration-test harness
173+
// call-site was adjusted to use the exported helper instead.
245174

246175
// copyFile copies a single file from src to dst.
247176
func copyFile(src, dst string) error {

0 commit comments

Comments
 (0)