Keep a BAM record's other tags when one of its tags is repeated

CHANGELOG.md

@@ -10,6 +10,16 @@ early infrastructure for a plugin-based codec framework and resource bundles.
 
 ---
 
+## 6.0.1
+
+Patch release on the 6.0 line.
+
+### Bug fixes
+
+- **A BAM record with a repeated tag keeps its other tags** ([#1946](https://github.com/samtools/htsjdk/issues/1946)).  When a repeated tag sorted highest of the tags decoded so far, as in `XA:A:a XA:A:b XB:A:c`, every later tag that sorted above it was silently dropped.  The later value of the repeated tag is still the one kept, as in SAM text.
+
+---
+
 ## 6.0.0
 
 Major release.  It includes every change in 5.0.2.  Before upgrading, read [Upgrading from 5.x](#upgrading-from-5x); to explain differences from files 5.x wrote, see [Output that differs from 5.x](#output-that-differs-from-5x).

src/main/java/htsjdk/samtools/BinaryTagCodec.java

@@ -306,6 +306,8 @@ public static SAMBinaryTagAndValue readTags(
                 tail = tmp;
             } else {
                 head = head.insert(tmp);
+                // A repeat of the last tag replaces it, so the replacement is the new tail
+                if (tmp.next == null) tail = tmp;
             }
         }
 

src/test/java/htsjdk/samtools/BinaryTagCodecTest.java

@@ -1,6 +1,8 @@
 package htsjdk.samtools;
 
 import htsjdk.HtsjdkTest;
+import java.util.LinkedHashMap;
+import java.util.Map;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
@@ -13,4 +15,39 @@ public void testCharTagAbove7FIsReadAsTheCharItWasWrittenFrom() {
         final SAMBinaryTagAndValue decoded = BinaryTagCodec.readTags(tag, 0, tag.length, ValidationStringency.STRICT);
         Assert.assertEquals(decoded.value, 'é');
     }
+
+    // A record may carry a tag twice. As in SAM text, the later value is kept, and the record's other tags must be too.
+
+    /** Decodes a run of A-type tags, each given as its two-letter name followed by its one-character value. */
+    private static Map<String, Object> decodeCharTags(final String... tagsAndValues) {
+        final byte[] bytes = new byte[tagsAndValues.length * 4];
+        for (int i = 0; i < tagsAndValues.length; i++) {
+            bytes[i * 4] = (byte) tagsAndValues[i].charAt(0);
+            bytes[i * 4 + 1] = (byte) tagsAndValues[i].charAt(1);
+            bytes[i * 4 + 2] = 'A';
+            bytes[i * 4 + 3] = (byte) tagsAndValues[i].charAt(2);
+        }
+        final Map<String, Object> decoded = new LinkedHashMap<>();
+        for (SAMBinaryTagAndValue tag = BinaryTagCodec.readTags(bytes, 0, bytes.length, ValidationStringency.STRICT);
+                tag != null;
+                tag = tag.getNext()) {
+            decoded.put(SAMTag.makeStringTag(tag.tag), tag.value);
+        }
+        return decoded;
+    }
+
+    @Test
+    public void testTagsAfterADuplicateOfTheLastTagAreKept() {
+        Assert.assertEquals(decodeCharTags("XAa", "XAb", "XBc"), Map.of("XA", 'b', "XB", 'c'));
+    }
+
+    @Test
+    public void testTagsAfterADuplicateOfTheLastTagAreKeptWhenTagsAreOutOfOrder() {
+        Assert.assertEquals(decodeCharTags("XBb", "XAa", "XBc", "XCd"), Map.of("XA", 'a', "XB", 'c', "XC", 'd'));
+    }
+
+    @Test
+    public void testADuplicateOfAnEarlierTagKeepsTheLaterValue() {
+        Assert.assertEquals(decodeCharTags("XAa", "XCc", "XAb", "XDd"), Map.of("XA", 'b', "XC", 'c', "XD", 'd'));
+    }
 }