Mirrors/super-productivity
1
0
Fork 0
mirror of https://github.com/johannesjo/super-productivity.git synced 2026-01-22 18:30:09 +00:00
Code Issues Projects Releases Wiki Activity

refactor(sync): remove rev param for download method, since we don't handle 304 and don't want manual caching

Browse source
This commit is contained in:
Johannes Millan 2025-07-18 19:26:11 +02:00
parent dd419a1d3c
commit c9e6bba64a
11 changed files with 24 additions and 280 deletions
Download patch file Download diff file Expand all files Collapse all files

8
src/app/pfapi/api/sync/meta-sync.service.ts
View file

@ -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

13
src/app/pfapi/api/sync/model-sync.service.spec.ts
View file

@ -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(

1
src/app/pfapi/api/sync/model-sync.service.ts
View file

@ -102,7 +102,6 @@ export class ModelSyncService<MD extends ModelCfgs> {
const syncProvider = this._currentSyncProvider$.getOrError();
const { rev, dataStr } = await syncProvider.downloadFile(
this._filePathForModelId(modelId),
expectedRev,
);
if (expectedRev) {
if (!rev || !this._isSameRev(rev, expectedRev)) {

4
src/app/pfapi/api/sync/providers/dropbox/dropbox-api.ts
View file

@ -83,10 +83,8 @@ export class DropboxApi {
*/
async download<T>({
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 } : {}),
},
});

8
src/app/pfapi/api/sync/providers/dropbox/dropbox.ts
View file

@ -112,14 +112,10 @@ export class Dropbox implements SyncProviderServiceInterface<SyncProviderId.Drop
* @throws RemoteFileNotFoundAPIError if the file doesn't exist
* @throws InvalidDataSPError if the data is invalid
*/
async downloadFile(
targetPath: string,
localRev: string,
): Promise<{ rev: string; dataStr: string }> {
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<SyncProviderId.Drop
if (this._isTokenError(e)) {
PFLog.critical('EXPIRED or INVALID TOKEN, trying to refresh');
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
return this.downloadFile(targetPath, localRev);
return this.downloadFile(targetPath);
}
throw e;
}

10
src/app/pfapi/api/sync/providers/local-file-sync/local-file-sync-base.ts
View file

@ -44,7 +44,7 @@ export abstract class LocalFileSyncBase
localRev,
});
try {
const r = await this.downloadFile(targetPath, localRev);
const r = await this.downloadFile(targetPath);
return { rev: r.rev };
} catch (e) {
PFLog.critical(`${LocalFileSyncBase.LB}.${this.getFileRev.name} error`, e);
@ -52,13 +52,9 @@ export abstract class LocalFileSyncBase
}
}
async downloadFile(
targetPath: string,
localRev: string,
): Promise<{ rev: string; dataStr: string }> {
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`,

178
src/app/pfapi/api/sync/providers/webdav/webdav-api.spec.ts
View file

@ -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', () => {

54
src/app/pfapi/api/sync/providers/webdav/webdav-api.ts
View file

@ -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<string, string> = {};
// 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,

18
src/app/pfapi/api/sync/providers/webdav/webdav.ts
View file

@ -70,28 +70,14 @@ export class Webdav implements SyncProviderServiceInterface<SyncProviderId.WebDA
return { rev: result.rev };
}
async downloadFile(
targetPath: string,
localRev: string,
): Promise<{ rev: string; dataStr: string }> {
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);
}

6
src/app/pfapi/api/sync/sync-provider.interface.ts
View file

@ -54,14 +54,10 @@ export interface SyncProviderServiceInterface<PID extends SyncProviderId> {
/**
* 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<FileDownloadResponse>;
downloadFile(targetPath: string): Promise<FileDownloadResponse>;
/**
* Uploads file data to the sync provider

4
src/app/pfapi/api/sync/sync.service.ts
View file

@ -113,9 +113,7 @@ export class SyncService<const MD extends ModelCfgs> {
}
}
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();