Remove Command::Done and add verification of Command state transitions
BUG:23906692
Change-Id: I1934ad70e1918cc5b40ac07ee1e1ccfee406867e
Reviewed-on: https://weave-review.googlesource.com/1259
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/libweave/src/commands/cloud_command_proxy_unittest.cc b/libweave/src/commands/cloud_command_proxy_unittest.cc
index e1d96b0..dbff905 100644
--- a/libweave/src/commands/cloud_command_proxy_unittest.cc
+++ b/libweave/src/commands/cloud_command_proxy_unittest.cc
@@ -147,14 +147,14 @@
TEST_F(CloudCommandProxyTest, ImmediateUpdate) {
const char expected[] = "{'state':'done'}";
EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expected), _, _));
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
}
TEST_F(CloudCommandProxyTest, DelayedUpdate) {
// Simulate that the current device state has changed.
current_state_update_id_ = 20;
// No command update is expected here.
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
// Still no command update here...
callbacks_.Notify(19);
// Now we should get the update...
@@ -247,7 +247,7 @@
EXPECT_TRUE(command_instance_->SetProgress(
*CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
// Device state #20 updated.
base::Closure on_success;
@@ -286,7 +286,7 @@
EXPECT_TRUE(command_instance_->SetProgress(
*CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
// Device state 20-21 updated.
base::Closure on_success;
@@ -315,7 +315,7 @@
EXPECT_TRUE(command_instance_->SetProgress(
*CreateDictionaryValue("{'status': 'busy'}"), nullptr));
current_state_update_id_ = 22;
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
// Device state 30 updated.
const char expected[] = R"({
@@ -336,7 +336,7 @@
*CreateDictionaryValue("{'status': 'finished'}"), nullptr));
EXPECT_TRUE(command_instance_->SetResults(
*CreateDictionaryValue("{'sum': 30}"), nullptr));
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
const char expected[] = R"({
'progress': {'status':'finished'},
@@ -360,7 +360,7 @@
// As soon as we change the command, the update to the server should be sent.
const char expected[] = "{'state':'done'}";
EXPECT_CALL(cloud_updater_, UpdateCommand(kCmdID, MatchJson(expected), _, _));
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
}
TEST_F(CloudCommandProxyTest, NonEmptyStateChangeQueue) {
@@ -372,7 +372,7 @@
CreateCommandInstance();
// No command updates right now.
- command_instance_->Done();
+ command_instance_->SetResults({}, nullptr);
// Only when the state #20 is published we should update the command
const char expected[] = "{'state':'done'}";
diff --git a/libweave/src/commands/command_instance.cc b/libweave/src/commands/command_instance.cc
index 45c376d..4701bb6 100644
--- a/libweave/src/commands/command_instance.cc
+++ b/libweave/src/commands/command_instance.cc
@@ -38,10 +38,21 @@
{CommandOrigin::kCloud, "cloud"},
};
-void ReportDestroyedError(ErrorPtr* error) {
+bool ReportDestroyedError(ErrorPtr* error) {
Error::AddTo(error, FROM_HERE, errors::commands::kDomain,
errors::commands::kCommandDestroyed,
"Command has been destroyed");
+ return false;
+}
+
+bool ReportInvalidStateTransition(ErrorPtr* error,
+ CommandStatus from,
+ CommandStatus to) {
+ Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain,
+ errors::commands::kInvalidState,
+ "State switch impossible: '%s' -> '%s'",
+ EnumToString(from).c_str(), EnumToString(to).c_str());
+ return false;
}
} // namespace
@@ -97,12 +108,14 @@
return TypedValueToJson(results_);
}
+const Error* CommandInstance::GetError() const {
+ return error_.get();
+}
+
bool CommandInstance::SetProgress(const base::DictionaryValue& progress,
ErrorPtr* error) {
- if (!command_definition_) {
- ReportDestroyedError(error);
- return false;
- }
+ if (!command_definition_)
+ return ReportDestroyedError(error);
ObjectPropType obj_prop_type;
obj_prop_type.SetObjectSchema(command_definition_->GetProgress()->Clone());
@@ -111,20 +124,21 @@
return false;
// Change status even if progress unchanged, e.g. 0% -> 0%.
- SetStatus(CommandStatus::kInProgress);
+ if (!SetStatus(CommandStatus::kInProgress, error))
+ return false;
+
if (obj != progress_) {
progress_ = obj;
FOR_EACH_OBSERVER(Observer, observers_, OnProgressChanged());
}
+
return true;
}
bool CommandInstance::SetResults(const base::DictionaryValue& results,
ErrorPtr* error) {
- if (!command_definition_) {
- ReportDestroyedError(error);
- return false;
- }
+ if (!command_definition_)
+ return ReportDestroyedError(error);
ObjectPropType obj_prop_type;
obj_prop_type.SetObjectSchema(command_definition_->GetResults()->Clone());
@@ -136,7 +150,16 @@
results_ = obj;
FOR_EACH_OBSERVER(Observer, observers_, OnResultsChanged());
}
- return true;
+ // Change status even if result is unchanged.
+ bool result = SetStatus(CommandStatus::kDone, error);
+ RemoveFromQueue();
+ // The command will be destroyed after that, so do not access any members.
+ return result;
+}
+
+bool CommandInstance::SetError(const Error* command_error, ErrorPtr* error) {
+ error_ = command_error ? command_error->Clone() : nullptr;
+ return SetStatus(CommandStatus::kError, error);
}
namespace {
@@ -267,31 +290,45 @@
observers_.RemoveObserver(observer);
}
-void CommandInstance::Abort(const Error* error) {
- if (error)
- error_ = error->Clone();
- SetStatus(CommandStatus::kAborted);
- RemoveFromQueue();
- // The command will be destroyed after that, so do not access any members.
+bool CommandInstance::Pause(ErrorPtr* error) {
+ return SetStatus(CommandStatus::kPaused, error);
}
-void CommandInstance::Cancel() {
- SetStatus(CommandStatus::kCancelled);
+bool CommandInstance::Abort(const Error* command_error, ErrorPtr* error) {
+ error_ = command_error ? command_error->Clone() : nullptr;
+ bool result = SetStatus(CommandStatus::kAborted, error);
RemoveFromQueue();
// The command will be destroyed after that, so do not access any members.
+ return result;
}
-void CommandInstance::Done() {
- SetStatus(CommandStatus::kDone);
+bool CommandInstance::Cancel(ErrorPtr* error) {
+ bool result = SetStatus(CommandStatus::kCancelled, error);
RemoveFromQueue();
// The command will be destroyed after that, so do not access any members.
+ return result;
}
-void CommandInstance::SetStatus(CommandStatus status) {
- if (status != status_) {
- status_ = status;
- FOR_EACH_OBSERVER(Observer, observers_, OnStatusChanged());
+bool CommandInstance::SetStatus(CommandStatus status, ErrorPtr* error) {
+ if (status == status_)
+ return true;
+ if (status == CommandStatus::kQueued)
+ return ReportInvalidStateTransition(error, status_, status);
+ switch (status_) {
+ case CommandStatus::kDone:
+ case CommandStatus::kCancelled:
+ case CommandStatus::kAborted:
+ case CommandStatus::kExpired:
+ return ReportInvalidStateTransition(error, status_, status);
+ case CommandStatus::kQueued:
+ case CommandStatus::kInProgress:
+ case CommandStatus::kPaused:
+ case CommandStatus::kError:
+ break;
}
+ status_ = status;
+ FOR_EACH_OBSERVER(Observer, observers_, OnStatusChanged());
+ return true;
}
void CommandInstance::RemoveFromQueue() {
diff --git a/libweave/src/commands/command_instance.h b/libweave/src/commands/command_instance.h
index 0bc3c0f..e8a838d 100644
--- a/libweave/src/commands/command_instance.h
+++ b/libweave/src/commands/command_instance.h
@@ -59,13 +59,15 @@
std::unique_ptr<base::DictionaryValue> GetParameters() const override;
std::unique_ptr<base::DictionaryValue> GetProgress() const override;
std::unique_ptr<base::DictionaryValue> GetResults() const override;
+ const Error* GetError() const override;
bool SetProgress(const base::DictionaryValue& progress,
ErrorPtr* error) override;
bool SetResults(const base::DictionaryValue& results,
ErrorPtr* error) override;
- void Abort(const Error* error) override;
- void Cancel() override;
- void Done() override;
+ bool Pause(ErrorPtr* error) override;
+ bool SetError(const Error* command_error, ErrorPtr* error) override;
+ bool Abort(const Error* command_error, ErrorPtr* error) override;
+ bool Cancel(ErrorPtr* error) override;
// Returns command definition.
const CommandDefinition* GetCommandDefinition() const {
@@ -107,7 +109,7 @@
private:
// Helper function to update the command status.
// Used by Abort(), Cancel(), Done() methods.
- void SetStatus(CommandStatus status);
+ bool SetStatus(CommandStatus status, ErrorPtr* error);
// Helper method that removes this command from the command queue.
// Note that since the command queue owns the lifetime of the command instance
// object, removing a command from the queue will also destroy it.
diff --git a/libweave/src/commands/command_instance_unittest.cc b/libweave/src/commands/command_instance_unittest.cc
index f783ca7..7c2536b 100644
--- a/libweave/src/commands/command_instance_unittest.cc
+++ b/libweave/src/commands/command_instance_unittest.cc
@@ -212,15 +212,37 @@
EXPECT_TRUE(instance->SetResults(*CreateDictionaryValue("{'testResult': 17}"),
nullptr));
- ErrorPtr error;
- Error::AddTo(&error, FROM_HERE, "DOMAIN", "CODE", "MESSAGE");
- instance->Abort(error.get());
-
json->MergeDictionary(CreateDictionaryValue(R"({
'id': 'testId',
'progress': {'progress': 15},
+ 'state': 'done',
+ 'results': {'testResult': 17}
+ })").get());
+
+ auto converted = instance->ToJson();
+ EXPECT_PRED2([](const base::Value& val1,
+ const base::Value& val2) { return val1.Equals(&val2); },
+ *json, *converted);
+}
+
+TEST_F(CommandInstanceTest, ToJsonError) {
+ auto json = CreateDictionaryValue(R"({
+ 'name': 'base.reboot',
+ 'parameters': {}
+ })");
+ auto instance = CommandInstance::FromJson(json.get(), CommandOrigin::kCloud,
+ dict_, nullptr, nullptr);
+ instance->SetID("testId");
+
+ ErrorPtr error;
+ Error::AddTo(&error, FROM_HERE, "DOMAIN", "CODE", "MESSAGE");
+ instance->Abort(error.get(), nullptr);
+
+ json->MergeDictionary(CreateDictionaryValue(R"({
+ 'id': 'testId',
'state': 'aborted',
- 'results': {'testResult': 17},
+ 'progress': {},
+ 'results': {},
'error': {'code': 'CODE', 'message': 'MESSAGE'}
})").get());
diff --git a/libweave/src/commands/schema_constants.cc b/libweave/src/commands/schema_constants.cc
index f53423f..0b0ba40 100644
--- a/libweave/src/commands/schema_constants.cc
+++ b/libweave/src/commands/schema_constants.cc
@@ -26,6 +26,7 @@
const char kInvalidCommandVisibility[] = "invalid_command_visibility";
const char kInvalidMinimalRole[] = "invalid_minimal_role";
const char kCommandDestroyed[] = "command_destroyed";
+const char kInvalidState[] = "invalid_state";
} // namespace commands
} // namespace errors
diff --git a/libweave/src/commands/schema_constants.h b/libweave/src/commands/schema_constants.h
index 6dded05..6cd3061 100644
--- a/libweave/src/commands/schema_constants.h
+++ b/libweave/src/commands/schema_constants.h
@@ -29,6 +29,7 @@
extern const char kInvalidCommandVisibility[];
extern const char kInvalidMinimalRole[];
extern const char kCommandDestroyed[];
+extern const char kInvalidState[];
} // namespace commands
} // namespace errors
