gnodet-bot commented on code in PR #12936: URL: https://github.com/apache/maven/pull/12936#discussion_r4061238541
########## maven-model-builder/src/test/java/org/apache/maven/model/interpolation/StringVisitorModelInterpolatorTest.java: ########## @@ -0,0 +1,39 @@ +/* + * 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.model.interpolation; + +import org.apache.maven.model.path.PathTranslator; +import org.junit.jupiter.api.BeforeEach; + Review Comment: ⚠️ **Failing inherited tests — blocks merge** The CI run for this PR reports two failures in this class: ``` StringVisitorModelInterpolatorTest>AbstractModelInterpolatorTest.testBaseUri:337 expected: <myBaseUri/temp-repo> but was: <${project.baseUri}/temp-repo> StringVisitorModelInterpolatorTest>AbstractModelInterpolatorTest.testBasedir:315 expected: <file://localhost/myBasedir/temp-repo> but was: <file://localhost/${basedir}/temp-repo> ``` These tests are inherited from `AbstractModelInterpolatorTest`. They pass system properties (`basedir`, `project.baseUri`) via `ModelBuildingRequest` and expect `StringVisitorModelInterpolator` to resolve them — but it doesn't. Two paths to fix this: 1. **Scope the new class to just the NPE regression test.** Override the inherited tests that don't apply to `StringVisitorModelInterpolator`'s value-source semantics, or exclude them with `@Disabled` and a comment explaining why. 2. **Fix `StringVisitorModelInterpolator`** to add a `PropertiesBasedValueSource` for the system properties from `ModelBuildingRequest`, so it resolves `${basedir}` and `${project.baseUri}` the same way `StringSearchModelInterpolator` does. Option 1 is the minimal fix that keeps this PR's scope tight. Option 2 is the correct long-term fix but expands scope significantly. Either way, merging a test class where two tests fail is not acceptable. ########## maven-model-builder/src/test/java/org/apache/maven/model/interpolation/AbstractModelInterpolatorTest.java: ########## @@ -457,6 +459,37 @@ public void testShouldInterpolateUnprefixedBasedirExpression() throws Exception new File(rDeps.get(0).getSystemPath()).getAbsolutePath()); } + @Test + public void testRecursiveExpressionCycleInPluginConfiguration() throws Exception { + // MNG-8174: a self-referencing property used in a plugin configuration attribute must not + // cause an NPE while interpolating the plugin configuration + Model model = new Model(); + model.addProperty("prop", "${prop}"); + + Xpp3Dom configuration = new Xpp3Dom("configuration"); + Xpp3Dom filter = new Xpp3Dom("filter"); + filter.setAttribute("token", "key"); + filter.setAttribute("value", "${prop}"); + configuration.addChild(filter); + + Plugin plugin = new Plugin(); + plugin.setArtifactId("maven-antrun-plugin"); + plugin.setConfiguration(configuration); + + Build build = new Build(); + build.addPlugin(plugin); + model.setBuild(build); + + SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelInterpolator interpolator = createInterpolator(); + Model out = interpolator.interpolateModel(model, null, createModelBuildingRequest(context), collector); + + assertNotNull(out); + assertCollectorState(0, 2, 0, collector); + Xpp3Dom outConfig = (Xpp3Dom) out.getBuild().getPlugins().get(0).getConfiguration(); + assertEquals("${prop}", outConfig.getChild("filter").getAttribute("value")); + } Review Comment: 💡 **Missing coverage: text-content path** The fix also guards `dom.setValue(val)` (line 717 of `StringVisitorModelInterpolator.java`) but this path is not exercised here — the `<filter>` element has no text content, so `dom.getValue()` returns `null` and the guard is never reached. Consider adding a `filter.setValue("${prop}")` before `configuration.addChild(filter)` and asserting `outConfig.getChild("filter").getValue()` equals `"${prop}"` after interpolation. This validates both halves of the fix. ```suggestion Xpp3Dom filter = new Xpp3Dom("filter"); filter.setAttribute("token", "key"); filter.setAttribute("value", "${prop}"); filter.setValue("${prop}"); configuration.addChild(filter); ``` -- 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]
