Skip to content

[IMP] fs_storage: use fs.info() to check for the marker file#334

Merged
OCA-git-bot merged 1 commit intoOCA:16.0from
nilshamerlinck:16.0-fix-fs_storage
Feb 21, 2024
Merged

[IMP] fs_storage: use fs.info() to check for the marker file#334
OCA-git-bot merged 1 commit intoOCA:16.0from
nilshamerlinck:16.0-fix-fs_storage

Conversation

@nilshamerlinck
Copy link
Contributor

@nilshamerlinck nilshamerlinck commented Feb 7, 2024

I'm facing an issue with #320 and a SFTP storage:

  File "/odoo/external-src/storage/fs_storage/models/fs_storage.py", line 293, in fs
    self._check_connection(self.__fs)
  File "/odoo/external-src/storage/fs_storage/models/fs_storage.py", line 372, in _check_connection
    marker_file = fs.ls(marker_file_name, detail=False)
  File "/home/odoo/.local/lib/python3.10/site-packages/fsspec/implementations/sftp.py", line 127, in ls
    stats = [self._decode_stat(stat, path) for stat in self.ftp.listdir_iter(path)]
  File "/home/odoo/.local/lib/python3.10/site-packages/fsspec/implementations/sftp.py", line 127, in <listcomp>
    stats = [self._decode_stat(stat, path) for stat in self.ftp.listdir_iter(path)]
  File "/usr/local/lib/python3.10/site-packages/paramiko/sftp_client.py", line 278, in listdir_iter
    t, msg = self._request(CMD_OPENDIR, path)
  File "/usr/local/lib/python3.10/site-packages/paramiko/sftp_client.py", line 857, in _request
    return self._read_response(num)
  File "/usr/local/lib/python3.10/site-packages/paramiko/sftp_client.py", line 909, in _read_response
    self._convert_status(msg)
  File "/usr/local/lib/python3.10/site-packages/paramiko/sftp_client.py", line 942, in _convert_status
    raise IOError(text)
OSError: Not a directory

That's because of using fs.ls() to check for the existing of the marker file, see here. The sftp implementation of that method does not (yet) support files: see here.

I understand that there was a rationale behind that choice, here.

But I think it's actually fine to use fs.info(), as underlying implementations override it with efficiency in mind:

Copy link
Contributor

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same error for an SFTP server. ("The specified file is not a directory.")

And for another server, I have an rights error on the fs.touch. I did not have any issue with previous version (before #320) and this PR also solve it.

The exact error is : [Errno 13] User XXX has no view rights to [SFTP_PATH/.odoofs_storage_2.marker]

Of course, it is specific to the rights of this sftp server but I did never met issue with this sftp server before so I guess the touch method require a different right and should be avoided if possible.

@lmignon
Copy link
Contributor

lmignon commented Feb 21, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-334-by-lmignon-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8607558 into OCA:16.0 Feb 21, 2024
@OCA-git-bot
Copy link
Contributor

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

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.

5 participants