[App Search] DRY helper for encoding/decoding routes that can have special characters in params #89811

cee-chen · 2021-01-30T00:21:20Z

Summary

There are two instances in App Search's routing where URLs can have special characters (user generated):

Document details, e.g. /app_search/engines/national-parks-demo/documents/test@#$
Analytics queries, e.g. /app_search/engines/national-parks-demo/analytics/query_detail/test@#$

Testing these changes

Documents testing:

Go to http://localhost:5601/xyz/app/enterprise_search/app_search/engines/national-parks-demo/documents
Index a new document with the following ID:

[
  {
    "id": "test!@#$%^&*[]/|;:'<>~`",
    "title": "test"
  }
]

Refresh the page and click on your newly created document
Confirm that the page correctly loads:

Analytics testing:

Go to http://localhost:5601/xyz/app/enterprise_search/app_search/engines/national-parks-demo/documents (or go to localhost:3002's query tester and use that, whatever works)
Search for test!@#$%^&*[]/|;:'<>~`
Now go to Analytics, and find your recent search query
Click on a link to the query from either the name in a table or the eye icon
Confirm that the page correctly loads:

NOTE

There is still a bug when you reload the page on a document or query with the special character % in the title. React Router tries to double-decode the % and unfortunately breaks the page. This is an issue with the history library, NOT our code, and there is nothing we can do about it: #82440

Checklist

Unit or functional tests were updated or added to match the most common scenarios

- Primarily document URLs & analytics queries (which uses generateEnginePath)

- Documents titles & Analytics queries

- Feedback from Davey - the pages don't open in a new window, so shouldn't use the popout icon - Not strictly related but since we're touching these links anyway, I'm shoving it in (sorry)

cee-chen · 2021-01-30T00:22:51Z

.../applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx

@@ -56,7 +56,7 @@ export const ACTIONS_COLUMN = {
        { defaultMessage: 'View query analytics' }
      ),
      type: 'icon',
-      icon: 'popout',
+      icon: 'eye',


This change has nothing to do with encoding/decoding, but Davey requested it recently (since the popout icon typically indicates a new tab/window, and that's not the case here) and I figure I'd shove it in this PR since it touches the 2 concerned views 😬 If you super hate that lmk and I can stop being lazy & pull it out to a separate PR haha

It's fine.........😒

cee-chen · 2021-01-30T00:24:10Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/utils.ts

 import { EngineLogic } from './';

 /**
 * Generate a path with engineName automatically filled from EngineLogic state
 */
 export const generateEnginePath = (path: string, pathParams: object = {}) => {
  const { engineName } = EngineLogic.values;
-  return generatePath(path, { engineName, ...pathParams });
+  return generatePath(path, encodePathParams({ engineName, ...pathParams }));


My hope is that by baking this into our new generateEnginePath helper that it'll "just work" going forward and at the very least we won't accidentally forget to encode user-generated IDs going forward.

I think the problem I see here is that you are actually increasing the odds that someone will accidentally forget to encode user-generate ids, because as soon as you're in a place in the code you're not able to use generateEnginePath, then you won't be in the habit of encoding path params.

I really wish there was just one way to create paths that we could used everywhere, decoupled from state, that would handle encoding path params.

generateAppPath(ENGINE_DOCUMENT_DETAIL_PATH, { engineName: resultMeta.engine, documentId: resultMeta.id, })

cee-chen · 2021-01-30T00:28:05Z

...k/plugins/enterprise_search/public/applications/app_search/utils/encode_path_params/index.ts

+  });
+
+  return decodedParams;
+};


Just want to note - on some reflection, it's maybe a little overkill to have a helper for 2 instances in our app, but I think it'll be worth it (from the perspective of future-proofing) that we have one documented and canonical place where we ensure our URLs are correctly encoded, vs. doing it manually and maybe slightly differently in multiple places in the future.

cee-chen · 2021-01-30T00:29:59Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

+  const documentLink = generatePath(
+    ENGINE_DOCUMENT_DETAIL_PATH,
+    encodePathParams({
+      engineName: resultMeta.engine,
+      documentId: resultMeta.id,
+    })
+  );


Instead of using the default RR generatePath, I'm tempted to convert this to

generateEnginePath(ENGINE_DOCUMENT_DETAIL_PATH, { engineName: resultMeta.engine, documentId: resultMeta.id, })

Just so we get the encoding baked in without having to manually call it. I know Jason mentioned earlier that was maybe confusing though 🤷 What do you all think?

I left another comment about a more generic generateAppPath that I think applies here.

My objection still stands. It's like we're trying to force things through this state based helper that don't make sense to be part of a state based helper.

That being said, it's also inconsequential to me. It will be just fine if you choose to do this. I stated my objection, but either way will be just fine.

Ya I think you're right about that, I shouldn't be conflating the two different helpers. I'm good with a generic generateAppPath (or maybe generateEncodedPath if it should be specific to encoding?), will add that helper to this PR and have generateEnginePath call that helper.

68a3057 & a72aef2

JasonStoltz

Thanks for doing this Constance, this will be super helpful. I left optional feedback.

- now handled/tested by useDecodedParams helper

- Should be used in place of generatePath

for consistency across the App Search codebase

cee-chen · 2021-02-02T00:10:37Z

@elasticmachine merge upstream

cee-chen · 2021-02-02T00:57:08Z

@elasticmachine merge upstream

kibanamachine · 2021-02-02T02:40:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8f3568f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1187	1188	+1

Async chunks

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

id	before	after	diff
`enterpriseSearch`	1.8MB	1.8MB	+1.0KB

History

💔 Build #102821 failed a64d53a
💔 Build #102406 failed c0ad74e

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

byronhulcher

This change is kind of a net zero for me, so I'm not going to block or request further changes, but I want to say it's not the direction I'd have gone. I'm not sure what we gain out of these helpers (or useDecodedParams) when they're mostly just wrappers around the base JavaScript API. If I manage to remember my path requires encoding (which is already a lot to ask of me tbh), I think the solution to that should be "use the JavaScript APIs that appear when I google 'stackoverflow encode url param' and not to hunt in our codebase to see whether we've written these helpers already.

If it was me I think I'd have relied on the default generatePath and useParams, changed param IDs in paths from stuff like :documentId to :encodedDocumentId and then either decoded encodedDocumentId in the component after I get the value from useParams or as a part of the relevant logic file, where I would pass encodedDocumentId directly instead of documentId.

Again, this is fine and if we want to go this way I'm down, it's just the pattern I'd have used.

byronhulcher · 2021-02-02T13:31:47Z

.../applications/app_search/components/analytics/components/analytics_tables/shared_columns.tsx

@@ -56,7 +56,7 @@ export const ACTIONS_COLUMN = {
        { defaultMessage: 'View query analytics' }
      ),
      type: 'icon',
-      icon: 'popout',
+      icon: 'eye',


It's fine.........😒

cee-chen · 2021-02-02T17:40:12Z

If I manage to remember my path requires encoding

I think the key here, especially for me who spent about half a dev day debugging this on Kibana, is that it's not obvious which encoding our server API expects and why. It's encodeURIComponent and not encodeURI, but that was not clear to me (and not something I would have personally expected in a non ?query= param) without any documentation/comments.

It also was not obvious to me that RR's generatePath automatically encodeURIs on top of that in history 4.5.0+, and that context is now additionally here as well in this work.

This helper manages all of that for us in a single place, making it easy to check git history & context. I know we're only really using this in 2 places in the codebase and that makes it "feel" not super useful right now, but I do think baking encoding into generateEnginePath future-proofs a lot more of these cases for us and is generally more secure than relying on us to remember to encode/decode (which we clearly didn't while porting to Kibana).

…-ml-jobs * 'master' of github.com:elastic/kibana: (254 commits) [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927) Skip test for cloud (elastic#89450) [Fleet] Fix duplicate data streams being shown in UI (elastic#89812) Bump package dependencies (elastic#90034) [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811) TypeScript project references for Observability plugin (elastic#89320) [SearchSource] Combine sort and parent fields when serializing (elastic#89808) Made imports static (elastic#89935) [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640) [Discover] Adapt default column behavior (elastic#89826) Round start and end values (elastic#89030) Rename getProxyAgents to getCustomAgents (elastic#89813) [Form lib] UseField `onError` listener (elastic#89895) [APM] use latency sum instead of avg for impact (elastic#89990) migrate more core-owned plugins to tsproject ref (elastic#89975) [Logs UI] Load <LogStream> entries via async searches (elastic#86899) [APM] Abort browser requests when appropriate (elastic#89557) [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062) [Data Table] Use shared CSV export mechanism (elastic#89702) chore(NA): improve logic check when installing Bazel tools (elastic#89634) ...

…ecial characters in params (#89811) (#90080) * Add encodePathParam helper + update components that need it - Primarily document URLs & analytics queries (which uses generateEnginePath) * Add useDecodedParams helper + update components that need it - Documents titles & Analytics queries * [Misc] Change popout icon to eye - Feedback from Davey - the pages don't open in a new window, so shouldn't use the popout icon - Not strictly related but since we're touching these links anyway, I'm shoving it in (sorry) * Remove document detail decode test - now handled/tested by useDecodedParams helper * Add new generateEncodedPath helper - Should be used in place of generatePath * Update all instances of generatePath to generateEncodedPath for consistency across the App Search codebase * Fix failing tests due to extra encodeURI() done by generatePath * Add missing branch test for analytics query titles Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen added 3 commits January 29, 2021 16:18

Add encodePathParam helper + update components that need it

8a644db

- Primarily document URLs & analytics queries (which uses generateEnginePath)

Add useDecodedParams helper + update components that need it

5a2d9a6

- Documents titles & Analytics queries

[Misc] Change popout icon to eye

c0ad74e

- Feedback from Davey - the pages don't open in a new window, so shouldn't use the popout icon - Not strictly related but since we're touching these links anyway, I'm shoving it in (sorry)

cee-chen added bug Fixes for quality problems that affect the customer experience Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 30, 2021

cee-chen requested review from JasonStoltz, a team and byronhulcher January 30, 2021 00:21

cee-chen commented Jan 30, 2021

View reviewed changes

JasonStoltz approved these changes Feb 1, 2021

View reviewed changes

cee-chen added 5 commits February 1, 2021 14:51

Remove document detail decode test

12e4db9

- now handled/tested by useDecodedParams helper

Add new generateEncodedPath helper

68a3057

- Should be used in place of generatePath

Update all instances of generatePath to generateEncodedPath

a72aef2

for consistency across the App Search codebase

Fix failing tests due to extra encodeURI() done by generatePath

9dd3fdf

Add missing branch test for analytics query titles

b4b4b6a

cee-chen requested a review from a team February 2, 2021 00:10

Merge branch 'master' into encode-decode-helper

a64d53a

Merge branch 'master' into encode-decode-helper

8f3568f

JasonStoltz self-requested a review February 2, 2021 13:20

JasonStoltz approved these changes Feb 2, 2021

View reviewed changes

byronhulcher reviewed Feb 2, 2021

View reviewed changes

cee-chen merged commit 977fc6c into elastic:master Feb 2, 2021

cee-chen deleted the encode-decode-helper branch February 2, 2021 17:41

cee-chen mentioned this pull request Feb 2, 2021

[7.x] [App Search] DRY helper for encoding/decoding routes that can have special characters in params (#89811) #90080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] DRY helper for encoding/decoding routes that can have special characters in params #89811

[App Search] DRY helper for encoding/decoding routes that can have special characters in params #89811

cee-chen commented Jan 30, 2021

cee-chen Jan 30, 2021

byronhulcher Feb 2, 2021

cee-chen Jan 30, 2021

JasonStoltz Feb 1, 2021 •

edited

Loading

cee-chen Jan 30, 2021

JasonStoltz Feb 1, 2021

cee-chen Jan 30, 2021

JasonStoltz Feb 1, 2021

JasonStoltz Feb 1, 2021

cee-chen Feb 1, 2021

cee-chen Feb 2, 2021

JasonStoltz left a comment

cee-chen commented Feb 2, 2021

cee-chen commented Feb 2, 2021

kibanamachine commented Feb 2, 2021

byronhulcher left a comment •

edited

Loading

byronhulcher Feb 2, 2021

cee-chen commented Feb 2, 2021 •

edited

Loading

[App Search] DRY helper for encoding/decoding routes that can have special characters in params #89811

[App Search] DRY helper for encoding/decoding routes that can have special characters in params #89811

Conversation

cee-chen commented Jan 30, 2021

Summary

Testing these changes

NOTE

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

cee-chen commented Feb 2, 2021

cee-chen commented Feb 2, 2021

kibanamachine commented Feb 2, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

byronhulcher left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Feb 2, 2021 • edited Loading

JasonStoltz Feb 1, 2021 •

edited

Loading

byronhulcher left a comment •

edited

Loading

cee-chen commented Feb 2, 2021 •

edited

Loading