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

AlonNavon · 2023-11-26T13:37:13Z

A first step in fixing the overrides feature. In this PR I'm aiming to fix 3 bugs:

When we add an edge going into a node we update the node's overrides, but we don't update the overrides of that node's outgoing edges, and so forth. We need the up-to-date overrides to filter through.
When we remove an edge going into a node we don't update the overrides at all (and we don't update the outgoing edges like in the previous bug).
When we add an edge going in, and we already have a different override set for the node, we just ignore the existing override set and overwrite it with that of the new edge. Instead, this PR chooses the most specific override set. This still isn't the absolutely correct logic, since different override sets can have implications down the line of the dependency chain, but it has the advantage of being consistent (instead of just going with the last edge in). Also, it raises an error if it encounters a real conflict, meaning two incoming edges with override sets that aren't just a subset of one another.
So if we have dependency chains A->B->C and A->C, and we override C under B, then C will be overridden.

References

Fixes some of the override issues.

…e edges out

wraithgar · 2023-11-28T15:37:14Z

This is going to need tests

workspaces/arborist/lib/node.js

wraithgar · 2023-11-28T15:50:09Z

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

AlonNavon · 2023-11-28T16:27:34Z

The edge's reload seems to me where this kind of thing should be happening. Is this a more subtle error perhaps where we're not reloading the overrides where we should?

There are several bugs in the mechanism, and this is just the first fix.
Here I handle the case where a node has two incoming edges with different override sets, and I percolate it downwards in the dependency tree. So I'm not sure why it should be in the edge's reload.
If you @wraithgar have time I can hop on a Google meet to discuss (and explain the other bugs).

AlonNavon · 2023-12-07T17:59:43Z

@wraithgar
The linter and the tests are successful (except a random failure in macOS 18.0 which is unrelated to the PR).

AlonNavon · 2023-12-18T12:08:47Z

@wraithgar Anything else we need to do to merge this?

sgarfinkel · 2023-12-26T17:18:24Z

@AlonNavon This looks awesome, really looking forward to having overrides work more the way you'd expect. With this change, will running npm install re-apply any npm overrides as expected, or will running npm u still be required? Hopefully this gets approved soon 👍

AlonNavon · 2023-12-26T17:45:22Z

@sgarfinkel
Hey, this is just the first fix. Concretely, there are at least 5 identifiable bugs. It fixes (1) and (2) and gives a reasonable solution for (3). I have other short PRs planned for (4) and (5), but first I want to merge this one. With those merged the behavior should be stable and correct (up to some freaky edge cases regarding the conflict resolution).
But I haven't been able to get this one merged yet. Hopefully after the holidays it will get renewed attention. Every upvote counts.

The major bugs:

No percolation to the out-edges.
Not updating when deleting in-edges.
No conflict resolution of override sets (though ultimately, edges with conflicting overrides shouldn't be valid and be deduplicated IMHO).
A certain flow that updates to the parent's overrides which doesn't make sense.
Mishandling version ranges on edges.

wraithgar · 2024-01-02T15:27:37Z

Anything else we need to do to merge this?

Just patience. Holidays are over and we got a lot of PRs. This PR is on the work board it's not been forgotten.

sgarfinkel · 2024-03-14T16:39:00Z

Why was this closed?

ljharb · 2024-03-14T16:41:13Z

The PR wasn't made from the OP's fork, but presumably from their company's (which means that maintainers can't push to the PR branch, btw - always only make PRs from your own personal forks), and presumably someone at their company deleted the fork.

AlonNavon · 2024-03-14T16:54:48Z

Yeah, this was a mistake on our end. Our devops deleted the repo by accident.
We restored the repo, but we need some maintainer with permissions to reopen the PR.
@sgarfinkel @ljharb @wraithgar

workspaces/arborist/lib/override-set.js

workspaces/arborist/lib/edge.js

-    return depValid(node, this.spec, this.#accept, this.#from)
+
+    // If there's no override we just use the spec.
+    if (!this.overridden) {


AlonNavon · 2024-11-07T18:47:50Z

workspaces/arborist/lib/node.js

+    }
+  }
+
+  static findSpecificOverrideSet (first, second) {


Better to just move it override-set.js, since it fundamentally compares two override sets as is.

workspaces/arborist/lib/edge.js

AlonNavon · 2024-11-07T18:19:56Z

workspaces/arborist/lib/node.js

+        return first
+      }
+    }
+    log.silly('Conflicting override sets', this, first, second)


AlonNavon · 2024-11-07T18:22:48Z

workspaces/arborist/lib/edge.js

-    return depValid(node, this.spec, this.#accept, this.#from)
+
+    // If there's no override we just use the spec.
+    if (!this.overrides) {


AlonNavon · 2024-11-10T15:24:00Z

workspaces/arborist/lib/override-set.js

@@ -142,6 +181,24 @@ class OverrideSet {

    return ruleset
  }
+
+  static findSpecificOverrideSet (first, second) {


@jdalton I moved the functions here, because fundamentally they are about comparing override sets, so this is the most natural location for them I think.

Is there anything else, or do you approve the PR?

@AlonNavon
I think it looks great. Could you add code comments inline explaining the cases when the .silly is called (the one you changed from throwing to .silly). This will help folks if they see the logging know why.

Then the only other thing is adding any unit tests to cover some of the things you discovered and tweaked.

Hey @wraithgar, can you run the tests? To make sure nothing got accidentally broken with all the changes.

@jdalton For the override set functions I created unit tests. I'm not sure how to translate the test cases that I'm running to unit tests.
This is the package.json of the simplest case:
{ "name": "test", "version": "1.0.0", "engines": { "npm": ">=8.3.0" }, "dependencies": { "json-server": "0.17.0" }, "overrides": { "json-server": { "package-json": "7.0.0" } } }

Looks good to me. @wraithgar I think this is where we'll need your help to push it over the line. (one last test case and a review). 🎉

workspaces/arborist/lib/override-set.js

    log.silly('Conflicting override sets', first, second)
  }

  static doOverrideSetsConflict (first, second) {
+    // If override sets contain one another then we can try to use the more specific one.
+    // This is imperfect, since we may lose some of the context of the less specific override
+    // set. But the more specific override set is more important to propagate, so we go with it.


workspaces/arborist/test/override-set.js

AlonNavon added 3 commits November 23, 2023 22:13

Settle overrides conflicts between edges, and propagate changes to th…

865070c

…e edges out

cleanup

cd20196

Optimization

0cad4e0

AlonNavon requested a review from a team as a code owner November 26, 2023 13:37

AlonNavon changed the title ~~Settle overrides conflicts between edges, and propagate changes to the edges out~~ fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes to the edges out Nov 26, 2023

AlonNavon mentioned this pull request Nov 27, 2023

bug: overrides property only honored when running install the first time #5850

Open

wraithgar self-assigned this Nov 28, 2023

wraithgar reviewed Nov 28, 2023

View reviewed changes

workspaces/arborist/lib/node.js Outdated Show resolved Hide resolved

AlonNavon added 7 commits November 28, 2023 19:34

Fix override sets comparisons

6b276f3

Add comparator

fff3072

Check for undefined

e97176e

Another place

19155ed

Fix

dedbccf

Lint fixes

4a83786

Added tests

7ee2f2e

AlonNavon changed the title ~~fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes to the edges out~~ fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes Dec 7, 2023

AlonNavon changed the title ~~fix: issue-5850 - Settle overrides conflicts between edges, and propagate changes~~ fix: issue-5850 - Settle override conflicts between edges and propagate changes Dec 7, 2023

levpachmanov mentioned this pull request Feb 15, 2024

🚨 HIGH Severity Vulnerability: Package unsafe for use as of v1.1.8 🚨 indutny/node-ip#136

Open

ali-security closed this by deleting the head repository Mar 12, 2024

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Oct 30, 2024

Update OverrideSet#getEdgeRule method to align with npm/cli#7025

3c9e4c6

jdalton reviewed Oct 30, 2024

View reviewed changes

workspaces/arborist/lib/override-set.js Show resolved Hide resolved

AlonNavon added 3 commits November 4, 2024 16:28

Code review fixes

7d97726

Fix edge satisfaction logic

35e5790

Fix dedupe logic

35ce2f7

AlonNavon commented Nov 4, 2024

View reviewed changes

workspaces/arborist/lib/edge.js Show resolved Hide resolved

AlonNavon added 3 commits November 4, 2024 18:46

Code review fixes

5630954

Oops

f859e92

Static function

29f0750

jdalton reviewed Nov 5, 2024

View reviewed changes

workspaces/arborist/lib/edge.js Outdated

return depValid(node, this.spec, this.#accept, this.#from)

// If there's no override we just use the spec.

if (!this.overridden) {

This comment was marked as resolved.

Sign in to view

jdalton reviewed Nov 5, 2024

View reviewed changes

Typo

17ec457

jdalton reviewed Nov 5, 2024

View reviewed changes

workspaces/arborist/lib/edge.js Show resolved Hide resolved

jdalton reviewed Nov 5, 2024

View reviewed changes

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 6, 2024

Update npm fixes based on npm/cli#7025

d13ad4d

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 6, 2024

Update npm fixes based on npm/cli#7025

9f80f87

AlonNavon added 3 commits November 7, 2024 20:13

Shouldn't throw

4cde831

CR fixes

701b125

CR fixes

b119f67

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024

Update npm fixes based on npm/cli#7025

cd678bf

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024

Update npm fixes based on npm/cli#7025

a44b029

jdalton added a commit to SocketDev/socket-cli that referenced this pull request Nov 7, 2024

Update npm fixes based on npm/cli#7025

3e2ada3

Move functions

3136b64

AlonNavon commented Nov 10, 2024

View reviewed changes

AlonNavon added 2 commits November 11, 2024 17:47

Comments

80126c7

Override sets unit tests

c4b5338

jdalton reviewed Nov 11, 2024

View reviewed changes

workspaces/arborist/test/override-set.js Show resolved Hide resolved

Clarify comment

9db1232

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AlonNavon commented Nov 26, 2023

wraithgar commented Nov 28, 2023

wraithgar commented Nov 28, 2023

AlonNavon commented Nov 28, 2023

AlonNavon commented Dec 7, 2023

AlonNavon commented Dec 18, 2023

sgarfinkel commented Dec 26, 2023

AlonNavon commented Dec 26, 2023 •

edited

Loading

wraithgar commented Jan 2, 2024

sgarfinkel commented Mar 14, 2024

ljharb commented Mar 14, 2024 •

edited

Loading

AlonNavon commented Mar 14, 2024

This comment was marked as resolved.

This comment was marked as resolved.

AlonNavon Nov 7, 2024

This comment was marked as resolved.

This comment was marked as resolved.

AlonNavon Nov 7, 2024

This comment was marked as resolved.

This comment was marked as resolved.

AlonNavon Nov 7, 2024

AlonNavon Nov 10, 2024

AlonNavon Nov 11, 2024

jdalton Nov 11, 2024

AlonNavon Nov 11, 2024

AlonNavon Nov 11, 2024

jdalton Nov 12, 2024

This comment was marked as resolved.

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

Are you sure you want to change the base?

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

Conversation

AlonNavon commented Nov 26, 2023

References

wraithgar commented Nov 28, 2023

wraithgar commented Nov 28, 2023

AlonNavon commented Nov 28, 2023

AlonNavon commented Dec 7, 2023

AlonNavon commented Dec 18, 2023

sgarfinkel commented Dec 26, 2023

AlonNavon commented Dec 26, 2023 • edited Loading

wraithgar commented Jan 2, 2024

sgarfinkel commented Mar 14, 2024

ljharb commented Mar 14, 2024 • edited Loading

AlonNavon commented Mar 14, 2024

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

AlonNavon commented Dec 26, 2023 •

edited

Loading

ljharb commented Mar 14, 2024 •

edited

Loading