Skip to content

Commit/optimize error feedback#580

Open
winfredLIN wants to merge 2 commits intomainfrom
commit/optimize-error-feedback
Open

Commit/optimize error feedback#580
winfredLIN wants to merge 2 commits intomainfrom
commit/optimize-error-feedback

Conversation

@winfredLIN
Copy link
Collaborator

关联的 issue

#579

描述你的变更

  1. 补充echo框架的HTTPErrorHandler,让中间件的错误信息能够传递到前端界面,而不是吞掉。
  2. 实现了工作台错误信息的国际化,提供用户友好的错误信息提示

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc

@actiontech-bot actiontech-bot requested a review from iwanghc March 11, 2026 09:52
@github-actions
Copy link

PR Reviewer Guide 🔍

🎫 Ticket compliance analysis 🔶

579 - Partially compliant

Compliant requirements:

  • 错误信息正确传递
  • 工作台错误信息国际化

Non-compliant requirements:

无不符合项

Requires further human verification:

无需进一步人工验证

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

错误处理

新增的 HTTPErrorHandler 固定返回 HTTP 400 状态码,建议检查是否需要针对不同错误类型返回不同状态码,并确保错误信息能准确传递给前端,进一步验证修改是否覆盖所有预期场景。

e.HTTPErrorHandler = func(err error, c echo.Context) {
	if c.Response().Committed {
		return
	}

	message := err.Error()
	if httpErr, ok := err.(*echo.HTTPError); ok {
		if msg, ok := httpErr.Message.(string); ok && msg != "" {
			message = msg
		}
	}

	_ = c.JSON(http.StatusBadRequest, bV1.GenericResp{
		Code:    int(apiError.BadRequestErr),
		Message: message,
	})
}
日志与错误上下文

在 AuditMiddleware 中用 errors.New() 替换 fmt.Errorf() 后,原始的错误上下文可能丢失,建议考虑同时记录详细的错误信息或者使用错误包装技术,以便日后排查问题。

bodyBytes, err := io.ReadAll(c.Request().Body)
if err != nil {
	sqlWorkbenchService.log.Errorf("failed to read request body: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditReadReqBodyErr))
}
// 恢复请求体,供后续处理使用
c.Request().Body = io.NopCloser(bytes.NewBuffer(bodyBytes))

// 解析请求体获取 SQL 和 datasource ID
sql, datasourceID, err := sqlWorkbenchService.parseStreamExecuteRequest(bodyBytes)
if err != nil {
	sqlWorkbenchService.log.Errorf("failed to parse streamExecute request, skipping audit: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditParseReqErr))
}

if sql == "" || datasourceID == "" {
	sqlWorkbenchService.log.Debugf("SQL or datasource ID is empty, skipping audit")
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditMissingSQLOrDatasourceErr))
}

// 获取当前用户 ID
dmsUserId, err := sqlWorkbenchService.getDMSUserIdFromRequest(c)
if err != nil {
	sqlWorkbenchService.log.Errorf("failed to get DMS user ID: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditGetDMSUserErr))
}

// 从缓存表获取 dms_db_service_id
dmsDBServiceID, err := sqlWorkbenchService.getDMSDBServiceIDFromCache(c.Request().Context(), datasourceID, dmsUserId)
if err != nil {
	sqlWorkbenchService.log.Errorf("failed to get dms_db_service_id from cache: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditGetDBServiceMappingErr))
}

if dmsDBServiceID == "" {
	sqlWorkbenchService.log.Debugf("dms_db_service_id not found in cache for datasource: %s", datasourceID)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditDBServiceMappingNotFoundErr))
}

// 获取 DBService 信息
dbService, err := sqlWorkbenchService.dbServiceUsecase.GetDBService(c.Request().Context(), dmsDBServiceID)
if err != nil {
	sqlWorkbenchService.log.Errorf("failed to get DBService: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditGetDBServiceErr))
}

// 检查是否启用 SQL 审核
if !sqlWorkbenchService.isEnableSQLAudit(dbService) {
	sqlWorkbenchService.log.Debugf("SQL audit is not enabled for DBService: %s", dmsDBServiceID)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditNotEnabledErr))
}

// 调用 SQLE 审核接口
auditResult, err := sqlWorkbenchService.callSQLEAudit(c.Request().Context(), sql, dbService)
if err != nil {
	sqlWorkbenchService.log.Errorf("call SQLE audit failed: %v", err)
	return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditCallSQLEErr))

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
根据错误类型返回状态码

*建议在错误处理函数中,当错误为 echo.HTTPError 类型时,根据其 Code 字段返回相应的 HTTP 状态码,而不是统一返回
http.StatusBadRequest。这样可以保证客户端接收到更加精确的错误状态,有助于更有效地定位问题和调试。

internal/apiserver/service/service.go [55-71]

 e.HTTPErrorHandler = func(err error, c echo.Context) {
 		if c.Response().Committed {
 			return
 		}
-
+		code := http.StatusBadRequest
 		message := err.Error()
 		if httpErr, ok := err.(*echo.HTTPError); ok {
 			if msg, ok := httpErr.Message.(string); ok && msg != "" {
 				message = msg
 			}
+			code = httpErr.Code
 		}
-
-		_ = c.JSON(http.StatusBadRequest, bV1.GenericResp{
+		_ = c.JSON(code, bV1.GenericResp{
 			Code:    int(apiError.BadRequestErr),
 			Message: message,
 		})
 	}
Suggestion importance[1-10]: 6

__

Why: The suggestion enhances error handling by returning the HTTP status code from httpErr instead of always using http.StatusBadRequest. This improvement is useful but only moderately impacts the overall functionality.

Low
General
保留错误上下文信息


建议在返回错误给客户端时,同时包含原始错误的上下文信息用于日志调试,这样可以帮助开发人员准确定位问题原因。可以在不暴露敏感信息的前提下,将原始错误与友好提示组合返回,同时仍记录详细错误日志。

internal/sql_workbench/service/sql_workbench_service.go [1023-1025]

 if err != nil {
 				sqlWorkbenchService.log.Errorf("failed to read request body: %v", err)
-				return errors.New(locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditReadReqBodyErr))
+				localizedMsg := locale.Bundle.LocalizeMsgByCtx(c.Request().Context(), locale.SqlWorkbenchAuditReadReqBodyErr)
+				return errors.New(fmt.Sprintf("%s: %v", localizedMsg, err))
 			}
Suggestion importance[1-10]: 4

__

Why: While including the original error context could improve debugging, appending the internal error details to the client-facing message might expose sensitive information, reducing its overall benefit.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant