Add "component" property to command instance When sending commands, we'll use "component" to route the command to the target component it was designated for. As a temporary stop-gap, use "device" as the component name before we have full implementation of component/trait schema model. Also removed CommandDictionary from CommandInstance::FromJson since the validation will be done outside of JSON parsing code in the future Component Manager class. BUG: 25841719 Change-Id: I5c649c257fb48ecaaedc1ced84931009f94c2bb3 Reviewed-on: https://weave-review.googlesource.com/1764 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/include/weave/command.h b/include/weave/command.h index 0a7d545..91949f6 100644 --- a/include/weave/command.h +++ b/include/weave/command.h
@@ -33,6 +33,9 @@ // Returns the full name of the command. virtual const std::string& GetName() const = 0; + // Returns the full path to the component this command is intended for. + virtual const std::string& GetComponent() const = 0; + // Returns the command state. virtual Command::State GetState() const = 0;
diff --git a/include/weave/test/mock_command.h b/include/weave/test/mock_command.h index fe1a02a..181b9f5 100644 --- a/include/weave/test/mock_command.h +++ b/include/weave/test/mock_command.h
@@ -22,7 +22,7 @@ MOCK_CONST_METHOD0(GetID, const std::string&()); MOCK_CONST_METHOD0(GetName, const std::string&()); - MOCK_CONST_METHOD0(GetCategory, const std::string&()); + MOCK_CONST_METHOD0(GetComponent, const std::string&()); MOCK_CONST_METHOD0(GetState, Command::State()); MOCK_CONST_METHOD0(GetOrigin, Command::Origin()); MOCK_CONST_METHOD0(GetParameters, const base::DictionaryValue&());
diff --git a/src/base_api_handler_unittest.cc b/src/base_api_handler_unittest.cc index 92b83a7..ded6315 100644 --- a/src/base_api_handler_unittest.cc +++ b/src/base_api_handler_unittest.cc
@@ -72,12 +72,12 @@ void AddCommand(const std::string& command) { auto command_instance = CommandInstance::FromJson( test::CreateDictionaryValue(command.c_str()).get(), - Command::Origin::kLocal, command_manager_->GetCommandDictionary(), - nullptr, nullptr); + Command::Origin::kLocal, nullptr, nullptr); EXPECT_TRUE(!!command_instance); std::string id{base::IntToString(++command_id_)}; command_instance->SetID(id); + command_instance->SetComponent("device"); command_manager_->AddCommand(std::move(command_instance)); EXPECT_EQ(Command::State::kDone, command_manager_->FindCommand(id)->GetState());
diff --git a/src/commands/cloud_command_proxy_unittest.cc b/src/commands/cloud_command_proxy_unittest.cc index 99ddffa..c022b79 100644 --- a/src/commands/cloud_command_proxy_unittest.cc +++ b/src/commands/cloud_command_proxy_unittest.cc
@@ -76,28 +76,6 @@ EXPECT_CALL(state_change_queue_, GetLastStateChangeId()) .WillRepeatedly(testing::ReturnPointee(¤t_state_update_id_)); - // Set up the command schema. - auto json = CreateDictionaryValue(R"({ - 'calc': { - 'add': { - 'minimalRole': 'user', - 'parameters': { - 'value1': 'integer', - 'value2': 'integer' - }, - 'progress': { - 'status' : 'string' - }, - 'results': { - 'sum' : 'integer' - } - } - } - })"); - CHECK(json.get()); - CHECK(command_dictionary_.LoadCommands(*json, nullptr)) - << "Failed to parse test command dictionary"; - CreateCommandInstance(); } @@ -112,9 +90,8 @@ })"); CHECK(command_json.get()); - command_instance_ = - CommandInstance::FromJson(command_json.get(), Command::Origin::kCloud, - command_dictionary_, nullptr, nullptr); + command_instance_ = CommandInstance::FromJson( + command_json.get(), Command::Origin::kCloud, nullptr, nullptr); CHECK(command_instance_.get()); // Backoff - start at 1s and double with each backoff attempt and no jitter. @@ -139,7 +116,6 @@ testing::StrictMock<MockStateChangeQueueInterface> state_change_queue_; testing::StrictMock<provider::test::FakeTaskRunner> task_runner_; std::queue<base::Closure> task_queue_; - CommandDictionary command_dictionary_; std::unique_ptr<CommandInstance> command_instance_; };
diff --git a/src/commands/command_instance.cc b/src/commands/command_instance.cc index 1f2e4a2..702a819 100644 --- a/src/commands/command_instance.cc +++ b/src/commands/command_instance.cc
@@ -74,6 +74,10 @@ return name_; } +const std::string& CommandInstance::GetComponent() const { + return component_; +} + Command::State CommandInstance::GetState() const { return state_; } @@ -169,7 +173,6 @@ std::unique_ptr<CommandInstance> CommandInstance::FromJson( const base::Value* value, Command::Origin origin, - const CommandDictionary& dictionary, std::string* command_id, ErrorPtr* error) { std::unique_ptr<CommandInstance> instance; @@ -198,14 +201,6 @@ errors::commands::kPropertyMissing, "Command name is missing"); return instance; } - // Make sure we know how to handle the command with this name. - auto command_def = dictionary.FindCommand(command_name); - if (!command_def) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidCommandName, - "Unknown command received: %s", command_name.c_str()); - return instance; - } auto parameters = GetCommandParameters(json, error); if (!parameters) { @@ -220,6 +215,11 @@ if (!command_id->empty()) instance->SetID(*command_id); + // Get the component name this command is for. + std::string component; + if (json->GetString(commands::attributes::kCommand_Component, &component)) + instance->SetComponent(component); + return instance; }
diff --git a/src/commands/command_instance.h b/src/commands/command_instance.h index 82df190..15f3ac2 100644 --- a/src/commands/command_instance.h +++ b/src/commands/command_instance.h
@@ -50,6 +50,7 @@ // Command overrides. const std::string& GetID() const override; const std::string& GetName() const override; + const std::string& GetComponent() const override; Command::State GetState() const override; Command::Origin GetOrigin() const override; const base::DictionaryValue& GetParameters() const override; @@ -65,9 +66,8 @@ bool Cancel(ErrorPtr* error) override; // 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 - // fills in error details in |error|. + // object. + // On error, returns null unique_ptr and fills in error details in |error|. // |command_id| is the ID of the command returned, as parsed from the |value|. // The command ID extracted (if present in the JSON object) even if other // parsing/validation error occurs and command instance is not constructed. @@ -75,7 +75,6 @@ static std::unique_ptr<CommandInstance> FromJson( const base::Value* value, Command::Origin origin, - const CommandDictionary& dictionary, std::string* command_id, ErrorPtr* error); @@ -84,6 +83,7 @@ // Sets the command ID (normally done by CommandQueue when the command // instance is added to it). void SetID(const std::string& id) { id_ = id; } + void SetComponent(const std::string& component) { component_ = component; } void AddObserver(Observer* observer); void RemoveObserver(Observer* observer); @@ -106,8 +106,10 @@ // Unique command ID within a command queue. std::string id_; - // Full command name as "<package_name>.<command_name>". + // Full command name as "<trait_name>.<command_name>". std::string name_; + // Full path to the component this command is intended for. + std::string component_; // The origin of the command, either "local" or "cloud". Command::Origin origin_ = Command::Origin::kLocal; // Command parameters and their values.
diff --git a/src/commands/command_instance_unittest.cc b/src/commands/command_instance_unittest.cc index 7c8aa2d..6f55ad2 100644 --- a/src/commands/command_instance_unittest.cc +++ b/src/commands/command_instance_unittest.cc
@@ -14,63 +14,7 @@ using test::CreateDictionaryValue; using test::CreateValue; -namespace { - -class CommandInstanceTest : public ::testing::Test { - protected: - void SetUp() override { - auto json = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'minimalRole': 'user', - 'parameters': {}, - 'results': {} - } - }, - 'robot': { - 'jump': { - 'minimalRole': 'user', - 'parameters': { - 'height': { - 'type': 'integer', - 'minimum': 0, - 'maximum': 100 - }, - '_jumpType': { - 'type': 'string', - 'enum': ['_withAirFlip', '_withSpin', '_withKick'] - } - }, - 'progress': {'progress': 'integer'}, - 'results': {'testResult': 'integer'} - }, - 'speak': { - 'minimalRole': 'user', - 'parameters': { - 'phrase': { - 'type': 'string', - 'enum': ['beamMeUpScotty', 'iDontDigOnSwine', - 'iPityDaFool', 'dangerWillRobinson'] - }, - 'volume': { - 'type': 'integer', - 'minimum': 0, - 'maximum': 10 - } - }, - 'results': {'foo': 'integer'} - } - } - })"); - CHECK(dict_.LoadCommands(*json, nullptr)) - << "Failed to parse test command dictionary"; - } - CommandDictionary dict_; -}; - -} // anonymous namespace - -TEST_F(CommandInstanceTest, Test) { +TEST(CommandInstanceTest, Test) { auto params = CreateDictionaryValue(R"({ 'phrase': 'iPityDaFool', 'volume': 5 @@ -91,15 +35,16 @@ EXPECT_EQ(Command::Origin::kLocal, instance2.GetOrigin()); } -TEST_F(CommandInstanceTest, SetID) { +TEST(CommandInstanceTest, SetID) { CommandInstance instance{"base.reboot", Command::Origin::kLocal, {}}; instance.SetID("command_id"); EXPECT_EQ("command_id", instance.GetID()); } -TEST_F(CommandInstanceTest, FromJson) { +TEST(CommandInstanceTest, FromJson) { auto json = CreateDictionaryValue(R"({ 'name': 'robot.jump', + 'component': 'comp1.comp2', 'id': 'abcd', 'parameters': { 'height': 53, @@ -109,64 +54,56 @@ })"); std::string id; auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, &id, nullptr); + &id, nullptr); EXPECT_EQ("abcd", id); EXPECT_EQ("abcd", instance->GetID()); EXPECT_EQ("robot.jump", instance->GetName()); + EXPECT_EQ("comp1.comp2", instance->GetComponent()); EXPECT_JSON_EQ("{'height': 53, '_jumpType': '_withKick'}", instance->GetParameters()); } -TEST_F(CommandInstanceTest, FromJson_ParamsOmitted) { +TEST(CommandInstanceTest, FromJson_ParamsOmitted) { auto json = CreateDictionaryValue("{'name': 'base.reboot'}"); auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, nullptr); + nullptr, nullptr); EXPECT_EQ("base.reboot", instance->GetName()); EXPECT_JSON_EQ("{}", instance->GetParameters()); } -TEST_F(CommandInstanceTest, FromJson_NotObject) { +TEST(CommandInstanceTest, FromJson_NotObject) { auto json = CreateValue("'string'"); ErrorPtr error; auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, &error); + nullptr, &error); EXPECT_EQ(nullptr, instance.get()); EXPECT_EQ("json_object_expected", error->GetCode()); } -TEST_F(CommandInstanceTest, FromJson_NameMissing) { +TEST(CommandInstanceTest, FromJson_NameMissing) { auto json = CreateDictionaryValue("{'param': 'value'}"); ErrorPtr error; auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, &error); + nullptr, &error); EXPECT_EQ(nullptr, instance.get()); EXPECT_EQ("parameter_missing", error->GetCode()); } -TEST_F(CommandInstanceTest, FromJson_UnknownCommand) { - auto json = CreateDictionaryValue("{'name': 'robot.scream'}"); - ErrorPtr error; - auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, &error); - EXPECT_EQ(nullptr, instance.get()); - EXPECT_EQ("invalid_command_name", error->GetCode()); -} - -TEST_F(CommandInstanceTest, FromJson_ParamsNotObject) { +TEST(CommandInstanceTest, FromJson_ParamsNotObject) { auto json = CreateDictionaryValue(R"({ 'name': 'robot.speak', 'parameters': 'hello' })"); ErrorPtr error; auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, &error); + nullptr, &error); EXPECT_EQ(nullptr, instance.get()); auto inner = error->GetInnerError(); EXPECT_EQ("json_object_expected", inner->GetCode()); EXPECT_EQ("command_failed", error->GetCode()); } -TEST_F(CommandInstanceTest, ToJson) { +TEST(CommandInstanceTest, ToJson) { auto json = CreateDictionaryValue(R"({ 'name': 'robot.jump', 'parameters': { @@ -176,7 +113,7 @@ 'results': {} })"); auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, nullptr); + nullptr, nullptr); EXPECT_TRUE(instance->SetProgress(*CreateDictionaryValue("{'progress': 15}"), nullptr)); EXPECT_TRUE(instance->SetProgress(*CreateDictionaryValue("{'progress': 15}"), @@ -198,13 +135,13 @@ *json, *converted); } -TEST_F(CommandInstanceTest, ToJsonError) { +TEST(CommandInstanceTest, ToJsonError) { auto json = CreateDictionaryValue(R"({ 'name': 'base.reboot', 'parameters': {} })"); auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud, - dict_, nullptr, nullptr); + nullptr, nullptr); instance->SetID("testId"); ErrorPtr error;
diff --git a/src/commands/command_manager.cc b/src/commands/command_manager.cc index 8d4602b..9e9852b 100644 --- a/src/commands/command_manager.cc +++ b/src/commands/command_manager.cc
@@ -57,9 +57,8 @@ UserRole role, std::string* id, ErrorPtr* error) { - auto command_instance = - CommandInstance::FromJson(&command, Command::Origin::kLocal, - GetCommandDictionary(), nullptr, error); + auto command_instance = CommandInstance::FromJson( + &command, Command::Origin::kLocal, nullptr, error); if (!command_instance) return false; @@ -76,6 +75,7 @@ return false; } + command_instance->SetComponent("device"); *id = std::to_string(++next_command_id_); command_instance->SetID(*id); AddCommand(std::move(command_instance)); @@ -101,7 +101,7 @@ const Device::CommandHandlerCallback& callback) { CHECK(command_name.empty() || dictionary_.FindCommand(command_name)) << "Command undefined: " << command_name; - command_queue_.AddCommandHandler(command_name, callback); + command_queue_.AddCommandHandler("device", command_name, callback); } } // namespace weave
diff --git a/src/commands/command_manager.h b/src/commands/command_manager.h index 04f49ee..644c165 100644 --- a/src/commands/command_manager.h +++ b/src/commands/command_manager.h
@@ -44,10 +44,6 @@ // Returns the command definitions for the device. const CommandDictionary& GetCommandDictionary() const; - // Same as the overload above, but takes a path to a json file to read - // the base command definitions from. - bool LoadStandardCommands(const std::string& json, ErrorPtr* error); - // Loads device command schema. bool LoadCommands(const base::DictionaryValue& dict, ErrorPtr* error);
diff --git a/src/commands/command_queue.cc b/src/commands/command_queue.cc index 3d2167f..134dc1c 100644 --- a/src/commands/command_queue.cc +++ b/src/commands/command_queue.cc
@@ -11,6 +11,11 @@ namespace { const int kRemoveCommandDelayMin = 5; + +std::string GetCommandHandlerKey(const std::string& component_path, + const std::string& command_name) { + return component_path + ":" + command_name; +} } void CommandQueue::AddCommandAddedCallback(const CommandCallback& callback) { @@ -25,6 +30,7 @@ } void CommandQueue::AddCommandHandler( + const std::string& component_path, const std::string& command_name, const Device::CommandHandlerCallback& callback) { if (!command_name.empty()) { @@ -33,20 +39,24 @@ for (const auto& command : map_) { if (command.second->GetState() == Command::State::kQueued && - command.second->GetName() == command_name) { + command.second->GetName() == command_name && + command.second->GetComponent() == component_path) { callback.Run(command.second); } } - CHECK(command_callbacks_.insert(std::make_pair(command_name, callback)) - .second) + std::string key = GetCommandHandlerKey(component_path, command_name); + CHECK(command_callbacks_.insert(std::make_pair(key, callback)).second) << command_name << " already has handler"; } else { + CHECK(component_path.empty()) + << "Default handler must not be component-specific"; for (const auto& command : map_) { + std::string key = GetCommandHandlerKey(command.second->GetComponent(), + command.second->GetName()); if (command.second->GetState() == Command::State::kQueued && - command_callbacks_.find(command.second->GetName()) == - command_callbacks_.end()) { + command_callbacks_.find(key) == command_callbacks_.end()) { callback.Run(command.second); } } @@ -66,7 +76,9 @@ for (const auto& cb : on_command_added_) cb.Run(pair.first->second.get()); - auto it_handler = command_callbacks_.find(pair.first->second->GetName()); + std::string key = GetCommandHandlerKey(pair.first->second->GetComponent(), + pair.first->second->GetName()); + auto it_handler = command_callbacks_.find(key); if (it_handler != command_callbacks_.end()) it_handler->second.Run(pair.first->second);
diff --git a/src/commands/command_queue.h b/src/commands/command_queue.h index 74ed86f..0f0a18b 100644 --- a/src/commands/command_queue.h +++ b/src/commands/command_queue.h
@@ -34,7 +34,8 @@ // Adds notifications callback for a command is removed from the queue. void AddCommandRemovedCallback(const CommandCallback& callback); - void AddCommandHandler(const std::string& command_name, + void AddCommandHandler(const std::string& component_path, + const std::string& command_name, const Device::CommandHandlerCallback& callback); // Checks if the command queue is empty.
diff --git a/src/commands/schema_constants.cc b/src/commands/schema_constants.cc index cd8bf84..34d6db8 100644 --- a/src/commands/schema_constants.cc +++ b/src/commands/schema_constants.cc
@@ -26,6 +26,7 @@ const char kCommand_Id[] = "id"; const char kCommand_Name[] = "name"; +const char kCommand_Component[] = "component"; const char kCommand_Parameters[] = "parameters"; const char kCommand_Progress[] = "progress"; const char kCommand_Results[] = "results";
diff --git a/src/commands/schema_constants.h b/src/commands/schema_constants.h index 57766e6..9199480 100644 --- a/src/commands/schema_constants.h +++ b/src/commands/schema_constants.h
@@ -29,6 +29,7 @@ // Command description JSON schema attributes. extern const char kCommand_Id[]; extern const char kCommand_Name[]; +extern const char kCommand_Component[]; extern const char kCommand_Parameters[]; extern const char kCommand_Progress[]; extern const char kCommand_Results[];
diff --git a/src/device_registration_info.cc b/src/device_registration_info.cc index 33b1643..9469d09 100644 --- a/src/device_registration_info.cc +++ b/src/device_registration_info.cc
@@ -1103,8 +1103,7 @@ std::string command_id; ErrorPtr error; auto command_instance = CommandInstance::FromJson( - &command, Command::Origin::kCloud, - command_manager_->GetCommandDictionary(), &command_id, &error); + &command, Command::Origin::kCloud, &command_id, &error); if (!command_instance) { LOG(WARNING) << "Failed to parse a command instance: " << command; if (!command_id.empty())