Add support for interfaces implementing other interfaces #436
Conversation
Interfaces now return an empty array for the "interfaces" introspection query
| for _, f := range iface.Interfaces { | ||
| if f == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Can this happen? Is it possible that one of the iface.Interfaces is nil?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| for _, iface := range s.interfaces { | ||
| if iface == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Can this ever be true? Can we have a nil interface?
| actualNames := iface.interfaceNames | ||
|
|
||
| expMap := make(map[string]int) | ||
| actMap := make(map[string]int) |
There was a problem hiding this comment.
Could you, please, remove the Map suffix from the names. From the syntax below it is obvious that it is a map.
| } | ||
| } | ||
|
|
||
| for _, iface := range s.interfaces { |
There was a problem hiding this comment.
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.
| // given interface | ||
| // | ||
| // TODO: Would be more efficient not to search every interface multiple | ||
| // times. |
There was a problem hiding this comment.
See my comment above how to handle that.
|
There are conflicts |
|
👋 |
|
@suessflorian I'd be happy to merge this change if you could resolve the conflicts |
|
Apologies @suessflorian + @pavelnikolov - have been very busy on another project. Feel free to take it from where I left off. Sorry, |
|
No worries @tomemmerson, your start here helped a lot - I've opened up a new PR continuing on from here and so I think we can close this PR @pavelnikolov 👍 |
|
Closing this PR because of #471 |
To fix issue Add support for interfaces implementing other interfaces
To match new draft spec here: http://spec.graphql.org/draft/#sec-Interfaces.Interfaces-Implementing-Interfaces
Allows the following functionality:
The implementation here isn't massively efficient, I check multiple interfaces multiple times, mostly due to the requirement that
There's definitely a more efficient solution than the one here, really just wanted an opinion as to whether it is worth it.
Thanks
Tom