diff --git a/src/iceberg/arrow/s3/arrow_s3_file_io.cc b/src/iceberg/arrow/s3/arrow_s3_file_io.cc index 7b9f1d4d6..7ce0d0fda 100644 --- a/src/iceberg/arrow/s3/arrow_s3_file_io.cc +++ b/src/iceberg/arrow/s3/arrow_s3_file_io.cc @@ -37,6 +37,7 @@ #include "iceberg/arrow/s3/s3_properties.h" #include "iceberg/logging/log_macros.h" #include "iceberg/util/macros.h" +#include "iceberg/util/property_util.h" #include "iceberg/util/string_util.h" namespace iceberg::arrow { @@ -52,22 +53,6 @@ const std::string* FindProperty( return it == properties.end() ? nullptr : &it->second; } -Result> ParseOptionalBool( - const std::unordered_map& properties, - std::string_view key) { - const auto* value = FindProperty(properties, key); - if (value == nullptr) { - return std::nullopt; - } - if (StringUtils::EqualsIgnoreCase(*value, "true")) { - return true; - } - if (StringUtils::EqualsIgnoreCase(*value, "false")) { - return false; - } - return InvalidArgument(R"("{}" must be "true" or "false")", key); -} - Status EnsureS3Initialized() { static const ::arrow::Status init_status = []() { auto options = ::arrow::fs::S3GlobalOptions::Defaults(); @@ -136,15 +121,17 @@ Result<::arrow::fs::S3Options> ConfigureS3Options( options.endpoint_override = SplitEndpointScheme(endpoint_env, options); } - ICEBERG_ASSIGN_OR_RAISE(const auto path_style_access, - ParseOptionalBool(properties, S3Properties::kPathStyleAccess)); + // Both boolean properties below read through PropertyUtil, so a value that does not + // spell a boolean reads as false instead of failing the build, as in Java. + const auto path_style_access = + PropertyUtil::PropertyAsOptionalBoolean(properties, S3Properties::kPathStyleAccess); if (path_style_access.has_value()) { options.force_virtual_addressing = !*path_style_access; } // Explicit `s3.ssl.enabled` overrides any endpoint-derived scheme. - ICEBERG_ASSIGN_OR_RAISE(const auto ssl_enabled, - ParseOptionalBool(properties, S3Properties::kSslEnabled)); + const auto ssl_enabled = + PropertyUtil::PropertyAsOptionalBoolean(properties, S3Properties::kSslEnabled); if (ssl_enabled.has_value()) { options.scheme = *ssl_enabled ? "https" : "http"; } diff --git a/src/iceberg/catalog/rest/auth/auth_managers.cc b/src/iceberg/catalog/rest/auth/auth_managers.cc index 6ee2637b3..0e914e6c3 100644 --- a/src/iceberg/catalog/rest/auth/auth_managers.cc +++ b/src/iceberg/catalog/rest/auth/auth_managers.cc @@ -23,6 +23,7 @@ #include "iceberg/catalog/rest/auth/auth_manager_internal.h" #include "iceberg/catalog/rest/auth/auth_properties.h" +#include "iceberg/util/property_util.h" #include "iceberg/util/string_util.h" namespace iceberg::rest::auth { @@ -47,8 +48,8 @@ const std::unordered_set& KnownAuthTypes() std::string InferAuthType( const std::unordered_map& properties) { // Deprecated alias: rest.sigv4-enabled=true forces SigV4. - if (auto it = properties.find(AuthProperties::kSigV4Enabled); - it != properties.end() && StringUtils::EqualsIgnoreCase(it->second, "true")) { + if (PropertyUtil::PropertyAsBoolean(properties, AuthProperties::kSigV4Enabled, + /*default_value=*/false)) { return AuthProperties::kAuthTypeSigV4; } diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 4a4f990ea..9635aebef 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -523,7 +523,7 @@ Result> RestCatalog::MakeTableReporter( const TableIdentifier& identifier, const std::shared_ptr& table_session) const { auto metrics_enabled = config_.Get(RestCatalogProperties::kMetricsReportingEnabled); - if (StringUtils::ToLower(metrics_enabled) == "true" && + if (StringUtils::ParseBoolean(metrics_enabled) && supported_endpoints_.contains(Endpoint::ReportMetrics())) { ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Metrics(identifier)); auto post = [client = client_](const std::string& endpoint, const std::string& body, diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index f23bc9181..6f6ff7603 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -143,6 +143,7 @@ add_iceberg_test(util_test roaring_position_bitmap_test.cc position_delete_index_test.cc position_delete_range_consumer_test.cc + property_util_test.cc resolving_file_io_test.cc retry_util_test.cc string_util_test.cc diff --git a/src/iceberg/test/arrow_s3_file_io_test.cc b/src/iceberg/test/arrow_s3_file_io_test.cc index 721c9ae2b..7235ea367 100644 --- a/src/iceberg/test/arrow_s3_file_io_test.cc +++ b/src/iceberg/test/arrow_s3_file_io_test.cc @@ -245,12 +245,6 @@ TEST_F(ArrowS3FileIOTest, RejectsIncompleteStaticCredentials) { "S3 client access key ID and secret access key must be set")); } -TEST_F(ArrowS3FileIOTest, RejectsInvalidBooleanProperties) { - auto result = - MakeS3FileIO({{std::string(S3Properties::kPathStyleAccess), "not-a-bool"}}); - EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); -} - TEST_F(ArrowS3FileIOTest, ReadWrite) { if (!HasIntegrationEnv()) { GTEST_SKIP() << "Set ICEBERG_TEST_S3_URI to enable S3 IO test"; @@ -401,6 +395,13 @@ TEST_F(ArrowS3FileIOTest, PathStyleAccess) { EXPECT_FALSE(path_style->force_virtual_addressing); } +TEST_F(ArrowS3FileIOTest, InvalidBooleanPropertyReadsAsFalse) { + auto options = + ConfigureS3Options({{std::string(S3Properties::kPathStyleAccess), "not-a-bool"}}); + ASSERT_THAT(options, IsOk()); + EXPECT_TRUE(options->force_virtual_addressing); +} + TEST_F(ArrowS3FileIOTest, Timeouts) { auto result = ConfigureS3Options({{std::string(S3Properties::kConnectTimeoutMs), "5000"}, diff --git a/src/iceberg/test/property_util_test.cc b/src/iceberg/test/property_util_test.cc new file mode 100644 index 000000000..6fefb5625 --- /dev/null +++ b/src/iceberg/test/property_util_test.cc @@ -0,0 +1,69 @@ +/* + * 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/util/property_util.h" + +#include +#include +#include +#include + +#include + +namespace iceberg { + +namespace { + +constexpr std::string_view kKey = "test.enabled"; + +std::unordered_map Properties(std::string_view value) { + return {{std::string(kKey), std::string(value)}}; +} + +} // namespace + +TEST(PropertyUtilTest, BooleanDefaultsWhenPropertyIsAbsent) { + const std::unordered_map empty; + EXPECT_TRUE(PropertyUtil::PropertyAsBoolean(empty, kKey, true)); + EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(empty, kKey, false)); + EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(empty, kKey), std::nullopt); +} + +TEST(PropertyUtilTest, BooleanIgnoresCase) { + for (const auto* value : {"true", "TRUE", "TrUe"}) { + EXPECT_TRUE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, false)) << value; + EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), true) + << value; + } + for (const auto* value : {"false", "FALSE", "FaLsE"}) { + EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, true)) << value; + EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), false) + << value; + } +} + +TEST(PropertyUtilTest, NonBooleanValueReadsAsFalse) { + for (const auto* value : {"", " true", "yes", "1", "ture"}) { + EXPECT_FALSE(PropertyUtil::PropertyAsBoolean(Properties(value), kKey, true)) << value; + EXPECT_EQ(PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey), false) + << value; + } +} + +} // namespace iceberg diff --git a/src/iceberg/test/string_util_test.cc b/src/iceberg/test/string_util_test.cc index 65d5c23cc..5f9a9a1cd 100644 --- a/src/iceberg/test/string_util_test.cc +++ b/src/iceberg/test/string_util_test.cc @@ -32,6 +32,16 @@ TEST(StringUtilsTest, ToLower) { ASSERT_EQ(StringUtils::ToLower("123"), "123"); } +TEST(StringUtilsTest, ParseBoolean) { + for (const auto* value : {"true", "TRUE", "TrUe"}) { + EXPECT_TRUE(StringUtils::ParseBoolean(value)) << value; + } + for (const auto* value : + {"false", "FALSE", "FaLsE", "", " true", "true ", "yes", "1", "ture"}) { + EXPECT_FALSE(StringUtils::ParseBoolean(value)) << value; + } +} + TEST(StringUtilsTest, ToUpper) { ASSERT_EQ(StringUtils::ToUpper("abc"), "ABC"); ASSERT_EQ(StringUtils::ToUpper("A-bC"), "A-BC"); diff --git a/src/iceberg/util/config.h b/src/iceberg/util/config.h index 36e92237d..8f81b6baf 100644 --- a/src/iceberg/util/config.h +++ b/src/iceberg/util/config.h @@ -54,7 +54,7 @@ U DefaultFromString(const std::string& val) { if constexpr (std::is_same_v) { return val; } else if constexpr (std::is_same_v) { - return StringUtils::EqualsIgnoreCase(val, "true"); + return StringUtils::ParseBoolean(val); } else if constexpr ((std::is_signed_v && std::is_integral_v) || std::is_floating_point_v) { ICEBERG_ASSIGN_OR_THROW(auto res, StringUtils::ParseNumber(val)); diff --git a/src/iceberg/util/property_util.cc b/src/iceberg/util/property_util.cc index 32d9e3ba1..7fbd23d44 100644 --- a/src/iceberg/util/property_util.cc +++ b/src/iceberg/util/property_util.cc @@ -20,6 +20,7 @@ #include "iceberg/util/property_util.h" #include +#include #include "iceberg/table_properties.h" #include "iceberg/util/string_util.h" @@ -46,4 +47,20 @@ Status PropertyUtil::ValidateCommitProperties( return {}; } +bool PropertyUtil::PropertyAsBoolean( + const std::unordered_map& properties, std::string_view key, + bool default_value) { + return PropertyAsOptionalBoolean(properties, key).value_or(default_value); +} + +std::optional PropertyUtil::PropertyAsOptionalBoolean( + const std::unordered_map& properties, + std::string_view key) { + auto it = properties.find(std::string(key)); + if (it == properties.end()) { + return std::nullopt; + } + return StringUtils::ParseBoolean(it->second); +} + } // namespace iceberg diff --git a/src/iceberg/util/property_util.h b/src/iceberg/util/property_util.h index c46d3a6ab..72bbf9f04 100644 --- a/src/iceberg/util/property_util.h +++ b/src/iceberg/util/property_util.h @@ -22,7 +22,9 @@ /// \file iceberg/util/property_util.h /// \brief Provide property conversion helpers. +#include #include +#include #include #include "iceberg/iceberg_export.h" @@ -34,6 +36,33 @@ class ICEBERG_EXPORT PropertyUtil { public: static Status ValidateCommitProperties( const std::unordered_map& properties); + + /// \brief Read a boolean property from a property map. + /// + /// Mirrors Java's PropertyUtil.propertyAsBoolean: the value is parsed with + /// StringUtils::ParseBoolean, so anything that is not "true" ignoring case reads as + /// false rather than being rejected. + /// + /// \param properties The property map to read from. + /// \param key The property key. + /// \param default_value Returned when the property is absent. + /// \return The parsed value, or default_value if the property is absent. + static bool PropertyAsBoolean( + const std::unordered_map& properties, + std::string_view key, bool default_value); + + /// \brief Read a boolean property that may be unset. + /// + /// Like PropertyAsBoolean, but returns std::nullopt when the property is absent so + /// callers can distinguish an unset property from an explicit "false". Mirrors Java's + /// PropertyUtil.propertyAsNullableBoolean. + /// + /// \param properties The property map to read from. + /// \param key The property key. + /// \return The parsed value, or std::nullopt if the property is absent. + static std::optional PropertyAsOptionalBoolean( + const std::unordered_map& properties, + std::string_view key); }; } // namespace iceberg diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h index 8202e60a3..2559b47a3 100644 --- a/src/iceberg/util/string_util.h +++ b/src/iceberg/util/string_util.h @@ -168,6 +168,15 @@ class ICEBERG_EXPORT StringUtils { return value; } + /// \brief Parse a boolean from its string representation, ignoring case. + /// + /// Mirrors Iceberg Java's Boolean.parseBoolean, which every boolean property in the + /// Java implementation is read through: "true" in any case reads as true, and every + /// other value reads as false, including "yes", "1" and typos such as "ture". Parsing + /// therefore cannot fail, so a caller that wants to reject a malformed value has to + /// check for it separately. + static bool ParseBoolean(std::string_view str) { return EqualsIgnoreCase(str, "true"); } + private: // ASCII-only case mappings. These avoid std::toupper/std::tolower, which are // locale-dependent and have undefined behavior for negative char values.