perf(GraphQL): JSON Part-7: JSON is generated by Dgraph for custom HTTP fields (GRAPHQL-878) #7361
Conversation
abhimanyusinghgaur
requested review from
manishrjain,
pawanrawal and
vvbalaji-dgraph
as code owners
There was a problem hiding this comment.
Reviewed 16 of 18 files at r1.
Reviewable status: 16 of 18 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)
graphql/e2e/custom_logic/custom_logic_test.go, line 2640 at r1 (raw file):
} queryUserTweetCounts(func: uid(tc), orderdesc: val(tc)) { UserTweetCount.screen_name: User.screen_name
This can be done dynamically so that it's backward compatible for now. Later we can make this easier by doing improvements to custom DQL.
graphql/schema/custom_http.go, line 78 at r1 (raw file):
// MakeAndDecodeHTTPRequest sends an HTTP request using the given url and body and then decodes the // response correctly based on whether it was a GraphQL or REST request. // It returns the decoded response along with one of soft and hard errors.
soft or hard errors
graphql/schema/custom_http.go, line 81 at r1 (raw file):
// Soft error means one can assume that the response is valid and continue the normal execution. // Hard error means there will be no response returned and one must stop the normal execution flow. func (fconf *FieldHTTPConfig) MakeAndDecodeHTTPRequest(client *http.Client, url string,
Can we add a comment that what is considered as a hard error for GraphQL and REST?
graphql/schema/wrappers.go, line 302 at r1 (raw file):
// this flag has already been calculated or not. If not calculated, it would be nil. // Otherwise, it would always contain a boolean value. hasCustomHTTPChild interface{}
We can keep it as *bool
graphql/schema/wrappers.go, line 1209 at r1 (raw file):
// defined in the schema being served. Resolving such a query would report that no // suitable resolver was found. // TODO: Ideally, this case shouldn't happen as the query isn't defined in the schema,
Can we create a ticket mentioning the test case that fails without this and assign it to Jatin?
query/outputnode_graphql.go, line 583 at r1 (raw file):
// For fields without custom directive we recursively call resolveCustomFields and let it do the // work. func (gqlCtx *graphQLEncodingCtx) resolveCustomFields(ctx context.Context, enc *encoder,
I'll have another look at this and leave any comments that I have.
There was a problem hiding this comment.
Reviewed 2 of 18 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Reviewable status: 12 of 29 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)
graphql/e2e/custom_logic/custom_logic_test.go, line 2640 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
This can be done dynamically so that it's backward compatible for now. Later we can make this easier by doing improvements to custom DQL.
Will do this in a later PR, skipping this test for now.
graphql/schema/custom_http.go, line 78 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
soft or hard errors
Done.
graphql/schema/custom_http.go, line 81 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can we add a comment that what is considered as a hard error for GraphQL and REST?
Done.
graphql/schema/wrappers.go, line 302 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
We can keep it as
*bool
Done.
graphql/schema/wrappers.go, line 1209 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Can we create a ticket mentioning the test case that fails without this and assign it to Jatin?
Done.
query/outputnode_graphql.go, line 583 at r1 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
I'll have another look at this and leave any comments that I have.
OK
There was a problem hiding this comment.
Reviewed 17 of 17 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)
…ding (GraphQL-730) (#7371) * perf(GraphQL): JSON Part-1: JSON is generated by Dgraph for simple queries (GRAPHQL-877) (#7230) This PR moves the GraphQL JSON generation for simple queries inside Dgraph. Previously, the GraphQL layer would get a JSON response from Dgraph in the form expected for DQL. That JSON response had to go through the following pipeline: Unmarshal Run GraphQL completion Marshal After this pipeline was executed, then the output JSON would be in the form as expected by GraphQL. Now, the response from Dgraph would already be in the form expected by GraphQL. So, we no more need to run the above pipeline. Hence, saving valuable time and memory. * perf(GraphQL): JSON Part-2: JSON is generated by Dgraph for simple mutations (GRAPHQL-877) (#7237) This PR is a continuation of #7230. It makes standard GraphQL mutations work with the JSON changes. Each mutation now returns the GraphQL JSON required for that mutation as a byte array from the MutationResolver itself, so no further processing of the JSON is required. Delete mutations now query the `queryField` explicitly before executing the mutation, to make it work with the JSON changes. * perf(GraphQL): JSON Part-3: Geo queries (GRAPHQL-879) (#7238) Continued from: * #7230 * #7237 This PR makes Geo queries/mutations work with the new JSON changes. * perf(GraphQL): JSON Part-4: Aggregate queries at root and child levels (GRAPHQL-936) (#7240) Continued from: * #7230 * #7237 * #7238 This PR makes aggregate queries at root and child levels work with the new JSON changes. It removes the default behaviour of rewriting the count field to DQL query everytime (introduced in #7119). Now, rewritten DQL will have count field only if it was requested in GraphQL. * review comments from merged PRs * perf(GraphQL): JSON Part-5: Change query rewriting for scalar/object fields asked multiple times at same level (GRAPHQL-938) (#7283) Previously, for a GraphQL query like this: ``` query { queryThing { i1: id i2: id name n: name n1: name } } ``` GraphQL layer used to generate following DQL query: ``` query { queryThing(func: type(Thing)) { id : uid name : Thing.name } } ``` This can’t work with the new way of JSON encoding for GraphQL because it expects that everything asked for in GraphQL query is present in DQL response. So, Now, the GraphQL layer generates the following DQL to make it work: ``` query { queryThing(func: type(Thing)) { Thing.i1 : uid Thing.i2: uid Thing.name : Thing.name Thing.n : Thing.name Thing.n1 : Thing.name } } ``` Basically, the `DgraphAlias()` is now generated using the `TypeCondition.GraphqlAlias` format. That works even with fragments as they have their specific type conditions. Here are related thoughts: ``` query { queryHuman { name n: name } # DQL: use field.name as DgAlias() # queryHuman(func: type(Human)) { # name: Human.name # name: Human.name # } # => just using field.name doesn't work => use field.alias # queryHuman(func: type(Human)) { # name: Human.name # n: Human.name # } # but there are interfaces and fragments queryCharacter { ... on Human { n: name } ... on Droid { n: bio } } # DQL: use field.alias as DgAlias() # queryCharacter(func: type(Character)) { # n: Character.name # n: Character.bio # } # => just using field.alias doesn't work => use field.name + field.alias # queryCharacter(func: type(Character)) { # name.n: Character.name # bio.n: Character.bio # } # but the implementing types may have fields with same name not inherited from the interface queryThing { ... on ThingOne { c: color } ... on ThingTwo { c: color } } # DQL: use field.name + field.alias as DgAlias() # queryThing(func: type(Thing)) { # color.c: ThingOne.color # color.c: ThingTwo.color # } # => even that doesn't work => use Typename + field.name + field.alias # queryThing(func: type(Thing)) { # ThingOne.color.c: ThingOne.color # ThingTwo.color.c: ThingTwo.color # } # nice! but then different frags explicitly ask for an inherited field with same name & alias queryCharacter { ... on Human { n: name } ... on Droid { n: name } } # DQL: use Typename + field.name + field.alias as DgAlias() # queryCharacter(func: type(Character)) { # Character.name.n: Character.name # Character.name.n: Character.name # } # => doesn't work => use typeCond + Typename + field.name + field.alias # queryCharacter(func: type(Character)) { # Human.Character.name.n: Character.name # Droid.Character.name.n: Character.name # } # but wait, wouldn't just typeCond + field.alias work? # queryCharacter(func: type(Character)) { # Human.n: Character.name # Droid.n: Character.name # } # yeah! that works even for all the above cases. # # OR # # just appending the count when encountering duplicate alias at the same level also works. # But, we need to maintain the count map every time we need DgAlias(). # Also, this requires query pre-processing and the generated aliases are non-deterministic. # # So, we are going with the typeCond + field.alias approach. That will work with @Custom(dql:...) too. } ``` * perf(GraphQL): JSON Part-6: Admin Server and custom HTTP query/mutation are now resolved separately (GRAPHQL-937) (#7317) This PR: * makes the admin server on `/admin` and `@custom(http: {...})` Query/Mutation work through a common pipeline separate from the queries/mutations resolved through Dgraph * adds scalar coercion to queries resolved through Dgraph * and, removes completion as a step from resolvers, as it is not needed anymore. * perf(GraphQL): JSON Part-7: JSON is generated by Dgraph for custom HTTP fields (GRAPHQL-878) (#7361) This PR moves the resolution and GraphQL JSON generation for `@custom(http: {...})` fields inside Dgraph. It also fixes following existing bugs with `@custom` field resolution: * https://discuss.dgraph.io/t/slash-graphql-lambda-bug/12233 (GRAPHQL-967) * race condition in custom logic with repeated fields in implementing types (GRAPHQL-860) * GRAPHQL-639 * perf(GraphQL): JSON part-8: Cleanup and comments (GRAPHQL-947) (#7376) This is a cleanup PR for the JSON changes. It is mostly about nit-picks, making some old tests work, skipping some tests to be ported later to e2e and making the CI green overall. It also fixes a [Discuss Issue](https://discuss.dgraph.io/t/custom-field-resolvers-are-not-always-called/12489) (GRAPHQL-989). * fix merge * review comments * fix tests * fix more tests
This PR moves the resolution and GraphQL JSON generation for
@custom(http: {...})fields inside Dgraph.It also fixes following existing bugs with
@customfield resolution:This change is