diff --git a/locale/en.ftl b/locale/en.ftl index e2821ddc..283c77f1 100644 --- a/locale/en.ftl +++ b/locale/en.ftl @@ -20,6 +20,7 @@ error-playlist-not-found = Playlist not found. error-playlist-item-not-found = Playlist item not found. error-item-not-in-playlist = Item not in playlist. error-empty-playlist = You don't have anything to play. Please add some songs to your playlist and try again. +error-active-playlist = Cannot delete the active playlist. error-history-entry-not-found = History entry not found. error-media-not-found = Media object not found. error-no-self-favorite = You can't favorite your own plays. diff --git a/src/controllers/playlists.js b/src/controllers/playlists.js index 98a1a735..d3ea7687 100644 --- a/src/controllers/playlists.js +++ b/src/controllers/playlists.js @@ -1,4 +1,4 @@ -import { HTTPError, PlaylistNotFoundError } from '../errors/index.js'; +import { HTTPError } from '../errors/index.js'; import { serializePlaylist, serializePlaylistItem } from '../utils/serialize.js'; import getOffsetPagination from '../utils/getOffsetPagination.js'; import toItemResponse from '../utils/toItemResponse.js'; @@ -89,10 +89,6 @@ async function getPlaylist(req) { const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } - return toItemResponse( serializePlaylist(playlist), { url: req.fullUrl }, @@ -136,16 +132,15 @@ async function createPlaylist(req) { async function deletePlaylist(req) { const { user } = req; const { id } = req.params; - const { playlists } = req.uwave; + const { db, playlists } = req.uwave; - const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } + await db.transaction().execute(async (tx) => { + const playlist = await playlists.getUserPlaylist(user, id, tx); - await playlists.deletePlaylist(playlist); + await playlists.deletePlaylist(playlist, tx); + }); - return toItemResponse({}, { url: req.fullUrl }); + return toItemResponse({}); } const patchableKeys = ['name', 'description']; @@ -174,9 +169,6 @@ async function updatePlaylist(req) { }); const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } const updatedPlaylist = await playlists.updatePlaylist(playlist, patch); @@ -204,9 +196,6 @@ async function renamePlaylist(req) { const { playlists } = req.uwave; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } const updatedPlaylist = await playlists.updatePlaylist(playlist, { name }); @@ -230,9 +219,6 @@ async function activatePlaylist(req) { const { id } = req.params; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } await db.updateTable('users') .where('id', '=', user.id) @@ -260,9 +246,6 @@ async function getPlaylistItems(req) { const pagination = getOffsetPagination(req.query); const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } const items = await playlists.getPlaylistItems(playlist, filter, pagination); @@ -298,9 +281,6 @@ async function addPlaylistItems(req) { const { at, after, items } = req.body; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } let options; if (at === 'start' || at === 'end') { @@ -348,9 +328,6 @@ async function removePlaylistItems(req) { const { items } = req.body; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } await playlists.removePlaylistItems(playlist, items); @@ -378,9 +355,6 @@ async function movePlaylistItems(req) { const { at, after, items } = req.body; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } let options; if (at === 'start' || at === 'end') { @@ -412,9 +386,6 @@ async function shufflePlaylistItems(req) { const { id } = req.params; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } await playlists.shufflePlaylist(playlist); @@ -436,9 +407,6 @@ async function getPlaylistItem(req) { const { id, itemID } = req.params; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } const { playlistItem, media } = await playlists.getPlaylistItem(playlist, itemID); @@ -476,9 +444,6 @@ async function updatePlaylistItem(req) { }; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } const { playlistItem, media } = await playlists.getPlaylistItem(playlist, itemID); const updatedItem = await playlists.updatePlaylistItem(playlistItem, patch); @@ -501,9 +466,6 @@ async function removePlaylistItem(req) { const { id, itemID } = req.params; const playlist = await playlists.getUserPlaylist(user, id); - if (!playlist) { - throw new PlaylistNotFoundError({ id }); - } await playlists.removePlaylistItems(playlist, [itemID]); diff --git a/src/errors/index.js b/src/errors/index.js index 8aa06c85..f864a312 100644 --- a/src/errors/index.js +++ b/src/errors/index.js @@ -252,6 +252,12 @@ const EmptyPlaylistError = createErrorClass('EmptyPlaylistError', { base: Forbidden, }); +const PlaylistActiveError = createErrorClass('PlaylistActiveError', { + code: 'active-playlist', + string: 'error-active-playlist', + base: BadRequest, +}); + const WaitlistLockedError = createErrorClass('WaitlistLockedError', { code: 'waitlist-locked', string: 'error-waitlist-locked', @@ -311,6 +317,7 @@ export { SourceNotFoundError, SourceNoImportError, EmptyPlaylistError, + PlaylistActiveError, WaitlistLockedError, AlreadyInWaitlistError, UserNotInWaitlistError, diff --git a/src/plugins/playlists.js b/src/plugins/playlists.js index 5e738863..00398a2f 100644 --- a/src/plugins/playlists.js +++ b/src/plugins/playlists.js @@ -4,6 +4,7 @@ import { ItemNotInPlaylistError, MediaNotFoundError, UserNotFoundError, + PlaylistActiveError, } from '../errors/index.js'; import Page from '../Page.js'; import routes from '../routes/playlists.js'; @@ -13,6 +14,7 @@ import { arrayCycle, arrayShuffle as arrayShuffle, fromJson, + isForeignKeyError, json, jsonb, jsonEach, @@ -249,35 +251,42 @@ class PlaylistsRepository { async createPlaylist(user, { name }, tx = this.#uw.db) { const id = /** @type {PlaylistID} */ (randomUUID()); - const playlist = await tx.insertInto('playlists') - .values({ - id, - name, - userID: user.id, - items: jsonb([]), - }) - .returning([ - 'id', - 'userID', - 'name', - (eb) => jsonLength(eb.ref('items')).as('size'), - 'createdAt', - 'updatedAt', - ]) - .executeTakeFirstOrThrow(); + const result = await tx.transaction().execute(async (tx) => { + const playlist = await tx.insertInto('playlists') + .values({ + id, + name, + userID: user.id, + items: jsonb([]), + }) + .returning([ + 'id', + 'userID', + 'name', + (eb) => jsonLength(eb.ref('items')).as('size'), + 'createdAt', + 'updatedAt', + ]) + .executeTakeFirstOrThrow(); - let active = false; - // If this is the user's first playlist, immediately activate it. - if (user.activePlaylistID == null) { - this.#logger.info({ userId: user.id, playlistId: playlist.id }, 'activating first playlist'); - await tx.updateTable('users') - .where('users.id', '=', user.id) + const updated = await tx.updateTable('users') + .where('id', '=', user.id) + .where('activePlaylistID', 'is', null) .set({ activePlaylistID: playlist.id }) - .execute(); - active = true; + .returning(['activePlaylistID']) + .executeTakeFirst(); + + return { + playlist, + active: updated != null && updated.activePlaylistID === playlist.id, + }; + }); + + if (result.active) { + this.#logger.info({ userId: user.id, playlistId: result.playlist.id }, 'activated first playlist'); } - return { playlist, active }; + return result; } /** @@ -345,12 +354,34 @@ class PlaylistsRepository { } /** + * Delete a playlist. An active playlist cannot be deleted. + * * @param {Playlist} playlist + * @returns {Promise} */ async deletePlaylist(playlist, tx = this.#uw.db) { - await tx.deleteFrom('playlists') - .where('id', '=', playlist.id) - .execute(); + // This *must* be executed in a transaction, else it would be possible for + // the items to be deleted but not the playlist metadata. + // Maybe it'd be better to just require the `tx` parameter, or not support + // passing one in? + if (!tx.isTransaction) { + return tx.transaction().execute((tx) => this.deletePlaylist(playlist, tx)); + } + + try { + // Missing `ON DELETE CASCADE`, so we have to do it manually, unfortunately... + await tx.deleteFrom('playlistItems') + .where('playlistID', '=', playlist.id) + .execute(); + await tx.deleteFrom('playlists') + .where('id', '=', playlist.id) + .execute(); + } catch (err) { + if (isForeignKeyError(err)) { + throw new PlaylistActiveError(); + } + throw err; + } } /** diff --git a/test/playlists.mjs b/test/playlists.mjs index 648f366e..43b42af5 100644 --- a/test/playlists.mjs +++ b/test/playlists.mjs @@ -287,6 +287,121 @@ describe('Playlists', () => { }); }); + describe('DELETE /playlists/:id', () => { + it('requires authentication', async () => { + const fakeID = 'e2c85d94-344b-4c2a-86bd-95edb939f3e6'; + + await supertest(uw.server) + .delete(`/api/playlists/${fakeID}`) + .expect(401); + }); + + it('validates input', async () => { + const notAnID = 'not-a-uuid'; + const fakeID = 'e2c85d94-344b-4c2a-86bd-95edb939f3e6'; + const token = await uw.test.createTestSessionToken(user); + + await supertest(uw.server) + .delete(`/api/playlists/${notAnID}`) + .set('Cookie', `uwsession=${token}`) + .expect(400); + + await supertest(uw.server) + .delete(`/api/playlists/${fakeID}`) + .set('Cookie', `uwsession=${token}`) + .expect(404); + }); + + it('deletes the playlist', async () => { + const token = await uw.test.createTestSessionToken(user); + + const first = await uw.playlists.createPlaylist(user, { name: 'Active Playlist' }); + assert(first.active, 'first playlist is active and will not be deleted'); + + const { active, playlist } = await uw.playlists.createPlaylist(user, { name: 'Test Playlist' }); + assert(!active, 'second playlist is active and will be deleted'); + + // Add some items that should be deleted by cascade + const items = await generateItems(20); + await uw.playlists.addPlaylistItems(playlist, items); + + await supertest(uw.server) + .get('/api/playlists') + .set('Cookie', `uwsession=${token}`) + .expect(200) + .expect((res) => { + sinon.assert.match(res.body.data, sinon.match.some( + sinon.match({ _id: playlist.id }), + )); + }); + + await supertest(uw.server) + .delete(`/api/playlists/${playlist.id}`) + .set('Cookie', `uwsession=${token}`) + .expect(200); + + await supertest(uw.server) + .get('/api/playlists') + .set('Cookie', `uwsession=${token}`) + .expect(200) + .expect((res) => { + for (const p of res.body.data) { + assert.strictEqual(typeof p._id, 'string'); + assert.notStrictEqual(p._id, playlist.id, 'playlist should not be in the list anymore'); + } + }); + }); + + it('cannot delete active playlist', async () => { + const token = await uw.test.createTestSessionToken(user); + + const { playlist, active } = await uw.playlists.createPlaylist(user, { name: 'Active Playlist' }); + assert(active, 'first playlist is active'); + + await supertest(uw.server) + .get('/api/now') + .set('Cookie', `uwsession=${token}`) + .expect(200) + .expect((res) => { + assert.strictEqual(res.body.activePlaylist, playlist.id); + }); + + await supertest(uw.server) + .delete(`/api/playlists/${playlist.id}`) + .set('Cookie', `uwsession=${token}`) + .expect(400) + .expect((res) => { + sinon.assert.match(res.body.errors[0], { + code: 'active-playlist', + }); + }); + }); + + it("does not delete someone else's playlist", async () => { + const token = await uw.test.createTestSessionToken(user); + const { playlist } = await uw.playlists.createPlaylist(user, { name: 'Test Playlist' }); + const otherUser = await uw.test.createUser(); + const otherToken = await uw.test.createTestSessionToken(otherUser); + + // DELETE on someone else's playlist should be a not found error + await supertest(uw.server) + .delete(`/api/playlists/${playlist.id}`) + .set('Cookie', `uwsession=${otherToken}`) + .expect(404); + + // Playlist should still exist + await supertest(uw.server) + .get('/api/playlists') + .set('Cookie', `uwsession=${token}`) + .expect(200) + .expect((res) => { + sinon.assert.match(res.body.data, sinon.match.some( + sinon.match({ _id: playlist.id }), + )); + }); + }); + }); + describe('PUT /playlists/:id/rename', () => { it('requires authentication', async () => { const fakeID = 'e2c85d94-344b-4c2a-86bd-95edb939f3e6';