feat(rest): only parse requests on success (JS-13)

* OnmsResult is now only for non-error responses * response text (JSON or XML) is parsed just-in-time upon success * parse failures will throw an OnmsError rather than bubbling up or silently leaving data alone
OpenNMS · Aug 22, 2017 · aa9257d · aa9257d
1 parent 6f7a4ab
commit aa9257d
Show file tree

Hide file tree

Showing 8 changed files with 101 additions and 46 deletions.
diff --git a/src/api/OnmsResult.ts b/src/api/OnmsResult.ts
@@ -1,15 +1,8 @@
-import {OnmsError} from './OnmsError';
-
 /**
  * An [[IOnmsHTTP]] query result.
  * @module OnmsResult
  */
 export class OnmsResult<T> {
-  /** Create a new error result. */
-  public static error(message: string, code?: number) {
-    return new OnmsResult(undefined, message, code);
-  }
-
   /** Create a new success result. */
   public static ok(response: any, message?: string, code?: number, type?: string) {
     return new OnmsResult(response, message || 'OK', code || 200, type);

diff --git a/src/rest/AbstractHTTP.ts b/src/rest/AbstractHTTP.ts
@@ -4,6 +4,7 @@
 import {IOnmsHTTP} from '../api/IOnmsHTTP';
 import {IFilterProcessor} from '../api/IFilterProcessor';
 
+import {OnmsError} from '../api/OnmsError';
 import {OnmsHTTPOptions} from '../api/OnmsHTTPOptions';
 import {OnmsResult} from '../api/OnmsResult';
 import {OnmsServer} from '../api/OnmsServer';
@@ -83,6 +84,39 @@ export abstract class AbstractHTTP implements IOnmsHTTP {
     return xmlTransformer.transform(data);
   }
 
+  /** Attempt to extract the data from a response. */
+  protected getData(response: any) {
+    const type = this.getType(response);
+    if (type === 'json') {
+      return this.transformJSON(response.data);
+    } else if (type === 'xml') {
+      return this.transformXML(response.data);
+    } else {
+      return response.data;
+    }
+  }
+
+  /**
+   * Attempt to determine the type of response.
+   * @hidden
+   */
+  protected getType(response: any) {
+    if (response.headers['content-type'] === 'application/json') {
+      return 'json';
+    } else if (response.config.responseType === 'json') {
+      return 'json';
+    } else if (response.config.headers.accept === 'application/json') {
+      return 'json';
+    } else if (response.responseType === 'json') {
+      return 'json';
+    } else if (response.config.headers.accept === 'application/xml') {
+      return 'xml';
+    } else if (response.headers['content-type'] === 'application/xml') {
+      return 'xml';
+    }
+    return 'text';
+  }
+
   /**
    * Get the [[OnmsServer]] object that should be used for making requests.  Favors the one
    * passed in the [[OnmsHTTPOptions]], otherwise it falls back to the default server associated
@@ -122,17 +156,14 @@ export abstract class AbstractHTTP implements IOnmsHTTP {
   }
 
   /**
-   * A callback to handle any request errors.
+   * Create an [[OnmsError]] from an error response.
    * @hidden
    */
-  protected handleError(err: any, options?: any): never {
+  protected handleError(err: any, options?: any): OnmsError {
     const message = AbstractHTTP.extractMessage(err);
     const status = AbstractHTTP.extractStatus(err);
-    if (status) {
-      throw OnmsResult.error(message, status);
-    } else {
-      throw OnmsResult.error('An unknown error has occurred: ' + message);
-    }
+    const data = AbstractHTTP.extractData(err);
+    return new OnmsError(message, status, options, data);
   }
 
   /* tslint:disable:member-ordering */
@@ -162,7 +193,7 @@ export abstract class AbstractHTTP implements IOnmsHTTP {
    * @hidden
    */
   protected static extractStatus(err: any): number {
-    let status;
+    let status = -1;
     if (err.code) {
       status = err.code;
     } else if (err.status) {
@@ -173,4 +204,15 @@ export abstract class AbstractHTTP implements IOnmsHTTP {
     return status;
   }
 
+  /**
+   * Attempt to determine the data in an error response.
+   * @hidden
+   */
+  protected static extractData(err: any): any {
+    if (err && err.response && err.response.data) {
+      return err.response.data;
+    }
+    return undefined;
+  }
+
 }
diff --git a/src/rest/AxiosHTTP.ts b/src/rest/AxiosHTTP.ts
@@ -1,5 +1,5 @@
 import axios from 'axios';
-import {AxiosStatic, AxiosInstance, AxiosRequestConfig} from 'axios';
+import {AxiosStatic, AxiosInstance, AxiosResponse, AxiosRequestConfig} from 'axios';
 import * as qs from 'qs';
 import * as clonedeep from 'lodash.clonedeep';
 
@@ -70,8 +70,10 @@ export class AxiosHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
-    }).catch(this.handleError);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
+    }).catch((err) => {
+      throw this.handleError(err, opts);
+    });
   }
 
   /**
@@ -94,8 +96,10 @@ export class AxiosHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
-    }).catch(this.handleError);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
+    }).catch((err) => {
+      throw this.handleError(err, opts);
+    });
   }
 
   /**
@@ -117,8 +121,10 @@ export class AxiosHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
-    }).catch(this.handleError);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
+    }).catch((err) => {
+      throw this.handleError(err, opts);
+    });
   }
 
   /**
@@ -140,8 +146,10 @@ export class AxiosHTTP extends AbstractHTTP {
         if (response.headers && response.headers['content-type']) {
             type = response.headers['content-type'];
         }
-        return OnmsResult.ok(response.data, undefined, response.status, type);
-    }).catch(this.handleError);
+        return OnmsResult.ok(this.getData(response), undefined, response.status, type);
+    }).catch((err) => {
+      throw this.handleError(err, opts);
+    });
   }
 
   /**
@@ -160,7 +168,9 @@ export class AxiosHTTP extends AbstractHTTP {
   private getConfig(options?: OnmsHTTPOptions): AxiosRequestConfig {
     const allOptions = this.getOptions(options);
 
-    const ret = {} as AxiosRequestConfig;
+    const ret = {
+      transformResponse: [], // we do this so we can post-process only on success
+    } as AxiosRequestConfig;
 
     if (allOptions.auth && allOptions.auth.username && allOptions.auth.password) {
       ret.auth = {
@@ -187,15 +197,13 @@ export class AxiosHTTP extends AbstractHTTP {
     }
 
     const type = ret.headers.accept;
+    ret.transformResponse = [];
     if (type === 'application/json') {
       ret.responseType = 'json';
-      ret.transformResponse = this.transformJSON;
     } else if (type === 'text/plain') {
       ret.responseType = 'text';
-      delete ret.transformResponse;
     } else if (type === 'application/xml') {
       ret.responseType = 'text';
-      ret.transformResponse = this.transformXML;
     } else {
       throw new OnmsError('Unhandled "Accept" header: ' + type);
     }

diff --git a/src/rest/GrafanaHTTP.ts b/src/rest/GrafanaHTTP.ts
@@ -51,7 +51,7 @@ export class GrafanaHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
     }).catch((e) => {
       this.handleError(e, query);
     });
@@ -73,7 +73,7 @@ export class GrafanaHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
     }).catch((e) => {
       this.handleError(e, query);
     });
@@ -94,7 +94,7 @@ export class GrafanaHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
     }).catch((e) => {
       this.handleError(e, query);
     });
@@ -115,7 +115,7 @@ export class GrafanaHTTP extends AbstractHTTP {
       if (response.headers && response.headers['content-type']) {
         type = response.headers['content-type'];
       }
-      return OnmsResult.ok(response.data, undefined, response.status, type);
+      return OnmsResult.ok(this.getData(response), undefined, response.status, type);
     }).catch((e) => {
         this.handleError(e, query);
     });
@@ -140,7 +140,9 @@ export class GrafanaHTTP extends AbstractHTTP {
    */
   private getConfig(options?: OnmsHTTPOptions): any {
     const allOptions = this.getOptions(options);
-    const ret = {} as any;
+    const ret = {
+      transformResponse: [], // we do this so we can post-process only on success
+    } as any;
 
     if (allOptions.headers) {
       ret.headers = clonedeep(allOptions.headers);
@@ -158,13 +160,10 @@ export class GrafanaHTTP extends AbstractHTTP {
     const type = ret.headers.accept;
     if (type === 'application/json') {
       ret.responseType = 'json';
-      ret.transformResponse = this.transformJSON;
     } else if (type === 'text/plain') {
       ret.responseType = 'text';
-      delete ret.transformResponse;
     } else if (type === 'application/xml') {
       ret.responseType = 'text';
-      ret.transformResponse = this.transformXML;
     } else {
       throw new OnmsError('Unhandled "Accept" header: ' + type);
     }

diff --git a/src/rest/JsonTransformer.ts b/src/rest/JsonTransformer.ts
@@ -1,3 +1,5 @@
+import {OnmsError} from '../api/OnmsError';
+
 /**
  * Helper to transform a json string to an json object.
  */
@@ -11,7 +13,11 @@ export class JsonTransformer {
             if (data.length < 1) {
                 return {};
             } else {
-                return JSON.parse(data);
+                try {
+                    return JSON.parse(data);
+                } catch (err) {
+                    throw new OnmsError(err.message, undefined, undefined, data);
+                }
             }
         } else {
             // assume it's already parsed

diff --git a/src/rest/XmlTransformer.ts b/src/rest/XmlTransformer.ts
@@ -7,6 +7,8 @@ if (global && !global.window) {
     }
 }
 
+import {OnmsError} from '../api/OnmsError';
+
 /** @hidden */
 // tslint:disable-next-line
 const X2JS = require('x2js');
@@ -28,7 +30,11 @@ export class XmlTransformer {
      */
     public transform(data: any) {
         if (typeof data === 'string') {
-            return xmlParser.xml2js(data);
+            try {
+                return xmlParser.xml2js(data);
+            } catch (err) {
+                throw new OnmsError(err.message, undefined, undefined, data);
+            }
         } else {
             // assume it's already parsed
             return data;

diff --git a/test/rest/MockHTTP19.ts b/test/rest/MockHTTP19.ts
@@ -67,7 +67,7 @@ export class MockHTTP19 extends AbstractHTTP {
       }
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: GET ' + urlObj.toString()));
+    throw new Error('Not yet implemented: GET ' + urlObj.toString());
   }
 
   public put(url: string, options?: OnmsHTTPOptions) {
@@ -119,7 +119,7 @@ export class MockHTTP19 extends AbstractHTTP {
       }
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: PUT ' + urlObj.toString()));
+    throw new Error('Not yet implemented: PUT ' + urlObj.toString());
   }
 
   public post(url: string, options?: OnmsHTTPOptions) {
@@ -138,11 +138,11 @@ export class MockHTTP19 extends AbstractHTTP {
       */
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: POST ' + urlObj.toString()));
+    throw new Error('Not yet implemented: POST ' + urlObj.toString());
   }
 
   public httpDelete(url: string, options?: OnmsHTTPOptions): Promise<OnmsResult<any>> {
     const urlObj = new URI(url);
-    return Promise.reject(OnmsResult.error('Not yet implemented: DELETE ' + urlObj.toString()));
+    throw new Error('Not yet implemented: DELETE ' + urlObj.toString());
   }
 }
diff --git a/test/rest/MockHTTP21.ts b/test/rest/MockHTTP21.ts
@@ -79,7 +79,8 @@ export class MockHTTP21 extends AbstractHTTP {
         return Promise.resolve(result);
       }
     }
-    return Promise.reject(OnmsResult.error('Not yet implemented: ' + urlObj.toString()));
+
+    throw new Error('Not yet implemented: GET ' + urlObj.toString());
   }
 
   public put(url: string, options?: OnmsHTTPOptions) {
@@ -143,7 +144,7 @@ export class MockHTTP21 extends AbstractHTTP {
       }
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: PUT ' + urlObj.toString()));
+    throw new Error('Not yet implemented: PUT ' + urlObj.toString());
   }
 
   public post(url: string, options?: OnmsHTTPOptions) {
@@ -173,7 +174,7 @@ export class MockHTTP21 extends AbstractHTTP {
       }
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: POST ' + urlObj.toString()));
+    throw new Error('Not yet implemented: POST ' + urlObj.toString());
   }
 
   public httpDelete(url: string, options?: OnmsHTTPOptions): Promise<OnmsResult<any>> {
@@ -197,6 +198,6 @@ export class MockHTTP21 extends AbstractHTTP {
       }
     }
 
-    return Promise.reject(OnmsResult.error('Not yet implemented: DELETE ' + urlObj.toString()));
+    throw new Error('Not yet implemented: DELETE ' + urlObj.toString());
   }
 }