apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
576 stars 45 forks source link

Migrate ApolloDiagnostic to new API #756

Closed goto-bus-stop closed 10 months ago

goto-bus-stop commented 11 months ago

With https://github.com/apollographql/apollo-rs/pull/747, we should port the compiler validation errors to use DiagnosticReport.

It may be reasonable to have two error types, one for executable validation rules and one for schema validation rules. However there is some significant overlap between the two (eg. in argument validation for directives).

We can do the new diagnostics more similarly to how the BuildErrors are done in the high-level representations. They are enums with data, and labelling and formatting is delayed until the errors are actually printed. For a server use case, this is much better, as labels will typically be thrown out (aside from the main message).

#[derive(thiserror::Error)]
enum ExecutableValidationError {
    // main error location stored in the `validation::DiagnosticData` type
    #[error("variable ${name} is not defined")]
    UndefinedVariable { name: ast::Name },
    UniqueArgument {
        name: ast::Name,
        first_use: Option<NodeLocation>, // locations for additional labels can be stored here
    }
}
impl ExecutableValidationError {
    // to be called from the Diagnostic / DiagnosticList wrappers
    // this is where we'd actually use location information to attach labels
    fn report(&self, SourceMap) -> DiagnosticReport;
}

// extend existing private `Details` type
enum Details {
    ...
    ExecutableBuildError(ExecutableBuildError),
    ExecutableValidationError(ExecutableValidationError),
}

We can address https://github.com/apollographql/apollo-rs/issues/691 at the same time.

SimonSapin commented 11 months ago

If we end up building a larger enum with both anyway, what would be the benefit of having separate enums for schema errors and executable doc errors?

SimonSapin commented 11 months ago

Build errors being separate enums is left over from when they were stored as-is inside Schema and ExecutableDocument, they could (should?) be inlined into Details too.

goto-bus-stop commented 11 months ago

Hm it's a good point 🤔 I think I got to that because I imagined executable.validate() would return only that enum as its error type, but that's not how the DiagnosticList currently works, and maybe doesn't actually have strong real-world benefits. It would be #[non_exhaustive] anyways so the way end-users interact with it would be the same.