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

soboko · 2019-01-03T01:45:14Z

🐛 Bug Fix

There's been an issue in SQLLab where numeric results appear inconsistently - sometimes they have quotes around them and sometimes they don't. This causes havoc with sorting, among other things.

This depends on whether the results come from a sync query or an async query. The logic in those two query handlers was handling numeric values inconsistently. In the case of sync queries, there's a potential loss of precision. In the case of sync queries, there are unwanted quote marks.

This PR mainly addresses the potential loss of precision by making sure that when a GET or POST is issued to SupersetClient (with a desired JSON response) the returned JSON contains BigNumbers. I'll submit a second PR to address the quoting issue (which is in the front end code in incubator-superset).

Notes:

I had to add a dependency on json-bigint (which handles the parsing).
Slight error-handling differences caused one of the existing unit tests to fail. I don't know if this makes the PR a non-starter but it's probably worth looking at that (parseResponse.test.ts).
I had to create a typescript declaration for json-bigint. I just added this to external.d.ts but I'm not sure if this is the proper place to do it. Maybe it's better to put somewhere where it can be used from other repos, or we might end up with code duplication...
I don't know if I should be commenting the code - I don't see any comments, so I didn't add any.
Please LMK if you see any other convention violations. :)

to: @conglei @williaster @kristw
cc: @john-bodley

…ion)

codecov · 2019-01-03T01:51:29Z

Codecov Report

Merging #71 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #71   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          67     67           
  Lines         734    738    +4     
  Branches      118    118           
=====================================
+ Hits          734    738    +4

Impacted Files	Coverage Δ
...uperset-ui-connection/src/callApi/parseResponse.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c48747...00e8cab. Read the comment docs.

williaster

Had a couple comments, happy you could get the repo set up and great first contribution overall!

packages/superset-ui-connection/package.json

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

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

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

williaster · 2019-01-03T02:02:12Z

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

@@ -1 +1,8 @@
 declare module 'fetch-mock';
+
+declare module 'json-bigint' {


@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.

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

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

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.

@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.

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.

… def

williaster · 2019-01-07T17:35:23Z

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

@@ -125,6 +126,7 @@ describe('SupersetClientClass', () => {
    it('does not set csrfToken if response is not json', () => {
      fetchMock.get(LOGIN_GLOB, '123', {
        overwriteRoutes: true,
+        // @ts-ignore


why does this one require a @ts-ignore?

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:

Upgrade to fetchMock v7 and fix the tests that become broken as a result

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.

Use this @ts-ignore annotation

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?

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 🤦‍♂️

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

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

williaster · 2019-01-08T23:10:04Z

hmm not sure why the travis branch is stuck 🤔

This reverts commit 2bcb1b1.

…-ui#71

… strings (#6591) * Fix: #6590, also depends on apache-superset/superset-ui#71 * Bumped version of superset-ui-connector * Added @babel/polyfill import * Reset mock history before each test, not after each test * Update CONTRIBUTING.md

…f precision) (#71)" This reverts commit e386612.

…oss of precision) (#71)" This reverts commit e386612.

…t loss of precision) (#71)" This reverts commit e386612.

…f precision) (#71)" (#126) * revert: revert "Handle BigNumber conversions in JSON properly (without loss of precision) (#71)" This reverts commit e386612. * fix: type errors * fix: typescript errors in superset-ui-demo

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

9c478d2

…ion)

soboko requested a review from a team as a code owner January 3, 2019 01:45

Bumped package version number

d214c05

williaster reviewed Jan 3, 2019

View reviewed changes

Address PR comments

0e93ac8

soboko mentioned this pull request Jan 3, 2019

Fix for #6590: Numeric values in columns sometimes returned as quoted strings apache/superset#6591

Merged

chris_council added 4 commits January 3, 2019 14:34

Rewrap "errors" from JSONbig.parse in proper error object.

5b4c893

Use DefinitelyTyped's fetch-mock type def, fleshed out JSONbig's type…

bdeceba

… def

Improved type def for json-bigint

e927fa1

Added type casts to some tests due to addition of fetchMock type def

1d6999c

williaster reviewed Jan 7, 2019

View reviewed changes

packages/superset-ui-connection/test/SupersetClientClass.test.ts Show resolved Hide resolved

Address code review comments

72174ae

chris_council added 2 commits January 8, 2019 17:23

Dummy commit, kick off Travis

2bcb1b1

Revert "Dummy commit, kick off Travis"

00e8cab

This reverts commit 2bcb1b1.

soboko merged commit e386612 into apache-superset:master Jan 9, 2019

soboko deleted the council--bignumberfix branch January 9, 2019 01:36

soboko pushed a commit to soboko/incubator-superset that referenced this pull request Jan 9, 2019

Part of fix for apache#6590, also depends on apache-superset/superset…

f55e181

…-ui#71

kristw added this to the v0.8.0 milestone Feb 1, 2019

xtinec added a commit that referenced this pull request Mar 29, 2019

Revert "Handle BigNumber conversions in JSON properly (without loss o…

7f49eb2

…f precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Mar 31, 2019

fix: revert "Handle BigNumber conversions in JSON properly (without l…

3cc7069

…oss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

fix: revert "Handle BigNumber conversions in JSON properly (without l…

9036716

…oss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

fix: revert "Handle BigNumber conversions in JSON properly (without l…

f02d176

…oss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

fix: revert "Handle BigNumber conversions in JSON properly (without l…

0c4c187

…oss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

fix: revert "Handle BigNumber conversions in JSON properly (without l…

94faf6f

…oss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

revert: revert "Handle BigNumber conversions in JSON properly (withou…

0be02c8

…t loss of precision) (#71)" This reverts commit e386612.

xtinec added a commit that referenced this pull request Apr 1, 2019

revert: revert "Handle BigNumber conversions in JSON properly (withou…

f1219e1

…t loss of precision) (#71)" This reverts commit e386612.

kristw added a commit that referenced this pull request Apr 17, 2020

fix: make line chart query operate in time series mode (#71)

b732022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

soboko commented Jan 3, 2019 •

edited

Loading

codecov bot commented Jan 3, 2019 •

edited

Loading

williaster left a comment

williaster Jan 3, 2019

xtinec Jan 4, 2019

williaster Jan 4, 2019

soboko Jan 4, 2019 •

edited

Loading

xtinec Jan 5, 2019

soboko Jan 8, 2019

williaster Jan 7, 2019

soboko Jan 8, 2019

williaster Jan 8, 2019

soboko Jan 8, 2019

williaster commented Jan 8, 2019

		@@ -1 +1,8 @@
		declare module 'fetch-mock';

		declare module 'json-bigint' {

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

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

Conversation

soboko commented Jan 3, 2019 • edited Loading

codecov bot commented Jan 3, 2019 • edited Loading

Codecov Report

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soboko Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster commented Jan 8, 2019

soboko commented Jan 3, 2019 •

edited

Loading

codecov bot commented Jan 3, 2019 •

edited

Loading

soboko Jan 4, 2019 •

edited

Loading