Copilot commented on code in PR #508:
URL:
https://github.com/apache/maven-build-cache-extension/pull/508#discussion_r3608841822
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -838,15 +838,12 @@ private DigestItem resolveArtifact(final Dependency
dependency)
return DtoUtils.createDigestedFile(artifact, hash);
}
- // Handle special dependency types that have implicit classifiers
- String classifier = dependency.getClassifier();
- String extension = null;
-
- // test-jar type requires "tests" classifier and "jar" extension
- if ("test-jar".equals(dependency.getType()) && (classifier == null ||
classifier.isEmpty())) {
- classifier = "tests";
- extension = "jar";
- }
+ ArtifactHandler handler =
artifactHandlerManager.getArtifactHandler(dependency.getType());
+ String classifier = dependency.getClassifier() == null
+ || dependency.getClassifier().trim().isEmpty()
+ ? handler.getClassifier()
+ : dependency.getClassifier();
+ String extension = handler.getExtension();
Review Comment:
The new classifier/extension resolution now relies on the `ArtifactHandler`
(e.g., to supply implicit classifiers like `tests` for `test-jar`), but the
current tests only cover plain `jar` dependencies. Please add a unit test that
stubs `artifactHandlerManager.getArtifactHandler("test-jar")` to return a
handler with classifier `tests` and extension `jar`, and assert that the
created `ArtifactRequest` resolves an artifact with classifier `tests` when
`Dependency.classifier` is blank.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -838,15 +838,12 @@ private DigestItem resolveArtifact(final Dependency
dependency)
return DtoUtils.createDigestedFile(artifact, hash);
}
- // Handle special dependency types that have implicit classifiers
- String classifier = dependency.getClassifier();
- String extension = null;
-
- // test-jar type requires "tests" classifier and "jar" extension
- if ("test-jar".equals(dependency.getType()) && (classifier == null ||
classifier.isEmpty())) {
- classifier = "tests";
- extension = "jar";
- }
+ ArtifactHandler handler =
artifactHandlerManager.getArtifactHandler(dependency.getType());
+ String classifier = dependency.getClassifier() == null
+ || dependency.getClassifier().trim().isEmpty()
+ ? handler.getClassifier()
+ : dependency.getClassifier();
+ String extension = handler.getExtension();
Review Comment:
`artifactHandlerManager.getArtifactHandler(dependency.getType())` can return
null (this codebase already guards against that in
`CacheControllerImpl.restoredArtifact()`), but here `handler.getClassifier()` /
`handler.getExtension()` would then throw a NullPointerException during
snapshot dependency resolution. Consider falling back to a default handler
(e.g., `jar`) when no handler is registered, and also reuse the trimmed
classifier value since you already treat whitespace-only classifiers as empty.
--
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]
