feat(rover-client): improves error handling

Adds some more strongly typed errors in rover-client
in addition to some new suggestions in rover.

Author: EverlastingBugstopper

crates/rover-client/src/blocking/client.rs

@@ -46,12 +46,7 @@ impl Client {
     pub fn handle_response<Q: graphql_client::GraphQLQuery>(
         response: reqwest::blocking::Response,
     ) -> Result<Q::ResponseData, RoverClientError> {
-        let response_body: graphql_client::Response<Q::ResponseData> =
-            response
-                .json()
-                .map_err(|_| RoverClientError::HandleResponse {
-                    msg: String::from("failed to parse response JSON"),
-                })?;
+        let response_body: graphql_client::Response<Q::ResponseData> = response.json()?;
 
         if let Some(errs) = response_body.errors {
             return Err(RoverClientError::GraphQL {
@@ -66,7 +61,7 @@ impl Client {
         if let Some(data) = response_body.data {
             Ok(data)
         } else {
-            Err(RoverClientError::NoData)
+            Err(RoverClientError::MalformedResponse)
         }
     }
 }

crates/rover-client/src/error.rs

@@ -4,8 +4,11 @@ use thiserror::Error;
 #[derive(Error, Debug)]
 pub enum RoverClientError {
     /// The provided GraphQL was invalid.
-    #[error("encountered a GraphQL error, registry responded with: {msg}")]
-    GraphQL { msg: String },
+    #[error("GraphQL error:\n               {msg}")]
+    GraphQL {
+        /// The encountered GraphQL error.
+        msg: String,
+    },
 
     /// Tried to build a [HeaderMap] with an invalid header name.
     #[error("invalid header name")]
@@ -20,37 +23,53 @@ pub enum RoverClientError {
     InvalidJSON(#[from] serde_json::Error),
 
     /// Encountered an error handling the received response.
-    #[error("encountered an error handling the response: {msg}")]
-    HandleResponse {
+    #[error("{msg}")]
+    AdhocError {
         /// The error message.
         msg: String,
     },
 
+    /// The user provided an invalid subgraph name.
+    #[error("Could not find subgraph \"{invalid_subgraph}\".")]
+    NoSubgraphInGraph {
+        /// The invalid subgraph name
+        invalid_subgraph: String,
+
+        /// A list of valid subgraph names
+        // this is not used in the error message, but can be accessed
+        // by application-level error handlers
+        valid_subgraphs: Vec<String>,
+    },
+
+    /// The Studio API could not find a variant for a graph
+    #[error(
+        "The graph registry does not contain variant \"{invalid_variant}\" for graph \"{graph}\""
+    )]
+    NoSchemaForVariant {
+        /// The name of the graph.
+        graph: String,
+
+        /// The non-existent variant.
+        invalid_variant: String,
+    },
+
     /// Encountered an error sending the request.
     #[error("encountered an error while sending a request")]
     SendRequest(#[from] reqwest::Error),
 
-    /// This error occurs when there are no `body.errors` but `body.data` is
-    /// also empty. In proper GraphQL responses, there should _always_ be either
-    /// body.errors or body.data
-    #[error("The response from the server was malformed. There was no data found in the reponse body. This is likely an error in GraphQL execution")]
-    NoData,
-
     /// when someone provides a bad graph/variant combination or isn't
     /// validated properly, we don't know which reason is at fault for data.service
     /// being empty, so this error tells them to check both.
-    #[error("No graph found. Either the graph@variant combination wasn't found or your API key is invalid.")]
-    NoService,
+    #[error("Could not find graph with name \"{graph}\"")]
+    NoService { graph: String },
 
-    /// This error occurs when the Studio API returns no composition errors AND
-    /// no check result. This response shouldn't be possible!
-    #[error(
-        "The response from the server was malformed, there was no data from the check operation."
-    )]
-    NoCheckData,
+    /// This error occurs when the Studio API returns no implementing services for a graph
+    /// This response shouldn't be possible!
+    #[error("The response from Apollo Studio was malformed.")]
+    MalformedResponse,
 
-    #[error("The graph `{graph_name}` is a non-federated graph. This operation is only possible for federated graphs")]
-    ExpectedFederatedGraph { graph_name: String },
+    #[error("The graph `{graph}` is a non-federated graph. This operation is only possible for federated graphs")]
+    ExpectedFederatedGraph { graph: String },
 
     /// The API returned an invalid ChangeSeverity value
     #[error("Invalid ChangeSeverity.")]

crates/rover-client/src/query/graph/check.rs

@@ -25,8 +25,9 @@ pub fn run(
     variables: check_schema_query::Variables,
     client: &StudioClient,
 ) -> Result<CheckResponse, RoverClientError> {
+    let graph = variables.graph_id.clone();
     let data = client.post::<CheckSchemaQuery>(variables)?;
-    get_check_response_from_data(data)
+    get_check_response_from_data(data, graph)
 }
 
 #[derive(Debug)]
@@ -39,8 +40,9 @@ pub struct CheckResponse {
 
 fn get_check_response_from_data(
     data: check_schema_query::ResponseData,
+    graph: String,
 ) -> Result<CheckResponse, RoverClientError> {
-    let service = data.service.ok_or(RoverClientError::NoService)?;
+    let service = data.service.ok_or(RoverClientError::NoService { graph })?;
     let target_url = get_url(service.check_schema.target_url);
 
     let diff_to_previous = service.check_schema.diff_to_previous;

crates/rover-client/src/query/graph/fetch.rs

@@ -26,24 +26,34 @@ pub fn run(
     variables: fetch_schema_query::Variables,
     client: &StudioClient,
 ) -> Result<String, RoverClientError> {
+    let graph = variables.graph_id.clone();
+    let invalid_variant = variables
+        .variant
+        .clone()
+        .unwrap_or_else(|| "current".to_string());
     let response_data = client.post::<FetchSchemaQuery>(variables)?;
-    get_schema_from_response_data(response_data)
+    get_schema_from_response_data(response_data, graph, invalid_variant)
     // if we want json, we can parse & serialize it here
 }
 
 fn get_schema_from_response_data(
     response_data: fetch_schema_query::ResponseData,
+    graph: String,
+    invalid_variant: String,
 ) -> Result<String, RoverClientError> {
     let service_data = match response_data.service {
         Some(data) => Ok(data),
-        None => Err(RoverClientError::NoService),
+        None => Err(RoverClientError::NoService {
+            graph: graph.clone(),
+        }),
     }?;
 
     if let Some(schema) = service_data.schema {
         Ok(schema.document)
     } else {
-        Err(RoverClientError::HandleResponse {
-            msg: "No schema found for this variant".to_string(),
+        Err(RoverClientError::NoSchemaForVariant {
+            graph,
+            invalid_variant,
         })
     }
 }
@@ -62,7 +72,8 @@ mod tests {
             }
         });
         let data: fetch_schema_query::ResponseData = serde_json::from_value(json_response).unwrap();
-        let output = get_schema_from_response_data(data);
+        let (graph, invalid_variant) = mock_vars();
+        let output = get_schema_from_response_data(data, graph, invalid_variant);
 
         assert!(output.is_ok());
         assert_eq!(output.unwrap(), "type Query { hello: String }".to_string());
@@ -72,7 +83,8 @@ mod tests {
     fn get_schema_from_response_data_errs_on_no_service() {
         let json_response = json!({ "service": null });
         let data: fetch_schema_query::ResponseData = serde_json::from_value(json_response).unwrap();
-        let output = get_schema_from_response_data(data);
+        let (graph, invalid_variant) = mock_vars();
+        let output = get_schema_from_response_data(data, graph, invalid_variant);
 
         assert!(output.is_err());
     }
@@ -85,8 +97,13 @@ mod tests {
             }
         });
         let data: fetch_schema_query::ResponseData = serde_json::from_value(json_response).unwrap();
-        let output = get_schema_from_response_data(data);
+        let (graph, invalid_variant) = mock_vars();
+        let output = get_schema_from_response_data(data, graph, invalid_variant);
 
         assert!(output.is_err());
     }
+
+    fn mock_vars() -> (String, String) {
+        ("mygraph".to_string(), "current".to_string())
+    }
 }

crates/rover-client/src/query/graph/push.rs

@@ -28,25 +28,27 @@ pub fn run(
     variables: push_schema_mutation::Variables,
     client: &StudioClient,
 ) -> Result<PushResponse, RoverClientError> {
+    let graph = variables.graph_id.clone();
     let data = client.post::<PushSchemaMutation>(variables)?;
-    let push_response = get_push_response_from_data(data)?;
+    let push_response = get_push_response_from_data(data, graph)?;
     build_response(push_response)
 }
 
 fn get_push_response_from_data(
     data: push_schema_mutation::ResponseData,
+    graph: String,
 ) -> Result<push_schema_mutation::PushSchemaMutationServiceUploadSchema, RoverClientError> {
     // then, from the response data, get .service?.upload_schema?
     let service_data = match data.service {
         Some(data) => data,
-        None => return Err(RoverClientError::NoService),
+        None => return Err(RoverClientError::NoService { graph }),
     };
 
     if let Some(opt_data) = service_data.upload_schema {
         Ok(opt_data)
     } else {
-        Err(RoverClientError::HandleResponse {
-            msg: "No response from mutation. Check your API key & graph name".to_string(),
+        Err(RoverClientError::AdhocError {
+            msg: "No response from the graph registry. Check your API key & graph name".to_string(),
         })
     }
 }
@@ -56,15 +58,18 @@ fn build_response(
 ) -> Result<PushResponse, RoverClientError> {
     if !push_response.success {
         let msg = format!("Schema upload failed with error: {}", push_response.message);
-        return Err(RoverClientError::HandleResponse { msg });
+        return Err(RoverClientError::AdhocError { msg });
     }
 
     let hash = match &push_response.tag {
         // we only want to print the first 6 chars of a hash
         Some(tag_data) => tag_data.schema.hash.clone()[..6].to_string(),
         None => {
-            let msg = format!("No schema tag info available ({})", push_response.message);
-            return Err(RoverClientError::HandleResponse { msg });
+            let msg = format!(
+                "No data in response from schema push. Failed with message: {}",
+                push_response.message
+            );
+            return Err(RoverClientError::AdhocError { msg });
         }
     };
 

crates/rover-client/src/query/subgraph/check.rs

@@ -25,8 +25,9 @@ pub fn run(
     variables: check_partial_schema_query::Variables,
     client: &StudioClient,
 ) -> Result<CheckResponse, RoverClientError> {
+    let graph = variables.graph_id.clone();
     let data = client.post::<CheckPartialSchemaQuery>(variables)?;
-    get_check_response_from_data(data)
+    get_check_response_from_data(data, graph)
 }
 
 pub enum CheckResponse {
@@ -44,8 +45,9 @@ pub struct CheckResult {
 
 fn get_check_response_from_data(
     data: check_partial_schema_query::ResponseData,
+    graph: String,
 ) -> Result<CheckResponse, RoverClientError> {
-    let service = data.service.ok_or(RoverClientError::NoService)?;
+    let service = data.service.ok_or(RoverClientError::NoService { graph })?;
 
     // for some reason this is a `Vec<Option<CompositionError>>`
     // we convert this to just `Vec<CompositionError>` because the `None`
@@ -60,7 +62,7 @@ fn get_check_response_from_data(
         let check_schema_result = service
             .check_partial_schema
             .check_schema_result
-            .ok_or(RoverClientError::NoCheckData)?;
+            .ok_or(RoverClientError::MalformedResponse)?;
 
         let target_url = get_url(check_schema_result.target_url);
 

crates/rover-client/src/query/subgraph/delete.graphql

@@ -1,10 +1,10 @@
 mutation DeleteServiceMutation(
-  $id: ID!
+  $graphId: ID!
   $graphVariant: String!
   $name: String!
   $dryRun: Boolean!
 ) {
-  service(id: $id) {
+  service(id: $graphId) {
     removeImplementingServiceAndTriggerComposition(
       graphVariant: $graphVariant
       name: $name

crates/rover-client/src/query/subgraph/delete.rs

@@ -33,17 +33,19 @@ pub fn run(
     variables: delete_service_mutation::Variables,
     client: &StudioClient,
 ) -> Result<DeleteServiceResponse, RoverClientError> {
+    let graph = variables.graph_id.clone();
     let response_data = client.post::<DeleteServiceMutation>(variables)?;
-    let data = get_delete_data_from_response(response_data)?;
+    let data = get_delete_data_from_response(response_data, graph)?;
     Ok(build_response(data))
 }
 
 fn get_delete_data_from_response(
     response_data: delete_service_mutation::ResponseData,
+    graph: String,
 ) -> Result<RawMutationResponse, RoverClientError> {
     let service_data = match response_data.service {
         Some(data) => Ok(data),
-        None => Err(RoverClientError::NoService),
+        None => Err(RoverClientError::NoService { graph }),
     }?;
 
     Ok(service_data.remove_implementing_service_and_trigger_composition)

crates/rover-client/src/query/subgraph/fetch.rs

@@ -24,21 +24,23 @@ pub fn run(
     // operation response by this name
     subgraph: &str,
 ) -> Result<String, RoverClientError> {
-    let graph_name = variables.graph_id.clone();
+    let graph = variables.graph_id.clone();
     let response_data = client.post::<FetchSubgraphQuery>(variables)?;
-    let services = get_services_from_response_data(response_data, &graph_name)?;
+    let services = get_services_from_response_data(response_data, graph)?;
     get_sdl_for_service(services, subgraph)
     // if we want json, we can parse & serialize it here
 }
 
 type ServiceList = Vec<fetch_subgraph_query::FetchSubgraphQueryServiceImplementingServicesOnFederatedImplementingServicesServices>;
 fn get_services_from_response_data(
     response_data: fetch_subgraph_query::ResponseData,
-    graph_name: &str,
+    graph: String,
 ) -> Result<ServiceList, RoverClientError> {
     let service_data = match response_data.service {
         Some(data) => Ok(data),
-        None => Err(RoverClientError::NoService),
+        None => Err(RoverClientError::NoService {
+            graph: graph.clone(),
+        }),
     }?;
 
     // get list of services
@@ -49,7 +51,7 @@ fn get_services_from_response_data(
         // of a non-federated graph. Fow now, this case still exists, but
         // wont' for long. Check on this later (Jake) :)
         None => Err(RoverClientError::ExpectedFederatedGraph {
-            graph_name: graph_name.to_string(),
+            graph: graph.clone(),
         }),
     }?;
 
@@ -58,7 +60,7 @@ fn get_services_from_response_data(
             Ok(services.services)
         },
         fetch_subgraph_query::FetchSubgraphQueryServiceImplementingServices::NonFederatedImplementingService => {
-            Err(RoverClientError::ExpectedFederatedGraph { graph_name: graph_name.to_string() })
+            Err(RoverClientError::ExpectedFederatedGraph { graph })
         }
     }
 }
@@ -72,14 +74,12 @@ fn get_sdl_for_service(services: ServiceList, subgraph: &str) -> Result<String,
     if let Some(service) = service {
         Ok(service.active_partial_schema.sdl.clone())
     } else {
-        let all_supgraph_names: Vec<String> = services.iter().map(|svc| svc.name.clone()).collect();
-        let msg = format!(
-            "Could not find subgraph `{}` in list of subgraphs. Available subgraphs to fetch: [{}]",
-            subgraph,
-            all_supgraph_names.join(", ")
-        );
+        let valid_subgraphs: Vec<String> = services.iter().map(|svc| svc.name.clone()).collect();
 
-        Err(RoverClientError::HandleResponse { msg })
+        Err(RoverClientError::NoSubgraphInGraph {
+            invalid_subgraph: subgraph.to_string(),
+            valid_subgraphs,
+        })
     }
 }