gus-asf commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2785127832
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -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);
Review Comment:
How about `SolrServlet.class` for the near term, with an eye to (maybe,
maybe not) later splitting out SolrAdminServlet, SolrQueryServlet and
SolrUpdateServlet?
##########
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.
Review Comment:
The previous version of this comment was none too clear. This is better, but
appears to still be confusing. Path exclusion filter uses this logic:
````
boolean shouldBeExcluded(HttpServletRequest request) {
String requestPath = ServletUtils.getPathAfterContext(request);
if (excludePatterns != null) {
return excludePatterns.stream().map(p ->
p.matcher(requestPath)).anyMatch(Matcher::lookingAt);
}
return false;
}
````
For solr, our servlet context path is `/solr/` but we're search engineers
and not many are j2ee webdevs, so let's be super direct here and say:
`Syntax: comma separated regular expressions for matching the URI path
*after* /solr`
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -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
Review Comment:
👍
##########
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:
Forward and redirect are options too, but yeah, no pass through
--
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]
