Merge branch 'main' into paritosh/engine-1551

Author: paritosh-08

changelog.md

@@ -8,6 +8,10 @@
 
 ### Fixed
 
+- Native operations will now interpret missing arguments as null values for that argument, instead of causing an error.
+- Pre and post-check arguments in v2 mutations can now either be missing or null and both will be interpreted as an always true predicate. Previously a null value would have caused an error.
+- In v2 update mutations update columns explicitly set to null (as opposed to being missing or being set with their `_set` value object) are now correctly interpreted as "no update should be made to that column", instead of causing an error.
+
 ## [v2.0.0] - 2025-01-09
 
 ### Changed

crates/query-engine/translation/src/translation/mutation/v2/common.rs

@@ -1,10 +1,11 @@
 //! Some common helper functions.
 
+use crate::translation::error::Error;
 use crate::translation::error::Warning;
 use ndc_models as models;
 use nonempty::NonEmpty;
 use query_engine_metadata::metadata;
-use std::collections::BTreeSet;
+use std::collections::{BTreeMap, BTreeSet};
 
 /// Create a description string for keys. For example:
 /// > "'TrackId' key", or
@@ -103,9 +104,28 @@ pub struct CheckArgument {
     pub description: String,
 }
 
-// Default check argument/constraint
-pub fn default_constraint() -> serde_json::Value {
-    serde_json::json!({"type": "and", "expressions": []})
+/// Gets a nullable predicate argument value from an arguments map.
+/// If the argument is missing or null, it is defaulted to an always true predicate.
+pub fn get_nullable_predicate_argument(
+    argument_name: &models::ArgumentName,
+    arguments: &BTreeMap<models::ArgumentName, serde_json::Value>,
+) -> Result<models::Expression, Error> {
+    Ok(arguments
+        .get(argument_name)
+        .map(|pre_predicate_json| {
+            serde_json::from_value::<Option<models::Expression>>(pre_predicate_json.clone())
+                .map_err(|_| {
+                    Error::UnexpectedStructure(format!(
+                        "Argument '{}' should have an ndc-spec Expression structure",
+                        argument_name.clone()
+                    ))
+                })
+        })
+        .transpose()?
+        .flatten()
+        .unwrap_or_else(|| models::Expression::And {
+            expressions: vec![],
+        })) // Always true predicate
 }
 
 // the old default was to prefix generated mutations with `v2_` or `v1_`

crates/query-engine/translation/src/translation/mutation/v2/delete.rs

@@ -11,7 +11,7 @@ use query_engine_metadata::metadata::database;
 use query_engine_sql::sql;
 use std::collections::BTreeMap;
 
-use super::common::{self, default_constraint, CheckArgument};
+use super::common::{self, get_nullable_predicate_argument, CheckArgument};
 
 /// A representation of an auto-generated delete mutation.
 ///
@@ -140,18 +140,8 @@ pub fn translate(
                 .collect::<Result<Vec<sql::ast::Expression>, Error>>()?;
 
             // Build the `pre_check` argument boolean expression.
-            let default_constraint = default_constraint();
-            let predicate_json = arguments
-                .get(&mutation.pre_check.argument_name)
-                .unwrap_or(&default_constraint);
-
-            let predicate: models::Expression = serde_json::from_value(predicate_json.clone())
-                .map_err(|_| {
-                    Error::UnexpectedStructure(format!(
-                        "Argument '{}' should have an ndc-spec Expression structure",
-                        mutation.pre_check.argument_name.clone()
-                    ))
-                })?;
+            let predicate =
+                get_nullable_predicate_argument(&mutation.pre_check.argument_name, arguments)?;
 
             let predicate_expression = filtering::translate(
                 env,

crates/query-engine/translation/src/translation/mutation/v2/insert.rs

@@ -11,7 +11,7 @@ use query_engine_metadata::metadata::database;
 use query_engine_sql::sql;
 use std::collections::{BTreeMap, BTreeSet};
 
-use super::common::{self, default_constraint, CheckArgument};
+use super::common::{self, get_nullable_predicate_argument, CheckArgument};
 
 /// A representation of an auto-generated insert mutation.
 ///
@@ -214,9 +214,7 @@ pub fn translate(
 ) -> Result<(sql::ast::Insert, sql::ast::ColumnAlias), Error> {
     let object = arguments
         .get(&mutation.objects_argument_name)
-        .ok_or(Error::ArgumentNotFound(
-            mutation.objects_argument_name.clone(),
-        ))?;
+        .ok_or_else(|| Error::ArgumentNotFound(mutation.objects_argument_name.clone()))?;
 
     let (columns, from) = translate_objects_to_columns_and_values(env, state, mutation, object)?;
 
@@ -229,18 +227,7 @@ pub fn translate(
     };
 
     // Build the `post_check` argument boolean expression.
-    let default_constraint = default_constraint();
-    let predicate_json = arguments
-        .get(&mutation.post_check.argument_name)
-        .unwrap_or(&default_constraint);
-
-    let predicate: models::Expression =
-        serde_json::from_value(predicate_json.clone()).map_err(|_| {
-            Error::UnexpectedStructure(format!(
-                "Argument '{}' should have an ndc-spec Expression structure",
-                mutation.post_check.argument_name.clone()
-            ))
-        })?;
+    let predicate = get_nullable_predicate_argument(&mutation.post_check.argument_name, arguments)?;
 
     let predicate_expression = filtering::translate(
         env,

crates/query-engine/translation/src/translation/mutation/v2/update.rs

@@ -12,7 +12,7 @@ use query_engine_metadata::metadata::database;
 use query_engine_sql::sql;
 use std::collections::BTreeMap;
 
-use super::common::{self, default_constraint, CheckArgument};
+use super::common::{self, get_nullable_predicate_argument, CheckArgument};
 
 /// A representation of an auto-generated update mutation.
 ///
@@ -109,9 +109,9 @@ pub fn translate(
         UpdateMutation::UpdateByKey(mutation) => {
             let object = arguments
                 .get(&mutation.update_columns_argument_name)
-                .ok_or(Error::ArgumentNotFound(
-                    mutation.update_columns_argument_name.clone(),
-                ))?;
+                .ok_or_else(|| {
+                    Error::ArgumentNotFound(mutation.update_columns_argument_name.clone())
+                })?;
 
             let set = parse_update_columns(env, state, mutation, object)?;
 
@@ -156,37 +156,16 @@ pub fn translate(
                 current_table: table_name_and_reference,
             };
 
-            // Set default constrainst
-            let default_constraint = default_constraint();
-
             // Build the `pre_constraint` argument boolean expression.
-            let pre_predicate_json = arguments
-                .get(&mutation.pre_check.argument_name)
-                .unwrap_or(&default_constraint);
-
-            let pre_predicate: models::Expression =
-                serde_json::from_value(pre_predicate_json.clone()).map_err(|_| {
-                    Error::UnexpectedStructure(format!(
-                        "Argument '{}' should have an ndc-spec Expression structure",
-                        mutation.pre_check.argument_name.clone()
-                    ))
-                })?;
+            let pre_predicate =
+                get_nullable_predicate_argument(&mutation.pre_check.argument_name, arguments)?;
 
             let pre_predicate_expression =
                 filtering::translate(env, state, &root_and_current_tables, &pre_predicate)?;
 
             // Build the `post_constraint` argument boolean expression.
-            let post_predicate_json = arguments
-                .get(&mutation.post_check.argument_name)
-                .unwrap_or(&default_constraint);
-
-            let post_predicate: models::Expression =
-                serde_json::from_value(post_predicate_json.clone()).map_err(|_| {
-                    Error::UnexpectedStructure(format!(
-                        "Argument '{}' should have an ndc-spec Expression structure",
-                        mutation.post_check.argument_name.clone()
-                    ))
-                })?;
+            let post_predicate =
+                get_nullable_predicate_argument(&mutation.post_check.argument_name, arguments)?;
 
             let post_predicate_expression =
                 filtering::translate(env, state, &root_and_current_tables, &post_predicate)?;
@@ -234,17 +213,18 @@ fn parse_update_columns(
             // For each field, look up the column name in the table
             // and update it and the value into the map.
             for (name, value) in object {
-                let column_info = mutation.table_columns.get(name.as_str()).ok_or(
+                let column_info = mutation.table_columns.get(name.as_str()).ok_or_else(|| {
                     Error::ColumnNotFoundInCollection(
                         name.clone().into(),
                         mutation.collection_name.clone(),
-                    ),
-                )?;
+                    )
+                })?;
 
-                columns_to_values.insert(
-                    sql::ast::ColumnName(column_info.name.clone()),
-                    parse_update_column(env, state, &name.as_str().into(), column_info, value)?,
-                );
+                if let Some(value) =
+                    parse_update_column(env, state, &name.as_str().into(), column_info, value)?
+                {
+                    columns_to_values.insert(sql::ast::ColumnName(column_info.name.clone()), value);
+                }
             }
             Ok(())
         }
@@ -275,7 +255,7 @@ fn parse_update_column(
     column_name: &models::FieldName,
     column_info: &metadata::database::ColumnInfo,
     object: &serde_json::Value,
-) -> Result<sql::ast::MutationValueExpression, Error> {
+) -> Result<Option<sql::ast::MutationValueExpression>, Error> {
     match object {
         serde_json::Value::Object(object) => {
             let vec = object.into_iter().collect::<Vec<_>>();
@@ -289,9 +269,9 @@ fn parse_update_column(
                     }
                     // _set operation.
                     if *operation == "_set" {
-                        Ok(sql::ast::MutationValueExpression::Expression(
+                        Ok(Some(sql::ast::MutationValueExpression::Expression(
                             values::translate(env, state, value, &column_info.r#type)?,
-                        ))
+                        )))
                     }
                     // Operation is not supported.
                     else {
@@ -304,6 +284,7 @@ fn parse_update_column(
                 }
             }
         }
+        serde_json::Value::Null => Ok(None),
         // Unexpected structures.
         serde_json::Value::Array(_) => Err(Error::UnexpectedStructure(format!(
             "array structure in update column '{column_name}' argument. Expecting an object.",

crates/query-engine/translation/src/translation/query/native_queries.rs

@@ -1,5 +1,7 @@
 //! Handle native queries translation after building the query.
 
+use std::borrow::Cow;
+
 use ndc_models as models;
 use ref_cast::RefCast;
 
@@ -43,33 +45,49 @@ pub fn translate(
             .map(|part| match part {
                 metadata::NativeQueryPart::Text(text) => Ok(sql::ast::RawSql::RawText(text)),
                 metadata::NativeQueryPart::Parameter(param) => {
-                    let typ = match native_query
+                    let (typ, nullable) = match native_query
                         .info
                         .arguments
                         .get(models::ArgumentName::ref_cast(&param))
                     {
                         None => Err(Error::ArgumentNotFound(param.to_string().into())),
-                        Some(argument) => Ok(argument.r#type.clone()),
+                        Some(argument) => Ok((&argument.r#type, &argument.nullable)),
                     }?;
-                    let exp = match native_query
+                    let argument = native_query
                         .arguments
                         .get(models::ArgumentName::ref_cast(&param))
-                    {
-                        None => Err(Error::ArgumentNotFound(param.to_string().into())),
-                        Some(argument) => match argument {
-                            models::Argument::Literal { value } => {
-                                values::translate(env, &mut translation_state, value, &typ)
-                            }
-                            models::Argument::Variable { name } => match &variables_table {
-                                Err(err) => Err(err.clone()),
-                                Ok(variables_table) => variables::translate(
-                                    env,
-                                    &mut translation_state,
-                                    variables_table.clone(),
-                                    name,
-                                    &typ,
-                                ),
+                        .map_or_else(
+                            || {
+                                // If the argument is missing ...
+                                match nullable {
+                                    // ... and the type is nullable, we treat this like a null value has been passed explicitly ...
+                                    metadata::Nullable::Nullable => {
+                                        Ok(Cow::Owned(models::Argument::Literal {
+                                            value: serde_json::Value::Null,
+                                        }))
+                                    }
+                                    // ... but if the type is non-nullable, we should have received a value and this is error
+                                    metadata::Nullable::NonNullable => {
+                                        Err(Error::ArgumentNotFound(param.to_string().into()))
+                                    }
+                                }
                             },
+                            |arg| Ok(Cow::Borrowed(arg)),
+                        )?;
+
+                    let exp = match argument.as_ref() {
+                        models::Argument::Literal { value } => {
+                            values::translate(env, &mut translation_state, value, typ)
+                        }
+                        models::Argument::Variable { name } => match &variables_table {
+                            Err(err) => Err(err.clone()),
+                            Ok(variables_table) => variables::translate(
+                                env,
+                                &mut translation_state,
+                                variables_table.clone(),
+                                name,
+                                typ,
+                            ),
                         },
                     }?;
                     Ok(sql::ast::RawSql::Expression(exp))

crates/query-engine/translation/src/translation/query/sorting.rs

@@ -97,8 +97,8 @@ struct Column(models::FieldName);
 /// An aggregate operation to select from a table used in an order by.
 #[derive(Debug)]
 enum Aggregate {
-    CountStarAggregate,
-    SingleColumnAggregate {
+    CountStar,
+    SingleColumn {
         column: models::FieldName,
         function: models::AggregateFunctionName,
     },
@@ -167,12 +167,7 @@ fn group_elements(elements: &[models::OrderByElement]) -> Vec<OrderByElementGrou
             ),
             models::OrderByTarget::StarCountAggregate { path } => aggregate_element_groups.insert(
                 hash_path(path),
-                (
-                    i,
-                    path,
-                    element.order_direction,
-                    Aggregate::CountStarAggregate,
-                ),
+                (i, path, element.order_direction, Aggregate::CountStar),
             ),
             models::OrderByTarget::SingleColumnAggregate {
                 path,
@@ -185,7 +180,7 @@ fn group_elements(elements: &[models::OrderByElement]) -> Vec<OrderByElementGrou
                     i,
                     path,
                     element.order_direction,
-                    Aggregate::SingleColumnAggregate {
+                    Aggregate::SingleColumn {
                         column: column.clone(),
                         function: function.clone(),
                     },
@@ -694,7 +689,7 @@ fn translate_targets(
                 .iter()
                 .map(|element| {
                     match &element.element {
-                        Aggregate::CountStarAggregate => {
+                        Aggregate::CountStar => {
                             let column_alias = sql::helpers::make_column_alias("count".to_string());
                             Ok(OrderBySelectExpression {
                                 index: element.index,
@@ -706,7 +701,7 @@ fn translate_targets(
                                 aggregate: Some(sql::ast::Function::Unknown("COUNT".to_string())),
                             })
                         }
-                        Aggregate::SingleColumnAggregate { column, function } => {
+                        Aggregate::SingleColumn { column, function } => {
                             let selected_column = target_collection.lookup_column(column)?;
                             // we are going to deliberately use the table column name and not an alias we get from
                             // the query request because this is internal to the sorting mechanism.

crates/query-engine/translation/tests/goldenfiles/mutations/v2_insert/request.json

@@ -12,6 +12,10 @@
           },
           {
             "name": "The Other Band"
+          },
+          {
+            "name": "The Null Band",
+            "id": null
           }
         ],
         "post_check": {