Skip to content

Commit 913b335

Browse files
wgtmacwgtmac
authored
refactor: consolidate rest error response model (#359)
1 parent 3b393e3 commit 913b335

File tree

8 files changed

+65
-102
lines changed

8 files changed

+65
-102
lines changed

‎src/iceberg/catalog/rest/error_handlers.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ const std::shared_ptr<DefaultErrorHandler>& DefaultErrorHandler::Instance() {
3838
return instance;
3939
}
4040

41-
Status DefaultErrorHandler::Accept(const ErrorModel& error) const {
41+
Status DefaultErrorHandler::Accept(const ErrorResponse& error) const {
4242
switch (error.code) {
4343
case 400:
4444
if (error.type == kIllegalArgumentException) {
@@ -69,7 +69,7 @@ const std::shared_ptr<NamespaceErrorHandler>& NamespaceErrorHandler::Instance()
6969
return instance;
7070
}
7171

72-
Status NamespaceErrorHandler::Accept(const ErrorModel& error) const {
72+
Status NamespaceErrorHandler::Accept(const ErrorResponse& error) const {
7373
switch (error.code) {
7474
case 400:
7575
if (error.type == kNamespaceNotEmptyException) {
@@ -93,7 +93,7 @@ const std::shared_ptr<DropNamespaceErrorHandler>& DropNamespaceErrorHandler::Ins
9393
return instance;
9494
}
9595

96-
Status DropNamespaceErrorHandler::Accept(const ErrorModel& error) const {
96+
Status DropNamespaceErrorHandler::Accept(const ErrorResponse& error) const {
9797
if (error.code == 409) {
9898
return NamespaceNotEmpty(error.message);
9999
}
@@ -106,7 +106,7 @@ const std::shared_ptr<TableErrorHandler>& TableErrorHandler::Instance() {
106106
return instance;
107107
}
108108

109-
Status TableErrorHandler::Accept(const ErrorModel& error) const {
109+
Status TableErrorHandler::Accept(const ErrorResponse& error) const {
110110
switch (error.code) {
111111
case 404:
112112
if (error.type == kNoSuchNamespaceException) {
@@ -125,7 +125,7 @@ const std::shared_ptr<ViewErrorHandler>& ViewErrorHandler::Instance() {
125125
return instance;
126126
}
127127

128-
Status ViewErrorHandler::Accept(const ErrorModel& error) const {
128+
Status ViewErrorHandler::Accept(const ErrorResponse& error) const {
129129
switch (error.code) {
130130
case 404:
131131
if (error.type == kNoSuchNamespaceException) {
@@ -145,7 +145,7 @@ const std::shared_ptr<TableCommitErrorHandler>& TableCommitErrorHandler::Instanc
145145
return instance;
146146
}
147147

148-
Status TableCommitErrorHandler::Accept(const ErrorModel& error) const {
148+
Status TableCommitErrorHandler::Accept(const ErrorResponse& error) const {
149149
switch (error.code) {
150150
case 404:
151151
return NoSuchTable(error.message);
@@ -167,7 +167,7 @@ const std::shared_ptr<ViewCommitErrorHandler>& ViewCommitErrorHandler::Instance(
167167
return instance;
168168
}
169169

170-
Status ViewCommitErrorHandler::Accept(const ErrorModel& error) const {
170+
Status ViewCommitErrorHandler::Accept(const ErrorResponse& error) const {
171171
switch (error.code) {
172172
case 404:
173173
return NoSuchView(error.message);

‎src/iceberg/catalog/rest/error_handlers.h‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,11 @@ class ICEBERG_REST_EXPORT ErrorHandler {
3636
public:
3737
virtual ~ErrorHandler() = default;
3838

39-
// TODO(Li Feiyang):removing ErrorModel as the inner layer and directly using
40-
// ErrorResponse
41-
4239
/// \brief Process an error response and return an appropriate Error.
4340
///
44-
/// \param error The error model parsed from the HTTP response body
41+
/// \param error The error response parsed from the HTTP response body
4542
/// \return An Error object with appropriate ErrorKind and message
46-
virtual Status Accept(const ErrorModel& error) const = 0;
43+
virtual Status Accept(const ErrorResponse& error) const = 0;
4744
};
4845

4946
/// \brief Default error handler for REST API responses.
@@ -52,7 +49,7 @@ class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
5249
/// \brief Returns the singleton instance
5350
static const std::shared_ptr<DefaultErrorHandler>& Instance();
5451

55-
Status Accept(const ErrorModel& error) const override;
52+
Status Accept(const ErrorResponse& error) const override;
5653

5754
protected:
5855
constexpr DefaultErrorHandler() = default;
@@ -64,7 +61,7 @@ class ICEBERG_REST_EXPORT NamespaceErrorHandler : public DefaultErrorHandler {
6461
/// \brief Returns the singleton instance
6562
static const std::shared_ptr<NamespaceErrorHandler>& Instance();
6663

67-
Status Accept(const ErrorModel& error) const override;
64+
Status Accept(const ErrorResponse& error) const override;
6865

6966
protected:
7067
constexpr NamespaceErrorHandler() = default;
@@ -76,7 +73,7 @@ class ICEBERG_REST_EXPORT DropNamespaceErrorHandler final : public NamespaceErro
7673
/// \brief Returns the singleton instance
7774
static const std::shared_ptr<DropNamespaceErrorHandler>& Instance();
7875

79-
Status Accept(const ErrorModel& error) const override;
76+
Status Accept(const ErrorResponse& error) const override;
8077

8178
private:
8279
constexpr DropNamespaceErrorHandler() = default;
@@ -88,7 +85,7 @@ class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler {
8885
/// \brief Returns the singleton instance
8986
static const std::shared_ptr<TableErrorHandler>& Instance();
9087

91-
Status Accept(const ErrorModel& error) const override;
88+
Status Accept(const ErrorResponse& error) const override;
9289

9390
private:
9491
constexpr TableErrorHandler() = default;
@@ -100,7 +97,7 @@ class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler {
10097
/// \brief Returns the singleton instance
10198
static const std::shared_ptr<ViewErrorHandler>& Instance();
10299

103-
Status Accept(const ErrorModel& error) const override;
100+
Status Accept(const ErrorResponse& error) const override;
104101

105102
private:
106103
constexpr ViewErrorHandler() = default;
@@ -112,7 +109,7 @@ class ICEBERG_REST_EXPORT TableCommitErrorHandler final : public DefaultErrorHan
112109
/// \brief Returns the singleton instance
113110
static const std::shared_ptr<TableCommitErrorHandler>& Instance();
114111

115-
Status Accept(const ErrorModel& error) const override;
112+
Status Accept(const ErrorResponse& error) const override;
116113

117114
private:
118115
constexpr TableCommitErrorHandler() = default;
@@ -124,7 +121,7 @@ class ICEBERG_REST_EXPORT ViewCommitErrorHandler final : public DefaultErrorHand
124121
/// \brief Returns the singleton instance
125122
static const std::shared_ptr<ViewCommitErrorHandler>& Instance();
126123

127-
Status Accept(const ErrorModel& error) const override;
124+
Status Accept(const ErrorResponse& error) const override;
128125

129126
private:
130127
constexpr ViewCommitErrorHandler() = default;

‎src/iceberg/catalog/rest/http_client.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ Status HandleFailureResponse(const cpr::Response& response,
103103
// TODO(gangwu): response status code is lost, wrap it with RestError.
104104
ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.text));
105105
ICEBERG_ASSIGN_OR_RAISE(auto error_response, ErrorResponseFromJson(json));
106-
return error_handler.Accept(error_response.error);
106+
return error_handler.Accept(error_response);
107107
}
108108
return {};
109109
}

‎src/iceberg/catalog/rest/json_internal.cc‎

Lines changed: 17 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -91,43 +91,34 @@ Result<CatalogConfig> CatalogConfigFromJson(const nlohmann::json& json) {
9191
return config;
9292
}
9393

94-
nlohmann::json ToJson(const ErrorModel& error) {
94+
nlohmann::json ToJson(const ErrorResponse& error) {
95+
nlohmann::json error_json;
96+
error_json[kMessage] = error.message;
97+
error_json[kType] = error.type;
98+
error_json[kCode] = error.code;
99+
SetContainerField(error_json, kStack, error.stack);
100+
95101
nlohmann::json json;
96-
json[kMessage] = error.message;
97-
json[kType] = error.type;
98-
json[kCode] = error.code;
99-
SetContainerField(json, kStack, error.stack);
102+
json[kError] = std::move(error_json);
100103
return json;
101104
}
102105

103-
Result<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
104-
ErrorModel error;
106+
Result<ErrorResponse> ErrorResponseFromJson(const nlohmann::json& json) {
107+
ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue<nlohmann::json>(json, kError));
108+
109+
ErrorResponse error;
105110
// NOTE: Iceberg's Java implementation allows missing required fields (message, type,
106111
// code) during deserialization, which deviates from the REST spec. We enforce strict
107112
// validation here.
108-
ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue<std::string>(json, kMessage));
109-
ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue<std::string>(json, kType));
110-
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint32_t>(json, kCode));
111-
ICEBERG_ASSIGN_OR_RAISE(error.stack,
112-
GetJsonValueOrDefault<std::vector<std::string>>(json, kStack));
113+
ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValue<std::string>(error_json, kMessage));
114+
ICEBERG_ASSIGN_OR_RAISE(error.type, GetJsonValue<std::string>(error_json, kType));
115+
ICEBERG_ASSIGN_OR_RAISE(error.code, GetJsonValue<uint32_t>(error_json, kCode));
116+
ICEBERG_ASSIGN_OR_RAISE(
117+
error.stack, GetJsonValueOrDefault<std::vector<std::string>>(error_json, kStack));
113118
ICEBERG_RETURN_UNEXPECTED(error.Validate());
114119
return error;
115120
}
116121

117-
nlohmann::json ToJson(const ErrorResponse& response) {
118-
nlohmann::json json;
119-
json[kError] = ToJson(response.error);
120-
return json;
121-
}
122-
123-
Result<ErrorResponse> ErrorResponseFromJson(const nlohmann::json& json) {
124-
ErrorResponse response;
125-
ICEBERG_ASSIGN_OR_RAISE(auto error_json, GetJsonValue<nlohmann::json>(json, kError));
126-
ICEBERG_ASSIGN_OR_RAISE(response.error, ErrorModelFromJson(error_json));
127-
ICEBERG_RETURN_UNEXPECTED(response.Validate());
128-
return response;
129-
}
130-
131122
nlohmann::json ToJson(const CreateNamespaceRequest& request) {
132123
nlohmann::json json;
133124
json[kNamespace] = request.namespace_.levels;
@@ -339,7 +330,6 @@ Result<ListTablesResponse> ListTablesResponseFromJson(const nlohmann::json& json
339330
}
340331

341332
ICEBERG_DEFINE_FROM_JSON(CatalogConfig)
342-
ICEBERG_DEFINE_FROM_JSON(ErrorModel)
343333
ICEBERG_DEFINE_FROM_JSON(ErrorResponse)
344334
ICEBERG_DEFINE_FROM_JSON(ListNamespacesResponse)
345335
ICEBERG_DEFINE_FROM_JSON(CreateNamespaceRequest)

‎src/iceberg/catalog/rest/json_internal.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ Result<Model> FromJson(const nlohmann::json& json);
4444
/// \note Don't forget to add `ICEBERG_DEFINE_FROM_JSON` to the end of
4545
/// `json_internal.cc` to define the `FromJson` function for the model.
4646
ICEBERG_DECLARE_JSON_SERDE(CatalogConfig)
47-
ICEBERG_DECLARE_JSON_SERDE(ErrorModel)
4847
ICEBERG_DECLARE_JSON_SERDE(ErrorResponse)
4948
ICEBERG_DECLARE_JSON_SERDE(ListNamespacesResponse)
5049
ICEBERG_DECLARE_JSON_SERDE(CreateNamespaceRequest)

‎src/iceberg/catalog/rest/type_fwd.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424

2525
namespace iceberg::rest {
2626

27-
struct ErrorModel;
27+
struct ErrorResponse;
2828

2929
class ErrorHandler;
3030
class HttpClient;

‎src/iceberg/catalog/rest/types.h‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,37 +52,27 @@ struct ICEBERG_REST_EXPORT CatalogConfig {
5252
};
5353

5454
/// \brief JSON error payload returned in a response with further details on the error.
55-
struct ICEBERG_REST_EXPORT ErrorModel {
55+
struct ICEBERG_REST_EXPORT ErrorResponse {
5656
std::string message; // required
5757
std::string type; // required
5858
uint32_t code; // required
5959
std::vector<std::string> stack;
6060

61-
/// \brief Validates the ErrorModel.
61+
/// \brief Validates the ErrorResponse.
6262
Status Validate() const {
6363
if (message.empty() || type.empty()) {
64-
return Invalid("Invalid error model: missing required fields");
64+
return Invalid("Invalid error response: missing required fields");
6565
}
6666

6767
if (code < 400 || code > 600) {
68-
return Invalid("Invalid error model: code {} is out of range [400, 600]", code);
68+
return Invalid("Invalid error response: code {} is out of range [400, 600]", code);
6969
}
7070

7171
// stack is optional, no validation needed
7272
return {};
7373
}
7474
};
7575

76-
/// \brief Error response body returned in a response.
77-
struct ICEBERG_REST_EXPORT ErrorResponse {
78-
ErrorModel error; // required
79-
80-
/// \brief Validates the ErrorResponse.
81-
// We don't validate the error field because ErrorModel::Validate has been called in the
82-
// FromJson.
83-
Status Validate() const { return {}; }
84-
};
85-
8676
/// \brief Request to create a namespace.
8777
struct ICEBERG_REST_EXPORT CreateNamespaceRequest {
8878
Namespace namespace_; // required

0 commit comments

Comments
 (0)