Changed SAMRecord.toString() to emit the SAM format string with all fields (#1762)

* Changed SAMRecord.toString() to emit the SAM format string with all fields.
* Drop sync + trailing newline from SAMRecord.getSAMString.
* Remove deprecated SAMRecord.format() and now-dead helpers.

The text formatting path used by SAMRecord.toString() / getSAMString() has been
cleaned up:

- SAMTextWriter.getSAMString no longer uses a JVM-wide synchronized static cache
  of a shared StringWriter/SAMTextWriter. It now allocates a fresh StringWriter
  per call. Without this change, every toString() call would have taken a global
  monitor.
- SAMTextWriter.writeAlignment is split into a private writeAlignmentNoNewline
  plus a thin wrapper that appends '\n'. getSAMString uses the no-newline
  variant, so the result no longer needs to be trimmed.

BREAKING CHANGE: SAMRecord.getSAMString() no longer terminates the returned
String with a newline character. This brings SAMRecord into line with the other
getSAMString() implementations (SAMSequenceRecord, SAMReadGroupRecord,
SAMProgramRecord, SAMFileHeader), which already return newline-free strings.
Callers that relied on the trailing '\n' as a separator (e.g. concatenating two
records' SAM strings) must now insert their own separator. Callers that stripped
the newline with .trim() can drop the call.

BREAKING CHANGE: SAMRecord.format() has been removed. Callers should use
SAMRecord.getSAMString() instead.

Author: tfenne

src/main/java/htsjdk/samtools/DuplicateSetIterator.java

@@ -148,7 +148,7 @@ public DuplicateSet next() {
 
                     if (0 < cmp) {
                         throw new SAMException("The input records were not sorted in duplicate order:\n" +
-                                representative.getSAMString() + record.getSAMString());
+                                representative.getSAMString() + "\n" + record.getSAMString());
                     } else if (cmp < 0) {
                         duplicateSet = this.duplicateSet;
                         this.duplicateSet = new DuplicateSet(this.comparator);

src/main/java/htsjdk/samtools/SAMRecord.java

@@ -1840,82 +1840,6 @@ public int getAttributesBinarySize() {
         return -1;
     }
 
-    /**
-     *
-     * @return String representation of this.
-     * @deprecated This method is not guaranteed to return a valid SAM text representation of the SAMRecord.
-     * To get standard SAM text representation, {@link SAMRecord#getSAMString}.
-     */
-    @Deprecated
-    public String format() {
-        final StringBuilder buffer = new StringBuilder();
-        addField(buffer, getReadName(), null, null);
-        addField(buffer, getFlags(), null, null);
-        addField(buffer, getReferenceName(), null, "*");
-        addField(buffer, getAlignmentStart(), 0, "*");
-        addField(buffer, getMappingQuality(), 0, "0");
-        addField(buffer, getCigarString(), null, "*");
-        addField(buffer, getMateReferenceName(), null, "*");
-        addField(buffer, getMateAlignmentStart(), 0, "*");
-        addField(buffer, getInferredInsertSize(), 0, "*");
-        addField(buffer, getReadString(), null, "*");
-        addField(buffer, getBaseQualityString(), null, "*");
-        if (mAttributes != null) {
-            SAMBinaryTagAndValue entry = getBinaryAttributes();
-            while (entry != null) {
-                addField(buffer, formatTagValue(entry.tag, entry.value));
-                entry = entry.getNext();
-            }
-        }
-        return buffer.toString();
-    }
-
-    private void addField(final StringBuilder buffer, final Object value, final Object defaultValue, final String defaultString) {
-        if (safeEquals(value, defaultValue)) {
-            addField(buffer, defaultString);
-        } else if (value == null) {
-            addField(buffer, "");
-        } else {
-            addField(buffer, value.toString());
-        }
-    }
-
-    private void addField(final StringBuilder buffer, final String field) {
-        if (buffer.length() > 0) {
-            buffer.append('\t');
-        }
-        buffer.append(field);
-    }
-
-    private static String formatTagValue(final short tag, final Object value) {
-        final String tagString = SAMTag.makeStringTag(tag);
-        if (value == null || value instanceof String) {
-            return tagString + ":Z:" + value;
-        } else if (value instanceof Integer || value instanceof Long ||
-                value instanceof Short || value instanceof Byte) {
-            return tagString + ":i:" + value;
-        } else if (value instanceof Character) {
-            return tagString + ":A:" + value;
-        } else if (value instanceof Float) {
-            return tagString + ":f:" + value;
-        } else if (value instanceof byte[]) {
-            return tagString + ":H:" + StringUtil.bytesToHexString((byte[]) value);
-        } else {
-            throw new RuntimeException("Unexpected value type for tag " + tagString +
-                    ": " + value + " of class " + value.getClass().getName());
-        }
-    }
-
-    private boolean safeEquals(final Object o1, final Object o2) {
-        if (o1 == o2) {
-            return true;
-        } else if (o1 == null || o2 == null) {
-            return false;
-        } else {
-            return o1.equals(o2);
-        }
-    }
-
     /**
      * Force all lazily-initialized data members to be initialized.  If a subclass overrides this method,
      * typically it should also call  super method.
@@ -2386,37 +2310,15 @@ public SAMRecord deepCopy() {
         return newSAM;
     }
 
-    /** Simple toString() that gives a little bit of useful info about the read. */
+    /** Returns this record formatted as it would appear in a SAM file. */
     @Override
     public String toString() {
-        final StringBuilder builder = new StringBuilder(64);
-        builder.append(getReadName());
-        if (getReadPairedFlag()) {
-            if (getFirstOfPairFlag()) {
-                builder.append(" 1/2");
-            }
-            else {
-                builder.append(" 2/2");
-            }
-        }
-
-        builder.append(' ')
-                .append(String.valueOf(getReadLength()))
-                .append('b');
-
-        if (getReadUnmappedFlag()) {
-            builder.append(" unmapped read.");
-        }
-        else {
-            builder.append(String.format(" aligned to %s:%d-%d.", getContig(), getAlignmentStart(), getAlignmentEnd()));
-        }
-
-        return builder.toString();
+        return getSAMString();
     }
 
     /**
-     Returns the record in the SAM line-based text format.  Fields are
-     separated by '\t' characters, and the String is terminated by '\n'.
+     Returns the record in the SAM line-based text format.  Fields are separated by
+     '\t' characters. The returned String is NOT terminated by a newline.
      */
     public String getSAMString() {
         return SAMTextWriter.getSAMString(this);

src/main/java/htsjdk/samtools/SAMSortOrderChecker.java

@@ -72,7 +72,7 @@ public String getSortKey(final SAMRecord rec) {
             case queryname:
                 return rec.getReadName();
             default:
-                return rec.getSAMString().trim();
+                return rec.getSAMString();
         }
     }
 }

src/main/java/htsjdk/samtools/SAMTextWriter.java

@@ -123,6 +123,16 @@ public SAMTextWriter(final OutputStream stream, final SamFlagField samFlagFieldO
      */
     @Override
     public void writeAlignment(final SAMRecord alignment) {
+        writeAlignmentNoNewline(alignment);
+        try {
+            out.write("\n");
+        } catch (final IOException e) {
+            throw new RuntimeIOException(e);
+        }
+    }
+
+    /** Writes the alignment fields without a trailing newline. Used by {@link #getSAMString}. */
+    private void writeAlignmentNoNewline(final SAMRecord alignment) {
         try {
             out.write(alignment.getReadName());
             out.write(FIELD_SEPARATOR);
@@ -164,21 +174,15 @@ public void writeAlignment(final SAMRecord alignment) {
                 out.write(encodedTag);
                 attribute = attribute.getNext();
             }
-            out.write("\n");
-
         } catch (final IOException e) {
             throw new RuntimeIOException(e);
         }
     }
 
     /* This method is called by SAMRecord.getSAMString(). */
-    private static SAMTextWriter textWriter = null;
-    private static StringWriter stringWriter = null;
-    static synchronized String getSAMString(final SAMRecord alignment) {
-        if (stringWriter == null) stringWriter = new StringWriter();
-        if (textWriter == null) textWriter = new SAMTextWriter(stringWriter);
-        stringWriter.getBuffer().setLength(0);
-        textWriter.writeAlignment(alignment);
+    static String getSAMString(final SAMRecord alignment) {
+        final StringWriter stringWriter = new StringWriter();
+        new SAMTextWriter(stringWriter).writeAlignmentNoNewline(alignment);
         return stringWriter.toString();
     }
 

src/main/java/htsjdk/samtools/SamReader.java

@@ -594,8 +594,8 @@ public SAMRecord next() {
                 if (!checker.isSorted(result)) {
                     throw new IllegalStateException(String.format(
                             "Record %s should come after %s when sorting with %s ordering.",
-                            previous.getSAMString().trim(),
-                            result.getSAMString().trim(), checker.getSortOrder()));
+                            previous.getSAMString(),
+                            result.getSAMString(), checker.getSortOrder()));
                 }
             }
             return result;

src/main/java/htsjdk/samtools/cram/CramComparison.java

@@ -131,8 +131,8 @@ private static int compareFiles(final SamReaderFactory factory, final String fil
                     diffCount++;
                     if (diffCount <= maxDiffs) {
                         emit(out, "Record %d: %s", recordCount, diff);
-                        emit(out, "  file1: %s", rec1.getSAMString().trim());
-                        emit(out, "  file2: %s", rec2.getSAMString().trim());
+                        emit(out, "  file1: %s", rec1.getSAMString());
+                        emit(out, "  file2: %s", rec2.getSAMString());
                     }
                 }
             }

src/main/java/htsjdk/samtools/sra/SRALazyRecord.java

@@ -725,14 +725,6 @@ public Object clone() throws CloneNotSupportedException {
         return newObject;
     }
 
-    @Override
-    public String format() {
-        if (!initializedAttributes.contains(LazyAttribute.RG)) {
-            getAttribute("RG");
-        }
-        return super.format();
-    }
-
     @Override
     public List<SAMValidationError> isValid(final boolean firstOnly) {
         loadFields();

src/test/java/htsjdk/samtools/BAMFileIndexTest.java

@@ -418,9 +418,6 @@ private int runQueryTest(final File bamFile, final String sequence, final int st
                 record2 = iter2.next();
                 count2++;
             }
-            // System.out.println("Iteration:");
-            // System.out.println(" Record1 = " + ((record1 == null) ? "null" : record1.format()));
-            // System.out.println(" Record2 = " + ((record2 == null) ? "null" : record2.format()));
             if (record1 == null && record2 == null) {
                 break;
             }

src/test/java/htsjdk/samtools/BAMRemoteFileTest.java

@@ -169,7 +169,6 @@ private void runLocalRemoteTest(final URL bamURL, final File bamFile, final Stri
         assertTrue(records1.size() > 0);
         assertEquals(records1.size(), records2.size());
         for (int i = 0; i < records1.size(); i++) {
-            //System.out.println(records1.get(i).format());
             assertEquals(records1.get(i).getSAMString(), records2.get(i).getSAMString());
         }
     }

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

@@ -1205,4 +1205,48 @@ public void testAlignmentBlockCacheInvalidation() {
         rec.setAlignmentStart(100);
         Assert.assertEquals(100, rec.getAlignmentBlocks().get(0).getReferenceStart());
     }
+
+    @Test
+    public void testToStringReturnsSamLine() {
+        final SAMRecord rec = createTestRecordHelper();
+        final String[] fields = rec.toString().split("\t", -1);
+        Assert.assertTrue(fields.length >= 11, "Expected at least 11 mandatory SAM fields, got " + fields.length);
+        Assert.assertEquals(fields[0], rec.getReadName());
+        Assert.assertEquals(fields[1], Integer.toString(rec.getFlags()));
+        Assert.assertEquals(fields[2], rec.getReferenceName());
+        Assert.assertEquals(fields[3], Integer.toString(rec.getAlignmentStart()));
+        Assert.assertEquals(fields[5], rec.getCigarString());
+    }
+
+    @Test
+    public void testToStringHasNoTrailingNewline() {
+        final SAMRecord rec = createTestRecordHelper();
+        final String s = rec.toString();
+        Assert.assertFalse(s.isEmpty());
+        Assert.assertNotEquals(s.charAt(s.length() - 1), '\n');
+    }
+
+    @Test
+    public void testGetSAMStringHasNoTrailingNewline() {
+        final SAMRecord rec = createTestRecordHelper();
+        final String s = rec.getSAMString();
+        Assert.assertFalse(s.isEmpty());
+        Assert.assertNotEquals(s.charAt(s.length() - 1), '\n');
+    }
+
+    @Test
+    public void testToStringEqualsGetSAMString() {
+        final SAMRecord rec = createTestRecordHelper();
+        Assert.assertEquals(rec.toString(), rec.getSAMString());
+    }
+
+    @Test
+    public void testToStringIncludesAttributes() {
+        final SAMRecord rec = createTestRecordHelper();
+        rec.setAttribute("XX", 42);
+        rec.setAttribute("YY", "hello");
+        final String s = rec.toString();
+        Assert.assertTrue(s.contains("XX:i:42"), "Expected XX:i:42 in: " + s);
+        Assert.assertTrue(s.contains("YY:Z:hello"), "Expected YY:Z:hello in: " + s);
+    }
 }