gnodet-bot commented on code in PR #2277:
URL: https://github.com/apache/maven/pull/2277#discussion_r4261084946
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java:
##########
@@ -210,100 +212,95 @@ public Collection<Lifecycle> provides() {
.filter(id -> !Lifecycle.CLEAN.equals(id)
&& !Lifecycle.DEFAULT.equals(id)
&& !Lifecycle.SITE.equals(id))
- .map(id -> wrap(all.get(id)))
- .collect(Collectors.toList());
+ .map(id -> (Lifecycle) new
WrappedLifecycle(all.get(id)))
+ .toList();
} catch (ComponentLookupException e) {
throw new LookupException(e);
}
}
+ }
- private Lifecycle wrap(org.apache.maven.lifecycle.Lifecycle lifecycle)
{
- return new Lifecycle() {
- @Override
- public String id() {
- return lifecycle.getId();
- }
+ /**
+ * Record implementation of Lifecycle.Phase for wrapped phases.
+ *
+ * @param name The name of the phase
+ * @param prev The name of the previous phase (may be null)
+ * @param lifecycle The original Maven 3 lifecycle
+ */
+ record WrappedPhase(String name, String prev,
org.apache.maven.lifecycle.Lifecycle lifecycle)
+ implements Lifecycle.Phase {
+ /**
+ * Compact constructor with null validation.
+ */
+ WrappedPhase {
+ Objects.requireNonNull(name, "name cannot be null");
+ Objects.requireNonNull(lifecycle, "lifecycle cannot be null");
+ // prev can be null for the first phase
+ }
- @Override
- public Collection<Phase> phases() {
- List<String> names = lifecycle.getPhases();
- List<Phase> phases = new ArrayList<>();
- for (int i = 0; i < names.size(); i++) {
- String name = names.get(i);
- String prev = i > 0 ? names.get(i - 1) : null;
- phases.add(new Phase() {
- @Override
- public String name() {
- return name;
- }
-
- @Override
- public List<Phase> phases() {
- return List.of();
- }
-
- @Override
- public Stream<Phase> allPhases() {
- return Stream.concat(
- Stream.of(this),
phases().stream().flatMap(Lifecycle.Phase::allPhases));
- }
-
- @Override
- public List<Plugin> plugins() {
- Map<String, LifecyclePhase> lfPhases =
lifecycle.getDefaultLifecyclePhases();
- LifecyclePhase phase = lfPhases != null ?
lfPhases.get(name) : null;
- if (phase != null) {
- Map<String, Plugin> plugins = new
LinkedHashMap<>();
-
DefaultPackagingRegistry.parseLifecyclePhaseDefinitions(plugins, name, phase);
- return plugins.values().stream().toList();
- }
- return List.of();
- }
-
- @Override
- public Collection<Link> links() {
- if (prev == null) {
- return List.of();
- } else {
- return List.of(new Link() {
- @Override
- public Kind kind() {
- return Kind.AFTER;
- }
-
- @Override
- public Pointer pointer() {
- return new Pointer() {
- @Override
- public String phase() {
- return prev;
- }
-
- @Override
- public Type type() {
- return Type.PROJECT;
- }
- };
- }
- });
- }
- }
- });
- }
- return phases;
- }
+ @Override
+ @Nonnull
+ public List<Lifecycle.Phase> phases() {
+ return List.of();
+ }
- @Override
- public Collection<Alias> aliases() {
- return Collections.emptyList();
- }
- };
+ @Override
+ @Nonnull
+ public List<Plugin> plugins() {
+ Map<String, LifecyclePhase> lfPhases =
lifecycle.getDefaultLifecyclePhases();
+ return lfPhases != null
+ ?
List.copyOf(DefaultPackagingRegistry.parseLifecyclePhaseDefinitions(lfPhases))
+ : List.of();
+ }
Review Comment:
đ **Bug: `WrappedPhase.plugins()` returns plugins from ALL phases, not just
this phase.**
The old code used `lfPhases.get(name)` to fetch only the single
`LifecyclePhase` entry for this phase's name, then called
`parseLifecyclePhaseDefinitions(plugins, name, phase)` on that one entry.
The new code calls `lifecycle.getDefaultLifecyclePhases()` which returns the
**entire map** of all phases, then passes that full map to
`parseLifecyclePhaseDefinitions(lfPhases)`, which iterates over **all entries**
via `phases.forEach(...)`. Every `WrappedPhase` instance will now return the
union of plugins from every phase in the lifecycle, rather than only those
bound to its own phase.
This needs to be filtered to only the current phase:
```suggestion
public List<Plugin> plugins() {
Map<String, LifecyclePhase> lfPhases =
lifecycle.getDefaultLifecyclePhases();
LifecyclePhase phase = lfPhases != null ? lfPhases.get(name) :
null;
return phase != null
?
List.copyOf(DefaultPackagingRegistry.parseLifecyclePhaseDefinitions(Map.of(name,
phase)))
: List.of();
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/CoreUtils.java:
##########
@@ -22,21 +22,36 @@
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
-import java.util.stream.Collectors;
+
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.Nullable;
class CoreUtils {
- public static <T> T cast(Class<T> clazz, Object o, String name) {
+ /**
+ * Casts an object to the specified type, with validation and error
handling.
+ *
+ * @param <T> the target type
+ * @param clazz the class representing the target type
+ * @param o the object to cast
+ * @param name the name of the parameter for error messages
+ * @return the cast object
+ * @throws NullPointerException if the object is null
+ * @throws ClassCastException if the object is not an instance of the
target type
+ */
+ @Nonnull
+ public static <T> T cast(@Nonnull Class<T> clazz, @Nullable Object o,
@Nonnull String name) {
if (!clazz.isInstance(o)) {
if (o == null) {
- throw new IllegalArgumentException(name + " is null");
+ throw new NullPointerException(name + " is null");
}
- throw new IllegalArgumentException(name + " is not an instance of
" + clazz.getName());
+ throw new ClassCastException(name + " is not an instance of " +
clazz.getName());
Review Comment:
â ī¸ **Behavioral change: exception types changed from
`IllegalArgumentException` to `NullPointerException`/`ClassCastException`.**
The previous contract threw `IllegalArgumentException` for both null and
type-mismatch cases. Changing to `NullPointerException` and
`ClassCastException` is arguably more semantically correct, but it is a
**breaking behavioral change** for any caller that currently catches
`IllegalArgumentException` (which is a supertype of neither
`NullPointerException` nor `ClassCastException`).
Since `CoreUtils` is internal (`impl` package), this may be acceptable â but
it's worth verifying no catch sites in the codebase rely on the old exception
type:
```bash
rg 'catch.*IllegalArgumentException' impl/maven-core/src/
```
If any caller catches `IAE` expecting this method's failures, they will
silently stop catching after this change.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Lifecycle.java:
##########
@@ -87,6 +90,7 @@ default Collection<Phase> v3phases() {
*
* @return a stream of all phases in this lifecycle, including nested
phases
*/
+ @Nonnull
default Stream<Phase> allPhases() {
return phases().stream().flatMap(Phase::allPhases);
}
Review Comment:
âšī¸ **Note: `Lifecycle.allPhases()` does not include the lifecycle itself
(intentional asymmetry).**
`Lifecycle.allPhases()` (here) uses
`phases().stream().flatMap(Phase::allPhases)`, which does NOT include `this`.
Meanwhile `Phase.allPhases()` (line ~175) uses `Stream.concat(Stream.of(this),
...)` which DOES include `this`.
This asymmetry is likely intentional since `Lifecycle` is not a `Phase`, but
worth a brief Javadoc note to clarify the intent, e.g.:
```suggestion
* @return a stream of all phases in this lifecycle, including nested
phases;
* the lifecycle itself is not included (unlike {@link
Phase#allPhases()})
*/
@Nonnull
default Stream<Phase> allPhases() {
```
--
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]
