feat: normalize genres to relational 1-to-many like tags (#169) - #173
Conversation
…ready seam, exact filter, schema-only migration (#169)
|
@codex please review this PR (head c5f8d3a, branch feat/169-relational-genres) for #169 — normalize Movie/Music/Game genres to a relational 1-to-many like tags (shared Genre entity + 3 join tables, schema-only migration, exact-membership filter, bulk genres replace-set). Please return MERGE-BLOCKING or FOLLOW-UP verdicts per finding. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5f8d3a9be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| migrationBuilder.CreateTable( | ||
| name: "Genres", |
There was a problem hiding this comment.
In PostgreSQL deployments, CollectifyDbContextExtensions selects the separate Collectify.PostgresMigrations assembly, but this change adds AddGenres only under Collectify.Infrastructure. Consequently MigrateAsync() applies no genre schema change there, while every collection query now references Genres and its join tables, causing collection endpoints to fail with undefined-table errors after deployment. Add the equivalent migration and updated snapshot to the PostgreSQL migration project.
AGENTS.md reference: AGENTS.md:L200-L200
Useful? React with 👍 / 👎.
| var normalized = names | ||
| .Where(n => !string.IsNullOrWhiteSpace(n)) | ||
| .Select(n => n.Trim().ToLowerInvariant()) | ||
| .Distinct() | ||
| .ToList(); |
There was a problem hiding this comment.
When a create, update, or bulk request contains a genre longer than 100 characters, this normalization passes it unchanged even though CollectifyDbContext configures Genre.Name with HasMaxLength(100). PostgreSQL therefore rejects the write during SaveChangesAsync and returns a 500, while SQLite accepts data that violates the declared model constraint; validate the normalized names and return a 400 before creating Genre rows.
Useful? React with 👍 / 👎.
|
Disposition of Codex findings (head c5f8d3a): P1 — missing Postgres genre migration: Owner decision 2026-08-24 — do NOT add a new Postgres migration. Postgres is treated as never-deployed (existing deployments are SQLite-only); the relational-genre schema will be folded into the existing P2 — genre name longer than schema limit (100): Deferred as a fast-follow (Pre-existing |
|
Round-2: head moved to
Product code is unchanged since your round-1 review; only the four allow-listed test files changed. Please re-review PR #173 at head For reference, your round-1 findings are dispositioned:
|
…e filter (round 2)
|
Round-3: head moved to
Product code is unchanged since your round-1 review; only the allow-listed test files changed. Please re-review PR #173 at head |
|
Round-4 (confirming): head moved to
Product code is unchanged since your round-1 review; only allow-listed test files changed. Please re-review at head |
…shot, regenerate PG manifests The runtime EF model gained Genre + GameGenre/GenreMovie/GenreMusicAlbum but the Collectify.PostgresMigrations snapshot still had only the legacy comma-separated Genres string columns, so db.Database.MigrateAsync() hit a model mismatch and the app never came up against Postgres (CI smoke gate failure). Since no Postgres deployment has ever run these migrations, fold the genre schema into the first migration (InitialPostgres) in place rather than adding a 6th migration, keeping the 5-migration lineage and all historical Postgres tests (bootstrap, lineage, MultiDigitalStores) intact. Regenerates the 12 catalog-manifest.json fixtures (and provenance.json) that PostgresMigrationLineageTests/PostgresProviderSelectionTests byte-compare against, since editing InitialPostgres changes the schema at every pinned migration prefix.
|
@codex — re-requesting review on new head bbf4b96 (was P1 (MERGE-BLOCKING): Add the PostgreSQL genre migration — RESOLVED. Per owner decision (2026-08-24): no new additive PG migration; PG is treated as never-deployed, so the genre schema (Genre + GameGenre/GenreMovie/GenreMusicAlbum + unique (OwnerId,Name) index) was folded into the FIRST Postgres migration P2 (FOLLOW-UP): Reject genre names >100 chars — ACKNOWLEDGED, still OPEN. Being filed as a fast-follow issue (normalized genre-name length guard → 400). Not a blocker for this PR. Please re-review head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf4b96b2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| var ownerId = users.GetUserId(ctx.User)!; | ||
| var q = cfg.Set(db).AsNoTracking().Include(e => e.Tags).Where(e => e.OwnerId == ownerId); | ||
| var q = cfg.Set(db).AsNoTracking().Include(e => e.Tags).Include(e => e.Genres).Where(e => e.OwnerId == ownerId); |
There was a problem hiding this comment.
When list entries have both tags and genres, these sibling collection includes execute as one joined query, so each item materializes roughly tag count × genre count database rows before EF de-duplicates them. Since this endpoint takes up to 500 items, ordinary collections with several values in each relation can make list requests disproportionately slow and memory-heavy; use a split query or project the two collections separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated the authenticated collection list query to use EF Core’s
AsSplitQuery(), preventing cartesian row multiplication when loading both tags and genres while preserving owner filtering and existing eager loading. src/server/Collectify.Api/Endpoints/CollectionEndpoints.csL83-L89 - Committed the change on the current branch as
0134604(perf(#175): split collection relation queries). - Prepared the follow-up pull request titled “perf: split collection relation queries (Split dual Include(tags)+Include(genres) in collection list endpoint (cartesian row blowup) #175)”, closing issue Split dual Include(tags)+Include(genres) in collection list endpoint (cartesian row blowup) #175.
Testing
- ✅
git diff --check ⚠️ dotnet test tests/Collectify.Tests/Collectify.Tests.csproj --locked-mode --no-restore --filter 'FullyQualifiedName~MoviesEndpointsTests|FullyQualifiedName~MusicEndpointsTests|FullyQualifiedName~GamesEndpointsTests'(the .NET SDK is unavailable in this environment)- ✅
git status --short --branch(working tree clean)
| public DateOnly? LastPlayedOn { get; set; } | ||
|
|
||
| public ICollection<Tag> Tags { get; set; } = new List<Tag>(); | ||
| public ICollection<Genre> Genres { get; set; } = new List<Genre>(); |
There was a problem hiding this comment.
When a Steam-imported game is enriched by IgdbBackfillRunner, the provider's genres still go through the unchanged ComposeDescription workaround, whose comment explicitly assumes that games have no dedicated genre field. As a result, backfilled games receive genre text in Description but this new collection remains empty, so genre chips and ?genre= filtering do not reflect metadata already returned by IGDB; update the backfill merge to resolve and attach owner-scoped genres.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Updated IGDB game backfill to normalize comma-separated genre metadata, reuse matching owner-scoped genre records, and populate the relational
Game.Genrescollection. Existing user-selected genres remain unchanged under the runner’s fill-only semantics. src/server/Collectify.Infrastructure/Lookup/Igdb/IgdbBackfillRunner.csL269-L307 src/server/Collectify.Infrastructure/Lookup/Igdb/IgdbBackfillRunner.csL324-L356 - Stopped appending IGDB genre text to
Description; descriptions now contain only the provider’s summary. src/server/Collectify.Infrastructure/Lookup/Igdb/IgdbBackfillRunner.csL331-L345 - Added persistence tests covering normalized relational genres, deduplication, owner isolation, reuse of existing genres, and preservation of existing game genres. src/server/tests/Collectify.Tests/Infrastructure/IgdbBackfillRunnerTests.csL70-L131
- Committed the changes as
edeec96(fix: populate game genres during IGDB backfill) and prepared the follow-up PR metadata withCloses #174.
Testing
- ✅
mise exec dotnet@10.0.107 -- dotnet test tests/Collectify.Tests/Collectify.Tests.csproj --filter FullyQualifiedName~IgdbBackfillRunnerTests— 22 passed. - ✅
git diff --check - ✅
git status --short --branch— clean working tree onwork.
…cker) Adds a Collectify.PostgresTests fact asserting context.Database.HasPendingModelChanges() is false against the pinned postgres:17-alpine Testcontainers image -- the same in-process diff `dotnet ef migrations has-pending-model-changes` runs, comparing the runtime model to the last migration's TargetModel. RED-proofed against both annotation-only model drift (while column/manifest tests stayed green) and a Genre-entity deletion from the final PG ModelSnapshot.cs.
|
@codex — new head 044a612 (Fix-7, addressing your round-2 blocker). The PG snapshot/model-parity guard is now a COMMITTED test: PostgresSnapshotParityTests.RuntimeModel_HasNoPendingChanges_AgainstLatestMigrationSnapshot asserts context.Database.HasPendingModelChanges() is false on the Npgsql provider vs the last migration TargetModel (catches non-DDL drift too), RED-proven against both annotation-only and DDL drift. Your 2 prior P2s are dispositioned (filed #174, #175). Please re-review head 044a612. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 044a612600
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| migrationBuilder.CreateTable( | ||
| name: "GameGenre", |
There was a problem hiding this comment.
When upgrading a PostgreSQL database that already records 20260528000000_InitialPostgres, EF will never execute this newly inserted CreateTable, and the unchanged five-ID lineage contains no later genre migration. Fresh evidence at head 044a612 is that the genre DDL was folded exclusively into that already-applied initial migration; the new parity test only compares the runtime model with the rewritten snapshot and therefore cannot detect this upgrade path. Such installations retain the legacy columns but lack Genres and its join tables, so collection queries fail after deployment; add a forward migration instead of rewriting the initial one.
AGENTS.md reference: AGENTS.md:L200-L200
Useful? React with 👍 / 👎.
1 participant
Summary
Normalizes
Movie.GenresandMusicAlbum.Genresfrom comma-separated strings to a relational 1-to-many relation like tags, adds genres to games, supports genre bulk updates, and updates filtering/client behavior.Implements #169.
Changes
Genreentity +ICollection<Genre> Genresnavs on Movie, MusicAlbum, Game; bothICollectionEntryandICollectionEntryDtoexposeGenres.AddGenres): createsGenres(unique(OwnerId, Name)) + three join tables; drops the legacyMovies.Genres/MusicAlbums.Genresstring columns. No data backfill — existing comma-separated genre values are discarded (owner decision 2026-08-24, "start fresh / ok wiping").GenreResolver(lowercase/trim/dedupe, owner-scoped, mirroringTagResolver); genre resolution wired into the async create/update handler (after scalar Apply);string[]? GenresDTOs; exact-membership list filter (not substring); bulkgenresreplace-set with message-pinned 400.TagInputon Movie/Album/Game forms + details; bulk-modal genres field;genrestypes asstring[]; exact filter.Acceptance criteria
Genreentity + three join tables (Movie/MusicAlbum/Game). ✔Verification (Phase 11, driver-run at HEAD
c5f8d3a)0 Warning(s) 0 Error(s)).Collectify.Tests: 687/687 passed.npm test27 files/189 passed;npm run build✓;npm run check:enumsparity ✓.Collectify.PostgresTests: 9 failures are all Docker-connect (no local daemon); pass in CI — environment-only, unrelated to this change.genrescontinue-guard RED ×3; M5 exact-filter negative case RED; M6 bulk lowercasing RED ×3. M1 (droppingGenreResolver .Distinct()) survived benignly —.Distinct()is redundant because LINQ.Except()already dedupes and the(OwnerId,Name)unique index backstops; dedup behavior is correct. Not a correctness defect.Attribution