gnodet-bot commented on code in PR #13658: URL: https://github.com/apache/maven/pull/13658#discussion_r4346732417
########## impl/maven-plugin-mixin/src/main/java/org/apache/maven/plugin/processor/MojoDescriptorProcessor.java: ########## @@ -0,0 +1,464 @@ +/* + * 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.plugin.processor; + +import javax.annotation.processing.AbstractProcessor; +import javax.annotation.processing.RoundEnvironment; +import javax.annotation.processing.SupportedAnnotationTypes; +import javax.annotation.processing.SupportedOptions; +import javax.lang.model.SourceVersion; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Modifier; +import javax.lang.model.element.TypeElement; +import javax.lang.model.element.VariableElement; +import javax.lang.model.type.DeclaredType; +import javax.lang.model.type.TypeKind; +import javax.lang.model.type.TypeMirror; +import javax.tools.Diagnostic; +import javax.tools.StandardLocation; +import javax.xml.stream.XMLStreamException; + +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.io.Writer; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; + +import org.apache.maven.api.plugin.annotations.After; +import org.apache.maven.api.plugin.annotations.Execute; +import org.apache.maven.api.plugin.annotations.Mojo; +import org.apache.maven.api.plugin.annotations.Resolution; +import org.apache.maven.api.plugin.descriptor.AfterLink; +import org.apache.maven.api.plugin.descriptor.MojoDescriptor; +import org.apache.maven.api.plugin.descriptor.PluginDescriptor; +import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter; + +/** + * JSR 269 annotation processor that generates Maven plugin descriptors ({@code META-INF/maven/plugin.xml}) + * and DI factory classes at compile time from {@link Mojo @Mojo}-annotated classes. + * + * <p>Plugin coordinates are read from compiler {@code -A} options, typically provided by the + * {@code maven-plugin-mixin}. + * + * @since 4.1.0 + */ +@SupportedAnnotationTypes("org.apache.maven.api.plugin.annotations.Mojo") +@SupportedOptions({ + MojoDescriptorProcessor.OPT_GROUP_ID, + MojoDescriptorProcessor.OPT_ARTIFACT_ID, + MojoDescriptorProcessor.OPT_VERSION, + MojoDescriptorProcessor.OPT_GOAL_PREFIX, + MojoDescriptorProcessor.OPT_NAME +}) +public class MojoDescriptorProcessor extends AbstractProcessor { + + static final String OPT_GROUP_ID = "maven.plugin.groupId"; + static final String OPT_ARTIFACT_ID = "maven.plugin.artifactId"; + static final String OPT_VERSION = "maven.plugin.version"; + static final String OPT_GOAL_PREFIX = "maven.plugin.goalPrefix"; + static final String OPT_NAME = "maven.plugin.name"; + + private final Map<String, TypeElement> mojoElements = new LinkedHashMap<>(); + private final Set<String> generatedFactoryNames = new LinkedHashSet<>(); + private String[] resolvedCoordinates; + + @Override + public SourceVersion getSupportedSourceVersion() { + return SourceVersion.latestSupported(); + } + + @Override + public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) { + for (TypeElement annotation : annotations) { + for (Element element : roundEnv.getElementsAnnotatedWith(annotation)) { + if (!(element instanceof TypeElement typeElement)) { + continue; + } + String fqn = typeElement.getQualifiedName().toString(); + if (generatedFactoryNames.contains(fqn)) { + continue; + } + if (typeElement.getKind() == ElementKind.INTERFACE) { + error("@Mojo must not be placed on an interface", element); + continue; + } + if (typeElement.getModifiers().contains(Modifier.ABSTRACT)) { + error("@Mojo must not be placed on an abstract class", element); + continue; + } + if (!mojoElements.containsKey(fqn)) { + mojoElements.put(fqn, typeElement); + String[] coords = resolveCoordinates(); + if (coords != null) { + try { + generateFactory(typeElement, coords); + } catch (IOException e) { + error("Failed to generate factory for " + fqn, e); + } + } + } + } + } + if (roundEnv.processingOver() && !mojoElements.isEmpty()) { + String[] coords = resolveCoordinates(); + if (coords != null) { + try { + writePluginXml(coords); + } catch (IOException e) { + error("Failed to generate plugin.xml", e); + } + } + } + return false; + } + + String[] resolveCoordinates() { + if (resolvedCoordinates != null) { + return resolvedCoordinates; + } + String groupId = getOption(OPT_GROUP_ID); + String artifactId = getOption(OPT_ARTIFACT_ID); + String version = getOption(OPT_VERSION); + String goalPrefix = getOption(OPT_GOAL_PREFIX); + String name = getOption(OPT_NAME); + if (isBlank(goalPrefix)) { + goalPrefix = deriveGoalPrefix(artifactId); + } + if (isBlank(name)) { + name = artifactId; + } + if (isBlank(groupId) || isBlank(artifactId) || isBlank(version)) { + error("Could not determine plugin coordinates. " + + "Supply -Amaven.plugin.groupId, -Amaven.plugin.artifactId and " + + "-Amaven.plugin.version via the maven-plugin-mixin."); + return null; Review Comment: ⚠️ **Duplicate error when coordinates are missing.** `resolveCoordinates()` is called once per mojo (inside the `process()` loop) and again in the `processingOver()` block. When coordinates are absent, the error is emitted on each call — for a plugin with one mojo, the compiler reports the same message twice; for N mojos, N+1 times. Two possible fixes: **Option A** — emit one error per missing option so the user sees exactly what's absent, and avoid the combined message that gets repeated: ```java if (isBlank(groupId)) error("Missing required option: -A" + OPT_GROUP_ID); if (isBlank(artifactId)) error("Missing required option: -A" + OPT_ARTIFACT_ID); if (isBlank(version)) error("Missing required option: -A" + OPT_VERSION); if (isBlank(groupId) || isBlank(artifactId) || isBlank(version)) return null; ``` Individual errors are de-duplicated by `javac` naturally (same error can't be emitted twice on the same element). This also fixes the test comment that claims "for each missing option." **Option B** — guard the error with a flag: `if (!coordinatesErrorReported) { error(...); coordinatesErrorReported = true; }` ########## impl/maven-plugin-mixin/src/test/java/org/apache/maven/plugin/processor/MojoDescriptorProcessorTest.java: ########## @@ -0,0 +1,693 @@ +/* + * 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.plugin.processor; + +import javax.tools.Diagnostic; +import javax.tools.DiagnosticCollector; +import javax.tools.FileObject; +import javax.tools.ForwardingJavaFileManager; +import javax.tools.JavaCompiler; +import javax.tools.JavaFileManager; +import javax.tools.JavaFileObject; +import javax.tools.SimpleJavaFileObject; +import javax.tools.StandardJavaFileManager; +import javax.tools.StandardLocation; +import javax.tools.ToolProvider; + +import java.io.ByteArrayOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.OutputStream; +import java.io.StringWriter; +import java.net.URI; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Unit tests for {@link MojoDescriptorProcessor} using the JDK in-process compiler. + * + * <p>Each test compiles a small in-memory source file that uses the Maven plugin + * annotations ({@code @Mojo}, {@code @Parameter}, etc.) from maven-api-core on + * the classpath, with the processor JAR on the processor path. Generated + * resources (plugin.xml) and source files (factory classes) are intercepted by + * a {@link CapturingFileManager} and asserted in-memory — no disk I/O.</p> + */ +class MojoDescriptorProcessorTest { + + /** Resolved classpath jars needed for compiling test sources. */ + private static List<File> classpathJars; + + /** The processor JAR (contains the compiled MojoDescriptorProcessor). */ + private static File processorJar; + + @BeforeAll + static void resolveClasspath() { + // Processor JAR — built from this module + processorJar = resolveFile( + "impl/maven-plugin-mixin/target/maven-plugin-mixin-4.1.0-SNAPSHOT.jar", + "maven-plugin-mixin/4.1.0-SNAPSHOT/maven-plugin-mixin-4.1.0-SNAPSHOT.jar"); + + // Annotation JARs needed on the compilation classpath + File apiCore = resolveFile( + "api/maven-api-core/target/maven-api-core-4.1.0-SNAPSHOT.jar", + "maven-api-core/4.1.0-SNAPSHOT/maven-api-core-4.1.0-SNAPSHOT.jar"); + File apiAnnotations = + resolveFile(null, "maven-api-annotations/4.1.0-SNAPSHOT/maven-api-annotations-4.1.0-SNAPSHOT.jar"); + + classpathJars = List.of(apiCore, apiAnnotations); + } + + /** + * Try to resolve a JAR first from the Maven multi-module project build output, then from the local Maven repo. + * + * <p>In a Maven build, {@code maven.multiModuleProjectDirectory} points to the project root, so + * {@code worktreeRelative} is resolved against that directory to find freshly-compiled JARs. + * The {@code processor.worktree} system property can override the root for IDE / manual runs.</p> + */ + private static File resolveFile(String worktreeRelative, String m2Relative) { + // Prefer the Maven multi-module project directory (set by Maven during build) + String worktreeBase = + System.getProperty("processor.worktree", System.getProperty("maven.multiModuleProjectDirectory", "")); + String m2Base = System.getProperty( + "processor.m2", System.getProperty("user.home") + "/.m2/repository/org/apache/maven"); + + if (worktreeRelative != null && !worktreeBase.isEmpty()) { + File f = new File(worktreeBase, worktreeRelative); + if (f.isFile()) { + return f; + } + } + if (m2Relative != null) { + File f = new File(m2Base, m2Relative); + if (f.isFile()) { + return f; + } + } + fail("Cannot resolve JAR: worktree=" + worktreeRelative + " m2=" + m2Relative); + return null; // unreachable + } + + // ------------------------------------------------------------------------- + // Test 1: Minimal @Mojo → plugin.xml + factory + // ------------------------------------------------------------------------- + + @Test + void minimalMojoGeneratesPluginXmlAndFactory() { + String source = """ + package com.example; + import org.apache.maven.api.plugin.annotations.Mojo; + @Mojo(name = "greet") + public class GreetMojo { + public void execute() {} + } + """; + + CompilationResult result = compile( + List.of(inMemorySource("com.example.GreetMojo", source)), + Map.of( + "maven.plugin.groupId", "com.example", + "maven.plugin.artifactId", "example-maven-plugin", + "maven.plugin.version", "1.0.0")); + + assertTrue(result.success, () -> "Compilation failed:\n" + result.diagnosticsToString()); + + // plugin.xml assertions + String pluginXml = result.getResource("META-INF/maven/plugin.xml"); + assertNotNull(pluginXml, "plugin.xml must be generated"); + assertContains(pluginXml, "<goal>greet</goal>"); + assertContains(pluginXml, "<implementation>com.example.GreetMojo</implementation>"); + assertContains(pluginXml, "<groupId>com.example</groupId>"); + assertContains(pluginXml, "<artifactId>example-maven-plugin</artifactId>"); + assertContains(pluginXml, "<version>1.0.0</version>"); + // goalPrefix derived from "example-maven-plugin": strips "-maven-plugin" suffix → "example" + assertContains(pluginXml, "<goalPrefix>example</goalPrefix>"); + assertContains(pluginXml, "<language>java</language>"); + // default projectRequired = true + assertContains(pluginXml, "<projectRequired>true</projectRequired>"); + + // Factory class assertion + String factorySource = result.getGeneratedSource("com.example.GreetMojoMojoFactory"); + assertNotNull(factorySource, "Factory source must be generated"); + assertContains( + factorySource, "@org.apache.maven.api.di.Named(\"com.example:example-maven-plugin:1.0.0:greet\")"); + assertContains(factorySource, "public class GreetMojoMojoFactory extends GreetMojo"); + } + + // ------------------------------------------------------------------------- + // Test 2: @Mojo with @Parameter fields (readonly → editable inversion) + // ------------------------------------------------------------------------- + + @Test + void mojoWithParametersGeneratesParametersBlock() { + String source = """ + package com.example; + import org.apache.maven.api.plugin.annotations.Mojo; + import org.apache.maven.api.plugin.annotations.Parameter; + @Mojo(name = "compile", defaultPhase = "compile") + public class CompileMojo { + @Parameter(property = "source", defaultValue = "17", required = true) + private String sourceVersion; + + @Parameter(readonly = true, property = "project.basedir") + private java.io.File basedir; + + @Parameter(name = "customName", alias = "alt") + private int threshold; + + public void execute() {} + } + """; + + CompilationResult result = compile( + List.of(inMemorySource("com.example.CompileMojo", source)), + Map.of( + "maven.plugin.groupId", "org.apache.maven.plugins", + "maven.plugin.artifactId", "maven-compiler-plugin", + "maven.plugin.version", "4.0.0")); + + assertTrue(result.success, () -> "Compilation failed:\n" + result.diagnosticsToString()); + + String pluginXml = result.getResource("META-INF/maven/plugin.xml"); + assertNotNull(pluginXml, "plugin.xml must be generated"); + + // defaultPhase + assertContains(pluginXml, "<phase>compile</phase>"); + + // goalPrefix derived from "maven-compiler-plugin" → "compiler" + assertContains(pluginXml, "<goalPrefix>compiler</goalPrefix>"); + + // <parameters> block + assertContains(pluginXml, "<parameters>"); + + // sourceVersion parameter + assertContains(pluginXml, "<name>sourceVersion</name>"); + assertContains(pluginXml, "<type>java.lang.String</type>"); + assertContains(pluginXml, "<required>true</required>"); + assertContains(pluginXml, "<expression>${source}</expression>"); + assertContains(pluginXml, "<defaultValue>17</defaultValue>"); + + // basedir: readonly=true → editable=false + assertContains(pluginXml, "<name>basedir</name>"); + // The readonly param should generate editable=false + assertTrue( + pluginXml.contains("<editable>false</editable>"), + "readonly=true should produce editable=false in plugin.xml"); + assertContains(pluginXml, "<expression>${project.basedir}</expression>"); + + // customName / alias + assertContains(pluginXml, "<name>customName</name>"); + assertContains(pluginXml, "<alias>alt</alias>"); + + // sourceVersion: readonly defaults to false → editable=true + // We need to check that the sourceVersion parameter has editable=true + // Since there are multiple editable elements, verify non-readonly defaults to editable=true + assertTrue( + pluginXml.contains("<editable>true</editable>"), + "readonly=false (default) should produce editable=true"); + } + + // ------------------------------------------------------------------------- + // Test 3: @Mojo with @After → afterLinks block + // ------------------------------------------------------------------------- + + @Test + void mojoWithAfterGeneratesAfterLinksBlock() { + String source = """ + package com.example; + import org.apache.maven.api.plugin.annotations.Mojo; + import org.apache.maven.api.plugin.annotations.After; + @Mojo(name = "post-compile") + @After(phase = "compile") + public class PostCompileMojo { + public void execute() {} + } + """; + + CompilationResult result = compile( + List.of(inMemorySource("com.example.PostCompileMojo", source)), + Map.of( + "maven.plugin.groupId", "com.example", + "maven.plugin.artifactId", "example-plugin", + "maven.plugin.version", "1.0.0")); + + assertTrue(result.success, () -> "Compilation failed:\n" + result.diagnosticsToString()); + + String pluginXml = result.getResource("META-INF/maven/plugin.xml"); + assertNotNull(pluginXml, "plugin.xml must be generated"); + assertContains(pluginXml, "<afterLinks>"); + assertContains(pluginXml, "<afterLink>"); + assertContains(pluginXml, "<phase>compile</phase>"); + // default type is PROJECT + assertContains(pluginXml, "<type>PROJECT</type>"); + } + + // ------------------------------------------------------------------------- + // Test 4: @Mojo with @Execute → executePhase/executeGoal + // ------------------------------------------------------------------------- + + @Test + void mojoWithExecuteGeneratesExecuteFields() { + String source = """ + package com.example; + import org.apache.maven.api.plugin.annotations.Mojo; + import org.apache.maven.api.plugin.annotations.Execute; + @Mojo(name = "report") + @Execute(phase = "test-compile", lifecycle = "custom") + public class ReportMojo { + public void execute() {} + } + """; + + CompilationResult result = compile( + List.of(inMemorySource("com.example.ReportMojo", source)), + Map.of( + "maven.plugin.groupId", "com.example", + "maven.plugin.artifactId", "example-plugin", + "maven.plugin.version", "2.0.0")); + + assertTrue(result.success, () -> "Compilation failed:\n" + result.diagnosticsToString()); + + String pluginXml = result.getResource("META-INF/maven/plugin.xml"); + assertNotNull(pluginXml, "plugin.xml must be generated"); + assertContains(pluginXml, "<executePhase>test-compile</executePhase>"); + assertContains(pluginXml, "<executeLifecycle>custom</executeLifecycle>"); + } + + // ------------------------------------------------------------------------- + // Test 5: Missing required -A options → ERROR diagnostic + // ------------------------------------------------------------------------- + + @Test + void missingRequiredOptionsEmitsErrorDiagnostic() { + String source = """ + package com.example; + import org.apache.maven.api.plugin.annotations.Mojo; + @Mojo(name = "noop") + public class NoopMojo { + public void execute() {} + } + """; + + // Omit all required -A options + CompilationResult result = compile(List.of(inMemorySource("com.example.NoopMojo", source)), Map.of()); + + // The processor should emit ERROR diagnostics for each missing option Review Comment: 🧹 **Misleading comment.** The implementation emits **one** combined error message ("Could not determine plugin coordinates. Supply -A...") that happens to contain all three option names — not three separate diagnostics as the comment implies. The `assertContains` checks pass because each name appears as a substring of the single message. ```suggestion // The processor emits an error naming all three required options in a single diagnostic ``` -- 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]
