From ede809a79211d3ad8d0fb12b0acee6d284712a7b Mon Sep 17 00:00:00 2001
From: drew2a <for.drew@gmail.com>
Date: Thu, 23 May 2024 11:10:37 +0200
Subject: [PATCH] Refactor proxy settings and add error handling

Reworked the 'set_proxy_settings' function in the download manager to handle invalid port values more gracefully. The function now attempts to convert the port value to an integer and logs any ValueError exceptions that occur during this process. This prevents crashes when non-integer values are provided for the port. Also, reorganized import statements according to PEP8 guidelines.

Additionally, a new test case 'test_set_proxy_settings_invalid_port' has been added to verify this behavior. It checks if proxy settings are not set when an invalid port number is provided.
---
 .../download_manager/download_manager.py      | 20 ++++++++++-----
 .../libtorrent/tests/test_download_manager.py | 25 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 7 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 06620faf6b0..36d9772a0f8 100644
--- a/src/tribler/core/components/libtorrent/download_manager/download_manager.py
+++ b/src/tribler/core/components/libtorrent/download_manager/download_manager.py
@@ -8,11 +8,11 @@
 import os
 import time as timemod
 from asyncio import CancelledError, gather, iscoroutine, shield, sleep, wait_for
-from binascii import unhexlify
 from copy import deepcopy
 from shutil import rmtree
-from typing import Callable, Dict, List, Optional
+from typing import Callable, Dict, List, Optional, Tuple
 
+from binascii import unhexlify
 from ipv8.taskmanager import TaskManager
 
 from tribler.core import notifications
@@ -362,7 +362,7 @@ def get_session(self, hops=0):
 
         return self.ltsessions[hops]
 
-    def set_proxy_settings(self, ltsession, ptype, server=None, auth=None):
+    def set_proxy_settings(self, ltsession, ptype, server: Optional[Tuple] = None, auth: Optional[List] = None):
         """
         Apply the proxy settings to a libtorrent session. This mechanism changed significantly in libtorrent 1.1.0.
         """
@@ -370,9 +370,17 @@ def set_proxy_settings(self, ltsession, ptype, server=None, auth=None):
         settings["proxy_type"] = ptype
         settings["proxy_hostnames"] = True
         settings["proxy_peer_connections"] = True
-        if server and server[0] and server[1]:
-            settings["proxy_hostname"] = server[0]
-            settings["proxy_port"] = int(server[1])
+        if server:
+            proxy_port = None
+
+            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]
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 c7f8c24f56a..2cae0d3d1ad 100644
--- a/src/tribler/core/components/libtorrent/tests/test_download_manager.py
+++ b/src/tribler/core/components/libtorrent/tests/test_download_manager.py
@@ -1,5 +1,4 @@
 import asyncio
-import functools
 import itertools
 from asyncio import Future
 from unittest.mock import AsyncMock, MagicMock, Mock
@@ -575,3 +574,27 @@ def test_update_trackers_list_append(fake_dlmgr) -> None:
     actual_trackers = set(itertools.chain.from_iterable(tracker_list))
     assert actual_trackers == {fake_tracker1, fake_tracker2}
 
+
+def test_set_proxy_settings_invalid_port(fake_dlmgr):
+    # Test setting the proxy settings for an invalid port number. In this case port and host should not be set.
+    session = Mock()
+    proxy_type = 2
+
+    fake_dlmgr.set_proxy_settings(session, proxy_type, ('host name', 'invalid port'))
+
+    settings, = session.method_calls[-1].args
+    assert 'proxy_port' not in settings
+    assert 'proxy_hostname' not in settings
+    assert settings['proxy_type'] == proxy_type
+
+
+def test_set_proxy_defaults(fake_dlmgr):
+    # Test setting the proxy settings with default values
+    session = Mock()
+    proxy_type = 2
+
+    fake_dlmgr.set_proxy_settings(session, proxy_type)
+    settings, = session.method_calls[-1].args
+    assert 'proxy_port' not in settings
+    assert 'proxy_hostname' not in settings
+    assert settings['proxy_type'] == proxy_type