Copilot commented on code in PR #996: URL: https://github.com/apache/maven-enforcer/pull/996#discussion_r3611191422
########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/JavaModuleInfoReader.java: ########## @@ -0,0 +1,163 @@ +/* + * 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.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 (cf. 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 + * (i.e. 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); + } catch (ClassNotFoundException | NoSuchMethodException e) { + throw new IOException( + "java.lang.module is not available; reading module-info requires a Java 9+ runtime", e); + } catch (ReflectiveOperationException e) { + throw new IOException("Could not read module descriptor", e); + } + } + + private static byte[] readAllBytes(InputStream in) throws IOException { + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + byte[] chunk = new byte[8192]; + int read; + while ((read = in.read(chunk)) != -1) { + buffer.write(chunk, 0, read); + } + return buffer.toByteArray(); + } + + /** + * Fail fast if the {@code module-info.class} was built for a newer Java release than the JVM + * running the enforcer, in which case {@code ModuleDescriptor.read} cannot parse it. + */ + private static void checkReadableVersion(byte[] classFile) throws IOException { + if (classFile.length < 8 + || (classFile[0] & 0xFF) != 0xCA + || (classFile[1] & 0xFF) != 0xFE + || (classFile[2] & 0xFF) != 0xBA + || (classFile[3] & 0xFF) != 0xBE) { + throw new IOException("Not a Java class file (bad magic)"); + } + int major = ((classFile[6] & 0xFF) << 8) | (classFile[7] & 0xFF); + int runtimeMajor = runtimeClassFileMajor(); + if (major > runtimeMajor) { + throw new IOException("module-info.class has class file version " + major + " (Java " + (major - 44) + + "), which is newer than the JVM running the enforcer (class file version " + runtimeMajor + + ", Java " + (runtimeMajor - 44) + "). Run the build on that Java release or newer."); + } + } + + /** The class-file major version of the running JVM (e.g. 52 for Java 8, 69 for Java 25). */ + private static int runtimeClassFileMajor() { + // "java.class.version" is "52.0", "61.0", "69.0", ... — available since Java 1.1. + String version = System.getProperty("java.class.version", "52.0"); + int dot = version.indexOf('.'); + return Integer.parseInt(dot >= 0 ? version.substring(0, dot) : version); + } + + private static List<String> requireNames(Object descriptor, Class<?> descriptorType) + throws ReflectiveOperationException { + Set<?> requires = (Set<?>) descriptorType.getMethod("requires").invoke(descriptor); + Method name = Class.forName(MODULE_DESCRIPTOR + "$Requires").getMethod("name"); + List<String> result = new ArrayList<String>(requires.size()); + for (Object require : requires) { + result.add((String) name.invoke(require)); + } + return result; + } + + private static List<JavaModuleInfo.Directive> directives( + Object descriptor, Class<?> descriptorType, String accessor, String innerType) + throws ReflectiveOperationException { + Set<?> entries = (Set<?>) descriptorType.getMethod(accessor).invoke(descriptor); + Class<?> entryType = Class.forName(MODULE_DESCRIPTOR + "$" + innerType); + Method source = entryType.getMethod("source"); + Method targets = entryType.getMethod("targets"); + List<JavaModuleInfo.Directive> result = new ArrayList<JavaModuleInfo.Directive>(entries.size()); + for (Object entry : entries) { + String pkg = (String) source.invoke(entry); + Set<?> to = (Set<?>) targets.invoke(entry); // empty for an unqualified directive + List<String> moduleTargets = new ArrayList<String>(to.size()); + for (Object target : to) { + moduleTargets.add((String) target); + } + result.add(new JavaModuleInfo.Directive(pkg, moduleTargets)); + } + return result; Review Comment: ModuleDescriptor#exports()/opens() and the directive targets are Sets, so their iteration order is not specified. Sorting targets and directives makes rule diagnostics stable across JVM/JDK versions. ########## 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 (e.g. + * {@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; + } Review Comment: If configuration injection sets <allowedExports/> to null, allowedExports.contains(...) will throw a NullPointerException. Normalize null to an empty list in the setter to keep the rule robust. ########## enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/modules/ModuleInfoFixtures.java: ########## @@ -0,0 +1,129 @@ +/* + * 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.File; +import java.io.IOException; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.List; + +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.ModuleVisitor; +import org.objectweb.asm.Opcodes; + +/** + * Test-only helper that generates real {@code module-info.class} fixtures with ASM and writes them + * (plus optional dummy classes) into an output directory, so the module rules can be exercised against + * a {@code project.build.outputDirectory} exactly as they see it at build time. + */ +final class ModuleInfoFixtures { + + private ModuleInfoFixtures() {} + + static Builder module(String name) { + return new Builder(name, false); + } + + static Builder openModule(String name) { + return new Builder(name, true); + } + + /** Write an arbitrary (empty) compiled class so an output directory looks non-empty to a rule. */ + static void writeDummyClass(File outputDirectory, String binaryName) throws IOException { + ClassWriter cw = new ClassWriter(0); + cw.visit(Opcodes.V1_8, Opcodes.ACC_PUBLIC, binaryName.replace('.', '/'), null, "java/lang/Object", null); + cw.visitEnd(); + Files.createDirectories(outputDirectory.toPath()); + Files.write(new File(outputDirectory, binaryName + ".class").toPath(), cw.toByteArray()); + } Review Comment: writeDummyClass writes the .class file to "<outputDir>/<binaryName>.class" (dots in the filename) instead of the normal package directory layout. Writing to the internal-name path (com/example/Foo.class) better matches real compiler output and avoids surprising fixtures. ########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/JavaModuleInfoReader.java: ########## @@ -0,0 +1,163 @@ +/* + * 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.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 (cf. 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 + * (i.e. 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); + } catch (ClassNotFoundException | NoSuchMethodException e) { + throw new IOException( + "java.lang.module is not available; reading module-info requires a Java 9+ runtime", e); + } catch (ReflectiveOperationException e) { + throw new IOException("Could not read module descriptor", e); + } + } + + private static byte[] readAllBytes(InputStream in) throws IOException { + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + byte[] chunk = new byte[8192]; + int read; + while ((read = in.read(chunk)) != -1) { + buffer.write(chunk, 0, read); + } + return buffer.toByteArray(); + } + + /** + * Fail fast if the {@code module-info.class} was built for a newer Java release than the JVM + * running the enforcer, in which case {@code ModuleDescriptor.read} cannot parse it. + */ + private static void checkReadableVersion(byte[] classFile) throws IOException { + if (classFile.length < 8 + || (classFile[0] & 0xFF) != 0xCA + || (classFile[1] & 0xFF) != 0xFE + || (classFile[2] & 0xFF) != 0xBA + || (classFile[3] & 0xFF) != 0xBE) { + throw new IOException("Not a Java class file (bad magic)"); + } + int major = ((classFile[6] & 0xFF) << 8) | (classFile[7] & 0xFF); + int runtimeMajor = runtimeClassFileMajor(); + if (major > runtimeMajor) { + throw new IOException("module-info.class has class file version " + major + " (Java " + (major - 44) + + "), which is newer than the JVM running the enforcer (class file version " + runtimeMajor + + ", Java " + (runtimeMajor - 44) + "). Run the build on that Java release or newer."); + } + } + + /** The class-file major version of the running JVM (e.g. 52 for Java 8, 69 for Java 25). */ + private static int runtimeClassFileMajor() { + // "java.class.version" is "52.0", "61.0", "69.0", ... — available since Java 1.1. + String version = System.getProperty("java.class.version", "52.0"); + int dot = version.indexOf('.'); + return Integer.parseInt(dot >= 0 ? version.substring(0, dot) : version); + } + + private static List<String> requireNames(Object descriptor, Class<?> descriptorType) + throws ReflectiveOperationException { + Set<?> requires = (Set<?>) descriptorType.getMethod("requires").invoke(descriptor); + Method name = Class.forName(MODULE_DESCRIPTOR + "$Requires").getMethod("name"); + List<String> result = new ArrayList<String>(requires.size()); + for (Object require : requires) { + result.add((String) name.invoke(require)); + } + return result; + } Review Comment: ModuleDescriptor#requires() returns a Set, so iteration order isn’t guaranteed across JDKs. Since these rules are intended to be deterministic (especially for error messages / logs), sort the collected require names before returning. ########## enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/modules/BanUnjustifiedOpens.java: ########## @@ -0,0 +1,95 @@ +/* + * 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 org.apache.maven.enforcer.rule.api.EnforcerRuleException; +import org.apache.maven.project.MavenProject; + +/** + * Bans unqualified deep-reflection access into a module. A bare {@code opens a.b.c;} (or an + * {@code open module}) grants every other module reflective access to a package's private members, + * which defeats strong encapsulation. A framework that needs reflective access should be named + * explicitly with a qualified {@code opens a.b.c to some.framework;}. + * + * <p>Packages that genuinely must be opened to the whole world can be listed in + * {@code <allowedOpens>} to pass the check. + */ +@Named("banUnjustifiedOpens") +public final class BanUnjustifiedOpens extends AbstractModuleInfoRule { + + /** Packages allowed to be opened unqualifiedly despite the ban. */ + private List<String> allowedOpens = new ArrayList<>(); + + @Inject + public BanUnjustifiedOpens(MavenProject project) { + super(project); + } + + public void setAllowedOpens(List<String> allowedOpens) { + this.allowedOpens = allowedOpens; + } Review Comment: If configuration injection sets <allowedOpens/> to null (common for empty list parameters), allowedOpens.contains(...) will throw a NullPointerException. Defensively normalize null to an empty list in the setter. -- 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]
