[Code health] Verify exceptions are either wrapped or handled in all Rx streams

[gino-m]

trafficstars

We can rename Result to something like ValueOrError, and wrap the value at the source, obviating the need to wrap/unwrap at arbitrary steps in the stream.

@scolsen FYI.

[gino-m]

Should we really though? Posted this question asking about an alternative approach, a là:

    disposeOnClear(
        addFeatureClicks
            .switchMap(
                f ->
                    dataRepository
                        .saveFeature(f)
                        .doOnComplete(() -> showFeatureSheet(f))
                        .doOnError(t -> onAddFeatureError(t))
                        .onErrorComplete()
                        .toObservable())
            .subscribe());

There's not need for a wrapper unless we want to propagate errors across long-lived streams. Since Rx streams obtained imperatively will return Completable, Single, or Maybe, this will usually only happen when declaratively setting up and subscribing in ViewModels.

[gino-m]

@scolsen, I discussed this with former teammates at FAO today (@cdanielw, @lpaolini) who work extensively with RxJS in SEPAL, and consensus is that it's better to wrap and unwrap in some intermediate object to represent errors as a value rather than at stream level (as we have now), rather than to rely on side-effects on the inner stream, e.g.:

https://github.com/google/ground-android/blob/fd1324aa8e59159c9941c5dfab6c7c082cff0b6f/gnd/src/main/java/com/google/android/gnd/ui/projectselector/ProjectSelectorViewModel.java#L62-L67

My guess is that with some additional syntactic sugar and clearer naming we can make this more self-documenting / obvious without sacrificing FRP "purity".

The syntax is also less verbose than the above proposed example, which may be an added bonus here.

[scolsen]

Sounds good! Let's stick with our basic idea then and iterate on improving the clarity of the semantics.

ValueOrError is fairly straightforward.

Would it be clearer to the majority of Java/Android devs if we emulated or took inspiration from an existing interface like Optional? i.e. instead of doing something like unwrap doing something like orError -- this particular example doesn't sound like a great idea to me, just speculating here haha, but maybe it's an approach we can consider.

[gino-m]

Yes, I was thinking the same thing. Afaict, the closest use of Optional is Java 9's "ifPresentOrElse()". What would the equivalent be here? ifSuccessOrElse?

On Tue, May 21, 2019, 2:25 AM Scott Olsen [email protected] wrote:

Sounds good! Let's stick with our basic idea then and iterate on improving the clarity of the semantics.

ValueOrError is fairly straightforward.

Would it be clearer to the majority of Java/Android devs if we emulated or took inspiration from an existing interface like Optional? i.e. instead of doing something like unwrap doing something like orError -- this particular example doesn't sound like a great idea to me, just speculating here haha, but maybe it's an approach we can consider.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/ground-android/issues/84?email_source=notifications&email_token=AABXVUWZ4RD664COGZDP6OTPWM6PZA5CNFSM4HLNENAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2NEJI#issuecomment-494195237, or mute the thread https://github.com/notifications/unsubscribe-auth/AABXVUR4J4GWBDZ3DQI4BZTPWM6PZANCNFSM4HLNENAA .

[scolsen]

ifSuccessOrElse sounds right to me!

Do we prefer ValueOrError, Result, or SuccessOrError?

I like Result because it's concise, but I could see it being a bit less understandable from the perspective of someone unfamiliar with the code base.

I think we'll want to change mapSingle and mapObservable to match too. Since Optional uses of how about: ofSingle ofObservable etc?

Or we could do a little bit more work on the API design side to expose only an of method -- this would make it really easy to use in practice as we wouldn't have to recall the particular type of item we're wrapping--I don't see any immediate value in making users recall which types they're wrapping (having to use particular ofSingle etc methods) since once it's wrapped it no longer matters, really.

I think my preference is for something like the following:

Result

• of(<Function<Observable|Flowable|Single|...> stream) : Wrap RX types, Observable, Single etc.
• ifSuccessOrElse(<Function<Result::Success>> f, <Function<Result::Error>> g) : Unwrap a result value calling f on success, g on error.

Note here that of is taking the place of our mapping functions mapSingle etc.

I think we more or less decided we don't need our current wrapSingle and wrapObservable methods (wrap into a result using identity, not a mapping function) but if we do we can overload of to take RX entities directly in addition to a function, or we can provide a method like from.

Of taking a function (since it's actually a map) is a little odd at first glance, but I think it works in practice and reads pretty well (example after the paragraph below).

Overloading generic methods (polymorphic methods) in Java is a bit messy because of type erasure. So unfortunately we can't just implement something like of over any RX type using overloads. Instead I think(?) we'll have to do something like define private methods for each type that the of method dispatches to somehow--so it's a little more work on our side, but it makes using wrapper class dead simple:

 this.activeProjectStream = 
     projectSelections.switchMap(Result.of(this::selectActiveProject)); 
  
 disposeOnClear( 
     activeProjectStream.subscribe( 
         Result.ifSuccessOrElse(activeProject::setValue, this::onActiveProjectError))); 

Another alternative if we need to wrap using identity:

Result

• of(<Observable|Single|...> stream)
• from(<Function<Observable|Single|...>> stream)
• ifSuccessOrElse(<Function<Result::Success>> f, <Function<Result::Error>> g)

Or we could make the function argument to of optional such that projectSelections.switchMap(Result::of) is valid and wraps the stream without performing any mapping.

Or if we want to make it really obvious we could even do something like Result.ofMapping(f)

I don't like the obvious choice of Result.map only because I think it's easy to confuse that method as mapping over a Result because of the way map is typically used--instead of mapping over something and returning a result(which is what it'd actually be doing).

Result|ValueOrError|SuccessOrError.of seems to me to be the shortest and clearest.

[gino-m]

I'm partial to ValueOrError because of existing precedent in some C++ libraries. On the other hand, I also like Result because it's less verbose. I'll need to see it used in context to get a sense of how clear it is.

Regarding of(), I remember reading a blog post always suggesting using compose() to mutate streams in Rx, since it represents stream mutations in a way that's consistent with other operations. I also have mixed feelings about how much logic should be hidden inside the helper (hidden but more concise), in which case you may need to know something about what the helper does, vs. represented using Rx idioms (more explicit, less to learn, more verbose).

So in other words, rather than wrapping with of() or calling compose(), we may just use Rx methods like:

    disposeOnClear(
        addFeatureClicks
            .switchMapSingle(
                f ->
                    dataRepository
                        .addFeature(f)
                        .map(ValueOrError::value)
                        .onErrorReturn(ValueOrError::error))
            .subscribe(voe -> voe.ifValueOrElse(this::showFeatureSheet, this::onAddFeatureError)));

We could also use compose, but it only saves one line in each declaration:

    disposeOnClear(
        addFeatureClicks
            .switchMapSingle(
                f ->
                    dataRepository
                        .addFeature(f)
                        .compose(ValueOrError::mapValueOrError)
            .subscribe(voe -> voe.ifValueOrElse(this::showFeatureSheet, this::onAddFeatureError)));

Still meditating on this one. Other ideas welcome.

[scolsen]

Edit: Silly me. I was indeed missing something. We do still need to call something like onError, since the stream will cease otherwise :/, so we do indeed need to return things on error. At that point though, I don't really see the need for a wrapper object if we're calling onErrorReturn on each stream explicitly. Another option is to reestablish streams in our error handling explicitly.

ValueOrError is cool with me!

I kind of like the first form you suggested. But tbh investigating this really just makes me wish RX had semantics for "never dies" and we didn't have to contend with wrappers.

One final small note--I think we can actually call onErrorReturn after switchMapSingle--it makes more sense to me semantically since the map is returning a new observable on each call anyway. Actually, same with map, so you could have:

addFeatureClicks
  .switchMapSingle(dataRepository::addFeature)
  .map(ValueOrError::value)
  .onErrorReturn(ValueOrError::error)
  .subscribe(voe -> voe.ifValueOrElse(f, g))

Since the error from the dataRepository is mapped out to the outer stream anyways.

I feel this form is slightly more straightforward since it's actually the click stream that we want to keep alive--then again, I've sufficiently muddled my head over this, so maybe not hah.

[gino-m]

Sorry to vacillate on this, but after more experimentation, I'm back to thinking handling errors with side-effects rather than stream values will lead to more readable, self-documenting code.

Consider this example, if you will:

    disposeOnClear(
        addFeatureClicks
            .switchMapSingle(
                feature ->
                    dataRepository
                        .addFeature(feature)
                        .doOnError(this::onAddFeatureError)  // Handle errors.
                        .onErrorResumeNext(Single.never()))  // Prevent error from breaking upstream.
            .subscribe(this::showFeatureSheet));  // Handle success case.

And compare with its "pure" equivalent using ValueOrError:

    disposeOnClear(
        addFeatureClicks
            .switchMapSingle(
                feature ->
                    dataRepository
                        .addFeature(feature)
                        .map(ValueOrError::value)
                        .onErrorReturn(ValueOrError::error))
            .subscribe(voe -> voe.ifValueOrError(this::showFeatureSheet, this::onAddFeatureOrError));

Advantages of using side effects over (un)wrapping:

1. No need to extract ValueOrError class.
2. Boilerplate for wrap/unwrap errors is replaced with idiomatic, self-describing method calls.
3. Allows errors handlers to be declared individually in context for each downstream stream rather than upstream where offending stream is unknown.

This approach isn't functionally "pure", but a) neither is most our application, and b) for our purposes, afaict this is a purely theoretical drawback that won't manifest in practice.

Rx itself treats errors as a property of the stream, that can be manually mapped to/from values if needed. This is perhaps the root cause of this impedance mismatch. But since errors are already a special case for Rx, I think we're on solid ground managing them as side-effects.

Note that errors must be handled downstream because if they're handled upstream, they still cause the source stream to break (just verified this manually, also described here). If you return a new Observable, the upstream (clicks) is replaces with it, breaking the flow.

Wdyt?

[scolsen]

I like it!

I agree that worrying about functional purity shouldn't be of high-priority when we're using RX--I think the most important thing is to stick with RX's semantics and definitions as much as possible--using side effects as you have here is a lot clearer and requires less work to grok for those that already understand RX Java (vs. having to learn about our own custom wrapper).

I think it also makes sense to conceive of "handling errors" as a side effect, which makes the use of the side-effecting operators clear--we want to do something to the stream's value (usually logging or displaying an error) without actually returning anything or modifying the contents of the stream (at that particular point). This is what I was trying to drive at with my suggestion to use these operations in #67 for UI related stuff, though I didn't express it very clearly and I think the title of the issue made it confusing since "pure" usually has a more pedantic meaning in the context of functional programming--I more so meant that we should express operations on RX streams using RX operators that signal our intent rather than tuck in additional logic in the functions passed to the mapping/transformative operators.

So it sounds like a good solution to me!

[gino-m]

Great, glad we're on the same page! tl;drs:

1. Ok to handle errors with onError*(), no need to wrap in value object.
2. With switchMapped streams, apply onErrorResumeNext() upstream (inside switchMap) where things can break; applying downstream is too late.
3. Avoid using doOnNext() and doOnSuccess() for non-error states, rather handle in subscribe().

I'll try to send out a PR for error handling this week.

[gino-m]

A few more thoughts while investigating adding consistent error handling:

• Where do we report errors to the user? Ideally all user-visible logic should live in views, which implies we'd like to report errors as far downstream as possible (i.e., in callers rather than functions returning streams). It might make sense for an eventual ErrorHandler class that requires a View context rather than the Application to make sure system classes aren't unexpectedly causing UI effects.
• This isn't always possible, however, since streams nested with flatMap* and switchMap* contain their own streams, which do not propagate errors, and if unhandled, break the outer stream. This implies we may want to at least wrap inner streams with Result<>.

[gino-m]

Snippet for wrapping/unwrapping mapped streams in ValueOrError objects: https://gist.github.com/gino-m/efcfd158a4d6e4461ac17a1a064d8a7e

[gino-m]

2. With switchMapped streams, apply onErrorResumeNext() upstream (inside switchMap) where things can break; applying downstream is too late.

I was just verifying this and I'm not convince this is the case. Will double check.

[gino-m]

Updated description to clarify what we need to verify.

[gino-m]

@dturner Going through the code you may have noticed that we have inconsistent handling of error states, esp. wrt persistence. Some are propagated to the UI layer and handled there, while others are handled (logged and ignored) in the Repository layer.

The most complex use case is when we read multiple map features from remote and sync them to the local db in a background thread, with the UI layer subscribing to changes in the local db. If one item in the list of fetched features is invalid, ideally we'd like to do something like:

1. Log what went wrong
2. Warn the user some data may not have been loaded, and
3. Ignore the faulty record and continue

How far forward to the UI layer should we propagate these? Is it common to show a Toast message from the Repository layer for issues like these? It breaks some abstract and complicates testing, but may reduce code complexity.

Would you mind taking a look to see if there's a more consistent approach we should take?

[gino-m]

See also big refactor PR #371 for reference.

[scolsen]

@gino-m -- is this covered by #371? Should we close this in favor of moving to coroutines where we can handle exceptions directly in blocks?

[gino-m]

@gino-m -- is this covered by #371? Should we close this in favor of moving to coroutines where we can handle exceptions directly in blocks?

Sorry just replying to this now. Agreed we should close this in favor of migrating to coroutitnes.