Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch task description error #12834

Merged
merged 4 commits into from
Mar 22, 2024
Merged

Catch task description error #12834

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fc4fb0
Always return a task description even when it cannot be serialized.
dblock Mar 21, 2024
190bb32
Expect tasks to fail.
dblock Mar 21, 2024
316479d
Only catch exceptions when getting description.
dblock Mar 21, 2024
71d3f02
Added <error: ...> to mark error more clearly.
dblock Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion server/src/main/java/org/opensearch/action/search/SearchRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -712,7 +713,13 @@
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));

Check warning on line 718 in server/src/main/java/org/opensearch/action/search/SearchRequest.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/search/SearchRequest.java#L718 

Added line #L718 was not covered by tests
} catch (final OpenSearchException ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta / @dblock / @andrross : This is not right. buildDescription() shouldn't swallow exception. Anything unexpected should fail the request as early as possible. Earlier, IOException was wrapped and thrown and now that also gets swallowed. Why an object which is throwing UnsupportedOperationException not handled properly and reached till here at all? In order to fix an user behavior (which may not be an issue, as actual request may not reach till this point as well, unless someone has tested an actual user rest request and confirmed the behavior?). Tomorrow, somebody would wrap NPE also as OpenSearchException and start swallowing here as apparently buildDescription should succeed for some reason.

Copy link
Collaborator

@reta reta Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why an object which is throwing UnsupportedOperationException not handled properly and reached till here at all?

@shwetathareja There are few levels this exception will be raised and it will be handled properly, it is not swallowed. The issue that the description of the request (serialization of the POJO) - the string representation - triggers the problem first, even before the request is processed (and validated). So the suggested fix here is to provide partial description of the request that will eventually fail for exactly the same cause. This is what tests are verifying - the request fails with UnsupportedOperationException

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically in the request path the request is read, then traced (including its description, which throws exception), then validated & al. If we switch the order of things and decide whether the request should be accepted before tracing it, then tracing will fail to show what the request being validated was.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the rest request will come, first the request has to be parsed/ deserialized, shouldn't it fail there, rather fail during serialization flow which comes after that.

I tried indexing and search linearring shape type with trace logging enabled on OpenSearch 2.11 cluster.
trace logging enabled

    "logger": {
      "org": {
        "opensearch": {
          "tasks": "TRACE"
        }
      }
    }

Below are the errors, I got:

During Indexing:

"caused_by": {
        "type": "illegal_argument_exception",
        "reason": "No enum constant org.opensearch.geometry.ShapeType.LINERRING"
      }

During search

    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "unknown geo_shape [linearring]"
    }

The error during search is as expected from the parsing flow:

OpenSearch/server/src/main/java/org/opensearch/common/geo/parsers/GeoJsonParser.java

Line 85 in 4010ff1

final GeoShapeType type = GeoShapeType.forName(subParser.text());

Do you have an example of a rest request which will reach the serialization error ( from .getDescription()) which is fixed in the code above or the validation error you mentioned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not an end-to-end test, The test simply testing a particular flow for GeoShapeQueryBuilder and expecting an exception.

Copy link
Collaborator

@reta reta Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The builder could validate the supported shapes in AbstractGeometryQueryBuilder when creating the POJO itself.

I already replied to that here #12834 (comment)

I think the ideal option is @andrross 's idea to get rid of LinearRing if possible - this is supporting abstraction

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock : Are you reverting this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I am still convinced that the implementation merged here is better than what we had. Today tasks can fail to getDescription() and the better place to guard it is in the code we added it in. If we get rid of LinearRing, then we don't need to protect from failed serialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we remove the Linearring, the only exception, you are taking care in getDescription() is IOException which we shouldn't catch and swallow even for getDescription() to work. I don't think there is need for this exception handling considering it has always been like this, not impacting any other tests/ flow yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree re:once we remove the Linearring. I opened #12910, I'll try to get to it.

sb.append(ex.getMessage());
andrross marked this conversation as resolved.
Show resolved Hide resolved
}

Check warning on line 721 in server/src/main/java/org/opensearch/action/search/SearchRequest.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/action/search/SearchRequest.java#L721 

Added line #L721 was not covered by tests
sb.append("]");
} else {
sb.append("source[]");
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/search/builder/SearchSourceBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
15 changes: 15 additions & 0 deletions server/src/test/java/org/opensearch/action/search/SearchRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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[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();
}
Expand Down
8 changes: 6 additions & 2 deletions server/src/test/java/org/opensearch/search/geo/GeoPointShapeQueryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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"));
}
Expand All @@ -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"));
}
}

Expand All @@ -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"));
}
Expand All @@ -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"));
}
Expand All @@ -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"));
}
Expand Down
Loading