2
\$\begingroup\$

I have a Web API application in .NET Core 3.1. In a controller I have a GET action that must return a JSON object with a name and a description. In order to retrieve the correct description I need to call an external API (not implemented by myself and outside of my control). This is the code in the controller:

public async Task<ActionResult<Models.Result>> Get(string name)
{
    try
    {
        var objectWrapper = await _clientService.GetObjectAsync(name.ToLowerInvariant());   

        //Other logic here....

        return Ok(new Models.Result { Name = name, Description = objectWrapper.Description} );                
    }          
    catch (Exception ex)
    {
        _logger.LogError($"Failed to get object {name}: {ex}");
        if (ex is ApiCallFailedException apiCallEx)                                   
            return StatusCode((int)apiCallEx.HttpStatusCode);

        return BadRequest($"Failed to get object {name}");
     }
}

The _clientService is the service responsible for the call to the external API:

public async Task<ObjectDto> GetObjectAsync(string name)
{
    var objectWrapper = new ObjectDto();

    var request = new HttpRequestMessage(HttpMethod.Get, $"object/{name}/");
    request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

    using (var response = await _client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead))
    {               
        if (response.IsSuccessStatusCode)
        {
            objectWrapper = await response.Content.ReadAsAsync<ObjectDto>();
        }
        else
        {                    
            throw new ApiCallFailedException(response.StatusCode);
        }
    }

    return objectWrapper;
}

Therefore, I created a custom exception class ApiCallFailedException to handle not successful status codes returned by the external API in order to be able to catch it in my controller action. In the controller action, in caso of not successful status codes from the external API, I decided to return the same status code.

My question is: is this approach correct to handle external API call failure?

Thank you!

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented Jan 8, 2020 at 10:49

1 Answer 1

3
\$\begingroup\$

I have only 2 things to note.

  1. Returning the same status code as the remote server could lead to confused clients of your API due to incorrect status codes. What if the remote server has returns a 5xx status code? Your server didn't have an internal server error, the remote did, but then why is your server returning a 5xx status code?

    Instead, I recommend replacing it with returning a fixed status code (or something depending on the status code the remote API returned). The method name is Get, so I'd go with a 404 Not Found.

  2. Use attributes. In ASP.NET and ASP.NET Core the runtime can figure out which method to call by the http method and the name of it (call Get when an http get request comes, Post when a post comes etc), but it would be a lot more obvious not only for the runtime but also for developers if you named your methods a good name (e.g GetName) and decorate it with attributes specifying the route and the http request method to respond to.

    Plus, you could also do yourself and any future developer a favor by specifying where the parameter to the method comes from. Right now it's part of the URI string (also called query parameters), but other methods might accept JSON to deserialize, you can do this by adding an attribute to the parameter. More about it here.

Putting number 2 into code:

[Route("Result/GetName")]
[HttpGet]
public async Task<ActionResult<Models.Result>> GetName([FromUri] string name) 
{
    // ...
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the tips! In particular, I can consider point 1 as an answer to my doubts and return 404 if something went worng in calling the external API. \$\endgroup\$
    – Sara
    Commented Jan 8, 2020 at 12:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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