Internal: Simplify and standarize server errors (Azure#695)

* Simplify server error * Simplify server error * update storage * Fix build * Fix some test * Fix some test * Fix more test * Fix * remove fdescribe * fix spec * fix tslint * Fix spec * Fix specs
davidkydd · Sep 21, 2017 · 831556c · 831556c
1 parent d8b63b5
commit 831556c
Show file tree

Hide file tree

Showing 25 changed files with 221 additions and 136 deletions.
diff --git a/app/components/account/details/storage-account-card/storage-account-card.html b/app/components/account/details/storage-account-card/storage-account-card.html
@@ -9,7 +9,7 @@ <h2 class="title">Storage account</h2>
                 <div *ngIf="storageAccount">
                     {{storageAccount.name}}
                 </div>
-                    <div *ngIf="!storageAccount && error" class="error" [title]="error.body?.error?.message">
+                    <div *ngIf="!storageAccount && error" class="error" [title]="error.message">
                     {{error.statusText}}
                 </div>
                 <div *ngIf="!storageAccount && !error">

diff --git a/app/components/base/form/server-error/server-error.component.ts b/app/components/base/form/server-error/server-error.component.ts
@@ -3,12 +3,6 @@ import { Component, Input } from "@angular/core";
 import { ServerError } from "app/models";
 import { log } from "app/utils";
 
-interface ErrorData {
-    message: string;
-    requestId: string;
-    time: string;
-}
-
 @Component({
     selector: "bl-server-error",
     templateUrl: "server-error.html",
@@ -21,43 +15,8 @@ export class ServerErrorComponent {
             log.error("Error passed to bl-server-error is not a server error.", error);
         }
         this._error = error;
-        this.errorData = this.parseErrorData();
     }
     public get error() { return this._error; }
 
-    public errorData: ErrorData;
-
     private _error: ServerError = null;
-
-    public parseErrorData(): ErrorData {
-        if (!this.error || !this.error.body.message) {
-            return {
-                message: null,
-                requestId: null,
-                time: null,
-            };
-        }
-        const value = this.error.body.message;
-        // Remove the request id from the the message
-        const lines = value.split("\n");
-        const message = lines.length > 0 ? lines[0] : null;
-        const requestId = lines.length > 1 ? this._parseRequestDetails(lines[1]) : null;
-        const time = lines.length > 2 ? this._parseRequestDetails(lines[2]) : null;
-
-        return {
-            message,
-            requestId,
-            time,
-        };
-    }
-
-    private _parseRequestDetails(value: string): string {
-        const data = value.split(":");
-        data.shift();
-        if (data.length > 1) {
-            return data.join(":");
-        } else {
-            return data[0];
-        }
-    }
 }
diff --git a/app/components/base/form/server-error/server-error.html b/app/components/base/form/server-error/server-error.html
@@ -2,26 +2,26 @@
     <div class="content">
         <div class="main-content">
             <div class="main-info">
-                <h4>{{error.body.code}}</h4>
-                <p>{{errorData.message}}</p>
+                <h4>{{error.code}}</h4>
+                <p>{{error.message}}</p>
             </div>
             <div class="troubleshoot-btn">
                 <i class="fa fa-bug" (click)="showErrorTroubleshoot = !showErrorTroubleshoot" title="Troubleshoot details"></i>
             </div>
         </div>
-        <div class="details" *ngIf="error.body?.values">
-            <div *ngFor="let detail of error.body.values"><b>{{detail.key}}</b>: {{detail.value}}</div>
+        <div class="details" *ngIf="error.details">
+            <div *ngFor="let detail of error.details"><b>{{detail.key}}</b>: {{detail.value}}</div>
         </div>
     </div>
     <div class="troubleshoot-info" *ngIf="showErrorTroubleshoot">
         <table>
-            <tr>
+            <tr *ngIf="error.requestId">
                 <td><b>RequestId</b></td>
-                <td>{{errorData.requestId}}</td>
+                <td>{{error.requestId}}</td>
             </tr>
-            <tr>
+            <tr *ngIf="error.timestamp">
                 <td><b>Time</b> </td>
-                <td>{{errorData.time}}</td>
+                <td>{{error.timestamp}}</td>
             </tr>
         </table>
     </div>

diff --git a/app/components/base/loading/loading.html b/app/components/base/loading/loading.html
@@ -6,7 +6,7 @@
         </div>
     </div>
     <div *ngSwitchCase="statuses.Error">
-        <div *ngIf="error" class="entity-not-found">{{error.body?.message}}</div>
+        <div *ngIf="error" class="entity-not-found">{{error.message}}</div>
     </div>
     <div class="loading-content" [class.loading]="(displayStatus | async) === statuses.Loading" #ref>
         <ng-content></ng-content>

diff --git a/app/components/file/browse/file-explorer/file-tree-view/file-tree-view.html b/app/components/file/browse/file-explorer/file-tree-view/file-tree-view.html
@@ -3,7 +3,7 @@
         <i class="fa" [class.fa-caret-down]="expanded" [class.fa-caret-right]="!expanded"></i>
     </bl-clickable>
     <bl-clickable class="name" (do)="handleClickTreeViewHeader()">{{name}}</bl-clickable>
-    <span class="error" *ngIf="fileNavigator.error" [title]="fileNavigator.error.body?.code">
+    <span class="error" *ngIf="fileNavigator.error" [title]="fileNavigator.error.code">
         <i class="fa fa-warning"></i>
     </span>
     <bl-clickable (do)="refresh()" [disabled]="refreshing">

diff --git a/app/components/file/details/file-details-view/file-details-view.component.ts b/app/components/file/details/file-details-view/file-details-view.component.ts
@@ -81,7 +81,7 @@ export class FileDetailsViewComponent implements OnChanges {
             error: (error: ServerError) => {
                 this.notificationService.error(
                     "Download failed",
-                    `${this.filename} failed to download. ${error.body.message}`,
+                    `${this.filename} failed to download. ${error.message}`,
                 );
             },
         });

diff --git a/app/components/file/details/log-file-viewer/log-file-viewer.component.ts b/app/components/file/details/log-file-viewer/log-file-viewer.component.ts
@@ -183,7 +183,7 @@ export class LogFileViewerComponent implements OnChanges, OnDestroy, AfterViewIn
         } else if (e.status === Constants.HttpCode.Conflict) {
             this.fileCleanupOperation = true;
             return;
-        } else if (!e.status && e.body.message.startsWith("An incorrect number of bytes")) {
+        } else if (!e.status && e.message.startsWith("An incorrect number of bytes")) {
             // gets an undefined error code for binary files.
             this.fileContentFailure = true;
             return;

diff --git a/app/components/pool/graphs/nodes-heatmap.component.ts b/app/components/pool/graphs/nodes-heatmap.component.ts
@@ -480,7 +480,7 @@ export class NodesHeatmapComponent implements AfterViewInit, OnChanges, OnDestro
                 this.nodeService.getOnce(this.pool.id, node.id);
             },
             error: (error: ServerError) => {
-                this.notificationService.error(error.body.code, error.body.message);
+                this.notificationService.error(error.code, error.message);
             },
         });
         return action;

diff --git a/app/components/task/details/output/task-outputs.component.ts b/app/components/task/details/output/task-outputs.component.ts
@@ -85,32 +85,26 @@ export class TaskOutputsComponent implements OnChanges {
         if (error.status === Constants.HttpCode.NotFound) {
             return new ServerError({
                 status: 404,
-                body: {
-                    code: "NodeNotFound",
-                    message: "The node the task ran on doesn't exists anymore or is in an invalid state.",
-                },
+                code: "NodeNotFound",
+                message: "The node the task ran on doesn't exists anymore or is in an invalid state.",
                 original: error.original,
             });
         } else if (error.status === Constants.HttpCode.Conflict) {
             return new ServerError({
                 status: 404,
-                body: {
-                    code: "TaskFilesDeleted",
-                    message: "The task files were cleaned from the node.",
-                },
+                code: "TaskFilesDeleted",
+                message: "The task files were cleaned from the node.",
                 original: error.original,
             });
         }
         return error;
     }
     private _processBlobError(error: ServerError): ServerError {
-        if (error.status === Constants.HttpCode.NotFound && error.body.code === "ContainerNotFound") {
+        if (error.status === Constants.HttpCode.NotFound && error.code === "ContainerNotFound") {
             return new ServerError({
                 status: 404,
-                body: {
-                    code: "NoPersistedOutput",
-                    message: this._fileConventionErrorMessage(),
-                },
+                code: "NoPersistedOutput",
+                message: this._fileConventionErrorMessage(),
                 original: error.original,
             });
         }

diff --git a/app/models/server-error.ts b/app/models/server-error.ts
@@ -4,75 +4,127 @@ import { BatchError } from "./batch-error";
 import { PythonPRpcError } from "./python-rpc-error";
 import { StorageError } from "./storage-error";
 
+interface ErrorDetail {
+    key?: string;
+    value?: string;
+}
+
 interface ServerErrorAttributes {
     status: number;
     statusText?: string;
-    body: ServerErrorBodyAttributes;
+    code: string;
+    message: string;
+    details?: ErrorDetail[];
+    requestId?: string;
+    timestamp?: Date;
     original?: any;
 }
 
-interface ServerErrorBodyAttributes {
-    code?: string;
-    message?: string;
-    requestId?: string;
-    values?: any[];
-    data?: any;
+function parseValueFromLine(line: string): string {
+    if (!line) { return null; }
+    const data = line.split(":");
+    data.shift();
+    if (data.length > 1) {
+        return data.join(":");
+    } else {
+        return data[0];
+    }
+}
+
+function parseRequestIdFromLine(line: string): string {
+    return parseValueFromLine(line);
 }
 
+function parseTimestampFromLine(line: string): Date {
+    const timeStr = parseValueFromLine(line);
+    if (!timeStr) { return null; }
+    return new Date(timeStr);
+}
+
+function parseMessage(fullMessage: string) {
+    if (!fullMessage) {
+        return { message: null, requestId: null, timestamp: null };
+    }
+    const lines = fullMessage.split("\n");
+    const message = lines[0];
+    const requestId = parseRequestIdFromLine(lines[1]);
+    const timestamp = parseTimestampFromLine(lines[2]);
+    return {
+        message,
+        requestId,
+        timestamp,
+    };
+}
 /**
  * Generic error structure for all api.
  * All their error format needs to be converted to this one so it can then be consumed easily
  */
 export class ServerError {
     public static fromBatch(error: BatchError) {
-        const body = {
-            code: error.code,
-            message: error.message && error.message.value,
-            values: error.body && error.body.values,
-        };
+        const { message, requestId, timestamp } = parseMessage(error.message && error.message.value);
 
         return new ServerError({
             status: error.statusCode,
-            body: body,
+            code: error.code,
+            details: error.body && error.body.values,
+            message,
+            requestId,
+            timestamp,
             original: error,
         });
     }
 
     public static fromStorage(error: StorageError) {
-        const body = {
-            code: error.code,
-            message: error.message,
-            requestId: error.requestId,
-            values: null,
-        };
+        const { message, timestamp } = parseMessage(error.message);
 
         return new ServerError({
             status: error.statusCode,
-            body: body,
             original: error,
+            code: error.code,
+            message: message,
+            requestId: error.requestId,
+            timestamp,
         });
     }
 
     public static fromARM(response: Response): ServerError {
-        const body = response.json();
+        const { error } = response.json();
+        let requestId = null;
+        let timestamp = null;
+        let code = null;
+        let message = null;
+
+        if (response.headers) {
+            requestId = response.headers.get("x-ms-request-id");
+            const date = response.headers.get("Date");
+            timestamp = date && new Date(date);
+        }
+
+        if (error) {
+            code = error.code;
+            message = error.message;
+        }
         return new ServerError({
             status: response.status,
+            code: code,
             statusText: response.statusText,
-            body: body,
             original: response,
+            message: message,
+            requestId,
+            timestamp,
         });
     }
 
     public static fromPython(error: PythonPRpcError) {
-        const body = {
-            code: ServerError._mapPythonErrorCode(error.code),
-            message: error.message,
-            data: error.data,
-        };
+        const { message, requestId, timestamp } = parseMessage(error.message);
 
         return new ServerError({
             status: error.data && error.data.status,
-            body: body,
+            code: ServerError._mapPythonErrorCode(error.code),
+            message,
+            details: error.data,
+            requestId,
+            timestamp,
             original: error,
         });
     }
@@ -90,23 +142,25 @@ export class ServerError {
 
     public status: number;
     public statusText: string;
-    public body: ServerErrorBodyAttributes;
+    public code: string;
     public message: string;
+    public details: ErrorDetail[];
+    public requestId: string;
+    public timestamp: Date;
     public original: any;
 
     constructor(attributes: ServerErrorAttributes) {
-        Object.assign(this, attributes);
-
-        const value = this.body.message;
-        if (value) {
-            // Remove the request id from the the message
-            const lines = value.split("\n");
-
-            this.message = lines.first();
-        }
+        this.status = attributes.status;
+        this.statusText = attributes.statusText;
+        this.code = attributes.code;
+        this.message = attributes.message;
+        this.details = attributes.details || [];
+        this.requestId = attributes.requestId;
+        this.timestamp = attributes.timestamp;
+        this.original = attributes.original;
     }
 
     public toString() {
-        return `${this.status} - ${this.body.message}`;
+        return `${this.status} - ${this.message}`;
     }
 }
diff --git a/app/services/application-service.ts b/app/services/application-service.ts
@@ -162,10 +162,8 @@ export class ApplicationService extends ServiceBase {
     /**
      * Check if the ServerError is an auto storage error from the application service
      */
-    public isAutoStorageError(error: any): boolean {
+    public isAutoStorageError(error: ServerError): boolean {
         const badCode = Constants.APIErrorCodes.accountNotEnabledForAutoStorage;
-        return error &&
-            (error.body.code === badCode ||
-                (error.body.error && error.body.error.code === badCode));
+        return error && error.code === badCode;
     }
 }