From a4e7a8bae0e822a47f61b8edec8e3d9a77ae3d58 Mon Sep 17 00:00:00 2001 From: Moritz Halbritter Date: Thu, 19 Jan 2023 17:00:53 +0100 Subject: [PATCH] Use bean name for servlet and filter registrations if name is not set If the name is set, it is used. If not, the bean name is used. If that's not set, the convention based name is taken. Registration failures now throw an IllegalStateException instead of being logged. DynamicRegistrationBean.setIgnoreRegistrationFailure can be used to restore the old behavior. See gh-33911 --- .../jersey/JerseyAutoConfiguration.java | 3 +- ...ebServerFactoryAutoConfigurationTests.java | 5 +- .../web/servlet/DynamicRegistrationBean.java | 46 +++++++++++-- .../AbstractFilterRegistrationBeanTests.java | 28 +++++++- ...atingFilterProxyRegistrationBeanTests.java | 7 +- .../servlet/DynamicRegistrationBeanTests.java | 66 +++++++++++++++++++ .../servlet/FilterRegistrationBeanTests.java | 4 +- .../servlet/ServletRegistrationBeanTests.java | 24 +++---- ...rvletWebServerApplicationContextTests.java | 3 +- .../SpringBootServletInitializerTests.java | 5 ++ 10 files changed, 165 insertions(+), 26 deletions(-) create mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DynamicRegistrationBeanTests.java diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jersey/JerseyAutoConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jersey/JerseyAutoConfiguration.java index 04541d1dfe..2f739bf74f 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jersey/JerseyAutoConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jersey/JerseyAutoConfiguration.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -144,6 +144,7 @@ public class JerseyAutoConfiguration implements ServletContextAware { addInitParameters(registration); registration.setName(getServletRegistrationName()); registration.setLoadOnStartup(this.jersey.getServlet().getLoadOnStartup()); + registration.setIgnoreRegistrationFailure(true); return registration; } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryAutoConfigurationTests.java index 6d0e1e226c..55978a63f0 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/servlet/ServletWebServerFactoryAutoConfigurationTests.java @@ -501,7 +501,10 @@ class ServletWebServerFactoryAutoConfigurationTests { @Bean(name = DispatcherServletAutoConfiguration.DEFAULT_DISPATCHER_SERVLET_REGISTRATION_BEAN_NAME) ServletRegistrationBean dispatcherRegistration(DispatcherServlet dispatcherServlet) { - return new ServletRegistrationBean<>(dispatcherServlet, "/app/*"); + ServletRegistrationBean registration = new ServletRegistrationBean<>(dispatcherServlet, + "/app/*"); + registration.setName(DispatcherServletAutoConfiguration.DEFAULT_DISPATCHER_SERVLET_BEAN_NAME); + return registration; } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/DynamicRegistrationBean.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/DynamicRegistrationBean.java index 01f49a9326..1ac1fa8d4a 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/DynamicRegistrationBean.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/servlet/DynamicRegistrationBean.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -24,6 +24,7 @@ import jakarta.servlet.ServletContext; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.BeanNameAware; import org.springframework.core.Conventions; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -34,9 +35,11 @@ import org.springframework.util.StringUtils; * * @param the dynamic registration result * @author Phillip Webb + * @author Moritz Halbritter * @since 2.0.0 */ -public abstract class DynamicRegistrationBean extends RegistrationBean { +public abstract class DynamicRegistrationBean extends RegistrationBean + implements BeanNameAware { private static final Log logger = LogFactory.getLog(RegistrationBean.class); @@ -46,6 +49,10 @@ public abstract class DynamicRegistrationBean ex private Map initParameters = new LinkedHashMap<>(); + private String beanName; + + private boolean ignoreRegistrationFailure; + /** * Set the name of this registration. If not specified the bean name will be used. * @param name the name of the registration @@ -106,12 +113,32 @@ public abstract class DynamicRegistrationBean ex protected final void register(String description, ServletContext servletContext) { D registration = addRegistration(description, servletContext); if (registration == null) { - logger.info(StringUtils.capitalize(description) + " was not registered (possibly already registered?)"); - return; + if (this.ignoreRegistrationFailure) { + logger.info(StringUtils.capitalize(description) + " was not registered (possibly already registered?)"); + return; + } + throw new IllegalStateException( + "Failed to register '%s' on the servlet context. Possibly already registered?" + .formatted(description)); } configure(registration); } + /** + * Sets whether registration failures should be ignored. If set to true, a failure + * will be logged. If set to false, an {@link IllegalStateException} will be thrown. + * @param ignoreRegistrationFailure whether to ignore registration failures + * @since 3.1.0 + */ + public void setIgnoreRegistrationFailure(boolean ignoreRegistrationFailure) { + this.ignoreRegistrationFailure = ignoreRegistrationFailure; + } + + @Override + public void setBeanName(String name) { + this.beanName = name; + } + protected abstract D addRegistration(String description, ServletContext servletContext); protected void configure(D registration) { @@ -123,12 +150,19 @@ public abstract class DynamicRegistrationBean ex /** * Deduces the name for this registration. Will return user specified name or fallback - * to convention based naming. + * to the bean name. If the bean name is not available, convention based naming is + * used. * @param value the object used for convention based names * @return the deduced name */ protected final String getOrDeduceName(Object value) { - return (this.name != null) ? this.name : Conventions.getVariableName(value); + if (this.name != null) { + return this.name; + } + if (this.beanName != null) { + return this.beanName; + } + return Conventions.getVariableName(value); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/AbstractFilterRegistrationBeanTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/AbstractFilterRegistrationBeanTests.java index 16f0ece123..f39c8298c7 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/AbstractFilterRegistrationBeanTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/AbstractFilterRegistrationBeanTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -32,7 +32,9 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; @@ -94,6 +96,7 @@ abstract class AbstractFilterRegistrationBeanTests { @Test void specificName() throws Exception { + given(this.servletContext.addFilter(anyString(), any(Filter.class))).willReturn(this.registration); AbstractFilterRegistrationBean bean = createFilterRegistrationBean(); bean.setName("specificName"); bean.onStartup(this.servletContext); @@ -102,6 +105,7 @@ abstract class AbstractFilterRegistrationBeanTests { @Test void deducedName() throws Exception { + given(this.servletContext.addFilter(anyString(), any(Filter.class))).willReturn(this.registration); AbstractFilterRegistrationBean bean = createFilterRegistrationBean(); bean.onStartup(this.servletContext); then(this.servletContext).should().addFilter(eq("mockFilter"), getExpectedFilter()); @@ -198,6 +202,28 @@ abstract class AbstractFilterRegistrationBeanTests { then(this.registration).should().addMappingForUrlPatterns(types, false, "/*"); } + @Test + void failsWithDoubleRegistration() { + assertThatThrownBy(() -> { + AbstractFilterRegistrationBean bean = createFilterRegistrationBean(); + bean.setName("double-registration"); + given(this.servletContext.addFilter(anyString(), any(Filter.class))).willReturn(null); + bean.onStartup(this.servletContext); + }).isInstanceOf(IllegalStateException.class).hasMessage( + "Failed to register 'filter double-registration' on the servlet context. Possibly already registered?"); + } + + @Test + void doesntFailIfDoubleRegistrationIsIgnored() { + assertThatCode(() -> { + AbstractFilterRegistrationBean bean = createFilterRegistrationBean(); + bean.setName("double-registration"); + given(this.servletContext.addFilter(anyString(), any(Filter.class))).willReturn(null); + bean.setIgnoreRegistrationFailure(true); + bean.onStartup(this.servletContext); + }).doesNotThrowAnyException(); + } + protected abstract Filter getExpectedFilter(); protected abstract AbstractFilterRegistrationBean createFilterRegistrationBean( diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DelegatingFilterProxyRegistrationBeanTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DelegatingFilterProxyRegistrationBeanTests.java index 25e6ded7c0..e63b7d2193 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DelegatingFilterProxyRegistrationBeanTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DelegatingFilterProxyRegistrationBeanTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -87,9 +87,8 @@ class DelegatingFilterProxyRegistrationBeanTests extends AbstractFilterRegistrat @Test void createServletRegistrationBeanMustNotBeNull() { - assertThatIllegalArgumentException() - .isThrownBy( - () -> new DelegatingFilterProxyRegistrationBean("mockFilter", (ServletRegistrationBean[]) null)) + assertThatIllegalArgumentException().isThrownBy( + () -> new DelegatingFilterProxyRegistrationBean("mockFilter", (ServletRegistrationBean[]) null)) .withMessageContaining("ServletRegistrationBeans must not be null"); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DynamicRegistrationBeanTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DynamicRegistrationBeanTests.java new file mode 100644 index 0000000000..70d598c95e --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/DynamicRegistrationBeanTests.java @@ -0,0 +1,66 @@ +/* + * Copyright 2012-2023 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.web.servlet; + +import jakarta.servlet.Registration.Dynamic; +import jakarta.servlet.ServletContext; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link DynamicRegistrationBean}. + * + * @author Moritz Halbritter + */ +class DynamicRegistrationBeanTests { + + @Test + void shouldUseNameIfSet() { + DynamicRegistrationBean bean = createBean(); + bean.setName("givenName"); + assertThat(bean.getOrDeduceName("dummy")).isEqualTo("givenName"); + } + + @Test + void shouldUseBeanNameIfNameIsNotSet() { + DynamicRegistrationBean bean = createBean(); + bean.setBeanName("beanName"); + assertThat(bean.getOrDeduceName("dummy")).isEqualTo("beanName"); + } + + @Test + void shouldUseConventionBasedNameIfNoNameOrBeanNameIsSet() { + DynamicRegistrationBean bean = createBean(); + assertThat(bean.getOrDeduceName("dummy")).isEqualTo("string"); + } + + private static DynamicRegistrationBean createBean() { + return new DynamicRegistrationBean() { + @Override + protected Dynamic addRegistration(String description, ServletContext servletContext) { + return null; + } + + @Override + protected String getDescription() { + return null; + } + }; + } + +} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/FilterRegistrationBeanTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/FilterRegistrationBeanTests.java index 78eaea6ff6..b6a4245f59 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/FilterRegistrationBeanTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/FilterRegistrationBeanTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -41,6 +41,7 @@ import static org.mockito.BDDMockito.then; * Tests for {@link FilterRegistrationBean}. * * @author Phillip Webb + * @author Moritz Halbritter */ class FilterRegistrationBeanTests extends AbstractFilterRegistrationBeanTests { @@ -58,6 +59,7 @@ class FilterRegistrationBeanTests extends AbstractFilterRegistrationBeanTests { @Test void setFilter() throws Exception { + given(this.servletContext.addFilter(anyString(), any(Filter.class))).willReturn(this.registration); FilterRegistrationBean bean = new FilterRegistrationBean<>(); bean.setFilter(this.filter); bean.onStartup(this.servletContext); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/ServletRegistrationBeanTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/ServletRegistrationBeanTests.java index a3a98ff62d..b1d0fc7b54 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/ServletRegistrationBeanTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/ServletRegistrationBeanTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2023 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. @@ -22,7 +22,6 @@ import java.util.HashMap; import java.util.LinkedHashSet; import java.util.Map; -import jakarta.servlet.FilterRegistration; import jakarta.servlet.Servlet; import jakarta.servlet.ServletContext; import jakarta.servlet.ServletRegistration; @@ -34,6 +33,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.boot.web.servlet.mock.MockServlet; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.BDDMockito.given; @@ -56,9 +56,6 @@ class ServletRegistrationBeanTests { @Mock private ServletRegistration.Dynamic registration; - @Mock - private FilterRegistration.Dynamic filterRegistration; - @Test void startupWithDefaults() throws Exception { given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(this.registration); @@ -70,12 +67,14 @@ class ServletRegistrationBeanTests { } @Test - void startupWithDoubleRegistration() throws Exception { - ServletRegistrationBean bean = new ServletRegistrationBean<>(this.servlet); - given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(null); - bean.onStartup(this.servletContext); - then(this.servletContext).should().addServlet("mockServlet", this.servlet); - then(this.registration).should(never()).setAsyncSupported(true); + void failsWithDoubleRegistration() { + assertThatThrownBy(() -> { + ServletRegistrationBean bean = new ServletRegistrationBean<>(this.servlet); + bean.setName("double-registration"); + given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(null); + bean.onStartup(this.servletContext); + }).isInstanceOf(IllegalStateException.class).hasMessage( + "Failed to register 'servlet double-registration' on the servlet context. Possibly already registered?"); } @Test @@ -103,6 +102,7 @@ class ServletRegistrationBeanTests { @Test void specificName() throws Exception { + given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(this.registration); ServletRegistrationBean bean = new ServletRegistrationBean<>(); bean.setName("specificName"); bean.setServlet(this.servlet); @@ -112,6 +112,7 @@ class ServletRegistrationBeanTests { @Test void deducedName() throws Exception { + given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(this.registration); ServletRegistrationBean bean = new ServletRegistrationBean<>(); bean.setServlet(this.servlet); bean.onStartup(this.servletContext); @@ -182,6 +183,7 @@ class ServletRegistrationBeanTests { @Test void withoutDefaultMappings() throws Exception { + given(this.servletContext.addServlet(anyString(), any(Servlet.class))).willReturn(this.registration); ServletRegistrationBean bean = new ServletRegistrationBean<>(this.servlet, false); bean.onStartup(this.servletContext); then(this.registration).should(never()).addMapping(any(String[].class)); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java index caef4cd7a4..340abefe48 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/context/ServletWebServerApplicationContextTests.java @@ -237,13 +237,14 @@ class ServletWebServerApplicationContextTests { OrderedFilter filter = new OrderedFilter(); this.context.registerBeanDefinition("filterBean", beanDefinition(filter)); FilterRegistrationBean registration = new FilterRegistrationBean<>(); + registration.setName("filterBeanRegistration"); registration.setFilter(mock(Filter.class)); registration.setOrder(100); this.context.registerBeanDefinition("filterRegistrationBean", beanDefinition(registration)); this.context.refresh(); MockServletWebServerFactory factory = getWebServerFactory(); then(factory.getServletContext()).should().addFilter("filterBean", filter); - then(factory.getServletContext()).should().addFilter("object", registration.getFilter()); + then(factory.getServletContext()).should().addFilter("filterBeanRegistration", registration.getFilter()); assertThat(factory.getRegisteredFilter(0).getFilter()).isEqualTo(filter); } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/support/SpringBootServletInitializerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/support/SpringBootServletInitializerTests.java index 84143d97c7..33169b092b 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/support/SpringBootServletInitializerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/web/servlet/support/SpringBootServletInitializerTests.java @@ -23,6 +23,8 @@ import java.util.Vector; import java.util.concurrent.atomic.AtomicBoolean; import jakarta.servlet.DispatcherType; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterRegistration.Dynamic; import jakarta.servlet.ServletContext; import jakarta.servlet.ServletContextEvent; import jakarta.servlet.ServletContextListener; @@ -55,6 +57,7 @@ import org.springframework.web.context.support.StandardServletEnvironment; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.mock; @@ -189,6 +192,7 @@ class SpringBootServletInitializerTests { @Test void servletContextPropertySourceIsAvailablePriorToRefresh() { ServletContext servletContext = mock(ServletContext.class); + given(servletContext.addFilter(any(), any(Filter.class))).willReturn(mock(Dynamic.class)); given(servletContext.getInitParameterNames()) .willReturn(Collections.enumeration(Collections.singletonList("spring.profiles.active"))); given(servletContext.getInitParameter("spring.profiles.active")).willReturn("from-servlet-context"); @@ -202,6 +206,7 @@ class SpringBootServletInitializerTests { @Test void whenServletContextIsDestroyedThenJdbcDriversAreDeregistered() throws ServletException { ServletContext servletContext = mock(ServletContext.class); + given(servletContext.addFilter(any(), any(Filter.class))).willReturn(mock(Dynamic.class)); given(servletContext.getInitParameterNames()).willReturn(new Vector().elements()); given(servletContext.getAttributeNames()).willReturn(new Vector().elements()); AtomicBoolean driversDeregistered = new AtomicBoolean();