PR https://github.com/samtools/htsjdk/pull/1194 removed a number of Array.getLength(value) == 0 checks when setting B-array tagged fields, as such empty tagged field values had recently been made explicitly valid in SAM/BAM/CRAM files.
Unfortunately the existing such check in setUnsignedArrayAttribute() was omitted from that PR and has now caused the issue raised in https://github.com/samtools/hts-specs/issues/728. Whatever additional problems might (but probably don't) exist elsewhere in the HTSJDK code for zero-length ML tags, this check for a generic zero-length array was incorrect and should be removed. A test case for setUnsignedArrayAttribute() has also been added.
In grepping for other candidates, I noticed one other potentially incorrect looking Array.getLength invocation.
This was in Slice::setAttribute(), which appears to set an optional tag in a Slice header. There are currently no such tags defined, so the question is somewhat moot but — as a separate commit for your consideration — I have also removed that check. (As there are no such tags currently defined, there are no test cases for these fields to which a test exercising this change could be added.)
--
Updated jmarshall pull request to remove unused Slice::setAttribute() methods.
PR https://github.com/samtools/htsjdk/pull/1194 removed a number of Array.getLength(value) == 0 checks when setting B-array tagged fields, as such empty tagged field values had recently been made explicitly valid in SAM/BAM/CRAM files.
Unfortunately the existing such check in setUnsignedArrayAttribute() was omitted from that PR and has now caused the issue raised in https://github.com/samtools/hts-specs/issues/728. Whatever additional problems might (but probably don't) exist elsewhere in the HTSJDK code for zero-length ML tags, this check for a generic zero-length array was incorrect and should be removed. A test case for setUnsignedArrayAttribute() has also been added.
In grepping for other candidates, I noticed one other potentially incorrect looking Array.getLength invocation.
This was in Slice::setAttribute(), which appears to set an optional tag in a Slice header. There are currently no such tags defined, so the question is somewhat moot but — as a separate commit for your consideration — I have also removed that check. (As there are no such tags currently defined, there are no test cases for these fields to which a test exercising this change could be added.)
--
Updated jmarshall pull request to remove unused Slice::setAttribute() methods.