diff --git a/changelog/unreleased/SOLR-18112-SolrDispatchFilterServlet.yml b/changelog/unreleased/SOLR-18112-SolrDispatchFilterServlet.yml new file mode 100644 index 00000000000..f406381287c --- /dev/null +++ b/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 diff --git a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java b/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java index a77541be552..b1178f378a6 100644 --- a/solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java +++ b/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; } /** diff --git a/solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java b/solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java index 1c381f06f8c..e71544f56eb 100644 --- a/solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java +++ b/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); } } diff --git a/solr/core/src/java/org/apache/solr/servlet/RequiredSolrRequestFilter.java b/solr/core/src/java/org/apache/solr/servlet/RequiredSolrRequestFilter.java index 50ec9decdc9..aedce1502ca 100644 --- a/solr/core/src/java/org/apache/solr/servlet/RequiredSolrRequestFilter.java +++ b/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 { diff --git a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java b/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java index 0d0bef9b6ce..2aa387eba17 100644 --- a/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java +++ b/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 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(); diff --git a/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java b/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java index c3db96528dd..f8a70eaffb4 100644 --- a/solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java +++ b/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); diff --git a/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java b/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java index 2530ed7acd4..9718c1ce5f1 100644 --- a/solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java +++ b/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 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!"); } } diff --git a/solr/webapp/web/WEB-INF/web.xml b/solr/webapp/web/WEB-INF/web.xml index e383133ea6d..f818968b3f9 100644 --- a/solr/webapp/web/WEB-INF/web.xml +++ b/solr/webapp/web/WEB-INF/web.xml @@ -29,13 +29,13 @@ PathExclusionsFilter org.apache.solr.servlet.PathExclusionFilter excludePatterns - /partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+,/ui/.* + /$,/(partials|libs|css|js|img|templates|ui)/ @@ -64,15 +64,15 @@ /* - - SolrRequestFilter - org.apache.solr.servlet.SolrDispatchFilter - + + SolrServlet + org.apache.solr.servlet.SolrDispatchFilter + - - SolrRequestFilter + + SolrServlet /* - + LoadAdminUI