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

@@ -34,9 +34,24 @@
 
 #include <test/support/tdb_catch.h>
 #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<std::string, std::string>& 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

tiledb/sm/config/config.cc

@@ -932,6 +932,17 @@ const char* Config::get_from_config_or_env(
   return *found ? value_config : "";
 }
 
+const std::map<std::string, std::string>
+Config::get_all_params_from_config_or_env() const {
+  std::map<std::string, std::string> 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 <class T, bool must_find_>
 optional<T> Config::get_internal(const std::string& key) const {
   auto value = get_internal_string<must_find_>(key);

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<T>* value, bool* found) const;
 
-  /** Returns the param -> value map. */
-  const std::map<std::string, std::string>& param_values() const;
-
   /** Gets the set parameters. */
   const std::set<std::string>& 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<std::string, std::string> get_all_params_from_config_or_env()
+      const;
+
  private:
   /* ********************************* */
   /*         PRIVATE ATTRIBUTES        */
@@ -791,6 +800,9 @@ class Config {
 
   template <bool must_find_>
   optional<std::string> get_internal_string(const std::string& key) const;
+
+  /** Returns the param -> value map. */
+  const std::map<std::string, std::string>& param_values() const;
 };
 
 /**

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;