gnodet-bot commented on code in PR #13897:
URL: https://github.com/apache/maven/pull/13897#discussion_r4507008309
##########
api/maven-api-model/src/main/mdo/maven.mdo:
##########
@@ -1450,6 +1460,34 @@
</field>
</fields>
</class>
+ <class>
+ <name>Inclusion</name>
+ <version>4.1.0+</version>
+ <description>
+ <![CDATA[
+ The {@code <inclusion>} element contains information required to
include
+ an artifact in the project when importing a bill of materials.
+ <p>
+ The {@code groupId} and {@code artifactId} fields are interpreted as
glob patterns.
+
+ @see java.nio.file.FileSystem#getPathMatcher
Review Comment:
⚠️ **False API contract — `@see` reference is misleading and should be
removed.**
`java.nio.file.FileSystem#getPathMatcher` documents glob patterns that
include `{x,y}` alternation groups and `[a-z]` character ranges as standard
features. Users who read this `@see` will expect those to work.
They **will not**. `compilePattern()` explicitly escapes `{`, `}`, `[`, `]`
as literals:
```java
case '{': case '}': case '[': case ']':
regex.append('\\').append(c); // treated as literal characters, NOT
glob syntax
```
So `{org.springframework,com.fasterxml}` matches the literal string
`{org.springframework,com.fasterxml}`, not an alternation. `[a-z]` matches the
literal string `[a-z]`.
Remove the `@see` and replace with an explicit description of what IS
supported:
```suggestion
The {@code groupId} and {@code artifactId} fields support simple
glob patterns.
Supported wildcards: {@code *} matches any sequence of characters,
{@code ?} matches exactly one character. Character classes and
alternation
groups (e.g., {@code [a-z]}, {@code {x,y}}) are not supported — use
separate
{@code <include>} elements for multiple patterns.
```
##########
api/maven-api-model/src/main/mdo/maven.mdo:
##########
@@ -1312,6 +1312,16 @@
<multiplicity>*</multiplicity>
</association>
</field>
+ <field>
+ <name>includes</name>
+ <version>4.1.0+</version>
+ <description>Lists a set of artifacts that should be included from
this dependency's
+ artifact list when importing a bill of materials.</description>
Review Comment:
💡 **Missing semantics documentation** — users who read this will not
understand the matching behavior.
The description doesn't explain:
- **OR semantics**: a dependency is included if it matches *any* `<include>`
entry (not all of them)
- **Wildcard fields**: omitting `groupId` or `artifactId` from an
`<include>` matches any value for that coordinate (equivalent to `*`)
- **Ordering with exclusions**: when both `<includes>` and `<exclusions>`
are specified, includes are applied first, then exclusions
```suggestion
<description>Lists a set of artifacts to include when importing a
bill of materials.
A dependency from the imported BOM is included if it matches at
least one
{@code <include>} entry (OR semantics). Omitting {@code groupId}
or
{@code artifactId} in an {@code <include>} acts as a wildcard
matching any value.
When both {@code <includes>} and {@code <exclusions>} are
present, inclusions
are applied first, then exclusions are applied to the remaining
set.</description>
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -3230,13 +3239,69 @@ private static org.apache.maven.api.model.Dependency
addExclusions(
.toList());
}
+ private final Map<String, Pattern> patternCache = new
ConcurrentHashMap<>();
Review Comment:
⚠️ **Unbounded cache on a `@Singleton` — potential memory leak in
long-running daemons.**
`patternCache` is a `ConcurrentHashMap` with no eviction.
`DefaultModelBuilder` is a `@Singleton`, so this map lives for the entire
lifecycle of the Maven process (including the Maven daemon). Every distinct
glob pattern encountered across all projects in the session adds an entry that
is never removed.
In practice, glob patterns are few and small, so this is unlikely to be a
problem in normal usage. But it's worth adding an explicit size cap or a
comment documenting the expected bound:
```suggestion
// Pattern cache: keyed by glob pattern string; entries are
deterministic and tiny.
// Size is bounded by the number of distinct glob patterns used across
the session
// (typically single digits). No eviction needed in practice.
private final Map<String, Pattern> patternCache = new
ConcurrentHashMap<>();
```
Alternatively, use a size-limited cache (e.g.,
`Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true) { protected
boolean removeEldestEntry(Map.Entry e) { return size() > 1000; } })`) if strict
bounding is required.
--
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]
