s3,gs: don't append "/" to prefixed path when listing objects (#4149)

* s3,gs: don't append "/" to prefixed path when listing objects

* remote: use optional prefix kwarg for walk_files

* if prefix is True, path will be walked as a prefix
* if prefix is False, path will be treated as a directory (trailing
  slash will be appended for S3-like remotes)

* tests: add unit test for BaseRemoteTree.list_paths

dvc/remote/azure.py

@@ -121,6 +121,8 @@ class AzureRemoteTree(BaseRemoteTree):
             next_marker = blobs.next_marker
 
     def walk_files(self, path_info, **kwargs):
+        if not kwargs.pop("prefix", False):
+            path_info = path_info / ""
         for fname in self._list_paths(
             path_info.bucket, path_info.path, **kwargs
         ):

dvc/remote/base.py

@@ -213,7 +213,12 @@ class BaseRemoteTree:
         return False  # We can't be sure by default
 
     def walk_files(self, path_info, **kwargs):
-        """Return a generator with `PathInfo`s to all the files"""
+        """Return a generator with `PathInfo`s to all the files.
+
+        Optional kwargs:
+            prefix (bool): If true `path_info` will be treated as a prefix
+                rather than directory path.
+        """
         raise NotImplementedError
 
     def is_empty(self, path_info):
@@ -522,14 +527,16 @@ class BaseRemoteTree:
                 path_info = self.path_info / prefix[:2] / prefix[2:]
             else:
                 path_info = self.path_info / prefix[:2]
+            prefix = True
         else:
             path_info = self.path_info
+            prefix = False
         if progress_callback:
-            for file_info in self.walk_files(path_info):
+            for file_info in self.walk_files(path_info, prefix=prefix):
                 progress_callback()
                 yield file_info.path
         else:
-            yield from self.walk_files(path_info)
+            yield from self.walk_files(path_info, prefix=prefix)
 
     def list_hashes(self, prefix=None, progress_callback=None):
         """Iterate over hashes in this tree.

dvc/remote/gdrive.py

@@ -553,7 +553,7 @@ class GDriveRemoteTree(BaseRemoteTree):
             )
 
     def walk_files(self, path_info, **kwargs):
-        if path_info == self.path_info:
+        if path_info == self.path_info or not kwargs.pop("prefix", False):
             prefix = None
         else:
             prefix = path_info.path

dvc/remote/gs.py

@@ -144,7 +144,9 @@ class GSRemoteTree(BaseRemoteTree):
             yield blob.name
 
     def walk_files(self, path_info, **kwargs):
-        for fname in self._list_paths(path_info / "", **kwargs):
+        if not kwargs.pop("prefix", False):
+            path_info = path_info / ""
+        for fname in self._list_paths(path_info, **kwargs):
             # skip nested empty directories
             if fname.endswith("/"):
                 continue

dvc/remote/oss.py

@@ -100,6 +100,8 @@ class OSSRemoteTree(BaseRemoteTree):  # pylint:disable=abstract-method
             yield blob.key
 
     def walk_files(self, path_info, **kwargs):
+        if not kwargs.pop("prefix", False):
+            path_info = path_info / ""
         for fname in self._list_paths(path_info):
             if fname.endswith("/"):
                 continue

dvc/remote/s3.py

@@ -187,7 +187,9 @@ class S3RemoteTree(BaseRemoteTree):
         )
 
     def walk_files(self, path_info, **kwargs):
-        for fname in self._list_paths(path_info / "", **kwargs):
+        if not kwargs.pop("prefix", False):
+            path_info = path_info / ""
+        for fname in self._list_paths(path_info, **kwargs):
             if fname.endswith("/"):
                 continue
 

tests/unit/remote/test_base.py

@@ -142,6 +142,20 @@ def test_list_hashes(dvc):
         assert hashes == ["123456"]
 
 
+def test_list_paths(dvc):
+    tree = BaseRemoteTree(dvc, {})
+    tree.path_info = PathInfo("foo")
+
+    with mock.patch.object(tree, "walk_files", return_value=[]) as walk_mock:
+        for _ in tree.list_paths():
+            pass
+        walk_mock.assert_called_with(tree.path_info, prefix=False)
+
+        for _ in tree.list_paths(prefix="000"):
+            pass
+        walk_mock.assert_called_with(tree.path_info / "00" / "0", prefix=True)
+
+
 @pytest.mark.parametrize(
     "hash_, result",
     [(None, False), ("", False), ("3456.dir", True), ("3456", False)],