talksik/genqlient
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Allow genqlient types to be marshaled safely (#120)

Browse Source 
## Summary:
When genqlient generates output types, it generates whatever code is
necessary to unmarshal them.  Conversely, when it generates input types,
it generates whatever code is necessary to marshal.  This is all that's
needed for genqlient itself: it never needs to marshal output types or
unmarshal input types.

But maybe you do!  (For example, to put the responses in a cache, which
is the use case that @csilvers hit at Khan, although there are others
one can imagine.)  While we can't support every serialization format you
might want (at least not without adding plugins or some such), it's not
unreasonable to expect that since genqlient can read JSON, it can write
it too.  Sadly, in the past this was not true for types requiring custom
unmarshaling logic, for several reasons.

In this commit I implement logic to always write both marshalers and
unmarshalers whenever they're needed to be able to correctly round-trip
the types, even though genqlient doesn't do so.  I wasn't starting from
scratch, since of course we already write both marshalers and
unmarshalers in some cases.  But this ended up requiring surprisingly
large changes on the marshaling side, mostly to correctly support
embedding (which we use for named fragments).

Specifically, as the comments in `types.go` discuss, the most difficult
issue is spreads with duplicate fields, which translate to Go embedded
fields which end up hidden from the json-marshaler.  Ultimately, I had
to do things quite differently from unmarshaling, and essentially
flatten the type when we write marshaler.  But in the end it's not so
ugly -- indeed arguably it's cleaner!  Mainly it's just different.

One thing to note is that we do marshal `__typename` based on
what we know about the types; users need not fill it in (and if they
do we'll ignore it).  This seemed to me to be a better UX, and
didn't add much complexity.

In general, I begin to wonder whether using `encoding/json` at all is
really right for genqlient: we're doing a lot of work to appease it,
despite knowing what our types look like.  I think it would still be a
significant increase in lines of code to roll our own, but that code
would perhaps be simpler, and would surely be faster (although if we
just want the speed gains we could use another JSON-generator library,
see also #47).  Anyway, something to think about in the future.

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: csilvers, StevenACoffman, benjaminjkraft, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo

Required Reviewers: 

Approved By: StevenACoffman, dnerdy

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

Pull Request URL: https://github.com/Khan/genqlient/pull/120
This commit is contained in:
Ben Kraft
2021-09-29 10:30:43 -07:00
committed by GitHub
parent 1f65445127
commit f4c981031e
30 changed files with 4143 additions and 180 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/integration/generated.go
+894 -21
View File
File diff suppressed because it is too large Load Diff
internal/integration/integration_test.go
+17 -12
View File
@@ -25,7 +25,7 @@ func TestSimpleQuery(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := simpleQuery(ctx, client)
require.NoError(t, err)
@@ -42,7 +42,7 @@ func TestServerError(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := failingQuery(ctx, client)
// As long as we get some response back, we should still return a full
@@ -55,7 +55,7 @@ func TestServerError(t *testing.T) {
func TestNetworkError(t *testing.T) {
ctx := context.Background()
client := graphql.NewClient("https://nothing.invalid/graphql", http.DefaultClient)
client := newRoundtripClient(t, "https://nothing.invalid/graphql")
resp, err := failingQuery(ctx, client)
// As we guarantee in the README, even on network error you always get a
@@ -75,6 +75,11 @@ func TestVariables(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
// This doesn't roundtrip successfully because the zero user gets marshaled
// as {"id": "", "name": "", ...}, not null. There's really no way to do
// this right in Go (without adding `pointer: true` just for this purpose),
// and unmarshal(marshal(resp)) == resp should still hold, so we don't
// worry about it.
client := graphql.NewClient(server.URL, http.DefaultClient)
resp, err := queryWithVariables(ctx, client, "2")
@@ -99,7 +104,7 @@ func TestOmitempty(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithOmitempty(ctx, client, "2")
require.NoError(t, err)
@@ -126,7 +131,7 @@ func TestCustomMarshal(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithCustomMarshal(ctx, client,
time.Date(2025, time.January, 1, 12, 34, 56, 789, time.UTC))
@@ -155,7 +160,7 @@ func TestCustomMarshalSlice(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithCustomMarshalSlice(ctx, client,
[]time.Time{time.Date(2025, time.January, 1, 12, 34, 56, 789, time.UTC)})
@@ -189,7 +194,7 @@ func TestCustomMarshalOptional(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
date := time.Date(2025, time.January, 1, 12, 34, 56, 789, time.UTC)
resp, err := queryWithCustomMarshalOptional(ctx, client, &date, nil)
@@ -223,7 +228,7 @@ func TestInterfaceNoFragments(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithInterfaceNoFragments(ctx, client, "1")
require.NoError(t, err)
@@ -286,7 +291,7 @@ func TestInterfaceListField(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithInterfaceListField(ctx, client,
[]string{"1", "3", "12847394823"})
@@ -333,7 +338,7 @@ func TestInterfaceListPointerField(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithInterfaceListPointerField(ctx, client,
[]string{"1", "3", "12847394823"})
@@ -387,7 +392,7 @@ func TestFragments(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithFragments(ctx, client, []string{"1", "3", "12847394823"})
require.NoError(t, err)
@@ -480,7 +485,7 @@ func TestNamedFragments(t *testing.T) {
ctx := context.Background()
server := server.RunServer()
defer server.Close()
client := graphql.NewClient(server.URL, http.DefaultClient)
client := newRoundtripClient(t, server.URL)
resp, err := queryWithNamedFragments(ctx, client, []string{"1", "3", "12847394823"})
require.NoError(t, err)
internal/integration/roundtrip.go
+111
View File
@@ -0,0 +1,111 @@
package integration
// Machinery for integration tests to round-trip check the JSON-marshalers and
// unmarshalers we generate.
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io/ioutil"
"net/http"
"testing"
"github.com/Khan/genqlient/graphql"
"github.com/stretchr/testify/assert"
)
// lastResponseTransport is an HTTP transport that keeps track of the last response
// that passed through it.
type lastResponseTransport struct {
wrapped http.RoundTripper
lastResponseBody []byte
}
func (t *lastResponseTransport) RoundTrip(req *http.Request) (*http.Response, error) {
resp, err := t.wrapped.RoundTrip(req)
if err != nil {
return resp, err
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return resp, fmt.Errorf("roundtrip failed: unreadable body: %w", err)
}
t.lastResponseBody = body
// Restore the body for the next reader:
resp.Body = ioutil.NopCloser(bytes.NewBuffer(body))
return resp, err
}
// roundtripClient is a graphql.Client that checks that
// unmarshal(marshal(req)) == req && marshal(unmarshal(resp)) == resp
// for each request it processes.
type roundtripClient struct {
wrapped graphql.Client
transport *lastResponseTransport
t *testing.T
}
// Put JSON in a stable and human-readable format.
func (c *roundtripClient) formatJSON(b []byte) []byte {
// We don't care about key ordering, so do another roundtrip through
// interface{} to drop that.
var parsed interface{}
err := json.Unmarshal(b, &parsed)
if err != nil {
c.t.Fatal(err)
}
// When marshaling, add indents to make things human-readable.
b, err = json.MarshalIndent(parsed, "", " ")
if err != nil {
c.t.Fatal(err)
}
return b
}
func (c *roundtripClient) roundtripResponse(resp interface{}) {
var graphqlResponse struct {
Data json.RawMessage `json:"data"`
}
err := json.Unmarshal(c.transport.lastResponseBody, &graphqlResponse)
if err != nil {
c.t.Error(err)
return
}
body := c.formatJSON(graphqlResponse.Data)
// resp is constructed to be unmarshal(body), so just use it
bodyAgain, err := json.Marshal(resp)
if err != nil {
c.t.Error(err)
return
}
bodyAgain = c.formatJSON(bodyAgain)
assert.Equal(c.t, string(body), string(bodyAgain))
}
func (c *roundtripClient) MakeRequest(ctx context.Context, opName, query string, retval, variables interface{}) error {
// TODO(benkraft): Also check the variables round-trip. This is a bit less
// important since most of the code is the same (and input types are
// strictly simpler), and a bit hard to do because when asserting about
// structs we need to worry about things like equality of time.Time values.
err := c.wrapped.MakeRequest(ctx, opName, query, retval, variables)
if err != nil {
return err
}
c.roundtripResponse(retval)
return nil
}
func newRoundtripClient(t *testing.T, endpoint string) graphql.Client {
transport := &lastResponseTransport{wrapped: http.DefaultTransport}
return &roundtripClient{
wrapped: graphql.NewClient(endpoint, &http.Client{Transport: transport}),
transport: transport,
t: t,
}
}
internal/testutil/types.go
+9
View File
@@ -31,6 +31,15 @@ func GetClientFromMyContext(ctx MyContext) (graphql.Client, error) { return
const dateFormat = "2006-01-02"
func MarshalDate(t *time.Time) ([]byte, error) {
// nil should never happen but we might as well check. zero-time does
// happen because omitempty doesn't consider it zero; we'd prefer to write
// null than "0001-01-01".
//
// (I mean, we're tests. Who cares! But we may as well try to match what
// prod code would want.)
if t == nil || t.IsZero() {
return []byte("null"), nil
}
return []byte(`"` + t.Format(dateFormat) + `"`), nil
}