api: refactor the low level cmd/src GraphQL functionality into a new api package

Created by: LawnGnome

This PR moves the apiRequest type and its associated functions in cmd/src out into a new internal/api package.

Why?

I'm working on https://github.com/sourcegraph/sourcegraph/issues/12333, and want to keep as much of the business logic of interacting with the campaign backend out of cmd/src as possible: ideally, I want cmd/src to be the "view" layer — handling flags and managing output — and have internal/campaigns be the service layer that implements all the actual logic. I believe this separation will allow for better testing and code quality. (And, if I'm honest, this is also partly because I was starting to have to be very careful about prefixing identifiers in cmd/src, and that was making for seriously unwieldy type and variable names.)

In order to do this, internal/campaigns has to be able to issue GraphQL requests. Just making apiRequest and friends public doesn't totally address this: it sets up potential circular dependencies, and apiRequest is tied to global state via its use of cfg, which makes it hard to reason about, and to mock for future testing purposes as I work on https://github.com/sourcegraph/sourcegraph/issues/12333.

What's changed

I haven't significantly changed the internals of issuing GraphQL requests: the code in api.Request's methods will look extremely familiar to anyone who's looked at apiRequest.do() previously.

There are a handful of specific changes that I think are improvements, but are open to discussion (just like everything else, of course):

1. There's no longer a concept of a done field: the request Do methods instead return a bool, error tuple that indicate whether the result was filled out (with the case where it's not being when -get-curl is enabled). This means that functions that make GraphQL requests that previously used done are now written in a more top-down, procedural way, which I think is easier to read, but at the expense of a slightly more complicated if statement to check if err != nil || !ok in many cases.

2. dontUnpackErrors has been replaced with a parallel set of Do methods: one does the normal default unwrapping of the GraphQL response, and one doesn't. I think this is clearer, but this is certainly a matter of taste.

3. Contexts are now propagated consistently through the API package, although I haven't done any work to really take advantage of that thus far.

What does this allow in the future

1. We can now test the API implementation more easily than when it lived in cmd/src, since it no longer relies on global state.

2. As Client and Request are interfaces, we can easily mock GraphQL requests in other tests within src-cli; I expect to add such a helper as I work on https://github.com/sourcegraph/sourcegraph/issues/12333.

3. This gives us some options for figuring out what context behaviour we think should be the default when executing src commands: we could now start looking at providing consistent options for timeouts and signal handling.

cc: @chrispine

Created by: mrnugget

Review: Approved

Nice! Thanks for doing this.

Disclaimer: I haven't checked every line here. But I like the overall structure, especially since I could now, theoretically, import this in my test scripts to talk to the API

Created by: efritz

This is a big change - seems good at a glance but probably won't be able to do an in-depth review. I'm good with this as long as the lsif-upload command still works :p

Created by: efritz

Review: Approved

Created by: LawnGnome

I tested our array of commands before I opened the PR, but I did an extra bonus check of the lsif upload functionality just now to be totally sure.

Merged by: LawnGnome at 2020-08-07 22:08:24 UTC