wgtmac commented on code in PR #292:
URL: https://github.com/apache/iceberg-cpp/pull/292#discussion_r2492982749
##########
src/iceberg/catalog/rest/types.h:
##########
@@ -34,6 +33,26 @@
namespace iceberg::rest {
+/// \brief Server-provided configuration for the catalog.
+struct ICEBERG_REST_EXPORT CatalogConfig {
+ std::unordered_map<std::string, std::string> overrides; // required
+ std::unordered_map<std::string, std::string> defaults; // required
Review Comment:
```suggestion
std::unordered_map<std::string, std::string> defaults; // required
std::unordered_map<std::string, std::string> overrides; // required
```
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -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<CatalogConfig> CatalogConfigFromJson(const nlohmann::json& json) {
+ CatalogConfig config;
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.overrides,
+ GetJsonValueOrDefault<decltype(config.overrides)>(json, kOverrides));
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.defaults, GetJsonValueOrDefault<decltype(config.defaults)>(json,
kDefaults));
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.endpoints,
+ GetJsonValueOrDefault<std::vector<std::string>>(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<ErrorModel> ErrorModelFromJson(const nlohmann::json& json) {
Review Comment:
It seems to be inconsistent with Java where all fields are nullable.
```java
public static ErrorResponse fromJson(JsonNode jsonNode) {
Preconditions.checkArgument(
jsonNode != null && jsonNode.isObject(),
"Cannot parse error response from non-object value: %s",
jsonNode);
Preconditions.checkArgument(jsonNode.has(ERROR), "Cannot parse missing
field: error");
JsonNode error = JsonUtil.get(ERROR, jsonNode);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
Integer code = JsonUtil.getIntOrNull(CODE, error);
List<String> stack = JsonUtil.getStringListOrNull(STACK, error);
return ErrorResponse.builder()
.withMessage(message)
.withType(type)
.responseCode(code)
.withStackTrace(stack)
.build();
}
```
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -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;
+ }
Review Comment:
```suggestion
SetContainerField(json, kEndpoints, config.endpoints);
```
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
Review Comment:
Can we use `std::string_view` instead of copying every key here?
##########
src/iceberg/catalog/rest/types.h:
##########
@@ -34,6 +33,26 @@
namespace iceberg::rest {
+/// \brief Server-provided configuration for the catalog.
+struct ICEBERG_REST_EXPORT CatalogConfig {
+ std::unordered_map<std::string, std::string> overrides; // required
+ std::unordered_map<std::string, std::string> defaults; // required
Review Comment:
Let's switch the order to be more natural.
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
Review Comment:
Perhaps it much cleaner to use `std::set_intersection`?
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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");
Review Comment:
nit: print the code in the message.
##########
src/iceberg/meson.build:
##########
@@ -121,6 +127,7 @@ iceberg_lib = library(
dependencies: iceberg_deps,
include_directories: iceberg_include_dir,
install: true,
+ cpp_args: iceberg_common_cpp_args,
gnu_symbol_visibility: 'inlineshidden',
Review Comment:
```suggestion
gnu_symbol_visibility: 'inlineshidden',
cpp_args: iceberg_common_cpp_args,
```
Let's put similar flags closer
##########
src/iceberg/catalog/rest/types.h:
##########
@@ -34,6 +33,26 @@
namespace iceberg::rest {
+/// \brief Server-provided configuration for the catalog.
+struct ICEBERG_REST_EXPORT CatalogConfig {
+ std::unordered_map<std::string, std::string> overrides; // required
+ std::unordered_map<std::string, std::string> defaults; // required
+ std::vector<std::string> 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
Review Comment:
Why `uint16_t` but not `int32_t`? The spec says `type: integer`.
##########
src/iceberg/meson.build:
##########
@@ -15,6 +15,12 @@
# specific language governing permissions and limitations
# under the License.
+cpp = meson.get_compiler('cpp')
+iceberg_common_cpp_args = []
+if cpp.get_id() == 'msvc'
+ iceberg_common_cpp_args += cpp.get_supported_arguments(['/bigobj'])
+endif
Review Comment:
Is it better to move them to above line 124?
##########
src/iceberg/meson.build:
##########
@@ -15,6 +15,12 @@
# specific language governing permissions and limitations
# under the License.
+cpp = meson.get_compiler('cpp')
+iceberg_common_cpp_args = []
+if cpp.get_id() == 'msvc'
+ iceberg_common_cpp_args += cpp.get_supported_arguments(['/bigobj'])
Review Comment:
Please add some comments on why we need this. BTW, does the issue happen
when compiling the test executable? If yes, perhaps only add this flag to the
test target?
cc @WillAyd
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -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<CatalogConfig> CatalogConfigFromJson(const nlohmann::json& json) {
+ CatalogConfig config;
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.overrides,
+ GetJsonValueOrDefault<decltype(config.overrides)>(json, kOverrides));
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.defaults, GetJsonValueOrDefault<decltype(config.defaults)>(json,
kDefaults));
+ ICEBERG_ASSIGN_OR_RAISE(
+ config.endpoints,
+ GetJsonValueOrDefault<std::vector<std::string>>(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;
+ }
Review Comment:
```suggestion
SetContainerField(json, kStack, error.stack);
```
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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]] {
Review Comment:
```suggestion
if (request.removals.empty() || request.updates.empty()) {
```
The unlikely attribute should not be used like this.
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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 {}; }
Review Comment:
It is worth adding a comment that `We don't validate the error field because
ErrorModel::Validate has been called in the FromJson`.
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
+ request.removals.end());
+ std::vector<std::string> 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: "
Review Comment:
```suggestion
"Invalid namespace update: cannot simultaneously set and remove
keys: "
```
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
+ request.removals.end());
+ std::vector<std::string> 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");
Review Comment:
```suggestion
return Invalid("Empty metadata location");
```
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
+ request.removals.end());
+ std::vector<std::string> 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");
Review Comment:
```suggestion
return Invalid("Missing table name");
```
It is misleading since users may think the table name is called `empty`.
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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<std::string> remove_set(request.removals.begin(),
+ request.removals.end());
+ std::vector<std::string> 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]] {
Review Comment:
How about adding a `Status Validator::Validate(const TableIdentifier&
identifier)`? BTW, I don't think we need to check `ns.levels.empty()`, do we?
It is totally fine to create a table without any namespace.
##########
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 <format>
+#include <ranges>
+#include <unordered_set>
+#include <utility>
+
+#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 {}; }
Review Comment:
Although the Java impl is doing the same thing. However, I think calling
validate on fields recursively is a more robust approach. This is not a strong
opinion. If we finally choose this approach, then we should remove this
`Validator` class and let each object to add a `Validate` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
