diff --git a/include/sharg/parser.hpp b/include/sharg/parser.hpp index 75679838..0015711d 100644 --- a/include/sharg/parser.hpp +++ b/include/sharg/parser.hpp @@ -359,7 +359,6 @@ class parser * \throws sharg::too_many_arguments if the command line call contained more arguments than expected. * \throws sharg::too_few_arguments if the command line call contained less arguments than expected. * \throws sharg::validation_error if the argument was not excepted by the provided validator. - * \throws sharg::user_input_error if a subparser was configured at construction but a subcommand is missing. * * \details * @@ -419,63 +418,22 @@ class parser if (parse_was_called) throw design_error("The function parse() must only be called once!"); - // Before creating the detail::version_checker, we have to make sure that - // malicious code cannot be injected through the app name. - if (!std::regex_match(info.app_name, app_name_regex)) - { - throw design_error{("The application name must only contain alpha-numeric characters or '_' and '-' " - "(regex: \"^[a-zA-Z0-9_-]+$\").")}; - } - - for (auto & sub : this->subcommands) - { - if (!std::regex_match(sub, app_name_regex)) - { - throw design_error{"The subcommand name must only contain alpha-numeric characters or '_' and '-' " - "(regex: \"^[a-zA-Z0-9_-]+$\")."}; - } - } + parse_was_called = true; - detail::version_checker app_version{info.app_name, info.version, info.url}; + // User input sanitization must happen before version check! + verify_app_and_subcommand_names(); init(); - if (std::holds_alternative(format) && !subcommands.empty() && sub_parser == nullptr) - { - assert(!subcommands.empty()); - std::string subcommands_str{"["}; - for (std::string const & command : subcommands) - subcommands_str += command + ", "; - subcommands_str.replace(subcommands_str.size() - 2, 2, "]"); // replace last ", " by "]" - - throw too_few_arguments{"You misspelled the subcommand! Please specify which sub-program " - "you want to use: one of " - + subcommands_str - + ". Use -h/--help for more " - "information."}; - } - - if (app_version.decide_if_check_is_performed(version_check_dev_decision, version_check_user_decision)) - { - // must be done before calling parse on the format because this might std::exit - std::promise app_version_prom; - version_check_future = app_version_prom.get_future(); - app_version(std::move(app_version_prom)); - } - + // Apply all defered operations to the parser, e.g., `add_option`, `add_flag`, `add_positional_option`. for (auto & operation : operations) operation(); - std::visit( - [this](T & f) - { - if constexpr (std::same_as) - f.parse(info, executable_name); - else - f.parse(info); - }, - format); - parse_was_called = true; + // The version check, which might exit the program, must be called before calling parse on the format. + run_version_check(); + + // Parse the command line arguments. + parse_format(); // Exit after parsing any special format. if (!std::holds_alternative(format)) @@ -1059,6 +1017,92 @@ class parser if (parse_was_called) throw design_error{detail::to_string(function_name.data(), " may only be used before calling parse().")}; } + + /*!\brief Verifies that the app and subcommand names are correctly formatted. + * \throws sharg::design_error if the app name is not correctly formatted. + * \throws sharg::design_error if the subcommand names are not correctly formatted. + * \details + * The app name must only contain alphanumeric characters, '_', or '-'. + * The subcommand names must only contain alphanumeric characters, '_', or '-'. + */ + inline void verify_app_and_subcommand_names() const + { + // Before creating the detail::version_checker, we have to make sure that + // malicious code cannot be injected through the app name. + if (!std::regex_match(info.app_name, app_name_regex)) + { + throw design_error{("The application name must only contain alpha-numeric characters or '_' and '-' " + "(regex: \"^[a-zA-Z0-9_-]+$\").")}; + } + + for (auto & sub : this->subcommands) + { + if (!std::regex_match(sub, app_name_regex)) + { + throw design_error{"The subcommand name must only contain alpha-numeric characters or '_' and '-' " + "(regex: \"^[a-zA-Z0-9_-]+$\")."}; + } + } + } + + /*!\brief Runs the version check if the user has not disabled it. + * \details + * If the user has not disabled the version check, the function will start a detached thread that will call the + * sharg::detail::version_checker and print a message if a new version is available. + */ + inline void run_version_check() + { + detail::version_checker app_version{info.app_name, info.version, info.url}; + + if (app_version.decide_if_check_is_performed(version_check_dev_decision, version_check_user_decision)) + { + // must be done before calling parse on the format because this might std::exit + std::promise app_version_prom; + version_check_future = app_version_prom.get_future(); + app_version(std::move(app_version_prom)); + } + } + + /*!\brief Parses the command line arguments according to the format. + * \throws sharg::option_declared_multiple_times if an option that is not a list was declared multiple times. + * \throws sharg::user_input_error if an incorrect argument is given as (positional) option value. + * \throws sharg::required_option_missing if the user did not provide a required option. + * \throws sharg::too_many_arguments if the command line call contained more arguments than expected. + * \throws sharg::too_few_arguments if the command line call contained less arguments than expected. + * \throws sharg::validation_error if the argument was not excepted by the provided validator. + * \details + * This function calls the parse function of the format member variable. + */ + inline void parse_format() + { + auto format_parse_fn = [this](format_t & f) + { + if constexpr (std::same_as) + f.parse(info, executable_name); + else + f.parse(info); + }; + + try + { + std::visit(std::move(format_parse_fn), format); + } + catch (sharg::too_many_arguments const & exception) + { + if (subcommands.empty() || sub_parser) + throw; + + std::string message = exception.what(); + message += "\nIf you intended to execute a subcommand, it is possible that you have misspelled it.\n" + "Available subcommands are: ["; + + for (std::string const & command : subcommands) + message += command + ", "; + message.replace(message.size() - 2, 2, "].\n"); // replace last ", " by "].\n" + + throw too_many_arguments{message}; + } + } }; } // namespace sharg diff --git a/test/unit/parser/format_parse_test.cpp b/test/unit/parser/format_parse_test.cpp index 934ed407..702dde5e 100644 --- a/test/unit/parser/format_parse_test.cpp +++ b/test/unit/parser/format_parse_test.cpp @@ -680,17 +680,43 @@ TEST_F(format_parse_test, subcommand_parser_success) TEST_F(format_parse_test, subcommand_parser_error) { + auto parser = get_parser(); + + constexpr std::string_view expected_message = + "Too many arguments provided. Please see -h/--help for more information.\n" + "If you intended to execute a subcommand, it is possible that you have misspelled it.\n" + "Available subcommands are: [sub1].\n"; + + auto check_error = [&parser, &expected_message](char const * file, int line) + { + ::testing::ScopedTrace trace(file, line, "check_error"); + try + { + parser.parse(); + FAIL(); + } + catch (sharg::too_many_arguments const & exception) + { + std::string_view const actual_message{exception.what()}; + EXPECT_EQ(expected_message, actual_message); + } + catch (...) + { + FAIL(); + } + }; + // incorrect sub command regardless of following arguments, https://github.com/seqan/seqan3/issues/2172 - auto top_level_parser = get_subcommand_parser({"subiddysub", "-f"}, {"sub1"}); - EXPECT_THROW(top_level_parser.parse(), sharg::parser_error); + parser = get_subcommand_parser({"subiddysub", "-f"}, {"sub1"}); + check_error(__FILE__, __LINE__); // incorrect sub command with no other arguments - top_level_parser = get_subcommand_parser({"subiddysub"}, {"sub1"}); - EXPECT_THROW(top_level_parser.parse(), sharg::parser_error); + parser = get_subcommand_parser({"subiddysub"}, {"sub1"}); + check_error(__FILE__, __LINE__); // incorrect sub command with trailing special option, https://github.com/seqan/sharg-parser/issues/171 - top_level_parser = get_subcommand_parser({"subiddysub", "-h"}, {"sub1"}); - EXPECT_THROW(top_level_parser.parse(), sharg::parser_error); + parser = get_subcommand_parser({"subiddysub", "-h"}, {"sub1"}); + check_error(__FILE__, __LINE__); } TEST_F(format_parse_test, issue1544) diff --git a/test/unit/parser/parser_design_error_test.cpp b/test/unit/parser/parser_design_error_test.cpp index 96c8cd98..c4f69da6 100644 --- a/test/unit/parser/parser_design_error_test.cpp +++ b/test/unit/parser/parser_design_error_test.cpp @@ -263,25 +263,27 @@ TEST_F(design_error_test, subcommand_parser_error) EXPECT_THROW(parser.parse(), sharg::design_error); // no positional options are allowed - parser = get_subcommand_parser({"foo"}, {"foo"}); + parser = get_subcommand_parser({"-f", "foo"}, {"foo"}); EXPECT_THROW(parser.add_positional_option(flag_value, sharg::config{}), sharg::design_error); EXPECT_NO_THROW(parser.add_option(flag_value, sharg::config{.short_id = 'o'})); EXPECT_NO_THROW(parser.add_flag(flag_value, sharg::config{.short_id = 'f'})); - EXPECT_EQ(flag_value, false); - EXPECT_THROW(parser.get_sub_parser(), sharg::design_error); + EXPECT_EQ(flag_value, false); EXPECT_NO_THROW(parser.parse()); // Prints nothing, but sets sub_parser. EXPECT_NO_THROW(parser.get_sub_parser()); + EXPECT_EQ(flag_value, true); + + flag_value = false; // options are allowed parser = get_subcommand_parser({"-o", "true"}, {"foo"}); EXPECT_THROW(parser.add_positional_option(flag_value, sharg::config{}), sharg::design_error); EXPECT_NO_THROW(parser.add_option(flag_value, sharg::config{.short_id = 'o'})); - EXPECT_NO_THROW(parser.add_flag(flag_value, sharg::config{.short_id = 'f'})); EXPECT_EQ(flag_value, false); - EXPECT_THROW(parser.parse(), sharg::user_input_error); // Todo: Subcommand is required? + EXPECT_NO_THROW(parser.parse()); + EXPECT_EQ(flag_value, true); } TEST_F(design_error_test, not_allowed_after_parse)