[18.0][IMP] fs_storage: add _move_file helper#606
Conversation
|
Build fixed here #603 |
ArnauCForgeFlow
force-pushed
the
18.0-imp-fs_storage-add_move_file_method
branch
from
480bdca to
5e87199
Compare
|
Rebased @simahawk |
simahawk
left a comment
simahawk
left a comment
There was a problem hiding this comment.
would be nice to have some test cov
ArnauCForgeFlow
force-pushed
the
18.0-imp-fs_storage-add_move_file_method
branch
from
5e87199 to
4726811
Compare
|
@simahawk added tests |
|
can we merge? @simahawk |
|
This PR has the |
| 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)) |
There was a problem hiding this comment.
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.
5 participants
Add a generic
_move_filehelper onfs.storage: checks that the source file exists and schedulesfs.moveas a post-commit hook so the storage stays in sync with the DB transaction.