gnodet-bot commented on code in PR #13348: URL: https://github.com/apache/maven/pull/13348#discussion_r4185327019
########## impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedStringConverter.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.configuration.internal; + +import org.codehaus.plexus.component.configurator.ComponentConfigurationException; +import org.codehaus.plexus.component.configurator.ConfigurationListener; +import org.codehaus.plexus.component.configurator.converters.AbstractConfigurationConverter; +import org.codehaus.plexus.component.configurator.converters.lookup.ConverterLookup; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluationException; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluator; +import org.codehaus.plexus.component.configurator.expression.TypeAwareExpressionEvaluator; +import org.codehaus.plexus.configuration.PlexusConfiguration; + +/** + * Converter for String, CharSequence, StringBuilder, and StringBuffer that properly handles + * empty configuration elements (e.g. {@code <setting></setting>} or {@code <setting/>}). + */ +class EnhancedStringConverter extends AbstractConfigurationConverter { + + @Override + public boolean canConvert(Class<?> type) { + return String.class.equals(type) + || CharSequence.class.equals(type) + || StringBuilder.class.equals(type) + || StringBuffer.class.equals(type); + } + + @Override + public Object fromConfiguration( + ConverterLookup lookup, + PlexusConfiguration configuration, + Class<?> type, + Class<?> enclosingType, + ClassLoader loader, + ExpressionEvaluator evaluator, + ConfigurationListener listener) + throws ComponentConfigurationException { + + if (configuration.getChildCount() > 0) { + throw new ComponentConfigurationException( + "Basic element '" + configuration.getName() + "' must not contain child elements"); + } + + String value = configuration.getValue(); + Object result = null; + + if (value != null && !value.isEmpty()) { + try { + if (evaluator instanceof TypeAwareExpressionEvaluator typeAware) { + result = typeAware.evaluate(value, type); + } else if (evaluator != null) { + result = evaluator.evaluate(value); + } else { + result = value; + } + } catch (ExpressionEvaluationException e) { + throw new ComponentConfigurationException( + configuration, + String.format( + "Cannot evaluate expression '%s' for configuration entry '%s'", + value, configuration.getName()), + e); + } + } else if (value != null) { + // Explicit empty content: <element></element> + result = ""; + } else { + // value == null: self-closing tag <element/> + String defaultValue = configuration.getAttribute("default-value"); + if (defaultValue != null && !defaultValue.isEmpty()) { + try { + if (evaluator instanceof TypeAwareExpressionEvaluator typeAware) { + result = typeAware.evaluate(defaultValue, type); + } else if (evaluator != null) { + result = evaluator.evaluate(defaultValue); + } else { + result = defaultValue; + } + } catch (ExpressionEvaluationException e) { + throw new ComponentConfigurationException( + configuration, + String.format( + "Cannot evaluate expression '%s' for configuration entry '%s'", + defaultValue, configuration.getName()), + e); + } + } else { + result = ""; + } + } + + if (result == null) { Review Comment: 🔵 **Robustness (low).** If the evaluator returns a non-String object (e.g., an expression that resolves to an Integer), `result.toString()` works but silently converts it. The Plexus `BasicStringConverter` had the same behavior, so this isn't a regression, but a `failIfNotTypeCompatible` check (like in `EnhancedConfigurationConverter.fromExpression`) would be more defensive. ########## impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedConfigurationConverter.java: ########## @@ -54,8 +54,10 @@ protected Object fromExpression( } else { result = evaluator.evaluate(value); } + } else if (null != value && (type == String.class || type == CharSequence.class || type == Object.class || type == null)) { + result = ""; } - if (null == result && configuration.getChildCount() == 0) { + if (null == result && configuration.getChildCount() == 0 && configuration.getValue() == null) { Review Comment: 💡 **Interaction concern (medium).** Two changes here work together but have a subtle side-effect: 1. Line 57-58: For `value==""` (empty tag) and String/CharSequence/Object types, `result` is set to `""`. 2. Line 60: The `default-value` fallback now requires `configuration.getValue() == null` — so it only triggers for self-closing `<tag/>`, not for `<tag></tag>`. This is correct for String types (handled by the new else-if), but for non-String, non-Object types where `value==""` (e.g., a composite bean parameter written as `<myBean></myBean>`), the old code would fall through to check `default-value`. Now it won't, because `getValue()` is `""`, not `null`. In practice this path is rarely hit (bean types aren't typically configured as empty tags with default-value attributes), but it IS a behavioral change. Consider whether the guard should be `configuration.getValue() == null || configuration.getValue().isEmpty()` instead — or document this is intentional. ########## impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java: ########## @@ -838,7 +838,7 @@ private void populateMojoExecutionFields( Collection<Parameter> missingParameters = validator.getMissingParameters(); if (!missingParameters.isEmpty()) { - if ("basic".equals(configuratorId)) { + if ("basic".equals(configuratorId) || "enhanced".equals(configuratorId)) { Review Comment: 💡 **Correct but worth noting (low).** Adding `"enhanced"` to the validation path is the right call — the enhanced configurator should also throw `PluginParameterException` for missing required parameters. Previously, enhanced-configured mojos would fall through to the generic `validateParameters` path which has different (weaker) validation semantics. ########## impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedCompositeBeanHelper.java: ########## @@ -307,6 +316,29 @@ private void setFieldValue(Object bean, Field field, Object value) throws Illega field.set(bean, value); } + private Object getEmptyValueForType(Class<?> type, PlexusConfiguration configuration) { + if (configuration.getChildCount() == 0) { + String value = configuration.getValue(); + if (value != null && value.isEmpty()) { + return createEmptyValue(type); + } else if (value == null && configuration.getAttribute("default-value") == null) { + return createEmptyValue(type); + } + } + return null; + } + + private Object createEmptyValue(Class<?> type) { + if (String.class.equals(type) || CharSequence.class.equals(type)) { + return ""; + } else if (StringBuilder.class.equals(type)) { + return new StringBuilder(""); + } else if (StringBuffer.class.equals(type)) { + return new StringBuffer(""); + } + return null; + } Review Comment: 💡 **Duplication concern (low).** `getEmptyValueForType` + `createEmptyValue` duplicate logic that also exists in `EnhancedStringConverter`. The converter handles the same types (String, CharSequence, StringBuilder, StringBuffer) with the same empty-value semantics. In the current call chain, when `convertProperty` calls `lookup.lookupConverterForType(String.class)`, it gets `EnhancedStringConverter` which returns `""` — so `getEmptyValueForType` is only reached when `convertProperty` returns null, which for String types should no longer happen with the new converter. This means `getEmptyValueForType` is effectively dead code for String types when using the enhanced converter lookup. It might still be needed for the `setDefault` path (line 103) where the conversion might go through a different code path. Consider adding a comment explaining when this fallback is actually needed. ########## impl/maven-core/src/test/java/org/apache/maven/configuration/DefaultBeanConfiguratorTest.java: ########## @@ -172,6 +172,42 @@ void testSealedTypeAmbiguousSimpleNameThrowsError() { assertTrue(e.getMessage().contains("is ambiguous for sealed type " + AmbiguousSealedType.class.getName())); } + @Test + void testNestedSettingOverridesPreInitializedDefaultWhenEmpty() throws BeanConfigurationException { + PluginConfigBean bean = new PluginConfigBean(); + assertEquals("article,report,book", bean.settings.docClassesToTargets); + + Xpp3Dom config = toConfig("<settings><docClassesToTargets></docClassesToTargets></settings>"); + DefaultBeanConfigurationRequest request = new DefaultBeanConfigurationRequest(); + request.setBean(bean).setConfiguration(config); + + configurator.configureBean(request); + assertEquals("", bean.settings.docClassesToTargets); + } + + @Test + void testNestedSettingOverridesPreInitializedDefaultWhenSelfClosing() throws BeanConfigurationException { + PluginConfigBean bean = new PluginConfigBean(); + assertEquals("article,report,book", bean.settings.docClassesToTargets); + + Xpp3Dom config = toConfig("<settings><docClassesToTargets/></settings>"); + DefaultBeanConfigurationRequest request = new DefaultBeanConfigurationRequest(); + request.setBean(bean).setConfiguration(config); + + configurator.configureBean(request); + assertEquals("", bean.settings.docClassesToTargets); + } + + public static class PluginConfigBean { + + public Settings settings = new Settings(); + } + + public static class Settings { + + public String docClassesToTargets = "article,report,book"; Review Comment: 🔴 **Missing test coverage (high).** The tests cover String fields but miss critical edge cases: 1. **Non-String types with empty tags:** What happens with `<count></count>` where `count` is `int`? Does it get 0, null, or throw? This is the most likely regression vector. 2. **Boolean with self-closing tag:** `<skip/>` where `skip` is `boolean` — does it stay at its Java default or get `false`? 3. **String field with `default-value` attribute + empty tag:** `<param default-value="fallback"></param>` — the `EnhancedCompositeBeanHelperTest` covers this but `DefaultBeanConfiguratorTest` (which exercises the full stack) doesn't. 4. **Expression in empty-adjacent tag:** `<param>${undefined}</param>` — does the evaluator return null, and if so, does the empty-value fallback kick in? At minimum, add a test for case 1 to verify no regression on non-String types. ########## impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedStringConverter.java: ########## @@ -0,0 +1,120 @@ +/* + * 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.configuration.internal; + +import org.codehaus.plexus.component.configurator.ComponentConfigurationException; +import org.codehaus.plexus.component.configurator.ConfigurationListener; +import org.codehaus.plexus.component.configurator.converters.AbstractConfigurationConverter; +import org.codehaus.plexus.component.configurator.converters.lookup.ConverterLookup; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluationException; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluator; +import org.codehaus.plexus.component.configurator.expression.TypeAwareExpressionEvaluator; +import org.codehaus.plexus.configuration.PlexusConfiguration; + +/** + * Converter for String, CharSequence, StringBuilder, and StringBuffer that properly handles + * empty configuration elements (e.g. {@code <setting></setting>} or {@code <setting/>}). + */ +class EnhancedStringConverter extends AbstractConfigurationConverter { + + @Override + public boolean canConvert(Class<?> type) { + return String.class.equals(type) + || CharSequence.class.equals(type) + || StringBuilder.class.equals(type) + || StringBuffer.class.equals(type); + } + + @Override + public Object fromConfiguration( + ConverterLookup lookup, + PlexusConfiguration configuration, + Class<?> type, + Class<?> enclosingType, + ClassLoader loader, + ExpressionEvaluator evaluator, + ConfigurationListener listener) + throws ComponentConfigurationException { + + if (configuration.getChildCount() > 0) { + throw new ComponentConfigurationException( + "Basic element '" + configuration.getName() + "' must not contain child elements"); + } + + String value = configuration.getValue(); + Object result = null; + + if (value != null && !value.isEmpty()) { + try { + if (evaluator instanceof TypeAwareExpressionEvaluator typeAware) { + result = typeAware.evaluate(value, type); + } else if (evaluator != null) { + result = evaluator.evaluate(value); + } else { + result = value; + } + } catch (ExpressionEvaluationException e) { + throw new ComponentConfigurationException( + configuration, + String.format( + "Cannot evaluate expression '%s' for configuration entry '%s'", + value, configuration.getName()), + e); + } + } else if (value != null) { + // Explicit empty content: <element></element> + result = ""; + } else { + // value == null: self-closing tag <element/> + String defaultValue = configuration.getAttribute("default-value"); + if (defaultValue != null && !defaultValue.isEmpty()) { + try { + if (evaluator instanceof TypeAwareExpressionEvaluator typeAware) { + result = typeAware.evaluate(defaultValue, type); + } else if (evaluator != null) { + result = evaluator.evaluate(defaultValue); + } else { + result = defaultValue; + } + } catch (ExpressionEvaluationException e) { + throw new ComponentConfigurationException( + configuration, + String.format( + "Cannot evaluate expression '%s' for configuration entry '%s'", + defaultValue, configuration.getName()), + e); + } + } else { + result = ""; + } Review Comment: ⚠️ **Behavioral regression risk (medium).** For self-closing `<tag/>` (value==null) with no `default-value` attribute, this returns `""`. Before this PR, the Plexus `BasicStringConverter` would return `null`, leaving the bean field at its Java-initialized default. This is intentional for the MNG-7927 fix, but it changes the contract: plugins that use `<param/>` to mean "no value / keep default" will now get an empty string instead. Consider whether this should only apply when there's explicit empty content (`<param></param>`, where `getValue()` returns `""`) but NOT for truly self-closing tags (`<param/>`, where `getValue()` returns `null`): ```suggestion } else { // value == null: self-closing tag <element/> String defaultValue = configuration.getAttribute("default-value"); if (defaultValue != null && !defaultValue.isEmpty()) { try { if (evaluator instanceof TypeAwareExpressionEvaluator typeAware) { result = typeAware.evaluate(defaultValue, type); } else if (evaluator != null) { result = evaluator.evaluate(defaultValue); } else { result = defaultValue; } } catch (ExpressionEvaluationException e) { throw new ComponentConfigurationException( configuration, String.format( "Cannot evaluate expression '%s' for configuration entry '%s'", defaultValue, configuration.getName()), e); } } // For self-closing <tag/> without default-value, return null // to preserve existing behavior (keep bean field's Java default) } ``` If returning `""` for self-closing tags IS the desired behavior, this needs explicit documentation in the class Javadoc and tests that verify this contract. -- 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]
