diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 242d2669ae4ca..32e523cbc83ca 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -276,9 +276,15 @@ class Edge { } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' } else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) { - // Any inconsistency between the edge's override set and the target's override set is potentially problematic. - // But we only say the edge is in error if the override sets are plainly conflicting. - // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant. + // Check for conflicts between the edge's override set and the target node's override set. + // This catches cases where different parts of the tree have genuinely incompatible + // version requirements for the same package. + // The improved conflict detection uses semantic comparison (checking for incompatible + // version ranges) rather than pure structural equality, avoiding false positives from: + // - Reference overrides ($syntax) that resolve to compatible versions + // - Peer dependencies with different but compatible override contexts + // Note: We only check if the target has dependencies (edgesOut.size > 0), since + // override conflicts are only relevant if the target has its own dependencies. this.#error = 'INVALID' } else { this.#error = 'OK' diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 3f05609bfacc1..b4a11ba589df7 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/workspaces/arborist/lib/override-set.js @@ -201,8 +201,82 @@ class OverrideSet { static doOverrideSetsConflict (first, second) { // If override sets contain one another then we can try to use the more specific one. - // If neither one is more specific, then we consider them to be in conflict. - return (this.findSpecificOverrideSet(first, second) === undefined) + // If neither one is more specific, check for semantic conflicts. + const specificSet = this.findSpecificOverrideSet(first, second) + if (specificSet !== undefined) { + // One contains the other, so no conflict + return false + } + + // The override sets are structurally incomparable, but this doesn't necessarily + // mean they conflict. We need to check if they have conflicting version requirements + // for any package that appears in both rulesets. + return this.haveConflictingRules(first, second) + } + + static haveConflictingRules (first, second) { + // Get all rules from both override sets + const firstRules = first.ruleset + const secondRules = second.ruleset + + // Check each package that appears in both rulesets + for (const [key, firstRule] of firstRules) { + const secondRule = secondRules.get(key) + if (!secondRule) { + // Package only appears in one ruleset, no conflict + continue + } + + // Same rule object means no conflict + if (firstRule === secondRule || firstRule.isEqual(secondRule)) { + continue + } + + // Both rulesets have rules for this package with different values. + // Check if the version requirements are actually incompatible. + const firstValue = firstRule.value + const secondValue = secondRule.value + + // If either value is a reference (starts with $), we can't determine + // compatibility here - the reference might resolve to compatible versions. + // We defer to runtime resolution rather than failing early. + if (firstValue.startsWith('$') || secondValue.startsWith('$')) { + continue + } + + // Check if the version ranges are compatible using semver + // If both specify version ranges, they conflict only if they have no overlap + try { + const firstSpec = npa(`${firstRule.name}@${firstValue}`) + const secondSpec = npa(`${secondRule.name}@${secondValue}`) + + // For range/version types, check if they intersect + if ((firstSpec.type === 'range' || firstSpec.type === 'version') && + (secondSpec.type === 'range' || secondSpec.type === 'version')) { + // Check if the ranges intersect + const firstRange = firstSpec.fetchSpec + const secondRange = secondSpec.fetchSpec + + // If the ranges don't intersect, we have a real conflict + if (!semver.intersects(firstRange, secondRange)) { + log.silly('Found conflicting override rules', { + package: firstRule.name, + first: firstValue, + second: secondValue, + }) + return true + } + } + // For other types (git, file, directory, tag), we can't easily determine + // compatibility, so we conservatively assume no conflict + } catch { + // If we can't parse the specs, conservatively assume no conflict + // Real conflicts will be caught during dependency resolution + } + } + + // No conflicting rules found + return false } } diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 85212e53a4a39..91fbfa22af721 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3277,48 +3277,212 @@ t.test('should propagate the new override set to the target node', t => { t.end() }) -t.test('should find inconsistency between the edge\'s override set and the target\'s override set', t => { - const tree = new Node({ - loadOverrides: true, - path: '/root', - pkg: { - name: 'root', - version: '1.0.0', - dependencies: { - mockDep: '1.x', +t.test('override conflict detection with semantic comparison', t => { + t.test('non-conflicting different override sets should be valid', t => { + // Regression test for issue #8688 + // This validates that the improved semantic conflict detection allows + // structurally different override sets that don't actually conflict. + + // Create two different override sets (simulating Vaadin's structure) + // These override different packages, so they don't conflict + const overridesComponents = new OverrideSet({ + overrides: { + '@vaadin/react-components': '24.9.2', }, + }) + + const overridesComponentsPro = new OverrideSet({ overrides: { - mockDep: '2.x', + '@vaadin/react-components-pro': '24.9.2', }, - }, - children: [{ - name: 'mockDep', - version: '2.0.0', + }) + + const tree = new Node({ + loadOverrides: true, + path: '/root', pkg: { + name: 'root', + version: '1.0.0', dependencies: { - subDep: '1.0.0', + mockDep: '1.x', + }, + overrides: { + mockDep: '2.x', }, }, children: [{ - name: 'subDep', - version: '1.0.0', - pkg: {}, + name: 'mockDep', + version: '2.0.0', + pkg: { + dependencies: { + subDep: '1.0.0', + }, + }, + children: [{ + name: 'subDep', + version: '1.0.0', + pkg: {}, + }], }], - }], + }) + + const edge = tree.edgesOut.get('mockDep') + + // Manually set an override to the edge + edge.overrides = overridesComponents + + // Override satisfiedBy so it returns true, ensuring the conflict branch is reached + edge.satisfiedBy = () => true + + // Set different but non-conflicting override on the target node + const mockDep = tree.children.get('mockDep') + mockDep.overrides = overridesComponentsPro + + // Force edge to recalculate + edge.reload(true) + + // The edge should be valid despite different override contexts + // because they don't have conflicting version requirements + t.equal(edge.error, null, 'Edge should be valid with non-conflicting override contexts') + t.ok(edge.valid, 'Edge.valid should be true') + + t.end() }) - // Force edge.override to a conflicting object so that it will differ from - // the computed override coming from the parent's override set. - const conflictingOverride = new OverrideSet({ - overrides: { mockDep: '1.x' }, + t.test('conflicting override sets should be detected as invalid', t => { + // This validates that actual conflicts ARE still caught by the semantic detection + + // Create two override sets with conflicting version requirements for the same package + const overridesV1 = new OverrideSet({ + overrides: { + lodash: '1.x', + }, + }) + + const overridesV4 = new OverrideSet({ + overrides: { + lodash: '4.x', + }, + }) + + const tree = new Node({ + loadOverrides: true, + path: '/root', + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { + mockDep: '1.x', + }, + overrides: { + mockDep: '2.x', + }, + }, + children: [{ + name: 'mockDep', + version: '2.0.0', + pkg: { + dependencies: { + lodash: '1.0.0', + }, + }, + children: [{ + name: 'lodash', + version: '1.0.0', + pkg: {}, + }], + }], + }) + + const edge = tree.edgesOut.get('mockDep') + const mockDep = tree.children.get('mockDep') + + // Manually set conflicting overrides + edge.overrides = overridesV1 + mockDep.overrides = overridesV4 + + // Override satisfiedBy so it returns true, ensuring the conflict branch is reached + edge.satisfiedBy = () => true + + // Clear the cached error by calling reload(true) + edge.reload(true) + + // Re-set the overrides after reload (since reload may have changed them) + edge.overrides = overridesV1 + mockDep.overrides = overridesV4 + + // The edge should be INVALID due to conflicting override requirements + t.equal(edge.error, 'INVALID', 'Edge should be invalid with conflicting override versions') + t.notOk(edge.valid, 'Edge.valid should be false') + + t.end() }) - const edge = tree.edgesOut.get('mockDep') - edge.overrides = conflictingOverride - // Override satisfiedBy so it returns true, ensuring the conflict branch is reached - edge.satisfiedBy = () => true + t.test('reference overrides should not cause false positives', t => { + // This validates that reference overrides ($syntax) don't trigger false positives + + const overridesRef1 = new OverrideSet({ + overrides: { + lodash: '$some-reference', + }, + }) - t.equal(tree.edgesOut.get('mockDep').error, 'INVALID', 'Edge should be marked INVALID due to conflicting overrides') + const overridesRef2 = new OverrideSet({ + overrides: { + lodash: '$another-reference', + }, + }) + + const tree = new Node({ + loadOverrides: true, + path: '/root', + pkg: { + name: 'root', + version: '1.0.0', + dependencies: { + mockDep: '1.x', + }, + overrides: { + mockDep: '2.x', + }, + }, + children: [{ + name: 'mockDep', + version: '2.0.0', + pkg: { + dependencies: { + lodash: '4.0.0', + }, + }, + children: [{ + name: 'lodash', + version: '4.0.0', + pkg: {}, + }], + }], + }) + + const edge = tree.edgesOut.get('mockDep') + + // Set reference overrides + edge.overrides = overridesRef1 + + // Override satisfiedBy so it returns true, ensuring the conflict branch is reached + edge.satisfiedBy = () => true + + const mockDep = tree.children.get('mockDep') + mockDep.overrides = overridesRef2 + + // Force edge to recalculate + edge.reload(true) + + // Reference overrides should not cause conflicts because we can't determine + // their compatibility at this stage - they might resolve to the same version + t.equal(edge.error, null, 'Edge should be valid with reference overrides') + t.ok(edge.valid, 'Edge.valid should be true with reference overrides') + + t.end() + }) t.end() }) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 6acd8c6eecf62..8b8be32c9bb81 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -378,56 +378,184 @@ t.test('constructor', async (t) => { t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides5), 'override sets are the same object') t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides6), 'one override set is the specific version of the other') t.ok(!OverrideSet.doOverrideSetsConflict(overrides6, overrides5), 'one override set is the specific version of the other') - t.ok(OverrideSet.doOverrideSetsConflict(overrides5, overrides7), 'no override set is the specific version of the other') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides5), 'no override set is the specific version of the other') + // With semantic conflict detection, overrides5 and overrides7 don't conflict because + // they have the same value for 'bat' (2.0.0). Structurally they're incomparable, + // but semantically they're compatible. + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides7), 'structurally incomparable but semantically compatible') + t.ok(!OverrideSet.doOverrideSetsConflict(overrides7, overrides5), 'structurally incomparable but semantically compatible') t.ok(!overrides7.isEqual(overrides8), 'two override sets that differ in the version are not equal') t.ok(!overrides8.isEqual(overrides9), 'two override sets that differ in the range are not equal') t.ok(!overrides7.isEqual(overrides9), 'two override sets that differ in both version and range are not equal') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets are incomparable due to version') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets are incomparable due to version and range') - t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets are incomparable due to range') + // overrides7 (bat: 2.0.0) and overrides8 (bat: 1.2.0) have conflicting versions for the same package + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets have conflicting versions for bat') + // overrides7 (bat: 2.0.0) and overrides9 (bat@3.0.0: 1.2.0) don't directly conflict because + // overrides9 only applies to bat@3.x, not bat@2.x + t.ok(!OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets apply to different version ranges') + // overrides8 (bat: 1.2.0) and overrides9 (bat@3.0.0: 1.2.0) have same target version + t.ok(!OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets have compatible target versions') }) -}) -t.test('coverage for final line in isEqual (parent != null)', async t => { - // Both parents have the SAME config -> parent.isEqual(...) will return TRUE - const parentA = new OverrideSet({ overrides: { foo: '1.0.0' } }) - const parentB = new OverrideSet({ overrides: { foo: '1.0.0' } }) + t.test('semantic conflict detection (haveConflictingRules)', async (t) => { + t.test('no conflict when packages are different', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + foo: '1.0.0', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + bar: '1.0.0', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'different packages should not conflict') + }) - // Child override sets with the same parent config => should be equal - const childA = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentA, - }) - const childB = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentB, - }) + t.test('no conflict when version ranges intersect', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '^4.0.0', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.0', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'intersecting ranges should not conflict') + }) - // This specifically covers the code path where parent != null - // AND parent.isEqual(...) returns true - t.ok(childA.isEqual(childB), 'two children with equivalent parents are equal') + t.test('conflict when version ranges do not intersect', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '1.x', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.x', + }, + }) + t.ok(OverrideSet.haveConflictingRules(overrides1, overrides2), 'non-intersecting ranges should conflict') + }) - // Different parent configs -> parent.isEqual(...) will return FALSE - const parentC = new OverrideSet({ overrides: { foo: '1.0.0' } }) - const parentD = new OverrideSet({ overrides: { foo: '1.0.1' } }) + t.test('no conflict when using reference overrides', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '$ref1', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '$ref2', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'reference overrides should not conflict') + }) - const childC = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentC, + t.test('no conflict when one is a reference override', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '$ref1', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'reference override with version should not conflict') + }) + + t.test('no conflict when rules are equal', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'equal rules should not conflict') + }) + + t.test('handles non-semver spec types gracefully', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + foo: 'github:user/repo', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + foo: 'file:./local', + }, + }) + // Non-semver types should not be considered conflicting + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'non-semver specs should not conflict') + }) + + t.test('regression test for issue #8688 - Vaadin case', async (t) => { + const overridesComponents = new OverrideSet({ + overrides: { + '@vaadin/react-components': '24.9.2', + }, + }) + const overridesComponentsPro = new OverrideSet({ + overrides: { + '@vaadin/react-components-pro': '24.9.2', + }, + }) + t.ok(!OverrideSet.doOverrideSetsConflict(overridesComponents, overridesComponentsPro), 'Vaadin components should not conflict') + }) }) - const childD = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentD, +}) + +t.test('coverage for isEqual edge cases', async t => { + t.test('isEqual with null/undefined other', async t => { + const overrides = new OverrideSet({ overrides: { foo: '1.0.0' } }) + t.ok(!overrides.isEqual(null), 'override set is not equal to null') + t.ok(!overrides.isEqual(undefined), 'override set is not equal to undefined') }) - // This specifically covers the code path where parent != null - // AND parent.isEqual(...) returns false - t.notOk(childC.isEqual(childD), 'two children with different parents are not equal') + t.test('isEqual when parent != null', async t => { + // Both parents have the SAME config -> parent.isEqual(...) will return TRUE + const parentA = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentB = new OverrideSet({ overrides: { foo: '1.0.0' } }) - t.end() + // Child override sets with the same parent config => should be equal + const childA = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentA, + }) + const childB = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentB, + }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns true + t.ok(childA.isEqual(childB), 'two children with equivalent parents are equal') + + // Different parent configs -> parent.isEqual(...) will return FALSE + const parentC = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentD = new OverrideSet({ overrides: { foo: '1.0.1' } }) + + const childC = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentC, + }) + const childD = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentD, + }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns false + t.notOk(childC.isEqual(childD), 'two children with different parents are not equal') + }) })