gnodet-bot commented on code in PR #1086: URL: https://github.com/apache/maven/pull/1086#discussion_r4178024137
########## maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecyclePluginSkipper.java: ########## @@ -0,0 +1,149 @@ +/* + * 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; + +import java.util.List; +import java.util.Objects; +import java.util.function.Predicate; + +import org.apache.maven.execution.MavenSession; +import org.apache.maven.plugin.MojoExecution; +import org.apache.maven.plugin.descriptor.MojoDescriptor; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.codehaus.plexus.component.annotations.Component; +import org.eclipse.aether.util.ConfigUtils; + +/** + * Lifecycle plugin skipper. + * + * @since TBD + */ +@Component(role = LifecyclePluginSkipper.class) +public class LifecyclePluginSkipper { + private static final String PARSED_FILTER_KEY = LifecyclePluginSkipper.class.getName() + ".filter"; Review Comment: 💡 **Naming clarity:** The constant names could better reflect the `removeIf` semantics. `NO_FILTER` returning `false` (meaning "don't remove anything") is correct but reads counterintuitively at first glance. A name like `REMOVE_NOTHING` or `SKIP_NONE` would be immediately clear. Similarly, `ANY_STRING` reads like a filter that accepts any string, but in the `removeIf` context it means "match any string *for removal*". Consider `MATCH_ANY` to stay neutral. ########## maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecyclePluginSkipper.java: ########## @@ -0,0 +1,149 @@ +/* + * 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; + +import java.util.List; +import java.util.Objects; +import java.util.function.Predicate; + +import org.apache.maven.execution.MavenSession; +import org.apache.maven.plugin.MojoExecution; +import org.apache.maven.plugin.descriptor.MojoDescriptor; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.codehaus.plexus.component.annotations.Component; +import org.eclipse.aether.util.ConfigUtils; + +/** + * Lifecycle plugin skipper. + * + * @since TBD + */ +@Component(role = LifecyclePluginSkipper.class) +public class LifecyclePluginSkipper { + private static final String PARSED_FILTER_KEY = LifecyclePluginSkipper.class.getName() + ".filter"; + private static final String MAVEN_LIFECYCLE_FILTER_KEY = "maven.lifecycle.filter"; + private static final Predicate<MojoExecution> NO_FILTER = t -> false; + private static final Predicate<String> ANY_STRING = t -> true; + + void processMojoExecutions(MavenSession session, List<MojoExecution> executions) { + Predicate<MojoExecution> filter = getFilter(session); + if (filter == NO_FILTER) { + return; + } + executions.removeIf(filter); + } + + @SuppressWarnings("unchecked") + private Predicate<MojoExecution> getFilter(MavenSession mavenSession) { + return (Predicate<MojoExecution>) mavenSession + .getRepositorySession() + .getData() + .computeIfAbsent(PARSED_FILTER_KEY, () -> createFilter(mavenSession)); + } + + private Predicate<MojoExecution> createFilter(MavenSession mavenSession) { + String filterString = + ConfigUtils.getString(mavenSession.getRepositorySession(), null, MAVEN_LIFECYCLE_FILTER_KEY); + if (filterString == null) { + return NO_FILTER; + } + + Predicate<MojoExecution> result = null; + String[] filterExpressions = filterString.split(","); + for (String filterExpression : filterExpressions) { + if (result == null) { + result = parseFilterExpression(mavenSession.getPluginGroups(), filterExpression); + } else { + result = result.or(parseFilterExpression(mavenSession.getPluginGroups(), filterExpression)); + } + } + return result; + } + + private Predicate<MojoExecution> parseFilterExpression(List<String> pluginGroups, String filterExpression) { + Predicate<String> groupIdPredicate; + Predicate<String> artifactIdPredicate = ANY_STRING; + Predicate<String> goalPredicate = ANY_STRING; + Predicate<String> executionIdPredicate = ANY_STRING; + + String[] elements = filterExpression.split(":"); + if (elements.length == 0 || elements.length > 4) { + throw new IllegalArgumentException("Unsupported lifecycle filter expression: " + filterExpression); + } + String groupId = elements[0]; + if (Objects.equals(groupId, "")) { + groupIdPredicate = pluginGroups::contains; + } else if (Objects.equals(groupId, "*")) { + groupIdPredicate = ANY_STRING; Review Comment: ⚠️ **Missing input validation:** `parseFilterExpression` throws a raw `IllegalArgumentException` when `elements.length > 4`, which will propagate up through `calculateExecutionPlan` and crash the build with an unhelpful stacktrace. Since this is user-facing input (`-Dmaven.lifecycle.filter=...`), the error should be caught at the `processMojoExecutions` entry point or wrapped in a lifecycle exception with a clear message explaining the expected syntax. Also: this validation misses the case where `elements.length == 0` — `String.split(":")` never returns an empty array in Java (even `"".split(":")` returns `[""]`, length 1), so that branch is dead code. ########## maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecyclePluginSkipper.java: ########## @@ -0,0 +1,149 @@ +/* + * 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; + +import java.util.List; +import java.util.Objects; +import java.util.function.Predicate; + +import org.apache.maven.execution.MavenSession; +import org.apache.maven.plugin.MojoExecution; +import org.apache.maven.plugin.descriptor.MojoDescriptor; +import org.apache.maven.plugin.descriptor.PluginDescriptor; +import org.codehaus.plexus.component.annotations.Component; +import org.eclipse.aether.util.ConfigUtils; + +/** + * Lifecycle plugin skipper. Review Comment: 📝 **Javadoc:** `@since TBD` — fine for draft, but the class-level Javadoc should describe what the filter expression syntax is and link to the JIRA issue. This will be the primary reference for anyone trying to understand the feature. ########## maven-core/src/test/java/org/apache/maven/lifecycle/internal/LifecycleExecutionPlanCalculatorTest.java: ########## @@ -62,7 +62,8 @@ public static LifecycleExecutionPlanCalculator createExecutionPlaceCalculator( new BuildPluginManagerStub(), DefaultLifecyclesStub.createDefaultLifecycles(), mojoDescriptorCreator, - lifecyclePluginResolver); + lifecyclePluginResolver, + new LifecyclePluginSkipper()); Review Comment: ⚠️ **Missing dedicated tests:** `LifecyclePluginSkipper` has non-trivial parsing logic (comma-separated expressions, colon-separated segments, wildcard matching, default group handling) but zero direct unit tests. The only test coverage is passing a no-op instance here. A `LifecyclePluginSkipperTest` should cover at minimum: - Empty/null filter string → no filtering - Single expression: `G:A:g:e` exact match - Wildcards: `*`, `*:*`, `G:*:g` - Default groups: empty groupId → `pluginGroups::contains` - Comma-separated multiple expressions - Malformed expressions (> 4 segments) - Edge cases: trailing colons, empty segments -- 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]
