gnodet-bot commented on code in PR #13722:
URL: https://github.com/apache/maven/pull/13722#discussion_r4436110227
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -34,6 +37,15 @@
*/
public interface ModelProcessor {
+ /**
+ * Option key for passing an {@link XmlReaderRequest.Transformer} through
the options map.
+ * When present, the processor should use this transformer for XML parsing
instead
+ * of any default transformer it would otherwise apply.
+ *
+ * @since 4.0.0
Review Comment:
💡 **`@since` version is wrong:** `TRANSFORMER` is being added in
`4.1.0-SNAPSHOT`; the correct tag is `@since 4.1.0`. Maven 4.0.0 is already
released and this constant doesn't exist there.
```suggestion
* @since 4.1.0
```
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -42,13 +54,26 @@ 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
Review Comment:
💡 **`@since` version is wrong:** This new method is being added on
`4.1.0-SNAPSHOT`; the correct tag is `@since 4.1.0`.
```suggestion
* @since 4.1.0
```
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java:
##########
@@ -42,13 +54,26 @@ 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.
*/
+ @Deprecated(since = "4.0.0")
Review Comment:
💡 **`@Deprecated(since = "4.0.0")` is wrong:** The `read(XmlReaderRequest)`
method existed without deprecation in 4.0.0 (confirmed via `git log`). It is
being deprecated in 4.1.0. The correct annotation is `@Deprecated(since =
"4.1.0")`.
```suggestion
@Deprecated(since = "4.1.0")
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java:
##########
@@ -114,21 +119,126 @@ public Model read(XmlReaderRequest request) throws
IOException {
}
}
try {
- return doRead(request);
- } catch (IOException | XmlReaderException e) {
+ return doRead(source, options);
+ } catch (ModelParserException e) {
if (!exceptions.isEmpty()) {
- IOException ioException = new
IOException(buildDetailedErrorMessage(pomFile, exceptions, e));
- exceptions.values().forEach(ioException::addSuppressed);
- ioException.addSuppressed(e);
- throw ioException;
+ ModelParserException mpe =
+ new
ModelParserException(buildDetailedErrorMessage(pomFile, exceptions, e), e);
+ exceptions.values().forEach(mpe::addSuppressed);
+ mpe.addSuppressed(e);
+ throw mpe;
}
throw e;
}
} else {
- return doRead(request);
+ return doRead(source, options);
}
}
+ /**
+ * Implements the deprecated bridge for backward compatibility.
+ * Delegates to {@link #read(Source, Map)}, then re-throws {@link
XmlReaderException}
+ * or {@link IOException} from the cause when possible.
+ *
+ * @deprecated Use {@link #read(Source, Map)} instead.
+ */
+ @Deprecated(since = "4.0.0")
+ @Override
+ public Model read(XmlReaderRequest request) throws IOException,
XmlReaderException {
+ Objects.requireNonNull(request, "request cannot be null");
+ Source source = toSource(request);
+ try {
+ return read(
+ source,
+ request.getTransformer() != null
+ ? Map.of(
+ ModelParser.STRICT,
+ request.isStrict(),
+ ModelProcessor.TRANSFORMER,
+ request.getTransformer())
+ : Map.of(ModelParser.STRICT, request.isStrict()));
+ } catch (ModelParserException e) {
+ // Unwrap one level: doRead wraps XmlReaderException/IOException
as the direct cause
+ if (e.getCause() instanceof XmlReaderException xre) {
+ throw xre;
+ }
+ if (e.getCause() instanceof IOException ioe) {
+ throw ioe;
+ }
+ throw new XmlReaderException(e.getMessage(), null, e);
+ }
+ }
+
+ private static Source toSource(XmlReaderRequest request) {
+ if (request.getInputStream() == null
+ && request.getPath() == null
+ && request.getReader() == null
+ && request.getURL() != null) {
+ return Sources.urlSource(request.getURL(), request.getModelId());
+ }
+ // Always return a ModelSource so modelId is preserved.
+ return new ModelSource() {
+ @Override
+ public Path getPath() {
+ return request.getPath();
+ }
+
+ @Override
+ public InputStream openStream() throws IOException {
+ if (request.getInputStream() != null) {
+ return request.getInputStream();
+ } else if (request.getURL() != null) {
+ return request.getURL().openStream();
+ } else if (request.getPath() != null) {
+ return Files.newInputStream(request.getPath());
+ } else if (request.getReader() != null) {
+ StringBuilder sb = new StringBuilder();
+ char[] buf = new char[4096];
+ int n;
+ while ((n = request.getReader().read(buf)) != -1) {
+ sb.append(buf, 0, n);
+ }
+ return new
ByteArrayInputStream(sb.toString().getBytes(StandardCharsets.UTF_8));
Review Comment:
⚠️ **Latent encoding bug:** When `XmlReaderRequest` is created with a
`Reader` (already decoded chars), this branch reads the reader to a `String`
and re-encodes as UTF-8. If the original XML had a non-UTF-8 encoding
declaration (e.g. `<?xml version="1.0" encoding="ISO-8859-1"?>`), the
re-encoded bytes are UTF-8 but the XML declaration still says ISO-8859-1 — any
non-ASCII characters will be garbled when the downstream XML parser re-reads
the stream by the declared encoding.
In practice, Maven POMs are UTF-8 or ASCII, so this is unlikely to bite in
the wild. However, the Reader code path in `XmlReaderRequest` is part of the
public API contract. Consider adding a comment here, or re-encoding as
`StandardCharsets.UTF_8` and stripping/rewriting the encoding declaration in
the byte stream.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/Sources.java:
##########
@@ -94,6 +95,92 @@ public static ModelSource resolvedSource(@Nonnull Path path,
@Nullable String mo
return new ResolvedPathSource(requireNonNull(path, "path"),
path.toString(), modelId);
}
+ /**
+ * Creates a new source backed by a URL, with an optional model identifier.
+ * This is used for classpath-based sources such as the Maven super POM.
+ * The source does not support resolving related sources.
+ *
+ * @param url the URL to read from, must not be {@code null}
+ * @param modelId the modelId (groupId:artifactId:version coordinates),
may be {@code null}
+ * @return a new ModelSource instance backed by the URL
+ * @throws NullPointerException if url is null
+ * @since 4.0.0
Review Comment:
💡 **`@since` version is wrong:** `urlSource()` is new in this PR which
targets `4.1.0-SNAPSHOT`. The correct tag is `@since 4.1.0`.
```suggestion
* @since 4.1.0
```
--
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]
