mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-25 08:53:50 +00:00
fix(sync-providers): bound dropbox token refresh to single retry; share md5 rev helper
The five hand-rolled token-refresh blocks in Dropbox.{getFileRev,
downloadFile, uploadFile, removeFile, listFiles} recursed on themselves
after refresh. If the post-refresh call still saw a token error (real
case: the refresh token itself was revoked), the recursion would not
terminate. Consolidated into a single _withTokenRefresh helper that
attempts the call, refreshes once on a token error, retries once, then
lets the outer 401 classifier surface AuthFailSPError.
Same log message, same _isTokenError discriminator, same refresh call.
Same five sites still apply their post-call non-token error mapping
(NoRev, InvalidData, RemoteFileNotFound, path-not-found swallow, etc.).
Also extracts md5 content-rev computation duplicated between
LocalFileSyncBase._getLocalRev and WebdavApi._computeContentHash into a
shared file-based/content-rev.ts; both call sites preserve their own
error wrapping at the boundary.
This commit is contained in:
parent
b50fb7fc49
commit
5089b8d987
4 changed files with 81 additions and 75 deletions
13
packages/sync-providers/src/file-based/content-rev.ts
Normal file
13
packages/sync-providers/src/file-based/content-rev.ts
Normal file
|
|
@ -0,0 +1,13 @@
|
|||
import { md5 } from 'hash-wasm';
|
||||
|
||||
/**
|
||||
* Compute the content-addressable revision (md5 hash) for a sync payload.
|
||||
*
|
||||
* Used by file-based providers (WebDAV, local-file) that have no
|
||||
* server-supplied revision and must derive one from the file contents.
|
||||
*
|
||||
* Callers are responsible for any wrapping in provider-specific API errors
|
||||
* (e.g. `FileHashCreationAPIError`) — this helper just forwards the
|
||||
* underlying `hash-wasm` rejection.
|
||||
*/
|
||||
export const computeContentRev = (content: string): Promise<string> => md5(content);
|
||||
|
|
@ -129,21 +129,13 @@ export class Dropbox implements FileSyncProvider<
|
|||
localRev: string | null,
|
||||
): Promise<{ rev: string }> {
|
||||
try {
|
||||
const r = await this._api.getMetaData(
|
||||
this._getPath(targetPath),
|
||||
localRev,
|
||||
targetPath,
|
||||
const r = await this._withTokenRefresh(() =>
|
||||
this._api.getMetaData(this._getPath(targetPath), localRev, targetPath),
|
||||
);
|
||||
return {
|
||||
rev: r.rev,
|
||||
};
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return this.getFileRev(targetPath, localRev);
|
||||
}
|
||||
|
||||
if (this._isPathNotFoundError(e)) {
|
||||
throw new RemoteFileNotFoundAPIError(targetPath);
|
||||
}
|
||||
|
|
@ -165,40 +157,33 @@ export class Dropbox implements FileSyncProvider<
|
|||
* @throws InvalidDataSPError if the data is invalid
|
||||
*/
|
||||
async downloadFile(targetPath: string): Promise<{ rev: string; dataStr: string }> {
|
||||
try {
|
||||
const r = await this._api.download({
|
||||
const r = await this._withTokenRefresh(() =>
|
||||
this._api.download({
|
||||
path: this._getPath(targetPath),
|
||||
targetPath,
|
||||
});
|
||||
}),
|
||||
);
|
||||
|
||||
if (!r.meta.rev) {
|
||||
throw new NoRevAPIError();
|
||||
}
|
||||
|
||||
if (!r.data) {
|
||||
throw new RemoteFileNotFoundAPIError(targetPath);
|
||||
}
|
||||
|
||||
if (typeof r.data !== 'string') {
|
||||
// A1: never log the raw user blob — only its shape.
|
||||
this._deps.logger.critical(`${Dropbox.L}.downloadFile got non-string data`, {
|
||||
dataType: typeof r.data,
|
||||
});
|
||||
throw new InvalidDataSPError('Dropbox download returned non-string data');
|
||||
}
|
||||
|
||||
return {
|
||||
rev: r.meta.rev,
|
||||
dataStr: r.data,
|
||||
};
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return this.downloadFile(targetPath);
|
||||
}
|
||||
throw e;
|
||||
if (!r.meta.rev) {
|
||||
throw new NoRevAPIError();
|
||||
}
|
||||
|
||||
if (!r.data) {
|
||||
throw new RemoteFileNotFoundAPIError(targetPath);
|
||||
}
|
||||
|
||||
if (typeof r.data !== 'string') {
|
||||
// A1: never log the raw user blob — only its shape.
|
||||
this._deps.logger.critical(`${Dropbox.L}.downloadFile got non-string data`, {
|
||||
dataType: typeof r.data,
|
||||
});
|
||||
throw new InvalidDataSPError('Dropbox download returned non-string data');
|
||||
}
|
||||
|
||||
return {
|
||||
rev: r.meta.rev,
|
||||
dataStr: r.data,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -238,30 +223,23 @@ export class Dropbox implements FileSyncProvider<
|
|||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const r = await this._api.upload({
|
||||
const r = await this._withTokenRefresh(() =>
|
||||
this._api.upload({
|
||||
path: this._getPath(targetPath),
|
||||
data: dataStr,
|
||||
revToMatch: effectiveRev,
|
||||
isForceOverwrite,
|
||||
targetPath,
|
||||
});
|
||||
}),
|
||||
);
|
||||
|
||||
if (!r.rev) {
|
||||
throw new NoRevAPIError();
|
||||
}
|
||||
|
||||
return {
|
||||
rev: r.rev,
|
||||
};
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return this.uploadFile(targetPath, dataStr, revToMatch, isForceOverwrite);
|
||||
}
|
||||
throw e;
|
||||
if (!r.rev) {
|
||||
throw new NoRevAPIError();
|
||||
}
|
||||
|
||||
return {
|
||||
rev: r.rev,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -272,14 +250,10 @@ export class Dropbox implements FileSyncProvider<
|
|||
*/
|
||||
async removeFile(targetPath: string): Promise<void> {
|
||||
try {
|
||||
await this._api.remove(this._getPath(targetPath), targetPath);
|
||||
await this._withTokenRefresh(() =>
|
||||
this._api.remove(this._getPath(targetPath), targetPath),
|
||||
);
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return this.removeFile(targetPath);
|
||||
}
|
||||
|
||||
if (this._isPathNotFoundError(e)) {
|
||||
throw new RemoteFileNotFoundAPIError(targetPath);
|
||||
}
|
||||
|
|
@ -296,14 +270,10 @@ export class Dropbox implements FileSyncProvider<
|
|||
this._deps.logger.normal(`${Dropbox.L}.listFiles()`, { dirPath });
|
||||
try {
|
||||
// DropboxApi.listFiles now returns full paths, so no need to prepend _getPath
|
||||
return await this._api.listFiles(this._getPath(dirPath), dirPath);
|
||||
return await this._withTokenRefresh(() =>
|
||||
this._api.listFiles(this._getPath(dirPath), dirPath),
|
||||
);
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return this.listFiles(dirPath);
|
||||
}
|
||||
|
||||
if (this._isPathNotFoundError(e)) {
|
||||
// If the directory doesn't exist, return empty array
|
||||
return [];
|
||||
|
|
@ -420,4 +390,27 @@ export class Dropbox implements FileSyncProvider<
|
|||
apiError.response.data?.error_summary?.includes(INVALID_TOKEN_ERROR))
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Runs `fn` and, if it throws an expired/invalid-token error, refreshes the
|
||||
* access token once and retries. Any other error (or a second token error
|
||||
* on retry) is rethrown unchanged so the caller's existing classifier can
|
||||
* map it to `RemoteFileNotFoundAPIError` / `AuthFailSPError` / etc.
|
||||
*
|
||||
* Single retry only — matches the previous hand-rolled recursion which
|
||||
* would have re-entered the same site on a still-expired token and looped
|
||||
* forever; bounding to one retry is the documented behavior here.
|
||||
*/
|
||||
private async _withTokenRefresh<T>(fn: () => Promise<T>): Promise<T> {
|
||||
try {
|
||||
return await fn();
|
||||
} catch (e) {
|
||||
if (this._isTokenError(e)) {
|
||||
this._deps.logger.critical('EXPIRED or INVALID TOKEN, trying to refresh');
|
||||
await this._api.updateAccessTokenFromRefreshTokenIfAvailable();
|
||||
return await fn();
|
||||
}
|
||||
throw e;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,4 +1,3 @@
|
|||
import { md5 } from 'hash-wasm';
|
||||
import type { SyncLogger } from '@sp/sync-core';
|
||||
import type { SyncCredentialStorePort } from '../../credential-store-port';
|
||||
import type { FileAdapter } from '../../file-adapter';
|
||||
|
|
@ -11,6 +10,7 @@ import {
|
|||
RemoteFileNotFoundAPIError,
|
||||
UploadRevToMatchMismatchAPIError,
|
||||
} from '../../errors';
|
||||
import { computeContentRev } from '../content-rev';
|
||||
import { PROVIDER_ID_LOCAL_FILE, type LocalFileSyncPrivateCfg } from './local-file.model';
|
||||
|
||||
export interface LocalFileSyncBaseDeps {
|
||||
|
|
@ -190,7 +190,7 @@ export abstract class LocalFileSyncBase implements FileSyncProvider<
|
|||
|
||||
let hash: string;
|
||||
try {
|
||||
hash = await md5(dataStr);
|
||||
hash = await computeContentRev(dataStr);
|
||||
} catch (e) {
|
||||
throw new FileHashCreationAPIError(e);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
import type { SyncLogger } from '@sp/sync-core';
|
||||
import { md5 as md5HashWasm } from 'hash-wasm';
|
||||
import {
|
||||
EmptyRemoteBodySPError,
|
||||
HttpNotOkAPIError,
|
||||
|
|
@ -9,6 +8,7 @@ import {
|
|||
RemoteFileNotFoundAPIError,
|
||||
} from '../../errors';
|
||||
import { errorMeta } from '../../log/error-meta';
|
||||
import { computeContentRev } from '../content-rev';
|
||||
import { WebDavHttpHeader, WebDavHttpMethod, WebDavHttpStatus } from './webdav.const';
|
||||
import type { WebDavHttpAdapter, WebDavHttpResponse } from './webdav-http-adapter';
|
||||
import { FileMeta, WebdavXmlParser } from './webdav-xml-parser';
|
||||
|
|
@ -35,7 +35,7 @@ export class WebdavApi {
|
|||
}
|
||||
|
||||
private async _computeContentHash(data: string): Promise<string> {
|
||||
return md5HashWasm(data);
|
||||
return computeContentRev(data);
|
||||
}
|
||||
|
||||
// ==============================
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue