Why it is important to use HTTP verbs appropriately

On the morning of Thursday 29th August it became apparent that our WIKI was empty and missing all of its pages. We took it offline shortly afterwards and started to investigate why the data was missing and how we might restore it.

Background

The previous day a team member came over to ask why they didn’t have permissions to edit the attachments they had uploaded so they could replace it with a different one. We are using Roadkill Wiki which unfortunately does not support granular permissions, you are either an editor or an admin and only admins can edit uploaded files. To unblock the user we promoted all domain users to admins which at the time was sub-optimal but didn’t seem totally unreasonable.

What happened to the data?

The data was intact and accessible that evening but missing the following morning, after checking the event logs we noticed a call to a page on the wiki that users would not usually ask for.

Request information: 
    Request URL: http://tfs-wiki/pages/version/b81d890b-b3ae-48cd-864b-06550aee8490 
    Request path: /pages/version/b81d890b-b3ae-48cd-864b-06550aee8490 
    User host address: 10.200.95.135 
    User: DOMAIN\SCOM_2012_DATA_READ 
    Is authenticated: True 
    Authentication Type: Negotiate 
    Thread account name: NT AUTHORITY\NETWORK SERVICE

We checked with infrastructure about the SCOM account and it seemed likely to be related to the Search Server 2010 indexer. This would normally be fine and has been running for a while, however with the elevated permissions this crawler would have access to links on the wiki which it lacked before.

On the allpages page there is a full list of pages and their corresponding delete links. Unfortunately the delete links are GET requests which look like normal links.

<a class="btn btn-mini confirm" href="/pages/delete/134">Delete</a>

So there was a good chance that all these delete links were hit by the indexer as it crawled our wiki. We were able to verify this in the search indexers database. A count of 116 pages (which is all of them) were removed.

 

A better solution?

Promoting all domain users to administrators to get around the limitations of the permission model was a grave error, which was unfortunately compounded by the URL design on the wiki engine. Considering a request to delete an object changes server state, it should have been a POST or DELETE. This is mentioned in the HTTP protocol section 9.1.1.

In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe". This allows user agents to represent other methods, such as POST, PUT and DELETE, in a special way, so that the user is made aware of the fact that a possibly unsafe action is being requested.

In the future we will definitely be more careful when making permission changes, and I will personally be even more mindful of HTTP guidelines when designing web applications and web services. Time permitting I hope to send a patch to the Roadkill Wiki contributors.

Unity IoC container: tips, tricks and dirty hacks

It took me two years to write this post. Each time I started writing it, it suffered from gradual and seemingly unavoidable feature creep: should I mention dependency inversion principle? What about SOLID principles in general? What about testability? The list goes on and on. But it was a recent interesting discussion with a friend about the use (and abuse) of dependency injection that prompted me to finally finish it.

I have been using Unity IoC container for a while now and in the process I have managed to ”abuse” it in every possible way so I hope this post covers some of the more advanced and intricate usage scenarios you may ever come across.

Trivial case

In case you have never used Unity before, the behaviour of the container is pretty trivial: call Register<T,TImpl>() on a number of types then make a call to Resolve<T> and with a bit of luck you will get a instance of it:

[TestFixture]
    class TrivialTests
    {
        [Test]
        public void TrivialOne()
        {
            var container = new UnityContainer();

            container.RegisterType<IBar, Bar>();

            var instance = container.Resolve<IBar>();

            Assert.NotNull(instance);
        }
    }

    internal class Bar : IBar {}
    internal interface IBar {}

Implementing a factory with Unityeven

Unity is not only able to resolve an implementation of a particular interface; it may also help in resolving dependencies when the type to be instantiated is known ( if it has not been registered with the container):

    [TestFixture]
    public class ObjectBuildupTest
    {
        [Test]
        public void ObjectBuildup()
        {
            var container = new UnityContainer();
            container.RegisterType<ISomeService, SomeService>();

            var anInstance = container.Resolve<ObjectWithDependencies>();

            Assert.NotNull(anInstance);
        }
    }

    public class ObjectWithDependencies
    {
        public ObjectWithDependencies(ISomeService  someService)
        {
            if (someService == null) throw new ArgumentNullException("someService");
        }
    }

    public interface ISomeService {}
    public class SomeService : ISomeService {}

As you can see from the code above, even though the type “ObjectWithDependencies” has not been registered with the container, Unity was able to correctly instantiate it and inject it with previously registered dependency. Such functionality may come very useful when implementing a factory: we know what we want to instantiate but not necessarily want to be bothered with instantiating all the dependencies. The following sample illustrates this approach.

    [TestFixture]
    public class FactoryTest
    {
        [Test]
        public void UnityBasedFactoryTest()
        {
            var container = new UnityContainer();

            container.RegisterType<ISomeService, SomeService>();
            container.RegisterType<ISomeFactory, SomeFactory>();

            var factory = container.Resolve<ISomeFactory>();

            Assert.NotNull(factory.ManufactureAnObject("A"));
        }
    }

    public interface ISomeFactory
    {
        ICommonInterface ManufactureAnObject(string anArgument);
    }

    public class SomeFactory : ISomeFactory
    {
        private readonly IUnityContainer _container;

        public SomeFactory(IUnityContainer container)
        {
            _container = container;
        }

        public ICommonInterface ManufactureAnObject(string anArgument)
        {
            switch(anArgument)
            {
                case "A":
                    return _container.Resolve<TypeA>();
                case "B":
                    return _container.Resolve<TypeB>();
                default:
                    throw new NotSupportedException();
            }
        }
    }

    public interface ICommonInterface {}

    public class TypeB : ICommonInterface
    {
        public TypeB(ISomeService someService)
        {            
        }
    }

    public class TypeA : ICommonInterface
    {
        public TypeA(ISomeService someService)
        {            
        }
    }

As you have probably noticed our factory requires an instance of IUnityContainer, luckily the container auto-registers itself, so whenever and instance of IUnityContainer is required, one will be injected without any fuss. Purists may frown upon direct use of the container, but in my opinion this is one of the very rare cases where it can be justified.

Multiple implementations of the same contract

The following example illustrates how to register multiple implementations of the same interface. Each registration is done with a particular key (quite a common feature among IoC containers) and then when resolving it we need to specify the key to get a particular implementation:

    [TestFixture]
    public class MultipleImplementationsTests
    {
        [Test]
        public void MultipleImplementations_ResolveCorrectly()
        {
            var container = new UnityContainer();
            container.RegisterType<IFooBar, SomeFooBar>("first");
            container.RegisterType<IFooBar, OtherFooBar>("second");

            Assert.NotNull(container.Resolve<IFooBar>("first"));
            Assert.NotNull(container.Resolve<IFooBar>("second"));
            Assert.Throws<ResolutionFailedException>(() => container.Resolve<IFooBar>());
        }
    }

    public interface IFooBar {}
    public class OtherFooBar : IFooBar {}
    public class SomeFooBar : IFooBar {}

This functionality can again be used to implement a factory of sorts but in general it is not that interesting until we get to the next point.

Building a composite

Having a multiple implementations of a particular contract registered with the container is the stepping stone to building a composite, as the following sample illustrates:

    [TestFixture]
    public class CompositeTests
    {
        [Test]
        public void BuildComposite()
        {
            var container = new UnityContainer();
            container.RegisterType<IFoo, SomeFoo>("first");
            container.RegisterType<IFoo, AnotherFoo>("second");
            container.RegisterType<IFoo, CompositeFoo>();

            var instanceOfFoo = container.Resolve<IFoo>();

            Assert.IsInstanceOf<CompositeFoo>(instanceOfFoo);
        }
    }

    public class CompositeFoo : IFoo
    {
        public CompositeFoo(IFoo[] others)
        {
            Debug.Assert(others != null);
            Debug.Assert(others.Any());
        }
    }

    public class AnotherFoo : IFoo {}
    public class SomeFoo : IFoo {}
    public interface IFoo {}

Composite in this case “consumes” an array of child objects and in a sense “sucks in” every implementation of IFoo registered with a key. This is an important aspect: if you were to register composite with a key, it would try to instantiate itself leading in immediately to StackOverflowException. The other interesting aspect is the fact that the composite requires an argument of type IChild[]. I am not sure about you but I personally find arrays very much 20th century and would rather have IEnumerable<IChild> injected, sadly Unity does not support it out of the box. There is however a workaround for it which I will share with you later.

Building chain of responsibility

Another interesting use case for naming your registrations is the implementation of Chain Of Responsibility pattern as the following sample illustrates:

    [TestFixture]
    class ChainOfResponsibilityTests
    {
        [Test]
        public void BuildChain()
        {
            var container = new UnityContainer();
            container.RegisterType<IChainLink, LastLink>("last");
            container.RegisterType<IChainLink, SecondLink>("second",
                                                           new InjectionConstructor(
                                                               new ResolvedParameter<IChainLink>("last")));
            container.RegisterType<IChainLink, FirstLink>(new InjectionConstructor(
                                                               new ResolvedParameter<IChainLink>("second")));

            var firstLink = container.Resolve<IChainLink>();

            Assert.IsInstanceOf<FirstLink>(firstLink);
        }
    }

    internal interface IChainLink {}

    internal class FirstLink : IChainLink
    {
        public FirstLink(IChainLink nextLink)
        {
            if (nextLink == null) throw new ArgumentNullException("nextLink");
        }
    }

    internal class SecondLink : IChainLink
    {
        public SecondLink(IChainLink nextLink)
        {
            if (nextLink == null) throw new ArgumentNullException("nextLink");
        }
    }

    internal class LastLink : IChainLink {}

 

In this case we need to resort to pointing each registered element of the chain to its predecessor which is achieved using ResolvedParameter<T>(name). This way we have full control as to what registration gets injected where. This may come helpful in cases where different implementations of the same contract are to be injected in different places.

Injecting the same object in multiple places in the object graph (semi-singleton)

Unity has a very interesting feature which allows the same object to be injected and shared in multiple places in the object graph. Let’s consider the following object diagram

:image

As you can see both the parent and the children contain references to an instance of Service. Standard behaviour of the IoC container is to inject new instance of the dependency into every object in the graph, this however may not be what we want:

image

In order to solve this conundrum Unity provides PerResolveLifetimeManager which within a single call to Resolve<T>() will create an instance of particular type and reuse it wherever required as the next sample illustrates:

    [TestFixture]
    public class PerResolutionLifetimeTests
    {
        [Test]
        public void Regiser_WithPerResolveLifetimeManager_ContainerInjectsSameInstance()
        {
            var container = new UnityContainer();
            container.RegisterType<IService, AService>(new PerResolveLifetimeManager());
            container.RegisterType<IParent, Parent>();
            container.RegisterType<IChild, Child>();

            var parent = container.Resolve<IParent>();

            Assert.AreSame(parent.Service, parent.Child.Service);
        }
    }

    public interface IService {}
    public class AService : IService {}
    public interface IChild
    {
        IService Service { get; set; }
    }

    public class Child : IChild
    {
        public IService Service { get; set; }

        public Child(IService service)
        {
            Service = service;
        }
    }

    public interface IParent
    {
        IChild Child { get; set; }
        IService Service { get; set; }
    }

    public class Parent : IParent
    {
        public IChild Child { get; set; }
        public IService Service { get; set; }

        public Parent(IChild child, IService service)
        {
            Child = child;
            Service = service;
        }
    }

Eternal problem: Chicken and Egg

Now there is an interesting chicken and egg problem which may seem to be impossible to solve using IoC container:

    
    public interface IEgg
    {
        IChicken Chicken { get; set; }
    }

    public interface IChicken
    {
        IEgg Egg { get; set; }
    }

    public class Chicken : IChicken
    {
        public IEgg Egg { get; set; }
    }

    public class Egg : IEgg
    {
        public Egg(IChicken chicken)
        {
            Chicken = chicken;
        }

        public IChicken Chicken
        {
            get;
            set;
        }
    }

As you can see we have a circular dependency going on here (Chicken “contains” an egg, while the egg points to its parent). This may seem to be a hopeless case but there is a solution to it: we need to combine property injection with PerResolveLifetimeManager:

[Test]
public void BuildObjectGraphWithCircularDependency()
{
    var container = new UnityContainer();

    container.RegisterType<IChicken, Chicken>(new PerResolveLifetimeManager(), new InjectionProperty("Egg"));
    container.RegisterType<IEgg, Egg>();

    var chicken = container.Resolve<IChicken>();

    Assert.AreSame(chicken, chicken.Egg.Chicken);
}

The magic above works as follows: container instantiates an instance of the Chicken class and then tries to resolve the Egg dependency. Egg in turn requires another instance of Chicken, luckily the Chicken class has been registered with PerResolveLifetimeManager which means it that instance of it can be reused in building the Egg. I think the case above is the only one I can think of where the use of property injection could be justified.

 

IoC and Interface segregation principle

There are some cases where a single type implements multiple contracts (interfaces). If you follow interface segregation principle this may lead to some troublesome cases:

[Test]
public void Resolve_ObjectDependantOnBaseInterface_ResolutionFails()
{
    var container = new UnityContainer();
    
    container.RegisterType<IDerived, Derived>();
    container.RegisterType<IDependsOnBase, DependsOnBase>();

    Assert.Throws<ResolutionFailedException>(() => container.Resolve<IDependsOnBase>());
}

public interface IBase { }
internal interface IDerived : IBase { }
internal class Derived : IDerived { }

public interface IDependsOnBase { }

public class DependsOnBase : IDependsOnBase
{
     public DependsOnBase(IBase dependency){}
}

As you can see from the code above, Derived implements IDerived which in turn is an instance of IBase. Inspite of that, resolution of DependsOnBase fails miserably. The reason is obviously the fact that there is no explicit implementation of IBase registered with the container. We could circumvent the problem by registering the Derived as type implementing the IBase interface. This approach would work but what if Derived implements more than one interface? There is however an easy way to work around the issue and this requires a hint to the container that instead of IBase it should search for an implementation of IDerived:

[Test]
public void Resolve_ObjectDependantOnBaseInterface_ResolutionSucceeds()
{
    var container = new UnityContainer();

    container.RegisterType<IDerived, Derived>();
    container.RegisterType<IDependsOnBase, DependsOnBase>(new InjectionConstructor(typeof(IDerived)));

    Assert.DoesNotThrow(() => container.Resolve<IDependsOnBase>());
}

This way btw it is the way to construct a composite using IEnumerable<> instead of an array as mentioned previously.

Overriding constructor parameters

The final set of Unity tricks deals with overriding constructor parameters. As you may remember we did this to a certain extent when registering individual elements of the chain of responsibility. There are two ways to do it: we can either instruct container at the time of registration to use specific instance of the parameters, or override parameter value at the time of resolution. Following sample illustrates both techniques.

[TestFixture]
public class ConstructorParameters
{
    [Test]
    public void OverridingCtorParams()
    {
        var container = new UnityContainer();
        container.RegisterType<SomeType>(new InjectionConstructor("Boo"));
        
        var firstInstance = container.Resolve<SomeType>();
        Assert.AreEqual("Boo", firstInstance.Message);

        var secondInstance = container.Resolve<SomeType>( new ParameterOverride("message", "Baa"));
        Assert.AreEqual("Baa", secondInstance.Message);
    }
}

public class SomeType
{
    public string Message { get; set; }

    public SomeType(string message)
    {
        Message = message;
    }
}
April 14 2012

How to focus on the wrong thing when writing code

A recent discussion on our mailing list revolved around the use of regions, and whether stripping them out of a codebase was a useful first task when joining a new project!  The conclusion was, sensibly, that there were more likely bigger fish to fry (especially since region outlining can easily be disabled in your IDE) and so regions should be left alone.  Along the way, however, someone voiced an opinion that the software devs have a professional responsibility to adhere to existing standards.

Consistency in code is a great aid to readability – no doubt about it – but often someone needs to be the first to write a unit test, for example.  The benefit that comes about from automated testing massively outweighs any benefit from uniform code, by many orders of magnitude.  Similar arguments apply to use of IOC, good design of code, proper consideration of class / method / variable names, refactoring, SOLID principles and so on.  All of these things are simply more important than whether you group all your methods into a region named “Methods” or not.  And unfortunately, none of these items can be adequately codified into standards – it’s simply not possible.

Curiously, coding standards are so mechanical that they can actually be captured by software and fixes can be automatically applied.  These are the only coding standards that I personally ever bother to try and apply on a project.  I want development tools to be worrying about capitalisation, the presence or absence of underscores at the start of variable names, positioning of braces, and so on.  I do not want developers to concern themselves with such issues – the time and brain power of a good developer is simply too valuable to waste on such niceties.

Adherence to coding standards is just like tidying the decks of a ship.  It’s a good thing to do … just make sure that you aren’t on a Titanic that is slowly sinking into the deep!

Bob the builder

I recently started a new project which is very focused on having good test coverage which is great goal to have.  However, looking through their tests I started to notice a lot of setup code which was very similar and looked a lot like the following:

   1: [TestMethod]
   2: public void WhenTheAddressRepositoryIsAskedToAPersistAddressesItShouldCorrectlyPersistThoseAddresses()
   3: {
   4:     // Arrange
   5:     var addressesToPersist = new List<Address>();
   6:  
   7:     var address = new Address
   8:                           {
   9:                               AddressLine1 = Guid.NewGuid().ToString(),
  10:                               AddressLine2 = Guid.NewGuid().ToString(),
  11:                               AddressLine3 = Guid.NewGuid().ToString(),
  12:                               AddressLine4 = Guid.NewGuid().ToString(),
  13:                               City = Guid.NewGuid().ToString(),
  14:                               County = Guid.NewGuid().ToString(),
  15:                               CountryCode = Guid.NewGuid().ToString(),
  16:                               Postcode = Guid.NewGuid().ToString()
  17:                           };
  18:     addressesToPersist.Add(address);
  19:  
  20:     var address1 = new Address
  21:                            {
  22:                                AddressLine1 = Guid.NewGuid().ToString(),
  23:                                AddressLine2 = Guid.NewGuid().ToString(),
  24:                                AddressLine3 = Guid.NewGuid().ToString(),
  25:                                AddressLine4 = Guid.NewGuid().ToString(),
  26:                                City = Guid.NewGuid().ToString(),
  27:                                County = Guid.NewGuid().ToString(),
  28:                                CountryCode = Guid.NewGuid().ToString(),
  29:                                Postcode = Guid.NewGuid().ToString()
  30:                            };
  31:     addressesToPersist.Add(address1);
  32:  
  33:     var addressRepository = new AddressRepository();
  34:     
  35:     // Act
  36:     addressRepository.Persist(addressesToPersist);
  37:     var actualPersistedAddresses = addressRepository.GetAll();
  38:     
  39:     // Assert
  40:     Assert.IsNotNull(actualPersistedAddresses);
  41:     Assert.AreEqual(2, actualPersistedAddresses.Count);
  42: }

Now the above code isn’t all that bad in a single test but what happens when you start to write more tests?  Well you could copy and paste the code for creating addresses from test to test which will certainly work but you end up with a lot of very similar code which you have to maintain going forward and update whenever a new address property is added or removed.  All this sounds like a lot of work plus it goes against the DRY principle.  DRY for those of you who don’t know means “Don’t Repeat Yourself”.  The principle encourages that duplication within an application is removed as every line of  code written has to be maintained and each line of code can cause bugs. More importantly the principle is about having a single source of knowledge within a system and then using that source to generate instances of that knowledge. In this case the instances of knowledge would be the address objects which are being created within each unit test.  The question then is how do we create something which we can use as the source of that knowledge?

Enter the builder

That single source of knowledge within the project I’m working on is provided by using the object builder pattern. Using this pattern we can have a single and consistent approach to creating the entities within tests and the ability to easily extend those entities within our domain and test data. We can also chain the builders together to create complex test data very quickly.

So how do you create a builder? Well below is my implementation of an address builder to get you going.

   1: public class AddressBuilder : BuilderBase<AddressBuilder, Address>
   2: {
   3:     private int id;
   4:     private string addressLine1 = GetUniqueValueFor();
   5:     private string addressLine2 = GetUniqueValueFor();
   6:     private string addressLine3 = GetUniqueValueFor();
   7:     private string addressLine4 = GetUniqueValueFor();
   8:     private string city = GetUniqueValueFor();
   9:     private string countryCode = "GB";
  10:     private string postCode = "SE1 9EU";
  11:     private string county = "London";
  12:  
  13:     public AddressBuilder WithId(int id)
  14:     {
  15:         this.id = id;
  16:         return this;
  17:     }
  18:    
  19:     public AddressBuilder WithAddressLine1(string addressLine1)
  20:     {
  21:         this.addressLine1 = addressLine1;
  22:         return this;
  23:     }
  24:  
  25:     public AddressBuilder WithAddressLine2(string addressLine2)
  26:     {
  27:         this.addressLine2 = addressLine2;
  28:         return this;
  29:     }
  30:  
  31:     public AddressBuilder WithAddressLine3(string addressLine3)
  32:     {
  33:         this.addressLine3 = addressLine3;
  34:         return this;
  35:     }
  36:  
  37:     public AddressBuilder WithAddressLine4(string addressLine4)
  38:     {
  39:         this.addressLine4 = addressLine4;
  40:         return this;
  41:     }
  42:  
  43:     public AddressBuilder WithCity(string city)
  44:     {
  45:         this.city = city;
  46:         return this;
  47:     }
  48:  
  49:     public AddressBuilder WithCountry(string countryCode)
  50:     {
  51:         this.countryCode = countryCode;
  52:         return this;
  53:     }
  54:  
  55:     public AddressBuilder WithPostCode(string postCode)
  56:     {
  57:         this.postCode = postCode;
  58:         return this;
  59:     }
  60:  
  61:     public AddressBuilder WithCounty(string county)
  62:     {
  63:         this.county = county;
  64:         return this;
  65:     }
  66:  
  67:     public override Address Build()
  68:     {
  69:         var address = new Address
  70:                           {
  71:                               Id = this.id,
  72:                               AddressLine1 = this.addressLine1,
  73:                               AddressLine2 = this.addressLine2,
  74:                               AddressLine3 = this.addressLine3,
  75:                               AddressLine4 = this.addressLine4,
  76:                               City = this.city,
  77:                               CountryCode = this.countryCode,
  78:                               County = this.county,
  79:                               Postcode = this.postCode
  80:                           };
  81:  
  82:         return address;
  83:     }
  84: }

As you can see the code behind the builder is really very simple with default values assigned to the properties which can then be explicitly set using the “With” methods on the builder. When I’m using builders I also like to add a base for them which provides default behaviour.

   1: public abstract class BuilderBase<TBuilder, TBuild> where TBuilder : BuilderBase<TBuilder, TBuild>
   2: {
   3:     /// <summary>
   4:     /// Builds this instance.
   5:     /// </summary>
   6:     /// <returns></returns>
   7:     public abstract TBuild Build();
   8:  
   9:     /// <summary>
  10:     /// Builds the many.
  11:     /// </summary>
  12:     /// <param name="number">The number.</param>
  13:     /// <returns></returns>
  14:     public List<TBuild> BuildMany(int number)
  15:     {
  16:         var result = new List<TBuild>();
  17:  
  18:         for (int i = 0; i < number; i++)
  19:         {
  20:             var builtItem = this.Build();
  21:             result.Add(builtItem);
  22:         }
  23:  
  24:         return result;
  25:     }
  26:  
  27:     /// <summary>
  28:     /// Gets the unique value for.
  29:     /// </summary>
  30:     /// <returns></returns>
  31:     protected static string GetUniqueValueFor()
  32:     {
  33:         return Guid.NewGuid().ToString();
  34:     }
  35: }

So now we have the address builder this put it all together to create our new and improved unit test.

   1: [TestMethod]
   2: public void WhenTheAddressRepositoryIsAskedToAPersistAddressesItShouldCorrectlyPersistThoseAddresses()
   3: {
   4:     // Arrange
   5:     var addressesToPersist = new AddressBuilder().BuildMany(2);
   6:     var addressRepository = new AddressRepository();
   7:     
   8:     // Act
   9:     addressRepository.Persist(addressesToPersist);
  10:     var actualPersistedAddresses = addressRepository.GetAll();
  11:     
  12:     // Assert
  13:     Assert.IsNotNull(actualPersistedAddresses);
  14:     Assert.AreEqual(2, actualPersistedAddresses.Count);
  15: }

As you can see the duplicate code for creating each address is now gone and we have a nice simple method for creating as many address as we want using the “BuildMany” method provided by the base builder. Well that’s it for now. The complete source code for this example can be found at http://cid-468e9f9e14e99f80.office.live.com/self.aspx/.Public/Blog.Builders.zip

Older Posts