Merge pull request #669 from fractal-analytics-platform/task-collecti…

…on-form-data

Used form data in tasks collection endpoint

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);

__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' })
 			})
 		);
 	});

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',

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 @@
 <div>
 	<form on:submit|preventDefault={handleTaskCollection}>
 		<div class="row">
-			<div
-				class="mb-2"
-				class:col-md-6={packageType === 'pypi'}
-				class:col-md-12={packageType === 'local'}
-			>
-				<div class="input-group has-validation">
-					<div class="input-group-text">
-						<label class="font-monospace" for="package">Package</label>
+			{#if packageType === 'pypi'}
+				<div class="mb-2 col-md-6">
+					<div class="input-group has-validation">
+						<div class="input-group-text">
+							<label class="font-monospace" for="package">Package</label>
+						</div>
+						<input
+							name="package"
+							id="package"
+							type="text"
+							class="form-control"
+							required
+							class:is-invalid={$validationErrors['package']}
+							bind:value={python_package}
+						/>
+						<span class="invalid-feedback">{$validationErrors['package']}</span>
 					</div>
-					<input
-						name="package"
-						id="package"
-						type="text"
-						class="form-control"
-						required
-						class:is-invalid={$validationErrors['package']}
-						bind:value={python_package}
-					/>
-					<span class="invalid-feedback">{$validationErrors['package']}</span>
+					<div class="form-text">The name of a package published on PyPI</div>
 				</div>
-				<div class="form-text">
-					{#if packageType === 'pypi'}
-						The name of a package published on PyPI
-					{:else}
-						The full path to a wheel file
-					{/if}
+			{:else if packageType === 'local'}
+				<div class="mb-2 col-md-6">
+					<div class="input-group has-validation">
+						<label for="wheelFile" class="input-group-text">
+							<i class="bi bi-file-earmark-arrow-up" /> &nbsp; Upload a wheel file
+						</label>
+						<input
+							class="form-control"
+							accept=".whl"
+							type="file"
+							name="wheelFile"
+							id="wheelFile"
+							bind:this={wheelFileInput}
+							bind:files={wheelFiles}
+							class:is-invalid={$validationErrors['file']}
+						/>
+						{#if wheelFiles && wheelFiles.length > 0}
+							<button class="btn btn-outline-secondary" on:click={clearWheelFileUpload}>
+								Clear
+							</button>
+						{/if}
+						<span class="invalid-feedback">{$validationErrors['file']}</span>
+					</div>
 				</div>
-			</div>
+			{/if}
 			{#if packageType === 'pypi'}
 				<div class="col-md-6 mb-2">
 					<div class="input-group has-validation">

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);

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();
+	});
 });