gnodet-bot commented on code in PR #13480:
URL: https://github.com/apache/maven/pull/13480#discussion_r4260899224
##########
maven-embedder/src/test/java/org/apache/maven/cli/MavenCliTest.java:
##########
@@ -104,6 +107,20 @@ public void testCalculateDegreeOfConcurrency() {
assertThrows(IllegalArgumentException.class, () ->
cli.calculateDegreeOfConcurrency("0C"));
}
+ @Test
+ public void testOptionalProfiles() throws Exception {
+ List<String> active = new ArrayList<>();
+ List<String> inactive = new ArrayList<>();
+
+ MavenCli.performProfileActivation(
+ new CLIManager().parse(new String[] {"-P",
"?run-its,+?docs,-?slow,!?legacy,plain", "-P", "?more"}),
+ active,
+ inactive);
+
+ assertEquals(Arrays.asList("run-its", "docs", "plain", "more"),
active);
+ assertEquals(Arrays.asList("slow", "legacy"), inactive);
+ }
Review Comment:
💡 **Suggestion: cover degenerate/edge-case tokens in the test**
The test only exercises well-formed profile IDs where every token resolves
to a non-empty string. Consider adding a second assertion (or a separate test
method) for the degenerate cases that should produce no list entries — e.g. `-P
?`, `-P !`, `-P -?`, `-P +?`, and an empty token from a trailing comma (`,`).
These are exactly the inputs that expose the missing `isEmpty()` guard
described above.
Example:
```java
@Test
public void testDegenerateProfileTokens() throws Exception {
List<String> active = new ArrayList<>();
List<String> inactive = new ArrayList<>();
MavenCli.performProfileActivation(
new CLIManager().parse(new String[] {"-P", "?,!,+?,-?"}),
active,
inactive);
assertEquals(Collections.emptyList(), active);
assertEquals(Collections.emptyList(), inactive);
}
```
##########
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -1658,6 +1639,43 @@ int calculateDegreeOfConcurrency(String
originalThreadConfiguration) {
}
}
+ // Visible for testing
+ static void performProfileActivation(
+ final CommandLine commandLine, final List<String> activeProfiles,
final List<String> inactiveProfiles) {
+ if (commandLine.hasOption(CLIManager.ACTIVATE_PROFILES)) {
+ String[] profileOptionValues =
commandLine.getOptionValues(CLIManager.ACTIVATE_PROFILES);
+ if (profileOptionValues != null) {
+ for (String profileOptionValue : profileOptionValues) {
+ StringTokenizer profileTokens = new
StringTokenizer(profileOptionValue, ",");
+
+ while (profileTokens.hasMoreTokens()) {
+ String profileId = profileTokens.nextToken().trim();
+ boolean active = true;
+
+ if (profileId.startsWith("-") ||
profileId.startsWith("!")) {
+ active = false;
+ profileId = profileId.substring(1);
+ } else if (profileId.startsWith("+")) {
+ profileId = profileId.substring(1);
+ }
+
+ // "?id" is an optional profile in Maven 4; Maven 3
never fails on a missing profile,
+ // so the prefix only has to be removed
+ if (profileId.startsWith("?")) {
+ profileId = profileId.substring(1);
+ }
+
+ if (active) {
+ activeProfiles.add(profileId);
+ } else {
+ inactiveProfiles.add(profileId);
Review Comment:
⚠️ **Bug: empty `profileId` after prefix stripping is silently added to the
list**
After stripping the `?` prefix (and any leading `-`/`!`/`+`), `profileId`
may be an empty string — for inputs like `-P ?`, `-P -?`, or `-P !?`. There is
no guard before the `activeProfiles.add(profileId)` /
`inactiveProfiles.add(profileId)` calls, so an empty string ends up in the
list. Maven 4's `performProfileActivation` has an `!profileId.isEmpty()` check
here.
```suggestion
if (profileId.startsWith("?")) {
profileId = profileId.substring(1);
}
if (!profileId.isEmpty()) {
if (active) {
activeProfiles.add(profileId);
} else {
inactiveProfiles.add(profileId);
}
}
```
--
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]
