From 716c278c9d6cf8816f5f06a85534181d8e14883c Mon Sep 17 00:00:00 2001 From: Avery Harnish Date: Thu, 20 May 2021 14:58:54 -0500 Subject: [PATCH] wip: refactor subgraph check and add line numbers --- crates/rover-client/src/error.rs | 21 +++++ .../check_query.graphql} | 7 +- .../src/query/subgraph/check/mod.rs | 3 + .../{check.rs => check/query_runner.rs} | 94 ++++++++----------- .../src/query/subgraph/check/types.rs | 53 +++++++++++ docs/source/errors.md | 8 ++ src/command/output.rs | 33 ++++++- src/command/subgraph/check.rs | 94 ++----------------- src/error/metadata/code.rs | 2 + src/error/metadata/codes/E028.md | 2 +- src/error/metadata/codes/E029.md | 5 + src/error/metadata/mod.rs | 8 ++ src/error/metadata/suggestion.rs | 2 + src/utils/git.rs | 2 +- 14 files changed, 191 insertions(+), 143 deletions(-) rename crates/rover-client/src/query/subgraph/{check.graphql => check/check_query.graphql} (86%) create mode 100644 crates/rover-client/src/query/subgraph/check/mod.rs rename crates/rover-client/src/query/subgraph/{check.rs => check/query_runner.rs} (50%) create mode 100644 crates/rover-client/src/query/subgraph/check/types.rs create mode 100644 src/error/metadata/codes/E029.md diff --git a/crates/rover-client/src/error.rs b/crates/rover-client/src/error.rs index 3f92d95bd..0fba9ff33 100644 --- a/crates/rover-client/src/error.rs +++ b/crates/rover-client/src/error.rs @@ -1,6 +1,8 @@ use reqwest::Url; use thiserror::Error; +use crate::query::subgraph::check::types::CompositionError; + /// RoverClientError represents all possible failures that can occur during a client request. #[derive(Error, Debug)] pub enum RoverClientError { @@ -103,6 +105,12 @@ pub enum RoverClientError { graph: String, composition_errors: Vec, }, + + #[error("{}", subgraph_composition_error_msg(.composition_errors))] + SubgraphCompositionErrors { + graph_name: String, + composition_errors: Vec + }, /// This error occurs when the Studio API returns no implementing services for a graph /// This response shouldn't be possible! @@ -141,3 +149,16 @@ pub enum RoverClientError { #[error("This endpoint doesn't support subgraph introspection via the Query._service field")] SubgraphIntrospectionNotAvailable, } + +fn subgraph_composition_error_msg(composition_errors: &[CompositionError]) -> String { + let num_failures = composition_errors.len(); + if num_failures == 0 { + unreachable!("No composition errors were encountered while composing the supergraph."); + } + let mut msg = String::new(); + msg.push_str(&match num_failures { + 1 => "Encountered 1 composition error while composing the supergraph.".to_string(), + _ => format!("Encountered {} composition errors while composing the supergraph.", num_failures) + }); + msg +} \ No newline at end of file diff --git a/crates/rover-client/src/query/subgraph/check.graphql b/crates/rover-client/src/query/subgraph/check/check_query.graphql similarity index 86% rename from crates/rover-client/src/query/subgraph/check.graphql rename to crates/rover-client/src/query/subgraph/check/check_query.graphql index 5037bd561..62a42dc28 100644 --- a/crates/rover-client/src/query/subgraph/check.graphql +++ b/crates/rover-client/src/query/subgraph/check/check_query.graphql @@ -1,4 +1,4 @@ - mutation CheckPartialSchemaQuery( + mutation SubgraphCheckQuery( $graph_id: ID! $variant: String! $implementingServiceName: String! @@ -17,6 +17,11 @@ compositionValidationResult { errors { message + code + locations { + line + column + } } } checkSchemaResult { diff --git a/crates/rover-client/src/query/subgraph/check/mod.rs b/crates/rover-client/src/query/subgraph/check/mod.rs new file mode 100644 index 000000000..e26e7a17f --- /dev/null +++ b/crates/rover-client/src/query/subgraph/check/mod.rs @@ -0,0 +1,3 @@ +pub mod query_runner; +pub(crate) mod types; +pub use types::SubgraphCheckResponse; \ No newline at end of file diff --git a/crates/rover-client/src/query/subgraph/check.rs b/crates/rover-client/src/query/subgraph/check/query_runner.rs similarity index 50% rename from crates/rover-client/src/query/subgraph/check.rs rename to crates/rover-client/src/query/subgraph/check/query_runner.rs index 7291bde2b..e2af265c2 100644 --- a/crates/rover-client/src/query/subgraph/check.rs +++ b/crates/rover-client/src/query/subgraph/check/query_runner.rs @@ -1,33 +1,31 @@ +use graphql_client::*; use crate::blocking::StudioClient; use crate::query::config::is_federated; use crate::RoverClientError; +use super::types::*; -use graphql_client::*; - -use reqwest::Url; - -type Timestamp = String; #[derive(GraphQLQuery)] // The paths are relative to the directory where your `Cargo.toml` is located. // Both json and the GraphQL schema language are supported as sources for the schema #[graphql( - query_path = "src/query/subgraph/check.graphql", - schema_path = ".schema/schema.graphql", - response_derives = "PartialEq, Debug, Serialize, Deserialize", - deprecated = "warn" + query_path = "src/query/subgraph/check/check_query.graphql", + schema_path = ".schema/schema.graphql", + response_derives = "PartialEq, Debug, Serialize, Deserialize", + deprecated = "warn" )] /// This struct is used to generate the module containing `Variables` and /// `ResponseData` structs. -/// Snake case of this name is the mod name. i.e. check_partial_schema_query -pub struct CheckPartialSchemaQuery; +/// Snake case of this name is the mod name. i.e. subgraph_check_query +pub struct SubgraphCheckQuery; + /// The main function to be used from this module. /// This function takes a proposed schema and validates it against a published /// schema. pub fn run( - variables: check_partial_schema_query::Variables, + variables: subgraph_check_query::Variables, client: &StudioClient, -) -> Result { +) -> Result { let graph = variables.graph_id.clone(); // This response is used to check whether or not the current graph is federated. let is_federated = is_federated::run( @@ -43,77 +41,67 @@ pub fn run( can_operation_convert: false, }); } - let data = client.post::(variables)?; + let data = client.post::(variables)?; get_check_response_from_data(data, graph) } -pub enum CheckResponse { - CompositionErrors(Vec), - CheckResult(CheckResult) -} - -#[derive(Debug)] -pub struct CheckResult { - pub target_url: Option, - pub number_of_checked_operations: i64, - pub change_severity: check_partial_schema_query::ChangeSeverity, - pub changes: Vec, -} - fn get_check_response_from_data( - data: check_partial_schema_query::ResponseData, - graph: String, -) -> Result { - let service = data.service.ok_or(RoverClientError::NoService { graph })?; + data: subgraph_check_query::ResponseData, + graph_name: String, +) -> Result { + let service = data.service.ok_or(RoverClientError::NoService { graph: graph_name.clone() })?; // for some reason this is a `Vec>` // we convert this to just `Vec` because the `None` // errors would be useless. - let composition_errors: Vec = service + let query_composition_errors: Vec = service .check_partial_schema .composition_validation_result .errors; - if composition_errors.is_empty() { + if query_composition_errors.is_empty() { let check_schema_result = service.check_partial_schema.check_schema_result.ok_or( RoverClientError::MalformedResponse { null_field: "service.check_partial_schema.check_schema_result".to_string(), }, )?; - let target_url = get_url(check_schema_result.target_url); + let target_url = check_schema_result.target_url.map(|u| u.to_string()); let diff_to_previous = check_schema_result.diff_to_previous; let number_of_checked_operations = diff_to_previous.number_of_checked_operations.unwrap_or(0); - let change_severity = diff_to_previous.severity; - let changes = diff_to_previous.changes; + let change_severity = diff_to_previous.severity.into(); + + let mut changes = Vec::with_capacity(diff_to_previous.changes.len()); + for change in diff_to_previous.changes { + changes.push(SchemaChange { + code: change.code, + severity: change.severity.into(), + description: change.description + }); + } - let check_result = CheckResult { + let check_result = SubgraphCheckResponse { target_url, number_of_checked_operations, - change_severity, changes, + change_severity }; - Ok(CheckResponse::CheckResult(check_result)) + Ok(check_result) } else { - Ok(CheckResponse::CompositionErrors(composition_errors)) - } -} - -fn get_url(url: Option) -> Option { - match url { - Some(url) => { - let url = Url::parse(&url); - match url { - Ok(url) => Some(url), - // if the API returns an invalid URL, don't put it in the response - Err(_) => None, - } + let num_failures = query_composition_errors.len(); + + let mut composition_errors = Vec::with_capacity(num_failures); + for query_composition_error in query_composition_errors { + composition_errors.push(CompositionError { + message: query_composition_error.message, + code: query_composition_error.code + }); } - None => None, + Err(RoverClientError::SubgraphCompositionErrors { graph_name, composition_errors }) } } diff --git a/crates/rover-client/src/query/subgraph/check/types.rs b/crates/rover-client/src/query/subgraph/check/types.rs new file mode 100644 index 000000000..7e5787d35 --- /dev/null +++ b/crates/rover-client/src/query/subgraph/check/types.rs @@ -0,0 +1,53 @@ +use std::fmt; + +use super::query_runner::subgraph_check_query; + +pub(crate) type Timestamp = String; +type QueryChangeSeverity = subgraph_check_query::ChangeSeverity; + +#[derive(Debug, Clone, PartialEq)] +pub struct SubgraphCheckResponse { + pub target_url: Option, + pub number_of_checked_operations: i64, + pub changes: Vec, + pub change_severity: ChangeSeverity +} + +#[derive(Debug, Clone, PartialEq)] +pub enum ChangeSeverity { + PASS, + FAIL, +} + +impl fmt::Display for ChangeSeverity { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let msg = match self { + ChangeSeverity::PASS => "PASS", + ChangeSeverity::FAIL => "FAIL" + }; + write!(f, "{}", msg) + } +} + +impl From for ChangeSeverity { + fn from(severity: QueryChangeSeverity) -> Self { + match severity { + QueryChangeSeverity::NOTICE => ChangeSeverity::PASS, + QueryChangeSeverity::FAILURE => ChangeSeverity::FAIL, + _ => unreachable!("Unknown change severity") + } + } +} + +#[derive(Debug, Clone, PartialEq)] +pub struct SchemaChange { + pub code: String, + pub description: String, + pub severity: ChangeSeverity +} + +#[derive(Debug, Clone, PartialEq)] +pub struct CompositionError { + pub message: String, + pub code: Option +} \ No newline at end of file diff --git a/docs/source/errors.md b/docs/source/errors.md index 5128e95da..b4ad876d5 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -233,4 +233,12 @@ This error occurs when Rover could not connect to an HTTP endpoint. If you encountered this error while running introspection, you'll want to make sure that you typed the endpoint correctly, your Internet connection is stable, and that your server is responding to requests. You may wish to run the command again with `--log=debug`. +### E029 + +This error occurs when you propose a subgraph schema that could not be composed. + +There are many reasons why you may run into composition errors. This error should include information about _why_ the proposed subgraph schema could not be composed. Error code references can be found [here](https://www.apollographql.com/docs/federation/errors/). + +Some composition errors are part of normal workflows. For instance, you may need to publish a subgraph that does not compose if you are trying to [migrate an entity or field](https://www.apollographql.com/docs/federation/entities/#migrating-entities-and-fields-advanced). + diff --git a/src/command/output.rs b/src/command/output.rs index ba5d6fae5..0ed0ce984 100644 --- a/src/command/output.rs +++ b/src/command/output.rs @@ -5,7 +5,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::query::subgraph::list::ListDetails; +use rover_client::query::subgraph::{check::SubgraphCheckResponse, list::ListDetails}; use termimad::MadSkin; /// RoverStdout defines all of the different types of data that are printed @@ -24,6 +24,7 @@ pub enum RoverStdout { CoreSchema(String), SchemaHash(String), SubgraphList(ListDetails), + SubgraphCheck(SubgraphCheckResponse), VariantList(Vec), Profiles(Vec), Introspection(String), @@ -97,6 +98,36 @@ 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 { diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index d4e85770a..983681fca 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -1,8 +1,7 @@ -use ansi_term::Colour::Red; use serde::Serialize; use structopt::StructOpt; -use rover_client::query::subgraph::check; +use rover_client::query::subgraph::check::{query_runner::{self, subgraph_check_query}}; use crate::command::RoverStdout; use crate::utils::client::StudioClientConfig; @@ -12,7 +11,6 @@ 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)] @@ -65,20 +63,22 @@ impl Check { let sdl = load_schema_from_flag(&self.schema, std::io::stdin())?; - let partial_schema = check::check_partial_schema_query::PartialSchemaInput { + let partial_schema = subgraph_check_query::PartialSchemaInput { sdl: Some(sdl), // we never need to send the hash since the back end computes it from SDL hash: None, }; + + eprintln!("Checking the proposed schema for subgraph {} against {}", &self.subgraph, &self.graph); - let res = check::run( - check::check_partial_schema_query::Variables { + let res = query_runner::run( + subgraph_check_query::Variables { graph_id: self.graph.name.clone(), variant: self.graph.variant.clone(), partial_schema, implementing_service_name: self.subgraph.clone(), git_context: git_context.into(), - config: check::check_partial_schema_query::HistoricQueryParameters { + config: subgraph_check_query::HistoricQueryParameters { query_count_threshold: self.query_count_threshold, query_count_threshold_percentage: self.query_percentage_threshold, from: self.validation_period.clone().unwrap_or_default().from, @@ -92,84 +92,6 @@ impl Check { &client, )?; - eprintln!("Checked the proposed subgraph against {}", &self.graph); - - match res { - check::CheckResponse::CompositionErrors(composition_errors) => { - handle_composition_errors(&composition_errors) - } - check::CheckResponse::CheckResult(check_result) => handle_checks(check_result), - } - } -} - -fn handle_checks(check_result: check::CheckResult) -> Result { - let num_changes = check_result.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_result.changes.len(), - check_result.number_of_checked_operations - ), - }; - - eprintln!("{}", &msg); - - let mut num_failures = 0; - - if !check_result.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_result.changes { - let change = match check.severity { - check::check_partial_schema_query::ChangeSeverity::NOTICE => "PASS", - check::check_partial_schema_query::ChangeSeverity::FAILURE => { - num_failures += 1; - "FAIL" - } - _ => unreachable!("Unknown change severity"), - }; - table.add_row(row![change, check.code, check.description]); - } - - eprintln!("{}", table); - } - - if let Some(url) = check_result.target_url { - eprintln!("View full details at {}", &url); - } - - match num_failures { - 0 => Ok(RoverStdout::None), - 1 => Err(anyhow::anyhow!("Encountered 1 failure while checking your subgraph.").into()), - _ => Err(anyhow::anyhow!( - "Encountered {} failures while checking your subgraph.", - num_failures - ) - .into()), - } -} - -fn handle_composition_errors( - composition_errors: &[check::check_partial_schema_query::CheckPartialSchemaQueryServiceCheckPartialSchemaCompositionValidationResultErrors], -) -> Result { - let num_failures = composition_errors.len(); - for error in composition_errors { - eprintln!("{} {}", Red.bold().paint("error:"), &error.message); - } - match num_failures { - 0 => Ok(RoverStdout::None), - 1 => Err( - anyhow::anyhow!("Encountered 1 composition error while composing the subgraph.").into(), - ), - _ => Err(anyhow::anyhow!( - "Encountered {} composition errors while composing the subgraph.", - num_failures - ) - .into()), + Ok(RoverStdout::SubgraphCheck(res)) } } diff --git a/src/error/metadata/code.rs b/src/error/metadata/code.rs index 07e09efb7..c50932b4b 100644 --- a/src/error/metadata/code.rs +++ b/src/error/metadata/code.rs @@ -34,6 +34,7 @@ pub enum Code { E026, E027, E028, + E029, } impl Display for Code { @@ -75,6 +76,7 @@ impl Code { (Code::E026, include_str!("./codes/E026.md").to_string()), (Code::E027, include_str!("./codes/E027.md").to_string()), (Code::E028, include_str!("./codes/E028.md").to_string()), + (Code::E029, include_str!("./codes/E029.md").to_string()), ]; contents.into_iter().collect() } diff --git a/src/error/metadata/codes/E028.md b/src/error/metadata/codes/E028.md index 52b1dd800..c31e8ea2f 100644 --- a/src/error/metadata/codes/E028.md +++ b/src/error/metadata/codes/E028.md @@ -1,3 +1,3 @@ This error occurs when Rover could not connect to an HTTP endpoint. -If you encountered this error while running introspection, you'll want to make sure that you typed the endpoint correctly, your Internet connection is stable, and that your server is responding to requests. You may wish to run the command again with `--log=debug`. +If you encountered this error while running introspection, you'll want to make sure that you typed the endpoint correctly, your Internet connection is stable, and that your server is responding to requests. You may wish to run the command again with `--log=debug`. \ No newline at end of file diff --git a/src/error/metadata/codes/E029.md b/src/error/metadata/codes/E029.md new file mode 100644 index 000000000..793b1022b --- /dev/null +++ b/src/error/metadata/codes/E029.md @@ -0,0 +1,5 @@ +This error occurs when you propose a subgraph schema that could not be composed. + +There are many reasons why you may run into composition errors. This error should include information about _why_ the proposed subgraph schema could not be composed. Error code references can be found [here](https://www.apollographql.com/docs/federation/errors/). + +Some composition errors are part of normal workflows. For instance, you may need to publish a subgraph that does not compose if you are trying to [migrate an entity or field](https://www.apollographql.com/docs/federation/entities/#migrating-entities-and-fields-advanced). diff --git a/src/error/metadata/mod.rs b/src/error/metadata/mod.rs index 8bc0d67ad..6917e6189 100644 --- a/src/error/metadata/mod.rs +++ b/src/error/metadata/mod.rs @@ -58,6 +58,14 @@ impl From<&mut anyhow::Error> for Metadata { RoverClientError::InvalidSeverity => { (Some(Suggestion::SubmitIssue), Some(Code::E006)) } + RoverClientError::SubgraphCompositionErrors { graph_name, composition_errors } => { + for composition_error in composition_errors { + let code = format!("error[{}]: ", composition_error.code.as_ref().unwrap_or(&"UNKNOWN".to_string())); + let color_code = Red.bold().paint(&code); + eprintln!("{}{}", &color_code, &composition_error.message); + } + (Some(Suggestion::FixSubgraphSchema { graph_name: graph_name.clone() }), Some(Code::E029)) + } RoverClientError::SubgraphIntrospectionNotAvailable => { (Some(Suggestion::UseFederatedGraph), Some(Code::E007)) } diff --git a/src/error/metadata/suggestion.rs b/src/error/metadata/suggestion.rs index 36a234eb4..48b04e43c 100644 --- a/src/error/metadata/suggestion.rs +++ b/src/error/metadata/suggestion.rs @@ -32,6 +32,7 @@ pub enum Suggestion { CheckServerConnection, ConvertGraphToSubgraph, CheckGnuVersion, + FixSubgraphSchema { graph_name: String} } impl Display for Suggestion { @@ -129,6 +130,7 @@ impl Display for Suggestion { Suggestion::CheckServerConnection => "Make sure the endpoint is accepting connections and is spelled correctly".to_string(), Suggestion::ConvertGraphToSubgraph => "If you are sure you want to convert a non-federated graph to a subgraph, you can re-run the same command with a `--convert` flag.".to_string(), Suggestion::CheckGnuVersion => "This is likely an issue with your current version of `glibc`. Try running `ldd --version`, and if the version >= 2.18, we suggest installing the Rover binary built for `x86_64-unknown-linux-gnu`".to_string(), + Suggestion::FixSubgraphSchema { graph_name } => format!("The changes in the schema you proposed are incompatible with graph {}. See {} for more information on resolving composition errors.", Yellow.normal().paint(graph_name), Cyan.normal().paint("https://www.apollographql.com/docs/federation/errors/")) }; write!(formatter, "{}", &suggestion) } diff --git a/src/utils/git.rs b/src/utils/git.rs index 48b4ffd37..cc19fb34f 100644 --- a/src/utils/git.rs +++ b/src/utils/git.rs @@ -171,7 +171,7 @@ impl From for SubgraphPublishContextInput { } } -type SubgraphCheckContextInput = subgraph::check::check_partial_schema_query::GitContextInput; +type SubgraphCheckContextInput = subgraph::check::query_runner::subgraph_check_query::GitContextInput; impl From for SubgraphCheckContextInput { fn from(git_context: GitContext) -> SubgraphCheckContextInput { SubgraphCheckContextInput {