fix: prevent schema dependency cycles (#894)

backend/schema/schema_test.go

@@ -481,3 +481,86 @@ var testSchema = MustValidate(&Schema{
 		},
 	},
 })
+
+func TestValidateDependencies(t *testing.T) {
+	// one <--> two, cyclical
+	_, err := ParseString("", `
+		module one {
+			verb one(builtin.Empty) builtin.Empty
+				calls two.two
+		}
+
+		module two {
+			verb two(builtin.Empty) builtin.Empty
+				calls one.one
+		}
+	`)
+	assert.Error(t, err)
+
+	// one --> two --> three, noncyclical
+	_, err = ParseString("", `
+		module one {
+			verb one(builtin.Empty) builtin.Empty
+				calls two.two
+		}
+
+		module two {
+			verb two(builtin.Empty) builtin.Empty
+				calls three.three
+		}
+
+		module three {
+			verb three(builtin.Empty) builtin.Empty
+		}
+	`)
+	assert.NoError(t, err)
+
+	// one --> two --> three -> one, cyclical
+	_, err = ParseString("", `
+		module one {
+			verb one(builtin.Empty) builtin.Empty
+				calls two.two
+		}
+
+		module two {
+			verb two(builtin.Empty) builtin.Empty
+				calls three.three
+		}
+
+		module three {
+			verb three(builtin.Empty) builtin.Empty
+			calls one.one
+		}
+	`)
+	assert.Error(t, err)
+
+	// one --> one, this is allowed
+	_, err = ParseString("", `
+		module one {
+			verb a(builtin.Empty) builtin.Empty
+				calls one.b
+
+			verb b(builtin.Empty) builtin.Empty
+				calls one.a
+		}
+	`)
+	assert.NoError(t, err)
+
+	// one.a --> two.a
+	// one.b <---
+	// cyclical (does not depend on verbs used)
+
+	_, err = ParseString("", `
+		module one {
+			verb a(builtin.Empty) builtin.Empty
+				calls two.a
+			verb b(builtin.Empty) builtin.Empty
+		}
+
+		module two {
+			verb a(builtin.Empty) builtin.Empty
+				calls one.b
+		}
+	`)
+	assert.Error(t, err)
+}

backend/schema/validate.go

@@ -32,6 +32,18 @@ var (
 	}
 )
 
+type dependencyVertex struct {
+	from, to string
+}
+
+type dependencyVertexState int
+
+const (
+	notExplored dependencyVertexState = iota
+	exploring
+	fullyExplored
+)
+
 // MustValidate panics if a schema is invalid.
 //
 // This is useful for testing.
@@ -58,6 +70,11 @@ func Validate(schema *Schema) (*Schema, error) {
 
 	scopes := NewScopes()
 
+	// Validate dependencies
+	if err := validateDependencies(schema); err != nil {
+		merr = append(merr, err)
+	}
+
 	// First pass, add all the modules.
 	for _, module := range schema.Modules {
 		if module == builtins {
@@ -309,3 +326,52 @@ func cleanErrors(merr []error) []error {
 	})
 	return merr
 }
+
+func validateDependencies(schema *Schema) error {
+	// go through schema's modules, find cycles in modules' dependencies
+
+	// First pass, set up direct imports and vertex states for each module
+	imports := map[string][]string{}
+	vertexStates := map[dependencyVertex]dependencyVertexState{}
+	for _, module := range schema.Modules {
+		currentImports := module.Imports()
+		imports[module.Name] = currentImports
+
+		for _, imp := range currentImports {
+			vertexStates[dependencyVertex{module.Name, imp}] = notExplored
+		}
+	}
+
+	// DFS to find cycles
+	for vertex := range vertexStates {
+		if cycle := dfsForDependencyCycle(imports, vertexStates, vertex); cycle != nil {
+			return fmt.Errorf("found cycle in dependencies: %s", strings.Join(cycle, " -> "))
+		}
+	}
+
+	return nil
+}
+
+func dfsForDependencyCycle(imports map[string][]string, vertexStates map[dependencyVertex]dependencyVertexState, v dependencyVertex) []string {
+	switch vertexStates[v] {
+	case notExplored:
+		vertexStates[v] = exploring
+
+		for _, toModule := range imports[v.to] {
+			nextV := dependencyVertex{v.to, toModule}
+			if cycle := dfsForDependencyCycle(imports, vertexStates, nextV); cycle != nil {
+				// found cycle. prepend current module to cycle and return
+				cycle = append([]string{nextV.from}, cycle...)
+				return cycle
+			}
+		}
+		vertexStates[v] = fullyExplored
+		return nil
+	case exploring:
+		return []string{v.to}
+	case fullyExplored:
+		return nil
+	}
+
+	return nil
+}