perf(GraphQL): JSON Part-2: JSON is generated by Dgraph for simple mutations (GRAPHQL-877) #7237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into abhimanyu/move-json # Conflicts: # graphql/admin/schema.go
…into abhimanyu/move-json # Conflicts: # edgraph/access_ee.go
…o abhimanyu/graphql-json-mutation
abhimanyu/graphql-json-mutation
…o abhimanyu/graphql-json-mutation # Conflicts: # graphql/schema/errors.go
abhimanyusinghgaur
requested review from
manishrjain,
MichaelJCompton,
pawanrawal and
vvbalaji-dgraph
as code owners
…o abhimanyu/graphql-json-mutation
…into abhimanyu/graphql-json-mutation
pawanrawal
approved these changes
Jan 4, 2021
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @MichaelJCompton, and @vvbalaji-dgraph)
graphql/resolve/mutation.go, line 240 at r1 (raw file):
Data: []byte(`{"` + mutation.ResponseName() + `":null}`), Field: mutation, // there is no completion down the pipeline, so error's path should be prepended with
the error path only contains the query field, so we prepend the mutation response name
graphql/resolve/mutation.go, line 397 at r1 (raw file):
} else { // need to write only the value returned for query field x.Check2(buf.Write(qryResult[4+len(f.ResponseName()) : len(qryResult)-1]))
Add another comment here about the value 4
abhimanyusinghgaur
added a commit
that referenced
this pull request
Jan 5, 2021
abhimanyusinghgaur
added a commit
that referenced
this pull request
Jan 5, 2021
…s (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.
abhimanyusinghgaur
added a commit
that referenced
this pull request
Jan 30, 2021
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
queryFieldexplicitly before executing the mutation, to make it work with the JSON changes.This change is