fix: actually throw subgraph client errors

Author: EverlastingBugstopper

ARCHITECTURE.md

@@ -271,15 +271,15 @@ Whenever you create a new command, make sure to add `#[serde(skip_serializing)]`
 
 ##### Adding a query to Apollo Studio
 
-The only piece of the `rover-client` crate that we need to be concerned with for now is the `src/query` directory. This is where all the queries to Apollo Studio live. This directory is roughly organized by the command names as well, but there might be some queries in these directories that are used by multiple commands.
+The only piece of the `rover-client` crate that we need to be concerned with for now is the `src/operations` directory. This is where all the queries to Apollo Studio live. This directory is roughly organized by the command names as well, but there might be some queries in these directories that are used by multiple commands.
 
-You can see in the `src/query/graph` directory a number of `.rs` files paired with `.graphql` files. The `.graphql` files are the files where the GraphQL operations live, and the matching `.rs` files contain the logic needed to execute those operations.
+You can see in the `src/operations/graph` directory a number of `.rs` files paired with `.graphql` files. The `.graphql` files are the files where the GraphQL operations live, and the matching `.rs` files contain the logic needed to execute those operations.
 
 ##### Writing a GraphQL operation
 
 For our basic `graph hello` command, we're going to make a request to Apollo Studio that inquires about the existence of a particular graph, and nothing else. For this, we can use the `Query.service` field.
 
-Create a `hello.graphql` file in `crates/rover-client/src/query/graph` and paste the following into it:
+Create a `hello.graphql` file in `crates/rover-client/src/operations/graph` and paste the following into it:
 
 ```graphql
 query GraphHello($graphId: ID!) {
@@ -295,9 +295,9 @@ This basic GraphQL operation uses a graph's unique ID (which we get from the `Gr
 
 This project uses [graphql-client](https://docs.rs/graphql_client/latest/graphql_client/) to generate types for each raw `.graphql` query that we write.
 
-First, create an empty file at `crates/rover-client/src/query/graph/hello.rs`.
+First, create an empty file at `crates/rover-client/src/operations/graph/hello.rs`.
 
-To start compiling this file, we need to export the module in `crates/rover-client/src/query/graph/mod.rs`:
+To start compiling this file, we need to export the module in `crates/rover-client/src/operations/graph/mod.rs`:
 
 ```rust
 ...

crates/rover-client/src/operations/graph/check/mutation_runner.rs

@@ -1,5 +1,7 @@
 use crate::blocking::StudioClient;
-use crate::operations::graph::check::types::{GraphCheckInput, MutationResponseData};
+use crate::operations::graph::check::types::{
+    GraphCheckInput, MutationChangeSeverity, MutationResponseData,
+};
 use crate::shared::CheckResponse;
 use crate::RoverClientError;
 
@@ -45,14 +47,19 @@ fn get_check_response_from_data(
 
     let change_severity = diff_to_previous.severity.into();
     let mut changes = Vec::with_capacity(diff_to_previous.changes.len());
+    let mut num_failures = 0;
     for change in diff_to_previous.changes {
+        if let MutationChangeSeverity::FAILURE = change.severity {
+            num_failures += 1;
+        }
         changes.push(change.into());
     }
 
     Ok(CheckResponse {
+        num_failures,
         target_url,
         number_of_checked_operations,
-        change_severity,
         changes,
+        change_severity,
     })
 }

crates/rover-client/src/operations/graph/check/types.rs

@@ -41,7 +41,7 @@ impl From<CheckConfig> for MutationConfig {
 type MutationVariables = graph_check_mutation::Variables;
 pub(crate) type MutationResponseData = graph_check_mutation::ResponseData;
 
-type MutationChangeSeverity = graph_check_mutation::ChangeSeverity;
+pub(crate) type MutationChangeSeverity = graph_check_mutation::ChangeSeverity;
 impl From<MutationChangeSeverity> for ChangeSeverity {
     fn from(severity: MutationChangeSeverity) -> Self {
         match severity {

crates/rover-client/src/operations/graph/introspect/schema.rs

@@ -367,7 +367,7 @@ mod tests {
 
     #[test]
     fn it_builds_simple_schema() {
-        let file = File::open("src/query/graph/introspect/fixtures/simple.json").unwrap();
+        let file = File::open("src/operations/graph/introspect/fixtures/simple.json").unwrap();
         let res: Response<QueryResponseData> = serde_json::from_reader(file).unwrap();
 
         let data = res.data.unwrap();
@@ -401,7 +401,7 @@ mod tests {
 
     #[test]
     fn it_builds_swapi_schema() {
-        let file = File::open("src/query/graph/introspect/fixtures/swapi.json").unwrap();
+        let file = File::open("src/operations/graph/introspect/fixtures/swapi.json").unwrap();
         let res: Response<QueryResponseData> = serde_json::from_reader(file).unwrap();
 
         let data = res.data.unwrap();
@@ -1364,7 +1364,7 @@ mod tests {
 
     #[test]
     fn it_builds_schema_with_interfaces() {
-        let file = File::open("src/query/graph/introspect/fixtures/interfaces.json").unwrap();
+        let file = File::open("src/operations/graph/introspect/fixtures/interfaces.json").unwrap();
         let res: Response<QueryResponseData> = serde_json::from_reader(file).unwrap();
 
         let data = res.data.unwrap();

crates/rover-client/src/operations/subgraph/check/mutation_runner.rs

@@ -79,7 +79,11 @@ fn get_check_response_from_data(
         let change_severity = diff_to_previous.severity.into();
 
         let mut changes = Vec::with_capacity(diff_to_previous.changes.len());
+        let mut num_failures = 0;
         for change in diff_to_previous.changes {
+            if let MutationChangeSeverity::FAILURE = change.severity {
+                num_failures += 1;
+            }
             changes.push(SchemaChange {
                 code: change.code,
                 severity: change.severity.into(),
@@ -88,6 +92,7 @@ fn get_check_response_from_data(
         }
 
         let check_result = CheckResponse {
+            num_failures,
             target_url: check_schema_result.target_url,
             number_of_checked_operations,
             changes,

crates/rover-client/src/operations/subgraph/check/types.rs

@@ -10,7 +10,7 @@ pub(crate) type MutationCompositionErrors =
 type MutationSchema = subgraph_check_mutation::PartialSchemaInput;
 type MutationConfig = subgraph_check_mutation::HistoricQueryParameters;
 
-type MutationChangeSeverity = subgraph_check_mutation::ChangeSeverity;
+pub(crate) type MutationChangeSeverity = subgraph_check_mutation::ChangeSeverity;
 impl From<MutationChangeSeverity> for ChangeSeverity {
     fn from(severity: MutationChangeSeverity) -> Self {
         match severity {

crates/rover-client/src/shared/check_response.rs

@@ -4,6 +4,7 @@ use std::fmt;
 /// `graph` and `subgraph` check operations
 #[derive(Debug, Clone, PartialEq)]
 pub struct CheckResponse {
+    pub num_failures: i64,
     pub target_url: Option<String>,
     pub number_of_checked_operations: i64,
     pub changes: Vec<SchemaChange>,

src/command/graph/check.rs

@@ -1,17 +1,17 @@
 use serde::Serialize;
 use structopt::StructOpt;
 
-use rover_client::operations::graph::check;
-use rover_client::shared::GitContext;
+use rover_client::operations::graph::check::{self, GraphCheckInput};
+use rover_client::shared::{CheckConfig, GitContext};
 
+use crate::command::shared::check::print_check_response;
 use crate::command::RoverStdout;
 use crate::utils::client::StudioClientConfig;
 use crate::utils::loaders::load_schema_from_flag;
 use crate::utils::parsers::{
     parse_graph_ref, parse_query_count_threshold, parse_query_percentage_threshold,
     parse_schema_source, parse_validation_period, GraphRef, SchemaSource, ValidationPeriod,
 };
-use crate::utils::table::{self, cell, row};
 use crate::Result;
 
 #[derive(Debug, Serialize, StructOpt)]
@@ -56,79 +56,29 @@ impl Check {
         git_context: GitContext,
     ) -> Result<RoverStdout> {
         let client = client_config.get_client(&self.profile_name)?;
-        let sdl = load_schema_from_flag(&self.schema, std::io::stdin())?;
+        let proposed_schema = load_schema_from_flag(&self.schema, std::io::stdin())?;
+
+        eprintln!(
+            "Checking the proposed schema against metrics from {}",
+            &self.graph
+        );
+
         let res = check::run(
-            check::check_schema_query::Variables {
+            GraphCheckInput {
                 graph_id: self.graph.name.clone(),
-                variant: Some(self.graph.variant.clone()),
-                schema: Some(sdl),
-                git_context: git_context.into(),
-                config: check::check_schema_query::HistoricQueryParameters {
+                variant: self.graph.variant.clone(),
+                proposed_schema,
+                git_context,
+                config: CheckConfig {
                     query_count_threshold: self.query_count_threshold,
                     query_count_threshold_percentage: self.query_percentage_threshold,
-                    from: self.validation_period.clone().unwrap_or_default().from,
-                    to: self.validation_period.clone().unwrap_or_default().to,
-                    // we don't support configuring these, but we can't leave them out
-                    excluded_clients: None,
-                    ignored_operations: None,
-                    included_variants: None,
+                    validation_period_from: self.validation_period.clone().unwrap_or_default().from,
+                    validation_period_to: self.validation_period.clone().unwrap_or_default().to,
                 },
             },
             &client,
         )?;
 
-        eprintln!(
-            "Validated the proposed subgraph against metrics from {}",
-            &self.graph
-        );
-
-        let num_changes = res.changes.len();
-
-        let msg = match num_changes {
-            0 => "There is no difference between the proposed graph and the graph that already exists in the graph registry. Try making a change to your proposed graph before running this command.".to_string(),
-            _ => format!("Compared {} schema changes against {} operations", res.changes.len(), res.number_of_checked_operations),
-        };
-
-        eprintln!("{}", &msg);
-
-        let num_failures = print_changes(&res.changes);
-
-        if let Some(url) = res.target_url {
-            eprintln!("View full details at {}", &url);
-        }
-
-        match num_failures {
-            0 => Ok(RoverStdout::None),
-            1 => Err(anyhow::anyhow!("Encountered 1 failure.").into()),
-            _ => Err(anyhow::anyhow!("Encountered {} failures.", num_failures).into()),
-        }
-    }
-}
-
-fn print_changes(
-    checks: &[check::check_schema_query::CheckSchemaQueryServiceCheckSchemaDiffToPreviousChanges],
-) -> u64 {
-    let mut num_failures = 0;
-
-    if !checks.is_empty() {
-        let mut table = table::get_table();
-
-        // bc => sets top row to be bold and center
-        table.add_row(row![bc => "Change", "Code", "Description"]);
-        for check in checks {
-            let change = match check.severity {
-                check::check_schema_query::ChangeSeverity::NOTICE => "PASS",
-                check::check_schema_query::ChangeSeverity::FAILURE => {
-                    num_failures += 1;
-                    "FAIL"
-                }
-                _ => unreachable!("Unknown change severity"),
-            };
-            table.add_row(row![change, check.code, check.description]);
-        }
-
-        eprintln!("{}", table);
+        print_check_response(res)
     }
-
-    num_failures
 }

src/command/mod.rs

@@ -5,6 +5,7 @@ mod graph;
 mod info;
 mod install;
 mod output;
+pub(crate) mod shared;
 mod subgraph;
 mod supergraph;
 mod update;

src/command/output.rs

@@ -6,9 +6,7 @@ use crate::utils::table::{self, cell, row};
 use ansi_term::{Colour::Yellow, Style};
 use atty::Stream;
 use crossterm::style::Attribute::Underlined;
-use rover_client::operations::subgraph::{
-    check::SubgraphCheckResponse, list::SubgraphListResponse,
-};
+use rover_client::operations::subgraph::list::SubgraphListResponse;
 use termimad::MadSkin;
 
 /// RoverStdout defines all of the different types of data that are printed
@@ -27,7 +25,6 @@ pub enum RoverStdout {
     CoreSchema(String),
     SchemaHash(String),
     SubgraphList(SubgraphListResponse),
-    SubgraphCheck(SubgraphCheckResponse),
     VariantList(Vec<String>),
     Profiles(Vec<String>),
     Introspection(String),
@@ -101,36 +98,6 @@ impl RoverStdout {
                     details.root_url, details.graph_name
                 );
             }
-            RoverStdout::SubgraphCheck(check_response) => {
-                let num_changes = check_response.changes.len();
-
-                let msg = match num_changes {
-                    0 => "There were no changes detected in the composed schema.".to_string(),
-                    _ => format!(
-                        "Compared {} schema changes against {} operations",
-                        check_response.changes.len(),
-                        check_response.number_of_checked_operations
-                    ),
-                };
-
-                eprintln!("{}", &msg);
-
-                if !check_response.changes.is_empty() {
-                    let mut table = table::get_table();
-
-                    // bc => sets top row to be bold and center
-                    table.add_row(row![bc => "Change", "Code", "Description"]);
-                    for check in &check_response.changes {
-                        table.add_row(row![check.severity, check.code, check.description]);
-                    }
-
-                    print_content(table.to_string());
-                }
-
-                if let Some(url) = &check_response.target_url {
-                    eprintln!("View full details at {}", url);
-                }
-            }
             RoverStdout::VariantList(variants) => {
                 print_descriptor("Variants");
                 for variant in variants {

src/command/shared/check.rs

@@ -0,0 +1,41 @@
+use crate::command::RoverStdout;
+use crate::{anyhow, Result};
+
+use crate::utils::table::{self, cell, row};
+use rover_client::shared::CheckResponse;
+
+pub(crate) fn print_check_response(check_response: CheckResponse) -> Result<RoverStdout> {
+    let num_changes = check_response.changes.len();
+
+    let msg = match num_changes {
+        0 => "There were no changes detected in the composed schema.".to_string(),
+        _ => format!(
+            "Compared {} schema changes against {} operations",
+            check_response.changes.len(),
+            check_response.number_of_checked_operations
+        ),
+    };
+
+    eprintln!("{}", &msg);
+
+    if !check_response.changes.is_empty() {
+        let mut table = table::get_table();
+
+        // bc => sets top row to be bold and center
+        table.add_row(row![bc => "Change", "Code", "Description"]);
+        for check in &check_response.changes {
+            table.add_row(row![check.severity, check.code, check.description]);
+        }
+
+        println!("{}", table);
+    }
+
+    if let Some(url) = &check_response.target_url {
+        eprintln!("View full details at {}", url);
+    }
+    match &check_response.num_failures {
+        0 => Ok(RoverStdout::None),
+        1 => Err(anyhow!("Encountered 1 failure.").into()),
+        _ => Err(anyhow!("Encountered {} failures.", &check_response.num_failures).into()),
+    }
+}

src/command/shared/mod.rs

@@ -0,0 +1 @@
+pub(crate) mod check;

src/command/subgraph/check.rs

@@ -4,6 +4,7 @@ use structopt::StructOpt;
 use rover_client::operations::subgraph::check::{mutation_runner, SubgraphCheckInput};
 use rover_client::shared::{CheckConfig, GitContext};
 
+use crate::command::shared::check::print_check_response;
 use crate::command::RoverStdout;
 use crate::utils::client::StudioClientConfig;
 use crate::utils::loaders::load_schema_from_flag;
@@ -85,6 +86,6 @@ impl Check {
             &client,
         )?;
 
-        Ok(RoverStdout::SubgraphCheck(res))
+        print_check_response(res)
     }
 }