-
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
[Index Management] Fix encoding issue on index details page #166882
[Index Management] Fix encoding issue on index details page #166882
Conversation
…y param instead of a path parameter for index name to avoid issues with encoded characters
…-ref HEAD~1..HEAD --fix'
* 2.0. | ||
*/ | ||
|
||
export enum Section { |
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 need to extract these constants to a separate file in common folder so that it could be imported without increasing the bundle size.
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
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 work! I'm happy we were able to find a temporary workaround for this 🎉
The downside of this workaround is that the url semantics doesn't reflect that the index name is mandatory for the page to work.
Does it make sense to add a check that the index was specified as a query param before making the request to fetch the index? We might also consider rendering a slightly different error message in that scenario. Here's the current behavior:

...s/index_management/public/application/sections/home/index_list/details_page/details_page.tsx
Outdated
Show resolved
Hide resolved
…me/index_list/details_page/details_page.tsx Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>
Thank you for the review, @alisonelizabeth! Great point about the empty index name, definitely adding a better error message for this case and will also check if I can prevent the unnecessary request as well. |
…est being unnecessarily sent out
I added a message when no index name is provided and stopped an unnecessary http request (d3591e6). |
…-ref HEAD~1..HEAD --fix'
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
…166882) ## Summary Fixes elastic#166100 This PR adds a workaround fix for the new index details page when opening for index names with special characters, for example `test_index%`. Because of how encoding/decoding works, we can't use the index name as a part of the url like `/indices/${indexName}` (see for more details). Instead we have to pass the index name in a query parameter like `/indices/index_details?indexName=${indexName}. The downside of this workaround is that the url semantics doesn't reflect that the index name is mandatory for the page to work. Once elastic#132600 is resolved, we should revert this workaround and use the index name as a url segment again. Note for reviewers: The jest tests for this fix are part of elastic#165705 ### How to test 1. Add `xpack.index_management.dev.enableIndexDetailsPage: true` to the file `config/kibana.dev.yml` to enable the new index details page 2. Navigate to Index Management and use the "create index" button 3. Type a name with special characters, for example `test%` 4. Click the new index name in the list and check that the details page and all tabs work 5. Also reload the page completely and check that the page still loads correctly --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Alison Goryachev <alisonmllr20@gmail.com>
Summary
Fixes #166100
This PR adds a workaround fix for the new index details page when opening for index names with special characters, for example
test_index%
. Because of how encoding/decoding works, we can't use the index name as a part of the url like/indices/${indexName}
(see for more details). Instead we have to pass the index name in a query parameter like `/indices/index_details?indexName=${indexName}. The downside of this workaround is that the url semantics doesn't reflect that the index name is mandatory for the page to work. Once #132600 is resolved, we should revert this workaround and use the index name as a url segment again.Note for reviewers: The jest tests for this fix are part of #165705
How to test
xpack.index_management.dev.enableIndexDetailsPage: true
to the fileconfig/kibana.dev.yml
to enable the new index details pagetest%