Skip to content

Commit

Permalink
fix(server): ensure new exclusion patterns work (immich-app#12102)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@etnoy
etnoy authored Aug 28, 2024
1 parent c6c7c54 commit bab5ad7
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 20 deletions.
67 changes: 53 additions & 14 deletions e2e/src/api/specs/library.e2e-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,19 +353,19 @@ 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 });

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`],
Expand All @@ -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`],
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand All @@ -482,21 +519,23 @@ 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({
originalFileName: 'assetC.png',
}),
]),
);

utils.removeImageFile(`${testAssetDir}/temp/directoryC/assetC.png`);
});

describe('with refreshModifiedFiles=true', () => {
Expand Down
3 changes: 2 additions & 1 deletion server/package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion server/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions server/src/interfaces/job.interface.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export interface ILibraryFileJob extends IEntityJob {

export interface ILibraryOfflineJob extends IEntityJob {
importPaths: string[];
exclusionPatterns: string[];
}

export interface ILibraryRefreshJob extends IEntityJob {
Expand Down
21 changes: 20 additions & 1 deletion server/src/services/library.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ describe(LibraryService.name, () => {
const mockAssetJob: ILibraryOfflineJob = {
id: assetStub.external.id,
importPaths: ['/'],
exclusionPatterns: [],
};

assetMock.getById.mockResolvedValue(null);
Expand All @@ -314,6 +315,7 @@ describe(LibraryService.name, () => {
const mockAssetJob: ILibraryOfflineJob = {
id: assetStub.external.id,
importPaths: ['/'],
exclusionPatterns: [],
};

assetMock.getById.mockResolvedValue(assetStub.offline);
Expand All @@ -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);
Expand All @@ -340,6 +357,7 @@ describe(LibraryService.name, () => {
const mockAssetJob: ILibraryOfflineJob = {
id: assetStub.external.id,
importPaths: ['/data/user2'],
exclusionPatterns: [],
};

assetMock.getById.mockResolvedValue(assetStub.external);
Expand All @@ -354,6 +372,7 @@ describe(LibraryService.name, () => {
const mockAssetJob: ILibraryOfflineJob = {
id: assetStub.external.id,
importPaths: ['/'],
exclusionPatterns: [],
};

assetMock.getById.mockResolvedValue(assetStub.external);
Expand Down
11 changes: 8 additions & 3 deletions server/src/services/library.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit bab5ad7

Please sign in to comment.