From 19033b14c8972228ff526d610c1ed9748ba57b59 Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Wed, 8 Jul 2026 16:37:42 -0500 Subject: [PATCH 1/3] fix(artists): deleteArtist must not hard-delete a shared canonical with song dependencies deleteArtist removed the caller's roster link, then hard-deleted the canonical artist account whenever zero links remained across all accounts. account_artist_ids.artist_id is ON DELETE CASCADE, so this destroyed song_artists and the shared song graph for a canonical with a catalog but a single roster link. Guard the deleteAccountById call with a song_artists existence check (fails safe on query error) so the canonical account and song graph survive; artists with no dependencies still hard-delete as before. Fixes recoupable/chat#1860 (promoted deleteArtist-guard item). Co-Authored-By: Claude Fable 5 --- lib/artists/__tests__/deleteArtist.test.ts | 77 +++++++++++++++++++ lib/artists/deleteArtist.ts | 12 ++- .../getSongArtistExistsByArtist.test.ts | 50 ++++++++++++ .../getSongArtistExistsByArtist.ts | 24 ++++++ 4 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 lib/artists/__tests__/deleteArtist.test.ts create mode 100644 lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts create mode 100644 lib/supabase/song_artists/getSongArtistExistsByArtist.ts diff --git a/lib/artists/__tests__/deleteArtist.test.ts b/lib/artists/__tests__/deleteArtist.test.ts new file mode 100644 index 000000000..9ef0c265c --- /dev/null +++ b/lib/artists/__tests__/deleteArtist.test.ts @@ -0,0 +1,77 @@ +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 { getSongArtistExistsByArtist } from "@/lib/supabase/song_artists/getSongArtistExistsByArtist"; + +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/getSongArtistExistsByArtist", () => ({ + getSongArtistExistsByArtist: 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(getSongArtistExistsByArtist).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(getSongArtistExistsByArtist).mockResolvedValue(true); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(getSongArtistExistsByArtist).toHaveBeenCalledWith(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(getSongArtistExistsByArtist).mockResolvedValue(false); + + 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 330826b39..253e441fa 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 { getSongArtistExistsByArtist } from "@/lib/supabase/song_artists/getSongArtistExistsByArtist"; 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,10 @@ export async function deleteArtist({ }); if (remainingLinks.length === 0) { - await deleteAccountById(artistId); + const hasSongDependencies = await getSongArtistExistsByArtist(artistId); + if (!hasSongDependencies) { + await deleteAccountById(artistId); + } } return artistId; diff --git a/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts b/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts new file mode 100644 index 000000000..5921a4f2a --- /dev/null +++ b/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { getSongArtistExistsByArtist } from "../getSongArtistExistsByArtist"; + +const mockFrom = vi.fn(); +const mockSelect = vi.fn(); +const mockEq = vi.fn(); + +vi.mock("@/lib/supabase/serverClient", () => ({ + default: { + from: (...args: unknown[]) => mockFrom(...args), + }, +})); + +describe("getSongArtistExistsByArtist", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockFrom.mockReturnValue({ select: mockSelect }); + mockSelect.mockReturnValue({ eq: mockEq }); + }); + + it("returns true when song_artists rows exist for the artist", async () => { + mockEq.mockResolvedValue({ count: 3, error: null }); + + const result = await getSongArtistExistsByArtist("artist-1"); + + expect(mockFrom).toHaveBeenCalledWith("song_artists"); + expect(mockEq).toHaveBeenCalledWith("artist", "artist-1"); + expect(result).toBe(true); + }); + + it("returns false when no song_artists rows exist", async () => { + mockEq.mockResolvedValue({ count: 0, error: null }); + + const result = await getSongArtistExistsByArtist("artist-1"); + + expect(result).toBe(false); + }); + + it("fails safe by returning true on query error", async () => { + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + mockEq.mockResolvedValue({ count: null, error: { message: "boom" } }); + + const result = await getSongArtistExistsByArtist("artist-1"); + + expect(result).toBe(true); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/lib/supabase/song_artists/getSongArtistExistsByArtist.ts b/lib/supabase/song_artists/getSongArtistExistsByArtist.ts new file mode 100644 index 000000000..de70d9d30 --- /dev/null +++ b/lib/supabase/song_artists/getSongArtistExistsByArtist.ts @@ -0,0 +1,24 @@ +import supabase from "../serverClient"; + +/** + * Check whether any song_artists rows reference the given artist account. + * + * Used as a hard-delete guard: on query error this fails safe and returns + * true so callers never treat an unknown state as "no dependencies". + * + * @param artistId - Artist account ID to check + * @returns True if at least one song_artists row exists (or the check failed) + */ +export async function getSongArtistExistsByArtist(artistId: string): Promise { + const { count, error } = await supabase + .from("song_artists") + .select("id", { count: "exact", head: true }) + .eq("artist", artistId); + + if (error) { + console.error("Error checking song_artists for artist:", error); + return true; + } + + return (count ?? 0) > 0; +} From d986c433206a5f0263356bdb26382bb27aa357f8 Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Thu, 9 Jul 2026 12:33:54 -0500 Subject: [PATCH 2/3] refactor(song_artists): replace bespoke existence helper with generic selectSongArtistsByArtist Per review: supabase libs should be generic/reusable, not single-purpose. Swap getSongArtistExistsByArtist (boolean, fail-safe baked in) for a generic selectSongArtistsByArtist(artist) that returns rows (null on error), mirroring selectSongArtistsBySongs. The fail-closed policy moves into deleteArtist where it is domain logic: a null (query error) or any rows => keep the canonical; only an empty array hard-deletes. Behavior preserved. 111 tests green (added a fail-closed deleteArtist case for the null/error path); tsc + lint + prettier clean on touched files. Co-Authored-By: Claude Opus 4.8 --- lib/artists/__tests__/deleteArtist.test.ts | 26 +++++++--- lib/artists/deleteArtist.ts | 7 ++- .../getSongArtistExistsByArtist.test.ts | 50 ------------------ .../selectSongArtistsByArtist.test.ts | 51 +++++++++++++++++++ .../getSongArtistExistsByArtist.ts | 24 --------- .../song_artists/selectSongArtistsByArtist.ts | 21 ++++++++ 6 files changed, 96 insertions(+), 83 deletions(-) delete mode 100644 lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts create mode 100644 lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts delete mode 100644 lib/supabase/song_artists/getSongArtistExistsByArtist.ts create mode 100644 lib/supabase/song_artists/selectSongArtistsByArtist.ts diff --git a/lib/artists/__tests__/deleteArtist.test.ts b/lib/artists/__tests__/deleteArtist.test.ts index 9ef0c265c..d1858679d 100644 --- a/lib/artists/__tests__/deleteArtist.test.ts +++ b/lib/artists/__tests__/deleteArtist.test.ts @@ -4,7 +4,7 @@ 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 { getSongArtistExistsByArtist } from "@/lib/supabase/song_artists/getSongArtistExistsByArtist"; +import { selectSongArtistsByArtist } from "@/lib/supabase/song_artists/selectSongArtistsByArtist"; vi.mock("@/lib/supabase/account_artist_ids/deleteAccountArtistId", () => ({ deleteAccountArtistId: vi.fn(), @@ -18,8 +18,8 @@ vi.mock("@/lib/supabase/accounts/deleteAccountById", () => ({ deleteAccountById: vi.fn(), })); -vi.mock("@/lib/supabase/song_artists/getSongArtistExistsByArtist", () => ({ - getSongArtistExistsByArtist: vi.fn(), +vi.mock("@/lib/supabase/song_artists/selectSongArtistsByArtist", () => ({ + selectSongArtistsByArtist: vi.fn(), })); describe("deleteArtist", () => { @@ -51,23 +51,35 @@ describe("deleteArtist", () => { expect(result).toBe(artistId); expect(deleteAccountById).not.toHaveBeenCalled(); - expect(getSongArtistExistsByArtist).not.toHaveBeenCalled(); + expect(selectSongArtistsByArtist).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(getSongArtistExistsByArtist).mockResolvedValue(true); + vi.mocked(selectSongArtistsByArtist).mockResolvedValue([ + { id: "sa-1", song: "ISRC1", artist: artistId }, + ] as never); + + const result = await deleteArtist({ artistId, requesterAccountId }); + + expect(result).toBe(artistId); + expect(selectSongArtistsByArtist).toHaveBeenCalledWith(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(selectSongArtistsByArtist).mockResolvedValue(null); const result = await deleteArtist({ artistId, requesterAccountId }); expect(result).toBe(artistId); - expect(getSongArtistExistsByArtist).toHaveBeenCalledWith(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(getSongArtistExistsByArtist).mockResolvedValue(false); + vi.mocked(selectSongArtistsByArtist).mockResolvedValue([] as never); const result = await deleteArtist({ artistId, requesterAccountId }); diff --git a/lib/artists/deleteArtist.ts b/lib/artists/deleteArtist.ts index 253e441fa..00af24295 100644 --- a/lib/artists/deleteArtist.ts +++ b/lib/artists/deleteArtist.ts @@ -1,7 +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 { getSongArtistExistsByArtist } from "@/lib/supabase/song_artists/getSongArtistExistsByArtist"; +import { selectSongArtistsByArtist } from "@/lib/supabase/song_artists/selectSongArtistsByArtist"; export interface DeleteArtistParams { artistId: string; @@ -38,7 +38,10 @@ export async function deleteArtist({ }); if (remainingLinks.length === 0) { - const hasSongDependencies = await getSongArtistExistsByArtist(artistId); + const songArtists = await selectSongArtistsByArtist(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); } diff --git a/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts b/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts deleted file mode 100644 index 5921a4f2a..000000000 --- a/lib/supabase/song_artists/__tests__/getSongArtistExistsByArtist.test.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; - -import { getSongArtistExistsByArtist } from "../getSongArtistExistsByArtist"; - -const mockFrom = vi.fn(); -const mockSelect = vi.fn(); -const mockEq = vi.fn(); - -vi.mock("@/lib/supabase/serverClient", () => ({ - default: { - from: (...args: unknown[]) => mockFrom(...args), - }, -})); - -describe("getSongArtistExistsByArtist", () => { - beforeEach(() => { - vi.clearAllMocks(); - mockFrom.mockReturnValue({ select: mockSelect }); - mockSelect.mockReturnValue({ eq: mockEq }); - }); - - it("returns true when song_artists rows exist for the artist", async () => { - mockEq.mockResolvedValue({ count: 3, error: null }); - - const result = await getSongArtistExistsByArtist("artist-1"); - - expect(mockFrom).toHaveBeenCalledWith("song_artists"); - expect(mockEq).toHaveBeenCalledWith("artist", "artist-1"); - expect(result).toBe(true); - }); - - it("returns false when no song_artists rows exist", async () => { - mockEq.mockResolvedValue({ count: 0, error: null }); - - const result = await getSongArtistExistsByArtist("artist-1"); - - expect(result).toBe(false); - }); - - it("fails safe by returning true on query error", async () => { - const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - mockEq.mockResolvedValue({ count: null, error: { message: "boom" } }); - - const result = await getSongArtistExistsByArtist("artist-1"); - - expect(result).toBe(true); - expect(consoleSpy).toHaveBeenCalled(); - consoleSpy.mockRestore(); - }); -}); diff --git a/lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts b/lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts new file mode 100644 index 000000000..ba8019eef --- /dev/null +++ b/lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { selectSongArtistsByArtist } from "../selectSongArtistsByArtist"; + +const mockFrom = vi.fn(); +const mockSelect = vi.fn(); +const mockEq = vi.fn(); + +vi.mock("@/lib/supabase/serverClient", () => ({ + default: { + from: (...args: unknown[]) => mockFrom(...args), + }, +})); + +describe("selectSongArtistsByArtist", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockFrom.mockReturnValue({ select: mockSelect }); + mockSelect.mockReturnValue({ eq: mockEq }); + }); + + it("returns the song_artists rows for the artist", async () => { + const rows = [{ id: "sa-1", song: "ISRC1", artist: "artist-1" }]; + mockEq.mockResolvedValue({ data: rows, error: null }); + + const result = await selectSongArtistsByArtist("artist-1"); + + expect(mockFrom).toHaveBeenCalledWith("song_artists"); + expect(mockEq).toHaveBeenCalledWith("artist", "artist-1"); + expect(result).toEqual(rows); + }); + + it("returns an empty array when no rows match", async () => { + mockEq.mockResolvedValue({ data: [], error: null }); + + const result = await selectSongArtistsByArtist("artist-1"); + + expect(result).toEqual([]); + }); + + it("returns null on query error (callers decide how to treat an unknown state)", async () => { + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + mockEq.mockResolvedValue({ data: null, error: { message: "boom" } }); + + const result = await selectSongArtistsByArtist("artist-1"); + + expect(result).toBeNull(); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); +}); diff --git a/lib/supabase/song_artists/getSongArtistExistsByArtist.ts b/lib/supabase/song_artists/getSongArtistExistsByArtist.ts deleted file mode 100644 index de70d9d30..000000000 --- a/lib/supabase/song_artists/getSongArtistExistsByArtist.ts +++ /dev/null @@ -1,24 +0,0 @@ -import supabase from "../serverClient"; - -/** - * Check whether any song_artists rows reference the given artist account. - * - * Used as a hard-delete guard: on query error this fails safe and returns - * true so callers never treat an unknown state as "no dependencies". - * - * @param artistId - Artist account ID to check - * @returns True if at least one song_artists row exists (or the check failed) - */ -export async function getSongArtistExistsByArtist(artistId: string): Promise { - const { count, error } = await supabase - .from("song_artists") - .select("id", { count: "exact", head: true }) - .eq("artist", artistId); - - if (error) { - console.error("Error checking song_artists for artist:", error); - return true; - } - - return (count ?? 0) > 0; -} diff --git a/lib/supabase/song_artists/selectSongArtistsByArtist.ts b/lib/supabase/song_artists/selectSongArtistsByArtist.ts new file mode 100644 index 000000000..183f5a6eb --- /dev/null +++ b/lib/supabase/song_artists/selectSongArtistsByArtist.ts @@ -0,0 +1,21 @@ +import supabase from "../serverClient"; +import type { Tables } from "@/types/database.types"; + +/** + * Select song_artists rows for a given artist account. + * + * @param artist - The artist account ID to fetch song links for + * @returns The matching rows, or null on query error + */ +export async function selectSongArtistsByArtist( + artist: string, +): Promise[] | null> { + const { data, error } = await supabase.from("song_artists").select("*").eq("artist", artist); + + if (error) { + console.error("Error fetching song_artists by artist:", error); + return null; + } + + return data ?? []; +} From 7cf611a26d80b838390a6ecc2891f7e46bfe1771 Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Thu, 9 Jul 2026 12:44:36 -0500 Subject: [PATCH 3/3] refactor(song_artists): consolidate to one param-driven selectSongArtists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review: match the shared param-driven select convention used elsewhere (e.g. getAccountArtistIds({ artistIds?, accountIds? })). Replace the two single-column helpers (selectSongArtistsBySongs / selectSongArtistsByArtist) with one selectSongArtists({ songs?, artists? }) — chunked .in(), null on error. Both callers updated: - attachCanonicalArtistToAccount -> selectSongArtists({ songs }) (coalesces null to [] for its best-effort path, behavior unchanged) - deleteArtist -> selectSongArtists({ artists: [artistId] }) (fail-closed: null or any rows keeps the canonical) 172 tests green across lib/artists, lib/supabase/song_artists, lib/catalog; tsc + lint + prettier clean on touched files. Co-Authored-By: Claude Opus 4.8 --- lib/artists/__tests__/deleteArtist.test.ts | 16 ++-- lib/artists/deleteArtist.ts | 4 +- .../attachCanonicalArtistToAccount.test.ts | 18 ++--- lib/catalog/attachCanonicalArtistToAccount.ts | 4 +- .../__tests__/selectSongArtists.test.ts | 74 +++++++++++++++++++ .../selectSongArtistsByArtist.test.ts | 51 ------------- .../selectSongArtistsBySongs.test.ts | 62 ---------------- .../song_artists/selectSongArtists.ts | 45 +++++++++++ .../song_artists/selectSongArtistsByArtist.ts | 21 ------ .../song_artists/selectSongArtistsBySongs.ts | 31 -------- 10 files changed, 140 insertions(+), 186 deletions(-) create mode 100644 lib/supabase/song_artists/__tests__/selectSongArtists.test.ts delete mode 100644 lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts delete mode 100644 lib/supabase/song_artists/__tests__/selectSongArtistsBySongs.test.ts create mode 100644 lib/supabase/song_artists/selectSongArtists.ts delete mode 100644 lib/supabase/song_artists/selectSongArtistsByArtist.ts delete mode 100644 lib/supabase/song_artists/selectSongArtistsBySongs.ts diff --git a/lib/artists/__tests__/deleteArtist.test.ts b/lib/artists/__tests__/deleteArtist.test.ts index d1858679d..1a892ab89 100644 --- a/lib/artists/__tests__/deleteArtist.test.ts +++ b/lib/artists/__tests__/deleteArtist.test.ts @@ -4,7 +4,7 @@ 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 { selectSongArtistsByArtist } from "@/lib/supabase/song_artists/selectSongArtistsByArtist"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; vi.mock("@/lib/supabase/account_artist_ids/deleteAccountArtistId", () => ({ deleteAccountArtistId: vi.fn(), @@ -18,8 +18,8 @@ vi.mock("@/lib/supabase/accounts/deleteAccountById", () => ({ deleteAccountById: vi.fn(), })); -vi.mock("@/lib/supabase/song_artists/selectSongArtistsByArtist", () => ({ - selectSongArtistsByArtist: vi.fn(), +vi.mock("@/lib/supabase/song_artists/selectSongArtists", () => ({ + selectSongArtists: vi.fn(), })); describe("deleteArtist", () => { @@ -51,25 +51,25 @@ describe("deleteArtist", () => { expect(result).toBe(artistId); expect(deleteAccountById).not.toHaveBeenCalled(); - expect(selectSongArtistsByArtist).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(selectSongArtistsByArtist).mockResolvedValue([ + vi.mocked(selectSongArtists).mockResolvedValue([ { id: "sa-1", song: "ISRC1", artist: artistId }, ] as never); const result = await deleteArtist({ artistId, requesterAccountId }); expect(result).toBe(artistId); - expect(selectSongArtistsByArtist).toHaveBeenCalledWith(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(selectSongArtistsByArtist).mockResolvedValue(null); + vi.mocked(selectSongArtists).mockResolvedValue(null); const result = await deleteArtist({ artistId, requesterAccountId }); @@ -79,7 +79,7 @@ describe("deleteArtist", () => { it("hard-deletes the account when the last link is removed and no dependencies exist", async () => { vi.mocked(getAccountArtistIds).mockResolvedValue([] as never); - vi.mocked(selectSongArtistsByArtist).mockResolvedValue([] as never); + vi.mocked(selectSongArtists).mockResolvedValue([] as never); const result = await deleteArtist({ artistId, requesterAccountId }); diff --git a/lib/artists/deleteArtist.ts b/lib/artists/deleteArtist.ts index 00af24295..8c3dff10f 100644 --- a/lib/artists/deleteArtist.ts +++ b/lib/artists/deleteArtist.ts @@ -1,7 +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 { selectSongArtistsByArtist } from "@/lib/supabase/song_artists/selectSongArtistsByArtist"; +import { selectSongArtists } from "@/lib/supabase/song_artists/selectSongArtists"; export interface DeleteArtistParams { artistId: string; @@ -38,7 +38,7 @@ export async function deleteArtist({ }); if (remainingLinks.length === 0) { - const songArtists = await selectSongArtistsByArtist(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; diff --git a/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts b/lib/catalog/__tests__/attachCanonicalArtistToAccount.test.ts index 51642bb8e..d57816666 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 e81057bd3..9e120ef28 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 000000000..d77ae7bf0 --- /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__/selectSongArtistsByArtist.test.ts b/lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts deleted file mode 100644 index ba8019eef..000000000 --- a/lib/supabase/song_artists/__tests__/selectSongArtistsByArtist.test.ts +++ /dev/null @@ -1,51 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; - -import { selectSongArtistsByArtist } from "../selectSongArtistsByArtist"; - -const mockFrom = vi.fn(); -const mockSelect = vi.fn(); -const mockEq = vi.fn(); - -vi.mock("@/lib/supabase/serverClient", () => ({ - default: { - from: (...args: unknown[]) => mockFrom(...args), - }, -})); - -describe("selectSongArtistsByArtist", () => { - beforeEach(() => { - vi.clearAllMocks(); - mockFrom.mockReturnValue({ select: mockSelect }); - mockSelect.mockReturnValue({ eq: mockEq }); - }); - - it("returns the song_artists rows for the artist", async () => { - const rows = [{ id: "sa-1", song: "ISRC1", artist: "artist-1" }]; - mockEq.mockResolvedValue({ data: rows, error: null }); - - const result = await selectSongArtistsByArtist("artist-1"); - - expect(mockFrom).toHaveBeenCalledWith("song_artists"); - expect(mockEq).toHaveBeenCalledWith("artist", "artist-1"); - expect(result).toEqual(rows); - }); - - it("returns an empty array when no rows match", async () => { - mockEq.mockResolvedValue({ data: [], error: null }); - - const result = await selectSongArtistsByArtist("artist-1"); - - expect(result).toEqual([]); - }); - - it("returns null on query error (callers decide how to treat an unknown state)", async () => { - const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - mockEq.mockResolvedValue({ data: null, error: { message: "boom" } }); - - const result = await selectSongArtistsByArtist("artist-1"); - - expect(result).toBeNull(); - expect(consoleSpy).toHaveBeenCalled(); - consoleSpy.mockRestore(); - }); -}); 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 9cf392200..000000000 --- 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 000000000..327a36b36 --- /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/selectSongArtistsByArtist.ts b/lib/supabase/song_artists/selectSongArtistsByArtist.ts deleted file mode 100644 index 183f5a6eb..000000000 --- a/lib/supabase/song_artists/selectSongArtistsByArtist.ts +++ /dev/null @@ -1,21 +0,0 @@ -import supabase from "../serverClient"; -import type { Tables } from "@/types/database.types"; - -/** - * Select song_artists rows for a given artist account. - * - * @param artist - The artist account ID to fetch song links for - * @returns The matching rows, or null on query error - */ -export async function selectSongArtistsByArtist( - artist: string, -): Promise[] | null> { - const { data, error } = await supabase.from("song_artists").select("*").eq("artist", artist); - - if (error) { - console.error("Error fetching song_artists by artist:", error); - return null; - } - - return data ?? []; -} diff --git a/lib/supabase/song_artists/selectSongArtistsBySongs.ts b/lib/supabase/song_artists/selectSongArtistsBySongs.ts deleted file mode 100644 index cd5a28833..000000000 --- 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; -}