Don't throw exceptions during DocumentField serialization #95673
Conversation
romseygeek
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
|
Yes, it's not just geo_shape that this can conceivably apply to, so I thought to make it as generic as possible. |
|
@elasticmachine run elasticsearch-ci/part-2 |
| try { | ||
| builder.value(value); | ||
| } catch (RuntimeException e) { | ||
| // if the value cannot be serialized, we catch here and return a placeholder value |
There was a problem hiding this comment.
Since we are catching a generic RuntimeException, we don't know if this was an intentional error (UnsupportedOperationException) or unintentional (NPE or other type of unexpected error, possibly indicating a code bug). Should we inspect the error and log it if it is not UnsupportedOperationException?
There was a problem hiding this comment.
We could make the placeholder value more explicit by including the exception message maybe? I'm not sure how frequent this issue actually is, or how useful an error message would be for something like a script field.
There was a problem hiding this comment.
I'd vote for logging to help find rare bugs and avoid "swallowing" unexpected errors, but I'm still ramping up on standard practices here and don't have much context on this issue yet :-)
But this is just optional feedback.
There was a problem hiding this comment.
Having thought about this, I think I'd like to leave it as it is for now - really the only useful information we could return here would be a stack trace, which isn't really very user friendly. It shouldn't be too difficult to reproduce a problem if it comes up as we will have the document source.
There was a problem hiding this comment.
this reminded me of Strings#toString, but there we catch IOException which is a checked exception. What are the exceptions that can happen in practice? I see that XContentBuilder#value may throw IllegalArgumentException. We could make the builder print out a more user-friendly error?
|
Does this issue need to reference another GH issue in the commit message (was there a previous bug report issue)? |
Good point, I've updated the description to refer to the overall search chunking meta issue. |
|
@elasticmachine update branch |
As a prerequisite to making search responses use chunked REST serialization,
we need to ensure that no exceptions are thrown when toXContent gets called.
DocumentFields can currently throw exceptions if their contained values are not
serializable. This commit changes the serialization code here to replace
unserializable values with a placeholder value rather than failing the whole request.
Relates to #95661