Eliminate assumption that asset keys are usable as filename #820

Author: soxofaan

docs/batch_jobs.rst

@@ -378,11 +378,11 @@ If you know that there is just a single result file, you can also download it di
 This will fail however if there are multiple assets in the job result
 (like in the metadata example above).
 In that case you can still download a single by specifying which one you
-want to download with the ``name`` argument:
+want to download with the ``key`` argument:
 
 .. code-block:: python
 
-    results.download_file("data/out/result.tiff", name="res002.tiff")
+    results.download_file("data/out/result.tiff", key="res002.tiff")
 
 
 Fine-grained asset downloads
@@ -396,9 +396,14 @@ with :py:meth:`~openeo.rest.job.ResultAsset.download`, like this:
 .. code-block:: python
 
     for asset in results.get_assets():
-        if asset.metadata["type"].startswith("image/tiff"):
-            asset.download("data/out/result-v2-" + asset.name)
+        if asset.media_type.startswith("image/tiff"):
+            asset.download("data/out/result-v2-" + asset.key)
 
+.. note::
+    The ``key`` of an asset is not guaranteed to be directly usable as filename safe,
+    so while the snippet above glosses over that aspect,
+    make sure to properly sanitize the filename or
+    at least check the behavior of the backend you are working with.
 
 Directly load batch job results
 ===============================

openeo/internal/warnings.py

@@ -27,6 +27,13 @@ class UserDeprecationWarning(Warning):
     pass
 
 
+def user_deprecation_warning(message: str, stacklevel: int = 3):
+    """
+    Helper to compactly raise a `UserDeprecationWarning` with proper stacklevel.
+    """
+    warnings.warn(message, category=UserDeprecationWarning, stacklevel=stacklevel
+    )
+
 def test_warnings(stacklevel=1):
     """Trigger some warnings (for test contexts)."""
     for warning in [UserWarning, DeprecationWarning, UserDeprecationWarning]:

openeo/rest/job.py

@@ -1,8 +1,11 @@
 from __future__ import annotations
 
+import warnings
+
 import datetime
 import json
 import logging
+import re
 import time
 import typing
 from pathlib import Path
@@ -12,7 +15,7 @@
 
 from openeo.internal.documentation import openeo_endpoint
 from openeo.internal.jupyter import VisualDict, render_component, render_error
-from openeo.internal.warnings import deprecated, legacy_alias
+from openeo.internal.warnings import deprecated, legacy_alias,  user_deprecation_warning
 from openeo.rest import (
     DEFAULT_DOWNLOAD_CHUNK_SIZE,
     DEFAULT_DOWNLOAD_RANGE_SIZE,
@@ -400,22 +403,41 @@ class ResultAsset:
     .. versionadded:: 0.4.10
     """
 
-    def __init__(self, job: BatchJob, name: str, href: str, metadata: dict):
-        self.job = job
+    __slots__ = ("job", "key", "href", "media_type", "metadata")
 
-        self.name = name
-        """Asset name as advertised by the backend."""
+    def __init__(self, job: BatchJob, key: str, href: str, metadata: dict):
+        # TODO: the owning job is actually only used to obtain a Connection for downloading. Eliminate BatchJob as requirement
+        self.job = job
+        self.key = key
 
+        # URL to the downloadable asset
         self.href = href
-        """Download URL of the asset."""
 
+        self.media_type = metadata.get("type")
+
+        # Asset metadata provided by the backend, possibly containing keys "type" (for media type), "roles", "title", "description".
         self.metadata = metadata
-        """Asset metadata provided by the backend, possibly containing keys "type" (for media type), "roles", "title", "description"."""
 
     def __repr__(self):
-        return "<ResultAsset {n!r} (type {t}) at {h!r}>".format(
-            n=self.name, t=self.metadata.get("type", "unknown"), h=self.href
-        )
+        return f"<ResultAsset {self.key!r} (type {self.media_type}) at {self.href!r}>"
+
+    @property
+    def name(self):
+        # TODO: remove this once users got enough time to migrate
+        user_deprecation_warning("`ResultAsset.name` is deprecated and will be removed, use `ResultAsset.key` instead")
+        return self.key
+
+    def _make_filename(self) -> str:
+        """
+        Produce a filename for downloading the asset to
+        as fallback when user did not provide something,
+        based on: asset key (which is not guaranteed to consist of filename-safe characters)
+        and filename in href (if any)
+        """
+        safe_filename_regex = re.compile(r"^[a-zA-Z0-9_.-]+\.[a-zA-Z0-9]+$")
+        if safe_filename_regex.fullmatch(self.key):
+            # Legacy mode: use asset key as filename
+            return self.key
 
     def download(
         self,
@@ -427,16 +449,18 @@ def download(
         """
         Download asset to given location
 
-        :param target: download target path. Can be an existing folder
-            (in which case the filename advertised by backend will be used)
-            or full file name. By default, the working directory will be used.
+        :param target: target path to download to.
+            Can be a path to file, or to an existing folder
+            (in which case the filename will be constructed
+            in best-effort fashion, based on available metadata)
+            By default, the working directory will be used.
         :param chunk_size: chunk size for streaming response.
         """
         target = Path(target or Path.cwd())
         if target.is_dir():
-            target = target / self.name
+            target = target / self._make_filename()
         ensure_dir(target.parent)
-        logger.info("Downloading Job result asset {n!r} from {h!s} to {t!s}".format(n=self.name, h=self.href, t=target))
+        logger.info(f"Downloading job result asset {self.key!r} from {self.href!s} to {target!s}")
         self._download_to_file(url=self.href, target=target, chunk_size=chunk_size, range_size=range_size)
         return target
 
@@ -445,7 +469,7 @@ def _get_response(self, stream=True) -> requests.Response:
 
     def load_json(self) -> dict:
         """Load asset in memory and parse as JSON."""
-        if not (self.name.lower().endswith(".json") or self.metadata.get("type") == "application/json"):
+        if self.metadata.get("type") != "application/json":
             logger.warning("Asset might not be JSON")
         return self._get_response().json()
 
@@ -559,50 +583,65 @@ def get_assets(self) -> List[ResultAsset]:
         if not assets:
             logger.warning("No assets found in job result metadata.")
         return [
-            ResultAsset(job=self._job, name=name, href=asset["href"], metadata=asset) for name, asset in assets.items()
+            ResultAsset(job=self._job, key=key, href=asset["href"], metadata=asset) for key, asset in assets.items()
         ]
 
-    def get_asset(self, name: str = None) -> ResultAsset:
+    def get_asset(self, key: Optional[str] = None, *, name: Optional[str] = None) -> ResultAsset:
         """
-        Get single asset by name or without name if there is only one.
+        Get single asset by asset key or without key if there is only one.
         """
         # TODO: also support getting a single asset by type or role?
+        if name:
+            # TODO: remove this legacy `name` support when users got enough time to migrate
+            user_deprecation_warning("Argument `name` is deprecated and will be removed, use `key` instead.")
+            key = name
+            del name
+
         assets = self.get_assets()
         if len(assets) == 0:
             raise OpenEoClientException("No assets in result.")
-        if name is None:
+        if key is None:
             if len(assets) == 1:
                 return assets[0]
             else:
                 raise MultipleAssetException(
-                    "Multiple result assets for job {j}: {a}".format(j=self._job.job_id, a=[a.name for a in assets])
+                    "Multiple result assets for job {j}: {a}".format(j=self._job.job_id, a=[a.key for a in assets])
                 )
         else:
             try:
-                return next(a for a in assets if a.name == name)
+                return next(a for a in assets if a.key == key)
             except StopIteration:
-                raise OpenEoClientException("No asset {n!r} in: {a}".format(n=name, a=[a.name for a in assets]))
+                raise OpenEoClientException("No asset {k!r} in: {a}".format(k=key, a=[a.key for a in assets]))
 
     def download_file(
         self,
-        target: Union[Path, str] = None,
-        name: str = None,
+        target: Union[Path, str, None] = None,
+        key: Optional[str] = None,
         *,
-        chunk_size=DEFAULT_DOWNLOAD_CHUNK_SIZE,
+        chunk_size: int = DEFAULT_DOWNLOAD_CHUNK_SIZE,
         range_size: int = DEFAULT_DOWNLOAD_RANGE_SIZE,
+        name: Optional[str] = None,
     ) -> Path:
         """
         Download single asset. Can be used when there is only one asset in the
-        :py:class:`JobResults`, or when the desired asset name is given explicitly.
-
-        :param target: path to download to. Can be an existing directory
-            (in which case the filename advertised by backend will be used)
-            or full file name. By default, the working directory will be used.
-        :param name: asset name to download (not required when there is only one asset)
+        :py:class:`JobResults`, or when the desired asset key is given explicitly.
+
+        :param target: target path to download to.
+            Can be a path to file, or to an existing folder
+            (in which case the filename will be constructed
+            in best-effort fashion, based on available metadata)
+            By default, the working directory will be used.
+        :param key: asset key to download (not required when there is only one asset)
         :return: path of downloaded asset
         """
+        if name:
+            # TODO: remove this legacy `name` support when users got enough time to migrate
+            user_deprecation_warning("Argument `name` is deprecated and will be removed, use `key` instead.")
+            key = name
+            del name
+
         try:
-            return self.get_asset(name=name).download(target=target, chunk_size=chunk_size, range_size=range_size)
+            return self.get_asset(key=key).download(target=target, chunk_size=chunk_size, range_size=range_size)
         except MultipleAssetException:
             raise OpenEoClientException(
                 "Can not use `download_file` with multiple assets. Use `download_files` instead."

tests/rest/test_job.py

@@ -827,7 +827,7 @@ def test_get_results_multiple_download_single(job_with_2_assets: BatchJob, tmp_p
 def test_get_results_multiple_download_single_by_name(job_with_2_assets: BatchJob, tmp_path):
     job = job_with_2_assets
     target = tmp_path / "res.tiff"
-    path = job.get_results().download_file(target, name="1.tiff")
+    path = job.get_results().download_file(target, key="1.tiff")
     assert path == target
     assert list(p.name for p in tmp_path.iterdir()) == ["res.tiff"]
     with path.open("rb") as f:
@@ -839,7 +839,7 @@ def test_get_results_multiple_download_single_by_wrong_name(job_with_2_assets: B
     target = tmp_path / "res.tiff"
     expected = r"No asset 'foobar\.tiff' in: \['.\.tiff', '.\.tiff'\]"
     with pytest.raises(OpenEoClientException, match=expected):
-        job.get_results().download_file(target, name="foobar.tiff")
+        job.get_results().download_file(target, key="foobar.tiff")
 
 
 def test_download_results(job_with_2_assets: BatchJob, tmp_path):
@@ -875,7 +875,7 @@ def test_get_results_download_files(job_with_2_assets: BatchJob, tmp_path):
     results = job.get_results()
 
     assets = job.get_results().get_assets()
-    assert {a.name: a.metadata for a in assets} == {
+    assert {a.key: a.metadata for a in assets} == {
         "1.tiff": {"href": "https://oeo.test/dl/jjr1.tiff", "type": "image/tiff; application=geotiff"},
         "2.tiff": {"href": "https://oeo.test/dl/jjr2.tiff", "type": "image/tiff; application=geotiff"},
     }
@@ -932,7 +932,7 @@ def test_result_asset_download_file(con100, requests_mock, tmp_path):
     requests_mock.get(href, content=TIFF_CONTENT)
 
     job = BatchJob("jj", connection=con100)
-    asset = ResultAsset(job, name="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
+    asset = ResultAsset(job, key="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
     target = tmp_path / "res.tiff"
     path = asset.download(target)
 
@@ -948,7 +948,7 @@ def test_result_asset_download_file_error(con100, requests_mock, tmp_path):
     requests_mock.get(href, status_code=500, text="Nope!")
 
     job = BatchJob("jj", connection=con100)
-    asset = ResultAsset(job, name="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
+    asset = ResultAsset(job, key="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
 
     with pytest.raises(OpenEoApiPlainError, match="Nope!"):
         _ = asset.download(tmp_path / "res.tiff")
@@ -963,7 +963,7 @@ def test_result_asset_download_folder(con100, requests_mock, tmp_path):
     requests_mock.get(href, content=TIFF_CONTENT)
 
     job = BatchJob("jj", connection=con100)
-    asset = ResultAsset(job, name="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
+    asset = ResultAsset(job, key="1.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
     target = tmp_path / "folder"
     target.mkdir()
     path = asset.download(target)
@@ -979,7 +979,7 @@ def test_result_asset_load_json(con100, requests_mock):
     requests_mock.get(href, json={"bands": [1, 2, 3]})
 
     job = BatchJob("jj", connection=con100)
-    asset = ResultAsset(job, name="out.json", href=href, metadata={"type": "application/json"})
+    asset = ResultAsset(job, key="out.json", href=href, metadata={"type": "application/json"})
     res = asset.load_json()
 
     assert res == {"bands": [1, 2, 3]}
@@ -991,7 +991,7 @@ def test_result_asset_load_bytes(con100, requests_mock):
     requests_mock.get(href, content=TIFF_CONTENT)
 
     job = BatchJob("jj", connection=con100)
-    asset = ResultAsset(job, name="out.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
+    asset = ResultAsset(job, key="out.tiff", href=href, metadata={"type": "image/tiff; application=geotiff"})
     res = asset.load_bytes()
 
     assert res == TIFF_CONTENT