buffet: Provide results definition in command definition. That allows us to provide typed results for supported commands. BUG=chromium:435607 TEST=cros_workon_make --test buffet Change-Id: I61b5a5b294b4d869366c821adf1ef7f6db31c7ea Reviewed-on: https://chromium-review.googlesource.com/231322 Reviewed-by: Anton Muhin <antonm@chromium.org> Commit-Queue: Anton Muhin <antonm@chromium.org> Tested-by: Anton Muhin <antonm@chromium.org>
diff --git a/buffet/commands/command_definition.cc b/buffet/commands/command_definition.cc index f24b22d..7494de8 100644 --- a/buffet/commands/command_definition.cc +++ b/buffet/commands/command_definition.cc
@@ -8,8 +8,9 @@ CommandDefinition::CommandDefinition( const std::string& category, - const std::shared_ptr<const ObjectSchema>& parameters) - : category_(category), parameters_(parameters) { + const std::shared_ptr<const ObjectSchema>& parameters, + const std::shared_ptr<const ObjectSchema>& results) + : category_(category), parameters_(parameters), results_(results) { } } // namespace buffet
diff --git a/buffet/commands/command_definition.h b/buffet/commands/command_definition.h index f02cf48..a7b793a 100644 --- a/buffet/commands/command_definition.h +++ b/buffet/commands/command_definition.h
@@ -22,7 +22,8 @@ class CommandDefinition { public: CommandDefinition(const std::string& category, - const std::shared_ptr<const ObjectSchema>& parameters); + const std::shared_ptr<const ObjectSchema>& parameters, + const std::shared_ptr<const ObjectSchema>& results); // Gets the category this command belongs to. const std::string& GetCategory() const { return category_; } @@ -30,10 +31,15 @@ const std::shared_ptr<const ObjectSchema>& GetParameters() const { return parameters_; } + // Gets the object schema for command results. + const std::shared_ptr<const ObjectSchema>& GetResults() const { + return results_; + } private: std::string category_; // Cmd category. Could be "powerd" for "base.reboot". - std::shared_ptr<const ObjectSchema> parameters_; // Command parameter def. + std::shared_ptr<const ObjectSchema> parameters_; // Command parameters def. + std::shared_ptr<const ObjectSchema> results_; // Command results def. DISALLOW_COPY_AND_ASSIGN(CommandDefinition); };
diff --git a/buffet/commands/command_definition_unittest.cc b/buffet/commands/command_definition_unittest.cc index f0adab1..0b06086 100644 --- a/buffet/commands/command_definition_unittest.cc +++ b/buffet/commands/command_definition_unittest.cc
@@ -8,7 +8,9 @@ TEST(CommandDefinition, Test) { auto params = std::make_shared<buffet::ObjectSchema>(); - buffet::CommandDefinition def("powerd", params); + auto results = std::make_shared<buffet::ObjectSchema>(); + buffet::CommandDefinition def("powerd", params, results); EXPECT_EQ("powerd", def.GetCategory()); EXPECT_EQ(params, def.GetParameters()); + EXPECT_EQ(results, def.GetResults()); }
diff --git a/buffet/commands/command_dictionary.cc b/buffet/commands/command_dictionary.cc index 98ffefc..930e044 100644 --- a/buffet/commands/command_dictionary.cc +++ b/buffet/commands/command_dictionary.cc
@@ -54,8 +54,8 @@ package_name.c_str()); return false; } - const base::DictionaryValue* command_value = nullptr; - if (!command_iter.value().GetAsDictionary(&command_value)) { + const base::DictionaryValue* command_def_json = nullptr; + if (!command_iter.value().GetAsDictionary(&command_def_json)) { chromeos::Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kTypeMismatch, @@ -66,32 +66,22 @@ // Construct the compound command name as "pkg_name.cmd_name". std::string full_command_name = chromeos::string_utils::Join( '.', package_name, command_name); - // Get the "parameters" definition of the command and read it into - // an object schema. - const base::DictionaryValue* command_schema_def = nullptr; - if (!command_value->GetDictionaryWithoutPathExpansion( - commands::attributes::kCommand_Parameters, &command_schema_def)) { - chromeos::Error::AddToPrintf( - error, FROM_HERE, errors::commands::kDomain, - errors::commands::kPropertyMissing, - "Command definition '%s' is missing property '%s'", - full_command_name.c_str(), - commands::attributes::kCommand_Parameters); - return false; - } - const ObjectSchema* base_def = nullptr; + const ObjectSchema* base_parameters_def = nullptr; + const ObjectSchema* base_results_def = nullptr; if (base_commands) { const CommandDefinition* cmd = base_commands->FindCommand(full_command_name); - if (cmd) - base_def = cmd->GetParameters().get(); + if (cmd) { + base_parameters_def = cmd->GetParameters().get(); + base_results_def = cmd->GetResults().get(); + } // If the base command dictionary was provided but the command was not // found in it, this must be a custom (vendor) command. GCD spec states // that all custom command names must begin with "_". Let's enforce // this rule here. - if (!base_def) { + if (!cmd) { if (command_name.front() != '_') { chromeos::Error::AddToPrintf( error, FROM_HERE, errors::commands::kDomain, @@ -104,17 +94,27 @@ } } - auto command_schema = std::make_shared<ObjectSchema>(); - if (!command_schema->FromJson(command_schema_def, base_def, error)) { - chromeos::Error::AddToPrintf(error, FROM_HERE, - errors::commands::kDomain, - errors::commands::kInvalidObjectSchema, - "Invalid definition for command '%s'", - full_command_name.c_str()); + auto parameters_schema = BuildObjectSchema( + command_def_json, + commands::attributes::kCommand_Parameters, + base_parameters_def, + full_command_name, + error); + if (!parameters_schema) return false; - } + + auto results_schema = BuildObjectSchema( + command_def_json, + commands::attributes::kCommand_Results, + base_results_def, + full_command_name, + error); + if (!results_schema) + return false; + auto command_def = std::make_shared<CommandDefinition>(category, - command_schema); + parameters_schema, + results_schema); new_defs.insert(std::make_pair(full_command_name, command_def)); command_iter.Advance(); @@ -150,6 +150,38 @@ return true; } +std::shared_ptr<ObjectSchema> CommandDictionary::BuildObjectSchema( + const base::DictionaryValue* command_def_json, + const char* property_name, + const ObjectSchema* base_def, + const std::string& command_name, + chromeos::ErrorPtr* error) { + auto object_schema = std::make_shared<ObjectSchema>(); + + const base::DictionaryValue* schema_def = nullptr; + if (!command_def_json->GetDictionaryWithoutPathExpansion(property_name, + &schema_def)) { + chromeos::Error::AddToPrintf( + error, FROM_HERE, errors::commands::kDomain, + errors::commands::kPropertyMissing, + "Command definition '%s' is missing property '%s'", + command_name.c_str(), + property_name); + return {}; + } + + if (!object_schema->FromJson(schema_def, base_def, error)) { + chromeos::Error::AddToPrintf(error, FROM_HERE, + errors::commands::kDomain, + errors::commands::kInvalidObjectSchema, + "Invalid definition for command '%s'", + command_name.c_str()); + return {}; + } + + return object_schema; +} + std::unique_ptr<base::DictionaryValue> CommandDictionary::GetCommandsAsJson( bool full_schema, chromeos::ErrorPtr* error) const { std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue);
diff --git a/buffet/commands/command_dictionary.h b/buffet/commands/command_dictionary.h index bd60986..c316fd5 100644 --- a/buffet/commands/command_dictionary.h +++ b/buffet/commands/command_dictionary.h
@@ -21,6 +21,7 @@ namespace buffet { class CommandDefinition; +class ObjectSchema; // CommandDictionary is a wrapper around a map of command name and the // corresponding command definition schema. The command name (the key in @@ -71,6 +72,13 @@ const CommandDefinition* FindCommand(const std::string& command_name) const; private: + std::shared_ptr<ObjectSchema> BuildObjectSchema( + const base::DictionaryValue* command_def_json, + const char* property_name, + const ObjectSchema* base_def, + const std::string& command_name, + chromeos::ErrorPtr* error); + using CommandMap = std::map<std::string, std::shared_ptr<const CommandDefinition>>;
diff --git a/buffet/commands/command_dictionary_unittest.cc b/buffet/commands/command_dictionary_unittest.cc index 20c1287..10e7ec9 100644 --- a/buffet/commands/command_dictionary_unittest.cc +++ b/buffet/commands/command_dictionary_unittest.cc
@@ -24,7 +24,8 @@ 'parameters': { 'height': 'integer', '_jumpType': ['_withAirFlip', '_withSpin', '_withKick'] - } + }, + 'results': {} } } })"); @@ -35,10 +36,12 @@ json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -57,13 +60,21 @@ chromeos::ErrorPtr error; // Command definition missing 'parameters' property. - auto json = CreateDictionaryValue("{'robot':{'jump':{}}}"); + auto json = CreateDictionaryValue("{'robot':{'jump':{'results':{}}}}"); EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); EXPECT_EQ("parameter_missing", error->GetCode()); EXPECT_EQ("Command definition 'robot.jump' is missing property 'parameters'", error->GetMessage()); error.reset(); + // Command definition missing 'results' property. + json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{}}}}"); + EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); + EXPECT_EQ("parameter_missing", error->GetCode()); + EXPECT_EQ("Command definition 'robot.jump' is missing property 'results'", + error->GetMessage()); + error.reset(); + // Command definition is not an object. json = CreateDictionaryValue("{'robot':{'jump':0}}"); EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); @@ -79,7 +90,8 @@ error.reset(); // Invalid command definition is not an object. - json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{'flip':0}}}}"); + json = CreateDictionaryValue( + "{'robot':{'jump':{'parameters':{'flip':0},'results':{}}}}"); EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); EXPECT_EQ("invalid_object_schema", error->GetCode()); EXPECT_EQ("Invalid definition for command 'robot.jump'", error->GetMessage()); @@ -87,7 +99,7 @@ error.reset(); // Empty command name. - json = CreateDictionaryValue("{'robot':{'':{'parameters':{'flip':0}}}}"); + json = CreateDictionaryValue("{'robot':{'':{'parameters':{},'results':{}}}}"); EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); EXPECT_EQ("invalid_command_name", error->GetCode()); EXPECT_EQ("Unnamed command encountered in package 'robot'", @@ -99,7 +111,8 @@ // Redefine commands in different category. buffet::CommandDictionary dict; chromeos::ErrorPtr error; - auto json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{}}}}"); + auto json = CreateDictionaryValue( + "{'robot':{'jump':{'parameters':{},'results':{}}}}"); dict.LoadCommands(*json, "category1", nullptr, &error); EXPECT_FALSE(dict.LoadCommands(*json, "category2", nullptr, &error)); EXPECT_EQ("duplicate_command_definition", error->GetCode()); @@ -116,13 +129,15 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} } } })"); base_dict.LoadCommands(*json, "", nullptr, &error); EXPECT_TRUE(dict.LoadCommands(*json, "robotd", &base_dict, &error)); - auto json2 = CreateDictionaryValue("{'base':{'jump':{'parameters':{}}}}"); + auto json2 = CreateDictionaryValue( + "{'base':{'jump':{'parameters':{},'results':{}}}}"); EXPECT_FALSE(dict.LoadCommands(*json2, "robotd", &base_dict, &error)); EXPECT_EQ("invalid_command_name", error->GetCode()); EXPECT_EQ("The name of custom command 'jump' in package 'base' must start " @@ -130,7 +145,8 @@ error.reset(); // If the command starts with "_", then it's Ok. - json2 = CreateDictionaryValue("{'base':{'_jump':{'parameters':{}}}}"); + json2 = CreateDictionaryValue( + "{'base':{'_jump':{'parameters':{},'results':{}}}}"); EXPECT_TRUE(dict.LoadCommands(*json2, "robotd", &base_dict, nullptr)); } @@ -142,15 +158,18 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {'version': 'integer'} } } })"); base_dict.LoadCommands(*json, "", nullptr, &error); + auto json2 = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'string'} + 'parameters': {'delay': 'string'}, + 'results': {'version': 'integer'} } } })"); @@ -165,16 +184,39 @@ EXPECT_EQ("Redefining a property of type integer as string", error->GetFirstError()->GetMessage()); error.reset(); + + auto json3 = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'}, + 'results': {'version': 'string'} + } + } + })"); + EXPECT_FALSE(dict.LoadCommands(*json3, "robotd", &base_dict, &error)); + EXPECT_EQ("invalid_object_schema", error->GetCode()); + EXPECT_EQ("Invalid definition for command 'base.reboot'", + error->GetMessage()); + // TODO(antonm): remove parameter from error below and use some generic. + EXPECT_EQ("invalid_parameter_definition", error->GetInnerError()->GetCode()); + EXPECT_EQ("Error in definition of property 'version'", + error->GetInnerError()->GetMessage()); + EXPECT_EQ("param_type_changed", error->GetFirstError()->GetCode()); + EXPECT_EQ("Redefining a property of type integer as string", + error->GetFirstError()->GetMessage()); + error.reset(); } TEST(CommandDictionary, GetCommandsAsJson) { auto json_base = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': {'maximum': 100}} + 'parameters': {'delay': {'maximum': 100}}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -184,12 +226,14 @@ auto json = buffet::unittests::CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': {'minimum': 10}} + 'parameters': {'delay': {'minimum': 10}}, + 'results': {} } }, 'robot': { '_jump': { - 'parameters': {'_height': 'integer'} + 'parameters': {'_height': 'integer'}, + 'results': {} } } })");
diff --git a/buffet/commands/command_instance_unittest.cc b/buffet/commands/command_instance_unittest.cc index b57de25..d5afc70 100644 --- a/buffet/commands/command_instance_unittest.cc +++ b/buffet/commands/command_instance_unittest.cc
@@ -21,7 +21,8 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {} + 'parameters': {}, + 'results': {} } }, 'robot': { @@ -36,7 +37,8 @@ 'type': 'string', 'enum': ['_withAirFlip', '_withSpin', '_withKick'] } - } + }, + 'results': {} }, 'speak': { 'parameters': { @@ -50,7 +52,8 @@ 'minimum': 0, 'maximum': 10 } - } + }, + 'results': {} } } })"); @@ -92,7 +95,8 @@ 'parameters': { 'height': 53, '_jumpType': '_withKick' - } + }, + 'results': {} })"); auto instance = buffet::CommandInstance::FromJson(json.get(), dict_, nullptr); EXPECT_EQ("robot.jump", instance->GetName());
diff --git a/buffet/commands/command_manager_unittest.cc b/buffet/commands/command_manager_unittest.cc index 37e3716..c40f2dc 100644 --- a/buffet/commands/command_manager_unittest.cc +++ b/buffet/commands/command_manager_unittest.cc
@@ -31,10 +31,12 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -46,10 +48,12 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -63,10 +67,12 @@ auto json = CreateDictionaryValue(R"({ 'robot': { '_jump': { - 'parameters': {'height': 'integer'} + 'parameters': {'height': 'integer'}, + 'results': {} }, '_speak': { - 'parameters': {'phrase': 'string'} + 'parameters': {'phrase': 'string'}, + 'results': {} } } })"); @@ -79,10 +85,12 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -91,12 +99,14 @@ json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} } }, 'robot': { '_jump': { - 'parameters': {'height': 'integer'} + 'parameters': {'height': 'integer'}, + 'results': {} } } })");
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc index af15c1d..ed272a1 100644 --- a/buffet/commands/dbus_command_dispatcher_unittest.cc +++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -73,10 +73,12 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })");
diff --git a/buffet/commands/dbus_command_proxy_unittest.cc b/buffet/commands/dbus_command_proxy_unittest.cc index 3cb907d..81d9827 100644 --- a/buffet/commands/dbus_command_proxy_unittest.cc +++ b/buffet/commands/dbus_command_proxy_unittest.cc
@@ -50,6 +50,7 @@ EXPECT_CALL(*bus_, AssertOnDBusThread()).Times(AnyNumber()); // Command instance. + // TODO(antonm): Test results. auto json = CreateDictionaryValue(R"({ 'robot': { 'jump': { @@ -63,7 +64,8 @@ 'type': 'string', 'enum': ['_withAirFlip', '_withSpin', '_withKick'] } - } + }, + 'results': {} } } })");
diff --git a/buffet/commands/schema_constants.cc b/buffet/commands/schema_constants.cc index faf58fb..a26c355 100644 --- a/buffet/commands/schema_constants.cc +++ b/buffet/commands/schema_constants.cc
@@ -46,6 +46,7 @@ const char kCommand_Id[] = "id"; const char kCommand_Name[] = "name"; const char kCommand_Parameters[] = "parameters"; +const char kCommand_Results[] = "results"; } // namespace attributes } // namespace commands
diff --git a/buffet/commands/schema_constants.h b/buffet/commands/schema_constants.h index d5a8c4e..6b06e2f 100644 --- a/buffet/commands/schema_constants.h +++ b/buffet/commands/schema_constants.h
@@ -50,6 +50,7 @@ extern const char kCommand_Id[]; extern const char kCommand_Name[]; extern const char kCommand_Parameters[]; +extern const char kCommand_Results[]; } // namespace attributes } // namespace commands
diff --git a/buffet/device_registration_info_unittest.cc b/buffet/device_registration_info_unittest.cc index 1ab5191..dd8ffbe 100644 --- a/buffet/device_registration_info_unittest.cc +++ b/buffet/device_registration_info_unittest.cc
@@ -316,10 +316,12 @@ auto json_base = buffet::unittests::CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': 'integer'} + 'parameters': {'delay': 'integer'}, + 'results': {} }, 'shutdown': { - 'parameters': {} + 'parameters': {}, + 'results': {} } } })"); @@ -327,12 +329,14 @@ auto json_cmds = buffet::unittests::CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': {'minimum': 10}} + 'parameters': {'delay': {'minimum': 10}}, + 'results': {} } }, 'robot': { '_jump': { - 'parameters': {'_height': 'integer'} + 'parameters': {'_height': 'integer'}, + 'results': {} } } })");
diff --git a/buffet/etc/buffet/commands/test.json b/buffet/etc/buffet/commands/test.json index 10f8297..f6201c7 100644 --- a/buffet/etc/buffet/commands/test.json +++ b/buffet/etc/buffet/commands/test.json
@@ -2,11 +2,15 @@ "base": { "reboot": { "parameters": { + }, + "results": { } }, "_jump": { "parameters": { "_height":"integer" + }, + "results": { } } }
diff --git a/buffet/etc/buffet/gcd.json b/buffet/etc/buffet/gcd.json index b488802..1f37cec 100644 --- a/buffet/etc/buffet/gcd.json +++ b/buffet/etc/buffet/gcd.json
@@ -2,6 +2,8 @@ "base": { "reboot": { "parameters": { + }, + "results": { } } }