Skip to content

Commit

Permalink
fix(GraphQL): multiple queries in a single request should not share t…
Browse files Browse the repository at this point in the history
…he same variables (#5953)

Fixes GraphQL-570.

Earlier, sending a query like this:
```
query ($filter: CountryFilter!){
  qc0: queryCountry(filter: $filter) {
    id
    name
  }
  qc1: queryCountry(filter: $filter) {
    id
    name
  }
}
```
where $filter is a GraphQL variable as below:
```
{
	"filter": {
		"id": [
			"0x0"
		]
	}
}
```
would fetch you results like this:
```
"data": {
    "qc0": [
      {
        "id": "0x2711",
        "name": "India"
      },
      {
        "id": "0x2712",
        "name": "Australia"
      },
      {
        "id": "0x2713",
        "name": "US"
      }
    ],
    "qc1": []
  }
```
While you should have got empty results for both the queries as `0x0` is not a uid that exists.

This PR fixes the above bug by doing a deep-copy of field arguments if variables are provided, so avoiding sharing the same memory address across all the queries in a request. Hence, avoiding any bugs that could be caused as a result of modification of that shared memory address.
  • Loading branch information
abhimanyusinghgaur authored Jul 13, 2020
1 parent ecec071 commit 58e90aa
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 0 deletions.
7 changes: 7 additions & 0 deletions graphql/schema/wrappers.go
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,13 @@ func (f *field) ArgValue(name string) interface{} {
if f.arguments == nil {
// Compute and cache the map first time this function is called for a field.
f.arguments = f.field.ArgumentMap(f.op.vars)
// use a deep-copy only if the request uses variables, as a variable could be shared by
// multiple queries in a single request and internally in our code we may overwrite field
// arguments which may result in the shared value being overwritten for all queries in a
// request.
if f.op.vars != nil {
f.arguments = x.DeepCopyJsonMap(f.arguments)
}
}
return f.arguments[name]
}
Expand Down
44 changes: 44 additions & 0 deletions x/x.go
Original file line number Diff line number Diff line change
Expand Up @@ -998,3 +998,47 @@ func StoreSync(db DB, closer *y.Closer) {
}
}
}

// DeepCopyJsonMap returns a deep copy of the input map `m`.
// `m` is supposed to be a map similar to the ones produced as a result of json unmarshalling. i.e.,
// any value in `m` at any nested level should be of an inbuilt go type.
func DeepCopyJsonMap(m map[string]interface{}) map[string]interface{} {
if m == nil {
return nil
}

mCopy := make(map[string]interface{})
for k, v := range m {
switch val := v.(type) {
case map[string]interface{}:
mCopy[k] = DeepCopyJsonMap(val)
case []interface{}:
mCopy[k] = DeepCopyJsonArray(val)
default:
mCopy[k] = val
}
}
return mCopy
}

// DeepCopyJsonArray returns a deep copy of the input array `a`.
// `a` is supposed to be an array similar to the ones produced as a result of json unmarshalling.
// i.e., any value in `a` at any nested level should be of an inbuilt go type.
func DeepCopyJsonArray(a []interface{}) []interface{} {
if a == nil {
return nil
}

aCopy := make([]interface{}, 0, len(a))
for _, v := range a {
switch val := v.(type) {
case map[string]interface{}:
aCopy = append(aCopy, DeepCopyJsonMap(val))
case []interface{}:
aCopy = append(aCopy, DeepCopyJsonArray(val))
default:
aCopy = append(aCopy, val)
}
}
return aCopy
}

0 comments on commit 58e90aa

Please sign in to comment.