Catch task description error (opensearch-project#12834)

* Always return a task description even when it cannot be serialized. Signed-off-by: dblock <[email protected]> * Expect tasks to fail. Signed-off-by: dblock <[email protected]> * Only catch exceptions when getting description. Signed-off-by: dblock <[email protected]> * Added <error: ...> to mark error more clearly. Signed-off-by: dblock <[email protected]> --------- Signed-off-by: dblock <[email protected]>
kkewwei · Mar 22, 2024 · 0c956e8 · 0c956e8
1 parent 0c0bcd9
commit 0c956e8
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 4 deletions.
diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequest.java b/server/src/main/java/org/opensearch/action/search/SearchRequest.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.action.search;
 
+import org.opensearch.OpenSearchException;
 import org.opensearch.Version;
 import org.opensearch.action.ActionRequest;
 import org.opensearch.action.ActionRequestValidationException;
@@ -712,7 +713,13 @@ public final String buildDescription() {
             sb.append("scroll[").append(scroll.keepAlive()).append("], ");
         }
         if (source != null) {
-            sb.append("source[").append(source.toString(FORMAT_PARAMS)).append("]");
+            sb.append("source[");
+            try {
+                sb.append(source.toString(FORMAT_PARAMS));
+            } catch (final OpenSearchException ex) {
+                sb.append("<error: ").append(ex.getMessage()).append(">");
+            }
+            sb.append("]");
         } else {
             sb.append("source[]");
         }

diff --git a/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java
@@ -1842,7 +1842,7 @@ public String toString() {
     public String toString(Params params) {
         try {
             return XContentHelper.toXContent(this, MediaTypeRegistry.JSON, params, true).utf8ToString();
-        } catch (IOException e) {
+        } catch (IOException | UnsupportedOperationException e) {
             throw new OpenSearchException(e);
         }
     }

diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestTests.java
@@ -39,6 +39,8 @@
 import org.opensearch.common.util.ArrayUtils;
 import org.opensearch.core.common.Strings;
 import org.opensearch.core.tasks.TaskId;
+import org.opensearch.geometry.LinearRing;
+import org.opensearch.index.query.GeoShapeQueryBuilder;
 import org.opensearch.index.query.QueryBuilders;
 import org.opensearch.search.AbstractSearchTestCase;
 import org.opensearch.search.Scroll;
@@ -269,6 +271,19 @@ public void testDescriptionIncludesScroll() {
         );
     }
 
+    public void testDescriptionOnSourceError() {
+        LinearRing linearRing = new LinearRing(new double[] { -25, -35, -25 }, new double[] { -25, -35, -25 });
+        GeoShapeQueryBuilder queryBuilder = new GeoShapeQueryBuilder("geo", linearRing);
+        SearchRequest request = new SearchRequest();
+        request.source(new SearchSourceBuilder().query(queryBuilder));
+        assertThat(
+            toDescription(request),
+            equalTo(
+                "indices[], search_type[QUERY_THEN_FETCH], source[<error: java.lang.UnsupportedOperationException: line ring cannot be serialized using GeoJson>]"
+            )
+        );
+    }
+
     private String toDescription(SearchRequest request) {
         return request.createTask(0, "test", SearchAction.NAME, TaskId.EMPTY_TASK_ID, emptyMap()).getDescription();
     }

diff --git a/server/src/test/java/org/opensearch/search/geo/GeoPointShapeQueryTests.java b/server/src/test/java/org/opensearch/search/geo/GeoPointShapeQueryTests.java
@@ -100,6 +100,7 @@ public void testProcessRelationSupport() throws Exception {
                     client().prepareSearch("test")
                         .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, rectangle).relation(shapeRelation))
                         .get();
+                    fail("Expected " + shapeRelation + " query relation not supported for Field [" + defaultGeoFieldName + "]");
                 } catch (SearchPhaseExecutionException e) {
                     assertThat(
                         e.getCause().getMessage(),
@@ -119,6 +120,7 @@ public void testQueryLine() throws Exception {
 
         try {
             client().prepareSearch("test").setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, line)).get();
+            fail("Expected field [" + defaultGeoFieldName + "] does not support LINEARRING queries");
         } catch (SearchPhaseExecutionException e) {
             assertThat(e.getCause().getMessage(), containsString("does not support " + GeoShapeType.LINESTRING + " queries"));
         }
@@ -138,13 +140,12 @@ public void testQueryLinearRing() throws Exception {
             searchRequestBuilder.setQuery(queryBuilder);
             searchRequestBuilder.setIndices("test");
             searchRequestBuilder.get();
+            fail("Expected field [" + defaultGeoFieldName + "] does not support LINEARRING queries");
         } catch (SearchPhaseExecutionException e) {
             assertThat(
                 e.getCause().getMessage(),
                 containsString("Field [" + defaultGeoFieldName + "] does not support LINEARRING queries")
             );
-        } catch (UnsupportedOperationException e) {
-            assertThat(e.getMessage(), containsString("line ring cannot be serialized using GeoJson"));
         }
     }
 
@@ -162,6 +163,7 @@ public void testQueryMultiLine() throws Exception {
 
         try {
             client().prepareSearch("test").setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, multiline)).get();
+            fail("Expected field [" + defaultGeoFieldName + "] does not support " + GeoShapeType.MULTILINESTRING + " queries");
         } catch (Exception e) {
             assertThat(e.getCause().getMessage(), containsString("does not support " + GeoShapeType.MULTILINESTRING + " queries"));
         }
@@ -177,6 +179,7 @@ public void testQueryMultiPoint() throws Exception {
 
         try {
             client().prepareSearch("test").setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, multiPoint)).get();
+            fail("Expected field [" + defaultGeoFieldName + "] does not support " + GeoShapeType.MULTIPOINT + " queries");
         } catch (Exception e) {
             assertThat(e.getCause().getMessage(), containsString("does not support " + GeoShapeType.MULTIPOINT + " queries"));
         }
@@ -192,6 +195,7 @@ public void testQueryPoint() throws Exception {
 
         try {
             client().prepareSearch("test").setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point)).get();
+            fail("Expected field [" + defaultGeoFieldName + "] does not support " + GeoShapeType.POINT + " queries");
         } catch (Exception e) {
             assertThat(e.getCause().getMessage(), containsString("does not support " + GeoShapeType.POINT + " queries"));
         }