From 37b7a94eb7216ce709cd682e4cfbd22fbd8cd4fc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Apr 2026 08:43:48 +0000 Subject: [PATCH] Fix nullable query parameters: accept omission and null for `?` params Parameters declared with `?` (e.g. `$changelogUrl: String?`) now correctly accept omission or explicit null in JSON input instead of requiring empty strings as a workaround. Adds `Literal::Null` variant and threads it through parameter parsing, type-checking, and Arrow array conversion. https://claude.ai/code/session_014oGFKL7EVg1b2cyPgt9Gne --- crates/omnigraph-compiler/src/query/ast.rs | 1 + .../omnigraph-compiler/src/query/typecheck.rs | 14 ++ crates/omnigraph-compiler/src/query_input.rs | 129 ++++++++++++++++-- crates/omnigraph/src/exec/mutation.rs | 1 + crates/omnigraph/src/exec/projection.rs | 2 + crates/omnigraph/src/exec/query.rs | 1 + 6 files changed, 135 insertions(+), 13 deletions(-) diff --git a/crates/omnigraph-compiler/src/query/ast.rs b/crates/omnigraph-compiler/src/query/ast.rs index c5e146b..f0bbdb5 100644 --- a/crates/omnigraph-compiler/src/query/ast.rs +++ b/crates/omnigraph-compiler/src/query/ast.rs @@ -160,6 +160,7 @@ impl std::fmt::Display for AggFunc { #[derive(Debug, Clone)] pub enum Literal { + Null, String(String), Integer(i64), Float(f64), diff --git a/crates/omnigraph-compiler/src/query/typecheck.rs b/crates/omnigraph-compiler/src/query/typecheck.rs index 5364897..5eb71bd 100644 --- a/crates/omnigraph-compiler/src/query/typecheck.rs +++ b/crates/omnigraph-compiler/src/query/typecheck.rs @@ -1436,6 +1436,8 @@ fn resolved_type_to_field_shape( fn literal_type(lit: &Literal) -> Result { match lit { + // Null is compatible with any nullable type; default to String for inference. + Literal::Null => Ok(PropType::scalar(ScalarType::String, true)), Literal::String(_) => Ok(PropType::scalar(ScalarType::String, false)), Literal::Integer(_) => Ok(PropType::scalar(ScalarType::I64, false)), Literal::Float(_) => Ok(PropType::scalar(ScalarType::F64, false)), @@ -1466,6 +1468,18 @@ fn literal_type(lit: &Literal) -> Result { } fn check_literal_type(lit: &Literal, expected: &PropType, prop_name: &str) -> Result<()> { + // Null is compatible with any nullable property type. + if matches!(lit, Literal::Null) { + return if expected.nullable { + Ok(()) + } else { + Err(NanoError::Type(format!( + "T3: property `{}` is non-nullable but got null", + prop_name + ))) + }; + } + if !expected.list && let ScalarType::Vector(expected_dim) = expected.scalar && let Some(actual_dim) = numeric_vector_literal_dim(lit) diff --git a/crates/omnigraph-compiler/src/query_input.rs b/crates/omnigraph-compiler/src/query_input.rs index e2bab52..6afb730 100644 --- a/crates/omnigraph-compiler/src/query_input.rs +++ b/crates/omnigraph-compiler/src/query_input.rs @@ -284,12 +284,31 @@ pub fn json_params_to_param_map( for (key, value) in object { let decl = query_params.iter().find(|param| param.name == *key); - let literal = if let Some(decl) = decl { - json_value_to_literal_typed(key, value, &decl.type_name, mode)? + if let Some(decl) = decl { + if matches!(value, Value::Null) { + if decl.nullable { + map.insert(key.clone(), Literal::Null); + } else { + return Err(RunInputError::message(format!( + "param '{}': null is not accepted for non-nullable parameter", + key + ))); + } + } else { + let literal = json_value_to_literal_typed(key, value, &decl.type_name, mode)?; + map.insert(key.clone(), literal); + } } else { - json_value_to_literal_inferred(key, value, mode)? + let literal = json_value_to_literal_inferred(key, value, mode)?; + map.insert(key.clone(), literal); }; - map.insert(key.clone(), literal); + } + + // Fill in Literal::Null for declared nullable params that were omitted. + for param in query_params { + if param.nullable && !map.contains_key(¶m.name) { + map.insert(param.name.clone(), Literal::Null); + } } Ok(map) @@ -568,15 +587,7 @@ fn json_value_to_literal_inferred( } Ok(Literal::List(out)) } - Value::Null => Err(match mode { - JsonParamMode::Standard => { - RunInputError::message(format!("param '{}': null is not supported", key)) - } - JsonParamMode::JavaScript => RunInputError::message(format!( - "param '{}': null values are not supported as query parameters", - key - )), - }), + Value::Null => Ok(Literal::Null), Value::Object(_) => Err(match mode { JsonParamMode::Standard => { RunInputError::message(format!("param '{}': object is not supported", key)) @@ -889,4 +900,96 @@ query q($tags: [String], $days: [Date]?, $due_at: DateTime) { other => panic!("expected date list param, got {:?}", other), } } + + #[test] + fn nullable_param_omitted_becomes_null() { + let query = find_named_query( + "query q($name: String, $bio: String?) { match { $u: User } return { $u } }", + "q", + ) + .expect("query"); + + let params = json_params_to_param_map( + Some(&json!({ "name": "Alice" })), + &query.params, + JsonParamMode::Standard, + ) + .expect("should accept omitted nullable param"); + + assert!(matches!(params.get("name"), Some(Literal::String(v)) if v == "Alice")); + assert!(matches!(params.get("bio"), Some(Literal::Null))); + } + + #[test] + fn nullable_param_explicit_null_becomes_null() { + let query = find_named_query( + "query q($name: String, $bio: String?) { match { $u: User } return { $u } }", + "q", + ) + .expect("query"); + + let params = json_params_to_param_map( + Some(&json!({ "name": "Alice", "bio": null })), + &query.params, + JsonParamMode::Standard, + ) + .expect("should accept explicit null for nullable param"); + + assert!(matches!(params.get("name"), Some(Literal::String(v)) if v == "Alice")); + assert!(matches!(params.get("bio"), Some(Literal::Null))); + } + + #[test] + fn non_nullable_param_rejects_null() { + let query = find_named_query( + "query q($name: String) { match { $u: User } return { $u } }", + "q", + ) + .expect("query"); + + let error = json_params_to_param_map( + Some(&json!({ "name": null })), + &query.params, + JsonParamMode::Standard, + ) + .expect_err("null for non-nullable param should fail"); + + assert!( + error + .to_string() + .contains("null is not accepted for non-nullable parameter"), + "unexpected error: {}", + error + ); + } + + #[test] + fn nullable_param_with_value_works_normally() { + let query = find_named_query( + "query q($bio: String?) { match { $u: User } return { $u } }", + "q", + ) + .expect("query"); + + let params = json_params_to_param_map( + Some(&json!({ "bio": "hello" })), + &query.params, + JsonParamMode::Standard, + ) + .expect("should accept string value for nullable param"); + + assert!(matches!(params.get("bio"), Some(Literal::String(v)) if v == "hello")); + } + + #[test] + fn inferred_null_param_becomes_literal_null() { + let params = json_params_to_param_map( + Some(&json!({ "extra": null })), + &[], + JsonParamMode::Standard, + ) + .expect("inferred null should succeed"); + + assert!(matches!(params.get("extra"), Some(Literal::Null))); + } } diff --git a/crates/omnigraph/src/exec/mutation.rs b/crates/omnigraph/src/exec/mutation.rs index 3ca1636..b437606 100644 --- a/crates/omnigraph/src/exec/mutation.rs +++ b/crates/omnigraph/src/exec/mutation.rs @@ -26,6 +26,7 @@ fn literal_to_typed_array( num_rows: usize, ) -> Result { Ok(match (lit, data_type) { + (Literal::Null, _) => arrow_array::new_null_array(data_type, num_rows), (Literal::String(s), DataType::Utf8) => { Arc::new(StringArray::from(vec![s.as_str(); num_rows])) as ArrayRef } diff --git a/crates/omnigraph/src/exec/projection.rs b/crates/omnigraph/src/exec/projection.rs index 73d5250..bcfae66 100644 --- a/crates/omnigraph/src/exec/projection.rs +++ b/crates/omnigraph/src/exec/projection.rs @@ -74,6 +74,7 @@ fn evaluate_expr(batch: &RecordBatch, expr: &IRExpr, params: &ParamMap) -> Resul /// Create a constant array from a literal value. fn literal_to_array(lit: &Literal, num_rows: usize) -> Result { Ok(match lit { + Literal::Null => arrow_array::new_null_array(&DataType::Utf8, num_rows), Literal::String(s) => Arc::new(StringArray::from(vec![s.as_str(); num_rows])) as ArrayRef, Literal::Integer(n) => { // Try to match the most common integer types @@ -283,6 +284,7 @@ fn list_scalar_type(items: &[Literal]) -> Result { fn literal_scalar_type(lit: &Literal) -> Result { match lit { + Literal::Null => Ok(ScalarType::String), Literal::String(_) => Ok(ScalarType::String), Literal::Integer(_) => Ok(ScalarType::I64), Literal::Float(_) => Ok(ScalarType::F64), diff --git a/crates/omnigraph/src/exec/query.rs b/crates/omnigraph/src/exec/query.rs index dc95a53..f4547a7 100644 --- a/crates/omnigraph/src/exec/query.rs +++ b/crates/omnigraph/src/exec/query.rs @@ -1225,6 +1225,7 @@ fn ir_expr_to_sql(expr: &IRExpr, params: &ParamMap) -> Option { pub(super) fn literal_to_sql(lit: &Literal) -> String { match lit { + Literal::Null => "NULL".to_string(), Literal::String(s) => format!("'{}'", s.replace('\'', "''")), Literal::Integer(n) => n.to_string(), Literal::Float(f) => f.to_string(),