Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API refactor to make it harder to ignore errors #752

Merged
merged 5 commits into from
Nov 23, 2023
Merged

API refactor to make it harder to ignore errors #752

merged 5 commits into from
Nov 23, 2023

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Nov 18, 2023

Fixes #709

BREAKING

  • ast::Document, Schema, and ExecutableDocument not longer contain potential errors that users need to check separately.
  • Instead, various constructors and methods now return a Result, with the Err case containing both both errors and a maybe-incomplete value.
  • Change validate methods of Schema and ExecutableDocument to take ownership of self. On success they return the schema or document (unmodified) wrapped in a Valid<_> marker type, which is immutable.
  • Change ExecutableDocument to require a &Valid<Schema> instead of &Schema, forcing callers to either run validation or opt out explicitly with Valid::assert_valid.
  • Make parse_mixed and to_mixed validate both the schema and document. Rename them with a _validate suffix.
  • Corresponding changes to all of the above in Parser method signatures
  • Remove ast::Document::check_parse_errors: parse errors are now encoded in the return value of parse.
  • Remove ast::Document::to_schema_builder. Use SchemaBuilder::add_ast instead.
  • Move items from the crate top-level to apollo_compiler::validation:
    • Diagnostic
    • DiagnosticList
    • FileId
    • NodeLocation
  • Move items from the crate top-level to apollo_compiler::execution:
    • GraphQLError
    • GraphQLLocation
  • Remove warning-level and advice-level diagnostics. See Consider re-introducing lints to apollo-rs #751

Highlight of signature changes:

+struct Valid<T>(T); // Implements `Deref` and `AsRef` but not `DerefMut` or `AsMut`
+
+struct WithErrors<T> {
+    partial: T, // Errors may cause components to be missing
+    errors: DiagnosticList,
+}

-pub fn parse_mixed(…) -> (Schema, ExecutableDocument)
+pub fn parse_mixed_validate(…)
+    -> Result<(Valid<Schema>, Valid<ExecutableDocument>), DiagnosticList>

  impl ast::Document {
-    pub fn parse(…) -> Self
+    pub fn parse(…) -> Result<Self, WithErrors<Self>>

-    pub fn to_schema(&self) -> Schema
+    pub fn to_schema(&self) -> Result<Schema, WithErrors<Schema>>

-    pub fn to_executable(&self) -> ExecutableDocument
+    pub fn to_executable(&self) -> Result<ExecutableDocument, WithErrors<ExecutableDocument>>

-    pub fn to_mixed(&self) -> (Schema, ExecutableDocument)
+    pub fn to_mixed_validate(
+        &self,
+    ) -> Result<(Valid<Schema>, Valid<ExecutableDocument>), DiagnosticList>
  }

  impl Schema {
-    pub fn parse(…) -> Self
-    pub fn validate(&self) -> Result<DiagnosticList, DiagnosticList>

+    pub fn parse_and_validate(…) -> Result<Valid<Self>, WithErrors<Self>>
+    pub fn parse(…) -> Result<Self, WithErrors<Self>>
+    pub fn validate(self) -> Result<Valid<Self>, WithErrors<Self>>
  }

  impl SchemaBuilder {
-    pub fn build(self) -> Schema
+    pub fn build(self) -> Result<Schema, WithErrors<Schema>>
  }

  impl ExecutableDocument {
-    pub fn parse(schema: &Schema, …) -> Self
-    pub fn validate(&self, schema: &Schema) -> Result<(), DiagnosticList>

+    pub fn parse_and_validate(schema: &Valid<Schema>, …) -> Result<Valid<Self>, WithErrors<Self>>
+    pub fn parse(schema: &Valid<Schema>, …) -> Result<Self, WithErrors<Self>>
+    pub fn validate(self, schema: &Valid<Schema>) -> Result<Valid<Self>, WithErrors<Self>>
  }

Features

  • Add parse_and_validate constructors for Schema and ExecutableDocument: when mutating isn’t needed after parsing, this returns an immutable Valid<_> value in one step.

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Nov 20, 2023

TODO: add this escape hatch:

impl<T> Valid<T> {
    pub fn assert_valid(value: T) -> Self {
        Self(value)
    }
}

@SimonSapin SimonSapin merged commit 64610dd into main Nov 23, 2023
11 checks passed
@SimonSapin SimonSapin deleted the results branch November 23, 2023 07:34
@SimonSapin SimonSapin assigned SimonSapin and unassigned SimonSapin Nov 23, 2023
@goto-bus-stop goto-bus-stop mentioned this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It’s too easy to use a schema or document with errors
2 participants