Copilot commented on code in PR #133:
URL: https://github.com/apache/maven-doap-plugin/pull/133#discussion_r2616437248
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -162,6 +170,9 @@ public class DoapMojo extends AbstractMojo {
@Parameter(defaultValue = "${project}", readonly = true, required = true)
private MavenProject project;
Review Comment:
The newly added repositorySystemSession parameter is missing Javadoc
documentation. Similar parameters in this class (like the 'project' parameter
above) include a comment block describing the parameter's purpose. Adding
documentation would improve code maintainability and help developers understand
this parameter's role in the Mojo.
```suggestion
/**
* The current repository system session, which is used for artifact
resolution
* and other repository-related operations during the execution of this
Mojo.
*/
```
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -1381,6 +1392,42 @@ private void writeReleases(XMLWriter writer,
MavenProject project) throws MojoEx
}
}
+ private boolean isArtifactInRepository(Artifact artifact,
ArtifactRepository repository) {
+ // Convert Legacy Artifact to Aether Artifact
+ String artifactCoordinates = String.format(
+ "%s:%s:%s:%s",
+ artifact.getGroupId(), artifact.getArtifactId(),
artifact.getType(), artifact.getVersion());
+
+ org.eclipse.aether.artifact.Artifact aetherArtifact =
+ new
org.eclipse.aether.artifact.DefaultArtifact(artifactCoordinates);
+
+ // Convert Legacy ArtifactRepository to Aether RemoteRepository
+ // TODO: do we need to handle authentication/proxies here?
+ RemoteRepository aetherRemoteRepository = new RemoteRepository.Builder(
+ repository.getId(), repository.getLayout().getId(),
repository.getUrl())
+ .build();
+
+ ArtifactRequest artifactRequest = new ArtifactRequest();
+ artifactRequest.setArtifact(aetherArtifact);
+
+ // Limit resolution to ONLY the target remote repository
+
artifactRequest.setRepositories(Collections.singletonList(aetherRemoteRepository));
+
+ try {
+ // Attempt to resolve the artifact
+ ArtifactResult result =
repositorySystem.resolveArtifact(repositorySystemSession, artifactRequest);
+
+ if (result.isResolved()) {
+ return true;
+ }
+
+ } catch (org.eclipse.aether.resolution.ArtifactResolutionException ex)
{
+ return false;
+ }
+
+ return false;
+ }
+
/**
* Write all DOAP repositories.
*
Review Comment:
The return statement inside the try block at line 1426 makes the return
statement at line 1433 redundant. Consider simplifying the logic by either
removing the explicit check at line 1425 and directly returning the result of
result.isResolved() before the catch block, or restructuring to avoid the
unreachable return false at line 1433. The current structure suggests that
result.isResolved() could be false, but in practice, if resolveArtifact
succeeds without throwing an exception, the artifact should be resolved.
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -162,6 +170,9 @@ public class DoapMojo extends AbstractMojo {
@Parameter(defaultValue = "${project}", readonly = true, required = true)
private MavenProject project;
+ @Parameter(defaultValue = "${repositorySystemSession}", readonly = true)
+ private RepositorySystemSession repositorySystemSession;
+
Review Comment:
This line contains trailing whitespace. Remove the trailing whitespace to
maintain code cleanliness and consistency with project coding standards.
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -1381,6 +1392,42 @@ private void writeReleases(XMLWriter writer,
MavenProject project) throws MojoEx
}
}
Review Comment:
The new private method isArtifactInRepository lacks Javadoc documentation.
Given that this method performs a critical function (checking artifact
availability in a repository using Maven Resolver), it should include
documentation explaining its purpose, parameters, return value, and potentially
the resolution strategy being used. This is especially important since the
method replaces the previous HTTP-based checking mechanism.
```suggestion
/**
* Checks whether the specified Maven artifact is available in the given
remote repository
* using the Maven Resolver (Aether).
* <p>
* This method converts the legacy Maven {@link Artifact} and {@link
ArtifactRepository}
* to their Aether equivalents and attempts to resolve the artifact
using only the specified
* remote repository. If the artifact can be resolved, it is considered
present.
*
* @param artifact the Maven artifact to check for availability
* @param repository the remote repository to check for the artifact
* @return {@code true} if the artifact is found in the repository,
{@code false} otherwise
*/
```
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -144,6 +149,9 @@ public class DoapMojo extends AbstractMojo {
@Inject
private RepositoryMetadataManager repositoryMetadataManager;
Review Comment:
The newly injected RepositorySystem component is missing Javadoc
documentation. For consistency with the other injected components in this class
(like repositoryMetadataManager above it), it should include a comment
describing its purpose and a @since tag. This would make the codebase more
maintainable and help other developers understand the component's role.
```suggestion
/**
* The repository system used for artifact resolution.
*
* @since 1.2
*/
```
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -144,6 +149,9 @@ public class DoapMojo extends AbstractMojo {
@Inject
private RepositoryMetadataManager repositoryMetadataManager;
+ @Inject
+ private RepositorySystem repositorySystem;
+
Review Comment:
This line contains trailing whitespace. Remove the trailing whitespace to
maintain code cleanliness and consistency with project coding standards.
##########
src/main/java/org/apache/maven/plugin/doap/DoapMojo.java:
##########
@@ -1381,6 +1392,42 @@ private void writeReleases(XMLWriter writer,
MavenProject project) throws MojoEx
}
}
+ private boolean isArtifactInRepository(Artifact artifact,
ArtifactRepository repository) {
+ // Convert Legacy Artifact to Aether Artifact
+ String artifactCoordinates = String.format(
+ "%s:%s:%s:%s",
+ artifact.getGroupId(), artifact.getArtifactId(),
artifact.getType(), artifact.getVersion());
+
+ org.eclipse.aether.artifact.Artifact aetherArtifact =
+ new
org.eclipse.aether.artifact.DefaultArtifact(artifactCoordinates);
+
+ // Convert Legacy ArtifactRepository to Aether RemoteRepository
+ // TODO: do we need to handle authentication/proxies here?
+ RemoteRepository aetherRemoteRepository = new RemoteRepository.Builder(
+ repository.getId(), repository.getLayout().getId(),
repository.getUrl())
+ .build();
Review Comment:
The TODO comment about handling authentication/proxies should be addressed
before merging. The old code using DoapUtil.fetchURL likely handled these
concerns through Maven's settings. Since you're now using Maven Resolver
directly, you should either implement proper authentication/proxy handling or
document why it's not needed. The repositorySystemSession parameter likely
already contains this configuration, but it should be verified that the
RemoteRepository.Builder properly inherits these settings or explicitly
configures them.
```suggestion
// Handle authentication/proxies using RepositorySystemSession
selectors
RemoteRepository.Builder repoBuilder = new RemoteRepository.Builder(
repository.getId(), repository.getLayout().getId(),
repository.getUrl());
// Apply authentication if available
if (repositorySystemSession != null) {
org.eclipse.aether.repository.Authentication authentication =
repositorySystemSession.getAuthenticationSelector().getAuthentication(repoBuilder.build());
if (authentication != null) {
repoBuilder.setAuthentication(authentication);
}
org.eclipse.aether.repository.Proxy proxy =
repositorySystemSession.getProxySelector().getProxy(repoBuilder.build());
if (proxy != null) {
repoBuilder.setProxy(proxy);
}
}
RemoteRepository aetherRemoteRepository = repoBuilder.build();
```
--
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]
