fix(GraphQL): fix order of entities query result #7542

minhaj-shakeel · 2021-03-10T11:57:37Z

Fixes GRAPHQL-1083.

This change is

abhimanyusinghgaur

Reviewed 4 of 4 files at r1, 1 of 2 files at r2.
Reviewable status: 4 of 6 files reviewed, 12 unresolved discussions (waiting on @abhimanyusinghgaur, @minhaj-shakeel, and @pawanrawal)

graphql/resolve/query.go, line 59 at r1 (raw file):

// 1) rewrite the query using qr (return error if failed)
// 2) execute the rewritten query with ex (return error if failed)
func NewQueryResolver(qr QueryRewriter, ex DgraphExecutor) QueryResolver {

add a comment about the 3rd step of completion too from previous code

graphql/resolve/query.go, line 64 at r1 (raw file):

// NewEntitiesQueryResolver ...
func NewEntitiesQueryResolver(qr QueryRewriter, ex DgraphExecutor) QueryResolver {

proper comment

graphql/resolve/query_rewriter.go, line 164 at r1 (raw file):

string, string, bool, []interface{}

make a struct and put these in that struct. Not a good practice to return so many results from a func.
The signature should be like:

type parsedRepresentations struct {
 ...
}

func parseRepresentationsArgument(field schema.Query) (parsedRepresentations, error) { ... }

graphql/resolve/query_rewriter.go, line 288 at r1 (raw file):

Quoted 4 lines of code…

	// Add the  ascending Order of the keyField in the query.
	// The result will be converted into the exact in the resultCompletion step.
	dgQuery.Order = append(dgQuery.Order,
		&pb.Order{Attr: typeDefn.DgraphPredicate(keyFieldName)})

we need to do this only when the key field is @id and not for ID. For, ID you won't get a DgraphPredicate as that maps to uid and also that for uid the result is by default sorted in the order of uid, so no need to sort for them.

So, put this in the else case above.

graphql/resolve/query_test.yaml, line 3301 at r2 (raw file):

 orderasc: Astronaut.id

so this should be removed as there is nothing called Astronaut.id stored in dgraph, it is only uid

graphql/resolve/resolver.go, line 88 at r1 (raw file):

}

// A ResultCompleter ....

add in the full comments from the previous code

graphql/resolve/resolver.go, line 328 at r1 (raw file):

entitiesCompletion

entitiesQueryCompletion

as it is only for queries

graphql/resolve/resolver.go, line 329 at r1 (raw file):

id

if

graphql/resolve/resolver.go, line 349 at r1 (raw file):

map[interface{}]int

add a comment about why the key in this map is an interface? (as there are multiple types like String, Int, Int64 allowed as @id)

graphql/resolve/resolver.go, line 356 at r1 (raw file):

.(string)

not necessary that they are string. Use a type switch and do the comparison accordingly.
Possible types are:

string
int/json.Number (one of these)
float/json.Number (one of these)

We need to check json.Number case.

Also, add one e2e test case for the case of @id with Int in entities query.

graphql/resolve/resolver.go, line 362 at r1 (raw file):

[]interface{}(data["_entities"])

let's save this bit in a variable entitiesQryResp prior to the for loop and use that.
also, can there be a case that data for a particular key was missing in dgraph, so that dgraph returns null for that index?
That should be handled by this right?
we should add this scenario in the e2e as well.

graphql/resolve/resolver.go, line 378 at r1 (raw file):

func entitiesQueryCompletion() CompletionFunc {
	return entitiesCompletion
}

not required

abhimanyusinghgaur

Reviewed 1 of 2 files at r2, 6 of 7 files at r3.
Reviewable status: 8 of 9 files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur, @minhaj-shakeel, and @pawanrawal)

graphql/resolve/resolver.go, line 350 at r3 (raw file):

key in the map as

key in the map is of type interface because

graphql/resolve/resolver.go, line 367 at r3 (raw file):

switch uniqueKeyList[i].(type)

switch val := uniqueKeyList[i].(type)

Then you can just use val in each case without having to explicitly cast it.

The order of cases can be optimized to:

string
json.Number
int64
float64

graphql/resolve/resolver.go, line 375 at r3 (raw file):

 uniqueKeyList[i].(json.Number) < uniqueKeyList[j].(json.Number)

json.Number is basically a string.
Doing this would end up in a lexicographic comparison, resulting in
[12, 100, 1] being sorted as [1, 100, 12]. Which is not what we want.

So, we should use val.Int64() or val.Float64() depending on the type of the key field, and then compare.

minhaj-shakeel

Reviewable status: 7 of 11 files reviewed, 14 unresolved discussions (waiting on @abhimanyusinghgaur, @minhaj-shakeel, and @pawanrawal)

graphql/resolve/query.go, line 64 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

proper comment

Done.

graphql/resolve/query_rewriter.go, line 164 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

string, string, bool, []interface{}
make a struct and put these in that struct. Not a good practice to return so many results from a func.
The signature should be like:
type parsedRepresentations struct {
 ...
}

func parseRepresentationsArgument(field schema.Query) (parsedRepresentations, error) { ... }

Done.

graphql/resolve/query_rewriter.go, line 288 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	// Add the  ascending Order of the keyField in the query.
	// The result will be converted into the exact in the resultCompletion step.
	dgQuery.Order = append(dgQuery.Order,
		&pb.Order{Attr: typeDefn.DgraphPredicate(keyFieldName)})
we need to do this only when the key field is @id and not for ID. For, ID you won't get a DgraphPredicate as that maps to uid and also that for uid the result is by default sorted in the order of uid, so no need to sort for them.

So, put this in the else case above.

Done.

graphql/resolve/resolver.go, line 88 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

add in the full comments from the previous code

Done.

graphql/resolve/resolver.go, line 328 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

entitiesCompletion
entitiesQueryCompletion
as it is only for queries

Done.

graphql/resolve/resolver.go, line 329 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

id

if

Done.

graphql/resolve/resolver.go, line 349 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

map[interface{}]int
add a comment about why the key in this map is an interface? (as there are multiple types like String, Int, Int64 allowed as @id)

Done.

graphql/resolve/resolver.go, line 356 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

.(string)
not necessary that they are string. Use a type switch and do the comparison accordingly.
Possible types are:

string

int/json.Number (one of these)

float/json.Number (one of these)

We need to check json.Number case.

Also, add one e2e test case for the case of @id with Int in entities query.

Done.

graphql/resolve/resolver.go, line 362 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

[]interface{}(data["_entities"])
let's save this bit in a variable entitiesQryResp prior to the for loop and use that.
also, can there be a case that data for a particular key was missing in dgraph, so that dgraph returns null for that index?
That should be handled by this right?
we should add this scenario in the e2e as well.

Done.

graphql/resolve/resolver.go, line 378 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

func entitiesQueryCompletion() CompletionFunc {
	return entitiesCompletion
}

not required

Done.

graphql/resolve/resolver.go, line 350 at r3 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

key in the map as 
key in the map is of type interface because

Done.

graphql/resolve/resolver.go, line 367 at r3 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

switch uniqueKeyList[i].(type)
switch val := uniqueKeyList[i].(type)

Then you can just use val in each case without having to explicitly cast it.

The order of cases can be optimized to:

string

json.Number

int64

float64

Even if I use the val, the second value will be typecasted explicitly. Can we do it in a single statement for both of the values?

graphql/resolve/resolver.go, line 375 at r3 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

 uniqueKeyList[i].(json.Number) < uniqueKeyList[j].(json.Number)
json.Number is basically a string.
Doing this would end up in a lexicographic comparison, resulting in
[12, 100, 1] being sorted as [1, 100, 12]. Which is not what we want.

So, we should use val.Int64() or val.Float64() depending on the type of the key field, and then compare.

Done.

abhimanyusinghgaur

Just got one more comment, and then good to go.

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @minhaj-shakeel and @pawanrawal)

graphql/resolve/resolver.go, line 375 at r4 (raw file):

case "Int"

case "Int","Int64":

minhaj-shakeel

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @abhimanyusinghgaur and @pawanrawal)

graphql/resolve/resolver.go, line 375 at r4 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

case "Int"
case "Int","Int64":

Done.

fix order of entities query result

4d3f8ab

minhaj-shakeel requested a review from pawanrawal as a code owner March 10, 2021 11:57

github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 10, 2021

minhaj-shakeel added 5 commits March 10, 2021 17:29

remove stale func

1a4f567

add test case

634cb65

fix delete in e2e test

0a7e9f9

fix old entities query rewriting test

9b7d6ba

fix deletion

83fe1bf

abhimanyusinghgaur suggested changes Mar 10, 2021

View reviewed changes

minhaj-shakeel added 9 commits March 10, 2021 20:41

address commebts

af0adef

fix JSON eq

e959315

fix JSON eq

4ea6765

handle duplicate keys

5bae825

add duplicate keys

304d35b

add test case for entities query with keyfield of type Int

c9448db

fix test case for int

ee3e899

fix test case for int

5953106

fix response test

3cf3dab

abhimanyusinghgaur suggested changes Mar 11, 2021

View reviewed changes

minhaj-shakeel added 3 commits March 11, 2021 20:11

fix directive e2e test

91a4d8f

fix comparison for json.Number

7946282

fix tokenizer

8d84991

minhaj-shakeel commented Mar 11, 2021

View reviewed changes

optimize explicit typecasting

35465ac

abhimanyusinghgaur approved these changes Mar 12, 2021

View reviewed changes

address comment

2da1d8a

minhaj-shakeel commented Mar 12, 2021

View reviewed changes

minhaj-shakeel added 2 commits March 12, 2021 11:56

Merge branch 'release/v21.03' into minhaj/fix-order

da2319d

fix auth test

76dd734

minhaj-shakeel merged commit f8a4d53 into release/v21.03 Mar 12, 2021

minhaj-shakeel deleted the minhaj/fix-order branch March 12, 2021 06:53

aman-bansal pushed a commit that referenced this pull request Mar 15, 2021

fix(GraphQL): fix order of entities query result (#7542)

cc7cb78

tomhollingworth mentioned this pull request Jan 27, 2022

[Bug] Entities Response for Apollo Federation is out of order #8120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(GraphQL): fix order of entities query result #7542

fix(GraphQL): fix order of entities query result #7542

minhaj-shakeel commented Mar 10, 2021 •

edited by manishrjain

Loading

abhimanyusinghgaur left a comment

abhimanyusinghgaur left a comment

minhaj-shakeel left a comment

abhimanyusinghgaur left a comment

minhaj-shakeel left a comment

fix(GraphQL): fix order of entities query result #7542

fix(GraphQL): fix order of entities query result #7542

Conversation

minhaj-shakeel commented Mar 10, 2021 • edited by manishrjain Loading

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

minhaj-shakeel left a comment

Choose a reason for hiding this comment

abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

minhaj-shakeel left a comment

Choose a reason for hiding this comment

minhaj-shakeel commented Mar 10, 2021 •

edited by manishrjain

Loading