[MNG-8490] Add a Builder to XmlNode and provide an XmlService

[jira-importer]

Elliotte Rusty Harold opened MNG-8490 and commented

Align XmlNode with the rest of the maven 4 API by using an immutable class and a builder.

---

(was: Single use interfaces are an antipattern. Make everything simpler by having a single concrete XmlNode class.)

---

Affects: API

Remote Links:

• GitHub Pull Request #2029
  
• GitHub Pull Request #2150
  

[jira-importer]

Yu Yu commented

I will try to finish this task in the future, could you please assign it to me.

[jira-importer]

Elliotte Rusty Harold commented

Thanks for your interest. In the past, assigning issues to new developers tends to not result in a mergeable PR. Just send a PR, assign me as reviewer, and I'll review it. Until we have a PR in hand though this issue is up for grabs.

[jira-importer]

Yu Yu commented

After attempting to combine the code, I realized that the API layer has a dependency on the XmlNode model. If I merge it into XmlNode and place the parsing implementation in the API layer, it seems inappropriate. I feel that they shouldn't be combined like this？

[jira-importer]

Elliotte Rusty Harold commented

I think you can just turn XmlNode into a concrete class and merge the impl class into that. Does that not work?

Also, a comment suggests mnoving thr static utility code elsewhere. That might be a good first step.

[jira-importer]

Yu Yu commented

I understand, I'll submit a PR later.Thank you.

[jira-importer]

Guillaume Nodet commented

I'm -1 on this.
The role of an API is to decouple the contract from the implementation.
All interfaces in Maven have almost a single implementation, except a couple of interfaces.
The Single Interface Single Implementation is only an anti-pattern in a closed world, but the API is designed to be consumed by all plugins, extensions, etc...

[jira-importer]

Elliotte Rusty Harold commented

Yes, the role of an API is to decouple the contract from the implementation. A class has an API. That is not the role of a Java interface, and confusing the two is a source of excessive complexity in Java programs. Java interfaces should only be used when both of these conditions are true:

1. There are multiple implementations.
2. The implementations have different superclasses

Otherwise you end up with needless duplication and an inability to enforce constraints or common behavior across the API since any method can do anything within the scope of the signature.

In this case, and many others in Maven, neither of these conditions is true, and the extra interface serves no useful purpose. It's not just needless complexity. It removes important tools for preventing bugs in code.

[jira-importer]

Guillaume Nodet commented

Using interfaces allows decoupling (not only abstracting) between the contract and the implementation. This is the very basic and first thing to do when modularizing applications.

Anyway, a few points need to be fixed to continue. See #2029 (review)

[jira-importer]

Elliotte Rusty Harold commented

Using interfaces does not enable decoupling between the contract and the implementation, and is not required to decouple contract and implementation. That is the distinction between non-private class and method signatures and the actual code. A concrete class has a published API and an implementation, and the implementation can be changed without touching the public API or modifying the contract in any way. Most other object oriented languages like C++ don't even have the concept of an interface like the Java keyword.

[jira-importer]

Yu Yu commented

Due to an accidental operation, I deleted the repository, which caused the PR to be closed. I am currently unable to restore it. I sincerely apologize for this. I will do my best to recover the code for this PR, but for a few points, I currently don’t have a clear idea of how to implement them. I may need some time to think about the best approach.Once again, I sincerely apologize.

[jira-importer]

Guillaume Nodet commented

Reopen if you want to continue the discussion.

[jira-importer]

Guillaume Nodet commented

So here are a few arguments:

• decoupling is usually done through abstraction. Even if C++ has no concept of interface, it can defines pure abstract classes and support multiple inheritance.  Interfaces were seen as an improvement over this pattern a few decades ago IIRC.
• testing by using mocking requires abstraction, it's much more difficult to achieve without that
• your argument about looking at the code and checking if there are multiple implementations only works in a closed environment, where you actually know all implementations. Maven can be extended through custom syntaxes, but plugin configuration is represented by {}XmlNode{}.  If we were to support yaml, I can easily think about an XmlNode implementation backed by a yaml node that has been returned by a parser.  So imho, this argument does not hold.  People may want to provide their own implementation for unforeseen reasons, and I prefer adding an abstraction now that can be used later, rather than not adding it and having no way to extend when the time comes.

Anyway, if you propose a PR which fixes the various problems raised in the other one, we can continue the discussion:

A few points need to be addressed:

• no third party dependency in the API
• no dependency on plexus-xml, this would re-create a cyclic dependency
• use an interface + builder api, similar to other data classes in the API
• the merge code needs to be abstracted and decoupled using a service interface so that it can be used from plugins (they are only provided with the API jars)

For the merge service, maybe using a factory using ServiceLoader if we want to make that generally available. However, if we only target plugins, a service accessible from the Session could actually be used.
But we do have plugins that use Xpp3Dom.mergeXpp3Dom so that feature cannot be removed from the API.

[jira-importer]

Elliotte Rusty Harold commented

On the mocking point specifically, that hasn't been true for a very long time. Mockito, EasyMock, etc., can all mock concrete and even final classes without any trouble. Using interfaces is absolutely not required. I vaguely recall this being a problem circa 2005 and maybe Java 1.2, but it really no longer is an issue.

[jira-importer]

Guillaume Nodet commented

I think it would make sense to use the same mechanism as we have for model objects.
Currently, there's no way to create the XmlNode objects from the API and this is clearly missing.
So having a Builder + private implementation would make sense.

[jira-importer]

Mirko Friedenhagen commented

FTR: 3ba8f2d leads to an error in mojohaus/extra-enforcer-rules#327. I did a bisect and this core change did break one rule.