Skip to content
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

Use maven-metadata-local.xml for file:// paths #4512

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Use maven-metadata-local.xml for file:// paths #4512

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
9 changes: 4 additions & 5 deletions rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,23 +256,22 @@ public MavenMetadata downloadMetadata(GroupArtifactVersion gav, @Nullable Resolv
boolean cacheEmptyResult = false;
try {
String scheme = URI.create(repo.getUri()).getScheme();
String uri = repo.getUri() + (repo.getUri().endsWith("/") ? "" : "/") +
String baseUri = repo.getUri() + (repo.getUri().endsWith("/") ? "" : "/") +
requireNonNull(gav.getGroupId()).replace('.', '/') + '/' +
gav.getArtifactId() + '/' +
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit is duplicated at lines 259-261 and 538-540. I wonder if the URI building logic (and possibly more) wouldn’t be better suited in the MavenRepository class itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand & appreciate the suggestion; in general we try to keep the classes under .tree. free of logic; they are intended mostly as a representation of the underlying source, with anything operating on those classes typically extracted elsewhere, like traits or in helper classes like this one. Since we only use this pattern twice (so far) I'd opt to for now keep that bit as-is. But definitely something to keep in mind if it repeats more often or elsewhere, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to mean that it was duplicated 3 times. The third time is actually at lines 356-358.

(gav.getVersion() == null ? "" : gav.getVersion() + '/') +
"maven-metadata.xml";
(gav.getVersion() == null ? "" : gav.getVersion() + '/');

if ("file".equals(scheme)) {
// A maven repository can be expressed as a URI with a file scheme
Path path = Paths.get(URI.create(uri));
Path path = Paths.get(URI.create(baseUri + "maven-metadata-local.xml"));
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory it should be maven-metadata-<repo-id>.xml, and the <localRepository>’s id is local.

I don’t know if it’s very common to have another local repository though. Maybe when people are running CI on the same server as the Maven repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes my main goal here was to have a better chance of matching an existing file, while not yet perfect. I'd say this is better than falling through to deriving that information needlessly further down.

if (Files.exists(path)) {
MavenMetadata parsed = MavenMetadata.parse(Files.readAllBytes(path));
if (parsed != null) {
result = Optional.of(parsed);
}
}
} else {
byte[] responseBody = requestAsAuthenticatedOrAnonymous(repo, uri);
byte[] responseBody = requestAsAuthenticatedOrAnonymous(repo, baseUri + "maven-metadata.xml");
MavenMetadata parsed = MavenMetadata.parse(responseBody);
if (parsed != null) {
result = Optional.of(parsed);
Expand Down
Loading