From a424081803818a5ab634826e215bc922de9764f1 Mon Sep 17 00:00:00 2001 From: "Rubinson,Ethan(erubinson)" Date: Wed, 17 May 2017 17:15:16 -0700 Subject: [PATCH 1/2] Copy conversion service when performing environment conversion Previously, when a web environment was converted to a StandardEnvironment, any customizations of the source environment's ConversionService were lost. This commit updates the logic that performs the conversion to copy the source's ConversionService to the converted environment, thereby ensuring that any customizations are retained. Closes gh-9259 See gh-9246 --- .../boot/EnvironmentConverter.java | 91 ++++++++++++++ .../boot/SpringApplication.java | 36 +----- .../boot/EnvironmentConverterTest.java | 112 ++++++++++++++++++ 3 files changed, 205 insertions(+), 34 deletions(-) create mode 100644 spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java create mode 100644 spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java diff --git a/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java b/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java new file mode 100644 index 00000000000..22d81910f85 --- /dev/null +++ b/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java @@ -0,0 +1,91 @@ +/* + * Copyright 2012-2017 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 + * + * http://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; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.MutablePropertySources; +import org.springframework.core.env.PropertySource; +import org.springframework.core.env.StandardEnvironment; +import org.springframework.web.context.support.StandardServletEnvironment; + +/** + * Utility class for converting one environment type to another. + * + * @author Ethan Rubinson + * @since 1.5.4 + */ +final class EnvironmentConverter { + + private static final Set SERVLET_ENVIRONMENT_SOURCE_NAMES; + + static { + final Set names = new HashSet(); + names.add(StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME); + names.add(StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME); + names.add(StandardServletEnvironment.JNDI_PROPERTY_SOURCE_NAME); + SERVLET_ENVIRONMENT_SOURCE_NAMES = Collections.unmodifiableSet(names); + } + + private EnvironmentConverter() { + + } + + /** + * Converts the specified environment to a {@link StandardEnvironment}. + * + * @param environment The environment to convert. + * @return The converted environment. + */ + protected static ConfigurableEnvironment convertToStandardEnvironment( + ConfigurableEnvironment environment) { + final StandardEnvironment result = new StandardEnvironment(); + + /* Copy the profiles */ + result.setActiveProfiles(environment.getActiveProfiles()); + + /* Copy the conversion service */ + result.setConversionService(environment.getConversionService()); + + /* + * Copy over all of the property sources except those unrelated to a standard + * environment + */ + removeAllPropertySources(result.getPropertySources()); + for (PropertySource propertySource : environment.getPropertySources()) { + if (!SERVLET_ENVIRONMENT_SOURCE_NAMES.contains(propertySource.getName())) { + result.getPropertySources().addLast(propertySource); + } + } + + return result; + } + + private static void removeAllPropertySources(MutablePropertySources propertySources) { + final Set names = new HashSet(); + for (PropertySource propertySource : propertySources) { + names.add(propertySource.getName()); + } + for (String name : names) { + propertySources.remove(name); + } + } + +} diff --git a/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java b/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java index a7234a97ef8..38eef24fe2e 100644 --- a/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java +++ b/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java @@ -140,6 +140,7 @@ import org.springframework.web.context.support.StandardServletEnvironment; * @author Jeremy Rickard * @author Craig Burke * @author Michael Simons + * @author Ethan Rubinson * @see #run(Object, String[]) * @see #run(Object[], String[]) * @see #SpringApplication(Object...) @@ -177,16 +178,6 @@ public class SpringApplication { private static final String SYSTEM_PROPERTY_JAVA_AWT_HEADLESS = "java.awt.headless"; - private static final Set SERVLET_ENVIRONMENT_SOURCE_NAMES; - - static { - Set names = new HashSet(); - names.add(StandardServletEnvironment.SERVLET_CONTEXT_PROPERTY_SOURCE_NAME); - names.add(StandardServletEnvironment.SERVLET_CONFIG_PROPERTY_SOURCE_NAME); - names.add(StandardServletEnvironment.JNDI_PROPERTY_SOURCE_NAME); - SERVLET_ENVIRONMENT_SOURCE_NAMES = Collections.unmodifiableSet(names); - } - private static final Log logger = LogFactory.getLog(SpringApplication.class); private final Set sources = new LinkedHashSet(); @@ -335,7 +326,7 @@ public class SpringApplication { configureEnvironment(environment, applicationArguments.getSourceArgs()); listeners.environmentPrepared(environment); if (isWebEnvironment(environment) && !this.webEnvironment) { - environment = convertToStandardEnvironment(environment); + environment = EnvironmentConverter.convertToStandardEnvironment(environment); } return environment; } @@ -465,29 +456,6 @@ public class SpringApplication { } } - private ConfigurableEnvironment convertToStandardEnvironment( - ConfigurableEnvironment environment) { - StandardEnvironment result = new StandardEnvironment(); - removeAllPropertySources(result.getPropertySources()); - result.setActiveProfiles(environment.getActiveProfiles()); - for (PropertySource propertySource : environment.getPropertySources()) { - if (!SERVLET_ENVIRONMENT_SOURCE_NAMES.contains(propertySource.getName())) { - result.getPropertySources().addLast(propertySource); - } - } - return result; - } - - private void removeAllPropertySources(MutablePropertySources propertySources) { - Set names = new HashSet(); - for (PropertySource propertySource : propertySources) { - names.add(propertySource.getName()); - } - for (String name : names) { - propertySources.remove(name); - } - } - /** * Add, remove or re-order any {@link PropertySource}s in this application's * environment. diff --git a/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java b/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java new file mode 100644 index 00000000000..4ec2e16de28 --- /dev/null +++ b/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java @@ -0,0 +1,112 @@ +/* + * Copyright 2012-2017 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 + * + * http://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; + +import org.bouncycastle.util.Arrays; +import org.junit.Assert; +import org.junit.Test; + +import org.springframework.core.convert.TypeDescriptor; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.ConverterFactory; +import org.springframework.core.convert.converter.GenericConverter; +import org.springframework.core.convert.support.ConfigurableConversionService; +import org.springframework.core.env.AbstractEnvironment; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.mock.env.MockEnvironment; + +/** + * Tests for the {@link EnvironmentConverter} methods + * + * @author Ethan Rubinson + */ +public class EnvironmentConverterTest { + + @Test + public void testConvertAbstractEnvironmentToStandardEnvironment() throws Exception { + final AbstractEnvironment baseEnv = new MockEnvironment(); + final CustomConversionService customConverterServce = new CustomConversionService( + baseEnv.getConversionService()); + final String[] activeProfiles = new String[] { "activeProfile1", "activeProfile2" }; + baseEnv.setActiveProfiles(activeProfiles); + baseEnv.setConversionService(customConverterServce); + + ConfigurableEnvironment convertedEnv = EnvironmentConverter + .convertToStandardEnvironment(baseEnv); + Assert.assertTrue(Arrays.areEqual(activeProfiles, + convertedEnv.getActiveProfiles())); + Assert.assertEquals(customConverterServce, convertedEnv.getConversionService()); + } + + private class CustomConversionService implements ConfigurableConversionService { + + private final ConfigurableConversionService delegate; + + CustomConversionService(ConfigurableConversionService delegate) { + this.delegate = delegate; + } + + @Override + public boolean canConvert(Class sourceType, Class targetType) { + return this.delegate.canConvert(sourceType, targetType); + } + + @Override + public boolean canConvert(TypeDescriptor sourceType, TypeDescriptor targetType) { + return this.delegate.canConvert(sourceType, targetType); + } + + @Override + public T convert(Object source, Class targetType) { + return this.delegate.convert(source, targetType); + } + + @Override + public Object convert(Object source, TypeDescriptor sourceType, + TypeDescriptor targetType) { + return this.delegate.convert(source, sourceType, targetType); + } + + @Override + public void addConverter(Converter converter) { + this.delegate.addConverter(converter); + } + + @Override + public void addConverter(Class sourceType, Class targetType, + Converter converter) { + this.delegate.addConverter(sourceType, targetType, converter); + } + + @Override + public void addConverter(GenericConverter converter) { + this.delegate.addConverter(converter); + } + + @Override + public void addConverterFactory(ConverterFactory factory) { + this.delegate.addConverterFactory(factory); + } + + @Override + public void removeConvertible(Class sourceType, Class targetType) { + this.delegate.removeConvertible(sourceType, targetType); + } + + } + +} From 5b30269ac9d9d938bcb36cb29fe485fe5c9bb35b Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 7 Jun 2017 10:20:53 +0100 Subject: [PATCH 2/2] Polish "Copy conversion service when performing environment conversion" Closes gh-9246 --- .../boot/EnvironmentConverter.java | 75 ++++++++---- .../boot/SpringApplication.java | 18 +-- .../boot/EnvironmentConverterTest.java | 112 ------------------ .../boot/EnvironmentConverterTests.java | 79 ++++++++++++ 4 files changed, 135 insertions(+), 149 deletions(-) delete mode 100644 spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java create mode 100644 spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTests.java diff --git a/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java b/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java index 22d81910f85..8501759b41c 100644 --- a/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java +++ b/spring-boot/src/main/java/org/springframework/boot/EnvironmentConverter.java @@ -21,19 +21,24 @@ import java.util.HashSet; import java.util.Set; import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.Environment; import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySource; import org.springframework.core.env.StandardEnvironment; +import org.springframework.util.ClassUtils; +import org.springframework.web.context.ConfigurableWebEnvironment; import org.springframework.web.context.support.StandardServletEnvironment; /** - * Utility class for converting one environment type to another. + * Utility class for converting one type of {@link Environment} to another. * * @author Ethan Rubinson - * @since 1.5.4 + * @author Andy Wilkinson */ final class EnvironmentConverter { + private static final String CONFIGURABLE_WEB_ENVIRONMENT_CLASS = "org.springframework.web.context.ConfigurableWebEnvironment"; + private static final Set SERVLET_ENVIRONMENT_SOURCE_NAMES; static { @@ -44,42 +49,68 @@ final class EnvironmentConverter { SERVLET_ENVIRONMENT_SOURCE_NAMES = Collections.unmodifiableSet(names); } - private EnvironmentConverter() { + private final ClassLoader classLoader; + /** + * Creates a new {@link EnvironmentConverter} that will use the given + * {@code classLoader} during conversion. + * @param classLoader the class loader to use + */ + EnvironmentConverter(ClassLoader classLoader) { + this.classLoader = classLoader; } /** - * Converts the specified environment to a {@link StandardEnvironment}. + * Converts the given {@code environment} to a {@link StandardEnvironment}. If the + * environment is already a {@code StandardEnvironment} and is not a + * {@link ConfigurableWebEnvironment} no conversion is performed and it is returned + * unchanged. * - * @param environment The environment to convert. - * @return The converted environment. + * @param environment The Environment to convert + * @return The converted Environment */ - protected static ConfigurableEnvironment convertToStandardEnvironment( + StandardEnvironment convertToStandardEnvironmentIfNecessary( ConfigurableEnvironment environment) { - final StandardEnvironment result = new StandardEnvironment(); + if (environment instanceof StandardEnvironment + && !isWebEnvironment(environment, this.classLoader)) { + return (StandardEnvironment) environment; + } + return convertToStandardEnvironment(environment); + } - /* Copy the profiles */ - result.setActiveProfiles(environment.getActiveProfiles()); + private boolean isWebEnvironment(ConfigurableEnvironment environment, + ClassLoader classLoader) { + try { + Class webEnvironmentClass = ClassUtils + .forName(CONFIGURABLE_WEB_ENVIRONMENT_CLASS, classLoader); + return (webEnvironmentClass.isInstance(environment)); + } + catch (Throwable ex) { + return false; + } + } - /* Copy the conversion service */ + private StandardEnvironment convertToStandardEnvironment( + ConfigurableEnvironment environment) { + StandardEnvironment result = new StandardEnvironment(); + result.setActiveProfiles(environment.getActiveProfiles()); result.setConversionService(environment.getConversionService()); + copyNonServletPropertySources(environment, result); + return result; + } - /* - * Copy over all of the property sources except those unrelated to a standard - * environment - */ - removeAllPropertySources(result.getPropertySources()); - for (PropertySource propertySource : environment.getPropertySources()) { + private void copyNonServletPropertySources(ConfigurableEnvironment source, + StandardEnvironment target) { + removeAllPropertySources(target.getPropertySources()); + for (PropertySource propertySource : source.getPropertySources()) { if (!SERVLET_ENVIRONMENT_SOURCE_NAMES.contains(propertySource.getName())) { - result.getPropertySources().addLast(propertySource); + target.getPropertySources().addLast(propertySource); } } - - return result; } - private static void removeAllPropertySources(MutablePropertySources propertySources) { - final Set names = new HashSet(); + private void removeAllPropertySources(MutablePropertySources propertySources) { + Set names = new HashSet(); for (PropertySource propertySource : propertySources) { names.add(propertySource.getName()); } diff --git a/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java b/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java index 38eef24fe2e..eb33939e4f2 100644 --- a/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java +++ b/spring-boot/src/main/java/org/springframework/boot/SpringApplication.java @@ -174,8 +174,6 @@ public class SpringApplication { */ public static final String BANNER_LOCATION_PROPERTY = SpringApplicationBannerPrinter.BANNER_LOCATION_PROPERTY; - private static final String CONFIGURABLE_WEB_ENVIRONMENT_CLASS = "org.springframework.web.context.ConfigurableWebEnvironment"; - private static final String SYSTEM_PROPERTY_JAVA_AWT_HEADLESS = "java.awt.headless"; private static final Log logger = LogFactory.getLog(SpringApplication.class); @@ -325,8 +323,9 @@ public class SpringApplication { ConfigurableEnvironment environment = getOrCreateEnvironment(); configureEnvironment(environment, applicationArguments.getSourceArgs()); listeners.environmentPrepared(environment); - if (isWebEnvironment(environment) && !this.webEnvironment) { - environment = EnvironmentConverter.convertToStandardEnvironment(environment); + if (!this.webEnvironment) { + environment = new EnvironmentConverter(getClassLoader()) + .convertToStandardEnvironmentIfNecessary(environment); } return environment; } @@ -445,17 +444,6 @@ public class SpringApplication { configureProfiles(environment, args); } - private boolean isWebEnvironment(ConfigurableEnvironment environment) { - try { - Class webEnvironmentClass = ClassUtils - .forName(CONFIGURABLE_WEB_ENVIRONMENT_CLASS, getClassLoader()); - return (webEnvironmentClass.isInstance(environment)); - } - catch (Throwable ex) { - return false; - } - } - /** * Add, remove or re-order any {@link PropertySource}s in this application's * environment. diff --git a/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java b/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java deleted file mode 100644 index 4ec2e16de28..00000000000 --- a/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java +++ /dev/null @@ -1,112 +0,0 @@ -/* - * Copyright 2012-2017 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 - * - * http://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; - -import org.bouncycastle.util.Arrays; -import org.junit.Assert; -import org.junit.Test; - -import org.springframework.core.convert.TypeDescriptor; -import org.springframework.core.convert.converter.Converter; -import org.springframework.core.convert.converter.ConverterFactory; -import org.springframework.core.convert.converter.GenericConverter; -import org.springframework.core.convert.support.ConfigurableConversionService; -import org.springframework.core.env.AbstractEnvironment; -import org.springframework.core.env.ConfigurableEnvironment; -import org.springframework.mock.env.MockEnvironment; - -/** - * Tests for the {@link EnvironmentConverter} methods - * - * @author Ethan Rubinson - */ -public class EnvironmentConverterTest { - - @Test - public void testConvertAbstractEnvironmentToStandardEnvironment() throws Exception { - final AbstractEnvironment baseEnv = new MockEnvironment(); - final CustomConversionService customConverterServce = new CustomConversionService( - baseEnv.getConversionService()); - final String[] activeProfiles = new String[] { "activeProfile1", "activeProfile2" }; - baseEnv.setActiveProfiles(activeProfiles); - baseEnv.setConversionService(customConverterServce); - - ConfigurableEnvironment convertedEnv = EnvironmentConverter - .convertToStandardEnvironment(baseEnv); - Assert.assertTrue(Arrays.areEqual(activeProfiles, - convertedEnv.getActiveProfiles())); - Assert.assertEquals(customConverterServce, convertedEnv.getConversionService()); - } - - private class CustomConversionService implements ConfigurableConversionService { - - private final ConfigurableConversionService delegate; - - CustomConversionService(ConfigurableConversionService delegate) { - this.delegate = delegate; - } - - @Override - public boolean canConvert(Class sourceType, Class targetType) { - return this.delegate.canConvert(sourceType, targetType); - } - - @Override - public boolean canConvert(TypeDescriptor sourceType, TypeDescriptor targetType) { - return this.delegate.canConvert(sourceType, targetType); - } - - @Override - public T convert(Object source, Class targetType) { - return this.delegate.convert(source, targetType); - } - - @Override - public Object convert(Object source, TypeDescriptor sourceType, - TypeDescriptor targetType) { - return this.delegate.convert(source, sourceType, targetType); - } - - @Override - public void addConverter(Converter converter) { - this.delegate.addConverter(converter); - } - - @Override - public void addConverter(Class sourceType, Class targetType, - Converter converter) { - this.delegate.addConverter(sourceType, targetType, converter); - } - - @Override - public void addConverter(GenericConverter converter) { - this.delegate.addConverter(converter); - } - - @Override - public void addConverterFactory(ConverterFactory factory) { - this.delegate.addConverterFactory(factory); - } - - @Override - public void removeConvertible(Class sourceType, Class targetType) { - this.delegate.removeConvertible(sourceType, targetType); - } - - } - -} diff --git a/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTests.java b/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTests.java new file mode 100644 index 00000000000..d65ef80211e --- /dev/null +++ b/spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTests.java @@ -0,0 +1,79 @@ +/* + * Copyright 2012-2017 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 + * + * http://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; + +import org.junit.Test; + +import org.springframework.core.convert.support.ConfigurableConversionService; +import org.springframework.core.env.AbstractEnvironment; +import org.springframework.core.env.StandardEnvironment; +import org.springframework.mock.env.MockEnvironment; +import org.springframework.web.context.support.StandardServletEnvironment; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +/** + * Tests for {@link EnvironmentConverter}. + * + * @author Ethan Rubinson + * @author Andy Wilkinson + */ +public class EnvironmentConverterTests { + + private final EnvironmentConverter environmentConverter = new EnvironmentConverter( + getClass().getClassLoader()); + + @Test + public void convertedEnvironmentHasSameActiveProfiles() { + AbstractEnvironment originalEnvironment = new MockEnvironment(); + originalEnvironment.setActiveProfiles("activeProfile1", "activeProfile2"); + StandardEnvironment convertedEnvironment = this.environmentConverter + .convertToStandardEnvironmentIfNecessary(originalEnvironment); + assertThat(convertedEnvironment.getActiveProfiles()) + .containsExactly("activeProfile1", "activeProfile2"); + } + + @Test + public void convertedEnvironmentHasSameConversionService() { + AbstractEnvironment originalEnvironment = new MockEnvironment(); + ConfigurableConversionService conversionService = mock( + ConfigurableConversionService.class); + originalEnvironment.setConversionService(conversionService); + StandardEnvironment convertedEnvironment = this.environmentConverter + .convertToStandardEnvironmentIfNecessary(originalEnvironment); + assertThat(convertedEnvironment.getConversionService()) + .isEqualTo(conversionService); + } + + @Test + public void standardEnvironmentIsReturnedUnconverted() { + StandardEnvironment standardEnvironment = new StandardEnvironment(); + StandardEnvironment convertedEnvironment = this.environmentConverter + .convertToStandardEnvironmentIfNecessary(standardEnvironment); + assertThat(convertedEnvironment).isSameAs(standardEnvironment); + } + + @Test + public void standardServletEnvironmentIsConverted() { + StandardServletEnvironment standardServletEnvironment = new StandardServletEnvironment(); + StandardEnvironment convertedEnvironment = this.environmentConverter + .convertToStandardEnvironmentIfNecessary(standardServletEnvironment); + assertThat(convertedEnvironment).isNotSameAs(standardServletEnvironment); + } + +}