This is an automated email from the ASF dual-hosted git repository. rgoers pushed a commit to branch release-2.x in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 90636cf5a00709716735d5ea67ad2fcfe736abae Author: Ralph Goers <[email protected]> AuthorDate: Mon Feb 4 15:48:45 2019 -0700 LOG4J2-913 - Code review changes --- .../logging/log4j/core/config/HttpWatcher.java | 28 ++-- .../core/net/ssl/SslConfigurationFactory.java | 22 +++- .../apache/logging/log4j/core/util/Watcher.java | 4 +- .../logging/log4j/core/util/WatcherFactory.java | 34 +++-- .../log4j/core/util/WrappedFileWatcher.java | 16 ++- .../logging/log4j/core/util/WatchHttpTest.java | 6 +- log4j-core/src/test/resources/logback-test.xml | 31 +++++ .../server/controller/Log4jResourceController.java | 142 ++++++++++----------- 8 files changed, 175 insertions(+), 108 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java index 6072168..8a2f189 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java @@ -1,6 +1,18 @@ /* - * Copyright (c) 2019 Nextiva, Inc. to Present. - * All rights reserved. + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.logging.log4j.core.config; @@ -11,8 +23,6 @@ import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; import java.util.List; -import java.util.Properties; - import javax.net.ssl.HttpsURLConnection; import org.apache.logging.log4j.Logger; @@ -25,7 +35,6 @@ import org.apache.logging.log4j.core.util.AbstractWatcher; import org.apache.logging.log4j.core.util.Source; import org.apache.logging.log4j.core.util.Watcher; import org.apache.logging.log4j.status.StatusLogger; -import org.apache.logging.log4j.util.PropertiesUtil; /** * @@ -72,8 +81,8 @@ public class HttpWatcher extends AbstractWatcher { @Override public void watching(Source source) { if (!source.getURI().getScheme().equals(HTTP) && !source.getURI().getScheme().equals(HTTPS)) { - throw new IllegalArgumentException("HttpWatcher requires a url using the HTTP or HTTPS protocol, not " + - source.getURI().getScheme()); + throw new IllegalArgumentException( + "HttpWatcher requires a url using the HTTP or HTTPS protocol, not " + source.getURI().getScheme()); } try { url = source.getURI().toURL(); @@ -136,15 +145,14 @@ public class HttpWatcher extends AbstractWatcher { return true; } catch (final IOException e) { try (InputStream es = urlConnection.getErrorStream()) { - LOGGER.info("Error accessing configuration at {}: {}", url, - readStream(es)); + LOGGER.info("Error accessing configuration at {}: {}", url, readStream(es)); } catch (final IOException ioe) { LOGGER.error("Error accessing configuration at {}: {}", url, e.getMessage()); } return false; } } - default : { + default: { if (code < 0) { LOGGER.info("Invalid response code returned"); } else { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfigurationFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfigurationFactory.java index e3cc4d3..d0a517d 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfigurationFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfigurationFactory.java @@ -1,6 +1,18 @@ /* - * Copyright (c) 2019 Nextiva, Inc. to Present. - * All rights reserved. + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.logging.log4j.core.net.ssl; @@ -23,9 +35,9 @@ public class SslConfigurationFactory { private static final String trustStoreKeyStoreType = "log4j2.trustStore.keyStoreType"; private static final String trustStoreKeyManagerFactoryAlgorithm = "log4j2.trustStore.keyManagerFactoryAlgorithm"; private static final String keyStoreLocation = "log4j2.keyStore.location"; - private static final String keyStorePassword = "log4j2.keyStorePassword"; - private static final String keyStorePasswordFile = "log4j2.keyStorePasswordFile"; - private static final String keyStorePasswordEnvVar = "log4j2.keyStorePasswordEnvironmentVariable"; + private static final String keyStorePassword = "log4j2.keyStore.password"; + private static final String keyStorePasswordFile = "log4j2.keyStore.passwordFile"; + private static final String keyStorePasswordEnvVar = "log4j2.keyStore.passwordEnvironmentVariable"; private static final String keyStoreType = "log4j2.keyStore.type"; private static final String keyStoreKeyManagerFactoryAlgorithm = "log4j2.keyStore.keyManagerFactoryAlgorithm"; private static final String verifyHostName = "log4j2.ssl.verifyHostName"; diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Watcher.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Watcher.java index c2be48d..426307f 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Watcher.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Watcher.java @@ -29,8 +29,8 @@ import org.apache.logging.log4j.core.config.Reconfigurable; */ public interface Watcher { - public static final String CATEGORY = "Watcher"; - public static final String ELEMENT_TYPE = "watcher"; + String CATEGORY = "Watcher"; + String ELEMENT_TYPE = "watcher"; /** * Returns the list of listeners for this configuration. diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatcherFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatcherFactory.java index ab9e86c..7671353 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatcherFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WatcherFactory.java @@ -1,12 +1,23 @@ /* - * Copyright (c) 2019 Nextiva, Inc. to Present. - * All rights reserved. + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.logging.log4j.core.util; import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; -import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -39,7 +50,7 @@ public class WatcherFactory { public static WatcherFactory getInstance(List<String> packages) { if (factory == null) { - synchronized(pluginManager) { + synchronized (pluginManager) { if (factory == null) { factory = new WatcherFactory(packages); } @@ -58,21 +69,22 @@ public class WatcherFactory { String name = source.getURI().getScheme(); PluginType<?> pluginType = plugins.get(name); if (pluginType != null) { - return instantiate(name, (Class<? extends Watcher>) pluginType.getPluginClass(), - configuration, reconfigurable, configurationListeners, lastModifiedMillis); + return instantiate(name, (Class<? extends Watcher>) pluginType.getPluginClass(), configuration, + reconfigurable, configurationListeners, lastModifiedMillis); } LOGGER.info("No Watcher plugin is available for protocol '{}'", name); return null; } } - @SuppressWarnings("unchecked") - public static <T extends Watcher> T instantiate(String name, final Class<T> clazz, final Configuration configuration, - final Reconfigurable reconfigurable, final List<ConfigurationListener> listeners, long lastModifiedMillis) { + public static <T extends Watcher> T instantiate(String name, final Class<T> clazz, + final Configuration configuration, final Reconfigurable reconfigurable, + final List<ConfigurationListener> listeners, long lastModifiedMillis) { Objects.requireNonNull(clazz, "No class provided"); try { - Constructor constructor = clazz.getConstructor(Configuration.class, Reconfigurable.class, List.class, long.class); - return (T) constructor.newInstance(configuration, reconfigurable, listeners, lastModifiedMillis); + Constructor<T> constructor = clazz + .getConstructor(Configuration.class, Reconfigurable.class, List.class, long.class); + return constructor.newInstance(configuration, reconfigurable, listeners, lastModifiedMillis); } catch (NoSuchMethodException ex) { throw new IllegalArgumentException("No valid constructor for Watcher plugin " + name, ex); } catch (final LinkageError | InstantiationException e) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WrappedFileWatcher.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WrappedFileWatcher.java index ffab8bb..54933b2 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WrappedFileWatcher.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/WrappedFileWatcher.java @@ -1,6 +1,18 @@ /* - * Copyright (c) 2019 Nextiva, Inc. to Present. - * All rights reserved. + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.logging.log4j.core.util; diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java index f9b5abf..1b47cfd 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java @@ -110,8 +110,7 @@ public class WatchHttpTest { try { watchManager.watch(new Source(url.toURI(), previous.getTimeInMillis()), new HttpWatcher(configuration, null, listeners, previous.getTimeInMillis())); - Thread.sleep(1500); - final String str = queue.poll(1, TimeUnit.SECONDS); + final String str = queue.poll(2, TimeUnit.SECONDS); assertNotNull("File change not detected", str); } finally { removeStub(stubMapping); @@ -147,8 +146,7 @@ public class WatchHttpTest { try { watchManager.watch(new Source(url.toURI(), previous.getTimeInMillis()), new HttpWatcher(configuration, null, listeners, previous.getTimeInMillis())); - Thread.sleep(1500); - final String str = queue.poll(1, TimeUnit.SECONDS); + final String str = queue.poll(2, TimeUnit.SECONDS); assertNull("File changed.", str); } finally { removeStub(stubMapping); diff --git a/log4j-core/src/test/resources/logback-test.xml b/log4j-core/src/test/resources/logback-test.xml new file mode 100644 index 0000000..a7885c9 --- /dev/null +++ b/log4j-core/src/test/resources/logback-test.xml @@ -0,0 +1,31 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You 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. + +--> +<configuration> + <appender name="TestLogfile" class="ch.qos.logback.core.FileAppender"> + <file>target/testlogback.log</file> + <append>false</append> + <encoder> + <immediateFlush>false</immediateFlush> + <Pattern>%d %5p [%t] %c{0} %X{transactionId} - %m%n</Pattern> + </encoder> + </appender> + + <root level="debug"> + <appender-ref ref="TestLogfile" /> + </root> +</configuration> diff --git a/log4j-spring-cloud-config/log4j-spring-cloud-config-server/src/main/java/org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java b/log4j-spring-cloud-config/log4j-spring-cloud-config-server/src/main/java/org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java index e6bdd66..dbfc198 100644 --- a/log4j-spring-cloud-config/log4j-spring-cloud-config-server/src/main/java/org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java +++ b/log4j-spring-cloud-config/log4j-spring-cloud-config-server/src/main/java/org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java @@ -18,7 +18,7 @@ package org.apache.logging.log4j.spring.cloud.config.server.controller; import java.io.IOException; import java.io.InputStream; -import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.cloud.config.environment.Environment; @@ -45,7 +45,7 @@ import static org.springframework.cloud.config.server.support.EnvironmentPropert /** * Modified version of Spring Cloud Config's ResourceController to support If-Modified-Since. * Should be dropeed when Spring implements this. - * + * <p> * An HTTP endpoint for serving up templated plain text resources from an underlying * repository. Can be used to supply config files for consumption by a wide variety of * applications and services. A {@link ResourceRepository} is used to locate a @@ -55,56 +55,51 @@ import static org.springframework.cloud.config.server.support.EnvironmentPropert * * @author Dave Syer * @author Daniel Lavoie - * */ @RestController @RequestMapping(method = RequestMethod.GET, path = "${spring.cloud.config.server.prefix:}/resource") public class Log4jResourceController { @Autowired - private ResourceRepository resourceRepository; + private ResourceRepository resourceRepository; @Autowired - private EnvironmentRepository environmentRepository; - - private UrlPathHelper helper = new UrlPathHelper(); - - public Log4jResourceController(ResourceRepository resourceRepository, - EnvironmentRepository environmentRepository) { - this.resourceRepository = resourceRepository; - this.environmentRepository = environmentRepository; - this.helper.setAlwaysUseFullPath(true); - } - - @RequestMapping("/{name}/{profile}/{label}/**") - public String retrieve(@PathVariable String name, @PathVariable String profile, - @PathVariable String label, ServletWebRequest request, - @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws IOException { - String path = getFilePath(request, name, profile, label); - return retrieve(request, name, profile, label, path, resolvePlaceholders); - } - - @RequestMapping(value = "/{name}/{profile}/**", params = "useDefaultLabel") - public String retrieve(@PathVariable String name, @PathVariable String profile, - ServletWebRequest request, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws IOException { - String path = getFilePath(request, name, profile, null); - return retrieve(request, name, profile, null, path, resolvePlaceholders); - } - - private String getFilePath(ServletWebRequest request, String name, String profile, - String label) { - String stem; - if(label != null ) { - stem = String.format("/%s/%s/%s/", name, profile, label); - }else { - stem = String.format("/%s/%s/", name, profile); - } - String path = this.helper.getPathWithinApplication(request.getRequest()); - path = path.substring(path.indexOf(stem) + stem.length()); - return path; - } + private EnvironmentRepository environmentRepository; + + private UrlPathHelper helper = new UrlPathHelper(); + + public Log4jResourceController(ResourceRepository resourceRepository, EnvironmentRepository environmentRepository) { + this.resourceRepository = resourceRepository; + this.environmentRepository = environmentRepository; + this.helper.setAlwaysUseFullPath(true); + } + + @RequestMapping("/{name}/{profile}/{label}/**") + public String retrieve(@PathVariable String name, @PathVariable String profile, @PathVariable String label, + ServletWebRequest request, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) + throws IOException { + String path = getFilePath(request, name, profile, label); + return retrieve(request, name, profile, label, path, resolvePlaceholders); + } + + @RequestMapping(value = "/{name}/{profile}/**", params = "useDefaultLabel") + public String retrieve(@PathVariable String name, @PathVariable String profile, ServletWebRequest request, + @RequestParam(defaultValue = "true") boolean resolvePlaceholders) throws IOException { + String path = getFilePath(request, name, profile, null); + return retrieve(request, name, profile, null, path, resolvePlaceholders); + } + + private String getFilePath(ServletWebRequest request, String name, String profile, String label) { + String stem; + if (label != null) { + stem = String.format("/%s/%s/%s/", name, profile, label); + } else { + stem = String.format("/%s/%s/", name, profile); + } + String path = this.helper.getPathWithinApplication(request.getRequest()); + path = path.substring(path.indexOf(stem) + stem.length()); + return path; + } private synchronized String retrieve(ServletWebRequest request, String name, String profile, String label, String path, boolean resolvePlaceholders) throws IOException { @@ -117,7 +112,7 @@ public class Log4jResourceController { } // ensure InputStream will be closed to prevent file locks on Windows try (InputStream is = resource.getInputStream()) { - String text = StreamUtils.copyToString(is, Charset.forName("UTF-8")); + String text = StreamUtils.copyToString(is, StandardCharsets.UTF_8); if (resolvePlaceholders) { Environment environment = this.environmentRepository.findOne(name, profile, label); text = resolvePlaceholders(prepareEnvironment(environment), text); @@ -129,27 +124,26 @@ public class Log4jResourceController { /* * Used only for unit tests. */ - String retrieve(String name, String profile, String label, String path, - boolean resolvePlaceholders) throws IOException { - return retrieve(null, name, profile, label, path, resolvePlaceholders); + String retrieve(String name, String profile, String label, String path, boolean resolvePlaceholders) + throws IOException { + return retrieve(null, name, profile, label, path, resolvePlaceholders); + } + + @RequestMapping(value = "/{name}/{profile}/{label}/**", produces = MediaType.APPLICATION_OCTET_STREAM_VALUE) + public synchronized byte[] binary(@PathVariable String name, @PathVariable String profile, + @PathVariable String label, ServletWebRequest request) throws IOException { + String path = getFilePath(request, name, profile, label); + return binary(request, name, profile, label, path); } - @RequestMapping(value = "/{name}/{profile}/{label}/**", produces = MediaType.APPLICATION_OCTET_STREAM_VALUE) - public synchronized byte[] binary(@PathVariable String name, - @PathVariable String profile, @PathVariable String label, - ServletWebRequest request) throws IOException { - String path = getFilePath(request, name, profile, label); - return binary(request, name, profile, label, path); - } - - /* - * Used only for unit tests. - */ - byte[] binary(String name, String profile, String label, String path) throws IOException { + /* + * Used only for unit tests. + */ + byte[] binary(String name, String profile, String label, String path) throws IOException { return binary(null, name, profile, label, path); } - private synchronized byte[] binary(ServletWebRequest request, String name, String profile, String label, + private synchronized byte[] binary(ServletWebRequest request, String name, String profile, String label, String path) throws IOException { name = resolveName(name); label = resolveLabel(label); @@ -158,14 +152,14 @@ public class Log4jResourceController { // Content was not modified. Just return. return null; } - // TODO: is this line needed for side effects? - prepareEnvironment(this.environmentRepository.findOne(name, profile, label)); - try (InputStream is = resource.getInputStream()) { - return StreamUtils.copyToByteArray(is); - } - } - - private boolean checkNotModified(ServletWebRequest request, Resource resource) { + // TODO: is this line needed for side effects? + prepareEnvironment(this.environmentRepository.findOne(name, profile, label)); + try (InputStream is = resource.getInputStream()) { + return StreamUtils.copyToByteArray(is); + } + } + + private boolean checkNotModified(ServletWebRequest request, Resource resource) { try { return request != null && request.checkNotModified(resource.lastModified()); } catch (Exception ex) { @@ -174,7 +168,7 @@ public class Log4jResourceController { return false; } - private String resolveName(String name) { + private String resolveName(String name) { if (name != null && name.contains("(_)")) { // "(_)" is uncommon in a git repo name, but "/" cannot be matched // by Spring MVC @@ -192,9 +186,9 @@ public class Log4jResourceController { return label; } - @ExceptionHandler(NoSuchResourceException.class) - @ResponseStatus(HttpStatus.NOT_FOUND) - public void notFound(NoSuchResourceException e) { - } + @ExceptionHandler(NoSuchResourceException.class) + @ResponseStatus(HttpStatus.NOT_FOUND) + public void notFound(NoSuchResourceException e) { + } }
