From 579033d55b121c11f0e3cf424ca05ea52d172657 Mon Sep 17 00:00:00 2001 From: kernitus <2789734+kernitus@users.noreply.github.com> Date: Sun, 29 Oct 2023 22:20:10 +0000 Subject: [PATCH] Add retries on network error. Closes #9 --- beetsplug/oldestdate.py | 156 ++++++++++++++++++++++----------------- tests/test_oldestdate.py | 2 +- 2 files changed, 90 insertions(+), 68 deletions(-) diff --git a/beetsplug/oldestdate.py b/beetsplug/oldestdate.py index 8b27f54..4fb6db7 100644 --- a/beetsplug/oldestdate.py +++ b/beetsplug/oldestdate.py @@ -1,4 +1,5 @@ -from typing import Optional, Any, List, Dict +import time +from typing import Optional, Any, List, Dict, Callable, TypeVar import mediafile import musicbrainzngs from beets import ui, config @@ -6,6 +7,7 @@ from beets.importer import action, ImportTask, ImportSession from beets.library import Item, Library from beets.plugins import BeetsPlugin +from musicbrainzngs import NetworkError from .date_wrapper import DateWrapper @@ -19,65 +21,11 @@ Recording = Dict[str, Any] Work = Dict[str, Any] +MAX_RETRIES = 3 +T = TypeVar('T') -def _get_work_id_from_recording(recording: Recording) -> Optional[str]: - """Extract first valid work_id from recording""" - work_id = None - if 'work-relation-list' in recording: - for work_rel in recording['work-relation-list']: - if 'work' in work_rel: - current_work = work_rel['work'] - if 'id' in current_work: - work_id = current_work['id'] - break - - return work_id - - -def _contains_artist(recording: Recording, artist_ids: List[str]) -> bool: - """Returns whether this recording contains at least one of the specified artists""" - artist_found = False - if 'artist-credit' in recording: - for artist in recording['artist-credit']: - if 'artist' in artist: - artist = artist['artist'] - if 'id' in artist and artist['id'] in artist_ids: # Contains at least one of the identified artists - artist_found = True - break - return artist_found - - -def _get_artist_ids_from_recording(recording: Recording) -> List[str]: - """Extract artist ids from a recording""" - ids = [] - - if 'artist-credit' in recording: - for artist in recording['artist-credit']: - if 'artist' in artist: - artist = artist['artist'] - if 'id' in artist: - ids.append(artist['id']) - return ids - - -def _is_cover(recording: Recording) -> bool: - """Returns whether given fetched recording is a cover of a work""" - if 'work-relation-list' in recording: - for work in recording['work-relation-list']: - if 'attribute-list' in work: - if 'cover' in work['attribute-list']: - return True - return False - - -def _fetch_work(work_id: str) -> Dict[str, Any]: - """Fetch work, including recording relations""" - work: Work = musicbrainzngs.get_work_by_id(work_id, ['recording-rels'])['work'] - return work - - -class OldestDatePlugin(BeetsPlugin): # type: ignore +class OldestDatePlugin(BeetsPlugin): # type: ignore _importing: bool = False _recordings_cache: Dict[str, Recording] = dict() @@ -147,10 +95,10 @@ def track_distance(self, _: Item, info: TrackInfo) -> hooks.Distance: return dist - def _import_task_created(self, task: ImportTask, _: ImportSession) -> None: + def _import_task_created(self, task: ImportTask, session: ImportSession) -> None: task.item.mb_trackid = None - def _import_task_choice(self, task: ImportTask, _: ImportSession) -> None: + def _import_task_choice(self, task: ImportTask, session: ImportSession) -> None: match = task.match if not match: return @@ -180,10 +128,79 @@ def _import_task_choice(self, task: ImportTask, _: ImportSession) -> None: task.choice_flag = action.SKIP return + def _retry_on_network_error(self, func: Callable[..., T], *args: Any, **kwargs: Any) -> T: + for attempt in range(MAX_RETRIES): + try: + return func(*args, **kwargs) + except NetworkError: + if attempt < MAX_RETRIES - 1: # No need to wait after the last attempt + delay: int = 2 ** attempt + self._log.info(f'Network call failed, attempt {attempt}/{MAX_RETRIES}. Trying again in {delay}') + time.sleep(delay) # Exponential backoff each attempt + else: + raise + assert False, "Unreachable code" # To satisfy mypy; this will never actually be reached + + def _get_work_id_from_recording(self, recording: Recording) -> Optional[str]: + """Extract first valid work_id from recording""" + work_id = None + + if 'work-relation-list' in recording: + for work_rel in recording['work-relation-list']: + if 'work' in work_rel: + current_work = work_rel['work'] + if 'id' in current_work: + work_id = current_work['id'] + break + + return work_id + + def _contains_artist(self, recording: Recording, artist_ids: List[str]) -> bool: + """Returns whether this recording contains at least one of the specified artists""" + artist_found = False + if 'artist-credit' in recording: + for artist in recording['artist-credit']: + if 'artist' in artist: + artist = artist['artist'] + if 'id' in artist and artist['id'] in artist_ids: # Contains at least one of the identified artists + artist_found = True + break + return artist_found + + def _get_artist_ids_from_recording(self, recording: Recording) -> List[str]: + """Extract artist ids from a recording""" + ids = [] + + if 'artist-credit' in recording: + for artist in recording['artist-credit']: + if 'artist' in artist: + artist = artist['artist'] + if 'id' in artist: + ids.append(artist['id']) + return ids + + def _is_cover(self, recording: Recording) -> bool: + """Returns whether given fetched recording is a cover of a work""" + if 'work-relation-list' in recording: + for work in recording['work-relation-list']: + if 'attribute-list' in work: + if 'cover' in work['attribute-list']: + return True + return False + + def _fetch_work(self, work_id: str) -> Work: + """Fetch work, including recording relations""" + work: Work = self._retry_on_network_error( + musicbrainzngs.get_work_by_id, + work_id, + includes=['recording-rels'] + )['work'] + return work + def _has_work_id(self, recording_id: str) -> bool: """Return whether the recording has a work id""" recording = self._get_recording(recording_id) - work_id = _get_work_id_from_recording(recording) + work_id = self._get_work_id_from_recording(recording) return work_id is not None def _command_func(self, lib: Library, _: ImportSession, args: List[str]) -> None: @@ -242,7 +259,12 @@ def _process_file(self, item: Item) -> None: def _fetch_recording(self, recording_id: str) -> Recording: """Fetch and cache recording from MusicBrainz, including releases and work relations""" - recording: Recording = musicbrainzngs.get_recording_by_id(recording_id, ['artists', 'releases', 'work-rels'])['recording'] + recording: Recording = self._retry_on_network_error( + musicbrainzngs.get_recording_by_id, + recording_id, + includes=['artists', 'releases', 'work-rels'] + )['recording'] + self._recordings_cache[recording_id] = recording return recording @@ -308,7 +330,7 @@ def _extract_oldest_release_date(self, recordings: List[Recording], starting_dat else: # Filter by artist, but only if cover (to avoid not matching solo careers of former groups) fetched_recording = self._get_recording(rec_id) - if not _contains_artist(fetched_recording, artist_ids): + if not self._contains_artist(fetched_recording, artist_ids): self._recordings_cache.pop(rec_id, None) # Remove recording from cache continue elif 'attribute-list' in rec and (self.config['filter_recordings'] or 'cover' in rec['attribute-list']): @@ -354,9 +376,9 @@ def _iterate_dates(self, recordings: List[Recording], starting_date: DateWrapper def _get_oldest_date(self, recording_id: str, item_date: Optional[DateWrapper]) -> Optional[DateWrapper]: recording = self._get_recording(recording_id) - is_cover = _is_cover(recording) - work_id = _get_work_id_from_recording(recording) - artist_ids = _get_artist_ids_from_recording(recording) + is_cover = self._is_cover(recording) + work_id = self._get_work_id_from_recording(recording) + artist_ids = self._get_artist_ids_from_recording(recording) today = DateWrapper.today() @@ -368,7 +390,7 @@ def _get_oldest_date(self, recording_id: str, item_date: Optional[DateWrapper]) return self._iterate_dates([recording], starting_date, is_cover, artist_ids) # Fetch work, including associated recordings - work = _fetch_work(work_id) + work = self._fetch_work(work_id) if 'recording-relation-list' not in work: self._log.error( diff --git a/tests/test_oldestdate.py b/tests/test_oldestdate.py index f10d143..822def5 100644 --- a/tests/test_oldestdate.py +++ b/tests/test_oldestdate.py @@ -24,7 +24,7 @@ def setUp(self): def test_get_work_id_from_recording(self): test_recording = {"work-relation-list": [{"work": {"id": 20}}]} - result = oldestdate._get_work_id_from_recording(test_recording) + result = self.oldestdateplugin._get_work_id_from_recording(test_recording) self.assertEqual(20, result) def test_extract_oldest_recording_date(self):