fix(server): hide isFavorite from album asset sync stream (#28923)

* fix(server): hide isFavorite from album asset sync stream

* some tests

* Revert "some tests"

This reverts commit 3242e6961c.

* alter existing test to clear test's intent

* Reapply "some tests"

This reverts commit f1d4c47f5f.

* drop one

* sql

server/src/database.ts

@@ -392,6 +392,27 @@ export const columns = {
     'asset.height',
     'asset.isEdited',
   ],
+  syncAlbumAsset: [
+    'asset.id',
+    'asset.ownerId',
+    'asset.originalFileName',
+    'asset.thumbhash',
+    'asset.checksum',
+    'asset.fileCreatedAt',
+    'asset.fileModifiedAt',
+    'asset.createdAt',
+    'asset.localDateTime',
+    'asset.type',
+    'asset.deletedAt',
+    'asset.visibility',
+    'asset.duration',
+    'asset.livePhotoVideoId',
+    'asset.stackId',
+    'asset.libraryId',
+    'asset.width',
+    'asset.height',
+    'asset.isEdited',
+  ],
   syncPartnerAsset: [
     'asset.id',
     'asset.ownerId',

server/src/queries/sync.repository.sql

@@ -69,7 +69,6 @@ select
   "asset"."localDateTime",
   "asset"."type",
   "asset"."deletedAt",
-  "asset"."isFavorite",
   "asset"."visibility",
   "asset"."duration",
   "asset"."livePhotoVideoId",
@@ -78,15 +77,19 @@ select
   "asset"."width",
   "asset"."height",
   "asset"."isEdited",
+  case
+    when "asset"."ownerId" = $1 then "asset"."isFavorite"
+    else $2
+  end as "isFavorite",
   "album_asset"."updateId"
 from
   "album_asset" as "album_asset"
   inner join "asset" on "asset"."id" = "album_asset"."assetId"
 where
-  "album_asset"."updateId" < $1
-  and "album_asset"."updateId" <= $2
-  and "album_asset"."updateId" >= $3
-  and "album_asset"."albumId" = $4
+  "album_asset"."updateId" < $3
+  and "album_asset"."updateId" <= $4
+  and "album_asset"."updateId" >= $5
+  and "album_asset"."albumId" = $6
 order by
   "album_asset"."updateId" asc
 
@@ -103,7 +106,6 @@ select
   "asset"."localDateTime",
   "asset"."type",
   "asset"."deletedAt",
-  "asset"."isFavorite",
   "asset"."visibility",
   "asset"."duration",
   "asset"."livePhotoVideoId",
@@ -112,16 +114,20 @@ select
   "asset"."width",
   "asset"."height",
   "asset"."isEdited",
+  case
+    when "asset"."ownerId" = $1 then "asset"."isFavorite"
+    else $2
+  end as "isFavorite",
   "asset"."updateId"
 from
   "asset" as "asset"
   inner join "album_asset" on "album_asset"."assetId" = "asset"."id"
   inner join "album_user" on "album_user"."albumId" = "album_asset"."albumId"
 where
-  "asset"."updateId" < $1
-  and "asset"."updateId" > $2
-  and "album_asset"."updateId" <= $3
-  and "album_user"."userId" = $4
+  "asset"."updateId" < $3
+  and "asset"."updateId" > $4
+  and "album_asset"."updateId" <= $5
+  and "album_user"."userId" = $6
 order by
   "asset"."updateId" asc
 
@@ -139,7 +145,6 @@ select
   "asset"."localDateTime",
   "asset"."type",
   "asset"."deletedAt",
-  "asset"."isFavorite",
   "asset"."visibility",
   "asset"."duration",
   "asset"."livePhotoVideoId",
@@ -147,15 +152,19 @@ select
   "asset"."libraryId",
   "asset"."width",
   "asset"."height",
-  "asset"."isEdited"
+  "asset"."isEdited",
+  case
+    when "asset"."ownerId" = $1 then "asset"."isFavorite"
+    else $2
+  end as "isFavorite"
 from
   "album_asset" as "album_asset"
   inner join "asset" on "asset"."id" = "album_asset"."assetId"
   inner join "album_user" on "album_user"."albumId" = "album_asset"."albumId"
 where
-  "album_asset"."updateId" < $1
-  and "album_asset"."updateId" > $2
-  and "album_user"."userId" = $3
+  "album_asset"."updateId" < $3
+  and "album_asset"."updateId" > $4
+  and "album_user"."userId" = $5
 order by
   "album_asset"."updateId" asc
 

server/src/repositories/sync.repository.ts

@@ -195,11 +195,20 @@ class AlbumSync extends BaseSync {
 }
 
 class AlbumAssetSync extends BaseSync {
-  @GenerateSql({ params: [dummyBackfillOptions, DummyValue.UUID], stream: true })
-  getBackfill(options: SyncBackfillOptions, albumId: string) {
+  @GenerateSql({ params: [dummyBackfillOptions, DummyValue.UUID, DummyValue.UUID], stream: true })
+  getBackfill(options: SyncBackfillOptions, albumId: string, userId: string) {
     return this.backfillQuery('album_asset', options)
       .innerJoin('asset', 'asset.id', 'album_asset.assetId')
-      .select(columns.syncAsset)
+      .select(columns.syncAlbumAsset)
+      .select((eb) =>
+        eb
+          .case()
+          .when('asset.ownerId', '=', userId)
+          .then(eb.ref('asset.isFavorite'))
+          .else(eb.val(false))
+          .end()
+          .as('isFavorite'),
+      )
       .select('album_asset.updateId')
       .where('album_asset.albumId', '=', albumId)
       .stream();
@@ -210,7 +219,16 @@ class AlbumAssetSync extends BaseSync {
     const userId = options.userId;
     return this.upsertQuery('asset', options)
       .innerJoin('album_asset', 'album_asset.assetId', 'asset.id')
-      .select(columns.syncAsset)
+      .select(columns.syncAlbumAsset)
+      .select((eb) =>
+        eb
+          .case()
+          .when('asset.ownerId', '=', userId)
+          .then(eb.ref('asset.isFavorite'))
+          .else(eb.val(false))
+          .end()
+          .as('isFavorite'),
+      )
       .select('asset.updateId')
       .where('album_asset.updateId', '<=', albumToAssetAck.updateId) // Ensure we only send updates for assets that the client already knows about
       .innerJoin('album_user', 'album_user.albumId', 'album_asset.albumId')
@@ -224,7 +242,16 @@ class AlbumAssetSync extends BaseSync {
     return this.upsertQuery('album_asset', options)
       .select('album_asset.updateId')
       .innerJoin('asset', 'asset.id', 'album_asset.assetId')
-      .select(columns.syncAsset)
+      .select(columns.syncAlbumAsset)
+      .select((eb) =>
+        eb
+          .case()
+          .when('asset.ownerId', '=', userId)
+          .then(eb.ref('asset.isFavorite'))
+          .else(eb.val(false))
+          .end()
+          .as('isFavorite'),
+      )
       .innerJoin('album_user', 'album_user.albumId', 'album_asset.albumId')
       .where('album_user.userId', '=', userId)
       .stream();

server/src/services/sync.service.ts

@@ -545,6 +545,7 @@ export class SyncService extends BaseService {
         const backfill = this.syncRepository.albumAsset.getBackfill(
           { ...options, afterUpdateId: startId, beforeUpdateId: endId },
           album.id,
+          options.userId,
         );
 
         for await (const { updateId, ...data } of backfill) {

server/test/medium/specs/sync/sync-album-asset.spec.ts

@@ -270,7 +270,7 @@ describe(SyncRequestType.AlbumAssetsV2, () => {
   it('should sync asset updates for an album shared with you', async () => {
     const { auth, ctx } = await setup();
     const { user: user2 } = await ctx.newUser();
-    const { asset } = await ctx.newAsset({ ownerId: user2.id, isFavorite: false });
+    const { asset } = await ctx.newAsset({ ownerId: user2.id, originalFileName: 'before' });
     const { album } = await ctx.newAlbum({ ownerId: user2.id });
     await wait(2);
     await ctx.newAlbumAsset({ albumId: album.id, assetId: asset.id });
@@ -281,9 +281,7 @@ describe(SyncRequestType.AlbumAssetsV2, () => {
       updateSyncAck,
       {
         ack: expect.any(String),
-        data: expect.objectContaining({
-          id: asset.id,
-        }),
+        data: expect.objectContaining({ id: asset.id, originalFileName: 'before' }),
         type: SyncEntityType.AlbumAssetCreateV2,
       },
       expect.objectContaining({ type: SyncEntityType.SyncCompleteV1 }),
@@ -291,24 +289,56 @@ describe(SyncRequestType.AlbumAssetsV2, () => {
 
     await ctx.syncAckAll(auth, response);
 
-    // update the asset
     const assetRepository = ctx.get(AssetRepository);
-    await assetRepository.update({
-      id: asset.id,
-      isFavorite: true,
-    });
+    await assetRepository.update({ id: asset.id, originalFileName: 'after' });
 
     const updateResponse = await ctx.syncStream(auth, [SyncRequestType.AlbumAssetsV2]);
     expect(updateResponse).toEqual([
       {
         ack: expect.any(String),
-        data: expect.objectContaining({
-          id: asset.id,
-          isFavorite: true,
-        }),
+        data: expect.objectContaining({ id: asset.id, originalFileName: 'after' }),
         type: SyncEntityType.AlbumAssetUpdateV2,
       },
       expect.objectContaining({ type: SyncEntityType.SyncCompleteV1 }),
     ]);
   });
+
+  it('should hide isFavorite for album assets owned by another user', async () => {
+    const { auth, ctx } = await setup();
+    const { user: user2 } = await ctx.newUser();
+    const { asset } = await ctx.newAsset({ ownerId: user2.id, isFavorite: true });
+    const { album } = await ctx.newAlbum({ ownerId: user2.id });
+    await ctx.newAlbumAsset({ albumId: album.id, assetId: asset.id });
+    await ctx.newAlbumUser({ albumId: album.id, userId: auth.user.id, role: AlbumUserRole.Viewer });
+
+    const response = await ctx.syncStream(auth, [SyncRequestType.AlbumAssetsV2]);
+    expect(response).toEqual([
+      updateSyncAck,
+      {
+        ack: expect.any(String),
+        data: expect.objectContaining({ id: asset.id, isFavorite: false }),
+        type: SyncEntityType.AlbumAssetCreateV2,
+      },
+      expect.objectContaining({ type: SyncEntityType.SyncCompleteV1 }),
+    ]);
+  });
+
+  it('should sync isFavorite for album assets owned by the requesting user', async () => {
+    const { auth, ctx } = await setup();
+    const { user: user2 } = await ctx.newUser();
+    const { asset } = await ctx.newAsset({ ownerId: auth.user.id, isFavorite: true });
+    const { album } = await ctx.newAlbum({ ownerId: user2.id });
+    await ctx.newAlbumAsset({ albumId: album.id, assetId: asset.id });
+    await ctx.newAlbumUser({ albumId: album.id, userId: auth.user.id, role: AlbumUserRole.Viewer });
+
+    const response = await ctx.syncStream(auth, [SyncRequestType.AlbumAssetsV2]);
+    expect(response).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          data: expect.objectContaining({ id: asset.id, isFavorite: true }),
+          type: SyncEntityType.AlbumAssetCreateV2,
+        }),
+      ]),
+    );
+  });
 });