Remove schema validation from command parameters/returns
Do not use ObjectSchema and instead pass parameters, progress and
return values as opaque JSON objects.
BUG: 25841230
Change-Id: I0ea5fc31d526b1e5d6c66453b613e7284aa3fcac
Reviewed-on: https://weave-review.googlesource.com/1611
Reviewed-by: Vitaly Buka <vitalybuka@google.com>
diff --git a/src/commands/command_instance.cc b/src/commands/command_instance.cc
index f152d82..aa71b0e 100644
--- a/src/commands/command_instance.cc
+++ b/src/commands/command_instance.cc
@@ -12,9 +12,7 @@
#include "src/commands/command_definition.h"
#include "src/commands/command_dictionary.h"
#include "src/commands/command_queue.h"
-#include "src/commands/prop_types.h"
#include "src/commands/schema_constants.h"
-#include "src/commands/schema_utils.h"
#include "src/json_error_codes.h"
#include "src/utils.h"
@@ -68,12 +66,12 @@
CommandInstance::CommandInstance(const std::string& name,
Command::Origin origin,
const CommandDefinition* command_definition,
- const ValueMap& parameters)
+ const base::DictionaryValue& parameters)
: name_{name},
origin_{origin},
- command_definition_{command_definition},
- parameters_{parameters} {
+ command_definition_{command_definition} {
CHECK(command_definition_);
+ parameters_.MergeDictionary(¶meters);
}
CommandInstance::~CommandInstance() {
@@ -97,15 +95,15 @@
}
std::unique_ptr<base::DictionaryValue> CommandInstance::GetParameters() const {
- return TypedValueToJson(parameters_);
+ return std::unique_ptr<base::DictionaryValue>(parameters_.DeepCopy());
}
std::unique_ptr<base::DictionaryValue> CommandInstance::GetProgress() const {
- return TypedValueToJson(progress_);
+ return std::unique_ptr<base::DictionaryValue>(progress_.DeepCopy());
}
std::unique_ptr<base::DictionaryValue> CommandInstance::GetResults() const {
- return TypedValueToJson(results_);
+ return std::unique_ptr<base::DictionaryValue>(results_.DeepCopy());
}
const Error* CommandInstance::GetError() const {
@@ -114,21 +112,13 @@
bool CommandInstance::SetProgress(const base::DictionaryValue& progress,
ErrorPtr* error) {
- if (!command_definition_)
- return ReportDestroyedError(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%.
if (!SetStatus(State::kInProgress, error))
return false;
- if (obj != progress_) {
- progress_ = obj;
+ if (!progress_.Equals(&progress)) {
+ progress_.Clear();
+ progress_.MergeDictionary(&progress);
FOR_EACH_OBSERVER(Observer, observers_, OnProgressChanged());
}
@@ -137,17 +127,9 @@
bool CommandInstance::Complete(const base::DictionaryValue& results,
ErrorPtr* error) {
- if (!command_definition_)
- return ReportDestroyedError(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;
+ if (!results_.Equals(&results)) {
+ results_.Clear();
+ results_.MergeDictionary(&results);
FOR_EACH_OBSERVER(Observer, observers_, OnResultsChanged());
}
// Change status even if result is unchanged.
@@ -171,36 +153,29 @@
// On success, returns |true| and the validated parameters and values through
// |parameters|. Otherwise returns |false| and additional error information in
// |error|.
-bool GetCommandParameters(const base::DictionaryValue* json,
- const CommandDefinition* command_def,
- ValueMap* parameters,
- ErrorPtr* error) {
+std::unique_ptr<base::DictionaryValue> GetCommandParameters(
+ const base::DictionaryValue* json,
+ const CommandDefinition* command_def,
+ ErrorPtr* error) {
// Get the command parameters from 'parameters' property.
- base::DictionaryValue no_params; // Placeholder when no params are specified.
- const base::DictionaryValue* params = nullptr;
+ std::unique_ptr<base::DictionaryValue> params;
const base::Value* params_value = nullptr;
if (json->Get(commands::attributes::kCommand_Parameters, ¶ms_value)) {
// Make sure the "parameters" property is actually an object.
- if (!params_value->GetAsDictionary(¶ms)) {
+ const base::DictionaryValue* params_dict = nullptr;
+ if (!params_value->GetAsDictionary(¶ms_dict)) {
Error::AddToPrintf(error, FROM_HERE, errors::json::kDomain,
errors::json::kObjectExpected,
"Property '%s' must be a JSON object",
commands::attributes::kCommand_Parameters);
- return false;
+ return params;
}
+ params.reset(params_dict->DeepCopy());
} else {
// "parameters" are not specified. Assume empty param list.
- params = &no_params;
+ params.reset(new base::DictionaryValue);
}
-
- // Now read in the parameters and validate their values against the command
- // definition schema.
- ObjectPropType obj_prop_type;
- obj_prop_type.SetObjectSchema(command_def->GetParameters()->Clone());
- if (!TypedValueFromJson(params, &obj_prop_type, parameters, error)) {
- return false;
- }
- return true;
+ return params;
}
} // anonymous namespace
@@ -246,8 +221,8 @@
return instance;
}
- ValueMap parameters;
- if (!GetCommandParameters(json, command_def, ¶meters, error)) {
+ auto parameters = GetCommandParameters(json, command_def, error);
+ if (!parameters) {
Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain,
errors::commands::kCommandFailed,
"Failed to validate command '%s'", command_name.c_str());
@@ -255,7 +230,7 @@
}
instance.reset(
- new CommandInstance{command_name, origin, command_def, parameters});
+ new CommandInstance{command_name, origin, command_def, *parameters});
if (!command_id->empty())
instance->SetID(*command_id);
@@ -268,12 +243,9 @@
json->SetString(commands::attributes::kCommand_Id, id_);
json->SetString(commands::attributes::kCommand_Name, name_);
- json->Set(commands::attributes::kCommand_Parameters,
- TypedValueToJson(parameters_).release());
- json->Set(commands::attributes::kCommand_Progress,
- TypedValueToJson(progress_).release());
- json->Set(commands::attributes::kCommand_Results,
- TypedValueToJson(results_).release());
+ json->Set(commands::attributes::kCommand_Parameters, parameters_.DeepCopy());
+ json->Set(commands::attributes::kCommand_Progress, progress_.DeepCopy());
+ json->Set(commands::attributes::kCommand_Results, results_.DeepCopy());
json->SetString(commands::attributes::kCommand_State, EnumToString(state_));
if (error_) {
json->Set(commands::attributes::kCommand_Error,
diff --git a/src/commands/command_instance.h b/src/commands/command_instance.h
index 5a2ebf7..30ef907 100644
--- a/src/commands/command_instance.h
+++ b/src/commands/command_instance.h
@@ -44,12 +44,12 @@
};
// Construct a command instance given the full command |name| which must
- // be in format "<package_name>.<command_name>", a command |category| and
- // a list of parameters and their values specified in |parameters|.
+ // be in format "<package_name>.<command_name>" and a list of parameters and
+ // their values specified in |parameters|.
CommandInstance(const std::string& name,
Command::Origin origin,
const CommandDefinition* command_definition,
- const ValueMap& parameters);
+ const base::DictionaryValue& parameters);
~CommandInstance() override;
// Command overrides.
@@ -124,11 +124,11 @@
// Command definition.
const CommandDefinition* command_definition_{nullptr};
// Command parameters and their values.
- ValueMap parameters_;
+ base::DictionaryValue parameters_;
// Current command execution progress.
- ValueMap progress_;
+ base::DictionaryValue progress_;
// Command results.
- ValueMap results_;
+ base::DictionaryValue results_;
// Current command state.
Command::State state_ = Command::State::kQueued;
// Error encountered during execution of the command.
diff --git a/src/commands/command_instance_unittest.cc b/src/commands/command_instance_unittest.cc
index 4e208ed..a03e92b 100644
--- a/src/commands/command_instance_unittest.cc
+++ b/src/commands/command_instance_unittest.cc
@@ -7,8 +7,6 @@
#include <gtest/gtest.h>
#include "src/commands/command_dictionary.h"
-#include "src/commands/prop_types.h"
-#include "src/commands/schema_utils.h"
#include "src/commands/unittest_utils.h"
namespace weave {
@@ -70,14 +68,12 @@
} // anonymous namespace
TEST_F(CommandInstanceTest, Test) {
- StringPropType str_prop;
- IntPropType int_prop;
- ValueMap params;
- params["phrase"] =
- str_prop.CreateValue(base::StringValue{"iPityDaFool"}, nullptr);
- params["volume"] = int_prop.CreateValue(base::FundamentalValue{5}, nullptr);
+ auto params = CreateDictionaryValue(R"({
+ 'phrase': 'iPityDaFool',
+ 'volume': 5
+ })");
CommandInstance instance{"robot.speak", Command::Origin::kCloud,
- dict_.FindCommand("robot.speak"), params};
+ dict_.FindCommand("robot.speak"), *params};
EXPECT_TRUE(
instance.Complete(*CreateDictionaryValue("{'foo': 239}"), nullptr));
@@ -174,25 +170,6 @@
EXPECT_EQ("command_failed", error->GetCode());
}
-TEST_F(CommandInstanceTest, FromJson_ParamError) {
- auto json = CreateDictionaryValue(R"({
- 'name': 'robot.speak',
- 'parameters': {
- 'phrase': 'iPityDaFool',
- 'volume': 20
- }
- })");
- ErrorPtr error;
- auto instance = CommandInstance::FromJson(json.get(), Command::Origin::kCloud,
- dict_, nullptr, &error);
- EXPECT_EQ(nullptr, instance.get());
- auto first = error->GetFirstError();
- EXPECT_EQ("out_of_range", first->GetCode());
- auto inner = error->GetInnerError();
- EXPECT_EQ("invalid_parameter_value", inner->GetCode());
- EXPECT_EQ("command_failed", error->GetCode());
-}
-
TEST_F(CommandInstanceTest, ToJson) {
auto json = CreateDictionaryValue(R"({
'name': 'robot.jump',
