From ab5e36471aaa24d11c60ec96497ac50af8aa9dc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 17:59:25 +0000 Subject: [PATCH] feat(sounds): add a resumable command to backfill legacy sound filenames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #982 makes new uploads land at sounds/., but the rows that predate it still point at files stored under the name their uploader chose — which are the ones actually exposed. #699 asked for a data migration. This is a management command instead: a migration renames live S3 objects inside a deploy, cannot be resumed after a partial run, and offers no way to see what would change first. The command is idempotent, so resuming is just running it again. It writes the new object, points the row at it, then deletes the old one, so an interruption leaves an orphan rather than a row pointing at nothing. It takes --dry-run and --limit, backfills file_name_original when empty, and isolates per-row failures so one unreadable object cannot abort the batch. Closes #991 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HXUw7kyELDu8ycGQxtaFbb --- .../commands/backfill_sound_filenames.py | 125 +++++++++++++++++ .../tests/test_backfill_sound_filenames.py | 127 ++++++++++++++++++ 2 files changed, 252 insertions(+) create mode 100644 src/core/management/commands/backfill_sound_filenames.py create mode 100644 src/core/tests/test_backfill_sound_filenames.py diff --git a/src/core/management/commands/backfill_sound_filenames.py b/src/core/management/commands/backfill_sound_filenames.py new file mode 100644 index 00000000..6c5ae2da --- /dev/null +++ b/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/.` (#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 diff --git a/src/core/tests/test_backfill_sound_filenames.py b/src/core/tests/test_backfill_sound_filenames.py new file mode 100644 index 00000000..5a0d87e2 --- /dev/null +++ b/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