From 5b9ef98428597d49fc2701936012cfda319fd271 Mon Sep 17 00:00:00 2001
From: Patrick Uiterwijk <puiterwijk@redhat.com>
Date: Oct 31 2019 12:31:59 +0000
Subject: [PATCH 1/3] Implement openRemoteFile with requests or compatrequests


Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>

---

diff --git a/koji/__init__.py b/koji/__init__.py
index 40403bb..40de895 100644
--- a/koji/__init__.py
+++ b/koji/__init__.py
@@ -1659,11 +1659,19 @@ def openRemoteFile(relpath, topurl=None, topdir=None, tempdir=None):
     This is done either via a mounted filesystem (nfs) or http, depending
     on options"""
     if topurl:
+        if requests is None:
+            import koji.compatrequests
+            rsession = koji.compatrequests.Session()
+        else:
+            rsession = requests.Session()
+
         url = "%s/%s" % (topurl, relpath)
-        src = six.moves.urllib.request.urlopen(url)
+        resp = rsession.get(url)
         fo = tempfile.TemporaryFile(dir=tempdir)
-        shutil.copyfileobj(src, fo)
-        src.close()
+        for chunk in resp.iter_content(chunk_size=8192):
+            if chunk:
+                fo.write(chunk)
+        resp.close()
         fo.seek(0)
     elif topdir:
         fn = "%s/%s" % (topdir, relpath)

From a00532721d4e3420c8083134e0cb2c1aa1cd769f Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Oct 31 2019 12:31:59 +0000
Subject: [PATCH 2/3] drop compatrequests variant


---

diff --git a/koji/__init__.py b/koji/__init__.py
index 40de895..354bc4a 100644
--- a/koji/__init__.py
+++ b/koji/__init__.py
@@ -1659,14 +1659,8 @@ def openRemoteFile(relpath, topurl=None, topdir=None, tempdir=None):
     This is done either via a mounted filesystem (nfs) or http, depending
     on options"""
     if topurl:
-        if requests is None:
-            import koji.compatrequests
-            rsession = koji.compatrequests.Session()
-        else:
-            rsession = requests.Session()
-
         url = "%s/%s" % (topurl, relpath)
-        resp = rsession.get(url)
+        resp = requests.get(url)
         fo = tempfile.TemporaryFile(dir=tempdir)
         for chunk in resp.iter_content(chunk_size=8192):
             if chunk:

From 768aaf3fb677cc15ca6f3ae26574705192551a49 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Oct 31 2019 12:58:33 +0000
Subject: [PATCH 3/3] move from urrlib.request.urlopen to requests.get


Fixes: https://pagure.io/koji/issue/1530

---

diff --git a/koji/__init__.py b/koji/__init__.py
index 354bc4a..1051a8a 100644
--- a/koji/__init__.py
+++ b/koji/__init__.py
@@ -1660,12 +1660,10 @@ def openRemoteFile(relpath, topurl=None, topdir=None, tempdir=None):
     on options"""
     if topurl:
         url = "%s/%s" % (topurl, relpath)
-        resp = requests.get(url)
         fo = tempfile.TemporaryFile(dir=tempdir)
-        for chunk in resp.iter_content(chunk_size=8192):
-            if chunk:
+        with requests.get(url) as resp:
+            for chunk in resp.iter_content(chunk_size=8192):
                 fo.write(chunk)
-        resp.close()
         fo.seek(0)
     elif topdir:
         fn = "%s/%s" % (topdir, relpath)
diff --git a/koji/tasks.py b/koji/tasks.py
index 9284d53..b422f29 100644
--- a/koji/tasks.py
+++ b/koji/tasks.py
@@ -29,8 +29,8 @@ import shutil
 import signal
 import time
 
+import requests
 import six.moves.xmlrpc_client
-import six.moves.urllib.request
 from six.moves import range
 
 import koji
@@ -479,12 +479,12 @@ class BaseTaskHandler(object):
                 return fn
             self.logger.debug("Downloading %s", relpath)
             url = "%s/%s" % (self.options.topurl, relpath)
-            fsrc = six.moves.urllib.request.urlopen(url)
-            if not os.path.exists(os.path.dirname(fn)):
-                os.makedirs(os.path.dirname(fn))
-            with open(fn, 'wb') as fdst:
-                shutil.copyfileobj(fsrc, fdst)
-            fsrc.close()
+            with requests.get(url) as resp:
+                if not os.path.exists(os.path.dirname(fn)):
+                    os.makedirs(os.path.dirname(fn))
+                with open(fn, 'wb') as fdst:
+                    for chunk in resp.iter_content(chunk_size=8192):
+                        fdst.write(chunk)
         else:
             fn = "%s/%s" % (self.options.topdir, relpath)
         return fn
diff --git a/tests/test_lib/test_client_session.py b/tests/test_lib/test_client_session.py
index a3db72d..48cbcbe 100644
--- a/tests/test_lib/test_client_session.py
+++ b/tests/test_lib/test_client_session.py
@@ -62,6 +62,7 @@ class TestFastUpload(unittest.TestCase):
 
     def setUp(self):
         self.ksession = koji.ClientSession('http://koji.example.com/kojihub')
+        self.ksession.logout = mock.MagicMock()
         self.do_fake_login()
         # mocks
         self.ksession._callMethod = mock.MagicMock()
@@ -89,7 +90,6 @@ class TestFastUpload(unittest.TestCase):
             self.ksession.fastUpload('nonexistent_file', 'target')
 
     def test_fastUpload_nofile(self):
-
         # fail with nonexistent file (IOError)
         self.file_mock.side_effect = IOError('mocked exception')
         with self.assertRaises(IOError):
@@ -180,6 +180,7 @@ class TestMultiCall(unittest.TestCase):
         self.ksession = koji.ClientSession('http://koji.example.com/kojihub')
         # mocks
         self.ksession._sendCall = mock.MagicMock()
+        self.ksession.logout = mock.MagicMock()
 
     def tearDown(self):
         del self.ksession
diff --git a/tests/test_lib/test_tasks.py b/tests/test_lib/test_tasks.py
index 0074660..ef2c04f 100644
--- a/tests/test_lib/test_tasks.py
+++ b/tests/test_lib/test_tasks.py
@@ -11,6 +11,7 @@ except ImportError:
 from os import path, makedirs
 from tempfile import gettempdir
 from mock import patch, MagicMock, Mock, call
+import requests_mock
 
 import koji
 from koji.tasks import BaseTaskHandler, FakeTask, ForkTask, SleepTask, \
@@ -452,8 +453,8 @@ class TasksTestCase(unittest.TestCase):
         obj = TestTask(123, 'some_method', ['random_arg'], None, options, temp_path)
         self.assertEquals(obj.localPath('test.txt'), dummy_file)
 
-    @patch('six.moves.urllib.request.urlopen', return_value=six.BytesIO(six.b('Important things\nSome more important things\n')))
-    def test_BaseTaskHandler_localPath_no_file(self, mock_urlopen):
+    @requests_mock.Mocker()
+    def test_BaseTaskHandler_localPath_no_file(self, m_requests):
         """
         """
         temp_path = get_tmp_dir_path('TestTask')
@@ -466,10 +467,13 @@ class TasksTestCase(unittest.TestCase):
 
         options = Mock()
         options.topurl = 'https://www.domain.local'
+        url = options.topurl + '/test.txt'
+        m_requests.register_uri('GET', url, text='Important things\nSome more important things\n')
         obj = TestTask(123, 'some_method', ['random_arg'], None, options, temp_path)
 
         self.assertEquals(obj.localPath('test.txt'), target_file_path)
-        mock_urlopen.assert_called_once_with('https://www.domain.local/test.txt')
+        self.assertEquals(m_requests.call_count, 1)
+        self.assertEquals(m_requests.request_history[0].url, url)
 
     def test_BaseTaskHandler_localPath_no_topurl(self):
         """ Tests that the localPath function returns a path when options.topurl is not defined.
diff --git a/tests/test_lib/test_utils.py b/tests/test_lib/test_utils.py
index 5b98dab..c2b2e21 100644
--- a/tests/test_lib/test_utils.py
+++ b/tests/test_lib/test_utils.py
@@ -14,6 +14,7 @@ try:
 except ImportError:
     import unittest
 
+import requests_mock
 from mock import call, patch
 from datetime import datetime
 import koji
@@ -101,62 +102,64 @@ class MiscFunctionTestCase(unittest.TestCase):
         move.assert_not_called()
 
     @mock_open()
-    @mock.patch('six.moves.urllib.request.urlopen')
     @mock.patch('tempfile.TemporaryFile')
-    @mock.patch('shutil.copyfileobj')
-    def test_openRemoteFile(self, m_copyfileobj, m_TemporaryFile,
-                            m_urlopen, m_open):
+    def test_openRemoteFile(self, m_TemporaryFile, m_open):
         """Test openRemoteFile function"""
 
-        mocks = [m_open, m_copyfileobj, m_TemporaryFile, m_urlopen]
+        mocks = [m_open, m_TemporaryFile ]
 
         topurl = 'http://example.com/koji'
         path = 'relative/file/path'
         url = 'http://example.com/koji/relative/file/path'
-
-        # using topurl, no tempfile
-        fo = koji.openRemoteFile(path, topurl)
-        m_urlopen.assert_called_once_with(url)
-        m_urlopen.return_value.close.assert_called_once()
-        m_TemporaryFile.assert_called_once_with(dir=None)
-        m_copyfileobj.assert_called_once()
-        m_open.assert_not_called()
-        assert fo is m_TemporaryFile.return_value
+        with requests_mock.Mocker() as m_requests:
+            m_requests.register_uri('GET', url, text='random content')
+            # using topurl, no tempfile
+            fo = koji.openRemoteFile(path, topurl)
+            self.assertEqual(m_requests.call_count, 1)
+            self.assertEqual(m_requests.request_history[0].url, url)
+            m_TemporaryFile.assert_called_once_with(dir=None)
+            m_open.assert_not_called()
+            assert fo is m_TemporaryFile.return_value
 
         for m in mocks:
             m.reset_mock()
 
-        # using topurl + tempfile
-        tempdir = '/tmp/koji/1234'
-        fo = koji.openRemoteFile(path, topurl, tempdir=tempdir)
-        m_urlopen.assert_called_once_with(url)
-        m_urlopen.return_value.close.assert_called_once()
-        m_TemporaryFile.assert_called_once_with(dir=tempdir)
-        m_copyfileobj.assert_called_once()
-        m_open.assert_not_called()
-        assert fo is m_TemporaryFile.return_value
+        with requests_mock.Mocker() as m_requests:
+            m_requests.register_uri('GET', url, text='random content')
+
+            # using topurl + tempfile
+            tempdir = '/tmp/koji/1234'
+            fo = koji.openRemoteFile(path, topurl, tempdir=tempdir)
+            self.assertEqual(m_requests.call_count, 1)
+            self.assertEqual(m_requests.request_history[0].url, url)
+            m_TemporaryFile.assert_called_once_with(dir=tempdir)
+            m_open.assert_not_called()
+            assert fo is m_TemporaryFile.return_value
 
         for m in mocks:
             m.reset_mock()
 
-        # using topdir
-        topdir = '/mnt/mykojidir'
-        filename = '/mnt/mykojidir/relative/file/path'
-        fo = koji.openRemoteFile(path, topdir=topdir)
-        m_urlopen.assert_not_called()
-        m_TemporaryFile.assert_not_called()
-        m_copyfileobj.assert_not_called()
-        m_open.assert_called_once_with(filename)
-        assert fo is m_open.return_value
+        with requests_mock.Mocker() as m_requests:
+            m_requests.register_uri('GET', url, text='random content')
+            # using topdir
+            topdir = '/mnt/mykojidir'
+            filename = '/mnt/mykojidir/relative/file/path'
+            fo = koji.openRemoteFile(path, topdir=topdir)
+            self.assertEqual(m_requests.call_count, 0)
+            m_TemporaryFile.assert_not_called()
+            m_open.assert_called_once_with(filename)
+            assert fo is m_open.return_value
 
         for m in mocks:
             m.reset_mock()
 
-        # using neither
-        with self.assertRaises(koji.GenericError):
-            koji.openRemoteFile(path)
-        for m in mocks:
-            m.assert_not_called()
+        with requests_mock.Mocker() as m_requests:
+            m_requests.register_uri('GET', url, text='random content')
+            # using neither
+            with self.assertRaises(koji.GenericError):
+                koji.openRemoteFile(path)
+            for m in mocks:
+                m.assert_not_called()
 
     def test_joinpath_bad(self):
         bad_joins = [

