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());
}
