You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "efegokdemir (via GitHub)" <gi...@apache.org> on 2026/10/08 13:31:56 UTC
[PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
efegokdemir opened a new pull request, #2186:
URL: https://github.com/apache/maven-resolver/pull/2186
## What
Include the per-repository failures for unresolved artifacts in `ArtifactResolutionException` messages. The primary cause and existing message format remain unchanged when there is only one failure.
## Why
When artifact resolution fails for different reasons across repositories, the exception message currently reports only one cause. For example, a missing artifact in Central can be hidden by a transfer error from another repository, leaving users without the information needed to distinguish a bad coordinate from an unavailable repository.
## How
Keep the current primary cause selection, then append other recorded failures with the artifact and repository ID. Sort those additional details so the message is stable despite the concurrent map used to store repository failures.
Fixes #1857
## Testing
- Fail-before: the regression test failed on the unmodified implementation because the exception message contained the transfer failure but omitted Central's not-found reason.
- Pass-after: `mvn -pl maven-resolver-api,maven-resolver-impl -am -Dspotless.apply.skip=true -Dspotless.check.skip=true test` — 583 tests passed, 5 skipped.
- `mvn -pl maven-resolver-api spotless:check` — passed.
- `git diff --check` — passed.
The reactor tests ran with Java 25; Spotless was verified separately with Java 17 because the configured formatter is incompatible with Java 25.
## Compatibility
Single-failure messages are unchanged and covered by a regression assertion. Successful resolution behavior is unchanged.
AI assistance was used for analysis and implementation; the patch and tests were reviewed before submission.
---
To make clear that you license this contribution under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0), check one of the following:
- [x] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
- [ ] I hereby declare that I have filed an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#discussion_r4219701536
##########
maven-resolver-api/src/test/java/org/eclipse/aether/RepositoryExceptionTest.java:
##########
@@ -111,6 +114,39 @@ void testArtifactResolutionException_Serializable() {
assertSerializable(new ArtifactResolutionException(Arrays.asList(result)));
}
+ @Test
+ void testArtifactResolutionExceptionMessageIncludesFailuresFromAllRepositories() {
+ RemoteRepository central =
+ new RemoteRepository.Builder("central", "default", "https://repo.maven.apache.org/maven2").build();
+ RemoteRepository corporate = new RemoteRepository.Builder(
+ "corporate", "default", "https://repo.internal.example/repository/maven")
+ .build();
+ Artifact artifact = newArtifact();
+ ArtifactRequest request = new ArtifactRequest()
+ .setArtifact(artifact)
+ .addRepository(central)
+ .addRepository(corporate);
+ ArtifactResult result = new ArtifactResult(request);
+ result.addException(central, new ArtifactNotFoundException(artifact, central, "not found in central"));
+ result.addException(corporate, new ArtifactTransferException(artifact, corporate, "DNS lookup failed"));
+
+ String message = new ArtifactResolutionException(Collections.singletonList(result)).getMessage();
+
+ assertTrue(message.contains("not found in central"), message);
+ assertTrue(message.contains("DNS lookup failed"), message);
Review Comment:
💡 **Suggestion:** `assertTrue(message.contains(...))` doesn't verify the full message format or confirm that the primary cause isn't duplicated in the additional-failures section. Consider asserting the exact message string instead — it documents the contract precisely and will catch unintended formatting changes:
```suggestion
assertTrue(message.contains("not found in central"), message);
assertTrue(message.contains("DNS lookup failed"), message);
// Stronger: verify exact format and no duplication
// assertEquals("The following artifacts could not be resolved: gid:aid:ext:1: not found in central " +
// "(additional failures: gid:aid:ext:1 from corporate: DNS lookup failed)", message);
```
Using `assertEquals` on the full expected string makes the test self-documenting and acts as a stronger non-duplication guard.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactResolutionException.java:
##########
@@ -133,9 +133,37 @@ private static String getSmartMessage(List<? extends ArtifactResult> results) {
buffer.append(": ").append(cause.getMessage());
}
+ String additionalFailures = getAdditionalFailures(results, cause);
+ if (!additionalFailures.isEmpty()) {
+ buffer.append(" (additional failures: ").append(additionalFailures).append(")");
+ }
+
return buffer.toString();
}
+ private static String getAdditionalFailures(List<? extends ArtifactResult> results, Throwable primaryCause) {
+ ArrayList<String> failures = new ArrayList<>();
Review Comment:
💡 **Nit:** Prefer `List<String>` over `ArrayList<String>` — program to the interface rather than the implementation. Since `Collections.sort` accepts any `List`, this works as-is:
```suggestion
List<String> failures = new ArrayList<>();
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactResolutionException.java:
##########
@@ -133,9 +133,37 @@ private static String getSmartMessage(List<? extends ArtifactResult> results) {
buffer.append(": ").append(cause.getMessage());
}
+ String additionalFailures = getAdditionalFailures(results, cause);
+ if (!additionalFailures.isEmpty()) {
+ buffer.append(" (additional failures: ").append(additionalFailures).append(")");
+ }
+
return buffer.toString();
}
+ private static String getAdditionalFailures(List<? extends ArtifactResult> results, Throwable primaryCause) {
+ ArrayList<String> failures = new ArrayList<>();
Review Comment:
💡 **Nit:** Prefer `List<String>` over `ArrayList<String>` — program to the interface rather than the implementation. Since `Collections.sort` accepts any `List`, this works as-is:
```suggestion
List<String> failures = new ArrayList<>();
```
##########
maven-resolver-api/src/test/java/org/eclipse/aether/RepositoryExceptionTest.java:
##########
@@ -111,6 +114,39 @@ void testArtifactResolutionException_Serializable() {
assertSerializable(new ArtifactResolutionException(Arrays.asList(result)));
}
+ @Test
+ void testArtifactResolutionExceptionMessageIncludesFailuresFromAllRepositories() {
+ RemoteRepository central =
+ new RemoteRepository.Builder("central", "default", "https://repo.maven.apache.org/maven2").build();
+ RemoteRepository corporate = new RemoteRepository.Builder(
+ "corporate", "default", "https://repo.internal.example/repository/maven")
+ .build();
+ Artifact artifact = newArtifact();
+ ArtifactRequest request = new ArtifactRequest()
+ .setArtifact(artifact)
+ .addRepository(central)
+ .addRepository(corporate);
+ ArtifactResult result = new ArtifactResult(request);
+ result.addException(central, new ArtifactNotFoundException(artifact, central, "not found in central"));
+ result.addException(corporate, new ArtifactTransferException(artifact, corporate, "DNS lookup failed"));
+
+ String message = new ArtifactResolutionException(Collections.singletonList(result)).getMessage();
+
+ assertTrue(message.contains("not found in central"), message);
+ assertTrue(message.contains("DNS lookup failed"), message);
Review Comment:
💡 **Suggestion:** `assertTrue(message.contains(...))` doesn't verify the full message format or confirm that the primary cause isn't duplicated in the additional-failures section. Consider asserting the exact message string instead — it documents the contract precisely and will catch unintended formatting changes:
```suggestion
assertTrue(message.contains("not found in central"), message);
assertTrue(message.contains("DNS lookup failed"), message);
// Stronger: verify exact format and no duplication
// assertEquals("The following artifacts could not be resolved: gid:aid:ext:1: not found in central " +
// "(additional failures: gid:aid:ext:1 from corporate: DNS lookup failed)", message);
```
Using `assertEquals` on the full expected string makes the test self-documenting and acts as a stronger non-duplication guard.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#discussion_r4219875668
##########
maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactResolutionException.java:
##########
@@ -133,9 +133,37 @@ private static String getSmartMessage(List<? extends ArtifactResult> results) {
buffer.append(": ").append(cause.getMessage());
}
+ String additionalFailures = getAdditionalFailures(results, cause);
+ if (!additionalFailures.isEmpty()) {
+ buffer.append(" (additional failures: ").append(additionalFailures).append(")");
+ }
+
return buffer.toString();
}
+ private static String getAdditionalFailures(List<? extends ArtifactResult> results, Throwable primaryCause) {
+ List<String> failures = new ArrayList<>();
Review Comment:
💡 **Nit (carried over):** Prefer `List<String>` over `ArrayList<String>` — program to the interface rather than the implementation. Since `Collections.sort` accepts any `List`, this works as-is:
```suggestion
List<String> failures = new ArrayList<>();
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#issuecomment-6067189963
> Nit: I would tell you _exactly the opposite_ as @gnodet told, given we talk about _local variable_; my pet peeve is to stick closest to the type as possible :smile: No changes or anything needed, just an observation.
Use 'var' ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#issuecomment-6063641397
Nit: I would tell you _exactly the opposite_ as @gnodet told, given we talk about _local variable_; my pet peeve is to stick closes to the type as possible :smile: No changes or anything needed, just an observation.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-1857: Include all repository failures in artifact messages [maven-resolver]
Posted by "efegokdemir (via GitHub)" <gi...@apache.org>.
efegokdemir commented on PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#issuecomment-6061399429
Addressed both suggestions in `74618f5`: the accumulator is typed to `List<String>`, and the regression now asserts the complete formatted message, including the primary cause exactly once and the additional repository failure. `mvn -pl maven-resolver-api -Dtest=RepositoryExceptionTest -Dspotless.apply.skip=true -Dspotless.check.skip=true test` passed (17 tests), as did `git diff --check`. `spotless:check` could not complete because the configured Palantir formatter throws `NoSuchFieldError` under the available JDK 27 while processing an unrelated existing test file.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org