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.

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 tests to check that genqlient handles covariance #203

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

benjaminjkraft
Copy link
Collaborator

I didn't realize until today that implementations of GraphQL interfaces
are actually allowed to be covariant: if the interface has a field
f: T, then the implementations may have fields f: U where U is a
subtype of T (for example U may be an implementation of the
interface T, or U may be T! if T is non-nullable. (I thought it
had to be f: T exactly.) So I figured I'd add a test and see what
breaks.

Surprisingly, and despite the fact that Go interfaces do not allow
covariance, everything... worked? There's at least one place where it's
possible we could ideally use a more specific type [1], but for now I
just wanted to make sure we at least write something that builds and is
vaguely reasonable. Of course I'm not sure if there's anything I've
missed (some day I need to find a fuzzing engine that can fuzz GraphQL).

[1] Specifically, the field
CovariantInterfaceImplementationRandomItemTopic.Next might ideally
have type ...NextContentTopic, not ...NextContent; we know it's a
topic. This doesn't directly cause covariance problems in Go: the method
GetNext still returns ...NextContent so the interface matches. But
that trick doesn't work for the sibling field .Related which is
slice-typed: or rather, we'd need the method to copy the slice to the
correct type. (Not to mention the implemention of any change here would
require a bunch of plumbing because the AST doesn't quite have what we
want.) So it's probably best to just keep this as-is for simplicity and
consistency.

Test plan: make check

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

I didn't realize until today that implementations of GraphQL interfaces
are actually allowed to be covariant: if the interface has a field
`f: T`, then the implementations may have fields `f: U` where `U` is a
subtype of `T` (for example `U` may be an implementation of the
interface `T`, or `U` may be `T!` if `T` is non-nullable. (I thought it
had to be `f: T` exactly.) So I figured I'd add a test and see what
breaks.

Surprisingly, and despite the fact that Go interfaces do *not* allow
covariance, everything... worked? There's at least one place where it's
possible we could ideally use a more specific type [1], but for now I
just wanted to make sure we at least write something that builds and is
vaguely reasonable. Of course I'm not sure if there's anything I've
missed (some day I need to find a fuzzing engine that can fuzz GraphQL).

[1] Specifically, the field
`CovariantInterfaceImplementationRandomItemTopic.Next` might ideally
have type `...NextContentTopic`, not `...NextContent`; we know it's a
topic. This doesn't directly cause covariance problems in Go: the method
`GetNext` still returns `...NextContent` so the interface matches. But
that trick doesn't work for the sibling field `.Related` which is
slice-typed: or rather, we'd need the method to copy the slice to the
correct type. (Not to mention the implemention of any change here would
require a bunch of plumbing because the AST doesn't quite have what we
want.) So it's probably best to just keep this as-is for simplicity and
consistency.

Test plan: make check
@csilvers
Copy link
Member

csilvers commented Jun 4, 2022

I didn't delve very deeply into all the types you use to see how they were covariant, but the test graphql file looks pretty extensive to me!

@benjaminjkraft benjaminjkraft merged commit 520532e into main Jun 4, 2022
@benjaminjkraft benjaminjkraft deleted the benkraft.covariance branch June 4, 2022 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants