diff --git a/lib/artists/__tests__/deleteArtist.test.ts b/lib/artists/__tests__/deleteArtist.test.ts new file mode 100644 index 00000000..1a892ab8 --- /dev/null +++ b/lib/artists/__tests__/deleteArtist.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { deleteArtist } from "../deleteArtist"; +import { deleteAccountArtistId } from "@/lib/supabase/account_artist_ids/deleteAccountArtistId"; +import { getAccountArtistIds } from "@/lib/supabase/account_artist_ids/getAccountArtistIds"; +import { deleteAccountById } from "@/lib/supabase/accounts/deleteAccountById"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; + +vi.mock("@/lib/supabase/account_artist_ids/deleteAccountArtistId", () => ({ + deleteAccountArtistId: vi.fn(), +})); + +vi.mock("@/lib/supabase/account_artist_ids/getAccountArtistIds", () => ({ + getAccountArtistIds: vi.fn(), +})); + +vi.mock("@/lib/supabase/accounts/deleteAccountById", () => ({ + deleteAccountById: vi.fn(), +})); + +vi.mock("@/lib/supabase/song_artists/selectSongArtists", () => ({ + selectSongArtists: vi.fn(), +})); + +describe("deleteArtist", () => { + const artistId = "550e8400-e29b-41d4-a716-446655440000"; + const requesterAccountId = "660e8400-e29b-41d4-a716-446655440000"; + const link = { account_id: requesterAccountId, artist_id: artistId }; + + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(deleteAccountArtistId).mockResolvedValue([link] as never); + vi.mocked(deleteAccountById).mockResolvedValue(undefined as never); + }); + + it("throws when the requester link could not be deleted", async () => { + vi.mocked(deleteAccountArtistId).mockResolvedValue([] as never); + + await expect(deleteArtist({ artistId, requesterAccountId })).rejects.toThrow( + "Failed to delete artist link", + ); + expect(deleteAccountById).not.toHaveBeenCalled(); + }); + + it("does not hard-delete when other roster links remain", async () => { + vi.mocked(getAccountArtistIds).mockResolvedValue([ + { account_id: "another-account", artist_id: artistId }, + ] as never); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(deleteAccountById).not.toHaveBeenCalled(); + expect(selectSongArtists).not.toHaveBeenCalled(); + }); + + it("keeps the canonical account when the last link is removed but song dependencies exist", async () => { + vi.mocked(getAccountArtistIds).mockResolvedValue([] as never); + vi.mocked(selectSongArtists).mockResolvedValue([ + { id: "sa-1", song: "ISRC1", artist: artistId }, + ] as never); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(selectSongArtists).toHaveBeenCalledWith({ artists: [artistId] }); + expect(deleteAccountById).not.toHaveBeenCalled(); + }); + + it("fails closed and keeps the canonical account when the dependency lookup errors", async () => { + vi.mocked(getAccountArtistIds).mockResolvedValue([] as never); + vi.mocked(selectSongArtists).mockResolvedValue(null); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(deleteAccountById).not.toHaveBeenCalled(); + }); + + it("hard-deletes the account when the last link is removed and no dependencies exist", async () => { + vi.mocked(getAccountArtistIds).mockResolvedValue([] as never); + vi.mocked(selectSongArtists).mockResolvedValue([] as never); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(deleteAccountById).toHaveBeenCalledWith(artistId); + }); +}); diff --git a/lib/artists/deleteArtist.ts b/lib/artists/deleteArtist.ts index 330826b3..8c3dff10 100644 --- a/lib/artists/deleteArtist.ts +++ b/lib/artists/deleteArtist.ts @@ -1,6 +1,7 @@ import { deleteAccountArtistId } from "@/lib/supabase/account_artist_ids/deleteAccountArtistId"; import { getAccountArtistIds } from "@/lib/supabase/account_artist_ids/getAccountArtistIds"; import { deleteAccountById } from "@/lib/supabase/accounts/deleteAccountById"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; export interface DeleteArtistParams { artistId: string; @@ -12,7 +13,11 @@ export interface DeleteArtistParams { * * The validator is responsible for existence and access checks. This helper * only removes the direct owner link and deletes the artist account if that - * link was the last remaining association. + * link was the last remaining association AND the canonical artist has no + * song dependencies. account_artist_ids.artist_id cascades on account delete, + * so hard-deleting a canonical with song_artists rows would destroy the song + * graph shared across customers — in that case only the caller's link is + * removed and the canonical account is kept. * * @param params - Delete artist parameters * @param params.artistId - Artist account ID to remove @@ -33,7 +38,13 @@ export async function deleteArtist({ }); if (remainingLinks.length === 0) { - await deleteAccountById(artistId); + const songArtists = await selectSongArtists({ artists: [artistId] }); + // Fail closed: a null (query error) is treated as "has dependencies" so an + // unknown state never hard-deletes a canonical that may own a song graph. + const hasSongDependencies = songArtists === null || songArtists.length > 0; + if (!hasSongDependencies) { + await deleteAccountById(artistId); + } } return artistId; diff --git a/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts b/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts index 51642bb8..d5781666 100644 --- a/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts +++ b/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts @@ -1,12 +1,12 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { attachCanonicalArtistToAccount } from "../attachCanonicalArtistToAccount"; -import { selectSongArtistsBySongs } from "@/lib/supabase/song_artists/selectSongArtistsBySongs"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; import { selectAccountArtistId } from "@/lib/supabase/account_artist_ids/selectAccountArtistId"; import { insertAccountArtistId } from "@/lib/supabase/account_artist_ids/insertAccountArtistId"; -vi.mock("@/lib/supabase/song_artists/selectSongArtistsBySongs", () => ({ - selectSongArtistsBySongs: vi.fn(), +vi.mock("@/lib/supabase/song_artists/selectSongArtists", () => ({ + selectSongArtists: vi.fn(), })); vi.mock("@/lib/supabase/account_artist_ids/selectAccountArtistId", () => ({ selectAccountArtistId: vi.fn(), @@ -23,7 +23,7 @@ describe("attachCanonicalArtistToAccount", () => { beforeEach(() => vi.clearAllMocks()); it("resolves the dominant artist through song_artists and links it to the account", async () => { - vi.mocked(selectSongArtistsBySongs).mockResolvedValue([ + vi.mocked(selectSongArtists).mockResolvedValue([ link("A", canonicalId), link("B", canonicalId), link("B", "collab-1"), @@ -32,13 +32,13 @@ describe("attachCanonicalArtistToAccount", () => { const result = await attachCanonicalArtistToAccount({ accountId, isrcs: ["A", "B"] }); - expect(selectSongArtistsBySongs).toHaveBeenCalledWith(["A", "B"]); + expect(selectSongArtists).toHaveBeenCalledWith({ songs: ["A", "B"] }); expect(insertAccountArtistId).toHaveBeenCalledWith(accountId, canonicalId); expect(result).toBe(canonicalId); }); it("does not insert when the account already has the canonical artist", async () => { - vi.mocked(selectSongArtistsBySongs).mockResolvedValue([link("A", canonicalId)]); + vi.mocked(selectSongArtists).mockResolvedValue([link("A", canonicalId)]); vi.mocked(selectAccountArtistId).mockResolvedValue({ id: "existing", artist_id: canonicalId, @@ -52,7 +52,7 @@ describe("attachCanonicalArtistToAccount", () => { }); it("no-ops when the songs have no artist links yet", async () => { - vi.mocked(selectSongArtistsBySongs).mockResolvedValue([]); + vi.mocked(selectSongArtists).mockResolvedValue([]); const result = await attachCanonicalArtistToAccount({ accountId, isrcs: ["A"] }); @@ -64,13 +64,13 @@ describe("attachCanonicalArtistToAccount", () => { it("no-ops when there are no ISRCs", async () => { const result = await attachCanonicalArtistToAccount({ accountId, isrcs: [] }); - expect(selectSongArtistsBySongs).not.toHaveBeenCalled(); + expect(selectSongArtists).not.toHaveBeenCalled(); expect(result).toBeNull(); }); it("never throws: a failed attach must not fail the claim", async () => { const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - vi.mocked(selectSongArtistsBySongs).mockResolvedValue([link("A", canonicalId)]); + vi.mocked(selectSongArtists).mockResolvedValue([link("A", canonicalId)]); vi.mocked(selectAccountArtistId).mockResolvedValue(null); vi.mocked(insertAccountArtistId).mockRejectedValue(new Error("insert failed")); diff --git a/lib/catalog/attachCanonicalArtistToAccount.ts b/lib/catalog/attachCanonicalArtistToAccount.ts index e81057bd..9e120ef2 100644 --- a/lib/catalog/attachCanonicalArtistToAccount.ts +++ b/lib/catalog/attachCanonicalArtistToAccount.ts @@ -1,4 +1,4 @@ -import { selectSongArtistsBySongs } from "@/lib/supabase/song_artists/selectSongArtistsBySongs"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; import { getDominantSongArtist } from "@/lib/songs/getDominantSongArtist"; import { selectAccountArtistId } from "@/lib/supabase/account_artist_ids/selectAccountArtistId"; import { insertAccountArtistId } from "@/lib/supabase/account_artist_ids/insertAccountArtistId"; @@ -25,7 +25,7 @@ export async function attachCanonicalArtistToAccount(params: { try { if (isrcs.length === 0) return null; - const links = await selectSongArtistsBySongs(isrcs); + const links = (await selectSongArtists({ songs: isrcs })) ?? []; const artistId = getDominantSongArtist(links); if (!artistId) return null; diff --git a/lib/supabase/song_artists/__tests__/selectSongArtists.test.ts b/lib/supabase/song_artists/__tests__/selectSongArtists.test.ts new file mode 100644 index 00000000..d77ae7bf --- /dev/null +++ b/lib/supabase/song_artists/__tests__/selectSongArtists.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { selectSongArtists } from "../selectSongArtists"; + +const mockFrom = vi.fn(); +const mockSelect = vi.fn(); +const mockIn = vi.fn(); + +vi.mock("@/lib/supabase/serverClient", () => ({ + default: { + from: (...args: unknown[]) => mockFrom(...args), + }, +})); + +describe("selectSongArtists", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockFrom.mockReturnValue({ select: mockSelect }); + mockSelect.mockReturnValue({ in: mockIn }); + }); + + it("filters by song ISRCs", async () => { + const rows = [{ id: "sa-1", song: "ISRC1", artist: "artist-1" }]; + mockIn.mockResolvedValue({ data: rows, error: null }); + + const result = await selectSongArtists({ songs: ["ISRC1"] }); + + expect(mockFrom).toHaveBeenCalledWith("song_artists"); + expect(mockIn).toHaveBeenCalledWith("song", ["ISRC1"]); + expect(result).toEqual(rows); + }); + + it("filters by artist account IDs", async () => { + const rows = [{ id: "sa-1", song: "ISRC1", artist: "artist-1" }]; + mockIn.mockResolvedValue({ data: rows, error: null }); + + const result = await selectSongArtists({ artists: ["artist-1"] }); + + expect(mockIn).toHaveBeenCalledWith("artist", ["artist-1"]); + expect(result).toEqual(rows); + }); + + it("returns an empty array without querying when the filter list is empty", async () => { + const result = await selectSongArtists({ artists: [] }); + + expect(result).toEqual([]); + expect(mockFrom).not.toHaveBeenCalled(); + }); + + it("chunks large filter lists to keep .in() within URL limits", async () => { + mockIn.mockResolvedValue({ data: [], error: null }); + const songs = Array.from({ length: 450 }, (_, i) => `ISRC${i}`); + + await selectSongArtists({ songs }); + + // 450 / 200 => 3 chunks + expect(mockIn).toHaveBeenCalledTimes(3); + }); + + it("returns null on query error (callers decide how to treat an unknown state)", async () => { + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + mockIn.mockResolvedValue({ data: null, error: { message: "boom" } }); + + const result = await selectSongArtists({ artists: ["artist-1"] }); + + expect(result).toBeNull(); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it("throws when neither songs nor artists is provided", async () => { + await expect(selectSongArtists({})).rejects.toThrow("Must provide either songs or artists"); + }); +}); diff --git a/lib/supabase/song_artists/__tests__/selectSongArtistsBySongs.test.ts b/lib/supabase/song_artists/__tests__/selectSongArtistsBySongs.test.ts deleted file mode 100644 index 9cf39220..00000000 --- a/lib/supabase/song_artists/__tests__/selectSongArtistsBySongs.test.ts +++ /dev/null @@ -1,62 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; - -import { selectSongArtistsBySongs } from "../selectSongArtistsBySongs"; - -const mockFrom = vi.fn(); -const mockSelect = vi.fn(); -const mockIn = vi.fn(); - -vi.mock("@/lib/supabase/serverClient", () => ({ - default: { - from: (...args: unknown[]) => mockFrom(...args), - }, -})); - -const row = (song: string, artist: string) => ({ id: `${song}-${artist}`, song, artist }); - -describe("selectSongArtistsBySongs", () => { - beforeEach(() => { - vi.clearAllMocks(); - mockFrom.mockReturnValue({ select: mockSelect }); - mockSelect.mockReturnValue({ in: mockIn }); - }); - - it("returns [] without querying when no songs are given", async () => { - const result = await selectSongArtistsBySongs([]); - - expect(result).toEqual([]); - expect(mockFrom).not.toHaveBeenCalled(); - }); - - it("selects song_artists rows for the given ISRCs", async () => { - mockIn.mockResolvedValue({ data: [row("A", "artist-1"), row("B", "artist-1")], error: null }); - - const result = await selectSongArtistsBySongs(["A", "B"]); - - expect(mockFrom).toHaveBeenCalledWith("song_artists"); - expect(mockIn).toHaveBeenCalledWith("song", ["A", "B"]); - expect(result).toHaveLength(2); - }); - - it("chunks large ISRC batches to keep the PostgREST filter bounded", async () => { - const songs = Array.from({ length: 450 }, (_, i) => `ISRC${i}`); - mockIn.mockResolvedValue({ data: [row("ISRC0", "artist-1")], error: null }); - - await selectSongArtistsBySongs(songs); - - expect(mockIn).toHaveBeenCalledTimes(3); // 200 + 200 + 50 - expect(mockIn.mock.calls[0][1]).toHaveLength(200); - expect(mockIn.mock.calls[2][1]).toHaveLength(50); - }); - - it("returns [] and logs on query error", async () => { - const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - mockIn.mockResolvedValue({ data: null, error: { message: "boom" } }); - - const result = await selectSongArtistsBySongs(["A"]); - - expect(result).toEqual([]); - expect(consoleSpy).toHaveBeenCalled(); - consoleSpy.mockRestore(); - }); -}); diff --git a/lib/supabase/song_artists/selectSongArtists.ts b/lib/supabase/song_artists/selectSongArtists.ts new file mode 100644 index 00000000..327a36b3 --- /dev/null +++ b/lib/supabase/song_artists/selectSongArtists.ts @@ -0,0 +1,45 @@ +import supabase from "../serverClient"; +import type { Tables } from "@/types/database.types"; + +// PostgREST encodes .in() filters in the query string — chunk so large +// inputs (1,000+ ISRCs) can't overflow the URL. +const CHUNK_SIZE = 200; + +/** + * Select song_artists rows filtered by song ISRCs or artist account IDs. + * + * Provide exactly one of `songs` or `artists` (songs takes precedence if both + * are given). The filter list is chunked to keep `.in()` within URL limits. + * + * @param params.songs - Song ISRCs to match on the `song` column + * @param params.artists - Artist account IDs to match on the `artist` column + * @returns The matching rows, or null on query error + */ +export async function selectSongArtists(params: { + songs?: string[]; + artists?: string[]; +}): Promise[] | null> { + const { songs, artists } = params; + const column = songs ? "song" : artists ? "artist" : null; + const values = songs ?? artists; + + if (!column || !values) { + throw new Error("Must provide either songs or artists"); + } + if (values.length === 0) return []; + + const rows: Tables<"song_artists">[] = []; + for (let i = 0; i < values.length; i += CHUNK_SIZE) { + const chunk = values.slice(i, i + CHUNK_SIZE); + const { data, error } = await supabase.from("song_artists").select("*").in(column, chunk); + + if (error) { + console.error("Error fetching song_artists:", error); + return null; + } + + rows.push(...(data ?? [])); + } + + return rows; +} diff --git a/lib/supabase/song_artists/selectSongArtistsBySongs.ts b/lib/supabase/song_artists/selectSongArtistsBySongs.ts deleted file mode 100644 index cd5a2883..00000000 --- a/lib/supabase/song_artists/selectSongArtistsBySongs.ts +++ /dev/null @@ -1,31 +0,0 @@ -import supabase from "../serverClient"; -import type { Tables } from "@/types/database.types"; - -// PostgREST encodes .in() filters in the query string — chunk so large -// catalogs (1,000+ ISRCs) can't overflow the URL. -const CHUNK_SIZE = 200; - -/** - * Select song_artists rows for a batch of song ISRCs. - * - * @param songs - The song ISRCs to fetch artist links for - * @returns The matching rows, or [] if none exist or on error - */ -export async function selectSongArtistsBySongs(songs: string[]): Promise[]> { - if (songs.length === 0) return []; - - const rows: Tables<"song_artists">[] = []; - for (let i = 0; i < songs.length; i += CHUNK_SIZE) { - const chunk = songs.slice(i, i + CHUNK_SIZE); - const { data, error } = await supabase.from("song_artists").select("*").in("song", chunk); - - if (error) { - console.error("Error fetching song_artists:", error); - return []; - } - - rows.push(...(data ?? [])); - } - - return rows; -}