diff --git a/README.md b/README.md index 71f69a2..e27f941 100644 --- a/README.md +++ b/README.md @@ -135,6 +135,36 @@ reproducibility. See [MusicBrainz Data License](https://musicbrainz.org/doc/Abou and [MusicBrainz API](https://musicbrainz.org/doc/MusicBrainz_API). Album covers remain out of scope for this issue. +### Catalog quality (#64) + +We checked the snapshot and the imported catalog (516 albums with the sample +seed) for duplicates, missing metadata, artist and genre links and track order. +Every sort was paged through the whole result set without gaps or repeats, and +the facet counts match the filtered totals. Fixes from the check: + +- MusicBrainz writes hyphens as U+2010, so "esa-pekka" or "switched-on" found + nothing. The importer now stores ASCII hyphens. +- Artists are shown in credit order ("Bob James, David Sanborn"), not in the + order their rows happened to be created. +- A reimport replaces artists, genres and tracks the snapshot no longer has, + instead of keeping the old ones next to the new ones. Reviews and crate items + are kept. + +Remaining limitations: + +- Only 50 of the 500 albums have a track list, taken from one official release + of the release group. The album page says when there are no tracks. +- Seven albums have no release year. They sort last and have no decade facet. +- Search is not accent-insensitive: "beyonce" does not find "Beyoncé", and + names in other scripts (for example Mulatu Astatke as ሙላቱ አስታጥቄ) only match + as written. +- Artists are matched by name, so two different artists with the same name + would share a row. None were found in the snapshot. +- Classical albums credit the composer as an artist, and compilations use + "Various Artists", as MusicBrainz does. +- Same title and artist is not always a duplicate: Color Humano released three + albums called "Color Humano" (1972-1974), kept as three albums. + ### Scalability check Use a separate test database, not the shared catalog. Apply migrations, run the large seed and start the API. The SQL checks and GraphQL request are committed in the repository. diff --git a/docs/AI-USAGE.md b/docs/AI-USAGE.md index 7cb270d..4612092 100644 --- a/docs/AI-USAGE.md +++ b/docs/AI-USAGE.md @@ -75,6 +75,7 @@ the part that matters most. | 2026-10-28 | evenkkl | Claude | #12, review fixes on #66. Robert asked for an up-to-date branch, a correct README and green CI. Claude merged main, renumbered our migration to `0003` after #63's `0002`, kept both sides of the other conflicts, fixed a duplicate import from the merge, updated the README status sections and added the sort control to the Load more keyboard test. I checked the diff against main contains only our sorting work and these fixes. | | 2026-10-28 | evenkkl | Claude | #13 review fixes on PR #65: merged main (infinite scroll), resolved conflicts, added a Filters button for small screens and reconciled the README. Even settled the open review questions and updated the issue text. | | 2026-10-29 | evenkkl | Claude | #13 second review round on PR #65: merged main (sorting), selected filters stay in the facets, Filters dropdown on all screens, PostgreSQL tests for AND/OR and facet counts, doc wording. Even chose the dropdown layout. | +| 2026-11-05 | evenkkl | Claude | #64 catalog check: scripts that compared the snapshot and the imported catalog against search, sorting, paging and facets; fixes for U+2010 hyphens in search, artist credit order and stale links on reimport; PostgreSQL tests and README limitations. | ## Notes diff --git a/docs/data-model.md b/docs/data-model.md index b9cc70a..7e825c0 100644 --- a/docs/data-model.md +++ b/docs/data-model.md @@ -21,7 +21,10 @@ catalog data. The separate MusicBrainz importer adds real catalog records from the committed snapshot without deleting albums, tracks, reviews or crate items. It matches an existing hand-seeded album by title and primary artist before assigning its MusicBrainz ID, preserving its database ID and user data. The -unique MusicBrainz release-group ID makes repeat imports idempotent. +unique MusicBrainz release-group ID makes repeat imports idempotent. For the +albums in the snapshot, a reimport replaces artist links, genre links and +tracks with the snapshot's, and removes artists and genres it left unused. +`album_artist.position` keeps the credit order (migration `0004`). The local snapshot keeps one record per release group, not per pressing or reissue. It stores normalized genre names and any available track data. The diff --git a/server/drizzle/0004_album-artist-position.sql b/server/drizzle/0004_album-artist-position.sql new file mode 100644 index 0000000..58dc35e --- /dev/null +++ b/server/drizzle/0004_album-artist-position.sql @@ -0,0 +1 @@ +ALTER TABLE "album_artist" ADD COLUMN "position" smallint DEFAULT 0 NOT NULL; \ No newline at end of file diff --git a/server/drizzle/meta/0004_snapshot.json b/server/drizzle/meta/0004_snapshot.json new file mode 100644 index 0000000..29e2ee3 --- /dev/null +++ b/server/drizzle/meta/0004_snapshot.json @@ -0,0 +1,604 @@ +{ + "id": "eae9124a-5b33-46a1-9607-f1c162a1d10a", + "prevId": "82666a74-d81e-4ae3-85d0-a0ff16223746", + "version": "7", + "dialect": "postgresql", + "tables": { + "public.album": { + "name": "album", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "title": { + "name": "title", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "release_year": { + "name": "release_year", + "type": "integer", + "primaryKey": false, + "notNull": false + }, + "musicbrainz_release_group_id": { + "name": "musicbrainz_release_group_id", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "search_vector": { + "name": "search_vector", + "type": "tsvector", + "primaryKey": false, + "notNull": false + }, + "average_rating": { + "name": "average_rating", + "type": "double precision", + "primaryKey": false, + "notNull": false + } + }, + "indexes": { + "album_search_vector_idx": { + "name": "album_search_vector_idx", + "columns": [ + { + "expression": "search_vector", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "gin", + "with": {} + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": { + "album_musicbrainz_release_group_id_unique": { + "name": "album_musicbrainz_release_group_id_unique", + "nullsNotDistinct": false, + "columns": ["musicbrainz_release_group_id"] + } + }, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.album_artist": { + "name": "album_artist", + "schema": "", + "columns": { + "album_id": { + "name": "album_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "artist_id": { + "name": "artist_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "position": { + "name": "position", + "type": "smallint", + "primaryKey": false, + "notNull": true, + "default": 0 + } + }, + "indexes": {}, + "foreignKeys": { + "album_artist_album_id_album_id_fk": { + "name": "album_artist_album_id_album_id_fk", + "tableFrom": "album_artist", + "tableTo": "album", + "columnsFrom": ["album_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + }, + "album_artist_artist_id_artist_id_fk": { + "name": "album_artist_artist_id_artist_id_fk", + "tableFrom": "album_artist", + "tableTo": "artist", + "columnsFrom": ["artist_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": { + "album_artist_album_id_artist_id_pk": { + "name": "album_artist_album_id_artist_id_pk", + "columns": ["album_id", "artist_id"] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.album_genre": { + "name": "album_genre", + "schema": "", + "columns": { + "album_id": { + "name": "album_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "genre_id": { + "name": "genre_id", + "type": "integer", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": { + "album_genre_album_id_album_id_fk": { + "name": "album_genre_album_id_album_id_fk", + "tableFrom": "album_genre", + "tableTo": "album", + "columnsFrom": ["album_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + }, + "album_genre_genre_id_genre_id_fk": { + "name": "album_genre_genre_id_genre_id_fk", + "tableFrom": "album_genre", + "tableTo": "genre", + "columnsFrom": ["genre_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": { + "album_genre_album_id_genre_id_pk": { + "name": "album_genre_album_id_genre_id_pk", + "columns": ["album_id", "genre_id"] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.app_user": { + "name": "app_user", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "uuid", + "primaryKey": true, + "notNull": true + }, + "display_name": { + "name": "display_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "created_at": { + "name": "created_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true, + "default": "now()" + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.artist": { + "name": "artist", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.crate_item": { + "name": "crate_item", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "user_id": { + "name": "user_id", + "type": "uuid", + "primaryKey": false, + "notNull": true + }, + "album_id": { + "name": "album_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "added_at": { + "name": "added_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true, + "default": "now()" + } + }, + "indexes": { + "crate_item_user_album_unique": { + "name": "crate_item_user_album_unique", + "columns": [ + { + "expression": "user_id", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "album_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": true, + "concurrently": false, + "method": "btree", + "with": {} + }, + "crate_item_user_added_at_idx": { + "name": "crate_item_user_added_at_idx", + "columns": [ + { + "expression": "user_id", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "added_at", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } + }, + "foreignKeys": { + "crate_item_user_id_app_user_id_fk": { + "name": "crate_item_user_id_app_user_id_fk", + "tableFrom": "crate_item", + "tableTo": "app_user", + "columnsFrom": ["user_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + }, + "crate_item_album_id_album_id_fk": { + "name": "crate_item_album_id_album_id_fk", + "tableFrom": "crate_item", + "tableTo": "album", + "columnsFrom": ["album_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.genre": { + "name": "genre", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": { + "genre_name_unique": { + "name": "genre_name_unique", + "nullsNotDistinct": false, + "columns": ["name"] + } + }, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.review": { + "name": "review", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "user_id": { + "name": "user_id", + "type": "uuid", + "primaryKey": false, + "notNull": true + }, + "album_id": { + "name": "album_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "rating": { + "name": "rating", + "type": "smallint", + "primaryKey": false, + "notNull": true + }, + "comment": { + "name": "comment", + "type": "text", + "primaryKey": false, + "notNull": true, + "default": "''" + }, + "created_at": { + "name": "created_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true, + "default": "now()" + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true, + "default": "now()" + } + }, + "indexes": { + "review_user_album_unique": { + "name": "review_user_album_unique", + "columns": [ + { + "expression": "user_id", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "album_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": true, + "concurrently": false, + "method": "btree", + "with": {} + }, + "review_album_idx": { + "name": "review_album_idx", + "columns": [ + { + "expression": "album_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } + }, + "foreignKeys": { + "review_user_id_app_user_id_fk": { + "name": "review_user_id_app_user_id_fk", + "tableFrom": "review", + "tableTo": "app_user", + "columnsFrom": ["user_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + }, + "review_album_id_album_id_fk": { + "name": "review_album_id_album_id_fk", + "tableFrom": "review", + "tableTo": "album", + "columnsFrom": ["album_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": { + "review_rating_range": { + "name": "review_rating_range", + "value": "\"review\".\"rating\" BETWEEN 1 AND 5" + } + }, + "isRLSEnabled": false + }, + "public.track": { + "name": "track", + "schema": "", + "columns": { + "id": { + "name": "id", + "type": "serial", + "primaryKey": true, + "notNull": true + }, + "album_id": { + "name": "album_id", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "title": { + "name": "title", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "track_number": { + "name": "track_number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "duration_seconds": { + "name": "duration_seconds", + "type": "integer", + "primaryKey": false, + "notNull": false + } + }, + "indexes": { + "track_album_track_number_unique": { + "name": "track_album_track_number_unique", + "columns": [ + { + "expression": "album_id", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "track_number", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": true, + "concurrently": false, + "method": "btree", + "with": {} + }, + "track_album_track_number_idx": { + "name": "track_album_track_number_idx", + "columns": [ + { + "expression": "album_id", + "isExpression": false, + "asc": true, + "nulls": "last" + }, + { + "expression": "track_number", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } + }, + "foreignKeys": { + "track_album_id_album_id_fk": { + "name": "track_album_id_album_id_fk", + "tableFrom": "track", + "tableTo": "album", + "columnsFrom": ["album_id"], + "columnsTo": ["id"], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + } + }, + "enums": {}, + "schemas": {}, + "sequences": {}, + "roles": {}, + "policies": {}, + "views": {}, + "_meta": { + "columns": {}, + "schemas": {}, + "tables": {} + } +} diff --git a/server/drizzle/meta/_journal.json b/server/drizzle/meta/_journal.json index 4eb27ca..23ce9e2 100644 --- a/server/drizzle/meta/_journal.json +++ b/server/drizzle/meta/_journal.json @@ -29,6 +29,13 @@ "when": 1793199189807, "tag": "0003_album-average-rating", "breakpoints": true + }, + { + "idx": 4, + "version": "7", + "when": 1793889343448, + "tag": "0004_album-artist-position", + "breakpoints": true } ] } diff --git a/server/src/db/musicBrainzCatalog.test.ts b/server/src/db/musicBrainzCatalog.test.ts index 6981349..c7ddd66 100644 --- a/server/src/db/musicBrainzCatalog.test.ts +++ b/server/src/db/musicBrainzCatalog.test.ts @@ -1,7 +1,7 @@ import { readFile } from 'node:fs/promises'; import { randomUUID } from 'node:crypto'; import { drizzle } from 'drizzle-orm/node-postgres'; -import { eq } from 'drizzle-orm'; +import { asc, eq } from 'drizzle-orm'; import pg from 'pg'; import { afterAll, describe, expect, it } from 'vitest'; import { @@ -11,9 +11,11 @@ import { import { album, albumArtist, + albumGenre, appUser, artist, crateItem, + genre, review, track, } from './schema.js'; @@ -42,6 +44,28 @@ describe('MusicBrainz catalog snapshot', () => { ).toBe(true); }); + it('stores ASCII hyphens and single spaces so searches for them match', () => { + const [parsed] = parseMusicBrainzCatalog({ + albums: [ + { + releaseGroupId: 'd7f4c5cc-3b9b-4e6e-8e4d-4ae6b7020f50', + title: 'Switched‐On Bach', + releaseYear: 1968, + artists: ['Esa‐Pekka Salonen', 'Esa-Pekka Salonen'], + genres: [' Classical '], + tracks: [{ title: 'Air‑on G ', trackNumber: 1, durationSeconds: null }], + }, + ], + }); + expect(parsed).toMatchObject({ + title: 'Switched-On Bach', + artists: ['Esa-Pekka Salonen'], + genres: ['Classical'], + tracks: [{ title: 'Air-on G' }], + }); + expect(JSON.stringify(catalog)).not.toMatch(/[‐‑]/); + }); + it('rejects repeated release groups and malformed catalog records', () => { const record = { releaseGroupId: 'd7f4c5cc-3b9b-4e6e-8e4d-4ae6b7020f50', @@ -151,6 +175,82 @@ describe.skipIf(!process.env.TEST_DATABASE_URL)('MusicBrainz catalog import', () } }); + it('replaces changed artists, genres and tracks on reimport and keeps user data', async () => { + const id = randomUUID(); + const userId = randomUUID(); + const names = ['First', 'Second', 'Old'].map((name) => `${name} artist ${id}`); + const genres = ['New', 'Old'].map((name) => `${name} genre ${id}`); + const before = { + releaseGroupId: id, + title: `Reimport fixture ${id}`, + releaseYear: 2001, + artists: [names[2]!, names[1]!], + genres: [genres[1]!], + tracks: [1, 2, 3].map((trackNumber) => ({ + title: `Old track ${trackNumber}`, + trackNumber, + durationSeconds: 60, + })), + }; + // Second is created first, so it has the lower artist id. + const after = { + ...before, + artists: [names[0]!, names[1]!], + genres: [genres[0]!], + tracks: [{ title: 'New track', trackNumber: 1, durationSeconds: 90 }], + }; + + try { + await importMusicBrainzAlbums(database, [before]); + const [imported] = await database + .select() + .from(album) + .where(eq(album.musicBrainzReleaseGroupId, id)); + await database.insert(appUser).values({ id: userId, displayName: 'Reimport user' }); + await database.insert(review).values({ userId, albumId: imported!.id, rating: 3 }); + await database.insert(crateItem).values({ userId, albumId: imported!.id }); + + await importMusicBrainzAlbums(database, [after]); + + const artists = await database + .select({ name: artist.name }) + .from(albumArtist) + .innerJoin(artist, eq(artist.id, albumArtist.artistId)) + .where(eq(albumArtist.albumId, imported!.id)) + .orderBy(asc(albumArtist.position), asc(artist.id)); + const linkedGenres = await database + .select({ name: genre.name }) + .from(albumGenre) + .innerJoin(genre, eq(genre.id, albumGenre.genreId)) + .where(eq(albumGenre.albumId, imported!.id)); + const tracks = await database + .select({ title: track.title }) + .from(track) + .where(eq(track.albumId, imported!.id)); + expect(artists.map((row) => row.name)).toEqual([names[0], names[1]]); + expect(linkedGenres.map((row) => row.name)).toEqual([genres[0]]); + expect(tracks.map((row) => row.title)).toEqual(['New track']); + // The artist and genre no album uses any more are gone. + expect( + await database.select().from(artist).where(eq(artist.name, names[2]!)), + ).toHaveLength(0); + expect( + await database.select().from(genre).where(eq(genre.name, genres[1]!)), + ).toHaveLength(0); + expect( + await database.select().from(review).where(eq(review.userId, userId)), + ).toHaveLength(1); + expect( + await database.select().from(crateItem).where(eq(crateItem.userId, userId)), + ).toHaveLength(1); + } finally { + await pool.query('DELETE FROM app_user WHERE id = $1', [userId]); + await pool.query('DELETE FROM album WHERE musicbrainz_release_group_id = $1', [id]); + await pool.query('DELETE FROM genre WHERE name = ANY($1)', [genres]); + await pool.query('DELETE FROM artist WHERE name = ANY($1)', [names]); + } + }); + it('keeps same-title albums with different release-group IDs separate', async () => { const id = randomUUID(); const title = `Same title fixture ${id}`; diff --git a/server/src/db/musicBrainzCatalog.ts b/server/src/db/musicBrainzCatalog.ts index a0196bc..b22d3ad 100644 --- a/server/src/db/musicBrainzCatalog.ts +++ b/server/src/db/musicBrainzCatalog.ts @@ -1,4 +1,4 @@ -import { and, eq, isNull } from 'drizzle-orm'; +import { and, eq, isNull, notInArray, sql } from 'drizzle-orm'; import { album, albumArtist, albumGenre, artist, genre, track } from './schema.js'; import type { Db } from './client.js'; @@ -17,6 +17,15 @@ export type MusicBrainzAlbum = { tracks: MusicBrainzTrack[]; }; +/** + * MusicBrainz writes hyphens as U+2010 ("Esa-Pekka", "Switched-On"). The + * search tokenizer only joins words around an ASCII hyphen, so a search for + * "esa-pekka" found nothing. Store the ASCII hyphen and collapse whitespace. + */ +export function cleanText(value: string): string { + return value.normalize('NFC').replace(/[‐‑]/g, '-').replace(/\s+/g, ' ').trim(); +} + export function parseMusicBrainzCatalog(value: unknown): MusicBrainzAlbum[] { if (!value || typeof value !== 'object' || !('albums' in value)) { throw new Error('MusicBrainz catalog must contain an albums array'); @@ -96,7 +105,7 @@ export function parseMusicBrainzCatalog(value: unknown): MusicBrainzAlbum[] { throw new Error(`Album ${index} track ${trackIndex} has an invalid duration`); } return { - title: item.title, + title: cleanText(item.title), trackNumber: item.trackNumber as number, durationSeconds: item.durationSeconds as number | null, }; @@ -104,10 +113,10 @@ export function parseMusicBrainzCatalog(value: unknown): MusicBrainzAlbum[] { return { releaseGroupId, - title: title.trim(), + title: cleanText(title), releaseYear: releaseYear as number | null, - artists: [...new Set((artists as string[]).map((name) => name.trim()))], - genres: [...new Set((genres as string[]).map((name) => name.trim()))], + artists: [...new Set((artists as string[]).map(cleanText))], + genres: [...new Set((genres as string[]).map(cleanText))], tracks: normalizedTracks, }; }); @@ -116,6 +125,8 @@ export function parseMusicBrainzCatalog(value: unknown): MusicBrainzAlbum[] { export async function importMusicBrainzAlbums(database: Db, records: MusicBrainzAlbum[]) { let inserted = 0; let updated = 0; + const unlinkedArtistIds = new Set(); + const unlinkedGenreIds = new Set(); await database.transaction(async (transaction) => { const findOrCreateArtist = async (name: string): Promise => { @@ -192,23 +203,56 @@ export async function importMusicBrainzAlbums(database: Db, records: MusicBrainz updated += 1; } + // The snapshot is the source of truth for its albums' artists, genres + // and tracks. A reimport replaces links and tracks that the snapshot no + // longer has, instead of leaving the old ones next to the new ones. + // Reviews and crate items hang off the album ID and are not touched. const artistIds: number[] = []; for (const name of record.artists) artistIds.push(await findOrCreateArtist(name)); - for (const artistId of artistIds) { + const removedArtists = await transaction + .delete(albumArtist) + .where( + and( + eq(albumArtist.albumId, albumId), + notInArray(albumArtist.artistId, artistIds), + ), + ) + .returning({ id: albumArtist.artistId }); + for (const { id } of removedArtists) unlinkedArtistIds.add(id); + for (const [position, artistId] of artistIds.entries()) { await transaction .insert(albumArtist) - .values({ albumId, artistId }) - .onConflictDoNothing(); + .values({ albumId, artistId, position }) + .onConflictDoUpdate({ + target: [albumArtist.albumId, albumArtist.artistId], + set: { position }, + }); } - for (const name of record.genres) { - const genreId = await findOrCreateGenre(name); + const genreIds: number[] = []; + for (const name of record.genres) genreIds.push(await findOrCreateGenre(name)); + const removedGenres = await transaction + .delete(albumGenre) + .where( + and(eq(albumGenre.albumId, albumId), notInArray(albumGenre.genreId, genreIds)), + ) + .returning({ id: albumGenre.genreId }); + for (const { id } of removedGenres) unlinkedGenreIds.add(id); + for (const genreId of genreIds) { await transaction .insert(albumGenre) .values({ albumId, genreId }) .onConflictDoNothing(); } + const trackNumbers = record.tracks.map((item) => item.trackNumber); + await transaction + .delete(track) + .where( + trackNumbers.length + ? and(eq(track.albumId, albumId), notInArray(track.trackNumber, trackNumbers)) + : eq(track.albumId, albumId), + ); for (const item of record.tracks) { await transaction .insert(track) @@ -224,6 +268,21 @@ export async function importMusicBrainzAlbums(database: Db, records: MusicBrainz }); } } + + // A renamed artist or genre leaves a row no album uses any more. Only + // rows this import unlinked are candidates, so it never touches others. + if (unlinkedArtistIds.size) { + await transaction.execute(sql` + DELETE FROM artist + WHERE id IN (${sql.join([...unlinkedArtistIds], sql`, `)}) + AND NOT EXISTS (SELECT 1 FROM album_artist WHERE artist_id = artist.id)`); + } + if (unlinkedGenreIds.size) { + await transaction.execute(sql` + DELETE FROM genre + WHERE id IN (${sql.join([...unlinkedGenreIds], sql`, `)}) + AND NOT EXISTS (SELECT 1 FROM album_genre WHERE genre_id = genre.id)`); + } }); return { inserted, updated }; diff --git a/server/src/db/schema.ts b/server/src/db/schema.ts index b3f30f3..1b8fa8e 100644 --- a/server/src/db/schema.ts +++ b/server/src/db/schema.ts @@ -56,6 +56,9 @@ export const albumArtist = pgTable( artistId: integer('artist_id') .notNull() .references(() => artist.id, { onDelete: 'cascade' }), + // Credit order from the source, so "Bob James & David Sanborn" is not + // shown the other way round because one artist row happened to exist first. + position: smallint('position').notNull().default(0), }, (t) => [primaryKey({ columns: [t.albumId, t.artistId] })], ); diff --git a/server/src/db/seed.ts b/server/src/db/seed.ts index 7983a5f..13ddabb 100644 --- a/server/src/db/seed.ts +++ b/server/src/db/seed.ts @@ -83,9 +83,13 @@ await db.transaction(async (tx) => { .values({ title: a.title, releaseYear: a.year }) .returning({ id: album.id }); const albumId = row!.id; - await tx - .insert(albumArtist) - .values(a.artists.map((n) => ({ albumId, artistId: artistId.get(n)! }))); + await tx.insert(albumArtist).values( + a.artists.map((n, position) => ({ + albumId, + artistId: artistId.get(n)!, + position, + })), + ); await tx .insert(albumGenre) .values(a.genres.map((n) => ({ albumId, genreId: genreId.get(n)! }))); diff --git a/server/src/loaders/index.ts b/server/src/loaders/index.ts index 8e0756c..aecefe7 100644 --- a/server/src/loaders/index.ts +++ b/server/src/loaders/index.ts @@ -40,7 +40,7 @@ export function createLoaders(db: Db, userId: string | null) { .from(albumArtist) .innerJoin(artist, eq(artist.id, albumArtist.artistId)) .where(inArray(albumArtist.albumId, [...albumIds])) - .orderBy(asc(artist.id)); + .orderBy(asc(albumArtist.position), asc(artist.id)); return groupByAlbum(albumIds, rows); }),