gnodet-bot commented on code in PR #13159:
URL: https://github.com/apache/maven/pull/13159#discussion_r4036738561
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultNode.java:
##########
@@ -72,8 +74,17 @@ public List<RemoteRepository> getRemoteRepositories() {
@Override
public Optional<RemoteRepository> getRepository() {
- // TODO: v4: implement
- throw new UnsupportedOperationException("Not implemented yet");
+ org.eclipse.aether.artifact.Artifact artifact = node.getArtifact();
+ List<org.eclipse.aether.repository.RemoteRepository> repos =
node.getRepositories();
+ if (artifact == null || repos.isEmpty()) {
+ return Optional.empty();
+ }
+ LocalArtifactRequest request = new LocalArtifactRequest(artifact,
repos, node.getRequestContext());
+ LocalArtifactResult result =
+
session.getSession().getLocalRepositoryManager().find(session.getSession(),
request);
Review Comment:
💡 **`SimpleLocalRepositoryManager` always returns empty here**
`SimpleLocalRepositoryManager.find()` (the legacy LRM, still used when
`<localRepositoryLayout>simple</localRepositoryLayout>` is configured) never
calls `result.setRepository(...)` — it only sets `available`. So on a project
using SimpleLRM, this method always returns `Optional.empty()` regardless of
whether the artifact was actually downloaded from a remote repo. The method
still behaves correctly for `EnhancedLocalRepositoryManager` (the default),
which does populate `result.repository` from `.repositories` tracking files.
The Javadoc should acknowledge this:
```suggestion
* Returns the remote repository from which this artifact was
downloaded, if known.
*
* @return an {@code Optional} containing the repository, or empty if
not available
* (e.g. local artifact, root node, or when the local repository
manager
* does not track artifact origins such as {@code
SimpleLocalRepositoryManager})
```
The `@return` lives in `Node.java` — update the Javadoc there.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultNode.java:
##########
@@ -72,8 +74,17 @@ public List<RemoteRepository> getRemoteRepositories() {
@Override
public Optional<RemoteRepository> getRepository() {
- // TODO: v4: implement
- throw new UnsupportedOperationException("Not implemented yet");
+ org.eclipse.aether.artifact.Artifact artifact = node.getArtifact();
+ List<org.eclipse.aether.repository.RemoteRepository> repos =
node.getRepositories();
+ if (artifact == null || repos.isEmpty()) {
+ return Optional.empty();
+ }
+ LocalArtifactRequest request = new LocalArtifactRequest(artifact,
repos, node.getRequestContext());
+ LocalArtifactResult result =
+
session.getSession().getLocalRepositoryManager().find(session.getSession(),
request);
Review Comment:
âš¡ **Uncached disk I/O on every call**
`find()` reads a `.repositories` tracking file from disk on each invocation.
`getRepository()` has no memoization, so a caller that accesses it multiple
times on the same node (e.g. iterating the dependency graph and logging) hits
disk repeatedly. For a resolved dependency tree with hundreds of nodes, this is
N disk reads per call site that iterates the tree.
If the resolver's `DependencyNode` already carries `requestContext` and
`repositories` — all the information needed to compute the result — consider
lazy-initializing with a cached field:
```java
private Optional<RemoteRepository> cachedRepository;
@Override
public Optional<RemoteRepository> getRepository() {
if (cachedRepository == null) {
org.eclipse.aether.artifact.Artifact artifact = node.getArtifact();
List<org.eclipse.aether.repository.RemoteRepository> repos =
node.getRepositories();
if (artifact == null || repos.isEmpty()) {
cachedRepository = Optional.empty();
} else {
LocalArtifactRequest request = new
LocalArtifactRequest(artifact, repos, node.getRequestContext());
LocalArtifactResult result =
session.getSession().getLocalRepositoryManager().find(session.getSession(),
request);
cachedRepository = Optional.ofNullable(result)
.map(LocalArtifactResult::getRepository)
.map(session::getRemoteRepository);
}
}
return cachedRepository;
}
```
This is safe because `DefaultNode` is not reused across sessions and
`DependencyNode` is immutable post-resolution.
--
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]
