jenkins-bot has submitted this change. ( https://gerrit.wikimedia.org/r/c/pywikibot/core/+/658058 )
Change subject: [IMPR] return requests.Response with http.request ......................................................................
[IMPR] return requests.Response with http.request
- return requests.Response with http.request; this is a breaking change but usual not used directly - modify api.Request._http_request to return the requests.Response object - modify api.Request._json_loads to use Resonse object and its json() method directly. Remove old compat code and ignore site.encoding() - update getImagePageHtml - remove api_tests.TestAPIMWException; fetch test was never used and was not functional. Now remove the requests tests too including utils.DummyHttp and PatchedHttp because both does not work with requests.Response object and it i to heavy to fix.
Bug: T265206 Change-Id: I4c9e18935cbec8ef3a6f7d974498b0fb140de31b --- M pywikibot/comms/http.py M pywikibot/data/api.py M pywikibot/page/__init__.py M tests/api_tests.py M tests/utils.py 5 files changed, 35 insertions(+), 263 deletions(-)
Approvals: JJMC89: Looks good to me, approved jenkins-bot: Verified
diff --git a/pywikibot/comms/http.py b/pywikibot/comms/http.py index 1c7f6e2..3e2748f 100644 --- a/pywikibot/comms/http.py +++ b/pywikibot/comms/http.py @@ -11,7 +11,7 @@ - Basic HTTP error handling """ # -# (C) Pywikibot team, 2007-2020 +# (C) Pywikibot team, 2007-2021 # # Distributed under the terms of the MIT license. # @@ -215,25 +215,26 @@
@deprecated_args(body='data') -def request(site, uri: Optional[str] = None, headers=None, **kwargs) -> str: +def request(site, + uri: Optional[str] = None, + headers: Optional[dict] = None, + **kwargs) -> requests.Response: """ Request to Site with default error handling and response decoding.
See L{requests.Session.request} for additional parameters.
- If the site argument is provided, the uri is a relative uri from - and including the document root '/'. - - If the site argument is None, the uri must be absolute. + The optional uri is a relative uri from site base uri including the + document root '/'.
@param site: The Site to connect to - @type site: L{pywikibot.site.BaseSite} + @type: site: pywikibot.site.BaseSite @param uri: the URI to retrieve @keyword charset: Either a valid charset (usable for str.decode()) or None to automatically chose the charset from the returned header (defaults to latin-1) @type charset: CodecInfo, str, None - @return: The received data + @return: The received data Response """ kwargs.setdefault('verify', site.verify_SSL_certificate()) old_validation = kwargs.pop('disable_ssl_certificate_validation', None) @@ -254,7 +255,7 @@ baseuri = site.base_url(uri) r = fetch(baseuri, headers=headers, **kwargs) site.throttle.retry_after = int(r.headers.get('retry-after', 0)) - return r.text + return r
def get_authentication(uri: str) -> Optional[Tuple[str, str]]: diff --git a/pywikibot/data/api.py b/pywikibot/data/api.py index bde7b4f..2106b0a 100644 --- a/pywikibot/data/api.py +++ b/pywikibot/data/api.py @@ -7,7 +7,6 @@ import datetime import hashlib import inspect -import json import os import pickle import pprint @@ -1534,16 +1533,17 @@ _logger) return use_get, uri, body, headers
- def _http_request(self, use_get, uri, body, headers, paramstring) -> tuple: + def _http_request(self, use_get: bool, uri: str, data, headers, + paramstring) -> tuple: """Get or post a http request with exception handling.
- @return: a tuple containing data from request and use_get value + @return: a tuple containing requests.Response object from + http.request and use_get value """ try: - data = http.request( - self.site, uri=uri, - method='GET' if use_get else 'POST', - data=body, headers=headers) + response = http.request(self.site, uri=uri, + method='GET' if use_get else 'POST', + data=data, headers=headers) except Server504Error: pywikibot.log('Caught HTTP 504 error; retrying') except Server414Error: @@ -1564,32 +1564,28 @@ pywikibot.error(traceback.format_exc()) pywikibot.log('{}, {}'.format(uri, paramstring)) else: - return data, use_get + return response, use_get self.wait() return None, use_get
- def _json_loads(self, data: Union[str, bytes]) -> Optional[dict]: - """Read source text and return a dict. + def _json_loads(self, response) -> Optional[dict]: + """Return a dict from requests.Response.
- @param data: raw data string + @param response: a requests.Response object + @type response: requests.Response @return: a data dict @raises APIError: unknown action found @raises APIError: unknown query result type """ - if not isinstance(data, str): - data = data.decode(self.site.encoding()) - pywikibot.debug(('API response received from {}:\n' - .format(self.site)) + data, _logger) - if data.startswith('unknown_action'): - raise APIError(data[:14], data[16:]) try: - result = json.loads(data) + result = response.json() except ValueError: # if the result isn't valid JSON, there must be a server # problem. Wait a few seconds and try again pywikibot.warning( 'Non-JSON response received from server {}; ' - 'the server may be down.'.format(self.site)) + 'the server may be down.\nStatus code:{}' + .format(self.site, response.status_code)) # there might also be an overflow, so try a smaller limit for param in self._params: if param.endswith('limit'): @@ -1600,11 +1596,6 @@ pywikibot.output('Set {} = {}' .format(param, self[param])) else: - if result and not isinstance(result, dict): - raise APIError('Unknown', - 'Unable to process query response of type {}.' - .format(type(result)), - data=result) return result or {} self.wait() return None @@ -1804,12 +1795,12 @@
use_get, uri, body, headers = self._get_request_params(use_get, paramstring) - rawdata, use_get = self._http_request(use_get, uri, body, headers, - paramstring) - if rawdata is None: + response, use_get = self._http_request(use_get, uri, body, headers, + paramstring) + if response is None: continue
- result = self._json_loads(rawdata) + result = self._json_loads(response) if result is None: continue
diff --git a/pywikibot/page/__init__.py b/pywikibot/page/__init__.py index a803780..7970018 100644 --- a/pywikibot/page/__init__.py +++ b/pywikibot/page/__init__.py @@ -2347,17 +2347,16 @@ self.site.loadimageinfo(self, history=True) return self._file_revisions
- def getImagePageHtml(self): - """ - Download the file page, and return the HTML, as a string. + def getImagePageHtml(self) -> str: + """Download the file page, and return the HTML, as a string.
Caches the HTML code, so that if you run this method twice on the same FilePage object, the page will only be downloaded once. """ if not hasattr(self, '_imagePageHtml'): - path = '%s/index.php?title=%s' \ - % (self.site.scriptpath(), self.title(as_url=True)) - self._imagePageHtml = http.request(self.site, path) + path = '{}/index.php?title={}'.format(self.site.scriptpath(), + self.title(as_url=True)) + self._imagePageHtml = http.request(self.site, path).text return self._imagePageHtml
@deprecated('get_file_url', since='20160609', future_warning=True) diff --git a/tests/api_tests.py b/tests/api_tests.py index 8974259..86f36a0 100644 --- a/tests/api_tests.py +++ b/tests/api_tests.py @@ -9,7 +9,6 @@
from collections import defaultdict from contextlib import suppress -from urllib.parse import unquote_to_bytes
import pywikibot.data.api as api import pywikibot.family @@ -27,105 +26,7 @@ DefaultDrySiteTestCase, ) from tests import patch -from tests.utils import FakeLoginManager, PatchedHttp - - -class TestAPIMWException(DefaultSiteTestCase): - - """Test raising an APIMWException.""" - - user = True - - data = {'error': {'code': 'internal_api_error_fake', - 'info': 'Fake error message'}, - 'servedby': 'unittest', - } - - def _dummy_request(self, *args, **kwargs): - self.assertLength(args, 1) # one positional argument for http.request - site = args[0] - self.assertIsInstance(site, pywikibot.BaseSite) - self.assertIn('data', kwargs) - self.assertIn('uri', kwargs) - if kwargs['data'] is None: - # use uri and remove script path - parameters = kwargs['uri'] - prefix = site.scriptpath() + '/api.php?' - self.assertEqual(prefix, parameters[:len(prefix)]) - parameters = parameters[len(prefix):] - else: - parameters = kwargs['data'] - parameters = parameters.encode('ascii') # it should be bytes anyway - # Extract parameter data from the body, it's ugly but allows us - # to verify that we actually test the right request - parameters = [p.split(b'=', 1) for p in parameters.split(b'&')] - keys = [p[0].decode('ascii') for p in parameters] - values = [unquote_to_bytes(p[1]) for p in parameters] - values = [v.decode(site.encoding()) for v in values] - values = [v.replace('+', ' ') for v in values] - values = [set(v.split('|')) for v in values] - parameters = dict(zip(keys, values)) - - if 'fake' not in parameters: - return False # do an actual request - if self.assert_parameters: - for param, value in self.assert_parameters.items(): - self.assertIn(param, parameters) - if value is not None: - if isinstance(value, str): - value = value.split('|') - self.assertLessEqual(set(value), parameters[param]) - return self.data - - def setUp(self): - """Mock warning and error.""" - super().setUp() - self.warning_patcher = patch.object(pywikibot, 'warning') - self.error_patcher = patch.object(pywikibot, 'error') - self.warning_patcher.start() - self.error_patcher.start() - - def tearDown(self): - """Check warning and error calls.""" - self.warning_patcher.stop() - self.error_patcher.stop() - super().tearDown() - - def _test_assert_called_with(self, req): - with self.assertRaises(api.APIMWException): - req.submit() - pywikibot.warning.assert_called_with( - 'API error internal_api_error_fake: Fake error message') - pywikibot.error.assert_called_with( - 'Detected MediaWiki API exception internal_api_error_fake: ' - 'Fake error message\n[servedby: unittest]; raising') - - def test_API_error(self): - """Test a static request.""" - req = api.Request(site=self.site, parameters={'action': 'query', - 'fake': True}) - with PatchedHttp(api, self.data): - self._test_assert_called_with(req) - - def test_API_error_encoding_ASCII(self): - """Test a Page instance as parameter using ASCII chars.""" - page = pywikibot.page.Page(self.site, 'ASCII') - req = api.Request(site=self.site, parameters={'action': 'query', - 'fake': True, - 'titles': page}) - self.assert_parameters = {'fake': ''} - with PatchedHttp(api, self._dummy_request): - self._test_assert_called_with(req) - - def test_API_error_encoding_Unicode(self): - """Test a Page instance as parameter using non-ASCII chars.""" - page = pywikibot.page.Page(self.site, 'Ümlä üt') - req = api.Request(site=self.site, parameters={'action': 'query', - 'fake': True, - 'titles': page}) - self.assert_parameters = {'fake': ''} - with PatchedHttp(api, self._dummy_request): - self._test_assert_called_with(req) +from tests.utils import FakeLoginManager
class TestApiFunctions(DefaultSiteTestCase): diff --git a/tests/utils.py b/tests/utils.py index 291f11d..5025dbd 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -5,18 +5,14 @@ # Distributed under the terms of the MIT license. # import inspect -import json import os import sys import warnings
-from collections.abc import Mapping from contextlib import contextmanager from subprocess import PIPE, Popen, TimeoutExpired from types import ModuleType
-from requests import Response - import pywikibot
from pywikibot import config @@ -449,122 +445,6 @@ pass
-class DummyHttp(object): - - """A class simulating the http module.""" - - def __init__(self, wrapper): - """Initializer with the given PatchedHttp instance.""" - self.__wrapper = wrapper - - def request(self, *args, **kwargs): - """The patched request method.""" - result = self.__wrapper.before_request(*args, **kwargs) - if result is False: - result = self.__wrapper._old_http.request(*args, **kwargs) - elif isinstance(result, Mapping): - result = json.dumps(result) - elif not isinstance(result, str): - raise ValueError('The result is not a valid type ' - '"{0}"'.format(type(result))) - response = self.__wrapper.after_request(result, *args, **kwargs) - if response is None: - response = result - return response - - def fetch(self, *args, **kwargs): - """The patched fetch method.""" - result = self.__wrapper.before_fetch(*args, **kwargs) - if result is False: - result = self.__wrapper._old_http.fetch(*args, **kwargs) - elif not isinstance(result, Response): - raise ValueError('The result is not a valid type "{}"' - .format(type(result))) - response = self.__wrapper.after_fetch(result, *args, **kwargs) - if response is None: - response = result - return response - - -class PatchedHttp(object): - - """ - A ContextWrapper to handle any data going through the http module. - - This patches the C{http} import in the given module to a class simulating - C{request} and C{fetch}. It has a C{data} attribute which is either a - static value which the requests will return or it's a callable returning - the data. If it's a callable it'll be called with the same parameters as - the original function in the L{http} module. For fine grained control it's - possible to override/monkey patch the C{before_request} and C{before_fetch} - methods. By default they just return C{data} directory or call it if it's - callable. - - Even though L{http.request} is calling L{http.fetch}, it won't call the - patched method. - - The data returned for C{request} may either be C{False}, a C{str} or a - C{Mapping} which is converted into a json string. The data returned for - C{fetch} can only be C{False} or a L{requests.Response}. For both - variants any other types are not allowed and if it is False it'll use the - original method and do an actual request. - - Afterwards it is always calling C{after_request} or C{after_fetch} with the - response and given arguments. That can return a different response too, but - can also return None so that the original response is forwarded. - """ - - def __init__(self, module, data=None): - """ - Initializer. - - @param module: The given module to patch. It must have the http module - imported as http. - @type module: Module - @param data: The data returned for any request or fetch. - @type data: callable or False (or other depending on request/fetch) - """ - super().__init__() - self._module = module - self.data = data - - def _handle_data(self, *args, **kwargs): - """Return the data after it may have been called.""" - if self.data is None: - raise ValueError('No handler is defined.') - - if callable(self.data): - return self.data(*args, **kwargs) - - return self.data - - def before_request(self, *args, **kwargs): - """Return the value which should is returned by request.""" - return self._handle_data(*args, **kwargs) - - def before_fetch(self, *args, **kwargs): - """Return the value which should is returned by fetch.""" - return self._handle_data(*args, **kwargs) - - def after_request(self, response, *args, **kwargs): - """Handle the response after request.""" - pass - - def after_fetch(self, response, *args, **kwargs): - """Handle the response after fetch.""" - pass - - def __enter__(self): - """Patch the http module property.""" - self._old_http = self._module.http - self._module.http = DummyHttp(self) - return self - - def __exit__(self, exc_type, exc_value, traceback): - """Reset the http module property.""" - self._module.http = self._old_http - - def execute(command, data_in=None, timeout=None, error=None): """ Execute a command and capture outputs.
pywikibot-commits@lists.wikimedia.org