Refine getIndices() to return an empty array if there are no matching indices. #12659
Conversation
- Return deduplicated aggregate of both alias and indices. - Fix bug that caused an array of the original query and undefined to be returned when the API response was a 404.
There was a problem hiding this comment.
Hmm. I don't understand the need to combine both result sets.
When I did the initial discovery here, I found the .getAlias endpoint returned everything we needed, but only if an existing alias existed. If no aliases were found, then we just need to search against indices (since there are no aliases to also search against).
What are the reasons for always making and combining the two separate calls?
…dy includes the indices.
cjcenizal
force-pushed
the
improvement/get-indices-and-aliases
branch
from
d3f9c00
to
e6f5b01
Compare
cjcenizal
changed the title
|
@chrisronline Ah you're right. Sorry, I think that must have been leftover from my exploration. I've updated the PR and the title/description. Could you take another look? |
src/ui/public/indices/get_indices.js
Outdated
| @@ -1,6 +1,10 @@ | |||
| import { pluck, reduce, size } from 'lodash'; | |||
|
|
|||
| const getIndicesFromResponse = json => { | |||
| const getIndexNamesFromAliasesResponse = json => { | |||
| if (json.status === 404) { | |||
There was a problem hiding this comment.
This shouldn't be necessary as it's handled on line 30
There was a problem hiding this comment.
I was thinking about this too... after chewing it over, I think we should consider it incidental that it's handled on line 30. My basis for this reasoning is that this function doesn't "know" about line 30. So by handling the 404 case we've future-proofed it. We'll never need to change it even if the surrounding code changes (e.g. by exporting this function and using it elsewhere, or by removing the condition on line 30). What do you think?
There was a problem hiding this comment.
Hmm. I'm not so sure I agree, but I don't mind it living here too if you feel strongly about it
There was a problem hiding this comment.
I'd be OK removing this check if we were to scope both helpers to the caller, e.g. by moving them to line 27. Want me to do that? It's just that since they're outside, they look like they're more generalized helper functions, which tend to be a little more bullet proof.
There was a problem hiding this comment.
Sure, but can you define the right under export function IndicesGetIndicesProvider(esAdmin) {?
|
@chrisronline Done! |
This fixes a bug that caused an array of the original query and undefined to be returned when the indices API response was a 404, e.g. submitting
.kibas the query will return[".kib", undefined], but we would expect to get back[].Extracted from bbbe8d3.