Fix inclusion/exclusion inconsistency #342
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes it consistent, if we decide a root artefact is allowed by the include/exclude list we use the same logic to decide if we should process it as a node. Note this changes the behaviour, an inclusion rule will beat an exclusion rule. Signed-off-by: Robert Young <robeyoun@redhat.com>
robobario
commented
Aug 27, 2024
| } else { | ||
| collect = !isExcluded(d) && isIncluded(d); | ||
| collect = !isExplicitlyExcluded(d) && isExplicitlyIncluded(d); |
There was a problem hiding this comment.
why do we check if the projectBomConstraint is explicitly included in this case? When discovering top level artefacts we check if they are included or not excluded.
robobario
commented
Aug 27, 2024
| @@ -752,7 +763,7 @@ private void processRootArtifact(ArtifactCoords rootArtifact) { | |||
| } | |||
| for (DependencyNode d : root.getChildren()) { | |||
| if (d.getDependency().isOptional() | |||
| && !(config.isIncludeOptionalDeps() || isIncluded(toCoords(d.getArtifact())))) { | |||
| && !(config.isIncludeOptionalDeps() || isExplicitlyIncluded(toCoords(d.getArtifact())))) { | |||
There was a problem hiding this comment.
why do we only consult the include list when deciding if we should recurse into optional deps?
robobario
commented
Aug 27, 2024
| @@ -1247,7 +1258,7 @@ private void processNodes(DependencyNode node, int level, boolean remaining) { | |||
| } | |||
|
|
|||
| final ArtifactCoords coords = toCoords(node.getArtifact()); | |||
| if (isExcluded(coords)) { | |||
| if (!isIncluded(coords)) { | |||
There was a problem hiding this comment.
this is the actual logic change, checking if the coords are in the inclusion list or not in the exclusion list.
|
Alternatively if exclusion should beat inclusion (hard to make everyone happy 😁), we should add that to the javadoc and put tests on it. |
robobario
commented
Aug 27, 2024
| @@ -1238,7 +1249,7 @@ private void processNodes(DependencyNode node, int level, boolean remaining) { | |||
| if (winner != null) { | |||
| final ArtifactCoords coords = toCoords(winner.getArtifact()); | |||
| // linked dependencies should also be checked for exclusions | |||
| if (!isExcluded(coords)) { | |||
| if (!isExplicitlyExcluded(coords)) { | |||
There was a problem hiding this comment.
should the inclusion list be considered here too?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #320
If we decide a root artefact is allowed by the include/exclude list we use the same logic to decide if we should process it as a node.
Note this changes the behaviour, an inclusion rule will beat an exclusion rule.
Context
We are using ProjectDependencyResolver to discover project dependencies and want to be able to configure it like:
However in this configuration
org.group:specific:*is not resolved as the exclusion takes precedence. Then looking into the code we see some usages where inclusion beats exclusion and wonder what the intent of the inclusion/exclusion lists are.This is more the start of a conversation as there are numerous places where the include/exclude list are consulted. For example when collecting projectBomConstraints it has different logic again:
where it applies this groupId restriction if there are no explicit inclusions else it requires it to be explicitly included.
There are other places in the resolver where we only check the exclusion list or inclusion list.
The javadoc on ProjectDependencyConfig is ambiguous around how
getIncludePatternsandgetExcludePatternsinteract and how they relate togetIncludeArtifacts.It feels like there should be some consistent way that the inclusion/exclusion rules are applied to dependencies.