Merge JSON responses from gh api - #8620

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.

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.

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.

@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}}.

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.

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!

--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.

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.

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"! 😉)

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 😬

Me, reading the jq docs.

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.

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.

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.

@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.

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...

@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.

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.