How To Design Easily Testable Asp.Net Mvc Action Filters

By Mike on 16 May 2013

While Asp.Net Mvc is a much more test friendly framework compared to WebForms, some things are still complicated to test. Custom Action Filters are very common and naturally you want to test them too. But you still have to setup a whole chain of dependencies (the filterContext argument) even for trivial filters.

However, we can test things much easier, without the complicating setup. Let's say I want a filter that takes the id from the url, gets the user for that id and then cache it in HttpContext.Items. It looks something like this

public class MyFilter:IActionFilter
    {
        
        public void OnActionExecuting(ActionExecutingContext filterContext)
        {
            var repo = DependencyResolver.Current.GetService<IMyRepository>();
            int uid;
            if (!int.TryParse(filterContext.RouteData.GetRequiredString("id"),out uid))
            {
                filterContext.Result=new HttpNotFoundResult();
                return;
            }
            var user = repo.GetById(uid);
            if (user == null)
            {
                filterContext.Result = new HttpNotFoundResult();
                return;
            }
            filterContext.HttpContext.Items["user"] = user;
        }
        
     
        public void OnActionExecuted(ActionExecutedContext filterContext)
        {
            
        }
    }

While the behavior is quite simplistic look what I have to fake: the DependecyResolver, the RouteData and the HttpContext. Let's refactor it so that it's easily testable

public void OnActionExecuting(ActionExecutingContext filterContext)
        {
            var repo = DependencyResolver.Current.GetService<IMyRepository>();            
            var user = GetUser(repo, filterContext.RouteData.GetRequiredString("id"));
            filterContext.Result = CacheUser(user, filterContext.HttpContext.Items);                        
        }

        public User GetUser(IMyRepository repo, string id)
        {
            int uid;
            User user = null;
            if (int.TryParse(id, out uid))
            {
                user = repo.GetById(uid);                
            }
            return user;
        }

        public ActionResult CacheUser(User user, IDictionary cache)
        {
            if (user == null)
            {
                return new HttpNotFoundResult();
            }
            cache["user"] = user;
            return null;
        }

I've isolated the desired behavior in 2 public easily testable methods. With GetUser you can test what happens if the 'id' is or not a valid user id having only to mock IMyRepository. With CacheUser you can test what happens if the User is null or not. None of these methods depend on asp.net mvc so no need to fake all the ActionExecutingContext object. Better yet, the behavior can be used outside the action filter as well.

It's a trivial example because it's a simple technique: extract the actual behavior in one or more relevant methods which take only the dependencies they need. In this example we needed only the repository and an IDictionary. Very rarely you'll need HttpRequest or HttpContext as a dependency, usually you need only a property.

Here's another example. I want to allow access only to users with the email belonging to some domain.

public class DomainEmailAuth:AuthorizeAttribute
    {
        public string EmailDomain { get; set; }

        protected override bool AuthorizeCore(System.Web.HttpContextBase httpContext)
        {
            if (base.AuthorizeCore(httpContext))
            {
                var repo = DependencyResolver.Current.GetService<IUserServices>();
                return IsAuthorized(repo, httpContext.User.Identity.Name);
            }
            return false;
        }

        public bool IsAuthorized(IUserServices users,string username)
        {
            Email email = users.GetEmail(username);
            if (email != null)
            {
                if (email.BelongsToDomain(EmailDomain))
                {
                    return true;
                }
            }
            return false;
        }
    }

 The relevant behavior needs only the UserServices and the username in order to get the email and find out if it is allowed. It can be tested without caring about asp.net or the AuthorizeAttribute itself.

Invoking A Private Method On A Subclass

By Mike on 19 April 2012

I was just (re)watching Greg Young's Event Sourcing Presentation when the following code appeared on slides

public abstract class Aggregate
 {
      private void ApplyChange(Event @event,bool isNew)
        {
            this.AsDynamic().Apply(@event);           
            if (isNew) _changes.Add(d);
        }
}

Greg tells the audience that this method does some black magic(.Net only) to basically call an Apply method defined in a subtype, like this

public class SubType: Aggregate
{
     private Apply(DeactivatedItemEvent @event)
	 {
	    _deactivated=true;
	 }
}

Of course the scenario was that there are have many overloads of Apply, each for a different event. With this trick, you just write the method and that's it. The compiler and the runtime will work for you to select the correct overload.
So I was curious about the magic that made the call possible. You might think that a simple

private dynamic AsDynamic() { return this; }

is enough. But it isn't. In fact, it will throw an exception that the member is inaccessible. So what can we do? If you're browsing to Greg's CQRS application example you'll see some complicated implementation of DynamicObject which does 2 things: uses reflection (member.Invoke) to call the private method and does nothing if the method is missing. Now, the performance is good enough (around 1 second for 100 000 calls) but surely we can do better?

After some messing around, I've come up with 3 ways to do it, besides reflection.

  1. The simplest way is to just make the method public , but that's kinda defeats the purpose of encapsulation. Not really an option.
  2. Another way is to define an abstract method that will be implemented by the sub type
    public class Aggregate
    {
      private void ApplyChange(Event d,bool isNew)
            {
                ApplyX(d);
               
                if (isNew) _changes.Add(d);
            }
    
            protected abstract void ApplyX(Event d); 
    }
    
    public class SubType: Aggregate
    {
         protected override void ApplyX(Event d)
            {
                Apply((dynamic)d);
            }
    }
    

    Casting the argument to dynamic does the trick here and this is the fastest implementation (around 185 ms for 100 000 method calls). But you have to write that override for every subtype. Not a high price to pay to get the best performance, IMO.

  3. Write a dynamic method which will invoke a private method, directly without reflection
    public class Aggregate
    {
             protected void ApplyX(Event d)
            {
                var evt = d.GetType();
                lock (_locker)
                {
                    if (!_invokers.ContainsKey(evt))
                    {
                        DynamicMethod met = new DynamicMethod("invoker", typeof(void), new[] { typeof(EventEntityBase), typeof(Event) }, typeof(EventEntityBase).Module, true);
                        var il = met.GetILGenerator();
                        il.Emit(OpCodes.Ldarg_0);//subtype           
                        il.Emit(OpCodes.Ldarg_1);//event
                        il.Emit(OpCodes.Call, GetType().GetMethod("Apply", BindingFlags.Instance | BindingFlags.NonPublic, null, new[] { d.GetType() }, null));//call apply
                        il.Emit(OpCodes.Ret);
                        var del = (Invoker)met.CreateDelegate(typeof(Invoker));
                        _invokers[evt] = del;
                    }
                }
                _invokers[evt](this, d);
            }
    
    
            static object _locker = new object();
            static Dictionary<Type, Invoker> _invokers = new Dictionary<Type, Invoker>();
    
            public delegate void Invoker(EventEntityBase bs, Event ev); 
    }
    
    In a nutshell, a static method is created via Reflection.Emit, then used via a delegate, nothing fancy here. The delegate is cached for further reuse. This approach doesn't require anything implemented by the subtype so everything is contained by the base class. However its performance was a bit of surprise as it's slower than using dynamic directly (around 260ms for 100 000 method calls).

There you have it. You can choose the reflection way, which is the slowest or to use the dynamic feature of C# (almost 10x faster) but requires the subtype support or to emit a dynamic method ('only' 5x times faster). I think I'm going to use the approach with the dynamic keyword.

The Generic Repository Is An Anti-Pattern

By Mike on 5 March 2012

It seems to me that when you search about repository pattern, you inevitable stumble upon the generic repository, touted as how a repository should be implemented. Too bad that's a fallacy. A repository is a concept to abstract the access to the persistence, that is not to depend on data access implementation details. There is no formula and no rules.

A repository is also the contract used by the rest of the application to save/load objects.  Every application is different and has different needs of accessing the persistence. And yet , many advise you to use a generic repository like this

public interface IRepository<T>
 {
    IEnumerable<T> GetAll();
    T GetByID(int id);   
    void Add(T entity);
    void Update(T entity);
    void Delete(T entity);
 }

  One of the touted benefits is the DRY (Don't Repeat Yourself) principle. It's good we don't apply the principle when talking, I mean it would be hard not to repeat the same sounds or words. Sadly, for some it seems that every application has the same persistence access needs, because that's the only scenario where a generic repository makes sense.
 
 I'm aware that a Save , Get or Update method are common in a repository but it depends... . Am I dealing with a query repository or a command one? Is it a repository specialized for reading or for updating model? Because a generic repository can't handle those cases. In one repository you might need GetById and GetByTitle with pagination support. In other you might need only a simple Get method. Other repository might deal only with Save/Update a domain entity.
 
 What I'm trying to say is that the design of a repository interface depends too much on what bounded context it applies for, so there isn't a generic form. If you find that things are repeating it might be a sign of poor architecture or that specific application is really suitable for something generic. But that's a specific case, not the rule.
 
 Other offender in regard to generic repositories is the fact that lots of developers just use it to wrap the DAO (Database Access Object) or an underlying ORM (like EF or Nhibernate). Doing so they add only a useless abstraction, pretty much just making the code more complex with no benefits. A DAO makes it easy to work with a database, an ORM makes it easy to access a database as an OOP virtual storage and to eventually abstract the access to a specific database.
 
 But the repository should abstract the whole persistence layer, hiding implementation details like database engine or what DAO or ORM the app is using but also providing a contract that makes sense from the application point of view. The repository serves the application needs, NOT the database needs.
 
 A generic repository serves the dogma because very few applications have a need for a generic contract applicable everywhere and when used just to mask a DAO, it becomes an unnecessary abstraction. These two reasons make the Generic Repository pattern an anti pattern.

Update: A generic repository fits almost naturally as a domain repository, but only as that. To use it anytime you want a repository and especially when dealing with query repos doesn't make sense.

When To Use The Static Keyword In .Net

By Mike on 2 March 2012

f you come from a procedural language background or mindset, you'll find a very useful friend in the static keyword. Since C# and Vb.Net are OOP languages, having static methods and static fields in a utility class is the only way to mimic global functions. However, if you want to do good OOP and you aren't very experienced,  misusing 'static' can lead to a headful (that's a pun) of problems.

Take this code (actual code from a StackOverflow question)

public static class dbConnection
{
    public static SqlConnection cn = new SqlConnection();

    public static void OpenConnection()
    {
        try
        {
            cn.ConnectionString = ConfigurationManager.ConnectionStrings["cnWebTwDrill"].ToString();

            if (cn.State == ConnectionState.Closed)
                cn.Open();
        }
        catch (Exception)
        {
            throw;             
        }
    }
}


This is a good example of bad usage of static, especially since this code is from a web application (read: multi threaded application)! Being static, means that the same connection will be available as long as the application runs and that it will be shared and modified by every thread accessing it, which is a major concern in a high concurrency application. Lacking any thread safety mechanism means that threads will read and modify each other data in a non determinant way. So this is a case where you SHOULDN'T use static.

In fact every time you're tempted to make something static you should think it thoroughly at least 2 times. The only case where you don't need to think about it is when defining extension methods which require static methods in a static class. But anything else is up for debate.

That being said, when it's appropriate to use static? First candidates are methods which don't require state at all. They process their arguments as input and return an output. The best example is a utility method, but you should consider if that utility doesn't fit better as a behavior of an object. In almost all cases, a 'real' utility method can be used unmodified  as an extension method.

Other important thing to be aware of is that anything static should take into account thread synchronization. A static field or property can be read or modified by any thread at random times and this should be handled properly and before you rush using locks everywhere, ask yourself again: 'Do I need this to be static?'. If so, make sure you deal with the synchronization issue.

Another aspect is testing. By their nature, static classes and properties are against testing, since they maintain their state as long as the containing application runs. An utility method is easy testable, a class with a static field just messes up things. If you need it to be testable, avoid making it static.

What about factory methods? A static factory method makes sense. The best usage is when you need to return an abstraction, a good example is an abstract class with a factory method which returns an instance of a derived class. But you can also use a static factory method to control the instantiation of a type, either to create an instance from another type ( example: the Int32.Parse method) or to validate a constructor argument gracefully without throwing exceptions (example: the Int32.TryParse method).

As you can see, static has pretty limited good usage and it's rather a trap if you're not experienced enough. So when you need a specific functionality, try first to see if it can fit an object, only afterwards decide if it helps more to be static. As a general principle avoid making static any code which needs disposing, which may take long to complete or which works with unmanaged resources.

In a nutshell, if
- functionality is utilitarian and it doesn't need state, or
- factory method, or
- needs to be accessed by multiple independent threads (like request processing in an asp.net application)
then you have a good candidate for a static method or a class with at least a static field.