From 1064ddc80ca98ee4d21059f47a6f1d2d556d30ed Mon Sep 17 00:00:00 2001 From: Alexey Efimov Date: Thu, 4 Jul 2024 06:09:35 +0200 Subject: [PATCH] change error format for http handlers (#6248) --- ydb/core/viewer/json_local_rpc.h | 12 ++++++--- ydb/core/viewer/json_query.h | 46 +++++++++++--------------------- ydb/core/viewer/viewer.cpp | 46 ++++++++++++++++++++++++++++++-- ydb/core/viewer/viewer.h | 5 +++- 4 files changed, 72 insertions(+), 37 deletions(-) diff --git a/ydb/core/viewer/json_local_rpc.h b/ydb/core/viewer/json_local_rpc.h index b1fb14b0d3aa..00cd683cbb72 100644 --- a/ydb/core/viewer/json_local_rpc.h +++ b/ydb/core/viewer/json_local_rpc.h @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -206,12 +207,15 @@ class TJsonLocalRpc : public TActorBootstrappedStatus) { if (!Result->Status->IsSuccess()) { + NJson::TJsonValue json; + TString message; + MakeErrorReply(json, message, Result->Status.value()); + TStringStream stream; + NJson::WriteJson(&stream, &json); if (Result->Status->GetStatus() == NYdb::EStatus::UNAUTHORIZED) { - return ReplyAndPassAway(Viewer->GetHTTPFORBIDDEN(Event->Get())); + return ReplyAndPassAway(Viewer->GetHTTPFORBIDDEN(Event->Get(), "application/json", stream.Str())); } else { - TStringStream error; - Result->Status->Out(error); - return ReplyAndPassAway(Viewer->GetHTTPBADREQUEST(Event->Get(), "text/plain", error.Str())); + return ReplyAndPassAway(Viewer->GetHTTPBADREQUEST(Event->Get(), "application/json", stream.Str())); } } else { TStringStream json; diff --git a/ydb/core/viewer/json_query.h b/ydb/core/viewer/json_query.h index ce2b19f7caea..0378f12d516e 100644 --- a/ydb/core/viewer/json_query.h +++ b/ydb/core/viewer/json_query.h @@ -398,13 +398,14 @@ class TJsonQuery : public TViewerPipeClient { if (ev->Get()->Record.GetRef().GetYdbStatus() == Ydb::StatusIds::SUCCESS) { QueryResponse.Set(std::move(ev)); MakeOkReply(jsonResponse, QueryResponse->Record.GetRef()); + if (Schema == ESchemaType::Classic && Stats.empty() && (Action.empty() || Action == "execute")) { + jsonResponse = std::move(jsonResponse["result"]); + } } else { QueryResponse.Error("QueryError"); - MakeErrorReply(jsonResponse, ev->Get()->Record.GetRef().MutableResponse()->MutableQueryIssues()); - } - - if (Schema == ESchemaType::Classic && Stats.empty() && (Action.empty() || Action == "execute")) { - jsonResponse = std::move(jsonResponse["result"]); + NYql::TIssues issues; + NYql::IssuesFromMessage(ev->Get()->Record.GetRef().GetResponse().GetQueryIssues(), issues); + MakeErrorReply(jsonResponse, NYdb::TStatus(NYdb::EStatus(ev->Get()->Record.GetRef().GetYdbStatus()), std::move(issues))); } TStringStream stream; @@ -421,7 +422,9 @@ class TJsonQuery : public TViewerPipeClient { auto& record(ev->Get()->Record); NJson::TJsonValue jsonResponse; if (record.IssuesSize() > 0) { - MakeErrorReply(jsonResponse, record.MutableIssues()); + NYql::TIssues issues; + NYql::IssuesFromMessage(record.GetIssues(), issues); + MakeErrorReply(jsonResponse, NYdb::TStatus(NYdb::EStatus(record.GetStatusCode()), std::move(issues))); } TStringStream stream; @@ -472,26 +475,11 @@ class TJsonQuery : public TViewerPipeClient { } private: - void MakeErrorReply(NJson::TJsonValue& jsonResponse, google::protobuf::RepeatedPtrField* protoIssues) { - NJson::TJsonValue& jsonIssues = jsonResponse["issues"]; - - // find first deepest error - std::stable_sort(protoIssues->begin(), protoIssues->end(), [](const Ydb::Issue::IssueMessage& a, const Ydb::Issue::IssueMessage& b) -> bool { - return a.severity() < b.severity(); - }); - while (protoIssues->size() > 0 && (*protoIssues)[0].issuesSize() > 0) { - protoIssues = (*protoIssues)[0].mutable_issues(); - } + void MakeErrorReply(NJson::TJsonValue& jsonResponse, const NYdb::TStatus& status) { TString message; - if (protoIssues->size() > 0) { - const Ydb::Issue::IssueMessage& issue = (*protoIssues)[0]; - NProtobufJson::Proto2Json(issue, jsonResponse["error"]); - message = issue.message(); - } - for (const auto& queryIssue : *protoIssues) { - NJson::TJsonValue& issue = jsonIssues.AppendValue({}); - NProtobufJson::Proto2Json(queryIssue, issue); - } + + NViewer::MakeErrorReply(jsonResponse, message, status); + if (Span) { Span.EndError("Error"); } @@ -513,11 +501,9 @@ class TJsonQuery : public TViewerPipeClient { } } catch (const std::exception& ex) { - google::protobuf::RepeatedPtrField protoIssues; - Ydb::Issue::IssueMessage* issue = protoIssues.Add(); - issue->set_message(TStringBuilder() << "Convert error: " << ex.what()); - issue->set_severity(NYql::TSeverityIds::S_ERROR); - MakeErrorReply(jsonResponse, &protoIssues); + NYql::TIssues issues; + issues.AddIssue(TStringBuilder() << "Convert error: " << ex.what()); + MakeErrorReply(jsonResponse, NYdb::TStatus(NYdb::EStatus::BAD_REQUEST, std::move(issues))); return; } } diff --git a/ydb/core/viewer/viewer.cpp b/ydb/core/viewer/viewer.cpp index fa49cbf08fd9..4ae033dcfda1 100644 --- a/ydb/core/viewer/viewer.cpp +++ b/ydb/core/viewer/viewer.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include "viewer.h" @@ -211,7 +212,7 @@ class TViewer : public TActorBootstrapped, public IViewer { TString GetHTTPOK(const TRequestState& request, TString type, TString response, TInstant lastModified) override; TString GetHTTPGATEWAYTIMEOUT(const TRequestState& request, TString type, TString response) override; TString GetHTTPBADREQUEST(const TRequestState& request, TString type, TString response) override; - TString GetHTTPFORBIDDEN(const TRequestState& request) override; + TString GetHTTPFORBIDDEN(const TRequestState& request, TString type, TString response) override; TString GetHTTPNOTFOUND(const TRequestState& request) override; TString GetHTTPINTERNALERROR(const TRequestState& request, TString contentType = {}, TString response = {}) override; TString GetHTTPFORWARD(const TRequestState& request, const TString& location) override; @@ -698,13 +699,19 @@ TString TViewer::GetHTTPBADREQUEST(const TRequestState& request, TString content return res; } -TString TViewer::GetHTTPFORBIDDEN(const TRequestState& request) { +TString TViewer::GetHTTPFORBIDDEN(const TRequestState& request, TString contentType, TString response) { TStringBuilder res; res << "HTTP/1.1 403 Forbidden\r\n" << "Connection: Close\r\n"; + if (contentType) { + res << "Content-Type: " << contentType << "\r\n"; + } FillCORS(res, request); FillTraceId(res, request); res << "\r\n"; + if (response) { + res << response; + } return res; } @@ -767,6 +774,41 @@ TString TViewer::GetHTTPFORWARD(const TRequestState& request, const TString& loc return res; } +void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status) { + google::protobuf::RepeatedPtrField protoIssues; + NYql::IssuesToMessage(status.GetIssues(), &protoIssues); + + message.clear(); + // find first deepest error + std::stable_sort(protoIssues.begin(), protoIssues.end(), [](const Ydb::Issue::IssueMessage& a, const Ydb::Issue::IssueMessage& b) -> bool { + return a.severity() < b.severity(); + }); + + while (protoIssues.size() > 0 && protoIssues[0].issuesSize() > 0) { + protoIssues = protoIssues[0].issues(); + } + + if (protoIssues.size() > 0) { + const Ydb::Issue::IssueMessage& issue = protoIssues[0]; + NProtobufJson::Proto2Json(issue, jsonResponse["error"]); + message = issue.message(); + } + + NJson::TJsonValue& jsonIssues = jsonResponse["issues"]; + for (const auto& queryIssue : protoIssues) { + NJson::TJsonValue& issue = jsonIssues.AppendValue({}); + NProtobufJson::Proto2Json(queryIssue, issue); + } + + TString textStatus = TStringBuilder() << status.GetStatus(); + + jsonResponse["status"] = textStatus; + + if (message.empty()) { + message = textStatus; + } +} + NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state) { NKikimrViewer::EFlag flag = NKikimrViewer::EFlag::Grey; switch (state) { diff --git a/ydb/core/viewer/viewer.h b/ydb/core/viewer/viewer.h index d985e44f921c..912de93d668d 100644 --- a/ydb/core/viewer/viewer.h +++ b/ydb/core/viewer/viewer.h @@ -11,6 +11,7 @@ #include #include #include +#include #include namespace NKikimr { @@ -194,7 +195,7 @@ class IViewer { virtual TString GetHTTPGATEWAYTIMEOUT(const TRequestState& request, TString contentType = {}, TString response = {}) = 0; virtual TString GetHTTPBADREQUEST(const TRequestState& request, TString contentType = {}, TString response = {}) = 0; - virtual TString GetHTTPFORBIDDEN(const TRequestState& request) = 0; + virtual TString GetHTTPFORBIDDEN(const TRequestState& request, TString contentType = {}, TString response = {}) = 0; virtual TString GetHTTPNOTFOUND(const TRequestState& request) = 0; virtual TString GetHTTPINTERNALERROR(const TRequestState& request, TString contentType = {}, TString response = {}) = 0; virtual TString GetHTTPFORWARD(const TRequestState& request, const TString& location) = 0; @@ -260,6 +261,8 @@ void SplitIds(TStringBuf source, char delim, std::unordered_set& valu GenericSplitIds(source, delim, std::inserter(values, values.end())); } +void MakeErrorReply(NJson::TJsonValue& jsonResponse, TString& message, const NYdb::TStatus& status); + TString GetHTTPOKJSON(); TString GetHTTPGATEWAYTIMEOUT(); NKikimrViewer::EFlag GetFlagFromTabletState(NKikimrWhiteboard::TTabletStateInfo::ETabletState state);