-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
Python 3.9 fixes, auth option for webdav, fixed stored filename #285
Conversation
Hi @lmignon, |
752f2e9
to
838337a
Compare
@@ -456,7 +457,7 @@ def _enforce_meaningful_storage_filename(self) -> None: | |||
# we need to update the store_fname with the new filename by | |||
# calling the write method of the field since the write method | |||
# of ir_attachment prevent normal write on store_fname | |||
attachment._force_write_store_fname(f"{storage}://{new_filename}") | |||
attachment._force_write_store_fname(f"{storage}://{new_filename_with_path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wpichler The name should not contains the directory path defined on the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you nest your filesystem into 'rooted_dir' filesystem, the path given as parameter to the rooted_dir
must not appear into the store_fname. (When you define a directory_path
on your backend,
your filesystem is nested into a rooted_dir
filesystem. This nesting could be done on multiple levels)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon I do see problems here - there is a function _get_fs_path - which does allow to optimize the filesystem path (by adding a sub directory). I did use this function (with a monkey patch) to create record depended file system paths (so the pdf generated for the invoice gets as path "Out-Invoice/2023/January/filename.pdf" - gets stored in nextcloud - so available in a nice directory structure for the end user).
The function _get_fs_path get used in _storage_file_write
_storage_file_read does use _fs_parse_store_fname - this function does work with the store_fname - and will not use _get_fs_path !
_file_open - does use _get_fs_parts - this does use _fs_parse_store_fname - so also no _get_fs_path
So the problem i got was - the file was stored in "some/path/filename" - but the system could not read the file - because the store_fname does not had the path - and the read functions do not use the _get_fs_path function to restore the path for reading.
So - why not store the path with store_fname ? I did this change - and it does work like a charm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can manage-it by determining which part of the path is related to the nested rooted_dir
filesystem and only remove this part of the path from the new_filename_with_path
... A rooted_dir
has a path
attribute and a fs
attribute...
something like.....
# rooted_dir path computation...
rooted_paths = []
rooted_dir_fs = fs
while rooted_dir_fs and hasattr(rooted_dir_fs, "path"):
rooted_paths.append(rooted_dir_fs.path)
rooted_dir_fs = getattr(rooted_dir_fs, "fs", None)
rooted_paths.reverse()
rooted_path = "/".join(rooted_paths)
new_filename = new_filename_with_path.replace(rooted_path, "")
In any cases, we must add unit tests for such a cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied your code (with comments that code is coming from your side....) - tested it - does work fine for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need unit tests. IMO, the code could be extracted into a model method on fs.storage.
fs_attachment/models/fs_storage.py
Outdated
@@ -1,6 +1,7 @@ | |||
# Copyright 2023 ACSONE SA/NV | |||
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
|
|||
from __future__ import annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, all oca addons are tested with py 3.10 and odoo 16 itself is tested on py 3.10. You should use py 3.10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah - but odoo 16 does also work with python 3.9 - even the default docker Dockerfile does use debian bullseye - which is based on python 3.9.
And other modules in the repo do already use this future import - so it should be consistent across the modules.
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
The part regarding the path preservation has to be refined. (unittests, ...) |
5fea8e7
to
4743337
Compare
… are not able to store files in custom sub directories [FIX] Do take nested rooted fs paths into account in computation of store_fname. Code by @lmignon - Laurent Mignon (ACSONE)
4743337
to
45f4272
Compare
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Hello Laurent! First, this is a great implementation of the storage concept and I hope there will be little issue with porting modules. It's been working like a charm, except for the paths issue, which breaks everything. I started working on an implementation of your proposed solution, putting that code snippet in a function. In our test case we had a rooted directory part before the filesystem path - basically for a thumbnail the rooted path is "odoo/attachments" and the added path will be something like "53/2b". But, it turns out that the 'rooted_path' was never a problem in our tests. Are you fine with the simple diff if there's a test case checking for the rooted path issue? |
By the way @wpichler, actually commit ef68e0d wasn't necessary, you could simply state: Although it's not very intuitive so it might be good to have some doc in the future. |
No sure what you suggest. Nevertheless a test case with the fix has more chance to be merged than a fix without tests 😏 |
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Add history for commit included from OCA#285 and the author to the contributors list
Hi all,
three small fixes for some small problems...
[FIX] Small fix to reformat the auth option as tuple for protocol webdav. It is not possible to specify a tuple in json !
[FIX] Fixed the stored filename to include the path. Without this you are not able to store files in custom sub directories
[FIX] Added future import annotations to be python 3.9 compatible