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):
-
There's no longer a concept of a
done
field: the requestDo
methods instead return abool, 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 useddone
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 complicatedif
statement to check iferr != nil || !ok
in many cases. -
dontUnpackErrors
has been replaced with a parallel set ofDo
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. -
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
-
We can now test the API implementation more easily than when it lived in
cmd/src
, since it no longer relies on global state. -
As
Client
andRequest
are interfaces, we can easily mock GraphQL requests in other tests withinsrc-cli
; I expect to add such a helper as I work on https://github.com/sourcegraph/sourcegraph/issues/12333. -
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