JsonReport improvements

[filipesperandio]

Fix #19

[dblandin]

You might be able to rely on codeclimate/codeclimate's validation of issues instead of raising an exception here. I think emitting a null or missing location value would fail the issue validation. We have a specific error code for invalid issues. I think in this case we'd report this error as an unexpected engine error, which isn't as specific.

[filipesperandio]

@dblandin I am actually catching those exceptions here and printing warnings to stderr, but maybe let the CLI validation work would be more appropriate?

[dblandin]

Yeah, I think letting the CLI do its thing would be more accurate. We treat those errors differently within the platform.

[filipesperandio]

Cool, I will roll back this part.

Did some testing too, the CLI validation messages are pretty good, I didn't know! 👍

error: (CC::CLI::Analyze::EngineFailure) engine sonar-java failed with status 99 and stderr
engine produced invalid output: {:message=>"Location is not formatted correctly: location.lines or location.positions must be present: `{\"type\"=>\"issue\", \"check_name\"=>\"squid:S1994\", \"description\"=>\"This loop's stop condition tests \\\"k\\\" but the incrementer updates \\\"i\\\".\", \"content\"=>{\"body\"=>\"<p>It is almost always an error when a <code>for</code> loop's stop condition and incrementer don't act on the same variable. Even when it is not, it\\ncould confuse future maintainers of the code, and should be avoided.</p>\\n<h2>Noncompliant Code Example</h2>\\n<pre>\\nfor (i = 0; i &lt; 10; j++) {  // Noncompliant\\n  // ...\\n}\\n</pre>\\n<h2>Compliant Solution</h2>\\n<pre>\\nfor (i = 0; i &lt; 10; i++) {\\n  // ...\\n}\\n</pre>\"}, \"location\"=>{\"path\"=>\"java_lib/main/java/Library.java\"}, \"categories\"=>[\"Bug Risk\"]}`.", :issue=>{"type"=>"issue", "check_name"=>"squid:S1994", "description"=>"This loop's stop condition tests \"k\" but the incrementer updates \"i\".", "content"=>{"body"=>"<p>It is almost always an error when a <code>for</code> loop's stop condition and incrementer don't act on the same variable. Even when it is not, it\ncould confuse future maintainers of the code, and should be avoided.</p>\n<h2>Noncompliant Code Example</h2>\n<pre>\nfor (i = 0; i &lt; 10; j++) {  // Noncompliant\n  // ...\n}\n</pre>\n<h2>Compliant Solution</h2>\n<pre>\nfor (i = 0; i &lt; 10; i++) {\n  // ...\n}\n</pre>"}, "location"=>{"path"=>"java_lib/main/java/Library.java"}, "categories"=>["Bug Risk"]}}

[filipesperandio]

@dblandin @brynary Through the discussion here we decided to let the CLI validate the issue, the engine builds what it can. I came across this issue that has location.path but not location.lines which seems to be a valid sonar-issue, but the CLI does not like it, any advice?

error: (CC::CLI::Analyze::EngineFailure) engine sonar-java failed with status 99 and stderr
engine produced invalid output: {:message=>"Location is not formatted correctly: location.lines or location.positions must be present: `{\"type\"=>\"issue\", \"check_name\"=>\"squid:S1220\", \"severity\"=>\"minor\", \"description\"=>\"Move this file to a named package.\", \"content\"=>{\"body\"=>\"<p>According to the Java Language Specification:</p>\\n<blockquote>\\n  <p>Unnamed packages are provided by the Java platform principally for convenience when developing small or temporary applications or when just\\n  beginning development.</p>\\n</blockquote>\\n<p>To enforce this best practice, classes located in default package can no longer be accessed from named ones since Java 1.4.</p>\\n<h2>Noncompliant Code Example</h2>\\n<pre>\\npublic class MyClass { /* ... */ }\\n</pre>\\n<h2>Compliant Solution</h2>\\n<pre>\\npackage org.example;\\n\\npublic class MyClass{ /* ... */ }\\n</pre>\"}, \"location\"=>{\"path\"=>\"multiple_paths/Main.java\"}, \"categories\"=>[\"Bug Risk\"]}`.", :issue=>{"type"=>"issue", "check_name"=>"squid:S1220", "severity"=>"minor", "description"=>"Move this file to a named package.", "content"=>{"body"=>"<p>According to the Java Language Specification:</p>\n<blockquote>\n  <p>Unnamed packages are provided by the Java platform principally for convenience when developing small or temporary applications or when just\n  beginning development.</p>\n</blockquote>\n<p>To enforce this best practice, classes located in default package can no longer be accessed from named ones since Java 1.4.</p>\n<h2>Noncompliant Code Example</h2>\n<pre>\npublic class MyClass { /* ... */ }\n</pre>\n<h2>Compliant Solution</h2>\n<pre>\npackage org.example;\n\npublic class MyClass{ /* ... */ }\n</pre>"}, "location"=>{"path"=>"multiple_paths/Main.java"}, "categories"=>["Bug Risk"]}}

[filipesperandio]

The initial implementation was to set location.lines[0].begin = 1 and location.lines[0].end = 1, which seems appropriate for this issue, but I don't know if it gonna be always the case.

[dblandin]

Maybe we can add this fallback to known cases like the squid:S1220 check?

[filipesperandio]

@dblandin I am afraid there will be other cases, should we dig into all the rules?

[dblandin]

Probably not worth the time involved. What about adding a STDERR message so we have better visibility into when we're defaulting to a file-based location?

[filipesperandio]

i like that

[filipesperandio]

@dblandin Validation removed. Logic only does some "massage" to data, to not raise runtime errors (mostly NullPointers) but let the validation to the CLI.

[dblandin]

lgtm