Skip to content

Commit

Permalink
fix(GraphQL): Dgraph directive with reverse edge should work smoothly…
Browse files Browse the repository at this point in the history
… with interfaces (#5911)

Fixes #5744

There was another bug where field which map to reverse edges in Dgraph where generated as part of AddTypeInput and TypeRef. They have been excluded now as we can't perform a mutation along them. They would earlier have given an error at runtime while doing mutations in Dgraph about predicate being incorrect because it starts with ~. Apart, from that to fix #5744, we also allow interfaces implemented by the current type in the che
  • Loading branch information
pawanrawal authored Jul 13, 2020
1 parent 144685a commit ecec071
Show file tree
Hide file tree
Showing 9 changed files with 447 additions and 13 deletions.
7 changes: 7 additions & 0 deletions graphql/schema/gqlschema.go
Original file line number Diff line number Diff line change
Expand Up @@ -1308,6 +1308,13 @@ func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition) ast.FieldL
continue
}

// Remove edges which have a reverse predicate as they should only be updated through their
// forward edge.
fname := fieldName(fld, defn.Name)
if strings.HasPrefix(fname, "~") || strings.HasPrefix(fname, "<~") {
continue
}

// see also comment in getNonIDFields
if schema.Types[fld.Type.Name()].Kind == ast.Interface &&
(!hasID(schema.Types[fld.Type.Name()]) && !hasXID(schema.Types[fld.Type.Name()])) {
Expand Down
73 changes: 73 additions & 0 deletions graphql/schema/gqlschema_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,56 @@ invalid_schemas:
"locations":[{"line":5, "column":12}]}
]

-
name: "Dgraph directive with reverse pred argument matching with wrong type implementing an interface produces an error"
input: |
type Z {
f1: String!
}
type X {
f1: [Z] @dgraph(pred: "f1")
}
interface Person {
id: ID!
}
type Y implements Person {
f1: [X] @dgraph(pred: "~f1")
}
errlist: [
{"message": "Type X; Field f1: should be any of types Y or Person to be compatible with @dgraph reverse
directive but is of type Z.",
"locations":[{"line":5, "column":12}]}
]

-
name: "Dgraph directive with reverse pred argument matching with wrong type implementing multiple interfaces produces an error"
input: |
type Z {
f1: String!
}
type X {
f1: [Z] @dgraph(pred: "f1")
}
interface Person {
id: ID!
}
interface Student {
ids: String! @id
}
type Y implements Person & Student {
f1: [X] @dgraph(pred: "~f1")
}
errlist: [
{"message": "Type X; Field f1: should be any of types Y, Person or Student to be compatible with @dgraph reverse
directive but is of type Z.",
"locations":[{"line":5, "column":12}]}
]

-
name: "Field with a dgraph directive with reverse pred argument should be a list"
input: |
Expand Down Expand Up @@ -2451,9 +2501,13 @@ valid_schemas:
name: "dgraph directive with correct reverse field works"
input: |
type X {
id: ID!
name: String
f1: [Y] @dgraph(pred: "~f1")
}
type Y {
id: ID!
name: String
f1: [X] @dgraph(pred: "f1")
}
Expand Down Expand Up @@ -2605,3 +2659,22 @@ valid_schemas:
body: "{sid: $id}"
})
}
-
name: "dgraph directive with reverse edges should work with interfaces"
input: |
type Object {
id: ID!
name: String
ownedBy: Person @dgraph(pred: "Object.owner")
}
type BusinessMan implements Person {
companyName: String
}
interface Person {
id: ID!
name: String
owns: [Object] @dgraph(pred: "~Object.owner")
}
2 changes: 0 additions & 2 deletions graphql/schema/introspection_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package schema

import (
"fmt"
"io/ioutil"
"path/filepath"
"testing"
Expand Down Expand Up @@ -366,7 +365,6 @@ func TestFullIntrospectionQuery(t *testing.T) {

queries := oper.Queries()
resp, err := Introspect(queries[0])
fmt.Println(string(resp))
require.NoError(t, err)

expectedBuf, err := ioutil.ReadFile("testdata/introspection/output/full_query.json")
Expand Down
28 changes: 23 additions & 5 deletions graphql/schema/rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -1085,12 +1085,30 @@ func dgraphDirectiveValidation(sch *ast.Schema, typ *ast.Definition, field *ast.
continue
}
if predArg.Value.Raw == forwardEdgePred {
if fld.Type.Name() != typ.Name {
errs = append(errs, gqlerror.ErrorPosf(dir.Position, "Type %s; Field %s: should be of"+
" type %s to be compatible with @dgraph reverse directive but is of"+
" type %s.", invTypeName, fld.Name, typ.Name, fld.Type.Name()))
possibleTypes := append([]string{typ.Name}, typ.Interfaces...)
allowedType := false
for _, pt := range possibleTypes {
if fld.Type.Name() == pt {
allowedType = true
break
}
}
if !allowedType {
typeMsg := ""
if len(possibleTypes) == 1 {
typeMsg = fmt.Sprintf("of type %s", possibleTypes[0])
} else {
l := len(possibleTypes)
typeMsg = fmt.Sprintf("any of types %s or %s",
strings.Join(possibleTypes[:l-1], ", "), possibleTypes[l-1])
}
errs = append(errs, gqlerror.ErrorPosf(dir.Position, "Type %s; Field %s: "+
"should be %s to be compatible with @dgraph"+
" reverse directive but is of type %s.",
invTypeName, fld.Name, typeMsg, fld.Type.Name()))
return errs
}

invDirective := fld.Directives.ForName(inverseDirective)
if invDirective != nil {
errs = append(errs, gqlerror.ErrorPosf(
Expand Down Expand Up @@ -1183,7 +1201,7 @@ func customDirectiveValidation(sch *ast.Schema,
if httpArg.Value.Kind != ast.ObjectValue {
errs = append(errs, gqlerror.ErrorPosf(
httpArg.Position,
"Type %s; Field %s: http argument for @custom directive should be of type Object.",
"Type %s; Field %s: http argument for @custom directive ff type Object.",
typ.Name, field.Name))
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
type Object {
id: ID!
name: String
ownedBy: Person @dgraph(pred: "Object.owner")
}

type BusinessMan implements Person {
companyName: String
}

interface Person {
id: ID!
name: String
owns: [Object] @dgraph(pred: "~Object.owner")
}
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ enum OscarMovieOrderable {

input AddDirectorInput {
name: String!
directed: [OscarMovieRef]
}

input AddOscarMovieInput {
Expand All @@ -237,7 +236,6 @@ input DirectorPatch {
input DirectorRef {
id: ID
name: String
directed: [OscarMovieRef]
}

input MovieFilter {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,6 @@ input AddDirectorInput {

input AddOscarMovieInput {
name: String!
director: [DirectorRef]
year: Int!
}

Expand Down Expand Up @@ -279,7 +278,6 @@ input OscarMoviePatch {
input OscarMovieRef {
id: ID
name: String
director: [DirectorRef]
year: Int
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ input AddMovieDirectorInput {

input AddMovieInput {
name: String!
director: [MovieDirectorRef]
}

input MovieDirectorFilter {
Expand Down Expand Up @@ -235,7 +234,6 @@ input MoviePatch {
input MovieRef {
id: ID
name: String
director: [MovieDirectorRef]
}

input UpdateMovieDirectorInput {
Expand Down
Loading

0 comments on commit ecec071

Please sign in to comment.