Skip to content

Do not automatically try downloading from a .gz URL from known broken servers. - #41

Open
gouttegd wants to merge 1 commit into
mainfrom
deal-with-broken-servers
Open

gouttegd wants to merge 1 commit into
mainfrom
deal-with-broken-servers

Conversation

@gouttegd

@gouttegd gouttegd commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

The default behaviour of the odk-helper download tool, whenever it is asked to download a file from a given url U, is to first try to download the file from U.gz (that is, downloading a compressed version, which will be automatically uncompressed on the fly).

But that some badly configured HTTP servers will wreak havoc if that behaviour is used with them. Notably, the www.ebi.ac.uk server returns a normal status code (200), along with a HTML home page, whenever it is asked for a non-existing resource.

This means that if a ODK ontology is importing EFO (http://www.ebi.ac.uk/efo/efo.owl), when odk-helper download will attempt to download http://www.ebi.ac.uk/efo/efo.owl.gz, instead of receiving a 404 error that would indicate that efo/efo.owl.gz does not exist (which would then trigger the fallback to efo/efo.owl), it will receive a 200 status code indicating that everything is fine -- and will then run into an error when trying to uncompress a HTML page that is not compressed at all.

Until such servers are fixed to behave correctly (not returning a 200 status code for a resource that does not exist), we have to explicitly exclude them from the "try-gzip-first" behaviour. This is done by having a list of known broken servers in src/ontology/config/broken-hosts.txt, that odk-helper download will query before deciding whether to try fetching a compressed file.

The default behaviour of the `odk-helper download` tool, whenever it is
asked to download a file from a given url _U_, is to first try to
download the file from _U.gz_ (that is, downloading a compressed
version, which will be automatically uncompressed on the fly).

But that some badly configured HTTP servers will wreak havoc if that
behaviour is used with them. Notably, the www.ebi.ac.uk server returns a
_normal_ status code (200), along with a HTML home page, whenever it is
asked for a non-existing resource.

This means that if a ODK ontology is importing EFO
(http://www.ebi.ac.uk/efo/efo.owl), when `odk-helper download` will
attempt to download http://www.ebi.ac.uk/efo/efo.owl.gz, instead of
receiving a 404 error that would indicate that `efo/efo.owl.gz` does not
exist (which would then trigger the fallback to `efo/efo.owl`), it will
receive a 200 status code indicating that everything is fine -- and will
then run into an error when trying to uncompress a HTML page that is not
compressed at all.

Until such servers are fixed to behave correctly (_not_ returning a 200
status code for a resource that does not exist), we have to explicitly
exclude them from the "try-gzip-first" behaviour. This is done by having
a list of known broken servers in
`src/ontology/config/broken-hosts.txt`, that `odk-helper download` will
query before deciding whether to try fetching a compressed file.
@gouttegd gouttegd self-assigned this Oct 11, 2026
@gouttegd
gouttegd requested a review from matentzn October 11, 2026 19:17
urlpath = Path(urlobj.path)
compression = Compression.from_extension(urlpath)

broken_hosts: List[str] = []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GzipFile.read() on some cases raises gzip.BadGzipFile (an OSError), not DownloadError, so the except DownloadError in the attempts loop below never catches it (like http://www.ebi.ac.uk/efo/efo.owl.gz). so we crash instead of falling back to the plain URL.

If _handle_successful_download in download.py caught the decompression error, deleted the tmp file and raised DownloadError, the existing fallback would just work, for EBI and any other server that answers 200 for things that don't exist (something like this, to get the idea):

try:
    with tmpfile.open("wb") as fd:
        for chunk in _ResponseWrapper.maybe_wrap(response, comp).iter_content(512):
            h.update(chunk)
            fd.write(chunk)
except (OSError, EOFError) as e:
    tmpfile.unlink(missing_ok=True)
    raise DownloadError(f"Cannot decompress file from {response.url}: {e}")

(EOFError is for truncated gzip. OSError also catches real disk-write errors, so maybe catch only around the decompression if you want to keep those apart.)

def download(url, output, reference, cache_info, max_retry, try_gzip) -> None:
@click.option(
"--broken-hosts-file",
default="config/broken-hosts.txt",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, not so much a fan of a list.. Cant we just try and fail to decompress?

if (
compression == Compression.NONE
and try_gzip
and urlobj.netloc not in broken_hosts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

netloc includes the port and userinfo, and it's case-sensitive, so http://www.ebi.ac.uk:80/efo/efo.owl or WWW.EBI.AC.UK gets past this check. Maybe urlobj.hostname?

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.

2 participants