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/examples/ubuntu/main.cc b/libweave/examples/ubuntu/main.cc index 4c4d251..ff0d450 100644 --- a/libweave/examples/ubuntu/main.cc +++ b/libweave/examples/ubuntu/main.cc
@@ -119,10 +119,7 @@ result.SetString("_greeting", "Hello " + name); cmd->SetResults(result, nullptr); LOG(INFO) << cmd->GetName() << " command finished: " << result; - LOG(INFO) << "New state: " << *device_->GetState(); - - cmd->Done(); } void OnGreetCommand(const std::weak_ptr<weave::Command>& command) { @@ -157,12 +154,13 @@ if (cmd_value != cur_state) { UpdateLedState(); } - return cmd->Done(); + cmd->SetResults({}, nullptr); + return; } weave::ErrorPtr error; weave::Error::AddTo(&error, FROM_HERE, "example", "invalid_parameter_value", "Invalid parameters"); - cmd->Abort(error.get()); + cmd->Abort(error.get(), nullptr); } void OnFlasherToggleCommand(const std::weak_ptr<weave::Command>& command) { @@ -177,12 +175,13 @@ led_status_[led_index] = ~led_status_[led_index]; UpdateLedState(); - return cmd->Done(); + cmd->SetResults({}, nullptr); + return; } weave::ErrorPtr error; weave::Error::AddTo(&error, FROM_HERE, "example", "invalid_parameter_value", "Invalid parameters"); - cmd->Abort(error.get()); + cmd->Abort(error.get(), nullptr); } void OnUnhandledCommand(const std::weak_ptr<weave::Command>& command) {
diff --git a/libweave/include/weave/command.h b/libweave/include/weave/command.h index b8bbbd7..c09da32 100644 --- a/libweave/include/weave/command.h +++ b/libweave/include/weave/command.h
@@ -48,6 +48,9 @@ // Returns the command results. virtual std::unique_ptr<base::DictionaryValue> GetResults() const = 0; + // Returns the command error. + virtual const Error* GetError() 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, @@ -55,17 +58,26 @@ // Updates the command results. The |results| should match the schema. // Returns false if |results| value is incorrect. + // Sets command into terminal "done" state. + // TODO(vitalybuka): Rename to Complete. virtual bool SetResults(const base::DictionaryValue& results, ErrorPtr* error) = 0; + // Sets command into paused state. + // This is not terminal state. Command can be resumed with |SetProgress| call. + virtual bool Pause(ErrorPtr* error) = 0; + + // Sets command into error state and assign error. + // This is not terminal state. Command can be resumed with |SetProgress| call. + virtual bool SetError(const Error* command_error, ErrorPtr* error) = 0; + // Aborts command execution. - virtual void Abort(const Error* error) = 0; + // Sets command into terminal "aborted" state. + virtual bool Abort(const Error* command_error, ErrorPtr* error) = 0; // Cancels command execution. - virtual void Cancel() = 0; - - // Marks the command as completed successfully. - virtual void Done() = 0; + // Sets command into terminal "canceled" state. + virtual bool Cancel(ErrorPtr* error) = 0; protected: virtual ~Command() = default;
diff --git a/libweave/include/weave/test/mock_command.h b/libweave/include/weave/test/mock_command.h index 07fe4b3..c4324df 100644 --- a/libweave/include/weave/test/mock_command.h +++ b/libweave/include/weave/test/mock_command.h
@@ -28,11 +28,13 @@ MOCK_CONST_METHOD0(MockGetParameters, const std::string&()); MOCK_CONST_METHOD0(MockGetProgress, const std::string&()); MOCK_CONST_METHOD0(MockGetResults, const std::string&()); + MOCK_CONST_METHOD0(GetError, const Error*()); MOCK_METHOD2(SetProgress, bool(const base::DictionaryValue&, ErrorPtr*)); MOCK_METHOD2(SetResults, bool(const base::DictionaryValue&, ErrorPtr*)); - MOCK_METHOD1(Abort, void(const Error*)); - MOCK_METHOD0(Cancel, void()); - MOCK_METHOD0(Done, void()); + MOCK_METHOD1(Pause, bool(ErrorPtr*)); + MOCK_METHOD2(SetError, bool(const Error*, ErrorPtr*)); + MOCK_METHOD2(Abort, bool(const Error*, ErrorPtr*)); + MOCK_METHOD1(Cancel, bool(ErrorPtr*)); std::unique_ptr<base::DictionaryValue> GetParameters() const override; std::unique_ptr<base::DictionaryValue> GetProgress() const override;
diff --git a/libweave/src/base_api_handler.cc b/libweave/src/base_api_handler.cc index 9e457e8..273162a 100644 --- a/libweave/src/base_api_handler.cc +++ b/libweave/src/base_api_handler.cc
@@ -74,13 +74,14 @@ errors::commands::kInvalidPropValue, "Invalid localAnonymousAccessMaxRole value '%s'", anonymous_access_role.c_str()); - return command->Abort(error.get()); + command->Abort(error.get(), nullptr); + return; } device_info_->UpdateBaseConfig(auth_scope, discovery_enabled, pairing_enabled); - command->Done(); + command->SetResults({}, nullptr); } void BaseApiHandler::OnConfigChanged(const Settings& settings) { @@ -112,7 +113,7 @@ parameters->GetString("location", &location); device_info_->UpdateDeviceInfo(name, description, location); - command->Done(); + command->SetResults({}, nullptr); } } // namespace weave
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
diff --git a/libweave/src/device_registration_info_unittest.cc b/libweave/src/device_registration_info_unittest.cc index f5c53fa..71c9832 100644 --- a/libweave/src/device_registration_info_unittest.cc +++ b/libweave/src/device_registration_info_unittest.cc
@@ -502,54 +502,51 @@ EXPECT_EQ(GcdState::kConnecting, GetGcdState()); } -TEST_F(DeviceRegistrationInfoTest, UpdateCommand) { - ReloadSettings(); - SetAccessToken(); +class DeviceRegistrationInfoUpdateCommandTest + : public DeviceRegistrationInfoTest { + protected: + void SetUp() override { + DeviceRegistrationInfoTest::SetUp(); - auto json_cmds = CreateDictionaryValue(R"({ - 'robot': { - '_jump': { - 'parameters': {'_height': 'integer'}, - 'progress': {'progress': 'integer'}, - 'results': {'status': 'string'}, - 'minimalRole': 'user' + ReloadSettings(); + SetAccessToken(); + + auto json_cmds = CreateDictionaryValue(R"({ + 'robot': { + '_jump': { + 'parameters': {'_height': 'integer'}, + 'progress': {'progress': 'integer'}, + 'results': {'status': 'string'}, + 'minimalRole': 'user' + } } - } - })"); - EXPECT_TRUE(command_manager_->LoadCommands(*json_cmds, nullptr)); + })"); + EXPECT_TRUE(command_manager_->LoadCommands(*json_cmds, nullptr)); - const std::string command_url = dev_reg_->GetServiceURL("commands/1234"); + command_url_ = dev_reg_->GetServiceURL("commands/1234"); - auto commands_json = CreateValue(R"([{ - 'name':'robot._jump', - 'id':'1234', - 'parameters': {'_height': 100}, - 'minimalRole': 'user' - }])"); - ASSERT_NE(nullptr, commands_json.get()); - const base::ListValue* command_list = nullptr; - ASSERT_TRUE(commands_json->GetAsList(&command_list)); - PublishCommands(*command_list); - auto command = command_manager_->FindCommand("1234"); - ASSERT_NE(nullptr, command); + auto commands_json = CreateValue(R"([{ + 'name':'robot._jump', + 'id':'1234', + 'parameters': {'_height': 100}, + 'minimalRole': 'user' + }])"); + ASSERT_NE(nullptr, commands_json.get()); + const base::ListValue* command_list = nullptr; + ASSERT_TRUE(commands_json->GetAsList(&command_list)); + PublishCommands(*command_list); + command_ = command_manager_->FindCommand("1234"); + ASSERT_NE(nullptr, command_); + } + Command* command_{nullptr}; + std::string command_url_; +}; + +TEST_F(DeviceRegistrationInfoUpdateCommandTest, SetProgress) { EXPECT_CALL(http_client_, MockSendRequest( - http::kPatch, command_url, - HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _)) - .WillOnce(WithArgs<3>(Invoke([](const std::string& data) { - EXPECT_JSON_EQ(R"({"results":{"status":"Ok"}})", - *CreateDictionaryValue(data)); - base::DictionaryValue json; - return ReplyWithJson(200, json); - }))); - EXPECT_TRUE( - command->SetResults(*CreateDictionaryValue("{'status': 'Ok'}"), nullptr)); - Mock::VerifyAndClearExpectations(&http_client_); - - EXPECT_CALL(http_client_, - MockSendRequest( - http::kPatch, command_url, + http::kPatch, command_url_, HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _)) .WillOnce(WithArgs<3>(Invoke([](const std::string& data) { EXPECT_JSON_EQ(R"({"state":"inProgress"})", @@ -563,13 +560,36 @@ base::DictionaryValue json; return ReplyWithJson(200, json); }))); - EXPECT_TRUE( - command->SetProgress(*CreateDictionaryValue("{'progress':18}"), nullptr)); + EXPECT_TRUE(command_->SetProgress(*CreateDictionaryValue("{'progress':18}"), + nullptr)); Mock::VerifyAndClearExpectations(&http_client_); +} +TEST_F(DeviceRegistrationInfoUpdateCommandTest, SetResults) { EXPECT_CALL(http_client_, MockSendRequest( - http::kPatch, command_url, + http::kPatch, command_url_, + HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _)) + .WillOnce(WithArgs<3>(Invoke([](const std::string& data) { + EXPECT_JSON_EQ(R"({"results":{"status":"Ok"}})", + *CreateDictionaryValue(data)); + base::DictionaryValue json; + return ReplyWithJson(200, json); + }))) + .WillOnce(WithArgs<3>(Invoke([](const std::string& data) { + EXPECT_JSON_EQ(R"({"state":"done"})", *CreateDictionaryValue(data)); + base::DictionaryValue json; + return ReplyWithJson(200, json); + }))); + EXPECT_TRUE(command_->SetResults(*CreateDictionaryValue("{'status': 'Ok'}"), + nullptr)); + Mock::VerifyAndClearExpectations(&http_client_); +} + +TEST_F(DeviceRegistrationInfoUpdateCommandTest, Cancel) { + EXPECT_CALL(http_client_, + MockSendRequest( + http::kPatch, command_url_, HttpClient::Headers{GetAuthHeader(), GetJsonHeader()}, _, _)) .WillOnce(WithArgs<3>(Invoke([](const std::string& data) { EXPECT_JSON_EQ(R"({"state":"cancelled"})", @@ -577,7 +597,7 @@ base::DictionaryValue json; return ReplyWithJson(200, json); }))); - command->Cancel(); + EXPECT_TRUE(command_->Cancel(nullptr)); Mock::VerifyAndClearExpectations(&http_client_); }
diff --git a/libweave/src/privet/cloud_delegate.cc b/libweave/src/privet/cloud_delegate.cc index f7a803e..045a235 100644 --- a/libweave/src/privet/cloud_delegate.cc +++ b/libweave/src/privet/cloud_delegate.cc
@@ -196,10 +196,8 @@ CHECK(user_info.scope() != AuthScope::kNone); ErrorPtr error; auto command = GetCommandInternal(id, user_info, &error); - if (!command) + if (!command || !command->Cancel(&error)) return error_callback.Run(error.get()); - - command->Cancel(); success_callback.Run(*command->ToJson()); }