Skip to content
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

[Maps] Ensure draw tools updates by index name, not index pattern title #108394

Merged

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Aug 12, 2021

Draw tools currently validates index patterns that match a single index. This works fine when both the index and pattern have the same name, but since we were passing the index pattern name rather than the index name for writes, we run into errors when the index pattern has a different name than the index. Like test* for the pattern vs. testdraw for the index. This ensures all 1:1 matches between index patterns and associated indexes are correctly validated and the index name is used for udpates.

image

Also fixed:

  • Inconsistent route shape. For consistency with our other endpoints and appearance, it makes more sense to make a call to /api/maps/getMatchingIndexes?indexPattern=test* vs. /api/maps/getMatchingIndexes/test*
  • Fixed route return type on ES 404. An ES 404 resp means there were no matching indexes for the pattern supplied. This should result in a valid response from this endpoint and should return an empty array
  • Misc. type updates

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.15.0 v7.14.1 labels Aug 12, 2021
@kindsun kindsun marked this pull request as ready for review August 17, 2021 22:28
@kindsun kindsun requested a review from a team as a code owner August 17, 2021 22:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@kindsun kindsun changed the title [Maps] Fix issue where draw tools tries to write to index pattern name that doesn't match index name [Maps] Ensure draw tools updates by index name, not index pattern title Aug 17, 2021
const {
payload: { matchingIndexes },
} = await getMatchingIndexes(this.indexPattern.title);
return matchingIndexes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to handle the case where matchingIndexes is undefined and return empty array in that case. This can be handled here or in getMatchingIndexes. Maybe it would be better if handled in getMatchingIndexes and getMatchingIndexes could just return an string[] and better encapsulate the API response.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with how we're handling other endpoints, I've made it conditional on whether the call was successful or not. Successful calls to the endpoint always return an array. Unsuccessful calls don't which we now handle here by detecting and returning an empty array from the function.

@kindsun kindsun requested a review from nreese August 18, 2021 16:57
@mistic mistic added v7.16.0 and removed v7.15.0 labels Aug 18, 2021
@nreese nreese added the v7.15.0 label Aug 18, 2021
@kindsun kindsun requested a review from nreese August 18, 2021 20:44
@kindsun
Copy link
Contributor Author

kindsun commented Aug 20, 2021

@elasticmachine merge upstream

@kindsun kindsun force-pushed the fix-draw-tools-index-pattern-index-mismatch branch from 1a60707 to 7dbc9b8 Compare August 24, 2021 19:23
@kindsun kindsun requested a review from nreese August 26, 2021 16:04
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gchaps Would you mind reviewing the copy?

x-pack/plugins/maps/public/actions/map_actions.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/public/actions/map_actions.ts Outdated Show resolved Hide resolved
@kindsun kindsun requested a review from nreese August 30, 2021 16:34
if (errorStatusCode === 404) {
return response.ok({ body: { success: true, matchingIndexes: [] } });
} else {
logger.error(error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to surface this Error in the Kibana logs? I think since the errors are handled by the UI and messages are displayed to the user, we should not surface these errors in the kibana logs. Thoughts?

Copy link
Contributor Author

@kindsun kindsun Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's consistent with the handling for our other feature editing endpoints (see here and here for example). I think there's some future work in here around possibly creating a wrapper function for endpoints where we decide if there's some standard or conditional handling we want to apply to all endpoints (error catching, logging, etc.). Since this is a bug fix, I'm mostly just trying to match existing patterns and not rock the boat too much.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
code review

@kindsun kindsun requested a review from gchaps August 30, 2021 23:10
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.1MB 3.1MB +1.0KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kindsun kindsun merged commit 10185fe into elastic:master Aug 31, 2021
@kindsun kindsun deleted the fix-draw-tools-index-pattern-index-mismatch branch August 31, 2021 14:49
kindsun pushed a commit to kindsun/kibana that referenced this pull request Aug 31, 2021
kindsun pushed a commit to kindsun/kibana that referenced this pull request Aug 31, 2021
kindsun pushed a commit to kindsun/kibana that referenced this pull request Aug 31, 2021
…le (elastic#108394)

# Conflicts:
#	x-pack/plugins/maps/public/classes/sources/es_search_source/es_search_source.tsx
kindsun pushed a commit that referenced this pull request Aug 31, 2021
…le (#108394) (#110649)

# Conflicts:
#	x-pack/plugins/maps/public/classes/sources/es_search_source/es_search_source.tsx
kindsun pushed a commit that referenced this pull request Aug 31, 2021
…le (#108394) (#110643)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants