Re-Ordering priority in SeekableStreamFactory #1693

lbergelson · 2023-12-11T17:18:25Z

Now when constructing streams it will prefer NIO plugins.
If no http / ftp plugin exists it will fall back to the htsjdk bulit in
No longer special casing local files, these will now be handled through NIO as well.
Deprecating SeekableStreamFactory.isFilePath() since it is no longer used and interacts poorly
with nio filesystem providers

* Now when constructing streams it will prefer NIO plugins. * If no http / ftp plugin exists it will fall back to the htsjdk bulit in * No longer special casing local files, these will now be handled through NIO as well. * Deprecating SeekableStreamFactory.isFilePath() since it is no longer used and interacts poorly with nio filesystem providers

droazen · 2023-12-11T18:20:24Z

Can you also fix the order in ParsingUtils.openInputStream() (used by TribbleIndexedFeatureReader.readHeader())?

droazen

Looks good, modulo a few requests

droazen · 2023-12-11T18:23:33Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

+     * @return if the path is not being handled by a FileSystemProvider and it can be read by legacy streams
+     */
+    public static boolean isSpecialCase(final String path){
+        return !new HtsPath(path).isPath()  //if we have a provider for it that's what we'll use


Maybe break out out a hasNIOPlugin() (or similar) method for the new HtsPath(path).isPath() everywhere, for clarity?

Or should you be using hasFileSystemProvider() instead?

droazen · 2023-12-11T18:24:56Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

+    }
+
+    //is this onee of the url types that has legacy htsjdk support built in?
+    private static boolean isSpecialCaseUrlType(final String path) {


isSupportedByLegacyCloudReaders()?

droazen · 2023-12-11T18:32:56Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

+        public static SeekableStream getStreamFor(final IOPath path, Function<SeekableByteChannel, SeekableByteChannel> wrapper) throws IOException {
+            if(path.isPath()) {
+                return path.getScheme().equals("file")
+                        ? new SeekablePathStream(path.toPath()) //don't apply the wrapper to local files


Check usages of SeekablePathStream.getSource() to make sure the switch from File -> Path for local files is safe (concerned about file:// appearing unexpectedly downstream). Or to be super-safe, could stick with SeekableFileStream for now.

I switched back to SeekableFileStream although it makes me sad.

droazen · 2023-12-11T18:38:04Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

+         * @param wrapper a wrapper to apply to the stream allowing direct transformations on the byte stream to be applied
+         * @throws IOException
+         */
+        public static SeekableStream getStreamFor(final IOPath path, Function<SeekableByteChannel, SeekableByteChannel> wrapper) throws IOException {


If there aren't already tests covering file:// vs. local file paths (no protocol), we might want one

…ad of trying to interpret them as file://

droazen

Looks good, just some minor comments. Merge once tests pass and you've addressed them. Have you verified that with this patch we are using http-nio in tribble in GATK? Did you look at the SAM/BAM/CRAM path?

droazen · 2023-12-12T21:15:45Z

src/main/java/htsjdk/io/HtsPath.java

@@ -253,6 +253,10 @@ private URI getURIForString(final String pathString) {
                tempURI = getCachedPath().toUri();
            }
        } catch (URISyntaxException uriException) {
+            //check that the uri wasn't a badly encoded absolute uri of some sort
+            //if you don't do this it will be treated as a badly formed file:/ url


droazen · 2023-12-12T21:15:54Z

src/main/java/htsjdk/io/HtsPath.java

+                        "\nCheck that it is fully encoded.", cause);
+            }
+            if(!split[0].equals("file")){
+                throw new IllegalArgumentException("Malformed url " + pathString + " includes a scheme: " + split[0] + ":/ but was an invalid URI." +


droazen · 2023-12-12T21:18:01Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

+
+    //is this onee of the url types that has legacy htsjdk support built in?
+    public static boolean canBeHandledByLegacyUrlSupport(final String path) {
+        return URL_SCHEMES_WITH_LEGACY_SUPPORT.stream().anyMatch(path::startsWith);


Shouldn't you be checking that the path starts with "http://" (for example) instead of "http"? What if there's a file named "http"?

droazen · 2023-12-12T21:18:37Z

src/main/java/htsjdk/samtools/seekablestream/SeekableStreamFactory.java

@@ -40,6 +43,14 @@
 public class SeekableStreamFactory{

    private static final ISeekableStreamFactory DEFAULT_FACTORY;
+    private static final String HTTP = "http";
+    private static final String HTTPS = "https";
+    private static final String FTP = "ftp";


See below -- recommend including "://" in these constants to guard against files named after a protocol

instead of adding them to the constants which are also used directly compared against getScheme the isHandledBy method now appends '://' to them.

droazen · 2023-12-12T21:26:35Z

src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java

+    }
+
+    @Test(dataProvider = "getSpecialCasePaths")
+    public void testCanBeHandledByLegacyUrlSuppoort(String path, boolean notExpected){


Suppoort....this test is identical to the one just before it. Did you mean to test canBeHandledByLegacyUrlSupport() here instead?

droazen · 2023-12-12T21:29:57Z

src/test/java/htsjdk/samtools/seekablestream/SeekableStreamFactoryTest.java

+                Assert.assertEquals(new String(bytes, StandardCharsets.UTF_8), "hello");
+            }
+        }
+    }


Is there any easy way to get test coverage for the case where isBeingHandledByLegacyUrlSupport() detects that an NIO plugin exists for the legacy-supported URL? I'm guessing not, but thought I'd ask.

not an easy way, we can add an additional test suite in gradle but it's going to be a hassle.

forget it then

I did a manual test where I added the http-nio library to the test dependencies and isBeingHandledByLegacySupport for http / https is changes as expected.

* fixing edge cases as well as documentation and tests from the change to HtsPath in #1693

lbergelson force-pushed the lb_fix_tribble_http_issue branch from 9974db4 to 6cdcbd4 Compare December 11, 2023 17:32

lbergelson marked this pull request as ready for review December 11, 2023 17:42

droazen approved these changes Dec 11, 2023

View reviewed changes

lbergelson added 3 commits December 11, 2023 16:29

Also handling ParsingUtils

b949659

update HtsPath to throw on non file schemes with malformed URIs inste…

9eca91a

…ad of trying to interpret them as file://

try this

5376f55

droazen approved these changes Dec 12, 2023

View reviewed changes

respond to comments

652eda2

lbergelson merged commit 40e3a8b into master Dec 12, 2023
4 checks passed

lbergelson added a commit that referenced this pull request Dec 13, 2023

Improving the last changes to HtsPath (#1694)

b23c36a

* fixing edge cases as well as documentation and tests from the change to HtsPath in #1693

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-Ordering priority in SeekableStreamFactory #1693

Re-Ordering priority in SeekableStreamFactory #1693

lbergelson commented Dec 11, 2023

droazen commented Dec 11, 2023 •

edited

Loading

droazen left a comment

droazen Dec 11, 2023

droazen Dec 11, 2023

droazen Dec 11, 2023

droazen Dec 11, 2023

lbergelson Dec 11, 2023

droazen Dec 11, 2023

droazen left a comment

droazen Dec 12, 2023

droazen Dec 12, 2023

droazen Dec 12, 2023

lbergelson Dec 12, 2023

droazen Dec 12, 2023

lbergelson Dec 12, 2023

droazen Dec 12, 2023

lbergelson Dec 12, 2023

droazen Dec 12, 2023

lbergelson Dec 12, 2023

droazen Dec 12, 2023

lbergelson Dec 12, 2023

Re-Ordering priority in SeekableStreamFactory #1693

Re-Ordering priority in SeekableStreamFactory #1693

Conversation

lbergelson commented Dec 11, 2023

droazen commented Dec 11, 2023 • edited Loading

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen commented Dec 11, 2023 •

edited

Loading