3
\$\begingroup\$

Our application architecture consists of:

  • SPA JavaScript calls a set of Web API endpoints
  • The Controllers then call an Entity specific Service
  • Entity specific Service calls a Pass Through Service
  • Pass Through Service calls an API Service that calls external APIs using the HttpClient library via a wrapper.
  • The response is "passed through" to the originating client

Controller:

[RoutePrefix(WebApiConfig.UrlPrefix)]
public class EntityController : ApiController
{
    [Route("entity/next")]
    public async Task<IHttpActionResult> PostPassthru(HttpRequestMessage requestMessage)
    {
        var entityApiService = new EntityApiService>();
        HttpResponseMessage response;
        string pathQuery;

        pathQuery = Common.StripPrefix(WebApiConfig.UrlPrefix, requestMessage.RequestUri.PathAndQuery, true);
        response = await EntityApiService.CallApiAsync(pathQuery, HttpMethod.Post, requestMessage.Content, requestMessage.Headers);

        return ResponseMessage(response);
    }
}

EntityApiService:

public class EntityApiService : IEntityApiService
{
    PassThroughApiService deafultApiService;

    public string API_NAME
    {
        get { return ApiNames.Entity; }
    }

    public EntityApiService()
    {
        deafultApiService = new PassThroughApiService(API_NAME);
    }

    public Task<HttpResponseMessage> GetAsync(string pathAndQuery, HttpRequestHeaders requestHeaders)
    {
        return deafultApiService.GetAsync(pathAndQuery, requestHeaders);
    }


    public async Task<HttpResponseMessage> CallApiAsync(string pathAndQuery, HttpMethod method, HttpContent content, HttpRequestHeaders requestHeaders, bool addJsonMimeRequest = true)
    {
        return await deafultApiService.CallApiAsync(pathAndQuery.ApplyPrefix(ApiPrefix), method, content, requestHeaders, addJsonMimeRequest);
    }
}

PassThroughApiService:

public class PassThroughApiService : IApiService
{
    private string _apiName;

    public string API_NAME { get { return _apiName; } }

    public PassThroughApiService(string apiName)
    {
        this._apiName = apiName;
    }

    public async Task<HttpResponseMessage> CallApiAsync(string pathAndQuery, HttpMethod method, HttpContent content, HttpRequestHeaders requestHeaders, bool addJsonMimeRequest = true)
    {
        HttpResponseMessage response;

        if (content != null && addJsonMimeRequest)
            content.Headers.ContentType = new MediaTypeWithQualityHeaderValue("application/json");

        var apiService = New ApiService();

        response = await apiService.CallApiAsync(API_NAME, pathAndQuery, method, content, requestHeaders, true);


        return response;
    }

    public Task<HttpResponseMessage> GetAsync(string pathAndQuery, HttpRequestHeaders requestHeaders)
    {
        return this.CallApiAsync(pathAndQuery, HttpMethod.Get, null, requestHeaders);
    }
}

ApiService:

public class ApiService : IApiService
{
    // Do not use a USING statement and do not call Dispose, see wrapper comments for details
    private static IHttpClient _httpClient;

    static ApiService()
    {
        _httpClient = new HttpClientWrapper();
    }

    public Uri BuildUri(IApiMap apiMap, string pathAndQuery)
    {
        Uri uri;
        Uri baseUri;

        try
        {
            baseUri = new Uri(apiMap.ApiUri);
        }
        catch (Exception ex)
        {
            throw new Exception("Base URI mapped from API MAP was invalid.", ex);
        }

        try
        {
            uri = new Uri(baseUri, pathAndQuery);
        }
        catch (Exception ex)
        {
            throw new Exception("PathAndQuery value was invalid.", ex);
        }

        return uri;
    }

    public Task<HttpResponseMessage> CallApiAsync(string apiName, string pathAndQuery, HttpMethod method, HttpContent content, HttpRequestHeaders requestHeaders, bool addJsonMimeAccept = true)
    {
        ApiMapService apiMapService = new ApiMapService();
        IApiMap apiMap;
        Uri uri;

        if (String.IsNullOrWhiteSpace(apiName))
            throw new ArgumentNullException("apiName", "ApiName cannot be null or empty.");

        if (String.IsNullOrWhiteSpace(pathAndQuery))
            throw new ArgumentNullException("pathAndQuery", "PathAndQuery cannot be null.");

        if (method == null)
            throw new ArgumentNullException("method", "Method cannot be null.");

        apiMap = apiMapService.GetByName(apiName);

        if (apiMap == null)
            throw new Exception("The ApiName value provided did not resolve to a record.");

        uri = BuildUri(apiMap, pathAndQuery);

        return CallApiAsync(uri, method, content, requestHeaders, addJsonMimeAccept);
    }

    public Task<HttpResponseMessage> CallApiAsync(Uri uri, HttpMethod method, HttpContent content, HttpRequestHeaders requestHeaders, bool addJsonMimeAccept = true)
    {
        HttpRequestMessage request;

        if (uri == null)
            throw new ArgumentNullException("uri");

        if (method == null)
            throw new ArgumentNullException("method");

        request = new HttpRequestMessage(method, uri.AbsoluteUri);

        if (addJsonMimeAccept)
            request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

        if (content != null)
            request.Content = content;

        if (requestHeaders != null)
        {
            foreach (var item in requestHeaders.Where(header => header.Key.Contains(HeaderPrefix)))
            {
                request.Headers.Add(item.Key.Replace(HeaderPrefix, ""), item.Value);
            }
        }

        return _httpClient.SendAsync(request);
    }
}

HttpClientWrapper

public class HttpClientWrapper : IHttpClient
{
    private readonly HttpClient _client;

    public Uri BaseAddress
    {
        get
        {
            return _client.BaseAddress;
        }

        set
        {
            _client.BaseAddress = value;
        }
    }

    public HttpRequestHeaders DefaultRequestHeaders
    {
        get
        {
            return _client.DefaultRequestHeaders;
        }
    }

    public HttpClientWrapper()
    {
        bool ignoreCertificateErrors = ConfigurationManager.AppSettings["IgnoreCertificateErrors"].ToBool();
        string environmentName = ConfigurationManager.AppSettings["EnvironmentName"];

        _client = new HttpClient();

        // If ignoreCertificateErrors config value is true, 
        // the setting ignores all Certificate validation errors in AppDomain, 
        // in every environment but PRODUCTION. 
        if (ignoreCertErrors && environmentName.ToUpper() != "PRODUCTION")
        {
            ServicePointManager.ServerCertificateValidationCallback = delegate { return true; };
        }
    }

    public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request)
    {
        return _client.SendAsync(request);
    }

    private bool disposedValue = false; // To detect redundant calls

    protected virtual void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing && _client != null)
            {
                _client.Dispose();
            }

            disposedValue = true;
        }
    }

    public void Dispose()
    {
        Dispose(true);
    }
}

Any suggestions to improve?

\$\endgroup\$
1
\$\begingroup\$

A small contribution, may I suggest a couple improvements here?

public class EntityApiService : IEntityApiService
{
    PassThroughApiService deafultApiService;

    public string API_NAME
    {
        get { return ApiNames.Entity; }
    }

First, would be to replace PassThroughApiService with the IApiService along with maybe a spelling correction of deafultApiService to defaultApiService or better yet, _apiService.

Second, would be to use Pascal Case for your properties, so that means API_NAME would be ApiName

The improvements applied to the above code block would be:

public class EntityApiService : IEntityApiService
{
    private IApiService _apiService;

    public string ApiName
    {
        get { return ApiNames.Entity; }
    }

I also see that you could make some improvements here:

if (String.IsNullOrWhiteSpace(apiName))
    throw new ArgumentNullException("apiName", "ApiName cannot be null or empty.");

        if (String.IsNullOrWhiteSpace(pathAndQuery))
    throw new ArgumentNullException("pathAndQuery", "PathAndQuery cannot be null.");

        if (method == null)
    throw new ArgumentNullException("method", "Method cannot be null.");

You should use nameof() instead of manually putting in the parameter name, as this will make for easier refactoring and maintainability in the future. In addition, I feel that it's redundant to place the message of repeating "{var} cannot be null." when the Exception type is already obvious.

The improvement would be:

if (String.IsNullOrWhiteSpace(apiName))
    throw new ArgumentNullException(nameof(apiName));
if (String.IsNullOrWhiteSpace(pathAndQuery)) 
    throw new ArgumentNullException(nameof(pathAndQuery));
if (method == null)
    throw new ArgumentNullException(nameof(method));
\$\endgroup\$
  • \$\begingroup\$ I agree with your comments, except for the last one. I originally had ArgumentNullException, but I'm throwing an exception for an empty string and white space as well. But I agree that it is redundant to repeat the same message. What I should do is create my own Exception, something like ArgumentNullOrEmptyException to control the message returned. \$\endgroup\$ – Josh Feb 25 '17 at 14:58
  • \$\begingroup\$ @Josh I admit these aren't the review suggestions you were expecting. --these are mostly "cosmetic" remarks. Hopefully, you'll get a better code review soon. \$\endgroup\$ – Svek Feb 25 '17 at 15:16
  • \$\begingroup\$ @Stek, as you can likely tell, I'm just trying out Code Review. I've used StackOverflow almost since they went live, but I like the idea behind this site. I've put different types of code up to figure out what seems to be better for feedback, small methods or larger set of code. \$\endgroup\$ – Josh Feb 25 '17 at 15:27
1
\$\begingroup\$
    var entityApiService = new EntityApiService>();
    HttpResponseMessage response;
    string pathQuery;

    pathQuery = Common.StripPrefix(WebApiConfig.UrlPrefix, requestMessage.RequestUri.PathAndQuery, true);
    response = await EntityApiService.CallApiAsync(pathQuery, HttpMethod.Post, requestMessage.Content, requestMessage.Headers);

The usage of var or full type name isn't consistand in your code. I suggest picking one and stick to it. You also don't need to declare the variables first and then assign values to them. Why not do everything in one line? This also lacks consistency because the first line does that and the other two not.


Common.StripPrefix

Common is not a good name for a class and it probably contains many unrelated methods. Try to group them in classes that have more concrete names like here the name Url would be great or even UrlHelper but at least something more specific. If I see Common.StripPrefix I think: StripPrefix from what? however Url.StripPrefix clearly suggests what's going on.


PassThroughApiService deafultApiService;

It's a good idea to declare fields like this that do/should not change as readonly.


public class ApiService : IApiService

I'm not fond of this class name because it's virtually the same as the interface which is still very abstract. This means it should either provide a default implementation for an IApiService and be an abstract class or if it is something concrete that its name should reflect this like you did with the EntityController derived from the ApiController where you replaced the API with Entity.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.