class RestClient C.41 conformance. (#4994)

The centerpiece of this PR is to bring `class RestClient` into
conformance with C.41. Full initialization of that class was happening
not only in its `init()` member function but also in
`StorageManager::set_default_tags`. The PR removes both of these
functions and moves their behavior to `RestClient::RestClient`, which
has a new signature and much-expanded body.

An interrelated goal is to extricate `class StorageManager` from the
implementation of C API function `tiledb_ctx_set_tag`; the new
implementation calls `RestClient` directly. Not only was initialization
for this happening outside of `RestClient`, but the underlying data
structure was duplicated in `StorageManager` and then only used to run a
few tests. This PR removes the relevant member functions and variables
from `class StorageManager`, reworks the C API function to call
`RestClient` directly, adds `RestClient::extra_headers()` to replace
`StorageManager::tags()`, and reworks the C API tests accordingly.

This PR has a bit of ancillary support work. It adds
`Context::has_rest_client()` and adds a proxy for this and
`Context::rest_client()` in the C API handle class for `class Context`.
It updates use of `Config::get` to non-deprecated varieties.

This PR remove all preprocessor use of the configuration variable
`TILEDB_SERIALIZATION` from `class RestClient`. Instead of using the
preprocessor, there is now a server-disabled base class and a
server-enabled derived class. The server-enabled class is now declared
and defined in separate translation units. `TILEDB_SERIALIZATION` now
affects what sources files are compiled with ordinary build
dependencies.

The only remaining involvement of `class StorageManager` with `class
RestClient` is an accessor to the `RestClient` instance (currently) held
within its `ContextResources` member variable. Removing this accessor is
out of scope for this PR.

---
TYPE: NO_HISTORY
DESC: `class RestClient` C.41 conformance.

---------

Co-authored-by: KiterLuc <[email protected]>

Author: eric-hughes-tiledb

CMakeLists.txt

@@ -384,28 +384,28 @@ set(TILEDB_EXPORT_HEADER "${TILEDB_EXPORT_HEADER_DIR}/${TILEDB_EXPORT_HEADER_NAM
 set(TILEDB_EXPORT_HEADER_LOCALINSTALL_PATH "tiledb/api/c_api/${TILEDB_EXPORT_HEADER_NAME}")
 
 ##################################
-# VFS Object Storage Configuration
+# Library Configuration
 ##################################
-# Create an interface library for use in passing the
-# appropriate compiler definitions for the set of
-# configured object storage backends.
+#
+# `configuration_definitions` is an interface library that contains compiler
+# definitions corresponding to the configuration variables passed to CMake
 
-add_library(object_store_definitions INTERFACE)
+add_library(configuration_definitions INTERFACE)
 
 if (TILEDB_AZURE)
-  target_compile_definitions(object_store_definitions INTERFACE -DHAVE_AZURE)
+  target_compile_definitions(configuration_definitions INTERFACE -DHAVE_AZURE)
 endif()
 
 if (TILEDB_GCS)
-  target_compile_definitions(object_store_definitions INTERFACE -DHAVE_GCS)
+  target_compile_definitions(configuration_definitions INTERFACE -DHAVE_GCS)
 endif()
 
 if (TILEDB_HDFS)
-  target_compile_definitions(object_store_definitions INTERFACE -DHAVE_HDFS)
+  target_compile_definitions(configuration_definitions INTERFACE -DHAVE_HDFS)
 endif()
 
 if (TILEDB_S3)
-  target_compile_definitions(object_store_definitions INTERFACE -DHAVE_S3)
+  target_compile_definitions(configuration_definitions INTERFACE -DHAVE_S3)
 endif()
 
 ##################################

cmake/object_library.cmake

@@ -178,6 +178,11 @@ macro(TileDB_Environment_object_library_end)
     foreach(Compile_Definition IN LISTS TileDB_Environment_object_library_end_Compile_Definitions)
         target_compile_definitions(${TileDB_Environment_object_library_end_Library} PUBLIC ${Compile_Definition})
     endforeach()
+    #
+    # All object libraries ought to depend upon the `configuration_definitions`
+    # library, but at present it's premature for that
+    #
+    #target_link_libraries(${TileDB_Environment_object_library_end_Library} PUBLIC configuration_definitions)
 
     # Compile test
     add_executable(${TileDB_Environment_object_library_end_Compile} EXCLUDE_FROM_ALL)

test/CMakeLists.txt

@@ -302,6 +302,7 @@ target_link_libraries(tiledb_unit
     TILEDB_CORE_OBJECTS
     Catch2::Catch2
     tiledb_test_support_lib
+    configuration_definitions
 )
 
 target_link_libraries(tiledb_unit PRIVATE $<BUILD_INTERFACE:common>)

test/src/unit-ctx.cc

@@ -43,56 +43,63 @@
 TEST_CASE("C API: Test context tags", "[capi][ctx-tags]") {
   tiledb_ctx_t* ctx = nullptr;
   REQUIRE(tiledb_ctx_alloc(nullptr, &ctx) == TILEDB_OK);
-  tiledb::sm::StorageManager* sm = ctx->storage_manager();
 
-  // Check defaults
-  REQUIRE(sm->tags().size() == 2);
-  REQUIRE(sm->tags().at("x-tiledb-api-language") == "c");
-  REQUIRE(
-      sm->tags().at("x-tiledb-version") ==
-      (std::to_string(tiledb::sm::constants::library_version[0]) + "." +
-       std::to_string(tiledb::sm::constants::library_version[1]) + "." +
-       std::to_string(tiledb::sm::constants::library_version[2])));
+  // Only run these tests if the rest client has been initialized
+  if (ctx->has_rest_client()) {
+    auto& rest_client{ctx->rest_client()};
+    SECTION("Defaults") {
+      REQUIRE(rest_client.extra_headers().size() == 2);
+      REQUIRE(rest_client.extra_headers().at("x-tiledb-api-language") == "c");
+      REQUIRE(
+          rest_client.extra_headers().at("x-tiledb-version") ==
+          (std::to_string(tiledb::sm::constants::library_version[0]) + "." +
+           std::to_string(tiledb::sm::constants::library_version[1]) + "." +
+           std::to_string(tiledb::sm::constants::library_version[2])));
+    }
+    SECTION("tiledb_ctx_set_tag") {
+      REQUIRE(tiledb_ctx_set_tag(ctx, "tag1", "value1") == TILEDB_OK);
+      REQUIRE(rest_client.extra_headers().size() == 3);
+      REQUIRE(rest_client.extra_headers().at("tag1") == "value1");
 
-  // Check setter
-  REQUIRE(tiledb_ctx_set_tag(ctx, "tag1", "value1") == TILEDB_OK);
-  REQUIRE(sm->tags().size() == 3);
-  REQUIRE(sm->tags().at("tag1") == "value1");
-
-  REQUIRE(tiledb_ctx_set_tag(ctx, "tag2", "value2") == TILEDB_OK);
-  REQUIRE(sm->tags().size() == 4);
-  REQUIRE(sm->tags().at("tag2") == "value2");
-
-  REQUIRE(tiledb_ctx_set_tag(ctx, "tag1", "value3") == TILEDB_OK);
-  REQUIRE(sm->tags().size() == 4);
-  REQUIRE(sm->tags().at("tag1") == "value3");
+      REQUIRE(tiledb_ctx_set_tag(ctx, "tag2", "value2") == TILEDB_OK);
+      REQUIRE(rest_client.extra_headers().size() == 4);
+      REQUIRE(rest_client.extra_headers().at("tag2") == "value2");
 
+      REQUIRE(tiledb_ctx_set_tag(ctx, "tag1", "value3") == TILEDB_OK);
+      REQUIRE(rest_client.extra_headers().size() == 4);
+      REQUIRE(rest_client.extra_headers().at("tag1") == "value3");
+    }
+  }
   tiledb_ctx_free(&ctx);
 }
 
 TEST_CASE("C++ API: Test context tags", "[cppapi][ctx-tags]") {
   tiledb::Context ctx;
-  tiledb::sm::StorageManager* sm = ctx.ptr().get()->storage_manager();
-
-  // Check defaults
-  REQUIRE(sm->tags().size() == 2);
-  REQUIRE(sm->tags().at("x-tiledb-api-language") == "c++");
-  REQUIRE(
-      sm->tags().at("x-tiledb-version") ==
-      (std::to_string(tiledb::sm::constants::library_version[0]) + "." +
-       std::to_string(tiledb::sm::constants::library_version[1]) + "." +
-       std::to_string(tiledb::sm::constants::library_version[2])));
 
-  // Check setter
-  REQUIRE_NOTHROW(ctx.set_tag("tag1", "value1"));
-  REQUIRE(sm->tags().size() == 3);
-  REQUIRE(sm->tags().at("tag1") == "value1");
+  // Only run these tests if the rest client has been initialized
+  if (ctx.ptr().get()->has_rest_client()) {
+    auto& rest_client{ctx.ptr().get()->rest_client()};
+    SECTION("Defaults") {
+      REQUIRE(rest_client.extra_headers().size() == 2);
+      REQUIRE(rest_client.extra_headers().at("x-tiledb-api-language") == "c++");
+      REQUIRE(
+          rest_client.extra_headers().at("x-tiledb-version") ==
+          (std::to_string(tiledb::sm::constants::library_version[0]) + "." +
+           std::to_string(tiledb::sm::constants::library_version[1]) + "." +
+           std::to_string(tiledb::sm::constants::library_version[2])));
+    }
+    SECTION("tiledb_ctx_set_tag") {
+      REQUIRE_NOTHROW(ctx.set_tag("tag1", "value1"));
+      REQUIRE(rest_client.extra_headers().size() == 3);
+      REQUIRE(rest_client.extra_headers().at("tag1") == "value1");
 
-  REQUIRE_NOTHROW(ctx.set_tag("tag2", "value2"));
-  REQUIRE(sm->tags().size() == 4);
-  REQUIRE(sm->tags().at("tag2") == "value2");
+      REQUIRE_NOTHROW(ctx.set_tag("tag2", "value2"));
+      REQUIRE(rest_client.extra_headers().size() == 4);
+      REQUIRE(rest_client.extra_headers().at("tag2") == "value2");
 
-  REQUIRE_NOTHROW(ctx.set_tag("tag1", "value3"));
-  REQUIRE(sm->tags().size() == 4);
-  REQUIRE(sm->tags().at("tag1") == "value3");
+      REQUIRE_NOTHROW(ctx.set_tag("tag1", "value3"));
+      REQUIRE(rest_client.extra_headers().size() == 4);
+      REQUIRE(rest_client.extra_headers().at("tag1") == "value3");
+    }
+  }
 }

test/src/unit-curl.cc

@@ -117,16 +117,13 @@ TEST_CASE(
   ThreadPool tp{1};
   ContextResources resources(
       cfg, tiledb::test::g_helper_logger(), 1, 1, "test");
-  tiledb::sm::RestClient rest_client;
-  REQUIRE(rest_client
-              .init(
-                  &tiledb::test::g_helper_stats,
-                  &cfg,
-                  &tp,
-                  tiledb::test::g_helper_logger(),
-                  resources)
-              .ok());
-  CHECK(rest_client.rest_server() == "http://localhost:8080");
+  auto rest_client{tiledb::sm::RestClientFactory::make(
+      tiledb::test::g_helper_stats,
+      cfg,
+      tp,
+      *tiledb::test::g_helper_logger().get(),
+      resources.create_memory_tracker())};
+  CHECK(rest_client->rest_server() == "http://localhost:8080");
 }
 
 TEST_CASE(

tiledb/CMakeLists.txt

@@ -322,8 +322,6 @@ set(TILEDB_CORE_SOURCES
 list(APPEND TILEDB_CORE_SOURCES ${TILEDB_COMMON_SOURCES})
 list(APPEND TILEDB_CORE_SOURCES ${TILEDB_API_SOURCES})
 
-
-
 #openssl3 md5 deprecation mitigation
 if(MSVC)
   set_source_files_properties(${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/crypto/crypto_openssl.cc PROPERTIES COMPILE_OPTIONS "/wd4996")
@@ -334,11 +332,12 @@ endif()
 if (TILEDB_SERIALIZATION)
   list(APPEND TILEDB_CORE_SOURCES
     ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/curl.cc
+    ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/rest_client_remote.cc
   )
 
   if(NOT WIN32)
     set_source_files_properties(
-      ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/rest_client.cc
+      ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/rest_client_remote.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array_directory.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array_schema.cc
@@ -362,7 +361,7 @@ if (TILEDB_SERIALIZATION)
   endif()
   if(MSVC)
     set_source_files_properties(
-      ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/rest_client.cc
+      ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/rest/rest_client_remote.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array_directory.cc
       ${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/serialization/array_schema.cc
@@ -440,7 +439,7 @@ target_link_libraries(TILEDB_CORE_OBJECTS
     ${TILEDB_CORE_OBJECTS_LIBS}
 )
 
-target_link_libraries(TILEDB_CORE_OBJECTS INTERFACE object_store_definitions)
+target_link_libraries(TILEDB_CORE_OBJECTS INTERFACE configuration_definitions)
 
 ############################################################
 # provide actions/target for preparation of magic.mgc data for embedding/build
@@ -503,7 +502,7 @@ endif()
 add_library(TILEDB_CORE_OBJECTS_ILIB INTERFACE)
 
 # object store definitions
-target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE object_store_definitions)
+target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE configuration_definitions)
 
 # blosc link dependencies
 target_link_libraries(TILEDB_CORE_OBJECTS_ILIB INTERFACE ${TileDB_blosc_LINK_OPTIONS})
@@ -938,8 +937,8 @@ set(TILEDB_INSTALL_TARGETS
 )
 if (NOT BUILD_SHARED_LIBS)
   list(APPEND TILEDB_INSTALL_TARGETS
-    TILEDB_CORE_OBJECTS_ILIB
-    object_store_definitions
+      TILEDB_CORE_OBJECTS_ILIB
+      configuration_definitions
   )
 endif()
 

tiledb/api/c_api/context/CMakeLists.txt

@@ -41,9 +41,9 @@ list(APPEND OTHER_SOURCES
 commence(object_library capi_context_stub)
   this_target_sources(${SOURCES} ${OTHER_SOURCES})
   this_target_link_libraries(export)
-  this_target_link_libraries(storage_manager_stub)
   this_target_object_libraries(capi_config_stub)
-  this_target_object_libraries(vfs stats thread_pool)
+  this_target_object_libraries(storage_manager_stub)
+  this_target_object_libraries(rest_client stats thread_pool vfs)
 conclude(object_library)
 
 add_test_subdirectory()

tiledb/api/c_api/context/context_api.cc

@@ -34,6 +34,7 @@
 #include "context_api_external.h"
 #include "context_api_internal.h"
 #include "tiledb/api/c_api_support/c_api_support.h"
+#include "tiledb/sm/rest/rest_client.h"
 
 namespace tiledb::api {
 
@@ -121,12 +122,12 @@ capi_return_t tiledb_ctx_cancel_tasks(tiledb_ctx_t* ctx) {
 capi_return_t tiledb_ctx_set_tag(
     tiledb_ctx_t* ctx, const char* key, const char* value) {
   if (key == nullptr) {
-    throw CAPIStatusException("tiledb_ctx_set_tag: key may not be null");
+    throw CAPIException("tiledb_ctx_set_tag: key may not be null");
   }
   if (value == nullptr) {
-    throw CAPIStatusException("tiledb_ctx_set_tag: value may not be null");
+    throw CAPIException("tiledb_ctx_set_tag: value may not be null");
   }
-  throw_if_not_ok(ctx->storage_manager()->set_tag(key, value));
+  ctx->rest_client().add_header(key, value);
   return TILEDB_OK;
 }
 

tiledb/api/c_api/context/context_api_internal.h

@@ -71,6 +71,14 @@ struct tiledb_ctx_handle_t
     return ctx_.storage_manager();
   }
 
+  inline tiledb::sm::RestClient& rest_client() {
+    return ctx_.rest_client();
+  }
+
+  inline bool has_rest_client() {
+    return ctx_.has_rest_client();
+  }
+
   inline optional<std::string> last_error() {
     return ctx_.last_error();
   }

tiledb/api/c_api_support/exception_wrapper/test/CMakeLists.txt

@@ -77,6 +77,7 @@ commence(unit_test capi_ew_with_hook)
   this_target_link_libraries(export)
   this_target_object_libraries(
       exception_wrapper
+      rest_client
       storage_manager_stub
       vfs
   )