fix: Preserve inner filter chain when composing RxRouter filters - #4173
Merged
Conversation
RxRouter.FilterNode.andThen(next: FilterNode) previously overwrote
next.parent, silently dropping intermediate filters when the inner
argument was already a filter chain. For example:
RxRouter.filter[A].andThen(
RxRouter.filter[B].andThen[C].andThen(routes)
)
produced chain A -> C, losing B.
Attach `this` at the top of next's existing parent chain instead, so
composition is associative and equivalent to the flat form
filter[A].andThen[B].andThen[C].andThen(routes).
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in RxRouter where composing filter chains would result in intermediate filters being dropped. The andThen implementation in FilterNode was updated to recursively traverse and attach to the top of the existing parent chain. Additionally, new test cases were added to verify that filter order and completeness are maintained during complex router compositions. I have no feedback to provide.
RxRoute case-class equality depends on MethodSurface equality, which is not reliable across Surface.methodsOf[T] calls on Scala Native (the CI failure came from comparing two separately-built routes). Flatten the FilterNode chain into a List[Surface] instead — that's what the test actually wants to assert.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RxRouter.FilterNode.andThen(next: FilterNode)overwrotenext.parent, silently dropping intermediate filters when composing already-built filter chains.thison top ofnext's existing parent chain so the composition is associative.Problem
Given:
The expected chain is
A -> B -> C, but the previous implementation producedA -> C, silently losingB. The bug also surfaced insideStemNode.wrapWithFilterandRxRoute.wrapWithFilter, both of which delegate toFilterNode.andThen(FilterNode)viaandThenOpt, so nestedStemNodes with their own filter chains were affected too.Fix
When composing
this.andThen(next), walk to the top ofnext.parentand attachthisthere instead of overwritingnext.parent.Test plan
RxRouterTest: existing tests passPreserve the outer filter when wrapping a pre-filtered router(simple 2-filter case)Preserve the full inner filter chain when wrapped by an outer filterfails before the fix, passes after; also asserts equivalence with the flatfilter[A].andThen[B].andThen[C].andThen(routes)form./sbt httpJVM/test(Scala 3): 195 tests pass./sbt '++ 2.13' 'httpJVM/testOnly *RxRouterTest *RxRouterConverterTest'passes./sbt netty/testpasses (119)./sbt httpCodeGen/testpasses (43)