Repository: man-group/ArcticDB (language: python)

Issue:
Libraries written by 4.5.0 are not backwards compatible for s3 https stоrages
### Describe the bug

Client using 4.5.0 are [no longer writing](https://github.com/man-group/ArcticDB/blob/5dd6a642c3ab70be0ead45665c8cfcfa8cffafa3/cpp/arcticdb/storage/storage_override.hpp#L118) the `https` flag in the library config, which is needed by the older clients to correctly for their http requests.

We need to change it, so:
- the `https`' value is written when creating the library - so the libraries are backwards compatible and can be read by older clients
- the `https`' flag is overridden when the config is read - so the newer clients can read libraries created by 4.5.0

### Steps/Code to Reproduce

```python

ac = Arctic("s3s://...")
lib = ac.create_library("test")
lib.write("sym", pd.DataFrame())

config = ac._library_manager.get_library_config("test")
s3_storage = get_s3_storage_config(config)
assert s3_storage.https
```

### Expected Results

The assert check should succeed as the `https` flag's value should reflect that the library was created with `s3s/https` in mind.

### OS, Python Version and ArcticDB Version

Python: 3.10.14 | packaged by conda-forge | (main, Mar 20 2024, 12:45:18) [GCC 12.3.0]
OS: Linux-5.15.153.1-microsoft-standard-WSL2-x86_64-with-glibc2.35 ArcticDB: 4.5.0

### Backend storage used

S3

### Additional Context

_No response_


Discussion / hints:

Fix patch:
diff --git a/cpp/arcticdb/storage/library_manager.cpp b/cpp/arcticdb/storage/library_manager.cpp
index 8f4e28095f..9302a1f417 100644
--- a/cpp/arcticdb/storage/library_manager.cpp
+++ b/cpp/arcticdb/storage/library_manager.cpp
@@ -30,21 +30,23 @@ const std::string BAD_CONFIG_IN_ATTEMPTED_WRITE = "Attempting to write forbidden
 template<typename T>
 struct StorageVisitor {
     arcticdb::proto::storage::LibraryConfig& lib_cfg_proto;
+    bool override_https;
 
     void operator()(const T& storage_override) {
         for(auto& storage: *lib_cfg_proto.mutable_storage_by_id()){
-            storage_override.modify_storage_config(storage.second);
+            storage_override.modify_storage_config(storage.second, override_https);
         }
     }
 };
 
 void apply_storage_override(const StorageOverride& storage_override,
-                                   arcticdb::proto::storage::LibraryConfig& lib_cfg_proto) {
+                                   arcticdb::proto::storage::LibraryConfig& lib_cfg_proto,
+                                   bool override_https) {
     util::variant_match(
             storage_override.variant(),
-            StorageVisitor<S3Override>{lib_cfg_proto},
-            StorageVisitor<AzureOverride>{lib_cfg_proto},
-            StorageVisitor<LmdbOverride>{lib_cfg_proto},
+            StorageVisitor<S3Override>{lib_cfg_proto, override_https},
+            StorageVisitor<AzureOverride>{lib_cfg_proto, override_https},
+            StorageVisitor<LmdbOverride>{lib_cfg_proto, override_https},
             [] (const std::monostate&) {});
 }
 
@@ -91,7 +93,7 @@ void LibraryManager::write_library_config(const py::object& lib_cfg, const Libra
     google::protobuf::Any output = {};
     python_util::pb_from_python(lib_cfg, lib_cfg_proto);
 
-    apply_storage_override(storage_override, lib_cfg_proto);
+    apply_storage_override(storage_override, lib_cfg_proto, false);
 
     output.PackFrom(lib_cfg_proto);
 
@@ -191,7 +193,7 @@ arcticdb::proto::storage::LibraryConfig LibraryManager::get_config_internal(cons
     auto any = segment_in_memory.metadata();
     arcticdb::proto::storage::LibraryConfig lib_cfg_proto;
     any->UnpackTo(&lib_cfg_proto);
-    apply_storage_override(storage_override, lib_cfg_proto);
+    apply_storage_override(storage_override, lib_cfg_proto, true);
     return lib_cfg_proto;
 }
 
diff --git a/cpp/arcticdb/storage/storage_override.hpp b/cpp/arcticdb/storage/storage_override.hpp
index 3a49b2fa6d..5fe0f7c643 100644
--- a/cpp/arcticdb/storage/storage_override.hpp
+++ b/cpp/arcticdb/storage/storage_override.hpp
@@ -102,7 +102,8 @@ class S3Override {
         ssl_ = ssl;
     }
 
-    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage) const {
+    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage,
+                                bool override_https) const {
         if(storage.config().Is<arcticdb::proto::s3_storage::Config>()) {
             arcticdb::proto::s3_storage::Config s3_storage;
             storage.config().UnpackTo(&s3_storage);
@@ -117,6 +118,10 @@ class S3Override {
             s3_storage.set_ca_cert_dir(ca_cert_dir_);
             s3_storage.set_ssl(ssl_);
 
+            if(override_https) {
+                s3_storage.set_https(https_);
+            }
+
             util::pack_to_any(s3_storage, *storage.mutable_config());
         }
     }
@@ -161,7 +166,8 @@ class AzureOverride {
         ca_cert_dir_ = ca_cert_dir;
     }
 
-    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage) const {
+    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage,
+                                bool override_https ARCTICDB_UNUSED) const {
         if(storage.config().Is<arcticdb::proto::azure_storage::Config>()) {
             arcticdb::proto::azure_storage::Config azure_storage;
             storage.config().UnpackTo(&azure_storage);
@@ -198,7 +204,8 @@ class LmdbOverride {
         map_size_ = map_size;
     }
 
-    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage) const {
+    void modify_storage_config(arcticdb::proto::storage::VariantStorage& storage,
+                                bool override_https ARCTICDB_UNUSED) const {
         if(storage.config().Is<arcticdb::proto::lmdb_storage::Config>()) {
             arcticdb::proto::lmdb_storage::Config lmdb_storage;
             storage.config().UnpackTo(&lmdb_storage);


Test patch:
diff --git a/python/tests/integration/arcticdb/test_arctic_library_management.py b/python/tests/integration/arcticdb/test_arctic_library_management.py
index 3b7f9c9397..5f87b13259 100644
--- a/python/tests/integration/arcticdb/test_arctic_library_management.py
+++ b/python/tests/integration/arcticdb/test_arctic_library_management.py
@@ -33,7 +33,7 @@
     ArcticInvalidApiUsageException,
 )
 
-from tests.util.mark import AZURE_TESTS_MARK, MONGO_TESTS_MARK, REAL_S3_TESTS_MARK
+from tests.util.mark import AZURE_TESTS_MARK, MONGO_TESTS_MARK, REAL_S3_TESTS_MARK, SSL_TEST_SUPPORTED
 from tests.util.storage_test import get_s3_storage_config
 
 from arcticdb.options import ModifiableEnterpriseLibraryOption, ModifiableLibraryOption
@@ -306,6 +306,8 @@ def test_do_not_persist_s3_details(s3_storage):
     """We apply an in-memory overlay for these instead. In particular we should absolutely not persist credentials
     in the storage."""
 
+    if SSL_TEST_SUPPORTED:
+        assert s3_storage.arctic_uri.startswith("s3s://")
     ac = Arctic(s3_storage.arctic_uri)
     lib = ac.create_library("test")
     lib.write("sym", pd.DataFrame())
@@ -321,10 +323,13 @@ def test_do_not_persist_s3_details(s3_storage):
     assert s3_storage.request_timeout == 0
     assert not s3_storage.ssl
     assert s3_storage.prefix.startswith("test")
-    # HTTS is persisted on purpose to support backwards compatibility
-    # assert not s3_storage.https
     assert s3_storage.region == ""
     assert not s3_storage.use_virtual_addressing
+    # HTTS is persisted on purpose to support backwards compatibility
+    if SSL_TEST_SUPPORTED:
+        assert s3_storage.https
+    else:
+        assert not s3_storage.https
 
     assert "sym" in ac["test"].list_symbols()