gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4059587982
##########
impl/maven-testing/src/main/java/org/apache/maven/testing/plugin/stubs/SessionStub.java:
##########
@@ -153,6 +154,11 @@ public SessionData getData() {
return null;
}
+ @Override
+ public BuildEnvironment buildEnvironment() {
+ return null;
Review Comment:
**[Medium] `@Nonnull` contract violation — returns `null`.**
`Session.buildEnvironment()` is declared `@Nonnull` in the interface.
`SessionStub` returns `null` here, which means any test that calls
`session.buildEnvironment().goals()` (or any other chained call) via a
`SessionStub` will throw NPE at runtime — silently, with no indication that the
stub is the culprit.
For a test stub, returning a no-op empty environment is straightforward:
```suggestion
@Override
public BuildEnvironment buildEnvironment() {
return new BuildEnvironment() {
@Override public java.util.List<String> goals() { return
java.util.List.of(); }
@Override public java.util.Map<String, String> userProperties()
{ return java.util.Map.of(); }
@Override public java.util.Map<String, String> systemInfo() {
return java.util.Map.of(); }
@Override public String localRepository() { return ""; }
@Override public java.util.List<String> activeProfiles() {
return java.util.List.of(); }
@Override public java.util.List<String> selectedProjects() {
return java.util.List.of(); }
@Override public String resumeFrom() { return null; }
@Override public String reactorFailureBehavior() { return
"FAIL_FAST"; }
@Override public boolean offline() { return false; }
@Override public boolean updateSnapshots() { return false; }
@Override public boolean noTransferProgress() { return false; }
@Override public boolean batchMode() { return false; }
@Override public int threads() { return 1; }
};
}
```
Alternatively, if `ApiRunner`'s anonymous implementation is extracted to a
package-accessible constant (see the other comment), `SessionStub` could reuse
it.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/standalone/ApiRunner.java:
##########
@@ -381,6 +382,78 @@ public int getDegreeOfConcurrency() {
return 0;
}
+ @Override
+ public BuildEnvironment buildEnvironment() {
+ // ApiRunner is a standalone/embedded session with no
MavenExecutionRequest;
+ // return a minimal environment reflecting defaults.
+ return new BuildEnvironment() {
+ @Override
+ public List<String> goals() {
+ return List.of();
+ }
+
+ @Override
+ public Map<String, String> userProperties() {
+ return Map.of();
+ }
+
+ @Override
+ public Map<String, String> systemInfo() {
+ return Map.of();
+ }
+
+ @Override
+ public String localRepository() {
+ return "";
+ }
+
+ @Override
+ public List<String> activeProfiles() {
+ return List.of();
+ }
+
+ @Override
+ public List<String> selectedProjects() {
+ return List.of();
+ }
+
+ @Override
+ public String resumeFrom() {
+ return null;
+ }
+
+ @Override
+ public String reactorFailureBehavior() {
+ return "FAIL_FAST";
+ }
+
+ @Override
+ public boolean offline() {
+ return false;
+ }
+
+ @Override
+ public boolean updateSnapshots() {
+ return false;
+ }
+
+ @Override
+ public boolean noTransferProgress() {
+ return false;
+ }
+
+ @Override
+ public boolean batchMode() {
+ return false;
+ }
+
+ @Override
+ public int threads() {
+ return 1;
+ }
+ };
+ }
Review Comment:
**[Low] New anonymous `BuildEnvironment` instance on every
`buildEnvironment()` call — no caching.**
All return values are static defaults (`List.of()`, `Map.of()`, `false`,
`1`, etc.). There is no reason to allocate a new anonymous object per call.
Extract this to a `private static final` field:
```suggestion
private static final BuildEnvironment EMPTY_ENV = new
BuildEnvironment() {
@Override public java.util.List<String> goals() { return
java.util.List.of(); }
@Override public java.util.Map<String, String> userProperties()
{ return java.util.Map.of(); }
@Override public java.util.Map<String, String> systemInfo() {
return java.util.Map.of(); }
@Override public String localRepository() { return ""; }
@Override public java.util.List<String> activeProfiles() {
return java.util.List.of(); }
@Override public java.util.List<String> selectedProjects() {
return java.util.List.of(); }
@Override public String resumeFrom() { return null; }
@Override public String reactorFailureBehavior() { return
"FAIL_FAST"; }
@Override public boolean offline() { return false; }
@Override public boolean updateSnapshots() { return false; }
@Override public boolean noTransferProgress() { return false; }
@Override public boolean batchMode() { return false; }
@Override public int threads() { return 1; }
};
@Override
public BuildEnvironment buildEnvironment() {
return EMPTY_ENV;
}
```
As a bonus, `SessionStub.buildEnvironment()` could then return this same
constant instead of `null`.
--
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]
