fix(server): Live Photo migration bug when album is in template (#25329)

Co-authored-by: Nikhil Alapati <nikhilalapati@meta.com>

server/src/queries/asset.job.repository.sql

@@ -562,6 +562,7 @@ select
   "asset"."checksum",
   "asset"."originalPath",
   "asset"."isExternal",
+  "asset"."visibility",
   "asset"."originalFileName",
   "asset"."livePhotoVideoId",
   "asset"."fileCreatedAt",
@@ -593,6 +594,7 @@ from
 where
   "asset"."deletedAt" is null
   and "asset"."id" = $2
+  and "asset"."visibility" != $3
 
 -- AssetJobRepository.streamForStorageTemplateJob
 select
@@ -602,6 +604,7 @@ select
   "asset"."checksum",
   "asset"."originalPath",
   "asset"."isExternal",
+  "asset"."visibility",
   "asset"."originalFileName",
   "asset"."livePhotoVideoId",
   "asset"."fileCreatedAt",
@@ -632,6 +635,7 @@ from
   inner join "asset_exif" on "asset"."id" = "asset_exif"."assetId"
 where
   "asset"."deletedAt" is null
+  and "asset"."visibility" != $2
 
 -- AssetJobRepository.streamForDeletedJob
 select

server/src/repositories/asset-job.repository.ts

@@ -353,6 +353,7 @@ export class AssetJobRepository {
         'asset.checksum',
         'asset.originalPath',
         'asset.isExternal',
+        'asset.visibility',
         'asset.originalFileName',
         'asset.livePhotoVideoId',
         'asset.fileCreatedAt',
@@ -367,13 +368,16 @@ export class AssetJobRepository {
   }
 
   @GenerateSql({ params: [DummyValue.UUID] })
-  getForStorageTemplateJob(id: string) {
-    return this.storageTemplateAssetQuery().where('asset.id', '=', id).executeTakeFirst();
+  getForStorageTemplateJob(id: string, options?: { includeHidden?: boolean }) {
+    return this.storageTemplateAssetQuery()
+      .where('asset.id', '=', id)
+      .$if(!options?.includeHidden, (qb) => qb.where('asset.visibility', '!=', AssetVisibility.Hidden))
+      .executeTakeFirst();
   }
 
   @GenerateSql({ params: [], stream: true })
   streamForStorageTemplateJob() {
-    return this.storageTemplateAssetQuery().stream();
+    return this.storageTemplateAssetQuery().where('asset.visibility', '!=', AssetVisibility.Hidden).stream();
   }
 
   @GenerateSql({ params: [DummyValue.DATE], stream: true })

server/src/services/storage-template.service.spec.ts

@@ -9,6 +9,9 @@ import { userStub } from 'test/fixtures/user.stub';
 import { getForStorageTemplate } from 'test/mappers';
 import { makeStream, newTestService, ServiceMocks } from 'test/utils';
 
+const motionAsset = AssetFactory.from({ type: AssetType.Video }).exif().build();
+const stillAsset = AssetFactory.from({ livePhotoVideoId: motionAsset.id }).exif().build();
+
 describe(StorageTemplateService.name, () => {
   let sut: StorageTemplateService;
   let mocks: ServiceMocks;
@@ -153,6 +156,58 @@ describe(StorageTemplateService.name, () => {
       expect(mocks.asset.update).toHaveBeenCalledWith({ id: motionAsset.id, originalPath: newMotionPicturePath });
     });
 
+    it('should migrate live photo motion video alongside the still image using album in path', async () => {
+      const motionAsset = AssetFactory.from({
+        type: AssetType.Video,
+        fileCreatedAt: new Date('2022-06-19T23:41:36.910Z'),
+      })
+        .exif()
+        .build();
+      const stillAsset = AssetFactory.from({
+        livePhotoVideoId: motionAsset.id,
+        fileCreatedAt: new Date('2022-06-19T23:41:36.910Z'),
+      })
+        .exif()
+        .build();
+
+      const album = AlbumFactory.from().asset().build();
+      const config = structuredClone(defaults);
+      config.storageTemplate.template = '{{y}}/{{#if album}}{{album}}{{else}}other/{{MM}}{{/if}}/{{filename}}';
+      sut.onConfigInit({ newConfig: config });
+
+      mocks.user.get.mockResolvedValue(userStub.user1);
+
+      const newMotionPicturePath = `/data/library/${motionAsset.ownerId}/2022/${album.albumName}/${stillAsset.originalFileName.slice(0, -4)}.mp4`;
+      const newStillPicturePath = `/data/library/${stillAsset.ownerId}/2022/${album.albumName}/${stillAsset.originalFileName}`;
+
+      mocks.assetJob.getForStorageTemplateJob.mockResolvedValueOnce(getForStorageTemplate(stillAsset));
+      mocks.assetJob.getForStorageTemplateJob.mockResolvedValueOnce(getForStorageTemplate(motionAsset));
+      mocks.album.getByAssetId.mockResolvedValue([album]);
+
+      mocks.move.create.mockResolvedValueOnce({
+        id: '123',
+        entityId: stillAsset.id,
+        pathType: AssetPathType.Original,
+        oldPath: stillAsset.originalPath,
+        newPath: newStillPicturePath,
+      });
+
+      mocks.move.create.mockResolvedValueOnce({
+        id: '124',
+        entityId: motionAsset.id,
+        pathType: AssetPathType.Original,
+        oldPath: motionAsset.originalPath,
+        newPath: newMotionPicturePath,
+      });
+
+      await expect(sut.handleMigrationSingle({ id: stillAsset.id })).resolves.toBe(JobStatus.Success);
+
+      expect(mocks.storage.checkFileExists).toHaveBeenCalledTimes(2);
+      expect(mocks.album.getByAssetId).toHaveBeenCalledWith(stillAsset.ownerId, stillAsset.id);
+      expect(mocks.asset.update).toHaveBeenCalledWith({ id: stillAsset.id, originalPath: newStillPicturePath });
+      expect(mocks.asset.update).toHaveBeenCalledWith({ id: motionAsset.id, originalPath: newMotionPicturePath });
+    });
+
     it('should use handlebar if condition for album', async () => {
       const user = UserFactory.create();
       const asset = AssetFactory.from().owner(user).exif().build();
@@ -709,12 +764,18 @@ describe(StorageTemplateService.name, () => {
       })
         .exif()
         .build();
-      const newMotionPicturePath = `/data/library/${motionAsset.ownerId}/2022/2022-06-19/${stillAsset.originalFileName.slice(0, -4)}.mp4`;
-      const newStillPicturePath = `/data/library/${stillAsset.ownerId}/2022/2022-06-19/${stillAsset.originalFileName}`;
+      const album = AlbumFactory.from().asset().build();
+      const config = structuredClone(defaults);
+      config.storageTemplate.template = '{{y}}/{{#if album}}{{album}}{{else}}other/{{MM}}{{/if}}/{{filename}}';
+      sut.onConfigInit({ newConfig: config });
+
+      const newMotionPicturePath = `/data/library/${motionAsset.ownerId}/2022/${album.albumName}/${stillAsset.originalFileName.slice(0, -4)}.mp4`;
+      const newStillPicturePath = `/data/library/${stillAsset.ownerId}/2022/${album.albumName}/${stillAsset.originalFileName}`;
 
       mocks.assetJob.streamForStorageTemplateJob.mockReturnValue(makeStream([getForStorageTemplate(stillAsset)]));
       mocks.user.getList.mockResolvedValue([userStub.user1]);
       mocks.assetJob.getForStorageTemplateJob.mockResolvedValueOnce(getForStorageTemplate(motionAsset));
+      mocks.album.getByAssetId.mockResolvedValue([album]);
 
       mocks.move.create.mockResolvedValueOnce({
         id: '123',
@@ -735,11 +796,53 @@ describe(StorageTemplateService.name, () => {
       await sut.handleMigration();
 
       expect(mocks.assetJob.streamForStorageTemplateJob).toHaveBeenCalled();
-      expect(mocks.assetJob.getForStorageTemplateJob).toHaveBeenCalledWith(motionAsset.id);
       expect(mocks.storage.checkFileExists).toHaveBeenCalledTimes(2);
       expect(mocks.asset.update).toHaveBeenCalledWith({ id: stillAsset.id, originalPath: newStillPicturePath });
       expect(mocks.asset.update).toHaveBeenCalledWith({ id: motionAsset.id, originalPath: newMotionPicturePath });
     });
+
+    it('should use still photo album info when migrating live photo motion video', async () => {
+      const user = userStub.user1;
+      const album = AlbumFactory.from().asset().build();
+      const config = structuredClone(defaults);
+      config.storageTemplate.template = '{{y}}/{{#if album}}{{album}}{{else}}other{{/if}}/{{filename}}';
+
+      sut.onConfigInit({ newConfig: config });
+
+      mocks.assetJob.streamForStorageTemplateJob.mockReturnValue(makeStream([getForStorageTemplate(stillAsset)]));
+      mocks.user.getList.mockResolvedValue([user]);
+      mocks.assetJob.getForStorageTemplateJob.mockResolvedValueOnce(getForStorageTemplate(motionAsset));
+      mocks.album.getByAssetId.mockResolvedValue([album]);
+
+      mocks.move.create.mockResolvedValueOnce({
+        id: '123',
+        entityId: stillAsset.id,
+        pathType: AssetPathType.Original,
+        oldPath: stillAsset.originalPath,
+        newPath: `/data/library/${user.id}/2022/${album.albumName}/${stillAsset.originalFileName}`,
+      });
+
+      mocks.move.create.mockResolvedValueOnce({
+        id: '124',
+        entityId: motionAsset.id,
+        pathType: AssetPathType.Original,
+        oldPath: motionAsset.originalPath,
+        newPath: `/data/library/${user.id}/2022/${album.albumName}/${motionAsset.originalFileName}`,
+      });
+
+      await sut.handleMigration();
+
+      expect(mocks.album.getByAssetId).toHaveBeenCalledWith(stillAsset.ownerId, stillAsset.id);
+      expect(mocks.album.getByAssetId).toHaveBeenCalledTimes(2);
+      expect(mocks.asset.update).toHaveBeenCalledWith({
+        id: stillAsset.id,
+        originalPath: expect.stringContaining(`/${album.albumName}/`),
+      });
+      expect(mocks.asset.update).toHaveBeenCalledWith({
+        id: motionAsset.id,
+        originalPath: expect.stringContaining(`/${album.albumName}/`),
+      });
+    });
   });
 
   describe('file rename correctness', () => {

server/src/services/storage-template.service.ts

@@ -158,12 +158,14 @@ export class StorageTemplateService extends BaseService {
 
     // move motion part of live photo
     if (asset.livePhotoVideoId) {
-      const livePhotoVideo = await this.assetJobRepository.getForStorageTemplateJob(asset.livePhotoVideoId);
+      const livePhotoVideo = await this.assetJobRepository.getForStorageTemplateJob(asset.livePhotoVideoId, {
+        includeHidden: true,
+      });
       if (!livePhotoVideo) {
         return JobStatus.Failed;
       }
       const motionFilename = getLivePhotoMotionFilename(filename, livePhotoVideo.originalPath);
-      await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename });
+      await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename }, asset);
     }
     return JobStatus.Success;
   }
@@ -191,10 +193,12 @@ export class StorageTemplateService extends BaseService {
 
       // move motion part of live photo
       if (asset.livePhotoVideoId) {
-        const livePhotoVideo = await this.assetJobRepository.getForStorageTemplateJob(asset.livePhotoVideoId);
+        const livePhotoVideo = await this.assetJobRepository.getForStorageTemplateJob(asset.livePhotoVideoId, {
+          includeHidden: true,
+        });
         if (livePhotoVideo) {
           const motionFilename = getLivePhotoMotionFilename(filename, livePhotoVideo.originalPath);
-          await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename });
+          await this.moveAsset(livePhotoVideo, { storageLabel, filename: motionFilename }, asset);
         }
       }
     }
@@ -214,7 +218,7 @@ export class StorageTemplateService extends BaseService {
     await this.moveRepository.cleanMoveHistorySingle(assetId);
   }
 
-  async moveAsset(asset: StorageAsset, metadata: MoveAssetMetadata) {
+  async moveAsset(asset: StorageAsset, metadata: MoveAssetMetadata, stillPhoto?: StorageAsset) {
     if (asset.isExternal || StorageCore.isAndroidMotionPath(asset.originalPath)) {
       // External assets are not affected by storage template
       // TODO: shouldn't this only apply to external assets?
@@ -224,7 +228,7 @@ export class StorageTemplateService extends BaseService {
     return this.databaseRepository.withLock(DatabaseLock.StorageTemplateMigration, async () => {
       const { id, originalPath, checksum, fileSizeInByte } = asset;
       const oldPath = originalPath;
-      const newPath = await this.getTemplatePath(asset, metadata);
+      const newPath = await this.getTemplatePath(asset, metadata, stillPhoto);
 
       if (!fileSizeInByte) {
         this.logger.error(`Asset ${id} missing exif info, skipping storage template migration`);
@@ -255,7 +259,11 @@ export class StorageTemplateService extends BaseService {
     });
   }
 
-  private async getTemplatePath(asset: StorageAsset, metadata: MoveAssetMetadata): Promise<string> {
+  private async getTemplatePath(
+    asset: StorageAsset,
+    metadata: MoveAssetMetadata,
+    stillPhoto?: StorageAsset,
+  ): Promise<string> {
     const { storageLabel, filename } = metadata;
 
     try {
@@ -296,8 +304,12 @@ export class StorageTemplateService extends BaseService {
       let albumName = null;
       let albumStartDate = null;
       let albumEndDate = null;
+      const assetForMetadata = stillPhoto || asset;
+
       if (this.template.needsAlbum) {
-        const albums = await this.albumRepository.getByAssetId(asset.ownerId, asset.id);
+        // For motion videos, use the still photo's album information since motion videos
+        // don't have album metadata attached directly
+        const albums = await this.albumRepository.getByAssetId(assetForMetadata.ownerId, assetForMetadata.id);
         const album = albums?.[0];
         if (album) {
           albumName = album.albumName || null;
@@ -310,16 +322,18 @@ export class StorageTemplateService extends BaseService {
         }
       }
 
+      // For motion videos that are part of live photos, use the still photo's date
+      // to ensure both parts end up in the same folder
       const storagePath = this.render(this.template.compiled, {
-        asset,
+        asset: assetForMetadata,
         filename: sanitized,
         extension,
         albumName,
         albumStartDate,
         albumEndDate,
-        make: asset.make,
-        model: asset.model,
-        lensModel: asset.lensModel,
+        make: assetForMetadata.make,
+        model: assetForMetadata.model,
+        lensModel: assetForMetadata.lensModel,
       });
       const fullPath = path.normalize(path.join(rootPath, storagePath));
       let destination = `${fullPath}.${extension}`;

server/test/mappers.ts

@@ -12,6 +12,7 @@ export const getForStorageTemplate = (asset: ReturnType<AssetFactory['build']>)
     isExternal: asset.isExternal,
     checksum: asset.checksum,
     timeZone: asset.exifInfo.timeZone,
+    visibility: asset.visibility,
     fileCreatedAt: asset.fileCreatedAt,
     originalPath: asset.originalPath,
     originalFileName: asset.originalFileName,