Update error handling for saved query service (#163904)

## Summary Resolves #153497. Updates the saved query service to properly handle & return errors from the saved object client. Instead of displaying "internal server error" and returning 500, specific error messages occur for corresponding saved object client errors. After: ![image](https://github.com/elastic/kibana/assets/1178348/f8ba7b90-77fe-4db9-8377-0a1f878fe3a0) ### To do - [x] API integration tests --------- Co-authored-by: Kibana Machine <[email protected]>
elastic · Aug 22, 2023 · 57b7efc · 57b7efc
1 parent 1caac7d
commit 57b7efc
Show file tree

Hide file tree

Showing 5 changed files with 184 additions and 14 deletions.
diff --git a/src/plugins/data/common/index.ts b/src/plugins/data/common/index.ts
@@ -13,6 +13,7 @@ export {
   DEFAULT_QUERY_LANGUAGE,
   KIBANA_USER_QUERY_LANGUAGE_KEY,
   KQL_TELEMETRY_ROUTE_LATEST_VERSION,
+  SAVED_QUERY_BASE_URL,
   SCRIPT_LANGUAGES_ROUTE_LATEST_VERSION,
   UI_SETTINGS,
 } from './constants';

diff --git a/src/plugins/data/server/query/routes.ts b/src/plugins/data/server/query/routes.ts
@@ -8,6 +8,7 @@
 
 import { schema } from '@kbn/config-schema';
 import { CoreSetup } from '@kbn/core/server';
+import { reportServerError } from '@kbn/kibana-utils-plugin/server';
 import { SavedQueryRouteHandlerContext } from './route_handler_context';
 import { SavedQueryRestResponse } from './route_types';
 import { SAVED_QUERY_BASE_URL } from '../../common/constants';
@@ -57,8 +58,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
         const body: SavedQueryRestResponse = await savedQuery.create(request.body);
         return response.ok({ body });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -85,8 +86,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
         const body: SavedQueryRestResponse = await savedQuery.update(id, request.body);
         return response.ok({ body });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -112,8 +113,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
         const body: SavedQueryRestResponse = await savedQuery.get(id);
         return response.ok({ body });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -136,8 +137,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
         const count: number = await savedQuery.count();
         return response.ok({ body: `${count}` });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -170,8 +171,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
           await savedQuery.find(request.body);
         return response.ok({ body });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -198,8 +199,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
           await savedQuery.getAll();
         return response.ok({ body });
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );
@@ -225,8 +226,8 @@ export function registerSavedQueryRoutes({ http }: CoreSetup): void {
         await savedQuery.delete(id);
         return response.ok();
       } catch (e) {
-        // TODO: Handle properly
-        return response.customError(e);
+        const err = e.output?.payload ?? e;
+        return reportServerError(response, err);
       }
     }
   );

diff --git a/test/api_integration/apis/index.ts b/test/api_integration/apis/index.ts
@@ -21,6 +21,7 @@ export default function ({ loadTestFile }: FtrProviderContext) {
     loadTestFile(require.resolve('./kql_telemetry'));
     loadTestFile(require.resolve('./saved_objects_management'));
     loadTestFile(require.resolve('./saved_objects'));
+    loadTestFile(require.resolve('./saved_queries'));
     loadTestFile(require.resolve('./scripts'));
     loadTestFile(require.resolve('./search'));
     loadTestFile(require.resolve('./short_url'));

diff --git a/test/api_integration/apis/saved_queries/index.js b/test/api_integration/apis/saved_queries/index.js
@@ -0,0 +1,13 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+export default function ({ loadTestFile }) {
+  describe('Saved queries', () => {
+    loadTestFile(require.resolve('./saved_queries'));
+  });
+}
diff --git a/test/api_integration/apis/saved_queries/saved_queries.js b/test/api_integration/apis/saved_queries/saved_queries.js
@@ -0,0 +1,154 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+import expect from '@kbn/expect';
+import { ELASTIC_HTTP_VERSION_HEADER } from '@kbn/core-http-common';
+import { SAVED_QUERY_BASE_URL } from '@kbn/data-plugin/common';
+
+// node scripts/functional_tests --config test/api_integration/config.js --grep="search session"
+
+const mockSavedQuery = {
+  title: 'my title',
+  description: 'my description',
+  query: {
+    query: 'foo: bar',
+    language: 'kql',
+  },
+  filters: [],
+};
+
+export default function ({ getService }) {
+  const esArchiver = getService('esArchiver');
+  const supertest = getService('supertest');
+  void SAVED_QUERY_BASE_URL;
+
+  describe('Saved queries API', function () {
+    before(async () => {
+      await esArchiver.emptyKibanaIndex();
+      await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');
+    });
+
+    after(async () => {
+      await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional');
+    });
+
+    it('should return 200 for create saved query', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_create`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send(mockSavedQuery)
+        .expect(200)
+        .then(({ body }) => {
+          expect(body.id).to.have.length(36);
+          expect(body.attributes.title).to.be('my title');
+          expect(body.attributes.description).to.be('my description');
+        }));
+
+    it('should return 400 for create invalid saved query', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_create`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send({ description: 'my description' })
+        .expect(400));
+
+    it('should return 200 for update saved query', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_create`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send(mockSavedQuery)
+        .expect(200)
+        .then(({ body }) =>
+          supertest
+            .put(`${SAVED_QUERY_BASE_URL}/${body.id}`)
+            .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+            .send({
+              ...mockSavedQuery,
+              title: 'my new title',
+            })
+            .expect(200)
+            .then((res) => {
+              expect(res.body.id).to.be(body.id);
+              expect(res.body.attributes.title).to.be('my new title');
+            })
+        ));
+
+    it('should return 404 for update non-existent saved query', () =>
+      supertest
+        .put(`${SAVED_QUERY_BASE_URL}/invalid_id`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send(mockSavedQuery)
+        .expect(404));
+
+    it('should return 200 for get saved query', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_create`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send(mockSavedQuery)
+        .expect(200)
+        .then(({ body }) =>
+          supertest
+            .get(`${SAVED_QUERY_BASE_URL}/${body.id}`)
+            .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+            .expect(200)
+            .then((res) => {
+              expect(res.body.id).to.be(body.id);
+              expect(res.body.attributes.title).to.be(body.attributes.title);
+            })
+        ));
+
+    it('should return 404 for get non-existent saved query', () =>
+      supertest
+        .get(`${SAVED_QUERY_BASE_URL}/invalid_id`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .expect(404));
+
+    it('should return 200 for saved query count', () =>
+      supertest
+        .get(`${SAVED_QUERY_BASE_URL}/_count`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .expect(200));
+
+    it('should return 200 for find saved queries', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_find`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send({})
+        .expect(200));
+
+    it('should return 400 for bad find saved queries request', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_find`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send({ foo: 'bar' })
+        .expect(400));
+
+    it('should return 200 for find all saved queries', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_all`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .expect(200));
+
+    it('should return 200 for delete saved query', () =>
+      supertest
+        .post(`${SAVED_QUERY_BASE_URL}/_create`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .send(mockSavedQuery)
+        .expect(200)
+        .then(({ body }) =>
+          supertest
+            .delete(`${SAVED_QUERY_BASE_URL}/${body.id}`)
+            .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+            .expect(200)
+        ));
+
+    it('should return 404 for get non-existent saved query', () =>
+      supertest
+        .delete(`${SAVED_QUERY_BASE_URL}/invalid_id`)
+        .set(ELASTIC_HTTP_VERSION_HEADER, '1')
+        .expect(404));
+  });
+}