Skip to content

Json parse error #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cyphercider opened this issue Jan 26, 2018 · 21 comments
Closed

Json parse error #17

cyphercider opened this issue Jan 26, 2018 · 21 comments

Comments

@cyphercider
Copy link

cyphercider commented Jan 26, 2018

Version: 1.0.2 (also applies to 1.0.3)
Dgraph version: 1.0.2

After running a query of the form via txn.query(...):

{
    classes(func: eq(<${sp.VertexType}>, "Class"), orderasc: ${sp.Name}) {
      uid: _uid_
      name: ${sp.Name}
      ...
}

Within your method Response.getJson(), the call to super.getJson() returns a string that is not valid Json:

"classes": [{"name":....

This results in a JSON parse error, "Unexpected token : in JSON at position 9"

Was there a recent change that would have changed the format of the string returned from super.getJson() so that it is missing an opening brace?

EDIT: the value returned from super.getJson() is determined to be Uint8 and gets put through the u8ToStr function.

EDIT 2: I have found that the last character returned from super.getJson() is ASCII character #26 ("SUBSTITUTE"). When I append a brace to the front of the result, and remove this "SUBSTITUTE" from the end, the string is valid JSON. So it looks like the query result may be incorrectly offset by 1 character.

@gpahal
Copy link
Contributor

gpahal commented Jan 27, 2018

Can you post the full query and response if it's not confidential? I am not able to reproduce this.

@cyphercider
Copy link
Author

Not a problem, I will do this first thing next week. I'm just now learning how the auto-generated protobuf code works and was planning to dig in a bit more there. If I'm not mistaken, the grpc module runs native code. I'm running the client on windows and accessing dgraph running on Ubuntu inside of VirtualBox. I'm not sure if the windows platform may have something to do with this.

Either way, I will post an exact query / response next week, and will try to dig further into the generated code to see if I can find out what is going on.

@cyphercider
Copy link
Author

Somehow I'm not able to repro this morning. Will reopen when / if it comes up again.

@roscioli
Copy link

roscioli commented Feb 2, 2018

I'm getting the same problem with 1.0.3

@roscioli
Copy link

roscioli commented Feb 2, 2018

Seems like the first { is missing and is breaking the JSON.parse in /lib/types.js line 80.

@cyphercider
Copy link
Author

@roscioli , I'm getting it again too, but I haven't bothered to re-open the issue until I can produce a solid repro. I'll re-open now though since you're also getting the issue.

Have you noticed a pattern with the volume of data you're returning in the queries that fail? The only pattern I've noticed so far is that the failures happen when the response payloads are large.

@cyphercider cyphercider reopened this Feb 2, 2018
@cyphercider
Copy link
Author

I've put this patch code in the library. I'm going to add some code to write the failed request / responses to the filesystem for repro purposes, but I haven't done it yet.

if(!jsonStr.startsWith('{')) {
	console.error('FYI - in patched code.  charcode at beginning: ' + jsonStr.charCodeAt(0) + ' and end: ' + jsonStr.charCodeAt(jsonStr.length - 1))
	jsonStr = ('{' + jsonStr)
	jsonStr = jsonStr.substr(0, jsonStr.length - 1)
}

@roscioli
Copy link

roscioli commented Feb 2, 2018

@tamethecomplex yes, on big data sets this is happening to me.

@roscioli
Copy link

roscioli commented Feb 2, 2018

That patch does work. Would be great to get a real fix in though.

@roscioli
Copy link

roscioli commented Feb 2, 2018

What is super curious though is that when I take the raw JSON string (which was throwing the error) and prepend { and run it through a JSON linter, the linter says it is fine JSON. However, when I updated the code with jsonStr = ('{' + jsonStr), the code was still getting a JSON parse error but this time it said there was an extra } at the end... Are there phantom characters or something or am I just going crazy?

@cyphercider
Copy link
Author

@roscioli yes definitely - what I found is that when the leading '{' is missing, the trailing character is ASCII character ID 26, "substitute". That character also broke the parse - so whenever I append the '{', I also have to remove the last character.

@gpahal
Copy link
Contributor

gpahal commented Feb 6, 2018

Has anyone been able to reproduce this? It seems the problem might be with the grpc server which will also affect other Dgraph clients. It's better to find and resolve the root cause.

@cyphercider
Copy link
Author

@gpahal , I just added some more logging that will allow me to provide a specific query / response for a repro. Will let you know when I have some good data. Probably by the end of the week.

@cyphercider
Copy link
Author

@gpahal , I was able to reproduce this issue. All I had to do was try to get a query result that has a sufficiently large response. The repro is available here: https://github.com/tamethecomplex/js_issue_repro/tree/master/dgraph-js/issue_17.

The error you're looking for us "Unexpected token :" being thrown from the response.getJson() method:

image

@emhagman
Copy link
Contributor

emhagman commented Feb 7, 2018

I am also having this issue on an array returning 9000+ items.

@gpahal
Copy link
Contributor

gpahal commented Feb 8, 2018

This issue is either caused by Dgraph server returning incorrect JSON string in grpc response or the grpc node library. I'll try to reproduce this issue and also use the Go client and raw HTTP to figure out what is happening.

@gpahal
Copy link
Contributor

gpahal commented Feb 9, 2018

There was a bug in a utility function. This error should not show in v1.1.1-beta.1. If you face problems with that version, please let me know.

@cyphercider
Copy link
Author

@gpahal , I will let you know, thank you!

@cyphercider
Copy link
Author

@gpahal , would it be possible for you to publish the beta to npm? Or I can clone and link if that is better for you. I didn't see either a branch or a tag for the beta.

@gpahal
Copy link
Contributor

gpahal commented Feb 10, 2018

It available on npm. Install it using npm install [email protected].

@cyphercider
Copy link
Author

Seems to be fixed now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants