Not proxied methods on class proxy with target should forward to the target [DYNPROXY-183]

[jonorossi]

Relogging YouTrack issue logged by Krzysztof 2 years ago.
http://issues.castleproject.org/youtrack/issue/DYNPROXY-183

---

Currently when we have a proxyable method on a class proxy with target, and chose not to proxy it, the default body of the method on the proxy object will be invoked (same behavior as inheritance-base class proxy).

The correct behavior should be to forward the call to the target object instead, like all the other "with target" proxy types are doing.

[kkozmic]

Ah yes.

[blairconrad]

Hey, Gents.

I thought I'd give this one a go, since it seemed straightforward (and to tell the truth, shocking), but I believe I've misunderstood the problem. I had thought from the description that this would be a failing test:

[Test]
public void Unproxied_method_delegates_to_target_method()
{
    var target = new ClassWithVirtualMethod("target");
    var proxy = generator.CreateClassProxyWithTarget(target);
    var result = proxy.VirtualMethod();
    Assert.AreEqual(result, "target");
}

public class ClassWithVirtualMethod
{
    private readonly string result;

    public ClassWithVirtualMethod():this("default")
    {
    }

    public ClassWithVirtualMethod(string result)
    {
        this.result = result;
    }

    public virtual string VirtualMethod()
    {
        return result;
    }
}

But it passes. I'm sure I've misunderstood something, but am not sure what. Perhaps there's a deeper meaning to "chose not to proxy [the method]" than I understand?

[mpipalia]

Hi Everyone,

I think this is what @jonorossi is trying to convey:

public class Foo
{
    private string _Bar;

    public virtual void SetBar(string value)
    {
        _Bar = value;
    }
    public virtual string GetBar()
    {
        return _Bar;
    }
}

public class OnlySetBar : IProxyGenerationHook
{
    public void MethodsInspected() { }
    public void NonProxyableMemberNotification(Type type, MemberInfo memberInfo) { }

    public bool ShouldInterceptMethod(Type type, MethodInfo methodInfo)
    {
        return methodInfo.Name == "SetBar";
    }
}

public class ToLowerInvariant : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        string value = (string)invocation.GetArgumentValue(0);
        string lowerCase = value?.ToLowerInvariant();
        invocation.SetArgumentValue(0, lowerCase);
        invocation.Proceed();
    }
}

public class Issue67
{
    [Fact]
    public void Unproxied_Methods_Should_Passthrough_To_Target()
    {
        ProxyGenerationOptions options = new ProxyGenerationOptions();
        options.Hook = new OnlySetBar();
        ProxyGenerator gen = new ProxyGenerator();
        Foo foo = new Foo();

        Foo proxy = gen.CreateClassProxyWithTarget<Foo>(foo, options, new ToLowerInvariant());
        proxy.SetBar("HELLO WORLD");

        Assert.Equal("hello world", foo.GetBar()); // Passes
        Assert.Equal("hello world", proxy.GetBar()); // Fails
    }
}

Here, we are only intercepting the SetBar method, and lower casing the value before it gets passed into the actual method. When we read bar by calling GetBar, we expect the lower case version of what we set. On the target instance, it works as expected because it is the target's SetBar method that is called. But, on the proxied instance, it fails because it is using its own GetBar method rather than using the target's. It ends up returning null because its own SetBar method is never called. The expectation is that calling proxy.GetBar() should actually call foo.GetBar() rather than using the existing implementation (returning _Bar).

When I first noticed this, I found it really strange too. I would have expected that CreateClassProxyWithTarget behaved just like CreateClassProxy, except that instead of DynamicProxy creating the object, I pass in an already created object. The WithTarget version is meant for cases where you aren't creating the object yourself (in my case it was a NetworkStream from TcpClient).

[blairconrad]

Wow, @mpipalia. I'd have never figured that out. Thanks. It seems like you know a lot about it. I'm currently occupied with other tasks, and have no particular attachment to this issue, so if you're interested in working on it, please do.

[kkozmic]

yup, what @mpipalia said. Thanks for clarifying it

[jonorossi]

@blairconrad sorry for not getting back to you on this in early January, but yep @mpipalia's test case is exactly it. @mpipalia it'd be great if you've got time to look into fixing it, feel free to ask for assistance.

[blairconrad]

@jonorossi, no worries. You've got stuff going on.

[jonorossi]

Since this is a long standing defect we'll be wrapping up v4.0 without.

[gentledepp]

any news on this?

[jonorossi]

@gentledepp no one is currently working on this issue, the reason it is marked as up for grabs. If you've got the time, we'd love to see a fix for it though.