Update patch with blank `remove` section removes selected nodes?

Issues GraphQL
, ,
1

I am using 20.11
and I am unsure if this is a bug or by design

with this schema

type X  {
  id: ID!
  name: String! @id @search(by: [hash, regexp])
  other: String
}

assuming node was created

mutation {
  addX(input:{name: "Example", other: "other_value"}){
    x {
      id       
    }     
  }
}

when i run a updateX mutation with a patch like

mutation($patch: UpdateXInput!) {
  update: updateX(input: $patch) {
  x {      
      id      
    }
  }
}

vars

{
	"patch": {
		"filter": {
			"name": {
				"eq": "Example"
			}
		},
		"set": {
			"other": "test"
		},
		"remove": {}
	}
}

I do get the id back, but then the node is removed

when running with without the remove section

vars

{
	"patch": {
		"filter": {
			"name": {
				"eq": "Example"
			}
		},
		"set": {
			"other": "test"
		}
	}
}

the values are update correctly

1 Like
2

Hey @jcsrb, I am able to reproduce this issue locally. This is a bug and should be fixed. "remove" = {} should have no effect on the update mutation. Both must be equivalent.

5

Hi @jcsrb, At first look it looks like a bug.
But after investigation I find out that this is expected behavior. remove {} gets translated to remove * and delete everything for the predicate that is in filter.

Thanks for reporting it, we will make changes in our docs to reflect this.

3 Likes
6

@JatinDevDG @minhaj

I just stumbled upon this issue and I would like to suggest reconsidering this as an intentional behavior.

The GraphQL schema already provides the deleteX mutation in order to delete one or more nodes, it is in my opinion dangerous and unexpected that an update with remove set to {} completely obliterates the node. This could result in disasters very, very easily.

Let’s consider as an example a Golang client generated from a GraphQL schema and used to update an item (the way we found out about this behavior):

	input := &client.UpdateLibraryItemInput{
		Filter: &client.LibraryItemFilter{
			ID: &client.StringHashFilter{
				Eq: graphql.StringRef(id),
			},
			Etag: &client.StringHashFilter{
				Eq: graphql.StringRef(etag),
			},
		},
		Set:    &client.LibraryItemPatch{},
		Remove: &client.LibraryItemPatch{},
	}

	input.Set.ModifiedAt = graphql.TimeRef(time.Now())
	input.Set.Generation = graphql.IntRef(libraryItem.GetLibraryItem.Generation + 1)
	input.Set.Etag = graphql.StringRef(graphql.NewETag())
	// ...

	resp, err := cli.UpdateLibraryItem(ctx.r.Context(), *input)
	// ...

If you look closely you will notice that Remove is initialized beforehand to be populated later, but what happens if you don’t set any value in the Remove struct is not that the value gets ignored, but the whole node gets wiped.

It is also worth mentioning that this behavior makes the @auth rule for delete operation meaningless as you can achieve deletion without being authorized to perform a delete just by executing an update with an empty remove object. This is by definition a security breach and a very important one, as delete is normally allowed to be performed only by system administrators.

3 Likes
7

I think there was another post on this and it was accepted to change this behaviour. I thought, but now can’t locate it, idk. I agree that this behavior is not what is expected if not very well documented.

@pawan is there a change ticket for this already?

1 Like
8

@JatinDevDG I agree that this must be treated as a bug; it is completely counterintuitive to delete something based on a wildcard update clause.

1 Like
9

Hi all, we are also having an internal discussion regarding it. I have reopened the ticket and will discuss and change this behavior.
Thanks for your critical responses.

2 Likes
10

Hi all, fix for this bug is merged in 21.03 branch. Now empty set{} or remove{} don’t have any effect in update mutation. Thanks for all your inputs on this bug.
PR: fix(GRAPHQL): fix empty remove in update mutation patch, that remove all the data for nodes in filter. by JatinDevDG · Pull Request #7563 · dgraph-io/dgraph · GitHub

3 Likes