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.

Sign up for GitHub

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

Jump to bottom

[Discover] Fix context view for document ids containing special characters #122737

Merged
merged 5 commits into from
Jan 14, 2022
Merged

[Discover] Fix context view for document ids containing special characters #122737

merged 5 commits into from
Jan 14, 2022

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Jan 12, 2022 •
edited
Loading

Summary

Fixes #122638

This PR adds decoding for encoded document id param.

Testing notes

  1. Create a document containing a + in its id
PUT context/_doc/1+1=2
{
  "timestamp":"2022-01-10T09:30:23",
  "name":"test"
}
  1. Create a data view
  2. Navigate to Discover, adapt time range so you can find the document
  3. Expand the document and click on "View surrounding documents"
  4. Context view of anchor document should be opened without errors.

@dimaanj dimaanj added Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v7.17.0 v8.2.0 labels Jan 12, 2022
@dimaanj dimaanj self-assigned this Jan 12, 2022
@dimaanj dimaanj requested a review from kertal January 12, 2022 09:35
kertal
kertal reviewed Jan 12, 2022
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Looking good, with a functionals its then 👍

kertal
kertal reviewed Jan 13, 2022
View reviewed changes
test/functional/apps/discover/_context_encoded_url_param.ts Outdated
Comment on lines 21 to 30
await PageObjects.common.navigateToApp('settings');
await es.transport.request({
path: '/includes-plus-symbol-doc-id/_doc/1+1=2',
method: 'PUT',
body: {
username: 'matt',
'@timestamp': '2015-09-21T09:30:23',
},
});
await PageObjects.settings.createIndexPattern('includes-plus-symbol-doc-id');
Copy link
Member

Choose a reason for hiding this comment

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

didn't have this way to create testdata on the radar, TIL. only disadvantage is, that it might be slower, more UI interaction needed to create the required testing data but, fine with that.
only nit: username should be Dmitry, the matt you are mentioning here just added comments here, else he was lazy as usual 😄.

kertal
kertal approved these changes Jan 13, 2022
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM, thx for the quick fix and the functional. 2 request, the title could be more descriptive like

[Discover] Fix context view for document ids containing special characters

And the description could and the info how to test it, even if it's a copy of the issue, it makes our testing team life easier, you know one click less, all info on one page.

@dimaanj dimaanj changed the title [Discover] Fix encoded param for context [Discover] Fix context view for document ids containing special characters Jan 14, 2022
@dimaanj dimaanj marked this pull request as ready for review January 14, 2022 07:45
@dimaanj dimaanj requested a review from a team as a code owner January 14, 2022 07:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@dimaanj
Copy link
Contributor Author

dimaanj commented Jan 14, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 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
discover 330.7KB 330.8KB +73.0B

History

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

cc @dmitriynj

@dimaanj dimaanj added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 14, 2022
@dimaanj dimaanj merged commit 6956d9c into elastic:main Jan 14, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.0 Backport failed because of merge conflicts
❌ 7.17 Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 122737

Questions ?

Please refer to the Backport tool documentation

dimaanj added a commit to dimaanj/kibana that referenced this pull request Jan 14, 2022
@dimaanj
[Discover] Fix context view for document ids containing special chara…
91f8d4f 
…cters (elastic#122737)

* [Discover] fix encoded param for context

* [Discover] add functional test

* [Discover] add test file

* [Discover] change field name

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 6956d9c)

# Conflicts:
#	src/plugins/discover/public/application/apps/context/context_app_route.tsx
@dimaanj dimaanj mentioned this pull request Jan 14, 2022
Merged
dimaanj added a commit to dimaanj/kibana that referenced this pull request Jan 14, 2022
@dimaanj
[Discover] Fix context view for document ids containing special chara…
8b921aa 
…cters (elastic#122737)

* [Discover] fix encoded param for context

* [Discover] add functional test

* [Discover] add test file

* [Discover] change field name

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 6956d9c)

# Conflicts:
#	src/plugins/discover/public/application/apps/context/context_app_route.tsx
@dimaanj dimaanj mentioned this pull request Jan 14, 2022
Merged
dimaanj added a commit that referenced this pull request Jan 14, 2022
@dimaanj
[Discover] Fix context view for document ids containing special chara…
28725c3 
…cters (#122737) (#123035)

* [Discover] fix encoded param for context

* [Discover] add functional test

* [Discover] add test file

* [Discover] change field name

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 6956d9c)

# Conflicts:
#	src/plugins/discover/public/application/apps/context/context_app_route.tsx
dimaanj added a commit that referenced this pull request Jan 14, 2022
@dimaanj
[8.0] [Discover] Fix context view for document ids containing special…
87c0f11 
… characters (#122737) (#123034)

* [Discover] Fix context view for document ids containing special characters (#122737)

* [Discover] fix encoded param for context

* [Discover] add functional test

* [Discover] add test file

* [Discover] change field name

Co-authored-by: Kibana Machine <[email protected]>
(cherry picked from commit 6956d9c)

# Conflicts:
#	src/plugins/discover/public/application/apps/context/context_app_route.tsx

* [Discover] fix linting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

Assignees

@dimaanj dimaanj

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discover] Context view doesn't work when there are encoded special chars in the id given by URL
5 participants
@dimaanj @elasticmachine @kibana-ci @kibanamachine @kertal