org.apache.maven.io.util.WriterUtils#replaceXpp3DOM may fail on some specific case

[keshin]

Hi team,
MetadataJDOMWriter would call org.apache.maven.io.util.WriterUtils#replaceXpp3DOM on writing plugin/configuration, since sub elements of configuration accept attributes namedcombine.children and combine.self to control how Maven combine the configurations from parent, a use case like below may output wrong result as WriterUtils only compare the name of elements.

I think the we also should compare the attribute name and value at https://github.com/Commonjava/maven-model-jdom-support/blob/master/src/main/java/org/apache/maven/io/util/WriterUtils.java#L137, how do you think about it? I can send a pull request for it if you'd like to.

Test pom:

<?xml version="1.0" encoding="UTF-8" ?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
  <modelVersion>4.0.0</modelVersion>
  <groupId>test.group</groupId>
  <artifactId>test-name</artifactId>
  <version>1</version>

  <build>
    <plugins>
      <plugin>
        <artifactId>plugin-1</artifactId>
        <version>1</version>
        <configuration>
          <properties combine.children="append">
            <property>1</property>
          </properties>
        </configuration>
      </plugin>
      <plugin>
        <artifactId>plugin-2</artifactId>
        <version>1</version>
        <configuration>
          <properties combine.children="override">
            <property>2</property>
          </properties>
        </configuration>
      </plugin>
    </plugins>
  </build>
</project>

Test case:

    @Test
    public void removePlugin() throws Exception {
        InputStream in = Thread.currentThread().getContextClassLoader().getResourceAsStream("test.pom");
        File file = temp.newFile();
        FileUtils.copyInputStreamToFile(in, file);

        in  = new FileInputStream(file);
        Model model = new MavenXpp3Reader().read(in);
        in.close();

        // remove first
        model.getBuild().getPlugins().remove(0);

        new MavenJDOMWriter(model).write(model, file);

        System.out.println(FileUtils.readFileToString(file));

        // build the model after change
        in  = new FileInputStream(file);
        model = new MavenXpp3Reader().read(in);
        in.close();
        Plugin plugin = model.getBuild().getPlugins().get(0);
        assertEquals("plugin-2", plugin.getArtifactId());
        Xpp3Dom config = (Xpp3Dom) plugin.getConfiguration();
        Xpp3Dom properties = config.getChild("properties");
// WOULD FAIL HERE
        assertEquals("override", properties.getAttribute("combine.children"));
    }

[rnc]

@jdcasey What do you think on this?

Pull requests would be great, thanks.

[jdcasey]

Hmm, so it looks like we need to consider whether self.combine or children.combine is set before we do blind replacement. It would take a bit of research to fix this, mainly just trying to go back and remember the values for those attributes, and what they mean. I'm sure they're in some obscure maven wiki page or something.

@keshin I know it's been awhile, but if you have the ability to send a PR for this that would be best. That way you get full credit for the work (in the git log). Also, you're probably better versed in the edge cases right now, since you've been looking at this problem specifically.

Regardless, I agree it's something we need to address.