Repository: datalad/datalad-next

Issue:
http-downloads via `download` (`HttpUrlOperations`) result in gzipped files on disk

Discussion:
mih: I have exactly the same question in my head. thx!

The current test verifies that not decoding an uncompressed file obtained via a compressed download was broken, and is not fixed. The other way round (download something that should remain compressed) is not yet tested.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) Report
Patch coverage: **`100.00`**% and project coverage change: **`-4.01`** :warning:
> Comparison is base [(`46db8c6`)](https://app.codecov.io/gh/datalad/datalad-next/commit/46db8c6669b8a57eebf85bf001043ac1b4851f2d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) 92.22% compared to head [(`6984fd7`)](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) 88.22%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #365      +/-   ##
==========================================
- Coverage   92.22%   88.22%   -4.01%     
==========================================
  Files          95       95              
  Lines        7747     7601     -146     
==========================================
- Hits         7145     6706     -439     
- Misses        602      895     +293     
```


| [Impacted Files](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) | Coverage Δ | |
|---|---|---|
| [datalad\_next/url\_operations/http.py](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9uZXh0L3VybF9vcGVyYXRpb25zL2h0dHAucHk=) | `92.39% <100.00%> (+0.25%)` | :arrow_up: |
| [datalad\_next/url\_operations/tests/test\_http.py](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9uZXh0L3VybF9vcGVyYXRpb25zL3Rlc3RzL3Rlc3RfaHR0cC5weQ==) | `56.70% <100.00%> (+0.75%)` | :arrow_up: |

... and [13 files with indirect coverage changes](https://app.codecov.io/gh/datalad/datalad-next/pull/365/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/datalad/datalad-next/pull/365?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad).

mih: There is something weird going on with the test that ensures progress reporting. Locally it is susceptible to changes in download chunksize, but even with no change it works sometimes. Grrrr.
mih: Now that the progress reporting reflects bytes-downloaded and not bytes decompressed, the test file size is too small to reliably take long enough for a progress update to appear
christian-monch: First observation: in failing tests download logs progresses with `tell=0` and `download=0`.

christian-monch: I got a 200 HTTP-response without `content-length` header. The reason that there is no `content-length` is, that the transfer-encoding is `chunked`.  I am not sure, when and why github returns the header, it might be related to locally cached data or CDN settings. Anyway, it is a valid response, and the client has no way of telling the overall size in advance. Commit 87858877ca9c32ed39f584591d530e8a5b59e072 should handle size reporting correctly in both cases, i.e. chunked and non-chunked

```python
{
    'Access-Control-Allow-Origin': 'https://render.githubusercontent.com',
    'content-disposition': 'attachment; filename=example-dicom-structural-f077bcc8d502ce8155507bd758cb3b7ccc887f40.zip',
    'Content-Security-Policy': "default-src 'none'; style-src 'unsafe-inline'; sandbox",
    'Content-Type': 'application/zip',
    'ETag': 'W/"55b2f976c68bdaeeec863a84b111afad6e788d3414cc34bf1ef4a925063bd72b"',
    'Strict-Transport-Security': 'max-age=31536000',
    'Vary': 'Authorization,Accept-Encoding,Origin',
    'X-Content-Type-Options': 'nosniff',
    'X-Frame-Options': 'deny',
    'X-XSS-Protection': '1; mode=block',
    'Date': 'Mon, 22 May 2023 11:53:24 GMT',
    'Transfer-Encoding': 'chunked',
    'X-GitHub-Request-Id': '0501:E603:C93E1E:F0E024:646B57B4'
}
```


For reference, a response header for non-chunked transfer from the same request:

```python
{
    'Access-Control-Allow-Origin': 'https://render.githubusercontent.com',
    'content-disposition': 'attachment; filename=example-dicom-structural-f077bcc8d502ce8155507bd758cb3b7ccc887f40.zip',
    'Content-Length': '16212183',
    'Content-Security-Policy': "default-src 'none'; style-src 'unsafe-inline'; sandbox",
    'Content-Type': 'application/zip',
    'ETag': 'W/"55b2f976c68bdaeeec863a84b111afad6e788d3414cc34bf1ef4a925063bd72b"',
    'Strict-Transport-Security': 'max-age=31536000',
    'Vary': 'Authorization,Accept-Encoding,Origin',
    'X-Content-Type-Options': 'nosniff',
    'X-Frame-Options': 'deny',
    'X-XSS-Protection': '1; mode=block',
    'Date': 'Mon, 22 May 2023 12:01:00 GMT',
    'X-GitHub-Request-Id': '055C:BBAA:9A54EC:BA29AC:646B597C'
}
```


Patch:
diff --git a/datalad_next/url_operations/http.py b/datalad_next/url_operations/http.py
index 8d31c03..63ddb89 100644
--- a/datalad_next/url_operations/http.py
+++ b/datalad_next/url_operations/http.py
@@ -9,7 +9,6 @@ import sys
 from typing import Dict
 import requests
 from requests_toolbelt import user_agent
-from requests_toolbelt.downloadutils.tee import tee as requests_tee
 import www_authenticate
 
 import datalad
@@ -247,24 +246,50 @@ class HttpUrlOperations(UrlOperations):
         progress_id = self._get_progress_id(from_url, to_path)
         # get download size, but not every server provides it
         try:
+            # for compressed downloads the content length refers to the
+            # compressed content
             expected_size = int(r.headers.get('content-length'))
         except (ValueError, TypeError):
+            # some responses do not have a `content-length` header,
+            # even though they HTTP200 and deliver the content.
+            # example:
+            # https://github.com/datalad/datalad-next/pull/365#issuecomment-1557114109
             expected_size = None
         self._progress_report_start(
             progress_id,
             ('Download %s to %s', from_url, to_path),
             'downloading',
+            # can be None, and that is OK
             expected_size,
         )
 
         fp = None
         props = {}
         try:
+            # we can only write to file-likes opened in bytes mode
             fp = sys.stdout.buffer if to_path is None else open(to_path, 'wb')
-            # TODO make chunksize a config item
-            for chunk in requests_tee(r, fp):
+            # we need to track how much came down the pipe for progress
+            # reporting
+            downloaded_bytes = 0
+            # TODO make chunksize a config item, 65536 is the default in
+            # requests_toolbelt
+            for chunk in r.raw.stream(amt=65536, decode_content=True):
+                # update how much data was transferred from the remote server,
+                # but we cannot use the size of the chunk for that,
+                # because content might be downloaded with transparent
+                # (de)compression. ask the download stream itself for its
+                # "position"
+                if expected_size:
+                    tell = r.raw.tell()
+                else:
+                    tell = downloaded_bytes + len(chunk)
                 self._progress_report_update(
-                    progress_id, ('Downloaded chunk',), len(chunk))
+                    progress_id,
+                    ('Downloaded chunk',),
+                    tell - downloaded_bytes,
+                )
+                fp.write(chunk)
+                downloaded_bytes = tell
                 # compute hash simultaneously
                 hasher.update(chunk)
             props.update(hasher.get_hexdigest())


Test patch:
diff --git a/datalad_next/url_operations/tests/test_http.py b/datalad_next/url_operations/tests/test_http.py
index 817d756..9850b00 100644
--- a/datalad_next/url_operations/tests/test_http.py
+++ b/datalad_next/url_operations/tests/test_http.py
@@ -1,4 +1,4 @@
-from pathlib import Path
+import gzip
 import pytest
 
 from ..any import AnyUrlOperations
@@ -57,3 +57,42 @@ def test_custom_http_headers_via_config(datalad_cfg):
     auo = AnyUrlOperations()
     huo = auo._get_handler(f'http://example.com')
     assert huo._headers['X-Funky'] == 'Stuff'
+
+
+def test_transparent_decompression(tmp_path):
+    # this file is offered with transparent compression/decompression
+    # by the github webserver
+    url = 'https://raw.githubusercontent.com/datalad/datalad-next/' \
+          'd0c4746425a48ef20e3b1c218e68954db9412bee/pyproject.toml'
+    dpath = tmp_path / 'test.txt'
+    ops = HttpUrlOperations()
+    ops.download(from_url=url, to_path=dpath)
+
+    # make sure it ends up on disk uncompressed
+    assert dpath.read_text() == \
+        '[build-system]\nrequires = ["setuptools >= 43.0.0", "wheel"]\n'
+
+
+def test_compressed_file_stay_compressed(tmp_path):
+    # this file is offered with transparent compression/decompression
+    # by the github webserver, but is also actually gzip'ed
+    url = \
+        'https://github.com/datalad/datalad-neuroimaging/raw/' \
+        '05b45c8c15d24b6b894eb59544daa17159a88945/' \
+        'datalad_neuroimaging/tests/data/files/nifti1.nii.gz'
+
+    # first confirm validity of the test approach, opening an
+    # uncompressed file should raise an exception
+    with pytest.raises(gzip.BadGzipFile):
+        testpath = tmp_path / 'uncompressed'
+        testpath.write_text('some')
+        with gzip.open(testpath, 'rb') as f:
+            f.read(1000)
+
+    # and now with a compressed file
+    dpath = tmp_path / 'test.nii.gz'
+    ops = HttpUrlOperations()
+    ops.download(from_url=url, to_path=dpath)
+    # make sure it ends up on disk compressed!
+    with gzip.open(dpath, 'rb') as f:
+        f.read(1000)