slawekjaranowski commented on PR #1025: URL: https://github.com/apache/maven-enforcer/pull/1025#issuecomment-5671222005
Thanks for digging into this! Before we go further with the current approach — I think the root cause sits one level deeper, and this is actually a regression. `BannedDependenciesBase#execute()` resolves the tree with: ```java DependencyNode rootNode = resolverUtil.resolveTransitiveDependenciesVerbose(Collections.emptyList()); ``` and `ResolverUtil#resolveTransitiveDependenciesVerbose(List)` hardcodes `excludeOptional = true`, so optional dependencies are filtered out of the `CollectRequest` before the tree is even built. The rule never gets a chance to see them. That filtering was introduced in https://github.com/apache/maven-enforcer/pull/256 (commit 5e0ca5a, first released in 3.3.0). Before that change `BannedDependenciesBase` called `resolveTransitiveDependenciesVerbose()` with no selectors, i.e. with the session defaults — and the default `OptionalDependencySelector` only drops optional dependencies from depth 2 on: ```java public boolean selectDependency(Dependency dependency) { return depth < 2 || !dependency.isOptional(); } ``` so direct optional dependencies were included. `DependencyConvergence` and `RequireUpperBoundDeps` previously used `AllLevelsOptionalDependencySelector`, so for them `excludeOptional = true` is correct and should stay — only `BannedDependenciesBase` lost behaviour it used to have. So the fix should live in `BannedDependenciesBase`, not in `RequireReleaseDeps`: ```java DependencyNode rootNode = resolverUtil.resolveTransitiveDependencies(true, false, Collections.emptyList()); ``` (or, for readability, a new `resolveTransitiveDependenciesVerbose(boolean excludeOptional, List<String> excludedScopes)` overload in `ResolverUtil`). Advantages over the current patch: - optional dependencies go through the normal tree walk, so the failure message keeps the usual tree format: ``` org.apache.maven.enforcer.its:requireReleaseDepsOptional:jar:1.0-SNAPSHOT org.apache.maven.plugins.enforcer.its:menforcer85_api:jar:1.0-SNAPSHOT <--- is not a release dependency ``` which is exactly what the reporter expected in MENFORCER-517 - transitive dependencies *of* an optional dependency are covered too, not only the direct ones - `includes`/`excludes` keep working through the existing code path - `bannedDependencies` — same base class, same blind spot — is fixed at the same time - no manual `DefaultArtifact` construction, so no need to reimplement the `ArtifactTypeRegistry` mapping (`test-jar` → classifier `tests`, `pom`/`bundle` extensions, …) I tried it locally: your IT `require-release-dependencies-optional_failure` passes with that one-line change, and `require-release-dependencies*`, `multimodule-require-release*`, `banned-dependencies*` and `ban-transitive*` all stay green. The only thing that needs touching is the Mockito stubs in `RequireReleaseDepsTest` and `BannedDependenciesTest`, which currently stub `resolveTransitiveDependenciesVerbose(anyList())`. Could you rework the PR along these lines? Please also: - prefix the commit message and PR title with `[MENFORCER-517]` - rebase on current `master` - mention in the description that this is a regression since 3.3.0 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
