Improve JSON body parsing error reporting

[robozati]

Closes #2056.

This add more checks on list_value, object_value and object_element to account for empty elements, reporting that there is one more comma in the first two functions and an empty element in the last. Makes sense because these errors are very common, and the user most likely forgot that there is one more comma in the first two cases.

Problem is that I don't know if you would like to change ParserError::Json to ParserError::Json(AnotherEnum), so I changed one existing member (Unexpected) that wasn't used in the rest of the code to account for unexpected commas and added another member, ExpectedAnElementInJson. All the three have the same description message, so there is repetition.

Before this patch:

POST http://foo.com
{
  "toto": {
    "a": "b",
    "c": "d",
  }
}

error: Parsing JSON
  --> /tmp/json.hurl:3:10
   |
 3 |   "toto": {
   |          ^ JSON error
   |

After:

error: Parsing JSON
  --> /tmp/json_test.hurl:5:13
   |
 5 |     "c": "d",
   |             ^ unexpected character: ','
   |

Also:

POST http://foo.com
{
  "toto": {
    "a": "b",
    "c": "d",
    "e": [1, 2,]
  }
}

error: Parsing JSON
  --> /tmp/json_test.hurl:6:15
   |
 6 |     "e": [1, 2,]
   |               ^ unexpected character: ','
   |

And:

POST http://foo.com
{
  "toto": {
    "a": "b",
    "c": "d",
    "e":
  }
}

error: Parsing JSON
  --> /tmp/json_test.hurl:6:9
   |
 6 |     "e":
   |         ^ expecting an element; found empty element instead
   |

[robozati]

I had a few problems with starting the server for the integration tests, so it took some time to fix them (despite none of the issues being in tests that needed the servers).

Reinforcing what I said in the comment above, I don't know if you want to change the ParserError::Json enum, so I am open to a possible decision.

Moreover, all the possible user errors catch techniques added in this PR degrade performance, obv. almost nothing in a modern computer so it's next to irrelevant, but I still think that generating better error messages is more important.

When I fixed the errors I also added another check when braces are unclosed, so there is this error message:

error: Parsing JSON
  --> tests_error_parser/json.hurl:2:1
   |
 2 | { "name":
   | ^ this brace is not closed later
   |

[jcamiel]

Hi @robozati

Thanks for your PR. We just need some time to review it, @fabricereix will look at it next week (he's on holiday), as he's implemented the JSON parser.

Don't hesitate to send us any issues/difficulties that you encounter (with how to launch integration tests for instance). We can improve the docs also if things are not well explained.

[fabricereix]

I also prefer to stick to one only variant ParserError::Json for any JSON parsing, and use another enum within this variant.

[fabricereix]

From the changes in current integ test

error: Parsing JSON
  --> tests_error_parser/json_unexpected_character.hurl:2:10
   |
 2 | { "name": x
   |           ^ JSON error

The error message could be improved to expecting a boolean, integer, ...
but I still find that the error position is correct.

I don't really find that the new error message is more relevant

 2 | { "name": x
   | ^ this brace is not closed later

Adding a closing brace will still produce an invalid JSON

[robozati]

I changed ParserError::Json to use a JsonErrorVariant enum.

Also, I added a new test to account for names that could not be resolved, as your last code snippet. So the json below now fails with:

   |
 2 | { "name": x}
   |           ^ failed to resolve 'x'. If it's a variable, try enclosing it with double braces, e.g. {{x}}
   |

If there is only punctuation:

   |
 2 | { "name": ,}
   |           ^ expecting an element; found empty element instead
   |

However, the test below still produces the same error. @fabricereix are you ok with this, or do you want to first check if there is a a valid element, then check for the braces?

 2 | { "name": x
   | ^ this brace is not closed later

EDIT: I forgot to run cargo fmt on parser/mod.rs, but I am planning to rebase all the commits into one in the end of the review, so this isn't important for now.

[fabricereix]

I have updated the issue with expected error messages. We can add them as integration tests.
They can be applied to both object and list.

tests_error_parser/json_object_trailing_comma.hurl

 5 |       "c": "d",   }
   |               ^  trailing comma is not allowed
   |

tests_error_parser/json_list_trailing_comma.hurl

 5 |       "c", "d",  ]
   |               ^  trailing comma is not allowed
   |

tests_error_parser/json_object_expecting_element.hurl (replaces json_unexpected_character.hurl)

 2 | { "name": x
   |           ^ expecting a boolean, number, string, array, object or null

tests_error_parser/json_list_expecting_element.hurl

 2 | [ "name", x
   |           ^ expecting a boolean, number, string, array, object or null

[robozati]

Okay, will do this later today or tomorrow and open this PR again.

[robozati]

@fabricereix I made the changes that you asked, but due to expecting an element having greater precedence to unclosed braces, I removed the unclosed braces check because it doesn't do nothing anymore. Ex.:

POST http://localhost:8000/data
{ "name":
HTTP 200

  --> tests_error_parser/json.hurl:3:1
   |
 3 | HTTP 200
   | ^ expecting a boolean, number, string, array, object or null
   |

This is because the parser looks for an element on HTTP, but this isn't an element, so the expecting element error fires.

[fabricereix]

/accept

[hurl-bot]

🕗 /accept is running, please wait for completion.

[hurl-bot]

🔨 Auto rebase from Orange-OpenSource/hurl/master succeeds, robozati/hurl/update-json-parser now embeds these commits:

• 80e35e1 Update crates
• 6ec1286 Update changelog.
• c72f04f Remove xmltree/indexmap dependency.
• 842a167 Add DOM XML parser based on xml-rs.
• 521966e Update crates
• 7313b61 Update crates
• f3d46ec Update crates
• 60e095a Update crates
• 7c1753e Update crates
• 8c0bec3 Fix mypy errors.
• 9612100 Update Python dependencies.
• 18da7ca Update Flask 3.0.0
• 0757d5e Add failed test in HTML/JUnit/TAP reports
• 20982f4 Report failures with nok
• 406da0c Update crates

[hurl-bot]

🕗 /accept is running, please wait for completion.

[hurl-bot]

✅ Pull request merged and closed by fabricereix with fast forward merge..

# List of commits merged from robozati/hurl/update-json-parser branch into Orange-OpenSource/hurl/master branch:

• 97a02f3 Update the JSON parser to account for fabricereix changes
• e0d9bb9 Change ParserError::Json to use a JsonErrorVariant enum and add a new check to account for unresolved names
• 678aaca Improve JSON body parsing error reporting