diff --git a/src/iceberg/catalog/rest/error_handlers.cc b/src/iceberg/catalog/rest/error_handlers.cc index 8465c0018..f3e5b8fb3 100644 --- a/src/iceberg/catalog/rest/error_handlers.cc +++ b/src/iceberg/catalog/rest/error_handlers.cc @@ -38,7 +38,7 @@ const std::shared_ptr& DefaultErrorHandler::Instance() { return instance; } -Status DefaultErrorHandler::Accept(const ErrorModel& error) const { +Status DefaultErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 400: if (error.type == kIllegalArgumentException) { @@ -69,7 +69,7 @@ const std::shared_ptr& NamespaceErrorHandler::Instance() return instance; } -Status NamespaceErrorHandler::Accept(const ErrorModel& error) const { +Status NamespaceErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 400: if (error.type == kNamespaceNotEmptyException) { @@ -93,7 +93,7 @@ const std::shared_ptr& DropNamespaceErrorHandler::Ins return instance; } -Status DropNamespaceErrorHandler::Accept(const ErrorModel& error) const { +Status DropNamespaceErrorHandler::Accept(const ErrorResponse& error) const { if (error.code == 409) { return NamespaceNotEmpty(error.message); } @@ -106,7 +106,7 @@ const std::shared_ptr& TableErrorHandler::Instance() { return instance; } -Status TableErrorHandler::Accept(const ErrorModel& error) const { +Status TableErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 404: if (error.type == kNoSuchNamespaceException) { @@ -125,7 +125,7 @@ const std::shared_ptr& ViewErrorHandler::Instance() { return instance; } -Status ViewErrorHandler::Accept(const ErrorModel& error) const { +Status ViewErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 404: if (error.type == kNoSuchNamespaceException) { @@ -145,7 +145,7 @@ const std::shared_ptr& TableCommitErrorHandler::Instanc return instance; } -Status TableCommitErrorHandler::Accept(const ErrorModel& error) const { +Status TableCommitErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 404: return NoSuchTable(error.message); @@ -167,7 +167,7 @@ const std::shared_ptr& ViewCommitErrorHandler::Instance( return instance; } -Status ViewCommitErrorHandler::Accept(const ErrorModel& error) const { +Status ViewCommitErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 404: return NoSuchView(error.message); diff --git a/src/iceberg/catalog/rest/error_handlers.h b/src/iceberg/catalog/rest/error_handlers.h index 42a0d8c8a..eae2c9b7f 100644 --- a/src/iceberg/catalog/rest/error_handlers.h +++ b/src/iceberg/catalog/rest/error_handlers.h @@ -36,14 +36,11 @@ class ICEBERG_REST_EXPORT ErrorHandler { public: virtual ~ErrorHandler() = default; - // TODO(Li Feiyang):removing ErrorModel as the inner layer and directly using - // ErrorResponse - /// \brief Process an error response and return an appropriate Error. /// - /// \param error The error model parsed from the HTTP response body + /// \param error The error response parsed from the HTTP response body /// \return An Error object with appropriate ErrorKind and message - virtual Status Accept(const ErrorModel& error) const = 0; + virtual Status Accept(const ErrorResponse& error) const = 0; }; /// \brief Default error handler for REST API responses. @@ -52,7 +49,7 @@ class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler { /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; protected: constexpr DefaultErrorHandler() = default; @@ -64,7 +61,7 @@ class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler { /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; protected: constexpr NamespaceErrorHandler() = default; @@ -76,7 +73,7 @@ class ICEBERG_REST_EXPORT DropNamespaceErrorHandler final : public NamespaceErro /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; private: constexpr DropNamespaceErrorHandler() = default; @@ -88,7 +85,7 @@ class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler { /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; private: constexpr TableErrorHandler() = default; @@ -100,7 +97,7 @@ class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler { /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; private: constexpr ViewErrorHandler() = default; @@ -112,7 +109,7 @@ class ICEBERG_REST_EXPORT TableCommitErrorHandler final : public DefaultErrorHan /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; private: constexpr TableCommitErrorHandler() = default; @@ -124,7 +121,7 @@ class ICEBERG_REST_EXPORT ViewCommitErrorHandler final : public DefaultErrorHand /// \brief Returns the singleton instance static const std::shared_ptr& Instance(); - Status Accept(const ErrorModel& error) const override; + Status Accept(const ErrorResponse& error) const override; private: constexpr ViewCommitErrorHandler() = default; diff --git a/src/iceberg/catalog/rest/http_client.cc b/src/iceberg/catalog/rest/http_client.cc index 1b026c66e..1e4fc1ee4 100644 --- a/src/iceberg/catalog/rest/http_client.cc +++ b/src/iceberg/catalog/rest/http_client.cc @@ -103,7 +103,7 @@ Status HandleFailureResponse(const cpr::Response& response, // TODO(gangwu): response status code is lost, wrap it with RestError. ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text)); ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json)); - return error_handler.Accept(error_response.error); + return error_handler.Accept(error_response); } return {}; } diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index bdec822cc..61fce9381 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -91,43 +91,34 @@ Result CatalogConfigFromJson(const nlohmann::json& json) { return config; } -nlohmann::json ToJson(const ErrorModel& error) { +nlohmann::json ToJson(const ErrorResponse& error) { + nlohmann::json error_json; + error_json[kMessage] = error.message; + error_json[kType] = error.type; + error_json[kCode] = error.code; + SetContainerField(error_json, kStack, error.stack); + nlohmann::json json; - json[kMessage] = error.message; - json[kType] = error.type; - json[kCode] = error.code; - SetContainerField(json, kStack, error.stack); + json[kError] = std::move(error_json); return json; } -Result ErrorModelFromJson(const nlohmann::json& json) { - ErrorModel error; +Result ErrorResponseFromJson(const nlohmann::json& json) { + ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue(json, kError)); + + ErrorResponse error; // NOTE: Iceberg's Java implementation allows missing required fields (message, type, // code) during deserialization, which deviates from the REST spec. We enforce strict // validation here. - ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue(json, kMessage)); - ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue(json, kType)); - ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue(json, kCode)); - ICEBERG_ASSIGN_OR_RAISE(error.stack, - GetJsonValueOrDefault>(json, kStack)); + ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue(error_json, kMessage)); + ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue(error_json, kType)); + ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue(error_json, kCode)); + ICEBERG_ASSIGN_OR_RAISE( + error.stack, GetJsonValueOrDefault>(error_json, kStack)); ICEBERG_RETURN_UNEXPECTED(error.Validate()); return error; } -nlohmann::json ToJson(const ErrorResponse& response) { - nlohmann::json json; - json[kError] = ToJson(response.error); - return json; -} - -Result ErrorResponseFromJson(const nlohmann::json& json) { - ErrorResponse response; - ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue(json, kError)); - ICEBERG_ASSIGN_OR_RAISE(response.error, ErrorModelFromJson(error_json)); - ICEBERG_RETURN_UNEXPECTED(response.Validate()); - return response; -} - nlohmann::json ToJson(const CreateNamespaceRequest& request) { nlohmann::json json; json[kNamespace] = request.namespace_.levels; @@ -339,7 +330,6 @@ Result ListTablesResponseFromJson(const nlohmann::json& json } ICEBERG_DEFINE_FROM_JSON(CatalogConfig) -ICEBERG_DEFINE_FROM_JSON(ErrorModel) ICEBERG_DEFINE_FROM_JSON(ErrorResponse) ICEBERG_DEFINE_FROM_JSON(ListNamespacesResponse) ICEBERG_DEFINE_FROM_JSON(CreateNamespaceRequest) diff --git a/src/iceberg/catalog/rest/json_internal.h b/src/iceberg/catalog/rest/json_internal.h index 986066f1a..ba6859221 100644 --- a/src/iceberg/catalog/rest/json_internal.h +++ b/src/iceberg/catalog/rest/json_internal.h @@ -44,7 +44,6 @@ Result FromJson(const nlohmann::json& json); /// \note Don't forget to add `ICEBERG_DEFINE_FROM_JSON` to the end of /// `json_internal.cc` to define the `FromJson` function for the model. ICEBERG_DECLARE_JSON_SERDE(CatalogConfig) -ICEBERG_DECLARE_JSON_SERDE(ErrorModel) ICEBERG_DECLARE_JSON_SERDE(ErrorResponse) ICEBERG_DECLARE_JSON_SERDE(ListNamespacesResponse) ICEBERG_DECLARE_JSON_SERDE(CreateNamespaceRequest) diff --git a/src/iceberg/catalog/rest/type_fwd.h b/src/iceberg/catalog/rest/type_fwd.h index 082630f2d..f63984b31 100644 --- a/src/iceberg/catalog/rest/type_fwd.h +++ b/src/iceberg/catalog/rest/type_fwd.h @@ -24,7 +24,7 @@ namespace iceberg::rest { -struct ErrorModel; +struct ErrorResponse; class ErrorHandler; class HttpClient; diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 0b589bdfd..dbc772ec4 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -52,20 +52,20 @@ struct ICEBERG_REST_EXPORT CatalogConfig { }; /// \brief JSON error payload returned in a response with further details on the error. -struct ICEBERG_REST_EXPORT ErrorModel { +struct ICEBERG_REST_EXPORT ErrorResponse { std::string message; // required std::string type; // required uint32_t code; // required std::vector stack; - /// \brief Validates the ErrorModel. + /// \brief Validates the ErrorResponse. Status Validate() const { if (message.empty() || type.empty()) { - return Invalid("Invalid error model: missing required fields"); + return Invalid("Invalid error response: missing required fields"); } if (code < 400 || code > 600) { - return Invalid("Invalid error model: code {} is out of range [400, 600]", code); + return Invalid("Invalid error response: code {} is out of range [400, 600]", code); } // stack is optional, no validation needed @@ -73,16 +73,6 @@ struct ICEBERG_REST_EXPORT ErrorModel { } }; -/// \brief Error response body returned in a response. -struct ICEBERG_REST_EXPORT ErrorResponse { - ErrorModel error; // required - - /// \brief Validates the ErrorResponse. - // We don't validate the error field because ErrorModel::Validate has been called in the - // FromJson. - Status Validate() const { return {}; } -}; - /// \brief Request to create a namespace. struct ICEBERG_REST_EXPORT CreateNamespaceRequest { Namespace namespace_; // required diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index eba669add..2cae201fc 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -96,15 +96,11 @@ bool operator==(const CatalogConfig& lhs, const CatalogConfig& rhs) { lhs.endpoints == rhs.endpoints; } -bool operator==(const ErrorModel& lhs, const ErrorModel& rhs) { +bool operator==(const ErrorResponse& lhs, const ErrorResponse& rhs) { return lhs.message == rhs.message && lhs.type == rhs.type && lhs.code == rhs.code && lhs.stack == rhs.stack; } -bool operator==(const ErrorResponse& lhs, const ErrorResponse& rhs) { - return lhs.error == rhs.error; -} - // Test parameter structure for roundtrip tests template struct JsonRoundTripParam { @@ -874,27 +870,27 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "WithoutStack", .expected_json_str = R"({"error":{"message":"The given namespace does not exist","type":"NoSuchNamespaceException","code":404}})", - .model = {.error = {.message = "The given namespace does not exist", - .type = "NoSuchNamespaceException", - .code = 404}}}, + .model = {.message = "The given namespace does not exist", + .type = "NoSuchNamespaceException", + .code = 404}}, // Error with stack trace ErrorResponseParam{ .test_name = "WithStack", .expected_json_str = R"({"error":{"message":"The given namespace does not exist","type":"NoSuchNamespaceException","code":404,"stack":["a","b"]}})", - .model = {.error = {.message = "The given namespace does not exist", - .type = "NoSuchNamespaceException", - .code = 404, - .stack = {"a", "b"}}}}, + .model = {.message = "The given namespace does not exist", + .type = "NoSuchNamespaceException", + .code = 404, + .stack = {"a", "b"}}}, // Different error type ErrorResponseParam{ .test_name = "DifferentError", .expected_json_str = R"({"error":{"message":"Internal server error","type":"InternalServerError","code":500,"stack":["line1","line2","line3"]}})", - .model = {.error = {.message = "Internal server error", - .type = "InternalServerError", - .code = 500, - .stack = {"line1", "line2", "line3"}}}}), + .model = {.message = "Internal server error", + .type = "InternalServerError", + .code = 500, + .stack = {"line1", "line2", "line3"}}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -909,17 +905,17 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "NullStack", .json_str = R"({"error":{"message":"The given namespace does not exist","type":"NoSuchNamespaceException","code":404,"stack":null}})", - .expected_model = {.error = {.message = "The given namespace does not exist", - .type = "NoSuchNamespaceException", - .code = 404}}}, + .expected_model = {.message = "The given namespace does not exist", + .type = "NoSuchNamespaceException", + .code = 404}}, // Stack field is missing (should deserialize to empty vector) ErrorResponseDeserializeParam{ .test_name = "MissingStack", .json_str = R"({"error":{"message":"The given namespace does not exist","type":"NoSuchNamespaceException","code":404}})", - .expected_model = {.error = {.message = "The given namespace does not exist", - .type = "NoSuchNamespaceException", - .code = 404}}}), + .expected_model = {.message = "The given namespace does not exist", + .type = "NoSuchNamespaceException", + .code = 404}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -936,35 +932,26 @@ INSTANTIATE_TEST_SUITE_P( // Null error field ErrorResponseInvalidParam{.test_name = "NullError", .invalid_json_str = R"({"error":null})", - .expected_error_message = "Missing 'error'"}), - [](const ::testing::TestParamInfo& info) { - return info.param.test_name; - }); - -DECLARE_INVALID_TEST(ErrorModel) - -INSTANTIATE_TEST_SUITE_P( - ErrorModelInvalidCases, ErrorModelInvalidTest, - ::testing::Values( + .expected_error_message = "Missing 'error'"}, // Missing required type field - ErrorModelInvalidParam{ + ErrorResponseInvalidParam{ .test_name = "MissingType", .invalid_json_str = - R"({"message":"The given namespace does not exist","code":404})", + R"({"error":{"message":"The given namespace does not exist","code":404}})", .expected_error_message = "Missing 'type'"}, // Missing required code field - ErrorModelInvalidParam{ + ErrorResponseInvalidParam{ .test_name = "MissingCode", .invalid_json_str = - R"({"message":"The given namespace does not exist","type":"NoSuchNamespaceException"})", + R"({"error":{"message":"The given namespace does not exist","type":"NoSuchNamespaceException"}})", .expected_error_message = "Missing 'code'"}, // Wrong type for message field - ErrorModelInvalidParam{ + ErrorResponseInvalidParam{ .test_name = "WrongMessageType", .invalid_json_str = - R"({"message":123,"type":"NoSuchNamespaceException","code":404})", + R"({"error":{"message":123,"type":"NoSuchNamespaceException","code":404}})", .expected_error_message = "type must be string, but is number"}), - [](const ::testing::TestParamInfo& info) { + [](const ::testing::TestParamInfo& info) { return info.param.test_name; });