gnodet-bot commented on code in PR #13612: URL: https://github.com/apache/maven/pull/13612#discussion_r4308281915
########## api/maven-api-core/src/main/java/org/apache/maven/api/services/LifecycleExecutor.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.api.services; + +import java.util.List; +import java.util.function.Predicate; + +import org.apache.maven.api.MojoExecution; +import org.apache.maven.api.Project; +import org.apache.maven.api.Service; +import org.apache.maven.api.Session; +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; + +/** + * Calculates and executes Maven lifecycle plans. + * + * @since 4.1.0 + */ +@Experimental +public interface LifecycleExecutor extends Service { Review Comment: ⚠️ **Naming collision with internal `org.apache.maven.lifecycle.LifecycleExecutor`** The same simple name exists in two packages: `org.apache.maven.api.services.LifecycleExecutor` (this new type) and `org.apache.maven.lifecycle.LifecycleExecutor` (the long-standing internal interface that runs the full build). `DefaultLifecycleExecutor` in `impl/maven-core` already lives between both namespaces and imports from both. IDE embedders implementing against the Maven 4 API — the stated audience for this service — will face import conflicts as soon as they touch any lifecycle-adjacent internal APIs. This is new `@Experimental` API, so renaming is cheap now. `LifecyclePlanExecutor` or `FilteredLifecycleExecutor` would be unambiguous. ########## impl/maven-core/src/test/java/org/apache/maven/internal/impl/DefaultLifecycleExecutorTest.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.List; + +import org.apache.maven.execution.MavenSession; +import org.apache.maven.lifecycle.MavenExecutionPlan; +import org.apache.maven.lifecycle.internal.LifecycleExecutionPlanCalculator; +import org.apache.maven.lifecycle.internal.LifecycleTask; +import org.apache.maven.lifecycle.internal.LifecycleTaskSegmentCalculator; +import org.apache.maven.lifecycle.internal.MojoExecutor; +import org.apache.maven.lifecycle.internal.TaskSegment; +import org.apache.maven.plugin.MojoExecution; +import org.apache.maven.plugin.descriptor.MojoDescriptor; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.apache.maven.project.MavenProject; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.same; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class DefaultLifecycleExecutorTest { + + @Test + void executesFilteredMojosAsOnePlanAndRestoresCurrentProject() throws Exception { Review Comment: 💡 **Add a failure-path test to exercise the `finally` block** The current test only covers the success path. The `finally { mavenSession.setCurrentProject(previousProject); }` in `DefaultLifecycleExecutor` is the critical safety net for embedders — without it, a lifecycle execution failure would corrupt `currentProject` state in the session. Add a test where `mojoExecutor.execute()` throws to verify the restore still fires: ```java @Test void restoresCurrentProjectOnExecutionFailure() throws Exception { InternalMavenSession session = mock(InternalMavenSession.class); MavenSession mavenSession = mock(MavenSession.class); LifecycleTaskSegmentCalculator taskSegmentCalculator = mock(LifecycleTaskSegmentCalculator.class); LifecycleExecutionPlanCalculator executionPlanCalculator = mock(LifecycleExecutionPlanCalculator.class); MojoExecutor mojoExecutor = mock(MojoExecutor.class); MavenProject project = new MavenProject(); MavenProject previousProject = new MavenProject(); DefaultProject apiProject = mock(DefaultProject.class); MavenExecutionPlan plan = mock(MavenExecutionPlan.class); TaskSegment segment = new TaskSegment(false, new LifecycleTask("verify")); when(session.getMavenSession()).thenReturn(mavenSession); when(mavenSession.getCurrentProject()).thenReturn(previousProject); when(apiProject.getProject()).thenReturn(project); when(taskSegmentCalculator.calculateTaskSegments(mavenSession, List.of("verify"))) .thenReturn(List.of(segment)); when(executionPlanCalculator.calculateExecutionPlan(mavenSession, project, segment.getTasks())) .thenReturn(plan); when(plan.getMojoExecutions()).thenReturn(List.of()); doThrow(new org.apache.maven.lifecycle.LifecycleExecutionException("boom")) .when(mojoExecutor).execute(eq(mavenSession), any()); DefaultLifecycleExecutor executor = new DefaultLifecycleExecutor(taskSegmentCalculator, executionPlanCalculator, mojoExecutor); assertThrows(LifecycleExecutionException.class, () -> executor.execute(session, apiProject, List.of("verify"), e -> true)); verify(mavenSession).setCurrentProject(same(previousProject)); } ``` ########## impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleExecutor.java: ########## @@ -0,0 +1,106 @@ +/* + * 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 javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Singleton; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.Predicate; + +import org.apache.maven.api.MojoExecution; +import org.apache.maven.api.Project; +import org.apache.maven.api.Service; +import org.apache.maven.api.Session; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.services.LifecycleExecutionException; +import org.apache.maven.api.services.LifecycleExecutor; +import org.apache.maven.execution.MavenSession; +import org.apache.maven.lifecycle.MavenExecutionPlan; +import org.apache.maven.lifecycle.internal.LifecycleExecutionPlanCalculator; +import org.apache.maven.lifecycle.internal.LifecycleTaskSegmentCalculator; +import org.apache.maven.lifecycle.internal.MojoExecutor; +import org.apache.maven.lifecycle.internal.TaskSegment; +import org.apache.maven.project.MavenProject; +import org.eclipse.sisu.Typed; + +import static java.util.Objects.requireNonNull; +import static org.apache.maven.internal.impl.CoreUtils.cast; + +@Named +@Typed({LifecycleExecutor.class, Service.class}) +@Singleton +public class DefaultLifecycleExecutor implements LifecycleExecutor { + + private final LifecycleTaskSegmentCalculator taskSegmentCalculator; + private final LifecycleExecutionPlanCalculator executionPlanCalculator; + private final MojoExecutor mojoExecutor; + + @Inject + public DefaultLifecycleExecutor( + LifecycleTaskSegmentCalculator taskSegmentCalculator, + LifecycleExecutionPlanCalculator executionPlanCalculator, + MojoExecutor mojoExecutor) { + this.taskSegmentCalculator = taskSegmentCalculator; + this.executionPlanCalculator = executionPlanCalculator; + this.mojoExecutor = mojoExecutor; + } + + @Override + public void execute( + @Nonnull Session requestedSession, + @Nonnull Project project, + @Nonnull List<String> tasks, + @Nonnull Predicate<MojoExecution> filter) { + requireNonNull(requestedSession, "session"); + requireNonNull(project, "project"); + requireNonNull(tasks, "tasks").forEach(task -> requireNonNull(task, "task")); + requireNonNull(filter, "filter"); + + InternalMavenSession internalSession = InternalMavenSession.from(requestedSession); + MavenSession mavenSession = internalSession.getMavenSession(); + MavenProject mavenProject = + cast(DefaultProject.class, project, "project").getProject(); + MavenProject previousProject = mavenSession.getCurrentProject(); + + try { + mavenSession.setCurrentProject(mavenProject); + List<TaskSegment> taskSegments = taskSegmentCalculator.calculateTaskSegments(mavenSession, tasks); + List<org.apache.maven.plugin.MojoExecution> filteredPlan = new ArrayList<>(); + + for (TaskSegment taskSegment : taskSegments) { + MavenExecutionPlan executionPlan = executionPlanCalculator.calculateExecutionPlan( + mavenSession, mavenProject, taskSegment.getTasks()); + for (org.apache.maven.plugin.MojoExecution execution : executionPlan.getMojoExecutions()) { + if (filter.test(new DefaultMojoExecution(internalSession, execution))) { + filteredPlan.add(execution); + } + } + } + + mojoExecutor.execute(mavenSession, filteredPlan); + } catch (Exception e) { Review Comment: ⚠️ **`catch(Exception e)` conflates execution failures with buggy filter predicates** `mojoExecutor.execute()` throws `org.apache.maven.lifecycle.LifecycleExecutionException` (a checked exception representing a genuine plan execution failure). That should be caught and wrapped. But the filter lambda at line 93 — `filter.test(new DefaultMojoExecution(...))` — can also throw `RuntimeException` (e.g. a `NullPointerException` or `ClassCastException` from a buggy embedder predicate). Both currently land in `LifecycleExecutionException("Unable to execute filtered lifecycle plan", e)`. The caller cannot distinguish a real plan failure from a broken filter. Move the filter loop outside the try block, so only `mojoExecutor.execute()` and the plan calculation calls are caught: ```suggestion List<org.apache.maven.plugin.MojoExecution> filteredPlan = new ArrayList<>(); try { mavenSession.setCurrentProject(mavenProject); List<TaskSegment> taskSegments = taskSegmentCalculator.calculateTaskSegments(mavenSession, tasks); for (TaskSegment taskSegment : taskSegments) { MavenExecutionPlan executionPlan = executionPlanCalculator.calculateExecutionPlan( mavenSession, mavenProject, taskSegment.getTasks()); for (org.apache.maven.plugin.MojoExecution execution : executionPlan.getMojoExecutions()) { if (filter.test(new DefaultMojoExecution(internalSession, execution))) { filteredPlan.add(execution); } } } } catch (Exception e) { throw new LifecycleExecutionException("Unable to calculate filtered lifecycle plan", e); } try { mojoExecutor.execute(mavenSession, filteredPlan); } catch (Exception e) { throw new LifecycleExecutionException("Unable to execute filtered lifecycle plan", e); ``` -- 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]
