From 9f15a1010367a773bc0d10c0417d4105c4b6280f Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Tue, 4 Nov 2025 15:17:14 +0800 Subject: [PATCH 1/3] feat: add config, error and validation for rest types --- src/iceberg/catalog/rest/CMakeLists.txt | 2 +- src/iceberg/catalog/rest/json_internal.cc | 79 +++++++ src/iceberg/catalog/rest/json_internal.h | 6 + src/iceberg/catalog/rest/meson.build | 11 +- src/iceberg/catalog/rest/types.h | 21 +- src/iceberg/catalog/rest/validator.cc | 139 ++++++++++++ src/iceberg/catalog/rest/validator.h | 82 +++++++ src/iceberg/test/rest_json_internal_test.cc | 223 +++++++++++++++++++- 8 files changed, 558 insertions(+), 5 deletions(-) create mode 100644 src/iceberg/catalog/rest/validator.cc create mode 100644 src/iceberg/catalog/rest/validator.h diff --git a/src/iceberg/catalog/rest/CMakeLists.txt b/src/iceberg/catalog/rest/CMakeLists.txt index 38d897270..02440514e 100644 --- a/src/iceberg/catalog/rest/CMakeLists.txt +++ b/src/iceberg/catalog/rest/CMakeLists.txt @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. -set(ICEBERG_REST_SOURCES rest_catalog.cc json_internal.cc) +set(ICEBERG_REST_SOURCES rest_catalog.cc json_internal.cc validator.cc) set(ICEBERG_REST_STATIC_BUILD_INTERFACE_LIBS) set(ICEBERG_REST_SHARED_BUILD_INTERFACE_LIBS) diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index 452de7a30..85f7e8ca8 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -27,6 +27,7 @@ #include #include "iceberg/catalog/rest/types.h" +#include "iceberg/catalog/rest/validator.h" #include "iceberg/json_internal.h" #include "iceberg/table_identifier.h" #include "iceberg/util/json_util_internal.h" @@ -59,9 +60,77 @@ constexpr std::string_view kDestination = "destination"; constexpr std::string_view kMetadata = "metadata"; constexpr std::string_view kConfig = "config"; constexpr std::string_view kIdentifiers = "identifiers"; +constexpr std::string_view kOverrides = "overrides"; +constexpr std::string_view kDefaults = "defaults"; +constexpr std::string_view kEndpoints = "endpoints"; +constexpr std::string_view kMessage = "message"; +constexpr std::string_view kType = "type"; +constexpr std::string_view kCode = "code"; +constexpr std::string_view kStack = "stack"; +constexpr std::string_view kError = "error"; } // namespace +nlohmann::json ToJson(const CatalogConfig& config) { + nlohmann::json json; + json[kOverrides] = config.overrides; + json[kDefaults] = config.defaults; + if (!config.endpoints.empty()) { + json[kEndpoints] = config.endpoints; + } + return json; +} + +Result CatalogConfigFromJson(const nlohmann::json& json) { + CatalogConfig config; + ICEBERG_ASSIGN_OR_RAISE( + config.overrides, + GetJsonValueOrDefault(json, kOverrides)); + ICEBERG_ASSIGN_OR_RAISE( + config.defaults, GetJsonValueOrDefault(json, kDefaults)); + ICEBERG_ASSIGN_OR_RAISE( + config.endpoints, + GetJsonValueOrDefault>(json, kEndpoints)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(config)); + return config; +} + +nlohmann::json ToJson(const ErrorModel& error) { + nlohmann::json json; + json[kMessage] = error.message; + json[kType] = error.type; + json[kCode] = error.code; + if (!error.stack.empty()) { + json[kStack] = error.stack; + } + return json; +} + +Result ErrorModelFromJson(const nlohmann::json& json) { + ErrorModel error; + 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_RETURN_UNEXPECTED(Validator::Validate(error)); + 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(Validator::Validate(response)); + return response; +} + nlohmann::json ToJson(const CreateNamespaceRequest& request) { nlohmann::json json; json[kNamespace] = request.namespace_.levels; @@ -77,6 +146,7 @@ Result CreateNamespaceRequestFromJson( ICEBERG_ASSIGN_OR_RAISE( request.properties, GetJsonValueOrDefault(json, kProperties)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); return request; } @@ -94,6 +164,7 @@ Result UpdateNamespacePropertiesRequestFromJso request.removals, GetJsonValueOrDefault>(json, kRemovals)); ICEBERG_ASSIGN_OR_RAISE( request.updates, GetJsonValueOrDefault(json, kUpdates)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); return request; } @@ -114,6 +185,7 @@ Result RegisterTableRequestFromJson(const nlohmann::json& GetJsonValue(json, kMetadataLocation)); ICEBERG_ASSIGN_OR_RAISE(request.overwrite, GetJsonValueOrDefault(json, kOverwrite, false)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); return request; } @@ -131,6 +203,7 @@ Result RenameTableRequestFromJson(const nlohmann::json& json ICEBERG_ASSIGN_OR_RAISE(auto dest_json, GetJsonValue(json, kDestination)); ICEBERG_ASSIGN_OR_RAISE(request.destination, TableIdentifierFromJson(dest_json)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(request)); return request; } @@ -177,6 +250,7 @@ Result ListNamespacesResponseFromJson( ICEBERG_ASSIGN_OR_RAISE(auto ns, NamespaceFromJson(ns_json)); response.namespaces.push_back(std::move(ns)); } + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response)); return response; } @@ -232,6 +306,7 @@ Result UpdateNamespacePropertiesResponseFromJ response.removed, GetJsonValueOrDefault>(json, kRemoved)); ICEBERG_ASSIGN_OR_RAISE( response.missing, GetJsonValueOrDefault>(json, kMissing)); + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response)); return response; } @@ -256,6 +331,7 @@ Result ListTablesResponseFromJson(const nlohmann::json& json ICEBERG_ASSIGN_OR_RAISE(auto identifier, TableIdentifierFromJson(id_json)); response.identifiers.push_back(std::move(identifier)); } + ICEBERG_RETURN_UNEXPECTED(Validator::Validate(response)); return response; } @@ -265,6 +341,9 @@ Result ListTablesResponseFromJson(const nlohmann::json& json return Model##FromJson(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) ICEBERG_DEFINE_FROM_JSON(CreateNamespaceResponse) diff --git a/src/iceberg/catalog/rest/json_internal.h b/src/iceberg/catalog/rest/json_internal.h index 129e88393..986066f1a 100644 --- a/src/iceberg/catalog/rest/json_internal.h +++ b/src/iceberg/catalog/rest/json_internal.h @@ -25,6 +25,9 @@ #include "iceberg/catalog/rest/types.h" #include "iceberg/result.h" +/// \file iceberg/catalog/rest/json_internal.h +/// JSON serialization and deserialization for Iceberg REST Catalog API types. + namespace iceberg::rest { template @@ -40,6 +43,9 @@ 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) ICEBERG_DECLARE_JSON_SERDE(CreateNamespaceResponse) diff --git a/src/iceberg/catalog/rest/meson.build b/src/iceberg/catalog/rest/meson.build index 5f1f635ab..e8edc35c0 100644 --- a/src/iceberg/catalog/rest/meson.build +++ b/src/iceberg/catalog/rest/meson.build @@ -15,7 +15,11 @@ # specific language governing permissions and limitations # under the License. -iceberg_rest_sources = files('json_internal.cc', 'rest_catalog.cc') +iceberg_rest_sources = files( + 'json_internal.cc', + 'rest_catalog.cc', + 'validator.cc', +) # cpr does not export symbols, so on Windows it must # be used as a static lib cpr_needs_static = ( @@ -46,4 +50,7 @@ iceberg_rest_dep = declare_dependency( meson.override_dependency('iceberg-rest', iceberg_rest_dep) pkg.generate(iceberg_rest_lib) -install_headers(['rest_catalog.h', 'types.h'], subdir: 'iceberg/catalog/rest') +install_headers( + ['rest_catalog.h', 'types.h', 'json_internal.h', 'validator.h'], + subdir: 'iceberg/catalog/rest', +) diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index 11411cdb7..bc3a734fe 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -20,7 +20,6 @@ #pragma once #include -#include #include #include #include @@ -34,6 +33,26 @@ namespace iceberg::rest { +/// \brief Server-provided configuration for the catalog. +struct ICEBERG_REST_EXPORT CatalogConfig { + std::unordered_map overrides; // required + std::unordered_map defaults; // required + std::vector endpoints; +}; + +/// \brief JSON error payload returned in a response with further details on the error. +struct ICEBERG_REST_EXPORT ErrorModel { + std::string message; // required + std::string type; // required + uint16_t code; // required + std::vector stack; +}; + +/// \brief Error response body returned in a response. +struct ICEBERG_REST_EXPORT ErrorResponse { + ErrorModel error; // required +}; + /// \brief Request to create a namespace. struct ICEBERG_REST_EXPORT CreateNamespaceRequest { Namespace namespace_; // required diff --git a/src/iceberg/catalog/rest/validator.cc b/src/iceberg/catalog/rest/validator.cc new file mode 100644 index 000000000..5e8749f00 --- /dev/null +++ b/src/iceberg/catalog/rest/validator.cc @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/validator.h" + +#include +#include +#include +#include + +#include "iceberg/catalog/rest/types.h" +#include "iceberg/result.h" + +namespace iceberg::rest { + +// Configuration and Error types + +Status Validator::Validate(const CatalogConfig& config) { + // TODO(Li Feiyang): Add an invalidEndpoint test that validates endpoint format. + // See: + // https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponseParser.java#L164 + // for reference. + return {}; +} + +Status Validator::Validate(const ErrorModel& error) { + if (error.message.empty() || error.type.empty()) [[unlikely]] { + return Invalid("Invalid error model: missing required fields"); + } + + if (error.code < 400 || error.code > 600) [[unlikely]] { + return Invalid("Invalid error model: code must be between 400 and 600"); + } + + // stack is optional, no validation needed + return {}; +} + +Status Validator::Validate(const ErrorResponse& response) { return {}; } + +// Namespace operations + +Status Validator::Validate(const ListNamespacesResponse& response) { return {}; } + +Status Validator::Validate(const CreateNamespaceRequest& request) { return {}; } + +Status Validator::Validate(const CreateNamespaceResponse& response) { return {}; } + +Status Validator::Validate(const GetNamespaceResponse& response) { return {}; } + +Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) { + // keys in updates and removals must not overlap + if (request.removals.empty() || request.updates.empty()) [[unlikely]] { + return {}; + } + + std::unordered_set remove_set(request.removals.begin(), + request.removals.end()); + std::vector common; + + for (const std::string& k : request.updates | std::views::keys) { + if (remove_set.contains(k)) { + common.push_back(k); + } + } + + if (!common.empty()) { + std::string keys; + bool first = true; + for (const std::string& s : common) { + if (!std::exchange(first, false)) keys += ", "; + keys += s; + } + + return Invalid( + "Invalid namespace properties update: cannot simultaneously set and remove keys: " + "[{}]", + keys); + } + return {}; +} + +Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) { + return {}; +} + +// Table operations + +Status Validator::Validate(const ListTablesResponse& response) { return {}; } + +Status Validator::Validate(const LoadTableResult& result) { + if (!result.metadata) [[unlikely]] { + return Invalid("Invalid metadata: null"); + } + return {}; +} + +Status Validator::Validate(const RegisterTableRequest& request) { + if (request.name.empty()) [[unlikely]] { + return Invalid("Invalid table name: empty"); + } + + if (request.metadata_location.empty()) [[unlikely]] { + return Invalid("Invalid metadata location: empty"); + } + + return {}; +} + +Status Validator::Validate(const RenameTableRequest& request) { + if (request.source.ns.levels.empty() || request.source.name.empty()) [[unlikely]] { + return Invalid("Invalid source identifier"); + } + + if (request.destination.ns.levels.empty() || request.destination.name.empty()) + [[unlikely]] { + return Invalid("Invalid destination identifier"); + } + + return {}; +} + +} // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/validator.h b/src/iceberg/catalog/rest/validator.h new file mode 100644 index 000000000..44c445eb0 --- /dev/null +++ b/src/iceberg/catalog/rest/validator.h @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/catalog/rest/types.h" +#include "iceberg/result.h" + +/// \file iceberg/catalog/rest/validator.h +/// Validator for REST Catalog API types. + +namespace iceberg::rest { + +/// \brief Validator for REST Catalog API types. Validation should be called after +/// deserializing objects from external sources to ensure data integrity before the +/// objects are used. +class ICEBERG_REST_EXPORT Validator { + public: + // Configuration and Error types + + /// \brief Validates a CatalogConfig object. + static Status Validate(const CatalogConfig& config); + + /// \brief Validates an ErrorModel object. + static Status Validate(const ErrorModel& error); + + /// \brief Validates an ErrorResponse object. + static Status Validate(const ErrorResponse& response); + + // Namespace operations + + /// \brief Validates a ListNamespacesResponse object. + static Status Validate(const ListNamespacesResponse& response); + + /// \brief Validates a CreateNamespaceRequest object. + static Status Validate(const CreateNamespaceRequest& request); + + /// \brief Validates a CreateNamespaceResponse object. + static Status Validate(const CreateNamespaceResponse& response); + + /// \brief Validates a GetNamespaceResponse object. + static Status Validate(const GetNamespaceResponse& response); + + /// \brief Validates an UpdateNamespacePropertiesRequest object. + static Status Validate(const UpdateNamespacePropertiesRequest& request); + + /// \brief Validates an UpdateNamespacePropertiesResponse object. + static Status Validate(const UpdateNamespacePropertiesResponse& response); + + // Table operations + + /// \brief Validates a ListTablesResponse object. + static Status Validate(const ListTablesResponse& response); + + /// \brief Validates a LoadTableResult object. + static Status Validate(const LoadTableResult& result); + + /// \brief Validates a RegisterTableRequest object. + static Status Validate(const RegisterTableRequest& request); + + /// \brief Validates a RenameTableRequest object. + static Status Validate(const RenameTableRequest& request); +}; + +} // namespace iceberg::rest diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index d95f6a2c3..24be1bebf 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -17,7 +17,6 @@ * under the License. */ -#include #include #include #include @@ -92,6 +91,20 @@ bool operator==(const RenameTableRequest& lhs, const RenameTableRequest& rhs) { lhs.destination.name == rhs.destination.name; } +bool operator==(const CatalogConfig& lhs, const CatalogConfig& rhs) { + return lhs.overrides == rhs.overrides && lhs.defaults == rhs.defaults && + lhs.endpoints == rhs.endpoints; +} + +bool operator==(const ErrorModel& lhs, const ErrorModel& 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 { @@ -747,4 +760,212 @@ INSTANTIATE_TEST_SUITE_P( return info.param.test_name; }); +DECLARE_ROUNDTRIP_TEST(CatalogConfig) + +INSTANTIATE_TEST_SUITE_P( + CatalogConfigCases, CatalogConfigTest, + ::testing::Values( + // Full config with both defaults and overrides + CatalogConfigParam{ + .test_name = "FullConfig", + .expected_json_str = + R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":{"clients":"5"}})", + .model = {.overrides = {{"clients", "5"}}, + .defaults = {{"warehouse", "s3://bucket/warehouse"}}}}, + // Only defaults + CatalogConfigParam{ + .test_name = "OnlyDefaults", + .expected_json_str = + R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":{}})", + .model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}}}, + // Only overrides + CatalogConfigParam{ + .test_name = "OnlyOverrides", + .expected_json_str = R"({"defaults":{},"overrides":{"clients":"5"}})", + .model = {.overrides = {{"clients", "5"}}}}, + // Both empty + CatalogConfigParam{.test_name = "BothEmpty", + .expected_json_str = R"({"defaults":{},"overrides":{}})", + .model = {}}, + // With endpoints + CatalogConfigParam{ + .test_name = "WithEndpoints", + .expected_json_str = + R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":{"clients":"5"},"endpoints":["GET /v1/config","POST /v1/tables"]})", + .model = {.overrides = {{"clients", "5"}}, + .defaults = {{"warehouse", "s3://bucket/warehouse"}}, + .endpoints = {"GET /v1/config", "POST /v1/tables"}}}, + // Only endpoints + CatalogConfigParam{ + .test_name = "OnlyEndpoints", + .expected_json_str = + R"({"defaults":{},"overrides":{},"endpoints":["GET /v1/config"]})", + .model = {.endpoints = {"GET /v1/config"}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_DESERIALIZE_TEST(CatalogConfig) + +INSTANTIATE_TEST_SUITE_P( + CatalogConfigDeserializeCases, CatalogConfigDeserializeTest, + ::testing::Values( + // Missing overrides field + CatalogConfigDeserializeParam{ + .test_name = "MissingOverrides", + .json_str = R"({"defaults":{"warehouse":"s3://bucket/warehouse"}})", + .expected_model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}}}, + // Null overrides field + CatalogConfigDeserializeParam{ + .test_name = "NullOverrides", + .json_str = + R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":null})", + .expected_model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}}}, + // Missing defaults field + CatalogConfigDeserializeParam{ + .test_name = "MissingDefaults", + .json_str = R"({"overrides":{"clients":"5"}})", + .expected_model = {.overrides = {{"clients", "5"}}}}, + // Null defaults field + CatalogConfigDeserializeParam{ + .test_name = "NullDefaults", + .json_str = R"({"defaults":null,"overrides":{"clients":"5"}})", + .expected_model = {.overrides = {{"clients", "5"}}}}, + // Empty JSON object + CatalogConfigDeserializeParam{ + .test_name = "EmptyJson", .json_str = R"({})", .expected_model = {}}, + // Both fields null + CatalogConfigDeserializeParam{.test_name = "BothNull", + .json_str = R"({"defaults":null,"overrides":null})", + .expected_model = {}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_INVALID_TEST(CatalogConfig) + +INSTANTIATE_TEST_SUITE_P( + CatalogConfigInvalidCases, CatalogConfigInvalidTest, + ::testing::Values( + // Defaults has wrong type (array instead of object) + CatalogConfigInvalidParam{ + .test_name = "WrongDefaultsType", + .invalid_json_str = + R"({"defaults":["warehouse","s3://bucket/warehouse"],"overrides":{"clients":"5"}})", + .expected_error_message = "type must be object, but is array"}, + // Overrides has wrong type (string instead of object) + CatalogConfigInvalidParam{ + .test_name = "WrongOverridesType", + .invalid_json_str = + R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":"clients"})", + .expected_error_message = "type must be object, but is string"}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_ROUNDTRIP_TEST(ErrorResponse) + +INSTANTIATE_TEST_SUITE_P( + ErrorResponseCases, ErrorResponseTest, + ::testing::Values( + // Error without stack trace + ErrorResponseParam{ + .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}}}, + // 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"}}}}, + // 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"}}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_DESERIALIZE_TEST(ErrorResponse) + +INSTANTIATE_TEST_SUITE_P( + ErrorResponseDeserializeCases, ErrorResponseDeserializeTest, + ::testing::Values( + // Stack field is null (should deserialize to empty vector) + ErrorResponseDeserializeParam{ + .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}}}, + // 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}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_INVALID_TEST(ErrorResponse) + +INSTANTIATE_TEST_SUITE_P( + ErrorResponseInvalidCases, ErrorResponseInvalidTest, + ::testing::Values( + // Missing error field + ErrorResponseInvalidParam{.test_name = "MissingError", + .invalid_json_str = R"({})", + .expected_error_message = "Missing 'error'"}, + // 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; + }); + +// ErrorModel tests - testing the nested error model structure +DECLARE_INVALID_TEST(ErrorModel) + +INSTANTIATE_TEST_SUITE_P( + ErrorModelInvalidCases, ErrorModelInvalidTest, + ::testing::Values( + // Missing required type field + ErrorModelInvalidParam{ + .test_name = "MissingType", + .invalid_json_str = + R"({"message":"The given namespace does not exist","code":404})", + .expected_error_message = "Missing 'type'"}, + // Missing required code field + ErrorModelInvalidParam{ + .test_name = "MissingCode", + .invalid_json_str = + R"({"message":"The given namespace does not exist","type":"NoSuchNamespaceException"})", + .expected_error_message = "Missing 'code'"}, + // Wrong type for message field + ErrorModelInvalidParam{ + .test_name = "WrongMessageType", + .invalid_json_str = + R"({"message":123,"type":"NoSuchNamespaceException","code":404})", + .expected_error_message = "type must be string, but is number"}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + } // namespace iceberg::rest From e25ff49cffa49e5d3e95a90ed7bc02be4b79e49f Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Wed, 5 Nov 2025 17:00:16 +0800 Subject: [PATCH 2/3] fix review and ci --- src/iceberg/catalog/rest/json_internal.cc | 14 ++-- src/iceberg/catalog/rest/types.h | 4 +- src/iceberg/catalog/rest/validator.cc | 77 +++++++++++---------- src/iceberg/catalog/rest/validator.h | 5 ++ src/iceberg/test/meson.build | 7 ++ src/iceberg/test/rest_json_internal_test.cc | 1 - 6 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/iceberg/catalog/rest/json_internal.cc b/src/iceberg/catalog/rest/json_internal.cc index 85f7e8ca8..404fe21bf 100644 --- a/src/iceberg/catalog/rest/json_internal.cc +++ b/src/iceberg/catalog/rest/json_internal.cc @@ -20,7 +20,6 @@ #include "iceberg/catalog/rest/json_internal.h" #include -#include #include #include @@ -75,9 +74,7 @@ nlohmann::json ToJson(const CatalogConfig& config) { nlohmann::json json; json[kOverrides] = config.overrides; json[kDefaults] = config.defaults; - if (!config.endpoints.empty()) { - json[kEndpoints] = config.endpoints; - } + SetContainerField(json, kEndpoints, config.endpoints); return json; } @@ -100,17 +97,18 @@ nlohmann::json ToJson(const ErrorModel& error) { json[kMessage] = error.message; json[kType] = error.type; json[kCode] = error.code; - if (!error.stack.empty()) { - json[kStack] = error.stack; - } + SetContainerField(json, kStack, error.stack); return json; } Result ErrorModelFromJson(const nlohmann::json& json) { ErrorModel 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.code, GetJsonValue(json, kCode)); ICEBERG_ASSIGN_OR_RAISE(error.stack, GetJsonValueOrDefault>(json, kStack)); ICEBERG_RETURN_UNEXPECTED(Validator::Validate(error)); diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index bc3a734fe..15e9831b8 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -35,8 +35,8 @@ namespace iceberg::rest { /// \brief Server-provided configuration for the catalog. struct ICEBERG_REST_EXPORT CatalogConfig { - std::unordered_map overrides; // required std::unordered_map defaults; // required + std::unordered_map overrides; // required std::vector endpoints; }; @@ -44,7 +44,7 @@ struct ICEBERG_REST_EXPORT CatalogConfig { struct ICEBERG_REST_EXPORT ErrorModel { std::string message; // required std::string type; // required - uint16_t code; // required + uint32_t code; // required std::vector stack; }; diff --git a/src/iceberg/catalog/rest/validator.cc b/src/iceberg/catalog/rest/validator.cc index 5e8749f00..de25fa334 100644 --- a/src/iceberg/catalog/rest/validator.cc +++ b/src/iceberg/catalog/rest/validator.cc @@ -19,13 +19,13 @@ #include "iceberg/catalog/rest/validator.h" +#include #include -#include -#include -#include #include "iceberg/catalog/rest/types.h" #include "iceberg/result.h" +#include "iceberg/util/formatter_internal.h" +#include "iceberg/util/macros.h" namespace iceberg::rest { @@ -40,18 +40,20 @@ Status Validator::Validate(const CatalogConfig& config) { } Status Validator::Validate(const ErrorModel& error) { - if (error.message.empty() || error.type.empty()) [[unlikely]] { + if (error.message.empty() || error.type.empty()) { return Invalid("Invalid error model: missing required fields"); } - if (error.code < 400 || error.code > 600) [[unlikely]] { - return Invalid("Invalid error model: code must be between 400 and 600"); + if (error.code < 400 || error.code > 600) { + return Invalid("Invalid error model: code {} is out of range [400, 600]", error.code); } // stack is optional, no validation needed return {}; } +// We don't validate the error field because ErrorModel::Validate has been called in the +// FromJson. Status Validator::Validate(const ErrorResponse& response) { return {}; } // Namespace operations @@ -66,32 +68,33 @@ Status Validator::Validate(const GetNamespaceResponse& response) { return {}; } Status Validator::Validate(const UpdateNamespacePropertiesRequest& request) { // keys in updates and removals must not overlap - if (request.removals.empty() || request.updates.empty()) [[unlikely]] { + if (request.removals.empty() || request.updates.empty()) { return {}; } - std::unordered_set remove_set(request.removals.begin(), - request.removals.end()); - std::vector common; - - for (const std::string& k : request.updates | std::views::keys) { - if (remove_set.contains(k)) { - common.push_back(k); + auto extract_and_sort = [](const auto& container, auto key_extractor) { + std::vector result; + result.reserve(container.size()); + for (const auto& item : container) { + result.push_back(std::string_view{key_extractor(item)}); } - } + std::ranges::sort(result); + return result; + }; - if (!common.empty()) { - std::string keys; - bool first = true; - for (const std::string& s : common) { - if (!std::exchange(first, false)) keys += ", "; - keys += s; - } + auto sorted_removals = + extract_and_sort(request.removals, [](const auto& s) -> const auto& { return s; }); + auto sorted_update_keys = extract_and_sort( + request.updates, [](const auto& pair) -> const auto& { return pair.first; }); + + std::vector common; + std::ranges::set_intersection(sorted_removals, sorted_update_keys, + std::back_inserter(common)); + if (!common.empty()) { return Invalid( - "Invalid namespace properties update: cannot simultaneously set and remove keys: " - "[{}]", - keys); + "Invalid namespace update: cannot simultaneously set and remove keys: {}", + common); } return {}; } @@ -105,34 +108,34 @@ Status Validator::Validate(const UpdateNamespacePropertiesResponse& response) { Status Validator::Validate(const ListTablesResponse& response) { return {}; } Status Validator::Validate(const LoadTableResult& result) { - if (!result.metadata) [[unlikely]] { + if (!result.metadata) { return Invalid("Invalid metadata: null"); } return {}; } Status Validator::Validate(const RegisterTableRequest& request) { - if (request.name.empty()) [[unlikely]] { - return Invalid("Invalid table name: empty"); + if (request.name.empty()) { + return Invalid("Missing table name"); } - if (request.metadata_location.empty()) [[unlikely]] { - return Invalid("Invalid metadata location: empty"); + if (request.metadata_location.empty()) { + return Invalid("Empty metadata location"); } return {}; } Status Validator::Validate(const RenameTableRequest& request) { - if (request.source.ns.levels.empty() || request.source.name.empty()) [[unlikely]] { - return Invalid("Invalid source identifier"); - } + ICEBERG_RETURN_UNEXPECTED(Validate(request.source)); + ICEBERG_RETURN_UNEXPECTED(Validate(request.destination)); + return {}; +} - if (request.destination.ns.levels.empty() || request.destination.name.empty()) - [[unlikely]] { - return Invalid("Invalid destination identifier"); +Status Validator::Validate(const TableIdentifier& identifier) { + if (identifier.name.empty()) { + return Invalid("Invalid table identifier: missing table name"); } - return {}; } diff --git a/src/iceberg/catalog/rest/validator.h b/src/iceberg/catalog/rest/validator.h index 44c445eb0..2c80cabc0 100644 --- a/src/iceberg/catalog/rest/validator.h +++ b/src/iceberg/catalog/rest/validator.h @@ -77,6 +77,11 @@ class ICEBERG_REST_EXPORT Validator { /// \brief Validates a RenameTableRequest object. static Status Validate(const RenameTableRequest& request); + + // Other types + + /// \brief Validates a TableIdentifier object. + static Status Validate(const TableIdentifier& identifier); }; } // namespace iceberg::rest diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 22ed4bdde..647997882 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -96,6 +96,12 @@ if get_option('rest').enabled() } endif +cpp = meson.get_compiler('cpp') +cpp_bigobj_args = [] +if cpp.get_id() == 'msvc' + cpp_bigobj_args += cpp.get_supported_arguments(['/bigobj']) +endif + foreach test_name, values : iceberg_tests exc = executable( test_name, @@ -104,6 +110,7 @@ foreach test_name, values : iceberg_tests 'dependencies', [], ), + cpp_args: cpp_bigobj_args, ) test(test_name, exc) endforeach diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index 24be1bebf..2ea0c021a 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -940,7 +940,6 @@ INSTANTIATE_TEST_SUITE_P( return info.param.test_name; }); -// ErrorModel tests - testing the nested error model structure DECLARE_INVALID_TEST(ErrorModel) INSTANTIATE_TEST_SUITE_P( From 9752f91b3175abd06e1a39e4d5edade7c22b4fd9 Mon Sep 17 00:00:00 2001 From: Li Feiyang Date: Thu, 6 Nov 2025 10:35:50 +0800 Subject: [PATCH 3/3] modify meson --- meson.build | 4 ++++ src/iceberg/test/meson.build | 7 ------- src/iceberg/test/rest_json_internal_test.cc | 9 +++++---- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/meson.build b/meson.build index eaeeda4c2..68dcaa0e4 100644 --- a/meson.build +++ b/meson.build @@ -30,6 +30,10 @@ project( ], ) +cpp = meson.get_compiler('cpp') +args = cpp.get_supported_arguments(['/bigobj']) +add_project_arguments(args, language: 'cpp') + subdir('src') install_data( diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 647997882..22ed4bdde 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -96,12 +96,6 @@ if get_option('rest').enabled() } endif -cpp = meson.get_compiler('cpp') -cpp_bigobj_args = [] -if cpp.get_id() == 'msvc' - cpp_bigobj_args += cpp.get_supported_arguments(['/bigobj']) -endif - foreach test_name, values : iceberg_tests exc = executable( test_name, @@ -110,7 +104,6 @@ foreach test_name, values : iceberg_tests 'dependencies', [], ), - cpp_args: cpp_bigobj_args, ) test(test_name, exc) endforeach diff --git a/src/iceberg/test/rest_json_internal_test.cc b/src/iceberg/test/rest_json_internal_test.cc index 2ea0c021a..eba669add 100644 --- a/src/iceberg/test/rest_json_internal_test.cc +++ b/src/iceberg/test/rest_json_internal_test.cc @@ -770,8 +770,8 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "FullConfig", .expected_json_str = R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":{"clients":"5"}})", - .model = {.overrides = {{"clients", "5"}}, - .defaults = {{"warehouse", "s3://bucket/warehouse"}}}}, + .model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}, + .overrides = {{"clients", "5"}}}}, // Only defaults CatalogConfigParam{ .test_name = "OnlyDefaults", @@ -792,8 +792,9 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "WithEndpoints", .expected_json_str = R"({"defaults":{"warehouse":"s3://bucket/warehouse"},"overrides":{"clients":"5"},"endpoints":["GET /v1/config","POST /v1/tables"]})", - .model = {.overrides = {{"clients", "5"}}, - .defaults = {{"warehouse", "s3://bucket/warehouse"}}, + .model = {.defaults = {{"warehouse", "s3://bucket/warehouse"}}, + .overrides = {{"clients", "5"}}, + .endpoints = {"GET /v1/config", "POST /v1/tables"}}}, // Only endpoints CatalogConfigParam{