You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "vy (via GitHub)" <gi...@apache.org> on 2023/12/06 13:53:40 UTC
[PR] Upgrade to Spring Boot 3 (logging-log4j2)
vy opened a new pull request, #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072
This PR addresses ports `log4j-spring-cloud-config-client` changes from `2.x` (#2018) and upgrades to Spring Boot 3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#issuecomment-1845190104
> @ppkarwasz, I am not strongly opinionated about `simple-jndi`. It is what Spring Framework recommends as a successor and uses itself too.
If Spring recommends it, it is good enough for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418773659
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -121,6 +121,11 @@ protected boolean isActive() {
return true;
}
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static ConfigurationFactory getInstance() {
+ return LoggerContext.getContext(false).getInstanceFactory().getInstance(KEY);
+ }
+
Review Comment:
`Log4J2LoggingSystem` of Spring Boot 3 uses this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#issuecomment-1845284610
We had a discussion with @ppkarwasz and we decided to address his concerns after merging this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418773872
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java:
##########
@@ -56,6 +56,17 @@ public class UrlConnectionFactory {
private static final String NO_PROTOCOLS = "_none";
public static final String ALLOWED_PROTOCOLS = "log4j2.Configuration.allowedProtocols";
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static <T extends URLConnection> T createConnection(
+ final URL url,
+ final long lastModifiedMillis,
+ final SslConfiguration sslConfiguration,
+ final AuthorizationProvider authorizationProvider)
+ throws IOException {
+ return createConnection(
+ url, lastModifiedMillis, sslConfiguration, authorizationProvider, PropertiesUtil.getProperties());
+ }
+
Review Comment:
`Log4J2LoggingSystem` of Spring Boot uses this.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java:
##########
@@ -56,6 +56,17 @@ public class UrlConnectionFactory {
private static final String NO_PROTOCOLS = "_none";
public static final String ALLOWED_PROTOCOLS = "log4j2.Configuration.allowedProtocols";
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static <T extends URLConnection> T createConnection(
+ final URL url,
+ final long lastModifiedMillis,
+ final SslConfiguration sslConfiguration,
+ final AuthorizationProvider authorizationProvider)
+ throws IOException {
+ return createConnection(
+ url, lastModifiedMillis, sslConfiguration, authorizationProvider, PropertiesUtil.getProperties());
+ }
+
Review Comment:
`Log4J2LoggingSystem` of Spring Boot 3 uses this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418771714
##########
log4j-jndi-test/src/main/java/org/apache/logging/log4j/jndi/test/junit/JndiRule.java:
##########
@@ -16,42 +16,113 @@
*/
package org.apache.logging.log4j.jndi.test.junit;
+import static java.util.Objects.requireNonNull;
+import static org.junit.Assert.assertNotNull;
+
import java.util.Collections;
+import java.util.Hashtable;
import java.util.Map;
+import java.util.Set;
+import java.util.Spliterators;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
import javax.naming.Context;
+import javax.naming.NameClassPair;
+import javax.naming.NamingException;
+import javax.naming.spi.InitialContextFactoryBuilder;
+import javax.naming.spi.NamingManager;
+import org.apache.logging.log4j.jndi.JndiManager;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
-import org.springframework.mock.jndi.SimpleNamingContextBuilder;
+import org.osjava.sj.jndi.MemoryContext;
/**
* JUnit rule to create a mock {@link Context} and bind an object to a name.
*
* @since 2.8
*/
+@SuppressWarnings("BanJNDI")
public class JndiRule implements TestRule {
- private final Map<String, Object> initialBindings;
+ static {
+ final InitialContextFactoryBuilder factoryBuilder =
+ factoryBuilderEnv -> factoryEnv -> new MemoryContext(new Hashtable<>()) {};
Review Comment:
`Context` is closeable, and if the underlying implementation doesn't support re-use after close (as a matter of fact `MemoryContext` indeed doesn't support reuse), you need a new context at every request.
As a side note, I actually tried reusing it, though bumped into several blockers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418815669
##########
log4j-jndi-test/src/main/java/org/apache/logging/log4j/jndi/test/junit/JndiRule.java:
##########
@@ -16,42 +16,113 @@
*/
package org.apache.logging.log4j.jndi.test.junit;
+import static java.util.Objects.requireNonNull;
+import static org.junit.Assert.assertNotNull;
+
import java.util.Collections;
+import java.util.Hashtable;
import java.util.Map;
+import java.util.Set;
+import java.util.Spliterators;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
import javax.naming.Context;
+import javax.naming.NameClassPair;
+import javax.naming.NamingException;
+import javax.naming.spi.InitialContextFactoryBuilder;
+import javax.naming.spi.NamingManager;
+import org.apache.logging.log4j.jndi.JndiManager;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
-import org.springframework.mock.jndi.SimpleNamingContextBuilder;
+import org.osjava.sj.jndi.MemoryContext;
/**
* JUnit rule to create a mock {@link Context} and bind an object to a name.
*
* @since 2.8
*/
+@SuppressWarnings("BanJNDI")
public class JndiRule implements TestRule {
- private final Map<String, Object> initialBindings;
+ static {
+ final InitialContextFactoryBuilder factoryBuilder =
+ factoryBuilderEnv -> factoryEnv -> new MemoryContext(new Hashtable<>()) {};
Review Comment:
Does anything actually call `Context#close()`?
I would prefer to overcome the blockers than expose an "unsafe" `JndiManager.getContext()` method that exposes exactly the dangerous object that `JndiManager` was conceived to wrap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418773659
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -121,6 +121,11 @@ protected boolean isActive() {
return true;
}
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static ConfigurationFactory getInstance() {
+ return LoggerContext.getContext(false).getInstanceFactory().getInstance(KEY);
+ }
+
Review Comment:
`Log4J2LoggingSystem` of Spring Boot uses this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418831243
##########
log4j-jndi-test/src/main/java/org/apache/logging/log4j/jndi/test/junit/JndiRule.java:
##########
@@ -16,42 +16,113 @@
*/
package org.apache.logging.log4j.jndi.test.junit;
+import static java.util.Objects.requireNonNull;
+import static org.junit.Assert.assertNotNull;
+
import java.util.Collections;
+import java.util.Hashtable;
import java.util.Map;
+import java.util.Set;
+import java.util.Spliterators;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
import javax.naming.Context;
+import javax.naming.NameClassPair;
+import javax.naming.NamingException;
+import javax.naming.spi.InitialContextFactoryBuilder;
+import javax.naming.spi.NamingManager;
+import org.apache.logging.log4j.jndi.JndiManager;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
-import org.springframework.mock.jndi.SimpleNamingContextBuilder;
+import org.osjava.sj.jndi.MemoryContext;
/**
* JUnit rule to create a mock {@link Context} and bind an object to a name.
*
* @since 2.8
*/
+@SuppressWarnings("BanJNDI")
public class JndiRule implements TestRule {
- private final Map<String, Object> initialBindings;
+ static {
+ final InitialContextFactoryBuilder factoryBuilder =
+ factoryBuilderEnv -> factoryEnv -> new MemoryContext(new Hashtable<>()) {};
+ try {
+ NamingManager.setInitialContextFactoryBuilder(factoryBuilder);
+ } catch (final NamingException error) {
+ throw new RuntimeException(error);
+ }
+ }
+
+ @Nullable
+ private final String managerName;
+
+ private final Map<String, Object> bindings;
public JndiRule(final String name, final Object value) {
- this.initialBindings = Collections.singletonMap(name, value);
+ this(null, Collections.singletonMap(name, value));
}
- public JndiRule(final Map<String, Object> initialBindings) {
- this.initialBindings = initialBindings;
+ public JndiRule(@Nullable final String managerName, final String name, final Object value) {
+ this(managerName, Collections.singletonMap(name, value));
+ }
+
+ public JndiRule(final Map<String, Object> bindings) {
+ this(null, bindings);
+ }
+
+ public JndiRule(@Nullable final String managerName, final Map<String, Object> bindings) {
+ this.managerName = managerName;
+ this.bindings = requireNonNull(bindings, "bindings");
}
@Override
public Statement apply(final Statement base, final Description description) {
return new Statement() {
@Override
public void evaluate() throws Throwable {
- final SimpleNamingContextBuilder builder = SimpleNamingContextBuilder.emptyActivatedContextBuilder();
- for (final Map.Entry<String, Object> entry : initialBindings.entrySet()) {
- builder.bind(entry.getKey(), entry.getValue());
- }
+ resetJndiManager();
base.evaluate();
}
};
}
+
+ private void resetJndiManager() throws NamingException {
+ if (JndiManager.isJndiEnabled()) {
+ final Context context = getContext();
+ clearBindings(context);
+ addBindings(context);
+ }
+ }
+
+ private Context getContext() {
+ final JndiManager manager =
+ managerName == null ? JndiManager.getDefaultManager() : JndiManager.getDefaultManager(managerName);
+ @Nullable final Context context = manager.getContext();
+ assertNotNull(context);
+ return context;
+ }
+
+ private static void clearBindings(final Context context) throws NamingException {
+ final Set<NameClassPair> existingBindings = StreamSupport.stream(
+ Spliterators.spliteratorUnknownSize(context.list("").asIterator(), 0), false)
+ .collect(Collectors.toSet());
+ existingBindings.forEach(binding -> {
+ try {
+ context.unbind(binding.getName());
+ } catch (NamingException error) {
+ throw new RuntimeException(error);
+ }
+ });
+ }
+
+ private void addBindings(final Context context) throws NamingException {
+ for (final Map.Entry<String, Object> entry : bindings.entrySet()) {
+ final String name = entry.getKey();
+ final Object object = entry.getValue();
+ context.bind(name, object);
+ }
Review Comment:
Comparing this method to #2071 I see a potential problem: a bind for `foo/bar` will fail if there is no context bound to `foo`. Maybe this method should also call `context.createSubcontext` for all subcontexts that do not exist?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy merged PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418010773
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java:
##########
@@ -56,6 +56,17 @@ public class UrlConnectionFactory {
private static final String NO_PROTOCOLS = "_none";
public static final String ALLOWED_PROTOCOLS = "log4j2.Configuration.allowedProtocols";
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static <T extends URLConnection> T createConnection(
+ final URL url,
+ final long lastModifiedMillis,
+ final SslConfiguration sslConfiguration,
+ final AuthorizationProvider authorizationProvider)
+ throws IOException {
+ return createConnection(
+ url, lastModifiedMillis, sslConfiguration, authorizationProvider, PropertiesUtil.getProperties());
+ }
+
Review Comment:
Same as above.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -121,6 +121,11 @@ protected boolean isActive() {
return true;
}
+ @Deprecated(since = "3.0.0", forRemoval = true)
+ public static ConfigurationFactory getInstance() {
+ return LoggerContext.getContext(false).getInstanceFactory().getInstance(KEY);
+ }
+
Review Comment:
Not sure why this is required. I can not find any usage in this PR.
##########
log4j-jndi-test/src/main/java/org/apache/logging/log4j/jndi/test/junit/JndiRule.java:
##########
@@ -16,42 +16,113 @@
*/
package org.apache.logging.log4j.jndi.test.junit;
+import static java.util.Objects.requireNonNull;
+import static org.junit.Assert.assertNotNull;
+
import java.util.Collections;
+import java.util.Hashtable;
import java.util.Map;
+import java.util.Set;
+import java.util.Spliterators;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
import javax.naming.Context;
+import javax.naming.NameClassPair;
+import javax.naming.NamingException;
+import javax.naming.spi.InitialContextFactoryBuilder;
+import javax.naming.spi.NamingManager;
+import org.apache.logging.log4j.jndi.JndiManager;
import org.junit.rules.TestRule;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
-import org.springframework.mock.jndi.SimpleNamingContextBuilder;
+import org.osjava.sj.jndi.MemoryContext;
/**
* JUnit rule to create a mock {@link Context} and bind an object to a name.
*
* @since 2.8
*/
+@SuppressWarnings("BanJNDI")
public class JndiRule implements TestRule {
- private final Map<String, Object> initialBindings;
+ static {
+ final InitialContextFactoryBuilder factoryBuilder =
+ factoryBuilderEnv -> factoryEnv -> new MemoryContext(new Hashtable<>()) {};
Review Comment:
Why not storing this `MemoryContext` somewhere?
The `NamingManager.setInitialContextFactoryBuilder` method is irreversible (can be called only once), so the context used by `JndiManager.getDefaultManager()` will always be the one defined in here.
##########
log4j-jndi/src/main/java/org/apache/logging/log4j/jndi/JndiManager.java:
##########
@@ -194,6 +195,14 @@ protected boolean releaseSub(final long timeout, final TimeUnit timeUnit) {
return JndiCloser.closeSilently(this.context);
}
+ /**
+ * @return the active context
+ */
+ @Nullable
+ public Context getContext() {
+ return context;
+ }
Review Comment:
I have mixed feeling about exposing `Context` (which is usually `InitialContext`, the source of all evil) from the public API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#issuecomment-1845135258
@ppkarwasz, I am not strongly opinionated about `simple-jndi`. It is what Spring Framework recommends as a successor and uses itself too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Upgrade to Spring Boot 3 (logging-log4j2)
Posted by "rgoers (via GitHub)" <gi...@apache.org>.
rgoers commented on PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#issuecomment-1843104630
This looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org