gnodet commented on code in PR #11742:
URL: https://github.com/apache/maven/pull/11742#discussion_r3284339265
##########
compat/maven-model/src/main/java/org/apache/maven/model/io/xpp3/MavenXpp3Writer.java:
##########
@@ -94,9 +103,28 @@ public void write(Writer writer, Model model) throws
IOException {
*/
public void write(OutputStream stream, Model model) throws IOException {
try {
+ configureDelegate(model);
delegate.write(stream, model.getDelegate());
} catch (XMLStreamException e) {
throw new IOException(e);
}
} // -- void write( OutputStream, Model )
+
+ private void configureDelegate(Model model) {
+ String version = model.getModelVersion();
+ if (version != null) {
+ version = version.trim();
+ }
+ if (version == null || version.isBlank()) {
+ String namespaceUri = model.getDelegate().getNamespaceUri();
+ if (namespaceUri != null &&
namespaceUri.startsWith(NAMESPACE_PREFIX)) {
+ version =
namespaceUri.substring(NAMESPACE_PREFIX.length()).trim();
+ }
+ }
+ if (version == null || version.isBlank()) {
+ version = DEFAULT_MODEL_VERSION;
+ }
Review Comment:
The version extraction from the namespace URI is a nice defensive fallback,
but the constants here duplicate values already defined in `TransformerSupport`
(`NAMESPACE_FORMAT`, `SCHEMA_LOCATION_FORMAT`). Consider extracting them to a
shared location to avoid drift.
##########
api/maven-api-xml/src/main/java/org/apache/maven/api/xml/XmlService.java:
##########
@@ -239,8 +239,15 @@ private static XmlService getService() {
/** Holder class for lazy initialization of the default instance */
private static final class Holder {
- static final XmlService INSTANCE = ServiceLoader.load(XmlService.class)
+ static final XmlService INSTANCE =
ServiceLoader.load(XmlService.class, XmlService.class.getClassLoader())
.findFirst()
+ .or(() -> {
+ ClassLoader contextClassLoader =
Thread.currentThread().getContextClassLoader();
+ return contextClassLoader != null
+ ? ServiceLoader.load(XmlService.class,
contextClassLoader)
+ .findFirst()
Review Comment:
This classloader fallback is a worthwhile robustness improvement, but it
addresses a different problem than #11715 (namespace preservation). Consider
splitting this into a separate commit/PR so the two fixes can be tracked and
reverted independently.
Also, the order here (class classloader first, then TCCL) inverts the
typical Java convention where TCCL is checked first. Is this intentional? In
most ServiceLoader patterns the TCCL takes precedence because it represents the
application's view. If the class's own classloader already sees the provider,
the TCCL fallback will never trigger in practice -- which is fine for
robustness, but worth documenting the intent.
--
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]
