Fix CATH downloads: use https, and check the response before caching it - #1136
Fix CATH downloads: use https, and check the response before caching it#1136aalhossary wants to merge 4 commits into
Conversation
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.
|
To be explicit about scope, since #1135 lists three separate causes and this PR only addresses one:
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 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 Verification for this one: |
|
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. |
Fixes cause 1 of 3 in #1135.
download.cathdb.infonow answers plain http with a 301 to https.HttpURLConnectionfollows redirects within a protocol but deliberately will not follow one that switches http to https, so our download never reached the real file.downloadFileFromRemoteread the response with a bareopenStream()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 aNullPointerExceptionmuch later, far from the cause: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
CATH_DOWNLOAD_URLuseshttps, which fixes the immediate breakage.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.infosends no ETag, so there is no checksum to record, but the size check alone would have caught a truncated file.CathDomainTestpasses again, in about 11 seconds.Note on the commit list
This branch is stacked on #1133, which is where
downloadFileWithValidationcomes from. GitHub therefore shows #1133's three commits here as well. The CATH change itself is the single commit7244f21— reviewing that one commit is enough to judge this PR.If you would rather not couple the two, the
httpsone-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.