byme8 / ZeroQL

C# GraphQL client with Linq-like syntax
MIT License
273 stars 13 forks source link

decouple GraphQL client from HttpClient dependency #61

Closed jenschude closed 1 year ago

jenschude commented 1 year ago

I try to integrate the ZeroQL into an existing library with a preconfigured client which wraps itself a HttpClient instance.

As ZeroQL is just using the HttpClient to post the query and retrieve it's result it may be useful to decouple the GraphQL client by introducing a HttpHandler interface. For backwards compatibility I added a default implementation which expects a HttpClient instance.

Btw. it may not be the best idea to dispose the HttpClient when it's not instantiated using the HttpClientFactory. This could lead to Socket starvation when creating a lot of client.

byme8 commented 1 year ago

In the current implementation, it looks weird. You have an interface that holds the HttpClient, but the public API depends on HttpRequest and HttpResponse. I have not seen a library that may use such primitives and doesn't require the HttpClient. So, what is the point of making a leaky abstraction from the beginning?

Can you tell more about your configuration maybe there will be a better solution?

into an existing library with a preconfigured client which wraps itself a HttpClient instance

Do you have control over this preconfigured client creation?

If you have, you can expose the HttpClientalongside it. It can be just an additional property or instance inside a DI container.

If you don't, then the HttpClient already has the HttpClientHandler abstraction.

public static async Task<IGraphQLResult> Execute()
  {
      ISpecialClientHandler specialClient = // .... <= this is your special client
      var specialHttpHandler = new SpecialClientHandler(specialClient);

      var httpClient = new HttpClient(specialHttpHandler)
      {
          BaseAddress = new Uri("http://localhost:10000/graphql")
      };

      var qlClient = new TestServerClient(httpClient);
      // place to replace
      var response = await qlClient.Query(static q => q.Me(o => o.FirstName));

      return response;
  }

public class SpecialClientHandler : HttpClientHandler
{
    private readonly ISpecialClientHandler client;

    public HotChocoClientHandler(ISpecialClientHandler client)
    {
        this.client = client;
    }

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage httpRequest, CancellationToken cancellationToken)
    {
        return await client.SendAnsync(httpRequest, cancellationToken);
    }
}

it may not be the best idea to dispose the HttpClient when it's not instantiated using the HttpClientFactory.

Yeap, you are right. I thought that there was no IDisposable interface on the ZeroQL client. I added the interface and forgot about that. It would be better to remove it and leave dependency lifecycle management outside the client.

jenschude commented 1 year ago

You are right in the way, that I would have to add a leaky abstraction returning HttpResponse.

We configure the abstraction to do the Token retrieval, caching etc for OAuth. Also error handling, logging. And none the less serialization/deserialization of the API requests/response. One of the endpoints is a GraphQL one.

The thing with the HttpClient is, that exposes Send and SendAsync methods they are even implemented independently also in ZeroQL you are only using PostAsync till now. I mean it could also be reduced to:

public Task<String> SendAsync(String query)

So the handler would have to pack it in a Request and extract the response string. So the pipelines would become more dumber and don't need knowledge about the HTTP abstraction of the dotnet framework anymore

byme8 commented 1 year ago

I pushed 5.0.0-preview.7. Basically, it is your pull request + my adjustments. You can have a look at full diff here

jenschude commented 1 year ago

Btw. I now remember why wrapping a special handler inside a HttpClient is not a good idea.

The HttpRequest is stateful. Once you send a request using the HttpClient it's in state Started. You can't put this Request any more inside another HttpClient to send it and you would have to create a new HttpRequest instance inside the SpecialHandler