500 error if workspace is invalid #429

hutiechuan · 2025-02-06T08:21:28Z

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test.
New functionality has user manual doc added.
Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Hailong-am · 2025-02-06T08:45:19Z

server/routes/summary_routes.ts

+      } catch (err) {
+        console.error('Error creating OpenSearch client:', err);
+
+        if (
+          err.statusCode === 403 &&
+          err.message?.includes('Saved object does not belong to the workspace')
+        ) {
+          return res.notFound({
+            body: 'Workspace/data source is invalid or not found!!',
+          });
+        }
+        if (
+          err.message?.includes('Data source not found') ||
+          err.message?.includes('Workspace not found') ||
+          err.message?.includes('Saved object does not belong to the workspace')
+        ) {
+          return res.notFound({
+            body: 'Workspace/data source is invalid or not found!!',
+          });
+        }
+
+        return handleError(err, res, context.assistant_plugin.logger);
+      }


should the whole error hanling be inside of getOpenSearchClientTransport method? It's been shared by other apis as well

yes, we could also do that.

Hailong-am · 2025-02-06T08:47:04Z

server/routes/summary_routes.ts

+
+        if (
+          err.statusCode === 403 &&
+          err.message?.includes('Saved object does not belong to the workspace')


can we have a better way instead of reply on message content?

Hailong-am · 2025-02-06T08:47:20Z

server/routes/summary_routes.ts

+          err.message?.includes('Data source not found') ||
+          err.message?.includes('Workspace not found') ||
+          err.message?.includes('Saved object does not belong to the workspace')


Hailong-am · 2025-02-06T08:47:51Z

server/routes/summary_routes.ts

+          err.message?.includes('Saved object does not belong to the workspace')
+        ) {
+          return res.notFound({
+            body: 'Workspace/data source is invalid or not found!!',


could you add i18n support for the error message?

Do we need i18n for server side error message?

xluo-aws · 2025-02-07T12:18:37Z

server/utils/get_opensearch_client_transport.ts

+    try {
+      return (await context.dataSource.opensearch.getClient(dataSourceId)).transport;
+    } catch (err) {
+      throw new DataSourceNotFoundError('Saved object does not belong to the workspace');


Shouldn't we check the error to decide whether to convert to DataSourceNotFoundError or not? What's the original err thrown if dataSourceId is not valid?

yes, error is: DataSourceError: Data Source Error: Saved object [data-source/3115e810-e39b-11ef-bf81-bf8337ab04fe] not found

@hutiechuan I think the point is we are not sure if the err here is about data source not found, it could be any unhandled exception, for example, network issue. So re-throw any error as DataSourceNotFoundError is inappropriate.
I'd suggest to not try/catch here, instead let handleError function to handle the error

export const handleError = (e: any, res: OpenSearchDashboardsResponseFactory, logger: Logger) => { logger.error('Error occurred', e); // Handle specific type of Errors if (e instanceof AgentNotFoundError) { return res.notFound({ body: 'Agent not found' }); } // handle http response error of calling backend API if (e.statusCode) { if (e.statusCode >= 400 && e.statusCode <= 499) { + let message = typeof e.body === 'string' ? e.body : JSON.stringify(e.body); + if (!message) { + message = e.message; + } return res.customError({ + body: { message: message || 'unknow error' }, statusCode: e.statusCode, }); } else { return res.customError({ statusCode: e.statusCode, }); } } // Return an general internalError for unhandled server-side issues return res.internalError(); };

but when i console the original err, it was throwing the message of "DataSourceError: Data Source Error: Saved object [data-source/3115e810-e39b-11ef-bf81-bf8337ab04fe] not found"

so instead of doing that, you don't want me to do that?

and if i dont try to catch the error of DataSourceNotFoundError, it wouldn't go to this condition if (e.statusCode >= 400 && e.statusCode <= 499) since the statusCode is 500

I saw statusCode is 403, did you try?

yes, i tried.
But on my side, the original error is 404

Original error: DataSourceError: Data Source Error: Saved object [data-source/3115e810-e39b-11ef-bf81-bf8337ab04fe] not found
at createDataSourceError (/Users/hutiech/Desktop/OpenSearch-Dashboards/src/plugins/data_source/server/lib/error.ts:13:12)
at configureClient (/Users/hutiech/Desktop/OpenSearch-Dashboards/src/plugins/data_source/server/client/configure_client.ts:105:32)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at getOpenSearchClientTransport (/Users/hutiech/Desktop/OpenSearch-Dashboards/plugins/dashboards-assistant/server/utils/get_opensearch_client_transport.ts:32:15)
at /Users/hutiech/Desktop/OpenSearch-Dashboards/plugins/dashboards-assistant/server/routes/summary_routes.ts:105:24
at /Users/hutiech/Desktop/OpenSearch-Dashboards/src/core/server/http/router/error_wrapper.ts:37:14
at Router.handle (/Users/hutiech/Desktop/OpenSearch-Dashboards/src/core/server/http/router/router.ts:286:44)
at handler (/Users/hutiech/Desktop/OpenSearch-Dashboards/src/core/server/http/router/router.ts:241:11)
at exports.Manager.execute (/Users/hutiech/Desktop/OpenSearch-Dashboards/node_modules/@hapi/hapi/lib/toolkit.js:60:28)
at Object.internals.handler (/Users/hutiech/Desktop/OpenSearch-Dashboards/node_modules/@hapi/hapi/lib/handler.js:46:20)
at exports.execute (/Users/hutiech/Desktop/OpenSearch-Dashboards/node_modules/@hapi/hapi/lib/handler.js:31:20)
at Request._lifecycle (/Users/hutiech/Desktop/OpenSearch-Dashboards/node_modules/@hapi/hapi/lib/request.js:371:32)
at Request._execute (/Users/hutiech/Desktop/OpenSearch-Dashboards/node_modules/@hapi/hapi/lib/request.js:281:9) {
statusCode: 404,
body: undefined
}

so if it's 404, it will be handled by if (e.statusCode >= 400 && e.statusCode <= 499)

ruanyl · 2025-02-08T01:15:49Z

server/routes/summary_routes.ts

+      let client;
+      try {
+        client = await getOpenSearchClientTransport({
+          context,
+          dataSourceId: req.query.dataSourceId,
+        });
+      } catch (err) {
+        if (err instanceof DataSourceNotFoundError) {
+          const msg = i18n.translate('assistant.server.error.workspaceDataSourceNotFound', {
+            defaultMessage: 'Workspace/data source is invalid or not found.',
+          });
+          return res.notFound({ body: msg });
+        }
+        return handleError(err, res, context.assistant_plugin.logger);
+      }


@hutiechuan there is a try/catch statement below, we can move const client = await getOpenSearchClientTransport(...) to there, so we can handle the error in a centralized place handleError()

ruanyl · 2025-02-08T07:45:38Z

yarn.lock

Could revert these changes? seems unrelated to this PR

which changes

The changes of this file (yarn.lock).

ruanyl · 2025-02-11T07:00:51Z

server/routes/error_handler.ts

+  // Handle specific type of Errors
+  if (e instanceof AgentNotFoundError) {
+    return res.notFound({ body: 'Agent not found' });
+  }


This is duplicated, let's remove it

ruanyl · 2025-02-11T07:04:17Z

yarn.lock

Unrelated file changes, please revert it.

codecov · 2025-02-11T08:59:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.00%. Comparing base (581a7ca) to head (151b08b).
Report is 21 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
- Coverage   87.46%   85.00%   -2.47%     
==========================================
  Files          65       72       +7     
  Lines        1891     2107     +216     
  Branches      473      520      +47     
==========================================
+ Hits         1654     1791     +137     
- Misses        236      314      +78     
- Partials        1        2       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ruanyl · 2025-02-11T09:47:28Z

server/routes/error_handler.ts

 import { Logger, OpenSearchDashboardsResponseFactory } from '../../../../src/core/server';
 import { AgentNotFoundError } from './errors';
+// import { DataSourceNotFoundError } from '../utils/get_opensearch_client_transport';


Let's remove the commented code

ruanyl · 2025-02-11T09:47:49Z

server/routes/summary_routes.ts

@@ -12,6 +13,8 @@ import { AssistantServiceSetup } from '../services/assistant_service';
 import { handleError } from './error_handler';
 import { AgentNotFoundError } from './errors';

+import { DataSourceNotFoundError } from '../utils/get_opensearch_client_transport';


This import never used, we can remove it.

ruanyl · 2025-02-11T09:48:16Z

server/utils/get_opensearch_client_transport.ts

+interface ErrorWithStatusCode extends Error {
+  statusCode?: number;
+}
+
+export class DataSourceNotFoundError extends Error {
+  public readonly statusCode: number;
+  constructor(message: string, originalError?: ErrorWithStatusCode) {
+    super(message);
+    this.name = 'DataSourceNotFoundError';
+    this.statusCode = originalError?.statusCode || 403;
+  }
+}
+


These changes are not going to be needed, we can remove them.

ruanyl · 2025-02-11T10:17:00Z

server/routes/error_handler.ts

@@ -3,6 +3,7 @@
 * SPDX-License-Identifier: Apache-2.0
 */

+import { i18n } from '@osd/i18n';


Suggested change

import { i18n } from '@osd/i18n';

ruanyl · 2025-02-12T05:28:00Z

@hutiechuan there are several checks failed, please add changelog and fix the DCO.

Signed-off-by: hutiechuan <[email protected]>

Signed-off-by: Tianyu Gao <[email protected]>

Signed-off-by: hutiechuan <[email protected]>

hutiechuan · 2025-02-12T09:30:43Z

@hutiechuan there are several checks failed, please add changelog and fix the DCO.

I added the changelog, and fix the DCO

hutiechuan requested review from lezzago, joshuali925, anirudha, ruanyl, SuZhou-Joe, xluo-aws, gaobinlong, Hailong-am, wanglam and raintygao as code owners February 6, 2025 08:21

hutiechuan force-pushed the 500Error branch 2 times, most recently from c25bec8 to 37eda3e Compare February 6, 2025 08:30

Hailong-am reviewed Feb 6, 2025

View reviewed changes

hutiechuan force-pushed the 500Error branch 2 times, most recently from 97139f9 to 3999e6f Compare February 6, 2025 09:28

xluo-aws reviewed Feb 7, 2025

View reviewed changes

ruanyl reviewed Feb 8, 2025

View reviewed changes

ruanyl reviewed Feb 11, 2025

View reviewed changes

ruanyl approved these changes Feb 11, 2025

View reviewed changes

ruanyl added the backport 2.x Trigger the backport flow to 2.x label Feb 11, 2025

hutiechuan force-pushed the 500Error branch 4 times, most recently from 217c847 to 7ea5e96 Compare February 12, 2025 08:57

hutiechuan and others added 2 commits February 12, 2025 17:04

Add feature changes for 500 Error Invalid Workspace

dda2bb2

Signed-off-by: hutiechuan <[email protected]>

doc: add release notes for 2.19 (opensearch-project#426)

2fc7558

Signed-off-by: Tianyu Gao <[email protected]>

hutiechuan added 10 commits February 12, 2025 17:04

Add i18n for error message

14623e8

Signed-off-by: hutiechuan <[email protected]>

no edit

faf776c

Signed-off-by: hutiechuan <[email protected]>

change error_handler

798a2af

Signed-off-by: hutiechuan <[email protected]>

revert yarn.lock and change error_handler

5e13e0e

Signed-off-by: hutiechuan <[email protected]>

revert yarn.lock and change error_handler

9a2258c

Signed-off-by: hutiechuan <[email protected]>

removing the customized error

624e655

Signed-off-by: hutiechuan <[email protected]>

removing unnecessary logs

fbb5c97

Signed-off-by: hutiechuan <[email protected]>

update yarn.lock fronm main branch and clean up my local branch

cbb131a

Signed-off-by: hutiechuan <[email protected]>

clean my current code

20cc01b

Signed-off-by: hutiechuan <[email protected]>

add changelog

3b332e3

Signed-off-by: hutiechuan <[email protected]>

hutiechuan force-pushed the 500Error branch from 7ea5e96 to 3b332e3 Compare February 12, 2025 09:05

500 error if workspace is invalid #429

Are you sure you want to change the base?

500 error if workspace is invalid #429

Conversation

hutiechuan commented Feb 6, 2025

Description

Issues Resolved

Check List

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Feb 8, 2025 • 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

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

codecov bot commented Feb 11, 2025 • edited Loading

Codecov Report

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

ruanyl commented Feb 12, 2025

hutiechuan commented Feb 12, 2025

ruanyl Feb 8, 2025 •

edited

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading