DEV Community

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

Andy on July 15, 2023

When developing controllers in ASP.NET Core, there are certain practices that should be avoided to ensure maintainability, performance, and adheren...

Read full post

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.