This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new 88f7db6bc2 Minor fixes from code review
88f7db6bc2 is described below
commit 88f7db6bc251b489ef5bbd103bb5f85ef0d32edc
Author: remm <[email protected]>
AuthorDate: Thu Jun 25 21:49:02 2026 +0200
Minor fixes from code review
---
java/org/apache/catalina/Pipeline.java | 2 +-
.../apache/catalina/manager/util/SessionUtils.java | 11 +++--
.../apache/catalina/mapper/WrapperMappingInfo.java | 37 --------------
.../org/apache/catalina/mbeans/ClassNameMBean.java | 7 ++-
.../org/apache/catalina/mbeans/ConnectorMBean.java | 8 ++-
.../org/apache/catalina/mbeans/ContainerMBean.java | 11 +++--
.../catalina/mbeans/ContextEnvironmentMBean.java | 6 ++-
.../catalina/mbeans/ContextResourceLinkMBean.java | 8 +--
.../catalina/mbeans/ContextResourceMBean.java | 8 +--
.../mbeans/GlobalResourcesLifecycleListener.java | 57 +++++++++++++++++++++-
.../apache/catalina/mbeans/LocalStrings.properties | 6 +++
java/org/apache/catalina/mbeans/MBeanFactory.java | 6 +++
java/org/apache/catalina/realm/CombinedRealm.java | 12 +++--
.../org/apache/catalina/realm/DataSourceRealm.java | 19 +++++++-
.../apache/catalina/realm/LocalStrings.properties | 4 ++
15 files changed, 139 insertions(+), 63 deletions(-)
diff --git a/java/org/apache/catalina/Pipeline.java
b/java/org/apache/catalina/Pipeline.java
index 2098778d66..d3e66f13f3 100644
--- a/java/org/apache/catalina/Pipeline.java
+++ b/java/org/apache/catalina/Pipeline.java
@@ -23,7 +23,7 @@ import java.util.Set;
* is invoked. It is required that a Valve somewhere in the pipeline (usually
the last one) must process the request and
* create the corresponding response, rather than trying to pass the request
on.
* <p>
- * There is generally a single Pipeline instance associated with each
Container. The container's normal request
+ * There is a single Pipeline instance associated with each Container. The
container's normal request
* processing functionality is generally encapsulated in a container-specific
Valve, which should always be executed at
* the end of a pipeline. To facilitate this, the <code>setBasic()</code>
method is provided to set the Valve instance
* that will always be executed last. Other Valves will be executed in the
order that they were added, before the basic
diff --git a/java/org/apache/catalina/manager/util/SessionUtils.java
b/java/org/apache/catalina/manager/util/SessionUtils.java
index 614c5606c7..00debd18eb 100644
--- a/java/org/apache/catalina/manager/util/SessionUtils.java
+++ b/java/org/apache/catalina/manager/util/SessionUtils.java
@@ -61,22 +61,23 @@ public class SessionUtils {
new String[] { "Login", "User", "userName", "UserName",
"Utilisateur", "SPRING_SECURITY_LAST_USERNAME" };
/**
- * Try to get user locale from the session, if possible. IMPLEMENTATION
NOTE: this method has explicit support for
- * Tapestry 3, Struts 1.x and Spring JSF check the browser meta tag
"accept languages" to choose what language to
- * display.
+ * Try to get user locale from the session, if possible.
*
* @param in_session The session
*
- * @return the locale
+ * @return the locale, or {@code null} if it cannot be determined
*/
public static Locale guessLocaleFromSession(final Session in_session) {
return guessLocaleFromSession(in_session.getSession());
}
/**
- * Try to get user locale from the HttpSession, if possible.
+ * Try to get user locale from the session, if possible. Searches for
Locale objects stored under known attribute
+ * names used by common frameworks (Struts, JSTL, Spring MVC), and falls
back to iterating all session attributes if
+ * exactly one Locale is found.
*
* @param in_session The HTTP session
+ *
* @return the locale, or {@code null} if it cannot be determined
*/
public static Locale guessLocaleFromSession(final HttpSession in_session) {
diff --git a/java/org/apache/catalina/mapper/WrapperMappingInfo.java
b/java/org/apache/catalina/mapper/WrapperMappingInfo.java
index 675da16386..5147143e03 100644
--- a/java/org/apache/catalina/mapper/WrapperMappingInfo.java
+++ b/java/org/apache/catalina/mapper/WrapperMappingInfo.java
@@ -27,41 +27,4 @@ import org.apache.catalina.Wrapper;
* @param resourceOnly Is this a resource only mapping?
*/
public record WrapperMappingInfo(String mapping, Wrapper wrapper, boolean
jspWildCard, boolean resourceOnly) {
-
- /**
- * Returns the URL pattern.
- *
- * @return the URL pattern
- */
- public String getMapping() {
- return mapping;
- }
-
- /**
- * Returns the wrapper for the Servlet.
- *
- * @return the wrapper
- */
- public Wrapper getWrapper() {
- return wrapper;
- }
-
- /**
- * Returns whether this is a mapping for JSP files.
- *
- * @return {@code true} if this is a JSP wildcard mapping
- */
- public boolean isJspWildCard() {
- return jspWildCard;
- }
-
- /**
- * Returns whether this is a resource only mapping.
- *
- * @return {@code true} if this is a resource only mapping
- */
- public boolean isResourceOnly() {
- return resourceOnly;
- }
-
}
diff --git a/java/org/apache/catalina/mbeans/ClassNameMBean.java
b/java/org/apache/catalina/mbeans/ClassNameMBean.java
index 440f0d1880..c1f46c52e0 100644
--- a/java/org/apache/catalina/mbeans/ClassNameMBean.java
+++ b/java/org/apache/catalina/mbeans/ClassNameMBean.java
@@ -38,6 +38,11 @@ public class ClassNameMBean<T> extends BaseCatalinaMBean<T> {
@Override
public String getClassName() {
- return this.resource.getClass().getName();
+ Object resource = this.resource;
+ if (resource != null) {
+ return resource.getClass().getName();
+ } else {
+ return null;
+ }
}
}
diff --git a/java/org/apache/catalina/mbeans/ConnectorMBean.java
b/java/org/apache/catalina/mbeans/ConnectorMBean.java
index 4189694176..1bdd2a8473 100644
--- a/java/org/apache/catalina/mbeans/ConnectorMBean.java
+++ b/java/org/apache/catalina/mbeans/ConnectorMBean.java
@@ -27,7 +27,7 @@ import org.apache.tomcat.util.IntrospectionUtils;
import org.apache.tomcat.util.res.StringManager;
/**
- * A <strong>ModelMBean</strong> implementation for the
<code>org.apache.coyote.tomcat5.CoyoteConnector</code>
+ * A <strong>ModelMBean</strong> implementation for the
<code>org.apache.catalina.connector.Connector</code>
* component.
*/
public class ConnectorMBean extends ClassNameMBean<Connector> {
@@ -72,6 +72,10 @@ public class ConnectorMBean extends
ClassNameMBean<Connector> {
}
Connector connector = doGetManagedResource();
- IntrospectionUtils.setProperty(connector, name, String.valueOf(value));
+ if (value == null) {
+ IntrospectionUtils.setProperty(connector, name, null);
+ } else {
+ IntrospectionUtils.setProperty(connector, name,
String.valueOf(value));
+ }
}
}
diff --git a/java/org/apache/catalina/mbeans/ContainerMBean.java
b/java/org/apache/catalina/mbeans/ContainerMBean.java
index 35f9b6b1d2..0e900a9c4e 100644
--- a/java/org/apache/catalina/mbeans/ContainerMBean.java
+++ b/java/org/apache/catalina/mbeans/ContainerMBean.java
@@ -75,14 +75,19 @@ public class ContainerMBean extends
BaseCatalinaMBean<ContainerBase> {
oldValue = container.getStartChildren();
container.setStartChildren(false);
container.addChild(contained);
- contained.init();
- } catch (LifecycleException e) {
- throw new MBeanException(e);
} finally {
if (container != null) {
container.setStartChildren(oldValue);
}
}
+
+ try {
+ contained.init();
+ } catch (LifecycleException e) {
+ // If init fails, try to cleanup since the MBean may not have been
added
+ container.removeChild(contained);
+ throw new MBeanException(e);
+ }
}
diff --git a/java/org/apache/catalina/mbeans/ContextEnvironmentMBean.java
b/java/org/apache/catalina/mbeans/ContextEnvironmentMBean.java
index 8a7ce9e07a..973029c9cd 100644
--- a/java/org/apache/catalina/mbeans/ContextEnvironmentMBean.java
+++ b/java/org/apache/catalina/mbeans/ContextEnvironmentMBean.java
@@ -47,7 +47,9 @@ public class ContextEnvironmentMBean extends
BaseCatalinaMBean<ContextEnvironmen
// cannot use side effects. It's removed and added back each time
// there is a modification in a resource.
NamingResources nr = ce.getNamingResources();
- nr.removeEnvironment(ce.getName());
- nr.addEnvironment(ce);
+ if (nr != null) {
+ nr.removeEnvironment(ce.getName());
+ nr.addEnvironment(ce);
+ }
}
}
diff --git a/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
b/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
index 9115f15f6a..ee438d127e 100644
--- a/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
+++ b/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
@@ -101,13 +101,15 @@ public class ContextResourceLinkMBean extends
BaseCatalinaMBean<ContextResourceL
// Updating the name actually needs removing and adding back the
component under the new name
case "name" -> log.info(sm.getString("mBean.nameChange"));
case "type" -> crl.setType((String) value);
- default -> crl.setProperty(name, "" + value);
+ default -> crl.setProperty(name, value == null ? null :
value.toString());
}
// cannot use side effects. It's removed and added back each time
// there is a modification in a resource.
NamingResources nr = crl.getNamingResources();
- nr.removeResourceLink(crl.getName());
- nr.addResourceLink(crl);
+ if (nr != null) {
+ nr.removeResourceLink(crl.getName());
+ nr.addResourceLink(crl);
+ }
}
}
diff --git a/java/org/apache/catalina/mbeans/ContextResourceMBean.java
b/java/org/apache/catalina/mbeans/ContextResourceMBean.java
index b70d174dfb..f485dd0e51 100644
--- a/java/org/apache/catalina/mbeans/ContextResourceMBean.java
+++ b/java/org/apache/catalina/mbeans/ContextResourceMBean.java
@@ -103,13 +103,15 @@ public class ContextResourceMBean extends
BaseCatalinaMBean<ContextResource> {
case "name" -> log.info(sm.getString("mBean.nameChange"));
case "scope" -> cr.setScope((String) value);
case "type" -> cr.setType((String) value);
- default -> cr.setProperty(name, "" + value);
+ default -> cr.setProperty(name, value == null ? null :
value.toString());
}
// cannot use side effects. It's removed and added back each time
// there is a modification in a resource.
NamingResources nr = cr.getNamingResources();
- nr.removeResource(cr.getName());
- nr.addResource(cr);
+ if (nr != null) {
+ nr.removeResource(cr.getName());
+ nr.addResource(cr);
+ }
}
}
diff --git
a/java/org/apache/catalina/mbeans/GlobalResourcesLifecycleListener.java
b/java/org/apache/catalina/mbeans/GlobalResourcesLifecycleListener.java
index 16bc43fc30..dd8a94adf0 100644
--- a/java/org/apache/catalina/mbeans/GlobalResourcesLifecycleListener.java
+++ b/java/org/apache/catalina/mbeans/GlobalResourcesLifecycleListener.java
@@ -238,6 +238,61 @@ public class GlobalResourcesLifecycleListener implements
LifecycleListener {
if (log.isTraceEnabled()) {
log.trace("Destroying MBeans for Global JNDI Resources");
}
- // FIXME: Implement removing MBeans
+ // Look up our global naming context
+ Context context;
+ try {
+ context = (Context) (new InitialContext()).lookup("java:/");
+ } catch (NamingException e) {
+ log.error(sm.getString("globalResources.noNamingContext"));
+ return;
+ }
+
+ // Recurse through the defined global JNDI resources context
+ try {
+ destroyMBeans("", context);
+ } catch (NamingException e) {
+ log.error(sm.getString("globalResources.destroyError"), e);
+ }
+ }
+
+
+ /**
+ * Destroy the MBeans for the interesting global JNDI resources in the
specified naming context.
+ *
+ * @param prefix Prefix for complete object name paths
+ * @param context Context to be scanned
+ *
+ * @exception NamingException if a JNDI exception occurs
+ */
+ protected void destroyMBeans(String prefix, Context context) throws
NamingException {
+
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("globalResources.destroy", prefix));
+ }
+
+ try {
+ NamingEnumeration<Binding> bindings = context.listBindings("");
+ while (bindings.hasMore()) {
+ Binding binding = bindings.next();
+ String name = prefix + binding.getName();
+ Object value = context.lookup(binding.getName());
+ if (log.isTraceEnabled()) {
+ log.trace("Checking resource " + name);
+ }
+ if (value instanceof Context) {
+ destroyMBeans(name + "/", (Context) value);
+ } else if (value instanceof UserDatabase) {
+ try {
+ MBeanUtils.destroyMBeanUserDatabase(name);
+ } catch (Exception e) {
+
log.error(sm.getString("globalResources.userDatabaseDestroyError", name), e);
+ }
+ }
+ }
+ } catch (RuntimeException ex) {
+ log.error(sm.getString("globalResources.destroyError.runtime"),
ex);
+ } catch (OperationNotSupportedException ex) {
+ log.error(sm.getString("globalResources.destroyError.operation"),
ex);
+ }
}
}
diff --git a/java/org/apache/catalina/mbeans/LocalStrings.properties
b/java/org/apache/catalina/mbeans/LocalStrings.properties
index 16d8b7fe41..5dc6a1311f 100644
--- a/java/org/apache/catalina/mbeans/LocalStrings.properties
+++ b/java/org/apache/catalina/mbeans/LocalStrings.properties
@@ -24,8 +24,13 @@ globalResources.createError.userDatabase=Cannot create
UserDatabase MBean for re
globalResources.createError.userDatabase.group=Cannot create Group MBean for
group [{0}]
globalResources.createError.userDatabase.role=Cannot create Role MBean for
role [{0}]
globalResources.createError.userDatabase.user=Cannot create User MBean for
user [{0}]
+globalResources.destroy=Destroying MBeans for Global JNDI Resources in Context
[{0}]
+globalResources.destroyError=Exception processing global JNDI Resources
+globalResources.destroyError.operation=Operation not supported error
destroying MBeans
+globalResources.destroyError.runtime=Unexpected error destroying MBeans
globalResources.noNamingContext=No global naming context defined for server
globalResources.userDatabaseCreateError=Exception creating UserDatabase MBeans
for [{0}]
+globalResources.userDatabaseDestroyError=Exception destroying UserDatabase
MBeans for [{0}]
listener.notServer=This listener must only be nested within Server elements,
but is in [{0}].
@@ -41,6 +46,7 @@ mBeanFactory.contextDestroyError=Error during context [{0}]
destroy
mBeanFactory.contextRemove.addServicedFail=Unable to remove context [{0}] as
another component is currently servicing that context
mBeanFactory.managerContext=Manager components may only be added to Contexts.
mBeanFactory.noDeployer=Deployer not found for host [{0}]
+mBeanFactory.noHost=No host found with object name [{0}]
mBeanFactory.noParent=No parent found with object name [{0}]
mBeanFactory.noService=Service with the domain [{0}] was not found
mBeanFactory.notServer=The container is not a Server
diff --git a/java/org/apache/catalina/mbeans/MBeanFactory.java
b/java/org/apache/catalina/mbeans/MBeanFactory.java
index a782328fcc..77d1cc74a7 100644
--- a/java/org/apache/catalina/mbeans/MBeanFactory.java
+++ b/java/org/apache/catalina/mbeans/MBeanFactory.java
@@ -441,6 +441,9 @@ public class MBeanFactory {
Service service = getService(pname);
Engine engine = service.getContainer();
Host host = (Host) engine.findChild(pname.getKeyProperty("host"));
+ if (host == null) {
+ throw new
IllegalArgumentException(sm.getString("mBeanFactory.noHost",
pname.getKeyProperty("host")));
+ }
host.addChild(context);
}
@@ -719,6 +722,9 @@ public class MBeanFactory {
} else {
log.warn(sm.getString("mBeanFactory.noDeployer", hostName));
Host host = (Host) engine.findChild(hostName);
+ if (host == null) {
+ throw new
IllegalArgumentException(sm.getString("mBeanFactory.noHost", hostName));
+ }
Context context = (Context) host.findChild(pathStr);
// Remove this component from its parent component
host.removeChild(context);
diff --git a/java/org/apache/catalina/realm/CombinedRealm.java
b/java/org/apache/catalina/realm/CombinedRealm.java
index d810de8758..30b9fc5204 100644
--- a/java/org/apache/catalina/realm/CombinedRealm.java
+++ b/java/org/apache/catalina/realm/CombinedRealm.java
@@ -75,15 +75,18 @@ public class CombinedRealm extends RealmBase {
/**
* Returns the JMX ObjectNames of the realms that this realm is wrapping.
+ * Entries for realms that do not implement LifecycleMBeanBase will be
null.
*
- * @return the array of realm ObjectNames
+ * @return the array of realm ObjectNames, which may contain null entries
*/
public ObjectName[] getRealms() {
ObjectName[] result = new ObjectName[realms.size()];
+ int i = 0;
for (Realm realm : realms) {
if (realm instanceof LifecycleMBeanBase) {
- result[realms.indexOf(realm)] = ((LifecycleMBeanBase)
realm).getObjectName();
+ result[i] = ((LifecycleMBeanBase) realm).getObjectName();
}
+ i++;
}
return result;
}
@@ -181,14 +184,15 @@ public class CombinedRealm extends RealmBase {
@Override
public void setContainer(Container container) {
+ int i = 0;
for (Realm realm : realms) {
// Set the realmPath for JMX naming
if (realm instanceof RealmBase) {
- ((RealmBase) realm).setRealmPath(getRealmPath() + "/realm" +
realms.indexOf(realm));
+ ((RealmBase) realm).setRealmPath(getRealmPath() + "/realm" +
Integer.toString(i));
}
-
// Set the container for sub-realms. Mainly so logging works.
realm.setContainer(container);
+ i++;
}
super.setContainer(container);
}
diff --git a/java/org/apache/catalina/realm/DataSourceRealm.java
b/java/org/apache/catalina/realm/DataSourceRealm.java
index 3e8b5343c9..845cecd545 100644
--- a/java/org/apache/catalina/realm/DataSourceRealm.java
+++ b/java/org/apache/catalina/realm/DataSourceRealm.java
@@ -522,7 +522,7 @@ public class DataSourceRealm extends RealmBase {
private boolean isRoleStoreDefined() {
- return userRoleTable != null || roleNameCol != null;
+ return (userRoleTable != null && !userRoleTable.isEmpty()) &&
(roleNameCol != null && !roleNameCol.isEmpty());
}
@@ -531,6 +531,23 @@ public class DataSourceRealm extends RealmBase {
@Override
protected void startInternal() throws LifecycleException {
+ if (userTable == null || userTable.isEmpty()) {
+ throw new
LifecycleException(sm.getString("dataSourceRealm.noUserTable"));
+ }
+ if (userNameCol == null || userNameCol.isEmpty()) {
+ throw new
LifecycleException(sm.getString("dataSourceRealm.noUserNameCol"));
+ }
+ if (userCredCol == null || userCredCol.isEmpty()) {
+ throw new
LifecycleException(sm.getString("dataSourceRealm.noUserCredCol"));
+ }
+
+ // Validate role configuration: either both must be set or neither
+ boolean hasRoleTable = userRoleTable != null &&
!userRoleTable.isEmpty();
+ boolean hasRoleNameCol = roleNameCol != null && !roleNameCol.isEmpty();
+ if (hasRoleTable != hasRoleNameCol) {
+ throw new
LifecycleException(sm.getString("dataSourceRealm.roleConfigMismatch"));
+ }
+
// Create the roles PreparedStatement string
StringBuilder temp = new StringBuilder("SELECT ");
temp.append(roleNameCol);
diff --git a/java/org/apache/catalina/realm/LocalStrings.properties
b/java/org/apache/catalina/realm/LocalStrings.properties
index 951847d188..2a68df8c87 100644
--- a/java/org/apache/catalina/realm/LocalStrings.properties
+++ b/java/org/apache/catalina/realm/LocalStrings.properties
@@ -36,6 +36,10 @@ dataSourceRealm.exception=Exception performing authentication
dataSourceRealm.getPassword.exception=Exception retrieving password for [{0}]
dataSourceRealm.getRoles.exception=Exception retrieving roles for [{0}]
dataSourceRealm.noNamingContext=Cannot access the server to retrieve the
naming context
+dataSourceRealm.noUserTable=No user table was specified
+dataSourceRealm.noUserNameCol=No column was specified for user names
+dataSourceRealm.noUserCredCol=No column was specified for user credentials
+dataSourceRealm.roleConfigMismatch=Role configuration is incomplete: both
userRoleTable and roleNameCol must be specified together
jaasCallback.username=Returned username [{0}]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
