Skip to content

fix: decouple ModelProcessor from XML-specific types (#13707) #13722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charangowdamd-cmd wants to merge 1 commit into
base: master
Choose a base branch
from
+536 −250
Open

fix: decouple ModelProcessor from XML-specific types (#13707) #13722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions api/maven-api-spi/src/main/java/org/apache/maven/api/services/model/ModelProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@

import java.io.IOException;
import java.nio.file.Path;
import java.util.Map;

import org.apache.maven.api.annotations.Nonnull;
import org.apache.maven.api.annotations.Nullable;
import org.apache.maven.api.model.Model;
import org.apache.maven.api.services.Source;
import org.apache.maven.api.services.xml.XmlReaderException;
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.spi.ModelParserException;

/**
* Locates and reads POM files from the filesystem.
Expand All @@ -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
*/
String TRANSFORMER = "transformer";
Comment thread
gnodet marked this conversation as resolved.

/**
* Returns the file containing the pom to be parsed or null if a pom can not be found
* at the given file or in the given directory.
Expand All @@ -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;

@gnodet-bot gnodet-bot Oct 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.


/**
* 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")
Model read(XmlReaderRequest request) throws IOException, XmlReaderException;
}
23 changes: 10 additions & 13 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSuperPomProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
*/
package org.apache.maven.impl;

import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.util.Map;
import java.util.Objects;
Expand All @@ -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;

@gnodet-bot gnodet-bot Oct 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [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.


@Named
@Singleton
Expand Down Expand Up @@ -61,16 +62,12 @@ 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 = DefaultModelProcessor.urlModelSource(url, modelId);
try {
return modelProcessor.read(source, Map.of(ModelParser.STRICT, false));
} catch (ModelParserException e) {
throw new IllegalStateException(
"The super POM " + resource + " is damaged"
+ ", please verify the integrity of your Maven installation",
Expand Down
172 changes: 40 additions & 132 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -114,7 +112,7 @@
import org.apache.maven.api.services.model.ProfileSelector;
import org.apache.maven.api.services.model.RootLocator;
import org.apache.maven.api.services.xml.XmlReaderException;
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.api.spi.ModelTransformer;
import org.apache.maven.impl.DefaultRemoteRepository;
Expand Down Expand Up @@ -1843,31 +1841,27 @@ Model doReadFileModel(Set<Path> activeModelReads) throws ModelBuilderException {
rootDirectory = rootDirectory.getParent();
}
}
try (InputStream is = modelSource.openStream()) {
model = modelProcessor.read(XmlReaderRequest.builder()
.strict(strict)
.location(modelSource.getLocation())
.modelId(modelSource.getModelId())
.path(modelSource.getPath())
.rootDirectory(rootDirectory)
.inputStream(is)
.transformer(new InterningTransformer(session))
.build());
} catch (XmlReaderException e) {
try {
model = modelProcessor.read(
modelSource,
Map.of(
ModelParser.STRICT,
strict,
ModelProcessor.TRANSFORMER,
new ModelInterningTransformer(session)));
} catch (ModelParserException e) {
if (!strict) {
throw e;
}
try (InputStream is = modelSource.openStream()) {
model = modelProcessor.read(XmlReaderRequest.builder()
.strict(false)
.location(modelSource.getLocation())
.modelId(modelSource.getModelId())
.path(modelSource.getPath())
.rootDirectory(rootDirectory)
.inputStream(is)
.transformer(new InterningTransformer(session))
.build());
} catch (XmlReaderException ne) {
try {
model = modelProcessor.read(
modelSource,
Map.of(
ModelParser.STRICT,
false,
ModelProcessor.TRANSFORMER,
new ModelInterningTransformer(session)));
} catch (ModelParserException ne) {
// still unreadable even in non-strict mode, rethrow original error
throw e;
}
Expand All @@ -1878,24 +1872,29 @@ Model doReadFileModel(Set<Path> activeModelReads) throws ModelBuilderException {
"Malformed POM " + modelSource.getLocation() + ": " + e.getMessage(),
e);
}
} catch (XmlReaderException e) {
add(
Severity.FATAL,
Version.BASE,
"Non-parseable POM " + modelSource.getLocation() + ": " + e.getMessage(),
e);
throw newModelBuilderException();
} catch (IOException e) {
String msg = e.getMessage();
if (msg == null || msg.isEmpty()) {
// NOTE: There's java.nio.charset.MalformedInputException and sun.io.MalformedInputException
if (e.getClass().getName().endsWith("MalformedInputException")) {
msg = "Some input bytes do not match the file encoding.";
} else {
msg = e.getClass().getSimpleName();
} catch (ModelParserException e) {
if (e.getCause() instanceof IOException ioe) {
String msg = ioe.getMessage();
if (msg == null || msg.isEmpty()) {
// NOTE: There's java.nio.charset.MalformedInputException and sun.io.MalformedInputException
if (ioe.getClass().getName().endsWith("MalformedInputException")) {
msg = "Some input bytes do not match the file encoding.";
} else {
msg = ioe.getClass().getSimpleName();
}
}
add(
Severity.FATAL,
Version.BASE,
"Non-readable POM " + modelSource.getLocation() + ": " + msg,
ioe);
} else {
add(
Severity.FATAL,
Version.BASE,
"Non-parseable POM " + modelSource.getLocation() + ": " + e.getMessage(),
e);
}
add(Severity.FATAL, Version.BASE, "Non-readable POM " + modelSource.getLocation() + ": " + msg, e);
throw newModelBuilderException();
}

Expand Down Expand Up @@ -3341,97 +3340,6 @@ public R getRequest() {
}
}

static class InterningTransformer implements XmlReaderRequest.Transformer {
static final Set<String> DEFAULT_CONTEXTS = Set.of(
// Core Maven coordinates
"groupId",
"artifactId",
"version",
"namespaceUri",
"packaging",

// Dependency-related fields
"scope",
"type",
"classifier",

// Build and plugin-related fields
"phase",
"goal",
"execution",

// Repository-related fields
"layout",
"policy",
"checksumPolicy",
"updatePolicy",

// Common metadata fields
"modelVersion",
"name",
"url",
"system",
"distribution",
"status",

// SCM fields
"connection",
"developerConnection",
"tag",

// Common enum-like values that appear frequently
"id",
"inherited",
"optional");

private final Set<String> contexts;

/**
* Creates an InterningTransformer with default contexts.
*/
InterningTransformer() {
this.contexts = DEFAULT_CONTEXTS;
}

/**
* Creates an InterningTransformer with contexts from session properties.
*
* @param session the Maven session to read properties from
*/
InterningTransformer(Session session) {
this.contexts = parseContextsFromSession(session);
}

private Set<String> parseContextsFromSession(Session session) {
String contextsProperty = session.getUserProperties().get(Constants.MAVEN_MODEL_BUILDER_INTERNS);
if (contextsProperty == null) {
contextsProperty = session.getSystemProperties().get(Constants.MAVEN_MODEL_BUILDER_INTERNS);
}

if (contextsProperty == null || contextsProperty.trim().isEmpty()) {
return DEFAULT_CONTEXTS;
}

return Arrays.stream(contextsProperty.split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.collect(Collectors.toSet());
}

@Override
public String transform(String input, String context) {
return input != null && contexts.contains(context) ? input.intern() : input;
}

/**
* Get the contexts that will be interned by this transformer.
* Used for testing purposes.
*/
Set<String> getContexts() {
return contexts;
}
}

/**
* Clears REQUEST_SCOPED cache entries for a specific request.
* <p>
Expand Down
Loading