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 034cffa0e6..fd0548caf4 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 40f42d2305..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);
@@ -115,9 +116,12 @@ class S3Override {
             s3_storage.set_use_virtual_addressing(use_virtual_addressing_);
             s3_storage.set_ca_cert_path(ca_cert_path_);
             s3_storage.set_ca_cert_dir(ca_cert_dir_);
-            s3_storage.set_https(https_);
             s3_storage.set_ssl(ssl_);
 
+            if(override_https) {
+                s3_storage.set_https(https_);
+            }
+
             util::pack_to_any(s3_storage, *storage.mutable_config());
         }
     }
@@ -162,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);
@@ -199,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 23e528754f..225eff7b0b 100644
--- a/python/tests/integration/arcticdb/test_arctic_library_management.py
+++ b/python/tests/integration/arcticdb/test_arctic_library_management.py
@@ -5,17 +5,26 @@
 
 As of the Change Date specified in that file, in accordance with the Business Source License, use of this software will be governed by the Apache License, version 2.0.
 """
+
 import pytest
 import pandas as pd
 
 from arcticdb_ext.exceptions import InternalException, UserInputException
 from arcticdb_ext.storage import KeyType
-from arcticdb.exceptions import ArcticDbNotYetImplemented, LibraryNotFound, MismatchingLibraryOptions
+from arcticdb.exceptions import (
+    ArcticDbNotYetImplemented,
+    LibraryNotFound,
+    MismatchingLibraryOptions,
+)
 from arcticdb.arctic import Arctic
 from arcticdb.options import LibraryOptions, EnterpriseLibraryOptions
 from arcticdb.encoding_version import EncodingVersion
 from arcticc.pb2.s3_storage_pb2 import Config as S3Config
-from arcticdb.storage_fixtures.api import StorageFixture, ArcticUriFields, StorageFixtureFactory
+from arcticdb.storage_fixtures.api import (
+    StorageFixture,
+    ArcticUriFields,
+    StorageFixtureFactory,
+)
 from arcticdb.version_store.library import (
     WritePayload,
     ArcticUnsupportedDataTypeException,
@@ -24,7 +33,13 @@
     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,
+    SSL_TEST_SUPPORTED,
+)
 from tests.util.storage_test import get_s3_storage_config
 
 from arcticdb.options import ModifiableEnterpriseLibraryOption, ModifiableLibraryOption
@@ -75,13 +90,21 @@ def test_get_library(arctic_client):
         columns_per_segment=10,
         encoding_version=EncodingVersion.V1 if ac._encoding_version == EncodingVersion.V2 else EncodingVersion.V2,
     )
-    lib = ac.get_library("pytest_test_lib_specified_options", create_if_missing=True, library_options=library_options)
+    lib = ac.get_library(
+        "pytest_test_lib_specified_options",
+        create_if_missing=True,
+        library_options=library_options,
+    )
     assert lib.options() == library_options
 
     # If the library already exists, create_if_missing is True, and options are provided, then the provided options must match the existing library
     library_options.dynamic_schema = False
     with pytest.raises(MismatchingLibraryOptions):
-        _ = ac.get_library("pytest_test_lib_specified_options", create_if_missing=True, library_options=library_options)
+        _ = ac.get_library(
+            "pytest_test_lib_specified_options",
+            create_if_missing=True,
+            library_options=library_options,
+        )
     # Throws if library_options are provided but create_if_missing is False
     with pytest.raises(ArcticInvalidApiUsageException):
         _ = ac.get_library("pytest_test_lib", create_if_missing=False, library_options=library_options)
@@ -98,8 +121,10 @@ def test_create_library_enterprise_options_defaults(lmdb_storage):
 
 def test_create_library_enterprise_options_set(lmdb_storage):
     ac = lmdb_storage.create_arctic()
-    lib = ac.create_library("lib", enterprise_library_options=EnterpriseLibraryOptions(replication=True,
-                                                                                       background_deletion=True))
+    lib = ac.create_library(
+        "lib",
+        enterprise_library_options=EnterpriseLibraryOptions(replication=True, background_deletion=True),
+    )
 
     enterprise_options = lib.enterprise_options()
     assert enterprise_options.replication
@@ -124,7 +149,10 @@ def test_create_library_replication_option_set_writes_logs(lmdb_storage):
 
 def test_create_library_background_deletion_option_set_does_not_delete(lmdb_storage):
     ac = lmdb_storage.create_arctic()
-    lib = ac.create_library("lib", enterprise_library_options=EnterpriseLibraryOptions(background_deletion=True))
+    lib = ac.create_library(
+        "lib",
+        enterprise_library_options=EnterpriseLibraryOptions(background_deletion=True),
+    )
     lt = lib._nvs.library_tool()
 
     df = pd.DataFrame({"col1": [1, 2, 3], "col2": [4, 5, 6]})
@@ -250,7 +278,7 @@ def test_create_library_with_invalid_name(arctic_client):
         ac.create_library(lib_name)
 
     # These should fail because the names are invalid
-    invalid_names = [chr(0), "lib>", "lib<", "lib*", "/lib", "lib...lib", "lib"*1000]
+    invalid_names = [chr(0), "lib>", "lib<", "lib*", "/lib", "lib...lib", "lib" * 1000]
     for lib_name in invalid_names:
         with pytest.raises(UserInputException):
             ac.create_library(lib_name)
@@ -264,7 +292,7 @@ def test_create_library_with_invalid_name(arctic_client):
 @pytest.mark.parametrize("suffix", ["", "suffix"])
 def test_create_library_with_all_chars(arctic_client_no_lmdb, prefix, suffix):
     # Create library names with each character (except '\' because Azure replaces it with '/' in some cases)
-    names = [f"{prefix}{chr(i)}{suffix}" for i in range(256) if chr(i) != '\\']
+    names = [f"{prefix}{chr(i)}{suffix}" for i in range(256) if chr(i) != "\\"]
 
     ac = arctic_client_no_lmdb
 
@@ -287,6 +315,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())
@@ -302,11 +332,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")
-    assert not s3_storage.https
     assert s3_storage.region == ""
     assert not s3_storage.use_virtual_addressing
-
-    assert "sym" in ac["test"].list_symbols()
+    # HTTS is persisted on purpose to support backwards compatibility
+    if SSL_TEST_SUPPORTED:
+        assert s3_storage.https
+    else:
+        assert not s3_storage.https
 
 
 def test_library_options(arctic_client):
@@ -323,7 +355,11 @@ def test_library_options(arctic_client):
     assert lib._nvs._lib_cfg.lib_desc.version.encoding_version == ac._encoding_version
 
     library_options = LibraryOptions(
-        dynamic_schema=True, dedup=True, rows_per_segment=20, columns_per_segment=3, encoding_version=EncodingVersion.V2
+        dynamic_schema=True,
+        dedup=True,
+        rows_per_segment=20,
+        columns_per_segment=3,
+        encoding_version=EncodingVersion.V2,
     )
     ac.create_library(
         "pytest_explicit_options",
@@ -360,8 +396,13 @@ def test_separation_between_libraries(arctic_client):
 
 def add_path_prefix(storage_fixture, prefix):
     if "path_prefix".casefold() in storage_fixture.arctic_uri.casefold():
-        return storage_fixture.replace_uri_field(storage_fixture.arctic_uri, ArcticUriFields.PATH_PREFIX,
-                                                 prefix, start=3, end=2)
+        return storage_fixture.replace_uri_field(
+            storage_fixture.arctic_uri,
+            ArcticUriFields.PATH_PREFIX,
+            prefix,
+            start=3,
+            end=2,
+        )
 
     if "azure" in storage_fixture.arctic_uri:  # azure connection string has a different format
         return f"{storage_fixture.arctic_uri};Path_prefix={prefix}"