fix(Dgraph): update reverse index when updating single UID predicates.

[martinmr]

When updating a single uid predicate with a reverse index, the existing entry in the
reverse index should be deleted first.

Fixes DGRAPH-1738

---

This change is 

Docs Preview:

[parasssh]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)

---

posting/index.go, line 219 at r1 (raw file):

	// entries for this key in the index are removed.
	pred, ok := schema.State().Get(ctx, t.Attr)
	isSingleUidUpdate := ok && !pred.GetList() && pred.GetValueType() == pb.Posting_UID &&

Wouldn't we need to fix for when object/value is UID list as well? e.g. A -> [B,C,D] and C is deleted?

---

posting/index.go, line 228 at r1 (raw file):

				hex.Dump(dataKey))
		}
		err = dataList.Iterate(txn.StartTs, 0, func(p *pb.Posting) error {

We expect only one item in the list, correct?

---

systest/mutations_test.go, line 2015 at r1 (raw file):

	testutil.CompareJSON(t, `{"me":[{"name":"Alice","best_friend": {"name": "Bob"}}]}`,
		string(resp.GetJson()))

Add check for reverse to make sure Bob has the reverse edge.

---

systest/mutations_test.go, line 2040 at r1 (raw file):

	require.NoError(t, err)
	testutil.CompareJSON(t, `{"reverse":[{"name":"Carol","~best_friend": [{"name": "Alice"}]}]}`,
		string(resp.GetJson()))

Other related Test cases could be:

1. When Bob is deleted, the edge and reverse edge should be deleted.
2. When Alice is deleted, the edge and reverse edge should be deleted.

[martinmr]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)

---

posting/index.go, line 219 at r1 (raw file):

Previously, parasssh wrote…

Wouldn't we need to fix for when object/value is UID list as well? e.g. A -> [B,C,D] and C is deleted?

No. This is just for the single uid. Overwriting a single uid predicate means the previous value needs to be deleted. We were doing that but not deleting the reverse edge.

---

posting/index.go, line 228 at r1 (raw file):

Previously, parasssh wrote…

We expect only one item in the list, correct?

Yes. I am just using iterate because it's the proper way to handle the values.

---

systest/mutations_test.go, line 2015 at r1 (raw file):

Previously, parasssh wrote…

Add check for reverse to make sure Bob has the reverse edge.

Done.

---

systest/mutations_test.go, line 2040 at r1 (raw file):

Previously, parasssh wrote…

Other related Test cases could be:

1. When Bob is deleted, the edge and reverse edge should be deleted.
2. When Alice is deleted, the edge and reverse edge should be deleted.

Not sure if this cases are relevant to this case. Deletion is handled the same for uid and [uid] predicates. The difference with single uid predicates is that writing to the list means that the previous value has to be deleted (along with the reverse index).

[parasssh]

minor comments. I'd like Pawan to bless it as well.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)

---

posting/index.go, line 219 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

No. This is just for the single uid. Overwriting a single uid predicate means the previous value needs to be deleted. We were doing that but not deleting the reverse edge.

Ok. With a list, it just extends the list and so issue doesn't exist there.

---

systest/mutations_test.go, line 2040 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Not sure if this cases are relevant to this case. Deletion is handled the same for uid and [uid] predicates. The difference with single uid predicates is that writing to the list means that the previous value has to be deleted (along with the reverse index).

I didn't mean tests for [uid] preds. I meant deleting Alice and Bob with your current test schema of single predicate (with reverse) to make sure reverse edges are not left dangling. Feel free to resolve.

[martinmr]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @parasssh, and @vvbalaji-dgraph)

---

posting/index.go, line 219 at r1 (raw file):

Previously, parasssh wrote…

Ok. With a list, it just extends the list and so issue doesn't exist there.

Yeah, that's the case.

---

systest/mutations_test.go, line 2040 at r1 (raw file):

Previously, parasssh wrote…

I didn't mean tests for [uid] preds. I meant deleting Alice and Bob with your current test schema of single predicate (with reverse) to make sure reverse edges are not left dangling. Feel free to resolve.

I added a test to verify the reverse edge is gone when I remove the original edge.

[pawanrawal]

:lgtm

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)

---

posting/index.go, line 214 at r2 (raw file):

		return err
	}
	x.AssertTrue(plist != nil)

Do we need this to be an assert or should this be an error that can be returned to the user?

[martinmr]

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

---

posting/index.go, line 214 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do we need this to be an assert or should this be an error that can be returned to the user?

Done.