Skip to content

Externalize default lifecycle plugin versions to POM properties - #13080

Open
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:feature/externalize-plugin-versions
Open

Externalize default lifecycle plugin versions to POM properties#13080
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:feature/externalize-plugin-versions

Conversation

@gnodet

@gnodet gnodet commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Move the 13 hardcoded plugin version constants from Java source into POM properties in impl/maven-core/pom.xml. The values are filtered at build time into plugin-versions.properties and loaded at runtime by a new PluginVersions utility class.

This makes the default lifecycle plugin versions visible to dependency-update bots (Dependabot, Renovate) that scan POM files for version properties, enabling automated version bump PRs.

Changes:

  • impl/maven-core/pom.xml — Add <properties> section with version.maven-<name>-plugin entries for all 13 default plugins
  • plugin-versions.properties — New resource file with ${...} placeholders, filtered at build time
  • PluginVersions.java — New utility class that loads versions from the properties file and exposes them as constants
  • AbstractLifecycleMappingProvider.java — Replace hardcoded version strings with PluginVersions.* constants
  • DefaultLifecycleRegistry.java — Replace hardcoded clean/site plugin versions with PluginVersions.* constants

No behavioral change: all version values are identical to the previous hardcoded constants.

Follow-up to the discussion in #13076 about automating plugin version bumps.

Move the 13 hardcoded plugin version constants from Java source into
POM properties in impl/maven-core/pom.xml. The values are filtered at
build time into plugin-versions.properties and loaded at runtime by a
new PluginVersions utility class.

This makes the default lifecycle plugin versions visible to
dependency-update bots (Dependabot, Renovate) that scan POM files for
version properties, enabling automated version bump PRs.

No behavioral change: all version values are identical to the previous
hardcoded constants.
@gnodet gnodet added this to the 4.0.0-rc-7 milestone Sep 8, 2026
@gnodet
gnodet requested a review from ascheman September 8, 2026 15:52

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three issues to fix before merge: a silent-failure mode when resource filtering is not applied, a public API surface that doesn't need to be public, and a milestone mismatch.

This review was generated by an AI agent, Hermès, on behalf of @gnodet.

Comment on lines +59 to +67
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;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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:

Suggested change
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;
}
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;
}

* @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) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔸 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:

Suggested change
public static String version(String pluginArtifactId) {
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).

Comment on lines +71 to +84
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");
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

@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);
        }
    }
}

@gnodet

gnodet commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Milestone mismatch: the PR targets master (4.1.0-SNAPSHOT) but is assigned to milestone 4.0.0-rc-7 (which tracks the maven-4.0.x branch). The @since 4.1.0 Javadoc in PluginVersions confirms this is intended for 4.1.0. Please update the milestone to 4.x or 4.1.0.

This comment was generated by an AI agent, Hermès, on behalf of @gnodet.

public abstract class AbstractLifecycleMappingProvider implements Provider<LifecycleMapping> {
// START SNIPPET: versions
protected static final String RESOURCES_PLUGIN_VERSION = "3.3.1";
protected static final String RESOURCES_PLUGIN_VERSION = PluginVersions.RESOURCES;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe deprecate these fields, or just remove them if this is all new in 4.0.lx

@gnodet gnodet modified the milestones: 4.0.0-rc-7, 4.1.0 Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants