Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r3007037321
##
solr/core/src/java/org/apache/solr/servlet/SolrServlet.java:
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.servlet;
+
+import static org.apache.solr.util.tracing.TraceUtils.getSpan;
+
+import jakarta.servlet.ServletConfig;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.UnavailableException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import org.apache.solr.api.V2HttpCall;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeRoles;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Solr's interface to Jetty. Formerly known as {@code SolrDispatchFilter}. */
+public class SolrServlet extends HttpServlet {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private HttpSolrCallFactory solrCallFactory;
+ private CoreContainerProvider containerProvider;
+
+ @Override
+ public void init(ServletConfig config) throws ServletException {
+try {
+ super.init(config);
+ containerProvider =
CoreContainerProvider.serviceForContext(config.getServletContext());
+ boolean isCoordinator =
+
NodeRoles.MODE_ON.equals(getCores().nodeRoles.getRoleMode(NodeRoles.Role.COORDINATOR));
+ solrCallFactory =
+ isCoordinator ? new CoordinatorHttpSolrCall.Factory() : new
HttpSolrCallFactory() {};
+ if (log.isTraceEnabled()) {
+log.trace("init(): {}", this.getClass().getClassLoader());
+ }
+} catch (Throwable t) {
+ // catch this so our servlet still works
+ log.error("Could not start Servlet.", t);
+ if (t instanceof Error) {
+throw (Error) t;
+ }
+} finally {
+ log.trace("init() done");
+}
+ }
+
+ /**
+ * The CoreContainer. It's guaranteed to be constructed before this servlet
is initialized, but
+ * could have been shut down. Never null.
+ */
+ public CoreContainer getCores() throws UnavailableException {
+return containerProvider.getCoreContainer();
+ }
+
+ @Override
+ protected void service(HttpServletRequest request, HttpServletResponse
response)
+ throws IOException, ServletException {
+// Handle root path specially - forward to index.html to trigger
welcome-file mechanism
Review Comment:
> Ideally we would eventually move the UI to the root
+1
--
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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
malliaridis commented on code in PR #4119: URL: https://github.com/apache/solr/pull/4119#discussion_r3006951080 ## solr/webapp/web/WEB-INF/web.xml: ## @@ -29,13 +29,13 @@ PathExclusionsFilter org.apache.solr.servlet.PathExclusionFilter excludePatterns - /partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+,/ui/.* + /$,/(partials|libs|css|js|img|templates|ui)/ Review Comment: I noticed that it is related to trailing slashes (including a trailing `/` and not makes a difference apparently). I will address path resolution issues once I find some time, if nobody notices and adresses it in the meantime. The navigation is also a bit "limited" right now, so I will likely have to make some more changes together with that. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
malliaridis commented on code in PR #4119: URL: https://github.com/apache/solr/pull/4119#discussion_r3006951080 ## solr/webapp/web/WEB-INF/web.xml: ## @@ -29,13 +29,13 @@ PathExclusionsFilter org.apache.solr.servlet.PathExclusionFilter excludePatterns - /partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+,/ui/.* + /$,/(partials|libs|css|js|img|templates|ui)/ Review Comment: I noticed that it is related to trailing slashes (including a trailing `/` and not makes a difference apparently). I can address it once I find some time, if nobody notices and adresses it in the meantime. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
gus-asf commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r3006825311
##
solr/core/src/java/org/apache/solr/servlet/SolrServlet.java:
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.servlet;
+
+import static org.apache.solr.util.tracing.TraceUtils.getSpan;
+
+import jakarta.servlet.ServletConfig;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.UnavailableException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import org.apache.solr.api.V2HttpCall;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeRoles;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Solr's interface to Jetty. Formerly known as {@code SolrDispatchFilter}. */
+public class SolrServlet extends HttpServlet {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private HttpSolrCallFactory solrCallFactory;
+ private CoreContainerProvider containerProvider;
+
+ @Override
+ public void init(ServletConfig config) throws ServletException {
+try {
+ super.init(config);
+ containerProvider =
CoreContainerProvider.serviceForContext(config.getServletContext());
+ boolean isCoordinator =
+
NodeRoles.MODE_ON.equals(getCores().nodeRoles.getRoleMode(NodeRoles.Role.COORDINATOR));
+ solrCallFactory =
+ isCoordinator ? new CoordinatorHttpSolrCall.Factory() : new
HttpSolrCallFactory() {};
+ if (log.isTraceEnabled()) {
+log.trace("init(): {}", this.getClass().getClassLoader());
+ }
+} catch (Throwable t) {
+ // catch this so our servlet still works
+ log.error("Could not start Servlet.", t);
+ if (t instanceof Error) {
+throw (Error) t;
+ }
+} finally {
+ log.trace("init() done");
+}
+ }
+
+ /**
+ * The CoreContainer. It's guaranteed to be constructed before this servlet
is initialized, but
+ * could have been shut down. Never null.
+ */
+ public CoreContainer getCores() throws UnavailableException {
+return containerProvider.getCoreContainer();
+ }
+
+ @Override
+ protected void service(HttpServletRequest request, HttpServletResponse
response)
+ throws IOException, ServletException {
+// Handle root path specially - forward to index.html to trigger
welcome-file mechanism
Review Comment:
Ideally we would eventually move the UI to the root as an entirely separate
entity and have it exist at https://solr.example.local:8983/ instead of
https://solr.example.local:8983/solr/
I haven't investigated jetty specifically, but I know that in tomcat it
always selects the more specific context first so there's no need for this sort
of code if you put the UI in the root. (via a ROOT.war file name there, but
maybe jetty uses a different mechanism)
Sorry I didn't get to this before you pushed, but in any case it's fine for
now, but maybe not a final destination.
--
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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley merged PR #4119: URL: https://github.com/apache/solr/pull/4119 -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119: URL: https://github.com/apache/solr/pull/4119#discussion_r2992130272 ## solr/core/src/java/org/apache/solr/servlet/SolrServlet.java: ## @@ -119,15 +97,13 @@ public void doFilter(HttpServletRequest request, HttpServletResponse response, F For over a decade this class did anything and everything In late 2021 SOLR-15590 moved container startup to CoreContainerProvider In late 2025 SOLR-18040 moved request wrappers to independent ServletFilters -such as PathExclusionFilter see web.xml for a full, up-to-date list - This class is only handling dispatch, please think twice before adding anything else to it. - */ + This class is now basically only passing the request to HttpSolrCall. Review Comment: Reworded this, as I don't think "dispatch" has been an ideal word here ever since HttpSolrCall came into existence to do exactly that. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on PR #4119: URL: https://github.com/apache/solr/pull/4119#issuecomment-4107399975 As you can see I did the class rename here as well. I've got a followup PR ready to post the moment this merges. It eliminates PathExclusionFilter. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2966051754
##
solr/core/src/java/org/apache/solr/servlet/SolrServlet.java:
##
@@ -177,22 +140,23 @@ protected HttpSolrCall getHttpSolrCall(
} catch (UnavailableException e) {
throw new SolrException(ErrorCode.SERVER_ERROR, "Core Container
Unavailable");
}
-return solrCallFactory.createInstance(this, path, cores, request,
response, retry);
+return solrCallFactory.createInstance(path, cores, request, response,
retry);
}
- /** internal API */
+ /**
+ * @lucene.internal
+ */
public interface HttpSolrCallFactory {
Review Comment:
I toyed with moving this to HttpSolrCall. Shrug... it wasn't _obviously_
better so I am leaving it.
--
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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2966027250
##
solr/core/src/java/org/apache/solr/servlet/SolrServlet.java:
##
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.servlet;
+
+import static org.apache.solr.util.tracing.TraceUtils.getSpan;
+
+import jakarta.servlet.ServletConfig;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.UnavailableException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import org.apache.solr.api.V2HttpCall;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeRoles;
+import org.apache.solr.handler.api.V2ApiUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Solr's interface to Jetty. Formerly known as {@code SolrDispatchFilter}. */
+public class SolrServlet extends HttpServlet {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private HttpSolrCallFactory solrCallFactory;
+ private CoreContainerProvider containerProvider;
+
+ @Override
+ public void init(ServletConfig config) throws ServletException {
+try {
+ super.init(config);
+ containerProvider =
CoreContainerProvider.serviceForContext(config.getServletContext());
+ boolean isCoordinator =
+
NodeRoles.MODE_ON.equals(getCores().nodeRoles.getRoleMode(NodeRoles.Role.COORDINATOR));
+ solrCallFactory =
+ isCoordinator ? new CoordinatorHttpSolrCall.Factory() : new
HttpSolrCallFactory() {};
+ if (log.isTraceEnabled()) {
+log.trace("init(): {}", this.getClass().getClassLoader());
+ }
+} catch (Throwable t) {
+ // catch this so our servlet still works
+ log.error("Could not start Servlet.", t);
+ if (t instanceof Error) {
+throw (Error) t;
+ }
+} finally {
+ log.trace("init() done");
+}
+ }
+
+ /**
+ * The CoreContainer. It's guaranteed to be constructed before this servlet
is initialized, but
+ * could have been shut down. Never null.
+ */
+ public CoreContainer getCores() throws UnavailableException {
+return containerProvider.getCoreContainer();
+ }
+
+ @Override
+ protected void service(HttpServletRequest request, HttpServletResponse
response)
+ throws IOException, ServletException {
+// Handle root path specially - forward to index.html to trigger
welcome-file mechanism
Review Comment:
@gus-asf , hope you're cool with this. With pass-through gone, either this
mechanism here is needed or PathExclusionFilter needs to be configured to
handle the root slash case. Since it's possible to remove PathExclusionFilter
and I think that's a good thing, I am leaving this here. The root slash case
seems like a perfectly in-scope exception to handle.
##
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##
@@ -1,355 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.solr.servlet;
-
-import static org.apache.solr.servlet.ServletUtils.closeShield;
-import static org.apache.solr.util.t
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on PR #4119: URL: https://github.com/apache/solr/pull/4119#issuecomment-3875483526 > 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... The ~realistic example you gave was splitting out "admin" and "update" and "query". (a big ) *If* we were to want to do that (which I'm not convinced of), we could do this by having the SolrServlet be a dispatcher that forwards the request to the other named servlets that otherwise don't have url patterns mapped to them. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119: URL: https://github.com/apache/solr/pull/4119#discussion_r2785922430 ## solr/webapp/web/WEB-INF/web.xml: ## @@ -29,13 +29,13 @@ PathExclusionsFilter org.apache.solr.servlet.PathExclusionFilter
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119: URL: https://github.com/apache/solr/pull/4119#discussion_r2785920743 ## solr/webapp/web/WEB-INF/web.xml: ## @@ -29,13 +29,13 @@ PathExclusionsFilter org.apache.solr.servlet.PathExclusionFilter
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
gus-asf commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2785127832
##
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##
@@ -421,15 +421,15 @@ public void contextInitialized(ServletContextEvent event)
{
rateLimitFilter =
root.getServletHandler().newFilterHolder(Source.EMBEDDED);
rateLimitFilter.setHeldClass(RateLimitFilter.class);
- // This is our main workhorse
- dispatchFilter =
root.getServletHandler().newFilterHolder(Source.EMBEDDED);
- dispatchFilter.setHeldClass(SolrDispatchFilter.class);
-
- // Map dispatchFilter in same path as in web.xml
+ // Map filters in same path as in web.xml
root.addFilter(pathExcludeFilter, "/*",
EnumSet.of(DispatcherType.REQUEST));
root.addFilter(requiredFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
root.addFilter(rateLimitFilter, "/*",
EnumSet.of(DispatcherType.REQUEST));
- root.addFilter(dispatchFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
+
+ // This is our main workhorse - now a servlet instead of filter
+ solrServlet = root.getServletHandler().newServletHolder(Source.EMBEDDED);
+ solrServlet.setHeldClass(SolrDispatchFilter.class);
Review Comment:
How about `SolrServlet.class` for the near term, with an eye to (maybe,
maybe not) later splitting out SolrAdminServlet, SolrQueryServlet and
SolrUpdateServlet?
##
solr/webapp/web/WEB-INF/web.xml:
##
@@ -29,13 +29,13 @@
PathExclusionsFilter
org.apache.solr.servlet.PathExclusionFilter
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
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:
>
> 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on PR #4119: URL: https://github.com/apache/solr/pull/4119#issuecomment-3874036237 LoadAdminUiServletTest failed because it used mocking, and didn't mock the internal changes to how the coreContainerProvider stuff changed. I really think we should have a BATS test here and not unit-test this thing. I'd like to manually test it, `@Ignore` it, and then in another PR do the BATS conversion and rip some pieces out of JettySolrRunner that relate to serving the UI. -- 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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
dsmiley commented on code in PR #4119:
URL: https://github.com/apache/solr/pull/4119#discussion_r2784636577
##
solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java:
##
@@ -75,6 +76,29 @@ public void testWrongUtf8InQ() throws Exception {
assertEquals(400, connection.getResponseCode());
}
+ @Test
+ public void test404() throws Exception {
+// once upon a time, Jetty (not Solr) handled 404. Now Solr does.
+try (LogListener log = LogListener.info(HttpSolrCall.class)) {
+ var baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
+ final var collection = "nonExistentColl";
+ final var handler = random().nextBoolean() ? "/select" : "/update"; //
doesn't matter
+ final var queryKeyVal = "q=myQuery";
+ var request =
+ new URI(baseUrl.toString() + "/" + collection + handler + "?" +
queryKeyVal).toURL();
+ var connection = (HttpURLConnection) request.openConnection();
+ assertEquals(404, connection.getResponseCode());
+ assertEquals(1, log.getCount());
+ final var msg = log.pollMessage();
+ // super basic test that merely shows Solr logs contain some basic info
+ // assertTrue(msg, msg.contains("status"));
Review Comment:
I have other WIP that I may add which would uncomment these additional
assertions.
##
solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java:
##
@@ -55,26 +55,26 @@
// 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 CoreContainerAwareHttpFilter {
+public class SolrDispatchFilter extends HttpServlet { // TODO rename to
SolrServlet
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
protected String abortErrorMessage = null;
private HttpSolrCallFactory solrCallFactory;
+ private CoreContainerProvider containerProvider;
public final boolean isV2Enabled = V2ApiUtils.isEnabled();
- /**
- * Enum to define action that needs to be processed. PASSTHROUGH: Pass
through to another filter
- * via webapp. FORWARD: Forward rewritten URI (without path prefix and
core/collection name) to
- * another filter in the chain RETURN: Returns the control, and no further
specific processing is
- * needed. This is generally when an error is set and returned. RETRY:Retry
the request. In cases
- * when a core isn't found to work with, this is set.
- */
+ /** Enum to define action that needs to be processed. */
public enum Action {
-PASSTHROUGH,
+/** Forwards the request via {@link jakarta.servlet.RequestDispatcher}. */
FORWARD,
Review Comment:
Interestingly, we don't use FORWARD.
##
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##
@@ -302,9 +301,11 @@ protected void init() throws Exception {
return; // we are done with a valid handler
}
}
-log.debug("no handler or core retrieved for {}, follow through...", path);
-action = PASSTHROUGH;
Review Comment:
A Servlet doesn't PASSTHROUGH; it serves the request. If we can't, we must
404.
##
solr/webapp/web/WEB-INF/web.xml:
##
@@ -29,13 +29,13 @@
PathExclusionsFilter
org.apache.solr.servlet.PathExclusionFilter
excludePatterns
-
/partials/.+,/libs/.+,/css/.+,/js/.+,/img/.+,/templates/.+,/ui/.*
+ /$,/(partials|libs|css|js|img|templates|ui)/
Review Comment:
@malliaridis the `ui` is still here.
If for some reason the new UI is no longer accessible; hopefully we have
tests for such. Ideally BATS/Docker.
--
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]
Re: [PR] SOLR-18112: SolrDispatchFilter is now a Servlet [solr]
malliaridis commented on PR #4119: URL: https://github.com/apache/solr/pull/4119#issuecomment-3872765277 This PR would break the new UI button reference, since the new UI is accessible at http://127.0.0.18983/solr/ui/ and the updated regex does not cover it I believe. The new UI is still accessible by opening http://127.0.0.18983/solr/ui/index.html, but updating the button link would break any path the new UI plans to use (e.g. for deep links or reloading page at specific sub-pages, like `solr/ui/collections`). Does it make sense to still include the wildcard `ui/*` path so that it continues to work? Or are there any better approaches to include the new UI perhaps that should be considered? -- 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]
