From f41876d64cc64c92479073fb1afda427416835e8 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 15 Feb 2024 00:59:12 -0800 Subject: [PATCH] Resolve PR comments --- internal/jsonmerge/nop.go | 19 +++ internal/jsonmerge/nop_test.go | 114 +++++++++++++ pkg/cmd/api/api.go | 38 +++-- pkg/cmd/api/api_test.go | 293 ++++++++++++++------------------- 4 files changed, 283 insertions(+), 181 deletions(-) create mode 100644 internal/jsonmerge/nop.go create mode 100644 internal/jsonmerge/nop_test.go diff --git a/internal/jsonmerge/nop.go b/internal/jsonmerge/nop.go new file mode 100644 index 000000000..d11a545b9 --- /dev/null +++ b/internal/jsonmerge/nop.go @@ -0,0 +1,19 @@ +package jsonmerge + +import ( + "io" +) + +type nopMerger struct{} + +func NewNopMerger() Merger { + return &nopMerger{} +} + +func (m *nopMerger) NewPage(r io.Reader, _ bool) io.ReadCloser { + return io.NopCloser(r) +} + +func (m *nopMerger) Close() error { + return nil +} diff --git a/internal/jsonmerge/nop_test.go b/internal/jsonmerge/nop_test.go new file mode 100644 index 000000000..713ac5501 --- /dev/null +++ b/internal/jsonmerge/nop_test.go @@ -0,0 +1,114 @@ +package jsonmerge + +import ( + "bytes" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNopMerger_nothingWritten(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + require.NoError(t, merger.Close()) + assert.Equal(t, ``, w.String()) +} + +func TestNopMerger_singleEmptyObject(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + r1 := bytes.NewBufferString(`{}`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `{}`, w.String()) + + require.NoError(t, merger.Close()) + assert.Equal(t, `{}`, w.String()) +} + +func TestNopMerger_finalEmptyObject(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + r1 := bytes.NewBufferString(`{"a":1,"b":2}`) + p1 := merger.NewPage(r1, false) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(13), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `{"a":1,"b":2}`, w.String()) + + r2 := bytes.NewBufferString(`{"c":3}`) + p2 := merger.NewPage(r2, true) + n, err = io.Copy(w, p2) + require.NoError(t, err) + assert.Equal(t, int64(7), n) + assert.NoError(t, p2.Close()) + assert.Equal(t, `{"a":1,"b":2}{"c":3}`, w.String()) + + require.NoError(t, merger.Close()) + assert.Equal(t, `{"a":1,"b":2}{"c":3}`, w.String()) +} + +func TestNopMerger_invalidJSON(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + r1 := bytes.NewBufferString(`invalid`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(7), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `invalid`, w.String()) + + require.NoError(t, merger.Close()) + assert.Equal(t, `invalid`, w.String()) +} + +func TestNopMerger_singleEmptyArray(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + r1 := bytes.NewBufferString(`[]`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `[]`, w.String()) + + require.NoError(t, merger.Close()) + assert.Equal(t, `[]`, w.String()) +} + +func TestNopMerger_finalEmptyArray(t *testing.T) { + w := &bytes.Buffer{} + merger := NewNopMerger() + + r1 := bytes.NewBufferString(`["a","b"]`) + p1 := merger.NewPage(r1, false) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(9), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `["a","b"]`, w.String()) + + r2 := bytes.NewBufferString(`[]`) + p2 := merger.NewPage(r2, true) + n, err = io.Copy(w, p2) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + assert.NoError(t, p2.Close()) + assert.Equal(t, `["a","b"][]`, w.String()) + + require.NoError(t, merger.Close()) + assert.Equal(t, `["a","b"][]`, w.String()) +} diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 3a2ccd4a1..35b99eef6 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -159,7 +159,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command ' # list all repositories for a user - $ gh api graphql --paginate --merge-pages -f query=' + $ gh api graphql --paginate -f query=' query($endCursor: String) { viewer { repositories(first: 100, after: $endCursor) { @@ -172,6 +172,23 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command } } ' + + # get the percentage of forks for the current user + # without --merge-pages you will get a different percentage for each page + $ gh api graphql --paginate --merge-pages -f query=' + query($endCursor: String) { + viewer { + repositories(first: 100, after: $endCursor) { + nodes { isFork } + pageInfo { + hasNextPage + endCursor + } + } + } + } + ' | jq 'def count(s): reduce s as $_ (0;.+1); + .data.viewer.repositories.nodes as $r | count(select($r[].isFork)) / count($r[])' `), Annotations: map[string]string{ "help:environment": heredoc.Doc(` @@ -292,14 +309,19 @@ func apiRun(opts *ApiOptions) error { // Merge JSON arrays and objects if paginating without filtering or templating. // MergePages retains compatibility with older versions but may be the default behavior with future major releases. - if opts.Paginate && opts.MergePages && opts.FilterOutput == "" && opts.Template == "" { - if isGraphQL { - opts.merger = jsonmerge.NewObjectMerger(bodyWriter) - } else { + if opts.Paginate && opts.FilterOutput == "" && opts.Template == "" { + if !isGraphQL { opts.merger = jsonmerge.NewArrayMerger() + } else if opts.MergePages { + opts.merger = jsonmerge.NewObjectMerger(bodyWriter) } } + if opts.merger == nil { + opts.merger = jsonmerge.NewNopMerger() + } + defer opts.merger.Close() + if opts.RequestInputFile != "" { file, size, err := openUserFile(opts.RequestInputFile, opts.IO.In) if err != nil { @@ -400,10 +422,6 @@ func apiRun(opts *ApiOptions) error { } } - if opts.merger != nil { - return opts.merger.Close() - } - return tmpl.Flush() } @@ -455,7 +473,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW } else if isJSON && opts.IO.ColorEnabled() { err = jsoncolor.Write(bodyWriter, responseBody, " ") } else { - if isJSON && opts.merger != nil && !opts.ShowResponseHeaders { + if isJSON && opts.Paginate && !opts.ShowResponseHeaders { responseBody = opts.merger.NewPage(responseBody, isLastPage) } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index f5145b3c3..0d02eff61 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -686,7 +686,7 @@ func Test_apiRun_paginationREST(t *testing.T) { Proto: "HTTP/1.1", Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":1}]`)), + Body: io.NopCloser(bytes.NewBufferString(`{"page":1}`)), Header: http.Header{ "Content-Type": []string{"application/json"}, "Link": []string{`; rel="next", ; rel="last"`}, @@ -697,7 +697,7 @@ func Test_apiRun_paginationREST(t *testing.T) { Proto: "HTTP/1.1", Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), + Body: io.NopCloser(bytes.NewBufferString(`{"page":2}`)), Header: http.Header{ "Content-Type": []string{"application/json"}, "Link": []string{`; rel="next", ; rel="last"`}, @@ -708,7 +708,7 @@ func Test_apiRun_paginationREST(t *testing.T) { Proto: "HTTP/1.1", Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), + Body: io.NopCloser(bytes.NewBufferString(`{"page":3}`)), Header: http.Header{ "Content-Type": []string{"application/json"}, "X-Github-Request-Id": []string{"3"}, @@ -741,153 +741,7 @@ func Test_apiRun_paginationREST(t *testing.T) { err := apiRun(&options) assert.NoError(t, err) - assert.Equal(t, `[{"page":1}][{"page":2}][{"page":3}]`, stdout.String(), "stdout") - assert.Equal(t, "", stderr.String(), "stderr") - - assert.Equal(t, "https://api.github.com/issues?page=1&per_page=50", responses[0].Request.URL.String()) - assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=2", responses[1].Request.URL.String()) - assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) -} - -func Test_apiRun_paginationREST_merge(t *testing.T) { - ios, _, stdout, stderr := iostreams.Test() - - requestCount := 0 - responses := []*http.Response{ - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":1}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"1"}, - }, - }, - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"2"}, - }, - }, - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "X-Github-Request-Id": []string{"3"}, - }, - }, - } - - options := ApiOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - var tr roundTripper = func(req *http.Request) (*http.Response, error) { - resp := responses[requestCount] - resp.Request = req - requestCount++ - return resp, nil - } - return &http.Client{Transport: tr}, nil - }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, - - RequestMethod: "GET", - RequestMethodPassed: true, - RequestPath: "issues", - Paginate: true, - MergePages: true, - RawFields: []string{"per_page=50", "page=1"}, - } - - err := apiRun(&options) - assert.NoError(t, err) - - assert.Equal(t, `[{"page":1},{"page":2},{"page":3}]`, stdout.String(), "stdout") - assert.Equal(t, "", stderr.String(), "stderr") - - assert.Equal(t, "https://api.github.com/issues?page=1&per_page=50", responses[0].Request.URL.String()) - assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=2", responses[1].Request.URL.String()) - assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) -} - -func Test_apiRun_paginationREST_with_headers(t *testing.T) { - ios, _, stdout, stderr := iostreams.Test() - - requestCount := 0 - responses := []*http.Response{ - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":1}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"1"}, - }, - }, - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"2"}, - }, - }, - { - Proto: "HTTP/1.1", - Status: "200 OK", - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "X-Github-Request-Id": []string{"3"}, - }, - }, - } - - options := ApiOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - var tr roundTripper = func(req *http.Request) (*http.Response, error) { - resp := responses[requestCount] - resp.Request = req - requestCount++ - return resp, nil - } - return &http.Client{Transport: tr}, nil - }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, - - RequestMethod: "GET", - RequestMethodPassed: true, - RequestPath: "issues", - Paginate: true, - RawFields: []string{"per_page=50", "page=1"}, - ShowResponseHeaders: true, - } - - err := apiRun(&options) - assert.NoError(t, err) - - assert.Equal(t, "HTTP/1.1 200 OK\nContent-Type: application/json\r\nLink: ; rel=\"next\", ; rel=\"last\"\r\nX-Github-Request-Id: 1\r\n\r\n[{\"page\":1}]\nHTTP/1.1 200 OK\nContent-Type: application/json\r\nLink: ; rel=\"next\", ; rel=\"last\"\r\nX-Github-Request-Id: 2\r\n\r\n[{\"page\":2}]\nHTTP/1.1 200 OK\nContent-Type: application/json\r\nX-Github-Request-Id: 3\r\n\r\n[{\"page\":3}]", stdout.String(), "stdout") + assert.Equal(t, `{"page":1}{"page":2}{"page":3}`, stdout.String(), "stdout") assert.Equal(t, "", stderr.String(), "stderr") assert.Equal(t, "https://api.github.com/issues?page=1&per_page=50", responses[0].Request.URL.String()) @@ -959,7 +813,7 @@ func Test_apiRun_arrayPaginationREST(t *testing.T) { err := apiRun(&options) assert.NoError(t, err) - assert.Equal(t, `[{"item":1},{"item":2}][{"item":3},{"item":4}][{"item":5}][]`, stdout.String(), "stdout") + assert.Equal(t, `[{"item":1},{"item":2},{"item":3},{"item":4},{"item":5} ]`, stdout.String(), "stdout") assert.Equal(t, "", stderr.String(), "stderr") assert.Equal(t, "https://api.github.com/issues?page=1&per_page=50", responses[0].Request.URL.String()) @@ -967,41 +821,41 @@ func Test_apiRun_arrayPaginationREST(t *testing.T) { assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) } -func Test_apiRun_arrayPaginationREST_merge(t *testing.T) { +func Test_apiRun_arrayPaginationREST_with_headers(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() - ios.SetStdoutTTY(false) requestCount := 0 responses := []*http.Response{ { + Proto: "HTTP/1.1", + Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":1},{"item":2}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"page":1}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, + "X-Github-Request-Id": []string{"1"}, }, }, { + Proto: "HTTP/1.1", + Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":3},{"item":4}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, + "X-Github-Request-Id": []string{"2"}, }, }, { + Proto: "HTTP/1.1", + Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":5}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, - }, - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, + "Content-Type": []string{"application/json"}, + "X-Github-Request-Id": []string{"3"}, }, }, } @@ -1025,14 +879,14 @@ func Test_apiRun_arrayPaginationREST_merge(t *testing.T) { RequestMethodPassed: true, RequestPath: "issues", Paginate: true, - MergePages: true, RawFields: []string{"per_page=50", "page=1"}, + ShowResponseHeaders: true, } err := apiRun(&options) assert.NoError(t, err) - assert.Equal(t, `[{"item":1},{"item":2},{"item":3},{"item":4},{"item":5} ]`, stdout.String(), "stdout") + assert.Equal(t, "HTTP/1.1 200 OK\nContent-Type: application/json\r\nLink: ; rel=\"next\", ; rel=\"last\"\r\nX-Github-Request-Id: 1\r\n\r\n[{\"page\":1}]\nHTTP/1.1 200 OK\nContent-Type: application/json\r\nLink: ; rel=\"next\", ; rel=\"last\"\r\nX-Github-Request-Id: 2\r\n\r\n[{\"page\":2}]\nHTTP/1.1 200 OK\nContent-Type: application/json\r\nX-Github-Request-Id: 3\r\n\r\n[{\"page\":3}]", stdout.String(), "stdout") assert.Equal(t, "", stderr.String(), "stderr") assert.Equal(t, "https://api.github.com/issues?page=1&per_page=50", responses[0].Request.URL.String()) @@ -1233,6 +1087,103 @@ func Test_apiRun_paginationGraphQL_merge(t *testing.T) { assert.Equal(t, "PAGE1_END", endCursor) } +func Test_apiRun_paginationGraphQL_merge_tty(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + + // Backcompat: setting color-enabled disables merging, but not TTY. + ios.SetStdoutTTY(true) + + requestCount := 0 + responses := []*http.Response{ + { + StatusCode: 200, + Header: http.Header{"Content-Type": []string{`application/json`}}, + Body: io.NopCloser(bytes.NewBufferString(heredoc.Doc(` + { + "data": { + "nodes": ["page one"], + "pageInfo": { + "endCursor": "PAGE1_END", + "hasNextPage": true + } + } + }`))), + }, + { + StatusCode: 200, + Header: http.Header{"Content-Type": []string{`application/json`}}, + Body: io.NopCloser(bytes.NewBufferString(heredoc.Doc(` + { + "data": { + "nodes": ["page two"], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } + } + }`))), + }, + } + + options := ApiOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := responses[requestCount] + resp.Request = req + requestCount++ + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + + RawFields: []string{"foo=bar"}, + RequestMethod: "POST", + RequestPath: "graphql", + Paginate: true, + MergePages: true, + } + + err := apiRun(&options) + require.NoError(t, err) + + assert.JSONEq(t, stdout.String(), `{ + "data": { + "nodes": [ + "page one", + "page two" + ], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } + } + }`) + assert.Equal(t, "", stderr.String(), "stderr") + + var requestData struct { + Variables map[string]interface{} + } + + bb, err := io.ReadAll(responses[0].Request.Body) + require.NoError(t, err) + err = json.Unmarshal(bb, &requestData) + require.NoError(t, err) + _, hasCursor := requestData.Variables["endCursor"].(string) + assert.Equal(t, false, hasCursor) + + bb, err = io.ReadAll(responses[1].Request.Body) + require.NoError(t, err) + err = json.Unmarshal(bb, &requestData) + require.NoError(t, err) + endCursor, hasCursor := requestData.Variables["endCursor"].(string) + assert.Equal(t, true, hasCursor) + assert.Equal(t, "PAGE1_END", endCursor) +} + func Test_apiRun_paginated_template(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(true)