feat: Add sanity check to indexed FASTA file - #1745
Conversation
| final Iterator<FastaSequenceIndexEntry> iterator = FastaSequenceIndex.iterator(); | ||
| FastaSequenceIndexEntry fastaSequenceIndex = null; | ||
| while (iterator.hasNext()) { | ||
| fastaSequenceIndex = iterator.next(); | ||
| } |
There was a problem hiding this comment.
It kinda sucks to have to do this iteration. I wonder if it would make sense to expand the PR a little bit, and modify FastaSequenceIndex to either store index entries in a pair of (Array/ArrayList, HashMap (instead of LinkedHashMap))? Or just to hold an extra pointer to the final entry in the index?
There was a problem hiding this comment.
There are several operations that explicitly take advantage of the linked-list aspect...come to think of it, I my sanity-check only works on a "freshly make" object, since operations such as "rename" remove and add entries, which moved the renamed entry to the "end" of the linked-list.... I only need it to work upon initialization, but I need to add some protections. upshot is that a list would be overkill, I'll go with the reference to the last element.
| while (iterator.hasNext()) { | ||
| fastaSequenceIndex = iterator.next(); | ||
| } | ||
| assert fastaSequenceIndex != null; |
There was a problem hiding this comment.
Asserts can be disabled at runtime - are you ok with that, or do you want to always check this?
| * @param fastaFile Used for error reporting only. | ||
| * @param index index file to check against the dictionary. | ||
| */ | ||
| public static void sanityCheckFastaAgainstIndex(final String fastaFile, |
| throw new IllegalArgumentException("The fasta file is shorter (%d) than its index claims (%d). Please reindex the fasta.".formatted(fastaLength, lastSequenceEnd)); | ||
| } | ||
| // not sure why need to add 1 here. | ||
| if (lastSequenceEnd + fastaSequenceIndex.getTerminatorLength() + 1 < fastaLength) { |
There was a problem hiding this comment.
Honestly I wonder if we should allow for more than 1? What if there are a handful of blank lines at the end? Maybe we could try a nested solution, that if the file is longer than you have here ... then we read the content after the last base and ensure it's all whitespace?
| final long lastSequenceStart = fastaSequenceIndex.getLocation(); | ||
| final long lastSequenceEnd = lastSequenceStart + fastaSequenceIndex.getOffset(lastSequenceLength); | ||
|
|
||
| final long fastaLength = new File(fastaFile).length(); |
There was a problem hiding this comment.
Is this safe to do? Does AbstractIndexedFastaSequenceFile support or have sub-classes that support URL based access (like http:// or ftp://)? If so ... is there a method to call to get the file/object length?
| final int basesPerLine = this.getBasesPerLine(); | ||
| final int bytesPerLine = this.getBytesPerLine(); | ||
|
|
||
| return ((pos - 1) / basesPerLine) * bytesPerLine + (pos - 1) % basesPerLine; |
There was a problem hiding this comment.
| return ((pos - 1) / basesPerLine) * bytesPerLine + (pos - 1) % basesPerLine; | |
| return ((pos - 1) / basesPerLine) * bytesPerLine + ((pos - 1) % basesPerLine); |
Just for clarity.
|
Thanks @tfenne. I responded to all your comments (I think!) please re-review. BTW, the test failures were due to the FTP site not responding. |
tfenne
left a comment
tfenne
left a comment
There was a problem hiding this comment.
LGTM - should anyone else review?
| final long lastSequenceEnd = lastSequenceStart + lastSequenceIndex.getOffset(lastSequenceLength); | ||
|
|
||
| final long fastaLength = Files.size(fastaFile); | ||
| //Question: should we worry about files with lots of whitespace in their end? |
There was a problem hiding this comment.
I wouldn't worry about "lots of whitespace" at the end. I think that is still technically valid fasta.
| if (!Character.isWhitespace((char) b)) { | ||
| throw new IllegalArgumentException( | ||
| ("The fasta file %s is too long (relative to the index). In particular has a non-whitespace " + | ||
| "character (%c) as a position too great (%d) given the claims of its index (%d)." + |
There was a problem hiding this comment.
| "character (%c) as a position too great (%d) given the claims of its index (%d)." + | |
| "character (%c) at a position (%d) beyond the last base according to its index (%d)." + |
| ("The fasta file %s is too long (relative to the index). In particular has a non-whitespace " + | ||
| "character (%c) as a position too great (%d) given the claims of its index (%d)." + | ||
| " Please reindex the fasta.") | ||
| .formatted(fastaFile.getFileName(), (char) b, posOfInterest + i, lastSequenceEnd)); |
There was a problem hiding this comment.
I would emit the absolute path here
|
@lbergelson is @tfenne 's review enough here? who else needs to review? |
lbergelson
left a comment
lbergelson
left a comment
There was a problem hiding this comment.
This looks sane to me I think. I need to convince myself that it works for fasta.gz though. If it doesn't it should be failing tests here so I'm assuming it works out somehow. I have a few comments. I started it running against GATK's tests to see if it hits any weirrd problems I didn't think of. broadinstitute/gatk#9179
| final long fastaLength = Files.size(fastaFile); | ||
|
|
||
| if (lastSequenceEnd > fastaLength) { | ||
| throw new IllegalArgumentException("The fasta file (%s) is shorter (%d) than its index (%s) claims (%d). Please reindex the fasta.".formatted(fastaFile.toUri().toString(),fastaLength, index.toString(), lastSequenceEnd)); |
There was a problem hiding this comment.
IllegalArgument seems like the wrong exception here. I might declare a new IncompatibleIndexException or something like that, it would make it easier for calling code to deal with this. If for example someone wanted to automatically reindex on a failure.
| .formatted(fastaFile.toUri().toString(), (char) b, posOfInterest + i, lastSequenceEnd,index.toString())); | ||
| } | ||
| } | ||
| posOfInterest += channelBuffer.limit(); |
There was a problem hiding this comment.
I think there might be minor bug here. When you clear the buffer it should set limit = capacity, but I don't think readFromPosition guarantees that the as many bytes as possible are read, it's possible for it to return fewer than requested. I think this might have the effect of skipping parts of the fasta file on incomplete reads.
Setting it with position or capturing the return value from readFromPosition would both do the right thing I think.
Unlikely to be a real issue since the bug would be to sometimes fail to detect a mismatch if there are characters embedded partway through a block of white space.
| * @param fastaFile Path to fasta file | ||
| * @param fastaSequenceIndexes index file to check against the fasta file. | ||
| * | ||
| * @throws IOException in case of io-error when reading fastaFile |
There was a problem hiding this comment.
This should say what it throws if it detects and error.
| * @param pos the (1-based) position in the contig that is requested | ||
| * @return the offset (0-based) from 'location' where pos is located in the file. | ||
| */ | ||
| public long getOffset(long pos) { |
| * @return The name of the last entry that was added when parsing the index file. Only guarranteed to be correct just | ||
| * after initialization. Protected for access from AbstractIndexedFastaSequenceFile. | ||
| */ | ||
| protected String getLastSequence() { |
There was a problem hiding this comment.
I don't love this with the caveats but it's fine if it's clearly labelled.
| * Empty, protected constructor for unit testing. Use with care, lastSequence will be incorrect. | ||
| */ | ||
| protected FastaSequenceIndex() {} | ||
| protected FastaSequenceIndex() { |
There was a problem hiding this comment.
I wonder if this is really only used for unit testing.
|
Hmn, I see a test failure that's not the broken FTP ones. |
|
It looks like I think the lastSequence cache needs to be kept up to date or just dynamically checked. I assume there was a performance reason for caching it? |
When opening an indexed FASTA, cheaply verify that the file is consistent with its index: the fasta must be at least as long as the last base position the index claims, and any bytes past the last base must be whitespace. This catches the common (and otherwise silent) failure where a stale or incorrect .fai causes the wrong bases to be returned. It reads only the file length and the trailing bytes rather than the whole file, so it is cheap to run on every open. Originally #1745 by @yfarjoun; rebased onto current master and reworked so that the index's last entry is derived on demand from the insertion-ordered entries (instead of a cached name that went stale for some index-construction paths and produced false positives), and so the trailing scan advances by the number of bytes actually read. Co-authored-by: Tim Fennell <tfenne@tfenne.com>
b6a1842 to
45c1a73
Compare
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIndexed FASTA reads now use index-derived offsets. Construction validates indexed FASTA content and permits only whitespace after the indexed region. Tests cover mismatched indexes, whitespace, missing newlines, truncation, and index renaming. ChangesFASTA index validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IndexedFastaSequenceFile
participant FastaSequenceIndex
participant FastaSequenceIndexEntry
participant FASTA
IndexedFastaSequenceFile->>FastaSequenceIndex: getLastIndexEntry()
FastaSequenceIndex-->>IndexedFastaSequenceFile: final indexed entry
IndexedFastaSequenceFile->>FastaSequenceIndexEntry: getOffset(last indexed base)
FastaSequenceIndexEntry-->>IndexedFastaSequenceFile: expected final base offset
IndexedFastaSequenceFile->>FASTA: inspect file length and trailing bytes
FASTA-->>IndexedFastaSequenceFile: content length and trailing bytes
IndexedFastaSequenceFile-->>IndexedFastaSequenceFile: accept or throw IllegalArgumentException
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.java (1)
77-88: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueApply the sanity check consistently across constructors.
The new
sanityCheckFastaAgainstIndexvalidation was added to thePathconstructor but omitted from thisIOPathconstructor. To ensure consistent protection against mismatched fasta and index files, consider adding the sanity check here as well.🛠️ Proposed fix
if (IOUtil.isBlockCompressed(path.toPath(), true)) { throw new SAMException("Indexed block-compressed FASTA file cannot be handled: " + path); } this.channel = Files.newByteChannel(path.toPath()); + sanityCheckFastaAgainstIndex(path.toPath(), index); } catch (IOException e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.java` around lines 77 - 88, Update the IOPath-based IndexedFastaSequenceFile constructor to invoke sanityCheckFastaAgainstIndex after opening the FASTA channel, matching the validation performed by the Path constructor. Ensure the check uses the provided index and preserves the existing block-compression and IOException handling.src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.java (1)
170-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer using the method parameter
fastaSequenceIndexinstead of the instance fieldindex.In the exception messages below (lines 180 and 199), the instance field
indexis used instead of thefastaSequenceIndexparameter. While they refer to the same object in the current implementation, using the parameter explicitly prevents potential bugs if this method is ever called with an index other than the instance field.♻️ Proposed refactor
Apply the parameter in the formatted string on line 180:
if (lastBasePosition > fastaLength) { throw new IllegalArgumentException( "The fasta file (%s) is shorter (%d) than its index (%s) claims (%d). Please reindex the fasta." - .formatted(fastaFile.toUri(), fastaLength, index, lastBasePosition)); + .formatted(fastaFile.toUri(), fastaLength, fastaSequenceIndex, lastBasePosition)); }And similarly on line 199:
if (!Character.isWhitespace((char) b)) { throw new IllegalArgumentException( ("The fasta file (%s) is longer than its index (%s) accounts for: found a non-whitespace " + "character (%c) at position %d, beyond the last base at %d. Please reindex the fasta.") - .formatted(fastaFile.toUri(), index, (char) b, position + i, lastBasePosition)); + .formatted(fastaFile.toUri(), fastaSequenceIndex, (char) b, position + i, lastBasePosition)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.java` at line 170, Update sanityCheckFastaAgainstIndex to use its fastaSequenceIndex parameter, rather than the instance field index, in both exception-message format arguments around the reported validation failures. Leave the validation logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.java`:
- Around line 177-185: In the FASTA length sanity check, update the
lastBasePosition comparison to reject files where fastaLength equals the last
base index by using an inclusive boundary. Initialize position to the byte
immediately after the last base, using lastBasePosition + 1 rather than
lastSequence.getTerminatorLength(), while preserving the existing trailing-byte
whitespace validation.
---
Nitpick comments:
In
`@src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.java`:
- Line 170: Update sanityCheckFastaAgainstIndex to use its fastaSequenceIndex
parameter, rather than the instance field index, in both exception-message
format arguments around the reported validation failures. Leave the validation
logic unchanged.
In `@src/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.java`:
- Around line 77-88: Update the IOPath-based IndexedFastaSequenceFile
constructor to invoke sanityCheckFastaAgainstIndex after opening the FASTA
channel, matching the validation performed by the Path constructor. Ensure the
check uses the provided index and preserves the existing block-compression and
IOException handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d897416-688f-42c6-b3f6-aceccb0cec9c
📒 Files selected for processing (6)
src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndex.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndexEntry.javasrc/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.javasrc/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.javasrc/test/resources/htsjdk/samtools/reference/header_with_extra_white_space.fasta
When opening an indexed FASTA, cheaply verify that the file is consistent with its index: the fasta must be at least as long as the last base position the index claims, and any bytes past the last base must be whitespace. This catches the common (and otherwise silent) failure where a stale or incorrect .fai causes the wrong bases to be returned. It reads only the file length and the trailing bytes rather than the whole file, so it is cheap to run on every open. Originally #1745 by @yfarjoun; rebased onto current master and reworked so that the index's last entry is derived on demand from the insertion-ordered entries (instead of a cached name that went stale for some index-construction paths and produced false positives), and so the trailing scan advances by the number of bytes actually read. Co-authored-by: Tim Fennell <tfenne@tfenne.com>
45c1a73 to
4168587
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.java`:
- Around line 572-590: Update the IndexedFastaSequenceFile constructor to close
its channel when sanityCheckFastaAgainstIndex throws a RuntimeException, then
rethrow the original exception. Update testSanityCheckRejectsTruncatedFasta
cleanup to remove the generated index and all directory contents before deleting
the temporary directory, using the existing directory-tree cleanup utility if
available.
- Around line 554-570: Update testSanityCheckAcceptsFastaWithoutTrailingNewline
cleanup to remove the generated FastaSequenceIndexCreator .fai file before
deleting the temporary directory, or use the project’s directory-tree deletion
utility. Preserve cleanup in the finally block so the test does not leave
artifacts and directory deletion succeeds.
- Around line 554-570: Update both test methods
testSanityCheckAcceptsFastaWithoutTrailingNewline at
src/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.java:554-570
and the sibling test at :572-590 to remove the generated .fai file during
cleanup, or use IOUtil.deleteDirectoryTree for complete temporary-directory
cleanup; ensure each finally block can delete the directory without
DirectoryNotEmptyException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18c81fb2-5e0e-4bf1-ba33-b6c8890fb1fc
📒 Files selected for processing (6)
src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndex.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndexEntry.javasrc/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.javasrc/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.javasrc/test/resources/htsjdk/samtools/reference/header_with_extra_white_space.fasta
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/resources/htsjdk/samtools/reference/header_with_extra_white_space.fasta
- src/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.java
- src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.java
- src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java
When opening an indexed FASTA, cheaply verify that the file is consistent with its index: the fasta must be at least as long as the last base position the index claims, and any bytes past the last base must be whitespace. This catches the common (and otherwise silent) failure where a stale or incorrect .fai causes the wrong bases to be returned. It reads only the file length and the trailing bytes rather than the whole file, so it is cheap to run on every open. Originally #1745 by @yfarjoun; rebased onto current master and reworked so that the index's last entry is derived on demand from the insertion-ordered entries (instead of a cached name that went stale for some index-construction paths and produced false positives), and so the trailing scan advances by the number of bytes actually read. Co-authored-by: Tim Fennell <tfenne@tfenne.com>
4168587 to
3f8c5b3
Compare
When opening an indexed FASTA, cheaply verify that the file is consistent with its index: the fasta must be at least as long as the last base position the index claims, and any bytes past the last base must be whitespace. This catches the common (and otherwise silent) failure where a stale or incorrect .fai causes the wrong bases to be returned. It reads only the file length and the trailing bytes rather than the whole file, so it is cheap to run on every open. Originally #1745 by @yfarjoun; rebased onto current master and reworked so that the index's last entry is derived on demand from the insertion-ordered entries (instead of a cached name that went stale for some index-construction paths and produced false positives), and so the trailing scan advances by the number of bytes actually read. Co-authored-by: Tim Fennell <tfenne@tfenne.com>
3f8c5b3 to
f683d18
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java (1)
132-132: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
entries.hashCode()directly.
Objects.hash(entries)wraps the argument in a single-element array to compute the hash (effectively computing31 * 1 + entries.hashCode()). Since you are only hashing one object, delegating directly toentries.hashCode()avoids the unnecessary array allocation and is more idiomatic.♻️ Proposed refactor
`@Override` public int hashCode() { - return Objects.hash(entries); + return entries.hashCode(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java` at line 132, Update the hashCode implementation in FastaSequenceIndex to return entries.hashCode() directly instead of using Objects.hash(entries), preserving the existing hash source while avoiding unnecessary wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java`:
- Around line 99-105: Update the rename logic in FastaSequenceIndex to
short-circuit when newName matches the entry’s current contig, preserving no-op
renames. Otherwise validate entriesByContig for newName before removing the old
mapping, then perform the existing entry update and map insertion only after
validation succeeds.
---
Nitpick comments:
In `@src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java`:
- Line 132: Update the hashCode implementation in FastaSequenceIndex to return
entries.hashCode() directly instead of using Objects.hash(entries), preserving
the existing hash source while avoiding unnecessary wrapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc7006fe-bd6d-4997-a000-a8f572c3b2e4
📒 Files selected for processing (6)
src/main/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFile.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndex.javasrc/main/java/htsjdk/samtools/reference/FastaSequenceIndexEntry.javasrc/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.javasrc/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.javasrc/test/resources/htsjdk/samtools/reference/header_with_extra_white_space.fasta
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/htsjdk/samtools/reference/IndexedFastaSequenceFile.java
- src/test/resources/htsjdk/samtools/reference/header_with_extra_white_space.fasta
- src/main/java/htsjdk/samtools/reference/FastaSequenceIndexEntry.java
- src/test/java/htsjdk/samtools/reference/AbstractIndexedFastaSequenceFileTest.java
When opening an indexed FASTA, cheaply verify that the file is consistent with its index: the fasta must be at least as long as the last base position the index claims, and any bytes past the last base must be whitespace. This catches the common (and otherwise silent) failure where a stale or incorrect .fai causes the wrong bases to be returned. It reads only the file length and the trailing bytes rather than the whole file, so it is cheap to run on every open. Originally #1745 by @yfarjoun; rebased onto current master and reworked so that the index's last entry is derived on demand from the insertion-ordered entries (instead of a cached name that went stale for some index-construction paths and produced false positives), and so the trailing scan advances by the number of bytes actually read. Co-authored-by: Tim Fennell <tfenne@tfenne.com>
f683d18 to
42e2a29
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
The last position in the fasta file (according ot the index) must be close to the size of the fasta file itself. A mismatch could indicate a corrupt fasta file, or an in correct index.
(tests included)
Description
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Things to think about before submitting:
Summary by CodeRabbit