gnodet-bot commented on code in PR #13249: URL: https://github.com/apache/maven/pull/13249#discussion_r4324836148
########## impl/maven-core/src/test/java/org/apache/maven/internal/impl/PhaseEnrichedLifecycleTest.java: ########## @@ -0,0 +1,332 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.internal.impl; + +import java.util.Collection; +import java.util.List; + +import org.apache.maven.api.Lifecycle; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Unit tests for {@link PhaseEnrichedLifecycle}. + * + * <p>In Maven 4 the "default" lifecycle is a <em>tree</em> of nested phases. Phases like + * {@code unit-test} and {@code integration-test} are children of {@code verify}, which is itself + * a child of {@code each}. Custom phases must therefore declare their {@code parent} explicitly. + * + * <p>Ordering is verified via {@link DefaultLifecycleRegistry#computePhases(Lifecycle)}, which + * performs the full topological sort on the lifecycle DAG. + */ +class PhaseEnrichedLifecycleTest { + + /** InjectedPhase(name, parent, after, before) */ + private static PhaseEnrichedLifecycle.InjectedPhase ip(String name, String parent, String after, String before) { + return new PhaseEnrichedLifecycle.InjectedPhase(name, parent, after, before); + } + + private static Lifecycle defaultLifecycle() { + DefaultLifecycleRegistry registry = new DefaultLifecycleRegistry(List.of()); + return registry.stream() + .filter(lc -> Lifecycle.DEFAULT.equals(lc.id())) + .findFirst() + .orElseThrow(() -> new AssertionError("Default lifecycle not found")); + } + + private static DefaultLifecycleRegistry registry() { + return new DefaultLifecycleRegistry(List.of()); + } + + // ------------------------------------------------------------------------- + // Phase presence + // ------------------------------------------------------------------------- + + @Test + void injectedPhaseAppearsInAllPhases() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", "unit-test", null))); + + List<String> names = enriched.allPhases().map(Lifecycle.Phase::name).toList(); + assertTrue(names.contains("pre-integration"), "'pre-integration' must appear in allPhases()"); + assertTrue(names.contains("unit-test"), "'unit-test' must still be present"); + assertTrue(names.contains("integration-test"), "'integration-test' must still be present"); + } + + @Test + void injectedPhaseAppearsInV3Phases() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", "unit-test", null))); + + List<String> names = enriched.v3phases().stream() + .flatMap(Lifecycle.Phase::allPhases) + .map(Lifecycle.Phase::name) + .toList(); + assertTrue(names.contains("pre-integration"), "'pre-integration' must appear in v3phases() tree"); + } + + // ------------------------------------------------------------------------- + // Topological ordering via computePhases() + // ------------------------------------------------------------------------- + + @Test + void computedPhasesRespectAfterConstraint() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", "unit-test", null))); + + List<String> computed = registry().computePhases(enriched); + + int unitTestIdx = computed.indexOf("unit-test"); + int newIdx = computed.indexOf("pre-integration"); + int itIdx = computed.indexOf("integration-test"); + assertTrue(unitTestIdx >= 0, "'unit-test' must appear in computePhases()"); + assertTrue(newIdx >= 0, "'pre-integration' must appear in computePhases()"); + assertTrue(itIdx >= 0, "'integration-test' must appear in computePhases()"); + assertTrue(unitTestIdx < newIdx, "'pre-integration' must come after 'unit-test'"); + assertTrue(newIdx < itIdx, "'pre-integration' must come before 'integration-test'"); + } + + @Test + void computedPhasesRespectBeforeConstraint() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", null, "integration-test"))); + + List<String> computed = registry().computePhases(enriched); + + int itIdx = computed.indexOf("integration-test"); + int newIdx = computed.indexOf("pre-integration"); + assertTrue(itIdx >= 0, "'integration-test' must appear in computePhases()"); + assertTrue(newIdx >= 0, "'pre-integration' must appear in computePhases()"); + assertTrue(newIdx < itIdx, "'pre-integration' must precede 'integration-test' in topological order"); + } + + @Test + void computedPhasesWithAfterAndBefore() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = new PhaseEnrichedLifecycle( + base, List.of(ip("pre-integration", "verify", "unit-test", "integration-test"))); + + List<String> computed = registry().computePhases(enriched); + + int unitTestIdx = computed.indexOf("unit-test"); + int itIdx = computed.indexOf("integration-test"); + int newIdx = computed.indexOf("pre-integration"); + assertTrue(unitTestIdx >= 0, "'unit-test' must appear"); + assertTrue(itIdx >= 0, "'integration-test' must appear"); + assertTrue(newIdx >= 0, "'pre-integration' must appear"); + assertTrue(unitTestIdx < newIdx, "'pre-integration' must come after 'unit-test'"); + assertTrue(newIdx < itIdx, "'pre-integration' must come before 'integration-test'"); + } + + // ------------------------------------------------------------------------- + // DAG links on the injected phase + // ------------------------------------------------------------------------- + + @Test + void injectedPhaseHasAfterLink() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", "unit-test", null))); + + Lifecycle.Phase phase = enriched.allPhases() + .filter(p -> "pre-integration".equals(p.name())) + .findFirst() + .orElseThrow(() -> new AssertionError("'pre-integration' not found in allPhases()")); + + Collection<Lifecycle.Link> links = phase.links(); + assertEquals(1, links.size(), "injected phase must have exactly one AFTER link"); + Lifecycle.Link link = links.iterator().next(); + assertEquals(Lifecycle.Link.Kind.AFTER, link.kind(), "link must be AFTER"); + assertEquals("unit-test", link.pointer().phase(), "AFTER link must point to 'unit-test'"); + } + + @Test + void injectedPhaseBeforeHasAfterLinkToLeftSibling() { + // When only 'before' is specified, the AFTER link points to the left sibling (not the before anchor) + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-integration", "verify", null, "integration-test"))); + + Lifecycle.Phase phase = enriched.allPhases() + .filter(p -> "pre-integration".equals(p.name())) + .findFirst() + .orElseThrow(() -> new AssertionError("'pre-integration' not found")); + + Collection<Lifecycle.Link> links = phase.links(); + assertEquals(1, links.size(), "injected phase must have exactly one AFTER link"); + assertEquals(Lifecycle.Link.Kind.AFTER, links.iterator().next().kind()); + // Left sibling when inserting before 'integration-test' in [unit-test, integration-test] is 'unit-test' + assertEquals( + "unit-test", + links.iterator().next().pointer().phase(), + "AFTER link must point to 'unit-test' (left sibling)"); + } + + // ------------------------------------------------------------------------- + // Identity / delegation + // ------------------------------------------------------------------------- + + @Test + void idIsPreserved() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("x", "verify", "unit-test", null))); + assertEquals(Lifecycle.DEFAULT, enriched.id()); + } + + @Test + void aliasesAreDelegated() { + Lifecycle base = defaultLifecycle(); + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("x", "verify", "unit-test", null))); + List<String> baseV3 = + base.aliases().stream().map(Lifecycle.Alias::v3Phase).toList(); + List<String> enrichedV3 = + enriched.aliases().stream().map(Lifecycle.Alias::v3Phase).toList(); + assertEquals(baseV3, enrichedV3, "aliases must be fully delegated to the base lifecycle"); + } + + @Test + void originalPhasesAreRetained() { + Lifecycle base = defaultLifecycle(); + long originalCount = base.allPhases().count(); + + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("x", "verify", "unit-test", null))); + long enrichedCount = enriched.allPhases().count(); + + assertEquals(originalCount + 1, enrichedCount, "exactly one phase should be added"); + } + + // ------------------------------------------------------------------------- + // Comma-separated after/before anchors + // ------------------------------------------------------------------------- + + @Test + void commaSeparatedAfterAnchorsUsesLatest() { + Lifecycle base = defaultLifecycle(); + // after="unit-test,integration-test" → insert after integration-test (the latest) + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("post-all", "verify", "unit-test,integration-test", null))); + + List<String> computed = registry().computePhases(enriched); + + int unitTestIdx = computed.indexOf("unit-test"); + int itIdx = computed.indexOf("integration-test"); + int newIdx = computed.indexOf("post-all"); + assertTrue(unitTestIdx >= 0, "'unit-test' must appear"); + assertTrue(itIdx >= 0, "'integration-test' must appear"); + assertTrue(newIdx >= 0, "'post-all' must appear"); + assertTrue(itIdx < newIdx, "'post-all' must come after 'integration-test'"); + } + + @Test + void commaSeparatedBeforeAnchorsUsesEarliest() { + Lifecycle base = defaultLifecycle(); + // before="unit-test,integration-test" → insert before unit-test (the earliest) + PhaseEnrichedLifecycle enriched = + new PhaseEnrichedLifecycle(base, List.of(ip("pre-all", "verify", null, "unit-test,integration-test"))); + + List<String> computed = registry().computePhases(enriched); + + int unitTestIdx = computed.indexOf("unit-test"); + int newIdx = computed.indexOf("pre-all"); + assertTrue(unitTestIdx >= 0, "'unit-test' must appear"); + assertTrue(newIdx >= 0, "'pre-all' must appear"); + assertTrue(newIdx < unitTestIdx, "'pre-all' must come before 'unit-test'"); Review Comment: ⚠️ **Test gap: reversed `before=` anchor order not covered** `before="unit-test,integration-test"` happens to be in lifecycle order so `get(0)` = `"unit-test"` (correct). Add a test with reversed order to pin the behavior: ```java @Test void commaSeparatedBeforeAnchorsUsesEarliestRegardlessOfStringOrder() { Lifecycle base = defaultLifecycle(); // before="integration-test,unit-test" — reversed string order, unit-test is still the earliest PhaseEnrichedLifecycle enriched = new PhaseEnrichedLifecycle(base, List.of(ip("pre-all", "verify", null, "integration-test,unit-test"))); List<String> computed = registry().computePhases(enriched); int unitTestIdx = computed.indexOf("unit-test"); int newIdx = computed.indexOf("pre-all"); assertTrue(unitTestIdx >= 0, "'unit-test' must appear"); assertTrue(newIdx >= 0, "'pre-all' must appear"); assertTrue(newIdx < unitTestIdx, "'pre-all' must come before 'unit-test' regardless of string order"); } ``` ########## impl/maven-core/src/main/java/org/apache/maven/internal/impl/PhaseEnrichedLifecycle.java: ########## @@ -0,0 +1,481 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.internal.impl; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.apache.maven.api.Lifecycle; +import org.apache.maven.api.model.Plugin; + +/** + * A {@link Lifecycle} decorator that injects custom phases into the lifecycle phase tree. + * + * <p>Maven 4 lifecycles carry two parallel phase trees: + * <ul> + * <li>{@link Lifecycle#phases()} — the native Maven 4 hierarchical tree.</li> + * <li>{@link Lifecycle#v3phases()} — a flatter, Maven-3-compatible tree used exclusively by + * {@link DefaultLifecycleRegistry#computePhases} to produce the ordered phase list.</li> + * </ul> + * + * <p>Injection strategy: + * <ul> + * <li>In {@code phases()}: the new phase is inserted as a child of the named {@code parent} + * container, positioned by {@code after}/{@code before} among that container's children. + * The injected phase's {@code AFTER} link points to its immediate left sibling so that + * {@code computePhases()} can build a correct DAG edge.</li> + * <li>In {@code v3phases()}: the anchor ({@code after} or {@code before}) is searched + * recursively; the new phase is inserted as a sibling with an explicit {@code AFTER} link + * to that anchor. This is sufficient for {@code computePhases()} which resolves order + * from DAG edges. Falls back to top-level append if the anchor is not found.</li> + * </ul> + * + * <p>Only {@code AFTER} links are emitted; {@code BEFORE} links are omitted to avoid + * introducing cycles when {@code addPhases()} also adds implicit sibling-ordering edges. + * + * <p>{@code after} and {@code before} accept comma-separated lists when multiple anchors are + * needed, and both can be specified together to constrain the insertion from both sides. + * + * <p>Example {@code reactor.xml} declarations: + * <pre>{@code + * <phase name="pre-integration" parent="verify" after="unit-test" before="integration-test"/> + * <phase name="post-all" parent="verify" after="unit-test,integration-test"/> + * }</pre> + * + * @since 4.1.0 + */ +public class PhaseEnrichedLifecycle implements Lifecycle { + + /** + * Descriptor of a single phase to inject. + * + * @param name name of the new phase + * @param parent name of the parent container phase (in the Maven 4 tree) whose children + * list receives the injection + * @param after comma-separated sibling anchor(s): insert after the latest (rightmost) + * match; also used as DAG anchor(s) in v3phases (nullable) + * @param before comma-separated sibling anchor(s): insert before the earliest (leftmost) + * match (nullable). Can be combined with {@code after}. + */ + public record InjectedPhase(String name, String parent, String after, String before) {} + + private final Lifecycle delegate; + private final Collection<Phase> enrichedPhases; + private final Collection<Phase> enrichedV3Phases; + + public PhaseEnrichedLifecycle(Lifecycle delegate, List<InjectedPhase> injections) { + this.delegate = delegate; + this.enrichedPhases = injectAll(new ArrayList<>(delegate.phases()), injections, false); + this.enrichedV3Phases = injectAll(new ArrayList<>(delegate.v3phases()), injections, true); + } + + // ------------------------------------------------------------------------- + // Lifecycle delegation + // ------------------------------------------------------------------------- + + @Override + public String id() { + return delegate.id(); + } + + @Override + public Collection<Phase> phases() { + return enrichedPhases; + } + + @Override + public Collection<Phase> v3phases() { + return enrichedV3Phases; + } + + @Override + public Stream<Phase> allPhases() { + return enrichedPhases.stream().flatMap(Phase::allPhases); + } + + @Override + public Collection<Alias> aliases() { + return delegate.aliases(); + } + + // ------------------------------------------------------------------------- + // Top-level dispatcher + // ------------------------------------------------------------------------- + + /** + * Applies all injections to the given phase tree. + * + * @param phases mutable top-level phase list to enrich + * @param injections phases to inject + * @param v3Mode when {@code true} uses v3-style injection (sibling of anchor with + * explicit AFTER link); when {@code false} uses the parent-based injection + * @return unmodifiable enriched list + */ + private static Collection<Phase> injectAll(List<Phase> phases, List<InjectedPhase> injections, boolean v3Mode) { + for (InjectedPhase inj : injections) { + if (v3Mode) { + phases = injectV3(phases, inj); + } else { + phases = injectMaven4(phases, inj); + } + } + return Collections.unmodifiableList(phases); + } + + // ------------------------------------------------------------------------- + // Maven 4 tree injection (phases()) + // ------------------------------------------------------------------------- + + /** + * Injects into the Maven 4 tree by finding the named {@code parent} node and inserting + * the new phase among its direct children. + * + * <p>Falls back to appending at the top level (no link) if the parent is not found. + */ + private static List<Phase> injectMaven4(List<Phase> phases, InjectedPhase inj) { + if (inj.parent() == null) { + return insertAmongSiblings(phases, inj); + } + Optional<List<Phase>> result = tryInjectIntoParent(phases, inj); + if (result.isPresent()) { + return result.get(); + } + // Parent not found — append at top level without a link + List<Phase> fallback = new ArrayList<>(phases); + fallback.add(buildPhase(inj.name(), null)); + return fallback; + } + + /** + * Recursively searches for {@code inj.parent()} in the tree rooted at {@code phases} and, + * when found, inserts the new phase among that node's children. + * + * @return the updated top-level list wrapped in an {@code Optional}, or empty if the parent + * was not found at this level or any descendant level + */ + private static Optional<List<Phase>> tryInjectIntoParent(List<Phase> phases, InjectedPhase inj) { + for (int i = 0; i < phases.size(); i++) { + Phase phase = phases.get(i); + if (inj.parent().equals(phase.name())) { + // Found the parent: insert among its children + List<Phase> newChildren = insertAmongSiblings(new ArrayList<>(phase.phases()), inj); + List<Phase> result = new ArrayList<>(phases); + result.set(i, withChildren(phase, newChildren)); + return Optional.of(result); + } + // Recurse into descendants + if (!phase.phases().isEmpty()) { + Optional<List<Phase>> sub = tryInjectIntoParent(new ArrayList<>(phase.phases()), inj); + if (sub.isPresent()) { + List<Phase> result = new ArrayList<>(phases); + result.set(i, withChildren(phase, sub.get())); + return Optional.of(result); + } + } + } + return Optional.empty(); + } + + // ------------------------------------------------------------------------- + // v3 tree injection (v3phases() / computePhases) + // ------------------------------------------------------------------------- + + /** + * Injects into the v3 tree by searching for the {@code after}/{@code before} anchors anywhere + * in the tree and inserting the new phase as a sibling with explicit {@code AFTER} links to + * all {@code after} anchors. + * + * <p>The explicit links are what {@code computePhases()} uses for DAG ordering — the exact + * position in the v3 tree is secondary. Falls back to top-level append when no anchor is + * found in the v3 tree. + */ + private static List<Phase> injectV3(List<Phase> phases, InjectedPhase inj) { + List<String> afterAnchors = inj.after() != null ? splitAnchors(inj.after()) : List.of(); + List<String> beforeAnchors = inj.before() != null ? splitAnchors(inj.before()) : List.of(); + + // Primary anchor for tree positioning: prefer the last after anchor, + // fall back to the first before anchor + String primaryAnchor = !afterAnchors.isEmpty() + ? afterAnchors.get(afterAnchors.size() - 1) + : (!beforeAnchors.isEmpty() ? beforeAnchors.get(0) : null); Review Comment: ⚠️ **`before`-only comma list: v3 tree positioning uses string order, not lifecycle order** `primaryAnchor = beforeAnchors.get(0)` picks the first element in the comma-separated string. But the intended semantic is the *earliest in lifecycle order* — which is what `insertAmongSiblings` correctly implements (`i < upperBound`). When string order and lifecycle order differ, the two trees disagree. For `before="integration-test,unit-test"` (reversed order): - `beforeAnchors.get(0)` = `"integration-test"` (later in lifecycle, index ~8) - `tryInjectV3Sibling` finds `integration-test` and inserts BEFORE it in the v3 list - `addPhases` adds sibling-ordering edges: `AFTER(unit-test) → BEFORE(new-phase)`, placing `new-phase` AFTER `unit-test` - Result: phase ends up between `unit-test` and `integration-test`, violating "before unit-test" `insertAmongSiblings` (Maven 4 tree) correctly resolves this by scanning for the minimum-index anchor (`i < upperBound`). `injectV3` should do the same — find the earliest lifecycle-positioned anchor, not the first string token. Fix: make `injectV3` mirror `insertAmongSiblings`'s logic for the before case — find the anchor that appears earliest in the flattened v3 phase list. The simplest fix is to pass all `before` anchors to `tryInjectV3Sibling` and scan rather than assuming string position order, or (simpler) pick the minimum by scanning `phases` once before calling into the tree search. -- 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]
