Error handling in a REST API

Question

I have implemented a small REST API using WebApi 2 and NHibernate for ORM.

When non-existent id is supplied, NHibernate will throw an InvalidOperationException. So I thought I would rethrow a custom exception and then handle all the error logging and returning (there is some logic to different errors/environments) to the client in one place - the ApiExceptionFilterAttribute.

Basically the question is : is this okay or I'm using the custom exception to handle normal flow? How can I improve this?

Example EntityController method:

// GET: api/Entity/5
[ApiExceptionFilter]
public IHttpActionResult Get(int id)
{
 try
 {
 return Ok(
 _mapper.Map<Entity, EntityDTO>(
 _repository.GetById<Entity>(id)));
 }
 catch (InvalidOperationException ex)
 {
 throw new NotFoundException(ex);
 }
}

Repository

public T GetById<T>(int id) where T : Entity
{
 return _session.Value.Query<T>().First(x => x.Id == id);
}

NotFoundException:

public class NotFoundException : ApiException
{
 private static readonly string CustomMessage = "Nothing found! :( Try refining the arguments";
 public NotFoundException(System.Exception ex)
 : base(HttpStatusCode.NotFound, CustomMessage, ex)
 { }
}

And finally, ApiExceptionFilterAttribute

public class ApiExceptionFilterAttribute : ExceptionFilterAttribute
{
 public override void OnException(HttpActionExecutedContext context)
 {
 _logger.Log(context.Exception);
 var exception = context.Exception as ApiException;
 if (exception != null)
 {
 context.Response = context.Request.CreateErrorResponse(exception.StatusCode, exception.Message);
 }
// if not dev or test environment, non-user-friendly exceptions will be returned as generic 500 error
#if !(DEBUG || TEST) 
 else
 {
 context.Response = context.Request.CreateErrorResponse(HttpStatusCode.InternalServerError, "Something went wrong");
 }
#endif
 }
}

• t3chb0t: Is this your real code?
• evictednoise: Yes. Well, except that I swapped the entity classname for Entity
• t3chb0t: ...so where is the _logger defined? The ExceptionFilterAttribute does not provide such field.
• evictednoise: you got me.. in my source its // TODO: _logger.Log(context.Exception)

Answer 1

You're really dragging your Web API through a performance drag by throwing an exception -- especially for something as trivial as a GetById returning a null. You could deal with the non-existence of the id upfront and save yourself the trouble...

public IHttpActionResult Get(int id)
{
 var entity = _repository.GetById<Entity>(id);
 if (entity == null)
 {
 // log here...
 // then I would recommend you return a meaningful statuscode of 4xx instead
 // of the 500 (internal error) you are doing at the moment.
 }
 else
 {
 // do your mapping and return the 200 (OK) status code on a success
 }
}

then change your repository to look like this:

public class Repository<T>
 where T : IEntity
{ 
 public T GetById(int id)
 { 
 return _session.Value.Where(x => x.Id == id).FirstOrDefault();
 }
}

• evictednoise: Sorry, I forgot to provide repository code. Added. The trick is that .First() throws an exception if doesnt find anything
• Svek: @evictednoise - I you should be using FirstOrDefault(). I have provided a sample in my answer.
• Jota.Toledo: I disagree, as the search+if/else section of code will then be duplicated on every part of the src that implements the same logic. IMO the concrete exception type + global filter approach although maybe less performance efficient, its a lot cleaner in terms of code duplication.
• Svek: @Jota.Toledo - In regards to your concern about code duplication, I agree that the code provided does not illustrate a cleaner implementation... Regardless, throwing an exception on conditional logic that does something so easy like GetByIdis not the right solution.
• Svek: @Jota.Toledo --- Maybe you could help illustrate your idea? Because I don't see the gains of repeating a try/catch block and throwing exceptions versus an if (obj == null) {...} else {...} block. The latter just reads easier to me... I guess it is a matter of opinion.