Skip to content

Commit 43d1cf3

Browse files
rennoxrennox
committed
BUG#35068427 Mysqlsh doesn't read socket from cnf if I provide any arg on the command line
The CLI argument mapper was using Null values to define missing optional arguments (ignoring any pre-defined default value), this causes problems when Null is not a valid value for the target parameter. This patch fixes the problem by using the optional parameter default values if availeble, otherwise uses: - Empty dictionary for missing dictionary arguments - Empty list for missing list arguments - Null for any other missing arguments Change-Id: I98e1ce1f55a5a859fb34a5fb1aba488b66334eff
1 parent c690dc0 commit 43d1cf3

File tree

8 files changed

+279
-78
lines changed

8 files changed

+279
-78
lines changed

‎modules/util/mod_util.cc

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,9 +159,14 @@ None Util::check_for_server_upgrade(dict options);
159159
#endif
160160

161161
void Util::check_for_server_upgrade(
162-
const mysqlshdk::db::Connection_options &connection_options,
162+
const std::optional<mysqlshdk::db::Connection_options> &connection_options,
163163
const shcore::Option_pack_ref<Upgrade_check_options> &options) {
164-
auto connection = connection_options;
164+
mysqlshdk::db::Connection_options connection;
165+
166+
if (connection_options.has_value()) {
167+
connection = *connection_options;
168+
}
169+
165170
if (connection.has_data()) {
166171
if (options->password.has_value()) {
167172
if (connection.has_password()) connection.clear_password();

‎modules/util/mod_util.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#define MODULES_UTIL_MOD_UTIL_H_
2626

2727
#include <memory>
28+
#include <optional>
2829
#include <string>
2930
#include <vector>
3031

@@ -78,8 +79,8 @@ class SHCORE_PUBLIC Util : public Extensible_object {
7879
None check_for_server_upgrade(dict options);
7980
#endif
8081
void check_for_server_upgrade(
81-
const mysqlshdk::db::Connection_options &connection_options =
82-
mysqlshdk::db::Connection_options(),
82+
const std::optional<mysqlshdk::db::Connection_options>
83+
&connection_options = {},
8384
const shcore::Option_pack_ref<Upgrade_check_options> &options = {});
8485

8586
#if DOXYGEN_JS

‎mysqlshdk/shellcore/shell_cli_mapper.cc

Lines changed: 91 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2020, 2022, Oracle and/or its affiliates.
2+
* Copyright (c) 2020, 2023, Oracle and/or its affiliates.
33
*
44
* This program is free software; you can redistribute it and/or modify
55
* it under the terms of the GNU General Public License, version 2.0,
@@ -553,11 +553,15 @@ Provider *Shell_cli_mapper::identify_operation(Provider *base_provider) {
553553
}
554554

555555
void Shell_cli_mapper::add_argument(const shcore::Value &argument) {
556-
for (size_t index = 0; index < m_missing_optionals; index++) {
557-
m_argument_list->push_back(shcore::Value::Null());
558-
}
556+
add_default_arguments();
559557
m_argument_list->push_back(argument);
560-
m_missing_optionals = 0;
558+
m_missing_optionals.clear();
559+
}
560+
561+
void Shell_cli_mapper::add_default_arguments() {
562+
for (auto &missing_optional : m_missing_optionals) {
563+
m_argument_list->push_back(std::move(missing_optional));
564+
}
561565
}
562566

563567
/**
@@ -675,13 +679,21 @@ void Shell_cli_mapper::process_positional_args() {
675679
m_cmdline_args.erase(m_cmdline_args.begin());
676680
} else {
677681
if (connection_data->empty()) {
678-
// If the parameter is empty, inserts a null if not optional,
679-
// otherwise counts it as parameters to be added in case a further
680-
// parameter has value
682+
// If a connection parameter is empty CLI determines the value to
683+
// use as follows:
684+
// - If required: Uses Null
685+
// - If optional, Caches it's default value if any, or Null
681686
if (m_metadata->signature[index]->flag ==
682-
shcore::Param_flag::Optional)
683-
m_missing_optionals++;
684-
else
687+
shcore::Param_flag::Optional) {
688+
if (m_metadata->signature[index]->def_value.type !=
689+
shcore::Value_type::Undefined) {
690+
m_missing_optionals.push_back(
691+
m_metadata->signature[index]->def_value);
692+
693+
} else {
694+
m_missing_optionals.push_back(shcore::Value::Null());
695+
}
696+
} else
685697
add_argument(shcore::Value::Null());
686698
} else {
687699
add_argument(shcore::Value(connection_data));
@@ -691,15 +703,19 @@ void Shell_cli_mapper::process_positional_args() {
691703
case 'D': {
692704
const auto &options = get_argument_options(param_name).as_map();
693705

694-
if (options->empty()) {
695-
// If the parameter is empty, inserts a null if not optional,
696-
// otherwise counts it as parameters to be added in case a further
697-
// parameter has value
698-
if (m_metadata->signature[index]->flag ==
699-
shcore::Param_flag::Optional)
700-
m_missing_optionals++;
701-
else
702-
add_argument(shcore::Value::Null());
706+
if (options->empty() && m_metadata->signature[index]->flag ==
707+
shcore::Param_flag::Optional) {
708+
// If a dictionary parameter is empty CLI determines the value to
709+
// use as follows:
710+
// - If required: Uses an empty dictionary
711+
// - If optional, Caches it's default value if any, or an empty dict
712+
if (m_metadata->signature[index]->def_value.type !=
713+
shcore::Value_type::Undefined) {
714+
m_missing_optionals.push_back(
715+
m_metadata->signature[index]->def_value);
716+
} else {
717+
m_missing_optionals.push_back(shcore::Value(options));
718+
}
703719
} else {
704720
add_argument(shcore::Value(options));
705721
}
@@ -709,35 +725,46 @@ void Shell_cli_mapper::process_positional_args() {
709725
shcore::Array_t list_argument;
710726
if (has_argument_options(param_name)) {
711727
list_argument = get_argument_options(param_name).as_array();
712-
} else if (!m_cmdline_args.empty()) {
728+
} else {
713729
list_argument = shcore::make_array();
714-
auto validator =
715-
m_metadata->signature[index]->validator<List_validator>();
716-
717-
// All remaining anonymous arguments are added to the list parameter
718-
while (!m_cmdline_args.empty()) {
719-
// Only anonymous arguments are added to the list
720-
if (m_cmdline_args[0].option.empty()) {
721-
if (m_cmdline_args[0].value.type == shcore::Array) {
722-
validate_list_items(m_cmdline_args[0],
730+
if (!m_cmdline_args.empty()) {
731+
auto validator =
732+
m_metadata->signature[index]->validator<List_validator>();
733+
734+
// All remaining anonymous arguments are added to the list parameter
735+
while (!m_cmdline_args.empty()) {
736+
// Only anonymous arguments are added to the list
737+
if (m_cmdline_args[0].option.empty()) {
738+
if (m_cmdline_args[0].value.type == shcore::Array) {
739+
validate_list_items(m_cmdline_args[0],
740+
validator->element_type());
741+
auto source = m_cmdline_args[0].value.as_array();
742+
std::move(source->begin(), source->end(),
743+
std::back_inserter(*list_argument));
744+
} else {
745+
add_value_to_list(&list_argument, m_cmdline_args[0],
723746
validator->element_type());
724-
auto source = m_cmdline_args[0].value.as_array();
725-
std::move(source->begin(), source->end(),
726-
std::back_inserter(*list_argument));
727-
} else {
728-
add_value_to_list(&list_argument, m_cmdline_args[0],
729-
validator->element_type());
747+
}
730748
}
731-
}
732749

733-
m_cmdline_args.erase(m_cmdline_args.begin());
750+
m_cmdline_args.erase(m_cmdline_args.begin());
751+
}
734752
}
735753
}
736754

737-
if ((!list_argument || list_argument->empty()) &&
738-
m_metadata->signature[index]->flag ==
739-
shcore::Param_flag::Optional) {
740-
m_missing_optionals++;
755+
if (list_argument->empty() && m_metadata->signature[index]->flag ==
756+
shcore::Param_flag::Optional) {
757+
// If a list parameter is empty CLI determines the value to use as
758+
// follows:
759+
// - If required: Uses an empty list
760+
// - If optional, Caches it's default value if any, or an empty list
761+
if (m_metadata->signature[index]->def_value.type !=
762+
shcore::Value_type::Undefined) {
763+
m_missing_optionals.push_back(
764+
m_metadata->signature[index]->def_value);
765+
} else {
766+
m_missing_optionals.push_back(shcore::Value(list_argument));
767+
}
741768
} else {
742769
add_argument(shcore::Value(list_argument));
743770
}
@@ -753,10 +780,24 @@ void Shell_cli_mapper::process_positional_args() {
753780
m_cmdline_args.erase(m_cmdline_args.begin());
754781
}
755782

783+
// If any other parameter is undefined CLI determines the value to use
784+
// as follows:
785+
// - If required: Uses Null
786+
// - If optional, Caches it's default value if any, or Null
787+
// list
788+
756789
if (value.type == shcore::Undefined) {
757790
if (m_metadata->signature[index]->flag ==
758791
shcore::Param_flag::Optional) {
759-
m_missing_optionals++;
792+
if (m_metadata->signature[index]->def_value.type !=
793+
shcore::Value_type::Undefined) {
794+
m_missing_optionals.push_back(
795+
m_metadata->signature[index]->def_value);
796+
} else {
797+
m_missing_optionals.push_back(shcore::Value::Null());
798+
}
799+
} else {
800+
add_argument(shcore::Value::Null());
760801
}
761802
} else {
762803
add_argument(value);
@@ -809,7 +850,7 @@ void Shell_cli_mapper::clear() {
809850
m_argument_options.clear();
810851
m_cli_option_registry.clear();
811852
m_normalized_cli_options.clear();
812-
m_missing_optionals = 0;
853+
m_missing_optionals.clear();
813854
m_waiting_value = false;
814855
m_help_type = Help_type::NONE;
815856
m_cmdline_args.clear();
@@ -927,13 +968,16 @@ void Shell_cli_mapper::define_named_args() {
927968
m_argument_options[param_name] = shcore::Value(shcore::make_dict());
928969
} break;
929970
case 'A':
971+
// If a single array parameter is defined, its items can not be handled
972+
// as named args, however, any further list are handled as named
973+
// parameters
930974
if (!found_list) {
931975
found_list = true;
932976
} else {
933977
define_named_arg(param_name, "", "",
934978
m_metadata->signature[index]->flag ==
935979
shcore::Param_flag::Mandatory);
936-
m_argument_options[param_name] = shcore::Value(shcore::Array_t());
980+
m_argument_options[param_name] = shcore::Value::new_array();
937981
}
938982
break;
939983
default:
@@ -1008,6 +1052,8 @@ void Shell_cli_mapper::process_arguments(shcore::Argument_list *argument_list) {
10081052

10091053
process_named_args();
10101054
process_positional_args();
1055+
// Adds any remaining argument with a default value
1056+
add_default_arguments();
10111057
}
10121058

10131059
} // namespace cli

‎mysqlshdk/shellcore/shell_cli_mapper.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2020, 2021, Oracle and/or its affiliates.
2+
* Copyright (c) 2020, 2023, Oracle and/or its affiliates.
33
*
44
* This program is free software; you can redistribute it and/or modify
55
* it under the terms of the GNU General Public License, version 2.0,
@@ -122,6 +122,7 @@ class Shell_cli_mapper {
122122

123123
private:
124124
void add_argument(const shcore::Value &argument);
125+
void add_default_arguments();
125126
void define_named_arg(const Cli_option &option);
126127
void define_named_arg(const std::string &param, const std::string &option,
127128
const std::string &short_option, bool required);
@@ -170,9 +171,9 @@ class Shell_cli_mapper {
170171
// Definition of the command line arguments specified by the user
171172
std::vector<Cmd_line_arg_definition> m_cmdline_args;
172173

173-
// Counter of optional arguments that has been skipped before a new argument
174-
// is specified, used to fill the missing args with NULLs
175-
size_t m_missing_optionals;
174+
// Stores the default values for optional arguments that has been skipped
175+
// before a new argument is specified, used to fill the missing args
176+
std::vector<shcore::Value> m_missing_optionals;
176177

177178
// Used when adding cmdline_args, for the case when the value for a named
178179
// argument comes in a separate cmd line arg.

‎unittest/scripts/auto/py_shell/scripts/plugin_cli_integration_norecord.py

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,5 +208,87 @@ def print_more_nested_info(param_a):
208208
call_mysqlsh(["--", "cli_tester", "emptyChild", "grandChild", "print-more-nested-info", "Failure"])
209209
EXPECT_STDOUT_CONTAINS("ERROR: Invalid operation for grandChild object: print-more-nested-info")
210210

211+
212+
213+
#@<> CLI mapper properly takes default values from plugin optional parameters
214+
# NOTE: The default values for the print_defaults function differ with the ones
215+
# registered in the plugin on purpose, this will be used to demonstrate who is
216+
# passing the default values to the call.
217+
# In general they would never differ, and indeed, this inconsistency is not possible
218+
# when using the plugin and plugin_function decorators.
219+
plugin_code = """
220+
def print_defaults(one=1, two="python default", three={"option1": "python option1 default"}, four={"option2": "python option2 default"}):
221+
print(one, two, three, four)
222+
223+
obj = shell.create_extension_object()
224+
shell.add_extension_object_member(obj, "printDefaults", print_defaults, {
225+
"brief": "Testing default values on plugins.",
226+
"cli": True,
227+
"parameters":[
228+
{
229+
"name": "one",
230+
"brief": "int parameter",
231+
"type": "integer",
232+
"default": 2,
233+
"required": False
234+
},
235+
{
236+
"name": "two",
237+
"brief": "string parameter",
238+
"type": "string",
239+
"default": "plugin default",
240+
"required": False
241+
},
242+
{
243+
"name": "three",
244+
"brief": "optional dictionary parameter",
245+
"type": "dictionary",
246+
"options": [
247+
{
248+
"name": "option1",
249+
"type": "string",
250+
}
251+
],
252+
"default": {"option1": "plugin option1 default"},
253+
"required": False
254+
},
255+
{
256+
"name": "four",
257+
"brief": "optional dictionary parameter",
258+
"type": "dictionary",
259+
"required": False,
260+
"options": [
261+
{
262+
"name": "option2",
263+
"type": "string",
264+
}
265+
],
266+
"default": {"option2": "plugin option2 default"},
267+
}
268+
]
269+
})
270+
271+
shell.register_global('cli_tester', obj, {"brief": "CLI Integration Testing Plugin"})
272+
"""
273+
274+
testutil.rmfile(plugin_path)
275+
testutil.create_file(plugin_path, plugin_code)
276+
277+
#@<> Using all default values
278+
rc = call_mysqlsh(["--", "cli_tester", "print-defaults"])
279+
EXPECT_STDOUT_CONTAINS("2 plugin default {\"option1\": \"plugin option1 default\"} {\"option2\": \"plugin option2 default\"}")
280+
WIPE_OUTPUT()
281+
282+
#@<> Using explicit value for 'three', CLI mapper handles defaults for 'one' and 'two'
283+
rc = call_mysqlsh(["--", "cli_tester", "print-defaults", "--option1", "explicit value for option1"])
284+
EXPECT_STDOUT_CONTAINS("2 plugin default {\"option1\": \"explicit value for option1\"} {\"option2\": \"plugin option2 default\"}")
285+
WIPE_OUTPUT()
286+
287+
#@<> CLI mapper handles defaults for 'one', 'two' and 'three'
288+
rc = call_mysqlsh(["--", "cli_tester", "print-defaults", "--option2", "explicit value for option2"])
289+
EXPECT_STDOUT_CONTAINS("2 plugin default {\"option1\": \"plugin option1 default\"} {\"option2\": \"explicit value for option2\"}")
290+
WIPE_OUTPUT()
291+
292+
211293
#@<> Finalization
212294
testutil.rmdir(plugins_path, True)

0 commit comments

Comments
 (0)