From 986c4fd9264929a64c0defc406a45a6a2284f91c Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Wed, 13 Mar 2024 14:31:33 +0100 Subject: [PATCH] Revise use of Objects.requireNonNull() Historically, we have rarely intentionally thrown a NullPointerException in the Spring Framework. Instead, we prefer to throw either an IllegalArgumentException or IllegalStateException instead of a NullPointerException. However, changes to the code in recent times have introduced the use of Objects.requireNonNull(Object) which throws a NullPointerException without an explicit error message. The latter ends up providing less context than a NullPointerException thrown by the JVM (since Java 14) due to actually de-referencing a null-pointer. See https://openjdk.org/jeps/358. In light of that, this commit revises our current use of Objects.requireNonNull(Object) by removing it or replacing it with Assert.notNull(). However, we still use Objects.requireNonNull(T, String) in a few places where we are required to throw a NullPointerException in order to comply with a third-party contract such as Reactive Streams. Closes gh-32430 --- .../java/org/springframework/core/CoroutinesUtils.java | 4 ++-- .../org/springframework/core/NamedThreadLocal.java | 6 +++--- .../springframework/core/ReactiveAdapterRegistry.java | 7 +++---- .../core/io/buffer/OutputStreamPublisher.java | 4 +++- .../test/web/reactive/server/CookieAssertions.java | 8 +++++--- .../test/web/reactive/server/HeaderAssertions.java | 8 +++++--- .../context/bean/override/OverrideMetadataTests.java | 3 +-- .../convention/TestBeanOverrideProcessorTests.java | 10 ++++++---- .../http/client/OutputStreamPublisher.java | 4 +++- .../AbstractNamedValueMethodArgumentResolver.java | 5 +++-- .../annotation/AbstractNamedValueArgumentResolver.java | 5 +++-- 11 files changed, 37 insertions(+), 27 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java b/spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java index 3b69ff38e4..30c0d475f1 100644 --- a/spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java +++ b/spring-core/src/main/java/org/springframework/core/CoroutinesUtils.java @@ -19,7 +19,6 @@ package org.springframework.core; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Map; -import java.util.Objects; import kotlin.Unit; import kotlin.coroutines.CoroutineContext; @@ -112,7 +111,8 @@ public abstract class CoroutinesUtils { public static Publisher invokeSuspendingFunction(CoroutineContext context, Method method, Object target, Object... args) { Assert.isTrue(KotlinDetector.isSuspendingFunction(method), "'method' must be a suspending function"); - KFunction function = Objects.requireNonNull(ReflectJvmMapping.getKotlinFunction(method)); + KFunction function = ReflectJvmMapping.getKotlinFunction(method); + Assert.notNull(function, () -> "Failed to get Kotlin function for method: " + method); if (method.isAccessible() && !KCallablesJvm.isAccessible(function)) { KCallablesJvm.setAccessible(function, true); } diff --git a/spring-core/src/main/java/org/springframework/core/NamedThreadLocal.java b/spring-core/src/main/java/org/springframework/core/NamedThreadLocal.java index ada54b5174..8388389aab 100644 --- a/spring-core/src/main/java/org/springframework/core/NamedThreadLocal.java +++ b/spring-core/src/main/java/org/springframework/core/NamedThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,7 +16,6 @@ package org.springframework.core; -import java.util.Objects; import java.util.function.Supplier; import org.springframework.util.Assert; @@ -76,7 +75,8 @@ public class NamedThreadLocal extends ThreadLocal { SuppliedNamedThreadLocal(String name, Supplier supplier) { super(name); - this.supplier = Objects.requireNonNull(supplier); + Assert.notNull(supplier, "Supplier must not be null"); + this.supplier = supplier; } @Override diff --git a/spring-core/src/main/java/org/springframework/core/ReactiveAdapterRegistry.java b/spring-core/src/main/java/org/springframework/core/ReactiveAdapterRegistry.java index 994b6e036e..c5b925b64c 100644 --- a/spring-core/src/main/java/org/springframework/core/ReactiveAdapterRegistry.java +++ b/spring-core/src/main/java/org/springframework/core/ReactiveAdapterRegistry.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,7 +19,6 @@ package org.springframework.core; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; @@ -383,8 +382,8 @@ public class ReactiveAdapterRegistry { Method multiPublisher = ClassUtils.getMethod( io.smallrye.mutiny.groups.MultiCreate.class, "publisher", Flow.Publisher.class); registry.registerReactiveType(uniDesc, - uni -> FlowAdapters.toPublisher((Flow.Publisher) Objects.requireNonNull( - ReflectionUtils.invokeMethod(uniToPublisher, ((io.smallrye.mutiny.Uni) uni).convert()))), + uni -> FlowAdapters.toPublisher((Flow.Publisher) + ReflectionUtils.invokeMethod(uniToPublisher, ((io.smallrye.mutiny.Uni) uni).convert())), publisher -> ReflectionUtils.invokeMethod(uniPublisher, io.smallrye.mutiny.Uni.createFrom(), FlowAdapters.toFlowPublisher(publisher))); registry.registerReactiveType(multiDesc, diff --git a/spring-core/src/main/java/org/springframework/core/io/buffer/OutputStreamPublisher.java b/spring-core/src/main/java/org/springframework/core/io/buffer/OutputStreamPublisher.java index 17e5d2cc29..1c46c5ad93 100644 --- a/spring-core/src/main/java/org/springframework/core/io/buffer/OutputStreamPublisher.java +++ b/spring-core/src/main/java/org/springframework/core/io/buffer/OutputStreamPublisher.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -66,6 +66,8 @@ final class OutputStreamPublisher implements Publisher { @Override public void subscribe(Subscriber subscriber) { + // We don't use Assert.notNull(), because a NullPointerException is required + // for Reactive Streams compliance. Objects.requireNonNull(subscriber, "Subscriber must not be null"); OutputStreamSubscription subscription = new OutputStreamSubscription( diff --git a/spring-test/src/main/java/org/springframework/test/web/reactive/server/CookieAssertions.java b/spring-test/src/main/java/org/springframework/test/web/reactive/server/CookieAssertions.java index d87162e1f6..e29e4e5295 100644 --- a/spring-test/src/main/java/org/springframework/test/web/reactive/server/CookieAssertions.java +++ b/spring-test/src/main/java/org/springframework/test/web/reactive/server/CookieAssertions.java @@ -17,7 +17,6 @@ package org.springframework.test.web.reactive.server; import java.time.Duration; -import java.util.Objects; import java.util.function.Consumer; import org.hamcrest.Matcher; @@ -213,10 +212,13 @@ public class CookieAssertions { private ResponseCookie getCookie(String name) { ResponseCookie cookie = this.exchangeResult.getResponseCookies().getFirst(name); - if (cookie == null) { + if (cookie != null) { + return cookie; + } + else { this.exchangeResult.assertWithDiagnostics(() -> fail("No cookie with name '" + name + "'")); } - return Objects.requireNonNull(cookie); + throw new IllegalStateException("This code path should not be reachable"); } private static String getMessage(String cookie) { diff --git a/spring-test/src/main/java/org/springframework/test/web/reactive/server/HeaderAssertions.java b/spring-test/src/main/java/org/springframework/test/web/reactive/server/HeaderAssertions.java index 66ac0730a2..fe4abf6c85 100644 --- a/spring-test/src/main/java/org/springframework/test/web/reactive/server/HeaderAssertions.java +++ b/spring-test/src/main/java/org/springframework/test/web/reactive/server/HeaderAssertions.java @@ -19,7 +19,6 @@ package org.springframework.test.web.reactive.server; import java.net.URI; import java.util.Arrays; import java.util.List; -import java.util.Objects; import java.util.function.Consumer; import org.hamcrest.Matcher; @@ -200,10 +199,13 @@ public class HeaderAssertions { private List getRequiredValues(String name) { List values = getHeaders().get(name); - if (CollectionUtils.isEmpty(values)) { + if (!CollectionUtils.isEmpty(values)) { + return values; + } + else { this.exchangeResult.assertWithDiagnostics(() -> fail(getMessage(name) + " not found")); } - return Objects.requireNonNull(values); + throw new IllegalStateException("This code path should not be reachable"); } /** diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/OverrideMetadataTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/OverrideMetadataTests.java index 731250bcb4..a9538133ec 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/OverrideMetadataTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/OverrideMetadataTests.java @@ -18,7 +18,6 @@ package org.springframework.test.context.bean.override; import java.lang.annotation.Annotation; import java.lang.reflect.Field; -import java.util.Objects; import org.junit.jupiter.api.Test; @@ -49,7 +48,7 @@ class OverrideMetadataTests { private static OverrideMetadata exampleOverride() throws Exception { Field field = OverrideMetadataTests.class.getDeclaredField("annotated"); - return new ConcreteOverrideMetadata(Objects.requireNonNull(field), field.getAnnotation(NonNull.class), + return new ConcreteOverrideMetadata(field, field.getAnnotation(NonNull.class), ResolvableType.forClass(String.class), BeanOverrideStrategy.REPLACE_DEFINITION); } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessorTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessorTests.java index 29032d7d72..b18ac5ea44 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessorTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessorTests.java @@ -19,7 +19,6 @@ package org.springframework.test.context.bean.override.convention; import java.lang.reflect.Field; import java.lang.reflect.Method; import java.util.List; -import java.util.Objects; import org.junit.jupiter.api.Test; @@ -91,7 +90,8 @@ class TestBeanOverrideProcessorTests { Class clazz = ExplicitMethodNameConf.class; Class returnType = ExampleService.class; Field field = clazz.getField("a"); - TestBean overrideAnnotation = Objects.requireNonNull(field.getAnnotation(TestBean.class)); + TestBean overrideAnnotation = field.getAnnotation(TestBean.class); + assertThat(overrideAnnotation).isNotNull(); TestBeanOverrideProcessor processor = new TestBeanOverrideProcessor(); assertThatIllegalStateException() @@ -106,7 +106,8 @@ class TestBeanOverrideProcessorTests { void createMetaDataForKnownExplicitMethod() throws Exception { Class returnType = ExampleService.class; Field field = ExplicitMethodNameConf.class.getField("b"); - TestBean overrideAnnotation = Objects.requireNonNull(field.getAnnotation(TestBean.class)); + TestBean overrideAnnotation = field.getAnnotation(TestBean.class); + assertThat(overrideAnnotation).isNotNull(); TestBeanOverrideProcessor processor = new TestBeanOverrideProcessor(); assertThat(processor.createMetadata(field, overrideAnnotation, ResolvableType.forClass(returnType))) @@ -117,7 +118,8 @@ class TestBeanOverrideProcessorTests { void createMetaDataWithDeferredCheckForExistenceOfConventionBasedFactoryMethod() throws Exception { Class returnType = ExampleService.class; Field field = MethodConventionConf.class.getField("field"); - TestBean overrideAnnotation = Objects.requireNonNull(field.getAnnotation(TestBean.class)); + TestBean overrideAnnotation = field.getAnnotation(TestBean.class); + assertThat(overrideAnnotation).isNotNull(); TestBeanOverrideProcessor processor = new TestBeanOverrideProcessor(); // When in convention-based mode, createMetadata() will not verify that diff --git a/spring-web/src/main/java/org/springframework/http/client/OutputStreamPublisher.java b/spring-web/src/main/java/org/springframework/http/client/OutputStreamPublisher.java index 67b33cd8fc..84eb00e8d9 100644 --- a/spring-web/src/main/java/org/springframework/http/client/OutputStreamPublisher.java +++ b/spring-web/src/main/java/org/springframework/http/client/OutputStreamPublisher.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -150,6 +150,8 @@ final class OutputStreamPublisher implements Flow.Publisher { @Override public void subscribe(Flow.Subscriber subscriber) { + // We don't use Assert.notNull(), because a NullPointerException is required + // for Reactive Streams compliance. Objects.requireNonNull(subscriber, "Subscriber must not be null"); OutputStreamSubscription subscription = new OutputStreamSubscription<>(subscriber, this.outputStreamHandler, diff --git a/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java index 332852691e..34dfa10edb 100644 --- a/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java @@ -18,7 +18,6 @@ package org.springframework.web.method.annotation; import java.lang.reflect.Method; import java.util.Map; -import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import jakarta.servlet.ServletException; @@ -35,6 +34,7 @@ import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.core.KotlinDetector; import org.springframework.core.MethodParameter; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; import org.springframework.web.bind.ServletRequestBindingException; import org.springframework.web.bind.WebDataBinder; import org.springframework.web.bind.annotation.ValueConstants; @@ -342,7 +342,8 @@ public abstract class AbstractNamedValueMethodArgumentResolver implements Handle * or an optional parameter (with a default value in the Kotlin declaration). */ public static boolean hasDefaultValue(MethodParameter parameter) { - Method method = Objects.requireNonNull(parameter.getMethod()); + Method method = parameter.getMethod(); + Assert.notNull(method, () -> "Retrieved null method from MethodParameter: " + parameter); KFunction function = ReflectJvmMapping.getKotlinFunction(method); if (function != null) { int index = 0; diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractNamedValueArgumentResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractNamedValueArgumentResolver.java index 4aff5cfd2c..10a217c121 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractNamedValueArgumentResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractNamedValueArgumentResolver.java @@ -18,7 +18,6 @@ package org.springframework.web.reactive.result.method.annotation; import java.lang.reflect.Method; import java.util.Map; -import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import kotlin.reflect.KFunction; @@ -37,6 +36,7 @@ import org.springframework.core.MethodParameter; import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.lang.Nullable; import org.springframework.ui.Model; +import org.springframework.util.Assert; import org.springframework.web.bind.WebDataBinder; import org.springframework.web.bind.annotation.ValueConstants; import org.springframework.web.reactive.BindingContext; @@ -334,7 +334,8 @@ public abstract class AbstractNamedValueArgumentResolver extends HandlerMethodAr * or an optional parameter (with a default value in the Kotlin declaration). */ public static boolean hasDefaultValue(MethodParameter parameter) { - Method method = Objects.requireNonNull(parameter.getMethod()); + Method method = parameter.getMethod(); + Assert.notNull(method, () -> "Retrieved null method from MethodParameter: " + parameter); KFunction function = ReflectJvmMapping.getKotlinFunction(method); if (function != null) { int index = 0;