Skip to content

Commit f14733e

Browse files
jonfroehlichclaude
andcommitted
Fix dotted-name extension bug + recover 3 diverged talks (#1390)
The 2.25.3 re-standardization renamed 210 files cleanly but errored on 3 talks (513, 528, 564) whose standardized name contains a dot ("D.C.", "Mobility.", "Dr."). Root cause: rename_artifact_in_db_and_filesystem used os.path.splitext to decide whether to append the file extension; on a dotted name it treats the text after the last dot as the "extension" and skips adding the real .pdf/.pptx. The file was renamed extension-less on disk, then generate_thumbnail_for_pdf raised "not a PDF" *before* super().save(), leaving the files moved on disk but the DB pointing at the old (now-missing) names. Fixes: - fileutils: append the original extension unless the name already ends with it (endswith), instead of the splitext check. Regression-tested with a dotted base. - artifact.save(): thumbnail generation is now non-fatal (try/except). A thumbnail error must never abort a save that has already renamed files on disk, or it re-creates this divergence. - New repair_diverged_artifact_filenames command: divergence-gated (only rows whose DB file is missing on disk), finds the orphaned file by content type (the orphan has no usable extension), renames it to the correct standardized name, repoints the DB via update(), and regenerates the thumbnail. Idempotent; runs in --dry-run at entrypoint 4.10c this release for review before the real recovery. Tested (repair, dry-run no-op, healthy-row untouched) against an isolated MEDIA_ROOT. Bump to 2.25.4. No recovery performed yet (dry-run only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 8f6dd9a commit f14733e

7 files changed

Lines changed: 390 additions & 11 deletions

File tree

docker-entrypoint.sh

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,14 @@ echo "4.10b Running 'python manage.py restandardize_artifact_filenames' to renam
168168
echo "******************************************"
169169
python manage.py restandardize_artifact_filenames
170170

171+
echo "****************** STEP 4.10c/5: docker-entrypoint.sh ************************"
172+
echo "4.10c Running 'python manage.py repair_diverged_artifact_filenames' to recover artifacts whose files were renamed on disk but not in the DB (#1390 dotted-name bug)"
173+
echo "******************************************"
174+
# TEMPORARY (#1390): --dry-run logs which diverged artifacts WOULD be repaired,
175+
# touching nothing on disk or in the DB, so we can review on prod before doing it
176+
# for real. REMOVE --dry-run and redeploy to perform the recovery.
177+
python manage.py repair_diverged_artifact_filenames --dry-run
178+
171179
# echo "****************** STEP 4.3/5: docker-entrypoint.sh ************************"
172180
# echo "4.3 Running 'python manage.py rename_person_images' to rename person images"
173181
# echo "******************************************"

makeabilitylab/settings.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,8 @@
8686
SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
8787

8888
# Makeability Lab Global Variables, including Makeability Lab version
89-
ML_WEBSITE_VERSION = "2.25.3" # Keep this updated with each release and also change the short description below
90-
ML_WEBSITE_VERSION_DESCRIPTION = "Performs the legacy-filename re-standardization (#1390). After two diagnostic dry-runs and a forum-name cleanup (2.25.1/2.25.2), this release renames the ~213 historical talk/poster/publication files that were never converted to the standardized Author_TitleInTitleCase_VenueYear scheme (bulk-imported rows that never went through an authored save). The original uploaded names were already captured (#1391), so provenance is preserved and stale publication links still resolve via the serve_pdf fallback. Each rename is logged to debug.log; the step is idempotent, so subsequent deploys are no-ops."
89+
ML_WEBSITE_VERSION = "2.25.4" # Keep this updated with each release and also change the short description below
90+
ML_WEBSITE_VERSION_DESCRIPTION = "Fixes a filename bug exposed by the #1390 re-standardization and prepares recovery of the 3 affected talks. The rename helper used os.path.splitext to decide whether to add a file's extension, which misfired on standardized names containing dots (e.g. '...Dr.SangMook2009'): the file was renamed extension-less on disk and thumbnail generation then raised before the DB save committed, leaving 3 talks with files moved on disk but stale DB paths. This release (1) appends the extension by checking the actual extension rather than splitext, (2) makes thumbnail generation non-fatal so a thumbnail error can never again leave a rename half-committed, and (3) adds a divergence-gated recovery command (running in --dry-run here) that finds each orphaned file by content type and repoints it. No recovery is performed yet."
9191
DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed
9292
MAX_BANNERS = 7 # Maximum number of banners on a page
9393

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
import os
2+
import logging
3+
4+
from django.core.management.base import BaseCommand
5+
6+
from website.models import Artifact, Talk, Poster, Publication
7+
8+
# This retrieves a Python logging instance (or creates it)
9+
_logger = logging.getLogger(__name__)
10+
11+
12+
# Map a file extension to the magic-byte signature we expect its content to
13+
# start with. Used to disambiguate orphaned files that the bug renamed WITHOUT
14+
# extensions (so we can't tell pdf from pptx from the name alone). Returns None
15+
# for extensions we can't verify, in which case we refuse to guess.
16+
def _expected_kind_for_ext(ext):
17+
ext = ext.lower()
18+
if ext == ".pdf":
19+
return "pdf"
20+
if ext in (".pptx", ".docx", ".key", ".zip"):
21+
return "zip" # modern Office / Keynote / zip-based formats
22+
if ext in (".ppt", ".doc"):
23+
return "ole" # legacy OLE compound formats
24+
return None
25+
26+
27+
def _sniff_kind(path):
28+
"""Best-effort content type of a file from its leading magic bytes."""
29+
try:
30+
with open(path, "rb") as fh:
31+
head = fh.read(8)
32+
except OSError:
33+
return None
34+
if head.startswith(b"%PDF"):
35+
return "pdf"
36+
if head.startswith(b"PK\x03\x04"):
37+
return "zip"
38+
if head.startswith(b"\xff\xd8\xff"):
39+
return "jpeg"
40+
if head.startswith(b"\xd0\xcf\x11\xe0"):
41+
return "ole"
42+
return None
43+
44+
45+
class Command(BaseCommand):
46+
help = (
47+
"Repairs artifacts whose pdf_file/raw_file row points at a file that no "
48+
"longer exists on disk because a rename moved it but the save() never "
49+
"committed (the #1390 dotted-name bug: a name like '...Dr.SangMook2009' "
50+
"made os.path.splitext eat the extension, the file was renamed "
51+
"extension-less on disk, and thumbnail generation then raised before "
52+
"super().save()). The file CONTENT is safe on disk under the malformed "
53+
"name; this finds that orphan (by matching the standardized base and "
54+
"confirming its content type, since the orphan has no usable "
55+
"extension), renames it to the correct standardized name, and repoints "
56+
"the DB. Divergence-gated and idempotent: a row whose files already "
57+
"exist on disk is skipped, so this is a safe no-op once repaired. Run "
58+
"with --dry-run first to review exactly what it would touch."
59+
)
60+
61+
MODELS = (Talk, Poster, Publication)
62+
63+
def add_arguments(self, parser):
64+
parser.add_argument(
65+
"--dry-run",
66+
action="store_true",
67+
help="Report what would be repaired without touching disk or DB.",
68+
)
69+
70+
def handle(self, *args, **options):
71+
dry_run = options["dry_run"]
72+
_logger.info(
73+
f"Running repair_diverged_artifact_filenames (dry_run={dry_run})."
74+
)
75+
76+
repaired = unrecoverable = 0
77+
for model in self.MODELS:
78+
for artifact in model.objects.prefetch_related("authors").all():
79+
result = self._repair_artifact(artifact, dry_run)
80+
repaired += result["repaired"]
81+
unrecoverable += result["unrecoverable"]
82+
83+
verb = "Would repair" if dry_run else "Repaired"
84+
_logger.info(
85+
f"repair_diverged_artifact_filenames: {verb} {repaired} file(s); "
86+
f"{unrecoverable} diverged file(s) could not be matched to an "
87+
f"orphan on disk and were left untouched."
88+
)
89+
90+
def _repair_artifact(self, artifact, dry_run):
91+
model_name = type(artifact).__name__
92+
repaired = unrecoverable = 0
93+
fixed_any = False
94+
95+
for field_name in ("pdf_file", "raw_file"):
96+
file_field = getattr(artifact, field_name)
97+
if not file_field:
98+
continue
99+
# Only act on a true divergence: the DB names a file that is gone.
100+
if file_field.storage.exists(file_field.name):
101+
continue
102+
103+
outcome = self._repair_field(artifact, field_name, dry_run)
104+
if outcome == "repaired":
105+
repaired += 1
106+
fixed_any = True
107+
elif outcome == "unrecoverable":
108+
unrecoverable += 1
109+
_logger.warning(
110+
f"{model_name} id={artifact.pk}: {field_name} points at "
111+
f"missing '{file_field.name}' and no matching orphan was "
112+
f"found on disk; left untouched for manual review."
113+
)
114+
115+
# If we repointed any files for real, persist + regenerate the thumbnail
116+
# via a normal save(). With the extension bug fixed the names are already
117+
# standardized, so save() does no further renaming; it just writes the
118+
# corrected field names and rebuilds the (now-missing) thumbnail.
119+
if fixed_any and not dry_run:
120+
artifact.save()
121+
122+
return {"repaired": repaired, "unrecoverable": unrecoverable}
123+
124+
def _repair_field(self, artifact, field_name, dry_run):
125+
"""Locate the orphaned file for one diverged field and fix it.
126+
127+
Returns "repaired", "unrecoverable", or "noop".
128+
"""
129+
model_name = type(artifact).__name__
130+
file_field = getattr(artifact, field_name)
131+
132+
# The extension is still correct in the (stale) DB name; the standardized
133+
# base comes from generate_filename. Together they form the correct name.
134+
ext = os.path.splitext(file_field.name)[1]
135+
correct_base = Artifact.generate_filename(artifact)
136+
# get_valid_filename is applied by the rename path; mirror it so our
137+
# on-disk comparisons match what the buggy rename actually wrote.
138+
from django.utils.text import get_valid_filename
139+
valid_base = get_valid_filename(correct_base)
140+
correct_basename = get_valid_filename(correct_base + ext)
141+
142+
directory = os.path.dirname(file_field.path)
143+
rel_dir = os.path.dirname(file_field.name)
144+
if not os.path.isdir(directory):
145+
return "unrecoverable"
146+
147+
expected_kind = _expected_kind_for_ext(ext)
148+
149+
# Candidate orphans: the bug wrote the file as the (extension-less) valid
150+
# base, possibly with a "-<timestamp>" uniqueness suffix from colliding
151+
# with the sibling files. Match those, then confirm by content type so we
152+
# never mis-pair a pdf with a pptx/thumbnail (they share the base name).
153+
candidates = []
154+
for entry in os.listdir(directory):
155+
if entry == valid_base or entry.startswith(valid_base + "-"):
156+
full = os.path.join(directory, entry)
157+
if os.path.isfile(full):
158+
candidates.append(entry)
159+
160+
matches = [
161+
c for c in candidates
162+
if expected_kind is not None
163+
and _sniff_kind(os.path.join(directory, c)) == expected_kind
164+
]
165+
166+
if len(matches) != 1:
167+
_logger.debug(
168+
f"{model_name} id={artifact.pk}: {field_name} expected kind="
169+
f"{expected_kind}; candidates={candidates}; content-matches="
170+
f"{matches} (need exactly 1)."
171+
)
172+
return "unrecoverable"
173+
174+
orphan = matches[0]
175+
target_full = os.path.join(directory, correct_basename)
176+
target_rel = os.path.join(rel_dir, correct_basename)
177+
178+
_logger.info(
179+
f"[{'dry-run' if dry_run else 'apply'}] {model_name} "
180+
f"id={artifact.pk}: {field_name} '{file_field.name}' (missing) -> "
181+
f"on-disk orphan '{orphan}' renamed to '{correct_basename}' and "
182+
f"repointed."
183+
)
184+
185+
if dry_run:
186+
return "repaired"
187+
188+
# If the target name is somehow already taken by a different file, don't
189+
# clobber it — bail out for manual review.
190+
if os.path.exists(target_full) and orphan != correct_basename:
191+
_logger.warning(
192+
f"{model_name} id={artifact.pk}: target '{correct_basename}' "
193+
f"already exists; not overwriting. Left for manual review."
194+
)
195+
return "unrecoverable"
196+
197+
os.rename(os.path.join(directory, orphan), target_full)
198+
# Persist the corrected name directly with update() rather than relying
199+
# on the caller's save(): save() may set update_fields=['thumbnail'] when
200+
# it rebuilds the stale thumbnail, which would drop a pdf_file/raw_file
201+
# write. update() guarantees the repointed name lands in the DB. We also
202+
# set it in memory so the caller's save() regenerates the thumbnail from
203+
# the now-correct pdf path.
204+
type(artifact).objects.filter(pk=artifact.pk).update(
205+
**{field_name: target_rel}
206+
)
207+
file_field.name = target_rel
208+
return "repaired"

website/models/artifact.py

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -401,12 +401,26 @@ def save(self, *args, **kwargs):
401401
# generate a thumbnail
402402
if self.pdf_file.storage.exists(self.pdf_file.name):
403403
thumbnail_local_path = os.path.dirname(thumbnail_filename_with_local_path)
404-
ml_fileutils.generate_thumbnail_for_pdf(self.pdf_file, self.thumbnail, thumbnail_local_path)
405-
406-
# If 'update_fields' does not exist in kwargs, all fields are saved
407-
# Add 'thumbnail' to the update_fields list so that it gets updated in the db
408-
if 'update_fields' in kwargs:
409-
kwargs.setdefault('update_fields', []).append('thumbnail')
404+
# Thumbnail generation must never abort save(): by this
405+
# point any file rename above has already happened on
406+
# disk, so raising here would leave the DB out of sync
407+
# with the filesystem (this is exactly what corrupted 3
408+
# dotted-name talks in #1390). A missing/odd thumbnail is
409+
# cosmetic and self-heals on a later save; a half-renamed
410+
# artifact is not. So log and continue rather than raise.
411+
try:
412+
ml_fileutils.generate_thumbnail_for_pdf(self.pdf_file, self.thumbnail, thumbnail_local_path)
413+
414+
# If 'update_fields' does not exist in kwargs, all fields are saved
415+
# Add 'thumbnail' to the update_fields list so that it gets updated in the db
416+
if 'update_fields' in kwargs:
417+
kwargs.setdefault('update_fields', []).append('thumbnail')
418+
except Exception:
419+
_logger.exception(
420+
f"Thumbnail generation failed for artifact.id={self.id} "
421+
f"(pdf={self.pdf_file.name}); continuing without it so "
422+
f"the save (and any preceding rename) still commits."
423+
)
410424
else:
411425
_logger.debug(f"Could not generate a thumbnail because the pdf {self.pdf_file.path} was not found in storage")
412426
elif thumbnail_exists_in_storage:

website/tests/test_fileutils.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"""
99

1010
import os
11+
import tempfile
1112
from datetime import date
1213

1314
from django.test import SimpleTestCase
@@ -19,9 +20,52 @@
1920
get_filename_no_ext,
2021
get_filename_without_ext_for_artifact,
2122
is_image,
23+
rename_artifact_on_filesystem,
2224
)
2325

2426

27+
class _FakeFileField:
28+
"""Minimal stand-in for a Django FieldFile: just the .path/.name the
29+
on-filesystem rename helper reads and writes (no DB, no storage)."""
30+
31+
def __init__(self, path):
32+
self.path = path
33+
self.name = os.path.basename(path)
34+
35+
36+
class RenameArtifactExtensionTests(SimpleTestCase):
37+
"""rename_artifact_on_filesystem must preserve the file extension even when
38+
the standardized base name contains dots (#1390). The old splitext()-based
39+
check mistook the text after the last dot for an extension and renamed the
40+
file extension-less on disk, which then broke thumbnail generation."""
41+
42+
def _rename(self, old_basename, new_base):
43+
with tempfile.TemporaryDirectory() as d:
44+
old_path = os.path.join(d, old_basename)
45+
with open(old_path, "wb") as fh:
46+
fh.write(b"%PDF-1.4 test")
47+
field = _FakeFileField(old_path)
48+
rename_artifact_on_filesystem(field, new_base)
49+
return field.name # basename of the renamed file
50+
51+
def test_dotted_base_keeps_pdf_extension(self):
52+
# "D.C." would make splitext see ".C.ArtScience...2014" as the extension.
53+
new_name = self._rename(
54+
"Old_zVYlRJ8.pdf",
55+
"Froehlich_SocialFabrics_NationalAcademyofSciencesD.C.ArtScience2014",
56+
)
57+
self.assertTrue(new_name.endswith(".pdf"), new_name)
58+
59+
def test_dotless_base_still_gets_extension(self):
60+
new_name = self._rename("Old.pdf", "Froehlich_AStandardTalk_CHI2024")
61+
self.assertTrue(new_name.endswith(".pdf"), new_name)
62+
63+
def test_extension_not_doubled_when_already_present(self):
64+
new_name = self._rename("Old.pdf", "Froehlich_AStandardTalk_CHI2024.pdf")
65+
self.assertTrue(new_name.endswith(".pdf"), new_name)
66+
self.assertFalse(new_name.endswith(".pdf.pdf"), new_name)
67+
68+
2569
class IsImageTests(SimpleTestCase):
2670
def test_known_image_extensions(self):
2771
for name in ("photo.jpg", "PHOTO.JPEG", "art.png", "anim.gif"):

0 commit comments

Comments
 (0)