Fix: prevent crashing from JSON parsing error (fixes #10364) #10376

g-plane · 2018-05-18T07:04:30Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

close #10364

What changes did you make? (Give an overview)
Display better error message when failed to parse JSON.

platinumazure

Thanks for contributing!

The pretty error messages only show up when using the command line interface directly (and not, for example, when using CLIEngine). So I think it could be valuable to leave the original error messages (that is, set both message as well as messageTemplate/data)... What do you think?

g-plane · 2018-05-18T12:18:31Z

So what should the message looks like? The original error message looks like SyntaxError: Unexpected token } in JSON at position ...

platinumazure · 2018-05-18T12:21:57Z

I was thinking of the removal of line 117 in lib/config/config-file.js: we could leave that as is, but also set messageTemplate and data.

Looking again, I don't see any other removals of e.message = /*something*/ lines, so those are fine and there's no need to add anything.

g-plane · 2018-05-18T12:37:23Z

The reason I removed that line is, IMHO, it's duplicated because the old line contains e.message which equals to the original error message. However, on the other hand, the template just says failed to parse JSON instead of Cannot read config file(it tells the users something broken in config file instead of other JSON files).

And, I haven't tested it by using Node.js API CLIEngine. So maybe you are right.

What do you think?

platinumazure · 2018-05-18T13:14:48Z

@g-plane I think you're probably right that the message "Failed to parse JSON" is not super useful. But I also think "Syntax error: Unexpected token }" is not very useful either, hence the motivation for the message template that says failed to parse config file.

Maybe we should still wrap/change e.message, but with something close to what the template says?

If you get a chance to test this case in CLIEngine, hopefully you'll see what I'm getting at. 😄

g-plane · 2018-05-18T13:34:32Z

Ok, you are right. Wrapping the error message is friendly for CLIEngine.

Another question,

I also think "Syntax error: Unexpected token }" is not very useful either

Can we use babel-code-frame to solve this problem?

platinumazure · 2018-05-18T15:07:00Z

@g-plane

Can we use babel-code-frame to solve this problem?

Not sure what you mean. Could you please elaborate on this? Thanks!

g-plane · 2018-05-18T15:23:26Z

Here is a malformed json example: (missing comma at the end of line 2)

{
  "name": "test-eslint"
  "version": "0.0.0",
  "main": "index.js",
  "license": "Apache-2.0",
  "devDependencies": {
    "eslint-config-gplane": "^3.0.0-beta.1"
  }
}

When using JSON.parse to parse malformed json, the error message will be:

Unexpected string in JSON at position 28
SyntaxError: Unexpected string in JSON at position 28

That is the default message, but maybe we can use babel-code-frame to show the location and the result (it's just a string) will be like this:

 |  {
 |    "name": "test-eslint"
>|    "version": "0.0.0",
      ^
 |    "main": "index.js",
 |    "license": "Apache-2.0",
 |    "devDependencies": {
 |      "eslint-config-gplane": "^3.0.0-beta.1"
 |    }
 |  }

But there is a problem that JSON.parse only provide the position while babel-code-frame needs line number and column number. In other words, if we use babel-code-frame we must calculate and convert position to line number and column number.

There is also an alternative to get better error. It's to use json-parse-better-errors. Using this library, the error message will be:

SyntaxError: Unexpected string in JSON at position 28 while parsing '{
  "name": "test-eslint"
  "version": "'

g-plane · 2018-05-19T10:55:40Z

@platinumazure I have updated for lib/config/config-file.js.

platinumazure

This is looking really good! Just a few small typos.

Also: Could you add some assertions to some of the tests, to make sure messageTemplate/data.file/data.message are set correctly on the error objects? Thanks!

platinumazure · 2018-05-19T15:59:59Z

lib/config/config-file.js

+        e.messageTemplate = "failed-to-parse-json";
+        e.messageData = {
+            path: filePath,
+            messsage: e.messsage


s/messsage/message (twice)

platinumazure · 2018-05-19T16:00:12Z

lib/util/npm-util.js

+        e.messageTemplate = "failed-to-parse-json";
+        e.messageData = {
+            path: pkgJson,
+            messsage: e.messsage


s/messsage/message (twice)

g-plane · 2018-05-19T16:12:55Z

It seems that there are no such tests to check message template. (Not only this template but also other templates.)

platinumazure · 2018-05-19T16:16:59Z

Hi, apologies, I just meant adding any assertions to existing tests that check that the exception is thrown in the first place. On top of checking that the exception is thrown, we could inspect its properties. We don't need to test the rendering of the template. We just want to test that this exception is associated with a template... Hope that makes sense.

…

On Sat, May 19, 2018, 11:12 Pig Fang ***@***.***> wrote: It seems that there no such tests to check message template. (Not only this template but also other templates.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10376 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWesYakvF3O2ujuDykmyP5YLUsNcKlks5t0ESIgaJpZM4UEOs9> .

g-plane · 2018-05-19T17:03:21Z

Updated.

platinumazure

LGTM, thanks for adding those tests/assertions! Would like another set of eyes on this before merging.

not-an-aardvark

Thanks for the PR! I just have a minor comment about improving the error message, but this looks good aside from that.

not-an-aardvark · 2018-05-23T21:42:18Z

lib/config/config-file.js

@@ -115,6 +115,11 @@ function loadJSONConfigFile(filePath) {
    } catch (e) {
        debug(`Error reading JSON file: ${filePath}`);
        e.message = `Cannot read config file: ${filePath}\nError: ${e.message}`;
+        e.messageTemplate = "failed-to-parse-json";


I don't think the error here will necessarily be a JSON parsing error. This error-handling code will run when anything in JSON.parse(stripComments(readFile(filePath))) throws an error, which could include errors from the filesystem.

This could either be addressed by using different try/catch blocks for the different function calls (and providing a different error message as appropriate), or making the failed-to-parse-json error message more generic (e.g. "Failed to read JSON file" rather than "Failed to parse JSON file").

g-plane · 2018-05-24T03:41:36Z

@not-an-aardvark I have moved it to a separated try/catch block.

not-an-aardvark · 2018-05-24T03:45:23Z

Thanks! However, I think the same concern about the error message applies to all of the other try/catch blocks in this PR too. (Sorry, I should have been more clear originally.)

g-plane · 2018-05-24T04:42:50Z

@not-an-aardvark Review again?

platinumazure · 2018-05-28T13:04:15Z

Ping @not-an-aardvark: Have your concerns been addressed?

not-an-aardvark

LGTM, thanks! Sorry about the delay in re-reviewing.

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 18, 2018

aladdin-add added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 18, 2018

platinumazure reviewed May 18, 2018

View reviewed changes

platinumazure requested changes May 19, 2018

View reviewed changes

platinumazure approved these changes May 19, 2018

View reviewed changes

not-an-aardvark suggested changes May 23, 2018

View reviewed changes

Fix: prevent crashing from JSON parsing error (fixes #10364)

425b9ec

not-an-aardvark approved these changes May 28, 2018

View reviewed changes

platinumazure merged commit 3e9f33a into eslint:master May 28, 2018

g-plane deleted the fix-parsing-malformed-json branch May 29, 2018 04:40

eslint-deprecated bot locked and limited conversation to collaborators Nov 26, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: prevent crashing from JSON parsing error (fixes #10364) #10376

Fix: prevent crashing from JSON parsing error (fixes #10364) #10376

g-plane commented May 18, 2018

platinumazure left a comment

g-plane commented May 18, 2018

platinumazure commented May 18, 2018

g-plane commented May 18, 2018

platinumazure commented May 18, 2018

g-plane commented May 18, 2018 •

edited

platinumazure commented May 18, 2018

g-plane commented May 18, 2018

g-plane commented May 19, 2018

platinumazure left a comment

platinumazure May 19, 2018

platinumazure May 19, 2018

g-plane commented May 19, 2018 •

edited

platinumazure commented May 19, 2018 via email

g-plane commented May 19, 2018

platinumazure left a comment

not-an-aardvark left a comment

not-an-aardvark May 23, 2018

g-plane commented May 24, 2018

not-an-aardvark commented May 24, 2018

g-plane commented May 24, 2018

platinumazure commented May 28, 2018

not-an-aardvark left a comment

Fix: prevent crashing from JSON parsing error (fixes #10364) #10376

Fix: prevent crashing from JSON parsing error (fixes #10364) #10376

Conversation

g-plane commented May 18, 2018

platinumazure left a comment

Choose a reason for hiding this comment

g-plane commented May 18, 2018

platinumazure commented May 18, 2018

g-plane commented May 18, 2018

platinumazure commented May 18, 2018

g-plane commented May 18, 2018 • edited

platinumazure commented May 18, 2018

g-plane commented May 18, 2018

g-plane commented May 19, 2018

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure May 19, 2018

Choose a reason for hiding this comment

platinumazure May 19, 2018

Choose a reason for hiding this comment

g-plane commented May 19, 2018 • edited

platinumazure commented May 19, 2018 via email

g-plane commented May 19, 2018

platinumazure left a comment

Choose a reason for hiding this comment

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark May 23, 2018

Choose a reason for hiding this comment

g-plane commented May 24, 2018

not-an-aardvark commented May 24, 2018

g-plane commented May 24, 2018

platinumazure commented May 28, 2018

not-an-aardvark left a comment

Choose a reason for hiding this comment

g-plane commented May 18, 2018 •

edited

g-plane commented May 19, 2018 •

edited