Andy

Posted on Jul 15, 2023

10 Bad Practices to Avoid in ASP.NET Core API Controllers

#dotnet #dotnetcore #csharp #aspnet

When developing controllers in ASP.NET Core, there are certain practices that should be avoided to ensure maintainability, performance, and adherence to best practices. Here are 10 things we should avoid in our controllers.

1. Tight coupling

Avoid tightly coupling in controllers with specific dependencies or frameworks. Instead, use dependency injection to inject dependencies into controllers. This promotes loose coupling and makes the code more testable and maintainable.

// Avoid:
[ApiController]
[Route("[controller]")]
public class ProductController : ControllerBase
{
    ProductService productService = new ProductService();
    // ...
}


// Prefer:
[ApiController]
[Route("[controller]")]
public class ProductController : ControllerBase
{
    private readonly ILogger<ProductController> _logger;
    private readonly IProductService _productService;

    public ProductController(IProductService productService, ILogger<ProductController> logger)
    {
        _logger = logger;
        _productService = productService;
    }
    // ...
}

Learn more about dependency injection

2. Mixing concerns

Controllers should focus on handling HTTP requests and generating responses. Avoid mixing concerns such as data access, authentication, or authorization directly in the controller. Delegate these responsibilities to separate classes or middleware.

// Avoid:
[HttpPost]
public async Task<IActionResult> CreateProduct(ProductDto productDto)
{
    // Authentication and authorization logic here
    // ...
    // Data access logic here
    // ...
    return Ok(StatusCodes.Status201Created);
}


// Prefer:
[HttpPost]
[Authorize]
public async Task<IActionResult> CreateProduct(ProductDto productDto)
{
    await _productService.CreateAsync(productDto);

    return Ok(StatusCodes.Status201Created);
}

Learn more about the Single-responsibility principle | Separation of concerns

3. Lack of exception handling

Controllers should handle exceptions gracefully and return appropriate error responses. Avoid letting exceptions propagate to the client without proper handling. Implement global exception-handling mechanisms, such as exception filters or middleware, to centralize error handling.

// Avoid: (Inconsistent error handling or try catch everywhere)
[HttpPost]
public async Task<IActionResult> CreateProduct(ProductDto productDto)
{
    try
    {
        await _productService.CreateAsync(productDto);

        return Ok(StatusCodes.Status201Created);
    }
    catch (ProductValidationException ex)
    {
        return BadRequest(ex.Message);
    }
    catch (Exception ex)
    {
        _logger.LogError(ex, "An error occurred while creating the product.");
        return StatusCode(StatusCodes.Status500InternalServerError);
    }
}


// Prefer: Exception filters or Middleware
[HttpPost]
public async Task<IActionResult> CreateProduct(ProductDto productDto)
{
    await _productService.CreateAsync(productDto);

    return Ok(StatusCodes.Status201Created);
}

E.g of a global exception middleware

public class ExceptionMiddleware
{
    private readonly RequestDelegate _next;

    public ExceptionMiddleware(RequestDelegate next)
    {
        _next = next;
    }

    public async Task InvokeAsync(HttpContext context)
    {
        try
        {
            await _next(context);
        }
        catch (Exception ex)
        {
            // Log the error
            // ...

            await HandleExceptionAsync(context, ex);
        }
    }

    private Task HandleExceptionAsync(HttpContext context, Exception exception)
    {
        context.Response.ContentType = "application/json";
        context.Response.StatusCode = (int)HttpStatusCode.InternalServerError;

        var response = new
        {
            StatusCode = context.Response.StatusCode,
            Message = "Internal Server Error"
        };

        return context.Response.WriteAsync(JsonConvert.SerializeObject(response));
    }
}

With this exception middleware, any unhandled exceptions that occur during the processing of an HTTP request will be caught, and a JSON response with a generic error message and the status code 500 (Internal Server Error) will be returned to the client.

We can customize the HandleExceptionAsync method to include more detailed error information or modify the response structure as per our requirements.

To use this middleware in our ASP.NET Core Web API application, we can add the following code to our Startup.cs file or Program.cs .net 6.

app.UseMiddleware<ExceptionMiddleware>();

Learn more about Exception filters | Middleware

4. Long-running operations

Avoid performing long-running operations synchronously in controllers. Offload such tasks to background workers or use asynchronous programming.

// Avoid:
[HttpGet]
public IActionResult GenerateReport()
{
    // Long-running operation
    // ...
    return Ok(report);
}


// Prefer:
[HttpGet]
public async Task<IActionResult> GenerateReport()
{
    // Offload the long-running operation to a background worker
    await _reportGenerationService.GenerateAsync();
    return Ok();
}

Learn more about Background tasks | Hangfire scheduler | Task asynchronous

5. Lack of validation

Input validation is crucial to ensure the integrity and security of the application. Avoid neglecting input validation in your controllers. Leverage model validation attributes, such as [Required], [MaxLength], and custom validation logic to validate incoming data.

// Avoid:
[HttpPost]
public async Task<IActionResult> CreateProduct(ProductDto productDto)
{
    // No validation
    await _productService.CreateAsync(productDto);
    return Ok(StatusCodes.Status201Created);
}


// Prefer:
[HttpPost]
public async Task<IActionResult> CreateProduct([FromBody] ProductDto productDto)
{
    if (!ModelState.IsValid)
    {
        return StatusCode(StatusCodes.Status400BadRequest, ModelState);                
    }

    await _productService.CreateAsync(productDto);
    return Ok(StatusCodes.Status201Created);
}

Learn more about Model Validation | FluentValidation

6. Direct database access

Avoid directly accessing the database from the controllers. Instead, use an abstraction layer, such as repositories or data access services, to decouple the controllers from specific data access technologies. This improves testability and maintainability.

// Avoid:
[HttpGet]
public IActionResult GetProduct(int productId)
{
    var product = dbContext.Products.Find(productId);
    return Ok(product);
}


// Prefer:
[HttpGet]
public async Task<IActionResult> GetProduct(int productId)
{
    var product = await _productService.GetByIdAsync(productId);
    return Ok(product);
}

Learn more about Repository Pattern or Data Layer

7. Lack of caching

Avoid not utilizing caching mechanisms when appropriate. Leverage caching to improve performance and reduce load on the server.

// Avoid:
[HttpGet]
public async Task<IActionResult> GetProducts()
{
    var products = await _productService.GetAllAsync();
    return Ok(products);
}


// Prefer:
[HttpGet]
[ResponseCache(Duration = 60)] // Cache the response for 60 seconds
public async Task<IActionResult> GetProducts()
{
    var products = await _productService.GetAllAsync();
    return Ok(products);
}

Learn more about Caching | Response caching

8. Lack of authentication and authorization

Avoid not implementing authentication and authorization for sensitive operations. Secure the controllers accordingly.

// Avoid:
[HttpPost]
public async Task<IActionResult> DeleteProduct(int productId)
{
    // No authentication or authorization
    await _productService.DeleteAsync(productId);
    return StatusCode(StatusCodes.Status200OK);
}


// Prefer:
[HttpPost]
[Authorize(Roles = "Admin")]
public async Task<IActionResult> DeleteProduct(int productId)
{
    await _productService.DeleteAsync(productId);
    return StatusCode(StatusCodes.Status200OK);
}

Learn more about Authentication and Authorization

9. Excessive logic

Controllers should primarily be responsible for handling incoming requests and returning responses. They should not contain complex business logic. Instead, delegate the business logic to dedicated service classes or other components.

// Avoid:
[HttpGet]
public async Task<IActionResult> GetProducts()
{
    // Complex business logic here
    // ...
    // ...
    // ...

    return Ok(products);
}


// Prefer:
[HttpGet]
public async Task<IActionResult> GetProducts()
{
    var products = await _productService.GetAllAsync();
    return Ok(products);
}

Learn more about Common web application architectures

10. Ignoring HTTP verbs and RESTful principles

Controllers in ASP.NET Core should adhere to the principles of RESTful architecture. Avoid using improper HTTP verbs or actions that don’t align with RESTful conventions. Use appropriate HTTP verbs (GET, POST, PUT, DELETE, etc.) to perform corresponding operations on resources.

// Avoid:
public IActionResult DeleteProduct(int id)
{
    // ...
}


// Prefer:
[HttpDelete("/api/products/{id}")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize]
public IActionResult DeleteProduct(int id)
{
    // ...
}

Learn more about RESTful web API design

By avoiding these common pitfalls, we can develop controllers in ASP.NET Core that are maintainable, testable, and follow best practices for API web development.

Thanks for reading!

Through my articles, I share Tips & Experiences on web development, career, and the latest tech trends. Join me as we explore these exciting topics together. Let’s learn, grow, and create together!

➕More article about Programming, Careers, and Tech Trends.

🔔 Follow me on Medium | Twitter

Top comments (11)

Vıɔk A Hıƃnıʇɐ C. • Jul 16 '23

Hey bro, what do you think about this proposal for first point?

[ApiController]
[Route("[controller]")]
public class ProductController : ControllerBase
{
    private readonly ILogger<ProductController> _logger;
    private readonly IServiceProvider _serviceProvider;

    public ProductController( IServiceProvider serviceProvider )
    {
      this._serviceProvider = serviceProvider;
      this._logger = serviceProvider.GetService<ILogger<ProductController>>();
    }

    [HttpPost]
    public async Task<IActionResult> CreateProduct([FromBody] ProductDto productDto)
    {
        // This validation must be a responsibility of the Service for create.
        if (!ModelState.IsValid)
        {
            return StatusCode(StatusCodes.Status400BadRequest, ModelState);                
        }

        await this._serviceProvider.GetService<IProductService>().CreateAsync(productDto);
        // Question: how return a different HttpStatusCode?
        return Ok(StatusCodes.Status201Created);
    }
   // ...
}

Why?
Because, Imagine that your product controller has 5 or more methods implemented, and that each of them requires services such as dependencies...
Then, you will be injecting to the controller constructor an unnecessary amount of service instances that are not going to be used, which increases the memory consumption unnecessarily.
For a small workload you may not notice the difference, but for high workloads, your service will scale faster unnecessarily, so it is better to use the IServiceProvider to resolve the instances in the context where they will be used.
In effect, you indirectly increase the response speed of your services by not having to resolve and inject unnecessary instances.

Leave your appreciations.
Good vives!!

Andy • Jul 16 '23

Hi, thanks for you comment,
Really good point.
I personally like to inject the specific interfaces instead of IServiceProvider for small workloads.

But at the end of the day it depends on the problem we are trying to solve, complexity and the choose architecture.

About your point "product controller has 5 or more methods implemented, and that each of them requires services such as dependencies...
Then, you will be injecting to the controller constructor an unnecessary amount of service instances that are not going to be used, which increases the memory consumption unnecessarily."

For high workloads or scalable services, in 90% of the case I will think about using CQRS, Command/Query separation so our controllers delegate the responsibility to commands or queries to do the actual work (another component)

Just an example...

[ApiController]
[Route("api/products")]
public class ProductController : ControllerBase
{
    private readonly IMediator _mediator;

    public ProductController(IMediator mediator)
    {
        _mediator = mediator;
    }

    [HttpPost]
    public async Task<IActionResult> CreateProduct(CreateProductCommand command)
    {
        await _mediator.Send(command);
        return Ok(StatusCodes.Status201Created);
    }

    [HttpGet]
    public async Task<IActionResult> GetAllProducts()
    {            
        var products = await _mediator.Send(new GetAllProductsQuery());
        return Ok(products);
    }
}

Apreciate your feadback

Vıɔk A Hıƃnıʇɐ C. • Jul 16 '23 • Edited

Andy, that is a good way and very common strategy in DotNet environment, so, if your mediator is something like the next, so, the unnecessary service instances problem, will not be presented, example:

public class Mediator : IMediator
{
    private readonly IServiceProvider _serviceProvider;

    public Mediator(IServiceProvider serviceProvider)
    {
        _serviceProvider = serviceProvider;
    }

    public Task<TResponse> Send<TResponse>(IRequest<TResponse> request)
    {
        var handlerType = typeof(IRequestHandler<,>).MakeGenericType(request.GetType(), typeof(TResponse));
        dynamic handler = _serviceProvider.GetService(handlerType);
        return handler.Handle((dynamic)request);
    }
}

But is your mediator receive for constructor a lot of Types, it can be a future problem in high workloads.

Personal appreciation:
Lately I am avoiding using Mediator pattern to handle the requests inside the controller, instead, I'm using directly the use case, this makes it simple and I should avoid extra code delegating all the application and domain responsibility to the use case, for example:

public class ProductController : ControllerBase
{
    private readonly IServiceProvider _serviceProvider;

    public ProductController(IServiceProvider serviceProvider)
    {
        this._serviceProvider = serviceProvider;
    }

    [HttpPost]
    public async Task<IActionResult> CreateProduct(CreateProductRequest request)
    {
        return this.HandleActionResult( await this._serviceProvider.GetService<CreateProductUseCase>().Execute( request, this.GetUseCaseTrace() ) );
    }

    [HttpPut("{id}")]
    public async Task<IActionResult> UpdateProduct(int id, UpdateProductRequest request)
    {
        return this.HandleActionResult( await this._serviceProvider.GetService<UpdateProductUseCase>().Execute( id, request, this.GetUseCaseTrace() ) );
    }

    [HttpDelete("{id}")]
    public async Task<IActionResult> DeleteProduct(int id)
    {
        return this.HandleActionResult( await this._serviceProvider.GetService<DeleteProductUseCase>().Execute( id, this.GetUseCaseTrace() ) );
    }
}

Let me know your appreciations, they will be of value for me!! :)

José Pablo Ramírez Vargas • Jul 16 '23

By injecting IServiceProvider you are falling into the Service Provider anit-pattern. This is particularly palpable when you examine the constructor signature that you have no idea what actual services are needed for the class to operate.

This is particularly troublesome for unit testing. What services should I mock to test the class? All I see is IServiceProvider. This is generally a bad idea.

The best approach would be to try to separate the concerns as much as possible from the get-go and to try to implement singleton services for the ones that may consume more RAM.

Vıɔk A Hıƃnıʇɐ C. • Jul 16 '23

José on that point you are very right, however if we have had good practices in development no matter if your architecture is MVC, Clean, Onion, N-Layers, among others, and within those good practices was the fact of not having added logic in your controller, then I assure you that you will not have any problem, because your unit tests would be focused around your Services, UseCases, Commands and Queries units, and you would not have to worry about your controller because you do not have to test anything in it, since it is simply a primary connector between your server and your application layer.

Let me know your appreciation after this.
Good vives!!

José Pablo Ramírez Vargas • Jul 17 '23

I still disagree. For example, one can easily write a unit test that makes sure all controllers can be instantiated by using a little reflection. If the controllers' only requirement is IServiceProvider, then the unit test achieves nothing.

It is best, as you were told by the author, to section the controller into smaller controllers. One way of doing this, as you were told, is to separate the CRUD operations into different controllers.

I don't think you can pose a scenario where I (or others) could not offer a better counter approach.

Vıɔk A Hıƃnıʇɐ C. • Jul 17 '23 • Edited

José, your point is respectable, however I do not see the need to do a unit test on a controller that should not have any logic beyond being a bridge between the Infra layer and the application layer.

I don't understand what value adds a test of those that you mention since the API is going to load everything that extends from BaseController, but respectable.

I withdraw from the crossing of ideas (debate).
¡Good week!

José Pablo Ramírez Vargas • Jul 16 '23

Number 4 is not really a background worker. By using await you merely release the threadpool thread. That's it. A background worker is a completely different thing, usually implemented as a class that derives from BackgroundService.

What is correct is the general statement: Long-running operations should be offloaded to a background worker. The correct way of doing it would be to accept the data, queue the work and return HTTP 202 ACCEPTED along with a transaction identifier for the user to check on the status/progress of the operation.

Number 5 collides with number 2: The fact that Microsoft has provided ModelState.IsValid, doesn't mean that it must be used. I actually recommend avoiding it altogether because it violates number 2: You are providing business validation logic in the controller, and the controller's only concern should be to translate data from and to the HTTP protocol. A simple test that demonstrates the violation: Imagine that you are asked to create a command-line tool that runs the business code. If you remove the HTTP layer, you lose the validation logic, which should not happen at all.

All in all, thanks for sharing. I wish my co-workers had followed a guide like this 3 years ago. That way I wouldn't have to deal with so many bad things at work.