Skip to content

[18.0][IMP] fs_storage: add _move_file helper #606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ArnauCForgeFlow wants to merge 1 commit into
base: 18.0
Choose a base branch
from
+29 −0
Open

[18.0][IMP] fs_storage: add _move_file helper #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions fs_storage/models/fs_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import os.path
import re
import warnings
from pathlib import PurePath
from typing import AnyStr

import fsspec
Expand Down Expand Up @@ -819,3 +820,12 @@ def _get_root_filesystem(self, fs=None):
while hasattr(fs, "fs"):
fs = fs.fs
return fs

def _move_file(self, from_dir_str, to_dir_str, filename):
self.ensure_one()
src = (PurePath(from_dir_str) / filename).as_posix()
if not self.fs.exists(src):
return False
dst = (PurePath(to_dir_str) / filename).as_posix()
self.env.cr.postcommit.add(functools.partial(self.fs.move, src, dst))
Copy link
Copy Markdown
Contributor

@lmignon lmignon May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the fs_storage library is to provide access to an fsspec proxy client instance based on a configured storage backend. The original intention was not to build an additional API layer on top of the fsspec API itself.

To me, it feels strange that calling the _move_file method would not actually move the file by the end of the method call.

If the goal is to address a transactional consistency issue — ensuring that modifications made to the external filesystem remain consistent with the Odoo transaction — then the approach should probably be different, and significantly more complex. (we should rely on the transactional layer of the fsspec library requiring that the different client stubs supports properly the caching mechanism)

In this specific case, you could simply move the file directly within the method and register a rollback hook to move it back if the transaction fails.

So, to be honest, I’m a bit confused by this proposal.

Copy link
Copy Markdown
Contributor

@simahawk simahawk May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ciao @lmignon :)
The fs_storage module itself is already providing a wrapper for that lib.
In this case, we want to make sure that IF for whatever reason the transaction fails on odoo, the file remains where it is.
When you say " the approach should probably be different, and significantly more complex. " do you have anything in mind?
Anyway, we could remove the postcommit part and offer an hook to customize this behavior elsewhere.

Copy link
Copy Markdown
Contributor

@lmignon lmignon May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not investigated further in details but fsspecs comes with a concept of transaction https://filesystem-spec.readthedocs.io/en/latest/api.html#fsspec.spec.AbstractFileSystem.transaction Maybe it doesn't apply to such a kind of operation and I've the feeling it introduce too much complexity....

Regarding the current proposal, I see 2 problems:

  • The call to the method has no direct effect. If the caller expect that the file has been moved as a result to the call to the method, it's not the case. (This should be reflected somewhere into the doc, the name of the method or ...)
  • Since the move is done after the transaction, if it fails, you are also in a incoherent state (yes we are not into a distributed transaction system....)

return True
19 changes: 19 additions & 0 deletions fs_storage/tests/test_fs_storage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,3 +304,22 @@ def test_no_unlink_in_safe_eval(self):
safe_eval.safe_eval(
"env['fs.storage'].search([]).unlink()", {"env": self.env}
)

def test_move_file(self):
src_dir = "src"
dst_dir = "dst"
self.backend.fs.makedirs(src_dir, exist_ok=True)
self.backend.fs.makedirs(dst_dir, exist_ok=True)
self._create_file(self.backend, f"{src_dir}/{self.filename}", self.filedata)

res = self.backend._move_file(src_dir, dst_dir, self.filename)
self.assertTrue(res)
# The actual move runs on post-commit, so until then the file
# must still live in the source directory.
self.assertTrue(self.backend.fs.exists(f"{src_dir}/{self.filename}"))
self.assertFalse(self.backend.fs.exists(f"{dst_dir}/{self.filename}"))

self.env.cr.postcommit.run()

self.assertFalse(self.backend.fs.exists(f"{src_dir}/{self.filename}"))
self.assertTrue(self.backend.fs.exists(f"{dst_dir}/{self.filename}"))
Loading