Skip to content

test cover DefaultModelProcessor#read #2304

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

Closed
wants to merge 1 commit into from
Closed

test cover DefaultModelProcessor#read #2304

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
81 changes: 43 additions & 38 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
import org.apache.maven.api.spi.ModelParser;
import org.apache.maven.api.spi.ModelParserException;

import static java.util.Objects.requireNonNull;
import static org.apache.maven.api.spi.ModelParser.STRICT;

/**
*
* Note: uses @Typed to limit the types it is available for injection to just ModelProcessor.
Expand Down Expand Up @@ -77,67 +80,69 @@ public DefaultModelProcessor(ModelXmlFactory modelXmlFactory, @Nullable List<Mod
this.modelParsers = modelParsers;
}

/**
* @implNote The ModelProcessor#locatePom never returns null while the ModelParser#locatePom needs to return an existing path!
*/
@Override
public Path locateExistingPom(Path projectDirectory) {
// Note that the ModelProcessor#locatePom never returns null
// while the ModelParser#locatePom needs to return an existing path!
Path pom = modelParsers.stream()
.map(m -> m.locate(projectDirectory)
.map(org.apache.maven.api.services.Source::getPath)
.orElse(null))
.filter(Objects::nonNull)
.findFirst()
.orElseGet(() -> doLocateExistingPom(projectDirectory));
Copy link
Contributor Author

@Pankraz76 Pankraz76 May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

unpleasant do prefix is fix for naming collision @elharo

return throwIfWrongProjectDirLocation(
projectDirectory,
modelParsers.stream()
.map(m -> m.locate(projectDirectory)
.map(org.apache.maven.api.services.Source::getPath)
.orElse(null))
.filter(Objects::nonNull)
.findFirst()
.orElseGet(() -> locateExistingPomWithFallback(projectDirectory)));
}

private static Path throwIfWrongProjectDirLocation(Path projectDirectory, Path pom) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SOC

if (pom != null && !pom.equals(projectDirectory) && !pom.getParent().equals(projectDirectory)) {
throw new IllegalArgumentException("The POM found does not belong to the given directory: " + pom);
}
return pom;
}

private Path locateExistingPomWithFallback(Path project) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SRP

if (project != null) {
if (Files.isDirectory(project)) {
Path pom = project.resolve("pom.xml");
return Files.isRegularFile(pom) ? pom : null;
}
return Files.isRegularFile(project) ? project : null;
}
return locateExistingPomWithFallback(Paths.get(System.getProperty("user.dir"))); // check in user dir
}

@Override
public Model read(XmlReaderRequest request) throws IOException {
Objects.requireNonNull(request, "source cannot be null");
Path pomFile = request.getPath();
Path pomFile = requireNonNull(request, "source cannot be null").getPath();
if (pomFile != null) {
Path projectDirectory = pomFile.getParent();
List<ModelParserException> exceptions = new ArrayList<>();
for (ModelParser parser : modelParsers) {
try {
Optional<Model> model =
parser.locateAndParse(projectDirectory, Map.of(ModelParser.STRICT, request.isStrict()));
if (model.isPresent()) {
return model.get().withPomFile(pomFile);
Optional<Model> parent = parent(request, parser, pomFile.getParent());
if (parent.isPresent()) {
return parent.get().withPomFile(pomFile);
}
} catch (ModelParserException e) {
exceptions.add(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it mandatory to continue if an error occurs to give full error report? Or can we have here early exit if the branch kicks in like above in its counterpart?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it depends on whether one parser throwing an exception invalidates answers from subsequent parsers in the loop. I suspect it doesn't since you skip subsequent parsers once one returns.

}
}
try {
return doRead(request);
} catch (IOException e) {
exceptions.forEach(e::addSuppressed);
throw e;
}
} else {
return doRead(request);
throwErrorsSuppressed(exceptions);
}
return modelXmlFactory.read(request);
}

private Path doLocateExistingPom(Path project) {
if (project == null) {
project = Paths.get(System.getProperty("user.dir"));
}
if (Files.isDirectory(project)) {
Path pom = project.resolve("pom.xml");
return Files.isRegularFile(pom) ? pom : null;
} else if (Files.isRegularFile(project)) {
return project;
} else {
return null;
}
private static Optional<Model> parent(XmlReaderRequest request, ModelParser parser, Path projectDirectory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A parent usually refers to the parent model, so I'd rather use parentDirectory to make it clear what we're looking for.

return parser.locateAndParse(projectDirectory, Map.of(STRICT, request.isStrict()));
}

private Model doRead(XmlReaderRequest request) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is wrong/counterfeit because:

Currently, the API is designed to throw XmlReaderException. Check the implemented interface, there's no IOException thrown here.

#2292 (comment)

return modelXmlFactory.read(request);
private static void throwErrorsSuppressed(List<ModelParserException> exceptions) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're not using the IOException in the API, so not sure why we'd want to build one here. Model parsing exception aren't necessarily IoExceptions.

if (!exceptions.isEmpty()) {
IOException ex = new IOException();
exceptions.forEach(ex::addSuppressed);
throw ex;
}
}
}
Loading