Add support for interfaces, part 1: the simplest cases #52

benjaminjkraft · 2021-08-19T23:04:14Z

Summary:

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

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

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

generate/config.go

generate/generate_test.go

generate/traverse.go

dnerdy · 2021-08-23T19:23:10Z

generate/testdata/snapshots/TestGenerate-UnionNoFragments.graphql-UnionNoFragments.graphql.go

+func (v *UnionNoFragmentsQueryRandomLeafVideo) implementsGraphQLInterfaceUnionNoFragmentsQueryRandomLeafLeafContent() {
+}
+
+func __unmarshalUnionNoFragmentsQueryRandomLeafLeafContent(v *UnionNoFragmentsQueryRandomLeafLeafContent, m json.RawMessage) error {


Is there an advantage of using v as an out argument instead of returning (InterfaceNestingRootTopicChildrenArticleParentTopicChildrenContent, error)?

Then the code below would be:

err, v.RandomLeaf = __unmarshalUnionNoFragmentsQueryRandomLeafLeafContent(firstPass.RandomLeaf)

Ah, interesting. I was doing this to match json.Unmarshal, and I do think it's marginally more efficient this way, although probably not to an extent that matters.

But I think actually the biggest factor is what will make the most sense when list-of-interface fields make the caller more complicated (#54). I think actually the out-argument makes things simpler; but maybe you can see what you think when you get there.

generate/unmarshal.go.tmpl

internal/integration/integration_test.go

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

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

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

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

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 · 2021-08-24T22:29:19Z

(@dnerdy just so you know, you never approved this one! no worries if you want to look again or whatever, just in case you missed it.)

dnerdy

Whoops. Thanks for the ping!

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

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

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

## 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

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

## 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 self-assigned this Aug 19, 2021

benjaminjkraft requested review from aberkan, csilvers, dnerdy and MiguelCastillo August 19, 2021 23:04

benjaminjkraft mentioned this pull request Aug 20, 2021

Add support for interfaces, part 2: list-of-interface #54

Merged

benjaminjkraft force-pushed the benkraft.refactor branch from 91c48e6 to bfd08f2 Compare August 20, 2021 17:42

benjaminjkraft force-pushed the benkraft.interfaces-1 branch from a83f63c to 6d13135 Compare August 20, 2021 17:42

benjaminjkraft force-pushed the benkraft.refactor branch from bfd08f2 to 5c3f806 Compare August 20, 2021 17:57

benjaminjkraft force-pushed the benkraft.interfaces-1 branch from 6d13135 to 1e265e6 Compare August 20, 2021 17:57

benjaminjkraft mentioned this pull request Aug 23, 2021

Add support for interfaces, part 3: automatically add __typename #56

Merged

dnerdy reviewed Aug 23, 2021

View reviewed changes

benjaminjkraft force-pushed the benkraft.interfaces-1 branch from 1e265e6 to 7496cbf Compare August 23, 2021 22:35

dnerdy approved these changes Aug 25, 2021

View reviewed changes

benjaminjkraft changed the base branch from benkraft.refactor to main August 25, 2021 18:49

benjaminjkraft added 2 commits August 25, 2021 11:49

Improve comments

3073246

benjaminjkraft force-pushed the benkraft.interfaces-1 branch from e98a8ce to 3073246 Compare August 25, 2021 18:50

benjaminjkraft merged commit 4c38cb7 into main Aug 25, 2021

benjaminjkraft deleted the benkraft.interfaces-1 branch August 25, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for interfaces, part 1: the simplest cases #52

Add support for interfaces, part 1: the simplest cases #52

benjaminjkraft commented Aug 19, 2021

dnerdy Aug 23, 2021

benjaminjkraft Aug 23, 2021

benjaminjkraft commented Aug 24, 2021

dnerdy left a comment

Add support for interfaces, part 1: the simplest cases #52

Add support for interfaces, part 1: the simplest cases #52

Conversation

benjaminjkraft commented Aug 19, 2021

Summary:

Test plan:

dnerdy Aug 23, 2021

Choose a reason for hiding this comment

benjaminjkraft Aug 23, 2021

Choose a reason for hiding this comment

benjaminjkraft commented Aug 24, 2021

dnerdy left a comment

Choose a reason for hiding this comment