[charangowdamd-cmd]

Summary

Decouple ModelProcessor from XML-specific types by introducing a format-agnostic read(Source, Map) method.

Fixes #13707

Motivation

ModelProcessor is intended to provide a format-independent entry point for reading Maven models, delegating parsing to registered ModelParser SPI implementations before falling back to XML parsing.

Previously, its API exposed XmlReaderRequest and XML-specific exceptions, requiring callers to construct XML-specific requests even when working with generic Source instances. Parser failures were also wrapped in IOException, making exception handling inconsistent with the ModelParser API.

This change aligns ModelProcessor with the ModelParser API and prepares it for the model dialect system (#13613).

Changes Made

• Updated ModelProcessor API

  • Introduced read(Source, Map) as a format-agnostic entry point.
  • Deprecated read(XmlReaderRequest) while retaining backward compatibility through delegation to the new method.
  • Added exception handling to preserve the underlying parsing and I/O failure information.

• Updated DefaultModelProcessor

  • Implemented parsing through registered ModelParser SPI implementations.
  • Kept XML parsing as the fallback, constructing XmlReaderRequest internally.
  • Converted XML parsing and I/O exceptions into ModelParserException, preserving line and column information where available.
  • Consolidated parser failures into ModelParserException with suppressed exceptions.

• Updated model processing callers

  • Modified DefaultSuperPomProvider and DefaultModelBuilder to pass Source instances and parser options directly.
  • Removed unnecessary XML request construction and InterningTransformer setup from DefaultModelBuilder.
  • Updated exception handling to distinguish I/O failures from parsing errors.

• Updated tests

  • Updated DefaultModelProcessorTest to test the new API.
  • Added coverage for exception aggregation and suppressed exceptions.
  • Verified delegation from the deprecated read(XmlReaderRequest) method.

Verification

• mvn spotless:apply
• mvn checkstyle:check — no violations
• Module compilation for api/maven-api-spi and impl/maven-impl

Compatibility

The existing read(XmlReaderRequest) method remains available as a deprecated method, allowing existing callers to transition to the new API.

License

• I hereby declare this contribution to be licensed under the [Apache License, Version 2.0](https://www.apache.org/licenses/LICENSE-2.0).
• I hereby declare that I have filed an [Apache Individual Contributor License Agreement (ICLA)](https://www.apache.org/licenses/icla.pdf).

[gnodet-bot]

This PR makes good progress on decoupling ModelProcessor from XML-specific types, but it introduces several correctness and design issues that need to be addressed.

Critical: InterningTransformer regression

The new DefaultModelProcessor.doRead() uses new DefaultModelBuilder.InterningTransformer() (no-arg constructor), which always uses DEFAULT_CONTEXTS. The old code used new InterningTransformer(session), which reads MAVEN_MODEL_BUILDER_INTERNS from user/system properties to let users customize which XML contexts to intern. This silently drops a user-facing configuration knob without replacement.

Critical: modelId lost in the toSource() fast path

In the SPI bridge ModelProcessor.toSource(), the fast path returns Sources.fromPath(request.getPath()), which creates a plain PathSource (not ModelSource). PathSource has no getModelId(). Downstream, DefaultModelProcessor.doRead() only sets modelId when source instanceof ModelSource — so callers that set request.modelId(...) silently get it dropped on the fast path.

Design: XML-specific code in the SPI interface

The ModelProcessor SPI now contains ~80 lines of implementation in the deprecated read(XmlReaderRequest) default method, including toSource() with ByteArrayInputStream, XmlReaderRequest, XmlReaderException, etc. SPI interfaces should not contain implementation logic or import XML-specific types. This code belongs in an abstract class or a utility in the impl module.

Design: InterningTransformer accessed via inner class cross-reference

DefaultModelProcessor references DefaultModelBuilder.InterningTransformer — a package-private inner class of an unrelated class. This creates tight coupling that will break if either class moves. InterningTransformer should be extracted to a dedicated top-level class in the impl package.

Robustness: error handling via cause-chain introspection

The deprecated read(XmlReaderRequest) bridge re-throws by walking e.getCause() chains to find XmlReaderException or IOException. This is fragile: any wrapping by an intermediate layer will hide the original exception, silently converting it to the fallback XmlReaderException(e.getMessage(), null, e) — losing location information. The old separate catch (XmlReaderException) / catch (IOException) blocks were explicit and predictable.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ Regression: InterningTransformer() (no-arg) always uses DEFAULT_CONTEXTS. The old code passed the session so users could configure which contexts to intern via the maven.model.builder.interns property (MAVEN_MODEL_BUILDER_INTERNS). That customization is now silently lost.

This needs either:

• The Session to be available here (requires threading it through), or
• Passing options that include the session-derived contexts, or
• At minimum, a @deprecated note on the configuration knob explaining it no longer works.

[gnodet-bot]

⚠️ Correctness: Sources.fromPath() returns a PathSource, not a ModelSource. Callers that set request.modelId(...) will silently lose the modelId: DefaultModelProcessor.doRead() only propagates modelId when source instanceof ModelSource, which PathSource is not.

Use Sources.resolvedSource(request.getPath(), request.getModelId()) instead, or check request.getModelId() != null before taking this fast path.

[gnodet-bot]

⚠️ SPI design: The SPI interface now imports ByteArrayInputStream, XmlReaderException, XmlReaderRequest, ModelParser, ModelParserException, Sources, ModelSource, etc., and contains ~80 lines of XML-specific implementation. SPI interfaces should be minimal contracts with no implementation logic.

The deprecated bridge method and toSource() should live in an abstract base class (e.g. AbstractModelProcessor) or a package-private utility class in maven-impl, not in the SPI.

[gnodet-bot]

💡 Design: DefaultModelBuilder.InterningTransformer is a package-private inner class of DefaultModelBuilder. Cross-referencing it from DefaultModelProcessor creates tight coupling: moving either class would break compilation. Extract InterningTransformer to a dedicated package-private top-level class (e.g. ModelInterningTransformer) in org.apache.maven.impl.model.

[gnodet-bot]

This PR makes good progress on the refactoring goals, addressing most issues from the previous review. Three issues remain.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

Following up on my earlier review — the first two issues (transformer drop and SPI Javadoc) are now fully addressed. One new design concern was introduced in this iteration.

Previous findings:

• ✅ Deprecated bridge drops transformer — fixed by passing transformer in options map
• ✅ SPI Javadoc references implementation classes — Javadoc correctly updated to abstract description
• ✅ InterningTransformer extracted to ModelInterningTransformer top-level class
• ✅ UrlModelSource record extracted from the anonymous inline implementation

New issue: DefaultSuperPomProvider now imports DefaultModelProcessor (impl→impl coupling)

See inline comment below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet-bot]

⚠️ [MEDIUM] Impl-to-impl coupling via DefaultModelProcessor.urlModelSource()

DefaultSuperPomProvider previously interacted with ModelProcessor exclusively through the SPI interface. This commit introduces a direct import of DefaultModelProcessor (a concrete implementation class) solely to call DefaultModelProcessor.urlModelSource(url, modelId).

This creates tight coupling between two implementation classes that should remain independent. If DefaultModelProcessor is refactored, renamed, or the urlModelSource factory is moved, DefaultSuperPomProvider breaks silently.

The fix is to move urlModelSource() — or an equivalent ModelSource factory for URL + modelId — to the existing Sources utility class in maven-api-core. Sources already provides fromPath(), buildSource(), and resolvedSource(); a URL variant fits naturally there and would eliminate this coupling:

// In Sources.java (maven-api-core)
@Nonnull
public static ModelSource urlSource(@Nonnull URL url, @Nullable String modelId) {
    return new UrlModelSource(url, modelId);
}

DefaultSuperPomProvider could then use Sources.urlSource(url, modelId) without importing any impl class. The UrlModelSource record itself can live in Sources (package-private) or be made package-private in the same module.