From 519d7706546a81e66871af30a946c6f50199a464 Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Thu, 17 Sep 2026 16:22:30 -0600 Subject: [PATCH 1/2] fix: stop reporting failed path lookups as 404 A Drive API lookup that timed out or failed was reported to WebDAV clients as a 404, so QNAP HBS3 read a transient backend timeout as "Target path does not exist" and went on to re-create folders, re-upload files and skip overwrites. Lookup failures are now classified: only a real 404 or a non-EXISTS item means absent. Timeouts, 408/429/5xx and network failures return 503 with Retry-After, and a rejected session returns 502 rather than a retry loop. The cached uuid is only evicted on a confirmed 404, and the metadata mappers reject a response body they cannot map. --- src/services/drive/drive-file.service.ts | 11 +- src/services/drive/drive-folder.service.ts | 2 +- src/services/drive/drive-item.service.ts | 39 ++++- src/utils/drive.utils.ts | 12 +- src/utils/errors.utils.ts | 90 ++++++++++-- src/utils/webdav.utils.ts | 40 ++--- src/webdav/middewares/errors.middleware.ts | 19 +-- test/fixtures/errors.fixture.ts | 20 +++ .../services/drive/drive-file.service.test.ts | 34 +++++ .../drive/drive-folder.service.test.ts | 29 +++- .../services/drive/drive-item.service.test.ts | 137 ++++++++++++++---- test/utils/errors.utils.test.ts | 108 +++++++++++--- test/utils/webdav.utils.test.ts | 79 +++++++++- test/webdav/handlers/GET.handler.test.ts | 24 ++- test/webdav/handlers/LOCK.handler.test.ts | 6 +- .../middlewares/errors.middleware.test.ts | 55 ++++++- 16 files changed, 590 insertions(+), 115 deletions(-) create mode 100644 test/fixtures/errors.fixture.ts diff --git a/src/services/drive/drive-file.service.ts b/src/services/drive/drive-file.service.ts index 6d0e186c..ad60c869 100644 --- a/src/services/drive/drive-file.service.ts +++ b/src/services/drive/drive-file.service.ts @@ -98,11 +98,10 @@ export class DriveFileService { const [getFileMetadata] = storageClient.getFile(uuid); - const fileMetadata = await getFileMetadata; - if (fileMetadata?.status !== FileStatus.EXISTS) { + const driveFileItem = DriveUtils.driveFileMetaToItem(await getFileMetadata); + if (driveFileItem.status !== FileStatus.EXISTS) { throw new NotFoundError(`File with uuid ${uuid} not found`); } - const driveFileItem = DriveUtils.driveFileMetaToItem(fileMetadata); return driveFileItem; }; @@ -110,12 +109,10 @@ export class DriveFileService { public getFileMetadataByPath = async (path: string): Promise => { const storageClient = SdkManager.instance.getStorage(); - const fileMetadata = await storageClient.getFileByPath(path); - - if (fileMetadata?.status !== FileStatus.EXISTS) { + const driveFileItem = DriveUtils.driveFileMetaToItem(await storageClient.getFileByPath(path)); + if (driveFileItem.status !== FileStatus.EXISTS) { throw new NotFoundError(`File with path ${path} not found`); } - const driveFileItem = DriveUtils.driveFileMetaToItem(fileMetadata); return driveFileItem; }; diff --git a/src/services/drive/drive-folder.service.ts b/src/services/drive/drive-folder.service.ts index 511846fa..fb088ff3 100644 --- a/src/services/drive/drive-folder.service.ts +++ b/src/services/drive/drive-folder.service.ts @@ -13,7 +13,7 @@ export class DriveFolderService { const storageClient = SdkManager.instance.getStorage(); const folderMeta = await storageClient.getFolderMeta(uuid); const folderItem = DriveUtils.driveFolderMetaToItem(folderMeta); - if (folderItem?.status !== FileStatus.EXISTS) { + if (folderItem.status !== FileStatus.EXISTS) { throw new NotFoundError(`Folder with uuid ${uuid} not found`); } return folderItem; diff --git a/src/services/drive/drive-item.service.ts b/src/services/drive/drive-item.service.ts index 878c5758..10834046 100644 --- a/src/services/drive/drive-item.service.ts +++ b/src/services/drive/drive-item.service.ts @@ -1,6 +1,6 @@ import { DriveItemRepository } from '../database/drive-item/drive-item.repository'; import { DriveFileItem, DriveFolderItem } from '../../types/drive.types'; -import { ErrorUtils, NotFoundError } from '../../utils/errors.utils'; +import { BadGatewayError, ErrorUtils, NotFoundError, ServiceUnavailableError } from '../../utils/errors.utils'; import { webdavLogger } from '../../utils/logger.utils'; import { DriveFileService } from './drive-file.service'; import { DriveFolderService } from './drive-folder.service'; @@ -9,6 +9,31 @@ import { DriveItemBD } from '../database/drive-item/drive-item.domain'; export class DriveItemService { static readonly instance = new DriveItemService(); + /** Only drop the cached uuid when the API confirms the item is gone; evicting on a timeout + * pushes every later request onto the slow path lookup. */ + private readonly dropCacheIfGone = async (uuid: string, error: unknown): Promise => { + if (!ErrorUtils.isNotFoundError(error)) return; + await DriveItemRepository.instance.delete([uuid]); + }; + + private readonly asLookupError = (itemType: 'File' | 'Folder', path: string, error: unknown): Error => { + switch (ErrorUtils.classifyLookupError(error)) { + case 'auth': + return new BadGatewayError( + ErrorUtils.withRequestId( + `The Internxt API rejected this session while looking up ${path}, log in again with 'internxt login'`, + error, + ), + ); + case 'inconclusive': + return new ServiceUnavailableError( + ErrorUtils.withRequestId(`${itemType} lookup at path ${path} could not be completed, retry later`, error), + ); + case 'not-found': + return new NotFoundError(`${itemType} not found at path: ${path}`); + } + }; + private readonly tryGetFileByUuid = async (cached: DriveItemBD, path: string): Promise => { try { const item = await DriveFileService.instance.getFileMetadata(cached.uuid); @@ -27,7 +52,7 @@ export class DriveItemService { path, uuid: cached.uuid, }); - await DriveItemRepository.instance.delete([cached.uuid]); + await this.dropCacheIfGone(cached.uuid, error); } }; @@ -52,14 +77,14 @@ export class DriveItemService { ErrorUtils.withRequestId('Folder metadata by uuid failed, falling back to path lookup', error), { path, uuid: cached.uuid }, ); - await DriveItemRepository.instance.delete([cached.uuid]); + await this.dropCacheIfGone(cached.uuid, error); } }; public getFileByPath = async (path: string): Promise => { const cached = await DriveItemRepository.instance.getByPath(path); - if (cached) { + if (cached?.type === 'file') { const item = await this.tryGetFileByUuid(cached, path); if (item) return item; } @@ -78,14 +103,14 @@ export class DriveItemService { return item; } catch (error) { ErrorUtils.logIfUnexpected(webdavLogger, 'File lookup by path failed', error, { path }); - throw new NotFoundError(`File not found at path: ${path}`); + throw this.asLookupError('File', path, error); } }; public getFolderByPath = async (path: string): Promise => { const cached = await DriveItemRepository.instance.getByPath(path); - if (cached) { + if (cached?.type === 'folder') { const item = await this.tryGetFolderByUuid(cached, path); if (item) return item; } @@ -104,7 +129,7 @@ export class DriveItemService { return item; } catch (error) { ErrorUtils.logIfUnexpected(webdavLogger, 'Folder lookup by path failed', error, { path }); - throw new NotFoundError(`Folder not found at path: ${path}`); + throw this.asLookupError('Folder', path, error); } }; } diff --git a/src/utils/drive.utils.ts b/src/utils/drive.utils.ts index fef2d105..5a66202d 100644 --- a/src/utils/drive.utils.ts +++ b/src/utils/drive.utils.ts @@ -1,7 +1,15 @@ import { FileMeta, FolderMeta, CreateFolderResponse, FileStatus } from '@internxt/sdk/dist/drive/storage/types'; import { DriveFileItem, DriveFolderItem } from '../types/drive.types'; +import { ServiceUnavailableError } from './errors.utils'; export class DriveUtils { + + private static assertUsableMeta(meta: { uuid?: string }, kind: 'file' | 'folder'): void { + if (!meta?.uuid) { + throw new ServiceUnavailableError(`Unusable ${kind} metadata received from the API`); + } + } + // WebDAV clients parse getcontentlength/Content-Length as an integer, a literal // "NaN" in the response breaks them, so any non-numeric size degrades to 0. static parseFileSize(size: string | number): number { @@ -10,9 +18,10 @@ export class DriveUtils { } static driveFileMetaToItem(fileMeta: FileMeta): DriveFileItem { + DriveUtils.assertUsableMeta(fileMeta, 'file'); return { itemType: 'file', - uuid: fileMeta.uuid ?? '', + uuid: fileMeta.uuid, status: fileMeta.status, folderUuid: fileMeta.folderUuid, size: DriveUtils.parseFileSize(fileMeta.size), @@ -28,6 +37,7 @@ export class DriveUtils { } static driveFolderMetaToItem(folderMeta: FolderMeta): DriveFolderItem { + DriveUtils.assertUsableMeta(folderMeta, 'folder'); return { itemType: 'folder', uuid: folderMeta.uuid, diff --git a/src/utils/errors.utils.ts b/src/utils/errors.utils.ts index 7e1faa31..6ce56f17 100644 --- a/src/utils/errors.utils.ts +++ b/src/utils/errors.utils.ts @@ -1,6 +1,10 @@ import { Logger } from 'winston'; import { logger } from './logger.utils'; +export type LookupErrorKind = 'not-found' | 'auth' | 'inconclusive'; + +export const DEFAULT_RETRY_AFTER_SECONDS = 5; + export class ErrorUtils { static readonly isError = (error: unknown): error is Error => { return typeof Error.isError === 'function' @@ -51,18 +55,61 @@ export class ErrorUtils { return requestId ? `${message} (requestId: ${requestId})` : message; }; + static readonly getStatusCode = (error: unknown, key?: 'statusCode' | 'status'): number | undefined => { + if (typeof error !== 'object' || error === null) return undefined; + + const source = error as Record; + const value = key ? source[key] : (source.statusCode ?? source.status); + return typeof value === 'number' && !Number.isNaN(value) ? value : undefined; + }; + static readonly isNotFoundError = (error: unknown): boolean => { - if (typeof error !== 'object' || error === null) return false; - const { status, statusCode } = error as { status?: unknown; statusCode?: unknown }; - return status === 404 || statusCode === 404; + return this.getStatusCode(error) === 404; + }; + + private static readonly hasResponseBody = (error: unknown): boolean => { + return typeof error === 'object' && error !== null && 'data' in error; }; - static readonly logIfUnexpected = ( - log: Logger, - message: string, - error: unknown, - meta?: Record, - ) => { + static readonly classifyLookupError = (error: unknown): LookupErrorKind => { + const status = this.getStatusCode(error); + + if (status === 404) return 'not-found'; + if (status === undefined || !this.hasResponseBody(error)) return 'inconclusive'; + + // TODO: the SDK sends `?path=` unencoded, so names with '%' can arrive malformed and names + // with '#'/'&' truncate the query. Until it encodes them, a rejected path reads as absent. + if (status === 400 || status === 414 || status === 422) return 'not-found'; + + if (this.isAuthStatus(status)) return 'auth'; + return 'inconclusive'; + }; + + private static readonly isAuthStatus = (status: number): boolean => status === 401 || status === 403; + + private static readonly isRetryableStatus = (status: number): boolean => + status === 408 || status === 425 || status === 429 || status >= 500; + + static readonly toWebDavStatus = (error: unknown): { statusCode: number; retryAfter?: number } => { + const ownStatus = this.getStatusCode(error, 'statusCode'); + if (ownStatus !== undefined) { + return error instanceof ServiceUnavailableError + ? { statusCode: ownStatus, retryAfter: error.retryAfter } + : { statusCode: ownStatus }; + } + + const apiStatus = this.getStatusCode(error, 'status'); + if (apiStatus === undefined) return { statusCode: 500 }; + + if (!this.hasResponseBody(error) || this.isRetryableStatus(apiStatus)) { + return { statusCode: 503, retryAfter: DEFAULT_RETRY_AFTER_SECONDS }; + } + if (this.isAuthStatus(apiStatus)) return { statusCode: 502 }; + + return { statusCode: apiStatus }; + }; + + static readonly logIfUnexpected = (log: Logger, message: string, error: unknown, meta?: Record) => { if (this.isNotFoundError(error)) return; const errorMessage = this.isError(error) ? error.message : String(error); log.warn(this.withRequestId(`${message}: ${errorMessage}`, error), meta); @@ -89,6 +136,31 @@ export class NotFoundError extends Error { } } +/** The resource's state could not be determined; 503 asks the client to retry instead of + * telling it the resource is gone, which makes clients re-create it. */ +export class ServiceUnavailableError extends Error { + public statusCode = 503; + public retryAfter: number; + + constructor(message: string, retryAfter = DEFAULT_RETRY_AFTER_SECONDS) { + super(message); + this.name = 'ServiceUnavailableError'; + this.retryAfter = retryAfter; + Object.setPrototypeOf(this, ServiceUnavailableError.prototype); + } +} + +/** The CLI reached the WebDAV client but not the Internxt API on its behalf. */ +export class BadGatewayError extends Error { + public statusCode = 502; + + constructor(message: string) { + super(message); + this.name = 'BadGatewayError'; + Object.setPrototypeOf(this, BadGatewayError.prototype); + } +} + export class BadRequestError extends Error { public statusCode = 400; diff --git a/src/utils/webdav.utils.ts b/src/utils/webdav.utils.ts index 1d4fdbd9..23015a33 100644 --- a/src/utils/webdav.utils.ts +++ b/src/utils/webdav.utils.ts @@ -8,6 +8,7 @@ import { ConfigService } from '../services/config.service'; import { TrashService } from '../services/drive/trash.service'; import { FormatUtils } from './format.utils'; import { DriveItemRepository } from '../services/database/drive-item/drive-item.repository'; +import { ErrorUtils, ServiceUnavailableError } from './errors.utils'; export class WebDavUtils { static joinURL(...pathComponents: string[]): string { @@ -59,38 +60,39 @@ export class WebDavUtils { static async getDriveFileFromResource(url: string): Promise { try { return await DriveItemService.instance.getFileByPath(url); - } catch { - // no op + } catch (error) { + if (ErrorUtils.isNotFoundError(error)) return undefined; + throw error; } } static async getDriveFolderFromResource(url: string): Promise { try { return await DriveItemService.instance.getFolderByPath(url); - } catch { - // no op + } catch (error) { + if (ErrorUtils.isNotFoundError(error)) return undefined; + throw error; } } static async getDriveItemFromResource(resource: WebDavRequestedResource): Promise { - let item: DriveItem | undefined = undefined; - - const isFolder = resource.url.endsWith('/'); + if (resource.url.endsWith('/')) { + return await this.getDriveFolderFromResource(resource.url); + } + let fileLookupError: ServiceUnavailableError | undefined; try { - if (isFolder) { - item = await DriveItemService.instance.getFolderByPath(resource.url); - } else { - try { - item = await DriveItemService.instance.getFileByPath(resource.url); - } catch { - item = await DriveItemService.instance.getFolderByPath(resource.url); - } - } - } catch { - //no op + const file = await this.getDriveFileFromResource(resource.url); + if (file) return file; + } catch (error) { + if (!(error instanceof ServiceUnavailableError)) throw error; + fileLookupError = error; } - return item; + + const folder = await this.getDriveFolderFromResource(resource.url); + if (folder) return folder; + if (fileLookupError) throw fileLookupError; + return undefined; } static async deleteOrTrashItem(driveItem: T) { diff --git a/src/webdav/middewares/errors.middleware.ts b/src/webdav/middewares/errors.middleware.ts index 572c7d22..c2ef6da2 100644 --- a/src/webdav/middewares/errors.middleware.ts +++ b/src/webdav/middewares/errors.middleware.ts @@ -19,19 +19,6 @@ const getErrorDetail = (err: unknown): string | undefined => { return undefined; }; -/** - * The CLI's own errors (BadRequestError, NotFoundError, ...) expose `statusCode`, - * but errors normalized by @internxt/sdk's HttpClient expose `status` instead. - */ -const getErrorStatusCode = (err: unknown): number | undefined => { - if (typeof err !== 'object' || err === null) return undefined; - - const { statusCode, status } = err as { statusCode?: unknown; status?: unknown }; - if (typeof statusCode === 'number' && !Number.isNaN(statusCode)) return statusCode; - if (typeof status === 'number' && !Number.isNaN(status)) return status; - return undefined; -}; - // eslint-disable-next-line @typescript-eslint/no-unused-vars export const ErrorHandlingMiddleware: ErrorRequestHandler = (err, req, res, _) => { let message = ErrorUtils.isError(err) ? err.message : 'Something went wrong'; @@ -59,7 +46,11 @@ export const ErrorHandlingMiddleware: ErrorRequestHandler = (err, req, res, _) = 'error', ); - const statusCode = getErrorStatusCode(err) ?? 500; + const { statusCode, retryAfter } = ErrorUtils.toWebDavStatus(err); + + if (retryAfter !== undefined) { + res.set('Retry-After', String(retryAfter)); + } res.set('Content-Type', 'application/xml; charset="utf-8"'); res.status(statusCode).send(errorBodyXML); diff --git a/test/fixtures/errors.fixture.ts b/test/fixtures/errors.fixture.ts new file mode 100644 index 00000000..2d6cb1a0 --- /dev/null +++ b/test/fixtures/errors.fixture.ts @@ -0,0 +1,20 @@ +import { AxiosResponseError, AxiosUnknownError } from '@internxt/sdk/dist/shared/types/errors'; +import { AxiosError } from 'axios'; + +/** An error as @internxt/sdk's HttpClient raises it from a real API response. */ +export const newApiError = ( + status: number, + { requestId, request = 'GET files/meta', data = {} }: { requestId?: string; request?: string; data?: unknown } = {}, +) => + new AxiosResponseError(`Request failed with status code ${status}`, request, { + status, + data, + headers: requestId ? { 'x-request-id': requestId } : {}, + statusText: '', + // @ts-expect-error partial AxiosResponse fixture, only the fields read by AxiosResponseError are needed + config: {}, + }); + +/** The SDK's error when no response arrived; it invents a status -- 500 if sent, 400 if not. */ +export const newNetworkError = ({ sent = true } = {}) => + new AxiosUnknownError('socket hang up', 'GET files/meta', { request: sent ? {} : undefined } as AxiosError); diff --git a/test/services/drive/drive-file.service.test.ts b/test/services/drive/drive-file.service.test.ts index 44096bbf..c6037d5b 100644 --- a/test/services/drive/drive-file.service.test.ts +++ b/test/services/drive/drive-file.service.test.ts @@ -7,6 +7,7 @@ import { randomUUID } from 'node:crypto'; import { CommonFixture } from '../../fixtures/common.fixture'; import { ConfigService } from '../../../src/services/config.service'; import { UserCredentialsFixture } from '../../fixtures/login.fixture'; +import { NotFoundError, ServiceUnavailableError } from '../../../src/utils/errors.utils'; describe('Drive file Service', () => { const sut = DriveFileService.instance; @@ -84,4 +85,37 @@ describe('Drive file Service', () => { expect(result.bucket).to.be.equal(fakeFileData.bucket); expect(result.uuid).to.be.equal(fakeFileData.uuid); }); + + test('when the API answers a path lookup with an empty body, then a retryable error is thrown', async () => { + const storageClientMock: Partial = { + getFileByPath: vi.fn().mockResolvedValue(''), + }; + + // @ts-expect-error - We only stub the method we need to test + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(storageClientMock); + + await expect(sut.getFileMetadataByPath('/a/b.txt')).rejects.toBeInstanceOf(ServiceUnavailableError); + }); + + test('when a path lookup answers with a trashed file, then a not found error is thrown', async () => { + const storageClientMock: Partial = { + getFileByPath: vi.fn().mockResolvedValue({ uuid: randomUUID(), status: 'TRASHED' }), + }; + + // @ts-expect-error - We only stub the method we need to test + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(storageClientMock); + + await expect(sut.getFileMetadataByPath('/a/b.txt')).rejects.toBeInstanceOf(NotFoundError); + }); + + test('when the API answers a uuid lookup with an empty body, then a retryable error is thrown', async () => { + const storageClientMock: Partial = { + getFile: vi.fn().mockReturnValue([Promise.resolve('')]), + }; + + // @ts-expect-error - We only stub the method we need to test + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(storageClientMock); + + await expect(sut.getFileMetadata(randomUUID())).rejects.toBeInstanceOf(ServiceUnavailableError); + }); }); diff --git a/test/services/drive/drive-folder.service.test.ts b/test/services/drive/drive-folder.service.test.ts index e12fa1f2..1ae67989 100644 --- a/test/services/drive/drive-folder.service.test.ts +++ b/test/services/drive/drive-folder.service.test.ts @@ -5,7 +5,13 @@ import { DriveFolderService } from '../../../src/services/drive/drive-folder.ser import { SdkManager } from '../../../src/services/sdk-manager.service'; import { DriveUtils } from '../../../src/utils/drive.utils'; import { generateSubcontent, newCreateFolderResponse, newFolderMeta } from '../../fixtures/drive.fixture'; -import { CreateFolderResponse, FetchPaginatedFile, FetchPaginatedFolder } from '@internxt/sdk/dist/drive/storage/types'; +import { + CreateFolderResponse, + FetchPaginatedFile, + FetchPaginatedFolder, + FolderMeta, +} from '@internxt/sdk/dist/drive/storage/types'; +import { NotFoundError, ServiceUnavailableError } from '../../../src/utils/errors.utils'; import { ConfigService } from '../../../src/services/config.service'; import { UserCredentialsFixture } from '../../fixtures/login.fixture'; @@ -98,4 +104,25 @@ describe('Drive Folder Service', () => { const newFolder = await createFolder; expect(newFolder).to.be.equal(newFolderResponse); }); + + test('when the API answers a path lookup with an empty body, then a retryable error is thrown', async () => { + vi.spyOn(Storage.prototype, 'getFolderByPath').mockResolvedValue('' as unknown as FolderMeta); + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(Storage.prototype); + + await expect(sut.getFolderMetaByPath('/a/b/')).rejects.toBeInstanceOf(ServiceUnavailableError); + }); + + test('when a path lookup answers with a trashed folder, then a not found error is thrown', async () => { + vi.spyOn(Storage.prototype, 'getFolderByPath').mockResolvedValue(newFolderMeta({ removed: true })); + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(Storage.prototype); + + await expect(sut.getFolderMetaByPath('/a/b/')).rejects.toBeInstanceOf(NotFoundError); + }); + + test('when the API answers a uuid lookup with an empty body, then a retryable error is thrown', async () => { + vi.spyOn(Storage.prototype, 'getFolderMeta').mockResolvedValue('' as unknown as FolderMeta); + vi.spyOn(SdkManager.instance, 'getStorage').mockReturnValue(Storage.prototype); + + await expect(sut.getFolderMetaByUuid(randomUUID())).rejects.toBeInstanceOf(ServiceUnavailableError); + }); }); diff --git a/test/services/drive/drive-item.service.test.ts b/test/services/drive/drive-item.service.test.ts index a3f2477f..a91ee778 100644 --- a/test/services/drive/drive-item.service.test.ts +++ b/test/services/drive/drive-item.service.test.ts @@ -5,19 +5,9 @@ import { DriveItemBD } from '../../../src/services/database/drive-item/drive-ite import { DriveFileService } from '../../../src/services/drive/drive-file.service'; import { DriveFolderService } from '../../../src/services/drive/drive-folder.service'; import { newFileItem, newFolderItem } from '../../fixtures/drive.fixture'; -import { NotFoundError } from '../../../src/utils/errors.utils'; +import { BadGatewayError, NotFoundError, ServiceUnavailableError } from '../../../src/utils/errors.utils'; import { webdavLogger } from '../../../src/utils/logger.utils'; -import { AxiosResponseError } from '@internxt/sdk/dist/shared/types/errors'; - -const createApiError = (status: number, requestId: string) => - new AxiosResponseError(`Request failed with status code ${status}`, 'GET files/meta', { - status, - data: {}, - headers: { 'x-request-id': requestId }, - statusText: '', - // @ts-expect-error partial AxiosResponse fixture, only the fields read by AxiosResponseError are needed - config: {}, - }); +import { newApiError } from '../../fixtures/errors.fixture'; describe('Drive Item Service', () => { const sut = DriveItemService.instance; @@ -26,19 +16,81 @@ describe('Drive Item Service', () => { test('when the path lookup fails with an unexpected API error, then it is logged with its request id', async () => { const path = '/test/file.txt'; vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); - vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(createApiError(500, 'req-123')); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue( + newApiError(500, { requestId: 'req-123' }), + ); - await expect(sut.getFileByPath(path)).rejects.toThrow('File not found at path'); + await expect(sut.getFileByPath(path)).rejects.toBeInstanceOf(ServiceUnavailableError); expect(webdavLogger.warn).toHaveBeenCalledWith( 'File lookup by path failed: Request failed with status code 500 (requestId: req-123)', { path }, ); }); + test('when the path lookup times out, then a retryable error is thrown instead of a not found', async () => { + const path = '/test/file.txt'; + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(newApiError(408)); + + await expect(sut.getFileByPath(path)).rejects.toMatchObject({ statusCode: 503, retryAfter: expect.any(Number) }); + }); + + test('when the path lookup fails with a network error, then a retryable error is thrown', async () => { + const path = '/test/file.txt'; + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new Error('socket hang up')); + + await expect(sut.getFileByPath(path)).rejects.toBeInstanceOf(ServiceUnavailableError); + }); + + test('when the API rejects the session, then a non-retryable gateway error is thrown', async () => { + const path = '/test/file.txt'; + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue( + newApiError(401, { requestId: 'req-401' }), + ); + + await expect(sut.getFileByPath(path)).rejects.toBeInstanceOf(BadGatewayError); + }); + + test('when the API rejects the path itself, then a not found error is thrown', async () => { + const path = '/test/file.txt'; + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue( + newApiError(400, { requestId: 'req-400' }), + ); + + await expect(sut.getFileByPath(path)).rejects.toBeInstanceOf(NotFoundError); + }); + + test('when the cached lookup fails transiently, then the cache entry is kept', async () => { + const path = '/test/file.txt'; + const cachedItem = new DriveItemBD({ + uuid: 'cached-uuid', + path, + type: 'file', + createdAt: new Date(), + updatedAt: new Date(), + }); + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); + vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue( + newApiError(504, { requestId: 'req-504' }), + ); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue( + newApiError(504, { requestId: 'req-504' }), + ); + const deleteSpy = vi.spyOn(DriveItemRepository.instance, 'delete').mockResolvedValue(undefined); + + await expect(sut.getFileByPath(path)).rejects.toBeInstanceOf(ServiceUnavailableError); + expect(deleteSpy).not.toHaveBeenCalled(); + }); + test('when the path lookup fails because the file does not exist, then nothing is logged', async () => { const path = '/test/file.txt'; vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); - vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(createApiError(404, 'req-404')); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue( + newApiError(404, { requestId: 'req-404' }), + ); await expect(sut.getFileByPath(path)).rejects.toThrow('File not found at path'); expect(webdavLogger.warn).not.toHaveBeenCalled(); @@ -79,7 +131,7 @@ describe('Drive Item Service', () => { vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue( new NotFoundError('File with uuid cached-uuid not found'), ); - vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new NotFoundError('Not found')); const deleteSpy = vi.spyOn(DriveItemRepository.instance, 'delete').mockResolvedValue(undefined); @@ -98,7 +150,7 @@ describe('Drive Item Service', () => { }); vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); - vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue(new Error('API error')); + vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue(new NotFoundError('API error')); const pathItem = newFileItem({ uuid: 'resolved-uuid' }); vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockResolvedValue(pathItem); @@ -124,8 +176,8 @@ describe('Drive Item Service', () => { }); vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); - vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue(new Error('API error')); - vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFileService.instance, 'getFileMetadata').mockRejectedValue(new NotFoundError('API error')); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new NotFoundError('Not found')); await expect(sut.getFileByPath(path)).rejects.toThrow('File not found at path'); }); @@ -147,7 +199,7 @@ describe('Drive Item Service', () => { test('when there is no cache and the path lookup fails, then a not found error is thrown', async () => { const path = '/test/nonexistent.txt'; vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); - vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFileService.instance, 'getFileMetadataByPath').mockRejectedValue(new NotFoundError('Not found')); await expect(sut.getFileByPath(path)).rejects.toThrow('File not found at path'); }); @@ -199,7 +251,7 @@ describe('Drive Item Service', () => { vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue( new NotFoundError('Folder with uuid cached-uuid not found'), ); - vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new NotFoundError('Not found')); const deleteSpy = vi.spyOn(DriveItemRepository.instance, 'delete').mockResolvedValue(undefined); @@ -218,7 +270,7 @@ describe('Drive Item Service', () => { }); vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); - vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue(new Error('API error')); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue(new NotFoundError('API error')); const pathItem = newFolderItem({ uuid: 'resolved-uuid' }); vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockResolvedValue(pathItem); @@ -244,8 +296,8 @@ describe('Drive Item Service', () => { }); vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); - vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue(new Error('API error')); - vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue(new NotFoundError('API error')); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new NotFoundError('Not found')); await expect(sut.getFolderByPath(path)).rejects.toThrow('Folder not found at path'); }); @@ -267,7 +319,7 @@ describe('Drive Item Service', () => { test('when there is no cache and the path lookup fails, then a not found error is thrown', async () => { const path = '/test/nonexistent/'; vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); - vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue(new NotFoundError('Not found')); await expect(sut.getFolderByPath(path)).rejects.toThrow('Folder not found at path'); }); @@ -281,5 +333,40 @@ describe('Drive Item Service', () => { await expect(sut.getFolderByPath(path)).rejects.toThrow('Folder not found at path'); }); + + test('when the path lookup times out, then a retryable error is thrown instead of a not found', async () => { + const path = '/test/folder/'; + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(undefined); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue( + newApiError(408, { requestId: 'req-408' }), + ); + + await expect(sut.getFolderByPath(path)).rejects.toMatchObject({ + statusCode: 503, + retryAfter: expect.any(Number), + }); + }); + + test('when the cached lookup fails transiently, then the cache entry is kept', async () => { + const path = '/test/folder/'; + const cachedItem = new DriveItemBD({ + uuid: 'cached-uuid', + path, + type: 'folder', + createdAt: new Date(), + updatedAt: new Date(), + }); + vi.spyOn(DriveItemRepository.instance, 'getByPath').mockResolvedValue(cachedItem); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByUuid').mockRejectedValue( + newApiError(500, { requestId: 'req-500' }), + ); + vi.spyOn(DriveFolderService.instance, 'getFolderMetaByPath').mockRejectedValue( + newApiError(500, { requestId: 'req-500' }), + ); + const deleteSpy = vi.spyOn(DriveItemRepository.instance, 'delete').mockResolvedValue(undefined); + + await expect(sut.getFolderByPath(path)).rejects.toBeInstanceOf(ServiceUnavailableError); + expect(deleteSpy).not.toHaveBeenCalled(); + }); }); }); diff --git a/test/utils/errors.utils.test.ts b/test/utils/errors.utils.test.ts index 66bc7445..2b998888 100644 --- a/test/utils/errors.utils.test.ts +++ b/test/utils/errors.utils.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from 'vitest'; -import { ErrorUtils } from '../../src/utils/errors.utils'; +import { ErrorUtils, NotFoundError, NotImplementedError, ServiceUnavailableError } from '../../src/utils/errors.utils'; +import { newApiError, newNetworkError } from '../fixtures/errors.fixture'; import { logger } from '../../src/utils/logger.utils'; -import { AxiosResponseError } from '@internxt/sdk/dist/shared/types/errors'; describe('Errors Utils', () => { test('when an error is reported, then it is logged with its details', () => { @@ -91,27 +91,13 @@ describe('Errors Utils', () => { describe('getRequestId', () => { test('when a Drive API error carries the x-request-id header, then its request id is returned', () => { - const error = new AxiosResponseError('Request failed with status code 500', 'PUT /files/uuid', { - status: 500, - data: {}, - headers: { 'x-request-id': 'req-123' }, - statusText: 'Internal Server Error', - // @ts-expect-error partial AxiosResponse fixture, only the fields read by AxiosResponseError are needed - config: {}, - }); + const error = newApiError(500, { requestId: 'req-123' }); expect(ErrorUtils.getRequestId(error)).toBe('req-123'); }); test('when a Drive API error only carries the request id in its response body, then it is returned', () => { - const error = new AxiosResponseError('Request failed with status code 500', 'PUT /files/uuid', { - status: 500, - data: { statusCode: 500, message: 'Internal Server Error', requestId: 'req-789' }, - headers: {}, - statusText: 'Internal Server Error', - // @ts-expect-error partial AxiosResponse fixture, only the fields read by AxiosResponseError are needed - config: {}, - }); + const error = newApiError(500, { data: { requestId: 'req-789' } }); expect(ErrorUtils.getRequestId(error)).toBe('req-789'); }); @@ -142,11 +128,95 @@ describe('Errors Utils', () => { }); }); + describe('classifyLookupError', () => { + test('when the API answered 404, then the item is conclusively absent', () => { + expect(ErrorUtils.classifyLookupError(newApiError(404))).toBe('not-found'); + expect(ErrorUtils.classifyLookupError(new NotFoundError('gone'))).toBe('not-found'); + }); + + test.each([400, 414, 422])('when the API rejected the path with %i, then the item reads as absent', (status) => { + expect(ErrorUtils.classifyLookupError(newApiError(status))).toBe('not-found'); + }); + + test.each([401, 403])('when the API rejected the session with %i, then retrying cannot help', (status) => { + expect(ErrorUtils.classifyLookupError(newApiError(status))).toBe('auth'); + }); + + test.each([408, 425, 429, 500, 502, 503, 504])( + 'when the lookup failed upstream with %i, then nothing can be concluded', + (status) => { + expect(ErrorUtils.classifyLookupError(newApiError(status))).toBe('inconclusive'); + }, + ); + + test('when no response ever arrived, then its invented status is not read as an answer', () => { + expect(ErrorUtils.getStatusCode(newNetworkError({ sent: false }))).toBe(400); + expect(ErrorUtils.classifyLookupError(newNetworkError({ sent: false }))).toBe('inconclusive'); + expect(ErrorUtils.classifyLookupError(newNetworkError())).toBe('inconclusive'); + }); + + test('when the error carries no status at all, then nothing can be concluded', () => { + expect(ErrorUtils.classifyLookupError(new Error('boom'))).toBe('inconclusive'); + expect(ErrorUtils.classifyLookupError(undefined)).toBe('inconclusive'); + }); + }); + + describe('toWebDavStatus', () => { + test('when a CLI error is raised, then its status reaches the client untouched', () => { + expect(ErrorUtils.toWebDavStatus(new NotFoundError('gone'))).toEqual({ statusCode: 404 }); + expect(ErrorUtils.toWebDavStatus(new NotImplementedError('no COPY'))).toEqual({ statusCode: 501 }); + expect(ErrorUtils.toWebDavStatus(new ServiceUnavailableError('busy', 7))).toEqual({ + statusCode: 503, + retryAfter: 7, + }); + }); + + test.each([408, 425, 429, 500, 502, 503, 504])( + 'when the API answered %i, then the client is told to retry', + (status) => { + expect(ErrorUtils.toWebDavStatus(newApiError(status))).toEqual({ statusCode: 503, retryAfter: 5 }); + }, + ); + + test.each([401, 403])('when the API rejected the session with %i, then the client gets a 502', (status) => { + expect(ErrorUtils.toWebDavStatus(newApiError(status))).toEqual({ statusCode: 502 }); + }); + + test.each([400, 404, 409, 412])('when the API answered %i, then it is passed through', (status) => { + expect(ErrorUtils.toWebDavStatus(newApiError(status))).toEqual({ statusCode: status }); + }); + + test('when no response ever arrived, then the invented status is not forwarded', () => { + expect(ErrorUtils.toWebDavStatus(newNetworkError({ sent: false }))).toEqual({ statusCode: 503, retryAfter: 5 }); + expect(ErrorUtils.toWebDavStatus(newNetworkError())).toEqual({ statusCode: 503, retryAfter: 5 }); + }); + + test('when the error carries no status at all, then it is an internal failure', () => { + expect(ErrorUtils.toWebDavStatus(new Error('boom'))).toEqual({ statusCode: 500 }); + }); + }); + + describe('getStatusCode', () => { + test('when the error exposes statusCode or status, then it is returned', () => { + expect(ErrorUtils.getStatusCode(new NotFoundError('gone'))).toBe(404); + expect(ErrorUtils.getStatusCode({ status: 503 })).toBe(503); + expect(ErrorUtils.getStatusCode({ statusCode: 409, status: 500 })).toBe(409); + }); + + test('when the error has no usable status, then nothing is returned', () => { + expect(ErrorUtils.getStatusCode(new Error('boom'))).toBeUndefined(); + expect(ErrorUtils.getStatusCode({ status: 'nope' })).toBeUndefined(); + expect(ErrorUtils.getStatusCode(null)).toBeUndefined(); + }); + }); + test('when a reported error has a request id, then it is logged with it', () => { const error = Object.assign(new Error('Test Error'), { xRequestId: 'req-123' }); ErrorUtils.report(error); - expect(logger.error).toHaveBeenCalledWith(expect.stringContaining('[REPORTED_ERROR]: Test Error (requestId: req-123)')); + expect(logger.error).toHaveBeenCalledWith( + expect.stringContaining('[REPORTED_ERROR]: Test Error (requestId: req-123)'), + ); }); }); diff --git a/test/utils/webdav.utils.test.ts b/test/utils/webdav.utils.test.ts index 2e86c601..5abcdf83 100644 --- a/test/utils/webdav.utils.test.ts +++ b/test/utils/webdav.utils.test.ts @@ -7,6 +7,7 @@ import { DriveItemRepository } from '../../src/services/database/drive-item/driv import { ConfigService } from '../../src/services/config.service'; import { TrashService } from '../../src/services/drive/trash.service'; import { getWebdavConfigMock } from '../fixtures/webdav.fixture'; +import { NotFoundError, ServiceUnavailableError } from '../../src/utils/errors.utils'; describe('Webdav utils', () => { describe('joinURL', () => { @@ -98,7 +99,9 @@ describe('Webdav utils', () => { test('when a folder is looked up by path, then it is returned', async () => { const expectedFolder = newFolderItem(); const findFolderStub = vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockResolvedValue(expectedFolder); - const findFileStub = vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue(new Error()); + const findFileStub = vi + .spyOn(DriveItemService.instance, 'getFileByPath') + .mockRejectedValue(new NotFoundError('File not found')); const driveFolderItem = await WebDavUtils.getDriveItemFromResource(requestFolderFixture); expect(driveFolderItem).to.be.deep.equal(expectedFolder); @@ -109,17 +112,63 @@ describe('Webdav utils', () => { test('when a folder is not found, then undefined is returned', async () => { const findFolderStub = vi .spyOn(DriveItemService.instance, 'getFolderByPath') - .mockRejectedValue(new Error('Folder not found')); + .mockRejectedValue(new NotFoundError('Folder not found')); const item = await WebDavUtils.getDriveItemFromResource(requestFolderFixture); expect(findFolderStub).toHaveBeenCalledOnce(); expect(item).toBeUndefined(); }); + test('when the folder lookup cannot be completed, then the error is propagated', async () => { + vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue( + new ServiceUnavailableError('Folder lookup could not be completed'), + ); + + await expect(WebDavUtils.getDriveItemFromResource(requestFolderFixture)).rejects.toBeInstanceOf( + ServiceUnavailableError, + ); + }); + + test('when neither the file nor the folder exist, then undefined is returned', async () => { + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue(new NotFoundError('File not found')); + const findFolderStub = vi + .spyOn(DriveItemService.instance, 'getFolderByPath') + .mockRejectedValue(new NotFoundError('Folder not found')); + + const item = await WebDavUtils.getDriveItemFromResource(requestFileFixture); + expect(findFolderStub).toHaveBeenCalledOnce(); + expect(item).toBeUndefined(); + }); + + test('when the file lookup cannot be completed but the path is a folder, then the folder is returned', async () => { + const expectedFolder = newFolderItem(); + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue( + new ServiceUnavailableError('File lookup could not be completed'), + ); + const findFolderStub = vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockResolvedValue(expectedFolder); + + const item = await WebDavUtils.getDriveItemFromResource(requestFileFixture); + expect(findFolderStub).toHaveBeenCalledOnce(); + expect(item).toBe(expectedFolder); + }); + + test('when the file lookup is inconclusive and the folder is absent, then the file error is propagated', async () => { + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue( + new ServiceUnavailableError('File lookup could not be completed'), + ); + vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue(new NotFoundError('Folder not found')); + + await expect(WebDavUtils.getDriveItemFromResource(requestFileFixture)).rejects.toThrow( + 'File lookup could not be completed', + ); + }); + test('when a file is looked up by path, then it is returned', async () => { const expectedFile = newFileItem(); const findFileStub = vi.spyOn(DriveItemService.instance, 'getFileByPath').mockResolvedValue(expectedFile); - const findFolderStub = vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue(new Error()); + const findFolderStub = vi + .spyOn(DriveItemService.instance, 'getFolderByPath') + .mockRejectedValue(new NotFoundError('Folder not found')); const driveFileItem = await WebDavUtils.getDriveItemFromResource(requestFileFixture); expect(driveFileItem).to.be.deep.equal(expectedFile); @@ -139,12 +188,22 @@ describe('Webdav utils', () => { }); test('when the file does not exist, then undefined is returned', async () => { - vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue(new NotFoundError('Not found')); const result = await WebDavUtils.getDriveFileFromResource('/path/to/nonexistent.txt'); expect(result).toBeUndefined(); }); + + test('when the file lookup cannot be completed, then the error is propagated', async () => { + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue( + new ServiceUnavailableError('File lookup could not be completed'), + ); + + await expect(WebDavUtils.getDriveFileFromResource('/path/to/file.txt')).rejects.toBeInstanceOf( + ServiceUnavailableError, + ); + }); }); describe('getDriveFolderFromResource', () => { @@ -158,12 +217,22 @@ describe('Webdav utils', () => { }); test('when the folder does not exist, then undefined is returned', async () => { - vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue(new Error('Not found')); + vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue(new NotFoundError('Not found')); const result = await WebDavUtils.getDriveFolderFromResource('/path/to/nonexistent/'); expect(result).toBeUndefined(); }); + + test('when the folder lookup cannot be completed, then the error is propagated', async () => { + vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue( + new ServiceUnavailableError('Folder lookup could not be completed'), + ); + + await expect(WebDavUtils.getDriveFolderFromResource('/path/to/folder/')).rejects.toBeInstanceOf( + ServiceUnavailableError, + ); + }); }); describe('generateETag', () => { diff --git a/test/webdav/handlers/GET.handler.test.ts b/test/webdav/handlers/GET.handler.test.ts index 1490a372..59d5af3b 100644 --- a/test/webdav/handlers/GET.handler.test.ts +++ b/test/webdav/handlers/GET.handler.test.ts @@ -10,7 +10,7 @@ import { import { GETRequestHandler } from '../../../src/webdav/handlers/GET.handler'; import { DriveItemService } from '../../../src/services/drive/drive-item.service'; import { AuthService } from '../../../src/services/auth.service'; -import { NotFoundError, RangeNotSatisfiableError } from '../../../src/utils/errors.utils'; +import { NotFoundError, RangeNotSatisfiableError, ServiceUnavailableError } from '../../../src/utils/errors.utils'; import { NetworkFacade } from '../../../src/services/network/network-facade.service'; import { WebDavUtils } from '../../../src/utils/webdav.utils'; import { WebDavRequestedResource } from '../../../src/types/webdav.types'; @@ -50,7 +50,7 @@ describe('GET request handler', () => { .mockResolvedValue(requestedFileResource); const getFileMetadataStub = vi .spyOn(DriveItemService.instance, 'getFileByPath') - .mockRejectedValue(new Error('File not found')); + .mockRejectedValue(new NotFoundError('File not found')); try { await sut.handle(request, response); @@ -62,6 +62,26 @@ describe('GET request handler', () => { expect(getFileMetadataStub).toHaveBeenCalledOnce(); }); + test('when the lookup cannot be completed, then the error is surfaced instead of a not found', async () => { + const requestedFileResource: WebDavRequestedResource = getRequestedFileResource(); + + const request = createWebDavRequestFixture({ + method: 'GET', + url: requestedFileResource.url, + headers: {}, + }); + const response = createWebDavResponseFixture({ + status: vi.fn().mockReturnValue({ send: vi.fn() }), + }); + + vi.spyOn(WebDavUtils, 'getRequestedResource').mockResolvedValue(requestedFileResource); + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue( + new ServiceUnavailableError('File lookup could not be completed'), + ); + + await expect(sut.handle(request, response)).rejects.toBeInstanceOf(ServiceUnavailableError); + }); + test('when a file is requested, then the server responds with its content', async () => { const requestedFileResource: WebDavRequestedResource = getRequestedFileResource(); diff --git a/test/webdav/handlers/LOCK.handler.test.ts b/test/webdav/handlers/LOCK.handler.test.ts index 7383ba3b..04e989e0 100644 --- a/test/webdav/handlers/LOCK.handler.test.ts +++ b/test/webdav/handlers/LOCK.handler.test.ts @@ -1,10 +1,14 @@ -import { describe, expect, test, vi } from 'vitest'; +import { beforeEach, describe, expect, test, vi } from 'vitest'; import { LOCKRequestHandler } from '../../../src/webdav/handlers/LOCK.handler'; import { WebDavUtils } from '../../../src/utils/webdav.utils'; import { newFileItem } from '../../fixtures/drive.fixture'; import { createWebDavRequestFixture, createWebDavResponseFixture } from '../../fixtures/webdav.fixture'; describe('LOCK request handler', () => { + beforeEach(() => { + vi.spyOn(WebDavUtils, 'getDriveItemFromResource').mockResolvedValue(undefined); + }); + test('when a lock request is made for a resource that does not exist yet, then the server responds with 201 (lock-null resource)', async () => { const requestHandler = new LOCKRequestHandler(); diff --git a/test/webdav/middlewares/errors.middleware.test.ts b/test/webdav/middlewares/errors.middleware.test.ts index 817bffec..bf79f6e2 100644 --- a/test/webdav/middlewares/errors.middleware.test.ts +++ b/test/webdav/middlewares/errors.middleware.test.ts @@ -1,9 +1,15 @@ import { describe, expect, test, vi } from 'vitest'; import { ErrorHandlingMiddleware } from '../../../src/webdav/middewares/errors.middleware'; import { createWebDavRequestFixture, createWebDavResponseFixture } from '../../fixtures/webdav.fixture'; -import { BadRequestError, NotFoundError, NotImplementedError } from '../../../src/utils/errors.utils'; +import { + BadRequestError, + NotFoundError, + NotImplementedError, + ServiceUnavailableError, +} from '../../../src/utils/errors.utils'; import { XMLUtils } from '../../../src/utils/xml.utils'; import { AxiosResponseError, AxiosUnknownError } from '@internxt/sdk/dist/shared/types/errors'; +import { newApiError } from '../../fixtures/errors.fixture'; import { AxiosError } from 'axios'; import { webdavLogger } from '../../../src/utils/logger.utils'; @@ -33,6 +39,46 @@ describe('Error handling middleware', () => { ); }); + test('when a lookup could not be completed, then the server responds with 503 and a Retry-After', () => { + const error = new ServiceUnavailableError('Folder lookup at path /a/b could not be completed, retry later', 7); + const res = createWebDavResponseFixture({ + status: vi.fn().mockReturnValue({ send: vi.fn() }), + }); + const req = createWebDavRequestFixture({ + method: 'PROPFIND', + url: '/a/b', + }); + + ErrorHandlingMiddleware(error, req, res, () => {}); + + expect(res.status).toHaveBeenCalledWith(503); + expect(res.set).toHaveBeenCalledWith('Retry-After', '7'); + }); + + test('when an error carries no retry hint, then no Retry-After header is set', () => { + const res = createWebDavResponseFixture({ + status: vi.fn().mockReturnValue({ send: vi.fn() }), + }); + const req = createWebDavRequestFixture({ + method: 'GET', + url: '/test', + }); + + ErrorHandlingMiddleware(new NotFoundError('Item not found'), req, res, () => {}); + + expect(res.set).not.toHaveBeenCalledWith('Retry-After', expect.anything()); + }); + + test('when the API could not answer, then the client is told to retry rather than given the raw status', () => { + const res = createWebDavResponseFixture({ status: vi.fn().mockReturnValue({ send: vi.fn() }) }); + const req = createWebDavRequestFixture({ method: 'PROPFIND', url: '/folder/' }); + + ErrorHandlingMiddleware(newApiError(408), req, res, () => {}); + + expect(res.status).toHaveBeenCalledWith(503); + expect(res.set).toHaveBeenCalledWith('Retry-After', '5'); + }); + test('when a bad request error occurs, then the server responds with a 400 status', () => { const errorMessage = 'Missing property "size"'; const error = new BadRequestError(errorMessage); @@ -159,7 +205,7 @@ describe('Error handling middleware', () => { ErrorHandlingMiddleware(error, req, res, () => {}); - expect(res.status).toHaveBeenCalledWith(500); + expect(res.status).toHaveBeenCalledWith(503); expect(webdavLogger.error).toHaveBeenCalledWith( expect.stringContaining('Request failed with status code 500 [Internal Server Error] (requestId: req-123)'), ); @@ -209,7 +255,7 @@ describe('Error handling middleware', () => { expect(destroySpy).not.toHaveBeenCalled(); }); - test('when a Drive API request fails without a response, then the server responds with the normalized status and no detail suffix', () => { + test('when a Drive API request fails without a response, then the client is told to retry and gets no detail suffix', () => { const axiosError = { message: 'Network Error', code: 'ECONNABORTED', @@ -228,7 +274,8 @@ describe('Error handling middleware', () => { ErrorHandlingMiddleware(error, req, res, () => {}); - expect(res.status).toHaveBeenCalledWith(400); + expect(res.status).toHaveBeenCalledWith(503); + expect(res.set).toHaveBeenCalledWith('Retry-After', '5'); expect(res.send).toHaveBeenCalledWith( XMLUtils.toWebDavXML( { From 0b50e8f56f48481e01b22675eceeee179fad8214 Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:01:46 -0600 Subject: [PATCH 2/2] test(webdav): assert handlers stay inert on an inconclusive lookup --- src/utils/errors.utils.ts | 3 +- .../webdav/lookup-failure-propagation.test.ts | 95 +++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 test/webdav/lookup-failure-propagation.test.ts diff --git a/src/utils/errors.utils.ts b/src/utils/errors.utils.ts index 6ce56f17..a35cf846 100644 --- a/src/utils/errors.utils.ts +++ b/src/utils/errors.utils.ts @@ -77,8 +77,7 @@ export class ErrorUtils { if (status === 404) return 'not-found'; if (status === undefined || !this.hasResponseBody(error)) return 'inconclusive'; - // TODO: the SDK sends `?path=` unencoded, so names with '%' can arrive malformed and names - // with '#'/'&' truncate the query. Until it encodes them, a rejected path reads as absent. + // The API rejects a malformed or over-long path with 400, which we cannot tell apart from the item being absent if (status === 400 || status === 414 || status === 422) return 'not-found'; if (this.isAuthStatus(status)) return 'auth'; diff --git a/test/webdav/lookup-failure-propagation.test.ts b/test/webdav/lookup-failure-propagation.test.ts new file mode 100644 index 00000000..65815729 --- /dev/null +++ b/test/webdav/lookup-failure-propagation.test.ts @@ -0,0 +1,95 @@ +import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { PROPFINDRequestHandler } from '../../src/webdav/handlers/PROPFIND.handler'; +import { PUTRequestHandler } from '../../src/webdav/handlers/PUT.handler'; +import { MKCOLRequestHandler } from '../../src/webdav/handlers/MKCOL.handler'; +import { MOVERequestHandler } from '../../src/webdav/handlers/MOVE.handler'; +import { DELETERequestHandler } from '../../src/webdav/handlers/DELETE.handler'; +import { + createWebDavRequestFixture, + createWebDavResponseFixture, + getWebdavConfigMock, +} from '../fixtures/webdav.fixture'; +import { UserCredentialsFixture } from '../fixtures/login.fixture'; +import { AuthService } from '../../src/services/auth.service'; +import { ConfigService } from '../../src/services/config.service'; +import { DriveItemService } from '../../src/services/drive/drive-item.service'; +import { DriveFileService } from '../../src/services/drive/drive-file.service'; +import { DriveFolderService } from '../../src/services/drive/drive-folder.service'; +import { WebDavFolderService } from '../../src/services/webdav/webdav-folder.service'; +import { TrashService } from '../../src/services/drive/trash.service'; +import { UploadUtils } from '../../src/utils/upload.utils'; +import { ServiceUnavailableError } from '../../src/utils/errors.utils'; + +/** A lookup that cannot be completed must leave the handler inert: nothing created, moved or + * trashed on a false "not found". */ +describe('WebDAV handlers when a path lookup cannot be completed', () => { + const lookupTimedOut = () => { + const error = new ServiceUnavailableError('Lookup at path could not be completed, retry later'); + vi.spyOn(DriveItemService.instance, 'getFileByPath').mockRejectedValue(error); + vi.spyOn(DriveItemService.instance, 'getFolderByPath').mockRejectedValue(error); + }; + + const response = () => createWebDavResponseFixture({ status: vi.fn().mockReturnThis() }); + + beforeEach(() => { + vi.spyOn(AuthService.instance, 'getAuthDetails').mockResolvedValue(UserCredentialsFixture); + vi.spyOn(ConfigService.instance, 'readWebdavConfig').mockResolvedValue(getWebdavConfigMock()); + lookupTimedOut(); + }); + + test('when a PROPFIND lookup is inconclusive, then a retryable error is reported instead of a 404', async () => { + const request = createWebDavRequestFixture({ method: 'PROPFIND', url: '/folder/file.txt', headers: {} }); + + await expect(new PROPFINDRequestHandler().handle(request, response())).rejects.toBeInstanceOf( + ServiceUnavailableError, + ); + }); + + test('when the parent lookup is inconclusive, then PUT does not create the parent folders', async () => { + vi.spyOn(UploadUtils, 'checkUploadSizeLimits').mockResolvedValue(undefined); + const createFolderSpy = vi.spyOn(WebDavFolderService.instance, 'createFolder'); + const createFileSpy = vi.spyOn(DriveFileService.instance, 'createFile'); + const request = createWebDavRequestFixture({ + method: 'PUT', + url: '/folder/file.txt', + headers: { 'content-length': '10' }, + }); + + await expect(new PUTRequestHandler().handle(request, response())).rejects.toBeInstanceOf(ServiceUnavailableError); + expect(createFolderSpy).not.toHaveBeenCalled(); + expect(createFileSpy).not.toHaveBeenCalled(); + }); + + test('when the lookup is inconclusive, then MKCOL does not create the folder', async () => { + const createFolderSpy = vi.spyOn(WebDavFolderService.instance, 'createFolder'); + const request = createWebDavRequestFixture({ method: 'MKCOL', url: '/folder/new/', headers: {} }); + + await expect(new MKCOLRequestHandler().handle(request, response())).rejects.toBeInstanceOf(ServiceUnavailableError); + expect(createFolderSpy).not.toHaveBeenCalled(); + }); + + test('when the source lookup is inconclusive, then MOVE does not move anything', async () => { + const moveFileSpy = vi.spyOn(DriveFileService.instance, 'moveFile'); + const moveFolderSpy = vi.spyOn(DriveFolderService.instance, 'moveFolder'); + const request = createWebDavRequestFixture({ + method: 'MOVE', + url: '/folder/file.txt', + headers: {}, + header: vi.fn((name: string) => (name === 'destination' ? 'http://localhost/folder/renamed.txt' : undefined)), + }); + + await expect(new MOVERequestHandler().handle(request, response())).rejects.toBeInstanceOf(ServiceUnavailableError); + expect(moveFileSpy).not.toHaveBeenCalled(); + expect(moveFolderSpy).not.toHaveBeenCalled(); + }); + + test('when the target lookup is inconclusive, then DELETE does not trash anything', async () => { + const trashSpy = vi.spyOn(TrashService.instance, 'trashItems'); + const request = createWebDavRequestFixture({ method: 'DELETE', url: '/folder/file.txt', headers: {} }); + + await expect(new DELETERequestHandler().handle(request, response())).rejects.toBeInstanceOf( + ServiceUnavailableError, + ); + expect(trashSpy).not.toHaveBeenCalled(); + }); +});