[16.0] fs attachment: A bunch of fixes

fs_attachment/fs_stream.py

@@ -28,6 +28,7 @@ def from_fs_attachment(cls, attachment: IrAttachment) -> FsStream:
             type="fs",
             size=attachment.file_size,
             last_modified=attachment["__last_update"],
+            public=attachment.public,
             fs_attachment=attachment,
         )
 
@@ -46,7 +47,10 @@ def get_response(
     ):
         if self.type != "fs":
             return super().get_response(
-                as_attachment=as_attachment, immutable=immutable, **send_file_kwargs
+                as_attachment=as_attachment,
+                immutable=immutable,
+                content_security_policy=content_security_policy,
+                **send_file_kwargs,
             )
         if as_attachment is None:
             as_attachment = self.as_attachment
@@ -64,6 +68,7 @@ def get_response(
             "max_age": STATIC_CACHE_LONG if immutable else self.max_age,
             "environ": request.httprequest.environ,
             "response_class": Response,
+            **send_file_kwargs,
         }
         use_x_sendfile = self.fs_attachment._fs_use_x_sendfile()
         # The file will be closed by werkzeug...
@@ -81,12 +86,19 @@ def get_response(
             res.headers["X-Sendfile"] = x_sendfile_path
             res.headers["Content-Length"] = 0
 
-        if immutable and res.cache_control:
-            res.cache_control["immutable"] = None
-
         res.headers["X-Content-Type-Options"] = "nosniff"
 
         if content_security_policy:
             res.headers["Content-Security-Policy"] = content_security_policy
 
+        # apply same cache policy as odoo.http.Stream.get_response
+        if self.public:
+            if (res.cache_control.max_age or 0) > 0:
+                res.cache_control.public = True
+        else:
+            res.cache_control.pop("public", "")
+            res.cache_control.private = True
+        if immutable:
+            res.cache_control["immutable"] = None
+
         return res

fs_attachment/models/ir_attachment.py

@@ -9,6 +9,7 @@
 import mimetypes
 import os
 import re
+import shutil
 import time
 from contextlib import closing, contextmanager
 from pathlib import Path
@@ -17,7 +18,6 @@
 import psycopg2
 from slugify import slugify  # pylint: disable=missing-manifest-dependency
 
-import odoo
 from odoo import _, api, fields, models
 from odoo.exceptions import AccessError, UserError
 from odoo.osv.expression import AND, OR, normalize_domain
@@ -131,7 +131,7 @@ def _compute_fs_url_path(self) -> None:
                     rec, exclude_base_url=True
                 )
 
-    @api.depends("fs_filename")
+    @api.depends("store_fname", "fs_filename")
     def _compute_fs_storage_id(self):
         for rec in self:
             if rec.store_fname:
@@ -414,6 +414,7 @@ def _storage_file_write(self, bin_data: bytes) -> str:
         with fs.open(path, "wb", **kwargs) as f:
             f.write(bin_data)
         self._fs_mark_for_gc(fname)
+        self._fs_get_files_written_in_transaction().add(fname)
         return fname
 
     @api.model
@@ -478,30 +479,44 @@ def _enforce_meaningful_storage_filename(self) -> None:
         Keeping the same meaning and mimetype is important to also ease to provide
         a meaningful and SEO friendly URL to the file in the filesystem storage.
         """
-        renamed_attachments = {}
+        to_rename = self.browse()
         for attachment in self:
             if not self._is_file_from_a_storage(attachment.store_fname):
                 continue
             fs, storage, filename = attachment._get_fs_parts()
 
             if self.env["fs.storage"]._must_use_filename_obfuscation(storage):
                 attachment.fs_filename = filename
-                continue
+            else:
+                to_rename |= attachment
+        to_rename._set_meaningful_storage_filename()
+
+    def _set_meaningful_storage_filename(self) -> None:
+        """Give each attachment's file its meaningful name"""
+        usage = self._count_by_store_fname()
+        written_files = self._fs_get_files_written_in_transaction()
+        for attachment in self:
+            fs, storage, filename = attachment._get_fs_parts()
+            old_store_fname = attachment.store_fname
             new_filename = attachment._build_fs_filename()
             # we must keep the same full path as the original filename
             new_filename_with_path = os.path.join(
                 os.path.dirname(filename), new_filename
             )
 
-            if filename in renamed_attachments:
-                if renamed_attachments[filename] == new_filename_with_path:
-                    # we already renamed this file, no need to rename it again
-                    continue
-                else:
-                    fs.copy(renamed_attachments[filename], new_filename_with_path)
+            if usage[old_store_fname] > 1:
+                # the file stays with the other attachments stored in it
+                usage[old_store_fname] -= 1
+                fs.copy(filename, new_filename_with_path)
+            elif old_store_fname not in written_files:
+                # The file has not been written by the current transaction
+                # We can't rename it to ensure that in case of rollback
+                # the original one still exists... In place we copy
+                # the original and we mark it to GC
+                fs.copy(filename, new_filename_with_path)
+                self._fs_mark_for_gc(old_store_fname)
             else:
                 fs.rename(filename, new_filename_with_path)
-            renamed_attachments[filename] = new_filename_with_path
 
             attachment.fs_filename = new_filename
             # we need to update the store_fname with the new filename by
@@ -510,6 +525,31 @@ def _enforce_meaningful_storage_filename(self) -> None:
             # flake8: noqa: E231
             attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}")
             self._fs_mark_for_gc(attachment.store_fname)
+            written_files.add(attachment.store_fname)
+
+    @api.model
+    def _fs_get_files_written_in_transaction(self) -> set[str]:
+        """Return the store_fname of the files written into a filesystem
+        storage by the current transaction.
+        """
+        return self.env.cr.postcommit.data.setdefault(
+            "fs_attachment.written_files", set()
+        )
+
+    def _count_by_store_fname(self) -> dict[str, int]:
+        """Return the number of attachments stored in each file of self"""
+        # "res_field = False OR res_field != False" to count the attachments of
+        # binary fields too, see fs.storage
+        domain = [
+            ("store_fname", "in", self.mapped("store_fname")),
+            "|",
+            ("res_field", "=", False),
+            ("res_field", "!=", False),
+        ]
+        groups = self.sudo()._read_group(
+            domain, ["store_fname"], ["store_fname"], lazy=False
+        )
+        return {group["store_fname"]: group["__count"] for group in groups}
 
     def _force_write_store_fname(self, store_fname):
         """Force the write of the store_fname field
@@ -520,6 +560,16 @@ def _force_write_store_fname(self, store_fname):
         """
         self._fields["store_fname"].write(self, store_fname)
 
+    def _force_write_file_values(self, vals):
+        """Force the write of the fields computed from the file content
+
+        The write method of ir_attachment ignores the fields 'checksum' and
+        'file_size'. This method bypass this limitation by calling the write
+        method of the fields directly.
+        """
+        for field_name, value in vals.items():
+            self._fields[field_name].write(self, value)
+
     @api.model
     def _get_fs_storage_for_code(
         self,
@@ -678,8 +728,7 @@ def _do_in_new_env(self, new_cr=False):
         Using a new Odoo Environment thus a new PG transaction.
         """
         if new_cr:
-            registry = odoo.modules.registry.Registry.new(self.env.cr.dbname)
-            with closing(registry.cursor()) as cr:
+            with closing(self.env.registry.cursor()) as cr:
                 try:
                     yield self.env(cr=cr)
                 except Exception:
@@ -1089,19 +1138,9 @@ def _file_open(self) -> io.IOBase:
                 attachment_res_model=self.attachment.res_model,
                 attachment_res_field=self.attachment.res_field,
             )._file_write(content, checksum)
-            if self.attachment._is_file_from_a_storage(new_store_fname):
-                (
-                    filesystem,
-                    _storage,
-                    new_filepath,
-                ) = self.attachment._fs_parse_store_fname(new_store_fname)
-                _fs, _storage, old_filepath = self.attachment._get_fs_parts()
-            else:
-                new_filepath = self.attachment._full_path(new_store_fname)
-                old_filepath = self.attachment._full_path(self.attachment.store_fname)
-                filesystem = fsspec.filesystem("file")
+            filesystem, new_filepath = self._get_filesystem_and_path(new_store_fname)
             if "a" in self.mode:
-                filesystem.cp_file(old_filepath, new_filepath)
+                self._copy_current_content(filesystem, new_filepath, new_store_fname)
             the_file = filesystem.open(
                 new_filepath,
                 mode=self.mode,
@@ -1114,6 +1153,49 @@ def _file_open(self) -> io.IOBase:
         self._new_store_fname = new_store_fname
         self._file = the_file
 
+    def _get_filesystem_and_path(
+        self, store_fname: str
+    ) -> tuple[fsspec.AbstractFileSystem, str]:
+        """Return the filesystem and the path of the file for the given
+        store_fname (filesystem storage or odoo filestore)"""
+        if self.attachment._is_file_from_a_storage(store_fname):
+            filesystem, _storage, path = self.attachment._fs_parse_store_fname(
+                store_fname
+            )
+            return filesystem, path
+        return fsspec.filesystem("file"), self.attachment._full_path(store_fname)
+
+    def _get_location(self, store_fname: str) -> str:
+        """Return the code of the filesystem storage where the file is
+        stored or 'file' for the odoo filestore"""
+        if self.attachment._is_file_from_a_storage(store_fname):
+            return store_fname.partition("://")[0]
+        return "file"
+
+    def _copy_current_content(
+        self,
+        filesystem: fsspec.AbstractFileSystem,
+        new_filepath: str,
+        new_store_fname: str,
+    ) -> None:
+        """Copy the current content of the attachment into the new file
+
+        The current content can be stored in the database, in the odoo
+        filestore or in a filesystem storage different from the one of the
+        new file.
+        """
+        old_store_fname = self.attachment.store_fname
+        if old_store_fname and self._get_location(
+            old_store_fname
+        ) == self._get_location(new_store_fname):
+            # same location: copy the file
+            _filesystem, old_filepath = self._get_filesystem_and_path(old_store_fname)
+            filesystem.cp_file(old_filepath, new_filepath)
+            return
+        with AttachmentFileLikeAdapter(self.attachment, mode="rb") as src:
+            with filesystem.open(new_filepath, "wb") as dst:
+                shutil.copyfileobj(src, dst)
+
     def _gen_random_content(self, size=256):
         """Generate a random content of size bytes"""
         return os.urandom(size)
@@ -1132,7 +1214,15 @@ def _file_close(self):
                 and self._is_stored_in_db
             ):
                 attachment_data["raw"] = self._file.getvalue()
+            # the values computed from the content are removed from the
+            # values by ir.attachment.write
+            file_values = {
+                "checksum": attachment_data["checksum"],
+                "file_size": attachment_data["file_size"],
+            }
             self.attachment.write(attachment_data)
+            if "raw" not in attachment_data:
+                self.attachment._force_write_file_values(file_values)
             if self.new_version and self._new_store_fname:
                 self.attachment._force_write_store_fname(self._new_store_fname)
             self.attachment._enforce_meaningful_storage_filename()

fs_attachment/readme/newsfragments/678.bugfix

@@ -0,0 +1,6 @@
+Don't move a file that other attachments still use.
+
+With filename obfuscation off, storing or renaming an attachment renames its
+file. Attachments stored while obfuscation was on share one file per content,
+so the others lost their content. The module now copies such a file and only
+moves it when no other attachment uses it.