jschneier / django-storages

https://django-storages.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
2.74k stars 859 forks source link

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

Closed pjpetersik closed 5 months ago

pjpetersik commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

jschneier commented 5 months ago

Looks great, thanks for updating!

Toruitas commented 5 months ago

Thanks for this!