feat(server): deterministic download order (#7658)

immich-app · Mar 5, 2024 · 972d5a3 · 972d5a3
1 parent 8df63b7
commit 972d5a3
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 3 deletions.
diff --git a/server/src/domain/download/download.service.spec.ts b/server/src/domain/download/download.service.spec.ts
@@ -90,7 +90,10 @@ describe(DownloadService.name, () => {
       };
 
       accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2']));
-      assetMock.getByIds.mockResolvedValue([assetStub.noResizePath, assetStub.noWebpPath]);
+      assetMock.getByIds.mockResolvedValue([
+        { ...assetStub.noResizePath, id: 'asset-1' },
+        { ...assetStub.noWebpPath, id: 'asset-2' },
+      ]);
       storageMock.createZipStream.mockReturnValue(archiveMock);
 
       await expect(sut.downloadArchive(authStub.admin, { assetIds: ['asset-1', 'asset-2'] })).resolves.toEqual({
@@ -110,7 +113,33 @@ describe(DownloadService.name, () => {
       };
 
       accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2']));
-      assetMock.getByIds.mockResolvedValue([assetStub.noResizePath, assetStub.noResizePath]);
+      assetMock.getByIds.mockResolvedValue([
+        { ...assetStub.noResizePath, id: 'asset-1' },
+        { ...assetStub.noResizePath, id: 'asset-2' },
+      ]);
+      storageMock.createZipStream.mockReturnValue(archiveMock);
+
+      await expect(sut.downloadArchive(authStub.admin, { assetIds: ['asset-1', 'asset-2'] })).resolves.toEqual({
+        stream: archiveMock.stream,
+      });
+
+      expect(archiveMock.addFile).toHaveBeenCalledTimes(2);
+      expect(archiveMock.addFile).toHaveBeenNthCalledWith(1, 'upload/library/IMG_123.jpg', 'IMG_123.jpg');
+      expect(archiveMock.addFile).toHaveBeenNthCalledWith(2, 'upload/library/IMG_123.jpg', 'IMG_123+1.jpg');
+    });
+
+    it('should be deterministic', async () => {
+      const archiveMock = {
+        addFile: jest.fn(),
+        finalize: jest.fn(),
+        stream: new Readable(),
+      };
+
+      accessMock.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1', 'asset-2']));
+      assetMock.getByIds.mockResolvedValue([
+        { ...assetStub.noResizePath, id: 'asset-2' },
+        { ...assetStub.noResizePath, id: 'asset-1' },
+      ]);
       storageMock.createZipStream.mockReturnValue(archiveMock);
 
       await expect(sut.downloadArchive(authStub.admin, { assetIds: ['asset-1', 'asset-2'] })).resolves.toEqual({

diff --git a/server/src/domain/download/download.service.ts b/server/src/domain/download/download.service.ts
@@ -81,9 +81,16 @@ export class DownloadService {
 
     const zip = this.storageRepository.createZipStream();
     const assets = await this.assetRepository.getByIds(dto.assetIds);
+    const assetMap = new Map(assets.map((asset) => [asset.id, asset]));
     const paths: Record<string, number> = {};
 
-    for (const { originalPath, originalFileName } of assets) {
+    for (const assetId of dto.assetIds) {
+      const asset = assetMap.get(assetId);
+      if (!asset) {
+        continue;
+      }
+
+      const { originalPath, originalFileName } = asset;
       const extension = extname(originalPath);
       let filename = `${originalFileName}${extension}`;
       const count = paths[filename] || 0;