[Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler #76106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…fig/log - So that routes don't have to continuously pass it in - just params that actually change (paths, queries) + misc refactors/lint fixes from code review
- This required updating the shared handler to accept custom params set by the route, since the engines route transmutes some param names - createRequest mock also had to be updated to correctly reflect a curried function in order to get tests passing
- Check for /login URL (indicates auth issue) - Refactor final catch return to allow being passed messages from previous throw - Change hasValidData fallback to only log (potentially sensitive?) JSON data to server, not to client - Minor test cleanup - move error tests to single describe block, minor URL/newline cleanup
- This will support future POST requests as well as support (e.g.) 404 responses Minor test refactors: - Set up new request defaults (method, empty body) + expected output & convert KibanaAuthHeader to string since they're set so close to one another - group request pass tests into a describe block, remove response body portion of their tests (since we don't really care about that for those tests) - group response tests into describe block - clarify how passed handler params arg overrides request.params
cee-chen
added
Feature:Plugins
release_note:skip
cee-chen
commented
Aug 27, 2020
Comment on lines
87
to
92
| const errorMessage = `Error connecting to Enterprise Search: ${e?.message || e.toString()}`; | ||
|
|
||
| this.log.error(errorMessage); | ||
| if (e instanceof Error) this.log.debug(e.stack as string); | ||
|
|
||
| return response.customError({ statusCode: 502, body: errorMessage }); |
There was a problem hiding this comment.
😁 I was accidentally able to test the new functionality where we send custom error messages back to the console (instead of the previous generic "Error connecting" message) when implementing the body payload:
VERY useful for debugging, and I'm happy w/ this change - the only thing to keep an eye on is if we start catching errors that leak sensitive info in the message somehow.
cee-chen
commented
Aug 27, 2020
Comment on lines
66
to
68
| const body = Object.keys(request.body as object).length | ||
| ? JSON.stringify(request.body) | ||
| : undefined; |
There was a problem hiding this comment.
Follow up to the above comment:
Kibana's request.body sends an empty obj, while node-fetch's expects an undefined if there's no body, hence this required ternary check.
We might also want to consider checking for method === 'GET'/HEAD and not allowing a body for GET requests (see below repeated screenshot), but that's maybe overkill? What do you think?
JasonStoltz
approved these changes
Aug 27, 2020
| ) => { | ||
| try { | ||
| // Set up API URL | ||
| params = params ?? (request.query ? `?${querystring.stringify(request.query)}` : ''); |
There was a problem hiding this comment.
Could params be an object rather than a string? This would let us "merge" params, if some need to be dynamic and some static.
const allParams = {
...(params && { params }), // base params
...(request.query && { request.query }) // override / mix in dynamic params
}
params = (Object.keys(allParams).length > 0 ? `?${querystring.stringify(allParams)}` : '');
There was a problem hiding this comment.
Ah! Great call, thanks!
There was a problem hiding this comment.
f6a709d - this is so much better, thanks for the amazing suggestion!!
I also realized while testing it that request.query is also an empty obj so in reality our request.query ? check was never failing 🤦♀️
| return response.customError({ statusCode: 502, body: 'cannot-connect' }); | ||
| } | ||
| })}`, | ||
| hasValidData: (body?: IEnginesResponse) => |
There was a problem hiding this comment.
Totally unrelated to this PR. Do we need to add this for every API request 🤔
There was a problem hiding this comment.
Nah, probably not. I think I originally added it to catch issues like the /login redirect (or anything else cropping up that I couldn't predict due to auth issues). I'd say it's probably only super important for pages where the front-end will crash with a typeerror of some kind if it receives unexpected data. 🤷
There was a problem hiding this comment.
To be honest I'd be fine if most API routes going forward didn't have this, and we added them on an as-needed basis (if we started getting error reports for bad API endpoints, for example).
💛 Build succeeded, but was flaky
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/infra/home_page·ts.InfraOps app Home page with metrics present records telemetry for dockerStandard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
cee-chen
pushed a commit
that referenced
this pull request
Aug 28, 2020
…xisting routes to use shared handler (#76106) (#76232) * Refactor enterprise_search_request_handler to a class that stores config/log - So that routes don't have to continuously pass it in - just params that actually change (paths, queries) + misc refactors/lint fixes from code review * Update mocks/dependencies so other tests pass type checks * Update /api/workplace_search/overview endpoint to use new request handler * Update /api/app_search/engines route to use new handler - This required updating the shared handler to accept custom params set by the route, since the engines route transmutes some param names - createRequest mock also had to be updated to correctly reflect a curried function in order to get tests passing * DRY out hasValidData to a reusable/cleaner call * Update shared handler to output specific message for auth errors - Check for /login URL (indicates auth issue) - Refactor final catch return to allow being passed messages from previous throw - Change hasValidData fallback to only log (potentially sensitive?) JSON data to server, not to client - Minor test cleanup - move error tests to single describe block, minor URL/newline cleanup * Update handler to pass method & body requests + pass back response code - This will support future POST requests as well as support (e.g.) 404 responses Minor test refactors: - Set up new request defaults (method, empty body) + expected output & convert KibanaAuthHeader to string since they're set so close to one another - group request pass tests into a describe block, remove response body portion of their tests (since we don't really care about that for those tests) - group response tests into describe block - clarify how passed handler params arg overrides request.params * PR feedback: Update custom params arg to take an object instead of a query string
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Aug 31, 2020
…s-for-710 * 'master' of github.com:elastic/kibana: (43 commits) [APM] Chart units don't update when toggling the chart legends (elastic#74931) [ILM] Add support for frozen phase in UI (elastic#75968) Hides advanced json for count metric (elastic#74636) add client-side feature usage API (elastic#75486) [Maps] add drilldown support map embeddable (elastic#75598) [Enterprise Search] Request handler refactors/enhancements + update existing routes to use shared handler (elastic#76106) [Resolver] model `location.search` in redux (elastic#76140) [APM] Prevent imports of public in server code (elastic#75979) fix eslint issue skip flaky suite (elastic#76223) [APM] Transaction duration anomaly alerting integration (elastic#75719) [Transforms] Avoid using "Are you sure" (elastic#75932) [Security Solution][Exceptions] - Fix bug of alerts not updating after closure from exceptions modal (elastic#76145) [plugin-helpers] improve 3rd party KP plugin support (elastic#75019) [docs/getting-started] link to yarn v1 specifically (elastic#76169) [Security_Solution][Resolver] Resolver loading and error state (elastic#75600) Fixes App Search documentation links (elastic#76133) Fix alerts unable to create / update when the name has trailing whitepace(s) (elastic#76079) [Resolver] Fix useSelector usage (elastic#76129) [Enterprise Search] Migrate util and components from ent-search (elastic#76051) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/data_tier_allocation/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/policies/types.ts # x-pack/plugins/index_lifecycle_management/public/application/services/policies/warm_phase.ts
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR follows up on the awesome work done in #75487 🎉
/api/app_search/enginesand/api/workplace_search/overviewto use the shared request handlerenterpriseSearchRequestHandlerto a classEnterpriseSearchRequestHandlerplugin.tsload withconfigandlogdependenciesrequest.query/api/app_Search/enginesendpoint, which renames some params between Kibana client-side and Enterprise Search API, as well as stores a param (page size) as a constant instead of passing it between public/ & server/./loginredirectsmockRequestHandlermock that represents an enterpriseSearchRequestHandler, and can be passed via mockDependencieshasValidDatahelper which pulls out the slightly uglyjestMock.mock.calls[0][0]syntax for us (which I'd previously whinged about)QA
Note that
localhost:5601/api/enterprise_search/config_dataandlocalhost:5601/api/enterprise_search/telemetryremain unchanged (telemetry because it doesn't need to hit Enterprise Search's API, and config_data because it has a decent amount of custom behavior such as a signal controller).Checklist