graphql: ensure upserts don't have accidental edge removal

[MichaelJCompton]

Fixes #5355

Adds testing around the issue. The tests now represent all the possible cases. One case ("updt single edge" cases) was the issue and failing.

The fix doesn't change any existing tests, except one that was also incorrect.

---

This change is 

[pawanrawal]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @MichaelJCompton)

---

graphql/resolve/add_mutation_test.yaml, line 1310 at r1 (raw file):

#
# This delete only needs to be done when there is a singular edge in the mutation:
# i.e. if both directions of the edge are [], then it's just an add.

So we generate the delete if any of edges (forward/backward) are a singular edge? Do we have test cases for when both the forward and backward edges are singular? I guess the behavior would be similar to what it is for the case above. Also, are there any tests where both edges are plural edges?

---

graphql/resolve/add_mutation_test.yaml, line 1312 at r1 (raw file):

# i.e. if both directions of the edge are [], then it's just an add.
#
# There's three cases to consider: add by ID, add by XID, deep add

For update, we have two cases each for by ID and by XID to test from both directions. I suppose that doesn't make much sense here?

---

graphql/resolve/mutation_rewriter.go, line 1145 at r1 (raw file):

		exclude = excludeVar[4 : len(excludeVar)-1]
	}
	if !strings.HasPrefix(excludeVar, "_:") {

Don't really understand what changed here. excludeVar can either be of the type uid(x) or be a blank node. Can it be something else as well?

---

graphql/resolve/update_mutation_test.yaml, line 1164 at r1 (raw file):

#
# This delete only needs to be done when there is a singular edge in the mutation:
# i.e. if both directions of the edge are [], then it's just an add.  We also need

Do we have cases where we have @hasInverse and both the directions are [] to verify that delete isn't generated in those cases?

[MichaelJCompton]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain and @pawanrawal)

---

graphql/resolve/add_mutation_test.yaml, line 1310 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

So we generate the delete if any of edges (forward/backward) are a singular edge? Do we have test cases for when both the forward and backward edges are singular? I guess the behavior would be similar to what it is for the case above. Also, are there any tests where both edges are plural edges?

Yeah, there's tests already for single<->single and list<->list. The single<->single case is the one test that changed in the update tests.

I decided not to add extra versions those cases here because they are covered elsewhere and the test is really just about 'if there is a single edge'.

---

graphql/resolve/add_mutation_test.yaml, line 1312 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

For update, we have two cases each for by ID and by XID to test from both directions. I suppose that doesn't make much sense here?

Yeah, that's because for update there's and additional constraint - the test in the query that we aren't removing the existing edge ... for adding, there's no existing edge, so it can never have the @filter(NOT (uid(...))) part in the upsert query. Where as when we are updating things, we need to check first that if the update sets it to the same value it has we don't do the removal

That was the bug. In some cases we'd do both the add and removal when it was just updating to the same value that was already there ... if the delete part of the upsert runs second, an existing edge disapears.

---

graphql/resolve/mutation_rewriter.go, line 1145 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Don't really understand what changed here. excludeVar can either be of the type uid(x) or be a blank node. Can it be something else as well?

Added a bunch more comments here and at the top of the fn

---

graphql/resolve/update_mutation_test.yaml, line 1164 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do we have cases where we have @hasInverse and both the directions are [] to verify that delete isn't generated in those cases?

There are existing cases that cover that.

[pawanrawal]

thanks for the detailed comments

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @MichaelJCompton)

---

graphql/resolve/mutation_rewriter.go, line 1114 at r2 (raw file):

// we are about to attach
//
// Post1 --- author --> Author1

Shouldn't this be Post2?