From c9e6bba64a5637281c6f4613d514684133342c8b Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 18 Jul 2025 19:26:11 +0200 Subject: [PATCH] refactor(sync): remove rev param for download method, since we don't handle 304 and don't want manual caching --- src/app/pfapi/api/sync/meta-sync.service.ts | 8 +- .../pfapi/api/sync/model-sync.service.spec.ts | 13 +- src/app/pfapi/api/sync/model-sync.service.ts | 1 - .../api/sync/providers/dropbox/dropbox-api.ts | 4 - .../api/sync/providers/dropbox/dropbox.ts | 8 +- .../local-file-sync/local-file-sync-base.ts | 10 +- .../sync/providers/webdav/webdav-api.spec.ts | 178 +----------------- .../api/sync/providers/webdav/webdav-api.ts | 54 +----- .../pfapi/api/sync/providers/webdav/webdav.ts | 18 +- .../pfapi/api/sync/sync-provider.interface.ts | 6 +- src/app/pfapi/api/sync/sync.service.ts | 4 +- 11 files changed, 24 insertions(+), 280 deletions(-) diff --git a/src/app/pfapi/api/sync/meta-sync.service.ts b/src/app/pfapi/api/sync/meta-sync.service.ts index fe074b877..e95f34035 100644 --- a/src/app/pfapi/api/sync/meta-sync.service.ts +++ b/src/app/pfapi/api/sync/meta-sync.service.ts @@ -46,23 +46,19 @@ export class MetaSyncService { /** * Download metadata from remote storage - * @param localRev Optional local revision for conditional download * @returns Promise with the remote metadata and its revision * @throws NoRemoteMetaFile if the remote file doesn't exist * @throws LockPresentError if a lock is present from another client * @throws LockFromLocalClientPresentError if a lock is present from this client */ - async download( - localRev: string | null = null, - ): Promise<{ remoteMeta: RemoteMeta; remoteMetaRev: string }> { + async download(): Promise<{ remoteMeta: RemoteMeta; remoteMetaRev: string }> { // return {} as any as MetaFileContent; - PFLog.normal(`${MetaSyncService.L}.${this.download.name}()`, { localRev }); + PFLog.normal(`${MetaSyncService.L}.${this.download.name}()`); const syncProvider = this._currentSyncProvider$.getOrError(); try { const r = await syncProvider.downloadFile( MetaModelCtrl.META_MODEL_REMOTE_FILE_NAME, - localRev, ); // Check if file is locked diff --git a/src/app/pfapi/api/sync/model-sync.service.spec.ts b/src/app/pfapi/api/sync/model-sync.service.spec.ts index 0b16e9ea9..0db97670c 100644 --- a/src/app/pfapi/api/sync/model-sync.service.spec.ts +++ b/src/app/pfapi/api/sync/model-sync.service.spec.ts @@ -175,10 +175,7 @@ describe('ModelSyncService', () => { data: 'remote-model-data', }); expect(result.rev).toBe('rev-123'); - expect(mockSyncProvider.downloadFile).toHaveBeenCalledWith( - 'singleModel', - 'rev-123', - ); + expect(mockSyncProvider.downloadFile).toHaveBeenCalledWith('singleModel'); }); it('should handle NoRemoteModelFile error', async () => { @@ -191,10 +188,10 @@ describe('ModelSyncService', () => { it('should handle RevMismatchForModelError', async () => { mockSyncProvider.downloadFile.and.returnValue( - Promise.resolve(JSON.stringify({ data: 'remote-model-data' })), - ); - mockSyncProvider.downloadFile.and.throwError( - new RevMismatchForModelError('singleModel'), + Promise.resolve({ + rev: 'different-rev', + dataStr: JSON.stringify({ data: 'remote-model-data' }), + }), ); await expectAsync(service.download('singleModel', 'rev-123')).toBeRejectedWithError( diff --git a/src/app/pfapi/api/sync/model-sync.service.ts b/src/app/pfapi/api/sync/model-sync.service.ts index 88b938d3f..1df8e2f58 100644 --- a/src/app/pfapi/api/sync/model-sync.service.ts +++ b/src/app/pfapi/api/sync/model-sync.service.ts @@ -102,7 +102,6 @@ export class ModelSyncService { const syncProvider = this._currentSyncProvider$.getOrError(); const { rev, dataStr } = await syncProvider.downloadFile( this._filePathForModelId(modelId), - expectedRev, ); if (expectedRev) { if (!rev || !this._isSameRev(rev, expectedRev)) { diff --git a/src/app/pfapi/api/sync/providers/dropbox/dropbox-api.ts b/src/app/pfapi/api/sync/providers/dropbox/dropbox-api.ts index 434eeb394..ce813c3fb 100644 --- a/src/app/pfapi/api/sync/providers/dropbox/dropbox-api.ts +++ b/src/app/pfapi/api/sync/providers/dropbox/dropbox-api.ts @@ -83,10 +83,8 @@ export class DropboxApi { */ async download({ path, - localRev, }: { path: string; - localRev?: string | null; }): Promise<{ meta: DropboxFileMetadata; data: T }> { try { const response = await this._request({ @@ -96,8 +94,6 @@ export class DropboxApi { 'Dropbox-API-Arg': JSON.stringify({ path }), 'Content-Type': 'application/octet-stream', // Don't send If-None-Match - always download full content - // localRev parameter kept for API consistency but not used - // ...(localRev ? { 'If-None-Match': localRev } : {}), }, }); diff --git a/src/app/pfapi/api/sync/providers/dropbox/dropbox.ts b/src/app/pfapi/api/sync/providers/dropbox/dropbox.ts index 2e2dbc32e..a090816f2 100644 --- a/src/app/pfapi/api/sync/providers/dropbox/dropbox.ts +++ b/src/app/pfapi/api/sync/providers/dropbox/dropbox.ts @@ -112,14 +112,10 @@ export class Dropbox implements SyncProviderServiceInterface { + async downloadFile(targetPath: string): Promise<{ rev: string; dataStr: string }> { try { const r = await this._api.download({ path: this._getPath(targetPath), - localRev, }); if (!r.meta.rev) { @@ -143,7 +139,7 @@ export class Dropbox implements SyncProviderServiceInterface { + async downloadFile(targetPath: string): Promise<{ rev: string; dataStr: string }> { PFLog.normal(`${LocalFileSyncBase.LB}.${this.downloadFile.name}()`, { targetPath, - localRev, }); try { @@ -109,7 +105,7 @@ export abstract class LocalFileSyncBase // Check if file exists and compare revs if not force overwrite if (!isForceOverwrite && revToMatch) { try { - const existingFile = await this.downloadFile(targetPath, revToMatch); + const existingFile = await this.downloadFile(targetPath); if (existingFile.rev !== revToMatch) { PFLog.critical( `${LocalFileSyncBase.LB}.${this.uploadFile.name}() rev mismatch`, diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav-api.spec.ts b/src/app/pfapi/api/sync/providers/webdav/webdav-api.spec.ts index f90cc5fe4..72c2fdfe2 100644 --- a/src/app/pfapi/api/sync/providers/webdav/webdav-api.spec.ts +++ b/src/app/pfapi/api/sync/providers/webdav/webdav-api.spec.ts @@ -149,7 +149,6 @@ describe('WebdavApi', () => { const result = await api.download({ path: '/test.txt', - localRev: null, }); expect(mockHttpAdapter.request).toHaveBeenCalledWith( @@ -189,7 +188,6 @@ describe('WebdavApi', () => { const result = await api.download({ path: '/test.txt', - localRev: null, }); expect(result.rev).toBe('cleanedAbc123'); @@ -208,180 +206,18 @@ describe('WebdavApi', () => { const result = await api.download({ path: '/test.txt', - localRev: null, }); expect(result.rev).toBe('Wed, 15 Jan 2025 10:00:00 GMT'); }); - it('should add If-None-Match header when localRev is an ETag', async () => { - const mockResponse = { - status: 200, - headers: { - etag: '"newrev123"', - }, - data: 'content', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - mockXmlParser.validateResponseContent.and.stub(); - - await api.download({ - path: '/test.txt', - localRev: 'abc123', - }); - - expect(mockHttpAdapter.request).toHaveBeenCalledWith( - jasmine.objectContaining({ - headers: jasmine.objectContaining({ - 'If-None-Match': 'abc123', - }), - }), - ); - }); - - it('should add If-Modified-Since header when localRev is a timestamp', async () => { - const mockResponse = { - status: 200, - headers: { - etag: '"newrev123"', - }, - data: 'content', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - mockXmlParser.validateResponseContent.and.stub(); - - const timestamp = '1642248000000'; // 2022-01-15T12:00:00.000Z - - await api.download({ - path: '/test.txt', - localRev: timestamp, - }); - - expect(mockHttpAdapter.request).toHaveBeenCalledWith( - jasmine.objectContaining({ - headers: jasmine.objectContaining({ - 'If-Modified-Since': 'Sat, 15 Jan 2022 12:00:00 GMT', - }), - }), - ); - }); - - it('should add If-Modified-Since header when localRev is already a date string', async () => { - const mockResponse = { - status: 200, - headers: { - etag: '"newrev123"', - }, - data: 'content', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - mockXmlParser.validateResponseContent.and.stub(); - - const dateString = 'Wed, 15 Jan 2025 10:00:00 GMT'; - - await api.download({ - path: '/test.txt', - localRev: dateString, - }); - - expect(mockHttpAdapter.request).toHaveBeenCalledWith( - jasmine.objectContaining({ - headers: jasmine.objectContaining({ - 'If-Modified-Since': dateString, - }), - }), - ); - }); - - it('should handle 304 Not Modified response', async () => { - const mockResponse = { - status: 304, - headers: {}, - data: '', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - - const result = await api.download({ - path: '/test.txt', - localRev: 'abc123', - }); - - expect(result.notModified).toBe(true); - expect(result.rev).toBe('abc123'); - expect(result.dataStr).toBe(''); - }); - - it('should handle 304 response without localRev', async () => { - const mockResponse = { - status: 304, - headers: {}, - data: '', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - - const result = await api.download({ - path: '/test.txt', - localRev: null, - }); - - expect(result.notModified).toBe(true); - expect(result.rev).toBe(''); - expect(result.dataStr).toBe(''); - }); - - it('should handle invalid timestamp in localRev', async () => { - const mockResponse = { - status: 200, - headers: { - etag: '"abc123"', - }, - data: 'content', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - mockXmlParser.validateResponseContent.and.stub(); - - await api.download({ - path: '/test.txt', - localRev: '999999999999999999999', // Invalid timestamp - }); - - // Should not add If-Modified-Since header - expect(mockHttpAdapter.request).toHaveBeenCalledWith( - jasmine.objectContaining({ - headers: jasmine.objectContaining({ - Authorization: jasmine.any(String), - }), - }), - ); - const callArgs = mockHttpAdapter.request.calls.mostRecent().args[0]; - expect(callArgs.headers?.['If-Modified-Since']).toBeUndefined(); - }); - - it('should handle invalid date string and fall back to ETag', async () => { - const mockResponse = { - status: 200, - headers: { - etag: '"abc123"', - }, - data: 'content', - }; - mockHttpAdapter.request.and.returnValue(Promise.resolve(mockResponse)); - mockXmlParser.validateResponseContent.and.stub(); - - await api.download({ - path: '/test.txt', - localRev: 'Invalid Date String', - }); - - // Should fall back to If-None-Match - expect(mockHttpAdapter.request).toHaveBeenCalledWith( - jasmine.objectContaining({ - headers: jasmine.objectContaining({ - 'If-None-Match': 'Invalid Date String', - }), - }), - ); - }); + // Test removed: If-None-Match header functionality has been removed + // Test removed: If-Modified-Since header functionality has been removed + // Test removed: If-Modified-Since header functionality has been removed + // Test removed: 304 Not Modified handling has been removed + // Test removed: 304 Not Modified handling has been removed + // Test removed: localRev parameter has been removed from download method + // Test removed: localRev parameter has been removed from download method }); describe('upload', () => { diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav-api.ts b/src/app/pfapi/api/sync/providers/webdav/webdav-api.ts index c1c03b76f..a964382e5 100644 --- a/src/app/pfapi/api/sync/providers/webdav/webdav-api.ts +++ b/src/app/pfapi/api/sync/providers/webdav/webdav-api.ts @@ -67,72 +67,20 @@ export class WebdavApi { } } - async download({ - path, - localRev, - }: { - path: string; - localRev?: string | null; - // TODO remove - }): Promise<{ + async download({ path }: { path: string }): Promise<{ rev: string; dataStr: string; lastModified?: string; - notModified?: boolean; }> { const cfg = await this._getCfgOrError(); const fullPath = this._buildFullPath(cfg.baseUrl, path); try { - const headers: Record = {}; - - // Add conditional headers if we have a local revision - if (localRev) { - if (this._isLikelyTimestamp(localRev)) { - // Convert timestamp to HTTP date - const date = new Date(parseInt(localRev)); - if (isNaN(date.getTime())) { - PFLog.warn( - `${WebdavApi.L}.download() Invalid timestamp for conditional request: ${localRev}`, - ); - } else { - headers[WebDavHttpHeader.IF_MODIFIED_SINCE] = date.toUTCString(); - } - } else if (this._isLikelyDateString(localRev)) { - // Validate and normalize the date string - const parsedDate = new Date(localRev); - if (isNaN(parsedDate.getTime())) { - PFLog.warn( - `${WebdavApi.L}.download() Invalid date string for conditional request: ${localRev}`, - ); - // Fall back to treating it as an ETag - headers[WebDavHttpHeader.IF_NONE_MATCH] = localRev; - } else { - // Use normalized UTC format - headers[WebDavHttpHeader.IF_MODIFIED_SINCE] = parsedDate.toUTCString(); - } - } else { - // Assume it's an ETag - headers[WebDavHttpHeader.IF_NONE_MATCH] = localRev; - } - } - const response = await this._makeRequest({ url: fullPath, method: WebDavHttpMethod.GET, - headers, }); - // Handle 304 Not Modified - if (response.status === WebDavHttpStatus.NOT_MODIFIED) { - // File hasn't changed - return the current revision - return { - rev: localRev || '', - dataStr: '', - notModified: true, - }; - } - // Validate it's not an HTML error page this.xmlParser.validateResponseContent( response.data, diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav.ts b/src/app/pfapi/api/sync/providers/webdav/webdav.ts index 7dba070b5..224e27588 100644 --- a/src/app/pfapi/api/sync/providers/webdav/webdav.ts +++ b/src/app/pfapi/api/sync/providers/webdav/webdav.ts @@ -70,28 +70,14 @@ export class Webdav implements SyncProviderServiceInterface { - SyncLog.debug(Webdav.L, 'downloadFile', { targetPath, localRev }); + async downloadFile(targetPath: string): Promise<{ rev: string; dataStr: string }> { + SyncLog.debug(Webdav.L, 'downloadFile', { targetPath }); const { filePath } = await this._getConfigAndPath(targetPath); - // For metadata file, don't send localRev if it might not exist remotely - const effectiveLocalRev = targetPath === '__meta_' && localRev ? null : localRev; - const result = await this._api.download({ path: filePath, - localRev: effectiveLocalRev, }); - // If the file wasn't modified (304), we should have notModified flag - if (result.notModified) { - // Return current revision with empty data - the sync system should handle this - // TODO handle this in syncService - return { rev: localRev || result.rev, dataStr: '' }; - } - if (!result.dataStr && result.dataStr !== '') { throw new InvalidDataSPError(targetPath); } diff --git a/src/app/pfapi/api/sync/sync-provider.interface.ts b/src/app/pfapi/api/sync/sync-provider.interface.ts index d75761737..203d3f6d8 100644 --- a/src/app/pfapi/api/sync/sync-provider.interface.ts +++ b/src/app/pfapi/api/sync/sync-provider.interface.ts @@ -54,14 +54,10 @@ export interface SyncProviderServiceInterface { /** * Downloads file data from the sync provider * @param targetPath Path to the file - * @param localRev Current local revision; can be used to check if download is necessary * @returns The file data and revision information * @throws Error if the download fails */ - downloadFile( - targetPath: string, - localRev: string | null, - ): Promise; + downloadFile(targetPath: string): Promise; /** * Uploads file data to the sync provider diff --git a/src/app/pfapi/api/sync/sync.service.ts b/src/app/pfapi/api/sync/sync.service.ts index ffb84e2c4..28f035ac3 100644 --- a/src/app/pfapi/api/sync/sync.service.ts +++ b/src/app/pfapi/api/sync/sync.service.ts @@ -113,9 +113,7 @@ export class SyncService { } } - const { remoteMeta, remoteMetaRev } = await this._metaFileSyncService.download( - localMeta0.metaRev, - ); + const { remoteMeta, remoteMetaRev } = await this._metaFileSyncService.download(); // we load again, to get the latest local changes for our checks and the data to upload const localMeta = await this._metaModelCtrl.load();