-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add uri decode to es_ui_shared and fix navigation issues with special characters #80835
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
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.
Great job @yuliacech ! Thanks for looking into this, I've tested all the scenario and it all works correctly except in one scenario.
I followed the steps from #80661
When I am in the indices list table, viewing the backing index
When I click on the data stream, the app crashes
ctx.core.elasticsearch.legacy.client.callAsCurrentUser('transport.request', { | ||
path: `/_data_stream/${name}/_stats`, | ||
path: `/_data_stream/${encodeURIComponent(name)}`, |
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 am surprised that we need to use transport.request
here and not in other places (e.g. "cross_cluster_replication" plugin which I do think also allow the same special chars.).
Not sure how you tested here but could you do the same test with a follower index or auto_follow pattern?
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.
You are right, @sebelga ! I reverted the getDataStream
route back to using custom client. As I understand it: the custom client adds encoding automatically when provided with a parameter. When using request (like here for stats), we need to encode manually.
Thanks a lot for your review, @sebelga! I fixed the navigation from indices back to data streams, nice catch :D |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
* under the License. | ||
*/ | ||
|
||
export const attemptToURIDecode = (value: string) => { |
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.
Can we add some unit tests for this function? I'd like to see cases that clarify the nuance behind result
being overwritten once on line 23 and then immediately overwritten again on line 24. I'd also expect the test cases to communicate the use cases for this function to someone who's wondering when and how they should use it.
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.
Thanks for review, @cjcenizal ! I added a comment when to use this function and tests with examples. I deleted decodeURI
since I found out that it's already done in react router.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
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.
Thanks for making the changes @yuliacech ! Tested locally and works as expected 👍
… characters (elastic#80835) * Add uri decode to es_ui_shared and fix data stream issue with % name * Add a test for data streams tab opened for name with a dollar sign * Import uri decode function from es_ui_shared and fix navigation issues for filters * Add tests for data streams with special characters in name * Revert react router navigate changes (is done in a separate PR) * Reverting changes to dataManagement es client and get data stream api route * Fix data stream name filter when activated from a url parameter * Clean up for better consistency and fixes after elastic#81664 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
… characters (#80835) (#81897) * Add uri decode to es_ui_shared and fix data stream issue with % name * Add a test for data streams tab opened for name with a dollar sign * Import uri decode function from es_ui_shared and fix navigation issues for filters * Add tests for data streams with special characters in name * Revert react router navigate changes (is done in a separate PR) * Reverting changes to dataManagement es client and get data stream api route * Fix data stream name filter when activated from a url parameter * Clean up for better consistency and fixes after #81664 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…kibana into alerts/convert-to-tm-intervals * 'alerts/convert-to-tm-intervals' of github.com:gmmorris/kibana: (88 commits) fixed jest APM Experiments settings (elastic#81554) [Resolver] Enable resolver test plugin tests (elastic#81339) Add TS project references for inspector (elastic#81792) Add uri decode to es_ui_shared and fix navigation issues with special characters (elastic#80835) [Fleet] Rename ingestManager translations fleet (elastic#81837) [Logs UI] Transmit and render array field values in log entries (elastic#81385) Audit Logging: use the original url (elastic#81282) [User experience] Fix JS error rate (elastic#81512) [UX] Add median/percentile info in titles (elastic#79824) Support export for SO with circular refs (elastic#81582) Get rid of global types (elastic#81739) [APM] Fix precommit script (elastic#81594) skips overview tests (elastic#81877) [Security Solution][Case] Fix connector's labeling (elastic#81824) Added simple test, which only covers successful case when edit happened right after task was complete previous execution [Maps] Fix EMS test (elastic#81856) [Security Solutions][Detections] - Fix bug, last response not showing for disabled rules (elastic#81783) skip flaky suite (elastic#81853) Fixed type checks and unit tests ...
Summary
Fixes #80661
This PR fixes several issues when navigating to Index Management pages with special characters in names:
Code changes
attemptToURIDecode
to safely decode values.data_stream="%my_data_stream"
)How to test
Dev Tools commands
Release Note
Fixes issues in Index Management and ILM when resources have special characters in names