Merge branch 'atomic-import' into 'develop'

Fixed broken import because of missing transaction

See merge request funkwhale/funkwhale!60

CHANGELOG

@@ -5,6 +5,7 @@ Changelog
 ----------------
 
 - Always use username in sidebar (#89)
+- Fixed broken import because of missing transaction
 
 
 0.5.2 (2018-02-26)

api/funkwhale_api/common/utils.py

@@ -1,6 +1,8 @@
 import os
 import shutil
 
+from django.db import transaction
+
 
 def rename_file(instance, field_name, new_name, allow_missing_file=False):
     field = getattr(instance, field_name)
@@ -17,3 +19,9 @@ def rename_file(instance, field_name, new_name, allow_missing_file=False):
     field.name = os.path.join(initial_path, new_name_with_extension)
     instance.save()
     return new_name_with_extension
+
+
+def on_commit(f, *args, **kwargs):
+    return transaction.on_commit(
+        lambda: f(*args, **kwargs)
+    )

api/funkwhale_api/music/tasks.py

@@ -73,7 +73,10 @@ def _do_import(import_job, replace):
 
 
 @celery.app.task(name='ImportJob.run', bind=True)
-@celery.require_instance(models.ImportJob, 'import_job')
+@celery.require_instance(
+    models.ImportJob.objects.filter(
+        status__in=['pending', 'errored']),
+    'import_job')
 def import_job_run(self, import_job, replace=False):
     def mark_errored():
         import_job.status = 'errored'

api/funkwhale_api/music/views.py

@@ -19,6 +19,7 @@ from musicbrainzngs import ResponseError
 from django.contrib.auth.decorators import login_required
 from django.utils.decorators import method_decorator
 
+from funkwhale_api.common import utils as funkwhale_utils
 from funkwhale_api.requests.models import ImportRequest
 from funkwhale_api.musicbrainz import api
 from funkwhale_api.common.permissions import (
@@ -116,7 +117,10 @@ class ImportJobViewSet(
     def perform_create(self, serializer):
         source = 'file://' + serializer.validated_data['audio_file'].name
         serializer.save(source=source)
-        tasks.import_job_run.delay(import_job_id=serializer.instance.pk)
+        funkwhale_utils.on_commit(
+            tasks.import_job_run.delay,
+            import_job_id=serializer.instance.pk
+        )
 
 
 class TrackViewSet(TagViewSetMixin, SearchMixin, viewsets.ReadOnlyModelViewSet):
@@ -336,6 +340,7 @@ class SubmitViewSet(viewsets.ViewSet):
             data, request, batch=None, import_request=import_request)
         return Response(import_data)
 
+    @transaction.atomic
     def _import_album(self, data, request, batch=None, import_request=None):
         # we import the whole album here to prevent race conditions that occurs
         # when using get_or_create_from_api in tasks
@@ -355,7 +360,11 @@ class SubmitViewSet(viewsets.ViewSet):
                 models.TrackFile.objects.get(track__mbid=row['mbid'])
             except models.TrackFile.DoesNotExist:
                 job = models.ImportJob.objects.create(mbid=row['mbid'], batch=batch, source=row['source'])
-                tasks.import_job_run.delay(import_job_id=job.pk)
+                funkwhale_utils.on_commit(
+                    tasks.import_job_run.delay,
+                    import_job_id=job.pk
+                )
+
         serializer = serializers.ImportBatchSerializer(batch)
         return serializer.data, batch
 

api/funkwhale_api/providers/audiofile/management/commands/import_files.py

@@ -3,6 +3,9 @@ import os
 
 from django.core.files import File
 from django.core.management.base import BaseCommand, CommandError
+from django.db import transaction
+
+from funkwhale_api.common import utils
 from funkwhale_api.music import tasks
 from funkwhale_api.users.models import User
 
@@ -86,6 +89,7 @@ class Command(BaseCommand):
         self.stdout.write(
             "For details, please refer to import batch #".format(batch.pk))
 
+    @transaction.atomic
     def do_import(self, matching, user, options):
         message = 'Importing {}...'
         if options['async']:
@@ -94,7 +98,7 @@ class Command(BaseCommand):
         # we create an import batch binded to the user
         batch = user.imports.create(source='shell')
         async = options['async']
-        handler = tasks.import_job_run.delay if async else tasks.import_job_run
+        import_handler = tasks.import_job_run.delay if async else tasks.import_job_run
         for path in matching:
             job = batch.jobs.create(
                 source='file://' + path,
@@ -105,7 +109,8 @@ class Command(BaseCommand):
 
             job.save()
             try:
-                handler(import_job_id=job.pk)
+                utils.on_commit(import_handler, import_job_id=job.pk)
             except Exception as e:
                 self.stdout.write('Error: {}'.format(e))
+
         return batch

api/tests/music/test_api.py

@@ -6,6 +6,7 @@ from django.urls import reverse
 from funkwhale_api.music import models
 from funkwhale_api.musicbrainz import api
 from funkwhale_api.music import serializers
+from funkwhale_api.music import tasks
 
 from . import data as api_data
 
@@ -208,7 +209,7 @@ def test_user_can_create_an_empty_batch(client, factories):
 
 def test_user_can_create_import_job_with_file(client, factories, mocker):
     path = os.path.join(DATA_DIR, 'test.ogg')
-    m = mocker.patch('funkwhale_api.music.tasks.import_job_run.delay')
+    m = mocker.patch('funkwhale_api.common.utils.on_commit')
     user = factories['users.SuperUser']()
     batch = factories['music.ImportBatch'](submitted_by=user)
     url = reverse('api:v1:import-jobs-list')
@@ -231,7 +232,9 @@ def test_user_can_create_import_job_with_file(client, factories, mocker):
     assert 'test.ogg' in job.source
     assert job.audio_file.read() == content
 
-    m.assert_called_once_with(import_job_id=job.pk)
+    m.assert_called_once_with(
+        tasks.import_job_run.delay,
+        import_job_id=job.pk)
 
 
 def test_can_search_artist(factories, client):

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 import tasks as music_tasks
 
 DATA_DIR = os.path.join(
     os.path.dirname(os.path.abspath(__file__)),
@@ -53,7 +54,7 @@ def test_management_command_requires_a_valid_username(factories, mocker):
 
 
 def test_import_files_creates_a_batch_and_job(factories, mocker):
-    m = mocker.patch('funkwhale_api.music.tasks.import_job_run.delay')
+    m = m = mocker.patch('funkwhale_api.common.utils.on_commit')
     user = factories['users.User'](username='me')
     path = os.path.join(DATA_DIR, 'dummy_file.ogg')
     call_command(
@@ -74,4 +75,6 @@ def test_import_files_creates_a_batch_and_job(factories, mocker):
         assert job.audio_file.read() == f.read()
 
     assert job.source == 'file://' + path
-    m.assert_called_once_with(import_job_id=job.pk)
+    m.assert_called_once_with(
+        music_tasks.import_job_run.delay,
+        import_job_id=job.pk)