gnodet-bot commented on code in PR #13897:
URL: https://github.com/apache/maven/pull/13897#discussion_r4507283476
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -3230,13 +3240,81 @@ private static org.apache.maven.api.model.Dependency
addExclusions(
.toList());
}
+ static final int MAX_PATTERN_CACHE_SIZE = 1000;
+
+ /**
+ * Bounded LRU cache for compiled regex patterns from glob expressions
used in BOM inclusion/exclusion matching.
+ * Prevents unbounded memory growth in long-running Maven daemon sessions
while maintaining thread safety.
+ */
+ final Map<String, Pattern> patternCache =
+ Collections.synchronizedMap(new LinkedHashMap<String, Pattern>(16,
0.75f, true) {
+ @Override
+ protected boolean removeEldestEntry(Map.Entry<String, Pattern>
eldest) {
+ return size() > MAX_PATTERN_CACHE_SIZE;
+ }
+ });
+
+ private boolean match(Inclusion inclusion, Dependency candidate) {
+ return match(inclusion.getGroupId(), candidate.getGroupId())
+ && match(inclusion.getArtifactId(), candidate.getArtifactId());
+ }
+
private boolean match(Exclusion exclusion, Dependency candidate) {
return match(exclusion.getGroupId(), candidate.getGroupId())
&& match(exclusion.getArtifactId(), candidate.getArtifactId());
}
- private boolean match(String match, String text) {
- return match.equals("*") || match.equals(text);
+ boolean match(String pattern, String text) {
+ if (pattern == null || pattern.isEmpty() || "*".equals(pattern)) {
+ return true;
+ }
+ if (text == null) {
+ return false;
+ }
+ if (pattern.equals(text)) {
+ return true;
+ }
+ if (!pattern.contains("*") && !pattern.contains("?")) {
+ return false;
+ }
+ return patternCache
+ .computeIfAbsent(pattern, DefaultModelBuilder::compilePattern)
+ .matcher(text)
+ .matches();
+ }
Review Comment:
⚠️ **Behavioral side-effect on exclusions:** `match(Exclusion, Dependency)`
calls this same `match(String, String)`, so glob pattern matching now also
applies to `<exclusion>` entries in POMs. Previously only literal `*` or exact
match was supported. This is likely a welcome improvement (wildcards in
exclusions previously didn't work as expected), but it's an undocumented
behavioral change. Worth mentioning in the commit message or javadoc.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelValidator.java:
##########
@@ -1571,48 +1595,93 @@ private void validateEffectiveDependency(
}
if (validationLevel >= ModelValidator.VALIDATION_LEVEL_MAVEN_2_0) {
- for (Exclusion exclusion : dependency.getExclusions()) {
- if (validationLevel <
ModelValidator.VALIDATION_LEVEL_MAVEN_3_0) {
- validateCoordinatesId(
- prefix,
- "exclusions.exclusion.groupId",
- problems,
- Severity.WARNING,
- Version.V20,
- exclusion.getGroupId(),
- SourceHint.dependencyManagementKey(dependency),
- exclusion);
+ validateDependencyExclusions(problems, dependency, prefix,
validationLevel);
+ validateDependencyInclusions(problems, dependency, prefix);
+ }
+ }
- validateCoordinatesId(
- prefix,
- "exclusions.exclusion.artifactId",
- problems,
- Severity.WARNING,
- Version.V20,
- exclusion.getArtifactId(),
- SourceHint.dependencyManagementKey(dependency),
- exclusion);
- } else {
- validateCoordinatesIdWithWildcards(
- prefix,
- "exclusions.exclusion.groupId",
- problems,
- Severity.WARNING,
- Version.V30,
- exclusion.getGroupId(),
- SourceHint.dependencyManagementKey(dependency),
- exclusion);
+ private void validateDependencyExclusions(
+ ModelProblemCollector problems, Dependency dependency, String
prefix, int validationLevel) {
+ for (Exclusion exclusion : dependency.getExclusions()) {
+ if (validationLevel < ModelValidator.VALIDATION_LEVEL_MAVEN_3_0) {
+ validateCoordinatesId(
+ prefix,
+ "exclusions.exclusion.groupId",
+ problems,
+ Severity.WARNING,
+ Version.V20,
+ exclusion.getGroupId(),
+ SourceHint.dependencyManagementKey(dependency),
+ exclusion);
- validateCoordinatesIdWithWildcards(
- prefix,
- "exclusions.exclusion.artifactId",
- problems,
- Severity.WARNING,
- Version.V30,
- exclusion.getArtifactId(),
- SourceHint.dependencyManagementKey(dependency),
- exclusion);
- }
+ validateCoordinatesId(
+ prefix,
+ "exclusions.exclusion.artifactId",
+ problems,
+ Severity.WARNING,
+ Version.V20,
+ exclusion.getArtifactId(),
+ SourceHint.dependencyManagementKey(dependency),
+ exclusion);
+ } else {
+ validateCoordinatesIdWithWildcards(
+ prefix,
+ "exclusions.exclusion.groupId",
+ problems,
+ Severity.WARNING,
+ Version.V30,
+ exclusion.getGroupId(),
+ SourceHint.dependencyManagementKey(dependency),
+ exclusion);
+
+ validateCoordinatesIdWithWildcards(
+ prefix,
+ "exclusions.exclusion.artifactId",
+ problems,
+ Severity.WARNING,
+ Version.V30,
+ exclusion.getArtifactId(),
+ SourceHint.dependencyManagementKey(dependency),
+ exclusion);
+ }
+ }
+ }
+
+ private void validateDependencyInclusions(ModelProblemCollector problems,
Dependency dependency, String prefix) {
+ for (Inclusion inclusion : dependency.getIncludes()) {
+ if ((inclusion.getGroupId() == null ||
inclusion.getGroupId().isEmpty())
+ && (inclusion.getArtifactId() == null
+ || inclusion.getArtifactId().isEmpty())) {
Review Comment:
💡 **Minor gap:** `"*"` is semantically equivalent to an omitted coordinate
(both return `true` from `match()`), but this check only warns when `groupId`
and `artifactId` are `null` or empty — a bare `<groupId>*</groupId>` with no
`artifactId` won't trigger the warning. Consider adding `||
"*".equals(inclusion.getGroupId())` / `||
"*".equals(inclusion.getArtifactId())` to the conditions, or document that `*`
is treated as a wildcard-all and is explicitly allowed.
--
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]
