From bb79d454af800f0cfa6f978cdb48001d078ecfe4 Mon Sep 17 00:00:00 2001 From: RenonDis Date: Tue, 19 Jun 2018 17:57:33 +0200 Subject: [PATCH 1/5] Bypassing cli skip for update flag --- api/funkwhale_api/music/models.py | 1 + .../audiofile/management/commands/import_files.py | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 8b638ce7d..16ac58dc9 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -567,6 +567,7 @@ class ImportBatch(models.Model): class ImportJob(models.Model): uuid = models.UUIDField(unique=True, db_index=True, default=uuid.uuid4) + update_if_duplicate = models.BooleanField(default=False) batch = models.ForeignKey( ImportBatch, related_name="jobs", on_delete=models.CASCADE ) diff --git a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py index 8de779230..2d1ddba1f 100644 --- a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py +++ b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py @@ -55,6 +55,16 @@ class Command(BaseCommand): "import and not much disk space available." ), ) + parser.add_argument( + "--update", + action="store_true", + dest="update", + default=False, + help=( + "Use this flag to replace duplicates (tracks with same " + "musicbrainz mbid) on import with their newest version." + ), + ) parser.add_argument( "--noinput", "--no-input", From 8103ea541f1463f857c374f9967c368e10f57184 Mon Sep 17 00:00:00 2001 From: RenonDis Date: Fri, 22 Jun 2018 11:10:23 +0200 Subject: [PATCH 2/5] cli import files with replace option --- api/funkwhale_api/music/models.py | 2 +- api/funkwhale_api/music/tasks.py | 9 ++++--- .../management/commands/import_files.py | 26 ++++++++++++------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 16ac58dc9..b75a51283 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -567,7 +567,7 @@ class ImportBatch(models.Model): class ImportJob(models.Model): uuid = models.UUIDField(unique=True, db_index=True, default=uuid.uuid4) - update_if_duplicate = models.BooleanField(default=False) + replace_if_duplicate = models.BooleanField(default=False) batch = models.ForeignKey( ImportBatch, related_name="jobs", on_delete=models.CASCADE ) diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 355af7706..398043a39 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -80,10 +80,11 @@ def import_track_from_remote(library_track): )[0] -def _do_import(import_job, replace=False, use_acoustid=False): +def _do_import(import_job, use_acoustid=False): logger.info("[Import Job %s] starting job", import_job.pk) from_file = bool(import_job.audio_file) mbid = import_job.mbid + replace = import_job.replace_if_duplicate acoustid_track_id = None duration = None track = None @@ -163,7 +164,7 @@ def _do_import(import_job, replace=False, use_acoustid=False): # no downloading, we hotlink pass elif not import_job.audio_file and not import_job.source.startswith("file://"): - # not an implace import, and we have a source, so let's download it + # not an inplace import, and we have a source, so let's download it logger.info("[Import Job %s] downloading audio file from remote", import_job.pk) track_file.download_file() elif not import_job.audio_file and import_job.source.startswith("file://"): @@ -243,14 +244,14 @@ def get_cover_from_fs(dir_path): @celery.require_instance( models.ImportJob.objects.filter(status__in=["pending", "errored"]), "import_job" ) -def import_job_run(self, import_job, replace=False, use_acoustid=False): +def import_job_run(self, import_job, use_acoustid=False): def mark_errored(exc): logger.error("[Import Job %s] Error during import: %s", import_job.pk, str(exc)) import_job.status = "errored" import_job.save(update_fields=["status"]) try: - tf = _do_import(import_job, replace, use_acoustid=use_acoustid) + tf = _do_import(import_job, use_acoustid=use_acoustid) return tf.pk if tf else None except Exception as exc: if not settings.DEBUG: diff --git a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py index 2d1ddba1f..c3fabd7f6 100644 --- a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py +++ b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py @@ -56,13 +56,14 @@ class Command(BaseCommand): ), ) parser.add_argument( - "--update", + "--replace", action="store_true", - dest="update", + dest="replace", default=False, help=( "Use this flag to replace duplicates (tracks with same " - "musicbrainz mbid) on import with their newest version." + "musicbrainz mbid, or same artist, album and title) on import " + "with their newest version." ), ) parser.add_argument( @@ -122,16 +123,23 @@ class Command(BaseCommand): "No superuser available, please provide a --username" ) - filtered = self.filter_matching(matching) + if options["replace"]: + filtered = {"initial": matching, "skipped": [], "new": matching} + message = "- {} files to be replaced" + import_paths = matching + else: + filtered = self.filter_matching(matching) + message = "- {} files already found in database" + import_paths = filtered["new"] + self.stdout.write("Import summary:") self.stdout.write( "- {} files found matching this pattern: {}".format( len(matching), options["path"] ) ) - self.stdout.write( - "- {} files already found in database".format(len(filtered["skipped"])) - ) + self.stdout.write(message.format(len(filtered["skipped"]))) + self.stdout.write("- {} new files".format(len(filtered["new"]))) self.stdout.write( @@ -151,12 +159,12 @@ class Command(BaseCommand): if input("".join(message)) != "yes": raise CommandError("Import cancelled.") - batch, errors = self.do_import(filtered["new"], user=user, options=options) + batch, errors = self.do_import(import_paths, user=user, options=options) message = "Successfully imported {} tracks" if options["async"]: message = "Successfully launched import for {} tracks" - self.stdout.write(message.format(len(filtered["new"]))) + self.stdout.write(message.format(len(import_paths))) if len(errors) > 0: self.stderr.write("{} tracks could not be imported:".format(len(errors))) From f3f07c1f8d752b3406025f7460e680dca2befd38 Mon Sep 17 00:00:00 2001 From: RenonDis Date: Fri, 22 Jun 2018 15:13:48 +0200 Subject: [PATCH 3/5] Updated tasks.py to process replace flag --- api/funkwhale_api/music/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 398043a39..2092b6ee7 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -136,8 +136,8 @@ def _do_import(import_job, use_acoustid=False): track_file = None if replace: - logger.info("[Import Job %s] replacing existing audio file", import_job.pk) - track_file = track.files.first() + logger.info("[Import Job %s] deleting existing audio file", import_job.pk) + track.files.all().delete() elif track.files.count() > 0: logger.info( "[Import Job %s] skipping, we already have a file for this track", From 8d9499332fdd4ab68007ca4a92a4699288e492e6 Mon Sep 17 00:00:00 2001 From: RenonDis Date: Fri, 22 Jun 2018 15:54:55 +0200 Subject: [PATCH 4/5] Migration file for ImportJob.replace_if_duplicate --- .../0028_importjob_replace_if_duplicate.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 api/funkwhale_api/music/migrations/0028_importjob_replace_if_duplicate.py diff --git a/api/funkwhale_api/music/migrations/0028_importjob_replace_if_duplicate.py b/api/funkwhale_api/music/migrations/0028_importjob_replace_if_duplicate.py new file mode 100644 index 000000000..d02a17ad2 --- /dev/null +++ b/api/funkwhale_api/music/migrations/0028_importjob_replace_if_duplicate.py @@ -0,0 +1,18 @@ +# Generated by Django 2.0.6 on 2018-06-22 13:36 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('music', '0027_auto_20180515_1808'), + ] + + operations = [ + migrations.AddField( + model_name='importjob', + name='replace_if_duplicate', + field=models.BooleanField(default=False), + ), + ] From 61eb8e4d61450dc7b1c164645402684fb17aa990 Mon Sep 17 00:00:00 2001 From: RenonDis Date: Fri, 22 Jun 2018 17:12:54 +0200 Subject: [PATCH 5/5] Test for _do_import with replace --- api/funkwhale_api/music/factories.py | 1 + .../management/commands/import_files.py | 4 +++- api/tests/music/test_tasks.py | 23 ++++++++++++++++--- api/tests/test_import_audio_file.py | 14 +++++++++++ changes/changelog.d/222.feature | 1 + 5 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 changes/changelog.d/222.feature diff --git a/api/funkwhale_api/music/factories.py b/api/funkwhale_api/music/factories.py index 2dd4ba303..c8f4bf322 100644 --- a/api/funkwhale_api/music/factories.py +++ b/api/funkwhale_api/music/factories.py @@ -89,6 +89,7 @@ class ImportJobFactory(factory.django.DjangoModelFactory): batch = factory.SubFactory(ImportBatchFactory) source = factory.Faker("url") mbid = factory.Faker("uuid4") + replace_if_duplicate = factory.Faker("boolean") class Meta: model = "music.ImportJob" diff --git a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py index c3fabd7f6..b59c0046f 100644 --- a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py +++ b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py @@ -214,7 +214,9 @@ class Command(BaseCommand): return batch, errors def import_file(self, path, batch, import_handler, options): - job = batch.jobs.create(source="file://" + path) + job = batch.jobs.create( + source="file://" + path, replace_if_duplicate=options["replace"] + ) if not options["in_place"]: name = os.path.basename(path) with open(path, "rb") as f: diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 71d605b2b..e91594d47 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -118,7 +118,7 @@ def test_run_import_skipping_accoustid(factories, mocker): path = os.path.join(DATA_DIR, "test.ogg") job = factories["music.FileImportJob"](audio_file__path=path) tasks.import_job_run(import_job_id=job.pk, use_acoustid=False) - m.assert_called_once_with(job, False, use_acoustid=False) + m.assert_called_once_with(job, use_acoustid=False) def test__do_import_skipping_accoustid(factories, mocker): @@ -130,7 +130,7 @@ def test__do_import_skipping_accoustid(factories, mocker): path = os.path.join(DATA_DIR, "test.ogg") job = factories["music.FileImportJob"](mbid=None, audio_file__path=path) p = job.audio_file.path - tasks._do_import(job, replace=False, use_acoustid=False) + tasks._do_import(job, use_acoustid=False) m.assert_called_once_with(p) @@ -144,10 +144,27 @@ def test__do_import_skipping_accoustid_if_no_key(factories, mocker, preferences) path = os.path.join(DATA_DIR, "test.ogg") job = factories["music.FileImportJob"](mbid=None, audio_file__path=path) p = job.audio_file.path - tasks._do_import(job, replace=False, use_acoustid=False) + tasks._do_import(job, use_acoustid=False) m.assert_called_once_with(p) +def test__do_import_replace_if_duplicate(factories, mocker): + existing_file = factories["music.TrackFile"]() + existing_track = existing_file.track + path = os.path.join(DATA_DIR, "test.ogg") + mocker.patch( + "funkwhale_api.providers.audiofile.tasks.import_track_data_from_path", + return_value=existing_track, + ) + job = factories["music.FileImportJob"]( + replace_if_duplicate=True, audio_file__path=path + ) + tasks._do_import(job) + with pytest.raises(existing_file.__class__.DoesNotExist): + existing_file.refresh_from_db() + assert existing_file.creation_date != job.track_file.creation_date + + def test_import_job_skip_if_already_exists(artists, albums, tracks, factories, mocker): path = os.path.join(DATA_DIR, "test.ogg") mbid = "9968a9d6-8d92-4051-8f76-674e157b6eed" diff --git a/api/tests/test_import_audio_file.py b/api/tests/test_import_audio_file.py index b5ed01e3c..43e596ff7 100644 --- a/api/tests/test_import_audio_file.py +++ b/api/tests/test_import_audio_file.py @@ -6,6 +6,7 @@ from django.core.management import call_command from django.core.management.base import CommandError from funkwhale_api.providers.audiofile import tasks +from funkwhale_api.music.models import ImportJob DATA_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "files") @@ -115,6 +116,19 @@ def test_import_with_multiple_argument(factories, mocker): mocked_filter.assert_called_once_with([path1, path2]) +def test_import_with_replace_flag(factories, mocker): + factories["users.User"](username="me") + path = os.path.join(DATA_DIR, "dummy_file.ogg") + mocked_job_run = mocker.patch("funkwhale_api.music.tasks.import_job_run") + call_command("import_files", path, username="me", replace=True, interactive=False) + created_job = ImportJob.objects.latest("id") + + assert created_job.replace_if_duplicate is True + mocked_job_run.assert_called_once_with( + import_job_id=created_job.id, use_acoustid=False + ) + + def test_import_files_creates_a_batch_and_job(factories, mocker): m = mocker.patch("funkwhale_api.music.tasks.import_job_run") user = factories["users.User"](username="me") diff --git a/changes/changelog.d/222.feature b/changes/changelog.d/222.feature new file mode 100644 index 000000000..ab10749f9 --- /dev/null +++ b/changes/changelog.d/222.feature @@ -0,0 +1 @@ +Added replace flag during import to replace already present tracks with a new version of their track file (#222)