From 4eea9bdbea856b26758a76d88e4d77e6d870ae2e Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Tue, 10 Mar 2020 09:23:40 +0100 Subject: [PATCH] fix(core): improved circular dependency detection We now detect circular dependencies in `ConfigGraph` after the graph has been augmented, possibly catching more circular dependencies. Also added circular task dependency detection to `TaskGraph`. We also use the new `DependencyValidationGraph` class everywhere we need to detect circular dependencies, e.g. when detecting circular plugin/plugin module dependencies. --- garden-service/src/config-graph.ts | 41 ++-- garden-service/src/garden.ts | 27 +-- garden-service/src/plugins.ts | 55 ++--- garden-service/src/task-graph.ts | 36 +++- .../src/util/validate-dependencies.ts | 190 ++++++++++-------- garden-service/test/unit/src/garden.ts | 26 +-- garden-service/test/unit/src/task-graph.ts | 28 ++- .../unit/src/util/validate-dependencies.ts | 94 +++++++-- 8 files changed, 315 insertions(+), 182 deletions(-) diff --git a/garden-service/src/config-graph.ts b/garden-service/src/config-graph.ts index 62f7e2a848..97028e6dee 100644 --- a/garden-service/src/config-graph.ts +++ b/garden-service/src/config-graph.ts @@ -18,7 +18,11 @@ import { TestConfig } from "./config/test" import { uniqByName, pickKeys } from "./util/util" import { ConfigurationError } from "./exceptions" import { deline } from "./util/string" -import { validateDependencies } from "./util/validate-dependencies" +import { + detectMissingDependencies, + handleDependencyErrors, + DependencyValidationGraph, +} from "./util/validate-dependencies" import { ServiceConfig } from "./config/service" import { TaskConfig } from "./config/task" import { makeTestTaskName } from "./tasks/helpers" @@ -69,6 +73,8 @@ interface EntityConfigEntry { config: C } +export type DependencyGraph = { [key: string]: DependencyGraphNode } + /** * A graph data structure that facilitates querying (recursive or non-recursive) of the project's dependency and * dependant relationships. @@ -76,7 +82,7 @@ interface EntityConfigEntry { * This should be initialized with fully resolved and validated ModuleConfigs. */ export class ConfigGraph { - private dependencyGraph: { [key: string]: DependencyGraphNode } + private dependencyGraph: DependencyGraph private moduleConfigs: { [key: string]: ModuleConfig } private serviceConfigs: { @@ -164,7 +170,11 @@ export class ConfigGraph { } } - this.validateDependencies() + const missingDepsError = detectMissingDependencies( + Object.values(this.moduleConfigs), + Object.keys(this.serviceConfigs), + Object.keys(this.taskConfigs) + ) // Add relations between nodes for (const moduleConfig of moduleConfigs) { @@ -260,6 +270,21 @@ export class ConfigGraph { } } } + + const validationGraph = DependencyValidationGraph.fromDependencyGraph(this.dependencyGraph) + const cycles = validationGraph.detectCircularDependencies() + + let circularDepsError + if (cycles.length > 0) { + const description = validationGraph.cyclesToString(cycles) + const errMsg = `\nCircular dependencies detected: \n\n${description}\n` + circularDepsError = new ConfigurationError(errMsg, { "circular-dependencies": description }) + } else { + circularDepsError = null + } + + // Throw an error if one or both of these errors is non-null. + handleDependencyErrors(missingDepsError, circularDepsError) } // Convenience method used in the constructor above. @@ -267,14 +292,6 @@ export class ConfigGraph { return getModuleKey(config.name, config.plugin) } - private validateDependencies() { - validateDependencies( - Object.values(this.moduleConfigs), - Object.keys(this.serviceConfigs), - Object.keys(this.taskConfigs) - ) - } - private addRuntimeRelation(node: DependencyGraphNode, depName: string) { const dep = this.serviceConfigs[depName] || this.taskConfigs[depName] @@ -729,7 +746,7 @@ export class DependencyGraphNode { * Note: If type === "build", name should be a prefix-qualified module name, as * returned by keyForModule or getModuleKey. */ -function nodeKey(type: DependencyGraphNodeType, name: string) { +export function nodeKey(type: DependencyGraphNodeType, name: string) { return `${type}.${name}` } diff --git a/garden-service/src/garden.ts b/garden-service/src/garden.ts index dee1f3cb08..ef65208af0 100644 --- a/garden-service/src/garden.ts +++ b/garden-service/src/garden.ts @@ -7,6 +7,10 @@ */ import Bluebird from "bluebird" +import chalk from "chalk" +import { ensureDir } from "fs-extra" +import dedent from "dedent" +import { platform, arch } from "os" import { parse, relative, resolve, dirname } from "path" import { flatten, isString, sortBy, fromPairs, keyBy } from "lodash" const AsyncLock = require("async-lock") @@ -37,7 +41,6 @@ import { ModuleConfigContext, OutputConfigContext } from "./config/config-contex import { createPluginContext, CommandInfo } from "./plugin-context" import { ModuleAndRuntimeActionHandlers, RegisterPluginParam } from "./types/plugin/plugin" import { SUPPORTED_PLATFORMS, SupportedPlatform, DEFAULT_GARDEN_DIR_NAME } from "./constants" -import { platform, arch } from "os" import { LogEntry } from "./logger/log-entry" import { EventBus } from "./events" import { Watcher } from "./watch" @@ -52,14 +55,11 @@ import { import { Provider, ProviderConfig, getAllProviderDependencyNames, defaultProvider } from "./config/provider" import { ResolveProviderTask } from "./tasks/resolve-provider" import { ActionRouter } from "./actions" -import { detectCycles, cyclesToString, Dependency } from "./util/validate-dependencies" -import chalk from "chalk" import { RuntimeContext } from "./runtime-context" -import { ensureDir } from "fs-extra" import { loadPlugins, getDependencyOrder, getModuleTypes } from "./plugins" import { deline, naturalList } from "./util/string" -import dedent from "dedent" import { ensureConnected } from "./db/connection" +import { DependencyValidationGraph } from "./util/validate-dependencies" export interface ActionHandlerMap { [actionName: string]: PluginActionHandlers[T] @@ -475,25 +475,26 @@ export class Garden { const plugins = keyBy(await this.getPlugins(), "name") - // Detect circular deps here - const pluginDeps: Dependency[] = [] + // Detect circular dependencies here + const validationGraph = new DependencyValidationGraph() await Bluebird.map(rawConfigs, async (config) => { const plugin = plugins[config.name] + validationGraph.addNode(plugin.name) for (const dep of await getAllProviderDependencyNames(plugin!, config!)) { - pluginDeps.push({ from: config!.name, to: dep }) + validationGraph.addNode(dep) + validationGraph.addDependency(plugin.name, dep) } }) - const cycles = detectCycles(pluginDeps) + const cycles = validationGraph.detectCircularDependencies() if (cycles.length > 0) { - const cyclesStr = cyclesToString(cycles) - + const description = validationGraph.cyclesToString(cycles) throw new PluginError( - "One or more circular dependencies found between providers or their configurations: " + cyclesStr, - { cycles } + `One or more circular dependencies found between providers or their configurations:\n\n${description}`, + { "circular-dependencies": description } ) } diff --git a/garden-service/src/plugins.ts b/garden-service/src/plugins.ts index 48c9cf9e93..b5424fe2c8 100644 --- a/garden-service/src/plugins.ts +++ b/garden-service/src/plugins.ts @@ -6,7 +6,6 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { DepGraph } from "dependency-graph" import { PluginMap, GardenPlugin, @@ -19,9 +18,10 @@ import { ProviderConfig } from "./config/provider" import { ConfigurationError, PluginError, RuntimeError } from "./exceptions" import { uniq, mapValues, fromPairs, flatten, keyBy, some } from "lodash" import { findByName, pushToKey, getNames } from "./util/util" -import { deline, naturalList } from "./util/string" +import { deline } from "./util/string" import { validateSchema } from "./config/validation" import { LogEntry } from "./logger/log-entry" +import { DependencyValidationGraph } from "./util/validate-dependencies" export function loadPlugins(log: LogEntry, registeredPlugins: PluginMap, configs: ProviderConfig[]) { const loadedPlugins: PluginMap = {} @@ -108,7 +108,7 @@ export function loadPlugins(log: LogEntry, registeredPlugins: PluginMap, configs * Returns the given provider configs in dependency order. */ export function getDependencyOrder(configs: T[], registeredPlugins: PluginMap): T[] { - let graph = new DepGraph() + const graph = new DependencyValidationGraph() for (const plugin of Object.values(registeredPlugins)) { graph.addNode(plugin.name) @@ -124,20 +124,16 @@ export function getDependencyOrder(configs: T[], regis } } - let ordered: string[] + const cycles = graph.detectCircularDependencies() - try { - ordered = graph.overallOrder() - } catch (err) { - if (err.cyclePath) { - throw new PluginError(`Found a circular dependency between registered plugins: ${err.cyclePath.join(" -> ")}`, { - cyclePath: err.cyclePath, - }) - } else { - throw err - } + if (cycles.length > 0) { + const description = graph.cyclesToString(cycles) + const detail = { "circular-dependencies": description } + throw new PluginError(`Found a circular dependency between registered plugins:\n\n${description}`, detail) } + const ordered = graph.overallOrder() + // Note: concat() makes sure we're not mutating the original array, because JS... return configs.concat().sort((a, b) => { return ordered.indexOf(a.name) - ordered.indexOf(b.name) @@ -316,7 +312,7 @@ interface ModuleDefinitionMap { function resolveModuleDefinitions(resolvedPlugins: PluginMap, configs: ProviderConfig[]): PluginMap { // Collect module type declarations - const graph = new DepGraph() + const graph = new DependencyValidationGraph() const moduleDefinitionMap: { [moduleType: string]: { plugin: GardenPlugin; spec: ModuleTypeDefinition }[] } = {} const moduleExtensionMap: { [moduleType: string]: { plugin: GardenPlugin; spec: ModuleTypeExtension }[] } = {} @@ -324,10 +320,10 @@ function resolveModuleDefinitions(resolvedPlugins: PluginMap, configs: ProviderC for (const spec of plugin.createModuleTypes) { pushToKey(moduleDefinitionMap, spec.name, { plugin, spec }) - graph.addNode(spec.name) + graph.addNode(spec.name, `${spec.name} (from plugin ${plugin.name})`) if (spec.base) { - graph.addNode(spec.base) + graph.addNode(spec.base, `${spec.base} (from plugin ${plugin.name})`) graph.addDependency(spec.name, spec.base) } } @@ -352,27 +348,16 @@ function resolveModuleDefinitions(resolvedPlugins: PluginMap, configs: ProviderC } // Make sure we don't have circular dependencies in module type bases - let ordered: string[] - - try { - ordered = graph.overallOrder() - } catch (err) { - if (err.cyclePath) { - const plugins: string[] = err.cyclePath.map( - (name: string) => "'" + moduleDefinitionMap[name][0].plugin.name + "'" - ) + const cycles = graph.detectCircularDependencies() - throw new PluginError( - `Found circular dependency between module type bases (defined in plugin(s) ${naturalList(uniq(plugins))}): ` + - err.cyclePath.join(" -> "), - { cyclePath: err.cyclePath } - ) - } else { - throw err - } + if (cycles.length > 0) { + const description = graph.cyclesToString(cycles) + const detail = { "circular-dependencies": description } + const msg = `Found circular dependency between module type bases:\n\n${description}` + throw new PluginError(msg, detail) } - ordered = ordered.filter((name) => name in moduleDefinitionMap) + const ordered = graph.overallOrder().filter((name) => name in moduleDefinitionMap) const resolvedDefinitions: ModuleDefinitionMap = {} diff --git a/garden-service/src/task-graph.ts b/garden-service/src/task-graph.ts index 6552f6f506..ddd7529c94 100644 --- a/garden-service/src/task-graph.ts +++ b/garden-service/src/task-graph.ts @@ -14,11 +14,12 @@ import { every, flatten, intersection, merge, padEnd, union, uniqWith, without } import { BaseTask, TaskDefinitionError, TaskType } from "./tasks/base" import { LogEntry, LogEntryMetadata, TaskLogStatus } from "./logger/log-entry" -import { toGardenError, GardenBaseError } from "./exceptions" +import { toGardenError, GardenBaseError, InternalError } from "./exceptions" import { Garden } from "./garden" import { dedent } from "./util/string" import { defer, relationshipClasses, uuidv4 } from "./util/util" import { renderError } from "./logger/renderers" +import { DependencyValidationGraph } from "./util/validate-dependencies" class TaskGraphError extends GardenBaseError { type = "task-graph" @@ -84,7 +85,15 @@ export class TaskGraph { async process(tasks: BaseTask[], opts?: ProcessTasksOpts): Promise { const unlimitedConcurrency = opts ? !!opts.unlimitedConcurrency : false - const nodes = await this.nodesWithDependencies(tasks, unlimitedConcurrency) + + const validationGraph = new DependencyValidationGraph() + + let nodes: TaskNode[] + try { + nodes = await this.nodesWithDependencies(tasks, validationGraph, unlimitedConcurrency) + } catch (circularDepsErr) { + throw circularDepsErr + } const batches = this.partition(nodes, { unlimitedConcurrency }) for (const batch of batches) { @@ -121,9 +130,28 @@ export class TaskGraph { return results } - async nodesWithDependencies(tasks: BaseTask[], unlimitedConcurrency = false): Promise { + async nodesWithDependencies( + tasks: BaseTask[], + validationGraph: DependencyValidationGraph, + unlimitedConcurrency = false + ): Promise { return Bluebird.map(tasks, async (task) => { - const depNodes = await this.nodesWithDependencies(await task.getDependencies(), unlimitedConcurrency) + const depTasks = await task.getDependencies() + + // Detect circular dependencies + validationGraph.addNode(task.getKey()) + for (const depTask of depTasks) { + validationGraph.addNode(depTask.getKey()) + validationGraph.addDependency(task.getKey(), depTask.getKey()) + } + const cycles = validationGraph.detectCircularDependencies() + if (cycles.length > 0) { + const description = validationGraph.cyclesToString(cycles) + const msg = `\nCircular task dependencies detected:\n\n${description}\n` + throw new InternalError(msg, { "circular-dependencies": description }) + } + + const depNodes = await this.nodesWithDependencies(depTasks, validationGraph, unlimitedConcurrency) return new TaskNode(task, depNodes, unlimitedConcurrency) }) } diff --git a/garden-service/src/util/validate-dependencies.ts b/garden-service/src/util/validate-dependencies.ts index 508f953ebd..457595c59a 100644 --- a/garden-service/src/util/validate-dependencies.ts +++ b/garden-service/src/util/validate-dependencies.ts @@ -6,22 +6,20 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import dedent from "dedent" +import { DepGraph } from "dependency-graph" import { merge, flatten, uniq } from "lodash" import indentString from "indent-string" import { get, isEqual, join, set, uniqWith } from "lodash" import { getModuleKey } from "../types/module" -import { ConfigurationError } from "../exceptions" -import { ServiceConfig } from "../config/service" -import { TaskConfig } from "../config/task" +import { ConfigurationError, ParameterError } from "../exceptions" import { ModuleConfig } from "../config/module" import { deline } from "./string" +import { DependencyGraph, DependencyGraphNode, nodeKey as configGraphNodeKey } from "../config-graph" -export function validateDependencies(moduleConfigs: ModuleConfig[], serviceNames: string[], taskNames: string[]): void { - // TODO: indicate in errors when modules are added by providers - const missingDepsError = detectMissingDependencies(moduleConfigs, serviceNames, taskNames) - const circularDepsError = detectCircularModuleDependencies(moduleConfigs) - +export function handleDependencyErrors( + missingDepsError: ConfigurationError | null, + circularDepsError: ConfigurationError | null +) { let errMsg = "" let detail = {} @@ -92,97 +90,123 @@ export function detectMissingDependencies( } } -export type Cycle = string[] +// Shared type used by ConfigGraph and TaskGraph to facilitate circular dependency detection +export type DependencyValidationGraphNode = { + key: string // same as a corresponding task's key + dependencies: string[] // array of keys + description?: string // used instead of key when rendering node in circular dependency error messages +} -/** - * Computes build and runtime dependency graphs for the given modules, and returns an error if cycles were found. - */ -export function detectCircularModuleDependencies(moduleConfigs: ModuleConfig[]): ConfigurationError | null { - // Sparse matrices - const buildDeps: Dependency[] = [] - const runtimeDeps: Dependency[] = [] - const services: ServiceConfig[] = [] - const tasks: TaskConfig[] = [] +export class DependencyValidationGraph { + graph: { [nodeKey: string]: DependencyValidationGraphNode } - /** - * Since dependencies listed in test configs cannot introduce circularities (because - * builds/deployments/tasks/tests cannot currently depend on tesxts), we don't need to - * account for test dependencies here. - */ - for (const module of moduleConfigs) { - // Build dependencies - for (const buildDep of module.build.dependencies) { - buildDeps.push({ - from: module.name, - to: getModuleKey(buildDep.name, buildDep.plugin), - }) - } + constructor(nodes?: DependencyValidationGraphNode[]) { + this.graph = Object.fromEntries((nodes || []).map((n) => [n.key, n])) + } - // Runtime (service & task) dependencies - for (const service of module.serviceConfigs || []) { - services.push(service) - for (const depName of service.dependencies) { - runtimeDeps.push({ - from: service.name, - to: depName, - }) + static fromDependencyGraph(dependencyGraph: DependencyGraph) { + const withDeps = (node: DependencyGraphNode): DependencyValidationGraphNode => { + return { + key: configGraphNodeKey(node.type, node.name), + dependencies: node.dependencies.map((d) => configGraphNodeKey(d.type, d.name)), } } + const nodes = Object.values(dependencyGraph).map((n) => withDeps(n)) + return new DependencyValidationGraph(nodes) + } + + overallOrder(): string[] { + const cycles = this.detectCircularDependencies() + if (cycles.length > 0) { + const description = cyclesToString(cycles) + const errMsg = `\nCircular dependencies detected: \n\n${description}\n` + throw new ConfigurationError(errMsg, { "circular-dependencies": description }) + } - for (const task of module.taskConfigs || []) { - tasks.push(task) - for (const depName of task.dependencies) { - runtimeDeps.push({ - from: task.name, - to: depName, - }) + const depGraph = new DepGraph() + for (const node of Object.values(this.graph)) { + depGraph.addNode(node.key) + for (const dep of node.dependencies) { + depGraph.addNode(dep) + depGraph.addDependency(node.key, dep) } } + return depGraph.overallOrder() } - const buildCycles = detectCycles(buildDeps) - const runtimeCycles = detectCycles(runtimeDeps) + /** + * Idempotent. + * + * If provided, description will be used instead of key when rendering the node in + * circular dependency error messages. + */ + addNode(key: string, description?: string) { + if (!this.graph[key]) { + this.graph[key] = { key, dependencies: [], description } + } + } - if (buildCycles.length > 0 || runtimeCycles.length > 0) { - const detail = {} + /** + * Idempotent. + * + * Throws an error if a node doesn't exist for either dependantKey or dependencyKey. + */ + addDependency(dependantKey: string, dependencyKey: string) { + if (!this.graph[dependantKey]) { + throw new ParameterError(`addDependency: no node exists for dependantKey ${dependantKey}`, { + dependantKey, + dependencyKey, + graph: this.graph, + }) + } - let errMsg = "Circular dependencies detected." + if (!this.graph[dependencyKey]) { + throw new ParameterError(`addDependency: no node exists for dependencyKey ${dependencyKey}`, { + dependantKey, + dependencyKey, + graph: this.graph, + }) + } - if (buildCycles.length > 0) { - const buildCyclesDescription = cyclesToString(buildCycles) - errMsg = errMsg.concat( - "\n\n" + - dedent` - Circular build dependencies: ${buildCyclesDescription} - ` - ) - detail["circular-build-dependencies"] = buildCyclesDescription + const dependant = this.graph[dependantKey] + if (!dependant.dependencies.find((d) => d === dependencyKey)) { + const dependency = this.graph[dependencyKey] + dependant.dependencies.push(dependency.key) } + } - if (runtimeCycles.length > 0) { - const runtimeCyclesDescription = cyclesToString(runtimeCycles) - errMsg = errMsg.concat( - "\n\n" + - dedent` - Circular service/task dependencies: ${runtimeCyclesDescription} - ` - ) - detail["circular-service-or-task-dependencies"] = runtimeCyclesDescription + /** + * Returns an error if cycles were found. + */ + detectCircularDependencies(): Cycle[] { + const edges: DependencyEdge[] = [] + + for (const node of Object.values(this.graph)) { + for (const dep of node.dependencies) { + edges.push({ from: node.key, to: dep }) + } } - return new ConfigurationError(errMsg, detail) + return detectCycles(edges) } - return null + cyclesToString(cycles: Cycle[]) { + const cycleDescriptions = cycles.map((c) => { + const nodeDescriptions = c.map((key) => this.graph[key].description || key) + return join(nodeDescriptions.concat([nodeDescriptions[0]]), " <- ") + }) + return cycleDescriptions.length === 1 ? cycleDescriptions[0] : cycleDescriptions.join("\n\n") + } } -// These types are used for our own dependency graph code, which we should likely deprecate -export interface Dependency { +type Cycle = string[] + +interface DependencyEdge { from: string to: string } -interface _DependencyGraph { +interface CycleGraph { [key: string]: { [target: string]: { distance: number @@ -194,17 +218,17 @@ interface _DependencyGraph { /** * Implements a variation on the Floyd-Warshall algorithm to compute minimal cycles. * - * This is approximately O(m^3) + O(s^3), where m is the number of modules and s is the number of services. + * This is approximately O(n^3), where n is the number of nodes in the graph. * * Returns a list of cycles found. */ -export function detectCycles(dependencies: Dependency[]): Cycle[] { +export function detectCycles(edges: DependencyEdge[]): Cycle[] { // Collect all the vertices and build a graph object - const vertices = uniq(flatten(dependencies.map((d) => [d.from, d.to]))) + const vertices = uniq(flatten(edges.map((d) => [d.from, d.to]))) - const graph: _DependencyGraph = {} + const graph: CycleGraph = {} - for (const { from, to } of dependencies) { + for (const { from, to } of edges) { set(graph, [from, to], { distance: 1, next: to }) } @@ -239,15 +263,15 @@ export function detectCycles(dependencies: Dependency[]): Cycle[] { ) } -function distance(graph: _DependencyGraph, source: string, destination: string): number { +function distance(graph: CycleGraph, source: string, destination: string): number { return get(graph, [source, destination, "distance"], Infinity) as number } -function next(graph: _DependencyGraph, source: string, destination: string): string | undefined { +function next(graph: CycleGraph, source: string, destination: string): string | undefined { return get(graph, [source, destination, "next"]) } export function cyclesToString(cycles: Cycle[]) { const cycleDescriptions = cycles.map((c) => join(c.concat([c[0]]), " <- ")) - return cycleDescriptions.length === 1 ? cycleDescriptions[0] : cycleDescriptions + return cycleDescriptions.length === 1 ? cycleDescriptions[0] : cycleDescriptions.join("\n\n") } diff --git a/garden-service/test/unit/src/garden.ts b/garden-service/test/unit/src/garden.ts index 67a57673c3..903b1e1b52 100644 --- a/garden-service/test/unit/src/garden.ts +++ b/garden-service/test/unit/src/garden.ts @@ -561,7 +561,8 @@ describe("Garden", () => { () => garden.getPlugins(), (err) => expect(err.message).to.equal(deline` - Found circular dependency between module type bases (defined in plugin(s) 'foo'): foo -> bar -> foo + Found circular dependency between module type + bases:\n\nfoo (from plugin foo) <- bar (from plugin foo) <- foo (from plugin foo) `) ) }) @@ -884,7 +885,7 @@ describe("Garden", () => { await expectError( () => garden.getPlugins(), (err) => - expect(err.message).to.equal("Found a circular dependency between registered plugins: foo -> bar -> foo") + expect(err.message).to.equal("Found a circular dependency between registered plugins:\n\nfoo <- bar <- foo") ) }) @@ -1252,7 +1253,7 @@ describe("Garden", () => { () => garden.getPlugins(), (err) => expect(err.message).to.equal( - "Found a circular dependency between registered plugins: base-a -> foo -> base-b -> base-a" + "Found a circular dependency between registered plugins:\n\nbase-a <- foo <- base-b <- base-a" ) ) }) @@ -1482,7 +1483,7 @@ describe("Garden", () => { () => garden.resolveProviders(), (err) => expect(err.message).to.equal( - "Found a circular dependency between registered plugins: test-a -> test-b -> test-a" + "Found a circular dependency between registered plugins:\n\ntest-a <- test-b <- test-a" ) ) }) @@ -1510,7 +1511,7 @@ describe("Garden", () => { await expectError( () => garden.resolveProviders(), (err) => - expect(err.message).to.equal("Found a circular dependency between registered plugins: test-a -> test-a") + expect(err.message).to.equal("Found a circular dependency between registered plugins:\n\ntest-a <- test-a") ) }) @@ -1543,10 +1544,9 @@ describe("Garden", () => { await expectError( () => garden.resolveProviders(), (err) => - expect(err.message).to.equal( - "One or more circular dependencies found between providers " + - "or their configurations: test-a <- test-b <- test-a" - ) + expect(err.message).to.equal(deline` + One or more circular dependencies found between providers or their configurations:\n\ntest-a <- test-b <- test-a + `) ) }) @@ -1578,10 +1578,10 @@ describe("Garden", () => { await expectError( () => garden.resolveProviders(), (err) => - expect(err.message).to.equal( - "One or more circular dependencies found between providers " + - "or their configurations: test-b <- test-a <- test-b" - ) + expect(err.message).to.equal(deline` + One or more circular dependencies found between providers or their + configurations:\n\ntest-a <- test-b <- test-a + `) ) }) diff --git a/garden-service/test/unit/src/task-graph.ts b/garden-service/test/unit/src/task-graph.ts index 0c43027313..102c80f464 100644 --- a/garden-service/test/unit/src/task-graph.ts +++ b/garden-service/test/unit/src/task-graph.ts @@ -14,12 +14,13 @@ import { TaskGraph, TaskResult, TaskResults } from "../../../src/task-graph" import { makeTestGarden, freezeTime, dataDir, expectError, TestGarden } from "../../helpers" import { Garden } from "../../../src/garden" import { deepFilter, defer, sleep, uuidv4 } from "../../../src/util/util" +import { DependencyValidationGraph } from "../../../src/util/validate-dependencies" const projectRoot = join(dataDir, "test-project-empty") -type TestTaskCallback = (name: string, result: any) => Promise +export type TestTaskCallback = (name: string, result: any) => Promise -interface TestTaskOptions { +export interface TestTaskOptions { callback?: TestTaskCallback dependencies?: BaseTask[] versionString?: string @@ -27,7 +28,7 @@ interface TestTaskOptions { throwError?: boolean } -class TestTask extends BaseTask { +export class TestTask extends BaseTask { type: TaskType = "test" name: string callback: TestTaskCallback | null @@ -160,8 +161,19 @@ describe("task-graph", () => { ]) }) - it.skip("should throw if tasks have circular dependencies", async () => { - throw new Error("TODO") + it("should throw if tasks have circular dependencies", async () => { + const garden = await getGarden() + const graph = new TaskGraph(garden, garden.log) + const taskA = new TestTask(garden, "a", false) + const taskB = new TestTask(garden, "b", false, { dependencies: [taskA] }) + const taskC = new TestTask(garden, "c", false, { dependencies: [taskB] }) + taskA["dependencies"] = [taskC] + const errorMsg = "\nCircular task dependencies detected:\n\nb <- a <- c <- b\n" + + await expectError( + () => graph.process([taskB]), + (err) => expect(err.message).to.eql(errorMsg) + ) }) it("should emit events when processing and completing a task", async () => { @@ -831,7 +843,8 @@ describe("task-graph", () => { const taskC = new TestTask(garden, "c", false) const tasks = [taskA, taskB, taskC, taskADep1, taskBDep, taskADep2] - const taskNodes = await graph.nodesWithDependencies(tasks, false) + const validationGraph = new DependencyValidationGraph() + const taskNodes = await graph.nodesWithDependencies(tasks, validationGraph, false) const batches = graph.partition(taskNodes, { unlimitedConcurrency: false }) const batchKeys = batches.map((b) => b.nodes.map((n) => n.getKey())) @@ -876,7 +889,8 @@ describe("task-graph", () => { taskC, ] - const taskNodes = await graph.nodesWithDependencies(tasks, false) + const validationGraph = new DependencyValidationGraph() + const taskNodes = await graph.nodesWithDependencies(tasks, validationGraph, false) const batches = graph.partition(taskNodes, { unlimitedConcurrency: false }) const batchKeys = batches.map((b) => b.nodes.map((n) => n.getKey())) diff --git a/garden-service/test/unit/src/util/validate-dependencies.ts b/garden-service/test/unit/src/util/validate-dependencies.ts index 863991b1b4..8df95c12d4 100644 --- a/garden-service/test/unit/src/util/validate-dependencies.ts +++ b/garden-service/test/unit/src/util/validate-dependencies.ts @@ -11,9 +11,9 @@ import { join } from "path" import { detectCycles, detectMissingDependencies, - detectCircularModuleDependencies, + DependencyValidationGraph, } from "../../../../src/util/validate-dependencies" -import { makeTestGarden, dataDir } from "../../../helpers" +import { makeTestGarden, dataDir, expectError } from "../../../helpers" import { ModuleConfig } from "../../../../src/config/module" import { ConfigurationError } from "../../../../src/exceptions" import { Garden } from "../../../../src/garden" @@ -66,21 +66,85 @@ describe("validate-dependencies", () => { }) }) - describe("detectCircularDependencies", () => { - it("should return an error when circular dependencies are present", async () => { - const circularProjectRoot = join(dataDir, "test-project-circular-deps") - const garden = await makeTestGarden(circularProjectRoot) - const { moduleConfigs } = await scanAndGetConfigs(garden) - const err = detectCircularModuleDependencies(moduleConfigs) - expect(err).to.be.an.instanceOf(ConfigurationError) + describe("DependencyValidationGraph", () => { + describe("detectCircularDependencies", () => { + it("should return an empty cycle array when no nodes or dependencies have been added", async () => { + const validationGraph = new DependencyValidationGraph() + const cycles = validationGraph.detectCircularDependencies() + expect(cycles).to.be.empty + }) + + it("should return a cycle when circular dependencies have been added", async () => { + const vg = new DependencyValidationGraph() + vg.addNode("a") + vg.addNode("b") + vg.addNode("c") + vg.addDependency("b", "a") + vg.addDependency("c", "b") + vg.addDependency("a", "c") + const cycles = vg.detectCircularDependencies() + expect(cycles).to.eql([["a", "c", "b"]]) + }) + + it("should return null when no circular dependencies have been added", async () => { + const vg = new DependencyValidationGraph() + vg.addNode("a") + vg.addNode("b") + vg.addNode("c") + vg.addDependency("b", "a") + vg.addDependency("c", "b") + vg.addDependency("c", "a") + const cycles = vg.detectCircularDependencies() + expect(cycles).to.be.empty + }) + + it("should return an error when circular config dependencies are present", async () => { + const circularProjectRoot = join(dataDir, "test-project-circular-deps") + const garden = await makeTestGarden(circularProjectRoot) + // This implicitly tests detectCircularDependencies, since that method is called in ConfigGraph's constructor. + await expectError( + () => garden.getConfigGraph(garden.log), + (e) => expect(e).to.be.instanceOf(ConfigurationError) + ) + }) + + it("should return null when no circular config dependencies are present", async () => { + const nonCircularProjectRoot = join(dataDir, "test-project-b") + const garden = await makeTestGarden(nonCircularProjectRoot) + const configGraph = await garden.getConfigGraph(garden.log) + const validationGraph = DependencyValidationGraph.fromDependencyGraph(configGraph["dependencyGraph"]) + const cycles = validationGraph.detectCircularDependencies() + expect(cycles).to.be.empty + }) }) - it("should return null when no circular dependencies are present", async () => { - const nonCircularProjectRoot = join(dataDir, "test-project-b") - const garden = await makeTestGarden(nonCircularProjectRoot) - const { moduleConfigs } = await scanAndGetConfigs(garden) - const err = detectCircularModuleDependencies(moduleConfigs) - expect(err).to.eql(null) + describe("overallOrder", () => { + it("should return the overall dependency order when circular dependencies are present", async () => { + const vg = new DependencyValidationGraph() + vg.addNode("a") + vg.addNode("b") + vg.addNode("c") + vg.addNode("d") + vg.addDependency("b", "a") + vg.addDependency("c", "b") + vg.addDependency("c", "a") + vg.addDependency("d", "c") + expect(vg.overallOrder()).to.eql(["a", "b", "c", "d"]) + }) + + it("should throw an error when circular dependencies are present", async () => { + const vg = new DependencyValidationGraph() + vg.addNode("a") + vg.addNode("b") + vg.addNode("c") + vg.addDependency("b", "a") + vg.addDependency("c", "b") + vg.addDependency("a", "c") + await expectError( + () => vg.overallOrder(), + (e) => expect(e).to.be.instanceOf(ConfigurationError) + ) + }) }) })