Skip to content

Fail on classpath resource names that are blank after removing leading slash #4761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fail on classpath resource names that are blank after removing leading slash #4761

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3502b7a
Fail on classpath resource names that are blank after removing leading /
marcphilipp Jul 12, 2025
e123ed6
Protect against potential problems when converting selectors to sources
marcphilipp Jul 12, 2025
665501c
Allow default package for `PackageSource`
marcphilipp Jul 13, 2025
4eb5925
Reduce scope of try-catch block
marcphilipp Jul 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion documentation/src/docs/asciidoc/release-notes/release-notes-5.13.4.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ repository on GitHub.
[[release-notes-5.13.4-junit-platform-bug-fixes]]
==== Bug Fixes

* ❓
* `ClasspathResourceSelector` no longer allows to be constructed with a resource name that
is blank after removing the leading slash.

[[release-notes-5.13.4-junit-platform-deprecations-and-breaking-changes]]
==== Deprecations and Breaking Changes
Expand Down
3 changes: 3 additions & 0 deletions ...m-engine/src/main/java/org/junit/platform/engine/discovery/ClasspathResourceSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.junit.platform.commons.PreconditionViolationException;
import org.junit.platform.commons.function.Try;
import org.junit.platform.commons.support.Resource;
import org.junit.platform.commons.util.Preconditions;
import org.junit.platform.commons.util.ReflectionUtils;
import org.junit.platform.commons.util.StringUtils;
import org.junit.platform.commons.util.ToStringBuilder;
Expand Down Expand Up @@ -64,6 +65,8 @@ public final class ClasspathResourceSelector implements DiscoverySelector {
ClasspathResourceSelector(String classpathResourceName, @Nullable FilePosition position) {
boolean startsWithSlash = classpathResourceName.startsWith("/");
this.classpathResourceName = (startsWithSlash ? classpathResourceName.substring(1) : classpathResourceName);
Preconditions.notBlank(this.classpathResourceName,
"classpath resource name must not be blank after removing leading slash");
this.position = position;
}

Expand Down
14 changes: 12 additions & 2 deletions ...form-launcher/src/main/java/org/junit/platform/launcher/core/DiscoveryIssueCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public void selectorProcessed(UniqueId engineId, DiscoverySelector selector, Sel
if (result.getStatus() == FAILED) {
this.issues.add(DiscoveryIssue.builder(Severity.ERROR, selector + " resolution failed") //
Copy link
Contributor

@mpkorstanje mpkorstanje Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a implicit toString call here to a 3rd party implementation which could throw. So the same caution that prompted toSourceSafely, should probably put a try-catch around it.

And I only just now realized that DiscoverySelector.toString() should have been implemented in Cucumber. 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if a toString() method throws an exception, I'm happy to let the entire discovery fail for that engine... 🤭

.cause(result.getThrowable()) //
.source(toSource(selector)) //
.source(toSourceSafely(selector)) //
.build());
}
else if (result.getStatus() == UNRESOLVED && selector instanceof UniqueIdSelector uniqueIdSelector) {
Expand All @@ -78,7 +78,17 @@ else if (result.getStatus() == UNRESOLVED && selector instanceof UniqueIdSelecto
}
}

static @Nullable TestSource toSource(DiscoverySelector selector) {
private static @Nullable TestSource toSourceSafely(DiscoverySelector selector) {
try {
return toSource(selector);
}
catch (Exception e) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary but we should do our best to avoid throwing an exception while reporting a discovery issue.

Copy link
Contributor

@mpkorstanje mpkorstanje Jul 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is both too much and not enough.

A broad try-catch would make sense if JUnit would be invoking third-party implementations. But the current the toSource method only supports a select number of selector implementations, all owned by JUnit. If the preconditions for those selectors align with those of the test sources, this could never happen. There is not even a test you could write to make this fail.

At the same time, any third-parties can implement custom selectors, but they'll have to do their own issue reporting.

Would it make sense to add DiscoverySelector.toTestSource()? This would allow JUnit to test the toSource implementation on its own selectors and would allow third-party selectors to also provide a source.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A broad try-catch would make sense if JUnit would be invoking third-party implementations. But the current the toSource method only supports a select number of selector implementations, all owned by JUnit. If the preconditions for those selectors align with those of the test sources, this could never happen. There is not even a test you could write to make this fail.

There was, actually. 😉 I found another inconsistency with PackageSelector which allowed "" and PackageSource which didn't.

Another problem is the normalization that FileSource and DirectorySource are doing by calling File.getCanonicalFile(). For now, I've reduced the scope of the try-catch block to those.

At the same time, any third-parties can implement custom selectors, but they'll have to do their own issue reporting.

They would still benefit from the built-in issue reporting but the reported discovery issue would not have a TestSource.

Would it make sense to add DiscoverySelector.toTestSource()? This would allow JUnit to test the toSource implementation on its own selectors and would allow third-party selectors to also provide a source.

I thought about it. I don't remember the specifics but we decided to introduce another FilePosition for use in DiscoverySelector implementations because they shouldn't reference the one in the ..support.descriptor package (see #2146 (comment)). If we now introduced toSource(), all selector implementations would have to reference that package.

Another option could be to introduce a ServiceLoader-based "converter" for DiscoverySelector to TestSource. But, if this is the only use case, I'm not sure it would be justified. If you feel strongly about needing a solution for third-party selector implementations, please open a new issue and we can discuss it for 6.0 (or later).

Copy link
Contributor

@mpkorstanje mpkorstanje Jul 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got no strong feelings about this.

Oh and nice catch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect a rethrowIfUnrecoverable here.

Copy link
Member Author

@marcphilipp marcphilipp Jul 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 4eb5925

logger.error(e, () -> "Failed to convert DiscoverySelector [%s] into TestSource".formatted(selector));
return null;
}
}

private static @Nullable TestSource toSource(DiscoverySelector selector) {
if (selector instanceof ClassSelector classSelector) {
return ClassSource.from(classSelector.getClassName());
}
Expand Down
3 changes: 3 additions & 0 deletions ...form-tests/src/test/java/org/junit/platform/engine/discovery/DiscoverySelectorsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,11 @@ void parseDirectorySelectorWithAbsolutePath() {
void selectClasspathResourcesPreconditions() {
assertViolatesPrecondition(() -> selectClasspathResource((String) null));
assertViolatesPrecondition(() -> selectClasspathResource(""));
assertViolatesPrecondition(() -> selectClasspathResource("/"));
assertViolatesPrecondition(() -> selectClasspathResource(" "));
assertViolatesPrecondition(() -> selectClasspathResource("/ "));
assertViolatesPrecondition(() -> selectClasspathResource("\t"));
assertViolatesPrecondition(() -> selectClasspathResource("/\t"));
assertViolatesPrecondition(() -> selectClasspathResource((Set<Resource>) null));
assertViolatesPrecondition(() -> selectClasspathResource(Collections.emptySet()));
assertViolatesPrecondition(() -> selectClasspathResource(Collections.singleton(null)));
Expand Down
Loading