Odd behaviour with `StackExchange.Redis` types

[dtchepak]

Describe the bug

NSubstitute behaving oddly with StackExchange.Redis library. Ilya Chernomordik on StackOverflow reports a TypeLoadException in DynamicProxy-generated RedisResultProxy. This type isn't explicitly configured, I'm guessing it is from a default auto-subbed value during stubbing.

From Ilya Chernomordik's question:

System.TypeLoadException: Method 'AsBoolean' in type 'Castle.Proxies.RedisResultProxy' from assembly 'DynamicProxyGenAssembly2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=a621a9e7e5c32e69' does not have an implementation.

This occurs with StackExchange.Redis.2.0.601, but not with StackExchange.Redis.2.1.0-preview.23.

To Reproduce

From Nkosi on SO (requires StackExchange.Redis package):

public interface IRedisClient
{
    Task<RedisResult> ScriptEvaluate(LuaScript script, object parameters);
}

public class Foo {
    private readonly IRedisClient _client;

    public Foo(IRedisClient client) {
        _client = client;
    }

    public Task<RedisResult> RunScript() {
        return _client.ScriptEvaluate(LuaScript.Prepare(""), new object());
    }
}

[TestClass]
public class MyTestClass {
    [TestMethod]
    public async Task Test1() {
        //Arrange
        var expected = RedisResult.Create((RedisKey)"result");
        var _client = Substitute.For<IRedisClient>();

        _client
            .ScriptEvaluate(Arg.Any<LuaScript>(), Arg.Any<object>())
            .Returns(expected);

        var _foo = new Foo(_client);

        //Act
        var actual = await _foo.RunScript();

        //Assert
        actual.Should().Be(expected);
    }

    [TestMethod]
    public async Task Test2() {
        //Arrange
        var expected = RedisResult.Create((RedisKey)"result");
        var _client = Mock.Of<IRedisClient>(_ => _.ScriptEvaluate(It.IsAny<LuaScript>(), It.IsAny<object>()) == Task.FromResult(expected));

        var _foo = new Foo(_client);

        //Act
        var actual = await _foo.RunScript();

        //Assert
        actual.Should().Be(expected);
    }
}

Environment:

• NSubstitute version: 4.2.1
• NSubstitute.Analyzers version: CSharp 1.0.12
• Platform: Mac dotnetcore 3.1, Win 10 dotnetcore 3.1.100
  • StackExchange.Redis.2.0.601 (problem does not occur with StackExchange.Redis.2.1.0-preview.23)

[dtchepak]

More information.

I have updated the question. It seems to me that being abstract class is not the issue here but perhaps the fact that there are a number of abstract internal methods (AsBoolean from my exception being one of them). My guess is that Castle proxy creates a dynamic implementation of RedisResult but it does not "see" the internal ones, and then when this type is being used the runtime figures out not everything is implemented

This type isn't explicitly proxied, so it's probably from the auto-subbed result. Maybe Configure() can fix this.

_client
  .Configure()
  .ScriptEvaluate(Arg.Any<LuaScript>(), Arg.Any<object>())
  .Returns(expected);

[nkosihenry]

@dtchepak After consulting the docs when I was assessing the issue I figured I'd try Configure() but I got the same issue with Configure() which is why I thought is was a bug as well.

[ilya-spv]

I have created a project from scratch with latest version of stackexchange redis and nsubstitute. And I got en error right away (using @nkosihenry example with only nsubstitute, I did run it right in console without testing framework or using Moq example), though interestingly enough it's a bit different error:

Unhandled exception. System.TypeLoadException: Type 'Castle.Proxies.ObjectProxy' from assembly 'DynamicProxyGenAssembly2, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' is attempting to implement an inaccessible interface.

"NSubstitute" Version="4.2.1" "StackExchange.Redis" Version="2.0.601"

.Net Core 3.1.100 on Windows 10.

Here is the bare minimum to get the error:

    public interface IRedisClient
    {
        Task<RedisResult> ScriptEvaluate(LuaScript script, object parameters);
    }

    public class Foo
    {
        public Foo(IRedisClient client)
        {
            client.ScriptEvaluate(LuaScript.Prepare(""), new object());
        }
    }

    static async Task Main(string[] args)
    {
        new Foo(Substitute.For<IRedisClient>());
    }

[dtchepak]

Thanks so much @ilya-spv !

I found with StackExchange.Redis.2.1.0-preview.23 this error does not occur. I was able to reproduce it with 2.0.601 (current stable, non-preview).

[dtchepak]

@nkosihenry Thanks for trying Configure and for taking the time to add to this issue. 👍 I can reproduce this now so I should have enough info to look into it.

[ilya-spv]

That's interesting, it does not seem that they took away internal abstract methods from RedisResult, so it might be something else after all. Thanks for checking this out @dtchepak

[zvirja]

Yes. Issue is exactly caused by the internal interface. The minimal scenario:

public interface IInterfaceWithInternal
{
    int GetResult();
    internal int InternalMethod();
}

static async Task Main(string[] args)
{
    Substitute.For<IInterfaceWithInternal>();
}

In the latest version they removed the internal methods, so it helped.

I believe there is nothing from our side we can do to solve the issue. Luckily, it has been already fixed in the latest stable.

@dtchepak Interestingly that .Configure() doesn't help to solve the issue. Even if you use it, NSub will still try to construct result of the method and fail. It happens because we create auto-value for configuration call. In its turn, we are doing that primarily to support this and this scenarios. It's a pity that there is no workaround for the issue :'( I remember we discussed that many times, but probably it worth reconsidering the priorities for v5 or just one day and trade-off support for recursive supports in favor of robustness 🤔

[dtchepak]

@zvirja Thanks for figuring this out! I notice the minimal case won't throw if we have InternalsVisibleTo dynamic proxy, but I'm guessing this does not help us much as normally the type will not be declared in our test assembly.

Would it help if we did any of the following?

• Detect type with internal members in Substitute.For<>, and throw a more useful exception. (or wrap that specific TypeLoadException with a friendlier message)
• Do not try to auto-sub for types with inaccessible members (alternatively, catch failed attempts to auto-sub and return default(T) instead?)
• Update Analyzer's NS1003 to catch this in Substitute.For.

[ilya-spv]

A more useful exception sounds always like a good idea to me. Though not trying to auto-sub in this edge case would be strange, since it does do that other places. I do not know all the internal workings of the nsubstitute, but perhaps a combination of a good exception message and some option to turn this auto-sub off? (with a hint in the exception perhaps) would do?

Analyzer seems like a very good idea as well, but if it would be possible to turn it off somehow it would be hard for analyzer to figure out (unless it is turned off right in the call)

[ilya-spv]

It is quite a questionable design I guess as well to have internal methods on interface. In my opinion it should rather be public interfaces and internal interfaces, with the latter ones never exposed to the user of the library. But C# apparently allows this anyway