Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-7596] Upgrade to plexus 3.5.0 #866

Merged
merged 1 commit into from
Dec 1, 2022
Merged

[MNG-7596] Upgrade to plexus 3.5.0 #866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/maven-api-xml/src/main/java/org/apache/maven/api/xml/Dom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ public interface Dom {

String SELF_COMBINATION_REMOVE = "remove";

/**
* In case of complex XML structures, combining can be done based on id.
*/
String ID_COMBINATION_MODE_ATTRIBUTE = "combine.id";

/**
* In case of complex XML structures, combining can be done based on keys.
* This is a comma separated list of attribute names.
*/
String KEYS_COMBINATION_MODE_ATTRIBUTE = "combine.keys";

/**
* This default mode for combining a DOM node during merge means that where element names match, the process will
* try to merge the element attributes and values, rather than overriding the recessive element completely with the
Expand Down
90 changes: 61 additions & 29 deletions maven-xml-impl/src/main/java/org/apache/maven/internal/xml/Xpp3Dom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.ListIterator;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand Down Expand Up @@ -180,8 +181,6 @@ public void writeToSerializer(String namespace, XmlSerializer serializer) throws
* </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>
* <li> For each attribute in the recessive root node which is not set in the dominant root node, set it.</li>
Comment on lines -183 to -184
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet 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'm also open to chat about it on the ASF Slack if required or if you have further questions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if that can help, but big efforts were made in the past on maven-shared-utils reimplementation, with its test suite: https://github.com/apache/maven-shared-utils/blob/master/src/test/java/org/apache/maven/shared/utils/xml/pull/Xpp3DomTest.java
perhaps that can be reused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guylabs I'm not really the author of the change, I simply back ported it because the code is duplicated somehow from plexus-utils. The original change is codehaus-plexus/plexus-utils@67ac243

* <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 @@ -222,16 +221,11 @@ public static Dom merge(Dom dominant, Dom recessive, Boolean childMergeOverride)

if (mergeSelf) {

String value = null;
Object location = null;
String value = dominant.getValue();
Object location = dominant.getInputLocation();
Map<String, String> attrs = null;
List<Dom> children = null;

if (isEmpty(dominant.getValue()) && !isEmpty(recessive.getValue())) {
value = recessive.getValue();
location = recessive.getInputLocation();
}

for (Map.Entry<String, String> attr : recessive.getAttributes().entrySet()) {
String key = attr.getKey();
if (isEmpty(dominant.getAttribute(key)) && !SELF_COMBINATION_MODE_ATTRIBUTE.equals(key)) {
Expand All @@ -253,25 +247,55 @@ public static Dom merge(Dom dominant, Dom recessive, Boolean childMergeOverride)
}
}

if (!mergeChildren) {
children = new ArrayList<>(recessive.getChildren().size()
+ dominant.getChildren().size());
children.addAll(recessive.getChildren());
children.addAll(dominant.getChildren());
} else {
Map<String, Iterator<Dom>> commonChildren = new HashMap<>();
Set<String> names =
recessive.getChildren().stream().map(Dom::getName).collect(Collectors.toSet());
for (String name : names) {
List<Dom> dominantChildren = dominant.getChildren().stream()
.filter(n -> n.getName().equals(name))
.collect(Collectors.toList());
if (dominantChildren.size() > 0) {
commonChildren.put(name, dominantChildren.iterator());
String keysValue = recessive.getAttribute(KEYS_COMBINATION_MODE_ATTRIBUTE);

for (Dom recessiveChild : recessive.getChildren()) {
String idValue = recessiveChild.getAttribute(ID_COMBINATION_MODE_ATTRIBUTE);

Dom childDom = null;
if (isNotEmpty(idValue)) {
for (Dom dominantChild : dominant.getChildren()) {
if (idValue.equals(dominantChild.getAttribute(ID_COMBINATION_MODE_ATTRIBUTE))) {
childDom = dominantChild;
// we have a match, so don't append but merge
mergeChildren = true;
}
}
} else if (isNotEmpty(keysValue)) {
String[] keys = keysValue.split(",");
Map<String, Optional<String>> recessiveKeyValues = Stream.of(keys)
.collect(Collectors.toMap(
k -> k, k -> Optional.ofNullable(recessiveChild.getAttribute(k))));

for (Dom dominantChild : dominant.getChildren()) {
Map<String, Optional<String>> dominantKeyValues = Stream.of(keys)
.collect(Collectors.toMap(
k -> k, k -> Optional.ofNullable(dominantChild.getAttribute(k))));

if (recessiveKeyValues.equals(dominantKeyValues)) {
childDom = dominantChild;
// we have a match, so don't append but merge
mergeChildren = true;
}
}
} else {
childDom = dominant.getChild(recessiveChild.getName());
}

for (Dom recessiveChild : recessive.getChildren()) {
if (mergeChildren && childDom != null) {
Map<String, Iterator<Dom>> commonChildren = new HashMap<>();
Set<String> names = recessive.getChildren().stream()
.map(Dom::getName)
.collect(Collectors.toSet());
for (String name : names) {
List<Dom> dominantChildren = dominant.getChildren().stream()
.filter(n -> n.getName().equals(name))
.collect(Collectors.toList());
if (dominantChildren.size() > 0) {
commonChildren.put(name, dominantChildren.iterator());
}
}

String name = recessiveChild.getName();
Iterator<Dom> it =
commonChildren.computeIfAbsent(name, n1 -> Stream.of(dominant.getChildren().stream()
Expand All @@ -297,7 +321,7 @@ public static Dom merge(Dom dominant, Dom recessive, Boolean childMergeOverride)
}
children.remove(dominantChild);
} else {
int idx = (children != null ? children : dominant.getChildren()).indexOf(dominantChild);
int idx = dominant.getChildren().indexOf(dominantChild);
Dom merged = merge(dominantChild, recessiveChild, childMergeOverride);
if (merged != dominantChild) {
if (children == null) {
Expand All @@ -307,6 +331,14 @@ public static Dom merge(Dom dominant, Dom recessive, Boolean childMergeOverride)
}
}
}
} else {
if (children == null) {
children = new ArrayList<>(dominant.getChildren());
}
int idx = mergeChildren
? children.size()
: recessive.getChildren().indexOf(recessiveChild);
children.add(idx, recessiveChild);
}
}
}
Expand Down Expand Up @@ -381,11 +413,11 @@ public String toUnescapedString() {
return writer.toString();
}

public static boolean isNotEmpty(String str) {
private static boolean isNotEmpty(String str) {
return ((str != null) && (str.length() > 0));
}

public static boolean isEmpty(String str) {
return ((str == null) || (str.trim().length() == 0));
private static boolean isEmpty(String str) {
return ((str == null) || (str.length() == 0));
}
}
9 changes: 8 additions & 1 deletion maven-xml-impl/src/main/java/org/apache/maven/internal/xml/Xpp3DomBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,10 @@ public static Xpp3Dom build(XmlPullParser parser, boolean trim, InputLocationBui
Map<String, String> attrs = null;
List<Dom> children = null;
int eventType = parser.getEventType();
boolean emptyTag = false;
while (eventType != XmlPullParser.END_DOCUMENT) {
if (eventType == XmlPullParser.START_TAG) {
emptyTag = parser.isEmptyElementTag();
if (name == null) {
name = parser.getName();
location = locationBuilder != null ? locationBuilder.toInputLocation(parser) : null;
Expand Down Expand Up @@ -158,7 +160,12 @@ public static Xpp3Dom build(XmlPullParser parser, boolean trim, InputLocationBui
}
value = value != null ? value + text : text;
} else if (eventType == XmlPullParser.END_TAG) {
return new Xpp3Dom(name, children == null ? value : null, attrs, children, location);
return new Xpp3Dom(
name,
children == null ? (value != null ? value : emptyTag ? null : "") : null,
attrs,
children,
location);
}
eventType = parser.next();
}
Expand Down
188 changes: 188 additions & 0 deletions maven-xml-impl/src/test/java/org/apache/maven/internal/xml/Xpp3DomTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
/*
* 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.internal.xml;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.io.IOException;
import java.io.StringReader;
import java.util.List;
import java.util.stream.Collectors;
import org.apache.maven.api.xml.Dom;
import org.codehaus.plexus.util.xml.pull.XmlPullParser;
import org.codehaus.plexus.util.xml.pull.XmlPullParserException;
import org.junit.jupiter.api.Test;

public class Xpp3DomTest {

/**
* <p>testCombineId.</p>
*
* @throws java.lang.Exception if any.
*/
@Test
public void testCombineId() throws Exception {
String lhs = "<props>" + "<property combine.id='LHS-ONLY'><name>LHS-ONLY</name><value>LHS</value></property>"
+ "<property combine.id='TOOVERWRITE'><name>TOOVERWRITE</name><value>LHS</value></property>"
+ "</props>";

String rhs = "<props>" + "<property combine.id='RHS-ONLY'><name>RHS-ONLY</name><value>RHS</value></property>"
+ "<property combine.id='TOOVERWRITE'><name>TOOVERWRITE</name><value>RHS</value></property>"
+ "</props>";

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

Dom mergeResult = Xpp3Dom.merge(leftDom, rightDom, true);
assertEquals(3, getChildren(mergeResult, "property").size());

Dom p0 = getNthChild(mergeResult, "property", 0);
assertEquals("LHS-ONLY", p0.getChild("name").getValue());
assertEquals("left", p0.getChild("name").getInputLocation());
assertEquals("LHS", p0.getChild("value").getValue());
assertEquals("left", p0.getChild("value").getInputLocation());

Dom p1 = getNthChild(mergeResult, "property", 1);
assertEquals(
"TOOVERWRITE",
getNthChild(mergeResult, "property", 1).getChild("name").getValue());
assertEquals("left", p1.getChild("name").getInputLocation());
assertEquals(
"LHS", getNthChild(mergeResult, "property", 1).getChild("value").getValue());
assertEquals("left", p1.getChild("value").getInputLocation());

Dom p2 = getNthChild(mergeResult, "property", 2);
assertEquals(
"RHS-ONLY",
getNthChild(mergeResult, "property", 2).getChild("name").getValue());
assertEquals("right", p2.getChild("name").getInputLocation());
assertEquals(
"RHS", getNthChild(mergeResult, "property", 2).getChild("value").getValue());
assertEquals("right", p2.getChild("value").getInputLocation());
}

/**
* <p>testCombineKeys.</p>
*
* @throws java.lang.Exception if any.
*/
@Test
public void testCombineKeys() throws Exception {
String lhs = "<props combine.keys='key'>"
+ "<property key=\"LHS-ONLY\"><name>LHS-ONLY</name><value>LHS</value></property>"
+ "<property combine.keys='name'><name>TOOVERWRITE</name><value>LHS</value></property>" + "</props>";

String rhs = "<props combine.keys='key'>"
+ "<property key=\"RHS-ONLY\"><name>RHS-ONLY</name><value>RHS</value></property>"
+ "<property combine.keys='name'><name>TOOVERWRITE</name><value>RHS</value></property>" + "</props>";

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

Dom mergeResult = Xpp3Dom.merge(leftDom, rightDom, true);
assertEquals(3, getChildren(mergeResult, "property").size());

Dom p0 = getNthChild(mergeResult, "property", 0);
assertEquals("LHS-ONLY", p0.getChild("name").getValue());
assertEquals("left", p0.getChild("name").getInputLocation());
assertEquals("LHS", p0.getChild("value").getValue());
assertEquals("left", p0.getChild("value").getInputLocation());

Dom p1 = getNthChild(mergeResult, "property", 1);
assertEquals(
"TOOVERWRITE",
getNthChild(mergeResult, "property", 1).getChild("name").getValue());
assertEquals("left", p1.getChild("name").getInputLocation());
assertEquals(
"LHS", getNthChild(mergeResult, "property", 1).getChild("value").getValue());
assertEquals("left", p1.getChild("value").getInputLocation());

Dom p2 = getNthChild(mergeResult, "property", 2);
assertEquals(
"RHS-ONLY",
getNthChild(mergeResult, "property", 2).getChild("name").getValue());
assertEquals("right", p2.getChild("name").getInputLocation());
assertEquals(
"RHS", getNthChild(mergeResult, "property", 2).getChild("value").getValue());
assertEquals("right", p2.getChild("value").getInputLocation());
}

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

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"));

Dom mergeResult = Xpp3Dom.merge(leftDom, rightDom, true);
assertEquals(" ", mergeResult.getValue());
}

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

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"));

Dom mergeResult = Xpp3Dom.merge(leftDom, rightDom, true);
assertEquals("", mergeResult.getValue());
}

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

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"));

Dom mergeResult = Xpp3Dom.merge(leftDom, rightDom, true);
assertEquals(null, mergeResult.getValue());
}

private static List<Dom> getChildren(Dom node, String name) {
return node.getChildren().stream().filter(n -> n.getName().equals(name)).collect(Collectors.toList());
}

private static Dom getNthChild(Dom node, String name, int nth) {
return node.getChildren().stream()
.filter(n -> n.getName().equals(name))
.skip(nth)
.findFirst()
.orElse(null);
}

private static class FixedInputLocationBuilder implements Xpp3DomBuilder.InputLocationBuilder {
private final Object location;

public FixedInputLocationBuilder(Object location) {
this.location = location;
}

public Object toInputLocation(XmlPullParser parser) {
return location;
}
}
}
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ under the License.
<plexusVersion>2.1.0</plexusVersion>
<plexusInterpolationVersion>1.26</plexusInterpolationVersion>
<plexusUtilsVersion>4.0.0-alpha-3-SNAPSHOT</plexusUtilsVersion>
<plexusUtilsVersionEmbedded>3.4.2</plexusUtilsVersionEmbedded>
<plexusUtilsVersionEmbedded>3.5.0</plexusUtilsVersionEmbedded>
<guiceVersion>5.1.0</guiceVersion>
<guavaVersion>30.1-jre</guavaVersion>
<guavafailureaccessVersion>1.0.1</guavafailureaccessVersion>
Expand Down