[bug] pkg/guacanalytics/patchPlanning.go fills in parent field for root nodes

[rmetzman]

Describe the bug
pkg/guacanalytics/patchPlanning.go for the root/input nodes which should have an empty parents list, the program for some reason populated this field to contain an empty string.

To Reproduce
Steps to reproduce the behavior:

1. Run the SearchDependenciesFromStartNode func
2. Run ToposortFromBfsNodeMap on the output (pkg/guacanalytics/toposort.go)
3. You will notice it doesn't enter the len(parentsList) == 0, even for the root nodes that do not have a parent so it thinks a cycle is detected
4. Try adding "parentsList[0] == "" && len(parentsList) == 1" to the if statement and you will see it does enter the clause

Expected behavior
len(parentsList) should == 1 for the start nodes inputted

[nathannaveen]

Could I work on this?

cc @pxp928

[pxp928]

@rmetzman are you already working to fix this?

[rmetzman]

Nope, I haven't started working on this yet.

[pxp928]

@nathannaveen assigning to you!

[nathannaveen]

@rmetzman and @pxp928

I assume that the statement in the expected behavior part: "len(parentsList) should == 1 for the start nodes inputted” is a typo and should be: "len(parentsList) should == 0 for the start nodes inputted”. I am using this assumption for the following part of this comment.

I am pretty sure that I have located the issue regarding where the root nodes having non empty parents is located.

If we start inside the function ToposortFromBfsNodeMap at the part of if len(parentsList) == 0 {, we can see that each parentsList is from parentsMap, and parentsMap is equal to copyParents(nodeMap). copyParents just copies the Parents parameter from the BfsNode struct of key in the inputMap. This shows us that we just have to look at where the Parents values of the nodeMap are being edited.

Now we get to SearchDependenciesFromStartNode. Now the only place where q.nodeMap can be edited is:

Line 80:

guac/pkg/guacanalytics/patchPlanning.go

Lines 80 to 86 in bc5c042

	if len(nodePkg.AllPkgTree.Namespaces[0].Names[0].Versions) == 0 {
	// TODO: handle case where there are circular dependencies that introduce more versions to the version list on a node that requires revisiting
	err := q.addNodesToQueueFromPackageName(ctx, gqlClient, nodePkg.AllPkgTree.Type, nodePkg.AllPkgTree.Namespaces[0].Namespace, nodePkg.AllPkgTree.Namespaces[0].Names[0].Name, startID)
	if err != nil {
	return nil, err
	}

On line 87 we are also editing q.nodeMap[startID], but that can't be the issue because we are only assigning the Parents with []string{}.

Now we have to go into the function addNodesToQueueFromPackageName. Since q is empty we pretty much pass by all if statements and for loops until we get to:

Line 445:

guac/pkg/guacanalytics/patchPlanning.go

Lines 445 to 451 in bc5c042

	q.nodeMap[id] = BfsNode{
	Parents: []string{q.now},
	Depth: q.nowNode.Depth + 1,
	Type: PackageName,
	nodeVersions: versionsList,
	PointOfContact: q.nowNode.PointOfContact,
	}

We are setting Parents to []string{q.now}. Since we haven't set q.now it is empty resulting in an empty string being added to the Parents array.

Correct me if I am wrong, but I am pretty sure that this is the only place where Parents is being set to an empty string.

[rmetzman]

Yes, this makes sense to me, I think that must be causing the issue.

[nathannaveen]

Yes, this makes sense to me, I think that must be causing the issue.

Thank You!