From 9b1920d6fa274c804130942f9d04d9bc7a14c1bb Mon Sep 17 00:00:00 2001 From: Daniel Harvey Date: Fri, 15 Mar 2024 13:24:15 +0000 Subject: [PATCH] basic typechecker for preset argument values (#350) V3_GIT_ORIGIN_REV_ID: 6dac85968ea599812db56568a9422657b5cf96f4 --- v3/engine/src/metadata/resolved.rs | 1 + v3/engine/src/metadata/resolved/command.rs | 19 +- v3/engine/src/metadata/resolved/error.rs | 10 +- v3/engine/src/metadata/resolved/typecheck.rs | 241 +++++++++++++++++++ 4 files changed, 269 insertions(+), 2 deletions(-) create mode 100644 v3/engine/src/metadata/resolved/typecheck.rs diff --git a/v3/engine/src/metadata/resolved.rs b/v3/engine/src/metadata/resolved.rs index 059c7e31ed8..8a91435396f 100644 --- a/v3/engine/src/metadata/resolved.rs +++ b/v3/engine/src/metadata/resolved.rs @@ -8,4 +8,5 @@ pub mod model; pub mod ndc_validation; pub mod relationship; pub mod subgraph; +mod typecheck; pub mod types; diff --git a/v3/engine/src/metadata/resolved/command.rs b/v3/engine/src/metadata/resolved/command.rs index 380c79b66f2..9324599026e 100644 --- a/v3/engine/src/metadata/resolved/command.rs +++ b/v3/engine/src/metadata/resolved/command.rs @@ -21,6 +21,7 @@ use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; use super::metadata::DataConnectorTypeMappings; +use super::typecheck; use super::types::{ collect_type_mapping_for_source, TypeMappingCollectionError, TypeMappingToCollect, }; @@ -297,9 +298,25 @@ pub fn resolve_command_permissions( }); } - // TODO: typecheck any literal values against the argument types match command.arguments.get(&argument_preset.argument) { Some(argument) => { + // if our value is a literal, typecheck it against expected type + match &argument_preset.value { + ValueExpression::SessionVariable(_) => Ok(()), + ValueExpression::Literal(json_value) => { + typecheck::typecheck_qualified_type_reference( + &argument.argument_type, + json_value, + ) + } + } + .map_err(|type_error| { + Error::CommandArgumentPresetTypeError { + command_name: command.name.clone(), + argument_name: argument_preset.argument.clone(), + type_error, + } + })?; argument_presets.insert( argument_preset.argument.clone(), ( diff --git a/v3/engine/src/metadata/resolved/error.rs b/v3/engine/src/metadata/resolved/error.rs index cd462418362..b18ca4d0426 100644 --- a/v3/engine/src/metadata/resolved/error.rs +++ b/v3/engine/src/metadata/resolved/error.rs @@ -14,7 +14,7 @@ use open_dds::{ types::{CustomTypeName, FieldName, OperatorName, TypeReference}, }; -use super::{ndc_validation::NDCValidationError, types::TypeMappingCollectionError}; +use super::{ndc_validation::NDCValidationError, typecheck, types::TypeMappingCollectionError}; // TODO: This enum really needs structuring #[derive(Error, Debug)] @@ -566,6 +566,14 @@ pub enum Error { }, #[error("Predicate types in data connectors are unsupported")] PredicateTypesUnsupported, + #[error( + "Type error in preset argument {argument_name:} for command {command_name:}: {type_error:}" + )] + CommandArgumentPresetTypeError { + command_name: Qualified, + argument_name: ArgumentName, + type_error: typecheck::TypecheckError, + }, // ---------------- Graphql Configuration Errors ---------------- #[error("graphql configuration is not defined in supergraph")] MissingGraphqlConfig, diff --git a/v3/engine/src/metadata/resolved/typecheck.rs b/v3/engine/src/metadata/resolved/typecheck.rs new file mode 100644 index 00000000000..661462390cc --- /dev/null +++ b/v3/engine/src/metadata/resolved/typecheck.rs @@ -0,0 +1,241 @@ +//! Functions for typechecking JSON literals against expected types + +use crate::metadata::resolved::subgraph; +use thiserror::Error; + +#[derive(Error, Debug, PartialEq)] +/// Errors that can occur when typechecking a value +pub enum TypecheckError { + #[error("Expected a value of type {expected:} but got value {actual:}")] + ScalarTypeMismatch { + expected: open_dds::types::InbuiltType, + actual: serde_json::Value, + }, + #[error("Error in array item: {inner_error:}")] + ArrayItemMismatch { inner_error: Box }, + #[error("Expected an array but instead got value {value:}")] + NonArrayValue { value: serde_json::Value }, + #[error("Expected a non-null value but received null")] + NullInNonNullableColumn, +} + +/// check whether a serde_json::Value matches our expected type +/// currently only works for primitive types (Int, String, etc) +/// and arrays of those types +pub fn typecheck_qualified_type_reference( + ty: &subgraph::QualifiedTypeReference, + value: &serde_json::Value, +) -> Result<(), TypecheckError> { + match (&ty.underlying_type, value) { + // check null values are allowed + (_, serde_json::Value::Null) => { + if ty.nullable { + Ok(()) + } else { + Err(TypecheckError::NullInNonNullableColumn) + } + } + // check basic inbuilt types + (subgraph::QualifiedBaseType::Named(subgraph::QualifiedTypeName::Inbuilt(inbuilt)), _) => { + typecheck_inbuilt_type(inbuilt, value) + } + // check each item in an array + (subgraph::QualifiedBaseType::List(inner_type), serde_json::Value::Array(array_values)) => { + for array_value in array_values { + match typecheck_qualified_type_reference(inner_type, array_value) { + Ok(_) => {} + Err(inner_error) => { + return Err(TypecheckError::ArrayItemMismatch { + inner_error: Box::new(inner_error), + }) + } + } + } + Ok(()) + } + // array expected, non-array value + (subgraph::QualifiedBaseType::List(_), value) => Err(TypecheckError::NonArrayValue { + value: value.clone(), + }), + + // we don't current check custom named types, so we just assume the values are OK + (subgraph::QualifiedBaseType::Named(subgraph::QualifiedTypeName::Custom(_)), _) => Ok(()), + } +} + +/// check a JSON value matches an expected inbuilt primitive type +fn typecheck_inbuilt_type( + inbuilt: &open_dds::types::InbuiltType, + value: &serde_json::Value, +) -> Result<(), TypecheckError> { + match (inbuilt, value) { + (open_dds::types::InbuiltType::Int, serde_json::Value::Number(_)) => Ok(()), + (open_dds::types::InbuiltType::Float, serde_json::Value::Number(_)) => Ok(()), + (open_dds::types::InbuiltType::String, serde_json::Value::String(_)) => Ok(()), + (open_dds::types::InbuiltType::Boolean, serde_json::Value::Bool(_)) => Ok(()), + + _ => Err(TypecheckError::ScalarTypeMismatch { + expected: inbuilt.clone(), + actual: value.clone(), + }), + } +} + +#[cfg(test)] +mod tests { + use super::{subgraph, typecheck_qualified_type_reference, TypecheckError}; + use serde_json::json; + + fn int_type(nullable: bool) -> subgraph::QualifiedTypeReference { + subgraph::QualifiedTypeReference { + nullable, + underlying_type: subgraph::QualifiedBaseType::Named( + subgraph::QualifiedTypeName::Inbuilt(open_dds::types::InbuiltType::Int), + ), + } + } + + fn string_type() -> subgraph::QualifiedTypeReference { + subgraph::QualifiedTypeReference { + nullable: false, + underlying_type: subgraph::QualifiedBaseType::Named( + subgraph::QualifiedTypeName::Inbuilt(open_dds::types::InbuiltType::String), + ), + } + } + + fn boolean_type() -> subgraph::QualifiedTypeReference { + subgraph::QualifiedTypeReference { + nullable: false, + underlying_type: subgraph::QualifiedBaseType::Named( + subgraph::QualifiedTypeName::Inbuilt(open_dds::types::InbuiltType::Boolean), + ), + } + } + + fn float_type() -> subgraph::QualifiedTypeReference { + subgraph::QualifiedTypeReference { + nullable: false, + underlying_type: subgraph::QualifiedBaseType::Named( + subgraph::QualifiedTypeName::Inbuilt(open_dds::types::InbuiltType::Float), + ), + } + } + + fn array_of(item: subgraph::QualifiedTypeReference) -> subgraph::QualifiedTypeReference { + subgraph::QualifiedTypeReference { + nullable: false, + underlying_type: subgraph::QualifiedBaseType::List(Box::new(item)), + } + } + + #[test] + fn test_typecheck_int() { + let ty = int_type(false); + // `Int` accepts any JSON number + let value = json!(1); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_float() { + let ty = float_type(); + // `Float` accepts any JSON number + let value = json!(123.123); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_string() { + let ty = string_type(); + let value = json!("dog"); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_boolean() { + let ty = boolean_type(); + let value = json!(true); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_array_of_boolean() { + let ty = array_of(boolean_type()); + let value = json!([true, false]); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_nested_array_of_boolean() { + let ty = array_of(array_of(boolean_type())); + let value = json!([[true, false]]); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_does_not_accept_mixture_of_int_and_boolean_in_array_of_boolean() { + let ty = array_of(boolean_type()); + let value = json!([true, 123]); + + assert_eq!( + typecheck_qualified_type_reference(&ty, &value), + Err(TypecheckError::ArrayItemMismatch { + inner_error: Box::new(TypecheckError::ScalarTypeMismatch { + expected: open_dds::types::InbuiltType::Boolean, + actual: serde_json::Value::Number(123.into()) + }) + }) + ) + } + + #[test] + fn test_typecheck_does_not_accept_single_boolean_in_array_of_boolean() { + let ty = array_of(boolean_type()); + let value = json!(true); + + assert_eq!( + typecheck_qualified_type_reference(&ty, &value), + Err(TypecheckError::NonArrayValue { value }) + ); + } + + #[test] + fn test_typecheck_int_does_not_accept_null_value() { + let ty = int_type(false); + let value = json!(null); + + assert_eq!( + typecheck_qualified_type_reference(&ty, &value), + Err(TypecheckError::NullInNonNullableColumn) + ) + } + + #[test] + fn test_typecheck_nullable_int_accepts_null_value() { + let ty = int_type(true); + let value = json!(null); + + assert_eq!(typecheck_qualified_type_reference(&ty, &value), Ok(())) + } + + #[test] + fn test_typecheck_int_does_not_accept_string_value() { + let ty = int_type(false); + let value = json!("dog"); + + assert_eq!( + typecheck_qualified_type_reference(&ty, &value), + Err(TypecheckError::ScalarTypeMismatch { + expected: open_dds::types::InbuiltType::Int, + actual: value.clone() + }) + ) + } +}