Merge JSON responses from gh api

[heaths]

Partly resolves #1268 and replaces #5652. Requires cli/go-gh#148 to be merged and optionally released.

• Move Support merging JSON arrays, objects go-gh#148 under internal package.
• Resolve PR comments therein.
• Migrate merge tests from Merge JSON responses from gh api #5652.
• Put changes under mutually exclusive --paginate-all switch.

See cli/go-gh#148 for a full discussion.

[heaths]

Whoops. Forgot we decided to do this as a new parameter: --paginate-all, IIRC. I'll fix that before I finalize this PR, but figured it still shows how the go-gh work plugs in neatly.

Could we rebase merge this when done? I will keep the commits clean. Might be good to keep the current commit with the necessary logic. Alternatively, I could put in some TODO comments for a future change.

[williammartin]

Thanks for providing this as an example of how jsonmerge would be called, it was very useful to my understanding. Agree we need --paginate-all or --slurp. Either seems reasonable to me.

Could we rebase merge this when done?

Not sure what the intent is here sorry. I don't understand what rebasing on trunk is getting you, or in the case you mean squash rebase then merge, what that would get you either. I'm just missing something obvious I suspect.

[heaths]

The eventual goal after talking with @andyfeller was to deprecate --paginate-all and bake this into --paginate. By rebasing onto trunk and merging - without squashing commits - the changes needed to wire up --paginate are retained; otherwise, I can add a bunch of comments for how it could be done.

More comments will be over on cli/go-gh#148.

[heaths]

@williammartin I ran into an issue when porting some of my old tests from #5652 that would be better served by bifurcating the logic. I will try to work around it given our discussion in cli/go-gh#148, but the problem is basically that the existing code only merges if there's no template; however, that ends up executing the template for each page, so the table alignment from {{tablerow}} I added a while back doesn't work.

Consider two pages:

[{"id":1,"title":"one"}]

[{"id":20,"title":"twenty"}]

It should render as:

1   one
20  twenty

However, because the template is executed for each page, that first page width is 1 less character:

1  one
20  twenty

Because of the refactoring I made in #5652 it actually worked correctly. The template execution happened later after merging was complete. And this worked for both REST and GraphQL responses because either were cached until the last page was read and merged. Because Nate's solution for REST responses no longer caches - which I've maintained here for reasons we discussed in #5652 - I could only do this for GraphQL responses currently.

Assuming I can work around this - and I'm updating status here in case I don't finish tonight, but I'll push what I have with that one test disabled for now - the future refactoring should take this into account.

UPDATE: I was able to work around this. Actually, it was because my old test intentionally added {{tablerender}} to make sure that arrays - even within objects - were first merged. The misalignment happens even with the current gh release, so this isn't a regression. I restored the test to what it was, which will render the template as a whole once flushed at the very end of apiRun. Still, what I suggested above is appropriate: if we were to refactor this, we'd still want to ideally merge REST arrays - or at least GraphQL objects - before passing to a template or filter. At that point, we no longer need to merge only if no --template option was passed. All that should still technically work, and work better because we template the merged pages, so rows would be aligned regardless of the presence of an explicit {{tablerender}}.

[heaths]

Given further discussions about stability, @andyfeller @williammartin do we still want to add --paginate-all? I'm working on the changes now, and it certainly increases the complexity of several conditions. It's not that it's complicated to implement, but I worry has negative ramifications to users' experience...at least in error cases.

UPDATE: Thinking about this more, maybe we should consider a --slurp in addition to --paginate i.e., both are required. @andyfeller, Sam, and I discussed this late last year, but I believe the reason we opted for --paginate-all because the parameters are sorted and this would put --paginate-all right after --paginate whereas --slurp would come after --silent several parameters down. Perhaps --merge would at least be closer, and I think would make both the user experience and maintenance easier because we don't have to distinguish between --paginate and --paginate-all in some cases but not others.

Still, marking the PR ready. I updated everything per discussions, but I'm open to changing a few things as suggested above.

[williammartin]

Because of the two comments that were also later updated I'm not sure how much of them is still considered relevant, so excuse me if these questions seem to have obvious answers.

Given further discussions about stability, @andyfeller @williammartin do we still want to add --paginate-all?

I don't understand your question. Are you talking about the flag, or the behaviour?

It's not that it's complicated to implement, but I worry has negative ramifications to users' experience...at least in error cases.

What error cases?

Thinking about this more, maybe we should consider a --slurp in addition to --paginate i.e., both are required.

I don't really have a strong opinion but I'm also not really following why you would prefer --slurp or --merge to --paginate-all. What's your take? If you asked me to flip a coin I would say --slurp as additive to --paginate.

Re: all the --template stuff, I'll have to dig a little more.

Thanks!

[heaths]

--paginate and --paginate-all are mutually exclusive. That means all errors when you specified --paginate e.g., with --input have to be updated to mention both of them. For code maintenance, in most cases I have to check either ApiOptions.Paginate or ApiOptions.PaginateAll (made a simple helper for leaner code in those cases) but in other cases (one, used as a proxy and henceforth the presence of a Merger is used) just check ApiOptions.PaginateAll. If we used - hopefully just temporarily - a separate option like --slurp or --merge (personally, I feel "slurp" probably won't be well-understood by most where "merge" should be easier to understand) in addition to --paginate, I think the usage and code are simplified. Want merged results as a user? Pass --merge. Want to know if we should page and merge? Check ApiOptions.Merge in addition to existing checks against ApiOptions.Paginate.

The discussion @andyfeller, Sam, and I had back in December was that this would be temporary until we see enough usage of merged pagination without regressions - even though I think, with the addition of more tests I added, we have a good battery of tests. I appreciate the emphasis on backward compatibility where it makes sense to.

[williammartin]

Understood. The only reason I leaned slightly towards --slurp is because merge is an overloaded term. --merge-pages could be a compromise. If we went with --merge I wouldn’t put up any kind of fight though, it's not maddeningly confusing given the context.

The discussion @andyfeller, Sam, and I had back in December was that this would be temporary until we see enough usage of merged pagination without regressions

To be specific, the follow up notes from the call target this behavioural change for a major version:

When we consider future 3.0 plans, that might be a good time to make this the default behavior

I’m sure this is clear to you already but for the sake of posterity for anyone reading this in the future, the issue at hand is that any user depending on interrogating the output of something like:

gh api graphql --paginate -f query='
    query($endCursor: String) {
      viewer {
        repositories(first: 100, after: $endCursor) {
          nodes { nameWithOwner }
          pageInfo {
            hasNextPage
            endCursor
          }
        }
      }
    }
  '

might be broken by changing --paginate to work like --paginate-all. I’m not sure of any reasonable way we could be sure we aren’t breaking any users here. On the other hand, as Linus says:

The "no regressions" rule is not about made-up "if I do this, behavior changes".

The "no regressions" rule is about users.

If you have an actual user that has been doing insane things, and we
change something, and now the insane thing no longer works, at that
point it's a regression, and we'll sigh, and go "Users are insane" and
have to fix it.

Not that our users would be insane for this but that if a CLI regresses and no one is around to notice it, did it really regress.

[heaths]

might be broken by changing --paginate to work like --paginate-all

Possibly, but that seems incredibly unlikely. The CLI would emit multiple pages like so:

{
  "data": {
    "viewer": {
      "repositories": {
        "nodes": [{"nameWithOwner": "heaths/cli"}]
      }
    }
  }
}
{
  "data": {
    "viewer": {
      "repositories": {
        "nodes": [{"nameWithOwner": "heaths/cli"}]
      }
    }
  }
}

Currently, that's invalid JSON. You can't pass it to jq or PowerShell's ConvertFrom-Json, for example. At least jq has a --slurp option, but you'd have to understand what that even means whereas --merge-pages is pretty intuitive.

Now, lets say that someone wrote a script (as I did before starting the original work) that looks for }{ with or without line breaks so that they could parse those pages separately. Such a script wouldn't be broken by the lack of a match now. And if that script further sent each page through to another tool that parsed JSON, that wouldn't be broken either by a single page.

That said, and re:

To be specific, the #1268 (comment) from the call target this behavioural change for a major version:

Thank you. I had forgotten not only that part of the discussion about the next major version but didn't realize or forgot we wrote down meeting notes in the issue. That does make sense, and while I think the risk of regression is slim to none, I agree it makes sense to wait to truly fix this in v3.

The only reason I leaned slightly towards --slurp is because merge is an overloaded term. --merge-pages could be a compromise.

I like --merge-pages. --merge is vague, you're right, though I think it's still better than --slurp which is probably only understood by those that know jq. In the vernacular, it means, "eat or drink (something) with a loud sloppy sucking noise". Why jq chose it I don't know, but I'm a fan of keeping things as obvious as possible without being..."cute". (Don't get me started on Rusts "yeet"! 😉)

[williammartin]

Currently, that's invalid JSON. You can't pass it to jq

I was surprised to find that at least on my machine jq actually is happy to query over multiple documents e.g. the query 'jq .data.viewer.repositories.nodes[].nameWithOwner' will return both namesWithOwner. However, after that things start get out of hand...try this one on for style with your example above:

jq -n 'reduce inputs as $i (""; . + " " + $i.data.viewer.repositories.nodes[].nameWithOwner)'

jq will actually allow you to use the provided documents as an array which opens up a world of madness that ends up with different behaviours between --paginate and --paginate-all output. My point isn't that this is a sensible thing to do (and any realistic example should learn to handle the array in each document correctly) just that I find it very hard to imagine the way in which things might break even just through usage of jq. We seem to be struggling with breaking things we should know about as it is 😅

That does make sense, and while I think the risk of regression is slim to none, I agree it makes sense to wait to truly fix this in v3.

I also think that most likely regressions are unlikely and it's actually likely that most regressions would have been indicative of a buggy script but I think waiting till v3 is the safest option.

but I'm a fan of keeping things as obvious as possible without being..."cute".

--merge-pages is my top choice at the moment, though probably worth @andyfeller weighing in.

Don't get me started on Rusts "yeet"! 😉

I've given enough projects terrible names in my time to keep my mouth zipped 😬

[williammartin]

Me, reading the jq docs.

[heaths]

I'll rename the switch and rejigger a bit of the logic and let you know. Probably tonight. Looks like I need to rebase again, but at least this has been clean so far unlike my first PR for this issue.

BTW, sorry for updating comments previously. I've typically done this when I expect the person is either 1) further away with a large offset in time zone, or 2) is close to me, but I'm active at a time later than they would usually be. It's the whole chunky vs. chatty discussion dilemma. While it may not have helped in this case (since I was up late and all I can glean from your profile is that you at least visited the Golden Gate bridge at some point), I did file a feature request: https://github.com/orgs/community/discussions/102931

I find the TZ offset in Teams to help communicating with partner teams to help decide if I should having a chunky reply or expect a chatty back-and-forth.

[heaths]

Me, reading the jq docs.

I frequently have to switch tools - and always have to refer back to docs - that use either jq syntax, JSONPath, or JMESPath. All similar but different enough it's a nightmare to keep straight which does what and how.

[williammartin]

Ah well for future reference I'm in Amsterdam, NL and I've updated my profile to reflect that. Not a problem with the updates.

[heaths]

@williammartin this should be ready now. I removed the "experimental" notes given this is an additional flag that still requires --paginate, but feel free, of course, to update the text if you'd like, assuming no other changes you'd like.

[heaths]

Wait: something is wrong. Tests pass, but I just tried it in a use case and it still emits separate pages and a final blank {}. This worked previously. 😕 Debugging...

[heaths]

@williammartin I realized the problem: I forgot, to maintain backcompat, this only works when piping or redirecting stdout. When I was testing it, I was just printing to the terminal. Still, that showed me that objectMerger.Close had to be conditional - at least, that was the easiest way to deal with it. Added a test as well.

To note, jq worked for you before because it doesn't seem to care if there are multiple top-level objects or arrays and still formats them as such. --slurp just means it merged those objects or arrays before formatting. Effectively, gh api --merge-pages now works like jq --slurp. I noticed - and further tested this - when debugging this last problem since I remembered you saying it already seemed to work for you. It wouldn't have worked with less...permissive...processes further down the pipe e.g., ConvertFrom-Json in PowerShell (which is what we are using in our scripts that originally put me on this path).

For a future v3, there's definitely a lot of room for improvement. For example, I think the merging behavior should be default but, IMO, I would use mergo for both JSON arrays and objects like #5652 did. Not only is it straight forward, but I see little value in "streaming" each page given the typical use cases. I fully appreciate that's a risk you're not willing to take now for v2. I could see this diverging down separate code paths depending on the presence of graphql (or maybe that convention even changes) but in both code paths JSON is merged into a top-level array or object based on the first page like I did here: https://github.com/heaths/cli/blob/1a7f803241123d36c9ce1ab3893b498fe1d71e89/pkg/cmd/api/pagination.go#L129-L140. This then works for a top-level array or object from REST. And for GraphQL, you wouldn't actually need to tee the response body to later find the cursor since you already have the merge buffer i.e., just a single buffer is needed. Just some thoughts for posterity.

[heaths]

PS: seems the pr-auto bot doesn't like it when a PR is readied again: says the PR is already linked to a project. It's not required, but just wanted to point that out since I took a peek as to why it failed.

[heaths]

Just need to address the trailing ] I think.

Any idea why I can't page when running go run ./cmd/gh ...? I've debugged through it. Everything should be working. I've exported PAGER as well as setting it on the command line e.g., PAGER=less go run ./cmd/gh .... Not asking you to debug into it or anything, but wondering if you've had any similar experience or some idea. I've been trying a couple different things to no avail.

[heaths]

Not that I don't believe you...just so I can repro it and fix it. Still trying to think how without getting "too creative" e.g., using a list of funcs to which I can add differs (I often use a similar "trick" in complex PowerShell scripts).

[williammartin]

Any idea why I can't page when running go run ./cmd/gh ...?

Not sure, it seems to work for me on whatever set up I have on mac. I can't think of any reason go run would act differently, I thought it just compiled the binary to a temp dir and ran it.

What's your set up? Maybe I can try and repro in my Windows VM.

[heaths]

What's your set up? Maybe I can try and repro in my Windows VM.

I'm actually doing this on a pure Ubuntu box (and even on Windows I use WSL). With quick-and-dirty println debugging, I've confirmed the env var is definitely getting plumbed through. Let me try a few other things, like do the same test running against main. While it seems none of my changes should make a difference, maybe somehow they did. The only thing that comes to mind is moving where the output streams are declared and possibly initialized, but I reviewed that before and it seems unlikely.

[andyfeller]

@heaths : handful of questions for you:

1. How are you accounting for all of the various ways that paging is configured?

   From how I read the code below, there are several things that affect paging with PAGER being the lowest precedence.

2. How does this compare when compiled versus go run?

   Normally I wouldn't assume this is a factor but worth comparing.

3. How does this compare with and without explicit piping?

   I think this was a question that @williammartin raised earlier, unsure if this yielded any results.

   If you pipe the output through jq - which will require valid JSON - does it work?

   Yes, but I would expect that since the pager only starts if connected to a TTY.

I'm going to try testing this on a variety of physical devices to see if I can't reproduce.

cli/pkg/cmd/factory/default.go

Lines 178 to 202 in 90b7bf9

	func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams {
	io := iostreams.System()
	cfg, err := f.Config()
	if err != nil {
	return io
	}
	if _, ghPromptDisabled := os.LookupEnv("GH_PROMPT_DISABLED"); ghPromptDisabled {
	io.SetNeverPrompt(true)
	} else if prompt := cfg.Prompt(""); prompt == "disabled" {
	io.SetNeverPrompt(true)
	}
	// Pager precedence
	// 1. GH_PAGER
	// 2. pager from config
	// 3. PAGER
	if ghPager, ghPagerExists := os.LookupEnv("GH_PAGER"); ghPagerExists {
	io.SetPager(ghPager)
	} else if pager := cfg.Pager(""); pager != "" {
	io.SetPager(pager)
	}
	return io
	}

[andyfeller]

Just reaffirming what @williammartin reported above, I see the hanging ] using go1.21.8 on MacOS and running the following:

make

QUERY='                                                                
query($endCursor: String) {
    viewer {
        repositories(first: 10, after: $endCursor) {
            nodes { nameWithOwner, isFork }
            pageInfo {
                hasNextPage
                endCursor
            }
        }
    }
}
'

GH_PAGER=less ./bin/gh api graphql --paginate --slurp -f query="$QUERY"

this results in the hanging ] outside of less:

andyfeller@Andys-MBP:cli/cli ‹merge-json›$ GH_PAGER=less ./bin/gh api graphql --paginate --slurp -f query="$QUERY"
]
andyfeller@Andys-MBP:cli/cli ‹merge-json›$

the same result even running it via go run:

andyfeller@Andys-MBP:cli/cli ‹merge-json›$ GH_PAGER=less go run ./cmd/gh/main.go api graphql --paginate --slurp -f query="$QUERY" 
]
andyfeller@Andys-MBP:cli/cli ‹merge-json›$ 

[andyfeller]

Doing testing on physical Windows 10 laptop in Git Bash with go1.21.7, I do notice that Windows behaves differently:

1. GH_PAGER=less does not show slurped data in alternative mode like Mac + Zsh
2. GH_PAGER=more has a notable difference in the closing ] than less as there is a separation reminiscent of Mac testing of ] coming after the pager is closed

I was digging into how gh detects if the terminal has support for alternative mode and trying to force that to showcase the problem on Windows, however setting TERM and/or COLORTERM to truecolor wasn't enough to trick it from printing the JSON to stdout.

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (ggm-issue-8677-tinker)
$ gh pr list --author heaths

Showing 1 of 1 pull request in cli/cli that matches your search

ID     TITLE                               BRANCH             CREATED AT
#8620  Merge JSON responses from `gh api`  heaths:merge-json  about 2 months ago

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (ggm-issue-8677-tinker)
$ gh pr checkout 8620
From https://github.com/cli/cli
 * [new ref]           refs/pull/8620/head -> merge-json
Switched to branch 'merge-json'

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (merge-json)
$ make
go build -o script/build.exe script/build.go
go build -trimpath -ldflags "-X github.com/cli/cli/v2/internal/build.Date=2024-04-02 -X github.com/cli/cli/v2/internal/build.Version=v2.45.0-13-gf276971b " -o bin/gh.exe ./cmd/gh
go: downloading github.com/cli/go-gh/v2 v2.6.0

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (merge-json)
$ QUERY='
query($endCursor: String) {
    viewer {
        repositories(first: 100, after: $endCursor) {
            nodes { nameWithOwner, isFork }
            pageInfo {
                hasNextPage
                endCursor
            }
        }
    }
}
'

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (merge-json)
$ GH_PAGER=less ./bin/gh.exe api graphql --paginate --slurp -f query="$QUERY"
[
  {
    "data": {
      "viewer": {
        "repositories": {
          "nodes": [
            {
              "nameWithOwner": "community/maintainers",
              "isFork": false
            },
            {
              "nameWithOwner": "githubtraining/training-manual",
              "isFork": false
            },
            {
              "nameWithOwner": "andyfeller/docker-alpine-abuild",
              "isFork": true
            },
            {
              "nameWithOwner": "andyfeller/docker.github.io",
              "isFork": true
            },
            ...
          ],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": "Y3Vyc29yOnYyOpHOLl0I8w=="
          }
        }
      }
    }
  }
]

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (merge-json)
$ GH_PAGER=more ./bin/gh.exe api graphql --paginate --slurp -f query="$QUERY"
[
  {
    "data": {
      "viewer": {
        "repositories": {
          "nodes": [
            {
              "nameWithOwner": "community/maintainers",
              "isFork": false
            },
            {
              "nameWithOwner": "githubtraining/training-manual",
              "isFork": false
            },
            {
              "nameWithOwner": "andyfeller/docker-alpine-abuild",
              "isFork": true
            },
            {
              "nameWithOwner": "andyfeller/docker.github.io",
              "isFork": true
            },
            ...
          ],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": "Y3Vyc29yOnYyOpHOLl0I8w=="
          }
        }
      }
    }
  }

]

andre@DESKTOP-A5BGD8P MINGW64 ~/Documents/cli/cli (merge-json)
$

[heaths]

Thanks. I'm not testing this in Windows, though. I'm in WSL, which is a linux kernel and binaries. I'm also not seeing it in my Ubuntu-on-bare-metal laptop. I don't deny it exists, but hard to verify a fix if I can't reproduce it.

That said, the reason why it happens is well-understood. I will try to come up with an alternative approach, but I have so little time these past few weeks and this coming week or two.

[williammartin]

These most recent changes resolve the paging issue for me!

Snippet from my paging output:

...
         ],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": "Y3Vyc29yOnYyOpHOLpRgvg=="
          }
        }
      }
    }
  }
]

[andyfeller]

From what I can gather, I think this is solid and will follow up with @williammartin for final review.

[williammartin]

Getting so close 😅

[williammartin]

Sorry @heaths, one of the issues with long running PRs like this is remembering all the context. When I played around with this just now I anticipated that for REST endpoints --paginate --slurp would be the same as --paginate since the content of REST responses already get slurped into an array. However what happens is we end up doubly nesting the array:

[
  [
    {
      "url": "https://api.github.com/repos/cli/cli/releases/149628351",
...
    }
  ]
]

Maybe my assumption is bad and we always want to apply the same semantics to --slurp even if it produces a strange result - we already know that the whole pagination is kind of quirky. What do you think?

[heaths]

That was how it originally worked, but then someone noted it didn't "solve" their problem of merging nested arrays - only top-level arrays. I still think that's up to business logic - there's no way for us to know which level of arrays to merge - but in the discussion above I agreed to just wrap everything in an array like jq --slurp so that we at least return valid JSON. That's all this PR has ever been about: returning valid JSON such that you can pipe it to something that requires it - not jq, but something like PowerShell's ConvertFrom-Json.

[williammartin]

I remember that discussion as it related to the GQL requests and I agree that we cannot know that and the consumer needs to determine that themselves.

To be clear, here I'm describing the existing merging of JSON array REST responses already getting slurped by --paginate with no further work (as far as I can tell). Below in the screenshot you can see the difference between ./bin/gh api --paginate repos/cli/go-gh/release and ./bin/gh api --paginate --slurp repos/cli/go-gh/release. The --slurp results in a double nesting of arrays with the top level one only having one item.

It's just a bit of a weird situation that the REST pagination ends up creating a single array object that the slurp then wraps in another array. I'm not sure there is a good solution to this though, except perhaps only allowing --slurp on graphql paginated requests? In any case someone is liable to get confused, so maybe we just ship this and move on with our lives.

[heaths]

Yes, that was the compromised we discussed in our meeting with @andyfeller as well: we'd mimic jq --slurp exactly, which also double-nests arrays. This way, the result is consistent: it's always an array. The array items are each page of the response regardless of whether it's an array or object. No guesswork as to how best to handle the output: it's always an array of each page. If you don't need that for a REST response of arrays, don't pass --slurp: you'll already get a merged array because of Nate's previous change based on my old PR.

I'd rather have consistency than conditional output formats myself. Easier to code without having to think of whether I'm making a REST call or GraphQL call.

[williammartin]

Thanks for all your hard work here @heaths, I know it has been a long and painful process.