Reduce scope of execution / introspection APIs (#944)

The `execution` module was sort of aiming for comprehensive handling of GraphQL execution. For example, `execution::Response` was able to represent any possible GraphQL response including `extensions` `enum ResponseData { Absent, Null, Object(JsonMap) }`. However a real-world GraphQL server is unlikely to be satisfied with using that external type as its only representation of responses. In practice Apollo Router has a struct that carries additional data not serialized to JSON.

This redesigns the public API and reduce it to what’s actually needed to support the main functionality we want to provide: introspection.

Additionally, the introspection "splitting" functionality is removed, as Apollo Router doesn’t use it and I don’t have great confidence it’s correct. Splitting a document into two `Valid<ExecutableDocument>`s requires keeping track of which variables and fragments are used where, since producing a document with unused definitions would make it invalid.
This is tricky to get right, and not particularly useful. Instead we now have introspection execution take the original unmodified document and ignore the fields it doesn’t know how to execute. It’s up to the caller to execute those fields separately and merge two partial responses.

New public API below, replacing `apollo_compiler::execution`. See doc-comments in the PR diff.

`apollo_compiler::request` module:

```rust
pub fn coerce_variable_values(
    schema: &Valid<Schema>,
    operation: &Operation,
    values: &JsonMap,
) -> Result<Valid<JsonMap>, RequestError> {…}

#[derive(Debug, Clone)]
pub struct RequestError {…}

impl RequestError {
    // Should this return a plain `&str`?
    // It’s a `String` underneath atm but we may want something more structured later
    pub fn message(&self) -> impl Display + '_ {…}
    pub fn location(&self) -> Option<SourceSpan> {…}
    pub fn to_graphql_error(&self, sources: &SourceMap) -> GraphQLError {…}
}
```

`apollo_compiler::response` module:

```rust
pub use serde_json_bytes;
pub type JsonValue = serde_json_bytes::Value;
pub type JsonMap = serde_json_bytes::Map<serde_json_bytes::ByteString, JsonValue>;

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct ExecutionResponse {
    pub errors: Vec<GraphQLError>,
    // Intentionally cannot represent request error (absent `data`), `Result` is used instead:
    pub data: Option<JsonMap>,
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct GraphQLError {
    pub message: String,
    pub locations: Vec<LineColumn>,
    pub path: Vec<ResponseDataPathSegment>,
    // Set to `{"APOLLO_SUSPECTED_VALIDATION_BUG": true}` for cases that "should never happen" in valid documents
    pub extensions: JsonMap,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub enum ResponseDataPathSegment {
    Field(crate::Name),
    ListIndex(usize),
}

impl GraphQLError {
    pub fn new(
        message: impl Into<String>,
        location: Option<SourceSpan>,
        sources: &SourceMap,
    ) -> Self {…}
}
```

`apollo_compiler::introspection` module:

```rust
pub fn check_max_depth(
    document: &Valid<ExecutableDocument>,
    operation: &Operation,
) -> Result<(), RequestError> {…}

pub fn partial_execute(
    schema: &Valid<Schema>,
    implementers_map: &HashMap<Name, Implementers>,
    document: &Valid<ExecutableDocument>,
    operation: &Operation,
    variable_values: &Valid<JsonMap>,
) -> Result<ExecutionResponse, RequestError> {…}
```

Fixes #937

Author: SimonSapin

crates/apollo-compiler/CHANGELOG.md

@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## BREAKING
 
+- **Reduce scope of execution / introspection APIs - [SimonSapin], [pull/944]**
 - **Move `apollo_compiler::schema::ArgumentByNameError` into `apollo_compiler::ast` - [SimonSapin], [pull/942]**
 
 ## Features
@@ -41,6 +42,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 [pull/925]: https://github.com/apollographql/apollo-rs/issues/925
 [pull/929]: https://github.com/apollographql/apollo-rs/pull/929
 [pull/942]: https://github.com/apollographql/apollo-rs/pull/942
+[pull/944]: https://github.com/apollographql/apollo-rs/pull/944
 
 
 # [1.0.0-beta.24](https://crates.io/crates/apollo-compiler/1.0.0-beta.24) - 2024-09-24

crates/apollo-compiler/examples/introspect.rs

@@ -1,4 +1,4 @@
-use apollo_compiler::execution::SchemaIntrospectionQuery;
+use apollo_compiler::introspection;
 use apollo_compiler::validation::Valid;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
@@ -20,7 +20,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
         return Err("Provide a query to execute".into());
     };
 
-    let query = if filename.starts_with('{') {
+    let doc = if filename.starts_with('{') {
         ExecutableDocument::parse_and_validate(&schema, filename, "input.graphql")
     } else {
         ExecutableDocument::parse_and_validate(
@@ -31,17 +31,16 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     }
     .map_err(|err| err.errors.to_string())?;
 
-    let variables = Default::default();
-    let response = SchemaIntrospectionQuery::split_and_execute(
+    let response = introspection::partial_execute(
         &schema,
-        &query,
-        query
-            .operations
+        &schema.implementers_map(),
+        &doc,
+        doc.operations
             .get(None)
-            .map_err(|_| "Provided query must be an anonymous introspection query")?,
-        Valid::assume_valid_ref(&variables),
-        |_| panic!("Provided query must not have non-introspection elements"),
-    );
+            .map_err(|_| "Must have exactly one operation")?,
+        Valid::assume_valid_ref(&Default::default()),
+    )
+    .map_err(|e| e.message().to_string())?;
 
     serde_json::to_writer_pretty(std::io::stdout().lock(), &response)?;
 

crates/apollo-compiler/src/ast/impls.rs

@@ -713,6 +713,18 @@ impl OperationType {
         }
     }
 
+    pub fn is_query(&self) -> bool {
+        matches!(self, Self::Query)
+    }
+
+    pub fn is_mutation(&self) -> bool {
+        matches!(self, Self::Mutation)
+    }
+
+    pub fn is_subscription(&self) -> bool {
+        matches!(self, Self::Subscription)
+    }
+
     serialize_method!();
 }
 

crates/apollo-compiler/src/diagnostic.rs

@@ -67,12 +67,12 @@
 //!     }
 //! }
 //! ```
-use crate::execution::GraphQLError;
 use crate::parser::FileId;
 use crate::parser::LineColumn;
 use crate::parser::SourceFile;
 use crate::parser::SourceMap;
 use crate::parser::SourceSpan;
+use crate::response::GraphQLError;
 #[cfg(doc)]
 use crate::ExecutableDocument;
 #[cfg(doc)]

crates/apollo-compiler/src/executable/mod.rs

@@ -79,6 +79,7 @@ pub use crate::ast::OperationType;
 pub use crate::ast::Type;
 pub use crate::ast::Value;
 pub use crate::ast::VariableDefinition;
+use crate::request::RequestError;
 pub use crate::Name;
 
 /// Executable definitions, annotated with type information
@@ -332,18 +333,6 @@ pub(crate) enum ExecutableDefinitionName {
     Fragment(Name),
 }
 
-/// A request error returned by [`OperationMap::get`]
-///
-/// If `get_operation`’s `name_request` argument was `Some`, this error indicates
-/// that the document does not contain an operation with the requested name.
-///
-/// If `name_request` was `None`, the request is ambiguous
-/// because the document contains multiple operations
-/// (or zero, though the document would be invalid in that case).
-#[derive(Debug, Clone, PartialEq, Eq)]
-#[non_exhaustive]
-pub struct GetOperationError();
-
 impl ExecutableDocument {
     /// Create an empty document, to be filled programatically
     pub fn new() -> Self {
@@ -432,52 +421,73 @@ impl OperationMap {
 
     /// Return the relevant operation for a request, or a request error
     ///
-    /// This the [GetOperation()](https://spec.graphql.org/October2021/#GetOperation())
+    /// This is the [_GetOperation()_](https://spec.graphql.org/October2021/#GetOperation())
     /// algorithm in the _Executing Requests_ section of the specification.
     ///
     /// A GraphQL request comes with a document (which may contain multiple operations)
     /// an an optional operation name. When a name is given the request executes the operation
     /// with that name, which is expected to exist. When it is not given / null / `None`,
     /// the document is expected to contain a single operation (which may or may not be named)
     /// to avoid ambiguity.
-    pub fn get(&self, name_request: Option<&str>) -> Result<&Node<Operation>, GetOperationError> {
+    pub fn get(&self, name_request: Option<&str>) -> Result<&Node<Operation>, RequestError> {
         if let Some(name) = name_request {
             // Honor the request
-            self.named.get(name)
-        } else if let Some(op) = &self.anonymous {
-            // No name request, return the anonymous operation if it’s the only operation
-            self.named.is_empty().then_some(op)
-        } else {
-            // No name request or anonymous operation, return a named operation if it’s the only one
             self.named
-                .values()
-                .next()
-                .and_then(|op| (self.named.len() == 1).then_some(op))
+                .get(name)
+                .ok_or_else(|| format!("No operation named '{name}'"))
+        } else {
+            // No name request (`operationName` unspecified or null)
+            if let Some(op) = &self.anonymous {
+                // Return the anonymous operation if it’s the only operation
+                self.named.is_empty().then_some(op)
+            } else {
+                // No anonymous operation, return a named operation if it’s the only one
+                self.named
+                    .values()
+                    .next()
+                    .and_then(|op| (self.named.len() == 1).then_some(op))
+            }
+            .ok_or_else(|| {
+                "Ambiguous request: multiple operations but no specified `operationName`".to_owned()
+            })
         }
-        .ok_or(GetOperationError())
+        .map_err(|message| RequestError {
+            message,
+            location: None,
+            is_suspected_validation_bug: false,
+        })
     }
 
     /// Similar to [`get`][Self::get] but returns a mutable reference.
-    pub fn get_mut(
-        &mut self,
-        name_request: Option<&str>,
-    ) -> Result<&mut Operation, GetOperationError> {
+    pub fn get_mut(&mut self, name_request: Option<&str>) -> Result<&mut Operation, RequestError> {
         if let Some(name) = name_request {
             // Honor the request
-            self.named.get_mut(name)
-        } else if let Some(op) = &mut self.anonymous {
-            // No name request, return the anonymous operation if it’s the only operation
-            self.named.is_empty().then_some(op)
-        } else {
-            // No name request or anonymous operation, return a named operation if it’s the only one
-            let len = self.named.len();
             self.named
-                .values_mut()
-                .next()
-                .and_then(|op| (len == 1).then_some(op))
+                .get_mut(name)
+                .ok_or_else(|| format!("No operation named '{name}'"))
+        } else {
+            // No name request (`operationName` unspecified or null)
+            if let Some(op) = &mut self.anonymous {
+                // Return the anonymous operation if it’s the only operation
+                self.named.is_empty().then_some(op)
+            } else {
+                // No anonymous operation, return a named operation if it’s the only one
+                let len = self.named.len();
+                self.named
+                    .values_mut()
+                    .next()
+                    .and_then(|op| (len == 1).then_some(op))
+            }
+            .ok_or_else(|| {
+                "Ambiguous request: multiple operations but no specified `operationName`".to_owned()
+            })
         }
         .map(Node::make_mut)
-        .ok_or(GetOperationError())
+        .map_err(|message| RequestError {
+            message,
+            location: None,
+            is_suspected_validation_bug: false,
+        })
     }
 
     /// Insert the given operation in either `named_operations` or `anonymous_operation`
@@ -532,7 +542,8 @@ impl Operation {
     ///
     /// This does **not** perform [field merging] nor fragment spreads de-duplication,
     /// so multiple items in this iterator may have the same response key,
-    /// point to the same field definition, or even be the same field selection.
+    /// point to the same field definition,
+    /// or even be the same field selection (reached through different fragments).
     ///
     /// [field merging]: https://spec.graphql.org/draft/#sec-Field-Selection-Merging
     pub fn root_fields<'doc>(

crates/apollo-compiler/src/execution/engine.rs

@@ -7,12 +7,12 @@ use crate::execution::input_coercion::coerce_argument_values;
 use crate::execution::resolver::ObjectValue;
 use crate::execution::resolver::ResolverError;
 use crate::execution::result_coercion::complete_value;
-use crate::execution::GraphQLError;
-use crate::execution::JsonMap;
-use crate::execution::JsonValue;
-use crate::execution::ResponseDataPathElement;
 use crate::parser::SourceMap;
 use crate::parser::SourceSpan;
+use crate::response::GraphQLError;
+use crate::response::JsonMap;
+use crate::response::JsonValue;
+use crate::response::ResponseDataPathSegment;
 use crate::schema::ExtendedType;
 use crate::schema::FieldDefinition;
 use crate::schema::ObjectType;
@@ -42,7 +42,7 @@ pub(crate) struct PropagateNull;
 pub(crate) type LinkedPath<'a> = Option<&'a LinkedPathElement<'a>>;
 
 pub(crate) struct LinkedPathElement<'a> {
-    pub(crate) element: ResponseDataPathElement,
+    pub(crate) element: ResponseDataPathSegment,
     pub(crate) next: LinkedPath<'a>,
 }
 
@@ -59,12 +59,13 @@ pub(crate) fn execute_selection_set<'a>(
     object_value: &ObjectValue<'_>,
     selections: impl IntoIterator<Item = &'a Selection>,
 ) -> Result<JsonMap, PropagateNull> {
-    let mut grouped_field_set = IndexMap::with_hasher(Default::default());
+    let mut grouped_field_set = IndexMap::default();
     collect_fields(
         schema,
         document,
         variable_values,
         object_type,
+        object_value,
         selections,
         &mut HashSet::default(),
         &mut grouped_field_set,
@@ -92,7 +93,7 @@ pub(crate) fn execute_selection_set<'a>(
             JsonValue::from(object_type.name.as_str())
         } else {
             let field_path = LinkedPathElement {
-                element: ResponseDataPathElement::Field(response_key.clone()),
+                element: ResponseDataPathSegment::Field(response_key.clone()),
                 next: path,
             };
             execute_field(
@@ -119,6 +120,7 @@ fn collect_fields<'a>(
     document: &'a ExecutableDocument,
     variable_values: &Valid<JsonMap>,
     object_type: &ObjectType,
+    object_value: &ObjectValue<'_>,
     selections: impl IntoIterator<Item = &'a Selection>,
     visited_fragments: &mut HashSet<&'a Name>,
     grouped_fields: &mut IndexMap<&'a Name, Vec<&'a Field>>,
@@ -130,10 +132,14 @@ fn collect_fields<'a>(
             continue;
         }
         match selection {
-            Selection::Field(field) => grouped_fields
-                .entry(field.response_key())
-                .or_default()
-                .push(field.as_ref()),
+            Selection::Field(field) => {
+                if !object_value.skip_field(&field.name) {
+                    grouped_fields
+                        .entry(field.response_key())
+                        .or_default()
+                        .push(field.as_ref())
+                }
+            }
             Selection::FragmentSpread(spread) => {
                 let new = visited_fragments.insert(&spread.fragment_name);
                 if !new {
@@ -150,6 +156,7 @@ fn collect_fields<'a>(
                     document,
                     variable_values,
                     object_type,
+                    object_value,
                     &fragment.selection_set.selections,
                     visited_fragments,
                     grouped_fields,
@@ -166,6 +173,7 @@ fn collect_fields<'a>(
                     document,
                     variable_values,
                     object_type,
+                    object_value,
                     &inline.selection_set.selections,
                     visited_fragments,
                     grouped_fields,
@@ -280,7 +288,7 @@ pub(crate) fn try_nullify(
     }
 }
 
-pub(crate) fn path_to_vec(mut link: LinkedPath<'_>) -> Vec<ResponseDataPathElement> {
+pub(crate) fn path_to_vec(mut link: LinkedPath<'_>) -> Vec<ResponseDataPathSegment> {
     let mut path = Vec::new();
     while let Some(node) = link {
         path.push(node.element.clone());
@@ -309,8 +317,10 @@ impl SuspectedValidationBug {
         sources: &SourceMap,
         path: LinkedPath<'_>,
     ) -> GraphQLError {
-        let mut err = self.into_graphql_error(sources);
-        err.path = path_to_vec(path);
+        let Self { message, location } = self;
+        let mut err = GraphQLError::field_error(message, path, location, sources);
+        err.extensions
+            .insert("APOLLO_SUSPECTED_VALIDATION_BUG", true.into());
         err
     }
 }