Skip to content

Commit

Permalink
finagle-core: Filter.andThenIf That Doesn't Require a Tuple
Browse files Browse the repository at this point in the history
Problem

The current version of `Filter.andThenIf` requires the user to pass
both the conditional and the conditionally applied `Filter` together
as a tuple. The ergonomics of this style of method matched a prior
version of how Scala code used to be written at Twitter. However, given
the current set of standards, this ends up feeling clunky, and requiring
additional unnecessary tuple allocations.

Solution

Provide a variant of the `Filter.andThenIf` method which takes the
individual components, the conditional and the conditionally applied
`Filter`, as individual parameters.

Differential Revision: https://phabricator.twitter.biz/D523010
  • Loading branch information
ryanoneill authored and jenkins committed Jul 28, 2020
1 parent 51c3e62 commit fb071d9
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 3 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
Expand Up @@ -25,6 +25,9 @@ New Features

* finagle-core: introduce type-safe `ReqRep` variant ``PHAB_ID=D520027``

* finagle-core: Added a new variant of `Filter.andThenIf` which allows passing the parameters
as individual parameters instead of a Scala tuple. ``PHAB_ID=D523010``

Breaking API Changes
~~~~~~~~~~~~~~~~~~~~

Expand Down
18 changes: 17 additions & 1 deletion finagle-core/src/main/scala/com/twitter/finagle/Filter.scala
Expand Up @@ -127,7 +127,23 @@ abstract class Filter[-ReqIn, +RepOut, +ReqOut, -RepIn]

/**
* Conditionally propagates requests down the filter chain. This may
* useful if you are statically wiring together filter chains based
* be useful if you are statically wiring together filter chains based
* on a configuration file, for instance.
*
* @param conditional a boolean value indicating whether the filter should be
* included in the filter chain.
* @param filter the filter to be conditionally included.
*/
def andThenIf[Req2 >: ReqOut, Rep2 <: RepIn](
conditional: Boolean,
filter: Filter[ReqOut, RepIn, Req2, Rep2]
): Filter[ReqIn, RepOut, Req2, Rep2] =
if (conditional) andThen(filter)
else this

/**
* Conditionally propagates requests down the filter chain. This may
* be useful if you are statically wiring together filter chains based
* on a configuration file, for instance.
*
* @param condAndFilter a tuple of boolean and filter.
Expand Down
18 changes: 16 additions & 2 deletions finagle-core/src/test/scala/com/twitter/finagle/FilterTest.scala
Expand Up @@ -379,20 +379,34 @@ class FilterTest extends FunSuite {
verify(spied).apply(any[ClientConnection])
}

test("Filter.andThenIf: applies next filter when true") {
test("Filter.andThenIf (tuple): applies next filter when true") {
val spied = spy(new PassThruFilter)
val svc = (new PassThruFilter).andThenIf((true, spied)).andThen(constSvc)
await(svc(4))
verify(spied).apply(any[Int], any[Service[Int, Int]])
}

test("Filter.andThenIf: doesn't apply next filter when false") {
test("Filter.andThenIf (tuple): doesn't apply next filter when false") {
val spied = spy(new PassThruFilter)
val svc = (new PassThruFilter).andThenIf((false, spied)).andThen(constSvc)
await(svc(4))
verify(spied, never).apply(any[Int], any[Service[Int, Int]])
}

test("Filter.andThenIf (params): applies next filter when true") {
val spied = spy(new PassThruFilter)
val svc = (new PassThruFilter).andThenIf(true, spied).andThen(constSvc)
await(svc(4))
verify(spied).apply(any[Int], any[Service[Int, Int]])
}

test("Filter.andThenIf (params): doesn't apply next filter when false") {
val spied = spy(new PassThruFilter)
val svc = (new PassThruFilter).andThenIf(false, spied).andThen(constSvc)
await(svc(4))
verify(spied, never).apply(any[Int], any[Service[Int, Int]])
}

test("Filter.choose: apply the underlying filter to certain requests") {
val spied = spy(new PassThruFilter)
val svc = Filter
Expand Down

0 comments on commit fb071d9

Please sign in to comment.