[GitHub] [solr] madrob commented on a change in pull request #648: SOLR-16019 Query parsing exception return HTTP 400 instead of 500

[GitBox <gi...@apache.org>]

madrob commented on a change in pull request #648:
URL: https://github.com/apache/solr/pull/648#discussion_r810232005



##########
File path: solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java
##########
@@ -54,19 +54,28 @@ public static void setupCluster() throws Exception {
   }
 
   @Test
-  public void test() throws Exception {
+  public void testCoreChosen() throws Exception {
     assertCoreChosen(NUM_SHARD, new TestRequest("/collection1/update"));
     assertCoreChosen(NUM_SHARD, new TestRequest("/collection1/update/json"));
     assertCoreChosen(NUM_SHARD * REPLICA_FACTOR, new TestRequest("/collection1/select"));
   }
 
+  // https://issues.apache.org/jira/browse/SOLR-16019
+  @Test
+  public void testRequestParsingFails() throws Exception {

Review comment:
       Maybe SolrRequestParserTest is a better place for this? testUrlParamParsing there already has some invalid strings.

##########
File path: solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java
##########
@@ -233,6 +232,8 @@ protected void init() throws Exception {
       path = path.substring(0, alternate.length());
     }
 
+    queryParams = SolrRequestParsers.parseQueryString(req.getQueryString());

Review comment:
       If this errors, then we go to the finally block in `call()` which calls `sendError()` and will try to parse the same query string again. And we'll get the same error, but suppress the first one? I haven't delved too deeply into what is correct here, but something feels like it is swallowing or diverting the proper code flow.

##########
File path: solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java
##########
@@ -54,19 +54,28 @@ public static void setupCluster() throws Exception {
   }
 
   @Test
-  public void test() throws Exception {
+  public void testCoreChosen() throws Exception {
     assertCoreChosen(NUM_SHARD, new TestRequest("/collection1/update"));
     assertCoreChosen(NUM_SHARD, new TestRequest("/collection1/update/json"));
     assertCoreChosen(NUM_SHARD * REPLICA_FACTOR, new TestRequest("/collection1/select"));
   }
 
+  // https://issues.apache.org/jira/browse/SOLR-16019
+  @Test
+  public void testRequestParsingFails() throws Exception {
+    var baseUrl = cluster.getJettySolrRunner(0).getBaseUrl();
+    var request = new URL(baseUrl.toString() + "/" + COLLECTION + "/select?q=%C0"); // Illegal UTF-8 string
+    var connection = (HttpURLConnection) request.openConnection();
+    assertEquals(400, connection.getResponseCode());
+  }
+
   private void assertCoreChosen(int numCores, TestRequest testRequest) throws UnavailableException {
-    JettySolrRunner jettySolrRunner = cluster.getJettySolrRunner(0);
-    Set<String> coreNames = new HashSet<>();
-    SolrDispatchFilter dispatchFilter = jettySolrRunner.getSolrDispatchFilter();
+    var jettySolrRunner = cluster.getJettySolrRunner(0);
+    var coreNames = new HashSet<String>();
+    var dispatchFilter = jettySolrRunner.getSolrDispatchFilter();
     for (int i = 0; i < NUM_SHARD * REPLICA_FACTOR * 20; i++) {
       if (coreNames.size() == numCores) return;
-      HttpSolrCall httpSolrCall = new HttpSolrCall(dispatchFilter, dispatchFilter.getCores(), testRequest, new TestResponse(), false);

Review comment:
       are these changes necessary?




-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org