buffet: Add command ID to CommandInstance class Rather than passing command ID along with CommandInstance class, make it to be part of CommandInstance itself. When a command instance is added to CommandQueue, the command queue will generate a new ID and set it to the command instance. Because CommandInstance, when saved in CommandQueue, is now mutable, remove 'const' in a bunch of places to allow the command instance to be modifiable. BUG=chromium:374864 TEST=USE=buffet P2_TEST_FILTER="buffet::*" FEATURES=test emerge-link platform2 Change-Id: Ia30dd4c9bd86b51694d9345dd91f6ed2ae0cb138 Reviewed-on: https://chromium-review.googlesource.com/213266 Reviewed-by: Chris Sosa <sosa@chromium.org> Tested-by: Alex Vakulenko <avakulenko@chromium.org> Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/command_dispatch_interface.h b/buffet/commands/command_dispatch_interface.h index b09c6ec..d1a727d 100644 --- a/buffet/commands/command_dispatch_interface.h +++ b/buffet/commands/command_dispatch_interface.h
@@ -18,12 +18,10 @@ public: virtual ~CommandDispachInterface() = default; // Callback invoked by CommandQueue when a new command is added to the queue. - virtual void OnCommandAdded(const std::string& command_id, - const CommandInstance* command_instance) = 0; + virtual void OnCommandAdded(CommandInstance* command_instance) = 0; // Callback invoked by CommandQueue when a new command is removed from // the queue. - virtual void OnCommandRemoved(const std::string& command_id, - const CommandInstance* command_instance) = 0; + virtual void OnCommandRemoved(CommandInstance* command_instance) = 0; }; } // namespace buffet
diff --git a/buffet/commands/command_instance.cc b/buffet/commands/command_instance.cc index 04b08ab..7c1502c 100644 --- a/buffet/commands/command_instance.cc +++ b/buffet/commands/command_instance.cc
@@ -72,11 +72,11 @@ } // anonymous namespace -std::unique_ptr<const CommandInstance> CommandInstance::FromJson( +std::unique_ptr<CommandInstance> CommandInstance::FromJson( const base::Value* value, const CommandDictionary& dictionary, chromeos::ErrorPtr* error) { - std::unique_ptr<const CommandInstance> instance; + std::unique_ptr<CommandInstance> instance; // Get the command JSON object from the value. const base::DictionaryValue* json = nullptr; if (!value->GetAsDictionary(&json)) { @@ -114,7 +114,8 @@ return instance; } - instance.reset(new CommandInstance(command_name, command_def->GetCategory(), + instance.reset(new CommandInstance(command_name, + command_def->GetCategory(), parameters)); return instance; }
diff --git a/buffet/commands/command_instance.h b/buffet/commands/command_instance.h index a901380..ebc43ad 100644 --- a/buffet/commands/command_instance.h +++ b/buffet/commands/command_instance.h
@@ -32,6 +32,8 @@ const std::string& category, const native_types::Object& parameters); + // Returns the full command ID. + const std::string& GetID() const { return id_; } // Returns the full name of the command. const std::string& GetName() const { return name_; } // Returns the command category. @@ -46,12 +48,18 @@ // 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|. - static std::unique_ptr<const CommandInstance> FromJson( + static std::unique_ptr<CommandInstance> FromJson( const base::Value* value, const CommandDictionary& dictionary, chromeos::ErrorPtr* error); + // Sets the command ID (normally done by CommandQueue when the command + // instance is added to it). + void SetID(const std::string& id) { id_ = id; } + private: + // Unique command ID within a command queue. + std::string id_; // Full command name as "<package_name>.<command_name>". std::string name_; // Command category. See comments for CommandDefinitions::LoadCommands for the
diff --git a/buffet/commands/command_instance_unittest.cc b/buffet/commands/command_instance_unittest.cc index ace0738..844ac92 100644 --- a/buffet/commands/command_instance_unittest.cc +++ b/buffet/commands/command_instance_unittest.cc
@@ -69,6 +69,7 @@ params.insert(std::make_pair("volume", int_prop.CreateValue(100))); buffet::CommandInstance instance("robot._beep", "robotd", params); + EXPECT_EQ("", instance.GetID()); EXPECT_EQ("robot._beep", instance.GetName()); EXPECT_EQ("robotd", instance.GetCategory()); EXPECT_EQ(params, instance.GetParameters()); @@ -78,6 +79,12 @@ EXPECT_EQ(nullptr, instance.FindParameter("blah").get()); } +TEST(CommandInstance, SetID) { + buffet::CommandInstance instance("robot._beep", "robotd", {}); + instance.SetID("command_id"); + EXPECT_EQ("command_id", instance.GetID()); +} + TEST_F(CommandInstanceTest, FromJson) { auto json = CreateDictionaryValue(R"({ 'name': 'robot.jump',
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc index 671db6d..557a303 100644 --- a/buffet/commands/command_queue.cc +++ b/buffet/commands/command_queue.cc
@@ -7,31 +7,32 @@ namespace buffet { -std::string CommandQueue::Add(std::unique_ptr<const CommandInstance> instance) { +std::string CommandQueue::Add(std::unique_ptr<CommandInstance> instance) { std::string id = std::to_string(++next_id_); + instance->SetID(id); auto pair = map_.insert(std::make_pair(id, std::move(instance))); LOG_IF(FATAL, !pair.second) << "Command with ID '" << id << "' is already in the queue"; if (dispatch_interface_) - dispatch_interface_->OnCommandAdded(id, pair.first->second.get()); + dispatch_interface_->OnCommandAdded(pair.first->second.get()); return id; } -std::unique_ptr<const CommandInstance> CommandQueue::Remove( +std::unique_ptr<CommandInstance> CommandQueue::Remove( const std::string& id) { - std::unique_ptr<const CommandInstance> instance; + std::unique_ptr<CommandInstance> instance; auto p = map_.find(id); if (p != map_.end()) { instance = std::move(p->second); map_.erase(p); if (dispatch_interface_) - dispatch_interface_->OnCommandRemoved(id, instance.get()); + dispatch_interface_->OnCommandRemoved(instance.get()); } return instance; } -const CommandInstance* CommandQueue::Find(const std::string& id) const { +CommandInstance* CommandQueue::Find(const std::string& id) const { auto p = map_.find(id); return (p != map_.end()) ? p->second.get() : nullptr; }
diff --git a/buffet/commands/command_queue.h b/buffet/commands/command_queue.h index e70fb8d..4e2706b 100644 --- a/buffet/commands/command_queue.h +++ b/buffet/commands/command_queue.h
@@ -39,21 +39,21 @@ // has no relation to any GCD command identifiers or anything else. Just a // unique key in this queue class. // The ID string of the added command is returned by this method. - std::string Add(std::unique_ptr<const CommandInstance> instance); + std::string Add(std::unique_ptr<CommandInstance> instance); // Removes a command identified by |id| from the queue. Returns a unique // pointer to the command instance if removed successfully, or an empty // unique_ptr if the command with the given ID doesn't exist in the queue. - std::unique_ptr<const CommandInstance> Remove(const std::string& id); + std::unique_ptr<CommandInstance> Remove(const std::string& id); // Finds a command instance in the queue by the instance |id|. Returns // nullptr if the command with the given |id| is not found. The returned // pointer should not be persisted for a long period of time. - const CommandInstance* Find(const std::string& id) const; + CommandInstance* Find(const std::string& id) const; private: // ID-to-CommandInstance map. - std::map<std::string, std::unique_ptr<const CommandInstance>> map_; + std::map<std::string, std::unique_ptr<CommandInstance>> map_; // Counter for generating unique command IDs. int next_id_ = 0; // Callback interface for command dispatch, if provided.
diff --git a/buffet/commands/command_queue_unittest.cc b/buffet/commands/command_queue_unittest.cc index a9d5ad9..ce81ea4 100644 --- a/buffet/commands/command_queue_unittest.cc +++ b/buffet/commands/command_queue_unittest.cc
@@ -14,9 +14,9 @@ namespace { -std::unique_ptr<const buffet::CommandInstance> CreateDummyCommandInstance( +std::unique_ptr<buffet::CommandInstance> CreateDummyCommandInstance( const std::string& name = "base.reboot") { - return std::unique_ptr<const buffet::CommandInstance>( + return std::unique_ptr<buffet::CommandInstance>( new buffet::CommandInstance(name, "powerd", {})); } @@ -25,20 +25,16 @@ // Aborts if duplicate commands are added or non-existent commands are removed. class FakeDispatchInterface : public buffet::CommandDispachInterface { public: - void OnCommandAdded( - const std::string& command_id, - const buffet::CommandInstance* command_instance) override { - CHECK(ids_.insert(command_id).second) - << "Command ID already exists: " << command_id; + void OnCommandAdded(buffet::CommandInstance* command_instance) override { + CHECK(ids_.insert(command_instance->GetID()).second) + << "Command ID already exists: " << command_instance->GetID(); CHECK(commands_.insert(command_instance).second) << "Command instance already exists"; } - void OnCommandRemoved( - const std::string& command_id, - const buffet::CommandInstance* command_instance) override { - CHECK_EQ(1, ids_.erase(command_id)) - << "Command ID not found: " << command_id; + void OnCommandRemoved(buffet::CommandInstance* command_instance) override { + CHECK_EQ(1, ids_.erase(command_instance->GetID())) + << "Command ID not found: " << command_instance->GetID(); CHECK_EQ(1, commands_.erase(command_instance)) << "Command instance not found"; } @@ -52,7 +48,7 @@ private: std::set<std::string> ids_; - std::set<const buffet::CommandInstance*> commands_; + std::set<buffet::CommandInstance*> commands_; }; } // anonymous namespace @@ -116,7 +112,9 @@ auto cmd1 = queue.Find(id1); EXPECT_NE(nullptr, cmd1); EXPECT_EQ("base.reboot", cmd1->GetName()); + EXPECT_EQ(id1, cmd1->GetID()); auto cmd2 = queue.Find(id2); EXPECT_NE(nullptr, cmd2); EXPECT_EQ("base.shutdown", cmd2->GetName()); + EXPECT_EQ(id2, cmd2->GetID()); }