Add support for multiple mutations

[mangalaman93]

Now, the upsert can have multiple mutation block and if..else mutations are now possible.

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@mangalaman93 you can click here to see the review status or cancel the code review job.

[pullrequest]

	1 Message
✅	It looks like the description for this pull request is either blank or very short. Adding a high-level summary will help our reviewers provide better feedback. Feel free to include questions for PullRequest reviewers and make specific feedback requests.

[pullrequest]

⚠️ Warning

PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More

[pullrequest]

The change looks good overall but does need some adjustments before getting it merged.

---

Reviewed with ❤️ by PullRequest

[pullrequest]

✖️ This code review was cancelled. See Details

[manishrjain]

Reviewable status: 0 of 13 files reviewed, 17 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/http.go, line 415 at r3 (raw file):

	mp := map[string]interface{}{}
	if len(resp.Json) != 0 {
		if err := json.Unmarshal(resp.Json, &mp); err != nil {

Avoid this. Find another way.

Query should contain: {"data": ..., "extensions": ...}

Perhaps find the first bracket, and inject the json from below into it. That'd be much faster.

---

edgraph/server.go, line 524 at r3 (raw file):

	req.Query = strings.TrimSpace(req.Query)
	isQuery := len(req.Query) != 0
	if !isQuery && !isMutation {

Might not need to check it up here.

---

edgraph/server.go, line 532 at r3 (raw file):

		req:      req,
		condVars: make([]string, len(req.Mutations)),
		uidRes:   make(map[string][]string),

Try to avoid these empty inits.

---

edgraph/server.go, line 535 at r3 (raw file):

		valRes:   make(map[string]map[uint64]types.Val),
		latency:  l,
		span:     span,

Don't really need this. ctx already has span.

---

edgraph/server.go, line 558 at r3 (raw file):

	resp = &api.Response{}
	if isQuery {

switch case for isQuery, isMutation, default

---

edgraph/server.go, line 632 at r3 (raw file):

// @if condition defined in Conditional Upsert.
func buildUpsertQuery(qc *queryContext) string {
	if len(qc.req.Query) == 0 {

check if len(qc.gmuList) == 0 or not here?

[mangalaman93]

Reviewable status: 0 of 13 files reviewed, 17 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])

---

go.mod, line 3 at r2 (raw file):

Previously, pullrequest[bot] wrote…

Was the CI switched to using Go 1.13 already? If not, I recommend reverting the change to go.mod, go.sum and redo it with Go 1.12.

This is because Go 1.13 has a GOPROXY enabled by default. This can lead to errors if any of your dependencies have been re-tagged upstream then Go 1.12 will see the new tag whilst Go 1.13 remains seeing the old tag (the goproxy is immutable) leading to a mismatch of the dependency's hash.

Done.

---

dgraph/cmd/alpha/http.go, line 415 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Avoid this. Find another way.

Query should contain: {"data": ..., "extensions": ...}

Perhaps find the first bracket, and inject the json from below into it. That'd be much faster.

Done.

---

edgraph/server.go, line 524 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Might not need to check it up here.

I have to check it here, otherwise parsing fails

---

edgraph/server.go, line 532 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Try to avoid these empty inits.

Done.

---

edgraph/server.go, line 535 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Don't really need this. ctx already has span.

ctx does have the key but the key for storing the context is not exported from the trace package. The exported function (FromContext) is only defined for type *SpanContext which is not the case here.

---

edgraph/server.go, line 558 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

switch case for isQuery, isMutation, default

We need to do both if it is an upsert. Not sure a switch statement will work.

---

edgraph/server.go, line 632 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

check if len(qc.gmuList) == 0 or not here?

Done.

[martinmr]

Reviewed 3 of 12 files at r3, 4 of 5 files at r5.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])

[manishrjain]

Reviewed 2 of 12 files at r3, 1 of 5 files at r5, 3 of 4 files at r6.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])

[pawanrawal]

apart from a small thing that I was not very sure of. Perhaps a comment would help.

Reviewed 3 of 12 files at r3, 1 of 5 files at r5, 3 of 4 files at r6.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, and @pullrequest[bot])

---

dgraph/cmd/alpha/http.go, line 354 at r7 (raw file):

			return mu, nil
		}
		if mu, err := extractMutation(ms); err != nil {

So ms can have set, delete and cond as keys and can also have mutations as keys? I would think that we only allow mutations as the key. Is that to maintain backward compatibility?

[mangalaman93]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @danielmai, @mangalaman93, @manishrjain, @MichaelJCompton, @pawanrawal, and @pullrequest[bot])

---

dgraph/cmd/alpha/http.go, line 354 at r7 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

So ms can have set, delete and cond as keys and can also have mutations as keys? I would think that we only allow mutations as the key. Is that to maintain backward compatibility?

yeah, pretty much. I'd add a comment in the code.