Fix connector router response 500 (#358) (#359)

* Response 400 instead of 500 with error message Signed-off-by: Lin Wang <[email protected]> * Add UT for 400 error in connector APIs Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> (cherry picked from commit 17e685d) Co-authored-by: Lin Wang <[email protected]>
opensearch-project · Sep 4, 2024 · 95ddeea · 95ddeea
1 parent e39ff32
commit 95ddeea
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 25 deletions.
diff --git a/server/routes/__tests__/connector_router.test.ts b/server/routes/__tests__/connector_router.test.ts
@@ -6,20 +6,26 @@
 import { ResponseObject } from '@hapi/hapi';
 
 import { Router } from '../../../../../src/core/server/http/router';
+import { OpenSearchClient } from '../../../../../src/core/server';
 import { triggerHandler, createDataSourceEnhancedRouter } from '../router.mock';
 import { httpServerMock } from '../../../../../src/core/server/http/http_server.mocks';
 import { loggerMock } from '../../../../../src/core/server/logging/logger.mock';
 import { connectorRouter } from '../connector_router';
 import { CONNECTOR_API_ENDPOINT, INTERNAL_CONNECTOR_API_ENDPOINT } from '../constants';
 import { ConnectorService } from '../../services/connector_service';
+import { Boom } from '@hapi/boom';
 
-const setupRouter = () => {
+const setupRouter = ({
+  getClient,
+}: {
+  getClient?: (dataSourceId: string) => Promise<Pick<OpenSearchClient, 'transport'>>;
+} = {}) => {
   const mockedLogger = loggerMock.create();
   const {
     router,
     dataSourceTransportMock,
     getLatestCurrentUserTransport,
-  } = createDataSourceEnhancedRouter(mockedLogger);
+  } = createDataSourceEnhancedRouter(mockedLogger, getClient);
 
   connectorRouter(router);
   return {
@@ -85,6 +91,20 @@ describe('connector routers', () => {
         size: 10000,
       });
     });
+
+    it('should throw 400 error when failed to get data source', async () => {
+      const getClientMock = jest.fn().mockRejectedValue(new Error('Unknown error'));
+      const { router } = setupRouter({ getClient: getClientMock });
+
+      const result = (await triggerGetAllConnectors(router, 'foo')) as Boom;
+      expect(result.output.payload).toMatchInlineSnapshot(`
+        Object {
+          "error": "Bad Request",
+          "message": "Unknown error",
+          "statusCode": 400,
+        }
+      `);
+    });
   });
 
   describe('get all internal connector', () => {
@@ -113,5 +133,19 @@ describe('connector routers', () => {
         size: 10000,
       });
     });
+
+    it('should throw 400 error when failed to get data source', async () => {
+      const getClientMock = jest.fn().mockRejectedValue(new Error('Unknown error'));
+      const { router } = setupRouter({ getClient: getClientMock });
+
+      const result = (await triggerGetAllInternalConnectors(router, 'foo')) as Boom;
+      expect(result.output.payload).toMatchInlineSnapshot(`
+        Object {
+          "error": "Bad Request",
+          "message": "Unknown error",
+          "statusCode": 400,
+        }
+      `);
+    });
   });
 });
diff --git a/server/routes/connector_router.ts b/server/routes/connector_router.ts
@@ -19,17 +19,21 @@ export const connectorRouter = (router: IRouter) => {
         }),
       },
     },
-    router.handleLegacyErrors(async (context, request, res) => {
-      const payload = await ConnectorService.search({
-        transport: await getOpenSearchClientTransport({
-          dataSourceId: request.query.data_source_id,
-          context,
-        }),
-        from: 0,
-        size: 10000,
-      });
-      return res.ok({ body: payload });
-    })
+    async (context, request, res) => {
+      try {
+        const payload = await ConnectorService.search({
+          transport: await getOpenSearchClientTransport({
+            dataSourceId: request.query.data_source_id,
+            context,
+          }),
+          from: 0,
+          size: 10000,
+        });
+        return res.ok({ body: payload });
+      } catch (err) {
+        return res.badRequest({ body: err.message });
+      }
+    }
   );
   router.get(
     {
@@ -40,15 +44,19 @@ export const connectorRouter = (router: IRouter) => {
         }),
       },
     },
-    router.handleLegacyErrors(async (context, request, res) => {
-      const data = await ConnectorService.getUniqueInternalConnectorNames({
-        transport: await getOpenSearchClientTransport({
-          dataSourceId: request.query.data_source_id,
-          context,
-        }),
-        size: 10000,
-      });
-      return res.ok({ body: { data } });
-    })
+    async (context, request, res) => {
+      try {
+        const data = await ConnectorService.getUniqueInternalConnectorNames({
+          transport: await getOpenSearchClientTransport({
+            dataSourceId: request.query.data_source_id,
+            context,
+          }),
+          size: 10000,
+        });
+        return res.ok({ body: { data } });
+      } catch (err) {
+        return res.badRequest({ body: err.message });
+      }
+    }
   );
 };
diff --git a/server/routes/router.mock.ts b/server/routes/router.mock.ts
@@ -126,7 +126,10 @@ export const triggerHandler = async (
   return await findRoute?.handler(options.req, new MockResponseToolkit());
 };
 
-export const createDataSourceEnhancedRouter = (logger: Logger) => {
+export const createDataSourceEnhancedRouter = (
+  logger: Logger,
+  getClient?: (dataSourceId: string) => Promise<Pick<OpenSearchClient, 'transport'>>
+) => {
   const dataSourceTransportMock = {};
   let latestCurrentUserTransport: OpenSearchClient['transport'];
   const router = new Router('', logger, (fn) => (req, res) => {
@@ -138,7 +141,9 @@ export const createDataSourceEnhancedRouter = (logger: Logger) => {
         core,
         dataSource: {
           opensearch: {
-            getClient: async (_dataSourceId: string) => ({ transport: dataSourceTransportMock }),
+            getClient:
+              getClient ??
+              (async (_dataSourceId: string) => ({ transport: dataSourceTransportMock })),
           },
         },
       },