A downcasting tragedy

And now, for another tale of legacy code woe, here’s a gut-wrenching tragedy of OCP and downcasting, in five acts.

Exposition

We have a feature we’re implementing that when the store’s configuration settings allow for a survey link to be shown, it should show up, otherwise, it shouldn’t.  We put these settings in a simple interface (cut down for brevity):

public interface ISettings
{
    bool ShowSurvey { get; }
}

Settings can come from many places, such as configuration files, database, or web service.  This is a pretty nice example of OCP, as our app is open for extension for different “ISettings” implementations, but closed for modification as someone can’t decide to use something else besides “ISettings”.  Since we have an interface, the source doesn’t matter.

We manage access to the settings through a static class:

public static class StoreContext
{
    public static ISettings Settings { get; set; }
}

Actual instances are set during early in the ASP.NET pipeline, so later stages have that information.  The original ISettings implementations are stored in ASP.NET Session.

Now, we need to provide functional and regression tests for this feature, using Selenium.

Rising action

To help with functional and regression tests, I’m employing the record and playback state technique I mentioned earlier.  I create an “ISettings” implementation that can be serialized and deserialized into XML, which helps when I want to manually edit the settings:

[XmlRoot("Settings")]
public class SettingsDto : ISettings
{
    public SettingsDto() { }

    public SettingsDto(ISettings settings)
    {
        ShowSurvey = settings.ShowSurvey;
    }

    public bool ShowSurvey { get; set; }
}

My “download.aspx” page combines both the cart and the settings information into a single DTO, and streams this file to the client.  In my “replay.aspx” page, I take the XML, deserialize it, and put it into Session.  Finally, I redirect to the “shoppingcart.aspx” page, which pulls the ISettings implementation out of Session and sets the StoreContext variable to my deserialized SettingsDto instance.

This is all fairly straightforward, and I’m pretty happy about this solution as I can now plug in any settings I like and verify the results through automated tests.  As it is with all legacy code, things just aren’t that simple.

Climax

Running the code the first time, downloading and replaying goes smoothly, and all of my values are serialized and deserialized properly as my unit tests prove.  However, when I get redirected to the “shoppingcart.aspx” page, I get a distressing exception:

System.InvalidCastException:
  Unable to cast object of type 'BusinessObjects.SettingsDto'
  to type 'BusinessObjects.DbSettings'.

The dreaded invalid cast exception, something is making some really bad assumptions and killing my code.

Falling action

Just when I thought I was making progress, the InvalidCastException came up and crushed my dreams.  Looking at the stack trace, I see that my the exception is coming from the base Page class that all pages in our app derive from.  Here’s the offending code:

public abstract class StorePage : Page
{
    protected DbSettings Settings
    {
        get { return (DbSettings)StoreContext.Settings; }
    }
}

A convenience property on the base class allowed derived classes to get at the static ISettings instance.  But on closer inspection, this property is downcasting the ISettings instance coming out of StoreContext to DbSettings.  Downcasting is when you cast to a derived type from an instance of a base type.  That’s a fairly large assumption to make, though.  How do I know for sure that the instance is really the derived type, and not some other derived type of the base type?  I can’t guarantee that assumption holds, as this example shows.

To make things worse, it explicitly casts instead of using the C# “as” operator (TryCast for VB.NET folks).  At least then it won’t fail when trying to downcast, it would just be null.  Using the “as” operator is perfectly acceptable as a means of downcasting.  I try to avoid it, as it can be a smell of Inappropriate Intimacy.

Because the code assumes that the ISettings instance is actually seriously for realz a DbSettings instance, I can never take advantage of OCP and swap out different “ISettings” implementations.  Quite depressing.

Denouement

As an experiment, and because I have a nice automated build process, I can just try to change the type referenced in the StorePage class to ISettings instead of DbSettings.  Luckily, the build succeeded and I kept the change.

If it had failed and clients absolutely needed DbSettings, I might have to look at pushing up methods to the ISettings interface, if it’s possible.  If I have access to ISettings that is.  If I don’t, well, I’m hosed, and my denouement would turn into a catastrophe.

The moral of this story is:

Avoid downcasting, as it makes potentially false assumptions about the underlying type of an object.  Use the “as” operator instead, and perform null checking to determine if the downcasting succeeded or not.  We can then perform safe downcasting and avoid using exceptions as flow control.