Web API: why IActionResult instead of using exceptions mechanism?

[tkrotoff]

Before:

// Inside ProductsController
[HttpGet("{id}")]
public Product GetProduct(int id)
{
    Product item = repository.Get(id);
    if (item == null) throw new HttpResponseException(HttpStatusCode.NotFound);
    return item;
}

Now:

[HttpGet("{id}")]
public IActionResult GetProduct(int id)
{
    Product item = repository.Get(id);
    if (item == null) NotFound();
    return item;
}

GetProduct signature is not explicit anymore, you have to read its implementation to understand that it returns a Product.

And there is a bigger problem: how do you easily factorize/reuse code? example:

private Product Get(int id)
{
    Product item = repository.Get(id);
    if (item == null) throw new HttpResponseException(HttpStatusCode.NotFound);
    return item;
}

[HttpGet("{id}")]
public IActionResult GetProduct(int id)
{
    return Get(id);
}

private void CheckProductExists(int id)
{
    Product item = repository.Get(id);
    if (item == null) throw new HttpResponseException(HttpStatusCode.NotFound);
}

[HttpPut("{id}")]
public IActionResult Update(int id, [FromBody] Product product)
{
    CheckProductExists(id);
    return Ok(repository.Update(id, product));
}

[HttpDelete("{id}")]
public IActionResult Delete(int id)
{
    CheckProductExists(id);
    repository.Delete(id);
    return NoContent();
}

• The idea is of course not to rely on Web API compat shim
• People seems to simply recreate the old exception way:
  • https://devblog.dymel.pl/2016/06/29/asp-net-core-status-code-empty-response/
  • http://stackoverflow.com/questions/38014379/error-handling-in-asp-net-core-1-0-web-api-sending-ex-message-to-the-client
  • http://stackoverflow.com/questions/29663557/how-do-you-throw-httpresponseexception-in-asp-5-vnext
  • http://stackoverflow.com/questions/31054012/asp-net-5-mvc-6-equivalent-of-httpexception

[Eilon]

We felt that using exceptions for control flow is an anti-pattern (which is generally a commonly-held belief).

But in practice exceptions are problematic for several reasons:

1. While debugging (and first-chance exceptions enabled), they can be very annoying because the debugger keeps breaking on things that aren't "errors".
2. It can make code more difficult to maintain. Exceptions blow right through all control-flow mechanisms aside from catch/finally.
3. The place where they are probably used the most often (from my experience working with customer's apps) is in business logic layers, which many would consider a layering violation. Only the web layer should know about HTTP status codes; business layers should know about business logic.
4. The exception mechanism is quite limited in that it offers only simple HTTP responses; on the other hand action results can do anything.
5. It's kind of an ugly divergence to have two completely different and unrelated ways to accomplish the same thing. You'll find some folks on a project like one, some like the other, and it creates less maintainable applications.

But then back to the non-practical answer: this is really, really, really not what exceptions were designed for.

So, as always, to each their own, but we found that overall the exception-based mechanism was not worth carrying forward.

[frankabbruzzese]

There is no advantage in throwing an exception on a top level module. Exceptions are intended for reaching easily the higher level module that handles errors, thus avoiding the burden of returning any error to the immediate caller.

Business layer top level is supposed to collect all exceptions and translating them into exceptions a controller "may understand", ie exceptions speaking the language of client operations.

Then, controllers are supposed to collect these "User level" exceptions and using them to drive the interaction with the client side, not to throw further exceptions with a standard behavior.

[tkrotoff]

@Eilon @frankabbruzzese

How do you factorize code (inside your controller) without using exceptions?

// class ProductsController

private void CheckProductExists(int id)
{
    Product item = repository.Get(id);
    if (item == null) throw new HttpResponseException(HttpStatusCode.NotFound);
}

[HttpPut("{id}")]
public IActionResult Update(int id, [FromBody] Product product)
{
    CheckProductExists(id);

    return Ok(repository.Update(id, product));
}

[HttpPatch("{id}")]
public IActionResult Patch(int id, [FromBody] Product product)
{
    CheckProductExists(id);

    return Ok(repository.Patch(id, product));
}

[HttpDelete("{id}")]
public IActionResult Delete(int id)
{
    CheckProductExists(id);

    repository.Delete(id);
    return NoContent();
}

I don't want to copy-paste code:

// class ProductsController

[HttpGet("{id}")]
public IActionResult Get(int id)
{
    Product item = repository.Get(id);
    if (item == null) return NotFound();

    return Ok(item);
}

[HttpPut("{id}")]
public IActionResult Update(int id, [FromBody] Product product)
{
    Product item = repository.Get(id);
    if (item == null) return NotFound();

    return Ok(repository.Update(id, product));
}

[HttpPatch("{id}")]
public IActionResult Patch(int id, [FromBody] Product product)
{
    Product item = repository.Get(id);
    if (item == null) return NotFound();

    return Ok(repository.Patch(id, product));
}

[HttpDelete("{id}")]
public IActionResult Delete(int id)
{
    Product item = repository.Get(id);
    if (item == null) return NotFound();

    repository.Delete(id);
    return NoContent();
}

[Eilon]

There are several possibilities, including:

You could return a bool (or enum) indicating the result of the operation:

private bool CheckProductExists(int id)
{
    Product item = repository.Get(id);
    return item == null;
}

And then in the controller convert true/false into an HTTP response.

Or you could have the helper method return an IActionResult:

private IActionResult CheckProductExists(int id)
{
    Product item = repository.Get(id);
    if (item == null)
    {
        return NotFound(...);
    }
    return null;
}

And then the controller code would check the return value, and if it's non-null, return it (or else continue).

[tkrotoff]

@Eilon your suggestion:

private IActionResult CheckProductExists(int id)
{
    Product item = repository.Get(id);
    if (item == null) return NotFound(...);
    return null;
}

[HttpPut("{id}")]
public IActionResult Update(int id, [FromBody] Product product)
{
    var action = CheckProductExists(id);
    if (action != null) return action;

    return Ok(repository.Update(id, product));
}

[HttpPatch("{id}")]
public IActionResult Patch(int id, [FromBody] Product product)
{
    var action = CheckProductExists(id);
    if (action != null) return action;

    return Ok(repository.Patch(id, product));
}

[HttpDelete("{id}")]
public IActionResult Delete(int id)
{
    var action = CheckProductExists(id);
    if (action != null) return action;

    repository.Delete(id);
    return NoContent();
}

[frankabbruzzese]

@tkrotoff, I prefer CheckProductExists return a boolean: more in the logic of what controllers are supposed to do. Here the point is exceptions are intended to handle "exceptiona1 cases" not errors in general! This way one may concentrate on "standard cases" without increasing the complexity of the object messages patterns.

Errors are "exceptional cases" for a "do computations" module but are not "exceptional cases" for an error handling module, since in this case they are just the module "standard job".

Now controllers are not just pipes connecting business layer with the client, but they are responsible for the overall clent-server communication protocol. This means dealing with all possible outcomes coming from the business layer. For a Controller, handling a "record not found" is no way and "exceptional case", on the contrary it is exactly its "standard job".

Thus, inside a controller errors and other "side cases" should be handled with adequate return values, not with exception or by "returning" immediately an answer to the client. The decision of when and what to return to the client should be left to the top method that answered the request and that is in charge for handling the client-server protocol, not to an helper method! In your case: you have a private method that verify if a record exists., so its natural result is a boolean that the caller method may use to drive the interaction with the client. In a few words, returning a boolean makes the helper method more general, and the whole design more modular and maintenable.

In fact, during your system maintenance you might add an action method that handles with "batches" of entities simultaneously with a more complex server-client protocol. In this case you don't need a "NotFound" result but, maybe, putting this information in a more complex answer.

[iscifoni]

@tkrotoff @Eilon Why don't use a filter ? You can define your logic into a filter and use it on the actions that need this check.

This is an example

    public class CheckExists : IAsyncActionFilter
    {
        public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
        {
            var id = context.ActionArguments["id"] as int?;

            if (item.HasValue)
            {
                var item = repository.Get(id);

                if (item != null)
                {
                    context.Result = new NotFoundResult();
                    return;
                }
            }
            await next();
        }
    }

// Inside ProductsController
[HttpGet("{id}")]
[TypeFilter(typeof(CheckExists))]
public Product GetProduct(int id)
{
    Product item = repository.Get(id);
    return item;
}

[HttpPut("{id}")]
[TypeFilter(typeof(CheckExists))]
public IActionResult Update(int id, [FromBody] Product product)
{
    return Ok(repository.Update(id, product));
}

[Eilon]

Filters can certainly do some of this, but if the logic is more complex, filters can be a challenge. For example, if the filter needs to run "in the middle" of some more complex validation process, it's difficult to orchestrate that.

[frankabbruzzese]

Both HttpExceptions, and Filters are attempts to avoid writing each time the repetitive standard code that handles the client-server communication protocol. Exceptions do it by relying on a standard protocol you cannot modify on the controller side, while Filters factor out some logics, but, in general, they cannot factor out the whole protocol since they act just on the start/end of controller processing.

The right way to factor out the whole protocol code is by defining an abstract controller that implements the protocol one might inherit from. Such an abstract controller should contain Generics to adapt to several types, and standard action methods+all standard methods needed to implement the protocol.

The inheriting controller might customize the behavior to adapt it to its business logics by overriding some protected abstract methods exposed to plug-in business logics.

Abstracting the controller protocol in WebApi apps is beneficial also if you don't reuse the code, since you concentrate on the protocol instead of your app, thus writing a better protocol that handles are situations.

[tkrotoff]

Thx for all your answers 👍

[tkrotoff]

In the end, I'm using filters to make sure REST API (action) parameters are passed using [Required], see:

• ModelState.IsValid reports valid model while it is null <= WTF!
• How to validate action parameters with DataAnnotation attributes?
• https://github.com/markvincze/rest-api-helpers

This somewhat lowers the Null References: The Billion Dollar Mistake that C# suffers.

Then if a parameter value is wrong I call ModelState.AddModelError(...) before calling if (!ModelState.IsValid) return BadRequest(ModelState), taking the example from above:

// [ValidateActionParameters]
// class ProductsController

private Product Product_GetWithCheck(int id)
{
    Product item = repository.Get(id);
    if (item == null) ModelState.AddModelError(nameof(id), $"Bad id: '{id}'");
    return item;
}

[HttpGet("{id}")]
public IActionResult Get([Required] int id)
{
    Product item = Product_GetWithCheck(id);
    if (!ModelState.IsValid) return BadRequest(ModelState);

    return Ok(item);
}

[HttpPut("{id}")]
public IActionResult Update([Required] int id, [Required] [FromBody] Product product)
{
    Product item = Product_GetWithCheck(id);
    if (!ModelState.IsValid) return BadRequest(ModelState);

    return Ok(repository.Update(id, product));
}

[HttpPatch("{id}")]
public IActionResult Patch([Required] int id, [Required] [FromBody] Product product)
{
    Product item = Product_GetWithCheck(id);
    if (!ModelState.IsValid) return BadRequest(ModelState);

    return Ok(repository.Patch(id, product));
}

[HttpDelete("{id}")]
public IActionResult Delete([Required] int id)
{
    Product item = Product_GetWithCheck(id);
    if (!ModelState.IsValid) return BadRequest(ModelState);

    repository.Delete(id);
    return NoContent();
}

if (!ModelState.IsValid) return BadRequest(ModelState) should be present in all actions.

ModelState.AddModelError(...) + if (!ModelState.IsValid) return BadRequest(ModelState) return nicely formatted JSON errors.

More real life example:

[ValidateActionParameters]
public class SlotsController : Controller
{
  private User User_FindByIdWithCheck(string userId)
  {
    var user = _usersRepository.FindById(userId);
    if (user == null) ModelState.AddModelError(nameof(userId), $"Couldn't find user '{userId}'");
    return user;
  }

  [HttpGet("{userId}/Slots")]
  public IActionResult Slots([Required] string userId,
                             [Required] DateTime start, [Required] DateTime end)
  {
    var user = User_FindByIdWithCheck(userId);
    if (start > end) ModelState.AddModelError(nameof(start), $"'{start}' should be inferior to '{end}'");
    if (!ModelState.IsValid) return BadRequest(ModelState);

    ...

    return Ok(...);
  }
}

[tkrotoff]

I think this can be closed now

[Korporal]

@Eilon - You wrote:

"We felt that using exceptions for control flow is an anti-pattern (which is generally a commonly-held belief)."

But the OP's examples do not use exceptions for control flow, they simply throw exceptions which is precisely the intent of the throw keyword!

Using exceptions for flow control has nothing to do with throw but with catch and his examples do not do that.

You also wrote:

"It can make code more difficult to maintain. Exceptions blow right through all control-flow mechanisms aside from catch/finally."

Does this mean you are advocating that developers are wrong to ever use throw? that it was a mistake to have included this in the language? if you don't mean that please explain?

The real weakness I see in a huge amount of MVC code is that the patterns used are specific to MVC REST and thus weaken reuse and complicate testing.

A far more helpful pattern is to to do as little as possible in the handlers, after all these are nothing more than HTTP over RPC - that's all that's going on most of the time.

Then all called code can be designed as agnostic (agnostic with respect to how arguments are propagated) with a filter handling and re-throwing as necessary. If the (agnostic) methods throw so be it, that's the designers intent and that is an established design pattern, the fact that we may be calling this over HTTP should be a minor technicality.

The basic fact is (which often gets lost in these discussions) is that REST is nothing more than a particular way of passing arguments over HTTP, the called code ideally should not be cluttered with code that's specific to how arguments have been supplied or over what medium.

[Grauenwolf]

What bothers me about this is that we're not only throwing away .NET's well established design patterns around exceptions, we're also throwing away type safety. Returning an IActionResult is akin to returning an System.Object or Variant.

We loose all of the type data needed by the compiler to ensure you are returning the correct thing. And needed by IDL generators such as RAML and Swagger to produce accurate response definitions. Which in turn are needed for client code generation.

[Grauenwolf]

But then back to the non-practical answer: this is really, really, really not what exceptions were designed for.

Huh? Indicating an error condition is EXACTLY what exceptions are designed for.

[khellang]

Returning an IActionResult is akin to returning an System.Object or Variant.

Then don't do it? MVC lets you return whatever you want. Just change the return type. Easy.

[khellang]

Huh? Indicating an error condition is EXACTLY what exceptions are designed for.

If you really want to throw exceptions and have them converted into responses, it's really easy to write a filter for it. Instead of complaining about it here, (in several issues now) you'd be done by now 😉

[Grauenwolf]

Yea, I could redo that for every project. Or it could be baked in as a standard feature like it should have been in the first place.

[jeffrey-igaw]

IActionResult design is not bad, more explicit and intuitive than throw exception pattern. In my experience it is not very common for developers to write code throws exceptions (exception is more defensive but causing duplicate exception handling and many developer think of a good case flow, just compromise)

But! IActionResult is not type safe in test code(Almost casting is required) and HTTP API documentation as Swagger (return type inference is impossible). I think IActionResult<ReturnType> is designed better than IActionResult. (however, this not allow to specify the type for other return statuses.)

Ideally, throwing an exception is best, but in a real project, it can also be thrown at the service or repository layer. (of course, it's possible to solve problems with conventions or with lint :joy:)

Finally, I really want to provide exception classes and handling filters (by configuration). In the next release.