Copilot commented on code in PR #13348:
URL: https://github.com/apache/maven/pull/13348#discussion_r4316245760
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedConfigurationConverter.java:
##########
@@ -54,6 +54,8 @@ protected Object fromExpression(
} else {
result = evaluator.evaluate(value);
}
+ } else if (null != value && (type == String.class || type ==
CharSequence.class || type == Object.class || type == null)) {
Review Comment:
This condition sets `result` to an empty string for any non-null `value`,
without checking `value.isEmpty()`. That would turn a normal non-empty literal
(when the earlier branches don’t run) into `\"\"`, which is incorrect and can
break configuration parsing. If the intent is to treat only explicit empty
content (`value != null && value.isEmpty()`) as empty-string, add that check
(and ensure `value` is preserved as-is for non-empty literals when
evaluator/expression handling is not used).
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedStringConverter.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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
+ * explicit empty configuration elements (e.g. {@code <setting></setting>}) by
converting them
+ * to empty strings, while preserving existing behavior for self-closing tags
without default-values
+ * (e.g. {@code <setting/>}) by returning {@code null} to keep the bean
field's Java default.
+ */
+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);
+ }
+ }
+ // For self-closing <tag/> without default-value, leave result null
+ // to preserve existing behavior (keep bean field's Java default)
+ }
+
+ if (result == null) {
+ return null;
+ }
+
+ if (!type.isInstance(result) && !(result instanceof CharSequence)) {
+ failIfNotTypeCompatible(result, type, configuration);
+ }
+
+ String str = result.toString();
+ if (StringBuilder.class.equals(type)) {
+ return new StringBuilder(str);
+ } else if (StringBuffer.class.equals(type)) {
+ return new StringBuffer(str);
+ }
+ return str;
Review Comment:
This rejects expression-evaluator results that are not `CharSequence` (e.g.,
`File` for `${basedir}`), which historically is typically accepted for
String-typed parameters by converting via `toString()`. This looks like a
behavior regression for common Maven/Plexus expression results. Consider
allowing any non-null `result` for String/CharSequence targets (and converting
with `toString()`), and only enforce stricter type-compatibility for
`StringBuilder`/`StringBuffer` if needed.
##########
impl/maven-core/src/main/java/org/apache/maven/configuration/internal/EnhancedStringConverter.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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
+ * explicit empty configuration elements (e.g. {@code <setting></setting>}) by
converting them
+ * to empty strings, while preserving existing behavior for self-closing tags
without default-values
+ * (e.g. {@code <setting/>}) by returning {@code null} to keep the bean
field's Java default.
+ */
+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) {
Review Comment:
Given the behavioral sensitivity here, it would be valuable to add a test
that covers an evaluator returning a non-`CharSequence` object for a
String-typed parameter (e.g., returning a `File`), asserting the configured
field becomes the object’s `toString()` rather than failing type checks or
producing an empty value.
##########
api/maven-api-model/src/main/mdo/maven.mdo:
##########
@@ -2589,6 +2589,17 @@
<li>{@code combine.self}: available values are {@code merge}
(default) and {@code override}.</li>
</ul>
+ <p>Since Maven 4.1.0, empty string plugin parameters have distinct
semantics based on XML form:</p>
+ <ul>
+ <li><b>Explicit empty tag</b> ({@code <param></param>}): overrides
the mojo's pre-initialized
+ Java default with an empty value (empty string for {@code
String}, {@code CharSequence},
+ {@code StringBuilder} and {@code StringBuffer} fields).</li>
+ <li><b>Self-closing tag</b> ({@code <param/>}): leaves the mojo's
Java-initialized default
+ unchanged, i.e. is a no-op for that parameter.</li>
+ </ul>
Review Comment:
The PR description states that both explicit empty tags (`<tag></tag>`) and
self-closing tags (`<tag/>`) should override pre-initialized defaults, but the
updated documentation (and added tests) explicitly define `<tag/>` as a no-op
that preserves defaults. Please reconcile this by updating the PR description
to match the implemented semantics, or changing the implementation/tests/docs
if the intended behavior is actually to override on self-closing tags as well.
##########
api/maven-api-model/src/main/mdo/maven.mdo:
##########
@@ -2589,6 +2589,17 @@
<li>{@code combine.self}: available values are {@code merge}
(default) and {@code override}.</li>
</ul>
+ <p>Since Maven 4.1.0, empty string plugin parameters have distinct
semantics based on XML form:</p>
Review Comment:
Hard-coding a specific version (“Since Maven 4.1.0”) can become inaccurate
if this change ships in a different release line or is backported. Consider
wording that doesn’t depend on a specific version (e.g., “Starting with Maven
4.x” / “In Maven 4+”), or ensure the referenced version is guaranteed for this
behavior across supported branches.
--
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]
