Fix Term Vectors with artificial docs and keyword fields

[matriv]

Previously, Term Vectors API was returning empty results for
artificial documents with keyword fields. Checking only for string()
on IndexableField is not enough, since for KeywordFieldType
binaryValue() must be used instead.

Fixes #53494

[elasticmachine]

Pinging @elastic/es-search (:Search/Search)

[jimczi]

Good catch @matriv, I left a comment regarding the applicability of the fix.

[jimczi]

We cannot use the binary value without checking the type of the field. It could be a real binary field that doesn't store utf8 bytes. Since this function is only used in the terms vector service, can you move it there and adds a check to extract binary values only if the field is of type keyword ?

[matriv]

The method is called as you said only by TermVectorService and it's guarded by isValidField:
https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java#L304 but I agree, it makes sense to move the method close to this so it's visible.

[matriv]

@jimczi I moved it to TermVectorService but I'm unsure if it worths avoiding the instanceof.

[jimczi]

Thanks for moving the function. I left one comment regarding the field value extraction.

[jimczi]

I think that this will create duplicates due to doc values field that also use a binary value (SortedSetDocValuesField). You should change this to something like:

if (field.fieldType() instanceof KeywordFieldMapper.KeywordFieldType) {
  result.add(field.binaryValue().utf8ToString());
} else if (field.fieldType() instanceof StringFieldType) {
  result.add(field.stringValue());
}

[jimczi]

LGTM