PropertyChanged event not fired if underliying properties was modified by a virtual method

[toumir]

Hi,
Let add two methods to the order class, one virtual and the other not virtual:

public class Order
{
	//...
	
	public virtual void VirtualModifier()
	{
		CustomerNumber = "test";
	}
	public void NonVirtualModifier()
	{
		CustomerNumber = "test";
	}
	
	//...
}

and add two tests :

[Fact]
public void Change_Property_Should_Raise_PropertyChanged_Event_if_non_virtual_method()
{
	var order = Helper.GetOrder();
	var trackable = order.AsTrackable();
	((INotifyPropertyChanged)trackable).MonitorEvents();

	trackable.NonVirtualModifier(); 

	trackable.ShouldRaisePropertyChangeFor(o => o.CustomerNumber);
}
[Fact]
public void Change_Property_Should_Raise_PropertyChanged_Event_if_method_virtual()
{
	var order = Helper.GetOrder();
	var trackable = order.AsTrackable();
	((INotifyPropertyChanged)trackable).MonitorEvents();

	trackable.VirtualModifier();

	trackable.ShouldRaisePropertyChangeFor(o => o.CustomerNumber);
}

the second test fails and the PropertyChange event not fired, the reason is the VirtualModifier is virtual
can you explain why ?

thanks

[joelweiss]

Thanks for the brain teaser 😄

Fixed in #38

[joelweiss]

@toumir thanks for the report

here is the pseudo code that gets generated, this explains what happened.

void Main()
{
    Order order = new Order();
    TrackableOrder trackableOrder = new TrackableOrder(order);
    trackableOrder.PropertyChanged += (sender, e) => Console.WriteLine($"Property {e.PropertyName} Changed");

    Console.WriteLine("Calling NonVirtualModifier()");
    trackableOrder.NonVirtualModifier();

    Console.WriteLine("Calling VirtualModifier()");
    trackableOrder.VirtualModifier();
}

class Order
{
    public virtual string CustomerNumber { get; set; }

    public virtual void VirtualModifier()
    {
        CustomerNumber = "VirtualModifier";
    }

    public void NonVirtualModifier()
    {
        CustomerNumber = "NonVirtualModifier";
    }
}

class TrackableOrder : Order, INotifyPropertyChanged
{
    private readonly Order _Order;

    public TrackableOrder(Order order)
    {
        _Order = order;
    }

    public override string CustomerNumber
    {
        get => _Order.CustomerNumber;
        set
        {
            if (_Order.CustomerNumber != value)
            {
                _Order.CustomerNumber = value;
                PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Order.CustomerNumber)));
            }
        }
    }

    //this had to be eliminated.
    public override void VirtualModifier()
    {
        _Order.VirtualModifier();
    }

    public event PropertyChangedEventHandler PropertyChanged;
}

[mayphi]

Hi,
if for example the class Order has the two Methods SetName and GetName:

public class Order
{
	//...
	
        private string _name;

        public virtual void SetName(string name)
        {
            _name = name;
        }

        public virtual string GetName()
        {
            return _name;
        }
	
	//...
}

and a test like that exists:

        [Fact]
        public void GetName_Should_Return_Correct_Value()
        {
            // Arrange
            var order = Helper.GetOrder();
            order.SetName("MyName");

            var trackable = order.AsTrackable();

            // Act
            var name = trackable.GetName();

            // Assert
            Assert.Equal("MyName", name);
        }

then without _InstanceMethodsOnClass GetName returns "MyName" and the test succeds, but with _InstanceMethodsOnClass GetName returns null and the test fails, because for the Method GetName the Method ShouldInterceptMethod returns false.

public class ChangeTrackingProxyGenerationHook : IProxyGenerationHook
{
        //...

        public ChangeTrackingProxyGenerationHook(Type type)
        {
            _Type = type;

            _InstanceMethodsOnClass = new HashSet<MethodInfo>(type
               .GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly)
               .Where(mi => !mi.IsSpecialName));
        }

        public bool ShouldInterceptMethod(Type type, System.Reflection.MethodInfo methodInfo) => !_MethodsToSkip.Contains(methodInfo.Name) && !_InstanceMethodsOnClass.Contains(methodInfo);

        //...
}

Is the old or the new behavior from the Method GetName the right one?

[joelweiss]

Before this change, when the methods weren't virtual your test would fail just as well

public class Order
{
    //...	
    private string _Name;
    public void SetName(string name) => _Name = name;
    public string GetName() => _Name;
    //...
}

So we are not worse off 😆

I think we would rather have a limitation that field changes made before calling AsTrackable() are lost than having the class not work properly after calling `AsTrackable()'.

What do you think?

[toumir]

Hi @joelweiss
Thanks for the thorough explanation
honestly I am a beginner developer and I don't know what to say 🙈
Thanks for the great project :)

[joelweiss]

@mayphi
Fixed in #39
Please test with unlisted package

[mayphi]

@joelweiss
Sorry for my late response, but i was on vacation :)

I made an small test project and also added the code temporary to #25 and used it in our project and couldn't find any problems.

[joelweiss]

Released in 2.0.22