Revert "Revert "Merge remote-tracking branch 'weave/master' into 'weave/aosp-master'"" This reverts commit 56686fe8ce35e287ffbece5fe042aa2e654d73bc. Change-Id: I404b98dbaf341de4ea47575251b28ea382bc7f4b Reviewed-on: https://weave-review.googlesource.com/1637 Reviewed-by: Alex Vakulenko <avakulenko@google.com>
diff --git a/Android.mk b/Android.mk deleted file mode 100644 index 9833c4b..0000000 --- a/Android.mk +++ /dev/null
@@ -1,230 +0,0 @@ -# Copyright (C) 2015 The Android Open Source Project -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -LOCAL_PATH := $(call my-dir) - -# Common variables -# ======================================================== - -libweaveCommonCppExtension := .cc -libweaveCommonCFlags := -Wall -Werror \ - -Wno-char-subscripts -Wno-missing-field-initializers \ - -Wno-unused-function -Wno-unused-parameter - -libweaveCommonCppFlags := \ - -Wno-deprecated-register \ - -Wno-sign-compare \ - -Wno-sign-promo \ - -Wno-non-virtual-dtor \ - -libweaveCommonCIncludes := \ - $(LOCAL_PATH)/.. \ - $(LOCAL_PATH)/include \ - $(LOCAL_PATH)/third_party/modp_b64/modp_b64 \ - external/gtest/include \ - -libweaveSharedLibraries := \ - libchrome \ - libexpat \ - libcrypto \ - -# libweave-external -# ======================================================== -include $(CLEAR_VARS) -LOCAL_MODULE := libweave-external -LOCAL_CPP_EXTENSION := $(libweaveCommonCppExtension) -LOCAL_CFLAGS := $(libweaveCommonCFlags) -LOCAL_CPPFLAGS := $(libweaveCommonCppFlags) -LOCAL_C_INCLUDES := $(libweaveCommonCIncludes) -LOCAL_SHARED_LIBRARIES := $(libweaveSharedLibraries) -LOCAL_STATIC_LIBRARIES := -LOCAL_RTTI_FLAG := -frtti -LOCAL_CLANG := true -LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/external - -LOCAL_SRC_FILES := \ - third_party/chromium/crypto/p224.cc \ - third_party/chromium/crypto/p224_spake.cc \ - third_party/chromium/crypto/sha2.cc \ - third_party/modp_b64/modp_b64.cc \ - -include $(BUILD_STATIC_LIBRARY) - -# libweave-common -# ======================================================== -include $(CLEAR_VARS) -LOCAL_MODULE := libweave-common -LOCAL_CPP_EXTENSION := $(libweaveCommonCppExtension) -LOCAL_CFLAGS := $(libweaveCommonCFlags) -LOCAL_CPPFLAGS := $(libweaveCommonCppFlags) -LOCAL_C_INCLUDES := $(libweaveCommonCIncludes) -LOCAL_SHARED_LIBRARIES := $(libweaveSharedLibraries) -LOCAL_STATIC_LIBRARIES := libweave-external -LOCAL_RTTI_FLAG := -frtti -LOCAL_CLANG := true -LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH) - -LOCAL_SRC_FILES := \ - src/backoff_entry.cc \ - src/base_api_handler.cc \ - src/commands/cloud_command_proxy.cc \ - src/commands/command_definition.cc \ - src/commands/command_dictionary.cc \ - src/commands/command_instance.cc \ - src/commands/command_manager.cc \ - src/commands/command_queue.cc \ - src/commands/object_schema.cc \ - src/commands/prop_constraints.cc \ - src/commands/prop_types.cc \ - src/commands/prop_values.cc \ - src/commands/schema_constants.cc \ - src/commands/schema_utils.cc \ - src/config.cc \ - src/data_encoding.cc \ - src/device_manager.cc \ - src/device_registration_info.cc \ - src/error.cc \ - src/http_constants.cc \ - src/json_error_codes.cc \ - src/notification/notification_parser.cc \ - src/notification/pull_channel.cc \ - src/notification/xml_node.cc \ - src/notification/xmpp_channel.cc \ - src/notification/xmpp_iq_stanza_handler.cc \ - src/notification/xmpp_stream_parser.cc \ - src/privet/cloud_delegate.cc \ - src/privet/constants.cc \ - src/privet/device_delegate.cc \ - src/privet/device_ui_kind.cc \ - src/privet/openssl_utils.cc \ - src/privet/privet_handler.cc \ - src/privet/privet_manager.cc \ - src/privet/privet_types.cc \ - src/privet/publisher.cc \ - src/privet/security_manager.cc \ - src/privet/wifi_bootstrap_manager.cc \ - src/privet/wifi_ssid_generator.cc \ - src/registration_status.cc \ - src/states/error_codes.cc \ - src/states/state_change_queue.cc \ - src/states/state_manager.cc \ - src/states/state_package.cc \ - src/string_utils.cc \ - src/utils.cc \ - -include $(BUILD_STATIC_LIBRARY) - -# libweave-test -# ======================================================== -include $(CLEAR_VARS) -LOCAL_MODULE := libweave-test -LOCAL_CPP_EXTENSION := $(libweaveCommonCppExtension) -LOCAL_CFLAGS := $(libweaveCommonCFlags) -LOCAL_CPPFLAGS := $(libweaveCommonCppFlags) -LOCAL_C_INCLUDES := \ - $(libweaveCommonCIncludes) \ - external/gmock/include \ - -LOCAL_SHARED_LIBRARIES := $(libweaveSharedLibraries) -LOCAL_STATIC_LIBRARIES := libgtest libgmock -LOCAL_CLANG := true -LOCAL_RTTI_FLAG := -frtti -LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include - -LOCAL_SRC_FILES := \ - src/test/fake_stream.cc \ - src/test/fake_task_runner.cc \ - src/test/mock_command.cc \ - src/test/unittest_utils.cc \ - -include $(BUILD_STATIC_LIBRARY) - -# libweave -# ======================================================== -include $(CLEAR_VARS) -LOCAL_MODULE := libweave -LOCAL_CPP_EXTENSION := $(libweaveCommonCppExtension) -LOCAL_CFLAGS := $(libweaveCommonCFlags) -LOCAL_CPPFLAGS := $(libweaveCommonCppFlags) -LOCAL_C_INCLUDES := $(libweaveCommonCIncludes) -LOCAL_SHARED_LIBRARIES := $(libweaveSharedLibraries) -LOCAL_WHOLE_STATIC_LIBRARIES := libweave-external libweave-common -LOCAL_CLANG := true -LOCAL_RTTI_FLAG := -frtti -LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include - -LOCAL_SRC_FILES := \ - src/empty.cc - -include $(BUILD_SHARED_LIBRARY) - -# libweave_test -# ======================================================== -include $(CLEAR_VARS) -LOCAL_MODULE := libweave_test -LOCAL_MODULE_TAGS := debug -LOCAL_CPP_EXTENSION := $(libweaveCommonCppExtension) -LOCAL_CFLAGS := $(libweaveCommonCFlags) -LOCAL_CPPFLAGS := $(libweaveCommonCppFlags) -LOCAL_C_INCLUDES := \ - $(libweaveCommonCIncludes) \ - external/gmock/include \ - -LOCAL_SHARED_LIBRARIES := \ - $(libweaveSharedLibraries) \ - -LOCAL_STATIC_LIBRARIES := \ - libweave-external \ - libweave-common \ - libweave-test \ - libgtest libgmock \ - libchrome_test_helpers \ - -LOCAL_CLANG := true -LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH) - -LOCAL_SRC_FILES := \ - src/backoff_entry_unittest.cc \ - src/base_api_handler_unittest.cc \ - src/commands/cloud_command_proxy_unittest.cc \ - src/commands/command_definition_unittest.cc \ - src/commands/command_dictionary_unittest.cc \ - src/commands/command_instance_unittest.cc \ - src/commands/command_manager_unittest.cc \ - src/commands/command_queue_unittest.cc \ - src/commands/object_schema_unittest.cc \ - src/commands/schema_utils_unittest.cc \ - src/config_unittest.cc \ - src/data_encoding_unittest.cc \ - src/device_registration_info_unittest.cc \ - src/error_unittest.cc \ - src/notification/notification_parser_unittest.cc \ - src/notification/xml_node_unittest.cc \ - src/notification/xmpp_channel_unittest.cc \ - src/notification/xmpp_iq_stanza_handler_unittest.cc \ - src/notification/xmpp_stream_parser_unittest.cc \ - src/privet/privet_handler_unittest.cc \ - src/privet/security_manager_unittest.cc \ - src/privet/wifi_ssid_generator_unittest.cc \ - src/states/state_change_queue_unittest.cc \ - src/states/state_manager_unittest.cc \ - src/states/state_package_unittest.cc \ - src/string_utils_unittest.cc \ - src/test/weave_testrunner.cc \ - src/weave_unittest.cc \ - third_party/chromium/crypto/p224_spake_unittest.cc \ - third_party/chromium/crypto/p224_unittest.cc \ - third_party/chromium/crypto/sha2_unittest.cc \ - -include $(BUILD_NATIVE_TEST)
diff --git a/examples/daemon/ledflasher/ledflasher.cc b/examples/daemon/ledflasher/ledflasher.cc index 38314f5..0019d2c 100644 --- a/examples/daemon/ledflasher/ledflasher.cc +++ b/examples/daemon/ledflasher/ledflasher.cc
@@ -25,7 +25,7 @@ device_ = device; device->AddStateDefinitionsFromJson(R"({ - "_ledflasher": {"_leds": {"items": "boolean"}} + "_ledflasher": {"_leds": {"type": "array", "items": {"type": "boolean"}}} })"); device->SetStatePropertiesFromJson(R"({ @@ -37,13 +37,13 @@ "_ledflasher": { "_set":{ "parameters": { - "_led": {"minimum": 1, "maximum": 3}, - "_on": "boolean" + "_led": {"type": "integer", "minimum": 1, "maximum": 3}, + "_on": {"type": "boolean"} } }, "_toggle":{ "parameters": { - "_led": {"minimum": 1, "maximum": 3} + "_led": {"type": "integer", "minimum": 1, "maximum": 3} } } }
diff --git a/examples/daemon/light/light.cc b/examples/daemon/light/light.cc index d54de93..a7eb9b3 100644 --- a/examples/daemon/light/light.cc +++ b/examples/daemon/light/light.cc
@@ -18,31 +18,31 @@ device_ = device; device->AddStateDefinitionsFromJson(R"({ - "onOff": {"state": ["on", "standby"]}, - "brightness": {"brightness": "integer"}, + "onOff": {"state": {"type": "string", "enum": ["on", "standby"]}}, + "brightness": {"brightness": {"type": "integer"}}, "colorXY": { "colorSetting": { "properties": { - "colorX": {"minimum": 0.0, "maximum": 1.0}, - "colorY": {"minimum": 0.0, "maximum": 1.0} + "colorX": {"type": "number", "minimum": 0.0, "maximum": 1.0}, + "colorY": {"type": "number", "minimum": 0.0, "maximum": 1.0} } }, "colorCapRed": { "properties": { - "colorX": {"minimum": 0.0, "maximum": 1.0}, - "colorY": {"minimum": 0.0, "maximum": 1.0} + "colorX": {"type": "number", "minimum": 0.0, "maximum": 1.0}, + "colorY": {"type": "number", "minimum": 0.0, "maximum": 1.0} } }, "colorCapGreen": { "properties": { - "colorX": {"minimum": 0.0, "maximum": 1.0}, - "colorY": {"minimum": 0.0, "maximum": 1.0} + "colorX": {"type": "number", "minimum": 0.0, "maximum": 1.0}, + "colorY": {"type": "number", "minimum": 0.0, "maximum": 1.0} } }, "colorCapBlue": { "properties": { - "colorX": {"minimum": 0.0, "maximum": 1.0}, - "colorY": {"minimum": 0.0, "maximum": 1.0} + "colorX": {"type": "number", "minimum": 0.0, "maximum": 1.0}, + "colorY": {"type": "number", "minimum": 0.0, "maximum": 1.0} } } } @@ -64,7 +64,7 @@ "onOff": { "setConfig":{ "parameters": { - "state": ["on", "standby"] + "state": {"type": "string", "enum": ["on", "standby"]} } } },
diff --git a/examples/daemon/lock/lock.cc b/examples/daemon/lock/lock.cc index 3014fb1..7d941c6 100644 --- a/examples/daemon/lock/lock.cc +++ b/examples/daemon/lock/lock.cc
@@ -35,8 +35,11 @@ device->AddStateDefinitionsFromJson(R"({ "lock": { - "lockedState": ["locked", "unlocked", "partiallyLocked"], - "isLockingSupported": "boolean"} + "lockedState": { + "type": "string", + "enum": ["locked", "unlocked", "partiallyLocked"], + } + "isLockingSupported": {"type": "boolean"}} })"); device->SetStatePropertiesFromJson(R"({ @@ -51,7 +54,7 @@ "lock": { "setConfig":{ "parameters": { - "lockedState": ["locked", "unlocked"] + "lockedState": {"type": "string", "enum":["locked", "unlocked"]} } } }
diff --git a/examples/daemon/sample/sample.cc b/examples/daemon/sample/sample.cc index 905a977..e065b06 100644 --- a/examples/daemon/sample/sample.cc +++ b/examples/daemon/sample/sample.cc
@@ -27,9 +27,9 @@ "_hello": { "minimalRole": "user", "parameters": { - "_name": "string" + "_name": {"type": "string"} }, - "results": { "_reply": "string" } + "results": { "_reply": {"type": "string"} } }, "_ping": { "minimalRole": "user", @@ -38,16 +38,16 @@ "_countdown": { "minimalRole": "user", "parameters": { - "_seconds": {"minimum": 1, "maximum": 25} + "_seconds": {"type": "integer", "minimum": 1, "maximum": 25} }, - "progress": { "_seconds_left": "integer"}, + "progress": { "_seconds_left": {"type": "integer"}}, "results": {} } } })"); device->AddStateDefinitionsFromJson(R"({ - "_sample": {"_ping_count":"integer"} + "_sample": {"_ping_count": {"type": "integer"}} })"); device->SetStatePropertiesFromJson(R"({
diff --git a/examples/daemon/speaker/speaker.cc b/examples/daemon/speaker/speaker.cc index 32591f9..178be14 100644 --- a/examples/daemon/speaker/speaker.cc +++ b/examples/daemon/speaker/speaker.cc
@@ -18,10 +18,10 @@ device_ = device; device->AddStateDefinitionsFromJson(R"({ - "onOff": {"state": ["on", "standby"]}, + "onOff": {"state": {"type": "string", "enum": ["on", "standby"]}}, "volume": { - "volume": "integer", - "isMuted": "boolean" + "volume": {"type": "integer"}, + "isMuted": {"type": "boolean"} } })"); @@ -38,7 +38,7 @@ "onOff": { "setConfig":{ "parameters": { - "state": ["on", "standby"] + "state": {"type": "string", "enum": ["on", "standby"]} } } }, @@ -50,7 +50,7 @@ "minimum": 0, "maximum": 100 }, - "isMuted": "boolean" + "isMuted": {"type": "boolean"} } } }
diff --git a/libweave_common.gypi b/libweave_common.gypi index 49f7ff9..73d02fd 100644 --- a/libweave_common.gypi +++ b/libweave_common.gypi
@@ -31,6 +31,7 @@ ], 'cflags!': ['-fPIE'], 'cflags': [ + '-fno-exceptions', '-fPIC', '-fvisibility=hidden', '-std=c++11',
diff --git a/src/base_api_handler.cc b/src/base_api_handler.cc index c3aa616..3d22a10 100644 --- a/src/base_api_handler.cc +++ b/src/base_api_handler.cc
@@ -42,20 +42,31 @@ "updateBaseConfiguration": { "minimalRole": "manager", "parameters": { - "localDiscoveryEnabled": "boolean", - "localAnonymousAccessMaxRole": [ "none", "viewer", "user" ], - "localPairingEnabled": "boolean" - }, - "results": {} + "localAnonymousAccessMaxRole": { + "enum": [ "none", "viewer", "user" ], + "type": "string" + }, + "localDiscoveryEnabled": { + "type": "boolean" + }, + "localPairingEnabled": { + "type": "boolean" + } + } }, "updateDeviceInfo": { "minimalRole": "manager", "parameters": { - "description": "string", - "name": "string", - "location": "string" - }, - "results": {} + "description": { + "type": "string" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + } + } } } })");
diff --git a/src/base_api_handler_unittest.cc b/src/base_api_handler_unittest.cc index a025e44..df78319 100644 --- a/src/base_api_handler_unittest.cc +++ b/src/base_api_handler_unittest.cc
@@ -31,7 +31,7 @@ class BaseApiHandlerTest : public ::testing::Test { protected: void SetUp() override { - EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_, _)) + EXPECT_CALL(mock_state_change_queue_, MockNotifyPropertiesUpdated(_, _)) .WillRepeatedly(Return(true)); command_manager_ = std::make_shared<CommandManager>(); @@ -107,39 +107,38 @@ TEST_F(BaseApiHandlerTest, Initialization) { auto command_defs = - command_manager_->GetCommandDictionary().GetCommandsAsJson( - [](const CommandDefinition* def) { return true; }, true, nullptr); + command_manager_->GetCommandDictionary().GetCommandsAsJson(nullptr); auto expected = R"({ "base": { "updateBaseConfiguration": { - "minimalRole": "manager", - "parameters": { - "localAnonymousAccessMaxRole": { - "enum": [ "none", "viewer", "user" ], - "type": "string" - }, - "localDiscoveryEnabled": { - "type": "boolean" - }, - "localPairingEnabled": { - "type": "boolean" - } - } + "minimalRole": "manager", + "parameters": { + "localAnonymousAccessMaxRole": { + "enum": [ "none", "viewer", "user" ], + "type": "string" + }, + "localDiscoveryEnabled": { + "type": "boolean" + }, + "localPairingEnabled": { + "type": "boolean" + } + } }, "updateDeviceInfo": { - "minimalRole": "manager", - "parameters": { - "description": { - "type": "string" - }, - "location": { - "type": "string" - }, - "name": { - "type": "string" - } - } + "minimalRole": "manager", + "parameters": { + "description": { + "type": "string" + }, + "location": { + "type": "string" + }, + "name": { + "type": "string" + } + } } } })";
diff --git a/src/commands/cloud_command_proxy_unittest.cc b/src/commands/cloud_command_proxy_unittest.cc index 0c04592..fdb22fc 100644 --- a/src/commands/cloud_command_proxy_unittest.cc +++ b/src/commands/cloud_command_proxy_unittest.cc
@@ -94,7 +94,7 @@ } })"); CHECK(json.get()); - CHECK(command_dictionary_.LoadCommands(*json, nullptr, nullptr)) + CHECK(command_dictionary_.LoadCommands(*json, nullptr)) << "Failed to parse test command dictionary"; CreateCommandInstance();
diff --git a/src/commands/command_definition.cc b/src/commands/command_definition.cc index d7ebc83..dbae630 100644 --- a/src/commands/command_definition.cc +++ b/src/commands/command_definition.cc
@@ -28,61 +28,31 @@ LIBWEAVE_EXPORT EnumToStringMap<UserRole>::EnumToStringMap() : EnumToStringMap(kMap) {} -bool CommandDefinition::Visibility::FromString(const std::string& str, - ErrorPtr* error) { - // This special case is useful for places where we want to make a command - // to ALL clients, even if new clients are added in the future. - if (str == commands::attributes::kCommand_Visibility_All) { - local = true; - cloud = true; - return true; - } +CommandDefinition::CommandDefinition(const base::DictionaryValue& definition, + UserRole minimal_role) + : minimal_role_{minimal_role} { + definition_.MergeDictionary(&definition); +} - // Clear any bits first. - local = false; - cloud = false; - if (str == commands::attributes::kCommand_Visibility_None) - return true; - - for (const std::string& value : Split(str, ",", true, true)) { - if (value == commands::attributes::kCommand_Visibility_Local) { - local = true; - } else if (value == commands::attributes::kCommand_Visibility_Cloud) { - cloud = true; - } else { +std::unique_ptr<CommandDefinition> CommandDefinition::FromJson( + const base::DictionaryValue& dict, ErrorPtr* error) { + std::unique_ptr<CommandDefinition> definition; + // Validate the 'minimalRole' value if present. That's the only thing we + // care about so far. + std::string value; + UserRole minimal_role; + if (dict.GetString(commands::attributes::kCommand_Role, &value)) { + if (!StringToEnum(value, &minimal_role)) { Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidPropValue, - "Invalid command visibility value '%s'", - value.c_str()); - return false; + errors::commands::kInvalidPropValue, + "Invalid role: '%s'", value.c_str()); + return definition; } + } else { + minimal_role = UserRole::kUser; } - return true; -} - -std::string CommandDefinition::Visibility::ToString() const { - if (local && cloud) - return commands::attributes::kCommand_Visibility_All; - if (!local && !cloud) - return commands::attributes::kCommand_Visibility_None; - if (local) - return commands::attributes::kCommand_Visibility_Local; - return commands::attributes::kCommand_Visibility_Cloud; -} - -CommandDefinition::CommandDefinition( - std::unique_ptr<const ObjectSchema> parameters, - std::unique_ptr<const ObjectSchema> progress, - std::unique_ptr<const ObjectSchema> results) - : parameters_{std::move(parameters)}, - progress_{std::move(progress)}, - results_{std::move(results)} { - // Set to be available to all clients by default. - visibility_ = Visibility::GetAll(); -} - -void CommandDefinition::SetVisibility(const Visibility& visibility) { - visibility_ = visibility; + definition.reset(new CommandDefinition{dict, minimal_role}); + return definition; } } // namespace weave
diff --git a/src/commands/command_definition.h b/src/commands/command_definition.h index 3bcc07f..da02bf5 100644 --- a/src/commands/command_definition.h +++ b/src/commands/command_definition.h
@@ -9,8 +9,8 @@ #include <string> #include <base/macros.h> - -#include "src/commands/object_schema.h" +#include <base/values.h> +#include <weave/error.h> namespace weave { @@ -25,55 +25,19 @@ // describing the command parameter types and constraints. class CommandDefinition final { public: - struct Visibility { - Visibility() = default; - Visibility(bool is_local, bool is_cloud) - : local{is_local}, cloud{is_cloud} {} - - // Converts a comma-separated string of visibility identifiers into the - // Visibility bitset (|str| is a string like "local,cloud"). - // Special string value "all" is treated as a list of every possible - // visibility values and "none" to have all the bits cleared. - bool FromString(const std::string& str, ErrorPtr* error); - - // Converts the visibility bitset to a string. - std::string ToString() const; - - static Visibility GetAll() { return Visibility{true, true}; } - static Visibility GetLocal() { return Visibility{true, false}; } - static Visibility GetCloud() { return Visibility{false, true}; } - static Visibility GetNone() { return Visibility{false, false}; } - - bool local{false}; // Command is available to local clients. - bool cloud{false}; // Command is available to cloud clients. - }; - - CommandDefinition(std::unique_ptr<const ObjectSchema> parameters, - std::unique_ptr<const ObjectSchema> progress, - std::unique_ptr<const ObjectSchema> results); - - // Gets the object schema for command parameters. - const ObjectSchema* GetParameters() const { return parameters_.get(); } - // Gets the object schema for command progress. - const ObjectSchema* GetProgress() const { return progress_.get(); } - // Gets the object schema for command results. - const ObjectSchema* GetResults() const { return results_.get(); } - // Returns the command visibility. - const Visibility& GetVisibility() const { return visibility_; } - // Changes the command visibility. - void SetVisibility(const Visibility& visibility); + // Factory method to construct a command definition from a JSON dictionary. + static std::unique_ptr<CommandDefinition> FromJson( + const base::DictionaryValue& dict, ErrorPtr* error); + const base::DictionaryValue& ToJson() const { return definition_; } // Returns the role required to execute command. UserRole GetMinimalRole() const { return minimal_role_; } - // Changes the role required to execute command. - void SetMinimalRole(UserRole minimal_role) { minimal_role_ = minimal_role; } private: - std::unique_ptr<const ObjectSchema> parameters_; // Command parameters def. - std::unique_ptr<const ObjectSchema> progress_; // Command progress def. - std::unique_ptr<const ObjectSchema> results_; // Command results def. - Visibility visibility_; // Available to all by default. - // Minimal role required to execute command. - UserRole minimal_role_{UserRole::kUser}; + CommandDefinition(const base::DictionaryValue& definition, + UserRole minimal_role); + + base::DictionaryValue definition_; + UserRole minimal_role_; DISALLOW_COPY_AND_ASSIGN(CommandDefinition); };
diff --git a/src/commands/command_definition_unittest.cc b/src/commands/command_definition_unittest.cc index 77b2754..ecd6e54 100644 --- a/src/commands/command_definition_unittest.cc +++ b/src/commands/command_definition_unittest.cc
@@ -6,87 +6,47 @@ #include <gtest/gtest.h> +#include "src/commands/unittest_utils.h" + namespace weave { -TEST(CommandVisibility, DefaultConstructor) { - CommandDefinition::Visibility visibility; - EXPECT_FALSE(visibility.local); - EXPECT_FALSE(visibility.cloud); +using test::CreateDictionaryValue; + +TEST(CommandDefinition, DefaultRole) { + auto params = CreateDictionaryValue(R"({ + 'parameters': { + 'height': 'integer', + 'jumpType': ['_withAirFlip', '_withSpin', '_withKick'] + }, + 'progress': {'progress': 'integer'}, + 'results': {'testResult': 'integer'} + })"); + auto def = CommandDefinition::FromJson(*params, nullptr); + EXPECT_EQ(UserRole::kUser, def->GetMinimalRole()); } -TEST(CommandVisibility, InitialState) { - auto visibility = CommandDefinition::Visibility::GetAll(); - EXPECT_TRUE(visibility.local); - EXPECT_TRUE(visibility.cloud); - - visibility = CommandDefinition::Visibility::GetLocal(); - EXPECT_TRUE(visibility.local); - EXPECT_FALSE(visibility.cloud); - - visibility = CommandDefinition::Visibility::GetCloud(); - EXPECT_FALSE(visibility.local); - EXPECT_TRUE(visibility.cloud); - - visibility = CommandDefinition::Visibility::GetNone(); - EXPECT_FALSE(visibility.local); - EXPECT_FALSE(visibility.cloud); +TEST(CommandDefinition, SpecifiedRole) { + auto params = CreateDictionaryValue(R"({ + 'parameters': {}, + 'progress': {}, + 'results': {}, + 'minimalRole': 'owner' + })"); + auto def = CommandDefinition::FromJson(*params, nullptr); + EXPECT_EQ(UserRole::kOwner, def->GetMinimalRole()); } -TEST(CommandVisibility, FromString) { - CommandDefinition::Visibility visibility; - - ASSERT_TRUE(visibility.FromString("local", nullptr)); - EXPECT_TRUE(visibility.local); - EXPECT_FALSE(visibility.cloud); - - ASSERT_TRUE(visibility.FromString("cloud", nullptr)); - EXPECT_FALSE(visibility.local); - EXPECT_TRUE(visibility.cloud); - - ASSERT_TRUE(visibility.FromString("cloud,local", nullptr)); - EXPECT_TRUE(visibility.local); - EXPECT_TRUE(visibility.cloud); - - ASSERT_TRUE(visibility.FromString("none", nullptr)); - EXPECT_FALSE(visibility.local); - EXPECT_FALSE(visibility.cloud); - - ASSERT_TRUE(visibility.FromString("all", nullptr)); - EXPECT_TRUE(visibility.local); - EXPECT_TRUE(visibility.cloud); - - ASSERT_TRUE(visibility.FromString("", nullptr)); - EXPECT_FALSE(visibility.local); - EXPECT_FALSE(visibility.cloud); - +TEST(CommandDefinition, IncorrectRole) { + auto params = CreateDictionaryValue(R"({ + 'parameters': {}, + 'progress': {}, + 'results': {}, + 'minimalRole': 'foo' + })"); ErrorPtr error; - ASSERT_FALSE(visibility.FromString("cloud,all", &error)); + auto def = CommandDefinition::FromJson(*params, &error); + EXPECT_EQ(nullptr, def.get()); EXPECT_EQ("invalid_parameter_value", error->GetCode()); } -TEST(CommandVisibility, ToString) { - EXPECT_EQ("none", CommandDefinition::Visibility::GetNone().ToString()); - EXPECT_EQ("local", CommandDefinition::Visibility::GetLocal().ToString()); - EXPECT_EQ("cloud", CommandDefinition::Visibility::GetCloud().ToString()); - EXPECT_EQ("all", CommandDefinition::Visibility::GetAll().ToString()); -} - -TEST(CommandDefinition, Test) { - std::unique_ptr<const ObjectSchema> params{ObjectSchema::Create()}; - std::unique_ptr<const ObjectSchema> progress{ObjectSchema::Create()}; - std::unique_ptr<const ObjectSchema> results{ObjectSchema::Create()}; - const ObjectSchema* param_ptr = params.get(); - const ObjectSchema* progress_ptr = progress.get(); - const ObjectSchema* results_ptr = results.get(); - CommandDefinition def{std::move(params), std::move(progress), - std::move(results)}; - EXPECT_EQ(param_ptr, def.GetParameters()); - EXPECT_EQ(progress_ptr, def.GetProgress()); - EXPECT_EQ(results_ptr, def.GetResults()); - EXPECT_EQ("all", def.GetVisibility().ToString()); - - def.SetVisibility(CommandDefinition::Visibility::GetLocal()); - EXPECT_EQ("local", def.GetVisibility().ToString()); -} - } // namespace weave
diff --git a/src/commands/command_dictionary.cc b/src/commands/command_dictionary.cc index 053e7aa..8fb7f43 100644 --- a/src/commands/command_dictionary.cc +++ b/src/commands/command_dictionary.cc
@@ -14,7 +14,6 @@ namespace weave { bool CommandDictionary::LoadCommands(const base::DictionaryValue& json, - const CommandDictionary* base_commands, ErrorPtr* error) { CommandMap new_defs; @@ -54,89 +53,16 @@ // Construct the compound command name as "pkg_name.cmd_name". std::string full_command_name = Join(".", package_name, command_name); - const ObjectSchema* base_parameters_def = nullptr; - const ObjectSchema* base_progress_def = nullptr; - const ObjectSchema* base_results_def = nullptr; - // By default make it available to all clients. - auto visibility = CommandDefinition::Visibility::GetAll(); - UserRole minimal_role{UserRole::kUser}; - if (base_commands) { - auto cmd = base_commands->FindCommand(full_command_name); - if (cmd) { - base_parameters_def = cmd->GetParameters(); - base_progress_def = cmd->GetProgress(); - base_results_def = cmd->GetResults(); - visibility = cmd->GetVisibility(); - minimal_role = cmd->GetMinimalRole(); - } - - // If the base command dictionary was provided but the command was not - // found in it, this must be a custom (vendor) command. GCD spec states - // that all custom command names must begin with "_". Let's enforce - // this rule here. - if (!cmd) { - if (command_name.front() != '_') { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidCommandName, - "The name of custom command '%s' in package '%s'" - " must start with '_'", - command_name.c_str(), package_name.c_str()); - return false; - } - } + auto command_def = CommandDefinition::FromJson(*command_def_json, error); + if (!command_def) { + Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, + errors::commands::kInvalidMinimalRole, + "Error parsing command '%s'", + full_command_name.c_str()); + return false; } - auto parameters_schema = BuildObjectSchema( - command_def_json, commands::attributes::kCommand_Parameters, - base_parameters_def, full_command_name, error); - if (!parameters_schema) - return false; - - auto progress_schema = BuildObjectSchema( - command_def_json, commands::attributes::kCommand_Progress, - base_progress_def, full_command_name, error); - if (!progress_schema) - return false; - - auto results_schema = BuildObjectSchema( - command_def_json, commands::attributes::kCommand_Results, - base_results_def, full_command_name, error); - if (!results_schema) - return false; - - std::string value; - if (command_def_json->GetString(commands::attributes::kCommand_Visibility, - &value)) { - if (!visibility.FromString(value, error)) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidCommandVisibility, - "Error parsing command '%s'", - full_command_name.c_str()); - return false; - } - } - - if (command_def_json->GetString(commands::attributes::kCommand_Role, - &value)) { - if (!StringToEnum(value, &minimal_role)) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidPropValue, - "Invalid role: '%s'", value.c_str()); - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidMinimalRole, - "Error parsing command '%s'", - full_command_name.c_str()); - return false; - } - } - - std::unique_ptr<CommandDefinition> command_def{new CommandDefinition{ - std::move(parameters_schema), std::move(progress_schema), - std::move(results_schema)}}; - command_def->SetVisibility(visibility); - command_def->SetMinimalRole(minimal_role); new_defs.emplace(full_command_name, std::move(command_def)); - command_iter.Advance(); } package_iter.Advance(); @@ -159,49 +85,10 @@ return true; } -std::unique_ptr<ObjectSchema> CommandDictionary::BuildObjectSchema( - const base::DictionaryValue* command_def_json, - const char* property_name, - const ObjectSchema* base_def, - const std::string& command_name, - ErrorPtr* error) { - auto object_schema = ObjectSchema::Create(); - - const base::DictionaryValue* schema_def = nullptr; - if (!command_def_json->GetDictionaryWithoutPathExpansion(property_name, - &schema_def)) { - if (base_def) - return base_def->Clone(); - return object_schema; - } - - if (!object_schema->FromJson(schema_def, base_def, error)) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidObjectSchema, - "Invalid definition for command '%s'", - command_name.c_str()); - return {}; - } - - return object_schema; -} - std::unique_ptr<base::DictionaryValue> CommandDictionary::GetCommandsAsJson( - const std::function<bool(const CommandDefinition*)>& filter, - bool full_schema, ErrorPtr* error) const { std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue); for (const auto& pair : definitions_) { - // Check if the command definition has the desired visibility. - // If not, then skip it. - if (!filter(pair.second.get())) - continue; - - std::unique_ptr<base::DictionaryValue> parameters = - pair.second->GetParameters()->ToJson(full_schema, true); - CHECK(parameters); - // Progress and results are not part of public commandDefs. - auto parts = SplitAtFirst(pair.first, ".", true); const std::string& package_name = parts.first; const std::string& command_name = parts.second; @@ -213,12 +100,8 @@ package = new base::DictionaryValue; dict->SetWithoutPathExpansion(package_name, package); } - base::DictionaryValue* command_def = new base::DictionaryValue; - command_def->Set(commands::attributes::kCommand_Parameters, - parameters.release()); - command_def->SetString(commands::attributes::kCommand_Role, - EnumToString(pair.second->GetMinimalRole())); - package->SetWithoutPathExpansion(command_name, command_def); + package->SetWithoutPathExpansion(command_name, + pair.second->ToJson().DeepCopy()); } return dict; }
diff --git a/src/commands/command_dictionary.h b/src/commands/command_dictionary.h index 8d3d45c..03e080a 100644 --- a/src/commands/command_dictionary.h +++ b/src/commands/command_dictionary.h
@@ -23,8 +23,6 @@ namespace weave { -class ObjectSchema; - // CommandDictionary is a wrapper around a map of command name and the // corresponding command definition schema. The command name (the key in // the map) is a compound name in a form of "package_name.command_name", @@ -37,25 +35,15 @@ // Loads command definitions from a JSON object. This is done at the daemon // startup and whenever a device daemon decides to update its command list. // |json| is a JSON dictionary that describes the complete commands. Optional - // |base_commands| parameter specifies the definition of standard GCD commands - // for parameter schema validation. Can be set to nullptr if no validation is - // needed. Returns false on failure and |error| provides additional error - // information when provided. + // Returns false on failure and |error| provides additional error information + // when provided. bool LoadCommands(const base::DictionaryValue& json, - const CommandDictionary* base_commands, ErrorPtr* error); // Converts all the command definitions to a JSON object for CDD/Device // draft. - // |filter| is a predicate used to filter out the command definitions to - // be returned by this method. Only command definitions for which the - // predicate returns true will be included in the resulting JSON. - // |full_schema| specifies whether full command definitions must be generated - // (true) for CDD or only overrides from the base schema (false). // Returns empty unique_ptr in case of an error and fills in the additional // error details in |error|. std::unique_ptr<base::DictionaryValue> GetCommandsAsJson( - const std::function<bool(const CommandDefinition*)>& filter, - bool full_schema, ErrorPtr* error) const; // Returns the number of command definitions in the dictionary. size_t GetSize() const { return definitions_.size(); } @@ -70,13 +58,6 @@ private: using CommandMap = std::map<std::string, std::unique_ptr<CommandDefinition>>; - std::unique_ptr<ObjectSchema> BuildObjectSchema( - const base::DictionaryValue* command_def_json, - const char* property_name, - const ObjectSchema* base_def, - const std::string& command_name, - ErrorPtr* error); - CommandMap definitions_; // List of all available command definitions. DISALLOW_COPY_AND_ASSIGN(CommandDictionary); };
diff --git a/src/commands/command_dictionary_unittest.cc b/src/commands/command_dictionary_unittest.cc index 819718f..5cecd76 100644 --- a/src/commands/command_dictionary_unittest.cc +++ b/src/commands/command_dictionary_unittest.cc
@@ -35,7 +35,7 @@ } })"); CommandDictionary dict; - EXPECT_TRUE(dict.LoadCommands(*json, nullptr, nullptr)); + EXPECT_TRUE(dict.LoadCommands(*json, nullptr)); EXPECT_EQ(1, dict.GetSize()); EXPECT_NE(nullptr, dict.FindCommand("robot.jump")); json = CreateDictionaryValue(R"({ @@ -47,7 +47,7 @@ } } })"); - EXPECT_TRUE(dict.LoadCommands(*json, nullptr, nullptr)); + EXPECT_TRUE(dict.LoadCommands(*json, nullptr)); EXPECT_EQ(3, dict.GetSize()); EXPECT_NE(nullptr, dict.FindCommand("robot.jump")); EXPECT_NE(nullptr, dict.FindCommand("base.reboot")); @@ -55,177 +55,41 @@ EXPECT_EQ(nullptr, dict.FindCommand("foo.bar")); } -TEST(CommandDictionary, LoadWithInheritance) { - auto json = CreateDictionaryValue(R"({ - 'robot': { - 'jump': { - 'minimalRole': 'viewer', - 'visibility':'local', - 'parameters': { - 'height': 'integer' - }, - 'progress': { - 'progress': 'integer' - }, - 'results': { - 'success': 'boolean' - } - } - } - })"); - CommandDictionary base_dict; - EXPECT_TRUE(base_dict.LoadCommands(*json, nullptr, nullptr)); - EXPECT_EQ(1, base_dict.GetSize()); - json = CreateDictionaryValue(R"({'robot': {'jump': {}}})"); - - CommandDictionary dict; - EXPECT_TRUE(dict.LoadCommands(*json, &base_dict, nullptr)); - EXPECT_EQ(1, dict.GetSize()); - - auto cmd = dict.FindCommand("robot.jump"); - EXPECT_NE(nullptr, cmd); - - EXPECT_EQ("local", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kViewer, cmd->GetMinimalRole()); - - EXPECT_JSON_EQ("{'height': {'type': 'integer'}}", - *cmd->GetParameters()->ToJson(true, true)); - EXPECT_JSON_EQ("{'progress': {'type': 'integer'}}", - *cmd->GetProgress()->ToJson(true, false)); - EXPECT_JSON_EQ("{'success': {'type': 'boolean'}}", - *cmd->GetResults()->ToJson(true, false)); -} - TEST(CommandDictionary, LoadCommands_Failures) { CommandDictionary dict; ErrorPtr error; // Command definition is not an object. auto json = CreateDictionaryValue("{'robot':{'jump':0}}"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); + EXPECT_FALSE(dict.LoadCommands(*json, &error)); EXPECT_EQ("type_mismatch", error->GetCode()); error.reset(); // Package definition is not an object. json = CreateDictionaryValue("{'robot':'blah'}"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); + EXPECT_FALSE(dict.LoadCommands(*json, &error)); EXPECT_EQ("type_mismatch", error->GetCode()); error.reset(); - // Invalid command definition is not an object. - json = CreateDictionaryValue( - "{'robot':{'jump':{'parameters':{'flip':0},'results':{}}}}"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); - EXPECT_EQ("invalid_object_schema", error->GetCode()); - EXPECT_NE(nullptr, error->GetInnerError()); // Must have additional info. - error.reset(); - // Empty command name. json = CreateDictionaryValue("{'robot':{'':{'parameters':{},'results':{}}}}"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); + EXPECT_FALSE(dict.LoadCommands(*json, &error)); EXPECT_EQ("invalid_command_name", error->GetCode()); error.reset(); } -TEST(CommandDictionaryDeathTest, LoadCommands_RedefineInDifferentCategory) { - // Redefine commands in different category. +TEST(CommandDictionaryDeathTest, LoadCommands_Redefine) { + // Redefine commands. CommandDictionary dict; ErrorPtr error; auto json = CreateDictionaryValue("{'robot':{'jump':{}}}"); - dict.LoadCommands(*json, nullptr, &error); - ASSERT_DEATH(dict.LoadCommands(*json, nullptr, &error), + dict.LoadCommands(*json, nullptr); + ASSERT_DEATH(dict.LoadCommands(*json, &error), ".*Definition for command 'robot.jump' overrides an " "earlier definition"); } -TEST(CommandDictionary, LoadCommands_CustomCommandNaming) { - // Custom command must start with '_'. - CommandDictionary base_dict; - CommandDictionary dict; - ErrorPtr error; - auto json = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': 'integer'}, - 'results': {} - } - } - })"); - base_dict.LoadCommands(*json, nullptr, &error); - EXPECT_TRUE(dict.LoadCommands(*json, &base_dict, &error)); - auto json2 = - CreateDictionaryValue("{'base':{'jump':{'parameters':{},'results':{}}}}"); - EXPECT_FALSE(dict.LoadCommands(*json2, &base_dict, &error)); - EXPECT_EQ("invalid_command_name", error->GetCode()); - error.reset(); - - // If the command starts with "_", then it's Ok. - json2 = CreateDictionaryValue( - "{'base':{'_jump':{'parameters':{},'results':{}}}}"); - EXPECT_TRUE(dict.LoadCommands(*json2, &base_dict, nullptr)); -} - -TEST(CommandDictionary, LoadCommands_RedefineStdCommand) { - // Redefine commands parameter type. - CommandDictionary base_dict; - CommandDictionary dict; - ErrorPtr error; - auto json = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': 'integer'}, - 'results': {'version': 'integer'} - } - } - })"); - base_dict.LoadCommands(*json, nullptr, &error); - - auto json2 = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': 'string'}, - 'results': {'version': 'integer'} - } - } - })"); - EXPECT_FALSE(dict.LoadCommands(*json2, &base_dict, &error)); - EXPECT_EQ("invalid_object_schema", error->GetCode()); - EXPECT_EQ("invalid_parameter_definition", error->GetInnerError()->GetCode()); - EXPECT_EQ("param_type_changed", error->GetFirstError()->GetCode()); - error.reset(); - - auto json3 = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': 'integer'}, - 'results': {'version': 'string'} - } - } - })"); - EXPECT_FALSE(dict.LoadCommands(*json3, &base_dict, &error)); - EXPECT_EQ("invalid_object_schema", error->GetCode()); - // TODO(antonm): remove parameter from error below and use some generic. - EXPECT_EQ("invalid_parameter_definition", error->GetInnerError()->GetCode()); - EXPECT_EQ("param_type_changed", error->GetFirstError()->GetCode()); - error.reset(); -} - TEST(CommandDictionary, GetCommandsAsJson) { - auto json_base = CreateDictionaryValue(R"({ - 'base': { - 'reboot': { - 'parameters': {'delay': {'maximum': 100}}, - 'results': {} - }, - 'shutdown': { - 'parameters': {}, - 'results': {} - } - } - })"); - CommandDictionary base_dict; - base_dict.LoadCommands(*json_base, nullptr, nullptr); - auto json = CreateDictionaryValue(R"({ 'base': { 'reboot': { @@ -236,21 +100,20 @@ 'robot': { '_jump': { 'parameters': {'_height': 'integer'}, - 'results': {} + 'minimalRole': 'user' } } })"); CommandDictionary dict; - dict.LoadCommands(*json, &base_dict, nullptr); + dict.LoadCommands(*json, nullptr); - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { return true; }, false, nullptr); + json = dict.GetCommandsAsJson(nullptr); ASSERT_NE(nullptr, json.get()); auto expected = R"({ 'base': { 'reboot': { 'parameters': {'delay': {'minimum': 10}}, - 'minimalRole': 'user' + 'results': {} } }, 'robot': { @@ -261,143 +124,6 @@ } })"; EXPECT_JSON_EQ(expected, *json); - - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { return true; }, true, nullptr); - ASSERT_NE(nullptr, json.get()); - expected = R"({ - 'base': { - 'reboot': { - 'parameters': { - 'delay': { - 'maximum': 100, - 'minimum': 10, - 'type': 'integer' - } - }, - 'minimalRole': 'user' - } - }, - 'robot': { - '_jump': { - 'parameters': { - '_height': { - 'type': 'integer' - } - }, - 'minimalRole': 'user' - } - } - })"; - EXPECT_JSON_EQ(expected, *json); -} - -TEST(CommandDictionary, GetCommandsAsJsonWithVisibility) { - auto json = CreateDictionaryValue(R"({ - 'test': { - 'command1': { - 'parameters': {}, - 'results': {}, - 'visibility': 'none' - }, - 'command2': { - 'parameters': {}, - 'results': {}, - 'visibility': 'local' - }, - 'command3': { - 'parameters': {}, - 'results': {}, - 'visibility': 'cloud' - }, - 'command4': { - 'parameters': {}, - 'results': {}, - 'visibility': 'all' - }, - 'command5': { - 'parameters': {}, - 'results': {}, - 'visibility': 'none' - }, - 'command6': { - 'parameters': {}, - 'results': {}, - 'visibility': 'local' - }, - 'command7': { - 'parameters': {}, - 'results': {}, - 'visibility': 'cloud' - }, - 'command8': { - 'parameters': {}, - 'results': {}, - 'visibility': 'all' - } - } - })"); - CommandDictionary dict; - ASSERT_TRUE(dict.LoadCommands(*json, nullptr, nullptr)); - - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { return true; }, false, nullptr); - ASSERT_NE(nullptr, json.get()); - auto expected = R"({ - 'test': { - 'command1': {'parameters': {}, 'minimalRole': 'user'}, - 'command2': {'parameters': {}, 'minimalRole': 'user'}, - 'command3': {'parameters': {}, 'minimalRole': 'user'}, - 'command4': {'parameters': {}, 'minimalRole': 'user'}, - 'command5': {'parameters': {}, 'minimalRole': 'user'}, - 'command6': {'parameters': {}, 'minimalRole': 'user'}, - 'command7': {'parameters': {}, 'minimalRole': 'user'}, - 'command8': {'parameters': {}, 'minimalRole': 'user'} - } - })"; - EXPECT_JSON_EQ(expected, *json); - - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { return def->GetVisibility().local; }, - false, nullptr); - ASSERT_NE(nullptr, json.get()); - expected = R"({ - 'test': { - 'command2': {'parameters': {}, 'minimalRole': 'user'}, - 'command4': {'parameters': {}, 'minimalRole': 'user'}, - 'command6': {'parameters': {}, 'minimalRole': 'user'}, - 'command8': {'parameters': {}, 'minimalRole': 'user'} - } - })"; - EXPECT_JSON_EQ(expected, *json); - - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { return def->GetVisibility().cloud; }, - false, nullptr); - ASSERT_NE(nullptr, json.get()); - expected = R"({ - 'test': { - 'command3': {'parameters': {}, 'minimalRole': 'user'}, - 'command4': {'parameters': {}, 'minimalRole': 'user'}, - 'command7': {'parameters': {}, 'minimalRole': 'user'}, - 'command8': {'parameters': {}, 'minimalRole': 'user'} - } - })"; - EXPECT_JSON_EQ(expected, *json); - - json = dict.GetCommandsAsJson( - [](const CommandDefinition* def) { - return def->GetVisibility().local && def->GetVisibility().cloud; - }, - false, nullptr); - ASSERT_NE(nullptr, json.get()); - expected = R"({ - 'test': { - 'command4': {'parameters': {}, 'minimalRole': 'user'}, - 'command8': {'parameters': {}, 'minimalRole': 'user'} - } - })"; - EXPECT_JSON_EQ(expected, *json); } TEST(CommandDictionary, LoadWithPermissions) { @@ -406,161 +132,51 @@ 'base': { 'command1': { 'parameters': {}, - 'results': {}, - 'visibility':'none' + 'results': {} }, 'command2': { 'minimalRole': 'viewer', 'parameters': {}, - 'results': {}, - 'visibility':'local' + 'results': {} }, 'command3': { 'minimalRole': 'user', 'parameters': {}, - 'results': {}, - 'visibility':'cloud' + 'results': {} }, 'command4': { 'minimalRole': 'manager', 'parameters': {}, - 'results': {}, - 'visibility':'all' + 'results': {} }, 'command5': { 'minimalRole': 'owner', 'parameters': {}, - 'results': {}, - 'visibility':'local,cloud' + 'results': {} } } })"); - EXPECT_TRUE(base_dict.LoadCommands(*json, nullptr, nullptr)); + EXPECT_TRUE(base_dict.LoadCommands(*json, nullptr)); auto cmd = base_dict.FindCommand("base.command1"); ASSERT_NE(nullptr, cmd); - EXPECT_EQ("none", cmd->GetVisibility().ToString()); EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); cmd = base_dict.FindCommand("base.command2"); ASSERT_NE(nullptr, cmd); - EXPECT_EQ("local", cmd->GetVisibility().ToString()); EXPECT_EQ(UserRole::kViewer, cmd->GetMinimalRole()); cmd = base_dict.FindCommand("base.command3"); ASSERT_NE(nullptr, cmd); - EXPECT_EQ("cloud", cmd->GetVisibility().ToString()); EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); cmd = base_dict.FindCommand("base.command4"); ASSERT_NE(nullptr, cmd); - EXPECT_EQ("all", cmd->GetVisibility().ToString()); EXPECT_EQ(UserRole::kManager, cmd->GetMinimalRole()); cmd = base_dict.FindCommand("base.command5"); ASSERT_NE(nullptr, cmd); - EXPECT_EQ("all", cmd->GetVisibility().ToString()); EXPECT_EQ(UserRole::kOwner, cmd->GetMinimalRole()); - - CommandDictionary dict; - json = CreateDictionaryValue(R"({ - 'base': { - 'command1': { - 'parameters': {}, - 'results': {} - }, - 'command2': { - 'parameters': {}, - 'results': {} - }, - 'command3': { - 'parameters': {}, - 'results': {} - }, - 'command4': { - 'parameters': {}, - 'results': {} - }, - 'command5': { - 'parameters': {}, - 'results': {} - }, - '_command6': { - 'parameters': {}, - 'results': {} - } - } - })"); - EXPECT_TRUE(dict.LoadCommands(*json, &base_dict, nullptr)); - - cmd = dict.FindCommand("base.command1"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("none", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); - - cmd = dict.FindCommand("base.command2"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("local", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kViewer, cmd->GetMinimalRole()); - - cmd = dict.FindCommand("base.command3"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("cloud", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); - - cmd = dict.FindCommand("base.command4"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("all", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kManager, cmd->GetMinimalRole()); - - cmd = dict.FindCommand("base.command5"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("all", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kOwner, cmd->GetMinimalRole()); - - cmd = dict.FindCommand("base._command6"); - ASSERT_NE(nullptr, cmd); - EXPECT_EQ("all", cmd->GetVisibility().ToString()); - EXPECT_EQ(UserRole::kUser, cmd->GetMinimalRole()); -} - -TEST(CommandDictionary, LoadWithPermissions_InvalidVisibility) { - CommandDictionary dict; - ErrorPtr error; - - auto json = CreateDictionaryValue(R"({ - 'base': { - 'jump': { - 'parameters': {}, - 'results': {}, - 'visibility':'foo' - } - } - })"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); - EXPECT_EQ("invalid_command_visibility", error->GetCode()); - EXPECT_EQ("invalid_parameter_value", error->GetInnerError()->GetCode()); - error.reset(); -} - -TEST(CommandDictionary, LoadWithPermissions_InvalidRole) { - CommandDictionary dict; - ErrorPtr error; - - auto json = CreateDictionaryValue(R"({ - 'base': { - 'jump': { - 'parameters': {}, - 'results': {}, - 'visibility':'local,cloud', - 'minimalRole':'foo' - } - } - })"); - EXPECT_FALSE(dict.LoadCommands(*json, nullptr, &error)); - EXPECT_EQ("invalid_minimal_role", error->GetCode()); - EXPECT_EQ("invalid_parameter_value", error->GetInnerError()->GetCode()); - error.reset(); } } // namespace weave
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..cacb86c 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 { @@ -61,7 +59,7 @@ } } })"); - CHECK(dict_.LoadCommands(*json, nullptr, nullptr)) + CHECK(dict_.LoadCommands(*json, nullptr)) << "Failed to parse test command dictionary"; } CommandDictionary dict_; @@ -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',
diff --git a/src/commands/command_manager.cc b/src/commands/command_manager.cc index 75d8295..a64c8e5 100644 --- a/src/commands/command_manager.cc +++ b/src/commands/command_manager.cc
@@ -28,7 +28,7 @@ bool CommandManager::LoadCommands(const base::DictionaryValue& dict, ErrorPtr* error) { - bool result = dictionary_.LoadCommands(dict, nullptr, error); + bool result = dictionary_.LoadCommands(dict, error); for (const auto& cb : on_command_changed_) cb.Run(); return result; @@ -83,37 +83,6 @@ return command_queue_.Find(id); } -bool CommandManager::SetCommandVisibility( - const std::vector<std::string>& command_names, - CommandDefinition::Visibility visibility, - ErrorPtr* error) { - if (command_names.empty()) - return true; - - std::vector<CommandDefinition*> definitions; - definitions.reserve(command_names.size()); - - // Find/validate command definitions first. - for (const std::string& name : command_names) { - CommandDefinition* def = dictionary_.FindCommand(name); - if (!def) { - Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain, - errors::commands::kInvalidCommandName, - "Command '%s' is unknown", name.c_str()); - return false; - } - definitions.push_back(def); - } - - // Now that we know that all the command names were valid, - // update the respective commands' visibility. - for (CommandDefinition* def : definitions) - def->SetVisibility(visibility); - for (const auto& cb : on_command_changed_) - cb.Run(); - return true; -} - void CommandManager::AddCommandAddedCallback( const CommandQueue::CommandCallback& callback) { command_queue_.AddCommandAddedCallback(callback);
diff --git a/src/commands/command_manager.h b/src/commands/command_manager.h index 7cc8907..04f49ee 100644 --- a/src/commands/command_manager.h +++ b/src/commands/command_manager.h
@@ -60,11 +60,6 @@ // Adds a new command to the command queue. void AddCommand(std::unique_ptr<CommandInstance> command_instance); - // Changes the visibility of commands. - bool SetCommandVisibility(const std::vector<std::string>& command_names, - CommandDefinition::Visibility visibility, - ErrorPtr* error); - bool AddCommand(const base::DictionaryValue& command, UserRole role, std::string* id,
diff --git a/src/commands/command_manager_unittest.cc b/src/commands/command_manager_unittest.cc index 0c890a9..eca5175 100644 --- a/src/commands/command_manager_unittest.cc +++ b/src/commands/command_manager_unittest.cc
@@ -91,59 +91,4 @@ EXPECT_NE(nullptr, manager.GetCommandDictionary().FindCommand("test._yo")); } -TEST(CommandManager, UpdateCommandVisibility) { - CommandManager manager; - int update_count = 0; - auto on_command_change = [&update_count]() { update_count++; }; - manager.AddCommandDefChanged(base::Bind(on_command_change)); - - auto json = CreateDictionaryValue(R"({ - 'foo': { - '_baz': { - 'parameters': {}, - 'results': {} - }, - '_bar': { - 'parameters': {}, - 'results': {} - } - }, - 'bar': { - '_quux': { - 'parameters': {}, - 'results': {}, - 'visibility': 'none' - } - } - })"); - ASSERT_TRUE(manager.LoadCommands(*json, nullptr)); - EXPECT_EQ(2, update_count); - const CommandDictionary& dict = manager.GetCommandDictionary(); - EXPECT_TRUE(manager.SetCommandVisibility( - {"foo._baz"}, CommandDefinition::Visibility::GetLocal(), nullptr)); - EXPECT_EQ(3, update_count); - EXPECT_EQ("local", dict.FindCommand("foo._baz")->GetVisibility().ToString()); - EXPECT_EQ("all", dict.FindCommand("foo._bar")->GetVisibility().ToString()); - EXPECT_EQ("none", dict.FindCommand("bar._quux")->GetVisibility().ToString()); - - ErrorPtr error; - ASSERT_FALSE(manager.SetCommandVisibility( - {"foo._baz", "foo._bar", "test.cmd"}, - CommandDefinition::Visibility::GetLocal(), &error)); - EXPECT_EQ(errors::commands::kInvalidCommandName, error->GetCode()); - // The visibility state of commands shouldn't have changed. - EXPECT_EQ(3, update_count); - EXPECT_EQ("local", dict.FindCommand("foo._baz")->GetVisibility().ToString()); - EXPECT_EQ("all", dict.FindCommand("foo._bar")->GetVisibility().ToString()); - EXPECT_EQ("none", dict.FindCommand("bar._quux")->GetVisibility().ToString()); - - EXPECT_TRUE(manager.SetCommandVisibility( - {"foo._baz", "bar._quux"}, CommandDefinition::Visibility::GetCloud(), - nullptr)); - EXPECT_EQ(4, update_count); - EXPECT_EQ("cloud", dict.FindCommand("foo._baz")->GetVisibility().ToString()); - EXPECT_EQ("all", dict.FindCommand("foo._bar")->GetVisibility().ToString()); - EXPECT_EQ("cloud", dict.FindCommand("bar._quux")->GetVisibility().ToString()); -} - } // namespace weave
diff --git a/src/commands/command_queue_unittest.cc b/src/commands/command_queue_unittest.cc index 5060071..a9e953e 100644 --- a/src/commands/command_queue_unittest.cc +++ b/src/commands/command_queue_unittest.cc
@@ -20,11 +20,15 @@ class CommandQueueTest : public testing::Test { public: + CommandQueueTest() { + command_definition_ = CommandDefinition::FromJson({}, nullptr); + } + std::unique_ptr<CommandInstance> CreateDummyCommandInstance( const std::string& name, const std::string& id) { std::unique_ptr<CommandInstance> cmd{new CommandInstance{ - name, Command::Origin::kLocal, &command_definition_, {}}}; + name, Command::Origin::kLocal, command_definition_.get(), {}}}; cmd->SetID(id); return cmd; } @@ -39,8 +43,7 @@ CommandQueue queue_; private: - CommandDefinition command_definition_{ - ObjectSchema::Create(), ObjectSchema::Create(), ObjectSchema::Create()}; + std::unique_ptr<CommandDefinition> command_definition_; }; // Keeps track of commands being added to and removed from the queue_.
diff --git a/src/commands/schema_constants.cc b/src/commands/schema_constants.cc index c99536b..7f8431f 100644 --- a/src/commands/schema_constants.cc +++ b/src/commands/schema_constants.cc
@@ -23,7 +23,6 @@ const char kDuplicateCommandDef[] = "duplicate_command_definition"; const char kInvalidCommandName[] = "invalid_command_name"; const char kCommandFailed[] = "command_failed"; -const char kInvalidCommandVisibility[] = "invalid_command_visibility"; const char kInvalidMinimalRole[] = "invalid_minimal_role"; const char kCommandDestroyed[] = "command_destroyed"; const char kInvalidState[] = "invalid_state"; @@ -66,12 +65,6 @@ const char kCommand_Role_User[] = "user"; const char kCommand_Role_Viewer[] = "viewer"; -const char kCommand_Visibility[] = "visibility"; -const char kCommand_Visibility_None[] = "none"; -const char kCommand_Visibility_Local[] = "local"; -const char kCommand_Visibility_Cloud[] = "cloud"; -const char kCommand_Visibility_All[] = "all"; - } // namespace attributes } // namespace commands
diff --git a/src/commands/schema_constants.h b/src/commands/schema_constants.h index 742245f..ea59f17 100644 --- a/src/commands/schema_constants.h +++ b/src/commands/schema_constants.h
@@ -26,7 +26,6 @@ extern const char kDuplicateCommandDef[]; extern const char kInvalidCommandName[]; extern const char kCommandFailed[]; -extern const char kInvalidCommandVisibility[]; extern const char kInvalidMinimalRole[]; extern const char kCommandDestroyed[]; extern const char kInvalidState[]; @@ -69,12 +68,6 @@ extern const char kCommand_Role_User[]; extern const char kCommand_Role_Viewer[]; -extern const char kCommand_Visibility[]; -extern const char kCommand_Visibility_None[]; -extern const char kCommand_Visibility_Local[]; -extern const char kCommand_Visibility_Cloud[]; -extern const char kCommand_Visibility_All[]; - } // namespace attributes } // namespace commands
diff --git a/src/device_registration_info.cc b/src/device_registration_info.cc index 751e530..5c8625a 100644 --- a/src/device_registration_info.cc +++ b/src/device_registration_info.cc
@@ -480,9 +480,8 @@ std::unique_ptr<base::DictionaryValue> DeviceRegistrationInfo::BuildDeviceResource(ErrorPtr* error) { // Limit only to commands that are visible to the cloud. - auto commands = command_manager_->GetCommandDictionary().GetCommandsAsJson( - [](const CommandDefinition* def) { return def->GetVisibility().cloud; }, - true, error); + auto commands = + command_manager_->GetCommandDictionary().GetCommandsAsJson(error); if (!commands) return nullptr; @@ -1121,23 +1120,11 @@ return; std::unique_ptr<base::ListValue> patches{new base::ListValue}; - for (const auto& state_change : state_changes) { + for (auto& state_change : state_changes) { std::unique_ptr<base::DictionaryValue> patch{new base::DictionaryValue}; patch->SetString("timeMs", std::to_string(state_change.timestamp.ToJavaTime())); - - std::unique_ptr<base::DictionaryValue> changes{new base::DictionaryValue}; - for (const auto& pair : state_change.changed_properties) { - auto value = pair.second->ToJson(); - CHECK(value); - // The key in |pair.first| is the full property name in format - // "package.property_name", so must use DictionaryValue::Set() instead of - // DictionaryValue::SetWithoutPathExpansion to recreate the JSON - // property tree properly. - changes->Set(pair.first, value.release()); - } - patch->Set("patch", changes.release()); - + patch->Set("patch", state_change.changed_properties.release()); patches->Append(patch.release()); }
diff --git a/src/device_registration_info_unittest.cc b/src/device_registration_info_unittest.cc index df4a438..9b53872 100644 --- a/src/device_registration_info_unittest.cc +++ b/src/device_registration_info_unittest.cc
@@ -355,16 +355,14 @@ auto json_cmds = CreateDictionaryValue(R"({ 'base': { 'reboot': { - 'parameters': {'delay': {'minimum': 10}}, - 'minimalRole': 'user', - 'results': {} + 'parameters': {'delay': {'minimum': 10, 'type': 'integer'}}, + 'minimalRole': 'user' } }, 'robot': { '_jump': { - 'parameters': {'_height': 'integer'}, - 'minimalRole': 'user', - 'results': {} + 'parameters': {'_height': {'type': 'integer'}}, + 'minimalRole': 'user' } } })");
diff --git a/src/privet/cloud_delegate.cc b/src/privet/cloud_delegate.cc index d612668..32ebf48 100644 --- a/src/privet/cloud_delegate.cc +++ b/src/privet/cloud_delegate.cc
@@ -251,9 +251,8 @@ void OnCommandDefChanged() { command_defs_.Clear(); - auto commands = command_manager_->GetCommandDictionary().GetCommandsAsJson( - [](const CommandDefinition* def) { return def->GetVisibility().local; }, - true, nullptr); + auto commands = + command_manager_->GetCommandDictionary().GetCommandsAsJson(nullptr); CHECK(commands); command_defs_.MergeDictionary(commands.get()); NotifyOnCommandDefsChanged();
diff --git a/src/states/mock_state_change_queue_interface.h b/src/states/mock_state_change_queue_interface.h index c3f50c0..78c6c82 100644 --- a/src/states/mock_state_change_queue_interface.h +++ b/src/states/mock_state_change_queue_interface.h
@@ -16,9 +16,11 @@ class MockStateChangeQueueInterface : public StateChangeQueueInterface { public: MOCK_CONST_METHOD0(IsEmpty, bool()); - MOCK_METHOD2(NotifyPropertiesUpdated, - bool(base::Time timestamp, ValueMap changed_properties)); - MOCK_METHOD0(GetAndClearRecordedStateChanges, std::vector<StateChange>()); + MOCK_METHOD2(MockNotifyPropertiesUpdated, + bool(base::Time timestamp, + const base::DictionaryValue& changed_properties)); + MOCK_METHOD0(MockGetAndClearRecordedStateChanges, + std::vector<StateChange>&()); MOCK_CONST_METHOD0(GetLastStateChangeId, UpdateID()); MOCK_METHOD1(MockAddOnStateUpdatedCallback, base::CallbackList<void(UpdateID)>::Subscription*( @@ -26,6 +28,15 @@ MOCK_METHOD1(NotifyStateUpdatedOnServer, void(UpdateID)); private: + bool NotifyPropertiesUpdated( + base::Time timestamp, + std::unique_ptr<base::DictionaryValue> changed_properties) override { + return MockNotifyPropertiesUpdated(timestamp, *changed_properties); + } + std::vector<StateChange> GetAndClearRecordedStateChanges() override { + return std::move(MockGetAndClearRecordedStateChanges()); + } + Token AddOnStateUpdatedCallback( const base::Callback<void(UpdateID)>& callback) override { return Token{MockAddOnStateUpdatedCallback(callback)};
diff --git a/src/states/state_change_queue.cc b/src/states/state_change_queue.cc index 288dadc..87cc18d 100644 --- a/src/states/state_change_queue.cc +++ b/src/states/state_change_queue.cc
@@ -13,12 +13,15 @@ CHECK_GT(max_queue_size_, 0U) << "Max queue size must not be zero"; } -bool StateChangeQueue::NotifyPropertiesUpdated(base::Time timestamp, - ValueMap changed_properties) { +bool StateChangeQueue::NotifyPropertiesUpdated( + base::Time timestamp, + std::unique_ptr<base::DictionaryValue> changed_properties) { auto& stored_changes = state_changes_[timestamp]; // Merge the old property set. - changed_properties.insert(stored_changes.begin(), stored_changes.end()); - stored_changes = std::move(changed_properties); + if (stored_changes) + stored_changes->MergeDictionary(changed_properties.get()); + else + stored_changes = std::move(changed_properties); while (state_changes_.size() > max_queue_size_) { // Queue is full. @@ -30,8 +33,8 @@ auto element_old = state_changes_.begin(); auto element_new = std::next(element_old); // This will skip elements that exist in both [old] and [new]. - element_new->second.insert(element_old->second.begin(), - element_old->second.end()); + element_old->second->MergeDictionary(element_new->second.get()); + std::swap(element_old->second, element_new->second); state_changes_.erase(element_old); } ++last_change_id_; @@ -41,8 +44,8 @@ std::vector<StateChange> StateChangeQueue::GetAndClearRecordedStateChanges() { std::vector<StateChange> changes; changes.reserve(state_changes_.size()); - for (const auto& pair : state_changes_) { - changes.emplace_back(pair.first, std::move(pair.second)); + for (auto& pair : state_changes_) { + changes.push_back(StateChange{pair.first, std::move(pair.second)}); } state_changes_.clear(); return changes;
diff --git a/src/states/state_change_queue.h b/src/states/state_change_queue.h index 00b827f..314f746 100644 --- a/src/states/state_change_queue.h +++ b/src/states/state_change_queue.h
@@ -21,8 +21,9 @@ // Overrides from StateChangeQueueInterface. bool IsEmpty() const override { return state_changes_.empty(); } - bool NotifyPropertiesUpdated(base::Time timestamp, - ValueMap changed_properties) override; + bool NotifyPropertiesUpdated( + base::Time timestamp, + std::unique_ptr<base::DictionaryValue> changed_properties) override; std::vector<StateChange> GetAndClearRecordedStateChanges() override; UpdateID GetLastStateChangeId() const override { return last_change_id_; } Token AddOnStateUpdatedCallback( @@ -35,7 +36,7 @@ const size_t max_queue_size_; // Accumulated list of device state change notifications. - std::map<base::Time, ValueMap> state_changes_; + std::map<base::Time, std::unique_ptr<base::DictionaryValue>> state_changes_; // An ID of last state change update. Each NotifyPropertiesUpdated() // invocation increments this value by 1.
diff --git a/src/states/state_change_queue_interface.h b/src/states/state_change_queue_interface.h index e3b3650..81a2cfc 100644 --- a/src/states/state_change_queue_interface.h +++ b/src/states/state_change_queue_interface.h
@@ -19,10 +19,11 @@ // |changed_properties| contains a property set with the new property values // which were updated at the time the event was recorded. struct StateChange { - StateChange(base::Time time, ValueMap properties) + StateChange(base::Time time, + std::unique_ptr<base::DictionaryValue> properties) : timestamp{time}, changed_properties{std::move(properties)} {} base::Time timestamp; - ValueMap changed_properties; + std::unique_ptr<base::DictionaryValue> changed_properties; }; // An abstract interface to StateChangeQueue to record and retrieve state @@ -37,8 +38,9 @@ virtual bool IsEmpty() const = 0; // Called by StateManager when device state properties are updated. - virtual bool NotifyPropertiesUpdated(base::Time timestamp, - ValueMap changed_properties) = 0; + virtual bool NotifyPropertiesUpdated( + base::Time timestamp, + std::unique_ptr<base::DictionaryValue> changed_properties) = 0; // Returns the recorded state changes since last time this method was called. virtual std::vector<StateChange> GetAndClearRecordedStateChanges() = 0;
diff --git a/src/states/state_change_queue_unittest.cc b/src/states/state_change_queue_unittest.cc index 9f26071..5fbc012 100644 --- a/src/states/state_change_queue_unittest.cc +++ b/src/states/state_change_queue_unittest.cc
@@ -11,6 +11,8 @@ namespace weave { +using test::CreateDictionaryValue; + class StateChangeQueueTest : public ::testing::Test { public: void SetUp() override { queue_.reset(new StateChangeQueue(100)); } @@ -27,43 +29,39 @@ } TEST_F(StateChangeQueueTest, UpdateOne) { - StateChange change{base::Time::Now(), - ValueMap{{"prop.name", test::make_int_prop_value(23)}}}; - ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change.timestamp, - change.changed_properties)); + auto timestamp = base::Time::Now(); + ASSERT_TRUE(queue_->NotifyPropertiesUpdated( + timestamp, CreateDictionaryValue("{'prop': {'name': 23}}"))); EXPECT_FALSE(queue_->IsEmpty()); EXPECT_EQ(1, queue_->GetLastStateChangeId()); auto changes = queue_->GetAndClearRecordedStateChanges(); EXPECT_EQ(1, queue_->GetLastStateChangeId()); ASSERT_EQ(1, changes.size()); - EXPECT_EQ(change.timestamp, changes.front().timestamp); - EXPECT_EQ(change.changed_properties, changes.front().changed_properties); + EXPECT_EQ(timestamp, changes.front().timestamp); + EXPECT_JSON_EQ("{'prop':{'name': 23}}", *changes.front().changed_properties); EXPECT_TRUE(queue_->IsEmpty()); EXPECT_TRUE(queue_->GetAndClearRecordedStateChanges().empty()); } -// TODO(vitalybuka): Fix flakiness. -TEST_F(StateChangeQueueTest, DISABLED_UpdateMany) { - StateChange change1{base::Time::Now(), - ValueMap{{"prop.name1", test::make_int_prop_value(23)}}}; - ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change1.timestamp, - change1.changed_properties)); - StateChange change2{base::Time::Now(), - ValueMap{ - {"prop.name1", test::make_int_prop_value(17)}, - {"prop.name2", test::make_double_prop_value(1.0)}, - {"prop.name3", test::make_bool_prop_value(false)}, - }}; - ASSERT_TRUE(queue_->NotifyPropertiesUpdated(change2.timestamp, - change2.changed_properties)); +TEST_F(StateChangeQueueTest, UpdateMany) { + auto timestamp1 = base::Time::Now(); + const std::string state1 = "{'prop': {'name1': 23}}"; + auto timestamp2 = timestamp1 + base::TimeDelta::FromSeconds(1); + const std::string state2 = + "{'prop': {'name1': 17, 'name2': 1.0, 'name3': false}}"; + ASSERT_TRUE(queue_->NotifyPropertiesUpdated( + timestamp1, CreateDictionaryValue(state1))); + ASSERT_TRUE(queue_->NotifyPropertiesUpdated( + timestamp2, CreateDictionaryValue(state2))); + EXPECT_EQ(2, queue_->GetLastStateChangeId()); EXPECT_FALSE(queue_->IsEmpty()); auto changes = queue_->GetAndClearRecordedStateChanges(); ASSERT_EQ(2, changes.size()); - EXPECT_EQ(change1.timestamp, changes[0].timestamp); - EXPECT_EQ(change1.changed_properties, changes[0].changed_properties); - EXPECT_EQ(change2.timestamp, changes[1].timestamp); - EXPECT_EQ(change2.changed_properties, changes[1].changed_properties); + EXPECT_EQ(timestamp1, changes[0].timestamp); + EXPECT_JSON_EQ(state1, *changes[0].changed_properties); + EXPECT_EQ(timestamp2, changes[1].timestamp); + EXPECT_JSON_EQ(state2, *changes[1].changed_properties); EXPECT_TRUE(queue_->IsEmpty()); EXPECT_TRUE(queue_->GetAndClearRecordedStateChanges().empty()); } @@ -73,33 +71,27 @@ base::TimeDelta time_delta = base::TimeDelta::FromMinutes(1); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - timestamp, ValueMap{{"prop.name1", test::make_int_prop_value(1)}})); + timestamp, CreateDictionaryValue("{'prop': {'name1': 1}}"))); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - timestamp, ValueMap{{"prop.name2", test::make_int_prop_value(2)}})); + timestamp, CreateDictionaryValue("{'prop': {'name2': 2}}"))); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - timestamp, ValueMap{{"prop.name1", test::make_int_prop_value(3)}})); + timestamp, CreateDictionaryValue("{'prop': {'name1': 3}}"))); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - timestamp + time_delta, - ValueMap{{"prop.name1", test::make_int_prop_value(4)}})); + timestamp + time_delta, CreateDictionaryValue("{'prop': {'name1': 4}}"))); auto changes = queue_->GetAndClearRecordedStateChanges(); EXPECT_EQ(4, queue_->GetLastStateChangeId()); ASSERT_EQ(2, changes.size()); - ValueMap expected1{ - {"prop.name1", test::make_int_prop_value(3)}, - {"prop.name2", test::make_int_prop_value(2)}, - }; - ValueMap expected2{ - {"prop.name1", test::make_int_prop_value(4)}, - }; + const std::string expected1 = "{'prop': {'name1': 3, 'name2': 2}}"; + const std::string expected2 = "{'prop': {'name1': 4}}"; EXPECT_EQ(timestamp, changes[0].timestamp); - EXPECT_EQ(expected1, changes[0].changed_properties); + EXPECT_JSON_EQ(expected1, *changes[0].changed_properties); EXPECT_EQ(timestamp + time_delta, changes[1].timestamp); - EXPECT_EQ(expected2, changes[1].changed_properties); + EXPECT_JSON_EQ(expected2, *changes[1].changed_properties); } TEST_F(StateChangeQueueTest, MaxQueueSize) { @@ -109,43 +101,29 @@ base::TimeDelta time_delta2 = base::TimeDelta::FromMinutes(3); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - start_time, ValueMap{ - {"prop.name1", test::make_int_prop_value(1)}, - {"prop.name2", test::make_int_prop_value(2)}, - })); + start_time, CreateDictionaryValue("{'prop': {'name1': 1, 'name2': 2}}"))); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( start_time + time_delta1, - ValueMap{ - {"prop.name1", test::make_int_prop_value(3)}, - {"prop.name3", test::make_int_prop_value(4)}, - })); + CreateDictionaryValue("{'prop': {'name1': 3, 'name3': 4}}"))); ASSERT_TRUE(queue_->NotifyPropertiesUpdated( start_time + time_delta2, - ValueMap{ - {"prop.name10", test::make_int_prop_value(10)}, - {"prop.name11", test::make_int_prop_value(11)}, - })); + CreateDictionaryValue("{'prop': {'name10': 10, 'name11': 11}}"))); EXPECT_EQ(3, queue_->GetLastStateChangeId()); auto changes = queue_->GetAndClearRecordedStateChanges(); ASSERT_EQ(2, changes.size()); - ValueMap expected1{ - {"prop.name1", test::make_int_prop_value(3)}, - {"prop.name2", test::make_int_prop_value(2)}, - {"prop.name3", test::make_int_prop_value(4)}, - }; + const std::string expected1 = + "{'prop': {'name1': 3, 'name2': 2, 'name3': 4}}"; EXPECT_EQ(start_time + time_delta1, changes[0].timestamp); - EXPECT_EQ(expected1, changes[0].changed_properties); + EXPECT_JSON_EQ(expected1, *changes[0].changed_properties); - ValueMap expected2{ - {"prop.name10", test::make_int_prop_value(10)}, - {"prop.name11", test::make_int_prop_value(11)}, - }; + const std::string expected2 = + "{'prop': {'name10': 10, 'name11': 11}}"; EXPECT_EQ(start_time + time_delta2, changes[1].timestamp); - EXPECT_EQ(expected2, changes[1].changed_properties); + EXPECT_JSON_EQ(expected2, *changes[1].changed_properties); } TEST_F(StateChangeQueueTest, ImmediateStateChangeNotification) { @@ -161,10 +139,8 @@ TEST_F(StateChangeQueueTest, DelayedStateChangeNotification) { // When queue is not empty, registering a new callback will not trigger it. ASSERT_TRUE(queue_->NotifyPropertiesUpdated( - base::Time::Now(), ValueMap{ - {"prop.name1", test::make_int_prop_value(1)}, - {"prop.name2", test::make_int_prop_value(2)}, - })); + base::Time::Now(), + CreateDictionaryValue("{'prop': {'name1': 1, 'name3': 2}}"))); auto callback = [](StateChangeQueueInterface::UpdateID id) { FAIL() << "This should not be called";
diff --git a/src/states/state_manager.cc b/src/states/state_manager.cc index fe6e669..a424588 100644 --- a/src/states/state_manager.cc +++ b/src/states/state_manager.cc
@@ -93,8 +93,9 @@ if (!package->SetPropertyValue(property_name, value, error)) return false; - ValueMap prop_set{{full_property_name, package->GetProperty(property_name)}}; - state_change_queue_->NotifyPropertiesUpdated(timestamp, prop_set); + std::unique_ptr<base::DictionaryValue> prop_set{new base::DictionaryValue}; + prop_set->Set(full_property_name, value.DeepCopy()); + state_change_queue_->NotifyPropertiesUpdated(timestamp, std::move(prop_set)); return true; }
diff --git a/src/states/state_manager_unittest.cc b/src/states/state_manager_unittest.cc index 3f854fe..933e11b 100644 --- a/src/states/state_manager_unittest.cc +++ b/src/states/state_manager_unittest.cc
@@ -22,17 +22,18 @@ using testing::_; using testing::Return; +using testing::ReturnRef; using test::CreateDictionaryValue; namespace { const char kBaseDefinition[] = R"({ "base": { - "manufacturer":"string", - "serialNumber":"string" + "manufacturer":{"type":"string"}, + "serialNumber":{"type":"string"} }, "device": { - "state_property":"string" + "state_property":{"type":"string"} } })"; @@ -51,6 +52,10 @@ return CreateDictionaryValue(kBaseDefaults); } +MATCHER_P(IsState, str, "") { + return arg.Equals(CreateDictionaryValue(str).get()); +} + } // anonymous namespace class StateManagerTest : public ::testing::Test { @@ -58,9 +63,9 @@ void SetUp() override { // Initial expectations. EXPECT_CALL(mock_state_change_queue_, IsEmpty()).Times(0); - EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(_, _)) + EXPECT_CALL(mock_state_change_queue_, MockNotifyPropertiesUpdated(_, _)) .WillRepeatedly(Return(true)); - EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges()) + EXPECT_CALL(mock_state_change_queue_, MockGetAndClearRecordedStateChanges()) .Times(0); mgr_.reset(new StateManager(&mock_state_change_queue_)); @@ -102,9 +107,7 @@ 'manufacturer': 'Test Factory', 'serialNumber': 'Test Model' }, - 'device': { - 'state_property': '' - } + 'device': {} })"; EXPECT_JSON_EQ(expected, *mgr_->GetState()); } @@ -122,12 +125,8 @@ 'manufacturer': 'Test Factory', 'serialNumber': 'Test Model' }, - 'power': { - 'battery_level': 0 - }, - 'device': { - 'state_property': '' - } + 'power': {}, + 'device': {} })"; EXPECT_JSON_EQ(expected, *mgr_->GetState()); } @@ -137,12 +136,15 @@ auto state_definition = R"({ "base": { - "firmwareVersion": "string", - "localDiscoveryEnabled": "boolean", - "localAnonymousAccessMaxRole": [ "none", "viewer", "user" ], - "localPairingEnabled": "boolean" + "firmwareVersion": {"type":"string"}, + "localDiscoveryEnabled": {"type":"boolean"}, + "localAnonymousAccessMaxRole": { + "type": "string", + "enum": ["none", "viewer", "user"] + }, + "localPairingEnabled": {"type":"boolean"} }, - "power": {"battery_level":"integer"} + "power": {"battery_level":{"type":"integer"}} })"; ASSERT_TRUE(manager.LoadStateDefinitionFromJson(state_definition, nullptr)); @@ -172,11 +174,9 @@ } TEST_F(StateManagerTest, SetPropertyValue) { - ValueMap expected_prop_set{ - {"device.state_property", test::make_string_prop_value("Test Value")}, - }; + const std::string state = "{'device': {'state_property': 'Test Value'}}"; EXPECT_CALL(mock_state_change_queue_, - NotifyPropertiesUpdated(timestamp_, expected_prop_set)) + MockNotifyPropertiesUpdated(timestamp_, IsState(state))) .WillOnce(Return(true)); ASSERT_TRUE(SetPropertyValue("device.state_property", base::StringValue{"Test Value"}, nullptr)); @@ -216,39 +216,36 @@ } TEST_F(StateManagerTest, SetPropertyValue_Error_UnknownProperty) { - ErrorPtr error; - ASSERT_FALSE( - SetPropertyValue("base.level", base::FundamentalValue{0}, &error)); - EXPECT_EQ(errors::state::kDomain, error->GetDomain()); - EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); + ASSERT_TRUE( + SetPropertyValue("base.level", base::FundamentalValue{0}, nullptr)); } TEST_F(StateManagerTest, GetAndClearRecordedStateChanges) { - EXPECT_CALL(mock_state_change_queue_, NotifyPropertiesUpdated(timestamp_, _)) + EXPECT_CALL(mock_state_change_queue_, + MockNotifyPropertiesUpdated(timestamp_, _)) .WillOnce(Return(true)); ASSERT_TRUE(SetPropertyValue("device.state_property", base::StringValue{"Test Value"}, nullptr)); - std::vector<StateChange> expected_val; - expected_val.emplace_back( - timestamp_, ValueMap{{"device.state_property", - test::make_string_prop_value("Test Value")}}); - EXPECT_CALL(mock_state_change_queue_, GetAndClearRecordedStateChanges()) - .WillOnce(Return(expected_val)); + std::vector<StateChange> expected_state; + const std::string expected_val = + "{'device': {'state_property': 'Test Value'}}"; + expected_state.emplace_back( + timestamp_, + CreateDictionaryValue(expected_val)); + EXPECT_CALL(mock_state_change_queue_, MockGetAndClearRecordedStateChanges()) + .WillOnce(ReturnRef(expected_state)); EXPECT_CALL(mock_state_change_queue_, GetLastStateChangeId()) .WillOnce(Return(0)); auto changes = mgr_->GetAndClearRecordedStateChanges(); ASSERT_EQ(1, changes.second.size()); - EXPECT_EQ(expected_val.back().timestamp, changes.second.back().timestamp); - EXPECT_EQ(expected_val.back().changed_properties, - changes.second.back().changed_properties); + EXPECT_EQ(timestamp_, changes.second.back().timestamp); + EXPECT_JSON_EQ(expected_val, *changes.second.back().changed_properties); } TEST_F(StateManagerTest, SetProperties) { - ValueMap expected_prop_set{ - {"base.manufacturer", test::make_string_prop_value("No Name")}, - }; + const std::string state = "{'base': {'manufacturer': 'No Name'}}"; EXPECT_CALL(mock_state_change_queue_, - NotifyPropertiesUpdated(_, expected_prop_set)) + MockNotifyPropertiesUpdated(_, IsState(state))) .WillOnce(Return(true)); EXPECT_CALL(*this, OnStateChanged()).Times(1); @@ -260,16 +257,14 @@ 'manufacturer': 'No Name', 'serialNumber': 'Test Model' }, - 'device': { - 'state_property': '' - } + 'device': {} })"; EXPECT_JSON_EQ(expected, *mgr_->GetState()); } TEST_F(StateManagerTest, GetProperty) { EXPECT_JSON_EQ("'Test Model'", *mgr_->GetProperty("base.serialNumber")); - EXPECT_JSON_EQ("''", *mgr_->GetProperty("device.state_property")); + EXPECT_EQ(nullptr, mgr_->GetProperty("device.state_property")); EXPECT_EQ(nullptr, mgr_->GetProperty("device.unknown")); EXPECT_EQ(nullptr, mgr_->GetProperty("unknown.state_property")); }
diff --git a/src/states/state_package.cc b/src/states/state_package.cc index 0b8e219..55650b6 100644 --- a/src/states/state_package.cc +++ b/src/states/state_package.cc
@@ -7,9 +7,6 @@ #include <base/logging.h> #include <base/values.h> -#include "src/commands/prop_types.h" -#include "src/commands/prop_values.h" -#include "src/commands/schema_utils.h" #include "src/states/error_codes.h" namespace weave { @@ -18,28 +15,18 @@ bool StatePackage::AddSchemaFromJson(const base::DictionaryValue* json, ErrorPtr* error) { - ObjectSchema schema; - if (!schema.FromJson(json, nullptr, error)) - return false; - // Scan first to make sure we have no property redefinitions. - for (const auto& pair : schema.GetProps()) { - if (types_.GetProp(pair.first)) { + for (base::DictionaryValue::Iterator it(*json); !it.IsAtEnd(); it.Advance()) { + if (types_.HasKey(it.key())) { Error::AddToPrintf(error, FROM_HERE, errors::state::kDomain, errors::state::kPropertyRedefinition, "State property '%s.%s' is already defined", - name_.c_str(), pair.first.c_str()); + name_.c_str(), it.key().c_str()); return false; } } - // Now move all the properties to |types_| object. - for (const auto& pair : schema.GetProps()) { - types_.AddProp(pair.first, pair.second->Clone()); - // Create default value for this state property. - values_.emplace(pair.first, pair.second->CreateDefaultValue()); - } - + types_.MergeDictionary(json); return true; } @@ -53,20 +40,14 @@ } std::unique_ptr<base::DictionaryValue> StatePackage::GetValuesAsJson() const { - std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue); - for (const auto& pair : values_) { - auto value = pair.second->ToJson(); - CHECK(value); - dict->SetWithoutPathExpansion(pair.first, value.release()); - } - return dict; + return std::unique_ptr<base::DictionaryValue>(values_.DeepCopy()); } std::unique_ptr<base::Value> StatePackage::GetPropertyValue( const std::string& property_name, ErrorPtr* error) const { - auto it = values_.find(property_name); - if (it == values_.end()) { + const base::Value* value = nullptr; + if (!values_.Get(property_name, &value)) { Error::AddToPrintf(error, FROM_HERE, errors::state::kDomain, errors::state::kPropertyNotDefined, "State property '%s.%s' is not defined", name_.c_str(), @@ -74,24 +55,13 @@ return nullptr; } - return it->second->ToJson(); + return std::unique_ptr<base::Value>(value->DeepCopy()); } bool StatePackage::SetPropertyValue(const std::string& property_name, const base::Value& value, ErrorPtr* error) { - auto it = values_.find(property_name); - if (it == values_.end()) { - Error::AddToPrintf(error, FROM_HERE, errors::state::kDomain, - errors::state::kPropertyNotDefined, - "State property '%s.%s' is not defined", name_.c_str(), - property_name.c_str()); - return false; - } - auto new_value = it->second->GetPropType()->CreatePropValue(value, error); - if (!new_value) - return false; - it->second = std::move(new_value); + values_.Set(property_name, value.DeepCopy()); return true; }
diff --git a/src/states/state_package.h b/src/states/state_package.h index e8e3964..7632145 100644 --- a/src/states/state_package.h +++ b/src/states/state_package.h
@@ -10,15 +10,9 @@ #include <string> #include <base/macros.h> +#include <base/values.h> #include <weave/error.h> -#include "src/commands/object_schema.h" -#include "src/commands/prop_values.h" - -namespace base { -class DictionaryValue; -} // namespace base - namespace weave { // A package is a set of related state properties. GCD specification defines @@ -62,20 +56,13 @@ const base::Value& value, ErrorPtr* error); - std::shared_ptr<const PropValue> GetProperty( - const std::string& property_name) const { - auto it = values_.find(property_name); - return it != values_.end() ? it->second - : std::shared_ptr<const PropValue>{}; - } - // Returns the name of the this package. const std::string& GetName() const { return name_; } private: std::string name_; - ObjectSchema types_; - ValueMap values_; + base::DictionaryValue types_; + base::DictionaryValue values_; friend class StatePackageTestHelper; DISALLOW_COPY_AND_ASSIGN(StatePackage);
diff --git a/src/states/state_package_unittest.cc b/src/states/state_package_unittest.cc index f958bf5..5e1b933 100644 --- a/src/states/state_package_unittest.cc +++ b/src/states/state_package_unittest.cc
@@ -21,11 +21,11 @@ class StatePackageTestHelper { public: // Returns the state property definitions (types/constraints/etc). - static const ObjectSchema& GetTypes(const StatePackage& package) { + static const base::DictionaryValue& GetTypes(const StatePackage& package) { return package.types_; } // Returns the all state property values in this package. - static const ValueMap& GetValues(const StatePackage& package) { + static const base::DictionaryValue& GetValues(const StatePackage& package) { return package.values_; } }; @@ -33,15 +33,30 @@ namespace { std::unique_ptr<base::DictionaryValue> GetTestSchema() { return CreateDictionaryValue(R"({ - 'light': 'boolean', - 'color': 'string', - 'direction':{ - 'properties':{ - 'azimuth': {'type': 'number', 'isRequired': true}, - 'altitude': {'maximum': 90.0} - } + 'color': { + 'type': 'string' }, - 'iso': [50, 100, 200, 400] + 'direction': { + 'additionalProperties': false, + 'properties': { + 'altitude': { + 'maximum': 90.0, + 'type': 'number' + }, + 'azimuth': { + 'type': 'number' + } + }, + 'type': 'object', + 'required': [ 'azimuth' ] + }, + 'iso': { + 'enum': [50, 100, 200, 400], + 'type': 'integer' + }, + 'light': { + 'type': 'boolean' + } })"); } @@ -54,11 +69,11 @@ })"); } -inline const ObjectSchema& GetTypes(const StatePackage& package) { +inline const base::DictionaryValue& GetTypes(const StatePackage& package) { return StatePackageTestHelper::GetTypes(package); } // Returns the all state property values in this package. -inline const ValueMap& GetValues(const StatePackage& package) { +inline const base::DictionaryValue& GetValues(const StatePackage& package) { return StatePackageTestHelper::GetValues(package); } @@ -78,15 +93,15 @@ TEST(StatePackage, Empty) { StatePackage package("test"); EXPECT_EQ("test", package.GetName()); - EXPECT_TRUE(GetTypes(package).GetProps().empty()); + EXPECT_TRUE(GetTypes(package).empty()); EXPECT_TRUE(GetValues(package).empty()); } TEST(StatePackage, AddSchemaFromJson_OnEmpty) { StatePackage package("test"); ASSERT_TRUE(package.AddSchemaFromJson(GetTestSchema().get(), nullptr)); - EXPECT_EQ(4, GetTypes(package).GetProps().size()); - EXPECT_EQ(4, GetValues(package).size()); + EXPECT_EQ(4, GetTypes(package).size()); + EXPECT_EQ(0, GetValues(package).size()); auto expected = R"({ 'color': { @@ -114,15 +129,9 @@ 'type': 'boolean' } })"; - EXPECT_JSON_EQ(expected, *GetTypes(package).ToJson(true, false)); + EXPECT_JSON_EQ(expected, GetTypes(package)); - expected = R"({ - 'color': '', - 'direction': {}, - 'iso': 0, - 'light': false - })"; - EXPECT_JSON_EQ(expected, *package.GetValuesAsJson()); + EXPECT_JSON_EQ("{}", *package.GetValuesAsJson()); } TEST(StatePackage, AddValuesFromJson_OnEmpty) { @@ -143,10 +152,13 @@ } TEST_F(StatePackageTest, AddSchemaFromJson_AddMore) { - auto dict = CreateDictionaryValue("{'brightness':['low', 'medium', 'high']}"); + auto dict = CreateDictionaryValue(R"({'brightness':{ + 'enum': ['low', 'medium', 'high'], + 'type': 'string' + }})"); ASSERT_TRUE(package_->AddSchemaFromJson(dict.get(), nullptr)); - EXPECT_EQ(5, GetTypes(*package_).GetProps().size()); - EXPECT_EQ(5, GetValues(*package_).size()); + EXPECT_EQ(5, GetTypes(*package_).size()); + EXPECT_EQ(4, GetValues(*package_).size()); auto expected = R"({ 'brightness': { 'enum': ['low', 'medium', 'high'], @@ -177,10 +189,9 @@ 'type': 'boolean' } })"; - EXPECT_JSON_EQ(expected, *GetTypes(*package_).ToJson(true, false)); + EXPECT_JSON_EQ(expected, GetTypes(*package_)); expected = R"({ - 'brightness': '', 'color': 'white', 'direction': { 'altitude': 89.9, @@ -193,7 +204,10 @@ } TEST_F(StatePackageTest, AddValuesFromJson_AddMore) { - auto dict = CreateDictionaryValue("{'brightness':['low', 'medium', 'high']}"); + auto dict = CreateDictionaryValue(R"({'brightness':{ + 'enum': ['low', 'medium', 'high'], + 'type': 'string' + }})"); ASSERT_TRUE(package_->AddSchemaFromJson(dict.get(), nullptr)); dict = CreateDictionaryValue("{'brightness':'medium'}"); ASSERT_TRUE(package_->AddValuesFromJson(dict.get(), nullptr)); @@ -212,7 +226,8 @@ } TEST_F(StatePackageTest, AddSchemaFromJson_Error_Redefined) { - auto dict = CreateDictionaryValue("{'color':['white', 'blue', 'red']}"); + auto dict = CreateDictionaryValue(R"({'color': + {'type':'string', 'enum':['white', 'blue', 'red']}})"); ErrorPtr error; EXPECT_FALSE(package_->AddSchemaFromJson(dict.get(), &error)); EXPECT_EQ(errors::state::kDomain, error->GetDomain()); @@ -221,10 +236,7 @@ TEST_F(StatePackageTest, AddValuesFromJson_Error_Undefined) { auto dict = CreateDictionaryValue("{'brightness':'medium'}"); - ErrorPtr error; - EXPECT_FALSE(package_->AddValuesFromJson(dict.get(), &error)); - EXPECT_EQ(errors::state::kDomain, error->GetDomain()); - EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); + EXPECT_TRUE(package_->AddValuesFromJson(dict.get(), nullptr)); } TEST_F(StatePackageTest, GetPropertyValue) { @@ -274,85 +286,4 @@ EXPECT_JSON_EQ(expected, *package_->GetValuesAsJson()); } -TEST_F(StatePackageTest, SetPropertyValue_Error_TypeMismatch) { - ErrorPtr error; - ASSERT_FALSE( - package_->SetPropertyValue("color", base::FundamentalValue{12}, &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kTypeMismatch, error->GetCode()); - error.reset(); - - ASSERT_FALSE( - package_->SetPropertyValue("iso", base::FundamentalValue{false}, &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kTypeMismatch, error->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_OutOfRange) { - ErrorPtr error; - ASSERT_FALSE( - package_->SetPropertyValue("iso", base::FundamentalValue{150}, &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kOutOfRange, error->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_Object_TypeMismatch) { - ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue( - "direction", - *CreateDictionaryValue("{'altitude': 45.0, 'azimuth': '15'}"), &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kInvalidPropValue, error->GetCode()); - const Error* inner = error->GetInnerError(); - EXPECT_EQ(errors::commands::kDomain, inner->GetDomain()); - EXPECT_EQ(errors::commands::kTypeMismatch, inner->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_Object_OutOfRange) { - ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue( - "direction", - *CreateDictionaryValue("{'altitude': 100.0, 'azimuth': 290.0}"), &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kInvalidPropValue, error->GetCode()); - const Error* inner = error->GetInnerError(); - EXPECT_EQ(errors::commands::kDomain, inner->GetDomain()); - EXPECT_EQ(errors::commands::kOutOfRange, inner->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_Object_UnknownProperty) { - ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue( - "direction", *CreateDictionaryValue( - "{'altitude': 10.0, 'azimuth': 20.0, 'spin': 30.0}"), - &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kUnknownProperty, error->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Object_OptionalProperty) { - EXPECT_JSON_EQ("{'altitude': 89.9, 'azimuth': 57.2957795}", - *package_->GetProperty("direction")->ToJson()); - ASSERT_TRUE(package_->SetPropertyValue( - "direction", *CreateDictionaryValue("{'azimuth': 10.0}"), nullptr)); - EXPECT_JSON_EQ("{'azimuth': 10.0}", - *package_->GetProperty("direction")->ToJson()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_Object_MissingProperty) { - ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue( - "direction", *CreateDictionaryValue("{'altitude': 10.0}"), &error)); - EXPECT_EQ(errors::commands::kDomain, error->GetDomain()); - EXPECT_EQ(errors::commands::kPropertyMissing, error->GetCode()); -} - -TEST_F(StatePackageTest, SetPropertyValue_Error_Unknown) { - ErrorPtr error; - ASSERT_FALSE(package_->SetPropertyValue("volume", base::FundamentalValue{100}, - &error)); - EXPECT_EQ(errors::state::kDomain, error->GetDomain()); - EXPECT_EQ(errors::state::kPropertyNotDefined, error->GetCode()); -} - } // namespace weave
diff --git a/src/weave_unittest.cc b/src/weave_unittest.cc index c2f1e8f..2e41852 100644 --- a/src/weave_unittest.cc +++ b/src/weave_unittest.cc
@@ -72,7 +72,7 @@ "base": { "reboot": { "minimalRole": "user", - "parameters": {"delay": "integer"}, + "parameters": {"delay": {"type": "integer"}}, "results": {} }, "shutdown": {