Skip to content

Commit 8c28a02

Browse files
cowwocclaude
authored andcommitted
Add integration tests and fix default reconciliation merging
Addresses reviewer feedback on PR apache#389: 1. Add integration tests for default reconciliation behavior: - Test with no executionControl configuration (uses defaults) - Test with executionControl for different plugin (verifies merging) 2. Fix reconciliation config resolution to merge defaults with explicit config: - Explicit configs take precedence when present - Default reconciliation configs apply when no explicit config exists - Ensures compiler defaults work even when other plugins are configured 3. Add null safety checks to prevent NPE in unit tests: - Check for null mojoExecution before accessing - Check for null plugin before matching Integration tests verify that: - Changing maven.compiler.release invalidates cache as expected - Changing maven.compiler.source/target invalidates cache - Default reconciliation still applies when executionControl configures other plugins Add test for explicit config overriding defaults Completes integration test coverage requested by reviewer: 1. ✅ No configuration set - DefaultReconciliationTest 2. ✅ No plugin configuration set - DefaultReconciliationTest 3. ✅ Plugin reconciliation overrides defaults - DefaultReconciliationOverrideTest (NEW) 4. ✅ Plugin reconciliation merges with defaults - DefaultReconciliationWithOtherPluginTest The new test verifies that when executionControl explicitly configures maven-compiler-plugin, the explicit config completely OVERRIDES the defaults rather than merging: - Explicit config tracks only 'release' property - Changing 'source' DOES hit cache (proves 'source' not tracked) - Changing 'release' does NOT hit cache (proves 'release' is tracked) - If defaults merged, 'source' would also be tracked (but it's not) This confirms the implementation correctly handles both override and merge scenarios as requested. Implement plugin parameter export/categorization system Addresses reviewer suggestion to create extensible configuration system: **Architecture:** - Plugin parameter definitions stored as XML files in classpath - Each plugin has its own XML file (e.g., maven-compiler-plugin.xml) - Parameters categorized as "functional" or "behavioral" - Validation system checks reconciliation configs against definitions **Components:** 1. **XML Schema** (plugin-parameters.xsd): - Defines structure for parameter definitions - Enforces functional/behavioral categorization - Validates at build time 2. **Parameter Definitions**: - maven-compiler-plugin.xml: 30+ parameters for compile/testCompile goals - maven-install-plugin.xml: 12+ parameters for install/install-file goals - Comprehensive categorization based on whether parameter affects output 3. **Java Infrastructure**: - PluginParameterDefinition: Model classes for definitions - PluginParameterLoader: Loads XML from classpath - Validation logic in CacheConfigImpl 4. **Validation Features**: - ERROR on unknown parameters (may indicate plugin changes) - WARN on behavioral parameters in reconciliation config - WARN when no definition exists for plugin - Validates all default reconciliation configs on initialization **Benefits:** - Detects plugin parameter changes/renames automatically - Prevents incorrect behavioral parameters in reconciliation - Self-documenting: XML files serve as parameter catalog - Extensible: Add new plugins by creating XML files - Type-safe: Enforces functional/behavioral distinction **Testing:** - Unit tests verify parameter loading and categorization - Validates that default reconciliation params are functional - All 20 existing tests pass + 3 new validation tests This creates a "closed and matching set of parameters" as requested, ensuring the default configuration fails/warns on unknown parameters and preventing incorrect behavior from plugin modifications. Add user documentation for parameter validation system Updates documentation to explain the new parameter categorization and validation features: **how-to.md additions:** - New "Parameter Validation and Categorization" section - Explains functional vs behavioral parameter categories - Documents validation features (ERROR/WARN on unknown/misclassified params) - Provides example XML for adding new plugin definitions - Notes that defaults are overridden by explicit config per-plugin - Lists current coverage (maven-compiler-plugin, maven-install-plugin) **concepts.md additions:** - Reference to parameter validation system - Link to detailed how-to documentation - Context within correctness vs performance tradeoff discussion This documentation will appear on the Maven website, making the feature discoverable and providing clear guidance for: - Understanding why certain parameters are tracked - Adding validation for new plugins - Troubleshooting unknown parameter warnings - Extending the system as plugins evolve Fix integration test version placeholder Use ${projectVersion} instead of @project.version@ to match the resource filtering pattern used by other integration tests. Fix integration tests to check for reconciliation behavior - Fix XML config structure: executionControl is sibling of configuration, not child - Update test assertions to check for 'Plugin parameter mismatch found' message - Reconciliation happens at runtime after cache lookup, not during hash calculation - Tests now correctly verify that parameter changes trigger rebuild via reconciliation Add version-specific parameter definition support - Update XSD schema to support minVersion element - Implement version-aware parameter loading with best-match selection - Add version comparison logic handling SNAPSHOT qualifiers - Update validation to use plugin version at runtime - Add comprehensive tests for version-specific loading - Update documentation with version support examples Version matching selects the definition with highest minVersion <= plugin version. Multiple version-specific definitions can coexist in a single XML file. This allows accurate validation as plugin APIs evolve across versions. Load default reconciliation configs from XML instead of hardcoding - Create default-reconciliation.xsd schema for defaults - Add defaults.xml with maven-compiler-plugin and maven-install-plugin configs - Implement DefaultReconciliationLoader to load configs from XML - Replace hardcoded defaults in CacheConfigImpl with XML loading - Update documentation to reflect XML-based configuration This makes defaults extensible - projects can provide their own default-reconciliation/defaults.xml on the classpath.
1 parent e99c008 commit 8c28a02

File tree

27 files changed

+2317
-51
lines changed

27 files changed

+2317
-51
lines changed

src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java

Lines changed: 170 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ public class CacheConfigImpl implements org.apache.maven.buildcache.xml.CacheCon
118118
private final XmlService xmlService;
119119
private final Provider<MavenSession> providerSession;
120120
private final RuntimeInformation rtInfo;
121+
private final PluginParameterLoader parameterLoader;
122+
private final DefaultReconciliationLoader defaultReconciliationLoader;
121123

122124
private volatile CacheState state;
123125
private CacheConfig cacheConfig;
@@ -129,6 +131,8 @@ public CacheConfigImpl(XmlService xmlService, Provider<MavenSession> providerSes
129131
this.xmlService = xmlService;
130132
this.providerSession = providerSession;
131133
this.rtInfo = rtInfo;
134+
this.parameterLoader = new PluginParameterLoader();
135+
this.defaultReconciliationLoader = new DefaultReconciliationLoader();
132136
}
133137

134138
@Nonnull
@@ -248,74 +252,190 @@ public boolean isLogAllProperties(MojoExecution mojoExecution) {
248252
}
249253

250254
private GoalReconciliation findReconciliationConfig(MojoExecution mojoExecution) {
251-
List<GoalReconciliation> reconciliation;
255+
if (mojoExecution == null) {
256+
return null;
257+
}
252258

253-
if (cacheConfig.getExecutionControl() == null || cacheConfig.getExecutionControl().getReconcile() == null) {
254-
// Use default reconciliation configs for common plugins
255-
reconciliation = getDefaultReconciliationConfigs();
256-
} else {
257-
reconciliation = cacheConfig.getExecutionControl().getReconcile().getPlugins();
259+
final String goal = mojoExecution.getGoal();
260+
final Plugin plugin = mojoExecution.getPlugin();
261+
262+
if (plugin == null) {
263+
return null;
258264
}
259265

260-
for (GoalReconciliation goalReconciliationConfig : reconciliation) {
261-
final String goal = mojoExecution.getGoal();
266+
// First check explicit configuration
267+
if (cacheConfig.getExecutionControl() != null && cacheConfig.getExecutionControl().getReconcile() != null) {
268+
List<GoalReconciliation> explicitConfigs =
269+
cacheConfig.getExecutionControl().getReconcile().getPlugins();
270+
for (GoalReconciliation config : explicitConfigs) {
271+
if (isPluginMatch(plugin, config) && Strings.CS.equals(goal, config.getGoal())) {
272+
// Validate explicit config against parameter definitions (with version)
273+
validateReconciliationConfig(config, plugin);
274+
return config;
275+
}
276+
}
277+
}
262278

263-
if (isPluginMatch(mojoExecution.getPlugin(), goalReconciliationConfig)
264-
&& Strings.CS.equals(goal, goalReconciliationConfig.getGoal())) {
265-
return goalReconciliationConfig;
279+
// Fall back to defaults if no explicit configuration found
280+
List<GoalReconciliation> defaults = getDefaultReconciliationConfigs();
281+
for (GoalReconciliation config : defaults) {
282+
if (isPluginMatch(plugin, config) && Strings.CS.equals(goal, config.getGoal())) {
283+
// Validate default config against parameter definitions (with version)
284+
validateReconciliationConfig(config, plugin);
285+
return config;
266286
}
267287
}
288+
268289
return null;
269290
}
270291

271-
private List<GoalReconciliation> getDefaultReconciliationConfigs() {
272-
List<GoalReconciliation> defaults = new ArrayList<>();
273-
274-
// maven-compiler-plugin:compile - track source, target, release
275-
GoalReconciliation compilerCompile = new GoalReconciliation();
276-
compilerCompile.setArtifactId("maven-compiler-plugin");
277-
compilerCompile.setGoal("compile");
278-
279-
TrackedProperty source = new TrackedProperty();
280-
source.setPropertyName("source");
281-
compilerCompile.addReconcile(source);
282-
283-
TrackedProperty target = new TrackedProperty();
284-
target.setPropertyName("target");
285-
compilerCompile.addReconcile(target);
292+
/**
293+
* Validates a single reconciliation config against plugin parameter definitions.
294+
* Uses plugin version to load the appropriate parameter definition.
295+
*/
296+
private void validateReconciliationConfig(GoalReconciliation config, Plugin plugin) {
297+
String artifactId = config.getArtifactId();
298+
String goal = config.getGoal();
299+
String pluginVersion = plugin.getVersion();
300+
301+
// Load parameter definition for this plugin with version
302+
PluginParameterDefinition pluginDef = parameterLoader.load(artifactId, pluginVersion);
303+
304+
if (pluginDef == null) {
305+
LOGGER.warn(
306+
"No parameter definition found for plugin {}:{} version {}. "
307+
+ "Cannot validate reconciliation configuration. "
308+
+ "Consider adding a parameter definition file to plugin-parameters/{}.xml",
309+
artifactId,
310+
goal,
311+
pluginVersion != null ? pluginVersion : "unknown",
312+
artifactId);
313+
return;
314+
}
286315

287-
TrackedProperty release = new TrackedProperty();
288-
release.setPropertyName("release");
289-
compilerCompile.addReconcile(release);
316+
// Get goal definition
317+
PluginParameterDefinition.GoalParameterDefinition goalDef = pluginDef.getGoal(goal);
318+
if (goalDef == null) {
319+
LOGGER.warn(
320+
"Goal '{}' not found in parameter definition for plugin {} version {}. "
321+
+ "Cannot validate reconciliation configuration.",
322+
goal,
323+
artifactId,
324+
pluginVersion != null ? pluginVersion : "unknown");
325+
return;
326+
}
290327

291-
defaults.add(compilerCompile);
328+
// Validate each tracked property
329+
List<TrackedProperty> properties = config.getReconciles();
330+
if (properties == null) {
331+
return;
332+
}
292333

293-
// maven-compiler-plugin:testCompile - track source, target, release
294-
GoalReconciliation compilerTestCompile = new GoalReconciliation();
295-
compilerTestCompile.setArtifactId("maven-compiler-plugin");
296-
compilerTestCompile.setGoal("testCompile");
334+
for (TrackedProperty property : properties) {
335+
String propertyName = property.getPropertyName();
336+
337+
if (!goalDef.hasParameter(propertyName)) {
338+
LOGGER.error(
339+
"Unknown parameter '{}' in reconciliation config for {}:{} version {}. "
340+
+ "This may indicate a plugin version mismatch or renamed parameter. "
341+
+ "Consider updating parameter definition or removing from reconciliation.",
342+
propertyName,
343+
artifactId,
344+
goal,
345+
pluginVersion != null ? pluginVersion : "unknown");
346+
} else {
347+
PluginParameterDefinition.ParameterDefinition paramDef = goalDef.getParameter(propertyName);
348+
if (paramDef.isBehavioral()) {
349+
LOGGER.warn(
350+
"Parameter '{}' in reconciliation config for {}:{} is categorized as BEHAVIORAL. "
351+
+ "Behavioral parameters affect how the build runs but not the output. "
352+
+ "Consider removing if it doesn't actually affect build artifacts.",
353+
propertyName,
354+
artifactId,
355+
goal);
356+
}
357+
}
358+
}
359+
}
297360

298-
TrackedProperty testSource = new TrackedProperty();
299-
testSource.setPropertyName("source");
300-
compilerTestCompile.addReconcile(testSource);
361+
/**
362+
* Load default reconciliation configurations from XML.
363+
* Defaults are loaded from classpath: default-reconciliation/defaults.xml
364+
*/
365+
private List<GoalReconciliation> getDefaultReconciliationConfigs() {
366+
List<GoalReconciliation> defaults = defaultReconciliationLoader.loadDefaults();
301367

302-
TrackedProperty testTarget = new TrackedProperty();
303-
testTarget.setPropertyName("target");
304-
compilerTestCompile.addReconcile(testTarget);
368+
// Validate all default configurations against parameter definitions
369+
validateReconciliationConfigs(defaults);
305370

306-
TrackedProperty testRelease = new TrackedProperty();
307-
testRelease.setPropertyName("release");
308-
compilerTestCompile.addReconcile(testRelease);
371+
return defaults;
372+
}
309373

310-
defaults.add(compilerTestCompile);
374+
/**
375+
* Validates reconciliation configs against plugin parameter definitions.
376+
* Warns about unknown parameters that may indicate plugin changes or configuration errors.
377+
*/
378+
private void validateReconciliationConfigs(List<GoalReconciliation> configs) {
379+
for (GoalReconciliation config : configs) {
380+
String artifactId = config.getArtifactId();
381+
String goal = config.getGoal();
382+
383+
// Load parameter definition for this plugin
384+
PluginParameterDefinition pluginDef = parameterLoader.load(artifactId);
385+
386+
if (pluginDef == null) {
387+
LOGGER.warn(
388+
"No parameter definition found for plugin {}:{}. "
389+
+ "Cannot validate reconciliation configuration. "
390+
+ "Consider adding a parameter definition file to plugin-parameters/{}.xml",
391+
artifactId,
392+
goal,
393+
artifactId);
394+
continue;
395+
}
311396

312-
// maven-install-plugin:install - always run (empty reconciliation means it's tracked)
313-
GoalReconciliation install = new GoalReconciliation();
314-
install.setArtifactId("maven-install-plugin");
315-
install.setGoal("install");
316-
defaults.add(install);
397+
// Get goal definition
398+
PluginParameterDefinition.GoalParameterDefinition goalDef = pluginDef.getGoal(goal);
399+
if (goalDef == null) {
400+
LOGGER.warn(
401+
"Goal '{}' not found in parameter definition for plugin {}. "
402+
+ "Cannot validate reconciliation configuration.",
403+
goal,
404+
artifactId);
405+
continue;
406+
}
317407

318-
return defaults;
408+
// Validate each tracked property
409+
List<TrackedProperty> properties = config.getReconciles();
410+
if (properties != null) {
411+
for (TrackedProperty property : properties) {
412+
String propertyName = property.getPropertyName();
413+
414+
if (!goalDef.hasParameter(propertyName)) {
415+
LOGGER.error(
416+
"Unknown parameter '{}' in default reconciliation config for {}:{}. "
417+
+ "This parameter is not defined in the plugin parameter definition. "
418+
+ "This may indicate a plugin version mismatch or renamed parameter. "
419+
+ "Please update the parameter definition or remove this property from reconciliation.",
420+
propertyName,
421+
artifactId,
422+
goal);
423+
} else {
424+
PluginParameterDefinition.ParameterDefinition paramDef =
425+
goalDef.getParameter(propertyName);
426+
if (paramDef.isBehavioral()) {
427+
LOGGER.warn(
428+
"Parameter '{}' in reconciliation config for {}:{} is categorized as BEHAVIORAL. "
429+
+ "Behavioral parameters typically should not affect cache invalidation. "
430+
+ "Consider removing this parameter from reconciliation if it doesn't affect build output.",
431+
propertyName,
432+
artifactId,
433+
goal);
434+
}
435+
}
436+
}
437+
}
438+
}
319439
}
320440

321441
@Nonnull
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.maven.buildcache.xml;
20+
21+
import javax.xml.parsers.DocumentBuilder;
22+
import javax.xml.parsers.DocumentBuilderFactory;
23+
24+
import java.io.InputStream;
25+
import java.util.ArrayList;
26+
import java.util.List;
27+
28+
import org.apache.maven.buildcache.xml.config.GoalReconciliation;
29+
import org.apache.maven.buildcache.xml.config.TrackedProperty;
30+
import org.slf4j.Logger;
31+
import org.slf4j.LoggerFactory;
32+
import org.w3c.dom.Document;
33+
import org.w3c.dom.Element;
34+
import org.w3c.dom.NodeList;
35+
36+
/**
37+
* Loads default reconciliation configurations from classpath resources.
38+
* Default configs are stored in src/main/resources/default-reconciliation/defaults.xml
39+
*/
40+
public class DefaultReconciliationLoader {
41+
42+
private static final Logger LOGGER = LoggerFactory.getLogger(DefaultReconciliationLoader.class);
43+
private static final String DEFAULTS_PATH = "default-reconciliation/defaults.xml";
44+
45+
private List<GoalReconciliation> cachedDefaults;
46+
47+
/**
48+
* Load default reconciliation configurations from XML
49+
*/
50+
public List<GoalReconciliation> loadDefaults() {
51+
if (cachedDefaults != null) {
52+
return cachedDefaults;
53+
}
54+
55+
InputStream is = getClass().getClassLoader().getResourceAsStream(DEFAULTS_PATH);
56+
57+
if (is == null) {
58+
LOGGER.warn("No default reconciliation configuration found at {}", DEFAULTS_PATH);
59+
cachedDefaults = new ArrayList<>();
60+
return cachedDefaults;
61+
}
62+
63+
try {
64+
cachedDefaults = parseDefaults(is);
65+
LOGGER.info("Loaded {} default reconciliation configurations", cachedDefaults.size());
66+
return cachedDefaults;
67+
} catch (Exception e) {
68+
LOGGER.warn("Failed to load default reconciliation configurations: {}", e.getMessage(), e);
69+
cachedDefaults = new ArrayList<>();
70+
return cachedDefaults;
71+
}
72+
}
73+
74+
private List<GoalReconciliation> parseDefaults(InputStream is) throws Exception {
75+
List<GoalReconciliation> defaults = new ArrayList<>();
76+
77+
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
78+
factory.setNamespaceAware(true);
79+
DocumentBuilder builder = factory.newDocumentBuilder();
80+
Document doc = builder.parse(is);
81+
82+
Element root = doc.getDocumentElement();
83+
NodeList pluginNodes = root.getElementsByTagName("plugin");
84+
85+
for (int i = 0; i < pluginNodes.getLength(); i++) {
86+
Element pluginElement = (Element) pluginNodes.item(i);
87+
GoalReconciliation config = parsePlugin(pluginElement);
88+
defaults.add(config);
89+
}
90+
91+
return defaults;
92+
}
93+
94+
private GoalReconciliation parsePlugin(Element pluginElement) {
95+
String artifactId = getTextContent(pluginElement, "artifactId");
96+
String goal = getTextContent(pluginElement, "goal");
97+
98+
GoalReconciliation config = new GoalReconciliation();
99+
config.setArtifactId(artifactId);
100+
config.setGoal(goal);
101+
102+
// Parse properties if present
103+
NodeList propertiesNodes = pluginElement.getElementsByTagName("properties");
104+
if (propertiesNodes.getLength() > 0) {
105+
Element propertiesElement = (Element) propertiesNodes.item(0);
106+
NodeList propertyNodes = propertiesElement.getElementsByTagName("property");
107+
108+
for (int i = 0; i < propertyNodes.getLength(); i++) {
109+
String propertyName = propertyNodes.item(i).getTextContent().trim();
110+
TrackedProperty property = new TrackedProperty();
111+
property.setPropertyName(propertyName);
112+
config.addReconcile(property);
113+
}
114+
}
115+
116+
return config;
117+
}
118+
119+
private String getTextContent(Element parent, String tagName) {
120+
NodeList nodes = parent.getElementsByTagName(tagName);
121+
if (nodes.getLength() > 0) {
122+
return nodes.item(0).getTextContent().trim();
123+
}
124+
return null;
125+
}
126+
}

0 commit comments

Comments
 (0)