[Files] Use self destruct flag in upload component (#141366)

* added test

* remove assertion of calling delete, we do not do this anymore

* removed the deletion call, cleaned up unused vars and added a comment

* added the query schema to the route definition and rather use the built in http method for setting query parameters

* fixed files client tests

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

x-pack/plugins/files/public/components/upload_file/upload_state.test.ts

@@ -33,6 +33,36 @@ describe('UploadState', () => {
     testScheduler = getTestScheduler();
   });
 
+  it('calls file client with expected arguments', async () => {
+    testScheduler.run(({ expectObservable, cold, flush }) => {
+      const file1 = { name: 'test.png', size: 1 } as File;
+
+      uploadState.setFiles([file1]);
+
+      // Simulate upload being triggered async
+      const upload$ = cold('--a|').pipe(tap(uploadState.upload));
+
+      expectObservable(upload$).toBe('--a|');
+
+      flush();
+
+      expect(filesClient.create).toHaveBeenCalledTimes(1);
+      expect(filesClient.create).toHaveBeenNthCalledWith(1, {
+        kind: 'test',
+        meta: 'a',
+        mimeType: 'image/png',
+        name: 'test',
+      });
+      expect(filesClient.upload).toHaveBeenCalledTimes(1);
+      expect(filesClient.upload).toHaveBeenNthCalledWith(
+        1,
+        expect.objectContaining({
+          selfDestructOnAbort: true,
+        })
+      );
+    });
+  });
+
   it('uploads all provided files and reports errors', async () => {
     testScheduler.run(({ expectObservable, cold, flush }) => {
       const file1 = { name: 'test', size: 1 } as File;
@@ -132,7 +162,6 @@ describe('UploadState', () => {
         name: 'test 2',
       });
       expect(filesClient.upload).toHaveBeenCalledTimes(2);
-      expect(filesClient.delete).toHaveBeenCalledTimes(2);
     });
   });
 

x-pack/plugins/files/public/components/upload_file/upload_state.ts

@@ -162,7 +162,6 @@ export class UploadState {
     }
 
     let uploadTarget: undefined | FileJSON;
-    let erroredOrAborted = false;
 
     file$.setState({ status: 'uploading', error: undefined });
 
@@ -190,6 +189,7 @@ export class UploadState {
             id: uploadTarget.id,
             kind: this.fileKind.id,
             abortSignal,
+            selfDestructOnAbort: true,
             contentType: mime,
           })
         );
@@ -198,15 +198,9 @@ export class UploadState {
         file$.setState({ status: 'uploaded', id: uploadTarget?.id });
       }),
       catchError((e) => {
-        erroredOrAborted = true;
         const isAbortError = e.message === 'Abort!';
         file$.setState({ status: 'upload_failed', error: isAbortError ? undefined : e });
         return of(isAbortError ? undefined : e);
-      }),
-      finalize(() => {
-        if (erroredOrAborted && uploadTarget) {
-          this.client.delete({ id: uploadTarget.id, kind: this.fileKind.id });
-        }
       })
     );
   };
@@ -219,9 +213,7 @@ export class UploadState {
     const sub = this.abort$.subscribe(abort$);
     const upload$ = this.files$$.pipe(
       take(1),
-      switchMap((files$) => {
-        return forkJoin(files$.map((file$) => this.uploadFile(file$, abort$, meta)));
-      }),
+      switchMap((files$) => forkJoin(files$.map((file$) => this.uploadFile(file$, abort$, meta)))),
       map(() => undefined),
       finalize(() => {
         if (this.opts.allowRepeatedUploads) this.clear();
@@ -230,7 +222,7 @@ export class UploadState {
       shareReplay()
     );
 
-    upload$.subscribe();
+    upload$.subscribe(); // Kick off the upload
 
     return upload$;
   };

x-pack/plugins/files/public/files_client/files_client.test.ts

@@ -27,9 +27,7 @@ describe('apiRoutes', () => {
       `"/api/files/files/test/123"`
     );
 
-    expect(apiRoutes.getListRoute('test', 1, 1)).toMatchInlineSnapshot(
-      `"/api/files/files/test/list?page=1&perPage=1"`
-    );
+    expect(apiRoutes.getListRoute('test')).toMatchInlineSnapshot(`"/api/files/files/test/list"`);
 
     expect(apiRoutes.getByIdRoute('test', '123')).toMatchInlineSnapshot(
       `"/api/files/files/test/123"`
@@ -39,17 +37,13 @@ describe('apiRoutes', () => {
       `"/api/files/shares/test/123"`
     );
 
-    expect(apiRoutes.getListSharesRoute('test', 1, 1)).toMatchInlineSnapshot(
-      `"/api/files/shares/test?page=1&perPage=1"`
+    expect(apiRoutes.getListSharesRoute('test')).toMatchInlineSnapshot(`"/api/files/shares/test"`);
+
+    expect(apiRoutes.getPublicDownloadRoute('my-file.pdf')).toMatchInlineSnapshot(
+      `"/api/files/public/blob/my-file.pdf"`
     );
 
-    expect(apiRoutes.getPublicDownloadRoute('test', 'my-file.pdf')).toMatchInlineSnapshot(
-      `"/api/files/public/blob/my-file.pdf?token=test"`
-    );
-
-    expect(apiRoutes.getFindRoute(1, 1)).toMatchInlineSnapshot(
-      `"/api/files/find?page=1&perPage=1"`
-    );
+    expect(apiRoutes.getFindRoute()).toMatchInlineSnapshot(`"/api/files/find"`);
 
     expect(apiRoutes.getMetricsRoute()).toMatchInlineSnapshot(`"/api/files/metrics"`);
   });

x-pack/plugins/files/public/files_client/files_client.ts

@@ -5,8 +5,6 @@
  * 2.0.
  */
 
-import { pipe } from 'fp-ts/lib/function';
-import * as qs from 'query-string';
 import type { HttpStart } from '@kbn/core/public';
 import type { ScopedFilesClient, FilesClient } from '../types';
 import {
@@ -16,13 +14,6 @@ import {
   FILES_SHARE_API_BASE_PATH,
 } from '../../common/api_routes';
 
-const addQueryParams =
-  (queryParams: object) =>
-  (path: string): string => {
-    const stringified = qs.stringify(queryParams);
-    return `${path}${stringified ? `?${stringified}` : ''}`;
-  };
-
 /**
  * @internal
  */
@@ -36,32 +27,25 @@ export const apiRoutes = {
     `${FILES_API_BASE_PATH}/${fileKind}/${id}/blob${fileName ? '/' + fileName : ''}`,
   getUpdateRoute: (fileKind: string, id: string) => `${FILES_API_BASE_PATH}/${fileKind}/${id}`,
   getDeleteRoute: (fileKind: string, id: string) => `${FILES_API_BASE_PATH}/${fileKind}/${id}`,
-  getListRoute: (fileKind: string, page?: number, perPage?: number) => {
-    return pipe(`${FILES_API_BASE_PATH}/${fileKind}/list`, addQueryParams({ page, perPage }));
-  },
+  getListRoute: (fileKind: string) => `${FILES_API_BASE_PATH}/${fileKind}/list`,
   getByIdRoute: (fileKind: string, id: string) => `${FILES_API_BASE_PATH}/${fileKind}/${id}`,
 
   /**
    * Scope to file shares and file kind
    */
   getShareRoute: (fileKind: string, id: string) => `${FILES_SHARE_API_BASE_PATH}/${fileKind}/${id}`,
-  getListSharesRoute: (fileKind: string, page?: number, perPage?: number, forFileId?: string) =>
-    pipe(`${FILES_SHARE_API_BASE_PATH}/${fileKind}`, addQueryParams({ page, perPage, forFileId })),
+  getListSharesRoute: (fileKind: string) => `${FILES_SHARE_API_BASE_PATH}/${fileKind}`,
 
   /**
    * Public routes
    */
-  getPublicDownloadRoute: (token: string, fileName?: string) =>
-    pipe(
-      `${FILES_PUBLIC_API_BASE_PATH}/blob${fileName ? '/' + fileName : ''}`,
-      addQueryParams({ token })
-    ),
+  getPublicDownloadRoute: (fileName?: string) =>
+    `${FILES_PUBLIC_API_BASE_PATH}/blob${fileName ? '/' + fileName : ''}`,
 
   /**
    * Top-level routes
    */
-  getFindRoute: (page?: number, perPage?: number) =>
-    pipe(`${API_BASE_PATH}/find`, addQueryParams({ page, perPage })),
+  getFindRoute: () => `${API_BASE_PATH}/find`,
   getMetricsRoute: () => `${API_BASE_PATH}/metrics`,
 };
 
@@ -118,8 +102,10 @@ export function createFilesClient({
     getById: ({ kind, ...args }) => {
       return http.get(apiRoutes.getByIdRoute(scopedFileKind ?? kind, args.id));
     },
-    list({ kind, ...args } = { kind: '' }) {
-      return http.get(apiRoutes.getListRoute(scopedFileKind ?? kind, args.page, args.perPage));
+    list({ kind, page, perPage } = { kind: '' }) {
+      return http.get(apiRoutes.getListRoute(scopedFileKind ?? kind), {
+        query: { page, perPage },
+      });
     },
     update: ({ kind, id, ...body }) => {
       return http.patch(apiRoutes.getUpdateRoute(scopedFileKind ?? kind, id), {
@@ -127,8 +113,9 @@ export function createFilesClient({
         body: JSON.stringify(body),
       });
     },
-    upload: ({ kind, abortSignal, contentType, ...args }) => {
+    upload: ({ kind, abortSignal, contentType, selfDestructOnAbort, ...args }) => {
       return http.put(apiRoutes.getUploadRoute(scopedFileKind ?? kind, args.id), {
+        query: { selfDestructOnAbort },
         headers: {
           'Content-Type': contentType ?? 'application/octet-stream',
         },
@@ -152,12 +139,16 @@ export function createFilesClient({
       return http.get(apiRoutes.getShareRoute(scopedFileKind ?? kind, id));
     },
     listShares: ({ kind, forFileId, page, perPage }) => {
-      return http.get(
-        apiRoutes.getListSharesRoute(scopedFileKind ?? kind, page, perPage, forFileId)
-      );
+      return http.get(apiRoutes.getListSharesRoute(scopedFileKind ?? kind), {
+        query: { page, perPage, forFileId },
+      });
     },
     find: ({ page, perPage, ...filterArgs }) => {
-      return http.post(apiRoutes.getFindRoute(page, perPage), {
+      return http.post(apiRoutes.getFindRoute(), {
+        query: {
+          page,
+          perPage,
+        },
         headers: commonBodyHeaders,
         body: JSON.stringify(filterArgs),
       });
@@ -166,7 +157,7 @@ export function createFilesClient({
       return http.get(apiRoutes.getMetricsRoute());
     },
     publicDownload: ({ token, fileName }) => {
-      return http.get(apiRoutes.getPublicDownloadRoute(token, fileName));
+      return http.get(apiRoutes.getPublicDownloadRoute(fileName), { query: { token } });
     },
   };
   return api;

x-pack/plugins/files/server/routes/file_kind/upload.ts

@@ -89,6 +89,7 @@ export function register(fileKindRouter: FileKindRouter, fileKind: FileKind) {
         validate: {
           body: bodySchema,
           params: paramsSchema,
+          query: querySchema,
         },
         options: {
           tags: fileKind.http.create.tags,