Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: issue-5850 - Settle override conflicts between edges and propagate changes #7025

Open
wants to merge 29 commits into
base: latest
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions workspaces/arborist/lib/edge.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ class Edge {
const newTo = this.#from.resolve(this.#name)
if (newTo !== this.#to) {
if (this.#to) {
this.#to.edgesIn.delete(this)
this.#to.deleteEdgeIn(this)
}
this.#to = newTo
this.#error = null
Expand All @@ -273,7 +273,7 @@ class Edge {
detach () {
this.#explanation = null
if (this.#to) {
this.#to.edgesIn.delete(this)
this.#to.deleteEdgeIn(this)
}
this.#from.edgesOut.delete(this.#name)
this.#to = null
Expand Down
94 changes: 92 additions & 2 deletions workspaces/arborist/lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -1344,9 +1344,99 @@ class Node {
this.edgesOut.set(edge.name, edge)
}

addEdgeIn (edge) {
recalculateOutEdgesOverrides () {
// For each edge out propogate the new overrides through.
for (const [, edge] of this.edgesOut) {

This comment was marked as resolved.

edge.reload(true)
if (edge.to) {
edge.to.updateNodeOverrideSet(edge.overrides)
}
}
}

findSpecificOverrideSet (first, second) {

This comment was marked as resolved.

for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) {
if (overrideSet == first) {
return second
}
}
for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) {
if (overrideSet == second) {
return first
}
}
console.log("Conflicting override sets")
}

updateNodeOverrideSetDueToEdgeRemoval (otherOverrideSet) {
// If this edge's overrides isn't equal to this node's overrides, then removing it won't change newOverrideSet later.
if (this.overrides != otherOverrideSet) {
return false
}
let newOverrideSet
for (const edge of this.edgesIn) {
if (newOverrideSet) {
newOverrideSet = this.findSpecificOverrideSet(edge.overrides, newOverrideSet)
} else {
newOverrideSet = edge.overrides
}
}
if (this.overrides == newOverrideSet) {
return false
}
this.overrides = newOverrideSet
if (this.overrides) {
// Optimization: if there's any override set at all, then no non-extraneous node has an empty override set. So if we temporarily have no
// override set (for example, we removed all the edges in), there's no use updating all the edges out right now. Let's just wait until
// we have an actual override set later.
this.recalculateOutEdgesOverrides()
}
return true
}

// This logic isn't perfect either. When we have two edges in that have different override sets, then we have to decide which set is correct.
// This function assumes the more specific override set is applicable, so if we have dependencies A->B->C and A->C
// and an override set that specifies what happens for C under A->B, this will work even if the new A->C edge comes along and tries to change
// the override set.
// The strictly correct logic is not to allow two edges with different overrides to point to the same node, because even if this node can satisfy
// both, one of its dependencies might need to be different depending on the edge leading to it.
// However, this might cause a lot of duplication, because the conflict in the dependencies might never actually happen.
updateNodeOverrideSet (otherOverrideSet) {
if (this.overrides == otherOverrideSet) {
return false
}
if (!this.overrides) {
this.overrides = otherOverrideSet
this.recalculateOutEdgesOverrides()
return true
}
let newOverrideSet = this.findSpecificOverrideSet(this.overrides, otherOverrideSet)
if (!newOverrideSet) {
// This is an error condition. We can only get here if the new override set is in conflict with the existing.
return false
} else {
if (this.overrides != newOverrideSet) {
this.overrides = newOverrideSet
this.recalculateOutEdgesOverrides()
return true
}
return false
}
}

deleteEdgeIn (edge) {
this.edgesIn.delete(edge)
if (edge.overrides) {
this.overrides = edge.overrides
this.updateNodeOverrideSetDueToEdgeRemoval(edge.overrides)
}
}

addEdgeIn (edge) {
// We need to handle the case where the new edge in has an overrides field which is different from the current value.
// Assuming there are any overrides at all, the overrides field is never undefined for any node at the end state of the tree.
// So if the new edge's overrides is undefined it will be updated later. So we can wait with updating the node's overrides field.
if (edge.overrides && this.overrides != edge.overrides) {
wraithgar marked this conversation as resolved.
Show resolved Hide resolved
this.updateNodeOverrideSet(edge.overrides)
}

this.edgesIn.add(edge)
Expand Down