jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/453944 )

Change subject: pywikibot.site.Siteinfo: Fix the bug in cache_time when loading a CachedRequest ......................................................................

pywikibot.site.Siteinfo: Fix the bug in cache_time when loading a CachedRequest

Siteinfo._post_process: - Make _post_process a static method (self was not used)

Siteinfo._get_siteinfo: - Make the try block narrower. Only request.submit raises APIError. - If the request was a CachedRequest, use the _cachetime attribute as cache_time. This will resolve T202227. Note that the _cachetime attribute is not UTC. In order to be compatible we will need to change CachedRequest to always use UTC time.

Siteinfo.get: - The cache in the except clause was not used. Replace it with a `pass`.

api.CachedRequest: - Always use UTC timestamps. (see the explanation of Siteinfo._get_siteinfo)

api_tests.py, dry_api_tests.py, cache.py: - Update code related to api.CachedRequest to use utcnow.

site_tests.py: - Add a dry test for this fix. The result of this test used to contain the value of utcnow() instead of '_cache_time'.

tests.__init__: Import MagicMock and add it to __all__ so that it can be used more conveniently.

Bug: T202227 Change-Id: Ic5fe36b3a2440c3e7fdf6837996d0b162d7643c7 --- M pywikibot/data/api.py M pywikibot/site.py M scripts/maintenance/cache.py M tests/__init__.py M tests/api_tests.py M tests/dry_api_tests.py M tests/site_tests.py 7 files changed, 48 insertions(+), 27 deletions(-)

Approvals: Xqt: Looks good to me, approved jenkins-bot: Verified

diff --git a/pywikibot/data/api.py b/pywikibot/data/api.py index 2acf7fb..9adc8f5 100644 --- a/pywikibot/data/api.py +++ b/pywikibot/data/api.py @@ -2389,7 +2389,7 @@ self._create_file_name())

def _expired(self, dt): - return dt + self.expiry < datetime.datetime.now() + return dt + self.expiry < datetime.datetime.utcnow()

def _load_cache(self): """Load cache entry for request, if available. @@ -2419,7 +2419,7 @@

def _write_cache(self, data): """Write data to self._cachefile_path().""" - data = [self._uniquedescriptionstr(), data, datetime.datetime.now()] + data = (self._uniquedescriptionstr(), data, datetime.datetime.utcnow()) with open(self._cachefile_path(), 'wb') as f: pickle.dump(data, f, protocol=config.pickle_protocol)

diff --git a/pywikibot/site.py b/pywikibot/site.py index 0adfbcc..0177c0e 100644 --- a/pywikibot/site.py +++ b/pywikibot/site.py @@ -1456,7 +1456,8 @@ else: return pywikibot.tools.EMPTY_DEFAULT

- def _post_process(self, prop, data): + @staticmethod + def _post_process(prop, data): """Do some default handling of data. Directly modifies data.""" # Be careful with version tests inside this here as it might need to # query this method to actually get the version number @@ -1523,19 +1524,19 @@ if len(props) == 0: raise ValueError('At least one property name must be provided.') invalid_properties = [] + request = self._site._request( + expiry=pywikibot.config.API_config_expiry + if expiry is False else expiry, + parameters={ + 'action': 'query', 'meta': 'siteinfo', 'siprop': props, + } + ) + # With 1.25wmf5 it'll require continue or rawcontinue. As we don't + # continue anyway we just always use continue. + request['continue'] = True + # warnings are handled later + request._warning_handler = warn_handler try: - request = self._site._request( - expiry=pywikibot.config.API_config_expiry - if expiry is False else expiry, - parameters={ - 'action': 'query', 'meta': 'siteinfo', 'siprop': props, - } - ) - # With 1.25wmf5 it'll require continue or rawcontinue. As we don't - # continue anyway we just always use continue. - request['continue'] = True - # warnings are handled later - request._warning_handler = warn_handler data = request.submit() except api.APIError as e: if e.code == 'siunknown_siprop': @@ -1561,7 +1562,9 @@ pywikibot.log(u"Unable to get siprop(s) '{0}'".format( u"', '".join(invalid_properties))) if 'query' in data: - cache_time = datetime.datetime.utcnow() + # If the request is a CachedRequest, use the _cachetime attr. + cache_time = getattr( + request, '_cachetime', None) or datetime.datetime.utcnow() for prop in props: if prop in data['query']: self._post_process(prop, data['query'][prop]) @@ -1658,7 +1661,7 @@ try: cached = self._get_cached(key) except KeyError: - cached = None + pass else: # cached value available # is a default value, but isn't accepted if not cached[1] and not get_default: diff --git a/scripts/maintenance/cache.py b/scripts/maintenance/cache.py index 27752da..6eb9085 100755 --- a/scripts/maintenance/cache.py +++ b/scripts/maintenance/cache.py @@ -348,13 +348,13 @@

def older_than(entry, interval): """Find older entries.""" - if entry._cachetime + interval < datetime.datetime.now(): + if entry._cachetime + interval < datetime.datetime.utcnow(): return entry

def newer_than(entry, interval): """Find newer entries.""" - if entry._cachetime + interval >= datetime.datetime.now(): + if entry._cachetime + interval >= datetime.datetime.utcnow(): return entry

diff --git a/tests/__init__.py b/tests/__init__.py index 650ccac..39a1651 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -7,8 +7,9 @@ # from __future__ import absolute_import, print_function, unicode_literals

-__all__ = ('requests', 'unittest', 'TestRequest', - 'patch_request', 'unpatch_request', 'mock', 'Mock', 'patch') +__all__ = ( + 'requests', 'unittest', 'TestRequest', 'patch_request', 'unpatch_request', + 'mock', 'Mock', 'MagicMock', 'patch')

import functools import os @@ -35,10 +36,10 @@ import unittest try: import unittest.mock as mock - from unittest.mock import Mock, patch + from unittest.mock import MagicMock, Mock, patch except ImportError: import mock - from mock import Mock, patch + from mock import MagicMock, Mock, patch

_root_dir = os.path.split(os.path.split(__file__)[0])[0]

diff --git a/tests/api_tests.py b/tests/api_tests.py index 0a04e61..7e0d9b6 100644 --- a/tests/api_tests.py +++ b/tests/api_tests.py @@ -933,7 +933,7 @@ mysite = self.get_site() # Run tests on a missing page unique to this test run so it can # not be cached the first request, but will be cached after. - now = datetime.datetime.now() + now = datetime.datetime.utcnow() params = {'action': 'query', 'prop': 'info', 'titles': 'TestCachedRequest_test_internals ' + str(now), diff --git a/tests/dry_api_tests.py b/tests/dry_api_tests.py index 2b2f206..b0ec916 100644 --- a/tests/dry_api_tests.py +++ b/tests/dry_api_tests.py @@ -73,8 +73,9 @@

def test_expired(self): """Test if the request is expired.""" - self.assertFalse(self.req._expired(datetime.datetime.now())) - self.assertTrue(self.req._expired(datetime.datetime.now() - datetime.timedelta(days=2))) + self.assertFalse(self.req._expired(datetime.datetime.utcnow())) + self.assertTrue(self.req._expired( + datetime.datetime.utcnow() - datetime.timedelta(days=2)))

def test_parameter_types(self): """Test _uniquedescriptionstr is identical using different ways.""" diff --git a/tests/site_tests.py b/tests/site_tests.py index 01ae4b0..cf7a9b4 100644 --- a/tests/site_tests.py +++ b/tests/site_tests.py @@ -34,7 +34,7 @@ UnicodeType as unicode, )

-from tests import patch, unittest_print +from tests import patch, unittest_print, MagicMock from tests.aspects import ( unittest, TestCase, DeprecationTestCase, TestCaseBase, @@ -2414,6 +2414,22 @@ self.assertFalse(entered_loop(mysite.siteinfo.get(not_exists).keys()))

+class TestSiteinfoDry(DefaultDrySiteTestCase): + + """Test Siteinfo in dry mode.""" + + def test_siteinfo_timestamps(self): + """Test that cache has the timestamp of CachedRequest.""" + site = self.get_site() + request_mock = MagicMock() + request_mock.submit = lambda: {'query': {'_prop': '_value'}} + request_mock._cachetime = '_cache_time' + with patch.object(site, '_request', return_value=request_mock): + siteinfo = pywikibot.site.Siteinfo(site) + result = siteinfo._get_siteinfo('_prop', False) + self.assertEqual(result, {'_prop': ('_value', '_cache_time')}) + + class TestSiteinfoAsync(DefaultSiteTestCase):

"""Test asynchronous siteinfo fetch."""