From e33211c6d7d1cc9346d2660826f13a5cc17c7bda Mon Sep 17 00:00:00 2001 From: Ismail Alammar <33684781+ismailalammar@users.noreply.github.com> Date: Thu, 5 May 2022 23:23:40 +0800 Subject: [PATCH] Include classname of null-returning `map` function in NPE msg (#2984) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit logs the class name of the mapper provided to FluxMap, FluxMapFuseable and FluxMapSignal operators in the NB: The later is exposed in the API as `flatMap(Function,Function,Function)`. Fixes #2982. Co-authored-by: Simon Baslé --- .../java/reactor/core/publisher/FluxMap.java | 20 ++-- .../core/publisher/FluxMapFuseable.java | 20 ++-- .../reactor/core/publisher/FluxMapSignal.java | 20 ++-- .../core/publisher/FluxMapSignalTest.java | 40 +++++++- .../reactor/core/publisher/FluxMapTest.java | 97 ++++++++++++++++--- 5 files changed, 164 insertions(+), 33 deletions(-) diff --git a/reactor-core/src/main/java/reactor/core/publisher/FluxMap.java b/reactor-core/src/main/java/reactor/core/publisher/FluxMap.java index d89f36b6dd..0f4fc1fcd6 100644 --- a/reactor-core/src/main/java/reactor/core/publisher/FluxMap.java +++ b/reactor-core/src/main/java/reactor/core/publisher/FluxMap.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016-2021 VMware Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2016-2022 VMware Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -103,8 +103,10 @@ public void onNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { Throwable e_ = Operators.onNextError(t, e, actual.currentContext(), s); @@ -203,8 +205,10 @@ public void onNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { Throwable e_ = Operators.onNextError(t, e, actual.currentContext(), s); @@ -230,8 +234,10 @@ public boolean tryOnNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } return actual.tryOnNext(v); } catch (Throwable e) { diff --git a/reactor-core/src/main/java/reactor/core/publisher/FluxMapFuseable.java b/reactor-core/src/main/java/reactor/core/publisher/FluxMapFuseable.java index 2d8db3d10b..51289ff262 100644 --- a/reactor-core/src/main/java/reactor/core/publisher/FluxMapFuseable.java +++ b/reactor-core/src/main/java/reactor/core/publisher/FluxMapFuseable.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016-2021 VMware Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2016-2022 VMware Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -110,8 +110,10 @@ public void onNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { Throwable e_ = Operators.onNextError(t, e, actual.currentContext(), s); @@ -278,8 +280,10 @@ public void onNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { Throwable e_ = Operators.onNextError(t, e, actual.currentContext(), s); @@ -306,8 +310,10 @@ public boolean tryOnNext(T t) { R v; try { - v = Objects.requireNonNull(mapper.apply(t), - "The mapper returned a null value."); + v = mapper.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapper.getClass().getName() + "] returned a null value."); + } return actual.tryOnNext(v); } catch (Throwable e) { diff --git a/reactor-core/src/main/java/reactor/core/publisher/FluxMapSignal.java b/reactor-core/src/main/java/reactor/core/publisher/FluxMapSignal.java index 6cd467e1a6..0a09b1f3d9 100644 --- a/reactor-core/src/main/java/reactor/core/publisher/FluxMapSignal.java +++ b/reactor-core/src/main/java/reactor/core/publisher/FluxMapSignal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016-2021 VMware Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2016-2022 VMware Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -141,8 +141,10 @@ public void onNext(T t) { R v; try { - v = Objects.requireNonNull(mapperNext.apply(t), - "The mapper returned a null value."); + v = mapperNext.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapperNext.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { done = true; @@ -171,8 +173,10 @@ public void onError(Throwable t) { R v; try { - v = Objects.requireNonNull(mapperError.apply(t), - "The mapper returned a null value."); + v = mapperError.apply(t); + if (v == null) { + throw new NullPointerException("The mapper [" + mapperError.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { done = true; @@ -203,8 +207,10 @@ public void onComplete() { R v; try { - v = Objects.requireNonNull(mapperComplete.get(), - "The mapper returned a null value."); + v = mapperComplete.get(); + if (v == null) { + throw new NullPointerException("The mapper [" + mapperComplete.getClass().getName() + "] returned a null value."); + } } catch (Throwable e) { done = true; diff --git a/reactor-core/src/test/java/reactor/core/publisher/FluxMapSignalTest.java b/reactor-core/src/test/java/reactor/core/publisher/FluxMapSignalTest.java index 581efb8d27..41e785b4b5 100644 --- a/reactor-core/src/test/java/reactor/core/publisher/FluxMapSignalTest.java +++ b/reactor-core/src/test/java/reactor/core/publisher/FluxMapSignalTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2021 VMware Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2015-2022 VMware Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -135,6 +135,44 @@ public void flatMapSignal2() { .verifyComplete(); } + @Test + void mapOnNextRejectsNull() { + FluxMapTest.NullFunction> mapper = new FluxMapTest.NullFunction<>(); + Flux.just("test") + .flatMap(mapper, null, null) + .as(StepVerifier::create) + .verifyErrorSatisfies(err -> assertThat(err) + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullFunction] returned a null value.") + ); + } + + @Test + void mapOnErrorRejectsNull() { + final IllegalStateException originalException = new IllegalStateException("expected"); + FluxMapTest.NullFunction> mapper = new FluxMapTest.NullFunction<>(); + Flux.error(originalException) + .flatMap(null, mapper, null) + .as(StepVerifier::create) + .verifyErrorSatisfies(err -> assertThat(err) + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullFunction] returned a null value.") + .hasSuppressedException(originalException) + ); + } + + @Test + void mapOnCompleteRejectsNull() { + FluxMapTest.NullSupplier> mapper = new FluxMapTest.NullSupplier<>(); + Flux.just("test") + .flatMap(null, null, mapper) + .as(StepVerifier::create) + .verifyErrorSatisfies(err -> assertThat(err) + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullSupplier] returned a null value.") + ); + } + @Test public void scanOperator(){ Flux parent = Flux.just(1); diff --git a/reactor-core/src/test/java/reactor/core/publisher/FluxMapTest.java b/reactor-core/src/test/java/reactor/core/publisher/FluxMapTest.java index e7fa0fd2bb..0c415bdb8b 100644 --- a/reactor-core/src/test/java/reactor/core/publisher/FluxMapTest.java +++ b/reactor-core/src/test/java/reactor/core/publisher/FluxMapTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016-2021 VMware Inc. or its affiliates, All Rights Reserved. + * Copyright (c) 2016-2022 VMware Inc. or its affiliates, All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,8 +21,14 @@ import java.util.List; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicLong; +import java.util.function.Function; +import java.util.function.Supplier; +import java.util.stream.Stream; +import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.Named; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestFactory; import org.mockito.Mockito; import org.reactivestreams.Subscription; @@ -33,9 +39,11 @@ import reactor.test.StepVerifier; import reactor.test.publisher.FluxOperatorTest; import reactor.test.subscriber.AssertSubscriber; +import reactor.test.subscriber.ConditionalTestSubscriber; +import reactor.test.subscriber.TestSubscriber; +import reactor.util.annotation.Nullable; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.*; import static reactor.core.publisher.Sinks.EmitFailureHandler.FAIL_FAST; public class FluxMapTest extends FluxOperatorTest { @@ -125,16 +133,65 @@ public void mapperThrows() { .assertNotComplete(); } - @Test - public void mapperReturnsNull() { - AssertSubscriber ts = AssertSubscriber.create(); + @TestFactory + Stream mapperReturnsNull() { + Function mapper = new NullFunction<>(); - just.map(v -> null) - .subscribe(ts); + Stream>> sources = Stream.of( + Named.named("normal", Flux.just(1).hide()), + Named.named("fused", Flux.just(1)) + ); - ts.assertError(NullPointerException.class) - .assertNoValues() - .assertNotComplete(); + return DynamicTest.stream(sources, src -> { + Flux underTest = src.map(mapper); + + underTest + .as(StepVerifier::create) + .verifyErrorSatisfies(err -> assertThat(err) + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullFunction] returned a null value.") + ); + }); + } + + @TestFactory + Stream mapperReturnsNullConditional() { + Function mapper = new NullFunction<>(); + + Stream> fusionModes = Stream.of( + Named.named("normal", false), + Named.named("fused", true) + ); + + return DynamicTest.stream(fusionModes, fused -> { + //the error will terminate the downstream, so we need two pairs: normal path and tryOnNext path + ConditionalTestSubscriber testSubscriberTryPath = TestSubscriber.builder().buildConditional(i -> true); + ConditionalTestSubscriber testSubscriberNormalPath = TestSubscriber.builder().buildConditional(i -> true); + Fuseable.ConditionalSubscriber conditionalSubscriberNormalPath; + Fuseable.ConditionalSubscriber conditionalSubscriberTryPath; + if (fused) { + conditionalSubscriberNormalPath = new FluxMapFuseable.MapFuseableConditionalSubscriber<>(testSubscriberNormalPath, mapper); + conditionalSubscriberTryPath = new FluxMapFuseable.MapFuseableConditionalSubscriber<>(testSubscriberTryPath, mapper); + } + else { + conditionalSubscriberNormalPath = new FluxMap.MapConditionalSubscriber<>(testSubscriberNormalPath, mapper); + conditionalSubscriberTryPath = new FluxMap.MapConditionalSubscriber<>(testSubscriberTryPath, mapper); + } + + //test the non-conditional path + conditionalSubscriberNormalPath.onNext(1); + assertThat(testSubscriberNormalPath.expectTerminalError()) + .as("normal path") + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullFunction] returned a null value."); + + //test the conditional path + conditionalSubscriberTryPath.tryOnNext(2); + assertThat(testSubscriberTryPath.expectTerminalError()) + .as("try path") + .isInstanceOf(NullPointerException.class) + .hasMessage("The mapper [reactor.core.publisher.FluxMapTest$NullFunction] returned a null value."); + }); } @Test @@ -514,4 +571,22 @@ public void mapFuseableTryOnNextFailureStrategyResume() { Hooks.resetOnNextError(); } } + + static class NullFunction implements Function { + + @Nullable + @Override + public R apply(T t) { + return null; + } + } + + static class NullSupplier implements Supplier { + + @Nullable + @Override + public T get() { + return null; + } + } }