Nested fields in optional fields should not be required (#4719)

core/src/doc/field.rs

@@ -6,7 +6,7 @@ use crate::err::Error;
 use crate::iam::Action;
 use crate::sql::permission::Permission;
 use crate::sql::value::Value;
-use crate::sql::Kind;
+use crate::sql::{Idiom, Kind};
 use reblessive::tree::Stk;
 use std::sync::Arc;
 
@@ -26,8 +26,23 @@ impl Document {
 		let rid = self.id.as_ref().unwrap();
 		// Get the user applied input
 		let inp = self.initial.doc.as_ref().changed(self.current.doc.as_ref());
+		// If set, the loop will skip certain clauses as long
+		// as the field name starts with the set idiom
+		let mut skip: Option<Idiom> = None;
 		// Loop through all field statements
 		for fd in self.fd(ctx, opt).await?.iter() {
+			let skipped = match skip {
+				Some(ref inner) => {
+					let skipped = fd.name.starts_with(inner);
+					if !skipped {
+						skip = None;
+					}
+
+					skipped
+				}
+				None => false,
+			};
+
 			// Loop over each field in document
 			for (k, mut val) in self.current.doc.as_ref().walk(&fd.name).into_iter() {
 				// Get the initial value
@@ -41,105 +56,110 @@ impl Document {
 						thing: rid.to_string(),
 					});
 				}
-				// Get the default value
+				if !skipped {
-				let def = match &fd.default {
+					// Get the default value
-					Some(v) => Some(v),
+					let def = match &fd.default {
-					_ => match &fd.value {
+						Some(v) => Some(v),
-						Some(v) if v.is_static() => Some(v),
+						_ => match &fd.value {
-						_ => None,
+							Some(v) if v.is_static() => Some(v),
-					},
+							_ => None,
-				};
-				// Check for a DEFAULT clause
-				if let Some(expr) = def {
-					if self.is_new() && val.is_none() {
-						// Configure the context
-						let mut ctx = MutableContext::new(ctx);
-						let v = Arc::new(val);
-						ctx.add_value("input", inp.clone());
-						ctx.add_value("value", v.clone());
-						ctx.add_value("after", v);
-						ctx.add_value("before", old.clone());
-						let ctx = ctx.freeze();
-						// Process the VALUE clause
-						val = expr.compute(stk, &ctx, opt, Some(&self.current)).await?;
-					}
-				}
-				// Check for a TYPE clause
-				if let Some(kind) = &fd.kind {
-					val = val.coerce_to(kind).map_err(|e| match e {
-						// There was a conversion error
-						Error::CoerceTo {
-							from,
-							..
-						} => Error::FieldCheck {
-							thing: rid.to_string(),
-							field: fd.name.clone(),
-							value: from.to_string(),
-							check: kind.to_string(),
 						},
-						// There was a different error
+					};
-						e => e,
+					// Check for a DEFAULT clause
-					})?;
+					if let Some(expr) = def {
-				}
+						if self.is_new() && val.is_none() {
-				// Check for a VALUE clause
-				if let Some(expr) = &fd.value {
-					// Only run value clause for mutable and new fields
-					if !fd.readonly || self.is_new() {
-						// Configure the context
-						let v = Arc::new(val);
-						let mut ctx = MutableContext::new(ctx);
-						ctx.add_value("input", inp.clone());
-						ctx.add_value("value", v.clone());
-						ctx.add_value("after", v);
-						ctx.add_value("before", old.clone());
-						let ctx = ctx.freeze();
-						// Process the VALUE clause
-						val = expr.compute(stk, &ctx, opt, Some(&self.current)).await?;
-					}
-				}
-				// Check for a TYPE clause
-				if let Some(kind) = &fd.kind {
-					val = val.coerce_to(kind).map_err(|e| match e {
-						// There was a conversion error
-						Error::CoerceTo {
-							from,
-							..
-						} => Error::FieldCheck {
-							thing: rid.to_string(),
-							field: fd.name.clone(),
-							value: from.to_string(),
-							check: kind.to_string(),
-						},
-						// There was a different error
-						e => e,
-					})?;
-				}
-				// Check for a ASSERT clause
-				if let Some(expr) = &fd.assert {
-					match (&val, &fd.kind) {
-						// The field TYPE is optional, and the field
-						// value was not set or a NONE value was
-						// specified, so let's ignore the ASSERT clause
-						(Value::None, Some(Kind::Option(_))) => (),
-						// Otherwise let's process the ASSERT clause
-						_ => {
 							// Configure the context
 							let mut ctx = MutableContext::new(ctx);
-							let v = Arc::new(val.clone());
+							let v = Arc::new(val);
 							ctx.add_value("input", inp.clone());
 							ctx.add_value("value", v.clone());
 							ctx.add_value("after", v);
 							ctx.add_value("before", old.clone());
 							let ctx = ctx.freeze();
-							// Process the ASSERT clause
+							// Process the VALUE clause
-							if !expr.compute(stk, &ctx, opt, Some(&self.current)).await?.is_truthy()
+							val = expr.compute(stk, &ctx, opt, Some(&self.current)).await?;
-							{
+						}
-								return Err(Error::FieldValue {
+					}
-									thing: rid.to_string(),
+					// Check for a TYPE clause
-									field: fd.name.clone(),
+					if let Some(kind) = &fd.kind {
-									value: val.to_string(),
+						val = val.coerce_to(kind).map_err(|e| match e {
-									check: expr.to_string(),
+							// There was a conversion error
-								});
+							Error::CoerceTo {
+								from,
+								..
+							} => Error::FieldCheck {
+								thing: rid.to_string(),
+								field: fd.name.clone(),
+								value: from.to_string(),
+								check: kind.to_string(),
+							},
+							// There was a different error
+							e => e,
+						})?;
+					}
+					// Check for a VALUE clause
+					if let Some(expr) = &fd.value {
+						// Only run value clause for mutable and new fields
+						if !fd.readonly || self.is_new() {
+							// Configure the context
+							let v = Arc::new(val);
+							let mut ctx = MutableContext::new(ctx);
+							ctx.add_value("input", inp.clone());
+							ctx.add_value("value", v.clone());
+							ctx.add_value("after", v);
+							ctx.add_value("before", old.clone());
+							let ctx = ctx.freeze();
+							// Process the VALUE clause
+							val = expr.compute(stk, &ctx, opt, Some(&self.current)).await?;
+						}
+					}
+					// Check for a TYPE clause
+					if let Some(kind) = &fd.kind {
+						val = val.coerce_to(kind).map_err(|e| match e {
+							// There was a conversion error
+							Error::CoerceTo {
+								from,
+								..
+							} => Error::FieldCheck {
+								thing: rid.to_string(),
+								field: fd.name.clone(),
+								value: from.to_string(),
+								check: kind.to_string(),
+							},
+							// There was a different error
+							e => e,
+						})?;
+					}
+					// Check for a ASSERT clause
+					if let Some(expr) = &fd.assert {
+						match (&val, &fd.kind) {
+							// The field TYPE is optional, and the field
+							// value was not set or a NONE value was
+							// specified, so let's ignore the ASSERT clause
+							(Value::None, Some(Kind::Option(_))) => (),
+							// Otherwise let's process the ASSERT clause
+							_ => {
+								// Configure the context
+								let mut ctx = MutableContext::new(ctx);
+								let v = Arc::new(val.clone());
+								ctx.add_value("input", inp.clone());
+								ctx.add_value("value", v.clone());
+								ctx.add_value("after", v);
+								ctx.add_value("before", old.clone());
+								let ctx = ctx.freeze();
+								// Process the ASSERT clause
+								if !expr
+									.compute(stk, &ctx, opt, Some(&self.current))
+									.await?
+									.is_truthy()
+								{
+									return Err(Error::FieldValue {
+										thing: rid.to_string(),
+										field: fd.name.clone(),
+										value: val.to_string(),
+										check: expr.to_string(),
+									});
+								}
 							}
 						}
 					}
@@ -184,11 +204,18 @@ impl Document {
 						}
 					}
 				}
-				// Set the value of the field
+
-				match val {
+				if !skipped {
-					Value::None => self.current.doc.to_mut().del(stk, ctx, opt, &k).await?,
+					if matches!(val, Value::None) && matches!(fd.kind, Some(Kind::Option(_))) {
-					v => self.current.doc.to_mut().set(stk, ctx, opt, &k, v).await?,
+						skip = Some(fd.name.to_owned());
-				};
+					}
+
+					// Set the value of the field
+					match val {
+						Value::None => self.current.doc.to_mut().del(stk, ctx, opt, &k).await?,
+						v => self.current.doc.to_mut().set(stk, ctx, opt, &k, v).await?,
+					};
+				}
 			}
 		}
 		// Carry on

core/src/sql/idiom.rs

@@ -153,6 +153,10 @@ impl Idiom {
 			self.0.truncate(self.len() - 1);
 		}
 	}
+
+	pub(crate) fn starts_with(&self, other: &[Part]) -> bool {
+		self.0.starts_with(other)
+	}
 }
 
 impl Idiom {

sdk/tests/typing.rs

@@ -281,3 +281,38 @@ async fn strict_typing_none_null() -> Result<(), Error> {
 	//
 	Ok(())
 }
+
+#[tokio::test]
+async fn strict_typing_optional_object() -> Result<(), Error> {
+	let sql = "
+        DEFINE TABLE test SCHEMAFULL;
+        DEFINE FIELD obj ON test TYPE option<object>;
+        DEFINE FIELD obj.a ON test TYPE string;
+
+        CREATE ONLY test:1;
+        CREATE ONLY test:2 SET obj = {};
+        CREATE ONLY test:3 SET obj = { a: 'abc' };
+	";
+	let mut t = Test::new(sql).await?;
+	//
+	t.skip_ok(3)?;
+	//
+	t.expect_val(
+		"{
+            id: test:1,
+        }",
+	)?;
+	//
+	t.expect_error("Found NONE for field `obj.a`, with record `test:2`, but expected a string")?;
+	//
+	t.expect_val(
+		"{
+            id: test:3,
+            obj: {
+                a: 'abc',
+            },
+        }",
+	)?;
+	//
+	Ok(())
+}