gnodet-bot commented on code in PR #13159:
URL: https://github.com/apache/maven/pull/13159#discussion_r4029621545
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultNode.java:
##########
@@ -72,8 +74,15 @@ 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:
⚠️ **Potential NPE if `find()` returns `null`**
`LocalRepositoryManager.find()` is not annotated `@Nonnull` in the resolver
API contract — a third-party LRM implementation that returns `null` will cause
an NPE on `result.getRepository()`. The standard `SimpleLocalRepositoryManager`
always creates a `new LocalArtifactResult(request)`, so this is safe for
Maven's own LRM, but the code is fragile against custom implementations.
Add a null guard:
```suggestion
LocalArtifactResult result =
session.getSession().getLocalRepositoryManager().find(session.getSession(),
request);
if (result == null) {
return Optional.empty();
}
```
Or alternatively use `Optional.ofNullable(result).map(r ->
r.getRepository()).map(session::getRemoteRepository)` as a one-liner.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultNodeTest.java:
##########
@@ -66,4 +80,91 @@ void testAsString() {
defaultNode = new DefaultNode(session, node, true);
assertEquals("(org.example:myapp:jar:1.0:compile - omitted for
conflict with 2.0)", defaultNode.asString());
}
+
+ @Test
+ void testGetRepositoryReturnsPresentWhenFoundInLocalRepo() {
+ // Set up a dependency node with an artifact and one candidate remote
repository
+ org.eclipse.aether.repository.RemoteRepository aetherRepo =
+ new org.eclipse.aether.repository.RemoteRepository.Builder(
+ "central", "default",
"https://repo1.maven.org/maven2";)
+ .build();
+ DefaultArtifact artifact = new
DefaultArtifact("org.example:myapp:1.0");
+ DefaultDependencyNode node = new DefaultDependencyNode(artifact);
+ node.setRepositories(Collections.singletonList(aetherRepo));
+
+ // LocalArtifactResult reports the artifact was fetched from aetherRepo
+ LocalArtifactResult localResult = mock(LocalArtifactResult.class);
+ when(localResult.getRepository()).thenReturn(aetherRepo);
+
+ LocalRepositoryManager lrm = mock(LocalRepositoryManager.class);
+ when(lrm.find(any(RepositorySystemSession.class),
any(LocalArtifactRequest.class)))
+ .thenReturn(localResult);
Review Comment:
🔍 **Loose argument matcher doesn't validate request content**
`any(LocalArtifactRequest.class)` accepts any request, so the test doesn't
verify that `getRepository()` actually passes the correct artifact,
repositories, and request context to the LRM. If someone changes the
implementation to pass the wrong artifact or omit the repos list, this test
would still pass.
Consider capturing and asserting on the `LocalArtifactRequest` argument:
```java
ArgumentCaptor<LocalArtifactRequest> captor =
ArgumentCaptor.forClass(LocalArtifactRequest.class);
verify(lrm).find(any(RepositorySystemSession.class), captor.capture());
LocalArtifactRequest capturedRequest = captor.getValue();
assertEquals(artifact, capturedRequest.getArtifact());
assertEquals(Collections.singletonList(aetherRepo),
capturedRequest.getRepositories());
```
Same applies to the `testGetRepositoryReturnsEmptyWhenNotInLocalRepo` test
at line ~134.
--
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]
