Polish "Copy conversion service when performing environment conversion"

Closes gh-9246

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<String> 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<String> names = new HashSet<String>();
+	private void removeAllPropertySources(MutablePropertySources propertySources) {
+		Set<String> names = new HashSet<String>();
 		for (PropertySource<?> propertySource : propertySources) {
 			names.add(propertySource.getName());
 		}

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.

spring-boot/src/test/java/org/springframework/boot/EnvironmentConverterTest.java

@@ -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> T convert(Object source, Class<T> 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 <S, T> void addConverter(Class<S> sourceType, Class<T> targetType,
-				Converter<? super S, ? extends T> 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);
-		}
-
-	}
-
-}

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);
+	}
+
+}