Browse Source

fix(mobile): sync issue due to time mismatch

Fynn Petersen-Frey 1 year ago
parent
commit
4ce3676127

+ 6 - 0
cli/src/api/open-api/api.ts

@@ -959,6 +959,12 @@ export interface AuditDeletesResponseDto {
      * @memberof AuditDeletesResponseDto
      */
     'needsFullSync': boolean;
+    /**
+     * 
+     * @type {string}
+     * @memberof AuditDeletesResponseDto
+     */
+    'timeOfRequest'?: string;
 }
 /**
  * 

+ 12 - 7
mobile/lib/shared/services/asset.service.dart

@@ -48,23 +48,28 @@ class AssetService {
     return changes;
   }
 
-  /// Returns `(null, null)` if changes are invalid -> requires full sync
-  Future<(List<Asset>? toUpsert, List<String>? toDelete)>
+  /// Returns `(null, null, time)` if changes are invalid -> requires full sync
+  Future<(List<Asset>? toUpsert, List<String>? toDelete, DateTime? time)>
       _getRemoteAssetChanges(User user, DateTime since) async {
     final deleted = await _apiService.auditApi
         .getAuditDeletes(EntityType.ASSET, since, userId: user.id);
-    if (deleted == null || deleted.needsFullSync) return (null, null);
+    if (deleted == null || deleted.needsFullSync) {
+      return (null, null, deleted?.timeOfRequest);
+    }
     final assetDto = await _apiService.assetApi
         .getAllAssets(userId: user.id, updatedAfter: since);
-    if (assetDto == null) return (null, null);
-    return (assetDto.map(Asset.remote).toList(), deleted.ids);
+    if (assetDto == null) return (null, null, deleted.timeOfRequest);
+    return (
+      assetDto.map(Asset.remote).toList(),
+      deleted.ids,
+      deleted.timeOfRequest
+    );
   }
 
   /// Returns `null` if the server state did not change, else list of assets
-  Future<List<Asset>?> _getRemoteAssets(User user) async {
+  Future<List<Asset>?> _getRemoteAssets(User user, DateTime now) async {
     const int chunkSize = 5000;
     try {
-      final DateTime now = DateTime.now().toUtc();
       final List<Asset> allAssets = [];
       for (int i = 0;; i += chunkSize) {
         final List<AssetResponseDto>? assets =

+ 28 - 22
mobile/lib/shared/services/sync.service.dart

@@ -41,17 +41,20 @@ class SyncService {
   /// Returns `true` if there were any changes
   Future<bool> syncRemoteAssetsToDb(
     User user,
-    Future<(List<Asset>? toUpsert, List<String>? toDelete)> Function(
+    Future<(List<Asset>? toUpsert, List<String>? toDelete, DateTime? time)>
+        Function(
       User user,
       DateTime since,
     ) getChangedAssets,
-    FutureOr<List<Asset>?> Function(User user) loadAssets,
+    FutureOr<List<Asset>?> Function(User user, DateTime now) loadAssets,
   ) =>
-      _lock.run(
-        () async =>
-            await _syncRemoteAssetChanges(user, getChangedAssets) ??
-            await _syncRemoteAssetsFull(user, loadAssets),
-      );
+      _lock.run(() async {
+        final (changes, serverTime) =
+            await _syncRemoteAssetChanges(user, getChangedAssets);
+        if (changes != null) return changes;
+        final time = serverTime ?? DateTime.now().toUtc();
+        return await _syncRemoteAssetsFull(user, time, loadAssets);
+      });
 
   /// Syncs remote albums to the database
   /// returns `true` if there were any changes
@@ -146,19 +149,22 @@ class SyncService {
     return true;
   }
 
-  /// Efficiently syncs assets via changes. Returns `null` when a full sync is required.
-  Future<bool?> _syncRemoteAssetChanges(
+  /// Efficiently syncs assets via changes. Returns `(null, serverTime)` when a full sync is required.
+  Future<(bool?, DateTime? serverTime)> _syncRemoteAssetChanges(
     User user,
-    Future<(List<Asset>? toUpsert, List<String>? toDelete)> Function(
+    Future<(List<Asset>? toUpsert, List<String>? toDelete, DateTime? time)>
+        Function(
       User user,
       DateTime since,
     ) getChangedAssets,
   ) async {
     final DateTime? since = _db.eTags.getByIdSync(user.id)?.time?.toUtc();
-    if (since == null) return null;
-    final DateTime now = DateTime.now();
-    final (toUpsert, toDelete) = await getChangedAssets(user, since);
-    if (toUpsert == null || toDelete == null) return null;
+    final DateTime now = DateTime.now().toUtc();
+    final (toUpsert, toDelete, serverTime) =
+        await getChangedAssets(user, since ?? now);
+    if (since == null || toUpsert == null || toDelete == null) {
+      return (null, serverTime);
+    }
     try {
       if (toDelete.isNotEmpty) {
         await handleRemoteAssetRemoval(toDelete);
@@ -168,14 +174,14 @@ class SyncService {
         await upsertAssetsWithExif(updated);
       }
       if (toUpsert.isNotEmpty || toDelete.isNotEmpty) {
-        await _updateUserAssetsETag(user, now);
-        return true;
+        await _updateUserAssetsETag(user, serverTime ?? now);
+        return (true, serverTime);
       }
-      return false;
+      return (false, serverTime);
     } on IsarError catch (e) {
       _log.severe("Failed to sync remote assets to db: $e");
     }
-    return null;
+    return (null, serverTime);
   }
 
   /// Deletes remote-only assets, updates merged assets to be local-only
@@ -202,11 +208,11 @@ class SyncService {
 
   /// Syncs assets by loading and comparing all assets from the server.
   Future<bool> _syncRemoteAssetsFull(
-    User user,
-    FutureOr<List<Asset>?> Function(User user) loadAssets,
+    final User user,
+    final DateTime now,
+    final FutureOr<List<Asset>?> Function(User user, DateTime now) loadAssets,
   ) async {
-    final DateTime now = DateTime.now().toUtc();
-    final List<Asset>? remote = await loadAssets(user);
+    final List<Asset>? remote = await loadAssets(user, now);
     if (remote == null) {
       return false;
     }

+ 1 - 0
mobile/openapi/doc/AuditDeletesResponseDto.md

@@ -10,6 +10,7 @@ Name | Type | Description | Notes
 ------------ | ------------- | ------------- | -------------
 **ids** | **List<String>** |  | [default to const []]
 **needsFullSync** | **bool** |  | 
+**timeOfRequest** | [**DateTime**](DateTime.md) |  | [optional] 
 
 [[Back to Model list]](../README.md#documentation-for-models) [[Back to API list]](../README.md#documentation-for-api-endpoints) [[Back to README]](../README.md)
 

+ 20 - 3
mobile/openapi/lib/model/audit_deletes_response_dto.dart

@@ -15,30 +15,46 @@ class AuditDeletesResponseDto {
   AuditDeletesResponseDto({
     this.ids = const [],
     required this.needsFullSync,
+    this.timeOfRequest,
   });
 
   List<String> ids;
 
   bool needsFullSync;
 
+  ///
+  /// Please note: This property should have been non-nullable! Since the specification file
+  /// does not include a default value (using the "default:" property), however, the generated
+  /// source code must fall back to having a nullable type.
+  /// Consider adding a "default:" property in the specification file to hide this note.
+  ///
+  DateTime? timeOfRequest;
+
   @override
   bool operator ==(Object other) => identical(this, other) || other is AuditDeletesResponseDto &&
      other.ids == ids &&
-     other.needsFullSync == needsFullSync;
+     other.needsFullSync == needsFullSync &&
+     other.timeOfRequest == timeOfRequest;
 
   @override
   int get hashCode =>
     // ignore: unnecessary_parenthesis
     (ids.hashCode) +
-    (needsFullSync.hashCode);
+    (needsFullSync.hashCode) +
+    (timeOfRequest == null ? 0 : timeOfRequest!.hashCode);
 
   @override
-  String toString() => 'AuditDeletesResponseDto[ids=$ids, needsFullSync=$needsFullSync]';
+  String toString() => 'AuditDeletesResponseDto[ids=$ids, needsFullSync=$needsFullSync, timeOfRequest=$timeOfRequest]';
 
   Map<String, dynamic> toJson() {
     final json = <String, dynamic>{};
       json[r'ids'] = this.ids;
       json[r'needsFullSync'] = this.needsFullSync;
+    if (this.timeOfRequest != null) {
+      json[r'timeOfRequest'] = this.timeOfRequest!.toUtc().toIso8601String();
+    } else {
+    //  json[r'timeOfRequest'] = null;
+    }
     return json;
   }
 
@@ -54,6 +70,7 @@ class AuditDeletesResponseDto {
             ? (json[r'ids'] as List).cast<String>()
             : const [],
         needsFullSync: mapValueOfType<bool>(json, r'needsFullSync')!,
+        timeOfRequest: mapDateTime(json, r'timeOfRequest', ''),
       );
     }
     return null;

+ 5 - 0
mobile/openapi/test/audit_deletes_response_dto_test.dart

@@ -26,6 +26,11 @@ void main() {
       // TODO
     });
 
+    // DateTime timeOfRequest
+    test('to test the property `timeOfRequest`', () async {
+      // TODO
+    });
+
 
   });
 

+ 37 - 16
mobile/test/sync_service_test.dart

@@ -95,8 +95,11 @@ void main() {
         makeAsset(checksum: "c", remoteId: "1-1"),
       ];
       expect(db.assets.countSync(), 5);
-      final bool c1 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c1 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c1, false);
       expect(db.assets.countSync(), 5);
     });
@@ -112,8 +115,11 @@ void main() {
         makeAsset(checksum: "g", remoteId: "3-1"),
       ];
       expect(db.assets.countSync(), 5);
-      final bool c1 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c1 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c1, true);
       expect(db.assets.countSync(), 7);
     });
@@ -129,23 +135,35 @@ void main() {
         makeAsset(checksum: "j", remoteId: "2-1d"),
       ];
       expect(db.assets.countSync(), 5);
-      final bool c1 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c1 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c1, true);
       expect(db.assets.countSync(), 8);
-      final bool c2 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c2 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c2, false);
       expect(db.assets.countSync(), 8);
       remoteAssets.removeAt(4);
-      final bool c3 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c3 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c3, true);
       expect(db.assets.countSync(), 7);
       remoteAssets.add(makeAsset(checksum: "k", remoteId: "2-1e"));
       remoteAssets.add(makeAsset(checksum: "l", remoteId: "2-2"));
-      final bool c4 =
-          await s.syncRemoteAssetsToDb(owner, _failDiff, (u) => remoteAssets);
+      final bool c4 = await s.syncRemoteAssetsToDb(
+        owner,
+        _failDiff,
+        (u, n) => remoteAssets,
+      );
       expect(c4, true);
       expect(db.assets.countSync(), 9);
     });
@@ -161,8 +179,8 @@ void main() {
       final List<String> toDelete = ["2-1", "1-1"];
       final bool c = await s.syncRemoteAssetsToDb(
         owner,
-        (user, since) async => (toUpsert, toDelete),
-        (user) => throw Exception(),
+        (user, since) async => (toUpsert, toDelete, null),
+        (user, n) => throw Exception(),
       );
       expect(c, true);
       expect(db.assets.countSync(), 6);
@@ -170,7 +188,10 @@ void main() {
   });
 }
 
-Future<(List<Asset>?, List<String>?)> _failDiff(User user, DateTime time) =>
-    Future.value((null, null));
+Future<(List<Asset>?, List<String>?, DateTime?)> _failDiff(
+  User user,
+  DateTime time,
+) =>
+    Future.value((null, null, null));
 
 class MockHashService extends Mock implements HashService {}

+ 4 - 0
server/immich-openapi-specs.json

@@ -6826,6 +6826,10 @@
           },
           "needsFullSync": {
             "type": "boolean"
+          },
+          "timeOfRequest": {
+            "format": "date-time",
+            "type": "string"
           }
         },
         "required": [

+ 1 - 0
server/src/domain/audit/audit.dto.ts

@@ -30,6 +30,7 @@ export enum PathEntityType {
 export class AuditDeletesResponseDto {
   needsFullSync!: boolean;
   ids!: string[];
+  timeOfRequest?: Date;
 }
 
 export class FileReportDto {

+ 3 - 1
server/src/domain/audit/audit.service.spec.ts

@@ -1,8 +1,8 @@
 import { DatabaseAction, EntityType } from '@app/infra/entities';
 import {
-  IAccessRepositoryMock,
   auditStub,
   authStub,
+  IAccessRepositoryMock,
   newAccessRepositoryMock,
   newAssetRepositoryMock,
   newAuditRepositoryMock,
@@ -61,6 +61,7 @@ describe(AuditService.name, () => {
       await expect(sut.getDeletes(authStub.admin, { after: date, entityType: EntityType.ASSET })).resolves.toEqual({
         needsFullSync: true,
         ids: [],
+        timeOfRequest: expect.any(Date),
       });
 
       expect(auditMock.getAfter).toHaveBeenCalledWith(date, {
@@ -77,6 +78,7 @@ describe(AuditService.name, () => {
       await expect(sut.getDeletes(authStub.admin, { after: date, entityType: EntityType.ASSET })).resolves.toEqual({
         needsFullSync: false,
         ids: ['asset-deleted'],
+        timeOfRequest: expect.any(Date),
       });
 
       expect(auditMock.getAfter).toHaveBeenCalledWith(date, {

+ 5 - 4
server/src/domain/audit/audit.service.ts

@@ -4,7 +4,7 @@ import { DateTime } from 'luxon';
 import { resolve } from 'node:path';
 import { AccessCore, Permission } from '../access';
 import { AuthUserDto } from '../auth';
-import { AUDIT_LOG_MAX_DURATION } from '../domain.constant';
+import { AUDIT_LOG_CLEANUP_DURATION, AUDIT_LOG_MAX_DURATION } from '../domain.constant';
 import { usePagination } from '../domain.util';
 import { JOBS_ASSET_PAGINATION_SIZE } from '../job';
 import {
@@ -44,7 +44,7 @@ export class AuditService {
   }
 
   async handleCleanup(): Promise<boolean> {
-    await this.repository.removeBefore(DateTime.now().minus(AUDIT_LOG_MAX_DURATION).toJSDate());
+    await this.repository.removeBefore(DateTime.now().minus(AUDIT_LOG_CLEANUP_DURATION).toJSDate());
     return true;
   }
 
@@ -52,15 +52,16 @@ export class AuditService {
     const userId = dto.userId || authUser.id;
     await this.access.requirePermission(authUser, Permission.TIMELINE_READ, userId);
 
+    const now = DateTime.utc();
+    const duration = now.diff(DateTime.fromJSDate(dto.after));
     const audits = await this.repository.getAfter(dto.after, {
       ownerId: userId,
       entityType: dto.entityType,
       action: DatabaseAction.DELETE,
     });
 
-    const duration = DateTime.now().diff(DateTime.fromJSDate(dto.after));
-
     return {
+      timeOfRequest: now.toJSDate(),
       needsFullSync: duration > AUDIT_LOG_MAX_DURATION,
       ids: audits.map(({ entityId }) => entityId),
     };

+ 3 - 6
server/src/domain/domain.constant.ts

@@ -4,6 +4,7 @@ import { extname } from 'node:path';
 import pkg from 'src/../../package.json';
 
 export const AUDIT_LOG_MAX_DURATION = Duration.fromObject({ days: 100 });
+export const AUDIT_LOG_CLEANUP_DURATION = Duration.fromObject({ days: 101 });
 export const ONE_HOUR = Duration.fromObject({ hours: 1 });
 
 export interface IServerVersion {
@@ -13,11 +14,7 @@ export interface IServerVersion {
 }
 
 export class ServerVersion implements IServerVersion {
-  constructor(
-    public readonly major: number,
-    public readonly minor: number,
-    public readonly patch: number,
-  ) {}
+  constructor(public readonly major: number, public readonly minor: number, public readonly patch: number) {}
 
   toString() {
     return `${this.major}.${this.minor}.${this.patch}`;
@@ -131,7 +128,7 @@ const sidecar: Record<string, string[]> = {
 const isType = (filename: string, r: Record<string, string[]>) => extname(filename).toLowerCase() in r;
 
 const lookup = (filename: string) =>
-  ({ ...image, ...video, ...sidecar })[extname(filename).toLowerCase()]?.[0] ?? 'application/octet-stream';
+  ({ ...image, ...video, ...sidecar }[extname(filename).toLowerCase()]?.[0] ?? 'application/octet-stream');
 
 export const mimeTypes = {
   image,

+ 6 - 0
web/src/api/open-api/api.ts

@@ -959,6 +959,12 @@ export interface AuditDeletesResponseDto {
      * @memberof AuditDeletesResponseDto
      */
     'needsFullSync': boolean;
+    /**
+     * 
+     * @type {string}
+     * @memberof AuditDeletesResponseDto
+     */
+    'timeOfRequest'?: string;
 }
 /**
  *