Handle protected and internal property setters

[zvirja]

Fixes #626

This change adds support for the non-public property setters. The original issue was about internal properties and this change handles them as well.

I didn't add unit test for internal properties as I didn't want to set assembly: InternalsVisibleTo() attribute for the test assembly to not affect the existing tests.

Please kindly take a look and give your feedback! Thanks!

[dtchepak]

Hi @zvirja ! Looks good 👍

One question about breaking change -- how big a risk of people accidentally calling code they do not expect to run do you think this is? For example, if they were relying on internal not being updated?

I don't think it would be a common problem, but wanted to get your thoughts on it in case I am just lacking the imagination to see an issue.

[dtchepak]

I was playing around with this and added these members:

            public virtual int OtherProp { protected get; set; }
            public int GetOtherProp() => OtherProp;

And this test:

        [Test]
        public void SetUpdatesProtectedGetter() {
            var subs = Substitute.For<TestClass>();

            subs.OtherProp = 21;

            var result = subs.GetOtherProp();

            Assert.That(result, Is.EqualTo(21));
        }

Not sure if that is a helpful example to include or not.

I did notice that if it was private get instead it did not work. I thought NonPublic would also match the private get?

[zvirja]

Thanks for the idea of Prop { protected get; set; } - added it as well 👍

public virtual int OtherProp { private get; set; }

As for the property with private getter, we cannot override and intercept the getter here. When proxy is created, we intercept a setter only. Therefore, no matter what we assign - it will be never possible to read it.

The same actually happens with the property like this:

public virtual int OtherProp { get; private set; }

Here you can override a getter only.

If you create a substitute, it will be very broken property functionality-wise, as it will "void" all the assigned values.

Notice that the scenarios above are only applicable to the partial mocks, which are not that frequent. For interface mocks which are way more popular those scenarios are not applicable.

[zvirja]

One question about breaking change -- how big a risk of people accidentally calling code they do not expect to run do you think this is? For example, if they were relying on internal not being updated?

As for the interfaces, I think there should be no breaking changes at all. Interfaces allow you to have internal modifier only. Even before it was not possible to activate interface substitute without InternalsVisibleTo() attribute. After the change we just made those properties behave as expected.

As for the classes, more I think more about it, more I believe that we are safe. This PR does not change what is intercepted - whatever was captured by DynamicProxy lib, is still captured. It's just that previously we simply ignored those set property values and now we also start handling corresponding accessors.

The only problematic case I can think of is the following:

public virtual int Prop { get; private set; }
public virtual int Prop { private get; set; }

// Or if you don't have `InternalsVisibleTo` attribute
public virtual int Prop { get; internal set; }
public virtual int Prop { internal get; set; }

If regular substitute was created (using Substitute.For()), it was already broken, as we intercepted public accessors only and never called base.
If a partial substitute was created (using Substitute.ForPartsOf()) - change makes no difference. If we handle setter, then we still call base and never intercept getter - nothing changed. If we handle getter and setter is not available to us - we still return base value, as we have no configured results for the properties; i.e. nothing changed.

Looks like our change only normalizes the expected property behavior and as for the weird cases - they were already broken before this change, so should be not a concern.

It's just my analysis, I might overlook something as well 😊

[dtchepak]

Thanks a lot for this. 👍

Happy for this to be merged if you are. 😄

[zvirja]

@dtchepak Done! Consider dropping a new version if we don't plan fix anything else in the nearby future 😉