Copilot commented on code in PR #996: URL: https://github.com/apache/maven-enforcer/pull/996#discussion_r3956795868
########## enforcer-rules/src/site/markdown/requireMinimalExports.md.vm: ########## @@ -0,0 +1,83 @@ +--- +title: Require Minimal Exports +author: + - Gerd Aschemann +date: 2026-07-16 +--- + +<!-- +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. +--> + +# Require Minimal Exports + +This rule keeps a Java module's public surface minimal by banning `exports` of packages that are meant to be internal. By default any package whose name has an `internal` or `impl` segment (e.g. `com.example.foo.internal` or `com.example.impl.util`) must not be exported. + +Qualified exports (`exports a.b.c to some.friend;`) are ignored by default, because naming the consumer is a deliberate, reviewable decision. The rule does nothing for non-modular projects (no `module-info.class`). + +## Reading the module descriptor + +This rule inspects the compiled `module-info.class` under `\${project.build.outputDirectory}`, so its `enforce` execution must run *after* compilation. Bind it to a phase such as `process-classes` (the default `validate` phase runs before `compile`, when no `module-info.class` exists yet). + +Both output layouts are supported: the classic one (the descriptor directly in the output directory, one Maven project = one Java module) and the Maven 4 *module source hierarchy* (POM model 4.1.0), where one Maven project compiles several modules, each to its own subdirectory `\${project.build.outputDirectory}/<module-name>/`. + +The following parameters are supported by this rule: + +- **internalPackagePattern** - a regular expression matching packages considered non-API. An exported package that matches fails the build. Default is `.*\.(internal|impl)(\..*)?`. Review Comment: In RequireMinimalExports the regex is applied with Pattern.matcher(...).matches(), which requires a full-string match. The docs currently say 'matching packages' but don't clarify that the pattern must match the entire package name (or include leading/trailing `.*`). Consider clarifying this in the parameter description, or adjusting the implementation to use find() if substring matching is the intended UX. ########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/RequireMinimalExports.java: ########## @@ -0,0 +1,112 @@ +/* + * 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.enforcer.rules.modules; + +import javax.inject.Inject; +import javax.inject.Named; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; + +import org.apache.maven.enforcer.rule.api.EnforcerRuleException; +import org.apache.maven.project.MavenProject; + +/** + * Keeps a module's public surface minimal by banning {@code exports} of packages that are meant to be + * internal. By default any package whose name has an {@code internal} or {@code impl} segment (such as + * {@code com.example.foo.internal} or {@code com.example.impl.util}) must not be exported; the set is + * configurable through {@code <internalPackagePattern>}. + * + * <p>Qualified exports ({@code exports a.b.c to some.friend;}) are ignored by default, because naming + * the consumer is a deliberate, reviewable decision; set {@code <ignoreQualifiedExports>false</ignoreQualifiedExports>} + * to check them too. Individual packages can be whitelisted through {@code <allowedExports>}. + */ +@Named("requireMinimalExports") +public final class RequireMinimalExports extends AbstractModuleInfoRule { + + /** Regex matching packages considered non-API; a match that is exported fails the build. */ + private String internalPackagePattern = ".*\\.(internal|impl)(\\..*)?"; + + /** Packages exempt from the check even if they match {@link #internalPackagePattern}. */ + private List<String> allowedExports = new ArrayList<>(); + + /** When {@code true} (default), only unqualified {@code exports} are checked. */ + private boolean ignoreQualifiedExports = true; + + @Inject + public RequireMinimalExports(MavenProject project) { + super(project); + } + + public void setInternalPackagePattern(String internalPackagePattern) { Review Comment: setInternalPackagePattern accepts null, but execute() later unconditionally calls Pattern.compile(internalPackagePattern), which will throw a NullPointerException with a poor diagnostic. Consider rejecting null early (throw EnforcerRuleException / IllegalArgumentException) or defaulting back to the built-in pattern when null is provided. ########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/RequireMinimalExports.java: ########## @@ -0,0 +1,112 @@ +/* + * 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.enforcer.rules.modules; + +import javax.inject.Inject; +import javax.inject.Named; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; + +import org.apache.maven.enforcer.rule.api.EnforcerRuleException; +import org.apache.maven.project.MavenProject; + +/** + * Keeps a module's public surface minimal by banning {@code exports} of packages that are meant to be + * internal. By default any package whose name has an {@code internal} or {@code impl} segment (such as + * {@code com.example.foo.internal} or {@code com.example.impl.util}) must not be exported; the set is + * configurable through {@code <internalPackagePattern>}. + * + * <p>Qualified exports ({@code exports a.b.c to some.friend;}) are ignored by default, because naming + * the consumer is a deliberate, reviewable decision; set {@code <ignoreQualifiedExports>false</ignoreQualifiedExports>} + * to check them too. Individual packages can be whitelisted through {@code <allowedExports>}. + */ +@Named("requireMinimalExports") +public final class RequireMinimalExports extends AbstractModuleInfoRule { + + /** Regex matching packages considered non-API; a match that is exported fails the build. */ + private String internalPackagePattern = ".*\\.(internal|impl)(\\..*)?"; + + /** Packages exempt from the check even if they match {@link #internalPackagePattern}. */ + private List<String> allowedExports = new ArrayList<>(); + + /** When {@code true} (default), only unqualified {@code exports} are checked. */ + private boolean ignoreQualifiedExports = true; + + @Inject + public RequireMinimalExports(MavenProject project) { + super(project); + } + + public void setInternalPackagePattern(String internalPackagePattern) { + this.internalPackagePattern = internalPackagePattern; + } + + public void setAllowedExports(List<String> allowedExports) { + this.allowedExports = allowedExports != null ? allowedExports : new ArrayList<>(); + } + + public void setIgnoreQualifiedExports(boolean ignoreQualifiedExports) { + this.ignoreQualifiedExports = ignoreQualifiedExports; + } + + @Override + public void execute() throws EnforcerRuleException { + Pattern internal = Pattern.compile(internalPackagePattern); + for (ModuleOutput output : moduleOutputs()) { + JavaModuleInfo module = output.moduleInfo(); + if (module == null) { + continue; + } + checkModule(module, internal); + } + } + + private void checkModule(JavaModuleInfo module, Pattern internal) throws EnforcerRuleException { + List<String> violations = new ArrayList<>(); + for (JavaModuleInfo.Directive export : module.exports()) { + if (ignoreQualifiedExports && export.isQualified()) { + continue; + } + String packageName = export.packageName(); + if (!allowedExports.contains(packageName) + && internal.matcher(packageName).matches()) { Review Comment: allowedExports is a List and is checked with contains() inside a loop over exports; this is O(n*m) for n exports and m allowed entries. Using a Set (e.g., HashSet) for allowedExports internally would make membership checks O(1) and avoid potential slowdowns for larger configurations. ########## enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/modules/JavaModuleInfoReaderTest.java: ########## @@ -0,0 +1,138 @@ +/* + * 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.enforcer.rules.modules; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledForJreRange; +import org.junit.jupiter.api.condition.JRE; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.ModuleVisitor; +import org.objectweb.asm.Opcodes; + +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.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +// Reads module-info via java.lang.module.ModuleDescriptor, which exists only on Java 9+. +@EnabledForJreRange(min = JRE.JAVA_9) +class JavaModuleInfoReaderTest { + + /** Build a real {@code module-info.class} with ASM (test scope only). */ + private static byte[] moduleInfo() { + ClassWriter cw = new ClassWriter(0); + cw.visit(Opcodes.V9, Opcodes.ACC_MODULE, "module-info", null, null, null); + ModuleVisitor mv = cw.visitModule("com.example.foo", 0, null); + mv.visitRequire("java.base", Opcodes.ACC_MANDATED, null); + mv.visitRequire("com.example.bar", 0, null); + mv.visitExport("com/example/foo/api", 0); // unqualified + mv.visitExport("com/example/foo/internal", 0, "com.example.bar"); // qualified + mv.visitOpen("com/example/foo/impl", 0); + mv.visitEnd(); + cw.visitEnd(); + return cw.toByteArray(); + } + + private static List<String> packages(List<JavaModuleInfo.Directive> directives) { + List<String> names = new ArrayList<String>(); + for (JavaModuleInfo.Directive d : directives) { + names.add(d.packageName()); + } + return names; + } + + @Test + void readsModuleNameRequiresExportsAndOpens() throws Exception { + JavaModuleInfo m = JavaModuleInfoReader.read(new ByteArrayInputStream(moduleInfo())); + + assertNotNull(m); + assertEquals("com.example.foo", m.name()); + + assertTrue(m.requires().contains("java.base")); + assertTrue(m.requires().contains("com.example.bar")); + + List<String> exported = packages(m.exports()); + assertTrue(exported.contains("com.example.foo.api"), "expected unqualified export"); + assertTrue(exported.contains("com.example.foo.internal"), "expected qualified export"); + + assertEquals(1, m.opens().size()); + assertEquals("com.example.foo.impl", m.opens().get(0).packageName()); + } + + @Test + void distinguishesQualifiedFromUnqualifiedExports() throws Exception { + JavaModuleInfo m = JavaModuleInfoReader.read(new ByteArrayInputStream(moduleInfo())); + + for (JavaModuleInfo.Directive d : m.exports()) { + if (d.packageName().equals("com.example.foo.api")) { + assertTrue(!d.isQualified(), "api export must be unqualified"); Review Comment: Using assertTrue(!condition) is less clear than assertFalse(condition) and produces worse failure messages. Consider switching the first assertion to assertFalse(d.isQualified(), ...) for readability and diagnostics. ########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/JavaModuleInfoReader.java: ########## @@ -0,0 +1,172 @@ +/* + * 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.enforcer.rules.modules; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Set; + +/** + * Reads the {@code Module} attribute of a {@code module-info.class} (name, {@code requires}, + * {@code exports}, {@code opens}) by delegating to {@link java.lang.module.ModuleDescriptor}. + * + * <p><b>Design decision.</b> This plugin compiles with {@code --release 8}, so it cannot + * reference {@code java.lang.module.ModuleDescriptor} (a Java 9 API) directly. The obvious + * alternative — a multi-release JAR overlay ({@code src/main/java9}) so the module-reading code + * could be compiled for Java 9 — is deliberately <em>not</em> used: multi-release JAR support + * in Maven 3 is incomplete and can even produce invalid JARs (see MNG-6892 / MNG-6293 and + * {@code maven-jar-plugin#484}); it is only cleanly solved in Maven 4 (POM model 4.1.0 + + * {@code maven-compiler-plugin} 4.0.0-beta-3). To keep these rules usable on <b>Maven 3</b> + * and a Java 8 source baseline, we instead access {@code ModuleDescriptor} <b>reflectively</b> + * through this small wrapper class: the API is present at runtime whenever a + * {@code module-info.class} exists (such a project is necessarily built on Java 9+), and the + * rules simply do nothing when there is no module descriptor. See {@code apache/maven-enforcer#995}. + */ +final class JavaModuleInfoReader { + + private static final String MODULE_DESCRIPTOR = "java.lang.module.ModuleDescriptor"; + private static final String INVALID_DESCRIPTOR = "java.lang.module.InvalidModuleDescriptorException"; + + private JavaModuleInfoReader() {} + + /** + * Parse a {@code module-info.class}. + * + * @param in the class-file bytes of a {@code module-info.class} + * @return the parsed module info, or {@code null} if the bytes are not a valid module descriptor + * @throws IOException if the bytes cannot be read, or if {@code java.lang.module} is unavailable + * because the enforcer is running on a Java 8 runtime + */ + static JavaModuleInfo read(InputStream in) throws IOException { + byte[] classFile = readAllBytes(in); + // ModuleDescriptor.read cannot parse a class file newer than the running JVM; without this + // check it would throw InvalidModuleDescriptorException and we would wrongly treat the module + // as "not present". Surface a clear diagnostic instead. + checkReadableVersion(classFile); + try { + Class<?> descriptorType = Class.forName(MODULE_DESCRIPTOR); + Object descriptor = descriptorType + .getMethod("read", InputStream.class) + .invoke(null, new ByteArrayInputStream(classFile)); + + String name = (String) descriptorType.getMethod("name").invoke(descriptor); + boolean open = (Boolean) descriptorType.getMethod("isOpen").invoke(descriptor); + List<String> requires = requireNames(descriptor, descriptorType); + List<JavaModuleInfo.Directive> exports = directives(descriptor, descriptorType, "exports", "Exports"); + List<JavaModuleInfo.Directive> opens = directives(descriptor, descriptorType, "opens", "Opens"); + return new JavaModuleInfo(name, open, requires, exports, opens); + } catch (InvocationTargetException e) { + Throwable cause = e.getCause(); + if (cause != null && INVALID_DESCRIPTOR.equals(cause.getClass().getName())) { + return null; // not a valid module-info.class + } + throw new IOException("Could not read module descriptor", cause != null ? cause : e); Review Comment: Returning null on InvalidModuleDescriptorException can cause a present-but-unreadable/corrupt module-info.class to be treated as 'no module descriptor', which can silently skip checks (or make RequireExplicitModules report 'no module-info.class' rather than 'invalid module-info.class'). Consider treating InvalidModuleDescriptorException as a hard failure when the caller is specifically reading a module-info.class file (e.g., throw an IOException with a clear diagnostic), and reserve 'return null' only for cases where the input is not expected to be module-info. -- 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]
