Improve parser error formatting

[goto-bus-stop]

ref https://github.com/apollographql/apollo-rs/pull/347#discussion_r1020325156

Right now parser errors show the index and the current token it's looking at, which isn't all that helpful to end users. IIRC Simon mentioned using the Python REPL to copy-paste the GraphQL source and slice it at the error index to get the context he needed.

ERROR@200:202 "Unterminated spread operator" ..

Maybe parser errors can use miette, like other parts of the project. We could even give suggestions in the future (like if we throw an "incomplete spread operator" error, we can suggest replacing it by a complete spread operator).

[lrlna]

yes! thank you for creating this issue! 🙏

I was thinking using miette reports for Display only, in case anyone using apollo-parser wants to use a different display library. In that case I think we can copy pretty much what the compiler does.

[goto-bus-stop]

we also talked about letting the router report detailed errors in a machine-readable format (i.e. JSON)

the graphql spec describes an error format: https://spec.graphql.org/October2021/#sec-Errors.Request-errors

we could add a conversion to json in that shape (or probably a struct that impls Serialize) for parse errors and validation errors.

[erikwrede]

Hey everyone, more summarized machine-readable details would definitely be helpful, especially those necessary to output spec compliant errors!

Is it already possible to extract the necessary information for a spec compliant format from the db.ast(file_id) query, or is this still a userland task? More specifically, the location: line, column part. As far as I could tell, the path field still has to be generated by traversal.

It might help to add the original parser syntax error to the SyntaxError ApolloDiagnostic, so the original info is available to consumers of functions like validate without having to traverse the AST again.

[goto-bus-stop]

The line, column is only available from the ApolloDiagnostic::SyntaxError at the moment (through miette, i'm actually not sure from memory if it has an API that exposes this). diagnostics IIRC don't have file paths at all at the moment. I want to address that very soon, this week or the next. It would likely use file IDs, and then you can use db.input(file_id).filename() to look up the name.

[erikwrede]

I couldn't find any public reference to get the line and column from browsing the Miette report docs. It seems like these reports are great for printing really pretty and helpful errors but extracting info is quite hard (maybe we should suggest adding that upstream? 🙂 ). It might be clearer to add the information to the diagnostics straight away. To clarify, I was referring to the Path of a Field corresponding to an AST Node that caused errors (e.g. query/MyType/myFieldWithErrors), not the file path.

In particular, the basics would be the following:

A locations list containing entries of:

• [ ] line: The line number of the Node with errors in the original file
• [ ] column: The line number of the Node with errors in the original file

Additionally:

• [ ] path: AST node names up to the Node with errors
• [ ] message: Message from the diagnostic The first two should already be accessible/generatable from the parser error's node index combined with the raw file.

Since these three fields are also helpful for validation errors, a generic structure might help here to streamline access. Edit: It seems like the SourceSpan type fields of ApolloDiagnostics can be used to get the location info. However, there are different source span fields on each type. Maybe a method error_locations on the ApolloDiagnostics enum could help with that?

[lrlna]

We are reworking a bit how our diagnostics are created in the compiler at the moment. It will allow us to add convenience methods for loc/column/path and will be a thiserror diagnostic first that we will add a Display for. Should be a lot easier!

You're using the compiler in rustberry, right? or just the parser?

[erikwrede]

Yes, we're using the compiler since we plan to use it for validation. Currently working on converting the parsed documents back to a GQL-Python compatible format and representing the errors. Since the issue mentioned GQL-Spec-style errors I thought this would be a good place to comment 🙂

Using thiserror sounds great as it interfaces nicely with miette, preserving those fancy console outputs while making the errors easier to process. Looking forward to seeing what you come up with ☺️