From cf8ea6cb02518c2f0b48ec7326ba134cd2fe07ed Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Thu, 16 Jan 2025 22:49:36 -0500 Subject: [PATCH 01/14] update description --- src/util/newconfig/ConfigDescription.hpp | 70 ++++++++++++------- .../newconfig/ClioConfigDefinitionTests.cpp | 5 +- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 71335a66c..836327eed 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -63,24 +63,30 @@ struct ClioConfigDescription { private: static constexpr auto kCONFIG_DESCRIPTION = std::array{ - KV{.key = "database.type", .value = "Type of database to use. Default is Scylladb."}, + KV{.key = "database.type", + .value = "Type of database to use. We currently support Cassandra and Scylladb. We default to Scylladb."}, KV{.key = "database.cassandra.contact_points", - .value = - "A list of IP addresses or hostnames of the initial nodes (Cassandra/Scylladb cluster nodes) that the " - "client will connect to when establishing a connection with the database."}, + .value = "A list of IP addresses or hostnames of the initial nodes (Cassandra/Scylladb cluster nodes) that " + "the client will connect to when establishing a connection with the database. If you're running " + "locally, it should be 'localhost' or 127.0.0.1"}, KV{.key = "database.cassandra.secure_connect_bundle", .value = "Configuration file that contains the necessary security credentials and connection details for " "securely " "connecting to a Cassandra database cluster."}, - KV{.key = "database.cassandra.port", .value = "Port number to connect to Cassandra."}, - KV{.key = "database.cassandra.keyspace", .value = "Keyspace to use in Cassandra."}, - KV{.key = "database.cassandra.replication_factor", .value = "Number of replicated nodes for Scylladb."}, - KV{.key = "database.cassandra.table_prefix", .value = "Prefix for Cassandra table names."}, + KV{.key = "database.cassandra.port", .value = "Port number to connect to the database."}, + KV{.key = "database.cassandra.keyspace", .value = "Keyspace to use for the database."}, + KV{.key = "database.cassandra.replication_factor", + .value = "Number of replicated nodes for Scylladb. Visit “here for more details : " + "https://university.scylladb.com/courses/scylla-essentials-overview/lessons/high-availability/" + "topic/fault-tolerance-replication-factor/ "}, + KV{.key = "database.cassandra.table_prefix", .value = "Prefix for Database table names."}, KV{.key = "database.cassandra.max_write_requests_outstanding", - .value = "Maximum number of outstanding write requests."}, + .value = "Maximum number of outstanding write requests. Write requests are api calls that write to database " + }, KV{.key = "database.cassandra.max_read_requests_outstanding", - .value = "Maximum number of outstanding read requests."}, - KV{.key = "database.cassandra.threads", .value = "Number of threads for Cassandra operations."}, + .value = "Maximum number of outstanding read requests, which reads from database"}, + KV{.key = "database.cassandra.threads", .value = "Number of threads that will be used for database operations." + }, KV{.key = "database.cassandra.core_connections_per_host", .value = "Number of core connections per host for Cassandra."}, KV{.key = "database.cassandra.queue_size_io", .value = "Queue size for I/O operations in Cassandra."}, @@ -106,9 +112,9 @@ struct ClioConfigDescription { .value = "Timeout duration for the forwarding cache used in Rippled communication."}, KV{.key = "forwarding.request_timeout", .value = "Timeout duration for the forwarding request used in Rippled communication."}, - KV{.key = "rpc.cache_timeout", .value = "Timeout duration for the rpc request."}, + KV{.key = "rpc.cache_timeout", .value = "Timeout duration for RPC requests."}, KV{.key = "num_markers", - .value = "The number of markers is the number of coroutines to load the cache concurrently."}, + .value = "The number of markers is the number of coroutines to download the initial ledger"}, KV{.key = "dos_guard.[].whitelist", .value = "List of IP addresses to whitelist for DOS protection."}, KV{.key = "dos_guard.max_fetches", .value = "Maximum number of fetch operations allowed by DOS guard."}, KV{.key = "dos_guard.max_connections", .value = "Maximum number of concurrent connections allowed by DOS guard." @@ -120,8 +126,11 @@ struct ClioConfigDescription { KV{.key = "server.port", .value = "Port number of the Clio HTTP server."}, KV{.key = "server.max_queue_size", .value = "Maximum size of the server's request queue. Value of 0 is no limit."}, - KV{.key = "server.local_admin", .value = "Indicates if the server should run with admin privileges."}, - KV{.key = "server.admin_password", .value = "Password for Clio admin-only APIs."}, + KV{.key = "server.local_admin", + .value = "Indicates if the server should run with admin privileges. Only one of local_admin or " + "admin_password can be set."}, + KV{.key = "server.admin_password", + .value = "Password for Clio admin-only APIs. Only one of local_admin or admin_password can be set."}, KV{.key = "server.processing_policy", .value = R"(Could be "sequent" or "parallel". For the sequent policy, requests from a single client connection are processed one by one, with the next request read only after the previous one is processed. For the parallel policy, Clio will accept @@ -133,26 +142,37 @@ struct ClioConfigDescription { KV{.key = "server.ws_max_sending_queue_size", .value = "Maximum size of the websocket sending queue."}, KV{.key = "prometheus.enabled", .value = "Enable or disable Prometheus metrics."}, KV{.key = "prometheus.compress_reply", .value = "Enable or disable compression of Prometheus responses."}, - KV{.key = "io_threads", .value = "Number of I/O threads. Value must be greater than 1"}, + KV{.key = "io_threads", .value = "Number of I/O threads. Value cannot be less than 1"}, KV{.key = "subscription_workers", .value = "The number of worker threads or processes that are responsible for managing and processing " - "subscription-based tasks."}, + "subscription-based tasks from rippled"}, KV{.key = "graceful_period", .value = "Number of milliseconds server will wait to shutdown gracefully."}, - KV{.key = "cache.num_diffs", .value = "Number of diffs to cache."}, + KV{.key = "cache.num_diffs", .value = "Number of diffs to cache. For more info, consult readme.md in etc"}, KV{.key = "cache.num_markers", .value = "Number of markers to cache."}, KV{.key = "cache.num_cursors_from_diff", .value = "Num of cursors that are different."}, KV{.key = "cache.num_cursors_from_account", .value = "Number of cursors from an account."}, KV{.key = "cache.page_fetch_size", .value = "Page fetch size for cache operations."}, KV{.key = "cache.load", .value = "Cache loading strategy ('sync' or 'async')."}, - KV{.key = "log_channels.[].channel", .value = "Name of the log channel."}, - KV{.key = "log_channels.[].log_level", .value = "Log level for the log channel."}, - KV{.key = "log_level", .value = "General logging level of Clio."}, + KV{.key = "log_channels.[].channel", + .value = "Name of the log channel. Possible values only include 'Backend', 'Webserver', 'Subscriptions', " + "'RPC', 'ETL', and 'Performance'"}, + KV{.key = "log_channels.[].log_level", + .value = "Log level for the specific log channel. Possible values only include `trace`, `debug`, `info`, " + "`warning`, `error`, `fatal`"}, + KV{.key = "log_level", + .value = "General logging level of Clio. Possible values only include `trace`, `debug`, `info`, `warning`, " + "`error`, `fatal`"}, KV{.key = "log_format", .value = "Format string for log messages."}, KV{.key = "log_to_console", .value = "Enable or disable logging to console."}, KV{.key = "log_directory", .value = "Directory path for log files."}, - KV{.key = "log_rotation_size", .value = "Log rotation size in megabytes."}, + KV{.key = "log_rotation_size", + .value = + "Log rotation size in megabytes. When the log file reaches this particular size, a new log file starts." + }, KV{.key = "log_directory_max_size", .value = "Maximum size of the log directory in megabytes."}, - KV{.key = "log_rotation_hour_interval", .value = "Interval in hours for log rotation."}, + KV{.key = "log_rotation_hour_interval", + .value = "Interval in hours for log rotation. If the current log file reaches this value in logging, a new " + "log file starts."}, KV{.key = "log_tag_style", .value = "Style for log tags."}, KV{.key = "extractor_threads", .value = "Number of extractor threads."}, KV{.key = "read_only", .value = "Indicates if the server should have read-only privileges."}, @@ -164,8 +184,8 @@ struct ClioConfigDescription { KV{.key = "api_version.default", .value = "Default API version Clio will run on."}, KV{.key = "api_version.min", .value = "Minimum API version."}, KV{.key = "api_version.max", .value = "Maximum API version."}, - KV{.key = "migration.full_scan_threads", .value = "The number of threads used to scan table."}, - KV{.key = "migration.full_scan_jobs", .value = "The number of coroutines used to scan table."}, + KV{.key = "migration.full_scan_threads", .value = "The number of threads used to scan the table."}, + KV{.key = "migration.full_scan_jobs", .value = "The number of coroutines used to scan the table."}, KV{.key = "migration.cursors_per_job", .value = "The number of cursors each coroutine will scan."} }; }; diff --git a/tests/unit/util/newconfig/ClioConfigDefinitionTests.cpp b/tests/unit/util/newconfig/ClioConfigDefinitionTests.cpp index 6d5846dd2..b2358a2e2 100644 --- a/tests/unit/util/newconfig/ClioConfigDefinitionTests.cpp +++ b/tests/unit/util/newconfig/ClioConfigDefinitionTests.cpp @@ -165,7 +165,10 @@ TEST(ConfigDescription, GetValues) { ClioConfigDescription const definition{}; - EXPECT_EQ(definition.get("database.type"), "Type of database to use. Default is Scylladb."); + EXPECT_EQ( + definition.get("database.type"), + "Type of database to use. We currently support Cassandra and Scylladb. We default to Scylladb." + ); EXPECT_EQ(definition.get("etl_sources.[].ip"), "IP address of the ETL source."); EXPECT_EQ(definition.get("prometheus.enabled"), "Enable or disable Prometheus metrics."); } From ffc7235adce11636a31977421ccc7d9510748e17 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 21 Jan 2025 11:39:06 -0500 Subject: [PATCH 02/14] create config --- src/app/CliArgs.cpp | 11 +++++++ src/main/impl/Build.cpp | 41 ++++++++++++++++++++++++ src/util/newconfig/ConfigDefinition.cpp | 1 + src/util/newconfig/ConfigDefinition.hpp | 20 ------------ src/util/newconfig/ConfigDescription.hpp | 29 +++++++++++++++++ 5 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 src/main/impl/Build.cpp diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 4673c9f90..89f3b507b 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -21,6 +21,7 @@ #include "migration/MigrationApplication.hpp" #include "util/build/Build.hpp" +#include "util/newconfig/ConfigDescription.hpp" #include #include @@ -44,6 +45,7 @@ CliArgs::parse(int argc, char const* argv[]) description.add_options() ("help,h", "print help message and exit") ("version,v", "print version and exit") + ("description,d", "generate config description") ("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") @@ -67,6 +69,15 @@ CliArgs::parse(int argc, char const* argv[]) return Action{Action::Exit{EXIT_SUCCESS}}; } + if (parsed.count("description") != 0u) { + auto const res = util::config::ClioConfigDescription::getMarkdown(); + if (res.has_value()) + return Action{Action::Exit{EXIT_SUCCESS}}; + + std::cerr << res.error().error << std::endl; + return Action{Action::Exit{EXIT_FAILURE}}; + } + auto configPath = parsed["conf"].as(); if (parsed.count("migrate") != 0u) { diff --git a/src/main/impl/Build.cpp b/src/main/impl/Build.cpp new file mode 100644 index 000000000..1ef23f2e5 --- /dev/null +++ b/src/main/impl/Build.cpp @@ -0,0 +1,41 @@ +//------------------------------------------------------------------------------ +/* + This file is part of clio: https://github.com/XRPLF/clio + Copyright (c) 2022, 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 "main/Build.hpp" + +#include + +namespace Build { +static constexpr char versionString[] = "20250116225553-checkData-63e03460"; + +std::string const& +getClioVersionString() +{ + static std::string const value = versionString; + return value; +} + +std::string const& +getClioFullVersionString() +{ + static std::string const value = "clio-" + getClioVersionString(); + return value; +} + +} // namespace Build diff --git a/src/util/newconfig/ConfigDefinition.cpp b/src/util/newconfig/ConfigDefinition.cpp index bad33717e..bbfff1d94 100644 --- a/src/util/newconfig/ConfigDefinition.cpp +++ b/src/util/newconfig/ConfigDefinition.cpp @@ -65,6 +65,7 @@ ClioConfigDefinition::getObject(std::string_view prefix, std::optional(mapVal)) { ASSERT(std::get(mapVal).size() > idx.value(), "Index provided is out of scope"); + // we want to support getObject("array") and getObject("array.[]"), so we check if "[]" exists if (!prefix.contains("[]")) return ObjectView{prefixWithDot + "[]", idx.value(), *this}; diff --git a/src/util/newconfig/ConfigDefinition.hpp b/src/util/newconfig/ConfigDefinition.hpp index 94a70b671..8e18821a6 100644 --- a/src/util/newconfig/ConfigDefinition.hpp +++ b/src/util/newconfig/ConfigDefinition.hpp @@ -84,26 +84,6 @@ class ClioConfigDefinition { [[nodiscard]] std::optional> parse(ConfigFileInterface const& config); - /** - * @brief Validates the configuration file - * - * Should only check for valid values, without populating - * - * @param config The configuration file interface - * @return An optional vector of Error objects stating all the failures if validation fails - */ - [[nodiscard]] std::optional> - validate(ConfigFileInterface const& config) const; - - /** - * @brief Generate markdown file of all the clio config descriptions - * - * @param configDescription The configuration description object - * @return An optional Error if generating markdown fails - */ - [[nodiscard]] std::expected - getMarkdown(ClioConfigDescription const& configDescription) const; - /** * @brief Returns the ObjectView specified with the prefix * diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 836327eed..62d6bdea1 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -20,9 +20,13 @@ #pragma once #include "util/Assert.hpp" +#include "util/newconfig/Error.hpp" #include #include +#include +#include +#include #include namespace util::config { @@ -61,6 +65,31 @@ struct ClioConfigDescription { return itr->value; } + static std::expected + getMarkdown() + { + std::ofstream file("Config-Descriptions.md"); + + if (!file.is_open()) + return std::unexpected{"failed to create file: Config-Descriptions.md "}; + + file << "# Config Description Markdown File\n"; + file << "This is a **markdown** file listing all Clio Configurations in detail.\n\n"; + file << "## Configuration Details\n\n"; + + for (auto const& [key, val] : kCONFIG_DESCRIPTION) { + file << "### " << key << "\n"; + file << val << "\n\n"; + } + file << "```\n"; + + // Close the file + file.close(); + + std::cout << "Markdown file generated successfully: Config-Descriptions.md" << "\n"; + return {}; + } + private: static constexpr auto kCONFIG_DESCRIPTION = std::array{ KV{.key = "database.type", From f2efbd1afb970bfa1431098ae7a38335a085d9f4 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 21 Jan 2025 14:08:53 -0500 Subject: [PATCH 03/14] add test --- src/util/newconfig/ConfigDescription.hpp | 9 +++++++-- tests/unit/app/CliArgsTests.cpp | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 62d6bdea1..ef1b5d247 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -22,6 +22,8 @@ #include "util/Assert.hpp" #include "util/newconfig/Error.hpp" +#include + #include #include #include @@ -38,6 +40,9 @@ namespace util::config { */ struct ClioConfigDescription { public: + /** @brief Name of the Markdown file containing detailed descriptions of all configuration values. */ + constexpr static std::string_view kCONFIG_DESCRIPTION_FILE_NAME = "Config-Descriptions.md"; + /** @brief Struct to represent a key-value pair*/ struct KV { std::string_view key; @@ -68,10 +73,10 @@ struct ClioConfigDescription { static std::expected getMarkdown() { - std::ofstream file("Config-Descriptions.md"); + std::ofstream file(kCONFIG_DESCRIPTION_FILE_NAME); if (!file.is_open()) - return std::unexpected{"failed to create file: Config-Descriptions.md "}; + return std::unexpected{fmt::format("failed to create file: {}", kCONFIG_DESCRIPTION_FILE_NAME)}; file << "# Config Description Markdown File\n"; file << "This is a **markdown** file listing all Clio Configurations in detail.\n\n"; diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index d6e098420..688e92f1a 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -18,12 +18,14 @@ //============================================================================== #include "app/CliArgs.hpp" +#include "util/newconfig/ConfigDescription.hpp" #include #include #include #include +#include #include using namespace app; @@ -145,3 +147,24 @@ TEST_F(CliArgsTests, Parse_VerifyConfig) returnCode ); } + +TEST_F(CliArgsTests, Parse_ConfigDescription) +{ + using namespace util::config; + std::array argv{"clio_server", "--description"}; // NOLINT(bugprone-suspicious-stringview-data-usage) + auto const action = CliArgs::parse(argv.size(), argv.data()); + EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); + + ASSERT_TRUE(std::filesystem::exists(ClioConfigDescription::kCONFIG_DESCRIPTION_FILE_NAME)); + std::filesystem::remove(ClioConfigDescription::kCONFIG_DESCRIPTION_FILE_NAME); + + EXPECT_EQ( + action.apply( + onRunMock.AsStdFunction(), + onExitMock.AsStdFunction(), + onMigrateMock.AsStdFunction(), + onVerifyMock.AsStdFunction() + ), + EXIT_SUCCESS + ); +} From c3ddc8a024c42a6ebaaad83faaa7765b6e5f6235 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 21 Jan 2025 14:17:19 -0500 Subject: [PATCH 04/14] remove extra file --- src/main/impl/Build.cpp | 41 ----------------------------------------- 1 file changed, 41 deletions(-) delete mode 100644 src/main/impl/Build.cpp diff --git a/src/main/impl/Build.cpp b/src/main/impl/Build.cpp deleted file mode 100644 index 1ef23f2e5..000000000 --- a/src/main/impl/Build.cpp +++ /dev/null @@ -1,41 +0,0 @@ -//------------------------------------------------------------------------------ -/* - This file is part of clio: https://github.com/XRPLF/clio - Copyright (c) 2022, 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 "main/Build.hpp" - -#include - -namespace Build { -static constexpr char versionString[] = "20250116225553-checkData-63e03460"; - -std::string const& -getClioVersionString() -{ - static std::string const value = versionString; - return value; -} - -std::string const& -getClioFullVersionString() -{ - static std::string const value = "clio-" + getClioVersionString(); - return value; -} - -} // namespace Build From e35dc6d29a56e4d4cadac7d6e9b9c82e67e3abe9 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 21 Jan 2025 14:35:47 -0500 Subject: [PATCH 05/14] fix doxygen --- src/util/newconfig/ConfigDescription.hpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index ef1b5d247..02bb97a8e 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -70,7 +70,12 @@ struct ClioConfigDescription { return itr->value; } - static std::expected + /** + * @brief Generate markdown file of all the clio config descriptions + * + * @return An Error if generating markdown fails, otherwise nothing + */ + [[nodiscard]] static std::expected getMarkdown() { std::ofstream file(kCONFIG_DESCRIPTION_FILE_NAME); From b7a35a5affeb5c5a1c35cfb30346eaf18f4f7732 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 21 Jan 2025 14:53:48 -0500 Subject: [PATCH 06/14] fix build --- src/util/newconfig/ConfigDescription.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 02bb97a8e..435d4c3a1 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -41,7 +41,7 @@ namespace util::config { struct ClioConfigDescription { public: /** @brief Name of the Markdown file containing detailed descriptions of all configuration values. */ - constexpr static std::string_view kCONFIG_DESCRIPTION_FILE_NAME = "Config-Descriptions.md"; + static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "Config-Descriptions.md"; /** @brief Struct to represent a key-value pair*/ struct KV { From cdf828bfee4ccb08fed5d8fa28633b35bf571f99 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Fri, 24 Jan 2025 23:18:29 -0500 Subject: [PATCH 07/14] fix comment --- src/app/CliArgs.cpp | 8 ++- src/util/newconfig/Array.hpp | 15 +++++ src/util/newconfig/ConfigConstraints.hpp | 78 ++++++++++++++++++++++++ src/util/newconfig/ConfigDefinition.hpp | 7 --- src/util/newconfig/ConfigDescription.hpp | 53 ++++++++++------ src/util/newconfig/ConfigValue.hpp | 24 ++++++++ src/util/newconfig/Types.hpp | 54 +++++++++++++++- src/util/newconfig/ValueView.hpp | 26 ++++++++ tests/unit/app/CliArgsTests.cpp | 20 +++++- 9 files changed, 255 insertions(+), 30 deletions(-) diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 89f3b507b..536b73575 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -45,11 +45,11 @@ CliArgs::parse(int argc, char const* argv[]) description.add_options() ("help,h", "print help message and exit") ("version,v", "print version and exit") - ("description,d", "generate config description") ("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") + ("config-description,d", po::value(),"generate config description") ; // clang-format on po::positional_options_description positional; @@ -69,8 +69,10 @@ CliArgs::parse(int argc, char const* argv[]) return Action{Action::Exit{EXIT_SUCCESS}}; } - if (parsed.count("description") != 0u) { - auto const res = util::config::ClioConfigDescription::getMarkdown(); + if (parsed.count("config-description") != 0u) { + auto const& filePath = parsed["config-description"].as(); + + auto const res = util::config::ClioConfigDescription::getMarkdown(filePath); if (res.has_value()) return Action{Action::Exit{EXIT_SUCCESS}}; diff --git a/src/util/newconfig/Array.hpp b/src/util/newconfig/Array.hpp index a6ccbe958..1f318bb8c 100644 --- a/src/util/newconfig/Array.hpp +++ b/src/util/newconfig/Array.hpp @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -98,6 +99,20 @@ class Array { [[nodiscard]] std::vector::const_iterator end() const; + /** + * @brief Custom output stream for Array + * + * @param stream The output stream + * @param arr The Array + * @return The same ostream we were given + */ + friend std::ostream& + operator<<(std::ostream& stream, Array arr) + { + stream << arr.getArrayPattern(); + return stream; + } + private: ConfigValue itemPattern_; std::vector elements_; diff --git a/src/util/newconfig/ConfigConstraints.hpp b/src/util/newconfig/ConfigConstraints.hpp index 315ea05f7..b047a4e40 100644 --- a/src/util/newconfig/ConfigConstraints.hpp +++ b/src/util/newconfig/ConfigConstraints.hpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -149,6 +150,28 @@ class Constraint { */ virtual std::optional checkValueImpl(Value const& val) const = 0; + + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + virtual void + print(std::ostream& stream) const = 0; + + /** + * @brief Custom output stream for constraint + * + * @param stream The output stream + * @param cons The constraint + * @return The same ostream we were given + */ + friend std::ostream& + operator<<(std::ostream& stream, Constraint const& cons) + { + cons.print(stream); + return stream; + } }; /** @@ -177,6 +200,17 @@ class PortConstraint final : public Constraint { [[nodiscard]] std::optional checkValueImpl(Value const& port) const override; + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + void + print(std::ostream& stream) const override + { + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`", kPORT_MIN, kPORT_MAX) << '\n'; + } + static constexpr uint32_t kPORT_MIN = 1; static constexpr uint32_t kPORT_MAX = 65535; }; @@ -206,6 +240,17 @@ class ValidIPConstraint final : public Constraint { */ [[nodiscard]] std::optional checkValueImpl(Value const& ip) const override; + + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + void + print(std::ostream& stream) const override + { + stream << fmt::format("The value must be a valid IP address") << '\n'; + } }; /** @@ -260,6 +305,17 @@ class OneOf final : public Constraint { return Error{makeErrorMsg(key_, val, arr_)}; } + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + void + print(std::ostream& stream) const override + { + stream << fmt::format("The value must be one of the following: `{}`", fmt::join(arr_, ", ")) << '\n'; + } + std::string_view key_; std::array arr_; }; @@ -312,6 +368,17 @@ class NumberValueConstraint final : public Constraint { return Error{fmt::format("Number must be between {} and {}", min_, max_)}; } + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + void + print(std::ostream& stream) const override + { + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`", min_, max_) << '\n'; + } + NumType min_; NumType max_; }; @@ -341,6 +408,17 @@ class PositiveDouble final : public Constraint { */ [[nodiscard]] std::optional checkValueImpl(Value const& num) const override; + + /** + * @brief Prints to the output stream for this specific constraint. + * + * @param stream The output stream + */ + void + print(std::ostream& stream) const override + { + stream << fmt::format("The value must be a positive float number") << '\n'; + } }; static constinit PortConstraint gValidatePort{}; diff --git a/src/util/newconfig/ConfigDefinition.hpp b/src/util/newconfig/ConfigDefinition.hpp index 8e18821a6..b3c5761a4 100644 --- a/src/util/newconfig/ConfigDefinition.hpp +++ b/src/util/newconfig/ConfigDefinition.hpp @@ -23,7 +23,6 @@ #include "util/Assert.hpp" #include "util/newconfig/Array.hpp" #include "util/newconfig/ConfigConstraints.hpp" -#include "util/newconfig/ConfigDescription.hpp" #include "util/newconfig/ConfigFileInterface.hpp" #include "util/newconfig/ConfigValue.hpp" #include "util/newconfig/Error.hpp" @@ -31,16 +30,10 @@ #include "util/newconfig/Types.hpp" #include "util/newconfig/ValueView.hpp" -#include -#include -#include - #include -#include #include #include #include -#include #include #include #include diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 435d4c3a1..63cd81efe 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -20,6 +20,7 @@ #pragma once #include "util/Assert.hpp" +#include "util/newconfig/ConfigDefinition.hpp" #include "util/newconfig/Error.hpp" #include @@ -27,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -76,27 +78,42 @@ struct ClioConfigDescription { * @return An Error if generating markdown fails, otherwise nothing */ [[nodiscard]] static std::expected - getMarkdown() + getMarkdown(std::string_view path) { - std::ofstream file(kCONFIG_DESCRIPTION_FILE_NAME); + namespace fs = std::filesystem; + // Construct the full file path + auto const fullFilePath = fmt::format("{}/{}", path, kCONFIG_DESCRIPTION_FILE_NAME); + + // Validate the directory exists + auto const dir = fs::path(fullFilePath).parent_path(); + if (!dir.empty() && !fs::exists(dir)) + return std::unexpected{fmt::format("Error: File path {} does not exist", fullFilePath)}; + + std::ofstream file(fullFilePath); if (!file.is_open()) return std::unexpected{fmt::format("failed to create file: {}", kCONFIG_DESCRIPTION_FILE_NAME)}; - file << "# Config Description Markdown File\n"; - file << "This is a **markdown** file listing all Clio Configurations in detail.\n\n"; + file << "# Clio Config Description\n"; + file << "This file lists all Clio Configuration definitions in detail.\n\n"; file << "## Configuration Details\n\n"; for (auto const& [key, val] : kCONFIG_DESCRIPTION) { - file << "### " << key << "\n"; - file << val << "\n\n"; + file << "### Key: " << key << "\n"; + + // Every type of value is directed to operator<< in ConfigValue.hpp + // as ConfigValue is the one that holds all the info regarding the config values + if (key.contains("[]")) { + file << gClioConfig.asArray(key); + } else { + file << gClioConfig.getValueView(key); + } + file << " - **Description**: " << val << "\n"; } - file << "```\n"; + file << "\n"; - // Close the file file.close(); - - std::cout << "Markdown file generated successfully: Config-Descriptions.md" << "\n"; + std::cout << "Markdown file generated successfully: " << fullFilePath << "\n"; return {}; } @@ -115,7 +132,7 @@ struct ClioConfigDescription { KV{.key = "database.cassandra.port", .value = "Port number to connect to the database."}, KV{.key = "database.cassandra.keyspace", .value = "Keyspace to use for the database."}, KV{.key = "database.cassandra.replication_factor", - .value = "Number of replicated nodes for Scylladb. Visit “here for more details : " + .value = "Number of replicated nodes for Scylladb. Visit this link for more details : " "https://university.scylladb.com/courses/scylla-essentials-overview/lessons/high-availability/" "topic/fault-tolerance-replication-factor/ "}, KV{.key = "database.cassandra.table_prefix", .value = "Prefix for Database table names."}, @@ -154,7 +171,7 @@ struct ClioConfigDescription { KV{.key = "rpc.cache_timeout", .value = "Timeout duration for RPC requests."}, KV{.key = "num_markers", .value = "The number of markers is the number of coroutines to download the initial ledger"}, - KV{.key = "dos_guard.[].whitelist", .value = "List of IP addresses to whitelist for DOS protection."}, + KV{.key = "dos_guard.whitelist.[]", .value = "List of IP addresses to whitelist for DOS protection."}, KV{.key = "dos_guard.max_fetches", .value = "Maximum number of fetch operations allowed by DOS guard."}, KV{.key = "dos_guard.max_connections", .value = "Maximum number of concurrent connections allowed by DOS guard." }, @@ -175,8 +192,8 @@ struct ClioConfigDescription { connection are processed one by one, with the next request read only after the previous one is processed. For the parallel policy, Clio will accept all requests and process them in parallel, sending a reply for each request as soon as it is ready.)"}, KV{.key = "server.parallel_requests_limit", - .value = R"(Optional parameter, used only if "processing_strategy" is - "parallel". It limits the number of requests for a single client connection that are processed in parallel. If not specified, the limit is infinite.)" + .value = + R"(Optional parameter, used only if processing_strategy `parallel`. It limits the number of requests for a single client connection that are processed in parallel. If not specified, the limit is infinite.)" }, KV{.key = "server.ws_max_sending_queue_size", .value = "Maximum size of the websocket sending queue."}, KV{.key = "prometheus.enabled", .value = "Enable or disable Prometheus metrics."}, @@ -193,14 +210,14 @@ struct ClioConfigDescription { KV{.key = "cache.page_fetch_size", .value = "Page fetch size for cache operations."}, KV{.key = "cache.load", .value = "Cache loading strategy ('sync' or 'async')."}, KV{.key = "log_channels.[].channel", - .value = "Name of the log channel. Possible values only include 'Backend', 'Webserver', 'Subscriptions', " + .value = "Name of the log channel." "'RPC', 'ETL', and 'Performance'"}, KV{.key = "log_channels.[].log_level", - .value = "Log level for the specific log channel. Possible values only include `trace`, `debug`, `info`, " + .value = "Log level for the specific log channel." "`warning`, `error`, `fatal`"}, KV{.key = "log_level", - .value = "General logging level of Clio. Possible values only include `trace`, `debug`, `info`, `warning`, " - "`error`, `fatal`"}, + .value = "General logging level of Clio. This level will be applied to all log channels that do not have an " + "explicitly defined logging level."}, KV{.key = "log_format", .value = "Format string for log messages."}, KV{.key = "log_to_console", .value = "Enable or disable logging to console."}, KV{.key = "log_directory", .value = "Directory path for log files."}, diff --git a/src/util/newconfig/ConfigValue.hpp b/src/util/newconfig/ConfigValue.hpp index d68daaa60..29d48d5ae 100644 --- a/src/util/newconfig/ConfigValue.hpp +++ b/src/util/newconfig/ConfigValue.hpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -201,6 +202,29 @@ class ConfigValue { return value_.value(); } + /** + * @brief Prints all the info of this config value to the output stream. + * + * @param stream The output stream + * @param val The config value to output to osstream + * @return The same ostream we were given + */ + friend std::ostream& + operator<<(std::ostream& stream, ConfigValue val) + { + stream << " - **Required**: " << (val.isOptional() ? "False" : "True") << "\n"; + stream << " - **Type**: : " << val.type() << "\n"; + stream << " - **Default value**: " << (val.hasValue() ? *val.value_ : "None") << "\n"; + stream << " - **Constraints**: "; + + if (val.getConstraint().has_value()) { + stream << val.getConstraint()->get() << "\n"; + } else { + stream << "None" << "\n"; + } + return stream; + } + private: /** * @brief Checks if the value type is consistent with the specified ConfigType diff --git a/src/util/newconfig/Types.hpp b/src/util/newconfig/Types.hpp index 42c9ddffa..401765a04 100644 --- a/src/util/newconfig/Types.hpp +++ b/src/util/newconfig/Types.hpp @@ -22,8 +22,9 @@ #include "util/UnsupportedType.hpp" #include +#include +#include #include -#include #include namespace util::config { @@ -31,9 +32,60 @@ namespace util::config { /** @brief Custom clio config types */ enum class ConfigType { Integer, String, Double, Boolean }; +/** + * @brief Prints the specified config type to output stream + * + * @param stream The output stream + * @param type The config type + * @return The same ostream we were given + */ +inline std::ostream& +operator<<(std::ostream& stream, ConfigType type) +{ + switch (type) { + case ConfigType::Integer: + stream << "int"; + break; + case ConfigType::String: + stream << "string"; + break; + case ConfigType::Double: + stream << "double"; + break; + case ConfigType::Boolean: + stream << "boolean"; + break; + default: + stream << "unsupported type"; + } + return stream; +} + /** @brief Represents the supported Config Values */ using Value = std::variant; +/** + * @brief Prints the specified value to output stream + * + * @param stream The output stream + * @param value The value type + * @return The same ostream we were given + */ +inline std::ostream& +operator<<(std::ostream& stream, Value value) +{ + if (std::holds_alternative(value)) { + stream << std::get(value) << "\n"; + } else if (std::holds_alternative(value)) { + stream << (std::get(value) ? "False" : "True") << "\n"; + } else if (std::holds_alternative(value)) { + stream << std::get(value) << "\n"; + } else if (std::holds_alternative(value)) { + stream << std::get(value) << "\n"; + } + return stream; +} + /** * @brief Get the corresponding clio config type * diff --git a/src/util/newconfig/ValueView.hpp b/src/util/newconfig/ValueView.hpp index 078296fa6..21c8eede7 100644 --- a/src/util/newconfig/ValueView.hpp +++ b/src/util/newconfig/ValueView.hpp @@ -20,6 +20,7 @@ #pragma once #include "util/Assert.hpp" +#include "util/newconfig/ConfigConstraints.hpp" #include "util/newconfig/ConfigValue.hpp" #include "util/newconfig/Types.hpp" @@ -30,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -141,6 +143,16 @@ class ValueView { return configVal_.get().isOptional(); } + /** + * @brief Retrieves the constraint associated with the ConfigValue in this ValueView, if any. + * + * @return An optional reference to the associated Constraint + */ + [[nodiscard]] std::optional> constexpr getConstraint() const + { + return configVal_.get().getConstraint(); + } + /** * @brief Retrieves the stored value as the specified type T * @@ -186,6 +198,20 @@ class ValueView { return std::make_optional(getValueImpl()); } + /** + * @brief Custom output stream for ValueView + * + * @param stream The output stream + * @param value The ValueView + * @return The same ostream we were given + */ + friend std::ostream& + operator<<(std::ostream& stream, ValueView value) + { + stream << value.configVal_; + return stream; + } + private: std::reference_wrapper configVal_; }; diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index 688e92f1a..615362b9e 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -151,7 +151,7 @@ TEST_F(CliArgsTests, Parse_VerifyConfig) TEST_F(CliArgsTests, Parse_ConfigDescription) { using namespace util::config; - std::array argv{"clio_server", "--description"}; // NOLINT(bugprone-suspicious-stringview-data-usage) + std::array argv{"clio_server", "--config-description", "."}; auto const action = CliArgs::parse(argv.size(), argv.data()); EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); @@ -168,3 +168,21 @@ TEST_F(CliArgsTests, Parse_ConfigDescription) EXIT_SUCCESS ); } + +TEST_F(CliArgsTests, Parse_ConfigDescriptionInvalidPath) +{ + using namespace util::config; + std::array argv{"clio_server", "--config-description", "...."}; + auto const action = CliArgs::parse(argv.size(), argv.data()); + 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() + ), + EXIT_FAILURE + ); +} From 6b18554b045bb5cbd77bd0b0681e4c6450d203a6 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Fri, 24 Jan 2025 23:47:44 -0500 Subject: [PATCH 08/14] fix doxygen --- src/util/newconfig/ConfigDescription.hpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 63cd81efe..6efe3e39e 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -75,6 +75,7 @@ struct ClioConfigDescription { /** * @brief Generate markdown file of all the clio config descriptions * + * @param path The path location to generate the Config-description file * @return An Error if generating markdown fails, otherwise nothing */ [[nodiscard]] static std::expected From 7b431d67a404193f1a0a6802a8762bc22dc3ad7c Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Sat, 25 Jan 2025 21:38:27 -0500 Subject: [PATCH 09/14] fix build --- src/util/newconfig/ConfigValue.hpp | 2 +- src/util/newconfig/ValueView.hpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/newconfig/ConfigValue.hpp b/src/util/newconfig/ConfigValue.hpp index 29d48d5ae..deeec8bd5 100644 --- a/src/util/newconfig/ConfigValue.hpp +++ b/src/util/newconfig/ConfigValue.hpp @@ -142,7 +142,7 @@ class ConfigValue { * * @return An optional reference to the associated Constraint. */ - [[nodiscard]] std::optional> + [[nodiscard]] constexpr std::optional> getConstraint() const { return cons_; diff --git a/src/util/newconfig/ValueView.hpp b/src/util/newconfig/ValueView.hpp index 21c8eede7..d23848542 100644 --- a/src/util/newconfig/ValueView.hpp +++ b/src/util/newconfig/ValueView.hpp @@ -148,7 +148,8 @@ class ValueView { * * @return An optional reference to the associated Constraint */ - [[nodiscard]] std::optional> constexpr getConstraint() const + [[nodiscard]] constexpr std::optional> + getConstraint() const { return configVal_.get().getConstraint(); } From ce1bb58a93caf781a0a15983554f032eaea9d4d6 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Tue, 28 Jan 2025 12:31:35 -0500 Subject: [PATCH 10/14] fix comments --- src/app/CliArgs.cpp | 9 +++-- src/util/newconfig/Array.hpp | 28 +++++++-------- src/util/newconfig/ConfigConstraints.hpp | 10 +++--- src/util/newconfig/ConfigDescription.hpp | 30 ++++++++++------ src/util/newconfig/ConfigValue.hpp | 2 +- tests/unit/app/CliArgsTests.cpp | 44 +++++++++++++++++++++--- 6 files changed, 84 insertions(+), 39 deletions(-) diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 536b73575..e1bb9403d 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -49,7 +50,7 @@ CliArgs::parse(int argc, char const* argv[]) ("ng-web-server,w", "Use ng-web-server") ("migrate", po::value(), "start migration helper") ("verify", "Checks the validity of config values") - ("config-description,d", po::value(),"generate config description") + ("config-description,d", po::value(), "generate config description") ; // clang-format on po::positional_options_description positional; @@ -70,9 +71,11 @@ CliArgs::parse(int argc, char const* argv[]) } if (parsed.count("config-description") != 0u) { - auto const& filePath = parsed["config-description"].as(); + std::filesystem::path const& filePath = parsed["config-description"].as(); + if (!filePath.string().ends_with(".md")) + filePath.string() += ".md"; - auto const res = util::config::ClioConfigDescription::getMarkdown(filePath); + auto const res = util::config::ClioConfigDescription::generateConfigDescriptionToFile(filePath); if (res.has_value()) return Action{Action::Exit{EXIT_SUCCESS}}; diff --git a/src/util/newconfig/Array.hpp b/src/util/newconfig/Array.hpp index 1f318bb8c..672757655 100644 --- a/src/util/newconfig/Array.hpp +++ b/src/util/newconfig/Array.hpp @@ -99,23 +99,23 @@ class Array { [[nodiscard]] std::vector::const_iterator end() const; - /** - * @brief Custom output stream for Array - * - * @param stream The output stream - * @param arr The Array - * @return The same ostream we were given - */ - friend std::ostream& - operator<<(std::ostream& stream, Array arr) - { - stream << arr.getArrayPattern(); - return stream; - } - private: ConfigValue itemPattern_; std::vector elements_; }; +/** + * @brief Custom output stream for Array + * + * @param stream The output stream + * @param arr The Array + * @return The same ostream we were given + */ +inline std::ostream& +operator<<(std::ostream& stream, Array arr) +{ + stream << arr.getArrayPattern(); + return stream; +} + } // namespace util::config diff --git a/src/util/newconfig/ConfigConstraints.hpp b/src/util/newconfig/ConfigConstraints.hpp index b047a4e40..673901c05 100644 --- a/src/util/newconfig/ConfigConstraints.hpp +++ b/src/util/newconfig/ConfigConstraints.hpp @@ -208,7 +208,7 @@ class PortConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`", kPORT_MIN, kPORT_MAX) << '\n'; + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`\n", kPORT_MIN, kPORT_MAX); } static constexpr uint32_t kPORT_MIN = 1; @@ -249,7 +249,7 @@ class ValidIPConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The value must be a valid IP address") << '\n'; + stream << "The value must be a valid IP address\n"; } }; @@ -313,7 +313,7 @@ class OneOf final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The value must be one of the following: `{}`", fmt::join(arr_, ", ")) << '\n'; + stream << fmt::format("The value must be one of the following: `{}`\n", fmt::join(arr_, ", ")); } std::string_view key_; @@ -376,7 +376,7 @@ class NumberValueConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`", min_, max_) << '\n'; + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`\n", min_, max_); } NumType min_; @@ -417,7 +417,7 @@ class PositiveDouble final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The value must be a positive float number") << '\n'; + stream << fmt::format("The value must be a positive double number\n"); } }; diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 6efe3e39e..9b0af342a 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -42,9 +42,6 @@ namespace util::config { */ struct ClioConfigDescription { public: - /** @brief Name of the Markdown file containing detailed descriptions of all configuration values. */ - static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "Config-Descriptions.md"; - /** @brief Struct to represent a key-value pair*/ struct KV { std::string_view key; @@ -79,22 +76,37 @@ struct ClioConfigDescription { * @return An Error if generating markdown fails, otherwise nothing */ [[nodiscard]] static std::expected - getMarkdown(std::string_view path) + generateConfigDescriptionToFile(std::filesystem::path path) { namespace fs = std::filesystem; // Construct the full file path - auto const fullFilePath = fmt::format("{}/{}", path, kCONFIG_DESCRIPTION_FILE_NAME); + fs::path const fullFilePath = path.string(); // Validate the directory exists auto const dir = fs::path(fullFilePath).parent_path(); if (!dir.empty() && !fs::exists(dir)) - return std::unexpected{fmt::format("Error: File path {} does not exist", fullFilePath)}; + return std::unexpected{fmt::format("Error: File path {} does not exist", fullFilePath.string())}; std::ofstream file(fullFilePath); if (!file.is_open()) - return std::unexpected{fmt::format("failed to create file: {}", kCONFIG_DESCRIPTION_FILE_NAME)}; + return std::unexpected{fmt::format("failed to create file: {}", fullFilePath.string())}; + + writeConfigDescriptionToFile(file); + file.close(); + + std::cout << "Markdown file generated successfully: " << fullFilePath << "\n"; + return {}; + } + /** + * @brief Writes to Config description to file + * + * @param file The config file to write to + */ + static void + writeConfigDescriptionToFile(std::ostream& file) + { file << "# Clio Config Description\n"; file << "This file lists all Clio Configuration definitions in detail.\n\n"; file << "## Configuration Details\n\n"; @@ -112,10 +124,6 @@ struct ClioConfigDescription { file << " - **Description**: " << val << "\n"; } file << "\n"; - - file.close(); - std::cout << "Markdown file generated successfully: " << fullFilePath << "\n"; - return {}; } private: diff --git a/src/util/newconfig/ConfigValue.hpp b/src/util/newconfig/ConfigValue.hpp index deeec8bd5..dc898eb4f 100644 --- a/src/util/newconfig/ConfigValue.hpp +++ b/src/util/newconfig/ConfigValue.hpp @@ -213,7 +213,7 @@ class ConfigValue { operator<<(std::ostream& stream, ConfigValue val) { stream << " - **Required**: " << (val.isOptional() ? "False" : "True") << "\n"; - stream << " - **Type**: : " << val.type() << "\n"; + stream << " - **Type**: " << val.type() << "\n"; stream << " - **Default value**: " << (val.hasValue() ? *val.value_ : "None") << "\n"; stream << " - **Constraints**: "; diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index 615362b9e..dd957b493 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -18,6 +18,7 @@ //============================================================================== #include "app/CliArgs.hpp" +#include "util/newconfig/ConfigDefinition.hpp" #include "util/newconfig/ConfigDescription.hpp" #include @@ -26,6 +27,9 @@ #include #include #include +#include +#include +#include #include using namespace app; @@ -37,6 +41,8 @@ struct CliArgsTests : testing::Test { testing::StrictMock> onVerifyMock; }; +static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "../configDescription"; + TEST_F(CliArgsTests, Parse_NoArgs) { std::array argv{"clio_server"}; @@ -150,13 +156,13 @@ TEST_F(CliArgsTests, Parse_VerifyConfig) TEST_F(CliArgsTests, Parse_ConfigDescription) { - using namespace util::config; - std::array argv{"clio_server", "--config-description", "."}; + std::array argv{"clio_server", "--config-description", kCONFIG_DESCRIPTION_FILE_NAME}; auto const action = CliArgs::parse(argv.size(), argv.data()); EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); - ASSERT_TRUE(std::filesystem::exists(ClioConfigDescription::kCONFIG_DESCRIPTION_FILE_NAME)); - std::filesystem::remove(ClioConfigDescription::kCONFIG_DESCRIPTION_FILE_NAME); + // user provide config markdown file name as well + ASSERT_TRUE(std::filesystem::exists(kCONFIG_DESCRIPTION_FILE_NAME)); + std::filesystem::remove(kCONFIG_DESCRIPTION_FILE_NAME); EXPECT_EQ( action.apply( @@ -169,10 +175,38 @@ TEST_F(CliArgsTests, Parse_ConfigDescription) ); } +TEST_F(CliArgsTests, Parse_ConfigDescriptionFileContent) +{ + using namespace util::config; + + std::ofstream file(kCONFIG_DESCRIPTION_FILE_NAME); + ASSERT_TRUE(file.is_open()); + ClioConfigDescription::writeConfigDescriptionToFile(file); + file.close(); + + std::ifstream inFile(kCONFIG_DESCRIPTION_FILE_NAME); + ASSERT_TRUE(inFile.is_open()); + + std::stringstream buffer; + buffer << inFile.rdbuf(); + inFile.close(); + + auto const fileContent = buffer.str(); + EXPECT_TRUE(fileContent.find("# Clio Config Description") != std::string::npos); + EXPECT_TRUE(fileContent.find("This file lists all Clio Configuration definitions in detail.") != std::string::npos); + EXPECT_TRUE(fileContent.find("## Configuration Details") != std::string::npos); + + // all keys that exist in clio config should be listed in config description file + for (auto const& key : gClioConfig) + EXPECT_TRUE(fileContent.find(key.first)); + + std::filesystem::remove(kCONFIG_DESCRIPTION_FILE_NAME); +} + TEST_F(CliArgsTests, Parse_ConfigDescriptionInvalidPath) { using namespace util::config; - std::array argv{"clio_server", "--config-description", "...."}; + std::array argv{"clio_server", "--config-description", ""}; auto const action = CliArgs::parse(argv.size(), argv.data()); EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); From a535717a0df90d37198beae4d259457a2a5b0cf1 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Wed, 29 Jan 2025 11:28:26 -0500 Subject: [PATCH 11/14] fix markdown rendering --- src/app/CliArgs.cpp | 16 ++++++++-------- src/util/newconfig/ConfigConstraints.hpp | 10 +++++----- src/util/newconfig/ConfigDescription.hpp | 2 +- src/util/newconfig/ConfigValue.hpp | 8 ++++---- src/util/newconfig/Types.hpp | 8 ++++---- tests/unit/app/CliArgsTests.cpp | 2 +- 6 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index e1bb9403d..623fd8dae 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -44,13 +44,13 @@ CliArgs::parse(int argc, char const* argv[]) // clang-format off po::options_description description("Options"); description.add_options() - ("help,h", "print help message and exit") - ("version,v", "print version and exit") - ("conf,c", po::value()->default_value(kDEFAULT_CONFIG_PATH), "configuration file") + ("help,h", "Print help message and exit") + ("version,v", "Print version and exit") + ("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") + ("migrate", po::value(), "Start migration helper") ("verify", "Checks the validity of config values") - ("config-description,d", po::value(), "generate config description") + ("config-description,d", po::value(), "Generate config description markdown file") ; // clang-format on po::positional_options_description positional; @@ -71,9 +71,9 @@ CliArgs::parse(int argc, char const* argv[]) } if (parsed.count("config-description") != 0u) { - std::filesystem::path const& filePath = parsed["config-description"].as(); - if (!filePath.string().ends_with(".md")) - filePath.string() += ".md"; + std::filesystem::path filePath = parsed["config-description"].as(); + if (!filePath.empty() && !filePath.string().ends_with(".md")) + filePath += ".md"; auto const res = util::config::ClioConfigDescription::generateConfigDescriptionToFile(filePath); if (res.has_value()) diff --git a/src/util/newconfig/ConfigConstraints.hpp b/src/util/newconfig/ConfigConstraints.hpp index 673901c05..910dff1e5 100644 --- a/src/util/newconfig/ConfigConstraints.hpp +++ b/src/util/newconfig/ConfigConstraints.hpp @@ -208,7 +208,7 @@ class PortConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`\n", kPORT_MIN, kPORT_MAX); + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}", kPORT_MIN, kPORT_MAX); } static constexpr uint32_t kPORT_MIN = 1; @@ -249,7 +249,7 @@ class ValidIPConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << "The value must be a valid IP address\n"; + stream << "The value must be a valid IP address"; } }; @@ -313,7 +313,7 @@ class OneOf final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The value must be one of the following: `{}`\n", fmt::join(arr_, ", ")); + stream << fmt::format("The value must be one of the following: `{}`", fmt::join(arr_, ", ")); } std::string_view key_; @@ -376,7 +376,7 @@ class NumberValueConstraint final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`\n", min_, max_); + stream << fmt::format("The minimum value is `{}`. The maximum value is `{}`", min_, max_); } NumType min_; @@ -417,7 +417,7 @@ class PositiveDouble final : public Constraint { void print(std::ostream& stream) const override { - stream << fmt::format("The value must be a positive double number\n"); + stream << fmt::format("The value must be a positive double number"); } }; diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 9b0af342a..4c252292b 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -90,7 +90,7 @@ struct ClioConfigDescription { std::ofstream file(fullFilePath); if (!file.is_open()) - return std::unexpected{fmt::format("failed to create file: {}", fullFilePath.string())}; + return std::unexpected{fmt::format("Failed to create file: {}", fullFilePath.string())}; writeConfigDescriptionToFile(file); file.close(); diff --git a/src/util/newconfig/ConfigValue.hpp b/src/util/newconfig/ConfigValue.hpp index dc898eb4f..d83025acf 100644 --- a/src/util/newconfig/ConfigValue.hpp +++ b/src/util/newconfig/ConfigValue.hpp @@ -212,10 +212,10 @@ class ConfigValue { friend std::ostream& operator<<(std::ostream& stream, ConfigValue val) { - stream << " - **Required**: " << (val.isOptional() ? "False" : "True") << "\n"; - stream << " - **Type**: " << val.type() << "\n"; - stream << " - **Default value**: " << (val.hasValue() ? *val.value_ : "None") << "\n"; - stream << " - **Constraints**: "; + stream << "- **Required**: " << (val.isOptional() ? "False" : "True") << "\n"; + stream << "- **Type**: " << val.type() << "\n"; + stream << "- **Default value**: " << (val.hasValue() ? *val.value_ : "None") << "\n"; + stream << "- **Constraints**: "; if (val.getConstraint().has_value()) { stream << val.getConstraint()->get() << "\n"; diff --git a/src/util/newconfig/Types.hpp b/src/util/newconfig/Types.hpp index 401765a04..58ec7bd1f 100644 --- a/src/util/newconfig/Types.hpp +++ b/src/util/newconfig/Types.hpp @@ -75,13 +75,13 @@ inline std::ostream& operator<<(std::ostream& stream, Value value) { if (std::holds_alternative(value)) { - stream << std::get(value) << "\n"; + stream << std::get(value); } else if (std::holds_alternative(value)) { - stream << (std::get(value) ? "False" : "True") << "\n"; + stream << (std::get(value) ? "False" : "True"); } else if (std::holds_alternative(value)) { - stream << std::get(value) << "\n"; + stream << std::get(value); } else if (std::holds_alternative(value)) { - stream << std::get(value) << "\n"; + stream << std::get(value); } return stream; } diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index dd957b493..bdd845b79 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -41,7 +41,7 @@ struct CliArgsTests : testing::Test { testing::StrictMock> onVerifyMock; }; -static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "../configDescription"; +static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "../configDescription.md"; TEST_F(CliArgsTests, Parse_NoArgs) { From 0d73ba84089af6d5277ee778387a2e0c814fa0fd Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Thu, 30 Jan 2025 14:16:32 -0500 Subject: [PATCH 12/14] fix comments --- src/app/CliArgs.cpp | 2 - src/util/CMakeLists.txt | 1 + src/util/newconfig/ConfigDescription.hpp | 13 +++-- src/util/newconfig/Types.cpp | 66 ++++++++++++++++++++++++ src/util/newconfig/Types.hpp | 39 ++------------ tests/common/util/TmpFile.hpp | 6 +++ tests/unit/app/CliArgsTests.cpp | 59 +++++++++++---------- 7 files changed, 118 insertions(+), 68 deletions(-) create mode 100644 src/util/newconfig/Types.cpp diff --git a/src/app/CliArgs.cpp b/src/app/CliArgs.cpp index 623fd8dae..2cfa04b2f 100644 --- a/src/app/CliArgs.cpp +++ b/src/app/CliArgs.cpp @@ -72,8 +72,6 @@ CliArgs::parse(int argc, char const* argv[]) if (parsed.count("config-description") != 0u) { std::filesystem::path filePath = parsed["config-description"].as(); - if (!filePath.empty() && !filePath.string().ends_with(".md")) - filePath += ".md"; auto const res = util::config::ClioConfigDescription::generateConfigDescriptionToFile(filePath); if (res.has_value()) diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index 8c28fdf95..ffc836833 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -33,6 +33,7 @@ target_sources( newconfig/ConfigDefinition.cpp newconfig/ConfigFileJson.cpp newconfig/ObjectView.cpp + newconfig/Types.cpp newconfig/ValueView.cpp ) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 4c252292b..9a500d2c0 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -27,6 +27,8 @@ #include #include +#include +#include #include #include #include @@ -86,11 +88,16 @@ struct ClioConfigDescription { // Validate the directory exists auto const dir = fs::path(fullFilePath).parent_path(); if (!dir.empty() && !fs::exists(dir)) - return std::unexpected{fmt::format("Error: File path {} does not exist", fullFilePath.string())}; + return std::unexpected{ + fmt::format("Error: Directory {} does not exist or provided path is invalid", dir.string()) + }; std::ofstream file(fullFilePath); - if (!file.is_open()) - return std::unexpected{fmt::format("Failed to create file: {}", fullFilePath.string())}; + if (!file.is_open()) { + return std::unexpected{ + fmt::format("Failed to create file '{}': {}", fullFilePath.string(), std::strerror(errno)) + }; + } writeConfigDescriptionToFile(file); file.close(); diff --git a/src/util/newconfig/Types.cpp b/src/util/newconfig/Types.cpp new file mode 100644 index 000000000..b8a6330db --- /dev/null +++ b/src/util/newconfig/Types.cpp @@ -0,0 +1,66 @@ +//------------------------------------------------------------------------------ +/* + 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 "util/newconfig/Types.hpp" + +#include +#include +#include +#include + +namespace util::config { + +std::ostream& +operator<<(std::ostream& stream, ConfigType type) +{ + switch (type) { + case ConfigType::Integer: + stream << "int"; + break; + case ConfigType::String: + stream << "string"; + break; + case ConfigType::Double: + stream << "double"; + break; + case ConfigType::Boolean: + stream << "boolean"; + break; + default: + stream << "unsupported type"; + } + return stream; +} + +std::ostream& +operator<<(std::ostream& stream, Value value) +{ + if (std::holds_alternative(value)) { + stream << std::get(value); + } else if (std::holds_alternative(value)) { + stream << (std::get(value) ? "False" : "True"); + } else if (std::holds_alternative(value)) { + stream << std::get(value); + } else if (std::holds_alternative(value)) { + stream << std::get(value); + } + return stream; +} + +} // namespace util::config diff --git a/src/util/newconfig/Types.hpp b/src/util/newconfig/Types.hpp index 58ec7bd1f..bc2f1cfdf 100644 --- a/src/util/newconfig/Types.hpp +++ b/src/util/newconfig/Types.hpp @@ -39,27 +39,8 @@ enum class ConfigType { Integer, String, Double, Boolean }; * @param type The config type * @return The same ostream we were given */ -inline std::ostream& -operator<<(std::ostream& stream, ConfigType type) -{ - switch (type) { - case ConfigType::Integer: - stream << "int"; - break; - case ConfigType::String: - stream << "string"; - break; - case ConfigType::Double: - stream << "double"; - break; - case ConfigType::Boolean: - stream << "boolean"; - break; - default: - stream << "unsupported type"; - } - return stream; -} +std::ostream& +operator<<(std::ostream& stream, ConfigType type); /** @brief Represents the supported Config Values */ using Value = std::variant; @@ -71,20 +52,8 @@ using Value = std::variant; * @param value The value type * @return The same ostream we were given */ -inline std::ostream& -operator<<(std::ostream& stream, Value value) -{ - if (std::holds_alternative(value)) { - stream << std::get(value); - } else if (std::holds_alternative(value)) { - stream << (std::get(value) ? "False" : "True"); - } else if (std::holds_alternative(value)) { - stream << std::get(value); - } else if (std::holds_alternative(value)) { - stream << std::get(value); - } - return stream; -} +std::ostream& +operator<<(std::ostream& stream, Value value); /** * @brief Get the corresponding clio config type diff --git a/tests/common/util/TmpFile.hpp b/tests/common/util/TmpFile.hpp index f09647035..a74f80ff4 100644 --- a/tests/common/util/TmpFile.hpp +++ b/tests/common/util/TmpFile.hpp @@ -37,6 +37,12 @@ struct TmpFile { ofs << content; } + static TmpFile + empty() + { + return TmpFile{""}; + } + TmpFile(TmpFile const&) = delete; TmpFile(TmpFile&& other) : path{std::move(other.path)} { diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index bdd845b79..1105b7d23 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -18,6 +18,7 @@ //============================================================================== #include "app/CliArgs.hpp" +#include "util/TmpFile.hpp" #include "util/newconfig/ConfigDefinition.hpp" #include "util/newconfig/ConfigDescription.hpp" @@ -41,8 +42,6 @@ struct CliArgsTests : testing::Test { testing::StrictMock> onVerifyMock; }; -static constexpr auto kCONFIG_DESCRIPTION_FILE_NAME = "../configDescription.md"; - TEST_F(CliArgsTests, Parse_NoArgs) { std::array argv{"clio_server"}; @@ -154,15 +153,37 @@ TEST_F(CliArgsTests, Parse_VerifyConfig) ); } -TEST_F(CliArgsTests, Parse_ConfigDescription) +TEST_F(CliArgsTests, Parse_ConfigDescriptionInvalidPath) { - std::array argv{"clio_server", "--config-description", kCONFIG_DESCRIPTION_FILE_NAME}; + using namespace util::config; + std::array argv{"clio_server", "--config-description", ""}; + auto const action = CliArgs::parse(argv.size(), argv.data()); + 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() + ), + EXIT_FAILURE + ); +} + +struct CliArgsTestsWithTmpFile : CliArgsTests { + TmpFile tmpFile = TmpFile::empty(); +}; + +TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescription) +{ + std::array argv{"clio_server", "--config-description", tmpFile.path.c_str()}; auto const action = CliArgs::parse(argv.size(), argv.data()); EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); // user provide config markdown file name as well - ASSERT_TRUE(std::filesystem::exists(kCONFIG_DESCRIPTION_FILE_NAME)); - std::filesystem::remove(kCONFIG_DESCRIPTION_FILE_NAME); + ASSERT_TRUE(std::filesystem::exists(tmpFile.path.c_str())); + std::filesystem::remove(tmpFile.path.c_str()); EXPECT_EQ( action.apply( @@ -175,16 +196,16 @@ TEST_F(CliArgsTests, Parse_ConfigDescription) ); } -TEST_F(CliArgsTests, Parse_ConfigDescriptionFileContent) +TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescriptionFileContent) { using namespace util::config; - std::ofstream file(kCONFIG_DESCRIPTION_FILE_NAME); + std::ofstream file(tmpFile.path.c_str()); ASSERT_TRUE(file.is_open()); ClioConfigDescription::writeConfigDescriptionToFile(file); file.close(); - std::ifstream inFile(kCONFIG_DESCRIPTION_FILE_NAME); + std::ifstream inFile(tmpFile.path.c_str()); ASSERT_TRUE(inFile.is_open()); std::stringstream buffer; @@ -200,23 +221,5 @@ TEST_F(CliArgsTests, Parse_ConfigDescriptionFileContent) for (auto const& key : gClioConfig) EXPECT_TRUE(fileContent.find(key.first)); - std::filesystem::remove(kCONFIG_DESCRIPTION_FILE_NAME); -} - -TEST_F(CliArgsTests, Parse_ConfigDescriptionInvalidPath) -{ - using namespace util::config; - std::array argv{"clio_server", "--config-description", ""}; - auto const action = CliArgs::parse(argv.size(), argv.data()); - 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() - ), - EXIT_FAILURE - ); + std::filesystem::remove(tmpFile.path.c_str()); } From 4c255114d17652a7a9b0a36c1376e30646d1f975 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Fri, 31 Jan 2025 14:34:33 -0500 Subject: [PATCH 13/14] remove extra .cstr() --- tests/unit/app/CliArgsTests.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index 1105b7d23..ec52763fc 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -182,8 +182,8 @@ TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescription) EXPECT_CALL(onExitMock, Call).WillOnce([](CliArgs::Action::Exit const& exit) { return exit.exitCode; }); // user provide config markdown file name as well - ASSERT_TRUE(std::filesystem::exists(tmpFile.path.c_str())); - std::filesystem::remove(tmpFile.path.c_str()); + ASSERT_TRUE(std::filesystem::exists(tmpFile.path)); + std::filesystem::remove(tmpFile.path); EXPECT_EQ( action.apply( @@ -200,12 +200,12 @@ TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescriptionFileContent) { using namespace util::config; - std::ofstream file(tmpFile.path.c_str()); + std::ofstream file(tmpFile.path); ASSERT_TRUE(file.is_open()); ClioConfigDescription::writeConfigDescriptionToFile(file); file.close(); - std::ifstream inFile(tmpFile.path.c_str()); + std::ifstream inFile(tmpFile.path); ASSERT_TRUE(inFile.is_open()); std::stringstream buffer; @@ -221,5 +221,5 @@ TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescriptionFileContent) for (auto const& key : gClioConfig) EXPECT_TRUE(fileContent.find(key.first)); - std::filesystem::remove(tmpFile.path.c_str()); + std::filesystem::remove(tmpFile.path); } From bc16c43f4c56074b2a22864d2ac3e54b9244d738 Mon Sep 17 00:00:00 2001 From: Peter Chen Date: Wed, 5 Feb 2025 22:43:08 -0500 Subject: [PATCH 14/14] fix comments --- src/util/newconfig/ConfigDescription.hpp | 18 +++++++----------- tests/unit/app/CliArgsTests.cpp | 3 --- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/util/newconfig/ConfigDescription.hpp b/src/util/newconfig/ConfigDescription.hpp index 9a500d2c0..52fbc6d06 100644 --- a/src/util/newconfig/ConfigDescription.hpp +++ b/src/util/newconfig/ConfigDescription.hpp @@ -82,27 +82,23 @@ struct ClioConfigDescription { { namespace fs = std::filesystem; - // Construct the full file path - fs::path const fullFilePath = path.string(); - // Validate the directory exists - auto const dir = fs::path(fullFilePath).parent_path(); - if (!dir.empty() && !fs::exists(dir)) + auto const dir = path.parent_path(); + if (!dir.empty() && !fs::exists(dir)) { return std::unexpected{ - fmt::format("Error: Directory {} does not exist or provided path is invalid", dir.string()) + fmt::format("Error: Directory '{}' does not exist or provided path is invalid", dir.string()) }; + } - std::ofstream file(fullFilePath); + std::ofstream file(path.string()); if (!file.is_open()) { - return std::unexpected{ - fmt::format("Failed to create file '{}': {}", fullFilePath.string(), std::strerror(errno)) - }; + return std::unexpected{fmt::format("Failed to create file '{}': {}", path.string(), std::strerror(errno))}; } writeConfigDescriptionToFile(file); file.close(); - std::cout << "Markdown file generated successfully: " << fullFilePath << "\n"; + std::cout << "Markdown file generated successfully: " << path << "\n"; return {}; } diff --git a/tests/unit/app/CliArgsTests.cpp b/tests/unit/app/CliArgsTests.cpp index ec52763fc..fd5ecb5f2 100644 --- a/tests/unit/app/CliArgsTests.cpp +++ b/tests/unit/app/CliArgsTests.cpp @@ -183,7 +183,6 @@ TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescription) // user provide config markdown file name as well ASSERT_TRUE(std::filesystem::exists(tmpFile.path)); - std::filesystem::remove(tmpFile.path); EXPECT_EQ( action.apply( @@ -220,6 +219,4 @@ TEST_F(CliArgsTestsWithTmpFile, Parse_ConfigDescriptionFileContent) // all keys that exist in clio config should be listed in config description file for (auto const& key : gClioConfig) EXPECT_TRUE(fileContent.find(key.first)); - - std::filesystem::remove(tmpFile.path); }