dsmiley commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2784636577
##########
solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java:
##########
@@ -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"));
Review Comment:
I have other WIP that I may add which would uncomment these additional
assertions.
##########
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##########
@@ -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,
Review Comment:
Interestingly, we don't use FORWARD.
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -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;
Review Comment:
A Servlet doesn't PASSTHROUGH; it serves the request. If we can't, we must
404.
##########
solr/webapp/web/WEB-INF/web.xml:
##########
@@ -29,13 +29,13 @@
<filter-name>PathExclusionsFilter</filter-name>
<filter-class>org.apache.solr.servlet.PathExclusionFilter</filter-class>
<!--
- Exclude patterns is a list of directories that would be short-circuited by
this
- Filter. It includes all Admin UI related static content.
- NOTE: It is NOT a pattern but only matches the start of the HTTP
ServletPath.
+ Requests with URL paths matching these patterns will be redirected to the
"default" servlet.
+ It includes all Admin UI related static content.
+ Syntax: comma delimited regular expressions, and only need to match the
start of the path.
-->
<init-param>
<param-name>excludePatterns</param-name>
-
<param-value>/partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+,/ui/.*</param-value>
+ <param-value>/$,/(partials|libs|css|js|img|templates|ui)/</param-value>
Review Comment:
@malliaridis the `ui` is still here.
If for some reason the new UI is no longer accessible; hopefully we have
tests for such. Ideally BATS/Docker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
