From 367990ae76523a88a6a863f1c41022da1016df82 Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Mon, 13 Jan 2025 10:52:40 -0500 Subject: [PATCH] Revert "feat: add config verify flag (#1814)" This reverts commit f1698c55ff8073b44cc12a8ebcc6eb03a209aced. --- src/app/CliArgs.cpp | 4 -- src/app/CliArgs.hpp | 9 +-- src/app/VerifyConfig.hpp | 55 ---------------- src/main/Main.cpp | 35 ++++++----- src/util/newconfig/ConfigDefinition.hpp | 1 - .../common/util/newconfig/FakeConfigData.hpp | 8 --- tests/unit/CMakeLists.txt | 1 - tests/unit/app/CliArgsTests.cpp | 53 ++-------------- tests/unit/app/VerifyConfigTests.cpp | 62 ------------------- 9 files changed, 27 insertions(+), 201 deletions(-) delete mode 100644 src/app/VerifyConfig.hpp delete mode 100644 tests/unit/app/VerifyConfigTests.cpp diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 4673c9f9..0c973dee 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -47,7 +47,6 @@ CliArgs::parse(int argc, char const* argv[]) ("conf,c", po::value()->default_value(kDEFAULT_CONFIG_PATH), "configuration file") ("ng-web-server,w", "Use ng-web-server") ("migrate", po::value(), "start migration helper") - ("verify", "Checks the validity of config values") ; // clang-format on po::positional_options_description positional; @@ -76,9 +75,6 @@ CliArgs::parse(int argc, char const* argv[]) return Action{Action::Migrate{.configPath = std::move(configPath), .subCmd = MigrateSubCmd::migration(opt)}}; } - if (parsed.count("verify") != 0u) - return Action{Action::VerifyConfig{.configPath = std::move(configPath)}}; - return Action{Action::Run{.configPath = std::move(configPath), .useNgWebServer = parsed.count("ng-web-server") != 0} }; } diff --git a/src/app/CliArgs.hpp b/src/app/CliArgs.hpp index 8f2a8c35..2142ad3f 100644 --- a/src/app/CliArgs.hpp +++ b/src/app/CliArgs.hpp @@ -59,11 +59,6 @@ public: MigrateSubCmd subCmd; }; - /** @brief Verify Config action. */ - struct VerifyConfig { - std::string configPath; - }; - /** * @brief Construct an action from a Run. * @@ -71,7 +66,7 @@ public: */ template requires std::is_same_v or std::is_same_v or - std::is_same_v or std::is_same_v + std::is_same_v explicit Action(ActionType&& action) : action_(std::forward(action)) { } @@ -91,7 +86,7 @@ public: } private: - std::variant action_; + std::variant action_; }; /** diff --git a/src/app/VerifyConfig.hpp b/src/app/VerifyConfig.hpp deleted file mode 100644 index 63c806dc..00000000 --- a/src/app/VerifyConfig.hpp +++ /dev/null @@ -1,55 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2025, the clio developers. - - Permission to use, copy, modify, and distribute this software for any - purpose with or without fee is hereby granted, provided that the above - copyright notice and this permission notice appear in all copies. - - THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES - WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF - MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR - ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN - ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF - OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -*/ -//============================================================================== - -#pragma once - -#include "util/newconfig/ConfigDefinition.hpp" -#include "util/newconfig/ConfigFileJson.hpp" - -#include -#include -#include - -namespace app { - -/** - * @brief Verifies user's config values are correct - * - * @param configPath The path to config - * @return true if config values are all correct, false otherwise - */ -inline bool -verifyConfig(std::string_view configPath) -{ - using namespace util::config; - - auto const json = ConfigFileJson::makeConfigFileJson(configPath); - if (!json.has_value()) { - std::cerr << json.error().error << std::endl; - return false; - } - auto const errors = gClioConfig.parse(json.value()); - if (errors.has_value()) { - for (auto const& err : errors.value()) - std::cerr << err.error << std::endl; - return false; - } - return true; -} -} // namespace app diff --git a/src/main/Main.cpp b/src/main/Main.cpp index bd060b12..6a817f0d 100644 --- a/src/main/Main.cpp +++ b/src/main/Main.cpp @@ -19,12 +19,12 @@ #include "app/CliArgs.hpp" #include "app/ClioApplication.hpp" -#include "app/VerifyConfig.hpp" #include "migration/MigrationApplication.hpp" #include "rpc/common/impl/HandlerProvider.hpp" #include "util/TerminationHandler.hpp" #include "util/log/Logger.hpp" #include "util/newconfig/ConfigDefinition.hpp" +#include "util/newconfig/ConfigFileJson.hpp" #include #include @@ -40,27 +40,34 @@ try { auto const action = app::CliArgs::parse(argc, argv); return action.apply( [](app::CliArgs::Action::Exit const& exit) { return exit.exitCode; }, - [](app::CliArgs::Action::VerifyConfig const& verify) { - if (app::verifyConfig(verify.configPath)) { - std::cout << "Config is correct" << "\n"; - return EXIT_SUCCESS; - } - return EXIT_FAILURE; - }, [](app::CliArgs::Action::Run const& run) { - auto const res = app::verifyConfig(run.configPath); - if (res != EXIT_SUCCESS) + auto const json = ConfigFileJson::makeConfigFileJson(run.configPath); + if (!json.has_value()) { + std::cerr << json.error().error << std::endl; return EXIT_FAILURE; - + } + auto const errors = gClioConfig.parse(json.value()); + if (errors.has_value()) { + for (auto const& err : errors.value()) + std::cerr << err.error << std::endl; + return EXIT_FAILURE; + } util::LogService::init(gClioConfig); app::ClioApplication clio{gClioConfig}; return clio.run(run.useNgWebServer); }, [](app::CliArgs::Action::Migrate const& migrate) { - auto const res = app::verifyConfig(migrate.configPath); - if (res != EXIT_SUCCESS) + auto const json = ConfigFileJson::makeConfigFileJson(migrate.configPath); + if (!json.has_value()) { + std::cerr << json.error().error << std::endl; return EXIT_FAILURE; - + } + auto const errors = gClioConfig.parse(json.value()); + if (errors.has_value()) { + for (auto const& err : errors.value()) + std::cerr << err.error << std::endl; + return EXIT_FAILURE; + } util::LogService::init(gClioConfig); app::MigratorApplication migrator{gClioConfig, migrate.subCmd}; return migrator.run(); diff --git a/src/util/newconfig/ConfigDefinition.hpp b/src/util/newconfig/ConfigDefinition.hpp index 69fb1b6e..2766e767 100644 --- a/src/util/newconfig/ConfigDefinition.hpp +++ b/src/util/newconfig/ConfigDefinition.hpp @@ -416,7 +416,6 @@ static ClioConfigDefinition gClioConfig = ClioConfigDefinition{ ConfigValue{ConfigType::Integer}.defaultValue(rpc::kAPI_VERSION_MIN).withConstraint(gValidateApiVersion)}, {"api_version.max", ConfigValue{ConfigType::Integer}.defaultValue(rpc::kAPI_VERSION_MAX).withConstraint(gValidateApiVersion)}, - {"migration.full_scan_threads", ConfigValue{ConfigType::Integer}.defaultValue(2).withConstraint(gValidateUint32)}, {"migration.full_scan_jobs", ConfigValue{ConfigType::Integer}.defaultValue(4).withConstraint(gValidateUint32)}, {"migration.cursors_per_job", ConfigValue{ConfigType::Integer}.defaultValue(100).withConstraint(gValidateUint32)}}, diff --git a/tests/common/util/newconfig/FakeConfigData.hpp b/tests/common/util/newconfig/FakeConfigData.hpp index f7340276..4167e2b2 100644 --- a/tests/common/util/newconfig/FakeConfigData.hpp +++ b/tests/common/util/newconfig/FakeConfigData.hpp @@ -208,11 +208,3 @@ static constexpr auto kINVALID_JSON_DATA = R"JSON({ "withDefault" : "0.0" } })JSON"; - -// used to Verify Config test -static constexpr auto kVALID_JSON_DATA = R"JSON({ - "server": { - "ip": "0.0.0.0", - "port": 51233 - } -})JSON"; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index fe74c1b8..c1371f59 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -5,7 +5,6 @@ target_sources( PRIVATE # Common ConfigTests.cpp app/CliArgsTests.cpp - app/VerifyConfigTests.cpp app/WebHandlersTests.cpp data/AmendmentCenterTests.cpp data/BackendCountersTests.cpp diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index d6e09842..437f2015 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -32,7 +32,6 @@ struct CliArgsTests : testing::Test { testing::StrictMock> onRunMock; testing::StrictMock> onExitMock; testing::StrictMock> onMigrateMock; - testing::StrictMock> onVerifyMock; }; TEST_F(CliArgsTests, Parse_NoArgs) @@ -47,13 +46,7 @@ TEST_F(CliArgsTests, Parse_NoArgs) return returnCode; }); EXPECT_EQ( - action.apply( - onRunMock.AsStdFunction(), - onExitMock.AsStdFunction(), - onMigrateMock.AsStdFunction(), - onVerifyMock.AsStdFunction() - ), - returnCode + action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), returnCode ); } @@ -69,12 +62,7 @@ TEST_F(CliArgsTests, Parse_NgWebServer) return returnCode; }); EXPECT_EQ( - action.apply( - onRunMock.AsStdFunction(), - onExitMock.AsStdFunction(), - onMigrateMock.AsStdFunction(), - onVerifyMock.AsStdFunction() - ), + action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), returnCode ); } @@ -91,12 +79,7 @@ TEST_F(CliArgsTests, Parse_VersionHelp) EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); EXPECT_EQ( - action.apply( - onRunMock.AsStdFunction(), - onExitMock.AsStdFunction(), - onMigrateMock.AsStdFunction(), - onVerifyMock.AsStdFunction() - ), + action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), EXIT_SUCCESS ); } @@ -114,34 +97,6 @@ TEST_F(CliArgsTests, Parse_Config) return returnCode; }); EXPECT_EQ( - action.apply( - onRunMock.AsStdFunction(), - onExitMock.AsStdFunction(), - onMigrateMock.AsStdFunction(), - onVerifyMock.AsStdFunction() - ), - returnCode - ); -} - -TEST_F(CliArgsTests, Parse_VerifyConfig) -{ - std::string_view configPath = "some_config_path"; - std::array argv{"clio_server", configPath.data(), "--verify"}; // NOLINT(bugprone-suspicious-stringview-data-usage) - auto const action = CliArgs::parse(argv.size(), argv.data()); - - int const returnCode = 123; - EXPECT_CALL(onVerifyMock, Call).WillOnce([&configPath](CliArgs::Action::VerifyConfig const& verify) { - EXPECT_EQ(verify.configPath, configPath); - return returnCode; - }); - EXPECT_EQ( - action.apply( - onRunMock.AsStdFunction(), - onExitMock.AsStdFunction(), - onMigrateMock.AsStdFunction(), - onVerifyMock.AsStdFunction() - ), - returnCode + action.apply(onRunMock.AsStdFunction(), onExitMock.AsStdFunction(), onMigrateMock.AsStdFunction()), returnCode ); } diff --git a/tests/unit/app/VerifyConfigTests.cpp b/tests/unit/app/VerifyConfigTests.cpp deleted file mode 100644 index 9dad9628..00000000 --- a/tests/unit/app/VerifyConfigTests.cpp +++ /dev/null @@ -1,62 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2025, the clio developers. - - Permission to use, copy, modify, and distribute this software for any - purpose with or without fee is hereby granted, provided that the above - copyright notice and this permission notice appear in all copies. - - THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES - WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF - MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR - ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN - ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF - OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -*/ -//============================================================================== - -#include "app/VerifyConfig.hpp" -#include "util/TmpFile.hpp" -#include "util/newconfig/FakeConfigData.hpp" - -#include - -using namespace app; -using namespace util::config; - -TEST(VerifyConfigTest, InvalidConfig) -{ - auto const tmpConfigFile = TmpFile(kJSON_DATA); - - // false because json data(kJSON_DATA) is not compatible with current configDefintion - EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); -} - -TEST(VerifyConfigTest, ValidConfig) -{ - auto const tmpConfigFile = TmpFile(kVALID_JSON_DATA); - - // current example config should always be compatible with configDefinition - EXPECT_TRUE(verifyConfig(tmpConfigFile.path)); -} - -TEST(VerifyConfigTest, ConfigFileNotExist) -{ - EXPECT_FALSE(verifyConfig("doesn't exist Config File")); -} - -TEST(VerifyConfigTest, InvalidJsonFile) -{ - // invalid json because extra "," after 51233 - static constexpr auto kINVALID_JSON = R"({ - "server": { - "ip": "0.0.0.0", - "port": 51233, - } - })"; - auto const tmpConfigFile = TmpFile(kINVALID_JSON); - - EXPECT_FALSE(verifyConfig(tmpConfigFile.path)); -}