shurcooL / graphql

Package graphql provides a GraphQL client implementation.
MIT License
704 stars 280 forks source link

Transport layer abstraction #5

Open romshark opened 6 years ago

romshark commented 6 years ago

To be able to use other transmission protocols, such as WebSockets for example, shurcooL/graphql needs to be abstracted away from the actual transport layer.

This abstraction could also allow:

I suggest to define an interface with a default HTTP client implementation available out of the box and proper documentation for custom implementations.

Code-wise it could look somewhat like this:

func NewClient(transport *TransportInterface, scalars []reflect.Type) *Client

client := graphql.NewClient(graphql.NewHTTPTransport("/query", &http.Client{...}, nil)

dmitshur commented 6 years ago

For reference, this also came up in the past at https://github.com/shurcooL/githubql/issues/2. However, the original author ended up no longer needing the feature, so I deferred thinking about it until a new concrete need would arise (such as this one).

Currently, graphql assumes/hardcodes HTTP as the transport protocol, and your feature request is to abstract out the transport, making it possible to supply other non-HTTP implementations.

According to http://facebook.github.io/graphql/October2016/ and http://graphql.org/learn/serving-over-http/, the GraphQL specification doesn't require the use of HTTP, it just happens to be the most common protocol. But others are valid too.

So, I think this is a valid request. We need to think about what a clean interface for this could look like.

dmitshur commented 6 years ago

Just wondering, what GraphQL server are you using, and does it support non-HTTP transports?

romshark commented 6 years ago

We're currently using neelance/graphql-go ontop of a WebSocket server. Basically a GraphQL server doesn't (or shouldn't) care about the transport layer. It receives a query, returns a result, that's it... how the stuff is transmitted is not its concern

eric-am commented 6 years ago

I'm also starting to pine for some pluggability. I'd like to start doing mocks.

I don't know if this requires replacing HTTP per se, but it might be one of the most direct approaches.

The project I'm working in already has a bunch of wrapper methods for our queries, so I could also just turn those into an interface, and implement a mock one which does some flipflops of json, etc. Having the pluggable part be in the graphql library's transport seems just as clean though, if not cleaner.

Also, having the transport be pluggable might make it easier for me to write a snippet of code to update my own saved mock data by just letting the regular control flow run and saving it. Still thinking about this though.

eric-am commented 6 years ago

I did an experiment with this, and here's what I learned:


It's hard to decide if the Transport interface should use structs...

type Request struct {
    Query     string                 `json:"query"`
    Variables map[string]interface{} `json:"variables,omitempty"`
}

type Response struct {
    Data   json.RawMessage
    Errors errors
}

type Transport interface {
    Do(context.Context, Marshaller, Request) (Response, error)
}

type Marshaller interface { /* ... */ }

... or if it should be the simpler, but less-typed binary pipe:

type Transport interface {
    Do(context.Context, []byte) ([]byte, error)
}

My instinct is to prefer types over bytes, but implementing that seems to imply a need for a marshaller utility function that any transport implementation would call in order to be DRY and consistent.

I also tried the plain bytes way, and it did involve less code... but I also almost immediately ended up writing code that wanted to presume json format of the bytes moments later, when I wrote a another transport implementation called TransportRecorder (which I'll talk about in a sec).

So, I'm still on the fence about which of these makes more sense.


Part 2: YES, using this for testing is really neat. I don't know how I lived before this. It's so convenient, and so useful!

I made TransportHTTP, TransportRecorder, and TransportReplayer types; the first does real network work, the second delegates to another transport, but saves the responses indexed by a hash of the query, and the third replays data saved by the second.

Then, simply adding some flags to my tests that switch between the TransportRecorder and TransportReplayer, I can automatically generate records of the responses of the APIs I'm using, and run tests against them. This makes some of my tests go from seconds to milliseconds... to say nothing of the fact it almost makes them actually reliable in CI, since I'm using fixtures I can commit to git instead of the variability of a live network. Building this into the graphql client means my entire service instantly became offline-testable -- and wow is that nice!


So I'd like to turn this stuff into a PR. But I thought I'd write this up first -- and request some feedback about the interface, and structs versus bytes question, before cleaning my experiment up into a worthy PR.

WDYT?

dmitshur commented 6 years ago

Thanks a lot for experimenting with this and writing up those notes @eric-am, that is very helpful and valuable. I want to take some time to think the proposed API, read up a bit on GraphQL spec to see what they specify, and then I can share my opinions.

We won't be able to merge a PR before making a decision here, so feel free to hold off on that. But you can send it earlier if you think seeing the code can provide additional insights.

dmitshur commented 6 years ago

Part 2: YES, using this for testing is really neat. I don't know how I lived before this. It's so convenient, and so useful!

Would you mind elaborating on how it compares to the current testing method used in this package (i.e., graphql_test.go)?

eric-am commented 6 years ago

Just for the sake of conversation the diff is here (with many things that are not meant for upstreaming, being too opinionated for that: https://github.com/dbmedialab/graphql/compare/master...pluggable-transport

NathanZook commented 5 years ago

Note that for testing and logging, as in https://github.com/shurcooL/githubv4/issues/36, it is not too hard to create a custom http.RoundTripper, and slide it into the http.Client that is fed to NewClient. If this transport layer wraps the default http.Transport by calling req.GetBody(), it is easy to do logging of the body. For testing, you can also insert a mock http.RoundTripper to do whatever you desire. That's a bit less ambitious than swapping out http.Client. But swapping the client may be what is needed for what graphql is doing.

CreatCodeBuild commented 5 years ago

IO agnostic is quite important for me to adapt a library like this. Because I would always prefer to handling IO with my own strategies. Even though I am using HTTP for GraphQL, I would like to fine control the timeout, retries, exponential backoff, and logging. Therefore, I believe it's very valuable for this lib to be IO agnostic, as I consider this the best GraphQL client in Go atm.

raphaelfff commented 3 years ago

Facing the same issue, we ended up creating https://github.com/infiotinc/gqlgenc

brandonbloom commented 2 years ago

I also wanted to use this library in a transport-agnostic way, so I extracted the marshaling/encoding bits to another package: https://github.com/deref/graphql-go/tree/d269e7a529c689bc6fbd40814615844d9a1e47ef#encoding

Thanks to @dmitshur for his excellent work on this original library!

EDIT: Direct link to latest version of my fork: https://github.com/deref/graphql-go

andig commented 2 years ago

Currently looking at the Tibber Pulse api integration for https://evcc.io. Looks like https://github.com/shurcooL/graphql/issues/5#issuecomment-1013489053 is the latest/ best suggestion for streaming API integration?

andig commented 1 year ago

For reference: my best experience regarding web sockets support (i.e. subscriptions) sofar has been https://github.com/hasura/go-graphql-client which is a fork. It would be nice to see something like that as part of this module.