Skip to content

Commit 15be9d1

Browse files
jonfroehlichjonfroehlich
authored
Merge pull request #1402 from makeabilitylab/1391-backfill-resilience
Harden backfill_original_filenames against per-row failures (#1391)
2 parents 5df25e3 + 9b7ec10 commit 15be9d1

2 files changed

Lines changed: 118 additions & 51 deletions

File tree

‎website/management/commands/backfill_original_filenames.py‎

Lines changed: 84 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -47,27 +47,39 @@ def handle(self, *args, **options):
4747

4848
total_updated = 0
4949
total_skipped = 0
50+
total_errors = 0
5051
for model in self.MODELS:
51-
updated, skipped = self._backfill_model(model, dry_run)
52+
updated, skipped, errors = self._backfill_model(model, dry_run)
5253
total_updated += updated
5354
total_skipped += skipped
55+
total_errors += errors
5456

5557
verb = "Would update" if dry_run else "Updated"
5658
_logger.info(
5759
f"backfill_original_filenames: {verb} {total_updated} filename "
5860
f"field(s); skipped {total_skipped} (already standardized — original "
59-
f"name unrecoverable)."
61+
f"name unrecoverable); {total_errors} row(s) errored and were skipped."
6062
)
6163
_logger.debug("Completed backfill_original_filenames.py")
6264

6365
def _backfill_model(self, model, dry_run):
6466
"""Backfill both file fields for one concrete artifact model.
6567
66-
Returns a ``(num_updated, num_skipped)`` tuple counting individual
67-
filename fields touched / deliberately left blank.
68+
Returns a ``(num_updated, num_skipped, num_errors)`` tuple counting
69+
individual filename fields touched / deliberately left blank / skipped
70+
because the row raised.
71+
72+
Each row is processed inside its own try/except: this iterates the
73+
whole production dataset, and a single malformed row (e.g. a null
74+
``date`` or ``title`` makes ``generate_filename`` raise) must not abort
75+
the entire backfill and leave every later row untouched. The entrypoint
76+
has no ``set -e``, so an aborted run would fail silently (the traceback
77+
prints but startup continues) — this per-row isolation is defensive
78+
insurance against that.
6879
"""
6980
num_updated = 0
7081
num_skipped = 0
82+
num_errors = 0
7183
for file_attr, original_attr in self.FILE_FIELDS:
7284
# Only rows that have a file but no captured original name yet.
7385
candidates = (
@@ -80,52 +92,73 @@ def _backfill_model(self, model, dry_run):
8092
candidates = candidates.prefetch_related("authors")
8193

8294
for artifact in candidates:
83-
file_field = getattr(artifact, file_attr)
84-
if not file_field:
85-
continue
86-
87-
current_basename = os.path.basename(file_field.name)
88-
current_no_ext = os.path.splitext(current_basename)[0]
89-
standardized_no_ext = Artifact.generate_filename(artifact)
90-
91-
# Treat the file as already-standardized when its name equals the
92-
# standardized scheme OR is a uniquified variant of it. When a
93-
# standardized name collides on disk, ensure_filename_is_unique()
94-
# (fileutils.py) appends "-<timestamp>" — e.g.
95-
# "Lee_Talk_CHI2021-1782399772.42.pdf" — so the on-disk name
96-
# still STARTS WITH the standardized base. Matching only on exact
97-
# equality would misread those as never-renamed and record the
98-
# standardized+suffix name as the "original" — a false positive.
99-
already_standardized = (
100-
current_no_ext == standardized_no_ext
101-
or current_no_ext.startswith(standardized_no_ext + "-")
102-
)
103-
if already_standardized:
104-
# Already renamed — the original upload name is gone.
105-
_logger.debug(
106-
f"Skipping {model.__name__} id={artifact.pk} {file_attr}="
107-
f"'{current_basename}': already standardized."
95+
try:
96+
if self._backfill_row(model, artifact, file_attr,
97+
original_attr, dry_run):
98+
num_updated += 1
99+
else:
100+
num_skipped += 1
101+
except Exception:
102+
# Log and move on — never let one row kill the batch.
103+
_logger.exception(
104+
"backfill_original_filenames: skipping %s id=%s %s due "
105+
"to an error", model.__name__,
106+
getattr(artifact, "pk", "?"), file_attr,
108107
)
109-
num_skipped += 1
110-
continue
111-
112-
# Never renamed: the current on-disk name is the original.
113-
if dry_run:
114-
_logger.debug(
115-
f"[dry-run] Would set {original_attr}='{current_basename}' "
116-
f"for {model.__name__} id={artifact.pk} '{artifact.title}'"
117-
)
118-
else:
119-
# Write directly via the queryset so this stays a pure data
120-
# backfill — no file-rename / thumbnail side effects from the
121-
# model's save().
122-
model.objects.filter(pk=artifact.pk).update(
123-
**{original_attr: current_basename}
124-
)
125-
_logger.debug(
126-
f"Set {original_attr}='{current_basename}' for "
127-
f"{model.__name__} id={artifact.pk} '{artifact.title}'"
128-
)
129-
num_updated += 1
108+
num_errors += 1
109+
110+
return num_updated, num_skipped, num_errors
111+
112+
def _backfill_row(self, model, artifact, file_attr, original_attr, dry_run):
113+
"""Backfill one (artifact, file field) pair.
114+
115+
Returns True if the original name was (or would be) recorded, False if
116+
the row was deliberately skipped because its file is already
117+
standardized. Raises on malformed data — the caller isolates that.
118+
"""
119+
file_field = getattr(artifact, file_attr)
120+
if not file_field:
121+
return False
122+
123+
current_basename = os.path.basename(file_field.name)
124+
current_no_ext = os.path.splitext(current_basename)[0]
125+
standardized_no_ext = Artifact.generate_filename(artifact)
126+
127+
# Treat the file as already-standardized when its name equals the
128+
# standardized scheme OR is a uniquified variant of it. When a
129+
# standardized name collides on disk, ensure_filename_is_unique()
130+
# (fileutils.py) appends "-<timestamp>" — e.g.
131+
# "Lee_Talk_CHI2021-1782399772.42.pdf" — so the on-disk name still
132+
# STARTS WITH the standardized base. Matching only on exact equality
133+
# would misread those as never-renamed and record the standardized+
134+
# suffix name as the "original" — a false positive.
135+
already_standardized = (
136+
current_no_ext == standardized_no_ext
137+
or current_no_ext.startswith(standardized_no_ext + "-")
138+
)
139+
if already_standardized:
140+
# Already renamed — the original upload name is gone.
141+
_logger.debug(
142+
f"Skipping {model.__name__} id={artifact.pk} {file_attr}="
143+
f"'{current_basename}': already standardized."
144+
)
145+
return False
130146

131-
return num_updated, num_skipped
147+
# Never renamed: the current on-disk name is the original.
148+
if dry_run:
149+
_logger.debug(
150+
f"[dry-run] Would set {original_attr}='{current_basename}' "
151+
f"for {model.__name__} id={artifact.pk} '{artifact.title}'"
152+
)
153+
else:
154+
# Write directly via the queryset so this stays a pure data
155+
# backfill — no file-rename / thumbnail side effects from the
156+
# model's save().
157+
model.objects.filter(pk=artifact.pk).update(
158+
**{original_attr: current_basename}
159+
)
160+
_logger.debug(
161+
f"Set {original_attr}='{current_basename}' for "
162+
f"{model.__name__} id={artifact.pk} '{artifact.title}'"
163+
)
164+
return True

‎website/tests/test_artifact.py‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,40 @@ def test_uniquified_standardized_name_is_not_backfilled(self):
296296
talk.refresh_from_db()
297297
self.assertIsNone(talk.original_pdf_filename)
298298

299+
def test_one_bad_row_does_not_abort_the_batch(self):
300+
"""
301+
The backfill runs on every container start over the whole dataset, so a
302+
single malformed row must not abort the run and leave every other row
303+
untouched. A null ``date`` makes ``generate_filename`` raise
304+
(``date.year``); before per-row isolation that exception propagated out
305+
of the command. Here the bad row is skipped and a good legacy row is
306+
still backfilled.
307+
"""
308+
# Malformed: no authors (so save() skips the rename and accepts the
309+
# null date) + a file, which makes it a backfill candidate that raises.
310+
bad = TalkFactory(
311+
title="Bad Row", forum_name="CHI", date=None,
312+
pdf_file=_pdf("bad_upload.pdf"),
313+
)
314+
# A good legacy candidate with a non-standard (never-renamed) name.
315+
good = TalkFactory(
316+
title="Good Row", forum_name="CHI", date=date(2019, 1, 1),
317+
pdf_file=_pdf("good_upload_final.pdf"),
318+
)
319+
good_name = os.path.basename(good.pdf_file.name)
320+
Talk.objects.filter(pk__in=[bad.pk, good.pk]).update(
321+
original_pdf_filename=None
322+
)
323+
324+
# Must not raise despite the malformed row...
325+
call_command("backfill_original_filenames")
326+
327+
good.refresh_from_db()
328+
bad.refresh_from_db()
329+
# ...and the good row is still processed.
330+
self.assertEqual(good.original_pdf_filename, good_name)
331+
self.assertIsNone(bad.original_pdf_filename)
332+
299333

300334
class OriginalUploadFilenamesDisplayTests(SimpleTestCase):
301335
"""ArtifactAdmin.original_upload_filenames read-only display (issue #1391)."""

0 commit comments

Comments
 (0)