Refactor JNDI across environments for common JNDI components that can be in core

jetty-core/jetty-bom/pom.xml

@@ -100,6 +100,11 @@
         <artifactId>jetty-osgi</artifactId>
         <version>12.0.5-SNAPSHOT</version>
       </dependency>
+      <dependency>
+        <groupId>org.eclipse.jetty</groupId>
+        <artifactId>jetty-plus</artifactId>
+        <version>12.0.5-SNAPSHOT</version>
+      </dependency>
       <dependency>
         <groupId>org.eclipse.jetty</groupId>
         <artifactId>jetty-proxy</artifactId>

jetty-core/jetty-jndi/pom.xml

@@ -40,24 +40,6 @@
       <artifactId>jetty-test-helper</artifactId>
       <scope>test</scope>
     </dependency>
-    <!--<dependency>
-      <groupId>org.eclipse.jetty.ee9</groupId>
-      <artifactId>jetty-ee9-security</artifactId>
-      <scope>provided</scope>
-    </dependency>-->
-    <!--<dependency>
-      <groupId>jakarta.activation</groupId>
-      <artifactId>jakarta.activation-api</artifactId>
-      <optional>true</optional>
-    </dependency>
-     -->
-    <!-- TODO: REMOVE -->
-    <!--<dependency>
-      <groupId>jakarta.mail</groupId>
-      <artifactId>jakarta.mail-api</artifactId>
-      <version>${project.version}</version>
-      <optional>true</optional>
-    </dependency>-->
   </dependencies>
 
   <build>
@@ -68,7 +50,7 @@
         <extensions>true</extensions>
         <configuration>
           <instructions>
-            <Import-Package>${osgi.slf4j.import.packages},jakarta.mail.*;resolution:=optional,*</Import-Package>
+            <Import-Package>${osgi.slf4j.import.packages},*</Import-Package>
           </instructions>
         </configuration>
       </plugin>

jetty-core/jetty-jndi/src/main/config/modules/jndi.mod

@@ -4,6 +4,7 @@
 Adds the Jetty JNDI implementation to the classpath.
 
 [depend]
+plus
 server
 
 [lib]

jetty-core/jetty-jndi/src/main/java/module-info.java

@@ -17,6 +17,7 @@
 
     requires transitive java.naming;
     requires transitive org.eclipse.jetty.server;
+    requires transitive org.eclipse.jetty.util;
 
     // Only required if using DataSourceCloser.
     requires static java.sql;

jetty-core/jetty-jndi/src/main/java/org/eclipse/jetty/jndi/ContextFactory.java

@@ -60,12 +60,6 @@ public class ContextFactory implements ObjectFactory
      */
     private static final Map<ClassLoader, Context> __contextMap = new WeakHashMap<>();
 
-    /**
-     * Threadlocal for injecting a context to use
-     * instead of looking up the map.
-     */
-    private static final ThreadLocal<Context> __threadContext = new ThreadLocal<Context>();
-
     /**
      * Threadlocal for setting a classloader which must be used
      * when finding the comp context.
@@ -97,39 +91,11 @@ public Object getObjectInstance(Object obj,
                                     Hashtable env)
         throws Exception
     {
-        //First, see if we have had a context injected into us to use.
-        Context ctx = (Context)__threadContext.get();
-        if (ctx != null)
-        {
-            if (LOG.isDebugEnabled())
-                LOG.debug("Using the Context that is bound on the thread");
-            return ctx;
-        }
-
-        //See if there is a classloader to use for finding the comp context
-        //Don't use its parent hierarchy if set.
-        ClassLoader loader = (ClassLoader)__threadClassLoader.get();
-        if (loader != null)
-        {
-            if (LOG.isDebugEnabled())
-                LOG.debug("Using threadlocal classloader");
-            try (AutoLock l = __lock.lock())
-            {
-                ctx = getContextForClassLoader(loader);
-                if (ctx == null)
-                {
-                    ctx = newNamingContext(obj, loader, env, name, nameCtx);
-                    __contextMap.put(loader, ctx);
-                    if (LOG.isDebugEnabled())
-                        LOG.debug("Made context {} for classloader {}", name.get(0), loader);
-                }
-                return ctx;
-            }
-        }
+        Context ctx = null;
 
         //If the thread context classloader is set, then try its hierarchy to find a matching context
         ClassLoader tccl = Thread.currentThread().getContextClassLoader();
-        loader = tccl;
+        ClassLoader loader = tccl;
         if (loader != null)
         {
             if (LOG.isDebugEnabled())
@@ -139,7 +105,7 @@ public Object getObjectInstance(Object obj,
                 while (ctx == null && loader != null)
                 {
                     ctx = getContextForClassLoader(loader);
-                    if (ctx == null && loader != null)
+                    if (ctx == null)
                         loader = loader.getParent();
                 }
 
@@ -158,7 +124,7 @@ public Object getObjectInstance(Object obj,
         //classloader associated with the current context
         if (ContextHandler.getCurrentContext() != null)
         {
-            if (LOG.isDebugEnabled() && loader != null)
+            if (LOG.isDebugEnabled())
                 LOG.debug("Trying classloader of current org.eclipse.jetty.server.handler.ContextHandler");
             try (AutoLock l = __lock.lock())
             {
@@ -223,37 +189,6 @@ public Context getContextForClassLoader(ClassLoader loader)
         }
     }
 
-    /**
-     * Associate the given Context with the current thread.
-     * disassociate method should be called to reset the context.
-     *
-     * @param ctx the context to associate to the current thread.
-     * @return the previous context associated on the thread (can be null)
-     */
-    public static Context associateContext(final Context ctx)
-    {
-        Context previous = (Context)__threadContext.get();
-        __threadContext.set(ctx);
-        return previous;
-    }
-
-    public static void disassociateContext(final Context ctx)
-    {
-        __threadContext.set(null);
-    }
-
-    public static ClassLoader associateClassLoader(final ClassLoader loader)
-    {
-        ClassLoader prev = (ClassLoader)__threadClassLoader.get();
-        __threadClassLoader.set(loader);
-        return prev;
-    }
-
-    public static void disassociateClassLoader()
-    {
-        __threadClassLoader.set(null);
-    }
-
     public static void dump(Appendable out, String indent) throws IOException
     {
         try (AutoLock l = __lock.lock())

jetty-core/jetty-jndi/src/test/java/org/eclipse/jetty/jndi/java/TestLocalJNDI.java

@@ -14,24 +14,33 @@
 package org.eclipse.jetty.jndi.java;
 
 import java.util.Hashtable;
+import java.util.Map;
+import javax.naming.Binding;
 import javax.naming.Context;
 import javax.naming.InitialContext;
 import javax.naming.Name;
 import javax.naming.NameNotFoundException;
 import javax.naming.NameParser;
+import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
 import javax.naming.RefAddr;
 import javax.naming.Reference;
 import javax.naming.Referenceable;
 import javax.naming.StringRefAddr;
 import javax.naming.spi.ObjectFactory;
 
-import org.eclipse.jetty.jndi.NamingUtil;
+import org.eclipse.jetty.util.jndi.NamingUtil;
 import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.fail;
 
 /**
@@ -94,11 +103,16 @@ public String toString()
         }
     }
 
-    @AfterEach
-    public void tearDown() throws Exception
+    @BeforeEach
+    public void setUp() throws Exception
     {
         InitialContext ic = new InitialContext();
-        ic.destroySubcontext("a");
+        Map<String, Object> nameBindings = NamingUtil.flattenBindings(ic, "");
+
+        for (String name : nameBindings.keySet())
+        {
+            NamingUtil.unbind(ic, name, true);
+        }
     }
 
     @Test
@@ -107,21 +121,27 @@ public void testLocalReferenceable() throws Exception
         Hashtable<String, String> env1 = new Hashtable<String, String>();
         env1.put("flavour", "orange");
         InitialContext ic1 = new InitialContext(env1);
-
-        ic1.bind("valencia", new Fruit("orange"));
-
-        Object o = ic1.lookup("valencia");
-
-        Hashtable<String, String> env2 = new Hashtable<String, String>();
-        InitialContext ic2 = new InitialContext(env2);
         try
         {
-            o = ic2.lookup("valencia");
-            fail("Constructed object from reference without correct environment");
+            ic1.bind("valencia", new Fruit("orange"));
+
+            Object o = ic1.lookup("valencia");
+
+            Hashtable<String, String> env2 = new Hashtable<String, String>();
+            InitialContext ic2 = new InitialContext(env2);
+            try
+            {
+                o = ic2.lookup("valencia");
+                fail("Constructed object from reference without correct environment");
+            }
+            catch (Exception e)
+            {
+                assertEquals("No flavour!", e.getMessage());
+            }
         }
-        catch (Exception e)
+        finally
         {
-            assertEquals("No flavour!", e.getMessage());
+            ic1.unbind("valencia");
         }
     }
 
@@ -179,22 +199,16 @@ public void testLocal() throws Exception
     {
         InitialContext ic = new InitialContext();
         NameParser parser = ic.getNameParser("");
+
         ic.bind("foo", "xxx");
 
         Object o = ic.lookup("foo");
         assertNotNull(o);
         assertEquals("xxx", (String)o);
 
         ic.unbind("foo");
-        try
-        {
-            ic.lookup("foo");
-            fail("Foo exists");
-        }
-        catch (NameNotFoundException e)
-        {
-            //expected
-        }
+        assertThrows(NameNotFoundException.class, () -> ic.lookup("foo"));
+
         Name name = parser.parse("a");
         name.addAll(parser.parse("b/c/d"));
         NamingUtil.bind(ic, name.toString(), "333");
@@ -207,15 +221,7 @@ public void testLocal() throws Exception
         assertEquals("333", (String)o);
         assertEquals("333", ic.lookup(name));
         ic.destroySubcontext("a");
-        try
-        {
-            ic.lookup("a");
-            fail("context a was not destroyed");
-        }
-        catch (NameNotFoundException e)
-        {
-            //expected
-        }
+        assertThrows(NameNotFoundException.class, () -> ic.lookup("a"));
 
         name = parser.parse("");
         name.add("x");
@@ -224,5 +230,49 @@ public void testLocal() throws Exception
         NamingUtil.bind(ic, name.toString(), "555");
         assertEquals("555", ic.lookup(name));
         ic.destroySubcontext("x");
+
+        //add some deep bindings into the tree
+        parser.parse("");
+        final Name thing = parser.parse("top/middle/bottom/thing");
+        NamingUtil.bind(ic, thing.toString(), "leaf");
+        assertEquals("leaf", ic.lookup(thing));
+
+        parser.parse("");
+        final Name thing2 = parser.parse("top2/middle2/bottom2/thing2");
+        NamingUtil.bind(ic, thing2.toString(), "leaf2");
+        assertEquals("leaf2", ic.lookup(thing2));
+
+        //and a shallow binding
+        parser.parse("");
+        Name shallow = parser.parse("shallow");
+        NamingUtil.bind(ic, shallow.toString(), "leaf3");
+
+
+        //and a context with no bindings
+        Name node = parser.parse("");
+        node = parser.parse("node");
+        Context nodeContext = ic.createSubcontext(node);
+        Context subnodeContext = nodeContext.createSubcontext("subnode");
+
+        //check flatten bindings
+        Map<String, Object> map = NamingUtil.flattenBindings(ic, "");
+        assertThat(map.keySet(), containsInAnyOrder(thing.toString(), thing2.toString(), shallow.toString(), node.add("subnode").toString()));
+        assertEquals(subnodeContext, map.get(node.toString()));
+
+        //check destroying
+        nodeContext.destroySubcontext("subnode");
+        assertThrows(NameNotFoundException.class, () -> ic.lookup("node/subnode"));
+
+        ic.destroySubcontext("node");
+        assertThrows(NameNotFoundException.class, () -> ic.lookup("node"));
+
+        //test NamingUtil.unbind with deep unbind
+        NamingUtil.unbind(ic, thing.toString(), true);
+        assertThrows(NameNotFoundException.class, () -> ic.lookup("top"));
+
+        //test NamingUtil.unbind without a deep unbind
+        NamingUtil.unbind(ic, thing2.toString(), false);
+        assertThrows(NameNotFoundException.class, () -> ic.lookup(thing2.toString()));
+        assertDoesNotThrow(() -> ic.lookup("top2/middle2/bottom2"));
     }
 }

jetty-core/jetty-plus/pom.xml

@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+
+  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>org.eclipse.jetty</groupId>
+    <artifactId>jetty-core</artifactId>
+    <version>12.0.5-SNAPSHOT</version>
+  </parent>
+  <artifactId>jetty-plus</artifactId>
+  <name>Core :: Plus</name>
+  <description>JNDI support</description>
+
+  <properties>
+    <bundle-symbolic-name>${project.groupId}.plus</bundle-symbolic-name>
+    <spotbugs.onlyAnalyze>org.eclipse.jetty.plus.*</spotbugs.onlyAnalyze>
+  </properties>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-util</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.slf4j</groupId>
+      <artifactId>slf4j-api</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.eclipse.jetty</groupId>
+      <artifactId>jetty-slf4j-impl</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.eclipse.jetty.toolchain</groupId>
+      <artifactId>jetty-test-helper</artifactId>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+</project>