Skip to content

Commit

Permalink
[INTERNAL] ProjectGraph: Improve comments, error handling, tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@RandomByte
RandomByte committed Jan 30, 2023
1 parent caf2bc5 commit 649107e
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 41 deletions.
56 changes: 27 additions & 29 deletions lib/graph/ProjectGraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import {getLogger} from "@ui5/logger";
const log = getLogger("graph:ProjectGraph");

/**
* A rooted, directed graph representing a UI5 project, its dependencies and available extensions
* A rooted, directed graph representing a UI5 project, its dependencies and available extensions.
* <br><br>
* While it allows defining cyclic dependencies, both traversal functions will throw if they encounter cycles.
*
* @public
* @class
Expand All @@ -20,9 +22,9 @@ class ProjectGraph {
}
this._rootProjectName = rootProjectName;

this._projects = new Map(); // maps project name to instance
this._adjList = new Map(); // maps project name to edges/dependencies
this._optAdjList = new Map(); // maps project name to optional dependencies
this._projects = new Map(); // maps project name to instance (= nodes)
this._adjList = new Map(); // maps project name to dependencies (= edges)
this._optAdjList = new Map(); // maps project name to optional dependencies (= edges)

this._extensions = new Map(); // maps extension name to instance

Expand Down Expand Up @@ -175,10 +177,6 @@ class ProjectGraph {
declareDependency(fromProjectName, toProjectName) {
this._checkSealed();
try {
// if (this._optAdjList[fromProjectName] && this._optAdjList[fromProjectName][toProjectName]) {
// // TODO: Do we even care?
// throw new Error(`Dependency has already been declared as optional`);
// }
log.verbose(`Declaring dependency: ${fromProjectName} depends on ${toProjectName}`);
this._declareDependency(this._adjList, fromProjectName, toProjectName);
} catch (err) {
Expand All @@ -199,10 +197,6 @@ class ProjectGraph {
declareOptionalDependency(fromProjectName, toProjectName) {
this._checkSealed();
try {
// if (this._adjList[fromProjectName] && this._adjList[fromProjectName][toProjectName]) {
// // TODO: Do we even care?
// throw new Error(`Dependency has already been declared as non-optional`);
// }
log.verbose(`Declaring optional dependency: ${fromProjectName} depends on ${toProjectName}`);
this._declareDependency(this._optAdjList, fromProjectName, toProjectName);
this._hasUnresolvedOptionalDependencies = true;
Expand Down Expand Up @@ -267,6 +261,11 @@ class ProjectGraph {
*/
getTransitiveDependencies(projectName) {
const dependencies = new Set();
if (!this._projects.has(projectName)) {
throw new Error(
`Failed to get transitive dependencies for project ${projectName}: ` +
`Unable to find project in project graph`);
}

const processDependency = (depName) => {
const adjacencies = this._adjList.get(depName);
Expand Down Expand Up @@ -375,7 +374,6 @@ class ProjectGraph {
* graph traversal will wait and only continue once the promise has resolved.
*/

// TODO: Use generator functions instead?
/**
* Visit every project in the graph that can be reached by the given entry project exactly once.
* The entry project defaults to the root project.
Expand All @@ -398,28 +396,28 @@ class ProjectGraph {

const queue = [{
projectNames: [startName],
predecessors: []
ancestors: []
}];

const visited = Object.create(null);

while (queue.length) {
const {projectNames, predecessors} = queue.shift(); // Get and remove first entry from queue
const {projectNames, ancestors} = queue.shift(); // Get and remove first entry from queue

await Promise.all(projectNames.map(async (projectName) => {
this._checkCycle(predecessors, projectName);
this._checkCycle(ancestors, projectName);

if (visited[projectName]) {
return visited[projectName];
}

return visited[projectName] = (async () => {
const newPredecessors = [...predecessors, projectName];
const newAncestors = [...ancestors, projectName];
const dependencies = this.getDependencies(projectName);

queue.push({
projectNames: dependencies,
predecessors: newPredecessors
ancestors: newAncestors
});

await callback({
Expand Down Expand Up @@ -453,17 +451,17 @@ class ProjectGraph {
return this._traverseDepthFirst(startName, Object.create(null), [], callback);
}

async _traverseDepthFirst(projectName, visited, predecessors, callback) {
this._checkCycle(predecessors, projectName);
async _traverseDepthFirst(projectName, visited, ancestors, callback) {
this._checkCycle(ancestors, projectName);

if (visited[projectName]) {
return visited[projectName];
}
return visited[projectName] = (async () => {
const newPredecessors = [...predecessors, projectName];
const newAncestors = [...ancestors, projectName];
const dependencies = this.getDependencies(projectName);
await Promise.all(dependencies.map((depName) => {
return this._traverseDepthFirst(depName, visited, newPredecessors, callback);
return this._traverseDepthFirst(depName, visited, newAncestors, callback);
}));

await callback({
Expand Down Expand Up @@ -609,13 +607,13 @@ class ProjectGraph {
}
}

_checkCycle(predecessors, projectName) {
if (predecessors.includes(projectName)) {
// We start to run in circles. That's neither expected nor something we can deal with

// Mark first and last occurrence in chain with an asterisk
predecessors[predecessors.indexOf(projectName)] = `${projectName}*`;
throw new Error(`Detected cyclic dependency chain: ${predecessors.join(" -> ")} -> ${projectName}*`);
_checkCycle(ancestors, projectName) {
if (ancestors.includes(projectName)) {
// "Back-edge" detected. Neither BFS nor DFS searches should continue
// Mark first and last occurrence in chain with an asterisk and throw an error detailing the
// problematic dependency chain
ancestors[ancestors.indexOf(projectName)] = `*${projectName}*`;
throw new Error(`Detected cyclic dependency chain: ${ancestors.join(" -> ")} -> *${projectName}*`);
}
}

Expand Down
21 changes: 17 additions & 4 deletions test/lib/graph/ProjectGraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,20 @@ test("getTransitiveDependencies", async (t) => {
], "Should store and return correct transitive dependencies for library.a");
});

test("getTransitiveDependencies: Unknown project", (t) => {
const {ProjectGraph} = t.context;
const graph = new ProjectGraph({
rootProjectName: "my root project"
});

const error = t.throws(() => {
graph.getTransitiveDependencies("library.x");
});
t.is(error.message,
"Failed to get transitive dependencies for project library.x: Unable to find project in project graph",
"Should throw with expected error message");
});

test("declareDependency: Unknown source", async (t) => {
const {ProjectGraph} = t.context;
const graph = new ProjectGraph({
Expand Down Expand Up @@ -485,7 +499,6 @@ test("declareDependency: Already declared as optional, now non-optional", async
"Should declare dependency as non-optional");
});


test("getDependencies: Project without dependencies", async (t) => {
const {ProjectGraph} = t.context;
const graph = new ProjectGraph({
Expand Down Expand Up @@ -775,7 +788,7 @@ test("traverseBreadthFirst: Detect cycle", async (t) => {

const error = await t.throwsAsync(graph.traverseBreadthFirst(() => {}));
t.is(error.message,
"Detected cyclic dependency chain: library.a* -> library.b -> library.a*",
"Detected cyclic dependency chain: *library.a* -> library.b -> *library.a*",
"Should throw with expected error message");
});

Expand Down Expand Up @@ -1000,7 +1013,7 @@ test("traverseDepthFirst: Detect cycle", async (t) => {

const error = await t.throwsAsync(graph.traverseDepthFirst(() => {}));
t.is(error.message,
"Detected cyclic dependency chain: library.a* -> library.b -> library.a*",
"Detected cyclic dependency chain: *library.a* -> library.b -> *library.a*",
"Should throw with expected error message");
});

Expand All @@ -1020,7 +1033,7 @@ test("traverseDepthFirst: Cycle which does not occur in BFS", async (t) => {

const error = await t.throwsAsync(graph.traverseDepthFirst(() => {}));
t.is(error.message,
"Detected cyclic dependency chain: library.a -> library.b* -> library.c -> library.b*",
"Detected cyclic dependency chain: library.a -> *library.b* -> library.c -> *library.b*",
"Should throw with expected error message");
});

Expand Down
12 changes: 6 additions & 6 deletions test/lib/graph/graphFromObject.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ test("Application Cycle A: Traverse project graph breadth first with cycles", as
t.is(callbackStub.callCount, 4, "Four projects have been visited");

t.is(error.message,
"Detected cyclic dependency chain: application.cycle.a* -> component.cycle.a " +
"-> application.cycle.a*",
"Detected cyclic dependency chain: *application.cycle.a* -> component.cycle.a " +
"-> *application.cycle.a*",
"Threw with expected error message");

const callbackCalls = callbackStub.getCalls().map((call) => call.args[0].project.getName());
Expand Down Expand Up @@ -143,8 +143,8 @@ test("Application Cycle A: Traverse project graph depth first with cycles", asyn
t.is(callbackStub.callCount, 0, "Zero projects have been visited");

t.is(error.message,
"Detected cyclic dependency chain: application.cycle.a* -> component.cycle.a " +
"-> application.cycle.a*",
"Detected cyclic dependency chain: *application.cycle.a* -> component.cycle.a " +
"-> *application.cycle.a*",
"Threw with expected error message");
});

Expand All @@ -157,8 +157,8 @@ test("Application Cycle B: Traverse project graph depth first with cycles", asyn
t.is(callbackStub.callCount, 0, "Zero projects have been visited");

t.is(error.message,
"Detected cyclic dependency chain: application.cycle.b -> module.d* " +
"-> module.e -> module.d*",
"Detected cyclic dependency chain: application.cycle.b -> *module.d* " +
"-> module.e -> *module.d*",
"Threw with expected error message");
});

Expand Down
25 changes: 24 additions & 1 deletion test/lib/graph/projectGraphBuilder.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,29 @@ test("Node depends on itself", async (t) => {
"Threw with expected error message");
});

test("Cyclic dependencies", async (t) => {
t.context.getRootNode.resolves(createNode({
id: "id1",
name: "project-1"
}));
t.context.getDependencies
.onFirstCall().resolves([
createNode({
id: "id2",
name: "project-2"
}),
])
.onSecondCall().resolves([
createNode({
id: "id1",
name: "project-1"
}),
]);
const graph = await projectGraphBuilder(t.context.provider);
t.deepEqual(graph.getDependencies("project-1"), ["project-2"], "Cyclic dependency has been added");
t.deepEqual(graph.getDependencies("project-2"), ["project-1"], "Cyclic dependency has been added");
});

test("Nested node with same id is ignored", async (t) => {
t.context.getRootNode.resolves(createNode({
id: "id1",
Expand All @@ -263,7 +286,7 @@ test("Nested node with same id is ignored", async (t) => {
t.context.getDependencies.onSecondCall().resolves([
createNode({
id: "id1",
name: "project-3"
name: "project-3" // name will be ignored, since the first "id1" node is being used
}),
]);
const graph = await projectGraphBuilder(t.context.provider);
Expand Down
2 changes: 1 addition & 1 deletion test/lib/graph/providers/NodePackageDependencies.integration.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ test("AppCycleD: cyclic npm deps - Cycles everywhere", async (t) => {

const error = await t.throwsAsync(testGraphCreationDfs(t, npmProvider, []));
t.is(error.message,
`Detected cyclic dependency chain: application.cycle.d -> module.h* -> module.i -> module.k -> module.h*`);
`Detected cyclic dependency chain: application.cycle.d -> *module.h* -> module.i -> module.k -> *module.h*`);
});

test("AppCycleE: cyclic npm deps - Cycle via devDependency", async (t) => {
Expand Down

0 comments on commit 649107e

Please sign in to comment.