From 2ae75dcf3cdaabd87106a0a51308bb730e8ffaff Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Wed, 7 Aug 2024 21:46:34 +0000 Subject: [PATCH 1/3] refactor: store __typename in value instead of separate entity --- src/core/blueprint/into_schema.rs | 41 +++-- src/core/ir/discriminator.rs | 179 +++++++++++----------- src/core/ir/eval.rs | 10 +- src/core/ir/eval_context.rs | 12 -- src/core/jit/builder.rs | 22 ++- src/core/jit/common/jp.rs | 6 +- src/core/jit/error.rs | 2 - src/core/jit/exec.rs | 69 ++------- src/core/jit/exec_const.rs | 9 +- src/core/jit/synth/synth.rs | 67 +++----- src/core/json/borrow.rs | 21 +++ src/core/json/graphql.rs | 23 ++- src/core/json/json_like.rs | 26 ++++ src/core/json/serde.rs | 20 +++ tests/core/snapshots/test-union.md_5.snap | 45 ++++++ tests/execution/test-union.md | 28 ++++ 16 files changed, 336 insertions(+), 244 deletions(-) create mode 100644 tests/core/snapshots/test-union.md_5.snap diff --git a/src/core/blueprint/into_schema.rs b/src/core/blueprint/into_schema.rs index cbfdb681f3..02aa0041bb 100644 --- a/src/core/blueprint/into_schema.rs +++ b/src/core/blueprint/into_schema.rs @@ -1,7 +1,6 @@ use std::borrow::Cow; use std::sync::Arc; -use anyhow::{bail, Result}; use async_graphql::dynamic::{self, FieldFuture, FieldValue, SchemaBuilder}; use async_graphql::ErrorExtensions; use async_graphql_value::ConstValue; @@ -11,7 +10,7 @@ use tracing::Instrument; use crate::core::blueprint::{Blueprint, Definition, Type}; use crate::core::http::RequestContext; -use crate::core::ir::{EvalContext, ResolverContext, TypeName}; +use crate::core::ir::{EvalContext, ResolverContext, TypedValue}; use crate::core::scalar; fn to_type_ref(type_of: &Type) -> dynamic::TypeRef { @@ -59,27 +58,23 @@ fn set_default_value( } } -fn to_field_value<'a>( - ctx: &mut EvalContext<'a, ResolverContext<'a>>, - value: async_graphql::Value, -) -> Result> { - let type_name = ctx.type_name.take(); - - Ok(match (value, type_name) { - // NOTE: Mostly type_name is going to be None so we should keep that as the first check. - (value, None) => FieldValue::from(value), - (ConstValue::List(values), Some(TypeName::Vec(names))) => FieldValue::list( - values - .into_iter() - .zip(names) - .map(|(value, type_name)| FieldValue::from(value).with_type(type_name)), - ), - (value @ ConstValue::Object(_), Some(TypeName::Single(type_name))) => { - FieldValue::from(value).with_type(type_name) +fn to_field_value(value: async_graphql::Value) -> FieldValue<'static> { + match value { + ConstValue::List(vec) => { + FieldValue::list(vec.into_iter().map(to_field_value)) } - (ConstValue::Null, _) => FieldValue::NULL, - (_, Some(_)) => bail!("Failed to match type_name"), - }) + value => { + let type_name = value.get_type_name().map(|s| s.to_string()); + + let field_value = FieldValue::from(value); + + if let Some(type_name) = type_name { + field_value.with_type(type_name) + } else { + field_value + } + } + } } fn to_type(def: &Definition) -> dynamic::Type { @@ -131,7 +126,7 @@ fn to_type(def: &Definition) -> dynamic::Type { if let ConstValue::Null = value { Ok(FieldValue::NONE) } else { - Ok(Some(to_field_value(ctx, value)?)) + Ok(Some(to_field_value(value))) } } .instrument(span) diff --git a/src/core/ir/discriminator.rs b/src/core/ir/discriminator.rs index b8907c70c8..bfd7511392 100644 --- a/src/core/ir/discriminator.rs +++ b/src/core/ir/discriminator.rs @@ -8,17 +8,40 @@ use indenter::indented; use indexmap::IndexMap; use crate::core::config::Type; +use crate::core::json::{JsonLike, JsonObjectLike}; use crate::core::valid::{Cause, Valid, Validator}; -/// Represents the type name for the resolved value. -/// It is used when the GraphQL executor needs to resolve values of a union -/// type. In order to select the correct fields, the executor must know the -/// exact type name for each resolved value. When the output is a list of a -/// union type, it should resolve the exact type for every entry in the list. -#[derive(PartialEq, Eq, Debug, Clone)] -pub enum TypeName { - Single(String), - Vec(Vec), +pub trait TypedValue<'a> { + type Error; + + fn get_type_name(&'a self) -> Option<&'a str>; + fn set_type_name(&'a mut self, type_name: String) -> Result<(), Self::Error>; +} + +const TYPENAME_FIELD: &str = "__typename"; + +impl<'json, T> TypedValue<'json> for T +where + T: JsonLike<'json>, + T::JsonObject<'json>: JsonObjectLike<'json, Value = T>, +{ + type Error = anyhow::Error; + + fn get_type_name(&'json self) -> Option<&'json str> { + self.as_object() + .and_then(|obj| obj.get_key(TYPENAME_FIELD)) + .and_then(|val| val.as_str()) + } + + fn set_type_name(&'json mut self, type_name: String) -> Result<(), Self::Error> { + if let Some(obj) = self.as_object_mut() { + obj.insert_key(TYPENAME_FIELD, T::string(type_name.into())); + + Ok(()) + } else { + bail!("Expected object") + } + } } /// Resolver for type member of a union. @@ -214,22 +237,7 @@ impl Discriminator { Valid::succeed(discriminator) } - pub fn resolve_type(&self, value: &Value) -> Result { - if let Value::List(list) = value { - let results: Result> = list - .iter() - .map(|item| Ok(self.resolve_type_for_single(item)?.to_string())) - .collect(); - - Ok(TypeName::Vec(results?)) - } else { - Ok(TypeName::Single( - self.resolve_type_for_single(value)?.to_string(), - )) - } - } - - fn resolve_type_for_single(&self, value: &Value) -> Result<&str> { + pub fn resolve_type(&self, value: &Value) -> Result<&str> { let Value::Object(obj) = value else { bail!("Value expected to be object"); }; @@ -346,7 +354,6 @@ mod tests { use super::Discriminator; use crate::core::config::{Field, Type}; - use crate::core::ir::discriminator::TypeName; use crate::core::valid::Validator; #[test] @@ -361,14 +368,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); // ambiguous cases @@ -376,21 +383,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test", "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); } @@ -408,14 +415,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); // ambiguous cases @@ -423,21 +430,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test", "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); } @@ -466,21 +473,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "a": 1, "ab": 1, "abab": 1 })).unwrap()) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "b": 1, "ab": 1, "abab": 1 })).unwrap()) .unwrap(), - TypeName::Single("B".to_string()) + "B" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "c": 1, "ac": 1 })).unwrap()) .unwrap(), - TypeName::Single("C".to_string()) + "C" ); // ambiguous cases @@ -488,21 +495,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "a": 1, "b": 1, "c": 1 })).unwrap()) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("C".to_string()) + "C" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("C".to_string()) + "C" ); } @@ -528,14 +535,14 @@ mod tests { &Value::from_json(json!({ "a": 123, "b": true, "foo": "test" })).unwrap() ) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); // ambiguous cases @@ -543,21 +550,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test", "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); // ambiguous cases @@ -565,21 +572,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test", "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); } @@ -599,14 +606,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "b": 123, "foo": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Bar".to_string()) + "Bar" ); assert_eq!( @@ -615,7 +622,7 @@ mod tests { &Value::from_json(json!({ "unknown": { "foo": "bar" }, "a": 1 })).unwrap() ) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); // ambiguous cases @@ -623,21 +630,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "foo": "test", "bar": "test" })).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Foo".to_string()) + "Foo" ); } @@ -667,21 +674,21 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "a": 1 })).unwrap()) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "b": 1, "aa": 1 })).unwrap()) .unwrap(), - TypeName::Single("B".to_string()) + "B" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "c": 1, "aaa": 1 })).unwrap()) .unwrap(), - TypeName::Single("C".to_string()) + "C" ); // ambiguous cases @@ -691,21 +698,21 @@ mod tests { &Value::from_json(json!({ "shared": 1, "a": 1, "b": 1, "c": 1 })).unwrap() ) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("A".to_string()) + "A" ); } @@ -766,14 +773,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({ "usual": 1 })).unwrap()) .unwrap(), - TypeName::Single("Var_Var".to_string()) + "Var_Var" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "usual": 1, "payload": 1 })).unwrap()) .unwrap(), - TypeName::Single("Var0_Var".to_string()) + "Var0_Var" ); assert_eq!( @@ -782,14 +789,14 @@ mod tests { &Value::from_json(json!({ "usual": 1, "command": 2, "useless": 1 })).unwrap() ) .unwrap(), - TypeName::Single("Var1_Var".to_string()) + "Var1_Var" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "usual": 1, "flag": true })).unwrap()) .unwrap(), - TypeName::Single("Var_Var0".to_string()) + "Var_Var0" ); assert_eq!( @@ -799,7 +806,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("Var_Var1".to_string()) + "Var_Var1" ); assert_eq!( @@ -808,7 +815,7 @@ mod tests { &Value::from_json(json!({ "usual": 1, "payload": 1, "flag": true })).unwrap() ) .unwrap(), - TypeName::Single("Var0_Var0".to_string()) + "Var0_Var0" ); assert_eq!( @@ -818,7 +825,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("Var0_Var1".to_string()) + "Var0_Var1" ); assert_eq!( @@ -827,7 +834,7 @@ mod tests { &Value::from_json(json!({ "usual": 1, "command": 1, "flag": true })).unwrap() ) .unwrap(), - TypeName::Single("Var1_Var0".to_string()) + "Var1_Var0" ); assert_eq!( @@ -837,7 +844,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("Var1_Var1".to_string()) + "Var1_Var1" ); // ambiguous cases @@ -855,14 +862,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("Var_Var".to_string()) + "Var_Var" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("Var_Var".to_string()) + "Var_Var" ); } @@ -901,14 +908,14 @@ mod tests { &Value::from_json(json!({ "uniqueA1": "value", "common": 1 })).unwrap() ) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "uniqueB1": true, "common": 2 })).unwrap()) .unwrap(), - TypeName::Single("TypeB".to_string()) + "TypeB" ); assert_eq!( @@ -918,7 +925,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("TypeC".to_string()) + "TypeC" ); assert_eq!( @@ -930,7 +937,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("TypeD".to_string()) + "TypeD" ); // ambiguous cases @@ -943,21 +950,21 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); } @@ -996,7 +1003,7 @@ mod tests { &Value::from_json(json!({ "field1": "value", "field2": "value" })).unwrap() ) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); assert_eq!( @@ -1005,7 +1012,7 @@ mod tests { &Value::from_json(json!({ "field2": "value", "field3": "value" })).unwrap() ) .unwrap(), - TypeName::Single("TypeB".to_string()) + "TypeB" ); assert_eq!( @@ -1014,7 +1021,7 @@ mod tests { &Value::from_json(json!({ "field1": "value", "field3": "value" })).unwrap() ) .unwrap(), - TypeName::Single("TypeC".to_string()) + "TypeC" ); assert_eq!( @@ -1026,7 +1033,7 @@ mod tests { .unwrap() ) .unwrap(), - TypeName::Single("TypeD".to_string()) + "TypeD" ); // ambiguous cases @@ -1047,14 +1054,14 @@ mod tests { discriminator .resolve_type(&Value::from_json(json!({})).unwrap()) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); assert_eq!( discriminator .resolve_type(&Value::from_json(json!({ "unknown": { "foo": "bar" }})).unwrap()) .unwrap(), - TypeName::Single("TypeA".to_string()) + "TypeA" ); } diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index 595cb57dbd..b99d965b10 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -5,7 +5,7 @@ use async_graphql_value::ConstValue; use super::eval_io::eval_io; use super::model::{Cache, CacheKey, Map, IR}; -use super::{Error, EvalContext, ResolverContextLike}; +use super::{Error, EvalContext, ResolverContextLike, TypedValue}; use crate::core::json::JsonLike; use crate::core::serde_value_ext::ValueExt; @@ -89,9 +89,13 @@ impl IR { second.eval(ctx).await } IR::Discriminate(discriminator, expr) => expr.eval(ctx).await.and_then(|value| { - let type_name = discriminator.resolve_type(&value)?; + let value = value.map(|mut value| { + let type_name = discriminator.resolve_type(&value)?; - ctx.set_type_name(type_name); + value.set_type_name(type_name.to_string())?; + + anyhow::Ok(value) + })?; Ok(value) }), diff --git a/src/core/ir/eval_context.rs b/src/core/ir/eval_context.rs index a1afbcf26e..597335c00e 100644 --- a/src/core/ir/eval_context.rs +++ b/src/core/ir/eval_context.rs @@ -5,7 +5,6 @@ use std::sync::Arc; use async_graphql::{ServerError, Value}; use reqwest::header::HeaderMap; -use super::discriminator::TypeName; use super::{GraphQLOperationContext, RelatedFields, ResolverContextLike, SelectionField}; use crate::core::document::print_directives; use crate::core::http::RequestContext; @@ -25,12 +24,6 @@ pub struct EvalContext<'a, Ctx: ResolverContextLike> { // Overridden Arguments for Async GraphQL Context graphql_ctx_args: Option>, - - /// Type name of resolved data that is calculated - /// dynamically based on the shape of the value itself. - /// Required for proper Union type resolutions. - /// More details at [TypeName] - pub type_name: Option, } impl<'a, Ctx: ResolverContextLike> EvalContext<'a, Ctx> { @@ -56,7 +49,6 @@ impl<'a, Ctx: ResolverContextLike> EvalContext<'a, Ctx> { graphql_ctx, graphql_ctx_value: None, graphql_ctx_args: None, - type_name: None, } } @@ -114,10 +106,6 @@ impl<'a, Ctx: ResolverContextLike> EvalContext<'a, Ctx> { pub fn add_error(&self, error: ServerError) { self.graphql_ctx.add_error(error) } - - pub fn set_type_name(&mut self, type_name: TypeName) { - self.type_name = Some(type_name); - } } impl<'a, Ctx: ResolverContextLike> GraphQLOperationContext for EvalContext<'a, Ctx> { diff --git a/src/core/jit/builder.rs b/src/core/jit/builder.rs index 838e99cc67..505f16c53d 100644 --- a/src/core/jit/builder.rs +++ b/src/core/jit/builder.rs @@ -231,8 +231,26 @@ impl Builder { fields.push(flat_field); fields = fields.merge_right(child_fields); - } else { - // TODO: error if the field is not found in the schema + } else if field_name == "__typename" { + let flat_field = Field { + id: FieldId::new(self.field_id.next()), + name: field_name.to_string(), + ir: None, + is_scalar: true, + type_of: crate::core::blueprint::Type::NamedType { + name: "String".to_owned(), + non_null: true, + }, + type_condition: type_condition.to_string(), + skip, + include, + args: Vec::new(), + pos: selection.pos, + extensions: exts.clone(), + directives, + }; + + fields.push(flat_field); } } Selection::FragmentSpread(Positioned { node: fragment_spread, .. }) => { diff --git a/src/core/jit/common/jp.rs b/src/core/jit/common/jp.rs index 45fa7c70a7..33eca2cf3f 100644 --- a/src/core/jit/common/jp.rs +++ b/src/core/jit/common/jp.rs @@ -7,7 +7,6 @@ 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}; @@ -28,7 +27,7 @@ struct TestData { users: Vec, } -type Entry = Data, Positioned>>; +type Entry = Data>>; struct ProcessedTestData { posts: Value, @@ -77,7 +76,6 @@ impl<'a, Value: JsonLike<'a> + Deserialize<'a> + Clone + 'a> TestData { Value::null() } }) - .map(TypedValue::new) .map(Ok) .map(Data::Single) .enumerate() @@ -132,7 +130,7 @@ impl< .to_owned(); let store = [ - (posts_id, Data::Single(Ok(TypedValue::new(posts)))), + (posts_id, Data::Single(Ok(posts))), (users_id, Data::Multiple(users)), ] .into_iter() diff --git a/src/core/jit/error.rs b/src/core/jit/error.rs index e2723efc12..0969070c96 100644 --- a/src/core/jit/error.rs +++ b/src/core/jit/error.rs @@ -30,8 +30,6 @@ 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)] diff --git a/src/core/jit/exec.rs b/src/core/jit/exec.rs index 8972e0ac3f..26c163f88f 100644 --- a/src/core/jit/exec.rs +++ b/src/core/jit/exec.rs @@ -9,12 +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::ir::TypedValue; use crate::core::jit; use crate::core::jit::synth::Synth; use crate::core::json::{JsonLike, JsonObjectLike}; -type SharedStore = Arc, Positioned>>>>; +type SharedStore = Arc>>>>; /// /// Default GraphQL executor that takes in a GraphQL Request and produces a @@ -38,7 +38,7 @@ where pub async fn store( &self, request: Request, - ) -> Store, Positioned>> { + ) -> Store>> { 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; @@ -62,7 +62,7 @@ struct ExecutorInner<'a, Input, Output, Error, Exec> { impl<'a, Input, Output, Error, Exec> ExecutorInner<'a, Input, Output, Error, Exec> where - Output: for<'i> JsonLike<'i> + Debug, + for<'i> Output: JsonLike<'i> + TypedValue<'i> + Debug, Input: Clone + Debug, Exec: IRExecutor, { @@ -105,22 +105,17 @@ where &'b self, ctx: &'b Context<'b, Input, Output>, data_path: &DataPath, - result: TypedValueRef<'b, Output>, + value: &'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(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(), - }; + let type_name = value.get_type_name().unwrap_or(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); @@ -136,11 +131,7 @@ 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 { - 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(), - }; + let type_name = value.get_type_name().unwrap_or(field.type_of.name()); join_all(field.nested_iter(type_name).map(|child| { let ctx = ctx.with_value_and_field(value, child); @@ -163,8 +154,8 @@ where if let Some(ir) = &field.ir { let result = self.ir_exec.execute(ir, ctx).await; - if let Ok(ref result) = result { - self.iter_field(ctx, &data_path, result.as_ref()).await?; + if let Ok(value) = &result { + self.iter_field(ctx, &data_path, value).await?; } let mut store = self.store.lock().unwrap(); @@ -189,49 +180,13 @@ where // here without doing the "fix" .unwrap_or(&default_obj); - let result = TypedValueRef { value, type_name: None }; - - self.iter_field(ctx, &data_path, result).await?; + self.iter_field(ctx, &data_path, value).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 pub trait IRExecutor { type Input; @@ -241,5 +196,5 @@ pub trait IRExecutor { &'a self, ir: &'a IR, ctx: &'a Context<'a, Self::Input, Self::Output>, - ) -> Result, Self::Error>; + ) -> Result; } diff --git a/src/core/jit/exec_const.rs b/src/core/jit/exec_const.rs index d7fcf6aca4..5df3642331 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, TypedValue}; +use super::exec::{Executor, IRExecutor}; use super::{Error, OperationPlan, Request, Response, Result}; use crate::core::app_context::AppContext; use crate::core::http::RequestContext; @@ -56,13 +56,10 @@ 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 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() })?) + Ok(ir.eval(&mut eval_ctx).await?) } } diff --git a/src/core/jit/synth/synth.rs b/src/core/jit/synth/synth.rs index bec53b5907..c6b05a441c 100644 --- a/src/core/jit/synth/synth.rs +++ b/src/core/jit/synth/synth.rs @@ -1,14 +1,13 @@ use async_graphql::Positioned; -use crate::core::ir::TypeName; -use crate::core::jit::exec::{TypedValue, TypedValueRef}; +use crate::core::ir::TypedValue; 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>>; +type ValueStore = Store>>; pub struct Synth { selection: Vec, Value>>, @@ -47,7 +46,7 @@ impl Synth { impl<'a, Value> Synth where - Value: JsonLike<'a> + Clone, + Value: JsonLike<'a> + Clone + std::fmt::Debug, Value::JsonObject<'a>: JsonObjectLike<'a, Value = Value>, { #[inline(always)] @@ -80,7 +79,7 @@ where fn iter( &'a self, node: &'a Field, Value>, - result: Option>, + value: Option<&'a Value>, data_path: &DataPath, ) -> Result> { match self.store.get(&node.id) { @@ -98,15 +97,12 @@ where match data { Data::Single(result) => { - let result = match result { - Ok(result) => result, - Err(err) => return Err(err.clone()), - }; + let value = result.as_ref().map_err(Clone::clone)?; - if !Self::is_array(&node.type_of, &result.value) { + if !Self::is_array(&node.type_of, value) { return Ok(Value::null()); } - self.iter_inner(node, result.as_ref(), data_path) + self.iter_inner(node, value, data_path) } _ => { // TODO: should bailout instead of returning Null @@ -114,8 +110,8 @@ where } } } - None => match result { - Some(result) => self.iter_inner(node, result, data_path), + None => match value { + Some(value) => self.iter_inner(node, value, data_path), None => Ok(Value::null()), }, } @@ -125,15 +121,13 @@ where fn iter_inner( &'a self, node: &'a Field, Value>, - result: TypedValueRef<'a, Value>, + value: &'a Value, data_path: &DataPath, ) -> Result> { 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); @@ -158,20 +152,7 @@ where (_, Some(obj)) => { let mut ans = Value::JsonObject::new(); - 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 Err(Positioned::new( - ValidationError::TypeNameMismatch.into(), - node.pos, - )); - } - } - None => node.type_of.name(), - }; + let type_name = value.get_type_name().unwrap_or(node.type_of.name()); for child in node.nested_iter(type_name) { // all checks for skip must occur in `iter_inner` @@ -180,10 +161,7 @@ where 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)?, - ); + ans.insert_key(child.name.as_str(), self.iter(child, val, data_path)?); } } @@ -192,11 +170,7 @@ where (Some(arr), _) => { let mut ans = vec![]; for (i, val) in arr.iter().enumerate() { - let val = self.iter_inner( - node, - result.map(|_| val), - &data_path.clone().with_index(i), - )?; + let val = self.iter_inner(node, val, &data_path.clone().with_index(i))?; ans.push(val) } Ok(Value::array(ans)) @@ -216,7 +190,6 @@ 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; @@ -272,22 +245,20 @@ 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(TypedValue::new(serde_json::from_str(POSTS).unwrap())), - Self::User1 => Data::Single(TypedValue::new(serde_json::from_str(USER1).unwrap())), + Self::Posts => Data::Single(serde_json::from_str(POSTS).unwrap()), + Self::User1 => Data::Single(serde_json::from_str(USER1).unwrap()), TestData::UsersData => Data::Multiple( vec![ - Data::Single(TypedValue::new(serde_json::from_str(USER1).unwrap())), - Data::Single(TypedValue::new(serde_json::from_str(USER2).unwrap())), + Data::Single(serde_json::from_str(USER1).unwrap()), + Data::Single(serde_json::from_str(USER2).unwrap()), ] .into_iter() .enumerate() .collect(), ), - TestData::Users => { - Data::Single(TypedValue::new(serde_json::from_str(USERS).unwrap())) - } + TestData::Users => Data::Single(serde_json::from_str(USERS).unwrap()), } } } diff --git a/src/core/json/borrow.rs b/src/core/json/borrow.rs index 7e6001cf2b..59fd376d7c 100644 --- a/src/core/json/borrow.rs +++ b/src/core/json/borrow.rs @@ -47,10 +47,31 @@ impl<'ctx> JsonLike<'ctx> for Value<'ctx> { } } + fn into_array(self) -> Option> { + match self { + Value::Array(array) => Some(array), + _ => None, + } + } + fn as_object(&self) -> Option<&Self::JsonObject<'_>> { self.as_object() } + fn as_object_mut(&mut self) -> Option<&mut Self::JsonObject<'ctx>> { + match self { + Value::Object(obj) => Some(obj), + _ => None, + } + } + + fn into_object(self) -> Option> { + match self { + Value::Object(obj) => Some(obj), + _ => None, + } + } + fn as_str(&self) -> Option<&str> { self.as_str() } diff --git a/src/core/json/graphql.rs b/src/core/json/graphql.rs index 0810f70c1e..a41609a80a 100644 --- a/src/core/json/graphql.rs +++ b/src/core/json/graphql.rs @@ -15,7 +15,7 @@ impl<'obj, Value: JsonLike<'obj> + Clone> JsonObjectLike<'obj> for IndexMap Option<&Self::Value> { - self.get(&Name::new(key)) + self.get(key) } fn insert_key(&mut self, key: &'obj str, value: Self::Value) { @@ -33,6 +33,13 @@ impl<'json> JsonLike<'json> for ConstValue { } } + fn into_array(self) -> Option> { + match self { + ConstValue::List(seq) => Some(seq), + _ => None, + } + } + fn as_str(&self) -> Option<&str> { match self { ConstValue::String(s) => Some(s), @@ -110,6 +117,20 @@ impl<'json> JsonLike<'json> for ConstValue { } } + fn as_object_mut(&mut self) -> Option<&mut Self::JsonObject<'_>> { + match self { + ConstValue::Object(map) => Some(map), + _ => None, + } + } + + fn into_object(self) -> Option> { + match self { + ConstValue::Object(map) => Some(map), + _ => None, + } + } + fn object(obj: Self::JsonObject<'json>) -> Self { ConstValue::Object(obj) } diff --git a/src/core/json/json_like.rs b/src/core/json/json_like.rs index 1b74bea3c8..e999a5bf5f 100644 --- a/src/core/json/json_like.rs +++ b/src/core/json/json_like.rs @@ -31,7 +31,10 @@ pub trait JsonLike<'json>: Sized { // Operators fn as_array(&self) -> Option<&Vec>; + fn into_array(self) -> Option>; fn as_object(&self) -> Option<&Self::JsonObject<'_>>; + fn as_object_mut(&mut self) -> Option<&mut Self::JsonObject<'json>>; + fn into_object(self) -> Option>; fn as_str(&self) -> Option<&str>; fn as_i64(&self) -> Option; fn as_u64(&self) -> Option; @@ -41,6 +44,29 @@ pub trait JsonLike<'json>: Sized { fn get_path>(&'json self, path: &[T]) -> Option<&Self>; fn get_key(&'json self, path: &str) -> Option<&Self>; fn group_by(&'json self, path: &[String]) -> HashMap>; + + fn map(self, mut mapper: impl FnMut(Self) -> Result) -> Result { + if self.as_array().is_some() { + let new = self + .into_array() + .unwrap() + .into_iter() + .map(mapper) + .collect::>()?; + + Ok(Self::array(new)) + } else { + mapper(self) + } + } + + fn try_for_each(&self, mut f: impl FnMut(&Self) -> Result<(), Err>) -> Result<(), Err> { + if let Some(arr) = self.as_array() { + arr.iter().try_for_each(f) + } else { + f(self) + } + } } /// A trait for objects that can be used as JSON objects diff --git a/src/core/json/serde.rs b/src/core/json/serde.rs index 0064e7b557..8ee36499c8 100644 --- a/src/core/json/serde.rs +++ b/src/core/json/serde.rs @@ -26,6 +26,14 @@ impl<'json> JsonLike<'json> for serde_json::Value { self.as_array() } + fn into_array(self) -> Option> { + if let Self::Array(vec) = self { + Some(vec) + } else { + None + } + } + fn as_str(&self) -> Option<&str> { self.as_str() } @@ -85,6 +93,18 @@ impl<'json> JsonLike<'json> for serde_json::Value { self.as_object() } + fn as_object_mut(&mut self) -> Option<&mut Self::JsonObject<'_>> { + self.as_object_mut() + } + + fn into_object(self) -> Option> { + if let Self::Object(obj) = self { + Some(obj) + } else { + None + } + } + fn object(obj: Self::JsonObject<'json>) -> Self { serde_json::Value::Object(obj) } diff --git a/tests/core/snapshots/test-union.md_5.snap b/tests/core/snapshots/test-union.md_5.snap new file mode 100644 index 0000000000..442efd36fc --- /dev/null +++ b/tests/core/snapshots/test-union.md_5.snap @@ -0,0 +1,45 @@ +--- +source: tests/core/spec.rs +expression: response +--- +{ + "status": 200, + "headers": { + "content-type": "application/json" + }, + "body": { + "data": { + "foo": { + "__typename": "Foo" + }, + "bar": { + "__typename": "Bar" + }, + "arr": [ + { + "__typename": "Foo" + }, + { + "__typename": "Bar" + }, + { + "__typename": "Foo" + }, + { + "__typename": "Foo" + }, + { + "__typename": "Bar" + } + ], + "nested": { + "foo": { + "__typename": "Foo" + }, + "bar": { + "__typename": "Bar" + } + } + } + } +} diff --git a/tests/execution/test-union.md b/tests/execution/test-union.md index 8d7eea5383..907920a1ee 100644 --- a/tests/execution/test-union.md +++ b/tests/execution/test-union.md @@ -37,6 +37,7 @@ type Query { - request: method: GET url: http://jsonplaceholder.typicode.com/foo + expectedHits: 2 response: status: 200 body: @@ -45,6 +46,7 @@ type Query { - request: method: GET url: http://jsonplaceholder.typicode.com/bar + expectedHits: 2 response: status: 200 body: @@ -53,6 +55,7 @@ type Query { - request: method: GET url: http://jsonplaceholder.typicode.com/nested + expectedHits: 2 response: status: 200 body: @@ -64,6 +67,7 @@ type Query { - request: method: GET url: http://jsonplaceholder.typicode.com/arr + expectedHits: 2 response: status: 200 body: @@ -155,4 +159,28 @@ type Query { } } } + +- method: POST + url: http://localhost:8080/graphql + body: + query: > + query { + foo { + __typename + } + bar { + __typename + } + arr { + __typename + } + nested { + foo { + __typename + } + bar { + __typename + } + } + } ``` From 724a405070eebc718ead9f3cc49db66af6384141 Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Thu, 8 Aug 2024 09:05:47 +0000 Subject: [PATCH 2/3] fix lint --- src/core/blueprint/into_schema.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/blueprint/into_schema.rs b/src/core/blueprint/into_schema.rs index 02aa0041bb..31fe06a964 100644 --- a/src/core/blueprint/into_schema.rs +++ b/src/core/blueprint/into_schema.rs @@ -60,9 +60,7 @@ fn set_default_value( fn to_field_value(value: async_graphql::Value) -> FieldValue<'static> { match value { - ConstValue::List(vec) => { - FieldValue::list(vec.into_iter().map(to_field_value)) - } + ConstValue::List(vec) => FieldValue::list(vec.into_iter().map(to_field_value)), value => { let type_name = value.get_type_name().map(|s| s.to_string()); From ba7d729344bab60f6022c2ca528869a81460183b Mon Sep 17 00:00:00 2001 From: meskill <8974488+meskill@users.noreply.github.com> Date: Mon, 19 Aug 2024 09:59:47 +0000 Subject: [PATCH 3/3] move json list operations to separate trait --- src/core/ir/eval.rs | 2 +- src/core/json/json_like.rs | 23 ----------------------- src/core/json/json_like_list.rs | 28 ++++++++++++++++++++++++++++ src/core/json/mod.rs | 2 ++ 4 files changed, 31 insertions(+), 24 deletions(-) create mode 100644 src/core/json/json_like_list.rs diff --git a/src/core/ir/eval.rs b/src/core/ir/eval.rs index b99d965b10..6bbf8871bd 100644 --- a/src/core/ir/eval.rs +++ b/src/core/ir/eval.rs @@ -6,7 +6,7 @@ use async_graphql_value::ConstValue; use super::eval_io::eval_io; use super::model::{Cache, CacheKey, Map, IR}; use super::{Error, EvalContext, ResolverContextLike, TypedValue}; -use crate::core::json::JsonLike; +use crate::core::json::{JsonLike, JsonLikeList}; use crate::core::serde_value_ext::ValueExt; // Fake trait to capture proper lifetimes. diff --git a/src/core/json/json_like.rs b/src/core/json/json_like.rs index e999a5bf5f..2b10d39eb0 100644 --- a/src/core/json/json_like.rs +++ b/src/core/json/json_like.rs @@ -44,29 +44,6 @@ pub trait JsonLike<'json>: Sized { fn get_path>(&'json self, path: &[T]) -> Option<&Self>; fn get_key(&'json self, path: &str) -> Option<&Self>; fn group_by(&'json self, path: &[String]) -> HashMap>; - - fn map(self, mut mapper: impl FnMut(Self) -> Result) -> Result { - if self.as_array().is_some() { - let new = self - .into_array() - .unwrap() - .into_iter() - .map(mapper) - .collect::>()?; - - Ok(Self::array(new)) - } else { - mapper(self) - } - } - - fn try_for_each(&self, mut f: impl FnMut(&Self) -> Result<(), Err>) -> Result<(), Err> { - if let Some(arr) = self.as_array() { - arr.iter().try_for_each(f) - } else { - f(self) - } - } } /// A trait for objects that can be used as JSON objects diff --git a/src/core/json/json_like_list.rs b/src/core/json/json_like_list.rs new file mode 100644 index 0000000000..6743b380a8 --- /dev/null +++ b/src/core/json/json_like_list.rs @@ -0,0 +1,28 @@ +use super::JsonLike; + +pub trait JsonLikeList<'json>: JsonLike<'json> { + fn map(self, mut mapper: impl FnMut(Self) -> Result) -> Result { + if self.as_array().is_some() { + let new = self + .into_array() + .unwrap() + .into_iter() + .map(mapper) + .collect::>()?; + + Ok(Self::array(new)) + } else { + mapper(self) + } + } + + fn try_for_each(&self, mut f: impl FnMut(&Self) -> Result<(), Err>) -> Result<(), Err> { + if let Some(arr) = self.as_array() { + arr.iter().try_for_each(f) + } else { + f(self) + } + } +} + +impl<'json, T: JsonLike<'json>> JsonLikeList<'json> for T {} diff --git a/src/core/json/mod.rs b/src/core/json/mod.rs index 95efa530d0..c1a11d6919 100644 --- a/src/core/json/mod.rs +++ b/src/core/json/mod.rs @@ -1,12 +1,14 @@ mod borrow; mod graphql; mod json_like; +mod json_like_list; mod json_schema; mod serde; use std::collections::HashMap; pub use json_like::*; +pub use json_like_list::*; pub use json_schema::*; // Highly micro-optimized and benchmarked version of get_path_all