refactor: parse booleans through a single shared helper

Boolean values were parsed ad hoc in four places, in two incompatible
flavors: `arrow_s3_file_io.cc` rejected anything that was not "true" or
"false", while `config.h`, `auth_managers.cc` and `rest_catalog.cc` read
any unrecognized value as false. Each site spelled out the "true"/"false"
literals itself, and `StringUtils` covered only numbers.

Add `StringUtils::ParseBoolean()`, a strict case-insensitive parser that
mirrors `ParseNumber()`, and `PropertyUtil::PropertyAsBoolean()` /
`PropertyUtil::PropertyAsOptionalBoolean()` for the property-map pattern,
with the optional variant distinguishing an unset property from "false".

Route the existing sites through them. The sites that cannot fail keep
their lenient contract via `.value_or(false)`, so behavior is unchanged
and no public header changes shape.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: manuzhang

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<std::optional<bool>> ParseOptionalBool(
-    const std::unordered_map<std::string, std::string>& 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();
@@ -137,14 +122,16 @@ Result<::arrow::fs::S3Options> ConfigureS3Options(
   }
 
   ICEBERG_ASSIGN_OR_RAISE(const auto path_style_access,
-                          ParseOptionalBool(properties, S3Properties::kPathStyleAccess));
+                          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));
+  ICEBERG_ASSIGN_OR_RAISE(
+      const auto ssl_enabled,
+      PropertyUtil::PropertyAsOptionalBoolean(properties, S3Properties::kSslEnabled));
   if (ssl_enabled.has_value()) {
     options.scheme = *ssl_enabled ? "https" : "http";
   }

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 {
@@ -46,9 +47,11 @@ const std::unordered_set<std::string, StringHash, StringEqual>& KnownAuthTypes()
 // Infer the authentication type from properties.
 std::string InferAuthType(
     const std::unordered_map<std::string, std::string>& properties) {
-  // Deprecated alias: rest.sigv4-enabled=true forces SigV4.
-  if (auto it = properties.find(AuthProperties::kSigV4Enabled);
-      it != properties.end() && StringUtils::EqualsIgnoreCase(it->second, "true")) {
+  // Deprecated alias: rest.sigv4-enabled=true forces SigV4. A value that is not a
+  // boolean reads as false rather than being rejected, since inference cannot fail.
+  auto sigv4_enabled = PropertyUtil::PropertyAsBoolean(
+      properties, AuthProperties::kSigV4Enabled, /*default_value=*/false);
+  if (sigv4_enabled.value_or(false)) {
     return AuthProperties::kAuthTypeSigV4;
   }
 

src/iceberg/catalog/rest/rest_catalog.cc

@@ -523,7 +523,9 @@ Result<std::shared_ptr<MetricsReporter>> RestCatalog::MakeTableReporter(
     const TableIdentifier& identifier,
     const std::shared_ptr<auth::AuthSession>& table_session) const {
   auto metrics_enabled = config_.Get(RestCatalogProperties::kMetricsReportingEnabled);
-  if (StringUtils::ToLower(metrics_enabled) == "true" &&
+  // A value that is not a boolean reads as false rather than being rejected, since
+  // building the reporter cannot fail on a malformed property.
+  if (StringUtils::ParseBoolean(metrics_enabled).value_or(false) &&
       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,

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

src/iceberg/test/property_util_test.cc

@@ -0,0 +1,91 @@
+/*
+ * 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 <optional>
+#include <string>
+#include <string_view>
+#include <unordered_map>
+
+#include <gtest/gtest.h>
+
+#include "iceberg/test/matchers.h"
+
+namespace iceberg {
+
+namespace {
+
+constexpr std::string_view kKey = "test.enabled";
+
+std::unordered_map<std::string, std::string> Properties(std::string_view value) {
+  return {{std::string(kKey), std::string(value)}};
+}
+
+Result<bool> PropertyAsBoolean(std::string_view value, bool default_value) {
+  return PropertyUtil::PropertyAsBoolean(Properties(value), kKey, default_value);
+}
+
+Result<std::optional<bool>> PropertyAsOptionalBoolean(std::string_view value) {
+  return PropertyUtil::PropertyAsOptionalBoolean(Properties(value), kKey);
+}
+
+}  // namespace
+
+TEST(PropertyUtilTest, BooleanDefaultsWhenPropertyIsAbsent) {
+  const std::unordered_map<std::string, std::string> empty;
+  EXPECT_THAT(PropertyUtil::PropertyAsBoolean(empty, kKey, true),
+              HasValue(::testing::Eq(true)));
+  EXPECT_THAT(PropertyUtil::PropertyAsBoolean(empty, kKey, false),
+              HasValue(::testing::Eq(false)));
+  EXPECT_THAT(PropertyUtil::PropertyAsOptionalBoolean(empty, kKey),
+              HasValue(::testing::Eq(std::optional<bool>{})));
+}
+
+TEST(PropertyUtilTest, BooleanIgnoresCase) {
+  for (const auto* value : {"true", "TRUE", "TrUe"}) {
+    EXPECT_THAT(PropertyAsBoolean(value, false), HasValue(::testing::Eq(true))) << value;
+    EXPECT_THAT(PropertyAsOptionalBoolean(value),
+                HasValue(::testing::Eq(std::optional<bool>{true})))
+        << value;
+  }
+  for (const auto* value : {"false", "FALSE", "FaLsE"}) {
+    EXPECT_THAT(PropertyAsBoolean(value, true), HasValue(::testing::Eq(false))) << value;
+    EXPECT_THAT(PropertyAsOptionalBoolean(value),
+                HasValue(::testing::Eq(std::optional<bool>{false})))
+        << value;
+  }
+}
+
+TEST(PropertyUtilTest, BooleanRejectsNonBooleanValue) {
+  for (const auto* value : {"", " true", "yes", "1", "maybe"}) {
+    EXPECT_THAT(PropertyAsBoolean(value, true), IsError(ErrorKind::kInvalidArgument))
+        << value;
+    EXPECT_THAT(PropertyAsOptionalBoolean(value), IsError(ErrorKind::kInvalidArgument))
+        << value;
+  }
+}
+
+TEST(PropertyUtilTest, BooleanErrorReportsKeyAndValue) {
+  auto result = PropertyAsBoolean("maybe", /*default_value=*/true);
+  EXPECT_THAT(result, HasErrorMessage("test.enabled"));
+  EXPECT_THAT(result, HasErrorMessage("maybe"));
+}
+
+}  // namespace iceberg

src/iceberg/test/string_util_test.cc

@@ -21,6 +21,8 @@
 
 #include <gtest/gtest.h>
 
+#include "iceberg/test/matchers.h"
+
 namespace iceberg {
 
 TEST(StringUtilsTest, ToLower) {
@@ -32,6 +34,21 @@ TEST(StringUtilsTest, ToLower) {
   ASSERT_EQ(StringUtils::ToLower("123"), "123");
 }
 
+TEST(StringUtilsTest, ParseBoolean) {
+  for (const auto* value : {"true", "TRUE", "TrUe"}) {
+    EXPECT_THAT(StringUtils::ParseBoolean(value), HasValue(::testing::Eq(true))) << value;
+  }
+  for (const auto* value : {"false", "FALSE", "FaLsE"}) {
+    EXPECT_THAT(StringUtils::ParseBoolean(value), HasValue(::testing::Eq(false)))
+        << value;
+  }
+  for (const auto* value : {"", " true", "true ", "yes", "no", "1", "0", "truthy"}) {
+    EXPECT_THAT(StringUtils::ParseBoolean(value), IsError(ErrorKind::kInvalidArgument))
+        << value;
+  }
+  EXPECT_THAT(StringUtils::ParseBoolean("maybe"), HasErrorMessage("'maybe'"));
+}
+
 TEST(StringUtilsTest, ToUpper) {
   ASSERT_EQ(StringUtils::ToUpper("abc"), "ABC");
   ASSERT_EQ(StringUtils::ToUpper("A-bC"), "A-BC");

src/iceberg/util/config.h

@@ -54,7 +54,9 @@ U DefaultFromString(const std::string& val) {
   if constexpr (std::is_same_v<U, std::string>) {
     return val;
   } else if constexpr (std::is_same_v<U, bool>) {
-    return StringUtils::EqualsIgnoreCase(val, "true");
+    // Unlike the numeric conversions, an unrecognized value is not an error here: any
+    // value other than "true" reads as false, matching Java's Boolean.parseBoolean.
+    return StringUtils::ParseBoolean(val).value_or(false);
   } else if constexpr ((std::is_signed_v<U> && std::is_integral_v<U>) ||
                        std::is_floating_point_v<U>) {
     ICEBERG_ASSIGN_OR_THROW(auto res, StringUtils::ParseNumber<U>(val));

src/iceberg/util/property_util.cc

@@ -20,8 +20,10 @@
 #include "iceberg/util/property_util.h"
 
 #include <cstdint>
+#include <string>
 
 #include "iceberg/table_properties.h"
+#include "iceberg/util/macros.h"
 #include "iceberg/util/string_util.h"
 
 namespace iceberg {
@@ -46,4 +48,26 @@ Status PropertyUtil::ValidateCommitProperties(
   return {};
 }
 
+Result<bool> PropertyUtil::PropertyAsBoolean(
+    const std::unordered_map<std::string, std::string>& properties, std::string_view key,
+    bool default_value) {
+  ICEBERG_ASSIGN_OR_RAISE(auto value, PropertyAsOptionalBoolean(properties, key));
+  return value.value_or(default_value);
+}
+
+Result<std::optional<bool>> PropertyUtil::PropertyAsOptionalBoolean(
+    const std::unordered_map<std::string, std::string>& properties,
+    std::string_view key) {
+  auto it = properties.find(std::string(key));
+  if (it == properties.end()) {
+    return std::nullopt;
+  }
+  auto value = StringUtils::ParseBoolean(it->second);
+  if (!value) {
+    return InvalidArgument(R"(Property {} must be "true" or "false", but got {})", key,
+                           it->second);
+  }
+  return *value;
+}
+
 }  // namespace iceberg

src/iceberg/util/property_util.h

@@ -22,7 +22,9 @@
 /// \file iceberg/util/property_util.h
 /// \brief Provide property conversion helpers.
 
+#include <optional>
 #include <string>
+#include <string_view>
 #include <unordered_map>
 
 #include "iceberg/iceberg_export.h"
@@ -34,6 +36,31 @@ class ICEBERG_EXPORT PropertyUtil {
  public:
   static Status ValidateCommitProperties(
       const std::unordered_map<std::string, std::string>& properties);
+
+  /// \brief Read a boolean property from a property map.
+  ///
+  /// The value must be "true" or "false" ignoring case; any other value is an error.
+  ///
+  /// \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 an error if the property is set to a non-boolean.
+  static Result<bool> PropertyAsBoolean(
+      const std::unordered_map<std::string, std::string>& 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".
+  ///
+  /// \param properties The property map to read from.
+  /// \param key The property key.
+  /// \return The parsed value, std::nullopt if the property is absent, or an error if
+  /// the property is set to a non-boolean.
+  static Result<std::optional<bool>> PropertyAsOptionalBoolean(
+      const std::unordered_map<std::string, std::string>& properties,
+      std::string_view key);
 };
 
 }  // namespace iceberg

src/iceberg/util/string_util.h

@@ -168,6 +168,22 @@ class ICEBERG_EXPORT StringUtils {
     return value;
   }
 
+  /// \brief Parse a boolean from its string representation, ignoring case.
+  ///
+  /// Only "true" and "false" are accepted; any other input is an error, mirroring
+  /// ParseNumber. Call sites that must not fail on an unrecognized value, where
+  /// anything other than "true" means false, can use `.value_or(false)`.
+  static Result<bool> ParseBoolean(std::string_view str) {
+    if (EqualsIgnoreCase(str, "true")) {
+      return true;
+    }
+    if (EqualsIgnoreCase(str, "false")) {
+      return false;
+    }
+    return InvalidArgument(
+        R"(Failed to parse bool from string '{}': expected "true" or "false")", str);
+  }
+
  private:
   // ASCII-only case mappings. These avoid std::toupper/std::tolower, which are
   // locale-dependent and have undefined behavior for negative char values.