From c0f89bc16ad4e0a25e4815e99b4744ad30c7f9ea Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Tue, 25 Oct 2022 11:33:50 +0200 Subject: [PATCH 1/4] Prefer `Flux#take(n, true)` over `Flux#take(n)` to limit generation In Reactor versions prior to 3.5.0, `Flux#take(n)` makes an unbounded request upstream, and is equivalent to `Flux#take(n, false)`. In 3.5.0, the behavior of `Flux#take(n)` will change to that of `Flux#take(n, true)`. The intent with this Refaster rule is to get the new behavior before upgrading to Reactor 3.5.0. --- .../errorprone/refasterrules/ReactorRules.java | 12 ++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 4 ++++ .../refasterrules/ReactorRulesTestOutput.java | 4 ++++ 3 files changed, 20 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 1b3bf9c3e9..809ff85d51 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -144,6 +144,18 @@ Mono after(Mono mono, S object) { } } + static final class FluxTakeGenerationLimit { + @BeforeTemplate + Flux before(Flux flux, long n) { + return flux.take(n); + } + + @AfterTemplate + Flux after(Flux flux, long n) { + return flux.take(n, /* limitRequest= */ true); + } + } + /** Don't unnecessarily pass an empty publisher to {@link Mono#switchIfEmpty(Mono)}. */ static final class MonoSwitchIfEmptyOfEmptyPublisher { @BeforeTemplate diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 270dce8306..71239e5059 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -58,6 +58,10 @@ Mono testMonoThenReturn() { return Mono.empty().then(Mono.just("foo")); } + Flux testFluxTakeGenerationLimit() { + return Flux.just(1, 2, 3).take(1); + } + Mono testMonoSwitchIfEmptyOfEmptyPublisher() { return Mono.just(1).switchIfEmpty(Mono.empty()); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 2c790c959b..acdeb99327 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -60,6 +60,10 @@ Mono testMonoThenReturn() { return Mono.empty().thenReturn("foo"); } + Flux testFluxTakeGenerationLimit() { + return Flux.just(1, 2, 3).take(1, true); + } + Mono testMonoSwitchIfEmptyOfEmptyPublisher() { return Mono.just(1); } From f2b7372515a081564d696c0ccd0583c678e3ae93 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 25 Oct 2022 13:54:57 +0200 Subject: [PATCH 2/4] Suggestions --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 3 ++- .../picnic/errorprone/refasterrules/ReactorRulesTestInput.java | 2 +- .../errorprone/refasterrules/ReactorRulesTestOutput.java | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 809ff85d51..e63b49e0b3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -144,7 +144,8 @@ Mono after(Mono mono, S object) { } } - static final class FluxTakeGenerationLimit { + /** Prefer {@link Flux#take(long, boolean)} over {@link Flux#take(long)} to limit generation. */ + static final class FluxTake { @BeforeTemplate Flux before(Flux flux, long n) { return flux.take(n); diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 71239e5059..60c0902de4 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -58,7 +58,7 @@ Mono testMonoThenReturn() { return Mono.empty().then(Mono.just("foo")); } - Flux testFluxTakeGenerationLimit() { + Flux testFluxTake() { return Flux.just(1, 2, 3).take(1); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index acdeb99327..e24d8157fe 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -60,7 +60,7 @@ Mono testMonoThenReturn() { return Mono.empty().thenReturn("foo"); } - Flux testFluxTakeGenerationLimit() { + Flux testFluxTake() { return Flux.just(1, 2, 3).take(1, true); } From 2b35d021d85c5a470f07d5bcb34c68b2d8a37bad Mon Sep 17 00:00:00 2001 From: Eric Staffas <114935344+eric-picnic@users.noreply.github.com> Date: Tue, 25 Oct 2022 14:30:35 +0200 Subject: [PATCH 3/4] Extend FluxTake Javadoc with additional information --- .../picnic/errorprone/refasterrules/ReactorRules.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index e63b49e0b3..804530c790 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -144,7 +144,16 @@ Mono after(Mono mono, S object) { } } - /** Prefer {@link Flux#take(long, boolean)} over {@link Flux#take(long)} to limit generation. */ + /** + * Prefer {@link Flux#take(long, boolean)} over {@link Flux#take(long)}. + * + *

In Reactor versions prior to 3.5.0, `Flux#take(long)` makes an unbounded request upstream, + * and is equivalent to `Flux#take(long, false)`. In 3.5.0, the behavior of `Flux#take(long)` will + * change to that of `Flux#take(long, true)`. + * + *

The intent with this Refaster rule is to get the new behavior before upgrading to Reactor + * 3.5.0. + */ static final class FluxTake { @BeforeTemplate Flux before(Flux flux, long n) { From 46ab26b5ad39382bb591836bcbbdecbf60e3ca1c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 25 Oct 2022 18:53:37 +0200 Subject: [PATCH 4/4] Suggestions --- .../errorprone/refasterrules/ReactorRules.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 804530c790..d2eb81d06d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1,6 +1,7 @@ package tech.picnic.errorprone.refasterrules; import static com.google.common.collect.MoreCollectors.toOptional; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS; import static java.util.function.Function.identity; import static org.assertj.core.api.Assertions.assertThat; @@ -28,7 +29,9 @@ import reactor.test.StepVerifier; import reactor.test.publisher.PublisherProbe; import reactor.util.context.Context; +import tech.picnic.errorprone.refaster.annotation.Description; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.Severity; import tech.picnic.errorprone.refaster.matchers.ThrowsCheckedException; /** Refaster rules related to Reactor expressions and statements. */ @@ -147,13 +150,19 @@ Mono after(Mono mono, S object) { /** * Prefer {@link Flux#take(long, boolean)} over {@link Flux#take(long)}. * - *

In Reactor versions prior to 3.5.0, `Flux#take(long)` makes an unbounded request upstream, - * and is equivalent to `Flux#take(long, false)`. In 3.5.0, the behavior of `Flux#take(long)` will - * change to that of `Flux#take(long, true)`. + *

In Reactor versions prior to 3.5.0, {@code Flux#take(long)} makes an unbounded request + * upstream, and is equivalent to {@code Flux#take(long, false)}. In 3.5.0, the behavior of {@code + * Flux#take(long)} will change to that of {@code Flux#take(long, true)}. * *

The intent with this Refaster rule is to get the new behavior before upgrading to Reactor * 3.5.0. */ + // XXX: Drop this rule some time after upgrading to Reactor 3.6.0, or introduce a way to apply + // this rule only when an older version of Reactor is on the classpath. + // XXX: Once Reactor 3.6.0 is out, introduce a rule that rewrites code in the opposite direction. + @Description( + "Prior to Reactor 3.5.0, `take(n)` requests and unbounded number of elements upstream.") + @Severity(WARNING) static final class FluxTake { @BeforeTemplate Flux before(Flux flux, long n) {