From d0549edd16dceb6939e538fdb1b4f2ec7ee816cc Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Fri, 1 Dec 2017 09:01:01 -0500 Subject: [PATCH] Implement missing edge cases in queryArguments. (#7) Previously, queryArguments was partially implemented to handle the most common types I had run into, with a TODO to complete the rest. This change implements the support for the remaining argument types. Now, slices of T are mapped to "[T]!", and pointers to slices of T are mapped to "[T]", as per GraphQL specification. Add a comment pointing out that commas are omitted because they're insignificant, and we're looking to produce minified output (less bytes to send over the network). Fixes #6. --- query.go | 73 +++++++++++++++++++++++++++++++++------------------ query_test.go | 36 ++++++++++++++++++------- 2 files changed, 74 insertions(+), 35 deletions(-) diff --git a/query.go b/query.go index de9696b..a6b6aac 100644 --- a/query.go +++ b/query.go @@ -30,33 +30,56 @@ func constructMutation(v interface{}, variables map[string]interface{}) string { // // E.g., map[string]interface{}{"a": Int(123), "b": NewBoolean(true)} -> "$a:Int!$b:Boolean". func queryArguments(variables map[string]interface{}) string { - sorted := make([]string, 0, len(variables)) + // Sort keys in order to produce deterministic output for testing purposes. + // TODO: If tests can be made to work with non-deterministic output, then no need to sort. + keys := make([]string, 0, len(variables)) for k := range variables { - sorted = append(sorted, k) + keys = append(keys, k) } - sort.Strings(sorted) - var s string - for _, k := range sorted { - v := variables[k] - s += "$" + k + ":" - t := reflect.TypeOf(v) - switch t.Kind() { - case reflect.Slice, reflect.Array: - // TODO: Support t.Elem() being a pointer, if needed. Probably want to do this recursively. - s += "[" + t.Elem().Name() + "!]" // E.g., "[IssueState!]". - case reflect.Ptr: - // Pointer is an optional type, so no "!" at the end. - s += t.Elem().Name() // E.g., "Int". - default: - name := t.Name() - if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubql/issues/12. - name = "ID" - } - // Value is a required type, so add "!" to the end. - s += name + "!" // E.g., "Int!". + sort.Strings(keys) + + var buf bytes.Buffer + for _, k := range keys { + io.WriteString(&buf, "$") + io.WriteString(&buf, k) + io.WriteString(&buf, ":") + writeArgumentType(&buf, reflect.TypeOf(variables[k]), true) + // Don't insert a comma here. + // Commas in GraphQL are insignificant, and we want minified output. + // See https://facebook.github.io/graphql/October2016/#sec-Insignificant-Commas. + } + return buf.String() +} + +// writeArgumentType writes a minified GraphQL type for t to w. +// value indicates whether t is a value (required) type or pointer (optional) type. +// If value is true, then "!" is written at the end of t. +func writeArgumentType(w io.Writer, t reflect.Type, value bool) { + if t.Kind() == reflect.Ptr { + // Pointer is an optional type, so no "!" at the end of the pointer's underlying type. + writeArgumentType(w, t.Elem(), false) + return + } + + switch t.Kind() { + case reflect.Slice, reflect.Array: + // List. E.g., "[Int]". + io.WriteString(w, "[") + writeArgumentType(w, t.Elem(), true) + io.WriteString(w, "]") + default: + // Named type. E.g., "Int". + name := t.Name() + if name == "string" { // HACK: Workaround for https://github.com/shurcooL/githubql/issues/12. + name = "ID" } + io.WriteString(w, name) + } + + if value { + // Value is a required type, so add "!" to the end. + io.WriteString(w, "!") } - return s } // query uses writeQuery to recursively construct @@ -69,8 +92,8 @@ func query(v interface{}) string { return buf.String() } -// writeQuery writes a minified query for t to w. If inline is true, -// the struct fields of t are inlined into parent struct. +// writeQuery writes a minified query for t to w. +// If inline is true, the struct fields of t are inlined into parent struct. func writeQuery(w io.Writer, t reflect.Type, inline bool) { switch t.Kind() { case reflect.Ptr, reflect.Slice: diff --git a/query_test.go b/query_test.go index 84f3a62..4de8cb5 100644 --- a/query_test.go +++ b/query_test.go @@ -273,7 +273,6 @@ func TestConstructMutation(t *testing.T) { func TestQueryArguments(t *testing.T) { tests := []struct { - name string in map[string]interface{} want string }{ @@ -282,26 +281,43 @@ func TestQueryArguments(t *testing.T) { want: "$a:Int!$b:Boolean", }, { - in: map[string]interface{}{"states": []IssueState{IssueStateOpen, IssueStateClosed}}, - want: "$states:[IssueState!]", + in: map[string]interface{}{ + "required": []IssueState{IssueStateOpen, IssueStateClosed}, + "optional": &[]IssueState{IssueStateOpen, IssueStateClosed}, + }, + want: "$optional:[IssueState!]$required:[IssueState!]!", }, { - in: map[string]interface{}{"states": []IssueState(nil)}, - want: "$states:[IssueState!]", + in: map[string]interface{}{ + "required": []IssueState(nil), + "optional": (*[]IssueState)(nil), + }, + want: "$optional:[IssueState!]$required:[IssueState!]!", }, { - in: map[string]interface{}{"states": [...]IssueState{IssueStateOpen, IssueStateClosed}}, - want: "$states:[IssueState!]", + in: map[string]interface{}{ + "required": [...]IssueState{IssueStateOpen, IssueStateClosed}, + "optional": &[...]IssueState{IssueStateOpen, IssueStateClosed}, + }, + want: "$optional:[IssueState!]$required:[IssueState!]!", }, { - in: map[string]interface{}{"id": ID("someid")}, + in: map[string]interface{}{"id": ID("someID")}, want: "$id:ID!", }, + { + in: map[string]interface{}{"ids": []ID{"someID", "anotherID"}}, + want: `$ids:[ID!]!`, + }, + { + in: map[string]interface{}{"ids": &[]ID{"someID", "anotherID"}}, + want: `$ids:[ID!]`, + }, } - for _, tc := range tests { + for i, tc := range tests { got := queryArguments(tc.in) if got != tc.want { - t.Errorf("%s: got: %q, want: %q", tc.name, got, tc.want) + t.Errorf("test case %d:\n got: %q\nwant: %q", i, got, tc.want) } } }