Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for interfaces implementing other interfaces #436

Closed
wants to merge 2 commits into from
Closed

Add support for interfaces implementing other interfaces #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2b542e
Add support for interfaces implementing other interfaces
Mar 5, 2021
a80eb39
Test updates for introspection
Mar 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions example/social/introspect.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@
}
],
"description": "Marks an element of a GraphQL schema as no longer supported.",
"locations": [
"FIELD_DEFINITION",
"ENUM_VALUE"
],
"locations": ["FIELD_DEFINITION", "ENUM_VALUE"],
"name": "deprecated"
},
{
Expand All @@ -39,11 +36,7 @@
}
],
"description": "Directs the executor to include this field or fragment only when the `if` argument is true.",
"locations": [
"FIELD",
"FRAGMENT_SPREAD",
"INLINE_FRAGMENT"
],
"locations": ["FIELD", "FRAGMENT_SPREAD", "INLINE_FRAGMENT"],
"name": "include"
},
{
Expand All @@ -64,11 +57,7 @@
}
],
"description": "Directs the executor to skip this field or fragment when the `if` argument is true.",
"locations": [
"FIELD",
"FRAGMENT_SPREAD",
"INLINE_FRAGMENT"
],
"locations": ["FIELD", "FRAGMENT_SPREAD", "INLINE_FRAGMENT"],
"name": "skip"
}
],
Expand Down Expand Up @@ -132,7 +121,7 @@
}
],
"inputFields": null,
"interfaces": null,
"interfaces": [],
"kind": "INTERFACE",
"name": "Admin",
"possibleTypes": [
Expand Down Expand Up @@ -236,7 +225,7 @@
}
],
"inputFields": null,
"interfaces": null,
"interfaces": [],
"kind": "INTERFACE",
"name": "Person",
"possibleTypes": [
Expand Down Expand Up @@ -1381,4 +1370,4 @@
}
]
}
}
}
21 changes: 5 additions & 16 deletions example/starwars/introspect.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@
}
],
"description": "Marks an element of a GraphQL schema as no longer supported.",
"locations": [
"FIELD_DEFINITION",
"ENUM_VALUE"
],
"locations": ["FIELD_DEFINITION", "ENUM_VALUE"],
"name": "deprecated"
},
{
Expand All @@ -39,11 +36,7 @@
}
],
"description": "Directs the executor to include this field or fragment only when the `if` argument is true.",
"locations": [
"FIELD",
"FRAGMENT_SPREAD",
"INLINE_FRAGMENT"
],
"locations": ["FIELD", "FRAGMENT_SPREAD", "INLINE_FRAGMENT"],
"name": "include"
},
{
Expand All @@ -64,11 +57,7 @@
}
],
"description": "Directs the executor to skip this field or fragment when the `if` argument is true.",
"locations": [
"FIELD",
"FRAGMENT_SPREAD",
"INLINE_FRAGMENT"
],
"locations": ["FIELD", "FRAGMENT_SPREAD", "INLINE_FRAGMENT"],
"name": "skip"
}
],
Expand Down Expand Up @@ -205,7 +194,7 @@
}
],
"inputFields": null,
"interfaces": null,
"interfaces": [],
"kind": "INTERFACE",
"name": "Character",
"possibleTypes": [
Expand Down Expand Up @@ -2023,4 +2012,4 @@
}
]
}
}
}
2 changes: 1 addition & 1 deletion graphql_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2204,7 +2204,7 @@ func TestIntrospection(t *testing.T) {
"b": {
"name": "Character",
"kind": "INTERFACE",
"interfaces": null,
"interfaces": [],
"possibleTypes": [
{
"name": "Human"
Expand Down
20 changes: 10 additions & 10 deletions internal/common/lexer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,21 +94,21 @@ func TestConsume(t *testing.T) {
}
}

var multilineStringTests = []consumeTestCase {
var multilineStringTests = []consumeTestCase{
{
description: "Oneline strings are okay",
definition: `"Hello World"`,
expected: "",
failureExpected: false,
useStringDescriptions: true,
description: "Oneline strings are okay",
definition: `"Hello World"`,
expected: "",
failureExpected: false,
useStringDescriptions: true,
},
{
description: "Multiline strings are not allowed",
definition: `"Hello
World"`,
expected: `graphql: syntax error: literal not terminated (line 1, column 1)`,
failureExpected: true,
useStringDescriptions: true,
expected: `graphql: syntax error: literal not terminated (line 1, column 1)`,
failureExpected: true,
useStringDescriptions: true,
},
}

Expand All @@ -130,5 +130,5 @@ func TestMultilineString(t *testing.T) {
t.Fatalf("Test '%s' failed with error: '%s'", test.description, err.Error())
}
})
}
}
}
99 changes: 94 additions & 5 deletions internal/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type Schema struct {
unions []*Union
enums []*Enum
extensions []*Extension
interfaces []*Interface
}

// Resolve a named type in the schema by its name.
Expand Down Expand Up @@ -101,11 +102,13 @@ type Object struct {
//
// http://facebook.github.io/graphql/draft/#sec-Interfaces
type Interface struct {
Name string
PossibleTypes []*Object
Fields FieldList // NOTE: the spec refers to this as `FieldsDefinition`.
Desc string
Directives common.DirectiveList
Name string
PossibleTypes []*Object
Fields FieldList // NOTE: the spec refers to this as `FieldsDefinition`.
Desc string
Directives common.DirectiveList
Interfaces []*Interface // NOTE: http://spec.graphql.org/draft/#sec-Interfaces.Interfaces-Implementing-Interfaces
interfaceNames []string
}

// Union types represent objects that could be one of a list of GraphQL object types, but provides no
Expand Down Expand Up @@ -340,6 +343,52 @@ func (s *Schema) Parse(schemaString string, useStringDescriptions bool) error {
}
}

for _, iface := range s.interfaces {
iface.Interfaces = make([]*Interface, len(iface.interfaceNames))

for i, intfName := range iface.interfaceNames {
t, ok := s.Types[intfName]
if !ok {
return errors.Errorf("interface %q not found", intfName)
}
intf, ok := t.(*Interface)
if !ok {
return errors.Errorf("type %q is not an interface", intfName)
}
for _, f := range intf.Fields.Names() {
if iface.Fields.Get(f) == nil {
return errors.Errorf("interface %q expects field %q but %q does not provide it", intfName, f, iface.Name)
}
}

iface.Interfaces[i] = intf
}
}

for _, iface := range s.interfaces {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid checking interfaces multiple times, you might define a variable of type map[string][]string above this for statement that contains all the dependencies for each interface. Then you will check every interface exactly once if it is not in the map already.
However, if I understand correctly, you will need to sort all the interfaces by len(interfaceNames) ascending. This way interfaces that implement multiple other interfaces will make use of that map efficiently. This map will be used only once at runtime and then it will be garbage collected.

if iface == nil {
continue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever be true? Can we have a nil interface?

expectedNames := interfaceDependancies(*iface)
actualNames := iface.interfaceNames

expMap := make(map[string]int)
actMap := make(map[string]int)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you, please, remove the Map suffix from the names. From the syntax below it is obvious that it is a map.


for _, name := range expectedNames {
expMap[name]++
}
for _, name := range actualNames {
actMap[name]++
}

for name, id := range expMap {
if actMap[name] != id {
return errors.Errorf("interface %q must explicitly implement transitive interface %q", iface.Name, name)
}
}
}

for _, union := range s.unions {
if err := resolveDirectives(s, union.Directives, "UNION"); err != nil {
return err
Expand Down Expand Up @@ -557,6 +606,7 @@ func parseSchema(s *Schema, l *common.Lexer) {
iface := parseInterfaceDef(l)
iface.Desc = desc
s.Types[iface.Name] = iface
s.interfaces = append(s.interfaces, iface)

case "union":
union := parseUnionDef(l)
Expand Down Expand Up @@ -634,6 +684,18 @@ func parseObjectDef(l *common.Lexer) *Object {
func parseInterfaceDef(l *common.Lexer) *Interface {
i := &Interface{Name: l.ConsumeIdent()}

if l.Peek() == scanner.Ident {
l.ConsumeKeyword("implements")

for l.Peek() != '{' && l.Peek() != '@' {
if l.Peek() == '&' {
l.ConsumeToken('&')
}

i.interfaceNames = append(i.interfaceNames, l.ConsumeIdent())
}
}

i.Directives = common.ParseDirectives(l)
l.ConsumeToken('{')
i.Fields = parseFieldsDef(l)
Expand Down Expand Up @@ -770,3 +832,30 @@ func parseFieldsDef(l *common.Lexer) FieldList {
}
return fields
}

// interfaceDependancies searches through the tree of interface relations
// to build a slice of all the names that should be implemented on the
// given interface
//
// TODO: Would be more efficient not to search every interface multiple
// times.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above how to handle that.

//
// NOTE: https://spec.graphql.org/draft/#sel-FAHbhBHCAACGB35P
func interfaceDependancies(iface Interface) []string {
var search func(requiredNames []string, iface Interface) []string

search = func(requiredNames []string, iface Interface) []string {
for _, f := range iface.Interfaces {
if f == nil {
continue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this happen? Is it possible that one of the iface.Interfaces is nil?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, good point, ifaces should never be nil in the current implementation. Just used to checking pointer types, is there a reason all the types are using pointers or can I change Schema.Interfaces to a non-pointer type and remove this check?

Copy link
Member

@pavelnikolov pavelnikolov Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the reason is since I haven't written this code but my guess is that the author didn't want to copy structs by value for performance reasons. On the other hand, interfaces are not supposed to be huge. But we never know how people use it. So let's leave it consistent with all other types.

I'd prefer if both the interfaceDependancies and search functions accept a reference. Then at the beginning of the search function check if the passed pointer to the Interface struct is nil and if yes, then panic. This is never supposed to happen. I don't like that in the current implementation you keep them as pointers but then you dereference them and pass them as values.

if len(f.Interfaces) > 0 {
requiredNames = append(requiredNames, search(requiredNames, *f)...)
}
requiredNames = append(requiredNames, f.interfaceNames...)
}
return requiredNames
}

return search([]string{}, iface)
}
77 changes: 77 additions & 0 deletions internal/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,79 @@ func TestParse(t *testing.T) {
return nil
},
},
{
name: "Parses interface implementing other interface",
sdl: `
interface Interaction {
message: String!
}
interface Greeting implements Interaction {
message: String!
}
`,
validateSchema: func(s *schema.Schema) error {
const implementedIfaceName = "Greeting"
typ, ok := s.Types[implementedIfaceName].(*schema.Interface)
if !ok {
return fmt.Errorf("interface %q not found", implementedIfaceName)
}
if want, have := 1, len(typ.Fields); want != have {
return fmt.Errorf("invalid number of fields: want %d, have %d", want, have)
}
const fieldName = "message"
if typ.Fields[0].Name != fieldName {
return fmt.Errorf("field %q not found", fieldName)
}

const implementingIfaceName = "Interaction"
if typ.Interfaces[0].Name != implementingIfaceName {
return fmt.Errorf("interface %q not found", implementingIfaceName)
}
return nil
},
},
{
name: "Transitively implemented interfaces must also be defined on an implementing type or interface",
sdl: `
interface Interaction {
message: String!
}
interface Greeting implements Interaction {
message: String!
name: String!
}
interface Welcome implements Greeting {
message: String!
name: String!
hug: Boolean!
}
`,
validateError: func(err error) error {
msg := `graphql: interface "Welcome" must explicitly implement transitive interface "Interaction"`
if err == nil || err.Error() != msg {
return fmt.Errorf("expected error %q, but got %q", msg, err)
}
return nil
},
},
{
name: "Parses interface implementing another without providing required fields",
sdl: `
interface Interaction {
message: String!
}
interface Greeting implements Interaction {
name: String!
}
`,
validateError: func(err error) error {
msg := `graphql: interface "Interaction" expects field "message" but "Greeting" does not provide it`
if err == nil || err.Error() != msg {
return fmt.Errorf("expected error %q, but got %q", msg, err)
}
return nil
},
},
{
name: "Parses type with description string",
sdl: `
Expand Down Expand Up @@ -863,6 +936,10 @@ Second line of the description.
if err := test.validateError(err); err != nil {
t.Fatal(err)
}
return
}
if test.validateError != nil {
t.Fatal("Error expected but no error returned")
}
if test.validateSchema != nil {
if err := test.validateSchema(s); err != nil {
Expand Down
Loading