Ingest never creates an Album or sets photo.album_id #140
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: HIGH — a documented feature with nothing behind it
Split out of #105, which had grown to four separable units. This is the first, and the other album work depends on it.
The problem
Nothing in
backend/app/has ever created anAlbumrow or assignedphoto.album_id.services/filename_parser.pyparses analbum_slugand asequenceout of names likeredbk42_001_1983.jpg, and is thorough about it — butapi/routes/ingest.pynever calls it. It mentions album parsing twice in comments and does nothing.The consequences are live today:
GET /api/photos?album_id=…is a working, tested filter that matches nothing in any deployment.ix_photo_collection_albumindexes a column that is NULL in every row.sequence_in_albumis likewise never set, sodocs/circa-spec.md's album ordering has no data.AiContext.album_nameis passed to the AI prompt (services/ai_claude.py:299) and is alwaysNone, so the model never receives the album hint the code goes to the trouble of formatting.Scope
parse_filenameduring ingest and use what it already returns.Albumon first sight of a slug within a collection, and link subsequent photographs to it.Albumhas aUniqueConstraint(collection_id, slug), so the get-or-create needs to be race-safe — bulk ingest is concurrent by nature, and two workers meeting a new slug at once must not both insert.sequence_in_albumfrom the parsed sequence.Album.nameshould be when it is derived from a slug. A slug likeredbk42is not a name a person would choose; the honest options are to use the slug verbatim until someone renames it, or to leavenameequal toslugand let a later admin UI fix it. Worth deciding deliberately rather than defaulting.original_filenamevalues and links them would make the feature real for collections already ingested —app/cli/has the pattern.Done when
redbk42_001_1983.jpgcreates or reuses albumredbk42and links the photosequence_in_albumis set from the filenameAlbumReferences
backend/app/services/filename_parser.py(parses, and is never called)backend/app/api/routes/ingest.py:63,205(comments describing parsing that does not happen)backend/app/models/models.py—Album,Photo.album_id,Photo.sequence_in_albumThe open question, decided:
Album.nameis the slug, verbatimredbk42becomes an album namedredbk42. Recording why, since this was deliberately left open.It is exactly what the archive knows. The slug is parsed out of a filename; nothing else about the album has been established. Anything more readable is invented, and inventing readability is the same class of error as rendering a date no evidence supports (#78) — just quieter, because a wrong album name looks like a typo rather than a claim.
The slug is also the most recognisable handle available. It is not an opaque identifier: it is the string on the front of every filename in that album, which is what a reviewer is looking at while they work. "redbk42" is a thing they can match against a physical box; "Red Book 42" is a thing I would have made up.
It is lossless and reversible. A later rename replaces it and nothing has to be un-guessed first.
Two alternatives, rejected
Prettifying —
redbk42→ "Red Book 42", or even "Redbk 42". This requires knowing thatredbkabbreviates "red book", which the archive has no basis for; it could as easily be a person's initials or a shelf code. Even the mechanical version ("Redbk 42") is a guess about where the word ends, and it is lossy: the slug cannot be recovered from it reliably.Making
namenullable, so the model distinguishes "derived from a filename" from "a person chose this". This is the philosophically tidiest — it is a real distinction and the codebase cares about that class of distinction elsewhere (derivatives_errorvs a broken tile). Rejected on cost:Album.nameisnullable=Falsetoday, so it needs a migration, and #141's album dropdown needs a non-null name to render anyway.name == slugalready carries the signal adequately at this scale. It is a convention rather than a guarantee — someone could rename an album to exactly its slug — but for a family archive that is not worth a schema change to make watertight.If a rename UI later wants to show "renamed from redbk42", the audit ledger will have the change.
Probe: one claim in the issue is wrong, and it makes this smaller than it reads
The ingest service does call it —
app/services/ingest.py:42importsparse_filenameand line 271 runs it on every scan. What is true is narrower and, I think, more interesting: it uses onlydate_low,date_highandpartial_text, to build thefilenameevidence row.album_slugandsequenceare parsed on every single ingest and thrown away.So this is not "wire up a parser that was never called". It is "stop discarding two fields the parser already hands us". Everything downstream in the issue still holds —
album_idis NULL in every row,ix_photo_collection_albumindexes nothing,sequence_in_albumis never set,AiContext.album_nameis alwaysNone— the cause is just one line further in than the issue says.Two consequences for the implementation:
The parse should move above
_buildrather than a second call being added. It is pure and cheap, and doing it once before thePhotois constructed meansalbum_idandsequence_in_albumare set on the INSERT rather than by an UPDATE afterwards._buildis called twice on the duplicate-race path, so the parsed result wants to be a local, not a recomputation.A duplicate row gets filed too. It is a real scan of a real print that came out of that album; leaving it unfiled would make the album's contents disagree with the box.
The
Album.namequestion is decided in the comment above: the slug, verbatim.Also worth stating before building: the get-or-create has to be race-safe against the same
IntegrityErrorpath ingest already uses forsha256.AlbumhasUniqueConstraint(collection_id, slug), bulk ingest is concurrent by nature, and two workers meeting a new slug at once must not both insert. The pattern is already in this file — savepoint, catchIntegrityError, re-read the winner — so it should be that one rather than a new one.Done in
f20f2b7, CI green (run 66).redbk42_001_1983.jpgcreates or reuses albumredbk42and links the photosequence_in_albumis set from the filenameAlbumpython -m app.cli.backfill_albumstest_filename_parser.pypasses untouchedThe two corrections are in the comments above: ingest already called the parser (it discarded
album_slugandsequencewhile using the dates), andAiContext.album_namewas already populated — it returnedNonebecause no photograph had ever had an album, not because the line was missing. Both were claims the issue made about missing code that was in fact present, which is the second and third time this milestone.One thing added beyond the scope, and worth knowing about.
parse_filenamereports a slug for any leading token, including one that is the whole stem —test_filename_parser.pypinsholidays.jpg → "holidays"deliberately. Filing on that alone would givescan.jpg,untitled.jpgandholidays1983.jpgan album each, which an unattended scanner produces by the hundred, and it would make this issue's own acceptance criterion ("does not create one named after the whole filename") unsatisfiable. So filing now requires structure past the slug — a sequence or a date — and the slug must not be the whole stem.Camera prefixes are excluded too (
IMG_,DSC_,DSCN,DSCF,PICT,GOPR,MVIMG,P), and that is a derivation rather than invented policy:filename_parser's own docstring listsIMG_<seq>_<date>.jpgas a camera-style name, separately from<album>_<seq>_<date>.jpg, so the parser already holds thatIMGis not an album. It matters because of what Circa is for — an album is a box or a book somebody assembled, and a memory card is not one; a card dump would otherwise land in one album named after the camera. The match is on the whole token, so an album genuinely calledimagesstill files.Failing conservatively is the right direction because the errors are not symmetric: an unfiled scan is
album_id NULL, which is where the entire archive is today and what the backfill can revisit; an invented album has to be un-invented by hand.Two findings outside the scope, both recorded rather than fixed here:
ix_photo_collection_albumhas never worked —(collection_id, album_id)with no sort key, so the planner prefersix_photo_collection_created. 4.893 ms on a deep page against 0.021 ms widened, ~230x. Nothing noticed because nothing could exercise it. Written up on #141 with what that issue must do, including that the widened index has to be partial ondeleted_at IS NULLsince #106, or it will not be used at all.expunge/no_autoflushguards in the duplicate-race handler are unreachable — every session factory setsautoflush=False, and removing both leaves the suite green. Kept, because they are correct against a session that does autoflush and the failure is one configuration change away, but the comment no longer claims a protection the code cannot currently reach. The same pair iningest_photo's sha256 handler is in the same position.Backend tests 1291 → 1320.