opensearch-project / opensearch-java

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

[FEATURE] toString on API classes #298

Open ginkel opened 1 year ago

ginkel commented 1 year ago

Is your feature request related to a problem?

When interacting with the OpenSearch-Java API during development and testing, I noticed the lack of a toString implementation on the generated API. On test assertion failures this leads to failed assertions looking like this:

java.lang.AssertionError: 
Expected size: 99 but was: 100 in:
[org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@4b65d9f4,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@44536de4,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@5fcfde70,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@4d95a72e,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@28da7d11,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@77b919a3,
    org.opensearch.client.opensearch.core.mget.MultiGetResponseItem@5624657a,
[...]

Which is not exactly helpful to analyse the error... ;-)

What solution would you like?

I'd like to propose adding a minimal toString implementation to the API's POJOs that captures the essence of the respective object. For the MultiGetResponseItem from the sample above that could be index + id of the returned document if there is a result and the failure status if it represents a failure.

wbeckler commented 1 year ago

This sounds like a good idea. Would adding this potentially break anyone's working code? Where do you think is the best place to put this?

ginkel commented 1 year ago

@wbeckler Thanks for the speedy reply!

I don't think there are use-cases that rely on the default Object.toString implementation, so breakage would probably be unlikely.

Regarding the place to put this: Most affected code is generated code (according to the comment before the package declaration), so the easiest way would be to adapt the code generator. If that is not available (I haven't found it yet and I think it was never released by Elastic): A non-representative sample shows that the generated API classes all seem to implement JsonpSerializable, which could serve as a place where common logic could be placed. One major issue with this approach, however, will be how to determine the fields to be included in toString - I think including all fields could be problematic due to the resulting string length. With the code-generation approach this information could be included with the API's meta model.

wbeckler commented 1 year ago

OpenSearch is reimplementing code generation using Smithy specs. The marking of fields for stringificatiion might be something we can add to the spec. This is a more long term solution.

ginkel commented 1 year ago

@wbeckler A long-term solution is fine, thanks for the feedback! Do you have a rough ETA for the Smithy implementation?

wbeckler commented 1 year ago

There is no timeline for this yet, but maybe 12 months.