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

[sftp] Check full path for existance in ._save() and ._mkdir() #1372

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

pjpetersik
Copy link
Contributor

@pjpetersik pjpetersik commented Mar 22, 2024

This pull request should fix #1363 and the recursion problem mentioned in the issue.

The problem was related to the fact that the .exists(self, name) function assumes that the input is the name and not the full path. However, in ._save() and ._mkdir() a full path instead of the name was passed to the .exists().

To not interfere with the interface of the .exists() function, I introduced a ._exists(self, path) function that uses the full path instead of the name as input.

I am aware that this is sort of a duplicate to what PR #1364 targets but I also wanted to "throw this solution into the ring" ;).

@jschneier
Copy link
Owner

Thanks this makes sense and also maintains the proper .exists() functionality. Can you make a few updates:

  1. Let's add a comment to _exists & exists to make it clear why both exists and let's move ._exists() next to .exist(). I also want to change _exists to _full_path_exists or something.
  2. Add a test or two that shows this is fixed

@pjpetersik
Copy link
Contributor Author

pjpetersik commented Apr 22, 2024

Thanks for the feedback. I added your suggestions.

The important change in the test cases is the addition of the root_path="root" keyword in the setUp() method as well as the following assert
https://github.com/pjpetersik/django-storages/blob/283391f6ebdb8927e6cd8f74b8cabd9ce34e9681/tests/test_sftp.py#L98

Without the changes in sftpstorage.py this assert would fail since mock_sftp.stat.call_args_list[0][0] would return ('root/root/bar',) because of the recursion bug.

@pjpetersik pjpetersik force-pushed the master branch 2 times, most recently from 9f6843b to 283391f Compare April 22, 2024 10:31
@jschneier
Copy link
Owner

Looks great, thanks for updating!

@jschneier jschneier merged commit aa9d0fb into jschneier:master Apr 25, 2024
19 checks passed
@Toruitas
Copy link
Contributor

Thanks for this!

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

Successfully merging this pull request may close these issues.

[Feature] Distinct SFTP_BASE_URL and [Bugfix] sftp.exists() works incorrectly when using root_path
3 participants