From e20d1fc061c4b19267c407c0a3fbf3efc52d4bc0 Mon Sep 17 00:00:00 2001 From: drew2a Date: Thu, 23 May 2024 13:24:54 +0200 Subject: [PATCH] Refactor proxy settings and enhance error handling The proxy settings method has been refactored for better readability and maintainability. The code now checks if all necessary values are provided before proceeding with the setup, improving error handling. This change also includes updates to the corresponding tests, ensuring they align with the new implementation. Additionally, new test cases have been added to cover corner cases where some values might be None. --- .../download_manager/download_manager.py | 44 +++++++------- .../libtorrent/tests/test_download_manager.py | 57 ++++++++++++------- 2 files changed, 60 insertions(+), 41 deletions(-) diff --git a/src/tribler/core/components/libtorrent/download_manager/download_manager.py b/src/tribler/core/components/libtorrent/download_manager/download_manager.py index 36d9772a0f..dfa0ba0964 100644 --- a/src/tribler/core/components/libtorrent/download_manager/download_manager.py +++ b/src/tribler/core/components/libtorrent/download_manager/download_manager.py @@ -362,29 +362,35 @@ def get_session(self, hops=0): return self.ltsessions[hops] - def set_proxy_settings(self, ltsession, ptype, server: Optional[Tuple] = None, auth: Optional[List] = None): + def set_proxy_settings(self, session, ptype, server: Optional[Tuple[str, str]] = None, + auth: Optional[Tuple[str, str]] = None): """ Apply the proxy settings to a libtorrent session. This mechanism changed significantly in libtorrent 1.1.0. """ - settings = {} - settings["proxy_type"] = ptype - settings["proxy_hostnames"] = True - settings["proxy_peer_connections"] = True - if server: - proxy_port = None + settings = {"proxy_type": ptype, "proxy_hostnames": True, "proxy_peer_connections": True} - try: - proxy_port = int(server[1]) - except ValueError as e: - self._logger.exception(e) - - if server[0] and proxy_port: - settings["proxy_hostname"] = server[0] - settings["proxy_port"] = proxy_port - if auth: - settings["proxy_username"] = auth[0] - settings["proxy_password"] = auth[1] - self.set_session_settings(ltsession, settings) + try: + if not all(v is not None for v in server): + raise ValueError('Host and port should not be None') + + host, port = server + port = int(port) + settings["proxy_hostname"] = host + settings["proxy_port"] = port + except (ValueError, TypeError) as e: + self._logger.exception(e) + + try: + if not all(v is not None for v in auth): + raise ValueError('Username and password should not be None') + + username, proxy_password = auth + settings["proxy_username"] = username + settings["proxy_password"] = proxy_password + except (ValueError, TypeError) as e: + self._logger.exception(e) + + self.set_session_settings(session, settings) def set_upload_rate_limit(self, rate, hops=None): # Rate conversion due to the fact that we had a different system with Swift diff --git a/src/tribler/core/components/libtorrent/tests/test_download_manager.py b/src/tribler/core/components/libtorrent/tests/test_download_manager.py index 2cae0d3d1a..4ce6aa98b4 100644 --- a/src/tribler/core/components/libtorrent/tests/test_download_manager.py +++ b/src/tribler/core/components/libtorrent/tests/test_download_manager.py @@ -346,28 +346,15 @@ def test_set_proxy_settings(fake_dlmgr): """ Test setting the proxy settings """ - - def on_proxy_set(settings): - assert settings - assert settings.hostname == 'a' - assert settings.port == 1234 - assert settings.username == 'abc' - assert settings.password == 'def' - - def on_set_settings(settings): - assert settings - assert settings['proxy_hostname'] == 'a' - assert settings['proxy_port'] == 1234 - assert settings['proxy_username'] == 'abc' - assert settings['proxy_password'] == 'def' - assert settings['proxy_peer_connections'] - assert settings['proxy_hostnames'] - - mock_lt_session = MagicMock() - mock_lt_session.get_settings = dict - mock_lt_session.set_settings = on_set_settings - mock_lt_session.set_proxy = on_proxy_set # Libtorrent < 1.1.0 uses set_proxy to set proxy settings - fake_dlmgr.set_proxy_settings(mock_lt_session, 0, ('a', "1234"), ('abc', 'def')) + session = Mock() + fake_dlmgr.set_proxy_settings(session, 0, ('a', "1234"), ('abc', 'def')) + settings, = session.method_calls[-1].args + assert settings['proxy_hostname'] == 'a' + assert settings['proxy_port'] == 1234 + assert settings['proxy_username'] == 'abc' + assert settings['proxy_password'] == 'def' + assert settings['proxy_peer_connections'] + assert settings['proxy_hostnames'] def test_payout_on_disconnect(fake_dlmgr): @@ -598,3 +585,29 @@ def test_set_proxy_defaults(fake_dlmgr): assert 'proxy_port' not in settings assert 'proxy_hostname' not in settings assert settings['proxy_type'] == proxy_type + + +def test_set_proxy_corner_case(fake_dlmgr): + # Test setting the proxy settings with None values + session = Mock() + + fake_dlmgr.set_proxy_settings(session, 2, (None, None)) + settings, = session.method_calls[-1].args + assert settings['proxy_type'] == 2 + assert 'proxy_port' not in settings + + fake_dlmgr.set_proxy_settings(session, 3, (None,)) + settings, = session.method_calls[-1].args + assert settings['proxy_type'] == 3 + assert 'proxy_port' not in settings + + fake_dlmgr.set_proxy_settings(session, 3, (None, 123)) + settings, = session.method_calls[-1].args + assert settings['proxy_type'] == 3 + assert 'proxy_port' not in settings + + fake_dlmgr.set_proxy_settings(session, 3, (None, 123), ('name', None)) + settings, = session.method_calls[-1].args + assert settings['proxy_type'] == 3 + assert 'proxy_port' not in settings + assert 'proxy_username' not in settings