Should I validate a route value in Controller?

Question

I am building a web app using ASP.NET MVC5. I have logic for validating the route value similar to the following.

[HttpGet]
public ActionResult Books(string name)
{
  if(!CheckBook(name))
  {
    return RedirectToAction(Index);
  }
  return View(model:name);
}

private bool CheckBook(string name)
{
  var exists = false;
  var allBooks = Library.Books; // Library is a static class and Books returns a list with all the book names
  foreach(var book in allBooks)
  {
    if(book == name)
    {
      exists = true;
    }
  }
  return exists;
}

Is it a bad practice to validate the route value in the controller? Should I create a folder such as validators and add validation functions there? Or is this where I create services? I learned that I shouldn't add business logic in controllers, so I am unsure.

You should not (cannot) rely on the client-side code to validate input. Validate at both the client and server. Validating on the client can make for a smoother user experience since you don't need to make an unnecessary request and wait for the fail response. — Jasen, Jan 11 '22 at 23:44
Thank you for your reply. Yes, I have validation on the client-side as well. — Programmer, Jan 11 '22 at 23:46
It can make for cleaner code, allow easier code reuse and testing, if you keep your validation code separate from the controller implementation. — Jasen, Jan 11 '22 at 23:50
How would approach it? Should I create a folder solely for validation? — Programmer, Jan 11 '22 at 23:55
It's true that you cannot rely on client data. But in this case, you're just getting a book by name. What is the risk or "hit" of just letting it run? Is it more than keeping track of a static list of "all the *[current]* book names"? The latter's probably harder than it sounds. — ChiefTwoPencils, Jan 12 '22 at 06:19

score 0 · Answer 1 · answered Jan 12 '22 at 01:37

First off it's OK to put simple Business Logic directly in a controller, as you can test it with DI. Putting the Business Logic in a library with a Domain Specific Language is obviously better.

Let me help you out with a few things:

Because you need to return View(model:name); and the name is based on the Route Value you will need to validate it exists.
Because the Book lookup is related to the Book controller I'd keep the "simple" logic in the controller.

When we talk about Validators, we tend to be talking about parsing form inputs (and validating at both front and back-ends). See this example with a link to a dozen more examples: https://stackoverflow.com/a/51851312/495455

In this case where you need to interrogate routes, headers and query strings you can keep the logic in the controller, or if its called twice or more move it to a library class for polymorphic use.

The Lookup method:

OP:

private bool CheckBook(string name)
{
  var exists = false;
  var allBooks = Library.Books; // Library is a static class and Books returns a list with all the book names
  foreach(var book in allBooks)
  {
    if(book == name)
    {
      exists = true;
    }
  }
  return exists;
}

Better:

private bool CheckBook(string name)
{
  var allBooks = Library.Books;
  foreach(var book in allBooks)
  {
    if(book == name)
    {
      return true; //Don't keep iterating to the end when we know its true!
    }
  }
  return false;
}

Better:

private bool CheckBookExists(string name)
{
  var bookExists = Library.Books.Contains(name);      
  return bookExists;
}

Sure, insanity checks are good but what about a rest endpoint that takes an id but the payload doesn't match the given id? Example: I'm updating a user and my permissions only allow me to update users and roles my role has access to. What if I hack the url to update a user with an id and role outside my access? That's a problem that requires more robust investigation. — ChiefTwoPencils, Jan 12 '22 at 03:46

Should I validate a route value in Controller?

1 Answers1