fix(jit): error handling for lists in JIT (#2771)

src/core/jit/synth/synth.rs

@@ -36,29 +36,33 @@ where
     #[inline(always)]
     pub fn synthesize(&'a self) -> Result<Value, Positioned<Error>> {
         let mut data = Value::JsonObject::new();
+        let mut path = Vec::new();
 
         for child in self.plan.as_nested().iter() {
             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())?;
-
+            let val = self.iter(child, None, &DataPath::new(), &mut path)?;
             data.insert_key(&child.output_name, val);
         }
 
         Ok(Value::object(data))
     }
 
+    #[allow(clippy::too_many_arguments)]
     #[inline(always)]
     fn iter(
         &'a self,
         node: &'a Field<Nested<Value>, Value>,
         value: Option<&'a Value>,
         data_path: &DataPath,
+        path: &mut Vec<PathSegment>,
     ) -> Result<Value, Positioned<Error>> {
-        match self.store.get(&node.id) {
+        path.push(PathSegment::Field(node.output_name.clone()));
+
+        let result = match self.store.get(&node.id) {
             Some(val) => {
                 let mut data = val;
 
@@ -76,9 +80,10 @@ where
                         let value = result.as_ref().map_err(Clone::clone)?;
 
                         if node.type_of.is_list() != value.as_array().is_some() {
-                            return self.node_nullable_guard(node);
+                            self.node_nullable_guard(node, path)
+                        } else {
+                            self.iter_inner(node, value, data_path, path)
                         }
-                        self.iter_inner(node, value, data_path)
                     }
                     _ => {
                         // TODO: should bailout instead of returning Null
@@ -87,32 +92,39 @@ where
                 }
             }
             None => match value {
-                Some(result) => self.iter_inner(node, result, data_path),
-                None => self.node_nullable_guard(node),
+                Some(result) => self.iter_inner(node, result, data_path, path),
+                None => self.node_nullable_guard(node, path),
             },
-        }
+        };
+
+        path.pop();
+        result
     }
 
     /// 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>,
+        path: &[PathSegment],
     ) -> 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))
+            Err(ValidationError::ValueRequired.into())
+                .map_err(|e| self.to_location_error(e, node, path))
         }
     }
 
+    #[allow(clippy::too_many_arguments)]
     #[inline(always)]
     fn iter_inner(
         &'a self,
         node: &'a Field<Nested<Value>, Value>,
         value: &'a Value,
         data_path: &DataPath,
+        path: &mut Vec<PathSegment>,
     ) -> Result<Value, Positioned<Error>> {
         // skip the field if field is not included in schema
         if !self.include(node) {
@@ -166,13 +178,12 @@ where
                     for child in self.plan.field_iter_only(node, value) {
                         // all checks for skip must occur in `iter_inner`
                         // and include be checked before calling `iter` or recursing.
-                        let include = self.include(child);
-                        if include {
+                        if self.include(child) {
                             let value = if child.name == "__typename" {
                                 Value::string(node.value_type(value).into())
                             } else {
                                 let val = obj.get_key(child.name.as_str());
-                                self.iter(child, val, data_path)?
+                                self.iter(child, val, data_path, path)?
                             };
                             ans.insert_key(&child.output_name, value);
                         }
@@ -183,41 +194,28 @@ where
                 (Some(arr), _) => {
                     let mut ans = vec![];
                     for (i, val) in arr.iter().enumerate() {
-                        let val = self.iter_inner(node, val, &data_path.clone().with_index(i))?;
-                        ans.push(val)
+                        path.push(PathSegment::Index(i));
+                        let val =
+                            self.iter_inner(node, val, &data_path.clone().with_index(i), path)?;
+                        path.pop();
+                        ans.push(val);
                     }
                     Ok(Value::array(ans))
                 }
                 _ => Ok(value.clone()),
             }
         };
 
-        eval_result.map_err(|e| self.to_location_error(e, node))
+        eval_result.map_err(|e| self.to_location_error(e, node, path))
     }
 
     fn to_location_error(
         &'a self,
         error: Error,
         node: &'a Field<Nested<Value>, Value>,
+        path: &[PathSegment],
     ) -> Positioned<Error> {
-        // create path from the root to the current node in the fields tree
-        let path = {
-            let mut path = Vec::new();
-
-            let mut parent = self.plan.find_field(node.id.clone());
-
-            while let Some(field) = parent {
-                path.push(PathSegment::Field(field.output_name.to_string()));
-                parent = field
-                    .parent()
-                    .and_then(|id| self.plan.find_field(id.clone()));
-            }
-
-            path.reverse();
-            path
-        };
-
-        Positioned::new(error, node.pos).with_path(path)
+        Positioned::new(error, node.pos).with_path(path.to_vec())
     }
 }
 

tests/core/snapshots/test-required-fields.md_13.snap

@@ -15,13 +15,12 @@ expression: response
         "locations": [
           {
             "line": 1,
-            "column": 29
+            "column": 9
           }
         ],
         "path": [
           "innerEntryMissing",
-          1,
-          "id"
+          1
         ]
       }
     ]

tests/core/snapshots/test-required-fields.md_17.snap

@@ -8,22 +8,14 @@ expression: response
     "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"
-        ]
-      }
-    ]
+    "data": {
+      "outerEntryMissing": [
+        {
+          "id": 1,
+          "bar": "bar_1"
+        },
+        null
+      ]
+    }
   }
 }

tests/core/snapshots/test-required-fields.md_21.snap

@@ -8,22 +8,14 @@ expression: response
     "content-type": "application/json"
   },
   "body": {
-    "data": null,
-    "errors": [
-      {
-        "message": "internal: non-null types require a return value",
-        "locations": [
-          {
-            "line": 1,
-            "column": 28
-          }
-        ],
-        "path": [
-          "noneEntryMissing",
-          1,
-          "id"
-        ]
-      }
-    ]
+    "data": {
+      "noneEntryMissing": [
+        {
+          "id": 1,
+          "bar": "bar_1"
+        },
+        null
+      ]
+    }
   }
 }

tests/core/snapshots/test-required-fields.md_9.snap

@@ -15,13 +15,12 @@ expression: response
         "locations": [
           {
             "line": 1,
-            "column": 28
+            "column": 9
           }
         ],
         "path": [
           "fullEntryMissing",
-          1,
-          "id"
+          1
         ]
       }
     ]

tests/core/snapshots/test-required-fields.md_merged.snap

@@ -2,7 +2,7 @@
 source: tests/core/spec.rs
 expression: formatter
 ---
-schema @server @upstream(baseURL: "http://jsonplaceholder.typicode.com") {
+schema @server(enableJIT: true) @upstream(baseURL: "http://jsonplaceholder.typicode.com") {
   query: Query
 }
 

tests/execution/test-required-fields.md

@@ -1,7 +1,7 @@
 # Test API
 
 ```graphql @config
-schema @server @upstream(baseURL: "http://jsonplaceholder.typicode.com") {
+schema @server(enableJIT: true) @upstream(baseURL: "http://jsonplaceholder.typicode.com") {
   query: Query
 }