From 62388a214117ceb9152f549f5ea99da073f70b90 Mon Sep 17 00:00:00 2001 From: Tom White Date: Wed, 27 Feb 2019 15:49:07 +0000 Subject: [PATCH] A few fixes for issues found by spotbugs (#1278) * A few fixes for issues found by spotbugs * Make hashCode consistent with equals and add tests. --- .../encoding/readfeatures/Substitution.java | 9 ++++ .../cram/structure/CramCompressionRecord.java | 12 +++++ .../reference/FastaSequenceIndex.java | 6 +++ .../util/AsyncBlockCompressedInputStream.java | 2 +- .../java/htsjdk/samtools/util/CigarUtil.java | 2 +- .../readers/PositionalBufferedStream.java | 2 +- .../variantcontext/GenotypeLikelihoods.java | 6 +++ .../cram/encoding/ReadFeaturesTest.java | 29 ++++++++++++ .../structure/CramCompressionRecordTest.java | 45 +++++++++++++++++++ 9 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java b/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java index 1747c44749..44cd9dd24f 100644 --- a/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java +++ b/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java @@ -18,6 +18,7 @@ package htsjdk.samtools.cram.encoding.readfeatures; import java.io.Serializable; +import java.util.Objects; /** * A substitution event captured in read coordinates. It is characterized by position in read, read base and reference base. @@ -116,6 +117,14 @@ public boolean equals(final Object obj) { return true; } + @Override + public int hashCode() { + if (code == NO_CODE) { + return Objects.hash(position); + } + return Objects.hash(position, base, referenceBase); + } + @Override public String toString() { return String.valueOf((char) operator) + '@' + position + '\\' + (char) base + (char) referenceBase; diff --git a/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java b/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java index 32badca323..a39339f1ff 100644 --- a/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java +++ b/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java @@ -29,6 +29,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.Objects; public class CramCompressionRecord { private static final int MULTI_FRAGMENT_FLAG = 0x1; @@ -139,6 +140,17 @@ private boolean deepEquals(final Collection c1, final Collection c2) { return (c1 == null || c1.isEmpty()) && (c2 == null || c2.isEmpty()) || c1 != null && c1.equals(c2); } + @Override + public int hashCode() { + int result = Objects.hash(alignmentStart, readLength, recordsToNextFragment, mappingQuality, flags, readName); + if (readFeatures != null && !readFeatures.isEmpty()) { + result = 31 * result + Objects.hash(readFeatures); + } + result = 31 * result + Arrays.hashCode(readBases); + result = 31 * result + Arrays.hashCode(qualityScores); + return result; + } + @Override public String toString() { final StringBuilder stringBuilder = new StringBuilder("["); diff --git a/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java b/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java index b51a89f06c..735ab6347f 100644 --- a/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java +++ b/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java @@ -39,6 +39,7 @@ import java.util.Iterator; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; import java.util.Scanner; import java.util.regex.MatchResult; @@ -135,6 +136,11 @@ public boolean equals(Object other) { return true; } + @Override + public int hashCode() { + return Objects.hash(sequenceEntries); + } + /** * Parse the contents of an index file, caching the results internally. * @param in InputStream to parse. diff --git a/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java b/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java index 4f71ef5810..66b188b7f3 100644 --- a/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java +++ b/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java @@ -45,7 +45,7 @@ * Note that this implementation is not synchronized. If multiple threads access an instance concurrently, it must be synchronized externally. */ public class AsyncBlockCompressedInputStream extends BlockCompressedInputStream { - private static final int READ_AHEAD_BUFFERS = (int)Math.ceil(Defaults.NON_ZERO_BUFFER_SIZE / BlockCompressedStreamConstants.MAX_COMPRESSED_BLOCK_SIZE); + private static final int READ_AHEAD_BUFFERS = (int)Math.ceil((double) Defaults.NON_ZERO_BUFFER_SIZE / BlockCompressedStreamConstants.MAX_COMPRESSED_BLOCK_SIZE); private static final Executor threadpool = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(),new ThreadFactory() { @Override public Thread newThread(Runnable r) { diff --git a/src/main/java/htsjdk/samtools/util/CigarUtil.java b/src/main/java/htsjdk/samtools/util/CigarUtil.java index e6c14ab297..86347644b1 100644 --- a/src/main/java/htsjdk/samtools/util/CigarUtil.java +++ b/src/main/java/htsjdk/samtools/util/CigarUtil.java @@ -88,7 +88,7 @@ static private void elementStraddlesClippedRead(List newCigar, Cig final CigarOperator op = c.getOperator(); int clipAmount = clippedBases; if (op.consumesReadBases()){ - if (op.consumesReferenceBases() & relativeClippedPosition > 0){ + if (op.consumesReferenceBases() && relativeClippedPosition > 0){ newCigar.add(new CigarElement(relativeClippedPosition, op)); } if (!op.consumesReferenceBases()){ diff --git a/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java b/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java index 45eb202459..0b77f6aaa3 100644 --- a/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java +++ b/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java @@ -167,7 +167,7 @@ public final void close() { try { is.close(); } catch (IOException ex) { - new TribbleException("Failed to close PositionalBufferedStream", ex); + throw new TribbleException("Failed to close PositionalBufferedStream", ex); } } diff --git a/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java b/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java index ddaaf55e3e..e9518c6349 100644 --- a/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java +++ b/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java @@ -35,6 +35,7 @@ import java.util.List; import java.util.Map; import java.util.HashMap; +import java.util.Objects; public class GenotypeLikelihoods { private final static int NUM_LIKELIHOODS_CACHE_N_ALLELES = 5; @@ -158,6 +159,11 @@ public String getAsString() { return Arrays.equals(getAsPLs(), that.getAsPLs()); } + @Override + public int hashCode() { + return Arrays.hashCode(getAsPLs()); + } + //Return genotype likelihoods as an EnumMap with Genotypes as keys and likelihoods as values //Returns null in case of missing likelihoods public EnumMap getAsMap(boolean normalizeFromLog10){ diff --git a/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java b/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java index cb957546d4..1b30b5795b 100644 --- a/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java +++ b/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java @@ -4,9 +4,13 @@ import htsjdk.samtools.cram.encoding.readfeatures.Bases; import htsjdk.samtools.cram.encoding.readfeatures.Scores; import htsjdk.samtools.cram.encoding.readfeatures.SoftClip; +import htsjdk.samtools.cram.encoding.readfeatures.Substitution; import org.testng.Assert; import org.testng.annotations.Test; +import java.util.ArrayList; +import java.util.List; + public class ReadFeaturesTest extends HtsjdkTest { // equals() was incorrect for these classes @@ -25,4 +29,29 @@ public void faultyEquality() { final SoftClip sc2 = new SoftClip(0, new byte[] {}); Assert.assertEquals(sc1, sc2); } + + @Test + public void testSubstitutionEqualsAndHashCodeAreConsistent() { + final List substitutions = new ArrayList<>(); + for (int position : new int[] {0, 1}) { + for (byte base : new byte[] {(byte) -1, (byte) 'A'}) { + for (byte referenceBase : new byte[] {(byte) -1, (byte) 'C'}) { + for (byte code : new byte[] {Substitution.NO_CODE, (byte) 1, (byte) 2}) { + Substitution substitution = new Substitution(position, base, referenceBase); + substitution.setCode(code); + substitutions.add(substitution); + } + } + } + } + + for (Substitution s1 : substitutions) { + for (Substitution s2 : substitutions) { + if (s1.equals(s2)) { + Assert.assertEquals(s1.hashCode(), s2. hashCode(), + String.format("Comparing %s (%s) and %s (%s)", s1, s1.getCode(), s2, s2.getCode())); + } + } + } + } } diff --git a/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java b/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java index b212bbd756..d3b9ae6156 100644 --- a/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java +++ b/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java @@ -1,5 +1,7 @@ package htsjdk.samtools.cram.structure; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import htsjdk.HtsjdkTest; import htsjdk.samtools.SAMRecord; import htsjdk.samtools.cram.encoding.readfeatures.*; @@ -189,4 +191,47 @@ public void test_placementForAlignmentSpanAndEnd(final int sequenceId, Assert.assertEquals(r.getAlignmentEnd(usePositionDeltaEncoding), Slice.NO_ALIGNMENT_END); } } + + @Test + public void testEqualsAndHashCodeAreConsistent() { + final List records = new ArrayList<>(); + + final List features = new ArrayList<>(); + String softClip = "AAA"; + features.add(new SoftClip(1, softClip.getBytes())); + String insertion = "CCCCCCCCCC"; + features.add(new Insertion(1, insertion.getBytes())); + + for (int alignmentStart : new int[] {0, 1}) { + for (int readLength : new int[] {100, 101}) { + for (int flags : new int[] {0, 0x4}) { + for (List readFeatures : Lists.>newArrayList(null, new ArrayList<>(), features)) { + for (String readName : new String[] {null, "", "r"}) { + for (byte[] readBases : new byte[][]{null, new byte[]{(byte) 'A', (byte) 'C'}}) { + for (byte[] qualityScores : new byte[][]{null, new byte[]{(byte) 1, (byte) 2}}) { + final CramCompressionRecord r = new CramCompressionRecord(); + r.alignmentStart = alignmentStart; + r.readLength = readLength; + r.flags = flags; + r.readFeatures = readFeatures; + r.readName = readName; + r.readBases = readBases; + r.qualityScores = qualityScores; + records.add(r); + } + } + } + } + } + } + } + + for (CramCompressionRecord r1 : records) { + for (CramCompressionRecord r2 : records) { + if (r1.equals(r2)) { + Assert.assertEquals(r1.hashCode(), r2.hashCode(), String.format("Comparing %s and %s", r1, r2)); + } + } + } + } }