Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

Handle BigNumber conversions in JSON properly (without loss of precision) #71

Merged
soboko merged 10 commits into from
Jan 9, 2019
Merged

Handle BigNumber conversions in JSON properly (without loss of precision) #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9c478d2
Handle BigNumber conversions in JSON properly (without loss of precis…
Jan 3, 2019
d214c05
Bumped package version number
Jan 3, 2019
0e93ac8
Address PR comments
Jan 3, 2019
5b4c893
Rewrap "errors" from JSONbig.parse in proper error object.
Jan 3, 2019
bdeceba
Use DefinitelyTyped's fetch-mock type def, fleshed out JSONbig's type…
Jan 4, 2019
e927fa1
Improved type def for json-bigint
Jan 4, 2019
1d6999c
Added type casts to some tests due to addition of fetchMock type def
Jan 5, 2019
72174ae
Address code review comments
Jan 8, 2019
2bcb1b1
Dummy commit, kick off Travis
Jan 9, 2019
00e8cab
Revert "Dummy commit, kick off Travis"
Jan 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/superset-ui-connection/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
14 changes: 13 additions & 1 deletion packages/superset-ui-connection/src/callApi/parseResponse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import JSONbig from 'json-bigint';
import { ParseMethod, SupersetClientResponse } from '../types';

function rejectIfNotOkay(response: Response): Promise<Response> {
Expand All @@ -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',
Expand All @@ -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}'.`);
Expand Down
52 changes: 49 additions & 3 deletions packages/superset-ui-connection/test/SupersetClientClass.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: '' });
Expand Down Expand Up @@ -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
Copy link
Contributor

@williaster williaster Jan 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this one require a @ts-ignore?

Copy link
Contributor Author

@soboko soboko Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's complicated...

We're using fetchMock v6.5.2, but type def 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 I opted to use the type def file for 6.0.2. It looks like sendAsJson was added sometime after that release, or else the type def file isn't completely accurate.

I can see a few possible solutions:

  1. Upgrade to fetchMock v7 and fix the tests that become broken as a result
  2. Create a fetchMock v6.5.2 type def file, include a local copy in our repo, and submit a PR to DefinitelyTyped. Would involve examining the fetchMock source and release notes to make sure that all API changes from 6.0.2 -> 6.5.2 are included.
  3. Use this @ts-ignore annotation
  4. Revert - i.e. don't use any type def file, go back to just declaring the module

I didn't want to invest the time into #1 or #2 without discussion, but I also didn't want to lose the type def information, so I opted for #3 for now.

Thoughts?

Copy link
Contributor

@williaster williaster Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I'm cool with #3 if we also add a @TODO comment for what is needed to get rid of it.

otherwise #1 seems next best / hopefully not many breaking changes, but understand that becomes a lot of yak-shaving 🤦‍♂️

Copy link
Contributor Author

@soboko soboko Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I stuck with #3 for now and added comments + an @TODO

sendAsJson: false,
});

Expand Down Expand Up @@ -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' });
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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 });
Expand All @@ -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]));
Expand All @@ -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]));
Expand Down
8 changes: 4 additions & 4 deletions packages/superset-ui-connection/test/callApi/callApi.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]));
Expand All @@ -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();

Expand All @@ -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]));
Expand Down
2 changes: 1 addition & 1 deletion packages/superset-ui-connection/test/callApi/parseResponse.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
31 changes: 30 additions & 1 deletion packages/superset-ui-connection/types/external.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,30 @@
declare module 'fetch-mock';
declare module 'json-bigint' {
Copy link
Contributor

@williaster williaster Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xtinec do you have any thoughts on this type of declaration? it looks okay to me, not sure if we can be more specific than any.

Copy link
Contributor

@xtinec xtinec Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do BigNumber | string here instead of any.

Looks like json-bigint doesn't have a type definition on DefinitelyTyped but we can add one for it pretty easily (it only has two exported methods and we already wrote the type for one of them here 🙂). Then, we could install our new type definition as a dependency. @soboko would you be interested in doing it?

P.S. Unrelated but we could also add the type def for fetch-mock which is already available on DefinitelyTyped @williaster

Copy link
Contributor

@williaster williaster Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let us know what you think about this @soboko, this could be done in a separate PR if you want to get this merged.

Copy link
Contributor Author

@soboko soboko Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used any as the return type for consistency with the return type of JSON.parse. I did just add the second method and stole some comments from lib.es5.d.ts...

I agree that adding a type def for json-bigint to DefinitelyTyped is the right thing to do and I can certainly do that. Looks like it could take up to a week for that PR to be accepted. Perhaps we could still merge this PR for the time being? There's a PR in incubator-superset that's dependent on this one.

I also added the type def for fetch-mock in my latest commit.

Copy link
Contributor

@xtinec xtinec Jan 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soboko you're absolutely right. any is the right return type. I totally missed the switch in https://github.com/sidorares/json-bigint/blob/master/lib/parse.js#L330 and thought it was only meant to be used for parsing numbers. My bad. 😊

I agree we could merge this PR while we get the type def going for DefinitelyTyped.

Copy link
Contributor Author

@soboko soboko Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I looked into creating a proper type def file for DefinitelyTyped, and there's a bit of a complication.

Specifically, the API for json-bigint looks like:

module.exports = function(options) {
    return  {
        parse: json_parse(options),
        stringify: json_stringify
    }
};
//create the default method members with no options applied for backwards compatibility
module.exports.parse = json_parse();
module.exports.stringify = json_stringify;

The idea is that you can construct a parser with non-default options using something like:

var JSONstrict = require('json-bigint')({"strict": true});

AFAICT there's no clean way to reconcile the json-bigint API with ES6/TypeScript-style imports and type definition files.

I've submitted a PR that would allow using something like the following to construct a "special" JSONbig parser:

const JSONbigspecial = JSONbig.newInstance({ storeAsString: true, strict: true });

If that PR is merged, then I can submit a decent type declaration file to DefinitelyTyped and problem solved.

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;
}