Make CommandManager::AddCommand() usable for both cloud and local cases Add Command::Origin parameter to AddCommand as well as let it return the cloud command ID for the command instance. Change-Id: I694c72aba80bc0f0f240453bfab0d11e773d70aa Reviewed-on: https://weave-review.googlesource.com/1786 Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/src/component_manager.h b/src/component_manager.h index 426a4cc..31f9228 100644 --- a/src/component_manager.h +++ b/src/component_manager.h
@@ -82,15 +82,10 @@ virtual void AddComponentTreeChangedCallback( const base::Closure& callback) = 0; - // Adds a new command instance to the command queue. The command specified in - // |command_instance| must be fully initialized and have its name, component, - // id populated. - virtual void AddCommand( - std::unique_ptr<CommandInstance> command_instance) = 0; - // Parses the command definition from a json dictionary and adds it to the // command queue. The new command ID is returned through optional |id| param. virtual bool AddCommand(const base::DictionaryValue& command, + Command::Origin command_origin, UserRole role, std::string* id, ErrorPtr* error) = 0;
diff --git a/src/component_manager_impl.cc b/src/component_manager_impl.cc index 6950969..8cfa95e 100644 --- a/src/component_manager_impl.cc +++ b/src/component_manager_impl.cc
@@ -147,17 +147,20 @@ callback.Run(); } -void ComponentManagerImpl::AddCommand( - std::unique_ptr<CommandInstance> command_instance) { - command_queue_.Add(std::move(command_instance)); -} - bool ComponentManagerImpl::AddCommand(const base::DictionaryValue& command, + Command::Origin command_origin, UserRole role, std::string* id, ErrorPtr* error) { - auto command_instance = CommandInstance::FromJson( - &command, Command::Origin::kLocal, nullptr, error); + std::string command_id; + auto command_instance = CommandInstance::FromJson(&command, command_origin, + &command_id, error); + // If we fail to validate the command definition, but there was a command ID + // specified there, return it to the caller when requested. This will be + // used to abort cloud commands. + if (id) + *id = command_id; + if (!command_instance) return false; @@ -218,11 +221,14 @@ return false; } - std::string command_id = std::to_string(++next_command_id_); - command_instance->SetID(command_id); + if (command_id.empty()) { + command_id = std::to_string(++next_command_id_); + command_instance->SetID(command_id); + } + if (id) *id = command_id; - AddCommand(std::move(command_instance)); + command_queue_.Add(std::move(command_instance)); return true; }
diff --git a/src/component_manager_impl.h b/src/component_manager_impl.h index f4f25a3..4f15ecd 100644 --- a/src/component_manager_impl.h +++ b/src/component_manager_impl.h
@@ -49,14 +49,10 @@ // Sets callback which is called when new components are added. void AddComponentTreeChangedCallback(const base::Closure& callback) override; - // Adds a new command instance to the command queue. The command specified in - // |command_instance| must be fully initialized and have its name, component, - // id populated. - void AddCommand(std::unique_ptr<CommandInstance> command_instance) override; - // Parses the command definition from a json dictionary and adds it to the // command queue. The new command ID is returned through optional |id| param. bool AddCommand(const base::DictionaryValue& command, + Command::Origin command_origin, UserRole role, std::string* id, ErrorPtr* error) override;
diff --git a/src/component_manager_unittest.cc b/src/component_manager_unittest.cc index 0c34041..dcefebd 100644 --- a/src/component_manager_unittest.cc +++ b/src/component_manager_unittest.cc
@@ -645,13 +645,17 @@ std::string id; const char kCommand1[] = R"({ "name": "trait1.command1", + "id": "1234-12345", "component": "comp1", "parameters": {} })"; auto command1 = CreateDictionaryValue(kCommand1); - EXPECT_TRUE(manager.AddCommand(*command1, UserRole::kUser, &id, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command1, Command::Origin::kLocal, + UserRole::kUser, &id, nullptr)); + EXPECT_EQ("1234-12345", id); // Not enough access rights - EXPECT_FALSE(manager.AddCommand(*command1, UserRole::kViewer, &id, nullptr)); + EXPECT_FALSE(manager.AddCommand(*command1, Command::Origin::kLocal, + UserRole::kViewer, &id, nullptr)); const char kCommand2[] = R"({ "name": "trait1.command3", @@ -660,7 +664,9 @@ })"; auto command2 = CreateDictionaryValue(kCommand2); // trait1.command3 doesn't exist - EXPECT_FALSE(manager.AddCommand(*command2, UserRole::kOwner, &id, nullptr)); + EXPECT_FALSE(manager.AddCommand(*command2, Command::Origin::kLocal, + UserRole::kOwner, &id, nullptr)); + EXPECT_TRUE(id.empty()); const char kCommand3[] = R"({ "name": "trait2.command1", @@ -669,7 +675,8 @@ })"; auto command3 = CreateDictionaryValue(kCommand3); // Component comp1 doesn't have trait2. - EXPECT_FALSE(manager.AddCommand(*command3, UserRole::kOwner, &id, nullptr)); + EXPECT_FALSE(manager.AddCommand(*command3, Command::Origin::kLocal, + UserRole::kOwner, &id, nullptr)); // No component specified, find the suitable component const char kCommand4[] = R"({ @@ -677,7 +684,8 @@ "parameters": {} })"; auto command4 = CreateDictionaryValue(kCommand4); - EXPECT_TRUE(manager.AddCommand(*command4, UserRole::kOwner, &id, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command4, Command::Origin::kLocal, + UserRole::kOwner, &id, nullptr)); auto cmd = manager.FindCommand(id); ASSERT_NE(nullptr, cmd); EXPECT_EQ("comp1", cmd->GetComponent()); @@ -687,7 +695,8 @@ "parameters": {} })"; auto command5 = CreateDictionaryValue(kCommand5); - EXPECT_TRUE(manager.AddCommand(*command5, UserRole::kOwner, &id, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command5, Command::Origin::kLocal, + UserRole::kOwner, &id, nullptr)); cmd = manager.FindCommand(id); ASSERT_NE(nullptr, cmd); EXPECT_EQ("comp2", cmd->GetComponent()); @@ -729,7 +738,8 @@ "component": "comp1" })"; auto command1 = CreateDictionaryValue(kCommand1); - EXPECT_TRUE(manager.AddCommand(*command1, UserRole::kUser, nullptr, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command1, Command::Origin::kCloud, + UserRole::kUser, nullptr, nullptr)); EXPECT_EQ("1", last_tags); last_tags.clear(); @@ -738,7 +748,8 @@ "component": "comp2" })"; auto command2 = CreateDictionaryValue(kCommand2); - EXPECT_TRUE(manager.AddCommand(*command2, UserRole::kUser, nullptr, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command2, Command::Origin::kCloud, + UserRole::kUser, nullptr, nullptr)); EXPECT_EQ("2", last_tags); last_tags.clear(); @@ -748,7 +759,8 @@ "parameters": {} })"; auto command3 = CreateDictionaryValue(kCommand3); - EXPECT_TRUE(manager.AddCommand(*command3, UserRole::kUser, nullptr, nullptr)); + EXPECT_TRUE(manager.AddCommand(*command3, Command::Origin::kLocal, + UserRole::kUser, nullptr, nullptr)); EXPECT_EQ("3", last_tags); last_tags.clear(); }
diff --git a/src/mock_component_manager.h b/src/mock_component_manager.h index 66b32d5..351e902 100644 --- a/src/mock_component_manager.h +++ b/src/mock_component_manager.h
@@ -29,8 +29,8 @@ ErrorPtr* error)); MOCK_METHOD1(AddComponentTreeChangedCallback, void(const base::Closure& callback)); - MOCK_METHOD1(MockAddCommand, void(CommandInstance* command_instance)); - MOCK_METHOD4(AddCommand, bool(const base::DictionaryValue& command, + MOCK_METHOD5(AddCommand, bool(const base::DictionaryValue& command, + Command::Origin command_origin, UserRole role, std::string* id, ErrorPtr* error)); @@ -82,9 +82,6 @@ std::string(const std::string& trait)); private: - void AddCommand(std::unique_ptr<CommandInstance> command_instance) override { - MockAddCommand(command_instance.get()); - } StateSnapshot GetAndClearRecordedStateChanges() override { return std::move(MockGetAndClearRecordedStateChanges()); }