Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules #142

cernat-catalin · 2022-06-22T07:24:23Z

Based on a suggestion from here.

Adds a Refaster template to prefer mono.map(x -> valueTransformation(x)) over the mono.flatMap(x -> Mono.just(valueTransformation(x)))

This avoids an unnecessary inner inner subscription.

rickie · 2022-06-23T06:27:59Z

Thanks for opening a PR @cernat-catalin. Just to let you know we saw it; next week I'll take a look at this!

Stephan202

Tnx for suggesting this! Added a commit.

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

...b/src/test/resources/tech/picnic/errorprone/refastertemplates/ReactorTemplatesTestInput.java

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

Stephan202

Some open questions/thoughts; need to leave the metro now :)

Stephan202 · 2022-06-24T07:24:58Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

+
+    @BeforeTemplate
+    Mono<S> before(Mono<T> mono) {
+      return mono.flatMap(x -> Mono.just(transformation(x)));


The same applies to Flux.just, and we could add that case with Refaster.anyOf. OTOH, perhaps we should separately flag Flux.just(singleton) passed to a Publisher-accepting method.

The same applies to Flux.just, and we could add that case with Refaster.anyOf

Do you mean something rewriting this one: Mono.flatMapMany(x -> Flux.just(transformation(x))) ? We could do it with something like this:

Publisher<S> before(Mono<T> mono) { return Refaster.anyOf( mono.flatMap(x -> Mono.just(transformation(x))), mono.flatMapMany(x -> Flux.just(transformation(x))) ); }

But the above would in some situations change the type of the expression from Flux to Mono resulting in compilation errors. Is that something that we consider acceptable?

flag Flux.just(singleton) passed to a Publisher-accepting method

I've tried a bit to come up with a Refaster rule for this but couldn't. Is this something that can actually be done with Refaster or only with error-prone?

The code your suggesting seems good!

But the above would in some situations change the type of the expression from Flux to Mono resulting in compilation errors. Is that something that we consider acceptable?

No this is generally a no-go. We only allow this if in ~99% of the cases this won't result in compilation errors..

flag Flux.just(singleton) passed to a Publisher-accepting method

To be honest, I fail to see how this would look like 😬.

But the above would in some situations change the type of the expression from Flux to Mono resulting in compilation errors. Is that something that we consider acceptable?

Good point. Ideally we indeed avoid this, though the similarity between the Mono and Flux APIs might allow us to get away with this in many cases. If we want to avoid this we could instead have a separate rule that adds .flux(). (And then have separate Refaster rules (or a check) which "pushes down"/"dissolves" the .flux() where possible.

IIUC we could have a rule to transform
mono.flatMapMany(x -> Mono.just(transformation(x))) -> mono.map(x -> transformation(x)).flux()
and then another rule / pattern for cases similar to this (the push down):
mono.flux().map(x -> transformation(x)) -> mono.map(x -> transformation(x)).flux()

At some point we might be able to go back to a mono (e.g. mono.flux().last() -> mono) (the dissolve).

I have to think about how many such cases there are. On one hand not adding the .flux() and generating a compile error might force someone to reevaluate if a flux is truly necessary and if so manually fix it by adding the .flux(). Just adding it automatically without properly optimizing it after might hide some things.

Hmm, it seems that the rule MonoFlatMapToFlux does interfere with this rule (see build failure).

The mentioned rule would rewrite mono.flatMapMany(x -> Mono.just(transformation(x)))
-> mono.flatMap(x -> Mono.just(transformation(x))).flux()

Then this rule would rewrite.
mono.flatMap(x -> Mono.just(transformation(x))).flux() -> mono.map(x -> transformation(x)).flux()

So we get there in the end. I've removed the flatMapMany cases.

Stephan202 · 2022-06-24T07:25:18Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

+   * Prefer {@link Mono#map(Function)} over alternatives that unnecessarily require an inner
+   * subscription.
+   */
+  abstract static class MonoMap<T, S> {


We can have two similar methods for Flux#map and Flux#mapNotNull :)

Is it okay if it is in the same PR?
Also, as far as a can see there are many possibilities here.

Flux<S> before(Flux<T> flux) { return Refaster.anyOf( flux.flatMap(x -> Flux.just(transformation(x))), flux.flatMap(x -> Mono.just(transformation(x))), flux.concatMap(x -> Flux.just(transformation(x))), flux.concatMap(x -> Mono.just(transformation(x))) ); }

These are just a few but potentially we might want to include more (e.g. for switchMap or concatMapDelayError). Also each of the above has multiple overloads that would also be valid candidates. How do we usually deal with such cases?

Is it okay if it is in the same PR?

As the changes are very similar this is fine 👍🏻 .

How do we usually deal with such cases?

There are a few template collections where we list many variants in the Refaster.anyOf. For example, see AssertJTemplates.AssertThatObjectEnumerableIsEmpty. It depends on how many overloads there are, it could always be a BugPattern.

I'm not that good in Reactor that I can now say that we should and/or can fix all these cases and whether they are behavior preserving 🤔.

Still, by using multiple before templates (I assume that is required), we could potentially flag a nice amount of violations with this Refaster template.

The FluxFlatMapUsage check introduced in #26 will disallow the two suggested flatMap cases. It seems unlikely that users will use {Flux,Mono}.just i.c.w. an explicit concurrency level. One could argue that similarly it's unlikely that we find such concatMap cases, but I actually found a few. So yeah, let's do this.

Added some more cases (I've checked and I couldn't find any other rules that would disallow them)

flux.concatMap(x -> Mono.just(transformation(x)), prefetch), flux.concatMap(x -> Flux.just(transformation(x)), prefetch), flux.concatMapDelayError(x -> Mono.just(transformation(x))), flux.concatMapDelayError(x -> Flux.just(transformation(x))),

cernat-catalin · 2022-06-24T11:47:59Z

Thanks @Stephan202 for the suggestions (both the explanation and the applying) :)

Stephan202

Rebased and added a commit. The build will now fail due to the extra templates without corresponding test cases. I.c.w. the fromSupplier cases perhaps it does make sense to create BugChecker instead.

(Sorry for the slow review!)

Stephan202 · 2022-07-31T12:52:32Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

+   * Prefer {@link Mono#map(Function)} over alternatives that unnecessarily require an inner
+   * subscription.
+   */
+  abstract static class MonoMap<T, S> {


The FluxFlatMapUsage check introduced in #26 will disallow the two suggested flatMap cases. It seems unlikely that users will use {Flux,Mono}.just i.c.w. an explicit concurrency level. One could argue that similarly it's unlikely that we find such concatMap cases, but I actually found a few. So yeah, let's do this.

Stephan202 · 2022-07-31T13:42:57Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ReactorTemplates.java

+
+    @BeforeTemplate
+    Mono<S> before(Mono<T> mono) {
+      return mono.flatMap(x -> Mono.just(transformation(x)));


But the above would in some situations change the type of the expression from Flux to Mono resulting in compilation errors. Is that something that we consider acceptable?

Good point. Ideally we indeed avoid this, though the similarity between the Mono and Flux APIs might allow us to get away with this in many cases. If we want to avoid this we could instead have a separate rule that adds .flux(). (And then have separate Refaster rules (or a check) which "pushes down"/"dissolves" the .flux() where possible.

cernat-catalin · 2022-08-14T19:46:31Z

Pushed a suggestion to MonoMap and MonoMapNotNull and added the missing tests. Sorry for the late response!

cernat-catalin · 2022-08-14T19:47:20Z

Rebased and added a commit. The build will now fail due to the extra templates without corresponding test cases. I.c.w. the fromSupplier cases perhaps it does make sense to create BugChecker instead.

Could you detail an example regarding fromSupplier ? I can't seem to wrap my head around it.

Stephan202 · 2022-08-14T19:56:26Z

Pushed a suggestion to MonoMap and MonoMapNotNull and added the missing tests. Sorry for the late response!

No worries! There's no urgency 😄. I'll add a re-review of this PR to my TODO list (might take a bit, though 🤞).

Could you detail an example regarding fromSupplier ? I can't seem to wrap my head around it.

Hah, it also took me a while to remember what I meant here 😅 . So I think what I meant is that expression such as flux.concatMap(x -> Mono.fromSupplier(() -> transformation(x))) might indicate that user tried to perform some null handling, and instead meant to do flux.mapNotNull(x -> transformation(x)).

cernat-catalin · 2022-08-14T19:59:33Z

Hah, it also took me a while to remember what I meant here 😅 . So I think what I meant is that expression such as flux.concatMap(x -> Mono.fromSupplier(() -> transformation(x))) might indicate that user tried to perform some null handling, and instead meant to do flux.mapNotNull(x -> transformation(x)).

Aah, that makes sense. Don't think I've ever saw that but might be worth covering it

Stephan202

Rebased and added a commit to cover the Mono.fromSupplier case. We should certainly start looking into a "smarter" approach, but this is a good step forward, so let's go! 🚀

Suggested commit message:

Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules (#142)

cernat-catalin · 2022-10-23T19:56:18Z

Thanks @Stephan202 for the changes!

rickie

Really curious to see how this will change things downstream.

Thanks for another Reactor PR @cernat-catalin 🚀 !

cernat-catalin requested review from Stephan202 and rickie June 22, 2022 07:24

Stephan202 reviewed Jun 24, 2022

View reviewed changes

Stephan202 force-pushed the cernat-catalin/mono_flatmapjust_check branch from 1779980 to 85f05df Compare July 31, 2022 13:56

Stephan202 reviewed Jul 31, 2022

View reviewed changes

cernat-catalin requested a review from Stephan202 August 14, 2022 19:56

Stephan202 force-pushed the cernat-catalin/mono_flatmapjust_check branch from f82e8f0 to 39bd678 Compare October 15, 2022 14:50

Stephan202 approved these changes Oct 15, 2022

View reviewed changes

Stephan202 added this to the 0.5.0 milestone Oct 15, 2022

rickie added the new feature label Oct 26, 2022

cernat-catalin and others added 7 commits October 26, 2022 10:53

Prefer mono.map over mono.flatMap(x -> Mono.just())

f6f09fd

Apply formatter

a58daf9

Suggestions

1830689

Some notes

d5bcff8

Add missing cases; Add tests

69de3b1

Remove from rule

146b8df

Suggestions

456cae9

rickie force-pushed the cernat-catalin/mono_flatmapjust_check branch from 39bd678 to 456cae9 Compare October 26, 2022 08:53

rickie approved these changes Oct 26, 2022

View reviewed changes

rickie changed the title ~~Prefer mono.map over mono.flatMap(x -> Mono.just())~~ Introduce {Mono,Flux}Map{,NotNull} Refaster rules Oct 26, 2022

rickie merged commit afb2a28 into master Oct 26, 2022

rickie deleted the cernat-catalin/mono_flatmapjust_check branch October 26, 2022 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules #142

Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules #142

cernat-catalin commented Jun 22, 2022

rickie commented Jun 23, 2022

Stephan202 left a comment

Stephan202 left a comment

Stephan202 Jun 24, 2022

cernat-catalin Jun 24, 2022

rickie Jun 30, 2022

Stephan202 Jul 31, 2022

cernat-catalin Aug 14, 2022

cernat-catalin Aug 14, 2022

Stephan202 Jun 24, 2022

cernat-catalin Jun 24, 2022

rickie Jun 30, 2022

Stephan202 Jul 31, 2022

cernat-catalin Aug 14, 2022

cernat-catalin commented Jun 24, 2022

Stephan202 left a comment

Stephan202 Jul 31, 2022

Stephan202 Jul 31, 2022

cernat-catalin commented Aug 14, 2022

cernat-catalin commented Aug 14, 2022

Stephan202 commented Aug 14, 2022

cernat-catalin commented Aug 14, 2022

Stephan202 left a comment •

edited by rickie

Loading

cernat-catalin commented Oct 23, 2022

rickie left a comment

Introduce {Mono,Flux}Map{,NotNull} Refaster rules #142

Introduce {Mono,Flux}Map{,NotNull} Refaster rules #142

Conversation

cernat-catalin commented Jun 22, 2022

rickie commented Jun 23, 2022

Stephan202 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cernat-catalin commented Jun 24, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cernat-catalin commented Aug 14, 2022

cernat-catalin commented Aug 14, 2022

Stephan202 commented Aug 14, 2022

cernat-catalin commented Aug 14, 2022

Stephan202 left a comment • edited by rickie Loading

Choose a reason for hiding this comment

cernat-catalin commented Oct 23, 2022

rickie left a comment

Choose a reason for hiding this comment

Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules #142

Introduce `{Mono,Flux}Map{,NotNull}` Refaster rules #142

Stephan202 left a comment •

edited by rickie

Loading