I have a C# WebAPI project with controller/service/repository structure. Most of my controller methods call a service method which then calls a repository method to access the DB. If one of the parameters from the controller method is null, then an ArgumentNullException is thrown which returns the following from my HttpGlobalExceptionFilter.cs:

{
    "code": 400,
    "messages": [
        "Value cannot be null. (Parameter 'year')"
    ],
    "developerMessage": null
}

However, what about exceptions thrown at the controller level? For example, this is my controller method:

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string DataId)
{
    int dId;
    DataType dType;
    try
    {
        string[] dataParts = DataId.Split('-');
        dType = (DataType)Enum.Parse(typeof(DataType), dataParts[0]);
        dId = int.Parse(dataParts[1]);
    }
    catch (Exception ex)
    {
        return BadRequest();
    }

    Data result = await _dataService.FindData(dType, dId).ConfigureAwait(false);

    if (result == null)
        return NotFound();

    return result;
}

If the DataId parameter is null then I get the following exception:

System.NullReferenceException: 'Object reference not set to an instance of an object.'

thrown at this line:

string[] dataParts = DataId.Split('-');

This leads to the following:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
    "title": "Bad Request",
    "status": 400,
    "traceId": "00-a0aaaa0aa000000aa00a00a00000a0a0-a0a000000000a000-00"
}

How should I return the error/exception at the controller level?

CodePudding user response：

You can throw the exception with the bad Request

Return BadRequest(ex.Message)

CodePudding user response：

Assume that you use ASP.NET 6, the non-nullable property must be required in ASP.NET 6, otherwise the model validation will fail.

Just make your parameter nullable by using ?:

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string? DataId)

Or another way, remove <Nullable>enable</Nullable> from your project file to globally ignore Required validation.

If your parameter contains model data and in this model contains other model validation, you can remove [ApiController] to skip validation before hitting the action.

CodePudding user response：

Seems the difference is that your attempt to get data from the database is not wrapped in a try/catch block so any exceptions go straight to the global handler. But the NullReferenceException on string[] dataParts = DataId.Split('-'); is being caught and handled by returning BadRequest.

So you could just remove the try/catch and allow any exception to be handled globally:

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string DataId)
{
    int dId;
    DataType dType;

    string[] dataParts = DataId.Split('-');
    dType = (DataType)Enum.Parse(typeof(DataType), dataParts[0]);
    dId = int.Parse(dataParts[1]);

    Data result = await _dataService.FindData(dType, dId).ConfigureAwait(false);

    if (result == null)
        return NotFound();

    return result;
}

Or you include the message in the returned BadRequest:

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string DataId)
{
    int dId;
    DataType dType;
    try
    {
        string[] dataParts = DataId.Split('-');
        dType = (DataType)Enum.Parse(typeof(DataType), dataParts[0]);
        dId = int.Parse(dataParts[1]);
    }
    catch (Exception ex)
    {
        return BadRequest(ex.Message);
    }

    Data result = await _dataService.FindData(dType, dId).ConfigureAwait(false);

    if (result == null)
        return NotFound();

    return result;
}

You could also clean up a little by handling your data errors in the controller too:

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string DataId)
{
    try
    {
        string[] dataParts = dataId.Split('-');
        DataType dType = (DataType)Enum.Parse(typeof(DataType), dataParts[0]);
        int dId = int.Parse(dataParts[1]);

        Data result = await _dataService.FindData(dType, dId).ConfigureAwait(false);

        if (result == null)
            return NotFound();

        return result;
    }
    catch (Exception ex)
    {
        return BadRequest(ex.Message);
    }
}

But you also have various potential issues with parsing the passed string that are well-known so you could reduce potential exceptions by ensuring you are checking the input correctly:

Note: method arguments should typically be camelCase.

public async Task<ActionResult<Data>> GetDataByYearAsync([FromRoute] string dataId)
{
    // check whether the argument is null before trying to use it.
    if (string.IsNullOrEmpty(dataId))
        return BadRequest($"Value cannot be null: {nameof(dataId)}");

    string[] dataParts = DataId.Split('-');
    // ensure that the string is in the expected format before progressing.
    if (dataParts.Length != 2)
        return BadRequest($"{nameof(dataId)} is invalid");

    // Use Enum.TryParse to ensure no exception is thrown for a non-matching string.
    if (!Enum.TryParse(dataParts[0], out DataType dType))
        return BadRequest($"{nameof(dataId)} unknown DataType");

    // Use int.TryParse to elegantly return if the value is invalid.
    if (!int.TryParse(dataParts[1], out int dId))
        return BadRequest($"{nameof(dataId)} could not parse integer value");

    // this will still throw and be handled globally if there are any issues.
    Data result = await _dataService.FindData(dType, dId).ConfigureAwait(false);

    if (result == null)
        return NotFound();

    return result;
}