SOLR-18048 Move authentication into a Servlet Filter #4120
Conversation
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class AuthenticationFilter extends CoreContainerAwareHttpFilter { |
There was a problem hiding this comment.
again, pleaese say some basic things in javadoc
There was a problem hiding this comment.
Doh, spent all my energy on the in-code comments and forgot this :) Will do.
|
|
||
|
|
||
|
|
||
| private record AuditSuccessChainWrapper(CoreContainer cc, FilterChain chain) |
There was a problem hiding this comment.
records are cool but this doesn't seem like an appropriate use since conceptually this is a "FilterChain" not some pojo-ish thing. I'm assuming equals & hashCode aren't needed.
There was a problem hiding this comment.
If you want, we can go back to the inner class I had before I converted it (automatically in Intellij) but that's just a lot of wasted lines of code (declaring the fields holding the CoreContainer and FilterChain)? I like it for it's conciseness.
There was a problem hiding this comment.
I'm sure you did it to save some LOC (and not much) but I have a principled stance against it for reasons stated.
There was a problem hiding this comment.
https://stackoverflow.com/questions/71473485/what-is-the-difference-between-a-final-class-and-a-record#comment134638557_71474203 -- that's Brian Goetz (Java Architect).
| // internal version of doFilter that tracks if we are in a retry | ||
|
|
||
| dispatch(chain, closeShield(request), closeShield(response), false); | ||
| dispatch(chain, request, response, false); |
There was a problem hiding this comment.
I think the closeShield calls should be here.
There was a problem hiding this comment.
They were, but I moved them to the required filter (an earlier point in the request) since they seem like noise when trying to understand the way Solr dispatches it's requests. Do you think that it's useful to be able to close the request and response before this point? What is the use case for that?
There was a problem hiding this comment.
I believe closeShield should be in SolrDispatchFilter (soon to be SolrServlet) because this is the juncture at which there the request/response escapes (into the massive Solr codebase) and we have no clear way to guarantee that we don't close them. The filters beforehand are tidy components we can reason about.
There was a problem hiding this comment.
Love the reduction in lines of code!
| rateLimitFilter.setHeldClass(RateLimitFilter.class); | ||
|
|
||
| // Ratelimit Requests | ||
| // Trace Requests |
There was a problem hiding this comment.
IMO these comments don't add anything to the obviousness of the names of these held classes & vars.
There was a problem hiding this comment.
For me at least, sectioning code makes it easier to read quickly. And while some of these comments are a bit obvious, others are more helpful and having a consistent cadence to the sections helps (IMHO). I try to add comments and organize code from the perspective of someone who is arriving at the code to troubleshoot some downstream issue, and what they really need is to find the bit of code that relates to the thing they are actually working on or fixing (e.g. tracing, or rate limiting). In the case of JettySolrRunner, I imagine the most common question relating to these filters will be things like (what order does X get applied in, does X and it's predecessors/successors match the web.xml, or how does the test for X that I'm working on initialize X). So I think it's key to be able to glance-read the filters here.
I'm also not fond of the "extra filters" and "extra servlets" bit just above, and I think it might not ever be used, but that's for another ticket.
There was a problem hiding this comment.
Fine.
Note: Tests use extra servlets for sure; not uncommon.
| * | ||
| * @param eventType the audit event | ||
| */ | ||
| public boolean shouldAudit(AuditEvent.EventType eventType) { |
There was a problem hiding this comment.
I don't see callers to what you added. We should be conservative in adding yet more methods to CoreContainer.
2 participants
https://issues.apache.org/jira/browse/SOLR-18048