Upgrade to Spring Boot 3 #2072
Upgrade to Spring Boot 3 #2072
Conversation
|
This looks good to me. |
There was a problem hiding this comment.
I am not entirely sure about the usage of Simple-JNDI:
- the author still answered some issue reports last March,
- but the project didn't have any release since 2020 and its dependencies are from 2016 (
commons-iohas even a CVE).
MemoryContext could be easily replaced with NamingContext for Apache Tomcat. The only difference with the latter is that it requires an explicit createSubcontext to create missing subcontexts.
| public Context getContext() { | ||
| return context; | ||
| } |
There was a problem hiding this comment.
I have mixed feeling about exposing Context (which is usually InitialContext, the source of all evil) from the public API.
| final InitialContextFactoryBuilder factoryBuilder = | ||
| factoryBuilderEnv -> factoryEnv -> new MemoryContext(new Hashtable<>()) {}; |
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java
Show resolved
Hide resolved
|
@ppkarwasz, I am not strongly opinionated about |
| 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); | ||
| } |
There was a problem hiding this 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?
If Spring recommends it, it is good enough for me. |
|
We had a discussion with @ppkarwasz and we decided to address his concerns after merging this PR. |
This PR ports
log4j-spring-cloud-config-clientchanges from2.x(#2018) and upgrades to Spring Boot 3.