gnodet commented on code in PR #13722:
URL: https://github.com/apache/maven/pull/13722#discussion_r4436028054
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java:
##########
@@ -143,8 +146,36 @@ private Path doLocateExistingPom(Path project) {
}
}
- private Model doRead(XmlReaderRequest request) throws IOException {
- return modelXmlFactory.read(request);
+ private Model doRead(Source source, Map<String, ?> options) throws
ModelParserException {
+ boolean strict = isStrict(options);
+ try (InputStream is = source.openStream()) {
+ XmlReaderRequest.XmlReaderRequestBuilder builder =
XmlReaderRequest.builder()
+ .strict(strict)
+ .location(source.getLocation())
+ .path(source.getPath())
+ .inputStream(is)
+ .transformer(new
DefaultModelBuilder.InterningTransformer());
Review Comment:
Fixed: `DefaultModelBuilder` now passes `new
ModelInterningTransformer(session)` via the `ModelProcessor.TRANSFORMER` option
key at every `modelProcessor.read()` call site (lines 1845–1862).
`DefaultModelProcessor.getTransformer()` picks it up from options first; the
no-arg fallback only applies to callers that don't have a session context (e.g.
the super POM provider, where session-specific intern config is irrelevant).
The regression is resolved — session-configured contexts are preserved on all
normal build reads.
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -42,13 +53,118 @@ public interface ModelProcessor {
Path locateExistingPom(@Nonnull Path project);
/**
- * Reads the model from the specified byte stream. The stream will be
automatically closed before the method
- * returns.
+ * Reads the model from the specified source.
+ *
+ * @param source The source to deserialize the model from, must not be
{@code null}.
+ * @param options Possible reading options, may be {@code null}.
+ * @return The deserialized model, never {@code null}.
+ * @throws ModelParserException If the model could not be deserialized.
+ * @since 4.0.0
+ */
+ @Nonnull
+ Model read(@Nonnull Source source, @Nullable Map<String, ?> options)
throws ModelParserException;
+
+ /**
+ * Reads the model from the specified reader request.
*
* @param request The reader request to deserialize the model, must not be
{@code null}.
* @return The deserialized model, never {@code null}.
* @throws IOException If the model could not be deserialized.
* @throws XmlReaderException If the input format could not be parsed.
+ * @deprecated Use {@link #read(Source, Map)} instead.
*/
- Model read(XmlReaderRequest request) throws IOException,
XmlReaderException;
+ @Deprecated(since = "4.0.0")
+ default Model read(XmlReaderRequest request) throws IOException,
XmlReaderException {
+ Objects.requireNonNull(request, "request cannot be null");
+ try {
+ return read(toSource(request), Map.of(ModelParser.STRICT,
request.isStrict()));
+ } catch (ModelParserException e) {
+ Throwable cause = e.getCause();
+ while (cause != null) {
+ if (cause instanceof XmlReaderException xre) {
+ throw xre;
+ }
+ if (cause instanceof IOException ioe) {
+ throw ioe;
+ }
+ cause = cause.getCause();
+ }
+ for (Throwable s : e.getSuppressed()) {
+ if (s instanceof XmlReaderException xre) {
+ throw xre;
+ }
+ if (s instanceof IOException ioe) {
+ throw ioe;
+ }
+ }
+ throw new XmlReaderException(e.getMessage(), null, e);
+ }
+ }
+
+ private static Source toSource(XmlReaderRequest request) {
+ if (request.getPath() != null
+ && request.getInputStream() == null
+ && request.getReader() == null
+ && request.getURL() == null) {
+ return Sources.fromPath(request.getPath());
Review Comment:
Fixed: `toSource()` in `DefaultModelProcessor` now always returns a full
`ModelSource` anonymous implementation that preserves `modelId` via
`getModelId()`. The fast-path `Sources.fromPath()` (which returned a plain
`PathSource` without `modelId`) is no longer used in this code path. `doRead()`
then correctly propagates `modelId` via `builder.modelId(ms.getModelId())` when
`source instanceof ModelSource`. No `modelId` loss can occur.
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -42,13 +53,118 @@ public interface ModelProcessor {
Path locateExistingPom(@Nonnull Path project);
/**
- * Reads the model from the specified byte stream. The stream will be
automatically closed before the method
- * returns.
+ * Reads the model from the specified source.
+ *
+ * @param source The source to deserialize the model from, must not be
{@code null}.
+ * @param options Possible reading options, may be {@code null}.
+ * @return The deserialized model, never {@code null}.
+ * @throws ModelParserException If the model could not be deserialized.
+ * @since 4.0.0
+ */
+ @Nonnull
+ Model read(@Nonnull Source source, @Nullable Map<String, ?> options)
throws ModelParserException;
Review Comment:
The implementation logic (the old `toSource()` helper and `~80 lines`) has
already been moved to `DefaultModelProcessor`. What remains in the SPI
interface is only the deprecated *signature* `Model read(XmlReaderRequest)
throws IOException, XmlReaderException` — no implementation code.
This signature cannot be removed from the interface without a binary break:
existing `ModelProcessor` implementors may have overridden it.
`XmlReaderRequest` and `XmlReaderException` are both in `maven-api-core`, which
is a declared compile dependency of `maven-api-spi`, so there is no
module-boundary violation — only a design purity concern. The two remaining
imports are the minimum required to express the deprecated contract. The
`TRANSFORMER` constant is a plain `String` with no runtime dependency on XML
types.
Leaving the signature in the SPI is the correct backward-compatible approach
here.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java:
##########
@@ -143,8 +146,36 @@ private Path doLocateExistingPom(Path project) {
}
}
- private Model doRead(XmlReaderRequest request) throws IOException {
- return modelXmlFactory.read(request);
+ private Model doRead(Source source, Map<String, ?> options) throws
ModelParserException {
+ boolean strict = isStrict(options);
+ try (InputStream is = source.openStream()) {
+ XmlReaderRequest.XmlReaderRequestBuilder builder =
XmlReaderRequest.builder()
+ .strict(strict)
+ .location(source.getLocation())
+ .path(source.getPath())
+ .inputStream(is)
+ .transformer(new
DefaultModelBuilder.InterningTransformer());
Review Comment:
Fixed: `DefaultModelBuilder.InterningTransformer` (the old package-private
inner class) has been extracted to `ModelInterningTransformer`, a dedicated
package-private top-level class in `org.apache.maven.impl.model`.
`DefaultModelProcessor` now references `ModelInterningTransformer` directly
with no cross-class inner-type coupling.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSuperPomProvider.java:
##########
@@ -29,9 +27,12 @@
import org.apache.maven.api.di.Named;
import org.apache.maven.api.di.Singleton;
import org.apache.maven.api.model.Model;
+import org.apache.maven.api.services.ModelSource;
import org.apache.maven.api.services.SuperPomProvider;
import org.apache.maven.api.services.model.ModelProcessor;
-import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.api.spi.ModelParser;
+import org.apache.maven.api.spi.ModelParserException;
+import org.apache.maven.impl.model.DefaultModelProcessor;
Review Comment:
Fixed: `Sources.urlSource(URL url, String modelId)` has been added to the
`Sources` utility class in `maven-api-core`, backed by the package-private
`UrlModelSource` record. `DefaultSuperPomProvider` now calls
`Sources.urlSource(url, modelId)` and no longer imports `DefaultModelProcessor`
at all. The impl-to-impl coupling is fully removed.
--
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]
