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

Handle interfaces, unions, and fragments #8

Closed
benjaminjkraft opened this issue Apr 22, 2021 · 0 comments · Fixed by #79
Closed

Handle interfaces, unions, and fragments #8

benjaminjkraft opened this issue Apr 22, 2021 · 0 comments · Fixed by #79
Assignees
Milestone

Comments

@benjaminjkraft
Copy link
Collaborator

See DESIGN.md for more on the plan.

@benjaminjkraft benjaminjkraft added this to the OSS "launch" milestone Apr 22, 2021
@benjaminjkraft benjaminjkraft modified the milestones: OSS "launch", Khan May 3, 2021
@benjaminjkraft benjaminjkraft self-assigned this Aug 18, 2021
benjaminjkraft added a commit that referenced this issue Aug 19, 2021
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8).  Well, it's not the beginning: I already had
some half-written broken code around.  But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.

Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations.  (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that.  So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how.  I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work.  (If only one
could just write it as an actual method!)  The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.

This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
  fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!

As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`).  I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.

[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md

Issue: #8

Test plan: make tesc
benjaminjkraft added a commit that referenced this issue Aug 20, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 20, 2021
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8).  Well, it's not the beginning: I already had
some half-written broken code around.  But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.

Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations.  (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that.  So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how.  I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work.  (If only one
could just write it as an actual method!)  The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.

This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
  fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!

As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`).  I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.

[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md

Issue: #8

Test plan: make tesc
benjaminjkraft added a commit that referenced this issue Aug 20, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 20, 2021
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8).  Well, it's not the beginning: I already had
some half-written broken code around.  But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.

Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations.  (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that.  So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how.  I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work.  (If only one
could just write it as an actual method!)  The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.

This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
  fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!

As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`).  I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.

[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md

Issue: #8

Test plan: make tesc
benjaminjkraft added a commit that referenced this issue Aug 20, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In this commit I begin the journey to add the long-awaited support for
interfaces (part of #8).  Well, it's not the beginning: I already had
some half-written broken code around.  But it's the first fully
functional support, and especially, the first *tested* support; it's
probably best to review the nontrivially-changed code as if it were new.

Conceptually, the code so far is pretty simple: we generate an interface
type, and the implementations.  (That code is in fact mostly unchanged.)
The complexity comes in because encoding/json doesn't know how to
unmarshal that.  So we have to add an UnmarshalJSON method, which
actually has to be on the types with interface-type fields, that knows
how.  I factored it into two methods, such that that UnmarshalJSON
method is just glue, and then there's a separate function, corresponding
to each interface-type, that actually does all the work.  (If only one
could just write it as an actual method!)  The method uses the same
trick suggested to me by a few others in another context to deserialize
all but one field, then handle that field specially, which is discussed
in the code.

This still has some limitations, which will be lifted in future commits:
- it doesn't allow for list-of-interface fields
- it requires that you manually ask for `__typename`
- it doesn't support fragments, i.e. you can only query for interface
  fields, not concrete-type-specific ones
But it works, even in integration tests, which is progress!

As a part of this, I added a proper config option for the "allow broken
features" flag, since I need to be able to set it from the integration
tests which are in a separate package (and actually shell out via `go
generate`).  I also renamed what was to be the first case
(InterfaceNoFragments), and replaced it with a further-simplified
version (avoiding list-of-interface fields.

[1] https://github.com/benjaminjkraft/notes/blob/master/go-json-interfaces.md

Issue: #8

Test plan: make tesc
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 23, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, csilvers, adam
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
## Summary:
In this commit I remove one of the limitations of our support for
interfaces, from #52, by adding support for list-of-interface fields.
This was surprisingly complex!  The issue is that, as before, it's the
containing type that has to do all the glue work -- and it's that glue
work that is complicated by list-of-interface fields.

All in all, it's not that much new code, and by far the hard part is
just 20 lines in the UnmarshalJSON template (which come with almost
twice as many lines of comments to explain them).  It may be easiest to
start by reading some of the generated code, and then read the template.

I also added support for such fields with `pointer: true` specified,
such that the type is `[][]...[]*MyInterface`, although I don't know why
you would want that.  This does *not* allow e.g. `*[]*[][]*MyInterface`;
that would require a way to specify it (see #16) but also add some extra
complexity (as we'd have to actually walk the type-unwrap chain
properly, instead of just counting the number of slices and whether
there's a pointer).

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint, ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13)

Pull request URL: #54
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
## Summary:
In #52, I added support for interface types, but with the simplifying
restriction (among others) that the user must request the field
`__typename`.  In this commit, I remove this restriction.

The basic idea is simple: we preprocess the query to add `__typename`.
The implementation isn't much more complicated!  Although it required
some new wiring in a few places.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint

Pull request URL: #56
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
## Summary:
Right now, if you make a query like `{ myInterface { field } }`, you
have to type-switch on all the possible implementations of `myInterface`
to get at `field`.  Now, we generate getter-methods (e.g. `GetField`),
to make that access easier.  Of course this only applies to shared
fields (which for now are the only ones, but once we support fragments
will no longer be).

This also includes a small change to the way we generate type-names for
interfaces: we no longer include the name of the concrete type in the
interface we propagate forward, so we generate
`MyInterfaceMyFieldMyType`, not `MyInterfaceMyImplMyFieldMyType`, in the
case where you have an interface `MyInterface` implemented by `MyImpl`
(and maybe other types) with field `myField: MyType`.  This is necessary
so the getter method returns a well-defined type, and also probably
convenient for calling code.  It will have to get a little bit more
complicated once we support fragments, where you could have two
implementing types with identically-named fields of different types, but
I think it'll be easiest to figure out how to deal with that when
implementing fragments.

While I was in the area, I added to the interface doc-comment a list of
the implementations.  (In GraphQL, we're guaranteed to know them all
assuming our schema is up to date.)

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: benjaminjkraft, dnerdy, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull request URL: #57
benjaminjkraft added a commit that referenced this issue Aug 25, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 26, 2021
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 26, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 26, 2021
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 27, 2021
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
## Summary:
Having implemented support for interfaces, it's time to implement
support for fragments.  And it turns out there's actually another design
decision I hadn't really thought about when thinking about interfaces!
In this commit I add a sketch of the design -- two proposed designs
really.  This one I think will be easier to change later (via a flag),
but opinions are still welcome.

Issue: #8

## Test plan:
read it


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, csilvers, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull request URL: #59
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
## Summary:
In this commit I add support for inline fragments
(`... on MyType { fields }`) to genqlient.  This will make interfaces a
lot more useful!  In future commits I'll add named fragments, for which
we'll generate slightly different types, as discussed in DESIGN.md.

In general, implementing the flattening approach described in DESIGN.md
was... surprisingly easy.  All we have to do is recurse on applicable
fragments when generating our selection-set.  The refactor to
selection-set handling this encouraged was, I think, quite beneficial.
It did reveal two tricky pre-existing issues.

One issue is that GraphQL allows for duplicate selections, as long as
they match.  (In practice, this is only useful in the context of
fragments, although GraphQL allows it even without.) I decided to handle
the simple case (duplicate leaf fields; we just deduplicate) but leave
to the future the complex cases where we need to merge different
sub-selections (now #64).  For now we just forbid that; we can see how
much it comes up.

The other issue is that we are generating type-names incorrectly for
interface types; I had intended to do `MyInterfaceMyFieldMyType` for
shared fields and `MyImplMyFieldMyType` for non-shared ones, but instead
I did `MyFieldMyType`, which is inconsistent already and can result in
conflicts in the presence of fragments.  I'm going to fix this in a
separate commit, though, because it's going to require some refactoring
and is irrelevant to the main logic of this commit; I left some TODOs in
the tests related to this.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ✅ Lint

Pull request URL: #65
benjaminjkraft added a commit that referenced this issue Aug 28, 2021
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 30, 2021
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

Test plan: make tesc

Reviewers: marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 30, 2021
…71)

## Summary:
When adding support for interfaces, I did not do the type-names as I
intended: they came out to be `MyFieldMyType`, not
`MyInterfaceMyFieldMyType`, which is inconsistent, but not strictly
wrong.  But once supporting fragments, this is also now incorrect.
(Exactly why is described in the comments inline.)  In this commit, in
any case, I fix it.

To do that, I finally did the last of the refactors I've been hoping to
do but unable to successfully implement, which is to make the type-name
and type-name-prefix management clearer.  In the past it was kind of
spread out, and each caller would have to pass the right name into
`convertDefinition`, which go quite unwieldy.  Now, the case that really
wanted that -- the operation toplevel -- just does it own thing; and the
main name-generation code  is factored out into a separate file with
tests, and with a long comment that goes into all the details of the
algorithm that the design-doc didn't cover.  (I even had some fun using
a linked list to implement the prefix-stack!)

This allowed me to fix the above bug fairly easily -- actually the fix
was pretty much automatic once I understood how to organize things.
There is one change which is that if your query name is unexported, we
no longer do the same with the input-type names; it's unclear to me if
anyone will actually care about this behavior (Khan always makes the
queries exported) but if they did it was very inconsistent (only at the
query toplevel, and only for input-objects, not enums), so we can
reimplement it properly if that comes up.  As a bonus fix, we now better
handle the case where your type-names are lowercase, which is legal if
nonstandard GraphQL.

Issue: #8

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo

Required Reviewers: 

Approved by: dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull request URL: #71
benjaminjkraft added a commit that referenced this issue Sep 3, 2021
In previous commits I added support to genqlient for interfaces and
inline fragments.  This means the only query structures that remain are
named fragments and their spreads, e.g.
```
fragment MyFragment on MyType { myField }
query MyQuery { getMyType { ...MyFragment } }
```
Other than mere completionism, these are potentially useful for code
sharing: you can spread the same fragment multiple places; and then
genqlient can notice that and generate the same type for each.  (They
can even be shared between different queries in the same package.)

In this commit I add support for named fragments of concrete
(object/struct, not interface) type, spread into either concrete or
abstract scope.  For genqlient's purposes, these are a new "root"
type-name, just like each operation, and are then embedded into the
appropriate struct.  (Using embeds allows their fields to be referenced
as fields of the containing type, if convenient.  Further design
considerations are discussed in DESIGN.md.)

This requires new code in two main places (plus miscellaneous glue),
both nontrivial but neither particularly complex:
- We need to actually traverse both structures and generate the types
  (in `convert.go`).
- We need to decide which fragments from this package to send to the
  server, both for good hyigene and because GraphQL requires we send
  only ones this query uses (in `generate.go`).
- We need a little new wiring for options -- because fragments can be
  shared between queries they get their own toplevel options, rather
  than inheriting the query's options.

Finally, this required slightly subtler changes to how we do
unmarshaling (in `types.go` and `unmarshal.go.tmpl`).  Basically,
because embedded fields' methods, including `UnmarshalJSON`, get
promoted to the parent type, and because the JSON library ignores their
fields when shadowed by those of the parent type, we need a little bit
of special logic in each such parent type to do its own unmarshal and
then delegate to each embed.  This is similar (and much simpler) to
what we did for interfaces, although it required some changes to the
"method-hiding" trick (used for both).  It's only really necessary in
certain specific cases (namely when an embedded type has an
`UnmarshalJSON` method or a field with the same name as the embedder),
but it's easier to just generate it always.  This is all described in
more detail inline.

This does not support fragments of abstract type, which have their own
complexities.  I'll address those, which are now the only remaining
piece of #8, in a future commit.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, adam
benjaminjkraft added a commit that referenced this issue Sep 7, 2021
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type.  This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates.  But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
   was getting more and more confusing and duplicative.  I'm still not
   sure how much of it it makes sense to inline vs. separate, but I
   think this is better than it was.  This resulted in some minor
   changes to descriptions, generally in the direction of making things
   more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
   for duplicate interface methods.  These are useful for
   abstract-in-absstract spreads; we generate an interface for the
   fragment, and (if the fragment-type implements the scope-type) we
   embed it into the interface we generate for its spread-context, and
   if the two have a duplicated field we thus duplicate the method.  It
   wouldn't be impossible to support this on 1.13 (maybe just by
   omitting said embed) but it didn't seem worth it.  This also removes
   a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
   generated code (which we see from `gofmt`).  This is mostly just an
   internal improvement; I wrote it because I got annoyed while hunting
   down a few such errors..

Fixes, at last, #8.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, adam
@benjaminjkraft benjaminjkraft linked a pull request Sep 7, 2021 that will close this issue
benjaminjkraft added a commit that referenced this issue Sep 9, 2021
In previous commits I added support to genqlient for interfaces and
inline fragments.  This means the only query structures that remain are
named fragments and their spreads, e.g.
```
fragment MyFragment on MyType { myField }
query MyQuery { getMyType { ...MyFragment } }
```
Other than mere completionism, these are potentially useful for code
sharing: you can spread the same fragment multiple places; and then
genqlient can notice that and generate the same type for each.  (They
can even be shared between different queries in the same package.)

In this commit I add support for named fragments of concrete
(object/struct, not interface) type, spread into either concrete or
abstract scope.  For genqlient's purposes, these are a new "root"
type-name, just like each operation, and are then embedded into the
appropriate struct.  (Using embeds allows their fields to be referenced
as fields of the containing type, if convenient.  Further design
considerations are discussed in DESIGN.md.)

This requires new code in two main places (plus miscellaneous glue),
both nontrivial but neither particularly complex:
- We need to actually traverse both structures and generate the types
  (in `convert.go`).
- We need to decide which fragments from this package to send to the
  server, both for good hyigene and because GraphQL requires we send
  only ones this query uses (in `generate.go`).
- We need a little new wiring for options -- because fragments can be
  shared between queries they get their own toplevel options, rather
  than inheriting the query's options.

Finally, this required slightly subtler changes to how we do
unmarshaling (in `types.go` and `unmarshal.go.tmpl`).  Basically,
because embedded fields' methods, including `UnmarshalJSON`, get
promoted to the parent type, and because the JSON library ignores their
fields when shadowed by those of the parent type, we need a little bit
of special logic in each such parent type to do its own unmarshal and
then delegate to each embed.  This is similar (and much simpler) to
what we did for interfaces, although it required some changes to the
"method-hiding" trick (used for both).  It's only really necessary in
certain specific cases (namely when an embedded type has an
`UnmarshalJSON` method or a field with the same name as the embedder),
but it's easier to just generate it always.  This is all described in
more detail inline.

This does not support fragments of abstract type, which have their own
complexities.  I'll address those, which are now the only remaining
piece of #8, in a future commit.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, adam
benjaminjkraft added a commit that referenced this issue Sep 9, 2021
## Summary:
In previous commits I added support to genqlient for interfaces and
inline fragments.  This means the only query structures that remain are
named fragments and their spreads, e.g.
```
fragment MyFragment on MyType { myField }
query MyQuery { getMyType { ...MyFragment } }
```
Other than mere completionism, these are potentially useful for code
sharing: you can spread the same fragment multiple places; and then
genqlient can notice that and generate the same type for each.  (They
can even be shared between different queries in the same package.)

In this commit I add support for named fragments of concrete
(object/struct, not interface) type, spread into either concrete or
abstract scope.  For genqlient's purposes, these are a new "root"
type-name, just like each operation, and are then embedded into the
appropriate struct.  (Using embeds allows their fields to be referenced
as fields of the containing type, if convenient.  Further design
considerations are discussed in DESIGN.md.)

This requires new code in two main places (plus miscellaneous glue),
both nontrivial but neither particularly complex:
- We need to actually traverse both structures and generate the types
  (in `convert.go`).
- We need to decide which fragments from this package to send to the
  server, both for good hyigene and because GraphQL requires we send
  only ones this query uses (in `generate.go`).
- We need a little new wiring for options -- because fragments can be
  shared between queries they get their own toplevel options, rather
  than inheriting the query's options.

Finally, this required slightly subtler changes to how we do
unmarshaling (in `types.go` and `unmarshal.go.tmpl`).  Basically,
because embedded fields' methods, including `UnmarshalJSON`, get
promoted to the parent type, and because the JSON library ignores their
fields when shadowed by those of the parent type, we need a little bit
of special logic in each such parent type to do its own unmarshal and
then delegate to each embed.  This is similar (and much simpler) to
what we did for interfaces, although it required some changes to the
"method-hiding" trick (used for both).  It's only really necessary in
certain specific cases (namely when an embedded type has an
`UnmarshalJSON` method or a field with the same name as the embedder),
but it's easier to just generate it always.  This is all described in
more detail inline.

This does not support fragments of abstract type, which have their own
complexities.  I'll address those, which are now the only remaining
piece of #8, in a future commit.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo

Required Reviewers: 

Approved By: dnerdy

Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.13), ✅ Lint

Pull Request URL: #75
benjaminjkraft added a commit that referenced this issue Sep 9, 2021
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type.  This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates.  But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
   was getting more and more confusing and duplicative.  I'm still not
   sure how much of it it makes sense to inline vs. separate, but I
   think this is better than it was.  This resulted in some minor
   changes to descriptions, generally in the direction of making things
   more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
   for duplicate interface methods.  These are useful for
   abstract-in-absstract spreads; we generate an interface for the
   fragment, and (if the fragment-type implements the scope-type) we
   embed it into the interface we generate for its spread-context, and
   if the two have a duplicated field we thus duplicate the method.  It
   wouldn't be impossible to support this on 1.13 (maybe just by
   omitting said embed) but it didn't seem worth it.  This also removes
   a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
   generated code (which we see from `gofmt`).  This is mostly just an
   internal improvement; I wrote it because I got annoyed while hunting
   down a few such errors..

Fixes, at last, #8.

Issue: #8

Test plan:
make check

Reviewers: marksandstrom, miguel, adam
benjaminjkraft added a commit that referenced this issue Sep 9, 2021
## Summary:
In previous commits I added support to genqlient for interfaces,
inline fragments, and, most recently, named fragments of concrete
(object) type.  This leaves only named fragments of interface type!
Like other named fragments, these are useful for code-sharing,
especially if you want some code that can handle the same fields of
several different types.

As seems to be inevitable with genqlient, this was mostly pretty
straightforward, although there turned out to be surprisingly many
places we needed to add some handling; almost anywhere that touches
interfaces *or* named fragments needed some updates.  But it's all
hopefully fairly clear code.

As a part of this change I made three semi-related improvements:
1. I refactored the handling of descriptions (i.e. GoDoc), because it
   was getting more and more confusing and duplicative.  I'm still not
   sure how much of it it makes sense to inline vs. separate, but I
   think this is better than it was.  This resulted in some minor
   changes to descriptions, generally in the direction of making things
   more consistent.
2. I bumped the minimum Go version to 1.14 so we can guarantee support
   for duplicate interface methods.  These are useful for
   abstract-in-absstract spreads; we generate an interface for the
   fragment, and (if the fragment-type implements the scope-type) we
   embed it into the interface we generate for its spread-context, and
   if the two have a duplicated field we thus duplicate the method.  It
   wouldn't be impossible to support this on 1.13 (maybe just by
   omitting said embed) but it didn't seem worth it.  This also removes
   a few special-cases in tests.
3. I added a bunch of code to better format syntax errors in the
   generated code (which we see from `gofmt`).  This is mostly just an
   internal improvement; I wrote it because I got annoyed while hunting
   down a few such errors..

Fixes, at last, #8.

Issue: #8

## Test plan:
make check


Author: benjaminjkraft

Reviewers: dnerdy, benjaminjkraft, aberkan, MiguelCastillo

Required Reviewers: 

Approved By: dnerdy

Checks: ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint

Pull Request URL: #79
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 a pull request may close this issue.

1 participant