From e1bf61ec3e66cfa1cd469866cfd1347d2ade1568 Mon Sep 17 00:00:00 2001 From: Kristian Cibulskis Date: Tue, 6 Dec 2016 13:41:02 -0500 Subject: [PATCH 1/3] Added support for use case where index is a file, and the source stream is a seekable path (e.g. a GCS location) --- .../java/htsjdk/samtools/SamReaderFactory.java | 12 ++++++-- .../java/htsjdk/samtools/SamReaderFactoryTest.java | 32 +++++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/main/java/htsjdk/samtools/SamReaderFactory.java b/src/main/java/htsjdk/samtools/SamReaderFactory.java index 2b57cbfb2..e4dd08699 100644 --- a/src/main/java/htsjdk/samtools/SamReaderFactory.java +++ b/src/main/java/htsjdk/samtools/SamReaderFactory.java @@ -317,9 +317,15 @@ public SamReader open(final SamInputResource resource) { // do not close bufferedStream, it's the same stream we're getting here. SeekableStream sourceSeekable = data.asUnbufferedSeekableStream(); if (indexFile!=null || null == sourceSeekable || null == indexSeekable) { - // not seekable. - // it's OK that we consumed a bit of the stream already, this ctor expects it. - primitiveSamReader = new BAMFileReader(bufferedStream, indexFile, false, asynchronousIO, validationStringency, this.samRecordFactory); + if (null == sourceSeekable || null == indexSeekable) { + // not seekable. + // it's OK that we consumed a bit of the stream already, this ctor expects it. + primitiveSamReader = new BAMFileReader(bufferedStream, indexFile, false, asynchronousIO, validationStringency, this.samRecordFactory); + } else { + sourceSeekable.seek(0); + primitiveSamReader = new BAMFileReader( + sourceSeekable, indexSeekable, false, asynchronousIO, validationStringency, this.samRecordFactory); + } } else { // seekable. // need to return to the beginning because it's the same stream we used earlier diff --git a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java index ece91e220..8c1e1d6fe 100644 --- a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java +++ b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java @@ -284,7 +284,37 @@ public void queryInputResourcePermutation(final SamInputResource resource) throw } reader.close(); } - + + public class NeverFilePathInpurResource extends PathInputResource { + public NeverFilePathInpurResource(Path pathResource) { + super(pathResource); + } + + @Override + public File asFile() { + return null; + } + } + + @Test + public void streamingPathBamWithFileIndex() throws IOException { + InputResource bam = new NeverFilePathInpurResource(localBam.toPath()); + InputResource index = new FileInputResource(localBamIndex); + + // ensure that the index is being used, not checked in queryInputResourcePermutation + final SamReader reader = SamReaderFactory.makeDefault().open(new SamInputResource(bam, index)); + Assert.assertTrue(reader.hasIndex()); + } + + @Test + public void queryStreamingPathBamWithFileIndex() throws IOException { + InputResource bam = new NeverFilePathInpurResource(localBam.toPath()); + InputResource index = new FileInputResource(localBamIndex); + + final SamInputResource resource = new SamInputResource(bam, index); + queryInputResourcePermutation(new SamInputResource(bam, index)); + } + @Test public void customReaderFactoryTest() throws IOException { try { From 751a11b8043ad139a26b94517ceba8511ba95f1e Mon Sep 17 00:00:00 2001 From: Kristian Cibulskis Date: Tue, 6 Dec 2016 16:56:21 -0500 Subject: [PATCH 2/3] Addressed PR comments --- src/main/java/htsjdk/samtools/SamReaderFactory.java | 16 +++++----------- .../java/htsjdk/samtools/SamReaderFactoryTest.java | 20 +++++++++++++------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/main/java/htsjdk/samtools/SamReaderFactory.java b/src/main/java/htsjdk/samtools/SamReaderFactory.java index e4dd08699..8769f4879 100644 --- a/src/main/java/htsjdk/samtools/SamReaderFactory.java +++ b/src/main/java/htsjdk/samtools/SamReaderFactory.java @@ -316,23 +316,17 @@ public SamReader open(final SamInputResource resource) { final SeekableStream indexSeekable = indexMaybe == null ? null : indexMaybe.asUnbufferedSeekableStream(); // do not close bufferedStream, it's the same stream we're getting here. SeekableStream sourceSeekable = data.asUnbufferedSeekableStream(); - if (indexFile!=null || null == sourceSeekable || null == indexSeekable) { - if (null == sourceSeekable || null == indexSeekable) { - // not seekable. - // it's OK that we consumed a bit of the stream already, this ctor expects it. - primitiveSamReader = new BAMFileReader(bufferedStream, indexFile, false, asynchronousIO, validationStringency, this.samRecordFactory); - } else { - sourceSeekable.seek(0); - primitiveSamReader = new BAMFileReader( - sourceSeekable, indexSeekable, false, asynchronousIO, validationStringency, this.samRecordFactory); - } + if (null == sourceSeekable || null == indexSeekable) { + // not seekable. + // it's OK that we consumed a bit of the stream already, this ctor expects it. + primitiveSamReader = new BAMFileReader(bufferedStream, indexFile, false, asynchronousIO, validationStringency, this.samRecordFactory); } else { // seekable. // need to return to the beginning because it's the same stream we used earlier // and read a bit from, and that form of the ctor expects the stream to start at 0. sourceSeekable.seek(0); primitiveSamReader = new BAMFileReader( - sourceSeekable, indexSeekable, false, asynchronousIO, validationStringency, this.samRecordFactory); + sourceSeekable, indexSeekable, false, asynchronousIO, validationStringency, this.samRecordFactory); } } else { bufferedStream.close(); diff --git a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java index 8c1e1d6fe..3de6ef671 100644 --- a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java +++ b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java @@ -285,8 +285,13 @@ public void queryInputResourcePermutation(final SamInputResource resource) throw reader.close(); } - public class NeverFilePathInpurResource extends PathInputResource { - public NeverFilePathInpurResource(Path pathResource) { + + /** + * A path that pretends it's not based upon a file. This helps in cases where we want to test branches + * that apply to non-file based paths without actually having to use non-file based resources (like cloud urls) + */ + public static class NeverFilePathInputResource extends PathInputResource { + public NeverFilePathInputResource(Path pathResource) { super(pathResource); } @@ -297,18 +302,19 @@ public File asFile() { } @Test - public void streamingPathBamWithFileIndex() throws IOException { - InputResource bam = new NeverFilePathInpurResource(localBam.toPath()); + public void checkHasIndexForStreamingPathBamWithFileIndex() throws IOException { + InputResource bam = new NeverFilePathInputResource(localBam.toPath()); InputResource index = new FileInputResource(localBamIndex); // ensure that the index is being used, not checked in queryInputResourcePermutation - final SamReader reader = SamReaderFactory.makeDefault().open(new SamInputResource(bam, index)); - Assert.assertTrue(reader.hasIndex()); + try (final SamReader reader = SamReaderFactory.makeDefault().open(new SamInputResource(bam, index))) { + Assert.assertTrue(reader.hasIndex()); + } } @Test public void queryStreamingPathBamWithFileIndex() throws IOException { - InputResource bam = new NeverFilePathInpurResource(localBam.toPath()); + InputResource bam = new NeverFilePathInputResource(localBam.toPath()); InputResource index = new FileInputResource(localBamIndex); final SamInputResource resource = new SamInputResource(bam, index); From 1049ff207393523bf50835f43168fd3f01f51dfa Mon Sep 17 00:00:00 2001 From: Kristian Cibulskis Date: Tue, 6 Dec 2016 17:11:51 -0500 Subject: [PATCH 3/3] Addressed missed PR comment --- src/test/java/htsjdk/samtools/SamReaderFactoryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java index 3de6ef671..31ad5c259 100644 --- a/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java +++ b/src/test/java/htsjdk/samtools/SamReaderFactoryTest.java @@ -290,7 +290,7 @@ public void queryInputResourcePermutation(final SamInputResource resource) throw * A path that pretends it's not based upon a file. This helps in cases where we want to test branches * that apply to non-file based paths without actually having to use non-file based resources (like cloud urls) */ - public static class NeverFilePathInputResource extends PathInputResource { + private static class NeverFilePathInputResource extends PathInputResource { public NeverFilePathInputResource(Path pathResource) { super(pathResource); }