From 6c7dbfce3b05c17621641c79a3ef1930ebf731a9 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 16 Sep 2026 18:57:27 +0200 Subject: [PATCH] fix(upload): honor `root` and `retries` upload options Both options were declared and documented on `UploadOptions` but never read: `upload()` and `batchUpload()` hard-coded the uploader destination as the base, and `retries` was spread into `UploadFile`, which only destructured `headers` and `noChunking` - so chunk workspace creation used a hard-coded 5 and the upload request got no retry count at all. `upload()` and `batchUpload()` now resolve their target via a shared helper falling back to the uploader destination, and `retries` (default 5) is stored on `UploadFile`/`UploadFileTree`, forwarded to `uploadData`/`initChunkWorkspace` and propagated to child uploads. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- lib/upload/uploader/Upload.spec.ts | 4 ++ lib/upload/uploader/Upload.ts | 19 +++++++- lib/upload/uploader/UploadFile.spec.ts | 44 +++++++++++++++++++ lib/upload/uploader/UploadFile.ts | 23 ++++------ lib/upload/uploader/UploadFileTree.spec.ts | 26 ++++++++++- lib/upload/uploader/UploadFileTree.ts | 46 +++++++------------- lib/upload/uploader/Uploader.spec.ts | 50 ++++++++++++++++++++++ lib/upload/uploader/Uploader.ts | 20 +++++++-- 8 files changed, 179 insertions(+), 53 deletions(-) diff --git a/lib/upload/uploader/Upload.spec.ts b/lib/upload/uploader/Upload.spec.ts index 641d5bff3..963c6cf50 100644 --- a/lib/upload/uploader/Upload.spec.ts +++ b/lib/upload/uploader/Upload.spec.ts @@ -11,6 +11,10 @@ import { Upload } from './Upload.ts' class TestUpload extends Upload { public source: string = '/destination/file.txt' + constructor() { + super({}) + } + public async start(queue: PQueue): Promise { queue.add(() => Promise.resolve()) } diff --git a/lib/upload/uploader/Upload.ts b/lib/upload/uploader/Upload.ts index c3eed9f4d..a3738a803 100644 --- a/lib/upload/uploader/Upload.ts +++ b/lib/upload/uploader/Upload.ts @@ -75,8 +75,25 @@ export interface IUpload extends TypedEventTarget { cancel(): void } -export abstract class Upload extends TypedEventTarget implements Partial { +export interface IUploadOptions { + headers: Record + noChunking: boolean + retries: number +} + +export abstract class Upload extends TypedEventTarget implements Partial { #abortController = new AbortController() + protected readonly options: IUploadOptions & CustomOpts + + protected constructor(options: Partial & CustomOpts) { + super() + this.options = { + headers: {}, + noChunking: false, + retries: 5, + ...options, + } + } /** * The destination of this upload. diff --git a/lib/upload/uploader/UploadFile.spec.ts b/lib/upload/uploader/UploadFile.spec.ts index 7d8fd387f..4061a32eb 100644 --- a/lib/upload/uploader/UploadFile.spec.ts +++ b/lib/upload/uploader/UploadFile.spec.ts @@ -96,6 +96,50 @@ describe('chunking', () => { }) }) +describe('retries', () => { + it('defaults to 5 retries for a plain upload', async () => { + isPublicShareMock.mockReturnValue(false) + getMaxChunksSizeMock.mockReturnValue(1024 * 1024) + uploadDataMock.mockResolvedValue(undefined) + + const uploadFile = new UploadFile('/destination', new File(['x'.repeat(1024)], 'filename'), {}) + const queue = { add: vi.fn((fn: () => Promise) => fn()) } + await uploadFile.start(queue as never) + await queue.add.mock.calls[0][0]() + + expect(uploadDataMock).toHaveBeenLastCalledWith('/destination', expect.anything(), expect.objectContaining({ retries: 5 })) + }) + + it('forwards the configured retries to the upload request', async () => { + isPublicShareMock.mockReturnValue(false) + getMaxChunksSizeMock.mockReturnValue(1024 * 1024) + uploadDataMock.mockResolvedValue(undefined) + + const uploadFile = new UploadFile('/destination', new File(['x'.repeat(1024)], 'filename'), { retries: 2 }) + const queue = { add: vi.fn((fn: () => Promise) => fn()) } + await uploadFile.start(queue as never) + await queue.add.mock.calls[0][0]() + + expect(uploadDataMock).toHaveBeenLastCalledWith('/destination', expect.anything(), expect.objectContaining({ retries: 2 })) + }) + + it('forwards the configured retries to chunked uploads and the workspace creation', async () => { + isPublicShareMock.mockReturnValue(false) + getMaxChunksSizeMock.mockReturnValue(1024) + initChunkWorkspaceMock.mockResolvedValue('/tmp/temporary') + uploadDataMock.mockResolvedValue(undefined) + vi.spyOn(axios, 'request').mockResolvedValueOnce({}) + + const uploadFile = new UploadFile('/destination', new File(['x'.repeat(4096)], 'bigfile'), { retries: 2 }) + const queue = { add: vi.fn((fn: () => Promise) => fn()) } + await uploadFile.start(queue as never) + await Promise.all(queue.add.mock.results.map((r) => r.value)) + + expect(initChunkWorkspaceMock).toHaveBeenLastCalledWith('/destination', 2, false, {}) + expect(uploadDataMock).toHaveBeenLastCalledWith(expect.any(String), expect.anything(), expect.objectContaining({ retries: 2 })) + }) +}) + describe('upload status and events', () => { it('initialized', () => { const uploadFile = new UploadFile('/destination', new File(['x'.repeat(2048)], 'filename'), { noChunking: false }) diff --git a/lib/upload/uploader/UploadFile.ts b/lib/upload/uploader/UploadFile.ts index c9daaee68..938d52263 100644 --- a/lib/upload/uploader/UploadFile.ts +++ b/lib/upload/uploader/UploadFile.ts @@ -4,7 +4,7 @@ */ import type PQueue from 'p-queue' -import type { IUpload, TUploadStatus } from './Upload.ts' +import type { IUpload, IUploadOptions, TUploadStatus } from './Upload.ts' import axios from '@nextcloud/axios' import { join } from '@nextcloud/paths' @@ -21,10 +21,8 @@ import { Upload, UploadStatus } from './Upload.ts' * A class representing a single file to be uploaded */ export class UploadFile extends Upload implements IUpload { - #customHeaders: Record #fileHandle: File | FileSystemFileEntry #file?: File - #noChunking: boolean public source: string public status: TUploadStatus = UploadStatus.INITIALIZED @@ -36,13 +34,9 @@ export class UploadFile extends Upload implements IUpload { constructor( destination: string, fileHandle: File | FileSystemFileEntry, - options: { headers?: Record, noChunking?: boolean }, + options: Partial = {}, ) { - super() - const { - headers = {}, - noChunking = false, - } = options + super(options) // exposed state this.source = destination @@ -50,8 +44,6 @@ export class UploadFile extends Upload implements IUpload { // private state this.#fileHandle = fileHandle - this.#customHeaders = headers - this.#noChunking = noChunking this.signal.addEventListener('abort', () => { if (this.status !== UploadStatus.FAILED) { this.status = UploadStatus.CANCELLED @@ -61,7 +53,7 @@ export class UploadFile extends Upload implements IUpload { get isChunked(): boolean { const maxChunkSize = getMaxChunksSize('size' in this.#fileHandle ? this.#fileHandle.size : undefined) - return !this.#noChunking + return !this.options.noChunking && maxChunkSize > 0 && this.totalBytes > maxChunkSize && (!isPublicShare() || supportsPublicChunking()) @@ -121,7 +113,7 @@ export class UploadFile extends Upload implements IUpload { this.status = UploadStatus.UPLOADING // The `Destination` header must be a URI, so the source has to be encoded here const destination = encodeUrl(this.source) - const temporaryUrl = await initChunkWorkspace(destination, 5, isPublicShare(), this.#customHeaders) + const temporaryUrl = await initChunkWorkspace(destination, this.options.retries, isPublicShare(), this.options.headers) const promises: Promise[] = [] const chunkSize = Math.floor(this.totalBytes / this.numberOfChunks) @@ -149,7 +141,7 @@ export class UploadFile extends Upload implements IUpload { method: 'MOVE', url: `${temporaryUrl}/.file`, headers: { - ...this.#customHeaders, + ...this.options.headers, ...getMtimeHeader(this.#file!), 'OC-Total-Length': this.totalBytes, Destination: destination, @@ -183,6 +175,7 @@ export class UploadFile extends Upload implements IUpload { chunk, { signal: this.signal, + retries: this.options.retries, onUploadProgress: ({ bytes }) => { // As this is only the sent bytes not the processed ones we only count 90%. // When the upload is finished (server acknowledged the upload) the remaining 10% will be correctly set. @@ -193,7 +186,7 @@ export class UploadFile extends Upload implements IUpload { this.uploadedBytes = 0 }, headers: { - ...this.#customHeaders, + ...this.options.headers, ...getMtimeHeader(this.#file!), 'Content-Type': this.#file!.type, }, diff --git a/lib/upload/uploader/UploadFileTree.spec.ts b/lib/upload/uploader/UploadFileTree.spec.ts index a0a615231..e699de748 100644 --- a/lib/upload/uploader/UploadFileTree.spec.ts +++ b/lib/upload/uploader/UploadFileTree.spec.ts @@ -20,6 +20,7 @@ const uploadFileMocks = vi.hoisted(() => { const instances: Array<{ source: string signal: AbortSignal + options: unknown start: ReturnType cancel: ReturnType rebase: ReturnType @@ -28,6 +29,7 @@ const uploadFileMocks = vi.hoisted(() => { class MockUploadFile { public source: string + public options: unknown public status: number = UploadStatus.INITIALIZED #abortController = new AbortController() @@ -49,9 +51,9 @@ const uploadFileMocks = vi.hoisted(() => { this.source = source }) - // eslint-disable-next-line @typescript-eslint/no-unused-vars - public constructor(source: string, _file: File, _options: unknown) { + public constructor(source: string, _file: File, options: unknown) { this.source = source + this.options = options instances.push(this) } } @@ -131,6 +133,26 @@ describe('UploadFileTree', () => { expect(snapshot[2].source).toBe('/destination/folder/nested.txt') }) + it('passes the configured retries down to nested child uploads', async () => { + const directory = await createDirectoryTree() + const tree = new UploadFileTree('/destination', directory, { retries: 2 }) + tree.initialize() + + // both the direct child file and the one nested in a sub directory + expect(uploadFileMocks.instances).toHaveLength(2) + for (const instance of uploadFileMocks.instances) { + expect(instance.options).toMatchObject({ retries: 2 }) + } + }) + + it('defaults to 5 retries for child uploads', async () => { + const directory = await createDirectoryTree() + const tree = new UploadFileTree('/destination', directory, {}) + tree.initialize() + + expect(uploadFileMocks.instances[0].options).toMatchObject({ retries: 5 }) + }) + it('cancels child uploads when aborted', async () => { const directory = await createDirectoryTree() const tree = new UploadFileTree('/destination', directory, {}) diff --git a/lib/upload/uploader/UploadFileTree.ts b/lib/upload/uploader/UploadFileTree.ts index 351df522e..77b94a145 100644 --- a/lib/upload/uploader/UploadFileTree.ts +++ b/lib/upload/uploader/UploadFileTree.ts @@ -4,7 +4,7 @@ */ import type PQueue from 'p-queue' -import type { IUpload, TUploadStatus } from './Upload.ts' +import type { IUpload, IUploadOptions, TUploadStatus } from './Upload.ts' import axios, { isAxiosError } from '@nextcloud/axios' import { basename } from '@nextcloud/paths' @@ -31,20 +31,19 @@ import { UploadFile } from './UploadFile.ts' */ export type ConflictsCallback = (nodes: string[], currentPath: string) => Promise> +interface IUploadFileTreeOptions { + /** The callback to handle conflicts */ + callback?: ConflictsCallback +} + /** * A class representing a single file to be uploaded */ -export class UploadFileTree extends Upload implements IUpload { - /** Customer headers passed */ - #customHeaders: Record +export class UploadFileTree extends Upload implements IUpload { /** The current file tree to upload */ #directory: FileTree - /** Whether chunking is disabled */ - #noChunking: boolean /** Children uploads of this parent folder upload */ #children: (Upload & IUpload)[] = [] - /** The callback to handle conflicts */ - #conflictsCallback?: ConflictsCallback /** Whether we need to check for conflicts or not (newly created parent folders = no conflict resolution needed) */ protected needConflictResolution = true @@ -58,24 +57,13 @@ export class UploadFileTree extends Upload implements IUpload { constructor( destination: string, directory: FileTree, - options: { - callback?: ConflictsCallback - headers?: Record - noChunking?: boolean - }, + options: Partial = {}, ) { - super() - const { - headers = {}, - noChunking = false, - } = options + super(options) // exposed state this.source = destination this.#directory = directory - this.#customHeaders = headers - this.#noChunking = noChunking - this.#conflictsCallback = options.callback this.signal.addEventListener('abort', () => { for (const child of this.#children) { @@ -121,11 +109,7 @@ export class UploadFileTree extends Upload implements IUpload { const upload = new UploadFileTree( concatUrl(this.source, child.originalName), child, - { - callback: this.#conflictsCallback, - headers: this.#customHeaders, - noChunking: this.#noChunking, - }, + this.options, ) this.#children.push(upload) grandchildren.push(...upload.initialize()) @@ -133,7 +117,7 @@ export class UploadFileTree extends Upload implements IUpload { const upload = new UploadFile( concatUrl(this.source, child.name), child, - { headers: this.#customHeaders, noChunking: this.#noChunking }, + this.options, ) this.#children.push(upload) } @@ -152,8 +136,8 @@ export class UploadFileTree extends Upload implements IUpload { this.status = UploadStatus.UPLOADING await this.#createDirectory(queue) - if (this.needConflictResolution && this.#conflictsCallback) { - const nodes = await this.#conflictsCallback( + if (this.needConflictResolution && this.options.callback) { + const nodes = await this.options.callback( this.#directory.children.map((node) => basename(node.name)), this.source, ) @@ -219,7 +203,7 @@ export class UploadFileTree extends Upload implements IUpload { await axios.head(encodeUrl(this.source), { signal: this.signal, headers: { - ...this.#customHeaders, + ...this.options.headers, }, }) return // directory already exists, no need to create it @@ -236,7 +220,7 @@ export class UploadFileTree extends Upload implements IUpload { method: 'MKCOL', url: encodeUrl(this.source), headers: { - ...this.#customHeaders, + ...this.options.headers, ...getMtimeHeader(this.#directory), }, signal: this.signal, diff --git a/lib/upload/uploader/Uploader.spec.ts b/lib/upload/uploader/Uploader.spec.ts index 3102d4c4a..f9926c405 100644 --- a/lib/upload/uploader/Uploader.spec.ts +++ b/lib/upload/uploader/Uploader.spec.ts @@ -26,6 +26,11 @@ vi.mock('../../utils/logger.ts', () => ({ default: { debug: vi.fn(), info: vi.fn // Provide simple mocks for UploadFile and UploadFileTree so we can deterministically // simulate progress/finished events and exercise uploader logic. +// The constructor arguments are captured so we can assert on the resolved upload target. +const uploadFileMock = vi.hoisted(() => ({ + instances: [] as Array<{ destination: string, options: Record }>, +})) + vi.mock('./UploadFile.ts', () => ({ UploadFile: class implements IUpload { source = 'file:///test' @@ -39,6 +44,7 @@ vi.mock('./UploadFile.ts', () => ({ private listeners: Record void)[]> constructor(..._args: any[]) { const file = _args[1] + uploadFileMock.instances.push({ destination: _args[0], options: _args[2] ?? {} }) this.listeners = {} this.totalBytes = (file && file.size) || 0 this.uploadedBytes = 0 @@ -118,6 +124,7 @@ describe('Uploader (current API)', () => { beforeEach(() => { authMock.getCurrentUser.mockReturnValue({ uid: 'tester' }) uploadFileTreeMock.instances.length = 0 + uploadFileMock.instances.length = 0 }) afterEach(() => { @@ -243,6 +250,49 @@ describe('Uploader (current API)', () => { expect(finished).toHaveBeenCalled() }) + describe('upload target resolution', () => { + // destination folder source is mocked to https://localhost/remote.php/dav/files/test + const defaultRoot = 'https://localhost/remote.php/dav/files/test' + const otherRoot = 'https://localhost/remote.php/dav/files/test/subfolder' + + it('uploads relative to the uploader destination by default', async () => { + const uploader = new Uploader() + await uploader.upload('/hello.txt', new File(['a'], 'hello.txt')) + expect(uploadFileMock.instances[0].destination).toBe(`${defaultRoot}/hello.txt`) + }) + + it('honours the root override for a single upload', async () => { + const uploader = new Uploader() + await uploader.upload('/hello.txt', new File(['a'], 'hello.txt'), { root: otherRoot }) + expect(uploadFileMock.instances[0].destination).toBe(`${otherRoot}/hello.txt`) + // the override must not leak into the uploader state + expect(uploader.destination.source).toBe(defaultRoot) + }) + + it('honours the root override for a batch upload', async () => { + const uploader = new Uploader() + await uploader.batchUpload('/dir', [new File(['a'], 'a.txt')], { root: otherRoot }) + expect(uploadFileTreeMock.instances[0].destination).toBe(`${otherRoot}/dir`) + expect(uploader.destination.source).toBe(defaultRoot) + }) + + it('normalizes slashes between the root override and the destination', async () => { + const uploader = new Uploader() + await uploader.upload('hello.txt', new File(['a'], 'hello.txt'), { root: `${otherRoot}/` }) + expect(uploadFileMock.instances[0].destination).toBe(`${otherRoot}/hello.txt`) + }) + + it('makes the batch upload conflicts callback relative to the overridden root', async () => { + const userCallback = vi.fn(async () => ({})) + const uploader = new Uploader() + await uploader.batchUpload('/dir', [new File(['a'], 'a.txt')], { root: otherRoot, callback: userCallback }) + + const wrapped = uploadFileTreeMock.instances[0].options.callback as (nodes: string[], path: string) => Promise + await wrapped(['file.txt'], `${otherRoot}/dir/sub`) + expect(userCallback).toHaveBeenLastCalledWith(['file.txt'], 'sub') + }) + }) + it('performs batchUpload using UploadFileTree and initializes children', async () => { const uploader = new Uploader() const uploads = await uploader.batchUpload('/dir', [new File(['a'], 'a.txt')]) diff --git a/lib/upload/uploader/Uploader.ts b/lib/upload/uploader/Uploader.ts index bde6da245..d7db7427d 100644 --- a/lib/upload/uploader/Uploader.ts +++ b/lib/upload/uploader/Uploader.ts @@ -57,8 +57,9 @@ interface BaseOptions { interface UploadOptions extends BaseOptions { /** - * The root folder where to upload. - * Allows to override the current root of the uploader for this upload + * The root folder where to upload, as an absolute WebDAV source URL. + * Allows to override the current destination of the uploader for this upload, + * without changing the destination for any other upload. */ root?: string @@ -313,7 +314,7 @@ export class Uploader extends TypedEventTarget { const rootFolder = new Directory('') await rootFolder.addChildren(files) // create a meta upload to ensure all ongoing child requests are listed - const target = `${this.destination.source.replace(/\/$/, '')}/${destination.replace(/^\//, '')}` + const target = this.#resolveTarget(destination, options?.root) const headers = Object.fromEntries(this.#customHeaders.entries()) const _callback = options?.callback // The callback is adjusted to be called with the path relative to the upload target @@ -347,7 +348,7 @@ export class Uploader extends TypedEventTarget { * @param options - Optional parameters */ public async upload(destination: string, fileHandle: File | FileSystemFileEntry, options?: UploadOptions): Promise { - const target = `${this.destination.source.replace(/\/$/, '')}/${destination.replace(/^\//, '')}` + const target = this.#resolveTarget(destination, options?.root) const headers = Object.fromEntries(this.#customHeaders.entries()) const upload = new UploadFile(target, fileHandle, { ...options, headers }) if (options?.signal) { @@ -362,6 +363,17 @@ export class Uploader extends TypedEventTarget { return upload } + /** + * Resolve the absolute upload target for a destination relative to the root folder. + * + * @param destination - The destination path relative to the root folder + * @param root - Optional root to use instead of the current uploader destination + */ + #resolveTarget(destination: string, root?: string): string { + const base = root ?? this.#destinationFolder.source + return `${base.replace(/\/$/, '')}/${destination.replace(/^\//, '')}` + } + /** * Start the statistics tracking for a newly queued upload. * The ETA is only resumed when the uploader is not paused,