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.

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

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