Skip to content

Fix CATH downloads: use https, and check the response before caching it - #1136

Closed
aalhossary wants to merge 4 commits into
biojava:masterfrom
aalhossary:aa/cath-https-download
Closed

Fix CATH downloads: use https, and check the response before caching it#1136
aalhossary wants to merge 4 commits into
biojava:masterfrom
aalhossary:aa/cath-https-download

Conversation

@aalhossary

Copy link
Copy Markdown
Member

Fixes cause 1 of 3 in #1135.

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 our 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 — a one-line dependency bump fails exactly as a feature branch does.

Changes

  1. CATH_DOWNLOAD_URL uses https, which fixes the immediate breakage.
  2. 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. 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.

Note on the commit list

This branch is stacked on #1133, which is where downloadFileWithValidation comes from. GitHub therefore shows #1133's three commits here as well. The CATH change itself is the single commit 7244f21 — reviewing that one commit is enough to judge this PR.

If you would rather not couple the two, the https one-liner alone turns the test green. I would not recommend stopping there: without the status check, the next time a service changes a redirect we get the same silent corruption and the same puzzling NPE, and it will take just as long to work out why.

The download-validation helpers added in 7.0.0 (biojava#979, biojava#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 <mtime>-<size> 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.
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.
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.
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.
@aalhossary

Copy link
Copy Markdown
Member Author

To be explicit about scope, since #1135 lists three separate causes and this PR only addresses one:

# Cause Status
1 CATH downloader follows a stale http:// URL that now 301s to https fixed here
2 EcodInstallationTest.testVersion downloads 657 MB from a slow server not addressed — needs a decision, see #1135
3 Master Build fails on an expired/absent SONAR_TOKEN not addressed — needs someone with the SonarCloud credentials

So merging this does not turn CI green on its own. It removes one of the two test failures that currently fail every pull request, and it fixes a real runtime bug: anyone calling CathInstallation today silently caches the body of a 301 instead of the classification data.

Cause 2 is the other test failure, and it is a judgement call rather than a bug — the download genuinely works, it is just 657 MB from a slow host, pulled once per matrix job. I have deliberately not picked an option for you.

Cause 3 only affects Master Build, which does not run on pull requests, so it is invisible here but keeps master red.

Verification for this one: CathDomainTest fails on master and passes on this branch in about 11 seconds.

mvn verify -pl biojava-integrationtest -Dtest=CathDomainTest

@aalhossary

Copy link
Copy Markdown
Member Author

Closing: this belongs in #1133 rather than as a separate pull request, since it builds on the download hardening there. The CATH commit has been moved onto that branch.

@aalhossary aalhossary closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant