Remove CommandDefinition class In preparation for traits support, remove CommandDefinition class and incorporate the missing functionality into CommandDictionary. BUG: 25841719 Change-Id: Iead48aa0503e9de6061c4c1588b0b930dd82c8d0 Reviewed-on: https://weave-review.googlesource.com/1680 Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/libweave.gypi b/libweave.gypi index a02f082..c330927 100644 --- a/libweave.gypi +++ b/libweave.gypi
@@ -7,7 +7,6 @@ 'src/backoff_entry.cc', 'src/base_api_handler.cc', 'src/commands/cloud_command_proxy.cc', - 'src/commands/command_definition.cc', 'src/commands/command_dictionary.cc', 'src/commands/command_instance.cc', 'src/commands/command_manager.cc', @@ -60,7 +59,6 @@ 'src/backoff_entry_unittest.cc', 'src/base_api_handler_unittest.cc', 'src/commands/cloud_command_proxy_unittest.cc', - 'src/commands/command_definition_unittest.cc', 'src/commands/command_dictionary_unittest.cc', 'src/commands/command_instance_unittest.cc', 'src/commands/command_manager_unittest.cc',
diff --git a/src/base_api_handler_unittest.cc b/src/base_api_handler_unittest.cc index 15a575a..92b83a7 100644 --- a/src/base_api_handler_unittest.cc +++ b/src/base_api_handler_unittest.cc
@@ -107,8 +107,8 @@ }; TEST_F(BaseApiHandlerTest, Initialization) { - auto command_defs = - command_manager_->GetCommandDictionary().GetCommandsAsJson(nullptr); + const auto& command_defs = + command_manager_->GetCommandDictionary().GetCommandsAsJson(); auto expected = R"({ "base": { @@ -143,7 +143,7 @@ } } })"; - EXPECT_JSON_EQ(expected, *command_defs); + EXPECT_JSON_EQ(expected, command_defs); } TEST_F(BaseApiHandlerTest, UpdateBaseConfiguration) {
diff --git a/src/commands/cloud_command_proxy_unittest.cc b/src/commands/cloud_command_proxy_unittest.cc index a65a967..99ddffa 100644 --- a/src/commands/cloud_command_proxy_unittest.cc +++ b/src/commands/cloud_command_proxy_unittest.cc
@@ -80,6 +80,7 @@ auto json = CreateDictionaryValue(R"({ 'calc': { 'add': { + 'minimalRole': 'user', 'parameters': { 'value1': 'integer', 'value2': 'integer'
diff --git a/src/commands/command_definition.cc b/src/commands/command_definition.cc deleted file mode 100644 index dbae630..0000000 --- a/src/commands/command_definition.cc +++ /dev/null
@@ -1,58 +0,0 @@ -// Copyright 2015 The Weave Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "src/commands/command_definition.h" - -#include <vector> - -#include <weave/error.h> -#include <weave/enum_to_string.h> - -#include "src/commands/schema_constants.h" -#include "src/string_utils.h" - -namespace weave { - -namespace { - -const EnumToStringMap<UserRole>::Map kMap[] = { - {UserRole::kViewer, commands::attributes::kCommand_Role_Viewer}, - {UserRole::kUser, commands::attributes::kCommand_Role_User}, - {UserRole::kOwner, commands::attributes::kCommand_Role_Owner}, - {UserRole::kManager, commands::attributes::kCommand_Role_Manager}, -}; -} - -template <> -LIBWEAVE_EXPORT EnumToStringMap<UserRole>::EnumToStringMap() - : EnumToStringMap(kMap) {} - -CommandDefinition::CommandDefinition(const base::DictionaryValue& definition, - UserRole minimal_role) - : minimal_role_{minimal_role} { - definition_.MergeDictionary(&definition); -} - -std::unique_ptr<CommandDefinition> CommandDefinition::FromJson( - const base::DictionaryValue& dict, ErrorPtr* error) { - std::unique_ptr<CommandDefinition> definition; - // Validate the 'minimalRole' value if present. That's the only thing we - // care about so far. - std::string value; - UserRole minimal_role; - if (dict.GetString(commands::attributes::kCommand_Role, &value)) { - if (!StringToEnum(value, &minimal_role)) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidPropValue, - "Invalid role: '%s'", value.c_str()); - return definition; - } - } else { - minimal_role = UserRole::kUser; - } - definition.reset(new CommandDefinition{dict, minimal_role}); - return definition; -} - -} // namespace weave
diff --git a/src/commands/command_definition.h b/src/commands/command_definition.h deleted file mode 100644 index da02bf5..0000000 --- a/src/commands/command_definition.h +++ /dev/null
@@ -1,47 +0,0 @@ -// Copyright 2015 The Weave Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef LIBWEAVE_SRC_COMMANDS_COMMAND_DEFINITION_H_ -#define LIBWEAVE_SRC_COMMANDS_COMMAND_DEFINITION_H_ - -#include <memory> -#include <string> - -#include <base/macros.h> -#include <base/values.h> -#include <weave/error.h> - -namespace weave { - -enum class UserRole { - kViewer, - kUser, - kManager, - kOwner, -}; - -// A simple GCD command definition. This class contains the full object schema -// describing the command parameter types and constraints. -class CommandDefinition final { - public: - // Factory method to construct a command definition from a JSON dictionary. - static std::unique_ptr<CommandDefinition> FromJson( - const base::DictionaryValue& dict, ErrorPtr* error); - const base::DictionaryValue& ToJson() const { return definition_; } - // Returns the role required to execute command. - UserRole GetMinimalRole() const { return minimal_role_; } - - private: - CommandDefinition(const base::DictionaryValue& definition, - UserRole minimal_role); - - base::DictionaryValue definition_; - UserRole minimal_role_; - - DISALLOW_COPY_AND_ASSIGN(CommandDefinition); -}; - -} // namespace weave - -#endif // LIBWEAVE_SRC_COMMANDS_COMMAND_DEFINITION_H_
diff --git a/src/commands/command_definition_unittest.cc b/src/commands/command_definition_unittest.cc deleted file mode 100644 index 867d48f..0000000 --- a/src/commands/command_definition_unittest.cc +++ /dev/null
@@ -1,51 +0,0 @@ -// Copyright 2015 The Weave Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "src/commands/command_definition.h" - -#include <gtest/gtest.h> -#include <weave/test/unittest_utils.h> - -namespace weave { - -using test::CreateDictionaryValue; - -TEST(CommandDefinition, DefaultRole) { - auto params = CreateDictionaryValue(R"({ - 'parameters': { - 'height': 'integer', - 'jumpType': ['_withAirFlip', '_withSpin', '_withKick'] - }, - 'progress': {'progress': 'integer'}, - 'results': {'testResult': 'integer'} - })"); - auto def = CommandDefinition::FromJson(*params, nullptr); - EXPECT_EQ(UserRole::kUser, def->GetMinimalRole()); -} - -TEST(CommandDefinition, SpecifiedRole) { - auto params = CreateDictionaryValue(R"({ - 'parameters': {}, - 'progress': {}, - 'results': {}, - 'minimalRole': 'owner' - })"); - auto def = CommandDefinition::FromJson(*params, nullptr); - EXPECT_EQ(UserRole::kOwner, def->GetMinimalRole()); -} - -TEST(CommandDefinition, IncorrectRole) { - auto params = CreateDictionaryValue(R"({ - 'parameters': {}, - 'progress': {}, - 'results': {}, - 'minimalRole': 'foo' - })"); - ErrorPtr error; - auto def = CommandDefinition::FromJson(*params, &error); - EXPECT_EQ(nullptr, def.get()); - EXPECT_EQ("invalid_parameter_value", error->GetCode()); -} - -} // namespace weave
diff --git a/src/commands/command_dictionary.cc b/src/commands/command_dictionary.cc index 516961f..f6a409d 100644 --- a/src/commands/command_dictionary.cc +++ b/src/commands/command_dictionary.cc
@@ -4,42 +4,55 @@ #include "src/commands/command_dictionary.h" +#include <algorithm> + #include <base/values.h> #include <weave/enum_to_string.h> -#include "src/commands/command_definition.h" #include "src/commands/schema_constants.h" #include "src/string_utils.h" namespace weave { +namespace { +const EnumToStringMap<UserRole>::Map kMap[] = { + {UserRole::kViewer, commands::attributes::kCommand_Role_Viewer}, + {UserRole::kUser, commands::attributes::kCommand_Role_User}, + {UserRole::kOwner, commands::attributes::kCommand_Role_Owner}, + {UserRole::kManager, commands::attributes::kCommand_Role_Manager}, +}; +} // anonymous namespace + +template <> +LIBWEAVE_EXPORT EnumToStringMap<UserRole>::EnumToStringMap() + : EnumToStringMap(kMap) {} + bool CommandDictionary::LoadCommands(const base::DictionaryValue& json, ErrorPtr* error) { - CommandMap new_defs; - // |json| contains a list of nested objects with the following structure: // {"<pkg_name>": {"<cmd_name>": {"parameters": {object_schema}}, ...}, ...} - // Iterate over packages - base::DictionaryValue::Iterator package_iter(json); - while (!package_iter.IsAtEnd()) { - std::string package_name = package_iter.key(); - const base::DictionaryValue* package_value = nullptr; - if (!package_iter.value().GetAsDictionary(&package_value)) { + // Iterate over traits + base::DictionaryValue::Iterator trait_iter(json); + for (base::DictionaryValue::Iterator trait_iter(json); + !trait_iter.IsAtEnd(); trait_iter.Advance()) { + std::string trait_name = trait_iter.key(); + const base::DictionaryValue* trait_def = nullptr; + if (!trait_iter.value().GetAsDictionary(&trait_def)) { Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kTypeMismatch, - "Expecting an object for package '%s'", - package_name.c_str()); + "Expecting an object for trait '%s'", + trait_name.c_str()); return false; } - // Iterate over command definitions within the current package. - base::DictionaryValue::Iterator command_iter(*package_value); - while (!command_iter.IsAtEnd()) { + // Iterate over command definitions within the current trait. + for (base::DictionaryValue::Iterator command_iter(*trait_def); + !command_iter.IsAtEnd(); command_iter.Advance()) { std::string command_name = command_iter.key(); if (command_name.empty()) { Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kInvalidCommandName, - "Unnamed command encountered in package '%s'", - package_name.c_str()); + "Unnamed command encountered in trait '%s'", + trait_name.c_str()); return false; } const base::DictionaryValue* command_def_json = nullptr; @@ -50,77 +63,89 @@ command_name.c_str()); return false; } - // Construct the compound command name as "pkg_name.cmd_name". - std::string full_command_name = Join(".", package_name, command_name); - auto command_def = CommandDefinition::FromJson(*command_def_json, error); - if (!command_def) { + // Construct the compound command name as "trait_name.cmd_name". + std::string full_command_name = Join(".", trait_name, command_name); + + // Validate the 'minimalRole' value if present. That's the only thing we + // care about so far. + std::string value; + if (!command_def_json->GetString(commands::attributes::kCommand_Role, + &value)) { Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kInvalidMinimalRole, - "Error parsing command '%s'", + "Missing '%s' attribute for command '%s'", + commands::attributes::kCommand_Role, full_command_name.c_str()); return false; } - - new_defs.insert( - std::make_pair(full_command_name, std::move(command_def))); - command_iter.Advance(); + UserRole minimal_role; + if (!StringToEnum(value, &minimal_role)) { + Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, + errors::commands::kInvalidMinimalRole, + "Invalid role '%s' for command '%s'", value.c_str(), + full_command_name.c_str()); + return false; + } + // Check if we already have this command defined. + CHECK(!definitions_.Get(full_command_name, nullptr)) + << "Definition for command '" << full_command_name + << "' overrides an earlier definition"; + definitions_.Set(full_command_name, command_def_json->DeepCopy()); } - package_iter.Advance(); } - - // Verify that newly loaded command definitions do not override existing - // definitions in another category. This is unlikely, but we don't want to let - // one vendor daemon to define the same commands already handled by another - // daemon on the same device. - for (const auto& pair : new_defs) { - auto iter = definitions_.find(pair.first); - CHECK(iter == definitions_.end()) << "Definition for command '" - << pair.first - << "' overrides an earlier definition"; - } - - // Insert new definitions into the global map. - for (auto& pair : new_defs) - definitions_.insert(std::make_pair(pair.first, std::move(pair.second))); return true; } -std::unique_ptr<base::DictionaryValue> CommandDictionary::GetCommandsAsJson( - ErrorPtr* error) const { - std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue); - for (const auto& pair : definitions_) { - auto parts = SplitAtFirst(pair.first, ".", true); - const std::string& package_name = parts.first; - const std::string& command_name = parts.second; +const base::DictionaryValue& CommandDictionary::GetCommandsAsJson() const { + return definitions_; +} - base::DictionaryValue* package = nullptr; - if (!dict->GetDictionaryWithoutPathExpansion(package_name, &package)) { - // If this is the first time we encounter this package, create a JSON - // object for it. - package = new base::DictionaryValue; - dict->SetWithoutPathExpansion(package_name, package); - } - package->SetWithoutPathExpansion(command_name, - pair.second->ToJson().DeepCopy()); +size_t CommandDictionary::GetSize() const { + size_t size = 0; + base::DictionaryValue::Iterator trait_iter(definitions_); + while (!trait_iter.IsAtEnd()) { + std::string trait_name = trait_iter.key(); + const base::DictionaryValue* trait_def = nullptr; + CHECK(trait_iter.value().GetAsDictionary(&trait_def)); + size += trait_def->size(); + trait_iter.Advance(); } - return dict; + return size; } -const CommandDefinition* CommandDictionary::FindCommand( +const base::DictionaryValue* CommandDictionary::FindCommand( const std::string& command_name) const { - auto pair = definitions_.find(command_name); - return (pair != definitions_.end()) ? pair->second.get() : nullptr; -} - -CommandDefinition* CommandDictionary::FindCommand( - const std::string& command_name) { - auto pair = definitions_.find(command_name); - return (pair != definitions_.end()) ? pair->second.get() : nullptr; + const base::DictionaryValue* definition = nullptr; + // Make sure the |command_name| came in form of trait_name.command_name. + // For this, we just verify it has a single period in its name. + if (std::count(command_name.begin(), command_name.end(), '.') != 1) + return definition; + definitions_.GetDictionary(command_name, &definition); + return definition; } void CommandDictionary::Clear() { - definitions_.clear(); + definitions_.Clear(); +} + +bool CommandDictionary::GetMinimalRole(const std::string& command_name, + UserRole* minimal_role, + ErrorPtr* error) const { + const base::DictionaryValue* command_def = FindCommand(command_name); + if (!command_def) { + Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, + errors::commands::kInvalidCommandName, + "Command definition for '%s' not found", + command_name.c_str()); + return false; + } + std::string value; + // The JSON definition has been pre-validated already in LoadCommands, so + // just using CHECKs here. + CHECK(command_def->GetString(commands::attributes::kCommand_Role, &value)); + CHECK(StringToEnum(value, minimal_role)); + return true; } } // namespace weave
diff --git a/src/commands/command_dictionary.h b/src/commands/command_dictionary.h index 03e080a..12f7e40 100644 --- a/src/commands/command_dictionary.h +++ b/src/commands/command_dictionary.h
@@ -12,22 +12,23 @@ #include <vector> #include <base/macros.h> +#include <base/values.h> #include <weave/error.h> -#include "src/commands/command_definition.h" - -namespace base { -class Value; -class DictionaryValue; -} // namespace base - namespace weave { -// CommandDictionary is a wrapper around a map of command name and the -// corresponding command definition schema. The command name (the key in -// the map) is a compound name in a form of "package_name.command_name", -// where "package_name" is a name of command package such as "base", "printers", -// and others. So the full command name could be "base.reboot", for example. +enum class UserRole { + kViewer, + kUser, + kManager, + kOwner, +}; + +// CommandDictionary is a wrapper around a container of command definition +// schema. The command name is a compound name in a form of +// "trait_name.command_name", where "trait_name" is a name of command trait such +// as "base", "onOff", and others. So the full command name could be +// "base.reboot", for example. class CommandDictionary final { public: CommandDictionary() = default; @@ -41,24 +42,24 @@ ErrorPtr* error); // Converts all the command definitions to a JSON object for CDD/Device // draft. - // Returns empty unique_ptr in case of an error and fills in the additional - // error details in |error|. - std::unique_ptr<base::DictionaryValue> GetCommandsAsJson( - ErrorPtr* error) const; + const base::DictionaryValue& GetCommandsAsJson() const; // Returns the number of command definitions in the dictionary. - size_t GetSize() const { return definitions_.size(); } + size_t GetSize() const; // Checks if the dictionary has no command definitions. bool IsEmpty() const { return definitions_.empty(); } // Remove all the command definitions from the dictionary. void Clear(); // Finds a definition for the given command. - const CommandDefinition* FindCommand(const std::string& command_name) const; - CommandDefinition* FindCommand(const std::string& command_name); + const base::DictionaryValue* FindCommand( + const std::string& command_name) const; + // Determines the minimal role for the given command. Returns false if the + // command with given name is not found. + bool GetMinimalRole(const std::string& command_name, + UserRole* minimal_role, + ErrorPtr* error) const; private: - using CommandMap = std::map<std::string, std::unique_ptr<CommandDefinition>>; - - CommandMap definitions_; // List of all available command definitions. + base::DictionaryValue definitions_; // List of all available command defs. DISALLOW_COPY_AND_ASSIGN(CommandDictionary); };
diff --git a/src/commands/command_dictionary_unittest.cc b/src/commands/command_dictionary_unittest.cc index adae4ec..7c53935 100644 --- a/src/commands/command_dictionary_unittest.cc +++ b/src/commands/command_dictionary_unittest.cc
@@ -22,6 +22,7 @@ auto json = CreateDictionaryValue(R"({ 'robot': { 'jump': { + 'minimalRole': 'manager', 'parameters': { 'height': 'integer', '_jumpType': ['_withAirFlip', '_withSpin', '_withKick'] @@ -40,9 +41,11 @@ json = CreateDictionaryValue(R"({ 'base': { 'reboot': { + 'minimalRole': 'owner', 'parameters': {'delay': 'integer'} }, 'shutdown': { + 'minimalRole': 'user' } } })"); @@ -75,80 +78,65 @@ EXPECT_FALSE(dict.LoadCommands(*json, &error)); EXPECT_EQ("invalid_command_name", error->GetCode()); error.reset(); + + // No 'minimalRole'. + json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'parameters': {'delay': 'integer'} + } + } + })"); + EXPECT_FALSE(dict.LoadCommands(*json, &error)); + EXPECT_EQ("invalid_minimal_role", error->GetCode()); + error.reset(); + + // Invalid 'minimalRole'. + json = CreateDictionaryValue(R"({ + 'base': { + 'reboot': { + 'minimalRole': 'foo', + 'parameters': {'delay': 'integer'} + } + } + })"); + EXPECT_FALSE(dict.LoadCommands(*json, &error)); + EXPECT_EQ("invalid_minimal_role", error->GetCode()); + error.reset(); } TEST(CommandDictionaryDeathTest, LoadCommands_Redefine) { // Redefine commands. CommandDictionary dict; ErrorPtr error; - auto json = CreateDictionaryValue("{'robot':{'jump':{}}}"); + auto json = + CreateDictionaryValue("{'robot':{'jump':{'minimalRole': 'viewer'}}}"); dict.LoadCommands(*json, nullptr); ASSERT_DEATH(dict.LoadCommands(*json, &error), ".*Definition for command 'robot.jump' overrides an " "earlier definition"); } -TEST(CommandDictionary, GetCommandsAsJson) { - auto json = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': {'minimum': 10}}, - 'results': {} - } - }, - 'robot': { - '_jump': { - 'parameters': {'_height': 'integer'}, - 'minimalRole': 'user' - } - } - })"); - CommandDictionary dict; - dict.LoadCommands(*json, nullptr); - - json = dict.GetCommandsAsJson(nullptr); - ASSERT_NE(nullptr, json.get()); - auto expected = R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': {'minimum': 10}}, - 'results': {} - } - }, - 'robot': { - '_jump': { - 'parameters': {'_height': 'integer'}, - 'minimalRole': 'user' - } - } - })"; - EXPECT_JSON_EQ(expected, *json); -} - -TEST(CommandDictionary, LoadWithPermissions) { +TEST(CommandDictionary, GetMinimalRole) { CommandDictionary base_dict; auto json = CreateDictionaryValue(R"({ 'base': { 'command1': { - 'parameters': {}, - 'results': {} - }, - 'command2': { 'minimalRole': 'viewer', 'parameters': {}, 'results': {} }, - 'command3': { + 'command2': { 'minimalRole': 'user', 'parameters': {}, 'results': {} }, - 'command4': { + 'command3': { 'minimalRole': 'manager', 'parameters': {}, 'results': {} }, - 'command5': { + 'command4': { 'minimalRole': 'owner', 'parameters': {}, 'results': {} @@ -156,26 +144,16 @@ } })"); EXPECT_TRUE(base_dict.LoadCommands(*json, nullptr)); - - auto cmd = base_dict.FindCommand("base.command1"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); - - cmd = base_dict.FindCommand("base.command2"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ(UserRole::kViewer, cmd->GetMinimalRole()); - - cmd = base_dict.FindCommand("base.command3"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); - - cmd = base_dict.FindCommand("base.command4"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ(UserRole::kManager, cmd->GetMinimalRole()); - - cmd = base_dict.FindCommand("base.command5"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ(UserRole::kOwner, cmd->GetMinimalRole()); + UserRole role; + EXPECT_TRUE(base_dict.GetMinimalRole("base.command1", &role, nullptr)); + EXPECT_EQ(UserRole::kViewer, role); + EXPECT_TRUE(base_dict.GetMinimalRole("base.command2", &role, nullptr)); + EXPECT_EQ(UserRole::kUser, role); + EXPECT_TRUE(base_dict.GetMinimalRole("base.command3", &role, nullptr)); + EXPECT_EQ(UserRole::kManager, role); + EXPECT_TRUE(base_dict.GetMinimalRole("base.command4", &role, nullptr)); + EXPECT_EQ(UserRole::kOwner, role); + EXPECT_FALSE(base_dict.GetMinimalRole("base.command5", &role, nullptr)); } } // namespace weave
diff --git a/src/commands/command_instance.cc b/src/commands/command_instance.cc index 8328299..c6a9600 100644 --- a/src/commands/command_instance.cc +++ b/src/commands/command_instance.cc
@@ -9,7 +9,6 @@ #include <weave/error.h> #include <weave/export.h> -#include "src/commands/command_definition.h" #include "src/commands/command_dictionary.h" #include "src/commands/command_queue.h" #include "src/commands/schema_constants.h" @@ -65,12 +64,8 @@ CommandInstance::CommandInstance(const std::string& name, Command::Origin origin, - const CommandDefinition* command_definition, const base::DictionaryValue& parameters) - : name_{name}, - origin_{origin}, - command_definition_{command_definition} { - CHECK(command_definition_); + : name_{name}, origin_{origin} { parameters_.MergeDictionary(¶meters); } @@ -148,14 +143,12 @@ namespace { // Helper method to retrieve command parameters from the command definition -// object passed in as |json| and corresponding command definition schema -// specified in |command_def|. +// object passed in as |json|. // On success, returns |true| and the validated parameters and values through // |parameters|. Otherwise returns |false| and additional error information in // |error|. std::unique_ptr<base::DictionaryValue> GetCommandParameters( const base::DictionaryValue* json, - const CommandDefinition* command_def, ErrorPtr* error) { // Get the command parameters from 'parameters' property. std::unique_ptr<base::DictionaryValue> params; @@ -221,7 +214,7 @@ return instance; } - auto parameters = GetCommandParameters(json, command_def, error); + auto parameters = GetCommandParameters(json, error); if (!parameters) { Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, errors::commands::kCommandFailed, @@ -229,8 +222,7 @@ return instance; } - instance.reset( - new CommandInstance{command_name, origin, command_def, *parameters}); + instance.reset(new CommandInstance{command_name, origin, *parameters}); if (!command_id->empty()) instance->SetID(*command_id);
diff --git a/src/commands/command_instance.h b/src/commands/command_instance.h index 32a93a9..82df190 100644 --- a/src/commands/command_instance.h +++ b/src/commands/command_instance.h
@@ -21,7 +21,6 @@ namespace weave { -class CommandDefinition; class CommandDictionary; class CommandObserver; class CommandQueue; @@ -45,7 +44,6 @@ // their values specified in |parameters|. CommandInstance(const std::string& name, Command::Origin origin, - const CommandDefinition* command_definition, const base::DictionaryValue& parameters); ~CommandInstance() override; @@ -66,11 +64,6 @@ bool Abort(const Error* command_error, ErrorPtr* error) override; bool Cancel(ErrorPtr* error) override; - // Returns command definition. - const CommandDefinition* GetCommandDefinition() const { - return command_definition_; - } - // Parses a command instance JSON definition and constructs a CommandInstance // object, checking the JSON |value| against the command definition schema // found in command |dictionary|. On error, returns null unique_ptr and @@ -100,7 +93,6 @@ void DetachFromQueue() { observers_.Clear(); queue_ = nullptr; - command_definition_ = nullptr; } private: @@ -118,8 +110,6 @@ std::string name_; // The origin of the command, either "local" or "cloud". Command::Origin origin_ = Command::Origin::kLocal; - // Command definition. - const CommandDefinition* command_definition_{nullptr}; // Command parameters and their values. base::DictionaryValue parameters_; // Current command execution progress.
diff --git a/src/commands/command_instance_unittest.cc b/src/commands/command_instance_unittest.cc index fb8fe84..7c8aa2d 100644 --- a/src/commands/command_instance_unittest.cc +++ b/src/commands/command_instance_unittest.cc
@@ -22,12 +22,14 @@ auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { + 'minimalRole': 'user', 'parameters': {}, 'results': {} } }, 'robot': { 'jump': { + 'minimalRole': 'user', 'parameters': { 'height': { 'type': 'integer', @@ -43,6 +45,7 @@ 'results': {'testResult': 'integer'} }, 'speak': { + 'minimalRole': 'user', 'parameters': { 'phrase': { 'type': 'string', @@ -72,8 +75,7 @@ 'phrase': 'iPityDaFool', 'volume': 5 })"); - CommandInstance instance{"robot.speak", Command::Origin::kCloud, - dict_.FindCommand("robot.speak"), *params}; + CommandInstance instance{"robot.speak", Command::Origin::kCloud, *params}; EXPECT_TRUE( instance.Complete(*CreateDictionaryValue("{'foo': 239}"), nullptr)); @@ -85,18 +87,12 @@ instance.GetParameters()); EXPECT_JSON_EQ("{'foo': 239}", instance.GetResults()); - CommandInstance instance2{"base.reboot", - Command::Origin::kLocal, - dict_.FindCommand("base.reboot"), - {}}; + CommandInstance instance2{"base.reboot", Command::Origin::kLocal, {}}; EXPECT_EQ(Command::Origin::kLocal, instance2.GetOrigin()); } TEST_F(CommandInstanceTest, SetID) { - CommandInstance instance{"base.reboot", - Command::Origin::kLocal, - dict_.FindCommand("base.reboot"), - {}}; + CommandInstance instance{"base.reboot", Command::Origin::kLocal, {}}; instance.SetID("command_id"); EXPECT_EQ("command_id", instance.GetID()); }
diff --git a/src/commands/command_manager.cc b/src/commands/command_manager.cc index a64c8e5..8d4602b 100644 --- a/src/commands/command_manager.cc +++ b/src/commands/command_manager.cc
@@ -63,8 +63,11 @@ if (!command_instance) return false; - UserRole minimal_role = - command_instance->GetCommandDefinition()->GetMinimalRole(); + UserRole minimal_role; + if (!GetCommandDictionary().GetMinimalRole(command_instance->GetName(), + &minimal_role, error)) { + return false; + } if (role < minimal_role) { Error::AddToPrintf( error, FROM_HERE, errors::commands::kDomain, "access_denied",
diff --git a/src/commands/command_manager_unittest.cc b/src/commands/command_manager_unittest.cc index f0dc95d..303cafa 100644 --- a/src/commands/command_manager_unittest.cc +++ b/src/commands/command_manager_unittest.cc
@@ -24,10 +24,12 @@ const char kTestVendorCommands[] = R"({ "robot": { "_jump": { + "minimalRole": "user", "parameters": {"height": "integer"}, "results": {} }, "_speak": { + "minimalRole": "user", "parameters": {"phrase": "string"}, "results": {} } @@ -37,6 +39,7 @@ const char kTestTestCommands[] = R"({ "test": { "_yo": { + "minimalRole": "user", "parameters": {"name": "string"}, "results": {} } @@ -63,12 +66,14 @@ auto json_str = R"({ "base": { "reboot": { + "minimalRole": "user", "parameters": {"delay": "integer"}, "results": {} } }, "robot": { "_jump": { + "minimalRole": "user", "parameters": {"height": "integer"}, "results": {} }
diff --git a/src/commands/command_queue_unittest.cc b/src/commands/command_queue_unittest.cc index dc7290a..08e8102 100644 --- a/src/commands/command_queue_unittest.cc +++ b/src/commands/command_queue_unittest.cc
@@ -12,22 +12,17 @@ #include <base/memory/weak_ptr.h> #include <gtest/gtest.h> -#include "src/commands/command_definition.h" #include "src/string_utils.h" namespace weave { class CommandQueueTest : public testing::Test { public: - CommandQueueTest() { - command_definition_ = CommandDefinition::FromJson({}, nullptr); - } - std::unique_ptr<CommandInstance> CreateDummyCommandInstance( const std::string& name, const std::string& id) { std::unique_ptr<CommandInstance> cmd{new CommandInstance{ - name, Command::Origin::kLocal, command_definition_.get(), {}}}; + name, Command::Origin::kLocal, {}}}; cmd->SetID(id); return cmd; } @@ -40,9 +35,6 @@ } CommandQueue queue_; - - private: - std::unique_ptr<CommandDefinition> command_definition_; }; // Keeps track of commands being added to and removed from the queue_.
diff --git a/src/device_registration_info.cc b/src/device_registration_info.cc index 0c914b7..30e8862 100644 --- a/src/device_registration_info.cc +++ b/src/device_registration_info.cc
@@ -21,7 +21,6 @@ #include "src/bind_lambda.h" #include "src/commands/cloud_command_proxy.h" -#include "src/commands/command_definition.h" #include "src/commands/command_manager.h" #include "src/commands/schema_constants.h" #include "src/data_encoding.h" @@ -480,10 +479,8 @@ std::unique_ptr<base::DictionaryValue> DeviceRegistrationInfo::BuildDeviceResource(ErrorPtr* error) { // Limit only to commands that are visible to the cloud. - auto commands = - command_manager_->GetCommandDictionary().GetCommandsAsJson(error); - if (!commands) - return nullptr; + const base::DictionaryValue& commands = + command_manager_->GetCommandDictionary().GetCommandsAsJson(); const base::DictionaryValue& state = state_manager_->GetState(); @@ -505,7 +502,7 @@ channel->SetString("supportedType", "pull"); } resource->Set("channel", channel.release()); - resource->Set("commandDefs", commands.release()); + resource->Set("commandDefs", commands.DeepCopy()); resource->Set("state", state.DeepCopy()); return resource;
diff --git a/src/privet/cloud_delegate.cc b/src/privet/cloud_delegate.cc index c771366..139bfc6 100644 --- a/src/privet/cloud_delegate.cc +++ b/src/privet/cloud_delegate.cc
@@ -250,10 +250,9 @@ void OnCommandDefChanged() { command_defs_.Clear(); - auto commands = - command_manager_->GetCommandDictionary().GetCommandsAsJson(nullptr); - CHECK(commands); - command_defs_.MergeDictionary(commands.get()); + const base::DictionaryValue& commands = + command_manager_->GetCommandDictionary().GetCommandsAsJson(); + command_defs_.MergeDictionary(&commands); NotifyOnCommandDefsChanged(); }
diff --git a/src/weave_unittest.cc b/src/weave_unittest.cc index 2e41852..dd10cbe 100644 --- a/src/weave_unittest.cc +++ b/src/weave_unittest.cc
@@ -47,8 +47,11 @@ const char kCommandDefs[] = R"({ "base": { - "reboot": {}, + "reboot": { + "minimalRole": "user" + }, "_shutdown": { + "minimalRole": "user", "parameters": {}, "results": {} }