gus-asf commented on PR #4119: URL: https://github.com/apache/solr/pull/4119#issuecomment-3874647696
> 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. Sounds great if it works. > > 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. 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: ```` <url-pattern type="regex"> ```` > > I think `CoreContainerAwareHttpFilter` should be eliminated in lieu of calling `CoreContainerProvider.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. 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... > > 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. 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. -- 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]
