From 1609afe9930b1c385d60cc92d9217b3fde785299 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 12 May 2020 18:50:08 +0200 Subject: [PATCH 01/12] Add `api` command --- api/client.go | 39 +++++++++++ command/api.go | 171 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) create mode 100644 command/api.go diff --git a/api/client.go b/api/client.go index d7b981bee..a0007c465 100644 --- a/api/client.go +++ b/api/client.go @@ -207,6 +207,45 @@ func (c Client) REST(method string, p string, body io.Reader, data interface{}) return nil } +// DirectRequest is a low-level interface to making generic API requests +func (c Client) DirectRequest(method string, p string, params interface{}, headers []string) (*http.Response, error) { + url := "https://api.github.com/" + p + var body io.Reader + var bodyIsJSON bool + + switch pp := params.(type) { + case map[string]interface{}: + b, err := json.Marshal(pp) + if err != nil { + return nil, fmt.Errorf("error serializing parameters: %w", err) + } + body = bytes.NewBuffer(b) + bodyIsJSON = true + case io.Reader: + body = pp + default: + return nil, fmt.Errorf("unrecognized parameters type: %v", params) + } + + req, err := http.NewRequest(method, url, body) + if err != nil { + return nil, err + } + + if bodyIsJSON { + req.Header.Set("Content-Type", "application/json; charset=utf-8") + } + for _, h := range headers { + idx := strings.IndexRune(h, ':') + if idx == -1 { + return nil, fmt.Errorf("header %q requires a value separated by ':'", h) + } + req.Header.Set(h[0:idx], strings.TrimSpace(h[idx+1:])) + } + + return c.http.Do(req) +} + func handleResponse(resp *http.Response, data interface{}) error { success := resp.StatusCode >= 200 && resp.StatusCode < 300 diff --git a/command/api.go b/command/api.go new file mode 100644 index 000000000..d27dacd0d --- /dev/null +++ b/command/api.go @@ -0,0 +1,171 @@ +package command + +import ( + "fmt" + "io" + "io/ioutil" + "os" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/spf13/cobra" +) + +func init() { + RootCmd.AddCommand(makeApiCommand()) +} + +type ApiOptions struct { + RequestMethod string + RequestMethodPassed bool + RequestPath string + MagicFields []string + RawFields []string + RequestHeaders []string + ShowResponseHeaders bool +} + +func makeApiCommand() *cobra.Command { + opts := ApiOptions{} + cmd := &cobra.Command{ + Use: "api ", + Short: "Make an authenticated GitHub API request", + Long: `Makes an authenticated HTTP request to the GitHub API and prints the response. + +The argument should either be a path of a GitHub API v3 endpoint, or +"graphql" to access the GitHub API v4. + +The default HTTP request method is "GET" normally and "POST" if any parameters +were added. Override the method with '--method'. + +Pass one or more '--raw-field' values in "=" format to add +JSON-encoded string parameters to the POST body. + +The '--field' flag behaves like '--raw-field' with magic type conversion based +on the format of the value: + +- literal values "true", "false", "null", and integer numbers get converted to + appropriate JSON types; +- if the value starts with "@", the rest of the value is interpreted as a + filename to read the value from. Pass "-" to read from standard input. +`, + Args: cobra.ExactArgs(1), + RunE: func(c *cobra.Command, args []string) error { + opts.RequestPath = args[0] + opts.RequestMethodPassed = c.Flags().Changed("method") + + ctx := contextForCommand(c) + client, err := apiClientForContext(ctx) + if err != nil { + return err + } + + return apiRun(&opts, client) + }, + } + + cmd.Flags().StringVarP(&opts.RequestMethod, "method", "X", "GET", "The HTTP method for the request") + cmd.Flags().StringArrayVarP(&opts.MagicFields, "field", "F", nil, "Add a parameter of inferred type") + cmd.Flags().StringArrayVarP(&opts.RawFields, "raw-field", "f", nil, "Add a string parameter") + cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add an additional HTTP request header") + cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") + return cmd +} + +func apiRun(opts *ApiOptions, client *api.Client) error { + params := make(map[string]interface{}) + for _, f := range opts.RawFields { + key, value, err := parseField(f) + if err != nil { + return err + } + params[key] = value + } + for _, f := range opts.MagicFields { + key, strValue, err := parseField(f) + if err != nil { + return err + } + value, err := magicFieldValue(strValue) + if err != nil { + return fmt.Errorf("error parsing %q value: %w", key, err) + } + params[key] = value + } + + method := opts.RequestMethod + if len(params) > 0 && !opts.RequestMethodPassed { + method = "POST" + } + + resp, err := client.DirectRequest(method, opts.RequestPath, params, opts.RequestHeaders) + if err != nil { + return err + } + + if opts.ShowResponseHeaders { + for name, vals := range resp.Header { + fmt.Printf("%s: %s\r\n", name, strings.Join(vals, ", ")) + } + fmt.Print("\r\n") + } + + if resp.StatusCode == 204 { + return nil + } + defer resp.Body.Close() + + // TODO: make stdout configurable for tests + _, err = io.Copy(os.Stdout, resp.Body) + if err != nil { + return err + } + + return nil +} + +func parseField(f string) (string, string, error) { + idx := strings.IndexRune(f, '=') + if idx == -1 { + return f, "", fmt.Errorf("field %q requires a value separated by an '=' sign", f) + } + return f[0:idx], f[idx+1:], nil +} + +func magicFieldValue(v string) (interface{}, error) { + if strings.HasPrefix(v, "@") { + return readUserFile(v[1:]) + } + + if n, err := strconv.Atoi(v); err != nil { + return n, nil + } + + switch v { + case "true": + return true, nil + case "false": + return false, nil + case "null": + return nil, nil + default: + return v, nil + } +} + +func readUserFile(fn string) ([]byte, error) { + var r io.ReadCloser + if fn == "-" { + // TODO: make stdin configurable for tests + r = os.Stdin + } else { + var err error + r, err = os.Open(fn) + if err != nil { + return nil, err + } + defer r.Close() + } + return ioutil.ReadAll(r) +} From fa3e25bb4db25ad03a26a9e20e5d08d2992e10d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 May 2020 11:42:03 +0200 Subject: [PATCH 02/12] Serialize GraphQL parameters under `variables` --- api/client.go | 68 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 5 deletions(-) diff --git a/api/client.go b/api/client.go index a0007c465..0e620365f 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "net/url" "regexp" "strings" @@ -212,15 +213,23 @@ func (c Client) DirectRequest(method string, p string, params interface{}, heade url := "https://api.github.com/" + p var body io.Reader var bodyIsJSON bool + isGraphQL := p == "graphql" switch pp := params.(type) { case map[string]interface{}: - b, err := json.Marshal(pp) - if err != nil { - return nil, fmt.Errorf("error serializing parameters: %w", err) + if strings.EqualFold(method, "GET") { + url = addQuery(url, pp) + } else { + if isGraphQL { + pp = groupGraphQLVariables(pp) + } + b, err := json.Marshal(pp) + if err != nil { + return nil, fmt.Errorf("error serializing parameters: %w", err) + } + body = bytes.NewBuffer(b) + bodyIsJSON = true } - body = bytes.NewBuffer(b) - bodyIsJSON = true case io.Reader: body = pp default: @@ -246,6 +255,55 @@ func (c Client) DirectRequest(method string, p string, params interface{}, heade return c.http.Do(req) } +func groupGraphQLVariables(params map[string]interface{}) map[string]interface{} { + topLevel := make(map[string]interface{}) + variables := make(map[string]interface{}) + + for key, val := range params { + switch key { + case "query": + topLevel[key] = val + default: + variables[key] = val + } + } + + if len(variables) > 0 { + topLevel["variables"] = variables + } + return topLevel +} + +func addQuery(path string, params map[string]interface{}) string { + if len(params) == 0 { + return path + } + + query := url.Values{} + for key, value := range params { + switch v := value.(type) { + case string: + query.Add(key, v) + case []byte: + query.Add(key, string(v)) + case nil: + query.Add(key, "") + case int: + query.Add(key, fmt.Sprintf("%d", v)) + case bool: + query.Add(key, fmt.Sprintf("%v", v)) + default: + panic(fmt.Sprintf("unknown type %v", v)) + } + } + + sep := "?" + if strings.ContainsRune(path, '?') { + sep = "&" + } + return path + sep + query.Encode() +} + func handleResponse(resp *http.Response, data interface{}) error { success := resp.StatusCode >= 200 && resp.StatusCode < 300 From 90fa193eafdc52b99cdd514f8177c6cf459da62a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 May 2020 15:26:42 +0200 Subject: [PATCH 03/12] Promote `api` command to a `pkg/cmd/api` package --- api/client.go | 109 +++----------------------------- command/root.go | 3 + {command => pkg/cmd/api}/api.go | 35 ++++++---- pkg/cmd/api/http.go | 107 +++++++++++++++++++++++++++++++ pkg/cmd/api/legacy.go | 34 ++++++++++ 5 files changed, 173 insertions(+), 115 deletions(-) rename {command => pkg/cmd/api}/api.go (87%) create mode 100644 pkg/cmd/api/http.go create mode 100644 pkg/cmd/api/legacy.go diff --git a/api/client.go b/api/client.go index 0e620365f..79883015e 100644 --- a/api/client.go +++ b/api/client.go @@ -7,7 +7,6 @@ import ( "io" "io/ioutil" "net/http" - "net/url" "regexp" "strings" @@ -17,14 +16,18 @@ import ( // ClientOption represents an argument to NewClient type ClientOption = func(http.RoundTripper) http.RoundTripper -// NewClient initializes a Client -func NewClient(opts ...ClientOption) *Client { +// NewHTTPClient initializes an http.Client +func NewHTTPClient(opts ...ClientOption) *http.Client { tr := http.DefaultTransport for _, opt := range opts { tr = opt(tr) } - http := &http.Client{Transport: tr} - client := &Client{http: http} + return &http.Client{Transport: tr} +} + +// NewClient initializes a Client +func NewClient(opts ...ClientOption) *Client { + client := &Client{http: NewHTTPClient(opts...)} return client } @@ -208,102 +211,6 @@ func (c Client) REST(method string, p string, body io.Reader, data interface{}) return nil } -// DirectRequest is a low-level interface to making generic API requests -func (c Client) DirectRequest(method string, p string, params interface{}, headers []string) (*http.Response, error) { - url := "https://api.github.com/" + p - var body io.Reader - var bodyIsJSON bool - isGraphQL := p == "graphql" - - switch pp := params.(type) { - case map[string]interface{}: - if strings.EqualFold(method, "GET") { - url = addQuery(url, pp) - } else { - if isGraphQL { - pp = groupGraphQLVariables(pp) - } - b, err := json.Marshal(pp) - if err != nil { - return nil, fmt.Errorf("error serializing parameters: %w", err) - } - body = bytes.NewBuffer(b) - bodyIsJSON = true - } - case io.Reader: - body = pp - default: - return nil, fmt.Errorf("unrecognized parameters type: %v", params) - } - - req, err := http.NewRequest(method, url, body) - if err != nil { - return nil, err - } - - if bodyIsJSON { - req.Header.Set("Content-Type", "application/json; charset=utf-8") - } - for _, h := range headers { - idx := strings.IndexRune(h, ':') - if idx == -1 { - return nil, fmt.Errorf("header %q requires a value separated by ':'", h) - } - req.Header.Set(h[0:idx], strings.TrimSpace(h[idx+1:])) - } - - return c.http.Do(req) -} - -func groupGraphQLVariables(params map[string]interface{}) map[string]interface{} { - topLevel := make(map[string]interface{}) - variables := make(map[string]interface{}) - - for key, val := range params { - switch key { - case "query": - topLevel[key] = val - default: - variables[key] = val - } - } - - if len(variables) > 0 { - topLevel["variables"] = variables - } - return topLevel -} - -func addQuery(path string, params map[string]interface{}) string { - if len(params) == 0 { - return path - } - - query := url.Values{} - for key, value := range params { - switch v := value.(type) { - case string: - query.Add(key, v) - case []byte: - query.Add(key, string(v)) - case nil: - query.Add(key, "") - case int: - query.Add(key, fmt.Sprintf("%d", v)) - case bool: - query.Add(key, fmt.Sprintf("%v", v)) - default: - panic(fmt.Sprintf("unknown type %v", v)) - } - } - - sep := "?" - if strings.ContainsRune(path, '?') { - sep = "&" - } - return path + sep + query.Encode() -} - func handleResponse(resp *http.Response, data interface{}) error { success := resp.StatusCode >= 200 && resp.StatusCode < 300 diff --git a/command/root.go b/command/root.go index 6233be957..85a6d722b 100644 --- a/command/root.go +++ b/command/root.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" + apiCmd "github.com/cli/cli/pkg/cmd/api" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -61,6 +62,8 @@ func init() { } return &FlagError{Err: err} }) + + RootCmd.AddCommand(apiCmd.NewCmdApi()) } // FlagError is the kind of error raised in flag processing diff --git a/command/api.go b/pkg/cmd/api/api.go similarity index 87% rename from command/api.go rename to pkg/cmd/api/api.go index d27dacd0d..2fa1b12f5 100644 --- a/command/api.go +++ b/pkg/cmd/api/api.go @@ -1,21 +1,18 @@ -package command +package api import ( "fmt" "io" "io/ioutil" + "net/http" "os" "strconv" "strings" - "github.com/cli/cli/api" + "github.com/cli/cli/context" "github.com/spf13/cobra" ) -func init() { - RootCmd.AddCommand(makeApiCommand()) -} - type ApiOptions struct { RequestMethod string RequestMethodPassed bool @@ -24,9 +21,11 @@ type ApiOptions struct { RawFields []string RequestHeaders []string ShowResponseHeaders bool + + HttpClient func() (*http.Client, error) } -func makeApiCommand() *cobra.Command { +func NewCmdApi() *cobra.Command { opts := ApiOptions{} cmd := &cobra.Command{ Use: "api ", @@ -55,13 +54,16 @@ on the format of the value: opts.RequestPath = args[0] opts.RequestMethodPassed = c.Flags().Changed("method") - ctx := contextForCommand(c) - client, err := apiClientForContext(ctx) - if err != nil { - return err + opts.HttpClient = func() (*http.Client, error) { + ctx := context.New() + token, err := ctx.AuthLogin() + if err != nil { + return nil, err + } + return apiClientFromContext(token), nil } - return apiRun(&opts, client) + return apiRun(&opts) }, } @@ -73,7 +75,7 @@ on the format of the value: return cmd } -func apiRun(opts *ApiOptions, client *api.Client) error { +func apiRun(opts *ApiOptions) error { params := make(map[string]interface{}) for _, f := range opts.RawFields { key, value, err := parseField(f) @@ -99,7 +101,12 @@ func apiRun(opts *ApiOptions, client *api.Client) error { method = "POST" } - resp, err := client.DirectRequest(method, opts.RequestPath, params, opts.RequestHeaders) + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + resp, err := httpRequest(httpClient, method, opts.RequestPath, params, opts.RequestHeaders) if err != nil { return err } diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go new file mode 100644 index 000000000..112f6ab1b --- /dev/null +++ b/pkg/cmd/api/http.go @@ -0,0 +1,107 @@ +package api + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" +) + +func httpRequest(client *http.Client, method string, p string, params interface{}, headers []string) (*http.Response, error) { + // TODO: GHE support + url := "https://api.github.com/" + p + var body io.Reader + var bodyIsJSON bool + isGraphQL := p == "graphql" + + switch pp := params.(type) { + case map[string]interface{}: + if strings.EqualFold(method, "GET") { + url = addQuery(url, pp) + } else { + if isGraphQL { + pp = groupGraphQLVariables(pp) + } + b, err := json.Marshal(pp) + if err != nil { + return nil, fmt.Errorf("error serializing parameters: %w", err) + } + body = bytes.NewBuffer(b) + bodyIsJSON = true + } + case io.Reader: + body = pp + default: + return nil, fmt.Errorf("unrecognized parameters type: %v", params) + } + + req, err := http.NewRequest(method, url, body) + if err != nil { + return nil, err + } + + if bodyIsJSON { + req.Header.Set("Content-Type", "application/json; charset=utf-8") + } + for _, h := range headers { + idx := strings.IndexRune(h, ':') + if idx == -1 { + return nil, fmt.Errorf("header %q requires a value separated by ':'", h) + } + req.Header.Set(h[0:idx], strings.TrimSpace(h[idx+1:])) + } + + return client.Do(req) +} + +func groupGraphQLVariables(params map[string]interface{}) map[string]interface{} { + topLevel := make(map[string]interface{}) + variables := make(map[string]interface{}) + + for key, val := range params { + switch key { + case "query": + topLevel[key] = val + default: + variables[key] = val + } + } + + if len(variables) > 0 { + topLevel["variables"] = variables + } + return topLevel +} + +func addQuery(path string, params map[string]interface{}) string { + if len(params) == 0 { + return path + } + + query := url.Values{} + for key, value := range params { + switch v := value.(type) { + case string: + query.Add(key, v) + case []byte: + query.Add(key, string(v)) + case nil: + query.Add(key, "") + case int: + query.Add(key, fmt.Sprintf("%d", v)) + case bool: + query.Add(key, fmt.Sprintf("%v", v)) + default: + panic(fmt.Sprintf("unknown type %v", v)) + } + } + + sep := "?" + if strings.ContainsRune(path, '?') { + sep = "&" + } + return path + sep + query.Encode() +} diff --git a/pkg/cmd/api/legacy.go b/pkg/cmd/api/legacy.go new file mode 100644 index 000000000..5c8429727 --- /dev/null +++ b/pkg/cmd/api/legacy.go @@ -0,0 +1,34 @@ +package api + +import ( + "fmt" + "net/http" + "os" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/utils" +) + +// TODO +func apiClientFromContext(token string) *http.Client { + var opts []api.ClientOption + if verbose := os.Getenv("DEBUG"); verbose != "" { + opts = append(opts, apiVerboseLog()) + } + opts = append(opts, + api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), + // FIXME + // api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", command.Version)), + // antiope-preview: Checks + api.AddHeader("Accept", "application/vnd.github.antiope-preview+json"), + ) + return api.NewHTTPClient(opts...) +} + +// TODO +func apiVerboseLog() api.ClientOption { + logTraffic := strings.Contains(os.Getenv("DEBUG"), "api") + colorize := utils.IsTerminal(os.Stderr) + return api.VerboseLog(utils.NewColorable(os.Stderr), logTraffic, colorize) +} From 7ffbde3e12beeb091fd49315f9cbffbbc63806f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 May 2020 15:34:18 +0200 Subject: [PATCH 04/12] Allow setting multiple values for a request header --- pkg/cmd/api/http.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go index 112f6ab1b..4f9064f18 100644 --- a/pkg/cmd/api/http.go +++ b/pkg/cmd/api/http.go @@ -43,15 +43,15 @@ func httpRequest(client *http.Client, method string, p string, params interface{ return nil, err } - if bodyIsJSON { - req.Header.Set("Content-Type", "application/json; charset=utf-8") - } for _, h := range headers { idx := strings.IndexRune(h, ':') if idx == -1 { return nil, fmt.Errorf("header %q requires a value separated by ':'", h) } - req.Header.Set(h[0:idx], strings.TrimSpace(h[idx+1:])) + req.Header.Add(h[0:idx], strings.TrimSpace(h[idx+1:])) + } + if bodyIsJSON && req.Header.Get("Content-Type") == "" { + req.Header.Set("Content-Type", "application/json; charset=utf-8") } return client.Do(req) From a7100b1fdd2f4a3347d36ebd88034c8b6ec97d8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 May 2020 15:59:52 +0200 Subject: [PATCH 05/12] Extract `parseFields` to a func --- pkg/cmd/api/api.go | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 2fa1b12f5..7e4106f9d 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -76,24 +76,9 @@ on the format of the value: } func apiRun(opts *ApiOptions) error { - params := make(map[string]interface{}) - for _, f := range opts.RawFields { - key, value, err := parseField(f) - if err != nil { - return err - } - params[key] = value - } - for _, f := range opts.MagicFields { - key, strValue, err := parseField(f) - if err != nil { - return err - } - value, err := magicFieldValue(strValue) - if err != nil { - return fmt.Errorf("error parsing %q value: %w", key, err) - } - params[key] = value + params, err := parseFields(opts) + if err != nil { + return err } method := opts.RequestMethod @@ -132,6 +117,29 @@ func apiRun(opts *ApiOptions) error { return nil } +func parseFields(opts *ApiOptions) (map[string]interface{}, error) { + params := make(map[string]interface{}) + for _, f := range opts.RawFields { + key, value, err := parseField(f) + if err != nil { + return params, err + } + params[key] = value + } + for _, f := range opts.MagicFields { + key, strValue, err := parseField(f) + if err != nil { + return params, err + } + value, err := magicFieldValue(strValue) + if err != nil { + return params, fmt.Errorf("error parsing %q value: %w", key, err) + } + params[key] = value + } + return params, nil +} + func parseField(f string) (string, string, error) { idx := strings.IndexRune(f, '=') if idx == -1 { From 4c762d5bd7a05377018960c112622b8c15cdf37d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 19 May 2020 22:30:03 +0200 Subject: [PATCH 06/12] Add iostreams package --- pkg/iostreams/iostreams.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 pkg/iostreams/iostreams.go diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go new file mode 100644 index 000000000..028b11264 --- /dev/null +++ b/pkg/iostreams/iostreams.go @@ -0,0 +1,33 @@ +package iostreams + +import ( + "bytes" + "io" + "io/ioutil" + "os" +) + +type IOStreams struct { + In io.ReadCloser + Out io.Writer + ErrOut io.Writer +} + +func System() *IOStreams { + return &IOStreams{ + In: os.Stdin, + Out: os.Stdout, + ErrOut: os.Stderr, + } +} + +func Test() (*IOStreams, *bytes.Buffer, *bytes.Buffer, *bytes.Buffer) { + in := &bytes.Buffer{} + out := &bytes.Buffer{} + errOut := &bytes.Buffer{} + return &IOStreams{ + In: ioutil.NopCloser(in), + Out: out, + ErrOut: errOut, + }, in, out, errOut +} From d8146cd16e4dc28725bfbba50f5a4b8920fdc1bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 19 May 2020 22:31:02 +0200 Subject: [PATCH 07/12] Extract cmdutil package --- cmd/gh/main.go | 7 ++++++- cmd/gh/main_test.go | 4 ++-- command/issue.go | 9 ++------- command/root.go | 16 ++-------------- pkg/cmdutil/errors.go | 19 +++++++++++++++++++ 5 files changed, 31 insertions(+), 24 deletions(-) create mode 100644 pkg/cmdutil/errors.go diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 9ddf79315..98916523e 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/command" "github.com/cli/cli/internal/config" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/update" "github.com/cli/cli/utils" "github.com/mgutz/ansi" @@ -48,6 +49,10 @@ func main() { } func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { + if err == cmdutil.SilentError { + return + } + var dnsError *net.DNSError if errors.As(err, &dnsError) { fmt.Fprintf(out, "error connecting to %s\n", dnsError.Name) @@ -60,7 +65,7 @@ func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { fmt.Fprintln(out, err) - var flagError *command.FlagError + var flagError *cmdutil.FlagError if errors.As(err, &flagError) || strings.HasPrefix(err.Error(), "unknown command ") { if !strings.HasSuffix(err.Error(), "\n") { fmt.Fprintln(out) diff --git a/cmd/gh/main_test.go b/cmd/gh/main_test.go index 3e0a02690..9036391fd 100644 --- a/cmd/gh/main_test.go +++ b/cmd/gh/main_test.go @@ -7,7 +7,7 @@ import ( "net" "testing" - "github.com/cli/cli/command" + "github.com/cli/cli/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -49,7 +49,7 @@ check your internet connection or githubstatus.com { name: "Cobra flag error", args: args{ - err: &command.FlagError{Err: errors.New("unknown flag --foo")}, + err: &cmdutil.FlagError{Err: errors.New("unknown flag --foo")}, cmd: cmd, debug: false, }, diff --git a/command/issue.go b/command/issue.go index 3d3f01708..ea9888bb4 100644 --- a/command/issue.go +++ b/command/issue.go @@ -73,14 +73,9 @@ var issueStatusCmd = &cobra.Command{ RunE: issueStatus, } var issueViewCmd = &cobra.Command{ - Use: "view { | }", - Args: func(cmd *cobra.Command, args []string) error { - if len(args) < 1 { - return FlagError{errors.New("issue number or URL required as argument")} - } - return nil - }, + Use: "view { | }", Short: "View an issue", + Args: cobra.ExactArgs(1), Long: `Display the title, body, and other information about an issue. With '--web', open the issue in a web browser instead.`, diff --git a/command/root.go b/command/root.go index 85a6d722b..93ed51109 100644 --- a/command/root.go +++ b/command/root.go @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" apiCmd "github.com/cli/cli/pkg/cmd/api" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -60,25 +61,12 @@ func init() { if err == pflag.ErrHelp { return err } - return &FlagError{Err: err} + return &cmdutil.FlagError{Err: err} }) RootCmd.AddCommand(apiCmd.NewCmdApi()) } -// FlagError is the kind of error raised in flag processing -type FlagError struct { - Err error -} - -func (fe FlagError) Error() string { - return fe.Err.Error() -} - -func (fe FlagError) Unwrap() error { - return fe.Err -} - // RootCmd is the entry point of command-line execution var RootCmd = &cobra.Command{ Use: "gh [flags]", diff --git a/pkg/cmdutil/errors.go b/pkg/cmdutil/errors.go new file mode 100644 index 000000000..77ca58340 --- /dev/null +++ b/pkg/cmdutil/errors.go @@ -0,0 +1,19 @@ +package cmdutil + +import "errors" + +// FlagError is the kind of error raised in flag processing +type FlagError struct { + Err error +} + +func (fe FlagError) Error() string { + return fe.Err.Error() +} + +func (fe FlagError) Unwrap() error { + return fe.Err +} + +// SilentError is an error that triggers exit code 1 without any error messaging +var SilentError = errors.New("SilentError") From f58e0bf710a29a6fd4556c8691f8a1dafd592b4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 18 May 2020 11:48:02 +0200 Subject: [PATCH 08/12] Add api tests --- pkg/cmd/api/api.go | 32 ++-- pkg/cmd/api/api_test.go | 115 +++++++++++++++ pkg/cmd/api/http.go | 2 + pkg/cmd/api/http_test.go | 306 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 444 insertions(+), 11 deletions(-) create mode 100644 pkg/cmd/api/api_test.go create mode 100644 pkg/cmd/api/http_test.go diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 7e4106f9d..02eac439b 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -10,10 +10,14 @@ import ( "strings" "github.com/cli/cli/context" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/spf13/cobra" ) type ApiOptions struct { + IO *iostreams.IOStreams + RequestMethod string RequestMethodPassed bool RequestPath string @@ -54,9 +58,12 @@ on the format of the value: opts.RequestPath = args[0] opts.RequestMethodPassed = c.Flags().Changed("method") + // TODO: pass in via caller + opts.IO = iostreams.System() + opts.HttpClient = func() (*http.Client, error) { ctx := context.New() - token, err := ctx.AuthLogin() + token, err := ctx.AuthToken() if err != nil { return nil, err } @@ -108,12 +115,16 @@ func apiRun(opts *ApiOptions) error { } defer resp.Body.Close() - // TODO: make stdout configurable for tests - _, err = io.Copy(os.Stdout, resp.Body) + _, err = io.Copy(opts.IO.Out, resp.Body) if err != nil { return err } + // TODO: detect GraphQL errors + if resp.StatusCode > 299 { + return cmdutil.SilentError + } + return nil } @@ -131,7 +142,7 @@ func parseFields(opts *ApiOptions) (map[string]interface{}, error) { if err != nil { return params, err } - value, err := magicFieldValue(strValue) + value, err := magicFieldValue(strValue, opts.IO.In) if err != nil { return params, fmt.Errorf("error parsing %q value: %w", key, err) } @@ -148,12 +159,12 @@ func parseField(f string) (string, string, error) { return f[0:idx], f[idx+1:], nil } -func magicFieldValue(v string) (interface{}, error) { +func magicFieldValue(v string, stdin io.ReadCloser) (interface{}, error) { if strings.HasPrefix(v, "@") { - return readUserFile(v[1:]) + return readUserFile(v[1:], stdin) } - if n, err := strconv.Atoi(v); err != nil { + if n, err := strconv.Atoi(v); err == nil { return n, nil } @@ -169,18 +180,17 @@ func magicFieldValue(v string) (interface{}, error) { } } -func readUserFile(fn string) ([]byte, error) { +func readUserFile(fn string, stdin io.ReadCloser) ([]byte, error) { var r io.ReadCloser if fn == "-" { - // TODO: make stdin configurable for tests - r = os.Stdin + r = stdin } else { var err error r, err = os.Open(fn) if err != nil { return nil, err } - defer r.Close() } + defer r.Close() return ioutil.ReadAll(r) } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go new file mode 100644 index 000000000..ad8211761 --- /dev/null +++ b/pkg/cmd/api/api_test.go @@ -0,0 +1,115 @@ +package api + +import ( + "bytes" + "fmt" + "io/ioutil" + "net/http" + "reflect" + "testing" + + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" +) + +func Test_apiRun(t *testing.T) { + tests := []struct { + name string + httpResponse *http.Response + err error + stdout string + stderr string + }{ + { + name: "success", + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`bam!`)), + }, + err: nil, + stdout: `bam!`, + stderr: ``, + }, + { + name: "failure", + httpResponse: &http.Response{ + StatusCode: 502, + Body: ioutil.NopCloser(bytes.NewBufferString(`gateway timeout`)), + }, + err: cmdutil.SilentError, + stdout: `gateway timeout`, + stderr: ``, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + + opts := ApiOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := tt.httpResponse + resp.Request = req + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + + RawFields: []string{}, + MagicFields: []string{}, + } + + err := apiRun(&opts) + if err != tt.err { + t.Errorf("expected error %v, got %v", tt.err, err) + } + + if stdout.String() != tt.stdout { + t.Errorf("expected output %q, got %q", tt.stdout, stdout.String()) + } + if stderr.String() != tt.stderr { + t.Errorf("expected error output %q, got %q", tt.stderr, stderr.String()) + } + }) + } +} + +func Test_parseFields(t *testing.T) { + io, stdin, _, _ := iostreams.Test() + fmt.Fprint(stdin, "pasted contents") + + opts := ApiOptions{ + IO: io, + RawFields: []string{ + "robot=Hubot", + "destroyer=false", + "helper=true", + "location=@work", + }, + MagicFields: []string{ + "input=@-", + "enabled=true", + "victories=123", + }, + } + + params, err := parseFields(&opts) + if err != nil { + t.Fatalf("parseFields error: %v", err) + } + + expect := map[string]interface{}{ + "robot": "Hubot", + "destroyer": "false", + "helper": "true", + "location": "@work", + "input": []byte("pasted contents"), + "enabled": true, + "victories": 123, + } + if !reflect.DeepEqual(params, expect) { + t.Errorf("expected %v, got %v", expect, params) + } +} diff --git a/pkg/cmd/api/http.go b/pkg/cmd/api/http.go index 4f9064f18..5f9ebc3ca 100644 --- a/pkg/cmd/api/http.go +++ b/pkg/cmd/api/http.go @@ -34,6 +34,8 @@ func httpRequest(client *http.Client, method string, p string, params interface{ } case io.Reader: body = pp + case nil: + body = nil default: return nil, fmt.Errorf("unrecognized parameters type: %v", params) } diff --git a/pkg/cmd/api/http_test.go b/pkg/cmd/api/http_test.go new file mode 100644 index 000000000..772a90409 --- /dev/null +++ b/pkg/cmd/api/http_test.go @@ -0,0 +1,306 @@ +package api + +import ( + "bytes" + "io/ioutil" + "net/http" + "reflect" + "testing" +) + +func Test_groupGraphQLVariables(t *testing.T) { + tests := []struct { + name string + args map[string]interface{} + want map[string]interface{} + }{ + { + name: "empty", + args: map[string]interface{}{}, + want: map[string]interface{}{}, + }, + { + name: "query only", + args: map[string]interface{}{ + "query": "QUERY", + }, + want: map[string]interface{}{ + "query": "QUERY", + }, + }, + { + name: "variables only", + args: map[string]interface{}{ + "name": "hubot", + }, + want: map[string]interface{}{ + "variables": map[string]interface{}{ + "name": "hubot", + }, + }, + }, + { + name: "query + variables", + args: map[string]interface{}{ + "query": "QUERY", + "name": "hubot", + "power": 9001, + }, + want: map[string]interface{}{ + "query": "QUERY", + "variables": map[string]interface{}{ + "name": "hubot", + "power": 9001, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := groupGraphQLVariables(tt.args); !reflect.DeepEqual(got, tt.want) { + t.Errorf("groupGraphQLVariables() = %v, want %v", got, tt.want) + } + }) + } +} + +type roundTripper func(*http.Request) (*http.Response, error) + +func (f roundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +func Test_httpRequest(t *testing.T) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + return &http.Response{Request: req}, nil + } + httpClient := http.Client{Transport: tr} + + type args struct { + client *http.Client + method string + p string + params interface{} + headers []string + } + type expects struct { + method string + u string + body string + headers string + } + tests := []struct { + name string + args args + want expects + wantErr bool + }{ + { + name: "simple GET", + args: args{ + client: &httpClient, + method: "GET", + p: "repos/octocat/spoon-knife", + params: nil, + headers: []string{}, + }, + wantErr: false, + want: expects{ + method: "GET", + u: "https://api.github.com/repos/octocat/spoon-knife", + body: "", + headers: "", + }, + }, + { + name: "GET with params", + args: args{ + client: &httpClient, + method: "GET", + p: "repos/octocat/spoon-knife", + params: map[string]interface{}{ + "a": "b", + }, + headers: []string{}, + }, + wantErr: false, + want: expects{ + method: "GET", + u: "https://api.github.com/repos/octocat/spoon-knife?a=b", + body: "", + headers: "", + }, + }, + { + name: "POST with params", + args: args{ + client: &httpClient, + method: "POST", + p: "repos", + params: map[string]interface{}{ + "a": "b", + }, + headers: []string{}, + }, + wantErr: false, + want: expects{ + method: "POST", + u: "https://api.github.com/repos", + body: `{"a":"b"}`, + headers: "Content-Type: application/json; charset=utf-8\r\n", + }, + }, + { + name: "POST GraphQL", + args: args{ + client: &httpClient, + method: "POST", + p: "graphql", + params: map[string]interface{}{ + "a": "b", + }, + headers: []string{}, + }, + wantErr: false, + want: expects{ + method: "POST", + u: "https://api.github.com/graphql", + body: `{"variables":{"a":"b"}}`, + headers: "Content-Type: application/json; charset=utf-8\r\n", + }, + }, + { + name: "POST with body and type", + args: args{ + client: &httpClient, + method: "POST", + p: "repos", + params: bytes.NewBufferString("CUSTOM"), + headers: []string{ + "content-type: text/plain", + "accept: application/json", + }, + }, + wantErr: false, + want: expects{ + method: "POST", + u: "https://api.github.com/repos", + body: `CUSTOM`, + headers: "Accept: application/json\r\nContent-Type: text/plain\r\n", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := httpRequest(tt.args.client, tt.args.method, tt.args.p, tt.args.params, tt.args.headers) + if (err != nil) != tt.wantErr { + t.Errorf("httpRequest() error = %v, wantErr %v", err, tt.wantErr) + return + } + req := got.Request + if req.Method != tt.want.method { + t.Errorf("Request.Method = %q, want %q", req.Method, tt.want.method) + } + if req.URL.String() != tt.want.u { + t.Errorf("Request.URL = %q, want %q", req.URL.String(), tt.want.u) + } + + if tt.want.body != "" { + bb, err := ioutil.ReadAll(req.Body) + if err != nil { + t.Errorf("Request.Body ReadAll error = %v", err) + return + } + if string(bb) != tt.want.body { + t.Errorf("Request.Body = %q, want %q", string(bb), tt.want.body) + } + } + + h := bytes.Buffer{} + err = req.Header.WriteSubset(&h, map[string]bool{}) + if err != nil { + t.Errorf("Request.Header WriteSubset error = %v", err) + return + } + if h.String() != tt.want.headers { + t.Errorf("Request.Header = %q, want %q", h.String(), tt.want.headers) + } + }) + } +} + +func Test_addQuery(t *testing.T) { + type args struct { + path string + params map[string]interface{} + } + tests := []struct { + name string + args args + want string + }{ + { + name: "string", + args: args{ + path: "", + params: map[string]interface{}{"a": "hello"}, + }, + want: "?a=hello", + }, + { + name: "append", + args: args{ + path: "path", + params: map[string]interface{}{"a": "b"}, + }, + want: "path?a=b", + }, + { + name: "append query", + args: args{ + path: "path?foo=bar", + params: map[string]interface{}{"a": "b"}, + }, + want: "path?foo=bar&a=b", + }, + { + name: "[]byte", + args: args{ + path: "", + params: map[string]interface{}{"a": []byte("hello")}, + }, + want: "?a=hello", + }, + { + name: "int", + args: args{ + path: "", + params: map[string]interface{}{"a": 123}, + }, + want: "?a=123", + }, + { + name: "nil", + args: args{ + path: "", + params: map[string]interface{}{"a": nil}, + }, + want: "?a=", + }, + { + name: "bool", + args: args{ + path: "", + params: map[string]interface{}{"a": true, "b": false}, + }, + want: "?a=true&b=false", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := addQuery(tt.args.path, tt.args.params); got != tt.want { + t.Errorf("addQuery() = %v, want %v", got, tt.want) + } + }) + } +} From bef62faaead78d4d4d986c557695f711df14be52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 12:54:09 +0200 Subject: [PATCH 09/12] Make `NewCmdApi` testable --- command/root.go | 29 +++++- go.mod | 2 +- go.sum | 5 +- pkg/cmd/api/api.go | 22 ++--- pkg/cmd/api/api_test.go | 185 ++++++++++++++++++++++++++++++++++++++- pkg/cmd/api/http_test.go | 8 +- pkg/cmd/api/legacy.go | 34 ------- pkg/cmdutil/factory.go | 12 +++ 8 files changed, 238 insertions(+), 59 deletions(-) delete mode 100644 pkg/cmd/api/legacy.go create mode 100644 pkg/cmdutil/factory.go diff --git a/command/root.go b/command/root.go index 93ed51109..95f993151 100644 --- a/command/root.go +++ b/command/root.go @@ -3,6 +3,7 @@ package command import ( "fmt" "io" + "net/http" "os" "regexp" "runtime/debug" @@ -14,6 +15,7 @@ import ( "github.com/cli/cli/internal/ghrepo" apiCmd "github.com/cli/cli/pkg/cmd/api" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" "github.com/spf13/cobra" @@ -64,7 +66,19 @@ func init() { return &cmdutil.FlagError{Err: err} }) - RootCmd.AddCommand(apiCmd.NewCmdApi()) + // TODO: iron out how a factory incorporates context + cmdFactory := &cmdutil.Factory{ + IOStreams: iostreams.System(), + HttpClient: func() (*http.Client, error) { + ctx := context.New() + token, err := ctx.AuthToken() + if err != nil { + return nil, err + } + return httpClient(token), nil + }, + } + RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) } // RootCmd is the entry point of command-line execution @@ -119,6 +133,19 @@ func contextForCommand(cmd *cobra.Command) context.Context { return ctx } +// for cmdutil-powered commands +func httpClient(token string) *http.Client { + var opts []api.ClientOption + if verbose := os.Getenv("DEBUG"); verbose != "" { + opts = append(opts, apiVerboseLog()) + } + opts = append(opts, + api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), + api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)), + ) + return api.NewHTTPClient(opts...) +} + // overridden in tests var apiClientForContext = func(ctx context.Context) (*api.Client, error) { token, err := ctx.AuthToken() diff --git a/go.mod b/go.mod index 193320caa..407c8ab8a 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( github.com/shurcooL/graphql v0.0.0-20181231061246-d48a9a75455f // indirect github.com/spf13/cobra v0.0.6 github.com/spf13/pflag v1.0.5 - github.com/stretchr/testify v1.4.0 // indirect + github.com/stretchr/testify v1.5.1 golang.org/x/crypto v0.0.0-20200219234226-1ad67e1f0ef4 golang.org/x/net v0.0.0-20200219183655-46282727080f // indirect golang.org/x/text v0.3.2 diff --git a/go.sum b/go.sum index 807e68f62..09c195c5d 100644 --- a/go.sum +++ b/go.sum @@ -167,13 +167,14 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/spf13/viper v1.4.0/go.mod h1:PTJ7Z/lr49W6bUbkmS1V3by4uWynFiR9p7+dSq/yZzE= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.1.1 h1:2vfRuCMp5sSVIDSqO8oNnWJq7mPa6KVP3iPIwFBuy8A= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0 h1:TivCn/peBQ7UY8ooIcPgZFpTNSz0Q2U6UrFlUfqbe0Q= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= -github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= -github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= +github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4= +github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U= github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 02eac439b..89dc53103 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -9,7 +9,6 @@ import ( "strconv" "strings" - "github.com/cli/cli/context" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/spf13/cobra" @@ -29,8 +28,12 @@ type ApiOptions struct { HttpClient func() (*http.Client, error) } -func NewCmdApi() *cobra.Command { - opts := ApiOptions{} +func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command { + opts := ApiOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + } + cmd := &cobra.Command{ Use: "api ", Short: "Make an authenticated GitHub API request", @@ -58,18 +61,9 @@ on the format of the value: opts.RequestPath = args[0] opts.RequestMethodPassed = c.Flags().Changed("method") - // TODO: pass in via caller - opts.IO = iostreams.System() - - opts.HttpClient = func() (*http.Client, error) { - ctx := context.New() - token, err := ctx.AuthToken() - if err != nil { - return nil, err - } - return apiClientFromContext(token), nil + if runF != nil { + return runF(&opts) } - return apiRun(&opts) }, } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index ad8211761..8b906d3ba 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -3,15 +3,115 @@ package api import ( "bytes" "fmt" + "io" "io/ioutil" "net/http" - "reflect" + "os" "testing" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" ) +func Test_NewCmdApi(t *testing.T) { + f := &cmdutil.Factory{} + + tests := []struct { + name string + cli string + wants ApiOptions + wantsErr bool + }{ + { + name: "no flags", + cli: "graphql", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "graphql", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "override method", + cli: "repos/octocat/Spoon-Knife -XDELETE", + wants: ApiOptions{ + RequestMethod: "DELETE", + RequestMethodPassed: true, + RequestPath: "repos/octocat/Spoon-Knife", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "with fields", + cli: "graphql -f query=QUERY -F body=@file.txt", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "graphql", + RawFields: []string{"query=QUERY"}, + MagicFields: []string{"body=@file.txt"}, + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + }, + wantsErr: false, + }, + { + name: "with headers", + cli: "user -H 'accept: text/plain' -i", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "user", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string{"accept: text/plain"}, + ShowResponseHeaders: true, + }, + wantsErr: false, + }, + { + name: "no arguments", + cli: "", + wantsErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := NewCmdApi(f, func(o *ApiOptions) error { + assert.Equal(t, tt.wants.RequestMethod, o.RequestMethod) + assert.Equal(t, tt.wants.RequestMethodPassed, o.RequestMethodPassed) + assert.Equal(t, tt.wants.RequestPath, o.RequestPath) + assert.Equal(t, tt.wants.RawFields, o.RawFields) + assert.Equal(t, tt.wants.MagicFields, o.MagicFields) + assert.Equal(t, tt.wants.RequestHeaders, o.RequestHeaders) + assert.Equal(t, tt.wants.ShowResponseHeaders, o.ShowResponseHeaders) + return nil + }) + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + cmd.SetArgs(argv) + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + }) + } +} + func Test_apiRun(t *testing.T) { tests := []struct { name string @@ -30,6 +130,16 @@ func Test_apiRun(t *testing.T) { stdout: `bam!`, stderr: ``, }, + { + name: "success 204", + httpResponse: &http.Response{ + StatusCode: 204, + Body: nil, + }, + err: nil, + stdout: ``, + stderr: ``, + }, { name: "failure", httpResponse: &http.Response{ @@ -109,7 +219,76 @@ func Test_parseFields(t *testing.T) { "enabled": true, "victories": 123, } - if !reflect.DeepEqual(params, expect) { - t.Errorf("expected %v, got %v", expect, params) + assert.Equal(t, expect, params) +} + +func Test_magicFieldValue(t *testing.T) { + f, err := ioutil.TempFile("", "gh-test") + if err != nil { + t.Fatal(err) + } + fmt.Fprint(f, "file contents") + f.Close() + t.Cleanup(func() { os.Remove(f.Name()) }) + + type args struct { + v string + stdin io.ReadCloser + } + tests := []struct { + name string + args args + want interface{} + wantErr bool + }{ + { + name: "string", + args: args{v: "hello"}, + want: "hello", + wantErr: false, + }, + { + name: "bool true", + args: args{v: "true"}, + want: true, + wantErr: false, + }, + { + name: "bool false", + args: args{v: "false"}, + want: false, + wantErr: false, + }, + { + name: "null", + args: args{v: "null"}, + want: nil, + wantErr: false, + }, + { + name: "file", + args: args{v: "@" + f.Name()}, + want: []byte("file contents"), + wantErr: false, + }, + { + name: "file error", + args: args{v: "@"}, + want: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := magicFieldValue(tt.args.v, tt.args.stdin) + if (err != nil) != tt.wantErr { + t.Errorf("magicFieldValue() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr { + return + } + assert.Equal(t, tt.want, got) + }) } } diff --git a/pkg/cmd/api/http_test.go b/pkg/cmd/api/http_test.go index 772a90409..e2ba8680b 100644 --- a/pkg/cmd/api/http_test.go +++ b/pkg/cmd/api/http_test.go @@ -4,8 +4,9 @@ import ( "bytes" "io/ioutil" "net/http" - "reflect" "testing" + + "github.com/stretchr/testify/assert" ) func Test_groupGraphQLVariables(t *testing.T) { @@ -57,9 +58,8 @@ func Test_groupGraphQLVariables(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := groupGraphQLVariables(tt.args); !reflect.DeepEqual(got, tt.want) { - t.Errorf("groupGraphQLVariables() = %v, want %v", got, tt.want) - } + got := groupGraphQLVariables(tt.args) + assert.Equal(t, tt.want, got) }) } } diff --git a/pkg/cmd/api/legacy.go b/pkg/cmd/api/legacy.go deleted file mode 100644 index 5c8429727..000000000 --- a/pkg/cmd/api/legacy.go +++ /dev/null @@ -1,34 +0,0 @@ -package api - -import ( - "fmt" - "net/http" - "os" - "strings" - - "github.com/cli/cli/api" - "github.com/cli/cli/utils" -) - -// TODO -func apiClientFromContext(token string) *http.Client { - var opts []api.ClientOption - if verbose := os.Getenv("DEBUG"); verbose != "" { - opts = append(opts, apiVerboseLog()) - } - opts = append(opts, - api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), - // FIXME - // api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", command.Version)), - // antiope-preview: Checks - api.AddHeader("Accept", "application/vnd.github.antiope-preview+json"), - ) - return api.NewHTTPClient(opts...) -} - -// TODO -func apiVerboseLog() api.ClientOption { - logTraffic := strings.Contains(os.Getenv("DEBUG"), "api") - colorize := utils.IsTerminal(os.Stderr) - return api.VerboseLog(utils.NewColorable(os.Stderr), logTraffic, colorize) -} diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go new file mode 100644 index 000000000..578b29561 --- /dev/null +++ b/pkg/cmdutil/factory.go @@ -0,0 +1,12 @@ +package cmdutil + +import ( + "net/http" + + "github.com/cli/cli/pkg/iostreams" +) + +type Factory struct { + IOStreams *iostreams.IOStreams + HttpClient func() (*http.Client, error) +} From 292b4284655f5d276260aeddc32557d5139b7788 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 16:28:27 +0200 Subject: [PATCH 10/12] Add test for showing response headers --- pkg/cmd/api/api.go | 4 ++-- pkg/cmd/api/api_test.go | 38 ++++++++++++++++++++++++-------------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 89dc53103..4158ff8c1 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -99,9 +99,9 @@ func apiRun(opts *ApiOptions) error { if opts.ShowResponseHeaders { for name, vals := range resp.Header { - fmt.Printf("%s: %s\r\n", name, strings.Join(vals, ", ")) + fmt.Fprintf(opts.IO.Out, "%s: %s\r\n", name, strings.Join(vals, ", ")) } - fmt.Print("\r\n") + fmt.Fprint(opts.IO.Out, "\r\n") } if resp.StatusCode == 204 { diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 8b906d3ba..27b342f7d 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -115,6 +115,7 @@ func Test_NewCmdApi(t *testing.T) { func Test_apiRun(t *testing.T) { tests := []struct { name string + options ApiOptions httpResponse *http.Response err error stdout string @@ -130,6 +131,20 @@ func Test_apiRun(t *testing.T) { stdout: `bam!`, stderr: ``, }, + { + name: "show response headers", + options: ApiOptions{ + ShowResponseHeaders: true, + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + err: nil, + stdout: "Content-Type: text/plain\r\n\r\nbody", + stderr: ``, + }, { name: "success 204", httpResponse: &http.Response{ @@ -156,22 +171,17 @@ func Test_apiRun(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, _, stdout, stderr := iostreams.Test() - opts := ApiOptions{ - IO: io, - HttpClient: func() (*http.Client, error) { - var tr roundTripper = func(req *http.Request) (*http.Response, error) { - resp := tt.httpResponse - resp.Request = req - return resp, nil - } - return &http.Client{Transport: tr}, nil - }, - - RawFields: []string{}, - MagicFields: []string{}, + tt.options.IO = io + tt.options.HttpClient = func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := tt.httpResponse + resp.Request = req + return resp, nil + } + return &http.Client{Transport: tr}, nil } - err := apiRun(&opts) + err := apiRun(&tt.options) if err != tt.err { t.Errorf("expected error %v, got %v", tt.err, err) } From ea3a55c3d6c024ca469a657e59cddb4473339887 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 20 May 2020 16:28:35 +0200 Subject: [PATCH 11/12] Ensure that cobra command tests don't write to system stdout/stderr --- pkg/cmd/api/api_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 27b342f7d..8149f5906 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -102,6 +102,9 @@ func Test_NewCmdApi(t *testing.T) { argv, err := shlex.Split(tt.cli) assert.NoError(t, err) cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) _, err = cmd.ExecuteC() if tt.wantsErr { assert.Error(t, err) From 13ba0aa56ec84818556cdea4e79b1fab4a2445d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 27 May 2020 13:09:05 +0200 Subject: [PATCH 12/12] Respect GITHUB_TOKEN in `api` command --- command/root.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/command/root.go b/command/root.go index 2f9a1fe3e..63ef4c4f9 100644 --- a/command/root.go +++ b/command/root.go @@ -71,10 +71,14 @@ func init() { cmdFactory := &cmdutil.Factory{ IOStreams: iostreams.System(), HttpClient: func() (*http.Client, error) { - ctx := context.New() - token, err := ctx.AuthToken() - if err != nil { - return nil, err + token := os.Getenv("GITHUB_TOKEN") + if len(token) == 0 { + ctx := context.New() + var err error + token, err = ctx.AuthToken() + if err != nil { + return nil, err + } } return httpClient(token), nil },