Copilot commented on code in PR #2715:
URL: https://github.com/apache/shiro/pull/2715#discussion_r3293834061
##########
lang/src/main/java/org/apache/shiro/lang/io/ResourceUtils.java:
##########
@@ -148,8 +148,10 @@ public static URL getURLForPath(String resourcePath)
throws IOException {
url = ClassUtils.getResource(stripPrefix(resourcePath));
} else if (resourcePath.startsWith(URL_PREFIX)) {
url = URI.create(stripPrefix(resourcePath)).toURL();
+ } else if (resourcePath.startsWith(FILE_PREFIX)) {
+ url = Paths.get(stripPrefix(resourcePath)).toUri().toURL();
} else {
- url = URI.create(resourcePath).toURL();
+ url = Paths.get(resourcePath).toUri().toURL();
}
Review Comment:
`getURLForPath` JavaDoc (above this method) still mentions loading
unprefixed paths via `URI#create(String)`, but the implementation now resolves
them via `Paths.get(...).toUri().toURL()`. Please update the JavaDoc to match
the current behavior (file-system `Path` resolution), so callers understand how
relative/absolute paths are interpreted.
##########
lang/src/test/java/org/apache/shiro/lang/util/ClassUtilsTest.java:
##########
@@ -84,4 +85,17 @@ void testGetClass() {
assertThat(ClassUtils.forName(ClassUtilsTest.class.getName())).isEqualTo(ClassUtilsTest.class);
assertThat(ClassUtils.forName(ClassUtilsTest[].class.getName())).isEqualTo(ClassUtilsTest[].class);
}
+
+ @Test
+ void inputStream() throws IOException {
Review Comment:
Test method name `inputStream` is quite generic and doesn’t indicate what
behavior is being validated (resource loading for
classpath/relative/file-prefixed paths). Renaming it to something more specific
(and consistent with the other `test...` methods in this class) would make
failures easier to interpret.
##########
lang/src/test/java/org/apache/shiro/lang/util/ClassUtilsTest.java:
##########
@@ -84,4 +85,17 @@ void testGetClass() {
assertThat(ClassUtils.forName(ClassUtilsTest.class.getName())).isEqualTo(ClassUtilsTest.class);
assertThat(ClassUtils.forName(ClassUtilsTest[].class.getName())).isEqualTo(ClassUtilsTest[].class);
}
+
+ @Test
+ void inputStream() throws IOException {
+ try (InputStream is =
getInputStreamForPath("classpath:org/apache/shiro/lang/util/ClassUtilsTest.class"))
{
+ assertThat(is.readAllBytes()).isNotEmpty();
+ }
+ try (InputStream is =
getInputStreamForPath("target/test-classes/test-data/file.json")) {
+ assertThat(is.readAllBytes()).isNotEmpty();
+ }
+ try (InputStream is =
getInputStreamForPath("file:target/test-classes/test-data/file.json")) {
+ assertThat(is.readAllBytes()).isNotEmpty();
+ }
Review Comment:
The PR description mentions restoring `file://` loading, but this test only
exercises an unprefixed relative path and a `file:`-prefixed relative path.
Please add a case that passes an actual file URI string (e.g., the value from
`Path#toUri()` which will be `file:/...` or `file:///...`) to ensure the
`file://`/URI form is covered.
##########
lang/src/main/java/org/apache/shiro/lang/io/ResourceUtils.java:
##########
@@ -148,8 +148,10 @@ public static URL getURLForPath(String resourcePath)
throws IOException {
url = ClassUtils.getResource(stripPrefix(resourcePath));
} else if (resourcePath.startsWith(URL_PREFIX)) {
url = URI.create(stripPrefix(resourcePath)).toURL();
+ } else if (resourcePath.startsWith(FILE_PREFIX)) {
+ url = Paths.get(stripPrefix(resourcePath)).toUri().toURL();
} else {
Review Comment:
For `file:`-prefixed values that are actual file URIs (e.g., `file:/...` /
`file:///...` with percent-encoding), stripping the scheme and feeding the raw
string into `Paths.get(...)` bypasses URI decoding and can point at the wrong
on-disk path (e.g., `%20` stays literal). Consider parsing as a `URI` when the
value is a URI form and converting via `Paths.get(uri)` (falling back to
treating it as a plain path only for `file:`-prefixed relative-path inputs).
--
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]
