gnodet-bot commented on code in PR #13249:
URL: https://github.com/apache/maven/pull/13249#discussion_r4492181587
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/BaseParser.java:
##########
@@ -499,11 +538,48 @@ protected List<CoreExtensions>
readCoreExtensionsDescriptor(LocalContext context
Map<String, String> eff = new HashMap<>(context.systemProperties);
eff.putAll(context.userProperties);
- // project
- file =
context.cwd.resolve(eff.get(Constants.MAVEN_PROJECT_EXTENSIONS));
- loaded = readCoreExtensionsDescriptorFromFile(file, false);
- if (!loaded.isEmpty()) {
- result.add(new CoreExtensions(file, loaded));
+ // project extensions: use reactor.xml <extensions> when present,
otherwise extensions.xml
+ if (context.reactorConfig != null
+ && context.reactorConfig.getExtensions() != null
+ && !context.reactorConfig.getExtensions().isEmpty()) {
+ // reactor.xml is authoritative for project extensions
+ Path dir = context.rootDirectory != null ? context.rootDirectory :
context.topDirectory;
+ Path reactorXml = dir != null ? dir.resolve(".mvn/reactor.xml") :
null;
+ loaded = context.reactorConfig.getExtensions().stream()
+ .map(re -> {
+ String groupId = re.getGroupId();
+ String artifactId = re.getArtifactId();
+ String version = re.getVersion();
+ String id = re.getId();
+ if (id != null && !id.isEmpty()) {
+ String[] parts = id.split(":", -1);
+ if (parts.length != 3) {
+ throw new IllegalArgumentException(
+ "Invalid extension id '" + id + "':
expected groupId:artifactId:version");
+ }
+ groupId = parts[0];
+ artifactId = parts[1];
+ version = parts[2];
+ }
Review Comment:
⚠️ **Missing validation for absent coordinates** — after `id` is processed
(or skipped), there is no check that `groupId`, `artifactId`, and `version` are
all non-null. A `<extension classLoadingStrategy="self-first"/>` element (no
id, no coordinates) produces a `CoreExtension` with three `null` fields and
fails later with an obscure NPE. Add before `return`:
```suggestion
}
if (groupId == null || artifactId == null || version
== null) {
throw new IllegalArgumentException(
"Extension must specify either id or all
of groupId, artifactId, and version");
}
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/BaseParser.java:
##########
@@ -499,11 +538,48 @@ protected List<CoreExtensions>
readCoreExtensionsDescriptor(LocalContext context
Map<String, String> eff = new HashMap<>(context.systemProperties);
eff.putAll(context.userProperties);
- // project
- file =
context.cwd.resolve(eff.get(Constants.MAVEN_PROJECT_EXTENSIONS));
- loaded = readCoreExtensionsDescriptorFromFile(file, false);
- if (!loaded.isEmpty()) {
- result.add(new CoreExtensions(file, loaded));
+ // project extensions: use reactor.xml <extensions> when present,
otherwise extensions.xml
+ if (context.reactorConfig != null
+ && context.reactorConfig.getExtensions() != null
+ && !context.reactorConfig.getExtensions().isEmpty()) {
+ // reactor.xml is authoritative for project extensions
+ Path dir = context.rootDirectory != null ? context.rootDirectory :
context.topDirectory;
+ Path reactorXml = dir != null ? dir.resolve(".mvn/reactor.xml") :
null;
+ loaded = context.reactorConfig.getExtensions().stream()
+ .map(re -> {
+ String groupId = re.getGroupId();
+ String artifactId = re.getArtifactId();
+ String version = re.getVersion();
+ String id = re.getId();
+ if (id != null && !id.isEmpty()) {
Review Comment:
⚠️ **No mutual-exclusion enforcement** — the model documents `id` as
mutually exclusive with `groupId`/`artifactId`/`version`, but the code silently
lets both coexist (id wins). Add a check:
```suggestion
if (id != null && !id.isEmpty()) {
if (groupId != null || artifactId != null ||
version != null) {
throw new IllegalArgumentException(
"Extension id '" + id + "' must not
be combined with groupId/artifactId/version");
}
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/BaseParser.java:
##########
@@ -499,11 +538,48 @@ protected List<CoreExtensions>
readCoreExtensionsDescriptor(LocalContext context
Map<String, String> eff = new HashMap<>(context.systemProperties);
eff.putAll(context.userProperties);
- // project
- file =
context.cwd.resolve(eff.get(Constants.MAVEN_PROJECT_EXTENSIONS));
- loaded = readCoreExtensionsDescriptorFromFile(file, false);
- if (!loaded.isEmpty()) {
- result.add(new CoreExtensions(file, loaded));
+ // project extensions: use reactor.xml <extensions> when present,
otherwise extensions.xml
+ if (context.reactorConfig != null
+ && context.reactorConfig.getExtensions() != null
+ && !context.reactorConfig.getExtensions().isEmpty()) {
+ // reactor.xml is authoritative for project extensions
+ Path dir = context.rootDirectory != null ? context.rootDirectory :
context.topDirectory;
+ Path reactorXml = dir != null ? dir.resolve(".mvn/reactor.xml") :
null;
+ loaded = context.reactorConfig.getExtensions().stream()
+ .map(re -> {
+ String groupId = re.getGroupId();
+ String artifactId = re.getArtifactId();
+ String version = re.getVersion();
+ String id = re.getId();
+ if (id != null && !id.isEmpty()) {
+ String[] parts = id.split(":", -1);
+ if (parts.length != 3) {
+ throw new IllegalArgumentException(
+ "Invalid extension id '" + id + "':
expected groupId:artifactId:version");
+ }
+ groupId = parts[0];
+ artifactId = parts[1];
+ version = parts[2];
Review Comment:
⚠️ **Empty component check missing** — `id.split(":", -1)` on `"::"` gives
`["","",""]`, which passes the `parts.length != 3` guard and silently loads an
extension with empty coordinates. Add non-empty validation for each component:
```suggestion
String[] parts = id.split(":", -1);
if (parts.length != 3
|| parts[0].isEmpty()
|| parts[1].isEmpty()
|| parts[2].isEmpty()) {
throw new IllegalArgumentException(
"Invalid extension id '" + id + "':
expected groupId:artifactId:version");
}
groupId = parts[0];
artifactId = parts[1];
version = parts[2];
```
--
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]
