umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Proxy class properties accessing object instance properties #188

Closed leekelleher closed 7 years ago

leekelleher commented 8 years ago

Added failing unit-test for issue #177

When using proxy classes, it appears that lazy-loaded properties can not access the property values of its object instance.

@JimBobSquarePants to review (in due course - no rush, just wanted to keep track of this) :sunglasses:

mattbrailsford commented 8 years ago

Just to note what lee and I observed when we reviewed this (to the best of our understanding), it looks like an IProxy object is made per property that only contains that individual property, so if one property accesses another, it doesn't exist on the IProxy, so it fails.

JimBobSquarePants commented 8 years ago

@mattbrailsford No, that's not correct at all. An IProxy is created per instance.

https://github.com/leekelleher/umbraco-ditto/blob/develop/src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs#L362

Basically if you have a class like this:

public class Foo {
    public virtual string Bar {get; set;}
}

The proxy generator will create a class like this:

public class FooProxy : Foo, IProxy {

    public IInterceptor Interceptor { get; set; }

    public override string Bar {
        get {
            return (string) interceptor.Intercept(methodof(Foo.get_Property), null);
        }
        set {
            interceptor.Intercept(methodof(Foo.set_Property), value);
        }
}

We set the interceptor containing all the virtual properties here. It also reference the current instance.

In the example code the lazy property seems to be getting called against the base class and not the proxy class but Ill have to dig around to be sure. Now we have a failing test I should easily be able to do so.

It really is an odd design pattern though. Why have to properties referencing the same object? If they want to convert it to a string they should be using a method or casting their property. They're hurting performance and making their code difficult to follow.

mattbrailsford commented 8 years ago

Is this still relevant with PR #191?

JimBobSquarePants commented 8 years ago

TBH I don't know. Will have to conduct further tests after we merge.

leekelleher commented 7 years ago

Merged in and tested locally... the failing unit-test (for issue #177) now passes.

Merged in this PR - we can decide whether or not to keep the actual unit-test, or to combine with another unit-test (from PR #190)?

JimBobSquarePants commented 7 years ago

I'd move the tests together. still want to have a dig around though.