outdir can be any valid URI ; add test for this (#1552)

* Fix to work with URI outdir, don't double-quote.
* Test that external outdir URI is handled correctly

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <[email protected]>

* exempt stdfsaccess from compilation instead of refactoring it
* sort inputs
* remove unused imports
* Add module level docstring

Co-authored-by: Michael R. Crusoe <[email protected]>

Author: tetron

cwltool/command_line_tool.py

@@ -200,11 +200,17 @@ def revmap_file(
     outside the container. Recognizes files in the pathmapper or remaps
     internal output directories to the external directory.
     """
-    outdir_uri, outdir_path = file_uri(str(outdir)), outdir
 
     # builder.outdir is the inner (container/compute node) output directory
     # outdir is the outer (host/storage system) output directory
 
+    if outdir.startswith("/"):
+        # local file path, turn it into a file:// URI
+        outdir = file_uri(outdir)
+
+    # note: outer outdir should already be a URI and should not be URI
+    # quoted any further.
+
     if "location" in f and "path" not in f:
         location = cast(str, f["location"])
         if location.startswith("file://"):
@@ -234,9 +240,9 @@ def revmap_file(
         ):
             f["location"] = revmap_f[1]
         elif (
-            uripath == outdir_uri
-            or uripath.startswith(outdir_uri + os.sep)
-            or uripath.startswith(outdir_uri + "/")
+            uripath == outdir
+            or uripath.startswith(outdir + os.sep)
+            or uripath.startswith(outdir + "/")
         ):
             f["location"] = uripath
         elif (
@@ -245,9 +251,9 @@ def revmap_file(
             or path.startswith(builder.outdir + "/")
         ):
             joined_path = builder.fs_access.join(
-                outdir_path, path[len(builder.outdir) + 1 :]
+                outdir, urllib.parse.quote(path[len(builder.outdir) + 1 :])
             )
-            f["location"] = file_uri(joined_path)
+            f["location"] = joined_path
         else:
             raise WorkflowException(
                 "Output file path %s must be within designated output directory (%s) or an input "

cwltool/errors.py

@@ -8,5 +8,3 @@ class UnsupportedRequirement(WorkflowException):
 
 class ArgumentException(Exception):
     """Mismatched command line arguments provided."""
-
-    pass

cwltool/executors.py

@@ -33,7 +33,7 @@
 from .provenance_profile import ProvenanceProfile
 from .task_queue import TaskQueue
 from .update import ORIGINAL_CWLVERSION
-from .utils import CWLObjectType, CWLOutputType, JobsType
+from .utils import CWLObjectType, JobsType
 from .workflow import Workflow
 from .workflow_job import WorkflowJob, WorkflowJobStep
 

cwltool/job.py

@@ -13,7 +13,6 @@
 import time
 import uuid
 from abc import ABCMeta, abstractmethod
-from io import IOBase
 from threading import Timer
 from typing import (
     IO,
@@ -451,15 +450,13 @@ def _required_env(self) -> Dict[str, str]:
         Note that with containers, the paths will (likely) be those from
         inside.
         """
-        pass
 
     def _preserve_environment_on_containers_warning(
         self, varname: Optional[Iterable[str]] = None
     ) -> None:
         """When running in a container, issue a warning."""
         # By default, don't do anything; ContainerCommandLineJob below
         # will issue a warning.
-        pass
 
     def prepare_environment(
         self, runtimeContext: RuntimeContext, envVarReq: Mapping[str, str]

cwltool/load_tool.py

@@ -1,12 +1,12 @@
 """Loads a CWL document."""
 
+import copy
 import hashlib
 import logging
 import os
 import re
 import urllib
 import uuid
-import copy
 from typing import (
     Any,
     Dict,

cwltool/provenance_profile.py

@@ -47,7 +47,6 @@
 from .stdfsaccess import StdFsAccess
 from .utils import (
     CWLObjectType,
-    CWLOutputType,
     JobsType,
     get_listing,
     posix_path,

setup.py

@@ -73,7 +73,7 @@
         "cwltool/secrets.py",
         "cwltool/singularity.py",
         "cwltool/software_requirements.py",
-        "cwltool/stdfsaccess.py",
+        # "cwltool/stdfsaccess.py",  # StdFsAccess needs to be subclassable
         "cwltool/subgraph.py",
         "cwltool/update.py",
         "cwltool/utils.py",

tests/test_environment.py

@@ -63,16 +63,13 @@ class CheckHolder(ABC):
     @abstractmethod
     def checks(tmp_prefix: str) -> EnvChecks:
         """Return a mapping from environment variable names to how to check for correctness."""
-        pass
 
     # Any flags to pass to cwltool to force use of the correct container
     flags: List[str]
 
     # Does the env tool (maybe in our container) accept a `-0` flag?
     env_accepts_null: bool
 
-    pass
-
 
 class NoContainer(CheckHolder):
     """No containers at all, just run in the host."""

tests/test_load_tool.py

@@ -1,15 +1,16 @@
+"""Tests for cwltool.load_tool."""
+import logging
 from pathlib import Path
 
 import pytest
-import logging
 
 from cwltool.context import LoadingContext, RuntimeContext
 from cwltool.errors import WorkflowException
 from cwltool.load_tool import load_tool
+from cwltool.loghandler import _logger, configure_logging
 from cwltool.process import use_custom_schema, use_standard_schema
 from cwltool.update import INTERNAL_VERSION
 from cwltool.utils import CWLObjectType
-from cwltool.loghandler import _logger, configure_logging
 
 from .util import get_data
 

tests/test_path_checks.py

@@ -1,6 +1,7 @@
 import urllib.parse
+from io import BytesIO
 from pathlib import Path
-from typing import cast
+from typing import IO, Any, List, cast
 
 import pytest
 from ruamel.yaml.comments import CommentedMap
@@ -9,6 +10,7 @@
 from cwltool.command_line_tool import CommandLineTool
 from cwltool.context import LoadingContext, RuntimeContext
 from cwltool.main import main
+from cwltool.stdfsaccess import StdFsAccess
 from cwltool.update import INTERNAL_VERSION
 from cwltool.utils import CWLObjectType
 
@@ -105,6 +107,26 @@ def test_unicode_in_output_files(tmp_path: Path, filename: str) -> None:
     assert main(params) == 0
 
 
+class TestFsAccess(StdFsAccess):
+    """Stub fs access object that doesn't rely on the filesystem."""
+
+    def glob(self, pattern: str) -> List[str]:
+        """glob."""
+        return [pattern]
+
+    def open(self, fn: str, mode: str) -> IO[Any]:
+        """open."""
+        return BytesIO(b"aoeu")
+
+    def isfile(self, fn: str) -> bool:
+        """isfile."""
+        return True
+
+    def size(self, fn: str) -> int:
+        """size."""
+        return 4
+
+
 def test_clt_returns_specialchar_names(tmp_path: Path) -> None:
     """Confirm that special characters in filenames do not cause problems."""
     loading_context = LoadingContext(
@@ -167,3 +189,30 @@ def test_clt_returns_specialchar_names(tmp_path: Path) -> None:
     assert result["basename"] == special
     assert result["nameroot"] == special
     assert str(result["location"]).endswith(urllib.parse.quote(special))
+
+    # Now test when outdir is a URI, make sure it doesn't get
+    # incorrectly quoted as a file.
+    builder = clt._init_job({}, RuntimeContext())
+    builder.pathmapper = clt.make_path_mapper(
+        builder.files, builder.stagedir, RuntimeContext(), True
+    )
+    builder.outdir = "/var/spool/cwl"
+    fs_access = TestFsAccess("")
+
+    result = cast(
+        CWLObjectType,
+        clt.collect_output(
+            output_schema,
+            builder,
+            "keep:ae755cd1b3cff63152ff4200f4dea7e9+52",
+            fs_access,
+        ),
+    )
+
+    assert result["class"] == "File"
+    assert result["basename"] == special
+    assert result["nameroot"] == special
+    assert (
+        result["location"]
+        == "keep:ae755cd1b3cff63152ff4200f4dea7e9+52/%3A%3F%23%5B%5D%40%21%24%26%27%28%29%2A%2B%2C%3B%3D"
+    )