Fixes for issues found by SpotBugs

src/main/java/htsjdk/samtools/CRAMIterator.java

@@ -41,7 +41,6 @@ public class CRAMIterator implements SAMRecordIterator {
     private final CountingInputStream countingInputStream;
     private final CramHeader cramHeader;
     private final ArrayList<SAMRecord> records;
-    private SAMRecord nextRecord = null;
     private final CramNormalizer normalizer;
     private byte[] refs;
     private int prevSeqId = SAMRecord.NO_ALIGNMENT_REFERENCE_INDEX;
@@ -131,20 +130,17 @@ void nextContainer() throws IllegalArgumentException, CRAMException {
         if (containerIterator != null) {
             if (!containerIterator.hasNext()) {
                 records.clear();
-                nextRecord = null;
                 return;
             }
             container = containerIterator.next();
             if (container.isEOF()) {
                 records.clear();
-                nextRecord = null;
                 return;
             }
         } else {
             container = ContainerIO.readContainer(cramHeader.getVersion(), countingInputStream);
             if (container.isEOF()) {
                 records.clear();
-                nextRecord = null;
                 return;
             }
         }
@@ -209,10 +205,9 @@ void nextContainer() throws IllegalArgumentException, CRAMException {
             if (mReader != null) {
                 final long chunkStart = (container.offset << 16) | cramRecord.sliceIndex;
                 final long chunkEnd = ((container.offset << 16) | cramRecord.sliceIndex) + 1;
-                nextRecord.setFileSource(new SAMFileSource(mReader,
-                        new BAMFileSpan(new Chunk(chunkStart, chunkEnd))));
+                samRecord.setFileSource(new SAMFileSource(mReader, new BAMFileSpan(new Chunk(chunkStart, chunkEnd))));
             }
-
+            
             records.add(samRecord);
         }
         cramRecords.clear();

src/main/java/htsjdk/samtools/cram/structure/CramHeader.java

@@ -70,22 +70,6 @@ public void setID(final String stringID) {
         System.arraycopy(stringID.getBytes(), 0, this.id, 0, Math.min(this.id.length, stringID.length()));
     }
 
-    /**
-     * Copy the CRAM header into a new {@link CramHeader} object.
-     * @return a complete copy of the header
-     */
-    @SuppressWarnings("CloneDoesntCallSuperClone")
-    @Override
-    public CramHeader clone() {
-        final CramHeader clone = new CramHeader();
-        clone.version = version;
-        System.arraycopy(id, 0, clone.id, 0, id.length);
-        clone.samFileHeader = getSamFileHeader().clone();
-
-        return clone;
-    }
-
-
     /**
      * Get the {@link SAMFileHeader} object associated with this CRAM file header.
      * @return the SAM file header

src/main/java/htsjdk/samtools/cram/structure/Slice.java

@@ -29,7 +29,6 @@
 import htsjdk.samtools.cram.io.DefaultBitInputStream;
 import htsjdk.samtools.cram.io.DefaultBitOutputStream;
 import htsjdk.samtools.cram.structure.block.Block;
-import htsjdk.samtools.cram.structure.block.ExternalBlock;
 import htsjdk.samtools.util.Log;
 import htsjdk.samtools.util.RuntimeIOException;
 import htsjdk.samtools.util.SequenceUtil;
@@ -449,7 +448,7 @@ else if (slice.isMappedSingleRef()) {
 
             final ExternalCompressor compressor = header.externalCompressors.get(contentId);
             final byte[] rawContent = externalBlockMap.get(contentId).toByteArray();
-            final ExternalBlock externalBlock = new ExternalBlock(compressor.getMethod(), contentId,
+            final Block externalBlock = Block.createExternalBlock(compressor.getMethod(), contentId,
                     compressor.compress(rawContent), rawContent.length);
 
             slice.external.put(contentId, externalBlock);

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

@@ -51,6 +51,11 @@ public class Block {
      */
     private final BlockContentType contentType;
 
+    /**
+     * The External Block Content ID, or NO_CONTENT_ID for non-External block
+     */
+    private final int contentId;
+
     /**
      * The content stored in this block, in compressed form (if applicable)
      */
@@ -66,15 +71,18 @@ public class Block {
      *
      * @param compressionMethod the block compression method.  Can be RAW, if uncompressed
      * @param contentType whether this is a header or data block, and which kind
+     * @param contentId the External Block Content ID, or NO_CONTENT_ID for non-External block
      * @param compressedContent the compressed form of the data to be stored in this block
      * @param uncompressedLength the length of the content stored in this block when uncompressed
      */
     protected Block(final BlockCompressionMethod compressionMethod,
                     final BlockContentType contentType,
+                    final int contentId,
                     final byte[] compressedContent,
                     final int uncompressedLength) {
         this.compressionMethod = compressionMethod;
         this.contentType = contentType;
+        this.contentId = contentId;
         this.compressedContent = compressedContent;
         this.uncompressedLength = uncompressedLength;
 
@@ -83,21 +91,24 @@ protected Block(final BlockCompressionMethod compressionMethod,
 //            throw new CRAMException("Valid Content ID required for external blocks.");
 //        }
 
-        if (contentType != BlockContentType.EXTERNAL && getContentId() != Block.NO_CONTENT_ID) {
+        if (contentType != BlockContentType.EXTERNAL && contentId != Block.NO_CONTENT_ID) {
             throw new CRAMException("Cannot set a Content ID for non-external blocks.");
         }
     }
 
     /**
-     * Protected constructor of a generic Block, to be called by static factory methods and subclasses.
-     * Creates RAW blocks only.
+     * Create a new non-external block with the given uncompressed content.
+     * The block will have RAW (no) compression.
      *
      * @param contentType whether this is a header or data block, and which kind
      * @param rawContent the raw data to be stored in this block
      */
-    protected Block(final BlockContentType contentType,
-                    final byte[] rawContent) {
-        this(BlockCompressionMethod.RAW, contentType, rawContent, rawContent.length);
+    private static Block createRawNonExternalBlock(final BlockContentType contentType, final byte[] rawContent) {
+        if (contentType == BlockContentType.EXTERNAL) {
+            throw new CRAMException("Code error: cannot use the non-external factory method for EXTERNAL blocks.");
+        }
+
+        return new Block(BlockCompressionMethod.RAW, contentType, NO_CONTENT_ID, rawContent, rawContent.length);
     }
 
     /**
@@ -108,7 +119,7 @@ protected Block(final BlockContentType contentType,
      * @return a new {@link Block} object
      */
     public static Block createRawFileHeaderBlock(final byte[] rawContent) {
-        return new Block(BlockContentType.FILE_HEADER, rawContent);
+        return createRawNonExternalBlock(BlockContentType.FILE_HEADER, rawContent);
     }
 
     /**
@@ -119,7 +130,7 @@ public static Block createRawFileHeaderBlock(final byte[] rawContent) {
      * @return a new {@link Block} object
      */
     public static Block createRawCompressionHeaderBlock(final byte[] rawContent) {
-        return new Block(BlockContentType.COMPRESSION_HEADER, rawContent);
+        return createRawNonExternalBlock(BlockContentType.COMPRESSION_HEADER, rawContent);
     }
 
     /**
@@ -130,7 +141,7 @@ public static Block createRawCompressionHeaderBlock(final byte[] rawContent) {
      * @return a new {@link Block} object
      */
     public static Block createRawSliceHeaderBlock(final byte[] rawContent) {
-        return new Block(BlockContentType.MAPPED_SLICE, rawContent);
+        return createRawNonExternalBlock(BlockContentType.MAPPED_SLICE, rawContent);
     }
 
     /**
@@ -141,7 +152,24 @@ public static Block createRawSliceHeaderBlock(final byte[] rawContent) {
      * @return a new {@link Block} object
      */
     public static Block createRawCoreDataBlock(final byte[] rawContent) {
-        return new Block(BlockContentType.CORE, rawContent);
+        return createRawNonExternalBlock(BlockContentType.CORE, rawContent);
+    }
+
+    /**
+     * Create a new external data block with the given compression method, uncompressed content, and content ID.
+     * The block will have EXTERNAL content type.
+     *
+     * @param compressionMethod the compression method used in this block
+     * @param contentId the external identifier for the block
+     * @param compressedContent the content of this block, in compressed mode
+     * @param uncompressedLength the length of the content stored in this block when uncompressed
+     */
+    public static Block createExternalBlock(final BlockCompressionMethod compressionMethod,
+                                            final int contentId,
+                                            final byte[] compressedContent,
+                                            final int uncompressedLength) {
+        return new Block(compressionMethod, BlockContentType.EXTERNAL,
+                contentId, compressedContent, uncompressedLength);
     }
 
     public final BlockCompressionMethod getCompressionMethod() {
@@ -157,14 +185,12 @@ public final BlockContentType getContentType() {
     }
 
     /**
-     * Return the External Content ID for this block.
-     *
-     * Only ExternalBlocks have a meaningful Content ID, so that class overrides this method.
+     * Return the External Content ID for this block.  Only ExternalBlocks have a meaningful Content ID.
      *
      * @return the External Content ID, or NO_CONTENT_ID
      */
     public int getContentId() {
-        return NO_CONTENT_ID;
+        return contentId;
     }
 
     /**
@@ -232,11 +258,7 @@ public static Block read(final int major, InputStream inputStream) {
                 }
             }
 
-            if (contentType == BlockContentType.EXTERNAL) {
-                return new ExternalBlock(compressionMethod, contentId, compressedContent, uncompressedSize);
-            } else {
-                return new Block(compressionMethod, contentType, compressedContent, uncompressedSize);
-            }
+            return new Block(compressionMethod, contentType, contentId, compressedContent, uncompressedSize);
         }
         catch (final IOException e) {
             throw new RuntimeIOException(e);

src/test/java/htsjdk/samtools/cram/structure/BlockTest.java → src/test/java/htsjdk/samtools/cram/structure/block/BlockTest.java

@@ -1,10 +1,10 @@
-package htsjdk.samtools.cram.structure;
+package htsjdk.samtools.cram.structure.block;
 
 import htsjdk.HtsjdkTest;
+import htsjdk.samtools.cram.CRAMException;
 import htsjdk.samtools.cram.common.CramVersions;
 import htsjdk.samtools.cram.common.Version;
 import htsjdk.samtools.cram.compression.ExternalCompressor;
-import htsjdk.samtools.cram.structure.block.*;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -102,12 +102,39 @@ public void testExternalBlockRoundTrips() throws IOException {
         final byte[] uncompressedData = "A TEST STRING WITH REDUNDANCY AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA".getBytes();
         final byte[] compressedData = compressor.compress(uncompressedData);
 
-        final Block extBlock = new ExternalBlock(compressor.getMethod(), contentID, compressedData, uncompressedData.length);
+        final Block extBlock = Block.createExternalBlock(compressor.getMethod(), contentID, compressedData, uncompressedData.length);
 
         final Block rtBlock2 = roundTrip(extBlock, CramVersions.CRAM_v2_1);
         contentCheck(rtBlock2, uncompressedData, compressedData);
 
         final Block rtBlock3 = roundTrip(extBlock, CramVersions.CRAM_v3);
         contentCheck(rtBlock3, uncompressedData, compressedData);
     }
+
+    @DataProvider(name = "nonExternalTypes")
+    private Object[][] nonExternalTypes() {
+        return new Object[][] {
+                {BlockContentType.COMPRESSION_HEADER},
+                {BlockContentType.CORE},
+                {BlockContentType.MAPPED_SLICE},
+                {BlockContentType.FILE_HEADER},
+                {BlockContentType.RESERVED},
+        };
+    }
+
+    // show that we can build all non-external Block types with NO_CONTENT_ID
+    // but we can't if they have other values
+
+    @Test(dataProvider = "nonExternalTypes")
+    public void nonExternalContentId(final BlockContentType contentType) {
+        new Block(BlockCompressionMethod.RAW, contentType, Block.NO_CONTENT_ID, new byte[0], 0);
+    }
+
+    // show that non-external content types cannot have valid contentIds
+
+    @Test(dataProvider = "nonExternalTypes", expectedExceptions = CRAMException.class)
+    public void nonExternalContentIdNegative(final BlockContentType contentType) {
+        final int VALID_CONTENT_ID = 1;
+        new Block(BlockCompressionMethod.RAW, contentType, VALID_CONTENT_ID, new byte[0], 0);
+    }
 }