libweave: Add more methods to modify weave::Command
Added SetProgress, SetResults, Abort, Cancel and Done methods.
base::DictionaryValue is used as type for complex values.
Switched DBusCommandProxy to weave::Command interface.
BUG=brillo:1245
TEST='FEATURES=test emerge-gizmo buffet'
Change-Id: I3a4a7f2ee63041c9f6b60b0af18d8d3d2c0cc7e6
Reviewed-on: https://chromium-review.googlesource.com/288250
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/libweave/include/weave/command.h b/libweave/include/weave/command.h
index 4a3af6b..3274664 100644
--- a/libweave/include/weave/command.h
+++ b/libweave/include/weave/command.h
@@ -8,6 +8,7 @@
#include <string>
#include <base/values.h>
+#include <chromeos/errors/error.h>
namespace weave {
@@ -53,6 +54,25 @@
// Returns the command results.
virtual std::unique_ptr<base::DictionaryValue> GetResults() const = 0;
+ // Updates the command progress. The |progress| should match the schema.
+ // Returns false if |progress| value is incorrect.
+ virtual bool SetProgress(const base::DictionaryValue& progress,
+ chromeos::ErrorPtr* error) = 0;
+
+ // Updates the command results. The |results| should match the schema.
+ // Returns false if |results| value is incorrect.
+ virtual bool SetResults(const base::DictionaryValue& results,
+ chromeos::ErrorPtr* error) = 0;
+
+ // Aborts command execution.
+ virtual void Abort() = 0;
+
+ // Cancels command execution.
+ virtual void Cancel() = 0;
+
+ // Marks the command as completed successfully.
+ virtual void Done() = 0;
+
// Returns JSON representation of the command.
virtual std::unique_ptr<base::DictionaryValue> ToJson() const = 0;
diff --git a/libweave/src/base_api_handler.cc b/libweave/src/base_api_handler.cc
index 5e3f559..72f9dd4 100644
--- a/libweave/src/base_api_handler.cc
+++ b/libweave/src/base_api_handler.cc
@@ -32,7 +32,7 @@
}
void BaseApiHandler::UpdateBaseConfiguration(CommandInstance* command) {
- command->SetProgress({});
+ command->SetProgress(base::DictionaryValue{}, nullptr);
const BuffetConfig& config{device_info_->GetConfig()};
std::string anonymous_access_role{config.local_anonymous_access_role()};
@@ -62,7 +62,7 @@
}
void BaseApiHandler::UpdateDeviceInfo(CommandInstance* command) {
- command->SetProgress({});
+ command->SetProgress(base::DictionaryValue{}, nullptr);
const BuffetConfig& config{device_info_->GetConfig()};
std::string name{config.name()};
diff --git a/libweave/src/commands/cloud_command_proxy_unittest.cc b/libweave/src/commands/cloud_command_proxy_unittest.cc
index 06ff331..bd45e25 100644
--- a/libweave/src/commands/cloud_command_proxy_unittest.cc
+++ b/libweave/src/commands/cloud_command_proxy_unittest.cc
@@ -200,8 +200,8 @@
EXPECT_CALL(cloud_updater_,
UpdateCommand(kCmdID, MatchJson("{'state':'inProgress'}"), _, _))
.WillOnce(SaveArg<2>(&on_success));
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
// Now simulate the first request completing.
// The second request should be sent now.
@@ -217,8 +217,8 @@
// state=inProgress
// progress={...}
// Both updates will be held until device state is updated.
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
// Now simulate the device state updated. Both updates should come in one
// request.
@@ -235,8 +235,8 @@
const char expect1[] = "{'state':'inProgress'}";
EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expect1), _, _))
.WillOnce(SaveArg<3>(&on_error));
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
// Now pretend the first command update request has failed.
// We should retry with both state and progress fields updated this time,
@@ -273,11 +273,11 @@
TEST_F(CloudCommandProxyTest, GateOnStateUpdates) {
current_state_update_id_ = 20;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
current_state_update_id_ = 21;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("busy")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
command_instance_->Done();
@@ -312,11 +312,11 @@
TEST_F(CloudCommandProxyTest, CombineSomeStates) {
current_state_update_id_ = 20;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
current_state_update_id_ = 21;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("busy")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
command_instance_->Done();
@@ -341,11 +341,11 @@
TEST_F(CloudCommandProxyTest, CombineAllStates) {
current_state_update_id_ = 20;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
current_state_update_id_ = 21;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("busy")}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
command_instance_->Done();
@@ -360,13 +360,14 @@
TEST_F(CloudCommandProxyTest, CoalesceUpdates) {
current_state_update_id_ = 20;
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("ready")}});
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("busy")}});
- command_instance_->SetProgress(
- {{"status", unittests::make_string_prop_value("finished")}});
- command_instance_->SetResults({{"sum", unittests::make_int_prop_value(30)}});
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'ready'}"), nullptr));
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'busy'}"), nullptr));
+ EXPECT_TRUE(command_instance_->SetProgress(
+ *CreateDictionaryValue("{'status': 'finished'}"), nullptr));
+ EXPECT_TRUE(command_instance_->SetResults(
+ *CreateDictionaryValue("{'sum': 30}"), nullptr));
command_instance_->Done();
const char expected[] = R"({
diff --git a/libweave/src/commands/command_instance.cc b/libweave/src/commands/command_instance.cc
index 5782695..34ed8dc 100644
--- a/libweave/src/commands/command_instance.cc
+++ b/libweave/src/commands/command_instance.cc
@@ -74,6 +74,42 @@
return TypedValueToJson(results_, nullptr);
}
+bool CommandInstance::SetProgress(const base::DictionaryValue& progress,
+ chromeos::ErrorPtr* error) {
+ ObjectPropType obj_prop_type;
+ obj_prop_type.SetObjectSchema(command_definition_->GetProgress()->Clone());
+
+ ValueMap obj;
+ if (!TypedValueFromJson(&progress, &obj_prop_type, &obj, error))
+ return false;
+
+ // Change status even if progress unchanged, e.g. 0% -> 0%.
+ SetStatus(kStatusInProgress);
+ if (obj != progress_) {
+ progress_ = obj;
+ for (auto observer : observers_)
+ observer->OnProgressChanged();
+ }
+ return true;
+}
+
+bool CommandInstance::SetResults(const base::DictionaryValue& results,
+ chromeos::ErrorPtr* error) {
+ ObjectPropType obj_prop_type;
+ obj_prop_type.SetObjectSchema(command_definition_->GetResults()->Clone());
+
+ ValueMap obj;
+ if (!TypedValueFromJson(&results, &obj_prop_type, &obj, error))
+ return false;
+
+ if (obj != results_) {
+ results_ = obj;
+ for (auto observer : observers_)
+ observer->OnResultsChanged();
+ }
+ return true;
+}
+
namespace {
// Helper method to retrieve command parameters from the command definition
@@ -198,27 +234,6 @@
observers_.push_back(observer);
}
-bool CommandInstance::SetResults(const ValueMap& results) {
- // TODO(antonm): Add validation.
- if (results != results_) {
- results_ = results;
- for (auto observer : observers_)
- observer->OnResultsChanged();
- }
- return true;
-}
-
-bool CommandInstance::SetProgress(const ValueMap& progress) {
- // Change status even if progress unchanged, e.g. 0% -> 0%.
- SetStatus(kStatusInProgress);
- if (progress != progress_) {
- progress_ = progress;
- for (auto observer : observers_)
- observer->OnProgressChanged();
- }
- return true;
-}
-
void CommandInstance::Abort() {
SetStatus(kStatusAborted);
RemoveFromQueue();
diff --git a/libweave/src/commands/command_instance.h b/libweave/src/commands/command_instance.h
index 0fdc643..e911ede 100644
--- a/libweave/src/commands/command_instance.h
+++ b/libweave/src/commands/command_instance.h
@@ -50,6 +50,13 @@
std::unique_ptr<base::DictionaryValue> GetParameters() const override;
std::unique_ptr<base::DictionaryValue> GetProgress() const override;
std::unique_ptr<base::DictionaryValue> GetResults() const override;
+ bool SetProgress(const base::DictionaryValue& progress,
+ chromeos::ErrorPtr* error) override;
+ bool SetResults(const base::DictionaryValue& results,
+ chromeos::ErrorPtr* error) override;
+ void Abort() override;
+ void Cancel() override;
+ void Done() override;
// Returns command definition.
const CommandDefinition* GetCommandDefinition() const {
@@ -77,21 +84,6 @@
// Sets the pointer to queue this command is part of.
void SetCommandQueue(CommandQueue* queue) { queue_ = queue; }
- // Updates the command progress. The |progress| should match the schema.
- // Returns false if |results| value is incorrect.
- bool SetProgress(const ValueMap& progress);
-
- // Updates the command results. The |results| should match the schema.
- // Returns false if |results| value is incorrect.
- bool SetResults(const ValueMap& results);
-
- // Aborts command execution.
- void Abort();
- // Cancels command execution.
- void Cancel();
- // Marks the command as completed successfully.
- void Done();
-
// Values for command execution status.
static const char kStatusQueued[];
static const char kStatusInProgress[];
diff --git a/libweave/src/commands/command_instance_unittest.cc b/libweave/src/commands/command_instance_unittest.cc
index 9fbe315..4a7972a 100644
--- a/libweave/src/commands/command_instance_unittest.cc
+++ b/libweave/src/commands/command_instance_unittest.cc
@@ -41,7 +41,8 @@
'enum': ['_withAirFlip', '_withSpin', '_withKick']
}
},
- 'results': {}
+ 'progress': {'progress': 'integer'},
+ 'results': {'testResult': 'integer'}
},
'speak': {
'parameters': {
@@ -56,7 +57,7 @@
'maximum': 10
}
},
- 'results': {}
+ 'results': {'foo': 'integer'}
}
}
})");
@@ -77,9 +78,8 @@
CommandInstance instance{
"robot.speak", "cloud", dict_.FindCommand("robot.speak"), params};
- ValueMap results;
- results["foo"] = int_prop.CreateValue(239, nullptr);
- instance.SetResults(results);
+ EXPECT_TRUE(
+ instance.SetResults(*CreateDictionaryValue("{'foo': 239}"), nullptr));
EXPECT_EQ("", instance.GetID());
EXPECT_EQ("robot.speak", instance.GetName());
@@ -211,14 +211,13 @@
})");
auto instance =
CommandInstance::FromJson(json.get(), "cloud", dict_, nullptr, nullptr);
- instance->SetProgress(
- ValueMap{{"progress", unittests::make_int_prop_value(15)}});
- instance->SetProgress(
- ValueMap{{"progress", unittests::make_int_prop_value(15)}});
+ EXPECT_TRUE(instance->SetProgress(*CreateDictionaryValue("{'progress': 15}"),
+ nullptr));
+ EXPECT_TRUE(instance->SetProgress(*CreateDictionaryValue("{'progress': 15}"),
+ nullptr));
instance->SetID("testId");
- ValueMap results;
- instance->SetResults(
- ValueMap{{"testResult", unittests::make_int_prop_value(17)}});
+ EXPECT_TRUE(instance->SetResults(*CreateDictionaryValue("{'testResult': 17}"),
+ nullptr));
json->MergeDictionary(CreateDictionaryValue(R"({
'id': 'testId',
diff --git a/libweave/src/commands/dbus_command_proxy.cc b/libweave/src/commands/dbus_command_proxy.cc
index aadf781..782a9b3 100644
--- a/libweave/src/commands/dbus_command_proxy.cc
+++ b/libweave/src/commands/dbus_command_proxy.cc
@@ -7,11 +7,7 @@
#include <chromeos/dbus/async_event_sequencer.h>
#include <chromeos/dbus/exported_object_manager.h>
-#include "libweave/src/commands/command_definition.h"
-#include "libweave/src/commands/command_instance.h"
-#include "libweave/src/commands/object_schema.h"
-#include "libweave/src/commands/prop_constraints.h"
-#include "libweave/src/commands/prop_types.h"
+#include "libweave/src/commands/dbus_conversion.h"
using chromeos::dbus_utils::AsyncEventSequencer;
using chromeos::dbus_utils::ExportedObjectManager;
@@ -20,9 +16,9 @@
DBusCommandProxy::DBusCommandProxy(ExportedObjectManager* object_manager,
const scoped_refptr<dbus::Bus>& bus,
- CommandInstance* command_instance,
+ Command* command,
std::string object_path)
- : command_instance_{command_instance},
+ : command_{command},
dbus_object_{object_manager, bus, dbus::ObjectPath{object_path}} {
}
@@ -31,17 +27,17 @@
dbus_adaptor_.RegisterWithDBusObject(&dbus_object_);
// Set the initial property values before registering the DBus object.
- dbus_adaptor_.SetName(command_instance_->GetName());
- dbus_adaptor_.SetCategory(command_instance_->GetCategory());
- dbus_adaptor_.SetId(command_instance_->GetID());
- dbus_adaptor_.SetStatus(command_instance_->GetStatus());
+ dbus_adaptor_.SetName(command_->GetName());
+ dbus_adaptor_.SetCategory(command_->GetCategory());
+ dbus_adaptor_.SetId(command_->GetID());
+ dbus_adaptor_.SetStatus(command_->GetStatus());
dbus_adaptor_.SetProgress(
- DictionaryToDBusVariantDictionary(*command_instance_->GetProgress()));
- dbus_adaptor_.SetOrigin(command_instance_->GetOrigin());
+ DictionaryToDBusVariantDictionary(*command_->GetProgress()));
+ dbus_adaptor_.SetOrigin(command_->GetOrigin());
dbus_adaptor_.SetParameters(
- DictionaryToDBusVariantDictionary(*command_instance_->GetParameters()));
+ DictionaryToDBusVariantDictionary(*command_->GetParameters()));
dbus_adaptor_.SetResults(
- DictionaryToDBusVariantDictionary(*command_instance_->GetResults()));
+ DictionaryToDBusVariantDictionary(*command_->GetResults()));
// Register the command DBus object and expose its methods and properties.
dbus_object_.RegisterAsync(completion_callback);
@@ -49,16 +45,16 @@
void DBusCommandProxy::OnResultsChanged() {
dbus_adaptor_.SetResults(
- DictionaryToDBusVariantDictionary(*command_instance_->GetResults()));
+ DictionaryToDBusVariantDictionary(*command_->GetResults()));
}
void DBusCommandProxy::OnStatusChanged() {
- dbus_adaptor_.SetStatus(command_instance_->GetStatus());
+ dbus_adaptor_.SetStatus(command_->GetStatus());
}
void DBusCommandProxy::OnProgressChanged() {
dbus_adaptor_.SetProgress(
- DictionaryToDBusVariantDictionary(*command_instance_->GetProgress()));
+ DictionaryToDBusVariantDictionary(*command_->GetProgress()));
}
void DBusCommandProxy::OnCommandDestroyed() {
@@ -68,49 +64,40 @@
bool DBusCommandProxy::SetProgress(
chromeos::ErrorPtr* error,
const chromeos::VariantDictionary& progress) {
- LOG(INFO) << "Received call to Command<" << command_instance_->GetName()
+ LOG(INFO) << "Received call to Command<" << command_->GetName()
<< ">::SetProgress()";
-
- auto progress_schema =
- command_instance_->GetCommandDefinition()->GetProgress();
- ValueMap obj;
- if (!ObjectFromDBusVariant(progress_schema, progress, &obj, error))
+ auto dictionary = DictionaryFromDBusVariantDictionary(progress, error);
+ if (!dictionary)
return false;
-
- command_instance_->SetProgress(obj);
- return true;
+ return command_->SetProgress(*dictionary, error);
}
bool DBusCommandProxy::SetResults(chromeos::ErrorPtr* error,
const chromeos::VariantDictionary& results) {
- LOG(INFO) << "Received call to Command<" << command_instance_->GetName()
+ LOG(INFO) << "Received call to Command<" << command_->GetName()
<< ">::SetResults()";
-
- auto results_schema = command_instance_->GetCommandDefinition()->GetResults();
- ValueMap obj;
- if (!ObjectFromDBusVariant(results_schema, results, &obj, error))
+ auto dictionary = DictionaryFromDBusVariantDictionary(results, error);
+ if (!dictionary)
return false;
-
- command_instance_->SetResults(obj);
- return true;
+ return command_->SetResults(*dictionary, error);
}
void DBusCommandProxy::Abort() {
- LOG(INFO) << "Received call to Command<" << command_instance_->GetName()
+ LOG(INFO) << "Received call to Command<" << command_->GetName()
<< ">::Abort()";
- command_instance_->Abort();
+ command_->Abort();
}
void DBusCommandProxy::Cancel() {
- LOG(INFO) << "Received call to Command<" << command_instance_->GetName()
+ LOG(INFO) << "Received call to Command<" << command_->GetName()
<< ">::Cancel()";
- command_instance_->Cancel();
+ command_->Cancel();
}
void DBusCommandProxy::Done() {
- LOG(INFO) << "Received call to Command<" << command_instance_->GetName()
+ LOG(INFO) << "Received call to Command<" << command_->GetName()
<< ">::Done()";
- command_instance_->Done();
+ command_->Done();
}
} // namespace weave
diff --git a/libweave/src/commands/dbus_command_proxy.h b/libweave/src/commands/dbus_command_proxy.h
index 62225c7..66ae17b 100644
--- a/libweave/src/commands/dbus_command_proxy.h
+++ b/libweave/src/commands/dbus_command_proxy.h
@@ -29,7 +29,7 @@
public:
DBusCommandProxy(chromeos::dbus_utils::ExportedObjectManager* object_manager,
const scoped_refptr<dbus::Bus>& bus,
- CommandInstance* command_instance,
+ Command* command,
std::string object_path);
~DBusCommandProxy() override = default;
@@ -57,7 +57,7 @@
// Handles calls to org.chromium.Buffet.Command.Done().
void Done() override;
- CommandInstance* command_instance_;
+ Command* command_;
org::chromium::Buffet::CommandAdaptor dbus_adaptor_{this};
chromeos::dbus_utils::DBusObject dbus_object_;
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc
index 9ef7731..9c45afd 100644
--- a/libweave/src/device_registration_info_unittest.cc
+++ b/libweave/src/device_registration_info_unittest.cc
@@ -24,9 +24,10 @@
namespace weave {
-using chromeos::http::request_header::kAuthorization;
using chromeos::http::fake::ServerRequest;
using chromeos::http::fake::ServerResponse;
+using chromeos::http::request_header::kAuthorization;
+using unittests::CreateDictionaryValue;
namespace {
@@ -522,6 +523,7 @@
'robot': {
'_jump': {
'parameters': {'_height': 'integer'},
+ 'progress': {'progress': 'integer'},
'results': {'status': 'string'},
'minimalRole': 'user'
}
@@ -543,9 +545,6 @@
PublishCommands(*command_list);
auto command = command_manager_->FindCommand("1234");
ASSERT_NE(nullptr, command);
- StringPropType string_type;
- ValueMap results{
- {"status", string_type.CreateValue(std::string{"Ok"}, nullptr)}};
// UpdateCommand when setting command results.
auto update_command_results = [](const ServerRequest& request,
@@ -559,7 +558,8 @@
transport_->AddHandler(command_url, chromeos::http::request_type::kPatch,
base::Bind(update_command_results));
- command->SetResults(results);
+ EXPECT_TRUE(
+ command->SetResults(*CreateDictionaryValue("{'status': 'Ok'}"), nullptr));
// UpdateCommand when setting command progress.
int count = 0; // This will be called twice...
@@ -579,8 +579,8 @@
transport_->AddHandler(command_url, chromeos::http::request_type::kPatch,
base::Bind(update_command_progress));
- ValueMap progress{{"progress", unittests::make_int_prop_value(18)}};
- command->SetProgress(progress);
+ EXPECT_TRUE(command->SetProgress(*CreateDictionaryValue("{'progress': 18}"),
+ nullptr));
// UpdateCommand when changing command status.
auto update_command_state = [](const ServerRequest& request,
