Dim/ws fixes

[didimitrie]

Attempts a fix #269 and potentially fixes (or clarifies more) the problem on #250. Defintively fixes the problem on our end!

What's the issue at hand: when connecting to an apollo server subscription endpoint, if an authorization header is present, the server expects it passed in the connection_init event like so:

{"id":"9ba247df77e64de9a68189c46d3c4300","type":"connection_init","payload":{"Authorization":"etc"}}

graphql-client was not passing that along. This PR simply checks in the provided httpClient if there is an Authorization header and, if so, passes it along by adding it to the connection_init request in CreateSubscriptionStream:

initRequest[GraphQLWebSocketRequest.PAYLOAD_KEY] = new Dictionary<string, string>() { { "Authorization", authHeader } };

Relying on a custom http client being passed with custom default request headers as we didn't find out a better way of setting them currently.

Happy to rework if there's a better way of doing this.

---

To clarify re #250: we were getting the same "binary websocket messages are not supported" error, but the actually it was a close event. Changed those if else statements to a switch :)

[rose-a]

Hi, thanks for your contribution.

I'll try to find some time to review this sometime this week...

[didimitrie]

Hi @rose-a! No worries. I'm totally not sure that this is the right approach, or that it actually solves a proper problem, as authentication overall is a mess in the ecosystem, with very vague approaches. The only thing i'm sure about is the last part, re the binary message not supported on close events :)

[rose-a]

I'm totally not sure that this is the right approach, or that it actually solves a proper problem, as authentication overall is a mess in the ecosystem, with very vague approaches. The only thing i'm sure about is the last part, re the binary message not supported on close events :)

I totally agree with you on that 😉 🙈

Regarding the authentication part:

Im worried that other GraphQL servers might vomit on the unexpected payload... I'd really like to not have this as default behaviour, since as you stated above there is no "standard" approach to authorization when using a GraphQL websocket connection.

Maybe we could configure a Func<object> SetSubscriptionInitPayload on GraphQLHttpClientOptions which is executed in place of your modifications. Then we would keep the current default behaviour while enabling you (and others) to inject that authorization header or something else into the connection_init message payload. Although that adds more clutter to GraphQLHttpClientOptions 🤔

[rose-a]

I cherrypicked your "undisputed" changes into V3.1.7.

Please rebase and let us focus on the authentication part in this PR.

[didimitrie]

Okay! I'll try to have a look this weekend, if that's fine! Sounds feasible; I do understand the concerns regarding clutter. If any suggestions pop by in the meantime, let me know - I definitively have less good of a grasp on the client internals than you :)

[ClarenceMG]

Every time that I try to subscribe to a GraphQL API that use Apollo Server I'm getting this message "Object reference not set to an instance of an object"

var client = new GraphQLHttpClient("myapi.com/graphql", new NewtonsoftJsonSerializer());
client.HttpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", myToken);

// My request  already has data and works, I tested it on graphql playground
var subscriptionStream = client.CreateSubscriptionStream<object>(request, (e) =>
{
    Console.WriteLine(e.Message); // I receive the message here
});

var subscription = subscriptionStream.Subscribe((response) =>
{
    if (response?.Data != null)
    {
        Console.WriteLine(response?.Data);
    }
}, (error) =>
{
    Console.WriteLine(string.Format("Exception when subscribe: {0}", error.Message));
}, () =>
{
    Console.WriteLine("Subscription Completed");
});

I think is a problem with authentication.
Did you find a solution?

[ClarenceMG]

@rose-a @didimitrie any idea when this will be released?
I used this changes on my Xamarin iOS and Xamarin Android apps and works fine.

[didimitrie]

I promised I'll cleanup as per @rose-a's suggestions and didn't follow up 😳 I'll try to get to it!

[rose-a]

@didimitrie The missing parts of this PR will end up in #294, so I'm going to close this one... feel free to comment on #294!

[didimitrie]

Thanks! And sorry for not delivering. Nevertheless, great news re #294!