gus-asf commented on PR #2835: URL: https://github.com/apache/solr/pull/2835#issuecomment-2494228698
> Would someone be willing to look at what is going on in SolrDispatchFilter? Around > > https://github.com/apache/solr/blob/620175a5626e69823b5aec8e20734a6352195000/solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java#L336 > it appears that MAYBE there is an opportunity for some refactoring to simplify the flow... Especially since we specifically mention the Hadoop Auth as the reason for the extra complexity.. I do not understand this bit and would love another set of eyes.... I could also see a path to updating the very lengthy comments to say "This has complexity due to hadoop auth partially, and now that it is gone there may be an opportunity for improvement"... I wrote that comment to memorialize several hours of digging I did back when I moved startup to a context listener. One of the things I found perplexing about SolrDispatchFilter when I first tried to understand it for that task was the lack of a call to doFilter(req,resp,filterchain) ... note that our custom version with the boolean retry doesn't count, because it doesn't make the normal call to the method specified by javax.servlet.Filter. Normally filter implementations look like: ```` public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { // Do stuff, that seems important here if (importantStuffSeemsHappy) { chain.doFilter(request, response, chain); } else { // do unhappy error type stuff. } // add try/finally if there is mandatory cleanup. } } ```` So it was very weird not to find a call to doFilter in the doFilter method, nor in our custom version of it. EVENTUALLY I figured out that that call is made either in the dispatch method, OR in our auth filter (I haven't tried to prove it can't get called twice, but with just SolrDispatchFilter in play that is not currently going to cause a problem since chain.doFilter is a no-op for the final filter). One of the long term goals I have is to start pulling stuff that we are doing in this monster filter out int a series of filters, which will make the individual missions easier to understand and put the cleanup code near the instantiation code where, again it would be much easier to understand (and nesting can be easily seen to be correct). My impulse (not yet informed by actual attempts) is to rework our auth plugin to be auth filters. The other thing I'm pointing out in that comment is that the HadoopAuthFilter is what seems to stand in the way of writing an if block such as: ```` if (authPlugin.authenticate(req, resp)) { // <<< note the lack of filterchain arg // do searchy stuff here chain.doFilter(request, response, chain); } else { // do unhappy 401 error type stuff. } // add try/finally for mandatory cleanups. } ```` That is of course the first step to breaking auth out to it's own filter where it becomes ```` public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { if (authPlugin.authenticate(req, resp)) { chain.doFilter(request, response, chain); // <<< dispatch filter is later in the chain. } else { // do unhappy 401 error type stuff. } // add try/finally for mandatory cleanups. } ```` The particular issue with the hadoop auth plugin that complicates the transition is that chain.doFilter() comes before a switch statement and other code... https://github.com/apache/solr/blob/6f94c505fc923c452c58c2a3946ee4408ea06fac/solr/modules/hadoop-auth/src/java/org/apache/solr/security/hadoop/HadoopAuthPlugin.java#L247 At least at the time of that comment it seemed that all the other plugins called chain.doFilter() at the end (or possibly in a shortcut followed by an immediate return statement). Only Hadoop auth seemed to have mandatory actions AFTER doFilter(). If it disappears, we can possibly remove the filterchain argument and make a simpler use of the return value from authenticate(). -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
