From d68f44cc9477db78fc3cec4e373fe5b2f3afb5d3 Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Fri, 4 Oct 2024 13:43:56 +0200 Subject: [PATCH] fix: use variance merge for ConfigModule (#2957) --- src/core/config/config_module.rs | 8 +- src/core/config/config_module/merge.rs | 10 +- ...dule__merge__tests__federation_router.snap | 2 +- src/core/config/reader.rs | 27 ++-- src/core/mod.rs | 2 +- tailcall-wasm/src/builder.rs | 7 +- .../snapshots/test-merge-input.md_merged.snap | 15 ++ .../test-merge-invalid.md_error.snap | 14 ++ tests/core/spec.rs | 144 +++++++----------- tests/execution/test-merge-input.md | 31 ++++ tests/execution/test-merge-invalid.md | 33 ++++ 11 files changed, 182 insertions(+), 111 deletions(-) create mode 100644 tests/core/snapshots/test-merge-input.md_merged.snap create mode 100644 tests/core/snapshots/test-merge-invalid.md_error.snap create mode 100644 tests/execution/test-merge-input.md create mode 100644 tests/execution/test-merge-invalid.md diff --git a/src/core/config/config_module.rs b/src/core/config/config_module.rs index 00e737af5d..0a46ab98ca 100644 --- a/src/core/config/config_module.rs +++ b/src/core/config/config_module.rs @@ -17,7 +17,7 @@ mod merge; /// A wrapper on top of Config that contains all the resolved extensions and /// computed values. -#[derive(Clone, Debug, Default, MergeRight)] +#[derive(Clone, Debug, Default)] pub struct ConfigModule { extensions: Extensions, cache: Cache, @@ -48,12 +48,6 @@ impl From for Cache { } } -impl MergeRight for Cache { - fn merge_right(self, other: Self) -> Self { - Cache::from(self.config.merge_right(other.config)) - } -} - impl ConfigModule { pub fn new(config: Config, extensions: Extensions) -> Self { ConfigModule { cache: Cache::from(config), extensions } diff --git a/src/core/config/config_module/merge.rs b/src/core/config/config_module/merge.rs index 357f3e3d17..7d9c5abfd4 100644 --- a/src/core/config/config_module/merge.rs +++ b/src/core/config/config_module/merge.rs @@ -270,7 +270,8 @@ impl Invariant for Cache { types.extend(merged_types); enums.extend(merged_enums); - let config = Config { types, enums, unions: self.config.unions.merge_right(other.config.unions), ..self.config }; + let config = Config { + types, enums, unions: self.config.unions.merge_right(other.config.unions), server: self.config.server.merge_right(other.config.server), upstream: self.config.upstream.merge_right(other.config.upstream), schema: self.config.schema.merge_right(other.config.schema), links: self.config.links.merge_right(other.config.links), telemetry: self.config.telemetry.merge_right(other.config.telemetry) }; Cache { config, @@ -284,9 +285,10 @@ impl Invariant for Cache { impl Invariant for ConfigModule { fn unify(self, other: Self) -> Valid { - self.cache - .unify(other.cache) - .map(|cache| Self { cache, extensions: self.extensions }) + self.cache.unify(other.cache).map(|cache| Self { + cache, + extensions: self.extensions.merge_right(other.extensions), + }) } } diff --git a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap index 16f3b73062..a514247c2d 100644 --- a/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap +++ b/src/core/config/config_module/snapshots/tailcall__core__config__config_module__merge__tests__federation_router.snap @@ -2,7 +2,7 @@ source: src/core/config/config_module/merge.rs expression: merged.to_sdl() --- -schema @server @upstream { +schema @server(port: 8000) @upstream(baseURL: "http://jsonplaceholder.typicode.com", batch: {delay: 100, headers: []}, httpCache: 42) { query: Query } diff --git a/src/core/config/reader.rs b/src/core/config/reader.rs index cd18936b7d..5eb4b31494 100644 --- a/src/core/config/reader.rs +++ b/src/core/config/reader.rs @@ -8,11 +8,12 @@ use url::Url; use super::{ConfigModule, Content, Link, LinkType, PrivateKey}; use crate::core::config::{Config, ConfigReaderContext, Source}; -use crate::core::merge_right::MergeRight; use crate::core::proto_reader::ProtoReader; use crate::core::resource_reader::{Cached, Resource, ResourceReader}; use crate::core::rest::EndpointSet; use crate::core::runtime::TargetRuntime; +use crate::core::valid::{Valid, Validator}; +use crate::core::variance::Invariant; /// Reads the configuration from a file or from an HTTP URL and resolves all /// linked extensions to create a ConfigModule. @@ -36,7 +37,7 @@ impl ConfigReader { #[async_recursion::async_recursion] async fn ext_links( &self, - mut config_module: ConfigModule, + config_module: ConfigModule, parent_dir: Option<&'async_recursion Path>, ) -> anyhow::Result { let links: Vec = config_module @@ -57,6 +58,8 @@ impl ConfigReader { } let mut extensions = config_module.extensions().clone(); + let mut config_module = Valid::succeed(config_module); + // let mut base_config = config_module.config().clone(); for link in links.iter() { @@ -68,13 +71,16 @@ impl ConfigReader { let content = source.content; let config = Config::from_source(Source::detect(&source.path)?, &content)?; - config_module = config_module.merge_right(config.clone().into()); + config_module = config_module.and_then(|config_module| { + config_module.unify(ConfigModule::from(config.clone())) + }); if !config.links.is_empty() { let cfg_module = self .ext_links(ConfigModule::from(config), Path::new(&link.src).parent()) .await?; - config_module = config_module.merge_right(cfg_module.clone()); + config_module = + config_module.and_then(|config_module| config_module.unify(cfg_module)); } } LinkType::Protobuf => { @@ -134,9 +140,9 @@ impl ConfigReader { } } - // Recreating the ConfigModule in order to recompute the values of - // `input_types`, `output_types` and `interface_types` - Ok(config_module.set_extensions(extensions)) + Ok(config_module + .map(|config_module| config_module.set_extensions(extensions)) + .to_result()?) } /// Reads the certificate from a given file @@ -182,7 +188,7 @@ impl ConfigReader { files: &[T], ) -> anyhow::Result { let files = self.resource_reader.read_files(files).await?; - let mut config_module = ConfigModule::default(); + let mut config_module = Valid::succeed(ConfigModule::default()); for file in files.iter() { let source = Source::detect(&file.path)?; @@ -197,10 +203,11 @@ impl ConfigReader { .await?; // Merge it with the original config set - config_module = config_module.merge_right(new_config_module); + config_module = + config_module.and_then(|config_module| config_module.unify(new_config_module)); } - Ok(config_module) + Ok(config_module.to_result()?) } /// Resolves all the links in a Config to create a ConfigModule diff --git a/src/core/mod.rs b/src/core/mod.rs index b219127e6f..e91554b001 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -40,7 +40,7 @@ pub mod tracing; mod transform; pub mod try_fold; pub mod valid; -mod variance; +pub mod variance; pub mod worker; mod wrapping_type; diff --git a/tailcall-wasm/src/builder.rs b/tailcall-wasm/src/builder.rs index 069b086dfc..df30b7d739 100644 --- a/tailcall-wasm/src/builder.rs +++ b/tailcall-wasm/src/builder.rs @@ -7,6 +7,8 @@ use tailcall::core::config::ConfigModule; use tailcall::core::merge_right::MergeRight; use tailcall::core::rest::EndpointSet; use tailcall::core::runtime::TargetRuntime; +use tailcall::core::valid::Validator; +use tailcall::core::variance::Invariant; use wasm_bindgen::prelude::wasm_bindgen; use wasm_bindgen::JsValue; @@ -40,7 +42,10 @@ impl TailcallBuilder { async fn with_config_inner(mut self, url: String) -> anyhow::Result { if url::Url::parse(&url).is_ok() { - self.module = self.module.merge_right(self.reader.read(url).await?); + self.module = self + .module + .unify(self.reader.read(url).await?) + .to_result()?; } else { return Err(anyhow::anyhow!("Config can only be loaded over URL")); } diff --git a/tests/core/snapshots/test-merge-input.md_merged.snap b/tests/core/snapshots/test-merge-input.md_merged.snap new file mode 100644 index 0000000000..9811970ff7 --- /dev/null +++ b/tests/core/snapshots/test-merge-input.md_merged.snap @@ -0,0 +1,15 @@ +--- +source: tests/core/spec.rs +expression: formatter +--- +schema @server @upstream(baseURL: "http://jsonplacheholder.typicode.com") { + query: Query +} + +input Test { + b: String +} + +type Query { + foo(x: Test): Boolean @http(path: "/foo") +} diff --git a/tests/core/snapshots/test-merge-invalid.md_error.snap b/tests/core/snapshots/test-merge-invalid.md_error.snap new file mode 100644 index 0000000000..e51fab4b42 --- /dev/null +++ b/tests/core/snapshots/test-merge-invalid.md_error.snap @@ -0,0 +1,14 @@ +--- +source: tests/core/spec.rs +expression: errors +--- +[ + { + "message": "Type mismatch: expected `String`, got `Int`", + "trace": [ + "Foo", + "a" + ], + "description": null + } +] diff --git a/tests/core/spec.rs b/tests/core/spec.rs index be287d044c..b69499b57f 100644 --- a/tests/core/spec.rs +++ b/tests/core/spec.rs @@ -18,9 +18,9 @@ use tailcall::core::config::reader::ConfigReader; use tailcall::core::config::transformer::Required; use tailcall::core::config::{Config, ConfigModule, Source}; use tailcall::core::http::handle_request; -use tailcall::core::merge_right::MergeRight; use tailcall::core::print_schema::print_schema; -use tailcall::core::valid::{Cause, ValidationError, Validator}; +use tailcall::core::valid::{Cause, Valid, ValidationError, Validator}; +use tailcall::core::variance::Invariant; use tailcall_prettier::Parser; use super::file::File; @@ -56,27 +56,12 @@ impl From> for SDLError { } } -async fn is_sdl_error(spec: ExecutionSpec, mock_http_client: Arc) -> bool { +async fn is_sdl_error(spec: &ExecutionSpec, config_module: Valid) -> bool { if spec.sdl_error { // errors: errors are expected, make sure they match - let (source, content) = &spec.server[0]; - - let config = source.decode(content); - - let config = match config { - Ok(config) => { - let mut runtime = runtime::create_runtime(mock_http_client, spec.env.clone(), None); - runtime.file = Arc::new(File::new(spec.clone())); - let reader = ConfigReader::init(runtime); - match reader.resolve(config, spec.path.parent()).await { - Ok(config) => Blueprint::try_from(&config), - Err(e) => Err(ValidationError::new(e.to_string())), - } - } - Err(e) => Err(e), - }; + let blueprint = config_module.and_then(|cfg| Valid::from(Blueprint::try_from(&cfg))); - match config { + match blueprint.to_result() { Ok(_) => { tracing::error!("\terror FAIL"); panic!( @@ -84,9 +69,9 @@ async fn is_sdl_error(spec: ExecutionSpec, mock_http_client: Arc) -> bool spec.name, spec.path ); } - Err(cause) => { + Err(error) => { let errors: Vec = - cause.as_vec().iter().map(|e| e.to_owned().into()).collect(); + error.as_vec().iter().map(|e| e.to_owned().into()).collect(); let snapshot_name = format!("{}_error", spec.safe_name); @@ -99,29 +84,17 @@ async fn is_sdl_error(spec: ExecutionSpec, mock_http_client: Arc) -> bool false } -async fn check_server_config(spec: ExecutionSpec) -> Vec { - let mut server: Vec = Vec::with_capacity(spec.server.len()); - - for (i, (source, content)) in spec.server.iter().enumerate() { - let config = Config::from_source(source.to_owned(), content).unwrap_or_else(|e| { - panic!( - "Couldn't parse GraphQL in server definition #{} of {:#?}: {}", - i + 1, - spec.path, - e - ) - }); - - let config = Config::default().merge_right(config); - - // TODO: we should probably figure out a way to do this for every test - // but GraphQL identity checking is very hard, since a lot depends on the code - // style the re-serializing check gives us some of the advantages of the - // identity check too, but we are missing out on some by having it only - // enabled for either new tests that request it or old graphql_spec - // tests that were explicitly written with it in mind - if spec.check_identity { +async fn check_identity(spec: &ExecutionSpec) { + // TODO: we should probably figure out a way to do this for every test + // but GraphQL identity checking is very hard, since a lot depends on the code + // style the re-serializing check gives us some of the advantages of the + // identity check too, but we are missing out on some by having it only + // enabled for either new tests that request it or old graphql_spec + // tests that were explicitly written with it in mind + if spec.check_identity { + for (source, content) in spec.server.iter() { if matches!(source, Source::GraphQL) { + let config = Config::from_source(source.to_owned(), content).unwrap(); let actual = config.to_sdl(); // \r is added automatically in windows, it's safe to replace it with \n @@ -153,10 +126,7 @@ async fn check_server_config(spec: ExecutionSpec) -> Vec { ); } } - - server.push(config); } - server } async fn run_query_tests_on_spec( @@ -215,48 +185,41 @@ async fn test_spec(spec: ExecutionSpec) { let mock_http_client = Arc::new(Http::new(&spec)); - // check sdl error if any - if is_sdl_error(spec.clone(), mock_http_client.clone()).await { - return; - } - - // Parse and validate all server configs + check for identity - let server = check_server_config(spec.clone()).await; - - // Resolve all configs let mut runtime = runtime::create_runtime(mock_http_client.clone(), spec.env.clone(), None); runtime.file = Arc::new(File::new(spec.clone())); let reader = ConfigReader::init(runtime); - let server: Vec = join_all( - server - .into_iter() - .map(|config| reader.resolve(config, spec.path.parent())), - ) - .await - .into_iter() - .enumerate() - .map(|(i, result)| { - result.unwrap_or_else(|e| { - panic!( - "Couldn't resolve GraphQL in server definition #{} of {:#?}: {}", - i + 1, - spec.path, - e - ) - }) + // Resolve all configs + let config_modules = join_all(spec.server.iter().map(|(source, content)| async { + let config = Config::from_source(source.to_owned(), content)?; + + reader.resolve(config, spec.path.parent()).await + })) + .await; + + let config_module = Valid::from_iter(config_modules.iter(), |config_module| { + Valid::from(config_module.as_ref().map_err(|e| { + match e.downcast_ref::>() { + Some(err) => err.clone(), + None => ValidationError::new(e.to_string()), + } + })) + }) + .and_then(|cfgs| { + cfgs.into_iter() + .fold(Valid::succeed(ConfigModule::default()), |acc, c| { + acc.and_then(|acc| acc.unify(c.clone())) + }) }) - .collect(); - - // merged: Run merged specs - let merged = server - .iter() - .fold(ConfigModule::default(), |acc, c| acc.merge_right(c.clone())) - // Apply required transformers to the configuration - .transform(Required) - .to_result() - .unwrap() - .to_sdl(); + // Apply required transformers to the configuration + .and_then(|cfg| cfg.transform(Required)); + + // check sdl error if any + if is_sdl_error(&spec, config_module.clone()).await { + return; + } + + let merged = config_module.to_result().unwrap().to_sdl(); let formatter = tailcall_prettier::format(merged, &Parser::Gql) .await @@ -266,9 +229,16 @@ async fn test_spec(spec: ExecutionSpec) { insta::assert_snapshot!(snapshot_name, formatter); + let config_modules = config_modules + .into_iter() + .collect::, _>>() + .unwrap(); + + check_identity(&spec).await; + // client: Check if client spec matches snapshot - if server.len() == 1 { - let config = &server[0]; + if config_modules.len() == 1 { + let config = &config_modules[0]; let client = print_schema( (Blueprint::try_from(config) @@ -286,7 +256,7 @@ async fn test_spec(spec: ExecutionSpec) { } // run query tests - run_query_tests_on_spec(spec, server, mock_http_client).await; + run_query_tests_on_spec(spec, config_modules, mock_http_client).await; } pub async fn load_and_test_execution_spec(path: &Path) -> anyhow::Result<()> { diff --git a/tests/execution/test-merge-input.md b/tests/execution/test-merge-input.md new file mode 100644 index 0000000000..fe7e9e54d9 --- /dev/null +++ b/tests/execution/test-merge-input.md @@ -0,0 +1,31 @@ +# Test merge input + +```graphql @config +schema @server @upstream(baseURL: "http://jsonplacheholder.typicode.com") { + query: Query +} + +input Test { + a: Int + b: String +} + +type Query { + foo(x: Test): Boolean @http(path: "/foo") +} +``` + +```graphql @config +schema @server @upstream(baseURL: "http://jsonplacheholder.typicode.com") { + query: Query +} + +input Test { + b: String + c: Boolean +} + +type Query { + foo(x: Test): Boolean @http(path: "/foo") +} +``` diff --git a/tests/execution/test-merge-invalid.md b/tests/execution/test-merge-invalid.md new file mode 100644 index 0000000000..09167f9f74 --- /dev/null +++ b/tests/execution/test-merge-invalid.md @@ -0,0 +1,33 @@ +--- +error: true +--- + +# Test merge error + +```graphql @config +schema @server @upstream(baseURL: "http://abc.com") { + query: Query +} + +type Query { + hi: Foo @expr(body: {a: "world"}) +} + +type Foo { + a: String +} +``` + +```graphql @config +schema @server { + query: Query +} + +type Query { + hi: Foo @expr(body: {a: "world"}) +} + +type Foo { + a: Int +} +```