From 5b9ef98428597d49fc2701936012cfda319fd271 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Oct 31 2019 12:31:59 +0000 Subject: [PATCH 1/3] Implement openRemoteFile with requests or compatrequests Signed-off-by: Patrick Uiterwijk --- 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 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 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 = [