GH-1905: Cleaned up the error handler.

Signed-off-by: Akos Kitta <kittaakos@gmail.com>

packages/file-download/package.json

@@ -10,6 +10,7 @@
     "@types/mime-types": "^2.1.0",
     "@types/rimraf": "^2.0.2",
     "@types/uuid": "^3.4.3",
+    "http-status-codes": "^1.3.0",
     "mime-types": "^2.1.18",
     "rimraf": "^2.6.2",
     "uuid": "^3.2.1",

packages/file-download/src/node/file-download-handler.ts

@@ -13,10 +13,11 @@ import { v4 } from 'uuid';
 import { lookup } from 'mime-types';
 import { Request, Response } from 'express';
 import { inject, injectable } from 'inversify';
+import { OK, METHOD_NOT_ALLOWED, PRECONDITION_FAILED, NOT_FOUND, INTERNAL_SERVER_ERROR } from 'http-status-codes';
 import URI from '@theia/core/lib/common/uri';
 import { ILogger } from '@theia/core/lib/common/logger';
 import { FileUri } from '@theia/core/lib/node/file-uri';
-import { FileSystem, FileStat } from '@theia/filesystem/lib/common/filesystem';
+import { FileSystem } from '@theia/filesystem/lib/common/filesystem';
 import { DirectoryZipper } from './directory-zipper';
 
 @injectable()
@@ -46,16 +47,21 @@ export abstract class FileDownloadHandler {
             await fs.access(filePath, fs.constants.W_OK);
             fs.readFile(filePath, (error, data) => {
                 if (error) {
-                    response.status(500).send(error).end();
+                    this.handleError(response, error, INTERNAL_SERVER_ERROR);
                     return;
                 }
-                response.status(200).send(data).end();
+                response.status(OK).send(data).end();
             });
         } catch (e) {
-            response.status(500).send(e).end();
+            this.handleError(response, e, INTERNAL_SERVER_ERROR);
         }
     }
 
+    protected async handleError(response: Response, reason: string | Error, status: number = INTERNAL_SERVER_ERROR): Promise<void> {
+        this.logger.warn(reason);
+        response.status(status).send(reason).end();
+    }
+
 }
 
 export namespace FileDownloadHandler {
@@ -69,48 +75,52 @@ export class SingleFileDownloadHandler extends FileDownloadHandler {
     async handle(request: Request, response: Response): Promise<void> {
         const { method, body, query } = request;
         if (method !== 'GET') {
-            this.logger.warn(`Unexpected HTTP method. Expected GET got '${method}' instead.`, request);
+            this.handleError(response, `Unexpected HTTP method. Expected GET got '${method}' instead.`, METHOD_NOT_ALLOWED);
             return;
         }
         if (body !== undefined) {
-            this.logger.warn(`The request body must undefined when downloading a single file. The body was: ${JSON.stringify(body)}.`);
+            this.handleError(response, `The request body must undefined when downloading a single file. The body was: ${JSON.stringify(body)}.`, PRECONDITION_FAILED);
             return;
         }
         if (query === undefined || query.uri === undefined || typeof query.uri !== 'string') {
-            this.logger.warn(`Cannot access the 'uri' query from the request. The query was: ${JSON.stringify(query)}.`);
+            this.handleError(response, `Cannot access the 'uri' query from the request. The query was: ${JSON.stringify(query)}.`, PRECONDITION_FAILED);
             return;
         }
         const uri = new URI(query.uri).toString(true);
         const stat = await this.fileSystem.getFileStat(uri);
         if (stat === undefined) {
-            this.logger.warn(`The file does not exist. URI: ${uri}.`);
-            response.status(404).send().end();
+            this.handleError(response, `The file does not exist. URI: ${uri}.`, NOT_FOUND);
             return;
         }
         try {
+            const filePath = FileUri.fsPath(uri);
             if (!stat.isDirectory) {
-                await this.download(FileUri.fsPath(uri), request, response);
+                await this.download(filePath, request, response);
             } else {
-                const outputPath = await this.zip(stat);
+                const outputPath = await this.zip(filePath);
                 await this.download(outputPath, request, response);
-                rimraf(outputPath, error => {
-                    if (error) {
-                        this.logger.warn(`An error occurred while cleaning up temporary data at: ${outputPath}.`, error);
-                    }
-                });
+                // Do not wait for the clean up.
+                this.deleteRecursively(outputPath);
             }
         } catch (e) {
-            response.status(500).send(e).end();
+            this.handleError(response, e, INTERNAL_SERVER_ERROR);
         }
     }
 
-    protected async zip(stat: FileStat): Promise<string> {
-        const inputPath = FileUri.fsPath(stat.uri);
+    protected async zip(inputPath: string): Promise<string> {
         const outputPath = path.join(os.tmpdir(), v4());
         await this.zipper.zip(inputPath, outputPath);
         return outputPath;
     }
 
+    protected async deleteRecursively(pathToDelete: string): Promise<void> {
+        rimraf(pathToDelete, error => {
+            if (error) {
+                this.logger.warn(`An error occurred while cleaning up temporary data. Cannot clean up: ${pathToDelete}.`, error);
+            }
+        });
+    }
+
 }
 
 @injectable()

yarn.lock

@@ -4342,6 +4342,10 @@ http-signature@~1.2.0:
     jsprim "^1.2.2"
     sshpk "^1.7.0"
 
+http-status-codes@^1.3.0:
+  version "1.3.0"
+  resolved "https://registry.yarnpkg.com/http-status-codes/-/http-status-codes-1.3.0.tgz#9cd0e71391773d0671b489d41cbc5094aa4163b6"
+
 https-browserify@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/https-browserify/-/https-browserify-1.0.0.tgz#ec06c10e0a34c0f2faf199f7fd7fc78fffd03c73"