gnodet-bot commented on code in PR #13722:
URL: https://github.com/apache/maven/pull/13722#discussion_r4376983584
##########
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:
💡 **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`.
##########
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:
⚠️ **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.
##########
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:
⚠️ **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.
##########
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:
⚠️ **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.
--
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]
