[16.0][IMP] database_cleanup: purge orphaned attachments - #3566
Conversation
699d8f0 to
ab70051
Compare
StefanRijnhart
left a comment
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks! Judging from the name of the PR and the patterns from this module, I would have expected signalling attachments where the model no longer exists or the res_id is not present as a record. This makes sense as well, but to make it explicit what the issue is with each attachment (and to prepare for extension with other possible error conditions), would you mind adding a reason field to the wizard line model as is done in cleanup.purge.line, with for now a fixed reason like 'File missing in filestore'?
b372147 to
95fb498
Compare
StefanRijnhart
left a comment
StefanRijnhart
left a comment
There was a problem hiding this comment.
Thanks for the updates! Please squash commits.
95fb498 to
e8b0061
Compare
There was a problem hiding this comment.
Thanks, the error message is stored correctly.
Usability wise, I think as a user I would want to see some more information about the attachments, the res_model, res_id and res_name, with or without optional visibility. Is this not something that you want for your own purposes?
Our objective in creating this PR was to clean approximately 15,000 orphaned attachments simultaneously. We didn’t have to inspect every single attachment. |
StefanRijnhart
left a comment
StefanRijnhart
left a comment
There was a problem hiding this comment.
OK, leaving my last comment as a nice to have. Nice work!
| from .common import Common, environment | ||
|
|
||
|
|
||
| # Use post_install to get all models loaded more info: odoo/odoo#13458 |
There was a problem hiding this comment.
I don't think this comment belongs here
|
|
||
| :var orphan: ir.attachment with backing file removed via os.unlink | ||
| :var valid: ir.attachment with file intact | ||
| :return: None |
There was a problem hiding this comment.
remove redundant lines such as ":return: None" to avoid unnecessary noise
e8b0061 to
831b3af
Compare
831b3af to
49ce4fe
Compare
|
|
||
| @tagged("post_install", "-at_install") | ||
| class TestCleanupPurgeLineAttachment(Common): | ||
| def setUp(self): |
There was a problem hiding this comment.
please rewrite it to use setUpClass, since that works faster
|
It appears that the CI failure here is unrelated to this change. Check this comment. |
1a94b71 to
c7c414d
Compare
| last_id = 0 | ||
| ir_attachment = self.env["ir.attachment"] | ||
| while True: | ||
| rows = ir_attachment.search_read( |
There was a problem hiding this comment.
since the purpose of this change is to avoid MemoryError on very large databases, please evict the processed batch from the cache before fetching the next one using invalidate_recordset(["store_fname", "name"])
search_read() limits the size of a single batch here but does not limit the total memory usage of the orm cache
| self.assertIn(self.orphan_attach_id, line_attachment_ids) | ||
| self.assertNotIn(self.valid_attach_id, line_attachment_ids) | ||
|
|
||
| def test_find_does_not_prefetch_unrequested_attachment_fields(self): |
There was a problem hiding this comment.
please add a regression test that processes multiple batches (the batch size can be patched to a small value) and also verifies that the processed batch is evicted from the cache
otherwise the implementation could start retaining all processed attachments in the Environment cache again while this test would still pass
|
@Aldeigja Thanks for the continued effort following up the comments! Can you squash the commits? |
63d90e2 to
eee5cc5
Compare
Add wizard to list ir.attachment records whose store_fname points to a missing file on disk when storage is "file", and allow purging those rows. task 5258
eee5cc5 to
84e80c2
Compare
|
And CI was fixed in the mean time by #3707, so ready for merge. /ocabot merge major |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at b31cab9. Thanks a lot for contributing to OCA. ❤️ |
4 participants
Add wizard to list ir.attachment records whose store_fname points to a missing file on disk when storage is "file", and allow purging those rows.