-
Notifications
You must be signed in to change notification settings - Fork 389
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
JSON parser throws exception if JSON is malformed #36
Conversation
JSON could be malformed.
Shouldn't this result in an error? |
Hmm well, the XML and YAML parsers are both wrapped in a try/catch block. Why not the JSON parser? |
should they also return an error then? if parsing the data fails when the content-type states that this is the format, shouldn't you be notified that the content is not valid json/xml/yaml? |
Yes I agree, that would be ideal. |
possible fix: lalitkapoor@3e9bd13 (just did it for the json parser) |
@lalitkapoor I like your fix and I think it's good practice to catch the error and pass it as a first argument to the callback! I'd love to see that fix pulled into upstream although it breaks backwards compatibility with previous versions since the parameter order of the callback has changed... Would you mind sending a pull request? |
@ctavan hey, I found node-wwwdude to be more reliable and more compliant: https://github.com/pfleidi/node-wwwdude note that the author of node-wwwdude hasn't published the updated version with my patches to wwwdude (proper handling of compressed data) to npmjs, so you'll have to pull from github to get the latest. |
@lalitkapoor thanks for the hint, I'm definitely gonna give it a try! |
I really like lalitkapoor's implementation to pass an error in. But like it was mentioned it isn't backwards compatible. Should we just bite the bullet and change the API? |
If a fix like this gets implemented it would automatically fix issue #47 as well. |
@lalitkapoor @ctavan I created branch 0.3.x specifically for this fix as it makes a lot of sense to do this. Upping the minor version to 0.3.x will allow those on 0.2.x to stay compatible until they're ready to upgrade. Let me know if any of you are willing to implement a more legit implementation for it -- most likely using @lalitkapoor 's pull request as a starting point. |
I have a 3rd party REST service returning invalid JSON. Creating a custom json parser works fine but perhaps a new option jsonParseValidation=true|false could force it to return the data ASIS as shown below in the parsers function around line 412. // ORIGINAL code that returns an error if JSON parse fails |
Currently, the parser just assumes that the input is valid JSON.
Great library BTW.