gnodet commented on code in PR #13410: URL: https://github.com/apache/maven/pull/13410#discussion_r4242386059
########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; + } + } + + private final ExclusionDependencySelector delegate; + private final int depth; + private final ExclusionTrackingRecord trackingRecord; + + public TrackingExclusionDependencySelector() { + this(new ExclusionDependencySelector(), 0, null); + } + + public TrackingExclusionDependencySelector(Collection<Exclusion> exclusions) { + this(new ExclusionDependencySelector(exclusions), 0, null); + } + + private TrackingExclusionDependencySelector( + ExclusionDependencySelector delegate, int depth, ExclusionTrackingRecord trackingRecord) { + this.delegate = Objects.requireNonNull(delegate, "delegate cannot be null"); + this.depth = depth; + this.trackingRecord = trackingRecord; + } + + @Override + public boolean selectDependency(Dependency dependency) { + if (!delegate.selectDependency(dependency)) { + if (trackingRecord != null && dependency != null) { + Artifact artifact = dependency.getArtifact(); + for (Exclusion exclusion : trackingRecord.getDeclaredExclusions()) { + if (matches(exclusion, artifact)) { + trackingRecord.getMatchedExclusions().add(exclusion); + } + } + } + return false; + } + return true; + } + + private static boolean matches(Exclusion exclusion, Artifact artifact) { + if (artifact == null) { + return false; + } + return matches(exclusion.getGroupId(), artifact.getGroupId()) + && matches(exclusion.getArtifactId(), artifact.getArtifactId()) + && matches(exclusion.getExtension(), artifact.getExtension()) + && matches(exclusion.getClassifier(), artifact.getClassifier()); + } + + private static boolean matches(String coordinate, String value) { + if ("*".equals(coordinate) || Objects.equals(coordinate, value)) { + return true; + } + if (coordinate != null && value != null && (coordinate.contains("*") || coordinate.contains("?"))) { + return matchesGlob(coordinate, value); + } + return false; + } + + private static boolean matchesGlob(String pattern, String text) { + StringBuilder sb = new StringBuilder("^"); + for (int i = 0; i < pattern.length(); i++) { + char c = pattern.charAt(i); + if (c == '*') { + sb.append(".*"); + } else if (c == '?') { + sb.append("."); + } else if ("\\.[]{}()+-^$|".indexOf(c) != -1) { + sb.append('\\').append(c); + } else { + sb.append(c); + } + } + sb.append('$'); + return text.matches(sb.toString()); + } + + @Override + public DependencySelector deriveChildSelector(DependencyCollectionContext context) { + ExclusionDependencySelector childDelegate = (ExclusionDependencySelector) delegate.deriveChildSelector(context); + + Dependency dependency = context.getDependency(); + ExclusionTrackingRecord childRecord = this.trackingRecord; + + if (depth == 0) { + if (dependency != null + && dependency.getExclusions() != null + && !dependency.getExclusions().isEmpty()) { + childRecord = new ExclusionTrackingRecord(dependency, dependency.getExclusions()); + registerTrackingRecord(context.getSession(), context.getArtifact(), childRecord); + } else { + childRecord = null; + } + } + + return new TrackingExclusionDependencySelector(childDelegate, depth + 1, childRecord); + } + + @SuppressWarnings("unchecked") + private static void registerTrackingRecord( + RepositorySystemSession session, Artifact rootArtifact, ExclusionTrackingRecord record) { + if (session == null || session.getData() == null) { + return; + } + String key = rootArtifact != null ? rootArtifact.getGroupId() + ":" + rootArtifact.getArtifactId() : ""; + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + map = new ConcurrentHashMap<>(); + session.getData().set(EXCLUSION_TRACKER_KEY, map); + } + map.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(record); + } + + @SuppressWarnings("unchecked") + public static List<ExclusionTrackingRecord> getTrackingRecords(RepositorySystemSession session, String projectKey) { + if (session == null || session.getData() == null) { + return Collections.emptyList(); + } + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + return Collections.emptyList(); + } + if (projectKey == null) { Review Comment: ⚠️ **TOCTOU race on map initialization** — `get()` → null-check → `set()` is not atomic. Two concurrent threads can both observe `null` and both call `session.getData().set()`, with the second overwriting the first's map. All records added by the first thread are silently lost. Use `session.getData().computeIfAbsent()` if available, or synchronize the initialization block. ########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; + } + } + + private final ExclusionDependencySelector delegate; + private final int depth; + private final ExclusionTrackingRecord trackingRecord; + + public TrackingExclusionDependencySelector() { + this(new ExclusionDependencySelector(), 0, null); + } + + public TrackingExclusionDependencySelector(Collection<Exclusion> exclusions) { + this(new ExclusionDependencySelector(exclusions), 0, null); + } + + private TrackingExclusionDependencySelector( + ExclusionDependencySelector delegate, int depth, ExclusionTrackingRecord trackingRecord) { + this.delegate = Objects.requireNonNull(delegate, "delegate cannot be null"); + this.depth = depth; + this.trackingRecord = trackingRecord; + } + + @Override + public boolean selectDependency(Dependency dependency) { + if (!delegate.selectDependency(dependency)) { + if (trackingRecord != null && dependency != null) { + Artifact artifact = dependency.getArtifact(); + for (Exclusion exclusion : trackingRecord.getDeclaredExclusions()) { + if (matches(exclusion, artifact)) { + trackingRecord.getMatchedExclusions().add(exclusion); + } + } + } + return false; + } + return true; + } + + private static boolean matches(Exclusion exclusion, Artifact artifact) { + if (artifact == null) { + return false; + } + return matches(exclusion.getGroupId(), artifact.getGroupId()) + && matches(exclusion.getArtifactId(), artifact.getArtifactId()) + && matches(exclusion.getExtension(), artifact.getExtension()) + && matches(exclusion.getClassifier(), artifact.getClassifier()); + } + + private static boolean matches(String coordinate, String value) { + if ("*".equals(coordinate) || Objects.equals(coordinate, value)) { + return true; + } + if (coordinate != null && value != null && (coordinate.contains("*") || coordinate.contains("?"))) { + return matchesGlob(coordinate, value); + } + return false; + } + + private static boolean matchesGlob(String pattern, String text) { + StringBuilder sb = new StringBuilder("^"); + for (int i = 0; i < pattern.length(); i++) { + char c = pattern.charAt(i); + if (c == '*') { + sb.append(".*"); + } else if (c == '?') { + sb.append("."); + } else if ("\\.[]{}()+-^$|".indexOf(c) != -1) { + sb.append('\\').append(c); + } else { + sb.append(c); + } + } + sb.append('$'); + return text.matches(sb.toString()); + } + + @Override + public DependencySelector deriveChildSelector(DependencyCollectionContext context) { + ExclusionDependencySelector childDelegate = (ExclusionDependencySelector) delegate.deriveChildSelector(context); + + Dependency dependency = context.getDependency(); + ExclusionTrackingRecord childRecord = this.trackingRecord; + + if (depth == 0) { + if (dependency != null + && dependency.getExclusions() != null + && !dependency.getExclusions().isEmpty()) { + childRecord = new ExclusionTrackingRecord(dependency, dependency.getExclusions()); + registerTrackingRecord(context.getSession(), context.getArtifact(), childRecord); + } else { + childRecord = null; + } + } + + return new TrackingExclusionDependencySelector(childDelegate, depth + 1, childRecord); + } + + @SuppressWarnings("unchecked") + private static void registerTrackingRecord( + RepositorySystemSession session, Artifact rootArtifact, ExclusionTrackingRecord record) { + if (session == null || session.getData() == null) { + return; + } + String key = rootArtifact != null ? rootArtifact.getGroupId() + ":" + rootArtifact.getArtifactId() : ""; + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + map = new ConcurrentHashMap<>(); + session.getData().set(EXCLUSION_TRACKER_KEY, map); + } + map.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(record); + } + + @SuppressWarnings("unchecked") + public static List<ExclusionTrackingRecord> getTrackingRecords(RepositorySystemSession session, String projectKey) { + if (session == null || session.getData() == null) { + return Collections.emptyList(); + } + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + return Collections.emptyList(); + } + if (projectKey == null) { + List<ExclusionTrackingRecord> all = new ArrayList<>(); + for (List<ExclusionTrackingRecord> list : map.values()) { + all.addAll(list); + } + map.clear(); + return all; + } + List<ExclusionTrackingRecord> records = map.remove(projectKey); + if (records == null) { + records = map.remove(""); + } + return records != null ? records : Collections.emptyList(); + } + + public static void clearTrackingRecords(RepositorySystemSession session) { + if (session != null && session.getData() != null) { + session.getData().set(EXCLUSION_TRACKER_KEY, null); + } + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + TrackingExclusionDependencySelector that = (TrackingExclusionDependencySelector) o; + return Objects.equals(delegate, that.delegate) && Objects.equals(trackingRecord, that.trackingRecord); + } Review Comment: ⚠️ **`ExclusionTrackingRecord` lacks `equals()`/`hashCode()`** — `Objects.equals(trackingRecord, that.trackingRecord)` falls through to identity comparison (`Object.equals()`) since `ExclusionTrackingRecord` does not override it. Two `ExclusionTrackingRecord` instances with identical fields will not compare equal, making `TrackingExclusionDependencySelector.equals()` incorrect for structural equality checks. Either implement `equals()`/`hashCode()` on `ExclusionTrackingRecord`, or document that selector equality only holds for the same record instance (identity semantics). ########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; + } + } + + private final ExclusionDependencySelector delegate; + private final int depth; + private final ExclusionTrackingRecord trackingRecord; + + public TrackingExclusionDependencySelector() { + this(new ExclusionDependencySelector(), 0, null); + } + + public TrackingExclusionDependencySelector(Collection<Exclusion> exclusions) { + this(new ExclusionDependencySelector(exclusions), 0, null); + } + + private TrackingExclusionDependencySelector( + ExclusionDependencySelector delegate, int depth, ExclusionTrackingRecord trackingRecord) { + this.delegate = Objects.requireNonNull(delegate, "delegate cannot be null"); + this.depth = depth; + this.trackingRecord = trackingRecord; + } + + @Override + public boolean selectDependency(Dependency dependency) { + if (!delegate.selectDependency(dependency)) { + if (trackingRecord != null && dependency != null) { + Artifact artifact = dependency.getArtifact(); + for (Exclusion exclusion : trackingRecord.getDeclaredExclusions()) { + if (matches(exclusion, artifact)) { + trackingRecord.getMatchedExclusions().add(exclusion); + } + } + } + return false; + } + return true; + } + + private static boolean matches(Exclusion exclusion, Artifact artifact) { + if (artifact == null) { + return false; + } + return matches(exclusion.getGroupId(), artifact.getGroupId()) + && matches(exclusion.getArtifactId(), artifact.getArtifactId()) + && matches(exclusion.getExtension(), artifact.getExtension()) + && matches(exclusion.getClassifier(), artifact.getClassifier()); + } + + private static boolean matches(String coordinate, String value) { + if ("*".equals(coordinate) || Objects.equals(coordinate, value)) { + return true; + } + if (coordinate != null && value != null && (coordinate.contains("*") || coordinate.contains("?"))) { + return matchesGlob(coordinate, value); + } + return false; + } + + private static boolean matchesGlob(String pattern, String text) { + StringBuilder sb = new StringBuilder("^"); + for (int i = 0; i < pattern.length(); i++) { + char c = pattern.charAt(i); + if (c == '*') { + sb.append(".*"); + } else if (c == '?') { + sb.append("."); + } else if ("\\.[]{}()+-^$|".indexOf(c) != -1) { + sb.append('\\').append(c); + } else { + sb.append(c); + } + } + sb.append('$'); + return text.matches(sb.toString()); + } + + @Override + public DependencySelector deriveChildSelector(DependencyCollectionContext context) { + ExclusionDependencySelector childDelegate = (ExclusionDependencySelector) delegate.deriveChildSelector(context); + + Dependency dependency = context.getDependency(); + ExclusionTrackingRecord childRecord = this.trackingRecord; + + if (depth == 0) { + if (dependency != null + && dependency.getExclusions() != null + && !dependency.getExclusions().isEmpty()) { + childRecord = new ExclusionTrackingRecord(dependency, dependency.getExclusions()); + registerTrackingRecord(context.getSession(), context.getArtifact(), childRecord); + } else { + childRecord = null; + } + } + + return new TrackingExclusionDependencySelector(childDelegate, depth + 1, childRecord); + } + + @SuppressWarnings("unchecked") + private static void registerTrackingRecord( + RepositorySystemSession session, Artifact rootArtifact, ExclusionTrackingRecord record) { + if (session == null || session.getData() == null) { + return; + } + String key = rootArtifact != null ? rootArtifact.getGroupId() + ":" + rootArtifact.getArtifactId() : ""; + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + map = new ConcurrentHashMap<>(); + session.getData().set(EXCLUSION_TRACKER_KEY, map); + } + map.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(record); + } + + @SuppressWarnings("unchecked") + public static List<ExclusionTrackingRecord> getTrackingRecords(RepositorySystemSession session, String projectKey) { + if (session == null || session.getData() == null) { + return Collections.emptyList(); + } + Map<String, List<ExclusionTrackingRecord>> map = + (Map<String, List<ExclusionTrackingRecord>>) session.getData().get(EXCLUSION_TRACKER_KEY); + if (map == null) { + return Collections.emptyList(); + } + if (projectKey == null) { + List<ExclusionTrackingRecord> all = new ArrayList<>(); + for (List<ExclusionTrackingRecord> list : map.values()) { + all.addAll(list); + } + map.clear(); + return all; + } + List<ExclusionTrackingRecord> records = map.remove(projectKey); + if (records == null) { + records = map.remove(""); + } Review Comment: 🚨 **`getTrackingRecords()` is destructive** — `map.remove()` at line 199/201 and `map.clear()` at line 196 consume the records on first call. Both `DefaultProjectDependenciesResolver.warnObsoleteExclusions` and `DefaultDependencyResolver.warnObsoleteExclusions` call this method. Whichever runs second gets an empty list and silently produces no warnings. Consider using `map.get()` (non-destructive) and adding an explicit `clearTrackingRecords()` call after both consumers have finished, or restructure so only one consumer calls this method. ########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; + } + } + + private final ExclusionDependencySelector delegate; + private final int depth; + private final ExclusionTrackingRecord trackingRecord; + + public TrackingExclusionDependencySelector() { + this(new ExclusionDependencySelector(), 0, null); + } + + public TrackingExclusionDependencySelector(Collection<Exclusion> exclusions) { + this(new ExclusionDependencySelector(exclusions), 0, null); + } + + private TrackingExclusionDependencySelector( + ExclusionDependencySelector delegate, int depth, ExclusionTrackingRecord trackingRecord) { + this.delegate = Objects.requireNonNull(delegate, "delegate cannot be null"); + this.depth = depth; + this.trackingRecord = trackingRecord; + } + + @Override + public boolean selectDependency(Dependency dependency) { + if (!delegate.selectDependency(dependency)) { + if (trackingRecord != null && dependency != null) { + Artifact artifact = dependency.getArtifact(); + for (Exclusion exclusion : trackingRecord.getDeclaredExclusions()) { + if (matches(exclusion, artifact)) { + trackingRecord.getMatchedExclusions().add(exclusion); + } + } + } + return false; + } + return true; + } + + private static boolean matches(Exclusion exclusion, Artifact artifact) { + if (artifact == null) { + return false; + } + return matches(exclusion.getGroupId(), artifact.getGroupId()) + && matches(exclusion.getArtifactId(), artifact.getArtifactId()) + && matches(exclusion.getExtension(), artifact.getExtension()) + && matches(exclusion.getClassifier(), artifact.getClassifier()); + } + + private static boolean matches(String coordinate, String value) { + if ("*".equals(coordinate) || Objects.equals(coordinate, value)) { + return true; + } + if (coordinate != null && value != null && (coordinate.contains("*") || coordinate.contains("?"))) { + return matchesGlob(coordinate, value); + } + return false; + } + + private static boolean matchesGlob(String pattern, String text) { + StringBuilder sb = new StringBuilder("^"); + for (int i = 0; i < pattern.length(); i++) { + char c = pattern.charAt(i); + if (c == '*') { Review Comment: ⚠️ **Tracker `matches()` diverges from `ExclusionDependencySelector.matches()`** — `ExclusionDependencySelector` only supports `*` as a full-field wildcard: `"*".equals(pattern) || pattern.equals(value)`. This tracker adds glob patterns (`com.*`, `foo?bar`) that the delegate never applies. If an exclusion uses a glob pattern, the tracker may mark it as "matched" when the delegate never excluded any artifact on its behalf, producing false negatives (missing obsolete-exclusion warnings). The tracking logic must mirror the delegate's matching logic exactly. ########## impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java: ########## @@ -194,6 +200,36 @@ public List<Node> flatten(@Nonnull Session s, @Nonnull Node node, @Nullable Path return map(dependencies, session::getNode); } + private void warnObsoleteExclusions(RepositorySystemSession session, DependencyResolverRequest request) { + if (!LOGGER.isWarnEnabled()) { + return; + } + String projectKey = request.getProject() + .map(p -> p.getGroupId() + ":" + p.getArtifactId()) + .orElse(null); + String projectId = request.getProject().map(Project::getId).orElse(projectKey); + List<TrackingExclusionDependencySelector.ExclusionTrackingRecord> records = + TrackingExclusionDependencySelector.getTrackingRecords(session, projectKey); + for (TrackingExclusionDependencySelector.ExclusionTrackingRecord record : records) { + org.eclipse.aether.graph.Dependency directDep = record.getDirectDependency(); + String depCoord = directDep.getArtifact().getGroupId() + ":" + + directDep.getArtifact().getArtifactId(); + for (org.eclipse.aether.graph.Exclusion exclusion : record.getDeclaredExclusions()) { + if ("*".equals(exclusion.getGroupId()) && "*".equals(exclusion.getArtifactId())) { + continue; + } + if (!record.getMatchedExclusions().contains(exclusion)) { + LOGGER.warn( + "The exclusion of {}:{} for {} in {} is obsolete - there is no dependency on this exclusion", + exclusion.getGroupId(), + exclusion.getArtifactId(), + depCoord, + projectId != null ? projectId : "project"); + } + } + } + } + private static DependencyFilter getScopeDependencyFilter(PathScope scope) { if (scope == null) { return null; Review Comment: 💡 **Duplicate warning logic** — near-identical `warnObsoleteExclusions` logic is copy-pasted into both `DefaultDependencyResolver` and `DefaultProjectDependenciesResolver`. Consider moving the shared logic into `TrackingExclusionDependencySelector` as a static helper: ```java public static void warnObsoleteExclusions(RepositorySystemSession session, String projectKey, String projectId, Logger logger) { ... } ``` This keeps the tracking logic co-located with the tracking data and avoids drift between the two consumers. ########## impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java: ########## @@ -184,6 +186,7 @@ public DependencyResolutionResult resolve(DependencyResolutionRequest request) + (message != null ? ": " + message : "")); } } + warnObsoleteExclusions(session, project); Review Comment: ⚠️ **Behavior change in compat layer** — `impl/maven-core` is the Maven 3.x compatibility layer. Adding a new `WARN`-level log message here changes observable behavior for Maven 3.x users (builds that fail on warnings, log parsers, CI pipelines). This should either: - Live only in `impl/maven-impl` (the Maven 4 API implementation), or - Be gated on a Maven 4 session flag so the compat path is unaffected. ########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; + } + } + + private final ExclusionDependencySelector delegate; + private final int depth; + private final ExclusionTrackingRecord trackingRecord; + + public TrackingExclusionDependencySelector() { + this(new ExclusionDependencySelector(), 0, null); + } + + public TrackingExclusionDependencySelector(Collection<Exclusion> exclusions) { + this(new ExclusionDependencySelector(exclusions), 0, null); + } + + private TrackingExclusionDependencySelector( + ExclusionDependencySelector delegate, int depth, ExclusionTrackingRecord trackingRecord) { + this.delegate = Objects.requireNonNull(delegate, "delegate cannot be null"); + this.depth = depth; + this.trackingRecord = trackingRecord; + } + + @Override + public boolean selectDependency(Dependency dependency) { + if (!delegate.selectDependency(dependency)) { + if (trackingRecord != null && dependency != null) { + Artifact artifact = dependency.getArtifact(); + for (Exclusion exclusion : trackingRecord.getDeclaredExclusions()) { + if (matches(exclusion, artifact)) { + trackingRecord.getMatchedExclusions().add(exclusion); + } + } + } + return false; + } + return true; + } + + private static boolean matches(Exclusion exclusion, Artifact artifact) { + if (artifact == null) { + return false; + } + return matches(exclusion.getGroupId(), artifact.getGroupId()) + && matches(exclusion.getArtifactId(), artifact.getArtifactId()) + && matches(exclusion.getExtension(), artifact.getExtension()) + && matches(exclusion.getClassifier(), artifact.getClassifier()); + } + + private static boolean matches(String coordinate, String value) { + if ("*".equals(coordinate) || Objects.equals(coordinate, value)) { + return true; + } + if (coordinate != null && value != null && (coordinate.contains("*") || coordinate.contains("?"))) { + return matchesGlob(coordinate, value); + } + return false; + } + + private static boolean matchesGlob(String pattern, String text) { + StringBuilder sb = new StringBuilder("^"); + for (int i = 0; i < pattern.length(); i++) { + char c = pattern.charAt(i); + if (c == '*') { + sb.append(".*"); + } else if (c == '?') { + sb.append("."); + } else if ("\\.[]{}()+-^$|".indexOf(c) != -1) { + sb.append('\\').append(c); + } else { + sb.append(c); + } + } + sb.append('$'); + return text.matches(sb.toString()); + } + + @Override + public DependencySelector deriveChildSelector(DependencyCollectionContext context) { + ExclusionDependencySelector childDelegate = (ExclusionDependencySelector) delegate.deriveChildSelector(context); Review Comment: ⚠️ **Unchecked cast** — `delegate.deriveChildSelector()` is declared to return `DependencySelector`, not `ExclusionDependencySelector`. This will throw `ClassCastException` if maven-resolver ever wraps or decorates the selector. Consider keeping a reference to the inner `ExclusionDependencySelector` directly, or restructuring to avoid the cast. ########## impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/TrackingExclusionDependencySelector.java: ########## @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.impl.resolver; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; + +import org.eclipse.aether.RepositorySystemSession; +import org.eclipse.aether.artifact.Artifact; +import org.eclipse.aether.collection.DependencyCollectionContext; +import org.eclipse.aether.collection.DependencySelector; +import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.Exclusion; +import org.eclipse.aether.util.graph.selector.ExclusionDependencySelector; + +/** + * A dependency selector that wraps {@link ExclusionDependencySelector} to track which declared + * exclusions on direct dependencies are actually encountered and applied during dependency collection. + * This allows detecting obsolete or invalid exclusions that have no impact on the dependency tree. + */ +public class TrackingExclusionDependencySelector implements DependencySelector { + + public static final String EXCLUSION_TRACKER_KEY = TrackingExclusionDependencySelector.class.getName(); + + public static class ExclusionTrackingRecord { + private final Dependency directDependency; + private final Collection<Exclusion> declaredExclusions; + private final Set<Exclusion> matchedExclusions = ConcurrentHashMap.newKeySet(); + + public ExclusionTrackingRecord(Dependency directDependency, Collection<Exclusion> declaredExclusions) { + this.directDependency = directDependency; + this.declaredExclusions = declaredExclusions; + } + + public Dependency getDirectDependency() { + return directDependency; + } + + public Collection<Exclusion> getDeclaredExclusions() { + return declaredExclusions; + } + + public Set<Exclusion> getMatchedExclusions() { + return matchedExclusions; Review Comment: ⚠️ **Mutable internal state leak** — `matchedExclusions` is the live `ConcurrentHashMap.newKeySet()`. Callers can mutate it externally. Return an unmodifiable view: ```suggestion return Collections.unmodifiableSet(matchedExclusions); ``` (or expose only a `contains()` method and an `addMatched()` mutator to keep the mutation path explicit) -- 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]
