Skip to content
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.

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

[16.0] [IMP] fs_attachment: store attachments linked to different model/fields to different FS storages #269

Merged
merged 42 commits into from
Aug 24, 2023

Conversation

marielejeune
Copy link
Contributor

@marielejeune marielejeune commented Aug 1, 2023

Supersedes #260 (This PR is part of the RFC #252 and depends on it.)

One may want to store attachments linked to different Odoo models/fields to different FS storages.
This is now possible: on the fs.storage one just has to configure for which models / fields it will be the default storage.
It is also possible to configure it in a server environment file, by giving a comma-separated list of models/fields XML ids.

@marielejeune marielejeune force-pushed the 16.0-fs_attachment_model_field_xmlids-mle branch 2 times, most recently from a93e740 to a954363 Compare August 2, 2023 14:28
@marielejeune marielejeune marked this pull request as ready for review August 2, 2023 14:29
TDu and others added 27 commits August 24, 2023 11:14
Using the base_attachment_object_storage module, the same way
attachment_swift is done. Fixed a few issues along the way in
attachment_swift.
When moving attachments from the filestore to an object storage,  the
filesystem files will be deleted only after the commit, so if the
transaction is rollbacked, we still have the local files for another
try.
Assume the following situation:

* We have installed addons base, sale and attachment_s3 (hence
base_attachment_object_storage as dependency)
* All attachments are in S3 already
* We run an upgrade of the 'base' addon, 'sale' is upgraded before
attachment_s3 in the order of loading.
* Sale updates the icon of the Sale menu
* As attachment_s3 is not loaded yet, the attachment is created in the
filestore

Now if we don't persist the filestore or use different servers, we'll
lose the images of the menus (or any attachment loaded by the
install/upgrade of an addon).

The implemented solution is to move the attachments from the filestore
to the object storage at the loading of the module. However, this
operation can take time and it shouldn't be run by 2 processes at the
same time, so we want to detect if the module is loaded during a normal odoo
startup or when some addons have been upgraded. There is nothing anymore
at this point which allow us to know that modules just have been
upgraded except... in the caller frame (load_modules). We have to rely
on the inpect module and get the caller frame, which is not recommended,
but seems the only way, besides, it's not called often and if
_register_hook was called from another place, it would have no effect
(unless the other place has a variable 'update_module' too).
The reason being:
https://github.com/odoo/odoo/blob/9032617120138848c63b3cfa5d1913c5e5ad76db/odoo/addons/base/ir/ir_attachment.py#L344-L347

I nearly deleted this domain but it was too weird to be there for no
reason. A comment explaining the issue was really missing.
Some attachments (e.g. image_small, image_medium) are stored in DB
instead of the object storage for faster access.

In some situations, we may have pushed all these files on the Object
Storage (migration from a filesystem to object storage) and want to
bring back these attachments from the object storage to the database.

This method is not called anywhere but can be called by RPC or scripts.
The initial issue that triggered this rework is that the forced storage in
database was working only on writes, and was never applied on attachment
creations.

This feature is used to store small files that need to be read in a fast way in
database rather than in the object storage. Reading a file from the object
storage can take 150-200ms, which is fine for downloading a PDF file or a single
image, but not if you need 40 thumbnails.

Down the path to make a correction, I found that:

* the logic to force storage was called in `_inverse_datas`, which is not called
  during a create
* odoo implemented a new method `_get_datas_related_values`, which is a model
  method that receive only the data and the mimetype, and return the attachment
  values and write the file to the correct place

The `_get_datas_related_values` is where we want to plug this special storage,
as it is called for create and write, and already handle the values and
conditional write. But using this method, we have less information than before
about the attachment, so let's review the different criterias we had before:

* res_model: we were using it to always store attachments related to
  'ir.ui.view' in db, because assets are related to this model. However, we
  don't really need to check this: we should store any javascript and css
  documents in database.
* exclude res_model: we could have an exclusion list, to tell that for instance,
  for mail.message, we should never store any image in db. We don't have this
  information anymore, but I think it was never used and added "in case of".
  Because the default configuration is "mail.mail" and "mail.message" and I
  couldn't find any attachment with such res_model in any of our biggest
  databases. So this is removed.
* mimetype and data (size) are the last criteria and we still have them

The new system is only based on mimetype and data size and I think it's actually
more versatile. Previously, we could set a global size and include mimetypes,
but we couldn't say "I want to store all images below 50KB and all files of type
X below 10KB". Now, we have a single system parameter with a dict configuration
(`ir_attachment.storage.force.database`) defaulting to:

    {"image/": 51200, "application/javascript": 0, "text/css": 0}

Assets have a limit of zero, which means they will all be stored in the database
whatever their size is.

Overall, this is a great simplification of the module too, as the method
`_get_datas_related_values` integrates it better in the base calls of IrAttachment.

Note for upgrade:

I doubt we customized the previous system parameters which are now obsolete, but
if yes, the configuration may need to be moved to `ir_attachment.storage.force.database`.
For the record, the params were:

* mimetypes.list.storedb (default: image)
* file.maxsize.storedb (default: 51200)
* excluded.models.storedb (mail.message,mail.mail), no equivalent now

The method IrAttachment.force_storage_to_db_for_special_fields() should be called
through a migration script on existing databases to move the attachments back into
the database.
The main goal is to be able to easily do grep and sed when we
do mass update on them
@lmignon lmignon force-pushed the 16.0-fs_attachment_model_field_xmlids-mle branch from a954363 to 593584c Compare August 24, 2023 09:19
@lmignon lmignon force-pushed the 16.0-fs_attachment_model_field_xmlids-mle branch from 593584c to df278ca Compare August 24, 2023 09:22
lmignon and others added 12 commits August 24, 2023 11:28
Also fix typo into summary
Avoid recompute of new columns when installed on an existing database
Allows to provide configuration parameters through server environement files.
Remove code used to try to read file from the root filesystem and write into the specialized filesystem. This code was used to try to provide a way to manage staging environments by reusing the same filesystem storage but with a different directory_path depending of the environement. A simpler method is to configure use a different filesystem storage by environement. If a production database is restored in pre production env, you can declare a new filesystem storage with a different code to store the attachements by default and configure the filesystem storage from the production with information allowing to read documents stored in it but not to modify or delete existing documents. This make the implementation far more simple.
To create a new cursor, just ask to the current registry.... Loading a registry is very time consuming and could lead to deadlocks...
@lmignon lmignon force-pushed the 16.0-fs_attachment_model_field_xmlids-mle branch from df278ca to e493887 Compare August 24, 2023 09:31
@lmignon
Copy link
Contributor

lmignon commented Aug 24, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-269-by-lmignon-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit b06fbf3 into OCA:16.0 Aug 24, 2023
4 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7a53380. Thanks a lot for contributing to OCA. ❤️

@lmignon lmignon deleted the 16.0-fs_attachment_model_field_xmlids-mle branch August 24, 2023 10:14
@xavierpiernas
Copy link

I've encountered an issue with fs_attachment. After installation, I'm unable to create new attachments as it triggers the "_compute_internal_url" function, which crashes at the following line:

filename, extension = os.path.splitext(rec.name)

This occurs because the attachment initially lacks a name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.