Adding additional attributes to a fake of an already faked type fails silently #436

Closed
cmerat opened this Issue Jan 13, 2015 · 16 comments

Comments

Projects
None yet
4 participants
@cmerat
Contributor

cmerat commented Jan 13, 2015

In this example:

var constructor = typeof(BarAttribute).GetConstructor(new[] { typeof(string) });

var fake1 = A.Fake<Foo>(x => x.WithAdditionalAttributes(new[] { new CustomAttributeBuilder(constructor, new object[] { "1" }) }));
var fake2 = A.Fake<Foo>(x => x.WithAdditionalAttributes(new[] { new CustomAttributeBuilder(constructor, new object[] { "2" }) }));
var fake3 = A.Fake<Foo>(x => x.WithAdditionalAttributes(new[] { new CustomAttributeBuilder(constructor, new object[] { "3" }) }));

Assert.AreEqual("3", fake3.GetType().GetCustomAttributes(typeof(BarAttribute), true).Cast<BarAttribute>().First().Value);

This test will fail because the attribute with the value of "1" is returned although I'd expect it to return the attribute with value "3" (since I'm checking fake3). This appears to be due to all fakes inheriting from the same type (Castle.Proxies.FooProxy, DynamicProxyGenAssembly2, DynamicProxyGenAssembly2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=a621a9e7e5c32e69).

The solution could be that when custom attributes are specified, another type name is created (FooProxy1, FooProxy2 and so on). I am not sure how easy this is to do using DynamicProxy or if it is a limitation but the current behavior feels like a bug.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 13, 2015

Member

@cmerat can you confirm how many instances of BarAttribute the type of each fake has after the first 3 lines are run? Do they all have all three instances? This is what I would expect, since all three fakes are of the same type, and any custom attributes are being applied to that type.

As you suggest, the only way of having different attributes for the type of each fake would be for each fake be of a different type (it's nothing to do with inheritance, it's just the type of the fake). I'm also unsure how feasible this is.

Member

adamralph commented Jan 13, 2015

@cmerat can you confirm how many instances of BarAttribute the type of each fake has after the first 3 lines are run? Do they all have all three instances? This is what I would expect, since all three fakes are of the same type, and any custom attributes are being applied to that type.

As you suggest, the only way of having different attributes for the type of each fake would be for each fake be of a different type (it's nothing to do with inheritance, it's just the type of the fake). I'm also unsure how feasible this is.

@cmerat

This comment has been minimized.

Show comment
Hide comment
@cmerat

cmerat Jan 13, 2015

Contributor

@adamralph no, only a single value is available (I've tried with both AllowMultiple set to true and false -- no difference). It appears that the first Fake to "create" the proxy type gets the attribute assignments and the type then lives for the entire duration of the AppDomain (which means it also impacts other tests).

Contributor

cmerat commented Jan 13, 2015

@adamralph no, only a single value is available (I've tried with both AllowMultiple set to true and false -- no difference). It appears that the first Fake to "create" the proxy type gets the attribute assignments and the type then lives for the entire duration of the AppDomain (which means it also impacts other tests).

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 13, 2015

Member

Ah, I see. I guess that also makes sense since once a type is instantiated in an AppDomain, it's definition can't be altered, which means the FakeItEasy API is really lying about it's capabilities in this case, since it only creates the proxy type once for each type that is being faked.

So I guess the only way to support this is to create a different type for each fake. This will need some investigation. The ability to add custom attributes is quite an old feature added in #20 and is, in fact, not even documented in our wiki. At this point, I would favour documenting the feature and stating this restriction, rather than calling this a bug. Indeed, it may not even be possible to enhance the feature so that each fake has it's own type with its own attributes. AFAIK no other .NET mocking framework even offers a custom attribute feature so at least what is there is already something extra, rather than a feature gap.

Having said that, the fact that the API fails to fulfill its promise silently, could indeed be considered a bug. It would be better for FakeItEasy to throw an exception if a fake is being created for a type with custom attributes when a fake of that type has already been created. That way at least the limitation is communicated to the test author rather than misleading behaviour which can only be identified through forensics.

In any case, thanks for bringing this to our attention @cmerat!

Member

adamralph commented Jan 13, 2015

Ah, I see. I guess that also makes sense since once a type is instantiated in an AppDomain, it's definition can't be altered, which means the FakeItEasy API is really lying about it's capabilities in this case, since it only creates the proxy type once for each type that is being faked.

So I guess the only way to support this is to create a different type for each fake. This will need some investigation. The ability to add custom attributes is quite an old feature added in #20 and is, in fact, not even documented in our wiki. At this point, I would favour documenting the feature and stating this restriction, rather than calling this a bug. Indeed, it may not even be possible to enhance the feature so that each fake has it's own type with its own attributes. AFAIK no other .NET mocking framework even offers a custom attribute feature so at least what is there is already something extra, rather than a feature gap.

Having said that, the fact that the API fails to fulfill its promise silently, could indeed be considered a bug. It would be better for FakeItEasy to throw an exception if a fake is being created for a type with custom attributes when a fake of that type has already been created. That way at least the limitation is communicated to the test author rather than misleading behaviour which can only be identified through forensics.

In any case, thanks for bringing this to our attention @cmerat!

@adamralph adamralph changed the title from When creating fakes using WithAdditionalAttributes, only the last specified attributes are applied to all instances of the type to Add additional attributes to each fake independently Jan 13, 2015

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 13, 2015

Member

once a type is instantiated in an AppDomain, it's definition can't be altered

That of course may not be strictly true since Castle may well provide some magic to do it, but it's a moot point anyway, since we don't want to simply add the additional attributes from the creation of fake2 onto those added from the creation of fake1. They need to have independent lists of attributes, so I think different types for each fake is the only way to do this.

Member

adamralph commented Jan 13, 2015

once a type is instantiated in an AppDomain, it's definition can't be altered

That of course may not be strictly true since Castle may well provide some magic to do it, but it's a moot point anyway, since we don't want to simply add the additional attributes from the creation of fake2 onto those added from the creation of fake1. They need to have independent lists of attributes, so I think different types for each fake is the only way to do this.

@cmerat

This comment has been minimized.

Show comment
Hide comment
@cmerat

cmerat Jan 13, 2015

Contributor

After a bit of digging in the Castle.DynamicProxy.Generators.BaseProxyGenerator class:

protected Type ObtainProxyType(CacheKey cacheKey, Func<string, INamingScope, Type> factory)
{
    using (var locker = Scope.Lock.ForReadingUpgradeable())
    {
        var cacheType = GetFromCache(cacheKey);
        if (cacheType != null)
        {
            Logger.DebugFormat("Found cached proxy type {0} for target type {1}.", cacheType.FullName, targetType.FullName);
            return cacheType;
        }

        // Upgrade the lock to a write lock, then read again. This is to avoid generating duplicate types
        // under heavy multithreaded load.
        locker.Upgrade();

        cacheType = GetFromCache(cacheKey);
        if (cacheType != null)
        {
            Logger.DebugFormat("Found cached proxy type {0} for target type {1}.", cacheType.FullName, targetType.FullName);
            return cacheType;
        }

        // Log details about the cache miss
        Logger.DebugFormat("No cached proxy type was found for target type {0}.", targetType.FullName);
        EnsureOptionsOverrideEqualsAndGetHashCode(ProxyGenerationOptions);

        var name = Scope.NamingScope.GetUniqueName("Castle.Proxies." + targetType.Name + "Proxy");
        var proxyType = factory.Invoke(name, Scope.NamingScope.SafeSubScope());

        AddToCache(cacheKey, proxyType);
        return proxyType;
    }
}

This is actually a "bug" due to how Castle.DynamicProxy manages its type cache. It uses a CacheKey that is based on the ProxyGenerationOptions but the AdditionalAttributes are NOT taken into account when calculating HashCode and Equality. This is causing the behavior that we see here. Had they been taken into account, the would have been a cache miss and a brand new type would have been generated (essentially, exactly the behavior we want).

There are a few items that are used to generate the cache key:

  • The type we are faking
  • The interfaces being implemented
  • The ProxyGenerationOptions

Within the ProxyGenerationOptions, the only "variable" element that we could influence to alter equality would be the Mixins collection. Since Mixins are given to add implementation behavior for interfaces, there could only be a single mixin per interface. I don't see how this could be used to our benefit.

The other option is the interfaces to implement. We would need to generate an (empty) interface type at runtime and add it to the list of interfaces to implement. This would "trick" DyanmicProxy into a cache miss and it will then generate a brand new type, applying the correct AdditionalAttributes.

I believe this would be the easiest approach to get the desired behavior (short of Castle fixing their Equality algorithm to take into account AdditionalAttributes, obviously). I'm not sure what dynamic interface generation at runtime requires (likely a fair amount of Emitting) but I'm guessing it would be fairly simple to do.

Contributor

cmerat commented Jan 13, 2015

After a bit of digging in the Castle.DynamicProxy.Generators.BaseProxyGenerator class:

protected Type ObtainProxyType(CacheKey cacheKey, Func<string, INamingScope, Type> factory)
{
    using (var locker = Scope.Lock.ForReadingUpgradeable())
    {
        var cacheType = GetFromCache(cacheKey);
        if (cacheType != null)
        {
            Logger.DebugFormat("Found cached proxy type {0} for target type {1}.", cacheType.FullName, targetType.FullName);
            return cacheType;
        }

        // Upgrade the lock to a write lock, then read again. This is to avoid generating duplicate types
        // under heavy multithreaded load.
        locker.Upgrade();

        cacheType = GetFromCache(cacheKey);
        if (cacheType != null)
        {
            Logger.DebugFormat("Found cached proxy type {0} for target type {1}.", cacheType.FullName, targetType.FullName);
            return cacheType;
        }

        // Log details about the cache miss
        Logger.DebugFormat("No cached proxy type was found for target type {0}.", targetType.FullName);
        EnsureOptionsOverrideEqualsAndGetHashCode(ProxyGenerationOptions);

        var name = Scope.NamingScope.GetUniqueName("Castle.Proxies." + targetType.Name + "Proxy");
        var proxyType = factory.Invoke(name, Scope.NamingScope.SafeSubScope());

        AddToCache(cacheKey, proxyType);
        return proxyType;
    }
}

This is actually a "bug" due to how Castle.DynamicProxy manages its type cache. It uses a CacheKey that is based on the ProxyGenerationOptions but the AdditionalAttributes are NOT taken into account when calculating HashCode and Equality. This is causing the behavior that we see here. Had they been taken into account, the would have been a cache miss and a brand new type would have been generated (essentially, exactly the behavior we want).

There are a few items that are used to generate the cache key:

  • The type we are faking
  • The interfaces being implemented
  • The ProxyGenerationOptions

Within the ProxyGenerationOptions, the only "variable" element that we could influence to alter equality would be the Mixins collection. Since Mixins are given to add implementation behavior for interfaces, there could only be a single mixin per interface. I don't see how this could be used to our benefit.

The other option is the interfaces to implement. We would need to generate an (empty) interface type at runtime and add it to the list of interfaces to implement. This would "trick" DyanmicProxy into a cache miss and it will then generate a brand new type, applying the correct AdditionalAttributes.

I believe this would be the easiest approach to get the desired behavior (short of Castle fixing their Equality algorithm to take into account AdditionalAttributes, obviously). I'm not sure what dynamic interface generation at runtime requires (likely a fair amount of Emitting) but I'm guessing it would be fairly simple to do.

@cmerat

This comment has been minimized.

Show comment
Hide comment
@cmerat

cmerat Jan 13, 2015

Contributor

And here's the bit of code required to generate a dynamic interface at runtime:

var asmName = new AssemblyName("AdditionalAttributesInterfaces");
var asmBuild = Thread.GetDomain().DefineDynamicAssembly(asmName, AssemblyBuilderAccess.Run);
var modBuild = asmBuild.DefineDynamicModule("AdditionalAttributes");

var type = modBuild.DefineType("AdditionalAttributes_" + Guid.NewGuid().ToString("N"), TypeAttributes.Interface | TypeAttributes.Abstract | TypeAttributes.Public).CreateType();

This generates an empty interface that can then be added to the proxy generation to create a unique key. I'll see if I can easily add this behavior to FakeItEasy on a feature branch.

Contributor

cmerat commented Jan 13, 2015

And here's the bit of code required to generate a dynamic interface at runtime:

var asmName = new AssemblyName("AdditionalAttributesInterfaces");
var asmBuild = Thread.GetDomain().DefineDynamicAssembly(asmName, AssemblyBuilderAccess.Run);
var modBuild = asmBuild.DefineDynamicModule("AdditionalAttributes");

var type = modBuild.DefineType("AdditionalAttributes_" + Guid.NewGuid().ToString("N"), TypeAttributes.Interface | TypeAttributes.Abstract | TypeAttributes.Public).CreateType();

This generates an empty interface that can then be added to the proxy generation to create a unique key. I'll see if I can easily add this behavior to FakeItEasy on a feature branch.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 13, 2015

Member

Great investigative work @cmerat!

I propose that you raise an issue for this over at https://github.com/castleproject/Core/issues since you clearly have a good handle on the problem. If we can get this fixed in castle that would be best. We've had fixes put into Castle.Core in the past.

Member

adamralph commented Jan 13, 2015

Great investigative work @cmerat!

I propose that you raise an issue for this over at https://github.com/castleproject/Core/issues since you clearly have a good handle on the problem. If we can get this fixed in castle that would be best. We've had fixes put into Castle.Core in the past.

@cmerat

This comment has been minimized.

Show comment
Hide comment
@cmerat

cmerat Jan 13, 2015

Contributor

I've posted the issue there (castleproject/Core#77) and am currently working on a fix that can be used in the meanwhile. The amount of code required is very small. Depending on the reception on the Castle Project issue, I'll send a pull request so that FakeItEasy can work around this limitation.

Contributor

cmerat commented Jan 13, 2015

I've posted the issue there (castleproject/Core#77) and am currently working on a fix that can be used in the meanwhile. The amount of code required is very small. Depending on the reception on the Castle Project issue, I'll send a pull request so that FakeItEasy can work around this limitation.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 15, 2015

Member

It appears that Castle are willing to push out a fix for this (castleproject/Core#77 (comment)), so the resolution of this issue will be an upgrade to that version of Castle.

Let's hold off on the workaround. If everything goes as hoped in Castle then we won't need it.

Member

adamralph commented Jan 15, 2015

It appears that Castle are willing to push out a fix for this (castleproject/Core#77 (comment)), so the resolution of this issue will be an upgrade to that version of Castle.

Let's hold off on the workaround. If everything goes as hoped in Castle then we won't need it.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Mar 17, 2015

Member

The Castle PR has been merged castleproject/Core#78

Member

adamralph commented Mar 17, 2015

The Castle PR has been merged castleproject/Core#78

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad May 31, 2016

Member

If I read things right, castleproject/Core#78 is available in the 4.0.0-alpha001 release, but I prefer not to take an alpha build. Since the alpha was released mostly to give people a chance to play Castle.Core under .NET Core (before the latest RC change), I don't think a production 4.0.0 is coming soon.

Member

blairconrad commented May 31, 2016

If I read things right, castleproject/Core#78 is available in the 4.0.0-alpha001 release, but I prefer not to take an alpha build. Since the alpha was released mostly to give people a chance to play Castle.Core under .NET Core (before the latest RC change), I don't think a production 4.0.0 is coming soon.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph May 31, 2016

Member

Agreed, this will have to wait for Castle.Core 4.0.0 stable to be released.

It's a shame that that seems to be coupled to .NET core.

Member

adamralph commented May 31, 2016

Agreed, this will have to wait for Castle.Core 4.0.0 stable to be released.

It's a shame that that seems to be coupled to .NET core.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad May 31, 2016

Member

I think the owner(s) just decided not to release 4.0 until it can include .NET Core (and to be fair, time to availability of the latter has not be monotonically decreasing, so the decision probably looked better then than it does now). I don't think our issue is a big enough deal to justify harassing them (him) to change the plan…

Member

blairconrad commented May 31, 2016

I think the owner(s) just decided not to release 4.0 until it can include .NET Core (and to be fair, time to availability of the latter has not be monotonically decreasing, so the decision probably looked better then than it does now). I don't think our issue is a big enough deal to justify harassing them (him) to change the plan…

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Nov 14, 2016

Member

Assigning to @thomaslevesque, because I think he's interested. @thomaslevesque, if not, throw it back in the pool!

Member

blairconrad commented Nov 14, 2016

Assigning to @thomaslevesque, because I think he's interested. @thomaslevesque, if not, throw it back in the pool!

@thomaslevesque

This comment has been minimized.

Show comment
Hide comment
@thomaslevesque

thomaslevesque Nov 14, 2016

Member

Already started ;)

Member

thomaslevesque commented Nov 14, 2016

Already started ;)

@adamralph adamralph changed the title from Add additional attributes to each fake independently to Adding additional attributes to a fake of an already faked type fails silently Nov 14, 2016

@adamralph adamralph closed this in #900 Nov 15, 2016

@adamralph adamralph added this to the 3.0.0 milestone Nov 15, 2016

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Feb 22, 2017

Member

This issue has been released in FakeItEasy 3.0.0:
https://github.com/FakeItEasy/FakeItEasy/releases/3.0.0

Thanks, @cmerat. Look for your name in the release notes! 🏆

Member

blairconrad commented Feb 22, 2017

This issue has been released in FakeItEasy 3.0.0:
https://github.com/FakeItEasy/FakeItEasy/releases/3.0.0

Thanks, @cmerat. Look for your name in the release notes! 🏆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment