FetchFieldsPhase needs to use local docid, not global #90901
Conversation
romseygeek
added
>non-issue
:Search/Search
|
Marked as |
|
Pinging @elastic/es-search (Team:Search) |
| when(sec.nestedLookup()).thenReturn(NestedLookup.EMPTY); | ||
| FetchContext fetchContext = mock(FetchContext.class); | ||
| when(fetchContext.fetchFieldsContext()).thenReturn(ffc); | ||
| when(fetchContext.getSearchExecutionContext()).thenReturn(sec); |
There was a problem hiding this comment.
Would it be a lot of work to try and do without mocking? MapperServiceTestCase has a utility method to create a search execution context but it requires a mapper service instance. Wondering if it may be worth changing.
There was a problem hiding this comment.
We need a special MappedFieldType that returns a DocValueFetcher, and we don't have any core mappers that use DocValueFetcher. So somewhat counter-intuitively, not using mocks here ends up with much more ceremony because we need to register plugins and create special Mappers and TypeParsers, etc.
I would love to extract some interfaces from SearchExecutionContext though. Loads of places that take it only need the 'return all matching field types' functionality, and it's a very heavy object to construct for tests.
This commit passes the correct docid to value fetchers, and adds a new test
to exercise the behaviour explicitly and guard against regressions.
Fixes #90881