diff --git a/server/controllers/AuthorController.js b/server/controllers/AuthorController.js index 0d99eb6a2..4af7f65cd 100644 --- a/server/controllers/AuthorController.js +++ b/server/controllers/AuthorController.js @@ -353,8 +353,11 @@ class AuthorController { return res.status(400).send('Library item already exists at that path') } + let createdPlaceholderDirectory = false try { + const placeholderDirectoryExisted = await fs.pathExists(outputDirectory) await fs.ensureDir(outputDirectory) + createdPlaceholderDirectory = !placeholderDirectoryExisted } catch (error) { Logger.error(`[AuthorController] Failed to create placeholder directory "${outputDirectory}"`, error) return res.status(500).send('Failed to create placeholder directory') @@ -430,6 +433,11 @@ class AuthorController { } catch (error) { Logger.error('[AuthorController] Failed to create author placeholder', error) await transaction.rollback() + if (createdPlaceholderDirectory) { + await fs.remove(outputDirectory).catch((cleanupError) => { + Logger.error(`[AuthorController] Failed to remove placeholder directory after rollback "${outputDirectory}"`, cleanupError) + }) + } return res.status(500).send('Failed to create placeholder') } diff --git a/server/controllers/MiscController.js b/server/controllers/MiscController.js index 923e84eb1..83c59a447 100644 --- a/server/controllers/MiscController.js +++ b/server/controllers/MiscController.js @@ -12,6 +12,7 @@ const patternValidation = require('../libs/nodeCron/pattern-validation') const { isObject, getTitleIgnorePrefix } = require('../utils/index') const fileUtils = require('../utils/fileUtils') const { sanitizeFilename } = fileUtils +const globals = require('../utils/globals') const LibraryItemScanner = require('../scanner/LibraryItemScanner') const LibraryScanner = require('../scanner/LibraryScanner') @@ -146,6 +147,14 @@ class MiscController { } if (placeholderItem) { + const hasUploadMedia = files.some((file) => { + const ext = Path.extname(file?.name || '') + .slice(1) + .toLowerCase() + if (!ext) return false + return globals.SupportedAudioTypes.includes(ext) || globals.SupportedEbookTypes.includes(ext) + }) + // Promote placeholder and attach files when upload targets a placeholder folder. const updateDetails = { libraryFolderId: folder.id, @@ -153,8 +162,10 @@ class MiscController { path: outputDirectory, isFile: placeholderItem.isFile } - await LibraryScanner.promotePlaceholder(placeholderItem) - await LibraryItemScanner.scanLibraryItem(placeholderItem.id, updateDetails) + if (hasUploadMedia) { + await LibraryScanner.promotePlaceholder(placeholderItem) + await LibraryItemScanner.scanLibraryItem(placeholderItem.id, updateDetails) + } } res.sendStatus(200) diff --git a/server/controllers/SeriesController.js b/server/controllers/SeriesController.js index 72ae730c1..aad9c5d1a 100644 --- a/server/controllers/SeriesController.js +++ b/server/controllers/SeriesController.js @@ -195,8 +195,11 @@ class SeriesController { return res.status(400).send('Library item already exists at that path') } + let createdPlaceholderDirectory = false try { + const placeholderDirectoryExisted = await fs.pathExists(outputDirectory) await fs.ensureDir(outputDirectory) + createdPlaceholderDirectory = !placeholderDirectoryExisted } catch (error) { Logger.error(`[SeriesController] Failed to create placeholder directory "${outputDirectory}"`, error) return res.status(500).send('Failed to create placeholder directory') @@ -280,6 +283,11 @@ class SeriesController { } catch (error) { Logger.error(`[SeriesController] Failed to create series placeholder`, error) await transaction.rollback() + if (createdPlaceholderDirectory) { + await fs.remove(outputDirectory).catch((cleanupError) => { + Logger.error(`[SeriesController] Failed to remove placeholder directory after rollback "${outputDirectory}"`, cleanupError) + }) + } return res.status(500).send('Failed to create placeholder') } diff --git a/test/server/controllers/AuthorController.placeholders.test.js b/test/server/controllers/AuthorController.placeholders.test.js index a2ef915d2..e51d90a6c 100644 --- a/test/server/controllers/AuthorController.placeholders.test.js +++ b/test/server/controllers/AuthorController.placeholders.test.js @@ -32,6 +32,7 @@ describe('AuthorController placeholders', () => { }) sinon.stub(Logger, 'warn') + sinon.stub(fs, 'pathExists').resolves(false) sinon.stub(fs, 'ensureDir').resolves() library = await Database.libraryModel.create({ name: 'Test Library', mediaType: 'book' }) @@ -167,4 +168,61 @@ describe('AuthorController placeholders', () => { expect(payload.folderId).to.equal(secondaryLibraryFolder.id) expect(payload.path.startsWith(secondaryLibraryFolder.path)).to.be.true }) + + it('removes placeholder directory when creation fails after directory is created', async () => { + sinon.stub(fs, 'remove').resolves() + fs.pathExists.resolves(false) + sinon.stub(Database.libraryItemModel, 'create').rejects(new Error('library item create failed')) + + const fakeReq = { + params: { + id: author.id + }, + author, + user, + body: {} + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + json: sinon.spy(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await AuthorController.createPlaceholder.bind(apiRouter)(fakeReq, fakeRes) + + expect(fakeRes.status.calledWith(500)).to.be.true + expect(fakeRes.send.calledWith('Failed to create placeholder')).to.be.true + expect(fs.remove.calledOnce).to.be.true + expect(fs.remove.firstCall.args[0]).to.equal('/test/Jane Doe/Placeholder') + }) + + it('does not remove placeholder directory on rollback when it already existed', async () => { + sinon.stub(fs, 'remove').resolves() + fs.pathExists.resolves(true) + sinon.stub(Database.libraryItemModel, 'create').rejects(new Error('library item create failed')) + + const fakeReq = { + params: { + id: author.id + }, + author, + user, + body: {} + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + json: sinon.spy(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await AuthorController.createPlaceholder.bind(apiRouter)(fakeReq, fakeRes) + + expect(fakeRes.status.calledWith(500)).to.be.true + expect(fakeRes.send.calledWith('Failed to create placeholder')).to.be.true + expect(fs.remove.called).to.be.false + }) }) diff --git a/test/server/controllers/MiscController.placeholders.test.js b/test/server/controllers/MiscController.placeholders.test.js new file mode 100644 index 000000000..c64031dd8 --- /dev/null +++ b/test/server/controllers/MiscController.placeholders.test.js @@ -0,0 +1,118 @@ +const { expect } = require('chai') +const { Sequelize } = require('sequelize') +const sinon = require('sinon') + +const MiscController = require('../../../server/controllers/MiscController') +const Database = require('../../../server/Database') +const fs = require('../../../server/libs/fsExtra') +const LibraryScanner = require('../../../server/scanner/LibraryScanner') +const LibraryItemScanner = require('../../../server/scanner/LibraryItemScanner') + +describe('MiscController placeholder uploads', () => { + let library + let folder + let placeholderItem + + beforeEach(async () => { + global.ServerSettings = {} + Database.sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }) + Database.sequelize.uppercaseFirst = (str) => (str ? `${str[0].toUpperCase()}${str.substr(1)}` : '') + await Database.buildModels() + + library = await Database.libraryModel.create({ name: 'Test Library', mediaType: 'book' }) + folder = await Database.libraryFolderModel.create({ path: '/library', libraryId: library.id }) + const book = await Database.bookModel.create({ title: 'Placeholder', audioFiles: [], tags: [], narrators: [], genres: [], chapters: [] }) + placeholderItem = await Database.libraryItemModel.create({ + libraryFiles: [], + mediaId: book.id, + mediaType: 'book', + libraryId: library.id, + libraryFolderId: folder.id, + relPath: 'Author/Title', + path: '/library/Author/Title', + isFile: false, + isPlaceholder: true + }) + + sinon.stub(fs, 'ensureDir').resolves() + sinon.stub(LibraryScanner, 'promotePlaceholder').resolves() + sinon.stub(LibraryItemScanner, 'scanLibraryItem').resolves() + }) + + afterEach(async () => { + sinon.restore() + await Database.sequelize.sync({ force: true }) + }) + + it('does not promote placeholder-targeted uploads when files are non-media only', async () => { + const fakeReq = { + user: { + canUpload: true, + username: 'admin', + checkCanAccessLibrary: () => true + }, + body: { + title: 'Placeholder', + library: library.id, + folder: folder.id, + placeholder: `id:${placeholderItem.id}` + }, + files: { + cover: { + name: 'cover.jpg', + mv: sinon.stub().resolves() + } + } + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await MiscController.handleUpload(fakeReq, fakeRes) + + expect(LibraryScanner.promotePlaceholder.called).to.be.false + expect(LibraryItemScanner.scanLibraryItem.called).to.be.false + expect(fakeRes.sendStatus.calledWith(200)).to.be.true + }) + + it('promotes placeholder-targeted uploads when files include media', async () => { + const fakeReq = { + user: { + canUpload: true, + username: 'admin', + checkCanAccessLibrary: () => true + }, + body: { + title: 'Placeholder', + library: library.id, + folder: folder.id, + placeholder: `id:${placeholderItem.id}` + }, + files: { + audio: { + name: 'track-01.mp3', + mv: sinon.stub().resolves() + }, + cover: { + name: 'cover.jpg', + mv: sinon.stub().resolves() + } + } + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await MiscController.handleUpload(fakeReq, fakeRes) + + expect(LibraryScanner.promotePlaceholder.calledOnce).to.be.true + expect(LibraryItemScanner.scanLibraryItem.calledOnce).to.be.true + expect(fakeRes.sendStatus.calledWith(200)).to.be.true + }) +}) diff --git a/test/server/controllers/SeriesController.placeholders.test.js b/test/server/controllers/SeriesController.placeholders.test.js index a61eebda5..d95b178cb 100644 --- a/test/server/controllers/SeriesController.placeholders.test.js +++ b/test/server/controllers/SeriesController.placeholders.test.js @@ -32,6 +32,7 @@ describe('SeriesController placeholders', () => { }) sinon.stub(Logger, 'warn') + sinon.stub(fs, 'pathExists').resolves(false) sinon.stub(fs, 'ensureDir').resolves() library = await Database.libraryModel.create({ name: 'Test Library', mediaType: 'book' }) @@ -309,4 +310,61 @@ describe('SeriesController placeholders', () => { expect(payload.path.startsWith(libraryFolder.path)).to.be.true expect(payload.path).to.include('/Test Series/Placeholder') }) + + it('removes placeholder directory when creation fails after directory is created', async () => { + sinon.stub(fs, 'remove').resolves() + fs.pathExists.resolves(false) + sinon.stub(Database.libraryItemModel, 'create').rejects(new Error('library item create failed')) + + const fakeReq = { + params: { + id: library.id, + seriesId: series.id + }, + user, + body: {} + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + json: sinon.spy(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await SeriesController.createPlaceholder.bind(apiRouter)(fakeReq, fakeRes) + + expect(fakeRes.status.calledWith(500)).to.be.true + expect(fakeRes.send.calledWith('Failed to create placeholder')).to.be.true + expect(fs.remove.calledOnce).to.be.true + expect(fs.remove.firstCall.args[0]).to.equal('/test/Test Series/Placeholder') + }) + + it('does not remove placeholder directory on rollback when it already existed', async () => { + sinon.stub(fs, 'remove').resolves() + fs.pathExists.resolves(true) + sinon.stub(Database.libraryItemModel, 'create').rejects(new Error('library item create failed')) + + const fakeReq = { + params: { + id: library.id, + seriesId: series.id + }, + user, + body: {} + } + + const fakeRes = { + status: sinon.stub().returnsThis(), + json: sinon.spy(), + sendStatus: sinon.spy(), + send: sinon.spy() + } + + await SeriesController.createPlaceholder.bind(apiRouter)(fakeReq, fakeRes) + + expect(fakeRes.status.calledWith(500)).to.be.true + expect(fakeRes.send.calledWith('Failed to create placeholder')).to.be.true + expect(fs.remove.called).to.be.false + }) })