search

samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

make VariantContextBuilder safer #1344

Closed yfarjoun closed 5 years ago

yfarjoun commented 5 years ago

Adds tests and fixes unsafe building patterns that were enabled by VariantContextBuilder.

This required a small change in the API around adding a genotype context. Now when .genotypes() is called with a GenotypeContext object the builder will call immutable() on that object, to make sure that the user doesn't change the GC prior to the creation of the VariantContext...If folks really need to reuse the GenotypeContext objects across multiple build calls, we could add a .genotypesKeepMutable() method with a scary javaDoc note.....

Another change in the API (somewhat) is that validation was added for the filters as per the hts-spec.

fixes #1365

yfarjoun commented 5 years ago

Several of these use-patterns fail (as you can see in the test results...) my question is..are they reasonable use-patterns and should we fix them?

codecov-io commented 5 years ago

Codecov Report

Merging #1344 into master will increase coverage by 1.872%. The diff coverage is 83.74%.

@@              Coverage Diff               @@
##             master     #1344       +/-   ##
==============================================
+ Coverage     67.86%   69.732%   +1.872%     
- Complexity     8284      9733     +1449     
==============================================
  Files           563       573       +10     
  Lines         33709     38324     +4615     
  Branches       5657      6997     +1340     
==============================================
+ Hits          22875     26724     +3849     
- Misses         8657      9212      +555     
- Partials       2177      2388      +211
Impacted Files Coverage Δ Complexity Δ
...tsjdk/variant/variantcontext/GenotypesContext.java 84.302% <100%> (ø) 67 <0> (ø) :arrow_down:
...ain/java/htsjdk/variant/variantcontext/Allele.java 77.703% <100%> (ø) 95 <0> (ø) :arrow_down:
...java/htsjdk/variant/variantcontext/CommonInfo.java 53.939% <50%> (+6.076%) 68 <3> (+21) :arrow_up:
.../htsjdk/variant/variantcontext/VariantContext.java 80.769% <80.822%> (+3.055%) 432 <16> (+186) :arrow_up:
.../variant/variantcontext/VariantContextBuilder.java 82.53% <86.486%> (+10.308%) 39 <13> (+4) :arrow_up:
...s/cram/encoding/external/ExternalLongEncoding.java 61.538% <0%> (-5.128%) 4% <0%> (+1%)
.../main/java/htsjdk/samtools/SAMValidationError.java 93.805% <0%> (-2.311%) 11% <0%> (+2%)
src/main/java/htsjdk/samtools/BAMSBIIndexer.java 73.333% <0%> (-1.667%) 1% <0%> (ø)
src/main/java/htsjdk/variant/bcf2/BCFVersion.java 93.103% <0%> (-1.633%) 12% <0%> (+5%)
src/main/java/htsjdk/samtools/SAMFileHeader.java 66.942% <0%> (-0.5%) 83% <0%> (+38%)
... and 78 more
yfarjoun commented 5 years ago

@d-cameron this fixes the issue you reported 4 years ago: #255

Care to take a look?

yfarjoun commented 5 years ago

back to you @pshapiro4broad

yfarjoun commented 5 years ago

@pshapiro4broad are you OK with this PR?

yfarjoun commented 5 years ago

@pshapiro4broad 👀 please?

yfarjoun commented 5 years ago

Thanks again @pshapiro4broad back to you.