From 8e27e9a8cd27cf6d748f1cf94a6dcc8a604a6e75 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 25 Jan 2024 02:26:16 -0800 Subject: [PATCH 01/11] Merge JSON responses from `gh api` Partly resolves cli/cli#1268 and replaces cli/cli#5652. Requires cli/go-gh#148 to be merged and optionally released. --- pkg/cmd/api/api.go | 53 ++++++++++++++++++++++++++------------- pkg/cmd/api/api_test.go | 14 +++++++++-- pkg/cmd/api/pagination.go | 40 ----------------------------- 3 files changed, 48 insertions(+), 59 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 6e366dfa70c..8b42c040e21 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -25,6 +25,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/jsoncolor" "github.com/cli/go-gh/v2/pkg/jq" + "github.com/cli/go-gh/v2/pkg/jsonmerge" "github.com/cli/go-gh/v2/pkg/template" "github.com/spf13/cobra" ) @@ -36,6 +37,7 @@ type ApiOptions struct { Config func() (config.Config, error) HttpClient func() (*http.Client, error) IO *iostreams.IOStreams + Merger jsonmerge.Merger Hostname string RequestMethod string @@ -265,10 +267,30 @@ func apiRun(opts *ApiOptions) error { method = "POST" } + var bodyWriter io.Writer = opts.IO.Out + var headersWriter io.Writer = opts.IO.Out + if opts.Silent { + bodyWriter = io.Discard + } + if opts.Verbose { + // httpClient handles output when verbose flag is specified. + bodyWriter = io.Discard + headersWriter = io.Discard + } + if opts.Paginate && !isGraphQL { requestPath = addPerPage(requestPath, 100, params) } + // Merge JSON arrays and object if paginating without a filter or template. + if opts.Paginate && opts.FilterOutput == "" && opts.Template == "" { + if isGraphQL { + opts.Merger = jsonmerge.NewObjectMerger(bodyWriter) + } else { + opts.Merger = jsonmerge.NewArrayMerger() + } + } + if opts.RequestInputFile != "" { file, size, err := openUserFile(opts.RequestInputFile, opts.IO.In) if err != nil { @@ -322,17 +344,6 @@ func apiRun(opts *ApiOptions) error { } } - var bodyWriter io.Writer = opts.IO.Out - var headersWriter io.Writer = opts.IO.Out - if opts.Silent { - bodyWriter = io.Discard - } - if opts.Verbose { - // httpClient handles output when verbose flag is specified. - bodyWriter = io.Discard - headersWriter = io.Discard - } - host, _ := cfg.Authentication().DefaultHost() if opts.Hostname != "" { @@ -380,6 +391,10 @@ func apiRun(opts *ApiOptions) error { } } + if opts.Merger != nil { + return opts.Merger.Close() + } + return tmpl.Flush() } @@ -431,14 +446,18 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW } else if isJSON && opts.IO.ColorEnabled() { err = jsoncolor.Write(bodyWriter, responseBody, " ") } else { - if isJSON && opts.Paginate && !isGraphQLPaginate && !opts.ShowResponseHeaders { - responseBody = &paginatedArrayReader{ - Reader: responseBody, - isFirstPage: isFirstPage, - isLastPage: isLastPage, - } + if isJSON && opts.Merger != nil && !opts.ShowResponseHeaders { + responseBody = opts.Merger.NewPage(responseBody, isLastPage) } + _, err = io.Copy(bodyWriter, responseBody) + if err != nil { + return + } + + if closer, ok := responseBody.(io.ReadCloser); ok { + err = closer.Close() + } } if err != nil { return diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 3ffc6a0b362..778cbd3e191 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -832,8 +832,18 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { err := apiRun(&options) require.NoError(t, err) - assert.Contains(t, stdout.String(), `"page one"`) - assert.Contains(t, stdout.String(), `"page two"`) + 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 { diff --git a/pkg/cmd/api/pagination.go b/pkg/cmd/api/pagination.go index 057a5a7facb..65d816480e4 100644 --- a/pkg/cmd/api/pagination.go +++ b/pkg/cmd/api/pagination.go @@ -106,43 +106,3 @@ func addPerPage(p string, perPage int, params map[string]interface{}) string { return fmt.Sprintf("%s%sper_page=%d", p, sep, perPage) } - -// paginatedArrayReader wraps a Reader to omit the opening and/or the closing square bracket of a -// JSON array in order to apply pagination context between multiple API requests. -type paginatedArrayReader struct { - io.Reader - isFirstPage bool - isLastPage bool - - isSubsequentRead bool - cachedByte byte -} - -func (r *paginatedArrayReader) Read(p []byte) (int, error) { - var n int - var err error - if r.cachedByte != 0 && len(p) > 0 { - p[0] = r.cachedByte - n, err = r.Reader.Read(p[1:]) - n += 1 - r.cachedByte = 0 - } else { - n, err = r.Reader.Read(p) - } - if !r.isSubsequentRead && !r.isFirstPage && n > 0 && p[0] == '[' { - if n > 1 && p[1] == ']' { - // empty array case - p[0] = ' ' - } else { - // avoid starting a new array and continue with a comma instead - p[0] = ',' - } - } - if !r.isLastPage && n > 0 && p[n-1] == ']' { - // avoid closing off an array in case we determine we are at EOF - r.cachedByte = p[n-1] - n -= 1 - } - r.isSubsequentRead = true - return n, err -} From a65103667effef9233b0f91bb260daf1a8f9d9ea Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Tue, 30 Jan 2024 22:14:43 -0800 Subject: [PATCH 02/11] Move jsonmerge package to internal --- go.mod | 1 + go.sum | 2 + internal/jsonmerge/array.go | 76 ++++++++++++++++++++++++ internal/jsonmerge/array_test.go | 86 +++++++++++++++++++++++++++ internal/jsonmerge/merge.go | 12 ++++ internal/jsonmerge/object.go | 66 +++++++++++++++++++++ internal/jsonmerge/object_test.go | 98 +++++++++++++++++++++++++++++++ pkg/cmd/api/api.go | 2 +- 8 files changed, 342 insertions(+), 1 deletion(-) create mode 100644 internal/jsonmerge/array.go create mode 100644 internal/jsonmerge/array_test.go create mode 100644 internal/jsonmerge/merge.go create mode 100644 internal/jsonmerge/object.go create mode 100644 internal/jsonmerge/object_test.go diff --git a/go.mod b/go.mod index ba96c7cb01f..9381694c1a3 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module github.com/cli/cli/v2 go 1.22 require ( + dario.cat/mergo v1.0.0 github.com/AlecAivazis/survey/v2 v2.3.7 github.com/MakeNowJust/heredoc v1.0.0 github.com/briandowns/spinner v1.18.1 diff --git a/go.sum b/go.sum index 1c759cbebf1..bfd00f3a81e 100644 --- a/go.sum +++ b/go.sum @@ -7,6 +7,8 @@ cloud.google.com/go/iam v1.1.5 h1:1jTsCu4bcsNsE4iiqNT5SHwrDRCfRmIaaaVFhRveTJI= cloud.google.com/go/iam v1.1.5/go.mod h1:rB6P/Ic3mykPbFio+vo7403drjlgvoWfYpJhMXEbzv8= cloud.google.com/go/kms v1.15.5 h1:pj1sRfut2eRbD9pFRjNnPNg/CzJPuQAzUujMIM1vVeM= cloud.google.com/go/kms v1.15.5/go.mod h1:cU2H5jnp6G2TDpUGZyqTCoy1n16fbubHZjmVXSMtwDI= +dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= +dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA= filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4= github.com/AdamKorcz/go-fuzz-headers-1 v0.0.0-20230919221257-8b5d3ce2d11d h1:zjqpY4C7H15HjRPEenkS4SAn3Jy2eRRjkjZbGR30TOg= diff --git a/internal/jsonmerge/array.go b/internal/jsonmerge/array.go new file mode 100644 index 00000000000..3a8c2c744e7 --- /dev/null +++ b/internal/jsonmerge/array.go @@ -0,0 +1,76 @@ +package jsonmerge + +import "io" + +type arrayMerger struct { + isFirstPage bool +} + +// NewArrayMerger creates a Merger for JSON arrays. +func NewArrayMerger() Merger { + return &arrayMerger{ + isFirstPage: true, + } +} + +func (merger *arrayMerger) NewPage(r io.Reader, isLastPage bool) io.ReadCloser { + return &arrayMergerPage{ + merger: merger, + Reader: r, + isLastPage: isLastPage, + } +} + +func (m *arrayMerger) Close() error { + // arrayMerger merges when reading, so any output was already written + // and there's nothing to do on Close. + return nil +} + +type arrayMergerPage struct { + merger *arrayMerger + + io.Reader + isLastPage bool + + isSubsequentRead bool + cachedByte byte +} + +func (page *arrayMergerPage) Read(p []byte) (int, error) { + var n int + var err error + + if page.cachedByte != 0 && len(p) > 0 { + p[0] = page.cachedByte + n, err = page.Reader.Read(p[1:]) + n += 1 + page.cachedByte = 0 + } else { + n, err = page.Reader.Read(p) + } + + if !page.isSubsequentRead && !page.merger.isFirstPage && n > 0 && p[0] == '[' { + if n > 1 && p[1] == ']' { + // Empty array case. + p[0] = ' ' + } else { + // Avoid starting a new array and continue with a comma instead. + p[0] = ',' + } + } + + if !page.isLastPage && n > 0 && p[n-1] == ']' { + // Avoid closing off an array in case we determine we are at EOF. + page.cachedByte = p[n-1] + n -= 1 + } + + page.isSubsequentRead = true + return n, err +} + +func (page *arrayMergerPage) Close() error { + page.merger.isFirstPage = false + return nil +} diff --git a/internal/jsonmerge/array_test.go b/internal/jsonmerge/array_test.go new file mode 100644 index 00000000000..8106ded30d8 --- /dev/null +++ b/internal/jsonmerge/array_test.go @@ -0,0 +1,86 @@ +package jsonmerge + +import ( + "bytes" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestArrayMerger_singleEmptyArray(t *testing.T) { + merger := NewArrayMerger() + w := &bytes.Buffer{} + + 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()) +} + +func TestArrayMerger_finalEmptyArray(t *testing.T) { + merger := NewArrayMerger() + w := &bytes.Buffer{} + + r1 := bytes.NewBufferString(`["a","b"]`) + p1 := merger.NewPage(r1, false) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(8), 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()) +} + +func TestArrayMerger_multiplePages(t *testing.T) { + merger := NewArrayMerger() + w := &bytes.Buffer{} + + r1 := bytes.NewBufferString(`["a","b"]`) + p1 := merger.NewPage(r1, false) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(8), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, `["a","b"`, w.String()) + + r2 := bytes.NewBufferString(`["c","d"]`) + p2 := merger.NewPage(r2, true) + n, err = io.Copy(w, p2) + require.NoError(t, err) + assert.Equal(t, int64(9), n) + assert.NoError(t, p2.Close()) + assert.Equal(t, `["a","b","c","d"]`, w.String()) + + require.NoError(t, merger.Close()) +} + +func TestArrayMerger_emptyObject(t *testing.T) { + merger := NewArrayMerger() + w := &bytes.Buffer{} + + 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()) +} diff --git a/internal/jsonmerge/merge.go b/internal/jsonmerge/merge.go new file mode 100644 index 00000000000..a40e4ee085c --- /dev/null +++ b/internal/jsonmerge/merge.go @@ -0,0 +1,12 @@ +// jsonmerge implements readers to merge JSON arrays or objects. +package jsonmerge + +import ( + "io" +) + +// Merger is implemented to merge JSON arrays or objects. +type Merger interface { + NewPage(r io.Reader, isLastPage bool) io.ReadCloser + Close() error +} diff --git a/internal/jsonmerge/object.go b/internal/jsonmerge/object.go new file mode 100644 index 00000000000..3e634fa17bd --- /dev/null +++ b/internal/jsonmerge/object.go @@ -0,0 +1,66 @@ +package jsonmerge + +import ( + "bytes" + "encoding/json" + "io" + + "dario.cat/mergo" +) + +type objectMerger struct { + io.Writer + dst map[string]interface{} +} + +// NewObjectMerger creates a Merger for JSON objects. +func NewObjectMerger(w io.Writer) Merger { + return &objectMerger{ + Writer: w, + dst: make(map[string]interface{}), + } +} + +func (merger *objectMerger) NewPage(r io.Reader, isLastPage bool) io.ReadCloser { + return &objectMergerPage{ + merger: merger, + Reader: r, + } +} + +func (merger *objectMerger) Close() error { + // Marshal to JSON and write to output. + buf, err := json.Marshal(merger.dst) + if err != nil { + return err + } + + _, err = merger.Writer.Write(buf) + return err +} + +type objectMergerPage struct { + merger *objectMerger + + io.Reader + buffer bytes.Buffer +} + +// Read caches the data in an internal buffer to be merged in Close. +// No data is copied into p so it's not written to stdout. +func (page *objectMergerPage) Read(p []byte) (int, error) { + _, err := io.CopyN(&page.buffer, page.Reader, int64(len(p))) + return 0, err +} + +// Close converts the internal buffer to a JSON object and merges it with the final JSON object. +func (page *objectMergerPage) Close() error { + var src map[string]interface{} + + err := json.Unmarshal(page.buffer.Bytes(), &src) + if err != nil { + return err + } + + return mergo.Merge(&page.merger.dst, src, mergo.WithAppendSlice, mergo.WithOverride) +} diff --git a/internal/jsonmerge/object_test.go b/internal/jsonmerge/object_test.go new file mode 100644 index 00000000000..b3a76df23b1 --- /dev/null +++ b/internal/jsonmerge/object_test.go @@ -0,0 +1,98 @@ +package jsonmerge + +import ( + "bytes" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestObjectMerger_singleEmptyObject(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + r1 := bytes.NewBufferString(`{}`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, ``, w.String()) + + require.NoError(t, merger.Close()) + assert.JSONEq(t, `{}`, w.String()) +} + +func TestObjectMerger_finalEmptyObject(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + 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(0), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, ``, w.String()) + + r2 := bytes.NewBufferString(`{}`) + p2 := merger.NewPage(r2, true) + n, err = io.Copy(w, p2) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.NoError(t, p2.Close()) + assert.Equal(t, ``, w.String()) + + require.NoError(t, merger.Close()) + assert.JSONEq(t, `{"a":1,"b":2}`, w.String()) +} + +func TestObjectMerger_multiplePages(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + r1 := bytes.NewBufferString(`{"a":1,"b":2,"arr":["a","b"]}`) + p1 := merger.NewPage(r1, false) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.NoError(t, p1.Close()) + assert.Equal(t, ``, w.String()) + + r2 := bytes.NewBufferString(`{"b":3,"c":{"d":4},"arr":["c","d"]}`) + p2 := merger.NewPage(r2, true) + n, err = io.Copy(w, p2) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.NoError(t, p2.Close()) + assert.Equal(t, ``, w.String()) + + require.NoError(t, merger.Close()) + assert.JSONEq(t, `{"a":1,"b":3,"c":{"d":4},"arr":["a","b","c","d"]}`, w.String()) +} + +func TestObjectMerger_invalidJSON(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + r1 := bytes.NewBufferString(`invalid`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.Error(t, p1.Close()) +} + +func TestObjectMerger_array(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + r1 := bytes.NewBufferString(`[]`) + p1 := merger.NewPage(r1, true) + n, err := io.Copy(w, p1) + require.NoError(t, err) + assert.Equal(t, int64(0), n) + assert.Error(t, p1.Close()) +} diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 8b42c040e21..004388872c9 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -20,12 +20,12 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/jsonmerge" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/jsoncolor" "github.com/cli/go-gh/v2/pkg/jq" - "github.com/cli/go-gh/v2/pkg/jsonmerge" "github.com/cli/go-gh/v2/pkg/template" "github.com/spf13/cobra" ) From e7f5dded06ae60d464c11de543fe54fddc51a8f9 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Tue, 30 Jan 2024 23:42:04 -0800 Subject: [PATCH 03/11] Add more JSON merge tests --- pkg/cmd/api/api_test.go | 135 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 126 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 778cbd3e191..1085a6b685e 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -551,6 +551,34 @@ func Test_apiRun(t *testing.T) { stderr: ``, isatty: false, }, + { + name: "output template with range", + options: ApiOptions{ + Template: `{{range .}}{{.title}} ({{.labels | pluck "name" | join ", " }}){{"\n"}}{{end}}`, + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`[ + { + "title": "First title", + "labels": [{"name":"bug"}, {"name":"help wanted"}] + }, + { + "title": "Second but not last" + }, + { + "title": "Alas, tis' the end", + "labels": [{}, {"name":"feature"}] + } + ]`)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, + stdout: heredoc.Doc(` + First title (bug, help wanted) + Second but not last () + Alas, tis' the end (, feature) + `), + }, { name: "output template when REST error", options: ApiOptions{ @@ -650,23 +678,108 @@ func Test_apiRun_paginationREST(t *testing.T) { 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"}, + } + + 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}`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"page":1}]`)), Header: http.Header{ - "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(`{"page":2}`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), Header: http.Header{ - "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(`{"page":3}`)), - Header: http.Header{}, + Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "X-Github-Request-Id": []string{"3"}, + }, }, } @@ -690,12 +803,13 @@ func Test_apiRun_paginationREST(t *testing.T) { RequestPath: "issues", Paginate: true, RawFields: []string{"per_page=50", "page=1"}, + ShowResponseHeaders: true, } err := apiRun(&options) assert.NoError(t, err) - assert.Equal(t, `{"page":1}{"page":2}{"page":3}`, 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()) @@ -867,6 +981,9 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { } func Test_apiRun_paginated_template(t *testing.T) { + // TODO: Refactor to execute template on the merged JSON. + t.Skip("Must execute template on merged JSON. See https://github.com/cli/cli/pull/8620.") + ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(true) @@ -929,8 +1046,8 @@ func Test_apiRun_paginated_template(t *testing.T) { RequestPath: "graphql", RawFields: []string{"foo=bar"}, Paginate: true, - // test that templates executed per page properly render a table. - Template: `{{range .data.nodes}}{{tablerow .page .caption}}{{end}}`, + // use explicit {{tablerender}} to assert all pages are rendered together when paginating. + Template: `{{range .data.nodes}}{{tablerow .page .caption}}{{end}}{{tablerender}}`, } err := apiRun(&options) From 310e5524bac477e7a788e8835ae097647789e18c Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Wed, 31 Jan 2024 01:28:55 -0800 Subject: [PATCH 04/11] Resolve test issues --- pkg/cmd/api/api.go | 34 ++++-- pkg/cmd/api/api_test.go | 260 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 276 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 004388872c9..71f91a91da6 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -50,6 +50,7 @@ type ApiOptions struct { Previews []string ShowResponseHeaders bool Paginate bool + PaginateAll bool Silent bool Template string CacheTTL time.Duration @@ -117,6 +118,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command there are no more pages of results. For GraphQL requests, this requires that the original query accepts an %[1]s$endCursor: String%[1]s variable and that it fetches the %[1]spageInfo{ hasNextPage, endCursor }%[1]s set of fields from a collection. + Pass %[1]s--paginate-all%[1]s (experimental) instead to merge REST arrays and GraphQL objects + into valid JSON output. `, "`"), Example: heredoc.Doc(` # list releases in the current repository @@ -199,18 +202,26 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command } } - if opts.Paginate && !strings.EqualFold(opts.RequestMethod, "GET") && opts.RequestPath != "graphql" { - return cmdutil.FlagErrorf("the `--paginate` option is not supported for non-GET requests") + if opts.shouldPaginate() && !strings.EqualFold(opts.RequestMethod, "GET") && opts.RequestPath != "graphql" { + return cmdutil.FlagErrorf("the `--paginate` and `--paginate-all` options are not supported for non-GET requests") } if err := cmdutil.MutuallyExclusive( - "the `--paginate` option is not supported with `--input`", - opts.Paginate, + "the `--paginate` and `--paginate-all` options are not supported with `--input`", + opts.shouldPaginate(), opts.RequestInputFile != "", ); err != nil { return err } + if err := cmdutil.MutuallyExclusive( + "specify only one of `--paginate` or `--paginate-all`", + opts.Paginate, + opts.PaginateAll, + ); err != nil { + return err + } + if err := cmdutil.MutuallyExclusive( "only one of `--template`, `--jq`, `--silent`, or `--verbose` may be used", opts.Verbose, @@ -236,6 +247,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command cmd.Flags().StringSliceVarP(&opts.Previews, "preview", "p", nil, "GitHub API preview `names` to request (without the \"-preview\" suffix)") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response status line and headers in the output") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") + cmd.Flags().BoolVar(&opts.PaginateAll, "paginate-all", false, "[Experimental] Make additional HTTP requests to fetch all pages and merge results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The `file` to use as body for the HTTP request (use \"-\" to read from standard input)") cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Do not print the response body") cmd.Flags().StringVarP(&opts.Template, "template", "t", "", "Format JSON output using a Go template; see \"gh help formatting\"") @@ -245,6 +257,10 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command return cmd } +func (opts *ApiOptions) shouldPaginate() bool { + return opts.Paginate || opts.PaginateAll +} + func apiRun(opts *ApiOptions) error { params, err := parseFields(opts) if err != nil { @@ -278,12 +294,12 @@ func apiRun(opts *ApiOptions) error { headersWriter = io.Discard } - if opts.Paginate && !isGraphQL { + if opts.shouldPaginate() && !isGraphQL { requestPath = addPerPage(requestPath, 100, params) } - // Merge JSON arrays and object if paginating without a filter or template. - if opts.Paginate && opts.FilterOutput == "" && opts.Template == "" { + // Merge JSON arrays and objects if paginating without filtering or templating (experimental). + if opts.PaginateAll && opts.FilterOutput == "" && opts.Template == "" { if isGraphQL { opts.Merger = jsonmerge.NewObjectMerger(bodyWriter) } else { @@ -375,7 +391,7 @@ func apiRun(opts *ApiOptions) error { } isFirstPage = false - if !opts.Paginate { + if !opts.shouldPaginate() { break } @@ -422,7 +438,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW } var bodyCopy *bytes.Buffer - isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.Paginate && opts.RequestPath == "graphql" + isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.shouldPaginate() && opts.RequestPath == "graphql" if isGraphQLPaginate { bodyCopy = &bytes.Buffer{} responseBody = io.TeeReader(responseBody, bodyCopy) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 1085a6b685e..def0c46f4fd 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -736,6 +736,78 @@ 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", + PaginateAll: 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") @@ -881,6 +953,78 @@ 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, "", 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_arrayPaginationREST_merge(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(false) + + requestCount := 0 + responses := []*http.Response{ + { + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`[{"item":1},{"item":2}]`)), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, + }, + }, + { + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`[{"item":3},{"item":4}]`)), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, + }, + }, + { + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`[{"item":5}]`)), + 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"}, + }, + }, + } + + 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", + PaginateAll: true, + RawFields: []string{"per_page=50", "page=1"}, + } + + 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, "", stderr.String(), "stderr") @@ -897,7 +1041,8 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { { StatusCode: 200, Header: http.Header{"Content-Type": []string{`application/json`}}, - Body: io.NopCloser(bytes.NewBufferString(`{ + Body: io.NopCloser(bytes.NewBufferString(heredoc.Doc(` + { "data": { "nodes": ["page one"], "pageInfo": { @@ -905,12 +1050,13 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { "hasNextPage": true } } - }`)), + }`))), }, { StatusCode: 200, Header: http.Header{"Content-Type": []string{`application/json`}}, - Body: io.NopCloser(bytes.NewBufferString(`{ + Body: io.NopCloser(bytes.NewBufferString(heredoc.Doc(` + { "data": { "nodes": ["page two"], "pageInfo": { @@ -918,7 +1064,7 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { "hasNextPage": false } } - }`)), + }`))), }, } @@ -946,6 +1092,105 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { err := apiRun(&options) require.NoError(t, err) + assert.Equal(t, heredoc.Doc(` + { + "data": { + "nodes": ["page one"], + "pageInfo": { + "endCursor": "PAGE1_END", + "hasNextPage": true + } + } + }{ + "data": { + "nodes": ["page two"], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } + } + }`), stdout.String()) + 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_paginationGraphQL_merge(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + + 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", + PaginateAll: true, + } + + err := apiRun(&options) + require.NoError(t, err) + assert.JSONEq(t, stdout.String(), `{ "data": { "nodes": [ @@ -981,9 +1226,6 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { } func Test_apiRun_paginated_template(t *testing.T) { - // TODO: Refactor to execute template on the merged JSON. - t.Skip("Must execute template on merged JSON. See https://github.com/cli/cli/pull/8620.") - ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(true) @@ -1046,8 +1288,8 @@ func Test_apiRun_paginated_template(t *testing.T) { RequestPath: "graphql", RawFields: []string{"foo=bar"}, Paginate: true, - // use explicit {{tablerender}} to assert all pages are rendered together when paginating. - Template: `{{range .data.nodes}}{{tablerow .page .caption}}{{end}}{{tablerender}}`, + // test that templates executed per page properly render a table. + Template: `{{range .data.nodes}}{{tablerow .page .caption}}{{end}}`, } err := apiRun(&options) From 48f0cd6f11cf8581e3720c74554409489472ac37 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 1 Feb 2024 21:00:21 -0800 Subject: [PATCH 05/11] Replace --paginate-all with --merge-pages --- pkg/cmd/api/api.go | 56 ++++++++++++++++++----------------------- pkg/cmd/api/api_test.go | 14 ++++++++--- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 71f91a91da6..9bae963188a 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -37,7 +37,7 @@ type ApiOptions struct { Config func() (config.Config, error) HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - Merger jsonmerge.Merger + merger jsonmerge.Merger Hostname string RequestMethod string @@ -50,7 +50,7 @@ type ApiOptions struct { Previews []string ShowResponseHeaders bool Paginate bool - PaginateAll bool + MergePages bool Silent bool Template string CacheTTL time.Duration @@ -117,9 +117,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command In %[1]s--paginate%[1]s mode, all pages of results will sequentially be requested until there are no more pages of results. For GraphQL requests, this requires that the original query accepts an %[1]s$endCursor: String%[1]s variable and that it fetches the - %[1]spageInfo{ hasNextPage, endCursor }%[1]s set of fields from a collection. - Pass %[1]s--paginate-all%[1]s (experimental) instead to merge REST arrays and GraphQL objects - into valid JSON output. + %[1]spageInfo{ hasNextPage, endCursor }%[1]s set of fields from a collection. Each page is a separate + JSON array or object. Pass %[1]s--merge-pages%[1]s to merge all pages into a single JSON array or object. `, "`"), Example: heredoc.Doc(` # list releases in the current repository @@ -159,7 +158,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command ' # list all repositories for a user - $ gh api graphql --paginate -f query=' + $ gh api graphql --paginate --merge-pages -f query=' query($endCursor: String) { viewer { repositories(first: 100, after: $endCursor) { @@ -202,24 +201,20 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command } } - if opts.shouldPaginate() && !strings.EqualFold(opts.RequestMethod, "GET") && opts.RequestPath != "graphql" { - return cmdutil.FlagErrorf("the `--paginate` and `--paginate-all` options are not supported for non-GET requests") + if opts.Paginate && !strings.EqualFold(opts.RequestMethod, "GET") && opts.RequestPath != "graphql" { + return cmdutil.FlagErrorf("the `--paginate` option is not supported for non-GET requests") } if err := cmdutil.MutuallyExclusive( - "the `--paginate` and `--paginate-all` options are not supported with `--input`", - opts.shouldPaginate(), + "the `--paginate` option is not supported with `--input`", + opts.Paginate, opts.RequestInputFile != "", ); err != nil { return err } - if err := cmdutil.MutuallyExclusive( - "specify only one of `--paginate` or `--paginate-all`", - opts.Paginate, - opts.PaginateAll, - ); err != nil { - return err + if opts.MergePages && !opts.Paginate { + return cmdutil.FlagErrorf("`--paginate` required when passing `--merge-pages`") } if err := cmdutil.MutuallyExclusive( @@ -246,8 +241,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add a HTTP request header in `key:value` format") cmd.Flags().StringSliceVarP(&opts.Previews, "preview", "p", nil, "GitHub API preview `names` to request (without the \"-preview\" suffix)") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response status line and headers in the output") + cmd.Flags().BoolVar(&opts.MergePages, "merge-pages", false, "Use with \"--paginate\" to merge all pages of JSON arrays or objects") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") - cmd.Flags().BoolVar(&opts.PaginateAll, "paginate-all", false, "[Experimental] Make additional HTTP requests to fetch all pages and merge results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The `file` to use as body for the HTTP request (use \"-\" to read from standard input)") cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Do not print the response body") cmd.Flags().StringVarP(&opts.Template, "template", "t", "", "Format JSON output using a Go template; see \"gh help formatting\"") @@ -257,10 +252,6 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command return cmd } -func (opts *ApiOptions) shouldPaginate() bool { - return opts.Paginate || opts.PaginateAll -} - func apiRun(opts *ApiOptions) error { params, err := parseFields(opts) if err != nil { @@ -294,16 +285,17 @@ func apiRun(opts *ApiOptions) error { headersWriter = io.Discard } - if opts.shouldPaginate() && !isGraphQL { + if opts.Paginate && !isGraphQL { requestPath = addPerPage(requestPath, 100, params) } - // Merge JSON arrays and objects if paginating without filtering or templating (experimental). - if opts.PaginateAll && opts.FilterOutput == "" && opts.Template == "" { + // 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) + opts.merger = jsonmerge.NewObjectMerger(bodyWriter) } else { - opts.Merger = jsonmerge.NewArrayMerger() + opts.merger = jsonmerge.NewArrayMerger() } } @@ -391,7 +383,7 @@ func apiRun(opts *ApiOptions) error { } isFirstPage = false - if !opts.shouldPaginate() { + if !opts.Paginate { break } @@ -407,8 +399,8 @@ func apiRun(opts *ApiOptions) error { } } - if opts.Merger != nil { - return opts.Merger.Close() + if opts.merger != nil { + return opts.merger.Close() } return tmpl.Flush() @@ -438,7 +430,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW } var bodyCopy *bytes.Buffer - isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.shouldPaginate() && opts.RequestPath == "graphql" + isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.Paginate && opts.RequestPath == "graphql" if isGraphQLPaginate { bodyCopy = &bytes.Buffer{} responseBody = io.TeeReader(responseBody, bodyCopy) @@ -462,8 +454,8 @@ 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 { - responseBody = opts.Merger.NewPage(responseBody, isLastPage) + if isJSON && opts.merger != nil && !opts.ShowResponseHeaders { + responseBody = opts.merger.NewPage(responseBody, isLastPage) } _, err = io.Copy(bodyWriter, responseBody) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index def0c46f4fd..f5145b3c394 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -328,6 +328,11 @@ func Test_NewCmdApi(t *testing.T) { cli: "user --jq .foo -t '{{.foo}}'", wantsErr: true, }, + { + name: "--merge-pages without --paginate", + cli: "user --merge-pages", + wantsErr: true, + }, { name: "with verbose", cli: "user --verbose", @@ -801,7 +806,8 @@ func Test_apiRun_paginationREST_merge(t *testing.T) { RequestMethod: "GET", RequestMethodPassed: true, RequestPath: "issues", - PaginateAll: true, + Paginate: true, + MergePages: true, RawFields: []string{"per_page=50", "page=1"}, } @@ -1018,7 +1024,8 @@ func Test_apiRun_arrayPaginationREST_merge(t *testing.T) { RequestMethod: "GET", RequestMethodPassed: true, RequestPath: "issues", - PaginateAll: true, + Paginate: true, + MergePages: true, RawFields: []string{"per_page=50", "page=1"}, } @@ -1185,7 +1192,8 @@ func Test_apiRun_paginationGraphQL_merge(t *testing.T) { RawFields: []string{"foo=bar"}, RequestMethod: "POST", RequestPath: "graphql", - PaginateAll: true, + Paginate: true, + MergePages: true, } err := apiRun(&options) From 0dfe6ec4b48f153f12ba1f30ebdbf1e36b319efc Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 1 Feb 2024 21:35:10 -0800 Subject: [PATCH 06/11] Clarify --merge-pages docs Only works when piping or redirecting stdout. --- internal/jsonmerge/object.go | 9 ++++++++- internal/jsonmerge/object_test.go | 8 ++++++++ pkg/cmd/api/api.go | 5 +++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/jsonmerge/object.go b/internal/jsonmerge/object.go index 3e634fa17bd..b8f077bbfdd 100644 --- a/internal/jsonmerge/object.go +++ b/internal/jsonmerge/object.go @@ -17,7 +17,6 @@ type objectMerger struct { func NewObjectMerger(w io.Writer) Merger { return &objectMerger{ Writer: w, - dst: make(map[string]interface{}), } } @@ -29,6 +28,10 @@ func (merger *objectMerger) NewPage(r io.Reader, isLastPage bool) io.ReadCloser } func (merger *objectMerger) Close() error { + if merger.dst == nil { + return nil + } + // Marshal to JSON and write to output. buf, err := json.Marshal(merger.dst) if err != nil { @@ -62,5 +65,9 @@ func (page *objectMergerPage) Close() error { return err } + if page.merger.dst == nil { + page.merger.dst = make(map[string]interface{}) + } + return mergo.Merge(&page.merger.dst, src, mergo.WithAppendSlice, mergo.WithOverride) } diff --git a/internal/jsonmerge/object_test.go b/internal/jsonmerge/object_test.go index b3a76df23b1..48074e33a0d 100644 --- a/internal/jsonmerge/object_test.go +++ b/internal/jsonmerge/object_test.go @@ -9,6 +9,14 @@ import ( "github.com/stretchr/testify/require" ) +func TestObjectMerger_nothingWritten(t *testing.T) { + w := &bytes.Buffer{} + merger := NewObjectMerger(w) + + require.NoError(t, merger.Close()) + assert.Equal(t, ``, w.String()) +} + func TestObjectMerger_singleEmptyObject(t *testing.T) { w := &bytes.Buffer{} merger := NewObjectMerger(w) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 9bae963188a..3a2ccd4a110 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -118,7 +118,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command there are no more pages of results. For GraphQL requests, this requires that the original query accepts an %[1]s$endCursor: String%[1]s variable and that it fetches the %[1]spageInfo{ hasNextPage, endCursor }%[1]s set of fields from a collection. Each page is a separate - JSON array or object. Pass %[1]s--merge-pages%[1]s to merge all pages into a single JSON array or object. + JSON array or object. Pass %[1]s--merge-pages%[1]s to merge all pages into a single JSON array or object + when piping or redirecting standard output. `, "`"), Example: heredoc.Doc(` # list releases in the current repository @@ -241,7 +242,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add a HTTP request header in `key:value` format") cmd.Flags().StringSliceVarP(&opts.Previews, "preview", "p", nil, "GitHub API preview `names` to request (without the \"-preview\" suffix)") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response status line and headers in the output") - cmd.Flags().BoolVar(&opts.MergePages, "merge-pages", false, "Use with \"--paginate\" to merge all pages of JSON arrays or objects") + cmd.Flags().BoolVar(&opts.MergePages, "merge-pages", false, "Use with \"--paginate\" to merge all pages of JSON arrays or objects when piping or redirecting standard output") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The `file` to use as body for the HTTP request (use \"-\" to read from standard input)") cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Do not print the response body") From f41876d64cc64c92479073fb1afda427416835e8 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 15 Feb 2024 00:59:12 -0800 Subject: [PATCH 07/11] 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 | 243 +++++++++++++-------------------- 4 files changed, 258 insertions(+), 156 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 00000000000..d11a545b9a3 --- /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 00000000000..713ac5501dd --- /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 3a2ccd4a110..35b99eef6ae 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 f5145b3c394..0d02eff6146 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,7 +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, `{"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()) @@ -749,41 +749,41 @@ func Test_apiRun_paginationREST(t *testing.T) { 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) { +func Test_apiRun_arrayPaginationREST(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(`[{"page":1}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"item":1},{"item":2}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"1"}, + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, }, }, { - Proto: "HTTP/1.1", - Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":2}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"item":3},{"item":4}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - "X-Github-Request-Id": []string{"2"}, + "Content-Type": []string{"application/json"}, + "Link": []string{`; rel="next", ; rel="last"`}, }, }, { - Proto: "HTTP/1.1", - Status: "200 OK", StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"page":3}]`)), + Body: io.NopCloser(bytes.NewBufferString(`[{"item":5}]`)), Header: http.Header{ - "Content-Type": []string{"application/json"}, - "X-Github-Request-Id": []string{"3"}, + "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"}, }, }, } @@ -807,14 +807,13 @@ func Test_apiRun_paginationREST_merge(t *testing.T) { 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, `[{"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()) @@ -822,7 +821,7 @@ func Test_apiRun_paginationREST_merge(t *testing.T) { 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) { +func Test_apiRun_arrayPaginationREST_with_headers(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() requestCount := 0 @@ -895,42 +894,38 @@ func Test_apiRun_paginationREST_with_headers(t *testing.T) { assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) } -func Test_apiRun_arrayPaginationREST(t *testing.T) { +func Test_apiRun_paginationGraphQL(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() - ios.SetStdoutTTY(false) requestCount := 0 responses := []*http.Response{ { StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":1},{"item":2}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, - }, - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":3},{"item":4}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, - }, - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":5}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, + 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, - Body: io.NopCloser(bytes.NewBufferString(`[]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - }, + 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 + } + } + }`))), }, } @@ -949,98 +944,56 @@ func Test_apiRun_arrayPaginationREST(t *testing.T) { return config.NewBlankConfig(), nil }, - RequestMethod: "GET", - RequestMethodPassed: true, - RequestPath: "issues", - Paginate: true, - RawFields: []string{"per_page=50", "page=1"}, + RawFields: []string{"foo=bar"}, + RequestMethod: "POST", + RequestPath: "graphql", + Paginate: 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, "", 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_arrayPaginationREST_merge(t *testing.T) { - ios, _, stdout, stderr := iostreams.Test() - ios.SetStdoutTTY(false) - - requestCount := 0 - responses := []*http.Response{ - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":1},{"item":2}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, - }, - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":3},{"item":4}]`)), - Header: http.Header{ - "Content-Type": []string{"application/json"}, - "Link": []string{`; rel="next", ; rel="last"`}, - }, - }, - { - StatusCode: 200, - Body: io.NopCloser(bytes.NewBufferString(`[{"item":5}]`)), - 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"}, - }, - }, - } + require.NoError(t, err) - 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 + assert.Equal(t, heredoc.Doc(` + { + "data": { + "nodes": ["page one"], + "pageInfo": { + "endCursor": "PAGE1_END", + "hasNextPage": true } - return &http.Client{Transport: tr}, nil - }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, + } + }{ + "data": { + "nodes": ["page two"], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } + } + }`), stdout.String()) + assert.Equal(t, "", stderr.String(), "stderr") - RequestMethod: "GET", - RequestMethodPassed: true, - RequestPath: "issues", - Paginate: true, - MergePages: true, - RawFields: []string{"per_page=50", "page=1"}, + var requestData struct { + Variables map[string]interface{} } - 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, "", stderr.String(), "stderr") + 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) - 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()) + 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_paginationGraphQL(t *testing.T) { +func Test_apiRun_paginationGraphQL_merge(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() requestCount := 0 @@ -1094,29 +1047,24 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { RequestMethod: "POST", RequestPath: "graphql", Paginate: true, + MergePages: true, } err := apiRun(&options) require.NoError(t, err) - assert.Equal(t, heredoc.Doc(` - { - "data": { - "nodes": ["page one"], - "pageInfo": { - "endCursor": "PAGE1_END", - "hasNextPage": true - } - } - }{ + assert.JSONEq(t, stdout.String(), `{ "data": { - "nodes": ["page two"], + "nodes": [ + "page one", + "page two" + ], "pageInfo": { "endCursor": "PAGE2_END", "hasNextPage": false } } - }`), stdout.String()) + }`) assert.Equal(t, "", stderr.String(), "stderr") var requestData struct { @@ -1139,9 +1087,12 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { assert.Equal(t, "PAGE1_END", endCursor) } -func Test_apiRun_paginationGraphQL_merge(t *testing.T) { +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{ { From e83e04930641e062249dc4aba2cba5b2d35d1278 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Tue, 12 Mar 2024 02:39:44 -0700 Subject: [PATCH 08/11] Wrap JSON arrays, objects in array with --slurp Effectively copies `jq --slurp` since `--jq` already uses the same grammar. --- go.mod | 1 - go.sum | 2 - internal/jsonmerge/array.go | 76 ----------------- internal/jsonmerge/array_test.go | 86 ------------------- internal/jsonmerge/merge.go | 12 --- internal/jsonmerge/nop.go | 19 ----- internal/jsonmerge/nop_test.go | 114 ------------------------- internal/jsonmerge/object.go | 73 ---------------- internal/jsonmerge/object_test.go | 106 ----------------------- pkg/cmd/api/api.go | 75 +++++++++-------- pkg/cmd/api/api_test.go | 134 +++++++----------------------- pkg/cmd/api/pagination.go | 133 +++++++++++++++++++++++++++++ pkg/cmd/api/pagination_test.go | 107 ++++++++++++++++++++++++ pkg/jsoncolor/jsoncolor.go | 22 +++++ 14 files changed, 332 insertions(+), 628 deletions(-) delete mode 100644 internal/jsonmerge/array.go delete mode 100644 internal/jsonmerge/array_test.go delete mode 100644 internal/jsonmerge/merge.go delete mode 100644 internal/jsonmerge/nop.go delete mode 100644 internal/jsonmerge/nop_test.go delete mode 100644 internal/jsonmerge/object.go delete mode 100644 internal/jsonmerge/object_test.go diff --git a/go.mod b/go.mod index 9381694c1a3..ba96c7cb01f 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,6 @@ module github.com/cli/cli/v2 go 1.22 require ( - dario.cat/mergo v1.0.0 github.com/AlecAivazis/survey/v2 v2.3.7 github.com/MakeNowJust/heredoc v1.0.0 github.com/briandowns/spinner v1.18.1 diff --git a/go.sum b/go.sum index bfd00f3a81e..1c759cbebf1 100644 --- a/go.sum +++ b/go.sum @@ -7,8 +7,6 @@ cloud.google.com/go/iam v1.1.5 h1:1jTsCu4bcsNsE4iiqNT5SHwrDRCfRmIaaaVFhRveTJI= cloud.google.com/go/iam v1.1.5/go.mod h1:rB6P/Ic3mykPbFio+vo7403drjlgvoWfYpJhMXEbzv8= cloud.google.com/go/kms v1.15.5 h1:pj1sRfut2eRbD9pFRjNnPNg/CzJPuQAzUujMIM1vVeM= cloud.google.com/go/kms v1.15.5/go.mod h1:cU2H5jnp6G2TDpUGZyqTCoy1n16fbubHZjmVXSMtwDI= -dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= -dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA= filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4= github.com/AdamKorcz/go-fuzz-headers-1 v0.0.0-20230919221257-8b5d3ce2d11d h1:zjqpY4C7H15HjRPEenkS4SAn3Jy2eRRjkjZbGR30TOg= diff --git a/internal/jsonmerge/array.go b/internal/jsonmerge/array.go deleted file mode 100644 index 3a8c2c744e7..00000000000 --- a/internal/jsonmerge/array.go +++ /dev/null @@ -1,76 +0,0 @@ -package jsonmerge - -import "io" - -type arrayMerger struct { - isFirstPage bool -} - -// NewArrayMerger creates a Merger for JSON arrays. -func NewArrayMerger() Merger { - return &arrayMerger{ - isFirstPage: true, - } -} - -func (merger *arrayMerger) NewPage(r io.Reader, isLastPage bool) io.ReadCloser { - return &arrayMergerPage{ - merger: merger, - Reader: r, - isLastPage: isLastPage, - } -} - -func (m *arrayMerger) Close() error { - // arrayMerger merges when reading, so any output was already written - // and there's nothing to do on Close. - return nil -} - -type arrayMergerPage struct { - merger *arrayMerger - - io.Reader - isLastPage bool - - isSubsequentRead bool - cachedByte byte -} - -func (page *arrayMergerPage) Read(p []byte) (int, error) { - var n int - var err error - - if page.cachedByte != 0 && len(p) > 0 { - p[0] = page.cachedByte - n, err = page.Reader.Read(p[1:]) - n += 1 - page.cachedByte = 0 - } else { - n, err = page.Reader.Read(p) - } - - if !page.isSubsequentRead && !page.merger.isFirstPage && n > 0 && p[0] == '[' { - if n > 1 && p[1] == ']' { - // Empty array case. - p[0] = ' ' - } else { - // Avoid starting a new array and continue with a comma instead. - p[0] = ',' - } - } - - if !page.isLastPage && n > 0 && p[n-1] == ']' { - // Avoid closing off an array in case we determine we are at EOF. - page.cachedByte = p[n-1] - n -= 1 - } - - page.isSubsequentRead = true - return n, err -} - -func (page *arrayMergerPage) Close() error { - page.merger.isFirstPage = false - return nil -} diff --git a/internal/jsonmerge/array_test.go b/internal/jsonmerge/array_test.go deleted file mode 100644 index 8106ded30d8..00000000000 --- a/internal/jsonmerge/array_test.go +++ /dev/null @@ -1,86 +0,0 @@ -package jsonmerge - -import ( - "bytes" - "io" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestArrayMerger_singleEmptyArray(t *testing.T) { - merger := NewArrayMerger() - w := &bytes.Buffer{} - - 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()) -} - -func TestArrayMerger_finalEmptyArray(t *testing.T) { - merger := NewArrayMerger() - w := &bytes.Buffer{} - - r1 := bytes.NewBufferString(`["a","b"]`) - p1 := merger.NewPage(r1, false) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(8), 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()) -} - -func TestArrayMerger_multiplePages(t *testing.T) { - merger := NewArrayMerger() - w := &bytes.Buffer{} - - r1 := bytes.NewBufferString(`["a","b"]`) - p1 := merger.NewPage(r1, false) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(8), n) - assert.NoError(t, p1.Close()) - assert.Equal(t, `["a","b"`, w.String()) - - r2 := bytes.NewBufferString(`["c","d"]`) - p2 := merger.NewPage(r2, true) - n, err = io.Copy(w, p2) - require.NoError(t, err) - assert.Equal(t, int64(9), n) - assert.NoError(t, p2.Close()) - assert.Equal(t, `["a","b","c","d"]`, w.String()) - - require.NoError(t, merger.Close()) -} - -func TestArrayMerger_emptyObject(t *testing.T) { - merger := NewArrayMerger() - w := &bytes.Buffer{} - - 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()) -} diff --git a/internal/jsonmerge/merge.go b/internal/jsonmerge/merge.go deleted file mode 100644 index a40e4ee085c..00000000000 --- a/internal/jsonmerge/merge.go +++ /dev/null @@ -1,12 +0,0 @@ -// jsonmerge implements readers to merge JSON arrays or objects. -package jsonmerge - -import ( - "io" -) - -// Merger is implemented to merge JSON arrays or objects. -type Merger interface { - NewPage(r io.Reader, isLastPage bool) io.ReadCloser - Close() error -} diff --git a/internal/jsonmerge/nop.go b/internal/jsonmerge/nop.go deleted file mode 100644 index d11a545b9a3..00000000000 --- a/internal/jsonmerge/nop.go +++ /dev/null @@ -1,19 +0,0 @@ -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 deleted file mode 100644 index 713ac5501dd..00000000000 --- a/internal/jsonmerge/nop_test.go +++ /dev/null @@ -1,114 +0,0 @@ -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/internal/jsonmerge/object.go b/internal/jsonmerge/object.go deleted file mode 100644 index b8f077bbfdd..00000000000 --- a/internal/jsonmerge/object.go +++ /dev/null @@ -1,73 +0,0 @@ -package jsonmerge - -import ( - "bytes" - "encoding/json" - "io" - - "dario.cat/mergo" -) - -type objectMerger struct { - io.Writer - dst map[string]interface{} -} - -// NewObjectMerger creates a Merger for JSON objects. -func NewObjectMerger(w io.Writer) Merger { - return &objectMerger{ - Writer: w, - } -} - -func (merger *objectMerger) NewPage(r io.Reader, isLastPage bool) io.ReadCloser { - return &objectMergerPage{ - merger: merger, - Reader: r, - } -} - -func (merger *objectMerger) Close() error { - if merger.dst == nil { - return nil - } - - // Marshal to JSON and write to output. - buf, err := json.Marshal(merger.dst) - if err != nil { - return err - } - - _, err = merger.Writer.Write(buf) - return err -} - -type objectMergerPage struct { - merger *objectMerger - - io.Reader - buffer bytes.Buffer -} - -// Read caches the data in an internal buffer to be merged in Close. -// No data is copied into p so it's not written to stdout. -func (page *objectMergerPage) Read(p []byte) (int, error) { - _, err := io.CopyN(&page.buffer, page.Reader, int64(len(p))) - return 0, err -} - -// Close converts the internal buffer to a JSON object and merges it with the final JSON object. -func (page *objectMergerPage) Close() error { - var src map[string]interface{} - - err := json.Unmarshal(page.buffer.Bytes(), &src) - if err != nil { - return err - } - - if page.merger.dst == nil { - page.merger.dst = make(map[string]interface{}) - } - - return mergo.Merge(&page.merger.dst, src, mergo.WithAppendSlice, mergo.WithOverride) -} diff --git a/internal/jsonmerge/object_test.go b/internal/jsonmerge/object_test.go deleted file mode 100644 index 48074e33a0d..00000000000 --- a/internal/jsonmerge/object_test.go +++ /dev/null @@ -1,106 +0,0 @@ -package jsonmerge - -import ( - "bytes" - "io" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestObjectMerger_nothingWritten(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - require.NoError(t, merger.Close()) - assert.Equal(t, ``, w.String()) -} - -func TestObjectMerger_singleEmptyObject(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - r1 := bytes.NewBufferString(`{}`) - p1 := merger.NewPage(r1, true) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.NoError(t, p1.Close()) - assert.Equal(t, ``, w.String()) - - require.NoError(t, merger.Close()) - assert.JSONEq(t, `{}`, w.String()) -} - -func TestObjectMerger_finalEmptyObject(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - 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(0), n) - assert.NoError(t, p1.Close()) - assert.Equal(t, ``, w.String()) - - r2 := bytes.NewBufferString(`{}`) - p2 := merger.NewPage(r2, true) - n, err = io.Copy(w, p2) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.NoError(t, p2.Close()) - assert.Equal(t, ``, w.String()) - - require.NoError(t, merger.Close()) - assert.JSONEq(t, `{"a":1,"b":2}`, w.String()) -} - -func TestObjectMerger_multiplePages(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - r1 := bytes.NewBufferString(`{"a":1,"b":2,"arr":["a","b"]}`) - p1 := merger.NewPage(r1, false) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.NoError(t, p1.Close()) - assert.Equal(t, ``, w.String()) - - r2 := bytes.NewBufferString(`{"b":3,"c":{"d":4},"arr":["c","d"]}`) - p2 := merger.NewPage(r2, true) - n, err = io.Copy(w, p2) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.NoError(t, p2.Close()) - assert.Equal(t, ``, w.String()) - - require.NoError(t, merger.Close()) - assert.JSONEq(t, `{"a":1,"b":3,"c":{"d":4},"arr":["a","b","c","d"]}`, w.String()) -} - -func TestObjectMerger_invalidJSON(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - r1 := bytes.NewBufferString(`invalid`) - p1 := merger.NewPage(r1, true) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.Error(t, p1.Close()) -} - -func TestObjectMerger_array(t *testing.T) { - w := &bytes.Buffer{} - merger := NewObjectMerger(w) - - r1 := bytes.NewBufferString(`[]`) - p1 := merger.NewPage(r1, true) - n, err := io.Copy(w, p1) - require.NoError(t, err) - assert.Equal(t, int64(0), n) - assert.Error(t, p1.Close()) -} diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 35b99eef6ae..c821c84c87f 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -20,7 +20,6 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/internal/jsonmerge" "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -30,6 +29,10 @@ import ( "github.com/spf13/cobra" ) +const ( + ttyIndent = " " +) + type ApiOptions struct { AppVersion string BaseRepo func() (ghrepo.Interface, error) @@ -37,7 +40,6 @@ type ApiOptions struct { Config func() (config.Config, error) HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - merger jsonmerge.Merger Hostname string RequestMethod string @@ -50,7 +52,7 @@ type ApiOptions struct { Previews []string ShowResponseHeaders bool Paginate bool - MergePages bool + Slurp bool Silent bool Template string CacheTTL time.Duration @@ -174,8 +176,8 @@ 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=' + # without --slurp you will get a different percentage for each page + $ gh api graphql --paginate --slurp -f query=' query($endCursor: String) { viewer { repositories(first: 100, after: $endCursor) { @@ -187,8 +189,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command } } } - ' | jq 'def count(s): reduce s as $_ (0;.+1); - .data.viewer.repositories.nodes as $r | count(select($r[].isFork)) / count($r[])' + ' | jq 'def count(e): reduce e as $_ (0;.+1); + [.[].data.viewer.repositories.nodes[]] as $r | count(select($r[].isFork))/count($r[])' `), Annotations: map[string]string{ "help:environment": heredoc.Doc(` @@ -231,8 +233,17 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command return err } - if opts.MergePages && !opts.Paginate { - return cmdutil.FlagErrorf("`--paginate` required when passing `--merge-pages`") + if opts.Slurp && !opts.Paginate { + return cmdutil.FlagErrorf("`--paginate` required when passing `--slurp`") + } + + if err := cmdutil.MutuallyExclusive( + "the `--slurp` option is not supported with `--jq` or `--template`", + opts.Slurp, + opts.FilterOutput != "", + opts.Template != "", + ); err != nil { + return err } if err := cmdutil.MutuallyExclusive( @@ -259,7 +270,7 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add a HTTP request header in `key:value` format") cmd.Flags().StringSliceVarP(&opts.Previews, "preview", "p", nil, "GitHub API preview `names` to request (without the \"-preview\" suffix)") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response status line and headers in the output") - cmd.Flags().BoolVar(&opts.MergePages, "merge-pages", false, "Use with \"--paginate\" to merge all pages of JSON arrays or objects when piping or redirecting standard output") + cmd.Flags().BoolVar(&opts.Slurp, "slurp", false, "Use with \"--paginate\" to return an array of all pages of either JSON arrays or objects") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The `file` to use as body for the HTTP request (use \"-\" to read from standard input)") cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Do not print the response body") @@ -307,20 +318,16 @@ func apiRun(opts *ApiOptions) error { requestPath = addPerPage(requestPath, 100, params) } - // 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.FilterOutput == "" && opts.Template == "" { - if !isGraphQL { - opts.merger = jsonmerge.NewArrayMerger() - } else if opts.MergePages { - opts.merger = jsonmerge.NewObjectMerger(bodyWriter) + // Similar to `jq --slurp`, write all pages JSON arrays or objects into a JSON array. + if opts.Paginate && opts.Slurp { + w := &jsonArrayWriter{ + Writer: bodyWriter, + color: opts.IO.ColorEnabled(), } - } + defer w.Close() - if opts.merger == nil { - opts.merger = jsonmerge.NewNopMerger() + bodyWriter = w } - defer opts.merger.Close() if opts.RequestInputFile != "" { file, size, err := openUserFile(opts.RequestInputFile, opts.IO.In) @@ -400,6 +407,12 @@ func apiRun(opts *ApiOptions) error { requestBody = nil // prevent repeating GET parameters } + // Tell optional jsonArrayWriter to start a new page. + err = startPage(bodyWriter) + if err != nil { + return err + } + endCursor, err := processResponse(resp, opts, bodyWriter, headersWriter, tmpl, isFirstPage, !hasNextPage) if err != nil { return err @@ -459,7 +472,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW // TODO: reuse parsed query across pagination invocations indent := "" if opts.IO.IsStdoutTTY() { - indent = " " + indent = ttyIndent } err = jq.EvaluateFormatted(responseBody, bodyWriter, opts.FilterOutput, indent, opts.IO.ColorEnabled()) if err != nil { @@ -471,20 +484,16 @@ func processResponse(resp *http.Response, opts *ApiOptions, bodyWriter, headersW return } } else if isJSON && opts.IO.ColorEnabled() { - err = jsoncolor.Write(bodyWriter, responseBody, " ") + err = jsoncolor.Write(bodyWriter, responseBody, ttyIndent) } else { - if isJSON && opts.Paginate && !opts.ShowResponseHeaders { - responseBody = opts.merger.NewPage(responseBody, isLastPage) + if isJSON && opts.Paginate && !opts.Slurp && !isGraphQLPaginate && !opts.ShowResponseHeaders { + responseBody = &paginatedArrayReader{ + Reader: responseBody, + isFirstPage: isFirstPage, + isLastPage: isLastPage, + } } - _, err = io.Copy(bodyWriter, responseBody) - if err != nil { - return - } - - if closer, ok := responseBody.(io.ReadCloser); ok { - err = closer.Close() - } } if err != nil { return diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 0d02eff6146..5244f04d480 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -329,8 +329,18 @@ func Test_NewCmdApi(t *testing.T) { wantsErr: true, }, { - name: "--merge-pages without --paginate", - cli: "user --merge-pages", + name: "--slurp without --paginate", + cli: "user --slurp", + wantsErr: true, + }, + { + name: "slurp with --jq", + cli: "user --paginate --slurp --jq .foo", + wantsErr: true, + }, + { + name: "slurp with --template", + cli: "user --paginate --slurp --template '{{.foo}}'", wantsErr: true, }, { @@ -993,7 +1003,7 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { assert.Equal(t, "PAGE1_END", endCursor) } -func Test_apiRun_paginationGraphQL_merge(t *testing.T) { +func Test_apiRun_paginationGraphQL_slurp(t *testing.T) { ios, _, stdout, stderr := iostreams.Test() requestCount := 0 @@ -1047,121 +1057,33 @@ func Test_apiRun_paginationGraphQL_merge(t *testing.T) { RequestMethod: "POST", RequestPath: "graphql", Paginate: true, - MergePages: true, + Slurp: 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_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{ + assert.JSONEq(t, stdout.String(), `[ { - 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 - } + "data": { + "nodes": ["page one"], + "pageInfo": { + "endCursor": "PAGE1_END", + "hasNextPage": true } - }`))), - }, - } - - 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 + "data": { + "nodes": ["page two"], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } } } - }`) + ]`) assert.Equal(t, "", stderr.String(), "stderr") var requestData struct { diff --git a/pkg/cmd/api/pagination.go b/pkg/cmd/api/pagination.go index 65d816480e4..bf4a2f794dc 100644 --- a/pkg/cmd/api/pagination.go +++ b/pkg/cmd/api/pagination.go @@ -8,6 +8,8 @@ import ( "net/url" "regexp" "strings" + + "github.com/cli/cli/v2/pkg/jsoncolor" ) var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) @@ -106,3 +108,134 @@ func addPerPage(p string, perPage int, params map[string]interface{}) string { return fmt.Sprintf("%s%sper_page=%d", p, sep, perPage) } + +// paginatedArrayReader wraps a Reader to omit the opening and/or the closing square bracket of a +// JSON array in order to apply pagination context between multiple API requests. +type paginatedArrayReader struct { + io.Reader + isFirstPage bool + isLastPage bool + + isSubsequentRead bool + cachedByte byte +} + +func (r *paginatedArrayReader) Read(p []byte) (int, error) { + var n int + var err error + if r.cachedByte != 0 && len(p) > 0 { + p[0] = r.cachedByte + n, err = r.Reader.Read(p[1:]) + n += 1 + r.cachedByte = 0 + } else { + n, err = r.Reader.Read(p) + } + if !r.isSubsequentRead && !r.isFirstPage && n > 0 && p[0] == '[' { + if n > 1 && p[1] == ']' { + // empty array case + p[0] = ' ' + } else { + // avoid starting a new array and continue with a comma instead + p[0] = ',' + } + } + if !r.isLastPage && n > 0 && p[n-1] == ']' { + // avoid closing off an array in case we determine we are at EOF + r.cachedByte = p[n-1] + n -= 1 + } + r.isSubsequentRead = true + return n, err +} + +// jsonArrayWriter wraps a Writer which writes multiple pages of both JSON arrays +// and objects. Call Close to write the end of the array. +type jsonArrayWriter struct { + io.Writer + started bool + color bool +} + +func (w *jsonArrayWriter) Preface() []json.Delim { + if w.started { + return []json.Delim{'['} + } + return nil +} + +// ReadFrom implements io.ReaderFrom to write more data than read, +// which otherwise results in an error from io.Copy(). +func (w *jsonArrayWriter) ReadFrom(r io.Reader) (int64, error) { + var written int64 + buf := make([]byte, 4069) + for { + n, err := r.Read(buf) + if n > 0 { + n, err := w.Write(buf[:n]) + written += int64(n) + + if err != nil { + return written, err + } + } + if err == io.EOF { + break + } + if err != nil { + return written, err + } + } + + return written, nil +} + +func (w *jsonArrayWriter) Close() error { + var delims string + if w.started { + delims = "]" + } else { + delims = "[]" + } + + w.started = false + if w.color { + return jsoncolor.WriteDelims(w, delims, ttyIndent) + } + + _, err := w.Writer.Write([]byte(delims)) + return err +} + +func startPage(w io.Writer) error { + if jaw, ok := w.(*jsonArrayWriter); ok { + var delims string + var indent bool + + if !jaw.started { + delims = "[" + jaw.started = true + } else { + delims = "," + indent = true + } + + if jaw.color { + if indent { + _, err := jaw.Write([]byte(ttyIndent)) + if err != nil { + return err + } + } + + return jsoncolor.WriteDelims(w, delims, ttyIndent) + } + + _, err := jaw.Write([]byte(delims)) + if err != nil { + return err + } + } + + return nil +} diff --git a/pkg/cmd/api/pagination_test.go b/pkg/cmd/api/pagination_test.go index 3bb1a8ec5c3..746a73c4ac5 100644 --- a/pkg/cmd/api/pagination_test.go +++ b/pkg/cmd/api/pagination_test.go @@ -5,6 +5,9 @@ import ( "io" "net/http" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_findNextPage(t *testing.T) { @@ -167,3 +170,107 @@ func Test_addPerPage(t *testing.T) { }) } } + +func TestJsonArrayWriter(t *testing.T) { + tests := []struct { + name string + pages []string + want string + }{ + { + name: "empty", + pages: nil, + want: "[]", + }, + { + name: "single array", + pages: []string{`[1,2]`}, + want: `[[1,2]]`, + }, + { + name: "multiple arrays", + pages: []string{`[1,2]`, `[3]`}, + want: `[[1,2],[3]]`, + }, + { + name: "single object", + pages: []string{`{"foo":1,"bar":"a"}`}, + want: `[{"foo":1,"bar":"a"}]`, + }, + { + name: "multiple pages", + pages: []string{`{"foo":1,"bar":"a"}`, `{"foo":2,"bar":"b"}`}, + want: `[{"foo":1,"bar":"a"},{"foo":2,"bar":"b"}]`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + buf := &bytes.Buffer{} + w := &jsonArrayWriter{ + Writer: buf, + } + + for _, page := range tt.pages { + require.NoError(t, startPage(w)) + + n, err := w.Write([]byte(page)) + require.NoError(t, err) + assert.Equal(t, len(page), n) + } + + require.NoError(t, w.Close()) + assert.Equal(t, tt.want, buf.String()) + }) + } +} + +func TestJsonArrayWriter_Copy(t *testing.T) { + tests := []struct { + name string + limit int + }{ + { + name: "unlimited", + }, + { + name: "limited", + limit: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + buf := &bytes.Buffer{} + w := &jsonArrayWriter{ + Writer: buf, + } + + r := &noWriteToReader{ + Reader: bytes.NewBufferString(`[1,2]`), + limit: tt.limit, + } + + require.NoError(t, startPage(w)) + + n, err := io.Copy(w, r) + require.NoError(t, err) + assert.Equal(t, int64(5), n) + + require.NoError(t, w.Close()) + assert.Equal(t, `[[1,2]]`, buf.String()) + }) + } +} + +type noWriteToReader struct { + io.Reader + limit int +} + +func (r *noWriteToReader) Read(p []byte) (int, error) { + if r.limit > 0 { + p = p[:r.limit] + } + return r.Reader.Read(p) +} diff --git a/pkg/jsoncolor/jsoncolor.go b/pkg/jsoncolor/jsoncolor.go index dbe3d9a4b42..8e20a11611a 100644 --- a/pkg/jsoncolor/jsoncolor.go +++ b/pkg/jsoncolor/jsoncolor.go @@ -16,6 +16,10 @@ const ( colorBool = "33" // yellow ) +type JsonWriter interface { + Preface() []json.Delim +} + // Write colorized JSON output parsed from reader func Write(w io.Writer, r io.Reader, indent string) error { dec := json.NewDecoder(r) @@ -24,6 +28,10 @@ func Write(w io.Writer, r io.Reader, indent string) error { var idx int var stack []json.Delim + if jsonWriter, ok := w.(JsonWriter); ok { + stack = append(stack, jsonWriter.Preface()...) + } + for { t, err := dec.Token() if err == io.EOF { @@ -96,6 +104,20 @@ func Write(w io.Writer, r io.Reader, indent string) error { return nil } +// WriteDelims writes delims in color and with the appropriate indent +// based on the stack size returned from an io.Writer that implements JsonWriter.Preface(). +func WriteDelims(w io.Writer, delims, indent string) error { + var stack []json.Delim + if jaw, ok := w.(JsonWriter); ok { + stack = jaw.Preface() + } + + fmt.Fprintf(w, "\x1b[%sm%s\x1b[m", colorDelim, delims) + fmt.Fprint(w, "\n", strings.Repeat(indent, len(stack))) + + return nil +} + // marshalJSON works like json.Marshal but with HTML-escaping disabled func marshalJSON(v interface{}) ([]byte, error) { buf := bytes.Buffer{} From a76af8588c817d88e17f28460c483debb1e50163 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 14 Mar 2024 00:22:45 -0700 Subject: [PATCH 09/11] Resolve PR comments --- pkg/cmd/api/api.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index c821c84c87f..7bd9fcd0d28 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -120,8 +120,8 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command there are no more pages of results. For GraphQL requests, this requires that the original query accepts an %[1]s$endCursor: String%[1]s variable and that it fetches the %[1]spageInfo{ hasNextPage, endCursor }%[1]s set of fields from a collection. Each page is a separate - JSON array or object. Pass %[1]s--merge-pages%[1]s to merge all pages into a single JSON array or object - when piping or redirecting standard output. + JSON array or object. Pass %[1]s--slurp%[1]s to wrap all pages of JSON arrays or objects + into an outer JSON array. `, "`"), Example: heredoc.Doc(` # list releases in the current repository From 4ea7bcacb3b67c99bfb9df64b64a30469180b8ac Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Thu, 4 Apr 2024 01:05:10 -0700 Subject: [PATCH 10/11] Run defers in queue --- pkg/cmd/api/api.go | 22 ++++++++++++++++++++-- pkg/cmd/api/api_test.go | 17 +++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 7bd9fcd0d28..f835cec4501 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -318,13 +318,19 @@ func apiRun(opts *ApiOptions) error { requestPath = addPerPage(requestPath, 100, params) } + // Execute defers in FIFO order. + deferQueue := queue{} + defer deferQueue.Close() + // Similar to `jq --slurp`, write all pages JSON arrays or objects into a JSON array. if opts.Paginate && opts.Slurp { w := &jsonArrayWriter{ Writer: bodyWriter, color: opts.IO.ColorEnabled(), } - defer w.Close() + deferQueue.Enqueue(func() { + _ = w.Close() + }) bodyWriter = w } @@ -376,7 +382,7 @@ func apiRun(opts *ApiOptions) error { if !opts.Silent { if err := opts.IO.StartPager(); err == nil { - defer opts.IO.StopPager() + deferQueue.Enqueue(opts.IO.StopPager) } else { fmt.Fprintf(opts.IO.ErrOut, "failed to start pager: %v\n", err) } @@ -681,3 +687,15 @@ func previewNamesToMIMETypes(names []string) string { } return strings.Join(types, ", ") } + +type queue []func() + +func (q *queue) Enqueue(f func()) { + *q = append(*q, f) +} + +func (q *queue) Close() { + for _, f := range *q { + f() + } +} diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 5244f04d480..944eaa4c95a 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -1760,3 +1760,20 @@ func Test_apiRun_acceptHeader(t *testing.T) { }) } } + +func TestQueue_Close(t *testing.T) { + sut := queue{} + actual := make([]int, 0, 2) + + func() { + defer sut.Close() + sut.Enqueue(func() { + actual = append(actual, 0) + }) + sut.Enqueue(func() { + actual = append(actual, 1) + }) + }() + + assert.Equal(t, []int{0, 1}, actual) +} From 2758b8001349bbef4181ea2ff552cdeb88dc7ff6 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Mon, 15 Apr 2024 21:38:16 -0700 Subject: [PATCH 11/11] Remove unnecessary --help comment --- pkg/cmd/api/api.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index f835cec4501..62459c0d6a3 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -176,7 +176,6 @@ func NewCmdApi(f *cmdutil.Factory, runF func(*ApiOptions) error) *cobra.Command ' # get the percentage of forks for the current user - # without --slurp you will get a different percentage for each page $ gh api graphql --paginate --slurp -f query=' query($endCursor: String) { viewer {