Skip to content

Commit ebb1c52

Browse files
committed
fix(reset): preserve path escaping in cycle detection
checkForCycle called path.String(), which un-escapes dots inside segment names. Service names containing "." (e.g. "test.other") were then misidentified as sub-paths of services with shorter prefix names (e.g. "test"), triggering a spurious "cycle detected" error. Compare on the raw form and use Path.Parts() for the services check so segment boundaries stay intact. Only render via .String() in the error message. Fixes docker/compose#13863 Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
1 parent faa67b2 commit ebb1c52

2 files changed

Lines changed: 30 additions & 13 deletions

File tree

loader/reset.go

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ type nodeCache struct {
4040
type ResetProcessor struct {
4141
target any
4242
paths []tree.Path
43-
visitedNodes map[*yaml.Node][]string
43+
visitedNodes map[*yaml.Node][]tree.Path
4444
resolvedNodes map[*yaml.Node]nodeCache
4545
visitCount int
4646
// maxNodeVisits is the per-document cap; when zero, defaultMaxNodeVisits is used.
@@ -49,7 +49,7 @@ type ResetProcessor struct {
4949

5050
// UnmarshalYAML implement yaml.Unmarshaler
5151
func (p *ResetProcessor) UnmarshalYAML(value *yaml.Node) error {
52-
p.visitedNodes = make(map[*yaml.Node][]string)
52+
p.visitedNodes = make(map[*yaml.Node][]tree.Path)
5353
p.resolvedNodes = make(map[*yaml.Node]nodeCache)
5454
p.visitCount = 0
5555
defer func() {
@@ -283,36 +283,41 @@ func (p *ResetProcessor) applyNullOverrides(target any, path tree.Path) error {
283283

284284
func (p *ResetProcessor) checkForCycle(node *yaml.Node, path tree.Path) error {
285285
paths := p.visitedNodes[node]
286-
pathStr := path.String()
287286

288287
for _, prevPath := range paths {
289288
// If we're visiting the exact same path, it's not a cycle
290-
if pathStr == prevPath {
289+
if path == prevPath {
291290
continue
292291
}
293292

293+
// Compare on the raw form so dots inside escaped segment names (e.g.
294+
// service names containing ".") aren't conflated with path separators.
295+
pathStr := string(path)
296+
prevStr := string(prevPath)
297+
294298
// If either path is using a merge key, it's legitimate YAML merging
295-
if strings.Contains(prevPath, "<<") || strings.Contains(pathStr, "<<") {
299+
if strings.Contains(prevStr, "<<") || strings.Contains(pathStr, "<<") {
296300
continue
297301
}
298302

299303
// Only consider it a cycle if one path is contained within the other
300304
// and they're not in different service definitions
301-
if (strings.HasPrefix(pathStr, prevPath+".") ||
302-
strings.HasPrefix(prevPath, pathStr+".")) &&
303-
!areInDifferentServices(pathStr, prevPath) {
304-
return fmt.Errorf("cycle detected: node at path %s references node at path %s", pathStr, prevPath)
305+
if (strings.HasPrefix(pathStr, prevStr+".") ||
306+
strings.HasPrefix(prevStr, pathStr+".")) &&
307+
!areInDifferentServices(path, prevPath) {
308+
return fmt.Errorf("cycle detected: node at path %s references node at path %s",
309+
path.String(), prevPath.String())
305310
}
306311
}
307312

308-
p.visitedNodes[node] = append(paths, pathStr)
313+
p.visitedNodes[node] = append(paths, path)
309314
return nil
310315
}
311316

312317
// areInDifferentServices checks if two paths are in different service definitions
313-
func areInDifferentServices(path1, path2 string) bool {
314-
parts1 := strings.Split(path1, ".")
315-
parts2 := strings.Split(path2, ".")
318+
func areInDifferentServices(path1, path2 tree.Path) bool {
319+
parts1 := path1.Parts()
320+
parts2 := path2.Parts()
316321
for i := 0; i < len(parts1) && i < len(parts2); i++ {
317322
if parts1[i] == "services" && i+1 < len(parts1) &&
318323
parts2[i] == "services" && i+1 < len(parts2) {

loader/reset_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,18 @@ services:
143143
backend-worker:
144144
<<: *common
145145
image: alpine:latest
146+
`,
147+
expectError: false,
148+
},
149+
{
150+
name: "dotted_service_names_prefix_no_cycle",
151+
config: `
152+
name: test
153+
services:
154+
test.unit: &shared
155+
image: alpine
156+
test: *shared
157+
test.other: *shared
146158
`,
147159
expectError: false,
148160
},

0 commit comments

Comments
 (0)