dsmiley commented on code in PR #4262:
URL: https://github.com/apache/solr/pull/4262#discussion_r3030969387
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -371,7 +363,7 @@ private void init(int port) {
{
// Initialize the servlets
final ServletContextHandler root =
- new ServletContextHandler("/solr", ServletContextHandler.SESSIONS);
+ new ServletContextHandler("/solr",
ServletContextHandler.NO_SESSIONS);
Review Comment:
Solr doesn't use sessions.
##########
solr/core/src/test/org/apache/solr/cloud/ShardRoutingTest.java:
##########
@@ -327,10 +330,15 @@ public void doAtomicUpdate() throws Exception {
}
}
+ @Override
+ public SequencedMap<Class<? extends Filter>, String>
getExtraRequestFilters() {
+ return new LinkedHashMap<>(Map.of(JettySolrRunner.DebugFilter.class,
"/*"));
+ }
+
long getNumRequests() {
- long n = controlJetty.getDebugFilter().getTotalRequests();
+ long n =
controlJetty.getFilter(JettySolrRunner.DebugFilter.class).getTotalRequests();
Review Comment:
IMO DebugFilter should be evicted from JettySolrRunner... perhaps moving to
inside `ServletFixtures` -- WDYT?
##########
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##########
@@ -410,43 +399,29 @@ public void contextInitialized(ServletContextEvent event)
{
// TODO: This needs to be driven by a parsing of web.xml eventually
// though we still want to avoid classpath scanning.
- // required request setup
- requiredFilter =
root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- requiredFilter.setHeldClass(RequiredSolrRequestFilter.class);
-
- // Ratelimit Requests
- rateLimitFilter =
root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- rateLimitFilter.setHeldClass(RateLimitFilter.class);
-
- // Trace Requests
- tracingFilter =
root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- tracingFilter.setHeldClass(TracingFilter.class);
-
- // Authenticate Requests
- authFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- authFilter.setHeldClass(AuthenticationFilter.class);
-
- // Map filters in same path as in web.xml
- root.addFilter(requiredFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
- root.addFilter(rateLimitFilter, "/*",
EnumSet.of(DispatcherType.REQUEST));
- root.addFilter(tracingFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
- root.addFilter(authFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
-
// This is our main workhorse - now a servlet instead of filter
solrServlet = root.getServletHandler().newServletHolder(Source.EMBEDDED);
+ solrServlet.setName("SolrServlet");
solrServlet.setHeldClass(SolrServlet.class);
root.addServlet(solrServlet, "/*");
- // Default servlet as a fall-through
- ServletHolder defaultHolder =
root.getServletHandler().newServletHolder(Source.EMBEDDED);
-
- // considered adding DefaultServlet.class here but perhaps that might
grant our unit tests
- // the power to serve static resources on the build machines? Not sure,
so I'll just give a
- // name to our existing hack. The tests passed without this, but it will
ensure that if anyone
- // ever hits the PathExcludeFilter in the unit test they get a 404 as
before not a 500
- defaultHolder.setHeldClass(Servlet404.class);
- defaultHolder.setName("default");
- root.addServlet(defaultHolder, "/");
+ // Map filters to SolrServlet by name (same order as web.xml)
+ for (var filterClass :
+ List.<Class<? extends Filter>>of(
+ RequiredSolrRequestFilter.class,
+ RateLimitFilter.class,
Review Comment:
It could be interesting to make the non-required filters opt-in. A test
using them would register an extra filter (as they would do for DebugFilter),
and the code here would pluck it out of the extra filter list if it's there.
Any way, I'm not in the mood to pursue.
##########
solr/core/src/test/org/apache/solr/cloud/ShardRoutingTest.java:
##########
@@ -327,10 +330,15 @@ public void doAtomicUpdate() throws Exception {
}
}
+ @Override
+ public SequencedMap<Class<? extends Filter>, String>
getExtraRequestFilters() {
+ return new LinkedHashMap<>(Map.of(JettySolrRunner.DebugFilter.class,
"/*"));
Review Comment:
no-longer auto-registered. If you want it, you register it. Interestingly,
nobody was overriding this before (I think). Moving to SequencedMap makes this
easier to construct and more sensible than a sorted map.
--
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]
