From 61cf04b3764522ae24cf0104e7ac7965687c5a4f Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Wed, 29 Jan 2020 15:33:50 +0100 Subject: [PATCH 1/2] Fix #348, #474, #557, #740, #928: improved deduplication logic to prevent skipped files during import --- .../migrations/0050_auto_20200129_1344.py | 18 +++ api/funkwhale_api/music/models.py | 1 + api/funkwhale_api/music/tasks.py | 13 +- api/tests/music/test_tasks.py | 131 ++++++++++++++++++ changes/changelog.d/348.bugfix | 1 + 5 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 api/funkwhale_api/music/migrations/0050_auto_20200129_1344.py create mode 100644 changes/changelog.d/348.bugfix diff --git a/api/funkwhale_api/music/migrations/0050_auto_20200129_1344.py b/api/funkwhale_api/music/migrations/0050_auto_20200129_1344.py new file mode 100644 index 000000000..ecd74003d --- /dev/null +++ b/api/funkwhale_api/music/migrations/0050_auto_20200129_1344.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.9 on 2020-01-29 13:44 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('music', '0049_auto_20200122_1020'), + ] + + operations = [ + migrations.AlterField( + model_name='track', + name='mbid', + field=models.UUIDField(blank=True, db_index=True, null=True), + ), + ] diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index e1852870e..1d156a6c6 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -475,6 +475,7 @@ def get_artist(release_list): class Track(APIModelMixin): + mbid = models.UUIDField(db_index=True, null=True, blank=True) title = models.CharField(max_length=MAX_LENGTHS["TRACK_TITLE"]) artist = models.ForeignKey(Artist, related_name="tracks", on_delete=models.CASCADE) disc_number = models.PositiveIntegerField(null=True, blank=True) diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 3e95e5325..636a54cf4 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -625,9 +625,18 @@ def _get_track(data, attributed_to=None, **forced_values): else truncate(data.get("copyright"), models.MAX_LENGTHS["COPYRIGHT"]) ) - query = Q(title__iexact=track_title, artist=artist, album=album, position=position) + query = Q( + title__iexact=track_title, + artist=artist, + album=album, + position=position, + disc_number=disc_number, + ) if track_mbid: - query |= Q(mbid=track_mbid) + if album_mbid: + query |= Q(mbid=track_mbid, album__mbid=album_mbid) + else: + query |= Q(mbid=track_mbid) if track_fid: query |= Q(fid=track_fid) diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 053f62ede..bbbc9c745 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -1164,3 +1164,134 @@ def test_can_download_image_file_for_album_mbid(binary_cover, mocker, factories) assert album.attachment_cover.file.read() == binary_cover assert album.attachment_cover.mimetype == "image/jpeg" + + +def test_can_import_track_with_same_mbid_in_different_albums(factories, mocker): + artist = factories["music.Artist"]() + upload = factories["music.Upload"]( + playable=True, track__artist=artist, track__album__artist=artist + ) + assert upload.track.mbid is not None + data = { + "title": upload.track.title, + "artists": [{"name": artist.name, "mbid": artist.mbid}], + "album": { + "title": "The Slip", + "mbid": uuid.UUID("12b57d46-a192-499e-a91f-7da66790a1c1"), + "release_date": datetime.date(2008, 5, 5), + "artists": [{"name": artist.name, "mbid": artist.mbid}], + }, + "position": 1, + "disc_number": 1, + "mbid": upload.track.mbid, + } + + mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data) + mocker.patch.object(tasks, "populate_album_cover") + + new_upload = factories["music.Upload"](library=upload.library) + + tasks.process_upload(upload_id=new_upload.pk) + + new_upload.refresh_from_db() + + assert new_upload.import_status == "finished" + + +def test_import_track_with_same_mbid_in_same_albums_skipped(factories, mocker): + artist = factories["music.Artist"]() + upload = factories["music.Upload"]( + playable=True, track__artist=artist, track__album__artist=artist + ) + assert upload.track.mbid is not None + data = { + "title": upload.track.title, + "artists": [{"name": artist.name, "mbid": artist.mbid}], + "album": { + "title": upload.track.album.title, + "mbid": upload.track.album.mbid, + "artists": [{"name": artist.name, "mbid": artist.mbid}], + }, + "position": 1, + "disc_number": 1, + "mbid": upload.track.mbid, + } + + mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data) + mocker.patch.object(tasks, "populate_album_cover") + + new_upload = factories["music.Upload"](library=upload.library) + + tasks.process_upload(upload_id=new_upload.pk) + + new_upload.refresh_from_db() + + assert new_upload.import_status == "skipped" + + +def test_can_import_track_with_same_position_in_different_discs(factories, mocker): + upload = factories["music.Upload"](playable=True) + artist_data = [ + { + "name": upload.track.album.artist.name, + "mbid": upload.track.album.artist.mbid, + } + ] + data = { + "title": upload.track.title, + "artists": artist_data, + "album": { + "title": "The Slip", + "mbid": upload.track.album.mbid, + "release_date": datetime.date(2008, 5, 5), + "artists": artist_data, + }, + "position": upload.track.position, + "disc_number": 2, + "mbid": None, + } + + mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data) + mocker.patch.object(tasks, "populate_album_cover") + + new_upload = factories["music.Upload"](library=upload.library) + + tasks.process_upload(upload_id=new_upload.pk) + + new_upload.refresh_from_db() + + assert new_upload.import_status == "finished" + + +def test_can_import_track_with_same_position_in_same_discs_skipped(factories, mocker): + upload = factories["music.Upload"](playable=True) + artist_data = [ + { + "name": upload.track.album.artist.name, + "mbid": upload.track.album.artist.mbid, + } + ] + data = { + "title": upload.track.title, + "artists": artist_data, + "album": { + "title": "The Slip", + "mbid": upload.track.album.mbid, + "release_date": datetime.date(2008, 5, 5), + "artists": artist_data, + }, + "position": upload.track.position, + "disc_number": upload.track.disc_number, + "mbid": None, + } + + mocker.patch.object(metadata.TrackMetadataSerializer, "validated_data", data) + mocker.patch.object(tasks, "populate_album_cover") + + new_upload = factories["music.Upload"](library=upload.library) + + tasks.process_upload(upload_id=new_upload.pk) + + new_upload.refresh_from_db() + + assert new_upload.import_status == "skipped" diff --git a/changes/changelog.d/348.bugfix b/changes/changelog.d/348.bugfix new file mode 100644 index 000000000..10c04b982 --- /dev/null +++ b/changes/changelog.d/348.bugfix @@ -0,0 +1 @@ +Improved deduplication logic to prevent skipped files during import (#348, #474, #557, #740, #928) From 48178c4167fd656f6c46e1be28094692dc58cd23 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Wed, 29 Jan 2020 18:19:11 +0100 Subject: [PATCH 2/2] Fixed broken cover rendering --- front/src/views/admin/library/ArtistDetail.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/front/src/views/admin/library/ArtistDetail.vue b/front/src/views/admin/library/ArtistDetail.vue index 0104431ea..3ee193e35 100644 --- a/front/src/views/admin/library/ArtistDetail.vue +++ b/front/src/views/admin/library/ArtistDetail.vue @@ -9,7 +9,7 @@

- +
{{ object.name | truncate(100) }}