From bab5ad7ebd34cb792054c090068e571e8c92ef7e Mon Sep 17 00:00:00 2001 From: Jonathan Jogenfors Date: Thu, 29 Aug 2024 01:51:25 +0200 Subject: [PATCH] fix(server): ensure new exclusion patterns work (#12102) * add test for bug * find excluded paths when checking offline * fix filename * fix unit tests * bump picomatch * fix e2e paths * improve e2e * add unit tests * cleanup e2e * set correct asset count * fix e2e test * fix lint --- e2e/src/api/specs/library.e2e-spec.ts | 67 ++++++++++++++++----- server/package-lock.json | 3 +- server/package.json | 2 +- server/src/interfaces/job.interface.ts | 1 + server/src/services/library.service.spec.ts | 21 ++++++- server/src/services/library.service.ts | 11 +++- 6 files changed, 85 insertions(+), 20 deletions(-) diff --git a/e2e/src/api/specs/library.e2e-spec.ts b/e2e/src/api/specs/library.e2e-spec.ts index 013e1364ca..ec42cbe4fa 100644 --- a/e2e/src/api/specs/library.e2e-spec.ts +++ b/e2e/src/api/specs/library.e2e-spec.ts @@ -353,7 +353,7 @@ describe('/libraries', () => { expect(assets.count).toBe(2); - utils.createImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + utils.createImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); await scan(admin.accessToken, library.id); await utils.waitForWebsocketEvent({ event: 'assetUpload', total: 3 }); @@ -361,11 +361,11 @@ describe('/libraries', () => { const { assets: newAssets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); expect(newAssets.count).toBe(3); - utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); }); it('should offline a file missing from disk', async () => { - utils.createImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + utils.createImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp`], @@ -374,26 +374,28 @@ describe('/libraries', () => { await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); - utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); + const { assets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + expect(assets.count).toBe(3); + + utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); - const { assets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + const { assets: newAssets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + expect(newAssets.count).toBe(3); - expect(assets.items).toEqual( + expect(newAssets.items).toEqual( expect.arrayContaining([ expect.objectContaining({ isOffline: true, - originalFileName: 'assetB.png', + originalFileName: 'assetC.png', }), ]), ); }); it('should offline a file outside of import paths', async () => { - utils.createImageFile(`${testAssetDir}/temp/directoryA/assetB.png`); - utils.createImageFile(`${testAssetDir}/temp/directoryB/assetC.png`); const library = await utils.createLibrary(admin.accessToken, { ownerId: admin.userId, importPaths: [`${testAssetDirInternal}/temp`], @@ -416,16 +418,49 @@ describe('/libraries', () => { expect.arrayContaining([ expect.objectContaining({ isOffline: false, - originalFileName: 'assetB.png', + originalFileName: 'assetA.png', }), expect.objectContaining({ isOffline: true, - originalFileName: 'assetC.png', + originalFileName: 'assetB.png', }), ]), ); + }); - utils.removeImageFile(`${testAssetDir}/temp/directoryB/assetC.png`); + it('should offline a file covered by an exclusion pattern', async () => { + const library = await utils.createLibrary(admin.accessToken, { + ownerId: admin.userId, + importPaths: [`${testAssetDirInternal}/temp`], + }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + await request(app) + .put(`/libraries/${library.id}`) + .set('Authorization', `Bearer ${admin.accessToken}`) + .send({ exclusionPatterns: ['**/directoryB/**'] }); + + await scan(admin.accessToken, library.id); + await utils.waitForQueueFinish(admin.accessToken, 'library'); + + const { assets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + + expect(assets.count).toBe(2); + + expect(assets.items).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + isOffline: false, + originalFileName: 'assetA.png', + }), + expect.objectContaining({ + isOffline: true, + originalFileName: 'assetB.png', + }), + ]), + ); }); it('should not try to delete offline files', async () => { @@ -471,6 +506,8 @@ describe('/libraries', () => { await utils.waitForWebsocketEvent({ event: 'assetDelete', total: 1 }); expect(existsSync(`${testAssetDir}/temp/offline1/assetA.png`)).toBe(true); + + utils.removeImageFile(`${testAssetDir}/temp/offline1/assetA.png`); }); it('should scan new files', async () => { @@ -482,14 +519,14 @@ describe('/libraries', () => { await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); - utils.createImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); + utils.createImageFile(`${testAssetDir}/temp/directoryC/assetC.png`); await scan(admin.accessToken, library.id); await utils.waitForQueueFinish(admin.accessToken, 'library'); - utils.removeImageFile(`${testAssetDir}/temp/directoryA/assetC.png`); const { assets } = await utils.metadataSearch(admin.accessToken, { libraryId: library.id }); + expect(assets.count).toBe(3); expect(assets.items).toEqual( expect.arrayContaining([ expect.objectContaining({ @@ -497,6 +534,8 @@ describe('/libraries', () => { }), ]), ); + + utils.removeImageFile(`${testAssetDir}/temp/directoryC/assetC.png`); }); describe('with refreshModifiedFiles=true', () => { diff --git a/server/package-lock.json b/server/package-lock.json index 1ec4fe0fb0..e90256e29b 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -51,7 +51,7 @@ "nodemailer": "^6.9.13", "openid-client": "^5.4.3", "pg": "^8.11.3", - "picomatch": "^4.0.0", + "picomatch": "^4.0.2", "react": "^18.3.1", "react-email": "^3.0.0", "reflect-metadata": "^0.2.0", @@ -11403,6 +11403,7 @@ "version": "4.0.2", "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-4.0.2.tgz", "integrity": "sha512-M7BAV6Rlcy5u+m6oPhAPFgJTzAioX/6B0DxyvDlo9l8+T3nLKbrczg2WLUyzd45L8RqfUMyGPzekbMvX2Ldkwg==", + "license": "MIT", "engines": { "node": ">=12" }, diff --git a/server/package.json b/server/package.json index 9b42922278..42552f20b7 100644 --- a/server/package.json +++ b/server/package.json @@ -77,7 +77,7 @@ "nodemailer": "^6.9.13", "openid-client": "^5.4.3", "pg": "^8.11.3", - "picomatch": "^4.0.0", + "picomatch": "^4.0.2", "react": "^18.3.1", "react-email": "^3.0.0", "reflect-metadata": "^0.2.0", diff --git a/server/src/interfaces/job.interface.ts b/server/src/interfaces/job.interface.ts index fab959936f..b2ac5ec6f1 100644 --- a/server/src/interfaces/job.interface.ts +++ b/server/src/interfaces/job.interface.ts @@ -133,6 +133,7 @@ export interface ILibraryFileJob extends IEntityJob { export interface ILibraryOfflineJob extends IEntityJob { importPaths: string[]; + exclusionPatterns: string[]; } export interface ILibraryRefreshJob extends IEntityJob { diff --git a/server/src/services/library.service.spec.ts b/server/src/services/library.service.spec.ts index 9e260e98ef..2d4e1d5776 100644 --- a/server/src/services/library.service.spec.ts +++ b/server/src/services/library.service.spec.ts @@ -301,6 +301,7 @@ describe(LibraryService.name, () => { const mockAssetJob: ILibraryOfflineJob = { id: assetStub.external.id, importPaths: ['/'], + exclusionPatterns: [], }; assetMock.getById.mockResolvedValue(null); @@ -314,6 +315,7 @@ describe(LibraryService.name, () => { const mockAssetJob: ILibraryOfflineJob = { id: assetStub.external.id, importPaths: ['/'], + exclusionPatterns: [], }; assetMock.getById.mockResolvedValue(assetStub.offline); @@ -323,10 +325,25 @@ describe(LibraryService.name, () => { expect(assetMock.update).not.toHaveBeenCalled(); }); - it('should offline assets no longer on disk or matching exclusion pattern', async () => { + it('should offline assets no longer on disk', async () => { const mockAssetJob: ILibraryOfflineJob = { id: assetStub.external.id, importPaths: ['/'], + exclusionPatterns: [], + }; + + assetMock.getById.mockResolvedValue(assetStub.external); + + await expect(sut.handleOfflineCheck(mockAssetJob)).resolves.toBe(JobStatus.SUCCESS); + + expect(assetMock.update).toHaveBeenCalledWith({ id: assetStub.external.id, isOffline: true }); + }); + + it('should offline assets matching an exclusion pattern', async () => { + const mockAssetJob: ILibraryOfflineJob = { + id: assetStub.external.id, + importPaths: ['/'], + exclusionPatterns: ['**/user1/**'], }; assetMock.getById.mockResolvedValue(assetStub.external); @@ -340,6 +357,7 @@ describe(LibraryService.name, () => { const mockAssetJob: ILibraryOfflineJob = { id: assetStub.external.id, importPaths: ['/data/user2'], + exclusionPatterns: [], }; assetMock.getById.mockResolvedValue(assetStub.external); @@ -354,6 +372,7 @@ describe(LibraryService.name, () => { const mockAssetJob: ILibraryOfflineJob = { id: assetStub.external.id, importPaths: ['/'], + exclusionPatterns: [], }; assetMock.getById.mockResolvedValue(assetStub.external); diff --git a/server/src/services/library.service.ts b/server/src/services/library.service.ts index 9e31107027..c7f82eddea 100644 --- a/server/src/services/library.service.ts +++ b/server/src/services/library.service.ts @@ -556,11 +556,16 @@ export class LibraryService { return JobStatus.SUCCESS; } + const isExcluded = job.exclusionPatterns.some((pattern) => picomatch.isMatch(asset.originalPath, pattern)); + if (isExcluded) { + this.logger.debug(`Asset is covered by an exclusion pattern, marking offline: ${asset.originalPath}`); + await this.assetRepository.update({ id: asset.id, isOffline: true }); + return JobStatus.SUCCESS; + } + const fileExists = await this.storageRepository.checkFileExists(asset.originalPath, R_OK); if (!fileExists) { - this.logger.debug( - `Asset is no longer found on disk or is covered by exclusion pattern, marking offline: ${asset.originalPath}`, - ); + this.logger.debug(`Asset is no longer found on disk, marking offline: ${asset.originalPath}`); await this.assetRepository.update({ id: asset.id, isOffline: true }); return JobStatus.SUCCESS; }