Find the issue: parameter types

Can you find the issue in this ASP.NET Web API code snippet?

public sealed class MyCommandController : ApiController
{
    public string Post(string useTheForce, string timeout, string auditMessage)
    {
        Process myCommand = Process.Start(
            "MyCommand.exe",
            $"-useTheForce {useTheForce} -timeout {timeout} -auditMessage {auditMessage}");
        myCommand.WaitForExit();
        return $"Completed with exit code {myCommand.ExitCode}";
    }
}

I admit that this is sort of a trick question because there is certainly more than one issue. But let’s start today with the Post() method signature.

// . . .
public string Post(
    string useTheForce,
    string timeout,
    string auditMessage)
// . . .

Judging by their use as inputs to MyCommand.exe, the parameters useTheForce and timeout expect a Boolean and an integer value, respectively. Yet, they are passed as strings. We have a classic case of stringly-typed code!

Luckily parameter binding in Web API is very easy for simple data types. So we can fix this by merely changing the parameter types:

public string Post(
    bool useTheForce,
    int timeout,
    string auditMessage)
// . . .

Not only is the intent of the code clearer but we also get basic parameter validation for free. For example, if you try to pass some value like “Yes” for useTheForce you get this error:

{
  "Message": "The request is invalid.",
  "MessageDetail": "The parameters dictionary contains a null entry for parameter 'useTheForce' of non-nullable type 'System.Boolean' for method 'System.String Post(Boolean, Int32, System.String)' in 'SampleApp.MyCommandController'. An optional parameter must be a reference type, a nullable type, or be declared as an optional parameter."
}

It is convenient, yes, but those error details do leave something to be desired. That can be addressed a couple different ways. One option is to use a custom ActionFilter for model validation. For instance:

    internal sealed class ValidateModelAttribute : ActionFilterAttribute
    {
        public override void OnActionExecuting(HttpActionContext actionContext)
        {
            if (!actionContext.ModelState.IsValid)
            {
                actionContext.Response =
                    actionContext.Request.CreateErrorResponse(
                        HttpStatusCode.BadRequest, "The request is invalid.");
            }
        }
    }

    // register globally in Startup.Configuration:
    internal sealed class Startup
    {
        public void Configuration(IAppBuilder appBuilder)
        {
            HttpConfiguration config = new HttpConfiguration();
            // . . .
            config.Filters.Add(new ValidateModelAttribute());
            // . . .
        }
    }

Now the error response looks like this:

{ "Message": "The request is invalid." }

That gets rid of the inscrutable programmer-ese, but it is now quite terse and doesn’t tell the user what actually went wrong. Let’s instead create a full-blown model class to represent the inputs. The added benefit there is the ability to specify even more finely grained validations on the various data members. In this example, we might come up with the following:

    using System.ComponentModel.DataAnnotations;

    public sealed class MyCommandParameters
    {
        [Required]
        public bool useTheForce { get; set; }

        [Required]
        [Range(0, 300)]
        public int timeout { get; set; }

        [Required]
        public string auditMessage { get; set; }
    }

In order to have the same parameter binding experience as before, we have named the properties in lowercase. To ensure the values can be passed as query string parameters, we also need to change the Post method signature as follows:

public string Post(
    [FromUri] MyCommandParameters p)

Finally, to get a more meaningful error message, we can change the ValidateModelAttribute method like so:

        public override void OnActionExecuting(HttpActionContext actionContext)
        {
            if (!actionContext.ModelState.IsValid)
            {
                actionContext.Response =
                    actionContext.Request.CreateErrorResponse(
                        HttpStatusCode.BadRequest, actionContext.ModelState);
            }
        }

The user error experience is now much better:

// If the user passes a large value for the timeout:
{
  "Message": "The request is invalid.",
  "ModelState": { "timeout": [ "The field timeout must be between 0 and 300." ] }
}

// If the user passes 'Yes' for the Boolean:
{
  "Message": "The request is invalid.",
  "ModelState": { "useTheForce": [ "The value 'Yes' is not valid for useTheForce.", "The useTheForce field is required." ] }
}

With just a bit of extra work, we can smoothly go from stringly-typed to strongly-typed. Tune in next time when we look at even more issues in this code sample!