From e8c5fcf64db3fbb2734c3d155f27fab9088685dc Mon Sep 17 00:00:00 2001 From: Jonathan Jogenfors Date: Wed, 18 Dec 2024 13:52:26 +0100 Subject: [PATCH] fix: don't delete offline files from disk when emptying trash --- e2e/src/api/specs/asset.e2e-spec.ts | 34 ++++- e2e/src/api/specs/library.e2e-spec.ts | 205 ++++++++++++++++++++++++-- e2e/src/api/specs/trash.e2e-spec.ts | 47 +++++- e2e/src/utils.ts | 15 +- server/src/services/asset.service.ts | 13 +- server/src/services/trash.service.ts | 8 +- 6 files changed, 299 insertions(+), 23 deletions(-) diff --git a/e2e/src/api/specs/asset.e2e-spec.ts b/e2e/src/api/specs/asset.e2e-spec.ts index a0c429a82e224..b4a298edf3fae 100644 --- a/e2e/src/api/specs/asset.e2e-spec.ts +++ b/e2e/src/api/specs/asset.e2e-spec.ts @@ -766,7 +766,7 @@ describe('/asset', () => { expect(body).toEqual(errorDto.badRequest('Not found or no asset.delete access')); }); - it('should move an asset to the trash', async () => { + it('should move an asset to trash', async () => { const { id: assetId } = await utils.createAsset(admin.accessToken); const before = await utils.getAssetInfo(admin.accessToken, assetId); @@ -782,6 +782,38 @@ describe('/asset', () => { expect(after.isTrashed).toBe(true); }); + it('should permanently delete an asset from trash', async () => { + const { id: assetId } = await utils.createAsset(admin.accessToken); + + { + const { status } = await request(app) + .delete('/assets') + .send({ ids: [assetId] }) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(204); + } + + const trashed = await utils.getAssetInfo(admin.accessToken, assetId); + expect(trashed.isTrashed).toBe(true); + + { + const { status } = await request(app) + .delete('/assets') + .send({ ids: [assetId], force: true }) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(204); + } + + await utils.waitForWebsocketEvent({ event: 'assetDelete', id: assetId }); + + { + const { status } = await request(app) + .get(`/assets/${assetId}`) + .set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(400); + } + }); + it('should clean up live photos', async () => { const { id: motionId } = await utils.createAsset(admin.accessToken, { assetData: { filename: 'test.mp4', bytes: makeRandomImage() }, diff --git a/e2e/src/api/specs/library.e2e-spec.ts b/e2e/src/api/specs/library.e2e-spec.ts index 3f910fa1e3756..3a7562847ec11 100644 --- a/e2e/src/api/specs/library.e2e-spec.ts +++ b/e2e/src/api/specs/library.e2e-spec.ts @@ -499,7 +499,7 @@ describe('/libraries', () => { expect(newAssets.items).toEqual([]); }); - it('should set an asset offline its file is not in any import path', async () => { + it('should set an asset offline if its file is not in any import path', async () => { utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); const library = await utils.createLibrary(admin.accessToken, { @@ -515,10 +515,9 @@ describe('/libraries', () => { utils.createDirectory(`${testAssetDir}/temp/another-path/`); - await request(app) - .put(`/libraries/${library.id}`) - .set('Authorization', `Bearer ${admin.accessToken}`) - .send({ importPaths: [`${testAssetDirInternal}/temp/another-path/`] }); + await utils.updateLibrary(admin.accessToken, library.id, { + importPaths: [`${testAssetDirInternal}/temp/another-path/`], + }); const { status } = await request(app) .post(`/libraries/${library.id}/scan`) @@ -555,10 +554,7 @@ describe('/libraries', () => { }); expect(assets.count).toBe(1); - await request(app) - .put(`/libraries/${library.id}`) - .set('Authorization', `Bearer ${admin.accessToken}`) - .send({ exclusionPatterns: ['**/directoryB/**'] }); + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/directoryB/**'] }); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); @@ -577,7 +573,7 @@ describe('/libraries', () => { ]); }); - it('should not trash an online asset', async () => { + it('should not set an asset offline if its file exists, is in an import path, and not covered by an exclusion pattern', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp`], @@ -601,6 +597,195 @@ describe('/libraries', () => { expect(assets).toEqual(assetsBefore); }); + + it('should set an offline asset to online if its file exists, is in an import path, and not covered by an exclusion pattern', async () => { + utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); + + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp/offline`], + }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id }); + + utils.renameImageFile(`${testAssetDir}/temp/offline/offline.png`, `${testAssetDir}/temp/offline.png`); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const offlineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + expect(offlineAsset.isTrashed).toBe(true); + expect(offlineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(offlineAsset.isOffline).toBe(true); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id, withDeleted: true }); + expect(assets.count).toBe(1); + } + + utils.renameImageFile(`${testAssetDir}/temp/offline.png`, `${testAssetDir}/temp/offline/offline.png`); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const backOnlineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + + expect(backOnlineAsset.isTrashed).toBe(false); + expect(backOnlineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(backOnlineAsset.isOffline).toBe(false); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id }); + expect(assets.count).toBe(1); + } + }); + + it('should not set an offline asset to online if its file exists, is not covered by an exclusion pattern, but is outside of all import paths', async () => { + utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); + + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp/offline`], + }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id }); + + utils.renameImageFile(`${testAssetDir}/temp/offline/offline.png`, `${testAssetDir}/temp/offline.png`); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id, withDeleted: true }); + expect(assets.count).toBe(1); + } + + const offlineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + + expect(offlineAsset.isTrashed).toBe(true); + expect(offlineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(offlineAsset.isOffline).toBe(true); + + utils.renameImageFile(`${testAssetDir}/temp/offline.png`, `${testAssetDir}/temp/offline/offline.png`); + + utils.createDirectory(`${testAssetDir}/temp/another-path/`); + + await utils.updateLibrary(admin.accessToken, library.id, { + importPaths: [`${testAssetDirInternal}/temp/another-path`], + }); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const stillOfflineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + + expect(stillOfflineAsset.isTrashed).toBe(true); + expect(stillOfflineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(stillOfflineAsset.isOffline).toBe(true); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id, withDeleted: true }); + expect(assets.count).toBe(1); + } + + utils.removeDirectory(`${testAssetDir}/temp/another-path/`); + }); + + it('should not set an offline asset to online if its file exists, is in an import path, but is covered by an exclusion pattern', async () => { + utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); + + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp/offline`], + }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id }); + + utils.renameImageFile(`${testAssetDir}/temp/offline/offline.png`, `${testAssetDir}/temp/offline.png`); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id, withDeleted: true }); + expect(assets.count).toBe(1); + } + + const offlineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + + expect(offlineAsset.isTrashed).toBe(true); + expect(offlineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(offlineAsset.isOffline).toBe(true); + + utils.renameImageFile(`${testAssetDir}/temp/offline.png`, `${testAssetDir}/temp/offline/offline.png`); + + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/offline/**'] }); + + { + const { status } = await request(app) + .post(`/libraries/${library.id}/scan`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send(); + expect(status).toBe(204); + } + + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const stillOfflineAsset = await utils.getAssetInfo(admin.accessToken, assets.items[0].id); + + expect(stillOfflineAsset.isTrashed).toBe(true); + expect(stillOfflineAsset.originalPath).toBe(`${testAssetDirInternal}/temp/offline/offline.png`); + expect(stillOfflineAsset.isOffline).toBe(true); + + { + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id, withDeleted: true }); + expect(assets.count).toBe(1); + } + }); }); describe('POST /libraries/:id/validate', () => { diff --git a/e2e/src/api/specs/trash.e2e-spec.ts b/e2e/src/api/specs/trash.e2e-spec.ts index f86f38ab61d2f..15b915ef2a769 100644 --- a/e2e/src/api/specs/trash.e2e-spec.ts +++ b/e2e/src/api/specs/trash.e2e-spec.ts @@ -73,7 +73,7 @@ describe('/trash', () => { expect(existsSync(before.originalPath)).toBe(false); }); - it('should not delete offline-trashed assets from disk', async () => { + it('should remove offline assets', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp/offline`], @@ -88,7 +88,7 @@ describe('/trash', () => { expect(assets.items.length).toBe(1); const asset = assets.items[0]; - utils.removeImageFile(`${testAssetDir}/temp/offline/offline.png`); + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/offline/**'] }); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); @@ -105,6 +105,41 @@ describe('/trash', () => { const assetAfter = await utils.getAssetInfo(admin.accessToken, asset.id); expect(assetAfter).toMatchObject({ isTrashed: true, isOffline: true }); + }); + + it.skip('should not delete offline assets from disk', async () => { + // Can't be tested at the moment due to no mechanism to forward time + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp/offline`], + }); + + utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets } = await utils.searchAssets(admin.accessToken, { libraryId: library.id }); + expect(assets.items.length).toBe(1); + const asset = assets.items[0]; + + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/offline/**'] }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const assetBefore = await utils.getAssetInfo(admin.accessToken, asset.id); + expect(assetBefore).toMatchObject({ isTrashed: true, isOffline: true }); + + utils.createImageFile(`${testAssetDir}/temp/offline/offline.png`); + + const { status } = await request(app).post('/trash/empty').set('Authorization', `Bearer ${admin.accessToken}`); + expect(status).toBe(200); + + await utils.waitForQueueFinish(admin.accessToken, 'backgroundTask'); + + const after = await getAssetStatistics({ isTrashed: true }, { headers: asBearerAuth(admin.accessToken) }); + expect(after.total).toBe(0); expect(existsSync(`${testAssetDir}/temp/offline/offline.png`)).toBe(true); @@ -137,7 +172,7 @@ describe('/trash', () => { expect(after).toStrictEqual(expect.objectContaining({ id: assetId, isTrashed: false })); }); - it('should not restore offline-trashed assets', async () => { + it('should not restore offline assets', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp/offline`], @@ -152,7 +187,7 @@ describe('/trash', () => { expect(assets.count).toBe(1); const assetId = assets.items[0].id; - utils.removeImageFile(`${testAssetDir}/temp/offline/offline.png`); + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/offline/**'] }); await scan(admin.accessToken, library.id); @@ -195,7 +230,7 @@ describe('/trash', () => { expect(after.isTrashed).toBe(false); }); - it('should not restore an offline-trashed asset', async () => { + it('should not restore an offline asset', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp/offline`], @@ -210,7 +245,7 @@ describe('/trash', () => { expect(assets.count).toBe(1); const assetId = assets.items[0].id; - utils.removeImageFile(`${testAssetDir}/temp/offline/offline.png`); + await utils.updateLibrary(admin.accessToken, library.id, { exclusionPatterns: ['**/offline/**'] }); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); diff --git a/e2e/src/utils.ts b/e2e/src/utils.ts index 14225ff063038..7b80ba49aab28 100644 --- a/e2e/src/utils.ts +++ b/e2e/src/utils.ts @@ -10,6 +10,7 @@ import { Permission, PersonCreateDto, SharedLinkCreateDto, + UpdateLibraryDto, UserAdminCreateDto, UserPreferencesUpdateDto, ValidateLibraryDto, @@ -35,6 +36,7 @@ import { updateAlbumUser, updateAssets, updateConfig, + updateLibrary, updateMyPreferences, upsertTags, validate, @@ -42,7 +44,7 @@ import { import { BrowserContext } from '@playwright/test'; import { exec, spawn } from 'node:child_process'; import { createHash } from 'node:crypto'; -import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path, { dirname } from 'node:path'; import { setTimeout as setAsyncTimeout } from 'node:timers/promises'; @@ -392,6 +394,14 @@ export const utils = { rmSync(path); }, + renameImageFile: (oldPath: string, newPath: string) => { + if (!existsSync(oldPath)) { + return; + } + + renameSync(oldPath, newPath); + }, + removeDirectory: (path: string) => { if (!existsSync(path)) { return; @@ -447,6 +457,9 @@ export const utils = { validateLibrary: (accessToken: string, id: string, dto: ValidateLibraryDto) => validate({ id, validateLibraryDto: dto }, { headers: asBearerAuth(accessToken) }), + updateLibrary: (accessToken: string, id: string, dto: UpdateLibraryDto) => + updateLibrary({ id, updateLibraryDto: dto }, { headers: asBearerAuth(accessToken) }), + createPartner: (accessToken: string, id: string) => createPartner({ id }, { headers: asBearerAuth(accessToken) }), updateMyPreferences: (accessToken: string, userPreferencesUpdateDto: UserPreferencesUpdateDto) => diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index 87510371192e4..c676b44414960 100644 --- a/server/src/services/asset.service.ts +++ b/server/src/services/asset.service.ts @@ -249,7 +249,18 @@ export class AssetService extends BaseService { const { thumbnailFile, previewFile } = getAssetFiles(asset.files); const files = [thumbnailFile?.path, previewFile?.path, asset.encodedVideoPath]; - if (deleteOnDisk) { + + let willDelete = deleteOnDisk; + + if (asset.isOffline) { + /* We don't want to delete an offline asset because it is either... + ...missing from disk => don't delete the file since it doesn't exist where we expect + ...outside of any import path => don't delete the file since we're not responsible for it + ...matching an exclusion pattern => don't delete the file since we're not responsible for it */ + willDelete = false; + } + + if (willDelete) { files.push(asset.sidecarPath, asset.originalPath); } diff --git a/server/src/services/trash.service.ts b/server/src/services/trash.service.ts index 621dee0f8176d..8136ff4c7e293 100644 --- a/server/src/services/trash.service.ts +++ b/server/src/services/trash.service.ts @@ -18,7 +18,7 @@ export class TrashService extends BaseService { await this.trashRepository.restoreAll(ids); await this.eventRepository.emit('assets.restore', { assetIds: ids, userId: auth.user.id }); - this.logger.log(`Restored ${ids.length} assets from trash`); + this.logger.log(`Restored ${ids.length} asset(s) from trash`); return { count: ids.length }; } @@ -26,7 +26,7 @@ export class TrashService extends BaseService { async restore(auth: AuthDto): Promise { const count = await this.trashRepository.restore(auth.user.id); if (count > 0) { - this.logger.log(`Restored ${count} assets from trash`); + this.logger.log(`Restored ${count} asset(s) from trash`); } return { count }; } @@ -52,7 +52,7 @@ export class TrashService extends BaseService { ); for await (const assetIds of assetPagination) { - this.logger.debug(`Queueing ${assetIds.length} assets for deletion from the trash`); + this.logger.debug(`Queueing ${assetIds.length} asset(s) for deletion from the trash`); count += assetIds.length; await this.jobRepository.queueAll( assetIds.map((assetId) => ({ @@ -65,7 +65,7 @@ export class TrashService extends BaseService { ); } - this.logger.log(`Queued ${count} assets for deletion from the trash`); + this.logger.log(`Queued ${count} asset(s) for deletion from the trash`); return JobStatus.SUCCESS; }