Remove custom HTTP Headers, cleanup API

[mangalaman93]

Fixes #3142
Why remove custom HTTP Headers?

• Needs to be whitelisted for CORS
• Parsing Unicode characters in headers is too hard
• Uncommon to use, there are better approaches

We are still not completely GraphQL compatible. In fact,
GraphQL specs are not clear around this to begin with but
this commit cleans up HTTP API further.

Todo -

• Tests for application/json content type
• Remove headers from other endpoints
• Update documentation
• Raise issues with the clients that are using HTTP endpoint

---

This change is 

[manishrjain]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @mangalaman93)

---

dgraph/cmd/alpha/http.go, line 187 at r1 (raw file):

		Variables    map[string]string `json:"variables"`
		StartTs      uint64            `json:"startTs"`
		Debug        bool              `json:"debug"`

Providing multiple ways of doing the same thing can create confusion. Also, if we provide two ways of doing N things, users would expect this to work with N+1 things (whenever another thing is added).

Best thing to do is to choose one way and just stick to it. I'd remove timeout, debug, be, ro, etc. -- all the vars that are being parsed from url.

---

dgraph/cmd/alpha/http.go, line 313 at r1 (raw file):

		MutationType string `json:"mutationType"`
		CommitNow    bool   `json:"commitNow"`
		StartTs      uint64 `json:"startTs"`

Same goes here.

---

dgraph/cmd/alpha/http_test.go, line 163 at r1 (raw file):

	var preds []string
	if isJson {
		url += separator + "mutationType=json"

You can put the key-value in a []string and call strings.Join(kvs, "&") at the end. Would be cleaner.

[mangalaman93]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

---

dgraph/cmd/alpha/http.go, line 187 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Providing multiple ways of doing the same thing can create confusion. Also, if we provide two ways of doing N things, users would expect this to work with N+1 things (whenever another thing is added).

Best thing to do is to choose one way and just stick to it. I'd remove timeout, debug, be, ro, etc. -- all the vars that are being parsed from url.

Done.

---

dgraph/cmd/alpha/http.go, line 313 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Same goes here.

Done.

---

dgraph/cmd/alpha/http_test.go, line 163 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You can put the key-value in a []string and call strings.Join(kvs, "&") at the end. Would be cleaner.

Done.

[martinmr]

Reviewed 5 of 5 files at r2, 7 of 8 files at r3.
Reviewable status: 12 of 13 files reviewed, 17 unresolved discussions (waiting on @danielmai, @golangcibot, @mangalaman93, and @manishrjain)

---

dgraph/cmd/alpha/http.go, line 436 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

Error return value of writeResponse is not checked (from errcheck)

fix this by doing _ = writeResponse(w, r, js)

---

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

	}

	// read parameters from body

// Read parameters from body.

---

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

		}

	// when content type is marked as application/graphql

Can you clarify this comment, it's not a full sentence so it's a bit ambiguous. I am assuming it means that the default case covers the case where contentType = application/graphql. If that's the case, it might be better to have a case for it explicitly and use the default case to handle unexpected values.

---

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

	}

	// setup context

The comment is not really needed. In general, comments should explain why the code is the way it is but in this case the purpose of the code is fairly obvious.

---

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

	ctx = attachAccessJwt(ctx, r)

	// setup timeout

Comment is not really needed.

---

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

	}

	// Setup query request

Comment is not really needed.

---

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

	}

	// read parameters from body

Comment is not really needed.

---

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

		}

	// when content type is marked as application/graphql

Same here as above. Have a explicit case for "application/graphql" and let the default case handle unexpected inputs.

---

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

	}

	// parsing complete

comment is not really needed.

---

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

	parseEnd := time.Now()

	// setup muation query parameters

comment is not really needed.

---

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

	var response map[string]interface{}
	if aborted {
		// Abort Handler

Comment is not really needed.

---

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

		}

		// Commit handler

Comment is not really needed.

---

dgraph/cmd/alpha/run.go, line 378 at r3 (raw file):

	http.HandleFunc("/mutate/", mutationHandler)
	http.HandleFunc("/commit/", commitHandler)
	http.HandleFunc("/abort/", abortHandler)

I am assuming the HTTP clients need to be changed to reflect this change, right?

---

wiki/content/tips/index.md, line 57 at r3 (raw file):

  }
}
{{< /runnable >}}

add a new line at the end of this file

[gitlw]

Reviewable status: 12 of 13 files reviewed, 22 unresolved discussions (waiting on @danielmai, @golangcibot, @mangalaman93, and @manishrjain)

---

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

	}

	body := readRequest(w, r)

In general, it's better to validate the metadata in headers before loading the request body, which may have a large payload.

---

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

	contentType := r.Header.Get("Content-Type")
	switch strings.ToLower(contentType) {

A subjective comment:
The approached adopted here says when a query can be put directly into the payload. However, when it has vars, it must be put into json format.
IMO, that adds confusion, and burden to users' memory. I'd prefer having a unified input format by always requiring the input format to be json.

---

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

	}

	body := readRequest(w, r)

Ditto about checking headers before loading content.

---

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

	// read parameters from body
	var params struct {
		Query []byte `json:"query"`

Is "mutation" a better name?

---

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

	contentType := r.Header.Get("Content-Type")
	switch strings.ToLower(contentType) {
	case "application/json":

Previously the x-Dgraph-MutationType header determines whether the content should be treated as json or N-Quads.

Now we are looking at both the content-type and the mutationType, which may lead to confusions if these two dimensions do not match, e.g.
what does it mean to have the content-type to be json but the mutationType to be not json.

Should we only keep one?

[mangalaman93]

Reviewable status: 12 of 13 files reviewed, 22 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/http.go, line 436 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

fix this by doing _ = writeResponse(w, r, js)

Done.

---

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

Previously, gitlw (Lucas Wang) wrote…

In general, it's better to validate the metadata in headers before loading the request body, which may have a large payload.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

// Read parameters from body.

Done.

---

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

Previously, gitlw (Lucas Wang) wrote…

A subjective comment:
The approached adopted here says when a query can be put directly into the payload. However, when it has vars, it must be put into json format.
IMO, that adds confusion, and burden to users' memory. I'd prefer having a unified input format by always requiring the input format to be json.

I like the non-json approach for its simplicity and avoids everyone to look at the a slightly more complex json API. Only when the variables are used (the not so common use case) is when you need to provide a JSON.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Can you clarify this comment, it's not a full sentence so it's a bit ambiguous. I am assuming it means that the default case covers the case where contentType = application/graphql. If that's the case, it might be better to have a case for it explicitly and use the default case to handle unexpected values.

I think even when the user forgets to send the content type, the API should work. I do not think it would make sense to throw an error if content type is not set.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

The comment is not really needed. In general, comments should explain why the code is the way it is but in this case the purpose of the code is fairly obvious.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Comment is not really needed.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Comment is not really needed.

Done.

---

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

Previously, gitlw (Lucas Wang) wrote…

Ditto about checking headers before loading content.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Comment is not really needed.

Done.

---

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

Previously, gitlw (Lucas Wang) wrote…

Is "mutation" a better name?

We are using query in other endpoints too. Do not see a lot of benefits of changing this to Mutation.

---

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

Previously, gitlw (Lucas Wang) wrote…

Previously the x-Dgraph-MutationType header determines whether the content should be treated as json or N-Quads.

Now we are looking at both the content-type and the mutationType, which may lead to confusions if these two dimensions do not match, e.g.
what does it mean to have the content-type to be json but the mutationType to be not json.

Should we only keep one?

I think mutationType could still be RDF while contentType is JSON. I would expect the query to contain RDF data instead of json in it. Note that, even when mutationType is JSON, the value stored in query will not be json, but a sequence of bytes containing the JSON.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Same here as above. Have a explicit case for "application/graphql" and let the default case handle unexpected inputs.

See my comment above and let me know what you think.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

comment is not really needed.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

comment is not really needed.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Comment is not really needed.

Done.

---

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

Previously, martinmr (Martin Martinez Rivera) wrote…

Comment is not really needed.

Done.

---

dgraph/cmd/alpha/run.go, line 378 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I am assuming the HTTP clients need to be changed to reflect this change, right?

I have raised an issue in Dgraph-JS-http library. I don't think we are using the HTTP client anywhere else. I have updated the docs here too. Anything else that comes to your mind?

---

wiki/content/tips/index.md, line 57 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

add a new line at the end of this file

Done.

[gitlw]

Reviewable status: 11 of 13 files reviewed, 24 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, @martinmr, and @MichaelJCompton)

---

dgraph/cmd/alpha/http.go, line 436 at r2 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Done.

Hmm, this looks very weird. why not actually checking the error returnedby writeResponse?

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

We are using query in other endpoints too. Do not see a lot of benefits of changing this to Mutation.

Which endpoint are you referring to?

---

dgraph/cmd/alpha/http.go, line 401 at r4 (raw file):

	}

	aborted, err := parseBool(r.URL.Query().Get("aborted"), "aborted")

I'd suggest renaming this to "abort" since its meaning here is a verb: "make the transaction abort".

---

dgraph/cmd/alpha/http.go, line 552 at r4 (raw file):

	}

	_, _ = writeResponse(w, r, js)

ditto about actually checking the error code

[mangalaman93]

Reviewable status: 11 of 13 files reviewed, 24 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, @martinmr, and @MichaelJCompton)

---

dgraph/cmd/alpha/http.go, line 436 at r2 (raw file):

Previously, gitlw (Lucas Wang) wrote…

Hmm, this looks very weird. why not actually checking the error returnedby writeResponse?

Even when we check for error, there is not much we can do after that. If writeResponse fails, it wouldn't make sense to try writing a error response now instead because that is likely to fail too. We can potentially panic, but I am not sure that's a good idea.

---

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

Previously, gitlw (Lucas Wang) wrote…

Which endpoint are you referring to?

/query endpoint, that's the only one.

---

dgraph/cmd/alpha/http.go, line 401 at r4 (raw file):

Previously, gitlw (Lucas Wang) wrote…

I'd suggest renaming this to "abort" since its meaning here is a verb: "make the transaction abort".

That make sense. I can change the variable name, changing the parameter name may be confusing. I see everywhere we have used aborted, in GRPC for example. Did you mean to change the variable name?

---

dgraph/cmd/alpha/http.go, line 552 at r4 (raw file):

Previously, gitlw (Lucas Wang) wrote…

ditto about actually checking the error code

same as above

---

dgraph/cmd/alpha/run.go, line 378 at r3 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

I have raised an issue in Dgraph-JS-http library. I don't think we are using the HTTP client anywhere else. I have updated the docs here too. Anything else that comes to your mind?

Done.

[manishrjain]

Got a bunch of comments. In general, use vars close to declaration. And avoid unnecessary changes.

Reviewed 4 of 5 files at r2, 7 of 8 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 33 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @martinmr, and @MichaelJCompton)

---

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

	}

	uintVal, err := strconv.ParseUint(value, 10, 64)

Base can be set to 0 to allow auto-interpretation, which is more robust anyway.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

I think even when the user forgets to send the content type, the API should work. I do not think it would make sense to throw an error if content type is not set.

What does the spec say about this?

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

/query endpoint, that's the only one.

What is the need for this? We were passing body directly to be parsed before. Now just read the body, we need to do another json unmarshal?

Also, this seems like a new change not required to implement the spec.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

See my comment above and let me know what you think.

What does the spec say about this?

---

dgraph/cmd/alpha/http.go, line 160 at r4 (raw file):

	}

	isDebugMode, err := parseBool(r.URL.Query().Get("debug"), "debug")

comment below.

---

dgraph/cmd/alpha/http.go, line 165 at r4 (raw file):

		return
	}
	queryTimeout, err := parseDuration(r.URL.Query().Get("timeout"), "timeout")

You can just pass request and the key to parseX funcs. parseDuration(r, "timeout") reads better. And do the URL.Query().Get(y) internally within the func.

---

dgraph/cmd/alpha/http.go, line 170 at r4 (raw file):

		return
	}
	startTs, err := parseUint64(r.URL.Query().Get("startTs"), "startTs")

comment above.

---

dgraph/cmd/alpha/http.go, line 182 at r4 (raw file):

	var params struct {
		Query     string            `json:"query"`

This params struct is being used both by query and mutation. Would make sense to move it out and use it across both.

---

dgraph/cmd/alpha/http.go, line 190 at r4 (raw file):

		if err := json.Unmarshal(body, &params); err != nil {
			x.SetStatus(w, x.ErrorInvalidRequest,
				"Error while unmarshalling body into json object")

Do send back the actual error as well. Otherwise, there's a loss of information here which could have been helpful to the user in resolving the issue.

---

dgraph/cmd/alpha/http.go, line 204 at r4 (raw file):

	ctx = attachAccessJwt(ctx, r)

	if queryTimeout != 0 {

Create the variable closer to usage. Or, in other words, use the variable closer to creation.

Move the queryTimeout parsing just above this.

---

dgraph/cmd/alpha/http.go, line 210 at r4 (raw file):

	}

	queryReq := api.Request{

s/queryReq/req

Also, init this right after you have all the information, i.e. startTs and params.

---

dgraph/cmd/alpha/http.go, line 218 at r4 (raw file):

	if queryReq.StartTs == 0 {
		// If be is set, run this as a best-effort query.
		isBestEffort, err := parseBool(r.URL.Query().Get("be"), "be")

See comment above about parseX.

---

dgraph/cmd/alpha/http.go, line 317 at r4 (raw file):

	var mu *api.Mutation
	mutationType := r.URL.Query().Get("mutationType")

This is deviating from the agreed upon spec. Why? Mutation type was going to be determined by content type header.

#3142 (comment)

---

dgraph/cmd/alpha/http.go, line 401 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

That make sense. I can change the variable name, changing the parameter name may be confusing. I see everywhere we have used aborted, in GRPC for example. Did you mean to change the variable name?

In Grpc, that's a decision. This is an instruction.

---

dgraph/cmd/alpha/http.go, line 519 at r4 (raw file):

	op := &api.Operation{}
	bodyStr := string(body)
	err := jsonpb.UnmarshalString(bodyStr, op)

Avoid unnecessary changes. This seems like a micro-optimization for an error case.

Also, b as a variable name is just as good as body for usage within this function.

---

dgraph/cmd/alpha/http_test.go, line 161 at r4 (raw file):

	var preds []string
	if isJson {
		queryParams = append(queryParams, "mutationType=json")

Should be header. And should be application/json or whatever the correct content-type is.

[mangalaman93]

Reviewable status: all files reviewed, 33 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, @martinmr, and @MichaelJCompton)

---

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

Previously, manishrjain (Manish R Jain) wrote…

Base can be set to 0 to allow auto-interpretation, which is more robust anyway.

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

What does the spec say about this?

Spec doesn't mention anything about this.

---

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

Previously, manishrjain (Manish R Jain) wrote…

What is the need for this? We were passing body directly to be parsed before. Now just read the body, we need to do another json unmarshal?

Also, this seems like a new change not required to implement the spec.

the /mutate endpoint doesn't have variables as of now. I was thinking that it will allow having variables in /mutate endpoint as well similar to the /query endpoint. That is why I kept this.

---

dgraph/cmd/alpha/http.go, line 160 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

comment below.

Done.

---

dgraph/cmd/alpha/http.go, line 165 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

You can just pass request and the key to parseX funcs. parseDuration(r, "timeout") reads better. And do the URL.Query().Get(y) internally within the func.

Done.

---

dgraph/cmd/alpha/http.go, line 170 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

comment above.

Done.

---

dgraph/cmd/alpha/http.go, line 182 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This params struct is being used both by query and mutation. Would make sense to move it out and use it across both.

I still prefer to keep them separately, in the hope that they can evolve separately.

---

dgraph/cmd/alpha/http.go, line 190 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Do send back the actual error as well. Otherwise, there's a loss of information here which could have been helpful to the user in resolving the issue.

Done.

---

dgraph/cmd/alpha/http.go, line 204 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Create the variable closer to usage. Or, in other words, use the variable closer to creation.

Move the queryTimeout parsing just above this.

The reason I moved it up is because we wanted to parse and validate the parameters before parsing the body.

---

dgraph/cmd/alpha/http.go, line 210 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

s/queryReq/req

Also, init this right after you have all the information, i.e. startTs and params.

Updated the name. To me, it made more sense to initialize it when I was about to use it.

---

dgraph/cmd/alpha/http.go, line 218 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

See comment above about parseX.

Done.

---

dgraph/cmd/alpha/http.go, line 317 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This is deviating from the agreed upon spec. Why? Mutation type was going to be determined by content type header.

#3142 (comment)

It mentions in your comment. Though, they query is supposed to be a string, not a json, containing either RDF bytes or json bytes.

---

dgraph/cmd/alpha/http.go, line 401 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

In Grpc, that's a decision. This is an instruction.

Should we change it then?

---

dgraph/cmd/alpha/http.go, line 519 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Avoid unnecessary changes. This seems like a micro-optimization for an error case.

Also, b as a variable name is just as good as body for usage within this function.

will be careful in future.

[mangalaman93]

Reviewable status: 8 of 13 files reviewed, 33 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, @martinmr, and @MichaelJCompton)

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

I like the non-json approach for its simplicity and avoids everyone to look at the a slightly more complex json API. Only when the variables are used (the not so common use case) is when you need to provide a JSON.

Done.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

Spec doesn't mention anything about this.

Done.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

the /mutate endpoint doesn't have variables as of now. I was thinking that it will allow having variables in /mutate endpoint as well similar to the /query endpoint. That is why I kept this.

Done.

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

I think mutationType could still be RDF while contentType is JSON. I would expect the query to contain RDF data instead of json in it. Note that, even when mutationType is JSON, the value stored in query will not be json, but a sequence of bytes containing the JSON.

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

What does the spec say about this?

Done.

---

dgraph/cmd/alpha/http.go, line 210 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Updated the name. To me, it made more sense to initialize it when I was about to use it.

Done.

---

dgraph/cmd/alpha/http.go, line 317 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

It mentions in your comment. Though, they query is supposed to be a string, not a json, containing either RDF bytes or json bytes.

Done.

---

dgraph/cmd/alpha/http.go, line 401 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Should we change it then?

Done.

---

dgraph/cmd/alpha/http.go, line 519 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

will be careful in future.

Done.

---

dgraph/cmd/alpha/http_test.go, line 161 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Should be header. And should be application/json or whatever the correct content-type is.

Done.

[danielmai]

Reviewed 11 of 14 files at r6.
Reviewable status: 14 of 17 files reviewed, 43 unresolved discussions (waiting on @gitlw, @golangcibot, @mangalaman93, @manishrjain, @martinmr, and @MichaelJCompton)

---

dgraph/cmd/alpha/http.go, line 204 at r6 (raw file):

	default:
		x.SetStatus(w, x.ErrorInvalidRequest, "Unsupported content type")

Unsupported Content-Type, so I know that this is related to the Content-Type header and not... the type of the payload content.

---

dgraph/cmd/alpha/notes.txt, line 1 at r6 (raw file):

curl -H "Content-Type: application/graphql" localhost:8080/query -XPOST -d '{

We can undo these changes. These queries as-is are old and won't be able to run anyway.

---

query/benchmark/README.md, line 15 at r6 (raw file):

for NUM in $NUMS; do
	curl -H "Content-Type: application/graphql" localhost:8912/query -XPOST -d "{

We can undo these changes. These queries as-is are old and won't be able to run anyway.

---

query/benchmark/run.sh, line 22 at r6 (raw file):

for ACTOR in $ACTORS; do
  curl -H "Content-Type: application/graphql" localhost:8912/query -XPOST -d "

We can undo these changes. These queries as-is are old and won't be able to run anyway.

---

wiki/content/clients/index.md, line 538 at r6 (raw file):

Remember to set the content type header

"Content-Type" instead of "content type".

---

wiki/content/clients/index.md, line 610 at r6 (raw file):

We now send the mutations via the `/mutate` endpoint. We need to provide our
transaction start timestamp as a path parameter, so that Dgraph knows which
transaction the mutation should be part of. We also need to set content type

"Content-Type" instead of "content type".

---

wiki/content/clients/index.md, line 739 at r6 (raw file):

$ curl -X POST \
  -H 'Content-Encoding: gzip' \
	-H "Content-Type: application/rdf" \

Line up the indentation with the other "-H" args.

---

wiki/content/clients/index.md, line 748 at r6 (raw file):

$ curl -X POST \
  -H 'Accept-Encoding: gzip' \
	-H "Content-Type: application/graphql"

• Line up the indentation with the other "-H" args.
• Missing trailing backslash for multi-line command.

---

wiki/content/clients/index.md, line 768 at r6 (raw file):

  -H 'Content-Encoding: gzip' \
  -H 'Accept-Encoding: gzip' \
	-H "Content-Type: application/graphql"

• Line up the indentation with the other "-H" args.
• Missing trailing backslash for multi-line command.

---

wiki/content/get-started/index.md, line 204 at r6 (raw file):

{{% notice "tip" %}}Once Dgraph is running, you can access Ratel at [`http://localhost:8000`](http://localhost:8000). It allows browser-based queries, mutations and visualizations.

The mutations and queries below can either be run from the command line using `curl -H "Content-Type: application/graphql" localhost:8080/query -XPOST -d $'...'` or by pasting everything between the two `'` into the running user interface on localhost.{{% /notice %}}

Mutations don't use the application/graphql Content-Type header. Only Content-Type: application/rdf or Content-Type: application/json.

[mangalaman93]

Reviewable status: 14 of 17 files reviewed, 43 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, @martinmr, and @MichaelJCompton)

---

dgraph/cmd/alpha/http.go, line 204 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Unsupported Content-Type, so I know that this is related to the Content-Type header and not... the type of the payload content.

Done.

---

dgraph/cmd/alpha/notes.txt, line 1 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

We can undo these changes. These queries as-is are old and won't be able to run anyway.

Done.

---

query/benchmark/README.md, line 15 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

We can undo these changes. These queries as-is are old and won't be able to run anyway.

Done.

---

query/benchmark/run.sh, line 22 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

We can undo these changes. These queries as-is are old and won't be able to run anyway.

Done.

---

wiki/content/clients/index.md, line 538 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Remember to set the content type header

"Content-Type" instead of "content type".

Done.

---

wiki/content/clients/index.md, line 610 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

"Content-Type" instead of "content type".

Done.

---

wiki/content/clients/index.md, line 739 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Line up the indentation with the other "-H" args.

Done.

---

wiki/content/clients/index.md, line 748 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

• Line up the indentation with the other "-H" args.
• Missing trailing backslash for multi-line command.

Done.

---

wiki/content/clients/index.md, line 768 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

• Line up the indentation with the other "-H" args.
• Missing trailing backslash for multi-line command.

Done.

---

wiki/content/get-started/index.md, line 204 at r6 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Mutations don't use the application/graphql Content-Type header. Only Content-Type: application/rdf or Content-Type: application/json.

Done.n

[MichaelJCompton]

👍 Looked at the docs changes for this PR and they look fine.

More broadly though, there's lots of bits in the docs like: "We now send the mutations via ... We need to provide .... We also need to set..." , etc. I think those should be tightened up and shortened as a single pass through. After this PR goes in, I'll go through some of the pages.

[manishrjain]

Got a couple of comments. Address those before merging.

Reviewed 6 of 14 files at r6, 8 of 8 files at r7.
Reviewable status: all files reviewed, 41 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @mangalaman93, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/http.go, line 190 at r4 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

Done.

I don't see the change. The error is still not part of the x.SetStatus call.

---

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

		}

	case "application/graphql":

Might want to call it application/graphqlpm to refer to GraphQL+-.

[mangalaman93]

Reviewable status: 13 of 14 files reviewed, 41 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, and @martinmr)

---

dgraph/cmd/alpha/http.go, line 190 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I don't see the change. The error is still not part of the x.SetStatus call.

Done, now!

---

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

Previously, manishrjain (Manish R Jain) wrote…

Might want to call it application/graphqlpm to refer to GraphQL+-.

I think application/graphql is fine, it would be another different to remember if we change it.

[mangalaman93]

Reviewable status: 5 of 14 files reviewed, 41 unresolved discussions (waiting on @danielmai, @gitlw, @golangcibot, @manishrjain, and @martinmr)

---

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

Previously, mangalaman93 (Aman Mangal) wrote…

I think application/graphql is fine, it would be another different to remember if we change it.

Made this change now.