New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always return metadata in get/search APIs. #11816
Conversation
@@ -138,7 +139,7 @@ public void testExplainWithFields() throws Exception { | |||
assertThat(response.getExplanation().getValue(), equalTo(1.0f)); | |||
assertThat(response.getGetResult().isExists(), equalTo(true)); | |||
assertThat(response.getGetResult().getId(), equalTo("1")); | |||
assertThat(response.getGetResult().getFields().size(), equalTo(1)); | |||
assertThat(response.getGetResult().getFields().size(), greaterThanOrEqualTo(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this greaterThanOrEqualTo()
? Do we not know how many metadata fields we should get returned here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have random index templates that enable _timestamp
for instance, which will be part of the list of return fields now that we force it to be returned when enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I didn't know that, thanks. I wonder if there is some way we can get the list of fields that have been randomly added? The problem I see here is that we aren't really testing that only the requesting fields are returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could change the assertion to something like fields.keySet().containsAll(Arrays.asList(expectedField1, expectedField2))
?
@colings86 I pushed a new commit |
} else if (field.equals(TimestampFieldMapper.NAME) && docMapper.timestampFieldMapper().enabled()) { | ||
value = source.timestamp; | ||
} else if (field.equals(TTLFieldMapper.NAME) && docMapper.TTLFieldMapper().enabled()) { | ||
// Call value for search with timestamp + ttl here to display the live remaining ttl value and be consistent with the search result display |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to -> so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops nevermind, I misread.
LGTM |
This commit makes the get and search APIs always return `_parent`, `_routing`, `_timestamp` and `_ttl` in addition to `_id` and `_type`. This way, consumers always have all required information in order to reindex a document.
0801d88
to
e4d475c
Compare
Always return metadata in get/search APIs.
This commit makes the get and search APIs always return
_parent
,_routing
,_timestamp
and_ttl
in addition to_id
and_type
. This way, consumersalways have all required information in order to reindex a document.
Closes #8068