From cf553f6110cfc236aceaabd17fc4dba700990b02 Mon Sep 17 00:00:00 2001 From: Yossi Farjoun Date: Thu, 1 Aug 2019 15:20:55 -0400 Subject: [PATCH 1/6] - fix bug in VariantContextBuilder - clean up braces while I'm at it. --- .../variantcontext/VariantContextBuilder.java | 6 +- .../java/htsjdk/variant/vcf/VCFEncoder.java | 146 +++++++++++------- .../VariantContextBuilderTest.java | 12 ++ 3 files changed, 107 insertions(+), 57 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java index 182b5fb9e4..57391c4c40 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java @@ -34,7 +34,6 @@ import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -354,7 +353,7 @@ private void makeFiltersModifiable() { if (!filtersCanBeModified) { this.filtersCanBeModified = true; final Set tempFilters = filters; - this.filters = new HashSet<>(); + this.filters = new LinkedHashSet<>(); if (tempFilters != null) { this.filters.addAll(tempFilters); } @@ -376,7 +375,7 @@ public VariantContextBuilder filters(final Set filters) { unfiltered(); } else { this.filtersCanBeModified = true; - filtersAsIs(new HashSet<>(filters)); + filtersAsIs(new LinkedHashSet<>(filters)); } return this; } @@ -435,6 +434,7 @@ public VariantContextBuilder passFilters() { */ public VariantContextBuilder unfiltered() { this.filters = null; + this.filtersCanBeModified = false; return this; } diff --git a/src/main/java/htsjdk/variant/vcf/VCFEncoder.java b/src/main/java/htsjdk/variant/vcf/VCFEncoder.java index 00d80c4791..d52f2d5c02 100644 --- a/src/main/java/htsjdk/variant/vcf/VCFEncoder.java +++ b/src/main/java/htsjdk/variant/vcf/VCFEncoder.java @@ -44,7 +44,9 @@ public class VCFEncoder { * allowing missing fields in the header. */ public VCFEncoder(final VCFHeader header, final boolean allowMissingFieldsInHeader, final boolean outputTrailingFormatFields) { - if (header == null) throw new NullPointerException("The VCF header must not be null."); + if (header == null) { + throw new NullPointerException("The VCF header must not be null."); + } this.header = header; this.allowMissingFieldsInHeader = allowMissingFieldsInHeader; this.outputTrailingFormatFields = outputTrailingFormatFields; @@ -128,8 +130,11 @@ public void write(final Appendable vcfOutput, final VariantContext context) thro vcfOutput.append(VCFConstants.FIELD_SEPARATOR); // QUAL - if ( ! context.hasLog10PError()) vcfOutput.append(VCFConstants.MISSING_VALUE_v4); - else vcfOutput.append(formatQualValue(context.getPhredScaledQual())); + if ( !context.hasLog10PError()) { + vcfOutput.append(VCFConstants.MISSING_VALUE_v4); + } else { + vcfOutput.append(formatQualValue(context.getPhredScaledQual())); + } vcfOutput.append(VCFConstants.FIELD_SEPARATOR) // FILTER .append(getFilterString(context)).append(VCFConstants.FIELD_SEPARATOR); @@ -137,11 +142,14 @@ public void write(final Appendable vcfOutput, final VariantContext context) thro // INFO final Map infoFields = new TreeMap<>(); for (final Map.Entry field : context.getAttributes().entrySet()) { - if (!this.header.hasInfoLine(field.getKey())) + if (!this.header.hasInfoLine(field.getKey())) { fieldIsMissingFromHeaderError(context, field.getKey(), "INFO"); + } final String outputValue = formatVCFField(field.getValue()); - if (outputValue != null) infoFields.put(field.getKey(), outputValue); + if (outputValue != null) { + infoFields.put(field.getKey(), outputValue); + } } writeInfoString(infoFields, vcfOutput); @@ -152,11 +160,12 @@ public void write(final Appendable vcfOutput, final VariantContext context) thro vcfOutput.append(((LazyGenotypesContext) gc).getUnparsedGenotypeData().toString()); } else { final List genotypeAttributeKeys = context.calcVCFGenotypeKeys(this.header); - if ( ! genotypeAttributeKeys.isEmpty()) { - for (final String format : genotypeAttributeKeys) - if ( ! this.header.hasFormatLine(format)) + if ( !genotypeAttributeKeys.isEmpty()) { + for (final String format : genotypeAttributeKeys) { + if (!this.header.hasFormatLine(format)) { fieldIsMissingFromHeaderError(context, format, "FORMAT"); - + } + } final String genotypeFormatString = ParsingUtils.join(VCFConstants.GENOTYPE_FIELD_SEPARATOR, genotypeAttributeKeys); vcfOutput.append(VCFConstants.FIELD_SEPARATOR); @@ -166,8 +175,6 @@ public void write(final Appendable vcfOutput, final VariantContext context) thro appendGenotypeData(context, alleleStrings, genotypeAttributeKeys, vcfOutput); } } - - } VCFHeader getVCFHeader() { @@ -181,52 +188,71 @@ boolean getAllowMissingFieldsInHeader() { private String getFilterString(final VariantContext vc) { if (vc.isFiltered()) { for (final String filter : vc.getFilters()) { - if (!this.header.hasFilterLine(filter)) fieldIsMissingFromHeaderError(vc, filter, "FILTER"); + if (!this.header.hasFilterLine(filter)) { + fieldIsMissingFromHeaderError(vc, filter, "FILTER"); + } } return ParsingUtils.join(";", ParsingUtils.sortList(vc.getFilters())); - } else if (vc.filtersWereApplied()) return VCFConstants.PASSES_FILTERS_v4; - else return VCFConstants.UNFILTERED; + } else { + if (vc.filtersWereApplied()){ + return VCFConstants.PASSES_FILTERS_v4; + } else { + return VCFConstants.UNFILTERED; + } + } } private static String formatQualValue(final double qual) { String s = String.format(QUAL_FORMAT_STRING, qual); - if (s.endsWith(QUAL_FORMAT_EXTENSION_TO_TRIM)) + if (s.endsWith(QUAL_FORMAT_EXTENSION_TO_TRIM)) { s = s.substring(0, s.length() - QUAL_FORMAT_EXTENSION_TO_TRIM.length()); + } return s; } private void fieldIsMissingFromHeaderError(final VariantContext vc, final String id, final String field) { - if (!allowMissingFieldsInHeader) + if (!allowMissingFieldsInHeader) { throw new IllegalStateException("Key " + id + " found in VariantContext field " + field - + " at " + vc.getContig() + ":" + vc.getStart() - + " but this key isn't defined in the VCFHeader. We require all VCFs to have" - + " complete VCF headers by default."); + + " at " + vc.getContig() + ":" + vc.getStart() + + " but this key isn't defined in the VCFHeader. We require all VCFs to have" + + " complete VCF headers by default."); + } } @SuppressWarnings("rawtypes") String formatVCFField(final Object val) { final String result; - if ( val == null ) + if (val == null) { result = VCFConstants.MISSING_VALUE_v4; - else if ( val instanceof Double ) - result = formatVCFDouble((Double) val); - else if ( val instanceof Boolean ) - result = (Boolean)val ? "" : null; // empty string for true, null for false - else if ( val instanceof List ) { - result = formatVCFField(((List)val).toArray()); - } else if ( val.getClass().isArray() ) { - final int length = Array.getLength(val); - if ( length == 0 ) - return formatVCFField(null); - final StringBuilder sb = new StringBuilder(formatVCFField(Array.get(val, 0))); - for ( int i = 1; i < length; i++) { - sb.append(','); - sb.append(formatVCFField(Array.get(val, i))); + } else { + if (val instanceof Double) { + result = formatVCFDouble((Double) val); + } else { + if (val instanceof Boolean) { + result = (Boolean) val ? "" : null; // empty string for true, null for false + } else { + if (val instanceof List) { + result = formatVCFField(((List) val).toArray()); + } else { + if (val.getClass().isArray()) { + final int length = Array.getLength(val); + if (length == 0) { + return formatVCFField(null); + } + final StringBuilder sb = new StringBuilder(formatVCFField(Array.get(val, 0))); + for (int i = 1; i < length; i++) { + sb.append(','); + sb.append(formatVCFField(Array.get(val, i))); + } + result = sb.toString(); + } else { + result = val.toString(); + } + } + } } - result = sb.toString(); - } else - result = val.toString(); + } return result; } @@ -245,9 +271,9 @@ public static String formatVCFDouble(final double d) { final String format; if (d < 1) { if (d < 0.01) { - if (Math.abs(d) >= 1e-20) + if (Math.abs(d) >= 1e-20) { format = "%.3e"; - else { + } else { // return a zero format return "0.00"; } @@ -299,7 +325,9 @@ public void addGenotypeData(final VariantContext vc, final Map a vcfoutput.append(VCFConstants.FIELD_SEPARATOR); Genotype g = vc.getGenotype(sample); - if (g == null) g = GenotypeBuilder.createMissing(sample, ploidy); + if (g == null) { + g = GenotypeBuilder.createMissing(sample, ploidy); + } final List attrs = new ArrayList<>(genotypeFormatKeys.size()); for (final String field : genotypeFormatKeys) { @@ -323,18 +351,21 @@ public void addGenotypeData(final VariantContext vc, final Map a final IntGenotypeFieldAccessors.Accessor accessor = GENOTYPE_FIELD_ACCESSORS.getAccessor(field); if (accessor != null) { final int[] intValues = accessor.getValues(g); - if (intValues == null) + if (intValues == null) { outputValue = VCFConstants.MISSING_VALUE_v4; - else if (intValues.length == 1) // fast path - outputValue = Integer.toString(intValues[0]); + } else { - final StringBuilder sb = new StringBuilder(); - sb.append(intValues[0]); - for (int i = 1; i < intValues.length; i++) { - sb.append(','); - sb.append(intValues[i]); + if (intValues.length == 1) { // fast path + outputValue = Integer.toString(intValues[0]); + } else { + final StringBuilder sb = new StringBuilder(); + sb.append(intValues[0]); + for (int i = 1; i < intValues.length; i++) { + sb.append(','); + sb.append(intValues[i]); + } + outputValue = sb.toString(); } - outputValue = sb.toString(); } } else { Object val = g.hasExtendedAttribute(field) ? g.getExtendedAttribute(field) : VCFConstants.MISSING_VALUE_v4; @@ -342,16 +373,20 @@ else if (intValues.length == 1) // fast path } } - if (outputValue != null) + if (outputValue != null) { attrs.add(outputValue); + } } } // strip off trailing missing values if (!outputTrailingFormatFields) { for (int i = attrs.size() - 1; i >= 0; i--) { - if (isMissingValue(attrs.get(i))) attrs.remove(i); - else break; + if (isMissingValue(attrs.get(i))) { + attrs.remove(i); + } else { + break; + } } } @@ -375,8 +410,11 @@ private void writeInfoString(final Map infoFields, final Appenda boolean isFirst = true; for (final Map.Entry entry : infoFields.entrySet()) { - if (isFirst) isFirst = false; - else vcfoutput.append(VCFConstants.INFO_FIELD_SEPARATOR); + if (isFirst) { + isFirst = false; + } else { + vcfoutput.append(VCFConstants.INFO_FIELD_SEPARATOR); + } vcfoutput.append(entry.getKey()); diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java index df309d5166..51bcf1bc3d 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java @@ -314,4 +314,16 @@ public void testGenotypesUnaffectedByClonedVariants(final VCBuilderScheme builde Assert.assertNotEquals(vc2.getGenotypes(), vc1.getGenotypes(), "The two genotype lists should be different. only saw " + vc1.getGenotypes().toString()); } + + + @Test + public void testCanResetFilters() { + + final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); + + builder.unfiltered(); + builder.filter("mayIPlease?"); + + } + } From d9676beb034ae6a4181bb564c610f29cf72153ae Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 1 Aug 2019 15:44:07 -0400 Subject: [PATCH 2/6] reformat deeply nested else-if statments --- .../java/htsjdk/variant/vcf/VCFEncoder.java | 72 ++++++++----------- 1 file changed, 29 insertions(+), 43 deletions(-) diff --git a/src/main/java/htsjdk/variant/vcf/VCFEncoder.java b/src/main/java/htsjdk/variant/vcf/VCFEncoder.java index d52f2d5c02..ac857fe428 100644 --- a/src/main/java/htsjdk/variant/vcf/VCFEncoder.java +++ b/src/main/java/htsjdk/variant/vcf/VCFEncoder.java @@ -195,11 +195,7 @@ private String getFilterString(final VariantContext vc) { return ParsingUtils.join(";", ParsingUtils.sortList(vc.getFilters())); } else { - if (vc.filtersWereApplied()){ - return VCFConstants.PASSES_FILTERS_v4; - } else { - return VCFConstants.UNFILTERED; - } + return vc.filtersWereApplied() ? VCFConstants.PASSES_FILTERS_v4 : VCFConstants.UNFILTERED; } } @@ -225,33 +221,26 @@ String formatVCFField(final Object val) { final String result; if (val == null) { result = VCFConstants.MISSING_VALUE_v4; - } else { - if (val instanceof Double) { - result = formatVCFDouble((Double) val); - } else { - if (val instanceof Boolean) { - result = (Boolean) val ? "" : null; // empty string for true, null for false - } else { - if (val instanceof List) { - result = formatVCFField(((List) val).toArray()); - } else { - if (val.getClass().isArray()) { - final int length = Array.getLength(val); - if (length == 0) { - return formatVCFField(null); - } - final StringBuilder sb = new StringBuilder(formatVCFField(Array.get(val, 0))); - for (int i = 1; i < length; i++) { - sb.append(','); - sb.append(formatVCFField(Array.get(val, i))); - } - result = sb.toString(); - } else { - result = val.toString(); - } - } - } + } else if (val instanceof Double) { + result = formatVCFDouble((Double) val); + } else if (val instanceof Boolean) { + result = (Boolean) val ? "" : null; // empty string for true, null for false + } else if (val instanceof List) { + result = formatVCFField(((List) val).toArray()); + } else if (val.getClass().isArray()) { + final int length = Array.getLength(val); + if (length == 0) { + return formatVCFField(null); } + final StringBuilder sb = new StringBuilder( + formatVCFField(Array.get(val, 0))); + for (int i = 1; i < length; i++) { + sb.append(','); + sb.append(formatVCFField(Array.get(val, i))); + } + result = sb.toString(); + } else { + result = val.toString(); } return result; @@ -353,19 +342,16 @@ public void addGenotypeData(final VariantContext vc, final Map a final int[] intValues = accessor.getValues(g); if (intValues == null) { outputValue = VCFConstants.MISSING_VALUE_v4; - } - else { - if (intValues.length == 1) { // fast path - outputValue = Integer.toString(intValues[0]); - } else { - final StringBuilder sb = new StringBuilder(); - sb.append(intValues[0]); - for (int i = 1; i < intValues.length; i++) { - sb.append(','); - sb.append(intValues[i]); - } - outputValue = sb.toString(); + } else if (intValues.length == 1) { // fast path + outputValue = Integer.toString(intValues[0]); + } else { + final StringBuilder sb = new StringBuilder(); + sb.append(intValues[0]); + for (int i = 1; i < intValues.length; i++) { + sb.append(','); + sb.append(intValues[i]); } + outputValue = sb.toString(); } } else { Object val = g.hasExtendedAttribute(field) ? g.getExtendedAttribute(field) : VCFConstants.MISSING_VALUE_v4; From 40036be34d213c49a532b042812f67fc3793c06d Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 1 Aug 2019 16:12:20 -0400 Subject: [PATCH 3/6] throw IllegalStateException instead of NPE --- .../htsjdk/variant/variantcontext/VariantContext.java | 3 +++ .../variantcontext/VariantContextBuilderTest.java | 9 +++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContext.java b/src/main/java/htsjdk/variant/variantcontext/VariantContext.java index 72c84c36af..ac2b44f1fe 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContext.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContext.java @@ -393,6 +393,9 @@ private static void validateFilters(final VariantContext variantContext) { } for (String filter : filters) { + if ( filter == null) { + throw new IllegalStateException("'null' is not a valid filter string."); + } if (!VALID_FILTER.matcher(filter).matches()) { throw new IllegalStateException("Filter '" + filter + "' contains an illegal character. It must conform to the regex ;'" + VALID_FILTER); diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java index 51bcf1bc3d..532039acc5 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java @@ -318,12 +318,17 @@ public void testGenotypesUnaffectedByClonedVariants(final VCBuilderScheme builde @Test public void testCanResetFilters() { - final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); - builder.unfiltered(); builder.filter("mayIPlease?"); + } + @Test(expectedExceptions = IllegalStateException.class) + public void testCantCreateNullFilter(){ + final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); + builder.filters((String)null); + builder.make(); } + } From 1cc3341cce680d35c7f9d7f6975f1eae6b566fc9 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 1 Aug 2019 16:17:52 -0400 Subject: [PATCH 4/6] validation fails with IllegalState instead of of NPE --- .../variantcontext/VariantContextBuilder.java | 5 ++++- .../variantcontext/VariantContextBuilderTest.java | 15 ++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java index 57391c4c40..d7ce5f52e8 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java @@ -397,10 +397,13 @@ private void filtersAsIs(final Set filters) { /** * {@link #filters} * - * @param filters Set of strings to set as the filters for this builder + * @param filters Strings to set as the filters for this builder * @return this builder */ public VariantContextBuilder filters(final String ... filters) { + if(filters == null){ + this.unfiltered(); + } filtersAsIs(new LinkedHashSet<>(Arrays.asList(filters))); return this; } diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java index 532039acc5..85cb25ac93 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java @@ -329,6 +329,19 @@ public void testCantCreateNullFilter(){ builder.filters((String)null); builder.make(); } - + + @Test + public void testNullFilterArray(){ + final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); + builder.filters((String[])null); + } + + @Test + public void testNullFilterSet(){ + final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); + builder.filters((Set)null); + builder.make(); + } + } From f63282ea909cca12656e3754d79c2d558c5a3579 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 2 Aug 2019 12:57:37 -0400 Subject: [PATCH 5/6] remove nl Co-authored-by: Yossi Farjoun Co-authored-by: Louis Bergelson --- .../variant/variantcontext/VariantContextBuilderTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java index 85cb25ac93..c8871bd2be 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantContextBuilderTest.java @@ -315,7 +315,6 @@ public void testGenotypesUnaffectedByClonedVariants(final VCBuilderScheme builde Assert.assertNotEquals(vc2.getGenotypes(), vc1.getGenotypes(), "The two genotype lists should be different. only saw " + vc1.getGenotypes().toString()); } - @Test public void testCanResetFilters() { final VariantContextBuilder builder = new VariantContextBuilder("source", "contig", 1, 1, Arrays.asList(Tref, C, G)).filter("TEST"); @@ -342,6 +341,4 @@ public void testNullFilterSet(){ builder.filters((Set)null); builder.make(); } - - } From ce997df169c5d0f5adb2337172c1e46a2e7de993 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 2 Aug 2019 14:06:17 -0400 Subject: [PATCH 6/6] fixup --- .../htsjdk/variant/variantcontext/VariantContextBuilder.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java index d7ce5f52e8..fae8d81514 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextBuilder.java @@ -403,8 +403,10 @@ private void filtersAsIs(final Set filters) { public VariantContextBuilder filters(final String ... filters) { if(filters == null){ this.unfiltered(); + } else { + this.filtersCanBeModified = true; + filtersAsIs(new LinkedHashSet<>(Arrays.asList(filters))); } - filtersAsIs(new LinkedHashSet<>(Arrays.asList(filters))); return this; }