feat(GraphQL): Add GraphQL schema validation Endpoint.

[arijitAD]

Fixes: GraphQL-578

Added admin/schema/validate endpoint to validate the GraphQL schema.

---

This change is 

Docs Preview:

[abhimanyusinghgaur]

Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @arijitAD, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 504 at r1 (raw file):

		schema := readRequest(w, r)
		if schema == nil {
			w.WriteHeader(http.StatusOK)

I think sending no input should return a 400 Bad Request, instead of 200 OK.
Or, insteadwe can just let the admin.SchemaValidate call take care of empty input, because internally NewHandler checks that condition and returns a proper error.

---

dgraph/cmd/alpha/run.go, line 515 at r1 (raw file):

			// There could be multiple errors, so replace the newline with whitespace since the newline is an
			// invalid JSON character.
			errStr := strings.ReplaceAll(err.Error(), "\n", " ")

We can make the error as an array of strings in response. So, we split here by \n and send that array as the value of an errors key in the response.

---

dgraph/cmd/alpha/run.go, line 516 at r1 (raw file):

			// invalid JSON character.
			errStr := strings.ReplaceAll(err.Error(), "\n", " ")
			x.Check2(w.Write([]byte(fmt.Sprintf(`{"status":"invalid", "error" : "%s"}`, errStr))))

since status is either valid/invalid, i.e., only boolean state, so instead we can send the response as one of:

{
  "valid": true
}

or

{
  "valid": false,
  "errors": ["err1", "err2", ...]
}

---

graphql/admin/schema.go, line 59 at r1 (raw file):

Quoted 7 lines of code…

schHandler, err := schema.NewHandler(input.Set.Schema, false)
	if err != nil {
		return resolve.EmptyResult(m, err), false
	}
	if _, err = schema.FromString(schHandler.GQLSchema()); err != nil {
		return resolve.EmptyResult(m, err), false
	}

This part only validates the schema, it does not set the in-memory schema here. So, true should be sent.
Instead, we can just useadmin.SchemaValidate(input.Set.Schema) here.

---

graphql/authorization/auth.go, line 60 at r1 (raw file):

	Algo            string
	Audience        []string
	sync.RWMutex

We shouldn't be needing this lock here. There is only two places from where in-memory schema can get updated with wanting to set authMeta. Both of those are in admin.go and they acquire locks there itself. So here we will never receive two concurrent requests to update authMeta.

more related comments on NewHandler...

---

graphql/authorization/auth.go, line 167 at r1 (raw file):

}

func GetAuthMeta() AuthMeta {

we should return a pointer.

---

graphql/authorization/auth.go, line 173 at r1 (raw file):

}

func SetAuthMeta(m AuthMeta) {

we should accept a pointer.

---

graphql/e2e/common/common.go, line 40 at r1 (raw file):

GraphqlURL      = "http://localhost:8180/graphql"
	GraphqlAdminURL = "http://localhost:8180/admin"

We have groupOneServer and groupOneAdminServer in schema_test.go which are same as these.
We can just use them there, instead of exporting these.

---

graphql/e2e/schema/schema_test.go, line 334 at r1 (raw file):

VerifyEmptySchema

should be unexported: verifyEmptySchema

---

graphql/e2e/schema/schema_test.go, line 398 at r1 (raw file):

	for _, tcase := range testCases {
		resp, err := http.Post(validateUrl, "application/json", bytes.NewBuffer([]byte(tcase.schema)))

Although it doesn't matter, but text/plain sounds better here.

---

graphql/schema/schemagen.go, line 209 at r1 (raw file):

	}

	headers := getAllowedHeaders(sch, defns)

we should pass metaInfo to this function, as this function uses authMeta from the authorization package, which won't be set yet.

---

graphql/schema/schemagen.go, line 220 at r1 (raw file):

Quoted 18 lines of code…

handler := &handler{
		input:          input,
		dgraphSchema:   dgSchema,
		completeSchema: sch,
		originalDefs:   defns,
	}
	// Return early since we are only validating the schema.
	if validateOnly {
		return handler, nil
	}
	hc.Lock()
	hc.allowed = headers
	hc.secrets = schemaSecrets
	hc.Unlock()
	if metaInfo != nil {
		authorization.SetAuthMeta(*metaInfo)
	}
	return handler, nil

Since, hc & authMeta is per schema, we should save them as a state variable in Handler and have a method called SetContext() on Handler to set those things once we are sure schema is valid, because the FromString call hasn't happened yet.
So, we should put the call to SetAuthMeta() and setting hc in SetContext() and call SetContex() in resetSchema() in admin.go. That is the only place from where we actually want to update the in-memory schema, and it is already protected by locks from the two places it is called.

We can discuss this.

[arijitAD]

Reviewable status: 4 of 16 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 504 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

I think sending no input should return a 400 Bad Request, instead of 200 OK.
Or, insteadwe can just let the admin.SchemaValidate call take care of empty input, because internally NewHandler checks that condition and returns a proper error.

Done.

---

dgraph/cmd/alpha/run.go, line 515 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

We can make the error as an array of strings in response. So, we split here by \n and send that array as the value of an errors key in the response.

Done.

---

dgraph/cmd/alpha/run.go, line 516 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

since status is either valid/invalid, i.e., only boolean state, so instead we can send the response as one of:

{
  "valid": true
}

or

{
  "valid": false,
  "errors": ["err1", "err2", ...]
}

Done.

---

graphql/admin/schema.go, line 59 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

schHandler, err := schema.NewHandler(input.Set.Schema, false)
	if err != nil {
		return resolve.EmptyResult(m, err), false
	}
	if _, err = schema.FromString(schHandler.GQLSchema()); err != nil {
		return resolve.EmptyResult(m, err), false
	}

This part only validates the schema, it does not set the in-memory schema here. So, true should be sent.
Instead, we can just useadmin.SchemaValidate(input.Set.Schema) here.

Done.

---

graphql/authorization/auth.go, line 60 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

We shouldn't be needing this lock here. There is only two places from where in-memory schema can get updated with wanting to set authMeta. Both of those are in admin.go and they acquire locks there itself. So here we will never receive two concurrent requests to update authMeta.

more related comments on NewHandler...

This can be accessed from multiple places hence lock is required.

---

graphql/authorization/auth.go, line 167 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should return a pointer.

Done.

---

graphql/authorization/auth.go, line 173 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should accept a pointer.

Done.

---

graphql/e2e/common/common.go, line 40 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

GraphqlURL      = "http://localhost:8180/graphql"
	GraphqlAdminURL = "http://localhost:8180/admin"

We have groupOneServer and groupOneAdminServer in schema_test.go which are same as these.
We can just use them there, instead of exporting these.

Done.

---

graphql/e2e/schema/schema_test.go, line 334 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

VerifyEmptySchema

should be unexported: verifyEmptySchema

Done.

---

graphql/e2e/schema/schema_test.go, line 398 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

Although it doesn't matter, but text/plain sounds better here.

Done.

---

graphql/schema/schemagen.go, line 209 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should pass metaInfo to this function, as this function uses authMeta from the authorization package, which won't be set yet.

Done.

---

graphql/schema/schemagen.go, line 220 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

handler := &handler{
		input:          input,
		dgraphSchema:   dgSchema,
		completeSchema: sch,
		originalDefs:   defns,
	}
	// Return early since we are only validating the schema.
	if validateOnly {
		return handler, nil
	}
	hc.Lock()
	hc.allowed = headers
	hc.secrets = schemaSecrets
	hc.Unlock()
	if metaInfo != nil {
		authorization.SetAuthMeta(*metaInfo)
	}
	return handler, nil

Since, hc & authMeta is per schema, we should save them as a state variable in Handler and have a method called SetContext() on Handler to set those things once we are sure schema is valid, because the FromString call hasn't happened yet.
So, we should put the call to SetAuthMeta() and setting hc in SetContext() and call SetContex() in resetSchema() in admin.go. That is the only place from where we actually want to update the in-memory schema, and it is already protected by locks from the two places it is called.

We can discuss this.

The schema update is a two-step process.
We first validate it and then update the auth info when we receive an update from the badger. Hence if there is an error we will get it in the validation phase.

[abhimanyusinghgaur]

Reviewed 13 of 17 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 509 at r2 (raw file):

"true"

a boolean true not a string "true"

---

dgraph/cmd/alpha/run.go, line 519 at r2 (raw file):

"false"

-> false

---

graphql/admin/admin.go, line 518 at r1 (raw file):

server.mux.Lock()
		defer server.mux.Unlock()

no need to move these lines up now, because we agreed on having locks inside for the authMeta and hc.

---

graphql/e2e/common/fragment.go, line 37 at r1 (raw file):

fmt.Sprintf(

When you merge master, you will get a merge conflict here because I already removed this DeepSource warning :P

---

graphql/e2e/schema/schema_test.go, line 410 at r2 (raw file):

"true"

-> true

---

graphql/e2e/schema/schema_test.go, line 414 at r2 (raw file):

"false"

-> false

---

graphql/schema/wrappers.go, line 224 at r2 (raw file):

Quoted 4 lines of code…

//
	authMeta *authorization.AuthMeta
	//
	hc *headersConfig

stale code from discussion

[arijitAD]

Reviewable status: 10 of 14 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 509 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"true"

a boolean true not a string "true"

Done.

---

dgraph/cmd/alpha/run.go, line 519 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"false"

-> false

Done.

---

graphql/admin/admin.go, line 518 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

server.mux.Lock()
		defer server.mux.Unlock()

no need to move these lines up now, because we agreed on having locks inside for the authMeta and hc.

Done.

---

graphql/e2e/schema/schema_test.go, line 410 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"true"

-> true

Done.

---

graphql/e2e/schema/schema_test.go, line 414 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

"false"

-> false

Done.

---

graphql/schema/wrappers.go, line 224 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

//
	authMeta *authorization.AuthMeta
	//
	hc *headersConfig

stale code from discussion

Done.

[abhimanyusinghgaur]

Reviewed 8 of 12 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

[pawanrawal]

Reviewed 3 of 18 files at r1, 4 of 17 files at r2, 4 of 12 files at r3, 3 of 7 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arijitAD, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 524 at r4 (raw file):

		}

		w.WriteHeader(http.StatusBadRequest)

There is x.SetStatusWithData and x.SetStatus which already does quite a bit of this. Can you check if you could use that here? For success, we send code as success usually.

---

graphql/admin/admin.go, line 369 at r4 (raw file):

	}

	if _, err := schema.FromString(schHandler.GQLSchema()); err != nil {

can just be `return schema.From....)

---

graphql/admin/schema.go, line 52 at r4 (raw file):

	}

	// We just need to validate the schema. Schema is later set in `resetSchema()` when the schema

Hope no tests are failing after this because even on this node the schema would be applied after some time now. Do we need to change the flag for this endpoint?

[arijitAD]

Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 524 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

There is x.SetStatusWithData and x.SetStatus which already does quite a bit of this. Can you check if you could use that here? For success, we send code as success usually.

Done.

---

graphql/admin/admin.go, line 369 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

can just be `return schema.From....)

Done.

---

graphql/admin/schema.go, line 52 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Hope no tests are failing after this because even on this node the schema would be applied after some time now. Do we need to change the flag for this endpoint?

Yes. Otherwise, we would end up updating the auth meta.

[pawanrawal]

Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @arijitAD, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 519 at r5 (raw file):

		if err == nil {
			w.WriteHeader(http.StatusOK)
			x.SetStatus(w, "success", "msg")

So the third argument is supposed to be the message that you want to send. It could be Schema is valid or something.

---

x/x.go, line 329 at r5 (raw file):

}

func SetStatusWithErrors(w http.ResponseWriter, code string, errs []string) {

strange there wasn't already something like this there

[arijitAD]

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)

---

dgraph/cmd/alpha/run.go, line 519 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

So the third argument is supposed to be the message that you want to send. It could be Schema is valid or something.

Done.

---

x/x.go, line 329 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

strange there wasn't already something like this there

Done.