Convert TypeFieldType to a constant field type #63214
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
| if (fullName.equals(TypeFieldMapper.NAME)) { | ||
| String type = mapper == null ? null : mapper.type(); | ||
| return new TypeFieldMapper.TypeFieldType(type); | ||
| } |
There was a problem hiding this comment.
for my understanding: does this mean that the field type does not come back with the below field type lookup? Did it before?
There was a problem hiding this comment.
It does come back with the lookup, but it may not have the correct type set on it (for example, if the index in question did not have any mappings set when the MapperService was originally built). This way we know we always have the correct type.
There was a problem hiding this comment.
ok, so could there be places where the type field mapper is retrieved in some way that could expose the wrong type then?
There was a problem hiding this comment.
From a quick review:
- the only other way to access field types is via
MapperService#documentMapper#fieldTypes()directly - this is exposed via
MapperService#fieldTypes() MapperService#fieldTypes()is only called by the index warming code, and the type field does not use global ordinals so is unaffected
So I think we're safe. We should probably remove MapperService#fieldTypes() at some point though, as it is indeed asking for trouble to expose this to the outside world.
There was a problem hiding this comment.
Thanks for keeping me honest :)
#63214 made TypeFieldType a constant field, and fixed things so that it always emits deprecation warnings whenever it is referenced in a query or aggregation. However, it also emits warnings when it is used to build a type filter through the search context; this is unnecessary, as warnings are already emitted by the REST layer when types are specified as part of the URL, and it is causing failures in some BWC tests. This commit adds a specialised typeFilter method to TypeFieldType to handle this case without emitted any extra warnings. It also removes an unused duplicate TypeFieldType class that resulted from a backport merge error. Fixes #63366
#63214 made TypeFieldType a constant field, and fixed things so that it always emits deprecation warnings whenever it is referenced in a query or aggregation. However, it also emits warnings when it is used to build a type filter through the search context; this is unnecessary, as warnings are already emitted by the REST layer when types are specified as part of the URL, and it is causing failures in some BWC tests. This commit adds a specialised typeFilter method to TypeFieldType to handle this case without emitted any extra warnings. It also removes an unused duplicate TypeFieldType class that resulted from a backport merge error. Fixes #63366
3 participants
In 6x and 7x, indexes can have only one type, which means that we can rework
all queries against the type field to use a ConstantFieldType. This has already
been done in master with the removal of the TypeFieldMapper, but we still need
that class in 7x to deal with nested documents. This commit leaves
TypeFieldMapper in place, but refactors TypeFieldType to extend
ConstantFieldType and consolidates deprecation warnings within that class.
It also incidentally removes the requirement to pass a MapperService to
IndexFieldData.Builder#build, which should allow #63197 to be backported.