Fix common router request handler issue #32275
Conversation
jen-huang
added
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui |
| } catch (err) { | ||
| if (isEsError(err)) { | ||
| throw wrapEsError(err); | ||
| return wrapEsError(err); |
There was a problem hiding this comment.
usage of throw wasn't an issue, but I changed it to return since that is more commonly used to return errors in x-pack server code
There was a problem hiding this comment.
@jen-huang Just seeing this. Have you tested this? It seems that by not throwing we would never be able to catch an error...
| export const callWithRequestFactory = (server, request) => { | ||
| return (...args) => { | ||
| return callWithRequest(server)(request, ...args); | ||
| const callWithRequest = server => { |
There was a problem hiding this comment.
this also wasn't necessarily an issue, but since this is now common code shared by multiple plugins, it should be a true factory
💚 Build Succeeded |
| const cluster = server.plugins.elasticsearch.getCluster('data'); | ||
| return cluster.callWithRequest; | ||
| }; | ||
| return once(callWithRequest)(server)(request, ...args); |
There was a problem hiding this comment.
I'm sure we've had discussion about this before and I just missed it, but what's the benefit of using once here? If it's not needed, then this code could be simplified a bit. I tested this locally and it seemed OK:
export const callWithRequestFactory = (server, request) => {
const { callWithRequest } = server.plugins.elasticsearch.getCluster('data');
return (...args) => {
return callWithRequest(request, ...args);
};
};There was a problem hiding this comment.
once was used in the original code to safeguard against multiple factory calls. now that the factory code for call with request and license checker is shared, you're right that it is no longer needed. I pushed an update to remove once from both factories
💚 Build Succeeded |
* Add `await` to common router request handler * Remove once() from common license checker and call with request factories
* Add `await` to common router request handler * Remove once() from common license checker and call with request factories
cjcenizal
added
the
non-issue
Change
requestHandlerwrapper toawaitthe real handler before returning it. Without this change, any errors coming back from ES results in a500status and code never reaches the error handling in thecatchblock.