From 4ce36761278d5b22b6305da860cd59f1c619243b Mon Sep 17 00:00:00 2001 From: Fynn Petersen-Frey Date: Thu, 26 Oct 2023 18:05:30 +0200 Subject: [PATCH] fix(mobile): sync issue due to time mismatch --- cli/src/api/open-api/api.ts | 6 +++ mobile/lib/shared/services/asset.service.dart | 19 ++++--- mobile/lib/shared/services/sync.service.dart | 50 +++++++++-------- mobile/openapi/doc/AuditDeletesResponseDto.md | 1 + .../lib/model/audit_deletes_response_dto.dart | 23 ++++++-- .../test/audit_deletes_response_dto_test.dart | 5 ++ mobile/test/sync_service_test.dart | 53 +++++++++++++------ server/immich-openapi-specs.json | 4 ++ server/src/domain/audit/audit.dto.ts | 1 + server/src/domain/audit/audit.service.spec.ts | 4 +- server/src/domain/audit/audit.service.ts | 9 ++-- server/src/domain/domain.constant.ts | 9 ++-- web/src/api/open-api/api.ts | 6 +++ 13 files changed, 131 insertions(+), 59 deletions(-) diff --git a/cli/src/api/open-api/api.ts b/cli/src/api/open-api/api.ts index 1f6669794..1b7f6d61e 100644 --- a/cli/src/api/open-api/api.ts +++ b/cli/src/api/open-api/api.ts @@ -959,6 +959,12 @@ export interface AuditDeletesResponseDto { * @memberof AuditDeletesResponseDto */ 'needsFullSync': boolean; + /** + * + * @type {string} + * @memberof AuditDeletesResponseDto + */ + 'timeOfRequest'?: string; } /** * diff --git a/mobile/lib/shared/services/asset.service.dart b/mobile/lib/shared/services/asset.service.dart index 8b1ee6a33..418c482d3 100644 --- a/mobile/lib/shared/services/asset.service.dart +++ b/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? toUpsert, List? toDelete)> + /// Returns `(null, null, time)` if changes are invalid -> requires full sync + Future<(List? toUpsert, List? 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?> _getRemoteAssets(User user) async { + Future?> _getRemoteAssets(User user, DateTime now) async { const int chunkSize = 5000; try { - final DateTime now = DateTime.now().toUtc(); final List allAssets = []; for (int i = 0;; i += chunkSize) { final List? assets = diff --git a/mobile/lib/shared/services/sync.service.dart b/mobile/lib/shared/services/sync.service.dart index 08025ed71..59b0f31a1 100644 --- a/mobile/lib/shared/services/sync.service.dart +++ b/mobile/lib/shared/services/sync.service.dart @@ -41,17 +41,20 @@ class SyncService { /// Returns `true` if there were any changes Future syncRemoteAssetsToDb( User user, - Future<(List? toUpsert, List? toDelete)> Function( + Future<(List? toUpsert, List? toDelete, DateTime? time)> + Function( User user, DateTime since, ) getChangedAssets, - FutureOr?> Function(User user) loadAssets, + FutureOr?> 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 _syncRemoteAssetChanges( + /// Efficiently syncs assets via changes. Returns `(null, serverTime)` when a full sync is required. + Future<(bool?, DateTime? serverTime)> _syncRemoteAssetChanges( User user, - Future<(List? toUpsert, List? toDelete)> Function( + Future<(List? toUpsert, List? 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 _syncRemoteAssetsFull( - User user, - FutureOr?> Function(User user) loadAssets, + final User user, + final DateTime now, + final FutureOr?> Function(User user, DateTime now) loadAssets, ) async { - final DateTime now = DateTime.now().toUtc(); - final List? remote = await loadAssets(user); + final List? remote = await loadAssets(user, now); if (remote == null) { return false; } diff --git a/mobile/openapi/doc/AuditDeletesResponseDto.md b/mobile/openapi/doc/AuditDeletesResponseDto.md index c7c9594f1..5c16fa8ba 100644 --- a/mobile/openapi/doc/AuditDeletesResponseDto.md +++ b/mobile/openapi/doc/AuditDeletesResponseDto.md @@ -10,6 +10,7 @@ Name | Type | Description | Notes ------------ | ------------- | ------------- | ------------- **ids** | **List** | | [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) diff --git a/mobile/openapi/lib/model/audit_deletes_response_dto.dart b/mobile/openapi/lib/model/audit_deletes_response_dto.dart index a0bc0dd4d..be0170955 100644 --- a/mobile/openapi/lib/model/audit_deletes_response_dto.dart +++ b/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 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 toJson() { final json = {}; 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() : const [], needsFullSync: mapValueOfType(json, r'needsFullSync')!, + timeOfRequest: mapDateTime(json, r'timeOfRequest', ''), ); } return null; diff --git a/mobile/openapi/test/audit_deletes_response_dto_test.dart b/mobile/openapi/test/audit_deletes_response_dto_test.dart index 45dbccc28..37a58efd0 100644 --- a/mobile/openapi/test/audit_deletes_response_dto_test.dart +++ b/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 + }); + }); diff --git a/mobile/test/sync_service_test.dart b/mobile/test/sync_service_test.dart index 9abb86352..2e148d609 100644 --- a/mobile/test/sync_service_test.dart +++ b/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 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?, List?)> _failDiff(User user, DateTime time) => - Future.value((null, null)); +Future<(List?, List?, DateTime?)> _failDiff( + User user, + DateTime time, +) => + Future.value((null, null, null)); class MockHashService extends Mock implements HashService {} diff --git a/server/immich-openapi-specs.json b/server/immich-openapi-specs.json index e3ed6402a..1d87a1bf3 100644 --- a/server/immich-openapi-specs.json +++ b/server/immich-openapi-specs.json @@ -6826,6 +6826,10 @@ }, "needsFullSync": { "type": "boolean" + }, + "timeOfRequest": { + "format": "date-time", + "type": "string" } }, "required": [ diff --git a/server/src/domain/audit/audit.dto.ts b/server/src/domain/audit/audit.dto.ts index d941f9a1d..a6b6dec05 100644 --- a/server/src/domain/audit/audit.dto.ts +++ b/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 { diff --git a/server/src/domain/audit/audit.service.spec.ts b/server/src/domain/audit/audit.service.spec.ts index 5e68250fa..6baf33059 100644 --- a/server/src/domain/audit/audit.service.spec.ts +++ b/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, { diff --git a/server/src/domain/audit/audit.service.ts b/server/src/domain/audit/audit.service.ts index 2b5a304ea..2d65ef413 100644 --- a/server/src/domain/audit/audit.service.ts +++ b/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 { - 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), }; diff --git a/server/src/domain/domain.constant.ts b/server/src/domain/domain.constant.ts index a4c17ff2d..027bb4ad2 100644 --- a/server/src/domain/domain.constant.ts +++ b/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 = { const isType = (filename: string, r: Record) => 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, diff --git a/web/src/api/open-api/api.ts b/web/src/api/open-api/api.ts index 1f6669794..1b7f6d61e 100644 --- a/web/src/api/open-api/api.ts +++ b/web/src/api/open-api/api.ts @@ -959,6 +959,12 @@ export interface AuditDeletesResponseDto { * @memberof AuditDeletesResponseDto */ 'needsFullSync': boolean; + /** + * + * @type {string} + * @memberof AuditDeletesResponseDto + */ + 'timeOfRequest'?: string; } /** *