Eliminate assumption that asset keys are usable as filename #820

Author: soxofaan

CHANGELOG.md

@@ -8,7 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
+
 - `DataCube.resample_spatial()` now supports parameterized `resolution` and `projection` arguments.  ([#897](https://github.com/Open-EO/openeo-python-client/issues/897))
+- Sanitize asset download filenames, instead of blindly using the asset key as filename. ([#820](https://github.com/Open-EO/openeo-python-client/issues/820))
 
 ### Changed
 

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

@@ -3,16 +3,18 @@
 import datetime
 import json
 import logging
+import re
 import time
 import typing
 from pathlib import Path
 from typing import Dict, List, Optional, Union
+from urllib.parse import urlparse
 
 import requests
 
 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,
@@ -393,29 +395,90 @@ class RESTJob(BatchJob):
     """
 
 
+FILENAME_UNSAFE_REGEX = re.compile(r"[^a-zA-Z0-9_.-]+")
+
+
+def _sanitize_filename(s: str, replacement: str = "") -> str:
+    """
+    Sanitize a filename (strip/replace risky characters)
+    so that it can be safely used as a filename.
+    """
+    s = str(s).strip()
+    return FILENAME_UNSAFE_REGEX.sub(replacement, s)
+
+
+_MEDIA_TYPE_EXTENSION_MAP = {
+    "image/tiff": ".tiff",
+    "image/tiff; application=geotiff": ".tiff",
+    "image/tiff; application=geotiff; profile=cloud-optimized": ".tiff",
+    "application/x-netcdf": ".nc",
+    "image/png": ".png",
+    "application/json": ".json",
+    "application/geo+json": ".json",
+    "text/csv": ".csv",
+    "x-gis/x-shapefile": ".shp",
+    "application/geopackage+sqlite3": ".gpkg",
+    "application/parquet; profile=geo": ".parquet",
+    "application/zip+zarr": ".zarr",
+    "application/zip": ".zip",
+}
+
+
 class ResultAsset:
     """
     Result asset of a batch job (e.g. a GeoTIFF or JSON file)
 
     .. 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 "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} (media 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)
+        """
+
+        if re.fullmatch(r"^[a-zA-Z0-9_.-]+\.[a-zA-Z0-9]{1,10}$", self.key):
+            # Legacy mode: asset key already looks like a filename
+            return self.key
+
+        # Build filename from key, href's path (if any)
+        # and guess extension from media type if necessary
+        sanitized_key = _sanitize_filename(self.key)
+        href_path = urlparse(str(self.href)).path
+        href_basename = _sanitize_filename(Path(href_path).name)
+        filename = f"{sanitized_key}-{href_basename}"
+
+        if not re.fullmatch(r".*\.[a-zA-Z0-9]{1,10}$", filename):
+            # Extension seems missing, do media type based guess (best effort)
+            if extension := _MEDIA_TYPE_EXTENSION_MAP.get(self.media_type):
+                filename += extension
+        return filename
 
     def download(
         self,
@@ -427,16 +490,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 +510,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.media_type not in {"application/json", "application/geo+json"}:
             logger.warning("Asset might not be JSON")
         return self._get_response().json()
 
@@ -559,50 +624,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."