Merge pull request #168 from badoo/issue-167-fix-serialization-behaviour

[#167] Reverted BaseFeature PostProcessor recursion behaviour change

Author: LachlanMcKee

documentation/whatsnew.md

@@ -2,6 +2,24 @@
 
 ### Pending changes
 
+No pending changes
+
+### 1.3.1
+
+#### Bug fixes
+
+([#138](https://github.com/badoo/MVICore/issues/167)):
+Fixed regression related to BaseFeature actor.
+
+The Actor subject was made serializable, and was also using a flatMap. Both of these changes caused a change in behaviour relating to the ordering of news (in features that have a PostProcessor which triggers extra actions).
+This change was made as part of introducing the optional `FeatureScheduler` to `BaseFeature`.
+
+If you provide a `FeatureScheduler` and use a PostProcessor, please be aware that the ordering of your news could change.
+
+The previous news ordering behaviour is actually a bug in BaseFeature caused by recursion, and will hopefully be addressed (as an opt in change) in a future release.
+
+### 1.3.0
+
 #### Additions
 
 ([#147](https://github.com/badoo/MVICore/pull/147)):

mvicore/src/main/java/com/badoo/mvicore/feature/BaseFeature.kt

@@ -47,7 +47,7 @@ open class BaseFeature<Wish : Any, in Action : Any, in Effect : Any, State : Any
 ) : Feature<Wish, State, News> {
 
     private val threadVerifier = if (featureScheduler == null) SameThreadVerifier(javaClass) else null
-    private val actionSubject = PublishSubject.create<Action>().toSerialized()
+    private val actionSubject = PublishSubject.create<Action>().toSerialized(featureScheduler)
     private val stateSubject = BehaviorSubject.createDefault(initialState)
     private val newsSubject = PublishSubject.create<News>()
     private val disposables = CompositeDisposable()
@@ -287,14 +287,25 @@ open class BaseFeature<Wish : Any, in Action : Any, in Effect : Any, State : Any
         private fun <T : Any> Observable<T>.observeOnFeatureScheduler(
             scheduler: FeatureScheduler?
         ): Observable<T> =
-            flatMap { value ->
-                val upstream = Observable.just(value)
-                if (scheduler == null || scheduler.isOnFeatureThread) {
-                    upstream
-                } else {
-                    upstream
-                        .subscribeOn(scheduler.scheduler)
+            if (scheduler != null) {
+                flatMap { value ->
+                    val upstream = Observable.just(value)
+                    if (scheduler.isOnFeatureThread) {
+                        upstream
+                    } else {
+                        upstream
+                            .subscribeOn(scheduler.scheduler)
+                    }
                 }
+            } else {
+                this
+            }
+
+        private fun <T> Subject<T>.toSerialized(scheduler: FeatureScheduler?) =
+            if (scheduler != null) {
+                toSerialized()
+            } else {
+                this
             }
     }
 }

mvicore/src/test/java/com/badoo/mvicore/feature/BaseFeaturePostProcessorTest.kt

@@ -0,0 +1,90 @@
+package com.badoo.mvicore.feature
+
+import com.badoo.mvicore.element.Actor
+import com.badoo.mvicore.element.NewsPublisher
+import com.badoo.mvicore.element.PostProcessor
+import com.badoo.mvicore.element.Reducer
+import com.badoo.mvicore.feature.PostProcessorTestFeature.*
+import io.reactivex.Observable
+import org.junit.Test
+
+class BaseFeaturePostProcessorTest {
+    @Test
+    fun `GIVEN feature scheduler provided AND InitialTrigger sent WHEN post processor sends PostProcessorTrigger THEN news is in wish order`() {
+        val feature = PostProcessorTestFeature(featureScheduler = FeatureSchedulers.TrampolineFeatureScheduler)
+        val newsTestObserver = Observable.wrap(feature.news).test()
+        feature.accept(Wish.InitialTrigger)
+
+        newsTestObserver.assertValues(News.TriggerNews, News.PostProcessorNews)
+    }
+
+    /**
+     * The post processor is recursively calling the actor, meaning the news is in reverse order in this scenario.
+     */
+    @Test
+    fun `GIVEN feature scheduler not provided AND InitialTrigger sent WHEN post processor sends PostProcessorTrigger THEN news is in recursive order`() {
+        val feature = PostProcessorTestFeature(featureScheduler = null)
+        val newsTestObserver = Observable.wrap(feature.news).test()
+        feature.accept(Wish.InitialTrigger)
+
+        newsTestObserver.assertValues(News.PostProcessorNews, News.TriggerNews)
+    }
+}
+
+private class PostProcessorTestFeature(featureScheduler: FeatureScheduler?) :
+    BaseFeature<Wish, Wish, Effect, State, News>(
+        actor = ActorImpl(),
+        initialState = State,
+        reducer = ReducerImpl(),
+        wishToAction = { it },
+        newsPublisher = NewsPublisherImpl(),
+        postProcessor = PostProcessorImpl(),
+        featureScheduler = featureScheduler
+    ) {
+
+    sealed class Wish {
+        object InitialTrigger : Wish()
+        object PostProcessorTrigger : Wish()
+    }
+
+    sealed class Effect {
+        object TriggerEffect : Effect()
+        object PostProcessorEffect : Effect()
+    }
+
+    object State
+
+    sealed class News {
+        object TriggerNews : News()
+        object PostProcessorNews : News()
+    }
+
+    class ActorImpl : Actor<State, Wish, Effect> {
+        override fun invoke(state: State, wish: Wish): Observable<out Effect> =
+            when (wish) {
+                is Wish.InitialTrigger -> Observable.just(Effect.TriggerEffect)
+                is Wish.PostProcessorTrigger -> Observable.just(Effect.PostProcessorEffect)
+            }
+    }
+
+    class ReducerImpl : Reducer<State, Effect> {
+        override fun invoke(state: State, effect: Effect): State = state
+    }
+
+    class NewsPublisherImpl : NewsPublisher<Wish, Effect, State, News> {
+        override fun invoke(action: Wish, effect: Effect, state: State): News =
+            when (effect) {
+                is Effect.TriggerEffect -> News.TriggerNews
+                is Effect.PostProcessorEffect -> News.PostProcessorNews
+            }
+    }
+
+    class PostProcessorImpl : PostProcessor<Wish, Effect, State> {
+        override fun invoke(action: Wish, effect: Effect, state: State): Wish? =
+            if (action is Wish.InitialTrigger) {
+                Wish.PostProcessorTrigger
+            } else {
+                null
+            }
+    }
+}