fix(jit): raise validation error for required fields (#2754)

Co-authored-by: Tushar Mathur <[email protected]>
tailcallhq · Aug 27, 2024 · aeb1043 · aeb1043 · github-actions · Aug 27, 2024
1 parent 7be002f
commit aeb1043
Show file tree

Hide file tree

Showing 27 changed files with 1,042 additions and 11 deletions.
diff --git a/src/core/blueprint/blueprint.rs b/src/core/blueprint/blueprint.rs
@@ -77,6 +77,7 @@ impl Type {
             Type::ListType { non_null, .. } => *non_null,
         }
     }
+
     /// checks if the type is a list
     pub fn is_list(&self) -> bool {
         matches!(self, Type::ListType { .. })

diff --git a/src/core/jit/synth/synth.rs b/src/core/jit/synth/synth.rs
@@ -42,6 +42,8 @@ where
             if !self.include(child) {
                 continue;
             }
+            // TODO: in case of error set `child.output_name` to null
+            // and append error to response error array
             let val = self.iter(child, None, &DataPath::new())?;
 
             data.insert_key(&child.output_name, val);
@@ -50,12 +52,6 @@ where
         Ok(Value::object(data))
     }
 
-    /// checks if type_of is an array and value is an array
-    #[inline(always)]
-    fn is_array(type_of: &crate::core::blueprint::Type, value: &Value) -> bool {
-        type_of.is_list() == value.as_array().is_some()
-    }
-
     #[inline(always)]
     fn iter(
         &'a self,
@@ -80,8 +76,8 @@ where
                     Data::Single(result) => {
                         let value = result.as_ref().map_err(Clone::clone)?;
 
-                        if !Self::is_array(&node.type_of, value) {
-                            return Ok(Value::null());
+                        if node.type_of.is_list() != value.as_array().is_some() {
+                            return self.node_nullable_guard(node);
                         }
                         self.iter_inner(node, value, data_path)
                     }
@@ -92,25 +88,45 @@ where
                 }
             }
             None => match value {
-                Some(value) => self.iter_inner(node, value, data_path),
-                None => Ok(Value::null()),
+                Some(result) => self.iter_inner(node, result, data_path),
+                None => self.node_nullable_guard(node),
             },
         }
     }
 
+    /// This guard ensures to return Null value only if node type permits it, in
+    /// case it does not it throws an Error
+    fn node_nullable_guard(
+        &'a self,
+        node: &'a Field<Nested<Value>, Value>,
+    ) -> Result<Value, Positioned<Error>> {
+        // according to GraphQL spec https://spec.graphql.org/October2021/#sec-Handling-Field-Errors
+        if node.type_of.is_nullable() {
+            Ok(Value::null())
+        } else {
+            Err(ValidationError::ValueRequired.into()).map_err(|e| self.to_location_error(e, node))
+        }
+    }
+
     #[inline(always)]
     fn iter_inner(
         &'a self,
         node: &'a Field<Nested<Value>, Value>,
         value: &'a Value,
         data_path: &DataPath,
     ) -> Result<Value, Positioned<Error>> {
+        // skip the field if field is not included in schema
         if !self.include(node) {
             return Ok(Value::null());
         }
 
         let eval_result = if value.is_null() {
-            if node.type_of.is_nullable() {
+            // check the nullability of this type unwrapping list modifier
+            let is_nullable = match &node.type_of {
+                crate::core::blueprint::Type::NamedType { non_null, .. } => !*non_null,
+                crate::core::blueprint::Type::ListType { of_type, .. } => of_type.is_nullable(),
+            };
+            if is_nullable {
                 Ok(Value::null())
             } else {
                 Err(ValidationError::ValueRequired.into())

diff --git a/tests/core/snapshots/test-required-fields.md_0.snap b/tests/core/snapshots/test-required-fields.md_0.snap
@@ -0,0 +1,18 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "basicPresent": {
+        "id": 1,
+        "bar": "bar_1"
+      }
+    }
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_1.snap b/tests/core/snapshots/test-required-fields.md_1.snap
@@ -0,0 +1,28 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 32
+          }
+        ],
+        "path": [
+          "basicFieldMissing",
+          "bar"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_10.snap b/tests/core/snapshots/test-required-fields.md_10.snap
@@ -0,0 +1,24 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "innerPresent": [
+        {
+          "id": 1,
+          "bar": "bar_1"
+        },
+        {
+          "id": 2,
+          "bar": "bar_2"
+        }
+      ]
+    }
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_11.snap b/tests/core/snapshots/test-required-fields.md_11.snap
@@ -0,0 +1,15 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "innerMissing": null
+    }
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_12.snap b/tests/core/snapshots/test-required-fields.md_12.snap
@@ -0,0 +1,29 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 32
+          }
+        ],
+        "path": [
+          "innerFieldMissing",
+          1,
+          "bar"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_13.snap b/tests/core/snapshots/test-required-fields.md_13.snap
@@ -0,0 +1,29 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 29
+          }
+        ],
+        "path": [
+          "innerEntryMissing",
+          1,
+          "id"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_14.snap b/tests/core/snapshots/test-required-fields.md_14.snap
@@ -0,0 +1,24 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "outerPresent": [
+        {
+          "id": 1,
+          "bar": "bar_1"
+        },
+        {
+          "id": 2,
+          "bar": "bar_2"
+        }
+      ]
+    }
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_15.snap b/tests/core/snapshots/test-required-fields.md_15.snap
@@ -0,0 +1,27 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 9
+          }
+        ],
+        "path": [
+          "outerMissing"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_16.snap b/tests/core/snapshots/test-required-fields.md_16.snap
@@ -0,0 +1,29 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 32
+          }
+        ],
+        "path": [
+          "outerFieldMissing",
+          1,
+          "bar"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_17.snap b/tests/core/snapshots/test-required-fields.md_17.snap
@@ -0,0 +1,29 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": null,
+    "errors": [
+      {
+        "message": "internal: non-null types require a return value",
+        "locations": [
+          {
+            "line": 1,
+            "column": 29
+          }
+        ],
+        "path": [
+          "outerEntryMissing",
+          1,
+          "id"
+        ]
+      }
+    ]
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_18.snap b/tests/core/snapshots/test-required-fields.md_18.snap
@@ -0,0 +1,24 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "nonePresent": [
+        {
+          "id": 1,
+          "bar": "bar_1"
+        },
+        {
+          "id": 2,
+          "bar": "bar_2"
+        }
+      ]
+    }
+  }
+}
diff --git a/tests/core/snapshots/test-required-fields.md_19.snap b/tests/core/snapshots/test-required-fields.md_19.snap
@@ -0,0 +1,15 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "noneMissing": null
+    }
+  }
+}
Thread Stats	Avg	Stdev	Max	+/- Stdev
Latency	6.70ms	3.04ms	98.27ms	73.01%
Req/Sec	3.78k	206.00	4.41k	92.25%