From f6820ef4a2792fd94fc71c86bea8b1417b3ce625 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Tue, 2 Feb 2021 14:16:36 -0500 Subject: [PATCH 1/8] feat(checks): add flag parsers --- src/utils/parsers.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/utils/parsers.rs b/src/utils/parsers.rs index 981a6a759..79572abf3 100644 --- a/src/utils/parsers.rs +++ b/src/utils/parsers.rs @@ -1,4 +1,5 @@ use regex::Regex; +use serde::Serialize; use std::{fmt, path::PathBuf}; use crate::{anyhow, Result}; @@ -60,6 +61,47 @@ pub fn parse_graph_ref(graph_id: &str) -> Result { } } +#[derive(Debug, Serialize, Default, Clone)] +pub struct ValidationPeriod { + pub from: Option, + pub to: Option, +} + +/// Validation period is a positive number of seconds to validate in the past. +// We just need to validate and negate it +pub fn parse_validation_period(period: &str) -> Result { + let window = period.parse::()?; + if window > 0 { + Ok(ValidationPeriod { + from: Some(format!("{}", -window)), + to: Some("-1".to_string()), + }) + } else { + Err( + anyhow!("Invalid validation period. Must be a positive number of seconds.".to_string()) + .into(), + ) + } +} + +pub fn parse_query_count_threshold(threshold: &str) -> Result { + let threshold = threshold.parse::()?; + if threshold < 1 { + Err(anyhow!("Invalid value for query count threshold. Must be a positive integer.").into()) + } else { + Ok(threshold) + } +} + +pub fn parse_query_percentage_threshold(threshold: &str) -> Result { + let threshold = threshold.parse::()?; + if threshold <= 0.0 || threshold >= 1.0 { + Err(anyhow!("Invalid value for query percentage threshold. Must be a positive value greater than 0 and less than 1").into()) + } else { + Ok(threshold) + } +} + #[cfg(test)] mod tests { use super::{parse_graph_ref, parse_schema_source, GraphRef, SchemaSource}; From 3b7504aa3b97b24d45085e8ecf7b55c9f8477b6d Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Tue, 2 Feb 2021 14:17:02 -0500 Subject: [PATCH 2/8] feat(checks): add config options to checks --- .../src/query/graph/check.graphql | 2 ++ crates/rover-client/src/query/graph/check.rs | 1 + .../src/query/subgraph/check.graphql | 2 ++ .../rover-client/src/query/subgraph/check.rs | 1 + src/command/graph/check.rs | 31 ++++++++++++++++++- src/command/subgraph/check.rs | 31 ++++++++++++++++++- 6 files changed, 66 insertions(+), 2 deletions(-) diff --git a/crates/rover-client/src/query/graph/check.graphql b/crates/rover-client/src/query/graph/check.graphql index 6b27d89c3..a891ba7eb 100644 --- a/crates/rover-client/src/query/graph/check.graphql +++ b/crates/rover-client/src/query/graph/check.graphql @@ -3,12 +3,14 @@ mutation CheckSchemaQuery( $variant: String $schema: String $gitContext: GitContextInput! + $config: HistoricQueryParameters! ) { service(id: $graphId) { checkSchema( proposedSchemaDocument: $schema baseSchemaTag: $variant gitContext: $gitContext + historicParameters: $config ) { targetUrl diffToPrevious { diff --git a/crates/rover-client/src/query/graph/check.rs b/crates/rover-client/src/query/graph/check.rs index 64cf89f8e..433de4180 100644 --- a/crates/rover-client/src/query/graph/check.rs +++ b/crates/rover-client/src/query/graph/check.rs @@ -4,6 +4,7 @@ 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 diff --git a/crates/rover-client/src/query/subgraph/check.graphql b/crates/rover-client/src/query/subgraph/check.graphql index aea269ba9..5037bd561 100644 --- a/crates/rover-client/src/query/subgraph/check.graphql +++ b/crates/rover-client/src/query/subgraph/check.graphql @@ -4,6 +4,7 @@ $implementingServiceName: String! $partialSchema: PartialSchemaInput! $gitContext: GitContextInput! + $config: HistoricQueryParameters! ) { service(id: $graph_id) { checkPartialSchema( @@ -11,6 +12,7 @@ implementingServiceName: $implementingServiceName partialSchema: $partialSchema gitContext: $gitContext + historicParameters: $config ) { compositionValidationResult { errors { diff --git a/crates/rover-client/src/query/subgraph/check.rs b/crates/rover-client/src/query/subgraph/check.rs index da19a7298..4c2d9fdd3 100644 --- a/crates/rover-client/src/query/subgraph/check.rs +++ b/crates/rover-client/src/query/subgraph/check.rs @@ -4,6 +4,7 @@ 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 diff --git a/src/command/graph/check.rs b/src/command/graph/check.rs index d7f307d46..67f147560 100644 --- a/src/command/graph/check.rs +++ b/src/command/graph/check.rs @@ -8,7 +8,10 @@ use crate::client::StudioClientConfig; use crate::command::RoverStdout; use crate::git::GitContext; use crate::utils::loaders::load_schema_from_flag; -use crate::utils::parsers::{parse_graph_ref, parse_schema_source, GraphRef, SchemaSource}; +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::Result; #[derive(Debug, Serialize, StructOpt)] @@ -29,6 +32,21 @@ pub struct Check { #[structopt(long, short = "s", parse(try_from_str = parse_schema_source))] #[serde(skip_serializing)] schema: SchemaSource, + + /// Minimum number of requests within the time window for a query to be + /// considered + #[structopt(long, parse(try_from_str = parse_query_count_threshold))] + query_count_threshold: Option, + + /// Minimum percentage of requests within the time window for a query to be + /// considered, relative to total request count. Expected values are 0.0-0.05 + /// (minimum 5% of total request volume) + #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] + query_percentage_threshold: Option, + + /// Size of the time window with which to validate schema against (in seconds) + #[structopt(long, parse(try_from_str = parse_validation_period))] + validation_period: Option, } impl Check { @@ -45,6 +63,17 @@ impl Check { variant: Some(self.graph.variant.clone()), schema: Some(sdl), git_context: git_context.into(), + // TODO: check what happens if a value is none, but existant + config: check::check_schema_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, + 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, + }, }, &client, )?; diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index 3ec8a567b..50ddec10c 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -9,7 +9,10 @@ use crate::client::StudioClientConfig; use crate::command::RoverStdout; use crate::git::GitContext; use crate::utils::loaders::load_schema_from_flag; -use crate::utils::parsers::{parse_graph_ref, parse_schema_source, GraphRef, SchemaSource}; +use crate::utils::parsers::{ + parse_graph_ref, parse_query_count_threshold, parse_query_percentage_threshold, + parse_schema_source, parse_validation_period, GraphRef, SchemaSource, ValidationPeriod, +}; #[derive(Debug, Serialize, StructOpt)] pub struct Check { @@ -34,6 +37,21 @@ pub struct Check { #[structopt(long, short = "s", parse(try_from_str = parse_schema_source))] #[serde(skip_serializing)] schema: SchemaSource, + + /// Minimum number of requests within the time window for a query to be + /// considered + #[structopt(long, parse(try_from_str = parse_query_count_threshold))] + query_count_threshold: Option, + + /// Minimum percentage of requests within the time window for a query to be + /// considered, relative to total request count. Expected values are 0.0-0.05 + /// (minimum 5% of total request volume) + #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] + query_percentage_threshold: Option, + + /// Size of the time window with which to validate schema against (in seconds) + #[structopt(long, parse(try_from_str = parse_validation_period))] + validation_period: Option, } impl Check { @@ -59,6 +77,17 @@ impl Check { partial_schema, implementing_service_name: self.subgraph.clone(), git_context: git_context.into(), + // TODO: check what happens if a value is none, but existant + config: check::check_partial_schema_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, + 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, + }, }, &client, )?; From cd6ef86c976abc71f42f3ae590ad7acee3d074e7 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Tue, 2 Feb 2021 14:22:13 -0500 Subject: [PATCH 3/8] feat(checks): change percentage from decimal to int --- src/command/subgraph/check.rs | 4 ++-- src/utils/parsers.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index 50ddec10c..6f06b28af 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -44,8 +44,8 @@ pub struct Check { query_count_threshold: Option, /// Minimum percentage of requests within the time window for a query to be - /// considered, relative to total request count. Expected values are 0.0-0.05 - /// (minimum 5% of total request volume) + /// considered, relative to total request count. Expected values are 0-5 + /// (i.e. minimum 5% of total request volume) #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option, diff --git a/src/utils/parsers.rs b/src/utils/parsers.rs index 79572abf3..5c6c73374 100644 --- a/src/utils/parsers.rs +++ b/src/utils/parsers.rs @@ -94,11 +94,11 @@ pub fn parse_query_count_threshold(threshold: &str) -> Result { } pub fn parse_query_percentage_threshold(threshold: &str) -> Result { - let threshold = threshold.parse::()?; - if threshold <= 0.0 || threshold >= 1.0 { - Err(anyhow!("Invalid value for query percentage threshold. Must be a positive value greater than 0 and less than 1").into()) + let threshold = threshold.parse::()?; + if threshold <= 0 || threshold >= 100 { + Err(anyhow!("Invalid value for query percentage threshold. Must be a positive integer greater than 0 and less than 100").into()) } else { - Ok(threshold) + Ok((threshold / 100) as f64) } } From d0c1c383b7caa7069cb3d5c9f54c525f8d252817 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Tue, 2 Feb 2021 14:24:09 -0500 Subject: [PATCH 4/8] feat(checks): update doc comment --- src/command/graph/check.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/command/graph/check.rs b/src/command/graph/check.rs index 67f147560..62c2c43fa 100644 --- a/src/command/graph/check.rs +++ b/src/command/graph/check.rs @@ -39,8 +39,8 @@ pub struct Check { query_count_threshold: Option, /// Minimum percentage of requests within the time window for a query to be - /// considered, relative to total request count. Expected values are 0.0-0.05 - /// (minimum 5% of total request volume) + /// considered, relative to total request count. Expected values are 0-5 + /// (i.e. minimum 5% of total request volume) #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option, From e60041f1aa2f89a541d4083de49c319208bdd090 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Wed, 3 Feb 2021 21:31:14 -0500 Subject: [PATCH 5/8] remove old todos --- src/command/graph/check.rs | 1 - src/command/subgraph/check.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/command/graph/check.rs b/src/command/graph/check.rs index 62c2c43fa..c6cc04ff3 100644 --- a/src/command/graph/check.rs +++ b/src/command/graph/check.rs @@ -63,7 +63,6 @@ impl Check { variant: Some(self.graph.variant.clone()), schema: Some(sdl), git_context: git_context.into(), - // TODO: check what happens if a value is none, but existant config: check::check_schema_query::HistoricQueryParameters { query_count_threshold: self.query_count_threshold, query_count_threshold_percentage: self.query_percentage_threshold, diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index 6f06b28af..ecb3de994 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -77,7 +77,6 @@ impl Check { partial_schema, implementing_service_name: self.subgraph.clone(), git_context: git_context.into(), - // TODO: check what happens if a value is none, but existant config: check::check_partial_schema_query::HistoricQueryParameters { query_count_threshold: self.query_count_threshold, query_count_threshold_percentage: self.query_percentage_threshold, From a599407801aa0b4f4b0a72586c3cbf8bea300d54 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Mon, 8 Feb 2021 10:29:18 -0500 Subject: [PATCH 6/8] clarify doc comments, and fix validation rules --- src/command/graph/check.rs | 10 +++++----- src/command/subgraph/check.rs | 10 +++++----- src/utils/parsers.rs | 22 +++++++++++++--------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/command/graph/check.rs b/src/command/graph/check.rs index c6cc04ff3..5e1dd0f6b 100644 --- a/src/command/graph/check.rs +++ b/src/command/graph/check.rs @@ -33,14 +33,14 @@ pub struct Check { #[serde(skip_serializing)] schema: SchemaSource, - /// Minimum number of requests within the time window for a query to be - /// considered + /// Minimum number of times an operation must have happened within the time + /// window for checks be able to fail on that operation #[structopt(long, parse(try_from_str = parse_query_count_threshold))] query_count_threshold: Option, - /// Minimum percentage of requests within the time window for a query to be - /// considered, relative to total request count. Expected values are 0-5 - /// (i.e. minimum 5% of total request volume) + /// Minimum percentage of times an operation must have happend in the time + /// window for checks to fail on that operation, relative to total request + /// count. Valid numbers are in the range 0 <= x <= 100 #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option, diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index ecb3de994..f413875a4 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -38,14 +38,14 @@ pub struct Check { #[serde(skip_serializing)] schema: SchemaSource, - /// Minimum number of requests within the time window for a query to be - /// considered + /// Minimum number of times an operation must have happened within the time + /// window for checks be able to fail on that operation #[structopt(long, parse(try_from_str = parse_query_count_threshold))] query_count_threshold: Option, - /// Minimum percentage of requests within the time window for a query to be - /// considered, relative to total request count. Expected values are 0-5 - /// (i.e. minimum 5% of total request volume) + /// Minimum percentage of times an operation must have happend in the time + /// window for checks to fail on that operation, relative to total request + /// count. Valid numbers are in the range 0 <= x <= 100 #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option, diff --git a/src/utils/parsers.rs b/src/utils/parsers.rs index 5c6c73374..86701c807 100644 --- a/src/utils/parsers.rs +++ b/src/utils/parsers.rs @@ -63,24 +63,28 @@ pub fn parse_graph_ref(graph_id: &str) -> Result { #[derive(Debug, Serialize, Default, Clone)] pub struct ValidationPeriod { + // these timstamps could be represented as i64, but the API expects + // Option pub from: Option, pub to: Option, } -/// Validation period is a positive number of seconds to validate in the past. -// We just need to validate and negate it +// Validation period is a positive number of seconds to validate in the past. +// We just need to validate and negate it. +// +// Valid windows of time to search are only in the past (negative seconds). +// We only support validating "to" now (-0) pub fn parse_validation_period(period: &str) -> Result { let window = period.parse::()?; if window > 0 { Ok(ValidationPeriod { + // search "from" a negative time window from: Some(format!("{}", -window)), - to: Some("-1".to_string()), + // search "to" now (-0) seconds + to: Some("-0".to_string()), }) } else { - Err( - anyhow!("Invalid validation period. Must be a positive number of seconds.".to_string()) - .into(), - ) + Err(anyhow!("Invalid validation period. Must be a positive number of seconds.").into()) } } @@ -95,8 +99,8 @@ pub fn parse_query_count_threshold(threshold: &str) -> Result { pub fn parse_query_percentage_threshold(threshold: &str) -> Result { let threshold = threshold.parse::()?; - if threshold <= 0 || threshold >= 100 { - Err(anyhow!("Invalid value for query percentage threshold. Must be a positive integer greater than 0 and less than 100").into()) + if threshold < 0 || threshold > 100 { + Err(anyhow!("Invalid value for query percentage threshold. Valid numbers are in the range 0 <= x <= 100").into()) } else { Ok((threshold / 100) as f64) } From d1f729dac57c5b831072765ad2b00cb44b8de4b4 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Mon, 8 Feb 2021 10:48:18 -0500 Subject: [PATCH 7/8] lol weird flex, linter, but okay --- src/utils/parsers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/parsers.rs b/src/utils/parsers.rs index 86701c807..ca049d4ad 100644 --- a/src/utils/parsers.rs +++ b/src/utils/parsers.rs @@ -99,7 +99,7 @@ pub fn parse_query_count_threshold(threshold: &str) -> Result { pub fn parse_query_percentage_threshold(threshold: &str) -> Result { let threshold = threshold.parse::()?; - if threshold < 0 || threshold > 100 { + if !(0..=100).contains(&threshold) { Err(anyhow!("Invalid value for query percentage threshold. Valid numbers are in the range 0 <= x <= 100").into()) } else { Ok((threshold / 100) as f64) From 7ff66fa16510fc4d34c17829e900ebcea54151e1 Mon Sep 17 00:00:00 2001 From: Jake Dawkins Date: Mon, 8 Feb 2021 10:52:03 -0500 Subject: [PATCH 8/8] update flag descriptions --- src/command/graph/check.rs | 10 +++++----- src/command/subgraph/check.rs | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/command/graph/check.rs b/src/command/graph/check.rs index d1ff3e70c..79c74a223 100644 --- a/src/command/graph/check.rs +++ b/src/command/graph/check.rs @@ -33,14 +33,14 @@ pub struct Check { #[serde(skip_serializing)] schema: SchemaSource, - /// Minimum number of times an operation must have happened within the time - /// window for checks be able to fail on that operation + /// The minimum number of times a query or mutation must have been executed + /// in order to be considered in the check operation #[structopt(long, parse(try_from_str = parse_query_count_threshold))] query_count_threshold: Option, - /// Minimum percentage of times an operation must have happend in the time - /// window for checks to fail on that operation, relative to total request - /// count. Valid numbers are in the range 0 <= x <= 100 + /// Minimum percentage of times a query or mutation must have been executed + /// in the time window, relative to total request count, for it to be + /// considered in the check. Valid numbers are in the range 0 <= x <= 100 #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option, diff --git a/src/command/subgraph/check.rs b/src/command/subgraph/check.rs index 7f5f9287a..4482fde76 100644 --- a/src/command/subgraph/check.rs +++ b/src/command/subgraph/check.rs @@ -38,14 +38,14 @@ pub struct Check { #[serde(skip_serializing)] schema: SchemaSource, - /// Minimum number of times an operation must have happened within the time - /// window for checks be able to fail on that operation + /// The minimum number of times a query or mutation must have been executed + /// in order to be considered in the check operation #[structopt(long, parse(try_from_str = parse_query_count_threshold))] query_count_threshold: Option, - /// Minimum percentage of times an operation must have happend in the time - /// window for checks to fail on that operation, relative to total request - /// count. Valid numbers are in the range 0 <= x <= 100 + /// Minimum percentage of times a query or mutation must have been executed + /// in the time window, relative to total request count, for it to be + /// considered in the check. Valid numbers are in the range 0 <= x <= 100 #[structopt(long, parse(try_from_str = parse_query_percentage_threshold))] query_percentage_threshold: Option,