From 8046ae2c8a0ee3d1edf888aefea60adb2dbdc23c Mon Sep 17 00:00:00 2001 From: Ypatia Tsavliri Date: Fri, 12 Apr 2024 14:31:23 +0300 Subject: [PATCH] Config serialization should take into account environment variables (#4865) sc-44928 We were getting failures while testing Query v3 , and specifically when setting up the REST-CI environment for it, because the `rest.use_refactored_array_open_and_query_submit` config we were setting was not getting propagated to the REST server. After debugging it seems that Config serialization code was calling `param_values()` method to get the config variables to set in Cap'n'proto, which is not taking into account environment variables. This PR also moves `param_values()` to private to prevent other classes from similar mistakes. `ConfiIter` is the only class that still uses it and I only saw that class being used in `S3Parameters::load_headers`. It's worth investigating further if that's a problem or not. --- TYPE: BUG DESC: Config serialization should take into account environment variables --- test/src/unit-cppapi-config.cc | 22 +++++++++++++++++++++- tiledb/sm/config/config.cc | 11 +++++++++++ tiledb/sm/config/config.h | 18 +++++++++++++++--- tiledb/sm/serialization/config.cc | 5 +++-- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/test/src/unit-cppapi-config.cc b/test/src/unit-cppapi-config.cc index 18504404d55..d84e9996b1b 100644 --- a/test/src/unit-cppapi-config.cc +++ b/test/src/unit-cppapi-config.cc @@ -34,9 +34,24 @@ #include #include "test/support/src/helpers.h" +#include "tiledb/api/c_api/config/config_api_internal.h" #include "tiledb/sm/c_api/tiledb_serialization.h" #include "tiledb/sm/cpp_api/tiledb" +using namespace tiledb::sm; + +class tiledb::sm::WhiteboxConfig { + public: + WhiteboxConfig(tiledb::sm::Config config) + : config_(config){}; + + const std::map& get_all_params() const { + return config_.param_values(); + } + + tiledb::sm::Config config_; +}; + TEST_CASE("C++ API: Config", "[cppapi][config]") { tiledb::Config config; config["foo"] = "bar"; @@ -159,7 +174,12 @@ TEST_CASE("C++ API: Config Serialization", "[cppapi][config][serialization]") { CHECK(rc == TILEDB_OK); tiledb::Config config2(&config2_ptr); - bool config_equal = config1 == config2; + auto cfg1 = config1.ptr().get()->config(); + auto cfg2 = config2.ptr().get()->config(); + // Check that the deserialized config already contains the values set in + // environment variables + bool config_equal = cfg1.get_all_params_from_config_or_env() == + WhiteboxConfig(cfg2).get_all_params(); CHECK(config_equal); // Check for inequality diff --git a/tiledb/sm/config/config.cc b/tiledb/sm/config/config.cc index e669cd02c60..a3434eddceb 100644 --- a/tiledb/sm/config/config.cc +++ b/tiledb/sm/config/config.cc @@ -932,6 +932,17 @@ const char* Config::get_from_config_or_env( return *found ? value_config : ""; } +const std::map +Config::get_all_params_from_config_or_env() const { + std::map values; + bool found = false; + for (const auto& [key, value] : param_values_) { + std::string val = get_from_config_or_env(key, &found); + values.emplace(key, val); + } + return values; +} + template optional Config::get_internal(const std::string& key) const { auto value = get_internal_string(key); diff --git a/tiledb/sm/config/config.h b/tiledb/sm/config/config.h index bd93bce3e8f..6b9f47a1689 100644 --- a/tiledb/sm/config/config.h +++ b/tiledb/sm/config/config.h @@ -61,12 +61,20 @@ using namespace tiledb::common; namespace tiledb::sm { +class WhiteboxConfig; + /** * This class manages the TileDB configuration options. * It is implemented as a simple map from string to string. * Parsing to appropriate types happens on demand. */ class Config { + friend class ConfigIter; + /** + * WhiteboxConfig makes available internals of Config for testing. + */ + friend class WhiteboxConfig; + public: /* ****************************** */ /* CONFIG DEFAULTS */ @@ -698,9 +706,6 @@ class Config { Status get_vector( const std::string& param, std::vector* value, bool* found) const; - /** Returns the param -> value map. */ - const std::map& param_values() const; - /** Gets the set parameters. */ const std::set& set_params() const; @@ -715,6 +720,10 @@ class Config { /** Compares configs for equality. */ bool operator==(const Config& rhs) const; + /** Get all config params taking into account environment variables */ + const std::map get_all_params_from_config_or_env() + const; + private: /* ********************************* */ /* PRIVATE ATTRIBUTES */ @@ -791,6 +800,9 @@ class Config { template optional get_internal_string(const std::string& key) const; + + /** Returns the param -> value map. */ + const std::map& param_values() const; }; /** diff --git a/tiledb/sm/serialization/config.cc b/tiledb/sm/serialization/config.cc index 9b1ff710287..c911a082b09 100644 --- a/tiledb/sm/serialization/config.cc +++ b/tiledb/sm/serialization/config.cc @@ -61,9 +61,10 @@ namespace serialization { Status config_to_capnp( const Config& config, capnp::Config::Builder* config_builder) { - auto entries = config_builder->initEntries(config.param_values().size()); + auto config_params = config.get_all_params_from_config_or_env(); + auto entries = config_builder->initEntries(config_params.size()); uint64_t i = 0; - for (const auto& kv : config.param_values()) { + for (const auto& kv : config_params) { entries[i].setKey(kv.first); entries[i].setValue(kv.second); ++i;