Skip to content

fix(GraphQL): Fix panic caused when trying to delete a nested object which doesn't have id/xid #6810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions graphql/resolve/mutation_rewriter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 0 additions & 1 deletion graphql/resolve/mutation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 10 additions & 1 deletion graphql/resolve/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -351,4 +351,13 @@ type Home {
members: [HomeMember]
favouriteMember: HomeMember
}
# union testing - end
# union testing - end

type Workflow {
id: ID!
nodes: [Node!]
}

type Node {
name: String!
}
61 changes: 44 additions & 17 deletions graphql/resolve/update_mutation_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@
}
dgmutations:
- deletejson: |
{
{
"Author.posts": [{
"uid" : "0x124",
"Post.author": {
Expand Down Expand Up @@ -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: |
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -1422,8 +1449,8 @@
}
}
gqlvariables: |
{
"patch": {
{
"patch": {
"filter": {
"id": ["0x123"]
},
Expand All @@ -1434,7 +1461,7 @@
}
dgmutations:
- setjson: |
{
{
"uid" : "uid(x)",
"Author.posts": [
{
Expand Down Expand Up @@ -1488,7 +1515,7 @@
- setjson: |
{ "uid" : "uid(x)",
"Post.text": "updated text",
"Post.author": {
"Post.author": {
"uid": "0x456",
"Author.posts": [ { "uid": "uid(x)" } ]
}
Expand Down Expand Up @@ -1524,8 +1551,8 @@
}
}
gqlvariables: |
{
"patch": {
{
"patch": {
"filter": {
"id": ["0x123"]
},
Expand All @@ -1545,7 +1572,7 @@
cond: "@if(eq(len(State4), 0) AND gt(len(x), 0))"
dgmutationssec:
- setjson: |
{
{
"uid" : "uid(x)",
"Country.states": [
{
Expand Down Expand Up @@ -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"
Expand All @@ -1636,7 +1663,7 @@
dgmutationssec:
- setjson: |
{ "uid" : "uid(x)",
"ComputerOwner.computers": {
"ComputerOwner.computers": {
"uid": "uid(Computer4)",
"Computer.owners": [ { "uid": "uid(x)" } ]
}
Expand Down