Web API architecture design

Question

I'm learning and diving into Web API. I've used web services before but not specifically with web API. Below is how I am designing it so far and I was curious on the feedback.

I have a ReturnMessages object. This basically is a standard object that gets returned from any of the API calls, correctly executed or an error happens. Within each API method I have a try catch. if everything is alright, I specify the values I need within my ReturnMessages object and than Ok(_returnMessages). Now if an error happens I fill in the ReturnMessages object with the error information and once again return Ok(_returnMessages).

The ReturnMessages object contains a ReturnClass field to hold any type of other objects I may need to return. A single object or an Array. It also has a return code, return message, friendly message for the end user in case something wrong happens and than a generic string list of data that was passed in that I can send off and use for testing purposes to try and re-create the error.

Below is a code sample from one of the methods that shows off what I am talking about. Is this approach ok with always returning Ok with the object I'm returning or am I missing potential pieces within the Web API that I should be utilizing? I've seen the NotFound exceptions and all that other fun stuff.

**EDIT: I made some changes in terms of what I was passing back for when things don't work out. I took out the ReturnMessage and also the ReturnData and wrote that to the Application event log for a new string value for this particular web api. Taht occurs in the _lw code line which is just a LogWriter class that uses the values passed in to write to the application event log.

public IHttpActionResult method1(string arg 1= null, string arg2 = null, string arg3 = null)
        {
            try
            {
                clrObject1 varClrObject = (from p in db.clrObject1
                                            where p.column1 == arg1
                                            select p).SingleOrDefault();

                if (varClrObject == null)
                {
                    _returnMessages = new ReturnMessages
                    {
                        ReturnCode = 204,
                        FriendlyErrorMessage = "Nothing found matches the supplied values."
                    };

                   _lw.SetupEventLog("No data was found that matched the information supplied.\n\nParameters:\narg1: " + arg1 + "\narg2: " + arg2 + "\narg2=" + arg2, "Warning");
                }
                else
                {
                    _returnMessages = new ReturnMessages
                    {
                        ReturnCode = 200,
                        ReturnClass = varClrObject,
                        ReturnMessage = "Information Successfully Retrieved"
                    };
                }
            }
            catch (Exception e)
            {
                _returnMessages = new ReturnMessages
                {
                    FriendlyErrorMessage = "An error has occurred while getting your information. Please try again in a few minutes.  A notification was already sent to the company about this issue.",
                    ReturnCode = 400
                };

                _lw.SetupEventLog("Parameters:\narg1: " + arg1 + "\narg2: " + arg2 + "\narg3=" + arg3, "Error", e);
            }

            return Ok(new { Response = _returnMessages });
        }

Answer 1

You are overcomplicating this.

Web API is designed to let you write your controller code is as "natural" a way as possible. The IHttpActionResult return type is mostly intended as an escape hatch when you want maximal flexibility.

Here is how I would write your method:

public ClrObject1 Method1(string arg1 = null, string arg2 = null, string arg3 = null)
{
    ClrObject1 varClrObject = (from p in db.clrObject1
                               where p.column1 == arg1
                               select p).SingleOrDefault();

    if (varClrObject == null)
    {
        // This line will automatically return a 404 error to the user.
        throw new HttpResponseException(HttpStatusCode.NotFound);
        // If you want, you can also build a full HttpResponseMessage
        // to give more details about the error.
    }

    // Web API will automatically return serialize and return your object
    // in a 200 OK response.
    return varClrObject;
}

You will notice that I do not put a try-catch block to handle exceptions - the reason is that, if any exception is raised and unhandled, an error 500 will be returned to the user, which is usually what I want. Also, you do not want always to return details about your exception in the HTTP response, as such information could disclose information that could be used in helping hacking the application server.

Answer 2

I agree with @jhominal that you are making this way more complicated than it needs to be. First, your action should return a concrete instance of an object. The reason for this is to allow the Web API framework to perform content negotiation between JSON and Xml without a lot of fuss. If you don't have to worry about this and can choose between one or the other, you can use an interface for your object (which is typically ideal), but you do not need an HttpActionResult because when you're performing a get in Web API you're looking for a specific resource and that resource should have a type. Anything that does not return a resource should return a properly formatted HttpResponseMessage which is most easily handled through an HttpResponseException. I'll paste a snippet below with comments to describe how I like to build controller actions:

// I always specify the http verb that the method is 
// intended to respond to. It's extremely rare that
// multiple verbs should perform the same actions in
// an API. Also, I'm not a big fan of using optional
// parameters in this manner. I much prefer to use a 
// common get action and then pull the arguments like
// this from the RequestUri. It helps keep my routes
// nice and clean
[HttpGet]        
public MyCustomObject Post(string arg 1= null, string arg2 = null, string arg3 = null)
{
    // Perform validation of arguments as necessary
    // If it's not necessary, then skip this
    if (string.IsNullOrWhiteSpace(arg1))
    {
        throw new HttpResponseException(Request.CreateErrorResponse(
            HttpStatusCode.BadRequest,
            "I need to be supplied with arg1."));
    }

    // I prefer to send a request with relevant data to 
    // a rules engine/library of some sort that returns 
    // a formatted response (very similar to your
    // ReturnMessages object
    var response = MyRules.PerformQuery<MyCustomObject>(arg1, arg2, arg3);

    // Just as your ReturnMessages I use a status code
    if (response.Code == ResponseCode.Success)
    {
        return response.Entity;
        // In my preferred architecture, response.Entity is a
        // generic type, in this case MyCustomObject as above
    }

    // If you get to this point, something has gone wrong.
    // You can add additional logic before or after your
    // call to deliver this response message. The arguments
    // necessary are a property HttpStatusCode (any code)
    // and a valid response message string.
    throw new HttpResponseException(
        Request.CreateErrorResponse(response.Code, response.Message));
}

So to sum up, I try to keep my architecture as simple as possible. The bones of it are argument validation, a call to a rules engine, concrete object response, and then exception cases. It helps keep the controller action delivering properly http responses relative to resources and messages.