From 57fbdc0ad5991e206e6fffb076069a7fa4cbf406 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 27 Jul 2017 16:02:18 +0200 Subject: [PATCH] Consistent logging in Environment and PropertySource implementations Issue: SPR-15825 (cherry picked from commit fac83b2) --- .../core/env/AbstractEnvironment.java | 26 ++++++-------- .../core/env/AbstractPropertyResolver.java | 6 ++-- .../core/env/CompositePropertySource.java | 2 +- .../MissingRequiredPropertiesException.java | 25 ++++++------- .../core/env/MutablePropertySources.java | 36 +++++++------------ .../core/env/PropertySource.java | 13 +++---- .../env/PropertySourcesPropertyResolver.java | 11 +++--- .../env/SystemEnvironmentPropertySource.java | 4 +-- .../core/env/PropertySourceTests.java | 32 ----------------- 9 files changed, 53 insertions(+), 102 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/env/AbstractEnvironment.java b/spring-core/src/main/java/org/springframework/core/env/AbstractEnvironment.java index 356a7a9143..4d00810b6b 100644 --- a/spring-core/src/main/java/org/springframework/core/env/AbstractEnvironment.java +++ b/spring-core/src/main/java/org/springframework/core/env/AbstractEnvironment.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-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. @@ -120,9 +120,8 @@ public abstract class AbstractEnvironment implements ConfigurableEnvironment { */ public AbstractEnvironment() { customizePropertySources(this.propertySources); - if (this.logger.isDebugEnabled()) { - this.logger.debug(String.format( - "Initialized %s with PropertySources %s", getClass().getSimpleName(), this.propertySources)); + if (logger.isDebugEnabled()) { + logger.debug("Initialized " + getClass().getSimpleName() + " with PropertySources " + this.propertySources); } } @@ -261,8 +260,8 @@ public abstract class AbstractEnvironment implements ConfigurableEnvironment { @Override public void addActiveProfile(String profile) { - if (this.logger.isDebugEnabled()) { - this.logger.debug(String.format("Activating profile '%s'", profile)); + if (logger.isDebugEnabled()) { + logger.debug("Activating profile '" + profile + "'"); } validateProfile(profile); doGetActiveProfiles(); @@ -392,9 +391,8 @@ public abstract class AbstractEnvironment implements ConfigurableEnvironment { } catch (AccessControlException ex) { if (logger.isInfoEnabled()) { - logger.info(String.format("Caught AccessControlException when accessing system " + - "environment variable [%s]; its value will be returned [null]. Reason: %s", - attributeName, ex.getMessage())); + logger.info("Caught AccessControlException when accessing system environment variable '" + + attributeName + "'; its value will be returned [null]. Reason: " + ex.getMessage()); } return null; } @@ -433,9 +431,8 @@ public abstract class AbstractEnvironment implements ConfigurableEnvironment { } catch (AccessControlException ex) { if (logger.isInfoEnabled()) { - logger.info(String.format("Caught AccessControlException when accessing system " + - "property [%s]; its value will be returned [null]. Reason: %s", - attributeName, ex.getMessage())); + logger.info("Caught AccessControlException when accessing system property '" + + attributeName + "'; its value will be returned [null]. Reason: " + ex.getMessage()); } return null; } @@ -574,9 +571,8 @@ public abstract class AbstractEnvironment implements ConfigurableEnvironment { @Override public String toString() { - return String.format("%s {activeProfiles=%s, defaultProfiles=%s, propertySources=%s}", - getClass().getSimpleName(), this.activeProfiles, this.defaultProfiles, - this.propertySources); + return getClass().getSimpleName() + " {activeProfiles=" + this.activeProfiles + + ", defaultProfiles=" + this.defaultProfiles + ", propertySources=" + this.propertySources + "}"; } } diff --git a/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java b/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java index 3de43d0d92..c53109d544 100644 --- a/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java +++ b/spring-core/src/main/java/org/springframework/core/env/AbstractPropertyResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-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. @@ -180,7 +180,7 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe public String getRequiredProperty(String key) throws IllegalStateException { String value = getProperty(key); if (value == null) { - throw new IllegalStateException(String.format("required key [%s] not found", key)); + throw new IllegalStateException("Required key '" + key + "' not found"); } return value; } @@ -189,7 +189,7 @@ public abstract class AbstractPropertyResolver implements ConfigurablePropertyRe public T getRequiredProperty(String key, Class valueType) throws IllegalStateException { T value = getProperty(key, valueType); if (value == null) { - throw new IllegalStateException(String.format("required key [%s] not found", key)); + throw new IllegalStateException("Required key '" + key + "' not found"); } return value; } diff --git a/spring-core/src/main/java/org/springframework/core/env/CompositePropertySource.java b/spring-core/src/main/java/org/springframework/core/env/CompositePropertySource.java index 8591360229..f73e8a4cce 100644 --- a/spring-core/src/main/java/org/springframework/core/env/CompositePropertySource.java +++ b/spring-core/src/main/java/org/springframework/core/env/CompositePropertySource.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2015 the original author or authors. + * Copyright 2002-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. diff --git a/spring-core/src/main/java/org/springframework/core/env/MissingRequiredPropertiesException.java b/spring-core/src/main/java/org/springframework/core/env/MissingRequiredPropertiesException.java index 8c4d338e2c..664e461e20 100644 --- a/spring-core/src/main/java/org/springframework/core/env/MissingRequiredPropertiesException.java +++ b/spring-core/src/main/java/org/springframework/core/env/MissingRequiredPropertiesException.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-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. @@ -33,6 +33,17 @@ public class MissingRequiredPropertiesException extends IllegalStateException { private final Set missingRequiredProperties = new LinkedHashSet(); + + void addMissingRequiredProperty(String key) { + this.missingRequiredProperties.add(key); + } + + @Override + public String getMessage() { + return "The following properties were declared as required but could not be resolved: " + + getMissingRequiredProperties(); + } + /** * Return the set of properties marked as required but not present * upon validation. @@ -40,17 +51,7 @@ public class MissingRequiredPropertiesException extends IllegalStateException { * @see ConfigurablePropertyResolver#validateRequiredProperties() */ public Set getMissingRequiredProperties() { - return missingRequiredProperties; + return this.missingRequiredProperties; } - void addMissingRequiredProperty(String key) { - missingRequiredProperties.add(key); - } - - @Override - public String getMessage() { - return String.format( - "The following properties were declared as required but could " + - "not be resolved: %s", this.getMissingRequiredProperties()); - } } diff --git a/spring-core/src/main/java/org/springframework/core/env/MutablePropertySources.java b/spring-core/src/main/java/org/springframework/core/env/MutablePropertySources.java index 1587833c08..4b40463884 100644 --- a/spring-core/src/main/java/org/springframework/core/env/MutablePropertySources.java +++ b/spring-core/src/main/java/org/springframework/core/env/MutablePropertySources.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-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. @@ -23,8 +23,6 @@ import java.util.concurrent.CopyOnWriteArrayList; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.util.StringUtils; - /** * Default implementation of the {@link PropertySources} interface. * Allows manipulation of contained property sources and provides a constructor @@ -94,8 +92,7 @@ public class MutablePropertySources implements PropertySources { */ public void addFirst(PropertySource propertySource) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Adding [%s] PropertySource with highest search precedence", - propertySource.getName())); + logger.debug("Adding PropertySource '" + propertySource.getName() + "' with highest search precedence"); } removeIfPresent(propertySource); this.propertySourceList.add(0, propertySource); @@ -106,8 +103,7 @@ public class MutablePropertySources implements PropertySources { */ public void addLast(PropertySource propertySource) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Adding [%s] PropertySource with lowest search precedence", - propertySource.getName())); + logger.debug("Adding PropertySource '" + propertySource.getName() + "' with lowest search precedence"); } removeIfPresent(propertySource); this.propertySourceList.add(propertySource); @@ -119,8 +115,8 @@ public class MutablePropertySources implements PropertySources { */ public void addBefore(String relativePropertySourceName, PropertySource propertySource) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Adding [%s] PropertySource with search precedence immediately higher than [%s]", - propertySource.getName(), relativePropertySourceName)); + logger.debug("Adding PropertySource '" + propertySource.getName() + + "' with search precedence immediately higher than '" + relativePropertySourceName + "'"); } assertLegalRelativeAddition(relativePropertySourceName, propertySource); removeIfPresent(propertySource); @@ -134,8 +130,8 @@ public class MutablePropertySources implements PropertySources { */ public void addAfter(String relativePropertySourceName, PropertySource propertySource) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Adding [%s] PropertySource with search precedence immediately lower than [%s]", - propertySource.getName(), relativePropertySourceName)); + logger.debug("Adding PropertySource '" + propertySource.getName() + + "' with search precedence immediately lower than '" + relativePropertySourceName + "'"); } assertLegalRelativeAddition(relativePropertySourceName, propertySource); removeIfPresent(propertySource); @@ -156,7 +152,7 @@ public class MutablePropertySources implements PropertySources { */ public PropertySource remove(String name) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Removing [%s] PropertySource", name)); + logger.debug("Removing PropertySource '" + name + "'"); } int index = this.propertySourceList.indexOf(PropertySource.named(name)); return (index != -1 ? this.propertySourceList.remove(index) : null); @@ -171,8 +167,7 @@ public class MutablePropertySources implements PropertySources { */ public void replace(String name, PropertySource propertySource) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Replacing [%s] PropertySource with [%s]", - name, propertySource.getName())); + logger.debug("Replacing PropertySource '" + name + "' with '" + propertySource.getName() + "'"); } int index = assertPresentAndGetIndex(name); this.propertySourceList.set(index, propertySource); @@ -187,11 +182,7 @@ public class MutablePropertySources implements PropertySources { @Override public String toString() { - String[] names = new String[this.size()]; - for (int i = 0; i < size(); i++) { - names[i] = this.propertySourceList.get(i).getName(); - } - return String.format("[%s]", StringUtils.arrayToCommaDelimitedString(names)); + return this.propertySourceList.toString(); } /** @@ -201,7 +192,7 @@ public class MutablePropertySources implements PropertySources { String newPropertySourceName = propertySource.getName(); if (relativePropertySourceName.equals(newPropertySourceName)) { throw new IllegalArgumentException( - String.format("PropertySource named [%s] cannot be added relative to itself", newPropertySourceName)); + "PropertySource named '" + newPropertySourceName + "' cannot be added relative to itself"); } } @@ -222,14 +213,13 @@ public class MutablePropertySources implements PropertySources { /** * Assert that the named property source is present and return its index. - * @param name the {@linkplain PropertySource#getName() name of the property source} - * to find + * @param name {@linkplain PropertySource#getName() name of the property source} to find * @throws IllegalArgumentException if the named property source is not present */ private int assertPresentAndGetIndex(String name) { int index = this.propertySourceList.indexOf(PropertySource.named(name)); if (index == -1) { - throw new IllegalArgumentException(String.format("PropertySource named [%s] does not exist", name)); + throw new IllegalArgumentException("PropertySource named '" + name + "' does not exist"); } return index; } diff --git a/spring-core/src/main/java/org/springframework/core/env/PropertySource.java b/spring-core/src/main/java/org/springframework/core/env/PropertySource.java index 4d4f4ee86c..5b3c3e15ab 100644 --- a/spring-core/src/main/java/org/springframework/core/env/PropertySource.java +++ b/spring-core/src/main/java/org/springframework/core/env/PropertySource.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-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. @@ -155,11 +155,11 @@ public abstract class PropertySource { @Override public String toString() { if (logger.isDebugEnabled()) { - return String.format("%s@%s [name='%s', properties=%s]", - getClass().getSimpleName(), System.identityHashCode(this), this.name, this.source); + return getClass().getSimpleName() + "@" + System.identityHashCode(this) + + " {name='" + this.name + "', properties=" + this.source + "}"; } else { - return String.format("%s [name='%s']", getClass().getSimpleName(), this.name); + return getClass().getSimpleName() + " {name='" + this.name + "'}"; } } @@ -240,11 +240,6 @@ public abstract class PropertySource { public String getProperty(String name) { throw new UnsupportedOperationException(USAGE_ERROR); } - - @Override - public String toString() { - return String.format("%s [name='%s']", getClass().getSimpleName(), this.name); - } } } diff --git a/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java b/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java index a17596ee98..a28709f3cb 100644 --- a/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java +++ b/spring-core/src/main/java/org/springframework/core/env/PropertySourcesPropertyResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-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. @@ -75,7 +75,8 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver { if (this.propertySources != null) { for (PropertySource propertySource : this.propertySources) { if (logger.isTraceEnabled()) { - logger.trace(String.format("Searching for key '%s' in [%s]", key, propertySource.getName())); + logger.trace("Searching for key '" + key + "' in PropertySource '" + + propertySource.getName() + "'"); } Object value = propertySource.getProperty(key); if (value != null) { @@ -88,7 +89,7 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver { } } if (logger.isDebugEnabled()) { - logger.debug(String.format("Could not find key '%s' in any property source", key)); + logger.debug("Could not find key '" + key + "' in any property source"); } return null; } @@ -148,8 +149,8 @@ public class PropertySourcesPropertyResolver extends AbstractPropertyResolver { */ protected void logKeyFound(String key, PropertySource propertySource, Object value) { if (logger.isDebugEnabled()) { - logger.debug(String.format("Found key '%s' in [%s] with type [%s]", - key, propertySource.getName(), value.getClass().getSimpleName())); + logger.debug("Found key '" + key + "' in PropertySource '" + propertySource.getName() + + "' with value of type " + value.getClass().getSimpleName()); } } diff --git a/spring-core/src/main/java/org/springframework/core/env/SystemEnvironmentPropertySource.java b/spring-core/src/main/java/org/springframework/core/env/SystemEnvironmentPropertySource.java index d72a822e12..5b9fc35040 100644 --- a/spring-core/src/main/java/org/springframework/core/env/SystemEnvironmentPropertySource.java +++ b/spring-core/src/main/java/org/springframework/core/env/SystemEnvironmentPropertySource.java @@ -90,8 +90,8 @@ public class SystemEnvironmentPropertySource extends MapPropertySource { public Object getProperty(String name) { String actualName = resolvePropertyName(name); if (logger.isDebugEnabled() && !name.equals(actualName)) { - logger.debug(String.format("PropertySource [%s] does not contain '%s', but found equivalent '%s'", - getName(), name, actualName)); + logger.debug("PropertySource '" + getName() + "' does not contain property '" + name + + "', but found equivalent '" + actualName + "'"); } return super.getProperty(actualName); } diff --git a/spring-core/src/test/java/org/springframework/core/env/PropertySourceTests.java b/spring-core/src/test/java/org/springframework/core/env/PropertySourceTests.java index fda1f06dba..8eaff31d3a 100644 --- a/spring-core/src/test/java/org/springframework/core/env/PropertySourceTests.java +++ b/spring-core/src/test/java/org/springframework/core/env/PropertySourceTests.java @@ -22,8 +22,6 @@ import java.util.List; import java.util.Map; import java.util.Properties; -import org.apache.log4j.Level; -import org.apache.log4j.Logger; import org.junit.Test; import static org.hamcrest.CoreMatchers.*; @@ -90,34 +88,4 @@ public class PropertySourceTests { propertySources.clear(); } - @Test @SuppressWarnings("serial") - public void toString_verbosityVariesOnLogLevel() { - String name = "props"; - Map map = new HashMap() {{ put("k1", "v1"); }}; - MapPropertySource ps = new MapPropertySource(name, map); - Logger logger = Logger.getLogger(ps.getClass()); - Level original = logger.getLevel(); - - try { - logger.setLevel(Level.DEBUG); - assertThat("PropertySource.toString() should render verbose output for log levels <= DEBUG", - ps.toString(), - equalTo(String.format("%s@%s [name='%s', properties=%s]", - ps.getClass().getSimpleName(), - System.identityHashCode(ps), - name, - map))); - - logger.setLevel(Level.INFO); - assertThat("PropertySource.toString() should render concise output for log levels >= INFO", - ps.toString(), - equalTo(String.format("%s [name='%s']", - ps.getClass().getSimpleName(), - name))); - } - finally { - logger.setLevel(original); - } - } - }