Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transport layer abstraction #5

Open
romshark opened this issue Nov 24, 2017 · 14 comments
Open

Transport layer abstraction #5

romshark opened this issue Nov 24, 2017 · 14 comments

Comments

@romshark
Copy link

romshark commented Nov 24, 2017

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:

  • subscriptions (which ideally need WebSockets for bi-directional communication)
  • request hijacking / proxying
  • internal mocking of the GraphQL endpoint
  • authentication middleware
  • and possibly even more...

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
Copy link
Member

dmitshur commented Nov 24, 2017

For reference, this also came up in the past at shurcooL/githubv4#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
Copy link
Member

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

@romshark
Copy link
Author

romshark commented Nov 24, 2017

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
Copy link

eric-am commented Feb 22, 2018

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
Copy link

eric-am commented Mar 9, 2018

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
Copy link
Member

dmitshur commented Mar 9, 2018

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
Copy link
Member

dmitshur commented Mar 9, 2018

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
Copy link

eric-am commented Mar 16, 2018

Just for the sake of conversation the diff is here (with many things that are not meant for upstreaming, being too opinionated for that: dbmedialab-archive/go-graphql-client@master...pluggable-transport

@NathanZook
Copy link

Note that for testing and logging, as in shurcooL/githubv4#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
Copy link

CreatCodeBuild commented Jan 11, 2019

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
Copy link

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

@brandonbloom
Copy link

brandonbloom commented Jan 14, 2022

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
Copy link

andig commented Sep 29, 2022

Currently looking at the Tibber Pulse api integration for https://evcc.io. Looks like #5 (comment) is the latest/ best suggestion for streaming API integration?

@andig
Copy link

andig commented Nov 20, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants