gnodet-bot commented on code in PR #13722:
URL: https://github.com/apache/maven/pull/13722#discussion_r4389615130
##########
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, {@code DefaultModelProcessor} uses this transformer for
XML parsing instead
+ * of the default {@code ModelInterningTransformer}.
+ *
+ * @since 4.0.0
+ */
+ String TRANSFORMER = "transformer";
Review Comment:
💡 **[MEDIUM] SPI Javadoc references implementation classes**
The Javadoc for the `TRANSFORMER` constant names two implementation classes:
> When present, `DefaultModelProcessor` uses this transformer … instead of
the default `ModelInterningTransformer`.
Both `DefaultModelProcessor` and `ModelInterningTransformer` are internal
implementation details (`impl/maven-impl`), not part of the SPI contract. SPI
Javadoc should describe behavior in terms of the abstraction only. Referencing
concrete impl classes tightly couples the contract to the current
implementation, makes future refactoring harder, and may mislead third-party
`ModelProcessor` implementors into thinking they must replicate this behavior.
Suggested rewrite:
```suggestion
/**
* 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
*/
String TRANSFORMER = "transformer";
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java:
##########
@@ -114,21 +118,112 @@ 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, Map.of(ModelParser.STRICT,
request.isStrict()));
Review Comment:
⚠️ **[HIGH] Deprecated bridge drops transformer from `XmlReaderRequest`**
The deprecated `read(XmlReaderRequest)` bridge at line 150 builds its
options map with only `ModelParser.STRICT`:
```java
return read(source, Map.of(ModelParser.STRICT, request.isStrict()));
```
Any transformer set via `request.getTransformer()` is silently dropped.
Callers using the deprecated method with a custom transformer will silently get
the default `ModelInterningTransformer` instead — a behavioral regression with
no warning.
Fix: include the transformer in the delegating call:
```suggestion
return read(source, request.getTransformer() != null
? Map.of(ModelParser.STRICT, request.isStrict(),
ModelProcessor.TRANSFORMER, request.getTransformer())
: Map.of(ModelParser.STRICT, request.isStrict()));
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSuperPomProvider.java:
##########
@@ -61,16 +65,42 @@ private Model readModel(String version, String v) {
throw new IllegalStateException("The super POM " + resource + "
was not found"
+ ", please verify the integrity of your Maven
installation");
}
- try (InputStream is = url.openStream()) {
- String modelId =
- "org.apache.maven:maven-api-impl:" +
getImplementationVersion(getClass()) + ":super-pom-" + version;
- return modelProcessor.read(XmlReaderRequest.builder()
- .modelId(modelId)
- .location(url.toExternalForm())
- .inputStream(is)
- .strict(false)
- .build());
- } catch (IOException e) {
+ String modelId =
+ "org.apache.maven:maven-api-impl:" +
getImplementationVersion(getClass()) + ":super-pom-" + version;
+ ModelSource source = new ModelSource() {
+ @Override
+ public Path getPath() {
+ return null;
+ }
+
+ @Override
+ public InputStream openStream() throws IOException {
+ return url.openStream();
+ }
+
+ @Override
+ public String getLocation() {
+ return url.toExternalForm();
+ }
+
+ @Override
+ public Source resolve(String relative) {
+ return null;
+ }
+
+ @Override
+ public ModelSource resolve(ModelLocator modelLocator, String
relative) {
Review Comment:
🔧 **[LOW] Anonymous `ModelSource` duplicates boilerplate from
`DefaultModelProcessor.toSource()`**
This anonymous `ModelSource` implementation (lines 70–96) is ~25 lines of
structural boilerplate that mirrors the `toSource()` factory in
`DefaultModelProcessor`. Consider extracting a small package-private
`UrlModelSource` record or static factory to reduce duplication and make the
pattern reusable:
```java
// e.g. in a shared utility or within DefaultModelProcessor
static ModelSource urlModelSource(URL url, String modelId) { ... }
```
This isn't blocking, but consolidating now while the refactoring is fresh
avoids accumulating duplicate implementations.
--
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]
