Skip to content

Commit

Permalink
Xpp3DomUtils#mergeIntoXpp3Dom() must not override the dominant value …
Browse files Browse the repository at this point in the history 
…in case it is empty (#216)

This fixes #216 and closes #217
  • Loading branch information
@kwin @michael-o
kwin authored and michael-o committed Oct 15, 2022
1 parent 748933c commit 67ac243
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 9 deletions.
8 changes: 0 additions & 8 deletions src/main/java/org/codehaus/plexus/util/xml/Xpp3DomUtils.java
View file
Expand Up @@ -95,8 +95,6 @@ public void writeToSerializer( String namespace, XmlSerializer serializer, Xpp3D
* </ol></li>
* <li> If mergeSelf == true
* <ol type="A">
* <li> if the dominant root node's value is empty, set it to the recessive root node's value</li>

This comment has been minimized.

Sign in to view

Copy link
@guylabs

guylabs Jan 4, 2023

Cross posting it from apache/maven#866 (comment).

@kwin @michael-o Why has this behavior been removed? Is there a migration path that you can share?

The following example has worked before that change where we have a parent pom and a child pom and a configuration that we'd like to merge. Here the parent and child pom definitions:

<!--parent pom-->
  ...
<pluginManagement>
  <plugins>
    <plugin>
      <groupId>foo.bar</groupId>
      <artifactId>foo-bar-plugin</artifactId>
      <configuration>
        <plugins>
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <bar>
              <value>foo</value>
            </bar>
          </plugin>
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <foo>
              <properties>
                <property>
                  <name>prop1</name>
                  <value>value1</value>
                </property>
              </properties>
            </foo>
          </plugin>
        </plugins>
      </configuration>
    </plugin>
  </plugins>
</pluginManagement>
  ...
  <!--child pom-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

And the expected effective child pom that is used after the merge:

  <!--expected effective child pom-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
          <bar>
            <value>foo</value>
          </bar>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop1</name>
                <value>value1</value>
              </property>
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

The problem we see now after this change, is that it will be merged the following way:

<!--expected effective child pom after this change-->
  ...
<pluginManagement>
<plugins>
  <plugin>
    <groupId>foo.bar</groupId>
    <artifactId>foo-bar-plugin</artifactId>
    <configuration>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-compiler-plugin</artifactId>
          <foo>
            <properties>
              <property>
                <name>prop1</name>
                <value>value1</value>
              </property>
            </properties>
          </foo>
        </plugin>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-surefire-plugin</artifactId>
          <foo>
            <properties combine.children="append">
              <property>
                <name>prop2</name>
                <value>value2</value>
              </property>
            </properties>
          </foo>
        </plugin>
      </plugins>
    </configuration>
  </plugin>
</plugins>
</pluginManagement>
  ...

The problems are the following:

  1. On the maven-compiler-plugin configuration the <bar> node is not merged from the parent pom configuration but rather the one from the maven-surefire-plugin configuration from the parent pom.
  2. The maven-surefire-plugin configuration not as expected as the children of the <properties> node are not appended as configured via the combine.children="append" property.

Is that changed something that stays for Maven 4 ? I assume that then a migration path should be given as there are projects that use the behavior before this change and it would result in different effective pom files, which will change how the build is configured.

I asked about it on the ASF Slack here already and it seems this is the right place for it.

This comment has been minimized.

Sign in to view

Copy link
@kwin

kwin Jan 5, 2023 •
edited

Author Contributor

@guylabs Please report a dedicated issue with reproducible steps. Your example above has an unknown foo-bar-plugin containing a configuration with plugins having configurations themselves. That is unusual to say the least and seems specific to that foo-bar-plugin.... Also I currently fail to see the relation with this change, as this would only be effective if the dominant value is empty (which I don't see here).

This comment has been minimized.

Sign in to view

Copy link
@guylabs

guylabs Jan 5, 2023

@kwin Created an issue and an accompanying reproducer. See #230
* <li> For each attribute in the recessive root node which is not set in the dominant root node, set it.</li>
* <li> Determine whether children from the recessive DOM will be merged or appended to the dominant DOM as
* siblings (flag=mergeChildren).
* <ol type="i">
Expand Down Expand Up @@ -140,12 +138,6 @@ private static void mergeIntoXpp3Dom( Xpp3Dom dominant, Xpp3Dom recessive, Boole

if ( mergeSelf )
{
if ( isEmpty( dominant.getValue() ) && !isEmpty( recessive.getValue() ) )
{
dominant.setValue( recessive.getValue() );
dominant.setInputLocation( recessive.getInputLocation() );
}

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Oct 24, 2022 •
edited

Member

@kwin This would have to be ported to the maven 4.x api:
https://github.com/apache/maven/blob/master/maven-xml-impl/src/main/java/org/apache/maven/internal/xml/Xpp3Dom.java#L266-L270
String[] recessiveAttrs = recessive.getAttributeNames();
for ( String attr : recessiveAttrs )
{
Expand Down
16 changes: 15 additions & 1 deletion src/test/java/org/codehaus/plexus/util/xml/Xpp3DomUtilsTest.java
View file
Expand Up @@ -115,7 +115,7 @@ public void testCombineKeys()
}

@Test
public void testOverwriteDominantBlankValue() throws XmlPullParserException, IOException {
public void testPreserveDominantBlankValue() throws XmlPullParserException, IOException {
String lhs = "<parameter xml:space=\"preserve\"> </parameter>";

String rhs = "<parameter>recessive</parameter>";
Expand All @@ -127,6 +127,20 @@ public void testOverwriteDominantBlankValue() throws XmlPullParserException, IOE
assertEquals( " ", mergeResult.getValue() );
}

@Test
public void testPreserveDominantEmptyNode() throws XmlPullParserException, IOException
{
String lhs = "<parameter></parameter>";

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Nov 9, 2022

Member

@kwin what should be the expected result if we have <parameter/> ?
It currently gives a null value in the mergeResult.getValue().
Also, this means that there's no way to override any value at all ?

This comment has been minimized.

Sign in to view

Copy link
@gnodet

gnodet Nov 9, 2022

Member

I'll mimic this behavior in maven 4, but I'm slightly worried.

This comment has been minimized.

Sign in to view

Copy link
@kwin

kwin Nov 9, 2022

Author Contributor

You always override with the dominant value!

String rhs = "<parameter>recessive</parameter>";

Xpp3Dom leftDom = Xpp3DomBuilder.build( new StringReader( lhs ), new FixedInputLocationBuilder( "left" ) );
Xpp3Dom rightDom = Xpp3DomBuilder.build( new StringReader( rhs ), new FixedInputLocationBuilder( "right" ) );

Xpp3Dom mergeResult = Xpp3DomUtils.mergeXpp3Dom( leftDom, rightDom, true );
assertEquals( "", mergeResult.getValue() );
}

@Test
public void testIsNotEmptyNegatesIsEmpty()
{
Expand Down

0 comments on commit 67ac243

Please sign in to comment.