From aea76c86937b8fd37252606d170c56f0d0c280f0 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 8 Aug 2025 13:39:30 -0400 Subject: [PATCH] Some improvements: - Get the default value from NuDB. - Throw on error instead of using the default value. - Check the parsed block size in unit tests. - Add some more BEAST_EXPECT checks. --- src/test/nodestore/NuDBFactory_test.cpp | 135 ++++++++++++-------- src/xrpld/nodestore/Backend.h | 8 ++ src/xrpld/nodestore/backend/NuDBFactory.cpp | 40 ++++-- 3 files changed, 117 insertions(+), 66 deletions(-) diff --git a/src/test/nodestore/NuDBFactory_test.cpp b/src/test/nodestore/NuDBFactory_test.cpp index 2bfee70f51..255d52d084 100644 --- a/src/test/nodestore/NuDBFactory_test.cpp +++ b/src/test/nodestore/NuDBFactory_test.cpp @@ -50,7 +50,9 @@ private: // Helper function to create a backend and test basic functionality bool - testBackendFunctionality(Section const& params) + testBackendFunctionality( + Section const& params, + std::size_t expectedBlocksize) { try { @@ -60,12 +62,15 @@ private: auto backend = Manager::instance().make_Backend( params, megabytes(4), scheduler, journal); - if (!backend) + if (!BEAST_EXPECT(backend)) + return false; + + if (!BEAST_EXPECT(backend->getBlockSize() == expectedBlocksize)) return false; backend->open(); - if (!backend->isOpen()) + if (!BEAST_EXPECT(backend->isOpen())) return false; // Test basic store/fetch functionality @@ -95,7 +100,7 @@ public: auto params = createSection(tempDir.path()); // Should work with default 4096 block size - BEAST_EXPECT(testBackendFunctionality(params)); + BEAST_EXPECT(testBackendFunctionality(params, 4096)); } void @@ -103,22 +108,27 @@ public: { testcase("Valid block sizes"); - std::vector validSizes = { - "4096", "8192", "16384", "32768"}; + std::vector validSizes = {4096, 8192, 16384, 32768}; for (auto const& size : validSizes) { beast::temp_dir tempDir; - auto params = createSection(tempDir.path(), size); + auto params = createSection(tempDir.path(), to_string(size)); - BEAST_EXPECT(testBackendFunctionality(params)); + BEAST_EXPECT(testBackendFunctionality(params, size)); } + // Empty value is ignored by the config parser, so uses the + // default + beast::temp_dir tempDir; + auto params = createSection(tempDir.path(), ""); + + BEAST_EXPECT(testBackendFunctionality(params, 4096)); } void testInvalidBlockSizes() { - testcase("Invalid block sizes with fallback to default"); + testcase("Invalid block sizes"); std::vector invalidSizes = { "2048", // Too small @@ -132,7 +142,6 @@ public: "-1", // Negative "abc", // Non-numeric "4k", // Invalid format - "", // Empty string "4096.5" // Decimal }; @@ -141,8 +150,8 @@ public: beast::temp_dir tempDir; auto params = createSection(tempDir.path(), size); - // Should still work by falling back to default 4096 - BEAST_EXPECT(testBackendFunctionality(params)); + // Fails + BEAST_EXPECT(!testBackendFunctionality(params, 4096)); } // Test whitespace cases separately since lexical_cast may handle them @@ -156,9 +165,8 @@ public: beast::temp_dir tempDir; auto params = createSection(tempDir.path(), size); - // Should work regardless (either parsed correctly or fallback to - // default) - BEAST_EXPECT(testBackendFunctionality(params)); + // Fails + BEAST_EXPECT(!testBackendFunctionality(params, 4096)); } } @@ -185,7 +193,7 @@ public: std::string::npos); } - // Test invalid block size warning + // Test invalid block size failure { beast::temp_dir tempDir; auto params = createSection(tempDir.path(), "5000"); @@ -194,19 +202,26 @@ public: beast::Journal journal(sink); DummyScheduler scheduler; - auto backend = Manager::instance().make_Backend( - params, megabytes(4), scheduler, journal); - - std::string logOutput = sink.messages().str(); - BEAST_EXPECT( - logOutput.find("Invalid nudb_block_size: 5000") != - std::string::npos); - BEAST_EXPECT( - logOutput.find("Must be power of 2 between 4096 and 32768") != - std::string::npos); + try + { + auto backend = Manager::instance().make_Backend( + params, megabytes(4), scheduler, journal); + fail(); + } + catch (std::exception const& e) + { + std::string logOutput{e.what()}; + BEAST_EXPECT( + logOutput.find("Invalid nudb_block_size: 5000") != + std::string::npos); + BEAST_EXPECT( + logOutput.find( + "Must be power of 2 between 4096 and 32768") != + std::string::npos); + } } - // Test non-numeric value warning + // Test non-numeric value failure { beast::temp_dir tempDir; auto params = createSection(tempDir.path(), "invalid"); @@ -215,15 +230,20 @@ public: beast::Journal journal(sink); DummyScheduler scheduler; - auto backend = Manager::instance().make_Backend( - params, megabytes(4), scheduler, journal); + try + { + auto backend = Manager::instance().make_Backend( + params, megabytes(4), scheduler, journal); - std::string logOutput = sink.messages().str(); - BEAST_EXPECT( - logOutput.find("Invalid nudb_block_size value: invalid") != - std::string::npos); - BEAST_EXPECT( - logOutput.find("Using default 4096") != std::string::npos); + fail(); + } + catch (std::exception const& e) + { + std::string logOutput{e.what()}; + BEAST_EXPECT( + logOutput.find("Invalid nudb_block_size value: invalid") != + std::string::npos); + } } } @@ -250,22 +270,25 @@ public: beast::temp_dir tempDir; auto params = createSection(tempDir.path(), size); - // All should work due to fallback, but we test the validation logic - // by checking if warnings are logged for invalid values + // We test the validation logic by catching exceptions for invalid + // values test::StreamSink sink(beast::severities::kWarning); beast::Journal journal(sink); DummyScheduler scheduler; - auto backend = Manager::instance().make_Backend( - params, megabytes(4), scheduler, journal); - - std::string logOutput = sink.messages().str(); - bool hasWarning = - logOutput.find("Invalid nudb_block_size") != std::string::npos; - - BEAST_EXPECT( - hasWarning == - !shouldWork); // Warning should appear for invalid values + try + { + auto backend = Manager::instance().make_Backend( + params, megabytes(4), scheduler, journal); + BEAST_EXPECT(shouldWork); + } + catch (std::exception const& e) + { + std::string logOutput{e.what()}; + BEAST_EXPECT( + logOutput.find("Invalid nudb_block_size") != + std::string::npos); + } } } @@ -285,7 +308,7 @@ public: auto backend1 = Manager::instance().make_Backend( params, megabytes(4), scheduler, journal); BEAST_EXPECT(backend1 != nullptr); - BEAST_EXPECT(testBackendFunctionality(params)); + BEAST_EXPECT(testBackendFunctionality(params, 16384)); } // Test second constructor (with nudb::context) @@ -346,11 +369,17 @@ public: beast::Journal journal(sink); DummyScheduler scheduler; - auto backend = Manager::instance().make_Backend( - params, megabytes(4), scheduler, journal); - - // Should work with either success or fallback to default - BEAST_EXPECT(testBackendFunctionality(params)); + try + { + auto backend = Manager::instance().make_Backend( + params, megabytes(4), scheduler, journal); + fail(); + } + catch (...) + { + // Fails + BEAST_EXPECT(!testBackendFunctionality(params, 8192)); + } } } diff --git a/src/xrpld/nodestore/Backend.h b/src/xrpld/nodestore/Backend.h index 1097895416..1f9a62716c 100644 --- a/src/xrpld/nodestore/Backend.h +++ b/src/xrpld/nodestore/Backend.h @@ -53,6 +53,14 @@ public: virtual std::string getName() = 0; + /** Get the block size for backends that support it + */ + virtual std::optional + getBlockSize() const + { + return std::nullopt; + } + /** Open the backend. @param createIfMissing Create the database files if necessary. This allows the caller to catch exceptions. diff --git a/src/xrpld/nodestore/backend/NuDBFactory.cpp b/src/xrpld/nodestore/backend/NuDBFactory.cpp index 54371387be..2386c590ce 100644 --- a/src/xrpld/nodestore/backend/NuDBFactory.cpp +++ b/src/xrpld/nodestore/backend/NuDBFactory.cpp @@ -68,7 +68,7 @@ public: , keyBytes_(keyBytes) , burstSize_(burstSize) , name_(get(keyValues, "path")) - , blockSize_(parseBlockSize(keyValues, journal)) + , blockSize_(parseBlockSize(name_, keyValues, journal)) , deletePath_(false) , scheduler_(scheduler) { @@ -88,7 +88,7 @@ public: , keyBytes_(keyBytes) , burstSize_(burstSize) , name_(get(keyValues, "path")) - , blockSize_(parseBlockSize(keyValues, journal)) + , blockSize_(parseBlockSize(name_, keyValues, journal)) , db_(context) , deletePath_(false) , scheduler_(scheduler) @@ -118,6 +118,12 @@ public: return name_; } + std::optional + getBlockSize() const override + { + return blockSize_; + } + void open(bool createIfMissing, uint64_t appType, uint64_t uid, uint64_t salt) override @@ -366,9 +372,18 @@ public: private: static std::size_t - parseBlockSize(Section const& keyValues, beast::Journal journal) + parseBlockSize( + std::string const& name, + Section const& keyValues, + beast::Journal journal) { - std::size_t blockSize = 4096; // Default 4K + using namespace boost::filesystem; + auto const folder = path(name); + auto const kp = (folder / "nudb.key").string(); + + std::size_t const defaultSize = + nudb::block_size(kp); // Default 4K from NuDB + std::size_t blockSize = defaultSize; std::string blockSizeStr; if (!get_if_exists(keyValues, "nudb_block_size", blockSizeStr)) @@ -385,11 +400,10 @@ private: if (parsedBlockSize < 4096 || parsedBlockSize > 32768 || (parsedBlockSize & (parsedBlockSize - 1)) != 0) { - JLOG(journal.warn()) - << "Invalid nudb_block_size: " << parsedBlockSize - << ". Must be power of 2 between 4096 and 32768. Using " - "default 4096."; - return 4096; + std::stringstream s; + s << "Invalid nudb_block_size: " << parsedBlockSize + << ". Must be power of 2 between 4096 and 32768."; + Throw(s.str()); } JLOG(journal.info()) @@ -399,10 +413,10 @@ private: } catch (std::exception const& e) { - JLOG(journal.warn()) - << "Invalid nudb_block_size value: " << blockSizeStr - << ". Using default 4096. Error: " << e.what(); - return 4096; + std::stringstream s; + s << "Invalid nudb_block_size value: " << blockSizeStr + << ". Error: " << e.what(); + Throw(s.str()); } } };