SOLR-18041 PathExclusionFilter extracted from SolrDispatchFilter #3981
Conversation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
No change needed in JettySolrRunner?
If tests pass then cool.
| Matcher matcher = p.matcher(requestPath); | ||
| if (matcher.lookingAt()) { | ||
| if (chain != null) { | ||
| chain.doFilter(request, response); |
There was a problem hiding this comment.
wow that was a sneaky side-effect of what looked like a simple boolean method
There was a problem hiding this comment.
Yup took me a moment to figure out why my first pass attempt was somehow still passing control to SolrDispatchFilter (which becomes next in line)
There was a problem hiding this comment.
Ah actually looking there, there is a change that probably should be made, since patterns are being set (but now ignored).
| // 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 HttpFilter implements PathExcluder { |
There was a problem hiding this comment.
looks like you can also remove PathExcluder
There was a problem hiding this comment.
and pull methods out of ServletUtils too! this is fun :-)
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
Just a thought -- perhaps this class should extend Jetty's org.eclipse.jetty.ee10.servlets.IncludeExcludeBasedFilter
On the other hand, maybe there isn't much value in doing so.
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| public class PathExclusionFilter extends HttpFilter { |
There was a problem hiding this comment.
Should have a bit of documentaton on what it does. Like "if the path matches the configured criteria, we process with the 'default' servlet, that which serves files."
Interesting thought, but looking at that class, I think it's too much of a "Swiss army knife", and employing it will likely add complexity and add extra logic to be executed on each request. |
I think this is because tests don't use static resources. We'll hit this eventually. JettySolrRunner's best/worst feature/failing is that it doesn't load a full web application. Which avoids classpath scanning making it much faster, but also makes it unrealistic. |
2 participants
https://issues.apache.org/jira/browse/SOLR-18041