0

I am trying to move logic that I do on all of my controllers into a class to follow the "Do Not Repeat Yourself" principle. What I am struggling with is how to elegantly return an error code.

Here are some examples of what I currently do within each controller:

public class SomethingRequest
{
    public SomethingModel Something { get; set; }


    public string Token { get; set; }
}

public ActionResult GetSomething(SomethingRequest request)
{

    var something = request.Something;

    var token = request.Token;

    if (something == null)
    {        
        return BadRequest("Something object is null. You may have sent data incorrectly");
    }

    if (token == null || token != "1234")
    {
        return Unauthorized("Token object is null");
    }
}

Now what I want to do is move the last two parts of that into their own class:

public class RequestValidation
{

    public void TokenCheck(string token)
    {
        if (token == null || token != "1234")
        {
            // doesn't work
            return Unauthorized("Token object is null");
        }
    }

    public void DataCheck(object someObject)
    {
        if (someObject == null)
        {
            // doesn't work
            return BadRequest("Object is null. You may have sent data incorrectly");
        }
    }       
}

And then I want to call them from SomethingController like so

RequestValidation.TokenCheck(token);

and

RequestValidation.DataCheck(something);

and then have them return the bad request or an exception.

How should I accomplish this?

thalacker
  • 2,389
  • 3
  • 23
  • 44
  • 1
    Camilo has provided an answer for you. Personally it is not something I would generally implement. You are creating complexity and still writing much the same amount of code. The only thing that could improve readability is not using the brackets for these one line if statements, but then if you wish to add anything else inside the if, its a bit of a pain – Kevin Apr 18 '19 at 14:43
  • 1
    You can do this with an [authorization filter](https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/filters?view=aspnetcore-2.2#authorization-filters). That keeps it separate from your controller code. Authorization can change. You might want to use the same controller with different types of authorization. Even the best solution that puts authorization in the controller results in duplicated code and changes to every single controller. – Scott Hannen Apr 18 '19 at 18:53

2 Answers2

4

A common way to do this is to have a helper class that returns the result of validations and/or operations to the Controller:

public class ValidationResult
{
    public bool Succeeded { get; set; }
    public string Message { get; set; }
    public int StatusCode { get; set; }
}

Since the question is tagged with ASP.NET Core, the correct way to do this would be to first create the interface:

public interface IRequestValidationService
{
    ValidationResult ValidateToken(string token);
    ValidationResult ValidateData(object data);
}

Then, create the implementation:

public class RequestValidationService : IRequestValidationService
{
    public ValidationResult ValidateToken(string token)
    {
        if (string.IsNullOrEmpty(token) || token != "1234")
        {
            return new ValidationResult
            {
                Succeeded = false,
                Message = "invalid token",
                StatusCode = 403
            };
        }

        return new ValidationResult { Succeeded = true };
    }

    ...
}

Add it to the DI container (in the Startup class):

services.AddScoped<IRequestValidationService, RequestValidationService>();

Inject it into the SomethingController:

public SomethingController(IRequestValidationService service)
{
    _requestValidationService = service;
}

And finally use it:

public IActionResult GetSomething(SomethingRequest request)
{
    var validationResult = _requestValidationService.ValidateToken(request?.Token);

    if (!validationResult.Succeeded)
    {
        return new StatusCode(validationResult.StatusCode, validationResult.Message);
    }
}

Notice that for something as trivial as validating that something isn't null, you should be using model validation:

public class SomethingRequest
{
    [Required(ErrorMessage = "Something is required, check your data")]
    public SomethingModel Something { get; set; }

    [Required(ErrorMessage = "Token is required!")]
    public string Token { get; set; }
}
Camilo Terevinto
  • 31,141
  • 6
  • 88
  • 120
0

@CamiloTerevinto 's idea got me on the right path. His method would work, but from what I read in the documentation, the proper way is with "Action Filters".

I used this article as additional inspiration.

Here is my filter I named ValidationFilterAttribute

using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Routing;
using System.Diagnostics;
using Microsoft.Extensions.Logging;

namespace Name_Of_Project.ActionFilters
{   
    // This filter can be applied to classes to do the automatic token validation.
    // This filter also handles the model validation.
    // inspiration https://code-maze.com/action-filters-aspnetcore/
    public class ValidationFilterAttribute: IActionFilter
    {
        // passing variables into an action filter https://stackoverflow.com/questions/18209735/how-do-i-pass-variables-to-a-custom-actionfilter-in-asp-net-mvc-app    

        private readonly ILogger<ValidationFilterAttribute> _logger;
        public ValidationFilterAttribute(ILogger<ValidationFilterAttribute> logger)
        {
            _logger = logger;
        }

        public void OnActionExecuting(ActionExecutingContext context)
        {
            //executing before action is called

            // this should only return one object since that is all an API allows. Also, it should send something else it will be a bad request
            var param = context.ActionArguments.SingleOrDefault();
            if (param.Value == null)
            {
                _logger.LogError("Object sent was null. Caught in ValidationFilterAttribute class.");
                context.Result = new BadRequestObjectResult("Object sent is null");
                return;
            }

            // the param should be named request (this is the input of the action in the controller)
            if (param.Key == "request")
            {
                Newtonsoft.Json.Linq.JObject jsonObject = Newtonsoft.Json.Linq.JObject.FromObject(param.Value);

                // case sensitive btw
                string token = jsonObject["Token"].ToString();

                // check that the token is valid
                if (token == null || token != "1234")
                {
                    _logger.LogError("Token object is null or incorrect.");
                    context.Result = new UnauthorizedObjectResult("");
                    return;
                }
            }

            if (!context.ModelState.IsValid)
            {
                context.Result = new BadRequestObjectResult(context.ModelState);
            }
        }


        public void OnActionExecuted(ActionExecutedContext context)
        {
            // executed after action is called
        }
    }
}

Then my Startup.cs

public void ConfigureServices(IServiceCollection services)
{
    services.AddMvc().SetCompatibilityVersion(CompatibilityVersion.Version_2_2);

    // Adding an action Filter
    services.AddScoped<ValidationFilterAttribute>();

}

Then I can add it to my controller.


using Name_Of_Project.ActionFilters;

namespace Name_Of_Project.Controllers
{
    [Route("api/[controller]")]
    [ApiController]
    public class SomethingController : ControllerBase
    {

        // POST api/something
        [HttpGet]
        [ServiceFilter(typeof(ValidationFilterAttribute))]
        public ActionResult GetSomething(SomethingRequest request)
        {
            var something= request.Something;

            var token = request.Token;
    }
}

Because I want to reuse this action filter many times, I need to figure out a way to pass in a parameter for the null check (could have many different objects coming in under the name of "request" that need to check). This is the answer I will be looking to for that portion of the solution.

thalacker
  • 2,389
  • 3
  • 23
  • 44