Should my MVC controller really know about JSON?

Question

The JsonResult class is a very useful way to return Json as an action to the client via AJAX.

public JsonResult JoinMailingList(string txtEmail)
{
        // ...

       return new JsonResult()
       {
           Data = new { foo = "123", success = true }
       };
}

However (at least according to my first impression) this really isn't a good separation of concerns.

• Unit test methods are harder to write becasue they don't have nice strongly typed data to test and have to know how to interpret the Json.
• Its harder for some other View in future that isn't over HTTP (or any remote protocol involving serialization) to be 'plugged in' because its unnecessary in such cases to be serializing and deserializing the response.
• What if you have TWO different places that need the results of that action? One wants Json and another wants XML or perhaps a fully or partially rendered view.

I'm wondering why the translation between an object and Json wasn't implemented declaratively via an attribute. In the code below you're essentially telling MVC that this method is convertible to Json, and then if it is called from an AJAX client a check is made for the attribute the new JsonResult() conversion performed internally.

Unit testing can just take the action result (ObjectActionResult) and pull out the strongly typed Foo.

[JsonConvertible]
public ActionResult JoinMailingList(string txtEmail)
{
        // ...

       return new ObjectActionResult()
       {
           Data = new Foo(123, true)
       };
}

I was just curious as to people's thoughts and any alternative pattern to follow.

These are also just my initial observations - there are probably more reasons why this is not an ideal design (and probably plenty why its a perfectly acceptable and practical one!) I'm just feeling theoretical and devils-advocatey tonight.

 * Disclaimer: I haven't even begun to think about how the attribute would be implemented or what sideeffects or repurcussions etc. it might have.

Answer 1

I think you're getting worked up over nothing. So what if the controller knows about JSON in its public interface?

I was once told: "Make your code generic, don't make your application generic."

You're writing an Application Controller here. It's OK for the Application Controller - whose responsibility is to mitigate between the model and views and to invoke changes in the model - to know about a certain view (JSON, HTML, PList, XML, YAML).

In my own projects, I usually have something like:

interface IFormatter {
    ActionResult Format(object o);
}
class HtmlFormatter : IFormatter {
    // ...
}
class JsonFormatter : IFormatter {
    // ...
}
class PlistFormatter : IFormatter {
    // ...
}
class XmlFormatter : IFormatter {
    // ...
}

Basically "formatters" that take objects and give them a different representation. The HtmlFormatters are even smart enough to output tables if their object implements IEnumerable.

Now the controllers that return data (or that can generate parts of the website using HtmlFormatters) take a "format" argument:

public ActionResult JoinMailingList(string txtEmail, string format) {
    // ...
    return Formatter.For(format).Format(
        new { foo = "123", success = true }
    );
}

You could add your "object" formatter for your unit tests:

class ObjectFormatter : IFormatter {
    ActionResult Format(object o) {
        return new ObjectActionResult() {
            Data = o
        };
    }
}

Using this methodology, any of your queries/actions/procedures/ajax calls, whatever you want to call them, can output in a variety of formats.

Answer 2

I generally try not to worry about it. The Asp.Net MVC is enough of a separation of concerns to keep leakage to a minimum. You're right though; there is a bit of a hurdle when testing.

Here's a test helper I use, and it's worked well:

protected static Dictionary<string, string> GetJsonProps(JsonResult result)
{
 var properties = new Dictionary<string, string>();
 if (result != null && result.Data != null)
 {
  object o = result.Data;
  foreach (PropertyDescriptor prop in TypeDescriptor.GetProperties(o))
   properties.Add(prop.Name, prop.GetValue(o) as string);
 }
 return properties;
}

You can use the Request.IsAjaxRequest() extension method to return different ActionResult types:

if (this.Request != null && this.Request.IsAjaxRequest())
 return Json(new { Message = "Success" });
else
 return RedirectToAction("Some Action");

Note: you'll need that Request != null to not break your tests.

Answer 3

I think you have a valid point - why not delegate the "accepted response types vs. generated response types resolution" to some place where it actually belongs?

It reminds me of one of the Jeremy Miller's opinions on how to make an ASP.NET MVC application: “Opinions” on the ASP.NET MVC

In their application, all controller actions have a lean and simple interface - some view model object enters, another view model object leaves.

Answer 4

I'm not sure how big of a problem this actually is, but the "alternative pattern" to follow in ASP.NET MVC would be to write a JSON ViewEngine. This wouldn't actually be that difficult, since the JSON functionality built into the framework will do much of the heavy lifting for you.

I do think that this would be a better design, but I'm not sure it's so much better that it's worth going against the "official" way of implementing JSON.

Answer 5

I'm not too worried about returning JSon as I was before. The nature of AJAX seems to be such that the message you want to process in Javascript only applies for that AJAX situation. The AJAX need for performance just has to influence the code somehow. You probably wouldn't want to return the same data to a different client.

Couple things regarding testing of JSonResult that I've noticed (and I still have yet to write any tests for my app) :

1) when you return a JSonResult from your action method that is 'received' by your test method you still have access to the original Data object. This wasn't apparent to me at first (despite being somewhat obvious). Rob's answer above (or maybe below!) uses this fact to take the Data parameter and create a dictionary from it. If Data is of a known type then of course you can cast it to that type.

Personally I've been only returning very very simple messages through AJAX, without any structure. I came up with an extension method which might be useful for testing if you just have a simple message constructed from an anonymous type. If you have more than one 'level' to your object - you're probably better off creating an actual class to represent the JSon object anyway, in which case you just cast jsonResult.Data to that type.

Sample usage first :

Action method:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult ContactUsForm(FormCollection formData){

     // process formData ...

     var result = new JsonResult()
     {
          Data = new { success = true, message = "Thank you " + firstName }
     };

     return result;
}

Unit test:

var result = controller.ContactUsForm(formsData);
if (result is JSonResult) {

     var json = result as JsonResult;
     bool success = json.GetProperty<bool>("success");
     string message = json.GetProperty<string>("message");

     // validate message and success are as expected
}

You can then run assertions or whatever you want in your test. In addition the extension method will throw exceptions if the type is not as expected.

Extension method:

public static TSource GetProperty<TSource>(this JsonResult json, string propertyName) 
{
    if (propertyName == null) 
    {
        throw new ArgumentNullException("propertyName");
    }

    if (json.Data == null)
    {
        throw new ArgumentNullException("JsonResult.Data"); // what exception should this be?
    }

    // reflection time!
    var propertyInfo = json.Data.GetType().GetProperty(propertyName);

    if (propertyInfo == null) {
        throw new ArgumentException("The property '" + propertyName + "' does not exist on class '" + json.Data.GetType() + "'");
    }

    if (propertyInfo.PropertyType != typeof(TSource))
    {
        throw new ArgumentException("The property '" + propertyName + "' was found on class '" + json.Data.GetType() + "' but was not of expected type '" + typeof(TSource).ToString());
    }

    var reflectedValue = (TSource) propertyInfo.GetValue(json.Data, null);
    return reflectedValue;
}

Answer 6

Alternatively, if you don't want to get into using reflection, you can create a RouteValueDictionary with the result's Data property. Going with the OP's data...

var jsonData = new RouteValueDictionary(result.Data);
Assert.IsNotNull(jsonData);

Assert.AreEqual(2,
                jsonData.Keys.Count);

Assert.AreEqual("123",
                jsonData["foo"]);

Assert.AreEqual(true,
                jsonData["success"]);

Answer 7

I had the same thought and implemented a JsonPox filter to do just that.