opensearch-project / opensearch-java

Java Client for OpenSearch
Apache License 2.0
119 stars 182 forks source link

When deserializing to SearchResponse<JsonData> getting error for suggest field #1024

Open AayushiJain012 opened 3 months ago

AayushiJain012 commented 3 months ago

What is the bug?

While trying to deserialize this response I am getting StringOutofBoundException Response - {"took":4,"timed_out":false,"_shards":{"failed":0.0,"successful":1.0,"total":1.0,"skipped":0.0},"hits":{"total":{"relation":"eq","value":0},"hits":[]},"aggregations":{"cardinality#itemCount":{"value":0}},"suggest":{"correction":[{"length":5,"offset":0,"text":"talbe","options":[{"text":"table","freq":22,"score":0.8}]}]}}

Exception - java.lang.StringIndexOutOfBoundsException: begin 0, end -1, length 10 at java.base/java.lang.String.checkBoundsBeginEnd(String.java:4608) at java.base/java.lang.String.substring(String.java:2711) at org.opensearch.client.json.ExternallyTaggedUnion.lambda$arrayDeserializer$0(ExternallyTaggedUnion.java:152) at org.opensearch.client.json.JsonpDeserializer$3.deserialize(JsonpDeserializer.java:138) at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87) at org.opensearch.client.json.ObjectDeserializer$FieldObjectDeserializer.deserialize(ObjectDeserializer.java:81) at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:185) at org.opensearch.client.json.ObjectDeserializer.deserialize(ObjectDeserializer.java:146) at org.opensearch.client.json.JsonpDeserializer.deserialize(JsonpDeserializer.java:87) at org.opensearch.client.json.ObjectBuilderDeserializer.deserialize(ObjectBuilderDeserializer.java:91)

Method to deserialize -

private static final SearchResponse deserializeSearchResponse(String astrJson) throws IOException {
        SearchResponse<JsonData> searchResponseCopy = null;
        JsonFactory jsonFactory = new JsonFactory();
        JsonParser jsonParser = jsonFactory.createParser(astrJson);
        JacksonJsonpParser jsonpParser = new JacksonJsonpParser(jsonParser);
        try {
            JsonpDeserializer<JsonData> jsonpDeserializer = JsonpDeserializer.of(JsonData.class);
               JsonpDeserializer<SearchResponse<JsonData>> searchResponseDeserializer = SearchResponse.createSearchResponseDeserializer(jsonpDeserializer);
            searchResponseCopy = searchResponseDeserializer.deserialize(jsonpParser, new JacksonJsonpMapper());
        } catch (Exception ex) {
            LOGGER.warn("Failed to deserialize {}", astrJson, ex);
            throw ex;
        }
        return searchResponseCopy;
}

Method used to serialize -

private static final String serializeSearchRequest(SearchRequest searchRequest)
            throws IOException {
        String strJson = null;
        try {
            JacksonJsonpMapper jsonpMapper = new JacksonJsonpMapper();

            StringWriter writer = new StringWriter();
            JacksonJsonpGenerator generator = new JacksonJsonpGenerator(new JsonFactory().createGenerator(writer));

            searchRequest.serialize(generator, jsonpMapper);
            generator.flush();
            strJson = writer.toString();
        } catch (Exception ex) {
            LOGGER.warn("Failed to serialize {}", searchRequest, ex);
            throw ex;
        }
        return strJson;
    } 

How can one reproduce the bug?

Create a request with suggester, serialize the response and then deserialize it.

What is the expected behavior?

No Error and get deserialized response

What is your host/environment?

Operating system, version.

Do you have any screenshots?

If applicable, add screenshots to help explain your problem.

Do you have any additional context?

Java client version - 2.10.2

dblock commented 3 months ago

Want to turn this into a (failing) unit test?

AayushiJain012 commented 3 months ago

This is not the only place where serialization is not happening properly. If we have sub aggregation in that case SearchResponse.serialize() is not able to serialize sub aggregation (second level aggregation) and then throwing error when i deserialize it.

dblock commented 3 months ago

@AayushiJain012 You're absolutely right, all these instances need to be fixed :( We have recently merged a beginning of a code generator that aims to resolve this entire class of problems (https://github.com/opensearch-project/opensearch-java/pull/366). There are a few things you can do to help:

  1. Ensure that the specification for these APIs in https://github.com/opensearch-project/opensearch-api-specification is correct, and add tests for them in that repo.
  2. Write tests in this repo for the scenarios that are broken like the ones you're reporting and manually fix any of these bugs.

The combination will ensure that as we switch to the generator we're not introducing regressions.

AayushiJain012 commented 3 months ago

I dont understand how the changes you have mentioned is related to the problem I am facing. I am talking about this serialize method https://github.com/Xtansia/opensearch-java/blob/13982b1ba1c89061c5886eb8232468c8798f3aec/java-client/src/main/java/org/opensearch/client/opensearch/core/search/SearchResult.java#L217C17-L217C26

dblock commented 3 months ago

Generally these problems are trying to deserialize something that's the wrong type in code vs. the response. Start by writing a unit test for this problem, should point exactly to the field that is the issue.

AayushiJain012 commented 3 months ago

The problem is with serialize method not properly serializing suggest and nested-aggregation

Can you help me with the logic how to correctly serialize search response in Java client

dblock commented 3 months ago

Can you help me with the logic how to correctly serialize search response in Java client

Probably. Write a failing unit test for it? @Xtansia can point you to something similar.

Xtansia commented 3 months ago

The failure is happening here: https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java#L150-L152

It's missing the check from this other path: https://github.com/opensearch-project/opensearch-java/blob/main/java-client/src/main/java/org/opensearch/client/json/ExternallyTaggedUnion.java#L126-L131

Which is to say, you need to enable typed_keys on your search request for the deserializer to be happy. The Java client does this automatically when using the client.search() method.

AayushiJain012 commented 3 months ago

I am using client.search() The issue is during serialization. Like aggregation, in suggest field we dont add # and during deserialization it throws an error. Please check serialization logic for suggest field here - https://github.com/opensearch-project/opensearch-java/blob/13982b1ba1c89061c5886eb8232468c8798f3aec/java-client/src/main/java/org/opensearch/client/opensearch/core/search/SearchResult.java#L293

Xtansia commented 3 months ago

I am using client.search() The issue is during serialization. Like aggregation, in suggest field we dont add # and during deserialization it throws an error. Please check serialization logic for suggest field here - https://github.com/opensearch-project/opensearch-java/blob/13982b1ba1c89061c5886eb8232468c8798f3aec/java-client/src/main/java/org/opensearch/client/opensearch/core/search/SearchResult.java#L293

@AayushiJain012 I see, that is a problem types should be able to round-trip if they implement both serialize & deserialize. However, could you please explain a bit your use case for why you need to re-serialize & deserialize the response object rather than using it directly or mapping into your own object structure?

AayushiJain012 commented 3 months ago

@Xtansia We are getting the response as JsonData and then serializing it to store in cache. For second request if data is in cache we simply deserialize it and send back, instead of making call to opensearch.

AayushiJain012 commented 3 months ago

@Xtansia @dblock Are we going to fix this serialization issue? Or can you point me how we serialize the opensearch response which is then deserialize by java-client. I can also try to do something same and write my own code to serialize.

It will be good if we fix these issue in code itself.

dblock commented 3 months ago

Would love a PR that fixes the serialization issue.

AayushiJain012 commented 3 months ago

who will work on that PR and in when we can get the fix?

dblock commented 3 months ago

who will work on that PR and in when we can get the fix?

Nobody is working on it right now. Maybe you?