diff --git a/.changeset/nested-array.md b/.changeset/nested-array.md new file mode 100644 index 000000000..2e793f383 --- /dev/null +++ b/.changeset/nested-array.md @@ -0,0 +1,139 @@ +--- +router: patch +executor: patch +--- + +Handle nested abstract items while projecting correctly; + +For the following schema; + +```graphql +interface IContent { + id: ID! +} + +interface IElementContent { + id: ID! +} + +type ContentAChild { + title: String! +} + +type ContentA implements IContent & IElementContent { + id: ID! + contentChildren: [ContentAChild!]! +} + +type ContentBChild { + title: String! +} + +type ContentB implements IContent & IElementContent { + id: ID! + contentChildren: [ContentBChild!]! +} + +type Query { + contentPage: ContentPage! +} + +type ContentPage { + contentBody: [ContentContainer!]! +} + +type ContentContainer { + id: ID! + section: IContent +} +``` + +```graphql +query { + contentPage { + contentBody { + section { + ...ContentAData + ...ContentBData + } + } + } +} + +fragment ContentAData on ContentA { + contentChildren { + title + } +} + +fragment ContentBData on ContentB { + contentChildren { + title + } +} +``` + +If a query like above is executed, the projection plan should be able to handle nested abstract types correctly. + +For the following subgraph response, array items should be handled by their own `__typename` values individually; + +```json +{ + "__typename": "Query", + "contentPage": [ + { + "__typename": "ContentPage", + "contentBody": [ + { + "__typename": "ContentContainer", + "id": "container1", + "section": { + "__typename": "ContentA", + "contentChildren": [] + } + }, + { + "__typename": "ContentContainer", + "id": "container2", + "section": { + "__typename": "ContentB", + "contentChildren": [ + { + "__typename": "ContentBChild", + "title": "contentBChild1" + } + ] + } + } + ] + } + ] +} +``` + +On the other hand if parent types of those don't have `__typename`, we don't need to check the parent types while projecting nested abstract items. In this case, the data to be projected would be; + +```json +{ + "contentPage": { + "contentBody": [ + { + "id": "container1", + "section": { + "__typename": "ContentA", + "id": "contentA1", + "contentChildren": [] + } + }, + { + "id": "container2", + "section": { + "__typename": "ContentB", + "id": "contentB1", + "contentChildren": null + } + } + ] + } +} +``` diff --git a/Cargo.lock b/Cargo.lock index 47187be13..0e6f5808a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2053,6 +2053,7 @@ dependencies = [ "http", "http-body-util", "hyper", + "indexmap 2.12.0", "insta", "jsonwebtoken", "lasso2", diff --git a/Cargo.toml b/Cargo.toml index 051df6d08..796478b0e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,3 +63,4 @@ vrl = { version = "0.29.0", features = ["compiler", "parser", "value", "diagnost regex-automata = "0.4.10" bytes = "1.10.1" ahash = "0.8.12" +indexmap = "2.10.0" diff --git a/bin/router/Cargo.toml b/bin/router/Cargo.toml index a68b3c0b1..fb9c7cb13 100644 --- a/bin/router/Cargo.toml +++ b/bin/router/Cargo.toml @@ -47,12 +47,13 @@ reqwest-middleware = { workspace = true } vrl = { workspace = true } serde_json = { workspace = true } regex-automata = { workspace = true } +tokio-util = { workspace = true } +indexmap = { workspace = true } mimalloc = { version = "0.1.48", features = ["v3"] } moka = { version = "0.12.10", features = ["future"] } hive-console-sdk = "0.2.2" ulid = "1.2.1" -tokio-util = "0.7.16" cookie = "0.18.1" arc-swap = "1.7.1" lasso2 = "0.8.2" diff --git a/bin/router/src/pipeline/authorization/mod.rs b/bin/router/src/pipeline/authorization/mod.rs index 93393edeb..fe6e0414f 100644 --- a/bin/router/src/pipeline/authorization/mod.rs +++ b/bin/router/src/pipeline/authorization/mod.rs @@ -31,6 +31,7 @@ use hive_router_plan_executor::projection::plan::FieldProjectionPlan; use hive_router_plan_executor::response::graphql_error::{GraphQLError, GraphQLErrorExtensions}; use hive_router_query_planner::ast::operation::OperationDefinition; +use indexmap::IndexMap; pub use metadata::{ AuthorizationMetadata, AuthorizationMetadataError, ScopeId, ScopeInterner, UserAuthContext, }; @@ -53,7 +54,7 @@ pub enum AuthorizationDecision { /// The operation was modified to remove unauthorized parts. Continue with the new operation. Modified { new_operation_definition: OperationDefinition, - new_projection_plan: Vec, + new_projection_plan: IndexMap, errors: Vec, }, /// The operation should be aborted due to unauthorized access and reject mode being enabled. diff --git a/bin/router/src/pipeline/authorization/rebuilder.rs b/bin/router/src/pipeline/authorization/rebuilder.rs index 531a47fec..ebc2b21fb 100644 --- a/bin/router/src/pipeline/authorization/rebuilder.rs +++ b/bin/router/src/pipeline/authorization/rebuilder.rs @@ -1,10 +1,11 @@ -use std::{collections::HashSet, sync::Arc}; +use std::collections::HashSet; use hive_router_plan_executor::projection::plan::{FieldProjectionPlan, ProjectionValueSource}; use hive_router_query_planner::ast::{ operation::OperationDefinition, selection_item::SelectionItem, selection_set::SelectionSet, value::Value, }; +use indexmap::IndexMap; use crate::pipeline::authorization::tree::{PathIndex, UnauthorizedPathTrie}; @@ -121,9 +122,9 @@ fn rebuild_authorized_selection_set<'op>( /// Rebuilds the projection plan to exclude unauthorized fields. pub(super) fn rebuild_authorized_projection_plan( - original_plans: &Vec, + original_plans: &IndexMap, unauthorized_path_trie: &UnauthorizedPathTrie, -) -> Vec { +) -> IndexMap { rebuild_authorized_projection_plan_recursive( original_plans, unauthorized_path_trie, @@ -134,23 +135,25 @@ pub(super) fn rebuild_authorized_projection_plan( /// Recursively filters projection plans. Unauthorized fields become null. fn rebuild_authorized_projection_plan_recursive( - original_plans: &Vec, + original_plans: &IndexMap, unauthorized_path_trie: &UnauthorizedPathTrie, path_position: PathIndex, -) -> Option> { - let mut authorized_plans = Vec::with_capacity(original_plans.len()); +) -> Option> { + let mut authorized_plans = IndexMap::with_capacity(original_plans.len()); - for plan in original_plans { - let path_segment = &plan.response_key; + for (path_segment, plan) in original_plans { let Some((child_path_position, is_unauthorized)) = unauthorized_path_trie.find_field(path_position, path_segment) else { - authorized_plans.push(plan.clone()); + authorized_plans.insert(path_segment.clone(), plan.clone()); continue; }; if is_unauthorized { - authorized_plans.push(plan.with_new_value(ProjectionValueSource::Null)); + authorized_plans.insert( + path_segment.clone(), + plan.with_new_value(ProjectionValueSource::Null), + ); continue; } @@ -162,12 +165,11 @@ fn rebuild_authorized_projection_plan_recursive( selections, unauthorized_path_trie, child_path_position, - ) - .map(Arc::new), + ), }, other => other.clone(), }; - authorized_plans.push(plan.with_new_value(new_value)); + authorized_plans.insert(path_segment.clone(), plan.with_new_value(new_value)); } if authorized_plans.is_empty() { diff --git a/bin/router/src/pipeline/normalize.rs b/bin/router/src/pipeline/normalize.rs index a8c1ec6ce..40c42517a 100644 --- a/bin/router/src/pipeline/normalize.rs +++ b/bin/router/src/pipeline/normalize.rs @@ -5,6 +5,7 @@ use hive_router_plan_executor::introspection::partition::partition_operation; use hive_router_plan_executor::projection::plan::FieldProjectionPlan; use hive_router_query_planner::ast::normalization::normalize_operation; use hive_router_query_planner::ast::operation::OperationDefinition; +use indexmap::IndexMap; use ntex::web::HttpRequest; use xxhash_rust::xxh3::Xxh3; @@ -20,7 +21,7 @@ pub struct GraphQLNormalizationPayload { pub operation_for_plan: Arc, pub operation_for_introspection: Option>, pub root_type_name: &'static str, - pub projection_plan: Arc>, + pub projection_plan: Arc>, } #[inline] diff --git a/lib/executor/Cargo.toml b/lib/executor/Cargo.toml index 55d364160..45c779b73 100644 --- a/lib/executor/Cargo.toml +++ b/lib/executor/Cargo.toml @@ -48,7 +48,7 @@ hyper-util = { version = "0.1.16", features = [ bytes = { workspace = true } itoa = "1.0.15" ryu = "1.0.20" -indexmap = "2.10.0" +indexmap = { workspace = true } bumpalo = "3.19.0" [dev-dependencies] diff --git a/lib/executor/src/execution/plan.rs b/lib/executor/src/execution/plan.rs index 7fb657f03..236976aad 100644 --- a/lib/executor/src/execution/plan.rs +++ b/lib/executor/src/execution/plan.rs @@ -7,6 +7,7 @@ use hive_router_query_planner::planner::plan_nodes::{ QueryPlan, SequenceNode, }; use http::HeaderMap; +use indexmap::IndexMap; use serde::Deserialize; use sonic_rs::ValueRef; @@ -50,7 +51,7 @@ use crate::{ pub struct QueryPlanExecutionContext<'exec, 'req> { pub query_plan: &'exec QueryPlan, - pub projection_plan: &'exec Vec, + pub projection_plan: &'exec IndexMap, pub headers_plan: &'exec HeaderRulesPlan, pub variable_values: &'exec Option>, pub extensions: Option>, diff --git a/lib/executor/src/projection/plan.rs b/lib/executor/src/projection/plan.rs index 6134668fe..46629fc4c 100644 --- a/lib/executor/src/projection/plan.rs +++ b/lib/executor/src/projection/plan.rs @@ -1,6 +1,5 @@ use ahash::HashSet; use indexmap::IndexMap; -use std::sync::Arc; use tracing::warn; use hive_router_query_planner::{ @@ -24,7 +23,7 @@ pub enum TypeCondition { pub enum ProjectionValueSource { /// Represents the entire response data from subgraphs. ResponseData { - selections: Option>>, + selections: Option>, }, /// Represents a null value. Null, @@ -33,7 +32,6 @@ pub enum ProjectionValueSource { #[derive(Debug, Clone)] pub struct FieldProjectionPlan { pub field_name: String, - pub field_type: String, pub response_key: String, pub conditions: FieldProjectionCondition, pub value: ProjectionValueSource, @@ -50,18 +48,30 @@ pub enum FieldProjectionCondition { And(Box, Box), } -pub enum FieldProjectionConditionError { - InvalidParentType, - InvalidFieldType, - Skip, - InvalidEnumValue, +#[derive(Debug)] +pub enum FieldProjectionConditionError<'a> { + InvalidParentType { + expected: &'a TypeCondition, + found: &'a str, + }, + InvalidFieldType { + expected: &'a TypeCondition, + found: &'a str, + }, + Skip { + variable_name: &'a str, + }, + InvalidEnumValue { + expected: &'a HashSet, + found_value: &'a str, + }, } impl FieldProjectionPlan { pub fn from_operation( operation: &OperationDefinition, schema_metadata: &SchemaMetadata, - ) -> (&'static str, Vec) { + ) -> (&'static str, IndexMap) { let root_type_name = match operation.operation_kind { Some(OperationKind::Query) => "Query", Some(OperationKind::Mutation) => "Mutation", @@ -97,7 +107,7 @@ impl FieldProjectionPlan { schema_metadata: &SchemaMetadata, parent_type_name: &str, parent_condition: &FieldProjectionCondition, - ) -> Option> { + ) -> Option> { let mut field_selections: IndexMap = IndexMap::new(); for selection_item in &selection_set.items { @@ -131,45 +141,34 @@ impl FieldProjectionPlan { if field_selections.is_empty() { None } else { - Some(field_selections.into_values().collect()) + Some(field_selections) } } - fn apply_directive_conditions( - mut condition: FieldProjectionCondition, - include_if: &Option, - skip_if: &Option, - ) -> FieldProjectionCondition { - if let Some(include_if_var) = include_if { - condition = FieldProjectionCondition::And( - Box::new(condition), - Box::new(FieldProjectionCondition::IncludeIfVariable( - include_if_var.clone(), - )), - ); - } - if let Some(skip_if_var) = skip_if { - condition = FieldProjectionCondition::And( - Box::new(condition), - Box::new(FieldProjectionCondition::SkipIfVariable( - skip_if_var.clone(), - )), - ); + fn merge_selections( + existing_selections: &mut Option>, + new_selections: Option>, + ) { + if let Some(new_selections) = new_selections { + match existing_selections { + Some(selections) => { + for (_key, new_selection) in new_selections { + new_selection.merge_into(selections); + } + } + None => *existing_selections = Some(new_selections), + } } - condition } - fn merge_plan( - field_selections: &mut IndexMap, - plan_to_merge: FieldProjectionPlan, - ) { - if let Some(existing_plan) = field_selections.get_mut(&plan_to_merge.response_key) { + fn merge_into(self, field_selections: &mut IndexMap) { + if let Some(existing_plan) = field_selections.get_mut(&self.response_key) { existing_plan.conditions = FieldProjectionCondition::Or( Box::new(existing_plan.conditions.clone()), - Box::new(plan_to_merge.conditions), + Box::new(self.conditions), ); - match (&mut existing_plan.value, plan_to_merge.value) { + match (&mut existing_plan.value, self.value) { ( ProjectionValueSource::ResponseData { selections: existing_selections, @@ -178,17 +177,7 @@ impl FieldProjectionPlan { selections: new_selections, }, ) => { - if let Some(new_selections) = new_selections { - match existing_selections { - Some(selections) => { - Arc::make_mut(selections).extend( - Arc::try_unwrap(new_selections) - .unwrap_or_else(|arc| (*arc).clone()), - ); - } - None => *existing_selections = Some(new_selections), - } - } + Self::merge_selections(existing_selections, new_selections); } (ProjectionValueSource::Null, ProjectionValueSource::Null) => { // Both plans have `Null` value source, so nothing to merge @@ -202,7 +191,7 @@ impl FieldProjectionPlan { } } } else { - field_selections.insert(plan_to_merge.response_key.clone(), plan_to_merge); + field_selections.insert(self.response_key.clone(), self); } } @@ -252,21 +241,16 @@ impl FieldProjectionPlan { .get_possible_types(&field_type), ) }; - let conditions_for_selections = Self::apply_directive_conditions( - FieldProjectionCondition::ParentTypeCondition(type_condition.clone()), - &field.include_if, - &field.skip_if, - ); + let conditions_for_selections = + FieldProjectionCondition::ParentTypeCondition(type_condition.clone()) + .apply_directive_conditions(&field.include_if, &field.skip_if); let mut condition_for_field = FieldProjectionCondition::And( Box::new(parent_condition.clone()), Box::new(FieldProjectionCondition::FieldTypeCondition(type_condition)), ); - condition_for_field = Self::apply_directive_conditions( - condition_for_field, - &field.include_if, - &field.skip_if, - ); + condition_for_field = + condition_for_field.apply_directive_conditions(&field.include_if, &field.skip_if); if let Some(enum_values) = schema_metadata.enum_values.get(&field_type) { condition_for_field = FieldProjectionCondition::And( @@ -279,7 +263,6 @@ impl FieldProjectionPlan { let new_plan = FieldProjectionPlan { field_name: field_name.to_string(), - field_type: field_type.clone(), response_key, conditions: condition_for_field, value: ProjectionValueSource::ResponseData { @@ -288,12 +271,11 @@ impl FieldProjectionPlan { schema_metadata, &field_type, &conditions_for_selections, - ) - .map(Arc::new), + ), }, }; - Self::merge_plan(field_selections, new_plan); + new_plan.merge_into(field_selections); } fn process_inline_fragment( @@ -320,11 +302,8 @@ impl FieldProjectionPlan { )), ); - condition_for_fragment = Self::apply_directive_conditions( - condition_for_fragment, - &inline_fragment.include_if, - &inline_fragment.skip_if, - ); + condition_for_fragment = condition_for_fragment + .apply_directive_conditions(&inline_fragment.include_if, &inline_fragment.skip_if); if let Some(inline_fragment_selections) = Self::from_selection_set( &inline_fragment.selections, @@ -332,8 +311,8 @@ impl FieldProjectionPlan { inline_fragment_type, &condition_for_fragment, ) { - for selection in inline_fragment_selections { - Self::merge_plan(field_selections, selection); + for (_key, selection) in inline_fragment_selections { + selection.merge_into(field_selections); } } } @@ -341,10 +320,35 @@ impl FieldProjectionPlan { pub fn with_new_value(&self, new_value: ProjectionValueSource) -> FieldProjectionPlan { FieldProjectionPlan { field_name: self.field_name.clone(), - field_type: self.field_type.clone(), response_key: self.response_key.clone(), conditions: self.conditions.clone(), value: new_value, } } } + +impl FieldProjectionCondition { + fn apply_directive_conditions( + mut self, + include_if: &Option, + skip_if: &Option, + ) -> FieldProjectionCondition { + if let Some(include_if_var) = include_if { + self = FieldProjectionCondition::And( + Box::new(self), + Box::new(FieldProjectionCondition::IncludeIfVariable( + include_if_var.clone(), + )), + ); + } + if let Some(skip_if_var) = skip_if { + self = FieldProjectionCondition::And( + Box::new(self), + Box::new(FieldProjectionCondition::SkipIfVariable( + skip_if_var.clone(), + )), + ); + } + self + } +} diff --git a/lib/executor/src/projection/response.rs b/lib/executor/src/projection/response.rs index 9e8d3897d..1630c0a05 100644 --- a/lib/executor/src/projection/response.rs +++ b/lib/executor/src/projection/response.rs @@ -6,15 +6,16 @@ use crate::projection::plan::{ use crate::response::graphql_error::{GraphQLError, GraphQLErrorExtensions}; use crate::response::value::Value; use bytes::BufMut; +use indexmap::IndexMap; use sonic_rs::JsonValueTrait; use std::collections::HashMap; -use tracing::{instrument, warn}; +use tracing::{instrument, trace, warn}; use crate::json_writer::{write_and_escape_string, write_f64, write_i64, write_u64}; use crate::utils::consts::{ - CLOSE_BRACE, CLOSE_BRACKET, COLON, COMMA, EMPTY_OBJECT, FALSE, NULL, OPEN_BRACE, OPEN_BRACKET, - QUOTE, TRUE, TYPENAME_FIELD_NAME, + CLOSE_BRACE, CLOSE_BRACKET, COLON, COMMA, EMPTY_ARRAY, EMPTY_OBJECT, FALSE, NULL, OPEN_BRACE, + OPEN_BRACKET, QUOTE, TRUE, TYPENAME_FIELD_NAME, }; #[instrument(level = "trace", skip_all)] @@ -23,7 +24,7 @@ pub fn project_by_operation( errors: Vec, extensions: &Option>, operation_type_name: &str, - selections: &Vec, + selections: &IndexMap, variable_values: &Option>, response_size_estimate: usize, ) -> Result, ProjectionError> { @@ -44,7 +45,7 @@ pub fn project_by_operation( &mut errors, selections, variable_values, - operation_type_name, + Some(operation_type_name), &mut buffer, &mut first, ); @@ -134,6 +135,7 @@ fn project_selection_set( ) { match data { Value::Array(arr) => { + // If the data is an array, we project each item in the array buffer.put(OPEN_BRACKET); let mut first = true; for item in arr.iter() { @@ -151,11 +153,8 @@ fn project_selection_set( selections: Some(selections), } => { let mut first = true; - let type_name = obj - .iter() - .position(|(k, _)| k == &TYPENAME_FIELD_NAME) - .and_then(|idx| obj[idx].1.as_str()) - .unwrap_or(selection.field_type.as_str()); + let type_name = + get_value_by_key(obj, TYPENAME_FIELD_NAME).and_then(|v| v.as_str()); project_selection_set_with_map( obj, errors, @@ -189,127 +188,226 @@ fn project_selection_set( }; } -// TODO: simplfy args #[allow(clippy::too_many_arguments)] -fn project_selection_set_with_map( - obj: &Vec<(&str, Value)>, - errors: &mut Vec, - plans: &Vec, +fn project_field( + field_val: Option<&Value>, + plan: &FieldProjectionPlan, + parent_type_name: Option<&str>, variable_values: &Option>, - parent_type_name: &str, buffer: &mut Vec, first: &mut bool, + errors: &mut Vec, + in_array: bool, ) { - for plan in plans { - let field_val = obj - .iter() - .position(|(k, _)| k == &plan.response_key.as_str()) - .map(|idx| &obj[idx].1); - let typename_field = field_val - .and_then(|value| value.as_object()) - .and_then(|obj| { - obj.iter() - .position(|(k, _)| k == &TYPENAME_FIELD_NAME) - .and_then(|idx| obj[idx].1.as_str()) - }) - .unwrap_or(&plan.field_type); + if let Some(Value::Array(arr)) = field_val { + if *first { + buffer.put(OPEN_BRACE); + } else { + buffer.put(COMMA); + } + *first = false; - let res = check( - &plan.conditions, - parent_type_name, - typename_field, - field_val, - variable_values, - ); + if !in_array { + buffer.put(QUOTE); + buffer.put(plan.response_key.as_bytes()); + buffer.put(QUOTE); + buffer.put(COLON); + } - match res { - Ok(_) => { - if *first { - buffer.put(OPEN_BRACE); + let mut first = true; + for item in arr.iter() { + project_field( + Some(item), + plan, + parent_type_name, + variable_values, + buffer, + &mut first, + errors, + true, + ); + } + + if first { + // If no selections were made, we should return an empty array + buffer.put(EMPTY_ARRAY); + } else { + buffer.put(CLOSE_BRACKET); + } + + return; + } + + let typename_field = field_val + .and_then(|value| value.as_object()) + .and_then(|obj| get_value_by_key(obj, TYPENAME_FIELD_NAME)) + .and_then(|v| v.as_str()); + + let res = check( + &plan.conditions, + parent_type_name, + typename_field, + field_val, + variable_values, + ); + + match res { + Ok(_) => { + if *first { + if in_array { + buffer.put(OPEN_BRACKET); } else { - buffer.put(COMMA); + buffer.put(OPEN_BRACE); } - *first = false; + } else { + buffer.put(COMMA); + } + *first = false; + if !in_array { buffer.put(QUOTE); buffer.put(plan.response_key.as_bytes()); buffer.put(QUOTE); buffer.put(COLON); + } - match &plan.value { - ProjectionValueSource::Null => { - buffer.put(NULL); - continue; - } - ProjectionValueSource::ResponseData { .. } => { - if let Some(field_val) = field_val { - project_selection_set(field_val, errors, plan, variable_values, buffer); - } else if plan.field_name == TYPENAME_FIELD_NAME { + match &plan.value { + ProjectionValueSource::Null => { + buffer.put(NULL); + } + ProjectionValueSource::ResponseData { .. } => { + if let Some(field_val) = field_val { + project_selection_set(field_val, errors, plan, variable_values, buffer); + } else if plan.field_name == TYPENAME_FIELD_NAME { + if let Some(parent_type_name) = parent_type_name { // If the field is TYPENAME_FIELD, we should set it to the parent type name buffer.put(QUOTE); buffer.put(parent_type_name.as_bytes()); buffer.put(QUOTE); } else { - // If the field is not found in the object, set it to Null buffer.put(NULL); } + } else { + // If the field is not found in the object, set it to Null + buffer.put(NULL); } } } - Err(FieldProjectionConditionError::Skip) => { - // Skip this field - continue; - } - Err(FieldProjectionConditionError::InvalidParentType) => { - // Skip this field as the parent type does not match - continue; - } - Err(FieldProjectionConditionError::InvalidEnumValue) => { - if *first { - buffer.put(OPEN_BRACE); + } + Err(FieldProjectionConditionError::Skip { variable_name }) => { + // Skip this field + trace!( + "Skipping field: {} due to variable: {}", + plan.response_key, + variable_name + ); + } + Err(FieldProjectionConditionError::InvalidParentType { expected, found }) => { + // Skip this field as the parent type does not match + trace!( + "Skipping field: {} due to parent type mismatch. Expected: {:?}, Found: {}", + plan.response_key, + expected, + found + ); + } + Err(FieldProjectionConditionError::InvalidEnumValue { + expected, + found_value, + }) => { + trace!( + "Invalid enum value for field: {}. Expected one of: {:?}, Found: {:?}", + plan.response_key, + expected, + found_value, + ); + if *first { + if in_array { + buffer.put(OPEN_BRACKET); } else { - buffer.put(COMMA); + buffer.put(OPEN_BRACE); } - *first = false; + } else { + buffer.put(COMMA); + } + *first = false; + if !in_array { buffer.put(QUOTE); buffer.put(plan.response_key.as_bytes()); buffer.put(QUOTE); buffer.put(COLON); - buffer.put(NULL); - errors.push(GraphQLError { - message: "Value is not a valid enum value".to_string(), - locations: None, - path: None, - extensions: GraphQLErrorExtensions::default(), - }); } - Err(FieldProjectionConditionError::InvalidFieldType) => { - if *first { - buffer.put(OPEN_BRACE); + + buffer.put(NULL); + errors.push(GraphQLError { + message: format!("Invalid enum value {:?}", found_value), + locations: None, + path: None, + extensions: GraphQLErrorExtensions::default(), + }); + } + Err(FieldProjectionConditionError::InvalidFieldType { expected, found }) => { + trace!( + "Skipping field: {} due to field type mismatch. Expected: {:?}, Found: {}, field_val: {:#?}, field_type: {:#?}, condition: {:#?}", + plan.response_key, expected, found, field_val, typename_field, plan.conditions + ); + if *first { + if in_array { + buffer.put(OPEN_BRACKET); } else { - buffer.put(COMMA); + buffer.put(OPEN_BRACE); } - *first = false; + } else { + buffer.put(COMMA); + } + *first = false; - // Skip this field as the field type does not match + // Skip this field as the field type does not match + if !in_array { buffer.put(QUOTE); buffer.put(plan.response_key.as_bytes()); buffer.put(QUOTE); buffer.put(COLON); - buffer.put(NULL); } + buffer.put(NULL); } } } -fn check( - cond: &FieldProjectionCondition, - parent_type_name: &str, - field_type_name: &str, - field_value: Option<&Value>, +// TODO: simplfy args +#[allow(clippy::too_many_arguments)] +fn project_selection_set_with_map( + obj: &Vec<(&str, Value)>, + errors: &mut Vec, + plans: &IndexMap, variable_values: &Option>, -) -> Result<(), FieldProjectionConditionError> { + parent_type_name: Option<&str>, + buffer: &mut Vec, + first: &mut bool, +) { + for (_, plan) in plans { + let field_val = get_value_by_key(obj, &plan.response_key); + project_field( + field_val, + plan, + parent_type_name, + variable_values, + buffer, + first, + errors, + false, + ); + } +} + +fn check<'a>( + cond: &'a FieldProjectionCondition, + parent_type_name: Option<&'a str>, + field_type_name: Option<&'a str>, + field_value: Option<&'a Value>, + variable_values: &'a Option>, +) -> Result<(), FieldProjectionConditionError<'a>> { match cond { FieldProjectionCondition::And(condition_a, condition_b) => check( condition_a, @@ -318,13 +416,15 @@ fn check( field_value, variable_values, ) - .and(check( - condition_b, - parent_type_name, - field_type_name, - field_value, - variable_values, - )), + .and_then(|()| { + check( + condition_b, + parent_type_name, + field_type_name, + field_value, + variable_values, + ) + }), FieldProjectionCondition::Or(condition_a, condition_b) => check( condition_a, parent_type_name, @@ -332,13 +432,15 @@ fn check( field_value, variable_values, ) - .or(check( - condition_b, - parent_type_name, - field_type_name, - field_value, - variable_values, - )), + .or_else(|_err| { + check( + condition_b, + parent_type_name, + field_type_name, + field_value, + variable_values, + ) + }), FieldProjectionCondition::IncludeIfVariable(variable_name) => { if let Some(values) = variable_values { if values @@ -347,10 +449,10 @@ fn check( { Ok(()) } else { - Err(FieldProjectionConditionError::Skip) + Err(FieldProjectionConditionError::Skip { variable_name }) } } else { - Err(FieldProjectionConditionError::Skip) + Err(FieldProjectionConditionError::Skip { variable_name }) } } FieldProjectionCondition::SkipIfVariable(variable_name) => { @@ -359,40 +461,55 @@ fn check( .get(variable_name) .is_some_and(|v| v.as_bool().unwrap_or(false)) { - return Err(FieldProjectionConditionError::Skip); + return Err(FieldProjectionConditionError::Skip { variable_name }); } } Ok(()) } FieldProjectionCondition::ParentTypeCondition(type_condition) => { - let is_valid = match type_condition { - TypeCondition::Exact(expected_type) => parent_type_name == expected_type, - TypeCondition::OneOf(possible_types) => possible_types.contains(parent_type_name), - }; - if is_valid { - Ok(()) - } else { - Err(FieldProjectionConditionError::InvalidParentType) + if let Some(parent_type_name) = parent_type_name { + let is_valid = match type_condition { + TypeCondition::Exact(expected_type) => parent_type_name == expected_type, + TypeCondition::OneOf(possible_types) => { + possible_types.contains(parent_type_name) + } + }; + if !is_valid { + return Err(FieldProjectionConditionError::InvalidParentType { + expected: type_condition, + found: parent_type_name, + }); + } } + Ok(()) } FieldProjectionCondition::FieldTypeCondition(type_condition) => { - let is_valid = match type_condition { - TypeCondition::Exact(expected_type) => field_type_name == expected_type, - TypeCondition::OneOf(possible_types) => possible_types.contains(field_type_name), - }; + if let Some(field_type_name) = field_type_name { + let is_valid = match type_condition { + TypeCondition::Exact(expected_type) => field_type_name == expected_type, + TypeCondition::OneOf(possible_types) => { + possible_types.contains(field_type_name) + } + }; - if is_valid { - Ok(()) - } else { - Err(FieldProjectionConditionError::InvalidFieldType) + if !is_valid { + return Err(FieldProjectionConditionError::InvalidFieldType { + expected: type_condition, + found: field_type_name, + }); + } } + Ok(()) } FieldProjectionCondition::EnumValuesCondition(enum_values) => { if let Some(Value::String(string_value)) = field_value { if enum_values.contains(&string_value.to_string()) { Ok(()) } else { - Err(FieldProjectionConditionError::InvalidEnumValue) + Err(FieldProjectionConditionError::InvalidEnumValue { + expected: enum_values, + found_value: string_value, + }) } } else { Ok(()) @@ -401,13 +518,26 @@ fn check( } } +fn get_value_by_key<'a, TKey: PartialEq>( + obj: &'a Vec<(TKey, Value)>, + key: TKey, +) -> Option<&'a Value<'a>> { + obj.iter() + .position(|(k, _)| k == &key) + .map(|idx| &obj[idx].1) +} + #[cfg(test)] mod tests { use graphql_parser::query::Definition; use hive_router_query_planner::{ - ast::{document::NormalizedDocument, normalization::create_normalized_document}, + ast::{ + document::NormalizedDocument, + normalization::{create_normalized_document, normalize_operation}, + }, consumer_schema::ConsumerSchema, - utils::parsing::parse_operation, + state::supergraph_state::SupergraphState, + utils::parsing::{parse_operation, parse_schema}, }; use sonic_rs::json; @@ -494,4 +624,225 @@ mod tests { let expected_response = r#"{"data":{"metadatas":[{"id":"meta1","data":{"float":41.5,"int":-42,"str":"value1","unsigned":123}},{"id":"meta2","data":null}]}}"#; assert_eq!(projected_str, expected_response); } + + static CONFLICTING_SELECTIONS_SUPERGRAPH: &'static str = r#" +interface IContent { + id: ID! +} + +interface IElementContent { + id: ID! +} + +type ContentAChild { + title: String! +} + +type ContentA implements IContent & IElementContent { + id: ID! + contentChildren: [ContentAChild!]! +} + +type ContentBChild { + title: String! +} + +type ContentB implements IContent & IElementContent { + id: ID! + contentChildren: [ContentBChild!]! +} + +type Query { + contentPage: ContentPage! +} + +type ContentPage { + contentBody: [ContentContainer!]! +} + +type ContentContainer { + id: ID! + section: IContent +} + "#; + + // This tests if the projection works correctly when there are conflicting selections with typenames + #[test] + fn project_conflicting_selections_1() { + let supergraph = parse_schema(CONFLICTING_SELECTIONS_SUPERGRAPH); + let supergraph_state = SupergraphState::new(&supergraph); + let consumer_schema = ConsumerSchema::new_from_supergraph(&supergraph); + let schema_metadata = consumer_schema.schema_metadata(); + let query = parse_operation( + r#" +query { + contentPage { + contentBody { + section { + ...ContentAData + ...ContentBData + } + } + } +} + +fragment ContentAData on ContentA { + contentChildren { + title + } +} + +fragment ContentBData on ContentB { + contentChildren { + title + } +} + "#, + ); + let normalized_operation: NormalizedDocument = + normalize_operation(&supergraph_state, &query, None).unwrap(); + let (operation_type_name, selections) = + FieldProjectionPlan::from_operation(&normalized_operation.operation, &schema_metadata); + let data_json = json!({ + "__typename": "Query", + "contentPage": [ + { + "__typename": "ContentPage", + "contentBody": [ + { + "__typename": "ContentContainer", + "id": "container1", + "section": { + "__typename": "ContentA", + "contentChildren": [] + } + }, + { + "__typename": "ContentContainer", + "id": "container2", + "section": { + "__typename": "ContentB", + "contentChildren": [ + { + "__typename": "ContentBChild", + "title": "contentBChild1" + } + ] + } + } + ] + } + ] + }); + let data = Value::from(data_json.as_ref()); + let projection = project_by_operation( + &data, + vec![], + &None, + operation_type_name, + &selections, + &None, + 1000, + ); + let projected_bytes = projection.unwrap(); + let projected_str = String::from_utf8(projected_bytes).unwrap(); + insta::assert_snapshot!( + projected_str, + @r#"{"data":{"contentPage":[{"contentBody":[{"section":{"contentChildren":[]}},{"section":{"contentChildren":[{"title":"contentBChild1"}]}}]}]}}"# + ) + } + + // This tests if the projection works correctly when there are conflicting selections without typenames + #[test] + fn project_conflicting_selections_2() { + let supergraph = parse_schema(CONFLICTING_SELECTIONS_SUPERGRAPH); + let supergraph_state = SupergraphState::new(&supergraph); + let consumer_schema = ConsumerSchema::new_from_supergraph(&supergraph); + let schema_metadata = consumer_schema.schema_metadata(); + let query = parse_operation( + r#" +query { + contentPage { + contentBody { + section { + ...SectionData + } + } + } +} + +fragment SectionData on IContent { + __typename + id + ...ContentAData + ...ContentBData +} + +fragment ContentAData on ContentA { + id + contentChildren { + title + } + __typename +} + +fragment ContentBData on ContentB { + id + contentChildren { + title + } +} + "#, + ); + let normalized_operation: NormalizedDocument = + normalize_operation(&supergraph_state, &query, None).unwrap(); + let (operation_type_name, selections) = + FieldProjectionPlan::from_operation(&normalized_operation.operation, &schema_metadata); + let data_json = json!({ + "contentPage": { + "contentBody": [ + { + "id": "container1", + "section": { + "__typename": "ContentA", + "id": "contentA1", + "contentChildren": [] + } + }, + { + "id": "container2", + "section": { + "__typename": "ContentB", + "id": "contentB1", + "contentChildren": [ + { + "title": "contentBChild1" + } + ] + } + }, + { + "id": "container3", + "section": null + } + ] + } + }); + let data = Value::from(data_json.as_ref()); + let projection = project_by_operation( + &data, + vec![], + &None, + operation_type_name, + &selections, + &None, + 1000, + ); + let projected_bytes = projection.unwrap(); + let projected_str = String::from_utf8(projected_bytes).unwrap(); + insta::assert_snapshot!( + projected_str, + @r#"{"data":{"contentPage":{"contentBody":[{"section":{"__typename":"ContentA","id":"contentA1","contentChildren":[]}},{"section":{"__typename":"ContentB","id":"contentB1","contentChildren":[{"title":"contentBChild1"}]}},{"section":null}]}}}"# + ) + } } diff --git a/lib/executor/src/response/value.rs b/lib/executor/src/response/value.rs index 6dcbc9e0d..7770e3905 100644 --- a/lib/executor/src/response/value.rs +++ b/lib/executor/src/response/value.rs @@ -14,7 +14,7 @@ use xxhash_rust::xxh3::Xxh3; use crate::{introspection::schema::PossibleTypes, utils::consts::TYPENAME_FIELD_NAME}; -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum Value<'a> { Null, F64(f64), diff --git a/lib/executor/src/utils/consts.rs b/lib/executor/src/utils/consts.rs index 51a0d2792..8dcd8fe56 100644 --- a/lib/executor/src/utils/consts.rs +++ b/lib/executor/src/utils/consts.rs @@ -11,3 +11,4 @@ pub const CLOSE_BRACE: &[u8] = b"}"; pub const OPEN_BRACKET: &[u8] = b"["; pub const CLOSE_BRACKET: &[u8] = b"]"; pub const EMPTY_OBJECT: &[u8] = b"{}"; +pub const EMPTY_ARRAY: &[u8] = b"[]"; diff --git a/lib/query-planner/src/ast/selection_item.rs b/lib/query-planner/src/ast/selection_item.rs index db3b5cac7..c2c80f6f8 100644 --- a/lib/query-planner/src/ast/selection_item.rs +++ b/lib/query-planner/src/ast/selection_item.rs @@ -225,8 +225,8 @@ impl<'a, T: query_ast::Text<'a>> From> for Selection query_ast::Selection::InlineFragment(fragment) => { SelectionItem::InlineFragment(fragment.into()) } - query_ast::Selection::FragmentSpread(_) => { - panic!("Received a fragment spread, but it should be inlined after normalization"); + query_ast::Selection::FragmentSpread(spread) => { + panic!("Received a fragment spread for {}, but it should be inlined after normalization", spread.fragment_name.as_ref()); } } }