Have `FluxFlatMapUsage` better handle nested `Publisher`s #224

eliashenko · 2022-09-06T09:10:49Z

Introduce check for calling flatMap() on Flux or Flux

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java

Stephan202

Tnx for this! Indeed important that for GroupedFlux we don't default to suggesting concatMap. Left some thoughts :)

Stephan202 · 2022-09-08T14:36:14Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java

+    if (NestedTypesUtils.isSameTypeNested(FLUX, tree, state)) {
+      return buildDescription(tree)
+          .addFix(maxConcurrencyFix)
+          .setMessage(
+              "`Flux#flatMap` and `Flux#flatMapSequential` have subtle semantics;"
+                  + " please explicitly specify the desired amount of concurrency")
+          .build();
+    }


I guess this rule applies to any nested Publisher, or just when GroupedFlux is the outer type. If the idea is that users will generally want to deal with a Flux<Publisher> by performing parallel subscriptions, then generalizing to the former might be nicer. Not sure.

^ In either case we shouldn't reuse NestedTypesUtils.isSameTypeNested, but IMHO that's not a bad thing; I suspect that a more general matcher for types of T<A, B, C, ...> may be more useful. (But perhaps that's out of scope for this PR 😄.)

Stephan202 · 2022-09-08T14:37:54Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java

+              "`Flux#flatMap` and `Flux#flatMapSequential` have subtle semantics;"
+                  + " please explicitly specify the desired amount of concurrency")
+          .build();
+    }

    return buildDescription(tree)
        .addFix(SuggestedFixes.renameMethodInvocation(tree, "concatMap", state))


Rather than omitting this fix, how about we make it the second fix? That would simplify the code and allow us to make fewer assumptions. (In this case patching will rewrite the code to use the bounded-concurrency flatMap, as desired.)

(In this case we wouldn't need a custom message above.)

^ I'll push a proposal for this. Of course still open to discuss the alternative!

rickie · 2022-09-10T13:31:14Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageTest.java

@@ -33,6 +33,10 @@ void identification() {
            "    Flux.just(1).flatMapSequential(Flux::just);",
            "    // BUG: Diagnostic contains:",
            "    Flux.just(1).<String>flatMapSequential(i -> Flux.just(String.valueOf(i)));",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(\"test_1\", \"test\").groupBy(String::length).flatMap(Flux::just);",


Suggested change

" Flux.just(\"test_1\", \"test\").groupBy(String::length).flatMap(Flux::just);",

" Flux.just(1, 2).groupBy(i -> i).flatMap(Flux::just);",

The other tests are using ints so I suggest to do the same here :).

rickie · 2022-09-10T15:12:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NestedTypesUtils.java

+import com.sun.tools.javac.util.List;
+
+/** Utility class that can be used to identify nesting of the same type. */
+public final class NestedTypesUtils {


We don't really use Utils as suffix for Util classes, so pushing something.

Final name TBD, maybe we can generalize this 👀.

Stephan202

Rebased and added a commit with a proposal. Note that most of the changes are also part of #234; the idea is to merge that PR first.

Stephan202 · 2022-09-11T14:31:12Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsageTest.java

            "  void m() {",
            "    Flux.just(1).concatMap(Flux::just);",
            "    Flux.just(1).concatMap(Flux::just);",
+            "    Flux.just(1, 2).groupBy(i -> i).flatMap(Flux::just, MAX_CONCURRENCY);",
+            "    Flux.just(1, 2).groupBy(i -> i).flatMapSequential(Flux::just, MAX_CONCURRENCY);",
            "  }",
            "}")
        .doTest();


Suggested change

.doTest();

.doTest(TEXT_MATCH);

@rickie this is another one of those things for which we should introduce a check.

Stephan202 · 2022-09-11T18:03:43Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java

+              "`Flux#flatMap` and `Flux#flatMapSequential` have subtle semantics;"
+                  + " please explicitly specify the desired amount of concurrency")
+          .build();
+    }

    return buildDescription(tree)
        .addFix(SuggestedFixes.renameMethodInvocation(tree, "concatMap", state))


^ I'll push a proposal for this. Of course still open to discuss the alternative!

Stephan202 · 2022-09-11T18:05:52Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/NestedTypes.java

+/** Utility class that can be used to identify nesting of the same type. */
+public final class NestedTypes {
+  private NestedTypes() {}


This approach seems rather niche, as more complex types wouldn't be supported. I think I found another approach. Will push a proposal to this PR, but since it's quite some code, the idea is that we'll merge those changes in a separate PR first, then rebase this PR.

NB: We should get into the habit of writing tests also for these utility classes.

Stephan202

I squashed the changes and rebased the PR on top of #234. The original state of the branch was tagged as tmp/eliashenko-PRP-12517-2202-10-15. No further suggestions from my side; I like this proposal!

Suggested commit message:

Have `FluxFlatMapUsage` better handle nested `Publisher`s (#224)

rickie · 2022-10-26T08:28:12Z

Rebased and resolved conflicts.

rickie

Really nice addition. Congrats on your first contribution 🚀 !

Sorry for taking a long time to review the MoreTypes PR 😬.

rickie added the improvement label Sep 6, 2022

rickie reviewed Sep 6, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FluxFlatMapUsage.java Outdated Show resolved Hide resolved

eliashenko force-pushed the eliashenko/PRP-12517 branch 2 times, most recently from 07c7915 to dcd5811 Compare September 8, 2022 13:14

Stephan202 reviewed Sep 8, 2022

View reviewed changes

rickie reviewed Sep 10, 2022

View reviewed changes

Stephan202 mentioned this pull request Sep 11, 2022

Introduce MoreTypes utility class #234

Merged

Stephan202 force-pushed the eliashenko/PRP-12517 branch from 0016ccf to 9b4006b Compare September 11, 2022 18:09

Stephan202 reviewed Sep 11, 2022

View reviewed changes

Stephan202 added this to the 0.5.0 milestone Oct 15, 2022

Stephan202 force-pushed the eliashenko/PRP-12517 branch from 9b4006b to 6a1085f Compare October 15, 2022 13:47

Stephan202 approved these changes Oct 15, 2022

View reviewed changes

Stephan202 force-pushed the eliashenko/PRP-12517 branch from 6a1085f to c110c92 Compare October 21, 2022 22:35

Stephan202 changed the base branch from master to sschroevers/introduce-moretypes October 21, 2022 22:36

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from df41c50 to 79ee60b Compare October 25, 2022 15:07

Base automatically changed from sschroevers/introduce-moretypes to master October 25, 2022 15:13

rickie and others added 3 commits October 26, 2022 10:25

Suggestions

8846ee4

Tweaks

0c184fc

Introduce check for calling flatMap() on Flux<GroupedFlux>

743c5a0

rickie force-pushed the eliashenko/PRP-12517 branch from c110c92 to 743c5a0 Compare October 26, 2022 08:28

rickie approved these changes Oct 26, 2022

View reviewed changes

rickie changed the title ~~Introduce check for calling flatMap() on Flux<GroupedFlux>~~ Have FluxFlatMapUsage better handle nested Publishers Oct 26, 2022

rickie merged commit 2196bbd into master Oct 26, 2022

rickie deleted the eliashenko/PRP-12517 branch October 26, 2022 08:40

rickie added new feature improvement and removed improvement new feature labels Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have `FluxFlatMapUsage` better handle nested `Publisher`s #224

Have `FluxFlatMapUsage` better handle nested `Publisher`s #224

eliashenko commented Sep 6, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Sep 8, 2022

Stephan202 Sep 8, 2022

Stephan202 Sep 11, 2022

rickie Sep 10, 2022

rickie Sep 10, 2022

Stephan202 left a comment

Stephan202 Sep 11, 2022

Stephan202 Sep 11, 2022

Stephan202 Sep 11, 2022

Stephan202 left a comment

rickie commented Oct 26, 2022

rickie left a comment

	" Flux.just(\"test_1\", \"test\").groupBy(String::length).flatMap(Flux::just);",
	" Flux.just(1, 2).groupBy(i -> i).flatMap(Flux::just);",

Have FluxFlatMapUsage better handle nested Publishers #224

Have FluxFlatMapUsage better handle nested Publishers #224

Conversation

eliashenko commented Sep 6, 2022 • edited Loading

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

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

Stephan202 left a comment

Choose a reason for hiding this comment

rickie commented Oct 26, 2022

rickie left a comment

Choose a reason for hiding this comment

Have `FluxFlatMapUsage` better handle nested `Publisher`s #224

Have `FluxFlatMapUsage` better handle nested `Publisher`s #224

eliashenko commented Sep 6, 2022 •

edited

Loading