feat(Query): Enable persistent queries in dgraph #6788
Conversation
all-seeing-code
requested review from
manishrjain,
MichaelJCompton and
vvbalaji-dgraph
as code owners
github-actions
bot
added
area/graphql
edgraph/graphql.go
Outdated
| if len(query) == 0 { | ||
| return errors.New("PersistedQueryNotFound") | ||
| } | ||
| if !hashMatches(query, sha256Hash) { |
There was a problem hiding this comment.
should we do this at the top of the function? Either way
graphql/schema/request.go
Outdated
| @@ -32,10 +32,18 @@ type Request struct { | |||
| Query string `json:"query"` | |||
| OperationName string `json:"operationName"` | |||
| Variables map[string]interface{} `json:"variables"` | |||
| Extensions struct { | |||
There was a problem hiding this comment.
Leave a comment here, that we should add explicit types for all extensions that we support in dgraph
| @@ -253,6 +259,13 @@ func getRequest(ctx context.Context, r *http.Request) (*schema.Request, error) { | |||
| query := r.URL.Query() | |||
| gqlReq.Query = query.Get("query") | |||
| gqlReq.OperationName = query.Get("operationName") | |||
| if extensions, ok := query["extensions"]; ok { | |||
There was a problem hiding this comment.
check length(extensions) before json decoding
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 8 unresolved discussions (waiting on @anurags92, @gja, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/admin/admin.go, line 291 at r1 (raw file):
getAllowedCORSOrigins: Cors querySchemaHistory(first: Int, offset: Int): [SchemaHistory] queryPersistedQuery: PersistedQuery
I am not sure, but should this query have IP whitelisting and Guardian only access (if ACL is enabled)?
graphql/schema/request.go, line 35 at r1 (raw file):
Previously, gja (Tejas Dinkar) wrote…
Leave a comment here, that we should add explicit types for all extensions that we support in dgraph
This is the first extension that comes in GraphQL request, we already have explicit types for the ones which go in GraphQL response.
But, what we should do is mark this a separate type called RequestExtensions instead of saying struct { ... }
graphql/web/http.go, line 219 at r1 (raw file):
Quoted 9 lines of code…
if err != nil { write(w, schema.ErrorResponse(err), strings.Contains(r.Header.Get("Accept-Encoding"), "gzip")) return } if err = edgraph.ProcessPersistedQuery(ctx, gqlReq); err != nil { write(w, schema.ErrorResponse(err), strings.Contains(r.Header.Get("Accept-Encoding"), "gzip")) return } res = gh.resolver.Resolve(ctx, gqlReq)
I believe we should just extended the if else logic instead of doing multiple returns and code duplication.
schema/schema.go, line 695 at r1 (raw file):
Previously, gja (Tejas Dinkar) wrote…
no need for index here
+1
x/keys.go, line 564 at r1 (raw file):
Previously, gja (Tejas Dinkar) wrote…
this may be too much, check with pawan
I think, this one should be there. There are various checks which happen for these pre-defined things at a lot of places.
Motivation
Enable persistent queries as discussed in RFC on discuss.
This PR fixes: GRAPHQL-721
This feature works out-of-the-box with Apollo client for GraphQL
Components affected by this PR
Query package
Does this PR break backwards compatibility?
No
Testing
Added following tests in:
persistedQueryincommon.go. It is an end-to-end test which checks following cases:provided sha doesn't match the queryPersistedQueryNotFoundstep iiito execute the query and check that it throws no error.step ivvia aGETrequestFixes
Fixes GRAPHQL-721
This change is