From 10951818743f98099ae01a50ff35ebed4f5cba60 Mon Sep 17 00:00:00 2001 From: JordiMForgeFlow Date: Fri, 12 Jan 2024 10:59:15 +0100 Subject: [PATCH 1/8] [IMP] fs_storage: invalidate filesystem cache when connection fails --- fs_storage/models/fs_storage.py | 23 ++++++++++++++++++++- fs_storage/readme/newsfragments/320.feature | 1 + 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 fs_storage/readme/newsfragments/320.feature diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index a42751a374..d140cb795f 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -264,12 +264,33 @@ def _compute_options_properties(self) -> None: doc = inspect.getdoc(cls.__init__) rec.options_properties = f"__init__{signature}\n{doc}" + def _get_marker_file_name(self): + return ".odoo_fs_storage_%s.marker" % self.id + + def _check_connection(self, fs): + marker_file_name = self._get_marker_file_name() + try: + marker_file = fs.ls(marker_file_name, detail=False) + if not marker_file: + fs.touch(marker_file_name) + except FileNotFoundError: + fs.touch(marker_file_name) + return True + @property def fs(self) -> fsspec.AbstractFileSystem: """Get the fsspec filesystem for this backend.""" self.ensure_one() if not self.__fs: self.__fs = self._get_filesystem() + if not tools.config["test_enable"]: + # Check whether we need to invalidate FS cache or not. + # Use a marker file to limit the scope of the LS command for performance. + try: + self._check_connection(self.__fs) + except Exception as e: + self.__fs.clear_instance_cache() + raise e return self.__fs def _get_filesystem_storage_path(self) -> str: @@ -431,7 +452,7 @@ def delete(self, relative_path) -> None: def action_test_config(self) -> None: try: - self.fs.ls("", detail=False) + self._check_connection(self.__fs) title = _("Connection Test Succeeded!") message = _("Everything seems properly set up!") msg_type = "success" diff --git a/fs_storage/readme/newsfragments/320.feature b/fs_storage/readme/newsfragments/320.feature new file mode 100644 index 0000000000..5644d66085 --- /dev/null +++ b/fs_storage/readme/newsfragments/320.feature @@ -0,0 +1 @@ +Invalidate FS filesystem object cache when the connection fails, forcing a reconnection. From 5901cb585f02e4e79ee426aff95e75f0b68c5a6d Mon Sep 17 00:00:00 2001 From: Tran Anh Tuan Date: Thu, 4 Jan 2024 17:35:18 +0700 Subject: [PATCH 2/8] [IMP] fs_storage: improve find_files pattern match, pattern matching should only apply to file name, not full path --- fs_storage/models/fs_storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index d140cb795f..4d7610d232 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -427,7 +427,8 @@ def find_files(self, pattern, relative_path="", **kw) -> list[str]: return [] regex = re.compile(pattern) for file_path in self.fs.ls(relative_path, detail=False): - if regex.match(file_path): + # fs.ls returns a relative path + if regex.match(os.path.basename(file_path)): result.append(file_path) return result From 292a6b6d41334cc0385774c9495ec4e9e3e94640 Mon Sep 17 00:00:00 2001 From: Nils Hamerlinck Date: Wed, 7 Feb 2024 14:17:55 +0700 Subject: [PATCH 3/8] [IMP] fs_storage: use fs.info() to check for the marker file --- fs_storage/models/fs_storage.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index 4d7610d232..9e9958d25f 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -270,9 +270,7 @@ def _get_marker_file_name(self): def _check_connection(self, fs): marker_file_name = self._get_marker_file_name() try: - marker_file = fs.ls(marker_file_name, detail=False) - if not marker_file: - fs.touch(marker_file_name) + fs.info(marker_file_name) except FileNotFoundError: fs.touch(marker_file_name) return True From cdc09b7153a8f66d0373817b95487f54c77ed74c Mon Sep 17 00:00:00 2001 From: JordiMForgeFlow Date: Wed, 7 Feb 2024 09:19:04 +0100 Subject: [PATCH 4/8] [FIX] fs_storage: fix test connection when __fs slot is not initialized --- fs_storage/models/fs_storage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index 9e9958d25f..ab75c5f3e1 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -451,7 +451,8 @@ def delete(self, relative_path) -> None: def action_test_config(self) -> None: try: - self._check_connection(self.__fs) + # pylint: disable=W0104 + self.fs title = _("Connection Test Succeeded!") message = _("Everything seems properly set up!") msg_type = "success" From 844385a42d2114e4d533945672f234bdeba77685 Mon Sep 17 00:00:00 2001 From: JordiMForgeFlow Date: Thu, 8 Feb 2024 15:21:43 +0100 Subject: [PATCH 5/8] [IMP] fs_storage: clear slot when connection fails --- fs_storage/models/fs_storage.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index ab75c5f3e1..0745df38d9 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -288,6 +288,7 @@ def fs(self) -> fsspec.AbstractFileSystem: self._check_connection(self.__fs) except Exception as e: self.__fs.clear_instance_cache() + self.__fs = None raise e return self.__fs @@ -451,6 +452,7 @@ def delete(self, relative_path) -> None: def action_test_config(self) -> None: try: + # Accessing the property will check the connection # pylint: disable=W0104 self.fs title = _("Connection Test Succeeded!") From daeada17fa4cbcc170e2bce7ffd84019b234175b Mon Sep 17 00:00:00 2001 From: Florian da Costa Date: Fri, 5 Apr 2024 10:39:20 +0200 Subject: [PATCH 6/8] [IMP] fs_storage : Allow to choose different method to check connection --- fs_storage/__init__.py | 1 + fs_storage/__manifest__.py | 1 + fs_storage/models/fs_storage.py | 46 ++++++++++++++++++++++- fs_storage/readme/USAGE.md | 10 +++++ fs_storage/security/ir.model.access.csv | 1 + fs_storage/views/fs_storage_view.xml | 1 + fs_storage/wizards/__init__.py | 1 + fs_storage/wizards/fs_test_connection.py | 28 ++++++++++++++ fs_storage/wizards/fs_test_connection.xml | 31 +++++++++++++++ 9 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 fs_storage/wizards/__init__.py create mode 100644 fs_storage/wizards/fs_test_connection.py create mode 100644 fs_storage/wizards/fs_test_connection.xml diff --git a/fs_storage/__init__.py b/fs_storage/__init__.py index 1df985fe64..6f3a6c7170 100644 --- a/fs_storage/__init__.py +++ b/fs_storage/__init__.py @@ -4,3 +4,4 @@ # then add normal imports from . import models +from . import wizards diff --git a/fs_storage/__manifest__.py b/fs_storage/__manifest__.py index 8d80a3cb18..d209fd8ea7 100644 --- a/fs_storage/__manifest__.py +++ b/fs_storage/__manifest__.py @@ -16,6 +16,7 @@ "data": [ "views/fs_storage_view.xml", "security/ir.model.access.csv", + "wizards/fs_test_connection.xml", ], "demo": ["demo/fs_storage_demo.xml"], "external_dependencies": {"python": ["fsspec>=2024.5.0"]}, diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index 0745df38d9..ed289a8f0e 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -146,6 +146,15 @@ def __init__(self, env, ids=(), prefetch_ids=()): compute="_compute_options_properties", store=False, ) + check_connection_method = fields.Selection( + selection="_get_check_connection_method_selection", + default="marker_file", + help="Set a method if you want the connection to remote to be checked every " + "time the storage is used, in order to remove the obsolete connection from" + " the cache.\n" + "* Create Marker file : Create a file on remote and check it exists\n" + "* List File : List all files from root directory", + ) _sql_constraints = [ ( @@ -157,6 +166,13 @@ def __init__(self, env, ids=(), prefetch_ids=()): _server_env_section_name_field = "code" + @api.model + def _get_check_connection_method_selection(self): + return [ + ("marker_file", _("Create Marker file")), + ("ls", _("List File")), + ] + @property def _server_env_fields(self): return {"protocol": {}, "options": {}, "directory_path": {}} @@ -267,14 +283,29 @@ def _compute_options_properties(self) -> None: def _get_marker_file_name(self): return ".odoo_fs_storage_%s.marker" % self.id - def _check_connection(self, fs): + def _marker_file_check_connection(self, fs): marker_file_name = self._get_marker_file_name() try: fs.info(marker_file_name) except FileNotFoundError: fs.touch(marker_file_name) + + def _ls_check_connection(self, fs): + fs.ls("", detail=False) + + def _check_connection_with_method(self, fs, check_connection_method): + if check_connection_method == "marker_file": + self._marker_file_check_connection(fs) + elif check_connection_method == "ls": + self._ls_check_connection(fs) return True + def _check_connection(self, fs): + check_connection_method = self.check_connection_method or self.env.context.get( + "force_connection_method", "" + ) + return self._check_connection_with_method(fs, check_connection_method) + @property def fs(self) -> fsspec.AbstractFileSystem: """Get the fsspec filesystem for this backend.""" @@ -450,7 +481,18 @@ def move_files(self, files, destination_path, **kw) -> None: def delete(self, relative_path) -> None: self.fs.rm_file(relative_path) - def action_test_config(self) -> None: + def action_test_config(self): + self.ensure_one() + if self.check_connection_method: + return self._test_config() + else: + action = self.env["ir.actions.actions"]._for_xml_id( + "fs_storage.act_open_fs_test_connection_view" + ) + action["context"] = {"active_model": "fs.storage", "active_id": self.id} + return action + + def _test_config(self): try: # Accessing the property will check the connection # pylint: disable=W0104 diff --git a/fs_storage/readme/USAGE.md b/fs_storage/readme/USAGE.md index 2379e3b398..ac73b9bc4b 100644 --- a/fs_storage/readme/USAGE.md +++ b/fs_storage/readme/USAGE.md @@ -18,6 +18,16 @@ When you create a new backend, you must specify the following: documentation. - Resolve env vars. This options resolves the protocol options values starting with \$ from environment variables +- Check Connection Method. If set, Odoo will always check the connection before + using a storage and it will remove the fs connection from the cache if the + check fails. + + - `Create Marker file`: create a hidden file on remote and then check it + exists with Use it if you have write access to the remote and if it is not + an issue to leave the marker file in the root directory. + - `List file`: list all files from the root directory. You can use it if the + directory path does not contain a big list of files (for performance + reasons) Some protocols defined in the fsspec package are wrappers around other protocols. For example, the SimpleCacheFileSystem protocol is a wrapper diff --git a/fs_storage/security/ir.model.access.csv b/fs_storage/security/ir.model.access.csv index 9c079b2eef..c1a81aae11 100644 --- a/fs_storage/security/ir.model.access.csv +++ b/fs_storage/security/ir.model.access.csv @@ -1,2 +1,3 @@ id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink access_fs_storage_edit,fs_storage edit,model_fs_storage,base.group_system,1,1,1,1 +access_fs_test_connection,fs.test.connection.access,model_fs_test_connection,base.group_system,1,1,1,1 diff --git a/fs_storage/views/fs_storage_view.xml b/fs_storage/views/fs_storage_view.xml index 3e25196230..712bb2a656 100644 --- a/fs_storage/views/fs_storage_view.xml +++ b/fs_storage/views/fs_storage_view.xml @@ -42,6 +42,7 @@ options="{'mode': 'python'}" placeholder="Enter you fsspec options here." /> + diff --git a/fs_storage/wizards/__init__.py b/fs_storage/wizards/__init__.py new file mode 100644 index 0000000000..197ee33cb7 --- /dev/null +++ b/fs_storage/wizards/__init__.py @@ -0,0 +1 @@ +from . import fs_test_connection diff --git a/fs_storage/wizards/fs_test_connection.py b/fs_storage/wizards/fs_test_connection.py new file mode 100644 index 0000000000..b5bc86a2ca --- /dev/null +++ b/fs_storage/wizards/fs_test_connection.py @@ -0,0 +1,28 @@ +# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). + +from odoo import api, fields, models + + +class FSTestConnection(models.TransientModel): + _name = "fs.test.connection" + _description = "FS Test Connection Wizard" + + def _get_check_connection_method_selection(self): + return self.env["fs.storage"]._get_check_connection_method_selection() + + storage_id = fields.Many2one("fs.storage") + check_connection_method = fields.Selection( + selection="_get_check_connection_method_selection", + required=True, + ) + + @api.model + def default_get(self, field_list): + res = super().default_get(field_list) + res["storage_id"] = self.env.context.get("active_id", False) + return res + + def action_test_config(self): + return self.storage_id.with_context( + force_connection_method=self.check_connection_method + )._test_config() diff --git a/fs_storage/wizards/fs_test_connection.xml b/fs_storage/wizards/fs_test_connection.xml new file mode 100644 index 0000000000..2abed80164 --- /dev/null +++ b/fs_storage/wizards/fs_test_connection.xml @@ -0,0 +1,31 @@ + + + + fs.test.connection.form + fs.test.connection + +
+ + + + +
+
+
+ + + + FS Test Connection + ir.actions.act_window + fs.test.connection + form + new + + From 58afaf14f57efef4fd9c531784f398cad512f845 Mon Sep 17 00:00:00 2001 From: Florian da Costa Date: Mon, 8 Apr 2024 18:39:31 +0200 Subject: [PATCH 7/8] [FIX] Avoid use of context to pass the check_connection_method --- fs_storage/models/fs_storage.py | 18 +++++------------- fs_storage/wizards/fs_test_connection.py | 4 +--- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index ed289a8f0e..1961a50bd2 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -293,19 +293,13 @@ def _marker_file_check_connection(self, fs): def _ls_check_connection(self, fs): fs.ls("", detail=False) - def _check_connection_with_method(self, fs, check_connection_method): + def _check_connection(self, fs, check_connection_method): if check_connection_method == "marker_file": self._marker_file_check_connection(fs) elif check_connection_method == "ls": self._ls_check_connection(fs) return True - def _check_connection(self, fs): - check_connection_method = self.check_connection_method or self.env.context.get( - "force_connection_method", "" - ) - return self._check_connection_with_method(fs, check_connection_method) - @property def fs(self) -> fsspec.AbstractFileSystem: """Get the fsspec filesystem for this backend.""" @@ -316,7 +310,7 @@ def fs(self) -> fsspec.AbstractFileSystem: # Check whether we need to invalidate FS cache or not. # Use a marker file to limit the scope of the LS command for performance. try: - self._check_connection(self.__fs) + self._check_connection(self.__fs, self.check_connection_method) except Exception as e: self.__fs.clear_instance_cache() self.__fs = None @@ -484,7 +478,7 @@ def delete(self, relative_path) -> None: def action_test_config(self): self.ensure_one() if self.check_connection_method: - return self._test_config() + return self._test_config(self.check_connection_method) else: action = self.env["ir.actions.actions"]._for_xml_id( "fs_storage.act_open_fs_test_connection_view" @@ -492,11 +486,9 @@ def action_test_config(self): action["context"] = {"active_model": "fs.storage", "active_id": self.id} return action - def _test_config(self): + def _test_config(self, connection_method): try: - # Accessing the property will check the connection - # pylint: disable=W0104 - self.fs + self._check_connection(self.fs, connection_method) title = _("Connection Test Succeeded!") message = _("Everything seems properly set up!") msg_type = "success" diff --git a/fs_storage/wizards/fs_test_connection.py b/fs_storage/wizards/fs_test_connection.py index b5bc86a2ca..ebaf6154cc 100644 --- a/fs_storage/wizards/fs_test_connection.py +++ b/fs_storage/wizards/fs_test_connection.py @@ -23,6 +23,4 @@ def default_get(self, field_list): return res def action_test_config(self): - return self.storage_id.with_context( - force_connection_method=self.check_connection_method - )._test_config() + return self.storage_id._test_config(self.check_connection_method) From 732a34fc1994f7c33a0dc48f9c940014aed8fafd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Bidoul?= Date: Fri, 12 Jul 2024 16:10:01 +0200 Subject: [PATCH 8/8] [FIX] fs_storage: add missing sudo when creating FilsSystem This is necessary for non admin users to access file systems, for instance for attachments. This went unnoticed so far presumably because the filesystem is cached and likely accessed by the server during server bootstrap. --- fs_storage/models/fs_storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs_storage/models/fs_storage.py b/fs_storage/models/fs_storage.py index 1961a50bd2..ebbb51149c 100644 --- a/fs_storage/models/fs_storage.py +++ b/fs_storage/models/fs_storage.py @@ -305,7 +305,7 @@ def fs(self) -> fsspec.AbstractFileSystem: """Get the fsspec filesystem for this backend.""" self.ensure_one() if not self.__fs: - self.__fs = self._get_filesystem() + self.__fs = self.sudo()._get_filesystem() if not tools.config["test_enable"]: # Check whether we need to invalidate FS cache or not. # Use a marker file to limit the scope of the LS command for performance.