SOLR-18112: SolrDispatchFilter is now a Servlet

changelog/unreleased/SOLR-18112-SolrDispatchFilterServlet.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: SolrDispatchFilter is now a Servlet, not a Filter
+type: other
+authors:
+  - name: David Smiley
+links:
+  - name: SOLR-18112
+    url: https://issues.apache.org/jira/browse/SOLR-18112

solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java

@@ -21,7 +21,6 @@
 import static org.apache.solr.common.cloud.ZkStateReader.NODE_NAME_PROP;
 import static org.apache.solr.servlet.SolrDispatchFilter.Action.ADMIN;
 import static org.apache.solr.servlet.SolrDispatchFilter.Action.FORWARD;
-import static org.apache.solr.servlet.SolrDispatchFilter.Action.PASSTHROUGH;
 import static org.apache.solr.servlet.SolrDispatchFilter.Action.PROCESS;
 import static org.apache.solr.servlet.SolrDispatchFilter.Action.REMOTEPROXY;
 import static org.apache.solr.servlet.SolrDispatchFilter.Action.RETRY;
@@ -302,9 +301,11 @@ protected void init() throws Exception {
         return; // we are done with a valid handler
       }
     }
-    log.debug("no handler or core retrieved for {}, follow through...", path);
 
-    action = PASSTHROUGH;
+    String msg = "no handler, collection, or core for " + path;
+    sendError(404, msg);
+    log.info(msg); // not "error" since Solr isn't necessarily at fault
+    action = RETURN;
   }
 
   /**

solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java

@@ -16,9 +16,8 @@
  */
 package org.apache.solr.servlet;
 
-import static org.apache.solr.servlet.RequiredSolrRequestFilter.CORE_CONTAINER_REQUEST_ATTRIBUTE;
-
 import com.google.common.net.HttpHeaders;
+import jakarta.servlet.UnavailableException;
 import jakarta.servlet.http.HttpServlet;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
@@ -31,7 +30,6 @@
 import java.util.Arrays;
 import java.util.List;
 import org.apache.commons.io.output.CloseShieldOutputStream;
-import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.SolrCore;
 
 /**
@@ -64,9 +62,10 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro
 
     // This attribute is set by the SolrDispatchFilter
     String admin = request.getRequestURI().substring(request.getContextPath().length());
-    CoreContainer cores = (CoreContainer) request.getAttribute(CORE_CONTAINER_REQUEST_ATTRIBUTE);
     try (InputStream in = getServletContext().getResourceAsStream(admin)) {
-      if (in != null && cores != null) {
+      CoreContainerProvider.serviceForContext(getServletConfig().getServletContext())
+          .getCoreContainer();
+      if (in != null) {
         response.setCharacterEncoding("UTF-8");
         response.setContentType("text/html");
         String connectSrc = generateCspConnectSrc();
@@ -88,6 +87,8 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro
       } else {
         response.sendError(404);
       }
+    } catch (UnavailableException e) { // from CoreContainer being unavailable
+      response.sendError(404);
     }
   }
 

solr/core/src/java/org/apache/solr/servlet/RequiredSolrRequestFilter.java

@@ -70,7 +70,6 @@ protected void doFilter(HttpServletRequest req, HttpServletResponse res, FilterC
       req.setAttribute(SolrRequestParsers.REQUEST_TIMER_SERVLET_ATTRIBUTE, new RTimerTree());
 
       // put the core container in request attribute
-      // This is required for the LoadAdminUiServlet class. Removing it will cause 404
       req.setAttribute(CORE_CONTAINER_REQUEST_ATTRIBUTE, getCores());
       chain.doFilter(req, res);
     } finally {

solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java

@@ -19,10 +19,10 @@
 import static org.apache.solr.servlet.ServletUtils.closeShield;
 import static org.apache.solr.util.tracing.TraceUtils.getSpan;
 
-import jakarta.servlet.FilterChain;
-import jakarta.servlet.FilterConfig;
+import jakarta.servlet.ServletConfig;
 import jakarta.servlet.ServletException;
 import jakarta.servlet.UnavailableException;
+import jakarta.servlet.http.HttpServlet;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
@@ -46,7 +46,7 @@
 import org.slf4j.LoggerFactory;
 
 /**
- * This filter looks at the incoming URL maps them to handlers defined in solrconfig.xml
+ * Solr's interface to Jetty.
  *
  * @since solr 1.2
  */
@@ -55,26 +55,26 @@
 // servlets that are more focused in scope. This should become possible now that we have a
 // ServletContextListener for startup/shutdown of CoreContainer that sets up a service from which
 // things like CoreContainer can be requested. (or better yet injected)
-public class SolrDispatchFilter extends CoreContainerAwareHttpFilter {
+public class SolrDispatchFilter extends HttpServlet { // TODO rename to SolrServlet
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   protected String abortErrorMessage = null;
 
   private HttpSolrCallFactory solrCallFactory;
+  private CoreContainerProvider containerProvider;
 
   public final boolean isV2Enabled = V2ApiUtils.isEnabled();
 
-  /**
-   * Enum to define action that needs to be processed. PASSTHROUGH: Pass through to another filter
-   * via webapp. FORWARD: Forward rewritten URI (without path prefix and core/collection name) to
-   * another filter in the chain RETURN: Returns the control, and no further specific processing is
-   * needed. This is generally when an error is set and returned. RETRY:Retry the request. In cases
-   * when a core isn't found to work with, this is set.
-   */
+  /** Enum to define action that needs to be processed. */
   public enum Action {
-    PASSTHROUGH,
+    /** Forwards the request via {@link jakarta.servlet.RequestDispatcher}. */
     FORWARD,
+    /**
+     * Returns the control, and no further specific processing is needed. This is generally when an
+     * error is set and returned.
+     */
     RETURN,
+    /** Retry the request. Currently used when a core isn't found, we refresh state, and retry. */
     RETRY,
     ADMIN,
     REMOTEPROXY,
@@ -98,9 +98,10 @@ public SolrDispatchFilter() {}
   public static final String SOLR_LOG_LEVEL = "solr.log.level";
 
   @Override
-  public void init(FilterConfig config) throws ServletException {
+  public void init(ServletConfig config) throws ServletException {
     try {
       super.init(config);
+      containerProvider = CoreContainerProvider.serviceForContext(config.getServletContext());
       boolean isCoordinator =
           NodeRoles.MODE_ON.equals(getCores().nodeRoles.getRoleMode(NodeRoles.Role.COORDINATOR));
       solrCallFactory =
@@ -110,8 +111,8 @@ public void init(FilterConfig config) throws ServletException {
       }
 
     } catch (Throwable t) {
-      // catch this so our filter still works
-      log.error("Could not start Dispatch Filter.", t);
+      // catch this so our servlet still works
+      log.error("Could not start Servlet.", t);
       if (t instanceof Error) {
         throw (Error) t;
       }
@@ -120,12 +121,20 @@ public void init(FilterConfig config) throws ServletException {
     }
   }
 
+  /**
+   * The CoreContainer. It's guaranteed to be constructed before this servlet is initialized, but
+   * could have been shut down. Never null.
+   */
+  public CoreContainer getCores() throws UnavailableException {
+    return containerProvider.getCoreContainer();
+  }
+
   @Override
-  public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
+  protected void service(HttpServletRequest request, HttpServletResponse response)
       throws IOException, ServletException {
-    // internal version of doFilter that tracks if we are in a retry
+    // internal version that tracks if we are in a retry
 
-    dispatch(chain, closeShield(request), closeShield(response), false);
+    dispatch(closeShield(request), closeShield(response), false);
   }
 
   /*
@@ -140,8 +149,7 @@ public void doFilter(HttpServletRequest request, HttpServletResponse response, F
   before adding anything else to it.
    */
 
-  private void dispatch(
-      FilterChain chain, HttpServletRequest request, HttpServletResponse response, boolean retry)
+  private void dispatch(HttpServletRequest request, HttpServletResponse response, boolean retry)
       throws IOException, ServletException {
 
     AtomicReference<HttpServletRequest> wrappedRequest = new AtomicReference<>();
@@ -157,7 +165,7 @@ private void dispatch(
             "Authentication Plugin threw SolrAuthenticationException without setting request status >= 400");
         response.sendError(401, "Authentication Plugin rejected credentials.");
       }
-      return; // Nothing more to do, chain.doFilter(req,res) doesn't get called.
+      return;
     }
     if (wrappedRequest.get() != null) {
       request = wrappedRequest.get();
@@ -182,25 +190,16 @@ private void dispatch(
     try {
       Action result = call.call();
       switch (result) {
-        case PASSTHROUGH:
-          span.addEvent("SolrDispatchFilter PASSTHROUGH");
-          chain.doFilter(request, response);
-          break;
-        case RETRY:
+        case RETRY -> {
           span.addEvent("SolrDispatchFilter RETRY");
           // RECURSION
-          dispatch(chain, request, response, true);
-          break;
-        case FORWARD:
+          dispatch(request, response, true);
+        }
+        case FORWARD -> {
           span.addEvent("SolrDispatchFilter FORWARD");
           request.getRequestDispatcher(call.getPath()).forward(request, response);
-          break;
-        case ADMIN:
-        case PROCESS:
-        case REMOTEPROXY:
-        case ADMIN_OR_REMOTEPROXY:
-        case RETURN:
-          break;
+        }
+        default -> {}
       }
     } finally {
       call.destroy();

solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java

@@ -33,6 +33,7 @@
 import org.apache.solr.cloud.SolrCloudTestCase;
 import org.apache.solr.common.util.SuppressForbidden;
 import org.apache.solr.embedded.JettySolrRunner;
+import org.apache.solr.util.LogListener;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.mockito.Mockito;
@@ -75,6 +76,29 @@ public void testWrongUtf8InQ() throws Exception {
     assertEquals(400, connection.getResponseCode());
   }
 
+  @Test
+  public void test404() throws Exception {
+    // once upon a time, Jetty (not Solr) handled 404.  Now Solr does.
+    try (LogListener log = LogListener.info(HttpSolrCall.class)) {
+      var baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
+      final var collection = "nonExistentColl";
+      final var handler = random().nextBoolean() ? "/select" : "/update"; // doesn't matter
+      final var queryKeyVal = "q=myQuery";
+      var request =
+          new URI(baseUrl.toString() + "/" + collection + handler + "?" + queryKeyVal).toURL();
+      var connection = (HttpURLConnection) request.openConnection();
+      assertEquals(404, connection.getResponseCode());
+      assertEquals(1, log.getCount());
+      final var msg = log.pollMessage();
+      // super basic test that merely shows Solr logs contain some basic info
+      //      assertTrue(msg, msg.contains("status"));
+      //      assertTrue(msg, msg.contains("404"));
+      assertTrue(msg, msg.contains(collection));
+      assertTrue(msg, msg.contains(handler));
+      //      assertTrue(msg, msg.contains(queryKeyVal));
+    }
+  }
+
   private void assertCoreChosen(int numCores, HttpServletRequest testRequest)
       throws UnavailableException {
     JettySolrRunner jettySolrRunner = cluster.getJettySolrRunner(0);

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -116,7 +116,7 @@ public class JettySolrRunner {
   volatile FilterHolder pathExcludeFilter;
   volatile FilterHolder requiredFilter;
   volatile FilterHolder rateLimitFilter;
-  volatile FilterHolder dispatchFilter;
+  volatile ServletHolder solrServlet;
 
   private int jettyPort = -1;
 
@@ -128,8 +128,8 @@ public class JettySolrRunner {
 
   private List<FilterHolder> extraFilters;
 
-  private static final String excludePatterns =
-      "/partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+";
+  private static final String DEFAULT_EXCLUDE_PATTERNS =
+      "/$,/(partials|libs|css|js|img|templates)/"; // root and Admin UI stuff
 
   private int proxyPort = -1;
 
@@ -411,7 +411,7 @@ public void contextInitialized(ServletContextEvent event) {
       // added for parity with the live application.
       pathExcludeFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
       pathExcludeFilter.setHeldClass(PathExclusionFilter.class);
-      pathExcludeFilter.setInitParameter("excludePatterns", excludePatterns);
+      pathExcludeFilter.setInitParameter("excludePatterns", DEFAULT_EXCLUDE_PATTERNS);
 
       // required request setup
       requiredFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
@@ -421,15 +421,15 @@ public void contextInitialized(ServletContextEvent event) {
       rateLimitFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
       rateLimitFilter.setHeldClass(RateLimitFilter.class);
 
-      // This is our main workhorse
-      dispatchFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
-      dispatchFilter.setHeldClass(SolrDispatchFilter.class);
-
-      // Map dispatchFilter in same path as in web.xml
+      // Map filters in same path as in web.xml
       root.addFilter(pathExcludeFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
       root.addFilter(requiredFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
       root.addFilter(rateLimitFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
-      root.addFilter(dispatchFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
+
+      // This is our main workhorse - now a servlet instead of filter
+      solrServlet = root.getServletHandler().newServletHolder(Source.EMBEDDED);
+      solrServlet.setHeldClass(SolrDispatchFilter.class);
+      root.addServlet(solrServlet, "/*");
 
       // Default servlet as a fall-through
       ServletHolder defaultHolder = root.getServletHandler().newServletHolder(Source.EMBEDDED);
@@ -482,7 +482,11 @@ protected Handler.Wrapper injectJettyHandlers(Handler.Wrapper chain) {
    * @return the {@link SolrDispatchFilter} for this node
    */
   public SolrDispatchFilter getSolrDispatchFilter() {
-    return (SolrDispatchFilter) dispatchFilter.getFilter();
+    try {
+      return (SolrDispatchFilter) solrServlet.getServlet();
+    } catch (ServletException e) {
+      throw new RuntimeException(e);
+    }
   }
 
   /**
@@ -515,13 +519,13 @@ public String getNodeName() {
   }
 
   public boolean isRunning() {
-    return server.isRunning() && dispatchFilter != null && dispatchFilter.isRunning();
+    return server.isRunning() && solrServlet != null && solrServlet.isRunning();
   }
 
   public boolean isStopped() {
-    return (server.isStopped() && dispatchFilter == null)
+    return (server.isStopped() && solrServlet == null)
         || (server.isStopped()
-            && dispatchFilter.isStopped()
+            && solrServlet.isStopped()
             && ((QueuedThreadPool) server.getThreadPool()).isStopped());
   }
 
@@ -569,7 +573,7 @@ public synchronized void start(boolean reusePort) throws Exception {
           server.start();
         }
       }
-      assert dispatchFilter.isRunning();
+      assert solrServlet.isRunning();
 
       if (config.waitForLoadingCoresToFinishMs != null
           && config.waitForLoadingCoresToFinishMs > 0L) {
@@ -881,17 +885,17 @@ public Properties getNodeProperties() {
   }
 
   private void waitForLoadingCoresToFinish(long timeoutMs) {
-    if (dispatchFilter != null) {
-      SolrDispatchFilter solrFilter = (SolrDispatchFilter) dispatchFilter.getFilter();
+    if (solrServlet != null) {
+      SolrDispatchFilter solrServlet = getSolrDispatchFilter();
       CoreContainer cores;
       try {
-        cores = solrFilter.getCores();
+        cores = solrServlet.getCores();
       } catch (UnavailableException e) {
         throw new IllegalStateException("The CoreContainer is unavailable!");
       }
       cores.waitForLoadingCoresToFinish(timeoutMs);
     } else {
-      throw new IllegalStateException("The dispatchFilter is not set!");
+      throw new IllegalStateException("solrServlet is not set!");
     }
   }