Merge pull request #502 from asclepiusaka/issue-451

Fix mkdir so that no create_bucket is called when one already exists

Author: martindurant

s3fs/core.py

@@ -684,8 +684,15 @@ async def _find(self, path, maxdepth=None, withdirs=None, detail=False, prefix="
 
     async def _mkdir(self, path, acl="", create_parents=True, **kwargs):
         path = self._strip_protocol(path).rstrip("/")
+        if not path:
+            raise ValueError
         bucket, key, _ = self.split_path(path)
-        if not key or (create_parents and not await self._exists(bucket)):
+        if await self._exists(bucket):
+            if not key:
+                # requested to create bucket, but bucket already exist
+                raise FileExistsError
+            # else: # do nothing as bucket is already created.
+        elif not key or create_parents:
             if acl and acl not in buck_acls:
                 raise ValueError("ACL not in %s", buck_acls)
             try:
@@ -705,7 +712,7 @@ async def _mkdir(self, path, acl="", create_parents=True, **kwargs):
             except ParamValidationError as e:
                 raise ValueError("Bucket create failed %r: %s" % (bucket, e))
         else:
-            # raises if bucket doesn't exist, but doesn't write anything
+            # raises if bucket doesn't exist and doesn't get create flag.
             await self._ls(bucket)
 
     mkdir = sync_wrapper(_mkdir)

s3fs/tests/test_s3fs.py

@@ -575,6 +575,32 @@ def test_mkdir(s3):
     assert bucket in s3.ls("/")
 
 
+def test_mkdir_existing_bucket(s3):
+    # mkdir called on existing bucket should be no-op and not calling create_bucket
+    # creating a s3 bucket
+    bucket = "test1_bucket"
+    s3.mkdir(bucket)
+    assert bucket in s3.ls("/")
+    # a second call.
+    with pytest.raises(FileExistsError):
+        s3.mkdir(bucket)
+
+
+def test_mkdir_bucket_and_key_1(s3):
+    bucket = "test1_bucket"
+    file = bucket + "/a/b/c"
+    s3.mkdir(file, create_parents=True)
+    assert bucket in s3.ls("/")
+
+
+def test_mkdir_bucket_and_key_2(s3):
+    bucket = "test1_bucket"
+    file = bucket + "/a/b/c"
+    with pytest.raises(FileNotFoundError):
+        s3.mkdir(file, create_parents=False)
+    assert bucket not in s3.ls("/")
+
+
 def test_mkdir_region_name(s3):
     bucket = "test2_bucket"
     s3.mkdir(bucket, region_name="eu-central-1")
@@ -598,6 +624,28 @@ def test_makedirs(s3):
     assert bucket in s3.ls("/")
 
 
+def test_makedirs_existing_bucket(s3):
+    bucket = "test_makedirs_bucket"
+    s3.mkdir(bucket)
+    assert bucket in s3.ls("/")
+    test_file = bucket + "/a/b/c/file"
+    # no-op, and no error.
+    s3.makedirs(test_file)
+
+
+def test_makedirs_pure_bucket_exist_ok(s3):
+    bucket = "test1_bucket"
+    s3.mkdir(bucket)
+    s3.makedirs(bucket, exist_ok=True)
+
+
+def test_makedirs_pure_bucket_error_on_exist(s3):
+    bucket = "test1_bucket"
+    s3.mkdir(bucket)
+    with pytest.raises(FileExistsError):
+        s3.makedirs(bucket, exist_ok=False)
+
+
 def test_bulk_delete(s3):
     with pytest.raises(FileNotFoundError):
         s3.rm(["nonexistent/file"])
@@ -1693,7 +1741,6 @@ def test_requester_pays(s3):
     fn = test_bucket_name + "/myfile"
     s3 = S3FileSystem(requester_pays=True, client_kwargs={"endpoint_url": endpoint_uri})
     assert s3.req_kw["RequestPayer"] == "requester"
-    s3.mkdir(test_bucket_name)
     s3.touch(fn)
     with s3.open(fn, "rb") as f:
         assert f.req_kw["RequestPayer"] == "requester"