You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2026/10/19 09:35:28 UTC
[PR] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
arturobernalg opened a new pull request, #2209:
URL: https://github.com/apache/maven-resolver/pull/2209
## What and why
`DefaultArtifactResolver` accepts a `VersionResult` whose version is null or blank, which happens whenever the request's `Artifact` has no version at all (#1269 / MRESOLVER-597). It keeps going, and the failure only surfaces much later as an opaque `IllegalArgumentException` thrown by `DefaultLocalPathComposer` about an invalid relative path, for instance `com/example/no-such-artifact//no-such-artifact-.jar`, with no mention of the real cause.
I reproduced this against the current `maven-resolver-impl` code, both as a unit test and as a standalone program using `maven-resolver-supplier-mvn3`: resolving `com.example:no-such-artifact:jar` with no version throws that unrelated path exception.
The issue suggested either documenting the exception or failing fast when the request is built. Failing fast at construction time is not possible in general, because the input version can be a symbolic one (`RELEASE`, `LATEST`, a range) that only the version resolver can turn into a concrete version, so the earliest point where an empty result is known is right after `versionResolver.resolveVersion` returns.
## How
`DefaultArtifactResolver#resolve` now rejects a null or blank resolved version immediately after it is applied to the artifact, with an `ArtifactNotFoundException` that names the artifact and says its version is missing or blank, instead of letting execution reach the local repository path composer.
## Tests
`DefaultArtifactResolverTest#testResolveArtifactWithBlankVersionFailsWithClearMessage` fails without the change (the stub local repository manager used in this test class does not reproduce the production `IllegalArgumentException`, but the assertion on the clear message fails, confirming no such message was produced before). With the change, this test and the rest of `maven-resolver-impl` pass (589 tests), and `checkstyle:check` is clean.
---
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004 (http://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)
- [x] 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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209#discussion_r4313569729
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -307,6 +307,20 @@ private List<ArtifactResult> resolve(
artifact = artifact.setVersion(versionResult.getVersion());
+ if (artifact.getVersion() == null
+ || artifact.getVersion().trim().isEmpty()) {
Review Comment:
💡 **Nit — dead null check:** `artifact.getVersion() == null` is unreachable after `artifact.setVersion(versionResult.getVersion())`. `AbstractArtifact.setVersion(null)` either short-circuits and returns `this` (when the current version is already `""`) or calls `newInstance(null, ...)` → `DefaultArtifact` constructor → `emptify(null)` → `""`. Either path produces an empty string, never null. Simplify to:
```suggestion
if (artifact.getVersion().trim().isEmpty()) {
```
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -307,6 +307,20 @@ private List<ArtifactResult> resolve(
artifact = artifact.setVersion(versionResult.getVersion());
+ if (artifact.getVersion() == null
+ || artifact.getVersion().trim().isEmpty()) {
Review Comment:
💡 **Nit — dead null check:** `artifact.getVersion() == null` is unreachable after `artifact.setVersion(versionResult.getVersion())`. `AbstractArtifact.setVersion(null)` either short-circuits and returns `this` (when the current version is already `""`) or calls `newInstance(null, ...)` → `DefaultArtifact` constructor → `emptify(null)` → `""`. Either path produces an empty string, never null. Simplify to:
```suggestion
if (artifact.getVersion().trim().isEmpty()) {
```
--
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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209#discussion_r4312538598
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -307,6 +307,19 @@ private List<ArtifactResult> resolve(
artifact = artifact.setVersion(versionResult.getVersion());
+ if (artifact.getVersion() == null || artifact.getVersion().isEmpty()) {
Review Comment:
The `== null` check is dead code: `DefaultArtifact` normalizes `null` to `""` via `emptify()` in all constructors, and `AbstractArtifact.setVersion()` handles the `null` case by returning `this` unchanged (version stays `""`). `getVersion()` never returns `null`.
Also, `isEmpty()` misses whitespace-only versions (e.g. `" "`) that would still produce an invalid path in the local path composer. Use `trim().isEmpty()` for completeness (Java 8-compatible).
```suggestion
if (artifact.getVersion().trim().isEmpty()) {
```
--
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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209#issuecomment-6266534837
Pushed the dead-branch fix (see inline reply/thread above). On the backport note: agreed this affects maven-resolver-1.9.x too, but I'll hold off opening that PR until after this one lands on master, to keep them separate and avoid rebasing a backport against a moving target.
--
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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209#discussion_r4313621998
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -307,6 +307,20 @@ private List<ArtifactResult> resolve(
artifact = artifact.setVersion(versionResult.getVersion());
+ if (artifact.getVersion() == null
+ || artifact.getVersion().trim().isEmpty()) {
Review Comment:
You're right, and I was wrong to push back on this. `Artifact.getVersion()` is documented as `@return The version, never {@code null}` and the interface itself is `@noimplement` (clients are only supposed to extend `AbstractArtifact`), so the `== null` branch really is unreachable per this module's own API contract, not just for the concrete classes used here. Simplified to just `artifact.getVersion().trim().isEmpty()`, pushed. Thanks for catching it twice.
--
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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209#discussion_r4313458321
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java:
##########
@@ -307,6 +307,19 @@ private List<ArtifactResult> resolve(
artifact = artifact.setVersion(versionResult.getVersion());
+ if (artifact.getVersion() == null || artifact.getVersion().isEmpty()) {
Review Comment:
Good catch on the whitespace-only case, added `trim()` for that.
I kept the `== null` guard, though: `Artifact` is a public interface third-party code can implement directly without going through `AbstractArtifact`/`DefaultArtifact`, so `getVersion()` returning `null` isn't impossible from the type system's point of view, even though it can't happen with this module's own classes. Removing the check would trade a clear `ArtifactNotFoundException` for an uncaught `NullPointerException` in that case, which is a regression for the exact problem this PR is fixing. Applied as `artifact.getVersion() == null || artifact.getVersion().trim().isEmpty()`, with a new test for the whitespace-only case (fails without the `trim()`, passes with it).
--
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] Fix #1269: Report a clear message when resolving an artifact with no … [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #2209:
URL: https://github.com/apache/maven-resolver/pull/2209
--
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