Initial GraphQL with query and mutation #3558

MichaelJCompton · 2019-06-13T12:09:03Z

(extension of previous closed PR #3483)

This PR adds a basic pipeline around GraphQL. It introduces new commands around dgraph graphql .... Really early days - just setting up some basic workings to get something to build around.

Basic idea is that you'd do:

dgraph graphql init --schema myschema.graphql --alpha 127.0.0.1:9080

and it will translate the GraphQL schema in myschema.graphql into a Dgraph schema and alter the schema at the given alpha and get that Dgraph instance ready to serve a GraphQL API.

2 ) dgraph graphql --alpha 127.0.0.1:9080

that brings a up a HTTP GraphQL API serving the schema from (1).

Thus far, it does parts of (1) and a little of (2).

Simple example ...

If the input schema is

type Message {
  id: ID!
  content: String!
  author: String
}

input MessageInput {
  content: String!
  author: String
}

type AddMessagePayLoad {
  message: Message!
}

type Query {
  getMessage(id: ID!): Message!
}

type Mutation {
  addMessage(input: MessageInput!): AddMessagePayLoad!
}

scalar ID
scalar String

and you run the init command it'll install a Dgraph schema and then running the API, it can mutations like

mutation addMessage($message: MessageInput!) {
  addMessage(input: $message) {
    message {
      content
      author
    }
  }
}

...vars...
{
  "message": {
    "content": "A nice message",
    "author": "An Author"
  }
}

and query

query {
  getMessage(id: "0x2") {
    content
    author
  }
}

This change is

and add simple errors

MichaelJCompton · 2019-06-13T12:17:08Z

@mangalaman93 do you mind having a look, as you looked at the earlier PR.

pawanrawal

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

dgraph/cmd/graphql/resolve/query.go, line 70 at r2 (raw file):

	if err != nil {
		r.WithErrors(gqlerror.Errorf("Failed to query dgraph with error : %s", err))
		glog.Infof("Dgraph query failed : %s", err) // maybe log more info if it could be a bug?

Should we return here?

dgraph/cmd/graphql/resolve/query.go, line 89 at r2 (raw file):

	// need to chop leading '{' and trailing '}' from response
	// is this the safe way?
	r.resp.Data.Write(res[1 : len(res)-1])

Are we sure that res always has length > 0?

dgraph/cmd/graphql/resolve/query.go, line 115 at r2 (raw file):

func (qb *queryBuilder) withIDArgRoot(q schema.Query) {
	if qb == nil || qb.graphQuery == nil || qb.err != nil {

repeated multiple times could be a function.

dgraph/cmd/graphql/resolve/query.go, line 122 at r2 (raw file):

	if err != nil || idArg == nil {
		qb.err = errors.New("ID arg not found (should be impossible in a valid schema)")
	}

return?

dgraph/cmd/graphql/resolve/query.go, line 127 at r2 (raw file):

	if !ok {
		qb.err = errors.New("ID arg not a string (should be impossible in a valid schema)")
	}

return here?

dgraph/cmd/graphql/resolve/query.go, line 139 at r2 (raw file):

func (qb *queryBuilder) withUIDRoot(uid uint64) {
	if qb == nil || qb.graphQuery == nil || qb.err != nil {

We are doing this everywhere, might as well have a function for this.

dgraph/cmd/graphql/resolve/resolver.go, line 102 at r2 (raw file):

// Resolve processes rh.GqlReq and returns a GraphQL response.
// rh.GqlReq should be set with a request before Resolve is called

Should be r.GqlReq.

dgraph/cmd/graphql/schema/request.go, line 38 at r2 (raw file):

// Operation.  If either the request is malformed or doesn't contain a valid
// operation, a GraphQL response containing all errors encountered is returned.
func (s schema) Operation(req *Request) (Operation, *Response) {

*Response return type here is only used for returning errors whereas in its definition it has Data as well. Would returning gqlerror.List make more response to clarify that the second argument only contains errors for the scope of this function?

dgraph/cmd/graphql/schema/request.go, line 43 at r2 (raw file):

	}

	doc, gqlErr := parser.ParseQuery(&ast.Source{Input: req.Query})

Usually, the convention is to just call all error values as err instead of gqlErr, listErr.

dgraph/cmd/graphql/schema/response.go, line 43 at r2 (raw file):

	// see https://graphql.github.io/graphql-spec/June2018/#sec-Response
	//
	Errors gqlerror.List

Don't you need the json tags here? Otherwise, the JSON keys would start with Uppercase whereas the spec says lowercase. (i.e. data, errors and not Data, Errors)

dgraph/cmd/graphql/schema/response.go, line 84 at r2 (raw file):

	if r.Data.Len() > 0 {
		out.WriteString("\"data\": {")
		out.Write(r.Data.Bytes())

What happens when this has a null string?

pawanrawal

I haven't reviewed all the files yet. Will have another look tomorrow and add some more comments.

Reviewable status: 3 of 10 files reviewed, 11 unresolved discussions (waiting on @manishrjain and @MichaelJCompton)

pawanrawal

Reviewed 5 of 10 files at r1.
Reviewable status: 8 of 10 files reviewed, 17 unresolved discussions (waiting on @manishrjain and @MichaelJCompton)

dgraph/cmd/graphql/run.go, line 59 at r2 (raw file):

}

var opt options

Does this need to be a global variable? We should avoid using global variables if possible as its not clear what all functions can/should mutate it.

dgraph/cmd/graphql/run.go, line 146 at r2 (raw file):

	}

	doc, gqlErr := parser.ParseSchema(&ast.Source{Input: string(schemas.Schemas[0].Schema)})

Are we sure we would always be able to access the first element of the array or should we check that first? Also please name errors as err.

dgraph/cmd/graphql/run.go, line 183 at r2 (raw file):

	defer f.Close()

	b, err := ioutil.ReadAll(f)

You could just have used ioutil.ReadFile instead.

dgraph/cmd/graphql/run.go, line 213 at r2 (raw file):

			}

			var ty, preds strings.Builder

What does ty mean? Is this function trying to build dgraph schema from a graphql schema? We should add some tests for that part.

dgraph/cmd/graphql/resolve/dgraph.go, line 33 at r2 (raw file):

func executeQuery(query *gql.GraphQuery, dgraphClient *dgo.Dgraph) ([]byte, error) {

	q := asString(query)

This seems to be a bit of a waste as the server would parse this string again into a gql.GraphQuery. It might make sense to have a gRPC endpoint which could accept a proto def of gql.GraphQuery type.

dgraph/cmd/graphql/resolve/mutation.go, line 154 at r2 (raw file):

	qb.withSelectionSetFrom(f)

	gq, err := qb.query()

Do graphql mutations return the result of the corresponding query?

MichaelJCompton

Reviewable status: 5 of 10 files reviewed, 17 unresolved discussions (waiting on @manishrjain and @pawanrawal)

dgraph/cmd/graphql/run.go, line 59 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Does this need to be a global variable? We should avoid using global variables if possible as its not clear what all functions can/should mutate it.

This is the pattern that's used in all the Dgraph commands.

dgraph/cmd/graphql/run.go, line 146 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are we sure we would always be able to access the first element of the array or should we check that first? Also please name errors as err.

Added test for empty.

As for gqlErr ... unfortunately all those gqlparser functions return types gqlerror.Error and gqlerror.List, not error.

That means we really can't assign them to err because then err would never come out as nil (it would always be an interface with a gqlerror.Error runtime type and nil internal value). That makes error checking yuck and you'd have to treat err differently depending on what runtime value you knew it to have in particular contexts in the code.

So I think the best option is to stray from err and just give the gqlparser functions their own return values - at least that way it's clear in the program text what they are and that they require a little bit of special attention.

dgraph/cmd/graphql/run.go, line 183 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

You could just have used ioutil.ReadFile instead.

Done.

dgraph/cmd/graphql/run.go, line 213 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What does ty mean? Is this function trying to build dgraph schema from a graphql schema? We should add some tests for that part.

Yep, that's just a quick cut of part of what it should do. There's a card on the Asana board to pull it out into it's own package, complete it and wrap tests around it etc ... I think it's ok in this version to have it without tests, it's just needed so we have a place to build from.

dgraph/cmd/graphql/resolve/dgraph.go, line 33 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This seems to be a bit of a waste as the server would parse this string again into a gql.GraphQuery. It might make sense to have a gRPC endpoint which could accept a proto def of gql.GraphQuery type.

Yeah, that would be nice. There's been a few discussions around exactly where the GraphQL layer should sit. ATM, that's settled on it being external to the cluster, but this gql.GraphQuery type seemed like a sensible way of building the queries anyway, so maybe in the end it could be run on an alpha or be able to inject gql.GraphQuery straight in ... though we'd probably have to be careful with that cause I expect that internally Dgraph expects certain consistency properties on gql.GraphQuery, so it might not be ok for just any client to use that.

dgraph/cmd/graphql/resolve/mutation.go, line 154 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Do graphql mutations return the result of the corresponding query?

All GraphQL mutations must have a result type. The effect is that you run the mutation and then run the query represented by the result type.

dgraph/cmd/graphql/resolve/query.go, line 70 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should we return here?

Done.

dgraph/cmd/graphql/resolve/query.go, line 89 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are we sure that res always has length > 0?

Done.

dgraph/cmd/graphql/resolve/query.go, line 115 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

repeated multiple times could be a function.

Done.

dgraph/cmd/graphql/resolve/query.go, line 122 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

return?

Done.

dgraph/cmd/graphql/resolve/query.go, line 127 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

return here?

Done.

dgraph/cmd/graphql/resolve/query.go, line 139 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We are doing this everywhere, might as well have a function for this.

Done.

dgraph/cmd/graphql/resolve/resolver.go, line 102 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should be r.GqlReq.

Done.

dgraph/cmd/graphql/schema/request.go, line 38 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

*Response return type here is only used for returning errors whereas in its definition it has Data as well. Would returning gqlerror.List make more response to clarify that the second argument only contains errors for the scope of this function?

Manish wants me to look at using Dgraph response and errors for GraphQL layer - that'll mean changing them slightly to make them more GraphQL spec compliant. That's on asana board, and when that happens I'll be replumbling how all the errors and responses work. This move to just an error when that happens.

dgraph/cmd/graphql/schema/request.go, line 43 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Usually, the convention is to just call all error values as err instead of gqlErr, listErr.

see other comment on gqlErr

dgraph/cmd/graphql/schema/response.go, line 43 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Don't you need the json tags here? Otherwise, the JSON keys would start with Uppercase whereas the spec says lowercase. (i.e. data, errors and not Data, Errors)

This type is probably going - see comment on Operation() and Response return.

dgraph/cmd/graphql/schema/response.go, line 84 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

What happens when this has a null string?

Not sure what you mean? Data.Bytes has to have something in it.

pawanrawal

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

dgraph/cmd/graphql/resolve/query.go, line 108 at r3 (raw file):

func (qb *queryBuilder) hasErrors() bool {
	if qb == nil || qb.graphQuery == nil || qb.err != nil {

This could be simplified to

return qb == nil || qb.graphQuery == nil || qb.err != nil

dgraph/cmd/graphql/schema/response.go, line 84 at r2 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

Not sure what you mean? Data.Bytes has to have something in it.

In some cases

{
  "data": null
}

is the correct response (when there are errors while parsing/executing the query). The current implementation would put null inside {}?

MichaelJCompton

Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @manishrjain and @pawanrawal)

dgraph/cmd/graphql/resolve/query.go, line 108 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This could be simplified to
return qb == nil || qb.graphQuery == nil || qb.err != nil

Done.

dgraph/cmd/graphql/schema/response.go, line 84 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

In some cases
{
  "data": null
}
is the correct response (when there are errors while parsing/executing the query). The current implementation would put null inside {}?

That case is meant to be handled by https://github.com/dgraph-io/dgraph/blob/9de257f9628b11f1bfcfcb0656a33300805146e2/dgraph/cmd/graphql/schema/response.go#L57 but I see that it would write { data: { null } }.

pawanrawal

Reviewable status: 5 of 10 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)

manishrjain

Got a bunch of comments. Address those and merge.

Reviewed 6 of 10 files at r1, 1 of 2 files at r2, 2 of 3 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @MichaelJCompton and @pawanrawal)

dgraph/cmd/graphql/http.go, line 34 at r4 (raw file):

)

type graphqlHTTPHandler struct {

graphqlHandler

dgraph/cmd/graphql/http.go, line 46 at r4 (raw file):

	w.WriteHeader(http.StatusOK)

	if gh == nil || gh.schema == nil || gh.dgraphClient == nil {

Add a func to check the validity of gh. And just call it.

gh.IsValid() error

No need to log the error.

dgraph/cmd/graphql/http.go, line 47 at r4 (raw file):

	if gh == nil || gh.schema == nil || gh.dgraphClient == nil {
		glog.Error(errors.New("Handler not initialised"))

For requests, you don't need to log errors.

dgraph/cmd/graphql/run.go, line 82 at r4 (raw file):

	GraphQL.EnvPrefix = "DGRAPH_GRAPHQL"

	flags := GraphQL.Cmd.Flags()

PersistentFlags, so the child would also have access automatically.

dgraph/cmd/graphql/run.go, line 104 at r4 (raw file):

		},
	}
	cmdInit.Flags().AddFlag(GraphQL.Cmd.Flag("alpha"))

Then this won't be needed.

dgraph/cmd/graphql/run.go, line 128 at r4 (raw file):

	q := `query {
		graphqlSchemas(func: type(dgraph.graphql.schema)) {

schemas(func: type(dgraph.graphql)) { ... }

dgraph/cmd/graphql/run.go, line 140 at r4 (raw file):

	}

	var schemas graphqlSchemas

gqlSchemas

dgraph/cmd/graphql/run.go, line 150 at r4 (raw file):

	}

	doc, gqlErr := parser.ParseSchema(&ast.Source{Input: string(schemas.Schemas[0].Schema)})

doc, err :=

err can still be nil as returned by ParseSchema.

dgraph/cmd/graphql/run.go, line 155 at r4 (raw file):

	}

	schema, gqlErr := validator.ValidateSchemaDocument(doc)

And here.

dgraph/cmd/graphql/run.go, line 187 at r4 (raw file):

	gqlSchema := string(b)

	doc, gqlErr := parser.ParseSchema(&ast.Source{Input: gqlSchema})

Same here as above with error.

dgraph/cmd/graphql/run.go, line 204 at r4 (raw file):

			if def.Name == "Query" ||
				def.Name == "Mutation" ||
				((strings.HasPrefix(def.Name, "Add") ||

Break it out into another statement assigning this to a variable.

dgraph/cmd/graphql/run.go, line 254 at r4 (raw file):

	glog.V(2).Infof("Built Dgraph schema:\n\n%s\n", dgSchema)

	fmt.Printf("Loading schema into Dgraph at %q\n", opt.alpha)

Tool should always fmt.Print. Server should always glog.

dgraph/cmd/graphql/run.go, line 292 at r4 (raw file):

	mu.SetJson = pb
	_, err = dgraphClient.NewTxn().Mutate(ctx, mu)

if _, err = ... ; err != nil

dgraph/cmd/graphql/run.go, line 304 at r4 (raw file):

func connect() (*dgo.Dgraph, func(), error) {
	var clients []api.DgraphClient
	disconnect := func() {}

no need for this here.

dgraph/cmd/graphql/run.go, line 315 at r4 (raw file):

		conn, err := x.SetupConnection(d, tlsCfg, opt.useCompression)
		if err != nil {
			disconnect()

no need.

dgraph/cmd/graphql/run.go, line 319 at r4 (raw file):

		}
		disconnect = func(dis func()) func() {
			return func() { dis(); conn.Close() }

No need. But, can have a slice which keeps track of all conns.

dgraph/cmd/graphql/run.go, line 324 at r4 (raw file):

		clients = append(clients, api.NewDgraphClient(conn))
	}

disconnect := func() { for _, conn := range conns { conn.Close() } }

dgraph/cmd/graphql/run.go, line 325 at r4 (raw file):

	}

	return dgo.NewDgraphClient(clients...), disconnect, nil

Can have a func on DgraphClient, which would close all connections.

dgraph/cmd/graphql/resolve/dgraph.go, line 34 at r4 (raw file):

	q := asString(query)
	glog.V(2).Infof("Executing Dgraph query: \n%s\n", q)

Maybe in V(3). And even then add an if first.

if glog.V(3) {

}

dgraph/cmd/graphql/resolve/dgraph.go, line 61 at r4 (raw file):

}

func writeQuery(query *gql.GraphQuery, b *strings.Builder, prefix string) {

b, query, prefix

dgraph/cmd/graphql/resolve/dgraph.go, line 83 at r4 (raw file):

}

func writeFunction(f *gql.Function, b *strings.Builder) {

b, f

dgraph/cmd/graphql/resolve/dgraph.go, line 90 at r4 (raw file):

}

func writeFilterFunction(f *gql.Function, b *strings.Builder) {

b, f

dgraph/cmd/graphql/resolve/dgraph.go, line 96 at r4 (raw file):

}

func writeFilter(f *gql.FilterTree, b *strings.Builder) {

b, f

dgraph/cmd/graphql/resolve/mutation.go, line 61 at r4 (raw file):

)

func (r *RequestResolver) resolveMutation(m schema.Mutation) {

This can take a context. As soon as request comes in, create a context and pass it around to all children functions.

dgraph/cmd/graphql/resolve/mutation.go, line 122 at r4 (raw file):

	}

	jsonMut, err := json.Marshal(buildMutationJSON(m, val))

jsonMu

dgraph/cmd/graphql/resolve/mutation.go, line 133 at r4 (raw file):

	}

	ctx := context.Background()

Remove this.

MichaelJCompton

Reviewable status: 4 of 10 files reviewed, 27 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

dgraph/cmd/graphql/http.go, line 34 at r4 (raw file):