Backfill legacy sound filenames with a resumable command (#991)

src/core/management/commands/backfill_sound_filenames.py

@@ -0,0 +1,125 @@
+"""Move sound files stored under their uploader's filename to generated names.
+
+See #991. New uploads already land at `sounds/<uuid>.<ext>` (#982); this walks
+the rows that predate that and brings them into line.
+"""
+
+import logging
+import re
+
+from django.core.files.base import ContentFile
+from django.core.management.base import BaseCommand
+
+from core.models import Sound, sound_file_path
+
+log = logging.getLogger(__name__)
+
+# What `sound_file_path` produces: sounds/<32 hex chars>[.ext]
+GENERATED_NAME = re.compile(r"^sounds/[0-9a-f]{32}(\.[A-Za-z0-9]+)?$")
+
+
+class Command(BaseCommand):
+    help = (
+        "Rename existing Sound files from the uploader's filename to a "
+        "generated one. Safe to re-run: rows already renamed are skipped, so "
+        "an interrupted run resumes by running it again."
+    )
+
+    def add_arguments(self, parser):
+        parser.add_argument(
+            "--dry-run",
+            action="store_true",
+            help="Report what would change without touching storage or the database.",
+        )
+        parser.add_argument(
+            "--limit",
+            type=int,
+            default=None,
+            help="Process at most this many rows, so a first batch can be small.",
+        )
+
+    def handle(self, *args, **options):
+        dry_run = options["dry_run"]
+        limit = options["limit"]
+
+        pending = [
+            sound
+            for sound in Sound.objects.exclude(file="").order_by("pk")
+            if not GENERATED_NAME.match(sound.file.name or "")
+        ]
+        if limit is not None:
+            pending = pending[:limit]
+
+        if not pending:
+            self.stdout.write(
+                self.style.SUCCESS(
+                    "Nothing to do: every sound file already has a generated name."
+                )
+            )
+            return
+
+        self.stdout.write(
+            f"{len(pending)} sound file(s) still stored under a user-supplied name."
+        )
+
+        renamed = failed = 0
+        for sound in pending:
+            old_name = sound.file.name
+            try:
+                if dry_run:
+                    self.stdout.write(
+                        f"  would rename {old_name} "
+                        f"-> {sound_file_path(sound, old_name)}"
+                    )
+                    continue
+                new_name = self._rename(sound, old_name)
+            except Exception:
+                # One unreadable or unwritable object must not abort the run:
+                # the whole point of a command over a migration is that the
+                # rest of the batch still gets done, and the key is recorded.
+                failed += 1
+                log.exception("Could not rename sound %s (%s)", sound.pk, old_name)
+                self.stderr.write(
+                    self.style.ERROR(f"  failed: {old_name} (sound {sound.pk})")
+                )
+                continue
+            renamed += 1
+            self.stdout.write(f"  {old_name} -> {new_name}")
+
+        if dry_run:
+            self.stdout.write(self.style.WARNING("Dry run: nothing was changed."))
+            return
+
+        self.stdout.write(self.style.SUCCESS(f"Renamed {renamed} file(s)."))
+        if failed:
+            self.stdout.write(
+                self.style.ERROR(
+                    f"{failed} file(s) failed and were left as they were; "
+                    "re-run to retry them."
+                )
+            )
+
+    @staticmethod
+    def _rename(sound, old_name):
+        """Write the new object, point the row at it, then drop the old one.
+
+        The order matters. Interrupted between any two steps, the worst
+        outcome is an orphaned old object, never a row pointing at a key that
+        no longer exists.
+        """
+        storage = sound.file.storage
+        with sound.file.open("rb") as handle:
+            content = handle.read()
+
+        if not sound.file_name_original:
+            sound.file_name_original = old_name.rsplit("/", 1)[-1]
+
+        # FileField.save writes through storage and resets the cached handle.
+        sound.file.save(
+            sound_file_path(sound, old_name), ContentFile(content), save=True
+        )
+        new_name = sound.file.name
+
+        if old_name != new_name and storage.exists(old_name):
+            storage.delete(old_name)
+        return new_name

src/core/tests/test_backfill_sound_filenames.py

@@ -0,0 +1,127 @@
+import re
+from io import StringIO
+from unittest import mock
+
+from django.core.files.base import ContentFile
+from django.core.management import call_command
+from django.test import TestCase
+
+from core.models import Sound, SoundExtensions
+from users.tests.factory import ProfileFactory
+
+GENERATED = re.compile(r"^sounds/[0-9a-f]{32}\.mp3$")
+
+
+def legacy_sound(owner, filename="Música #1 (final).mp3"):
+    """A row as it looks before #982: stored under the uploader's own name."""
+    sound = Sound.objects.create(
+        title=filename,
+        author="A",
+        owner=owner,
+        file_extension=SoundExtensions.MP3,
+        file_name_original="",
+    )
+    # Bypass upload_to so the old layout can be reproduced verbatim.
+    sound.file.name = f"sounds/{filename}"
+    sound.file.storage.save(sound.file.name, ContentFile(b"audio-bytes"))
+    sound.save(update_fields=["file"])
+    return sound
+
+
+class TestBackfillSoundFilenames(TestCase):
+    def setUp(self):
+        self.owner = ProfileFactory()
+
+    def run_command(self, **kwargs):
+        out = StringIO()
+        call_command("backfill_sound_filenames", stdout=out, stderr=out, **kwargs)
+        return out.getvalue()
+
+    def test_a_legacy_file_is_moved_to_a_generated_name(self):
+        sound = legacy_sound(self.owner)
+
+        self.run_command()
+        sound.refresh_from_db()
+
+        assert GENERATED.match(sound.file.name), sound.file.name
+        assert "Música" not in sound.file.name
+
+    def test_the_content_survives_the_move(self):
+        sound = legacy_sound(self.owner)
+
+        self.run_command()
+        sound.refresh_from_db()
+
+        with sound.file.open("rb") as handle:
+            assert handle.read() == b"audio-bytes"
+
+    def test_the_old_object_is_removed_from_storage(self):
+        sound = legacy_sound(self.owner)
+        old_name = sound.file.name
+
+        self.run_command()
+
+        assert not sound.file.storage.exists(old_name)
+
+    def test_the_original_filename_is_preserved_when_it_was_empty(self):
+        sound = legacy_sound(self.owner)
+
+        self.run_command()
+        sound.refresh_from_db()
+
+        assert sound.file_name_original == "Música #1 (final).mp3"
+
+    def test_an_already_generated_name_is_left_alone(self):
+        sound = legacy_sound(self.owner)
+        self.run_command()
+        sound.refresh_from_db()
+        settled = sound.file.name
+
+        output = self.run_command()
+        sound.refresh_from_db()
+
+        assert sound.file.name == settled, "a second run must not rename again"
+        assert "Nothing to do" in output
+
+    def test_dry_run_changes_nothing(self):
+        sound = legacy_sound(self.owner)
+        before = sound.file.name
+
+        output = self.run_command(dry_run=True)
+        sound.refresh_from_db()
+
+        assert sound.file.name == before
+        assert "would rename" in output
+        assert "Dry run" in output
+
+    def test_limit_processes_only_the_first_rows(self):
+        first = legacy_sound(self.owner, "one.mp3")
+        second = legacy_sound(self.owner, "two.mp3")
+
+        self.run_command(limit=1)
+        first.refresh_from_db()
+        second.refresh_from_db()
+
+        assert GENERATED.match(first.file.name)
+        assert second.file.name == "sounds/two.mp3"
+
+    def test_one_failure_does_not_abort_the_batch(self):
+        broken = legacy_sound(self.owner, "broken.mp3")
+        healthy = legacy_sound(self.owner, "healthy.mp3")
+        real_open = Sound.file.field.attr_class.open
+
+        def fail_for_broken(self_file, *args, **kwargs):
+            if "broken" in (self_file.name or ""):
+                raise OSError("cannot read")
+            return real_open(self_file, *args, **kwargs)
+
+        with mock.patch.object(Sound.file.field.attr_class, "open", fail_for_broken):
+            output = self.run_command()
+
+        broken.refresh_from_db()
+        healthy.refresh_from_db()
+
+        assert broken.file.name == "sounds/broken.mp3", "the failure stays untouched"
+        assert GENERATED.match(healthy.file.name), "the rest of the batch proceeds"
+        assert "failed" in output
+        assert "re-run to retry" in output