From 4680f944aecbdd7d88164592ef39c3209d79e184 Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Wed, 7 Aug 2024 11:34:15 +0200 Subject: [PATCH] fix(jit): union support (#2452) Co-authored-by: Tushar Mathur --- src/core/ir/resolver_context_like.rs | 5 +- src/core/jit/builder.rs | 26 ++- src/core/jit/common/jp.rs | 8 +- src/core/jit/error.rs | 10 +- src/core/jit/exec.rs | 71 +++++-- src/core/jit/exec_const.rs | 13 +- src/core/jit/model.rs | 64 ++++--- ...e__jit__builder__tests__default_value.snap | 2 + ...core__jit__builder__tests__directives.snap | 3 + ..._core__jit__builder__tests__fragments.snap | 20 ++ ...e__jit__builder__tests__from_document.snap | 4 + ...__builder__tests__multiple_operations.snap | 6 + ...builder__tests__resolving_operation-2.snap | 5 + ...__builder__tests__resolving_operation.snap | 4 + ..._jit__builder__tests__simple_mutation.snap | 7 + ...re__jit__builder__tests__simple_query.snap | 3 + ...ll__core__jit__builder__tests__unions.snap | 23 +++ ..._core__jit__builder__tests__variables.snap | 3 + src/core/jit/synth/synth.rs | 180 +++++++++--------- tests/execution/grpc-oneof.md | 2 + 20 files changed, 316 insertions(+), 143 deletions(-) diff --git a/src/core/ir/resolver_context_like.rs b/src/core/ir/resolver_context_like.rs index 2195ffd763..6733a43875 100644 --- a/src/core/ir/resolver_context_like.rs +++ b/src/core/ir/resolver_context_like.rs @@ -96,7 +96,10 @@ impl SelectionField { field: &crate::core::jit::Field, ConstValue>, ) -> SelectionField { let name = field.name.clone(); - let selection_set = field.nested_iter().map(Self::from_jit_field).collect(); + let selection_set = field + .nested_iter(field.type_of.name()) + .map(Self::from_jit_field) + .collect(); let args = field .args .iter() diff --git a/src/core/jit/builder.rs b/src/core/jit/builder.rs index a16b28c764..838e99cc67 100644 --- a/src/core/jit/builder.rs +++ b/src/core/jit/builder.rs @@ -129,7 +129,7 @@ impl Builder { fn iter( &self, selection: &SelectionSet, - type_of: &str, + type_condition: &str, exts: Option, fragments: &HashMap<&str, &FragmentDefinition>, ) -> Vec> { @@ -170,7 +170,7 @@ impl Builder { .map(|(k, v)| (k.node.as_str().to_string(), v.node.to_owned())) .collect::>(); - if let Some(field_def) = self.index.get_field(type_of, field_name) { + if let Some(field_def) = self.index.get_field(type_condition, field_name) { let mut args = Vec::with_capacity(request_args.len()); if let QueryField::Field((_, schema_args)) = field_def { for (arg_name, arg_value) in schema_args { @@ -220,6 +220,7 @@ impl Builder { ir, is_scalar: self.index.type_is_scalar(type_of.name()), type_of, + type_condition: type_condition.to_string(), skip, include, args, @@ -246,7 +247,20 @@ impl Builder { )); } } - _ => {} + Selection::InlineFragment(Positioned { node: fragment, .. }) => { + let type_of = fragment + .type_condition + .as_ref() + .map(|cond| cond.node.on.node.as_str()) + .unwrap_or(type_condition); + + fields.extend(self.iter( + &fragment.selection_set.node, + type_of, + exts.clone(), + fragments, + )); + } } } @@ -429,9 +443,15 @@ mod tests { phone } + fragment PostPII on Post { + title + body + } + query { user(id:1) { ...UserPII + ...PostPII } } "#, diff --git a/src/core/jit/common/jp.rs b/src/core/jit/common/jp.rs index 52c09f5202..45fa7c70a7 100644 --- a/src/core/jit/common/jp.rs +++ b/src/core/jit/common/jp.rs @@ -7,6 +7,7 @@ use crate::core::blueprint::Blueprint; use crate::core::config::{Config, ConfigModule}; use crate::core::jit; use crate::core::jit::builder::Builder; +use crate::core::jit::exec::TypedValue; use crate::core::jit::store::{Data, Store}; use crate::core::jit::synth::Synth; use crate::core::jit::{OperationPlan, Variables}; @@ -27,9 +28,11 @@ struct TestData { users: Vec, } +type Entry = Data, Positioned>>; + struct ProcessedTestData { posts: Value, - users: HashMap>>>, + users: HashMap>, } impl<'a, Value: JsonLike<'a> + Deserialize<'a> + Clone + 'a> TestData { @@ -74,6 +77,7 @@ impl<'a, Value: JsonLike<'a> + Deserialize<'a> + Clone + 'a> TestData { Value::null() } }) + .map(TypedValue::new) .map(Ok) .map(Data::Single) .enumerate() @@ -128,7 +132,7 @@ impl< .to_owned(); let store = [ - (posts_id, Data::Single(Ok(posts))), + (posts_id, Data::Single(Ok(TypedValue::new(posts)))), (users_id, Data::Multiple(users)), ] .into_iter() diff --git a/src/core/jit/error.rs b/src/core/jit/error.rs index 6342529ce4..e2723efc12 100644 --- a/src/core/jit/error.rs +++ b/src/core/jit/error.rs @@ -30,6 +30,8 @@ pub enum ValidationError { // with async_graphql error message for this case #[error(r#"internal: invalid value for scalar "{type_of}", expected "FieldValue::Value""#)] ScalarInvalid { type_of: String, path: String }, + #[error("TypeName shape doesn't satisfy the processed object")] + TypeNameMismatch, } #[derive(Debug, Clone, Error)] @@ -58,11 +60,9 @@ impl ErrorExtensions for Error { impl Error { pub fn path(&self) -> Vec { match self { - Error::Validation(error) => match error { - ValidationError::ScalarInvalid { type_of: _, path } => { - vec![PathSegment::Field(path.clone())] - } - }, + Error::Validation(ValidationError::ScalarInvalid { type_of: _, path }) => { + vec![PathSegment::Field(path.clone())] + } _ => Vec::new(), } } diff --git a/src/core/jit/exec.rs b/src/core/jit/exec.rs index 69ef3689de..8972e0ac3f 100644 --- a/src/core/jit/exec.rs +++ b/src/core/jit/exec.rs @@ -9,11 +9,12 @@ use futures_util::future::join_all; use super::context::Context; use super::{DataPath, OperationPlan, Request, Response, Store}; use crate::core::ir::model::IR; +use crate::core::ir::TypeName; use crate::core::jit; use crate::core::jit::synth::Synth; use crate::core::json::{JsonLike, JsonObjectLike}; -type SharedStore = Arc>>>>; +type SharedStore = Arc, Positioned>>>>; /// /// Default GraphQL executor that takes in a GraphQL Request and produces a @@ -37,7 +38,7 @@ where pub async fn store( &self, request: Request, - ) -> Store>> { + ) -> Store, Positioned>> { let store = Arc::new(Mutex::new(Store::new())); let mut ctx = ExecutorInner::new(request, store.clone(), self.plan.to_owned(), &self.exec); ctx.init().await; @@ -104,16 +105,23 @@ where &'b self, ctx: &'b Context<'b, Input, Output>, data_path: &DataPath, - value: &'b Output, + result: TypedValueRef<'b, Output>, ) -> Result<(), Error> { let field = ctx.field(); + let TypedValueRef { value, type_name } = result; // Array // Check if the field expects a list if field.type_of.is_list() { // Check if the value is an array if let Some(array) = value.as_array() { - join_all(field.nested_iter().map(|field| { - join_all(array.iter().enumerate().map(|(index, value)| { + join_all(array.iter().enumerate().map(|(index, value)| { + let type_name = match &type_name { + Some(TypeName::Single(type_name)) => type_name, /* TODO: should throw */ + // ValidationError + Some(TypeName::Vec(v)) => &v[index], + None => field.type_of.name(), + }; + join_all(field.nested_iter(type_name).map(|field| { let ctx = ctx.with_value_and_field(value, field); let data_path = data_path.clone().with_index(index); async move { self.execute(&ctx, data_path).await } @@ -128,7 +136,13 @@ where // TODO: Validate if the value is an Object // Has to be an Object, we don't do anything while executing if its a Scalar else { - join_all(field.nested_iter().map(|child| { + let type_name = match &type_name { + Some(TypeName::Single(type_name)) => type_name, + Some(TypeName::Vec(_)) => panic!("TypeName type mismatch"), /* TODO: should throw ValidationError */ + None => field.type_of.name(), + }; + + join_all(field.nested_iter(type_name).map(|child| { let ctx = ctx.with_value_and_field(value, child); let data_path = data_path.clone(); async move { self.execute(&ctx, data_path).await } @@ -149,8 +163,8 @@ where if let Some(ir) = &field.ir { let result = self.ir_exec.execute(ir, ctx).await; - if let Ok(ref value) = result { - self.iter_field(ctx, &data_path, value).await?; + if let Ok(ref result) = result { + self.iter_field(ctx, &data_path, result.as_ref()).await?; } let mut store = self.store.lock().unwrap(); @@ -175,15 +189,50 @@ where // here without doing the "fix" .unwrap_or(&default_obj); - self.iter_field(ctx, &data_path, value).await?; + let result = TypedValueRef { value, type_name: None }; + + self.iter_field(ctx, &data_path, result).await?; } Ok(()) } } +#[derive(Clone)] +pub struct TypedValue { + pub value: V, + pub type_name: Option, +} + +pub struct TypedValueRef<'a, V> { + pub value: &'a V, + pub type_name: Option<&'a TypeName>, +} + +impl TypedValue { + pub fn new(value: V) -> Self { + Self { value, type_name: None } + } + + pub fn as_ref(&self) -> TypedValueRef<'_, V> { + TypedValueRef { value: &self.value, type_name: self.type_name.as_ref() } + } +} + +impl<'a, V> TypedValueRef<'a, V> { + pub fn new(value: &'a V) -> Self { + Self { value, type_name: None } + } + + pub fn map<'out, U>(&self, map: impl FnOnce(&V) -> &'out U) -> TypedValueRef<'out, U> + where + 'a: 'out, + { + TypedValueRef { value: map(self.value), type_name: self.type_name } + } +} + /// Executor for IR -#[async_trait::async_trait] pub trait IRExecutor { type Input; type Output; @@ -192,5 +241,5 @@ pub trait IRExecutor { &'a self, ir: &'a IR, ctx: &'a Context<'a, Self::Input, Self::Output>, - ) -> Result; + ) -> Result, Self::Error>; } diff --git a/src/core/jit/exec_const.rs b/src/core/jit/exec_const.rs index 1559426c43..d7fcf6aca4 100644 --- a/src/core/jit/exec_const.rs +++ b/src/core/jit/exec_const.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use async_graphql_value::ConstValue; use super::context::Context; -use super::exec::{Executor, IRExecutor}; +use super::exec::{Executor, IRExecutor, TypedValue}; use super::{Error, OperationPlan, Request, Response, Result}; use crate::core::app_context::AppContext; use crate::core::http::RequestContext; @@ -47,7 +47,6 @@ impl<'a> ConstValueExec<'a> { } } -#[async_trait::async_trait] impl<'ctx> IRExecutor for ConstValueExec<'ctx> { type Input = ConstValue; type Output = ConstValue; @@ -57,9 +56,13 @@ impl<'ctx> IRExecutor for ConstValueExec<'ctx> { &'a self, ir: &'a IR, ctx: &'a Context<'a, Self::Input, Self::Output>, - ) -> Result { + ) -> Result> { let req_context = &self.req_context; - let mut ctx = EvalContext::new(req_context, ctx); - Ok(ir.eval(&mut ctx).await?) + let mut eval_ctx = EvalContext::new(req_context, ctx); + + Ok(ir + .eval(&mut eval_ctx) + .await + .map(|value| TypedValue { value, type_name: eval_ctx.type_name.take() })?) } } diff --git a/src/core/jit/model.rs b/src/core/jit/model.rs index 74c0ad8a24..49229b4d49 100644 --- a/src/core/jit/model.rs +++ b/src/core/jit/model.rs @@ -108,6 +108,11 @@ pub struct Field { pub name: String, pub ir: Option, pub type_of: crate::core::blueprint::Type, + /// Specifies the name of type used in condition to fetch that field + /// The type could be anything from graphql type system: + /// interface, type, union, input type. + /// See [spec](https://spec.graphql.org/October2021/#sec-Type-Conditions) + pub type_condition: String, pub skip: Option, pub include: Option, pub args: Vec>, @@ -140,11 +145,12 @@ impl Field, Input> { let mut extensions = None; if let Some(nested) = self.extensions { - let mut exts = vec![]; - for v in nested.0 { - exts.push(v.try_map(map)?); - } - extensions = Some(Nested(exts)); + let nested = nested + .0 + .into_iter() + .map(|v| v.try_map(map)) + .collect::>()?; + extensions = Some(Nested(nested)); } Ok(Field { @@ -152,6 +158,7 @@ impl Field, Input> { name: self.name, ir: self.ir, type_of: self.type_of, + type_condition: self.type_condition, extensions, pos: self.pos, skip: self.skip, @@ -181,6 +188,7 @@ impl Field { name: self.name, ir: self.ir, type_of: self.type_of, + type_condition: self.type_condition, extensions: self.extensions, skip: self.skip, include: self.include, @@ -201,13 +209,27 @@ impl Field { } impl Field, Input> { - pub fn nested(&self) -> Option<&Vec, Input>>> { - self.extensions.as_ref().map(|Nested(nested)| nested) - } - - pub fn nested_iter(&self) -> impl Iterator, Input>> { - self.nested() - .map(|nested| nested.iter()) + /// iters over children fields that are + /// related to passed `type_name` either + /// as direct field of the queried type or + /// field from fragment on type `type_name` + pub fn nested_iter<'a>( + &'a self, + type_name: &'a str, + ) -> impl Iterator, Input>> + 'a { + self.extensions + .as_ref() + .map(move |nested| { + nested + .0 + .iter() + // TODO: handle Interface and Union types here + // Right now only exact type name is used to check the set of fields + // but with Interfaces/Unions we need to check if that specific type + // is member of some Interface/Union and if so call the fragments for + // the related Interfaces/Unions + .filter(move |field| field.type_condition == type_name) + }) .into_iter() .flatten() } @@ -215,7 +237,7 @@ impl Field, Input> { impl Field { fn parent(&self) -> Option<&FieldId> { - self.extensions.as_ref().map(|Flat(id)| id) + self.extensions.as_ref().map(|flat| &flat.0) } fn into_nested(self, fields: &[Field]) -> Field, Input> @@ -242,6 +264,7 @@ impl Field { name: self.name, ir: self.ir, type_of: self.type_of, + type_condition: self.type_condition, skip: self.skip, include: self.include, args: self.args, @@ -262,6 +285,7 @@ impl Debug for Field { debug_struct.field("ir", &"Some(..)"); } debug_struct.field("type_of", &self.type_of); + debug_struct.field("type_condition", &self.type_condition); if !self.args.is_empty() { debug_struct.field("args", &self.args); } @@ -282,20 +306,12 @@ impl Debug for Field { /// Stores field relationships in a flat structure where each field links to its /// parent. -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct Flat(FieldId); impl Flat { - pub fn new(id: FieldId) -> Self { - Flat(id) - } - pub fn as_id(&self) -> &FieldId { - &self.0 - } -} -impl Debug for Flat { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "Flat({:?})", self.0) + pub fn new(parent_id: FieldId) -> Self { + Flat(parent_id) } } diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__default_value.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__default_value.snap index c47a3b7ad3..5b7c0ce3f4 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__default_value.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__default_value.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "createPost", ir: "Some(..)", type_of: Post, + type_condition: "Mutation", args: [ Arg { id: 0, @@ -44,6 +45,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__directives.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__directives.snap index dc22b1b27d..d947684965 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__directives.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__directives.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "users", ir: "Some(..)", type_of: [User], + type_condition: "Query", extensions: Some( Nested( [ @@ -15,6 +16,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [ Directive { @@ -34,6 +36,7 @@ expression: plan.into_nested() id: 2, name: "name", type_of: String!, + type_condition: "User", is_scalar: true, include: Some( Variable( diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__fragments.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__fragments.snap index 01ada69850..21fa563145 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__fragments.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__fragments.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "user", ir: "Some(..)", type_of: User, + type_condition: "Query", args: [ Arg { id: 0, @@ -28,6 +29,7 @@ expression: plan.into_nested() id: 1, name: "name", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -35,6 +37,7 @@ expression: plan.into_nested() id: 2, name: "email", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -42,6 +45,23 @@ expression: plan.into_nested() id: 3, name: "phone", type_of: String, + type_condition: "User", + is_scalar: true, + directives: [], + }, + Field { + id: 4, + name: "title", + type_of: String!, + type_condition: "Post", + is_scalar: true, + directives: [], + }, + Field { + id: 5, + name: "body", + type_of: String!, + type_condition: "Post", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__from_document.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__from_document.snap index 7ff69aa95f..94cffc19cb 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__from_document.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__from_document.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "posts", ir: "Some(..)", type_of: [Post], + type_condition: "Query", extensions: Some( Nested( [ @@ -16,6 +17,7 @@ expression: plan.into_nested() name: "user", ir: "Some(..)", type_of: User, + type_condition: "Post", extensions: Some( Nested( [ @@ -23,6 +25,7 @@ expression: plan.into_nested() id: 2, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -30,6 +33,7 @@ expression: plan.into_nested() id: 3, name: "name", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__multiple_operations.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__multiple_operations.snap index a5e6f34234..c0e405e8f7 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__multiple_operations.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__multiple_operations.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "user", ir: "Some(..)", type_of: User, + type_condition: "Query", args: [ Arg { id: 0, @@ -28,6 +29,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -35,6 +37,7 @@ expression: plan.into_nested() id: 2, name: "username", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -49,6 +52,7 @@ expression: plan.into_nested() name: "posts", ir: "Some(..)", type_of: [Post], + type_condition: "Query", extensions: Some( Nested( [ @@ -56,6 +60,7 @@ expression: plan.into_nested() id: 4, name: "id", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -63,6 +68,7 @@ expression: plan.into_nested() id: 5, name: "title", type_of: String!, + type_condition: "Post", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation-2.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation-2.snap index 51f4bd58d2..96d50e9124 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation-2.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation-2.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "createPost", ir: "Some(..)", type_of: Post, + type_condition: "Mutation", args: [ Arg { id: 0, @@ -44,6 +45,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -51,6 +53,7 @@ expression: plan.into_nested() id: 2, name: "userId", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -58,6 +61,7 @@ expression: plan.into_nested() id: 3, name: "title", type_of: String!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -65,6 +69,7 @@ expression: plan.into_nested() id: 4, name: "body", type_of: String!, + type_condition: "Post", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation.snap index e1689a3fdf..b44a5a2885 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__resolving_operation.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "posts", ir: "Some(..)", type_of: [Post], + type_condition: "Query", extensions: Some( Nested( [ @@ -15,6 +16,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -22,6 +24,7 @@ expression: plan.into_nested() id: 2, name: "userId", type_of: ID!, + type_condition: "Post", is_scalar: true, directives: [], }, @@ -29,6 +32,7 @@ expression: plan.into_nested() id: 3, name: "title", type_of: String!, + type_condition: "Post", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_mutation.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_mutation.snap index 389772e702..4f687ca327 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_mutation.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_mutation.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "createUser", ir: "Some(..)", type_of: User, + type_condition: "Mutation", args: [ Arg { id: 0, @@ -59,6 +60,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -66,6 +68,7 @@ expression: plan.into_nested() id: 2, name: "name", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -73,6 +76,7 @@ expression: plan.into_nested() id: 3, name: "email", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -80,6 +84,7 @@ expression: plan.into_nested() id: 4, name: "phone", type_of: String, + type_condition: "User", is_scalar: true, directives: [], }, @@ -87,6 +92,7 @@ expression: plan.into_nested() id: 5, name: "website", type_of: String, + type_condition: "User", is_scalar: true, directives: [], }, @@ -94,6 +100,7 @@ expression: plan.into_nested() id: 6, name: "username", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_query.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_query.snap index d003ae86a2..3bb30ca833 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_query.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__simple_query.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "posts", ir: "Some(..)", type_of: [Post], + type_condition: "Query", extensions: Some( Nested( [ @@ -16,6 +17,7 @@ expression: plan.into_nested() name: "user", ir: "Some(..)", type_of: User, + type_condition: "Post", extensions: Some( Nested( [ @@ -23,6 +25,7 @@ expression: plan.into_nested() id: 2, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__unions.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__unions.snap index 1ff02ae66e..4846b6c441 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__unions.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__unions.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "getUserIdOrEmail", ir: "Some(..)", type_of: UserIdOrEmail, + type_condition: "Query", args: [ Arg { id: 0, @@ -21,6 +22,28 @@ expression: plan.into_nested() default_value: None, }, ], + extensions: Some( + Nested( + [ + Field { + id: 1, + name: "id", + type_of: ID!, + type_condition: "UserId", + is_scalar: true, + directives: [], + }, + Field { + id: 2, + name: "email", + type_of: String!, + type_condition: "UserEmail", + is_scalar: true, + directives: [], + }, + ], + ), + ), is_scalar: false, directives: [], }, diff --git a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__variables.snap b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__variables.snap index 03d172fded..243eae2044 100644 --- a/src/core/jit/snapshots/tailcall__core__jit__builder__tests__variables.snap +++ b/src/core/jit/snapshots/tailcall__core__jit__builder__tests__variables.snap @@ -8,6 +8,7 @@ expression: plan.into_nested() name: "user", ir: "Some(..)", type_of: User, + type_condition: "Query", args: [ Arg { id: 0, @@ -28,6 +29,7 @@ expression: plan.into_nested() id: 1, name: "id", type_of: ID!, + type_condition: "User", is_scalar: true, directives: [], }, @@ -35,6 +37,7 @@ expression: plan.into_nested() id: 2, name: "name", type_of: String!, + type_condition: "User", is_scalar: true, directives: [], }, diff --git a/src/core/jit/synth/synth.rs b/src/core/jit/synth/synth.rs index f59a766968..bec53b5907 100644 --- a/src/core/jit/synth/synth.rs +++ b/src/core/jit/synth/synth.rs @@ -1,14 +1,18 @@ use async_graphql::Positioned; +use crate::core::ir::TypeName; +use crate::core::jit::exec::{TypedValue, TypedValueRef}; use crate::core::jit::model::{Field, Nested, OperationPlan, Variable, Variables}; use crate::core::jit::store::{Data, DataPath, Store}; use crate::core::jit::{Error, ValidationError}; use crate::core::json::{JsonLike, JsonObjectLike}; use crate::core::scalar; +type ValueStore = Store, Positioned>>; + pub struct Synth { selection: Vec, Value>>, - store: Store>>, + store: ValueStore, variables: Variables, } @@ -34,7 +38,7 @@ impl Synth { #[inline(always)] pub fn new( plan: OperationPlan, - store: Store>>, + store: ValueStore, variables: Variables, ) -> Self { Self { selection: plan.into_nested(), store, variables } @@ -76,72 +80,69 @@ where fn iter( &'a self, node: &'a Field, Value>, - parent: Option<&'a Value>, + result: Option>, data_path: &DataPath, ) -> Result> { - // TODO: this implementation prefer parent value over value in the store - // that's opposite to the way async_graphql engine works in tailcall - match parent { - Some(parent) => { - if !Self::is_array(&node.type_of, parent) { - return Ok(Value::null()); - } - self.iter_inner(node, parent, data_path) - } - None => { - // we perform this check to avoid unnecessary hashing - - match self.store.get(&node.id) { - Some(val) => { - let mut data = val; - - for index in data_path.as_slice() { - match data { - Data::Multiple(v) => { - data = &v[index]; - } - _ => return Ok(Value::null()), - } + match self.store.get(&node.id) { + Some(val) => { + let mut data = val; + + for index in data_path.as_slice() { + match data { + Data::Multiple(v) => { + data = &v[index]; } + _ => return Ok(Value::null()), + } + } - match data { - Data::Single(val) => self.iter( - node, - Some(val.as_ref().map_err(Clone::clone)?), - data_path, - ), - _ => { - // TODO: should bailout instead of returning Null - Ok(Value::null()) - } + match data { + Data::Single(result) => { + let result = match result { + Ok(result) => result, + Err(err) => return Err(err.clone()), + }; + + if !Self::is_array(&node.type_of, &result.value) { + return Ok(Value::null()); } + self.iter_inner(node, result.as_ref(), data_path) } - None => { - // IR exists, so there must be a value. - // if there is no value then we must return Null + _ => { + // TODO: should bailout instead of returning Null Ok(Value::null()) } } } + None => match result { + Some(result) => self.iter_inner(node, result, data_path), + None => Ok(Value::null()), + }, } } + #[inline(always)] fn iter_inner( &'a self, node: &'a Field, Value>, - parent: &'a Value, + result: TypedValueRef<'a, Value>, data_path: &DataPath, ) -> Result> { - let include = self.include(node); - if include && node.is_scalar { + if !self.include(node) { + return Ok(Value::null()); + } + + let TypedValueRef { type_name, value } = result; + + if node.is_scalar { let scalar = scalar::Scalar::find(node.type_of.name()).unwrap_or(&scalar::Scalar::Empty); // TODO: add validation for input type as well. But input types are not checked // by async_graphql anyway so it should be done after replacing // default engine with JIT - if scalar.validate(parent) { - Ok(parent.clone()) + if scalar.validate(value) { + Ok(value.clone()) } else { Err(Positioned { pos: node.pos, @@ -153,62 +154,54 @@ where }) } } else { - match (parent.as_array(), parent.as_object()) { + match (value.as_array(), value.as_object()) { (_, Some(obj)) => { let mut ans = Value::JsonObject::new(); - if include { - if let Some(children) = node.nested() { - for child in children { - // all checks for skip must occur in `iter_inner` - // and include be checked before calling `iter` or recursing. - let include = self.include(child); - if include { - let val = obj.get_key(child.name.as_str()); - if let Some(val) = val { - ans.insert_key( - child.name.as_str(), - self.iter_inner(child, val, data_path)?, - ); - } else { - ans.insert_key( - child.name.as_str(), - self.iter(child, None, data_path)?, - ); - } - } - } - } else { - let val = obj.get_key(node.name.as_str()); - // if it's a leaf node, then push the value - if let Some(val) = val { - ans.insert_key(node.name.as_str(), val.to_owned()); + + let type_name = match &type_name { + Some(TypeName::Single(type_name)) => type_name, + Some(TypeName::Vec(v)) => { + if let Some(index) = data_path.as_slice().last() { + &v[*index] } else { - return Ok(Value::null()); + return Err(Positioned::new( + ValidationError::TypeNameMismatch.into(), + node.pos, + )); } } - } else { - let val = obj.get_key(node.name.as_str()); - // if it's a leaf node, then push the value - if let Some(val) = val { - ans.insert_key(node.name.as_str(), val.to_owned()); - } else { - return Ok(Value::null()); + None => node.type_of.name(), + }; + + for child in node.nested_iter(type_name) { + // all checks for skip must occur in `iter_inner` + // and include be checked before calling `iter` or recursing. + let include = self.include(child); + if include { + let val = obj.get_key(child.name.as_str()); + + ans.insert_key( + child.name.as_str(), + self.iter(child, val.map(TypedValueRef::new), data_path)?, + ); } } + Ok(Value::object(ans)) } (Some(arr), _) => { let mut ans = vec![]; - if include { - for (i, val) in arr.iter().enumerate() { - let val = - self.iter_inner(node, val, &data_path.clone().with_index(i))?; - ans.push(val) - } + for (i, val) in arr.iter().enumerate() { + let val = self.iter_inner( + node, + result.map(|_| val), + &data_path.clone().with_index(i), + )?; + ans.push(val) } Ok(Value::array(ans)) } - _ => Ok(parent.clone()), + _ => Ok(value.clone()), } } } @@ -223,6 +216,7 @@ mod tests { use crate::core::config::{Config, ConfigModule}; use crate::core::jit::builder::Builder; use crate::core::jit::common::JP; + use crate::core::jit::exec::TypedValue; use crate::core::jit::model::{FieldId, Variables}; use crate::core::jit::store::{Data, Store}; use crate::core::jit::synth::Synth; @@ -278,20 +272,22 @@ mod tests { } impl TestData { - fn into_value<'a, Value: Deserialize<'a>>(self) -> Data { + fn into_value<'a, Value: Deserialize<'a>>(self) -> Data> { match self { - Self::Posts => Data::Single(serde_json::from_str(POSTS).unwrap()), - Self::User1 => Data::Single(serde_json::from_str(USER1).unwrap()), + Self::Posts => Data::Single(TypedValue::new(serde_json::from_str(POSTS).unwrap())), + Self::User1 => Data::Single(TypedValue::new(serde_json::from_str(USER1).unwrap())), TestData::UsersData => Data::Multiple( vec![ - Data::Single(serde_json::from_str(USER1).unwrap()), - Data::Single(serde_json::from_str(USER2).unwrap()), + Data::Single(TypedValue::new(serde_json::from_str(USER1).unwrap())), + Data::Single(TypedValue::new(serde_json::from_str(USER2).unwrap())), ] .into_iter() .enumerate() .collect(), ), - TestData::Users => Data::Single(serde_json::from_str(USERS).unwrap()), + TestData::Users => { + Data::Single(TypedValue::new(serde_json::from_str(USERS).unwrap())) + } } } } diff --git a/tests/execution/grpc-oneof.md b/tests/execution/grpc-oneof.md index 5bfab46df5..a407cc5a76 100644 --- a/tests/execution/grpc-oneof.md +++ b/tests/execution/grpc-oneof.md @@ -169,6 +169,8 @@ type oneof__Response__Var2 @tag(id: "oneof.Response") { query: > query { oneof__OneOfService__GetOneOfVar1(request: { command: { command: "start" } }) { + # TODO: check that it's possible to get shared field from Union like that + # outside of the fragment usual ... on oneof__Response__Var1 { command {