From 60450a29e04da5722c90cfb614baa691697e64db Mon Sep 17 00:00:00 2001 From: Sergey Pyatykh Date: Wed, 27 May 2026 09:15:50 +0200 Subject: [PATCH 01/26] Issue 1038 - Test was refactored by using junit5 features --- biojava-genome/pom.xml | 12 + .../nbio/genome/io/fastq/FastqToolsTest.java | 415 ++++++------------ 2 files changed, 143 insertions(+), 284 deletions(-) diff --git a/biojava-genome/pom.xml b/biojava-genome/pom.xml index 698fa280c7..8974fdbfa3 100644 --- a/biojava-genome/pom.xml +++ b/biojava-genome/pom.xml @@ -67,6 +67,18 @@ junit test + + org.junit.jupiter + junit-jupiter-engine + + + org.junit.jupiter + junit-jupiter-params + + + org.junit.vintage + junit-vintage-engine + org.biojava biojava-core diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqToolsTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqToolsTest.java index 469601e624..43672b517b 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqToolsTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqToolsTest.java @@ -27,493 +27,340 @@ import org.biojava.nbio.core.sequence.features.QualityFeature; import org.biojava.nbio.core.sequence.features.QuantityFeature; import org.biojava.nbio.core.sequence.template.AbstractSequence; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Objects; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; /** * Unit test for FastqTools. */ -public final class FastqToolsTest { +final class FastqToolsTest { private final FastqBuilder builder = new FastqBuilder().withDescription("foo").withSequence("ACTG").withQuality("ZZZZ"); @Test - public void testCreateDNASequence() throws CompoundNotFoundException + void testCreateDNASequence() throws CompoundNotFoundException { DNASequence sequence = FastqTools.createDNASequence(builder.build()); - Assert.assertNotNull(sequence); + Assertions.assertNotNull(sequence); } @Test - public void testCreateDNASequenceNullFastq() throws CompoundNotFoundException + void testCreateDNASequenceNullFastq() { - try - { - FastqTools.createDNASequence(null); - Assert.fail("createDNASequence(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createDNASequence(null)); } @Test - public void testCreateDNASequenceWithQualityScores() throws CompoundNotFoundException + void testCreateDNASequenceWithQualityScores() throws CompoundNotFoundException { DNASequence sequence = FastqTools.createDNASequenceWithQualityScores(builder.build()); - Assert.assertNotNull(sequence); + Assertions.assertNotNull(sequence); List, NucleotideCompound>> features = sequence.getFeaturesByType("qualityScores"); - Assert.assertNotNull(features); - Assert.assertEquals(1, features.size()); + Assertions.assertNotNull(features); + Assertions.assertEquals(1, features.size()); QualityFeature, NucleotideCompound> qualityScores = (QualityFeature, NucleotideCompound>) features.get(0); - Assert.assertEquals(sequence.getLength(), qualityScores.getQualities().size()); - Assert.assertEquals(sequence.getLength(), qualityScores.getLocations().getLength()); + Assertions.assertEquals(sequence.getLength(), qualityScores.getQualities().size()); + Assertions.assertEquals(sequence.getLength(), qualityScores.getLocations().getLength()); } @Test - public void testCreateDNASequenceWithQualityScoresNullFastq() throws CompoundNotFoundException + void testCreateDNASequenceWithQualityScoresNullFastq() { - try - { - FastqTools.createDNASequenceWithQualityScores(null); - Assert.fail("createDNASequenceWithQualityScores(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createDNASequenceWithQualityScores(null)); } @Test - public void testCreateDNASequenceWithErrorProbabilies() throws CompoundNotFoundException + void testCreateDNASequenceWithErrorProbabilies() throws CompoundNotFoundException { DNASequence sequence = FastqTools.createDNASequenceWithErrorProbabilities(builder.build()); - Assert.assertNotNull(sequence); + Assertions.assertNotNull(sequence); List, NucleotideCompound>> features = sequence.getFeaturesByType("errorProbabilities"); - Assert.assertNotNull(features); - Assert.assertEquals(1, features.size()); + Assertions.assertNotNull(features); + Assertions.assertEquals(1, features.size()); QuantityFeature, NucleotideCompound> errorProbabilities = (QuantityFeature, NucleotideCompound>) features.get(0); - Assert.assertEquals(sequence.getLength(), errorProbabilities.getQuantities().size()); - Assert.assertEquals(sequence.getLength(), errorProbabilities.getLocations().getLength()); + Assertions.assertEquals(sequence.getLength(), errorProbabilities.getQuantities().size()); + Assertions.assertEquals(sequence.getLength(), errorProbabilities.getLocations().getLength()); } @Test - public void testCreateDNASequenceWithErrorProbabilitiesNullFastq() throws CompoundNotFoundException + void testCreateDNASequenceWithErrorProbabilitiesNullFastq() { - try - { - FastqTools.createDNASequenceWithErrorProbabilities(null); - Assert.fail("createDNASequenceWithErrorProbabilities(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createDNASequenceWithErrorProbabilities(null)); } @Test - public void testCreateDNASequenceWithQualityScoresAndErrorProbabilities() throws CompoundNotFoundException + void testCreateDNASequenceWithQualityScoresAndErrorProbabilities() throws CompoundNotFoundException { DNASequence sequence = FastqTools.createDNASequenceWithQualityScoresAndErrorProbabilities(builder.build()); - Assert.assertNotNull(sequence); + Assertions.assertNotNull(sequence); List, NucleotideCompound>> qualityScoresFeatures = sequence.getFeaturesByType("qualityScores"); - Assert.assertNotNull(qualityScoresFeatures); - Assert.assertEquals(1, qualityScoresFeatures.size()); + Assertions.assertNotNull(qualityScoresFeatures); + Assertions.assertEquals(1, qualityScoresFeatures.size()); QualityFeature, NucleotideCompound> qualityScores = (QualityFeature, NucleotideCompound>) qualityScoresFeatures.get(0); - Assert.assertEquals(sequence.getLength(), qualityScores.getQualities().size()); - Assert.assertEquals(sequence.getLength(), qualityScores.getLocations().getLength()); + Assertions.assertEquals(sequence.getLength(), qualityScores.getQualities().size()); + Assertions.assertEquals(sequence.getLength(), qualityScores.getLocations().getLength()); List, NucleotideCompound>> errorProbabilitiesFeatures = sequence.getFeaturesByType("errorProbabilities"); - Assert.assertNotNull(errorProbabilitiesFeatures); - Assert.assertEquals(1, errorProbabilitiesFeatures.size()); + Assertions.assertNotNull(errorProbabilitiesFeatures); + Assertions.assertEquals(1, errorProbabilitiesFeatures.size()); QuantityFeature, NucleotideCompound> errorProbabilities = (QuantityFeature, NucleotideCompound>) errorProbabilitiesFeatures.get(0); - Assert.assertEquals(sequence.getLength(), errorProbabilities.getQuantities().size()); - Assert.assertEquals(sequence.getLength(), errorProbabilities.getLocations().getLength()); + Assertions.assertEquals(sequence.getLength(), errorProbabilities.getQuantities().size()); + Assertions.assertEquals(sequence.getLength(), errorProbabilities.getLocations().getLength()); } @Test - public void testCreateDNASequenceWithQualityScoresAndErrorProbabilitiesNullFastq() throws CompoundNotFoundException + void testCreateDNASequenceWithQualityScoresAndErrorProbabilitiesNullFastq() { - try - { - FastqTools.createDNASequenceWithQualityScoresAndErrorProbabilities(null); - Assert.fail("createDNASequenceWithQualityScoresAndErrorProbabilities(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createDNASequenceWithQualityScoresAndErrorProbabilities(null)); } @Test - public void testCreateQualityScores() + void testCreateQualityScores() { Fastq fastq = builder.build(); QualityFeature, NucleotideCompound> qualityScores = FastqTools.createQualityScores(fastq); - Assert.assertNotNull(qualityScores); - Assert.assertEquals(fastq.getSequence().length(), qualityScores.getQualities().size()); + Assertions.assertNotNull(qualityScores); + Assertions.assertEquals(fastq.getSequence().length(), qualityScores.getQualities().size()); } @Test - public void testCreateQualityScoresNullFastq() + void testCreateQualityScoresNullFastq() { - try - { - FastqTools.createQualityScores(null); - Assert.fail("createQualityScores(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createQualityScores(null)); } @Test - public void testCreateErrorProbabilities() + void testCreateErrorProbabilities() { Fastq fastq = builder.build(); QuantityFeature, NucleotideCompound> errorProbabilities = FastqTools.createErrorProbabilities(fastq); - Assert.assertNotNull(errorProbabilities); - Assert.assertEquals(fastq.getSequence().length(), errorProbabilities.getQuantities().size()); + Assertions.assertNotNull(errorProbabilities); + Assertions.assertEquals(fastq.getSequence().length(), errorProbabilities.getQuantities().size()); } @Test - public void testCreateErrorProbabilitiesNullFastq() + void testCreateErrorProbabilitiesNullFastq() { - try - { - FastqTools.createErrorProbabilities(null); - Assert.fail("createErrorProbabilities(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.createErrorProbabilities(null)); } @Test - public void testQualityScores() + void testQualityScores() { Iterable qualityScores = FastqTools.qualityScores(builder.build()); - Assert.assertNotNull(qualityScores); - int count = 0; - for (Number qualityScore : qualityScores) - { - Assert.assertNotNull(qualityScore); - count++; - } - Assert.assertEquals(4, count); + List scoresList = StreamSupport.stream(qualityScores.spliterator(), false) + .collect(Collectors.toList()); + Assertions.assertAll( + () -> Assertions.assertEquals(4, scoresList.size()), + () -> Assertions.assertFalse(scoresList.contains(null)) + ); } @Test - public void testQualityScoresNullFastq() + void testQualityScoresNullFastq() { - try - { - FastqTools.qualityScores(null); - Assert.fail("qualityScores(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.qualityScores(null)); } @Test - public void testQualityScoresIntArray() + void testQualityScoresIntArray() { int[] qualityScores = new int[4]; FastqTools.qualityScores(builder.build(), qualityScores); - for (int i = 0; i < 4; i++) - { - Assert.assertTrue(qualityScores[i] != 0); - } + + Assertions.assertTrue(Arrays.stream(qualityScores).allMatch(score -> score != 0), () -> + "Array contains zero at some position: " + Arrays.toString(qualityScores)); } @Test - public void testQualityScoresIntArrayNullFastq() + void testQualityScoresIntArrayNullFastq() { - try - { - FastqTools.qualityScores(null, new int[0]); - Assert.fail("qualityScores(null, int[]) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.qualityScores(null, new int[0])); } @Test - public void testQualityScoresNullIntArray() + void testQualityScoresNullIntArray() { - try - { - FastqTools.qualityScores(builder.build(), null); - Assert.fail("qualityScores(fastq, null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.qualityScores(fastq, null)); } @Test - public void testQualityScoresQualityScoresTooSmall() + void testQualityScoresQualityScoresTooSmall() { - try - { - FastqTools.qualityScores(builder.build(), new int[3]); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.qualityScores(fastq, new int[3])); } @Test - public void testQualityScoresQualityScoresTooLarge() + void testQualityScoresQualityScoresTooLarge() { - try - { - FastqTools.qualityScores(builder.build(), new int[5]); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.qualityScores(fastq, new int[5])); } @Test - public void testErrorProbabilities() + void testErrorProbabilities() { Iterable errorProbabilities = FastqTools.errorProbabilities(builder.build()); - Assert.assertNotNull(errorProbabilities); - int count = 0; - for (Number errorProbability : errorProbabilities) - { - Assert.assertNotNull(errorProbability); - count++; - } - Assert.assertEquals(4, count); + List scores = StreamSupport.stream(errorProbabilities.spliterator(), false) + .collect(Collectors.toList()); + + Assertions.assertNotNull(scores); + Assertions.assertEquals(4, scores.size()); + Assertions.assertTrue(scores.stream().allMatch(Objects::nonNull)); } @Test - public void testErrorProbabilitiesNullFastq() + void testErrorProbabilitiesNullFastq() { - try - { - FastqTools.errorProbabilities(null); - Assert.fail("errorProbabilities(null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.errorProbabilities(null)); } @Test - public void testErrorProbabilitiesDoubleArray() + void testErrorProbabilitiesDoubleArray() { double[] errorProbabilities = new double[4]; FastqTools.errorProbabilities(builder.build(), errorProbabilities); - for (int i = 0; i < 0; i++) - { - Assert.assertTrue(errorProbabilities[i] > 0.0d); - } + Assertions.assertTrue( + Arrays.stream(errorProbabilities).allMatch(p -> p > 0.0), + () -> "Expected all probabilities to be > 0.0, but got: " + Arrays.toString(errorProbabilities) + ); } @Test - public void testErrorProbabilitiesDoubleArrayNullFastq() + void testErrorProbabilitiesDoubleArrayNullFastq() { - try - { - FastqTools.errorProbabilities(null, new double[0]); - Assert.fail("errorProbabilities(null, double[]) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.errorProbabilities(null, new double[0])); } @Test - public void testErrorProbabilitiesNullErrorProbabilities() + void testErrorProbabilitiesNullErrorProbabilities() { - try - { - FastqTools.errorProbabilities(builder.build(), null); - Assert.fail("errorProbabilities(fastq, null) expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.errorProbabilities(fastq, null)); } @Test - public void testErrorProbabilitiesErrorProbabilitiesTooSmall() + void testErrorProbabilitiesErrorProbabilitiesTooSmall() { - try - { - FastqTools.errorProbabilities(builder.build(), new double[3]); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.errorProbabilities(fastq, new double[3])); } @Test - public void testErrorProbabilitiesErrorProbabilitiesTooLarge() + void testErrorProbabilitiesErrorProbabilitiesTooLarge() { - try - { - FastqTools.errorProbabilities(builder.build(), new double[5]); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.errorProbabilities(fastq, new double[5])); } @Test - public void testConvertNullFastq() + void testConvertNullFastq() { - try - { - FastqTools.convert(null, FastqVariant.FASTQ_SANGER); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.convert(null, FastqVariant.FASTQ_SANGER)); } @Test - public void testConvertNullVariant() + void testConvertNullVariant() { - try - { - FastqTools.convert(builder.build(), null); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.convert(fastq, null)); } @Test - public void testConvertSameVariant() + void testConvertSameVariant() { Fastq fastq = builder.build(); - Assert.assertEquals(fastq, FastqTools.convert(fastq, fastq.getVariant())); + Assertions.assertEquals(fastq, FastqTools.convert(fastq, fastq.getVariant())); } @Test - public void testConvertQualitiesNullFastq() + void testConvertQualitiesNullFastq() { - try - { - FastqTools.convertQualities(null, FastqVariant.FASTQ_SANGER); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.convertQualities(null, FastqVariant.FASTQ_SANGER)); } @Test - public void testConvertQualitiesNullVariant() + void testConvertQualitiesNullVariant() { - try - { - FastqTools.convertQualities(builder.build(), null); - Assert.fail("expected IllegalArgumentException"); - } - catch (IllegalArgumentException e) - { - // expected - } + Fastq fastq = builder.build(); + Assertions.assertThrows(IllegalArgumentException.class, () -> FastqTools.convertQualities(fastq, null)); } @Test - public void testConvertQualitiesSameVariant() + void testConvertQualitiesSameVariant() { Fastq fastq = builder.build(); - Assert.assertEquals(fastq.getQuality(), FastqTools.convertQualities(fastq, fastq.getVariant())); + Assertions.assertEquals(fastq.getQuality(), FastqTools.convertQualities(fastq, fastq.getVariant())); } @Test - public void testConvertQualitiesSangerToSolexa() + void testConvertQualitiesSangerToSolexa() { Fastq fastq = builder.build(); - Assert.assertEquals("yyyy", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SOLEXA)); + Assertions.assertEquals("yyyy", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SOLEXA)); } @Test - public void testConvertQualitiesSangerToIllumina() + void testConvertQualitiesSangerToIllumina() { Fastq fastq = builder.build(); - Assert.assertEquals("yyyy", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_ILLUMINA)); + Assertions.assertEquals("yyyy", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_ILLUMINA)); } @Test - public void testConvertQualitiesSolexaToSanger() + void testConvertQualitiesSolexaToSanger() { Fastq fastq = builder.withVariant(FastqVariant.FASTQ_SOLEXA).build(); - Assert.assertEquals(";;;;", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SANGER)); + Assertions.assertEquals(";;;;", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SANGER)); } @Test - public void testConvertQualitiesIlluminaToSanger() + void testConvertQualitiesIlluminaToSanger() { Fastq fastq = builder.withVariant(FastqVariant.FASTQ_ILLUMINA).build(); - Assert.assertEquals(";;;;", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SANGER)); + Assertions.assertEquals(";;;;", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SANGER)); } @Test - public void testConvertQualitiesSolexaToIllumina() + void testConvertQualitiesSolexaToIllumina() { Fastq fastq = builder.withVariant(FastqVariant.FASTQ_SOLEXA).build(); - Assert.assertEquals("ZZZZ", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_ILLUMINA)); + Assertions.assertEquals("ZZZZ", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_ILLUMINA)); } @Test - public void testConvertQualitiesIlluminaToSolexa() + void testConvertQualitiesIlluminaToSolexa() { Fastq fastq = builder.withVariant(FastqVariant.FASTQ_ILLUMINA).build(); - Assert.assertEquals("ZZZZ", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SOLEXA)); + Assertions.assertEquals("ZZZZ", FastqTools.convertQualities(fastq, FastqVariant.FASTQ_SOLEXA)); } @Test - public void testToList() + void testToList() { - List list = new ArrayList(); - Assert.assertSame(list, FastqTools.toList(list)); + List list = new ArrayList<>(); + Assertions.assertSame(list, FastqTools.toList(list)); } @Test - public void testToListNotAList() + void testToListNotAList() { - Collection collection = new HashSet(); - Assert.assertTrue(FastqTools.toList(collection) instanceof List); - Assert.assertNotSame(collection, FastqTools.toList(collection)); + Collection collection = new HashSet<>(); + Assertions.assertTrue(FastqTools.toList(collection) instanceof List); + Assertions.assertNotSame(collection, FastqTools.toList(collection)); } } From bcd2e0a602cf12ae0d8d3ae633889708cabd917e Mon Sep 17 00:00:00 2001 From: Sergey Pyatykh Date: Wed, 27 May 2026 09:30:53 +0200 Subject: [PATCH 02/26] All tests in the biojava-genome module were migrated to junit5 --- biojava-genome/pom.xml | 9 -- .../biojava/nbio/genome/FeatureListTest.java | 12 +- .../nbio/genome/GeneFeatureHelperTest.java | 24 +-- .../genome/TestChromosomeMappingTools.java | 17 +- .../nbio/genome/TestGenomeMapping.java | 26 +-- .../org/biojava/nbio/genome/TestIssue355.java | 17 +- .../org/biojava/nbio/genome/TestLocation.java | 67 ++++---- .../io/fastq/AbstractFastqReaderTest.java | 82 +++++----- .../io/fastq/AbstractFastqWriterTest.java | 48 +++--- .../nbio/genome/io/fastq/ConvertTest.java | 19 ++- .../genome/io/fastq/FastqBuilderTest.java | 153 +++++++++--------- .../nbio/genome/io/fastq/FastqTest.java | 81 +++++----- .../genome/io/fastq/FastqVariantTest.java | 58 +++---- .../io/fastq/IlluminaFastqReaderTest.java | 58 +++---- .../io/fastq/IlluminaFastqWriterTest.java | 6 +- .../io/fastq/SangerFastqReaderTest.java | 97 ++++++----- .../io/fastq/SangerFastqWriterTest.java | 6 +- .../io/fastq/SolexaFastqReaderTest.java | 59 ++++--- .../io/fastq/SolexaFastqWriterTest.java | 6 +- .../io/fastq/StreamingFastqParserTest.java | 18 +-- 20 files changed, 426 insertions(+), 437 deletions(-) diff --git a/biojava-genome/pom.xml b/biojava-genome/pom.xml index 8974fdbfa3..c5bccfa23c 100644 --- a/biojava-genome/pom.xml +++ b/biojava-genome/pom.xml @@ -62,11 +62,6 @@ guava compile - - junit - junit - test - org.junit.jupiter junit-jupiter-engine @@ -75,10 +70,6 @@ org.junit.jupiter junit-jupiter-params - - org.junit.vintage - junit-vintage-engine - org.biojava biojava-core diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/FeatureListTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/FeatureListTest.java index 6e1cae5d8b..8c50e19bc2 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/FeatureListTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/FeatureListTest.java @@ -26,26 +26,26 @@ import org.biojava.nbio.genome.parsers.gff.Feature; import org.biojava.nbio.genome.parsers.gff.FeatureList; import org.biojava.nbio.genome.parsers.gff.Location; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; /** * @author mckeee1 * */ -public class FeatureListTest { +class FeatureListTest { @Test - public void testAddIndex() throws Exception + void testAddIndex() throws Exception { FeatureList fl = new FeatureList(); fl.add(new Feature("seqname", "source", "type", new Location(1, 2), (double)0, 0, "gene_id \"gene_id_1\"; transcript_id \"transcript_id_1\";")); fl.addIndex("transcript_id"); - Assert.assertEquals(1, fl.selectByAttribute("transcript_id").size()); + Assertions.assertEquals(1, fl.selectByAttribute("transcript_id").size()); FeatureList f2 = new FeatureList(); f2.addIndex("transcript_id"); f2.add(new Feature("seqname", "source", "type", new Location(1, 2), (double)0, 0, "gene_id \"gene_id_1\"; transcript_id \"transcript_id_1\";")); - Assert.assertEquals(1, f2.selectByAttribute("transcript_id").size()); + Assertions.assertEquals(1, f2.selectByAttribute("transcript_id").size()); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java index 3c81c50916..9c3a8877c6 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java @@ -28,9 +28,9 @@ import org.biojava.nbio.core.sequence.GeneSequence; import org.biojava.nbio.core.sequence.ProteinSequence; import org.biojava.nbio.core.sequence.io.FastaWriterHelper; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -45,20 +45,20 @@ * * @author Scooter Willis */ -public class GeneFeatureHelperTest { +class GeneFeatureHelperTest { private static final Logger logger = LoggerFactory.getLogger(GeneFeatureHelperTest.class); - @Before + @BeforeEach public void setUp() throws Exception { } - @After + @AfterEach public void tearDown() throws Exception { } @Test - public void testZeroLocation() throws Exception { + void testZeroLocation() throws Exception { @SuppressWarnings("unused") FeatureList listGenes = GFF3Reader.read("src/test/resources/amphimedon.gff3"); @@ -71,7 +71,7 @@ public void testZeroLocation() throws Exception { */ @Test - public void testLoadFastaAddGeneFeaturesFromUpperCaseExonFastaFile() throws Exception { + void testLoadFastaAddGeneFeaturesFromUpperCaseExonFastaFile() throws Exception { // logger.info("loadFastaAddGeneFeaturesFromUpperCaseExonFastaFile"); File fastaSequenceFile = new File("src/test/resources/volvox_all.fna"); File uppercaseFastaFile = new File("src/test/resources/volvox_all_genes_exon_uppercase.fna"); @@ -93,7 +93,7 @@ public void testLoadFastaAddGeneFeaturesFromUpperCaseExonFastaFile() throws Exce * Test of outputFastaSequenceLengthGFF3 method, of class GeneFeatureHelper. */ @Test - public void testOutputFastaSequenceLengthGFF3() throws Exception { + void testOutputFastaSequenceLengthGFF3() throws Exception { // logger.info("outputFastaSequenceLengthGFF3"); File fastaSequenceFile = new File("src/test/resources/volvox_all.fna"); @@ -112,7 +112,7 @@ public void testOutputFastaSequenceLengthGFF3() throws Exception { */ @Test - public void testAddGFF3Note() throws Exception { + void testAddGFF3Note() throws Exception { Map chromosomeSequenceList = GeneFeatureHelper .loadFastaAddGeneFeaturesFromGmodGFF3(new File("src/test/resources/volvox_all.fna"), new File( "src/test/resources/volvox.gff3"), false); @@ -128,7 +128,7 @@ public void testAddGFF3Note() throws Exception { * output. */ @Test - public void testGetProteinSequences() throws Exception { + void testGetProteinSequences() throws Exception { Map chromosomeSequenceList = GeneFeatureHelper .loadFastaAddGeneFeaturesFromGmodGFF3(new File("src/test/resources/volvox_all.fna"), new File( "src/test/resources/volvox.gff3"), false); @@ -148,7 +148,7 @@ public void testGetProteinSequences() throws Exception { * Test of getGeneSequences method, of class GeneFeatureHelper. */ @Test - public void testGetGeneSequences() throws Exception { + void testGetGeneSequences() throws Exception { // logger.info("getGeneSequences"); Map chromosomeSequenceList = GeneFeatureHelper .loadFastaAddGeneFeaturesFromGmodGFF3(new File("src/test/resources/volvox_all.fna"), new File( diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java index 9ddd43357a..a86b8718c5 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java @@ -21,7 +21,8 @@ package org.biojava.nbio.genome; import org.biojava.nbio.genome.util.ChromosomeMappingTools; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.Arrays; @@ -32,10 +33,10 @@ /** * Created by Yana Valasatava on 8/14/17. */ -public class TestChromosomeMappingTools { +class TestChromosomeMappingTools { @Test - public void testGetCDSLengthForward() { + void testGetCDSLengthForward() { List exonStarts = new ArrayList<>(Arrays.asList(10, 30, 50, 70)); List exonEnds = new ArrayList<>(Arrays.asList(20, 40, 60, 80)); @@ -46,11 +47,11 @@ public void testGetCDSLengthForward() { ChromosomeMappingTools.setCoordinateSystem(0); int cdsTest = ChromosomeMappingTools.getCDSLengthForward(exonStarts, exonEnds, cdsStart, cdsEnd); - assertEquals(cdsDesired, cdsTest); + Assertions.assertEquals(cdsDesired, cdsTest); } @Test - public void testGetCDSLengthReverseAsc() { + void testGetCDSLengthReverseAsc() { List exonStarts = new ArrayList<>(Arrays.asList(10, 50, 70)); List exonEnds = new ArrayList<>(Arrays.asList(20, 60, 80)); @@ -61,11 +62,11 @@ public void testGetCDSLengthReverseAsc() { ChromosomeMappingTools.setCoordinateSystem(0); int cdsTest = ChromosomeMappingTools.getCDSLengthReverse(exonStarts, exonEnds, cdsStart, cdsEnd); - assertEquals(cdsDesired, cdsTest); + Assertions.assertEquals(cdsDesired, cdsTest); } @Test - public void testGetCDSLengthReverseDesc() { + void testGetCDSLengthReverseDesc() { List exonStarts = new ArrayList<>(Arrays.asList(70, 50, 10)); List exonEnds = new ArrayList<>(Arrays.asList(80, 60, 20)); @@ -76,6 +77,6 @@ public void testGetCDSLengthReverseDesc() { ChromosomeMappingTools.setCoordinateSystem(0); int cdsTest = ChromosomeMappingTools.getCDSLengthReverse(exonStarts, exonEnds, cdsStart, cdsEnd); - assertEquals(cdsDesired, cdsTest); + Assertions.assertEquals(cdsDesired, cdsTest); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestGenomeMapping.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestGenomeMapping.java index 4999cfa6fb..257b88e3e1 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestGenomeMapping.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestGenomeMapping.java @@ -22,8 +22,8 @@ import com.google.common.collect.Range; import org.biojava.nbio.genome.util.ChromosomeMappingTools; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.Arrays; @@ -32,10 +32,10 @@ /** * Created by andreas on 7/19/16. */ -public class TestGenomeMapping { +class TestGenomeMapping { @Test - public void testGenomeMappingToolGetCDSRanges(){ + void testGenomeMappingToolGetCDSRanges(){ List lst1 = new ArrayList<>(Arrays.asList( 86346823, 86352858, 86354529)); List lst2 = new ArrayList<>(Arrays.asList(86348878, 86352984, 86354692)); @@ -45,21 +45,21 @@ public void testGenomeMappingToolGetCDSRanges(){ List> result = ChromosomeMappingTools.getCDSRegions(lst1,lst2,cdsStart,cdsEnd); // makes sure the first list does not get changed; - Assert.assertEquals(86346823, (int) lst1.get(0)); + Assertions.assertEquals(86346823, (int) lst1.get(0)); - Assert.assertEquals(86348749, (int) result.get(0).lowerEndpoint()); - Assert.assertEquals(86352858, (int) result.get(1).lowerEndpoint()); - Assert.assertEquals(86354529, (int) result.get(2).lowerEndpoint()); + Assertions.assertEquals(86348749, (int) result.get(0).lowerEndpoint()); + Assertions.assertEquals(86352858, (int) result.get(1).lowerEndpoint()); + Assertions.assertEquals(86354529, (int) result.get(2).lowerEndpoint()); - Assert.assertEquals(86348878, (int) result.get(0).upperEndpoint()); - Assert.assertEquals(86352984, (int) result.get(1).upperEndpoint()); - Assert.assertEquals(86387027, (int) result.get(2).upperEndpoint()); + Assertions.assertEquals(86348878, (int) result.get(0).upperEndpoint()); + Assertions.assertEquals(86352984, (int) result.get(1).upperEndpoint()); + Assertions.assertEquals(86387027, (int) result.get(2).upperEndpoint()); } @Test - public void testGenomeMappingToolGetCDSRangesSERINC2(){ + void testGenomeMappingToolGetCDSRangesSERINC2(){ List lst1 = new ArrayList<>(Arrays.asList(31413812, 31415872, 31423692)); List lst2 = new ArrayList<>(Arrays.asList(31414777, 31415907, 31423854)); @@ -69,7 +69,7 @@ public void testGenomeMappingToolGetCDSRangesSERINC2(){ List> result = ChromosomeMappingTools.getCDSRegions(lst1,lst2,cdsStart,cdsEnd); // makes sure the first list does not get changed; - Assert.assertEquals(31423818, (int) result.get(0).lowerEndpoint()); + Assertions.assertEquals(31423818, (int) result.get(0).lowerEndpoint()); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestIssue355.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestIssue355.java index 5543682f17..4c98225b04 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestIssue355.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestIssue355.java @@ -20,29 +20,28 @@ */ package org.biojava.nbio.genome; -import static org.junit.Assert.*; - import org.biojava.nbio.genome.parsers.gff.Location; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; -public class TestIssue355 { +class TestIssue355 { @Test - public void testIssue1() { + void testIssue1() { Location l1 = Location.fromBio(51227320, 51227381, '+'); Location l2 = Location.fromBio(51227323, 51227382, '+'); Location union = l1.union(l2); - assertEquals(51227320,union.bioStart()); - assertEquals(51227382,union.bioEnd()); + Assertions.assertEquals(51227320, union.bioStart()); + Assertions.assertEquals(51227382, union.bioEnd()); } @Test - public void testIssue2() { + void testIssue2() { Location l1 = Location.fromBio(100, 200, '+'); Location l2 = Location.fromBio(1, 99, '+'); Location intersection = l1.intersection(l2); - assertNull(intersection); + Assertions.assertNull(intersection); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestLocation.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestLocation.java index 1289cb757e..42893cf22b 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestLocation.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestLocation.java @@ -20,15 +20,14 @@ */ package org.biojava.nbio.genome; -import static org.junit.Assert.*; - import org.biojava.nbio.genome.parsers.gff.Location; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; -public class TestLocation { +class TestLocation { @Test - public void testLocation() { + void testLocation() { // tests taken from Location.main() //Location p3_7= new Location( 3, 7 ); @@ -49,70 +48,70 @@ public void testLocation() { Location r5_8= new Location( 5, 8 ); //distance - assertEquals(7, L(14,14).distance( L(3,7) )); - assertEquals(7, L(3,7).distance( L(14,14) )); - assertEquals(3, L(1,4).distance( L(7, 10) )); + Assertions.assertEquals(7, L(14,14).distance( L(3,7) )); + Assertions.assertEquals(7, L(3,7).distance( L(14,14) )); + Assertions.assertEquals(3, L(1,4).distance( L(7, 10) )); //union - assertEquals(p10_17, p10_12.union( p14_17 )); - assertEquals(p10_17, p14_17.union( p10_12 )); - assertEquals(p15_19, p15_19.union( p15_16 )); + Assertions.assertEquals(p10_17, p10_12.union( p14_17 )); + Assertions.assertEquals(p10_17, p14_17.union( p10_12 )); + Assertions.assertEquals(p15_19, p15_19.union( p15_16 )); //intersection - assertEquals(new Location( 21, 25 ), r13_17.union( r21_25 ).intersection( r21_25 )); + Assertions.assertEquals(new Location( 21, 25 ), r13_17.union( r21_25 ).intersection( r21_25 )); //isBefore - assertTrue( r2_5.isBefore( r5_8 )); - assertTrue( !r2_5.isBefore( r4_7 )); + Assertions.assertTrue(r2_5.isBefore( r5_8 )); + Assertions.assertTrue(!r2_5.isBefore( r4_7 )); //isAfter - assertTrue(r5_8.isAfter( r2_5 )); - assertTrue(!r5_8.isAfter( r4_7 )); + Assertions.assertTrue(r5_8.isAfter( r2_5 )); + Assertions.assertTrue(!r5_8.isAfter( r4_7 )); //contains - assertTrue(p15_19.contains( p16_19 )); + Assertions.assertTrue(p15_19.contains( p16_19 )); //overlaps - assertTrue(r2_5.overlaps( r4_7 )); - assertTrue(r2_5.overlaps( r0_3 )); - assertTrue(!r5_8.overlaps( r2_5 )); - assertTrue(!r2_5.overlaps( r5_8 )); + Assertions.assertTrue(r2_5.overlaps( r4_7 )); + Assertions.assertTrue(r2_5.overlaps( r0_3 )); + Assertions.assertTrue(!r5_8.overlaps( r2_5 )); + Assertions.assertTrue(!r2_5.overlaps( r5_8 )); //prefix - assertEquals(L(2,3), L(2,20).prefix(1)); - assertEquals(L(2,19), L(2,20).prefix(-1)); - assertEquals( L(2,10), L(2,20).prefix( L(10,12))); + Assertions.assertEquals(L(2,3), L(2,20).prefix(1)); + Assertions.assertEquals(L(2,19), L(2,20).prefix(-1)); + Assertions.assertEquals(L(2,10), L(2,20).prefix( L(10,12))); //suffix - assertEquals(L(3,20), L(2,20).suffix(1)); - assertEquals(L(19,20), L(2,20).suffix(-1)); - assertEquals(L(12,20), L(2,20).suffix( L(10,12))); + Assertions.assertEquals(L(3,20), L(2,20).suffix(1)); + Assertions.assertEquals(L(19,20), L(2,20).suffix(-1)); + Assertions.assertEquals(L(12,20), L(2,20).suffix( L(10,12))); } @Test - public void testLocationIntersections() { + void testLocationIntersections() { // One inside another Location r21_25 = new Location( 21, 25 ); Location r1_100 = new Location(1, 100 ); - assertEquals(r21_25, r21_25.intersection( r1_100)); - assertEquals(r21_25, r1_100.intersection( r21_25)); + Assertions.assertEquals(r21_25, r21_25.intersection( r1_100)); + Assertions.assertEquals(r21_25, r1_100.intersection( r21_25)); // Non overlapping Location r10_100 = new Location(10, 100 ); Location r1_9 = new Location( 1, 9 ); - assertNull(r10_100.intersection( r1_9)); - assertNull(r1_9.intersection( new Location( 9, 10 ))); + Assertions.assertNull(r10_100.intersection( r1_9)); + Assertions.assertNull(r1_9.intersection( new Location( 9, 10 ))); // Partially overlappping Location r1_25 = new Location( 1, 25 ); Location r21_100 = new Location(21, 100 ); - assertEquals(r21_25, r1_25.intersection( r21_100)); - assertEquals(r21_25, r21_100.intersection( r1_25)); + Assertions.assertEquals(r21_25, r1_25.intersection( r21_100)); + Assertions.assertEquals(r21_25, r21_100.intersection( r1_25)); } //shorthand for testing diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqReaderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqReaderTest.java index 7cde3d3ec9..3e1385f3a2 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqReaderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqReaderTest.java @@ -20,8 +20,8 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.File; import java.io.IOException; @@ -34,7 +34,7 @@ /** * Abstract unit test for implementations of FastqReader. */ -public abstract class AbstractFastqReaderTest { +abstract class AbstractFastqReaderTest { /** Array of example files that should throw IOExceptions. */ static final String[] ERROR_EXAMPLES = new String[] { @@ -87,21 +87,21 @@ public abstract class AbstractFastqReaderTest { public void testCreateFastq() { Fastq fastq = createFastq(); - Assert.assertNotNull(fastq); + Assertions.assertNotNull(fastq); } @Test public void testCreateFastqReader() { FastqReader reader = createFastqReader(); - Assert.assertNotNull(reader); + Assertions.assertNotNull(reader); } @Test public void testCreateFastqWriter() { FastqWriter writer = createFastqWriter(); - Assert.assertNotNull(writer); + Assertions.assertNotNull(writer); } @Test @@ -111,7 +111,7 @@ public void testReadFile() throws Exception try { reader.read((File) null); - Assert.fail("read((File) null) expected IllegalArgumentException"); + Assertions.fail("read((File) null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -121,7 +121,7 @@ public void testReadFile() throws Exception { File noSuchFile = new File("no such file"); reader.read(noSuchFile); - Assert.fail("read(no such file) expected IOException"); + Assertions.fail("read(no such file) expected IOException"); } catch (IOException e) { @@ -135,14 +135,14 @@ public void testReadEmptyFile() throws Exception FastqReader reader = createFastqReader(); File empty = Files.createTempFile("abstractFastqReaderTest",null).toFile(); Iterable iterable = reader.read(empty); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); + Assertions.assertNotNull(f); count++; } - Assert.assertEquals(0, count); + Assertions.assertEquals(0, count); } @Test @@ -154,14 +154,14 @@ public void testReadRoundTripSingleFile() throws Exception FastqWriter writer = createFastqWriter(); writer.write(single, fastq); Iterable iterable = reader.read(single); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); + Assertions.assertNotNull(f); count++; } - Assert.assertEquals(1, count); + Assertions.assertEquals(1, count); } @Test @@ -175,14 +175,14 @@ public void testReadRoundTripMultipleFile() throws Exception FastqWriter writer = createFastqWriter(); writer.write(multiple, fastq0, fastq1, fastq2); Iterable iterable = reader.read(multiple); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); + Assertions.assertNotNull(f); count++; } - Assert.assertEquals(3, count); + Assertions.assertEquals(3, count); } @Test @@ -192,7 +192,7 @@ public void testReadURL() throws Exception try { reader.read((URL) null); - Assert.fail("read((URL) null) expected IllegalArgumentException"); + Assertions.fail("read((URL) null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -202,7 +202,7 @@ public void testReadURL() throws Exception { URL noSuchURL = new URL("file:///no such url"); reader.read(noSuchURL); - Assert.fail("read(no such URL) expected IOException"); + Assertions.fail("read(no such URL) expected IOException"); } catch (IOException e) { @@ -216,14 +216,14 @@ public void testReadEmptyURL() throws Exception FastqReader reader = createFastqReader(); URL empty = getClass().getResource("empty.fastq"); Iterable iterable = reader.read(empty); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); + Assertions.assertNotNull(f); count++; } - Assert.assertEquals(0, count); + Assertions.assertEquals(0, count); } @Test @@ -233,7 +233,7 @@ public void testReadInputStream() throws Exception try { reader.read((InputStream) null); - Assert.fail("read((InputStream) null) expected IllegalArgumentException"); + Assertions.fail("read((InputStream) null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -247,14 +247,14 @@ public void testReadEmptyInputStream() throws Exception FastqReader reader = createFastqReader(); InputStream empty = getClass().getResourceAsStream("empty.fastq"); Iterable iterable = reader.read(empty); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); + Assertions.assertNotNull(f); count++; } - Assert.assertEquals(0, count); + Assertions.assertEquals(0, count); empty.close(); } @@ -264,15 +264,15 @@ public void testWrappedSequence() throws Exception FastqReader reader = createFastqReader(); InputStream wrappedSequence = getClass().getResourceAsStream("wrapped-sequence.fastq"); Iterable iterable = reader.read(wrappedSequence); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); - Assert.assertEquals("ACTG", f.getSequence()); + Assertions.assertNotNull(f); + Assertions.assertEquals("ACTG", f.getSequence()); count++; } - Assert.assertEquals(1, count); + Assertions.assertEquals(1, count); wrappedSequence.close(); } @@ -282,15 +282,15 @@ public void testWrappedQuality() throws Exception FastqReader reader = createFastqReader(); InputStream wrappedQuality = getClass().getResourceAsStream("wrapped-quality.fastq"); Iterable iterable = reader.read(wrappedQuality); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); - Assert.assertEquals("ZZZZ", f.getQuality()); + Assertions.assertNotNull(f); + Assertions.assertEquals("ZZZZ", f.getQuality()); count++; } - Assert.assertEquals(1, count); + Assertions.assertEquals(1, count); wrappedQuality.close(); } @@ -300,15 +300,15 @@ public void testMultipleWrappedQuality() throws Exception FastqReader reader = createFastqReader(); InputStream wrappedQuality = getClass().getResourceAsStream("multiple-wrapped-quality.fastq"); Iterable iterable = reader.read(wrappedQuality); - Assert.assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - Assert.assertNotNull(f); - Assert.assertEquals("ZZZZ", f.getQuality()); + Assertions.assertNotNull(f); + Assertions.assertEquals("ZZZZ", f.getQuality()); count++; } - Assert.assertEquals(4, count); + Assertions.assertEquals(4, count); wrappedQuality.close(); } @@ -322,7 +322,7 @@ public void testErrorExamples() throws Exception try { reader.read(inputStream); - Assert.fail("error example " + errorExample + " expected IOException"); + Assertions.fail("error example " + errorExample + " expected IOException"); } catch (IOException e) { @@ -430,7 +430,7 @@ public void complete() throws IOException { // empty } }); - Assert.fail("parse(null, ) expected IllegalArgumentException"); + Assertions.fail("parse(null, ) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -446,7 +446,7 @@ public void testParseNullParseListener() throws Exception try { reader.parse(new StringReader(input), null); - Assert.fail("parse(, null) expected IllegalArgumentException"); + Assertions.fail("parse(, null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqWriterTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqWriterTest.java index cf2b695968..f2000b5096 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqWriterTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/AbstractFastqWriterTest.java @@ -20,8 +20,8 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.ByteArrayOutputStream; import java.io.File; @@ -34,7 +34,7 @@ /** * Abstract unit test for implementations of FastqWriter. */ -public abstract class AbstractFastqWriterTest { +abstract class AbstractFastqWriterTest { /** * Create and return a new FASTQ formatted sequence suitable for testing. @@ -54,14 +54,14 @@ public abstract class AbstractFastqWriterTest { public void testCreateFastq() { Fastq fastq = createFastq(); - Assert.assertNotNull(fastq); + Assertions.assertNotNull(fastq); } @Test public void testCreateFastqWriter() { FastqWriter writer = createFastqWriter(); - Assert.assertNotNull(writer); + Assertions.assertNotNull(writer); } @Test @@ -72,16 +72,16 @@ public void testAppendVararg() throws Exception Fastq fastq0 = createFastq(); Fastq fastq1 = createFastq(); Fastq fastq2 = createFastq(); - Assert.assertSame(appendable, writer.append(appendable, fastq0)); - Assert.assertSame(appendable, writer.append(appendable, fastq0, fastq1)); - Assert.assertSame(appendable, writer.append(appendable, fastq0, fastq1, fastq2)); - Assert.assertSame(appendable, writer.append(appendable, fastq0, fastq1, fastq2, null)); - Assert.assertSame(appendable, writer.append(appendable, (Fastq) null)); + Assertions.assertSame(appendable, writer.append(appendable, fastq0)); + Assertions.assertSame(appendable, writer.append(appendable, fastq0, fastq1)); + Assertions.assertSame(appendable, writer.append(appendable, fastq0, fastq1, fastq2)); + Assertions.assertSame(appendable, writer.append(appendable, fastq0, fastq1, fastq2, null)); + Assertions.assertSame(appendable, writer.append(appendable, (Fastq) null)); try { writer.append((Appendable) null, fastq0); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -98,20 +98,20 @@ public void testAppendIterable() throws Exception Fastq fastq1 = createFastq(); Fastq fastq2 = createFastq(); List list = new ArrayList(); - Assert.assertSame(appendable, writer.append(appendable, list)); + Assertions.assertSame(appendable, writer.append(appendable, list)); list.add(fastq0); - Assert.assertSame(appendable, writer.append(appendable, list)); + Assertions.assertSame(appendable, writer.append(appendable, list)); list.add(fastq1); - Assert.assertSame(appendable, writer.append(appendable, list)); + Assertions.assertSame(appendable, writer.append(appendable, list)); list.add(fastq2); - Assert.assertSame(appendable, writer.append(appendable, list)); + Assertions.assertSame(appendable, writer.append(appendable, list)); list.add(null); - Assert.assertSame(appendable, writer.append(appendable, list)); + Assertions.assertSame(appendable, writer.append(appendable, list)); try { writer.append((Appendable) null, list); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -120,7 +120,7 @@ public void testAppendIterable() throws Exception try { writer.append(appendable, (Iterable) null); - Assert.fail("append(,null) expected IllegalArgumentException"); + Assertions.fail("append(,null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -149,7 +149,7 @@ public void testWriteFileVararg() throws Exception try { writer.write((File) null, fastq0); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -189,7 +189,7 @@ public void testWriteFileIterable() throws Exception try { writer.write((File) null, fastq0); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -198,7 +198,7 @@ public void testWriteFileIterable() throws Exception try { writer.write(file5, (Iterable) null); - Assert.fail("append(,null) expected IllegalArgumentException"); + Assertions.fail("append(,null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -223,7 +223,7 @@ public void testWriteOutputStreamVararg() throws Exception try { writer.write((OutputStream) null, fastq0); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -253,7 +253,7 @@ public void testWriteOutputStreamIterable() throws Exception try { writer.write((OutputStream) null, fastq0); - Assert.fail("append(null,) expected IllegalArgumentException"); + Assertions.fail("append(null,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -262,7 +262,7 @@ public void testWriteOutputStreamIterable() throws Exception try { writer.write(outputStream, (Iterable) null); - Assert.fail("append(,null) expected IllegalArgumentException"); + Assertions.fail("append(,null) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/ConvertTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/ConvertTest.java index b07e237ef2..c6789e622f 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/ConvertTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/ConvertTest.java @@ -26,19 +26,18 @@ import java.util.List; import java.util.Map; -import org.junit.Test; -import static org.junit.Assert.*; - import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; /** * Round trip conversion functional tests. */ -public final class ConvertTest { +final class ConvertTest { @Test - public void testConvert() throws Exception + void testConvert() throws Exception { Map readers = Maps.newHashMap(); readers.put(FastqVariant.FASTQ_SANGER, new SangerFastqReader()); @@ -88,13 +87,13 @@ public void testConvert() throws Exception List observed = Lists.newArrayList(resultReader.read(tmp)); List expected = Lists.newArrayList(resultReader.read(getClass().getResource(expectedFileName))); - assertEquals(expected.size(), observed.size()); + Assertions.assertEquals(expected.size(), observed.size()); for (int i = 0; i < expected.size(); i++) { - assertEquals(expected.get(i).getDescription(), observed.get(i).getDescription()); - assertEquals(expected.get(i).getSequence(), observed.get(i).getSequence()); - assertEquals(expected.get(i).getQuality(), observed.get(i).getQuality()); - assertEquals(expected.get(i).getVariant(), observed.get(i).getVariant()); + Assertions.assertEquals(expected.get(i).getDescription(), observed.get(i).getDescription()); + Assertions.assertEquals(expected.get(i).getSequence(), observed.get(i).getSequence()); + Assertions.assertEquals(expected.get(i).getQuality(), observed.get(i).getQuality()); + Assertions.assertEquals(expected.get(i).getVariant(), observed.get(i).getVariant()); } } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java index 5803068276..3307256126 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java @@ -21,24 +21,25 @@ package org.biojava.nbio.genome.io.fastq; import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import org.junit.function.ThrowingRunnable; /** * Unit test for FastqBuilder. */ -public final class FastqBuilderTest { +final class FastqBuilderTest { @Test - public void testConstructor() + void testConstructor() { FastqBuilder fastqBuilder = new FastqBuilder(); - Assert.assertNotNull(fastqBuilder); + Assertions.assertNotNull(fastqBuilder); } @Test - public void testConstructorFastq() + void testConstructorFastq() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -49,17 +50,17 @@ public void testConstructorFastq() Fastq fastq = fastqBuilder.build(); FastqBuilder fastqBuilder2 = new FastqBuilder(fastq); - Assert.assertNotNull(fastqBuilder2); + Assertions.assertNotNull(fastqBuilder2); Fastq fastq2 = fastqBuilder2.build(); - Assert.assertEquals("description", fastq2.getDescription()); - Assert.assertEquals("sequence", fastq2.getSequence()); - Assert.assertEquals("quality_", fastq2.getQuality()); - Assert.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq2.getVariant()); + Assertions.assertEquals("description", fastq2.getDescription()); + Assertions.assertEquals("sequence", fastq2.getSequence()); + Assertions.assertEquals("quality_", fastq2.getQuality()); + Assertions.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq2.getVariant()); } @Test - public void testConstructorNullFastq() + void testConstructorNullFastq() { Assert.assertThrows(IllegalArgumentException.class, new ThrowingRunnable() { @Override @@ -70,13 +71,13 @@ public void run() { } @Test - public void testBuildDefault() + void testBuildDefault() { try { FastqBuilder fastqBuilder = new FastqBuilder(); fastqBuilder.build(); - Assert.fail("build default expected IllegalStateException"); + Assertions.fail("build default expected IllegalStateException"); } catch (IllegalStateException e) { @@ -85,7 +86,7 @@ public void testBuildDefault() } @Test - public void testBuildNullDescription() + void testBuildNullDescription() { try { @@ -96,7 +97,7 @@ public void testBuildNullDescription() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build null description expected IllegalArgumentException"); + Assertions.fail("build null description expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -105,7 +106,7 @@ public void testBuildNullDescription() } @Test - public void testBuildNullSequence() + void testBuildNullSequence() { try { @@ -116,7 +117,7 @@ public void testBuildNullSequence() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build null sequence expected IllegalArgumentException"); + Assertions.fail("build null sequence expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -125,7 +126,7 @@ public void testBuildNullSequence() } @Test - public void testBuildNullAppendSequence() + void testBuildNullAppendSequence() { try { @@ -136,7 +137,7 @@ public void testBuildNullAppendSequence() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build null append sequence expected IllegalArgumentException"); + Assertions.fail("build null append sequence expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -145,7 +146,7 @@ public void testBuildNullAppendSequence() } @Test - public void testBuildNullQuality() + void testBuildNullQuality() { try { @@ -156,7 +157,7 @@ public void testBuildNullQuality() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build null quality expected IllegalArgumentException"); + Assertions.fail("build null quality expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -165,7 +166,7 @@ public void testBuildNullQuality() } @Test - public void testBuildNullAppendQuality() + void testBuildNullAppendQuality() { try { @@ -176,7 +177,7 @@ public void testBuildNullAppendQuality() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build null append quality expected IllegalArgumentException"); + Assertions.fail("build null append quality expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -185,7 +186,7 @@ public void testBuildNullAppendQuality() } @Test - public void testBuildNullVariant() + void testBuildNullVariant() { try { @@ -196,7 +197,7 @@ public void testBuildNullVariant() .withVariant(null); fastqBuilder.build(); - Assert.fail("build null variant expected IllegalArgumentException"); + Assertions.fail("build null variant expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -205,7 +206,7 @@ public void testBuildNullVariant() } @Test - public void testBuildMissingDescription() + void testBuildMissingDescription() { try { @@ -215,7 +216,7 @@ public void testBuildMissingDescription() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build missing description expected IllegalStateException"); + Assertions.fail("build missing description expected IllegalStateException"); } catch (IllegalStateException e) { @@ -224,7 +225,7 @@ public void testBuildMissingDescription() } @Test - public void testBuildMissingSequence() + void testBuildMissingSequence() { try { @@ -234,7 +235,7 @@ public void testBuildMissingSequence() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build missing sequence expected IllegalStateException"); + Assertions.fail("build missing sequence expected IllegalStateException"); } catch (IllegalStateException e) { @@ -243,7 +244,7 @@ public void testBuildMissingSequence() } @Test - public void testBuildMissingQuality() + void testBuildMissingQuality() { try { @@ -253,7 +254,7 @@ public void testBuildMissingQuality() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build missing quality expected IllegalStateException"); + Assertions.fail("build missing quality expected IllegalStateException"); } catch (IllegalStateException e) { @@ -262,7 +263,7 @@ public void testBuildMissingQuality() } @Test - public void testBuildDefaultVariant() + void testBuildDefaultVariant() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -270,16 +271,16 @@ public void testBuildDefaultVariant() .withQuality("quality_"); Fastq fastq = fastqBuilder.build(); - Assert.assertEquals("description", fastqBuilder.getDescription()); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); - Assert.assertEquals("description", fastq.getDescription()); - Assert.assertEquals("sequence", fastq.getSequence()); - Assert.assertEquals("quality_", fastq.getQuality()); - Assert.assertEquals(FastqBuilder.DEFAULT_VARIANT, fastq.getVariant()); + Assertions.assertEquals("description", fastqBuilder.getDescription()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals("description", fastq.getDescription()); + Assertions.assertEquals("sequence", fastq.getSequence()); + Assertions.assertEquals("quality_", fastq.getQuality()); + Assertions.assertEquals(FastqBuilder.DEFAULT_VARIANT, fastq.getVariant()); } @Test - public void testBuild() + void testBuild() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -287,16 +288,16 @@ public void testBuild() .withQuality("quality_") .withVariant(FastqVariant.FASTQ_SOLEXA); Fastq fastq = fastqBuilder.build(); - Assert.assertEquals("description", fastqBuilder.getDescription()); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); - Assert.assertEquals("description", fastq.getDescription()); - Assert.assertEquals("sequence", fastq.getSequence()); - Assert.assertEquals("quality_", fastq.getQuality()); - Assert.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); + Assertions.assertEquals("description", fastqBuilder.getDescription()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals("description", fastq.getDescription()); + Assertions.assertEquals("sequence", fastq.getSequence()); + Assertions.assertEquals("quality_", fastq.getQuality()); + Assertions.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); } @Test - public void testBuildAppendSequence() + void testBuildAppendSequence() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -305,16 +306,16 @@ public void testBuildAppendSequence() .withQuality("quality_") .withVariant(FastqVariant.FASTQ_SOLEXA); Fastq fastq = fastqBuilder.build(); - Assert.assertEquals("description", fastqBuilder.getDescription()); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); - Assert.assertEquals("description", fastq.getDescription()); - Assert.assertEquals("sequence", fastq.getSequence()); - Assert.assertEquals("quality_", fastq.getQuality()); - Assert.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); + Assertions.assertEquals("description", fastqBuilder.getDescription()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals("description", fastq.getDescription()); + Assertions.assertEquals("sequence", fastq.getSequence()); + Assertions.assertEquals("quality_", fastq.getQuality()); + Assertions.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); } @Test - public void testBuildAppendQuality() + void testBuildAppendQuality() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -323,48 +324,48 @@ public void testBuildAppendQuality() .appendQuality("ity_") .withVariant(FastqVariant.FASTQ_SOLEXA); Fastq fastq = fastqBuilder.build(); - Assert.assertEquals("description", fastqBuilder.getDescription()); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); - Assert.assertEquals("description", fastq.getDescription()); - Assert.assertEquals("sequence", fastq.getSequence()); - Assert.assertEquals("quality_", fastq.getQuality()); - Assert.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); + Assertions.assertEquals("description", fastqBuilder.getDescription()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals("description", fastq.getDescription()); + Assertions.assertEquals("sequence", fastq.getSequence()); + Assertions.assertEquals("quality_", fastq.getQuality()); + Assertions.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); } @Test - public void testBuildNonMatchingSequenceQualityScoreLengthsBothNull() + void testBuildNonMatchingSequenceQualityScoreLengthsBothNull() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") .withVariant(FastqVariant.FASTQ_SOLEXA); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); } @Test - public void testBuildNonMatchingSequenceQualityScoreLengthsSequenceNull() + void testBuildNonMatchingSequenceQualityScoreLengthsSequenceNull() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") .withQuality("0123") .withVariant(FastqVariant.FASTQ_SOLEXA); - Assert.assertEquals(false, fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals(false, fastqBuilder.sequenceAndQualityLengthsMatch()); } @Test - public void testBuildNonMatchingSequenceQualityScoreLengthsQualityNull() + void testBuildNonMatchingSequenceQualityScoreLengthsQualityNull() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") .withSequence("ACTG") .withVariant(FastqVariant.FASTQ_SOLEXA); - Assert.assertEquals(false, fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals(false, fastqBuilder.sequenceAndQualityLengthsMatch()); } @Test - public void testBuildNonMatchingSequenceQualityScoreLengths0() + void testBuildNonMatchingSequenceQualityScoreLengths0() { try { @@ -375,7 +376,7 @@ public void testBuildNonMatchingSequenceQualityScoreLengths0() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build sequence length > quality length expected IllegalStateException"); + Assertions.fail("build sequence length > quality length expected IllegalStateException"); } catch (IllegalStateException e) { @@ -384,7 +385,7 @@ public void testBuildNonMatchingSequenceQualityScoreLengths0() } @Test - public void testBuildNonMatchingSequenceQualityScoreLengths1() + void testBuildNonMatchingSequenceQualityScoreLengths1() { try { @@ -395,7 +396,7 @@ public void testBuildNonMatchingSequenceQualityScoreLengths1() .withVariant(FastqVariant.FASTQ_SOLEXA); fastqBuilder.build(); - Assert.fail("build sequence length < quality length expected IllegalStateException"); + Assertions.fail("build sequence length < quality length expected IllegalStateException"); } catch (IllegalStateException e) { @@ -404,7 +405,7 @@ public void testBuildNonMatchingSequenceQualityScoreLengths1() } @Test - public void testBuildMultiple() + void testBuildMultiple() { FastqBuilder fastqBuilder = new FastqBuilder() .withDescription("description") @@ -414,12 +415,12 @@ public void testBuildMultiple() for (int i = 0; i < 10; i++) { Fastq fastq = fastqBuilder.withSequence("sequence" + i).build(); - Assert.assertEquals("description", fastqBuilder.getDescription()); - Assert.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); - Assert.assertEquals("description", fastq.getDescription()); - Assert.assertEquals("sequence" + i, fastq.getSequence()); - Assert.assertEquals("quality__", fastq.getQuality()); - Assert.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); + Assertions.assertEquals("description", fastqBuilder.getDescription()); + Assertions.assertTrue(fastqBuilder.sequenceAndQualityLengthsMatch()); + Assertions.assertEquals("description", fastq.getDescription()); + Assertions.assertEquals("sequence" + i, fastq.getSequence()); + Assertions.assertEquals("quality__", fastq.getQuality()); + Assertions.assertEquals(FastqVariant.FASTQ_SOLEXA, fastq.getVariant()); } } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java index 62d7ee9368..31102b251d 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java @@ -21,25 +21,26 @@ package org.biojava.nbio.genome.io.fastq; import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import org.junit.function.ThrowingRunnable; /** * Unit test for Fastq. */ -public final class FastqTest { +final class FastqTest { @Test - public void testConstructor() + void testConstructor() { Fastq fastq = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertNotNull(fastq); + Assertions.assertNotNull(fastq); try { new Fastq(null, "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.fail("ctr(null description) expected IllegalArgumentException"); + Assertions.fail("ctr(null description) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -48,7 +49,7 @@ public void testConstructor() try { new Fastq("description", null, "quality_", FastqVariant.FASTQ_SANGER); - Assert.fail("ctr(null sequence) expected IllegalArgumentException"); + Assertions.fail("ctr(null sequence) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -57,7 +58,7 @@ public void testConstructor() try { new Fastq("description", "sequence", null, FastqVariant.FASTQ_SANGER); - Assert.fail("ctr(null quality) expected IllegalArgumentException"); + Assertions.fail("ctr(null quality) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -66,7 +67,7 @@ public void testConstructor() try { new Fastq("description", "sequence", "quality_", null); - Assert.fail("ctr(null variant) expected IllegalArgumentException"); + Assertions.fail("ctr(null variant) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -75,45 +76,45 @@ public void testConstructor() } @Test - public void testDescription() + void testDescription() { Fastq fastq = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertTrue(fastq.getDescription() != null); - Assert.assertEquals("description", fastq.getDescription()); + Assertions.assertTrue(fastq.getDescription() != null); + Assertions.assertEquals("description", fastq.getDescription()); } @Test - public void testSequence() + void testSequence() { Fastq fastq = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertTrue(fastq.getSequence() != null); - Assert.assertEquals("sequence", fastq.getSequence()); + Assertions.assertTrue(fastq.getSequence() != null); + Assertions.assertEquals("sequence", fastq.getSequence()); } @Test - public void testQuality() + void testQuality() { Fastq fastq = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertTrue(fastq.getQuality() != null); - Assert.assertEquals("quality_", fastq.getQuality()); + Assertions.assertTrue(fastq.getQuality() != null); + Assertions.assertEquals("quality_", fastq.getQuality()); } @Test - public void testVariant() + void testVariant() { Fastq fastq = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertTrue(fastq.getVariant() != null); - Assert.assertEquals(FastqVariant.FASTQ_SANGER, fastq.getVariant()); + Assertions.assertTrue(fastq.getVariant() != null); + Assertions.assertEquals(FastqVariant.FASTQ_SANGER, fastq.getVariant()); } @Test - public void testBuilder() + void testBuilder() { - Assert.assertNotNull(Fastq.builder()); + Assertions.assertNotNull(Fastq.builder()); } @Test - public void testBuilderNullFastq() + void testBuilderNullFastq() { Assert.assertThrows(IllegalArgumentException.class, new ThrowingRunnable() { @Override @@ -124,39 +125,39 @@ public void run() { } @Test - public void testEquals() + void testEquals() { Fastq fastq0 = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); Fastq fastq1 = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertFalse(fastq0.equals(null)); - Assert.assertFalse(fastq1.equals(null)); - Assert.assertFalse(fastq0.equals(new Object())); - Assert.assertFalse(fastq1.equals(new Object())); - Assert.assertTrue(fastq0.equals(fastq0)); - Assert.assertTrue(fastq1.equals(fastq1)); - Assert.assertFalse(fastq0 == fastq1); - Assert.assertFalse(fastq0.equals(fastq1)); - Assert.assertFalse(fastq1.equals(fastq0)); + Assertions.assertFalse(fastq0.equals(null)); + Assertions.assertFalse(fastq1.equals(null)); + Assertions.assertFalse(fastq0.equals(new Object())); + Assertions.assertFalse(fastq1.equals(new Object())); + Assertions.assertTrue(fastq0.equals(fastq0)); + Assertions.assertTrue(fastq1.equals(fastq1)); + Assertions.assertFalse(fastq0 == fastq1); + Assertions.assertFalse(fastq0.equals(fastq1)); + Assertions.assertFalse(fastq1.equals(fastq0)); } @Test - public void testHashCode() + void testHashCode() { Fastq fastq0 = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); Fastq fastq1 = new Fastq("description", "sequence", "quality_", FastqVariant.FASTQ_SANGER); - Assert.assertEquals(fastq0.hashCode(), fastq0.hashCode()); - Assert.assertEquals(fastq1.hashCode(), fastq1.hashCode()); + Assertions.assertEquals(fastq0.hashCode(), fastq0.hashCode()); + Assertions.assertEquals(fastq1.hashCode(), fastq1.hashCode()); if (fastq0.equals(fastq1)) { - Assert.assertEquals(fastq0.hashCode(), fastq1.hashCode()); - Assert.assertEquals(fastq1.hashCode(), fastq0.hashCode()); + Assertions.assertEquals(fastq0.hashCode(), fastq1.hashCode()); + Assertions.assertEquals(fastq1.hashCode(), fastq0.hashCode()); } if (fastq1.equals(fastq0)) { - Assert.assertEquals(fastq0.hashCode(), fastq1.hashCode()); - Assert.assertEquals(fastq1.hashCode(), fastq0.hashCode()); + Assertions.assertEquals(fastq0.hashCode(), fastq1.hashCode()); + Assertions.assertEquals(fastq1.hashCode(), fastq0.hashCode()); } } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqVariantTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqVariantTest.java index f8b0855a8e..a47896b714 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqVariantTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqVariantTest.java @@ -22,66 +22,66 @@ import static org.biojava.nbio.genome.io.fastq.FastqVariant.*; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; /** * Unit test for FastqVariant. */ -public final class FastqVariantTest { +final class FastqVariantTest { @Test - public void testDescription() + void testDescription() { for (FastqVariant variant : values()) { - Assert.assertNotNull(variant.getDescription()); + Assertions.assertNotNull(variant.getDescription()); } } @Test - public void testIsSanger() + void testIsSanger() { - Assert.assertTrue(FASTQ_SANGER.isSanger()); - Assert.assertFalse(FASTQ_SOLEXA.isSanger()); - Assert.assertFalse(FASTQ_ILLUMINA.isSanger()); + Assertions.assertTrue(FASTQ_SANGER.isSanger()); + Assertions.assertFalse(FASTQ_SOLEXA.isSanger()); + Assertions.assertFalse(FASTQ_ILLUMINA.isSanger()); } @Test - public void testIsSolexa() + void testIsSolexa() { - Assert.assertFalse(FASTQ_SANGER.isSolexa()); - Assert.assertTrue(FASTQ_SOLEXA.isSolexa()); - Assert.assertFalse(FASTQ_ILLUMINA.isSolexa()); + Assertions.assertFalse(FASTQ_SANGER.isSolexa()); + Assertions.assertTrue(FASTQ_SOLEXA.isSolexa()); + Assertions.assertFalse(FASTQ_ILLUMINA.isSolexa()); } @Test - public void testIsIllumina() + void testIsIllumina() { - Assert.assertFalse(FASTQ_SANGER.isIllumina()); - Assert.assertFalse(FASTQ_SOLEXA.isIllumina()); - Assert.assertTrue(FASTQ_ILLUMINA.isIllumina()); + Assertions.assertFalse(FASTQ_SANGER.isIllumina()); + Assertions.assertFalse(FASTQ_SOLEXA.isIllumina()); + Assertions.assertTrue(FASTQ_ILLUMINA.isIllumina()); } @Test - public void testParseFastqVariant() + void testParseFastqVariant() { - Assert.assertEquals(null, parseFastqVariant(null)); - Assert.assertEquals(null, parseFastqVariant("")); - Assert.assertEquals(null, parseFastqVariant("not a valid FASTQ variant")); - Assert.assertEquals(FASTQ_SANGER, parseFastqVariant("FASTQ_SANGER")); - Assert.assertEquals(FASTQ_SANGER, parseFastqVariant("fastq-sanger")); + Assertions.assertEquals(null, parseFastqVariant(null)); + Assertions.assertEquals(null, parseFastqVariant("")); + Assertions.assertEquals(null, parseFastqVariant("not a valid FASTQ variant")); + Assertions.assertEquals(FASTQ_SANGER, parseFastqVariant("FASTQ_SANGER")); + Assertions.assertEquals(FASTQ_SANGER, parseFastqVariant("fastq-sanger")); } @Test - public void testQualityLessThanMinimumQualityScore() + void testQualityLessThanMinimumQualityScore() { for (FastqVariant variant : values()) { try { variant.quality(variant.minimumQualityScore() - 1); - Assert.fail("expected IllegalArgumentException"); + Assertions.fail("expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -91,14 +91,14 @@ public void testQualityLessThanMinimumQualityScore() } @Test - public void testQualityMoreThanMaximumQualityScore() + void testQualityMoreThanMaximumQualityScore() { for (FastqVariant variant : values()) { try { variant.quality(variant.maximumQualityScore() + 1); - Assert.fail("expected IllegalArgumentException"); + Assertions.fail("expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -108,13 +108,13 @@ public void testQualityMoreThanMaximumQualityScore() } @Test - public void testQualityQualityScoreRoundTrip() + void testQualityQualityScoreRoundTrip() { for (FastqVariant variant : values()) { for (int i = variant.minimumQualityScore(); i < (variant.maximumQualityScore() + 1); i++) { - Assert.assertEquals(i, variant.qualityScore(variant.quality(i))); + Assertions.assertEquals(i, variant.qualityScore(variant.quality(i))); } } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqReaderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqReaderTest.java index d7b0a8b9d2..1d2adf76b6 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqReaderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqReaderTest.java @@ -20,8 +20,8 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; -import static org.junit.Assert.*; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.IOException; import java.io.InputStream; @@ -31,7 +31,7 @@ /** * Unit test for IlluminaFastqReader. */ -public final class IlluminaFastqReaderTest +final class IlluminaFastqReaderTest extends AbstractFastqReaderTest { @@ -59,119 +59,119 @@ public FastqWriter createFastqWriter() } @Test - public void testValidateDescription() throws Exception + void testValidateDescription() throws Exception { IlluminaFastqReader reader = new IlluminaFastqReader(); URL invalidDescription = getClass().getResource("illumina-invalid-description.fastq"); try { reader.read(invalidDescription); - fail("read(invalidDescription) expected IOException"); + Assertions.fail("read(invalidDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("description must begin with a '@' character")); + Assertions.assertTrue(e.getMessage().contains("description must begin with a '@' character")); } } @Test - public void testValidateRepeatDescription() throws Exception + void testValidateRepeatDescription() throws Exception { IlluminaFastqReader reader = new IlluminaFastqReader(); URL invalidRepeatDescription = getClass().getResource("illumina-invalid-repeat-description.fastq"); try { reader.read(invalidRepeatDescription); - fail("read(invalidRepeatDescription) expected IOException"); + Assertions.fail("read(invalidRepeatDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("repeat description must match description")); + Assertions.assertTrue(e.getMessage().contains("repeat description must match description")); } } @Test - public void testWrappingAsIllumina() throws Exception + void testWrappingAsIllumina() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("wrapping_as_illumina.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(3, count); + Assertions.assertEquals(3, count); inputStream.close(); } @Test - public void testFullRangeAsIllumina() throws Exception + void testFullRangeAsIllumina() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("illumina_full_range_as_illumina.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(2, count); + Assertions.assertEquals(2, count); inputStream.close(); } @Test - public void testMiscDnaAsIllumina() throws Exception + void testMiscDnaAsIllumina() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_dna_as_illumina.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testMiscRnaAsIllumina() throws Exception + void testMiscRnaAsIllumina() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_rna_as_illumina.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testLongReadsAsIllumina() throws Exception + void testLongReadsAsIllumina() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("longreads_as_illumina.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(10, count); + Assertions.assertEquals(10, count); inputStream.close(); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqWriterTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqWriterTest.java index c9701595fa..384e204ff0 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqWriterTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/IlluminaFastqWriterTest.java @@ -21,12 +21,12 @@ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; +import org.junit.jupiter.api.Test; /** * Unit test for IlluminaFastqWriter. */ -public final class IlluminaFastqWriterTest +final class IlluminaFastqWriterTest extends AbstractFastqWriterTest { @@ -48,7 +48,7 @@ public Fastq createFastq() } @Test - public void testConvertNotIlluminaVariant() throws Exception + void testConvertNotIlluminaVariant() throws Exception { IlluminaFastqWriter writer = new IlluminaFastqWriter(); Appendable appendable = new StringBuilder(); diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqReaderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqReaderTest.java index af6f67319f..3e99a4cdc8 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqReaderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqReaderTest.java @@ -20,18 +20,17 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.IOException; import java.io.InputStream; import java.net.URL; -import static org.junit.Assert.*; - /** * Unit test for SangerFastqReader. */ -public final class SangerFastqReaderTest +final class SangerFastqReaderTest extends AbstractFastqReaderTest { @@ -65,197 +64,197 @@ public void testValidateDescription() throws Exception try { reader.read(invalidDescription); - fail("read(invalidDescription) expected IOException"); + Assertions.fail("read(invalidDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("description must begin with a '@' character")); + Assertions.assertTrue(e.getMessage().contains("description must begin with a '@' character")); } } @Test - public void testValidateRepeatDescription() throws Exception + void testValidateRepeatDescription() throws Exception { SangerFastqReader reader = new SangerFastqReader(); URL invalidRepeatDescription = getClass().getResource("sanger-invalid-repeat-description.fastq"); try { reader.read(invalidRepeatDescription); - fail("read(invalidRepeatDescription) expected IOException"); + Assertions.fail("read(invalidRepeatDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("repeat description must match description")); + Assertions.assertTrue(e.getMessage().contains("repeat description must match description")); } } @Test - public void testWrappingOriginal() throws Exception + void testWrappingOriginal() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("wrapping_original_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(3, count); + Assertions.assertEquals(3, count); inputStream.close(); } @Test - public void testWrappingAsSanger() throws Exception + void testWrappingAsSanger() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("wrapping_as_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(3, count); + Assertions.assertEquals(3, count); inputStream.close(); } @Test - public void testFullRangeOriginal() throws Exception + void testFullRangeOriginal() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("sanger_full_range_original_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(2, count); + Assertions.assertEquals(2, count); inputStream.close(); } @Test - public void testFullRangeAsSanger() throws Exception + void testFullRangeAsSanger() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("sanger_full_range_as_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(2, count); + Assertions.assertEquals(2, count); inputStream.close(); } @Test - public void testMiscDnaOriginal() throws Exception + void testMiscDnaOriginal() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_dna_original_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testMiscDnaAsSanger() throws Exception + void testMiscDnaAsSanger() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_dna_as_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testMiscRnaOriginal() throws Exception + void testMiscRnaOriginal() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_rna_original_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testMiscRnaAsSanger() throws Exception + void testMiscRnaAsSanger() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_rna_as_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testLongReadsOriginal() throws Exception + void testLongReadsOriginal() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("longreads_original_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(10, count); + Assertions.assertEquals(10, count); inputStream.close(); } @Test - public void testLongReadsAsSanger() throws Exception + void testLongReadsAsSanger() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("longreads_as_sanger.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(10, count); + Assertions.assertEquals(10, count); inputStream.close(); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqWriterTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqWriterTest.java index f94db84e0a..fcb1638c71 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqWriterTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SangerFastqWriterTest.java @@ -21,12 +21,12 @@ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; +import org.junit.jupiter.api.Test; /** * Unit test for SangerFastqWriter. */ -public final class SangerFastqWriterTest +final class SangerFastqWriterTest extends AbstractFastqWriterTest { @@ -48,7 +48,7 @@ public Fastq createFastq() } @Test - public void testConvertNotSangerVariant() throws Exception + void testConvertNotSangerVariant() throws Exception { SangerFastqWriter writer = new SangerFastqWriter(); Appendable appendable = new StringBuilder(); diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqReaderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqReaderTest.java index 5f6f041c84..cd87c53dc6 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqReaderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqReaderTest.java @@ -20,19 +20,18 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.IOException; import java.io.InputStream; import java.net.URL; -import static org.junit.Assert.*; - /** * Unit test for SolexaFastqReader. */ -public final class SolexaFastqReaderTest +final class SolexaFastqReaderTest extends AbstractFastqReaderTest { @@ -60,119 +59,119 @@ public FastqWriter createFastqWriter() } @Test - public void testValidateDescription() throws Exception + void testValidateDescription() throws Exception { SolexaFastqReader reader = new SolexaFastqReader(); URL invalidDescription = getClass().getResource("solexa-invalid-description.fastq"); try { reader.read(invalidDescription); - fail("read(invalidDescription) expected IOException"); + Assertions.fail("read(invalidDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("description must begin with a '@' character")); + Assertions.assertTrue(e.getMessage().contains("description must begin with a '@' character")); } } @Test - public void testValidateRepeatDescription() throws Exception + void testValidateRepeatDescription() throws Exception { SolexaFastqReader reader = new SolexaFastqReader(); URL invalidRepeatDescription = getClass().getResource("solexa-invalid-repeat-description.fastq"); try { reader.read(invalidRepeatDescription); - fail("read(invalidRepeatDescription) expected IOException"); + Assertions.fail("read(invalidRepeatDescription) expected IOException"); } catch (IOException e) { - assertTrue(e.getMessage().contains("repeat description must match description")); + Assertions.assertTrue(e.getMessage().contains("repeat description must match description")); } } @Test - public void testWrappingAsSolexa() throws Exception + void testWrappingAsSolexa() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("wrapping_as_solexa.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(3, count); + Assertions.assertEquals(3, count); inputStream.close(); } @Test - public void testFullRangeAsSolexa() throws Exception + void testFullRangeAsSolexa() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("solexa_full_range_as_solexa.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(2, count); + Assertions.assertEquals(2, count); inputStream.close(); } @Test - public void testMiscDnaAsSolexa() throws Exception + void testMiscDnaAsSolexa() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_dna_as_solexa.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testMiscRnaAsSolexa() throws Exception + void testMiscRnaAsSolexa() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("misc_rna_as_solexa.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(4, count); + Assertions.assertEquals(4, count); inputStream.close(); } @Test - public void testLongReadsAsSolexa() throws Exception + void testLongReadsAsSolexa() throws Exception { FastqReader reader = createFastqReader(); InputStream inputStream = getClass().getResourceAsStream("longreads_as_solexa.fastq"); Iterable iterable = reader.read(inputStream); - assertNotNull(iterable); + Assertions.assertNotNull(iterable); int count = 0; for (Fastq f : iterable) { - assertNotNull(f); + Assertions.assertNotNull(f); count++; } - assertEquals(10, count); + Assertions.assertEquals(10, count); inputStream.close(); } } diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqWriterTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqWriterTest.java index 2f2011e849..0927bf0cff 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqWriterTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/SolexaFastqWriterTest.java @@ -21,12 +21,12 @@ package org.biojava.nbio.genome.io.fastq; -import org.junit.Test; +import org.junit.jupiter.api.Test; /** * Unit test for SolexaFastqWriter. */ -public final class SolexaFastqWriterTest +final class SolexaFastqWriterTest extends AbstractFastqWriterTest { @@ -48,7 +48,7 @@ public Fastq createFastq() } @Test - public void testConvertNotSolexaVariant() throws Exception + void testConvertNotSolexaVariant() throws Exception { SolexaFastqWriter writer = new SolexaFastqWriter(); Appendable appendable = new StringBuilder(); diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/StreamingFastqParserTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/StreamingFastqParserTest.java index a80f44a43d..02d49d3177 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/StreamingFastqParserTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/StreamingFastqParserTest.java @@ -20,8 +20,8 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; import java.io.StringReader; @@ -29,10 +29,10 @@ /** * Unit test for StreamingFastqParser. */ -public class StreamingFastqParserTest { +class StreamingFastqParserTest { @Test - public void testStreamNullReadable() throws Exception + void testStreamNullReadable() throws Exception { try { @@ -42,7 +42,7 @@ public void fastq(final Fastq fastq) { // empty } }); - Assert.fail("stream(null,,) expected IllegalArgumentException"); + Assertions.fail("stream(null,,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -51,7 +51,7 @@ public void fastq(final Fastq fastq) { } @Test - public void testStreamNullVariant() throws Exception + void testStreamNullVariant() throws Exception { try { @@ -62,7 +62,7 @@ public void fastq(final Fastq fastq) { // empty } }); - Assert.fail("stream(null,,) expected IllegalArgumentException"); + Assertions.fail("stream(null,,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { @@ -71,13 +71,13 @@ public void fastq(final Fastq fastq) { } @Test - public void testStreamNullListener() throws Exception + void testStreamNullListener() throws Exception { try { final String input = ""; StreamingFastqParser.stream(new StringReader(input), FastqVariant.FASTQ_SANGER, null); - Assert.fail("stream(null,,) expected IllegalArgumentException"); + Assertions.fail("stream(null,,) expected IllegalArgumentException"); } catch (IllegalArgumentException e) { From 91975bd73199078cdcf465d73bf8e0b5f02935e5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:51:44 +0000 Subject: [PATCH 03/26] Bump org.apache.logging.log4j:log4j-api from 2.25.4 to 2.25.5 Bumps org.apache.logging.log4j:log4j-api from 2.25.4 to 2.25.5. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-version: 2.25.5 dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index aeaafe6ffe..79db94ee26 100644 --- a/pom.xml +++ b/pom.xml @@ -41,7 +41,7 @@ 512M 1.0.11 2.0.12 - 2.25.4 + 2.25.5 5.10.1 ciftools-java 7.0.1 From f061f5718b597ae63654c0c4618f5e7ba03ba650 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:27:28 -0400 Subject: [PATCH 04/26] Fix FileDownloadUtils validation and implement checksum verification The download-validation helpers added in 7.0.0 (#979, #980) had several gaps that only surface once a caller passes a real hash URL or downloads from a server that can 404. All of them are fixed here, and hash verification is implemented rather than stubbed. Correctness fixes: * createValidationFiles(URL, ...) passed a literal Hash.UNKNOWN to its URLConnection overload instead of the caller's argument, so it could never write a hash file and threw IllegalArgumentException for any caller that supplied a hashURL. * Neither downloadFile nor createValidationFiles checked the HTTP status, so a 404 error page was written into the cache as though it were the requested file. Because the .size sidecar was then taken from that same error response, validateFile subsequently declared it valid. A new HttpStatusException lets callers tell "the resource is not there" apart from a transport failure, which matters for anything that tries several mirrors in turn. * downloadFile used FileChannel.transferFrom(rbc, 0, Long.MAX_VALUE), which is not guaranteed to drain a socket-backed channel and could silently truncate a download. Replaced with Files.copy, which loops to end of stream. * downloadFile leaked its temporary file on every failure path. * validateFile threw NullPointerException for a file with no parent directory, and again if listFiles() returned null; an empty .size file raised an unchecked NoSuchElementException that escaped the surrounding catch. * validateFile checked only the first hash sidecar it found, ignoring the rest. New functionality: * validateFile now really verifies MD5, SHA-1 and SHA-256 instead of throwing UnsupportedOperationException. Sidecars are written as bare lowercase hex and parsed tolerantly, so a file downloaded verbatim from a server in coreutils or BSD layout is also understood. A sidecar that cannot be parsed is skipped with a warning rather than failing an otherwise good download. * ETagPolicy lets an ETag that is a bare hex digest be recorded as a checksum without a second request. files.wwpdb.org and files.rcsb.org return the content MD5 as the ETag, so every download from the wwPDB archive now gets a real integrity check for free. The - ETags used by the EBI servers contain a dash and can never be misread as a digest; a test pins that. * downloadFileWithValidation downloads and validates over a single connection. The previous pattern opened one connection to read Content-Length and another to fetch the bytes, so the recorded size described a different response than the one written; if the resource changed in between, the cache entry was left permanently failing validation. Content is digested while streaming to a temp file and only moved into place once length and checksum check out. LocalPDBDirectory uses the new single-connection download, and its two-character directory hash is promoted to the reusable getMiddleHash(String). That hash deliberately counts from the end of the identifier so that both spellings of an entry land in the same bucket: 1cbs and pdb_00001cbs both give "cb", where counting from the start would file the extended form under "db". Defaults change slightly: the existing four-argument createValidationFiles overloads now use ETagPolicy.USE_IF_HEX_DIGEST, so callers start recording checksums where the server offers one. Targeted at 7.3.0. --- .../nbio/core/util/FileDownloadUtils.java | 621 ++++++++++++++++-- .../nbio/core/util/HttpStatusException.java | 84 +++ .../nbio/core/util/FileDownloadUtilsTest.java | 218 +++++- .../nbio/structure/io/LocalPDBDirectory.java | 35 +- 4 files changed, 883 insertions(+), 75 deletions(-) create mode 100644 biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java diff --git a/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java b/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java index 0b132b180e..5b8c656658 100644 --- a/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java +++ b/biojava-core/src/main/java/org/biojava/nbio/core/util/FileDownloadUtils.java @@ -21,9 +21,9 @@ */ package org.biojava.nbio.core.util; +import java.io.BufferedInputStream; import java.io.File; import java.io.FileNotFoundException; -import java.io.FileOutputStream; import java.io.FilenameFilter; import java.io.IOException; import java.io.InputStream; @@ -32,11 +32,15 @@ import java.net.SocketTimeoutException; import java.net.URL; import java.net.URLConnection; -import java.nio.channels.Channels; -import java.nio.channels.ReadableByteChannel; +import java.nio.charset.StandardCharsets; import java.nio.file.*; import java.nio.file.attribute.BasicFileAttributes; +import java.security.DigestInputStream; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.util.Scanner; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,10 +51,45 @@ public class FileDownloadUtils { private static final String HASH_EXT = ".hash"; private static final Logger logger = LoggerFactory.getLogger(FileDownloadUtils.class); + /** Buffer used when streaming a file through a {@link MessageDigest}. */ + private static final int DIGEST_BUFFER_SIZE = 64 * 1024; + + /** A bare hex digest, optionally followed by whitespace and a file name (the + * layout written by md5sum, sha1sum and friends). */ + private static final Pattern BARE_HEX_HASH = Pattern.compile("^([0-9a-fA-F]{32,128})(?:[\\s*].*)?$"); + + /** The BSD layout, e.g. MD5 (file.txt) = d41d8cd9.... */ + private static final Pattern BSD_HASH = Pattern.compile("^\\w+\\s*\\(.*\\)\\s*=\\s*([0-9a-fA-F]{32,128})$"); + public enum Hash{ MD5, SHA1, SHA256, UNKNOWN } + /** + * What to do with the ETag response header when no explicit hash + * URL is available. + *

+ * Some archives — notably files.wwpdb.org and + * files.rcsb.org — return the MD5 digest of the content as + * the bare ETag value, which lets us record a real checksum + * without a second request. Others (the EBI servers, for instance) return a + * <modification-time>-<size> form instead; because that + * always contains a -, it can never be mistaken for a hex digest. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public enum ETagPolicy { + /** Never look at the ETag header. */ + IGNORE, + /** Record the ETag as a checksum when it is a bare hex digest + * of a length matching one of the supported algorithms. */ + USE_IF_HEX_DIGEST, + /** As {@link #USE_IF_HEX_DIGEST}, but log a warning when the header is + * missing or is not a usable digest. */ + REQUIRE + } + /** * Gets the file extension of a file, excluding '.'. * If the file name has no extension the file name is returned. @@ -95,44 +134,158 @@ public static void downloadFile(URL url, File destination) throws IOException { int maxTries = 10; int timeout = 60000; //60 sec - File tempFile = Files.createTempFile(getFilePrefix(destination), "." + getFileExtension(destination)).toFile(); + File tempFile = createTempFileFor(destination); - // Took following recipe from stackoverflow: - // http://stackoverflow.com/questions/921262/how-to-download-and-save-a-file-from-internet-using-java - // It seems to be the most efficient way to transfer a file - // See: http://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileChannel.html - ReadableByteChannel rbc = null; - FileOutputStream fos = null; - while (true) { - try { - URLConnection connection = prepareURLConnection(url.toString(), timeout); - connection.connect(); - InputStream inputStream = connection.getInputStream(); - - rbc = Channels.newChannel(inputStream); - fos = new FileOutputStream(tempFile); - fos.getChannel().transferFrom(rbc, 0, Long.MAX_VALUE); - break; - } catch (SocketTimeoutException e) { - if (++count == maxTries) throw e; - } finally { - if (rbc != null) { - rbc.close(); - } - if (fos != null) { - fos.close(); + try { + while (true) { + try { + URLConnection connection = prepareURLConnection(url.toString(), timeout); + connection.connect(); + checkHttpStatus(connection); + try (InputStream inputStream = connection.getInputStream()) { + // Files.copy loops until end of stream. FileChannel.transferFrom(), used + // here previously, is not guaranteed to drain a socket-backed channel in + // a single call and could silently truncate a download. + Files.copy(inputStream, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + break; + } catch (SocketTimeoutException e) { + if (++count == maxTries) throw e; } } + + logger.debug("Copying temp file [{}] to final location [{}]", tempFile, destination); + Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + } finally { + // on every path, including failure: the temp file used to leak whenever + // the download threw. + deleteQuietly(tempFile); } + } + + /** + * Downloads a file and writes its validation metadata in a single pass over a + * single connection. + *

+ * This is preferable to calling {@link #createValidationFiles(URL, File, URL, Hash)} + * followed by {@link #downloadFile(URL, File)}: those open separate connections, + * so the Content-Length recorded in the .size file + * comes from a different response than the bytes actually written. If the + * resource changes between the two requests, the cache entry is left + * permanently failing validation. + *

+ * The content is streamed to a temporary file and only moved into place once + * the declared length and (where available) the checksum have been confirmed, + * so a failed download never leaves a partial file at destination. + * + * @param url the remote file to download + * @param destination the local file to download into. Its parent directory must exist. + * @param hashURL URL of a file containing the expected hash. May be null. + * @param hash the hashing algorithm matching hashURL. Ignored when + * hashURL is null. + * @param eTagPolicy what to do with an ETag response header when + * hashURL is null. May be null, + * which is treated as {@link ETagPolicy#IGNORE}. + * @throws HttpStatusException if the server answered with a non-2xx status + * @throws IOException if the transfer failed, or the transferred content did not + * match the length or checksum the server declared + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void downloadFileWithValidation(URL url, File destination, URL hashURL, Hash hash, + ETagPolicy eTagPolicy) throws IOException { + int timeout = 60000; //60 sec + ETagPolicy policy = eTagPolicy == null ? ETagPolicy.IGNORE : eTagPolicy; + + File tempFile = createTempFileFor(destination); + try { + URLConnection connection = prepareURLConnection(url.toString(), timeout); + connection.connect(); + checkHttpStatus(connection); - logger.debug("Copying temp file [{}] to final location [{}]", tempFile, destination); - Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + long declaredSize = connection.getContentLengthLong(); + String eTag = connection.getHeaderField("ETag"); - // delete the tmp file - tempFile.delete(); + // Only digest when we have something to compare against; hashing every + // download would cost CPU for no benefit. + Hash eTagHash = policy == ETagPolicy.IGNORE ? Hash.UNKNOWN : hashFromETag(eTag); + if (policy == ETagPolicy.REQUIRE && eTagHash == Hash.UNKNOWN) { + logger.warn("ETag [{}] of {} is not a usable hex digest; no checksum will be recorded.", eTag, url); + } + + MessageDigest digest = eTagHash == Hash.UNKNOWN ? null : newDigest(eTagHash); + long written; + try (InputStream raw = connection.getInputStream(); + InputStream in = digest == null ? raw : new DigestInputStream(raw, digest)) { + written = Files.copy(in, tempFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + + if (declaredSize >= 0 && written != declaredSize) { + throw new IOException(String.format( + "Incomplete download of %s: got %d bytes but the server declared %d.", + url, written, declaredSize)); + } + + String actualDigest = digest == null ? null : toHex(digest.digest()); + if (actualDigest != null && !actualDigest.equalsIgnoreCase(normalizeETag(eTag))) { + throw new IOException(String.format( + "Corrupt download of %s: %s of the content is %s but the server's ETag says %s.", + url, eTagHash, actualDigest, normalizeETag(eTag))); + } + + moveIntoPlace(tempFile, destination); + // Sidecars are written only once the content is known good, so a failed + // download can never leave validation metadata describing a file that is + // not there. + writeSizeFile(destination, written); + if (hashURL != null) { + if (hash == null || hash == Hash.UNKNOWN) { + throw new IllegalArgumentException("Hash URL given but algorithm is unknown"); + } + downloadFile(hashURL, hashFileFor(destination, hash)); + } else if (actualDigest != null) { + writeHashFile(destination, eTagHash, actualDigest); + } + } finally { + deleteQuietly(tempFile); + } } - + + /** + * Verifies that an HTTP connection returned a 2xx status. Connections using a + * non-HTTP protocol (file:, ftp:, ...) are left alone. + *

+ * Without this check a 404 error page is written into the cache as though it + * were the requested file — and because the .size sidecar is + * then taken from that same error response, {@link #validateFile(File)} would + * subsequently declare it valid. + * + * @param connection an already connected {@link URLConnection} + * @throws HttpStatusException if the status is outside the 2xx range + * @throws IOException if the status could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void checkHttpStatus(URLConnection connection) throws IOException { + if (!(connection instanceof HttpURLConnection)) { + return; + } + HttpURLConnection http = (HttpURLConnection) connection; + int code = http.getResponseCode(); + if (code >= 200 && code < 300) { + return; + } + if (code == 301 || code == 302 || code == 307 || code == 308) { + // The JDK follows redirects automatically, but never across protocols, so + // an http -> https redirect surfaces here and is worth naming explicitly. + logger.warn("{} returned redirect {} to [{}], which was not followed " + + "(the JDK does not follow redirects that change protocol).", + connection.getURL(), code, http.getHeaderField("Location")); + } + throw new HttpStatusException(code, connection.getURL().toString(), http.getResponseMessage()); + } + /** * Creates validation files beside a file to be downloaded.
* Whenever possible, for a file.ext file, it creates @@ -146,9 +299,27 @@ public static void downloadFile(URL url, File destination) throws IOException { * @param hash The Hashing algorithm. Ignored if hashURL is null. */ public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash){ + createValidationFiles(url, localDestination, hashURL, hash, ETagPolicy.USE_IF_HEX_DIGEST); + } + + /** + * Creates validation files beside a file to be downloaded, with explicit control + * over how the ETag response header is treated. + * + * @param url the remote file URL to download + * @param localDestination the local file to download into + * @param hashURL the URL of the hash file to download. Can be null. + * @param hash The Hashing algorithm. Ignored if hashURL is null. + * @param eTagPolicy how to treat the ETag header when hashURL + * is null. May be null, treated as {@link ETagPolicy#IGNORE}. + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void createValidationFiles(URL url, File localDestination, URL hashURL, Hash hash, + ETagPolicy eTagPolicy){ try { URLConnection resourceConnection = url.openConnection(); - createValidationFiles(resourceConnection, localDestination, hashURL, FileDownloadUtils.Hash.UNKNOWN); + createValidationFiles(resourceConnection, localDestination, hashURL, hash, eTagPolicy); } catch (IOException e) { logger.warn("could not open connection to resource file due to exception: {}", e.getMessage()); } @@ -169,31 +340,246 @@ public static void createValidationFiles(URL url, File localDestination, URL has * @since 7.0.0 */ public static void createValidationFiles(URLConnection resourceUrlConnection, File localDestination, URL hashURL, Hash hash){ + createValidationFiles(resourceUrlConnection, localDestination, hashURL, hash, ETagPolicy.USE_IF_HEX_DIGEST); + } + + /** + * Creates validation files beside a file to be downloaded, with explicit control + * over how the ETag response header is treated. + *

+ * Nothing is written when the connection reports a non-2xx status: previously an + * error page's Content-Length would be recorded as the expected + * size, so the cached error page then passed validation. + * + * @param resourceUrlConnection the remote file URLConnection to download + * @param localDestination the local file to download into + * @param hashURL the URL of the hash file to download. Can be null. + * @param hash The Hashing algorithm. Ignored if hashURL is null. + * @param eTagPolicy how to treat the ETag header when hashURL + * is null. May be null, treated as {@link ETagPolicy#IGNORE}. + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void createValidationFiles(URLConnection resourceUrlConnection, File localDestination, URL hashURL, + Hash hash, ETagPolicy eTagPolicy){ + try { + checkHttpStatus(resourceUrlConnection); + } catch (IOException e) { + logger.warn("Not writing validation metadata for {}: {}", resourceUrlConnection.getURL(), e.getMessage()); + return; + } + long size = resourceUrlConnection.getContentLengthLong(); if(size == -1) { logger.debug("Could not find expected file size for resource {}. Size validation metadata file won't be available for this download.", resourceUrlConnection.getURL()); } else { logger.debug("Content-Length: {}", size); - File sizeFile = new File(localDestination.getParentFile(), localDestination.getName() + SIZE_EXT); - try (PrintStream sizePrintStream = new PrintStream(sizeFile)) { - sizePrintStream.print(size); - } catch (FileNotFoundException e) { - logger.warn("Could not write size validation metadata file due to exception: {}", e.getMessage()); - } + writeSizeFile(localDestination, size); } - - if(hashURL == null) + + if(hashURL == null) { + ETagPolicy policy = eTagPolicy == null ? ETagPolicy.IGNORE : eTagPolicy; + if (policy != ETagPolicy.IGNORE) { + String eTag = resourceUrlConnection.getHeaderField("ETag"); + Hash eTagHash = hashFromETag(eTag); + if (eTagHash == Hash.UNKNOWN) { + if (policy == ETagPolicy.REQUIRE) { + logger.warn("ETag [{}] of {} is not a usable hex digest; no checksum recorded.", + eTag, resourceUrlConnection.getURL()); + } + } else { + writeHashFile(localDestination, eTagHash, normalizeETag(eTag)); + } + } return; + } - if(hash == Hash.UNKNOWN) + if(hash == null || hash == Hash.UNKNOWN) throw new IllegalArgumentException("Hash URL given but algorithm is unknown"); try { - File hashFile = new File(localDestination.getParentFile(), String.format("%s%s_%s", localDestination.getName(), HASH_EXT, hash)); - downloadFile(hashURL, hashFile); + downloadFile(hashURL, hashFileFor(localDestination, hash)); } catch (IOException e) { logger.warn("Could not write validation hash file due to exception: {}", e.getMessage()); } } + + /** + * Determines which hashing algorithm an ETag header value + * represents, based on the length of the hex digest it contains. + *

+ * Only a value consisting solely of hex characters is considered. The + * <time>-<size> form used by nginx and Apache always + * contains a - and therefore never matches. + * + * @param eTagHeaderValue the raw header value, possibly quoted or weak-prefixed. + * May be null. + * @return the matching algorithm, or {@link Hash#UNKNOWN} if the value is not a + * hex digest of a recognised length + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static Hash hashFromETag(String eTagHeaderValue) { + String value = normalizeETag(eTagHeaderValue); + if (value == null || !value.matches("[0-9a-fA-F]+")) { + return Hash.UNKNOWN; + } + switch (value.length()) { + case 32: return Hash.MD5; + case 40: return Hash.SHA1; + case 64: return Hash.SHA256; + default: return Hash.UNKNOWN; + } + } + + /** + * Strips the weak-validator prefix and surrounding quotes from an + * ETag header value. + * + * @param eTagHeaderValue the raw header value. May be null. + * @return the bare value, or null if the input was null + * or blank + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String normalizeETag(String eTagHeaderValue) { + if (eTagHeaderValue == null) { + return null; + } + String value = eTagHeaderValue.trim(); + if (value.startsWith("W/")) { + value = value.substring(2).trim(); + } + if (value.length() >= 2 && value.startsWith("\"") && value.endsWith("\"")) { + value = value.substring(1, value.length() - 1); + } + return value.isEmpty() ? null : value; + } + + /** + * Writes a <name>.hash_<ALGORITHM> sidecar file + * containing the given digest as bare lowercase hex. + * + * @param localDestination the file the digest describes + * @param hash the hashing algorithm + * @param hexDigest the digest, in hex + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static void writeHashFile(File localDestination, Hash hash, String hexDigest) { + if (hash == null || hash == Hash.UNKNOWN || hexDigest == null) { + return; + } + File hashFile = hashFileFor(localDestination, hash); + try (PrintStream out = new PrintStream(hashFile, StandardCharsets.UTF_8.name())) { + out.println(hexDigest.toLowerCase()); + } catch (IOException e) { + logger.warn("Could not write validation hash file due to exception: {}", e.getMessage()); + } + } + + /** + * Computes the digest of a file. + * + * @param file the file to digest + * @param hash the algorithm to use + * @return the digest as bare lowercase hex + * @throws IOException if the file could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String computeHash(File file, Hash hash) throws IOException { + try (InputStream in = new BufferedInputStream(Files.newInputStream(file.toPath()), DIGEST_BUFFER_SIZE)) { + return computeHash(in, hash); + } + } + + /** + * Computes the digest of a stream. The stream is read to its end but not closed. + * + * @param in the stream to digest + * @param hash the algorithm to use + * @return the digest as bare lowercase hex + * @throws IOException if the stream could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String computeHash(InputStream in, Hash hash) throws IOException { + MessageDigest digest = newDigest(hash); + byte[] buffer = new byte[DIGEST_BUFFER_SIZE]; + int read; + while ((read = in.read(buffer)) != -1) { + digest.update(buffer, 0, read); + } + return toHex(digest.digest()); + } + + /** + * Checks a file against an expected digest. + * + * @param file the file to check + * @param hash the algorithm to use + * @param expectedHex the expected digest, in hex; compared case-insensitively + * @return true if the digests match + * @throws IOException if the file could not be read + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static boolean verifyHash(File file, Hash hash, String expectedHex) throws IOException { + return expectedHex != null && expectedHex.trim().equalsIgnoreCase(computeHash(file, hash)); + } + + /** + * The JDK name of a hashing algorithm, which differs from the enum constant for + * the SHA variants. + * + * @param hash the algorithm + * @return the name to pass to {@link MessageDigest#getInstance(String)} + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + public static String getAlgorithmName(Hash hash) { + switch (hash) { + case MD5: return "MD5"; + case SHA1: return "SHA-1"; + case SHA256: return "SHA-256"; + default: throw new IllegalArgumentException("Hashing algorithm not known: " + hash); + } + } + + /** + * Reads the expected digest out of a .hash_XXXX sidecar file. + *

+ * The file may have been downloaded verbatim from a server, so several common + * layouts are accepted: a bare hex digest, the md5sum style + * <hex>  <filename>, and the BSD style + * MD5 (<filename>) = <hex>. + * + * @param hashFile the sidecar file + * @return the digest in hex, or null if nothing recognisable was found + */ + static String parseHashFile(File hashFile) { + try (Scanner scanner = new Scanner(hashFile, StandardCharsets.UTF_8.name())) { + while (scanner.hasNextLine()) { + String line = scanner.nextLine().trim(); + if (line.isEmpty()) { + continue; + } + Matcher bare = BARE_HEX_HASH.matcher(line); + if (bare.matches()) { + return bare.group(1); + } + Matcher bsd = BSD_HASH.matcher(line); + if (bsd.matches()) { + return bsd.group(1); + } + return null; // first meaningful line was not a digest + } + } catch (IOException e) { + logger.warn("Could not read hash file [{}]: {}", hashFile, e.getMessage()); + } + return null; + } + /** * Validate a local file based on pre-existing metadata files for size and hash.
@@ -210,45 +596,154 @@ public static void createValidationFiles(URLConnection resourceUrlConnection, Fi * @since 7.0.0 */ public static boolean validateFile(File localFile) { - File sizeFile = new File(localFile.getParentFile(), localFile.getName() + SIZE_EXT); + // getParentFile() is null for a bare relative name such as new File("x.cif"), + // which used to make this method throw a NullPointerException. + File parent = localFile.getAbsoluteFile().getParentFile(); + if (parent == null) { + logger.debug("Cannot determine the parent directory of [{}]; nothing to validate against.", localFile); + return true; + } + + File sizeFile = new File(parent, localFile.getName() + SIZE_EXT); if(sizeFile.exists()) { try (Scanner scanner = new Scanner(sizeFile)) { - long expectedSize = scanner.nextLong(); - long actualSize = localFile.length(); - if (expectedSize != actualSize) { - logger.warn("File [{}] size ({}) does not match expected size ({}).", localFile, actualSize, expectedSize); - return false; + if (!scanner.hasNextLong()) { + // An empty or truncated .size file used to raise an unchecked + // NoSuchElementException that escaped the catch below. + logger.warn("Size metadata file [{}] is empty or malformed; skipping size validation.", sizeFile); + } else { + long expectedSize = scanner.nextLong(); + long actualSize = localFile.length(); + if (expectedSize != actualSize) { + logger.warn("File [{}] size ({}) does not match expected size ({}).", localFile, actualSize, expectedSize); + return false; + } } } catch (FileNotFoundException e) { logger.warn("could not validate size of file [{}] because no size metadata file exists.", localFile); } } - File[] hashFiles = localFile.getParentFile().listFiles(new FilenameFilter() { - final String hashPattern = String.format("%s%s_(%s|%s|%s)", localFile.getName(), HASH_EXT, Hash.MD5, Hash.SHA1, Hash.SHA256); + File[] hashFiles = parent.listFiles(new FilenameFilter() { + final String hashPattern = String.format("%s%s_(%s|%s|%s)", Pattern.quote(localFile.getName()), HASH_EXT, Hash.MD5, Hash.SHA1, Hash.SHA256); @Override public boolean accept(File dir, String name) { return name.matches(hashPattern); } }); - if(hashFiles.length > 0) { - File hashFile = hashFiles[0]; + // listFiles() returns null if the parent is not a directory or cannot be read. + if (hashFiles == null || hashFiles.length == 0) { + return true; + } + + // Verify against every sidecar present, not only the first one found. + for (File hashFile : hashFiles) { String name = hashFile.getName(); String algo = name.substring(name.lastIndexOf('_') + 1); - switch (Hash.valueOf(algo)) { - case MD5: - case SHA1: - case SHA256: - throw new UnsupportedOperationException("Not yet implemented"); - case UNKNOWN: - default: // No need. Already checked above + Hash hash; + try { + hash = Hash.valueOf(algo); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Hashing algorithm not known: " + algo, e); + } + if (hash == Hash.UNKNOWN) { throw new IllegalArgumentException("Hashing algorithm not known: " + algo); } + + String expected = parseHashFile(hashFile); + if (expected == null) { + // A sidecar we cannot read should not condemn an otherwise good download. + logger.warn("Could not read a digest from [{}]; skipping {} validation of [{}].", hashFile, hash, localFile); + continue; + } + try { + if (!verifyHash(localFile, hash, expected)) { + logger.warn("File [{}] {} does not match the expected digest {}.", localFile, hash, expected); + return false; + } + } catch (IOException e) { + logger.warn("Could not compute the {} of [{}]: {}", hash, localFile, e.getMessage()); + return false; + } } - + return true; } + /** + * The <name>.hash_<ALGORITHM> sidecar file for a + * downloaded file. + */ + private static File hashFileFor(File localDestination, Hash hash) { + return new File(localDestination.getAbsoluteFile().getParentFile(), + String.format("%s%s_%s", localDestination.getName(), HASH_EXT, hash)); + } + + /** + * Writes the <name>.size sidecar file. + */ + private static void writeSizeFile(File localDestination, long size) { + File sizeFile = new File(localDestination.getAbsoluteFile().getParentFile(), + localDestination.getName() + SIZE_EXT); + try (PrintStream sizePrintStream = new PrintStream(sizeFile, StandardCharsets.UTF_8.name())) { + sizePrintStream.print(size); + } catch (IOException e) { + logger.warn("Could not write size validation metadata file due to exception: {}", e.getMessage()); + } + } + + private static MessageDigest newDigest(Hash hash) { + try { + return MessageDigest.getInstance(getAlgorithmName(hash)); + } catch (NoSuchAlgorithmException e) { + // MD5, SHA-1 and SHA-256 are required of every Java platform. + throw new IllegalStateException("Required hashing algorithm is unavailable: " + hash, e); + } + } + + private static String toHex(byte[] bytes) { + StringBuilder sb = new StringBuilder(bytes.length * 2); + for (byte b : bytes) { + sb.append(Character.forDigit((b >> 4) & 0xF, 16)); + sb.append(Character.forDigit(b & 0xF, 16)); + } + return sb.toString(); + } + + /** + * Creates a temp file whose name is derived from the destination. + * {@link Files#createTempFile} rejects prefixes shorter than 3 characters, so + * short names are padded. + */ + private static File createTempFileFor(File destination) throws IOException { + String prefix = getFilePrefix(destination); + while (prefix.length() < 3) { + prefix = prefix + "_"; + } + return Files.createTempFile(prefix, "." + getFileExtension(destination)).toFile(); + } + + private static void moveIntoPlace(File tempFile, File destination) throws IOException { + try { + Files.move(tempFile.toPath(), destination.toPath(), + StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + // The temp directory is often on a different filesystem than the cache. + Files.copy(tempFile.toPath(), destination.toPath(), StandardCopyOption.REPLACE_EXISTING); + } + } + + private static void deleteQuietly(File file) { + if (file == null) { + return; + } + try { + Files.deleteIfExists(file.toPath()); + } catch (IOException e) { + logger.debug("Could not delete temporary file [{}]: {}", file, e.getMessage()); + } + } + /** * Converts path to Unix convention and adds a terminating slash if it was * omitted. diff --git a/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java b/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java new file mode 100644 index 0000000000..c74c6ffb05 --- /dev/null +++ b/biojava-core/src/main/java/org/biojava/nbio/core/util/HttpStatusException.java @@ -0,0 +1,84 @@ +/** + * BioJava development code + * + * This code may be freely distributed and modified under the terms of the GNU + * Lesser General Public Licence. This should be distributed with the code. If + * you do not have a copy, see: + * + * http://www.gnu.org/copyleft/lesser.html + * + * Copyright for this code is held jointly by the individual authors. These + * should be listed in @author doc comments. + * + * For more information on the BioJava project and its aims, or to join the + * biojava-l mailing list, visit the home page at: + * + * http://www.biojava.org/ + */ +package org.biojava.nbio.core.util; + +import java.io.IOException; + +/** + * Signals that an HTTP request completed but returned a status code outside the + * 2xx range. + *

+ * This exists so that callers can tell apart the two very different reasons a + * download can fail: + *

    + *
  • The resource does not exist ({@link #isNotFound()}, i.e. HTTP 404 or + * 410). For a caller that tries several mirrors or several alternative data + * sources in turn, this simply means "not here, try the next one".
    • + *
  • Anything else — a server error, a redirect that was not + * followed, an authentication failure. These usually mean the whole operation + * should be abandoned rather than silently treated as "no data available".
    • + *
+ * Without a distinct exception type the only way to tell these apart is by + * parsing the message of a plain {@link IOException}, which is brittle. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ +public class HttpStatusException extends IOException { + + private static final long serialVersionUID = 1L; + + private final int statusCode; + private final String url; + + /** + * @param statusCode the HTTP status code returned by the server + * @param url the URL that was requested + * @param responseMessage the HTTP reason phrase, may be null + */ + public HttpStatusException(int statusCode, String url, String responseMessage) { + super(String.format("HTTP %d%s for %s", statusCode, + responseMessage == null || responseMessage.isEmpty() ? "" : " " + responseMessage, url)); + this.statusCode = statusCode; + this.url = url; + } + + /** + * @return the HTTP status code returned by the server + */ + public int getStatusCode() { + return statusCode; + } + + /** + * @return the URL that was requested + */ + public String getUrl() { + return url; + } + + /** + * Whether the status indicates that the resource is simply not there, as + * opposed to a transport or server problem. + * + * @return true for HTTP 404 (Not Found) and 410 (Gone) + */ + public boolean isNotFound() { + return statusCode == 404 || statusCode == 410; + } +} diff --git a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java index 201ad88e48..1177c1d1fc 100644 --- a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java +++ b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java @@ -4,6 +4,7 @@ import static org.biojava.nbio.core.util.FileDownloadUtils.getFilePrefix; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -11,6 +12,7 @@ import java.io.IOException; import java.io.PrintStream; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import org.junit.jupiter.api.Nested; @@ -187,17 +189,223 @@ void testValidationFiles() throws IOException{ assertTrue(sizeFile.exists(), "couldn't create size file"); assertTrue(FileDownloadUtils.validateFile(destFile), "file not detected to be invalid although there is correct size validation file"); + // files.wwpdb.org returns the content MD5 as the ETag, so the default + // ETag policy records a real checksum without a separate hash URL. + assertTrue(hashFile.exists(), "no hash file was derived from the ETag"); + assertTrue(FileDownloadUtils.validateFile(destFile), "correctly downloaded file failed hash validation"); + PrintStream temp2 = new PrintStream(hashFile); - temp2.print("ABCD"); // some wrong hash value + temp2.print("ABCD"); // not a digest of any supported length temp2.close(); - //This is not yet implemented. I am using this test for documentation purpose. - assertThrows(UnsupportedOperationException.class, - () -> FileDownloadUtils.validateFile(destFile), + // An unreadable sidecar must not condemn an otherwise good download. + assertTrue(FileDownloadUtils.validateFile(destFile), + "a malformed hash file should be ignored, not treated as a mismatch"); + + PrintStream temp3 = new PrintStream(hashFile); + temp3.print("00000000000000000000000000000000"); // well-formed but wrong MD5 + temp3.close(); + assertFalse(FileDownloadUtils.validateFile(destFile), "file not detected to be invalid although hash value is wrong."); - + System.out.println("Just ignore the previous warning. It is expected."); + destFile.delete(); sizeFile.delete(); hashFile.delete(); } } + + @Nested + class Hashing { + + private File writeTemp(String name, byte[] content) throws IOException { + File f = new File(System.getProperty("java.io.tmpdir"), name); + Files.write(f.toPath(), content); + f.deleteOnExit(); + return f; + } + + @Test + void digestsOfEmptyFileMatchKnownValues() throws IOException { + File empty = writeTemp("bj-empty.bin", new byte[0]); + assertEquals("d41d8cd98f00b204e9800998ecf8427e", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.MD5)); + assertEquals("da39a3ee5e6b4b0d3255bfef95601890afd80709", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.SHA1)); + assertEquals("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + FileDownloadUtils.computeHash(empty, FileDownloadUtils.Hash.SHA256)); + } + + @Test + void digestOfKnownContent() throws IOException { + File abc = writeTemp("bj-abc.bin", "abc".getBytes(StandardCharsets.UTF_8)); + assertEquals("900150983cd24fb0d6963f7d28e17f72", + FileDownloadUtils.computeHash(abc, FileDownloadUtils.Hash.MD5)); + assertTrue(FileDownloadUtils.verifyHash(abc, FileDownloadUtils.Hash.MD5, + "900150983CD24FB0D6963F7D28E17F72"), "comparison should be case-insensitive"); + assertFalse(FileDownloadUtils.verifyHash(abc, FileDownloadUtils.Hash.MD5, + "00000000000000000000000000000000")); + } + + @Test + void algorithmNames() { + assertEquals("MD5", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.MD5)); + assertEquals("SHA-1", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.SHA1)); + assertEquals("SHA-256", FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.SHA256)); + assertThrows(IllegalArgumentException.class, + () -> FileDownloadUtils.getAlgorithmName(FileDownloadUtils.Hash.UNKNOWN)); + } + } + + @Nested + class HashFileParsing { + + private static final String MD5 = "900150983cd24fb0d6963f7d28e17f72"; + + private String parse(String content) throws IOException { + File f = new File(System.getProperty("java.io.tmpdir"), "bj-hashfile.txt"); + Files.write(f.toPath(), content.getBytes(StandardCharsets.UTF_8)); + f.deleteOnExit(); + return FileDownloadUtils.parseHashFile(f); + } + + @Test + void bareHex() throws IOException { + assertEquals(MD5, parse(MD5)); + assertEquals(MD5, parse(MD5 + "\n")); + } + + @Test + void uppercaseHexIsKeptVerbatim() throws IOException { + assertEquals(MD5.toUpperCase(), parse(MD5.toUpperCase())); + } + + @Test + void coreutilsLayouts() throws IOException { + assertEquals(MD5, parse(MD5 + " somefile.cif.gz\n")); + assertEquals(MD5, parse(MD5 + " *somefile.cif.gz\n")); + } + + @Test + void bsdLayout() throws IOException { + assertEquals(MD5, parse("MD5 (somefile.cif.gz) = " + MD5 + "\n")); + } + + @Test + void blankLeadingLinesAreSkipped() throws IOException { + assertEquals(MD5, parse("\n \n" + MD5 + "\n")); + } + + @Test + void garbageYieldsNull() throws IOException { + assertNull(parse("not a hash at all\n")); + assertNull(parse("ABCD\n")); + assertNull(parse("")); + } + } + + @Nested + class ETagParsing { + + @Test + void wwpdbStyleMd5IsRecognised() { + assertEquals(FileDownloadUtils.Hash.MD5, + FileDownloadUtils.hashFromETag("\"f99fb9d964e1e1c22f2ea559ac5745cf\"")); + assertEquals("f99fb9d964e1e1c22f2ea559ac5745cf", + FileDownloadUtils.normalizeETag("\"f99fb9d964e1e1c22f2ea559ac5745cf\"")); + } + + @Test + void sha1AndSha256LengthsAreRecognised() { + assertEquals(FileDownloadUtils.Hash.SHA1, + FileDownloadUtils.hashFromETag("da39a3ee5e6b4b0d3255bfef95601890afd80709")); + assertEquals(FileDownloadUtils.Hash.SHA256, + FileDownloadUtils.hashFromETag( + "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")); + } + + @Test + void ebiStyleTimeSizeETagIsNotMistakenForADigest() { + // nginx and Apache emit -; the dash keeps it out of + // the hex-only pattern, which is what stops us recording a bogus checksum. + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("\"67910788-1009e0\"")); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("\"14c95dd-51eedb9922b40\"")); + } + + @Test + void weakAndMissingETags() { + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag("W/\"abc\"")); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag(null)); + assertEquals(FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.hashFromETag(" ")); + assertNull(FileDownloadUtils.normalizeETag(null)); + assertNull(FileDownloadUtils.normalizeETag("\"\"")); + } + } + + @Nested + class ValidateFile { + + @Test + void bareRelativeNameDoesNotThrow() { + // getParentFile() is null here; this used to be a NullPointerException. + assertTrue(FileDownloadUtils.validateFile(new File("no-such-file-in-cwd.cif"))); + } + + @Test + void emptySizeFileIsIgnoredRatherThanThrowing() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "hello".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.size").toPath(), new byte[0]); + assertTrue(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void sizeMismatchIsDetected() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "hello".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.size").toPath(), "99".getBytes(StandardCharsets.UTF_8)); + assertFalse(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void everyHashSidecarIsChecked() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "abc".getBytes(StandardCharsets.UTF_8)); + // correct MD5, wrong SHA1: the second sidecar must still be caught + Files.write(new File(dir, "data.bin.hash_MD5").toPath(), + "900150983cd24fb0d6963f7d28e17f72".getBytes(StandardCharsets.UTF_8)); + Files.write(new File(dir, "data.bin.hash_SHA1").toPath(), + "0000000000000000000000000000000000000000".getBytes(StandardCharsets.UTF_8)); + assertFalse(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + + @Test + void writeHashFileRoundTrip() throws IOException { + File dir = Files.createTempDirectory("bj-validate").toFile(); + try { + File data = new File(dir, "data.bin"); + Files.write(data.toPath(), "abc".getBytes(StandardCharsets.UTF_8)); + FileDownloadUtils.writeHashFile(data, FileDownloadUtils.Hash.MD5, + FileDownloadUtils.computeHash(data, FileDownloadUtils.Hash.MD5)); + assertTrue(new File(dir, "data.bin.hash_MD5").exists()); + assertTrue(FileDownloadUtils.validateFile(data)); + } finally { + FileDownloadUtils.deleteDirectory(dir.toPath()); + } + } + } } diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java index 4ec4577f59..d08f6cb05f 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java @@ -550,7 +550,7 @@ private File downloadStructure(PdbId pdbId, String pathOnServer, boolean obsolet ftp = DEFAULT_BCIF_FILE_SERVER + filename; } else { ftp = String.format("%s%s/%s/%s", - serverName, pathOnServer, id.substring(id.length()-3, id.length()-1), getFilename(id)); + serverName, pathOnServer, getMiddleHash(id), getFilename(id)); } URL url = new URL(ftp); @@ -576,21 +576,44 @@ private File downloadStructure(PdbId pdbId, String pathOnServer, boolean obsolet logger.info("Fetching {}", ftp); logger.info("Writing to {}", realFile); - FileDownloadUtils.createValidationFiles(url, realFile, null, FileDownloadUtils.Hash.UNKNOWN); - FileDownloadUtils.downloadFile(url, realFile); + // A single connection, so the recorded size and checksum describe exactly the + // bytes that were written. The wwPDB servers return the content MD5 as the + // ETag, so this also gives the cached file a real integrity check. + FileDownloadUtils.downloadFileWithValidation(url, realFile, null, FileDownloadUtils.Hash.UNKNOWN, + FileDownloadUtils.ETagPolicy.USE_IF_HEX_DIGEST); if(! FileDownloadUtils.validateFile(realFile)) throw new IOException("Downloaded file invalid: "+realFile); return realFile; } + /** + * Returns the two-character directory name under which an entry is filed in the + * PDB's divided layout, e.g. cb for 1cbs. + *

+ * The characters are taken relative to the end of the identifier rather + * than the start, so that both spellings of the same entry land in the same + * bucket: 1cbs and its extended form pdb_00001cbs both + * give cb. Taking them from the start would file the extended form + * under db instead. The extended PDB identifier format is expected + * to keep using this same hashing scheme. + * + * @param pdbId a PDB identifier, in either the short or the extended form + * @return the lowercase two-character directory name + * @since 7.3.0 + */ + public static String getMiddleHash(String pdbId) { + int offset = pdbId.length() - 3; + return pdbId.substring(offset, offset + 2).toLowerCase(); + } + /** * Get the last modified time of the file in given url by retrieveing the "Last-Modified" header. * Note that this only works for http URLs * @param url * @return the last modified date or null if it couldn't be retrieved (in that case a warning will be logged) */ - private Date getLastModifiedTime(URL url) { + protected static Date getLastModifiedTime(URL url) { // see http://stackoverflow.com/questions/2416872/how-do-you-obtain-modified-date-from-a-remote-file-java Date date = null; @@ -629,14 +652,12 @@ private Date getLastModifiedTime(URL url) { protected File getDir(String pdbId, boolean obsolete) { File dir = null; - int offset = pdbId.length() - 3; + String middle = getMiddleHash(pdbId); if (obsolete) { // obsolete is always split - String middle = pdbId.substring(offset, offset + 2).toLowerCase(); dir = new File(obsoleteDirPath, middle); } else { - String middle = pdbId.substring(offset, offset + 2).toLowerCase(); dir = new File(splitDirPath, middle); } From 80e9daab55d7269f380239acc7140bf8b1681703 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:28:41 -0400 Subject: [PATCH 05/26] Test that a 404 is neither cached nor recorded as valid Exercises the new status checking against the real wwPDB archive: downloadFile must throw HttpStatusException rather than writing the error page to the destination, and createValidationFiles must not record a .size for it. Without the second half the cached error page would pass validateFile, since its recorded size would match the error body exactly. --- .../nbio/core/util/FileDownloadUtilsTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java index 1177c1d1fc..4a3cae7fb7 100644 --- a/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java +++ b/biojava-core/src/test/java/org/biojava/nbio/core/util/FileDownloadUtilsTest.java @@ -214,6 +214,31 @@ void testValidationFiles() throws IOException{ } } + @Nested + class HttpStatus { + + @Test + void notFoundThrowsAndLeavesNothingBehind() throws IOException { + // A path that is guaranteed absent from the wwPDB archive. + URL missing = new URL("https://files.wwpdb.org/pub/pdb/data/structures/divided/mmCIF/zz/zzzz.cif.gz"); + File dest = new File(System.getProperty("java.io.tmpdir"), "bj-missing.cif.gz"); + File sizeFile = new File(dest.getParentFile(), dest.getName() + ".size"); + dest.delete(); + sizeFile.delete(); + + HttpStatusException e = assertThrows(HttpStatusException.class, + () -> FileDownloadUtils.downloadFile(missing, dest)); + assertEquals(404, e.getStatusCode()); + assertTrue(e.isNotFound()); + assertFalse(dest.exists(), "a 404 body must never be written to the destination"); + + // ... and no validation metadata may be recorded for it either, or the + // cached error page would later pass validation. + FileDownloadUtils.createValidationFiles(missing, dest, null, FileDownloadUtils.Hash.UNKNOWN); + assertFalse(sizeFile.exists(), "no size file should be written for a 404 response"); + } + } + @Nested class Hashing { From 66d27c77a427f82812a2e05f5338a33b0b4d14f6 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 14:42:58 -0400 Subject: [PATCH 06/26] Make LocalPDBDirectory.getLastModifiedTime public It is a general 'has the server got a newer copy' helper with nothing PDB-specific about it, and other caching code outside this package needs the same Last-Modified comparison. Protected access only reached subclasses and the io package itself. --- .../java/org/biojava/nbio/structure/io/LocalPDBDirectory.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java index d08f6cb05f..2e6b09fe9a 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/LocalPDBDirectory.java @@ -612,8 +612,9 @@ public static String getMiddleHash(String pdbId) { * Note that this only works for http URLs * @param url * @return the last modified date or null if it couldn't be retrieved (in that case a warning will be logged) + * @since 7.3.0 made public so that other caching code can reuse it */ - protected static Date getLastModifiedTime(URL url) { + public static Date getLastModifiedTime(URL url) { // see http://stackoverflow.com/questions/2416872/how-do-you-obtain-modified-date-from-a-remote-file-java Date date = null; From 29af91d770f0414f0f63578d1878d3558fa409d8 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 19:30:39 -0400 Subject: [PATCH 07/26] Run integration tests nightly rather than on every pull request The integration tests reach CATH, ECOD, RCSB, EBI and others. That is what makes them worth having: they are how we learn that an upstream service changed a URL, a format or a redirect. It is also what makes them a poor gate on pull requests, because any of those services having a bad day blocks every contributor, and a real regression then cannot be distinguished from the surrounding noise. That is not hypothetical. Every PR Build since 2025-12-19 has failed, on all five matrix jobs, whatever the pull request contained - a one-line dependency bump fails exactly as a feature branch does. Two integration tests account for it: CathDomainTest, because download.cathdb.info began redirecting http to https, and EcodInstallationTest.testVersion, which downloads a 657 MB file from a slow server once per matrix job. Master Build already excludes the module with -pl '!biojava-integrationtest'. This applies the same exclusion to pull requests and adds a scheduled workflow that runs the full suite nightly, so the coverage is kept but is no longer in anybody's way. An upstream break still gets caught, within a day, by a run whose failure means what it says. The nightly job uses a single JDK, since the pull request build already covers 11, 17 and 21 and the point here is the network paths. It carries a 90 minute timeout, because these downloads are large and the six hour default is not a useful ceiling, and it keeps surefire reports for 30 days on failure, since GitHub expires run logs after 90 and those reports are what identify which service misbehaved. --- .github/workflows/nightly.yml | 54 ++++++++++++++++++++++++++++++ .github/workflows/pull_request.yml | 20 ++++++++--- 2 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/nightly.yml diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml new file mode 100644 index 0000000000..07a707b7ef --- /dev/null +++ b/.github/workflows/nightly.yml @@ -0,0 +1,54 @@ +name: Nightly Integration Tests + +# The integration tests reach out to CATH, ECOD, RCSB, EBI, UniProt and others. +# That makes them valuable - they are how we find out that an upstream service +# has changed a URL, a format or a redirect - but it also makes them unsuitable +# as a gate on pull requests, because an outage anywhere blocks every +# contributor. Running them on a schedule keeps the coverage while decoupling it +# from people's ability to merge. +on: + schedule: + # 03:17 UTC daily. Off the hour deliberately: scheduled jobs that ask for + # exactly midnight queue behind everybody else's. + - cron: '17 3 * * *' + # Also runnable by hand, e.g. to confirm an upstream service is back. + workflow_dispatch: + +permissions: + contents: read # to fetch code (actions/checkout) + +jobs: + integrationtest: + runs-on: ubuntu-latest + # These tests download large files from servers we do not control; the + # default 6 hour limit is far more than they need and far more than we want + # to spend if one of them hangs. + timeout-minutes: 90 + strategy: + matrix: + # One JDK only. The point of this run is to exercise the network paths, + # not the language level, which the pull request build already covers + # across 11, 17 and 21. + java: [21] + fail-fast: false + name: Integration tests, JDK ${{ matrix.java }} + + steps: + - uses: actions/checkout@v4 + - name: Set up JDK + uses: actions/setup-java@v4 + with: + distribution: 'oracle' + java-version: ${{ matrix.java }} + - name: Build and run integration tests + run: mvn verify --no-transfer-progress + - name: Upload surefire reports + # Kept on failure so an upstream break can be diagnosed after the fact: + # GitHub expires run logs after 90 days, and these reports carry the + # stack traces that say which service misbehaved. + if: failure() + uses: actions/upload-artifact@v4 + with: + name: surefire-reports + path: '**/target/surefire-reports/**' + retention-days: 30 diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index a0d31ee08a..340418cf68 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -30,8 +30,14 @@ jobs: with: distribution: 'oracle' java-version: ${{ matrix.java }} - - name: Build, test and integration test - run: mvn verify --no-transfer-progress + - name: Build and test (no integration tests) + # Integration tests are excluded here and run nightly instead, see + # nightly.yml. They depend on CATH, ECOD, RCSB, EBI and others being up + # and responsive, so running them on every pull request means a third + # party having a bad day blocks contributors, and a real regression + # cannot be told apart from the resulting noise. Master Build already + # excludes them for the same reason. + run: mvn verify -pl '!biojava-integrationtest' --no-transfer-progress # Note that 11 is not available in openjdk. So we need to do it with the Zulu distribution (see https://github.com/actions/setup-java) # When we drop 11, it will be safe to drop the copy-pasted workflow excerpt below @@ -54,5 +60,11 @@ jobs: with: distribution: 'zulu' java-version: ${{ matrix.java }} - - name: Build, test and integration test - run: mvn verify --no-transfer-progress + - name: Build and test (no integration tests) + # Integration tests are excluded here and run nightly instead, see + # nightly.yml. They depend on CATH, ECOD, RCSB, EBI and others being up + # and responsive, so running them on every pull request means a third + # party having a bad day blocks contributors, and a real regression + # cannot be told apart from the resulting noise. Master Build already + # excludes them for the same reason. + run: mvn verify -pl '!biojava-integrationtest' --no-transfer-progress From ed36d3a47752d66b9d85eaf1b750f2b33ea131b5 Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 19:29:20 -0400 Subject: [PATCH 08/26] Fix CATH downloads: use https, and check the response before caching it download.cathdb.info now answers plain http with a 301 to https. HttpURLConnection follows redirects within a protocol but deliberately will not follow one that switches http to https, so the download never reached the real file. downloadFileFromRemote read the response with a bare openStream() and no status check, which meant the body of the 301 was written into the local cache as though it were classification data. Parsing it produced no domains, and the first sign of trouble was a NullPointerException much later, far from the cause: CathDomainTest.test:40 NullPointer Cannot invoke "CathDomain.toCanonical()" because "domain" is null This has been failing on every pull request since the redirect appeared. Two changes. CATH_DOWNLOAD_URL now uses https, which fixes the immediate breakage. And the hand-rolled copy loop is replaced by FileDownloadUtils.downloadFileWithValidation, so a non-2xx response throws instead of being cached, and a redirect that changes protocol is logged explicitly rather than passing silently. That is what turns this class of failure from a mysterious NPE into an error naming the URL and the status. The shared download also records the byte count it actually wrote, so CATH files now get size validation they never had; download.cathdb.info sends no ETag, so there is no checksum to record, but the size check alone would have caught a truncated file. CathDomainTest passes again, in about 11 seconds. --- .../nbio/structure/cath/CathInstallation.java | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index 3f0f44158b..a72e85ed16 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -31,8 +31,6 @@ import java.io.*; import java.net.URL; -import java.nio.file.Files; -import java.nio.file.StandardCopyOption; import java.text.DateFormat; import java.text.DecimalFormat; import java.text.ParseException; @@ -54,7 +52,7 @@ public class CathInstallation implements CathDatabase{ public static final String nodeListFileName = "cath-names-v%s.txt"; public static final String domallFileName = "cath-domain-boundaries-v%s.txt"; - public static final String CATH_DOWNLOAD_URL = "http://download.cathdb.info/cath/releases/"; + public static final String CATH_DOWNLOAD_URL = "https://download.cathdb.info/cath/releases/"; public static final String CATH_DOWNLOAD_ALL_RELEASES_DIR = "all-releases"; public static final String CATH_DOWNLOAD_CLASSIFICATION_DATA_DIR = "cath-classification-data"; @@ -637,27 +635,26 @@ private void parseCathDomall(BufferedReader bufferedReader) throws IOException{ } protected void downloadFileFromRemote(URL remoteURL, File localFile) throws IOException{ -// System.out.println("downloading " + remoteURL + " to: " + localFile); LOGGER.info("Downloading file {} to local file {}", remoteURL, localFile); long timeS = System.currentTimeMillis(); - File tempFile = Files.createTempFile(FileDownloadUtils.getFilePrefix(localFile),"." + FileDownloadUtils.getFileExtension(localFile)).toFile(); - FileOutputStream out = new FileOutputStream(tempFile); - - InputStream in = remoteURL.openStream(); - byte[] buf = new byte[4 * 1024]; // 4K buffer - int bytesRead; - while ((bytesRead = in.read(buf)) != -1) { - out.write(buf, 0, bytesRead); + File parent = localFile.getAbsoluteFile().getParentFile(); + if (parent != null && !parent.isDirectory() && !parent.mkdirs()) { + throw new IOException("Could not create directory " + parent); } - in.close(); - out.close(); - Files.copy(tempFile.toPath(), localFile.toPath(), StandardCopyOption.REPLACE_EXISTING); + // Previously this read the response with a bare remoteURL.openStream() and + // copied whatever came back. That silently accepted error and redirect + // responses: when download.cathdb.info began redirecting http to https, the + // body of the 301 was written here as though it were classification data, + // and the failure only surfaced much later as an unparseable file. + FileDownloadUtils.downloadFileWithValidation(remoteURL, localFile, null, + FileDownloadUtils.Hash.UNKNOWN, FileDownloadUtils.ETagPolicy.USE_IF_HEX_DIGEST); - // delete the tmp file - tempFile.delete(); + if (!FileDownloadUtils.validateFile(localFile)) { + throw new IOException("Downloaded file invalid: " + localFile); + } long size = localFile.length(); From 5ec48ac6760fd4fff4daf839491801dec091dbeb Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 19:56:59 -0400 Subject: [PATCH 09/26] Reject redirects and error responses in the chem comp download downloadChemCompRecord read the response body without ever checking the status. A 4xx or 5xx already failed safely, because getInputStream() throws for those, but a redirect did not: when the JDK declines to follow a 3xx - which it always does when the redirect changes http to https - getInputStream() returns the body of the redirect instead. That body is short but not empty, so the "did we read any lines" check accepted it, gzipped it, and stored it under the component's name. Every later lookup then read that back and failed to parse it, far from the request that caused it. This is the same failure that took the CATH downloader out when download.cathdb.info moved to https, and files.rcsb.org would do the same to us the day it stops serving plain http. Checking the status closes it. The download is left otherwise alone: it re-compresses the response as it writes, so the local .cif.gz is not a byte copy of the remote .cif and downloadFileWithValidation cannot stand in for it here. --- .../structure/chem/DownloadChemCompProvider.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/chem/DownloadChemCompProvider.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/chem/DownloadChemCompProvider.java index 9eb9c7c6cf..e19535bce8 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/chem/DownloadChemCompProvider.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/chem/DownloadChemCompProvider.java @@ -1,5 +1,6 @@ package org.biojava.nbio.structure.chem; +import org.biojava.nbio.core.util.FileDownloadUtils; import org.biojava.nbio.core.util.InputStreamProvider; import org.biojava.nbio.structure.align.util.URLConnectionTools; import org.biojava.nbio.structure.align.util.UserConfiguration; @@ -396,6 +397,17 @@ private static boolean downloadChemCompRecord(String recordName) { url = new URL(u); URLConnection uconn = URLConnectionTools.openURLConnection(url); + // A 4xx or 5xx already fails safely, because getInputStream() throws for + // those. A redirect does not: if the server answers 3xx and the JDK + // declines to follow it - which it always does when the redirect changes + // http to https - getInputStream() hands back the body of the redirect + // instead. That body is short but not empty, so the "did we read any + // lines" check below accepts it, and it is gzipped and stored under the + // component's name. Every later lookup then reads it back and fails to + // parse, long after the request that caused it. This is exactly how the + // CATH downloader broke when download.cathdb.info moved to https. + FileDownloadUtils.checkHttpStatus(uconn); + try (PrintWriter pw = new PrintWriter(new GZIPOutputStream(new FileOutputStream(newFile))); BufferedReader fileBuffer = new BufferedReader(new InputStreamReader(uconn.getInputStream()))) { String line; From 5d072410f44fc6e7f966964451d78e235a3c850f Mon Sep 17 00:00:00 2001 From: Amr Date: Sat, 15 Aug 2026 20:05:00 -0400 Subject: [PATCH 10/26] Test that a redirect or error is never cached as a chem comp definition Serves the responses from a local HttpServer rather than a real service. Aiming a test at a third party that happens to redirect today would make it fail on the day they stop, which is the coupling that made the build unreliable in the first place; this one needs no network at all. Three cases: a 301 the JDK will not follow because it changes protocol, which is the case that broke CATH and the one the previous code accepted; a 503; and a 200, to show the guard rejects bad responses rather than simply refusing to download. The 200 case deliberately ignores a parse failure on its minimal body. What is under test is the download path, and the response reaches the cache before anything tries to parse it, so whether the CIF is well formed says nothing about whether the guard behaved. The existing testWeDontCacheGarbage covers a 404, which was already safe: getInputStream() throws for 4xx, which is why that case never showed the bug. --- .../chem/TestChemCompRedirectNotCached.java | 154 ++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 biojava-structure/src/test/java/org/biojava/nbio/structure/chem/TestChemCompRedirectNotCached.java diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/chem/TestChemCompRedirectNotCached.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/chem/TestChemCompRedirectNotCached.java new file mode 100644 index 0000000000..b948e9fbfb --- /dev/null +++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/chem/TestChemCompRedirectNotCached.java @@ -0,0 +1,154 @@ +/** + * BioJava development code + * + * This code may be freely distributed and modified under the terms of the GNU + * Lesser General Public Licence. This should be distributed with the code. If + * you do not have a copy, see: + * + * http://www.gnu.org/copyleft/lesser.html + * + * Copyright for this code is held jointly by the individual authors. These + * should be listed in @author doc comments. + * + * For more information on the BioJava project and its aims, or to join the + * biojava-l mailing list, visit the home page at: + * + * http://www.biojava.org/ + */ +package org.biojava.nbio.structure.chem; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.io.OutputStream; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; + +import com.sun.net.httpserver.HttpServer; + +import org.biojava.nbio.core.util.FlatFileCache; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * A server that redirects, or errors, must not have its response body cached as + * though it were a chemical component definition. + *

+ * This is the failure that took the CATH downloader out when + * download.cathdb.info moved to https, and the chem comp download had + * the same shape. A 4xx already failed safely, because + * getInputStream() throws for those; a redirect did not, because when + * the JDK declines to follow a 3xx it hands back the redirect's body instead, and + * that body is short but not empty. + *

+ * The test serves the responses from a local {@link HttpServer} rather than a real + * service. Pointing it at a third-party server that happens to redirect today would + * make the test fail on the day they stop, which is precisely the coupling that made + * the build unreliable in the first place. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ +public class TestChemCompRedirectNotCached { + + private HttpServer server; + private String originalServerUrl; + + @Before + public void setUp() { + originalServerUrl = DownloadChemCompProvider.serverBaseUrl; + } + + @After + public void tearDown() { + if (server != null) { + server.stop(0); + } + // Static state: leaving either of these set would corrupt unrelated tests. + DownloadChemCompProvider.serverBaseUrl = originalServerUrl; + FlatFileCache.clear(); + } + + /** + * Starts a local server that answers every request with the given status and body. + * + * @return the base URL to point the provider at + */ + private String startServer(int status, String location, String body) throws IOException { + server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + server.createContext("/", exchange -> { + byte[] bytes = body.getBytes(StandardCharsets.UTF_8); + if (location != null) { + exchange.getResponseHeaders().add("Location", location); + } + exchange.sendResponseHeaders(status, bytes.length); + try (OutputStream out = exchange.getResponseBody()) { + out.write(bytes); + } + }); + server.start(); + return "http://127.0.0.1:" + server.getAddress().getPort() + "/"; + } + + private File cacheFileFor(String id) { + File file = new File(DownloadChemCompProvider.getLocalFileName(id)); + file.delete(); + FlatFileCache.clear(); + return file; + } + + /** + * The case that broke CATH: a redirect the JDK will not follow because it + * changes protocol. Its body must not end up on disk under the component's name. + */ + @Test + public void redirectBodyIsNotCached() throws IOException { + File cached = cacheFileFor("ATP"); + DownloadChemCompProvider.serverBaseUrl = + startServer(301, "https://example.invalid/ATP.cif", "Moved Permanently"); + + ChemComp cc = new DownloadChemCompProvider().getChemComp("ATP"); + + assertFalse("the body of a redirect must never be cached as a definition", cached.exists()); + assertNull("nothing parseable was returned, so the component must be empty", cc.getName()); + } + + /** + * A 200 is still cached, so the guard has not simply disabled downloading. + *

+ * What is under test is the download path, not the CIF parser: the response is + * written to the cache before anything tries to parse it, so a parse failure on + * this deliberately minimal body says nothing about whether the guard behaved. + */ + @Test + public void aValidResponseIsStillCached() throws IOException { + File cached = cacheFileFor("ATP"); + DownloadChemCompProvider.serverBaseUrl = startServer(200, null, + "data_ATP\n#\n_chem_comp.id ATP\n_chem_comp.name \"ADENOSINE-5'-TRIPHOSPHATE\"\n#\n"); + + try { + new DownloadChemCompProvider().getChemComp("ATP"); + } catch (RuntimeException parseFailure) { + // see the note above + } + + assertTrue("a 200 response should still be cached", cached.exists()); + cached.delete(); + } + + /** A server error must not be cached either. */ + @Test + public void serverErrorBodyIsNotCached() throws IOException { + File cached = cacheFileFor("ATP"); + DownloadChemCompProvider.serverBaseUrl = + startServer(503, null, "Service Unavailable"); + + new DownloadChemCompProvider().getChemComp("ATP"); + + assertFalse("the body of a 5xx must never be cached as a definition", cached.exists()); + } +} From 6cb96336b496bb05089fe9cefd7adf8b0435ae8f Mon Sep 17 00:00:00 2001 From: Amr ALHOSSARY Date: Sun, 16 Aug 2026 21:42:21 -0400 Subject: [PATCH 11/26] Support the new ECOD distribution format ECOD redesigned its distribution at v294.1 and neither the version comment nor the column layout is recognised any more. The version comment changed from "#ECOD version develop291" to "# Version: v295", so parsedVersion stayed null and getVersion() fell back to the string that was requested. The columns changed from 15 (or 16 in develop291, which inserts unp_acc) to 23 and then 25, so every data line in the current release is rejected and getAllDomains() quietly returns an empty list. Files that declare a column header - every release since develop101 - are now read by column name rather than by position, which covers all four layouts at once and will not need revisiting the next time a column moves. Older files keep the positional path unchanged. Along with the columns: - manual_rep now holds True/False rather than MANUAL_REP/AUTO_NONREP - assembly_id is empty on every row, meaning what NOT_DOMAIN_ASSEMBLY meant - the ligand list moved to ligand_comp_ids - the last column is empty on four rows in five, so trailing empty fields are kept rather than discarded by split() - 53% of rows now describe domains found in AlphaFold models, which have no PDB entry and so cannot be an EcodDomain; they are counted and reported rather than logged as errors Parsing nothing at all is now reported as an error instead of returning an empty list silently, and the per-line NumberFormatException warning is capped like the others rather than printing a stack trace per row. getVersion() reads only the file header. Parsing the current 657 MB release in full to answer that question costs over a gigabyte of heap, which does not fit comfortably in the 1500 MB the test JVM is given. --- .../nbio/structure/ecod/EcodInstallation.java | 455 ++++++++++++++++-- 1 file changed, 404 insertions(+), 51 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java index f4be5cd4f5..5bd59dd14b 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java @@ -272,12 +272,41 @@ public void clear() { * * Note that this may differ from the version requested in the constructor * for the special case of "latest" + *

+ * Since 7.3.0 this reads only the file's header rather than parsing the whole + * file, so it no longer has the side effect of loading every domain. * @return the ECOD version * @throws IOException If an error occurs while downloading or parsing the file */ @Override public String getVersion() throws IOException { - ensureDomainsFileInstalled(); + domainsFileLock.readLock().lock(); + logger.trace("LOCK readlock"); + try { + if( parsedVersion != null ) { + return parsedVersion; + } + } finally { + logger.trace("UNLOCK readlock"); + domainsFileLock.readLock().unlock(); + } + + // The version is declared in the first few lines of the file, so read those rather + // than the millions of domain records behind them. The current release is 657 MB and + // holds nearly three million records; parsing it in full to answer this question + // costs over a gigabyte of heap and several seconds. + ensureDomainsFileDownloaded(); + + domainsFileLock.writeLock().lock(); + logger.trace("LOCK writelock"); + try { + if( parsedVersion == null ) { + parsedVersion = parseVersionOnly(); + } + } finally { + logger.trace("UNLOCK writelock"); + domainsFileLock.writeLock().unlock(); + } if( parsedVersion == null) { return requestedVersion; @@ -285,6 +314,30 @@ public String getVersion() throws IOException { return parsedVersion; } + /** + * Reads the version from the header of the local domains file without parsing the + * domains themselves. + * @return the version, or null if the header does not declare one + * @throws IOException if the file cannot be read + * @since 7.3.0 + */ + private String parseVersionOnly() throws IOException { + try( BufferedReader in = new BufferedReader(new FileReader(getDomainFile())) ) { + String line; + while( (line = in.readLine()) != null ) { + Matcher match = EcodParser.VERSION_RE.matcher(line); + if( match.matches() ) { + return match.group(1); + } + if( !line.startsWith("#") ) { + // past the header block; from v294.1 the column names are not commented + return null; + } + } + } + return null; + } + /** * Get the top-level ECOD server URL. Defaults to "http://prodata.swmed.edu" * @return the url to the ecod server @@ -325,6 +378,24 @@ public void setCacheLocation(String cacheLocation) { domainsFileLock.writeLock().unlock(); } + /** + * Ensures the domains file is present and current locally, without parsing it. + * @throws IOException in cases of file I/O, including failure to download a healthy file + * @since 7.3.0 + */ + private void ensureDomainsFileDownloaded() throws IOException { + domainsFileLock.writeLock().lock(); + logger.trace("LOCK writelock"); + try { + if( !domainsAvailable() ) { + downloadDomains(); + } + } finally { + logger.trace("UNLOCK writelock"); + domainsFileLock.writeLock().unlock(); + } + } + /** * Blocks until ECOD domains file has been downloaded and parsed. * @@ -549,6 +620,24 @@ Current version (1.4) contains the following columns: v1.2 - added f-group identifiers to fasta file, domain description file. ECODf identifiers now used when available for F-group name. Domain assemblies now represented by assembly uid in domain assembly status. v1.4 - added seqid_range and headers (develop101) +v1.6 - renamed column 4 from f_id to t_id and inserted unp_acc (UniProt accession) as + column 9, giving 16 columns (seen in develop291) + +From v294.1 the distribution was redesigned. The header comment changed from +"#ECOD version develop291" to "# Version: v294.1", the column header row is no longer +commented out, and the columns became: + + uid ecod_domain_id manual_rep f_id pdb chain pdb_range seqid_range architecture_name + x_name h_name t_name f_name assembly_id domain_id_short range_count arch_manual + x_manual h_manual t_manual f_manual valid_structure ligand_binding + +v295 appends ligand_comp_ids and ligand_pdbnum, for 25 columns. Also note that +manual_rep now holds True/False rather than MANUAL_REP/AUTO_NONREP, that assembly_id +and domain_id_short are empty on every row, that f_name is empty rather than +F_UNCLASSIFIED for unclassified domains, and that uid restarts from 0. + +Because the columns have been renamed, reordered and added to repeatedly, files that +declare a column header are read by column name rather than by position. */ /** String for unclassified F-groups */ @@ -561,10 +650,28 @@ Current version (1.4) contains the following columns: public static final String IS_REPRESENTATIVE = "MANUAL_REP"; /** Indicates not a manual representative */ public static final String NOT_REPRESENTATIVE = "AUTO_NONREP"; + /** + * Matches the comment declaring the version, which has taken two forms: + * {@code #ECOD version develop291} up to develop292, and {@code # Version: v295} + * from v294.1 onwards. + * @since 7.3.0 + */ + static final Pattern VERSION_RE = Pattern.compile( + "^\\s*#\\s*(?:ECOD\\s+)?version\\s*:?\\s*(\\S+).*", Pattern.CASE_INSENSITIVE); private List domains; private String version; + // prevent too many warnings; negative numbers print all warnings + private int warnIsDomainAssembly = 1; + private int warnHierarchicalFormat = 5; + private int warnNumberOfFields = 10; + private int warnNumberFormat = 10; + /** Data lines that could not be turned into a domain, for the summary at the end */ + private int skippedLines = 0; + /** Data lines describing a domain in a computed model rather than a PDB entry */ + private int modelLines = 0; + public EcodParser(String filename) throws IOException { this(new File(filename)); } @@ -584,30 +691,55 @@ private void parse(BufferedReader in) throws IOException { // Allocate plenty of space for ECOD as of 2015 ArrayList domainsList = new ArrayList<>(500000); - Pattern versionRE = Pattern.compile("^\\s*#.*ECOD\\s*version\\s+(\\S+).*"); Pattern commentRE = Pattern.compile("^\\s*#.*"); - // prevent too many warnings; negative numbers print all warnings - int warnIsDomainAssembly = 1; - int warnHierarchicalFormat = 5; - int warnNumberOfFields = 10; + ColumnLayout layout = null; String line = in.readLine(); int lineNum = 1; while( line != null ) { // Check for requestedVersion string - Matcher match = versionRE.matcher(line); + Matcher match = VERSION_RE.matcher(line); if(match.matches()) { // special requestedVersion comment this.version = match.group(1); + } else if( ColumnLayout.isColumnHeader(line) ) { + // The column names. Since the columns have been renamed, reordered and + // added to several times, later lines are read by name rather than by + // position wherever this header is present (develop101 onwards). + layout = ColumnLayout.fromHeader(line); + logger.debug("Read ECOD column header at line {}: {} columns",lineNum,layout.size()); } else { match = commentRE.matcher(line); if(match.matches()) { // ignore comments } else { - // data line - String[] fields = line.split("\t"); - if( fields.length == 13 || fields.length == 14 || fields.length == 15) { + // data line. The last column is frequently empty, so keep trailing + // empty fields rather than letting split() discard them. + String[] fields = line.split("\t", -1); + if( layout != null ) { + String pdb = layout.get(fields, "pdb"); + if( pdb != null && pdb.isEmpty() ) { + // From v294.1 the distribution also classifies domains + // found in computed (AlphaFold) models, which have no PDB + // entry and so cannot be represented by an EcodDomain. + modelLines++; + } else { + try { + EcodDomain domain = parseDomain(fields, layout, lineNum); + if(domain != null) { + domainsList.add(domain); + } else { + skippedLines++; + warnMissingColumns(lineNum); + } + } catch(IllegalArgumentException e) { + // includes NumberFormatException and an unusable PDB id + skippedLines++; + warnUnparseableLine(lineNum, e); + } + } + } else if( fields.length == 13 || fields.length == 14 || fields.length == 15) { try { int i = 0; // field number, to allow future insertion of fields @@ -620,32 +752,16 @@ private void parse(BufferedReader in) throws IOException { // Manual column may be missing in version 1.0 files Boolean manual = null; if( fields.length >= 14) { - String manualString = fields[i++]; - if(manualString.equalsIgnoreCase(IS_REPRESENTATIVE)) { - manual = true; - } else if(manualString.equalsIgnoreCase(NOT_REPRESENTATIVE)) { - manual = false; - } else { - logger.warn("Unexpected value for manual field: {} in line {}",manualString,lineNum); - } + manual = parseManualRep(fields[i++], lineNum); } //Column 4: ECOD hierachy identifier - [X-group].[H-group].[T-group].[F-group] // hierarchical field, e.g. "1.1.4.1" - String[] xhtGroup = fields[i++].split("\\."); - if(xhtGroup.length < 3 || 4 < xhtGroup.length) { - if(warnHierarchicalFormat > 1) { - logger.warn("Unexpected format for hierarchical field \"{}\" in line {}",fields[i-1],lineNum); - warnHierarchicalFormat--; - } else if(warnHierarchicalFormat != 0) { - logger.warn("Unexpected format for hierarchical field \"{}\" in line {}. Not printing future similar warnings.",fields[i-1],lineNum); - warnHierarchicalFormat--; - } - } - Integer xGroup = xhtGroup.length>0 ? Integer.parseInt(xhtGroup[0]) : null; - Integer hGroup = xhtGroup.length>1 ? Integer.parseInt(xhtGroup[1]) : null; - Integer tGroup = xhtGroup.length>2 ? Integer.parseInt(xhtGroup[2]) : null; - Integer fGroup = xhtGroup.length>3 ? Integer.parseInt(xhtGroup[3]) : null; + Integer[] xhtfGroup = parseHierarchy(fields[i++], lineNum); + Integer xGroup = xhtfGroup[0]; + Integer hGroup = xhtfGroup[1]; + Integer tGroup = xhtfGroup[2]; + Integer fGroup = xhtfGroup[3]; //Column 5: PDB identifier String pdbId = fields[i++]; @@ -699,32 +815,18 @@ private void parse(BufferedReader in) throws IOException { assemblyId = Long.parseLong(assemblyStr); } - String ligandStr = fields[i++]; - Set ligands = null; - if( "NO_LIGANDS_4A".equals(ligandStr) || ligandStr.isEmpty() ) { - ligands = Collections.emptySet(); - } else { - String[] ligSplit = ligandStr.split(","); - ligands = new LinkedHashSet<>(ligSplit.length); - for(String s : ligSplit) { - ligands.add(s.intern()); - } - } + Set ligands = parseLigands(fields[i++]); EcodDomain domain = new EcodDomain(uid, domainId, manual, xGroup, hGroup, tGroup, fGroup,pdbId, chainId, range, seqId, architectureName, xGroupName, hGroupName, tGroupName, fGroupName, assemblyId, ligands); domainsList.add(domain); } catch(NumberFormatException e) { - logger.warn("Error in ECOD parsing at line "+lineNum,e); + skippedLines++; + warnUnparseableLine(lineNum, e); } } else { - if(warnNumberOfFields > 1) { - logger.warn("Unexpected number of fields in line {}.",lineNum); - warnNumberOfFields--; - } else if(warnNumberOfFields == 0) { - logger.warn("Unexpected number of fields in line {}. Not printing future similar warnings",lineNum); - warnIsDomainAssembly--; - } + skippedLines++; + warnMissingColumns(lineNum); } } } @@ -737,6 +839,23 @@ private void parse(BufferedReader in) throws IOException { else logger.info("Parsed {} ECOD domains from version {}",domainsList.size(),this.version); + if(modelLines > 0) { + logger.info("Ignored {} ECOD domains classified from computed models, " + + "which have no PDB entry", modelLines); + } + + if(domainsList.isEmpty() && skippedLines > 0) { + // Returning an empty list quietly is how an upstream format change went + // unnoticed for eight months. Say so instead. + logger.error("Parsed no ECOD domains from {} data lines of version {}. " + + "The file format has probably changed; please report this at " + + "https://github.com/biojava/biojava/issues", skippedLines, + this.version == null ? "unknown" : this.version); + } else if(skippedLines > 0) { + logger.warn("Skipped {} of {} ECOD data lines that could not be parsed", + skippedLines, skippedLines + domainsList.size()); + } + this.domains = Collections.unmodifiableList( domainsList ); @@ -747,6 +866,169 @@ private void parse(BufferedReader in) throws IOException { } } + /** + * Builds a domain from a data line using the column names the file declares in its + * header, rather than fixed offsets. This is what allows one parser to read the + * 15-column develop101 layout, the 16-column develop291 layout (which inserts + * {@code unp_acc}) and the 23- and 25-column v294.1 and v295 layouts. + * @param fields the tab-separated values of one data line + * @param layout the column names read from the file's header + * @param lineNum the line number, for warnings + * @return the domain, or null if the line does not carry every required column + * @throws NumberFormatException if a numeric column does not hold a number + * @since 7.3.0 + */ + private EcodDomain parseDomain(String[] fields, ColumnLayout layout, int lineNum) { + String uidStr = layout.get(fields, "uid"); + String domainId = layout.get(fields, "ecod_domain_id"); + // renamed from t_id to f_id when the hierarchy gained a fourth level + String hierarchy = layout.get(fields, "f_id", "t_id"); + String pdbId = layout.get(fields, "pdb"); + String chainId = layout.get(fields, "chain"); + String range = layout.get(fields, "pdb_range"); + if( uidStr == null || domainId == null || hierarchy == null + || pdbId == null || chainId == null || range == null ) { + return null; + } + + Long uid = Long.parseLong(uidStr); + Boolean manual = parseManualRep(layout.get(fields, "manual_rep"), lineNum); + Integer[] xhtfGroup = parseHierarchy(hierarchy, lineNum); + // absent before version 1.4 + String seqId = layout.get(fields, "seqid_range"); + + String architectureName = internName(layout.get(fields, "architecture_name", "arch_name")); + String xGroupName = internName(layout.get(fields, "x_name")); + String hGroupName = internName(layout.get(fields, "h_name")); + String tGroupName = internName(layout.get(fields, "t_name")); + // Up to develop292 an unclassified domain carried F_UNCLASSIFIED here. From + // v294.1 the name is simply empty, while f_id still classifies the domain to + // four levels, so the two are no longer equivalent and the empty value is + // deliberately left as it is rather than translated. + String fGroupName = internName(layout.get(fields, "f_name")); + + // v294.1 and later declare assembly_id but leave it empty on every row, which + // means the same as the NOT_DOMAIN_ASSEMBLY of earlier versions. + Long assemblyId = null; + String assemblyStr = layout.get(fields, "assembly_id", "asm_status"); + if( assemblyStr == null || assemblyStr.isEmpty() || NOT_DOMAIN_ASSEMBLY.equals(assemblyStr) ) { + assemblyId = uid; + } else if( IS_DOMAIN_ASSEMBLY.equals(assemblyStr) ) { + warnDomainAssembly(lineNum); + } else { + assemblyId = Long.parseLong(assemblyStr); + } + + // the ligand list moved from the last column to ligand_comp_ids in v295 + Set ligands = parseLigands(layout.get(fields, "ligand_comp_ids", "ligand")); + + return new EcodDomain(uid, domainId, manual, xhtfGroup[0], xhtfGroup[1], xhtfGroup[2], + xhtfGroup[3], pdbId, chainId, range, seqId, architectureName, xGroupName, + hGroupName, tGroupName, fGroupName, assemblyId, ligands); + } + + /** + * Reads the representative-status column, which held MANUAL_REP or AUTO_NONREP up to + * develop292 and True or False from v294.1 onwards. + * @return true, false, or null if the column is absent or unrecognised + * @since 7.3.0 + */ + private Boolean parseManualRep(String manualString, int lineNum) { + if(manualString == null) { + return null; + } + if(manualString.equalsIgnoreCase(IS_REPRESENTATIVE) || manualString.equalsIgnoreCase("true")) { + return true; + } + if(manualString.equalsIgnoreCase(NOT_REPRESENTATIVE) || manualString.equalsIgnoreCase("false")) { + return false; + } + logger.warn("Unexpected value for manual field: {} in line {}",manualString,lineNum); + return null; + } + + /** + * Splits the hierarchical identifier, e.g. "1.1.4.1". + * @return the X, H, T and F group numbers, any of which may be null if absent + * @since 7.3.0 + */ + private Integer[] parseHierarchy(String hierarchy, int lineNum) { + String[] xhtGroup = hierarchy.split("\\."); + if(xhtGroup.length < 3 || 4 < xhtGroup.length) { + if(warnHierarchicalFormat > 1) { + logger.warn("Unexpected format for hierarchical field \"{}\" in line {}",hierarchy,lineNum); + warnHierarchicalFormat--; + } else if(warnHierarchicalFormat != 0) { + logger.warn("Unexpected format for hierarchical field \"{}\" in line {}. Not printing future similar warnings.",hierarchy,lineNum); + warnHierarchicalFormat--; + } + } + Integer[] groups = new Integer[4]; + for(int j = 0; j < groups.length && j < xhtGroup.length; j++) { + groups[j] = Integer.parseInt(xhtGroup[j]); + } + return groups; + } + + /** + * Reads a comma-separated list of non-polymer entities close to the domain. + * @return the ligands, or an empty set for NO_LIGANDS_4A, an empty value or no column + * @since 7.3.0 + */ + private Set parseLigands(String ligandStr) { + if( ligandStr == null || ligandStr.isEmpty() || "NO_LIGANDS_4A".equals(ligandStr) ) { + return Collections.emptySet(); + } + String[] ligSplit = ligandStr.split(","); + Set ligands = new LinkedHashSet<>(ligSplit.length); + for(String s : ligSplit) { + ligands.add(s.intern()); + } + return ligands; + } + + /** + * Interns a name likely to be shared by many domains, stripping the quotes that + * versions up to develop292 wrapped some of them in. + * @since 7.3.0 + */ + private String internName(String name) { + if(name == null) { + return null; + } + return clearStringQuotes(name).intern(); + } + + private void warnDomainAssembly(int lineNum) { + if(warnIsDomainAssembly > 1) { + logger.info("Deprecated 'IS_DOMAIN_ASSEMBLY' value ignored in line {}.",lineNum); + warnIsDomainAssembly--; + } else if(warnIsDomainAssembly == 0) { + logger.info("Deprecated 'IS_DOMAIN_ASSEMBLY' value ignored in line {}. Not printing future similar warnings.",lineNum); + warnIsDomainAssembly--; + } + } + + private void warnMissingColumns(int lineNum) { + if(warnNumberOfFields > 1) { + logger.warn("Unexpected number of fields in line {}.",lineNum); + warnNumberOfFields--; + } else if(warnNumberOfFields == 1) { + logger.warn("Unexpected number of fields in line {}. Not printing future similar warnings",lineNum); + warnNumberOfFields--; + } + } + + private void warnUnparseableLine(int lineNum, IllegalArgumentException e) { + if(warnNumberFormat > 1) { + logger.warn("Error in ECOD parsing at line "+lineNum,e); + warnNumberFormat--; + } else if(warnNumberFormat == 1) { + logger.warn("Error in ECOD parsing at line "+lineNum+". Not printing future similar warnings",e); + warnNumberFormat--; + } + } + private String clearStringQuotes(String name) { if ( name.startsWith("\"")) name = name.substring(1); @@ -770,6 +1052,77 @@ public List getDomains() { public String getVersion() { return version; } + + /** + * Maps the column names an ECOD domain file declares in its header onto their + * positions, so a data line can be read by name rather than by offset. + *

+ * Every distribution since develop101 carries such a header. It is commented + * (#uid<tab>ecod_domain_id<tab>...) up to develop292 and + * uncommented (uid<tab>ecod_domain_id<tab>...) from v294.1 + * onwards. Because names have also been changed between versions, lookups accept + * aliases and any name the file does not declare simply reads as absent. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ + private static class ColumnLayout { + private final Map columns; + + private ColumnLayout(Map columns) { + this.columns = columns; + } + + /** + * @return true if this line names the columns rather than holding domain data + */ + public static boolean isColumnHeader(String line) { + int tab = line.indexOf('\t'); + if(tab < 0) { + return false; + } + String first = line.substring(0, tab).trim(); + if(first.startsWith("#")) { + first = first.substring(1).trim(); + } + return first.equalsIgnoreCase("uid"); + } + + public static ColumnLayout fromHeader(String line) { + String[] names = line.split("\t", -1); + Map columns = new HashMap<>(names.length * 2); + for(int i = 0; i < names.length; i++) { + String name = names[i].trim(); + if(i == 0 && name.startsWith("#")) { + name = name.substring(1).trim(); + } + if(!name.isEmpty()) { + columns.put(name.toLowerCase(), i); + } + } + return new ColumnLayout(columns); + } + + /** + * @param fields the values of one data line + * @param aliases the names this column has gone by, most recent first + * @return the value of the first alias the file declares, or null if it declares + * none of them or this line is too short to reach it + */ + public String get(String[] fields, String... aliases) { + for(String alias : aliases) { + Integer i = columns.get(alias); + if(i != null) { + return i < fields.length ? fields[i] : null; + } + } + return null; + } + + public int size() { + return columns.size(); + } + } } From 2ad8e09e8140132196b3999c57f38cb1faab66df Mon Sep 17 00:00:00 2001 From: Amr ALHOSSARY Date: Sun, 16 Aug 2026 21:42:31 -0400 Subject: [PATCH 12/26] Test every ECOD layout offline The distribution is 657 MB, so the only test that touched the current release was testVersion, which asserts on one string. Every other test pins develop204. That is why a wholesale format change surfaced as a single one-line assertion failure, eight months after it happened. EcodParserTest reads fixtures taken verbatim from the real files - 13, 15, 16, 23 and 25 columns, both version comment forms, a row ending in an empty column, an AlphaFold-derived row, and a malformed row - through the Reader constructor. Fifteen tests, no network, well under a second. testVersion additionally parses the first few thousand lines of the local file. That is enough to notice a column change without building the three million domains the whole file now holds. --- .../test/ecod/EcodInstallationTest.java | 40 +++ .../nbio/structure/ecod/EcodParserTest.java | 323 ++++++++++++++++++ 2 files changed, 363 insertions(+) create mode 100644 biojava-structure/src/test/java/org/biojava/nbio/structure/ecod/EcodParserTest.java diff --git a/biojava-integrationtest/src/test/java/org/biojava/nbio/structure/test/ecod/EcodInstallationTest.java b/biojava-integrationtest/src/test/java/org/biojava/nbio/structure/test/ecod/EcodInstallationTest.java index 8ab3f29fb3..f384f2ba65 100644 --- a/biojava-integrationtest/src/test/java/org/biojava/nbio/structure/test/ecod/EcodInstallationTest.java +++ b/biojava-integrationtest/src/test/java/org/biojava/nbio/structure/test/ecod/EcodInstallationTest.java @@ -22,8 +22,12 @@ import static org.junit.Assert.*; +import java.io.BufferedReader; import java.io.File; +import java.io.FileReader; import java.io.IOException; +import java.io.Reader; +import java.io.StringReader; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -47,6 +51,7 @@ import org.biojava.nbio.structure.ecod.EcodDomain; import org.biojava.nbio.structure.ecod.EcodFactory; import org.biojava.nbio.structure.ecod.EcodInstallation; +import org.biojava.nbio.structure.ecod.EcodInstallation.EcodParser; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -277,12 +282,47 @@ public void testFilterByHierarchy() throws IOException { assertEquals(expected,actual); } + /** + * Checks that the current release can still be read. + *

+ * The version is read from the file's header without parsing the domains, so this + * additionally parses the first few thousand lines of the same file. That is enough to + * notice a column change — which is what ECOD did at v294.1, unnoticed for months — + * without building the three million domains the whole file now holds. + */ @Test public void testVersion() throws IOException { EcodDatabase ecod3 = EcodFactory.getEcodDatabase("latest"); String version = ecod3.getVersion(); assertNotNull(version); assertNotEquals("latest", version); + System.out.println("latest version of ECOD is "+version); + + File domainsFile = new File(((EcodInstallation) ecod3).getCacheLocation(), + "ecod.latest.domains.txt"); + assertTrue("No local copy of the domains file at "+domainsFile, domainsFile.exists()); + + EcodParser parser = new EcodParser(firstLines(domainsFile, 5000)); + assertEquals(version, parser.getVersion()); + assertFalse("No domains parsed from ECOD "+version + + "; the distribution format has probably changed", + parser.getDomains().isEmpty()); + } + + /** + * @return a reader over the first {@code maxLines} lines of the file + */ + private static Reader firstLines(File f, int maxLines) throws IOException { + StringBuilder head = new StringBuilder(); + try (BufferedReader in = new BufferedReader(new FileReader(f))) { + String line; + int n = 0; + while (n < maxLines && (line = in.readLine()) != null) { + head.append(line).append('\n'); + n++; + } + } + return new StringReader(head.toString()); } /** diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/ecod/EcodParserTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/ecod/EcodParserTest.java new file mode 100644 index 0000000000..295806e915 --- /dev/null +++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/ecod/EcodParserTest.java @@ -0,0 +1,323 @@ +/* + * BioJava development code + * + * This code may be freely distributed and modified under the + * terms of the GNU Lesser General Public Licence. This should + * be distributed with the code. If you do not have a copy, + * see: + * + * http://www.gnu.org/copyleft/lesser.html + * + * Copyright for this code is held jointly by the individual + * authors. These should be listed in @author doc comments. + * + * For more information on the BioJava project and its aims, + * or to join the biojava-l mailing list, visit the home page + * at: + * + * http://www.biojava.org/ + */ +package org.biojava.nbio.structure.ecod; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.io.StringReader; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; + +import org.biojava.nbio.structure.ecod.EcodInstallation.EcodParser; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +/** + * Checks that {@link EcodParser} reads every layout ECOD has distributed. + *

+ * The columns have been renamed, reordered and added to several times, and the version + * comment itself changed form at v294.1. Because the full distribution is 657 MB, none of + * that was covered by a test that could run in reasonable time, and a format change went + * unnoticed for months. These fixtures are taken verbatim from the real files, so the + * contract is pinned in milliseconds rather than by a download. + * + * @author Amr ALHOSSARY + * @since 7.3.0 + */ +class EcodParserTest { + + /** develop204, list format 1.5: 15 columns, commented header, quoted names. */ + private static final String DEVELOP204 = String.join("\n", + "#/data/ecod/database_versions/v204/ecod.develop204.domains.txt", + "#ECOD version develop204", + "#Domain list version 1.5", + "#Grishin lab (http://prodata.swmed.edu/ecod)", + "#uid\tecod_domain_id\tmanual_rep\tf_id\tpdb\tchain\tpdb_range\tseqid_range" + + "\tarch_name\tx_name\th_name\tt_name\tf_name\tasm_status\tligand", + "002137905\te6b4nA1\tAUTO_NONREP\t1.1.1\t6b4n\tA\tA:1-99\tA:1-99\tbeta barrels" + + "\t\"cradle loop barrel\"\t\"RIFT-related\"\t\"acid protease\"" + + "\tF_UNCLASSIFIED\tNOT_DOMAIN_ASSEMBLY\tCL,G53,NA"); + + /** develop291, list format 1.6: 16 columns, f_id renamed t_id, unp_acc inserted at 9. */ + private static final String DEVELOP291 = String.join("\n", + "#/data/ecod/database_versions/v291/ecod.develop291.domains.txt", + "#ECOD version develop291", + "#Domain list version 1.6", + "#Grishin lab (http://prodata.swmed.edu/ecod)", + "#uid\tecod_domain_id\tmanual_rep\tt_id\tpdb\tchain\tpdb_range\tseqid_range\tunp_acc" + + "\tarch_name\tx_name\th_name\tt_name\tf_name\tasm_status\tligand", + "000000267\te1udzA1\tMANUAL_REP\t1.1.1\t1udz\tA\tA:203-381\tA:4-182\tP12345" + + "\tbeta barrels\t\"cradle loop barrel\"\t\"RIFT-related\"\t\"acid protease\"" + + "\tF_UNCLASSIFIED\tNOT_DOMAIN_ASSEMBLY\tNO_LIGANDS_4A"); + + private static final String V295_COLUMNS = + "uid\tecod_domain_id\tmanual_rep\tf_id\tpdb\tchain\tpdb_range\tseqid_range" + + "\tarchitecture_name\tx_name\th_name\tt_name\tf_name\tassembly_id\tdomain_id_short" + + "\trange_count\tarch_manual\tx_manual\th_manual\tt_manual\tf_manual" + + "\tvalid_structure\tligand_binding\tligand_comp_ids\tligand_pdbnum"; + + /** v295: 25 columns, uncommented header, True/False, empty assembly_id, moved ligands. */ + private static final String V295 = String.join("\n", + "# ECOD Domain List", + "# Version: v295", + "# Generated: 2026-06-24 22:47:42", + "# Ligand cutoff: 4.0 A (NO_LIGANDS_4A = no contact within cutoff)", + "#", + V295_COLUMNS, + "0\te2nmzA1\tTrue\t1.1.1.3\t2nmz\tA\tA:1-99\tA:1-99\tbeta barrels\tcradle loop barrel" + + "\tRIFT-related\tacid protease\tRVP\t\t\t1\tFalse\tFalse\tFalse\tFalse\tTrue" + + "\tTrue\tTrue\tROC,SO4\tA:601,A:602,B:401", + // the last column is empty on four rows in five, so split() must keep it + "3\te2rspA1\tTrue\t1.1.1.3\t2rsp\tA\tA:1-124\tA:1-124\tbeta barrels\tcradle loop barrel" + + "\tRIFT-related\tacid protease\tRVP\t\t\t1\tFalse\tFalse\tFalse\tFalse\tTrue" + + "\tTrue\tFalse\tNO_LIGANDS_4A\t", + // a domain classified from an AlphaFold model: no PDB entry, so no EcodDomain + "3163557\tP44140_F1_nD2\tFalse\t2004.1.1.123\t\t\t131-315\t131-315\talpha bundles" + + "\tsomething\tsomething else\ta third thing\t\t\t\t1\tFalse\tFalse\tFalse" + + "\tFalse\tTrue\tTrue\tFalse\tNO_LIGANDS_4A\t"); + + private static List parse(String contents) throws IOException { + return new EcodParser(new StringReader(contents)).getDomains(); + } + + private static String version(String contents) throws IOException { + return new EcodParser(new StringReader(contents)).getVersion(); + } + + @Nested + class Version { + @Test + void oldHeaderForm() throws IOException { + assertEquals("develop204", version(DEVELOP204)); + assertEquals("develop291", version(DEVELOP291)); + } + + @Test + void newHeaderForm() throws IOException { + assertEquals("v295", version(V295)); + assertEquals("v294.1", version("# ECOD Domain List\n# Version: v294.1\n")); + } + + @Test + void listFormatVersionIsNotTheEcodVersion() throws IOException { + // "#Domain list version 1.5" describes the columns, not the release + assertNull(version("#Grishin lab\n#Domain list version 1.5\n")); + } + + @Test + void absentVersionIsNull() throws IOException { + assertNull(version("#Grishin lab (http://prodata.swmed.edu/ecod)\n")); + } + } + + @Nested + class OldFormats { + @Test + void listFormat15() throws IOException { + List domains = parse(DEVELOP204); + assertEquals(1, domains.size()); + EcodDomain d = domains.get(0); + assertEquals(Long.valueOf(2137905), d.getUid()); + assertEquals("e6b4nA1", d.getDomainId()); + assertEquals(Boolean.FALSE, d.getManual()); + assertEquals(Integer.valueOf(1), d.getXGroup()); + assertEquals(Integer.valueOf(1), d.getHGroup()); + assertEquals(Integer.valueOf(1), d.getTGroup()); + assertNull(d.getFGroup()); + assertEquals("6B4N", d.getPdbId().getId()); + assertEquals("A", d.getChainId()); + assertEquals("A:1-99", d.getRange()); + assertEquals("A:1-99", d.getSeqIdRange()); + assertEquals("beta barrels", d.getArchitectureName()); + // quotes were stripped up to develop292 + assertEquals("cradle loop barrel", d.getXGroupName()); + assertEquals("RIFT-related", d.getHGroupName()); + assertEquals("acid protease", d.getTGroupName()); + assertEquals("F_UNCLASSIFIED", d.getFGroupName()); + assertEquals(Long.valueOf(2137905), d.getAssemblyId()); + assertEquals(new LinkedHashSet<>(Arrays.asList("CL", "G53", "NA")), d.getLigands()); + } + + /** + * develop291 inserted unp_acc before arch_name. Read positionally, every field from + * there on shifts by one and the domain is silently mangled or dropped. + */ + @Test + void listFormat16InsertsUnpAcc() throws IOException { + List domains = parse(DEVELOP291); + assertEquals(1, domains.size()); + EcodDomain d = domains.get(0); + assertEquals(Boolean.TRUE, d.getManual()); + assertEquals("1UDZ", d.getPdbId().getId()); + assertEquals("A:4-182", d.getSeqIdRange()); + assertEquals("beta barrels", d.getArchitectureName()); + assertEquals("acid protease", d.getTGroupName()); + assertEquals("F_UNCLASSIFIED", d.getFGroupName()); + assertEquals(Long.valueOf(267), d.getAssemblyId()); + assertEquals(Collections.emptySet(), d.getLigands()); + } + + /** + * Headers were only added in develop101. Older files are still read by position. + */ + @Test + void thirteenColumnsWithoutAHeader() throws IOException { + List domains = parse(String.join("\n", + "#ECOD version develop45", + "000000001\te1udzA1\t1.1.1\t1udz\tA\tA:203-381\tbeta barrels" + + "\t\"cradle loop barrel\"\t\"RIFT-related\"\t\"acid protease\"" + + "\tF_UNCLASSIFIED\tNOT_DOMAIN_ASSEMBLY\tNO_LIGANDS_4A")); + assertEquals(1, domains.size()); + EcodDomain d = domains.get(0); + assertNull(d.getManual(), "no manual_rep column before list format 1.1"); + assertNull(d.getSeqIdRange(), "no seqid_range column before list format 1.4"); + assertEquals("1UDZ", d.getPdbId().getId()); + assertEquals("acid protease", d.getTGroupName()); + } + } + + @Nested + class NewFormat { + @Test + void twentyFiveColumns() throws IOException { + List domains = parse(V295); + // two PDB domains; the AlphaFold-derived row cannot be an EcodDomain + assertEquals(2, domains.size()); + + EcodDomain d = domains.get(0); + assertEquals(Long.valueOf(0), d.getUid()); + assertEquals("e2nmzA1", d.getDomainId()); + assertEquals(Boolean.TRUE, d.getManual(), "manual_rep is now True/False"); + assertEquals(Integer.valueOf(1), d.getXGroup()); + assertEquals(Integer.valueOf(1), d.getHGroup()); + assertEquals(Integer.valueOf(1), d.getTGroup()); + assertEquals(Integer.valueOf(3), d.getFGroup(), "f_id now carries a fourth level"); + assertEquals("2NMZ", d.getPdbId().getId()); + assertEquals("A", d.getChainId()); + assertEquals("A:1-99", d.getRange()); + assertEquals("A:1-99", d.getSeqIdRange()); + assertEquals("beta barrels", d.getArchitectureName()); + assertEquals("cradle loop barrel", d.getXGroupName()); + assertEquals("RIFT-related", d.getHGroupName()); + assertEquals("acid protease", d.getTGroupName()); + assertEquals("RVP", d.getFGroupName()); + // assembly_id is empty on every row of v294.1 and later, which means the same + // as the NOT_DOMAIN_ASSEMBLY of earlier versions + assertEquals(Long.valueOf(0), d.getAssemblyId()); + assertEquals(new LinkedHashSet<>(Arrays.asList("ROC", "SO4")), d.getLigands(), + "the ligand list moved to ligand_comp_ids"); + } + + /** + * ligand_pdbnum, the last column, is empty on four rows in five. String.split + * discards trailing empty fields unless asked not to, which would make those rows + * look one column short. + */ + @Test + void rowEndingInAnEmptyColumn() throws IOException { + EcodDomain d = parse(V295).get(1); + assertEquals("e2rspA1", d.getDomainId()); + assertEquals("2RSP", d.getPdbId().getId()); + assertEquals(Collections.emptySet(), d.getLigands()); + } + + @Test + void twentyThreeColumnsOfV2941() throws IOException { + List domains = parse(String.join("\n", + "# ECOD Domain List", + "# Version: v294.1", + "#", + "uid\tecod_domain_id\tmanual_rep\tf_id\tpdb\tchain\tpdb_range\tseqid_range" + + "\tarchitecture_name\tx_name\th_name\tt_name\tf_name\tassembly_id" + + "\tdomain_id_short\trange_count\tarch_manual\tx_manual\th_manual" + + "\tt_manual\tf_manual\tvalid_structure\tligand_binding", + "1\te1hvcA1\tFalse\t1.1.1.3\t1hvc\tA\tA:1B-99A\tA:1-203\tbeta barrels" + + "\tcradle loop barrel\tRIFT-related\tacid protease\tRVP\t\t\t\tFalse" + + "\tFalse\tFalse\tFalse\tFalse\tTrue\tFalse")); + assertEquals(1, domains.size()); + EcodDomain d = domains.get(0); + assertEquals("1HVC", d.getPdbId().getId()); + assertEquals("A:1B-99A", d.getRange()); + assertEquals("RVP", d.getFGroupName()); + // there is no ligand column at all in v294.1 + assertEquals(Collections.emptySet(), d.getLigands()); + } + + @Test + void columnHeaderIsNotADomain() throws IOException { + // v294.1 stopped commenting the column names out, so they arrive looking like data + for (EcodDomain d : parse(V295)) { + assertFalse("uid".equals(d.getDomainId())); + } + } + + /** + * An empty f_name is not the same as F_UNCLASSIFIED: f_id still classifies the + * domain to four levels, so the empty value is left as it is rather than translated. + */ + @Test + void emptyFGroupNameIsLeftAlone() throws IOException { + List domains = parse(String.join("\n", + "# Version: v295", + V295_COLUMNS, + "7\te4fivA1\tTrue\t1.1.1.3\t4fiv\tA\tA:4-116\tA:1-113\tbeta barrels" + + "\tcradle loop barrel\tRIFT-related\tacid protease\t\t\t\t1\tFalse" + + "\tFalse\tFalse\tFalse\tTrue\tTrue\tTrue\tLP1\tA:201")); + assertEquals(1, domains.size()); + assertEquals("", domains.get(0).getFGroupName()); + } + } + + @Nested + class Robustness { + @Test + void unparseableLinesAreSkippedNotFatal() throws IOException { + List domains = parse(String.join("\n", + "# Version: v295", + V295_COLUMNS, + "not-a-number\tefoo\tTrue\t1.1.1.3\tfoo1\tA\tA:1-9\tA:1-9\ta\tb\tc\td\te" + + "\t\t\t1\tFalse\tFalse\tFalse\tFalse\tTrue\tTrue\tFalse\t\t", + "0\te2nmzA1\tTrue\t1.1.1.3\t2nmz\tA\tA:1-99\tA:1-99\tbeta barrels" + + "\tcradle loop barrel\tRIFT-related\tacid protease\tRVP\t\t\t1" + + "\tFalse\tFalse\tFalse\tFalse\tTrue\tTrue\tTrue\tROC,SO4\tA:601")); + assertEquals(1, domains.size(), "the good line is still read"); + assertEquals("e2nmzA1", domains.get(0).getDomainId()); + } + + @Test + void shortLineIsSkipped() throws IOException { + assertTrue(parse(String.join("\n", + "# Version: v295", + V295_COLUMNS, + "0\te2nmzA1\tTrue")).isEmpty()); + } + + @Test + void emptyFileYieldsNoDomains() throws IOException { + assertTrue(parse("").isEmpty()); + } + } +} From 60d03c41daa4dacbc3f6866320da24502062f908 Mon Sep 17 00:00:00 2001 From: Jose Duarte Date: Sun, 23 Aug 2026 18:48:58 -0700 Subject: [PATCH 13/26] Pure array solution for sphere points. About 10% faster --- .../nbio/structure/asa/AsaCalculator.java | 58 +++++++++++++------ 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java index 0f46b7f416..b5479c6101 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java @@ -27,7 +27,6 @@ import org.slf4j.LoggerFactory; import javax.vecmath.Point3d; -import javax.vecmath.Vector3d; import java.util.*; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -116,7 +115,14 @@ static class IndexAndDistance { private final double[] radii; private final double probe; private final int nThreads; - private Vector3d[] spherePoints; + /** + * The sphere points to sample, as a flat array of interleaved x,y,z coordinates (thus of size 3 x nSpherePoints). + * A flat array of primitives (rather than an array of Vector3d objects) is used for performance: it keeps the + * points contiguous in memory and avoids a pointer dereference per point in the innermost loop of + * {@link #calcSingleAsa(int)}. + */ + private double[] spherePoints; + private int nSpherePoints; private double cons; private IndexAndDistance[][] neighborIndices; @@ -239,7 +245,8 @@ private void initSpherePoints(int nSpherePoints) { logger.debug("Will use {} sphere points", nSpherePoints); // initialising the sphere points to sample - spherePoints = generateSpherePoints(nSpherePoints); + this.nSpherePoints = nSpherePoints; + this.spherePoints = generateSpherePoints(nSpherePoints); cons = 4.0 * Math.PI / nSpherePoints; } @@ -334,17 +341,19 @@ void setUseSpatialHashingForNeighbors(boolean useSpatialHashingForNeighbors) { * Returns list of 3d coordinates of points on a unit sphere using the * Golden Section Spiral algorithm. * @param nSpherePoints the number of points to be used in generating the spherical dot-density - * @return the array of points as Vector3d objects + * @return a flat array of interleaved x,y,z coordinates, of size 3 x nSpherePoints */ - private Vector3d[] generateSpherePoints(int nSpherePoints) { - Vector3d[] points = new Vector3d[nSpherePoints]; + private double[] generateSpherePoints(int nSpherePoints) { + double[] points = new double[3 * nSpherePoints]; double inc = Math.PI * (3.0 - Math.sqrt(5.0)); double offset = 2.0 / nSpherePoints; for (int k=0;k> 2]++; - if (dotProd > sqRadii[nbArrayInd]) { + if (dotProd > nbData[off + 3]) { is_accessible = false; break; } From 5c8cff02bf7d95abd0a601f3915bd1d832cf23da Mon Sep 17 00:00:00 2001 From: Jose Duarte Date: Wed, 26 Aug 2026 10:46:46 -0700 Subject: [PATCH 14/26] Use parallel primitive arrays for neighbors in AsaCalculator Replaces IndexAndDistance object arrays with primitive int[] and double[] arrays in a Neighbors holder, sorted at creation time to eliminate per-atom sort calls and reduce object allocations. --- .../nbio/structure/asa/AsaCalculator.java | 199 +++++++++++------- .../nbio/structure/asa/TestAsaCalc.java | 22 +- 2 files changed, 139 insertions(+), 82 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java index b5479c6101..ed576b9abe 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/asa/AsaCalculator.java @@ -100,12 +100,56 @@ public void run() { } } - static class IndexAndDistance { - final int index; - final double dist; - IndexAndDistance(int index, double dist) { - this.index = index; - this.dist = dist; + /** + * The neighbors of a single atom, as parallel primitive arrays of neighbor atom indices and their distances to + * the central atom, sorted by increasing distance. + *

+ * Parallel primitive arrays are used rather than an array of index-distance objects because there are ~30 + * neighbors per atom: for a large structure that would mean millions of small short-lived objects. + */ + static class Neighbors { + + /** The neighbor atom indices, ordered by increasing distance to the central atom */ + final int[] indices; + /** The distances to the central atom, in increasing order and parallel to {@link #indices} */ + final double[] dists; + + private Neighbors(int[] indices, double[] dists) { + this.indices = indices; + this.dists = dists; + } + + /** + * Creates a Neighbors from the first count elements of the given buffers, copying them to exact-size arrays + * and sorting them by increasing distance. + * @param indicesBuffer the neighbor indices, only the first count elements are used + * @param distsBuffer the neighbor distances, only the first count elements are used + * @param count the number of neighbors + * @return the sorted neighbors + */ + static Neighbors createSorted(int[] indicesBuffer, double[] distsBuffer, int count) { + int[] indices = Arrays.copyOf(indicesBuffer, count); + double[] dists = Arrays.copyOf(distsBuffer, count); + // Sorting by closest to farthest away neighbors achieves faster runtimes when checking for occluded + // sphere sample points in calcSingleAsa. This follows the ideas exposed in + // Eisenhaber et al, J Comp Chemistry 1994 (https://onlinelibrary.wiley.com/doi/epdf/10.1002/jcc.540160303) + // This is essential for performance: it brings down the number of occlusion checks to + // an average of n_sphere_points/10 per atom, producing ~ x4 performance gain overall. + // An insertion sort is used because the arrays are small (~30 elements on average) and because it avoids + // both the boxing of a comparator-based sort and the object allocation an index-distance array would need. + for (int i = 1; i < count; i++) { + double dist = dists[i]; + int index = indices[i]; + int j = i - 1; + while (j >= 0 && dists[j] > dist) { + dists[j + 1] = dists[j]; + indices[j + 1] = indices[j]; + j--; + } + dists[j + 1] = dist; + indices[j + 1] = index; + } + return new Neighbors(indices, dists); } } @@ -124,7 +168,7 @@ static class IndexAndDistance { private double[] spherePoints; private int nSpherePoints; private double cons; - private IndexAndDistance[][] neighborIndices; + private Neighbors[] neighbors; private boolean useSpatialHashingForNeighbors; @@ -292,10 +336,10 @@ public double[] calculateAsas() { long start = System.currentTimeMillis(); if (useSpatialHashingForNeighbors) { logger.debug("Will use spatial hashing to find neighbors"); - neighborIndices = findNeighborIndicesSpatialHashing(); + neighbors = findNeighborIndicesSpatialHashing(); } else { logger.debug("Will not use spatial hashing to find neighbors"); - neighborIndices = findNeighborIndices(); + neighbors = findNeighborIndices(); } long end = System.currentTimeMillis(); logger.debug("Took {} s to find neighbors", (end-start)/1000.0); @@ -359,93 +403,108 @@ private double[] generateSpherePoints(int nSpherePoints) { } /** - * Returns the 2-dimensional array with neighbor indices for every atom. - * @return 2-dimensional array of size: n_atoms x n_neighbors_per_atom + * Tells whether the 2 given atoms are neighbors for the purposes of the ASA calculation, i.e. whether their + * probe-inflated spheres overlap. + * @param i the index of the first atom + * @param j the index of the second atom + * @param dist the distance between the 2 atoms + * @return true if they are neighbors, false otherwise */ - IndexAndDistance[][] findNeighborIndices() { + private boolean areNeighbors(int i, int j, double dist) { + return dist < radii[i] + probe + probe + radii[j]; + } - // looking at a typical protein case, number of neighbours are from ~10 to ~50, with an average of ~30 - int initialCapacity = 60; + /** + * Returns the neighbors of every atom, sorted by increasing distance. + * @return array of size n_atoms + */ + Neighbors[] findNeighborIndices() { - IndexAndDistance[][] nbsIndices = new IndexAndDistance[atomCoords.length][]; + Neighbors[] nbs = new Neighbors[atomCoords.length]; + + // looking at a typical protein case, number of neighbours are from ~10 to ~50, with an average of ~30. + // The buffers are reused for all atoms and grown on demand, so that only the exact-size arrays kept in the + // returned Neighbors objects are allocated per atom. + int[] indicesBuffer = new int[60]; + double[] distsBuffer = new double[60]; for (int k=0; k thisNbIndices = new ArrayList<>(initialCapacity); + int count = 0; for (int i = 0; i < atomCoords.length; i++) { if (i == k) continue; double dist = atomCoords[i].distance(atomCoords[k]); - if (dist < radius + radii[i]) { - thisNbIndices.add(new IndexAndDistance(i, dist)); + if (areNeighbors(k, i, dist)) { + if (count == indicesBuffer.length) { + indicesBuffer = Arrays.copyOf(indicesBuffer, count * 2); + distsBuffer = Arrays.copyOf(distsBuffer, count * 2); + } + indicesBuffer[count] = i; + distsBuffer[count] = dist; + count++; } } - IndexAndDistance[] indicesArray = thisNbIndices.toArray(new IndexAndDistance[0]); - nbsIndices[k] = indicesArray; + nbs[k] = Neighbors.createSorted(indicesBuffer, distsBuffer, count); } - return nbsIndices; + return nbs; } /** - * Returns the 2-dimensional array with neighbor indices for every atom, + * Returns the neighbors of every atom, sorted by increasing distance, * using spatial hashing to avoid all to all distance calculation. - * @return 2-dimensional array of size: n_atoms x n_neighbors_per_atom + * @return array of size n_atoms */ - IndexAndDistance[][] findNeighborIndicesSpatialHashing() { - - // looking at a typical protein case, number of neighbours are from ~10 to ~50, with an average of ~30 - int initialCapacity = 60; + Neighbors[] findNeighborIndicesSpatialHashing() { List contactList = calcContacts(); - Map> indices = new HashMap<>(atomCoords.length); + + // A first pass to count the neighbors per atom, so that exact-size arrays can be allocated in the second + // pass. Atom indices are dense, so plain arrays are used rather than a map: that avoids boxing the indices + // and the repeated hashing, which are significant given that there are ~30 contacts per atom. + int[] counts = new int[atomCoords.length]; for (Contact contact : contactList) { // note contacts are stored 1-way only, with j>i int i = contact.getI(); int j = contact.getJ(); - - List iIndices; - List jIndices; - if (!indices.containsKey(i)) { - iIndices = new ArrayList<>(initialCapacity); - indices.put(i, iIndices); - } else { - iIndices = indices.get(i); - } - if (!indices.containsKey(j)) { - jIndices = new ArrayList<>(initialCapacity); - indices.put(j, jIndices); - } else { - jIndices = indices.get(j); - } - - double radius = radii[i] + probe + probe; - double dist = contact.getDistance(); - if (dist < radius + radii[j]) { - iIndices.add(new IndexAndDistance(j, dist)); - jIndices.add(new IndexAndDistance(i, dist)); + if (areNeighbors(i, j, contact.getDistance())) { + counts[i]++; + counts[j]++; } } - // convert map to array for fast access - IndexAndDistance[][] nbsIndices = new IndexAndDistance[atomCoords.length][]; - for (Map.Entry> entry : indices.entrySet()) { - List list = entry.getValue(); - IndexAndDistance[] indexAndDistances = list.toArray(new IndexAndDistance[0]); - nbsIndices[entry.getKey()] = indexAndDistances; + int[][] indices = new int[atomCoords.length][]; + double[][] dists = new double[atomCoords.length][]; + for (int i = 0; i < atomCoords.length; i++) { + // note that some atoms might have no neighbors at all, in which case these are empty arrays + indices[i] = new int[counts[i]]; + dists[i] = new double[counts[i]]; } - // important: some atoms might have no neighbors at all: we need to initialise to empty arrays - for (int i=0; i calcContacts() { private double calcSingleAsa(int i) { Point3d atom_i = atomCoords[i]; - int n_neighbor = neighborIndices[i].length; - IndexAndDistance[] neighbor_indices = neighborIndices[i]; - // Sorting by closest to farthest away neighbors achieves faster runtimes when checking for occluded - // sphere sample points below. This follows the ideas exposed in - // Eisenhaber et al, J Comp Chemistry 1994 (https://onlinelibrary.wiley.com/doi/epdf/10.1002/jcc.540160303) - // This is essential for performance. In my tests this brings down the number of occlusion checks in loop below to - // an average of n_sphere_points/10 per atom i, producing ~ x4 performance gain overall - Arrays.sort(neighbor_indices, Comparator.comparingDouble(o -> o.dist)); + // note the neighbors are already sorted by increasing distance (see Neighbors#createSorted), which is + // essential for the performance of the occlusion checks in the loop below + Neighbors nbs = neighbors[i]; + int[] neighbor_indices = nbs.indices; + double[] neighbor_dists = nbs.dists; + int n_neighbor = neighbor_indices.length; double radius_i = probe + radii[i]; @@ -492,8 +549,8 @@ private double calcSingleAsa(int i) { // pattern of the early break and is significantly faster than an array of Vector3d objects. double[] nbData = new double[4 * n_neighbor]; for (int nbArrayInd =0; nbArrayInd listOfMatchingIndices = new ArrayList<>(); for (int i = 0; i < nbsSh.length; i++) { for (int j = 0; j < nbs.length; j++) { - if (nbs[j].index == nbsSh[i].index) { + if (nbs[j] == nbsSh[i]) { listOfMatchingIndices.add(j); break; } @@ -229,21 +229,21 @@ public void testNoNeighborsIssue() { AsaCalculator.DEFAULT_PROBE_SIZE, 1000, 1); - AsaCalculator.IndexAndDistance[][] allNbsSh = asaCalc.findNeighborIndicesSpatialHashing(); + AsaCalculator.Neighbors[] allNbsSh = asaCalc.findNeighborIndicesSpatialHashing(); - AsaCalculator.IndexAndDistance[][] allNbs = asaCalc.findNeighborIndices(); + AsaCalculator.Neighbors[] allNbs = asaCalc.findNeighborIndices(); assertEquals(3, allNbs.length); assertEquals(3, allNbsSh.length); for (int indexToTest =0; indexToTest < asaCalc.getAtomCoords().length; indexToTest++) { - AsaCalculator.IndexAndDistance[] nbsSh = allNbsSh[indexToTest]; - AsaCalculator.IndexAndDistance[] nbs = allNbs[indexToTest]; + int[] nbsSh = allNbsSh[indexToTest].indices; + int[] nbs = allNbs[indexToTest].indices; List listOfMatchingIndices = new ArrayList<>(); for (int i = 0; i < nbsSh.length; i++) { for (int j = 0; j < nbs.length; j++) { - if (nbs[j].index == nbsSh[i].index) { + if (nbs[j] == nbsSh[i]) { listOfMatchingIndices.add(j); break; } @@ -256,7 +256,7 @@ public void testNoNeighborsIssue() { } // first atom should have no neighbors - assertEquals(0, allNbsSh[0].length); + assertEquals(0, allNbsSh[0].indices.length); } private Atom getAtom(double x, double y, double z) { From 72b5ed10566b87a0d11b7bd81d05b0af34f95997 Mon Sep 17 00:00:00 2001 From: Jose Duarte Date: Wed, 26 Aug 2026 11:50:07 -0700 Subject: [PATCH 15/26] Fix ArrayIndexOutOfBoundsException and file validation in CathInstallation --- .../nbio/structure/cath/CathInstallation.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index a72e85ed16..737bee9441 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -355,7 +355,9 @@ private void parseCathDomainList(BufferedReader bufferedReader) throws IOExcepti // int counter = 0; while ( (line = bufferedReader.readLine()) != null ) { if ( line.startsWith("#") ) continue; + if ( line.trim().isEmpty() ) continue; CathDomain cathDomain = parseCathListFileLine(line); + if ( cathDomain == null ) continue; // counter++; String pdbId = cathDomain.getPdbIdAndChain().substring(0,4); // includes chain letter @@ -386,7 +388,9 @@ private void parseCathNames(BufferedReader bufferedReader) throws IOException{ //int counter = 0; while ( (line = bufferedReader.readLine()) != null ) { if ( line.startsWith("#") ) continue; + if ( line.trim().isEmpty() ) continue; CathNode cathNode = parseCathNamesFileLine(line); + if ( cathNode == null ) continue; cathTree.put(cathNode.getNodeId(), cathNode); } } @@ -413,6 +417,7 @@ private void parseCathDomainDescriptionFile(BufferedReader bufferedReader) throw StringBuilder sseqs = null; while ( (line = bufferedReader.readLine()) != null ) { if ( line.startsWith("#") ) continue; + if ( line.trim().isEmpty() ) continue; if ( line.startsWith("FORMAT") ) { cathDescription = new CathDomain(); cathDescription.setFormat( line.substring(10) ); @@ -504,8 +509,11 @@ private void parseCathDomainDescriptionFile(BufferedReader bufferedReader) throw }*/ private CathDomain parseCathListFileLine(String line) { + String [] token = line.trim().split("\\s+"); + if (token.length < 12) { + return null; + } CathDomain cathDomain = new CathDomain(); - String [] token = line.split("\\s+"); cathDomain.setDomainName(token[0]); cathDomain.setClassId(Integer.parseInt(token[1])); cathDomain.setArchitectureId(Integer.parseInt(token[2])); @@ -522,8 +530,11 @@ private CathDomain parseCathListFileLine(String line) { } private CathNode parseCathNamesFileLine(String line) { + String[] token = line.trim().split("\\s+",3); + if (token.length < 3) { + return null; + } CathNode cathNode = new CathNode(); - String[] token = line.split("\\s+",3); cathNode.setNodeId( token[0] ); int idx = token[0].lastIndexOf("."); if ( idx == -1 ) idx = token[0].length(); @@ -544,8 +555,8 @@ private void parseCathDomall(BufferedReader bufferedReader) throws IOException{ String line; while ( ((line = bufferedReader.readLine()) != null) ) { if ( line.startsWith("#") ) continue; - if ( line.length() == 0 ) continue; - String[] token = line.split("\\s+"); + if ( line.trim().isEmpty() ) continue; + String[] token = line.trim().split("\\s+"); String chainId = token[0]; Integer numberOfDomains = Integer.parseInt( token[1].substring(1) ); Integer numberOfFragments = Integer.parseInt( token[2].substring(1) ); @@ -671,25 +682,25 @@ protected void downloadFileFromRemote(URL remoteURL, File localFile) throws IOEx private boolean domainDescriptionFileAvailable(){ String fileName = getDomainDescriptionFileName(); File f = new File(fileName); - return f.exists(); + return f.exists() && FileDownloadUtils.validateFile(f); } private boolean domainListFileAvailable(){ String fileName = getDomainListFileName(); File f = new File(fileName); - return f.exists(); + return f.exists() && FileDownloadUtils.validateFile(f); } private boolean nodeListFileAvailable(){ String fileName = getNodeListFileName(); File f = new File(fileName); - return f.exists(); + return f.exists() && FileDownloadUtils.validateFile(f); } private boolean domallFileAvailable() { String fileName = getDomallFileName(); File f= new File(fileName); - return f.exists(); + return f.exists() && FileDownloadUtils.validateFile(f); } protected void downloadDomainListFile() throws IOException{ From 241baafe81712644bff064d4aca37c5b5970b26a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 26 Aug 2026 20:19:42 +0000 Subject: [PATCH 16/26] Bump com.fasterxml.jackson.core:jackson-databind in /biojava-structure Bumps [com.fasterxml.jackson.core:jackson-databind](https://github.com/FasterXML/jackson-databind) from 2.13.4.2 to 2.18.9. - [Commits](https://github.com/FasterXML/jackson-databind/compare/jackson-databind-2.13.4.2...jackson-databind-2.18.9) --- updated-dependencies: - dependency-name: com.fasterxml.jackson.core:jackson-databind dependency-version: 2.18.9 dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- biojava-structure/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/biojava-structure/pom.xml b/biojava-structure/pom.xml index f881200d48..647ab49c72 100644 --- a/biojava-structure/pom.xml +++ b/biojava-structure/pom.xml @@ -87,7 +87,7 @@ com.fasterxml.jackson.core jackson-databind - 2.13.4.2 + 2.18.9 From e0b7797fe4e5a1c0e5f53df4f6cddb6e615da42b Mon Sep 17 00:00:00 2001 From: Amr ALHOSSARY Date: Tue, 18 Aug 2026 10:22:02 -0400 Subject: [PATCH 17/26] Write the entry identifier as a data item, not only as the block name AbstractCifFileSupplier put the PDB identifier only in the data_ block header, so it was lost for every consumer that reads it as a data item: - BioJava's own CifStructureConsumerImpl.consumeStruct reads _struct.entry_id, so Structure.getPdbId() came back null after a write-then-read round trip; - Jmol's mmCIF reader takes _M.pdbID from _entry.id, so a structure handed over with openStringInline(structure.toMMCIF()) arrived with no identifier, which in turn breaks anything keyed on the entry (for instance "isosurface ... eds", which needs an entry to fetch the electron density map for). Write both categories when the structure has an identifier. Both are needed: _entry.id is the canonical item and the one Jmol reads, while BioJava's own reader only looks at _struct.entry_id. Adds a round-trip regression test; there was nothing asserting that an identifier survived toMMCIF(). Fixes #1143 --- .../io/cif/AbstractCifFileSupplier.java | 21 +++++++++- .../io/cif/CifFileSupplierImplTest.java | 40 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java index 6bf8af90ef..e43565c827 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/AbstractCifFileSupplier.java @@ -40,8 +40,27 @@ protected CifFile getInternal(Structure structure, List wrappedAtom // entity information List entityInfos = structure.getEntityInfos(); + PdbId pdbId = structure.getPdbId(); + MmCifBlockBuilder blockBuilder = CifBuilder.enterFile(StandardSchemata.MMCIF) - .enterBlock(structure.getPdbId() == null? "" : structure.getPdbId().getId()); + .enterBlock(pdbId == null? "" : pdbId.getId()); + + if (pdbId != null) { + // The block header alone does not carry the identifier for consumers: readers pick it up from + // _entry.id (e.g. Jmol) or from _struct.entry_id (BioJava's own CifStructureConsumerImpl). + // Both are written so that the identifier survives a write-then-read round trip either way. + blockBuilder.enterEntry() + .enterId() + .add(pdbId.getId()) + .leaveColumn() + .leaveCategory(); + + blockBuilder.enterStruct() + .enterEntryId() + .add(pdbId.getId()) + .leaveColumn() + .leaveCategory(); + } blockBuilder.enterStructKeywords().enterText() .add(String.join(", ", structure.getPDBHeader().getKeywords())) diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java index df227a8669..1d5f496ff5 100644 --- a/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java +++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/io/cif/CifFileSupplierImplTest.java @@ -1,5 +1,6 @@ package org.biojava.nbio.structure.io.cif; +import org.biojava.nbio.structure.PdbId; import org.biojava.nbio.structure.Structure; import org.biojava.nbio.structure.io.FileParsingParameters; import org.biojava.nbio.structure.io.PDBFileParser; @@ -41,4 +42,43 @@ public void shouldReadRawPdbOutputtingCifWithEntity() throws IOException { } } + + /** + * The identifier must be written as a data item and not only as the name of the data block: consumers read it + * from _entry.id or from _struct.entry_id, so writing the block header alone loses it. See issue #1143. + */ + @Test + public void shouldWriteEntryIdAndSurviveRoundTrip() throws IOException { + Structure s; + try (InputStream inStream = new GZIPInputStream(this.getClass().getResourceAsStream("/4hhb.cif.gz"))) { + s = CifStructureConverter.fromInputStream(inStream); + } + assertEquals(new PdbId("4HHB"), s.getPdbId()); + + String cifText = CifStructureConverter.toText(s); + assertTrue("_entry.id must be written", cifText.contains("_entry.id")); + assertTrue("_struct.entry_id must be written", cifText.contains("_struct.entry_id")); + + Structure readStruct = CifStructureConverter.fromInputStream( + new ByteArrayInputStream(cifText.getBytes())); + + assertEquals(s.getPdbId(), readStruct.getPdbId()); + assertEquals(s.getPdbId(), readStruct.getPDBHeader().getPdbId()); + } + + /** + * Structures without an identifier must not gain empty entry categories. + */ + @Test + public void shouldNotWriteEntryIdWhenPdbIdIsAbsent() throws IOException { + Structure s; + try (InputStream inStream = new GZIPInputStream(this.getClass().getResourceAsStream("/4hhb.cif.gz"))) { + s = CifStructureConverter.fromInputStream(inStream); + } + s.setPdbId(null); + + String cifText = CifStructureConverter.toText(s); + assertFalse(cifText.contains("_entry.id")); + assertFalse(cifText.contains("_struct.entry_id")); + } } From 83fe61a4718e096bfca3f622d2b51dc247c35adc Mon Sep 17 00:00:00 2001 From: josemduarte Date: Wed, 26 Aug 2026 15:43:52 -0700 Subject: [PATCH 18/26] Safeguard for case of nothing parseable in file --- .../nbio/structure/cath/CathInstallation.java | 9 +- .../structure/cath/CathInstallationTest.java | 84 +++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index 737bee9441..7e4b9abbc9 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -350,15 +350,15 @@ private void parseCathDomainList() throws IOException { parseCathDomainList(buffer); } - private void parseCathDomainList(BufferedReader bufferedReader) throws IOException{ + protected void parseCathDomainList(BufferedReader bufferedReader) throws IOException{ String line; - // int counter = 0; + int counter = 0; while ( (line = bufferedReader.readLine()) != null ) { if ( line.startsWith("#") ) continue; if ( line.trim().isEmpty() ) continue; CathDomain cathDomain = parseCathListFileLine(line); if ( cathDomain == null ) continue; - // counter++; + counter++; String pdbId = cathDomain.getPdbIdAndChain().substring(0,4); // includes chain letter @@ -374,6 +374,9 @@ private void parseCathDomainList(BufferedReader bufferedReader) throws IOExcepti domainMap.put( cathDomain.getDomainName(), cathDomain ); } + if (counter == 0) { + throw new IOException("Could not parse any CATH domains from the domain list file."); + } } private void parseCathNames() throws IOException { diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java new file mode 100644 index 0000000000..efad1ef077 --- /dev/null +++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java @@ -0,0 +1,84 @@ +/* + * BioJava development code + * + * This code may be freely distributed and modified under the + * terms of the GNU Lesser General Public Licence. This should + * be distributed with the code. If you do not have a copy, + * see: + * + * http://www.gnu.org/copyleft/lesser.html + * + * Copyright for this code is held jointly by the individual + * authors. These should be listed in @author doc comments. + * + * For more information on the BioJava project and its aims, + * or to join the biojava-l mailing list, visit the home page + * at: + * + * http://www.biojava.org/ + */ +package org.biojava.nbio.structure.cath; + +import org.junit.jupiter.api.Test; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.StringReader; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class CathInstallationTest { + + @Test + public void testParseCathDomainListSuccess() throws IOException { + String data = "# CATH domain list\n" + + "\n" + + "1oaiA00 1 10 490 10 1 1 1 1 1 124 1.80\n" + + "1oaiA01 1 10 490 10 1 1 1 1 2 150 1.80\n"; + + CathInstallation installation = new CathInstallation(""); + BufferedReader reader = new BufferedReader(new StringReader(data)); + installation.parseCathDomainList(reader); + + CathDomain domain = installation.getDomainByCathId("1oaiA00"); + assertNotNull(domain); + assertEquals("1oaiA00", domain.getDomainName()); + assertEquals(1, domain.getClassId()); + assertEquals(10, domain.getArchitectureId()); + assertEquals(490, domain.getTopologyId()); + assertEquals(10, domain.getHomologyId()); + assertEquals(124, domain.getLength()); + assertEquals(1.80, domain.getResolution(), 0.001); + } + + @Test + public void testParseCathDomainListEmptyThrowsException() { + CathInstallation installation = new CathInstallation(""); + BufferedReader reader = new BufferedReader(new StringReader("")); + assertThrows(IOException.class, () -> installation.parseCathDomainList(reader)); + } + + @Test + public void testParseCathDomainListOnlyCommentsAndWhitespaceThrowsException() { + String data = "# comment 1\n" + + "# comment 2\n" + + " \n" + + "\t\n"; + CathInstallation installation = new CathInstallation(""); + BufferedReader reader = new BufferedReader(new StringReader(data)); + assertThrows(IOException.class, () -> installation.parseCathDomainList(reader)); + } + + @Test + public void testParseCathDomainListNoParsableLinesThrowsException() { + String data = "# comment\n" + + "invalid line with too few tokens\n" + + "another bad line\n"; + CathInstallation installation = new CathInstallation(""); + BufferedReader reader = new BufferedReader(new StringReader(data)); + IOException exception = assertThrows(IOException.class, () -> installation.parseCathDomainList(reader)); + assertNotNull(exception.getMessage()); + } +} From 27bb418c2787cc5848d871bb5a4ae2a0145b4456 Mon Sep 17 00:00:00 2001 From: josemduarte Date: Wed, 26 Aug 2026 15:50:44 -0700 Subject: [PATCH 19/26] Logging --- .../java/org/biojava/nbio/structure/cath/CathInstallation.java | 1 + 1 file changed, 1 insertion(+) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index 7e4b9abbc9..2caa8652dc 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -535,6 +535,7 @@ private CathDomain parseCathListFileLine(String line) { private CathNode parseCathNamesFileLine(String line) { String[] token = line.trim().split("\\s+",3); if (token.length < 3) { + LOGGER.info("Invalid line in cath names file, was expecting 3 tokens but got {} tokens: {}", token.length, line); return null; } CathNode cathNode = new CathNode(); From 26c03d12d0b557eb24c75d917bd98bf2e5109ed4 Mon Sep 17 00:00:00 2001 From: Jose Duarte Date: Wed, 26 Aug 2026 21:44:37 -0700 Subject: [PATCH 20/26] Demoting --- .../java/org/biojava/nbio/structure/cath/CathInstallation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java index 2caa8652dc..4201f8d5ca 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/cath/CathInstallation.java @@ -535,7 +535,7 @@ private CathDomain parseCathListFileLine(String line) { private CathNode parseCathNamesFileLine(String line) { String[] token = line.trim().split("\\s+",3); if (token.length < 3) { - LOGGER.info("Invalid line in cath names file, was expecting 3 tokens but got {} tokens: {}", token.length, line); + LOGGER.debug("Invalid line in cath names file, was expecting 3 tokens but got {} tokens: {}", token.length, line); return null; } CathNode cathNode = new CathNode(); From 954d75a9dedf7136ce35c8c85aa6e3a519000cce Mon Sep 17 00:00:00 2001 From: josemduarte Date: Thu, 27 Aug 2026 08:56:43 -0700 Subject: [PATCH 21/26] Test fix suggested by AI: test was failing when download was too slow --- .../org/biojava/nbio/structure/cath/CathInstallationTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java index efad1ef077..69944cb51c 100644 --- a/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java +++ b/biojava-structure/src/test/java/org/biojava/nbio/structure/cath/CathInstallationTest.java @@ -24,6 +24,7 @@ import java.io.BufferedReader; import java.io.IOException; import java.io.StringReader; +import java.util.concurrent.atomic.AtomicBoolean; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -41,6 +42,8 @@ public void testParseCathDomainListSuccess() throws IOException { CathInstallation installation = new CathInstallation(""); BufferedReader reader = new BufferedReader(new StringReader(data)); installation.parseCathDomainList(reader); + installation.setInstalledDomainList(new AtomicBoolean(true)); //1 + installation.setInstalledDomall(new AtomicBoolean(true)); //2 CathDomain domain = installation.getDomainByCathId("1oaiA00"); assertNotNull(domain); From 8af5253c9a5390f9f24409aab463d5e7f51b31c1 Mon Sep 17 00:00:00 2001 From: josemduarte Date: Thu, 27 Aug 2026 11:33:29 -0700 Subject: [PATCH 22/26] Logging cosmetics --- .../org/biojava/nbio/structure/ecod/EcodInstallation.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java index 5bd59dd14b..f5d8d11be3 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java @@ -1021,10 +1021,10 @@ private void warnMissingColumns(int lineNum) { private void warnUnparseableLine(int lineNum, IllegalArgumentException e) { if(warnNumberFormat > 1) { - logger.warn("Error in ECOD parsing at line "+lineNum,e); + logger.warn("Error in ECOD parsing at line {}: {}", lineNum, e.getMessage()); warnNumberFormat--; } else if(warnNumberFormat == 1) { - logger.warn("Error in ECOD parsing at line "+lineNum+". Not printing future similar warnings",e); + logger.warn("Error in ECOD parsing at line {}: {}. Not printing future similar warnings", lineNum, e.getMessage()); warnNumberFormat--; } } From d6a960419d3690e472a6367aaec65952f4514a23 Mon Sep 17 00:00:00 2001 From: josemduarte Date: Thu, 27 Aug 2026 12:04:45 -0700 Subject: [PATCH 23/26] Logging for resolution parsing: warn only when they are actually different --- .../main/java/org/biojava/nbio/structure/io/PDBFileParser.java | 2 +- .../nbio/structure/io/cif/CifStructureConsumerImpl.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java index f5cc851fec..b1d327599e 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java @@ -1406,7 +1406,7 @@ public void handleResolutionLine(String line, Pattern pR) { try { float res = Float.parseFloat(resString); final float resInHeader = pdbHeader.getResolution(); - if (resInHeader!=PDBHeader.DEFAULT_RESOLUTION && resInHeader != res) { + if (resInHeader!=PDBHeader.DEFAULT_RESOLUTION && Math.abs(resInHeader - res) > 0.001) { logger.warn("More than 1 resolution value present, will use last one {} and discard previous {} " ,resString, String.format("%4.2f",resInHeader)); } diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java index 94b96b13f8..f44eb44ab1 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java @@ -856,7 +856,8 @@ public void consumeRefine(Refine refine) { // we take the last one found so that behaviour is like in PDB file parsing double lsDResHigh = refine.getLsDResHigh().get(rowIndex); // TODO this could use a check to keep reasonable values - 1.5 may be overwritten by 0.0 - if (pdbHeader.getResolution() != PDBHeader.DEFAULT_RESOLUTION) { + if (pdbHeader.getResolution() != PDBHeader.DEFAULT_RESOLUTION && + Math.abs(pdbHeader.getResolution() - lsDResHigh) > 0.001) { logger.warn("More than 1 resolution value present, will use last one {} and discard previous {}", lsDResHigh, String.format("%4.2f",pdbHeader.getResolution())); } From 92adddc79422cba001aa6ac31aafaf8249d4927b Mon Sep 17 00:00:00 2001 From: Sergey Pyatykh Date: Fri, 28 Aug 2026 13:11:00 +0200 Subject: [PATCH 24/26] Finish biojava-genome JUnit 5 migration and drop junit-addons Replace FileAssert and leftover JUnit 4 APIs so genome tests compile without junit-addons. --- biojava-genome/pom.xml | 16 ---------------- .../nbio/genome/GeneFeatureHelperTest.java | 14 +++++++++----- .../nbio/genome/TestChromosomeMappingTools.java | 2 -- .../biojava/nbio/genome/io/fastq/FastqTest.java | 10 +--------- 4 files changed, 10 insertions(+), 32 deletions(-) diff --git a/biojava-genome/pom.xml b/biojava-genome/pom.xml index 34b3facf81..e469de412c 100644 --- a/biojava-genome/pom.xml +++ b/biojava-genome/pom.xml @@ -82,22 +82,6 @@ 7.2.7-SNAPSHOT compile - - junit-addons - junit-addons - 1.4 - test - - - xerces - xmlParserAPIs - - - xerces - xercesImpl - - - org.slf4j diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java index 9c3a8877c6..53ab7c20cb 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/GeneFeatureHelperTest.java @@ -20,7 +20,6 @@ */ package org.biojava.nbio.genome; -import junitx.framework.FileAssert; import org.biojava.nbio.genome.parsers.gff.FeatureList; import org.biojava.nbio.genome.parsers.gff.GFF3Reader; import org.biojava.nbio.genome.parsers.gff.GFF3Writer; @@ -29,6 +28,7 @@ import org.biojava.nbio.core.sequence.ProteinSequence; import org.biojava.nbio.core.sequence.io.FastaWriterHelper; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.slf4j.Logger; @@ -100,8 +100,10 @@ void testOutputFastaSequenceLengthGFF3() throws Exception { File gffFile = Files.createTempFile("volvox_length","gff3").toFile(); gffFile.deleteOnExit(); GeneFeatureHelper.outputFastaSequenceLengthGFF3(fastaSequenceFile, gffFile); - FileAssert.assertEquals("volvox_length.gff3 and volvox_length_output.gff3 are not equal", gffFile, - new File("src/test/resources/volvox_length_reference.gff3")); + Assertions.assertEquals( + Files.readString(new File("src/test/resources/volvox_length_reference.gff3").toPath()), + Files.readString(gffFile.toPath()), + "volvox_length.gff3 and volvox_length_output.gff3 are not equal"); } @@ -140,8 +142,10 @@ void testGetProteinSequences() throws Exception { File tmp = Files.createTempFile("volvox_all","faa").toFile(); tmp.deleteOnExit(); FastaWriterHelper.writeProteinSequence(tmp, proteinSequenceList.values()); - FileAssert.assertEquals("volvox_all_reference.faa and volvox_all.faa are not equal", new File( - "src/test/resources/volvox_all_reference.faa"), tmp); + Assertions.assertEquals( + Files.readString(new File("src/test/resources/volvox_all_reference.faa").toPath()), + Files.readString(tmp.toPath()), + "volvox_all_reference.faa and volvox_all.faa are not equal"); } /** diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java index a86b8718c5..b17539a0ac 100644 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/TestChromosomeMappingTools.java @@ -28,8 +28,6 @@ import java.util.Arrays; import java.util.List; -import static org.junit.Assert.assertEquals; - /** * Created by Yana Valasatava on 8/14/17. */ diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java index 31102b251d..5bcbb43ee9 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqTest.java @@ -20,12 +20,9 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Assert; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import org.junit.function.ThrowingRunnable; - /** * Unit test for Fastq. */ @@ -116,12 +113,7 @@ void testBuilder() @Test void testBuilderNullFastq() { - Assert.assertThrows(IllegalArgumentException.class, new ThrowingRunnable() { - @Override - public void run() { - Fastq.builder(null); - } - }); + Assertions.assertThrows(IllegalArgumentException.class, () -> Fastq.builder(null)); } @Test From 0e18f042dad3228eda941499e395ecfc670bf233 Mon Sep 17 00:00:00 2001 From: Sergey Pyatykh Date: Fri, 28 Aug 2026 13:20:37 +0200 Subject: [PATCH 25/26] Finish biojava-genome JUnit 5 migration and drop junit-addons Replace leftover JUnit 4 APIs so genome tests compile without junit-addons. --- .../biojava/nbio/genome/io/fastq/FastqBuilderTest.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java index 3307256126..46957aeb26 100755 --- a/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java +++ b/biojava-genome/src/test/java/org/biojava/nbio/genome/io/fastq/FastqBuilderTest.java @@ -20,12 +20,9 @@ */ package org.biojava.nbio.genome.io.fastq; -import org.junit.Assert; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import org.junit.function.ThrowingRunnable; - /** * Unit test for FastqBuilder. */ @@ -62,12 +59,7 @@ void testConstructorFastq() @Test void testConstructorNullFastq() { - Assert.assertThrows(IllegalArgumentException.class, new ThrowingRunnable() { - @Override - public void run() { - new FastqBuilder(null); - } - }); + Assertions.assertThrows(IllegalArgumentException.class, () -> new FastqBuilder(null)); } @Test From dd9092b5301dca5eefb9798ff9f92eebc7720644 Mon Sep 17 00:00:00 2001 From: Amr ALHOSSARY Date: Sun, 30 Aug 2026 07:41:52 -0400 Subject: [PATCH 26/26] Keep the ECOD read lock balanced when loading fails getDomainsForPdb and getAllDomains release the read lock inside their try block, so that the loader they call can take the write lock, and re-acquire it afterwards. When that loader throws, the re-acquisition never happens, and the outer finally unlocks a lock the thread no longer holds. The resulting IllegalMonitorStateException is thrown from a finally block, so it supersedes the IOException that actually caused the failure. Last night's nightly showed this: one upstream change - ECOD now answers with a 308 that HttpURLConnection will not follow - produced eight failures with three different-looking causes, only three of which named the redirect. Re-acquiring in a finally of its own keeps the lock balanced on both paths, so the original exception propagates intact. --- .../nbio/structure/ecod/EcodInstallation.java | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java index 5bd59dd14b..aeb1c50ee5 100644 --- a/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java +++ b/biojava-structure/src/main/java/org/biojava/nbio/structure/ecod/EcodInstallation.java @@ -137,9 +137,15 @@ public List getDomainsForPdb(String id) throws IOException { // unlock to allow ensureDomainsFileInstalled to get the write lock logger.trace("UNLOCK readlock"); domainsFileLock.readLock().unlock(); - indexDomains(); - domainsFileLock.readLock().lock(); - logger.trace("LOCK readlock"); + try { + indexDomains(); + } finally { + // re-acquire even if indexing failed, so the outer finally has a + // lock to release; otherwise IllegalMonitorStateException replaces + // the real cause and the failure becomes unreadable + domainsFileLock.readLock().lock(); + logger.trace("LOCK readlock"); + } } PdbId pdbId = null; @@ -244,9 +250,15 @@ public List getAllDomains() throws IOException { // unlock to allow ensureDomainsFileInstalled to get the write lock logger.trace("UNLOCK readlock"); domainsFileLock.readLock().unlock(); - ensureDomainsFileInstalled(); - domainsFileLock.readLock().lock(); - logger.trace("LOCK readlock"); + try { + ensureDomainsFileInstalled(); + } finally { + // re-acquire even if the download failed, so the outer finally has a + // lock to release; otherwise IllegalMonitorStateException replaces + // the real cause and the failure becomes unreadable + domainsFileLock.readLock().lock(); + logger.trace("LOCK readlock"); + } } return allDomains; } finally {