Repository: datalad/datalad-next

Issue:
`UrlOperations.sniff()` exceptions should distinguish connection error from access error
When a URL works in principle, but points to something that is just not there (e.g. HTTP404) vs we cannot tell right now (HTTP5xx, or network down).

This is needed for a reliable operation of CHECKPRESENT in git-annex special remotes, and for meaningful error reporting in general.

Discussion:

Patch:
diff --git a/datalad_next/exceptions.py b/datalad_next/exceptions.py
index 85015a3..4e99d16 100644
--- a/datalad_next/exceptions.py
+++ b/datalad_next/exceptions.py
@@ -4,8 +4,17 @@ from datalad.runner import CommandError
 from datalad.support.exceptions import (
     AccessDeniedError,
     AccessFailedError,
-    NoDatasetFound,
     CapturedException,
-    IncompleteResultsError,
     DownloadError,
+    IncompleteResultsError,
+    NoDatasetFound,
+    TargetFileAbsent,
 )
+
+# derive from TargetFileAbsent as the closest equivalent in datalad-core
+class UrlTargetNotFound(TargetFileAbsent):
+    """A connection request succeeded in principle, but target was not found
+
+    Equivalent of an HTTP404 response.
+    """
+    pass
diff --git a/datalad_next/url_operations/__init__.py b/datalad_next/url_operations/__init__.py
index 1a4f8ba..7b15a3a 100644
--- a/datalad_next/url_operations/__init__.py
+++ b/datalad_next/url_operations/__init__.py
@@ -64,6 +64,11 @@ class UrlOperations:
           status code) as its `status` property.  Any underlying exception must
           be linked via the `__cause__` property (e.g. `raise
           AccessFailedError(...) from ...`).
+        UrlTargetNotFound
+          Implementations that can distinguish a general "connection error"
+          from an absent download target raise `AccessFailedError` for
+          connection errors, and `UrlTargetNotFound` for download targets
+          found absent after a conenction was established successfully.
         """
         raise NotImplementedError
 
@@ -111,6 +116,12 @@ class UrlOperations:
           a status code (e.g. HTTP status code) as its `status` property.
           Any underlying exception must be linked via the `__cause__`
           property (e.g. `raise DownloadError(...) from ...`).
+        AccessFailedError
+        UrlTargetNotFound
+          Implementations that can distinguish a general "connection error"
+          from an absent download target raise `AccessFailedError` for
+          connection errors, and `UrlTargetNotFound` for download targets
+          found absent after a conenction was established successfully.
         """
         raise NotImplementedError
 
diff --git a/datalad_next/url_operations/file.py b/datalad_next/url_operations/file.py
index e174d5e..39ebe9c 100644
--- a/datalad_next/url_operations/file.py
+++ b/datalad_next/url_operations/file.py
@@ -7,7 +7,7 @@ import logging
 from pathlib import Path
 try:
     from shutil import COPY_BUFSIZE
-except ImportError:
+except ImportError:  # pragma: no cover
     # too old
     from datalad_next.utils import on_windows
     # from PY3.10
@@ -19,7 +19,7 @@ from urllib import (
     parse,
 )
 
-from datalad_next.exceptions import DownloadError
+from datalad_next.exceptions import UrlTargetNotFound
 
 from . import UrlOperations
 
@@ -43,6 +43,16 @@ class FileUrlOperations(UrlOperations):
         return Path(path)
 
     def sniff(self, url: str, *, credential: str = None) -> Dict:
+        """Gather information on a URL target, without downloading it
+
+        See :meth:`datalad_next.url_operations.UrlOperations.sniff`
+        for parameter documentation.
+
+        Raises
+        ------
+        UrlTargetNotFound
+          Raises `UrlTargetNotFound` for download targets found absent.
+        """
         # filter out internals
         return {
             k: v for k, v in self._sniff(url, credential).items()
@@ -54,7 +64,10 @@ class FileUrlOperations(UrlOperations):
         from_path = self._file_url_to_path(url)
         # if anything went wrong with the conversion, or we lack
         # permissions: die here
-        size = from_path.stat().st_size
+        try:
+            size = from_path.stat().st_size
+        except FileNotFoundError as e:
+            raise UrlTargetNotFound(url) from e
         return {
             'content-length': size,
             '_path': from_path,
@@ -73,6 +86,11 @@ class FileUrlOperations(UrlOperations):
 
         See :meth:`datalad_next.url_operations.UrlOperations.download`
         for parameter documentation.
+
+        Raises
+        ------
+        UrlTargetNotFound
+          Raises `UrlTargetNotFound` for download targets found absent.
         """
         # this is pretty much shutil.copyfileobj() with the necessary
         # wrapping to perform hashing and progress reporting
@@ -107,7 +125,7 @@ class FileUrlOperations(UrlOperations):
         except Exception as e:
             # wrap this into the datalad-standard, but keep the
             # original exception linked
-            raise DownloadError(msg=str(e)) from e
+            raise UrlTargetNotFound(msg=str(e)) from e
         finally:
             if dst_fp and to_path is not None:
                 dst_fp.close()
diff --git a/datalad_next/url_operations/http.py b/datalad_next/url_operations/http.py
index a55c53b..2d73961 100644
--- a/datalad_next/url_operations/http.py
+++ b/datalad_next/url_operations/http.py
@@ -13,7 +13,11 @@ from requests_toolbelt.downloadutils.tee import tee as requests_tee
 import www_authenticate
 
 import datalad
-from datalad_next.exceptions import DownloadError
+from datalad_next.exceptions import (
+    AccessFailedError,
+    UrlTargetNotFound,
+    DownloadError,
+)
 
 from datalad_next.requests_auth import DataladAuth
 from . import UrlOperations
@@ -45,6 +49,19 @@ class HttpUrlOperations(UrlOperations):
         return hdrs
 
     def sniff(self, url: str, *, credential: str = None) -> Dict:
+        """Gather information on a URL target, without downloading it
+
+        See :meth:`datalad_next.url_operations.UrlOperations.sniff`
+        for parameter documentation.
+
+        Raises
+        ------
+        AccessFailedError
+        UrlTargetNotFound
+          Raises `AccessFailedError` for connection errors, and
+          `UrlTargetNotFound` for download targets found absent after a
+          connection was established successfully.
+        """
         auth = DataladAuth(self.cfg, credential=credential)
         with requests.head(
                 url,
@@ -61,8 +78,13 @@ class HttpUrlOperations(UrlOperations):
             except requests.exceptions.RequestException as e:
                 # wrap this into the datalad-standard, but keep the
                 # original exception linked
-                raise AccessFailedError(
-                    msg=str(e), status=e.response.status_code) from e
+                if e.response.status_code == 404:
+                    # special case reporting for a 404
+                    raise UrlTargetNotFound(
+                        url, status=e.response.status_code) from e
+                else:
+                    raise AccessFailedError(
+                        msg=str(e), status=e.response.status_code) from e
             props = {
                 # standardize on lower-case header keys.
                 # also prefix anything other than 'content-length' to make
@@ -75,6 +97,13 @@ class HttpUrlOperations(UrlOperations):
         auth.save_entered_credential(
             context=f'sniffing {url}'
         )
+        if 'content-length' in props:
+            # make an effort to return size in bytes as int
+            try:
+                props['content-length'] = int(props['content-length'])
+            except (TypeError, ValueError):
+                # but be resonably robust against unexpected responses
+                pass
         return props
 
     def download(self,
@@ -87,6 +116,14 @@ class HttpUrlOperations(UrlOperations):
 
         See :meth:`datalad_next.url_operations.UrlOperations.download`
         for parameter documentation.
+
+        Raises
+        ------
+        AccessFailedError
+        UrlTargetNotFound
+          Raises `AccessFailedError` for connection errors, and
+          `UrlTargetNotFound` for download targets found absent after a
+          connection was established successfully.
         """
         # a new manager per request
         # TODO optimize later to cache credentials per target
@@ -104,8 +141,13 @@ class HttpUrlOperations(UrlOperations):
             except requests.exceptions.RequestException as e:
                 # wrap this into the datalad-standard, but keep the
                 # original exception linked
-                raise DownloadError(
-                    msg=str(e), status=e.response.status_code) from e
+                if e.response.status_code == 404:
+                    # special case reporting for a 404
+                    raise UrlTargetNotFound(
+                        from_url, status=e.response.status_code) from e
+                else:
+                    raise AccessFailedError(
+                        msg=str(e), status=e.response.status_code) from e
 
             download_props = self._stream_download_from_request(
                 r, to_path, hash=hash)
diff --git a/datalad_next/url_operations/ssh.py b/datalad_next/url_operations/ssh.py
index c6ae6f0..9b63b99 100644
--- a/datalad_next/url_operations/ssh.py
+++ b/datalad_next/url_operations/ssh.py
@@ -16,7 +16,12 @@ from urllib.parse import urlparse
 from datalad.runner import StdOutCapture
 from datalad.runner.protocol import GeneratorMixIn
 from datalad.runner.nonasyncrunner import ThreadedRunner
-from datalad_next.exceptions import DownloadError
+from datalad_next.exceptions import (
+    AccessFailedError,
+    CommandError,
+    DownloadError,
+    UrlTargetNotFound,
+)
 
 from . import UrlOperations
 
@@ -40,17 +45,42 @@ class SshUrlOperations(UrlOperations):
        likely to be removed in the future, and connection multiplexing
        supported where possible (non-Windows platforms).
     """
-    _stat_cmd = "printf \"\1\2\3\"; ls -nl '{fpath}' | awk 'BEGIN {{ORS=\"\1\"}} {{print $5}}'"
+    # first try ls'ing the path, and catch a missing path with a dedicated 244
+    # exit code, to be able to distinguish the original exit=2 that ls-call
+    # from a later exit=2 from awk in case of a "fatal error".
+    # when executed through ssh, only a missing file would yield 244, while
+    # a conenction error or other problem unrelated to the present of a file
+    # would a different error code (255 in case of a connection error)
+    _stat_cmd = "printf \"\1\2\3\"; ls '{fpath}' &> /dev/null " \
+                "&& ls -nl '{fpath}' | awk 'BEGIN {{ORS=\"\1\"}} {{print $5}}' " \
+                "|| exit 244"
     _cat_cmd = "cat '{fpath}'"
 
     def sniff(self, url: str, *, credential: str = None) -> Dict:
+        """Gather information on a URL target, without downloading it
+
+        See :meth:`datalad_next.url_operations.UrlOperations.sniff`
+        for parameter documentation.
+
+        Raises
+        ------
+        AccessFailedError
+        UrlTargetNotFound
+          Raises `AccessFailedError` for connection errors, and
+          `UrlTargetNotFound` for download targets found absent after a
+          connection was established successfully.
+        """
         try:
             props = self._sniff(
                 url,
                 cmd=SshUrlOperations._stat_cmd,
             )
-        except Exception as e:
-            raise AccessFailedError(str(e)) from e
+        except CommandError as e:
+            if e.code == 244:
+                # this is the special code for a file-not-found
+                raise UrlTargetNotFound(url) from e
+            else:
+                raise AccessFailedError(str(e)) from e
 
         return {k: v for k, v in props.items() if not k.startswith('_')}
 
@@ -73,7 +103,8 @@ class SshUrlOperations(UrlOperations):
                 incoming_magic = chunk[:len(need_magic)]
                 # does the incoming data have the remaining magic bytes?
                 if incoming_magic != expected_magic:
-                    raise ValueError("magic missing")
+                    raise RuntimeError(
+                        "Protocol error: report header not received")
                 # reduce (still missing) magic, if any
                 need_magic = need_magic[len(expected_magic):]
                 # strip magic from input
@@ -113,6 +144,14 @@ class SshUrlOperations(UrlOperations):
 
         See :meth:`datalad_next.url_operations.UrlOperations.download`
         for parameter documentation.
+
+        Raises
+        ------
+        AccessFailedError
+        UrlTargetNotFound
+          Raises `AccessFailedError` for connection errors, and
+          `UrlTargetNotFound` for download targets found absent after a
+          connection was established successfully.
         """
         # this is pretty much shutil.copyfileobj() with the necessary
         # wrapping to perform hashing and progress reporting
@@ -144,10 +183,14 @@ class SshUrlOperations(UrlOperations):
                 self._progress_report_update(progress_id, len(chunk))
             props.update(self._get_hash_report(hash, hasher))
             return props
-        except Exception as e:
-            # wrap this into the datalad-standard, but keep the
-            # original exception linked
-            raise DownloadError(msg=str(e)) from e
+        except CommandError as e:
+            if e.code == 244:
+                # this is the special code for a file-not-found
+                raise UrlTargetNotFound(from_url) from e
+            else:
+                # wrap this into the datalad-standard, but keep the
+                # original exception linked
+                raise AccessFailedError(msg=str(e)) from e
         finally:
             if dst_fp and to_path is not None:
                 dst_fp.close()


Test patch:
diff --git a/datalad_next/tests/utils.py b/datalad_next/tests/utils.py
index ccd15a1..1608a4d 100644
--- a/datalad_next/tests/utils.py
+++ b/datalad_next/tests/utils.py
@@ -29,6 +29,7 @@ from datalad.tests.utils_pytest import (
     rmtree,
     serve_path_via_http,
     skip_if_on_windows,
+    skip_ssh,
     skip_wo_symlink_capability,
     swallow_logs,
     with_tempfile,
diff --git a/datalad_next/url_operations/tests/__init__.py b/datalad_next/url_operations/tests/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/datalad_next/url_operations/tests/test_any.py b/datalad_next/url_operations/tests/test_any.py
new file mode 100644
index 0000000..bd07288
--- /dev/null
+++ b/datalad_next/url_operations/tests/test_any.py
@@ -0,0 +1,39 @@
+import pytest
+from datalad_next.exceptions import (
+    AccessFailedError,
+    UrlTargetNotFound,
+)
+from ..any import AnyUrlOperations
+
+
+def test_any_url_operations(tmp_path):
+    test_path = tmp_path / 'myfile'
+    test_url = test_path.as_uri()
+    ops = AnyUrlOperations()
+    # no target file (yet), precise exception
+    with pytest.raises(UrlTargetNotFound):
+        ops.sniff(test_url)
+    # now put something at the target location
+    test_path.write_text('surprise!')
+    # and now it works
+    props = ops.sniff(test_url)
+    # we get the correct file size reported
+    assert props['content-length'] == test_path.stat().st_size
+
+    # and download
+    download_path = tmp_path / 'download'
+    props = ops.download(test_url, download_path, hash=['sha256'])
+    assert props['sha256'] == '71de4622cf536ed4aa9b65fc3701f4fc5a198ace2fa0bda234fd71924267f696'
+    assert props['content-length'] == 9 == test_path.stat().st_size
+
+    # remove source and try again
+    test_path.unlink()
+    with pytest.raises(UrlTargetNotFound):
+        ops.download(test_url, download_path)
+
+    # try some obscure URL scheme
+    with pytest.raises(ValueError):
+        ops.sniff('weird://stuff')
+
+    # and it could have been figured out before
+    assert ops.is_supported_url('weird://stuff') == False
diff --git a/datalad_next/url_operations/tests/test_file.py b/datalad_next/url_operations/tests/test_file.py
new file mode 100644
index 0000000..aa7a392
--- /dev/null
+++ b/datalad_next/url_operations/tests/test_file.py
@@ -0,0 +1,32 @@
+import pytest
+from datalad_next.exceptions import (
+    AccessFailedError,
+    UrlTargetNotFound,
+)
+from ..file import FileUrlOperations
+
+
+def test_file_url_operations(tmp_path):
+    test_path = tmp_path / 'myfile'
+    test_url = test_path.as_uri()
+    ops = FileUrlOperations()
+    # no target file (yet), precise exception
+    with pytest.raises(UrlTargetNotFound):
+        ops.sniff(test_url)
+    # now put something at the target location
+    test_path.write_text('surprise!')
+    # and now it works
+    props = ops.sniff(test_url)
+    # we get the correct file size reported
+    assert props['content-length'] == test_path.stat().st_size
+
+    # and download
+    download_path = tmp_path / 'download'
+    props = ops.download(test_url, download_path, hash=['sha256'])
+    assert props['sha256'] == '71de4622cf536ed4aa9b65fc3701f4fc5a198ace2fa0bda234fd71924267f696'
+    assert props['content-length'] == 9 == test_path.stat().st_size
+
+    # remove source and try again
+    test_path.unlink()
+    with pytest.raises(UrlTargetNotFound):
+        ops.download(test_url, download_path)
diff --git a/datalad_next/url_operations/tests/test_http.py b/datalad_next/url_operations/tests/test_http.py
new file mode 100644
index 0000000..2f75c17
--- /dev/null
+++ b/datalad_next/url_operations/tests/test_http.py
@@ -0,0 +1,44 @@
+from pathlib import Path
+import pytest
+from datalad_next.exceptions import (
+    AccessFailedError,
+    UrlTargetNotFound,
+)
+from datalad_next.tests.utils import with_credential
+from ..http import HttpUrlOperations
+
+
+hbsurl = 'https://httpbin.org'
+hbscred = (
+    'hbscred',
+    dict(user='mike', secret='dummy', type='user_password',
+         realm=f'{hbsurl}/Fake Realm'),
+)
+
+@with_credential(hbscred[0], **hbscred[1])
+def test_http_url_operations(tmp_path):
+    ops = HttpUrlOperations()
+    # authentication after redirect
+    target_url = f'{hbsurl}/basic-auth/mike/dummy'
+    props = ops.sniff(f'{hbsurl}/redirect-to?url={target_url}')
+    # we get the resolved URL after redirect back
+    assert props['url'] == target_url
+    # same again, but credentials are wrong
+    target_url = f'{hbsurl}/basic-auth/mike/WRONG'
+    with pytest.raises(AccessFailedError):
+        ops.sniff(f'{hbsurl}/redirect-to?url={target_url}')
+    # make sure we get the size info
+    assert ops.sniff(f'{hbsurl}/bytes/63')['content-length'] == 63
+
+    # download
+    # SFRUUEJJTiBpcyBhd2Vzb21l == 'HTTPBIN is awesome'
+    props = ops.download(f'{hbsurl}/base64/SFRUUEJJTiBpcyBhd2Vzb21l',
+                         tmp_path / 'mydownload',
+                         hash=['md5'])
+    assert (tmp_path / 'mydownload').read_text() == 'HTTPBIN is awesome'
+
+    # 404s
+    with pytest.raises(UrlTargetNotFound):
+        ops.sniff(f'{hbsurl}/status/404')
+    with pytest.raises(UrlTargetNotFound):
+        ops.download(f'{hbsurl}/status/404', tmp_path / 'dontmatter')
diff --git a/datalad_next/url_operations/tests/test_ssh.py b/datalad_next/url_operations/tests/test_ssh.py
new file mode 100644
index 0000000..713c68e
--- /dev/null
+++ b/datalad_next/url_operations/tests/test_ssh.py
@@ -0,0 +1,55 @@
+import pytest
+from datalad_next.exceptions import (
+    AccessFailedError,
+    UrlTargetNotFound,
+)
+from datalad_next.tests.utils import (
+    skip_ssh,
+    skip_if_on_windows,
+)
+from ..ssh import SshUrlOperations
+
+
+# path magic inside the test is posix only
+@skip_if_on_windows
+# SshUrlOperations does not work against a windows server
+# and the test uses 'localhost' as target
+@skip_ssh
+def test_ssh_url_operations(tmp_path, monkeypatch):
+    test_path = tmp_path / 'myfile'
+    test_url = f'ssh://localhost{test_path}'
+    ops = SshUrlOperations()
+    # no target file (yet), precise exception
+    with pytest.raises(UrlTargetNotFound):
+        ops.sniff(test_url)
+    # this is different for a general connection error
+    with pytest.raises(AccessFailedError):
+        ops.sniff(f'ssh://localhostnotaround{test_path}')
+    # now put something at the target location
+    test_path.write_text('surprise!')
+    # and now it works
+    props = ops.sniff(test_url)
+    # we get the correct file size reported
+    assert props['content-length'] == test_path.stat().st_size
+
+    # simulate a "protocol error" where the server-side command
+    # is not reporting the magic header
+    with monkeypatch.context() as m:
+        m.setattr(SshUrlOperations, '_stat_cmd', 'echo nothing')
+        # we get a distinct exception
+        with pytest.raises(RuntimeError):
+            ops.sniff(test_url)
+
+    # and download
+    download_path = tmp_path / 'download'
+    props = ops.download(test_url, download_path, hash=['sha256'])
+    assert props['sha256'] == '71de4622cf536ed4aa9b65fc3701f4fc5a198ace2fa0bda234fd71924267f696'
+    assert props['content-length'] == 9 == test_path.stat().st_size
+
+    # remove source and try again
+    test_path.unlink()
+    with pytest.raises(UrlTargetNotFound):
+        ops.download(test_url, download_path)
+    # this is different for a general connection error
+    with pytest.raises(AccessFailedError):
+        ops.download(f'ssh://localhostnotaround{test_path}', download_path)