fix: don't delete offline files from disk when emptying trash

2024-12-25 10:43:13 +02:00 · 2024-12-18 13:52:26 +01:00 · 2024-12-18 13:52:26 +01:00 · e8c5fcf64d
commit e8c5fcf64d
parent e0fc8734bf
6 changed files with 299 additions and 23 deletions
--- 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() },
--- 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', () => {
--- 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');
--- 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) =>
--- 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);
    }

--- 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<TrashResponseDto> {
    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;
  }