fix a false positive case of parsing table factory from options file …

…(#10094) Summary: During options file parsing, reset table factory before attempting to parse it from string. This avoids mistakenly treating the default table factory as a newly created one. Signed-off-by: tabokie <[email protected]> Pull Request resolved: facebook/rocksdb#10094 Reviewed By: akankshamahajan15 Differential Revision: D36945378 Pulled By: ajkr fbshipit-source-id: 94b2604e5e87682063b4b78f6370f3e8f101dc44
zoltar9264 · Jun 14, 2022 · 1d2950b · 1d2950b
1 parent d665afd
commit 1d2950b
Show file tree

Hide file tree

Showing 2 changed files with 22 additions and 1 deletion.
diff --git a/options/options_parser.cc b/options/options_parser.cc
@@ -453,12 +453,13 @@ Status RocksDBOptionsParser::EndSection(
           section_arg);
     }
     // Ignore error as table factory deserialization is optional
+    cf_opt->table_factory.reset();
     s = TableFactory::CreateFromString(
         config_options,
         section_title.substr(
             opt_section_titles[kOptionSectionTableOptions].size()),
         &(cf_opt->table_factory));
-    if (s.ok()) {
+    if (s.ok() && cf_opt->table_factory != nullptr) {
       s = cf_opt->table_factory->ConfigureFromMap(config_options, opt_map);
       // Translate any errors (NotFound, NotSupported, to InvalidArgument
       if (s.ok() || s.IsInvalidArgument()) {

diff --git a/options/options_test.cc b/options/options_test.cc
@@ -47,6 +47,22 @@ namespace ROCKSDB_NAMESPACE {
 
 class OptionsTest : public testing::Test {};
 
+class UnregisteredTableFactory : public TableFactory {
+ public:
+  UnregisteredTableFactory() {}
+  const char* Name() const override { return "Unregistered"; }
+  using TableFactory::NewTableReader;
+  Status NewTableReader(const ReadOptions&, const TableReaderOptions&,
+                        std::unique_ptr<RandomAccessFileReader>&&, uint64_t,
+                        std::unique_ptr<TableReader>*, bool) const override {
+    return Status::NotSupported();
+  }
+  TableBuilder* NewTableBuilder(const TableBuilderOptions&,
+                                WritableFileWriter*) const override {
+    return nullptr;
+  }
+};
+
 #ifndef ROCKSDB_LITE  // GetOptionsFromMap is not supported in ROCKSDB_LITE
 TEST_F(OptionsTest, GetOptionsFromMapTest) {
   std::unordered_map<std::string, std::string> cf_options_map = {
@@ -3662,6 +3678,10 @@ TEST_F(OptionsParserTest, DumpAndParse) {
       cf_opt.table_factory.reset(test::RandomTableFactory(&rnd, c));
     } else if (c == 4) {
       cf_opt.table_factory.reset(NewBlockBasedTableFactory(special_bbto));
+    } else if (c == 5) {
+      // A table factory that doesn't support deserialization should be
+      // supported.
+      cf_opt.table_factory.reset(new UnregisteredTableFactory());
     }
     base_cf_opts.emplace_back(cf_opt);
   }