From 7e3b593c687e0da6ecf83e8d58eebf733bece71f Mon Sep 17 00:00:00 2001 From: Sander Vocke Date: Thu, 17 Dec 2020 17:29:43 +0100 Subject: [PATCH] Patch and put requests' tests are working, some bugs were fixed. --- .vscode/launch.json | 2 +- server/db/Tag.ts | 35 ++++++-- server/db/Track.ts | 29 +++++-- server/endpoints/Album.ts | 14 +-- server/endpoints/Artist.ts | 14 +-- server/endpoints/Integration.ts | 14 +-- server/endpoints/Tag.ts | 21 +++-- server/endpoints/Track.ts | 14 +-- server/test/integration/flows/ResourceFlow.ts | 86 +++++++++++-------- .../test/reference_model/DBReferenceModel.ts | 38 ++++++-- server/test/reference_model/randomGen.ts | 7 +- 11 files changed, 187 insertions(+), 87 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 05c4452..eb75935 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -10,7 +10,7 @@ "name": "Jasmine Tests with SQLite", "env": { "MUDBASE_DB_CONFIG": "{\"client\": \"sqlite3\", \"connection\": \":memory:\"}", - "TEST_RANDOM_SEED": "0.77715" + "TEST_RANDOM_SEED": "0.04801" }, "program": "${workspaceFolder}/server/node_modules/jasmine-ts/lib/index", "args": [ diff --git a/server/db/Tag.ts b/server/db/Tag.ts index 0a80299..fa6412a 100644 --- a/server/db/Tag.ts +++ b/server/db/Tag.ts @@ -5,14 +5,25 @@ import { TagBaseWithRefs, TagWithDetails, TagWithId, TagWithRefs, TagWithRefsWit import { DBError, DBErrorKind } from "../endpoints/types"; import { makeNotFoundError } from "./common"; -export async function getTagChildrenRecursive(id: number, userId: number, trx: any): Promise { +export async function getTagChildrenRecursive(id: number, + userId: number, + trx: any, + visited: number[] = [], // internal, for cycle detection +): Promise { + + // check for cycles, these are not allowed. + // a cycle would be if the same ID occurs more than once in the visited set. + if ((new Set(visited)).size < visited.length) { + throw new Error('cyclic tag dependency') + } + const directChildren = (await trx.select('id') .from('tags') .where({ 'user': userId }) .where({ 'parentId': id })).map((r: any) => r.id); const indirectChildrenPromises = directChildren.map( - (child: number) => getTagChildrenRecursive(child, userId, trx) + (child: number) => getTagChildrenRecursive(child, userId, trx, [...visited, id]) ); const indirectChildrenNested = await Promise.all(indirectChildrenPromises); const indirectChildren = indirectChildrenNested.flat(); @@ -151,7 +162,7 @@ export async function modifyTag(userId: number, tagId: number, tag: TagBaseWithR .from('tags') .where({ 'user': userId }) .where({ 'id': tag.parentId }) - .then((ts: any) => ts.map((t: any) => t['tagId'])) : + .then((ts: any) => ts.length ? ts.map((t: any) => t['tagId']) : null) : (async () => { return null })(); // Start retrieving the tag itself. @@ -161,13 +172,27 @@ export async function modifyTag(userId: number, tagId: number, tag: TagBaseWithR .where({ id: tagId }) .then((r: any) => (r && r[0]) ? r[0]['id'] : undefined) + // Start retrieving all current children. This is to prevent + // cycles. + const childrenPromise = getTagChildrenRecursive(tagId, userId, trx); + // Wait for the requests to finish. - var [dbTag, parent] = await Promise.all([tagPromise, parentTagIdPromise]); + var [dbTag, parent, children] = await Promise.all([tagPromise, parentTagIdPromise, childrenPromise]); + + // Check that modifying this will not cause a dependency cycle. + if (tag.parentId && [...children, tagId].includes(tag.parentId)) { + const e: DBError = { + name: "DBError", + kind: DBErrorKind.ResourceConflict, + message: 'Modifying this tag would cause a tag parent cycle.', + }; + throw e; + } // Check that we found all objects we need. if ((tag.parentId && !parent) || !dbTag) { - throw makeNotFoundError(); + throw makeNotFoundError(); } // Modify the tag. diff --git a/server/db/Track.ts b/server/db/Track.ts index 5c1a146..3da46ae 100644 --- a/server/db/Track.ts +++ b/server/db/Track.ts @@ -24,7 +24,7 @@ export async function getTrack(id: number, userId: number, knex: Knex): const artistsPromise: Promise = knex.select('artistId') - .from('artists_tracks') + .from('tracks_artists') .where({ 'trackId': id }) .then((artists: any) => artists.map((artist: any) => artist['artistId'])) .then((ids: number[]) => @@ -166,7 +166,7 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB const artistIdsPromise: Promise = track.artistIds ? trx.select('artistId') - .from('artists_tracks') + .from('tracks_artists') .whereIn('artistId', track.artistIds) .then((as: any) => as.map((a: any) => a['artistId'])) : (async () => undefined)(); @@ -180,12 +180,25 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB .then((ts: any) => ts.map((t: any) => t['tagId'])) : (async () => undefined)(); + // Start retrieving album if we are modifying that. + const albumIdPromise = + track.albumId ? + trx.select('id') + .from('albums') + .where({ 'user': userId }) + .where({ id: track.albumId }) + .then((r: any) => (r && r[0]) ? r[0]['id'] : undefined) : + (async () => undefined)(); + + let blablums = await trx.select('id').from('albums'); + // Wait for the requests to finish. - var [oldTrack, artists, tags] = await Promise.all([trackIdPromise, artistIdsPromise, tagIdsPromise]);; + var [oldTrack, artists, tags, album] = await Promise.all([trackIdPromise, artistIdsPromise, tagIdsPromise, albumIdPromise]);; // Check that we found all objects we need. if ((!artists || !_.isEqual(artists.sort(), (track.artistIds || []).sort())) || (!tags || !_.isEqual(tags.sort(), (track.tagIds || []).sort())) || + (!album && track.albumId) || !oldTrack) { throw makeNotFoundError(); } @@ -194,7 +207,7 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB var update: any = {}; if ("name" in track) { update["name"] = track.name; } if ("storeLinks" in track) { update["storeLinks"] = JSON.stringify(track.storeLinks || []); } - if ("albumId" in track) { update["albumId"] = track.albumId; } + if ("albumId" in track) { update["album"] = track.albumId; } const modifyTrackPromise = trx('tracks') .where({ 'user': userId }) @@ -202,7 +215,7 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB .update(update) // Remove unlinked artists. - const removeUnlinkedArtists = artists ? trx('artists_tracks') + const removeUnlinkedArtists = artists ? trx('tracks_artists') .where({ 'trackId': trackId }) .whereNotIn('artistId', track.artistIds || []) .delete() : undefined; @@ -214,7 +227,7 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB .delete() : undefined; // Link new artists. - const addArtists = artists ? trx('artists_tracks') + const addArtists = artists ? trx('tracks_artists') .where({ 'trackId': trackId }) .then((as: any) => as.map((a: any) => a['artistId'])) .then((doneArtistIds: number[]) => { @@ -232,7 +245,7 @@ export async function modifyTrack(userId: number, trackId: number, track: TrackB // Link them return Promise.all( insertObjects.map((obj: any) => - trx('artists_tracks').insert(obj) + trx('tracks_artists').insert(obj) ) ); }) : undefined; @@ -279,7 +292,7 @@ export async function deleteTrack(userId: number, trackId: number, knex: Knex): // FIXME remove let tracks = await trx.select('id', 'name') - .from('tracks'); + .from('tracks'); console.log("All tracks:", tracks); // Start by retrieving the track itself for sanity. diff --git a/server/endpoints/Album.ts b/server/endpoints/Album.ts index 8113390..3d23539 100644 --- a/server/endpoints/Album.ts +++ b/server/endpoints/Album.ts @@ -8,10 +8,11 @@ import { GetArtist } from './Artist'; export const GetAlbum: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); try { const maybeAlbum: api.GetAlbumResponse | null = - await getAlbum(req.params.id, userId, knex); + await getAlbum(id, userId, knex); if (maybeAlbum) { await res.send(maybeAlbum); @@ -57,11 +58,12 @@ export const PutAlbum: EndpointHandler = async (req: any, res: any, knex: Knex) } const reqObject: api.PutAlbumRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Put Album ", reqObject); try { - await modifyAlbum(userId, req.params.id, reqObject, knex); + await modifyAlbum(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -79,11 +81,12 @@ export const PatchAlbum: EndpointHandler = async (req: any, res: any, knex: Knex } const reqObject: api.PatchAlbumRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Patch Album ", reqObject); try { - await modifyAlbum(userId, req.params.id, reqObject, knex); + await modifyAlbum(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -92,11 +95,12 @@ export const PatchAlbum: EndpointHandler = async (req: any, res: any, knex: Knex export const DeleteAlbum: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); - console.log("User ", userId, ": Delete Album ", req.params.id); + console.log("User ", userId, ": Delete Album ", id); try { - await deleteAlbum(userId, req.params.id, knex); + await deleteAlbum(userId, id, knex); res.status(200).send(); } catch (e) { diff --git a/server/endpoints/Artist.ts b/server/endpoints/Artist.ts index 1663ee2..e7bc368 100644 --- a/server/endpoints/Artist.ts +++ b/server/endpoints/Artist.ts @@ -6,9 +6,10 @@ import { createArtist, deleteArtist, getArtist, modifyArtist } from '../db/Artis export const GetArtist: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); try { - let artist = await getArtist(req.params.id, userId, knex); + let artist = await getArtist(id, userId, knex); await res.status(200).send(artist); } catch (e) { handleErrorsInEndpoint(e) @@ -50,11 +51,12 @@ export const PutArtist: EndpointHandler = async (req: any, res: any, knex: Knex) } const reqObject: api.PutArtistRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Put Artist ", reqObject); try { - await modifyArtist(userId, req.params.id, reqObject, knex); + await modifyArtist(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { @@ -73,11 +75,12 @@ export const PatchArtist: EndpointHandler = async (req: any, res: any, knex: Kne } const reqObject: api.PatchArtistRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Patch Artist ", reqObject); try { - await modifyArtist(userId, req.params.id, reqObject, knex); + await modifyArtist(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { @@ -87,11 +90,12 @@ export const PatchArtist: EndpointHandler = async (req: any, res: any, knex: Kne export const DeleteArtist: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); - console.log("User ", userId, ": Delete Artist ", req.params.id); + console.log("User ", userId, ": Delete Artist ", id); try { - await deleteArtist(userId, req.params.id, knex); + await deleteArtist(userId, id, knex); res.status(200).send(); } catch (e) { diff --git a/server/endpoints/Integration.ts b/server/endpoints/Integration.ts index 9be5be5..8dc1caa 100644 --- a/server/endpoints/Integration.ts +++ b/server/endpoints/Integration.ts @@ -33,7 +33,8 @@ export const PostIntegration: EndpointHandler = async (req: any, res: any, knex: export const GetIntegration: EndpointHandler = async (req: any, res: any, knex: Knex) => { try { - let integration = await getIntegration(req.user.id, req.params.id, knex); + let id = parseInt(req.params.id); + let integration = await getIntegration(req.user.id, id, knex); res.status(200).send(integration); } catch (e) { handleErrorsInEndpoint(e) @@ -54,11 +55,12 @@ export const ListIntegrations: EndpointHandler = async (req: any, res: any, knex export const DeleteIntegration: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); - console.log("User ", userId, ": Delete Integration ", req.params.id); + console.log("User ", userId, ": Delete Integration ", id); try { - await deleteIntegration(userId, req.params.id, knex); + await deleteIntegration(userId, id, knex); res.status(200).send(); } catch (e) { @@ -77,11 +79,12 @@ export const PutIntegration: EndpointHandler = async (req: any, res: any, knex: } const reqObject: api.PutIntegrationRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Put Integration ", reqObject); try { - await modifyIntegration(userId, req.params.id, reqObject, knex); + await modifyIntegration(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { @@ -102,9 +105,10 @@ export const PatchIntegration: EndpointHandler = async (req: any, res: any, knex const { id: userId } = req.user; console.log("User ", userId, ": Patch Integration ", reqObject); + let id = parseInt(req.params.id); try { - await modifyIntegration(userId, req.params.id, reqObject, knex); + await modifyIntegration(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { diff --git a/server/endpoints/Tag.ts b/server/endpoints/Tag.ts index 2201c6a..3075fc1 100644 --- a/server/endpoints/Tag.ts +++ b/server/endpoints/Tag.ts @@ -33,11 +33,12 @@ export const PostTag: EndpointHandler = async (req: any, res: any, knex: Knex) = export const DeleteTag: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; - console.log("User ", userId, ": Delete Tag ", req.params.id); + let id = parseInt(req.params.id); + console.log("User ", userId, ": Delete Tag ", id); try { - await deleteTag(userId, req.params.id, knex); + await deleteTag(userId, id, knex); res.status(200).send(); } catch (e) { @@ -47,8 +48,9 @@ export const DeleteTag: EndpointHandler = async (req: any, res: any, knex: Knex) export const GetTag: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); try { - let tag = await getTag(req.params.id, userId, knex); + let tag = await getTag(id, userId, knex); await res.status(200).send(tag); } catch (e) { handleErrorsInEndpoint(e) @@ -66,11 +68,12 @@ export const PutTag: EndpointHandler = async (req: any, res: any, knex: Knex) => } const reqObject: api.PutTagRequest = req.body; const { id: userId } = req.user; + let id: number = parseInt(req.params.id); console.log("User ", userId, ": Put Tag ", reqObject); try { - await modifyTag(userId, req.params.id, reqObject, knex); + await modifyTag(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -88,11 +91,12 @@ export const PatchTag: EndpointHandler = async (req: any, res: any, knex: Knex) } const reqObject: api.PatchTagRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Patch Tag ", reqObject); try { - await modifyTag(userId, req.params.id, reqObject, knex); + await modifyTag(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -102,9 +106,10 @@ export const PatchTag: EndpointHandler = async (req: any, res: any, knex: Knex) export const MergeTag: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; - console.log("User ", userId, ": Merge Tag ", req.params.id, req.params.toId); - const fromId = req.params.id; - const toId = req.params.toId; + const fromId = parseInt(req.params.id); + const toId = parseInt(req.params.toId); + + console.log("User ", userId, ": Merge Tag ", fromId, req.params.toId); try { await mergeTag(userId, fromId, toId, knex); diff --git a/server/endpoints/Track.ts b/server/endpoints/Track.ts index 2246621..a2aa7bb 100644 --- a/server/endpoints/Track.ts +++ b/server/endpoints/Track.ts @@ -31,9 +31,10 @@ export const PostTrack: EndpointHandler = async (req: any, res: any, knex: Knex) export const GetTrack: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); try { - let track = await getTrack(req.params.id, userId, knex); + let track = await getTrack(id, userId, knex); await res.status(200).send(track); } catch (e) { handleErrorsInEndpoint(e) @@ -51,11 +52,12 @@ export const PutTrack: EndpointHandler = async (req: any, res: any, knex: Knex) } const reqObject: api.PutTrackRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Put Track ", reqObject); try { - await modifyTrack(userId, req.params.id, reqObject, knex); + await modifyTrack(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -73,11 +75,12 @@ export const PatchTrack: EndpointHandler = async (req: any, res: any, knex: Knex } const reqObject: api.PatchTrackRequest = req.body; const { id: userId } = req.user; + let id = parseInt(req.params.id); console.log("User ", userId, ": Patch Track ", reqObject); try { - await modifyTrack(userId, req.params.id, reqObject, knex); + await modifyTrack(userId, id, reqObject, knex); res.status(200).send(); } catch (e) { handleErrorsInEndpoint(e); @@ -86,11 +89,12 @@ export const PatchTrack: EndpointHandler = async (req: any, res: any, knex: Knex export const DeleteTrack: EndpointHandler = async (req: any, res: any, knex: Knex) => { const { id: userId } = req.user; + let id = parseInt(req.params.id); - console.log("User ", userId, ": Delete Track ", req.params.id); + console.log("User ", userId, ": Delete Track ", id); try { - await deleteTrack(userId, req.params.id, knex); + await deleteTrack(userId, id, knex); res.status(200).send(); } catch (e) { diff --git a/server/test/integration/flows/ResourceFlow.ts b/server/test/integration/flows/ResourceFlow.ts index 069653d..cb770ab 100644 --- a/server/test/integration/flows/ResourceFlow.ts +++ b/server/test/integration/flows/ResourceFlow.ts @@ -75,21 +75,21 @@ function normalizeDB(oldDb: ReferenceDatabase) { db[userId].tags.forEach((x: TagWithRefsWithId) => { x.id = remapId(x.id, tagMap); }) } for (const userId in db) { - // Now remap the references. + // Now remap and sort the references. db[userId].tracks.forEach((x: TrackWithRefsWithId) => { - x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)); - x.artistIds = x.artistIds.map((id: number) => remapId(id, artistMap)); + x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)).sort(); + x.artistIds = x.artistIds.map((id: number) => remapId(id, artistMap)).sort(); x.albumId = x.albumId ? remapId(x.albumId, albumMap) : null; }); db[userId].albums.forEach((x: AlbumWithRefsWithId) => { - x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)); - x.artistIds = x.artistIds.map((id: number) => remapId(id, artistMap)); - x.trackIds = x.trackIds.map((id: number) => remapId(id, trackMap)); + x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)).sort(); + x.artistIds = x.artistIds.map((id: number) => remapId(id, artistMap)).sort(); + x.trackIds = x.trackIds.map((id: number) => remapId(id, trackMap)).sort(); }); db[userId].artists.forEach((x: ArtistWithRefsWithId) => { - x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)); - x.albumIds = x.albumIds.map((id: number) => remapId(id, albumMap)); - x.trackIds = x.trackIds.map((id: number) => remapId(id, trackMap)); + x.tagIds = x.tagIds.map((id: number) => remapId(id, tagMap)).sort(); + x.albumIds = x.albumIds.map((id: number) => remapId(id, albumMap)).sort(); + x.trackIds = x.trackIds.map((id: number) => remapId(id, trackMap)).sort(); }); db[userId].tags.forEach((x: TagWithRefsWithId) => { x.parentId = x.parentId ? remapId(x.parentId, tagMap) : null; @@ -118,32 +118,48 @@ function transformActionIDs(action: DBAction, mappings: IDMappings, rng: any) { } switch (r.type) { - case DBActionType.CreateTrack: { - let track = r.payload as TrackWithRefsWithId; - track.tagIds.forEach((id: number) => doMap(id, mappings.tags)); - track.artistIds.forEach((id: number) => doMap(id, mappings.artists)); - track.albumId = track.albumId ? doMap(track.albumId, mappings.albums) : null; - break; - } - case DBActionType.CreateArtist: { - let artist = r.payload as ArtistWithRefsWithId; - artist.tagIds.forEach((id: number) => doMap(id, mappings.tags)); - artist.albumIds.forEach((id: number) => doMap(id, mappings.albums)); - artist.trackIds.forEach((id: number) => doMap(id, mappings.tracks)); - break; - } - case DBActionType.CreateAlbum: { - let album = r.payload as AlbumWithRefsWithId; - album.tagIds.forEach((id: number) => doMap(id, mappings.tags)); - album.artistIds.forEach((id: number) => doMap(id, mappings.artists)); - album.trackIds.forEach((id: number) => doMap(id, mappings.tracks)); - break; - } - case DBActionType.CreateTag: { - let tag = r.payload as TagWithRefsWithId; - tag.parentId = tag.parentId ? doMap(tag.parentId, mappings.tags) : null; - break; - } + case DBActionType.CreateTrack: + case DBActionType.PutTrack: + case DBActionType.PatchTrack: + { + let track = r.payload as TrackWithRefsWithId; + track.tagIds && track.tagIds.forEach((id: number) => doMap(id, mappings.tags)); + track.artistIds && track.artistIds.forEach((id: number) => doMap(id, mappings.artists)); + if (track.albumId) { track.albumId = doMap(track.albumId, mappings.albums); } + if (track.id) { track.id = doMap(track.id, mappings.tracks); } + break; + } + case DBActionType.CreateArtist: + case DBActionType.PutArtist: + case DBActionType.PatchArtist: + { + let artist = r.payload as ArtistWithRefsWithId; + artist.tagIds && artist.tagIds.forEach((id: number) => doMap(id, mappings.tags)); + artist.albumIds && artist.albumIds.forEach((id: number) => doMap(id, mappings.albums)); + artist.trackIds && artist.trackIds.forEach((id: number) => doMap(id, mappings.tracks)); + if (artist.id) { artist.id = doMap(artist.id, mappings.artists); } + break; + } + case DBActionType.CreateAlbum: + case DBActionType.PutAlbum: + case DBActionType.PatchAlbum: + { + let album = r.payload as AlbumWithRefsWithId; + album.tagIds && album.tagIds.forEach((id: number) => doMap(id, mappings.tags)); + album.artistIds && album.artistIds.forEach((id: number) => doMap(id, mappings.artists)); + album.trackIds && album.trackIds.forEach((id: number) => doMap(id, mappings.tracks)); + if (album.id) { album.id = doMap(album.id, mappings.albums); } + break; + } + case DBActionType.CreateTag: + case DBActionType.PutTag: + case DBActionType.PatchTag: + { + let tag = r.payload as TagWithRefsWithId; + if (tag.parentId) { tag.parentId = doMap(tag.parentId, mappings.tags); } + if (tag.id) { tag.id = doMap(tag.id, mappings.tags); } + break; + } case DBActionType.DeleteTrack: case DBActionType.DeleteArtist: case DBActionType.DeleteAlbum: diff --git a/server/test/reference_model/DBReferenceModel.ts b/server/test/reference_model/DBReferenceModel.ts index a53a5db..5de36f2 100644 --- a/server/test/reference_model/DBReferenceModel.ts +++ b/server/test/reference_model/DBReferenceModel.ts @@ -1,5 +1,6 @@ import { AlbumBaseWithRefs, AlbumWithRefsWithId, ArtistBaseWithRefs, ArtistWithRefsWithId, DBDataFormat, PostAlbumRequest, PostArtistRequest, PostTagRequest, PostTrackRequest, TagBaseWithRefs, TagWithRefsWithId, TrackBaseWithRefs, TrackWithDetails, TrackWithRefsWithId } from "../../../client/src/api/api"; import { makeNotFoundError } from "../../db/common"; +import { DBError, DBErrorKind } from "../../endpoints/types"; import filterInPlace from "../../lib/filterInPlace"; let _ = require('lodash'); @@ -29,7 +30,7 @@ function checkExists(objects: any[], ids: number[]) { // If not in the array, put the number in the array. function ensureInSet(n: number, s: number[]) { - if (!(n in s)) { s.push(n); } + if (!s.includes(n)) { s.push(n); } } // For a set of objects, ensure they point to another object. @@ -302,11 +303,9 @@ export function deleteAlbum(userId: number, id: number, db: ReferenceDatabase): } // Delete a tag. -export function deleteTag(userId: number, id: number, db: ReferenceDatabase, recursiveIdsSoFar: number[] = []): void { +export function deleteTag(userId: number, id: number, db: ReferenceDatabase): void { // Tags are special in that deleting them also deletes their children. Implement that here // with recursive calls. - let _recursiveIdsSoFar = [...recursiveIdsSoFar, id] - if (!(userId in db)) { throw makeNotFoundError() } let tag = db[userId].tags.find((o: any) => 'id' in o && o.id === id); if (!tag) { @@ -314,10 +313,7 @@ export function deleteTag(userId: number, id: number, db: ReferenceDatabase, rec } let children = db[userId].tags.filter((t: TagWithRefsWithId) => t.parentId === id); children.forEach((child: TagWithRefsWithId) => { - // Prevent cyclic dependencies. - if (!_recursiveIdsSoFar.includes(child.id)) { - deleteTag(userId, child.id, db, _recursiveIdsSoFar) - } + deleteTag(userId, child.id, db) }) // Do the actual deletion of this tag. @@ -372,6 +368,32 @@ export function modifyAlbum(userId: number, id: number, updates: AlbumBaseWithRe // Modify a tag. export function modifyTag(userId: number, id: number, updates: TagBaseWithRefs, db: ReferenceDatabase): void { + // A special case for tracks is that we do not allow "parent cycles", which could happen + // through modification. Catch that here. + if (!(userId in db)) { + throw makeNotFoundError() + } + let tag = (db[userId].tags as any[]).find((o: any) => 'id' in o && o.id === id); + if (!tag) { + throw makeNotFoundError() + } + + let getChildrenRecursive: (tag: TagWithRefsWithId) => number[] = (tag: TagWithRefsWithId) => { + let directChildren: TagWithRefsWithId[] = db[userId].tags + .filter((t: TagWithRefsWithId) => t.parentId === tag.id); + let indirectChildren: number[][] = directChildren.map((child: TagWithRefsWithId) => getChildrenRecursive(child)) + return [...(directChildren.map((t: TagWithRefsWithId) => t.id)), ...indirectChildren.flat()]; + } + + if (updates.parentId && [...getChildrenRecursive(tag), id].includes(updates.parentId)) { + const e: DBError = { + name: "DBError", + kind: DBErrorKind.ResourceConflict, + message: 'Modifying this tag would cause a tag parent cycle.', + }; + throw e; + } + return modifyObject(userId, id, updates, 'tags', [{ field: 'parentId', otherObjectType: 'tags', doReverseReference: false },], [], diff --git a/server/test/reference_model/randomGen.ts b/server/test/reference_model/randomGen.ts index 14c23c2..3a7a19b 100644 --- a/server/test/reference_model/randomGen.ts +++ b/server/test/reference_model/randomGen.ts @@ -251,6 +251,9 @@ export function applyReferenceDBAction( if (e.kind === DBErrorKind.ResourceNotFound) { status = 404; response = {}; + } else if(e.kind === DBErrorKind.ResourceConflict) { + status = 409; + response = {}; } } else { throw e; @@ -422,7 +425,7 @@ export function randomDBAction( DBActionType.PatchTrack | DBActionType.PatchTag | DBActionType.PatchAlbum | DBActionType.PatchArtist | DBActionType.PutTrack | DBActionType.PutTag | DBActionType.PutAlbum | DBActionType.PutArtist let validIdx = Math.floor(randomNumGen() * objectArrays[_type].length); - let validId = objectArrays[_type][validIdx].id; + let validId = objectArrays[_type][validIdx] ? objectArrays[_type][validIdx].id : 1; object.payload.id = applyDistribution(params[_type].validId, randomNumGen) ? validId : validId + 99999; } return object; @@ -444,7 +447,7 @@ export function randomDBAction( [DBActionType.DeleteTag]: db[userId].tags, } let validIdx = Math.floor(randomNumGen() * objectArrays[type].length); - let validId = objectArrays[type][validIdx].id; + let validId = objectArrays[type][validIdx] ? objectArrays[type][validIdx].id : 1; return { type: type, payload: applyDistribution(params[type].validId, randomNumGen) ? validId : validId + 99999,