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

benjaminjkraft · 2021-08-20T01:18:03Z

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

dnerdy

This looks good to me. I mostly reviewed the generated code and not the template. As mentioned inline, only supporting a pointer for the inner-most type seems right. Also, it would be nice to have an integration test that exercises pointer handling.

dnerdy · 2021-08-24T21:10:39Z

generate/testdata/queries/schema.graphql

@@ -111,6 +111,7 @@ type Query {
  getJunk: Junk
  getComplexJunk: ComplexJunk
  listOfListsOfLists: [[[String!]!]!]!
+  listOfListsOfListsOfContent: [[[Content!]!]!]!


listOfListsOfListsOfContent: [[[Content]!]!]! is a type where one might want to use a pointer (though types like this usually seem to usually be accidental). Pointers to slices shouldn't be necessary, so it seems correct to only support pointers on the most-inner type.

I think there are valid uses for required-list-of-optional-item, if you want the input in correspondence to the output. But I agree I can only see a reason to support it at the innermost point. Anyways, this is all mentioned at #16; my attitude is basically "if someone finds a legitimate reason to want it, we can implement it" :-) .

dnerdy · 2021-08-24T21:20:06Z

...enerate-InterfaceListOfListsOfListsField.graphql-InterfaceListOfListsOfListsField.graphql.go

+}
+
+// InterfaceListOfListOfListsFieldListOfListsOfListsOfContentArticle includes the requested fields of the GraphQL type Article.
+type InterfaceListOfListOfListsFieldListOfListsOfListsOfContentArticle struct {


Whew. Hopefully no one would actually need to use a type named like this anywhere.

dnerdy · 2021-08-24T21:22:08Z

generate/types.go

+	Unwrap() goType
+
+	// Count the number of times Unwrap() will unwrap a slice type.  For
+	// example, given []*[]**[]MyStruct, return 3.


nit: this type isn't supported in the current design; it seems like it would be nice to use a valid example (even though the method could in principle support this type as well).

dnerdy · 2021-08-24T21:24:21Z

generate/unmarshal.go.tmpl

+         This gets a little complicated because we may have a slice field.
+         So what we do is basically, for each field:
+
+            target := &v.MyField      // *[][]...[]MyType


Same nit here about this not being a supported type.

This is correct as-is -- target has type *[][]...[]MyType (or *[][]...[]*MyType, because it's a pointer to the field-type, again because that simplifies the templating at the cost of making the generated code look a bit strange.

dnerdy · 2021-08-24T21:30:45Z

internal/integration/integration_test.go

@@ -99,6 +99,36 @@ func TestInterfaceNoFragments(t *testing.T) {
 	assert.Nil(t, resp.Being)
 }

+func TestInterfaceListField(t *testing.T) {


Could you also add an integration test that uses @genqlient(pointer: true)? I think such a test is the only place the generated code would be exercises (and where we'd suss out any issues). Is that right?

My theory was that any issues there would show up at build-time. But it's probably better not to be lazy :-) .

LOL, you were right, the code is actually incorrect (crashes with a nil pointer dereference)! Thanks for the prod :-) .

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

I was like "I don't need no stinkin' integration test, the compiler would catch any problems", but Mark asked for one anyway, and it turns out it caught a bug! Also some other minor fixes to documentation, from review comments.

benjaminjkraft self-assigned this Aug 20, 2021

benjaminjkraft requested review from aberkan, csilvers, dnerdy and MiguelCastillo August 20, 2021 01:18

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

benjaminjkraft force-pushed the benkraft.interfaces-2 branch from 256aacc to 2f03b6e Compare August 20, 2021 17:42

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

benjaminjkraft force-pushed the benkraft.interfaces-2 branch from 2f03b6e to 51efbc0 Compare August 20, 2021 17:57

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

benjaminjkraft force-pushed the benkraft.interfaces-2 branch from b5469dc to cc1942e Compare August 23, 2021 22:35

benjaminjkraft mentioned this pull request Aug 23, 2021

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

Merged

benjaminjkraft force-pushed the benkraft.interfaces-2 branch from 624c176 to f8d359c Compare August 23, 2021 22:48

dnerdy approved these changes Aug 24, 2021

View reviewed changes

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

benjaminjkraft changed the base branch from benkraft.interfaces-1 to main August 25, 2021 18:51

benjaminjkraft added 3 commits August 25, 2021 11:55

Re-enable another test

91f0d6d

Fix up pointer: true code to initialize list elements

9ffbc9f

I was like "I don't need no stinkin' integration test, the compiler would catch any problems", but Mark asked for one anyway, and it turns out it caught a bug! Also some other minor fixes to documentation, from review comments.

benjaminjkraft force-pushed the benkraft.interfaces-2 branch from f5e4166 to 9ffbc9f Compare August 25, 2021 18:57

benjaminjkraft merged commit 1e87553 into main Aug 25, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benjaminjkraft commented Aug 20, 2021

dnerdy left a comment

dnerdy Aug 24, 2021

benjaminjkraft Aug 24, 2021 •

edited

Loading

dnerdy Aug 24, 2021

dnerdy Aug 24, 2021

dnerdy Aug 24, 2021

benjaminjkraft Aug 24, 2021

dnerdy Aug 24, 2021

benjaminjkraft Aug 24, 2021

benjaminjkraft Aug 24, 2021 •

edited

Loading

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

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

Conversation

benjaminjkraft commented Aug 20, 2021

Summary:

Test plan:

dnerdy left a comment

Choose a reason for hiding this comment

dnerdy Aug 24, 2021

Choose a reason for hiding this comment

benjaminjkraft Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

dnerdy Aug 24, 2021

Choose a reason for hiding this comment

dnerdy Aug 24, 2021

Choose a reason for hiding this comment

dnerdy Aug 24, 2021

Choose a reason for hiding this comment

benjaminjkraft Aug 24, 2021

Choose a reason for hiding this comment

dnerdy Aug 24, 2021

Choose a reason for hiding this comment

benjaminjkraft Aug 24, 2021

Choose a reason for hiding this comment

benjaminjkraft Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

benjaminjkraft Aug 24, 2021 •

edited

Loading

benjaminjkraft Aug 24, 2021 •

edited

Loading