graphql: Support queries from custom HTTP endpoints.

[pawanrawal]

This PR supports types with @not_dgraph directive and allows us to specify the @custom directive for queries.

For types with @not_dgraph directives, queries, mutations and other types are not generated. Queries with @custom directive, are sent to the remote endpoint and the result returned is validated against the specified schema.

Reopening the changes from #4929

---

This change is 

Docs Preview:

[MichaelJCompton]

Looks great. I'm OK to merge this into the custom logic branch so we can move forward with other things. Let's collect together all my comments as things to fix and do that as PRs into the custom logic branch.

I've got a further question about headers / keys. What about a case where the auth between the backend URL and dgraph isn't anything to do with the end user. E.g. I'm writing some API, I have access to some backend on a plan I'm paying for, I have an API key to access that service. So the request between Dgraph and the service has to inject that API KEY, but's it's not coming in via the user's request, it's a fixed value.

This could even happen for example with Auth0, I have an auth0 key for my app, a user request comes in and I want to check something in Auth0 before processing the request ... that'd be my auth0 api key, not anything to do with the end user.

I guess you could do it by deploying a proxy endpoint that has the key embedded in it, but it feels like a real usecase.

Reviewed 15 of 45 files at r1.
Reviewable status: 15 of 45 files reviewed, 19 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

---

graphql/resolve/custom_query_test.yaml, line 33 at r1 (raw file):

      ]
    }
  url: http://myapi.com/favMovies/0x1?name=Michael&num=

In the previous PR, I had been wondering how we should handle the case were an arg is missing. Here's my thought...

Can we tell in myFavoriteMovies the difference between an argument missing vs an argument that's null? E.g. there's three possible states for an argument 1) present with value, present with null, missing.

I'm wondering if we can tell those three states if use that as rules for how to convert.

1. if it's present with a value we copy in the value
2. if it's present but null, we do like in in the test here and leave it as num=
3. if it's not present, we don't put it into the url/body at all

Is that sensible??

---

graphql/resolve/custom_query_test.yaml, line 57 at r1 (raw file):

-
  name: "custom GET query returning users one of which becomes null"

not worth doing in this PR, but I think the right testing approach might be to now pull out the bits that do the GraphQL completion, error checking etc into it's own package and wrap tests around that. Then we only have to test once that all the error checking, nulls, etc work properly. Then if we show that the pipeline dgraph / custom / remote graphql / whatever, pumps its results through there, then we have enough assurance.

otherwise we are gong to have tests like this for everything.

Let's have a quick meeting tonight and either me or someone else can pick this up right now.

---

graphql/resolve/custom_query_test.yaml, line 87 at r1 (raw file):

      ]
    }
  url: http://myapi.com/favMovies/0x1?name=Michael&num=

On top of the point above, I think we need some tests that just show how we build the url / body / (eventually) graphql query.

Shouldn't be part of these tests, just goes through a bunch of possible cases and shows what we build. It'd be too verbose to test those cases here.

---

graphql/resolve/custom_query_test.yaml, line 102 at r1 (raw file):

-
  name: "custom GET query gets URL filled from GraphQL variables"

see this test should be a two liner

input : myFavoriteMovies(id: $id, name: "Michael Compton", num: 10)

and

expected : http://myapi.com/favMovies/0x9?name=Michael+Compton&num=10

---

graphql/resolve/custom_query_test.yaml, line 162 at r1 (raw file):

  variables: { "id": "0x9" }
  httpresponse: |
    {

yeah, just way too much verbose stuff in here, when you are really testing that the building of the requests works. Let's split it up and we'll get more efficient+readable tests.

---

graphql/schema/gqlschema.go, line 74 at r1 (raw file):

input CustomHTTP {
	url: String!
	method: String!

what about body and headers?

---

graphql/schema/gqlschema.go, line 74 at r1 (raw file):

input CustomHTTP {
	url: String!
	method: String!

should method be an enum, then we don't have to check it's values - GraphQL parsing will do that for us

---

graphql/schema/gqlschema.go, line 83 at r1 (raw file):

directive @secret(field: String!, pred: String) on OBJECT | INTERFACE
directive @custom(http: CustomHTTP) on FIELD_DEFINITION
directive @not_dgraph on OBJECT

and interface?

---

graphql/schema/rules.go, line 66 at r1 (raw file):

				custom := fld.Directives.ForName("custom")
				if custom == nil {
					errMesg = "GraphQL Query and Mutation types are only allowed to have fields " +

can we mention the field that caused the error here

---

graphql/schema/rules.go, line 668 at r1 (raw file):

			field.Name)
	}
	if method.Raw != "GET" && method.Raw != "POST" {

this'll go if we us an enum

---

graphql/schema/wrappers.go, line 128 at r1 (raw file):

	QueryType() QueryType
	Rename(newName string)
	HTTPResolver() (HTTPResolverConfig, error)

I didn't understand why this had to happen. Can't we pass the query into the resolver when we make it, and thus not have to expand this interface?

---

graphql/schema/wrappers.go, line 779 at r1 (raw file):

func queryType(name string, custom *ast.Directive) QueryType {
	switch {
	case custom != nil:

feels a bit strange to do this as a null check.

Is it better to refactor the method so it just takes in the defn and it works this out internally?

---

graphql/schema/wrappers.go, line 817 at r1 (raw file):

	// We have to fetch the original definition of the query from the name of the query to be
	// able to get the value stored in custom directive.
	// TODO - This should be cached later.

yeah, let's add an item to do this at preprocessing time, not query time.

Probably we can inject it directly into the resolver factory so all this info gets pushed straight into the resolver.

---

graphql/schema/wrappers.go, line 1297 at r1 (raw file):

// { "author" : "0x3", "post": { "id": "0x9" }}
// If the final result is not a valid JSON, then an error is returned.
func substitueVarsInBody(body string, variables map[string]interface{}) ([]byte, error) {

We can probably also precompute this into a function that mints up the body without the parsing at runtime.

---

graphql/schema/testdata/schemagen/input/custom-nested-types.graphql, line 1 at r1 (raw file):

type Car @not_dgraph {

I'm not sure what this case is testing?

---

graphql/schema/testdata/schemagen/input/custom-query.graphql, line 1 at r1 (raw file):

type User {

can we call this custom-query-with-dgraph-type

---

graphql/schema/testdata/schemagen/input/custom-types.graphql, line 1 at r1 (raw file):

type User @not_dgraph {

custom-query-not-dgraph-type

• add a case that shows what happens when there's a Dgraph type and a not dgraph type in the schema ... something like custom-query-mixed-types

---

graphql/schema/testdata/schemagen/output/custom-query.graphql, line 163 at r1 (raw file):

Quoted 5 lines of code…

type Query {
	getMyFavoriteUsers(id: ID!): [User] @custom(http: {url:"http://my-api.com",method:"GET"})
	getUser(id: ID!): User
	queryUser(filter: UserFilter, order: UserOrder, first: Int, offset: Int): [User]
}

yep, this is exactly what I was expecting.

However, I think we should be stripping @Custom from the generated schema -> having it there exposes backend urls to the API consumers. I'm guessing that has an impact on the implementation, but I think it's important.

---

graphql/schema/dgraph_schemagen_test.yml, line 445 at r1 (raw file):

      }

      type A implements C @not_dgraph {

I'll re-add this point here from the other PR

"Also some strange things that might be worth thinking about

should @Custom be allowed on interfaces? Would that mean that all the implementing objects must be custom
what happens if an object type that implements an interface is marked as @Custom
in the case of @Custom on type's implementing interfaces, what does it mean to query the interface?"

---

Let's pull out a list of things to change. I think it's worth saying that if @not_dgraph is anywhere in an interface hierarchy, it's got to be @not_dgraph throughout the hierarchy.

I think we'll need some other consistency rules as well ... e.g. a Dgraph type shouldn't have fields that point to a @not_dgraph type - how would we resolve that? I guess the only way is if that field is an @Custom. So we'll need to pick through and think of rules like that.

---

I think it's worth adjusting this test though so it doesn't have interfaces, just have two types