From d840c39c1940e7dd2acf85d76ba61f68c4396a9a Mon Sep 17 00:00:00 2001
From: Pawan Rawal <pawan0201@gmail.com>
Date: Tue, 3 Nov 2020 11:45:12 +0530
Subject: [PATCH] fix(GraphQL): Fix panic caused when trying to delete a nested
 object which doesn't have id/xid (#6810)

Fixes GRAPHQL-745.

While doing an updateMutation, when the linked object didn't have an id or xid our code was panicking. This change fixes that behavior by returning an error instead.
---
 graphql/resolve/mutation_rewriter.go      |  5 ++
 graphql/resolve/mutation_test.go          |  1 -
 graphql/resolve/schema.graphql            | 11 +++-
 graphql/resolve/update_mutation_test.yaml | 61 ++++++++++++++++-------
 4 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/graphql/resolve/mutation_rewriter.go b/graphql/resolve/mutation_rewriter.go
index e5fff16a7f7..a1a90512b72 100644
--- a/graphql/resolve/mutation_rewriter.go
+++ b/graphql/resolve/mutation_rewriter.go
@@ -1053,6 +1053,11 @@ func rewriteObject(
 			}
 		} else if !withAdditionalDeletes {
 			// In case of delete, id/xid is required
+			if xid == nil && id == nil {
+				err := errors.Errorf("object of type: %s doesn't have a field of type ID! "+
+					"or @id and can't be referenced for deletion", typ.Name())
+				return &mutationRes{secondPass: []*mutationFragment{{err: err}}}
+			}
 			var name string
 			if xid != nil {
 				name = xid.Name()
diff --git a/graphql/resolve/mutation_test.go b/graphql/resolve/mutation_test.go
index 475ac022f27..25aa27c6ef7 100644
--- a/graphql/resolve/mutation_test.go
+++ b/graphql/resolve/mutation_test.go
@@ -206,7 +206,6 @@ func mutationRewriting(t *testing.T, file string, rewriterFactory func() Mutatio
 
 			// -- Act --
 			upsert, err := rewriterToTest.Rewrite(context.Background(), mut)
-
 			// -- Assert --
 			if tcase.Error != nil || err != nil {
 				require.NotNil(t, err)
diff --git a/graphql/resolve/schema.graphql b/graphql/resolve/schema.graphql
index e7cb421ae56..981dff81f43 100644
--- a/graphql/resolve/schema.graphql
+++ b/graphql/resolve/schema.graphql
@@ -351,4 +351,13 @@ type Home {
     members: [HomeMember]
     favouriteMember: HomeMember
 }
-# union testing - end
\ No newline at end of file
+# union testing - end
+
+type Workflow {
+    id: ID!
+    nodes: [Node!]
+}
+
+type Node {
+    name: String!
+}
\ No newline at end of file
diff --git a/graphql/resolve/update_mutation_test.yaml b/graphql/resolve/update_mutation_test.yaml
index cfaea685789..808c23f8006 100644
--- a/graphql/resolve/update_mutation_test.yaml
+++ b/graphql/resolve/update_mutation_test.yaml
@@ -676,7 +676,7 @@
     }
   dgmutations:
     - deletejson: |
-        { 
+        {
                 "Author.posts": [{
                         "uid" : "0x124",
                         "Post.author": {
@@ -738,6 +738,33 @@
       }
     }
 
+-
+  name: "Update remove reference without id or xid"
+  gqlmutation: |
+    mutation updateWorkflow($patch: UpdateWorkflowInput!) {
+      updateWorkflow(input: $patch) {
+        workflow {
+          id
+        }
+      }
+    }
+  gqlvariables: |
+    { "patch":
+      { "filter": {
+          "id": ["0x123"]
+        },
+        "remove": {
+          "nodes": [ { "name": "node" } ]
+        },
+        "set": {
+          "nodes": [ { "name": "node" } ]
+        }
+      }
+    }
+  error:
+    message: |-
+      failed to rewrite mutation payload because object of type: Node doesn't have a field of type ID! or @id and can't be referenced for deletion
+
 -
   name: "Update add and remove together"
   gqlmutation: |
@@ -1376,16 +1403,16 @@
 # Additional Deletes
 #
 # If we have
-# 
+#
 # type Post { ... author: Author @hasInverse(field: posts) ... }
 # type Author { ... posts: [Post] ... }
 #
 # and existing edge
-# 
+#
 # Post1 --- author --> Author1
-# 
+#
 # there must also exist edge
-# 
+#
 # Author1 --- posts --> Post1
 #
 # So if we did an update that changes the author of Post1 to Author2, we need to
@@ -1402,7 +1429,7 @@
 # author we are updating.
 #
 # Updates can only happen at the top level of a mutation, so there's no deep cases.
-# There's four cases to consider: 
+# There's four cases to consider:
 #  * updating a node by adding a reference by ID (e.g. attaching a post to an author
 #    causes a delete on the author the post was attached to - if it's not the post
 #    being updated)
@@ -1422,8 +1449,8 @@
       }
     }
   gqlvariables: |
-    { 
-      "patch": { 
+    {
+      "patch": {
         "filter": {
           "id": ["0x123"]
         },
@@ -1434,7 +1461,7 @@
     }
   dgmutations:
     - setjson: |
-        { 
+        {
           "uid" : "uid(x)",
           "Author.posts": [
             {
@@ -1488,7 +1515,7 @@
     - setjson: |
         { "uid" : "uid(x)",
           "Post.text": "updated text",
-          "Post.author": { 
+          "Post.author": {
             "uid": "0x456",
             "Author.posts": [ { "uid": "uid(x)" } ]
           }
@@ -1524,8 +1551,8 @@
       }
     }
   gqlvariables: |
-    { 
-      "patch": { 
+    {
+      "patch": {
         "filter": {
           "id": ["0x123"]
         },
@@ -1545,7 +1572,7 @@
       cond: "@if(eq(len(State4), 0) AND gt(len(x), 0))"
   dgmutationssec:
     - setjson: |
-        { 
+        {
           "uid" : "uid(x)",
           "Country.states": [
             {
@@ -1618,16 +1645,16 @@
       }
     }
   gqlvariables: |
-    { 
+    {
       "patch":
-      { 
+      {
         "filter": { "name": { "eq": "A.N. Owner" } },
         "set": { "computers": { "name": "Comp" } }
       }
     }
   dgmutations:
     - setjson: |
-        { 
+        {
           "uid": "_:Computer4",
           "dgraph.type": ["Computer"],
           "Computer.name": "Comp"
@@ -1636,7 +1663,7 @@
   dgmutationssec:
     - setjson: |
         { "uid" : "uid(x)",
-          "ComputerOwner.computers": { 
+          "ComputerOwner.computers": {
             "uid": "uid(Computer4)",
             "Computer.owners": [ { "uid": "uid(x)" } ]
           }