Skip to content
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

Add JSON mutations to raw HTTP #2396

Merged
merged 5 commits into from
May 30, 2018
Merged

Add JSON mutations to raw HTTP #2396

merged 5 commits into from
May 30, 2018

Conversation

gpahal
Copy link
Contributor

@gpahal gpahal commented May 17, 2018

This adds Json mutations to Dgraph's HTTP server.

Syntax:

{
    "set": [{ "name": "Alice" }],
    "delete": { "uid": "0x1", "name": null }
}

Possible limitations/problems:

  • Parses the JSON twice: first for veryfing json and separating set and delete and second time to actually perform the mutation on api.Mutation
  • Requires header indicating the type is JSON so that error handling is better and json mutations are not slowed down because of first checking for nquad mutations

This change is Reviewable

@gpahal gpahal requested a review from manishrjain May 17, 2018 07:51
@manishrjain
Copy link
Contributor

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


dgraph/cmd/server/http.go, line 173 at r1 (raw file):

		// Parse JSON.
		ms := make(map[string]interface{})
		err := json.Unmarshal(m, &ms)

JSON unmarshal needs to be worked upon. So, let's consolidate that in one place. Can you use the same method that grpc uses to unmarshal JSON; so it's all in one place?


Comments from Reviewable

@gpahal
Copy link
Contributor Author

gpahal commented May 18, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


dgraph/cmd/server/http.go, line 173 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

JSON unmarshal needs to be worked upon. So, let's consolidate that in one place. Can you use the same method that grpc uses to unmarshal JSON; so it's all in one place?

This JSON marshal only validates the JSON and checks its "set" and "delete" keys. It doesn't do anything with the values or numbers. And the rest of the function requires mu *api.Mutation which has SetJson and DeleteJson as unparsed JSON strings, so anyways we need the unparsed JSON. To avoid this, we would need to change the interface of edgraph.Mutate and nquadsToJson.

I didn't change much because I wanted to introduce this change without affecting any other code.


Comments from Reviewable

@gpahal
Copy link
Contributor Author

gpahal commented May 21, 2018

@manishrjain ^

@manishrjain
Copy link
Contributor

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


dgraph/cmd/server/http.go, line 172 at r1 (raw file):

	if mType := r.Header.Get("X-Dgraph-MutationType"); mType == "json" {
		// Parse JSON.
		ms := make(map[string]interface{})

map[string][]byte? So, you don't need to marshal below.


Comments from Reviewable

@gpahal
Copy link
Contributor Author

gpahal commented May 29, 2018

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


dgraph/cmd/server/http.go, line 172 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

map[string][]byte? So, you don't need to marshal below.

Done.


Comments from Reviewable

@manishrjain
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@gpahal gpahal merged commit f81956b into master May 30, 2018
@gpahal gpahal deleted the gp/json-http branch May 30, 2018 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants