Why you shouldn't add methods to interfaces in APIs
- by Simon Cooper
It is an oft-repeated maxim that you shouldn't add methods to a publically-released interface in an API. Recently, I was hit hard when this wasn't followed.
As part of the work on ApplicationMetrics, I've been implementing auto-reporting of MVC action methods; whenever an action was called on a controller, ApplicationMetrics would automatically report it without the developer needing to add manual ReportEvent calls. Fortunately, MVC provides easy hook when a controller is created, letting me log when it happens - the IControllerFactory interface.
Now, the dll we provide to instrument an MVC webapp has to be compiled against .NET 3.5 and MVC 1, as the lowest common denominator. This MVC 1 dll will still work when used in an MVC 2, 3 or 4 webapp because all MVC 2+ webapps have a binding redirect redirecting all references to previous versions of System.Web.Mvc to the correct version, and type forwards taking care of any moved types in the new assemblies. Or at least, it should.
IControllerFactory
In MVC 1 and 2, IControllerFactory was defined as follows:
public interface IControllerFactory
{
IController CreateController(RequestContext requestContext, string controllerName);
void ReleaseController(IController controller);
}
So, to implement the logging controller factory, we simply wrap the existing controller factory:
internal sealed class LoggingControllerFactory : IControllerFactory
{
private readonly IControllerFactory m_CurrentController;
public LoggingControllerFactory(IControllerFactory currentController)
{
m_CurrentController = currentController;
}
public IController CreateController(
RequestContext requestContext, string controllerName)
{
// log the controller being used
FeatureSessionData.ReportEvent("Controller used:", controllerName);
return m_CurrentController.CreateController(requestContext, controllerName);
}
public void ReleaseController(IController controller)
{
m_CurrentController.ReleaseController(controller);
}
}
Easy. This works as expected in MVC 1 and 2. However, in MVC 3 this type was throwing a TypeLoadException, saying a method wasn't implemented. It turns out that, in MVC 3, the definition of IControllerFactory was changed to this:
public interface IControllerFactory
{
IController CreateController(RequestContext requestContext, string controllerName);
SessionStateBehavior GetControllerSessionBehavior(
RequestContext requestContext, string controllerName);
void ReleaseController(IController controller);
}
There's a new method in the interface. So when our MVC 1 dll was redirected to reference System.Web.Mvc v3, LoggingControllerFactory tried to implement version 3 of IControllerFactory, was missing the GetControllerSessionBehaviour method, and so couldn't be loaded by the CLR.
Implementing the new method
Fortunately, there was a workaround. Because interface methods are normally implemented implicitly in the CLR, if we simply declare a virtual method matching the signature of the new method in MVC 3, then it will be ignored in MVC 1 and 2 and implement the extra method in MVC 3:
internal sealed class LoggingControllerFactory : IControllerFactory
{
...
public virtual SessionStateBehaviour GetControllerSessionBehaviour(
RequestContext requestContext, string controllerName) {}
...
}
However, this also has problems - the SessionStateBehaviour type only exists in .NET 4, and we're limited to .NET 3.5 by support for MVC 1 and 2.
This means that the only solutions to support all MVC versions are:
Construct the LoggingControllerFactory type at runtime using reflection
Produce entirely separate dlls for MVC 1&2 and MVC 3.
Ugh. And all because of that blasted extra method!
Another solution?
Fortunately, in this case, there is a third option - System.Web.Mvc also provides a DefaultControllerFactory type that can provide the implementation of GetControllerSessionBehaviour for us in MVC 3, while still allowing us to override CreateController and ReleaseController.
However, this does mean that LoggingControllerFactory won't be able to wrap any calls to GetControllerSessionBehaviour. This is an acceptable bug, given the other options, as very few developers will be overriding GetControllerSessionBehaviour in their own custom controller factory.
So, if you're providing an interface as part of an API, then please please please don't add methods to it. Especially if you don't provide a 'default' implementing type. Any code compiled against the previous version that can't be updated will have some very tough decisions to make to support both versions.
