Skip to content

Commit

Permalink
if I never think about URIs again it will be too soon
Browse files Browse the repository at this point in the history
  • Loading branch information
lbergelson committed Dec 13, 2023
1 parent ab7659f commit c971a89
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 16 deletions.
12 changes: 6 additions & 6 deletions src/main/java/htsjdk/io/HtsPath.java
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ private URI getURIForString(final String pathString) {
}

/**
* Check for problems associated with the presence of a heirachical scheme.
* Check for problems associated with the presence of a hierarchical scheme.
*
* It's better to reject cases like `://` or `ftp://I forgot to encode this` than to treat them as relative file paths
* It's almost certainly an error on the users part instead of an atttempt to intentionally reference a file named
Expand All @@ -295,19 +295,19 @@ private URI getURIForString(final String pathString) {
* @param pathString the path being examined
* @param cause the original failure reason
*/
static void assertNoProblematicScheme(String pathString, URISyntaxException cause){
static void assertNoProblematicScheme(String pathString, URISyntaxException cause){
if(pathString.equals(HIERARCHICAL_SCHEME_SEPARATOR)){
throw new IllegalArgumentException(HIERARCHICAL_SCHEME_SEPARATOR + " is not a valid path.", cause);
}

if(pathString.endsWith(HIERARCHICAL_SCHEME_SEPARATOR)){
final String[] split = pathString.split(HIERARCHICAL_SCHEME_SEPARATOR, -1);
final String scheme = split[0];

if(split.length == 2 && pathString.endsWith(HIERARCHICAL_SCHEME_SEPARATOR)) {
throw new IllegalArgumentException("A path consisting of only a scheme is not allowed: " + pathString, cause);
}

final String[] split = pathString.split(HIERARCHICAL_SCHEME_SEPARATOR);

if(split.length > 1){
final String scheme = split[0];
if(scheme == null || scheme.isEmpty()){
throw new IllegalArgumentException("Malformed path " + pathString + " includes an empty scheme.", cause);
}
Expand Down
26 changes: 16 additions & 10 deletions src/test/java/htsjdk/io/HtsPathUnitTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,14 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.FileSystem;
import java.nio.file.FileSystemNotFoundException;
import java.nio.file.FileSystems;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.ProviderNotFoundException;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

import static htsjdk.io.HtsPath.assertNoProblematicScheme;

public class HtsPathUnitTest extends HtsjdkTest {

final static String FS_SEPARATOR = FileSystems.getDefault().getSeparator();
Expand Down Expand Up @@ -130,6 +124,8 @@ public Object[][] validHtsPath() {
{"file:project/gvcf-pcr/23232_1#1/1.g.vcf.gz", "file:project/gvcf-pcr/23232_1#1/1.g.vcf.gz", true, false},
{"file:/project/gvcf-pcr/23232_1#1/1.g.vcf.gz", "file:/project/gvcf-pcr/23232_1#1/1.g.vcf.gz", true, false},
{"file:///project/gvcf-pcr/23232_1%231/1.g.vcf.g", "file:///project/gvcf-pcr/23232_1%231/1.g.vcf.g", true, true},

{"http://host/path://", "http://host/path://", false, false}
};
}

Expand Down Expand Up @@ -671,29 +667,39 @@ public Object[][] getNonProblematic() {
{"file/ name-sare/ :wierd-"},
{"hello there"},

//schemes schemes everywheret
{"file://://"},
{"file://://://"},
{"file://something://"},
{"file://something://somoethingelse"},

// file scheme is deliberately ignored here since it's handled specially later
{"file://unencoded file names/ are/ok!"},

//these aren't invalid URIs so they should never be tested by this method, they'll pass it though

{"eep/ee:e:e:ep"},
{"file:///"},
{"file:///"}
};
}
@Test(dataProvider = "getNonProblematic")
public void testAssertNoProblematicScheme(String path){
assertNoProblematicScheme(path, null);
HtsPath.assertNoProblematicScheme(path, null);
}

@DataProvider
public Object[][] getProblematic(){
return new Object[][]{

//This is the primary use case, to detect unencoded uris that were intended to be encoded
// This is the primary use case, to detect unencoded uris that were intended to be encoded.
// Note that assertNoProblematicScheme doesn't check for issues constructing the URI itself
// it is only called after a URI parsing exception has already occured.
{"http://forgot.com/to encode"},
{"ftp://server.com/file name.txt"},

{"://"},
{"://://"},
{"://://://"},

//this is technically a valid file name, but it seems very unlikely that anyone would do this delierately
//better to call it out
Expand All @@ -710,6 +716,6 @@ public Object[][] getProblematic(){

@Test(dataProvider = "getProblematic", expectedExceptions = IllegalArgumentException.class)
public void testAssertNoProblematicSchemeRejectedCases(String path){
assertNoProblematicScheme(path, null);
HtsPath.assertNoProblematicScheme(path, null);
}
}

0 comments on commit c971a89

Please sign in to comment.