Custom HttpClient implementation for third part usage with sync/async calls

Question

I'm doing an implementation of HttpClient that is built in a NET Standard project, which will be used as a base to build and process JSON requests/responses for a third part REST API.

Client is built in a simple way, as there are two different address for the API, both used under production environment.

internal class Client : IDisposable
{
    private static HttpClient _client;
    private static Uri _baseAddress;
    private static readonly JsonSerializerSettings _settings = new JsonSerializerSettings
        { DefaultValueHandling = DefaultValueHandling.Ignore, NullValueHandling = NullValueHandling.Ignore, MissingMemberHandling = MissingMemberHandling.Ignore };

    private Client(string baseUrl, Config config)
    {
        _baseAddress = new Uri(baseUrl);

        _client = new HttpClient { Timeout = TimeSpan.FromSeconds(config.Timeout) };

        _client.DefaultRequestHeaders.Accept.Clear();
        _client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
        _client.DefaultRequestHeaders.Add("X-API-KEY", config.Token);
    }

    private static Client _paymentClient;
    private static Client _mainClient;

    public static Client Create(bool payment, Config config = null)
    {
        if (!payment)
        {
            _mainClient = _mainClient ?? new Client("https://api.address.com/", config);
            return _mainClient;
        }

        _paymentClient = _paymentClient ?? new Client("https://payment.address.com/", config);
        return _paymentClient;
    }

    public void Dispose() => _client.Dispose();

    private static async Task<T> Send<T>(HttpMethod method, string url, object data = null)
    {
        var request = new HttpRequestMessage(method, _baseAddress + url);

        if (data != null)
            request.Content = new StringContent(JsonConvert.SerializeObject(data, _settings), Encoding.UTF8,"application/json");

        var response = await _client.SendAsync(request);
            response.EnsureSuccessStatusCode();

        var content = await response.Content.ReadAsStringAsync();

        T result = default;

        try
        {
            if (response.IsSuccessStatusCode)
            {
                if (response.Content.Headers.ContentType.MediaType == "application/json")
                {
                    var responseObj = JsonConvert.DeserializeObject<Response<T>>(content, _settings);

                    if (responseObj.HasError)
                        throw new Safe2PayException(responseObj.ErrorCode, responseObj.Error);

                    result = responseObj.ResponseDetail;
                }
            }
            else throw new Exception($"{(int) response.StatusCode}-{response.StatusCode}");
        }
        catch (Exception)
        {
            throw new Exception();
        }

        request.Dispose();
        response.Dispose();

        return result;
    }

    internal static async Task<T> Get<T>(string url) => await Send<T>(HttpMethod.Get, url);

    internal static async Task<T> Post<T>(string url, object data) => await Send<T>(HttpMethod.Post, url, data);

    internal static async Task<T> Put<T>(string url, object data) => await Send<T>(HttpMethod.Put, url, data);

    internal static async Task<T> Delete<T>(string url) => await Send<T>(HttpMethod.Delete, url);
}

Configuration for API authentication is added on a separated class, called config.

public class Config
{
    public Config(string token, string secret = null, int timeout = 60)
    {
        Token = token;
        Secret = secret;
        Timeout = timeout;
    }

    public string Token { get; }
    public string Secret { get; }
    public int Timeout { get; }
}

This authentication is basically applied on headers, but as per request, it must be allowed for the user to instantiate it on initialization of the desider class, which is built this way:

public class Checkout
{
    private Client Client { get; }

    public Checkout(Config config = null) => Client = Client.Create(true, config);

    public object Credit(Transaction<Credit> transaction)
    {
        var response = Client.Post<Transaction<Credit>>("v2/Payment", transaction).GetAwaiter().GetResult();
        return response;
    }

}

So the user can instantiate the config parameters on initialization of Checkout class.

The usage must be simple as this, so the user uses the return object to process the information...

var checkout = new Checkout(config);
var response = (CreditCard)checkout.Credit(transaction);

Console.WriteLine($"Transaction: {response.Id}");

This would be a simple transaction generation example, which user could cast the class that he made the transaction object to assert and use the information returned. I don't like this cast approach, but I haven't found any other approach that could pass the complete properties of the object to the end user as well, so any tip about that would be really appreciated.

It works as expected and at this first moment, I was requested that it must allow only synchronous usage, that's why so far until method usage is all being made async, but it's being called synchronously to the user's view.

I would really appreciate any other and more experienced view about this. It provides the expected result, but as a beginner I'm not sure if that's the best approach for this kind of usage of the HttpClient.

Answer 1

internal class Client : IDisposable
{
    private static HttpClient _client;
    private static Uri _baseAddress;

Neither of those fields should be static.

---

    private static Client _paymentClient;
    private static Client _mainClient;

    public static Client Create(bool payment, Config config = null)
    {
        if (!payment)
        {
            _mainClient = _mainClient ?? new Client("https://api.address.com/", config);
            return _mainClient;
        }

        _paymentClient = _paymentClient ?? new Client("https://payment.address.com/", config);
        return _paymentClient;
    }

This is weird. At first I responded with an explanation of how to use Lazy<> to create threadsafe singletons, but then I realised that they're actually being constructed with config. But that means that the Client returned might have a different configuration to the one I passed in!

Also, there are potential problems if two threads try to use the same Client.

For both of these reasons, I think you should leave caching instances of Client to the caller.

---

        var request = new HttpRequestMessage(method, _baseAddress + url);

        if (data != null)
            request.Content = new StringContent(JsonConvert.SerializeObject(data, _settings), Encoding.UTF8,"application/json");

        var response = await _client.SendAsync(request);
            response.EnsureSuccessStatusCode();

        var content = await response.Content.ReadAsStringAsync();

        ...

        request.Dispose();
        response.Dispose();

If the method exits early due to an exception, those objects aren't going to be disposed. You should either use using statements or try/finally.

Also, the StringContent objects are also IDisposable.

You might want to add a package reference to Microsoft.CodeQuality.Analyzers and configure CA2000 to be reported as an error. (And CA2213 likewise).

---

        catch (Exception)
        {
            throw new Exception();
        }

This loses all the useful information that the original exception could have given you to help debug. If you have to catch it to log it, rethrow it with just throw; or wrap it and throw it, but don't discard it.