AutoRefresh should fire when a nullable child property is set to null

[ccopsey]

ObservablePropertyFactory.GetPropertyValue() is interpreting the legitimate null value of the final property in the member chain as "unobtainable", and consequently AutoRefresh() doesn't fire. I would suggest setting this property to null is a legitimate value change and AutoRefresh() should fire.

Further, setting the parent property to null correctly leads to an "unobtainable" value, but I still think AutoRefresh() should fire. An "unobtainable" value could (and in my opinion should) be considered distinct from an "obtainable null".

Having looked around the code I think there are potentially a number of of reasons why I might be observing this behaviour and it is difficult for me to know which is misbehaving, if indeed this isn't by design...

Annotated example code:

public class ClassA : AbstractNotifyPropertyChanged
{
    private ClassB? _child;

    public ClassB? Child
    {
        get => _child;
        set => SetAndRaise(ref _child, value);
    }
}

public class ClassB : AbstractNotifyPropertyChanged
{
    private string? _target;

    public string? Target
    {
        get => _target;
        set => SetAndRaise(ref _target, value);
    }
}

public class Test
{
    public void Main()
    {
        var result = 0;
        var list = new SourceList<ClassA>();
        list.Connect()
          .AutoRefresh(x => x.Child.Target)
          .Do(x => result++)
          .Subscribe();

        result.Should().Be(0);

        var subjectA = new ClassA();
        list.Add(subjectA);
        result.Should().Be(1);

        var subjectB = new ClassB();
        subjectA.Child = subjectB;
        // subjectB.Target is null, which is interpreted as "unobtainable", and hence this doesn't fire.
        // I would suggest that an "obtainable null" is distinct from "unobtainable" and hence this should fire.
        result.Should().Be(1); // Required
        // result.Should().Be(2); // Desired

        subjectB.Target = "TEST";
        result.Should().Be(2); // Required
        // result.Should().Be(3); // Desired

        subjectB.Target = null;
        // subjectB.Target is null, which is interpreted as "unobtainable", and hence this doesn't fire.
        // I would suggest that null is a valid new value and hence this should fire.
        result.Should().Be(2); // Required
        // result.Should().Be(4); // Desired

        subjectA.Child = null;
        // subjectA.Child is null, which is interpreted as "unobtainable", and hence this doesn't fire.
        // I would suggest that an "obtainable null" is distinct from "unobtainable" and hence this should fire.
        result.Should().Be(2); // Required
        // result.Should().Be(5); // Desired
    }
}

If the misbehaviour is that a null final member value shouldn't be considered "unobtainable", then the following change gives me the behaviour I expect, however this class is too fundamental for me to understand the potential wide-spread implications:

diff --git a/src/DynamicData/Binding/ObservablePropertyFactory.cs b/src/DynamicData/Binding/ObservablePropertyFactory.cs
index 7717431..4486461 100644
--- a/src/DynamicData/Binding/ObservablePropertyFactory.cs
+++ b/src/DynamicData/Binding/ObservablePropertyFactory.cs
@@ -64,7 +64,7 @@ internal class ObservablePropertyFactory<TObject, TProperty>
     private static IEnumerable<IObservable<Unit>> GetNotifiers(TObject source, IEnumerable<ObservablePropertyPart> chain)
     {
         object value = source;
-        foreach (var metadata in chain.Reverse())
+        foreach (var metadata in chain.Reverse().SkipLast(1))
         {
             var obs = metadata.Factory(value).Publish().RefCount();
             value = metadata.Accessor(value);

Alternatively, if you allow the passing of a non-null fallbackValue from AutoRefresh() to WhenPropertyChanged() then this also works, is much lower risk, and is backward compatible:

diff --git a/src/DynamicData/List/ObservableListEx.cs b/src/DynamicData/List/ObservableListEx.cs
index b1e33ea..8315ef4 100644
--- a/src/DynamicData/List/ObservableListEx.cs
+++ b/src/DynamicData/List/ObservableListEx.cs
@@ -224,8 +224,8 @@ public static class ObservableListEx
     /// <param name="changeSetBuffer">Batch up changes by specifying the buffer. This greatly increases performance when many elements have successive property changes.</param>
     /// <param name="propertyChangeThrottle">When observing on multiple property changes, apply a throttle to prevent excessive refresh invocations.</param>
     /// <param name="scheduler">The scheduler.</param>
     /// <returns>An observable change set with additional refresh changes.</returns>
-    public static IObservable<IChangeSet<TObject>> AutoRefresh<TObject, TProperty>(this IObservable<IChangeSet<TObject>> source, Expression<Func<TObject, TProperty>> propertyAccessor, TimeSpan? changeSetBuffer = null, TimeSpan? propertyChangeThrottle = null, IScheduler? scheduler = null)
+    public static IObservable<IChangeSet<TObject>> AutoRefresh<TObject, TProperty>(this IObservable<IChangeSet<TObject>> source, Expression<Func<TObject, TProperty>> propertyAccessor, TimeSpan? changeSetBuffer = null, TimeSpan? propertyChangeThrottle = null, IScheduler? scheduler = null, Func<TProperty?>? fallbackValue = null)
         where TObject : INotifyPropertyChanged
     {
         if (source is null)
@@ -243,10 +243,10 @@ public static class ObservableListEx
             {
                 if (propertyChangeThrottle is null)
                 {
-                    return t.WhenPropertyChanged(propertyAccessor, false);
+                    return t.WhenPropertyChanged(propertyAccessor, false, fallbackValue);
                 }

-                return t.WhenPropertyChanged(propertyAccessor, false).Throttle(propertyChangeThrottle.Value, scheduler ?? Scheduler.Default);
+                return t.WhenPropertyChanged(propertyAccessor, false, fallbackValue).Throttle(propertyChangeThrottle.Value, scheduler ?? Scheduler.Default);
             },
             changeSetBuffer,
             scheduler);

Which could be implemented a little more cleanly via a flag:

diff --git a/src/DynamicData/List/ObservableListEx.cs b/src/DynamicData/List/ObservableListEx.cs
index b1e33ea..54752f6 100644
--- a/src/DynamicData/List/ObservableListEx.cs
+++ b/src/DynamicData/List/ObservableListEx.cs
@@ -224,8 +224,8 @@ public static class ObservableListEx
     /// <param name="changeSetBuffer">Batch up changes by specifying the buffer. This greatly increases performance when many elements have successive property changes.</param>
     /// <param name="propertyChangeThrottle">When observing on multiple property changes, apply a throttle to prevent excessive refresh invocations.</param>
     /// <param name="scheduler">The scheduler.</param>
     /// <returns>An observable change set with additional refresh changes.</returns>
-    public static IObservable<IChangeSet<TObject>> AutoRefresh<TObject, TProperty>(this IObservable<IChangeSet<TObject>> source, Expression<Func<TObject, TProperty>> propertyAccessor, TimeSpan? changeSetBuffer = null, TimeSpan? propertyChangeThrottle = null, IScheduler? scheduler = null)
+    public static IObservable<IChangeSet<TObject>> AutoRefresh<TObject, TProperty>(this IObservable<IChangeSet<TObject>> source, Expression<Func<TObject, TProperty>> propertyAccessor, TimeSpan? changeSetBuffer = null, TimeSpan? propertyChangeThrottle = null, IScheduler? scheduler = null, bool fireOnUnobtainableValue = false)
         where TObject : INotifyPropertyChanged
     {
         if (source is null)
@@ -238,15 +238,17 @@ public static class ObservableListEx
             throw new ArgumentNullException(nameof(propertyAccessor));
         }

+        var fallbackValue = fireOnUnobtainableValue ? new Func<TProperty?>(() => default(TProperty)) : null;
+
         return source.AutoRefreshOnObservable(
             t =>
             {
                 if (propertyChangeThrottle is null)
                 {
-                    return t.WhenPropertyChanged(propertyAccessor, false);
+                    return t.WhenPropertyChanged(propertyAccessor, false, fallbackValue);
                 }

-                return t.WhenPropertyChanged(propertyAccessor, false).Throttle(propertyChangeThrottle.Value, scheduler ?? Scheduler.Default);
+                return t.WhenPropertyChanged(propertyAccessor, false, fallbackValue).Throttle(propertyChangeThrottle.Value, scheduler ?? Scheduler.Default);
             },
             changeSetBuffer,
             scheduler);

If all of the above is by design, then I have no problem creating my own flavour of AutoRefresh(). If it is not by design, then it is beyond me to know where precisely to make, and propose, the change. Thoughts?

[RolandPheasant]

Thanks for the detailed analysis and I apologize for the long delay.

The behaviour is not by design and more as a result of flawed implementation, which I never got around to fixing

[RolandPheasant]

The whole property change code in DD needs a rework