SOLR-18112: SolrDispatchFilter is now a Servlet #4119
Conversation
|
This PR would break the new UI button reference, since the new UI is accessible at http://127.0.0.18983/solr/ui/ and the updated regex does not cover it I believe. The new UI is still accessible by opening http://127.0.0.18983/solr/ui/index.html, but updating the button link would break any path the new UI plans to use (e.g. for deep links or reloading page at specific sub-pages, like Does it make sense to still include the wildcard |
| } | ||
| log.debug("no handler or core retrieved for {}, follow through...", path); | ||
|
|
||
| action = PASSTHROUGH; |
There was a problem hiding this comment.
A Servlet doesn't PASSTHROUGH; it serves the request. If we can't, we must 404.
There was a problem hiding this comment.
Forward and redirect are options too, but yeah, no pass through
| public enum Action { | ||
| PASSTHROUGH, | ||
| /** Forwards the request via {@link jakarta.servlet.RequestDispatcher}. */ | ||
| FORWARD, |
There was a problem hiding this comment.
Interestingly, we don't use FORWARD.
| 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")); |
There was a problem hiding this comment.
I have other WIP that I may add which would uncomment these additional assertions.
| <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> |
There was a problem hiding this 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.
|
LoadAdminUiServletTest failed because it used mocking, and didn't mock the internal changes to how the coreContainerProvider stuff changed. I really think we should have a BATS test here and not unit-test this thing. I'd like to manually test it, |
Sounds great if it works.
One of the things I hate about the servlet spec is the fact that they used these silly patterns instead of regexes. In a world where we only have one servlet it won't matter much, but if we have additional servlets that want to benefit from the filters, we will probably wind up going back to /* and then adding a regex parameter (like the existing path exclusion) to express alternation again... Wanted:
config is not especially convenient to procure in most places that getCores() is used. Setting things to a field in init() simplifies and enhances readability. Sure each filter could just do that in it's own init(), but then we are duplicating code...
I agree this is internal, and I have trouble imagining what changes (other than direct customization to the class) would be impacted. The most significant thing would be if they had customized it to pass through more stuff to their own custom filters, or custom default servlet, but these are all pretty serious customization. I don't see a reason to hold of on the rename. |
| } | ||
| log.debug("no handler or core retrieved for {}, follow through...", path); | ||
|
|
||
| action = PASSTHROUGH; |
There was a problem hiding this comment.
Forward and redirect are options too, but yeah, no pass through
| 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 |
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java
Show resolved
Hide resolved
| 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. |
There was a problem hiding this 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
There was a problem hiding this comment.
Sure. Albeit I probably wasted time bothering with this this tiny optimization and touching the docs since I'm confident I can remove PathExclusionFilter after this merges in a follow-up PR. Granted it will be a good deal more configuration here but simple and no more custom Java code.
There was a problem hiding this comment.
Maybe I'll just back this out, and thus keeping the scope even more clear.
The ~realistic example you gave was splitting out "admin" and "update" and "query". (a big ) If we were to want to do that (which I'm not convinced of), we could do this by having the SolrServlet be a dispatcher that forwards the request to the other named servlets that otherwise don't have url patterns mapped to them. |
3 participants
https://issues.apache.org/jira/browse/SOLR-18112
Also, optimized the configuration of excludePatterns to leverage the regular expression nature it supported but we weren't using. Nonetheless, to me, PathExclusionFilter seems pointless when we could directly map these paths to the "default" servlet.
I'm very tempted to, in this PR, also change the configuration of the new filters to directly reference the servlet instead of using a
/*pattern. This separates them from other servlets that might be registered, e.g. in tests where DebugServlet etc. are used.I think
CoreContainerAwareHttpFiltershould be eliminated in lieu of callingCoreContainerProvider.serviceForContext(config.getServletContext());. Surely that's easy enough to not require a parent class. (classes can only have one parent class so lets not use one for trivial reasons). The Servlet couldn't extend that special Filter, for example. Not sure if that should be in this PR.The first commit/iteration of this PR doesn't rename SolrDispatchFilter to SolrFilter. Maybe leave that for a 2nd PR, same issue. Is this a backwards compatibility concern? Um... probably not. We're changing so much with the filters/servlet SDF that we might as well do it in 10.x. It's "internal" anyway.