gnodet commented on code in PR #13080: URL: https://github.com/apache/maven/pull/13080#discussion_r3962051044
########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Properties; + +/** + * Provides default plugin versions for the built-in lifecycle bindings. + * <p> + * Versions are read from {@code plugin-versions.properties}, which is filtered + * at build time from POM properties ({@code version.maven-<name>-plugin}). + * Centralising them in the POM makes them visible to dependency-update bots + * such as Dependabot and Renovate. + * + * @since 4.1.0 + */ +public final class PluginVersions { + + private static final Properties VERSIONS = new Properties(); + + static { + try (InputStream in = PluginVersions.class.getResourceAsStream("plugin-versions.properties")) { + if (in == null) { + throw new ExceptionInInitializerError("plugin-versions.properties not found on classpath"); + } + VERSIONS.load(in); + } catch (IOException e) { + throw new ExceptionInInitializerError(e); + } + } + + private PluginVersions() {} + + /** + * Returns the default version for the given plugin. + * + * @param pluginArtifactId the artifact id, e.g. {@code "maven-compiler-plugin"} + * @return the version string, never {@code null} + * @throws IllegalArgumentException if the plugin is not listed in the properties file + */ + public static String version(String pluginArtifactId) { + String key = pluginArtifactId + ".version"; + String version = VERSIONS.getProperty(key); + if (version == null) { + throw new IllegalArgumentException("No default version defined for " + pluginArtifactId + "; add " + key + + " to plugin-versions.properties"); + } + return version; + } Review Comment: ⚠️ **Silent failure: unfiltered placeholder survives null check** `VERSIONS.getProperty(key)` returns `"${version.maven-clean-plugin}"` (not `null`) when resource filtering is skipped — e.g. when the class is loaded from an IDE or test classpath built without the Maven resources plugin running. The null guard does not catch this: the constant is set to a literal `${…}` string, Maven silently tries to resolve a plugin at that version, and the user gets a cryptic "not found in repository" error at build time with no clue that the properties file was never filtered. Add a format guard that fails fast at class-init time: ```suggestion public static String version(String pluginArtifactId) { String key = pluginArtifactId + ".version"; String version = VERSIONS.getProperty(key); if (version == null) { throw new IllegalArgumentException("No default version defined for " + pluginArtifactId + "; add " + key + " to plugin-versions.properties"); } if (version.startsWith("${")) { throw new ExceptionInInitializerError( "plugin-versions.properties was not filtered at build time; " + key + " still contains placeholder: " + version); } return version; } ``` ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Properties; + +/** + * Provides default plugin versions for the built-in lifecycle bindings. + * <p> + * Versions are read from {@code plugin-versions.properties}, which is filtered + * at build time from POM properties ({@code version.maven-<name>-plugin}). + * Centralising them in the POM makes them visible to dependency-update bots + * such as Dependabot and Renovate. + * + * @since 4.1.0 + */ +public final class PluginVersions { + + private static final Properties VERSIONS = new Properties(); + + static { + try (InputStream in = PluginVersions.class.getResourceAsStream("plugin-versions.properties")) { + if (in == null) { + throw new ExceptionInInitializerError("plugin-versions.properties not found on classpath"); + } + VERSIONS.load(in); + } catch (IOException e) { + throw new ExceptionInInitializerError(e); + } + } + + private PluginVersions() {} + + /** + * Returns the default version for the given plugin. + * + * @param pluginArtifactId the artifact id, e.g. {@code "maven-compiler-plugin"} + * @return the version string, never {@code null} + * @throws IllegalArgumentException if the plugin is not listed in the properties file + */ + public static String version(String pluginArtifactId) { Review Comment: 🔸 **Unnecessary public API surface** `version(String)` is `public` but its only callers are the 13 constants in this same class (all called during class initialisation). Once the constants exist, nothing outside this class needs to call `version()` at runtime — the public constants are the intended API. Exposing the method invites callers to store the result in their own fields, bypassing future caching or validation improvements. Make it `private`: ```suggestion private static String version(String pluginArtifactId) { ``` If external code genuinely needs to look up an arbitrary plugin version, that can be added as a separate, explicitly-documented `public` method later (with a stronger contract). ########## impl/maven-core/src/main/java/org/apache/maven/lifecycle/PluginVersions.java: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +import java.io.IOException; +import java.io.InputStream; +import java.util.Properties; + +/** + * Provides default plugin versions for the built-in lifecycle bindings. + * <p> + * Versions are read from {@code plugin-versions.properties}, which is filtered + * at build time from POM properties ({@code version.maven-<name>-plugin}). + * Centralising them in the POM makes them visible to dependency-update bots + * such as Dependabot and Renovate. + * + * @since 4.1.0 + */ +public final class PluginVersions { + + private static final Properties VERSIONS = new Properties(); + + static { + try (InputStream in = PluginVersions.class.getResourceAsStream("plugin-versions.properties")) { + if (in == null) { + throw new ExceptionInInitializerError("plugin-versions.properties not found on classpath"); + } + VERSIONS.load(in); + } catch (IOException e) { + throw new ExceptionInInitializerError(e); + } + } + + private PluginVersions() {} + + /** + * Returns the default version for the given plugin. + * + * @param pluginArtifactId the artifact id, e.g. {@code "maven-compiler-plugin"} + * @return the version string, never {@code null} + * @throws IllegalArgumentException if the plugin is not listed in the properties file + */ + public static String version(String pluginArtifactId) { + String key = pluginArtifactId + ".version"; + String version = VERSIONS.getProperty(key); + if (version == null) { + throw new IllegalArgumentException("No default version defined for " + pluginArtifactId + "; add " + key + + " to plugin-versions.properties"); + } + return version; + } + + // --- convenience constants used by lifecycle mapping providers --- + + public static final String CLEAN = version("maven-clean-plugin"); + public static final String COMPILER = version("maven-compiler-plugin"); + public static final String DEPLOY = version("maven-deploy-plugin"); + public static final String EAR = version("maven-ear-plugin"); + public static final String EJB = version("maven-ejb-plugin"); + public static final String INSTALL = version("maven-install-plugin"); + public static final String JAR = version("maven-jar-plugin"); + public static final String PLUGIN = version("maven-plugin-plugin"); + public static final String RAR = version("maven-rar-plugin"); + public static final String RESOURCES = version("maven-resources-plugin"); + public static final String SITE = version("maven-site-plugin"); + public static final String SUREFIRE = version("maven-surefire-plugin"); + public static final String WAR = version("maven-war-plugin"); +} Review Comment: 💡 **No test for the loading mechanism** The static initialiser, the filtering round-trip, and the null/placeholder guards are the critical path of this new class, yet there is no unit test. A minimal test verifying that every constant is non-null and does not look like an unfiltered placeholder (`!CLEAN.startsWith("${")`) would catch the filtering-skipped scenario and guard against future regressions (e.g. a new constant added to the class but forgotten in the properties file). Example: ```java @Test void pluginVersionsAreResolved() { // Verify all constants are loaded and not unfiltered placeholders for (Field f : PluginVersions.class.getFields()) { if (f.getType() == String.class) { String value = (String) f.get(null); assertNotNull(value, f.getName() + " is null"); assertFalse(value.startsWith("${"), f.getName() + " is unfiltered: " + value); } } } ``` -- 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]
