Skip to content

Commit 5c8c4e5

Browse files
zhjwpkuzhjwpku
authored
fix(rest): align error handlers with Java implementation (#763)
While reviewing #614, I noticed that `PlanErrorHandler::Accept` does not match the behavior of the Java implementation. A detailed comparison with Java's `ErrorHandlers` revealed several gaps between iceberg-cpp and Iceberg Java. Some of these are oversights in the original implementation, while others correspond to improvements that were made later in Iceberg Java, including: * apache/iceberg#13143 * apache/iceberg#14927 * apache/iceberg#15051 * apache/iceberg#16059 This PR closes those gaps and brings the error handling behavior in iceberg-cpp closer to the Java implementation.
1 parent 7fe2e93 commit 5c8c4e5

9 files changed

Lines changed: 488 additions & 57 deletions

File tree

‎src/iceberg/catalog/rest/auth/oauth2_util.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ Result<OAuthTokenResponse> FetchToken(HttpClient& client, AuthSession& session,
6666
ICEBERG_ASSIGN_OR_RAISE(
6767
auto response,
6868
client.PostForm(properties.oauth2_server_uri(), form_data,
69-
/*headers=*/{}, *DefaultErrorHandler::Instance(), session));
69+
/*headers=*/{}, *OAuthErrorHandler::Instance(), session));
7070

7171
ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body()));
7272
ICEBERG_ASSIGN_OR_RAISE(auto token_response, FromJson<OAuthTokenResponse>(json));

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

Lines changed: 128 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@
2121

2222
#include <string_view>
2323

24+
#include "iceberg/catalog/rest/json_serde_internal.h"
2425
#include "iceberg/catalog/rest/types.h"
26+
#include "iceberg/json_serde_internal.h"
27+
#include "iceberg/util/json_util_internal.h"
28+
#include "iceberg/util/macros.h"
2529

2630
namespace iceberg::rest {
2731

@@ -31,8 +35,29 @@ constexpr std::string_view kIllegalArgumentException = "IllegalArgumentException
3135
constexpr std::string_view kNoSuchNamespaceException = "NoSuchNamespaceException";
3236
constexpr std::string_view kNamespaceNotEmptyException = "NamespaceNotEmptyException";
3337
constexpr std::string_view kNoSuchTableException = "NoSuchTableException";
34-
constexpr std::string_view kNoSuchPlanIdException = "NoSuchPlanIdException";
35-
constexpr std::string_view kNoSuchPlanTaskException = "NoSuchPlanTaskException";
38+
constexpr std::string_view kNotFoundException = "NotFoundException";
39+
constexpr std::string_view kRestException = "RESTException";
40+
constexpr std::string_view kInvalidClient = "invalid_client";
41+
constexpr std::string_view kInvalidRequest = "invalid_request";
42+
constexpr std::string_view kInvalidGrant = "invalid_grant";
43+
constexpr std::string_view kUnauthorizedClient = "unauthorized_client";
44+
constexpr std::string_view kUnsupportedGrantType = "unsupported_grant_type";
45+
constexpr std::string_view kInvalidScope = "invalid_scope";
46+
constexpr std::string_view kNull = "null";
47+
constexpr std::string_view kOAuthError = "error";
48+
constexpr std::string_view kOAuthErrorDescription = "error_description";
49+
50+
std::string_view NullIfEmpty(const std::string& value) {
51+
if (value.empty()) {
52+
return kNull;
53+
}
54+
return value;
55+
}
56+
57+
Status CreateRestError(const ErrorResponse& error) {
58+
return RestError("Unable to process (code: {}, type: {}): {}", error.code,
59+
NullIfEmpty(error.type), NullIfEmpty(error.message));
60+
}
3661

3762
} // namespace
3863

@@ -63,7 +88,17 @@ Status DefaultErrorHandler::Accept(const ErrorResponse& error) const {
6388
return ServiceUnavailable("Service unavailable: {}", error.message);
6489
}
6590

66-
return RestError("Code: {}, message: {}", error.code, error.message);
91+
return CreateRestError(error);
92+
}
93+
94+
Result<ErrorResponse> DefaultErrorHandler::ParseResponse(uint32_t /*code*/,
95+
const std::string& text) const {
96+
if (text.empty()) {
97+
return InvalidArgument("Empty response body");
98+
}
99+
ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text));
100+
ICEBERG_ASSIGN_OR_RAISE(auto error_result, ErrorResponseFromJson(json_result));
101+
return error_result;
67102
}
68103

69104
const std::shared_ptr<NamespaceErrorHandler>& NamespaceErrorHandler::Instance() {
@@ -84,7 +119,7 @@ Status NamespaceErrorHandler::Accept(const ErrorResponse& error) const {
84119
case 409:
85120
return AlreadyExists(error.message);
86121
case 422:
87-
return RestError("Unable to process: {}", error.message);
122+
return CreateRestError(error);
88123
}
89124

90125
return DefaultErrorHandler::Accept(error);
@@ -104,37 +139,34 @@ Status DropNamespaceErrorHandler::Accept(const ErrorResponse& error) const {
104139
return NamespaceErrorHandler::Accept(error);
105140
}
106141

107-
const std::shared_ptr<TableErrorHandler>& TableErrorHandler::Instance() {
108-
static const std::shared_ptr<TableErrorHandler> instance{new TableErrorHandler()};
142+
const std::shared_ptr<ConfigErrorHandler>& ConfigErrorHandler::Instance() {
143+
static const std::shared_ptr<ConfigErrorHandler> instance{new ConfigErrorHandler()};
109144
return instance;
110145
}
111146

112-
Status TableErrorHandler::Accept(const ErrorResponse& error) const {
113-
switch (error.code) {
114-
case 404:
115-
if (error.type == kNoSuchNamespaceException) {
116-
return NoSuchNamespace(error.message);
117-
}
118-
return NoSuchTable(error.message);
119-
case 409:
120-
return AlreadyExists(error.message);
147+
Status ConfigErrorHandler::Accept(const ErrorResponse& error) const {
148+
if (error.code == 404 && !error.type.empty() && error.type != kRestException) {
149+
return NoSuchWarehouse(error.message);
121150
}
122151

123152
return DefaultErrorHandler::Accept(error);
124153
}
125154

126-
const std::shared_ptr<ViewErrorHandler>& ViewErrorHandler::Instance() {
127-
static const std::shared_ptr<ViewErrorHandler> instance{new ViewErrorHandler()};
155+
const std::shared_ptr<TableErrorHandler>& TableErrorHandler::Instance() {
156+
static const std::shared_ptr<TableErrorHandler> instance{new TableErrorHandler()};
128157
return instance;
129158
}
130159

131-
Status ViewErrorHandler::Accept(const ErrorResponse& error) const {
160+
Status TableErrorHandler::Accept(const ErrorResponse& error) const {
132161
switch (error.code) {
133162
case 404:
134163
if (error.type == kNoSuchNamespaceException) {
135164
return NoSuchNamespace(error.message);
136165
}
137-
return NoSuchView(error.message);
166+
if (error.type == kNotFoundException) {
167+
return NotFound(error.message);
168+
}
169+
return NoSuchTable(error.message);
138170
case 409:
139171
return AlreadyExists(error.message);
140172
}
@@ -164,6 +196,23 @@ Status TableCommitErrorHandler::Accept(const ErrorResponse& error) const {
164196
return DefaultErrorHandler::Accept(error);
165197
}
166198

199+
const std::shared_ptr<CreateTableErrorHandler>& CreateTableErrorHandler::Instance() {
200+
static const std::shared_ptr<CreateTableErrorHandler> instance{
201+
new CreateTableErrorHandler()};
202+
return instance;
203+
}
204+
205+
Status CreateTableErrorHandler::Accept(const ErrorResponse& error) const {
206+
switch (error.code) {
207+
case 404:
208+
return NoSuchNamespace(error.message);
209+
case 409:
210+
return AlreadyExists(error.message);
211+
}
212+
213+
return TableCommitErrorHandler::Accept(error);
214+
}
215+
167216
const std::shared_ptr<ViewCommitErrorHandler>& ViewCommitErrorHandler::Instance() {
168217
static const std::shared_ptr<ViewCommitErrorHandler> instance{
169218
new ViewCommitErrorHandler()};
@@ -186,6 +235,25 @@ Status ViewCommitErrorHandler::Accept(const ErrorResponse& error) const {
186235
return DefaultErrorHandler::Accept(error);
187236
}
188237

238+
const std::shared_ptr<ViewErrorHandler>& ViewErrorHandler::Instance() {
239+
static const std::shared_ptr<ViewErrorHandler> instance{new ViewErrorHandler()};
240+
return instance;
241+
}
242+
243+
Status ViewErrorHandler::Accept(const ErrorResponse& error) const {
244+
switch (error.code) {
245+
case 404:
246+
if (error.type == kNoSuchNamespaceException) {
247+
return NoSuchNamespace(error.message);
248+
}
249+
return NoSuchView(error.message);
250+
case 409:
251+
return AlreadyExists(error.message);
252+
}
253+
254+
return DefaultErrorHandler::Accept(error);
255+
}
256+
189257
const std::shared_ptr<PlanErrorHandler>& PlanErrorHandler::Instance() {
190258
static const std::shared_ptr<PlanErrorHandler> instance{new PlanErrorHandler()};
191259
return instance;
@@ -200,12 +268,7 @@ Status PlanErrorHandler::Accept(const ErrorResponse& error) const {
200268
if (error.type == kNoSuchTableException) {
201269
return NoSuchTable(error.message);
202270
}
203-
if (error.type == kNoSuchPlanIdException) {
204-
return NoSuchPlanId(error.message);
205-
}
206-
return NotFound(error.message);
207-
case 406:
208-
return NotSupported(error.message);
271+
return NoSuchPlanId(error.message);
209272
}
210273

211274
return DefaultErrorHandler::Accept(error);
@@ -225,13 +288,49 @@ Status PlanTaskErrorHandler::Accept(const ErrorResponse& error) const {
225288
if (error.type == kNoSuchTableException) {
226289
return NoSuchTable(error.message);
227290
}
228-
if (error.type == kNoSuchPlanTaskException) {
229-
return NoSuchPlanTask(error.message);
230-
}
231-
return NotFound(error.message);
291+
return NoSuchPlanTask(error.message);
232292
}
233293

234294
return DefaultErrorHandler::Accept(error);
235295
}
236296

297+
const std::shared_ptr<OAuthErrorHandler>& OAuthErrorHandler::Instance() {
298+
static const std::shared_ptr<OAuthErrorHandler> instance{new OAuthErrorHandler()};
299+
return instance;
300+
}
301+
302+
Status OAuthErrorHandler::Accept(const ErrorResponse& error) const {
303+
if (!error.type.empty()) {
304+
if (error.type == kInvalidClient) {
305+
return NotAuthorized("Not authorized: {}: {}", error.type,
306+
NullIfEmpty(error.message));
307+
}
308+
if (error.type == kInvalidRequest || error.type == kInvalidGrant ||
309+
error.type == kUnauthorizedClient || error.type == kUnsupportedGrantType ||
310+
error.type == kInvalidScope) {
311+
return BadRequest("Malformed request: {}: {}", error.type,
312+
NullIfEmpty(error.message));
313+
}
314+
}
315+
316+
return CreateRestError(error);
317+
}
318+
319+
Result<ErrorResponse> OAuthErrorHandler::ParseResponse(uint32_t code,
320+
const std::string& text) const {
321+
if (text.empty()) {
322+
return InvalidArgument("Empty response body");
323+
}
324+
325+
ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text));
326+
327+
ErrorResponse error;
328+
error.code = code;
329+
ICEBERG_ASSIGN_OR_RAISE(error.type,
330+
GetJsonValue<std::string>(json_result, kOAuthError));
331+
ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValueOrDefault<std::string>(
332+
json_result, kOAuthErrorDescription));
333+
return error;
334+
}
335+
237336
} // namespace iceberg::rest

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

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,12 @@
1919

2020
#pragma once
2121

22+
#include <cstdint>
2223
#include <memory>
24+
#include <string>
2325

2426
#include "iceberg/catalog/rest/iceberg_rest_export.h"
25-
#include "iceberg/catalog/rest/type_fwd.h"
27+
#include "iceberg/catalog/rest/types.h"
2628
#include "iceberg/result.h"
2729

2830
/// \file iceberg/catalog/rest/error_handlers.h
@@ -41,6 +43,14 @@ class ICEBERG_REST_EXPORT ErrorHandler {
4143
/// \param error The error response parsed from the HTTP response body
4244
/// \return An Error object with appropriate ErrorKind and message
4345
virtual Status Accept(const ErrorResponse& error) const = 0;
46+
47+
/// \brief Parse an HTTP error response body.
48+
///
49+
/// \param code The HTTP status code from the failed response
50+
/// \param text The HTTP response body
51+
/// \return The parsed error response
52+
virtual Result<ErrorResponse> ParseResponse(uint32_t code,
53+
const std::string& text) const = 0;
4454
};
4555

4656
/// \brief Default error handler for REST API responses.
@@ -50,6 +60,8 @@ class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler {
5060
static const std::shared_ptr<DefaultErrorHandler>& Instance();
5161

5262
Status Accept(const ErrorResponse& error) const override;
63+
Result<ErrorResponse> ParseResponse(uint32_t code,
64+
const std::string& text) const override;
5365

5466
protected:
5567
constexpr DefaultErrorHandler() = default;
@@ -79,6 +91,18 @@ class ICEBERG_REST_EXPORT DropNamespaceErrorHandler final : public NamespaceErro
7991
constexpr DropNamespaceErrorHandler() = default;
8092
};
8193

94+
/// \brief Error handler for the catalog config endpoint.
95+
class ICEBERG_REST_EXPORT ConfigErrorHandler final : public DefaultErrorHandler {
96+
public:
97+
/// \brief Returns the singleton instance
98+
static const std::shared_ptr<ConfigErrorHandler>& Instance();
99+
100+
Status Accept(const ErrorResponse& error) const override;
101+
102+
private:
103+
constexpr ConfigErrorHandler() = default;
104+
};
105+
82106
/// \brief Table-level error handler.
83107
class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler {
84108
public:
@@ -91,28 +115,40 @@ class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler {
91115
constexpr TableErrorHandler() = default;
92116
};
93117

94-
/// \brief View-level error handler.
95-
class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler {
118+
/// \brief Table commit operation error handler.
119+
class ICEBERG_REST_EXPORT TableCommitErrorHandler : public DefaultErrorHandler {
96120
public:
97121
/// \brief Returns the singleton instance
98-
static const std::shared_ptr<ViewErrorHandler>& Instance();
122+
static const std::shared_ptr<TableCommitErrorHandler>& Instance();
123+
124+
Status Accept(const ErrorResponse& error) const override;
125+
126+
protected:
127+
constexpr TableCommitErrorHandler() = default;
128+
};
129+
130+
/// \brief Table create commit operation error handler.
131+
class ICEBERG_REST_EXPORT CreateTableErrorHandler final : public TableCommitErrorHandler {
132+
public:
133+
/// \brief Returns the singleton instance
134+
static const std::shared_ptr<CreateTableErrorHandler>& Instance();
99135

100136
Status Accept(const ErrorResponse& error) const override;
101137

102138
private:
103-
constexpr ViewErrorHandler() = default;
139+
constexpr CreateTableErrorHandler() = default;
104140
};
105141

106-
/// \brief Table commit operation error handler.
107-
class ICEBERG_REST_EXPORT TableCommitErrorHandler final : public DefaultErrorHandler {
142+
/// \brief View-level error handler.
143+
class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler {
108144
public:
109145
/// \brief Returns the singleton instance
110-
static const std::shared_ptr<TableCommitErrorHandler>& Instance();
146+
static const std::shared_ptr<ViewErrorHandler>& Instance();
111147

112148
Status Accept(const ErrorResponse& error) const override;
113149

114150
private:
115-
constexpr TableCommitErrorHandler() = default;
151+
constexpr ViewErrorHandler() = default;
116152
};
117153

118154
/// \brief View commit operation error handler.
@@ -149,4 +185,17 @@ class ICEBERG_REST_EXPORT PlanTaskErrorHandler final : public DefaultErrorHandle
149185
constexpr PlanTaskErrorHandler() = default;
150186
};
151187

188+
/// \brief OAuth token endpoint error handler.
189+
class ICEBERG_REST_EXPORT OAuthErrorHandler final : public ErrorHandler {
190+
public:
191+
static const std::shared_ptr<OAuthErrorHandler>& Instance();
192+
193+
Status Accept(const ErrorResponse& error) const override;
194+
Result<ErrorResponse> ParseResponse(uint32_t code,
195+
const std::string& text) const override;
196+
197+
private:
198+
constexpr OAuthErrorHandler() = default;
199+
};
200+
152201
} // namespace iceberg::rest

0 commit comments

Comments
 (0)