gnodet-bot commented on code in PR #13392: URL: https://github.com/apache/maven/pull/13392#discussion_r4277289820
########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/filter/FilterPredicate.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.lifecycle.internal.filter; + +import org.apache.maven.api.MojoExecution; + +/** + * Predicate for filtering {@link MojoExecution} entries from the lifecycle build plan. + * + * <p>Used by {@code -Dmaven.lifecycle.filter} to remove matching executions before the build starts. Review Comment: 📝 **Inaccurate Javadoc: filtering happens at execution time, not plan-building time** "before the build starts" implies this is a build-plan filter applied during lifecycle plan calculation. In reality filtering happens inside `MojoExecutor.execute()` — at execution time, after the plan is built. This distinction matters: the mojo *is* in the plan and *will* be skipped at runtime, not removed from the plan upfront. ```suggestion * <p>Used by {@code -Dmaven.lifecycle.filter} to skip matching executions at runtime. ``` ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java: ########## @@ -162,20 +165,43 @@ public void execute(final MavenSession session, final List<MojoExecution> mojoEx final PhaseRecorder phaseRecorder = new PhaseRecorder(session.getCurrentProject()); + // Resolve the mojo execution filter once per project execution. + final List<FilterPredicate> filterPredicates = resolveFilterPredicates(session); + mojosExecutionStrategy.get().execute(mojoExecutions, session, new MojoExecutionRunner() { @Override public void run(MojoExecution mojoExecution) throws LifecycleExecutionException { - MojoExecutor.this.execute(session, mojoExecution, dependencyContext, phaseRecorder); + MojoExecutor.this.execute(session, mojoExecution, dependencyContext, phaseRecorder, filterPredicates); } }); } + /** + * Resolves the mojo execution filter predicates for this build. + * Uses {@code -Dmaven.lifecycle.filter} user property; returns an empty list if absent. + */ + private List<FilterPredicate> resolveFilterPredicates(MavenSession session) throws LifecycleExecutionException { + Properties userProps = session.getUserProperties(); + String filterExpression = userProps != null ? userProps.getProperty(MojoExecutionFilter.PROPERTY_NAME) : null; + try { + return MojoExecutionFilter.parse(filterExpression); + } catch (IllegalArgumentException e) { + throw new LifecycleExecutionException( + "Invalid value for '" + MojoExecutionFilter.PROPERTY_NAME + "': " + e.getMessage(), e); + } + } + private void execute( MavenSession session, MojoExecution mojoExecution, DependencyContext dependencyContext, - PhaseRecorder phaseRecorder) + PhaseRecorder phaseRecorder, + List<FilterPredicate> filterPredicates) throws LifecycleExecutionException { + if (MojoExecutionFilter.matches(mojoExecution, filterPredicates)) { + LOGGER.info("Skipping {} (filtered by {})", mojoExecution, MojoExecutionFilter.PROPERTY_NAME); + return; Review Comment: ⚠️ **Missing `MojoSkipped` execution event** The existing skip path for offline mojos (line 234) fires `eventCatapult.fire(ExecutionEvent.Type.MojoSkipped, session, mojoExecution)` so that build event listeners, Maven extensions, and lifecycle reporters are notified. The filter skip path returns silently, meaning any registered `ExecutionListener` will see the mojo disappear without a corresponding event. This breaks extensions that track build timelines, generate build reports, or react to skipped mojos. ```suggestion if (MojoExecutionFilter.matches(mojoExecution, filterPredicates)) { LOGGER.info("Skipping {} (filtered by {})", mojoExecution, MojoExecutionFilter.PROPERTY_NAME); eventCatapult.fire(ExecutionEvent.Type.MojoSkipped, session, mojoExecution); return; } ``` ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/filter/MojoExecutionFilter.java: ########## @@ -0,0 +1,214 @@ +/* + * 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.lifecycle.internal.filter; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.Constants; +import org.apache.maven.api.Dependency; +import org.apache.maven.api.MojoExecution; +import org.apache.maven.api.Plugin; +import org.apache.maven.api.model.PluginExecution; +import org.apache.maven.api.plugin.descriptor.PluginDescriptor; +import org.apache.maven.api.plugin.descriptor.lifecycle.Lifecycle; +import org.apache.maven.api.xml.XmlNode; + +/** + * Parses the {@code maven.lifecycle.filter} user property value into a list of {@link FilterPredicate}s, + * and applies them at mojo execution time in {@code MojoExecutor}.</p> Review Comment: 📝 **Dangling `</p>` with no opening `<p>`** The first paragraph of the class-level Javadoc ends with `.</p>` but has no matching `<p>` opener. Maven Javadoc convention is `<p>` at the start of a new paragraph, not `</p>` as a paragraph closer (which is valid HTML5 but inconsistent with the rest of the codebase and confuses some Javadoc renderers). ```suggestion * and applies them at mojo execution time in {@code MojoExecutor}. ``` ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/filter/MojoExecutionFilter.java: ########## @@ -0,0 +1,214 @@ +/* + * 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.lifecycle.internal.filter; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Optional; + +import org.apache.maven.api.Artifact; +import org.apache.maven.api.Constants; +import org.apache.maven.api.Dependency; +import org.apache.maven.api.MojoExecution; +import org.apache.maven.api.Plugin; +import org.apache.maven.api.model.PluginExecution; +import org.apache.maven.api.plugin.descriptor.PluginDescriptor; +import org.apache.maven.api.plugin.descriptor.lifecycle.Lifecycle; +import org.apache.maven.api.xml.XmlNode; + +/** + * Parses the {@code maven.lifecycle.filter} user property value into a list of {@link FilterPredicate}s, + * and applies them at mojo execution time in {@code MojoExecutor}.</p> + * + * <p>The property value is a comma-separated list of predicates, OR-ed together: + * a mojo execution matching <em>any</em> predicate is skipped. + * + * <p>Supported predicate forms: + * <ul> + * <li>{@code *} — skip all mojo executions</li> + * <li>{@code :A} — skip by artifactId (e.g. {@code :maven-enforcer-plugin})</li> + * <li>{@code G:A} — skip by groupId:artifactId</li> + * <li>{@code P} — skip by plugin prefix (e.g. {@code enforcer})</li> + * <li>{@code P:v:g} — skip by prefix:version:goal</li> + * <li>{@code P:v:g@e} — skip by prefix:version:goal@executionId</li> + * <li>{@code phase(name)} — skip all mojos bound to the named phase</li> + * </ul> + * + * @since 4.1.0 + */ +public class MojoExecutionFilter { + + /** The name of the user property that activates the filter. */ + public static final String PROPERTY_NAME = Constants.MAVEN_LIFECYCLE_FILTER; + + private MojoExecutionFilter() { + // utility class + } + + /** + * Parses a filter expression string into a list of {@link FilterPredicate}s. + * + * @param expression the comma-separated filter expression (may be {@code null} or blank) + * @return list of parsed predicates; empty if the expression is absent or blank + */ + public static List<FilterPredicate> parse(String expression) { + if (expression == null || expression.isBlank()) { + return List.of(); + } + List<FilterPredicate> predicates = new ArrayList<>(); + for (String token : expression.split(",")) { + token = token.strip(); + if (token.isEmpty()) { + continue; + } + predicates.add(parseToken(token)); + } + return List.copyOf(predicates); + } + + private static FilterPredicate parseToken(String token) { + if (token.startsWith("phase(")) { + if (!token.endsWith(")")) { + throw new IllegalArgumentException("Invalid phase() predicate '" + token + "': missing closing ')'"); + } + String phaseName = token.substring("phase(".length(), token.length() - 1); + if (phaseName.isBlank()) { + throw new IllegalArgumentException("phase() predicate requires a non-blank phase name"); + } + if (phaseName.contains(")")) { + throw new IllegalArgumentException( + "Invalid phase() predicate '" + token + "': phase name must not contain ')'"); + } + return new PhasePredicate(phaseName); + } + return CoordinatePredicate.parse(token); + } + + /** + * Returns {@code true} if the given legacy mojo execution matches any predicate in the list + * and should be skipped. + * + * <p>Adapts the legacy {@link org.apache.maven.plugin.MojoExecution} into a lightweight + * {@link MojoExecution} view — only the fields accessed by the predicates + * ({@code plugin()}, {@code goal()}, {@code executionId()}, {@code lifecyclePhase()}) + * are populated. No session is required. + * + * @param execution the legacy mojo execution to test + * @param predicates the predicates to apply (OR-ed) + * @return {@code true} if the execution should be skipped + */ + public static boolean matches(org.apache.maven.plugin.MojoExecution execution, List<FilterPredicate> predicates) { + if (predicates.isEmpty()) { + return false; + } + MojoExecution apiExecution = adapt(execution); + for (FilterPredicate predicate : predicates) { + if (predicate.matches(apiExecution)) { + return true; + } + } + return false; + } + + /** + * Adapts a legacy {@link org.apache.maven.plugin.MojoExecution} to the API + * {@link MojoExecution} interface using only the fields needed by the predicates. + * No session is required. + */ + private static MojoExecution adapt(org.apache.maven.plugin.MojoExecution legacy) { + org.apache.maven.plugin.descriptor.MojoDescriptor legacyMojo = legacy.getMojoDescriptor(); + org.apache.maven.plugin.descriptor.PluginDescriptor legacyPlugin = + legacyMojo != null ? legacyMojo.getPluginDescriptor() : null; + PluginDescriptor descriptorV4 = legacyPlugin != null ? legacyPlugin.getPluginDescriptorV4() : null; + + Plugin plugin = new Plugin() { + @Override + public org.apache.maven.api.model.Plugin getModel() { + return null; + } + + @Override + public PluginDescriptor getDescriptor() { + return descriptorV4; Review Comment: 💡 **`@Nonnull` contract violated by the anonymous `Plugin` impl** `Plugin.getDescriptor()` and `MojoExecution.descriptor()` are both annotated `@Nonnull`, but the anonymous implementations in `adapt()` return `null` when the legacy plugin or mojo descriptor is absent. `CoordinatePredicate.matches()` already null-guards the descriptor correctly, so there is no NPE today. However, any future code trusting the `@Nonnull` contract (e.g. an extension implementing `FilterPredicate` that calls `execution.plugin().getDescriptor().getGoalPrefix()` without a null check) will throw. Consider returning a sentinel/empty `PluginDescriptor` instead of `null`, or at minimum annotating the anonymous method override with `@Nullable` to signal the violation to static analysis tools. -- 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]
