jenkins-bot has submitted this change. ( https://gerrit.wikimedia.org/r/c/pywikibot/core/+/965243 )

Change subject: [IMPR] Improve handling of uninitialized MediaInfo ......................................................................

[IMPR] Improve handling of uninitialized MediaInfo

Make it possible to retrieve just the id for non-existing mediainfo's of existing files, and use this as an internal assertion. It will also throw an error for invalid mediainfo references.

Drop MediaInfo.get_data_for_new_entity because it will not be called anymore.

Change-Id: I7ad7396708e9fc7fcd98274c3668f07e25a1be30 --- M pywikibot/page/_wikibase.py M tests/file_tests.py 2 files changed, 101 insertions(+), 38 deletions(-)

Approvals: Zache-tool: Looks good to me, but someone else must approve Xqt: Looks good to me, approved jenkins-bot: Verified

diff --git a/pywikibot/page/_wikibase.py b/pywikibot/page/_wikibase.py index 753dc3f..bbfd71f 100644 --- a/pywikibot/page/_wikibase.py +++ b/pywikibot/page/_wikibase.py @@ -130,14 +130,17 @@ def __getattr__(self, name): if name in self.DATA_ATTRIBUTES: if self.getID() == '-1': - for key, cls in self.DATA_ATTRIBUTES.items(): - setattr(self, key, cls.new_empty(self.repo)) + self._initialize_empty() return getattr(self, name) return self.get()[name]

raise AttributeError("'{}' object has no attribute '{}'" .format(self.__class__.__name__, name))

+ def _initialize_empty(self): + for key, cls in self.DATA_ATTRIBUTES.items(): + setattr(self, key, cls.new_empty(self.repo)) + def _defined_by(self, singular: bool = False) -> dict: """ Internal function to provide the API parameters to identify the entity. @@ -355,7 +358,7 @@ """ Return the full concept URI.

- :raise NoWikibaseEntityError: if this entity doesn't exist + :raise NoWikibaseEntityError: if this entity's id is not known """ entity_id = self.getID() if entity_id == '-1': @@ -370,6 +373,7 @@ .. versionadded:: 6.5 """

+ entity_type = 'mediainfo' title_pattern = r'M[1-9]\d*' DATA_ATTRIBUTES = { 'labels': LanguageDict, @@ -378,12 +382,39 @@

def __getattr__(self, name): if name == 'claims': # T149410 - name = 'statements' - if hasattr(self, name): - return getattr(self, name) + return self.statements + + if name in self.DATA_ATTRIBUTES: + if not self.exists(): + self._assert_has_id() + self._initialize_empty() + return getattr(self, name)

return super().__getattr__(name)

+ def _assert_has_id(self): + if self.id != '-1': + return + + if not self.file.exists(): + exc = NoPageError(self.file) + raise NoWikibaseEntityError(self) from exc + + self.id = 'M' + str(self.file.pageid) + + def _defined_by(self, singular: bool = False) -> dict: + """ + Internal function to provide the API parameters to identify the entity. + + :param singular: Whether the parameter names should use the singular + form + :raise NoWikibaseEntityError: if this entity is associated with + a non-existing file + :return: API parameters + """ + self._assert_has_id() + return super()._defined_by(singular) + @property def file(self) -> FilePage: """Get the file associated with the mediainfo.""" @@ -397,7 +428,8 @@ pywikibot.error(msg) raise Error(msg)

- page_id = self.getID(numeric=True) + # avoid recursion with self.getID() + page_id = int(self.id[1:]) result = list(self.repo.load_pages_from_pageids([page_id])) if not result: raise Error(f'There is no existing page with id "{page_id}"') @@ -410,27 +442,24 @@

return self._file

- def get_data_for_new_entity(self) -> dict: - """Return data required for creation of a new mediainfo.""" - self.id = 'M' + str(self.file.pageid) - self._content = {} - return super().get() - def get(self, force: bool = False) -> dict: """Fetch all MediaInfo entity data and cache it.

+ .. note:: This method may raise exception even if the associated file + exists because the mediainfo may not have been initialized yet. + :attr:`labels` and :attr:`statements` can still be accessed and + modified. :meth:`exists` suppresses the exception. + + .. note:: dicts returned by this method are references to content + of this entity and their modifying may indirectly cause + unwanted change to the live content + :param force: override caching :raise NoWikibaseEntityError: if this entity doesn't exist :return: actual data which entity holds """ if self.id == '-1': - if force: - if not self.file.exists(): - exc = NoPageError(self.file) - raise NoWikibaseEntityError(self) from exc - # get just the id for Wikibase API call - self.id = 'M' + str(self.file.pageid) - else: + if not force: try: data = self.file.latest_revision.slots['mediainfo']['*'] except NoPageError as exc: @@ -443,6 +472,8 @@ self._content = jsonlib.loads(data) self.id = self._content['id']

+ self._assert_has_id() + return super().get(force=force)

def getID(self, numeric: bool = False): @@ -450,9 +481,10 @@ Get the entity identifier.

:param numeric: Strip the first letter and return an int + :raise NoWikibaseEntityError: if this entity is associated with + a non-existing file """ - if self.id == '-1': - self.get() + self._assert_has_id() return super().getID(numeric=numeric)

diff --git a/tests/file_tests.py b/tests/file_tests.py index 911c595..8a252c4 100755 --- a/tests/file_tests.py +++ b/tests/file_tests.py @@ -13,6 +13,7 @@

import pywikibot from pywikibot.exceptions import ( + Error, NoPageError, NoWikibaseEntityError, PageRelatedError, @@ -351,7 +352,7 @@ self.assertTrue(item.file is page) self.assertEqual('-1', item.id) item.get() - self.assertEqual('M14634781', item.getID()) + self.assertEqual('M14634781', item.id) self.assertIsInstance( item.labels, pywikibot.page._collections.LanguageDict) self.assertIsInstance( @@ -371,6 +372,15 @@ del item._file self.assertEqual(page, item.file)

+ def test_data_item_not_file(self): + """Test data item with invalid pageid.""" + item = pywikibot.MediaInfo(self.site, 'M1') # Main Page + with self.assertRaises(Error): + item.file + with self.assertRaises(NoWikibaseEntityError): + item.get() + self.assertFalse(item.exists()) + def test_data_item_when_no_file_or_data_item(self): """Test data item associated to file that does not exist.""" page = pywikibot.FilePage(self.site, @@ -381,6 +391,8 @@

with self.assertRaises(NoWikibaseEntityError): item.get() + with self.assertRaises(NoWikibaseEntityError): + item.labels

def test_data_item_when_file_exist_but_without_item(self): """Test if data item is missing from file.""" @@ -394,24 +406,26 @@

# Seek to first page without mediainfo. for page in gen: - if 'mediainfo' not in page.latest_revision.slots: - item = page.data_item() - self.assertIsInstance(item, pywikibot.MediaInfo) + if 'mediainfo' in page.latest_revision.slots: + continue

- # Get fails as there is no mediainfo. - with self.assertRaises(NoWikibaseEntityError): - item.get() + item = page.data_item() + self.assertIsInstance(item, pywikibot.MediaInfo)

- # Create new empty mediainfo. - item.get_data_for_new_entity() - self.assertIsInstance( - item.labels, pywikibot.page._collections.LanguageDict) - self.assertIsInstance( - item.statements, - pywikibot.page._collections.ClaimCollection) + # Get fails as there is no mediainfo. + with self.assertRaises(NoWikibaseEntityError): + item.get()

- # break the loop after checking first file - break + self.assertFalse(item.exists()) + self.assertEqual(f'M{page.pageid}', item.id) + self.assertIsInstance( + item.labels, pywikibot.page._collections.LanguageDict) + self.assertIsInstance( + item.statements, + pywikibot.page._collections.ClaimCollection) + + # break the loop after checking first file + break

def test_data_list_to_dict_workaround(self): """Test that T222159 workaround converts [] to {}."""