From 018511d30a9f941bed1cf77054083f01533f8a75 Mon Sep 17 00:00:00 2001 From: kingamjick Date: Sun, 8 Jan 2023 15:06:11 +0000 Subject: [PATCH 1/4] Add more detailed assertions for entries generated by the sftp_list plugin. Previously the test cases only focused on if a certain entry existed, not that it contined the correct data. As such this change adds assert_entries() method which allows more detail about the entries generated to be specified as well as checking if any erroneous entires were also generated. This change preceeds the change of symlink handling in the sftp_list plugin to make it easier to assert the URL change in the test. This change also includes a small bit of cleanup in terms of the task names to make it clearer which variables are been set in that test case. --- flexget/tests/test_sftp_download.py | 72 +++++------ flexget/tests/test_sftp_list.py | 192 +++++++++++++++++++--------- 2 files changed, 168 insertions(+), 96 deletions(-) diff --git a/flexget/tests/test_sftp_download.py b/flexget/tests/test_sftp_download.py index d5049e42f5..fc2b3c9beb 100644 --- a/flexget/tests/test_sftp_download.py +++ b/flexget/tests/test_sftp_download.py @@ -14,63 +14,57 @@ class TestSftpDownload: _config = """ templates: anchors: - _sftp_list: &base_sftp_download - to: {{ download_path }} - socket_timeout_sec: 2 - connection_tries: 1 + _sftp_download: &base_sftp_download + to: {{ download_path }} + socket_timeout_sec: 2 + connection_tries: 1 + _mock_file: + - &mock_file + {'title': 'file.mkv', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', 'host_key': { + 'key_type': 'ssh-rsa', + 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' + }} + _mock_dir: + - &mock_dir + {'title': 'dir', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', 'host_key': { + 'key_type': 'ssh-rsa', + 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' + }} tasks: sftp_download_file: - mock: - - {'title': 'file.mkv', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', 'host_key': { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' - }} + mock: + - *mock_file accept_all: True sftp_download: <<: *base_sftp_download - delete_origin: False sftp_download_dir: - mock: - - {'title': 'dir', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', 'host_key': { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' - }} + mock: + - *mock_dir accept_all: True sftp_download: to: {{ download_path }} - delete_origin: False - - sftp_download_dir_recusive: - mock: - - {'title': 'dir', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', 'host_key': { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' - }} + + sftp_download_dir_recusive_true: + mock: + - *mock_dir accept_all: True sftp_download: to: {{ download_path }} - delete_origin: False recursive: True - sftp_download_file_delete_origin: - mock: - - {'title': 'file.mkv', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', 'host_key': { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' - }} + sftp_download_file_delete_origin_true: + mock: + - *mock_file accept_all: True sftp_download: <<: *base_sftp_download delete_origin: True - sftp_download_dir_delete_origin: - mock: - - {'title': 'dir', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', 'host_key': { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj' - }} + sftp_download_dir_delete_origin_true_recursive_true: + mock: + - *mock_dir accept_all: True sftp_download: <<: *base_sftp_download @@ -117,7 +111,7 @@ def test_sftp_download_dir_recusive( sftp_fs.create_file('dir/file.mkv', 100) sftp_fs.create_file('dir/nested/file.mkv', 100) - execute_task('sftp_download_dir_recusive') + execute_task('sftp_download_dir_recusive_true') assert filecmp.dircmp(remote_dir, download_path / 'dir') def test_sftp_download_file_and_delete( @@ -125,7 +119,7 @@ def test_sftp_download_file_and_delete( ): remote_file: Path = sftp_fs.create_file('file.mkv', 100) - execute_task('sftp_download_file_delete_origin') + execute_task('sftp_download_file_delete_origin_true') assert not remote_file.exists() @pytest.mark.skip( @@ -138,5 +132,5 @@ def test_sftp_download_dir_and_delete( sftp_fs.create_file('dir/file.mkv', 100) sftp_fs.create_file('dir/nested/file.mkv', 100) - execute_task('sftp_download_dir_delete_origin') + execute_task('sftp_download_dir_delete_origin_true_recursive_true') assert not remote_dir.exists() diff --git a/flexget/tests/test_sftp_list.py b/flexget/tests/test_sftp_list.py index 0d49e8d07a..f23a5579d6 100644 --- a/flexget/tests/test_sftp_list.py +++ b/flexget/tests/test_sftp_list.py @@ -1,6 +1,5 @@ # coding=utf-8 -from pathlib import Path -from typing import Callable +from typing import Any, Callable, Dict, List import pytest @@ -32,49 +31,38 @@ class TestSftpList: <<: *base_sftp_list <<: *sftp_basic_auth - sftp_list_files: + sftp_list: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth - sftp_list_files_absolute: + sftp_list_dirs_absolute: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth dirs: - '/downloads' - sftp_list_files_recursive: + sftp_list_recursive_true: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth recursive: True - sftp_list_dirs: + sftp_list_files_only_false: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth files_only: False - sftp_list_dirs_recursive: + sftp_list_files_only_false_recursive_true: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth files_only: False recursive: True - sftp_list_symlink: - sftp_list: - <<: *base_sftp_list - <<: *sftp_basic_auth - - sftp_list_symlink_recursive: - sftp_list: - <<: *base_sftp_list - <<: *sftp_basic_auth - recursive: True - - sftp_list_size: + sftp_list_get_size_true_files_only_false_recursive_true: sftp_list: <<: *base_sftp_list <<: *sftp_basic_auth @@ -82,7 +70,7 @@ class TestSftpList: get_size: True recursive: True - sftp_list_key: + sftp_list_private_key: sftp_list: <<: *base_sftp_list username: test_user @@ -107,62 +95,88 @@ def test_sftp_list_bad_login( def test_sftp_list_files(self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem): sftp_fs.create_file('file.mkv') - task = execute_task('sftp_list_files') - assert task.find_entry(title='file.mkv'), 'file.mkv not found' - - def test_sftp_list_files(self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem): - sftp_fs.create_file('file.mkv') - - task = execute_task('sftp_list_files') - assert task.find_entry(title='file.mkv'), 'file.mkv not found' + assert_entries( + execute_task('sftp_list'), + { + 'title': 'file.mkv', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', + }, + ) def test_sftp_list_files_recursive_false( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('dir/file.mkv') - task = execute_task('sftp_list_files') - assert not task.find_entry(title='file.mkv'), 'dir/file.mkv found when not recusive' + assert_no_entries(execute_task('sftp_list')) def test_sftp_list_files_recursive_true( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('dir/file.mkv') - task = execute_task('sftp_list_files_recursive') - assert task.find_entry(title='file.mkv'), 'dir/file.mkv not found' + assert_entries( + execute_task('sftp_list_recursive_true'), + { + 'title': 'file.mkv', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir/file.mkv', + }, + ) def test_sftp_list_dirs(self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem): sftp_fs.create_dir('dir') - task = execute_task('sftp_list_dirs') - assert task.find_entry(title='dir'), 'dir dir not found' + assert_entries( + execute_task('sftp_list_files_only_false'), + { + 'title': 'dir', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', + }, + ) def test_sftp_list_dirs_recursive_false( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_dir('dir/nested') - task = execute_task('sftp_list_dirs') - assert not task.find_entry(title='nested'), 'dir/nested found when not recusive' + assert_entries( + execute_task('sftp_list_files_only_false'), + { + 'title': 'dir', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', + }, + ) def test_sftp_list_dirs_recursive_true( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_dir('dir/nested') - task = execute_task('sftp_list_dirs_recursive') - assert task.find_entry(title='nested'), 'dir/nested not found when recusive' + assert_entries( + execute_task('sftp_list_files_only_false_recursive_true'), + { + 'title': 'dir', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', + }, + { + 'title': 'nested', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir/nested', + }, + ) def test_sftp_list_file_size( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('file.mkv', 24) - task = execute_task('sftp_list_size') - assert ( - task.find_entry(title='file.mkv')['content_size'] == 24 - ), 'file should have size of 24' + assert_entries( + execute_task('sftp_list_get_size_true_files_only_false_recursive_true'), + { + 'title': 'file.mkv', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', + 'content_size': 24, + }, + ) def test_sftp_list_dir_size( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem @@ -170,16 +184,25 @@ def test_sftp_list_dir_size( sftp_fs.create_file('dir/file1.mkv', 40) sftp_fs.create_file('dir/file2.mkv', 60) - task = execute_task('sftp_list_size') - assert task.find_entry(title='dir')['content_size'] == 100, 'dir should have size of 100' + assert_entries( + execute_task('sftp_list_get_size_true_files_only_false_recursive_true'), + { + 'title': 'dir', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir', + 'content_size': 100, + }, + allow_unexpected_entires=True, + ) def test_sftp_list_symlink_file( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): - sftp_fs.create_symlink('file.mkv', sftp_fs.create_file('target.mkv', 100)) + sftp_fs.create_symlink('file.mkv', sftp_fs.create_file('/target.mkv', 100)) - task = execute_task('sftp_list_symlink') - assert task.find_entry(title='file.mkv'), 'file.mkv not found' + assert_entries( + execute_task('sftp_list'), + {'title': 'file.mkv', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/target.mkv'}, + ) def test_sftp_list_uses_private_key_auth( self, execute_task: Callable[..., Task], sftp: TestSFTPServerController @@ -187,31 +210,86 @@ def test_sftp_list_uses_private_key_auth( sftp_fs: TestSFTPFileSystem = sftp.start(key_only=True) sftp_fs.create_file('file.mkv') - task = execute_task('sftp_list_key') + execute_task('sftp_list_private_key') def test_sftp_list_private_key_set_on_entry( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('file.mkv') - task = execute_task('sftp_list_key') - assert task.find_entry(title='file.mkv')['private_key'] == 'test_sftp_user_key' + assert_entries( + execute_task('sftp_list_private_key'), + {'title': 'file.mkv', 'private_key': 'test_sftp_user_key'}, + ) def test_sftp_list_private_key_pass_set_on_entry( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('file.mkv') - task = execute_task('sftp_list_key') - assert task.find_entry(title='file.mkv')['private_key_pass'] == 'password' + assert_entries( + execute_task('sftp_list_private_key'), + {'title': 'file.mkv', 'private_key_pass': 'password'}, + ) def test_sftp_list_host_key_set_on_entry( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ): sftp_fs.create_file('file.mkv') - task = execute_task('sftp_list_key') - assert task.find_entry(title='file.mkv')['host_key'] == { - 'key_type': 'ssh-rsa', - 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj', - } + assert_entries( + execute_task('sftp_list'), + { + 'title': 'file.mkv', + 'host_key': { + 'key_type': 'ssh-rsa', + 'public_key': 'AAAAB3NzaC1yc2EAAAADAQABAAABAQC7Hn9BizDY6wI1oNYUBoVHAVioXzOJkZDPB+QsUHDBOqVIcdL/glfMtgIO1E5khoBYql8DSSI+EyrxaC+mfeJ7Ax5qZnimOFvZsJvwvO5h7LI4W1KkoJrYUfMLFfHkDy5EbPIuXeAQGdF/JzOXoIqMcCmKQDS56WRDnga91CGQeXAuzINiviZ63R55b8ynN2JFqKW5V6WZiYZBSmTia68s2ZefkFMiv7E6gmD4WYj6hitz8FGPUoyFAGIR+NVqZ5i9l/8CDuNcZ8E8G7AmNFQhChAeQdEOPO0f2vdH6aRb8Cn0EAy6zpBllxQO8EuLjiEfH01n4/VlGeQEiXlyCLqj', + }, + }, + ) + + +def assert_entries( + task: Task, + entry_matcher: Dict[str, Any], + *argv: Dict[str, Any], + allow_unexpected_entires: bool = False, +): + """ + Asserts that the entries generated for a given task match the list of dictionaries given as + entry matches. Only the keys specified will be check for. + + :param task: Task to assert the entries from. + :param entry_matcher: Dict continain the expected entry values, must have at least a 'title' + set. + :param allow_unexpected_entires: bool to assert if there are any additional entries generated + that matchers arn't specified for. + """ + expected = list(map(lambda m: m['title'], [entry_matcher, *argv])) + found = list(map(lambda m: m['title'], task.all_entries)) + if not allow_unexpected_entires: + unexpected: List[str] = [title for title in found if title not in expected] + assert not unexpected, f'Found unexpected entries {unexpected}' + + not_found: List[str] = [title for title in expected if title not in found] + assert not not_found, f'Entires not found {not_found} in {found}' + + for matcher in [entry_matcher, *argv]: + entry = task.find_entry(title=matcher['title']) + assert entry, f"Expected entry with title {matcher['title']}, but found none" + for k, v in matcher.items(): + assert k in entry.store, f"Expected entry {matcher['title']} to have attribute {k}" + assert ( + entry[k] == v + ), f"Expected entry {matcher['title']} to have value {v} for attribute {k}, but was {entry[k]}" + + +def assert_no_entries(task: Task): + """ + Asserts that there are no entries generated for a given task. + + :param task: Task to assert no entires are generated for. + """ + assert ( + len(task.all_entries) == 0 + ), f"Expected no entries, but found {list(map(lambda m: m['title'], task.all_entries))}" From 9001587b94685502605c27d0ece750e5c35b82c1 Mon Sep 17 00:00:00 2001 From: kingamjick Date: Sat, 4 Mar 2023 16:47:22 +0000 Subject: [PATCH 2/4] Dont normalized every path returned via the sftp_list plugin. Previously every path returned from the sftp_list plugin was normalised, this meant that symlinks were always resolved before been returned making it impossible to get the symlinks path. --- flexget/components/ftp/sftp_client.py | 11 +++++++++-- flexget/tests/test_sftp_list.py | 19 ++++++++++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/flexget/components/ftp/sftp_client.py b/flexget/components/ftp/sftp_client.py index cb2cbab74c..e3f9f67c3b 100644 --- a/flexget/components/ftp/sftp_client.py +++ b/flexget/components/ftp/sftp_client.py @@ -107,8 +107,15 @@ def list_directories( for directory in directories: try: + # Always normalize the root path so it's not necessary to normalised + # nodes as there are discovered, which means that symlinks will appear + # in the entry paths raw rather than been resolved to their target. self._sftp.walktree( - directory, file_handler, dir_handler, unknown_handler, recursive + self._sftp.normalize(directory), + file_handler, + dir_handler, + unknown_handler, + recursive, ) except OSError as e: logger.warning('Failed to open {} ({})', directory, str(e)) @@ -572,7 +579,7 @@ def _get_entry( private_key_pass: Optional[str], host_key: Optional[HostKey], ) -> Entry: - url = urljoin(prefix, quote(sftp.normalize(path))) + url = urljoin(prefix, quote(path)) title = PurePosixPath(path).name entry = Entry(title, url) diff --git a/flexget/tests/test_sftp_list.py b/flexget/tests/test_sftp_list.py index f23a5579d6..9f5608c0b1 100644 --- a/flexget/tests/test_sftp_list.py +++ b/flexget/tests/test_sftp_list.py @@ -201,7 +201,24 @@ def test_sftp_list_symlink_file( assert_entries( execute_task('sftp_list'), - {'title': 'file.mkv', 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/target.mkv'}, + { + 'title': 'file.mkv', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', + }, + ) + + def test_sftp_list_symlink_dir( + self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem + ): + sftp_fs.create_symlink('dir', sftp_fs.create_dir('/target_dir')) + sftp_fs.create_file('/target_dir/file.mkv') + + assert_entries( + execute_task('sftp_list_recursive_true'), + { + 'title': 'file.mkv', + 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/dir/file.mkv', + }, ) def test_sftp_list_uses_private_key_auth( From 769c9b353c7635a7b28ac582d1f7b511cd7de67b Mon Sep 17 00:00:00 2001 From: kingamjick Date: Mon, 9 Jan 2023 22:43:13 +0000 Subject: [PATCH 3/4] If a sftp_download entry points to a symlink and delete_origin is true, only remove the symlink, and not it's target. This changes the behaviour when dealing with entries representing symlinks. If entry previously represented a symlink it would remove it's target or in the case of a directory it's contents. This changes that behaviour such as in the case of a symlink, only the link will be removed, but not it's target. --- flexget/components/ftp/sftp_client.py | 29 +++++++++++++++++++++------ flexget/tests/test_sftp_download.py | 10 +++++++++ flexget/tests/test_sftp_server.py | 14 +++++++------ 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/flexget/components/ftp/sftp_client.py b/flexget/components/ftp/sftp_client.py index e3f9f67c3b..84c2436bea 100644 --- a/flexget/components/ftp/sftp_client.py +++ b/flexget/components/ftp/sftp_client.py @@ -5,6 +5,7 @@ from dataclasses import dataclass from functools import partial from pathlib import Path, PurePath, PurePosixPath +from stat import S_ISLNK from typing import Callable, List, Optional from urllib.parse import quote, urljoin @@ -129,7 +130,8 @@ def download(self, source: str, to: str, recursive: bool, delete_origin: bool) - :param source: path of the resource to download :param to: path of the directory to download to :param recursive: indicates whether to download the contents of "source" recursively - :param delete_origin: indicates whether to delete the source resource upon download + :param delete_origin: indicates whether to delete the source resource upon download, is the source + is a symlink, only the symlink will be removed rather than it's target. """ dir_handler: NodeHandler = self._handler_builder.get_null_handler() @@ -140,21 +142,25 @@ def download(self, source: str, to: str, recursive: bool, delete_origin: bool) - if not self.path_exists(source): raise SftpError(f'Remote path does not exist: {source}') + is_symlink: bool = self.is_link(source) if self.is_file(source): source_file: str = parsed_path.name source_dir: str = parsed_path.parent.as_posix() try: self._sftp.cwd(source_dir) - self._download_file(to, delete_origin, source_file) + self._download_file(to, delete_origin and not is_symlink, source_file) except Exception as e: raise SftpError(f'Failed to download file {source} ({str(e)})') - if delete_origin: - self.remove_dir(source_dir) + if delete_origin and is_symlink: + self.remove_file(source) + elif self.is_dir(source): base_path: str = parsed_path.joinpath('..').as_posix() dir_name: str = parsed_path.name - handle_file: NodeHandler = partial(self._download_file, to, delete_origin) + handle_file: NodeHandler = partial( + self._download_file, to, delete_origin and not is_symlink + ) try: self._sftp.cwd(base_path) @@ -163,7 +169,10 @@ def download(self, source: str, to: str, recursive: bool, delete_origin: bool) - raise SftpError(f'Failed to download directory {source} ({str(e)})') if delete_origin: - self.remove_dir(source) + if self.is_link(source): + self.remove_file(source) + else: + self.remove_dir(source) else: logger.warning('Skipping unknown file: {}', source) @@ -218,6 +227,14 @@ def is_dir(self, path: str) -> bool: """ return self._sftp.isdir(path) + def is_link(self, path: str) -> bool: + """ + Check if the node at a given path is a directory + :param path: path to check + :return: boolean indicating if the path is a directory + """ + return S_ISLNK(self._sftp.sftp_client.lstat(path).st_mode) + def path_exists(self, path: str) -> bool: """ Check of a path exists diff --git a/flexget/tests/test_sftp_download.py b/flexget/tests/test_sftp_download.py index fc2b3c9beb..8b2ce74c09 100644 --- a/flexget/tests/test_sftp_download.py +++ b/flexget/tests/test_sftp_download.py @@ -122,6 +122,16 @@ def test_sftp_download_file_and_delete( execute_task('sftp_download_file_delete_origin_true') assert not remote_file.exists() + def test_sftp_download_file_and_delete_when_symlink_deletes_symlink_only( + self, execute_task, download_path: Path, sftp_fs: TestSFTPFileSystem + ): + remote_file: Path = sftp_fs.create_file('/target.mkv', 100) + remote_link: Path = sftp_fs.create_symlink('file.mkv', remote_file) + + execute_task('sftp_download_file_delete_origin_true') + assert remote_file.exists(), '/target.mkv should not have been deleted.' + assert not remote_link.exists(), 'file.mkv should have been deleted.' + @pytest.mark.skip( reason='No attempt is made by the sftp_download plugin to remove the directories)' ) diff --git a/flexget/tests/test_sftp_server.py b/flexget/tests/test_sftp_server.py index c89665d415..ea20a612cb 100644 --- a/flexget/tests/test_sftp_server.py +++ b/flexget/tests/test_sftp_server.py @@ -2,6 +2,7 @@ import logging import os +import posixpath import socket import threading import time @@ -167,23 +168,24 @@ def create_symlink(self, path: str, target: Path) -> Path: canonicalized.symlink_to(target, target.is_dir()) return canonicalized - def canonicalize(self, path: str) -> Path: + def canonicalize(self, path: str, resolve: bool = True) -> Path: """ Canonicalizes the given SFTP path to the local file system either from the the cwd, or the user home if none is set. :param path: The path to canonicalize. + :param resolve: If symlinks shoul be resovled. :raises ValueError: If the path or taget is relative and would take return a directory outside the SFTP filesytem. :return: An :class:`pathlib.Path` of the canonicalized path. """ canonicalized: Path if Path(path).is_absolute(): path = path[1:] - canonicalized = (self.__root / path).resolve() + canonicalized = Path(posixpath.normpath((self.__root / path).as_posix())) else: - canonicalized = ((self.__cwd or self.__home) / path).resolve() + canonicalized = Path(posixpath.normpath(((self.__cwd or self.__home) / path))) if self.__root == canonicalized or self.__root in canonicalized.parents: - return canonicalized + return canonicalized.resolve() if resolve else canonicalized raise ValueError(f'Unable to canoicalize {path}') def root(self) -> Path: @@ -334,14 +336,14 @@ def stat(self, path: str) -> Union[SFTPAttributes, int]: def lstat(self, path: str) -> Union[SFTPAttributes, int]: logger.debug('lstat(%s)', path) try: - return SFTPAttributes.from_stat(os.lstat(self.__fs.canonicalize(path))) + return SFTPAttributes.from_stat(os.lstat(self.__fs.canonicalize(path, resolve=False))) except OSError as e: return TestSFTPServer.log_and_return_error_code(e) def remove(self, path: str) -> int: logger.debug('remove(%s)', path) try: - self.__fs.canonicalize(path).unlink() + self.__fs.canonicalize(path, resolve=False).unlink() except OSError as e: return TestSFTPServer.log_and_return_error_code(e) return SFTP_OK From a07f18a22d07c0e20c0b33096e2525feb9248fa6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 4 Mar 2023 17:09:03 +0000 Subject: [PATCH 4/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- flexget/tests/test_sftp_list.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flexget/tests/test_sftp_list.py b/flexget/tests/test_sftp_list.py index 9f5608c0b1..43491885ac 100644 --- a/flexget/tests/test_sftp_list.py +++ b/flexget/tests/test_sftp_list.py @@ -206,7 +206,7 @@ def test_sftp_list_symlink_file( 'url': 'sftp://test_user:test_pass@127.0.0.1:40022/home/test_user/file.mkv', }, ) - + def test_sftp_list_symlink_dir( self, execute_task: Callable[..., Task], sftp_fs: TestSFTPFileSystem ):