From 7651f70c888618e510e91a07e157565573331b3d Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Mon, 10 Jun 2024 13:04:34 -0400 Subject: [PATCH] fix(server): asset delete logic (#10077) * fix(server): asset delete logic * test: e2e --- e2e/src/api/specs/library.e2e-spec.ts | 64 ++++++++++++++++++-- server/src/interfaces/job.interface.ts | 6 +- server/src/services/asset.service.spec.ts | 25 +++++--- server/src/services/asset.service.ts | 29 ++++++--- server/src/services/library.service.spec.ts | 2 +- server/src/services/library.service.ts | 16 ++++- server/src/services/metadata.service.spec.ts | 2 +- server/src/services/metadata.service.ts | 5 +- server/src/services/trash.service.spec.ts | 2 +- server/src/services/trash.service.ts | 8 ++- 10 files changed, 130 insertions(+), 29 deletions(-) diff --git a/e2e/src/api/specs/library.e2e-spec.ts b/e2e/src/api/specs/library.e2e-spec.ts index 762606de5e..59968f3b79 100644 --- a/e2e/src/api/specs/library.e2e-spec.ts +++ b/e2e/src/api/specs/library.e2e-spec.ts @@ -1,4 +1,11 @@ -import { LibraryResponseDto, LoginResponseDto, ScanLibraryDto, getAllLibraries, scanLibrary } from '@immich/sdk'; +import { + LibraryResponseDto, + LoginResponseDto, + ScanLibraryDto, + getAllLibraries, + removeOfflineFiles, + scanLibrary, +} from '@immich/sdk'; import { cpSync, existsSync } from 'node:fs'; import { Socket } from 'socket.io-client'; import { userDto, uuidDto } from 'src/fixtures'; @@ -384,6 +391,51 @@ describe('/libraries', () => { ); }); + it('should not try to delete offline files', async () => { + utils.createImageFile(`${testAssetDir}/temp/offline1/assetA.png`); + + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp/offline1`], + }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets: initialAssets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + expect(initialAssets).toEqual({ + count: 1, + total: 1, + facets: [], + items: [expect.objectContaining({ originalFileName: 'assetA.png' })], + nextPage: null, + }); + + utils.removeImageFile(`${testAssetDir}/temp/offline1/assetA.png`); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets: offlineAssets } = await utils.metadataSearch(admin.accessToken, { + libraryId: library.id, + isOffline: true, + }); + expect(offlineAssets).toEqual({ + count: 1, + total: 1, + facets: [], + items: [expect.objectContaining({ originalFileName: 'assetA.png' })], + nextPage: null, + }); + + utils.createImageFile(`${testAssetDir}/temp/offline1/assetA.png`); + await removeOfflineFiles({ id: library.id }, { headers: asBearerAuth(admin.accessToken) }); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + await utils.waitForWebsocketEvent({ event: 'assetDelete', total: 1 }); + + expect(existsSync(`${testAssetDir}/temp/offline1/assetA.png`)).toBe(true); + }); + it('should scan new files', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, @@ -507,10 +559,10 @@ describe('/libraries', () => { it('should remove offline files', async () => { const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, - importPaths: [`${testAssetDirInternal}/temp`], + importPaths: [`${testAssetDirInternal}/temp/offline2`], }); - utils.createImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + utils.createImageFile(`${testAssetDir}/temp/offline2/assetA.png`); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); @@ -518,9 +570,9 @@ describe('/libraries', () => { const { assets: initialAssets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id, }); - expect(initialAssets.count).toBe(3); + expect(initialAssets.count).toBe(1); - utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + utils.removeImageFile(`${testAssetDir}/temp/offline2/assetA.png`); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); @@ -541,7 +593,7 @@ describe('/libraries', () => { const { assets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); - expect(assets.count).toBe(2); + expect(assets.count).toBe(0); }); it('should not remove online files', async () => { diff --git a/server/src/interfaces/job.interface.ts b/server/src/interfaces/job.interface.ts index 3497a12969..69e5226cfe 100644 --- a/server/src/interfaces/job.interface.ts +++ b/server/src/interfaces/job.interface.ts @@ -120,6 +120,10 @@ export interface IEntityJob extends IBaseJob { source?: 'upload' | 'sidecar-write' | 'copy'; } +export interface IAssetDeleteJob extends IEntityJob { + deleteOnDisk: boolean; +} + export interface ILibraryFileJob extends IEntityJob { ownerId: string; assetPath: string; @@ -246,7 +250,7 @@ export type JobItem = // Asset Deletion | { name: JobName.PERSON_CLEANUP; data?: IBaseJob } - | { name: JobName.ASSET_DELETION; data: IEntityJob } + | { name: JobName.ASSET_DELETION; data: IAssetDeleteJob } | { name: JobName.ASSET_DELETION_CHECK; data?: IBaseJob } // Library Management diff --git a/server/src/services/asset.service.spec.ts b/server/src/services/asset.service.spec.ts index 70a2b94ecc..73dbfb6393 100755 --- a/server/src/services/asset.service.spec.ts +++ b/server/src/services/asset.service.spec.ts @@ -389,8 +389,8 @@ describe(AssetService.name, () => { await sut.deleteAll(authStub.user1, { ids: ['asset1', 'asset2'], force: true }); expect(jobMock.queueAll).toHaveBeenCalledWith([ - { name: JobName.ASSET_DELETION, data: { id: 'asset1' } }, - { name: JobName.ASSET_DELETION, data: { id: 'asset2' } }, + { name: JobName.ASSET_DELETION, data: { id: 'asset1', deleteOnDisk: true } }, + { name: JobName.ASSET_DELETION, data: { id: 'asset2', deleteOnDisk: true } }, ]); }); @@ -410,7 +410,7 @@ describe(AssetService.name, () => { assetMock.getById.mockResolvedValue(assetWithFace); - await sut.handleAssetDeletion({ id: assetWithFace.id }); + await sut.handleAssetDeletion({ id: assetWithFace.id, deleteOnDisk: true }); expect(jobMock.queue.mock.calls).toEqual([ [ @@ -435,7 +435,7 @@ describe(AssetService.name, () => { it('should update stack primary asset if deleted asset was primary asset in a stack', async () => { assetMock.getById.mockResolvedValue(assetStub.primaryImage as AssetEntity); - await sut.handleAssetDeletion({ id: assetStub.primaryImage.id }); + await sut.handleAssetDeletion({ id: assetStub.primaryImage.id, deleteOnDisk: true }); expect(assetStackMock.update).toHaveBeenCalledWith({ id: 'stack-1', @@ -446,10 +446,21 @@ describe(AssetService.name, () => { it('should delete a live photo', async () => { assetMock.getById.mockResolvedValue(assetStub.livePhotoStillAsset); - await sut.handleAssetDeletion({ id: assetStub.livePhotoStillAsset.id }); + await sut.handleAssetDeletion({ + id: assetStub.livePhotoStillAsset.id, + deleteOnDisk: true, + }); expect(jobMock.queue.mock.calls).toEqual([ - [{ name: JobName.ASSET_DELETION, data: { id: assetStub.livePhotoMotionAsset.id } }], + [ + { + name: JobName.ASSET_DELETION, + data: { + id: assetStub.livePhotoMotionAsset.id, + deleteOnDisk: true, + }, + }, + ], [ { name: JobName.DELETE_FILES, @@ -463,7 +474,7 @@ describe(AssetService.name, () => { it('should update usage', async () => { assetMock.getById.mockResolvedValue(assetStub.image); - await sut.handleAssetDeletion({ id: assetStub.image.id }); + await sut.handleAssetDeletion({ id: assetStub.image.id, deleteOnDisk: true }); expect(userMock.updateUsage).toHaveBeenCalledWith(assetStub.image.ownerId, -5000); }); }); diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index 58ff0d1f85..e48de8c0c1 100644 --- a/server/src/services/asset.service.ts +++ b/server/src/services/asset.service.ts @@ -27,7 +27,7 @@ import { IAssetStackRepository } from 'src/interfaces/asset-stack.interface'; import { IAssetRepository } from 'src/interfaces/asset.interface'; import { ClientEvent, IEventRepository } from 'src/interfaces/event.interface'; import { - IEntityJob, + IAssetDeleteJob, IJobRepository, ISidecarWriteJob, JOBS_ASSET_PAGINATION_SIZE, @@ -256,15 +256,21 @@ export class AssetService { for await (const assets of assetPagination) { await this.jobRepository.queueAll( - assets.map((asset) => ({ name: JobName.ASSET_DELETION, data: { id: asset.id } })), + assets.map((asset) => ({ + name: JobName.ASSET_DELETION, + data: { + id: asset.id, + deleteOnDisk: true, + }, + })), ); } return JobStatus.SUCCESS; } - async handleAssetDeletion(job: IEntityJob): Promise { - const { id } = job; + async handleAssetDeletion(job: IAssetDeleteJob): Promise { + const { id, deleteOnDisk } = job; const asset = await this.assetRepository.getById(id, { faces: { @@ -301,12 +307,14 @@ export class AssetService { // TODO refactor this to use cascades if (asset.livePhotoVideoId) { - await this.jobRepository.queue({ name: JobName.ASSET_DELETION, data: { id: asset.livePhotoVideoId } }); + await this.jobRepository.queue({ + name: JobName.ASSET_DELETION, + data: { id: asset.livePhotoVideoId, deleteOnDisk }, + }); } const files = [asset.thumbnailPath, asset.previewPath, asset.encodedVideoPath]; - // skip originals if the user deleted the whole library - if (!asset.library?.deletedAt) { + if (deleteOnDisk) { files.push(asset.sidecarPath, asset.originalPath); } @@ -321,7 +329,12 @@ export class AssetService { await this.access.requirePermission(auth, Permission.ASSET_DELETE, ids); if (force) { - await this.jobRepository.queueAll(ids.map((id) => ({ name: JobName.ASSET_DELETION, data: { id } }))); + await this.jobRepository.queueAll( + ids.map((id) => ({ + name: JobName.ASSET_DELETION, + data: { id, deleteOnDisk: true }, + })), + ); } else { await this.assetRepository.softDeleteAll(ids); this.eventRepository.clientSend(ClientEvent.ASSET_TRASH, auth.user.id, ids); diff --git a/server/src/services/library.service.spec.ts b/server/src/services/library.service.spec.ts index b63df692a4..5b5da61770 100644 --- a/server/src/services/library.service.spec.ts +++ b/server/src/services/library.service.spec.ts @@ -1276,7 +1276,7 @@ describe(LibraryService.name, () => { await expect(sut.handleOfflineRemoval({ id: libraryStub.externalLibrary1.id })).resolves.toBe(JobStatus.SUCCESS); expect(jobMock.queueAll).toHaveBeenCalledWith([ - { name: JobName.ASSET_DELETION, data: { id: assetStub.image1.id } }, + { name: JobName.ASSET_DELETION, data: { id: assetStub.image1.id, deleteOnDisk: false } }, ]); }); }); diff --git a/server/src/services/library.service.ts b/server/src/services/library.service.ts index 9a01f2325f..8dbf31d23d 100644 --- a/server/src/services/library.service.ts +++ b/server/src/services/library.service.ts @@ -355,7 +355,13 @@ export class LibraryService { const assetIds = await this.repository.getAssetIds(job.id, true); this.logger.debug(`Will delete ${assetIds.length} asset(s) in library ${job.id}`); await this.jobRepository.queueAll( - assetIds.map((assetId) => ({ name: JobName.ASSET_DELETION, data: { id: assetId } })), + assetIds.map((assetId) => ({ + name: JobName.ASSET_DELETION, + data: { + id: assetId, + deleteOnDisk: false, + }, + })), ); if (assetIds.length === 0) { @@ -544,7 +550,13 @@ export class LibraryService { for await (const assets of assetPagination) { this.logger.debug(`Removing ${assets.length} offline assets`); await this.jobRepository.queueAll( - assets.map((asset) => ({ name: JobName.ASSET_DELETION, data: { id: asset.id } })), + assets.map((asset) => ({ + name: JobName.ASSET_DELETION, + data: { + id: asset.id, + deleteOnDisk: false, + }, + })), ); } diff --git a/server/src/services/metadata.service.spec.ts b/server/src/services/metadata.service.spec.ts index 2c2c59cbbb..b36a3c0be9 100644 --- a/server/src/services/metadata.service.spec.ts +++ b/server/src/services/metadata.service.spec.ts @@ -510,7 +510,7 @@ describe(MetadataService.name, () => { await sut.handleMetadataExtraction({ id: assetStub.livePhotoWithOriginalFileName.id }); expect(jobMock.queue).toHaveBeenNthCalledWith(1, { name: JobName.ASSET_DELETION, - data: { id: assetStub.livePhotoWithOriginalFileName.livePhotoVideoId }, + data: { id: assetStub.livePhotoWithOriginalFileName.livePhotoVideoId, deleteOnDisk: true }, }); expect(jobMock.queue).toHaveBeenNthCalledWith(2, { name: JobName.METADATA_EXTRACTION, diff --git a/server/src/services/metadata.service.ts b/server/src/services/metadata.service.ts index 0bafe032e2..5a0b363211 100644 --- a/server/src/services/metadata.service.ts +++ b/server/src/services/metadata.service.ts @@ -460,7 +460,10 @@ export class MetadataService { // (if it did, getByChecksum() would've returned a motionAsset with the same ID as livePhotoVideoId) // note asset.livePhotoVideoId is not motionAsset.id yet if (asset.livePhotoVideoId) { - await this.jobRepository.queue({ name: JobName.ASSET_DELETION, data: { id: asset.livePhotoVideoId } }); + await this.jobRepository.queue({ + name: JobName.ASSET_DELETION, + data: { id: asset.livePhotoVideoId, deleteOnDisk: true }, + }); this.logger.log(`Removed old motion photo video asset (${asset.livePhotoVideoId})`); } } diff --git a/server/src/services/trash.service.spec.ts b/server/src/services/trash.service.spec.ts index 7974efed62..73a4f3d57b 100644 --- a/server/src/services/trash.service.spec.ts +++ b/server/src/services/trash.service.spec.ts @@ -79,7 +79,7 @@ describe(TrashService.name, () => { assetMock.getByUserId.mockResolvedValue({ items: [assetStub.image], hasNextPage: false }); await expect(sut.empty(authStub.user1)).resolves.toBeUndefined(); expect(jobMock.queueAll).toHaveBeenCalledWith([ - { name: JobName.ASSET_DELETION, data: { id: assetStub.image.id } }, + { name: JobName.ASSET_DELETION, data: { id: assetStub.image.id, deleteOnDisk: true } }, ]); }); }); diff --git a/server/src/services/trash.service.ts b/server/src/services/trash.service.ts index f74ea80984..0c64332941 100644 --- a/server/src/services/trash.service.ts +++ b/server/src/services/trash.service.ts @@ -49,7 +49,13 @@ export class TrashService { for await (const assets of assetPagination) { await this.jobRepository.queueAll( - assets.map((asset) => ({ name: JobName.ASSET_DELETION, data: { id: asset.id } })), + assets.map((asset) => ({ + name: JobName.ASSET_DELETION, + data: { + id: asset.id, + deleteOnDisk: true, + }, + })), ); } }