From ac37bfded0cdb3351809a1ca1417244ff432c558 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Fri, 16 Jul 2021 15:04:29 -0700 Subject: [PATCH] Allow CreateFromString to work on complex URIs (#8547) Summary: Some URIs for creating instances (ala SecondaryCache) use complex URIs like (cache://name;prop=value). These URIs were treated as name-value properties. With this change, if the URI does not contain an "id=XX" setting, it will be treated as a single string value (and not an ID and map of name-value properties). Pull Request resolved: https://github.com/facebook/rocksdb/pull/8547 Reviewed By: anand1976 Differential Revision: D29741386 Pulled By: mrambacher fbshipit-source-id: 0621f62bec3a6699a7b66c7c0b5634b2856653aa --- options/configurable.cc | 13 +++++++++---- options/customizable_test.cc | 17 +++++++++++++++++ options/options_helper.cc | 5 ++++- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/options/configurable.cc b/options/configurable.cc index 44ad5d865..1e85afcb9 100644 --- a/options/configurable.cc +++ b/options/configurable.cc @@ -747,15 +747,20 @@ Status ConfigurableHelper::GetOptionsMap( #ifndef ROCKSDB_LITE } else { status = StringToMap(value, props); - if (status.ok()) { + if (!status.ok()) { // There was an error creating the map. + *id = value; // Treat the value as id + props->clear(); // Clear the properties + status = Status::OK(); // and ignore the error + } else { auto iter = props->find(ConfigurableHelper::kIdPropName); if (iter != props->end()) { *id = iter->second; props->erase(iter); - } else if (default_id.empty()) { // Should this be an error?? - status = Status::InvalidArgument("Name property is missing"); - } else { + } else if (!default_id.empty()) { *id = default_id; + } else { // No id property and no default + *id = value; // Treat the value as id + props->clear(); // Clear the properties } } #else diff --git a/options/customizable_test.cc b/options/customizable_test.cc index 68ed84f43..ee6ca4b85 100644 --- a/options/customizable_test.cc +++ b/options/customizable_test.cc @@ -785,6 +785,23 @@ TEST_F(CustomizableTest, FactoryFunctionTest) { ASSERT_EQ(pointer, nullptr); } +TEST_F(CustomizableTest, URLFactoryTest) { + std::unique_ptr unique; + ConfigOptions ignore = config_options_; + ignore.ignore_unsupported_options = false; + ignore.ignore_unsupported_options = false; + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "A=1;x=y", &unique)); + ASSERT_NE(unique, nullptr); + ASSERT_EQ(unique->GetId(), "A=1;x=y"); + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "A;x=y", &unique)); + ASSERT_NE(unique, nullptr); + ASSERT_EQ(unique->GetId(), "A;x=y"); + unique.reset(); + ASSERT_OK(TestCustomizable::CreateFromString(ignore, "A=1?x=y", &unique)); + ASSERT_NE(unique, nullptr); + ASSERT_EQ(unique->GetId(), "A=1?x=y"); +} + TEST_F(CustomizableTest, MutableOptionsTest) { static std::unordered_map mutable_option_info = { {"mutable", diff --git a/options/options_helper.cc b/options/options_helper.cc index bf5356e75..c11984ff9 100644 --- a/options/options_helper.cc +++ b/options/options_helper.cc @@ -644,10 +644,13 @@ Status StringToMap(const std::string& opts_str, } while (pos < opts.size()) { - size_t eq_pos = opts.find('=', pos); + size_t eq_pos = opts.find_first_of("={};", pos); if (eq_pos == std::string::npos) { return Status::InvalidArgument("Mismatched key value pair, '=' expected"); + } else if (opts[eq_pos] != '=') { + return Status::InvalidArgument("Unexpected char in key"); } + std::string key = trim(opts.substr(pos, eq_pos - pos)); if (key.empty()) { return Status::InvalidArgument("Empty key found");