Introduce some Refaster rules that avoid nested `Publisher`s #374

werli · 2022-11-26T12:08:13Z

Context

These proposed Refaster rules primarily aim to make Reactor code more readable as nested Publishers are avoided (e.g. Mono<Mono<String>>).

They may result in a minor performance improvement due to the dropped map() calls.

Summary

I've noticed an occurrence of the following code pattern:

Mono<String> method1() {
  Mono.just("foo")
    .map(str -> method2(str))
    .flatMap(identity());
}

Mono<String> method2(String str) {
  ...
}

which these changes rewrite to:

Mono<String> method1() {
  Mono.just("foo")
    .flatMap(str -> method2(str));
}

Conceptionally, the same applies for:

Mono#flatMap(Function)
Mono#flatMapMap(Function)
Flux#concatMap(Function)
Flux#concatMap(Function, int)

and in theory also for some others (e.g. Flux#flatMap()), but these are already re-written by other rules IIUC.

Suggested commit message

Introduce some Refaster rules that avoid nested `Publisher`s (#374)

werli · 2022-11-26T12:12:42Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+    @AfterTemplate
+    Flux<T> after(Mono<S> mono, Function<S, ? extends Publisher<T>> function) {
+      return mono.flatMapMany(function);
+    }


In the case of function being of type Function<S, Mono<T>> first this rule and then MonoFlatMapToFlux are applied, which ultimately rewrite this code piece

mono.map(function).flatMapMany(identity());

to

mono.flatMap(function).flux();

werli · 2022-11-26T12:13:45Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */
+  static final class FlatMapManyIdentity<S, T> {
+    @BeforeTemplate
+    Flux<T> before(Mono<S> mono, Function<S, ? extends Publisher<T>> function) {


Admittedly, I couldn't figure out why

Suggested change

Flux<T> before(Mono<S> mono, Function<S, ? extends Publisher<T>> function) {

Flux<T> before(Mono<S> mono, Function<S, Publisher<T>> function) {

alone wouldn't do it. Anyone an idea what I'm missing here?

In the latter case only function that return precisely a Publisher (so not a subtype) are matched.

💡 Thanks a lot - clear.

Stephan202

Added a commit with some suggestions and also tweaked the suggested commit message; PTAL. :)

Tnx!

Stephan202 · 2022-11-27T14:25:02Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

@@ -615,6 +615,58 @@ Flux<S> after(Flux<T> flux) {
    }
  }

+  /** Prefer {@link Mono#flatMap(Function)} over more contrived alternatives. */
+  static final class MonoFlatMapIdentity<S, T> {


We generally name the patterns after the @AfterTemplate. So this would be MonoFlatMap.

I didn't get this pattern beforehand - noted 📝

No worries; we plan to automate this 😄

Stephan202 · 2022-11-27T14:26:15Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  }
+
+  /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */
+  static final class FlatMapManyIdentity<S, T> {


W.r.t. the use of generic type parameters: further up we use T and S the other way around... perhaps something we should try to "auto-fix" somehow.

Stephan202 · 2022-11-27T14:27:11Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Prefer {@link Mono#flatMapMany(Function)} over more contrived alternatives. */
+  static final class FlatMapManyIdentity<S, T> {
+    @BeforeTemplate
+    Flux<T> before(Mono<S> mono, Function<S, ? extends Publisher<T>> function) {


In the latter case only function that return precisely a Publisher (so not a subtype) are matched.

Stephan202 · 2022-11-27T14:27:50Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Prefer {@link Mono#flatMap(Function)} over more contrived alternatives. */
+  static final class MonoFlatMapIdentity<S, T> {
+    @BeforeTemplate
+    Mono<T> before(Mono<S> mono, Function<S, Mono<T>> function) {


This matches more, and is consistent with the relevant parameter types:

Suggested change

Mono<T> before(Mono<S> mono, Function<S, Mono<T>> function) {

Mono<T> before(Mono<S> mono, Function<? super S, ? extends Mono<? extends T>> function) {

Stephan202 · 2022-11-27T14:29:29Z

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java

+  ImmutableSet<Flux<String>> testFlatMapManyIdentity() {
+    return ImmutableSet.of(
+        Mono.just("foo").map(Mono::just).flatMapMany(identity()),
+        Mono.just("foo").map(Flux::just).flatMapMany(identity()));
+  }


One test suffices for this case :)

Stephan202 · 2022-11-27T14:30:27Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Prefer {@link Flux#concatMap(Function)} over more contrived alternatives. */
+  static final class ConcatMapIdentity<S, T> {
+    @BeforeTemplate
+    Flux<T> before(Flux<S> flux, Function<S, ? extends Publisher<T>> function) {
+      return flux.map(function).concatMap(identity());
+    }
+
+    @AfterTemplate
+    Flux<T> after(Flux<S> flux, Function<S, ? extends Publisher<T>> function) {
+      return flux.concatMap(function);
+    }
+  }


This one can be merged with the existing FluxConcatMap rule. Likewise for the one below.

Thx - I didn't get this apparently.

werli · 2022-11-27T18:19:48Z

Thx @Stephan202 for the good comments. Updated commit message LGTM, that's closer to what I had in mind anyways.

rickie

Nice @werli , you are on a roll! Keep it going 🚀 !

Suggested commit message LGTM!

(Rebased, will merge once 🍏)

werli commented Nov 26, 2022

View reviewed changes

werli added the new feature label Nov 26, 2022

Stephan202 approved these changes Nov 27, 2022

View reviewed changes

rickie self-requested a review November 28, 2022 12:16

werli and others added 2 commits November 28, 2022 13:27

Introduce Reactor identity Refaster rules

5dacf5f

Suggestions

193979f

rickie force-pushed the werli/reactor-publisher-identity branch from 65c7672 to 193979f Compare November 28, 2022 12:27

rickie approved these changes Nov 28, 2022

View reviewed changes

rickie added this to the 0.6.0 milestone Nov 28, 2022

werli changed the title ~~Extend Reactor identity Refaster rules~~ Introduce some Refaster rules that avoid nested Publishers Nov 28, 2022

rickie merged commit f46859a into master Nov 28, 2022

rickie deleted the werli/reactor-publisher-identity branch November 28, 2022 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce some Refaster rules that avoid nested `Publisher`s #374

Introduce some Refaster rules that avoid nested `Publisher`s #374

werli commented Nov 26, 2022 •

edited by Stephan202

Loading

werli Nov 26, 2022

werli Nov 26, 2022

Stephan202 Nov 27, 2022

werli Nov 27, 2022

Stephan202 left a comment

Stephan202 Nov 27, 2022

werli Nov 27, 2022 •

edited

Loading

Stephan202 Nov 28, 2022

Stephan202 Nov 27, 2022

Stephan202 Nov 27, 2022

Stephan202 Nov 27, 2022

Stephan202 Nov 27, 2022

Stephan202 Nov 27, 2022

werli Nov 27, 2022

werli commented Nov 27, 2022

rickie left a comment •

edited

Loading

	Flux<T> before(Mono<S> mono, Function<S, ? extends Publisher<T>> function) {
	Flux<T> before(Mono<S> mono, Function<S, Publisher<T>> function) {

	Mono<T> before(Mono<S> mono, Function<S, Mono<T>> function) {
	Mono<T> before(Mono<S> mono, Function<? super S, ? extends Mono<? extends T>> function) {

Introduce some Refaster rules that avoid nested Publishers #374

Introduce some Refaster rules that avoid nested Publishers #374

Conversation

werli commented Nov 26, 2022 • edited by Stephan202 Loading

Context

Summary

Suggested commit message

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli commented Nov 27, 2022

rickie left a comment • edited Loading

Choose a reason for hiding this comment

Introduce some Refaster rules that avoid nested `Publisher`s #374

Introduce some Refaster rules that avoid nested `Publisher`s #374

werli commented Nov 26, 2022 •

edited by Stephan202

Loading

werli Nov 27, 2022 •

edited

Loading

rickie left a comment •

edited

Loading