Khan / genqlient

a truly type-safe Go GraphQL client
MIT License
1.02k stars 99 forks source link

Add an option to add custom Header when submitting via MakeRequest() #302

Closed LeonZh0u closed 4 months ago

LeonZh0u commented 5 months ago

Is your feature request related to a problem? Please describe. Currently, users rely on roundtripper in http client to add custom headers at client initialization. However, this becomes an issue when we need to use a different bearer token for each of the query.

Describe the solution you'd like

type Request struct {
    // The literal string representing the GraphQL query, e.g.
    // `query myQuery { myField }`.
    Query string `json:"query"`
    // A JSON-marshalable value containing the variables to be sent
    // along with the query, or nil if there are none.
    Variables interface{} `json:"variables,omitempty"`
    // The GraphQL operation name. The server typically doesn't
    // require this unless there are multiple queries in the
    // document, but genqlient sets it unconditionally anyway.
    OpName string `json:"operationName"`
        //ADDED FIELD to allow custom header
        Headers map[string]string `json:"requestHeaders,omitempty"`
}

add header from req.headers into the returned http.request in this function https://github.com/Khan/genqlient/blob/28aafc79a9f9065f7cb7e8bb2df49e5795896aeb/graphql/client.go#L197

Describe alternatives you've considered https://github.com/Khan/genqlient/issues/249 This approach doesn't support a different token for each of the request.

Additional context Add any other context or screenshots about the feature request here.

benjaminjkraft commented 5 months ago

You can totally do different tokens per request with the approach in the docs! The question is just how you know what token to use for each request. The standard way is from req.Context(). If I remember correctly we did exactly this at @Khan to plumb a bunch of headers around.

If that doesn't work for you, could you say more about how this approach would improve the situation? The graphql.Request is only accessible once you're in the graphql.Client.MakeRequest method anyway, so I think you have to get your header from context anyway.

(You can also probably construct the client dynamically for each request, if you prefer. That may have some drawbacks in terms of code structure, depending how you like to pass clients around, but I doubt the perf cost is significant as long as the underlying http.Client (or at least its innermost Transport) is shared.)

benjaminjkraft commented 4 months ago

Closing -- happy to reopen if you're able to provide more details!

zdunecki commented 4 months ago

Hi,

You can do something like this:

import (
    "context"
    "errors"
    "github.com/Khan/genqlient/graphql"
    "net/http"
)

type authedTransport struct {
    wrapped http.RoundTripper
}

func (t *authedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
    v := req.Context().Value("token") // get token from request context since you always pass context to generated graphql queries
    token, ok := v.(string)
    if !ok {
        return nil, errors.New("no token in context")
    }

    req.Header.Set("Authorization", "Bearer "+token)

    return t.wrapped.RoundTrip(req)
}

// you can create client refernce just once and use for every request
func NewClient() graphql.Client {
    return graphql.NewClient(
        "http://localhost:3000/graphql",
        &http.Client{Transport: &authedTransport{wrapped: http.DefaultTransport}},
    )
}

type Handler func(writer http.ResponseWriter, request *http.Request, next http.HandlerFunc)

// use this middleware in your routing library
func WithAuth() Handler {
    return func(w http.ResponseWriter, r *http.Request, n http.HandlerFunc) {
        ctx := context.WithValue(r.Context(), "token", "EXAMPLE_TOKEN")

        r = r.WithContext(ctx)
        n(w, r)
    }
}

Best

sylnsr commented 3 months ago

That's more code than just adding the feature, and its a filthy hack. I think I've just been convinced to use a different library.

benjaminjkraft commented 3 months ago

Hi @sylnsr, let's try to keep the discussion polite.

If you'd like to propose a clean way to add this to genqlient, it's something we are open to, but there are a lot of different things that "add a custom header" can mean to different people, so you'll need to get specific about what it is you're looking for and how you see it could be simpler without adding a ton of API surface that we need to maintain forever. The proposal in the original issue, as I describe in my prior comment, doesn't actually help as far as I understand!