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

GraphQL endpoints accept application/graphql content type #5125

Merged
merged 5 commits into from
Apr 14, 2020

Conversation

gja
Copy link
Contributor

@gja gja commented Apr 7, 2020

Description.

/graphql and /admin accepts "Content-Type: application/graphql "

NOTE: https://graphql.org/learn/serving-over-http/#post-request mentions that application/graphql should be treated as a graphql query the entire body being the query.

Changelog tags.

  • Added support for application/graphql to graphql endpoints

Please indicate if documentation needs to be updated.

Getting started examples should use this instead of the json endpoints


This change is Reviewable

Docs Preview: Dgraph Preview

@gja gja requested review from manishrjain and a team as code owners April 7, 2020 07:15
default:
// https://graphql.org/learn/serving-over-http/#post-request says:
// "A standard GraphQL POST request should use the application/json
// content type ..."
return nil, errors.New(
"Unrecognised Content-Type. Please use application/json for GraphQL requests")
"Unrecognised Content-Type. Please use application/json or application/graphql for GraphQL requests")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is 106 characters (from lll)

Copy link
Contributor

@MichelDiz MichelDiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Tested locally with

curl --request POST \
  --url http://localhost:8080/admin \
  --header 'content-type: application/graphql' \
  --data-binary '@./graphql/fullbackup.graphql'

works fine.

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

@@ -27,7 +27,7 @@ function restartCluster {
docker_compose_gopath=`pwd`/../osx-docker-gopath

# FIXME: read the go version from a constant
docker run -it --rm \
docker run --rm \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed -it. I think in a previous commit i put -it, though this isn't interactive

@@ -442,6 +443,17 @@ func (params *GraphQLParams) ExecuteAsPost(t *testing.T, url string) *GraphQLRes
return params.Execute(t, req)
}

// ExecuteAsPostApplicationGraphql builds an HTTP Post with type application/graphql
// Note, variables are not allowed
func (params *GraphQLParams) ExecuteAsPostApplicationGraphql(t *testing.T, url string) *GraphQLResponse {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is 105 characters (from lll)

return nil, err
}

func (params *GraphQLParams) buildPostRequest(url string, body []byte, contentType string) (*http.Request, error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line is 115 characters (from lll)

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gja, @manishrjain, and @MichaelJCompton)


graphql/web/http.go, line 200 at r2 (raw file):

				return nil, errors.Wrap(err, "Could not read GraphQL request body")
			}
			gqlReq.Query = string(string(bytes))

Probably a typo, just need to have one string here.

Copy link
Contributor Author

@gja gja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


graphql/web/http.go, line 200 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Probably a typo, just need to have one string here.

Done.

@gja gja merged commit 659dcaf into master Apr 14, 2020
@gja gja deleted the tejas/accept-application/graphql-to-graphql-endpoint branch April 14, 2020 11:06
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.

4 participants