Fix NullPointerException in LongComparator caused by null search_after values #132434
Conversation
elasticsearchmachine
added
v9.2.0
needs:triage
john-wagster
added
the
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
removed
the
needs:triage
|
@elasticsearchmachine test this please |
| fieldValues[i] = convertValueFromSortField(values[i], sortField, format); | ||
| } else { | ||
| fieldValues[i] = null; | ||
| throw new IllegalArgumentException("Values cannot contain null at position " + i + "."); |
There was a problem hiding this comment.
do you know what scenario this covers for in practice?
There was a problem hiding this comment.
In the constructor SearchAfterBuilder(StreamInput in), readGenericValue() can return null (case -1), and these null values are directly assigned to the sortValues array, bypassing the validation in setSortValues().
There was a problem hiding this comment.
Right, I see, but these you can only get from a serialized SearchAfterBuilder, so perhaps null values can only come in from previous versions (before your change is made)? At the same time version based logic complicates things so maybe it is ok to leave it as-is.
There was a problem hiding this comment.
I agree with your point about backward compatibility. Keeping the validation as it is seems like the simplest and safest approach. Appreciate the feedback!
| values.add(parser.booleanValue()); | ||
| } else if (token == XContentParser.Token.VALUE_NULL) { | ||
| values.add(null); | ||
| throw new ParsingException(parser.getTokenLocation(), "Values cannot contain null."); |
There was a problem hiding this comment.
Should we be more specific in the error message that it's the search_after values?
There was a problem hiding this comment.
You're right, that would be much clearer. I think we could update this to:
Values cannot contain null.→[search_after] values cannot contain null.
|
test this please |
5 participants
Change Summary
Fix NullPointerException in LongComparator when search_after contains null values by implementing validation at multiple layers.
As null values in search_after arrays are not valid and cause NPE when passed to Lucene's comparators, this change updates the SearchAfterBuilder to reject null values during JSON parsing, object setting, and FieldDoc creation with clear error messages.
Open to suggestions or corrections!
Related Issue
#132370