gnodet-bot commented on code in PR #2224:
URL: https://github.com/apache/maven-resolver/pull/2224#discussion_r4356905894
##########
maven-resolver-api/src/main/java/org/eclipse/aether/repository/RepositoryUriUtils.java:
##########
@@ -113,6 +130,15 @@ private static String basedir(String url) {
return retValue.trim();
}
+ /**
+ * Returns {@code true} if the given protocol token is a single ASCII
letter, which on Windows indicates
+ * a drive letter (e.g. {@code C} in {@code C:\path} or {@code c} in
{@code c:/path}) rather than a URL scheme.
+ * The check is locale-independent.
+ */
+ private static boolean isWindowsDriveLetter(String protocol) {
+ return protocol.length() == 1 &&
Character.isLetter(protocol.charAt(0));
Review Comment:
🔧 **Nit — Javadoc/implementation mismatch**: The Javadoc says "single ASCII
letter" and "locale-independent", but `Character.isLetter()` is Unicode-aware
and returns `true` for non-ASCII letters (e.g. `'\u00F1'` ñ). Use an explicit
ASCII range check:
```suggestion
private static boolean isWindowsDriveLetter(String protocol) {
if (protocol.length() != 1) {
return false;
}
char c = protocol.charAt(0);
return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
}
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/repository/RepositoryUriUtils.java:
##########
@@ -39,11 +39,22 @@ private RepositoryUriUtils() {}
*/
public static URI toUri(String repositoryUrl) {
final String protocol = protocol(repositoryUrl);
- if ("file".equals(protocol)
- || protocol.isEmpty()
- || protocol.length() == 1
- && (Character.isLetter(protocol.charAt(0)) &&
Character.isUpperCase(protocol.charAt(0)))) {
- return new File(basedir(repositoryUrl)).toURI();
+ if ("file".equals(protocol)) {
+ // Parse as a proper file: URI so that percent-encoding and
Windows paths like
+ // file:/C:/... or file:///C:/... are handled correctly by the JDK.
+ // Normalize single-slash file URIs (file:/D:/...) to triple-slash
(file:///D:/...)
+ // so that Paths.get(URI) works on Windows: the JDK strips the
leading '/' when
+ // the authority component is empty (""), giving "D:/..." which
Windows accepts.
+ String normalized = repositoryUrl;
+ if (repositoryUrl.startsWith("file:/") &&
!repositoryUrl.startsWith("file://")) {
+ // file:/X:/... → file:///X:/... (single-slash non-standard
form)
+ normalized = "file://" +
repositoryUrl.substring("file:".length());
+ }
+ return Paths.get(URI.create(normalized)).toUri();
Review Comment:
⚠️ **Regression**: `URI.create(normalized)` throws
`IllegalArgumentException` for URLs with unencoded spaces or opaque `file:`
URLs, breaking existing `FileTransporterTest` cases:
- `file:repo space` → `Illegal character in opaque part`
(`testInit_OpaqueUrlSpacesDecoded`)
- `file:/repo space` → normalized to `file:///repo space` → `Illegal
character in path` (`testInit_HierarchicalUrlSpacesDecoded`)
- `file:repo%20space` → `URI is not hierarchical` — `Paths.get(URI)`
requires a hierarchical URI; opaque `file:` URIs fail
(`testInit_OpaqueUrlSpaces`)
The old `new File(basedir(url)).toURI()` tolerated all of these because
`File(String)` doesn't treat the string as a URI. Consider handling opaque
`file:` URLs (no leading `/` after `file:`) by stripping the scheme and using
`Paths.get(String)`, and separately percent-encoding spaces before
`URI.create()` for the hierarchical case.
--
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]
