Handle BigNumber conversions in JSON properly (without loss of precis…

…ion) (#71)

* Handle BigNumber conversions in JSON properly (without loss of precision)
* Rewrap "errors" from JSONbig.parse in proper error object.
* Use DefinitelyTyped's fetch-mock type def
* Improved type def for json-bigint
* Added type casts to some tests due to addition of fetchMock type def

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/package.json

@@ -26,12 +26,14 @@
   },
   "homepage": "https://github.com/apache-superset/superset-ui#readme",
   "devDependencies": {
+    "@types/fetch-mock": "^6.0.0",
     "fetch-mock": "^6.5.2",
     "node-fetch": "^2.2.0"
   },
   "dependencies": {
     "@babel/runtime": "^7.1.2",
-    "whatwg-fetch": "^2.0.4"
+    "whatwg-fetch": "^2.0.4",
+    "json-bigint": "^0.3.0"
   },
   "publishConfig": {
     "access": "public"

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/src/callApi/parseResponse.ts

@@ -1,3 +1,4 @@
+import JSONbig from 'json-bigint';
 import { ParseMethod, SupersetClientResponse } from '../types';
 
 function rejectIfNotOkay(response: Response): Promise<Response> {
@@ -6,6 +7,15 @@ function rejectIfNotOkay(response: Response): Promise<Response> {
   return Promise.resolve<Response>(response);
 }
 
+function parseJson(text: string): any {
+  try {
+    return JSONbig.parse(text);
+  } catch (e) {
+    // if JSONbig.parse fails, it throws an object (not a proper Error), so let's re-wrap the message.
+    throw new Error(e.message);
+  }
+}
+
 export default function parseResponse(
   apiPromise: Promise<Response>,
   parseMethod: ParseMethod = 'json',
@@ -17,7 +27,9 @@ export default function parseResponse(
   } else if (parseMethod === 'text') {
     return checkedPromise.then(response => response.text().then(text => ({ response, text })));
   } else if (parseMethod === 'json') {
-    return checkedPromise.then(response => response.json().then(json => ({ json, response })));
+    return checkedPromise.then(response =>
+      response.text().then(text => ({ json: parseJson(text), response })),
+    );
   }
 
   throw new Error(`Expected parseResponse=null|json|text, got '${parseMethod}'.`);

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/test/SupersetClientClass.test.ts

@@ -1,9 +1,22 @@
 import fetchMock from 'fetch-mock';
 
+import { BigNumber } from 'bignumber.js';
 import { SupersetClientClass, ClientConfig } from '../src/SupersetClientClass';
 import throwIfCalled from './utils/throwIfCalled';
 import { LOGIN_GLOB } from './fixtures/constants';
 
+/* NOTE: We're using fetchMock v6.5.2, but corresponding fetchMock type declaration files are only available for v6.0.2
+ * and v7+. It looks like there are behavior changes between v6 and v7 that break our tests, so upgrading to v7 is
+ * probably some work.
+ *
+ * To avoid this, we're using the type declarations for v6.0.2, but there is at least one API inconsistency between that
+ * type declaration file and the actual library we're using. It looks like `sendAsJson` was added sometime after that
+ * release, or else the type declaration file isn't completely accurate. To get around this, it's necessary to add
+ * a `@ts-ignore` decorator before references to `sendAsJson` (there's one instance of that in this file).
+ *
+ * The **right** solution is probably to upgrade to fetchMock v7 (and the latest type declaration) and fix the tests
+ * that become broken as a result.
+ */
 describe('SupersetClientClass', () => {
   beforeAll(() => {
     fetchMock.get(LOGIN_GLOB, { csrf_token: '' });
@@ -125,6 +138,8 @@ describe('SupersetClientClass', () => {
     it('does not set csrfToken if response is not json', () => {
       fetchMock.get(LOGIN_GLOB, '123', {
         overwriteRoutes: true,
+        // @TODO remove once fetchMock is upgraded to 7+, see note at top of this file
+        // @ts-ignore
         sendAsJson: false,
       });
 
@@ -246,7 +261,8 @@ describe('SupersetClientClass', () => {
     const mockGetUrl = `${protocol}//${host}${mockGetEndpoint}`;
     const mockPostUrl = `${protocol}//${host}${mockPostEndpoint}`;
     const mockTextUrl = `${protocol}//${host}${mockTextEndpoint}`;
-    const mockTextJsonResponse = '{ "value": 9223372036854775807 }';
+    const mockBigNumber = '9223372036854775807';
+    const mockTextJsonResponse = `{ "value": ${mockBigNumber} }`;
 
     fetchMock.get(mockGetUrl, { json: 'payload' });
     fetchMock.post(mockPostUrl, { json: 'payload' });
@@ -315,6 +331,21 @@ describe('SupersetClientClass', () => {
         );
       });
 
+      it('supports parsing a response as JSON while preserving precision of large numbers', () => {
+        expect.assertions(2);
+        const client = new SupersetClientClass({ protocol, host });
+
+        return client.init().then(() =>
+          client.get({ url: mockTextUrl }).then(({ json }) => {
+            expect(fetchMock.calls(mockTextUrl)).toHaveLength(1);
+            // @ts-ignore
+            expect(json.value.toString()).toBe(new BigNumber(mockBigNumber).toString());
+
+            return Promise.resolve();
+          }),
+        );
+      });
+
       it('supports parsing a response as text', () => {
         expect.assertions(2);
         const client = new SupersetClientClass({ protocol, host });
@@ -424,6 +455,21 @@ describe('SupersetClientClass', () => {
         );
       });
 
+      it('supports parsing a response as JSON while preserving precision of large numbers', () => {
+        expect.assertions(2);
+        const client = new SupersetClientClass({ protocol, host });
+
+        return client.init().then(() =>
+          client.post({ url: mockTextUrl }).then(({ json }) => {
+            expect(fetchMock.calls(mockTextUrl)).toHaveLength(1);
+            // @ts-ignore
+            expect(json.value.toString()).toBe(new BigNumber(mockBigNumber).toString());
+
+            return Promise.resolve();
+          }),
+        );
+      });
+
       it('supports parsing a response as text', () => {
         expect.assertions(2);
         const client = new SupersetClientClass({ protocol, host });
@@ -446,7 +492,7 @@ describe('SupersetClientClass', () => {
 
         return client.init().then(() =>
           client.post({ url: mockPostUrl, postPayload }).then(() => {
-            const formData = fetchMock.calls(mockPostUrl)[0][1].body;
+            const formData = fetchMock.calls(mockPostUrl)[0][1].body as FormData;
             expect(fetchMock.calls(mockPostUrl)).toHaveLength(1);
             Object.keys(postPayload).forEach(key => {
               expect(formData.get(key)).toBe(JSON.stringify(postPayload[key]));
@@ -464,7 +510,7 @@ describe('SupersetClientClass', () => {
 
         return client.init().then(() =>
           client.post({ url: mockPostUrl, postPayload, stringify: false }).then(() => {
-            const formData = fetchMock.calls(mockPostUrl)[0][1].body;
+            const formData = fetchMock.calls(mockPostUrl)[0][1].body as FormData;
             expect(fetchMock.calls(mockPostUrl)).toHaveLength(1);
             Object.keys(postPayload).forEach(key => {
               expect(formData.get(key)).toBe(String(postPayload[key]));

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/test/callApi/callApi.test.ts

@@ -82,7 +82,7 @@ describe('callApi()', () => {
         expect(calls).toHaveLength(1);
 
         const fetchParams = calls[0][1];
-        const { body } = fetchParams;
+        const body = fetchParams.body as FormData;
 
         Object.keys(postPayload).forEach(key => {
           expect(body.get(key)).toBe(JSON.stringify(postPayload[key]));
@@ -102,7 +102,7 @@ describe('callApi()', () => {
         expect(calls).toHaveLength(1);
 
         const fetchParams = calls[0][1];
-        const { body } = fetchParams;
+        const body = fetchParams.body as FormData;
         expect(body.get('key')).toBe(JSON.stringify(postPayload.key));
         expect(body.get('noValue')).toBeNull();
 
@@ -129,8 +129,8 @@ describe('callApi()', () => {
         const calls = fetchMock.calls(mockPostUrl);
         expect(calls).toHaveLength(2);
 
-        const stringified = calls[0][1].body;
-        const unstringified = calls[1][1].body;
+        const stringified = calls[0][1].body as FormData;
+        const unstringified = calls[1][1].body as FormData;
 
         Object.keys(postPayload).forEach(key => {
           expect(stringified.get(key)).toBe(JSON.stringify(postPayload[key]));

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/test/callApi/parseResponse.test.ts

@@ -61,7 +61,7 @@ describe('parseResponse()', () => {
       .catch(error => {
         expect(fetchMock.calls(mockTextUrl)).toHaveLength(1);
         expect(error.stack).toBeDefined();
-        expect(error.message.includes('Unexpected token')).toBe(true);
+        expect(error.message.includes('Unexpected')).toBe(true);
 
         return Promise.resolve();
       });

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/types/external.d.ts

@@ -1 +1,30 @@
-declare module 'fetch-mock';
+declare module 'json-bigint' {
+  interface JSONbig {
+    /**
+     * Converts a JavaScript Object Notation (JSON) string into an object, preserving precision for numeric values.
+     * @param text A valid JSON string.
+     * @param reviver A function that transforms the results. This function is called for each member of the object.
+     * If a member contains nested objects, the nested objects are transformed before the parent object is.
+     */
+    parse(text: string, reviver?: (key: any, value: any) => any): any;
+
+    /**
+     * Converts a JavaScript value to a JavaScript Object Notation (JSON) string, preserving precision for numeric values.
+     * @param value A JavaScript value, usually an object or array, to be converted.
+     * @param replacer A function that transforms the results, or an array of strings and numbers that acts
+     *                 as a approved list for selecting the object properties that will be stringified.
+     * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
+     */
+    stringify(
+      value: any,
+      replacer?: (number | string)[] | null | ((key: string, value: any) => any),
+      space?: string | number,
+    ): string;
+  }
+
+  /**
+   * An intrinsic object that provides functions to convert JavaScript values to and from the JavaScript Object Notation (JSON) format.
+   */
+  const JSONbig: JSONbig;
+  export = JSONbig;
+}