buffet: load standard GCD command definitions in CommandManager Changed CommandManager to load the base command definitions and use it as a base schema for loading device-specific commands and make sure device vendors are not redefining standard commands in breaking manner. BUG=chromium:374861 TEST=USE=buffet P2_TEST_FILTER="buffet::*" FEATURES=test emerge-link platform2 Change-Id: I5f2d5500bc90ef918a8a6df1242bdd1fe3b78615 Reviewed-on: https://chromium-review.googlesource.com/209175 Tested-by: Alex Vakulenko <avakulenko@chromium.org> Reviewed-by: Christopher Wiley <wiley@chromium.org> Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/command_dictionary.cc b/buffet/commands/command_dictionary.cc index 38c8bce..dcba8c9 100644 --- a/buffet/commands/command_dictionary.cc +++ b/buffet/commands/command_dictionary.cc
@@ -24,6 +24,7 @@ bool CommandDictionary::LoadCommands(const base::DictionaryValue& json, const std::string& category, + const CommandDictionary* base_commands, ErrorPtr* error) { std::map<std::string, std::shared_ptr<const CommandDefinition>> new_defs; @@ -32,29 +33,37 @@ // Iterate over packages base::DictionaryValue::Iterator package_iter(json); while (!package_iter.IsAtEnd()) { - std::string package = package_iter.key(); + std::string package_name = package_iter.key(); const base::DictionaryValue* package_value = nullptr; if (!package_iter.value().GetAsDictionary(&package_value)) { Error::AddToPrintf(error, errors::commands::kDomain, errors::commands::kTypeMismatch, "Expecting an object for package '%s'", - package.c_str()); + package_name.c_str()); return false; } // Iterate over command definitions within the current package. base::DictionaryValue::Iterator command_iter(*package_value); while (!command_iter.IsAtEnd()) { - std::string command = command_iter.key(); + std::string command_name = command_iter.key(); + if (command_name.empty()) { + Error::AddToPrintf(error, errors::commands::kDomain, + errors::commands::kInvalidCommandName, + "Unnamed command encountered in package '%s'", + package_name.c_str()); + return false; + } const base::DictionaryValue* command_value = nullptr; if (!command_iter.value().GetAsDictionary(&command_value)) { Error::AddToPrintf(error, errors::commands::kDomain, errors::commands::kTypeMismatch, "Expecting an object for command '%s'", - command.c_str()); + command_name.c_str()); return false; } // Construct the compound command name as "pkg_name.cmd_name". - std::string command_name = string_utils::Join('.', package, command); + std::string full_command_name = 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; @@ -63,21 +72,45 @@ Error::AddToPrintf(error, errors::commands::kDomain, errors::commands::kPropertyMissing, "Command definition '%s' is missing property '%s'", - command_name.c_str(), + full_command_name.c_str(), commands::attributes::kCommand_Parameters); return false; } + + const ObjectSchema* base_def = nullptr; + if (base_commands) { + const CommandDefinition* cmd = + base_commands->FindCommand(full_command_name); + if (cmd) + base_def = cmd->GetParameters().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 (command_name.front() != '_') { + Error::AddToPrintf(error, errors::commands::kDomain, + errors::commands::kInvalidCommandName, + "The name of custom command '%s' in package '%s'" + " must start with '_'", + command_name.c_str(), package_name.c_str()); + return false; + } + } + } + auto command_schema = std::make_shared<ObjectSchema>(); - if (!command_schema->FromJson(command_schema_def, nullptr, error)) { + if (!command_schema->FromJson(command_schema_def, base_def, error)) { Error::AddToPrintf(error, errors::commands::kDomain, errors::commands::kInvalidObjectSchema, "Invalid definition for command '%s'", - command_name.c_str()); + full_command_name.c_str()); return false; } auto command_def = std::make_shared<CommandDefinition>(category, command_schema); - new_defs.insert(std::make_pair(command_name, command_def)); + new_defs.insert(std::make_pair(full_command_name, command_def)); command_iter.Advance(); }
diff --git a/buffet/commands/command_dictionary.h b/buffet/commands/command_dictionary.h index 11295c9..78f2cee 100644 --- a/buffet/commands/command_dictionary.h +++ b/buffet/commands/command_dictionary.h
@@ -46,10 +46,14 @@ // When LoadCommands is called, all previous definitions of commands from the // same category are removed, effectively replacing all the commands in the // given category. + // Optional |base_commands| parameter specifies the definition of standard + // GCD commands for parameter schema validation. Can be set to nullptr if + // no validation is needed. // Returns false on failure and |error| provides additional error information // when provided. bool LoadCommands(const base::DictionaryValue& json, - const std::string& category, ErrorPtr* error); + const std::string& category, + const CommandDictionary* base_commands, ErrorPtr* error); // Returns the number of command definitions in the dictionary. size_t GetSize() const { return definitions_.size(); } // Checks if the dictionary has no command definitions.
diff --git a/buffet/commands/command_dictionary_unittest.cc b/buffet/commands/command_dictionary_unittest.cc index f345019..0aeab50 100644 --- a/buffet/commands/command_dictionary_unittest.cc +++ b/buffet/commands/command_dictionary_unittest.cc
@@ -28,7 +28,7 @@ } })"); buffet::CommandDictionary dict; - EXPECT_TRUE(dict.LoadCommands(*json, "robotd", nullptr)); + EXPECT_TRUE(dict.LoadCommands(*json, "robotd", nullptr, nullptr)); EXPECT_EQ(1, dict.GetSize()); EXPECT_NE(nullptr, dict.FindCommand("robot.jump")); json = CreateDictionaryValue(R"({ @@ -41,7 +41,7 @@ } } })"); - EXPECT_TRUE(dict.LoadCommands(*json, "powerd", nullptr)); + EXPECT_TRUE(dict.LoadCommands(*json, "powerd", nullptr, nullptr)); EXPECT_EQ(3, dict.GetSize()); EXPECT_NE(nullptr, dict.FindCommand("robot.jump")); EXPECT_NE(nullptr, dict.FindCommand("base.reboot")); @@ -57,7 +57,7 @@ // Command definition missing 'parameters' property. auto json = CreateDictionaryValue("{'robot':{'jump':{}}}"); - EXPECT_FALSE(dict.LoadCommands(*json, "robotd", &error)); + 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()); @@ -65,33 +65,103 @@ // Command definition is not an object. json = CreateDictionaryValue("{'robot':{'jump':0}}"); - EXPECT_FALSE(dict.LoadCommands(*json, "robotd", &error)); + EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); EXPECT_EQ("type_mismatch", error->GetCode()); EXPECT_EQ("Expecting an object for command 'jump'", error->GetMessage()); error.reset(); // Package definition is not an object. json = CreateDictionaryValue("{'robot':'blah'}"); - EXPECT_FALSE(dict.LoadCommands(*json, "robotd", &error)); + EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); EXPECT_EQ("type_mismatch", error->GetCode()); EXPECT_EQ("Expecting an object for package 'robot'", error->GetMessage()); error.reset(); // Invalid command definition is not an object. json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{'flip':0}}}}"); - EXPECT_FALSE(dict.LoadCommands(*json, "robotd", &error)); + 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()); EXPECT_NE(nullptr, error->GetInnerError()); // Must have additional info. error.reset(); + // Empty command name. + json = CreateDictionaryValue("{'robot':{'':{'parameters':{'flip':0}}}}"); + EXPECT_FALSE(dict.LoadCommands(*json, "robotd", nullptr, &error)); + EXPECT_EQ("invalid_command_name", error->GetCode()); + EXPECT_EQ("Unnamed command encountered in package 'robot'", + error->GetMessage()); + error.reset(); +} + +TEST(CommandDictionary, LoadCommands_RedefineInDifferentCategory) { // Redefine commands in different category. - json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{}}}}"); - dict.Clear(); - dict.LoadCommands(*json, "category1", &error); - EXPECT_FALSE(dict.LoadCommands(*json, "category2", &error)); + buffet::CommandDictionary dict; + buffet::ErrorPtr error; + auto json = CreateDictionaryValue("{'robot':{'jump':{'parameters':{}}}}"); + dict.LoadCommands(*json, "category1", nullptr, &error); + EXPECT_FALSE(dict.LoadCommands(*json, "category2", nullptr, &error)); EXPECT_EQ("duplicate_command_definition", error->GetCode()); EXPECT_EQ("Definition for command 'robot.jump' overrides an earlier " "definition in category 'category1'", error->GetMessage()); error.reset(); } + +TEST(CommandDictionary, LoadCommands_CustomCommandNaming) { + // Custom command must start with '_'. + buffet::CommandDictionary base_dict; + buffet::CommandDictionary dict; + buffet::ErrorPtr error; + auto json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + } + } + })"); + base_dict.LoadCommands(*json, "", nullptr, &error); + EXPECT_TRUE(dict.LoadCommands(*json, "robotd", &base_dict, &error)); + auto json2 = CreateDictionaryValue("{'base':{'jump':{'parameters':{}}}}"); + 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 " + "with '_'", error->GetMessage()); + error.reset(); + + // If the command starts with "_", then it's Ok. + json2 = CreateDictionaryValue("{'base':{'_jump':{'parameters':{}}}}"); + EXPECT_TRUE(dict.LoadCommands(*json2, "robotd", &base_dict, nullptr)); +} + +TEST(CommandDictionary, LoadCommands_RedefineStdCommand) { + // Redefine commands parameter type. + buffet::CommandDictionary base_dict; + buffet::CommandDictionary dict; + buffet::ErrorPtr error; + auto json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + } + } + })"); + base_dict.LoadCommands(*json, "", nullptr, &error); + auto json2 = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'string'} + } + } + })"); + EXPECT_FALSE(dict.LoadCommands(*json2, "robotd", &base_dict, &error)); + EXPECT_EQ("invalid_object_schema", error->GetCode()); + EXPECT_EQ("Invalid definition for command 'base.reboot'", + error->GetMessage()); + EXPECT_EQ("invalid_parameter_definition", error->GetInnerError()->GetCode()); + EXPECT_EQ("Error in definition of property 'delay'", + 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(); +}
diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc index fc04b2d..d7cf34c 100644 --- a/buffet/commands/command_manager.cc +++ b/buffet/commands/command_manager.cc
@@ -4,10 +4,78 @@ #include "buffet/commands/command_manager.h" +#include <base/file_util.h> +#include <base/values.h> +#include <base/json/json_reader.h> + +#include "buffet/commands/schema_constants.h" +#include "buffet/error_codes.h" + namespace buffet { const CommandDictionary& CommandManager::GetCommandDictionary() const { return dictionary_; } +bool CommandManager::LoadBaseCommands(const base::DictionaryValue& json, + ErrorPtr* error) { + return base_dictionary_.LoadCommands(json, "", nullptr, error); +} + +bool CommandManager::LoadBaseCommands(const base::FilePath& json_file_path, + ErrorPtr* error) { + std::unique_ptr<const base::DictionaryValue> json = LoadJsonDict( + json_file_path, error); + if (!json) + return false; + return LoadBaseCommands(*json, error); +} + +bool CommandManager::LoadCommands(const base::DictionaryValue& json, + const std::string& category, + ErrorPtr* error) { + return dictionary_.LoadCommands(json, category, &base_dictionary_, error); +} + +bool CommandManager::LoadCommands(const base::FilePath& json_file_path, + ErrorPtr* error) { + std::unique_ptr<const base::DictionaryValue> json = LoadJsonDict( + json_file_path, error); + if (!json) + return false; + std::string category = json_file_path.BaseName().RemoveExtension().value(); + return LoadCommands(*json, category, error); +} + +std::unique_ptr<const base::DictionaryValue> CommandManager::LoadJsonDict( + const base::FilePath& json_file_path, ErrorPtr* error) { + std::string json_string; + if (!base::ReadFileToString(json_file_path, &json_string)) { + Error::AddToPrintf(error, errors::file_system::kDomain, + errors::file_system::kFileReadError, + "Failed to read file '%s'", + json_file_path.value().c_str()); + return std::unique_ptr<const base::DictionaryValue>(); + } + std::string error_message; + base::Value* value = base::JSONReader::ReadAndReturnError( + json_string, base::JSON_PARSE_RFC, nullptr, &error_message); + if (!value) { + Error::AddToPrintf(error, errors::json::kDomain, errors::json::kParseError, + "Error parsing content of JSON file '%s': %s", + json_file_path.value().c_str(), error_message.c_str()); + return std::unique_ptr<const base::DictionaryValue>(); + } + const base::DictionaryValue* dict_value = nullptr; + if (!value->GetAsDictionary(&dict_value)) { + delete value; + Error::AddToPrintf(error, errors::json::kDomain, + errors::json::kObjectExpected, + "Content of file '%s' is not a JSON object", + json_file_path.value().c_str()); + return std::unique_ptr<const base::DictionaryValue>(); + } + return std::unique_ptr<const base::DictionaryValue>(dict_value); +} + } // namespace buffet
diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h index 1700712..64037b8 100644 --- a/buffet/commands/command_manager.h +++ b/buffet/commands/command_manager.h
@@ -5,7 +5,10 @@ #ifndef BUFFET_COMMANDS_COMMAND_MANAGER_H_ #define BUFFET_COMMANDS_COMMAND_MANAGER_H_ +#include <string> + #include <base/basictypes.h> +#include <base/files/file_path.h> #include "buffet/commands/command_dictionary.h" @@ -21,7 +24,42 @@ // Get the command definitions for the device. const CommandDictionary& GetCommandDictionary() const; + // Loads base/standard GCD command definitions. + // |json| is the full JSON schema of standard GCD commands. These commands + // are not necessarily supported by a particular device but rather + // all the standard commands defined by GCD standard for all known/supported + // device kinds. + // On success, returns true. Otherwise, |error| contains additional + // error information. + bool LoadBaseCommands(const base::DictionaryValue& json, + ErrorPtr* error); + + // Same as the overload above, but takes a path to a json file to read + // the base command definitions from. + bool LoadBaseCommands(const base::FilePath& json_file_path, + ErrorPtr* error); + + // Loads device command schema for particular category. + // See CommandDictionary::LoadCommands for detailed description of the + // parameters. + bool LoadCommands(const base::DictionaryValue& json, + const std::string& category, ErrorPtr* error); + + // Same as the overload above, but takes a path to a json file to read + // the base command definitions from. Also, the command category is + // derived from file name (without extension). So, if the path points to + // "power_manager.json", the command category used will be "power_manager". + bool LoadCommands(const base::FilePath& json_file_path, + ErrorPtr* error); + private: + // Helper function to load a JSON file that is expected to be + // an object/dictionary. In case of error, returns empty unique ptr and fills + // in error details in |error|. + std::unique_ptr<const base::DictionaryValue> LoadJsonDict( + const base::FilePath& json_file_path, ErrorPtr* error); + + CommandDictionary base_dictionary_; // Base/std command definitions/schemas. CommandDictionary dictionary_; // Command definitions/schemas. DISALLOW_COPY_AND_ASSIGN(CommandManager); };
diff --git a/buffet/commands/command_manager_unittest.cc b/buffet/commands/command_manager_unittest.cc index 1a0bfec..65241fe 100644 --- a/buffet/commands/command_manager_unittest.cc +++ b/buffet/commands/command_manager_unittest.cc
@@ -2,11 +2,107 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <base/file_util.h> +#include <base/json/json_writer.h> #include <gtest/gtest.h> #include "buffet/commands/command_manager.h" +#include "buffet/commands/unittest_utils.h" + +using buffet::unittests::CreateDictionaryValue; + +static base::FilePath SaveJsonToTempFile(const base::DictionaryValue& dict) { + std::string json; + base::JSONWriter::Write(&dict, &json); + base::FilePath temp_file; + base::CreateTemporaryFile(&temp_file); + base::WriteFile(temp_file, json.data(), static_cast<int>(json.size())); + return temp_file; +} TEST(CommandManager, Empty) { buffet::CommandManager manager; EXPECT_TRUE(manager.GetCommandDictionary().IsEmpty()); } + +TEST(CommandManager, LoadBaseCommandsJSON) { + buffet::CommandManager manager; + auto json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + }, + 'shutdown': { + 'parameters': {} + } + } + })"); + EXPECT_TRUE(manager.LoadBaseCommands(*json, nullptr)); +} + +TEST(CommandManager, LoadBaseCommandsFile) { + buffet::CommandManager manager; + auto json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + }, + 'shutdown': { + 'parameters': {} + } + } + })"); + base::FilePath temp_file = SaveJsonToTempFile(*json); + EXPECT_TRUE(manager.LoadBaseCommands(temp_file, nullptr)); + base::DeleteFile(temp_file, false); +} + +TEST(CommandManager, LoadCommandsJSON) { + buffet::CommandManager manager; + auto json = CreateDictionaryValue(R"({ + 'robot': { + '_jump': { + 'parameters': {'height': 'integer'} + }, + '_speak': { + 'parameters': {'phrase': 'string'} + } + } + })"); + EXPECT_TRUE(manager.LoadCommands(*json, "category", nullptr)); +} + +TEST(CommandManager, LoadCommandsFile) { + buffet::CommandManager manager; + // Load some standard command definitions first. + auto json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + }, + 'shutdown': { + 'parameters': {} + } + } + })"); + manager.LoadBaseCommands(*json, nullptr); + // Load device-supported commands. + json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + } + }, + 'robot': { + '_jump': { + 'parameters': {'height': 'integer'} + } + } + })"); + base::FilePath temp_file = SaveJsonToTempFile(*json); + EXPECT_TRUE(manager.LoadCommands(temp_file, nullptr)); + base::DeleteFile(temp_file, false); + EXPECT_EQ(2, manager.GetCommandDictionary().GetSize()); + EXPECT_NE(nullptr, manager.GetCommandDictionary().FindCommand("base.reboot")); + EXPECT_NE(nullptr, manager.GetCommandDictionary().FindCommand("robot._jump")); +}
diff --git a/buffet/commands/schema_constants.cc b/buffet/commands/schema_constants.cc index d1677c0..90c9faa 100644 --- a/buffet/commands/schema_constants.cc +++ b/buffet/commands/schema_constants.cc
@@ -21,6 +21,7 @@ const char kUnknownProperty[] = "unexpected_parameter"; const char kInvalidObjectSchema[] = "invalid_object_schema"; const char kDuplicateCommandDef[] = "duplicate_command_definition"; +const char kInvalidCommandName[] = "invalid_command_name"; } // namespace commands } // namespace errors
diff --git a/buffet/commands/schema_constants.h b/buffet/commands/schema_constants.h index 6a508e0..903228e 100644 --- a/buffet/commands/schema_constants.h +++ b/buffet/commands/schema_constants.h
@@ -24,6 +24,7 @@ extern const char kUnknownProperty[]; extern const char kInvalidObjectSchema[]; extern const char kDuplicateCommandDef[]; +extern const char kInvalidCommandName[]; } // namespace commands } // namespace errors