diff --git a/reference.go b/reference.go index 55f84c2..bd916d1 100644 --- a/reference.go +++ b/reference.go @@ -11,11 +11,7 @@ import ( "github.com/lukasjarosch/skipper/data" ) -// TODO: handle reference-to-reference -// TODO: PathReferences / KeyReferences -// TODO: handle cyclic references -// TODO: handle multiple references per path -// TODO: handle reference-to-reference with multiple references on the same path +// TODO: PathReferences which allow yaml keys to be references as well var ( // ReferenceRegex defines the strings which are valid references @@ -24,6 +20,7 @@ var ( ErrUndefinedReferenceTarget = fmt.Errorf("undefined reference target path") ErrReferenceSourceIsNil = fmt.Errorf("reference source is nil") + ErrReferenceCycle = fmt.Errorf("reference cycles are not allowed") ) // Reference is a reference to a value with a different path. @@ -113,7 +110,7 @@ func ResolveReferences(references []Reference, resolveSource ReferenceSourceGett return ref.TargetPath.String() } - g := graph.New(referenceNodeHash, graph.Acyclic(), graph.Directed()) + g := graph.New(referenceNodeHash, graph.Acyclic(), graph.Directed(), graph.PreventCycles()) // Register all references as nodes var nodes []referenceVertex @@ -151,6 +148,9 @@ func ResolveReferences(references []Reference, resolveSource ReferenceSourceGett err = g.AddEdge(refNode.TargetPath.String(), n.TargetPath.String()) if err != nil { + if errors.Is(err, graph.ErrEdgeCreatesCycle) { + return nil, fmt.Errorf("reference %s -> %s would introduce cycle: %w", refNode.Name(), refDep.Name(), ErrReferenceCycle) + } return nil, err } } diff --git a/reference_test.go b/reference_test.go index b24a3e8..eef15c5 100644 --- a/reference_test.go +++ b/reference_test.go @@ -150,7 +150,7 @@ func TestResolveReferencesChained(t *testing.T) { expected := []Reference{ { Path: data.NewPath("chained.gotcha"), - TargetPath: data.NewPath("chained.john.name"), + TargetPath: data.NewPath("chained.john.first_name"), }, { Path: data.NewPath("chained.name_placeholder"), @@ -168,3 +168,61 @@ func TestResolveReferencesChained(t *testing.T) { assert.Equal(t, resolved, expected) } + +func TestResolveReferencesCycle(t *testing.T) { + class, err := NewClass("testdata/references/local/cycle.yaml", codec.NewYamlCodec(), data.NewPathFromOsPath("cycle")) + assert.NoError(t, err) + + references, err := ParseReferences(class) + assert.NoError(t, err) + assert.NotNil(t, references) + + resolved, err := ResolveReferences(references, class) + assert.ErrorIs(t, err, ErrReferenceCycle) + assert.Len(t, resolved, 0) +} + +func TestResolveReferencesMulti(t *testing.T) { + class, err := NewClass("testdata/references/local/multi.yaml", codec.NewYamlCodec(), data.NewPathFromOsPath("multi")) + assert.NoError(t, err) + + expected := []Reference{ + { + Path: data.NewPath("multi.project.description"), + TargetPath: data.NewPath("project.name"), + }, + { + Path: data.NewPath("multi.project.description"), + TargetPath: data.NewPath("multi.project.name"), + }, + { + Path: data.NewPath("multi.project.description"), + TargetPath: data.NewPath("project.name"), + }, + { + Path: data.NewPath("multi.project.description"), + TargetPath: data.NewPath("multi.project.repo"), + }, + { + Path: data.NewPath("multi.project.repo"), + TargetPath: data.NewPath("multi.common.repo_url"), + }, + } + + references, err := ParseReferences(class) + assert.NoError(t, err) + assert.NotNil(t, references) + assert.Len(t, references, len(expected)) + + resolved, err := ResolveReferences(references, class) + assert.NoError(t, err) + assert.Len(t, resolved, len(expected)) + + // The TopologicalSort does always produce a correct + // result, but the order still may change as there + // is usually more than one solution. + // Hence we only check that the sincle nested reference + // is properly resolved. Here the 'multi.common.repo_url' + // MUST be the first reference to be resolved. + assert.Equal(t, resolved[0], expected[4]) +} diff --git a/testdata/references/local/chained.yaml b/testdata/references/local/chained.yaml index e867f20..b0eea1b 100644 --- a/testdata/references/local/chained.yaml +++ b/testdata/references/local/chained.yaml @@ -1,7 +1,8 @@ chained: greeting: Hello, ${first_name} - gotcha: ${chained:john:name} + gotcha: ${chained:john:first_name} john: - name: John + first_name: John + last_name: Doe first_name: ${name_placeholder} name_placeholder: ${gotcha} diff --git a/testdata/references/local/cycle.yaml b/testdata/references/local/cycle.yaml new file mode 100644 index 0000000..f77b87f --- /dev/null +++ b/testdata/references/local/cycle.yaml @@ -0,0 +1,4 @@ +cycle: + john: ${middle} + name: ${john} + middle: ${name} diff --git a/testdata/references/local/multi.yaml b/testdata/references/local/multi.yaml new file mode 100644 index 0000000..ac82c41 --- /dev/null +++ b/testdata/references/local/multi.yaml @@ -0,0 +1,9 @@ +multi: + common: + repo_url: "github.com/lukasjarosch/skipper" + project: + name: "skipper" + repo: "${multi:common:repo_url}" + description: > + The ${project:name} project is very cool. Because ${multi:project:name} helps in working with the Infrastructure as Data concept. + The project '${project:name}' is hosted on ${multi:project:repo}.