import-url: support importing empty directories on object storage services (#5536)

* import-url: support importing empty directories on object storage services

Empty directories are basically files with a trailing slash at the end of their names,
with empty contents on object service providers (such as azure, google storage). This
patch implements the `is_empty()` method for the `FSSpecWrapper` to determining whether
a path is empty (directory or a file) and also tweaks `.isdir()` in order to see empty
directories.

The base class is only used by azure and gcsfs for now, but if we go on the route of
migrating normal providers (like proper filesystems, not just object storages), we might
need to do these tweaks on a subclass (like `ObjectStorageFSSpecWrapper` etc), but I
don't think it is neccesary as of now.

The `is_empty` method is already there, and also used in the importing part to
warn users that they are importing something empty (https://github.com/iterative/dvc/blob/e385e59a098cd661ea1ea4257c9a5735c5c32bc6/dvc/output/base.py#L252-L254)
so this is not a new method. Just for the reference, as far as I can locate there
is no variant of this method in the fsspec.

* tests: revert custom mkdir for cloud storages

* tests: allign assingments

* add some comments

dvc/fs/base.py

@@ -283,6 +283,9 @@ class BaseFileSystem:
         self, from_info, to_info, name, no_progress_bar, jobs, **kwargs,
     ):
         from_infos = list(self.walk_files(from_info, **kwargs))
+        if not from_infos:
+            makedirs(to_info, exist_ok=True)
+            return None
         to_infos = (
             to_info / info.relative_to(from_info) for info in from_infos
         )

dvc/fs/fsspec_wrapper.py

@@ -1,5 +1,6 @@
 import os
 import shutil
+from functools import lru_cache
 
 from dvc.progress import Tqdm
 
@@ -11,6 +12,7 @@ class FSSpecWrapper(BaseFileSystem):
     def fs(self):
         raise NotImplementedError
 
+    @lru_cache(512)
     def _with_bucket(self, path):
         if isinstance(path, self.PATH_CLS):
             return f"{path.bucket}/{path.path}"
@@ -37,10 +39,32 @@ class FSSpecWrapper(BaseFileSystem):
         return entry
 
     def isdir(self, path_info):
-        return self.fs.isdir(self._with_bucket(path_info))
+        if not self.exists(path_info):
+            return False
+
+        # Directory in object storages are interpreted differently
+        # among different fsspec providers, so this logic is a temporary
+        # measure for us to adapt as of now. It checks whether it is a
+        # directory (as in a prefix with contents) or whether it is an empty
+        # file where it's name ends with a forward slash
+        entry = self.info(path_info)
+        return entry["type"] == "directory" or (
+            entry["size"] == 0
+            and entry["type"] == "file"
+            and entry["name"].endswith("/")
+        )
 
     def isfile(self, path_info):
-        return self.fs.isfile(self._with_bucket(path_info))
+        if not self.exists(path_info):
+            return False
+
+        return not self.isdir(path_info)
+
+    def is_empty(self, path_info):
+        entry = self.info(path_info)
+        if entry["type"] == "directory":
+            return not self.fs.ls(self._with_bucket(path_info))
+        return entry["size"] == 0
 
     def open(
         self, path_info, mode="r", **kwargs
@@ -56,6 +80,9 @@ class FSSpecWrapper(BaseFileSystem):
     def ls(
         self, path_info, detail=False, recursive=False
     ):  # pylint: disable=arguments-differ
+        if self.isdir(path_info) and self.is_empty(path_info):
+            return None
+
         path = self._with_bucket(path_info)
         if recursive:
             for root, _, files in self.fs.walk(path, detail=detail):

tests/func/test_import_url.py

@@ -353,3 +353,42 @@ def test_import_url_to_remote_absolute(
 def test_import_url_to_remote_invalid_combinations(dvc):
     with pytest.raises(InvalidArgumentError, match="--no-exec"):
         dvc.imp_url("s3://bucket/foo", no_exec=True, to_remote=True)
+
+
+empty_xfail = pytest.mark.xfail(
+    reason="https://github.com/iterative/dvc/issues/5521"
+)
+
+
+@pytest.mark.parametrize(
+    "workspace",
+    [
+        pytest.lazy_fixture("s3"),
+        pytest.lazy_fixture("hdfs"),
+        pytest.param(pytest.lazy_fixture("gs"), marks=empty_xfail),
+        pytest.param(pytest.lazy_fixture("azure"), marks=empty_xfail),
+        pytest.param(
+            pytest.lazy_fixture("ssh"),
+            marks=pytest.mark.skipif(
+                os.name == "nt", reason="disabled on windows"
+            ),
+        ),
+    ],
+    indirect=True,
+)
+def test_import_url_empty_directory(tmp_dir, dvc, workspace):
+    # prefix based storage services (e.g s3) doesn't have the real concept
+    # of directories. So instead we create an empty file that ends with a
+    # trailing slash in order to actually support this operation
+    if workspace.IS_OBJECT_STORAGE:
+        contents = ""
+    else:
+        contents = {}
+
+    workspace.gen({"empty_dir/": contents})
+
+    dvc.imp_url("remote://workspace/empty_dir/")
+
+    empty_dir = tmp_dir / "empty_dir"
+    assert empty_dir.is_dir()
+    assert tuple(empty_dir.iterdir()) == ()

tests/remotes/azure.py

@@ -22,6 +22,7 @@ TEST_AZURE_CONNECTION_STRING = (
 
 class Azure(Base, CloudURLInfo):
 
+    IS_OBJECT_STORAGE = True
     CONNECTION_STRING = None
 
     @cached_property

tests/remotes/base.py

@@ -7,6 +7,9 @@ from dvc.path_info import URLInfo
 
 
 class Base(URLInfo):
+
+    IS_OBJECT_STORAGE = False
+
     def is_file(self):
         raise NotImplementedError
 

tests/remotes/gs.py

@@ -23,6 +23,9 @@ os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = TEST_GCP_CREDS_FILE
 
 
 class GCP(Base, CloudURLInfo):
+
+    IS_OBJECT_STORAGE = True
+
     @staticmethod
     def should_test():
         do_test = env2bool("DVC_TEST_GCP", undefined=None)

tests/remotes/oss.py

@@ -16,6 +16,9 @@ EMULATOR_OSS_ACCESS_KEY_SECRET = "AccessKeySecret"
 
 
 class OSS(Base, CloudURLInfo):
+
+    IS_OBJECT_STORAGE = True
+
     @staticmethod
     def get_url():
         return f"oss://{TEST_OSS_REPO_BUCKET}/{uuid.uuid4()}"

tests/remotes/s3.py

@@ -15,6 +15,9 @@ TEST_AWS_REPO_BUCKET = os.environ.get("DVC_TEST_AWS_REPO_BUCKET", "dvc-temp")
 
 
 class S3(Base, CloudURLInfo):
+
+    IS_OBJECT_STORAGE = True
+
     @staticmethod
     def should_test():
         do_test = env2bool("DVC_TEST_AWS", undefined=None)