Avoid NPE during query parsing #10333
Conversation
|
In case #10124 isn't feasible... |
|
I think this looks good! Can you please sign the CLA for this contribution? @kimchy can you please take another look at this too? |
| // TODO: Remove this when we upgrade jackson to 2.6.x. | ||
| char[] chars = parser.getTextCharacters(); | ||
| if (chars == null) { | ||
| chars = EMPTY_CHARS; |
There was a problem hiding this comment.
can we just return new BytesRef() in this case? I don't know if the text characters are null if we can really rely on text offset and length? Maybe a better check, not relying on null check is:
if (parser.getTextLength() == 0) {
return new BytesRef();
}
|
I made a small comment here, I think it would be cleaner not to rely on null value for the char array, but use the length and check if its 0. If agreed, +1 on pushing it, thanks @masaruh! |
masaruh
changed the title
|
Thanks @kimchy! Addressed your comment along with a few changes to make tests pass (rebase, include license header, extend ElasticsearchTestCase). |
| return new BytesRef(CharBuffer.wrap(parser.getTextCharacters(), parser.getTextOffset(), parser.getTextLength())); | ||
| // Tentative workaround for https://github.com/elastic/elasticsearch/issues/8629 | ||
| // TODO: Remove this when we upgrade jackson to 2.6.x. | ||
| char[] chars = parser.getTextCharacters(); |
There was a problem hiding this comment.
we don't need this now, and can call parser.getTextCharacters() in line 96?
|
@masaruh left another really small comment, other than that, LGTM |
Workaround until Jackson is upgraded to 2.6.x. Fixes elastic#8629.
|
Thanks @kimchy. Fixed and merged. |
clintongormley
changed the title
clintongormley
added
:Search/Search
Workaround until Jackson is upgraded to 2.6.x.