diff --git a/CHANGELOG.md b/CHANGELOG.md index 235e9f0d..ade02343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ *Note: Numbers like (\#123) point to closed Pull Requests on the fractal-web repository.* +# Unreleased + +* Used form data in tasks collection endpoint (\#669); + # 1.11.2 * Removed usage of `cache_dir` field (\#667); diff --git a/__tests__/v2/TaskCollection.test.js b/__tests__/v2/TaskCollection.test.js index 0fbb3fca..db394220 100644 --- a/__tests__/v2/TaskCollection.test.js +++ b/__tests__/v2/TaskCollection.test.js @@ -1,4 +1,4 @@ -import { describe, it, beforeEach, expect, vi } from 'vitest'; +import { describe, it, beforeEach, expect, vi, beforeAll } from 'vitest'; import { render, screen } from '@testing-library/svelte'; import userEvent from '@testing-library/user-event'; @@ -21,6 +21,20 @@ const mockedUser = { import TaskCollection from '../../src/lib/components/v2/tasks/TaskCollection.svelte'; describe('TaskCollection', () => { + beforeAll(() => { + expect.extend({ + toBeFormDataWith(received, expectedProperties) { + const pass = received instanceof FormData; + const receivedObject = pass ? Object.fromEntries(received.entries()) : {}; + expect(receivedObject).toMatchObject(expectedProperties); + return { + message: () => `expected ${received} to be FormData`, + pass + }; + } + }); + }); + beforeEach(() => { fetch.mockClear(); }); @@ -54,7 +68,7 @@ describe('TaskCollection', () => { expect(fetch).toHaveBeenCalledWith( '/api/v2/task/collect/pip?private=false&user_group_id=2', expect.objectContaining({ - body: JSON.stringify({ package: 'test-task' }) + body: expect.toBeFormDataWith({ package: 'test-task' }) }) ); }); @@ -88,7 +102,7 @@ describe('TaskCollection', () => { expect(fetch).toHaveBeenCalledWith( '/api/v2/task/collect/pip?private=true', expect.objectContaining({ - body: JSON.stringify({ package: 'test-task' }) + body: expect.toBeFormDataWith({ package: 'test-task' }) }) ); }); diff --git a/playwright.config.js b/playwright.config.js index e4de4388..e67cef38 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -107,7 +107,7 @@ export default defineConfig({ webServer: [ { - command: './tests/start-test-server.sh 2.9.2', + command: './tests/start-test-server.sh 2.10.0a0', port: 8000, waitForPort: true, stdout: 'pipe', diff --git a/src/lib/components/v2/tasks/TaskCollection.svelte b/src/lib/components/v2/tasks/TaskCollection.svelte index b6a07d0e..88833437 100644 --- a/src/lib/components/v2/tasks/TaskCollection.svelte +++ b/src/lib/components/v2/tasks/TaskCollection.svelte @@ -2,7 +2,6 @@ import { env } from '$env/dynamic/public'; import { onDestroy, onMount } from 'svelte'; import TaskGroupActivityLogsModal from '$lib/components/v2/tasks/TaskGroupActivityLogsModal.svelte'; - import { replaceEmptyStrings } from '$lib/common/component_utilities'; import { FormErrorHandler } from '$lib/common/errors'; import TaskGroupSelector from './TaskGroupSelector.svelte'; import { @@ -34,6 +33,12 @@ let pinnedPackageVersions = []; let privateTask = false; let selectedGroup = null; + + /** @type {FileList|null} */ + let wheelFiles = null; + /** @type {HTMLInputElement|undefined} */ + let wheelFileInput = undefined; + /** @type {TaskGroupActivityLogsModal} */ let taskGroupActivitiesLogsModal; /** @type {number|null} */ @@ -98,22 +103,34 @@ async function handleTaskCollection() { formErrorHandler.clearErrors(); - const headers = new Headers(); - headers.append('Content-Type', 'application/json'); + if (packageType === 'local' && (wheelFiles === null || wheelFiles.length === 0)) { + formErrorHandler.addValidationError('file', 'Required field'); + return; + } - const requestData = { - package: python_package, - python_version, - package_extras - }; + const formData = new FormData(); if (packageType === 'pypi') { - requestData.package_version = package_version; + formData.append('package', python_package); + } else if (wheelFiles?.length === 1) { + formData.append('file', wheelFiles[0]); + } + + if (python_version) { + formData.append('python_version', python_version); + } + + if (package_extras) { + formData.append('package_extras', package_extras); + } + + if (packageType === 'pypi' && package_version) { + formData.append('package_version', package_version); } const ppv = getPinnedPackageVersionsMap(); if (ppv) { - requestData.pinned_package_versions = ppv; + formData.append('pinned_package_versions', JSON.stringify(ppv)); } let url = `/api/v2/task/collect/pip?private=${privateTask}`; @@ -125,9 +142,9 @@ const response = await fetch(url, { method: 'POST', credentials: 'include', - headers: headers, - body: JSON.stringify(requestData, replaceEmptyStrings) + body: formData }); + taskCollectionInProgress = false; if (response.ok) { @@ -140,6 +157,7 @@ python_version = ''; package_extras = ''; pinnedPackageVersions = []; + clearWheelFileUpload(); } else { console.error('Task collection request failed'); await formErrorHandler.handleErrorResponse(response); @@ -216,6 +234,14 @@ ); } + function clearWheelFileUpload() { + wheelFiles = null; + if (wheelFileInput) { + wheelFileInput.value = ''; + } + formErrorHandler.removeValidationError('file'); + } + onDestroy(() => { clearTimeout(updateTasksCollectionTimeout); }); @@ -226,34 +252,50 @@
-
-
-
- + {#if packageType === 'pypi'} +
+
+
+ +
+ + {$validationErrors['package']}
- - {$validationErrors['package']} +
The name of a package published on PyPI
-
- {#if packageType === 'pypi'} - The name of a package published on PyPI - {:else} - The full path to a wheel file - {/if} + {:else if packageType === 'local'} +
+
+ + + {#if wheelFiles && wheelFiles.length > 0} + + {/if} + {$validationErrors['file']} +
-
+ {/if} {#if packageType === 'pypi'}
diff --git a/src/routes/proxy.js b/src/routes/proxy.js index 1460dc78..71e01c91 100644 --- a/src/routes/proxy.js +++ b/src/routes/proxy.js @@ -33,7 +33,10 @@ export function createPostProxy(path) { method: 'POST', credentials: 'include', headers: filterHeaders(request.headers), - body: JSON.stringify(await request.json()) + body: request.body, + // To avoid error "RequestInit: duplex option is required when sending a body" + // @ts-ignore, not standard, but supported by undici; enable re-streaming of request + duplex: 'half' }); } catch (err) { logger.debug(err); @@ -53,7 +56,9 @@ export function createPatchProxy(path) { method: 'PATCH', credentials: 'include', headers: filterHeaders(request.headers), - body: JSON.stringify(await request.json()) + body: request.body, + // @ts-ignore, not standard, but supported by undici; enable re-streaming of request + duplex: 'half' }); } catch (err) { logger.debug(err); diff --git a/tests/v2/collect_mock_tasks.setup.js b/tests/v2/collect_mock_tasks.setup.js index 0de5dae7..c7cfeced 100644 --- a/tests/v2/collect_mock_tasks.setup.js +++ b/tests/v2/collect_mock_tasks.setup.js @@ -15,15 +15,6 @@ test('Collect mock tasks [v2]', async ({ page, request }) => { await waitPageLoading(page); }); - await test.step('Attempt to collect tasks with invalid path', async () => { - await page.getByText('Local', { exact: true }).click(); - await page.getByRole('textbox', { name: 'Package', exact: true }).fill('./foo'); - await page.getByRole('button', { name: 'Collect', exact: true }).click(); - await expect( - page.getByText('Local-package path must be a wheel file (given ./foo).') - ).toBeVisible(); - }); - if ((await page.getByRole('table').last().getByText('MIP_compound').count()) > 0) { console.warn('WARNING: Mock tasks V2 already collected. Skipping tasks collection'); return; @@ -36,16 +27,25 @@ test('Collect mock tasks [v2]', async ({ page, request }) => { const response = await request.get(tasksMockWheelUrl); expect(response.status()).toEqual(200); const body = await response.body(); - const tmpDir = path.resolve(os.tmpdir(), 'playwright'); - if (!fs.existsSync(tmpDir)) { - fs.mkdirSync(tmpDir); + const tasksMockWheelFolder = path.resolve(os.tmpdir(), 'playwright'); + if (!fs.existsSync(tasksMockWheelFolder)) { + fs.mkdirSync(tasksMockWheelFolder); } - tasksMockWheelFile = path.resolve(tmpDir, 'fractal_tasks_mock-0.0.1-py3-none-any.whl'); + tasksMockWheelFile = path.resolve( + tasksMockWheelFolder, + 'fractal_tasks_mock-0.0.1-py3-none-any.whl' + ); fs.writeFileSync(tasksMockWheelFile, body); }); await test.step('Collect mock tasks', async () => { - await page.getByRole('textbox', { name: 'Package', exact: true }).fill(tasksMockWheelFile); + await page.getByText('Local', { exact: true }).click(); + + const fileChooserPromise = page.waitForEvent('filechooser'); + await page.getByText('Upload a wheel file', { exact: true }).click(); + const fileChooser = await fileChooserPromise; + await fileChooser.setFiles(tasksMockWheelFile); + await page.getByRole('button', { name: 'Collect', exact: true }).click(); // Wait for Task collections table @@ -69,4 +69,9 @@ test('Collect mock tasks [v2]', async ({ page, request }) => { await test.step('Cleanup temporary wheel file', async () => { fs.rmSync(tasksMockWheelFile); }); + + await test.step('Attempt to collect tasks without a wheel file', async () => { + await page.getByRole('button', { name: 'Collect', exact: true }).click(); + await expect(page.getByText('Required field')).toBeVisible(); + }); });