Fixed bug disallowing file-based index and seekable path based source #769

Merged
merged 3 commits into from Dec 6, 2016
Jump to file or symbol
Failed to load files and symbols.
+39 −3
Split
@@ -316,7 +316,7 @@ 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);
@@ -326,7 +326,7 @@ public SamReader open(final SamInputResource resource) {
// 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();
@@ -284,7 +284,43 @@ public void queryInputResourcePermutation(final SamInputResource resource) throw
}
reader.close();
}
-
+
+
+ /**
+ * 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)
+ */
+ private static class NeverFilePathInputResource extends PathInputResource {
+ public NeverFilePathInputResource(Path pathResource) {
+ super(pathResource);
+ }
+
+ @Override
+ public File asFile() {
+ return null;
+ }
+ }
+
+ @Test
@droazen

droazen Dec 6, 2016

Contributor

Can you confirm that these tests both failed before this patch?

@kcibul

kcibul Dec 6, 2016 edited

Contributor

The query test actually doesn't fail if it can't use the index, it just falls back to not using the index and emits a warning. That seemed strange (to log a warning in a test rather than fail) but I didn't want to change it's behavior as it predates me. So I added the second test instead to assert the index is available which combined with the first test ensure it's both available AND does the right thing in query

+ 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
+ try (final SamReader reader = SamReaderFactory.makeDefault().open(new SamInputResource(bam, index))) {
+ Assert.assertTrue(reader.hasIndex());
+ }
+ }
+
+ @Test
+ public void queryStreamingPathBamWithFileIndex() throws IOException {
+ InputResource bam = new NeverFilePathInputResource(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 {