From ea9269b4267598fe7f35334381cbe7ac73271e0d Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Tue, 11 Jun 2024 23:26:52 +0200 Subject: [PATCH] feedback 5 --- .../src/client/elementHandle.ts | 62 +++++++++++-------- .../playwright-core/src/protocol/validator.ts | 2 + .../dispatchers/writableStreamDispatcher.ts | 4 -- packages/playwright-core/src/server/dom.ts | 1 + .../src/server/fileUploadUtils.ts | 13 ++-- packages/protocol/src/channels.ts | 4 ++ packages/protocol/src/protocol.yml | 2 + tests/page/page-set-input-files.spec.ts | 14 +++++ 8 files changed, 66 insertions(+), 36 deletions(-) diff --git a/packages/playwright-core/src/client/elementHandle.ts b/packages/playwright-core/src/client/elementHandle.ts index deb335e3b0..b7cf8cd57d 100644 --- a/packages/playwright-core/src/client/elementHandle.ts +++ b/packages/playwright-core/src/client/elementHandle.ts @@ -250,7 +250,7 @@ export function convertSelectOptionValues(values: string | api.ElementHandle | S return { options: values as SelectOption[] }; } -type SetInputFilesFiles = Pick; +type SetInputFilesFiles = Pick; function filePayloadExceedsSizeLimit(payloads: FilePayload[]) { return payloads.reduce((size, item) => size + (item.buffer ? item.buffer.byteLength : 0), 0) >= fileUploadSizeLimit; @@ -262,36 +262,46 @@ export async function convertInputFiles(files: string | FilePayload | string[] | if (items.some(item => typeof item === 'string')) { if (!items.every(item => typeof item === 'string')) throw new Error('File paths cannot be mixed with buffers'); - const itemFileTypes = (await Promise.all((items as string[]).map(async item => (await fs.promises.stat(item)).isDirectory() ? 'directory' : 'file'))); - if (new Set(itemFileTypes).size > 1 || itemFileTypes.filter(type => type === 'directory').length > 1) + let localPaths: string[] | undefined; + let localDirectory: string | undefined; + for (const item of items) { + const stat = await fs.promises.stat(item as string); + if (stat.isDirectory()) { + if (localDirectory) + throw new Error('Multiple directories are not supported'); + localDirectory = path.resolve(item as string); + } else { + localPaths ??= []; + localPaths.push(path.resolve(item as string)); + } + } + if (localPaths?.length && localDirectory) throw new Error('File paths must be all files or a single directory'); if (context._connection.isRemote()) { - const streams = (await Promise.all((items).map(async item => { - const isDirectory = (await fs.promises.stat(item as string)).isDirectory(); - const files = isDirectory ? (await fs.promises.readdir(item as string, { withFileTypes: true, recursive: true })).filter(f => f.isFile()).map(f => path.join(f.path, f.name)) : [item]; - const { writableStreams, rootDir } = await context._wrapApiCall(async () => context._channel.createTempFiles({ - rootDirName: isDirectory ? path.basename(item as string) : undefined, - items: await Promise.all((files as string[]).map(async file => { - const lastModifiedMs = (await fs.promises.stat(item as string)).mtimeMs; - return { - name: isDirectory ? path.relative(item as string, file) : path.basename(file), - lastModifiedMs - }; - })), - }), true); - for (let i = 0; i < files.length; i++) { - const writable = WritableStream.from(writableStreams[i]); - await pipelineAsync(fs.createReadStream((files as string[])[i]), writable.stream()); - } - return rootDir ?? writableStreams; - }))).flat(); - return { streams }; + const files = localDirectory ? (await fs.promises.readdir(localDirectory, { withFileTypes: true, recursive: true })).filter(f => f.isFile()).map(f => path.join(f.path, f.name)) : localPaths!; + const { writableStreams, rootDir } = await context._wrapApiCall(async () => context._channel.createTempFiles({ + rootDirName: localDirectory ? path.basename(localDirectory as string) : undefined, + items: await Promise.all(files.map(async file => { + const lastModifiedMs = (await fs.promises.stat(file)).mtimeMs; + return { + name: localDirectory ? path.relative(localDirectory as string, file) : path.basename(file), + lastModifiedMs + }; + })), + }), true); + for (let i = 0; i < files.length; i++) { + const writable = WritableStream.from(writableStreams[i]); + await pipelineAsync(fs.createReadStream(files[i]), writable.stream()); + } + return { + directoryStream: rootDir, + streams: localDirectory ? undefined : writableStreams, + }; } - const resolvedItems = (items as string[]).map(f => path.resolve(f)); return { - localPaths: resolvedItems.filter((_, i) => itemFileTypes[i] === 'file'), - localDirectory: resolvedItems.find((_, i) => itemFileTypes[i] === 'directory'), + localPaths, + localDirectory, }; } diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index f976a05834..47ad034b03 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -1631,6 +1631,7 @@ scheme.FrameSetInputFilesParams = tObject({ buffer: tBinary, }))), localDirectory: tOptional(tString), + directoryStream: tOptional(tChannel(['WritableStream'])), localPaths: tOptional(tArray(tString)), streams: tOptional(tArray(tChannel(['WritableStream']))), timeout: tOptional(tNumber), @@ -1999,6 +2000,7 @@ scheme.ElementHandleSetInputFilesParams = tObject({ buffer: tBinary, }))), localDirectory: tOptional(tString), + directoryStream: tOptional(tChannel(['WritableStream'])), localPaths: tOptional(tArray(tString)), streams: tOptional(tArray(tChannel(['WritableStream']))), timeout: tOptional(tNumber), diff --git a/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts b/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts index 86623fbfcf..3e54e644d5 100644 --- a/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts @@ -57,8 +57,4 @@ export class WritableStreamDispatcher extends Dispatcher<{ guid: string, streamO return this._object.streamOrDirectory; return this._object.streamOrDirectory.path as string; } - - isDirectory(): boolean { - return typeof this._object.streamOrDirectory === 'string'; - } } diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index ecf6c8f92c..30eb04d3f6 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -652,6 +652,7 @@ export class ElementHandle extends js.JSHandle { await Promise.all((localPathsOrDirectory).map(localPath => ( fs.promises.access(localPath, fs.constants.F_OK) ))); + // Browsers traverse the given directory asynchronously and we want to ensure all files are uploaded. const waitForInputEvent = localDirectory ? this.evaluate(node => new Promise(fulfill => { node.addEventListener('input', fulfill, { once: true }); })).catch(() => {}) : Promise.resolve(); diff --git a/packages/playwright-core/src/server/fileUploadUtils.ts b/packages/playwright-core/src/server/fileUploadUtils.ts index 5e935057cd..acf75c14b1 100644 --- a/packages/playwright-core/src/server/fileUploadUtils.ts +++ b/packages/playwright-core/src/server/fileUploadUtils.ts @@ -30,16 +30,17 @@ async function filesExceedUploadLimit(files: string[]) { } export async function prepareFilesForUpload(frame: Frame, params: channels.ElementHandleSetInputFilesParams): Promise { - const { payloads, streams } = params; + const { payloads, streams, directoryStream } = params; let { localPaths, localDirectory } = params; - if ([payloads, localPaths, streams].filter(Boolean).length !== 1) + if ([payloads, localPaths, localDirectory, streams, directoryStream].filter(Boolean).length !== 1) throw new Error('Exactly one of payloads, localPaths and streams must be provided'); - if (streams) { - localPaths = streams.map(c => (c as WritableStreamDispatcher)).filter(s => !s.isDirectory()).map(s => s.path()); - localDirectory = streams.map(c => (c as WritableStreamDispatcher)).filter(s => s.isDirectory()).map(s => s.path())[0]; - } + if (streams) + localPaths = streams.map(c => (c as WritableStreamDispatcher)).map(s => s.path()); + if (directoryStream) + localDirectory = (directoryStream as WritableStreamDispatcher | undefined)?.path(); + if (localPaths) { for (const p of localPaths) assert(path.isAbsolute(p) && path.resolve(p) === p, 'Paths provided to localPaths must be absolute and fully resolved.'); diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 8ed7b1ba5a..7cb6acf47c 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -2918,6 +2918,7 @@ export type FrameSetInputFilesParams = { buffer: Binary, }[], localDirectory?: string, + directoryStream?: WritableStreamChannel, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -2931,6 +2932,7 @@ export type FrameSetInputFilesOptions = { buffer: Binary, }[], localDirectory?: string, + directoryStream?: WritableStreamChannel, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -3544,6 +3546,7 @@ export type ElementHandleSetInputFilesParams = { buffer: Binary, }[], localDirectory?: string, + directoryStream?: WritableStreamChannel, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -3556,6 +3559,7 @@ export type ElementHandleSetInputFilesOptions = { buffer: Binary, }[], localDirectory?: string, + directoryStream?: WritableStreamChannel, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index 096f88ec5c..50fc2c2caf 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -2197,6 +2197,7 @@ Frame: mimeType: string? buffer: binary localDirectory: string? + directoryStream: WritableStream? localPaths: type: array? items: string @@ -2758,6 +2759,7 @@ ElementHandle: mimeType: string? buffer: binary localDirectory: string? + directoryStream: WritableStream? localPaths: type: array? items: string diff --git a/tests/page/page-set-input-files.spec.ts b/tests/page/page-set-input-files.spec.ts index 093be87f2b..cbada40fcc 100644 --- a/tests/page/page-set-input-files.spec.ts +++ b/tests/page/page-set-input-files.spec.ts @@ -80,6 +80,20 @@ it('should upload a folder and throw for multiple directories', async ({ page, s await expect(input.setInputFiles([ path.join(dir, 'folder1'), path.join(dir, 'folder2'), + ])).rejects.toThrow('Multiple directories are not supported'); +}); + +it('should throw if a directory and files are passed', async ({ page, server, browserName, headless, browserMajorVersion }) => { + await page.goto(server.PREFIX + '/input/folderupload.html'); + const input = await page.$('input'); + const dir = path.join(it.info().outputDir, 'file-upload-test'); + { + await fs.promises.mkdir(path.join(dir, 'folder1'), { recursive: true }); + await fs.promises.writeFile(path.join(dir, 'folder1', 'file1.txt'), 'file1 content'); + } + await expect(input.setInputFiles([ + path.join(dir, 'folder1'), + path.join(dir, 'folder1', 'file1.txt'), ])).rejects.toThrow('File paths must be all files or a single directory'); });