Repository navigation
Conversation
FindOrCreate and BatchFindOrCreate fall back to a case-insensitive name match when no stored TMDB or IMDb ID matches. That fallback merged an incoming person into any stored person with the same name, even when the two carried different provider IDs, so one actor's credits could land on a namesake. The name fallback now resolves only when the two share a provider ID, or when neither has any. An identified person no longer takes over a name-only placeholder of the same name, and a name-only credit no longer attaches to an identified person; each keeps its own row.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Quick104
added
priority: P1
Cursor
|
The changed fallback alters the The current Automated check: Macroscope check run agent (gpt-6-luna). No validation was performed. Posted via Macroscope — v1 validation impact |
2 participants
Problem
Related issue: N/A
Validation tasks: changes #1143 C1 and C2; changes #1144 C1 and C3; changes Silo-Server/silo-apple#307 C1 and C3; changes Silo-Server/silo-apple#308 C1 and C3; changes Silo-Server/silo-android#317 C1 and C3; changes Silo-Server/silo-android#318 C1 and C3 (all passed on build 941)
Two different people with the same name can end up as one person in Silo. When a credit comes in and neither its TMDB ID nor its IMDb ID is stored yet,
FindOrCreateandBatchFindOrCreatefall back to a case-insensitive name match and attach the credit to whoever already has that name, even when the two carry different provider IDs. One actor's films then appear on a namesake's person page, and the second person's provider IDs are thrown away.This change lets the name fallback match only when the two share a provider ID, or when neither has any.
The person filmography and name search cases listed above passed with provider-identified fixture people. Those resolve by stored TMDB or IMDb ID before the name fallback runs, so their outcomes should still meet the criteria. Only namesakes with different provider IDs and name-only placeholders now resolve differently, which can change a filmography or add a same-name search result. Those cases should be re-checked after merge.
I've run this on my server since August. It has 1,127 names that belong to more than one TMDB person (2,271 people in total). Under the old fallback, each of those people whose provider IDs weren't stored yet when its first credit arrived would have been merged into the first person with that name.
Root cause: phase 3 of
BatchFindOrCreate, and the last lookup inFindOrCreate, select byLOWER(name)alone and take the first match without looking at provider IDs.Approach
canResolvePersonByNameaccepts a candidate only when the names match, no provider ID conflicts, and either the two share at least one provider ID or neither has any.docs/*-api.mdchanges.Validation
go test ./internal/catalog/ -count=1: pass.TestCanResolvePersonByNameis a unit test of the rule. It covers a shared TMDB or TVDB ID, two placeholders, disjoint IDs, IDs from different providers, an identified person meeting a placeholder, and different names with a shared ID.TestPersonNameFallbackKeepsNamesakesApartPostgresis a new database contract, listed inscripts/ci/db-contracts.txt. ThroughFindOrCreateandBatchFindOrCreate, it checks that:Against a migrated local PostgreSQL 18 database it passes on this branch, along with the other person database tests in
internal/catalog. Withmain'sperson_repo.goit fails: "FindOrCreate merged a namesake with another TMDB ID into person …".go vet ./internal/catalog/andmake lint-changed: clean.On my server, the same change has run since August as part of my fork.
CI on
b7c75b314: passed, includingGo DB pins, which runs the new contract and fails on a listed test that is missing or skipped.Benchmarks
Not applicable. The name lookups read four more columns from rows they already select, and still use the same
LOWER(name)predicate.Evidence
Which person a credit lands on is visible on item and person pages, but I have no before-and-after capture from an upstream build: that needs a deploy of
mainand of this branch against the same data. What I can show is the database test above, which fails onmainand passes here, and the counts from my server, which runs my fork at6ce7fb5ccwith this change.Evidence: https://evidence.siloserver.org/r/silo-server/pr-2082/
Risks
person_repo.go, in the refresh and update functions and a comment nearFindOrCreate, not in the name lookups. They may need a rebase but shouldn't conflict in logic.Checklist
AI Disclosure
main's ID and name lookups in both functions are unchanged since then (only the enrichment step after them changed), and added the database contract.AI-assisted with Claude Opus. I directed the task and designed the work.