From f98010ad23b2462a321f8a33c280a6fdc112b739 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 16:09:08 +0200 Subject: [PATCH 1/7] Add 'look before you leap' defensive code. --- beetsplug/beatport.py | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/beetsplug/beatport.py b/beetsplug/beatport.py index 8ef356fe8..7385684f4 100644 --- a/beetsplug/beatport.py +++ b/beetsplug/beatport.py @@ -150,9 +150,11 @@ class BeatportClient(object): :rtype: :py:class:`BeatportRelease` """ response = self._get('/catalog/3/releases', id=beatport_id) - release = BeatportRelease(response[0]) - release.tracks = self.get_release_tracks(beatport_id) - return release + if response: + release = BeatportRelease(response[0]) + release.tracks = self.get_release_tracks(beatport_id) + return release + return None def get_release_tracks(self, beatport_id): """ Get all tracks for a given release. @@ -261,10 +263,10 @@ class BeatportTrack(BeatportObject): self.musical_key = six.text_type(data['key'].get('shortName')) # Use 'subgenre' and if not present, 'genre' as a fallback. - if 'subGenres' in data: + if 'subGenres' in data and data['subGenres']: self.genre = six.text_type(data['subGenres'][0].get('name')) - if not self.genre and 'genres' in data: - self.genre = six.text_type(data['genres'][0].get('name')) + elif 'genres' in data and data['genres']: + self.genre = six.text_type(data['genres'][0].get('name')) class BeatportPlugin(BeetsPlugin): @@ -375,27 +377,33 @@ class BeatportPlugin(BeetsPlugin): def album_for_id(self, release_id): """Fetches a release by its Beatport ID and returns an AlbumInfo object - or None if the release is not found. + or None if the query is not a valid ID or release is not found. """ self._log.debug(u'Searching for release {0}', release_id) match = re.search(r'(^|beatport\.com/release/.+/)(\d+)$', release_id) if not match: + self._log.debug(u'Not a valid Beatport release ID.') return None release = self.client.get_release(match.group(2)) - album = self._get_album_info(release) - return album + if release is not None: + album = self._get_album_info(release) + return album + return None def track_for_id(self, track_id): """Fetches a track by its Beatport ID and returns a TrackInfo object - or None if the track is not found. + or None if the track is not a valid Beatport ID or track is not found. """ self._log.debug(u'Searching for track {0}', track_id) match = re.search(r'(^|beatport\.com/track/.+/)(\d+)$', track_id) if not match: + self._log.debug(u'Not a valid Beatport track ID.') return None bp_track = self.client.get_track(match.group(2)) - track = self._get_track_info(bp_track) - return track + if bp_track is not None: + track = self._get_track_info(bp_track) + return track + return None def _get_releases(self, query): """Returns a list of AlbumInfo objects for a beatport search query. From dde905f9a805b102d452cc84ec52f25208fdddb2 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 16:41:16 +0200 Subject: [PATCH 2/7] Correct typo in docstring. --- test/test_beatport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_beatport.py b/test/test_beatport.py index a1591d58c..823668c82 100644 --- a/test/test_beatport.py +++ b/test/test_beatport.py @@ -31,7 +31,7 @@ class BeatportTest(_common.TestCase, TestHelper): def _make_release_response(self): """Returns a dict that mimics a response from the beatport API. - The results were retrived from: + The results were retrieved from: https://oauth-api.beatport.com/catalog/3/releases?id=1742984 The list of elements on the returned dict is incomplete, including just those required for the tests on this class. @@ -72,7 +72,7 @@ class BeatportTest(_common.TestCase, TestHelper): def _make_tracks_response(self): """Return a list that mimics a response from the beatport API. - The results were retrived from: + The results were retrieved from: https://oauth-api.beatport.com/catalog/3/tracks?releaseId=1742984 The list of elements on the returned list is incomplete, including just those required for the tests on this class. From 2691781d4e53909c7bb993d16b4ede972f218e95 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 16:41:57 +0200 Subject: [PATCH 3/7] Remove forgotten print statement. --- test/test_beatport.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test_beatport.py b/test/test_beatport.py index 823668c82..4589a9eba 100644 --- a/test/test_beatport.py +++ b/test/test_beatport.py @@ -425,7 +425,6 @@ class BeatportTest(_common.TestCase, TestHelper): # Set up 'test_album'. self.test_album = self.mk_test_album() - # print(self.test_album.keys()) # Set up 'test_tracks' self.test_tracks = self.test_album.items() From 486cfa1df1a149689017cb4d2646894ad0ce3c04 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 16:54:47 +0200 Subject: [PATCH 4/7] Add tests for empty responses. --- test/test_beatport.py | 64 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/test/test_beatport.py b/test/test_beatport.py index 4589a9eba..8e830df76 100644 --- a/test/test_beatport.py +++ b/test/test_beatport.py @@ -558,6 +558,70 @@ class BeatportTest(_common.TestCase, TestHelper): self.assertEqual(track.genre, test_track.genre) +class BeatportResponseEmptyTest(_common.TestCase, TestHelper): + def _make_tracks_response(self): + results = [{ + "id": 7817567, + "name": "Mirage a Trois", + "genres": [{ + "id": 9, + "name": "Breaks", + "slug": "breaks", + "type": "genre" + }], + "subGenres": [{ + "id": 209, + "name": "Glitch Hop", + "slug": "glitch-hop", + "type": "subgenre" + }], + }] + return results + + def setUp(self): + self.setup_beets() + self.load_plugins('beatport') + self.lib = library.Library(':memory:') + + # Set up 'tracks'. + self.response_tracks = self._make_tracks_response() + self.tracks = [beatport.BeatportTrack(t) for t in self.response_tracks] + + # Make alias to be congruent with class `BeatportTest`. + self.test_tracks = self.response_tracks + + def tearDown(self): + self.unload_plugins() + self.teardown_beets() + + def test_response_tracks_empty(self): + response_tracks = [] + tracks = [beatport.BeatportTrack(t) for t in response_tracks] + self.assertEqual(tracks, []) + + def test_sub_genre_empty_fallback(self): + """No 'sub_genre' is provided. Test if fallback to 'genre' works. + """ + self.response_tracks[0]['subGenres'] = [] + tracks = [beatport.BeatportTrack(t) for t in self.response_tracks] + + self.test_tracks[0]['subGenres'] = [] + + self.assertEqual(tracks[0].genre, + self.test_tracks[0]['genres'][0]['name']) + + def test_genre_empty(self): + """No 'genre' is provided. Test if 'sub_genre' is applied. + """ + self.response_tracks[0]['genres'] = [] + tracks = [beatport.BeatportTrack(t) for t in self.response_tracks] + + self.test_tracks[0]['genres'] = [] + + self.assertEqual(tracks[0].genre, + self.test_tracks[0]['subGenres'][0]['name']) + + def suite(): return unittest.TestLoader().loadTestsFromName(__name__) From 2cd38fc7df178d37276d24aa5f9acaa027fcddf0 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 17:12:46 +0200 Subject: [PATCH 5/7] Add bugfix. --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index aa87929c6..261b74505 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -107,6 +107,8 @@ Fixes: * ``none_rec_action`` does not import automatically when ``timid`` is enabled. Thanks to :user:`RollingStar`. :bug:`3242` +* Fix a bug that caused a crash when tagging items with the beatport plugin. + :bug:`3374` For plugin developers: From 9a4175dcd05b4565addf614ab2e69044fffd9960 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 21:13:41 +0200 Subject: [PATCH 6/7] Return value directly. --- beetsplug/beatport.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/beetsplug/beatport.py b/beetsplug/beatport.py index 7385684f4..1f2fbe451 100644 --- a/beetsplug/beatport.py +++ b/beetsplug/beatport.py @@ -385,9 +385,8 @@ class BeatportPlugin(BeetsPlugin): self._log.debug(u'Not a valid Beatport release ID.') return None release = self.client.get_release(match.group(2)) - if release is not None: - album = self._get_album_info(release) - return album + if release: + return self._get_album_info(release) return None def track_for_id(self, track_id): @@ -401,8 +400,7 @@ class BeatportPlugin(BeetsPlugin): return None bp_track = self.client.get_track(match.group(2)) if bp_track is not None: - track = self._get_track_info(bp_track) - return track + return self._get_track_info(bp_track) return None def _get_releases(self, query): From dfb6fc3f5bfa4d471ccdcf18d1ad0f6f568be194 Mon Sep 17 00:00:00 2001 From: temrix Date: Sat, 21 Sep 2019 21:14:11 +0200 Subject: [PATCH 7/7] Test for presence and non-emptiness in one go. --- beetsplug/beatport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beetsplug/beatport.py b/beetsplug/beatport.py index 1f2fbe451..c3933ed73 100644 --- a/beetsplug/beatport.py +++ b/beetsplug/beatport.py @@ -263,9 +263,9 @@ class BeatportTrack(BeatportObject): self.musical_key = six.text_type(data['key'].get('shortName')) # Use 'subgenre' and if not present, 'genre' as a fallback. - if 'subGenres' in data and data['subGenres']: + if data.get('subGenres'): self.genre = six.text_type(data['subGenres'][0].get('name')) - elif 'genres' in data and data['genres']: + elif data.get('genres'): self.genre = six.text_type(data['genres'][0].get('name'))