fix: keep same permissions when using transaction (#1826)

Author: siemdejong

fsspec/implementations/local.py

@@ -6,6 +6,7 @@
 import shutil
 import stat
 import tempfile
+from functools import lru_cache
 
 from fsspec import AbstractFileSystem
 from fsspec.compression import compr
@@ -354,6 +355,19 @@ def trailing_sep(path):
     return path.endswith(os.sep) or (os.altsep is not None and path.endswith(os.altsep))
 
 
+@lru_cache(maxsize=1)
+def get_umask(mask: int = 0o666) -> int:
+    """Get the current umask.
+
+    Follows https://stackoverflow.com/a/44130549 to get the umask.
+    Temporarily sets the umask to the given value, and then resets it to the
+    original value.
+    """
+    value = os.umask(mask)
+    os.umask(value)
+    return value
+
+
 class LocalFileOpener(io.IOBase):
     def __init__(
         self, path, mode, autocommit=True, fs=None, compression=None, **kwargs
@@ -417,6 +431,11 @@ def commit(self):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
         shutil.move(self.temp, self.path)
+        try:
+            mask = 0o666
+            os.chmod(self.path, mask & ~get_umask(mask))
+        except RuntimeError:
+            pass
 
     def discard(self):
         if self.autocommit:

fsspec/implementations/tests/test_local.py

@@ -15,7 +15,7 @@
 import fsspec
 from fsspec import compression
 from fsspec.core import OpenFile, get_fs_token_paths, open_files
-from fsspec.implementations.local import LocalFileSystem, make_path_posix
+from fsspec.implementations.local import LocalFileSystem, get_umask, make_path_posix
 from fsspec.tests.test_utils import WIN
 
 files = {
@@ -508,6 +508,60 @@ def test_commit_discard(tmpdir):
         assert not fs.exists(tmpdir + "/bfile")
 
 
+def test_same_permissions_with_and_without_transaction(tmpdir):
+    tmpdir = str(tmpdir)
+
+    with fsspec.open(tmpdir + "/afile", "wb") as f:
+        f.write(b"data")
+
+    fs, urlpath = fsspec.core.url_to_fs(tmpdir + "/bfile")
+    with fs.transaction, fs.open(urlpath, "wb") as f:
+        f.write(b"data")
+
+    assert fs.info(tmpdir + "/afile")["mode"] == fs.info(tmpdir + "/bfile")["mode"]
+
+
+def test_get_umask_uses_cache():
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    get_umask()  # Retrieve the current umask value.
+    get_umask()  # Retrieve the umask again; this should result in a cache hit.
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_file_transaction_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs = LocalFileSystem()
+    with fs.transaction:
+        with fs.open(tmpdir + "/afile", "wb") as f:
+            f.write(b"data")
+        with fs.open(tmpdir + "/bfile", "wb") as f:
+            f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_transactions_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs = LocalFileSystem()
+    with fs.transaction:
+        with fs.open(tmpdir + "/afile", "wb") as f:
+            f.write(b"data")
+    with fs.transaction:
+        with fs.open(tmpdir + "/bfile", "wb") as f:
+            f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
+def test_multiple_filesystems_use_umask_cache(tmpdir):
+    get_umask.cache_clear()  # Clear the cache before the test for testing purposes.
+    fs1 = LocalFileSystem()
+    fs2 = LocalFileSystem()
+    with fs1.transaction, fs1.open(tmpdir + "/afile", "wb") as f:
+        f.write(b"data")
+    with fs2.transaction, fs2.open(tmpdir + "/bfile", "wb") as f:
+        f.write(b"data")
+    assert get_umask.cache_info().hits == 1
+
+
 def test_make_path_posix():
     cwd = os.getcwd()
     if WIN: