[Feature]: FilterOnObservable passes to downstream anyways then it gets back to check?!

[bakhtvar]

Describe the functionality desired 🐞

.FilterOnObservable(item => item.WhenAnyValue(x => x.AlwaysFalse))
.Transform(x => {
Log.Warning("Shall never be called!");
return x;
})

While it is expected the log never happens, but the log is happening once!
I have asked some AI, they said that this is by design, but it is totally wired from my point of view.

The steps the functionality will provide

I guess, with the above design ultimately FilterOnObservable becomes useless,,,, and I have used the following workaround everywhere..

.AutoRefreshOnObservable(item => item.WhenAnyValue(x => x.AlwaysFalse).Skip(1))
.Filter(x => x.AlwaysFalse)
.Transform(x => {
Log.Warning("Shall never be called!");
return x;
})

this correctly never goes to log, but I had two lines of code instead of one.

Considerations

Above I provided the workaround.

[JakenVeina]

What you describe would absolutely be a defect, not a request for new functionality. However, I'm unable to replicate the behavior you describe.

public class Item
    : INotifyPropertyChanged
{
    public int Id { get; init; }

    public bool IsIncluded { get; init; }
    
    public event PropertyChangedEventHandler? PropertyChanged;
}

using var source = new SourceCache<Item, int>(static item => item.Id);

using var subscription = source
    .Connect()
    .FilterOnObservable(static item => item
        .WhenAnyValue(static item => item.IsIncluded))
    .Transform(static item => item)
    .Bind(out var target)
    .Subscribe();

source.AddOrUpdate(new Item()
{
    Id          = 1,
    IsIncluded  = false
});

With this setup, a debugger confirms static item => item is never invoked, and target remains empty, after the item is added to source.

If you're still experiencing this issue, you're gonna have to provide some more details on how to replicate it.

Also, I'm assuming .WhenAnyValue() is from ReactiveUI?

[bakhtvar]

I have attached a project simply shows the issue.

using DynamicData;
using DynamicData.Binding;
using ReactiveUI;
using ReactiveUI.Fody.Helpers;
using System.Reactive.Linq;

Console.WriteLine("Hello, World!");

var members = new ObservableCollectionExtended();

members.Add(new() { Name = "Joe", Room = 1 });
members.Add(new() { Name = "Sam", Room = 2 });
members.Add(new() { Name = "Tim", Room = 3 });

members.ToObservableChangeSet()
.FilterOnObservable(p => p.WhenAnyValue(x => x.Room).Select(r => r > 4))
.Transform(x =>
{
Console.Write(x.Name);
return x;
})
.Bind(out var highRoom)
.Subscribe();

var cnt = highRoom.Count;

class Persion : ReactiveObject
{
public string Name { get; set; }
[Reactive]
public int Room { get; set; }
}

While the final count is zero, but the pipeline passes the Console.Write(x.Name) three times!

ConsoleApp1.zip

[JakenVeina]

Alrighty, so the issue is specific to subscribing to a source that is already populated, rather than having items added after subscription.It's also specific only to the list version of this operator, not the cache version.

After some research, the behavior was actually an intentional decision, from when the operator was first introduced in 2019: to have items be included in the downstream collection, instead of excluded, when the filtering stream has not yet emitted its initial value. This behavior then combines with the fact that the operator does not flatten changes observed from different sources, within the same frame AND does not ensure that filtering subscriptions occur before adding the new items.

DynamicData/src/DynamicData/List/Internal/FilterOnObservable.cs

Line 24 in 546f3ad

var shared = _source.Synchronize(locker).Transform(v => new ObjWithFilterValue(v, true)) // we default to true (include all items)

640c210

#205 (comment)

Me personally, I think that the decision to include items by default, instead of exclude them, was the wrong decision. It forces someone who DOESN'T want items to be included by default to do a weird workaround to get rid of them, as you posted above (double up on the filtering operators). Whereas, if we don't include items by default, the workaround for a consumer to get them included is trivial (just toss .StartWith(true) on the predicate stream).

But even more significant, I think, is the fact that our two different FilterOnObservable() operators do not have the same functional behavior. That definitely strikes me as a usability issue that we should change, even if it means a breaking change for one of the two operators.

However, just looking at this issue, specifically, I think it arguably qualifies as a bug. The items in question are doing what they're supposed to and emitting a false immediately on subscription, but it's getting missed because the operator only subscribes to an item's filter stream AFTER it's added it. Although, I'm not sure if that's easily changeable, without a large-scale rewrite, as it's a byproduct of combining .Transform() with ObjWithFilterValue.

@RolandPheasant @dwcullop thoughts?

[dwcullop]

That's my fault. I implemented the Cache version without looking at the List version. I think the behavior to omit items until the Filter observable has emitted one value (so we know for sure to include or not) is the right thing because consumer can opt-in to either behavior with StartWith(true) or StartWith(false).

The question is... Do we want to change either behavior and possibly break users who have already adapted to the different behavior?

[JakenVeina]

I think it's worth a breaking change, yeah. Like you and I both pointed out, it's a pretty simple workaround, either way. So long as we do a major version bump with it, I think it's acceptable.

I think the behavior to omit items until the Filter observable has emitted one value (so we know for sure to include or not) is the right thing because consumer can opt-in to either behavior with StartWith(true) or StartWith(false).

Yeah, you're right. For this operator in particular, the caller has complete control over building the filtering stream, so we should just GIVE them complete control over the filtering logic.

Edit: It occurs to me that that's actually the same thing. If an item comes in and we don't add it, because it didn't emit any value at all... well, we've assumed it's false then. The consumer opting in with .StartWith(false) would then be redundant.

[RolandPheasant]

Me personally, I think that the decision to include items by default, instead of exclude them, was the wrong decision.

I agree with this statement, and therefore a breaking change is fine,