feat: Display schema parse errors.

[o0Ignition0o]

fixes #182

With the previous apollo-rs dependency bump, we are now able to provide a bit more context and details when a schema parse failed.

This commit displays the schema errors that have been noticed during parsing.

[o0Ignition0o]

     Running `target/debug/router -s ./examples/local.graphql`
Nov 15 11:41:30.511 ERROR apollo_router: Failed to read schema: Could not read schema: parse errors [
    ERROR@1716:1717 "Unexpected character" ;,
    ERROR@1702:1716 "expected definition" theiakjldfjsad,
    ERROR@1717:1723 "expected definition" fkjasf,
]
Nov 15 11:41:30.512  INFO router: Starting Apollo Router
*******************************************************************
⚠️  Experimental software, not YET recommended for production use ⚠️
*******************************************************************
Nov 15 11:41:30.513  INFO router: Stopped with error
Nov 15 11:41:30.513 ERROR router: Schema was not supplied.
Error: Schema was not supplied.

We can possibly iterate on this if / when we have a display impl for apollo_parser::Error, but this would be a nice first step for us to be able to work on supported features with users.

[cecton]

Keep using Display as it provides the doc at the same time than the error message

[o0Ignition0o]

the parser errors don't expose any impl display, do you want me to build an impl display for ParseErrors ?

[cecton]

no I meant displaydoc::Display

[cecton]

But eventually parse errors should implement Display indeed. Just not in this repository

[o0Ignition0o]

Oh I see, thanks for clearing it up!

[Geal]

since the error has span information, could we get the context too? Like https://github.com/zkat/miette

[o0Ignition0o]

@Geal @cecton This sounds like good ideas!

CCing @lrlna because I think they have something else in mind since the members are pub(crate) and there is no impl Display or Error.

[lrlna]

I haven't yet implemented display for errors, but I was going to use miette. Might be able to get you something workable this week!

[o0Ignition0o]

Super cool, marking this as draft, until we can do it right! Thanks again for your feedback @lrlna 🚀

[o0Ignition0o]

This looks super good! 🎉

[cecton]

Super cool PR!

Though I'm not sure you are handling this properly. This diagnostic library inserts colors to the terminal. This is fine in the terminal but not so great for the logs in a text file. I believe Display should still be free of any ansi codes and stuff.

What I would suggest is:

1. move this code to its own function: rename fmt() to print_pretty() or something
2. it doesn't need to take the formatter in argument, you can use stdout or stderr directly (the formatter is a requirement for Display but here we know that the output will has to be the tty)
3. You will need to change the main.rs to inspect the error you return and do a special handling for SchemaError::Parse

I know this sounds a lot more complicated but it will be more correct. The risk here is to sending logs with ansi codes somewhere by mistake.

[o0Ignition0o]

Hmm so what you mean is that we're conflating Error propagation and reporting, which is very true!

I wonder what responsibility should lie within Display and to the std::fmt::formatter. I would for example have liked to be able to ask the formatter if it supports colors.

[o0Ignition0o]

There might be a couple of clues https://docs.rs/miette/3.2.0/miette/trait.ReportHandler.html around here, I'll let you know what I've found!

[cecton]

I would for example have liked to be able to ask the formatter if it supports colors.

afaik this is unsupported. That is why I think you can't put ansi code in implementations of Debug and Display.

For example ToString depends on Display. Is a string supposed to have ansi codes like that?

But then really my concern is that we report errors through other channels (journald, syslog, etc...) and we incorrectly send ansi codes in them.

Again this is all fancy but we should make it clear that this targets only the terminal (aka stdout or stderr if and only if it is a tty, otherwise you need to strip the ansi codes).

[lrlna]

ohhh i did not know this is how you were going to send the error to the logs! I think the logs should just get the error slice, nothing should be formatted at all! Whatever reads the logs can do the formatting, or you can pipe the logs to a formatter.

[o0Ignition0o]

Well it's in flux really, We do show the logs to a console at the moment, so we might as well have nice ones. OTOH we do have log exporters as well, so I'll probably move the formating to the "console logging layer", I feel this is what @cecton is hinting at?

[cecton]

To be sure we understand each other:

1. Remove the {0} from the doc comment which will prevent this enum's Display to use the Display impl of ParseErrors. If the error gets logged, the user will have no other message than "Parsing error(s).". This is fine.

   /// Parsing error(s).
   Parse(ParseErrors),

2. Change this piece of code:

   impl std::fmt::Display for ParseErrors {
       fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
           // ...
       }
   }

   To this:

   impl ParseErrors {
       fn print_pretty(&self) {
           // ... also replace writeln!(f, ...) by eprintln!(...)
       }
   }

   So we don't end up with a Display implementation and a ToString implementation that renders ansi codes (things like this ESC[38;5; in the string. See ANSI escape codes).

3. In crates/apollo-router-core/src/schema.rs, instead of just simply returning the error, use your print_pretty function:

   if !tree.errors().is_empty() {
       let err = SchemaError::from_parse_errors(
           s.to_string(),
           tree.errors().to_vec(),
       );
       if isatty() {
           err.print_pretty();
       }
       return Err(err);

   You'll need this: isatty

[o0Ignition0o]

marking it as draft, until errors return an iterator

[cecton]

I'm unsubscribing for now. Ping me when you need a review again.

[o0Ignition0o]

This input:

schema
  @core(feature: "https://specs.apollo.dev/core/v0.1"),
  @core(feature: "https://specs.apollo.dev/join/v0.1")
{
  query: Query
  mutation: Mutation
}

directive @core(feature: String!) repeatable on SCHEMA
thisdoesntbelonghere
directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION

generates this output in a TTY:

and this output if not in a TTY:

Running `target/debug/router -s ./examples/supergraph.graphql`
ERROR@214:234 "expected definition" thisdoesntbelonghere
2021-12-06T15:33:58.217603Z ERROR apollo_router: Failed to read schema: Could not read schema: Parsing error(s).

I think this can be refactored and then reviewed.

[cecton]

Awesomely simple 👏