fix(GraphQL): Fix mutation with Int Xid variables. #7565

minhaj-shakeel · 2021-03-15T06:35:55Z

Fixes GRAPHQL-1094.

This change is

abhimanyusinghgaur

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)

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

$bookId

so this adds the test for variable but removes the case for hardcoded value.
Let's not do this.

Let's keep the existing one, but just add one more book in the same mutation which uses a variable.

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

$chId

same comment as the above test

graphql/resolve/mutation_rewriter.go, line 1758 at r1 (raw file):

Quoted 60 lines of code…

					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Int64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					default:
						val, ok := xidVal.(int64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int but data type in schema is Int", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					}
				case "Int64":
					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Int64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					case int64:
						val, ok := xidVal.(int64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int64 but data type in schema is Int64", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					default:
						xidString, ok = xidVal.(string)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int64", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
					}
				case "Float":
					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Float64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatFloat(val, 'f', -1, 64)
					default:
						val, ok := xidVal.(float64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Float but data type in schema is Float", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatFloat(val, 'f', -1, 64)
					}

so this block of code seems repetitive.
So, put this in a function

func extractVal(xidVal interface{}, typename string) (string, error)

and use at both the places.

abhimanyusinghgaur

This should go in release/v21.03 not master.

vmrajas · 2021-03-15T11:55:14Z

graphql/resolve/mutation_rewriter.go

@@ -1383,11 +1383,34 @@ func rewriteObject(
 				// TODO: Add a function for parsing idVal. This is repeatitive


You may remove this TODO after addressing Abhimanyu's comment on writing extractVal function.

vmrajas · 2021-03-15T11:56:16Z

graphql/resolve/mutation_rewriter.go

+					case json.Number:
+						val, _ := xidVal.(json.Number).Int64()
+						xidString = strconv.FormatInt(val, 10)
+					case int64:


This part is different for int64 than for int or float. Add a comment here on why that is so and the fact that default handles the case for string.

abhimanyusinghgaur

We should close this PR, and raise a new one against the release/v21.03 branch with the changes in this.

Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @manishrjain, @martinmr, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)

graphql/resolve/mutation_rewriter.go, line 2264 at r2 (raw file):

switch xidVal.(type)

switch xVal := xidVal.(type)

And then use xVal, no need to again cast it, if we are already using a type switch.
Also, as we are using a type switch, we can just have explicit cases for json.Number and int64, then the default case should return errors only.
At present, the default case handles the int64 case too, which is a bit ugly.

Same comments for other Int64 and Float cases.

graphql/resolve/mutation_rewriter.go, line 2288 at r2 (raw file):

.(int64)

this shouldn't be required after addressing the above comments, as we are already in the case for int64.

graphql/resolve/mutation_rewriter.go, line 2294 at r2 (raw file):

 as per spec

remove this bit.
It is not as per spec, it is just what we allow for convenience to clients.

graphql/resolve/mutation_rewriter.go, line 2295 at r2 (raw file):

id

typo here?

graphql/resolve/mutation_rewriter.go, line 2321 at r2 (raw file):

Quoted 7 lines of code…

	default:
		xidString, ok := xidVal.(string)
		if !ok {
			return "", errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
				"a String", xidName, typeName))
		}
		return xidString, nil

we should explicitly have a case "String" that handles this.
The default case then can just always return an error that the type is not allowed as xid.

minhaj-shakeel

Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @martinmr, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

$bookId
so this adds the test for variable but removes the case for hardcoded value.
Let's not do this.

Let's keep the existing one, but just add one more book in the same mutation which uses a variable.

Done.

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

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

$chId
same comment as the above test

Done.

graphql/resolve/mutation_rewriter.go, line 1383 at r1 (raw file):

Previously, vmrajas wrote…

You may remove this TODO after addressing Abhimanyu's comment on writing extractVal function.

Done.

graphql/resolve/mutation_rewriter.go, line 1399 at r1 (raw file):

Previously, vmrajas wrote…

This part is different for int64 than for int or float. Add a comment here on why that is so and the fact that default handles the case for string.

Done.

graphql/resolve/mutation_rewriter.go, line 1758 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Int64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					default:
						val, ok := xidVal.(int64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int but data type in schema is Int", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					}
				case "Int64":
					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Int64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					case int64:
						val, ok := xidVal.(int64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int64 but data type in schema is Int64", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatInt(val, 10)
					default:
						xidString, ok = xidVal.(string)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Int64", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
					}
				case "Float":
					switch xidVal.(type) {
					case json.Number:
						val, err := xidVal.(json.Number).Float64()
						if err != nil {
							retErrors = append(retErrors, err)
							return nil, retErrors
						}
						xidString = strconv.FormatFloat(val, 'f', -1, 64)
					default:
						val, ok := xidVal.(float64)
						if !ok {
							retErrors = append(retErrors, errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
								"a Float but data type in schema is Float", xid.Name(), xid.Type().Name())))
							return nil, retErrors
						}
						xidString = strconv.FormatFloat(val, 'f', -1, 64)
					}

so this block of code seems repetitive.
So, put this in a function

func extractVal(xidVal interface{}, typename string) (string, error)

and use at both the places.

Done.

graphql/resolve/mutation_rewriter.go, line 2264 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

switch xidVal.(type)
switch xVal := xidVal.(type)
And then use xVal, no need to again cast it, if we are already using a type switch.
Also, as we are using a type switch, we can just have explicit cases for json.Number and int64, then the default case should return errors only.
At present, the default case handles the int64 case too, which is a bit ugly.

Same comments for other Int64 and Float cases.

Done.

graphql/resolve/mutation_rewriter.go, line 2288 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

.(int64)
this shouldn't be required after addressing the above comments, as we are already in the case for int64.

Done.

graphql/resolve/mutation_rewriter.go, line 2294 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

 as per spec
remove this bit.
It is not as per spec, it is just what we allow for convenience to clients.

Done.

graphql/resolve/mutation_rewriter.go, line 2295 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

 id
typo here?

Done.

graphql/resolve/mutation_rewriter.go, line 2321 at r2 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

	default:
		xidString, ok := xidVal.(string)
		if !ok {
			return "", errors.New(fmt.Sprintf("encountered an XID %s with %s that isn't "+
				"a String", xidName, typeName))
		}
		return xidString, nil
we should explicitly have a case "String" that handles this.
The default case then can just always return an error that the type is not allowed as xid.

Done.

abhimanyusinghgaur

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @martinmr, @pawanrawal, @vmrajas, and @vvbalaji-dgraph)

(cherry picked from commit eb6e27b)

minhaj-shakeel requested review from manishrjain, martinmr, pawanrawal and vvbalaji-dgraph as code owners March 15, 2021 06:35

fix mutation with Xid Int variables

7f332df

minhaj-shakeel changed the base branch from release/v21.03 to master March 15, 2021 06:44

minhaj-shakeel added 5 commits March 15, 2021 12:25

fix e2e test

bc1d252

remove int64

c5b469b

handle int64

58e7faf

modify e2e test

36f03b7

fix e2e test

4607fad

abhimanyusinghgaur suggested changes Mar 15, 2021

View reviewed changes

abhimanyusinghgaur reviewed Mar 15, 2021

View reviewed changes

vmrajas reviewed Mar 15, 2021

View reviewed changes

abhimanyusinghgaur mentioned this pull request Mar 15, 2021

Docs (GraphQL): 21.03 feature documentation for federation dgraph-io/dgraph-docs#104

Merged

minhaj-shakeel added 5 commits March 15, 2021 19:22

restore old cases

7f2c240

fix test

a0cb230

create function for extractVal

ed3d0fb

add book

36b4134

add variable

f287df5

abhimanyusinghgaur suggested changes Mar 16, 2021

View reviewed changes

minhaj-shakeel added 2 commits March 16, 2021 16:03

address Comments

344bdcc

handled ID also

a8e475f

minhaj-shakeel commented Mar 16, 2021

View reviewed changes

address more comments

a4e561a

abhimanyusinghgaur approved these changes Mar 16, 2021

View reviewed changes

minhaj-shakeel merged commit eb6e27b into master Mar 16, 2021

minhaj-shakeel deleted the minhaj/mutation-variables branch March 16, 2021 11:51

minhaj-shakeel added a commit that referenced this pull request Mar 16, 2021

fix(GraphQL): Fix mutation with Int Xid variables. (#7565)

966a626

(cherry picked from commit eb6e27b)

minhaj-shakeel added a commit that referenced this pull request Mar 16, 2021

fix(GraphQL): Fix mutation with Int Xid variables. (#7565) (#7588)

731f97b

(cherry picked from commit eb6e27b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(GraphQL): Fix mutation with Int Xid variables. #7565

fix(GraphQL): Fix mutation with Int Xid variables. #7565

minhaj-shakeel commented Mar 15, 2021 •

edited by manishrjain

Loading

abhimanyusinghgaur left a comment

abhimanyusinghgaur left a comment

vmrajas Mar 15, 2021

vmrajas Mar 15, 2021

abhimanyusinghgaur left a comment

minhaj-shakeel left a comment

abhimanyusinghgaur left a comment

		@@ -1383,11 +1383,34 @@ func rewriteObject(
		// TODO: Add a function for parsing idVal. This is repeatitive

fix(GraphQL): Fix mutation with Int Xid variables. #7565

fix(GraphQL): Fix mutation with Int Xid variables. #7565

Conversation

minhaj-shakeel commented Mar 15, 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

vmrajas Mar 15, 2021

Choose a reason for hiding this comment

vmrajas Mar 15, 2021

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 commented Mar 15, 2021 •

edited by manishrjain

Loading